Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe WSL login-shell command builder now supports startup-stdio isolation and exposes the outer-shell arguments. Git WSL execution enables isolation for Git commands. Tests cover payload construction, descriptor handling, binary output, process and stream errors, exit statuses, missing resources, and native, generic WSL, and SSH routing. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🧹 Nitpick comments (5)
src/shared/wsl-login-shell-command.ts (1)
26-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a one-line rationale for the descriptor parking.
The descriptor sequence is correct. The outer shell parks caller stdin, stdout, and stderr on 3, 4, and 5, points startup stdio at
/dev/null, and the inner script restores them before the payload runs. That ordering is what suppresses inner.bashrcbanners, because rc files execute before the script body.The mechanism is non-obvious. A single
// Why:line matches the existing comment style in this file and prevents a later refactor from moving the restore lines out of the inner script.♻️ Proposed comment
+ // Why: rc banners run before the inner script body, so restoring fds inside the body drops startup output only (issue `#10917`). const quotedCommand = quotePosixShell(Source: Coding guidelines
src/shared/wsl-login-shell-command.test.ts (1)
97-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip this test explicitly on Windows instead of returning on ENOENT.
On Windows there is no
shon PATH, soexecFileSyncthrowsENOENTand line 123 returns. The test then reports as passed while it verified nothing. An explicit skip states the platform contract and keeps the pass signal honest.The success path also throws inside
try, and the samecatchhandles it. That produces the message "POSIX shell integration failed: expected the payload to exit with status 7", which is misleading. Move the "expected status 7" failure out of thetryblock or assert on a captured result.♻️ Proposed change
- it('isolates startup output and restores caller stdin in a headless POSIX shell', () => { + it.skipIf(process.platform === 'win32')( + 'isolates startup output and restores caller stdin in a headless POSIX shell', + () => {Based on learnings: tests that depend on platform-specific capabilities should be guarded with
it.skipIf(process.platform === 'win32')so the assertions are not silently skipped.Source: Learnings
src/main/git/runner.ts (1)
234-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the repeated
command === 'git'check.Line 223 already tests
command === 'git'to selectlocalePrefix. Line 234 repeats the same literal. A singleconst isGit = command === 'git'above line 223 keeps the two Git-specific behaviors tied to one predicate.♻️ Proposed change
- const isolateStartupStdio = command === 'git' + const isolateStartupStdio = isGitsrc/main/git/runner-wsl-login-shell-stdio.test.ts (2)
157-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a fresh child per scenario in this test.
This test covers four scenarios and reuses the single
childemitter created at line 159. The listeners registered at lines 180 through 189 stay attached when line 208 and line 217 emit'error'on the same emitter. The scenarios pass today, but a later change to listener ordering can produce cross-talk that is hard to diagnose.Create a new
fakeChild()inside eachspawnMock/execFileMockimplementation, or split the four scenarios into separateitblocks. Separate blocks also give precise failure names for spawned channels, launch errors, execFile child errors, and spawn child errors.
285-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
as neverwith an explicit intent cast.
as neverdisables all type checking on the options object and hides which required field is omitted. The test omitscwdon purpose. State that with a cast throughunknown, or build the object with an explicit partial type.♻️ Proposed change
- await expect(gitExecFileAsync(['status'], { wslDistro: 'Ubuntu' } as never)).rejects.toBe( - missingGit - ) + await expect( + gitExecFileAsync(['status'], { wslDistro: 'Ubuntu' } as unknown as GitExecOptions) + ).rejects.toBe(missingGit)Import
GitExecOptionsas a type from./runnerif it is exported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: abc17466-162e-4aed-b97a-c036dcde686d
📒 Files selected for processing (5)
src/main/git/runner-command-exec.test.tssrc/main/git/runner-wsl-login-shell-stdio.test.tssrc/main/git/runner.tssrc/shared/wsl-login-shell-command.test.tssrc/shared/wsl-login-shell-command.ts
Greptile SummaryThis PR fixes issue #10917 by isolating the WSL login-shell startup phase from Git's stdio channels. When a Git command is routed through WSL, a new
Confidence Score: 4/5Safe to merge after fixing the one broken test assertion; the core stdio-isolation logic is correct and well-tested by the new file. The FD-parking/restoration logic in wsl-login-shell-command.ts, the isolateStartupStdio opt-in in runner.ts, and the full new test suite are all internally consistent and correct. One assertion in runner-command-exec.test.ts was changed to expect -c but the implementation produces -lc, so that test will fail when the file is executed. Everything else in the changeset is clean. Files Needing Attention: src/main/git/runner-command-exec.test.ts — the outer-flag assertion at line 490 needs to be reverted to -lc.
|
| Filename | Overview |
|---|---|
| src/shared/wsl-login-shell-command.ts | Adds getWslLoginShellOuterArgs() and optional isolateStartupStdio mode; parks caller FDs 3/4/5, redirects startup stdio to /dev/null, and restores them in the inner-shell payload. |
| src/main/git/runner.ts | Wires isolateStartupStdio = command === 'git' and getWslLoginShellOuterArgs() into resolveCommand; non-git WSL login-shell callers are unchanged. |
| src/main/git/runner-wsl-login-shell-stdio.test.ts | New test file covering banner isolation, exact wsl.exe argv (expects outer -lc), binary output, spawn channels/errors, streaming, and negative-space routing. |
| src/main/git/runner-command-exec.test.ts | One assertion changed from -lc to -c contradicts the implementation and the new test file which both expect -lc; another added assertion correctly checks -lc and no-isolation. |
| src/shared/wsl-login-shell-command.test.ts | Adds unit tests for isolateStartupStdio: FD ordering, null-isolation, and a POSIX integration test confirming startup output is suppressed and stdin is restored. |
Sequence Diagram
sequenceDiagram
participant Node as Node.js gitExecFileAsync
participant wsl as wsl.exe sh -lc
participant outer as Outer sh startup
participant inner as Inner bash -ilc payload
participant git as git
Node->>wsl: spawn with stdin/stdout/stderr pipes
wsl->>outer: "park FDs 3=stdin 4=stdout 5=stderr"
Note over outer: redirect all stdio to devnull
Note over outer: login scripts and banners go to devnull
outer->>outer: detect shell via getent passwd
outer->>inner: exec bash -ilc with restore commands
Note over inner: restore original pipes from FD 3/4/5
inner->>git: run with restored stdio
git-->>Node: clean stdout with no banner prefix
Reviews (3): Last reviewed commit: "Prevent banner output from corrupting WS..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Closing this as superseded by PR #15257, which merged the same WSL Git read fix into |
|
Thanks @rodboev for all the changes generally... Actually trying to fix all the WSL this week. |
|
Thanks, just getting back to these. I use a patched build so I'll have to see what's still live on current main and I'll ping you when I have the list. |
Thanks! |
Summary
The shared WSL login-shell builder has an opt-in descriptor phase.
resolveCommandselects it only for local WSL Git; default helper callers retain their generated command.Base/head proof used
origin/main8fc892dd023850df379c07cc4e7a46cff9a3ea84: the extracted base failed the public issue banner identity assertion withnull; the rebuilt head passed with{ owner: 'stablyai', repo: 'orca' }.Closes #10917.
Screenshots
No visual change
Testing
pnpm lintpnpm typecheckpnpm testpnpm buildFocused validation:
pnpm run typecheck:node, changed-fileoxlint, changed-fileoxfmt --check,pnpm run check:max-lines-ratchet, andgit diff --checkpassed.AI Review Report
The final diff was reviewed for Windows-to-WSL argv construction, login-shell branches, caller stdin, text and binary channels, Git status, shell-owned setup behavior, synchronous and event-based launch errors, and native, generic WSL, non-Git login-shell, SSH-provider, and default-helper negative space. Native Git remains direct binary-plus-argv execution on macOS and Linux. No UI or rendering path changed.
Security Audit
Descriptor commands are fixed shell text. Existing POSIX quoting and Windows-to-WSL dollar escaping remain in place, and descriptors 3, 4, and 5 close before the Git payload. No marker parsing, output file, process, dependency, credential, network, IPC, persistence, or user-facing copy surface was added.
Notes
gh,glab, SSH-provider execution, and default shared-helper output remain unchanged.docker-desktop, WSL2. Real user-distro runtime proof was not run because no representative distro is available.