fix(runtime): detect agent CLIs on Windows#31
Conversation
lefarcen
added
size/S
|
Heads-up: there are already a few open PRs in the same Windows agent-runtime area — #29, #10, and #3 all touch This PR looks narrower in scope than those threads, so sharing the overlap here early to help reviewers compare approaches and avoid duplicate effort. |
|
Thanks for the heads-up. I checked the overlap. This PR is intentionally narrower than #29/#10/#3: it only fixes Windows PATH resolution for agent CLIs and spawning npm Happy for maintainers to either land this as the minimal runtime fix or fold the same approach into the broader Windows runtime PR they prefer. |
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
Thanks for narrowing this to the Windows agent launch path. I found one blocker in the new child-process branch: the code now resolves npm-generated .cmd/.bat shims, but it still spawns them directly, so the fix does not yet make those Windows shims runnable.
| const bin = resolveCommandSync(def.bin) ?? def.bin; | ||
|
|
||
| const child = cpSpawn(def.bin, args, { | ||
| const child = cpSpawn(bin, args, { |
There was a problem hiding this comment.
Resolving def.bin to the absolute .cmd/.bat shim here is not enough to make Windows launches work, because the next line still passes that batch file straight to child_process.spawn() without shell: true or an explicit cmd.exe /c wrapper. Node documents batch files as requiring one of those wrappers on Windows, so this path will still fail for the npm-generated shims this PR is trying to support even though detection now reports them as available.
Please branch on Windows batch extensions here and spawn cmd.exe (or enable shell: true only for that case), then add a regression test that covers launching a .cmd shim through spawnAgent.
There was a problem hiding this comment.
Addressed in 75865ea. Windows .cmd/.bat shims now go through cmd.exe /d /c call <shim> ... with windowsVerbatimArguments so Node does not escape the command string before cmd.exe sees it. I also added a Windows-only regression test that creates a temporary .cmd shim and launches it through spawnAgent.
Verified locally:
pnpm --filter @html-video/runtime typecheckpnpm --filter @html-video/runtime test(includes the new.cmdshim test)pnpm --filter @html-video/cli typecheck
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
Thanks for fixing the direct .cmd/.bat spawn path and adding the Windows shim regression test. I found one remaining Windows edge case in the new cmd.exe wrapper: argv values can still be rewritten by cmd before the shim receives them.
| } | ||
|
|
||
| const comspec = process.env.ComSpec || 'cmd.exe'; | ||
| const commandLine = ['call', quoteCmdArg(bin), ...args.map(quoteCmdArg)].join(' '); |
There was a problem hiding this comment.
quoteCmdArg() only doubles embedded quotes before building the cmd.exe /c command line. On Windows, cmd still expands %VAR% inside double quotes, so this wrapper can rewrite argv before the batch shim sees it. That matters because this runtime already has argv-prompt agents such as aider, hermes, and grok; if any of them are installed behind a .cmd shim, prompts like 100% done or %USERPROFILE% will arrive mutated even though the new stdin-based shim test passes. Please escape % (and ! if you want to be safe against delayed expansion) for cmd semantics, or switch the batch-shim launch path to one that preserves argv without shell expansion, and add a Windows regression that launches a .cmd shim with a % in an argv argument.
There was a problem hiding this comment.
Addressed in 9eef524. Rather than trying to make % escaping through cmd.exe/call perfect, npm-generated .cmd shims are now detected and resolved to their underlying Node entrypoint (%dp0%\node_modules\... %*). spawnAgent then launches node <entry> ...args directly, so argv values do not pass through cmd expansion at all.
The cmd.exe wrapper remains only as a fallback for non-npm .cmd/.bat files.
I also added a Windows regression for an npm-style .cmd shim with argv values including %USERPROFILE%, 100% done, and bang!.
Verified locally:
pnpm --filter @html-video/runtime typecheckpnpm --filter @html-video/runtime testpnpm --filter @html-video/cli typecheck
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
Thanks for iterating on the Windows shim path. I found one remaining blocker in the new npm-shim fast path: the parser only matches the synthetic %dp0% form, so real npm-generated .cmd shims still fall back to cmd.exe and keep the %...% argv rewriting bug.
| return null; | ||
| } | ||
|
|
||
| const match = text.match(/"%dp0%\\node_modules\\([^"]+)"\s+%\*/i); |
There was a problem hiding this comment.
The new npm-shim detection only matches "%dp0%\node_modules\..." %*, but npm-generated Windows shims use %~dp0 here instead. For example, the stock shims shipped with npm resolve their entrypoints through %~dp0\... %*, so this regex never matches the real launcher format. That means commandForNpmShim() returns null, we fall back to the cmd.exe /c call ... branch, and argv values containing %...% are still rewritten before argv-based agents see them. Please parse the actual %~dp0/%~dp0\.. shim forms that npm emits and update the regression test fixture to match a real generated .cmd file so this direct-node path is exercised for production shims.
There was a problem hiding this comment.
Addressed in 143cda6. The parser now handles real npm shim forms instead of only the synthetic %dp0% fixture:
- direct
%~dp0\...node_modules\... %*entries - parent path variants such as
%~dp0\..\node_modules\... %* - npm-generated
SET dp0=%~dp0followed by%dp0%\node_modules\... %*
I also updated the regression fixture to match npm's actual generated .cmd structure (GOTO start, SET dp0=%~dp0, CALL :find_dp0, final %dp0%\node_modules\... %*) and added a second %~dp0\.. fixture. Both verify %USERPROFILE% / 100% done argv values are preserved by the direct-node path.
Verified locally:
pnpm --filter @html-video/runtime typecheckpnpm --filter @html-video/runtime testpnpm --filter @html-video/cli typecheck- smoke: real
F:\npm-global\codex.cmdlaunches throughspawnAgentand returnscodex-cli 0.137.0
nettee
left a comment
nettee
left a comment
There was a problem hiding this comment.
@xuejianrpa I re-checked the final Windows runtime path on this head. The updated detection logic now resolves PATHEXT-aware executables on Windows, and the spawn path now unwraps real npm-generated .cmd shims into direct node <entry> launches so %...% argv values no longer pass through cmd.exe expansion on the supported agent CLIs this PR targets. The added regression fixtures also cover both %~dp0 and SET dp0=%~dp0 shim forms, which closes the production gap from the earlier review rounds.
I could not get the local pnpm --filter @html-video/runtime test / typecheck flow to complete in this reviewer worktree because the installed TypeScript is older than the repo's tsconfig options, but that environment mismatch is broader than this PR and did not surface a code issue in the changed ranges. Nice iteration on narrowing the Windows fix without expanding scope.
3 participants
Summary
where.exe/PATHEXTinstead of Unix-onlywhich.cmd/.batshims such ascodex,claude,gemini, andopencodework withoutshell: truewhichpathVerification
pnpm --filter @html-video/runtime typecheckpnpm --filter @html-video/runtime testpnpm --filter @html-video/cli typecheckcodex,claude,gemini,opencode, andcopilotare detected as available via their.cmd/.batshimsNotes
While testing the local Studio, I also hit an existing
ERR_HTTP_HEADERS_SENTcrash from an in-flight Studio message request. That appears unrelated to this runtime PATH fix and is intentionally not included here.