fix(desktop): Windows goose discovery, install PATH, and hidden consoles#2247
fix(desktop): Windows goose discovery, install PATH, and hidden consoles#2247tzarebczan wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 681ed8379b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::path::Path::new(r"C:\nvm4w\nodejs"), | ||
| std::path::Path::new(r"C:\Windows\System32"), |
There was a problem hiding this comment.
Use platform-native paths in the PATH fallback test
On Linux and macOS, this unconditionally compiled test panics before exercising the helper because std::env::join_paths rejects components containing :—both Windows drive-letter paths contain one. Consequently, the desktop Tauri unit-test suite fails on Unix CI; gate this test with #[cfg(windows)] or use platform-native sample paths and assertions.
AGENTS.md reference: AGENTS.md:L93-L94
Useful? React with 👍 / 👎.
681ed83 to
9968bea
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
Reviewed at head 681ed83 — full-file reads plus running the new unit tests locally on macOS. These are well-scoped fixes for three real Windows issues, and the architecture is right: pure testable helpers (install_shell_path_parts, windows_well_known_binary_dirs), post-install resolve verification that turns a false-positive "Installed" into an actionable step failure, and honest UI state (INSTALLED now means discovery confirmed it). I also verified goose's download_cli.sh upstream — line 63 confirms DEFAULT_BIN_DIR="$USERPROFILE/goose" on native Windows, so the new probe dir matches the installer exactly.
Requesting changes for two blockers:
test_install_shell_path_falls_back_to_process_path_without_login_shellpanics on macOS/Linux (reproduced locally, details inline) — CI's macOSjust desktop-tauri-testjob will fail.- DCO check is failing — commits need
Signed-off-by; amend/rebase with-s.
Non-blocking: the 5-line CREATE_NO_WINDOW block is now copy-pasted ~9× across the codebase (7 new here + runtime.rs:1897, process_lifecycle.rs:114) — a shared helper would make the next missed spawn site greppable to one name. Two nits inline.
c8cd1d9 to
8775485
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
Re-reviewed at the new head 8775485 — thanks for the thorough rework, both original blockers are confirmed resolved:
- ✅ Test portability: the new
install_shell.rstests use platform-neutral paths; I re-rancargo test --lib install_shell_pathon macOS at this head — 4 passed, 0 failed. - ✅ DCO: commit is signed off, DCO check is green.
- ✅
CREATE_NO_WINDOWduplication:windows_console::hide_console()adopted across the touched spawn sites, with the tokio variant kept local inbuzz-dev-mcp— exactly the right split. - ✅ The new
windows_paths.rsmodule, PATH dedup, and theGOOSE_BIN_DIR=$HOME/.local/binpin all look good — I verified the goose installer honors theGOOSE_BIN_DIRoverride (download_cli.sh line 69) and~/.local/binis already on Buzz's discovery path list.
One new blocker introduced in this revision (line comment below): the branch now reverts the Hermit-pinned lefthook change from #2241 (Justfile + CONTRIBUTING.md). Once that's dropped or split out with rationale, this is good to approve.
9d713a9 to
0ff8a7b
Compare
|
rebased after the merge conflict. Checked the failing tests on https://github.com/block/buzz/actions/runs/29856956174/job/88727060509 and they should work now on the latest commit. |
|
Nice — this cleans up the install/probe PATH and the discovery/git/shell/auth consoles. One console source that looks outside this PR's diff: the persistent agent-harness child spawned in // crates/buzz-acp/src/acp.rs (~line 466)
#[cfg(unix)]
cmd.process_group(0);
let mut child = cmd.spawn()?;Because the parent is windowless, each managed agent's adapter ( Minimal fix mirroring the sidecar flag (tokio's #[cfg(windows)]
{
const CREATE_NO_WINDOW: u32 = 0x0800_0000;
cmd.creation_flags(CREATE_NO_WINDOW);
}Happy to fold this into this PR or open a separate one — whichever you prefer. |
|
I think a separate one would be better. This one was already approved. Thanks for looking and helping! Edit: Actually I re-read it, and yes please, fix that here if possible. If not, I can tomorrow in the am... Late here. |
|
I ran into another Windows case that i don’t think this PR covers. I posted the details and timings in #2328: #2328 (comment) On my machine, Buzz picks:
instead of the actual Git Bash:
The WindowsApps one launches WSL and the lookup hangs. It didn’t finish within 15 seconds, while the same lookup using Git Bash finished in about 0.5–0.7 seconds. From what I can tell, Could we also prefer Git Bash here, or skip the WindowsApps/WSL alias? |
|
Building on @jjgarciarovira’s
My local patch covers these and the already-mentioned agent CLI spawn: It overlaps this PR, so it is for reference rather than cherry-picking as a whole. |
|
thanks @tzarebczan, happy to help! the one-liner is already committed on my fork if it's easiest to cherry-pick into this pr: jjgarciarovira@36fbcd4 (branch also worth folding in @suraciii's catch in the same pass: the same whatever's easiest on your end, and i'm glad to open a separate pr instead if you'd rather keep this one focused. thanks for the quick look! |
0ff8a7b to
f54f15a
Compare
Desktop launches buzz-acp windowless; without the same flag the adapter child (cmd.exe/node, buzz-agent, …) allocates its own console per agent. Refs: block#2292, PR block#2247 review
PATH often lists %LOCALAPPDATA%\Microsoft\WindowsApps\bash.exe first; that App Execution Alias launches WSL and can hang lookups (~15s+) while real Git Bash is sub-second. Prefer git-derived / install / registry paths, then PATH bash, and skip WindowsApps (and System32) on PATH. Refs: block#2328, PR block#2247
Nested CreateProcess sites still flash consoles when the parent is windowless: buzz-agent MCP spawns and buzz-dev-mcp delegated rg. Refs: PR block#2247 review, block#2292
|
Thanks again, just pushed up a commit with all the fixes mentioned recently. Tried to refactor/simplify where possible. Would be great to have another set of eyes/tests too. Much appreciated! |
Goose install could report success while discovery still saw NotInstalled (binary in %USERPROFILE%\goose off PATH), blocking onboarding Next (block#2239). Install shells also dropped the process PATH on Windows so system npm was invisible for Codex/Claude adapters (block#2238). Console-subsystem probes stole focus via Git Bash. - Probe well-known Windows Node/npm/goose paths; fall back to process PATH for install shells; pin goose GOOSE_BIN_DIR to ~/.local/bin on Windows. - Post-install resolve check; SetupStep only shows Installed after discovery. - Centralize CREATE_NO_WINDOW via windows_console::hide_console (and buzz-dev-mcp hide helper) on spawn sites this fix touches. Signed-off-by: Thomas Zarebczan <thomas.zarebczan@gmail.com>
…ECK AGAIN - Restore Justfile + CONTRIBUTING.md from main (accidental soft-reset squash had undone Hermit-pinned lefthook from block#2241). - beforeDevCommand: use `pnpm exec vite` instead of Unix-only `exec` (tauri.conf + instance-env) so Windows cmd can start the dev server. - After install success, reuse the existing CHECK AGAIN / CHECKING… pattern so rediscovery is not a permanent spinner. Signed-off-by: Thomas Zarebczan <thomas.zarebczan@gmail.com>
Desktop launches buzz-acp windowless; without the same flag the adapter child (cmd.exe/node, buzz-agent, …) allocates its own console per agent. Refs: block#2292, PR block#2247 review Signed-off-by: Thomas Zarebczan <thomas.zarebczan@gmail.com>
PATH often lists %LOCALAPPDATA%\Microsoft\WindowsApps\bash.exe first; that App Execution Alias launches WSL and can hang lookups (~15s+) while real Git Bash is sub-second. Prefer git-derived / install / registry paths, then PATH bash, and skip WindowsApps (and System32) on PATH. Refs: block#2328, PR block#2247 Signed-off-by: Thomas Zarebczan <thomas.zarebczan@gmail.com>
Nested CreateProcess sites still flash consoles when the parent is windowless: buzz-agent MCP spawns and buzz-dev-mcp delegated rg. Refs: PR block#2247 review, block#2292 Signed-off-by: Thomas Zarebczan <thomas.zarebczan@gmail.com>
- Route desktop spawns through windows_console::hide_console (runtime, taskkill); stop re-inlining CREATE_NO_WINDOW. - Add buzz-dev-mcp windows_console for std/tokio; use from shell + rg. - Single hide_console helper in buzz-acp and buzz-agent MCP spawn. - Drop redundant WindowsApps also_skip; one path_looks_like check. - Align Doctor/MCP PATH scan API + KEEP IN SYNC notes (full shared resolver deferred: buzz-agent forbids unsafe registry code). - SetupStep: one CheckAgainButton for auth-unknown and post-install. No push. Signed-off-by: Thomas Zarebczan <thomas.zarebczan@gmail.com>
f54f15a to
c51b985
Compare
|
I reproduced a second Windows PATH gap on the affected v0.4.23 machine that is still present at this PR's current head ( End-to-end reproduction:
Root cause: this PR fixes The focused patch is now published as draft PR #2533: #2533. It is stacked on this PR and adds one commit (
The isolated |
|
Thanks for getting this into a release with your pr @wpfleger96 , will close. |
Summary
Fixes Windows onboarding/runtime install friction reported in #2239 and #2238.
#2239 — Goose shows Installed but Next stays blocked
%USERPROFILE%\gooseand does not put it on PATH.NotInstalledafter a successful install.installSuccesswhile Next requiresavailability === available.%USERPROFILE%\goose, post-install resolve verification, only show Installed after discovery confirms (Checking while waiting).#2238 — Codex ACP install fails on Windows
login_shell_path()isNone(always on Windows).npm: command not found.Console focus-steal
CREATE_NO_WINDOW.Test plan
windows_well_known_dirsinclude goose path%USERPROFILE%\gooseshould be found without reinstall)codex-acpinstall (system npm present)Closes #2239
Closes #2238