Refactor buildMainJob into composable helpers to reduce function-length lint findings#37491
Refactor buildMainJob into composable helpers to reduce function-length lint findings#37491Copilot wants to merge 7 commits into
buildMainJob into composable helpers to reduce function-length lint findings#37491Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
buildMainJob into composable helpers to reduce function-length lint findings
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR refactors the main agent-job compiler path by decomposing buildMainJob in pkg/workflow/compiler_main_job.go into smaller helper methods, aiming to reduce function-length lint findings while preserving existing compilation behavior. It also adds a focused unit test for the extracted ensureMainJobContentsRead helper.
Changes:
- Split
buildMainJobinto composable helpers for step construction, condition building, dependency resolution (includingengine.envscanning), built-in needs warnings, outputs/env assembly, and permissions enforcement. - Extracted permissions shaping + gh-write command enforcement into
buildMainJobPermissions, with new pure helpersensureMainJobContentsReadandextractMainJobScripts. - Added unit tests for
ensureMainJobContentsRead.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_main_job.go | Refactors main job construction into helpers; extracts permissions shaping and dependency/warning logic. |
| pkg/workflow/compiler_main_job_test.go | Adds targeted unit tests for the new ensureMainJobContentsRead helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
| if permissions == "" { | ||
| return NewPermissionsContentsRead().RenderToYAML() | ||
| } |
| if level, exists := perms.Get(PermissionContents); !exists || level == PermissionNone { | ||
| perms.Set(PermissionContents, PermissionRead) | ||
| return perms.RenderToYAML() | ||
| } |
| t.Run("creates default contents read permissions when empty", func(t *testing.T) { | ||
| result := ensureMainJobContentsRead("", true) | ||
| assert.Equal(t, NewPermissionsContentsRead().RenderToYAML(), result) | ||
| }) |
Generated by the Design Decision Gate workflow for PR #37491. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (227 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 for capturing the rationale behind structural decisions like this decomposition. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot recompile - review all comments |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, and /improve-codebase-architecture — requesting changes on test coverage and two correctness findings.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: 7 of 8 extracted helpers have no unit tests.
buildMainJobCondition,buildMainJobDependencies, andbuildMainJobPermissionsare the highest-risk omissions — they contain non-trivial branching and a security guard respectively. - Dead code in
warnOnBuiltInEngineEnvNeeds:builtinsWarnedmap and its check are unreachable; the sorted unique slice already dedups. - Silent error swallow in
buildMainJobOutputs:err != nilandGetErrorDetectionScriptId() == ""are conflated — an engine-lookup failure drops all four error outputs with no log. - Observability loss: per-dependency log lines and the IMPORTANT transitive-dependency rationale comment were both removed.
Positive Highlights
- ✅ The decomposition is clean —
buildMainJobnow reads as a flat sequential assembly, much easier to scan - ✅
map[string]bool→map[string]struct{}is a nice idiomatic cleanup - ✅
ensureMainJobContentsReadextracted as a pure package-level function is directly testable — good call - ✅ Early-return patterns in
buildMainJobConditionandbuildMainJobOutputsreduce nesting without changing behaviour
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.6M · 469.7 AIC · ⌖ 13.7 AIC
| return RenderCondition(BuildAnd(&ExpressionNode{Expression: stripExpressionWrapper(jobCondition)}, guard)) | ||
| } | ||
| return jobCondition | ||
| } |
There was a problem hiding this comment.
[/tdd] buildMainJobCondition has three non-trivial branches (keep condition / clear it / append ET guardrail), but no unit tests were added. A regression in the ET-guardrail path or the "keep condition when referencing non-pre_activation custom jobs" path would go silently undetected.
💡 Suggested test cases
func TestBuildMainJobCondition(t *testing.T) {
t.Run("clears condition when activation exists and no custom-job-output references", ...)
t.Run("keeps condition when referencing non-pre_activation custom job outputs", ...)
t.Run("appends ET guardrail guard when hasMaxDailyEffectiveTokensGuardrail is true", ...)
}These cover the three branching paths that were preserved from the original function.
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg)) | ||
| c.IncrementWarningCount() | ||
| } | ||
| if _, warned := builtinsWarned[builtinJobName]; warned { |
There was a problem hiding this comment.
[/improve-codebase-architecture] builtinsWarned is dead code: builtinNames is built from unique map keys and then sorted, so each builtin name appears exactly once in the range loop. The if _, warned := builtinsWarned[builtinJobName]; warned guard can never be true and the map is never consulted.
�� Simplification
Remove builtinsWarned entirely. The sorted unique slice is already sufficient deduplication:
for _, builtinJobName := range builtinNames {
if slices.Contains(depends, builtinJobName) {
continue
}
if !strings.Contains(engineEnvContent, fmt.Sprintf("needs.%s.", builtinJobName)) {
continue
}
// emit warning ...
}This also appeared in the original code — worth cleaning up while the refactor touches this path.
| compilerMainJobLog.Printf("Added model_not_supported_error output (engine=%s, step=%s)", engine.GetID(), constants.DetectAgentErrorsStepID) | ||
| } | ||
| engine, err := c.getAgenticEngine(data.AI) | ||
| if err != nil || engine.GetErrorDetectionScriptId() == "" { |
There was a problem hiding this comment.
[/zoom-out] err != nil (engine not found) and engine.GetErrorDetectionScriptId() == "" (no script) are combined in one condition, silently dropping all four error-detection outputs on an engine-lookup failure with no log. The two cases should be separated so an unexpected error is observable.
💡 Suggestion
engine, err := c.getAgenticEngine(data.AI)
if err != nil {
compilerMainJobLog.Printf("skipping engine error detection outputs: %v", err)
return outputs
}
if engine.GetErrorDetectionScriptId() == "" {
return outputs
}This preserves the original intent while making engine-lookup errors visible in the log.
| depends = append(depends, jobName) | ||
| compilerMainJobLog.Printf("Added direct dependency on custom job '%s' because it's referenced in workflow content or engine.env", jobName) | ||
| } | ||
| return depends, engineEnvContent |
There was a problem hiding this comment.
[/tdd] buildMainJobDependencies — the content-scan that decides which custom jobs become direct needs: dependencies — has no unit test. This is the highest-correctness path in the file: a wrong result here silently produces invalid GitHub Actions YAML.
💡 Suggested test cases
- MarkdownContent referencing
${{ needs.my_job.outputs.foo }}→my_jobadded todepends engineEnvContentreferencing a builtin job → not added todepends(builtin guard)- Job already in base
depends→ not duplicated customStepswith aneeds.*expression → included in scan
| if len(agentAllScripts) == 0 { | ||
| return permissions, nil | ||
| } | ||
| if writeCmds := detectWriteCommandsInShellScripts(agentAllScripts); len(writeCmds) > 0 { |
There was a problem hiding this comment.
[/tdd] The write-command safety guard (detectWriteCommandsInShellScripts) has no test here. If extractMainJobScripts ever misses a section, the guard would silently stop enforcing the read-only constraint.
💡 Suggested test
t.Run("returns error when write gh command detected in pre-steps", func(t *testing.T) {
c := &Compiler{actionMode: ...}
data := &WorkflowData{PreSteps: "steps:\n - run: gh pr edit #1 --title foo"}
_, err := c.buildMainJobPermissions(data)
assert.Error(t, err)
assert.Contains(t, err.Error(), "write gh command")
})| perms := NewPermissionsParser(result).ToPermissions() | ||
| level, exists := perms.Get(PermissionContents) | ||
| assert.True(t, exists) | ||
| assert.Equal(t, PermissionRead, level) |
There was a problem hiding this comment.
[/tdd] Missing edge case: ensureMainJobContentsRead when needsContentsRead=true but contents: write is already set. The function should preserve write (not downgrade it), but no test proves this invariant.
💡 Suggested test
t.Run("does not downgrade contents:write to contents:read", func(t *testing.T) {
permissions := "permissions:\n contents: write\n"
result := ensureMainJobContentsRead(permissions, true)
perms := NewPermissionsParser(result).ToPermissions()
level, exists := perms.Get(PermissionContents)
assert.True(t, exists)
assert.Equal(t, PermissionWrite, level)
})| // Find custom jobs that depend on pre_activation - these are handled by the activation job | ||
| func (c *Compiler) buildMainJobCondition(data *WorkflowData, activationJobCreated bool) string { | ||
| jobCondition := data.If | ||
| customJobsBeforeActivation := c.getCustomJobsDependingOnPreActivation(data.Jobs) |
There was a problem hiding this comment.
[/improve-codebase-architecture] getCustomJobsDependingOnPreActivation(data.Jobs) is called unconditionally, but its result is only used inside if activationJobCreated. Move it inside the block to avoid the unnecessary traversal when activationJobCreated=false.
💡 Suggestion
func (c *Compiler) buildMainJobCondition(data *WorkflowData, activationJobCreated bool) string {
jobCondition := data.If
if activationJobCreated {
customJobsBeforeActivation := c.getCustomJobsDependingOnPreActivation(data.Jobs)
if c.referencesCustomJobOutputs(data.If, data.Jobs) && len(customJobsBeforeActivation) > 0 {
jobCondition = ""
} else if !c.referencesCustomJobOutputs(data.If, data.Jobs) {
jobCondition = ""
}
}
...| continue | ||
| } | ||
| depends = append(depends, jobName) | ||
| compilerMainJobLog.Printf("Added direct dependency on custom job '%s' because it's referenced in workflow content or engine.env", jobName) |
There was a problem hiding this comment.
[/zoom-out] The original code had an // IMPORTANT: comment explaining why direct dependencies must be added even for jobs transitively reachable through activation — GitHub Actions expression evaluation requires needs.X.outputs.* references to appear in a job's direct needs: list, or the expression silently evaluates to empty at runtime. That rationale is now gone. A future maintainer could mistake these additions for redundant work and remove them.
Consider adding a brief inline comment to buildMainJobDependencies preserving this invariant.
| if !alreadyDepends { | ||
| configMap, ok := data.Jobs[jobName].(map[string]any) | ||
| if ok && !jobDependsOnPreActivation(configMap) && !jobDependsOnAgent(configMap) { | ||
| depends = append(depends, jobName) |
There was a problem hiding this comment.
[/zoom-out] The original code logged "Added direct dependency on custom job %q" for every non-pre_activation, non-agent job added as a base dependency. That log line is now gone from getMainJobBaseDependencies. Only reference-based additions (in buildMainJobDependencies) still log. When debugging a needs: list issue, the absence of this log makes it harder to distinguish base-dependency additions from content-scan additions.
|
|
||
| assert.Equal(t, permissions, result) | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] The test file covers only ensureMainJobContentsRead (1 of 8+ extracted helpers). The following extracted methods have no unit tests at all:
buildMainJobCondition— non-trivial 3-way branchbuildMainJobDependencies+getMainJobBaseDependencies— determinesneeds:list correctnessbuildMainJobPermissions+extractMainJobScripts— security-relevant write-command guardwarnOnBuiltInEngineEnvNeeds— warning emission (also contains the deadbuiltinsWarnedcode)buildMainJobOutputs— engine error detection output inclusion
A refactor that decomposes a large function into smaller helpers is most valuable when those helpers gain independent test coverage. Adding at least the condition, dependency, and permissions helpers would meaningfully increase confidence in the refactor.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent (check failed — coding-guideline violation)
📊 Metrics & Test Classification (3 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
|
| Assertion | Suggested Message |
|---|---|
assert.Equal(t, NewPermissionsContentsRead().RenderToYAML(), result) |
"empty permissions should default to contents:read YAML" |
assert.True(t, exists) |
"PermissionContents key should be present after merge" |
assert.Equal(t, PermissionRead, level) |
"contents permission level should be read after merge" |
assert.Equal(t, permissions, result) |
"permissions should be unchanged when contents read is not required" |
Verdict
❌ Check failed. New file
pkg/workflow/compiler_main_job_test.gois missing the required//go:build !integrationbuild tag on line 1. Please add the tag before merging. (The behavioral quality of the tests themselves is excellent — 3/3 are design tests covering observable contracts.)
References: §27084472587
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
🧪 Test quality analysis by Test Quality Sentinel · sonnet46 1.1M · 323.7 AIC · ⌖ 30.3 AIC · ◷
There was a problem hiding this comment.
❌ Test Quality Sentinel: 90/100. Coding-guideline violation detected — new file pkg/workflow/compiler_main_job_test.go is missing the required //go:build !integration build tag on line 1. Please add //go:build !integration as the first line of the file, followed by a blank line, before merging. See the comment above for details and suggested assertion message fixes.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I reviewed the threads and pushed |
There was a problem hiding this comment.
The decomposition is mechanically correct — logic is preserved across the extracted helpers. Three issues need addressing before merge.
### Blocking issues (3)
1. Double referencesCustomJobOutputs call (buildMainJobCondition, lines 93/95) — iterates data.Jobs twice per compile with identical arguments. Cache the result in a local bool before the if/else-if chain.
2. Dead builtinsWarned map (warnOnBuiltInEngineEnvNeeds, lines 175–186) — since builtinNames is derived from a map then sorted, every name appears exactly once in the loop. The deduplication map can never fire and misleads readers into thinking it does something. Drop it.
3. Test coverage gaps in the newly-added TestEnsureMainJobContentsRead:
- "adds contents read" test never asserts the pre-existing
issues: readscope survived the merge (a destructiveSet()bug would pass undetected). - No test for the
contents: none→contents: readupgrade path on line 299 — the most security-sensitive branch in the function. - No idempotency test (input already has
contents: read, expect no change).
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 474.4K · ⌖ 13.6 AIC
| if c.referencesCustomJobOutputs(data.If, data.Jobs) && len(customJobsBeforeActivation) > 0 { | ||
| jobCondition = "" // Activation job handles this condition | ||
| jobCondition = "" | ||
| } else if !c.referencesCustomJobOutputs(data.If, data.Jobs) { |
There was a problem hiding this comment.
referencesCustomJobOutputs called twice with identical arguments, scanning all custom jobs each time.
💡 Suggested fix
func (c *Compiler) buildMainJobCondition(data *WorkflowData, activationJobCreated bool) string {
jobCondition := data.If
customJobsBeforeActivation := c.getCustomJobsDependingOnPreActivation(data.Jobs)
if activationJobCreated {
refsCustomOutputs := c.referencesCustomJobOutputs(data.If, data.Jobs)
if refsCustomOutputs && len(customJobsBeforeActivation) > 0 {
jobCondition = ""
} else if !refsCustomOutputs {
jobCondition = ""
}
}
...
}referencesCustomJobOutputs iterates data.Jobs on every call. Calling it twice per invocation is unnecessary; cache the result in a local variable before the if/else-if chain.
| builtinsWarned := make(map[string]struct{}) | ||
| for _, builtinJobName := range builtinNames { | ||
| if slices.Contains(depends, builtinJobName) { | ||
| continue | ||
| } | ||
| sort.Strings(builtinNames) | ||
| builtinsWarned := make(map[string]bool) | ||
| for _, builtinJobName := range builtinNames { | ||
| // Skip built-ins that are already direct dependencies (e.g., activation) — | ||
| // their outputs are accessible and the expression is valid. | ||
| if slices.Contains(depends, builtinJobName) { | ||
| continue | ||
| } | ||
| if !builtinsWarned[builtinJobName] && strings.Contains(engineEnvContent, fmt.Sprintf("needs.%s.", builtinJobName)) { | ||
| builtinsWarned[builtinJobName] = true | ||
| warningMsg := fmt.Sprintf( | ||
| "engine.env references built-in job '%s' in a needs expression. "+ | ||
| "Built-in jobs are managed by the compiler and cannot be added as direct agent dependencies; "+ | ||
| "this expression will silently evaluate to an empty string at runtime.", | ||
| builtinJobName, | ||
| ) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg)) | ||
| c.IncrementWarningCount() | ||
| } | ||
| if _, warned := builtinsWarned[builtinJobName]; warned { | ||
| continue | ||
| } | ||
| if !strings.Contains(engineEnvContent, fmt.Sprintf("needs.%s.", builtinJobName)) { | ||
| continue | ||
| } | ||
| builtinsWarned[builtinJobName] = struct{}{} |
There was a problem hiding this comment.
builtinsWarned is dead code — the deduplication check at line 180 can never fire.
💡 Why and suggested fix
builtinNames is built by ranging over constants.KnownBuiltInJobNames (a map[string]struct{}), so every key appears exactly once. After sort.Strings, the slice contains each name at most one time. No name can be visited more than once in the loop, so builtinsWarned[builtinJobName] will never be true when checked on line 180.
The map was changed from map[string]bool to map[string]struct{} in this PR but the unreachable logic was preserved, making it look like meaningful deduplication when it provides none.
Drop the map entirely:
for _, builtinJobName := range builtinNames {
if slices.Contains(depends, builtinJobName) {
continue
}
if !strings.Contains(engineEnvContent, fmt.Sprintf("needs.%s.", builtinJobName)) {
continue
}
warningMsg := fmt.Sprintf(...)
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg))
c.IncrementWarningCount()
}| t.Run("adds contents read when required", func(t *testing.T) { | ||
| permissions := "permissions:\n issues: read\n" | ||
|
|
||
| result := ensureMainJobContentsRead(permissions, true) | ||
|
|
||
| perms := NewPermissionsParser(result).ToPermissions() | ||
| level, exists := perms.Get(PermissionContents) | ||
| assert.True(t, exists) | ||
| assert.Equal(t, PermissionRead, level) | ||
| }) |
There was a problem hiding this comment.
Test does not verify that pre-existing permissions are preserved after contents: read is injected.
💡 Suggested addition
The test calls ensureMainJobContentsRead on a block that already contains issues: read, then only checks that contents: read was added. It never asserts issues: read is still present. A bug in Set() that wiped unrelated scopes would pass this test.
Add:
issueLevel, issueExists := perms.Get(PermissionIssues)
assert.True(t, issueExists, "pre-existing issues permission should be preserved")
assert.Equal(t, PermissionRead, issueLevel)|
|
||
| assert.Equal(t, permissions, result) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing test for needsContentsRead=true when contents: none is explicitly set — a security-adjacent gap.
💡 Why this matters and suggested test
Line 299 of the implementation has:
if level, exists := perms.Get(PermissionContents); !exists || level == PermissionNone {
perms.Set(PermissionContents, PermissionRead)This actively upgrades an explicit contents: none to contents: read in dev/script mode when a checkout is needed. That is a deliberate policy decision (and arguably the right call), but it has no test. A regression that stops the upgrade — or one that silently starts upgrading in release mode — would go undetected.
t.Run("upgrades explicit contents:none to contents:read when required", func(t *testing.T) {
permissions := "permissions:\n contents: none\n"
result := ensureMainJobContentsRead(permissions, true)
perms := NewPermissionsParser(result).ToPermissions()
level, exists := perms.Get(PermissionContents)
assert.True(t, exists)
assert.Equal(t, PermissionRead, level)
})
t.Run("leaves existing contents:read unchanged (idempotent)", func(t *testing.T) {
permissions := "permissions:\n contents: read\n"
result := ensureMainJobContentsRead(permissions, true)
assert.Equal(t, permissions, result)
})
This PR addresses one of the highest-impact function-length findings from the lint-monster backlog by refactoring
pkg/workflow/compiler_main_job.go. The change keeps agent job compilation behavior intact while decomposingbuildMainJobinto smaller, focused units.Main-job compilation decomposition
buildMainJobinto dedicated helpers for:needs.*warning emissionDependency and warning logic extraction
needswarning behavior, with explicit set-style tracking for deduped warnings.Permission path extraction
buildMainJobPermissions.ensureMainJobContentsReadextractMainJobScriptsFocused test coverage for extracted helper
compiler_main_job_test.gowith targeted coverage forensureMainJobContentsRead: