Repository navigation
Conversation
📝 WalkthroughWalkthroughThe change adds shared Git execution-host classification for native and WSL targets. WSL distro detection prioritizes the working directory over a conflicting distro hint. Capability-state keys now use the shared host identity. Git command routing also uses the shared classification. Tests cover host keys, precedence, distro routing, and WSL shell modes. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/git/runner.ts (1)
417-418: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one resolver for command host selection.
Line 417 uses the shared resolver, but
resolveCommandstill selects its own WSL distro at Lines 323-325. Derive the routed distro fromgitExecutionHostForTargetthere too. RetainparseWslPathonly to obtainlinuxPath. Add a runner assertion with an Ubuntu UNC cwd and a conflicting Debian hint.As per coding guidelines, “scope capability state to the executing host and test fallbacks, caching, concurrent probes, and host isolation.”
Sources: Coding guidelines, Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23b3d96b-34ec-4b4d-9bde-70aea6e6c1b3
📒 Files selected for processing (6)
src/main/git/git-capability-state.test.tssrc/main/git/git-capability-state.tssrc/main/git/git-execution-host.test.tssrc/main/git/git-execution-host.tssrc/main/git/runner-wsl-execution-mode.test.tssrc/main/git/runner.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 2 remain after this review.
Several places derived "which host runs this git call" independently, and two disagreed on precedence. The runner routes by cwd -- a WSL UNC cwd names the distro, and resolveCommand reads it before the caller's wslDistro hint. git-capability-state preferred the hint. So a call with a cwd in one distro and a hint naming another executed in the cwd's distro while its capability results were filed under the hint's. A probe answered by distro A then served calls claiming distro B, which is the failure AGENTS.md's "scope capability state to the host that executes Git" warns about. Resolve the host once, in git-execution-host.ts, and key routing, capability state and output translation off it. Precedence follows execution: the cwd wins, and the hint applies only when the cwd cannot name one. The one route that overrides the name -- a prepared Windows-authored linked worktree running host git.exe -- is passed in as `usesHostGit` so it resolves in the same place instead of being compensated for by the caller. Two consequences beyond the keying fix: - The shell-free WSL read route no longer requires the caller to repeat what the cwd already says. It was gated on `options.wslDistro` being present, so a read with a WSL UNC cwd and no hint took `bash -c` -- and an explicit `preferWslDirectGit: true` was ignored outright on that path. - The capability key folds distro case. Windows treats `\\wsl$\ubuntu` and `\\wsl.localhost\Ubuntu` as one host, so two spellings of one repo's distro were each re-probing every capability and eating its fallback. Writes still pick their shell from the presence of `wslDistro` rather than from the host and command class. That is characterized by a test here, not fixed: flipping it decides whether every WSL write gets the user's PATH and ssh-agent, which needs its own PR and real-Windows evidence.
01010e3 to
ac3e2ba
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/git/runner.ts (1)
1222-1228: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the last opening fence for buffer output.
Login-shell output can echo the fenced command before the Git payload. Use
Buffer.lastIndexOfsogit showreturns only the blob bytes. Add a regression test for an echoed opening fence.Proposed fix
- const beginIndex = stdout.indexOf(captured.beginMarker, 0, 'utf8') + const beginIndex = stdout.lastIndexOf(captured.beginMarker, undefined, 'utf8')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ef2f0ca-fa38-4a4c-82a2-81c46608a46c
📒 Files selected for processing (7)
src/main/git/git-capability-state.test.tssrc/main/git/git-capability-state.tssrc/main/git/git-execution-host.test.tssrc/main/git/git-execution-host.tssrc/main/git/runner-wsl-direct-read.test.tssrc/main/git/runner-wsl-execution-mode.test.tssrc/main/git/runner.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/git/git-capability-state.test.ts
- src/main/git/runner-wsl-execution-mode.test.ts
- src/main/git/git-capability-state.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
ELI5
Orca can run git in three different places: on Windows directly, inside a WSL distro, or over SSH. Before running a command it has to answer "which one?", and it also remembers what each host's git can do (which flags it supports) so it doesn't re-check every time.
The problem: it answered that question in several places, and they could disagree. If a call pointed at a folder inside distro A but also carried a leftover hint saying B, git ran in A — but everything Orca learned about git got written down under B. Later calls that really were in B then trusted notes taken on a completely different machine.
This makes one function the answer to "which host?", and points the runner, the notes, and the output translation at it.
Root cause
Independent derivations of the execution host, with two different precedence orders:
runner.ts:wslDistroForCommandrunner.ts:resolveCommandwsl.exe -d, guest cwd)runner.ts:translateWslOutputPathsgit-capability-state.ts:getLocalGitExecutionHostKeyRouting follows the cwd, because
resolveCommandreadsparseWslPath(cwd)before it looks at the override. Capability state preferred the override. They agree in every case except a cwd and hint naming different distros — and there they disagree silently, which is the shape AGENTS.md calls out:A capability probed against the distro in the cwd was filed under the hinted one, so a later call genuinely on the hinted distro reused a result from a host that never ran the probe. Git versions differ per distro (
docs/reference/git-compatibility.md), so this can hand a caller a "supported" verdict for a flag its actual git does not have.Fix
git-execution-host.tsowns the derivation. Precedence follows execution: the cwd wins; the hint applies only when the cwd cannot name a distro. All four sites above now go through it.One route overrides the name of the host and cannot be decided synchronously: a prepared Windows-authored linked worktree runs native
git.exeeven under a WSL hint (usesHostGitForWslLinkedWorktree). That answer is now passed in asusesHostGitand resolved inside the owner, rather than being compensated for by the caller stripping the hint before calling in. The module docblock says explicitly that it reports the host a target names, and points at the probe that overrides it.Three behavior changes ride along, each with a mutation-checked test:
1. Capability keying for a mismatched cwd + hint. The original fix. Reverting the precedence fails exactly four tests (two key-shape, two runner-level argv) and nothing else.
2. The shell-free WSL read route no longer requires the caller to repeat what the cwd already says.
shouldAttemptWslDirectGitended in&& options.wslDistro, so a read with a WSL UNC cwd and no hint fell back tobash -c— and an explicitpreferWslDirectGit: truewas ignored outright on that path. There was a test pinning that (leaves override-less WSL UNC routing on its existing non-login shell); it pinned the defect and is now flipped, with a comment saying why. The distro was always available:wslDistroForCommand(cwd, hint)on the very next line already returned it.3. The capability key folds distro case. Windows treats
\\wsl$\ubuntuand\\wsl.localhost\Ubuntuas one host (foldWslUncPathCaseInsensitivePartsinsrc/shared/wsl-paths.tssays so). Before, the key interpolated the raw string, so two spellings of one distro got twoGitCapabilityCacheinstances, each independently probingrev-parse --path-format,worktree list -z,merge-tree --write-treeandfor-each-ref --excludeand eating the failure before falling back. That is duplicatedwsl.exespawns on exactly the path the sibling probe-storm work (#15293) is trying to quiet.GitExecutionHost.distrostays verbatim, because that is whatwsl.exe -dreceives.What this does NOT change (deliberate)
runner-wsl-execution-mode.test.tscharacterizes a remaining defect without fixing it: a write still picks its shell from whetherwslDistrowas populated, not from the host and command class.The distro is
Ubuntuin both — derived from the cwd. SowslDistrocarries no routing information here; its only effect is to flip login shell vs barebash -c. A write on the no-hint path therefore runs with no~/.profile, no user PATH, and no ssh-agent, which an authenticated push needs.The test uses a write deliberately. An earlier draft asserted this with
rev-parse, which is now an always-read command: once the WSL read-environment probe resolves, a read takes the shell-free route and thesh -lcassertion stops holding. That version only passed becausebeforeEachreset the probe cache, so it pinned a cold-start artifact. A write can never take the read route, so the assertion is cache-independent — and it is the case the rationale is actually about.Fixing it is not in scope here because flipping it decides whether every WSL write gets the user's PATH and ssh-agent. That needs its own PR and real-Windows evidence.
Relationship to #15039 / #15040 / #15060 / #15257
All of these have now merged. An earlier revision of this description described them as open and sequenced work behind them; that was written against a base from before they landed and was wrong.
6e8da1df8d93a9f40ed709b7f2e1771216efd4061dc4#15257 auto-classified reads (
options.preferWslDirectGit || isWslDirectGitReadCommand(args)) but left the trailing&& options.wslDistro, so it moved the command-class decision off the caller while leaving the host decision on the caller. That residue is what change 2 above removes. Collapsing the write-side shell choice onto (host, command class) is unblocked today — it is not waiting on anything.Testing
Rebased onto
1be6a035b9eand re-verified there:src/main/git+src/main/github: 1578 passed, 4 skipped, 0 failed (145 files).pnpm run typecheckexit 0 ·pnpm exec oxlintexit 0 ·audit:code-quality:nativeexit 0 ·audit:code-quality:type-awareexit 0 ·check:reliability-gatesOK (85 gates) ·check:max-lines-ratchetOK (188 grandfathered, no new bypasses) ·oxfmt --checkclean.src/main+src/sharedon this macOS machine: 13 failures across 10 files, none insrc/main/git. Re-running those 10 files directly narrowed it to 9 failures in 6 files; the other 4 files pass in isolation and fail only under full-suite load. Those same 9 tests fail identically, by name, on a cleanorigin/mainworktree run side by side. All are timing-sensitive (fs watchers, SFTP abort races, retention caps) and pre-existing.Mutation checks (each reverts one part of the fix and re-runs all 84
src/main/gitfiles):hint ?? cwdlets the cwd win over a hint that names a different distro,files state against the distro in the cwd, not a hint naming another one,runs a read in the cwd distro, with that distro environment, despite a conflicting hint,runs a write in the cwd distro despite a conflicting hint&& options.wslDistrotakes the shell-free route for an override-less WSL UNC cwd,gives a read the same shell-free route with or without the hint.toLowerCase()from the keykeys both UNC spellings of one distro together while passing it through verbatim,shares one cache across both UNC spellings of a distroLimitations
All verification was on macOS. There is no Windows or WSL host in this environment. Every WSL claim here is from code reading and from tests that force
process.platform = 'win32'and mockchild_process. An earlier revision of this description quoted realwsl.exemeasurements (git 2.43.0 in 82ms, asudo_rootbanner on the login-shell route, absent-distro fast-fail); those were not reproducible here and are not repeated as evidence.The mismatched cwd+hint scenario has not been observed in the field. It was confirmed reachable by reading
project-runtime-git-options.tsandlocal-project-runtime-resolution.ts, not from a crash report.Change 2 is a real behavior change on Windows that no Windows machine has run. Reads with a WSL UNC cwd and no
wslDistronow take the shell-free/usr/bin/env … gitroute instead ofbash -c, once the read-environment probe has resolved for that distro. It degrades safely by construction — a non-zero exit from the direct route re-runs throughresolveGitCommandWithoutProbeand can disable the route for that distro (invalidateMissingDirectWslGit/disableDirectWslGitAfterSuccessfulFallback) — but the first-run path is untested on real hardware.Not every derivation of the git execution host is unified, despite this PR's framing. Four sites in
src/main/gitare. At least three more survive and were deliberately left alone:src/main/runtime/orca-runtime.ts:getCanonicalFetchKeykeyswsl:${gitOptions.wslDistro} : 'local'— hint-only, same key vocabulary — and then runsgitExecFileAsynccwd-first. A UNCrepoPathwith no hint is filed underlocal. This is a live latent mis-keying of the in-flight-fetch dedup key, out of this diff.src/main/github/conflict-summary-cache.ts:getConflictSummaryGitRuntimeKeyreturnswsl:${wslDistro}ignoring cwd. Safe today only becauserepoPathis also part of the composite key and therefore pins the host.src/main/gitlab/gitlab-known-host-probe.tsis hint-only; it has no cwd, so it is defensible.translateWslOutputPathskeeps its ownwin32gate rather than relying on the owner's platform-agnostic answer. Off Windows a literal//wsl$/xdirectory is an ordinary path whose git output must not be rewritten. The gate is now expressed by passingcwd: undefinedoffwin32, so the derivation is still shared, but the platform knowledge is still duplicated at the call site.gitExecutionHostForTargetis not platform-gated. On Linux/macOS a directory literally named//wsl$/X/...keys aswsl:Xrather thanlocal. This matches the key function it replaced exactly, and is unreachable via the project-runtime hint (resolveProjectExecutionRuntimereturnshostforappPlatform !== 'win32'), but the key is still not strictly "the host that executes git" off Windows.No capability state is persisted anywhere, so no migration is needed or missing.
GitCapabilityCacheis three in-memory maps andlocalCapabilitiesByExecutionHostis a module-levelMap; a restart starts empty. Both the precedence change and the case fold therefore only affect keys within a single process run.No wire, RPC or IPC surface changed. All touched files are under
src/main/git. The relay's per-handlerGitCapabilityCache(src/relay/git-handler.ts) and the SSH-provider-keyed cache are untouched, and neither is WSL-aware, so a mixed-version client/host pair sees nothing new.Key collision was checked and is impossible for reachable shapes.
localcan never equalwsl:${distro}; a distro literally namedlocalkeys aswsl:local. Nothing routes SSH- or relay-executed git throughgetLocalGitCapabilityCache.CI needed one re-run for an unrelated flake, and hit a different one before that. On the pre-rebase head, shard
tests node 24 4/16failed insrc/main/native-chat/transcript-watch-liveness.test.ts:181— an fs-watcher timing test with no git dependency and a prior deflake commit (#14335). On the first post-rebase run, shard2/16failed insrc/main/browser/browser-cookie-import-partition.electron.test.ts:188withXIO: fatal IO error 2 ... on X server ":99"and a dbus connect failure, i.e. the Xvfb display died under the Electron fixture. Neither file imports anything fromsrc/main/git. Re-running the failed jobs turned the run green with no code change, which is what a flake looks like — but it also means these two shards have passed once each rather than repeatedly.Adversarial review
Three independent adversarial reviewers went over this branch before merge. Lenses: (a) does the new abstraction actually own what it claims to own, and what still bypasses it; (b) do the added tests discriminate — mutation-testing each assertion rather than trusting a green run; (c) mergeability, cross-platform reachability, cache-key collision/fragmentation, and whether the PR description matches the code and the current state of
main.They returned 11 blocking findings (3 major, 8 minor). All were fixed; none were rejected. The notable ones:
mergeable=CONFLICTING. It was based on a merge-base from before fix(git): fence buffered WSL login-shell reads #15060 and fix(git): run WSL git reads without a shell #15257 landed, and the import block collided. Resolving it by taking the branch's wholerunner.tssilently reverts fix(git): run WSL git reads without a shell #15257 and fails 12 tests. Fixed by rebasing ontoorigin/main, keeping both imports.shouldAttemptWslDirectGitstill ended in&& options.wslDistro, so the hint-less WSL callers the description named as the victims were still excluded by the raw optional field. One reviewer demonstrated it: same host, same argv, same distro, shell-free vsbash -cdecided purely by the presence of an optional field. Fixed (change 2), and the test that pinned the old behavior was flipped.{cwd: Ubuntu}against{cwd: Ubuntu, wslDistro: 'Ubuntu'}— matching distros — so it passed unchanged under a hint-first mutation. Replaced with an assertion that seeds distinguishable read environments for Ubuntu and Debian and checks both the-dflag and the guestgitPath/PATHfor a deliberately mismatched hint.rev-parse, which after fix(git): run WSL git reads without a shell #15257 takes the shell-free route once the probe resolves; it only passed becausebeforeEachreset the cache. Switched to a write.usesHostGitand rewriting the docblock.resolveCommandandtranslateWslOutputPathsstill derived the host themselves. Both migrated;GitExecutionHostnow carries the parsed guest path soresolveCommandcan consume it.