Repository navigation
fix(wsl): transfer selected account homes only in login tabs - #2170
openwong2kim merged 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 14 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: transferring selected WSL account homes only for login tabs.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 14 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
|
Thanks for picking up #2127 — WSLENV is the right transport, and the runtime test with spaces and an apostrophe is a nice touch. One thing we noticed while reading it, and I'd love your take on how to handle it: A couple of ways I can see:
The first feels like the smaller step to me, but you know this path better — what do you think? Happy to take the follow-up on our side either way. (Tiny aside, not blocking: |
snowyukitty
changed the title
Jorlpj
left a comment
Jorlpj
left a comment
There was a problem hiding this comment.
Thanks for narrowing this to the login tab. I checked it against today's main and on a Windows box with WSL. Nothing blocking; two notes from probes, in case they help the follow-up.
What I ran
- Merged
80e193cf6withmainat469d7607blocally: no conflicts. On the merge, the four focused unit files (wslIntegration,wslDistroSpawnArgs,PTYManager.createCancel,accountLogin) give 50 passed / 15 skipped. - The opt-in runtime file,
WMUX_TEST_WSL=1 WMUX_TEST_WSL_DISTRO=Ubuntu-24.04withvitest.runtime.config.ts, gives 6/6 on Windows 11 + Ubuntu-24.04 (WSL2).
1. The fix only changes behaviour for a Linux-native CLI (informational)
A common WSL setup puts the Windows npm global dir on the Linux PATH through interop. There, command -v codex in WSL resolves to /mnt/c/Users/<user>/AppData/Roaming/npm/codex. That npm shim falls back to node.exe, so Codex runs as a Windows process.
A Windows process started from WSL inherits the Windows environment of the wsl.exe that launched the session, whatever WSLENV says. Probe, with a fresh variable and no WSLENV entry:
$env:WMUXFRESH_X = "C:\fresh\<random>"; $env:WSLENV = ''
wsl -d Ubuntu-24.04 -- sh -c 'echo "linux=[${WMUXFRESH_X-unset}]"; cmd.exe /d /c "echo win=[%WMUXFRESH_X%]"'
linux=[unset]
win=[C:\fresh\<random>]
So with a Windows Codex reached through interop, the login tab already wrote to the selected account dir before this PR, and it still does: /pu does not pass the variable back to Win32. The change only matters when the WSL codex / claude is a Linux install. That fits the changelog wording ("translated for the Linux agent"). It also means #2127 is fixed by this PR only if the reporter's WSL codex is Linux-native. If it is the Windows shim, the wait has another cause.
2. UNC account dirs that the distro cannot translate (not reachable from Settings today)
The windowsPath check also sends \\… paths through /pu. WSL translates \\wsl.localhost\<this distro>\… and \\wsl$\<this distro>\… correctly (→ /home/…). For another distro's \\wsl.localhost\<other>\… or a network \\server\share\…, the variable is dropped entirely in Linux (${VAR+set} is empty). wsl.exe also prints wsl: Failed to translate '<path>' on stderr, and in a PTY that lands in the login tab. The login then falls back to the Linux default home, and the watcher keeps waiting.
Settings never builds such a path: onboarding always uses <wmux dir>/accounts/<vendor>-<id> on the local drive, and canonicalizeConfigDir keeps a \\wsl.localhost\… input as UNC. So this only matters if account dirs ever become user-chosen. The PR's test plan lists a real UNC fixture as not run, so this is just the observed behaviour for that gap. If you want to cover it, a small guard is enough: forward only drive paths and \\wsl.localhost\<target distro>\ / \\wsl$\<target distro>\, and otherwise skip the entry. I have not tested that guard.
openwong2kim
added
the
needs-wsl-verify
Live WSL verification of #2170Verified build: this PR's head The #2127 flow through the real UI
What each WSL pane receives (same
|
| Case | CODEX_HOME in WSL |
CLAUDE_CONFIG_DIR in WSL |
User var |
|---|---|---|---|
Ordinary pane, CODEX_HOME = the Windows account folder in env, no hint. This is exactly how the login tab was spawned before this PR, so codex login fell back to ~/.codex and the app kept waiting. |
(empty) | (empty) | |
| Ordinary pane, no env | (empty) | (empty) | |
Hint CLAUDE_CONFIG_DIR, Windows path with a space and an apostrophe, plus a CODEX_HOME that must not transfer |
(empty) | /mnt/c/Users/rizz/.wmux-wv2170/accounts/claude dir's x |
|
Hint CODEX_HOME, Linux path /home/dev/.codex-wv2170 |
/home/dev/.codex-wv2170 (unchanged, /u) |
(empty) | |
Ordinary pane with the user's own WSLENV=WV2170_USER/u |
(empty) | (empty) | kept |
Tests on Windows
- Focused unit tests (
wslIntegration,wslDistroSpawnArgs,PTYManager.createCancel,accountLogin): 4 files, 50 passed and 15 platform skips. - The opt-in real-WSL runtime test (
WMUX_TEST_WSL=1 WMUX_TEST_WSL_DISTRO=Ubuntu,wslAccountEnv.runtime.test.tsthroughvitest.runtime.config.ts): 6 passed.
No side effects: the owner's ~/.codex/auth.json and ~/.claude/settings.json are unchanged, and the isolated app and daemon were stopped (0 processes left).
Not covered here: a real OpenAI sign-in (the credential write was synthetic), the Claude login command end to end (I checked only its environment), and a non-default WSL distro.
openwong2kim
removed
the
needs-wsl-verify
|
Thanks @Jorlpj, and sorry I missed this review before my verification comment. Both points stand:
|
Summary
Settings account-login tabs now explicitly transfer the selected account home into WSL, so the Linux login command writes credentials where the Windows reader expects them. Ordinary WSL panes keep their existing Linux profile selection, avoiding Windows-only notify/MCP/hooks copied into provisioned account directories.
Changes
wslAccountLoginEnvKeyhint only in the Settings login flow and carry it through both native and daemon PTY creation. The shared injection accepts onlyCODEX_HOMEorCLAUDE_CONFIG_DIR; it never infers this hint from a workspace binding, inherited account home, or profile env./pufor Windows drive/UNC paths,/ufor explicit Linux paths. Login tabs normalize only that key's flags; ordinary panes preserve user-authored WSLENV entries. Preserve all unrelated entries and the caller's env object.CHANGELOG entry
Fixed
Stability tier impact
internalsurface (optional client/daemon login hint; no stable external contract change).Substrate contract impact (Substrate 3.0)
No stable substrate contract changes. The internal
daemon.createSessionhint is documented in docs/api/inventory.md.Test plan
09385dc67) produce 12 failures / 38 passes / 15 skips, covering unwanted default transfers, cross-vendor transfer, unsupported hints, and missing login-hint forwarding.WMUX_TEST_WSL=1and optionalWMUX_TEST_WSL_DISTRO, then runsrc/shared/__tests__/wslAccountEnv.runtime.test.tsthroughvitest.runtime.config.ts.20573f3d2upstream base); Windows daemon/web build and CSP also pass. ESLint baseline comparison against that upstream finds zero new diagnostics (17 existing errors / 24 existing warnings across touched legacy files).Related issues / PRs
Related to #2127; does not claim to close every cause of the reported login wait. The scope follows the login-only option discussed in the maintainer's review.
Reviewer checklist