Skip to content

fix(setup): quote repo path in --spawn terminal commands (fixes #1035) #1037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kagura-agent wants to merge 1 commit into from
Closed

fix(setup): quote repo path in --spawn terminal commands (fixes #1035) #1037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/cli/src/commands/setup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,17 @@ CODEX_ACCOUNT_ID=account1
// Just verify the function is exported and callable
expect(typeof spawnTerminalWithSetup).toBe('function');
});

it('should quote paths with spaces for Windows terminal spawning', () => {
// Verify the function handles paths with spaces without throwing
// On non-Windows platforms, this just tests that the fallback path works
const pathWithSpaces = join(tmpdir(), 'path with spaces', 'Archon');
const result = spawnTerminalWithSetup(pathWithSpaces);
// On CI/Linux, all terminal spawns will fail (no wt.exe, no gnome-terminal, etc.)
// That's expected — we just verify it doesn't crash and returns a proper result
expect(result).toHaveProperty('success');
expect(typeof result.success).toBe('boolean');
});
});

describe('copyArchonSkill', () => {
Expand Down
13 changes: 11 additions & 2 deletions packages/cli/src/commands/setup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,19 +1223,28 @@ function trySpawn(
* Tries: Windows Terminal -> cmd.exe with start
*/
function spawnWindowsTerminal(repoPath: string): SpawnResult {
// Quote the path for Windows shell expansion — paths like
// "C:\Users\...\GitHub Libraries\Archon" must be double-quoted
// to survive both wt.exe and cmd.exe argument parsing (#1035).
const quoted = `"${repoPath}"`;

// Try Windows Terminal first (modern Windows 10/11)
// 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,
})
Comment on lines +1232 to 1239
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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:

🏁 Script executed:

# Find the trySpawn function definition
rg -n "function trySpawn|const trySpawn|export.*trySpawn" packages/cli/src/commands/setup.ts

Repository: coleam00/Archon

Length of output: 82

🏁 Script executed:

# Get full context around lines 1232-1239
sed -n '1220,1250p' packages/cli/src/commands/setup.ts

Repository: 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.ts

Repository: 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 -40

Repository: 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 -20

Repository: 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 -80

Repository: 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.

) {
return { success: true };
}

// Fallback to cmd.exe with start command (works on all Windows)
// /D requires a quoted path when it contains spaces.
if (
trySpawn('cmd.exe', ['/c', 'start', '""', '/D', repoPath, 'cmd', '/k', 'archon setup'], {
trySpawn('cmd.exe', ['/c', 'start', '""', '/D', quoted, 'cmd', '/k', 'archon setup'], {
detached: true,
stdio: 'ignore',
shell: true,
Expand Down