Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion skills-manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"files": 10
},
"media-use": {
"hash": "389942983c2c78c2",
"hash": "e3f818afb0afbb34",
"files": 122
},
"motion-graphics": {
Expand Down
32 changes: 22 additions & 10 deletions skills/media-use/audio/scripts/lib/tts.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,17 @@ export function resolveNpxCliFromNpmExecPath(
return pathExists(npxCliPath) ? npxCliPath : null;
}

export function resolveNpxCliPath(
npmExecPath = process.env.npm_execpath,
nodeExecPath = process.env.npm_node_execpath || process.execPath,
pathExists = existsSync,
) {
const fromNpm = resolveNpxCliFromNpmExecPath(npmExecPath, pathExists);
if (fromNpm) return fromNpm;
const besideNode = join(dirname(nodeExecPath), "node_modules", "npm", "bin", "npx-cli.js");
return pathExists(besideNode) ? besideNode : null;
}

export function resolveSpawnCommand(
cmd,
args,
Expand All @@ -143,10 +154,11 @@ export function resolveSpawnCommand(
// On Windows, npx resolves to npx.cmd, which Node cannot execute directly.
// Avoid `shell:true` and the .cmd shim entirely by invoking npm's JS CLI with
// node, preserving request-provided values as argv data instead of shell text.
const npxCliPath = resolveNpxCliFromNpmExecPath(env.npm_execpath, pathExists);
const nodeExecPath = env.npm_node_execpath || process.execPath;
const npxCliPath = resolveNpxCliPath(env.npm_execpath, nodeExecPath, pathExists);
if (!npxCliPath) return null;
return {
cmd: env.npm_node_execpath || process.execPath,
cmd: nodeExecPath,
args: [npxCliPath, ...args.map((arg) => String(arg))],
opts: { stdio: "ignore", windowsHide: true, ...opts },
};
Expand Down Expand Up @@ -174,17 +186,17 @@ export function spawnP(
const resolved = resolveSpawnCommand(cmd, args, opts, platform, env, pathExists);
if (!resolved) {
// resolveSpawnCommand only returns null for the npx-on-win32 case where
// npm_execpath isn't set (e.g. audio.mjs invoked directly with `node`, not
// through npm/npx). Without this, every call silently returns status:-1 and
// stdio:"ignore" hides why — callers just report "TTS failed - omitted" for
// every line. Surface the real reason once so it's diagnosable.
// neither npm's configured CLI nor the beside-node fallback exists. Without
// this, every call silently returns status:-1 and stdio:"ignore" hides why.
if (!_warnedNpxResolution) {
_warnedNpxResolution = true;
const reason = env.npm_execpath
? `npm_execpath (${env.npm_execpath}) and the beside-node npm fallback could not be found`
: "npm_execpath is unset and the beside-node npm fallback could not be found";
console.error(
`[media-use] Cannot run "${cmd}" on Windows: npm_execpath is not set, so the ` +
`npx JS CLI can't be located. This happens when this script is run directly with ` +
`\`node\` instead of through npm/npx. Every "${cmd}" call is being skipped. ` +
`Fix: run via \`npx\`/\`npm run\`, or export npm_execpath pointing at your npm-cli.js.`,
`[media-use] Cannot run "${cmd}" on Windows: ${reason}. ` +
`Every "${cmd}" call is being skipped. Install npm with Node, or run via ` +
`\`npx\`/\`npm run\` with a valid npm_execpath.`,
);
}
return Promise.resolve({ status: -1 });
Expand Down
75 changes: 45 additions & 30 deletions skills/media-use/audio/scripts/lib/tts.spawn.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import assert from "node:assert/strict";
import { EventEmitter } from "node:events";
import {
resolveNpxCliFromNpmExecPath,
resolveNpxCliPath,
resolveSpawnCommand,
spawnP,
_resetNpxResolutionWarnForTests,
Expand Down Expand Up @@ -33,6 +34,15 @@ test("resolveNpxCliFromNpmExecPath finds npx-cli next to npm-cli", () => {
assert.equal(resolveNpxCliFromNpmExecPath(envWithNpxCli.npm_execpath, pathExists), npxCliPath);
});

test("resolveNpxCliPath finds npx-cli beside node when npm_execpath is unset", () => {
const node = "C:/Program Files/nodejs/node.exe";
const expected = "C:/Program Files/nodejs/node_modules/npm/bin/npx-cli.js";
assert.equal(
resolveNpxCliPath(undefined, node, (path) => path === expected),
expected,
);
});

test("resolveSpawnCommand routes npx through node+npx-cli on win32 without shell:true", () => {
const resolved = resolveSpawnCommand(
"npx",
Expand Down Expand Up @@ -101,43 +111,48 @@ test("spawnP does not enable shell for non-npx commands even on win32", async ()
assert.equal(captured[0].opts.shell, undefined);
});

// Regression: win32 + npx with npm_execpath unset can't locate the npx JS CLI,
// so resolveSpawnCommand returns null and spawnP short-circuits. Previously it
// returned {status:-1} silently — every TTS line just dropped as "TTS failed -
// omitted" with no hint. Now it must surface a clear one-time diagnostic naming
// npm_execpath, while still returning {status:-1} without spawning anything.
test("spawnP surfaces a clear diagnostic (once) when npx can't be resolved on win32", async () => {
test("spawnP resolves npx beside node when npm_execpath is unset on win32", async () => {
_resetNpxResolutionWarnForTests();
const captured = [];
const node = "C:/Program Files/nodejs/node.exe";
const npxCli = "C:/Program Files/nodejs/node_modules/npm/bin/npx-cli.js";
const result = await spawnP(
"npx",
["hyperframes", "tts"],
{},
"win32",
fakeSpawn(captured),
{ npm_node_execpath: node },
(path) => path === npxCli,
);
assert.equal(result.status, 0);
assert.equal(captured.length, 1);
assert.equal(captured[0].cmd, node);
assert.deepEqual(captured[0].args, [npxCli, "hyperframes", "tts"]);
});

test("spawnP warns once with an accurate diagnostic when neither npx path exists", async () => {
_resetNpxResolutionWarnForTests();
const errors = [];
const originalError = console.error;
console.error = (msg) => errors.push(msg);
const captured = [];
const emptyEnv = {}; // no npm_execpath
console.error = (message) => errors.push(String(message));
try {
const r1 = await spawnP(
"npx",
["hyperframes", "tts"],
{},
"win32",
fakeSpawn(captured),
emptyEnv,
() => false,
const env = { npm_execpath: "C:/missing/npm-cli.js", npm_node_execpath: "C:/node/node.exe" };
const missing = () => false;
assert.equal(
(await spawnP("npx", ["hyperframes", "tts"], {}, "win32", fakeSpawn([]), env, missing))
.status,
-1,
);
const r2 = await spawnP(
"npx",
["hyperframes", "tts"],
{},
"win32",
fakeSpawn(captured),
emptyEnv,
() => false,
assert.equal(
(await spawnP("npx", ["hyperframes", "tts"], {}, "win32", fakeSpawn([]), env, missing))
.status,
-1,
);
assert.equal(r1.status, -1);
assert.equal(r2.status, -1);
assert.equal(captured.length, 0, "must not spawn anything when resolution fails");
assert.equal(errors.length, 1, "diagnostic is emitted once for a batch, not per line");
assert.match(errors[0], /npm_execpath/);
} finally {
console.error = originalError;
}
assert.equal(errors.length, 1);
assert.match(errors[0], /npm_execpath \(C:\/missing\/npm-cli\.js\)/);
assert.doesNotMatch(errors[0], /npm_execpath is not set/);
});
Loading