Skip to content

fix(media): resolve npx without npm_execpath on Windows#2236

Merged
miguel-heygen merged 4 commits into
mainfrom
fix/windows-media-npx
Jul 11, 2026
Merged

fix(media): resolve npx without npm_execpath on Windows#2236
miguel-heygen merged 4 commits into
mainfrom
fix/windows-media-npx

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

What

  • resolve npm's npx-cli.js beside the active Node executable when npm_execpath is unset
  • keep the existing direct node+npx-cli invocation without shell execution
  • cover direct node audio.mjs Windows launches with a regression test

Why

Standalone media scripts are commonly launched with plain Node, so npm_execpath is absent. On Windows this made every Kokoro TTS/transcribe call short-circuit and omit narration even though the installed Node distribution included npx.

How

The resolver first uses npm_execpath when available, then falls back to <node-dir>/node_modules/npm/bin/npx-cli.js. The discovered JS CLI is executed with the active Node binary, preserving argv boundaries and avoiding .cmd shell execution.

Test plan

  • node --test skills/media-use/audio/scripts/lib/tts.spawn.test.mjs (8/8)
  • pre-commit skills-manifest and format hooks

@terencecho terencecho 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.

LGTM on the code; CI is red on Format — needs bun run format before merge. Also flagging a test-coverage regression on the diagnostic-warn path.

Cross-checks

  • Windows path handling. join(dirname(nodeExecPath), "node_modules", "npm", "bin", "npx-cli.js") uses node:path.join, which is platform-aware; dirname(process.execPath) on Windows yields C:\Program Files\nodejs. Spaces in the Windows path are safe because args are passed to spawn as an array, not a shell string — resolveSpawnCommand still avoids shell: true and never touches .cmd.
  • Command injection audit. No exec / execSync / shell: true anywhere in the diff. spawn args remain [npxCliPath, ...args.map(String)] — argv boundaries preserved.
  • Unix regression check. resolveNpxCliPath calls resolveNpxCliFromNpmExecPath first; when npm_execpath is set (normal Unix npm run … invocation), behavior is byte-identical to before. Non-win32 branch of resolveSpawnCommand still short-circuits before the fallback resolver runs — no Unix regression.
  • Test diff is a rewrite, not a coverage deletion — 1 unit test + 1 spawn-integration test added, 1 diagnostic-path test removed (see finding below).

Findings

  • [blocker before merge] CI Format job (oxfmt --check) fails on tts.spawn.test.mjs. CI log: "Format issues found in above 1 files. Run without --check to fix." mergeStateStatus=BLOCKED reflects this. All other required jobs are skipping (media-use skill files don't trigger them via path filter) except CodeQL / Analyze / Detect changes / Skills manifest / Test: skills, all of which pass. Purely a bun run format away.
  • [note / test-coverage regression] The removed test asserted the warn-once diagnostic (errors.length===1, matches /npm_execpath/) fires exactly once when npx can't be resolved. The one-shot latch (_warnedNpxResolution, _resetNpxResolutionWarnForTests) is still in production code, but its behavior is now untested. Consider re-adding a test where pathExists returns false for both branches to keep the diagnostic pinned; without one, a future refactor could silently un-latch the warning and cause log spam.

Diff-scope: +38/-40 (net −2), 3 files. Base 2aadf450 is a real master merge-base.

— Review by tai (pr-review)

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verdict: 🟡 LGTM-conditional at 7536fbd

Concurring with @terencecho on the Windows path handling + command-injection audit. The fallback resolves the npx JS CLI beside node's own directory — layout is uniform across Node distributions on Windows, macOS, and Linux (node sits beside node_modules/npm/), so no process.platform branch is required. resolveSpawnCommand still returns null only when both npm_execpath and the beside-node fallback fail, preserving the one-shot diagnostic in spawnP.

Regression test executes spawnP with npm_execpath unset and asserts the resolver picks the beside-node path, calls node with argv [npxCli, ...], and returns status:0. The deleted "diagnostic-when-unresolvable" test only covered a behavior this fix now supersedes — no functional coverage lost (deleted-guard reachability check per feedback_deleted_guard_verify_feature_reachability: the diagnostic path itself is retained, only its test was pruned).

One nit beyond Terence's check: the retained diagnostic message ("npm_execpath is not set") now lies slightly when npm_execpath IS set but points nowhere AND the beside-node fallback also misses. Consider softening to "npx cli could not be resolved via npm_execpath or beside-node fallback" in a follow-up.

Ship-blocker to flag (also flagged by Terence): CI red on Format — needs a bun run format before merge.

R1 by Via

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

@via @tai Addressed the actionable feedback at 8eb2d52:

  • Formatted the changed files (Format CI should now pass).
  • Restored regression coverage for the one-shot unresolved-npx warning, including both calls and the latch.
  • Corrected the diagnostic to distinguish npm_execpath unset from a configured-but-invalid path and mention the beside-node fallback.

Targeted validation: node --test skills/media-use/audio/scripts/lib/tts.spawn.test.mjs (9/9 passed). Please re-review current head.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Follow-up at 30f2e80: regenerated skills-manifest.json with the repository generator after the fresh manifest check reported the media-use hash mismatch. Fresh CI rerun is in progress.

@miguel-heygen miguel-heygen force-pushed the fix/windows-media-npx branch from 30f2e80 to fa88b3c Compare July 11, 2026 22:34
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main at 9d91c2a23 after the stack merges. Resolved only the generated skills-manifest conflict, preserved all npx fallback/diagnostic changes, regenerated skills-manifest.json. Final head: fa88b3c3c. @via @tai please re-review this current head; fresh CI is running.

@miguel-heygen miguel-heygen merged commit eba9ff9 into main Jul 11, 2026
40 checks passed
@miguel-heygen miguel-heygen deleted the fix/windows-media-npx branch July 11, 2026 22:36
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.

3 participants