fix: bypass Windows cmd shim length limit#518
Conversation
Greptile SummaryThis PR resolves a Windows
Confidence Score: 5/5Safe to merge; the shim-resolution logic is well-structured and the fallback chain to cmd.exe is preserved for all unrecognised shim formats. The core bypass logic is correct: shim patterns are checked in the right priority order, all three branches verify the binary exists before returning, and both callers correctly build cmd-shell args when the fallback path is taken. Findings are limited to an inconsistency in how the node-script paths handle a missing script file vs the exe path, and a test-organisation nit. src/core/windows-cmd-launch.ts — the
|
| Filename | Overview |
|---|---|
| src/core/windows-cmd-launch.ts | Core Windows launch resolution logic — adds shim-reading, three pattern-based shim branches, and a unified resolveWindowsSpawnLaunch entry point; directExecutableMatch now guards with canAccessPath (previous reviewer fix addressed), but the two node-script branches do not verify the script path exists before returning. |
| src/commands/hooks.ts | Switches buildCodexWrapperSpawn to use the unified resolveWindowsSpawnLaunch; correctly continues to call buildWindowsCmdArgsArray(realBinary, childArgs) when useCmdShell: true rather than relying on resolvedLaunch.args. |
| src/terminal/pty-session.ts | PTY spawn path updated to use resolveWindowsSpawnLaunch; correctly falls through to buildWindowsCmdArgsCommandLine when useCmdShell: true, preserving existing cmd-shell wrapping for PTY. |
| test/runtime/core/windows-cmd-launch.test.ts | Five new test cases for shim resolution and fallback logic, but all new tests are placed inside the pre-existing describe("shouldUseWindowsCmdLaunch") block even though they exercise resolveWindowsBatchShimLaunch and resolveWindowsSpawnLaunch. |
| test/runtime/hooks-codex-wrapper.test.ts | Adds a Windows cmd-shim integration test for buildCodexWrapperSpawn; uses try/finally for cleanup and correctly creates node.exe and the JS entrypoint on disk. |
| test/runtime/terminal/pty-session.test.ts | Existing Windows tests updated to set PATH/PATHEXT explicitly; new test verifies PTY launches node directly instead of wrapping in cmd.exe. |
| AGENTS.md | Documents the Windows cmd.exe command-line limit trap and the preferred shim-resolution approach. |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/core/windows-cmd-launch.ts:193-204
The `indirectNodeMatch` branch returns a resolved launch without verifying the script file exists, unlike `directExecutableMatch` (where `canAccessPath` was added after the previous review). If the script is missing — e.g., a half-uninstalled package where only the shim remains — the function returns a non-null result pointing at a path node cannot find, bypassing the cmd.exe fallback entirely. The same gap exists in `directNodeMatch`.
```suggestion
const indirectNodeMatch = shimContent.match(WINDOWS_BATCH_INDIRECT_NODE_PATTERN);
if (indirectNodeMatch) {
const nodeBinary = resolveWindowsBatchShimNodeBinary(resolvedBinaryPath, env);
if (!nodeBinary) {
return null;
}
const scriptPath = resolveWindowsBatchShimPath(indirectNodeMatch[1], resolvedBinaryPath);
if (!canAccessPath(scriptPath)) {
return null;
}
return {
binary: nodeBinary,
args: [scriptPath, ...args],
};
}
```
### Issue 2 of 3
src/core/windows-cmd-launch.ts:219-226
The `directNodeMatch` branch also doesn't verify `scriptPath` exists before returning, mirroring the inconsistency noted above. A shim with a valid node binary but a missing/moved JS entrypoint will resolve successfully here and then fail at node startup, with no fallback to cmd.exe.
```suggestion
if (!nodeBinary || !canAccessPath(nodeBinary) || !canAccessPath(scriptPath)) {
return null;
}
return {
binary: nodeBinary,
args: [scriptPath, ...args],
};
}
```
### Issue 3 of 3
test/runtime/core/windows-cmd-launch.test.ts:103
**New tests grouped under wrong describe block.** The five new test cases starting at this line exercise `resolveWindowsBatchShimLaunch` and `resolveWindowsSpawnLaunch`, not `shouldUseWindowsCmdLaunch`. They should live in their own `describe` blocks (e.g., `describe("resolveWindowsBatchShimLaunch")` / `describe("resolveWindowsSpawnLaunch")`), otherwise the test output groups the wrong subjects together and future grep-by-function searches miss them.
Reviews (2): Last reviewed commit: "fix(terminal): harden Windows shim fallb..." | Re-trigger Greptile
|
@greptileai please re-review |
Closes #83.
Summary
Testing
px tsc --noEmit still fails on pre-existing unrelated src/cline-sdk/* type errors in this checkout