refactor(workflow): split 420-line buildMainJob into focused helpers#45333
Conversation
…unction Split the 420-line buildMainJob function in compiler_main_job.go into focused helpers in a new compiler_main_job_helpers.go file: - buildMainJobCondition: job if-condition logic - buildMainJobDependencies: needs list + engine env content - warnBuiltinJobEnvReferences: engine.env built-in job warnings - buildMainJobCoreOutputs: fixed output declarations - addMainJobEngineErrorOutputs: engine error detection outputs - buildMainJobOutputs: complete outputs map - buildMainJobEnv: job-level env vars - augmentPermissionsForDevMode: contents:read augmentation for dev/script mode - collectAgentJobScripts: script gathering for permission inference - buildMainJobPermissions: full permissions logic The buildMainJob body is reduced from 420 lines to under 60. All extracted helpers are each under 60 lines. Targeted unit tests added in compiler_main_job_helpers_test.go covering condition, dependency, output, env, and permission logic. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Split combined if-condition in warnBuiltinJobEnvReferences into two explicit continue-guards and use string concatenation instead of fmt.Sprintf for the needle construction - Add gh-command-name assertion to the write-command error test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors main agent job construction into focused helpers while preserving existing workflow compiler behavior.
Changes:
- Extracts condition, dependency, output, environment, and permission logic.
- Reduces
buildMainJobto orchestration. - Adds targeted helper tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_main_job.go |
Delegates main-job construction concerns to helpers. |
pkg/workflow/compiler_main_job_helpers.go |
Implements extracted construction helpers. |
pkg/workflow/compiler_main_job_helpers_test.go |
Tests helper behavior and boundaries. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, faithful refactor. Logic is preserved across all 9 extracted helpers, tests cover key branches, and the 420-to-60-line reduction is significant.
One minor nit: augmentPermissionsForDevMode is a package-level function that takes *Compiler as a parameter rather than a method receiver. This is slightly unconventional Go style and could be made a method for consistency with the rest of the helpers. Not blocking.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 28.8 AIC · ⌖ 4.27 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Two issues need fixing before merge
1 (high) — Vacuous test for permissions: {} skip logic (compiler_main_job_helpers_test.go:1140): The PreSteps YAML is missing the pre-steps: section key wrapper, so extractRunScriptsFromSectionYAML returns nothing, and the function exits at the empty-scripts early-return before the inference-skip branch is ever reached. The permissions: {} guard (a security-relevant boundary) goes untested. See inline comment.
2 (medium) — Silent engine-error discard in addMainJobEngineErrorOutputs (compiler_main_job_helpers.go:640): getAgenticEngine errors are swallowed without logging. Downstream jobs referencing the five error-detection outputs receive empty strings instead of the 'false' fallback, which can silently break conditional logic in safe_outputs and conclusion jobs. See inline comment.
Non-blocking observations
- Dead dedup map in
warnBuiltinJobEnvReferences(compiler_main_job_helpers.go:577):builtinsWarnedcan never be triggered becausesliceutil.SortedKeysalready deduplicates the slice. Carry-over from the original; safe to remove. See inline comment. - Double call to
referencesCustomJobOutputs(compiler_main_job_helpers.go:487): same args called twice in an if/else-if; hoist into a local variable to avoid the double traversal.
The overall refactor is well-structured and the extracted helpers are properly scoped. Test coverage is thorough outside the one broken case.
🔎 Code quality review by PR Code Quality Reviewer · 59 AIC · ⌖ 4.82 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_main_job_helpers_test.go:1146
Vacuous test — exercises empty-scripts early-return, not the permission-inference skip it claims to cover.
<details>
<summary>💡 Details and fix</summary>
collectAgentJobScripts calls extractRunScriptsFromSectionYAML(data.PreSteps, "pre-steps"), which expects YAML wrapped under a pre-steps: key (the sibling write-command test on line 1126 even has an explicit comment about this). The value here is bare list syntax:
PreSteps: `- name: read step
run: gh issue list`,So `…
pkg/workflow/compiler_main_job_helpers.go:643
Silent engine-lookup failure: addMainJobEngineErrorOutputs discards the error and the caller has no way to detect the omission.
<details>
<summary>💡 Details and fix</summary>
addMainJobEngineErrorOutputs calls c.getAgenticEngine and returns early on error (line 640-641) without logging or returning the error. Its caller, buildMainJobOutputs, has no way to know that the five error-detection outputs (inference_access_error, mcp_policy_error, etc.) were silently dropped. Downstr…
pkg/workflow/compiler_main_job_helpers.go:584
Dead code: builtinsWarned dedup map is populated but its guard can never fire.
<details>
<summary>💡 Details</summary>
sliceutil.SortedKeys returns a deduplicated, sorted slice — each builtin name appears exactly once in the loop. The setutil.Contains(builtinsWarned, builtinJobName) check on line 584 is therefore always false on the single visit to each name, making the entire dedup map dead code.
This was carried over from the original implementation. Remove the map and the contai…
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (5 tests)
Verdict
Key observations:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (694 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two correctness issues.
📋 Key Themes & Highlights
Issues to address
- False-positive test (
compiler_main_job_helpers_test.goline 294):PreStepsis missing its"pre-steps:"YAML wrapper, soextractRunScriptsFromSectionYAMLsilently skips the step and the test never exercises permission inference. - Missing test coverage:
warnBuiltinJobEnvReferencesis the only helper without a test; its side-effect (IncrementWarningCount) is completely uncovered. - Minor consistency nit:
buildMainJobCoreOutputshardcodes"needs.activation.outputs.model"as a string literal instead of usingconstants.ActivationJobName.
Positive Highlights
- ✅ Excellent decomposition — 9 single-responsibility helpers with clear, descriptive names
- ✅
buildMainJobis now well under the 60-line threshold and easy to skim - ✅ Strong test coverage overall: 22 tests covering condition, dependency deduplication, outputs, env, and permissions paths
- ✅ Imports correctly trimmed in the original file
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 98.7 AIC · ⌖ 4.73 AIC · ⊞ 6.6K
Comment /matt to run again
| // buildMainJobCoreOutputs returns the fixed output declarations that every main agent job exposes. | ||
| func buildMainJobCoreOutputs() map[string]string { | ||
| return map[string]string{ | ||
| "model": "${{ needs.activation.outputs.model }}", |
There was a problem hiding this comment.
[/codebase-design] Hardcoded "needs.activation.outputs.model" — should use constants.ActivationJobName for consistency.
Every other activation reference in this file uses the constant (lines 35, 49, 209), but this key embeds the job name as a raw string literal. If the constant ever changes, this output silently breaks.
💡 Suggested fix
"model": fmt.Sprintf("${{ needs.%s.outputs.model }}", constants.ActivationJobName),Same pattern used for artifact_prefix at line 209.
@copilot please address this.
There was a problem hiding this comment.
Fixed in the latest commit — "model" output now uses fmt.Sprintf("${{ needs.%s.outputs.model }}", constants.ActivationJobName) for consistency with all other activation references in the file.
| c := NewCompiler() | ||
| data := &WorkflowData{ | ||
| Permissions: "permissions: {}", | ||
| PreSteps: `- name: read step |
There was a problem hiding this comment.
[/tdd] PreSteps YAML is missing the "pre-steps:" wrapper key — extractRunScriptsFromSectionYAML will silently return nil scripts, so the test never actually exercises permission inference.
extractRunScriptsFromSectionYAML(data.PreSteps, "pre-steps") does yaml.Unmarshal then looks up wrapper["pre-steps"]. Without the top-level key the lookup returns nil and the gh issue list script is never parsed. The test passes vacuously.
💡 Suggested fix
PreSteps: "pre-steps:\n- name: read step\n run: gh issue list",Compare the working test two cases above (line ~282) which correctly wraps with "pre-steps:\n...".
@copilot please address this.
There was a problem hiding this comment.
Fixed — PreSteps is now "pre-steps:\n- name: read step\n run: gh issue list" so extractRunScriptsFromSectionYAML correctly finds the top-level pre-steps key and the test genuinely exercises the inference-skip path.
| // warnBuiltinJobEnvReferences emits a warning when engine.env values reference built-in | ||
| // job names in needs expressions. Built-in jobs are managed by the compiler and cannot be | ||
| // added as direct agent dependencies, so such expressions silently evaluate to empty at runtime. | ||
| func (c *Compiler) warnBuiltinJobEnvReferences(depends []string, engineEnvContent string) { |
There was a problem hiding this comment.
[/tdd] warnBuiltinJobEnvReferences has no test — the warning side-effect and IncrementWarningCount() call are not covered.
This is the only helper in the new file without a corresponding test case. A malformed engine.env reference to a built-in job would silently pass with zero test signal.
💡 Suggested test
func TestWarnBuiltinJobEnvReferences(t *testing.T) {
t.Run("warns when engine.env references a built-in job not in depends", func(t *testing.T) {
c := NewCompiler()
// engineEnvContent references safe_outputs (a built-in not in depends)
c.warnBuiltinJobEnvReferences([]string{}, "${{ needs.safe_outputs.outputs.foo }}")
assert.Equal(t, 1, c.WarningCount())
})
t.Run("no warning when built-in is already a direct dependency", func(t *testing.T) {
c := NewCompiler()
c.warnBuiltinJobEnvReferences([]string{"activation"}, "${{ needs.activation.outputs.model }}")
assert.Equal(t, 0, c.WarningCount())
})
}@copilot please address this.
There was a problem hiding this comment.
Added TestWarnBuiltinJobEnvReferences with three sub-tests: warns when a built-in job not in depends is referenced, no warning when the built-in is already a direct dependency, and no warning when engineEnvContent is empty.
|
Run: https://github.com/github/gh-aw/actions/runs/29303664684
|
- Use constants.ActivationJobName in buildMainJobCoreOutputs model output - Fix missing pre-steps: wrapper key in TestBuildMainJobPermissions - Add TestWarnBuiltinJobEnvReferences with 3 sub-tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
buildMainJobincompiler_main_job.gowas a 420-line monolith — 7× over the 60-linelargefuncthreshold — mixing condition logic, dependency resolution, output declaration, env setup, and permission inference.Changes
pkg/workflow/compiler_main_job_helpers.go(new)Extracts 9 single-responsibility helpers from
buildMainJob:buildMainJobConditionif:condition with AIC guardrailbuildMainJobDependenciesneeds:list + engine env contentwarnBuiltinJobEnvReferencesengine.envreferences to built-in jobsbuildMainJobCoreOutputs/addMainJobEngineErrorOutputs/buildMainJobOutputsbuildMainJobEnvaugmentPermissionsForDevMode/collectAgentJobScripts/buildMainJobPermissionspkg/workflow/compiler_main_job.gobuildMainJobbody reduced from 420 lines to under 60; imports trimmed accordingly.pkg/workflow/compiler_main_job_helpers_test.go(new)22 targeted unit tests covering the boundaries of each helper — condition variants, dependency deduplication, output structure, env stubbing, and permission inference for dev/prod modes.