Retry Copilot startup no-output watchdog failures in scheduled/push runs - #48514
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extends scheduled/push startup recovery to recognize watchdog-triggered no-output exits.
Changes:
- Adds a shared startup retry predicate and watchdog state tracking.
- Expands retry diagnostics and policy tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/copilot_harness.cjs |
Expands startup retry handling and diagnostics. |
actions/setup/js/copilot_harness.test.cjs |
Adds watchdog retry policy coverage. |
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: 1
- Review effort level: Medium
| // This covers both exit code 2 interruptions and post-result watchdog idle exits | ||
| // (exit code 1 with watchdogFired=true), where there is no partial session state to | ||
| // continue from (Turns=0 driver-handoff failure). | ||
| if (isStartupRetryEligible && isStartupNoOutputRetryCandidate(result) && scheduledExit2Retries < MAX_SCHEDULED_EXIT2_RETRIES && attempt < maxRetries) { |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48514 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (4 tests)
Quality Highlights✅ 100% design tests — All new tests directly verify startup retry policy invariants (the core behavioral change in this PR) Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /codebase-design, and /tdd — requesting changes on one correctness issue and two follow-on improvements.
📋 Key Findings
Correctness concern (blocking)
- Dead retry branch (line 1395): The post-result watchdog is armed only after
hasTerminalSafeOutputreturns true, meaningwatchdogFired === trueimplies the process produced output — making!hasOutput && watchdogFiredunreachable in practice. The new retry path silently does nothing. The PR should verify this state combination is reachable by checking the actualagent-stdio.logfrom the triggering failure.
Non-blocking improvements
- Stale naming (line 79):
MAX_SCHEDULED_EXIT2_RETRIES,scheduledExit2Retries,scheduledExit2RetryAttemptedstill encode old exit-code-2 semantics; renaming to matchisStartupNoOutputRetryCandidatewould complete the conceptual cleanup. - Test predicate duplication (test line 407):
isStartupNoOutputRetryCandidateis inlined twice in tests rather than imported from the production module — drift risk if the predicate changes.
Positive Highlights
- ✅ Good extraction of the candidate predicate into a named function — improves readability
- ✅ Diagnostic messaging clearly distinguishes the two failure modes
- ✅ Negative test cases (watchdog-not-fired) properly constrain the retry boundary
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.7 AIC · ⌖ 7.8 AIC · ⊞ 6.7K
Comment /matt to run again
| // This covers both exit code 2 interruptions and post-result watchdog idle exits | ||
| // (exit code 1 with watchdogFired=true), where there is no partial session state to | ||
| // continue from (Turns=0 driver-handoff failure). | ||
| if (isStartupRetryEligible && isStartupNoOutputRetryCandidate(result) && scheduledExit2Retries < MAX_SCHEDULED_EXIT2_RETRIES && attempt < maxRetries) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The watchdogFired === true && !hasOutput condition is contradictory: the post-result watchdog is armed only after hasTerminalSafeOutput(safeOutputsPath) returns true (line 1151), which requires safe-output to exist — implying hasOutput is true when the watchdog fires. This means the new watchdog-based retry branch can never trigger in practice.
💡 Root cause analysis
The watchdog's shouldArm predicate is () => hasTerminalSafeOutput(safeOutputsPath). A process that exits code 1 via watchdog will have produced terminal safe-output, so hasOutput will be true. The retry candidate predicate !result.hasOutput && exitCode === 1 && watchdogFired therefore requires a state combination the current wiring cannot produce.
Before merging: pull the actual agent-stdio.log from the failing Daily Issues Report Generator run and confirm whether hasOutput=false watchdogFired=true appears. If it does not, this branch is dead code.
@copilot please address this.
| // with code 1 after the post-result watchdog terminates an idle turns=0 run. | ||
| // Push-triggered runs share the same transient startup-failure mode as scheduled runs; the | ||
| // retry budget is equally useful there to avoid a deterministic red on push/main. | ||
| const MAX_SCHEDULED_EXIT2_RETRIES = 1; |
There was a problem hiding this comment.
[/codebase-design] MAX_SCHEDULED_EXIT2_RETRIES, scheduledExit2Retries, and scheduledExit2RetryAttempted now have stale names after this PR broadens the policy to include watchdog failures. The variable names still encode the old exit code 2 semantics, making the code harder to navigate for future readers.
💡 Suggested renaming
const MAX_STARTUP_NO_OUTPUT_RETRIES = 1; // was MAX_SCHEDULED_EXIT2_RETRIES
let startupNoOutputRetries = 0; // was scheduledExit2Retries
let startupNoOutputRetryAttempted = false; // was scheduledExit2RetryAttemptedA one-shot rename keeps the naming consistent with the new isStartupNoOutputRetryCandidate predicate that was already introduced in this PR.
@copilot please address this.
| function shouldRetry(result, attempt, isStartupRetryEligible, scheduledExit2Retries) { | ||
| if (result.exitCode === 0) return false; | ||
|
|
||
| const isStartupNoOutputRetryCandidate = !result.hasOutput && (result.exitCode === 2 || (result.exitCode === 1 && result.watchdogFired === true)); |
There was a problem hiding this comment.
[/tdd] The isStartupNoOutputRetryCandidate predicate is duplicated inline in both test helpers (shouldRetry at line 407 and shouldEmitIncomplete at line 480) instead of being imported from the production module. If the production predicate changes, the tests won't catch a drift between the two copies.
💡 Import the function instead
Export isStartupNoOutputRetryCandidate from copilot_harness.cjs and import it in the test file:
const { isStartupNoOutputRetryCandidate } = require('./copilot_harness.cjs');This makes the tests actually validate the production implementation rather than a hand-copied mirror.
@copilot please address this.
There was a problem hiding this comment.
Review: Retry Copilot startup no-output watchdog failures
The approach and rationale are sound — extending the startup retry predicate to cover exitCode=1, watchdogFired=true correctly addresses the Daily Issues Report Generator failure pattern. The isStartupNoOutputRetryCandidate helper is clean and reused consistently across retry, logging, and incomplete-emission paths.
One blocking concern: the startup no-output retry gate does not guard against the case where hasTerminalSafeOutput is already true from a prior attempt's write. The failureClass === "partial_execution" guard at ~line 1273 only protects against partial_execution/long_run_exit failures — when hasOutput=false, failureClass is "no_output" and that guard is bypassed. If a prior attempt already wrote a terminal safe-output but still exited with code 1 (watchdog fired, no console output), the new predicate would classify it as a startup failure and issue a spurious retry.
See the inline comment for a targeted fix.
Tests look good — the added cases cover both the happy path (watchdog no-output retries) and the negative case (exit 1 without watchdog). No duplication of existing coverage.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 51.2 AIC · ⌖ 5.86 AIC · ⊞ 5K
| // This covers both exit code 2 interruptions and post-result watchdog idle exits | ||
| // (exit code 1 with watchdogFired=true), where there is no partial session state to | ||
| // continue from (Turns=0 driver-handoff failure). | ||
| if (isStartupRetryEligible && isStartupNoOutputRetryCandidate(result) && scheduledExit2Retries < MAX_SCHEDULED_EXIT2_RETRIES && attempt < maxRetries) { |
There was a problem hiding this comment.
The existing review comment raises a concern about watchdogFired being true in a startup context, and there is a related correctness gap worth addressing.
If a prior attempt already wrote terminal safe-output to disk (e.g. from a partial-execution run that produced output), the watchdog on the next attempt arms on the first poll interval, fires before the process produces any console output, and yields hasOutput=false, watchdogFired=true, failureClass=no_output.
The existing terminal-safe-output guard at ~line 1273 only fires for failureClass === "partial_execution" or "long_run_exit". When hasOutput=false, failureClass is "no_output" so this guard is bypassed — and the startup no-output retry can fire even though a terminal safe-output was already written by the prior attempt.
Suggested fix: add a hasTerminalSafeOutput guard to the startup retry condition:
if (
isStartupRetryEligible &&
isStartupNoOutputRetryCandidate(result) &&
!(safeOutputsPath && hasTerminalSafeOutput(safeOutputsPath)) &&
scheduledExit2Retries < MAX_SCHEDULED_EXIT2_RETRIES &&
attempt < maxRetries
) {This prevents a spurious fresh retry when watchdogFired=true originates from stale safe-outputs arming the watchdog, rather than a true startup-only failure.
@copilot please address this.
PR Triage
Fixes false-negative failures for scheduled/push runs when startup watchdog triggers. High urgency (masks real failures), but has two CHANGES_REQUESTED reviews outstanding — resolve before merge.
|
|
@copilot run pr-finisher skil |
…startup retry Per reviewer feedback: the postResultWatchdog is only armed after hasTerminalSafeOutput is true, so watchdogFired cannot indicate a startup failure. A run with watchdogFired=true and hasOutput=false means the agent completed its task (wrote safe-output to disk) but produced no stdio output before the watchdog terminated the idle process. - Remove watchdogFired from isStartupNoOutputRetryCandidate (exit code 2 only) - Extend the late-activity suppression to also handle no_output + watchdogFired + hasTerminalSafeOutput, treating it as success rather than a startup retry - Remove lastWatchdogFired tracking (no longer needed for startup predicate) - Update startup retry comment/log to mention exit code 2 only - Update tests: remove watchdog startup retry cases, add late-activity suppression test for no-stdio-output + watchdogFired + hasTerminalSafeOutput - Fix pre-existing smoke-copilot-auto.lock.yml drift from merge Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in the latest commit. The approach is now correct:
|
Daily Issues Report Generatorwas failing inExecute GitHub Copilot CLIon a turns=0 startup path (failureClass=no_output) where the harness exited with code 1 after watchdog idle termination. The startup retry policy only handled exit code 2 no-output failures, so this path failed immediately instead of taking the one allowed fresh retry.Startup no-output retry policy (harness)
exitCode === 2, orexitCode === 1 && watchdogFired === trueState tracking + diagnostics alignment
lastWatchdogFiredacross attempts.exit code 2vswatchdog idle exit (exit code 1)while preserving turns=0 framing.Targeted policy coverage in tests
Run URL: https://github.com/github/gh-aw/actions/runs/30337076577
pr-sous-chef run https://github.com/github/gh-aw/actions/runs/30345098513