Repository navigation
fix(git): run WSL git reads without a shell - #15257
Conversation
d79a008 to
bdc65a0
Compare
WSL-routed git ran through the distro user's interactive login shell for one reason: to inherit their PATH. That shell also runs the distro's rc/motd and writes it to the stdout callers parse, which is why #10917 reports a shell banner breaking GitHub source detection. A shell-free route already existed (`--exec /usr/bin/env PATH=... git`, added for status reads in #13207) but it was opt-in, and only gitStatusReadOptionsForWorktree opted in. Every other read -- remote get-url, config --get, log, show, rev-parse -- took the login shell on every call, so the reported parse never benefited. Classify reads at the resolver instead of at each caller. A read needs nothing the login shell provides, so it takes the direct route without the caller asking. Writes and network operations stay on the login shell: they can depend on credential helpers and ssh-agent that the user's profile sets up. Subcommands that both read and write (config, remote, branch, submodule) require an explicit read flag before they qualify, so `config --get` goes direct while `config user.email x` does not. `git show` blob reads stop forcing the login shell and go direct too. The fence stays on that path: the login shell is still the fallback when the environment probe is cold or rejected, and a banner there would become file content. Behavior note: the first WSL read per distro now also warms the environment probe in the background, so a cold read spawns one extra wsl.exe. Subsequent reads start no shell at all.
The conditional read markers were matched anywhere after the subcommand, so a positional argument that happened to share a marker's name routed a write shell-free: `worktree remove list` read as a listing, and `submodule foreach status` as a status query -- the latter can run arbitrary commands, including network ones. Split the two kinds of marker apart. `config`/`branch` are flag-marked and still match anywhere; `remote`/`worktree`/`submodule` are action-marked and must match the first non-flag argument. The queried `remote show` rule and the `symbolic-ref` arity rule are unchanged. Neither case is reachable today -- Orca issues no `submodule foreach`, and the worktree paths are its own CLI, not git argv -- but the loose match was the shape of the defect, not those two instances.
5ceb5c1 to
4cdf9a9
Compare
Writes hidden behind -c/-C/--git-dir must not reach the shell-free route, and an unparsed global form must fall back to the login shell rather than guess at the subcommand. Both directions fail safe.
4cdf9a9 to
c401997
Compare
📝 WalkthroughWalkthroughThe change adds classification for shell-free WSL Git read commands, including global options and supported listing forms. It routes eligible reads through direct Git execution while retaining login-shell handling for writes, network-sensitive commands, unsupported forms, and fallback retries. New tests cover command classification and WSL routing. Existing tests now account for background environment probes, Git call ordering, and explicit undefined askpass variables. Merge Risk: 🟡 Moderate · up to This change reroutes WSL Git reads directly, but cold-cache and direct-read failure paths can still expose login-shell output as Git data, while git-show failures may not fall back to the shell and can fail unnecessarily. The test suite also leaves a mocked platform value behind between tests, so the PR is not merge-ready until these bounded issues are fixed. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/git/runner.ts (2)
450-463: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFence login-shell fallbacks for direct-safe reads.
On a cold environment cache, Line 452 returns no environment. Line 460 starts the probe without waiting. Line 463 then runs the read through an unfenced login shell.
Shell startup output can therefore still prefix
remote get-urloutput on the first request. The retry at Line 1119 has the same problem after a direct-Git failure.src/main/git/runner-wsl-read-routing.test.tsseeds the cache, so it does not cover this path.Use
captureLoginShellOutputfor these login-shell fallbacks. Strip the captured markers before returningstdout. Add cold-cache and direct-failure routing tests.Based on learnings: retry any direct-Git failure through the login shell and disable the cached direct environment only after the retry succeeds.
Also applies to: 1117-1123
Source: Learnings
1191-1206: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRetry direct buffer reads through the login shell.
If direct
git showfails, this function rejects immediately. It does not retry withresolveGitCommand(args, options, true, true).Profile-dependent Git configuration can require the login shell. Keep the existing captured-output handling, but retry direct-Git failures through the fenced login shell. Disable the cached direct environment only after that retry succeeds. Add a regression test for this path.
Based on learnings: retry any direct-Git failure through the login shell and disable the cached direct environment only after the retry succeeds.
Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 642585da-9c54-4418-b62b-a2f787176dce
📒 Files selected for processing (6)
src/main/git/runner-command-exec.test.tssrc/main/git/runner-wsl-linked-gitdir-timeout.test.tssrc/main/git/runner-wsl-read-routing.test.tssrc/main/git/runner.tssrc/main/git/wsl-direct-git-read-commands.test.tssrc/main/git/wsl-direct-git-read-commands.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 3 remain after this review.
| Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| Object.defineProperty(process, 'platform', { configurable: true, value: process.platform }) | ||
| resetWslGitReadEnvironmentForTests() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the original platform value.
Line 61 reads process.platform after Line 57 changes it to win32. The suite therefore leaves the mocked platform active after every test.
Store the original value before the suite starts. Restore that value in afterEach.
Proposed fix
+const originalPlatform = process.platform
+
describe('WSL git read routing', () => {
@@
afterEach(() => {
- Object.defineProperty(process, 'platform', { configurable: true, value: process.platform })
+ Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform })
resetWslGitReadEnvironmentForTests()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) | |
| }) | |
| afterEach(() => { | |
| Object.defineProperty(process, 'platform', { configurable: true, value: process.platform }) | |
| resetWslGitReadEnvironmentForTests() | |
| const originalPlatform = process.platform | |
| describe('WSL git read routing', () => { | |
| // ... | |
| afterEach(() => { | |
| Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) | |
| resetWslGitReadEnvironmentForTests() | |
| }) | |
| }) |
* fix(git): run WSL git reads without a shell WSL-routed git ran through the distro user's interactive login shell for one reason: to inherit their PATH. That shell also runs the distro's rc/motd and writes it to the stdout callers parse, which is why stablyai#10917 reports a shell banner breaking GitHub source detection. A shell-free route already existed (`--exec /usr/bin/env PATH=... git`, added for status reads in stablyai#13207) but it was opt-in, and only gitStatusReadOptionsForWorktree opted in. Every other read -- remote get-url, config --get, log, show, rev-parse -- took the login shell on every call, so the reported parse never benefited. Classify reads at the resolver instead of at each caller. A read needs nothing the login shell provides, so it takes the direct route without the caller asking. Writes and network operations stay on the login shell: they can depend on credential helpers and ssh-agent that the user's profile sets up. Subcommands that both read and write (config, remote, branch, submodule) require an explicit read flag before they qualify, so `config --get` goes direct while `config user.email x` does not. `git show` blob reads stop forcing the login shell and go direct too. The fence stays on that path: the login shell is still the fallback when the environment probe is cold or rejected, and a banner there would become file content. Behavior note: the first WSL read per distro now also warms the environment probe in the background, so a cold read spawns one extra wsl.exe. Subsequent reads start no shell at all. * fix(git): keep queried WSL remote reads on login shell * style: format WSL runner test * fix(git): match WSL read markers positionally The conditional read markers were matched anywhere after the subcommand, so a positional argument that happened to share a marker's name routed a write shell-free: `worktree remove list` read as a listing, and `submodule foreach status` as a status query -- the latter can run arbitrary commands, including network ones. Split the two kinds of marker apart. `config`/`branch` are flag-marked and still match anywhere; `remote`/`worktree`/`submodule` are action-marked and must match the first non-flag argument. The queried `remote show` rule and the `symbolic-ref` arity rule are unchanged. Neither case is reachable today -- Orca issues no `submodule foreach`, and the worktree paths are its own CLI, not git argv -- but the loose match was the shape of the defect, not those two instances. * test(git): pin read routing behind global options Writes hidden behind -c/-C/--git-dir must not reach the shell-free route, and an unparsed global form must fall back to the login shell rather than guess at the subcommand. Both directions fail safe. --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
* fix(git): run WSL git reads without a shell WSL-routed git ran through the distro user's interactive login shell for one reason: to inherit their PATH. That shell also runs the distro's rc/motd and writes it to the stdout callers parse, which is why stablyai#10917 reports a shell banner breaking GitHub source detection. A shell-free route already existed (`--exec /usr/bin/env PATH=... git`, added for status reads in stablyai#13207) but it was opt-in, and only gitStatusReadOptionsForWorktree opted in. Every other read -- remote get-url, config --get, log, show, rev-parse -- took the login shell on every call, so the reported parse never benefited. Classify reads at the resolver instead of at each caller. A read needs nothing the login shell provides, so it takes the direct route without the caller asking. Writes and network operations stay on the login shell: they can depend on credential helpers and ssh-agent that the user's profile sets up. Subcommands that both read and write (config, remote, branch, submodule) require an explicit read flag before they qualify, so `config --get` goes direct while `config user.email x` does not. `git show` blob reads stop forcing the login shell and go direct too. The fence stays on that path: the login shell is still the fallback when the environment probe is cold or rejected, and a banner there would become file content. Behavior note: the first WSL read per distro now also warms the environment probe in the background, so a cold read spawns one extra wsl.exe. Subsequent reads start no shell at all. * fix(git): keep queried WSL remote reads on login shell * style: format WSL runner test * fix(git): match WSL read markers positionally The conditional read markers were matched anywhere after the subcommand, so a positional argument that happened to share a marker's name routed a write shell-free: `worktree remove list` read as a listing, and `submodule foreach status` as a status query -- the latter can run arbitrary commands, including network ones. Split the two kinds of marker apart. `config`/`branch` are flag-marked and still match anywhere; `remote`/`worktree`/`submodule` are action-marked and must match the first non-flag argument. The queried `remote show` rule and the `symbolic-ref` arity rule are unchanged. Neither case is reachable today -- Orca issues no `submodule foreach`, and the worktree paths are its own CLI, not git argv -- but the loose match was the shape of the defect, not those two instances. * test(git): pin read routing behind global options Writes hidden behind -c/-C/--git-dir must not reach the shell-free route, and an unparsed global form must fall back to the login shell rather than guess at the subcommand. Both directions fail safe. --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
* fix(git): run WSL git reads without a shell WSL-routed git ran through the distro user's interactive login shell for one reason: to inherit their PATH. That shell also runs the distro's rc/motd and writes it to the stdout callers parse, which is why stablyai#10917 reports a shell banner breaking GitHub source detection. A shell-free route already existed (`--exec /usr/bin/env PATH=... git`, added for status reads in stablyai#13207) but it was opt-in, and only gitStatusReadOptionsForWorktree opted in. Every other read -- remote get-url, config --get, log, show, rev-parse -- took the login shell on every call, so the reported parse never benefited. Classify reads at the resolver instead of at each caller. A read needs nothing the login shell provides, so it takes the direct route without the caller asking. Writes and network operations stay on the login shell: they can depend on credential helpers and ssh-agent that the user's profile sets up. Subcommands that both read and write (config, remote, branch, submodule) require an explicit read flag before they qualify, so `config --get` goes direct while `config user.email x` does not. `git show` blob reads stop forcing the login shell and go direct too. The fence stays on that path: the login shell is still the fallback when the environment probe is cold or rejected, and a banner there would become file content. Behavior note: the first WSL read per distro now also warms the environment probe in the background, so a cold read spawns one extra wsl.exe. Subsequent reads start no shell at all. * fix(git): keep queried WSL remote reads on login shell * style: format WSL runner test * fix(git): match WSL read markers positionally The conditional read markers were matched anywhere after the subcommand, so a positional argument that happened to share a marker's name routed a write shell-free: `worktree remove list` read as a listing, and `submodule foreach status` as a status query -- the latter can run arbitrary commands, including network ones. Split the two kinds of marker apart. `config`/`branch` are flag-marked and still match anywhere; `remote`/`worktree`/`submodule` are action-marked and must match the first non-flag argument. The queried `remote show` rule and the `symbolic-ref` arity rule are unchanged. Neither case is reachable today -- Orca issues no `submodule foreach`, and the worktree paths are its own CLI, not git argv -- but the loose match was the shape of the defect, not those two instances. * test(git): pin read routing behind global options Writes hidden behind -c/-C/--git-dir must not reach the shell-free route, and an unparsed global form must fall back to the login shell rather than guess at the subcommand. Both directions fail safe. --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com>
Stack
Merge bottom-up; each PR is based on the one above it.
main#15039#15040#15060ELI5
To run git inside WSL, Orca was starting the user's login shell first — the same one that greets you when you open a terminal — purely so git would be on the PATH. That shell also prints the distro's welcome banner, into the same pipe Orca reads git's answer from. So Orca would read the banner as if it were git's output, and conclude the repo had no GitHub remote.
The fix isn't to filter the banner out. It's to stop starting a shell for work that never needed one: ask the shell once what the PATH is, remember it, then run git directly.
Why the earlier PRs didn't fix #10917
#15040 and #15060 fenced the output so the banner could be sliced off. That treats the symptom. This PR removes the cause for reads.
The shell-free route already existed —
--exec /usr/bin/env PATH=… HOME=… /usr/bin/git, added for status reads in #13207 — but it was opt-in, and exactly one options-builder opted in:Everything else —
remote get-url,config --get,log,show,rev-parse— usedgitOptionsForWorktree, which never set it. So the exact parse #10917 reports (remote.ts) took the login shell on every call. Fencing it would have been a fifth patch on the same wound.What changed
Reads are classified at the resolver, not at each caller, so no call site has to remember to opt in:
Writes and network operations deliberately stay on the login shell — they can depend on credential helpers and ssh-agent that only the user's profile sets up.
Subcommands that both read and write require an explicit read flag, so
config --getgoes direct whileconfig user.email xdoes not. Same forremote get-urlvsremote add, andbranch --listvsbranch -D.git showblob reads stop forcing the login shell. The fence added in #15060 stays on that path: the login shell is still the fallback when the environment probe is cold or rejected, and a banner there would become file content. It becomes rarely-hit rather than unnecessary.Safety
The existing fallback is what makes this low-risk: a direct-git failure retries on the login shell, and a successful retry disables the direct route for that distro. A misclassified command self-heals rather than breaking.
The probe itself already refuses to engage when the environment is unusual —
gitmissing or not executable (exit 127), orXDG_CONFIG_HOME/LD_LIBRARY_PATH/ anyGIT_*set (exit 78).Testing
commit,push,fetch,pull,clone,checkout,add,worktree add,submodule update,config <set>,config --unset,remote add,remote remove,branch -D,branch <name>) is asserted to stay on the login shell.remote get-url,config --get) resolves to--exec /usr/bin/env PATH=…with nosh, no-lc, and nogetent passwdanywhere in the argv — mutation-checked: reverting the routing fails all four read cases while the three write cases still pass.origin/mainat test-name granularity acrosssrc/main/git,src/main/ipc,src/shared: zero new failures, 12 fewer thanmain.Behavior note
The first WSL read per distro now also warms the environment probe in the background, so a cold read spawns one extra
wsl.exe(once per distro per session). Subsequent reads start no shell at all. Two tests asserted the old spawn sequence and were updated to ignore the probe rather than weakened.Measured on a real Ubuntu-24.04: shell startup is not the dominant cost (
sh -c67ms vsbash -ilc70ms;wsl.exespawn dominates), so this is a correctness and robustness change, not a performance one.