Surface Copilot CLI execution step exit code as an error annotation - #53656
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Surfaces non-zero Copilot CLI exit codes as GitHub Actions error annotations, making silent execution failures diagnosable.
Changes:
- Adds a shared non-zero exit annotation to direct and firewall execution paths.
- Reuses the execution-step name in generated annotations.
- Adds Bash integration coverage and regenerates workflow fixtures.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/copilot_engine_execution.go |
Generates the exit-code annotation. |
pkg/workflow/copilot_exit_code_annotation_test.go |
Tests success, failure, pipefail, and execution paths. |
pkg/workflow/testdata/**/*.golden |
Updates generated golden fixtures. |
.github/workflows/*.lock.yml |
Propagates the generated trap to compiled workflows. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 189/189 changed files
- Comments generated: 0
- Review effort level: Balanced
PR TriageCategory: bug | Risk: medium | Priority: high (score 78)
Recommended action: fast_track
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review: source diff (pkg/workflow/copilot_engine_execution.go + new copilot_exit_code_annotation_test.go) is a single-line trap addition with a named constant to prevent name drift, reusing existing test helpers (runBashWithHome, requireCopilotExecutionStep). No speculative abstractions, unused flexibility, or hand-rolled stdlib replacements found. Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ 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
COMMENT — I don't see a changed-line issue here that should block merge.
Why I'm not blocking this
The trap change is narrowly scoped, preserves the existing exit-code persistence and settings cleanup, and the added tests cover the important failure modes: silent success, explicit non-zero exit, and a pipefail-driven command failure. The generated workflow diffs are the expected mechanical propagation of the shared trap string.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 8.59 AIC · ⌖ 6.79 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The implementation is clean and correct. The ::error:: annotation is emitted exclusively on non-zero exit, the step name is DRYed into a copilotExecutionStepName constant shared with buildCopilotExecutionStep, tests cover unit pinning + bash integration + both command paths, and all lock files are consistently regenerated. No actionable issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 36.5 AIC · ⌖ 8.77 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — approving with one minor suggestion.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Root cause properly addressed: the EXIT trap now surfaces the non-zero exit code as a
::error::annotation, making failure visible where it was previously silent - ✅
copilotExecutionStepNameconstant ties the stepname:and annotation text together — drift between the two is impossible - ✅ Test coverage is thorough: string-level pin (
TestCopilotExitCodeTrap_EmitsErrorAnnotation), bash integration tests covering success/failure/pipefail, and end-to-end step generation tests for both direct and firewall paths - ✅ Follows existing test helpers (
runBashWithHome,requireCopilotExecutionStep) rather than inventing new patterns - ✅ Silent on success — no noise in the happy path
One suggestion
The ::error:: annotation embeds the step name in the message body. GitHub Actions also supports title= which renders it as a distinct header in the annotation panel (see inline comment). Minor quality-of-life improvement, not a correctness issue.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 48.6 AIC · ⌖ 9.94 AIC · ⊞ 7.8K
Comment /matt to run again
| agentExecutionExitCodePath, | ||
| copilotSettingsPath, | ||
| copilotExecutionStepName, | ||
| ) |
There was a problem hiding this comment.
[/diagnosing-bugs] The ::error:: annotation format works, but GitHub Actions also supports a title= parameter that renders the step name as a separate header in the annotation panel — cleaner than embedding it in the message text.
💡 Suggested improvement
Instead of:
echo "::error::Execute GitHub Copilot CLI exited with code $gh_aw_exit_code"Consider:
echo "::error title=Execute GitHub Copilot CLI::exited with code $gh_aw_exit_code"The title= parameter makes the step name a distinct heading in the Checks UI, so the message body can focus on the actionable detail (exited with code N). This avoids the slight redundancy of the step name appearing both as the derived context and in the message text.
This is a quality-of-life improvement — the current form is already a meaningful correctness gain over the status quo.
@copilot please address this.
There was a problem hiding this comment.
Applied in f2fb816438: the trap now emits ::error title=Execute GitHub Copilot CLI::exited with code N, using the shared step-name constant. Updated tests and regenerated fixtures validate the title and exit code.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (132 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
|
… error annotation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@copilot Please address the remaining merge blockers on this PR, then run the
|
|
@copilot consider applying fix to all agentic engine generated code |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The
Execute GitHub Copilot CLIstep can terminate non-zero while the job log looks clean — the agent stream and its post-processing (rpc-messages.jsonl,gateway.md,token-usage.jsonl, AI Credits) complete normally and no##[error]is emitted anywhere, making failure indistinguishable from success.The step's EXIT trap already captured
$?and persisted it to/tmp/gh-aw/agent_execution_exit_code.txtfor the OTLP conclusion span, but never surfaced it in the log. This adds that missing signal on the shared invocation path.Changes
buildCopilotSettingsCleanupAndExitCodeTrap(pkg/workflow/copilot_engine_execution.go): emits a GitHub Actions error annotation when the exit code is non-zero. Applies to both generated command paths (direct and firewall/AWF), so every Copilot workflow gets it — no per-workflow logic touched. Exit-code persistence and settings cleanup are unchanged; successful runs stay silent.copilotExecutionStepNameconstant: used for both the step'sname:and the annotation text so the two cannot drift.copilot_exit_code_annotation_test.go): string-level pin of the trap, plus bash integration tests that execute the real trap (success silent;exit 3and a failingpipefailpipeline surface the true code and still write the exit-code file), following the existing style incopilot_home_expansion_test.go..lock.ymlworkflow files — diff is the trap line only.Generated trap:
Scope
This covers remediation item 1 (explicit exit-code/error line on termination), which is the prerequisite for diagnosing the three affected runs. Item 2 — auditing the safe-outputs post-processing for a swallowed exception — is deliberately not attempted here: with the exit code now visible, a subsequent failure will identify whether the fault is in that path or upstream, rather than guessing.