fix(ship): keep repeated rebase conflicts with the runner - #2134
Conversation
There was a problem hiding this comment.
Changes requested: Runner ownership is not durable, so a restart can let the sweep take over a pull request still owned by a live pipeline.
Warning
Changes requested · head 859ee41 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Runner ownership disappears across restarts | src/index.ts:704 |
Full review
F1: runnerOwnedPulls starts empty after every process restart and is populated only by later pr-check or rebase requests. A resumed runner already waiting on a review child can therefore remain live without appearing in this set, allowing pulls rebase or the merge watch to force-push its branch or start a detached fix round. Derive ownership from durable coordinator state, or rebuild it during recovery, so every live runner retains sole ownership across restarts.
1a1d83f to
7cf64fa
Compare
There was a problem hiding this comment.
Changes requested: Runner ownership recovery still fails open when the ledger cannot provide the live-run listing.
Warning
Changes requested · head 7cf64fa · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Runner ownership disappears across restarts | src/index.ts:1738 |
Full review
F1: The recovery still fails open on ledger errors. reclaimRuns returns an empty outcome when ledger.reclaim fails and omits foreign-live rows when listLive fails, but guardCards nevertheless sets runnerOwnershipRecovered = true. A sweep can then treat a live runner-owned pull request as unowned and take over its branch. Keep sweeps fenced until a complete live-run listing succeeds, and add coverage for both failure paths.
7cf64fa to
17a569d
Compare
There was a problem hiding this comment.
Changes requested: Whole-listing failures now stay fenced, but an individual hosted-row reclaim failure can still omit a live runner from recovered ownership.
Warning
Changes requested · head 17a569d · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F2 Per-run reclaim failures open the ownership fence | src/core/boot.ts:368 |
Full review
F2: If classification of one rehosted ship row fails—for example, readEvents throws—the row remains live under the current generation but appears in neither resumable nor liveElsewhere. The subsequent successful listing still sets liveListingComplete = true, so RunnerOwnershipFence opens without that runner’s pull request. An operator sweep can then treat the PR as unowned and force-push its branch. Include all live hosted instances from the complete listing in ownership recovery, including current-generation rows whose classification failed, or keep the fence closed until those rows can be classified.
17a569d to
8a69e15
Compare
There was a problem hiding this comment.
Changes requested: The command-mode sweep result changed shape and deterministically breaks the required real-git gate.
Warning
Changes requested · head 8a69e15 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Keep runner-only rebase metadata out of command sweep results | src/core/pullSweep.ts:177 |
Full review
F1 — High confidence. Ordinary command sweeps now return headSha and approvalCarried, so the real-git assertion in src/execution/sweepCheckout.test.ts:120 deterministically fails because it expects the established command result shape. Return this metadata only in runner mode, or otherwise separate the runner’s internal result from the public sweep report; do not weaken the real-git test.
8a69e15 to
fbe465e
Compare
fbe465e to
37d1804
Compare
There was a problem hiding this comment.
Changes requested: The code patch remains approved, but the rebase silently restored three contradictory agent-ship spec rows.
Warning
Changes requested · head 37d1804 · 3 findings: 3 minor
| Severity | Finding | Where |
|---|---|---|
| minor | F2 Spec contradiction — agent-ship.md item 9: conflicts are documented as refused instead of re-entering runner rebase | docs/reference/specs/agent-ship.md:88 |
| minor | F3 Spec contradiction — agent-ship.md item 20: runner-owned conflicts are documented as exhausting the sweep round | docs/reference/specs/agent-ship.md:191 |
| minor | F4 Spec contradiction — agent-ship.md item 21: the merge door is documented as refusing the runner-owned conflict | docs/reference/specs/agent-ship.md:198 |
Full review
F2: The rebase restored the old item 9 behavior: conflicting pull requests are refused and delegated to pulls rebase. Restore the approved row documenting runner re-entry, approval carry/re-review, and repeated automatic rebases.
F3: Item 20 again says a second conflict always exhausts the single model round. Restore the runner-owned distinction; only unowned sweeps have the lifetime-spent behavior.
F4: Item 21 again promises a merge-door refusal and references a removed test. Restore the runner-ownership row and its current proof binding.
There was a problem hiding this comment.
Changes requested: The implementation remains sound, but the rebased agent-ship contract still contradicts it in three places and specs:check fails.
Warning
Changes requested · head 37d1804 · 3 findings: 3 minor
| Severity | Finding | Where |
|---|---|---|
| minor | F2 Spec contradiction — agent-ship.md item 9: approved conflicts are documented as endings and refusals | docs/reference/specs/agent-ship.md:87 |
| minor | F3 Spec contradiction — agent-ship.md item 20: conflict budget omits the runner-owned per-conflict path | docs/reference/specs/agent-ship.md:191 |
| minor | F4 Spec contradiction — agent-ship.md item 21: merge door still promises the removed watch-dependent refusal | docs/reference/specs/agent-ship.md:199 |
Full review
F2: Lines 87–88 still describe a conflicting approved head as an ending/refusal delegated to pulls rebase. Rewrite them for runner re-entry and bind the proofs to the successive-base-moves and typed-conflict tests. Both current references are unbound.
F3: Item 20 still describes one model round per pull-request lifetime without distinguishing runner-owned conflicts. Document that only unowned sweeps retain that limit; each runner-owned conflict gets a lease-bounded fix round.
F4: Item 21 still promises the removed watch-dependent merge-door refusal and references its deleted test. Replace it with the live runner’s sole-ownership behavior and its current proof.
c554a5a to
538b7e8
Compare
There was a problem hiding this comment.
Changes requested: The prior spec findings are fixed, but rollout recovery can still lose runner ownership and permit a concurrent sweep.
Warning
Changes requested · head 538b7e8 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F5 Foreign-generation runners are omitted from ownership recovery | src/core/runnerOwnership.ts:81 |
Full review
F5: recover() rebuilds ownership from local/rehosted runners but ignores outcome.liveElsewhere[*].hosting. During a rolling deploy, the new generation can therefore treat a pull request still driven by the old generation as unowned, allowing pulls rebase to mutate its branch concurrently. Include foreign hosted instance IDs in activeRunnerInstances and add a recovery test covering that overlap.
538b7e8 to
49fb262
Compare
There was a problem hiding this comment.
LGTM: F5 is resolved: foreign-generation hosted runners now participate in ownership recovery, with regression coverage.
Note
Approved · head 49fb262 · no findings
Full review
F5 is fixed: foreign-generation hosted runners now remain fenced during ownership recovery, with focused regression coverage.
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
49fb262 to
ba43849
Compare
|
Item 8 sample (approved PR goes DIRTY when a sibling merges), 2026-09-22 00:15–00:29Z: PR #2168 went DIRTY when #2167 merged (both edited agent-ship.md and the coordinator files). The unit's live fix child (round 2 of attempt 3) hit the conflict in its pre-push rebase, resolved it in-run and pushed 80861a5 rebased on main; no person touched it and the runner's sweep was not needed because a child was live. The runner-side path (no live child) remains untested by this sample. |
Approved pull requests stay with their ship runner through repeated base conflicts, bot restarts, and rolling deploys. Each conflict gets a lease-bounded rebase round, so the pipeline can merge without a person taking ownership.
Why: Issue #2124 exposed an ownership gap after a second base move. Record 0071 requires one live runner to own its pull request until the unit ends.
Where to look
Feedback wanted: Please focus on repeated rebase transitions, unowned-versus-runner conflict budgets, and ownership recovery across rolling-deploy generations.
Risk: This 1,067-line diff spans runner state, sweep wiring, recovery, tests, and specs. A mistake can stall or race an approved pull request; rollback is one commit. Splitting recovery out was rejected because it preserves the ownership race.
Verified: Focused tests (373), root typecheck, specs, hygiene, changed-file formatting, and title passed at ba43849; current-head CI remains the final gate.
Decisions (5)
Validation (7 criteria)
For agents
Addressed the two blocking CI findings in the existing coherent commit and force-pushed ba43849. The bot aggregate failed only because format:check rejected src/core/ship/coordinator.ts; Prettier collapsed the IdleWhy union and all changed-file formatting now passes. F5 remains fixed: RunnerOwnershipFence includes liveElsewhere hosting IDs in its complete-listing rebuild, while incomplete listings remain fail-closed. No generated files were edited; current-head CI is the remaining gate.
Requested by @justinhelmer in slack:C0BRRHKFLCB
🤖 Generated with Claude Code