fix(harness): preserve coding work through model outages - #2136
Conversation
There was a problem hiding this comment.
Changes requested: The ending salvage can still let an unfinished, previously pushed coding child proceed to review, and the durable pushed-head spec is stale.
Warning
Changes requested · head 887e774 · 2 findings: 1 major, 1 minor
| Severity | Finding | Where |
|---|---|---|
| major | F1 A clean already-pushed failure bypasses the WIP checkpoint | src/core/codingPrPostStep.ts:329 |
| minor | F2 Spec contradiction — run-history.md item 2: pushed records now retain source and ending salvage | docs/reference/specs/run-history.md:14 |
Full review
F1 — Major, high confidence. If a coding child pushes an intermediate commit, then fails while its tree is clean, salvageBudgetPush returns “nothing to preserve” without recording a by: "salvage" checkpoint. The coordinator sees only the earlier ordinary push and proceeds through PR recovery into review, treating unfinished work as review-ready. Create and push a WIP marker commit on every abnormal ending, including when the prior ordinary push left a clean tree, so the coordinator reliably aborts with the resumable checkpoint.
F2 — Minor, high confidence. Item 2 still defines pushed as { ref, sha }[] and says only budget and compaction salvage produce by: "salvage". The implementation now persists by and adds ending salvage. Update the behavior statement to match the new durable record contract.
The unlicensed codingPrPostStep.test.ts title change is a refactor with verification intact; its replacement strengthens coverage by adding an untracked file.
887e774 to
58a24d6
Compare
There was a problem hiding this comment.
LGTM: Both prior findings are resolved: clean abnormal endings now leave a salvage-marked WIP head, and the durable record spec documents its source and compatibility.
Note
Approved · head 58a24d6 · no findings
Full review
Both prior findings are resolved. The coding post-step test title changes are refactors with verification intact and strengthened abnormal-ending coverage.
82e39f0 to
05c75e4
Compare
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
05c75e4 to
2024440
Compare
There was a problem hiding this comment.
LGTM: The replacement docs checks pass and no unresolved code or spec findings remain.
Note
Approved · head 2024440 · no findings
Full review
The prior docs CI finding is resolved; the test-title changes preserve or strengthen their verification.
Transport failures now hold and retry model turns inside the run lease. If a coding child still ends, its branch gets a WIP checkpoint so interrupted work resumes instead of being reviewed or discarded.
Why: Issue #2120 showed a gateway failure could erase half an hour of coding. This closes the provider-hold record at the execution boundary so outages spend lease time rather than work.
Where to look
Feedback wanted: Check the held-turn wake/backoff against the lease and that every abnormal ending leaves salvage—not ordinary-push—provenance.
Risk: This is over 400 changed lines: 22 files, +662/-254. Bugs could stall turns or checkpoint the wrong branch; rollback is this commit. I considered separate PRs, but retry exhaustion is the checkpoint boundary joining both fixes.
Verified: 594 focused tests and local docs, TypeScript, format, hygiene, specs, and title gates pass. Two replacement docs CI checks passed; fresh CI on 2024440 remains GitHub-gated.
Decisions (4)
git add -u: a new source or regression test can be the most valuable interrupted work.git add -Apreserves it while ignore rules still exclude caches, credentials, and staged attachments.Validation (7 criteria)
npx vitest runon the 7 changed test files after the final rebase — 7 files passed, 594 tests passed.npm run verify -w docs && npm run check:site && npm run verify -w deploy/cloudflare-docs— passed locally; two replacementci / docschecks also completed successfully.NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json— passed after the final rebase.npx prettier --checkon all 22 changed files — passed after the final rebase.npm run hygiene:check— passed; 9 files, 37 hits, 138 allowed lines.npm run specs:check— passed; 50 specs and 5600 proof references checked.npm run check:pr-title -- "fix(harness): preserve coding work through model outages"— passed.For agents
Rebased onto origin/main at 1bec076 before the final fast gates. The branch is one coherent commit at 2024440; its tree matches 82e39f0 and 05c75e4, amended only to retrigger Depot legs that failed without logs or annotations while passing at the identical tree on adjacent runs. The agent-ship conflict retained main's empty-check refire behavior and this PR's salvage-provenance rule. No generated files were edited.
Requested by @justinhelmer in slack:C0BRRHKFLCB
🤖 Generated with Claude Code