Keep the GUI-subsystem node out of every pane's PATH - #697
Conversation
Deploying mouseterm with
|
| Latest commit: |
5142a72
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f83fe993.mouseterm.pages.dev |
| Branch Preview URL: | https://dev-pane-node-path.mouseterm.pages.dev |
On Windows the bundled node.exe is patched to the GUI subsystem so spawning the sidecar never flashes a console window. A child of that node gets no console at all: stdin is already at EOF, `setRawMode` is missing, and everything it prints is dropped. `cargo run` puts the directory holding it on the dev app's PATH so Windows resolves the app's DLLs, and panes inherit the app's env -- so a bare `node` in a dev pane found that node.exe ahead of the developer's own and failed silently in both directions. The installed app never has that directory on PATH, so this only ever bit dev builds, and dropping the entry makes a dev pane match an installed one. `start_sidecar` passes the directory as DORMOUSE_GUI_NODE_DIR and the sidecar strips it from the PATH each pane inherits. The host names the directory because `process.execPath` is the wrong answer twice over: the agent-browser harness runs the sidecar under the developer's own node, and the same module runs under the VS Code pty host, where the running node's directory must stay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
70719e1 to
de08469
Compare
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark the PR ready when you want the full review.
The PATH-position/subsystem table and the "verified against 0.11.0 installed" scope check are what make this rule credible, and right now they live only in the PR body. AGENTS.md ("Specs") routes exactly that — evidence and measurements — to docs/specs/standalone.rationale.md, keyed by the spec's heading and marked (rationale) at the rule it backs; the two sibling bullets in this same section already do it, and ## Windows node subsystem exists there to hang it on. Without that, the next person to touch the strip has no record of why the installed app was ruled out, and re-derives it from a Windows box. The inline suggestion trims the bullet to match the siblings, assuming the evidence moves.
Per review: AGENTS.md routes measurements and dead approaches to `<foo>.rationale.md`, keyed by the spec heading and marked `(rationale)` at the rule. The PATH-position/subsystem table and the ruling-out of the installed app lived only in the PR body, where the next person to touch the strip would not find them; they now sit under `## Windows node subsystem` in `docs/specs/standalone.rationale.md`, along with why `process.execPath` is the wrong way to name the directory. `node_path` comes back from `resolve_node_binary_path` as `dir.join(...)`, so `parent()` is always `Some` and the `unwrap_or_default()` arm was dead. Passing the `&Path` straight to `Command::env` also drops a `to_string_lossy` round-trip that could have handed the sidecar a spelling no longer matching the real directory, silently turning the strip into a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
spec-lint check 4 resolves any backticked repo-relative path, and the dev app's target dir is not in SKIP_PATH_PREFIXES -- so the rationale's `standalone/src-tauri/target/debug/node.exe` passed here, where the artifact exists, and failed CI on a clean checkout. Describe the binary by where it sits instead of spelling a path that only exists after a build; the skip list is for references the specs need, not for prose that can be reworded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
145 added lines for dropping one PATH entry was too much. No behaviour change, and every case the tests pinned is still pinned. - Fold `normalizeWindowsPathEntry` into a local. The trim and quote-strip were dead: the only entry that has to match is the one `cargo run` appended, which is neither padded nor quoted. Case folding stays and is load-bearing, not just test-pinned -- cargo builds its entry from the workspace path as typed, while the host derives the directory from `current_exe()`, so the two can differ in case. - Collapse four guards to one. `!target` was unreachable given `dir` is truthy, and `!existing` folds into the first condition. - Cut the comment from 16 lines to 9. The measurements and the installed-app ruling-out are in the rationale now; what stays is the mechanism an editor of this module needs, and the one sentence that stops a future simplification from reaching for `process.execPath`. - Merge the two win32 tests. They were one rule plus two facts about matching: spelling the target entry with `/`, a trailing separator and a different case pins all three at once, next to the `debug\deps` sibling that pins no prefix matching. - Hoist `paneEnv` so the env chain keeps its original shape. Also corrects a reason I had wrong: the comment and the non-win32 test claimed `DORMOUSE_GUI_NODE_DIR` could name a directory a pane needs, like `/usr/bin`. Only `start_sidecar` ever sets it, always to the bundled node's directory -- that hazard belongs to the rejected `process.execPath` approach. The gate is still right, for the real reason: on Unix the app's directory is never on PATH. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress, not a merge verdict — mark the PR ready when you want the full review.
The cut-down reads well and the evidence landed cleanly in the rationale. The rewrite did introduce one thing worth fixing before this goes out: the comment on withoutGuiNodeDir names the wrong mechanism. It says a child of a GUI-subsystem process gets no console at all — but the pane's shell is exactly that child (the sidecar runs under the GUI node and node-pty hands the shell a ConPTY), and it has a console and works. What actually fails is the patched node.exe itself: a GUI-subsystem binary does not attach to an inherited console. That is what resolve_dor_node_path in standalone/src-tauri/src/lib.rs already says — "A GUI-subsystem node, however, does not attach to an inherited console" — and what this spec section's dor bullet repeats.
The consequence is that the comment points a future editor at parentage: read it literally and the strip looks removable by spawning panes from a console-subsystem process, which would not help. The same sentence is in the win32 test's comment. The spec bullet and both rationale paragraphs state it correctly, so only these two need the edit.
Per review, the rewritten comment said a *child* of a GUI-subsystem process gets no console. That is wrong, and this branch's own measurements disprove it: the pane's shell is exactly such a child -- the sidecar runs under the GUI node and node-pty hands the shell a ConPTY -- and it works, as does the console-subsystem node.exe from `node_modules/.bin`, which reported `isTTY` true from inside the same pane. What fails is the patched binary itself: a GUI-subsystem binary does not attach to an *inherited* console, which is what `resolve_dor_node_path` and this section's `dor` bullet already say. Read the old wording literally and the strip looks removable by spawning panes from a console-subsystem process, which would not help at all. The spec bullet and both rationale paragraphs already stated it correctly; only the code comment and the win32 test's comment needed the edit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
On Windows,
nodein apnpm dev:standalonepane resolved to the bundlednode.exeinstandalone/src-tauri/target/debug, whichbuild.rspatches tothe GUI subsystem so spawning the sidecar never flashes a console window.
A child of a GUI-subsystem node gets no console at all, and the symptoms are
silent in both directions:
process.stdin.isTTYisundefined, stdin is already at EOF, andsetRawModedoes not exist — so a script that reads the terminal dies atstartup.
cargo runputs that directory on the dev app's PATH (that is how Windowsfinds the app's DLLs) and panes inherit the app's env, so the patched node.exe
sat ahead of the developer's own. Measured in a dev pane before this change:
isTTYsetRawModetarget/debug/node.exeundefinedundefinednode_modules/.bin/node.EXEtruefunction.version-fox/sdks/nodejs/node.exetruefunctionThe fix
start_sidecarpasses the directory asDORMOUSE_GUI_NODE_DIR, and thesidecar drops it from the PATH each pane inherits. It goes in the same chain as
the existing
DORMOUSE_CLI_BINprepend andORIGINAL_PATHscrub, and thevariable itself is stripped from the pane env like the other internal ones.
The host names the directory rather than the sidecar deriving it, because
process.execPathis the wrong answer twice over:standalone/scripts/dev-agent-browser.mjsruns the sidecar under the developer's own node, and this module is also the VS
Code pty host (
vscode-ext/src/pty-host.js), where the running node'sdirectory —
/usr/bin, say — must stay on PATH. The strip is win32-only forthe same reason.
Scope
Dev builds only. The installed app ships the same GUI-subsystem
node.exe,but neither its install directory nor any
target/debugis on the persistentuser or machine PATH that a shortcut-launched app inherits, and panes add only
DORMOUSE_CLI_BIN— sonodethere was already the developer's own. Verifiedagainst Dormouse Terminal 0.11.0 installed on this machine. The fix makes a dev
pane match an installed one rather than adding a special case.
Verification
cargo checkclean (the two warnings are pre-existing, elsewhere inlib.rs).standalone/sidecar/pty-core.test.jscover the strip, pathspelling (case,
/vs\, trailing separator), and that non-win32 keeps thedirectory. All pass.
pnpm lint:specsOK.docs/specs/standalone.mdgained the rule under"Windows node subsystem" and was re-baselined with
--ratchetin this PR.Not verified in a running dev app: the sidecar has to be rebuilt to pick this
up, so the end-to-end check is
nodein a fresh dev pane resolving to thedeveloper's own node.
Pre-existing Windows test failures, untouched by this PR
Confirmed identical on pristine
origin/main, so left alone:standalone/sidecar/pty-core.test.js— 3 shell-integration cases assertPOSIX-separator paths that
path.joinrenders with\on Windows.standalone/scripts/dev-standalone.test.mjs— all 3 cases fail.🤖 Generated with Claude Code