Repository navigation
fix(repos): stop reporting clones that never ran as successful - #23770
Conversation
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Shared checkout checker — the new
src/main/git/registered-clone-target.tsinspects<path>/.gitasynchronously on both local and SSH hosts, returningvalid/absent/invalid/unverifiableunderscanWorktreeMarkerSync's validity rules, with WSL confirmation before trusting a negative. - Local clone —
repo-clone-lifecycle.tsprobes the registered checkout before reusing a row and now matches only local-host rows; a missing.gitre-clones and keeps the row, an invalid/unverifiable checkout throws instead of reporting success. - Runtime clone —
runtime-repository-clone-controller.tsapplies the same probe and now excludes SSH-owned rows from its target lookup. - SSH clone —
remote-repo-clone.tsclaims the in-flight key before probing, recognizesexecutionHostId: ssh:*-only rows, and updates a stale row after re-cloning. - Bounded local read —
runBoundedLocalPathReadruns a read under the shared two-slot probe semaphore; new host-probe tests cover the timeout and both WSL confirmation outcomes.
I also reverted repo-clone-lifecycle.ts to the base revision locally and confirmed 4 of the new repos-clone-registered-target.test.ts cases fail without the fix, so the regression coverage is real.
ℹ️ Clone probes share the polling semaphore, so a hung read elsewhere can block a healthy clone
inspectLocalCloneCheckout reads through runBoundedLocalPathRead, which acquires the module-global localWorktreePathProbeSemaphore (2 slots) also used by sidebar worktree polling. An in-flight read holds its slot until the underlying stat settles, so two hung reads on an unrelated mount can keep both slots busy and make a clone target probe abort at its 2s deadline, reporting could not check the existing project … try again once the folder is reachable for a destination that is perfectly healthy. Not blocking — the result is safe (no false success) — just flagging the coupling in case a dedicated bound for the clone probe is warranted.
Technical details
# Shared semaphore can starve clone probes
## Affected sites
- `src/main/local-worktree-path-presence.ts` — `runBoundedLocalPathRead` releases the semaphore slot only in `read().finally(release)`, so a `stat` that never settles holds the slot.
- `src/main/git/registered-clone-target.ts` — `inspectLocalCloneCheckout` runs up to ~7 reads (`.git`, then `HEAD`/`objects`/`refs`, then `commondir`) under one 2s deadline, so they all queue on the same two slots.
## Required outcome
- A clone probe against a reachable destination should not be reported as "did not respond in time" because unrelated reads are occupying the shared slots.
## Suggested approach (optional)
- Give `inspectLocalCloneCheckout` its own semaphore/limit, or acquire a single slot for the whole inspection so one clone never depends on unrelated polling reads.
## Open questions for the human (optional)
- Is the coupling intentional (the body describes reusing the two-slot limit to cap threads), and if so is a starved-but-healthy clone acceptable?DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📥 CommitsReviewing files that changed from the base of the PR and between 88ca8a47433ace6471b278cff31b5b41a0b261da and 884cb39. 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughClone handling now verifies saved repositories against Git checkout state and remote identity before reuse. Local, runtime, and SSH paths use the shared saved-target check with host-specific Git probes. The clone paths filter saved repositories by path and execution host. When they return an existing repository, they update worktree or session-root state. Tests cover matching and mismatching remotes, incomplete checkouts, occupied paths, cancellation, and concurrent clone requests. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change makes local, runtime and SSH clones verify a saved project's checkout and remote before reusing it. This prevents false-success clones. No actionable merge-blocking risk was identified in the supplied context. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change replaces path-only reuse with checks that the requested repository exists on the correct machine. Cancellation and restoration handling support that stronger boundary. No introduced security concern was established, but remote enforcement and broader security coverage remain incompletely verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The clone changes support [
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 |
Design review: the reported case is fixed, but the approach has a basic problemThe diagnosis is right: a saved project outlives its folder, and clone trusted the saved project. The repro is real and the new tests fail on The problem is the fix itself. It keeps the assumption that "a saved project at this path means the clone already happened". Then it adds about 200 lines of filesystem checks to predict what 1. Most of the new checks guard against failures git already refusesReal git (2.52) at the destination:
Here's what happens when each checker verdict is wrong:
The "Why" section says a broken or unclear 2.
|
9e4fd8d to
a77ea35
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 5cb6ec56-ba45-4f39-ba1d-9d52a7a3438a
📥 Commits
Reviewing files that changed from the base of the PR and between 9e4fd8d1d1e52c2bd3ff7efcf18c36ac04ee5cce and a77ea35164b1f408de502f2800dd8f3fa50335d3.
📒 Files selected for processing (9)
src/main/git/saved-clone-target.test.tssrc/main/git/saved-clone-target.tssrc/main/ipc/repos-local-clone-lifecycle.test.tssrc/main/ipc/repos-remote.test.tssrc/main/ipc/repos/remote-repo-clone.tssrc/main/ipc/repos/repo-clone-lifecycle.tssrc/main/repo-git-remote-identity.tssrc/main/runtime/orca-runtime-tests/repository-project-operations-part-02.spec.tssrc/main/runtime/runtime-repository-clone-controller.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.
|
Thanks for the review, and for spelling out how it was produced. You were right about the premise: the first version made "a saved project at this path means the clone happened" stricter, instead of checking the thing the dialog promises. I've replaced the branch with a redesign built on your suggested design. What it does now When a saved project sits at the clone path on the machine doing the clone, the desktop local clone, the SSH clone and the runtime clone make the same decision (
The host routing and timeouts come from #2981's queued retry still works because the second request finds the first request's finished clone. The same applies to #5071's SSH repeat clone and a repeat
Where I went a different way from your suggestion, and why (happy to switch either one):
Not covered, on purpose
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This revision replaces the previous filesystem-marker approach entirely; the prior head (9e4fd8d) is not an ancestor of the new one, so the full diff was reviewed as the delta.
- New shared decision —
reuseSavedCloneTarget(src/main/git/saved-clone-target.ts) returns the saved project only when git on the owning host shows the folder is its own checkout (rev-parse --show-cdupempty,--verify HEADresolves) with a singleoriginURL naming the requested repo; otherwise it lets git clone when the project's storedoriginidentity matches, and throws naming the project. - Host-routed probe —
runGitProbeOnHostextracted fromprobeGitRemoteIdentity(src/main/repo-git-remote-identity.ts), preserving local/WSL/SSH routing and the 5s/20s timeouts; both commands are on the relay's read-only allowlist. - Call sites — the local (
repo-clone-lifecycle.ts), SSH (remote-repo-clone.ts) and runtime (runtime-repository-clone-controller.ts) clones all probe before reusing. Local/runtime skip SSH-owned rows; SSH probes after the in-flight guard using the clone's own cancel handle; a real re-clone into a saved row invalidates/notifies. - Tests — real-git unit tests for the decision plus IPC and runtime boundary tests. Verified locally:
saved-clone-target(8),repos-remote(35),repos-local-clone-lifecycle(27),repo-git-remote-identity+ enrichment (39) all pass.
All four prior inline threads (Greptile) are marked resolved and superseded by this revision.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
a77ea35 to
e3ce705
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Delta since the prior pullfrog review (a77ea35), i.e. commit e3ce705:
- SSH saved-project lookup widened —
cloneRemoteRepo'sfindSavednow matches viagetRepoSshConnectionId(repo) === args.connectionId(src/main/ipc/repos/remote-repo-clone.ts:65), so a saved SSH project stored with onlyexecutionHostId: ssh:<target>is found, matching how SSH registration writes rows. Previously such a row was missed and git would clone over it. - Parameterized SSH reuse test —
repos-remote.test.tsnow runs the "returns an existing SSH repo" case over bothconnectionIdandexecutionHostId: ssh:*-only ownership, and asserts no clone/add happened. - Test-only helper refactor —
readGitExecOptionsreplacesgitOptionin the local clone lifecycle test, typing the shared spy's(argv, options)shape.
Verified locally at e3ce705: saved-clone-target (8), repos-remote (36), repos-local-clone-lifecycle (27) all pass. No open Pullfrog inline threads remain.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Delta since the prior pullfrog review (e3ce705), commits aa326d2, f5721c4, 786c849, 88ca8a4.
- Empty-repository repeat clone now reuses —
isFinishedCheckoutsplits the HEAD check:rev-parse --show-cdup --verify HEAD, and on failurerev-parse --show-cdup(top level) plussymbolic-ref HEAD. A finished clone of a repository with no commits has an unborn HEAD, which--verifyrejects butsymbolic-refresolves; a killed clone'sHEAD -> refs/heads/.invalidis an invalid refname, sosymbolic-refstill fails (verified empirically). - Worktree root is re-prepared after a local/runtime re-clone —
repo-clone-lifecycle.tsandruntime-repository-clone-controller.tscallprepareLocalWorktreeRootForRepo+invalidateAuthorizedRootsCache(and notify/invalidate) when git re-creates a saved project's folder, since the deleted folder took its worktree root with it. - SSH re-clone registers the recovered root —
remote-repo-clone.tsnotifiessession.registerRootwhen git re-creates a saved SSH project's folder, becauseaddRemoteRepoFromPathreturns an existing project without registering it. - Refusal message says which reason applies — the error now names the stored
originURL (Orca has that project recorded as <url>) or reports that there is no record, since the two cases need different user action.
The new real-git tests fail on the prior revision (unborn-HEAD reuse, killed-clone refusal, detached HEAD, nested-in-empty-clone) and the IPC/runtime tests assert the new worktree-root and registerRoot side effects. No open Pullfrog inline threads remain.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
88ca8a4 to
a4dc55f
Compare
nwparker
left a comment
There was a problem hiding this comment.
Saved clone reuse requires bounded host-targeted Git verification of checkout and requested origin; identity is rechecked after await and cancellation/path locks retained. Reclone repairs roots/cache registration. Rebase retained upstream promptGuardGitEnv. 92 tests, node typecheck and quality gate pass. Refuses ambiguous stale projects instead of reporting false success.
Required PR Checks verify passed on this exact commit: a4dc55f2cbde7d938e0e6409baead18077cc7b9f.
nwparker
left a comment
There was a problem hiding this comment.
Pre-merge audit changes the risk assessment to high; this PR is excluded from the requested low-risk bug-fix merge batch.
The current head a4dc55f2cbde7d938e0e6409baead18077cc7b9f still includes the stored origin remoteUrl verbatim in the refusal error in src/main/git/saved-clone-target.ts. Git URLs can contain credentials, so the diagnostic needs redaction or omission of the raw URL before merge.
The unresolved interrupted-clone finding also needs a regression test covering SSH cancellation during fetch and proof that an unborn HEAD is sufficient evidence of a completed empty clone. The existing symbolic-ref fallback does not establish clone completion independently.
CI is green, but these unresolved bot findings prevent the low-risk merge decision. My earlier approval overlooked the credential disclosure and is superseded by this review.
A saved project outlived its folder, so Clone reported success without running git. Fixes stablyai#23563
A clone of a repository with no commits leaves HEAD unborn, which `rev-parse --verify HEAD` rejects exactly like the `refs/heads/.invalid` HEAD a killed clone leaves. The saved-project check read both as "not a finished clone", so cloning a brand-new empty repo a second time — a double-clicked Clone, a re-run `orca project setup-clone`, or stablyai#2981's queued retry — fell through to git and died on "destination path already exists and is not an empty directory". Ask the two questions apart when `--verify` fails: `symbolic-ref HEAD` resolves an unborn branch but not a killed clone's HEAD. It is already on the relay's read-only allowlist. Probes also short-circuit once the clone is cancelled, so no probe after Cancel spawns git.
…lder The branch that re-clones into a saved project's folder returned the project without the two steps every sibling branch runs: the worktree root that vanished with the deleted folder is never re-created, and the authorized-roots cache keeps the answers it cached while the folder was missing. Creating a worktree or reading files in the recovered project then fails on a root that is not there.
`addRemoteRepoFromPath` returns an existing project before it notifies `session.registerRoot`, which was unreachable before the saved-project check could re-clone into a project's own path. The relay therefore never learned the folder came back, so sessions under it stayed unregistered. Notify from the clone, mirroring the folder-to-git upgrade beside it.
One message covered both "that project holds a different repository" and "Orca has no record of what that project held", which need different things from the user. Name the recorded URL when there is one, and say plainly that there is no record when there isn't — the second is the case a project predating remote-identity enrichment lands in, and the user cannot act on it without being told why. Also drops the `upstream` remote-name check into a named binding: `origin` is the only stored remote that names a project's own repo, because `Repo.upstream` and the GitHub avatar derived from it both name the repo a fork came from.
a4dc55f to
884cb39
Compare
nwparker
left a comment
There was a problem hiding this comment.
Reviewed the repaired source and independently verified the two substantive bot findings. The refusal diagnostic no longer includes the stored origin URL; a synthetic credential-bearing regression fails before the change and passes afterward. An actual killed fetch with Git 2.25 reproduces the incomplete-clone false success, and the repaired check rejects it while retaining completed empty-clone reuse.
Performance checks confirm two small owning-host reads for an ordinary reused clone, four for an empty clone, and zero extra checks on fresh clones. Local median validation is about 11 ms with either 100 or 10,000 tracked files. The repair eliminates the second read/timeout after a first transport failure and introduces no polling, network fetch, tree scan, or ref fan-out. This is a small necessary validation cost; live SSH/WSL latency was not measured locally.
Focused tests, real Git 2.25 fixtures, typecheck and changed-code quality pass. Fresh full PR Checks CI on this exact head is green and all review threads are resolved. Approving the repaired change; this review does not merge the PR.

ELI5
Orca remembers every project you've added. When you cloned a repo into a folder where Orca already had a saved project, it assumed the repo was still there and said "Repository cloned" without running git. If the folder had been deleted or moved, you got a success message and nothing on disk. Now Orca asks git whether that folder really holds the repo you asked for. If it does, Orca opens it. If the folder is gone and the saved project was that same repo, Orca clones it again. Otherwise it tells you which project is in the way instead of pretending it worked.
What Changed
Before: Clone from URL returned any saved project whose path matched the destination, straight away. Git never ran, and nothing looked at the folder. The dialog said "Repository cloned" instantly and the folder could be empty or missing. It looked intermittent because it only happened when a saved project pointed at that folder, and picking another destination "fixed" it. The desktop local clone also matched SSH projects that happened to share the path string, so a local clone could hand back a project on another machine.
After: When a saved project sits at the clone path on the machine doing the clone, one shared decision runs for the desktop local clone, the desktop SSH clone and the runtime clone (
orca project setup-clone,repo.clone):The folder is already a clone of that repo: Orca returns the saved project. Git, on the machine that holds the folder, has to show three things:
rev-parse --show-cdupprints an empty line);HEADis one git finished writing: either it resolves (--verify --quiet HEAD), or — for a clone of a repository that has no commits yet — it is an unborn branch thatsymbolic-ref HEADresolves and whose exact branch has theorigintracking settings Git writes for a completed empty clone. Git 2.25 can leave a normal unborn branch during an interrupted fetch; that branch has no completed-clone tracking settings and is refused;originhas exactly one URL, and it names the requested repo. Both URLs go throughnormalizeGitRemoteUrl, so.git, https vsgit@and the username don't matter; local paths must match exactly.Repeat clones, a double-clicked Clone and re-running
orca project setup-clonestill succeed this way, including for a brand-new repository with no commits.Git can't show that, but the saved project was this repo: Orca decides this from the project's stored
originidentity. Git clones as usual: into a missing or empty folder it clones, and a non-empty folder it refuses. The project keeps its settings.Anything else: the clone stops with an error naming the saved project, and git never runs. This covers a different repo with the same folder name, a fork, or a project Orca has no record for. The error says the project may hold a different repository or have been moved or deleted, and to remove it from Orca or choose another folder.
Also:
runtime:projects without an SSH connection are registered in this process and stay candidates.getRepoSshConnectionId), so a project stored with onlyexecutionHostId: ssh:*is found too.addRemoteRepoFromPathreturns an existing project before it gets that far, so without this the relay never learns the path came back.How it works:
reuseSavedCloneTarget(new,src/main/git/saved-clone-target.ts) makes the decision. It runs its git commands throughrunGitProbeOnHost, which is extracted fromprobeGitRemoteIdentityso it reuses that function's host routing (local runner, WSL by path, SSH relay) and timeouts. Every command it runs (rev-parse,symbolic-ref,config --get-all) is on the SSH relay's read-only allowlist and has been since SSH support shipped (#590), so older relays work. A probe also stops issuing commands as soon as Cancel fires.Why
The first version of this PR kept the premise "a saved project at this path means the clone happened" and made it stricter with filesystem checks. As the review pointed out, that still never compared the folder's repo with the one requested, and most of those checks guarded against cases git already refuses. This version checks what "Repository cloned" promises: the requested repo is at that folder. Every other outcome either lets git decide or stops loudly.
Alternatives considered:
orca project setup-cloneall rely on a repeat clone succeeding.upstreamand the icon still leaves the old repo's other settings (base branch, hooks, GitHub account) on a different repo. Adding a second project at the same path duplicates it. Deciding before git whether the saved project was this repo avoids both.probeGitRemoteIdentity's identity. It prefers theupstreamremote, so a fork would match its parent's URL. The check readsorigindirectly.Linked Issue
Fixes #23563
Visual Proof
N/A. Nothing is redesigned: this changes the mechanism behind an existing dialog, meaning when a clone reuses a saved project instead of running git. The only visible difference is which existing outcome you get: a real clone, the existing project, or an error shown in the clone dialog's existing error line. The behaviour is covered by the tests below, and by a hidden-app run described under Testing.
Testing
Additional review repairs: stored credentials never appear in the refusal error; a quiet missing-HEAD exit preserves localized-host compatibility; timeouts and transport failures do not start another probe; local clone matching excludes runtime-host rows as well as SSH rows. Focused clone/path/provider/relay tests and runtime clone tests pass, as do official node types and the changed-code gate.
Regression tests at each boundary. Each fails on
mainand passes here (checked by reverting the source files):repos:clone: a stale project for this repo re-clones; a different repo is refused, naming the project; a saved SSH project at the same path doesn't block a local clone; Cancel during the check stops without cloning;repos:cloneRemote: refuses a saved SSH project whose folder isn't this clone, and reuses a finished clone saved with onlyexecutionHostId;Real-git unit tests for the check in
saved-clone-target.test.ts:HEADplus tracking settings), and a checkout left on a detachedHEAD;Updated tests: Fix clone abort cleanup ownership #2981's queued-retry and serialize tests (local and runtime) and Make remote hosts first class: concurrent multi-host workbench #5071's SSH repeat-clone test. The second request now has to pass the check against a finished clone.
Hidden-app run, before and after:
octocat/Hello-World, delete the folder outside Orca, clonesomeone-else/Hello-World, then the original URL again.mainthe third clone reports "Repository cloned" with the folder still missing.Platforms: macOS with real Git 2.55 and a locally built Git 2.25. A real Git 2.25 fetch was paused at its pack response and its isolated process tree killed: the original helper reused the incomplete folder, while the repaired helper refuses it; the real completed empty clone remains reusable. Thirty saved-clone tests pass with Git 2.25. SSH execution-host/provider and relay cancellation paths are covered by mocks; no live SSH, WSL or Windows host was used.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
Review
Not covered, on purpose:
HEADandoriginare written, passes the check. On SSH the relay doesn't clean up a cancelled or failed clone, so this can follow a Cancel there.insteadOf), and a hand-madeoriginthat was never fetched can also pass.normalizeGitRemoteUrlignores ports, and treatsgit@host:xandssh://host/xas the same. Two git servers on one hostname could be mistaken for each other.originidentity, which includes forks. For the last one the project has to be removed from Orca (or another folder chosen) before the URL can be cloned there again. The error distinguishes a recorded different repository from an unknown repository, without displaying the stored remote URL, which may contain credentials.originis the only stored remote that can name a project's own repo, so there is no second field to fall back on:Repo.upstreamis documented as the parent of a fork, and the GitHub avatarrepoIconthatgetProjectProviderIdentityfalls back to is derived from that same upstream. Storing an origin-sourced key alongside the existing one would shrink this case to nothing, and is the follow-up worth doing.Out of scope (follow-ups):
mainthe same race returns the stale project with nothing on disk.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
repo.clonecan now return an error where it returned a stale project, and older clients show that error.src/sharedchanged.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred). Focused gates only locally, see Testing; CI covers the rest.