fix(ship): recover original units after post-approval changes - #2414
coreplane-switchboard[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Changes requested: Recovery can proceed on superseded or dismissed external review evidence.
Warning
Changes requested · head 387d032 · 2 findings: 1 major, 1 minor
| Severity | Finding | Where |
|---|---|---|
| major | F1 Competing later approval by another reviewer does not invalidate the trigger | src/channels/adminCoordinator.ts:2275 |
| minor | F2 Dismissal after the full-review spawn does not fence later coding | src/channels/adminCoordinator.ts:909 |
Full review
F1 (major, high confidence): laterRecoveryReview rejects a later decision from the same reviewer, but not an approval from a different reviewer. If reviewer A requests changes and reviewer B later approves the unchanged head, recovery still accepts A’s request as its trigger. Treat a competing later decision as stale evidence, and add that case to the refusal tests.
F2 (minor, medium confidence): The external review is revalidated before the first read-only child, but not before the subsequent findings spawn. If GitHub dismisses it while that child runs, coding can still start from the claimed recovery. Recheck the claimed review before admitting later children, or explicitly establish why the independent typed review remains sufficient authority after dismissal.
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
387d032 to
e8db530
Compare
There was a problem hiding this comment.
Changes requested: Later cross-reviewer approvals can leave a stale trigger authorized, and cost-cap settlement can strand a pushed findings head.
Warning
Changes requested · head e8db530 · 2 findings: 2 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Recheck competing approvals across reviewers before coding | src/channels/adminCoordinator.ts:2299 |
| major | F2 Reconcile a completed findings push before cost-cap termination | src/core/ship/coordinator.ts:1998 |
Full review
F1 (major, high confidence): The initial selection rejects an approval from another reviewer after the changes request, but claimedRecoveryReviewValid checks only the claimed review. If another reviewer approves after the full read-only child runs, the findings child can still be admitted on a superseded trigger. Recheck later competing decisions across reviewers before admitting each subsequent child. The dismissal fence is present; this is a separate supersession gap.
F2 (major, high confidence): A findings child can complete and push a new head that takes cumulative spend over the cost cap. settleCoding then aborts before pr-check reconciles that push, leaving the durable publication binding at the old head and consuming the recovery on settlement. Reconcile the completed push before ending the unit without dispatching another child.
An original Ship unit that reached merge-ready can resume after a later authorized changes-requested review at its unchanged PR head. Recovery runs a full review first and carries the original identity, lease, rounds, grant, and cumulative spend forward.
Why: Issue #2412 blocks correcting #2411 after its first approval. A fresh unit would lose the original writer and budget limits. This repair creates a guarded continuation of that unit before any code fix or retry is admitted.
Where to look
Feedback wanted: Check reviewer authorization, exact-head and original-unit binding, complete priced child history, lease and round enforcement, and the #2411 timing regression.
Risk: A false admission could start a duplicate or over-budget writer. Missing or conflicting identity, review, head, child, lease, or cost evidence must fail closed. No live #2411 recovery is claimed here.
Verified: Initial coding run reported 704 focused tests; the findings round passed focused F1/F2 tests. Current-head CI and CodeQL are green; re-review and live #2411 recovery remain open.
Decisions (3)
Validation (4 criteria)
src/channels/adminCoordinator.test.ts::POST /admin/coordinator/recover-unit — unchanged-head original-unit recovery::post-approval recovery*src/core/ship/coordinator.test.ts::original-unit recovery accounting::*;src/channels/adminCoordinator.test.ts::POST /admin/coordinator/recover-unit — unchanged-head original-unit recovery::post-approval recovery*src/core/coordinator/driver.test.ts::runOriginalUnitRecovery::starts a full original-unit review*;src/core/coordinator/briefs.test.ts::composeChild — the child a brief names::briefs the post-approval review*For agents
Original Ship thread and unit
plan-fix-p0-issue-2412-from-c-85c007:U1own this PR. Keep #2411 held until this source is reviewed, person-merged, released, and proved live; do not wake its original unit or nominal U4 from this PR. Legacy publication/budget recovery for website #405 and #2407 is a separate serialized #2341 gate. This body was repaired at the exact PR head without changing code or branch.