Skip to content

fix(scripts): launch check:changed on Windows without a vp shim - #24164

Merged
maria-rcks merged 5 commits into
pingdotgg:mainfrom
Zacharon:fix/check-changed-windows-vp
Nov 1, 2026
Merged

maria-rcks merged 5 commits into
pingdotgg:mainfrom
Zacharon:fix/check-changed-windows-vp

Conversation

@Zacharon

@Zacharon Zacharon commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Windows contributors hit spawn vp ENOENT when check:changed tries to launch the installed vp.cmd without a shell (https://git.cafe/pingdotgg/t3code/pulls/24005, https://git.cafe/pingdotgg/t3code/pulls/24006, https://git.cafe/pingdotgg/t3code/pulls/24007). On Windows, the shared command path now runs the installed vite-plus/bin JavaScript entry with process.execPath, resolved relative to the script rather than the temporary install directory. Arguments stay literal arrays; Linux and macOS keep bare vp.

The fix preserves main's Effect process scopes, concurrency budget, timeouts, output collection, and failure handling. Both normal checks and the temporary dependency-patch install use the launcher. No application behavior or dependencies change.

Verification at fc1052ddcd634b7e3b5434acd203eefc77d0e847:

  • Blacksmith vp run check:changed passed formatting, lint, both knip passes, scripts typecheck, and all 13 selected tests. The worker's shallow snapshot needed a main comparison ref and a metadata-only parent graft; source contents were unchanged.
  • The two focused test files also passed separately. Coverage includes literal spaces/Unicode/metacharacters/quotes/empty arguments, stdout/stderr, failure status, source selection, temporary install cwd, uncached failures, and main's existing scheduling/timeout/git-error cases.
  • The author's native Windows acceptance covers the previous a608c26c21 head, including real installed Vite+, source and manifest changes, failing checks, and literal paths.

Unverified in this Linux pass: native Windows execution of the merged Effect implementation and full CI suites. The selected Windows branch in Linux fixtures proves launcher coverage, not native Windows process behavior. No UI changes or browser evidence apply.

Model: gpt-6.1-sol. Harness: Codex in T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

@Zacharon, thanks for resolving the installed vite-plus/bin relative to the script and using process.execPath without a shell. that handles both launch sites and preserves literal arguments, including the temporary install cwd. i kept your implementation and commits; neither independent review found a useful simplification that preserved coverage.

i merged current main (220eb73524ca) and corrected the existing wrapper fixture to set TEMP explicitly. without it, selecting win32 on the linux worker made node's temporary directory resolve to undefined\temp, so two tests failed before reaching the install launcher. the install assertion now includes captured output for diagnosis. no new test files or production behavior changes were added during takeover.

verification at a608c26c21a4bd616ca1700eb03750e1591494c0:

  • blacksmith: vp run check:changed 220eb73524ca7882d53b6a12c824ccbcd97ad279 passed format, lint, both knip passes, scripts typecheck, and all nine tests. the worker's shallow snapshot needed an exact-main parent for the comparison; its source tree was unchanged.
  • the nine focused subprocess tests also passed independently. selecting the old bare-vp helper behavior made the four windows helper cases fail while the linux/macOS cases passed.
  • two independent reviews of this exact head found no code defects.
  • all seven reported pr checks are green. these reported checks do not establish native windows acceptance or a full application test-suite run.

unverified: native windows vp run check:changed with representative source and manifest changes, including the real installed cli and the temporary-cwd install. this linux vm has no native windows host. the pr remains draft pending that verification and maintainer approval. no ui changes or ui evidence apply.

@Zacharon

Copy link
Copy Markdown
Contributor Author

Native Windows acceptance at a608c26c21a4bd616ca1700eb03750e1591494c0

Host: Windows 10 Pro 10.0.19045 x64 (not WSL or a container). Node 26.12.1, pnpm 12.14.0, installed Vite+ 1.4.0. Fresh clone with pnpm install --frozen-lockfile. No global vp, no vp.cmd on PATH, and NODE_OPTIONS empty, so no shim was used. Each run was node_modules\.bin\vp.cmd run check:changed against origin/main (merge base 220eb73524). The checkout was reset and the pass cache cleared between runs.

Change set Result
Baseline: main's check-changed.ts + a source edit exit 1, every step spawn vp ENOENT in 0.0s (reproduces #24005–#24007)
Source edit in packages/ssh/src/command.ts exit 0: format, lint, knip 1/2 and 2/2, typecheck (4 packages), beside tests (3 files, 16 tests)
Manifest edit in packages/ssh/package.json exit 0: unused patches passed (real temp-cwd vp install --lockfile-only --ignore-scripts), typecheck (34 packages), knip, tests
Manifest adds a nonexistent dependency exit 1: FAIL unused patches with the full ERR_PNPM_FETCH_404 output from the temp-cwd install, no pass cached
Source edit adds new TextEncoder(); exit 1: FAIL lint with eslint(no-new) at packages/ssh/src/command.ts:31:1, no pass cached
Untracked, badly formatted docs/pr24164 space & (draft) %PATH% ! ^ 研究.md exit 1: FAIL format naming that exact path
Same path, formatted exit 0, pass cached

Also native:

  • The nine focused tests pass: cd scripts && vp test run --config ../vite.config.ts --dir . check-changed-command.test.ts check-changed.test.ts. On Windows the windows.mjs override is a no-op, so these use real win32 process creation.
  • A small probe called vitePlusCommand against the real installed CLI from an unrelated temp cwd containing & and (), with child PATH set to only the Node directory. Through both spawnSync and spawn, --version exited 0, fmt --check exited 1 on the odd path (space & (draft) %PATH% ! ^ 研究.md) and named it literally, and fmt --check exited 0 on a formatted file. The launcher resolved to node.exe …\node_modules\.pnpm\vite-plus@1.4.0…\vite-plus\dist\bin.js.

Not covered: Windows 11, ARM64, a globally installed Vite+ as the outer vp, and the full CI suites. This is acceptance evidence only, not maintainer approval.

Written by Claude Opus 5.5 (Claude Code in T3 Code) on behalf of the PR author.

@Zacharon
Zacharon marked this pull request as ready for review October 27, 2026 02:50
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between fc1052d and 01930f7.

Files selected for processing: 2
  • scripts/check-changed.test.ts
  • scripts/check-changed.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

The change adds vitePlusCommand to select a Vite+ executable and arguments by platform. scripts/check-changed.ts uses it to launch commands. Tests cover platform selection, argument forwarding, command output, and failure handling.

Changes: Vite+ command launch

Layer / File(s) Summary
Platform-specific command selection
scripts/check-changed-command.ts, scripts/check-changed-command.test.ts
vitePlusCommand returns the Node executable and resolved Vite+ entry on Windows, and vp on other platforms. Tests cover argument forwarding and execution results.
Changed-source check integration
scripts/check-changed.ts, scripts/check-changed.test.ts
The script uses vitePlusCommand to launch commands. Tests cover successful checks and install or lint failures.

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge, t3dotgg, maria-rcks

Merge Risk: Minimal · up to 01930

Windows checks now launch the installed CLI through Node; other platforms retain vp. No merge-blocking issue was identified.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly identifies the main change: fixing Windows launch behavior for check:changed without a vp shim. It is concise and specific.
Description check Passed The description explains the Windows spawn failure, the launcher change, preserved behavior, verification results, and known verification limits. It does not use the template headings and does not exp…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the Windows spawn failure, the launcher change, preserved behavior, verification results, and known verification limits. It does not use the template headings and does not explicitly document maintainer approval, but it provides enough substantive information for this focused fix.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

no changed runtime behavior was verified on this vm. i inspected the description, full diff, commits and discussion at a608c26c21a4bd616ca1700eb03750e1591494c0. the pr is open, and its three listed commits are by Zacharon and maria-rcks, with none by juliusmarminge or t3dotgg. git.cafe reports seven successful checks, zero pending and zero failing; these are reported checks, not checks i ran.

host transcript:

$ node -p 'JSON.stringify({platform:process.platform,arch:process.arch,node:process.version})'
{"platform":"linux","arch":"x64","node":"v24.22.1"}
$ uname -srm
Linux 6.8.0-117-generic x86_64

the production change selects Node plus the installed vite-plus/bin entry only on win32, at the synchronous temporary-cwd install and asynchronous check-step launch sites. linux and macos retain bare vp. this is a contributor command, with no running-app client/provider entry point. an app screenshot or a Codex turn would not exercise the fix.

Not verified

native windows reproduction on main and acceptance on the pr head, including source checks, manifest-triggered temporary-cwd installation, literal arguments and failure propagation. this linux vm cannot establish windows process-launch behavior. the author's native windows acceptance comment is existing evidence, not independently reproduced evidence from this pass.

no main worktree, dependency install, dev server, project, thread or provider turn was created, because none would reach the changed windows behavior on this host. no screenshots or recordings were captured. no code or branch changes were made. status for this pass: unverifiable; native windows is required.

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Oct 30, 2026

maria-rcks commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

merged current main (ce48e4590018) into the existing branch and pushed 01930f768d340f97fe4dbba267f4bb8c7ee51dbf without force. resolved the test-fixture conflict by preserving main's comparison-base tests and the windows launcher tests. production behavior remains the contributor's implementation. the diff stays at four files, 290 additions and one deletion.

blacksmith vp run check:changed github/main passed format, lint, both knip passes, scripts typecheck, and all 17 tests. the worker's shallow snapshot needed temporary history repair to compare with exact main; source contents were unchanged. two independent reviews approved this exact head without findings. no inline review findings remain.

ci and github's current conflict observation are pending. native windows execution was not reproduced on this linux host; the author's acceptance covers the previous head. no ui changes apply.

@Zacharon

Copy link
Copy Markdown
Contributor Author

Native Windows check at fc1052ddcd634b7e3b5434acd203eefc77d0e847 (after the merge of main)

Host: Windows 10 Pro 10.0.19045 x64. Node 26.12.1, Vite+ 1.4.0. The working tree had a source change in packages/shared/src/fileLinks.ts and a manifest change in packages/shared/package.json, so unused patches ran its install in a temp folder.

  • On main (6575ce32508b), vp run check:changed fails all 7 steps in under a second each with NotFound: ChildProcess.spawn (vp ...).
  • On this head, vp run check:changed exits 0 and all 7 steps pass: unused patches (155s), format, lint, knip 1/2 and 2/2, typecheck (39 packages), and tests (6 files).

Written by Claude Opus 5.5 (Claude Code in T3 Code) on behalf of the PR author.

@maria-rcks
maria-rcks merged commit bc57ebf into pingdotgg:main Nov 1, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants