From c62a5312fa5b457a2d5a7d5af0491ac3f0f88007 Mon Sep 17 00:00:00 2001 From: grzdev Date: Tue, 10 Nov 2026 07:31:16 +0100 Subject: [PATCH] fix: reject empty and whitespace ids with VALIDATION_ERROR in read-by-id commands (#368) Fixes #368 --- src/commands/project.ts | 3 - src/commands/test.ts | 7 -- src/index.ts | 11 ++- src/lib/args.test.ts | 163 ++++++++++++++++++++++++++++++++++++++++ src/lib/args.ts | 64 ++++++++++++++++ src/lib/errors.test.ts | 46 ++++++++++++ src/lib/errors.ts | 10 ++- 7 files changed, 289 insertions(+), 15 deletions(-) create mode 100644 src/lib/args.test.ts create mode 100644 src/lib/args.ts diff --git a/src/commands/project.ts b/src/commands/project.ts index 99542e74..6284306e 100644 --- a/src/commands/project.ts +++ b/src/commands/project.ts @@ -724,9 +724,6 @@ export async function runDelete( deps: ProjectDeps = {}, ): Promise { assertIdempotencyKey(opts.idempotencyKey); - if (opts.projectId === undefined || opts.projectId.trim().length === 0) { - throw localValidationError(' is required'); - } if (!opts.confirm && !opts.dryRun) { throw ApiError.fromEnvelope({ diff --git a/src/commands/test.ts b/src/commands/test.ts index ffab7a82..d705ecf7 100644 --- a/src/commands/test.ts +++ b/src/commands/test.ts @@ -2012,7 +2012,6 @@ export async function runPlanPut( deps: TestDeps = {}, ): Promise { assertIdempotencyKey(opts.idempotencyKey); - requireNonEmpty('test-id', opts.testId); requireNonEmpty('steps', opts.stepsFile); if ( @@ -2425,7 +2424,6 @@ export async function runUpdate( deps: TestDeps = {}, ): Promise { assertIdempotencyKey(opts.idempotencyKey); - requireNonEmpty('test-id', opts.testId); // P1-3: client-side length checks matching server limits. if (opts.name !== undefined && opts.name.trim().length === 0) { throw localValidationError( @@ -2568,7 +2566,6 @@ export async function runDelete( deps: TestDeps = {}, ): Promise { assertIdempotencyKey(opts.idempotencyKey); - requireNonEmpty('test-id', opts.testId); if (!opts.confirm && !opts.dryRun) { throw ApiError.fromEnvelope({ @@ -5801,7 +5798,6 @@ export async function runCodePut( deps: TestDeps = {}, ): Promise { assertIdempotencyKey(opts.idempotencyKey); - requireNonEmpty('test-id', opts.testId); requireNonEmpty('code-file', opts.codeFile); assertPythonCodeFile(opts.codeFile); @@ -16226,9 +16222,6 @@ export async function runFlaky( const stderrFn = deps.stderr ?? ((line: string) => process.stderr.write(`${line}\n`)); const out = makeOutput(opts.output, deps); - if (typeof opts.testId !== 'string' || opts.testId.length === 0) { - throw localValidationError('test-id', 'is required'); - } if (!Number.isInteger(opts.runs) || opts.runs < 1 || opts.runs > MAX_FLAKY_RUNS) { throw localValidationError('runs', `must be an integer between 1 and ${MAX_FLAKY_RUNS}`); } diff --git a/src/index.ts b/src/index.ts index 37920436..e46e4224 100755 --- a/src/index.ts +++ b/src/index.ts @@ -22,6 +22,7 @@ import { createUsageCommand } from './commands/usage.js'; import { resolveProfileName } from './lib/config.js'; import { classifyBillingRefusal } from './lib/billing-refusal.js'; import { TARGETS, type AgentTarget } from './lib/agent-targets.js'; +import { validatePositionalArgs } from './lib/args.js'; import { ApiError, CLIError, @@ -285,6 +286,13 @@ function telemetryGlobals(): { // action; the helper self-gates (text-only, non-dry-run, a small command // allowlist, opt-out via TESTSPRITE_NO_SKILL_WARNING) and never throws. program.hook('preAction', (_thisCommand, actionCommand) => { + const commandPath = commandPathOf(actionCommand); + // Record which leaf command ran, for the telemetry emit around parseAsync. + ranCommandPath = commandPath; + + // Validate positional arguments across all commands before executing their actions. + validatePositionalArgs(actionCommand); + const globals = actionCommand.optsWithGlobals() as { output?: string; profile?: string; @@ -295,9 +303,6 @@ program.hook('preAction', (_thisCommand, actionCommand) => { local?: string; localHost?: string; }; - const commandPath = commandPathOf(actionCommand); - // Record which leaf command ran, for the telemetry emit around parseAsync. - ranCommandPath = commandPath; // Preserve explicit --local attempts, including refusals and borrowed tunnels; // only flag presence is recorded. Automatic tunnel paths record the same fact. if (globals.local !== undefined) recordTelemetryExtras({ local: true }); diff --git a/src/lib/args.test.ts b/src/lib/args.test.ts new file mode 100644 index 00000000..b5bb3737 --- /dev/null +++ b/src/lib/args.test.ts @@ -0,0 +1,163 @@ +import { Command, Argument } from 'commander'; +import { describe, expect, it, vi } from 'vitest'; +import { formatArgumentName, validatePositionalArgs } from './args.js'; +import { createProjectCommand } from '../commands/project.js'; +import { createTestCommand } from '../commands/test.js'; + +describe('formatArgumentName', () => { + it('formats required positional argument as ', () => { + const arg = new Argument(''); + expect(formatArgumentName(arg)).toBe(''); + }); + + it('formats optional positional argument as [name]', () => { + const arg = new Argument('[environment]'); + expect(formatArgumentName(arg)).toBe('[environment]'); + }); + + it('formats required variadic argument as ', () => { + const arg = new Argument(''); + expect(formatArgumentName(arg)).toBe(''); + }); + + it('formats optional variadic argument as [name...]', () => { + const arg = new Argument('[test-ids...]'); + expect(formatArgumentName(arg)).toBe('[test-ids...]'); + }); +}); + +describe('validatePositionalArgs with throwaway Commander program', () => { + function makeProgram(actionFn = vi.fn()) { + const program = new Command(); + program.exitOverride(); + program.hook('preAction', (_thisCmd, actionCmd) => { + validatePositionalArgs(actionCmd); + }); + + program.command('test-req ').action(actionFn); + + program.command('test-opt [opt]').action(actionFn); + + program.command('test-var [rest...]').action(actionFn); + + return { program, actionFn }; + } + + it('passes when valid positional arguments are provided', async () => { + const { program, actionFn } = makeProgram(); + await program.parseAsync(['node', 'test', 'test-req', 'valid-id']); + expect(actionFn).toHaveBeenCalledWith('valid-id', expect.anything(), expect.anything()); + }); + + it('rejects empty string required positional with VALIDATION_ERROR (exit 5)', async () => { + const { program, actionFn } = makeProgram(); + await expect(program.parseAsync(['node', 'test', 'test-req', ''])).rejects.toMatchObject({ + code: 'VALIDATION_ERROR', + exitCode: 5, + nextAction: 'Argument `` is invalid: cannot be empty or whitespace-only.', + details: { + field: '', + reason: 'cannot be empty or whitespace-only', + }, + }); + expect(actionFn).not.toHaveBeenCalled(); + }); + + it('rejects whitespace-only required positional with VALIDATION_ERROR (exit 5)', async () => { + const { program, actionFn } = makeProgram(); + await expect(program.parseAsync(['node', 'test', 'test-req', ' \t '])).rejects.toMatchObject( + { + code: 'VALIDATION_ERROR', + exitCode: 5, + nextAction: 'Argument `` is invalid: cannot be empty or whitespace-only.', + }, + ); + expect(actionFn).not.toHaveBeenCalled(); + }); + + it('passes when optional positional is omitted', async () => { + const { program, actionFn } = makeProgram(); + await program.parseAsync(['node', 'test', 'test-opt']); + expect(actionFn).toHaveBeenCalled(); + }); + + it('rejects empty string optional positional when passed explicitly', async () => { + const { program, actionFn } = makeProgram(); + await expect(program.parseAsync(['node', 'test', 'test-opt', ''])).rejects.toMatchObject({ + code: 'VALIDATION_ERROR', + exitCode: 5, + nextAction: 'Argument `[opt]` is invalid: cannot be empty or whitespace-only.', + }); + expect(actionFn).not.toHaveBeenCalled(); + }); + + it('rejects whitespace-only optional positional when passed explicitly', async () => { + const { program, actionFn } = makeProgram(); + await expect(program.parseAsync(['node', 'test', 'test-opt', ' '])).rejects.toMatchObject({ + code: 'VALIDATION_ERROR', + exitCode: 5, + nextAction: 'Argument `[opt]` is invalid: cannot be empty or whitespace-only.', + }); + expect(actionFn).not.toHaveBeenCalled(); + }); + + it('rejects empty or whitespace elements in variadic positional arguments', async () => { + const { program, actionFn } = makeProgram(); + await expect( + program.parseAsync(['node', 'test', 'test-var', 'ok-first', 'valid-item', ' ']), + ).rejects.toMatchObject({ + code: 'VALIDATION_ERROR', + exitCode: 5, + nextAction: 'Argument `[rest...]` is invalid: cannot be empty or whitespace-only.', + }); + expect(actionFn).not.toHaveBeenCalled(); + }); +}); + +describe('command validation via parseAsync with real commands', () => { + function makeCliProgram(cmd: Command) { + const program = new Command(); + program.option('--dry-run', 'dry run mode'); + program.hook('preAction', (_thisCmd, actionCmd) => { + validatePositionalArgs(actionCmd); + }); + program.addCommand(cmd); + + function applyExitOverride(c: Command): void { + c.exitOverride(); + for (const sub of c.commands) { + applyExitOverride(sub); + } + } + applyExitOverride(program); + return program; + } + + it('rejects project get with whitespace project id', async () => { + const program = makeCliProgram(createProjectCommand()); + + await expect( + program.parseAsync(['node', 'testsprite', 'project', 'get', ' ']), + ).rejects.toMatchObject({ + code: 'VALIDATION_ERROR', + exitCode: 5, + nextAction: 'Argument `` is invalid: cannot be empty or whitespace-only.', + details: { + field: '', + reason: 'cannot be empty or whitespace-only', + }, + }); + }); + + it('rejects test get with empty test id under --dry-run', async () => { + const program = makeCliProgram(createTestCommand()); + + await expect( + program.parseAsync(['node', 'testsprite', 'test', 'get', '', '--dry-run']), + ).rejects.toMatchObject({ + code: 'VALIDATION_ERROR', + exitCode: 5, + nextAction: 'Argument `` is invalid: cannot be empty or whitespace-only.', + }); + }); +}); diff --git a/src/lib/args.ts b/src/lib/args.ts new file mode 100644 index 00000000..15dc1e75 --- /dev/null +++ b/src/lib/args.ts @@ -0,0 +1,64 @@ +import type { Argument, Command } from 'commander'; +import { localValidationError } from './errors.js'; + +/** + * Returns the formatted argument label matching Commander's display convention: + * `` for required arguments, `[name]` for optional arguments, with `...` + * appended for variadic arguments. + */ +export function formatArgumentName(arg: Argument): string { + const name = arg.name() + (arg.variadic ? '...' : ''); + return arg.required ? `<${name}>` : `[${name}]`; +} + +/** + * Validates positional arguments on a Commander command before its action executes. + * Rejects any empty string or whitespace-only positional argument (including variadics) + * with exit 5 VALIDATION_ERROR. + */ +export function validatePositionalArgs(command: Command): void { + const registeredArgs = command.registeredArguments; + if (!registeredArgs || registeredArgs.length === 0) { + return; + } + const processedArgs = command.processedArgs; + if (!processedArgs || processedArgs.length === 0) { + return; + } + + for (let i = 0; i < registeredArgs.length; i++) { + const argDef = registeredArgs[i]; + if (!argDef) { + continue; + } + const val = processedArgs[i]; + + if (val === undefined) { + continue; + } + + const argLabel = formatArgumentName(argDef); + + if (typeof val === 'string') { + if (val.trim().length === 0) { + throw localValidationError( + argLabel, + 'cannot be empty or whitespace-only', + undefined, + 'argument', + ); + } + } else if (Array.isArray(val)) { + for (const item of val) { + if (typeof item === 'string' && item.trim().length === 0) { + throw localValidationError( + argLabel, + 'cannot be empty or whitespace-only', + undefined, + 'argument', + ); + } + } + } + } +} diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index 5c266ecf..841625ea 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -602,6 +602,52 @@ describe('localValidationError', () => { expect(err.nextAction).toBe('Flag `--page-size` is invalid: must be a positive integer.'); expect(err.nextAction.endsWith('..')).toBe(false); }); + + it('kind=argument formats bare positional name wrapped in angle brackets', () => { + const err = localValidationError( + 'projectId', + 'cannot be empty or whitespace-only', + undefined, + 'argument', + ); + expect(err.code).toBe('VALIDATION_ERROR'); + expect(err.exitCode).toBe(5); + expect(err.nextAction).toBe( + 'Argument `` is invalid: cannot be empty or whitespace-only.', + ); + }); + + it('kind=argument preserves existing bracketed argument names', () => { + const required = localValidationError( + '', + 'cannot be empty or whitespace-only', + undefined, + 'argument', + ); + expect(required.nextAction).toBe( + 'Argument `` is invalid: cannot be empty or whitespace-only.', + ); + + const optional = localValidationError( + '[output-path]', + 'cannot be empty or whitespace-only', + undefined, + 'argument', + ); + expect(optional.nextAction).toBe( + 'Argument `[output-path]` is invalid: cannot be empty or whitespace-only.', + ); + + const variadic = localValidationError( + '', + 'cannot be empty or whitespace-only', + undefined, + 'argument', + ); + expect(variadic.nextAction).toBe( + 'Argument `` is invalid: cannot be empty or whitespace-only.', + ); + }); }); describe('ApiError.getDetail', () => { diff --git a/src/lib/errors.ts b/src/lib/errors.ts index b87452ac..6d303d56 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -396,12 +396,18 @@ export function localValidationError( field: string, reason: string, accepted?: unknown, - kind: 'flag' | 'field' = 'flag', + kind: 'flag' | 'field' | 'argument' = 'flag', ): ApiError { + let argName = field; + if (kind === 'argument' && !field.startsWith('<') && !field.startsWith('[')) { + argName = `<${field.replace(/[A-Z]/g, ch => `-${ch.toLowerCase()}`)}>`; + } const subject = kind === 'flag' ? `Flag \`--${field.replace(/[A-Z]/g, ch => `-${ch.toLowerCase()}`)}\`` - : `Field \`${field}\``; + : kind === 'argument' + ? `Argument \`${argName}\`` + : `Field \`${field}\``; const punctuatedReason = reason.endsWith('.') ? reason : `${reason}.`; return ApiError.fromEnvelope({ error: {