fix(vibe-tests): run tsx through cmd.exe on Windows in public-artifact-cli.test.mjs - #5908
Merged
cixzhang merged 1 commit intoSep 3, 2026
Conversation
…t-cli.test.mjs The suite spawned node_modules/.bin/tsx directly via spawnSync. pnpm generates tsx.cmd on Windows (a batch shim), which spawnSync can't run without a shell, so the spawn silently failed: result.status came back null instead of a real exit code, both tests asserting on it failed. Shells out through cmd.exe /c instead, matching the pattern already used for pnpm elsewhere in this package. Needed two things beyond that baseline pattern, both required together (verified by testing simpler combinations first): quoting each argument before joining them into a single command-line string, since cmd.exe /c reconstructs one string from the args array rather than receiving them as discrete arguments and this suite deliberately exercises a path containing a space (a "setup report <pid>" tmpdir); and windowsVerbatimArguments so spawnSync doesn't re-quote that already-quoted string as if it were one plain argument, which corrupted it before cmd.exe ever saw it. Verified by running the actual test file, not just reading the source: both previously-failing tests now pass, and the full internal/vibe-tests/setup-test/ suite is 116/124 passing (8 skipped, unrelated env-gated canonical suite), up from 114 before this fix.
HelloOjasMutreja
requested review from
cixzhang and
imdreamrunner
as code owners
September 2, 2026 07:18
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
community
Contributor
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
cixzhang
approved these changes
Sep 3, 2026
cixzhang
left a comment
cixzhang
left a comment
Contributor
There was a problem hiding this comment.
Thanks, this is a clean compatibility fix. The explicit cmd.exe call matches Node’s Windows batch-file contract while keeping every shell argument fixed or test-generated.
[Reviewed by Robohands]
github-actions
Bot
removed
the
needs:code-review
cixzhang
enabled auto-merge (squash)
September 3, 2026 21:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
public-artifact-cli.test.mjsspawnsnode_modules/.bin/tsxdirectly viaspawnSync(TSX, [script, ...args], {cwd, encoding: 'utf8'}). pnpm generatestsx.cmdon Windows (a batch shim), not a baretsxexecutable, andspawnSynccan't run a batch file without a shell. The spawn fails silently:result.statuscomes backnullinstead of a real exit code,result.stderrstays empty. Both tests assertingresult.statusto be0fail as a result.Confirmed on a real Windows run before the fix:
npx vitest run internal/vibe-tests/setup-test/public-artifact-cli.test.mjs --project node, 2 of 10 tests failing withexpected null to be +0.Fix
Shells out through
cmd.exe /cinstead, matching the established pattern for pnpm elsewhere in this package. Needed two things beyond that baseline pattern, both required together (confirmed by testing simpler combinations first and watching them fail differently):cmd.exe /cdoesn't receive Node's argv array as discrete arguments; it reconstructs one string from them and reparses it with its own rules. This suite deliberately exercises a path containing a space (a"setup report <pid>"tmpdir), which gets split apart beforetsxever sees it without this.windowsVerbatimArguments: true. Without it,spawnSyncre-quotes/escapes that already-quoted command-line string as if it were a single plain argument (sincecmd.exeis, to Node, just another regular executable), corrupting the quotes beforecmd.exeever sees them.Testing
internal/vibe-tests/setup-test/suite: 116/124 passing (8 skipped, the unrelated env-gated canonical suite), up from 114 before this fix.eslintclean (the file matches this package's existing eslint ignore pattern, same as the otherinternal/vibe-testsscripts touched in prior PRs).Related to the same root-cause bug class already fixed for pnpm in #5841 and #5843.