Repository navigation
Surface zero-token driver exits and repair Avenger and PR Triage startup - #65406
Conversation
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>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Copilot workflows derive the wrong exit-code path, and unrelated work-queue routing is removed.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds engine startup diagnostics and repairs two failing workflow configurations.
Changes:
- Persists and reports validated driver exit codes.
- Fixes Avenger’s npm mount and PR Triage’s model.
- Regenerates 319 workflow lock files.
| File | Description |
|---|---|
.github/skills/agentic-workflows/SKILL.md |
Removes unrelated work-queue routing. |
.github/workflows/avenger.md |
Removes the invalid npm symlink mount. |
.github/workflows/pr-triage-agent.md |
Selects an existing model identifier. |
.github/workflows/*.lock.yml (319 files) |
Propagates source changes and exit-code artifacts. |
actions/setup/js/handle_agent_failure.cjs |
Reads and reports driver exit codes. |
actions/setup/js/handle_agent_failure.test.cjs |
Tests exit-code reporting and validation. |
pkg/workflow/compiler_artifacts_test.go |
Tests fallback artifact inclusion. |
pkg/workflow/compiler_yaml_artifacts.go |
Adds exit codes to fallback artifacts. |
pkg/workflow/compiler_yaml_post_agent.go |
Adds exit codes to unified artifacts. |
pkg/workflow/unified_session_artifact_test.go |
Tests unified artifact inclusion. |
| @@ -2984,13 +2984,16 @@ function buildEngineFailureContext(options = {}) { | |||
| // Derive agent-stdio.log path from the agent output file path (same directory) | |||
| const agentOutputFile = process.env.GH_AW_AGENT_OUTPUT; | |||
| const stdioLogPath = agentOutputFile ? path.join(path.dirname(agentOutputFile), "agent-stdio.log") : "/tmp/gh-aw/agent-stdio.log"; | |||
| const exitCodePath = path.join(path.dirname(stdioLogPath), "agent_execution_exit_code.txt"); | |||
| - `.github/aw/visual-regression.md` | ||
| - `.github/aw/workflow-constraints.md` | ||
| - `.github/aw/workflow-editing.md` | ||
| - `.github/aw/workflow-patterns.md` |
| - Analyze coverage workflows: `.github/aw/test-coverage.md` | ||
| - Render compact markdown charts: `.github/aw/asciicharts.md` | ||
| - Map CLI commands to MCP usage: `.github/aw/cli-commands.md` | ||
| - Choose workflow architecture and patterns: `.github/aw/patterns.md` |
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch PR file list
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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.
Request changes
This still leaves the startup and diagnostics path half-fixed: the new exit-code lookup reads beside GH_AW_AGENT_OUTPUT instead of the real /tmp/gh-aw/agent_execution_exit_code.txt location in Copilot flows, and Avenger removes npm from the sandbox instead of replacing the bad symlink mount.
Blocking themes
actions/setup/js/handle_agent_failure.cjs: Copilot workflows commonly exportGH_AW_AGENT_OUTPUT=/tmp/gh-aw/sandbox/agent/logs/, sopath.dirname(...)now points the new lookup at/tmp/gh-aw/sandbox/agent/agent_execution_exit_code.txtwhile the driver still writes/tmp/gh-aw/agent_execution_exit_code.txt. The new diagnostics therefore disappear in the exact zero-token startup failures this PR is trying to surface..github/workflows/avenger.md: dropping/usr/local/bin/npmavoids the current symlink bind error by removingnpmentirely. Avenger is a CI fixer; if the agent cannot invokenpm, JavaScript based fixes inside the sandbox will fail immediately.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 55.7 AIC · ⌖ 7.53 AIC · ⊞ 20.2K
Comment /review to run again
| mounts: | ||
| - "/usr/bin/make:/usr/bin/make:ro" | ||
| - "/usr/local/bin/node:/usr/local/bin/node:ro" | ||
| - "/usr/local/bin/npm:/usr/local/bin/npm:ro" |
| mounts: | ||
| - "/usr/bin/make:/usr/bin/make:ro" | ||
| - "/usr/local/bin/node:/usr/local/bin/node:ro" | ||
| - "/usr/local/bin/npm:/usr/local/bin/npm:ro" |
There was a problem hiding this comment.
Deleting the npm mount fixes the symlink bind error by removing npm from the sandbox entirely, so Avenger can no longer apply Node or JavaScript CI fixes that need the normal npm entrypoint.
💡 Keep npm available without bind-mounting the broken symlink
/usr/local/bin/npm is the command the agent gets on PATH; mounting only /usr/local/lib/node_modules does not recreate that executable. Please replace the symlink mount with the real npm executable, or a small non-symlink wrapper, instead of deleting npm support. Otherwise this just turns the current startup failure into npm: command not found when the agent tries to repair JS based CI failures.
mounts:
- "/usr/local/bin/node:/usr/local/bin/node:ro"
- "<real npm executable>:/usr/local/bin/npm:ro"
- "/usr/local/lib/node_modules:/usr/local/lib/node_modules:ro"
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd to the driver-exit diagnostics and startup fixes. Existing Copilot review comments already flag the key correctness issue (the GH_AW_AGENT_OUTPUT-is-a-directory case for the Copilot engine) and two unrelated regressions in SKILL.md. I added one supporting comment pinpointing the missing regression test for that exact Copilot-engine scenario, which is the one this PR is meant to fix (Avenger, PR Triage).
📋 Key Themes & Highlights
Key Themes
- Root cause gap:
buildEngineFailureContextderivesexitCodePathviapath.dirname(stdioLogPath), which only matches where the exit-code file is actually written (/tmp/gh-aw/agent_execution_exit_code.txt, perengine_helpers.go) whenGH_AW_AGENT_OUTPUTpoints at a file. For the Copilot engine, it's a directory (/tmp/gh-aw/sandbox/agent/logs/), so the new exit-code surfacing silently no-ops for exactly the workflows (Avenger, PR Triage) this PR sets out to repair. - Test coverage gap: new
handle_agent_failure.test.cjstests cover the file-path case only; no test exercises the directory-path (Copilot) case, so this regression could ship undetected per/tdd. - Unrelated scope creep: the
SKILL.mddiff dropswork-queue.mdfrom the discoverable file list and dispatch rules even though the file still exists in the repo — flagged by the bot, confirmed viagit status, unrelated to this PR's driver/startup scope. - Model fix looks correct:
mai-code-1.1-flashis a validgithub-copilotprovider entry inpkg/cli/data/models.json, so the PR Triage model swap is a legitimate fix (the oldmai-code-1-flash-pickeralias was unresolved for this workflow's context).
Positive Highlights
- ✅ Good regression-test discipline for the "driver exit code" feature in general (missing-log, log-with-stderr, invalid/successful exit code cases are all covered).
- ✅ Avenger's symlink-mount removal is a clean, minimal fix matching the reported failure mode.
- ✅ Artifact-path wiring (
agentExecutionExitCodePathadded to fallback upload +collectArtifactPaths) is consistent and covered by Go tests.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 105 AIC · ⌖ 14.6 AIC · ⊞ 10K
Comment /matt to run again
| @@ -2699,6 +2699,28 @@ describe("handle_agent_failure", () => { | |||
| expect(buildEngineFailureContext()).toBe(""); | |||
| }); | |||
|
|
|||
| it("reports a nonzero driver exit even when the stdio log is missing", () => { | |||
There was a problem hiding this comment.
[/tdd] New tests for the exit-code surfacing feature only exercise GH_AW_AGENT_OUTPUT pointed at a file (agent_output.json), never the Copilot-engine case where it's set to a directory (/tmp/gh-aw/sandbox/agent/logs/, per copilot_logs.go's GetLogFileForParsing). That's exactly the startup-failure scenario this PR targets (Avenger/PR Triage), so the regression this PR is meant to prevent isn't covered.
💡 Suggested test
it("derives exit code path correctly when GH_AW_AGENT_OUTPUT is a directory (Copilot engine)", () => {
process.env.GH_AW_AGENT_OUTPUT = "/tmp/gh-aw/sandbox/agent/logs/";
fs.writeFileSync("/tmp/gh-aw/agent_execution_exit_code.txt", "1");
const result = buildEngineFailureContext();
expect(result).toContain("Driver exit code:** 1");
});With the current path.dirname(stdioLogPath) derivation, this resolves to /tmp/gh-aw/sandbox/agent/agent_execution_exit_code.txt instead of the actual write location /tmp/gh-aw/agent_execution_exit_code.txt (set via agentExecutionExitCodePath in engine_helpers.go), so the new exit-code reporting silently no-ops for Copilot workflows — which includes Avenger and PR Triage, the two workflows this PR explicitly repairs.
@copilot please address this.
There was a problem hiding this comment.
Review Summary
Confirmed the PR's two intended startup fixes are correct and verified:
pr-triage-agent.md:model: mai-code-1-flash-pickercompiles successfully (go run ./cmd/gh-aw compile), resolving the unresolved-alias failure.avenger.md: adding the/usr/local/bin/npmmount fixes the symlink-bind-mount failure seen in the sampled run.
One blocking issue remains (reinforcing an existing unaddressed review comment with additional evidence): the new exitCodePath in buildEngineFailureContext (handle_agent_failure.cjs) is derived from stdioLogPath's directory rather than the fixed artifact path (/tmp/gh-aw/agent_execution_exit_code.txt). For Copilot-engine workflows where GH_AW_AGENT_OUTPUT is a directory (e.g. /tmp/gh-aw/sandbox/agent/logs/, as used by PR Triage itself), this computes the wrong path and the new driver-exit-code surfacing silently never activates for exactly the workflows this PR is trying to repair.
Two other open comments from a prior review (unrelated SKILL.md work-queue removals) remain unaddressed and still look valid/unrelated to this PR's stated scope.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 245.5 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
| @@ -2984,13 +2984,16 @@ function buildEngineFailureContext(options = {}) { | |||
| // Derive agent-stdio.log path from the agent output file path (same directory) | |||
| const agentOutputFile = process.env.GH_AW_AGENT_OUTPUT; | |||
| const stdioLogPath = agentOutputFile ? path.join(path.dirname(agentOutputFile), "agent-stdio.log") : "/tmp/gh-aw/agent-stdio.log"; | |||
| const exitCodePath = path.join(path.dirname(stdioLogPath), "agent_execution_exit_code.txt"); | |||
There was a problem hiding this comment.
agentExecutionExitCodePath is /tmp/gh-aw/agent_execution_exit_code.txt (a fixed, non-sandbox path per pkg/workflow/engine_helpers.go:395), but this derives exitCodePath from path.dirname(stdioLogPath). When GH_AW_AGENT_OUTPUT is /tmp/gh-aw/sandbox/agent/logs/ (Copilot/PR Triage engines, confirmed in pr-triage-agent.lock.yml:1520), stdioLogPath becomes /tmp/gh-aw/sandbox/agent/agent-stdio.log and exitCodePath becomes /tmp/gh-aw/sandbox/agent/agent_execution_exit_code.txt — never matching the real file. The driver-exit surfacing this PR adds will silently never fire for those workflows (same root cause already flagged by the other reviewer at line 2987). Consider deriving exitCodePath independently as path.join(path.dirname(agentOutputFile || "/tmp/gh-aw/agent-stdio.log"), ...)-free, i.e. reuse the fixed constant directly (mirroring AGENT_EXECUTION_EXIT_CODE_PATH in send_otlp_span.cjs:1608) instead of deriving it from stdioLogPath.
@copilot please address this.
|
@copilot merge main |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated all affected golden snapshots to include the exit-code artifact path. Focused golden tests and final impacted validation pass. Fix: |
…gnose-engine-driver-exit Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged |
|
🎉 This pull request is included in a new release. Release: |
Engine failures before agent output were reported without an exit code, obscuring startup faults. Recent Avenger and PR Triage runs also failed on a symlink mount and an unresolved model alias, respectively.