fix(ship): review completed same-head salvage

[coreplane-switchboard]

Completed coding children now proceed to pull-request review when a salvage push only republishes the run’s independently observed final head. Finished work no longer aborts as an interrupted checkpoint while genuine WIP remains protected.

Why: Issue #2197 records completed, handed-off work stranded because salvage replayed the same commit. This narrow inference fix complements PR #2203 without changing salvage publication itself.

Where to look

1. Checkpoint inference Exempts salvage only when completed status, submitted handoff, and the independent run head exactly match the salvaged head. ⚠ A broad exemption could send unfinished work to review.
2. Regression and preserved endings Replays run 6081ede5 and proves differing heads, stopped children, failed children, and provider wording retain checkpoint behavior.
3. Ship behavior contract Binds the same-head exception beside push-before-abort while preserving every other mechanical WIP checkpoint.

Feedback wanted: Confirm the run-record head is the correct independent witness and that exact normalized equality is preferable to prefix equivalence.

Risk: Limited to ship coding-run settlement. A false match could review WIP; a false mismatch preserves today’s abort. Roll back this commit to restore the prior inference.

Verified: 133 coordinator tests, scoped TypeScript, Prettier, hygiene, and spec proof checks pass; CI and the post-deploy receipt remain human-gated.

Decisions (2)

• Trust only an independent run head. The salvage record cannot prove its own readiness. The exception compares salvage to the run record’s separate head fact and also requires completed status plus a submitted handoff.
• Require exact normalized heads. Prefix matching was rejected because the request requires the salvage SHA to equal the child’s own final head; normalization accepts canonical casing while strict equality keeps the exception narrow.

Validation (6 criteria)

Criterion	Proof
Completed, handed-off same-head salvage enters review	src/core/ship/coordinator.test.ts test naming run 6081ede5 passed; it reaches the pull-request check and then spawns review round 1.
Differing salvage head still aborts	src/core/ship/coordinator.test.ts differing-head regression passed.
Stopped and failed/provider-transient children retain checkpoints	Stopped and provider-transient regressions passed, including the unchanged provider retry-budget wording.
Targeted ship coordinator behavior remains green	npx vitest run src/core/ship/coordinator.test.ts — 133 passed.
Changed TypeScript is type-safe	NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json — passed.
Changed files and behavioral proof bindings are valid	Prettier check passed; npm run hygiene:check and npm run specs:check passed.

For agents

Rebased onto origin/main immediately before push; it was already current. The fix intentionally changes only interruptedCheckpoint and its settleCoding call site, plus focused tests and agent-ship item 8. Reproduce with: npx vitest run src/core/ship/coordinator.test.ts.
Requested by @justinhelmer in slack:C0BRRHKFLCB

🤖 Generated with Claude Code

[justinhelmer]

Context for review, since this is a one-function change with a subtle failure mode.

The defect is an inference, not the salvage mechanism. interruptedCheckpoint returns a checkpoint whenever any push on the unit branch carries by: "salvage", and settleCoding reads the presence of that checkpoint as proof the child never declared its work ready. That inference is wrong in one case, and that case happened four times on 2026-09-22: the child finished, pushed, submitted its pull-request description and handed off, but its own push never reached pushed[], so salvage re-published the identical commit and the unit aborted with finished work stranded on a branch and no pull request opened.

Two things worth checking hardest in review.

First, the witness. The comparison must take the child's head from the run record's own head fact, which arrives as a sibling of pushed alongside the pull request, the description and the handoff, and never from the salvage entry. Taking it from the salvage entry would compare a value with itself, always pass, and silently turn this into "any salvage push counts as ready", which is strictly weaker than today's behaviour. There should be a test where salvage pushed a commit that differs from the recorded head and the unit still aborts; that test fails if the comparison ever collapses.

Second, the untouched cases. A genuinely interrupted child, a stopped run, a failed run and a provider-transient ending must all abort exactly as they do now.

Fixtures are real runs from today: 6081ede5 in nominal, and the two OpenAI branches whose work this defect stranded, each carrying exactly one commit ahead of main so salvage published the child's own commit rather than a duplicate.

[coreplane-switchboard]

Changes requested: The same-head salvage exception is unreachable through the production read-record path because the child head is never returned.

Warning

Changes requested · head d0a7c46 · 1 finding: 1 major

Severity	Finding	Where
major	F1 Spec contradiction — docs/reference/specs/agent-ship.md item 8: production read-record omits the child head required by the exception	src/core/ship/coordinator.ts:1826

Full review

F1 (high confidence): interruptedCheckpoint requires facts.headSha, but the production /admin/coordinator/read-record response never returns a coding child head; coordinatorRunView omits it, while the response adds handoff and pushed only. Therefore a completed same-head salvage still becomes an interrupted checkpoint and aborts, contradicting item 8. Persist and return the independently observed final head through read-record, then add an adapter-level regression rather than injecting ChildFacts.headSha directly.

[coreplane-switchboard]

Changes requested: The salvage exception is not reachable through the production read-record path because the child head is never returned.

Warning

Changes requested · head d0a7c46 · 1 finding: 1 major

Severity	Finding	Where
major	F1 The production child record never supplies the head required by the salvage exception	src/core/ship/coordinator.ts:1825

Full review

F1 — settleCoding can only take the new exception when facts.headSha is present, but the production read-record response in adminCoordinator.ts never adds the finished child’s head. The unit therefore still treats the same-head salvage as interrupted WIP and aborts. The tests miss this because they inject headSha directly into the pure coordinator. Derive and return the independently recorded final head through the read-record boundary, with an integration test covering that response.

[justinhelmer]

Re-review requested at 9038ee56, scoped to the one finding from the previous head and the diff since it.

The finding was that the exception could never fire in production: interruptedCheckpoint requires the child's observed head, coordinatorRunView did not return one, and the tests passed only because they supplied a field the real adapter never does. The head is now persisted with the run record's other end-of-run facts and returned through read-record beside handoff and pushed, the regression drives the production view rather than a constructed ChildFacts, and the non-vacuity test is unchanged so salvage at a commit differing from the returned head still aborts with the checkpoint.

This is the first pull request reviewed under the new rule: the first pair audits a head in full, and every later round is a single arm scoped to the findings raised plus the diff since. The reason is on issue #2230, along with the correction that the earlier stall was a posting-threshold problem rather than a scope one.

[coreplane-switchboard]

LGTM: Scoped re-review of the prior same-head salvage finding and d0a7c46..9038ee5: the production path and non-vacuity regression now resolve it.

Note

Approved · head 9038ee5 · no findings

Full review

Scoped re-review only: the prior same-head salvage finding is resolved by the d0a7c46..9038ee5 delta, with the production adapter path and differing-head checkpoint regression both proven.

[github-actions]

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.