fix(terminal): run unsplit command lines through the system shell [AI-assisted]#313
Open
xdjyxu wants to merge 1 commit into
Open
fix(terminal): run unsplit command lines through the system shell [AI-assisted]#313xdjyxu wants to merge 1 commit into
xdjyxu wants to merge 1 commit into
Conversation
When an ACP agent sends terminal/create with the entire shell command line in the `command` field and no `args` (the request shape used by Claude Code, codebuddy, and other agents that don't pre-split their Bash tool input), Node's `spawn()` treats the whole string as an executable name and fails with `spawn ... ENOENT`. Other ACP clients — notably Zed via its ShellBuilder — handle this by always running the request through the system shell, so those agents work out of the box. Match the same de facto behavior: when `args` is empty, route the command through `/bin/sh -c <command>` on Unix or `cmd.exe /d /s /c <command>` on Windows. When `args` is non-empty, honor the literal ACP contract and spawn the executable with the provided argv unchanged. This keeps well-behaved agents on the existing direct-spawn fast path while letting raw-shell-line agents work transparently. The Windows .bat/.cmd auto-shell path in buildSpawnCommandOptions is unaffected: it still triggers when an agent legitimately passes a batch file as `command` plus a non-empty argv.
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.
Problem
When an ACP agent sends
terminal/createwith the entire shell command line in thecommandfield and noargs, acpx fails with:This is because
terminal-manager.tsdoes:so the whole string
\"ls -la /Users/foo/\"is treated as an executable name.This affects multiple real-world agents that don't pre-split their Bash tool input, including:
mcp-server.ts-based path before commitbe618f5d):https://github.com/agentclientprotocol/claude-agent-acp/blob/main/src/tools.ts
@tencent-ai/codebuddy-codeACP integration)Why other ACP clients aren't affected
Zed always wraps the request through its
ShellBuilderregardless ofargsshape:https://github.com/zed-industries/zed/blob/main/crates/acp_thread/src/acp_thread.rs (search for
ShellBuilder::new(&Shell::Program(shell), is_windows)nearcreate_terminal_entity)So those agents "just work" against Zed and never hit this code path. acpx's stricter literal interpretation of the schema is the outlier in practice.
Fix
Introduce
buildShellExec(command, args, platform)insrc/spawn-command-options.ts:argsnon-empty → return{command, args}unchanged. Honor the literal ACP contract; well-behaved agents stay on the existing direct-spawn fast path with zero behavior change.argsempty/missing → return{command: \"/bin/sh\", args: [\"-c\", command]}on Unix, or{command: \"cmd.exe\", args: [\"/d\", \"/s\", \"/c\", command]}on Windows.terminal-manager.tscallsbuildShellExecbeforespawn(). The existing Windows.bat/.cmdauto-shell path inbuildSpawnCommandOptionsis unaffected — it still triggers for the legitimate "command is a batch file, args is the argv" case.Why
/bin/shnotbash -lc~/.zshrc,~/.bash_profile); environment should come fromparams.env, not the operator's dotfiles./bin/shis universally available;bashmay not be in containers.Shell::Systemchoice in spirit.Why "always wrap when args empty" rather than "detect metacharacters"
A heuristic like "only wrap if
commandcontains|&&;>*…" would misfire on the most common case in the wild —\"ls -la /path\", which is plain whitespace with no metacharacters and would still ENOENT. Routing all empty-args requests through the shell is simpler, has no false negatives, and at most adds one shell fork for bare commands like\"ls\".Permission prompt
Unaffected.
toCommandLine(params.command, params.args)still computes the user-facing display string from the original request, so the prompt shows\"ls -la /Users/foo/\"not\"/bin/sh -c ls -la /Users/foo/\".Tests
test/spawn-options.test.ts: 4 new cases forbuildShellExec(non-empty args passthrough, Unix wrapping, Windows wrapping, bare command).test/terminal.test.ts: 2 new end-to-end cases — one for\"echo hello-from-shell\"(whitespace only), one for a pipeline (printf … | wc -l). Both skip on Windows where the POSIX-shell path doesn't apply.pnpm test→# pass 608 # fail 0.pnpm typecheck,pnpm lint, andpnpm format:check(on the changed files) all clean.Compatibility
command: \"ls\", args: [\"-la\"]): unchanged.args.length > 0short-circuits to direct spawn.command: \"ls -la /path\", no args): now work via/bin/sh -c.command: \"npx\", args: [\"create-foo\"]): unchanged.buildSpawnCommandOptionsstill detects the resolved.cmd/.batand addsshell: true.The only theoretical break is an agent that intentionally sends
command: \"/path/with spaces/exe\"with empty args expecting direct spawn. That's already an awkward request shape under the schema (argsexists for exactly this kind of thing) and I haven't found such a caller in the wild.AI-assisted disclosure
pnpm testsuite passes locally on macOS arm64. Have not exercised the Windowscmd.exe /d /s /cpath on a real Windows machine, but the unit test covers the argv shape.The session that produced this PR was a back-and-forth investigation: dumping ACP history JSONL to see the actual failing requests, comparing claude-code-acp's
command: input.commandpattern with Zed'sShellBuilderwrap, then reading the acpx terminal-manager + spawn-command-options to design the fix.