fix(media): resolve npx without npm_execpath on Windows#2236
Conversation
terencecho
left a comment
There was a problem hiding this comment.
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")usesnode:path.join, which is platform-aware;dirname(process.execPath)on Windows yieldsC:\Program Files\nodejs. Spaces in the Windows path are safe because args are passed tospawnas an array, not a shell string —resolveSpawnCommandstill avoidsshell: trueand never touches.cmd. - Command injection audit. No
exec/execSync/shell: trueanywhere in the diff.spawnargs remain[npxCliPath, ...args.map(String)]— argv boundaries preserved. - Unix regression check.
resolveNpxCliPathcallsresolveNpxCliFromNpmExecPathfirst; whennpm_execpathis set (normal Unixnpm run …invocation), behavior is byte-identical to before. Non-win32branch ofresolveSpawnCommandstill 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 ontts.spawn.test.mjs. CI log: "Format issues found in above 1 files. Run without--checkto fix."mergeStateStatus=BLOCKEDreflects this. All other required jobs areskipping(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 abun run formataway. - [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 wherepathExistsreturns 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
left a comment
There was a problem hiding this comment.
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
|
@via @tai Addressed the actionable feedback at 8eb2d52:
Targeted validation: |
|
Follow-up at 30f2e80: regenerated |
30f2e80 to
fa88b3c
Compare
What
npx-cli.jsbeside the active Node executable whennpm_execpathis unsetnode audio.mjsWindows launches with a regression testWhy
Standalone media scripts are commonly launched with plain Node, so
npm_execpathis 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_execpathwhen 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.cmdshell execution.Test plan
node --test skills/media-use/audio/scripts/lib/tts.spawn.test.mjs(8/8)