diff --git a/src/agent_backend.jl b/src/agent_backend.jl index 6efedf1..d07b217 100644 --- a/src/agent_backend.jl +++ b/src/agent_backend.jl @@ -74,6 +74,32 @@ end _spawn_argv(argv::Vector{String}) = Utils.launch_argv(argv) _spawn_argv(argv::Vector{String}, iswin::Bool) = Utils.launch_argv(argv; iswin = iswin) +# A failed process spawn raises an `IOError` whose message stringifies the WHOLE +# `setenv(cmd, env)` — the process environment included, which may hold API keys +# (ANTHROPIC_API_KEY, …). Surfacing `showerror(e)` on it (e.g. into the agent UI, or an +# error a user pastes into an issue) leaks those secrets. Build a clean, actionable message +# from the argv alone (safe — no env) and drop the original error entirely. `iswin` is +# injectable so both branches are testable off-Windows. +function _agent_spawn_error(e, args::Vector{String}; iswin::Bool = Sys.iswindows()) + cli = isempty(args) ? "the agent CLI" : "`$(args[1])`" + notfound = e isa Base.IOError && e.code == Base.UV_ENOENT + if notfound && iswin + return ErrorException( + "Could not launch $cli. If it was installed via npm it exists only as a " * + "`.cmd`/`.ps1` shim, which may not be resolvable on this process's PATH. Install " * + "the native CLI so a real executable is on PATH (Claude Code: " * + "`irm https://claude.ai/install.ps1 | iex`), then retry.") + elseif notfound + return ErrorException( + "Could not launch $cli — not found on PATH. Install the CLI and ensure it is on PATH.") + else + # Any other spawn failure: report the libuv error CODE only, never the message + # (which carries the command + environment). + code = e isa Base.IOError ? " (error $(e.code))" : "" + return ErrorException("Could not launch $cli$code. Check that it is installed and executable.") + end +end + mutable struct ClaudeHandle <: AgentHandle backend::ClaudeBackend proc::Base.Process @@ -148,7 +174,13 @@ function backend_start(b::ClaudeBackend; cwd::String, agent_id::String, log_io = open(log_file, "a") cmd = setenv(Cmd(_spawn_argv(args)), env; dir = cwd) # wrap a Windows .cmd/.ps1 shim in its interpreter - proc = open(pipeline(cmd; stderr = log_io), "r+") # proc.in = write, proc.out = read + proc = try + open(pipeline(cmd; stderr = log_io), "r+") # proc.in = write, proc.out = read + catch e + try; close(log_io); catch; end + # A spawn IOError stringifies `env` (may hold API keys) — re-raise a sanitized error. + throw(_agent_spawn_error(e, args)) + end events = Channel{ACP.AgentEvent}(Inf) h = ClaudeHandle(b, proc, proc.in, proc.out, events, Task(() -> nothing), diff --git a/src/utils.jl b/src/utils.jl index 4a513bf..373bc80 100644 --- a/src/utils.jl +++ b/src/utils.jl @@ -62,26 +62,62 @@ end export terminate_process +""" + _which_pathext(name; path, pathext, exists) -> String | nothing + +Resolve a bare command `name` across `PATH` × `PATHEXT` (Windows), returning the first +existing `\\` in PATHEXT order, or `nothing`. This fills the gap where +`Sys.which` resolves a bare name only as `.exe` on Windows — so an npm-installed CLI that +ships `.cmd`/`.ps1` shims (Claude Code, gemini, …) is invisible to `Sys.which` and thus to +`Base.run`, which then ENOENTs. `path`/`pathext`/`exists` are injectable so the search is +unit-testable off-Windows. A `name` that already contains a path separator returns `nothing` +(it isn't a bare name to resolve). +""" +function _which_pathext(name::AbstractString; + path = get(ENV, "PATH", ""), + pathext = get(ENV, "PATHEXT", ".COM;.EXE;.BAT;.CMD"), + exists = isfile) + (occursin('/', name) || occursin('\\', name)) && return nothing + exts = [lowercase(e) for e in split(pathext, ';'; keepempty = false)] + for dir in split(path, ';'; keepempty = false) + isempty(strip(dir)) && continue + for e in exts + cand = joinpath(String(dir), name * e) + exists(cand) && return cand + end + end + return nothing +end + """ launch_argv(argv; iswin=Sys.iswindows(), which=Sys.which) -> Vector{String} Return an argv that will actually execute on this platform. `argv[1]` is the executable -(a bare name or a path). On Windows a bare name is resolved via `PATHEXT` (`Sys.which`), and -a resulting `.cmd`/`.bat`/`.ps1` shim — which `CreateProcess` (what `run`/libuv use) cannot -execute directly — is launched through its interpreter (`cmd.exe /d /c`, or -`powershell -File`). A native `.exe`, or any non-Windows target, is returned unchanged apart -from name→path resolution. `iswin`/`which` are injectable so the rewrite is unit-testable -off-Windows. +(a bare name or a path). On Windows a bare name is resolved first via `Sys.which` (which only +finds `.exe`), then via a `PATH` × `PATHEXT` search (`_which_pathext`) so npm `.cmd`/`.ps1` +shims are found too; a resulting `.cmd`/`.bat`/`.ps1` shim — which `CreateProcess` (what +`run`/libuv use) cannot execute directly — is launched through its interpreter (`cmd.exe /d +/c`, or `powershell -File`). A native `.exe`, or any non-Windows target, is returned unchanged +apart from name→path resolution. `iswin`/`which`/`which_ext` are injectable so the rewrite is +unit-testable off-Windows. Known edge: `cmd.exe` re-parses the command line, so an argv VALUE containing cmd metacharacters (`% ! & | < >`) can mis-quote — rare for CLI flag values. """ function launch_argv(argv::AbstractVector{<:AbstractString}; - iswin::Bool = Sys.iswindows(), which = Sys.which) + iswin::Bool = Sys.iswindows(), which = Sys.which, + which_ext = _which_pathext) a = String[String(x) for x in argv] (iswin && !isempty(a)) || return a exe = a[1] - resolved = isfile(exe) ? exe : something(which(exe), exe) + # `Sys.which` resolves a bare name only as `.exe` on Windows, so npm `.cmd`/`.ps1` shims + # slip through — fall back to a PATH × PATHEXT search so they're found and can be wrapped. + resolved = if isfile(exe) + exe + else + w = which(exe) + w === nothing ? something(which_ext(exe), exe) : w + end rest = @view a[2:end] ext = lowercase(splitext(resolved)[2]) if ext == ".cmd" || ext == ".bat" diff --git a/test/agent_tests.jl b/test/agent_tests.jl index 82bdb94..3b294e8 100644 --- a/test/agent_tests.jl +++ b/test/agent_tests.jl @@ -236,6 +236,37 @@ end @test Kaimon._spawn_argv(String[], true) == String[] # empty argv is a no-op end + @testset "agent spawn error is sanitized (never leaks the environment)" begin + # A real failed spawn raises an IOError whose text embeds `setenv(cmd, env)` — the + # whole process environment, API keys included. The surfaced error must not contain it. + leaky = Base.IOError( + "could not spawn setenv(`claude -p`, [\"ANTHROPIC_API_KEY=sk-secret-XYZ\", " * + "\"PATH=/x\"]): no such file or directory (ENOENT)", Base.UV_ENOENT) + + for win in (false, true) + err = Kaimon._agent_spawn_error(leaky, ["claude", "-p"]; iswin = win) + @test err isa ErrorException + msg = sprint(showerror, err) + @test occursin("claude", msg) # names the CLI that failed + @test !occursin("sk-secret-XYZ", msg) # never the secret value + @test !occursin("ANTHROPIC_API_KEY", msg) # nor any env at all + end + + # Windows ENOENT points the user at the npm-shim / native-install remedy. + win_msg = sprint(showerror, Kaimon._agent_spawn_error(leaky, ["claude"]; iswin = true)) + @test occursin("install", lowercase(win_msg)) + # Non-Windows ENOENT gives a PATH hint, not the Windows shim spiel. + nix_msg = sprint(showerror, Kaimon._agent_spawn_error(leaky, ["claude"]; iswin = false)) + @test occursin("PATH", nix_msg) + + # A non-ENOENT spawn failure still never leaks the env. + other = Base.IOError( + "could not spawn setenv(`claude`, [\"ANTHROPIC_API_KEY=sk-x\"])", Base.UV_EACCES) + om = sprint(showerror, Kaimon._agent_spawn_error(other, ["claude"]; iswin = false)) + @test !occursin("sk-x", om) + @test occursin("claude", om) + end + @testset "Utils.launch_argv resolves bare CLI names via which" begin U = Kaimon.Utils # Bare name → resolved by `which` to a .cmd shim → launched through cmd.exe. @@ -253,6 +284,38 @@ end # Non-Windows → never touched, regardless of what which would return. @test U.launch_argv(["claude", "x"]; iswin = false, which = _ -> "C:\\claude.cmd") == ["claude", "x"] + + # Sys.which misses the shim (only finds .exe on Windows) → PATHEXT fallback resolves it + # and the .cmd is still wrapped. This is the real npm-shim case (Sys.which("claude")=nothing). + @test U.launch_argv(["claude", "mcp"]; iswin = true, which = _ -> nothing, + which_ext = _ -> "C:\\npm\\claude.cmd") == + ["cmd.exe", "/d", "/c", "C:\\npm\\claude.cmd", "mcp"] + # Neither which nor the PATHEXT search finds it → bare name kept (handled by the + # sanitized spawn error downstream). + @test U.launch_argv(["claude"]; iswin = true, which = _ -> nothing, + which_ext = _ -> nothing) == ["claude"] + end + + @testset "Utils._which_pathext searches PATH × PATHEXT (the Sys.which .exe-only gap)" begin + U = Kaimon.Utils + pathext = ".COM;.EXE;.BAT;.CMD" + path = "C:\\a;C:\\npm" + # Build expected paths the same way the function does (`joinpath`), so the test is + # host-agnostic (macOS joins with `/`, Windows with `\`). + claude_cmd = joinpath("C:\\npm", "claude.cmd") + @test U._which_pathext("claude"; path = path, pathext = pathext, + exists = p -> p == claude_cmd) == claude_cmd # Sys.which would return nothing here + # PATHEXT order wins: prefer .EXE over .CMD when both exist in the same dir. + tool_exe = joinpath("C:\\a", "tool.exe") + tool_cmd = joinpath("C:\\a", "tool.cmd") + @test U._which_pathext("tool"; path = path, pathext = pathext, + exists = p -> p in (tool_exe, tool_cmd)) == tool_exe + # Nothing on PATH → nothing. + @test U._which_pathext("ghost"; path = path, pathext = pathext, + exists = _ -> false) === nothing + # A name that's already a path is not a bare name → nothing (caller handles it). + @test U._which_pathext("C:\\x\\claude"; path = path, pathext = pathext, + exists = _ -> true) === nothing end @testset "image downscale (tool-result PNG)" begin