copilot_harness: treat post-result watchdog partial executions as successful completions#44548
Conversation
|
Hey A few things to address before this is ready for review:
If you'd like a hand completing this, you can assign the following prompt to your coding agent:
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR metadata shows 0 files changed with 0 additions and 0 deletions. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44548 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Review: No Code Changes to Evaluate
This PR contains a single planning commit with 0 additions and 0 deletions. There are no changed files in the diff, so there is nothing to review at this stage.
Once the implementation lands (harness changes, watchdog logic, tests), a full code review will be possible. The PR description accurately captures the intended approach — the implementation just needs to be written.
🔎 Code quality review by PR Code Quality Reviewer · 28.4 AIC · ⌖ 5.57 AIC · ⊞ 5.4K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — this PR is at the "Initial plan" stage with 0 code changes. The problem statement is accurate and the fix direction is sound, but no implementation has landed yet.
📋 Design Assessment & Required Changes
Root Cause Analysis (/diagnosing-bugs)
The diagnosis is correct: runProcess() has no way to signal why a non-zero exit happened. When the post-result watchdog sends SIGTERM, the close handler sees a non-zero exit code and hasOutput=true, which classifyCopilotFailure maps to "partial_execution" — indistinguishable from a genuine mid-task crash. The proposed terminatedByPostResultWatchdog flag is the right seam to cut.
Implementation Required (/tdd)
The PR has zero code changes today. To be mergeable, the following must land:
-
process_runner.cjs—runProcess()must tracksentSigtermAt > 0atclosetime and exposeterminatedByPostResultWatchdog: booleanin the resolve payload. Update the JSDoc@returnstype too. -
copilot_harness.cjs— Insert the guard before the existing retry decision tree (right afterclassifyCopilotFailure) so it short-circuits before any retry path. -
Env-var parsing — parse
GH_AW_POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MSusing the same guardedNumber/Math.maxpattern already used elsewhere in the harness. -
Tests — see inline comments.
Positive Highlights
- ✅ Guard condition requires all three predicates — low false-positive risk.
- ✅ Only touches
copilot_harness.cjs;claude_harnessandcodex_harnesshave no watchdog and are unaffected. - ✅ Watchdog arming logic (
hasTerminalSafeOutput) is correctly left unchanged.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 135.3 AIC · ⌖ 6.9 AIC · ⊞ 6.6K
Comment /matt to run again
|
@copilot please run the
|
Checked with |
|
@copilot please run the
|
Re-ran |
|
Run: https://github.com/github/gh-aw/actions/runs/29038065496
|
Ran
I can’t trigger new PR CI runs from this agent context; a maintainer must re-run checks (close/reopen PR, workflow_dispatch, or maintainer push). |
Copilot review workflows were reaching terminal output, then being SIGTERM’d by the post-result watchdog and classified as
partial_execution, causing unnecessary retries and false-redaction_requiredoutcomes. This change preserves successful completions when output already exists and termination is watchdog-induced.Runtime signal attribution (process runner)
terminatedByPostResultWatchdogtorunProcess()results so callers can distinguish watchdog-driven termination from other non-zero exits.Harness retry/exit semantics
copilot_harness.cjs, if all of the following are true:terminatedByPostResultWatchdog === truehasOutput === truefailureClass === "partial_execution"exitCode=0) and retry is skipped.Watchdog timeout configurability
GH_AW_POST_RESULT_WATCHDOG_IDLE_TIMEOUT_MSwith explicit default/min constants for clearer control and safer bounds.Regression coverage