Split pkg/workflow/compiler_types.go by lifecycle: options, mutators, types - #52109
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Splits compiler construction, runtime mutation, and type declarations by lifecycle without intended behavior changes.
Changes:
- Moves functional options and construction into
compiler_options.go. - Moves compiler mutators, accessors, and shared caches into
compiler_mutators.go. - Adds focused tests and updates symbol documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/README.md |
Updates relocated symbol paths. |
pkg/workflow/compiler_types.go |
Retains compiler types and state declarations. |
pkg/workflow/compiler_options.go |
Houses options and compiler construction. |
pkg/workflow/compiler_options_test.go |
Tests options and defaults. |
pkg/workflow/compiler_mutators.go |
Houses runtime methods and cache helpers. |
pkg/workflow/compiler_mutators_test.go |
Tests representative mutator and cache behavior. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| // Create compiler with defaults | ||
| c := &Compiler{ | ||
| ctx: context.Background(), // Default context; override with WithContext |
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR is a pure mechanical file split (compiler_types.go into compiler_options.go, compiler_mutators.go, compiler_types.go) with no new abstractions, wrappers, or behavior changes introduced. The only duplicate-looking pair (effectiveActionsRepo/EffectiveActionsRepo) predates this PR and was just relocated. Nothing new to cut under ponytail-review scope.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a clean, mechanical file split with no logic changes. compiler_types.go now contains only types, compiler_mutators.go contains setter/getter methods, and compiler_options.go contains functional options and NewCompiler. Test coverage for the moved code is added in compiler_mutators_test.go and compiler_options_test.go. No issues found in the changed lines.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 42.6 AIC · ⌖ 6.21 AIC · ⊞ 5.4K
|
Great work on this refactoring! 🎯 This PR successfully splits a monolithic 900+ line file into three focused modules by lifecycle concern — a textbook example of separation of concerns. The diff is clean, mechanical, and includes comprehensive test coverage for both new files. ✅ Aligned with guidelines — The PR follows the project's core contribution model: this work was scoped in issue #52092 (generated by Deep Report), and the Copilot coding agent has delivered a focused, well-structured refactor that improves navigability and reduces merge-conflict surface in a high-churn file. ✅ Well-organized split — Three new/modified files with clear responsibilities:
✅ Full test coverage — Two new test files with meaningful coverage. ✅ Documentation updated — pkg/workflow/README.md symbol table correctly reflects the relocated 38 symbols with proper file assignments. No actionable changes needed — this is ready for review and merge.
|
Test Quality Sentinel 🧪SummaryScore: 90/100 ✅ Excellent PR adds 2 new test files (6 test functions, 119 lines) for the refactored Test Coverage AnalysisNew Test Functions Analyzed: 6
Quality Signals
Implementation vs. DesignImplementation tests / total: 0/6 = 0% (threshold: ≤30%) ✅ All tests verify design contracts (functional option correctness, state invariants, locking mechanism, singleton pattern, edge cases). No tests verify implementation details only. RecommendationsNone — test coverage is strong for the refactored code. Setter/getter pairs and functional options are well-exercised.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking issues, but four actionable improvements flagged as inline comments.
📋 Key Themes & Highlights
Key Themes
- Test structure:
TestCompilerMutatorsbundles too many behaviours —t.Runsub-tests would make specs readable and failures pinpointable. - Dead code:
actionModeis set twice inNewCompiler; the struct-literal assignment is always overwritten after the options loop. - Naming:
getSharedActionResolver()returns two values but its name only describes one — callers need to read the implementation to understand the full contract. - Doc gap:
NewCompiler's doc comment lists only 4 of 7With*options and doesn't articulate theWith*vsSet*lifecycle distinction.
Positive Highlights
- ✅ Clean lifecycle split — options / mutators / types is a meaningful and navigable boundary.
- ✅ Good test coverage for non-obvious semantics: slug-locking no-op, shared-cache instance reuse, nil-manifest reset.
- ✅ Orphaned
SkipIfMatchConfigdoc comment correctly removed. - ✅ README symbol table kept consistent with the new file names.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 43.3 AIC · ⌖ 6.88 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_mutators_test.go:474
[/tdd] TestCompilerMutators bundles ~10 independent behaviours into a single function — one failure masks the rest and the test name doesn't read as a specification.
<details>
<summary>💡 Suggested structure</summary>
Split into focused sub-tests:
func TestCompilerMutators(t *testing.T) {
t.Run("SetNoEmit toggles flag", func(t *testing.T) {
c := NewCompiler()
c.SetNoEmit(true)
assert.True(t, c.noEmit)
})
t.Run("WarningCounter increments and res…
</details>
<details><summary>pkg/workflow/compiler_options.go:603</summary>
**[/codebase-design]** `actionMode` is assigned twice in `NewCompiler`: once in the struct literal (line 582) and again unconditionally at line 603. The first assignment is dead code.
<details>
<summary>💡 Fix</summary>
Remove the `actionMode` field from the struct literal:
```go
// Before (struct literal):
actionMode: DetectActionMode(version), // redundant — overwritten below
// After applying options, the single assignment suffices:
c.actionMode = DetectActionMode(c.version)The cur…
pkg/workflow/compiler_mutators.go:395
[/codebase-design] getSharedActionResolver() returns both the cache and the resolver, but its name only mentions the resolver — a caller reading the signature has to look inside the function to discover it also initialises the cache.
<details>
<summary>💡 Suggestion</summary>
Rename to initSharedActionCacheAndResolver() or introduce separate lazy-init helpers so each accessor (GetSharedActionCache, GetSharedActionResolver) is independently readable. The current dual-return private…
pkg/workflow/compiler_options.go:565
[/codebase-design] The NewCompiler doc comment says "Common options: WithVerbose, WithEngineOverride, WithNoEmit, WithSkipValidation" but the file defines 7 With* options — WithFailFast, WithWorkflowIdentifier, and WithVersion are omitted from the example list. There's also no guidance on when callers should prefer With* (construction time) vs Set* (post-construction), leaving the dual-API surface undocumented.
<details>
<summary>💡 Suggestion</summary>
Update the doc commen…
…ecycle Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (+579 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot Please address the remaining PR follow-ups in one pass, then refresh the branch and use the pr-finisher skill before handing back. Open review feedback to consider:
Please also link the ADR already committed at
|
…ed helper Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 6be1a63 (pushed via report_progress):
|
PR Triage: #52109
Automated triage — see labels for machine-readable classification.
|
compiler_types.gomixed three lifecycles in one file: build-time functional options, post-construction runtime mutators/accessors, and plain type declarations. Mechanical split, no behavior change.File split
compiler_options.go—CompilerOptiontype,With*builders, andNewCompiler.compiler_mutators.go— all*Compilersetters/getters (SetContext,SetStrictMode,Get*, warning counters, repository-slug locking) plus the lazily-initialized shared cache helpers (getSharedActionResolver,getSharedImportCache).compiler_types.go— now onlylogTypes,FileCreationTracker, theCompilerstruct, andallowedDomain. Also drops an orphaned doc comment forSkipIfMatchConfig, whose type actually lives inworkflow_data.go.Tests
The existing
compiler_types_test.goonly coversWorkflowDatapin context, so there was nothing to relocate. Added direct coverage alongside the new files instead:compiler_options_test.go— every option is applied byNewCompiler; defaults are unchanged.compiler_mutators_test.go— mutators, repository-slug lock semantics (SetRepositorySlugIfUnlockedis a no-op once locked), shared cache/resolver instance reuse, andSetPriorManifests(nil)resetting to an empty map.Docs
pkg/workflow/README.mdsymbol table: the 38 relocated symbols now point atcompiler_mutators.go, kept in the table's existing file-name ordering.pr-sous-chef branch refresh requested.> Generated by 👨🍳 PR Sous Chef · gpt54 · 9.67 AIC · ⌖ 6.25 AIC · ⊞ 8.5K · ◷