fix(setup): quote repo path in --spawn terminal commands (fixes #1035)#1037
fix(setup): quote repo path in --spawn terminal commands (fixes #1035)#1037kagura-agent wants to merge 1 commit intocoleam00:devfrom
Conversation
…m00#1035) On Windows, spawnWindowsTerminal passes the repo path unquoted to wt.exe and cmd.exe. When the path contains spaces (e.g. 'C:\Users\...\GitHub Libraries\Archon'), the shell splits it at the first space, causing a 'could not be found' error dialog. Fix: wrap repoPath in double-quotes for both the wt.exe and cmd.exe spawn paths. Also switch wt.exe to shell: true for consistent quoting behavior across both paths.
📝 WalkthroughWalkthroughUpdated Windows terminal spawning to correctly handle repository paths containing spaces by explicitly quoting the path before passing it to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/commands/setup.test.ts (1)
365-374: This test is not actually validating quoting and is environment-dependent.Line 369 spawns real terminal logic, and the assertions only verify
{ success: boolean }. This can pass even if quoting breaks, and may behave differently across developer/CI environments. Consider making this deterministic by asserting constructed Windows spawn arguments (without launching real terminals).Based on learnings: Applies to **/*.ts : Prefer reproducible commands and locked dependency behavior in CI-sensitive paths; keep tests deterministic — no flaky timing or network dependence without guardrails; ensure local validation commands (
bun run validate) map directly to CI expectations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.test.ts` around lines 365 - 374, The test spawnTerminalWithSetup currently invokes real terminal-spawn logic making it environment-dependent and not asserting quoted Windows args; modify the test to avoid launching real terminals by mocking/stubbing the platform/terminal detection and the internal spawn construction path inside spawnTerminalWithSetup, then call the function or the helper that builds the command array and assert that for a path like join(tmpdir(), 'path with spaces', 'Archon') the returned/constructed Windows command/arguments include proper quoting (e.g., wrapped in quotes) and that the function returns the expected object shape; specifically locate and stub the spawnTerminalWithSetup code paths that detect Windows vs POSIX and the internal command-builder, verify the produced command string/args for Windows quoting deterministically, and remove any real-process spawning from the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 1232-1239: The wt.exe launch currently uses shell: true to
preserve quoting (as added for `#1035`) which causes trySpawn to report success
even if wt.exe inside the shell fails; keep shell: true but change the logic in
trySpawn (and the call site that spawns 'wt.exe') to detect inner-process
failures by attaching to the returned ChildProcess: remove or adjust stdio:
'ignore' so you can listen for 'exit'/'error' and inspect the exit code and
stderr, and on non-zero exit or error fall back to spawning cmd.exe; update the
spawn handling around trySpawn('wt.exe', ...) and the ChildProcess listeners to
only treat the spawn as successful when the child exits with code 0 (or no
error) so the cmd.exe fallback runs when wt.exe fails.
---
Nitpick comments:
In `@packages/cli/src/commands/setup.test.ts`:
- Around line 365-374: The test spawnTerminalWithSetup currently invokes real
terminal-spawn logic making it environment-dependent and not asserting quoted
Windows args; modify the test to avoid launching real terminals by
mocking/stubbing the platform/terminal detection and the internal spawn
construction path inside spawnTerminalWithSetup, then call the function or the
helper that builds the command array and assert that for a path like
join(tmpdir(), 'path with spaces', 'Archon') the returned/constructed Windows
command/arguments include proper quoting (e.g., wrapped in quotes) and that the
function returns the expected object shape; specifically locate and stub the
spawnTerminalWithSetup code paths that detect Windows vs POSIX and the internal
command-builder, verify the produced command string/args for Windows quoting
deterministically, and remove any real-process spawning from the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95a5198d-ea37-4d90-98e7-2d762bc8b729
📒 Files selected for processing (2)
packages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.ts
| // wt.exe parses its own command line, so we need shell: true | ||
| // with explicit quoting rather than relying on Node's auto-quoting. | ||
| if ( | ||
| trySpawn('wt.exe', ['-d', repoPath, 'cmd', '/k', 'archon setup'], { | ||
| trySpawn('wt.exe', ['-d', quoted, 'cmd', '/k', 'archon setup'], { | ||
| detached: true, | ||
| stdio: 'ignore', | ||
| shell: true, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process.spawn on Windows, when { shell: true } is used, can spawn(...) return a child PID even if the target executable (e.g., wt.exe) fails to launch, with failure only visible via quick shell exit?
💡 Result:
Yes.
With { shell: true } on Windows, spawn() first launches a shell (by default process.env.ComSpec, typically cmd.exe, using /d /s /c) and the ChildProcess you get back represents that shell process. So child.pid can be a valid PID as long as the shell itself started successfully—even if the command inside the shell (e.g., wt.exe) fails to launch, in which case the only sign may be that the shell prints an error to stderr and exits quickly with a non‑zero exit code. [1][2]
In other words, that PID can be “real” but correspond to the shell wrapper, not the target executable; this wrapper-vs-real-command PID distinction is a known/observed behavior when using shell:true. [3]
What to check instead: listen for error (shell failed to spawn) vs close/exit + exit code and stderr (command failed inside the shell). [2]
Sources: [1] (nodejs.org) [2] (nodejs.org) [3] (stackoverflow.com)
Citations:
- 1: https://nodejs.org/download/release/v22.19.0/docs/api/child_process.html
- 2: https://nodejs.org/api/child_process.html
- 3: https://stackoverflow.com/questions/75575209/child-process-not-killed-when-starting-another-one-immediately
🏁 Script executed:
# Find the trySpawn function definition
rg -n "function trySpawn|const trySpawn|export.*trySpawn" packages/cli/src/commands/setup.tsRepository: coleam00/Archon
Length of output: 82
🏁 Script executed:
# Get full context around lines 1232-1239
sed -n '1220,1250p' packages/cli/src/commands/setup.tsRepository: coleam00/Archon
Length of output: 1137
🏁 Script executed:
# Find the trySpawn implementation
rg -B5 -A15 "function trySpawn|const trySpawn\s*=" packages/cli/src/commands/setup.tsRepository: coleam00/Archon
Length of output: 581
🏁 Script executed:
# Search for any comments or issues mentioning wt.exe and shell: true
rg -B2 -A2 "wt\.exe" packages/cli/src/commands/setup.ts | head -40Repository: coleam00/Archon
Length of output: 598
🏁 Script executed:
# Check git history if available for context on this code
git log --oneline -n 20 -- packages/cli/src/commands/setup.ts 2>/dev/null | head -20Repository: coleam00/Archon
Length of output: 137
🏁 Script executed:
# Look for any tests of spawnWindowsTerminal
fd -t f "setup" packages/cli/src/ --exec grep -l "spawnWindowsTerminal\|trySpawn" {} \;Repository: coleam00/Archon
Length of output: 93
🏁 Script executed:
# Check the actual git commit to understand the fix for `#1035`
git show 8d91dde 2>/dev/null | head -80Repository: coleam00/Archon
Length of output: 2579
🏁 Script executed:
# Search for issue `#1035` context in comments or docs
rg "#1035" packages/Repository: coleam00/Archon
Length of output: 159
Removing shell: true will likely break path quoting for #1035; instead, detect wt.exe failure by listening to exit codes.
The issue is real: with shell: true, child.pid represents the shell process, not wt.exe. If wt.exe fails inside the shell, trySpawn() returns true anyway and the cmd.exe fallback never runs.
However, the suggested fix (removing shell: true) would revert the quoting behavior intentionally added in commit 8d91dde (issue #1035) to handle paths with spaces. The shell: true was added specifically so the shell parses the quoted path before passing it to wt.exe.
Instead, keep shell: true but detect when the command inside the shell fails. Add event listeners to ChildProcess for exit code checking, or remove stdio: 'ignore' temporarily to capture stderr when wt.exe fails. This preserves the quoting fix while properly detecting launch failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/setup.ts` around lines 1232 - 1239, The wt.exe
launch currently uses shell: true to preserve quoting (as added for `#1035`) which
causes trySpawn to report success even if wt.exe inside the shell fails; keep
shell: true but change the logic in trySpawn (and the call site that spawns
'wt.exe') to detect inner-process failures by attaching to the returned
ChildProcess: remove or adjust stdio: 'ignore' so you can listen for
'exit'/'error' and inspect the exit code and stderr, and on non-zero exit or
error fall back to spawning cmd.exe; update the spawn handling around
trySpawn('wt.exe', ...) and the ChildProcess listeners to only treat the spawn
as successful when the child exits with code 0 (or no error) so the cmd.exe
fallback runs when wt.exe fails.
… tool_results (coleam00#1037) * fix(sse): extend buffer TTL beyond reconnect grace to prevent dropped tool_results The SSE event buffer held events for only 3s, but the conversation reconnect grace period is 5s — meaning events emitted during a reconnect window could expire *before* the client even had a chance to reconnect. When a tool_result happened to land in that gap, the UI would show a perpetually spinning tool card with no recovery path. This is one of the remaining causes from the 'tool cards stuck running' investigation. The two biggest causes (Claude hook coverage and Codex tool_result emission) were already fixed in coleam00#1031 and coleam00#1032. This closes the last high-impact backend gap. Changes: - EVENT_BUFFER_TTL_MS: 3_000 → 60_000. Covers typical EventSource auto-reconnect delays on flaky networks (mobile, VPN, laptop sleep). - EVENT_BUFFER_MAX: 50 → 500. Events are small JSON strings; 500 bounds worst-case memory while giving real headroom for bursts. - Warn when buffer cap evicts oldest (previously silent). - Warn when events expire on TTL at replay time (previously silent). Both warnings give us observability if the new bounds are still ever insufficient. Note: a full Last-Event-ID resume protocol would be more principled but requires monotonic event IDs and client-side offset tracking — a larger change with its own risks. The TTL bump alone closes the vast majority of the window at near-zero cost. * fix(sse): throttle eviction warns, reset cleanup timer, enforce TTL invariant Address review feedback on the SSE buffer TTL bump: - Reset the buffer cleanup timer on each new event so the buffer is held for TTL past the most recent event, not the first one. With the 20x TTL bump this gap became meaningful — a fresh event could be wiped by a cleanup timer scheduled when the first (now-stale) event was buffered. - Throttle 'transport.buffer_evicted_oldest' warns to one per conversation per 5s. A runaway producer overflowing the cap by hundreds would otherwise flood logs. - Fail-fast at module load if EVENT_BUFFER_TTL_MS < RECONNECT_GRACE_MS. Locks in the invariant the comment already documents. - Add test covering the eviction-warn throttle.
… tool_results (coleam00#1037) * fix(sse): extend buffer TTL beyond reconnect grace to prevent dropped tool_results The SSE event buffer held events for only 3s, but the conversation reconnect grace period is 5s — meaning events emitted during a reconnect window could expire *before* the client even had a chance to reconnect. When a tool_result happened to land in that gap, the UI would show a perpetually spinning tool card with no recovery path. This is one of the remaining causes from the 'tool cards stuck running' investigation. The two biggest causes (Claude hook coverage and Codex tool_result emission) were already fixed in coleam00#1031 and coleam00#1032. This closes the last high-impact backend gap. Changes: - EVENT_BUFFER_TTL_MS: 3_000 → 60_000. Covers typical EventSource auto-reconnect delays on flaky networks (mobile, VPN, laptop sleep). - EVENT_BUFFER_MAX: 50 → 500. Events are small JSON strings; 500 bounds worst-case memory while giving real headroom for bursts. - Warn when buffer cap evicts oldest (previously silent). - Warn when events expire on TTL at replay time (previously silent). Both warnings give us observability if the new bounds are still ever insufficient. Note: a full Last-Event-ID resume protocol would be more principled but requires monotonic event IDs and client-side offset tracking — a larger change with its own risks. The TTL bump alone closes the vast majority of the window at near-zero cost. * fix(sse): throttle eviction warns, reset cleanup timer, enforce TTL invariant Address review feedback on the SSE buffer TTL bump: - Reset the buffer cleanup timer on each new event so the buffer is held for TTL past the most recent event, not the first one. With the 20x TTL bump this gap became meaningful — a fresh event could be wiped by a cleanup timer scheduled when the first (now-stale) event was buffered. - Throttle 'transport.buffer_evicted_oldest' warns to one per conversation per 5s. A runaway producer overflowing the cap by hundreds would otherwise flood logs. - Fail-fast at module load if EVENT_BUFFER_TTL_MS < RECONNECT_GRACE_MS. Locks in the invariant the comment already documents. - Add test covering the eviction-warn throttle.
|
Closing — this has been open for a week without maintainer review. Happy to reopen or resubmit if there's interest. Thanks! 🌸 |
… tool_results (coleam00#1037) * fix(sse): extend buffer TTL beyond reconnect grace to prevent dropped tool_results The SSE event buffer held events for only 3s, but the conversation reconnect grace period is 5s — meaning events emitted during a reconnect window could expire *before* the client even had a chance to reconnect. When a tool_result happened to land in that gap, the UI would show a perpetually spinning tool card with no recovery path. This is one of the remaining causes from the 'tool cards stuck running' investigation. The two biggest causes (Claude hook coverage and Codex tool_result emission) were already fixed in coleam00#1031 and coleam00#1032. This closes the last high-impact backend gap. Changes: - EVENT_BUFFER_TTL_MS: 3_000 → 60_000. Covers typical EventSource auto-reconnect delays on flaky networks (mobile, VPN, laptop sleep). - EVENT_BUFFER_MAX: 50 → 500. Events are small JSON strings; 500 bounds worst-case memory while giving real headroom for bursts. - Warn when buffer cap evicts oldest (previously silent). - Warn when events expire on TTL at replay time (previously silent). Both warnings give us observability if the new bounds are still ever insufficient. Note: a full Last-Event-ID resume protocol would be more principled but requires monotonic event IDs and client-side offset tracking — a larger change with its own risks. The TTL bump alone closes the vast majority of the window at near-zero cost. * fix(sse): throttle eviction warns, reset cleanup timer, enforce TTL invariant Address review feedback on the SSE buffer TTL bump: - Reset the buffer cleanup timer on each new event so the buffer is held for TTL past the most recent event, not the first one. With the 20x TTL bump this gap became meaningful — a fresh event could be wiped by a cleanup timer scheduled when the first (now-stale) event was buffered. - Throttle 'transport.buffer_evicted_oldest' warns to one per conversation per 5s. A runaway producer overflowing the cap by hundreds would otherwise flood logs. - Fail-fast at module load if EVENT_BUFFER_TTL_MS < RECONNECT_GRACE_MS. Locks in the invariant the comment already documents. - Add test covering the eviction-warn throttle.
1 participant
Problem
archon setup --spawnfails on Windows when the repo path contains spaces (e.g.C:\Users\...\GitHub Libraries\Archon). Both the Windows Terminal (wt.exe) andcmd.exefallback paths pass the path unquoted, causing it to be split at the first space.The user sees a Windows error dialog:
"Libraries\Archon\packages\cli" could not be found.Fix
Wrap
repoPathin double-quotes before passing towt.exeandcmd.exe:"..."quoting and switch toshell: truefor consistent shell-level quoting/Dargument sostartreceives the full pathBoth paths now handle spaces, special characters, and parentheses in directory names.
Changes
packages/cli/src/commands/setup.ts: QuoterepoPathinspawnWindowsTerminal()packages/cli/src/commands/setup.test.ts: Add test for paths with spacesValidation
bun test packages/cli/src/commands/setup.test.ts— 19 pass, 0 failbun run type-check— clean across all packagesbun run lint— cleanNote
The Linux terminal spawning functions have a similar potential issue with unquoted paths (
--working-directory=concatenation), but that is out of scope for this PR.Closes #1035
Summary by CodeRabbit
Bug Fixes
Tests