Fix Windows CLI shim execution#445
Conversation
|
Opened the matching bug report as #446 for the ugig bug-fix trail. This PR fixes the Windows .cmd shim execution issue. |
Greptile SummaryThis PR fixes Windows CLI shim execution by routing bare command names through
Confidence Score: 3/5The cmd.exe routing strategy is architecturally correct, but windowsEnvArg corrupts arguments containing ! characters before they reach the child process on Windows. The delayed-expansion approach of stashing arguments in numbered env vars is the right design for avoiding cmd.exe injection. However, windowsEnvArg adds ^! for every ! in a value: since cmd.exe does not re-scan substituted !NAME! values, the ^ is not consumed as an escape and passes through CommandLineToArgvW as a literal character. The roundtrip test includes hello!world and will fail on Windows as written. packages/core/src/exec.ts — specifically the windowsEnvArg function's !-escaping step. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["exec(cmd, args, opts)"] --> B{shouldUseWindowsCmd?}
B -- "win32 AND no path sep AND not .exe/.com" --> C["windowsCommandLine(cmd, args, env)"]
C --> D["Store each arg as SH1PT_EXEC_ARG_N in env via windowsEnvArg"]
D --> E["Build command string with !NAME! references"]
E --> F["spawn cmd.exe /d /s /v:on /c command"]
B -- "non-Windows OR has path sep OR .exe/.com" --> G["spawn(cmd, args, spawnOptions)"]
F --> H["Collect stdout/stderr"]
G --> H
H --> I["resolve ExecResult"]
I --> J{ensureCli caller?}
J -- yes --> K{isWindowsCommandNotFound?}
K -- "win32 AND exitCode=9009 OR not-recognized text" --> L["throw command not found"]
K -- otherwise --> M["CLI considered present"]
Reviews (8): Last reviewed commit: "Preserve Windows shim metachar args" | Re-trigger Greptile |
|
Fixed the Greptile review items in 99b9eee: restored stdin to ignore via explicit stdio, kept stdout/stderr piped, and hardened Windows shell quoting for trailing backslashes before quotes. Re-ran packages/docs/pandoc/src/index.test.ts (5 passed), @profullstack/sh1pt-core typecheck, and @profullstack/sh1pt-docs-pandoc typecheck. |
|
Fixed the latest Greptile finding in commit bb071a9: �nsureCli now treats non-zero --version exits as missing, with coverage for the Windows 9009-style case. I also limited Windows shell dispatch to bare commands so absolute .exe paths avoid shell quoting/percent-expansion concerns. Re-ran packages/core/src/exec.test.ts + packages/docs/pandoc/src/index.test.ts (7 passed), plus core and docs-pandoc typechecks. |
|
Fixed the latest Windows argument-handling concern in e8b8dea: bare Windows commands now run through cmd.exe /d /s /c with the command arguments passed separately, so we no longer build one custom-quoted command string that can corrupt percent-wrapped args or trailing backslashes. I also updated the regression test so it exercises a bare .cmd command on PATH and verifies both %SH1PT_EXEC_LITERAL% and a trailing-backslash path survive.\n\nVerification run locally:\n- npx --yes pnpm@9.12.0 exec vitest run packages/core/src/exec.test.ts packages/docs/pandoc/src/index.test.ts -> 7 passed\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-core typecheck -> passed\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-docs-pandoc typecheck -> passed |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
e8b8dea to
ec35ac5
Compare
|
Fixed the latest ensureCli review item in ec35ac5 after rebasing onto current upstream master. ensureCli now treats non-zero --version as missing only for Windows command-not-found signatures (9009 or cmd.exe's 'is not recognized...' output), preserving the previous non-Windows behavior for installed CLIs whose --version exits non-zero.\n\nVerification after rebase:\n- npx --yes pnpm@9.12.0 exec vitest run packages/core/src/exec.test.ts packages/docs/pandoc/src/index.test.ts -> 7 passed, 1 Windows-only test skipped here\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-core typecheck -> passed\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-docs-pandoc typecheck -> passed |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
ec35ac5 to
a0eda03
Compare
|
Rebased again onto current upstream master after the latest auto-rebase bot comment. New head is a0eda03.\n\nVerification after rebase on Windows (node process.platform = win32):\n- npx --yes pnpm@9.12.0 exec vitest run packages/core/src/exec.test.ts packages/docs/pandoc/src/index.test.ts -> 7 passed, 1 non-Windows-only test skipped\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-core typecheck -> passed\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-docs-pandoc typecheck -> passed\n\nThe remaining Greptile note about % arguments and trailing-backslash args is covered by the Windows run of packages/core/src/exec.test.ts: it executes a bare .cmd shim through the cmd.exe dispatch path and asserts that both %SH1PT_EXEC_LITERAL% and C:\tmp\path\ survive unchanged. |
|
Fixed the remaining Windows cmd.exe argument-escaping review concern in 635e7e5. The Windows cmd dispatch now builds an escaped command line for cmd.exe, escaping percent signs and other cmd metacharacters and preserving trailing-backslash args. I also strengthened the regression test so SH1PT_EXEC_LITERAL is present in the environment; the test still verifies the child receives the literal %SH1PT_EXEC_LITERAL% plus C:\tmp\path\ unchanged. Also removed the redundant slash from the fake pandoc.cmd %~dp0 helper path.\n\nVerification on Windows:\n- npx --yes pnpm@9.12.0 exec vitest run packages/core/src/exec.test.ts packages/docs/pandoc/src/index.test.ts -> 7 passed, 1 non-Windows-only test skipped\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-core typecheck -> passed\n- npx --yes pnpm@9.12.0 --filter @profullstack/sh1pt-docs-pandoc typecheck -> passed |
|
Fixed the latest Windows cmd.exe quoting review item in b8b50e7. The cmd.exe shim path now stores child arguments in temporary environment variables and invokes them through delayed expansion, with escaping for !, embedded quotes, and trailing backslashes. That avoids leaking literal carets into quoted arguments like "Foo & Bar" while still preserving %SH1PT_EXEC_LITERAL%, C:\tmp\path, hello!world, and quoted values. Verification on Windows:
|
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
| function windowsEnvArg(value: string): string { | ||
| return value | ||
| .replace(/(\\*)"/g, '$1$1\\"') | ||
| .replace(/\\+$/, '$&$&') | ||
| .replace(/!/g, '^!'); | ||
| } |
There was a problem hiding this comment.
windowsEnvArg incorrectly escapes ! as ^!. cmd.exe's delayed expansion (/v:on) substitutes !NAME! into the output buffer and continues scanning from after the closing ! in the original input — the substituted value is never re-scanned for further ! patterns. Because ^ is also not an escape character inside CommandLineToArgvW (CRT) double-quoted strings, a value like hello!world stored as hello^!world will arrive in the child process as the literal string hello^!world. The test case 'hello!world' asserts the round-trip is clean, which is the right contract, but the assertion will fail on Windows. Because the test is not guarded by itOnWindows, the breakage won't surface on Linux CI.
Summary
exec()commands through the Windows shell on Windows so.cmdshims such asnpx,vsce,jsr, andpandocresolve correctlyWhy
On Windows,
child_process.spawn('npx', ...)andspawn('pandoc', ...)fail withENOENTin this environment because command shims are exposed as.cmdfiles. That means target/doc adapters that rely on external CLIs cannot execute even when the CLI is installed on PATH.Validation
pnpm exec vitest run packages/docs/pandoc/src/index.test.ts-> 5 passedpnpm --filter @profullstack/sh1pt-core typecheckpnpm --filter @profullstack/sh1pt-docs-pandoc typecheckFor the ugig bug-fix bounty: this fixes a Windows CLI execution bug found while testing the repository locally.