fix: resolve Windows .cmd shims to underlying .exe before spawn#68819
fix: resolve Windows .cmd shims to underlying .exe before spawn#68819HaseebDev wants to merge 7 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes Confidence Score: 5/5Safe to merge; the one finding is a minor hardening suggestion that does not affect the happy path. All platform guards, fallthrough paths, and the regex are correct. The single P2 note (bare name fallthrough to CWD-relative read) is an unlikely edge case that still degrades safely to a spawn error. The happy path is verified by 10 new unit tests and a live end-to-end run on Windows 11. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/process/windows-command.ts
Line: 36-51
Comment:
**Bare shimPath falls through to CWD-relative `readFileSync`**
When a bare `.cmd` name (no path separators) is not found anywhere in `PATH`, `shimPath` is still the original bare value (e.g. `"claude.cmd"`). The code then attempts `fs.readFileSync("claude.cmd", "utf8")`, which reads relative to the process's working directory. If a file by that name happens to exist in the CWD and matches the shim regex, an unintended target in the CWD gets spawned instead of failing cleanly.
Adding an early-return when the PATH walk ends without resolving the name to an absolute path would close the gap:
```ts
if (!shimPath.includes("\\") && !shimPath.includes("/")) {
const pathEnv = options?.pathEnv ?? process.env.PATH ?? "";
let resolved = false;
for (const dir of pathEnv.split(path.delimiter)) {
if (!dir) continue;
const candidate = path.join(dir, shimPath);
if (fs.existsSync(candidate)) {
shimPath = candidate;
resolved = true;
break;
}
}
if (!resolved) {
return result;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: resolve Windows .cmd shims to under..." | Re-trigger Greptile |
| if (!shimPath.includes("\\") && !shimPath.includes("/")) { | ||
| const pathEnv = options?.pathEnv ?? process.env.PATH ?? ""; | ||
| for (const dir of pathEnv.split(path.delimiter)) { | ||
| if (!dir) continue; | ||
| const candidate = path.join(dir, shimPath); | ||
| if (fs.existsSync(candidate)) { | ||
| shimPath = candidate; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| let shimContent: string; | ||
| try { | ||
| shimContent = fs.readFileSync(shimPath, "utf8"); | ||
| } catch { | ||
| return result; |
There was a problem hiding this comment.
readFileSync
When a bare .cmd name (no path separators) is not found anywhere in PATH, shimPath is still the original bare value (e.g. "claude.cmd"). The code then attempts fs.readFileSync("claude.cmd", "utf8"), which reads relative to the process's working directory. If a file by that name happens to exist in the CWD and matches the shim regex, an unintended target in the CWD gets spawned instead of failing cleanly.
Adding an early-return when the PATH walk ends without resolving the name to an absolute path would close the gap:
if (!shimPath.includes("\\") && !shimPath.includes("/")) {
const pathEnv = options?.pathEnv ?? process.env.PATH ?? "";
let resolved = false;
for (const dir of pathEnv.split(path.delimiter)) {
if (!dir) continue;
const candidate = path.join(dir, shimPath);
if (fs.existsSync(candidate)) {
shimPath = candidate;
resolved = true;
break;
}
}
if (!resolved) {
return result;
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/process/windows-command.ts
Line: 36-51
Comment:
**Bare shimPath falls through to CWD-relative `readFileSync`**
When a bare `.cmd` name (no path separators) is not found anywhere in `PATH`, `shimPath` is still the original bare value (e.g. `"claude.cmd"`). The code then attempts `fs.readFileSync("claude.cmd", "utf8")`, which reads relative to the process's working directory. If a file by that name happens to exist in the CWD and matches the shim regex, an unintended target in the CWD gets spawned instead of failing cleanly.
Adding an early-return when the PATH walk ends without resolving the name to an absolute path would close the gap:
```ts
if (!shimPath.includes("\\") && !shimPath.includes("/")) {
const pathEnv = options?.pathEnv ?? process.env.PATH ?? "";
let resolved = false;
for (const dir of pathEnv.split(path.delimiter)) {
if (!dir) continue;
const candidate = path.join(dir, shimPath);
if (fs.existsSync(candidate)) {
shimPath = candidate;
resolved = true;
break;
}
}
if (!resolved) {
return result;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 418e6c9392
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return resolveWindowsCommandShim({ | ||
| command, | ||
| cmdCommands: ["npm", "pnpm", "yarn", "npx"], | ||
| cmdCommands: ["npm", "pnpm", "yarn", "npx", "claude"], |
There was a problem hiding this comment.
Avoid forcing claude to .cmd unconditionally
Adding "claude" to cmdCommands rewrites every bare claude invocation to claude.cmd. This regresses Windows setups where Claude is installed as claude.exe (or another PATHEXT-resolved executable) without a .cmd shim: spawn("claude", ...) would resolve before, but now OpenClaw always attempts claude.cmd and fails with ENOENT. Keep the original command when no shim is actually present, or only append .cmd after confirming that shim can be resolved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52550f9951
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| resolvedArgv[0] = resolveCommand(resolvedArgv[0] ?? ""); | ||
| const withResolvedShim = [...params.argv]; | ||
| withResolvedShim[0] = resolveCommand(withResolvedShim[0] ?? ""); | ||
| const resolvedArgv = resolveWindowsCmdShimArgv(withResolvedShim); |
There was a problem hiding this comment.
Resolve shim paths using the child environment PATH
createChildAdapter calls resolveWindowsCmdShimArgv without passing params.env, so shim discovery falls back to process.env.PATH instead of the PATH actually used for the spawned child. If callers provide a custom env.PATH (for sandboxed toolchains, workspace-local bins, etc.), this can resolve a different command than the child would execute—or fail to resolve the intended .cmd shim at all—causing wrong-binary execution or Windows spawn failures to persist for those runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4b69d8efc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } | ||
| } | ||
| return process.env.PATH ?? ""; |
There was a problem hiding this comment.
Honor explicit PATH removal when resolving Windows shims
When createChildAdapter receives a custom env that intentionally omits PATH (or sets it to undefined), resolveEffectivePathEnv still falls back to process.env.PATH. That means resolveWindowsCmdShimArgv can resolve and rewrite argv[0] using the parent process PATH even though the spawned child will run with a different environment, leading to wrong-binary execution and bypassing PATH-based sandbox restrictions in Windows runs. If env is provided but has no PATH key, this should be treated as an empty PATH rather than inheriting the parent PATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6ba4d6680
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } else if (!ext && !hasSeparator) { | ||
| const resolved = findCmdShimOnPath(`${first}.cmd`, options?.pathEnv ?? process.env.PATH ?? ""); | ||
| if (!resolved) { |
There was a problem hiding this comment.
Preserve PATHEXT precedence for extensionless commands
The new !ext && !hasSeparator branch rewrites any bare command by scanning only for <name>.cmd on PATH, which does not match Windows resolution semantics. spawn("tool", ...) resolves by PATH directory order plus PATHEXT precedence (typically .EXE before .CMD), but this code can pick a later .cmd shim even when an earlier .exe should win, leading to wrong-binary execution. Fresh evidence in this revision is the newly added findCmdShimOnPath(${first}.cmd, ...) path for all extensionless commands.
Useful? React with 👍 / 👎.
| for (const [key, value] of Object.entries(env)) { | ||
| if (key.toUpperCase() === "PATH") { | ||
| return typeof value === "string" ? value : ""; | ||
| } |
There was a problem hiding this comment.
Continue scanning PATH variants after undefined match
resolveEffectiveWindowsPath returns immediately on the first case-insensitive PATH key, even when that key is non-string. With mixed-case duplicates like { PATH: undefined, Path: "C:\\custom\\bin" }, this returns "" and ignores the later valid path; meanwhile toStringEnv drops the undefined key and still passes Path to the child. That mismatch makes shim lookup run against the wrong PATH and can skip the .cmd rewrite, reintroducing Windows spawn failures for affected env merges.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ac77305c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (env === undefined) { | ||
| return process.env.PATH ?? ""; |
There was a problem hiding this comment.
Read parent PATH case-insensitively on Windows
When createChildAdapter is invoked without params.env (the default path), resolveWindowsPathEnv(undefined) returns only process.env.PATH. On Windows setups where the key is surfaced as Path, this can become "", so resolveWindowsCmdShimArgv cannot resolve *.cmd from PATH and leaves argv[0] as a .cmd wrapper, bringing back the same Windows spawn failures this change is meant to avoid. The parent-environment fallback should perform the same case-insensitive PATH lookup used for explicit env objects (or check both PATH and Path).
Useful? React with 👍 / 👎.
|
Related work from PRtags group Title: Open PR duplicate: Windows claude cmd shim spawning
|
|
Codex review: found issues before merge. Summary Reproducibility: yes. The reporter supplied Windows 11 steps and logs, and current main still routes Next step before merge Security Review findings
Review detailsBest possible solution: Land a rebased process fix that resolves npm-style Windows Do we have a high-confidence way to reproduce the issue? Yes. The reporter supplied Windows 11 steps and logs, and current main still routes Is this the best way to solve the issue? No, not as written. Shim unwrapping is the right direction, but the implementation should first match Node/libuv command selection, add the required changelog entry, and be validated on a current-main merge result. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 89f73a5ef2b0. |
Summary
provider=claude-clispawn fails at the supervisor withspawn claude ENOENT; thefallback chain cascades and the user sees
Embedded agent failed before reply.claude-cliis broken..cmdshims are now resolved to their underlying.exe(ornode.exe <cli.js>) and spawneddirectly, bypassing Node's CVE-2024-27980 restriction on
.cmdandcmd.exe's argument-escaping limits."claude"added to the supervisor'scmdCommandsallowlist.resolveChildProcessInvocationinexec.tsis untouched, and shims with non-npm-bin bodies fall through to the original spawn path.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
"claude"was missing from the supervisor'scmdCommandsallowlist, soresolveWindowsCommandShimreturned"claude"unchanged andspawn("claude", …)failed ENOENT. Adding"claude"then surfaces a second bug: Node >=18.20.2 rejectsspawn("claude.cmd", …)with EINVAL(CVE-2024-27980), and wrapping via
cmd.exe /d /s /cfails because system prompts contain\r \n & | < > ^ %which
cmd.execannot parse.npm.cmdshims through the supervisor's childadapter on Windows.
resolveNpmArgvForWindowsinsrc/process/exec.tsalready solves the.cmd-vs-CVE problem fornpm/npx, but the supervisor'screateChildAdapterdoes not route throughresolveChildProcessInvocation, so it did not inherit that protection.Regression Test Plan (if applicable)
src/process/windows-command.test.tsresolveWindowsCmdShimArgvrewritesargv[0]from a.cmdshim to itsunderlying
.exe, prependsprocess.execPathfor.jstargets, performs PATH lookup for bare names, andleaves
argvunchanged on non-Windows / missing file / non-matching shim body.before spawn, so unit-testing the resolver covers it without a full Windows spawn harness.
User-visible / Behavior Changes
claude-cli-backed providers now spawn successfully on Windows where they previously failed withspawn claude ENOENT.Security Impact (required)
path source is unchanged — it comes from OpenClaw's own resolveCommand output and process.env.PATH, same as
before. The regex matches only the narrow npm-bin shim pattern ("%dp0%…target.(exe|js)" %*); any non-matching
shim falls through and the original argv is spawned as-is. No new untrusted input is introduced.
Repro + Verification
Environment
Steps
Expected
Actual
[process/supervisor] spawn failed: reason=Error: spawn claude ENOENT
[model-fallback/decision] decision=candidate_failed ... detail=spawn claude ENOENT
(repeats across opus-4-7, sonnet-4-6, opus-4-6, opus-4-5, sonnet-4-5, haiku-4-5)
Embedded agent failed before reply: spawn claude ENOENT
Evidence
After pnpm test src/process/windows-command.test.ts: 10 passed. After live Discord run: claude.exe spawns with
an absolute path, bot replies normally, no supervisor errors.
Human Verification (required)
pnpm build; bundled dist wires the new helper through correctly.
file and non-matching shim body both fall through unchanged.
fall through unchanged but aren't exercised.
Review Conversations
Compatibility / Migration
Risks and Mitigations
sets, chained commands) would be bypassed in favor of the detected target.
scripts with control flow won't match and fall through untouched.