Reliable stacked-PR rebase anchors#319
Merged
Merged
Conversation
Foundation for the stacked-PR rebase anchor work. Anchor is a recorded fact (base, sha, observed_at_remote) with a 40-char lowercase-hex smart constructor; Anchor_history is a newest-first bounded list (cap 8) that dedups by (base, sha) on push and truncates over-cap legacy snapshots on load. No call sites are touched in this commit — pure additions. Property tests in test/test_anchor.ml cover SHA validation, accessor round-trip, yojson round-trip on both modules, push monotonicity / cap / dedup / newest-at-head, and over-cap truncation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a richer pure decision interface on top of the existing single-SHA
upstream shim:
- type input { anchor; recorded_history; base_branch; head_sha }
- type plan = Onto { target; upstream; reason } | Plain { target; reason }
- type reason = Anchor_matches_head | History_fallback of string |
No_anchor | Anchor_unreachable_from_head | Head_unobservable
- val plan : input -> ancestor_oracle:(...) -> plan
- val anchor_after_result : prev:... -> result:Worktree_parser.rebase_result
-> resolved_remote_sha:... -> base_branch:...
-> Anchor.t option
plan picks the safest upstream by checking, in order, whether each anchor
(newest first, then history) is reachable from the patch's HEAD via a
caller-supplied ancestor_oracle. The newest reachable anchor wins
(Anchor_matches_head); an older history entry is the fallback when the
newest is unreachable (History_fallback). Plain is returned only when no
anchor is reachable (or HEAD is unobservable).
anchor_after_result encapsulates the three-blind-spot policy: refresh on
Ok and Noop when a remote SHA was resolved; preserve the prior anchor on
Conflict and Error so a failed attempt never erases prior knowledge.
Existing upstream shim is unchanged and still consumed by
Worktree.rebase_onto. Step 5 wires plan into the runner fiber and removes
the shim.
Property tests cover RD-PLAN-1..8 and RD-AAR-1..5, including the
production squash-merge scenario (RD-PLAN-8) — anchor recorded at Start
of a stacked patch, dep squash-merges, plan must produce
Onto { upstream = old_dep_tip }.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the anchor_history : Anchor_history.t field to Patch_agent.t with
two new smart setters:
- record_anchor : t -> Anchor.t -> t
Pushes onto the history (newest-first, dedup, capped at 8) AND
mirrors the anchor into the legacy branch_rebased_onto /
branch_rebased_onto_sha view fields so existing readers (drift
detector, persistence writer) continue to see a coherent single-
anchor view during the migration.
- clear_anchor_for_base : t -> Branch.t -> t
Used by the orchestrator when retargeting a base (next step) —
preserves history so Rebase_decision.plan can still fall back to
older entries, but clears the legacy SHA which would otherwise
pin an obsolete value.
Persistence writes a new "anchor_history" key; missing key on load
defaults to Anchor_history.empty so older snapshots round-trip
identically. The new field threads through Patch_agent.restore at every
caller (test_patch_agent, test_patch_controller, test_poll_log,
persistence) — the codebase convention is explicit labeled args, not
optional defaults.
Property tests added to test_persistence_properties.ml:
- anchor_history_roundtrip — non-empty histories survive
yojson_of_t / patch_agent_of_yojson identically.
- missing_anchor_history_defaults_empty — legacy snapshots without
the key load with empty history (back-compat).
Also fixes a non-fatal doc-comment markup warning in
test_rebase_decision_properties.ml from step 2.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds two new ops to the plan vocabulary:
- Capture_anchor of { ref_name; slot }
Resolves ref_name (e.g. "origin/main") to a SHA via
Worktree.read_branch_sha and stores it in slot.
- Record_anchor_on_success of { slot; base }
Emits a Worktree_plan.anchor_event from slot's captured SHA iff
the most recent Rebase_onto returned Ok or Noop (or no rebase ran).
The new event type Worktree_plan.anchor_event = Anchor_recorded of
Anchor.t | Anchor_capture_failed is the executor's side-channel for
telling the runner fiber which anchors to fold into agent state.
Adds Worktree_plan.for_start ~base = [Ensure_worktree; Fetch_origin;
Capture_anchor; Record_anchor_on_success]. No Rebase_onto — this plan
just captures and records an initial anchor for freshly-branched-off
patches. Step 7 wires it into the runner Start path, closing the
production-bug blind spot.
Executor changes:
- Maintains a sparse int -> string option slot table for the plan.
- Returns (rebase_result, path, anchor_event list) instead of
(rebase_result, path). Existing callers in runner_fiber_impl.ml
accept the new third element with _anchor_events (consumed in
step 5+).
- Conflict/Error short-circuit (Record_anchor_on_success after a
failed rebase does NOT fire); Ok and Noop both continue (Noop
DOES refresh the anchor, matching anchor_after_result's policy).
- Default last_rebase = Ok so for_start plans without any rebase
still record their anchor.
Plan-validity property tests extended for the new variants;
for_rebase / for_merge_conflict are unchanged in this commit — step
5/6 update them to include the new ops when the runner is rewired.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
for_rebase now emits anchor events around its Rebase_onto:
Ensure_worktree -> Fetch_origin -> Capture_anchor {origin/<new_base>, slot=0}
-> Rebase_onto origin/<new_base> -> Record_anchor_on_success {slot=0, new_base}
The executor emits Anchor_recorded on Ok/Noop and nothing on Conflict/Error.
Adds Orchestrator.apply_rebase_with_anchor that combines apply_rebase_result
with a fold over the anchor events into a single atomic update. Each
Anchor_recorded calls Patch_agent.record_anchor (pushes onto
Anchor_history and mirrors into the legacy view fields);
Anchor_capture_failed events are ignored.
Runner Rebase path now calls apply_rebase_with_anchor and drops the
bespoke post_rebase_sha capture + the separate set_branch_rebased_onto_sha
update. The orchestrator transition and the anchor recording happen in a
single update_orchestrator_returning call.
Net result for an Ok rebase: branch_rebased_onto and
branch_rebased_onto_sha are populated identically to before, AND a new
entry is pushed onto anchor_history. For a Noop rebase: same behavior,
where previously the SHA was NOT captured — closing one of the three
blind spots.
for_merge_conflict is still the old 3-op plan; step 6 wires it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Worktree.S signature changes:
- rebase_onto loses ?prev_base_sha:string option; gains required
~upstream:string. The caller (the executor) is now responsible for
computing the right upstream from anchor history; W.rebase_onto's job
is just to invoke git with what it's given. find_old_base's cherry-
pick / patch-id detection stays as defense-in-depth: it still runs
first; the supplied upstream is used only on its fallback path.
- New is_ancestor : path -> ancestor -> descendant -> bool via
[git merge-base --is-ancestor]. Returns false on any error.
Worktree_plan_executor.Rebase_onto handler now:
1. Reads HEAD sha via W.read_branch_sha ~ref_name:"HEAD".
2. Builds a Rebase_decision.input from the agent's anchor_history,
base_branch, and the HEAD sha.
3. Calls Rebase_decision.plan with W.is_ancestor as the ancestor_oracle.
4. Passes Onto.upstream (or Plain.target as a fallback) to W.rebase_onto.
Deletes Rebase_decision.upstream and its RD-1..5 property tests.
For agents whose anchor_history is empty, plan returns Plain { target =
base_branch_name; reason = No_anchor }, the executor passes upstream =
target, and W.rebase_onto picks the 2-arg form. Bit-identical to today's
None-prev_base_sha path.
Test surface updates:
- test_rebase_onto.ml: every direct W.rebase_onto call gains
~upstream:"<target_name>".
- test_dependency_injection_functors.ml: Fake_worktree updated to the
new signature; adds is_ancestor stub.
- test_rebase_decision_properties.ml: deletes RD-1..5 (the shim
tests); keeps the structured-plan suite.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
for_merge_conflict now includes Capture_anchor + Record_anchor_on_success around its Rebase_onto, exactly like for_rebase (step 5.1). Orchestrator.apply_conflict_rebase_with_anchor combines apply_conflict_rebase_result with a fold over Worktree_plan.anchor_event into a single atomic update. Same policy as apply_rebase_with_anchor: Anchor_recorded calls Patch_agent.record_anchor; Anchor_capture_failed is ignored (prior anchor preserved). Runner Merge_conflict path now calls apply_conflict_rebase_with_anchor. Net effect: a successful conflict rebase (Ok) now refreshes the agent's anchor with the post-rebase origin/<base> SHA — closing the second of the three blind spots from the design. A Noop conflict rebase also refreshes the anchor; previously only branch_rebased_onto (the legacy field) was set, never the SHA. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Runner Start path now executes Worktree_plan.for_start AFTER the worktree
exists but BEFORE the LLM session begins. The plan:
Ensure_worktree -> Fetch_origin
-> Capture_anchor {origin/<base_branch>, slot=0}
-> Record_anchor_on_success {slot=0, base=base_branch}
emits an Anchor_recorded event with origin/<base_branch>'s current tip
SHA, which Orchestrator.apply_anchor_events folds into the agent via
Patch_agent.record_anchor.
Adds Orchestrator.apply_anchor_events for the events-only case (no
rebase result to combine).
Why this closes the production bug:
In the production state (ts-lowerability-effect-boundary-checker, p4/p5):
- p4 was branched off patch-2 at Start.
- branch_rebased_onto_sha was None at Start (recorded only on
successful Rebase).
- patch-2 squash-merged.
- First rebase fired; with no anchor, Rebase_decision.plan returned
Plain { reason = No_anchor }, the 2-arg form ran, and git replayed
patch-2's pre-squash commits on top of main's post-squash version
=> "both added" conflict.
With this commit:
- At Start, for_start captures origin/patch-2's tip SHA (51fce1265).
- Anchor recorded in agent's anchor_history.
- When patch-2 squash-merges and first rebase fires,
Rebase_decision.plan returns Onto { upstream = 51fce1265; reason =
Anchor_matches_head } because 51fce1265 is reachable from p4's HEAD.
- git rebase --onto origin/main 51fce1265 HEAD replays just p4's own
commit on top of main. No conflict.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 8 was originally specified as "invalidate the legacy anchor view fields when refresh_base_branch retargets the base," but doing so breaks the drift detector (PI-16) which relies on branch_rebased_onto reflecting where the local branch was LAST REBASED, not where the orchestrator currently WANTS the base to be. The squash-merge / dep-retarget cases the original step 8 worried about are already handled at the right layer: Rebase_decision.plan reads anchor_history directly and uses an is_ancestor oracle at rebase time to decide whether the recorded anchor is still safe for the new base. The orchestrator transition has no oracle and shouldn't try to second-guess the planner. This commit adds a comment in refresh_base_branch explaining the decision so a future "why doesn't this invalidate the anchor?" check finds the answer in-line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
test/test_rebase_state_machine.ml exercises realistic event sequences
against an in-memory commit DAG, asserting integration invariants
between Patch_agent.record_anchor, Rebase_decision.plan, and
Rebase_decision.anchor_after_result. Named scenarios:
SM-1: dep merges WHILE a dependent's rebase is in flight; the
recorded anchor remains coherent across the in-flight result.
SM-2: production-bug scenario end-to-end. p4 branched off patch-2
at Start records the patch-2 tip as its anchor; patch-2
squash-merges; the next rebase planner returns Onto with the
old patch-2 tip as upstream, never replaying patch-2's
commits onto post-squash main.
SM-3a: dep branch force-pushed; the recorded anchor is still
reachable from the patch's HEAD, so plan still picks
Anchor_matches_head.
SM-3b: variant — the patch's HEAD itself was reset such that the
anchor is no longer reachable; plan correctly returns
Plain {Anchor_unreachable_from_head}.
SM-4: 4-deep stack p1 <- p2 <- p3 <- p4; p1, p2, p3 squash-merge in
sequence; p4's anchor stays valid throughout.
SM-7: a Noop rebase result still refreshes the anchor — closes the
old blind spot where the SHA was captured only on Ok.
SM-8: a Conflict result preserves the prior anchor unchanged so a
retry sees the same input.
SM-no-anchor: legacy / fresh agent without any recorded anchor;
plan returns Plain {No_anchor}, executor passes upstream=target,
rebase falls back to the 2-arg form.
SM-5 (crash mid-plan), SM-6 (worktree deleted mid-rebase) require
orchestrator-level state interleaving and are deferred to a follow-up;
the pure-decision pipeline tested here is the load-bearing piece.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a "Rebase anchor lifecycle" section explaining:
- The three recording moments (Start, Rebase Ok/Noop, Merge-conflict
Ok/Noop) and the Worktree_plan ops that mediate them.
- The Conflict/Error preservation policy.
- How Rebase_decision.plan consumes anchor_history at rebase time
using the is_ancestor oracle.
- Why refresh_base_branch deliberately doesn't invalidate the anchor.
- Pointers to the test suites that cover the pipeline end-to-end.
This is the last step of the stacked-pr-rebase-anchors workstream.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements rebase anchor history tracking across the patch orchestration pipeline. Anchors record upstream branch, resolved SHA, and remote-observation state. Patch agents now track a bounded (cap=8), newest-first, deduplicated history of anchors. Rebase planning uses this history plus an ancestor oracle to safely select upstreams. The executor captures origin refs into slots and emits anchor events on successful rebases. The orchestrator and runner atomically incorporate these events into agent state. ChangesRebase anchor history tracking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![flowglad-review-service[bot]](https://avatars.githubusercontent.com/u/178508461?s=60&v=4){kind=small}
Review SummaryThis is a well-structured
Variant: convergence-v2 Candidates: 2 | Posted: 2 | Suppressed: 0 2 comments posted · Model: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib_core/anchor_history.ml`:
- Around line 24-27: of_yojson_opt currently returns Some (List.take xs cap)
after raw_of_yojson which only truncates by length and can leave duplicate
(base, sha) pairs; instead decode into xs and rebuild the history by feeding
entries through the module's push function to enforce invariants (uniqueness,
ordering, and capacity). Replace the direct List.take call with a fold over xs
that starts from the empty history and sequentially calls push (using push and
the module's empty/history constructor) so the resulting history is normalized
and respects cap; keep raw_of_yojson and cap as the sources of input and size
limit.
In `@test/test_worktree_plan.ml`:
- Around line 115-118: The test "for_start: includes Capture_anchor for
origin/<base> in slot 0" only checks ref_name via plan_has_capture_for(for_start
~base) but doesn't assert the slot; update the test to also assert the capture
slot equals 0. Modify the invocation to include an explicit slot check (e.g.,
call plan_has_capture_for with a slot argument like ~slot:0 if the helper
supports it, or add a separate assertion that inspects the returned capture's
slot from for_start ~base and compares it to 0) so that both the ref name
("origin/" ^ Branch.to_string base) and slot 0 are verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82285ab5-9e35-4554-8c8a-abe8e5c77908
📒 Files selected for processing (30)
AGENTS.mdlib/orchestrator.mllib/orchestrator.mlilib/persistence.mllib/runner_fiber_impl.mllib/worktree.mllib/worktree.mlilib/worktree_plan_executor.mllib/worktree_plan_executor.mlilib_core/anchor.mllib_core/anchor.mlilib_core/anchor_history.mllib_core/anchor_history.mlilib_core/patch_agent.mllib_core/patch_agent.mlilib_core/rebase_decision.mllib_core/rebase_decision.mlilib_core/worktree_plan.mllib_core/worktree_plan.mlitest/dunetest/test_anchor.mltest/test_dependency_injection_functors.mltest/test_patch_agent.mltest/test_patch_controller.mltest/test_persistence_properties.mltest/test_poll_log_properties.mltest/test_rebase_decision_properties.mltest/test_rebase_onto.mltest/test_rebase_state_machine.mltest/test_worktree_plan.ml
Review SummaryOne must-fix correctness bug: in
Variant: convergence-v2 Candidates: 1 | Posted: 1 | Suppressed: 0 1 comment posted · Model: |
Review SummaryAfter thorough review of the incremental push — covering Key verified properties:
Variant: convergence-v2 Candidates: 0 | Posted: 0 | Suppressed: 0 0 comments posted · Model: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the squash-merge stacked-PR rebase trap observed in production at
~/.local/share/onton/ts-lowerability-effect-boundary-checker(p4/p5). A patch branched off a dependency at Start, idle through the dep's squash-merge, would on its first rebase try to replay the dep's pre-squash commits on top of the post-squash main and conflict on every file the dep added.Root cause:
branch_rebased_onto_shawas recorded at exactly one site — successful Rebase. A patch branched-off-dep had no anchor at first-rebase time, fell back to the legacy 2-arg form, and conflicted.The fix is structural, not a patch: records anchors at every base transition (Start, Ok/Noop rebase, Ok/Noop merge-conflict rebase), keeps a bounded history for divergence fallback, splits the decision into pure (
Rebase_decision.plan) and effectful (executor + handler), and deletes the legacyupstreamshim now that the planner is consulted directly.Commit sequence (11 commits, each green on its own)
Anchor+Anchor_historyvalue modules (validation, dedup, cap 8) + property tests.Rebase_decision:plan+anchor_after_resultwith reason variants.anchor_historyonPatch_agent+ persistence round-trip.Worktree_plangainsCapture_anchor/Record_anchor_on_successops andfor_start.5.1 Runner Rebase path uses atomic
Orchestrator.apply_rebase_with_anchor.5.2 Executor computes
Rebase_decision.planfrom anchor history; legacyRebase_decision.upstreamshim deleted;Worktree.rebase_ontotakes~upstream:string.for_startruns at Start before the LLM session — closes the production bug.refresh_base_branchdeliberately preserves the anchor (drift detector relies on it; PI-16).Test plan
dune buildstrict-warnings cleandune runtestfull suite green at every committest_rebase_decision_properties.ml(RD-PLAN-1..8, RD-AAR-1..5)test_anchor.ml(12 tests)test_rebase_state_machine.ml(8 named scenarios, production bug among them)test_rebase_onto.mlintegration tests pass against the new~upstreamsignaturetest_persistence_properties.mlextended withanchor_historyround-trip + missing-key defaultIn-flight broken agents (p4/p5 in production) are out of scope per prior discussion — they will be unstuck manually.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Makes stacked-PR rebases reliable by recording and using rebase anchors to compute safe git --onto upstreams. This stops replaying dependency commits after squash-merges and removes “both added” conflicts.
New Features
Bug Fixes
Written for commit f97057a. Summary will update on new commits. Review in cubic
Summary by CodeRabbit