Rescue completed watchdog-fired Copilot runs from false authentication_failed classification - #49792
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
authentication_failed classification
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Extends Copilot harness recovery to preserve completed runs falsely classified as authentication failures.
Changes:
- Rescues watchdog-fired
authentication_failedruns with terminal safe-output. - Adds end-to-end regression coverage for benign
gh auth loginoutput.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/copilot_harness.cjs |
Expands post-result watchdog suppression. |
actions/setup/js/copilot_harness.test.cjs |
Tests the false-authentication failure scenario. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is minimal, correct, and well-tested.
- The
authentication_failedrescue is properly gated by bothresult.watchdogFired === trueandhasTerminalSafeOutput(safeOutputsPath), so genuine auth failures that occur before any terminal safe-output is produced remain classified as failures — no regression risk. - The regression test faithfully reproduces the observed failure shape (benign auth text in tool output, watchdog fires,
callCount === 1verifies no retry, exit 0).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.6 AIC · ⌖ 10.3 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — two minor suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Readability: The rescue
ifcondition is now long enough to deserve a named intermediate variable (isExpectedLateExit) — easier to audit and extend safely. - Test coverage gap: The new test proves the rescue works, but there's no negative case confirming that a genuine auth failure (no terminal safe-output) still propagates correctly.
Positive Highlights
- ✅ Minimal, targeted fix — exactly one new clause added to the existing rescue logic
- ✅ Rescue is correctly gated on both
watchdogFiredANDhasTerminalSafeOutput, so real auth failures are unaffected - ✅ Regression test faithfully reproduces the observed failure shape (benign auth text in tool payload + watchdog + terminal safe-output)
- ✅ PR description includes a clear behavioral example
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.6 AIC · ⌖ 8.03 AIC · ⊞ 7.1K
Comment /matt to run again
| // output before the watchdog terminated the idle process. | ||
| if ((failureClass === "partial_execution" || failureClass === "long_run_exit" || (failureClass === "no_output" && result.watchdogFired)) && safeOutputsPath && hasTerminalSafeOutput(safeOutputsPath)) { | ||
| if ( | ||
| (failureClass === "partial_execution" || failureClass === "long_run_exit" || (failureClass === "no_output" && result.watchdogFired) || (failureClass === "authentication_failed" && result.watchdogFired)) && |
There was a problem hiding this comment.
[/diagnosing-bugs] The rescue condition is growing long; extracting each clause to a named boolean would make the intent self-documenting and guard against future mis-reads.
💡 Suggested refactor
const isExpectedLateExit =
failureClass === "partial_execution" ||
failureClass === "long_run_exit" ||
(failureClass === "no_output" && result.watchdogFired) ||
(failureClass === "authentication_failed" && result.watchdogFired);
if (isExpectedLateExit && safeOutputsPath && hasTerminalSafeOutput(safeOutputsPath)) {This keeps the rescue logic readable and makes it easy to add new cases without scanning one very long line.
@copilot please address this.
There was a problem hiding this comment.
Extracted the condition into isExpectedLateExit as suggested. The four-clause guard is now a named boolean, making it easy to read and extend.
| GH_AW_HARNESS_WATCHDOG_TIMEOUT_MS: "100", | ||
| }, | ||
| encoding: "utf8", | ||
| timeout: 15000, |
There was a problem hiding this comment.
[/tdd] The test verifies the happy path well, but does not assert the negative case: that a genuine authentication_failed (no terminal safe-output yet) is not rescued. Without that guard, a future regression could silently swallow real auth failures.
💡 Suggested companion test
Add a sibling test where safeOutputsPath points to an empty file (no terminal safe-output entry), the watchdog fires, and the harness exits non-zero (or retries). The structure mirrors this test but omits the add_comment write to the safe-outputs file before the watchdog fires.
@copilot please address this.
There was a problem hiding this comment.
Added the negative test: "does not rescue authentication_failed when no terminal safe-output was produced before the watchdog fires". The stub emits "Error: No authentication information found." and exits 1 without writing any safe-output entry; the harness exits non-zero and the late-activity exit suppressed log line is absent.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (1 test)
AnalysisTest Summary: Why it matters: Assertion quality: 4 distinct checks cover the essential guarantees: single attempt (no retries), correct exit code (0), and proper logging. All are direct behavioral assertions (exit code, process count, stderr messages). Verdict
|
|
@copilot Please address the remaining PR follow-up and then run the pr-finisher skill. Current signals to close:
Please summarize whether any concrete code/test change is still needed versus this being ready for human review once checks settle.
|
…h test, fix stale contract test Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please continue triage on this PR, then run the Current signals:
If the code is already correct on the current head, say that clearly in a short handoff and note any exact follow-up still needed.
|
…event test timeouts Adds the same AWF_REFLECT_ENABLED guard used in copilot_harness.cjs around both fetchAWFReflect calls in codex_harness.cjs. Without this guard, the codex harness unconditionally makes a network request to api-proxy with a 60-second timeout. In CI environments where api-proxy is accessible but slow, spawnSync tests with 10-second timeouts would be killed (returning null status) rather than completing cleanly. Also removes the unused AWF_REFLECT_TIMEOUT_MS dead import." Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The root cause was that The fix adds the same |
The PR is ready for maintainer review. Here's the current state: Failing JS check (run 30754151275) — fixed. Root cause: Review threads — both substantively answered by Copilot:
Branch is merged from main (no conflicts, Hand-off: A maintainer needs to re-trigger CI (close/reopen or push) to get a fresh green run before merge. No further code changes are needed. |
|
🎉 This pull request is included in a new release. Release: |
copilot_harness.cjscould classify a completed run asauthentication_failedwhen agent-emitted tool output contained benign auth text such asgh auth login. Because the post-result watchdog rescue did not cover that class, completeddaily-experiment-reportruns were discarded after the agent had already produced terminal safe-output.Harness rescue path
actions/setup/js/copilot_harness.cjsto also rescueauthentication_failedwhen:result.watchdogFired === true, andRegression coverage
actions/setup/js/copilot_harness.test.cjsfor the observed failure shape:"not logged in"text from a tool payload,Behavioral example
Run: https://github.com/github/gh-aw/actions/runs/30752949969