Repository navigation
Worktree delete: keep Git's safe delete; ask the review host instead of a local squash proof - #24269
brennanb2025 wants to merge 7 commits into
Conversation
…a local merge proof When `git branch -d` refuses after a worktree delete, Orca no longer runs a background merge-tree / cherry / patch-id proof (which wrote objects into the repo and ran practice merges, and fetched with --prune). It asks the review host once, capped at 10 s and outside the per-repo branch-delete queue, and deletes the branch at its exact head only when the merged review's head is that commit (or GitHub/GitLab confirm it belongs to the review). Otherwise the branch is kept with the existing "branch kept" toast. Never -D.
…head Orca cuts workspace branches with --no-track, so `git branch -d` compares them against the main checkout's HEAD, not their base. A workspace with no commits of its own was refused (and kept with a "local commits" warning) whenever local main was behind origin/main, the main checkout was on another branch, or the workspace was opened on someone's review branch. When -d refuses, check with `merge-base --is-ancestor` whether the head is in the history of the branch's saved base (branch.<b>.base), the remote's default branch (origin/HEAD) or HEAD, and if so delete it with the existing guarded update-ref delete. Local reads only: no fetch, no objects written. The same shared check runs on the SSH relay. Only a branch no base holds goes on to the review-host question.
…e every kept branch Serial delete paths (WSL deletes inside the per-repo acceptance queue, Workspace Cleanup's one-at-a-time loop, SSH host removal) asked the review host once per workspace, so a hung host cost 10 s per workspace. After a lookup hits the 10 s cap, later kept branches in that repo on that host are kept without asking for 60 s. In memory only, it expires by time; a fast lookup error does not open it. Which removal paths asked the review host was decided caller by caller: stale-registration removals and quit-interrupted removals that Git had already unregistered kept the branch without asking. settleKeptBranch / settleKeptSshBranch are now the only route from removal code to the push-target remote cleanup, every removal path goes through them, and a ratchet test fails if a removal file calls the cleanup directly. The worktree metadata read now happens only when a branch was kept.
…adata read A prunable registration's kept branch is now settled with the review host on the desktop path; a removal whose branch Git deleted reads no worktree metadata.
…ttle check never throw In a bare repository HEAD names a branch without checking it out, so neither `git branch -d` nor the checked-out guard refuses it. When HEAD named the workspace's own branch, the base check compared the branch with itself and deleted it, even with unpushed commits Git had just refused to drop. Every base candidate (saved base, origin/HEAD, HEAD) is now resolved through symbolic refs and dropped when it names the branch itself; a detached HEAD is pinned to the commit it names. The review-host check closures are async, so a throw while reading the workspace's metadata becomes a rejection the settle absorbs: the delete finishes and the branch is kept. The ratchet test and module comment now state exactly what the route rule enforces.
…leanup-forge-merged
…wn ref case-insensitively A detached HEAD is no branch: counting it deleted a branch whose upstream does not hold its unpushed commits (Git refuses that delete) while the main checkout was only looking at it. HEAD now counts only when it names a branch. On macOS and Windows a ref spelled in another case (`refs/heads/feat` for `Feat`) opens the branch's own ref file, so the own-ref check is now case-insensitive. Both changes only keep more branches.
Review status: ready (head bf821c2)The problem. After a workspace delete, Orca runs Git's safe branch delete ( What changes for you. When you delete a workspace:
What the review fixed (3 review rounds, plus a readiness check):
Deferred:
Verified:
Live app on a remote Mac, isolated rig, no credentials:
Loose Git objects stayed at 0 in every case (no practice merges).
Worth knowing: when there is no review host, the kept message still says "so Orca kept it to avoid losing local commits". That wording predates this PR, but it now also shows for a squash-merged branch kept offline. Rewording it is a product call, left as a follow-up. Not verified:
|
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Why: a GitLab suggestion or rebase moves the MR head past the local one; any pushed version counts. | ||
| const versionHeads = await getMergeRequestVersionHeadShas( | ||
| repo.path, | ||
| review.number, | ||
| hostedReviewSshConnectionId(executionHostId), | ||
| { localGitExecOptions } | ||
| ) | ||
| return versionHeads.includes(head) |
There was a problem hiding this comment.
If a merge request was force-pushed from commit A to an unrelated commit B and then merged, A can still appear in its version history. This check treats that earlier push as proof A was merged and deletes a preserved branch at A, even though A’s commits may not be retained elsewhere. How this was verified: The deletion decision accepts any historical GitLab version head after checking only that the merge request is merged.
Knowledge Base Used: Workspace and worktree lifecycle
| const answer = await withTimeout<boolean | 'timed-out'>( | ||
| confirmMergedAtHead(branchName, head).catch((error: unknown) => { | ||
| console.warn( | ||
| `[worktrees] Could not ask the review host whether "${branchName}" merged`, | ||
| error | ||
| ) | ||
| return false | ||
| }), | ||
| FORGE_MERGED_LOOKUP_TIMEOUT_MS, | ||
| 'timed-out' | ||
| ) | ||
| if (answer === 'timed-out') { | ||
| // Why only a timeout: a fast error costs the next delete nothing, a hang costs it the cap. | ||
| unansweredUntilByReviewHost.set(reviewHostKey, Date.now() + FORGE_UNANSWERED_WINDOW_MS) | ||
| return result |
There was a problem hiding this comment.
The ten-second deadline finishes branch settlement but does not cancel the review lookup. A slow GitLab versions request can continue running and hold a provider request slot after removal finishes, until its separate execution deadline. This leaves background work consuming capacity despite the timeout and cooldown.
Knowledge Base Used: Source control and reviews
Comments Outside DiffThese findings could not be posted inline.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Replaces the local squash-merge proof that ran after a refused git branch -d with a local base-containment check followed by a single review-host lookup.
- Base containment — new
src/shared/git-branch-base-containment.tswalks saved base,origin/HEADand the main checkout'sHEAD, resolved through symbolic refs, dropping the branch's own ref (case-insensitively) and a detachedHEAD; onemerge-base --is-ancestorper candidate, local reads only. Shared by the local Git layer and the SSH relay. - Review-host settlement — new
src/main/source-control/forge-merged-branch-cleanup.tsasks once with a 10s cap (settlePreservedBranchWithForge) and deletes only at the exact head, with an in-memory 60s "host did not answer" window keyed by execution host + repo path.settleKeptBranch/settleKeptSshBranchare now the only removal-side route to the fork-remote cleanup. - Guarded delete —
deletePreservedBranchAtHeadshares the per-repo branch-delete queue; every delete still goes throughupdate-ref -d <ref> <oldHead>and refuses a checked-out branch. - Removal paths — registered local/remote, unregistered, Windows recovery, interrupted and runtime variants all settle a kept branch; a ratchet test pins the allowed direct callers of the push-target cleanup.
- GitLab — new
getMergeRequestVersionHeadShasreads the MR/versionsheads to confirm a head the host advanced past the local one. - Docs — the
merge-tree-write-treecompatibility row no longer claims branch-cleanup use.
Verified in this run: no data-loss path in the base check (surviving-ref ancestry + guarded update-ref), no dropped or mis-propagated preservedBranch in the migrated callers, and the GitLab reader follows the established glab admission pattern. The behavioral notes (delete can wait up to 10s; temporary dependency on #24266 for the GitHub conflict lookup; mixed-version remote servers keep their old proof) are documented and intended.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (34)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughWorktree removal now checks whether a branch head is in its base history before using guarded force deletion. If the branch remains preserved, local, WSL, and SSH removal paths can check linked forge reviews and delete the branch only when the review is confirmed merged at that head. GitLab merge-request version heads support head matching. Settlement runs before push-target cleanup. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to Orca now decides whether to delete a workspace's branch from local history and confirmation from the review host. If anything is uncertain, it keeps the branch. A retained branch remains recoverable with the existing Force Delete action. No concrete defect blocking merge was found. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to GitLab review-version history is treated as proof that an older commit was merged. An older version can contain changes discarded before the final merge, so workspace removal could delete a branch that should remain available for recovery. Exact-head and checkout protections limit the affected branch but do not correct this authorization decision. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 36.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 47 functions across 31 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Plan change (corrects two items in the status comment above). The conflicting-file list on a conflicting pull request stays exactly as it is: it keeps using
|
|
Closing. Asking the review host instead of the local squash-merge check changes which branches get deleted after a workspace is removed, and offline cleanup would stop working. #22356 fixes the throwaway objects that check left behind while keeping the same deletion rule. |
ELI5
When you delete a workspace, Orca also deletes its branch with Git's safe delete. If Git refused (usually because the pull request was squash-merged), Orca used to quietly do practice merges in your repository to prove the branch was already merged, which left junk files in the repository and sometimes fetched from the network. Now Orca first checks, without the network, whether the branch's commit is already part of the branch it was created from; if so it deletes it. Otherwise it asks the review host (GitHub, GitLab, Bitbucket, Azure DevOps or Gitea) once: "was this exact commit merged?" If yes, the branch is deleted. If not, or if the answer does not come back, the branch is kept and you get the usual "branch kept" message with a "Force Delete Branch" button.
What Changed
The problem. After a workspace delete, Orca runs
git branch -d. Git refuses when it cannot see the branch's commits in the branch it compares against, which is normal after a squash or rebase merge whose remote branch was already deleted. On every such refusal Orca ran a local "already merged" proof: agit merge-tree --write-treepractice merge of the branch into each saved base (repeated per patch-id candidate), agit cherryandgit patch-idscan, and, if the local base did not prove it, agit fetch --pruneof the base's remote with no timeout. Each practice merge wrote unreferenced Git objects into the user's repository, the same junk-object problem #22356 set out to contain. On SSH the same proof ran on the remote host through the relay.A second refusal case the proof was quietly covering. Orca creates workspace branches without an upstream (
--no-track), sogit branch -dcompares the branch against whatever the main checkout has checked out, not against the branch the workspace was created from. A workspace with no commits of its own is therefore refused when localmainis behindorigin/main, when the main checkout is on another branch, or when the workspace was opened on someone else's review branch. The old proof deleted these; dropping it without a replacement would have kept them with a warning about "local commits" that do not exist.The change. That local proof is gone, on local, WSL and SSH alike. The rule is now, in order:
git branch -d, unchanged.branch.<name>.base, which Orca writes when it creates the workspace), the remote's default branch (origin/HEAD), or the branch the main checkout has checked out (a detached main checkout counts for nothing, since no branch keeps those commits). Each candidate is first resolved to the branch it names, and one that is the workspace's own branch is never counted, compared without regard to letter case because macOS and Windows treatfeatandFeatas the same ref: in a bare repository, HEAD can name the workspace's branch without it being checked out anywhere. Onegit merge-base --is-ancestorper candidate; local reads only, no fetch, nothing written. This is the check-ditself would have made if the branch tracked its base.Every delete in steps 2 and 3 is guarded: it only removes the branch while it still points at that commit, using the same guarded delete primitive as the "Force Delete Branch" button, and refuses if the branch is checked out anywhere. Orca never uses
git branch -Dhere.-ddeletes itmainis behindorigin/mainor the main checkout is on another branch-drefuses).gitfile is left (any platform)-dand the base check are not run)-dran.-dand the local proof-d, base check, review host), whether or not Git had already dropped the workspace before the quit-dand the base check on the remote; this machine asks the review host; the guarded delete runs on the remote over the existing "delete preserved branch" request-d, the base check and the guarded delete run in the distro; the review-host question runs as it does for every other WSL review lookup-DTiming. Git's branch delete, the base check and the guarded delete wait in the per-repository queue that already serializes branch deletes. The review-host question runs outside that queue and after the file watchers are released. Local deletes on macOS, Linux and Windows run their Git delete in the background, so several deletes started together ask at the same time; each delete's reply still waits for its own answer. So on every platform, when Git refuses the branch delete and no base holds the branch, that workspace stays "Deleting…" until the review host answers: usually a second or two, at most 10 seconds. WSL deletes, Workspace Cleanup (which deletes one workspace at a time) and removing an SSH host's workspaces ask one after another, and on WSL the wait also holds the next delete in that repository. A workspace Git still lists although its folder is gone or is only a leftover
.gitfile asks before the desktop app moves on to the next delete in that repository, so it holds that delete too. When one question hits the 10 second limit, later kept branches in that repository on that host are kept without asking for the next 60 seconds, so a hung review host costs about one 10 second wait per repository per minute on every path, not one per workspace. Before, the same spot could wait on agit fetch --prunewith no limit.The mechanism.
src/shared/git-branch-base-containment.ts: reads the branch's base candidates and runsmerge-base --is-ancestor, taking a Git runner so the local Git layer (src/main/git/worktree-branch-removal.ts) and the SSH relay (src/relay/git-handler-worktree-remove.ts) share it. Every candidate is resolved through symbolic refs first; one that names the branch itself (in any letter case) is dropped, a detached HEAD is not counted, and a saved base that is a bare commit id is ignored, since it keeps nothing reachable once the branch is gone. Neither side knows about review hosts.src/main/source-control/forge-merged-branch-cleanup.ts:settlePreservedBranchWithForge(ask once with a 10 second limit, delete at the exact commit, keep on any failure), the provider check, the short in-memory "did not answer" window (keyed by execution host and repository, expires by time, opened only by a timeout, never stored), and two entry points,settleKeptBranch(local and WSL) andsettleKeptSshBranch. Each asks the review host about a kept branch and then removes the fork remote Orca added for a pull request from someone else's fork, if no local branch still tracks it, so that removal always sees the branch already deleted..gitfile; Windows recovery; deletes interrupted by quitting; leftover folders Git no longer lists; on the desktop app and the CLI, local and SSH) goes through them, andkept-branch-settlement-route.test.tsfails if a removal file calls the fork-remote removal directly. The leftover-folder paths have no branch result, so nothing is asked there. The test cannot see a caller that hands over an empty result or skips the fork-remote removal. Only the "Force Delete Branch" button handlers are allowed to call it directly, since they act after the fact.deletePreservedBranchAtHeadinsrc/main/git/worktree-removal.tsruns the existing guarded delete through the same per-repository queue as removal branch deletes.src/main/gitlab/merge-request-versions.tsreads a merge request's version heads (GET …/merge_requests/:iid/versions, newest 100).src/shared/git-branch-cleanup.ts(the practice merge,git cherry,patch-idscan andfetch --prune), the local and relay callers of it, and their tests.docs/reference/git-compatibility.md: themerge-tree-write-treerow no longer mentions branch cleanup.Not changed. No new stored data and no settings. The SSH relay protocol is unchanged: the relay's remove result is the same shape, and the guarded delete uses the existing
git.forceDeletePreservedBranchrequest. Each desktop talks only to a relay of its own version (the relay refuses a different version when it connects), so the relay change ships with the desktop. The real mixed-version case is a paired remote Orca server: it runs its own removal code, so an older server keeps its old proof, and a newer one asks the review host with the server's own GitHub or GitLab login (a server without a login keeps the branch). Git 2.25 is enough:branch -d,merge-base --is-ancestor,update-ref -d <ref> <old>,worktree list --porcelainandconfig --remove-sectionall predate it, and branch cleanup no longer needs Git 2.38.Why
The practice merge was a local guess at a question that has two exact answers. If the branch's commit is already in the history of the branch it was created from, deleting it loses nothing, and Git answers that locally with a few reads. If not, the review host answers "was this exact commit merged?" exactly. Together that is cheaper (local reads, then one review lookup, where GitLab adds one read of the merge request's versions, instead of fetches and practice merges), writes nothing into the repository, works the same for SSH, and covers cases the practice merge got wrong (a revert after merge, an abandoned stack parent).
The safety argument: in step 2 the commit stays reachable from another branch in the same repository, which is Git's own
-dguarantee. In step 3 the host merged the review, so every commit of it is kept on the host; if the local branch still points at the review's head, or at a commit the host confirms belongs to the review, deleting the local branch loses nothing the host does not hold. One extra local commit and both checks fail, so the branch is kept. The delete itself refuses if the branch moved in the meantime.Alternatives considered:
-dsucceeds, so that would spend a host request on almost every delete to save at most one 10 second wait on the rarer refusal.Where this fits
Orca runs
git merge-treein two places: the conflicting-file list on a conflicting pull request, and (before this PR) the branch-cleanup proof after a workspace delete. Each run wrote junk objects into the user's repository.git merge-treeat all. It checks the branch's base locally, then asks the review host.git merge-tree, and fix(git): keep merge-tree's throwaway trees out of the real object store #22356 keeps its objects out of the user's repository. Conflicting PRs: stop running a background merge to list files; the agent's own merge shows them #24266, which would have dropped that list, is on hold.This PR deletes nothing the conflicting-file list uses:
src/shared/git-merge-tree-capability.ts, themerge-tree-write-treecapability entry, its row in the compatibility doc (now describing only the conflict list) and its real-Git compatibility test all stay. It deletes only the branch-cleanup proof (src/shared/git-branch-cleanup.ts) and its local and relay callers.#22356 also edits those branch-cleanup files. Whichever of the two lands second drops #22356's branch-cleanup part and keeps its conflict-list part.
The review-host step reuses Orca's existing pull request lookup. When GitHub returns an open pull request that has merge conflicts, that lookup still computes the conflicting-file list, the same work the pull request card's refresh already does today.
This work is part of the wider worktree-create latency plan: #23699 is its step 1, and #23698 is tracked separately.
Differences from the common pattern
Linked Issue
Part of the plan described in "Where this fits" (replaces the approach in #22356 for branch cleanup only; #22356 still covers the conflicting-file list).
Visual Proof
Recorded on a remote Mac in an isolated rig without credentials. Main is the before, this PR (3bc6b08) is the after.
A review host saying "merged" needs a credential in the rig, which our QA rules forbid; mocked-review-host and real-Git tests cover it.
Testing
Typecheck (node project, which includes
src/sharedand the relay), after merging currentmain: passes.Unit tests, after merging current
main: 95 test files that import a changed module or exercise workspace removal (desktop and CLI removal, relay Git handler, recovery, background removal, fork-remote removal,orca-runtime.test.ts): 2261 passed, 2 skipped.New tests:
mainis behindorigin/main(base known only fromorigin/HEAD), while the main checkout is on another branch, and when opened on someone's review branch; a workspace with one unpushed commit is kept with its head; in a bare repository (both a bare clone and a.barefolder behind a.gitfile), the branch HEAD names, with an upstream and an unpushed commit, is kept; so is a branch with an upstream and an unpushed commit while the main checkout is detached at it, and (on macOS, whose filesystem ignores letter case) a bare repository's branchFeatwhose HEAD saysfeat. Unit: base candidates (no repeats, short names qualified, a detached HEAD not counted, the branch named in another letter case ignored, bare commit ids ignored, the branch itself ignored directly or through a symbolic ref, a malformed head runs no Git)..gitfile is left asks the review host about the branch it kept and deletes it when merged at that commit.-druns no practice merge, cherry, patch-id or fetch; the guarded delete waits in the same per-repository queue as removal branch deletes.Each new rule was checked to fail its test when reverted: the base check (local: 3 tests red; relay: 1), treating every head as contained (the keep tests red), the
origin/HEADcandidate, ignoring commit-id bases, the window opening, expiring and staying closed on fast errors, asking about the branch an interrupted delete kept, asking about the branch a leftover.gitfile row kept, reverting any one removal file's switch to the two entry points (the test that only allows these two entry points goes red), reading the workspace's saved data only when the review host must be asked, never counting the branch as its own base (the bare-repository tests red), and turning a throw while reading saved data into a kept branch. Earlier rules (merged, exact commit, GitHub containment, GitLab versions, Bitbucket minimum, keep on timeout and on delete failure, the question outside the queue, after the watcher gate) were checked the same way in the first round.Lint and the changed-code quality gate pass.
Platforms: macOS unit tests; live app QA on a remote Mac (see Visual Proof).
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
.gitfile, Windows recovery, a delete interrupted by quitting), now asks the same way; the guarded delete makes that safe whether or not-dran.Author: @BrennanKB5
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)