Refactor compiler job assembly - #54847
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors compiler job assembly into focused phases while preserving workflow generation behavior.
Changes:
- Splits
buildJobsorchestration into focused helper methods. - Adds an evals-job construction and registration wrapper.
- Tests evals-job registration.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_jobs.go |
Extracts job assembly phases and finalization helpers. |
pkg/workflow/evals_job_test.go |
Verifies the evals wrapper registers its job. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review with no actionable findings; no GitHub review comments or final review were emitted.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54847 does not have the 'implementation' label and has only 89 new lines of code in business logic directories (threshold is 100).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Refactor: Compiler Job Assembly — COMMENT
The refactor cleanly decomposes buildJobs into focused helper methods with consistent error wrapping. No correctness or security issues found.
One non-blocking suggestion (see inline): finalizeBuiltJobs should carry a short doc comment explaining the ordering constraints of its three finalization passes, since the original explanatory comments were removed during extraction.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 20.4 AIC · ⌖ 9.04 AIC · ⊞ 6.2K
| // finalizeBuiltJobs applies final dependency, permission, and token-reference passes. | ||
| func (c *Compiler) finalizeBuiltJobs(data *WorkflowData) error { | ||
| if err := c.applyBuiltinJobAugmentations(data); err != nil { | ||
| return fmt.Errorf("built-in job needs augmentations could not be applied: %w. Check that jobs referenced in needs actually exist in the workflow", err) |
There was a problem hiding this comment.
The ordering constraints for the three finalization passes were removed along with their explanatory comments. These passes have non-obvious sequencing requirements:
ensureConclusionIsLastJobmust run after all custom jobs existensureOTLPOIDCJobPermissions/ensureDriveMemoryJobPermissionsmust run per-job after all jobs are created (job-level permissions override workflow-level)validateSafeOutputStepTokenReferencescatches a runtime-silent empty-token bug and must run last
Consider adding a brief doc comment on finalizeBuiltJobs explaining these ordering constraints so future contributors do not inadvertently reorder the passes.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Missing doc comment:
resolveSafeOutputActionSchemaslacks the doc comment that every other new helper has — a small but real consistency gap. - Test coverage: The new
TestBuildEvalsJobWrapperAddsJobtest covers the happy path only; a nil-Evals no-op test would lock in the early-return branch.
Positive Highlights
- ✅ Mechanical, order-preserving extraction — zero behaviour change risk.
- ✅ All extracted helpers have clear, consistent doc comments (except one).
- ✅
buildEvalsJobWrapperis a clean "build + register" deep module — one public interface, rich interior. - ✅ New test correctly asserts job registration, not just absence of error.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.5 AIC · ⌖ 9.95 AIC · ⊞ 7.6K
Comment /matt to run again
| return nil | ||
| } | ||
|
|
||
| func (c *Compiler) resolveSafeOutputActionSchemas(data *WorkflowData, markdownPath string) { |
There was a problem hiding this comment.
[/codebase-design] resolveSafeOutputActionSchemas is the only extracted helper without a doc comment, making it inconsistent with the other new methods and slightly harder to navigate.
💡 Suggested doc comment
// resolveSafeOutputActionSchemas resolves custom safe-output action schemas early
// so that tool schemas derived from action.yml are available when buildMainJobWrapper
// generates MCP tool metadata. No-op when no custom safe-output actions are configured.
func (c *Compiler) resolveSafeOutputActionSchemas(data *WorkflowData, markdownPath string) {@copilot please address this.
| require.True(t, exists) | ||
| assert.Equal(t, string(constants.EvalsJobName), job.Name) | ||
| assert.Contains(t, job.Needs, string(constants.AgentJobName)) | ||
| } |
There was a problem hiding this comment.
[/tdd] TestBuildEvalsJobWrapperAddsJob tests the happy path well, but the error paths in buildEvalsJobWrapper — when buildEvalsJob returns an error, or when jobManager.AddJob fails due to a duplicate job name — have no coverage.
💡 Suggested additional tests
func TestBuildEvalsJobWrapperNilEvalsIsNoop(t *testing.T) {
compiler := NewCompiler()
data := &WorkflowData{AI: "copilot"} // no Evals set
err := compiler.buildEvalsJobWrapper(data)
require.NoError(t, err)
_, exists := compiler.jobManager.GetJob(string(constants.EvalsJobName))
assert.False(t, exists, "no evals job should be added when Evals is nil")
}A nil-evals test locks in the early-return branch and documents the intended no-op behaviour.
@copilot please address this.
There was a problem hiding this comment.
Ponytail review: this refactor splits a monolithic function into named phases, but a few of the new helpers only have one caller and add indirection without reducing complexity.
net: -25 lines possible.
Generated by ✂️ Ponytail Reviewer for #54847 · auto · 26.2 AIC · ⌖ 5.53 AIC · ⊞ 7.3K
Comment /ponytail to run again
| return nil | ||
| } | ||
|
|
||
| func (c *Compiler) resolveSafeOutputActionSchemas(data *WorkflowData, markdownPath string) { |
There was a problem hiding this comment.
L324-332: yagni: 9-line helper with a single caller wrapping one if check. Inline the body into buildJobs.
| } | ||
|
|
||
| // Build additional custom jobs from frontmatter jobs section | ||
| // buildFrontmatterCustomJobs builds additional jobs declared in the workflow frontmatter. |
There was a problem hiding this comment.
L376-385: yagni: single-caller wrapper that only forwards to buildCustomJobs plus a log line. Inline into buildJobs, dropping one indirection layer.
| return nil | ||
| } | ||
|
|
||
| // buildEvalsJobWrapper builds and adds the BinEval evals job if evals are declared in frontmatter. |
There was a problem hiding this comment.
L361-374: yagni: single-caller helper duplicating buildEvalsJob's nil-check/AddJob dance that used to live inline in buildJobs. Fold back into buildSafeOutputsAndEvalsJobs and drop the dedicated wrapper test in evals_job_test.go.
|
🎉 This pull request is included in a new release. Release: |
buildJobshad grown into a large orchestration function, makingcompiler_jobs.goharder to read and below the Daily Compiler Quality threshold. This refactor splits job assembly into focused phases without changing generated workflow behavior.Job orchestration
buildJobsas the high-level sequence.Evals job construction
buildEvalsJobWrapperto isolate evals job creation and registration.