refactor(workflow): split compiler_yaml_main_job.go into focused modules#44651
Conversation
Splits the 1265-line compiler_yaml_main_job.go into smaller, focused files: - compiler_yaml_main_job.go: reduced to 42-line orchestrator only - compiler_yaml_checkout.go (new, 302 lines): Phase 1 – OTLP masking, pre-steps, workspace checkout, dev-mode CLI build, repository import checkouts, legacy agent import checkout, and merge-remote-.github-folder step - compiler_yaml_runtime_setup.go (new, 431 lines): Phase 2 – runtime detection, ARC/DinD adapters, activation artifact download, comment-memory setup, custom step emission and sanitization - compiler_yaml_post_agent.go (new, 267 lines): Phase 5 – artifact path accumulation, step-summary generation, post-agent upload and cleanup - compiler_yaml_ai_execution.go (extended, 497 lines): Phases 3 & 4 – generateEngineInstallAndPreAgentSteps and generateAgentRunSteps added All files remain in package workflow; no imports changed; all tests pass. Closes #44549 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Refactors the workflow YAML compiler’s main agent-job generation by splitting the previously large compiler_yaml_main_job.go into phase-focused modules, keeping behavior the same while making the code easier to navigate and modify.
Changes:
- Replaced the monolithic main-job generator with a small phase orchestrator.
- Extracted checkout-related compilation logic into
compiler_yaml_checkout.go. - Extracted runtime/workspace setup and post-agent collection/upload logic into dedicated modules, and moved pre-agent/agent-run logic into
compiler_yaml_ai_execution.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_yaml_main_job.go | Reduced to a phase orchestrator that delegates to extracted phase functions. |
| pkg/workflow/compiler_yaml_checkout.go | New module containing checkout/import/merge steps generation. |
| pkg/workflow/compiler_yaml_runtime_setup.go | New module for runtime detection/setup, activation artifact + memory prep, and custom step emission/sanitization. |
| pkg/workflow/compiler_yaml_ai_execution.go | Extended to include pre-agent install/setup and agent execution step generation previously in main job. |
| pkg/workflow/compiler_yaml_post_agent.go | New module for artifact path collection, step-summary generation, and post-agent upload/cleanup orchestration. |
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: 1
- Review effort level: Low
| // generateLegacyAgentImportCheckout generates a checkout step for legacy agent imports | ||
| // Legacy format: owner/repo/path/to/file.md@ref | ||
| // This checks out the entire repository (not just .github folder) since the file could be anywhere |
There was a problem hiding this comment.
Review: Pure Mechanical Refactor ✅
This PR is a clean structural split of compiler_yaml_main_job.go into five focused modules. All 22 functions were moved verbatim with no logic changes.
Verification:
- Every removed function matches an added function in the new files — no omissions or additions
- Imports correctly redistributed:
os,encoding/json,console,constantsmoved to the files that use them compiler_yaml_main_job.goretains onlystringsimport, which is correct- All new files use
package workflow— no package boundary issues - No behavioral changes detected in any changed line
No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 30.4 AIC · ⌖ 5.35 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — the split is a clean improvement, with one naming observation worth addressing.
📋 Key Themes & Highlights
Issue Found
- File name / content mismatch:
compiler_yaml_ai_execution.gohosts both Phase 3 (engine setup: git config, MCP, engine installation) and Phase 4 (actual agent run). The other extracted files each map to one logical phase by name; this one deviates from that pattern. See the inline comment for options.
Positive Highlights
generateMainJobStepsis now a crisp 42-line phase orchestrator — easy to audit at a glance- Phase comment blocks in the orchestrator clearly document the responsibility boundary of each call
- Strict
package workflowcontainment — no import rewiring, zero risk of API surface changes - The ordering comments in
generateEngineInstallAndPreAgentStepsexplain why each step must precede the next (e.g. base-branch restore before pre-agent steps, secret redaction before upload) sanitizeAndWarnCustomStepsandaddCustomSteps*are cohesively grouped incompiler_yaml_runtime_setup.goalongside the step emission logic
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 45.1 AIC · ⌖ 4.49 AIC · ⊞ 6.6K
Comment /matt to run again
| // generateActivationArtifactAndCommentMemorySteps) so that user steps: can access prior | ||
| // comment-memory state. | ||
| // It returns the resolved CodingAgentEngine for use in subsequent phases. | ||
| func (c *Compiler) generateEngineInstallAndPreAgentSteps(yaml *strings.Builder, data *WorkflowData, needsGitConfig bool) (CodingAgentEngine, error) { |
There was a problem hiding this comment.
[/codebase-design] Phase 3 (generateEngineInstallAndPreAgentSteps) lives in ai_execution.go, but it covers git credentials, MCP setup, and engine installation — none of which is AI execution. A future reader navigating to 'AI execution' code will land here and need to mentally separate the two phases.
💡 Suggested rename or split
Consider either:
- Renaming the file to
compiler_yaml_engine_setup_and_execution.goto reflect that it covers both setup and run phases. - Or, since the PR description calls them distinct phases, extract Phase 3 into
compiler_yaml_engine_setup.goand keep onlygenerateAgentRunStepsincompiler_yaml_ai_execution.go.
Option 1 is the lower-effort fix and is consistent with the file-per-phase pattern established by the other new files.
@copilot please address this.
…o into phase modules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,243 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.
Review: Pure mechanical split — logic preserved, but the file boundaries leak
The refactor successfully breaks a 1265-line monolith into four focused files. The compiler builds cleanly and all tests pass. Phase ordering and variable scoping in the orchestrator are correct.
One security-adjacent concern and two cohesion issues survive adjudication:
📋 Findings summary
High — security-adjacent (non-blocking, pre-existing, but worsened by split)
compiler_yaml_ai_execution.go:481— secret redaction snapshot covers only Phase 1–4 YAML; Phase 5 steps are outside the scan window. This was already true in the monolith, but the split distributes phases across files with no in-file warning, making it easy for a future contributor to add a${{ secrets.* }}reference incompiler_yaml_post_agent.gowithout realising it will never be scanned.
Medium — maintainability
compiler_yaml_ai_execution.go:139—isOTLPEnabledshould live inobservability_otlp.gowithisOTLPHeadersPresent/isOTLPAttributesPresent.
Low — maintainability
compiler_yaml_ai_execution.go:172—generateAgentOutputPlaceholderStephas its sole call site in Phase 5 but is defined in the Phase 4 file.
None of these are regressions introduced by new logic — they are either pre-existing hazards made less visible by the split, or under-grouping of helpers. Recommend addressing the redaction scope concern (at minimum with a clear doc comment) before this lands, so future Phase 5 contributors are not silently in a blind spot.
🔎 Code quality review by PR Code Quality Reviewer · 172.6 AIC · ⌖ 4.81 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_yaml_ai_execution.go:481
Secret redaction snapshot excludes all Phase 5 YAML — any secrets first referenced in post-agent steps are never scanned by the redactor.
<details>
<summary>💡 Details</summary>
generateSecretRedactionStep at line 481 receives yaml.String() as a snapshot of the entire YAML output at the end of Phase 4. generatePostAgentCollectionAndUpload (Phase 5) then appends more YAML after this call — any ${{ secrets.* }} expressions that appear only in Phase 5 steps (generatePostSteps…
pkg/workflow/compiler_yaml_ai_execution.go:139
isOTLPEnabled is stranded in the AI execution file while its sibling helpers live in observability_otlp.go, creating a hidden cross-file dependency for post-agent code.
<details>
<summary>💡 Details</summary>
isOTLPEnabled is defined here at line 139, but its two closest relatives — isOTLPHeadersPresent (line 281 of observability_otlp.go) and isOTLPAttributesPresent (line 317 of observability_otlp.go) — already live in the OTLP observability file. The function is called from…
pkg/workflow/compiler_yaml_ai_execution.go:172
generateAgentOutputPlaceholderStep is defined here (Phase 4 file) but its only call site is in compiler_yaml_post_agent.go (Phase 5), making navigation unnecessarily confusing.
<details>
<summary>💡 Details</summary>
This function has a single caller: compiler_yaml_post_agent.go line 195 (inside generatePostAgentCollectionAndUpload). Defining it in the AI execution file means a maintainer reading Phase 5 code must look in the wrong file to understand or modify this step. Move it t…
|
Run: https://github.com/github/gh-aw/actions/runs/29072075004
|
…entImportCheckout The previous comment incorrectly described: - The accepted format as 'owner/repo/path/to/file.md@ref' — specs with extra path segments are rejected by parseRepositoryImportSpec. - The checkout scope as 'entire repository' — only .github/ is checked out via sparse-checkout. Update the comment to match actual parser behavior and step output. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the review feedback on the doc comment at
Fixed in commit |
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/compiler_yaml_main_job.gohad grown to 1265 lines across 23 methods spanning 5 distinct compilation phases, making targeted edits and navigation expensive.Changes
compiler_yaml_main_job.go— reduced to a 42-line phase orchestrator calling the four phase functions belowcompiler_yaml_checkout.go(new, 302 lines) — Phase 1: OTLP masking, pre-steps, workspace checkout, dev-mode CLI build, repository import checkouts, legacy agent import checkout, merge-.github-folder stepcompiler_yaml_runtime_setup.go(new, 431 lines) — Phase 2: runtime detection, ARC/DinD tool-cache redirect, activation artifact download, comment-memory setup, custom step emission/sanitizationcompiler_yaml_post_agent.go(new, 267 lines) — Phase 5: artifact path accumulation, step-summary generation, post-agent upload and cleanupcompiler_yaml_ai_execution.go(extended, 497 lines) — absorbs Phases 3 & 4:generateEngineInstallAndPreAgentStepsandgenerateAgentRunStepsAll files stay in
package workflow; no imports rewired, no logic changed — pure mechanical split.