Skip to content

fix(command): route Windows .cmd shims through PowerShell for pm commands#1498

Draft
fengmk2 wants to merge 16 commits intovp-create-support-orgfrom
ps1-v2-for-pm
Draft

fix(command): route Windows .cmd shims through PowerShell for pm commands#1498
fengmk2 wants to merge 16 commits intovp-create-support-orgfrom
ps1-v2-for-pm

Conversation

@fengmk2
Copy link
Copy Markdown
Member

@fengmk2 fengmk2 commented Apr 29, 2026

Apply the same .cmd → .ps1 rewrite to vite_command::run_command (and
run_command_with_fspy) so package-manager-routed commands (vp dlx,
vp add, vp remove, vp update, vp install, …) stop spawning
cmd.exe on Windows and Ctrl+C no longer leaves the terminal in a
broken state.

The task-layer fix (vite_task_plan::ps1_shim) only covered
node_modules/.bin/*.cmd — too narrow for pm shims like npm.cmd /
pnpm.cmd / yarn.cmd which live in ~/.vite-plus/js_runtime/...
and system PATH. The new module drops the node_modules/.bin check:
if a .cmd has a sibling .ps1, route through PowerShell.

Closes #1489


Note

Medium Risk
Changes process spawning behavior on Windows (rewriting some .cmd executions to PowerShell), which could affect edge-case CLI semantics or environments lacking PowerShell. Scope is constrained to vp-managed paths and node_modules/.bin shims and is covered by targeted unit tests.

Overview
Fixes Windows Ctrl+C/terminal corruption for package-manager–routed commands by resolving the target binary to an absolute path and, when it’s a vp-managed or node_modules/.bin .cmd with a sibling .ps1, spawning powershell with a -File <shim.ps1> prefix instead.

This adds a new vite_command::ps1_shim module (with unit tests), wires it into both run_command and run_command_with_fspy, and updates workspace deps/lockfile to a newer vite-task revision including the new vite_powershell crate. Separately, the snap-test runner now guards temp-dir cleanup on exit with error handling.

Reviewed by Cursor Bugbot for commit ad7b4bf. Configure here.

Copy link
Copy Markdown
Member Author

fengmk2 commented Apr 29, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label auto-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Copy Markdown
Member Author

fengmk2 commented Apr 29, 2026

deps on voidzero-dev/vite-task#368

@fengmk2 fengmk2 force-pushed the pm-features-at-local-cli branch from 74c9564 to 46f67b1 Compare April 30, 2026 01:11
Copy link
Copy Markdown
Member Author

fengmk2 commented Apr 30, 2026

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit fc05b17. Configure here.

Comment thread crates/vite_command/src/lib.rs Outdated
fengmk2 added a commit that referenced this pull request Apr 30, 2026
Per PR review on #1498: every Rust call site of `run_command` /
`run_command_with_fspy` already builds envs with uppercase `"PATH"`
(via `HashMap::from([("PATH".to_string(), format_path_env(...))])`
in vite_install command handlers and vite_pm_cli). The `Path`-keyed
scenario the case-insensitive lookup defended against doesn't reach
this code path, so the lookup and its Windows-only regression test
are dead weight. Use `envs.get("PATH")` directly.
@fengmk2
Copy link
Copy Markdown
Member Author

fengmk2 commented Apr 30, 2026

@cursor review

@fengmk2 fengmk2 added test: e2e Auto run e2e tests test: install-e2e run vite install e2e test test: create-e2e Run `vp create` e2e tests labels Apr 30, 2026
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit ad7b4bf. Configure here.

Comment thread packages/cli/src/utils/command.ts Outdated
fengmk2 added a commit that referenced this pull request Apr 30, 2026
…ProjectDir

Per PR review on #1498: `projectDir?: string` doesn't belong on the
canonical `ExecutionResult` in `utils/command.ts` — plain `runCommand`
and `runCommandSilently` never produce one. Move it onto a new
domain-specific interface `ExecutionWithProjectDir extends
ExecutionResult` defined next to `runCommandAndDetectProjectDir` in
`create/command.ts` (the function that originates the field).

All template executors that produce projectDir now declare
`Promise<ExecutionWithProjectDir>`: `executeBundledTemplate`,
`executeBuiltinTemplate`, `executeGeneratorScaffold`,
`executeMonorepoTemplate`, `executeRemoteTemplate`,
`runRemoteTemplateCommand`. `bin.ts:938`'s `result` variable is
widened to the same interface so consumers can read `.projectDir`.

Type-only refactor — no runtime change.
fengmk2 added 12 commits April 30, 2026 21:47
…ands

Apply the same .cmd → .ps1 rewrite to `vite_command::run_command` (and
`run_command_with_fspy`) so package-manager-routed commands (`vp dlx`,
`vp add`, `vp remove`, `vp update`, `vp install`, …) stop spawning
`cmd.exe` on Windows and Ctrl+C no longer leaves the terminal in a
broken state.

The task-layer fix (`vite_task_plan::ps1_shim`) only covered
`node_modules/.bin/*.cmd` — too narrow for pm shims like `npm.cmd` /
`pnpm.cmd` / `yarn.cmd` which live in `~/.vite-plus/js_runtime/...`
and system PATH. The new module drops the `node_modules/.bin` check:
if a `.cmd` has a sibling `.ps1`, route through PowerShell.

Closes #1489
…bing

Both `run_command` and `run_command_with_fspy` were doing the same
resolve_bin + rewrite_cmd_to_powershell match. Pull that into a
private helper so each call site is one line.
Switch `vite_command::ps1_shim` to depend on the new `vite_powershell`
crate (companion change in voidzero-dev/vite-task#368) for the
PowerShell host lookup, the fixed argument prefix, and the
sibling-`.ps1` discovery. This module now just composes those
primitives with vp's own conventions: absolute `.ps1` path in args
and `Vec<OsString>` return type to match the spawn API.

Bumps the vite-task git rev across all pinned crates to pick up the
extraction.
Picks up the test-only AbsolutePathBuf import move from
voidzero-dev/vite-task#368 review feedback.
Two minor cleanups from review:
- Hoist `OsString` into the existing `std::ffi` `use` so the
  `resolve_program` return type reads `Vec<OsString>` instead of
  the inlined `Vec<std::ffi::OsString>`.
- Drop the `Arc::<AbsolutePath>::from(...)` turbofish in the
  `host_arc` test helper — type inference handles it.
On Windows the canonical key is `Path`, and the TS layer's
`prependToPathToEnvs` preserves whatever casing the process started
with when adding the downloaded package-manager bin dir. Hard-coding
`envs.get("PATH")` missed a `Path`-keyed override and silently fell
back to the process PATH — where the prepended shim is absent —
making `vp create` (and any other flow routed through
`runCommandAndDetectProjectDir` / `run_command_with_fspy`) fail with
`Cannot find binary path...` for `npx`/`pnpm`.

Match the case-insensitive lookup `fspy::Command::resolve_program`
already does, so the previously-correct behavior of the fspy path
holds after vp-side resolution moved up a layer in
1b77707 (refactor(command): extract resolve_program to dedupe
ps1-rewrite plumbing).

Adds a Windows-only regression test (`resolve_program_tests`) that
writes a `.exe` into a tempdir and resolves via a `Path`-keyed env
map; without the fix the test fails because resolution falls back to
the process PATH.
Per the Codex adversarial review: the previous implementation
rewrote any `.cmd` with a sibling `.ps1` to spawn through
`PowerShell -ExecutionPolicy Bypass`. That covers vp's own pm
shims but also silently retargets unrelated PATH commands —
system tools, globally-installed npm wrappers, third-party CLIs
whose `.cmd`/`.ps1` wrappers may not be semantically equivalent —
and broadens execution semantics on locked-down hosts that
intentionally block unsigned `.ps1` execution.

Gate the rewrite on `resolved.starts_with($VP_HOME)`, where
`$VP_HOME` is the vp install root (`~/.vite-plus` by default).
That covers the legitimate cases (`$VP_HOME/js_runtime/node/<v>/npm.cmd`
and `$VP_HOME/package_manager/<pm>/<v>/<pm>/bin/<pm>.cmd`) and
leaves anything else alone. `$VP_HOME` is read once via
`vite_shared::get_vp_home()` and cached in a `LazyLock`.

Adds a regression test (`returns_none_for_cmd_outside_vp_home`)
that puts a matching `.cmd`+`.ps1` pair outside the scope and
asserts no rewrite happens.
…test

Two minor cleanups from review:
- Merge the two-sentence "outside `$VP_HOME` is left alone" paragraph
  into one that focuses on the WHY (avoid silently changing execution
  semantics for unrelated commands; respect locked-down hosts) instead
  of restating the trade-off as status-quo prose.
- Rename the second `ps1_str` binding in the inside-vp-home test to
  `ps1_path` so the `PathBuf` and the `&str` derived from it have
  distinct names.
Extends the rewrite scope so `<...>/node_modules/.bin/*.cmd` is also
routed through PowerShell, in addition to paths inside `$VP_HOME`.
The structural check is purely path-component based (case-insensitive
on `.bin`/`node_modules` to match Windows semantics) — no project
locality gate. npm/pnpm/yarn-emitted shims always come with a sibling
`.ps1`, so the rewrite stays self-consistent across single-package
projects, hoisted monorepos, and globally-installed shims.

Anything outside both scopes — system tools, third-party CLIs whose
`.cmd`/`.ps1` wrappers may diverge — still keeps its existing `.cmd`
path.

Adds tests for the node_modules/.bin path and a casing variant
(`Node_Modules\.Bin`); removes the now-redundant cwd-locality tests.
Picks up the squash-merged voidzero-dev/vite-task#368 (the
`vite_powershell` crate extraction). Replaces the draft-branch SHA
`957278df` with the merged-to-main SHA across all 7 vite-task crate
pins (fspy, vite_glob, vite_path, vite_powershell, vite_str,
vite_task, vite_workspace).
Per PR review on #1498: every Rust call site of `run_command` /
`run_command_with_fspy` already builds envs with uppercase `"PATH"`
(via `HashMap::from([("PATH".to_string(), format_path_env(...))])`
in vite_install command handlers and vite_pm_cli). The `Path`-keyed
scenario the case-insensitive lookup defended against doesn't reach
this code path, so the lookup and its Windows-only regression test
are dead weight. Use `envs.get("PATH")` directly.
…cstring

Two cleanups from review:
- Trim the "complementary task-layer rewrite" paragraph from the
  module docstring — readers can `git grep ps1_shim` if they need to
  find the sibling module in vite-task.
- Tighten `rewrite_in_scope`'s `host` parameter from
  `&Arc<AbsolutePath>` to `&AbsolutePath` so the internal Arc wrapper
  no longer leaks through the test boundary. Tests now build a plain
  `AbsolutePathBuf` host via a renamed `host_buf` helper.
fengmk2 added 4 commits April 30, 2026 21:48
… PowerShell deadlock

Two intertwined fixes:

1. Drop the stdin pipe in `runCommandSilently` — use `stdio: ['ignore',
   'pipe', 'pipe']` instead of `stdio: 'pipe'`. Leaving an open stdin
   pipe deadlocks any descendant `.ps1` shim whose
   `$MyInvocation.ExpectingInput` branch reads stdin to EOF before
   invoking `node` (the npm/pnpm/yarn cmd-shim pwsh template). The
   `migration-prettier-eslint-combo` snap test on Windows hung because
   `vp migrate` shells out via `runCommandSilently` to `vp dlx
   @oxlint/migrate`, and after the .cmd → PowerShell rewrite landed on
   this branch the inherited stdin pipe was never closed.

2. Consolidate the two duplicate `runCommandSilently` / `runCommand`
   pairs (`packages/cli/src/utils/command.ts` and
   `packages/cli/src/create/command.ts`) into one canonical copy in
   `utils/command.ts`. `create/command.ts` now re-exports them and
   keeps only the create-specific helpers
   (`runCommandAndDetectProjectDir`, `getPackageRunner`,
   `formatDlxCommand`, `prependToPathToEnvs`). Aligns the previously
   diverging `runCommand` return types (`number` vs `ExecutionResult`)
   on `Promise<ExecutionResult>` — utils' `runCommand` had no callers
   so the rename is safe — and prevents the stdin fix from drifting
   between two copies.
`vp create vite@9.0.5 ... --template react-ts` was failing on Windows
with `vite-plus: failed to execute …\vp.exe`. The chain:

  fspy spawns powershell.exe (after the .cmd → .ps1 rewrite)
    → pnpm.ps1 invokes node.exe
      → node.exe is the vite_trampoline shim
        → trampoline tries `Command::new(vp_exe).status()` → fails

fspy uses Microsoft Detours (`DetourUpdateProcessWithDll` in
`crates/fspy/src/windows/mod.rs`) to inject a tracking DLL into spawned
processes and propagate the injection to descendants via the hooked
`CreateProcess`. With the rewrite landed, an extra PowerShell frame
sits between the fspy spawn and the descendant trampolines, and the
injection chain into the trampoline's vp.exe child fails — surfacing
as the "failed to execute" error from `vite_trampoline/src/main.rs`.

Pre-branch the chain went through cmd.exe, where Detours' standard
child-process hook propagates injection cleanly.

Restore `run_command_with_fspy` to its pre-branch form:
`fspy::Command::new(bin_name)`, no upfront `resolve_bin`, no rewrite —
let fspy do its own PATH lookup as before. Also reverts the test
assertion to fspy's native error message.

The fspy path is one-shot and non-interactive (project-dir detection
in `vp create`); the Ctrl+C corruption the rewrite avoids isn't a
concern there. The rewrite stays in place for `run_command`, which
serves all the pm subcommands (`vp dlx`, `vp add`, `vp remove`,
`vp update`, `vp install`, …) where Ctrl+C does matter.
…ProjectDir

Per PR review on #1498: `projectDir?: string` doesn't belong on the
canonical `ExecutionResult` in `utils/command.ts` — plain `runCommand`
and `runCommandSilently` never produce one. Move it onto a new
domain-specific interface `ExecutionWithProjectDir extends
ExecutionResult` defined next to `runCommandAndDetectProjectDir` in
`create/command.ts` (the function that originates the field).

All template executors that produce projectDir now declare
`Promise<ExecutionWithProjectDir>`: `executeBundledTemplate`,
`executeBuiltinTemplate`, `executeGeneratorScaffold`,
`executeMonorepoTemplate`, `executeRemoteTemplate`,
`runRemoteTemplateCommand`. `bin.ts:938`'s `result` variable is
widened to the same interface so consumers can read `.projectDir`.

Type-only refactor — no runtime change.
The five re-exports in `create/command.ts` were a leftover from the
earlier consolidation pass:

- `ExecutionResult`, `RunCommandOptions`, `RunCommandResult` (type
  re-exports): nobody imported them through `create/command.ts`. Three
  dead bridges.
- `runCommand`, `runCommandSilently` (value re-exports): only
  `create/templates/remote.ts` consumed them. Migrate that one file to
  import directly from `'../../utils/command.ts'` and the bridge can
  go.

`create/command.ts` is now strictly a create-flow module: it defines
`ExecutionWithProjectDir`, `runCommandAndDetectProjectDir`,
`getPackageRunner`, `formatDlxCommand`, `prependToPathToEnvs` — no
bridging from utils.
@fengmk2 fengmk2 changed the base branch from pm-features-at-local-cli to graphite-base/1498 April 30, 2026 12:48
@fengmk2 fengmk2 force-pushed the graphite-base/1498 branch from 46f67b1 to 4e511a3 Compare April 30, 2026 12:48
@fengmk2 fengmk2 changed the base branch from graphite-base/1498 to vp-create-support-org April 30, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test: create-e2e Run `vp create` e2e tests test: e2e Auto run e2e tests test: install-e2e run vite install e2e test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant