Skip to content

fix(ship): keep repeated rebase conflicts with the runner - #2134

Merged
justinhelmer merged 1 commit into
mainfrom
plan/fix-issue-2124-an-approv-9f3948/u1
Sep 21, 2026
Merged

justinhelmer merged 1 commit into
mainfrom
plan/fix-issue-2124-an-approv-9f3948/u1

Conversation

@coreplane-switchboard

@coreplane-switchboard coreplane-switchboard Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

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

  1. Re-enter approved rebases Tracks each fresh base move on unit state instead of publishing the former merge-ready ending. ⚠ A wrong transition can strand an approved pull request.
  2. Resolve runner rebase outcomes Carries unchanged approvals, re-reviews changed patches, and sends each conflict into a lease-bounded round. ⚠ Approval and review routing must stay pinned to the new head.
  3. Map the runner rebase route Converts the shared resolver into typed carried, changed, conflict, and refusal outcomes. ⚠ Incorrect mapping could preserve a stale approval.
  4. Fence command sweeps Defers operator sweeps to a live runner while keeping the hand-rebase remedy for unowned pull requests. ⚠ A missed owner can force-push live runner work.
  5. Rebuild ownership across generations Rebuilds the fence from current and foreign hosted runners before sweeps may treat a pull request as unowned. ⚠ Failing open lets a sweep race an older deployment's runner.
  6. Prove rolling-deploy ownership Pins the overlap where a foreign-generation runner retains ownership while its durable lease is live.
  7. Bind the corrected contract Separates unowned sweep limits from per-conflict runner rounds and proves sole ownership at the merge door.

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)
  • Reuse the sweep's git-first resolver. A separate runner resolver would duplicate range-diff approval carry and conflict handling. Runner mode reuses those facts but returns conflicts to the owning Workflow instead of dispatching a detached sweep round.
  • Keep metadata mode-scoped. Adding runner continuation facts to every SweepResult changed command output contracts. Returning them only for runner mode preserves command reports while giving the coordinator its new head.
  • Bound repeated conflicts by the lease. The review-round cap may already be spent after approval, but a changed rebase still needs review. Each conflict continues only while the existing pipeline lease can carve another round.
  • Keep hand rebases on the unowned sweep. A live runner has durable state and a unit thread, so handing its conflict to an operator creates two owners. The sweep retains its one-round rule only where no live runner owns the pull request.
  • Recover every hosted generation. During a rolling deploy, the old generation remains the valid host until its lease ends. Recovery therefore includes both current liveHosted rows and foreign liveElsewhere hosting rows before the sweep fence opens.
Validation (7 criteria)
Criterion Proof
The focused behavior and ownership regressions pass npx vitest run on the eight changed test files passed at ba43849: 8 files, 373 tests.
The root TypeScript graph accepts the change NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json passed at ba43849.
Every spec proof still binds npm run specs:check passed at ba43849: 51 specs and 5,618 proof references checked.
Public-tree hygiene does not grow npm run hygiene:check passed at ba43849: 9 files and 37 retained hits remain listed; 138 lines are allowed.
Every changed file follows repository formatting npx prettier --check over all 21 changed files passed at ba43849 after formatting src/core/ship/coordinator.ts.
The pull request title follows the changelog rule npm run check:pr-title -- "fix(ship): keep repeated rebase conflicts with the runner" passed at ba43849.
The fix is current with the base branch Fetched origin/main and rebased the single coherent commit onto 9dc78b7 before force-pushing ba43849; the branch was already current.
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

@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: 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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2124-an-approv-9f3948/u1 branch 2 times, most recently from 1a1d83f to 7cf64fa Compare September 21, 2026 16:57

@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: 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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2124-an-approv-9f3948/u1 branch from 7cf64fa to 17a569d Compare September 21, 2026 17:10

@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: 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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2124-an-approv-9f3948/u1 branch from 17a569d to 8a69e15 Compare September 21, 2026 17:24

@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: F2 is resolved: complete hosted-row listings now keep every live runner in the recovered ownership fence.

Note

Approved · head 8a69e15 · no findings

Full review

F2 is resolved, and no new findings remain.

@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.

@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: 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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2124-an-approv-9f3948/u1 branch from 8a69e15 to fbe465e Compare September 21, 2026 17:41

@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: F1 is resolved: runner-only rebase metadata no longer changes command sweep results.

Note

Approved · head fbe465e · no findings

Full review

F1 is resolved; runner-only rebase metadata no longer changes command sweep results.

@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.

@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: 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.

@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: 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.

@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: 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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2124-an-approv-9f3948/u1 branch from 538b7e8 to 49fb262 Compare September 21, 2026 19:05

@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: 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.

@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.

Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-issue-2124-an-approv-9f3948/u1 branch from 49fb262 to ba43849 Compare September 21, 2026 19:14

@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 formatting-only delta resolves both prior CI findings without changing behavior.

Note

Approved · head ba43849 · no findings

Full review

The formatting-only delta resolves both prior CI findings without changing behavior.

@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

Copy link
Copy Markdown
Contributor

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.

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