fix(ship): retain authorized findings push receipts - #2415
Conversation
There was a problem hiding this comment.
Changes requested: Two receipt-attribution paths remain unsafe under failed persistence and concurrent workspace changes.
Warning
Changes requested · head 5c4842b · 2 findings: 1 major, 1 minor
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — agent-ship.md item 10: uncommitted receipt can become restart authority | src/core/dispatch/runLoop.ts:623 |
| minor | F2 Spec contradiction — agent-ship.md item 10: successful push can lose its receipt before local tip read | src/core/dispatch/runLoop.ts:534 |
Full review
F1 (major, high confidence): When assignLiveState fails, the run still calls setState with the receipt. That state can persist without the paired result being committed atomically; after a restart, restoredPublicationHead treats it as authority. Retain restart authority only when the receipt commit succeeds.
F2 (minor, medium confidence): The tool-result handler reads the local ref asynchronously, while the harness can continue issuing commands. If a subsequent command moves or deletes that ref before git rev-parse runs, a successful leased push gets no receipt—the failure this change is meant to repair. Capture the pushed head at the execution boundary, or prevent the next tool from changing the ref until attribution completes.
5c4842b to
24507e2
Compare
There was a problem hiding this comment.
Changes requested: F1 remains: a detached ledger run can still project an uncommitted push receipt.
Warning
Changes requested · head 24507e2 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — agent-ship.md item 10: detached runs treat a skipped receipt commit as successful | src/core/dispatch/runLoop.ts:585 |
Full review
F1 (major, high confidence): A coordinator child can start tracked and then lose its ledger connection mid-run. In that state, ledgerRun.tracked() is false, so line 585 treats a successful push as receipt-committed without attempting the atomic write. Lines 632–635 then project the receipt and advance publication authority. If the process dies before the fallback record is written, the push has no durable receipt—the failure this change is meant to prevent. Item 10 requires an unavailable or skipped commit to grant no push authority. Keep the receipt and authority blocked unless the atomic commit succeeds.
F2 is resolved: both gates now fence subsequent tool calls from push authorization through attribution.
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
24507e2 to
440237d
Compare
There was a problem hiding this comment.
LGTM: The detached-ledger receipt-authority bug is fixed; no outstanding findings at this head.
Note
Approved · head 440237d · no findings
Full review
F1 is resolved: a detached run no longer gains push-receipt authority without the atomic commit. No findings remain at this head.
Requested by justin · Thread
Records authorized findings pushes before cleanup and safely reconciles completed children whose receipt is missing. Atomic restart state and a short tool fence prevent false attribution or loss of a successful push, including when ledger tracking is lost mid-run.
Why: The #2350 recurrence on #2413 and #2414 left owned rewrites unattributed. Review F1 exposed detached runs granting authority without a commit. This main-based source repair preserves the original lease; it does not recover the held units.
Where to look
Feedback wanted: Confirm a detached ledger cannot grant publication authority through local projection or final-record fallback. Recheck historical evidence refusal boundaries.
Risk: 1,277 changed lines, 735 in tests. Wrong attribution could admit the wrong head; a stuck fence could refuse later tools. Considered splitting live receipts from reconciliation; retained their shared proof. Revert this PR to restore prior behavior; held units remain untouched.
Verified: 641 focused tests, scoped TypeScript, lint, formatting, hygiene, specs and docs checks passed. New-head CI is running; exact-head review and deployed recovery acceptance remain pending.
Decisions (5)
Validation (12 criteria)
For agents
Current F1 is fixed; F2 was already resolved and remains verified. Head 440237d is one amended commit on main 39607a2. This round changes only runLoop.ts, its test and the agent-ship.md proof row (28 additions, 4 deletions versus 24507e2). No generated files changed. Full focused command: npx vitest run src/core/publicationPush.test.ts src/channels/adminCoordinator.test.ts src/core/dispatch/runLoop.test.ts src/core/dispatch/reply.test.ts src/core/harness/pi/relay.test.ts src/core/harness/opencode/bridge.test.ts. The detached case refuses the preceding tool-call assignment with unknown-run through the real write-through. It proves tracking was true at harness entry and false at the source-head read, no receipt atomic assignment ran, no receipt reached restart state or either push projection, and authority was blocked. Successful durable receipts still survive restart and bounded history trimming. Historical reconciliation is unchanged in this review round and still refuses cost-capped recovery whose cumulative spend is unknown; #2341 remains separate. This PR does not close the live-acceptance portion of #2350.