Skip to content

fix(ship): retain authorized findings push receipts - #2415

Merged
justinhelmer merged 1 commit into
mainfrom
plan/fix-p0-issue-2350-from-c-0bb2a8/u1
Sep 26, 2026
Merged

justinhelmer merged 1 commit into
mainfrom
plan/fix-p0-issue-2350-from-c-0bb2a8/u1

Conversation

@coreplane-switchboard

@coreplane-switchboard coreplane-switchboard Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

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

  1. Exact leased command/result proof Pairs one complete successful bash result with its exact leased source and destination, rejecting ambiguous or compound commands. ⚠ Output or the live GitHub head alone must never establish attribution.
  2. Only committed receipts become restart authority Missing or detached tracking starts uncommitted; only a successful atomic write can publish the receipt and advance authority. ⚠ A skipped or failed receipt commit must leave further publication blocked.
  3. Protect attribution from subsequent tools Both harness gates refuse later calls until attribution settles, while the push's own secondary permission requests remain allowed.
  4. Completed-child evidence gate Historical reconciliation requires complete persisted events, known prices, the original dispatch tag and no competing push evidence. ⚠ Unknown evidence stays refused; no historical spend is inferred.
  5. Original publication authority remains mandatory The reconciled push must match the full child head and fresh same-repository PR/ref before the existing ownership and conditional-update gates.
  6. Mid-run ledger detach regression A real write-through detaches before the successful result; assertions cover skipped commits, restart state, push projections and blocked authority.
  7. Behavior and proof contract Binds receipt persistence, tool fencing, safe historical reconciliation and refusal cases to focused tests, now explicitly including mid-run detachment.

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)
  • One atomic owner of restart authority. The successful result, receipt and restart state share assignLiveState's commit. Receipt authority defaults false even after losing ledger tracking: a local projection is not durable. Only confirmed atomic writes advance the receipt chain and push projection; later ordinary state writes cannot repair missing paired evidence.
  • Refuse subsequent tools rather than block the event feed. Authorization synchronously fences later tool calls until result attribution settles. Awaiting inside a harness feed could deadlock because that feed still owes the push result. The original call's secondary permissions remain allowed; subsequent calls can retry once attribution settles.
  • Use a narrow historical proof language. Accept standalone literal leased pushes, optionally with upstream setup and stderr redirection. Reject compound commands, incomplete output, multiple pushes and unsupported forms rather than infer which operation succeeded.
  • Keep evidence and authority separate. A paired result supplies attribution only. Original child identity, dispatch binding, full head, fresh PR/ref agreement, exclusive ownership and the existing conditional row update still govern admission.
  • Leave recovery accounting separate. Missing or unpriced history cannot mint a budget. Existing cap, round and lease refusals remain; no Ship cannot schedule findings fix when PR lacks publication binding #2341 cost reconstruction or held fix(ship): recover original units after post-approval changes #2414 product fixes are included.
Validation (12 criteria)
Criterion Proof
F1 regression: a detached run cannot treat a skipped commit as success RED: detached matrix case failed because a pushed_head appeared before salvage without a receipt commit. GREEN: all eight matrix cases pass; real write-through detachment skips the atomic call, leaves restart state and pushed projections empty, and blocks publication authority.
F2 remains resolved: later tools cannot delete the ref during attribution Re-ran delayed-tip regression plus pi/relay and opencode/bridge tests. Both gates refuse ref mutation during attribution, allow the push's secondary permission and release the temporary fence after the result.
All changed test files pass npx vitest run on publicationPush, adminCoordinator, dispatch/runLoop, dispatch/reply, pi/relay and opencode/bridge test files: 6 files, 641 passed; exit 0.
Changed TypeScript dependency closure checks NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p /tmp/tsconfig-detached-receipt.json — exit 0. Config extends root, include:[], files=14 changed TS paths.
Changed-file lint and formatting xargs npx eslint < /tmp/detached-receipt-ts; xargs npx prettier --check < /tmp/detached-receipt-files — both exit 0. Initial test-file formatting failure was corrected before the final checks.
No new public-tree imprint; behavioral proof bindings resolve npm run hygiene:check — exit 0. npm run specs:check — 6,019 proof references checked; exit 0.
Sources covered and verification retained npm run specs:coverage -- --changed origin/main...HEAD --test-guard — every changed source covered, six test files inspected; exit 0.
Documentation consistency npm run docs:check — 12 files match; exit 0. No generated files or decision records changed in this round.
Accepted changelog title npm run check:pr-title -- 'fix(ship): retain authorized findings push receipts' — exit 0.
Coherent history, rebased before exact leased push Amended into one commit; git fetch origin main && git rebase origin/main — current at 39607a2. Final fast gates passed, then exact force-with-lease push advanced 24507e2 to 440237d.
CI checks at the pushed head At 440237d: title, zizmor, deploy-targets and cloudflare/cloudflare-sandbox worker verification succeeded. Remaining CI and CodeQL checks were still running when inspected. Full tests, full typecheck and build are CI-only gates.
Independent exact-head review and held-unit live acceptance Pending: re-review and deployed recovery proof. No merge, approval, deployment or original-unit wake/recovery performed. Actual held-child compatibility and continuation remain deployment-gated; held #2413/#2414 refs and units were not changed.
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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-p0-issue-2350-from-c-0bb2a8/u1 branch from 5c4842b to 24507e2 Compare September 26, 2026 05:27

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-p0-issue-2350-from-c-0bb2a8/u1 branch from 24507e2 to 440237d Compare September 26, 2026 05:39

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@justinhelmer
justinhelmer merged commit cfeca37 into main Sep 26, 2026
30 checks passed
@justinhelmer
justinhelmer deleted the plan/fix-p0-issue-2350-from-c-0bb2a8/u1 branch September 26, 2026 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant