Decompose safe-output compiler helpers by Copilot · Pull Request #53720 · github/gh-aw

Copilot · 2026-08-18T14:54:29Z

compiler_safe_outputs_job.go exceeded the compiler file-size guideline and contained a 144-line job environment builder, making safe-output compilation harder to review and evolve.

Environment construction
- Moved buildJobLevelSafeOutputEnvVars to compiler_safe_outputs_envvars.go.
- Preserved the existing compiler interface and emitted environment values.
Step helper organization
- Moved artifact-upload and custom safe-output script helpers into compiler_safe_outputs_steps.go.
- Reduced compiler_safe_outputs_job.go to 765 lines.

func (c *Compiler) buildJobLevelSafeOutputEnvVars(
    data *WorkflowData,
    workflowID string,
) map[string]string

Fixes [deep-report] Decompose compiler_safe_outputs_job.go: prior fix issue auto-expired unfixed, same 144-line function rediscovered #53612

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot

Pull request overview

Refactors safe-output compiler helpers into focused files without changing behavior.

Changes:

Extracts job environment construction into a dedicated file.
Moves upload and custom-script helpers into the steps module.
Reduces the main safe-output job compiler to 765 lines.

Show a summary per file

File	Description
`pkg/workflow/compiler_safe_outputs_envvars.go`	Houses job-level environment construction.
`pkg/workflow/compiler_safe_outputs_steps.go`	Houses upload and custom-script step helpers.
`pkg/workflow/compiler_safe_outputs_job.go`	Removes extracted helpers and retains job orchestration.

Review details

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Files reviewed: 3/3 changed files
Comments generated: 0
Review effort level: Balanced

github-actions · 2026-08-18T17:24:09Z

✅ PR Code Quality Reviewer completed the code quality review.

🔎 Code quality review by PR Code Quality Reviewer

github-actions · 2026-08-18T17:24:10Z

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped. The PR 'Decompose safe-output compiler helpers' contains only production code changes (pkg/workflow/compiler_safe_outputs_envvars.go, compiler_safe_outputs_job.go, compiler_safe_outputs_steps.go) with no accompanying test changes.

🧪 Test quality analysis by Test Quality Sentinel

github-actions · 2026-08-18T17:24:11Z

✅ Design Decision Gate 🏗️ completed the design decision gate check.

🏗️ ADR gate enforced by Design Decision Gate 🏗️

github-actions · 2026-08-18T17:24:13Z

✅ Ponytail Reviewer completed successfully!

Lean already. Ship. PR is a pure code-move refactor (splitting compiler_safe_outputs_job.go into compiler_safe_outputs_envvars.go and compiler_safe_outputs_steps.go) with no logic changes, no new abstractions, and no added complexity. Nothing to cut.

Generated by Ponytail Reviewer for #53720

github-actions · 2026-08-18T17:24:54Z

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer

github-actions · 2026-08-18T17:27:32Z

Comment Memory

reviewed_at: 2026-08-18T00:00:00Z
review_event: COMMENT
top_themes:
  - refactor only; no changed-line correctness regressions found
files_reviewed:
  - pkg/workflow/compiler_safe_outputs_envvars.go
  - pkg/workflow/compiler_safe_outputs_job.go
  - pkg/workflow/compiler_safe_outputs_steps.go
comment_count: 0

Note

This comment is managed by comment memory.

It stores persistent context for this thread in the code block at the top of this comment.
Edit only the text inside the backtick fences; workflow metadata and the footer are regenerated automatically.

Learn more about comment memory

🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.92 AIC · ⌖ 6.74 AIC · ⊞ 4.5K · ◷
_{Comment /review to run again}

github-actions

Verdict

No blocking issues found in the changed lines. This is a mechanical helper extraction, and I did not find any changed-line correctness, performance, or safety regressions in the moved code.

Reviewed themes

job-level safe-output environment assembly stayed behaviorally identical
helper extraction for artifact upload and custom script generation stayed functionally equivalent
no new tests were added, but this PR also does not appear to change logic

🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.92 AIC · ⌖ 6.74 AIC · ⊞ 4.5K
_{Comment /review to run again}

github-actions

Skills-Based Review 🧠

Applied /codebase-design — approving with one minor suggestion.

📋 Key Themes & Highlights

Key Themes

Cross-file logger coupling: the new compiler_safe_outputs_envvars.go uses consolidatedSafeOutputsJobLog from compiler_safe_outputs_job.go, leaking a package-internal identifier across files. A dedicated consolidatedSafeOutputsEnvvarsLog would make each file self-contained.

Positive Highlights

✅ Clean mechanical decomposition — the moved code is identical to what was removed; no logic changes.
✅ Import cleanup in compiler_safe_outputs_job.go correctly drops the now-unused encoding/json, sort, and sliceutil imports.
✅ compiler_safe_outputs_steps.go now owns all step-builder helpers, making the responsibility boundary clear.
✅ The scriptNameToHandlerName and generateSafeOutputScriptContent functions are a better fit in the steps file; the move improves navigability.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37.6 AIC · ⌖ 9.93 AIC · ⊞ 7.8K
_{Comment /matt to run again}

github-actions · 2026-08-18T17:30:20Z

+		}
+	}
+
+	// Note: GH_AW_CI_TRIGGER_TOKEN is added at the step level (in buildHandlerManagerStep)


[/codebase-design] consolidatedSafeOutputsJobLog is declared in compiler_safe_outputs_job.go but used here — this cross-file logger coupling makes compiler_safe_outputs_envvars.go harder to read and maintain independently.

💡 Suggested fix

Declare a dedicated logger at the top of compiler_safe_outputs_envvars.go:

import "github.com/github/gh-aw/pkg/logger" var consolidatedSafeOutputsEnvvarsLog = logger.New("workflow:compiler_safe_outputs_envvars")

Then replace consolidatedSafeOutputsJobLog.Printf(...) on this line with consolidatedSafeOutputsEnvvarsLog.Printf(...). This keeps each file self-contained and the namespace matches the filename — consistent with the rest of the codebase.

@copilot please address this.

Implemented a file-scoped consolidatedSafeOutputsEnvvarsLog in compiler_safe_outputs_envvars.go and updated the serialization warning to use it. Commit 727b809.

github-actions

Clean mechanical decomposition — all functions moved verbatim into focused files with no behavioral changes. Imports adjusted correctly. LGTM.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28 AIC · ⌖ 8.74 AIC · ⊞ 5.7K

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

github-actions · 2026-08-18T17:38:23Z

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (278 new lines in pkg/workflow/) but does not have a linked Architecture Decision Record (ADR).

📄 Draft ADR committed: docs/adr/53720-decompose-safe-outputs-compiler-helpers.md — review and complete it before merging.

🔒 This PR cannot merge until an ADR is linked in the PR body.

📋 What to do next

Review the draft ADR committed to your branch — it was generated from the PR diff
Complete the missing sections — add context the AI couldn't infer, refine the decision rationale, and list real alternatives you considered
Commit the finalized ADR to docs/adr/ on your branch
Reference the ADR in this PR body by adding a line such as:

ADR: ADR-53720: Decompose Safe-Outputs Compiler Helpers

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

❓ Why ADRs Matter

"AI made me procrastinate on key design decisions. Because refactoring was cheap, I could always say 'I'll deal with this later.' Deferring decisions corroded my ability to think clearly."

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 Reference

An ADR must contain these four sections to be considered complete:

Context — What is the problem? What forces are at play?
Decision — What did you decide? Why?
Alternatives Considered — What else could have been done?
Consequences — What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number (e.g., 0042-use-postgresql.md for PR #42).

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · sonnet46 · 97.8 AIC · ⌖ 28 AIC · ⊞ 9.1K · ◷
_{Comment /review to run again}

…outputs-job

pelikhan · 2026-08-18T17:47:52Z

@copilot run pr-finisher skill

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot · 2026-08-18T18:21:39Z

@copilot run pr-finisher skill

PR-finisher pass completed. Addressed the outstanding trusted automation review, pushed 727b809, and completed local validation.

github-actions · 2026-08-18T18:23:30Z

PR Triage

Category: refactor
Risk: low
Priority score: 45/100 (impact 18, urgency 12, quality 15)
Recommended action: batch_review
Batch: pr-batch:safe-outputs-compiler (with Enable runner-group runs-on for safe-outputs.jobs (and runner alias) #53725)

Pure decomposition of compiler_safe_outputs_job.go with accompanying ADR. Low risk, but CI run shows cancelled — recommend re-running before merge. Batched with #53725 as both touch the safe-outputs compiler surface.

Generated by 🔧 PR Triage Agent · auto · 45 AIC · ⌖ 2.09 AIC · ⊞ 8.3K · ◷

Initial plan

1dc708b

Copilot AI assigned Copilot and pelikhan Aug 18, 2026

Copilot AI linked an issue Aug 18, 2026 that may be closed by this pull request

[deep-report] Decompose compiler_safe_outputs_job.go: prior fix issue auto-expired unfixed, same 144-line function rediscovered #53612

Open

Copilot started work on behalf of pelikhan August 18, 2026 14:54 View session

Decompose safe outputs compiler helpers

337ff62

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Decompose compiler_safe_outputs_job.go to reduce file size~~ Decompose safe-output compiler helpers Aug 18, 2026

Copilot finished work on behalf of pelikhan August 18, 2026 15:12

Copilot AI requested a review from pelikhan August 18, 2026 15:12

pelikhan marked this pull request as ready for review August 18, 2026 16:37

Copilot AI balanced review requested due to automatic review settings August 18, 2026 16:37

Copilot started reviewing on behalf of pelikhan August 18, 2026 16:38 View session

Copilot AI reviewed Aug 18, 2026

View reviewed changes

github-actions Bot reviewed Aug 18, 2026

View reviewed changes

github-actions Bot approved these changes Aug 18, 2026

View reviewed changes

Add draft ADR for safe-outputs compiler helper decomposition (PR #53720)

89b66a5

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Merge branch 'main' into copilot/deep-report-decompose-compiler-safe-…

987590d

…outputs-job

Copilot started work on behalf of pelikhan August 18, 2026 17:48 View session

Decouple safe-output environment logger

727b809

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot finished work on behalf of pelikhan August 18, 2026 18:22

github-actions Bot added pr-action:batch_review pr-agent:copilot pr-batch:safe-outputs-compiler pr-priority:medium pr-risk:low pr-type:refactor labels Aug 18, 2026

This was referenced Aug 18, 2026

Enable runner-group runs-on for safe-outputs.jobs (and runner alias) #53725

Closed

[PR Triage Report] PR Triage Report — 2026-08-18 (9 PRs) #53784

Open

Uh oh!

Conversation

Copilot AI commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

github-actions Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 18, 2026

Comment Memory

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Verdict

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Skills-Based Review 🧠

Key Themes

Positive Highlights

Uh oh!

github-actions Bot Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 18, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 18, 2026

🏗️ Design Decision Gate — ADR Required

Uh oh!

pelikhan commented Aug 18, 2026

Uh oh!

Copilot AI commented Aug 18, 2026

Uh oh!

github-actions Bot commented Aug 18, 2026

PR Triage

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Aug 18, 2026 •

edited

Loading

github-actions Bot commented Aug 18, 2026 •

edited

Loading

github-actions Bot commented Aug 18, 2026 •

edited

Loading

github-actions Bot commented Aug 18, 2026 •

edited

Loading

github-actions Bot commented Aug 18, 2026 •

edited

Loading

github-actions Bot commented Aug 18, 2026 •

edited

Loading