Skip to content

Fix Windows CLI shim execution#445

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

Fix Windows CLI shim execution#445
ayskobtw-lil wants to merge 4 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 execution by routing bare command names through cmd.exe /d /s /c on Windows, enabling .cmd shims (npx, vsce, pandoc, etc.) to resolve correctly. It also fixes ensureCli silently reporting absent CLIs as present on Windows by checking the exit code explicitly.

  • exec.ts: Adds shouldUseWindowsCmd predicate; on Windows, bare commands are dispatched through cmd.exe instead of spawn directly. ensureCli now throws on any non-zero exit code to catch Windows's "command not found" code 9009.
  • exec.test.ts: New test suite verifying Windows shim resolution and the corrected ensureCli non-zero-exit behavior via portable fake CLI helpers.
  • index.test.ts (pandoc): Adds a Windows .cmd shim for the fake pandoc binary and switches the PATH concatenation to the cross-platform delimiter.

Confidence Score: 3/5

The Windows dispatch logic is sound for typical usage, but ensureCli now incorrectly reports an installed CLI as missing on all platforms when its --version command exits non-zero — a behavioral regression on Linux that didn't exist before this PR.

The exitCode !== 0 guard in ensureCli is applied unconditionally across all platforms. On Linux the original code relied solely on ENOENT to detect a missing command; any CLI whose --version exits non-zero (even when fully installed) now gets silently mislabeled as "not installed" and callers receive a misleading error. This is a change to a shared utility that affects every adapter that calls ensureCli.

packages/core/src/exec.ts — specifically the ensureCli exit-code check at line 86.

Important Files Changed

Filename Overview
packages/core/src/exec.ts Adds Windows-specific cmd.exe dispatch via shouldUseWindowsCmd; fixes ensureCli silent-pass on Windows by checking exitCode, but the exitCode !== 0 guard is now applied on all platforms, introducing a regression on Linux.
packages/core/src/exec.test.ts New test file covering Windows-shim resolution and ensureCli non-zero-exit detection; tests pass on Linux CI but the percent-arg test is designed for a Windows-only code path.
packages/docs/pandoc/src/index.test.ts Adds Windows .cmd shim for fake pandoc and fixes cross-platform PATH delimiter; logic is straightforward and low-risk.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["exec(cmd, args, opts)"] --> B{shouldUseWindowsCmd?}
    B -- "win32 + bare name\n(no / \\ , not .exe/.com)" --> C["spawn('cmd.exe', ['/d','/s','/c', cmd, ...args])"]
    B -- "non-Windows or\npath / .exe / .com" --> D["spawn(cmd, args)"]
    C --> E[child process runs via cmd.exe\n.cmd shim resolves on PATH]
    D --> F[child process runs directly]
    E & F --> G{exitCode / error?}
    G -- ENOENT --> H["reject: 'command not found: cmd'"]
    G -- "exitCode !== 0\n(throwOnNonZero)" --> I["reject: cmd failed"]
    G -- exitCode 0 --> J["resolve: ExecResult"]

    K["ensureCli(cmd, hint, log)"] --> L["exec(cmd, ['--version'], throwOnNonZero:false)"]
    L --> M{exitCode !== 0?}
    M -- yes --> N["throw 'command not found: cmd'"]
    M -- no --> O["CLI present — return"]
    N --> P["catch: log error\nthrow 'cmd not installed. hint'"]
Loading

Reviews (4): Last reviewed commit: "Address Windows cmd argument handling" | 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
Comment on lines +85 to +86
const result = await exec(cmd, ['--version'], { log: () => {}, throwOnNonZero: false });
if (result.exitCode !== 0) throw new Error(`command not found: ${cmd}`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 ensureCli now rejects installed CLIs that exit non-zero for --version

On Linux (where spawn still directly executes the binary), any CLI that returns a non-zero exit code from --version — even though it is installed and working — will now be thrown as command not found: <cmd> and ultimately reported as "not installed". Before this PR, only ENOENT triggered that branch on Linux; a non-zero exit was silently ignored and treated as "present". The broader exitCode !== 0 guard is necessary on Windows (where cmd.exe swallows ENOENT and returns 9009), but applying it unconditionally on Linux changes the contract for every caller. Consider limiting the exitCode check to process.platform === 'win32' so the Linux path retains its original ENOENT-only semantics.

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