Harden worktree start point + push safety against PR #315-class wipes#318
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds pure planners (Start_point_plan, Push_plan), wires them into worktree creation and push flows, introduces git subprocess helpers and best-effort fetch, surfaces structured refusals, updates rejection classification, and adds property and integration tests. ChangesPure Planning Modules and Integration
🎯 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
One
Variant: convergence-v2 Candidates: 3 | Posted: 3 | Suppressed: 0 3 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/push_reject_classify.ml`:
- Around line 110-111: The Local_state_unsafe arm of detail_excerpt currently
returns reason verbatim which can exceed the documented excerpt bound; update it
to strip and then truncate the reason to the canonical excerpt length (use the
existing excerpt bound constant or helper, e.g., max_detail_excerpt or an
excerpt helper used elsewhere) and return None if the stripped result is empty,
otherwise return the truncated excerpt so Local_state_unsafe always respects the
documented bound.
In `@test/test_push_plan_integration.ml`:
- Around line 58-59: The git_capture helper currently ignores the subprocess
exit status; update git_capture to capture the result of Unix.close_process_in
ic into a value (e.g., status), check that it equals Unix.WEXITED 0, and if not
raise a test failure (failwith or OUnit assert) including the non-zero status
and the Buffer.contents buf to aid debugging; reference the git_capture
function, the call to Unix.close_process_in, and Buffer.contents buf when making
the change.
🪄 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: c853aa00-a626-418f-a0b8-d873de787b7c
📒 Files selected for processing (17)
lib/worktree.mllib/worktree.mlilib/worktree_setup.mllib_core/push_plan.mllib_core/push_plan.mlilib_core/push_reject_classify.mllib_core/push_reject_classify.mlilib_core/start_point_plan.mllib_core/start_point_plan.mlilib_core/worktree_parser.mltest/dunetest/test_dependency_injection_functors.mltest/test_push_plan_integration.mltest/test_push_plan_properties.mltest/test_push_reject_classify_properties.mltest/test_start_point_plan_properties.mltest/test_worktree_start_point_integration.ml
Review SummaryOne must-fix finding in the new integration test. The
Variant: convergence-v2 Candidates: 1 | Posted: 1 | Suppressed: 0 1 comment posted · Model: |
Review SummaryOne must-fix: the
Variant: convergence-v2 Candidates: 1 | Posted: 1 | Suppressed: 0 1 comment posted · Model: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/test_push_plan_properties.ml (1)
385-399: ⚡ Quick winPPP-10 can pass even if required refusal mappings regress to
None.Because
None -> true, this property won’t fail ifBranch_ref_missing,Branch_switched,Local_missing_remote_commits, orLocal_diverged_from_remote_commitsstop mapping toLocal_state_unsafe.✅ Stronger property shape
- List.for_all refusals ~f:(fun r -> - match PP.to_push_reject_classify_rejection r with - | None -> true - | Some rej -> Push_reject_classify.is_permanent rej)) + List.for_all refusals ~f:(fun r -> + match r, PP.to_push_reject_classify_rejection r with + | (PP.No_commits_ahead_of_base | PP.Worktree_missing), None -> true + | ( PP.Branch_ref_missing _ | PP.Branch_switched _ + | PP.Local_missing_remote_commits _ + | PP.Local_diverged_from_remote_commits _ ), + Some rej -> + Push_reject_classify.is_permanent rej + | _ -> false))🤖 Prompt for 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. In `@test/test_push_plan_properties.ml` around lines 385 - 399, The property currently treats a None result from PP.to_push_reject_classify_rejection as success, letting regressions slip through; update the test in test_push_plan_properties.ml so that for each refusal in refusals you assert PP.to_push_reject_classify_rejection r returns Some rej and that Push_reject_classify.is_permanent rej is true (i.e., replace the permissive None -> true logic with a check that rejects are present and permanent), referencing PP.to_push_reject_classify_rejection and Push_reject_classify.is_permanent to locate the code to change.lib_core/push_plan.ml (1)
73-93: ⚡ Quick winDerive
Local_state_unsafe.reasonfromshort_labelto prevent label drift.
to_push_reject_classify_rejectionduplicates refusal labels as string literals. Ifshort_labelchanges later, these can silently diverge from the documented contract.♻️ Proposed refactor
+let refusal_reason (r : refusal) = short_label (Refuse r) + let to_push_reject_classify_rejection (r : refusal) : Push_reject_classify.rejection option = match r with | No_commits_ahead_of_base | Worktree_missing -> None - | Branch_ref_missing _ -> - Some - (Push_reject_classify.Local_state_unsafe - { reason = "refuse_ref_missing" }) - | Branch_switched _ -> - Some - (Push_reject_classify.Local_state_unsafe - { reason = "refuse_branch_switched" }) - | Local_missing_remote_commits _ -> - Some - (Push_reject_classify.Local_state_unsafe - { reason = "refuse_local_behind" }) - | Local_diverged_from_remote_commits _ -> - Some - (Push_reject_classify.Local_state_unsafe - { reason = "refuse_local_diverged" }) + | ( Branch_ref_missing _ | Branch_switched _ | Local_missing_remote_commits _ + | Local_diverged_from_remote_commits _ ) -> + Some + (Push_reject_classify.Local_state_unsafe + { reason = refusal_reason r })🤖 Prompt for 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. In `@lib_core/push_plan.ml` around lines 73 - 93, The function to_push_reject_classify_rejection emits hard-coded reason strings for several refusal variants; change it to derive Local_state_unsafe.{ reason = ... } from the canonical short_label for the given refusal instead of string literals. Concretely, in to_push_reject_classify_rejection (matching on the refusal type 'r'), replace occurrences like { reason = "refuse_ref_missing" } / "refuse_branch_switched" / "refuse_local_behind" / "refuse_local_diverged" with { reason = short_label r } (or call whatever existing short_label/refusal_short_label function exists) so the reason always follows the refusal's short_label; keep the same match arms and return type Push_reject_classify.rejection option.
🤖 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/push_plan.mli`:
- Around line 28-32: Update the top-level documentation list of refusals that
map to the planner-only state by adding Branch_ref_missing to the existing
entries (Branch_switched, Local_missing_remote_commits,
Local_diverged_from_remote_commits) so the text and the Local_state_unsafe
mapping match to_push_reject_classify_rejection and the later API docs; locate
the paragraph mentioning Local_state_unsafe in push_plan.mli and include the
symbol Branch_ref_missing in that list wording.
In `@lib/worktree_setup.ml`:
- Around line 52-57: When detecting a repo-root checkout conflict (the
W.is_checked_out_in_repo_root br branch) do not just log and return None; treat
it as the same permanent local-state refusal as W.create refusals by returning a
Push_reject_classify.Local_state_unsafe with the same reason constructed by
start_point_refusal_rejection (so the Session_push_failed path is taken). Apply
the same change to the other similar branch around the 154-165 area so both
repo-root checks produce the same Push_reject_classify.Local_state_unsafe
outcome instead of being treated as missing worktree/retriable.
---
Nitpick comments:
In `@lib_core/push_plan.ml`:
- Around line 73-93: The function to_push_reject_classify_rejection emits
hard-coded reason strings for several refusal variants; change it to derive
Local_state_unsafe.{ reason = ... } from the canonical short_label for the given
refusal instead of string literals. Concretely, in
to_push_reject_classify_rejection (matching on the refusal type 'r'), replace
occurrences like { reason = "refuse_ref_missing" } / "refuse_branch_switched" /
"refuse_local_behind" / "refuse_local_diverged" with { reason = short_label r }
(or call whatever existing short_label/refusal_short_label function exists) so
the reason always follows the refusal's short_label; keep the same match arms
and return type Push_reject_classify.rejection option.
In `@test/test_push_plan_properties.ml`:
- Around line 385-399: The property currently treats a None result from
PP.to_push_reject_classify_rejection as success, letting regressions slip
through; update the test in test_push_plan_properties.ml so that for each
refusal in refusals you assert PP.to_push_reject_classify_rejection r returns
Some rej and that Push_reject_classify.is_permanent rej is true (i.e., replace
the permissive None -> true logic with a check that rejects are present and
permanent), referencing PP.to_push_reject_classify_rejection and
Push_reject_classify.is_permanent to locate the code to change.
🪄 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: 92babcb1-2f9d-48a2-beba-74cced4905ec
📒 Files selected for processing (8)
lib/worktree.mllib/worktree_setup.mllib_core/push_plan.mllib_core/push_plan.mlilib_core/push_reject_classify.mltest/test_push_plan_integration.mltest/test_push_plan_properties.mltest/test_push_reject_classify_properties.ml
--force-with-lease alone only verifies that the remote tracking ref matches actual remote — once a background fetch refreshes that tracking ref, the lease passes even when local is strictly behind, silently wiping remote commits. PR #315 was force-pushed empty and auto-closed this way. --force-if-includes additionally requires that the remote tip is in the local branch's reflog, which refuses pushes that would lose commits the local clone never observed. Requires git ≥ 2.30 (already a prerequisite for the worktree features onton relies on). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
[Worktree.create] used to take the local branch ref as authoritative when it existed in the user's working clone. The user's [refs/heads/<branch>] could be stale (left at the PR's base, behind the real PR head), so the worktree would start missing every PR commit — exactly the state that wiped PR #315 when the session pushed it. Split the decision into a pure module [Start_point_plan] in lib_core/ that takes observed local/remote-ref SHAs and a precomputed two-way ancestry, returns one of [Reset_and_use_remote_tracking], [Use_local_branch_unchanged], [Create_new_branch_from_base], or a [refusal] for the diverged/local-ahead cases. The effectful side in [Worktree.create] gathers the inputs, calls the planner, executes the chosen action via [git worktree add -B/-b]/[add], and returns [(t, refusal) Result.t]. [Worktree_setup.ensure_worktree] runs a new [fetch_origin_branch] before [create] so the planner observes a fresh remote ref. Property tests (SPP-1..9) cover totality, determinism, the "no-silent-clobber" invariant, remote authority, label bounds, and exhaustive variant reachability. An integration test exercises every arm against real git fixtures, including a direct reproduction of the #315 stale-local-ref state. Refusals are logged through the existing activity-log channel and return [None] from [ensure_worktree], so the next scheduling tick sees the agent idle. Future commits will route refusals through [needs_intervention] explicitly via [Push_reject_classify]. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
[Worktree.force_push_with_lease] used to gate on [git rev-list --count <base>..HEAD] (worktree HEAD) and then push the named branch — so an agent that [git switch]ed to a different local branch mid-session could pass the gate with commits the push command would never upload. Combined with --force-with-lease's blind spot for local commit loss (the lease passes whenever the local tracking ref matches actual remote, even if local strictly lacks remote commits), this put PR #315 in the position where a push of a stale branch wiped its remote. New pure module [Push_plan] in lib_core/ takes the observed worktree HEAD, branch ref SHA, remote tracking SHA, two-way ancestry, and [base..refs/heads/<branch>] commit count, and returns [Push (Force_push_if_includes | Initial_push)] or [Refuse (No_commits_ahead_of_base | Worktree_missing | Branch_ref_missing | Branch_switched | Local_missing_remote_commits)]. The named branch — never HEAD — is what gets measured and pushed. [Push_reject_classify.rejection] gains a [Local_state_unsafe] variant (permanent), and [Push_plan.to_push_reject_classify_rejection] maps the three "supervisor said no" refusals onto it so the orchestrator's existing [is_permanent] → [needs_intervention] escalation applies without new state. Property tests (PPP-1..11) cover totality, branch-switch refusal, local-missing-remote refusal, zero-commits skip, initial-push, happy-path, worktree-missing pre-emption, determinism, exhaustive variant reachability, refusal-to-rejection permanence, and label bounds. PRC tests extend their existing variant scans for Local_state_unsafe. An integration test drives real-git fixtures for the branch-switched and local-missing-remote refusal arms plus the happy-path force-push, asserting nothing reaches the remote when the planner refuses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
subsetpark
force-pushed
the
worktree-start-point-planner
branch
from
02b2e7d to
593c5e5
Compare
Review SummaryRound 4 delta review — no new issues found in this push. The changes in this push are clean. Key observations:
Four outstanding comments from prior rounds remain live on the PR thread but were not re-introduced by this push.
Variant: convergence-v2 Candidates: 0 | Posted: 0 | Suppressed: 0 0 comments posted · Model: |
1 participant
Summary
Three-patch fix for the bug class that caused PR #315 to be force-pushed empty and auto-closed by GitHub. Each commit is independently shippable and adds its own pure decision module + property tests.
Add --force-if-includes to session-end force-push(one-line) — the smallest possible safety lever.--force-with-leasealone passes whenever the local tracking ref matches actual remote, even when local is strictly behind on real content;--force-if-includesadditionally requires the remote tip in the local branch's reflog, refusing pushes that would silently wipe commits the local clone never observed.Plan worktree start point against fresh remote refs— newlib_core/start_point_plan.ml(pure) plusWorktree_setup.ensure_worktreefetch-before-create.Worktree.createno longer trusts the user's localrefs/heads/<branch>when it's stale; the planner returnsReset_and_use_remote_tracking/Use_local_branch_unchanged/Create_new_branch_from_baseor one of four refusal variants. Signature changes to(t, refusal) Result.t. Property tests SPP-1..9 (totality, no-silent-clobber, remote-authority, pre-emption order, determinism, label bounds, exhaustive variant reachability) plus a real-git integration test reproducing the exact Push-rejection loop guard + SSH transport auto-detect #315 stale-local state.Plan pushes against worktree state, not just lease— newlib_core/push_plan.ml(pure) plus an extension toPush_reject_classify.rejection(newLocal_state_unsafevariant, permanent). The planner takes worktree-HEAD-branch, branch-ref SHA, remote tracking SHA, two-way ancestry, andbase..refs/heads/<branch>commits-ahead (the named branch, not HEAD) and returns either a push action or one of five refusal variants.Worktree.force_push_with_leasenow consults it before invoking git; refusals route through the existingis_permanent→needs_interventionpath. Property tests PPP-1..11 plus a real-git integration test covering branch-switched, local-missing-remote, and happy-path scenarios.The architecture mirrors the existing
Push_reject_classify/Rebase_decision/Patch_decisionconvention: pure decision logic inlib_core/, effectful handlers inlib/, one property-test file per pure module.Test plan
dune buildcleandune runtestpasses (full suite, including the new SPP-* and PPP-* property tests and two new real-git integration tests)dune fmtproduces no diffrefs/heads/<branch>to the PR base (stale local), then start onton against that PR — confirm the activity log shows the fetch-then-reset path and the worktree HEAD matches the PR head, not the stale local refPush_refusedwithrefuse_local_behind/refuse_branch_switchedrather than agit pushreaching the remote🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Hardened worktree creation and push safety to prevent silent branch wipes like PR #315. Adds pure planners for start-point and push, uses
--force-if-includes, and permanently refuses unsafe local states before any git runs.Bug Fixes
--force-with-lease+--force-if-includesto block pushes that would drop unseen remote commits.origin/<branch>when local is stale, create from base if new, and refuse local-ahead/diverged.Worktree_setupmaps refusals to permanentLocal_state_unsafe.Local_state_unsafe.Worktree_setup.Refuseddistinctly fromMissingacross callers to avoid ambiguous failures and escalate permanently.Migration
Worktree.createnow returns(t, Start_point_plan.refusal) Result.t.Worktree_setup.ensure_worktreenow returnsWorktree_setup.ensure_result(Path|Missing|Refused).Worktree.Saddsfetch_origin_branch;Worktree_setup.ensure_worktreecalls it beforecreate.Push_reject_classify.rejectionaddsLocal_state_unsafe(permanent).--force-if-includes.Written for commit 593c5e5. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
API Changes
Tests