Skip to content

Fix Windows CLI shim execution#445

Open
ayskobtw-lil wants to merge 5 commits into
profullstack:masterfrom
ayskobtw-lil:codex/windows-pandoc-test-shim
Open

Fix Windows CLI shim execution#445
ayskobtw-lil wants to merge 5 commits into
profullstack:masterfrom
ayskobtw-lil:codex/windows-pandoc-test-shim

Conversation

@ayskobtw-lil
Copy link
Copy Markdown

Summary

  • Run shared exec() commands through the Windows shell on Windows so .cmd shims such as npx, vsce, jsr, and pandoc resolve correctly
  • Keep non-Windows execution unchanged
  • Make the docs-pandoc fake CLI test install a Windows-compatible shim and use the platform PATH delimiter

Why

On Windows, child_process.spawn('npx', ...) and spawn('pandoc', ...) fail with ENOENT in this environment because command shims are exposed as .cmd files. 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 passed
  • pnpm --filter @profullstack/sh1pt-core typecheck
  • pnpm --filter @profullstack/sh1pt-docs-pandoc typecheck

For the ugig bug-fix bounty: this fixes a Windows CLI execution bug found while testing the repository locally.

@ayskobtw-lil
Copy link
Copy Markdown
Author

Opened the matching bug report as #446 for the ugig bug-fix trail. This PR fixes the Windows .cmd shim execution issue.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 26, 2026

Greptile Summary

This PR fixes Windows CLI shim resolution by routing bare command names through cmd.exe /d /s /c on Windows, and improves ensureCli to detect the Windows "command not found" exit code (9009) rather than relying solely on ENOENT.

  • exec.ts: shouldUseWindowsCmd gates the shell route to bare names without path separators or .exe/.com extensions; isWindowsCommandNotFound checks for exit 9009 and the "not recognized" error string on Windows only, leaving non-Windows behavior unchanged.
  • exec.test.ts: New tests cover the Windows false-positive detection and the non-Windows non-zero-exit distinction; a cross-platform test for %-wrapped arguments installs platform-appropriate shims.
  • index.test.ts (pandoc): Adds a Windows .cmd shim for fake pandoc and replaces the hard-coded : PATH delimiter with node:path's delimiter.

Confidence Score: 3/5

The Windows argument-passing path in exec.ts has correctness gaps that prior review rounds identified and that remain unaddressed; merging before those are resolved risks silent argument corruption on Windows for any caller that passes metadata or paths with special characters.

The core change routes bare command names through cmd.exe /d /s /c without escaping arguments for the cmd.exe command-line context. Prior review threads documented that %VAR% patterns are still subject to shell expansion and that arguments ending with a backslash break cmd.exe double-quote parsing, both of which go unaddressed in this revision. The new test that is supposed to validate %-literal preservation also includes a trailing-backslash argument that would be mangled in the same cmd.exe context. Until the argument escaping is resolved, the Windows execution path cannot be trusted to deliver arguments to the spawned process intact.

packages/core/src/exec.ts — the Windows spawn path and its argument handling warrant the most attention before merging.

Important Files Changed

Filename Overview
packages/core/src/exec.ts Adds Windows cmd.exe routing via shouldUseWindowsCmd and isWindowsCommandNotFound; previous review threads have flagged unresolved % expansion and trailing-backslash argument corruption that this PR does not address.
packages/core/src/exec.test.ts New test file covering Windows cmd.exe routing and ensureCli detection; the % preservation test runs on all platforms, covering the Unix path trivially but only exercising cmd.exe behaviour on Windows.
packages/docs/pandoc/src/index.test.ts Adds Windows fake-pandoc shim and fixes PATH delimiter; pandoc.cmd uses redundant %~dp0\pandoc.js (extra \ since %~dp0 already ends with ), unlike the exec test helper which correctly uses %~dp0echo-args.js.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["exec(cmd, args, opts)"] --> B{shouldUseWindowsCmd?}
    B -- "win32 && no slashes && not .exe/.com" --> C["spawn('cmd.exe', ['/d','/s','/c', cmd, ...args])"]
    B -- "otherwise" --> D["spawn(cmd, args)"]
    C --> E[Collect stdout/stderr]
    D --> E
    E --> F{throwOnNonZero && exitCode != 0?}
    F -- yes --> G[reject with error]
    F -- no --> H[resolve ExecResult]
    H --> I["ensureCli uses exec with throwOnNonZero:false"]
    I --> J{isWindowsCommandNotFound?}
    J -- "win32 && exitCode==9009 or 'not recognized' in output" --> K[throw 'command not found']
    J -- "non-win32 or exitCode==0" --> L[return void]
    K --> M[catch: log + rethrow as 'not installed']
Loading

Reviews (6): Last reviewed commit: "Narrow Windows ensureCli missing detecti..." | Re-trigger Greptile

Comment thread packages/core/src/exec.ts Outdated
Comment thread packages/core/src/exec.ts Outdated
@ayskobtw-lil
Copy link
Copy Markdown
Author

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.

Comment thread packages/core/src/exec.ts Outdated
@ayskobtw-lil
Copy link
Copy Markdown
Author

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.

@ayskobtw-lil
Copy link
Copy Markdown
Author

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

Comment thread packages/core/src/exec.ts Outdated
@github-actions
Copy link
Copy Markdown

🤖 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: git fetch upstream master && git rebase upstream/master.

@ayskobtw-lil ayskobtw-lil force-pushed the codex/windows-pandoc-test-shim branch from e8b8dea to ec35ac5 Compare May 27, 2026 15:39
@ayskobtw-lil
Copy link
Copy Markdown
Author

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

@github-actions
Copy link
Copy Markdown

🤖 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: git fetch upstream master && git rebase upstream/master.

@ayskobtw-lil ayskobtw-lil force-pushed the codex/windows-pandoc-test-shim branch from ec35ac5 to a0eda03 Compare May 27, 2026 21:39
@ayskobtw-lil
Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant