Skip to content

wsl: prefer Linux-side git/gh over Windows interop binaries#10137

Open
JonRC wants to merge 3 commits intowarpdotdev:masterfrom
JonRC:jonrc/wsl-prefer-linux-git
Open

wsl: prefer Linux-side git/gh over Windows interop binaries#10137
JonRC wants to merge 3 commits intowarpdotdev:masterfrom
JonRC:jonrc/wsl-prefer-linux-git

Conversation

@JonRC
Copy link
Copy Markdown

@JonRC JonRC commented May 5, 2026

Description

When Warp runs as a Linux ELF inside WSL on Windows, WSL's default appendWindowsPath = true (in /etc/wsl.conf) puts directories like /mnt/c/Program Files/Git/cmd/ on PATH. That makes a bare Command::new("git") (or "gh") potentially resolve to the Windows .exe through WSL interop. The interop call is dramatically slower (cross-OS), can mishandle Linux paths, and breaks Linux-side hooks (e.g. LFS). Affected features include the GitDiffStats context chip, code-review per-file diffs, branch detection, file-search status, AI skill resolution, and passive suggestions.

This PR adds a small warp_util::wsl module with three cached helpers:

  • is_wsl() — the existing /proc/sys/fs/binfmt_misc/WSLInterop probe, deduplicated from two prior copies in crates/warpui/src/platform/linux/mod.rs and app/src/crash_reporting/linux.rs.
  • git_binary() / gh_binary() — return the literal program name on non-WSL, and on WSL walk PATH while skipping any entry under /mnt/, returning the first Linux-side executable match. If nothing is found on a filtered PATH the helper logs once and falls back to the literal name (so behavior is no worse than before).

Every Warp-internal Command::new("git"|"gh") site is rewired through the helpers, including the test/debug helpers under integration_testing/, the snapshot tests, and crates/integration/src/test/code_review.rs. The /mnt/* filter mirrors the existing precedent at app/src/terminal/model/session/command_executor/wsl_command_executor.rs:60-65 used for compgen.

User-typed shell git invocations are unchanged — those remain governed by the user's shell PATH.

Linked Issue

Tracks #8410. The issue is currently labeled enhancement without ready-to-spec / ready-to-implement. The change here is a behavior fix for an existing feature rather than new functionality, so I'd appreciate maintainers / Oz triaging it as a bug fix; happy to file a separate bug report and re-link if a fresh issue is preferred.

Other open WSL issues likely fixed by this PR

These reports describe symptoms of the same root cause (Warp-internal git falling through to git.exe via WSL interop). Maintainers may want to verify and close them once this lands:

  • #7909 — Git repository detection not working on WSL (bug, ready-to-implement): the "git changes" button fails to surface diffs because the underlying git diff invocation goes through git.exe.

  • #6645 — Git Repositories not detected for @ attachment in WSL (bug, ready-to-implement): the @ menu stays disabled because git ls-files (now rewired through git_binary() in app/src/ai/agent_sdk/driver/snapshot.rs and app/src/ai/skills/resolve_skill_spec.rs) fails under WSL interop.

  • The linked issue is labeled ready-to-spec or ready-to-implement.

  • No UI changes; screenshots/videos N/A.

Screenshots / Videos

Not applicable — change is entirely in subprocess invocation plumbing.

Testing

  • New unit tests in crates/warp_util/src/wsl_tests.rs cover the pure resolver resolve_binary_in_wsl_safe_path across 8 cases: WSL prefers Linux-side dirs over /mnt/...; non-WSL passes through; only-/mnt falls back to None; user-local bin is found via PATH walk; symlinks resolve; non-executables are skipped; PATH=None is safe; non-UTF-8 PATH bytes don't panic.
  • Locally verified: cargo fmt --all -- --check, cargo check and cargo clippy --all-targets --tests -- -D warnings for warp_util, warpui, warp, and integration; full cargo test -p warp_util (66 tests + 3 doc tests, all green). Full-workspace clippy/nextest weren't run on my machine because of an unrelated missing system dep (libasound2-dev).
  • Manual end-to-end check: in WSL, build and run inside a git repo with both /usr/bin/git and Windows git on PATH; the GitDiffStats chip and code-review diff view continue to work, and the [GIT OPERATION] debug logs no longer warn.

Agent Mode

  • Warp Agent Mode - This PR was created via Warp's AI Agent Mode

When Warp runs as a Linux ELF inside WSL, WSL's default
appendWindowsPath=true puts directories like
/mnt/c/Program Files/Git/cmd/ on PATH. That makes a bare
Command::new("git") resolve to Windows git.exe through interop,
which is dramatically slower (cross-OS calls), can mishandle
Linux paths, and can break Linux-side hooks (e.g. LFS).

Add warp_util::wsl with cached is_wsl(), git_binary(), and
gh_binary() helpers. On WSL, the helpers walk PATH, skipping any
entry under /mnt/, and return the first executable Linux-side
match; everywhere else they return the literal program name. The
/mnt/* filter mirrors the existing precedent in
wsl_command_executor.rs:60-65 used for compgen.

Rewire every Warp-internal git/gh subprocess invocation through
the helpers, including production callers (diff stats, code
review, branch detection, file search, AI skill resolution,
passive suggestions) and the test/debug helpers, and dedupe two
existing /proc/sys/fs/binfmt_misc/WSLInterop checks while we're
here.

Tracks warpdotdev#8410.
@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented May 5, 2026

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JonRC on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment @cla-bot check to trigger another check.

@github-actions github-actions Bot added the external-contributor Indicates that a PR has been opened by someone outside the Warp team. label May 5, 2026
@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 5, 2026

@JonRC

I'm starting a first review of this pull request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss[bot]
oz-for-oss Bot previously requested changes May 5, 2026
Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR centralizes WSL detection in warp_util and routes Warp-internal git/gh subprocess creation through helpers that prefer Linux-side binaries under WSL.

Concerns

  • The new helper also resolves and caches an absolute binary path on non-WSL hosts, which conflicts with the stated literal-name fallback and can bypass existing call-site PATH overrides.

Verdict

Found: 0 critical, 1 important, 0 suggestions

Request changes

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Comment thread crates/warp_util/src/wsl.rs Outdated
Per Oz review feedback (warpdotdev#10137): the helper resolved and cached an
absolute path to git/gh on every host, not just on WSL. That froze
the binary path at module init and silently ignored call-site
`cmd.env("PATH", ...)` overrides used to expose user-installed
hooks (e.g. `run_git_command_with_env` for LFS `git-lfs`,
`run_gh_command` for Homebrew gh on macOS).

Return the bare program name immediately when `is_wsl()` is false
so `Command::new` performs its normal PATH lookup at spawn time,
matching the original contract advertised in the module docs.
@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented May 5, 2026

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JonRC on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment @cla-bot check to trigger another check.

@JonRC
Copy link
Copy Markdown
Author

JonRC commented May 5, 2026

Thanks for the review! Addressed in fce8b74:

resolve_or_warn now short-circuits when is_wsl() is false and returns the bare program name, so Command::new performs its normal PATH lookup at spawn time — restoring the per-call cmd.env("PATH", …) override behavior that callers like run_git_command_with_env (LFS hooks) and run_gh_command (Homebrew gh on macOS) depend on. Module docs updated accordingly. The pure resolver and existing tests are unchanged; the WSL-only path is exercised explicitly with is_wsl=true.

/oz-review

@oz-for-oss
Copy link
Copy Markdown
Contributor

oz-for-oss Bot commented May 5, 2026

@JonRC

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I reviewed this pull request and requested human review from: @vorporeal.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

@JonRC
Copy link
Copy Markdown
Author

JonRC commented May 5, 2026

@cla-bot check

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented May 5, 2026

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JonRC on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment @cla-bot check to trigger another check.

@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented May 5, 2026

The cla-bot has been summoned, and re-checked this pull request!

@JonRC
Copy link
Copy Markdown
Author

JonRC commented May 5, 2026

@cla-bot check

@cla-bot cla-bot Bot added the cla-signed label May 5, 2026
@cla-bot
Copy link
Copy Markdown

cla-bot Bot commented May 5, 2026

The cla-bot has been summoned, and re-checked this pull request!

@oz-for-oss oz-for-oss Bot dismissed their stale review May 5, 2026 08:29

Oz no longer requests changes for this pull request after the latest automated review.

Copy link
Copy Markdown
Contributor

@oz-for-oss oz-for-oss Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR centralizes WSL detection in warp_util::wsl, adds cached WSL-safe git/gh binary resolution, and rewires Warp-internal subprocess invocations to avoid Windows interop binaries when a Linux-side executable is available. The added resolver tests cover the main PATH filtering, fallback, symlink, executability, and non-UTF-8 PATH cases.

Concerns

No blocking correctness or security concerns found in the annotated diff.

Verdict

Found: 0 critical, 0 important, 0 suggestions

Approve

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

@oz-for-oss oz-for-oss Bot requested a review from vorporeal May 5, 2026 08:29
Copy link
Copy Markdown
Contributor

@vorporeal vorporeal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR! some thoughts:

at a high level, i'm concerned that "leaking" this detail to invokers of Command will make it too easy for someone to fail to use this at a future call site.

if we want to always be doing this additional logic when using Command and BlockingCommand, should this be an implementation detail of those wrappers? i.e.: we still call Command::new("git") but internally, it uses your helper functions to perform the translation, when needed.

Comment thread crates/warpui/src/platform/linux/mod.rs Outdated
IS_WSL
.get_or_init(|| std::path::Path::new("/proc/sys/fs/binfmt_misc/WSLInterop").exists())
.to_owned()
warp_util::wsl::is_wsl()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this dependency edge is in the wrong direction - the UI framework shouldn't be dependent on "app code". (in general, warp_-prefixed crates should all sit above the UI framework. also, as time goes on, i'm not loving the warp_util grab-bag crate, so i'm generally trying to avoid putting new things in there.)

given warpui already depends on command (on non-macOS platforms), if you agree that we should move the git/gh resolution logic into the command crate, i'd say we also move the is_wsl() function in there. (it's not my favorite home for that function, but i also don't necessarily feel like making a wsl crate just to hold is_wsl() is necessary.)

@JonRC
Copy link
Copy Markdown
Author

JonRC commented May 5, 2026

Thanks @vorporeal! Both points make a lot of sense — the "every new call site has to remember the helper" risk is real, and the dep direction critique on warpui → warp_util is fair. I'll move the WSL detection and the git/gh resolution into the command crate so Command::new("git") / BlockingCommand::new("git") transparently do the right thing, drop the new module from warp_util entirely, and have warpui::platform::linux::is_wsl() delegate into command. Will push and ping you once it's done.

Per @vorporeal review (warpdotdev#10137):

- Folding the helper into `command` so `Command::new("git")` /
  `BlockingCommand::new("git")` transparently substitute the
  Linux-side binary on WSL — no per-callsite remembering.
- The dependency edge `warpui -> warp_util` was wrong (warp_*
  crates should sit above the UI framework). `command` is already a
  warpui dep on non-macOS, and a much better home for both `is_wsl`
  and the resolution logic.

Add `command::wsl` with cached `is_wsl()`, the pure
`resolve_binary_in_wsl_safe_path` resolver, and an internal
`translate_program_for_spawn` invoked from the wrappers' `new`,
`new_with_session`, and `new_with_process_group` constructors. Only
bare-name programs in `KNOWN_NAMES = ["git", "gh"]` are touched —
path-qualified or unknown programs pass through unchanged.

All previous `warp_util::wsl::git_binary()` / `gh_binary()` call
sites revert to bare `Command::new("git" | "gh")`, and the
`warp_util::wsl` module + its `warpui` / `integration` Cargo edges
are removed. `warpui::platform::linux::is_wsl` and
`app::crash_reporting::linux` now delegate to `command::wsl::is_wsl`.

11 unit tests cover the resolver and the bare-name detector.
@JonRC
Copy link
Copy Markdown
Author

JonRC commented May 5, 2026

@vorporeal — addressed in a811077:

  • The WSL detection + git/gh resolution now live in crates/command/src/wsl.rs. command::r#async::Command::new / command::blocking::Command::new (plus new_with_session / new_with_process_group) call an internal translate_program_for_spawn so call sites continue to write Command::new("git") and the substitution happens automatically — no per-callsite remembering.
  • Only bare-name programs in KNOWN_NAMES = ["git", "gh"] are touched; path-qualified or unknown programs pass through unchanged, so any caller that already specified /usr/bin/git is preserved.
  • The new warp_util::wsl module is gone — every previous warp_util::wsl::git_binary() / gh_binary() callsite reverted to bare Command::new("git" | "gh"). The warpui → warp_util and integration → warp_util Cargo edges are gone with it.
  • warpui::platform::linux::is_wsl() and app::crash_reporting::linux now delegate to command::wsl::is_wsl(). Dep direction is warpui → command (which already existed on non-macOS) and app → command — no warp_* crate depending on warp_util for this.
  • 11 unit tests live alongside the new module: 8 cover the pure resolver, 3 cover the bare-name detector (recognizing git/gh, skipping paths, skipping unknowns).

Local checks (touched crates): cargo fmt --check, cargo clippy --all-targets --tests -- -D warnings, and cargo test for command, warp_util, warpui, warp, and integration all green.

@JonRC JonRC requested a review from vorporeal May 5, 2026 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed external-contributor Indicates that a PR has been opened by someone outside the Warp team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants