Surface rebase recovery info in the merge-conflict prompt#225
Conversation
When the orchestrator's auto-rebase hits conflicts and hands off to the
patch agent, the prompt previously warned "do not run `git rebase
origin/<base>`" without telling the agent which command WOULD be correct
if the in-progress rebase state was lost. The orchestrator already
computed the `--onto` arguments via `find_old_base` but did not forward
them, so an agent that aborted resolution couldn't reconstruct the right
restart command.
Decompose the rebase logic into a pure decision layer
(`classify_unique_commits`, `parse_rebase_merge_state`) and effectful
handlers (`rebase_onto`, `read_in_progress_conflict_info`), following
the existing `classify_fetch_result` / `fetch_origin` split. Extend
`Conflict` with a `conflict_info` payload carrying the target branch,
old_base SHA, patch-unique commit list, strategy, and pre-rebase HEAD.
Thread it through both delivery paths: fresh-rebase (binds `Conflict
ci`) and rebase-already-in-progress (reads
`.git/rebase-merge/{onto,orig-head}` and runs `git log onto..orig-head`
against `origin/<base>`).
The conflict prompt now renders a "Recovery" section with `git fetch
origin`, the exact `git rebase --onto origin/<base> <old_base>` command,
the unique-commit list (oldest first), and a `git reset --hard
<orig_head>` emergency-recovery anchor when capture succeeded. Plain-
strategy fallback (no unique commits isolated) renders a similar block
with `git rebase origin/<base>` and no commit list.
Decision layer is exhaustively property-tested in
test_rebase_onto.ml (10 single-shot properties incl. ordering,
ancestor-filter, CRLF, back-compat wrapper round-trip,
parse_rebase_merge_state orig_head stripping). Prompt rendering
asserted in test_prompt_recovery.ml. State-machine interleavings in
test_conflict_recovery_state_machine.ml verify R-1..R-5 over 500-step
random command sequences (Onto delivery cannot accept Plain recovery,
reconstruction round-trips commit set, etc.).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis pull request refactors merge-conflict handling by converting Changes
Sequence DiagramsequenceDiagram
participant Main as Main
participant Worktree as Worktree
participant Orchestrator as Orchestrator
participant Prompt as Prompt
participant Agent as Agent
Main->>Worktree: rebase_onto()
Worktree->>Worktree: capture orig_head<br/>compute old_base<br/>classify unique_commits
alt Rebase fails
Worktree-->>Main: Conflict of conflict_info
else Rebase in progress
Main->>Worktree: read_in_progress_conflict_info()
Worktree-->>Main: enriched conflict_info
end
Main->>Orchestrator: apply_rebase_result<br/>(conflict_info)
Orchestrator->>Main: deliver_to_agent<br/>(?conflict_info)
Main->>Prompt: render_merge_conflict_prompt<br/>(?conflict_info)
alt conflict_info present
Prompt->>Prompt: render_recovery_section<br/>(unique_commits,<br/>old_base, orig_head)
Prompt-->>Main: prompt with<br/>Recovery section
else conflict_info absent
Prompt-->>Main: legacy prompt<br/>(unchanged)
end
Main->>Agent: deliver(prompt)
Agent-->>Main: recovery command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 57 minutes and 12 seconds. Comment |
![flowglad-review-service[bot]](https://avatars.githubusercontent.com/u/178508461?s=60&v=4){kind=small}
Review SummaryThe PR is well-structured and the core logic is sound. Three findings, all in the newly added code:
3 comments posted · Model: |
Wrap the channel in Fun.protect so close_in always runs on the truncated/short-read path, document why parse_rebase_merge_state emits strategy = Onto unconditionally, and explain the None-conflict_info degraded path at the Conflict_needs_agent arm.
Review SummaryTwo findings:
2 comments posted · Model: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test/test_conflict_recovery_state_machine.ml">
<violation number="1" location="test/test_conflict_recovery_state_machine.ml:254">
P2: `prop_eventually_terminal_when_finished` is currently vacuous: both `Terminal` and `Active` outcomes return `true`, so it does not verify the claimed R-5 termination behavior.
(Based on your team's feedback about avoiding vacuous passes in state-machine property tests.) [FEEDBACK_USED]</violation>
</file>
<file name="lib/worktree.ml">
<violation number="1" location="lib/worktree.ml:470">
P1: In in-progress conflict reconstruction, `old_base` is set from `.git/rebase-merge/onto` (the new base) instead of the upstream anchor, so the recovery `git rebase --onto <target> <old_base>` command can replay the wrong commit range.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
parse_rebase_merge_state was reading .git/rebase-merge/onto and using its SHA as old_base, but onto holds the rebase destination, not the upstream. The recovery command was therefore git rebase --onto X X with the same SHA twice. Now read .git/rebase-merge/upstream as well and use its stripped value as old_base; range upstream..orig-head also matches the rebase's actual replay set. In render_recovery_section, build commits_section conditionally so an empty unique_commits list no longer renders a dangling header.
Review SummaryThe diff is well-structured and the major
2 comments posted · Model: |
The previous prop_eventually_terminal_when_finished returned true on both `Terminal and `Active outcomes, so it could not detect a model where a precondition-holding final command failed to reach a terminal state. Split out a precondition_holds predicate (Auto_rebased + Auto_resolve, or Conflict_delivered + matching strategy on Agent_recovers_*) and require Terminal-equivalence only in that arm; no-op tail commands from non-eligible states still pass via the `Active, false branch.
Review SummaryOne new issue found in this incremental diff: the
1 comment posted · Model: |
parse_rebase_merge_state now returns Some when classify_unique_commits errors but onto/upstream are valid SHAs: the recovery command is fully determined by target and old_base, so a restarted orchestrator should still surface it. unique_commits is empty in that case; the prompt renderer (changed in b6a52ea) omits the bullet header. Also clarify the conflict_info doc on unique_commits ordering: head of the list is the newest commit, last element is the oldest, and the prompt reverses for display. Two new property tests pin the new empty-commits-but-valid-command behavior.
The .mli still described reading .git/rebase-merge/{onto,orig-head}
and running git log onto..orig-head; the implementation in 6a2ab5f
reads {onto,upstream,orig-head} and runs git log upstream..orig-head.
Sync the doc.
Review SummaryTwo issues found in code introduced by this diff:
2 comments posted · Model: |
production parse_rebase_merge_state can produce
{ strategy = Onto; unique_commits = []; old_base = <upstream> }
when the log is empty or every commit is ancestor-filtered (covered
by prop_empty_log_keeps_command and prop_all_filtered_keeps_command
in test_rebase_onto.ml). The model's R-1 was stricter than the type's
contract, which would spuriously fail any future model extension that
threaded a reconstructed conflict_info through apply.
Review SummaryThe diff is overall well-structured and the previously flagged correctness issues (upstream vs onto, fd leak, doc mismatches) are now addressed or outstanding. Two new findings in the newly introduced code:
2 comments posted · Model: |
The Onto recovery section used base_branch for the warning sentence
('do NOT run git rebase origin/<base_branch>') and ci.target for the
actual rebase command. They happened to agree in production
(ci.target = origin/<base_branch>), but the relationship was implicit
and would diverge silently if a caller ever constructed conflict_info
with a different format. Use ci.target in both spots so the warning
and the command can never disagree; ~base_branch is no longer needed
by render_recovery_section, so drop it (the public
render_merge_conflict_prompt API is unchanged).
Also align onto_fixture and plain_fixture in the state-machine model
to use 'origin/main' (matching production), so future model
extensions threading these fixtures through render_recovery_section
exercise the same target shape the agents actually see.
Review SummaryThe incremental diff successfully addresses the outstanding issues from Turns 1–6:
1 comment 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: 1
🧹 Nitpick comments (1)
test/test_prompt_recovery.ml (1)
95-135: ⚡ Quick winAdd the degraded
Onto/empty-commit recovery case.This file covers
Ontowith commits andPlainwith no commits, but the new behavior in this PR also allowsstrategy = Ontowithunique_commits = []. A renderer regression that treats every empty list likePlainwould still pass here. Please add an explicit assertion thatOntostill rendersgit rebase --onto <target> <old_base>while simply omitting the commit bullets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_prompt_recovery.ml` around lines 95 - 135, Add a new test case similar to the existing Plain one that constructs a Worktree.conflict_info with strategy = Onto and unique_commits = [] (and appropriate old_base and target like "origin/release"/"release"), render the prompt via Prompt.render_merge_conflict_prompt, slice out the "## Recovery (if rebase state is lost)" section, and assert that the recovery_section contains the exact rebase form "git rebase --onto <target> <old_base>" (e.g., "git rebase --onto origin/release <old_base>") while also asserting that there are no per-patch commit bullets (reuse assert_contains/assert_not_contains helpers to check presence of the --onto rebase command and absence of any commit list bullets).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/prompt.mli`:
- Around line 64-68: Update the docstring for the merge-conflict prompt function
to accurately describe strategy-dependent Recovery output: state that when
~conflict_info is provided the Recovery section contains a reconstruction hint
but its exact contents depend on the chosen strategy (e.g., the Plain strategy
renders a simple "git rebase <target>" path and may omit the per-commit bullet
list, whereas other strategies include the explicit "git rebase --onto" command
plus unique-commit bullets). Reference the existing parameters by name (e.g.,
~conflict_info and the Plain strategy) so readers can find the relevant behavior
in the merge-conflict prompt renderer.
---
Nitpick comments:
In `@test/test_prompt_recovery.ml`:
- Around line 95-135: Add a new test case similar to the existing Plain one that
constructs a Worktree.conflict_info with strategy = Onto and unique_commits = []
(and appropriate old_base and target like "origin/release"/"release"), render
the prompt via Prompt.render_merge_conflict_prompt, slice out the "## Recovery
(if rebase state is lost)" section, and assert that the recovery_section
contains the exact rebase form "git rebase --onto <target> <old_base>" (e.g.,
"git rebase --onto origin/release <old_base>") while also asserting that there
are no per-patch commit bullets (reuse assert_contains/assert_not_contains
helpers to check presence of the --onto rebase command and absence of any commit
list bullets).
🪄 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: dac09721-ca8a-4b72-9c07-ac90f8e38fdb
📒 Files selected for processing (13)
bin/main.mllib/orchestrator.mllib/prompt.mllib/prompt.mlilib/worktree.mllib/worktree.mlilib_test/test_generators.mltest/dunetest/test_conflict_recovery_state_machine.mltest/test_interleaving_properties.mltest/test_orchestrator_properties.mltest/test_prompt_recovery.mltest/test_rebase_onto.ml
If run_sequence on the core or final cmds reports an invariant error, that's prop_invariants_hold's concern, not R-5's. Returning false turned an R-1..R-4 violation into an R-5 counter-example that QCheck would shrink toward the tail command — misleading. Treat both as precondition failures and pass.
Review SummaryOne new issue found in this incremental diff. The
1 comment posted · Model: |
Previously, a missing or empty .git/rebase-merge/orig-head dropped the entire recovery section even though onto and upstream were valid. Mirror the fresh-rebase path (which stores "" when git rev-parse HEAD fails and still emits the conflict_info): only onto and upstream are required; orig-head is best-effort. When orig-head is empty, fall back to upstream..HEAD for the log range so the prompt still gets the rebase's intended replay set.
Review SummaryThe incremental diff addresses several prior comments (R-1 guard corrected,
1 comment posted · Model: |
gen_sha, gen_non_ancestor_subject, gen_unique_commit, log_body_of_commits, gen_log_lines, gen_rebase_strategy, gen_target_branch, and gen_conflict_info were added in this branch but no test imports them. Drop them until a caller exists rather than carry dead generator code.
Review SummaryThis incremental diff is clean. It correctly addresses several previously-raised concerns:
0 comments posted · Model: |
1 participant
Summary
git rebase --ontocommand, the patch's unique-commit list, and agit reset --hard <orig_head>emergency recovery anchor — so an agent that aborts the in-progress rebase can reconstruct the correct restart instead of guessing.git fetch originfirst and usesorigin/<base>as the rebase target in both delivery paths (fresh-rebase + rebase-already-in-progress), so the agent never operates against a stale local tracking ref.classify_unique_commits,parse_rebase_merge_state) and effectful handlers (rebase_onto,read_in_progress_conflict_info), following the existingclassify_fetch_result/fetch_originsplit.Background
The orchestrator's auto-rebase already computes
--ontoarguments viafind_old_baseand uses them correctly when runninggit rebase --onto target old_base. But on conflict, the prompt only warned don't rungit rebase origin/<base>— without telling the agent which command WOULD be correct. If the agent aborted mid-resolution (a common path when stuck), they had no way to reconstruct the right restart and would re-pick already-merged dependency commits. This was a real failure mode observed during a recent patch-4 conflict where the agent's local view ofmainwas stale.What changed
Decision layer (
lib/worktree.ml)unique_commit,rebase_strategy,conflict_infotypes.Conflictnow carries the payload (was a bare constructor).classify_unique_commits ~project_name ~ancestor_ids log_output— pure: returns(unique_commit list * oldest_sha)fromgit log %H %soutput, with the ancestor-subject filter applied. Existingoldest_non_ancestor_commitkept as a back-compat wrapper.parse_rebase_merge_state— pure: assemblesconflict_infofrom.git/rebase-merge/{onto,orig-head}contents +git log onto..orig-headoutput.Effectful handlers
rebase_ontorestructured to captureorig_head(HEAD before rebase runs) and emitConflict { target; old_base; unique_commits; strategy; orig_head }on conflict. Plain-strategy fallback used whenfind_old_basecouldn't isolate unique commits.read_in_progress_conflict_info— new effectful helper that reconstructsconflict_infofor the rebase-already-in-progress path. Best-effort: degrades toNone(no recovery section) on any read/parse failure.Prompt (
lib/prompt.ml)render_merge_conflict_promptgains?conflict_info. New "Recovery" section withgit fetch origin, the exactgit rebase --onto origin/<base> <old_base>command, oldest-first commit bullets, and agit reset --hard <orig_head>block when orig_head is populated. Plain-strategy renders an analogous block withgit rebase origin/<base>and no commit list.recovery_section,old_base,target_branch,orig_head) for prompt overrides.Plumbing (
bin/main.ml)deliver_to_agentgains?conflict_info. Both call paths populate it: fresh-rebase bindsConflict ci; in-progress reads viaread_in_progress_conflict_info.target = origin/<base>(was bare<base>), matching the fresh-rebase path's behavior.Tests
Pure decision layer (
test/test_rebase_onto.ml, +10 properties):classify_unique_commitsproperties: empty → Error; length pairs oldest; all-ancestor → Error; kept count + ancestor count = total; subject-with-spaces; CRLF; back-compat wrapper round-tripparse_rebase_merge_stateproperties: well-formed → Some Onto with N commits; orig_head whitespace-stripped; blank onto → None; empty log → NonePrompt rendering (
test/test_prompt_recovery.ml, new): legacy byte-equal regression (no Recovery section when?conflict_info:None); Onto strategy renders fetch + exact--onto origin/main <old_base>+git reset --hard <orig_head>+ commits oldest-first; Plain strategy renders fetch +git rebase origin/<base>+ no--onto+ no reset (orig_head empty).State-machine interleavings (
test/test_conflict_recovery_state_machine.ml, new): 500-step random command sequences overIdle → Auto_rebased → Conflict_delivered → Resolved/Aborted_then_recovered/Terminal. Invariants R-1..R-5: Onto strategy implies non-empty old_base + commits; Onto delivery cannot transition viaAgent_recovers_with_plain(R-2); reconstruction round-trips commit set; sequences ending in recovery commands reach Terminal.All ~155 properties + 10 git-repo integration tests pass; pre-commit hooks (build + test + format) clean.
Test plan
dune buildclean (fatal warnings)dune runtestgreendune fmtcleangit rebase --onto origin/main <sha>command and orig_head reset anchor🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Bug Fixes
Tests