Fix Windows npm-shim claude spawn + stop leaking env in spawn errors (#63)

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),

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 `<dir>\\<name><ext>` 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"

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