fix(desktop): make the Windows terminal spawn, close, and inherit PATH (#4930) - #5425
Open
Glucksberg wants to merge 7 commits into
Open
fix(desktop): make the Windows terminal spawn, close, and inherit PATH (#4930)#5425Glucksberg wants to merge 7 commits into
Glucksberg wants to merge 7 commits into
Conversation
Glucksberg
marked this pull request as ready for review
August 9, 2026 20:07
The env fence (env_clear + allowlist) shipped with a Unix-only allowlist, which on Windows wiped ComSpec and USERPROFILE from the builder's env map. portable-pty resolves the default program by reading ComSpec from that map and passes it to CreateProcessW as lpApplicationName — no path search — so every terminal spawn on Windows failed with: CreateProcessW "cmd.exe" in cwd None failed: The system cannot find the file specified. (os error 2) Fix, entirely inside buzz-terminal: - Split the allowlist into UNIX_INHERIT_ALLOWLIST (unchanged) and WINDOWS_INHERIT_ALLOWLIST (ComSpec, SystemRoot, windir, PATHEXT, USERPROFILE, HOMEDRIVE/HOMEPATH, APPDATA, LOCALAPPDATA, TEMP/TMP, USERNAME), cfg-selected. Still allowlist-not-denylist: BUZZ_* secrets remain fenced out on both platforms. - fence_env's final step is platform-aware: Unix sets SHELL, Windows sets ComSpec to the resolved shell, overriding any inherited value. - Windows resolve_shell validates ComSpec (must be drive/UNC-rooted and an existing file — a relative ComSpec is a planted-cmd.exe hijack vector) and falls back to the absolute %SystemRoot%\System32\cmd.exe. The pure selection half (pick_windows_shell) compiles on every platform so the unix CI actually exercises it. - Restored USERPROFILE also supplies portable-pty's cwd fallback, fixing the "in cwd None" half of the failure. Six new portable tests cover the Windows allowlist (no secrets, spawn contract) and shell selection (valid ComSpec honoured; missing/relative/ empty-env fall back to the absolute default). Addresses the root cause behind upstream issue block#4930. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Glucksberg <glucksberg89@gmail.com>
`user_shell_path` returned a hand-built `System32;Windows;Wbem` on Windows, mirroring the Unix design. The mirror does not hold: on Unix a minimal PATH is a starting point because the login shell's rc files rebuild it, so Hermit can be fenced out without cost. `cmd.exe` has no rc file, and a Windows user's PATH lives in the registry and is composed into the process environment at launch — nothing inside the session ever restores what we left out. The constructed value therefore deleted `ssh`, `git`, `node`, and every other installed tool for the whole session. Field-verified on 0.5.9-fork.3: `ssh` was "not recognized as an internal or external command" in the Buzz terminal on a machine where it resolves everywhere else. Windows now inherits `PATH` and appends the system directories it must contain (System32, Windows, Wbem, WindowsPowerShell\v1.0), preserving inherited order — an entry the user put ahead of System32 is a deliberate override — and de-duplicating case-insensitively so the value does not grow a second copy of a directory it already names. Hermit, the reason for constructing on Unix, is a POSIX shell script and is never active in a Windows process. The Unix path is untouched. The Windows allowlist gains the rest of the machine-layout keys for the same structural reason (SystemDrive, ProgramData/ALLUSERSPROFILE, the ProgramFiles trio, PUBLIC, PSModulePath, USERDOMAIN, COMPUTERNAME, NUMBER_OF_PROCESSORS, PROCESSOR_ARCHITECTURE, OS): what a Windows shell does not inherit, it does not get. All are paths and counts any process can read; the keys the fence exists to withhold stay unlisted, and the no-secrets test covers the list as it grows. Three new portable tests: inherited entries keep order and precedence, system directories are never duplicated, and an empty inherited PATH still yields a usable one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Glucksberg <glucksberg89@gmail.com>
Every `Path` in this module belongs to the Unix executability check, so a Windows build compiled the import as dead weight and warned about it — which becomes an error under `-D warnings`, the way a contributor would lint the crate on the platform this branch is about. Verified with `cargo clippy -p buzz-terminal --target x86_64-pc-windows-msvc -- -D warnings`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Glucksberg <glucksberg89@gmail.com>
…Windows Session::shutdown's non-unix arm joined the reader thread while the PTY master was still alive. ConPTY inverts the Unix EOF contract that order was written against: the output pipe does not EOF when the child dies, only when the pseudo console is closed (ClosePseudoConsole runs in the master's Drop). The reader sits blocked in read(), stop()'s AtomicBool is never observed, and join() blocks forever. terminal_close is a sync Tauri command, so the deadlock lands on the app's main thread: closing the Term panel, switching channels, or quitting hangs the whole app — the AppHangB1 / "Application Hang" half of upstream block#4930 (the 0.5.5 symptom, masked since the env fence broke spawning entirely). Fix: drop the master (closing the pseudo console) after the child is killed and reaped but before joining the reader. That is also the order ConPTY requires — the close blocks until pending output is consumed, and the reader is still draining at that point, then reads EOF and exits. The Unix path is untouched: there the child's death EOFs the master by itself, and shutdown_draining already encodes the drain law. Field evidence: 0.5.9-fork.3 (first build where spawning works) hung twice within ~2 minutes with identical WER AppHangB1 signatures and no crash/panic in any log — consistent with a parked main thread, not a fault. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Glucksberg <glucksberg89@gmail.com>
The Windows teardown was written inline in `Session::shutdown`, which put the ConPTY reasoning in the Tauri crate while its Unix counterpart — the drain law it is an exception to — lives in `buzz-terminal::lifecycle`. A reader comparing the two orders had to look in two crates to find them. `shutdown_closing_console` now sits beside `shutdown_draining`, and takes the master **by value** for the same reason `shutdown_draining` takes the reader by value: the ordering becomes a property of the signature rather than of a comment. A caller cannot close the pseudo console early, because passing it here is the only way to close it at all. Behaviour is unchanged. The helper also returns `Shutdown`, so the Windows arm now reports how the session ended instead of discarding it, and `terminal_runtime.rs` returns under the 1000-line file guard it had just crossed — the split the guard asks for, not a raised limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Glucksberg <glucksberg89@gmail.com>
`POLL_INTERVAL` and the `Instant` import exist only for `leader_exited_by`, which is `#[cfg(unix)]`. Every non-unix build therefore compiled them as dead code and emitted two warnings — harmless under a plain build, an error the moment the crate is linted with `-D warnings`, which is how a Windows contributor would run it. Verified with `cargo clippy -p buzz-terminal --target x86_64-pc-windows-msvc -- -D warnings`, which now reaches the crate clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Glucksberg <glucksberg89@gmail.com>
Signed-off-by: Glucksberg <glucksberg89@gmail.com>
Glucksberg
force-pushed
the
fix/windows-terminal-4930
branch
from
August 9, 2026 22:34
3f40759 to
1466771
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Buzz Term never worked on Windows, and the failure mode was an app-wide hang rather than an error. Investigating #4930 turned up three separate defects, each one hiding the next, and all three with the same origin: this subsystem was written against Unix and the Windows arms were written as mirrors of it. Two of the three mirrors are invalid, because the platform contract they mirror is inverted on Windows.
Fixing only the first gets you a terminal that hangs the app. Fixing the first two gets you a shell with no
ssh,git, ornode. So they are here together.1. The spawn never happened —
CreateProcessW "cmd.exe" in cwd None failed (os error 2)env_fence::fence_envcallsenv_clear()and rebuilds from an allowlist. That allowlist was Unix-only (HOME,USER,LANG, …), so on Windows it clearedComSpecandUSERPROFILEand put back neither.Those two are not cosmetic on Windows.
portable-pty0.9.0 readsComSpecout of the builder's own env map (cmdbuilder.rs,cmdline()) and hands the result toCreateProcessWaslpApplicationName— which performs no path search. WithComSpecgone the builder falls back to the bare string"cmd.exe",CreateProcessWrefuses to search for it, and you getos error 2.current_directory()readsUSERPROFILEfrom that same map and filters it throughis_dir(), which is wherecwd Nonecomes from.shell::resolve_shellwas also#[cfg(unix)]only, so there was no Windows shell resolution at all. This PR adds one, and it validates: aComSpeccandidate must be rooted (drive-qualified or UNC) and an existing file, otherwise it falls back to%SystemRoot%\System32\cmd.exe. The rootedness check is not decoration — a relativeComSpecwould let acmd.exedropped in the working directory be launched instead.@BoydVC's hypothesis in the issue was close: the resolution chain really does have no valid candidate on a stock Windows install. The reason is one layer deeper than
$SHELL— the fence had already deleted the variable Windows uses in its place.2. The hang itself — ConPTY inverts the EOF contract
This is the
AppHangB1, and it is not the spawn failure. It only becomes reachable after #1 is fixed, which is why the issue reports a hang rather than the error above.On Unix, the pty slave closing when the child dies is what EOFs the master, so a reader blocked in
read()ends on its own. ConPTY does not do this. The conout pipe stays open after the child exits and EOFs only whenClosePseudoConsoleruns — whichportable-ptyperforms in the master'sDrop(win/psuedocon.rs:73-75).Session::shutdown's non-unix arm joined the reader while still holding the master. The reader was parked inread(), so it could never observe the stop flag, andjoin()blocked forever.terminal_closeis a synchronous Tauri command, so that block landed on the app's main thread: the whole window stopped responding, with no panic and no stack — exactly the "Application Hang rather than a crash/panic … blocks the main thread" in the report.The fix drops the master before the join. That is also the order ConPTY itself requires:
ClosePseudoConsoleblocks until pending output is consumed, so the reader has to still be draining when it runs.This now lives in
buzz-terminal::lifecycle::shutdown_closing_console, beside the Unixshutdown_drainingwhose ordering law it is an exception to — a reader comparing the two orders shouldn't have to look in two crates. It takes the master by value for the same reasonshutdown_drainingtakes the reader by value: the ordering becomes a property of the signature instead of a comment. A caller cannot close the console early, because passing it in is the only way to close it at all.3.
PATHwas constructed instead of inherited, which deletes the user's toolchainpath::user_shell_pathbuilds a minimalPATHrather than inheriting Buzz's, deliberately: Buzz runs under Hermit activation, and inheriting it would hand the user Buzz's pinnedcargo/nodeinstead of their own. The Windows arm mirrored that with a constructedSystem32;Windows;Wbemstub.That mirror is invalid. On Unix a minimal
PATHis a starting point, because the login shell reads rc files that rebuild it — that is precisely what lets Hermit be fenced out without collateral.cmd.exereads no rc file. The user'sPATHlives in the registry (HKCU\Environmentplus the machineSession Manager\Environment) and is composed into the process environment at launch, so anything omitted at spawn is omitted for the entire session with no mechanism to restore it. In practice that deletedssh,git,node, andpythonfrom the Term.Windows now inherits
PATHand appends the guaranteed system directories, preserving inherited order (so user overrides stay ahead) and de-duplicating case-insensitively. The Hermit rationale simply does not apply:bin/activate-hermitis a POSIX shell script and is never active on Windows. The Unix arm is untouched.The security invariant is unchanged
env_fenceexists to keepBUZZ_PRIVATE_KEY,BUZZ_AUTH_TAG, and relay credentials out of spawned children, and it is an allowlist, never a denylist. That is preserved exactly. The Windows list is longer than the Unix one for a structural reason rather than a permissive one — no rc file means no second chance — and every entry on it is machine or user layout: paths, counts, architecture, things any process on the box can already read. No key, token, or credential is listed, andwindows_allowlist_admits_no_secretsasserts that case-insensitively, since Windows environment names are case-insensitive and a case-only bypass would otherwise be invisible.Related issue
Fixes #4930.
Overlap with #4829 — please read before merging either
@weiox's #4829 (open since Aug 5) touches the same two files, and the two changes interact. I found it while checking for duplicates and I'd rather flag it than have it discovered at merge time.
POLL_INTERVAL,Instant, andPathbehind#[cfg(unix)]. My two smallest commits do exactly that. If fix(desktop): make desktop terminal and sidecars Windows-clean #4829 lands first, I'll drop them and rebase — they exist here only so this branch compiles forx86_64-pc-windows-msvcon its own.use std::io;anduse portable_pty::Child;to unix. This PR'sshutdown_closing_consoleis#[cfg(not(unix))]and uses both, so those two imports must stay ungated. Merging fix(desktop): make desktop terminal and sidecars Windows-clean #4829 as-is on top of this PR breaks the Windows build with unresolvedioandChild.Nothing in #4829's
Justfilehalf overlaps with this PR. Happy to rebase onto it, fold this into it, or split the gating out entirely — whichever the maintainers prefer; I don't want to step on an older PR.Otherwise: no other open PR touches
env_fence.rs,path.rs,terminal_runtime.rs, ordesktop/src/features/term, and #4930 has no linked PR.The report's fourth symptom — "the Term panel renders permanently blank" after relaunch — is a separate surface (a failed attach renders nothing rather than an error) and is not addressed here. With this PR the attach succeeds, so the blank panel should not be reachable by this path, but the missing empty/error state is its own defect and deserves its own change.
Testing
Field-verified on the reported platform. Reproduced and fixed on Windows 11 build 26200 across six unsigned canary builds, one defect per cycle:
CreateProcessW "cmd.exe" in cwd None failed … (os error 2)AppHangB1, twice, no panic and no stack in WER'ssh' is not recognized as an internal or external commandssh,git,noderesolve; open / close / switch channels / quit all healthyAutomated. 36 unit tests in
buzz-terminal, of which the Windows-specific logic is 9. Those tests run on the Unix CI: the Windows behaviour is factored into pure functions with injected effects (pick_windows_shell,windows_shell_path), compiled on all platforms and driven directly, with thin#[cfg(windows)]wrappers supplyingstd::env::var/std::fs::metadatain production. Windows logic that only compiles on Windows is Windows logic nobody runs until a user finds it.They cover: the no-secrets property; the full spawn contract (
ComSpec,USERPROFILE,SystemRoot,PATHEXT); a validComSpechonoured; a missing one falling back; a relativeComSpecrejected even when the file exists; inheritedPATHkeeping its order and precedence; system directories not duplicated across case and trailing-separator differences; and an entirely empty environment still yielding a usable shell andPATH.Cross-target.
cargo clippy -p buzz-terminal --target x86_64-pc-windows-msvc -- -D warningsis clean. Reaching that also required cfg-gating three pre-existing unix-only items (POLL_INTERVAL, and theInstantandPathimports) that were dead code on every non-unix build — harmless under a plain build, an error the moment the crate is linted with-D warnings. Those are the two smallest commits here, and the ones #4829 already covers (see above).Full CI. Verified green on a branch simulating this merged into
main:Windows Rust (x86_64-pc-windows-msvc),Rust Lint,Desktop Core, all smoke and integration E2E shards, macOS build,Unit Tests,Web,Mobile, and both cross-compile targets. The only red wasSecurity, which fails identically onmainitself (nostr-relay-poolunmaintained advisory) and is unrelated to this change.No screenshots: the change is a shell that spawns instead of an app that hangs, and the terminal contents are the user's own machine.