diff --git a/.claude/skills/developing-insta-cli/SKILL.md b/.claude/skills/developing-insta-cli/SKILL.md index 1873cba..3bbb4d5 100644 --- a/.claude/skills/developing-insta-cli/SKILL.md +++ b/.claude/skills/developing-insta-cli/SKILL.md @@ -118,7 +118,7 @@ skipped because its argv is split before commander ever sees it — there, pass | `npm error code EOTP` | Publish needs `--otp=` | | `npm publish` ENOENT package.json | Ran outside the repo root | | `npx insta@latest` behind the GH release | `publish-npm` job failed (OIDC trust/config?) — see step 4 | -| `'C:\Program' is not recognized` from a spawned tool (win CI only) | `resolveSpawnable`'s cmd.exe hop strips the quotes around a spaced executable path (`C:\Program Files\…`). It exists for npm-installed `.cmd` shims — a real `.exe` (git, …) must be spawned directly, which finds it through PATHEXT anyway | +| `'C:\Program' is not recognized` from a spawned tool (win CI only) | Callers of `resolveSpawnable` must pass its `windowsVerbatimArguments` option to `spawn`; cmd.exe needs the resolver's outer command quotes and inner argument quotes intact. Spawn real `.exe` files directly. | | CLI hits the wrong server in tests | Persisted `~/.insta/config.json` apiUrl; set `INSTA_API_URL` (≥0.0.7) or move the config aside | | `insta --api-url X compute exec …` works but `insta compute exec --api-url X …` says unknown option | exec's argv is split before commander (`splitExecArgs`); pass `--api-url` at the root for exec | diff --git a/src/commands/setup.ts b/src/commands/setup.ts index 7006250..a3e9199 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -172,7 +172,7 @@ export async function ensureCliInstalled( // guidance. Ignoring stdin keeps the installer's own output intact. (-y means no prompt anyway.) const defaultRunner: Runner = (cmdIn, argsIn) => new Promise((resolve) => { - const { cmd, args } = resolveSpawnable(cmdIn, argsIn) + const { cmd, args, windowsVerbatimArguments } = resolveSpawnable(cmdIn, argsIn) const env: NodeJS.ProcessEnv = { ...process.env, AI_AGENT: process.env.AI_AGENT || 'insta', FORCE_COLOR: '0' } // When THIS process was launched by npx, npx exports its flags as npm_config_* env vars. // npm_config_package pins package resolution for every nested npm/npx child — the inner @@ -181,7 +181,7 @@ const defaultRunner: Runner = (cmdIn, argsIn) => // (deliberate user configuration). delete env.npm_config_package delete env.npm_config_call - const p = spawn(cmd, args, { stdio: ['ignore', 'pipe', 'pipe'], env }) + const p = spawn(cmd, args, { stdio: ['ignore', 'pipe', 'pipe'], env, windowsVerbatimArguments }) let output = '' const grab = (chunk: Buffer) => { output += chunk.toString() } p.stdout?.on('data', grab) diff --git a/src/commands/upgrade.ts b/src/commands/upgrade.ts index 7e26e1f..041052f 100644 --- a/src/commands/upgrade.ts +++ b/src/commands/upgrade.ts @@ -46,7 +46,7 @@ export function resolveRunSpec( npmExecpath = spec.env.npm_execpath, execPath = process.execPath, platform: NodeJS.Platform = process.platform, -): RunSpec { +): RunSpec & { windowsVerbatimArguments?: boolean } { const resolved = resolveSpawnable(spec.cmd, spec.args, npmExecpath, execPath, platform, spec.env) return { ...spec, ...resolved } } @@ -220,7 +220,7 @@ export function autoEnabled(home = homedir()): boolean { const spawnRun: Runner = ({ cmd, args, env }) => new Promise((resolve, reject) => { const resolved = resolveRunSpec({ cmd, args, env }) - const p = spawn(resolved.cmd, resolved.args, { stdio: 'inherit', env }) + const p = spawn(resolved.cmd, resolved.args, { stdio: 'inherit', env, windowsVerbatimArguments: resolved.windowsVerbatimArguments }) p.on('error', reject) p.on('close', (code) => (code === 0 ? resolve() : reject(new Error(`upgrade failed (exit ${code})`)))) }) @@ -238,7 +238,7 @@ const observeInstalled: Observer = (channel, installDir) => // INSTA_NO_AUTOUPDATE: the child must not kick off yet another upgrade while we are reading it. const env: NodeJS.ProcessEnv = { ...process.env, INSTA_NO_AUTOUPDATE: '1' } const resolved = resolveRunSpec({ cmd, args: ['--version'], env }) - const p = spawn(resolved.cmd, resolved.args, { stdio: ['ignore', 'pipe', 'ignore'], env }) + const p = spawn(resolved.cmd, resolved.args, { stdio: ['ignore', 'pipe', 'ignore'], env, windowsVerbatimArguments: resolved.windowsVerbatimArguments }) p.stdout.on('data', (d) => (out += String(d))) p.on('error', () => resolve(null)) p.on('close', () => { diff --git a/src/ensure-skills.ts b/src/ensure-skills.ts index 9bc6759..56f6a0b 100644 --- a/src/ensure-skills.ts +++ b/src/ensure-skills.ts @@ -34,13 +34,13 @@ const defaultRunner: Runner = (cmdIn, argsIn, inherit = false) => new Promise((resolve) => { // resolveSpawnable: on Windows `npx` is a .cmd shim spawn() refuses without a shell — // re-enter npm's CLI script via node instead (same treatment as `insta agent setup`). - const { cmd, args } = resolveSpawnable(cmdIn, argsIn) + const { cmd, args, windowsVerbatimArguments } = resolveSpawnable(cmdIn, argsIn) const env: NodeJS.ProcessEnv = { ...process.env, AI_AGENT: process.env.AI_AGENT || 'insta' } // npx exports its flags as npm_config_* to children; npm_config_package would pin the inner // `npx -y skills …` to whatever package launched this CLI (see setup.ts defaultRunner). delete env.npm_config_package delete env.npm_config_call - const p = spawn(cmd, args, { stdio: inherit ? 'inherit' : 'ignore', env }) + const p = spawn(cmd, args, { stdio: inherit ? 'inherit' : 'ignore', env, windowsVerbatimArguments }) p.on('error', () => resolve({ ok: false })) // e.g. npx not on PATH p.on('close', (code) => resolve({ ok: code === 0 })) }) diff --git a/src/spawn.ts b/src/spawn.ts index aba593d..4af8ae7 100644 --- a/src/spawn.ts +++ b/src/spawn.ts @@ -54,7 +54,7 @@ export function resolveSpawnable( platform: NodeJS.Platform = process.platform, env: NodeJS.ProcessEnv = process.env, systemRoot: string = process.env.SYSTEMROOT ?? process.env.windir ?? 'C:\\Windows', -): { cmd: string; args: string[] } { +): { cmd: string; args: string[]; windowsVerbatimArguments?: boolean } { const execIsNode = /(^|[\\/])node(\.exe)?$/i.test(execPath) if ((cmd === 'npm' || cmd === 'npx') && execIsNode) { if (npmExecpath && /(^|[\\/])np[mx](-cli)?\.[cm]?js$/.test(npmExecpath)) { @@ -69,13 +69,16 @@ export function resolveSpawnable( } const bareShim = !/[\\/]/.test(cmd) && !/\.exe$/i.test(cmd) - if (platform === 'win32' && bareShim && !args.some((arg) => /[&|<>^%"]/.test(arg))) { + if (platform === 'win32' && bareShim && !args.some((arg) => /[&|<>^%"\r\n]/.test(arg))) { const absolute = whichOnPath(cmd, env, platform) - // The resolved path itself also becomes cmd.exe input. If a PATH directory contains a shell - // metacharacter, fail via the caller's normal bare-spawn fallback rather than interpret it. - // Pin cmd.exe to System32 too: CreateProcess-style lookup checks cwd before PATH. - if (absolute && !/[&|<>^%"]/.test(absolute)) { - return { cmd: win32.join(systemRoot, 'System32', 'cmd.exe'), args: ['/d', '/s', '/c', absolute, ...args] } + if (absolute && !/[&|<>^%"\r\n]/.test(absolute)) { + const command = [absolute, ...args].map((arg) => `"${arg.replace(/\\+$/, '$&$&')}"`).join(' ') + // cmd /s consumes the outer quotes; libuv must not escape the inner argument quotes. + return { + cmd: win32.join(systemRoot, 'System32', 'cmd.exe'), + args: ['/d', '/v:off', '/s', '/c', `"${command}"`], + windowsVerbatimArguments: true, + } } } return { cmd, args } diff --git a/test/self-install.test.ts b/test/self-install.test.ts index 06113e4..4b138d0 100644 --- a/test/self-install.test.ts +++ b/test/self-install.test.ts @@ -164,14 +164,12 @@ test('resolveSpawnable re-enters npm/npx as node scripts so Windows .cmd shims a // Non-npm commands on Windows (claude is an npm-installed .cmd shim too) go through cmd.exe, // resolved to the shim's ABSOLUTE PATH first — cmd.exe searches the current directory before // PATH, so a bare name would let a claude.cmd planted in the project dir shadow the real CLI. - // Args stay UNquoted: libuv quotes spaced args when building the child command line, so - // pre-quoting would double up and deliver literal quote characters to the target. const shimBin = mkdtempSync(join(tmpdir(), 'insta-shim-')) const claudeShim = join(shimBin, 'claude.CMD') writeFileSync(claudeShim, '@echo off\n') const winEnv = { PATH: shimBin, PATHEXT: '.COM;.EXE;.BAT;.CMD' } expect(resolveSpawnable('claude', ['mcp', 'add', '--header', 'Authorization: Bearer x'], npmCli, winNode, 'win32', winEnv, 'C:\\Windows')) - .toEqual({ cmd: 'C:\\Windows\\System32\\cmd.exe', args: ['/d', '/s', '/c', claudeShim, 'mcp', 'add', '--header', 'Authorization: Bearer x'] }) + .toEqual({ cmd: 'C:\\Windows\\System32\\cmd.exe', args: ['/d', '/v:off', '/s', '/c', `""${claudeShim}" "mcp" "add" "--header" "Authorization: Bearer x""`], windowsVerbatimArguments: true }) // Not on PATH → passthrough; the bare spawn fails and the probe treats it as not-installed. expect(resolveSpawnable('claude', ['--version'], npmCli, winNode, 'win32', { PATH: '' })) .toEqual({ cmd: 'claude', args: ['--version'] }) diff --git a/test/spawn-windows.test.ts b/test/spawn-windows.test.ts new file mode 100644 index 0000000..62a2e0a --- /dev/null +++ b/test/spawn-windows.test.ts @@ -0,0 +1,49 @@ +import { spawnSync } from 'node:child_process' +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, win32 } from 'node:path' +import { expect, test } from 'vitest' +import { resolveSpawnable } from '../src/spawn.js' +import { resolveRunSpec } from '../src/commands/upgrade.js' + +test('native Windows shim invocations preserve their quoted command line', () => { + const dir = mkdtempSync(join(tmpdir(), 'insta spaced ')) + try { + const shim = join(dir, 'npx.CMD') + writeFileSync(shim, '@echo off\r\n') + expect(resolveSpawnable('npx', ['-y', 'two words', '', 'C:\\work dir\\'], '', 'insta.exe', 'win32', { PATH: dir, PATHEXT: '.CMD' }, 'C:\\Windows')) + .toEqual({ + cmd: 'C:\\Windows\\System32\\cmd.exe', + args: ['/d', '/v:off', '/s', '/c', `""${shim}" "-y" "two words" "" "C:\\work dir\\\\""`], + windowsVerbatimArguments: true, + }) + } finally { rmSync(dir, { recursive: true, force: true }) } +}) + +test.each(['&', '|', '<', '>', '^', '%', '"', '\r', '\n'])( + 'Windows shim wrapping refuses shell syntax %j in arguments', (character) => { + const dir = mkdtempSync(join(tmpdir(), 'insta-shim-')) + try { + writeFileSync(join(dir, 'npx.CMD'), '@echo off\r\n') + const args = ['first', `x${character}y`] + expect(resolveSpawnable('npx', args, '', 'insta.exe', 'win32', { PATH: dir, PATHEXT: '.CMD' })) + .toEqual({ cmd: 'npx', args }) + } finally { rmSync(dir, { recursive: true, force: true }) } + }, +) + +test.skipIf(process.platform !== 'win32')('Windows executes spaced npm shims with literal arguments', () => { + const dir = mkdtempSync(join(tmpdir(), 'insta spaced (shim)! ')) + try { + writeFileSync(join(dir, 'npx.CMD'), '@echo off\r\n"%INSTA_TEST_NODE%" "%~dp0argv.cjs" %*\r\n') + writeFileSync(join(dir, 'argv.cjs'), 'console.log(JSON.stringify(process.argv.slice(2)))\n') + const env = { ...process.env, PATH: dir, PATHEXT: '.CMD', INSTA_TEST_NODE: process.execPath, INSTA_TEST_EXPAND: 'expanded' } + const args = ['-y', 'skills', 'add', 'two words', '', 'C:\\work dir\\', '*', '(parentheses)', '!INSTA_TEST_EXPAND!'] + const spec = resolveRunSpec({ cmd: 'npx', args, env }, '', join(dir, 'insta.exe'), 'win32') + const result = spawnSync(spec.cmd, spec.args, { env, encoding: 'utf8', windowsVerbatimArguments: spec.windowsVerbatimArguments }) + expect(result.error).toBeUndefined() + expect(result.status, result.stderr).toBe(0) + expect(JSON.parse(result.stdout.trim())).toEqual(args) + expect(spec.cmd).toBe(win32.join(process.env.SYSTEMROOT ?? process.env.windir ?? 'C:\\Windows', 'System32', 'cmd.exe')) + } finally { rmSync(dir, { recursive: true, force: true }) } +}) diff --git a/test/upgrade-resolver.test.ts b/test/upgrade-resolver.test.ts index c5f447c..4f77062 100644 --- a/test/upgrade-resolver.test.ts +++ b/test/upgrade-resolver.test.ts @@ -55,7 +55,7 @@ test('the Windows version probe resolves the installed shim through a copied `Pa const shim = join(shimBin, 'insta.CMD') writeFileSync(shim, '@echo off\n') const cmdExe = win32.join(process.env.SYSTEMROOT ?? process.env.windir ?? 'C:\\Windows', 'System32', 'cmd.exe') - const wrapped = { cmd: cmdExe, args: ['/d', '/s', '/c', shim, '--version'] } + const wrapped = { cmd: cmdExe, args: ['/d', '/v:off', '/s', '/c', `""${shim}" "--version""`], windowsVerbatimArguments: true } // Windows' real casing, as a spread of process.env preserves it. const copied = { Path: shimBin, PATHEXT: '.CMD', INSTA_NO_AUTOUPDATE: '1' }