Add fallback artifact so safe outputs survive agent artifact upload failures - #53109
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Diff contains only generated .lock.yml files (regenerated workflow artifacts); the actual Go source changes are not present in the available diff. Per review scope, generated files are skipped. Nothing actionable to flag for over-engineering.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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.
Verdict
Non-blocking from my pass: the fallback artifact upload/download path is wired consistently and I did not find a changed-line correctness bug worth blocking on.
What I checked
buildArtifactDownloadStepsswitches to a mergedpatterndownload only when a fallback artifact is configured.buildAgentOutputDownloadStepsand safe-jobs both opt into the fallback artifact.- The fallback upload is emitted before the large unified agent artifact upload.
- The regression test covers the new upload step ordering and merged download behavior.
- Regenerated lockfiles reflect the compiler change.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 19.2 AIC · ⌖ 5.34 AIC · ⊞ 4.5K
Comment /review to run again
Documents the architectural decision to introduce a dedicated fallback artifact to decouple safe-output transport from the failure-prone large agent artifact upload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (144 new lines in business logic directories) but does not have a linked Architecture Decision Record (ADR). 📄 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.
The fallback artifact approach is well-implemented. Key observations:
if: always()+continue-on-error: true+if-no-files-found: ignorecorrectly handles the case where the agent run itself failed before producing output.- The fallback artifact contains only the two critical files (
agent_output.json,safeoutputs.jsonl), making it a smaller, more reliable upload compared to the fullagentartifact. - The
cp "$GH_AW_SAFE_OUTPUTS" /tmp/gh-aw/safeoutputs.jsonlcopy step already runs before these uploads, so the path is correct. - Download steps use
pattern: "{agent,agent-output-fallback}"withmerge-multiple: true, which correctly merges files from either artifact — safe-output processing will findsafeoutputs.jsonlregardless of which upload succeeded.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 53.5 AIC · ⌖ 7.95 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with minor gaps to address.
📋 Key Themes & Highlights
Key Themes
- Root cause is well diagnosed: the PR description clearly traces the failure from timed-out blob upload → missing artifact → silently dropped safe outputs. The fix correctly decouples the critical small payload from the large, failure-prone artifact.
- Test coverage:
TestAgentOutputFallbackArtifactverifies the happy path thoroughly (ordering, content, download pattern). The negative path — no fallback step whensafe-outputs:is absent — is not tested. - Fragile test section extraction: the substring windowing in the test is correct today but brittle to future YAML changes.
Positive Highlights
- ✅ Constant
AgentOutputFallbackArtifactNameis clearly documented and justified in comments. - ✅
if-no-files-found: ignoreon the fallback upload is the right default — avoids spurious failures when the agent never wrote output. - ✅
merge-multiple: truecorrectly handles the case where both artifacts exist without path conflicts. - ✅ The fallback upload is placed before the large agent artifact upload, so even a hard job timeout is survived.
- ✅ The nil-guard in
generateAgentOutputFallbackUploadis correct — the fallback step is skipped when no safe outputs are declared.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 56.9 AIC · ⌖ 11.5 AIC · ⊞ 7.7K
Comment /matt to run again
| if !strings.Contains(lockYAML, "merge-multiple: true") { | ||
| t.Error("Expected 'merge-multiple: true' so both artifacts extract into the same directory") | ||
| } | ||
| } |
There was a problem hiding this comment.
[/tdd] The generateAgentOutputFallbackUpload nil-guard (if data.SafeOutputs == nil { return }) is not tested — there is no test verifying that workflows without safe-outputs: omit the fallback upload step entirely.
💡 Suggested test
Add a case to TestAgentOutputFallbackArtifact (or a sibling test) that compiles a workflow without safe-outputs: and asserts the lock file does not contain "Upload agent output fallback artifact":
func TestAgentOutputFallbackArtifact_NoSafeOutputs(t *testing.T) {
// Compile a minimal workflow with no safe-outputs section.
// The fallback upload step must be absent because no safe output
// processing will run and the artifact would never be consumed.
...
if strings.Contains(lockYAML, "Upload agent output fallback artifact") {
t.Error("Expected no fallback artifact upload when safe-outputs is not declared")
}
}Without this test the guard is invisible to the test suite and a refactor could accidentally delete it.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 9c041ae: added TestAgentOutputFallbackArtifact_NoSafeOutputs and tightened generateAgentOutputFallbackUpload so default-only/no-declared safe-output workflows omit the fallback upload.
| uploadIdx := strings.Index(lockYAML, "- name: Upload agent output fallback artifact") | ||
| if uploadIdx == -1 { | ||
| t.Fatal("Expected 'Upload agent output fallback artifact' step in generated workflow") | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The test uses a fragile index-based substring extraction to isolate the fallback upload section:
uploadSection := lockYAML[uploadIdx:]
if next := strings.Index(uploadSection[1:], "- name:"); next != -1 {
uploadSection = uploadSection[:next+1]
}This silently over- or under-captures if the YAML structure changes (e.g. an additional step is inserted between the comment and the - name: marker, or the step comment itself contains - name:). The test could then pass while checking the wrong section.
💡 Suggestion
Consider extracting the section with a YAML-aware approach or by matching from "- name: Upload agent output fallback artifact" to the next step that starts at the same indentation level. Alternatively, add a t.Log(uploadSection) so failures at least print what was actually checked.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 9c041ae: replaced the fragile substring slicing with extractWorkflowStepByName, which ends the section at the next step with the same indentation.
There was a problem hiding this comment.
Pull request overview
Adds a dedicated fallback artifact so safe outputs survive failures uploading the larger agent artifact.
Changes:
- Uploads critical agent-output files separately.
- Downloads and merges primary and fallback artifacts.
- Regenerates affected workflow lockfiles and updates tests.
Show a summary per file
| File | Description |
|---|---|
pkg/constants/job_constants.go |
Defines the fallback artifact name. |
pkg/workflow/artifacts.go |
Adds fallback-aware downloads. |
pkg/workflow/compiler_yaml_artifacts.go |
Generates the fallback upload step. |
pkg/workflow/compiler_yaml_post_agent.go |
Emits fallback before unified upload. |
pkg/workflow/safe_outputs_steps.go |
Enables fallback for downstream jobs. |
pkg/workflow/safe_jobs.go |
Enables fallback for custom safe jobs. |
pkg/workflow/safe_output_helpers_test.go |
Updates download expectations. |
pkg/workflow/compiler_artifacts_test.go |
Tests fallback generation and ordering. |
.github/workflows/*.lock.yml |
Regenerates 285 compiled workflows. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 294/294 changed files
- Comments generated: 1
- Review effort level: Balanced
| steps = append(steps, fmt.Sprintf(" pattern: \"{%s,%s}\"\n", config.ArtifactName, config.FallbackArtifact)) | ||
| steps = append(steps, " merge-multiple: true\n") |
There was a problem hiding this comment.
Addressed in 9c041ae: the setup step now checks for agent_output.json before writing GH_AW_AGENT_OUTPUT, so zero-match pattern downloads no longer publish a nonexistent file.
|
@copilot resolve the merge conflicts on this branch. |
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>
|
@copilot update audit/logs go commands to use new artifact |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved the merge conflicts and pushed the merge resolution in |
|
@copilot PR #53109 still needs a follow-up pass. Quick triage
Next step
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot PR #53109 still needs a follow-up pass. Quick triage
Next step
Run: https://github.com/github/gh-aw/actions/runs/31953024314
|
|
@copilot PR #53109 still needs a follow-up pass. Quick triage
Next step
Run: https://github.com/github/gh-aw/actions/runs/31953623248
|
Current head already contains the requested fixes in |
Current head already contains these fixes in |
|
🎉 This pull request is included in a new release. Release: |
Run 31942623626 reported "no safe outputs" even though the agent successfully called
create_discussion. The agent job'sUpload agent artifactsstep timed out against blob storage; since that step iscontinue-on-error: true, the job still reported success but theagentartifact was never created. Agent output reaches downstream jobs only through that artifact, soDownload agent output artifactfailed,GH_AW_AGENT_OUTPUTwas never set, andprocess_safe_outputs.cjssilently processed zero items.The fix decouples safe-output transport from the large, failure-prone artifact by also uploading a tiny dedicated copy, and matching both on download.
Upload
constants.AgentOutputFallbackArtifactName = "agent-output-fallback".generateAgentOutputFallbackUploademits an upload step immediately before the unified agent upload, carrying onlyagent_output.jsonandsafeoutputs.jsonl(if: always(),continue-on-error: true,if-no-files-found: ignore). Skipped when the workflow declares no safe outputs. Emitted after secret redaction so step-order validation still holds.Download
ArtifactDownloadConfiggainsFallbackArtifact. When set,buildArtifactDownloadStepsswitches fromname:to a brace pattern withmerge-multiple: true, which extracts intopathidentically — downstream file paths are unchanged.buildAgentOutputDownloadSteps(safe_outputs, detection, evals, conclusion) and the custom safe-jobs download.If both artifacts are missing the step still fails and the env var stays unset, preserving existing behavior.
Notes
agent-output-fallbackrather than reusingconstants.AgentOutputArtifactName("agent-output") because the CLI has legacy flattening logic keyed on a directory of that exact name..lock.ymlfiles regenerated.