refactor: split compiler_yaml.go (1262 lines) into focused modules#45494
Conversation
Extract distinct functional domains from the 1262-line compiler_yaml.go into 4 new focused files: - compiler_yaml_normalize.go: YAML text normalization utilities (normalizeBlankLines, countLeadingSpaces, blockScalarHeaderIndentForLine) - compiler_yaml_header.go: YAML header/metadata emission (generateWorkflowHeader, effectiveStrictMode, effectiveSafeUpdate) - compiler_yaml_prompt.go: Prompt assembly and chunking (generatePrompt, splitContentIntoChunks, extractPromptChunksFromMarkdown, writePromptBashStep, resolveWorkspaceRoot) - compiler_yaml_steps.go: Individual step generation (generatePreSteps, generatePostSteps, generatePreAgentSteps, writeStepsSection, generateCreateAwInfo, generateOutputCollectionStep) compiler_yaml.go trimmed to top-level orchestration only: (buildJobsAndValidate, generateYAML, generateWorkflowBody) All tests pass. No behavior changes. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Splits 1,262-line
|
🤖 PR Triage — Run §29379921158
Score breakdown: impact 25 + urgency 12 + quality 23 Rationale: Splits
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Refactors the YAML compiler into focused modules while preserving existing behavior and package-level APIs.
Changes:
- Retains orchestration in
compiler_yaml.go. - Extracts header, normalization, prompt, and step generation logic into dedicated files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_yaml.go |
Keeps core YAML compilation orchestration. |
pkg/workflow/compiler_yaml_header.go |
Contains header and strict-mode logic. |
pkg/workflow/compiler_yaml_normalize.go |
Contains YAML whitespace normalization. |
pkg/workflow/compiler_yaml_prompt.go |
Contains prompt assembly and import handling. |
pkg/workflow/compiler_yaml_steps.go |
Contains workflow step generation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Medium
There was a problem hiding this comment.
Clean mechanical split — no logic changes, imports correctly distributed, shared compilerYamlLog logger works fine across same-package files. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 31 AIC · ⌖ 4.93 AIC · ⊞ 5K
There was a problem hiding this comment.
No issues found. Clean mechanical file-boundary split — all extracted functions are verbatim copies in the same package with correct per-file imports. No behavior changes, no visibility changes, no import cycles.
🔎 Code quality review by PR Code Quality Reviewer · 72.7 AIC · ⌖ 4.26 AIC · ⊞ 5.6K
Comment /review to run again
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,071 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 MatterADRs 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — observations only, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Misplaced policy methods:
effectiveStrictMode/effectiveSafeUpdatelive in_header.gobut are called from_steps.goandcompiler.go. A cross-cutting policy method in a domain-named file misleads readers about where to look. - Invisible shared logger:
compilerYamlLogis declared incompiler_yaml.goand referenced across all new modules. For a refactor whose goal is focused modules, this is harmless today but creates hidden coupling that should be addressed to complete the decomposition. - Naming collision:
compiler_yaml_steps.goclashes with the existingcompiler_yaml_step_*.gonaming pattern already present in the package.
Positive Highlights
- ✅ Clear, well-motivated split — 1262 lines → 5 focused files, zero behavior changes
- ✅ The PR description is excellent: table of responsibilities makes the intent obvious
- ✅
compiler_yaml_normalize.gois a clean self-contained module with no cross-dependencies - ✅ All existing tests continue to pass through the same public signatures
- ✅ The diff is mechanically safe — only file boundaries changed, no logic moved
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 59.6 AIC · ⌖ 4.72 AIC · ⊞ 6.7K
Comment /matt to run again
| // Priority: CLI flag (c.strictMode) > frontmatter strict field > default (true). | ||
| // This should be used when emitting metadata/env vars to correctly reflect the | ||
| // workflow's strictness as inferred from the source (frontmatter). | ||
| func (c *Compiler) effectiveStrictMode(frontmatter map[string]any) bool { |
There was a problem hiding this comment.
[/codebase-design] effectiveStrictMode and effectiveSafeUpdate are policy methods used by compiler_yaml_steps.go and compiler.go, not just the header — placing them in _header.go creates a semantically misleading home.
💡 Suggestion
Move these two methods into a dedicated compiler_yaml_policy.go (or keep them in compiler_yaml.go as part of the orchestration layer). Their placement in the header file implies they are header-specific, but callers span at least three files:
compiler_yaml_header.go(line 74)compiler_yaml_steps.go(line 174)compiler.go(line 459)
A module whose name implies one responsibility (_header) but hosts cross-cutting policy methods is a depth-design smell: the interface no longer matches the implementation.
@copilot please address this.
There was a problem hiding this comment.
Moved effectiveStrictMode and effectiveSafeUpdate to a new compiler_yaml_policy.go. They're no longer in _header.go — the policy file is a neutral home for cross-cutting compiler policy that is consumed by the header, step-lifecycle, and compiler.go layers alike. Committed in 94deb83.
| "github.com/github/gh-aw/pkg/workflow/compilerenv" | ||
| ) | ||
|
|
||
| var compilerYamlLog = logger.New("workflow:compiler_yaml") |
There was a problem hiding this comment.
[/codebase-design] compilerYamlLog is declared in compiler_yaml.go but is used across the three new files (_prompt.go, _steps.go, _header.go) via implicit package-level sharing. This invisible dependency makes each extracted module harder to understand in isolation.
💡 Suggestion
Each extracted file should either:
- Declare its own logger scoped to its domain (e.g.
var compilerYamlPromptLog = logger.New("workflow:compiler_yaml:prompt")), or - Accept a logger through a parameter/interface if shared state is intentional.
Using a single package-level logger across 5 files silently couples them and makes it impossible to tune log verbosity per domain. Option 1 aligns better with the stated goal of focused modules.
@copilot please address this.
There was a problem hiding this comment.
Each extracted file now declares its own domain-scoped logger: compilerYamlHeaderLog (workflow:compiler_yaml:header), compilerYamlPromptLog (workflow:compiler_yaml:prompt), and compilerYamlStepLifecycleLog (workflow:compiler_yaml:steps). The orchestration file keeps compilerYamlLog. All call sites in the three extracted files were updated to use the local logger. Committed in 94deb83.
| "github.com/github/gh-aw/pkg/workflow/compilerenv" | ||
| ) | ||
|
|
||
| func (c *Compiler) generatePreSteps(yaml *strings.Builder, data *WorkflowData) { |
There was a problem hiding this comment.
[/codebase-design] The new compiler_yaml_steps.go conflicts with the naming pattern of compiler_yaml_step_generation.go and compiler_yaml_step_conversion.go. Plural _steps vs singular _step_* is ambiguous about each file's scope.
💡 Suggestion
Consider renaming to follow the existing compiler_yaml_step_ prefix, e.g. compiler_yaml_step_lifecycle.go (pre/post/pre-agent hooks) or splitting into the appropriate existing files. This avoids confusing rg "compiler_yaml_step" navigation.
@copilot please address this.
There was a problem hiding this comment.
Renamed compiler_yaml_steps.go → compiler_yaml_step_lifecycle.go so it follows the established compiler_yaml_step_ prefix used by compiler_yaml_step_generation.go and compiler_yaml_step_conversion.go. This keeps rg "compiler_yaml_step" navigation consistent. Committed in 94deb83.
|
@copilot please run the Unresolved review threads (newest first):
Run: https://github.com/github/gh-aw/actions/runs/29389971239
|
- Move effectiveStrictMode/effectiveSafeUpdate to compiler_yaml_policy.go
(cross-cutting policy used by header, steps, and compiler.go should
not live in _header.go which implies header-specific scope)
- Add per-domain loggers to compiler_yaml_header.go, compiler_yaml_prompt.go,
and compiler_yaml_step_lifecycle.go (workflow:compiler_yaml:{header,prompt,steps})
so each module can be tuned independently
- Rename compiler_yaml_steps.go -> compiler_yaml_step_lifecycle.go to follow
the existing compiler_yaml_step_* naming convention used by
compiler_yaml_step_generation.go and compiler_yaml_step_conversion.go
- Update ADR-45494 status Draft->Accepted and reflect the above three design
decisions with rationale
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…rify ADR file list Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
pkg/workflow/compiler_yaml.gohad grown to 1262 lines across 5 unrelated functional domains, exceeding the 800-line threshold and making the file hard to navigate and maintain.Changes
Extracted into 4 new files, leaving
compiler_yaml.goas a thin orchestration layer:compiler_yaml.gobuildJobsAndValidate,generateYAML,generateWorkflowBodycompiler_yaml_normalize.gocompiler_yaml_header.gogenerateWorkflowHeader,effectiveStrictMode,effectiveSafeUpdatecompiler_yaml_prompt.gocompiler_yaml_steps.gogenerateCreateAwInfo, output collectionAll functions remain in the same package with identical signatures — purely a file-boundary split with no behavior changes.