Skip to content

test(cli): run the real npx shim check only in its own CI job - #6969

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/npx-command-test-hermetic
Nov 2, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
fix/npx-command-test-hermetic

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

What

npxCommand.test.ts ran a real cmd.exe /c npx.cmd --version with a 30 s timeout in every CLI shard. On the Windows runner that timed out (spawnSync cmd.exe ETIMEDOUT) and turned a required check red on an unrelated PR.

The real-execution test stays, but runs only when HF_NPX_SHIM_E2E=1, which the dedicated CLI: npx shim job (ubuntu, macos, windows) now sets on its test step. The general CLI shards no longer spawn npx and skip that one test; the per-platform builder tests, which assert the exact command and arguments, run everywhere.

Why

Flaky required check on #6964, a CLI change that never touches npx: one ETIMEDOUT in engine-cli-2 on windows-latest. The shim job keeps the only real execution of the cmd shim, so a shim that builds the right argv but fails to run on Windows still turns that job red.

Test

Without the variable the file runs 6 tests and skips the real-npx one; with HF_NPX_SHIM_E2E=1 all 7 pass, including the real npx --version. With the win32 branch of buildNpxCommand broken (/d /s dropped), the win32 npx and npm builder tests fail. 3 runs in a row.

fails without the fix: test-only change with no failing-test counterpart. Its effect is that engine-cli-2 no longer spawns npx, and CLI: npx shim (windows-latest) still runs it for real.

Size

Under 100 changed lines.

@heygengenesis heygengenesis Bot left a comment

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.

Requesting changes at d4b4db9c.

Blocker: this removes the only real shim execution, though the body says that coverage remains. The body says "Real execution of the cmd shim stays covered by the CLI: npx shim (windows-latest) job". That job (.github/workflows/ci.yml:845, cli-npx-shim) runs only src/utils/npxCommand.test.ts src/commands/skills.test.ts. skills.test.ts mocks node:child_process (spawn/execFile/execFileSync are vi.fn()). So the test deleted here was the only thing in that job that ran cmd.exe /d /s /c npx.cmd for real. After this PR the job just repeats the hermetic builder assertions on three OSes, and nothing in CI would catch a shim invocation that builds the right argv but doesn't run on Windows. The deleted comment also said the test was kept unmocked on purpose ("it isn't reduced to a tautology by mocking"). The new host-platform test compares the default parameter with itself.

Suggested fix: keep the real-execution test but run it only in the dedicated job. For example, gate it with it.runIf(process.env.HF_NPX_SHIM_E2E === "1") (keep the 30 s / 60 s timeouts) and set HF_NPX_SHIM_E2E: "1" on the cli-npx-shim test step. The general CLI shards then stop spawning npx, so the engine-cli-2 ETIMEDOUT goes away, and the shim job still runs the real thing. If you'd rather drop real execution entirely, the body has to say that coverage is gone, and the cli-npx-shim job should be removed or given a real check.

fails without the fix: line. The line doesn't name a test that fails without the change. The two tests it names pass with or without this PR. Once the change above makes the shim job the place where real execution lives, please say which check would go red without it, or state plainly that this is a test-only change with no failing-test counterpart. Then a reviewer can verify the claim instead of having to trust it.

Not blocking: the per-platform builder tests are correct, and the hermetic shape is fine for the general shards.


Reviewed by Genesis V2 on behalf of @miguel-heygen.

@miguel-heygen miguel-heygen changed the title test(cli): the npx command test builds the command instead of running a real npx test(cli): run the real npx shim check only in its own CI job Nov 2, 2026
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Fixed in 0c74d36, both points. The real npx test is back, gated with it.runIf on HF_NPX_SHIM_E2E=1, and the cli-npx-shim test step sets it, so that job is again the one place that really runs cmd.exe /d /s /c npx.cmd; the general shards skip it. The body now says plainly this is a test-only change with no failing-test counterpart, and what it changes in CI. Verified on miga: 6 pass and 1 skipped without the variable, 7 pass with it (real npx --version), 3 runs.

@heygengenesis heygengenesis Bot left a comment

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.

Approving at 0c74d364. Both points from my earlier review are resolved.

  • The real-npx test is still there. It now runs only when it.runIf(process.env.HF_NPX_SHIM_E2E === "1") is true, with the same 30 s / 60 s timeouts. The cli-npx-shim test step is the only place that sets the variable. test is plain vitest run, so the variable reaches the worker. I checked the CLI: npx shim (ubuntu-latest) log on this head: npxCommand.test.ts (7 tests) passed and the real test was not skipped. The general shards skip it, so engine-cli-2 no longer spawns npx.
  • The fails without the fix: line now says plainly that this is a test-only change with no failing-test counterpart and names the effect. That fits a test(...) PR with no production change.

The CLI: npx shim (windows-latest) job was still running when I reviewed. The merge gate covers it.


Reviewed by Genesis V2 on behalf of @miguel-heygen.

@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 99 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit c06427b Nov 2, 2026
130 of 131 checks passed
@miguel-heygen
miguel-heygen deleted the fix/npx-command-test-hermetic branch November 2, 2026 02:34
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