OTLP conclusion: mark non-zero Copilot CLI exits as run failures when conclusion/error signals are absent#47268
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot merge main and recompile |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47268 does not have the 'implementation' label and has only 17 new lines of code in business logic directories (threshold: 100). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
|
There was a problem hiding this comment.
Pull request overview
Persists Copilot CLI exit codes and uses them to correctly mark OTLP conclusion spans as failures when other conclusion signals are unavailable.
Changes:
- Records direct and firewall execution exit codes through an
EXITtrap. - Adds exit-code-based OTLP status inference and diagnostics.
- Adds regression coverage and recompiles affected workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/copilot_engine_execution.go |
Generates the exit-code persistence trap. |
actions/setup/js/send_otlp_span.cjs |
Reads exit codes and derives OTLP failure status. |
actions/setup/js/send_otlp_span.test.cjs |
Tests non-zero exit failure inference. |
.github/workflows/ace-editor.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/agent-performance-analyzer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/agentic-token-audit.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/agentic-token-optimizer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/agentic-token-trend-audit.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/architecture-guardian.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/archie.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/artifacts-summary.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/bot-detection.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/brave.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/breaking-change-checker.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/ci-coach.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/cli-consistency-checker.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/code-scanning-fixer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/code-simplifier.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/constraint-solving-potd.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/contribution-check.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/copilot-centralization-drilldown.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/copilot-centralization-optimizer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/copilot-cli-deep-research.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/copilot-opt.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/copilot-pr-merged-report.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/copilot-pr-nlp-analysis.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/copilot-pr-prompt-analysis.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/craft.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-agent-of-the-day-blog-writer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-ambient-context-optimizer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-architecture-diagram.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-assign-issue-to-user.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-byok-ollama-test.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-cli-performance.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-cli-tools-tester.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-community-attribution.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-compiler-quality.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-compiler-threat-spec-optimizer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-credit-limit-test.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-experiment-report.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-file-diet.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-firewall-report.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-formal-spec-verifier.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-geo-optimizer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-issues-report.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-malicious-code-scan.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-max-ai-credits-test.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-mcp-concurrency-analysis.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-model-inventory.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-model-resolution.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-performance-summary.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-regulatory.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-repo-chronicle.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-safe-output-integrator.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-security-observability.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-secrets-analysis.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-semgrep-scan.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-sentrux-report.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-skill-optimizer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-spdd-spec-planner.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-syntax-error-quality.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-team-status.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-testify-uber-super-expert.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-windows-terminal-integration-builder.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/daily-workflow-updater.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/dataflow-pr-discussion-dataset.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/dead-code-remover.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/delight.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/dependabot-burner.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/dependabot-go-checker.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/deployment-incident-monitor.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/designer-drift-audit.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/dev-hawk.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/dictation-prompt.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/discussion-task-miner.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/docs-noob-tester.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/draft-pr-cleanup.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/eslint-miner.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/eslint-monster.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/example-failure-category-filter.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/example-permissions-warning.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/firewall-escape.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/firewall.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/functional-pragmatist.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/github-remote-mcp-auth-test.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/glossary-maintainer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/gpclean.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/impeccable-skills-reviewer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/issue-monster.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/issue-triage-agent.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/jsweep.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/layout-spec-maintainer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/lint-monster.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/linter-miner.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/mattpocock-skills-reviewer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/mcp-inspector.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/mergefest.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/metrics-collector.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/notion-issue-summary.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/objective-impact-report.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/org-health-report.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/outcome-collector.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/pdf-summary.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/plan.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/poem-bot.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/pr-code-quality-reviewer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/pr-description-caveman.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/pr-nitpick-reviewer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/pr-sous-chef.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/pr-triage-agent.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/python-data-charts.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/q.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/refactoring-cadence.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/refiner.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/release.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/repo-audit-analyzer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/repo-tree-map.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/repository-quality-improver.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/research.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/schema-consistency-checker.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/security-compliance.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/security-review.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/sighthound-security-scan.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/skillet.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/slide-deck-maintainer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-ci.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-copilot-arm.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-copilot-mai.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-copilot-sdk.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-copilot-small.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-copilot-sub-agents.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-copilot.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-create-cross-repo-pr.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-multi-pr.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-otel-backends.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-pi.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-project.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-service-ports.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-temporary-id.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-test-tools.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-update-cross-repo-pr.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-workflow-call-with-inputs.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/smoke-workflow-call.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/spec-enforcer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/spec-extractor.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/spec-librarian.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/stale-pr-cleanup.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/stale-repo-identifier.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/sub-issue-closer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/super-linter.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/technical-doc-writer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/terminal-stylist.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/test-quality-sentinel.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/tidy.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/ubuntu-image-analyzer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/uk-ai-operational-resilience.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/unbloat-docs.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/update-astro.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/video-analyzer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/visual-regression-checker.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/weekly-blog-post-writer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/weekly-editors-health-check.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/weekly-issue-summary.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/weekly-safe-outputs-spec-review.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/workflow-generator.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/workflow-health-manager.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/workflow-normalizer.lock.yml |
Regenerated Copilot execution trap. |
.github/workflows/workflow-skill-extractor.lock.yml |
Regenerated Copilot execution trap. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 170/170 changed files
- Comments generated: 1
- Review effort level: Medium
| func buildCopilotSettingsCleanupAndExitCodeTrap() string { | ||
| return fmt.Sprintf( | ||
| "trap 'gh_aw_exit_code=$?; mkdir -p /tmp/gh-aw >/dev/null 2>&1 || true; printf \"%%s\" \"$gh_aw_exit_code\" > %s || true; rm -f \"%s\"' EXIT\n", | ||
| agentExecutionExitCodePath, | ||
| copilotSettingsPath, | ||
| ) |
There was a problem hiding this comment.
LGTM
The fix correctly addresses the silent-failure gap in sendJobConclusionSpan.
Key points verified:
- Trap correctness: Single-quoted trap body captures exit code at fire-time before cleanup. HOME expansion also works correctly at evaluation time.
- Priority ordering: Exit-code check takes precedence over error-count check, which is the exact gap being fixed.
- Zero-exit safety: The agentExecutionExitCode !== 0 guard prevents false positives on successful runs.
- Both execution paths covered: Both buildCopilotAWFPathSetup and buildCopilotDirectCommand now use the new trap.
- Test coverage: The new test case directly exercises the regression scenario (exit code 22, zero errors, absent conclusion maps to failure/STATUS_CODE_ERROR).
- Lock files: Consistently regenerated across all 167 workflows.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 46.7 AIC · ⌖ 5.18 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Key Issues
-
Signal loss when exit code + errors coexist (
send_otlp_span.cjs:2080): ThehasAgentExecutionFailureExitCodebranch takes full priority overoutputErrors, silently discarding richer error context. No test covers this case. -
Dead code + stale tests (
copilot_engine_execution.go:89–95):buildCopilotSettingsCleanupTrap()is no longer called in production but was not deleted. Three tests incopilot_home_expansion_test.gocontinue to test it, giving false confidence. The important$HOME-at-trap-fire-time property is not asserted for the newbuildCopilotSettingsCleanupAndExitCodeTrap. -
Missing edge-case tests (
send_otlp_span.test.cjs):readIntegerIfExistsgracefully handles malformed content, but no test locks this in as a regression guard.
Positive Highlights
- ✅ Correct root-cause fix: EXIT trap → persisted file → post-step span reads it. Clean and reliable across fresh shell environments.
- ✅
readIntegerIfExistsis a well-isolated helper with solid null/error handling. - ✅ The new
gh-aw.agent.execution.exit_codeattribute improves OTel query surface. - ✅ Regression test is well-structured with clear Arrange/Act/Assert and good assertions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 77.8 AIC · ⌖ 4.96 AIC · ⊞ 6.7K
Comment /matt to run again
| // agent job's own post-step where needs.<job>.result is not yet visible), fall back to | ||
| // observable failure evidence so gh-aw.run.status and status.code are accurate. | ||
| if (!rawRunStatus && outputErrors.length > 0) { | ||
| if (!rawRunStatus && hasAgentExecutionFailureExitCode) { |
There was a problem hiding this comment.
[/diagnosing-bugs] When hasAgentExecutionFailureExitCode is true AND outputErrors is non-empty, the error messages are silently discarded — the span says only "copilot cli exited with code N" with no hint of actual errors. This makes the non-zero-exit case less diagnosable than the errors-only path.
💡 Suggestion
Combine both signals when both are present:
if (!rawRunStatus && hasAgentExecutionFailureExitCode) {
runStatus = "failure";
statusCode = 2;
const errSuffix = errorMessages.length > 0 ? `; errors detected: ${errorMessages[0]}` : "";
statusMessage = `copilot cli exited with code ${agentExecutionExitCode}${errSuffix}`.slice(0, 256);
} else if (!rawRunStatus && outputErrors.length > 0) {Also add a test: non-zero exit + errors present → message includes both signals.
@copilot please address this.
| // time (matching buildCopilotSettingsCleanupTrap behavior). | ||
| func buildCopilotSettingsCleanupAndExitCodeTrap() string { | ||
| return fmt.Sprintf( | ||
| "trap 'gh_aw_exit_code=$?; mkdir -p /tmp/gh-aw >/dev/null 2>&1 || true; printf \"%%s\" \"$gh_aw_exit_code\" > %s || true; rm -f \"%s\"' EXIT\n", |
There was a problem hiding this comment.
[/tdd] buildCopilotSettingsCleanupTrap() is now dead code — both call sites in production were replaced by buildCopilotSettingsCleanupAndExitCodeTrap(), but the old function was left in place. The three tests in copilot_home_expansion_test.go (lines 106, 308, 444) still test the removed behaviour and give false confidence.
💡 Suggestion
Either:
- Delete
buildCopilotSettingsCleanupTrapand migrate those 3 tests to coverbuildCopilotSettingsCleanupAndExitCodeTrap(preferred — ensures the new trap is tested for the$HOME-expansion-at-fire-time property), or - Keep it as a private helper called by the new function to reduce duplication.
The $HOME-at-fire-time property tested in TestBuildCopilotSettingsCleanupTrap_SingleQuotedSoHomeExpandsAtTrapFire is equally important for the new trap — add an equivalent assertion.
@copilot please address this.
| @@ -5127,6 +5127,34 @@ describe("sendJobConclusionSpan", () => { | |||
| expect(attrs["gh-aw.run.status"]).toBe("success"); | |||
There was a problem hiding this comment.
[/tdd] The new test only covers the "happy path" of the new feature (exit code 22, no errors). The case where agentExecutionExitCode file contains non-integer content (e.g. empty string, "abc", whitespace) is not covered — readIntegerIfExists handles it, but a test would lock that in as a regression guard.
💡 Missing test cases
Add at minimum:
agent_execution_exit_code.txtcontains"abc"→ treated as absent, falls back to error-count logicagent_execution_exit_code.txtcontains" \n"(whitespace only) → same- File absent (already covered by ENOENT in other tests, but explicit is better)
@copilot please address this.
…t-conclusion-status Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Review: OTLP conclusion exit-code fix — mostly correct, several gaps
The core fix (EXIT trap + readIntegerIfExists fallback when both conclusion signals are absent) is sound and closes the original bug. However there are correctness issues that leave residual blind spots.
Blocking issues (2)
-
Non-zero exit code ignored when
rawRunStatusis present (line 2080) — the fix only applies whenagentConclusionANDworkflowRunConclusionare both absent. IfworkflowRunConclusion=successis visible in the post-step environment while the CLI actually exited non-zero, the status is still reported as success. This is the most likely scenario on the class of runs the PR intends to fix. -
Stale exit-code file on self-hosted/reused runners (line 41 in Go) — the trap writes but never deletes the file, and the setup step does not clear it before the run. A prior non-zero exit code survives into the next run and will falsely flag a successful run as failed.
Medium issues (3)
parseIntpermissiveness inreadIntegerIfExistscombined with a non-atomicprintf >write (line 1516)else ifdiscardsoutputErrorsdetail when both failure signals are present (line 2084)gh-aw.agent.execution.exit_codeattribute is absent when file is missing —0vs "trap never fired" are indistinguishable (line 2060)
Low issues (1)
buildCopilotSettingsCleanupTrapis dead production code; its tests give false coverage confidence (line 92)
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 176.5 AIC · ⌖ 5.56 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/send_otlp_span.cjs:2080
Exit code ignored when rawRunStatus is present — a non-zero Copilot CLI exit still reports run.status=success if workflowRunConclusion is set.
<details>
<summary>💡 Details and suggested fix</summary>
The fix correctly handles the case where both agentConclusion and workflowRunConclusion are absent. But it misses the inverse: when workflowRunConclusion returns "success" (or any value other than "failure") while the Copilot CLI actually exited non-zero, rawRunStatus i…
actions/setup/js/send_otlp_span.cjs:2084
else if discards error detail when both exit-code failure and output errors are present — statusMessage will be "copilot cli exited with code N" even when outputErrors contain more specific diagnostics.
<details>
<summary>💡 Details</summary>
The branch order is:
if (!rawRunStatus && hasAgentExecutionFailureExitCode) {
statusMessage = `copilot cli exited with code ${agentExecutionExitCode}`;
} else if (!rawRunStatus && outputErrors.length > 0) {
statusMessage = `errors d…
</details>
<details><summary>actions/setup/js/send_otlp_span.cjs:1516</summary>
**`parseInt` accepts partially-numeric content** — `readIntegerIfExists("22abc")` silently returns `22`, potentially reading a partially-written exit-code file as a valid exit code.
<details>
<summary>💡 Details</summary>
`parseInt("22abc", 10)` returns `22`. `Number.isInteger(22)` is `true`. So if the write
```sh
printf "%s" "$gh_aw_exit_code" > /tmp/gh-aw/agent_execution_exit_code.txtis interrupted mid-write under disk pressure (unlikely but possible), a partial file like "2" or `"…
pkg/workflow/copilot_engine_execution.go:41
Stale exit-code file on self-hosted/reused runners will falsely mark a good run as failed — the setup step does not clear this file before the agent runs.
<details>
<summary>💡 Details</summary>
The EXIT trap only writes /tmp/gh-aw/agent_execution_exit_code.txt — it never deletes it. GitHub-hosted runners are ephemeral, but self-hosted runners and matrix jobs on cached runners reuse the same /tmp between runs. If a prior run left a non-zero exit code in this file and the next run's Co…
pkg/workflow/copilot_engine_execution.go:92
buildCopilotSettingsCleanupTrap is now dead production code — it is never called outside of tests, which still assert against its old output and give false confidence.
<details>
<summary>💡 Details</summary>
All production call sites now use buildCopilotSettingsCleanupAndExitCodeTrap. The old function exists at line 92 with three test cases in copilot_home_expansion_test.go that assert against its single-cleanup-only trap body. Those tests are now testing a code path that is never e…
actions/setup/js/send_otlp_span.cjs:2060
gh-aw.agent.execution.exit_code absent when file is missing makes 0 vs missing ambiguous — consumers cannot tell whether the agent exited cleanly or the trap never fired.
<details>
<summary>💡 Details</summary>
The attribute is only emitted when the file exists and contains a valid integer:
if (Number.isInteger(agentExecutionExitCode)) {
attributes.push(buildAttr("gh-aw.agent.execution.exit_code", agentExecutionExitCode));
}Absence means either:
- Agent step was skipped (t…
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing... |
|
✅ Smoke Copilot Small completed successfully! |
|
✅ Smoke Copilot MAI completed successfully! |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
|
✅ Smoke OTEL completed successfully! |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS 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.
|
Smoke Test: FAIL\n\n✅ GitHub MCP Testing\n✅ Web Fetch Testing\n✅ File Writing Testing\n✅ Bash Tool Testing\n❌ Build gh-aw\n\nOverall status: FAILWarning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Smoke test FAIL
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
Merged PRs: #47288 feat(linters): add stringsjoinone analyzer; #47287 refactor(pkg/workflow): break up 5 large functions exceeding 60-line lint limit Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "aus5.mozilla.org"See Network Configuration for more information.
|
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. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "aus5.mozilla.org"See Network Configuration for more information.
|
|
Smoke test 29923969749
|
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.
|
There was a problem hiding this comment.
Smoke pass walk done. Me leave two tiny inline notes.
📰 BREAKING: Report filed by Smoke Copilot · gpt54 87.5 AIC · ⌖ 8.14 AIC · ⊞ 19.4K
Comment /smoke-copilot to run again
Add label smoke to run again
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! — Run 29923680952
💥 [THE END] — Illustrated by Smoke Claude · sonnet46 90.2 AIC · ⌖ 19.5 AIC · ⊞ 6.1K
Comment /smoke-claude to run again
| const outputItems = Array.isArray(agentOutput.items) ? agentOutput.items : []; | ||
| const errorMessages = outputErrors.map(getErrorMessage).filter(Boolean).slice(0, 5); | ||
| const agentExecutionExitCode = readIntegerIfExists(AGENT_EXECUTION_EXIT_CODE_PATH); | ||
| const hasAgentExecutionFailureExitCode = Number.isInteger(agentExecutionExitCode) && agentExecutionExitCode !== 0; |
There was a problem hiding this comment.
👍 Good use of Number.isInteger() for null safety here. Consider also documenting the fallback behavior when the file is absent (returns null) in a JSDoc comment above readIntegerIfExists to make this contract explicit. — Smoke Test Run 29923680952
| // | ||
| // The body is single-quoted so $HOME in copilotSettingsPath is expanded at trap-fire | ||
| // time (matching buildCopilotSettingsCleanupTrap behavior). | ||
| func buildCopilotSettingsCleanupAndExitCodeTrap() string { |
There was a problem hiding this comment.
🔍 The buildCopilotSettingsCleanupAndExitCodeTrap() function nicely captures the exit code before cleanup. One suggestion: consider using || : instead of || true for POSIX portability in the trap body. — Smoke Test Run 29923680952
|
Smoke tests results:
Overall Status: FAIL
|
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.
|
|
🎉 This pull request is included in a new release. Release: |
gh-aw.agent.conclusioncould emitgh-aw.run.status=success/STATUS_CODE_OKwhen the Copilot CLI execution step failed non-zero butagent_output.jsoncontained no errors. This made a real class of agent-job failures invisible to OTel queries and failure dashboards.Persist Copilot CLI execution exit code for post-step telemetry
/tmp/gh-aw/agent_execution_exit_code.txtfrom anEXITtrap.Extend conclusion span failure inference
send_otlp_span.cjs, added exit-code file parsing and integrated it into run status derivation.GH_AW_AGENT_CONCLUSION/ workflow conclusion are absent and persisted exit code is non-zero, the conclusion span now sets:gh-aw.run.status = "failure"status.code = STATUS_CODE_ERRORstatus.message = "copilot cli exited with code <n>"Expose explicit execution signal in span attributes
gh-aw.agent.execution.exit_codeattribute when the persisted value is available, improving diagnosability and filtering.Add regression coverage for the zero-error/non-zero-exit gap
error_count=0+ non-zero persisted exit code ⇒ failure/ERROR conclusion span.✨ PR Review Safe Output Test - Run 29923680952