fix: pass GH_AW_SAFE_OUTPUTS to "Parse agent logs" step to activate graceful fallback#44354
Conversation
…ul fallback The "Parse agent logs for step summary" step was missing GH_AW_SAFE_OUTPUTS in its env block. The log_parser_bootstrap.cjs has a graceful fallback that downgrades setFailed to core.warning when structured log entries are absent but safe outputs exist — but this fallback was dead code because the env var was never set. This intermittent failure pattern (only debug content in agent-stdio.log, no JSON entries) caused the Avenger workflow to fail 5 of 6 runs even though the agent actually completed successfully with safe outputs. Fix: Add GH_AW_SAFE_OUTPUTS env var to the log parsing step when data.SafeOutputs != nil, consistent with applySafeOutputEnvToMap(). Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Status: DRAFT — needs undraft + CI before review. Fixes 5/6 Avenger runs failing with Run §28966928999
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 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. No ADR enforcement needed: PR #44354 does not have the 'implementation' label and has only 28 new lines of code in business logic directories (threshold: 100). |
🤖 PR Triage
Score breakdown: Impact 38 + Urgency 24 + Quality 10 Rationale: DRAFT. Fixes silent missing env-var that prevents GH_AW_SAFE_OUTPUTS from reaching Parse-agent-logs step, causing graceful fallback to never activate. High risk — behaviour change in critical CI path. Needs undraft + CI. Run §28986426320
|
There was a problem hiding this comment.
Pull request overview
This PR updates the gh-aw workflow compiler so that workflows using safe-outputs pass GH_AW_SAFE_OUTPUTS into the “Parse agent logs for step summary” step, activating log_parser_bootstrap.cjs’s existing graceful fallback for Claude when structured logs are missing due to intermittent infrastructure log overwrite during teardown.
Changes:
- Emit
GH_AW_SAFE_OUTPUTS: ${{ steps.set-runtime-paths.outputs.GH_AW_SAFE_OUTPUTS }}ingenerateLogParsing()whensafe-outputsare configured. - Extend
agentic_output_test.goto assertGH_AW_SAFE_OUTPUTSis present specifically in the “Parse agent logs for step summary” step. - Regenerate affected compiled workflow
.lock.ymlfiles so the log-parsing step includes the new env var.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_yaml_ai_execution.go | Threads GH_AW_SAFE_OUTPUTS into the log-parsing step env when safe-outputs are enabled, so the parser can use safe outputs as evidence of successful completion. |
| pkg/workflow/agentic_output_test.go | Adds a targeted assertion that the log-parsing step includes GH_AW_SAFE_OUTPUTS, preventing regressions where the fallback becomes dead code again. |
| .github/workflows/*.lock.yml (259 files) | Regenerated compiled workflows so “Parse agent logs for step summary” includes GH_AW_SAFE_OUTPUTS when safe-outputs are configured. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 259/259 changed files
- Comments generated: 0
- Review effort level: Low
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (1 test)
📋 Test Details
Verdict
The added test code extends Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: ✅ Approve
The fix is correct, minimal, and complete.
What was broken
The graceful fallback in log_parser_bootstrap.cjs (lines 341–347) was dead code. It reads process.env.GH_AW_SAFE_OUTPUTS to count entries and downgrade a setFailed to a core.warning when the agent demonstrably completed, but the env var was never injected into the log-parsing step's env: block.
Fix analysis
compiler_yaml_ai_execution.go — The data.SafeOutputs != nil guard is correct: it ensures GH_AW_SAFE_OUTPUTS is only injected in workflows that actually use safe-outputs (where the graceful fallback applies). Workflows without safe-outputs would never emit safe-output entries, so the guard is semantically appropriate.
agentic_output_test.go — The new test correctly:
- Finds the parse step by name
- Slices to the next step boundary (with fallback for last-step case)
- Asserts the env var is present specifically within that section
Lock files — Verified independently: all 257 workflows with a "Parse agent logs" step now have GH_AW_SAFE_OUTPUTS in that step's env block. No workflows are missing it.
No concerns
The step reference steps.set-runtime-paths.outputs.GH_AW_SAFE_OUTPUTS matches the existing pattern used for the engine execution step and is confirmed present in generated lock files.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 62.6 AIC · ⌖ 6.28 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues, but two improvements worth making.
📋 Key Themes & Highlights
Key Themes
- Off-by-one in boundary slice (
agentic_output_test.goline 112):nextStepIdxis relative to the shifted sub-slice, so the final slice misses one character. Harmless almost always, but correctness matters in test assertions. - Missing negative-path test: no assertion that
GH_AW_SAFE_OUTPUTSis absent from the log-parsing step whensafe-outputsis not configured. Thedata.SafeOutputs != nilguard is the invariant being tested, and right now only the positive path is covered.
Positive Highlights
- ✅ Minimal, surgical fix — one new
ifblock, one new env var, correctly gated ondata.SafeOutputs != nil. - ✅ Root cause clearly identified and described in the PR body.
- ✅ Existing fallback logic in
log_parser_bootstrap.cjsis left untouched — no risk of regressions there. - ✅ 259 lock files regenerated consistently — the diff pattern is uniform and correct.
- ✅ Regression test added for the happy path immediately alongside the fix.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 80.8 AIC · ⌖ 6.95 AIC · ⊞ 6.6K
Comment /matt to run again
| t.Error("Expected 'Parse agent logs for step summary' step to be in generated workflow") | ||
| } else { | ||
| // Find the next step boundary after the parse step | ||
| nextStepIdx := strings.Index(lockContent[parseAgentLogsIdx+1:], " - name: ") |
There was a problem hiding this comment.
[/tdd] The step-boundary extraction is fragile: strings.Index(lockContent[parseAgentLogsIdx+1:], " - name: ") returns an index relative to the shifted slice, so the final slice lockContent[parseAgentLogsIdx : parseAgentLogsIdx+1+nextStepIdx] is off-by-one — it stops one character short of the actual next step boundary. In practice this is usually harmless, but it could cause a false-positive pass if GH_AW_SAFE_OUTPUTS appears exactly at the missed character.
💡 Simpler, correct boundary slice
after := lockContent[parseAgentLogsIdx:]
nextStep := strings.Index(after[1:], " - name: ")
var parseAgentLogsSection string
if nextStep >= 0 {
parseAgentLogsSection = after[:nextStep+1]
} else {
parseAgentLogsSection = after
}By slicing after (rooted at parseAgentLogsIdx) from the start, the index arithmetic stays in one coordinate space.
@copilot please address this.
| // Verify GH_AW_SAFE_OUTPUTS is also present in the "Parse agent logs for step summary" step | ||
| // so the log parser can detect safe-output entries and downgrade a "no structured log entries" | ||
| // failure to a warning when the agent demonstrably completed (e.g. emitted a noop). | ||
| parseAgentLogsIdx := strings.Index(lockContent, "- name: Parse agent logs for step summary") |
There was a problem hiding this comment.
[/diagnosing-bugs] Missing negative-path test: there is no assertion that GH_AW_SAFE_OUTPUTS is absent from the Parse agent logs step env when data.SafeOutputs == nil. The if data.SafeOutputs != nil guard in generateLogParsing() is the critical invariant — without a counterpart test for a workflow without safe-outputs, a future regression that unconditionally injects the var would go undetected.
💡 Suggested addition
Add a small companion test or a sub-case in TestAgenticOutputCollection that compiles a workflow with no safe-outputs frontmatter and asserts GH_AW_SAFE_OUTPUTS does not appear in the Parse agent logs for step summary step section.
@copilot please address this.
There was a problem hiding this comment.
The fix is correct and well-scoped. Two non-blocking issues flagged in the test code.
Review summary
What the PR does
Activates a pre-existing graceful fallback in log_parser_bootstrap.cjs by forwarding GH_AW_SAFE_OUTPUTS into the "Parse agent logs" step env. The fallback logic itself (safeOutputEntriesCount > 0 → core.warning instead of core.setFailed) was already in place and correct — this PR closes the gap where the env var was never visible to it.
Compiler change (compiler_yaml_ai_execution.go)
Correct. The guard if data.SafeOutputs != nil is the right condition and the generated YAML line is consistent with how the same var is forwarded to the engine step.
Test change (agentic_output_test.go)
Two weaknesses flagged (see inline comments):
- Step-boundary detection relies on hard-coded 6-space indentation — if unmatched, the assertion silently tests the entire file tail.
- No negative test covers the
SafeOutputs == nilpath — the conditional guard in the compiler has no coverage for its false branch.
Neither blocks the fix from working in production, but both could hide future regressions.
Lock file regeneration (259 files)
Mechanical output of the compiler change; each file gets +1 line in the correct position. Consistent across all safe-outputs-enabled workflows.
🔎 Code quality review by PR Code Quality Reviewer · 186.7 AIC · ⌖ 5.8 AIC · ⊞ 5.4K
Comment /review to run again
| t.Error("Expected 'Parse agent logs for step summary' step to be in generated workflow") | ||
| } else { | ||
| // Find the next step boundary after the parse step | ||
| nextStepIdx := strings.Index(lockContent[parseAgentLogsIdx+1:], " - name: ") |
There was a problem hiding this comment.
Fragile step-boundary detection: hard-coding 6-space indentation to find the next step will silently over-scan the file if indentation ever changes, making the assertion trivially pass without actually isolating the parse step.
💡 Details
When nextStepIdx == -1 (no match found), the code falls through to the else branch and sets parseAgentLogsSection = lockContent[parseAgentLogsIdx:] — the entire tail of the lock file. The strings.Contains assertion then passes trivially as long as GH_AW_SAFE_OUTPUTS exists anywhere after the parse step, not necessarily inside it.
A more robust boundary is the step's own with: key, which always terminates the env: block without depending on indentation of the next step:
// More precise: scope to the env: block only
withIdx := strings.Index(lockContent[parseAgentLogsIdx:], " with:")
var parseAgentLogsSection string
if withIdx >= 0 {
parseAgentLogsSection = lockContent[parseAgentLogsIdx : parseAgentLogsIdx+withIdx]
} else {
parseAgentLogsSection = lockContent[parseAgentLogsIdx:]
}This is scoped to the step itself and is unaffected by changes to YAML indentation or adjacent step names.
| // demonstrably completed (e.g. emitted a noop). Without this, runs where the container | ||
| // debug log overwrites the JSON stream in agent-stdio.log are permanently red even though | ||
| // the agent ran to completion. | ||
| if data.SafeOutputs != nil { |
There was a problem hiding this comment.
Missing negative-path test: no test verifies that GH_AW_SAFE_OUTPUTS is absent from the log-parsing step when SafeOutputs == nil, so a future regression (e.g. unconditionally emitting the env var) would go undetected.
💡 Suggested addition
Add a test that compiles a minimal workflow without safe-outputs: and asserts the log-parsing step does not contain GH_AW_SAFE_OUTPUTS:
func TestLogParsingStepHasNoSafeOutputsWhenNotConfigured(t *testing.T) {
tmpDir := testutil.TempDir(t, "no-safe-outputs")
testContent := `---
on: push
engine: claude
---
# Minimal workflow
`
testFile := filepath.Join(tmpDir, "test.md")
os.WriteFile(testFile, []byte(testContent), 0644)
compiler := NewCompiler()
if err := compiler.CompileWorkflow(testFile); err != nil {
t.Fatal(err)
}
content, _ := os.ReadFile(filepath.Join(tmpDir, "test.lock.yml"))
lockContent := string(content)
parseIdx := strings.Index(lockContent, "- name: Parse agent logs for step summary")
if parseIdx < 0 {
t.Fatal("parse step missing")
}
withIdx := strings.Index(lockContent[parseIdx:], " with:")
var parseSection string
if withIdx >= 0 {
parseSection = lockContent[parseIdx : parseIdx+withIdx]
} else {
parseSection = lockContent[parseIdx:]
}
if strings.Contains(parseSection, "GH_AW_SAFE_OUTPUTS") {
t.Error("GH_AW_SAFE_OUTPUTS must not appear in log-parsing step when safe-outputs is not configured")
}
}Without this, the if data.SafeOutputs != nil guard in generateLogParsing has no test coverage for the false branch.
|
🎉 This pull request is included in a new release. Release: |
Avenger (and any workflow with
safe-outputs) was failing 5/6 detection-enabled runs withERR_CONFIG: Claude execution failed: no structured log entries were produceddespite the agent completing successfully and emitting safe outputs.Root cause: An intermittent infrastructure condition causes the Claude
--debug-fileto overwriteagent-stdio.logon the host during container teardown, leaving only plain-text debug lines with no JSON.log_parser_bootstrap.cjsalready has a graceful fallback — if the parser name isClaude, no JSON entries were found, butGH_AW_SAFE_OUTPUTSexists with entries, it emitscore.warninginstead ofcore.setFailed. This fallback was dead code becauseGH_AW_SAFE_OUTPUTSwas never set in the "Parse agent logs for step summary" step's env block.Changes:
pkg/workflow/compiler_yaml_ai_execution.go— IngenerateLogParsing(), addGH_AW_SAFE_OUTPUTSto the step env whendata.SafeOutputs != nil:pkg/workflow/agentic_output_test.go— AssertGH_AW_SAFE_OUTPUTSis present specifically in the "Parse agent logs for step summary" step section.259
.lock.ymlfiles — Regenerated; all workflows withsafe-outputsnow include the env var in their log-parsing step.