From b5d60a18011377c977c48412921c46573ba04f11 Mon Sep 17 00:00:00 2001 From: Hoang Dinh Date: Sat, 18 Jul 2026 12:03:29 +1000 Subject: [PATCH 1/5] fix: pass external check arguments as argv --- src/checks/external.test.ts | 45 +++++++++++++++++---- src/checks/external.ts | 16 ++++---- src/checks/maxWarnings.ts | 6 +-- src/shared/spawn.test.ts | 43 +++++++++++++++++++- src/shared/spawn.ts | 80 ++++++++++++++++++++++++++++++++----- 5 files changed, 160 insertions(+), 30 deletions(-) diff --git a/src/checks/external.test.ts b/src/checks/external.test.ts index ab7ee62..e0f8f9a 100644 --- a/src/checks/external.test.ts +++ b/src/checks/external.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import { runCaptured } from '../shared/output.ts' -import { appendArgs, defineExternalCheck, externalFailureHint, runCountedBudget, selectCommand } from './external.ts' +import { buildArgv, defineExternalCheck, externalFailureHint, formatCommand, runCountedBudget, selectCommand } from './external.ts' const fixable = { checkCommand: 'oxfmt --check .', fixCommand: 'oxfmt .' } const notFixable = { checkCommand: 'tsc --noEmit' } @@ -38,14 +38,45 @@ describe('externalFailureHint', () => { }) }) -describe('appendArgs', () => { - it('appends passthrough args verbatim, unquoted (so shell globs still expand)', () => { - expect(appendArgs('skott --showCircularDependencies', ['src/*.ts'])).toBe('skott --showCircularDependencies src/*.ts') +describe('buildArgv', () => { + it('tokenises the base command, keeping a quoted default as a single literal entry', () => { + expect(buildArgv('jscpd --format typescript,tsx --ignore "**/*.test.*" -r consoleFull src')).toEqual([ + 'jscpd', + '--format', + 'typescript,tsx', + '--ignore', + '**/*.test.*', + '-r', + 'consoleFull', + 'src', + ]) }) - it('returns the command unchanged when there are no extra args', () => { - expect(appendArgs('oxlint .', [])).toBe('oxlint .') - expect(appendArgs('oxlint .')).toBe('oxlint .') + it('appends each passthrough arg as its own literal entry (globs and spaces are never split or expanded)', () => { + expect(buildArgv('jscpd --ignore "**/*.test.*" src', ['--ignore', '**/generated/**', 'has space'])).toEqual([ + 'jscpd', + '--ignore', + '**/*.test.*', + 'src', + '--ignore', + '**/generated/**', + 'has space', + ]) + }) + + it('preserves shell metacharacters in passthrough args verbatim', () => { + expect(buildArgv('tool', ['$HOME', ';whoami', 'a|b', '`id`'])).toEqual(['tool', '$HOME', ';whoami', 'a|b', '`id`']) + }) + + it('returns just the tokenised command when there are no extra args', () => { + expect(buildArgv('oxlint .', [])).toEqual(['oxlint', '.']) + expect(buildArgv('oxlint .')).toEqual(['oxlint', '.']) + }) +}) + +describe('formatCommand', () => { + it('renders a copy-pasteable, quoted command line for diagnostics without changing what runs', () => { + expect(formatCommand(['jscpd', '--ignore', '**/generated/**', 'has space'])).toBe('jscpd --ignore "**/generated/**" "has space"') }) }) diff --git a/src/checks/external.ts b/src/checks/external.ts index e43230f..0cb538f 100644 --- a/src/checks/external.ts +++ b/src/checks/external.ts @@ -4,11 +4,11 @@ import path from 'node:path' import { color } from '../shared/color.ts' import { resolveMode } from '../shared/mode.ts' import { emit } from '../shared/output.ts' -import { appendArgs, runCommand } from '../shared/spawn.ts' +import { buildArgv, formatCommand, runCommand } from '../shared/spawn.ts' import { type MaxWarningsSupport, withinBudget } from './maxWarnings.ts' import type { Check, CheckMode, CheckResult, RunDefaultOptions } from './types.ts' -export { appendArgs } from '../shared/spawn.ts' +export { buildArgv, formatCommand } from '../shared/spawn.ts' const BIN_EXTENSIONS = ['', '.cmd', '.ps1', '.exe'] @@ -95,7 +95,7 @@ export async function runCountedBudget( const unit = count === 1 ? budget.unit : `${budget.unit}s` console.error(color.dim(`↳ ${spec.name}: ${count} ${unit} found, exceeds --max-warnings ${maxWarnings}.`)) if (report) emit(spec.transformOutput ? spec.transformOutput(report) : report) - console.error(color.dim(externalFailureHint(spec, appendArgs(spec.checkCommand, extraArgs)))) + console.error(color.dim(externalFailureHint(spec, formatCommand(buildArgv(spec.checkCommand, extraArgs))))) return { name: spec.name, ok: false } } @@ -124,17 +124,17 @@ export function defineExternalCheck(spec: ExternalCheckSpec): Check { return { name: spec.name, ok: true, skipped: true } } // quiet: buffer the tool's output and flush only on failure (streamed live under --verbose). - const runReport = async (command: string): Promise => { - const code = await runCommand(command, { env: envWithLocalBin(), quiet: true, transform: spec.transformOutput }) - if (code !== 0) console.error(color.dim(externalFailureHint(spec, command))) + const runReport = async (argv: string[]): Promise => { + const code = await runCommand(argv, { env: envWithLocalBin(), quiet: true, transform: spec.transformOutput }) + if (code !== 0) console.error(color.dim(externalFailureHint(spec, formatCommand(argv)))) return { name: spec.name, ok: code === 0 } } if (maxWarnings !== undefined && spec.maxWarnings) { const budget = spec.maxWarnings if (budget.strategy === 'count') return runCountedBudget(spec, budget, maxWarnings, extraArgs, envWithLocalBin()) - return runReport(appendArgs(selectCommand(spec, resolveMode()), [...extraArgs, ...budget.toArgs(maxWarnings)])) + return runReport(buildArgv(selectCommand(spec, resolveMode()), [...extraArgs, ...budget.toArgs(maxWarnings)])) } - return runReport(appendArgs(selectCommand(spec, resolveMode()), extraArgs)) + return runReport(buildArgv(selectCommand(spec, resolveMode()), extraArgs)) }, } } diff --git a/src/checks/maxWarnings.ts b/src/checks/maxWarnings.ts index 3cf852b..0c9c180 100644 --- a/src/checks/maxWarnings.ts +++ b/src/checks/maxWarnings.ts @@ -2,7 +2,7 @@ import fs from 'node:fs' import os from 'node:os' import path from 'node:path' -import { appendArgs, captureCommand } from '../shared/spawn.ts' +import { buildArgv, captureCommand } from '../shared/spawn.ts' type MaxWarningsCountContext = { extraArgs: string[]; env: Record; checkCommand: string } @@ -29,8 +29,8 @@ export function countJscpdClones(report: { statistics?: { total?: { clones?: unk export async function jscpdCount(ctx: MaxWarningsCountContext): Promise { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'verifyx-jscpd-')) try { - const command = `${appendArgs(ctx.checkCommand, ctx.extraArgs)} --reporters json --output "${dir}"` - const { code, stdout, stderr } = await captureCommand(command, { env: ctx.env }) + const argv = [...buildArgv(ctx.checkCommand, ctx.extraArgs), '--reporters', 'json', '--output', dir] + const { code, stdout, stderr } = await captureCommand(argv, { env: ctx.env }) const reportPath = path.join(dir, 'jscpd-report.json') if (!fs.existsSync(reportPath)) throw new Error(`jscpd produced no report (exit ${code})${stderr.trim() ? `: ${stderr.trim()}` : ''}`) const count = countJscpdClones(JSON.parse(fs.readFileSync(reportPath, 'utf8'))) diff --git a/src/shared/spawn.test.ts b/src/shared/spawn.test.ts index ee7b992..f8fab24 100644 --- a/src/shared/spawn.test.ts +++ b/src/shared/spawn.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest' -import { captureCommand } from './spawn.ts' +import { buildArgv, captureCommand, formatCommand, tokenizeCommand } from './spawn.ts' describe('captureCommand', () => { it('returns the captured stdout and a zero exit code for a successful command', async () => { @@ -19,4 +19,45 @@ describe('captureCommand', () => { expect(stdout).toBe('') expect(stderr).toContain('to-stderr') }) + + // Regression: forwarded passthrough args used to be joined into a shell string, so the spawn shell + // glob-expanded/word-split them before the tool saw them. An argv array must reach the tool verbatim. + it('passes an argv array to the process with no shell, so globs and metacharacters arrive literally', async () => { + const echoArgv = 'process.stdout.write(JSON.stringify(process.argv.slice(1)))' + const passthrough = ['--ignore', '**/generated/**', 'has space', '$HOME', ';whoami'] + const { code, stdout } = await captureCommand(['node', '-e', echoArgv, '--', ...passthrough]) + expect(code).toBe(0) + expect(JSON.parse(stdout)).toEqual(passthrough) + }) +}) + +describe('tokenizeCommand', () => { + it('splits on whitespace and strips quotes, keeping a quoted arg as one entry', () => { + expect(tokenizeCommand('jscpd --ignore "**/*.test.*" -r consoleFull src')).toEqual([ + 'jscpd', + '--ignore', + '**/*.test.*', + '-r', + 'consoleFull', + 'src', + ]) + }) +}) + +describe('buildArgv', () => { + it('appends passthrough args as their own literal entries', () => { + expect(buildArgv('jscpd src', ['--ignore', '**/generated/**', 'has space'])).toEqual([ + 'jscpd', + 'src', + '--ignore', + '**/generated/**', + 'has space', + ]) + }) +}) + +describe('formatCommand', () => { + it('quotes only the entries that need it, for a copy-pasteable diagnostic line', () => { + expect(formatCommand(['jscpd', '--ignore', '**/generated/**', 'plain'])).toBe('jscpd --ignore "**/generated/**" plain') + }) }) diff --git a/src/shared/spawn.ts b/src/shared/spawn.ts index df141ed..5f6e013 100644 --- a/src/shared/spawn.ts +++ b/src/shared/spawn.ts @@ -2,9 +2,53 @@ import { spawn } from 'node:child_process' import { emit, isCapturing } from './output.ts' -// Keep passthrough globs unquoted for shell expansion. -export function appendArgs(command: string, extraArgs: readonly string[] = []): string { - return extraArgs.length > 0 ? `${command} ${extraArgs.join(' ')}` : command +// Honours single/double quotes so a quoted default like --ignore "**/*.test.*" stays one argv entry. Not a full shell parser. +export function tokenizeCommand(command: string): string[] { + const argv: string[] = [] + let current = '' + let started = false + let quote: '"' | "'" | null = null + for (const ch of command) { + if (quote) { + if (ch === quote) quote = null + else current += ch + continue + } + if (ch === '"' || ch === "'") { + quote = ch + started = true + continue + } + if (ch === ' ' || ch === '\t' || ch === '\n' || ch === '\r') { + if (started) { + argv.push(current) + current = '' + started = false + } + continue + } + current += ch + started = true + } + if (started) argv.push(current) + return argv +} + +// Passthrough args stay as their own literal entries (never re-parsed) so globs/spaces/metacharacters reach the tool intact. +export function buildArgv(command: string, extraArgs: readonly string[] = []): string[] { + return [...tokenizeCommand(command), ...extraArgs] +} + +const SAFE_ARG = /^[A-Za-z0-9_@%+=:,./-]+$/ + +function quoteForDisplay(arg: string): string { + if (arg.length > 0 && SAFE_ARG.test(arg)) return arg + return `"${arg.replace(/(["\\$`])/g, '\\$1')}"` +} + +/** Render an argv array as a copy-pasteable, safely-quoted command line for diagnostics only (never executed). */ +export function formatCommand(argv: readonly string[]): string { + return argv.map(quoteForDisplay).join(' ') } let verboseMode = false @@ -23,6 +67,18 @@ function shouldSuppress(quiet?: boolean): boolean { return !!quiet || !!process.env.CLAUDECODE } +// A string runs through a shell (consumer verify:*/npm run scripts); an argv array runs with NO shell so entries reach the tool verbatim. +export type Command = string | readonly string[] + +type SpawnInvocation = { file: string; args: string[]; shell: boolean } + +function toInvocation(command: Command): SpawnInvocation { + if (typeof command === 'string') return { file: command, args: [], shell: true } + const [file, ...args] = command + if (!file) throw new Error('argv command must have at least one entry (the executable)') + return { file, args, shell: false } +} + export type RunCommandOptions = { cwd?: string env?: Record @@ -32,15 +88,16 @@ export type RunCommandOptions = { } /** - * Run a shell command, returning its exit code. Suppressed output (quiet, or under Claude Code) is buffered - * and flushed to stdout only if the command fails, keeping passing runs quiet. + * Run a command, returning its exit code. Suppressed output (quiet, or under Claude Code) is buffered and + * flushed to stdout only if the command fails, keeping passing runs quiet. */ -export function runCommand(command: string, opts: RunCommandOptions = {}): Promise { +export function runCommand(command: Command, opts: RunCommandOptions = {}): Promise { return new Promise((resolve) => { const suppress = shouldSuppress(opts.quiet) - const child = spawn(command, [], { + const { file, args, shell } = toInvocation(command) + const child = spawn(file, args, { stdio: suppress ? 'pipe' : 'inherit', - shell: true, + shell, cwd: opts.cwd ?? process.cwd(), env: opts.env ? { ...process.env, ...opts.env } : undefined, }) @@ -63,13 +120,14 @@ export function runCommand(command: string, opts: RunCommandOptions = {}): Promi } export function captureCommand( - command: string, + command: Command, opts: { cwd?: string; env?: Record } = {}, ): Promise<{ code: number; stdout: string; stderr: string }> { return new Promise((resolve) => { - const child = spawn(command, [], { + const { file, args, shell } = toInvocation(command) + const child = spawn(file, args, { stdio: ['ignore', 'pipe', 'pipe'], - shell: true, + shell, cwd: opts.cwd ?? process.cwd(), env: opts.env ? { ...process.env, ...opts.env } : undefined, }) From 1ee8b597aa84f67d3944f39d72dd60a0a0690ffe Mon Sep 17 00:00:00 2001 From: Hoang Dinh Date: Sat, 18 Jul 2026 12:09:55 +1000 Subject: [PATCH 2/5] fix: support argv commands on Windows --- package-lock.json | 22 +++++++++++++++------- package.json | 2 ++ src/shared/spawn.test.ts | 30 ++++++++++++++++++++++++++++++ src/shared/spawn.ts | 9 ++++++--- 4 files changed, 53 insertions(+), 10 deletions(-) diff --git a/package-lock.json b/package-lock.json index 065a714..e4565a9 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,15 +10,17 @@ "license": "MIT", "dependencies": { "commander": "^15.0.0", + "cross-spawn": "^7.0.6", "enquirer": "^2.4.1", "minimatch": "^10.2.5", "typescript": "^6.0.3" }, "bin": { - "verify": "dist/cli.mjs" + "verifyx": "dist/cli.mjs" }, "devDependencies": { "@rollup/plugin-typescript": "^12.3.0", + "@types/cross-spawn": "^6.0.6", "@types/node": "^26.1.1", "@vitest/coverage-v8": "^4.1.10", "better-npm-audit": "^3.11.0", @@ -2710,6 +2712,16 @@ "assertion-error": "^2.0.1" } }, + "node_modules/@types/cross-spawn": { + "version": "6.0.6", + "resolved": "https://registry.npmjs.org/@types/cross-spawn/-/cross-spawn-6.0.6.tgz", + "integrity": "sha512-fXRhhUkG4H3TQk5dBhQ7m/JDdSNHKwR2BBia62lhwEIq9xGiQKLxd6LymNhn47SjXhsUEPmxi+PKw2OkW4LLjA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/deep-eql": { "version": "4.0.2", "dev": true, @@ -3416,7 +3428,8 @@ }, "node_modules/cross-spawn": { "version": "7.0.6", - "dev": true, + "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.6.tgz", + "integrity": "sha512-uV2QOWP2nWzsy2aMp8aRibhi9dlzF5Hgh5SHaB9OiTGEyDTiJJyx0uy51QXdyWbtAHNua4XJzUKca3OzKUd3vA==", "license": "MIT", "dependencies": { "path-key": "^3.1.0", @@ -3429,12 +3442,10 @@ }, "node_modules/cross-spawn/node_modules/isexe": { "version": "2.0.0", - "dev": true, "license": "ISC" }, "node_modules/cross-spawn/node_modules/which": { "version": "2.0.2", - "dev": true, "license": "ISC", "dependencies": { "isexe": "^2.0.0" @@ -5074,7 +5085,6 @@ }, "node_modules/path-key": { "version": "3.1.1", - "dev": true, "license": "MIT", "engines": { "node": ">=8" @@ -5432,7 +5442,6 @@ }, "node_modules/shebang-command": { "version": "2.0.0", - "dev": true, "license": "MIT", "dependencies": { "shebang-regex": "^3.0.0" @@ -5443,7 +5452,6 @@ }, "node_modules/shebang-regex": { "version": "3.0.0", - "dev": true, "license": "MIT", "engines": { "node": ">=8" diff --git a/package.json b/package.json index 5f57fff..f0da1d7 100644 --- a/package.json +++ b/package.json @@ -58,12 +58,14 @@ }, "dependencies": { "commander": "^15.0.0", + "cross-spawn": "^7.0.6", "enquirer": "^2.4.1", "minimatch": "^10.2.5", "typescript": "^6.0.3" }, "devDependencies": { "@rollup/plugin-typescript": "^12.3.0", + "@types/cross-spawn": "^6.0.6", "@types/node": "^26.1.1", "@vitest/coverage-v8": "^4.1.10", "better-npm-audit": "^3.11.0", diff --git a/src/shared/spawn.test.ts b/src/shared/spawn.test.ts index f8fab24..a73cc5d 100644 --- a/src/shared/spawn.test.ts +++ b/src/shared/spawn.test.ts @@ -1,3 +1,7 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' + import { describe, expect, it } from 'vitest' import { buildArgv, captureCommand, formatCommand, tokenizeCommand } from './spawn.ts' @@ -20,6 +24,12 @@ describe('captureCommand', () => { expect(stderr).toContain('to-stderr') }) + it('returns the spawn error when an argv executable cannot be launched', async () => { + const { code, stderr } = await captureCommand(['verifyx-command-that-does-not-exist']) + expect(code).toBe(127) + expect(stderr).toContain('ENOENT') + }) + // Regression: forwarded passthrough args used to be joined into a shell string, so the spawn shell // glob-expanded/word-split them before the tool saw them. An argv array must reach the tool verbatim. it('passes an argv array to the process with no shell, so globs and metacharacters arrive literally', async () => { @@ -29,6 +39,26 @@ describe('captureCommand', () => { expect(code).toBe(0) expect(JSON.parse(stdout)).toEqual(passthrough) }) + + it.runIf(process.platform === 'win32')('launches npm-style .cmd shims without interpreting their arguments', async () => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'verifyx-spawn-')) + const binDir = path.join(root, 'node_modules', '.bin') + fs.mkdirSync(binDir, { recursive: true }) + fs.writeFileSync(path.join(binDir, 'verifyx-echo-argv.cjs'), 'process.stdout.write(JSON.stringify(process.argv.slice(2)))') + fs.writeFileSync(path.join(binDir, 'verifyx-echo-argv.cmd'), '@ECHO off\r\nnode "%~dp0\\verifyx-echo-argv.cjs" %*\r\n') + + const pathKey = Object.keys(process.env).find((key) => key.toLowerCase() === 'path') ?? 'PATH' + const env = { [pathKey]: `${binDir}${path.delimiter}${process.env[pathKey] ?? ''}` } + const passthrough = ['**/*.ts', 'has space', '$HOME', '%PATH%', ';whoami', 'a&b'] + + try { + const { code, stdout, stderr } = await captureCommand(['verifyx-echo-argv', ...passthrough], { env }) + expect({ code, stderr }).toEqual({ code: 0, stderr: '' }) + expect(JSON.parse(stdout)).toEqual(passthrough) + } finally { + fs.rmSync(root, { recursive: true, force: true, maxRetries: 3 }) + } + }) }) describe('tokenizeCommand', () => { diff --git a/src/shared/spawn.ts b/src/shared/spawn.ts index 5f6e013..3836389 100644 --- a/src/shared/spawn.ts +++ b/src/shared/spawn.ts @@ -1,4 +1,4 @@ -import { spawn } from 'node:child_process' +import spawn from 'cross-spawn' import { emit, isCapturing } from './output.ts' @@ -115,7 +115,10 @@ export function runCommand(command: Command, opts: RunCommandOptions = {}): Prom } resolve(exitCode) }) - child.on('error', () => resolve(127)) + child.on('error', (error) => { + emit(`${String(error)}\n`, 'err') + resolve(127) + }) }) } @@ -138,6 +141,6 @@ export function captureCommand( child.on('close', (code) => { resolve({ code: code ?? 1, stdout: Buffer.concat(out).toString(), stderr: Buffer.concat(err).toString() }) }) - child.on('error', () => resolve({ code: 127, stdout: '', stderr: '' })) + child.on('error', (error) => resolve({ code: 127, stdout: '', stderr: String(error) })) }) } From 2a2cf2bb67204f3a36446a69e96dc1f95e9f41eb Mon Sep 17 00:00:00 2001 From: Hoang Dinh Date: Sat, 18 Jul 2026 13:02:01 +1000 Subject: [PATCH 3/5] fix: make external command handling cross-platform --- README.md | 10 +-- src/checks/external.test.ts | 87 ++++++++----------------- src/checks/external.ts | 43 ++++++------- src/checks/maxWarnings.ts | 8 +-- src/checks/registry.test.ts | 4 +- src/checks/registry.ts | 18 +++--- src/checks/types.ts | 4 +- src/orchestrator/run.ts | 4 +- src/orchestrator/runAll.ts | 6 +- src/scaffold/installDeps.ts | 6 +- src/shared/spawn.test.ts | 61 ++++++++++-------- src/shared/spawn.ts | 124 +++++++++++++++--------------------- 12 files changed, 160 insertions(+), 215 deletions(-) diff --git a/README.md b/README.md index 8d02dff..3f32d24 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ What makes it worth wiring in: - **Auto-fixes locally, fails in CI (same command).** Run it on your machine and it _fixes_ what it can (lint, formatting) instead of just complaining. Run it under CI and the identical command is check-only, so a PR can't merge with problems that should have been fixed. - **Silent when green, so it's cheap to loop.** A passing run prints nothing and exits `0`, with no output to burn an agent's tokens or bury the one failure that matters. Agents can run it as often as they like. -- **Failure output written for an agent to act on.** When a check fails it names the tool it ran, the exact command, and a docs link, so the agent (or you) knows what to fix and how instead of guessing. +- **Failure output written for an agent to act on.** When a check fails it names the tool it ran, the exact argv, and a docs link, so the agent (or you) knows what to fix and how instead of guessing. - **Convention over configuration.** Checks are just `verify:*` npm scripts run in parallel. Add, drop, or override any of them; there's no bespoke config format to learn. ## Install @@ -127,11 +127,11 @@ Flags on the bare `verifyx` command: | `circular-deps` | external | Circular dependencies ([skott](https://github.com/antoine-coulon/skott)). | | `duplicate-code` | external | Copy-paste detection ([jscpd](https://github.com/kucherenko/jscpd)). | -External checks shell out to their tool and **skip gracefully when it is not installed**; `verifyx init` installs the ones you opt into. They run the tool from your local `node_modules/.bin` regardless of how `verifyx` was invoked. `oxlint`/`oxfmt`/`tsc` are resolved if present; the rest are declared as optional `peerDependencies`. +External checks spawn their tool and **skip gracefully when it is not installed**; `verifyx init` installs the ones you opt into. They run the tool from your local `node_modules/.bin` regardless of how `verifyx` was invoked. `oxlint`/`oxfmt`/`tsc` are resolved if present; the rest are declared as optional `peerDependencies`. -Because checks are named for their function, **on failure** an external check prints the tool it used, the exact command it ran, and a docs link, so you (or an agent) can add the tool's config (e.g. `knip.json`) without guessing. On success it prints nothing (output is buffered and flushed only on failure, to keep runs quiet and cheap). If you override a check with your own `verify:` script, a failure shows that `npm run verify:` was what ran. +Because checks are named for their function, **on failure** an external check prints the tool it used, the exact argv it ran, and a docs link, so you (or an agent) can add the tool's config (e.g. `knip.json`) without guessing. On success it prints nothing (output is buffered and flushed only on failure, to keep runs quiet and cheap). If you override a check with your own `verify:` script, a failure shows that `npm run verify:` was what ran. -**Passing extra arguments through.** When you run an external check directly, anything after `--` is forwarded verbatim to the underlying tool, so you can tweak an invocation without ejecting it: `verifyx circular-deps -- src/*.ts` runs skott against `src/*.ts`, `verifyx lint -- --quiet` passes `--quiet` to oxlint. `verifyx init` scaffolds `verify:circular-deps` as `verifyx circular-deps -- src/*.ts` (skott needs a target), so the default is visible in your `package.json` and easy to point at your own source layout. +**Passing extra arguments through.** When you run an external check directly, anything after `--` is forwarded as literal argv entries to the underlying tool, so you can tweak an invocation without ejecting it: `verifyx circular-deps -- src/index.ts` asks skott to start at that file, while `verifyx lint -- --quiet` passes `--quiet` to oxlint. With no entrypoint, skott scans the current project, so `verifyx init` scaffolds the cross-platform default `verifyx circular-deps` without a shell glob. Each external check is configured through its **tool's own config file**, exactly as you would use that tool standalone: @@ -326,7 +326,7 @@ import { analyzeComplexity, getCheck, runAll } from '@makerx/verify' // Run the maintainability analysis directly. const { failing } = analyzeComplexity({ pattern: 'src/**/*.ts', threshold: 50 }) -// Run any single check by name, including the ones that shell out to an external tool. +// Run any single check by name, including the ones that spawn an external tool. const lint = await getCheck('lint')?.runDefault() const unused = await getCheck('unused-code')?.runDefault({ maxWarnings: 5 }) diff --git a/src/checks/external.test.ts b/src/checks/external.test.ts index e0f8f9a..be35726 100644 --- a/src/checks/external.test.ts +++ b/src/checks/external.test.ts @@ -1,125 +1,88 @@ import { describe, expect, it, vi } from 'vitest' import { runCaptured } from '../shared/output.ts' -import { buildArgv, defineExternalCheck, externalFailureHint, formatCommand, runCountedBudget, selectCommand } from './external.ts' +import { defineExternalCheck, externalFailureHint, runCountedBudget, selectCommand } from './external.ts' -const fixable = { checkCommand: 'oxfmt --check .', fixCommand: 'oxfmt .' } -const notFixable = { checkCommand: 'tsc --noEmit' } +const fixable = { checkCommand: ['oxfmt', '--check', '.'], fixCommand: ['oxfmt', '.'] } +const notFixable = { checkCommand: ['tsc', '--noEmit'] } describe('selectCommand', () => { it('uses the fix command in fix mode when the check is fixable', () => { - expect(selectCommand(fixable, 'fix')).toBe('oxfmt .') + expect(selectCommand(fixable, 'fix')).toEqual(['oxfmt', '.']) }) it('uses the check command in check mode', () => { - expect(selectCommand(fixable, 'check')).toBe('oxfmt --check .') + expect(selectCommand(fixable, 'check')).toEqual(['oxfmt', '--check', '.']) }) it('always uses the check command when the check has no fix command', () => { - expect(selectCommand(notFixable, 'fix')).toBe('tsc --noEmit') - expect(selectCommand(notFixable, 'check')).toBe('tsc --noEmit') + expect(selectCommand(notFixable, 'fix')).toEqual(['tsc', '--noEmit']) + expect(selectCommand(notFixable, 'check')).toEqual(['tsc', '--noEmit']) }) }) describe('externalFailureHint', () => { - it('names the tool, the exact command that ran, and the docs link', () => { - const hint = externalFailureHint({ name: 'unused-code', bin: 'knip', docs: 'https://knip.dev' }, 'knip --no-progress') + it('names the tool, the exact argv that ran, and the docs link', () => { + const hint = externalFailureHint({ name: 'unused-code', bin: 'knip', docs: 'https://knip.dev' }, ['knip', '--no-progress']) expect(hint).toContain('unused-code') expect(hint).toContain('knip') - expect(hint).toContain('knip --no-progress') + expect(hint).toContain('["knip","--no-progress"]') expect(hint).toContain('https://knip.dev') }) it('still names the tool and command when no docs link is set', () => { - const hint = externalFailureHint({ name: 'circular-deps', bin: 'skott' }, 'skott src') + const hint = externalFailureHint({ name: 'circular-deps', bin: 'skott' }, ['skott', 'src']) expect(hint).toContain('skott') - expect(hint).toContain('skott src') + expect(hint).toContain('["skott","src"]') expect(hint).not.toContain('undefined') }) }) -describe('buildArgv', () => { - it('tokenises the base command, keeping a quoted default as a single literal entry', () => { - expect(buildArgv('jscpd --format typescript,tsx --ignore "**/*.test.*" -r consoleFull src')).toEqual([ - 'jscpd', - '--format', - 'typescript,tsx', - '--ignore', - '**/*.test.*', - '-r', - 'consoleFull', - 'src', - ]) - }) - - it('appends each passthrough arg as its own literal entry (globs and spaces are never split or expanded)', () => { - expect(buildArgv('jscpd --ignore "**/*.test.*" src', ['--ignore', '**/generated/**', 'has space'])).toEqual([ - 'jscpd', - '--ignore', - '**/*.test.*', - 'src', - '--ignore', - '**/generated/**', - 'has space', - ]) - }) - - it('preserves shell metacharacters in passthrough args verbatim', () => { - expect(buildArgv('tool', ['$HOME', ';whoami', 'a|b', '`id`'])).toEqual(['tool', '$HOME', ';whoami', 'a|b', '`id`']) - }) - - it('returns just the tokenised command when there are no extra args', () => { - expect(buildArgv('oxlint .', [])).toEqual(['oxlint', '.']) - expect(buildArgv('oxlint .')).toEqual(['oxlint', '.']) - }) -}) - -describe('formatCommand', () => { - it('renders a copy-pasteable, quoted command line for diagnostics without changing what runs', () => { - expect(formatCommand(['jscpd', '--ignore', '**/generated/**', 'has space'])).toBe('jscpd --ignore "**/generated/**" "has space"') - }) -}) - describe('defineExternalCheck', () => { it('exposes raw commands for eject, including the fix variant when fixable', () => { const lint = defineExternalCheck({ name: 'lint', description: '', bin: 'oxlint', - checkCommand: 'oxlint .', - fixCommand: 'oxlint --fix .', + checkCommand: ['oxlint', '.'], + fixCommand: ['oxlint', '--fix', '.'], devDeps: [], }) expect(lint.eject).toEqual({ check: 'oxlint .', fix: 'oxlint --fix .' }) }) - it('scaffolds default trailing args after `--` so a consumer can see and tweak them', () => { + it('scaffolds a bare CLI call and serializes argv only at the eject boundary', () => { const circular = defineExternalCheck({ name: 'circular-deps', description: '', bin: 'skott', - checkCommand: 'skott', + checkCommand: ['skott'], devDeps: [], - scaffoldArgs: 'src/*.ts', }) - expect(circular.scaffold.script).toBe('verifyx circular-deps -- src/*.ts') + expect(circular.scaffold.script).toBe('verifyx circular-deps') expect(circular.eject).toEqual({ check: 'skott', fix: undefined }) }) it('scaffolds a bare CLI call when there are no default args', () => { - const knip = defineExternalCheck({ name: 'unused-code', description: '', bin: 'knip', checkCommand: 'knip', devDeps: [] }) + const knip = defineExternalCheck({ name: 'unused-code', description: '', bin: 'knip', checkCommand: ['knip'], devDeps: [] }) expect(knip.scaffold.script).toBe('verifyx unused-code') }) }) describe('runCountedBudget', () => { - const spec = { name: 'duplicate-code', bin: 'jscpd', checkCommand: 'jscpd src', docs: 'https://x' } + const spec = { name: 'duplicate-code', bin: 'jscpd', checkCommand: ['jscpd', 'src'], docs: 'https://x' } const budget = (count: number, report = '') => ({ strategy: 'count' as const, unit: 'clone', count: async () => ({ count, report }) }) it('passes when the finding count is at or below the budget', async () => { expect(await runCountedBudget(spec, budget(5), 5, [], {})).toEqual({ name: 'duplicate-code', ok: true }) }) + it('passes one prebuilt argv, including passthrough args, to the counter', async () => { + const count = vi.fn(async () => ({ count: 0, report: '' })) + await runCountedBudget(spec, { strategy: 'count', unit: 'clone', count }, 5, ['--ignore', 'has space'], {}) + expect(count).toHaveBeenCalledWith({ argv: ['jscpd', 'src', '--ignore', 'has space'], env: {} }) + }) + it('fails over budget, printing the counting run’s report instead of re-running the tool', async () => { const transformingSpec = { ...spec, transformOutput: (output: string) => output.toUpperCase() } const spy = vi.spyOn(console, 'error').mockImplementation(() => {}) diff --git a/src/checks/external.ts b/src/checks/external.ts index 0cb538f..36d966b 100644 --- a/src/checks/external.ts +++ b/src/checks/external.ts @@ -4,12 +4,10 @@ import path from 'node:path' import { color } from '../shared/color.ts' import { resolveMode } from '../shared/mode.ts' import { emit } from '../shared/output.ts' -import { buildArgv, formatCommand, runCommand } from '../shared/spawn.ts' +import { appendArgv, formatArgv, formatShellCommand, runArgvCommand } from '../shared/spawn.ts' import { type MaxWarningsSupport, withinBudget } from './maxWarnings.ts' import type { Check, CheckMode, CheckResult, RunDefaultOptions } from './types.ts' -export { buildArgv, formatCommand } from '../shared/spawn.ts' - const BIN_EXTENSIONS = ['', '.cmd', '.ps1', '.exe'] /** True when a project-local binary is installed under node_modules/.bin (cross-platform). */ @@ -34,9 +32,9 @@ export type ExternalCheckSpec = { /** The node_modules/.bin executable that must be present for the check to run. */ bin: string /** Command run in check mode (report + fail, never rewrite). */ - checkCommand: string + checkCommand: readonly string[] /** Command run in fix mode. When omitted, the check is not fixable and always runs `checkCommand`. */ - fixCommand?: string + fixCommand?: readonly string[] devDeps: string[] recommended?: boolean /** Docs / config reference for the underlying tool, surfaced when the check fails. */ @@ -45,26 +43,21 @@ export type ExternalCheckSpec = { canRun?: () => boolean /** Rewrite the tool's captured output before it is printed, e.g. to strip a tool's own hardcoded colouring. */ transformOutput?: (output: string) => string - /** - * Default trailing args scaffolded after `--` (e.g. skott's `src/*.ts` target), surfaced in the `verify:` - * script so a consumer can see and tweak them. Not baked into `runDefault`; only the scaffolded script carries them. - */ - scaffoldArgs?: string maxWarnings?: MaxWarningsSupport } /** Pick the command for the run mode: the fix command only in fix mode and only when the check is fixable. */ -export function selectCommand(spec: Pick, mode: CheckMode): string { +export function selectCommand(spec: Pick, mode: CheckMode): readonly string[] { return mode === 'fix' && spec.fixCommand ? spec.fixCommand : spec.checkCommand } /** * The line printed when an external check fails: names the tool (checks are named for their function, so the - * tool is otherwise hidden), the exact command that ran, and where to configure it — so an agent can set up + * tool is otherwise hidden), the exact argv that ran, and where to configure it — so an agent can set up * the tool's config file (e.g. knip.json) without guessing. */ -export function externalFailureHint(spec: Pick, command: string): string { - return `↳ ${spec.name} uses ${spec.bin}: ran \`${command}\`. Configure ${spec.bin}${spec.docs ? ` — ${spec.docs}` : ''}.` +export function externalFailureHint(spec: Pick, argv: readonly string[]): string { + return `↳ ${spec.name} uses ${spec.bin}: ran argv \`${formatArgv(argv)}\`. Configure ${spec.bin}${spec.docs ? ` — ${spec.docs}` : ''}.` } type CountBudget = Extract @@ -77,9 +70,10 @@ export async function runCountedBudget( extraArgs: string[], env: Record, ): Promise { + const argv = appendArgv(spec.checkCommand, extraArgs) let counted: { count: number; report: string } try { - counted = await budget.count({ extraArgs, env, checkCommand: spec.checkCommand }) + counted = await budget.count({ argv, env }) } catch (error) { const docs = spec.docs ? ` — ${spec.docs}` : '' console.error( @@ -95,11 +89,11 @@ export async function runCountedBudget( const unit = count === 1 ? budget.unit : `${budget.unit}s` console.error(color.dim(`↳ ${spec.name}: ${count} ${unit} found, exceeds --max-warnings ${maxWarnings}.`)) if (report) emit(spec.transformOutput ? spec.transformOutput(report) : report) - console.error(color.dim(externalFailureHint(spec, formatCommand(buildArgv(spec.checkCommand, extraArgs))))) + console.error(color.dim(externalFailureHint(spec, argv))) return { name: spec.name, ok: false } } -/** Build a Check that shells out to an external tool, skipping gracefully when the tool cannot run. */ +/** Build a Check that spawns an external tool, skipping gracefully when the tool cannot run. */ export function defineExternalCheck(spec: ExternalCheckSpec): Check { return { name: spec.name, @@ -109,11 +103,14 @@ export function defineExternalCheck(spec: ExternalCheckSpec): Check { supportsMaxWarnings: !!spec.maxWarnings, // Scaffold as a call into this CLI so fix-vs-check lives in one place, not the consumer's script. scaffold: { - script: spec.scaffoldArgs ? `verifyx ${spec.name} -- ${spec.scaffoldArgs}` : `verifyx ${spec.name}`, + script: `verifyx ${spec.name}`, devDeps: spec.devDeps, }, // `verifyx eject ` inlines these raw commands into the consumer's verify:* scripts. - eject: { check: spec.checkCommand, fix: spec.fixCommand }, + eject: { + check: formatShellCommand(spec.checkCommand), + fix: spec.fixCommand ? formatShellCommand(spec.fixCommand) : undefined, + }, async runDefault({ extraArgs = [], maxWarnings }: RunDefaultOptions = {}): Promise { if (!hasLocalBin(spec.bin)) { console.log(color.dim(`${spec.name}: ${spec.bin} not installed — skipping (add it with \`npx verifyx init\`)`)) @@ -125,16 +122,16 @@ export function defineExternalCheck(spec: ExternalCheckSpec): Check { } // quiet: buffer the tool's output and flush only on failure (streamed live under --verbose). const runReport = async (argv: string[]): Promise => { - const code = await runCommand(argv, { env: envWithLocalBin(), quiet: true, transform: spec.transformOutput }) - if (code !== 0) console.error(color.dim(externalFailureHint(spec, formatCommand(argv)))) + const code = await runArgvCommand(argv, { env: envWithLocalBin(), quiet: true, transform: spec.transformOutput }) + if (code !== 0) console.error(color.dim(externalFailureHint(spec, argv))) return { name: spec.name, ok: code === 0 } } if (maxWarnings !== undefined && spec.maxWarnings) { const budget = spec.maxWarnings if (budget.strategy === 'count') return runCountedBudget(spec, budget, maxWarnings, extraArgs, envWithLocalBin()) - return runReport(buildArgv(selectCommand(spec, resolveMode()), [...extraArgs, ...budget.toArgs(maxWarnings)])) + return runReport(appendArgv(selectCommand(spec, resolveMode()), [...extraArgs, ...budget.toArgs(maxWarnings)])) } - return runReport(buildArgv(selectCommand(spec, resolveMode()), extraArgs)) + return runReport(appendArgv(selectCommand(spec, resolveMode()), extraArgs)) }, } } diff --git a/src/checks/maxWarnings.ts b/src/checks/maxWarnings.ts index 0c9c180..fda53fa 100644 --- a/src/checks/maxWarnings.ts +++ b/src/checks/maxWarnings.ts @@ -2,9 +2,9 @@ import fs from 'node:fs' import os from 'node:os' import path from 'node:path' -import { buildArgv, captureCommand } from '../shared/spawn.ts' +import { captureArgvCommand } from '../shared/spawn.ts' -type MaxWarningsCountContext = { extraArgs: string[]; env: Record; checkCommand: string } +type MaxWarningsCountContext = { argv: readonly string[]; env: Record } /** The finding count plus the tool's rendered console report, so a failing budget can print it without a second run. */ export type CountResult = { count: number; report: string } @@ -29,8 +29,8 @@ export function countJscpdClones(report: { statistics?: { total?: { clones?: unk export async function jscpdCount(ctx: MaxWarningsCountContext): Promise { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'verifyx-jscpd-')) try { - const argv = [...buildArgv(ctx.checkCommand, ctx.extraArgs), '--reporters', 'json', '--output', dir] - const { code, stdout, stderr } = await captureCommand(argv, { env: ctx.env }) + const argv = [...ctx.argv, '--reporters', 'json', '--output', dir] + const { code, stdout, stderr } = await captureArgvCommand(argv, { env: ctx.env }) const reportPath = path.join(dir, 'jscpd-report.json') if (!fs.existsSync(reportPath)) throw new Error(`jscpd produced no report (exit ${code})${stderr.trim() ? `: ${stderr.trim()}` : ''}`) const count = countJscpdClones(JSON.parse(fs.readFileSync(reportPath, 'utf8'))) diff --git a/src/checks/registry.test.ts b/src/checks/registry.test.ts index 4dd066c..3383509 100644 --- a/src/checks/registry.test.ts +++ b/src/checks/registry.test.ts @@ -39,8 +39,8 @@ describe('check registry', () => { expect(getCheck('complexity')?.scaffold.script).toBe('verifyx complexity') }) - it('scaffolds circular-deps with a default skott target after `--`', () => { - expect(getCheck('circular-deps')?.scaffold.script).toBe('verifyx circular-deps -- src/*.ts') + it('scaffolds circular-deps without a shell-dependent glob', () => { + expect(getCheck('circular-deps')?.scaffold.script).toBe('verifyx circular-deps') }) it('exposes raw tool commands for eject on external checks, but not native ones', () => { diff --git a/src/checks/registry.ts b/src/checks/registry.ts index a89826f..9daf54a 100644 --- a/src/checks/registry.ts +++ b/src/checks/registry.ts @@ -37,8 +37,8 @@ export const CHECKS: Check[] = [ name: 'lint', description: 'Lint — auto-fixes locally, checks in CI', bin: 'oxlint', - checkCommand: 'oxlint .', - fixCommand: 'oxlint --fix .', + checkCommand: ['oxlint', '.'], + fixCommand: ['oxlint', '--fix', '.'], devDeps: ['oxlint'], recommended: true, docs: 'https://oxc.rs/docs/guide/usage/linter.html', @@ -47,8 +47,8 @@ export const CHECKS: Check[] = [ name: 'format', description: 'Formatting — writes locally, checks in CI', bin: 'oxfmt', - checkCommand: 'oxfmt --check .', - fixCommand: 'oxfmt .', + checkCommand: ['oxfmt', '--check', '.'], + fixCommand: ['oxfmt', '.'], devDeps: ['oxfmt'], recommended: true, docs: 'https://oxc.rs', @@ -57,7 +57,7 @@ export const CHECKS: Check[] = [ name: 'check-types', description: 'TypeScript type check', bin: 'tsc', - checkCommand: 'tsc --noEmit', + checkCommand: ['tsc', '--noEmit'], devDeps: ['typescript'], canRun: () => fs.existsSync('tsconfig.json'), recommended: true, @@ -67,7 +67,7 @@ export const CHECKS: Check[] = [ name: 'unused-code', description: 'Unused files, exports and dependencies', bin: 'knip', - checkCommand: 'knip --no-progress --treat-config-hints-as-errors', + checkCommand: ['knip', '--no-progress', '--treat-config-hints-as-errors'], devDeps: ['knip'], docs: 'https://knip.dev/reference/configuration', maxWarnings: { strategy: 'flag', toArgs: (n) => ['--max-issues', String(n)] }, @@ -76,17 +76,15 @@ export const CHECKS: Check[] = [ name: 'circular-deps', description: 'Circular dependency detection', bin: 'skott', - checkCommand: 'skott --displayMode=raw --showCircularDependencies --exitCodeOnCircularDependencies=1', + checkCommand: ['skott', '--displayMode=raw', '--showCircularDependencies', '--exitCodeOnCircularDependencies=1'], devDeps: ['skott'], docs: 'https://github.com/antoine-coulon/skott', - // skott needs a target; scaffold it after `--` so consumers can see and adjust it (e.g. to their source layout). - scaffoldArgs: 'src/*.ts', }), defineExternalCheck({ name: 'duplicate-code', description: 'Copy-paste / duplicate-code detection', bin: 'jscpd', - checkCommand: 'jscpd --format typescript,tsx --exit-code 1 --ignore "**/*.test.*" -r consoleFull src', + checkCommand: ['jscpd', '--format', 'typescript,tsx', '--exit-code', '1', '--ignore', '**/*.test.*', '-r', 'consoleFull', 'src'], devDeps: ['jscpd'], // jscpd hardcodes red header cells in its stats table (even on a clean run) — reads like a failure. It ignores // NO_COLOR/FORCE_COLOR, so strip the red foreground from its output; the table renders in the default colour. diff --git a/src/checks/types.ts b/src/checks/types.ts index 0a248fd..4167564 100644 --- a/src/checks/types.ts +++ b/src/checks/types.ts @@ -13,12 +13,12 @@ export type CheckResult = { /** Per-run options for a check. `extraArgs` are the tokens a user passes after `--` (external checks only). */ export type RunDefaultOptions = { - /** Extra arguments appended verbatim to an external check's underlying command (e.g. `verifyx circular-deps -- src/*.ts`). */ + /** Extra arguments appended verbatim to an external check's underlying argv (e.g. `verifyx circular-deps -- src/index.ts`). */ extraArgs?: string[] maxWarnings?: number } -/** A single verification. Native checks run in-process; external checks shell out to a tool. */ +/** A single verification. Native checks run in-process; external checks spawn a tool. */ export type Check = { name: string description: string diff --git a/src/orchestrator/run.ts b/src/orchestrator/run.ts index 117ddcf..a7695de 100644 --- a/src/orchestrator/run.ts +++ b/src/orchestrator/run.ts @@ -1,5 +1,5 @@ import { configureMode, resolveMode } from '../shared/mode.ts' -import { runCommand, setVerbose } from '../shared/spawn.ts' +import { runShellCommand, setVerbose } from '../shared/spawn.ts' import { type MeasureRecord, printMeasureTable } from './measure.ts' import { chatty, reportOutcomes } from './report.ts' import { entryCheckName, resolveEntries, selectEntries, type VerifyEntry } from './resolveEntries.ts' @@ -16,7 +16,7 @@ export type OrchestrateOptions = { async function runEntry(entry: VerifyEntry): Promise { const startTime = Date.now() - const code = await runCommand(entry.command, { cwd: entry.cwd, quiet: true }) + const code = await runShellCommand(entry.command, { cwd: entry.cwd, quiet: true }) return { script: entry.name, code, durationMs: Date.now() - startTime } } diff --git a/src/orchestrator/runAll.ts b/src/orchestrator/runAll.ts index 07ff7dd..8cecd68 100644 --- a/src/orchestrator/runAll.ts +++ b/src/orchestrator/runAll.ts @@ -2,7 +2,7 @@ import { CHECKS } from '../checks/registry.ts' import { color, paintRed } from '../shared/color.ts' import { resolveMode } from '../shared/mode.ts' import { installConsoleCapture, runCaptured } from '../shared/output.ts' -import { runCommand } from '../shared/spawn.ts' +import { runShellCommand } from '../shared/spawn.ts' import { type MeasureRecord, printMeasureTable } from './measure.ts' import { reportOutcomes } from './report.ts' import { entryCheckName, resolveEntries, resolveOverride, selectEntries } from './resolveEntries.ts' @@ -19,7 +19,7 @@ function buildTasks(opts: RunAllOptions): Task[] { const spawn = (name: string, command: string, cwd: string, note?: string): Task => ({ name, note, - run: async () => (await runCommand(command, { cwd, quiet: true })) === 0, + run: async () => (await runShellCommand(command, { cwd, quiet: true })) === 0, }) const tasks: Task[] = CHECKS.map((check) => { @@ -29,7 +29,7 @@ function buildTasks(opts: RunAllOptions): Task[] { name: check.name, note: 'overridden', run: async () => { - const ok = (await runCommand(override.command, { cwd: override.cwd, quiet: true })) === 0 + const ok = (await runShellCommand(override.command, { cwd: override.cwd, quiet: true })) === 0 if (!ok) console.error(color.dim(`↳ ${check.name}: ran \`${override.command}\` (override)`)) return ok }, diff --git a/src/scaffold/installDeps.ts b/src/scaffold/installDeps.ts index 976e228..9a53a1a 100644 --- a/src/scaffold/installDeps.ts +++ b/src/scaffold/installDeps.ts @@ -1,9 +1,9 @@ import fs from 'node:fs' import path from 'node:path' -import { runCommand } from '../shared/spawn.ts' +import { runShellCommand } from '../shared/spawn.ts' -/** Signature of the command runner (defaults to `runCommand`); injectable so tests don't shell out. */ +/** Signature of the command runner (defaults to `runShellCommand`); injectable so tests don't shell out. */ export type Runner = (command: string, opts: { cwd: string }) => Promise export type InstallReport = { @@ -41,7 +41,7 @@ function isInstalled(name: string, cwd: string, declared: Set): boolean * fails — typically a single peer-dependency conflict — falls back to per-package installs so one bad package * can't block the rest, and the exact failures are reported instead of aborting `init`. */ -export async function installDevDeps(devDeps: readonly string[], cwd: string, run: Runner = runCommand): Promise { +export async function installDevDeps(devDeps: readonly string[], cwd: string, run: Runner = runShellCommand): Promise { const declared = declaredDeps(cwd) const skipped: string[] = [] const toInstall: string[] = [] diff --git a/src/shared/spawn.test.ts b/src/shared/spawn.test.ts index a73cc5d..67b0853 100644 --- a/src/shared/spawn.test.ts +++ b/src/shared/spawn.test.ts @@ -4,28 +4,33 @@ import path from 'node:path' import { describe, expect, it } from 'vitest' -import { buildArgv, captureCommand, formatCommand, tokenizeCommand } from './spawn.ts' +import { appendArgv, captureArgvCommand, formatArgv, formatShellCommand } from './spawn.ts' -describe('captureCommand', () => { +describe('captureArgvCommand', () => { it('returns the captured stdout and a zero exit code for a successful command', async () => { - const { code, stdout } = await captureCommand(`node -e "process.stdout.write('captured-output')"`) + const command = + process.platform === 'win32' ? [process.execPath, '-e', "process.stdout.write('captured-output')"] : ['printf', 'captured-output'] + const { code, stdout } = await captureArgvCommand(command) expect(code).toBe(0) expect(stdout).toBe('captured-output') }) it('surfaces a non-zero exit code without throwing', async () => { - const { code } = await captureCommand(`node -e "process.exit(3)"`) + const command = process.platform === 'win32' ? [process.execPath, '-e', 'process.exit(3)'] : ['sh', '-c', 'exit 3'] + const { code } = await captureArgvCommand(command) expect(code).toBe(3) }) it('captures stderr separately from stdout', async () => { - const { stdout, stderr } = await captureCommand(`node -e "process.stderr.write('to-stderr')"`) + const command = + process.platform === 'win32' ? [process.execPath, '-e', "process.stderr.write('to-stderr')"] : ['sh', '-c', 'printf to-stderr >&2'] + const { stdout, stderr } = await captureArgvCommand(command) expect(stdout).toBe('') expect(stderr).toContain('to-stderr') }) it('returns the spawn error when an argv executable cannot be launched', async () => { - const { code, stderr } = await captureCommand(['verifyx-command-that-does-not-exist']) + const { code, stderr } = await captureArgvCommand(['verifyx-command-that-does-not-exist']) expect(code).toBe(127) expect(stderr).toContain('ENOENT') }) @@ -33,11 +38,14 @@ describe('captureCommand', () => { // Regression: forwarded passthrough args used to be joined into a shell string, so the spawn shell // glob-expanded/word-split them before the tool saw them. An argv array must reach the tool verbatim. it('passes an argv array to the process with no shell, so globs and metacharacters arrive literally', async () => { - const echoArgv = 'process.stdout.write(JSON.stringify(process.argv.slice(1)))' const passthrough = ['--ignore', '**/generated/**', 'has space', '$HOME', ';whoami'] - const { code, stdout } = await captureCommand(['node', '-e', echoArgv, '--', ...passthrough]) + const command = + process.platform === 'win32' + ? [process.execPath, '-e', 'process.stdout.write(JSON.stringify(process.argv.slice(1)))', '--', ...passthrough] + : ['printf', '%s\\n', ...passthrough] + const { code, stdout } = await captureArgvCommand(command) expect(code).toBe(0) - expect(JSON.parse(stdout)).toEqual(passthrough) + expect(process.platform === 'win32' ? JSON.parse(stdout) : stdout.split('\n').slice(0, -1)).toEqual(passthrough) }) it.runIf(process.platform === 'win32')('launches npm-style .cmd shims without interpreting their arguments', async () => { @@ -52,7 +60,7 @@ describe('captureCommand', () => { const passthrough = ['**/*.ts', 'has space', '$HOME', '%PATH%', ';whoami', 'a&b'] try { - const { code, stdout, stderr } = await captureCommand(['verifyx-echo-argv', ...passthrough], { env }) + const { code, stdout, stderr } = await captureArgvCommand(['verifyx-echo-argv', ...passthrough], { env }) expect({ code, stderr }).toEqual({ code: 0, stderr: '' }) expect(JSON.parse(stdout)).toEqual(passthrough) } finally { @@ -61,22 +69,9 @@ describe('captureCommand', () => { }) }) -describe('tokenizeCommand', () => { - it('splits on whitespace and strips quotes, keeping a quoted arg as one entry', () => { - expect(tokenizeCommand('jscpd --ignore "**/*.test.*" -r consoleFull src')).toEqual([ - 'jscpd', - '--ignore', - '**/*.test.*', - '-r', - 'consoleFull', - 'src', - ]) - }) -}) - -describe('buildArgv', () => { +describe('appendArgv', () => { it('appends passthrough args as their own literal entries', () => { - expect(buildArgv('jscpd src', ['--ignore', '**/generated/**', 'has space'])).toEqual([ + expect(appendArgv(['jscpd', 'src'], ['--ignore', '**/generated/**', 'has space'])).toEqual([ 'jscpd', 'src', '--ignore', @@ -86,8 +81,18 @@ describe('buildArgv', () => { }) }) -describe('formatCommand', () => { - it('quotes only the entries that need it, for a copy-pasteable diagnostic line', () => { - expect(formatCommand(['jscpd', '--ignore', '**/generated/**', 'plain'])).toBe('jscpd --ignore "**/generated/**" plain') +describe('formatArgv', () => { + it('renders exact argv without making platform-specific shell quoting claims', () => { + expect(formatArgv(['tool', 'C:\\src path', '$HOME', '%PATH%'])).toBe('["tool","C:\\\\src path","$HOME","%PATH%"]') + }) +}) + +describe('formatShellCommand', () => { + it('serializes built-in argv for package.json scripts on POSIX and Windows', () => { + expect(formatShellCommand(['jscpd', '--ignore', '**/*.test.*', 'src'])).toBe('jscpd --ignore "**/*.test.*" src') + }) + + it('rejects arguments that cannot be serialized consistently for both shells', () => { + expect(() => formatShellCommand(['tool', '$HOME'])).toThrow('cannot safely serialize') }) }) diff --git a/src/shared/spawn.ts b/src/shared/spawn.ts index 3836389..8f62d58 100644 --- a/src/shared/spawn.ts +++ b/src/shared/spawn.ts @@ -2,53 +2,31 @@ import spawn from 'cross-spawn' import { emit, isCapturing } from './output.ts' -// Honours single/double quotes so a quoted default like --ignore "**/*.test.*" stays one argv entry. Not a full shell parser. -export function tokenizeCommand(command: string): string[] { - const argv: string[] = [] - let current = '' - let started = false - let quote: '"' | "'" | null = null - for (const ch of command) { - if (quote) { - if (ch === quote) quote = null - else current += ch - continue - } - if (ch === '"' || ch === "'") { - quote = ch - started = true - continue - } - if (ch === ' ' || ch === '\t' || ch === '\n' || ch === '\r') { - if (started) { - argv.push(current) - current = '' - started = false - } - continue - } - current += ch - started = true - } - if (started) argv.push(current) - return argv +/** Append user-supplied arguments without parsing, splitting, or shell expansion. */ +export function appendArgv(command: readonly string[], extraArgs: readonly string[] = []): string[] { + return [...command, ...extraArgs] } -// Passthrough args stay as their own literal entries (never re-parsed) so globs/spaces/metacharacters reach the tool intact. -export function buildArgv(command: string, extraArgs: readonly string[] = []): string[] { - return [...tokenizeCommand(command), ...extraArgs] +/** Render the exact argv used by a process in a platform-neutral diagnostic form. */ +export function formatArgv(argv: readonly string[]): string { + return JSON.stringify(argv) } -const SAFE_ARG = /^[A-Za-z0-9_@%+=:,./-]+$/ +const SAFE_SHELL_ARG = /^[A-Za-z0-9_@+=:,./-]+$/ +const PORTABLE_QUOTED_ARG = /^[^"\\$`%!\r\n]*$/ -function quoteForDisplay(arg: string): string { - if (arg.length > 0 && SAFE_ARG.test(arg)) return arg - return `"${arg.replace(/(["\\$`])/g, '\\$1')}"` +function formatShellArg(arg: string): string { + if (arg.length > 0 && SAFE_SHELL_ARG.test(arg)) return arg + if (!PORTABLE_QUOTED_ARG.test(arg)) { + throw new Error(`cannot safely serialize argument for both POSIX and Windows npm scripts: ${JSON.stringify(arg)}`) + } + return `"${arg}"` } -/** Render an argv array as a copy-pasteable, safely-quoted command line for diagnostics only (never executed). */ -export function formatCommand(argv: readonly string[]): string { - return argv.map(quoteForDisplay).join(' ') +/** Serialize a trusted built-in argv for a cross-platform package.json script. */ +export function formatShellCommand(argv: readonly string[]): string { + if (argv.length === 0) throw new Error('argv command must have at least one entry (the executable)') + return argv.map(formatShellArg).join(' ') } let verboseMode = false @@ -67,40 +45,39 @@ function shouldSuppress(quiet?: boolean): boolean { return !!quiet || !!process.env.CLAUDECODE } -// A string runs through a shell (consumer verify:*/npm run scripts); an argv array runs with NO shell so entries reach the tool verbatim. -export type Command = string | readonly string[] - type SpawnInvocation = { file: string; args: string[]; shell: boolean } -function toInvocation(command: Command): SpawnInvocation { - if (typeof command === 'string') return { file: command, args: [], shell: true } - const [file, ...args] = command +function shellInvocation(command: string): SpawnInvocation { + return { file: command, args: [], shell: true } +} + +function argvInvocation(argv: readonly string[]): SpawnInvocation { + const [file, ...args] = argv if (!file) throw new Error('argv command must have at least one entry (the executable)') return { file, args, shell: false } } -export type RunCommandOptions = { - cwd?: string - env?: Record +type SpawnOptions = { cwd?: string; env?: Record } + +function spawnChild(invocation: SpawnInvocation, stdio: 'inherit' | 'pipe' | ['ignore', 'pipe', 'pipe'], opts: SpawnOptions) { + return spawn(invocation.file, invocation.args, { + stdio, + shell: invocation.shell, + cwd: opts.cwd ?? process.cwd(), + env: opts.env ? { ...process.env, ...opts.env } : undefined, + }) +} + +export type RunCommandOptions = SpawnOptions & { quiet?: boolean /** Rewrite the command's buffered output before it is emitted (only applies when output is suppressed/buffered). */ transform?: (output: string) => string } -/** - * Run a command, returning its exit code. Suppressed output (quiet, or under Claude Code) is buffered and - * flushed to stdout only if the command fails, keeping passing runs quiet. - */ -export function runCommand(command: Command, opts: RunCommandOptions = {}): Promise { +function runInvocation(invocation: SpawnInvocation, opts: RunCommandOptions): Promise { return new Promise((resolve) => { const suppress = shouldSuppress(opts.quiet) - const { file, args, shell } = toInvocation(command) - const child = spawn(file, args, { - stdio: suppress ? 'pipe' : 'inherit', - shell, - cwd: opts.cwd ?? process.cwd(), - env: opts.env ? { ...process.env, ...opts.env } : undefined, - }) + const child = spawnChild(invocation, suppress ? 'pipe' : 'inherit', opts) const chunks: Buffer[] = [] if (suppress) { child.stdout?.on('data', (data: Buffer) => chunks.push(data)) @@ -122,18 +99,23 @@ export function runCommand(command: Command, opts: RunCommandOptions = {}): Prom }) } -export function captureCommand( - command: Command, - opts: { cwd?: string; env?: Record } = {}, +/** Run a consumer-owned command through the platform shell. */ +export function runShellCommand(command: string, opts: RunCommandOptions = {}): Promise { + return runInvocation(shellInvocation(command), opts) +} + +/** Run a structured argv directly, using no shell. */ +export function runArgvCommand(argv: readonly string[], opts: RunCommandOptions = {}): Promise { + return runInvocation(argvInvocation(argv), opts) +} + +/** Run a structured argv directly and capture stdout/stderr separately. */ +export function captureArgvCommand( + argv: readonly string[], + opts: SpawnOptions = {}, ): Promise<{ code: number; stdout: string; stderr: string }> { return new Promise((resolve) => { - const { file, args, shell } = toInvocation(command) - const child = spawn(file, args, { - stdio: ['ignore', 'pipe', 'pipe'], - shell, - cwd: opts.cwd ?? process.cwd(), - env: opts.env ? { ...process.env, ...opts.env } : undefined, - }) + const child = spawnChild(argvInvocation(argv), ['ignore', 'pipe', 'pipe'], opts) const out: Buffer[] = [] const err: Buffer[] = [] child.stdout?.on('data', (data: Buffer) => out.push(data)) From 363965c9b91c71e32ffd3a1dc7e4f4aba45c0762 Mon Sep 17 00:00:00 2001 From: Hoang Dinh Date: Mon, 20 Jul 2026 20:22:22 +1000 Subject: [PATCH 4/5] fix: lazy eject and PATHEXT bin lookup --- src/checks/external.test.ts | 5 +++++ src/checks/external.ts | 14 +++++++++----- src/shared/spawn.ts | 8 ++++++-- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/checks/external.test.ts b/src/checks/external.test.ts index be35726..98e6751 100644 --- a/src/checks/external.test.ts +++ b/src/checks/external.test.ts @@ -67,6 +67,11 @@ describe('defineExternalCheck', () => { const knip = defineExternalCheck({ name: 'unused-code', description: '', bin: 'knip', checkCommand: ['knip'], devDeps: [] }) expect(knip.scaffold.script).toBe('verifyx unused-code') }) + + it('serializes eject lazily, so an unserializable command fails eject rather than check construction', () => { + const check = defineExternalCheck({ name: 'x', description: '', bin: 'tool', checkCommand: ['tool', '$HOME'], devDeps: [] }) + expect(() => check.eject).toThrow('cannot safely serialize') + }) }) describe('runCountedBudget', () => { diff --git a/src/checks/external.ts b/src/checks/external.ts index 36d966b..9ab4c1b 100644 --- a/src/checks/external.ts +++ b/src/checks/external.ts @@ -8,7 +8,8 @@ import { appendArgv, formatArgv, formatShellCommand, runArgvCommand } from '../s import { type MaxWarningsSupport, withinBudget } from './maxWarnings.ts' import type { Check, CheckMode, CheckResult, RunDefaultOptions } from './types.ts' -const BIN_EXTENSIONS = ['', '.cmd', '.ps1', '.exe'] +// Mirror the launcher's resolution (cross-spawn → which + PATHEXT) so "installed" and "runnable" agree. +const BIN_EXTENSIONS = process.platform === 'win32' ? (process.env.PATHEXT ?? '.EXE;.CMD;.BAT;.COM').split(';').filter(Boolean) : [''] /** True when a project-local binary is installed under node_modules/.bin (cross-platform). */ function hasLocalBin(bin: string, cwd: string = process.cwd()): boolean { @@ -106,10 +107,13 @@ export function defineExternalCheck(spec: ExternalCheckSpec): Check { script: `verifyx ${spec.name}`, devDeps: spec.devDeps, }, - // `verifyx eject ` inlines these raw commands into the consumer's verify:* scripts. - eject: { - check: formatShellCommand(spec.checkCommand), - fix: spec.fixCommand ? formatShellCommand(spec.fixCommand) : undefined, + // `verifyx eject ` inlines these raw commands into the consumer's verify:* scripts. Lazy so + // formatShellCommand's unserializable-arg throw fires on eject only, not when CHECKS is built at import. + get eject() { + return { + check: formatShellCommand(spec.checkCommand), + fix: spec.fixCommand ? formatShellCommand(spec.fixCommand) : undefined, + } }, async runDefault({ extraArgs = [], maxWarnings }: RunDefaultOptions = {}): Promise { if (!hasLocalBin(spec.bin)) { diff --git a/src/shared/spawn.ts b/src/shared/spawn.ts index 8f62d58..e908615 100644 --- a/src/shared/spawn.ts +++ b/src/shared/spawn.ts @@ -23,9 +23,13 @@ function formatShellArg(arg: string): string { return `"${arg}"` } +function assertNonEmptyArgv(argv: readonly string[]): asserts argv is readonly [string, ...string[]] { + if (argv.length === 0) throw new Error('argv command must have at least one entry (the executable)') +} + /** Serialize a trusted built-in argv for a cross-platform package.json script. */ export function formatShellCommand(argv: readonly string[]): string { - if (argv.length === 0) throw new Error('argv command must have at least one entry (the executable)') + assertNonEmptyArgv(argv) return argv.map(formatShellArg).join(' ') } @@ -52,8 +56,8 @@ function shellInvocation(command: string): SpawnInvocation { } function argvInvocation(argv: readonly string[]): SpawnInvocation { + assertNonEmptyArgv(argv) const [file, ...args] = argv - if (!file) throw new Error('argv command must have at least one entry (the executable)') return { file, args, shell: false } } From 0b9e6b697ab5803d183ff8456b112c43e75cc616 Mon Sep 17 00:00:00 2001 From: Hoang Dinh Date: Tue, 21 Jul 2026 08:51:03 +1000 Subject: [PATCH 5/5] fix: address PR review nits - list ejectable checks by kind instead of evaluating every eject getter - refresh stale glob pass-through example in help-text comment --- src/commands/registerChecks.ts | 2 +- src/scaffold/eject.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/commands/registerChecks.ts b/src/commands/registerChecks.ts index 8ef879c..0217b05 100644 --- a/src/commands/registerChecks.ts +++ b/src/commands/registerChecks.ts @@ -82,7 +82,7 @@ export function registerChecks(program: Command): void { const command = program .command(check.name) .description(check.description) - // Everything after `--` is forwarded verbatim to the underlying tool (e.g. `verifyx circular-deps -- src/*.ts`). + // Everything after `--` is forwarded verbatim to the underlying tool (e.g. `verifyx unused-code -- --production`). .argument('[toolArgs...]', 'extra arguments passed through to the underlying tool (after `--`)') if (check.supportsMaxWarnings) { command.option('--max-warnings ', 'tolerate up to n findings before failing (counts findings)', parseMaxWarnings) diff --git a/src/scaffold/eject.ts b/src/scaffold/eject.ts index e0340e6..11178fa 100644 --- a/src/scaffold/eject.ts +++ b/src/scaffold/eject.ts @@ -12,7 +12,7 @@ export type EjectResult = { export function ejectScripts(name: string): Record { const check = getCheck(name) if (!check) { - const known = CHECKS.filter((c) => c.eject) + const known = CHECKS.filter((c) => c.kind === 'external') .map((c) => c.name) .join(', ') throw new Error(`Unknown check "${name}". Ejectable checks: ${known}.`)