refactor(workflow): break up largefunc violations in maintenance/compiler YAML builders#46335
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hi Also, for a structural refactor of this size (2970 lines), there are no test file changes in the diff — a follow-up plan should explicitly cover validation. A core team member can then implement or sponsor the change through the project's normal agentic workflow.
|
There was a problem hiding this comment.
Pull request overview
Refactors oversized workflow YAML generators into focused helpers while preserving public interfaces.
Changes:
- Splits maintenance workflow generation into section builders.
- Extracts prompt, header, lifecycle, and output helpers.
- Refreshes action-version annotations in four generated lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/maintenance_workflow_yaml.go |
Converts maintenance generation into orchestration. |
pkg/workflow/maintenance_workflow_yaml_sections.go |
Adds maintenance section templates and builders. |
pkg/workflow/compiler_yaml_step_lifecycle.go |
Extracts lifecycle parameter and output helpers. |
pkg/workflow/compiler_yaml_prompt.go |
Splits prompt import and expression processing. |
pkg/workflow/compiler_yaml_header.go |
Extracts workflow header writers. |
.github/workflows/skillet.lock.yml |
Refreshes generated action annotations. |
.github/workflows/release.lock.yml |
Refreshes generated setup-go annotation. |
.github/workflows/hourly-ci-cleaner.lock.yml |
Refreshes generated checkout annotations. |
.github/workflows/avenger.lock.yml |
Refreshes generated checkout annotations. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 5
- Review effort level: Medium
| if len(data.PromptImports) > 0 { | ||
| return buildOrderedPromptImportChunks(c, data) |
| GH_AW_INFO_ENGINE_ID: "copilot" | ||
| - name: Checkout skills directory | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # 9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
| RELEASE_TAG: ${{ needs.config.outputs.release_tag }} | ||
| - name: Setup Go | ||
| uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # b7ad1dad31e06c5925ef5d2fc7ad053ef454303e | ||
| uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 |
| echo "GH_HOST=${GH_HOST}" >> "$GITHUB_ENV" | ||
| - name: Checkout repository | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # 9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
| echo "GH_HOST=${GH_HOST}" >> "$GITHUB_ENV" | ||
| - name: Checkout repository | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # 9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #46335 contains only production code refactoring of workflow YAML builders (compiler_yaml_header.go, compiler_yaml_prompt.go, compiler_yaml_step_lifecycle.go, maintenance_workflow_yaml.go, and new maintenance_workflow_yaml_sections.go). Test Quality Sentinel analysis skipped. |
…w YAML builders Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,387 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: largefunc refactor
The refactor is well-structured and extracted helpers are clean. One correctness regression was found.
Findings:
| Severity | File | Issue |
|---|---|---|
| 🔴 Correctness | compiler_yaml_prompt.go:71 |
Expression extraction from MainWorkflowMarkdown dropped for the PromptImports path |
| 🟡 Noise | *.lock.yml |
Action-version metadata regenerated, contradicting the "emitted YAML unchanged" claim |
Correctness regression detail
In the original code, Step 1.5 (extracting needs.* expressions from MainWorkflowMarkdown) ran unconditionally — it applied to both the ordered-imports path and the legacy path.
In the refactor, that extraction only executes in buildUserPromptChunks inside the legacy branch (after buildOrderedPromptImportChunks returns early). Workflows that use PromptImports and reference needs.* expressions in their main markdown will silently produce incomplete substitution steps, causing actionlint errors or runtime failures.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 57.5 AIC · ⌖ 4.36 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on one correctness issue and two maintainability patterns.
📋 Key Themes & Highlights
Key Themes
- Correctness gap (line 71,
compiler_yaml_prompt.go): thebuildUserPromptChunksrefactor accidentally conditionalized theMainWorkflowMarkdownexpression-extraction step — it now runs only in the legacy path, not whenPromptImportsis present. This was a single sequential block before the split. - Template call readability (
maintenance_workflow_yaml_sections.go): severalfmt.Fprintfcalls pass 6–8 positional arguments in a single line against long templates; a misaligned argument silently corrupts generated YAML. - Intentionality of conditionals:
resolveAwInfoMCPGatewayVersiondropped theVersion != ""guard — behaviorally equivalent today but loses the explicit intent.
Positive Highlights
- ✅ The builder-struct pattern for
maintenanceWorkflowYAMLBuilderis clean and navigable — a marked improvement over the 961-line monolith. - ✅
writeHeaderCommentBlockand its helpers decompose the header into single-responsibility writers that are easy to test independently. - ✅ The
awInfoParamsstruct is a good deep-module move: all resolution logic is co-located and the write step becomes trivially thin. - ✅ Lock file changes are all trivial metadata-only diffs — consistent with the PR's behavioral-equivalence claim.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 126.6 AIC · ⌖ 4.91 AIC · ⊞ 6.7K
Comment /matt to run again
| compilerYamlPromptLog.Printf("Inlined main workflow markdown in %d chunks", len(inlinedChunks)) | ||
| func buildUserPromptChunks(c *Compiler, data *WorkflowData) ([]string, []*ExpressionMapping) { | ||
| if len(data.PromptImports) > 0 { | ||
| return buildOrderedPromptImportChunks(c, data) |
There was a problem hiding this comment.
[/codebase-design] buildUserPromptChunks early-returns on line 71 for the ordered-import path, so the MainWorkflowMarkdown expression-extraction guard (lines 74–82) is only reachable in the legacy path. In the original monolith, that extraction ran after both import paths; the split accidentally conditionalized it.
💡 Impact & suggested fix
When a workflow uses PromptImports (the non-legacy path), needs.* expressions in the main markdown body will not be extracted, which means the substitute-placeholders step will later emit unresolved __EXPR_*__ placeholders at runtime.
Consider moving the MainWorkflowMarkdown extraction into buildOrderedPromptImportChunks, or lifting it into generatePrompt after both helpers return.
@copilot please address this.
| if data.SandboxConfig != nil && data.SandboxConfig.MCP != nil && data.SandboxConfig.MCP.Version != "" { | ||
| mcpGatewayVersion = data.SandboxConfig.MCP.Version | ||
| func resolveAwInfoMCPGatewayVersion(data *WorkflowData) string { | ||
| if data.SandboxConfig != nil && data.SandboxConfig.MCP != nil { |
There was a problem hiding this comment.
[/codebase-design] resolveAwInfoMCPGatewayVersion returns data.SandboxConfig.MCP.Version unconditionally when MCP != nil, but the original code only set mcpGatewayVersion when Version != "". If Version is an empty string, the old path left mcpGatewayVersion as "", which is the same result here — so this is behaviorally safe. However the comment (Version != "") was load-bearing documentation that is now silently gone.
💡 Suggested improvement
The original conditional was there to make the intent explicit. Consider re-adding the guard:
func resolveAwInfoMCPGatewayVersion(data *WorkflowData) string {
if data.SandboxConfig != nil && data.SandboxConfig.MCP != nil && data.SandboxConfig.MCP.Version != "" {
return data.SandboxConfig.MCP.Version
}
return ""
}This makes the "only set when non-empty" intent visible at the extractor level rather than relying on the zero-value coincidence.
@copilot please address this.
| if b.opts.maintenanceConfig.IsJobDisabled("close-expired-entities") { | ||
| return | ||
| } | ||
| jobs := []maintenanceCloseExpiredJob{{"close-expired-discussions", "discussions: write", "Close expired discussions", "close_expired_discussions"}, {"close-expired-issues", "issues: write", "Close expired issues", "close_expired_issues"}, {"close-expired-pull-requests", "pull-requests: write", "Close expired pull requests", "close_expired_pull_requests"}} |
There was a problem hiding this comment.
[/codebase-design] The three maintenanceCloseExpiredJob literals are packed into a single long slice literal on one line (line 624), which is hard to scan and would fail a longline linter if one is ever added.
💡 Suggested formatting
jobs := []maintenanceCloseExpiredJob{
{"close-expired-discussions", "discussions: write", "Close expired discussions", "close_expired_discussions"},
{"close-expired-issues", "issues: write", "Close expired issues", "close_expired_issues"},
{"close-expired-pull-requests", "pull-requests: write", "Close expired pull requests", "close_expired_pull_requests"},
}Minor, but the extracted helper is otherwise a clean pattern — do not undercut it with an opaque one-liner.
@copilot please address this.
| } | ||
|
|
||
| func (b *maintenanceWorkflowYAMLBuilder) writeRunOperationJob() { | ||
| fmt.Fprintf(b.yaml, maintenanceRunOperationJobTemplate, RenderCondition(buildRunOperationCondition("safe_outputs", "create_labels", "activity_report", "close_agentic_workflows_issues", "clean_cache_memories", "update_pull_request_branches", "validate", "forecast")), b.opts.runsOnValue, b.repositoryCheckoutBlock(), b.setupScriptsBlock(), b.adminPermissionsBlock(""), generateInstallCLISteps(b.ctx, b.opts.actionMode, b.opts.version, b.opts.actionTag, b.opts.resolver), b.githubScriptPin(), getCLICmdPrefix(b.opts.actionMode)) |
There was a problem hiding this comment.
[/codebase-design] writeRunOperationJob calls fmt.Fprintf with 8 positional arguments on a single line. This is harder to maintain than the original inline construction because a new argument requires counting format verbs.
💡 Suggested improvement
Extract the template arguments to named locals before the Fprintf call, so each argument is self-documenting:
func (b *maintenanceWorkflowYAMLBuilder) writeRunOperationJob() {
condition := RenderCondition(buildRunOperationCondition(...))
repoCheckout := b.repositoryCheckoutBlock()
scripts := b.setupScriptsBlock()
adminCheck := b.adminPermissionsBlock("")
cliSteps := generateInstallCLISteps(b.ctx, b.opts.actionMode, b.opts.version, b.opts.actionTag, b.opts.resolver)
ghScript := b.githubScriptPin()
cmdPrefix := getCLICmdPrefix(b.opts.actionMode)
fmt.Fprintf(b.yaml, maintenanceRunOperationJobTemplate,
condition, b.opts.runsOnValue,
repoCheckout, scripts, adminCheck, cliSteps,
ghScript, cmdPrefix)
}@copilot please address this.
There was a problem hiding this comment.
Review: behavioral-preserving refactor with two correctness concerns
Two issues warrant attention before merging:
1. Expression extraction skipped for ordered imports (high, existing comment at line 71)
When PromptImports is non-empty, buildUserPromptChunks returns via buildOrderedPromptImportChunks without running the Step 1.5 expression extraction from MainWorkflowMarkdown. The original code always ran that step after the import branching. This drops needs.* and other compile-time expressions from the substitution step for any workflow that uses ordered imports.
2. Firewall version silently dropped when firewall disabled (medium, new comment)
resolveAwInfoFirewallVersion returns "" whenever !firewallConfig.Enabled, but the original set firewallVersion = firewallConfig.Version unconditionally and only applied the default fallback under Enabled && Version == "". The refactor silently drops a non-empty version for a disabled-but-explicitly-versioned firewall config.
Other observations (non-blocking)
- Lock file changes (comment annotations
9c091bb...→v7.0.0) are unrelated to the refactor and contradict the PR's claim that emitted YAML is unchanged (already flagged in existing comments). - The
resolveWorkflowHeaderSourcehelper introduced for internal use returns a fallback string but is never called externally, keeping its scope clean. mergeKnownNeedsExpressionscorrectly preserves the original merge-with-higher-priority semantics.
🔎 Code quality review by PR Code Quality Reviewer · 490.9 AIC · ⌖ 4.53 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_yaml_step_lifecycle.go:1079
Behavioral change in resolveAwInfoFirewallVersion: the original code set firewallVersion = firewallConfig.Version unconditionally, so a disabled-but-explicitly-versioned firewall config still emitted its version string; the refactored helper returns "" whenever !firewallConfig.Enabled, silently dropping a non-empty version.
<details>
<summary>💡 Detail</summary>
Original semantics:
firewallVersion = firewallConfig.Version // set regardless of Enabled
if firewallEnab…
</details>
🤖 PR Triage
Rationale: Mechanical largefunc lint fix across workflow YAML builders. Large diff (1441+/1575−, 10 files) but purely structural. Review together with #46332.
|
|
@copilot please run the Open review feedback to address:
Please fix the reported correctness issues, update the branch if needed, rerun relevant validation/checks, and summarize what changed plus any remaining blockers for maintainers.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in f727227. I restored main-workflow expression extraction for the PromptImports path, preserved the explicit disabled-firewall aw_info version behavior, reinstated the MCP gateway non-empty guard, applied the requested readability cleanups in the maintenance helper, and reverted the affected generated lockfile drift. Local |
Five functions in
pkg/workflowexceeded the 60-linelargefunclint limit, withbuildMaintenanceWorkflowYAMLat 961 lines being the primary offender. Pure behavioral-preserving refactor — emitted YAML and all external interfaces are unchanged.Changes
maintenance_workflow_yaml.go+ newmaintenance_workflow_yaml_sections.goExtracted ~13 per-job helpers from the monolithic 961-line
buildMaintenanceWorkflowYAML. The main function is now a thin orchestrator:compiler_yaml_prompt.goSplit
generatePrompt(284 lines) by extracting import-processing and expression-merging phases into focused helpers.compiler_yaml_header.goSplit
generateWorkflowHeader(172 lines) into separate metadata-line writer and comment-block writer.compiler_yaml_step_lifecycle.goSplit
generateCreateAwInfo(180 lines) into param-resolution + YAML env-var writing helpers; splitgenerateOutputCollectionStep(82 lines) into step and domain-resolution helpers.