fix(harness): extend startup retry to push events; add Turns=0 driver-handoff diagnostics#47384
Conversation
…gnostics - Extend exit-code-2 startup retry (MAX_SCHEDULED_EXIT2_RETRIES=1) to push-triggered runs via new isStartupRetryEligible flag (schedule OR push) - Add driver-handoff-turns0 diagnostic log when no output is produced after an attempt, making the Turns=0 signature visible in run logs for cross-engine triage - Update emitInfrastructureIncomplete to fire for push runs (not just schedule), with an explicit reference to the Turns=0 driver-handoff failure signature - Add assistantTurnCount counter in copilot_sdk_session.cjs, incremented on each assistant.message event, logged at session completion (hasOutput + assistantTurns + durationMs) - Update tests: rename isScheduledRun param to isStartupRetryEligible, add push retry test and non-eligible-trigger test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Extends transient Copilot startup recovery to push-triggered workflows and adds Turns=0 diagnostics.
Changes:
- Enables one fresh retry for push startup failures.
- Logs driver-handoff and SDK assistant-turn diagnostics.
- Adds retry-policy tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/copilot_harness.cjs |
Extends retry eligibility and diagnostics. |
actions/setup/js/copilot_harness.test.cjs |
Adds push and non-eligible retry tests. |
actions/setup/js/copilot_sdk_session.cjs |
Logs assistant turn counts. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Medium
|
|
||
| const durationMs = Date.now() - startTime; | ||
| log(`session completed: hasOutput=${hasOutput} durationMs=${durationMs}`); | ||
| log(`session completed: hasOutput=${hasOutput} assistantTurns=${assistantTurnCount} durationMs=${durationMs}`); |
| it("retries once for push startup interruption with exit code 2 and no output", () => { | ||
| const result = { exitCode: 2, hasOutput: false }; | ||
| // push events are startup-retry-eligible; first attempt retries, second does not | ||
| expect(shouldRetry(result, 0, true, 0)).toBe(true); | ||
| expect(shouldRetry(result, 1, true, 1)).toBe(false); |
| if (isStartupRetryEligible && lastExitCode === 2 && scheduledExit2RetryAttempted) { | ||
| const triggerLabel = isScheduledRun ? "scheduled" : "push"; | ||
| emitInfrastructureIncomplete( | ||
| `Copilot API interruption (exit code 2) persisted after automatic retry in ${triggerLabel} workflow run. ` + "This is the Turns=0 driver-handoff failure signature. Check the agent-stdio.log for startup diagnostics." |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47384 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Review
The fix is directionally correct — extending startup retry eligibility to push events addresses the Turns=0 deterministic failure on push-triggered workflows. Three blocking issues identified by prior inline comments remain unaddressed:
-
assistantTurnCountnot incremented insendAndWaitfallback (copilot_sdk_session.cjs) — sessions completing via that branch logassistantTurns=0despite having output, making the diagnostic actively misleading. -
Push-event retry test doesn't exercise
GITHUB_EVENT_NAME(copilot_harness.test.cjs) — the test passes a precomputedtrueflag; the production guardprocess.env.GITHUB_EVENT_NAME === "push"is never covered. -
emitInfrastructureIncompletecan fire on successful retry (copilot_harness.cjs) — the guard fires wheneverlastExitCode === 2 && scheduledExit2RetryAttempted, even if the retry succeeded and a later attempt failed for an unrelated reason.
Please address the three issues flagged in the inline comments before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 18.6 AIC · ⌖ 7.35 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review
Applied /diagnosing-bugs and /tdd — requesting changes on issues already flagged inline by Copilot.
Key issues:
- assistantTurnCount not incremented in fallback paths (sendAndWait, idle-timeout, post-completion watchdog) — sessions that produced output will log Turns=0, defeating the diagnostic.
- Push-event test passes a precomputed bool, so it never exercises the GITHUB_EVENT_NAME === 'push' expression itself.
- emitInfrastructureIncomplete fires based on final exit code + retry flag alone — a retry that succeeds then later errors still triggers the false-positive message.
Positive highlights:
- Clean, minimal diff scoped to the retry block
- driver-handoff-turns0 diagnostic log is a useful traceability addition
- Non-eligible trigger guard test is solid
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 19.4 AIC · ⌖ 4.51 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Review: non-blocking observations
The core change is correct: extending isStartupRetryEligible to push events is sound, and the driver-handoff Turns=0 diagnostics are a clear improvement. Two medium issues worth fixing before this pattern is reused:
Issues found
1. assistantTurnCount counts streaming chunks, not turns (copilot_sdk_session.cjs:148) — the counter increments on every if (content) fragment, not once per assistant.message event. This inflates assistantTurns=N and makes Turns=0 triage ambiguous. Move the increment outside the content guard.
2. triggerLabel is duplicated and will silently lie for future triggers (copilot_harness.cjs:~1374, ~1406) — isScheduledRun ? "scheduled" : "push" appears twice and produces "push" for any non-scheduled eligible trigger that might be added later. Compute it once as process.env.GITHUB_EVENT_NAME ?? "unknown" at the top of main().
Existing review comments (by @Copilot) on sendAndWait fallback missing turn-count and the emitInfrastructureIncomplete false-positive are also valid and worth addressing.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 32.3 AIC · ⌖ 4.81 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/copilot_sdk_session.cjs:148
assistantTurnCount over-counts on multi-chunk turns: incrementing inside the if (content) block counts streaming fragments, not assistant turns, inflating assistantTurns=N in the log.
<details>
<summary>💡 Suggested fix</summary>
Move the increment outside the content guard so it fires once per assistant.message event:
case "assistant.message": {
const content = event.message?.content?.text ?? "";
if (content) {
hasOutput = true;
output += content;
}
assista…
</details>
<details><summary>actions/setup/js/copilot_harness.cjs:1374</summary>
**`triggerLabel` will silently say "push" for any future non-push eligible trigger**: the expression `isScheduledRun ? "scheduled" : "push"` is also duplicated at ~line 1406, so both sites must be updated if `isStartupRetryEligible` ever covers a third event type.
<details>
<summary>💡 Suggested fix</summary>
Compute it once from the actual event name rather than deriving from `isScheduledRun`:
```js
const triggerLabel = process.env.GITHUB_EVENT_NAME ?? "unknown";Or, if a display-frien…
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (2 tests)
|
|
@copilot run pr-finisher skill |
…ng tests, terminal hasOutput guard Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All three review comments addressed in commit
|
|
@copilot please run the
|
The exit-code-2 startup retry (
MAX_SCHEDULED_EXIT2_RETRIES=1) only fired forschedule-triggered runs. Push-triggered agentic workflows (e.g. Tidy on push/main) that hit a transient Copilot API startup failure exited immediately withTurns=0— no retry, deterministic red.Changes
copilot_harness.cjs— extend retry eligibility to pushisStartupRetryEligible = isScheduledRun || GITHUB_EVENT_NAME === "push", replacing theisScheduledRunguard on the exit-code-2 retry block and onemitInfrastructureIncompletescheduledvspush) and thedriver-handoff Turns=0marker for traceabilitydriver-handoff-turns0diagnostic log whenever any attempt produces no output, making the signature scannable across all event typescopilot_sdk_session.cjs— surface turn count at session endassistantTurnCountcounter incremented on eachassistant.messageeventassistantTurns=Nto allsession completed:log lines; Turns=0 sessions are now directly visible without cross-referencing JSONLcopilot_harness.test.cjs— update and extend retry testsisScheduledRuntest parameter toisStartupRetryEligiblepull_request) do not retry