Repository navigation
Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. 📝 WalkthroughWalkthroughWorktree removal now treats a missing Git registration as partial success after filesystem removal fails. Local and SSH paths continue to metadata purge when verification confirms that the worktree is no longer registered. Unregistered leftover directories can be cleaned up, while deletion failures are logged without blocking metadata purge. SSH removal requests now include a resolved timeout. New tests cover these recovery paths. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to Remote removal can report success after a PTY shutdown failure, and SSH cleanup can recursively delete an unrelated Git checkout. Both behaviors should be corrected before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Restrict recovery to removeWorktree failures. · remove-registered-remote-worktree.ts:54-78
src/main/ipc/worktrees/removal/remove-registered-remote-worktree.ts:54-78
🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestrict recovery to
removeWorktreefailures.
stopPtysForDestructiveWorktreeRemovalandprovider.removeWorktreeshare the sametryblock. If PTY teardown fails andlistWorktreesno longer returns the target, the handler marks removal complete and purges metadata without callingprovider.removeWorktree. Limit recovery to the Git removal call so PTY teardown failures propagate.Proposed control-flow change
- rawRemovalResult = await withWorktreeRemoveStageSpan('git_remove', 'remote', async () => - Object.keys(remoteRemoveOptions).length > 0 - ? provider!.removeWorktree(canonicalWorktreePath, args.force, remoteRemoveOptions) - : provider!.removeWorktree(canonicalWorktreePath, args.force) - ) - removalCompleted = true - } catch (error) { - // Treat partial success as successful removal when git registration was already dropped. - const remainingWorktrees = await provider!.listWorktrees(repo.path).catch(() => null) - if ( - remainingWorktrees && - !remainingWorktrees.some((w) => areWorktreePathsEqual(w.path, canonicalWorktreePath)) - ) { - console.warn( - `[worktrees] Remote git worktree registration was removed but filesystem removal failed: ${canonicalWorktreePath}`, - error - ) - removalCompleted = true - } else { - throw error - } + try { + rawRemovalResult = await withWorktreeRemoveStageSpan('git_remove', 'remote', async () => + Object.keys(remoteRemoveOptions).length > 0 + ? provider!.removeWorktree(canonicalWorktreePath, args.force, remoteRemoveOptions) + : provider!.removeWorktree(canonicalWorktreePath, args.force) + ) + removalCompleted = true + } catch (error) { + const remainingWorktrees = await provider!.listWorktrees(repo.path).catch(() => null) + if ( + remainingWorktrees && + !remainingWorktrees.some((w) => areWorktreePathsEqual(w.path, canonicalWorktreePath)) + ) { + console.warn( + `[worktrees] Remote git worktree registration was removed but filesystem removal failed: ${canonicalWorktreePath}`, + error + ) + removalCompleted = true + } else { + throw error + } + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 6dc4a075-1e23-4091-bf2f-5482f477444d
📥 Commits
Reviewing files that changed from the base of the PR and between c77a82f and 07a916bd3b3e69b68462bb2061a96340a3966682.
📒 Files selected for processing (6)
src/main/ipc/worktrees-partial-removal-recovery.test.tssrc/main/ipc/worktrees/removal/remove-registered-local-worktree.tssrc/main/ipc/worktrees/removal/remove-registered-remote-worktree.tssrc/main/ipc/worktrees/removal/remove-unregistered-worktree.tssrc/main/providers/ssh-git-provider-worktree.test.tssrc/main/providers/ssh-git-worktree-provider.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| registeredWorktrees, | ||
| statPath: (path) => fsProvider.lstat!(path), | ||
| home: removalHome, | ||
| isGitRepository: async () => false |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the safety module before inspecting the relevant predicates.
ast-grep outline src/main/worktree-removal-safety.ts --items all
# Verify whether dangerous-path handling rejects descendants of repo.path.
rg -n -C 16 'isDangerousWorktreeRemovalPath|canCleanupUnregisteredOrcaLeftoverDirectory' \
src/main/worktree-removal-safety.ts
# Inspect the remote cleanup call and available SSH Git provider operations.
rg -n -C 10 'isGitRepository: async \(\) => false|deletePath\(worktreePath, true\)|provider!' \
src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts
rg -n -C 6 'class SshGitProvider|isGitRepository|rev-parse|status' \
src/main/providersRepository: stablyai/orca
Length of output: 40398
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- safety path guard ---'
sed -n '50,92p' src/main/worktree-removal-safety.ts
printf '%s\n' '--- leftover cleanup predicate ---'
sed -n '184,236p' src/main/worktree-removal-safety.ts
printf '%s\n' '--- path helper bindings ---'
rg -n -C 8 'function (containsPath|getPathOps)|const (containsPath|getPathOps)|export .*containsPath|export .*getPathOps' src/main
printf '%s\n' '--- callback call sites and provider contract ---'
rg -n -C 8 'isGitRepository' src/main/worktree-removal-safety.ts src/main/ipc/worktrees/removal src/main/providersRepository: stablyai/orca
Length of output: 12772
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- removal imports and provider setup ---'
sed -n '1,155p' src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts
printf '%s\n' '--- repository checks ---'
rg -n -C 12 'isLocalGitRepository|isGitRepository' src/main
printf '%s\n' '--- SSH provider declarations and git execution methods ---'
rg -n -C 10 'class SshGitProvider|class SshGitNoninteractiveProvider|git\.exec|rev-parse|isGit' src/main/providers src/main/gitRepository: stablyai/orca
Length of output: 45543
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- removal imports and remote branch ---'
sed -n '1,35p' src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts
sed -n '105,135p' src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts
printf '%s\n' '--- provider files declaring repository checks ---'
rg -l 'class SshGitProvider|isGitRepository|git\.exec' src/main/providers src/main/git | head -40
printf '%s\n' '--- exact provider declarations ---'
for f in $(rg -l 'class SshGitProvider|class SshGitNoninteractiveProvider' src/main/providers); do
echo "FILE:$f"
rg -n -C 12 'class SshGitProvider|class SshGitNoninteractiveProvider|isGitRepository|git\.exec|rev-parse' "$f"
doneRepository: stablyai/orca
Length of output: 6701
Preserve the remote Git repository guard. canCleanupUnregisteredOrcaLeftoverDirectory uses isGitRepository to reject a target that has no local .git marker but is still inside a parent Git repository. The SSH branch passes async () => false, so a target inside an unrelated parent checkout can pass the cleanup predicate and reach recursive deletePath(worktreePath, true). isDangerousWorktreeRemovalPath only rejects the configured repository and its descendants. Use the SSH Git repository check for this callback.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new SSH leftover-cleanup path hardcodes isGitRepository to false, disabling the "don't recursively delete a directory that is still a git repository" guard that the local path keeps — and the PR body states that guard "remains completely intact." An existing remote probe can preserve it.
Reviewed changes
- Partial success on dropped registration —
remove-registered-remote-worktree.tsandremove-registered-local-worktree.tsre-list worktrees when git removal throws and, if the target is no longer registered, treat the removal as complete and continue to push-target cleanup and metadata purge. - SSH leftover-directory cleanup —
remove-unregistered-worktree.tsnow computescanCleanLeftoverDirectoryforrepo.connectionIdhosts too, and catches filesystem-delete failures so they no longer block the metadata purge. - Purge deduplication — the three duplicated metadata-purge tails are folded into
finalizeUnregisteredWorktreePurge. - SSH remove timeout —
git.removeWorktreenow usesresolveWorktreeAddTimeoutMs()(default 180s) instead of the 30s mux default. - Tests — new
worktrees-partial-removal-recovery.test.ts(4 recovery cases) plus the updated timeout assertion inssh-git-provider-worktree.test.ts.
ℹ️ Leftovers from a registered partial success are unreachable by the new cleanup
When the registered-removal path now treats a dropped registration as success, it purges the worktree metadata immediately. The newly-added canCleanupUnregisteredOrcaLeftoverDirectory path — the one this PR adds for SSH — requires persisted Orca provenance on removedMeta (worktree-removal-safety.ts:199-201), so it cannot be reached for that path: the workspace card is gone, no record remains, and the only trace of the files still on disk is the console.warn. The cleanup does rescue records stranded by older builds (the #21763 case), so this may be acceptable; if so, it is worth stating explicitly, and if not, the partial-success branch may need to attempt the proven-orphan cleanup before purging or surface a user-visible warning rather than a log line.
Technical details
# Leftover cleanup gated on metadata the partial-success path deletes
## Affected sites
- `src/main/ipc/worktrees/removal/remove-registered-remote-worktree.ts:67-82` and `remove-registered-local-worktree.ts:213-233` — purge metadata on dropped registration.
- `src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts:116-147` — leftover cleanup requires `meta` with Orca provenance.
## Open questions for the human
- Is "drop the record, leave the files" the intended terminal state for the just-fixed flow, or should the partial-success branch try the orphan/leftover cleanup before purging?ℹ️ Nitpicks
remove-registered-remote-worktree.ts:55-82— thecatchwraps bothstopPtysForDestructiveWorktreeRemovalandprovider.removeWorktree, whereas the local counterpart'scatchcovers only the git removal (local PTY failures propagate). Narrowing the remotetryto the git call keeps the two flows symmetric.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| registeredWorktrees, | ||
| statPath: (path) => fsProvider.lstat!(path), | ||
| home: removalHome, | ||
| isGitRepository: async () => false |
There was a problem hiding this comment.
The SSH leftover path hardcodes isGitRepository to false, so canCleanupUnregisteredOrcaLeftoverDirectory returns true unconditionally at worktree-removal-safety.ts:233. That skips the check the local path keeps via isLocalGitRepository — refusing a leftover that is itself, or is nested inside, a live git repository (worktree-removal-safety.test.ts:462 → rejects unregistered leftover directories that still answer git status). isDangerousWorktreeRemovalPath only rejects a target that contains repo.path (containsPath(parent, child) at worktree-removal-home-guard.ts:66), so a leftover nested inside a repo is not caught by any other guard. SshGitProvider.isGitRepoAsync (ssh-git-provider.ts:107, relay git.isGitRepo) already exposes the remote equivalent.
Technical details
# Preserve the foreign-repository refusal on SSH leftover cleanup
## Affected sites
- `src/main/ipc/worktrees/removal/remove-unregistered-worktree.ts:129` — `isGitRepository: async () => false`
- `src/main/worktree-removal-safety.ts:232-233` — `return !(await args.isGitRepository(args.runtimeWorktreePath))`
- `src/main/providers/ssh-git-provider.ts:107-112` — `isGitRepoAsync`
## Required outcome
- The SSH leftover branch must refuse a recursive delete when the target still resolves as a git repository (or as a subdirectory of one), matching the local path before purging metadata.
## Suggested approach (optional)
- Pass the existing remote probe: `isGitRepository: async (path) => (await provider!.isGitRepoAsync(path)).isRepo`.
- Older relays may not implement `git.isGitRepo` (`isJsonRpcMethodNotFoundError`); degrade to refusing the delete rather than assuming `false`, per the `live`/`unverifiable`/`exited` rule in `docs/reference/ssh-execution-boundary.md`.
## Open questions for the human (optional)
- If the probe is deliberately skipped to unblock #21763, document the SSH asymmetry and the accepted residual risk instead of leaving `false` unexplained.
On an SSH host, `git worktree remove --force` drops the worktree's registration even when deleting the files fails (or the 30 s relay request times out first). Orca then kept the record, and every retry was refused as an unregistered path, so the card could never be removed. - When the remote git removal throws but a fresh listing no longer contains the worktree, treat the removal as done. Only the git call is covered: a terminal teardown failure or an unreadable listing still fails the delete and keeps the record. - Let a retry delete an Orca-created leftover directory on an SSH host with the same rule local hosts use. The "still inside a git repository" check now asks the remote host (`git rev-parse --is-inside-work-tree`); only git's exact "not a git repository (or any of the parent directories)" line clears the recursive delete; a lost connection, a refused command or any other git error counts as a repository. - Give the SSH `git.removeWorktree` request the worktree-add timeout instead of the 30 s default. Local hosts are unchanged: main already records and retries failed local deletes (stablyai#23952). Fixes stablyai#21763
07a916b to
fb1121f
Compare
bbingz
changed the title
|
Thanks for both reviews. Pushed fb1121f, rebased on main and narrowed to SSH hosts. Local hosts are already covered on main by #23952, which records a local delete that failed after Git dropped the registration and retries it.
Tests: the new recovery suite, with ten cases, plus the worktree removal suites. That is 20 files and 252 tests, all passing. The |
|
Thank you @bbingz for the careful recovery work here. SSH projects now live on Orca's managed remote server on the host (the SSH relay and its direct SSH git path were removed in #27214/#27215), so a delete runs as a host-local delete there and gets the partial-removal recovery from #23952. Closing as fixed; please reopen if a half-done delete still sticks on current main. |
ELI5
Deleting a workspace on an SSH host can fail halfway: Git forgets the workspace, but some of its files stay on disk (for example on a container bind mount), or the request times out after 30 seconds while Git keeps working. Orca then kept the workspace card, and every later delete was refused because Git no longer knew the folder. The card could never be removed. Now Orca notices that Git already let go and finishes the delete, and a retry can clean up the leftover folder, but only when the remote host confirms the folder is not part of any Git repository.
What Changed
This PR now covers SSH hosts only. Local hosts already handle this on main: #23952 records a local delete that failed after Git dropped the registration and retries it on the next delete.
git worktree remove --forcefailed after dropping the registration (Directory not empty, orRequest "git.removeWorktree" timed out after 30000ms), Orca skipped the metadata purge. Every retry went to the unregistered path, which on SSH had no leftover cleanup and failed withRefusing to delete unregistered worktree path.removeRegisteredRemoteWorktree: only the Git removal call is wrapped. If it throws, Orca lists the remote worktrees again. When the listing is readable and the path is gone, the removal counts as done and the normal cleanup and metadata purge run. When the path is still listed, or the listing cannot be read, the original error is thrown and the record stays. A terminal-teardown failure is no longer inside this recovery and always fails the delete.removeUnregisteredWorktree: SSH hosts can use the same leftover rule local hosts use (canCleanupUnregisteredOrcaLeftoverDirectory: Orca-created provenance, a resolved home directory, not a dangerous path, no.gitmarker, and not inside a Git repository). The "inside a Git repository" check now asks the remote host withgit rev-parse --is-inside-work-treethrough the existing relaygit.exec. Only Git's exactfatal: not a git repository (or any of the parent directories)line allows the recursive delete. A lost connection, a refused command, a dubious-ownership refusal, a stop at a filesystem boundary, or any other error counts as "is a repository" and the delete is refused. A failed remote delete keeps the record, so it can be retried.SshGitWorktreeProvider.removeWorktreeusesresolveWorktreeAddTimeoutMs()(default 180 s, configurable) instead of the 30 s request default, matchinggit.addWorktree.Why
Once Git has dropped the registration, keeping Orca's record only traps the user, because Orca itself refuses to delete an unregistered path. The leftover cleanup reuses the existing local safety rule instead of a weaker SSH copy, and the repository probe follows the SSH execution rule that losing contact is never evidence that a delete is safe. No new relay method is needed:
git rev-parseis already allowed throughgit.execon existing relays, so mixed client and relay versions keep working, and an older relay that refused the command would only make Orca refuse the delete.Known limit: when the registered-removal recovery in step 1 runs, files Git could not delete stay on the remote disk and are only logged, because the record that would let a retry find them is purged. That is the trade-off that removes the stuck card for worktrees without Orca provenance.
Second known limit: a leftover folder nested inside the main checkout (workspaces kept inside the repository folder) still answers as part of that repository, so the leftover rule refuses it, exactly as it does on local hosts. Records stranded that way by older builds still need a manual cleanup; new partial failures are handled by step 1.
Linked Issue
Fixes #21763
Visual Proof
N/A- main-process worktree removal logic only; no UI change.Testing
src/main/ipc/worktrees-partial-removal-recovery.test.ts:ssh-git-provider-worktree.test.tschecks the removal timeout.tc:nodetypecheck,oxlintandoxfmt --checkon changed files.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
The repository-root, home, parent-directory and registered-worktree guards are unchanged, and the "not inside a Git repository" guard now applies on SSH hosts too. Like the local probe, the remote probe runs Git with the host's environment, so a
GIT_CEILING_DIRECTORIESset on the SSH host can hide an enclosing repository from it; tightening that would need a relay change and is left out of this PR.Checklist