diff --git a/scripts/cli-args.mjs b/scripts/cli-args.mjs index 87b1e97..6ecb192 100644 --- a/scripts/cli-args.mjs +++ b/scripts/cli-args.mjs @@ -101,6 +101,19 @@ function githubRepoFromPullRequest(value) { } } +function validPullRequest(value) { + if (/^[1-9]\d*$/.test(value)) return true; + try { + const url = new URL(value); + return ( + (url.protocol === 'https:' || url.protocol === 'http:') && + /^\/[^/]+\/[^/]+\/pull\/[1-9]\d*(?:\/|$)/.test(url.pathname) + ); + } catch { + return false; + } +} + function remoteRepo(value, callerDirectory, pathExists) { if (pathExists(resolve(callerDirectory, value))) return undefined; if ( @@ -137,32 +150,38 @@ export function parseCliArgs( const argument = rawArgs[index]; const parsed = splitOption(argument); if (!parsed) { + if (argument.startsWith('-')) fail(`Unknown option: ${argument}`); + if (!argument) fail('Repo cannot be empty'); positionals.push(argument); continue; } if (parsed.name === '--agent') { + if (agentSet) fail('--agent was passed more than once'); agentSet = true; if (parsed.value !== undefined) { if (!parsed.value) fail('--agent needs a value'); agent = parsed.value; } else { const next = rawArgs[index + 1]; - if (next && !next.startsWith('-')) { - agent = next; - index += 1; - } + if (!next || splitOption(next)) fail('--agent needs a value'); + agent = next; + index += 1; } continue; } if (parsed.name === '--no-agent') { + if (noAgent) fail('--no-agent was passed more than once'); if (parsed.value !== undefined) fail('--no-agent does not take a value'); noAgent = true; continue; } if (flagOptions.has(parsed.name)) { + if (options.has(parsed.name)) { + fail(`${parsed.name} was passed more than once`); + } if (parsed.value !== undefined) { fail(`${parsed.name} does not take a value`); } @@ -178,7 +197,7 @@ export function parseCliArgs( let value = parsed.value; if (value === undefined) { value = rawArgs[index + 1]; - if (!value || value.startsWith('--')) fail(`${parsed.name} needs a value`); + if (!value || splitOption(value)) fail(`${parsed.name} needs a value`); index += 1; } if (!value) fail(`${parsed.name} needs a value`); @@ -214,6 +233,9 @@ export function parseCliArgs( if (!branch && !pullRequest && !worktree && Boolean(base) !== Boolean(head)) { fail('--base and --head must be used together'); } + if (pullRequest && !validPullRequest(pullRequest)) { + fail('--pr must be a positive number or a pull request URL'); + } const repoArgument = positionals[0] || options.get('--repo'); let remote = options.get('--remote'); diff --git a/tests/cli-args.test.mjs b/tests/cli-args.test.mjs index 68d3d65..320040b 100644 --- a/tests/cli-args.test.mjs +++ b/tests/cli-args.test.mjs @@ -64,6 +64,96 @@ test('accepts a repo URL and pull request', () => { ]); }); +test('keeps split and equals-style values consistent', () => { + const split = parseCliArgs( + [ + '--repo', + 'repos/widgets', + '--base', + 'refs/heads/main', + '--head', + 'refs/heads/topic', + '--remote', + 'upstream', + '--summaries', + 'state/notes.json', + '--output', + 'state/review.json', + '--cache-dir', + 'state/git', + '--codex-bin', + 'bin/codex', + '--model', + 'review-model', + '--port', + '0', + ], + { + callerDirectory: cwd, + pathExists: (path) => path === resolve(cwd, 'repos/widgets'), + }, + ); + const equals = parseCliArgs( + [ + '--repo=repos/widgets', + '--base=refs/heads/main', + '--head=refs/heads/topic', + '--remote=upstream', + '--summaries=state/notes.json', + '--output=state/review.json', + '--cache-dir=state/git', + '--codex-bin=bin/codex', + '--model=review-model', + '--port=0', + ], + { + callerDirectory: cwd, + pathExists: (path) => path === resolve(cwd, 'repos/widgets'), + }, + ); + + assert.deepEqual(equals, split); + assert.deepEqual(split.feedArgs, [ + '--repo', + resolve(cwd, 'repos/widgets'), + '--base', + 'refs/heads/main', + '--head', + 'refs/heads/topic', + '--remote', + 'upstream', + '--summaries', + resolve(cwd, 'state/notes.json'), + '--output', + resolve(cwd, 'state/review.json'), + '--cache-dir', + resolve(cwd, 'state/git'), + ]); + assert.deepEqual(split.agentArgs.slice(-4), [ + '--codex-bin', + resolve(cwd, 'bin/codex'), + '--model', + 'review-model', + ]); +}); + +test('passes the worktree target to both builders unchanged', () => { + const parsed = parseCliArgs( + ['--repo', 'repos/widgets', '--worktree', '--no-agent'], + { + callerDirectory: cwd, + pathExists: (path) => path === resolve(cwd, 'repos/widgets'), + }, + ); + + assert.deepEqual(parsed.feedArgs, [ + '--repo', + resolve(cwd, 'repos/widgets'), + '--worktree', + ]); + assert.deepEqual(parsed.agentArgs, parsed.feedArgs); +}); + test('keeps an existing repo path local', () => { const parsed = parseCliArgs(['repos/widgets', '--pr', '42'], { callerDirectory: cwd, @@ -126,6 +216,66 @@ test('rejects an unknown coding agent', () => { ); }); +test('rejects a missing value for every value option', () => { + for (const option of [ + '--repo', + '--branch', + '--pr', + '--base', + '--head', + '--remote', + '--summaries', + '--output', + '--cache-dir', + '--codex-bin', + '--model', + '--reasoning', + '--batch-size', + '--jobs', + '--port', + '--agent', + ]) { + assert.throws( + () => parseCliArgs([option]), + new RegExp(`${option} needs a value`), + option, + ); + assert.throws( + () => parseCliArgs([`${option}=`]), + new RegExp(`${option} needs a value`), + `${option}=`, + ); + assert.throws( + () => parseCliArgs([option, '--help']), + new RegExp(`${option} needs a value`), + `${option} --help`, + ); + } +}); + +test('rejects duplicate options and aliases', () => { + for (const args of [ + ['--repo', 'first', '--repo', 'second'], + ['--agent', 'codex', '--agent=claude'], + ['--no-agent', '--no-agent'], + ['--worktree', '--worktree'], + ['--force', '--force'], + ['-h', '--help'], + ['-v', '--version'], + ]) { + assert.throws( + () => parseCliArgs(args), + /was passed more than once/i, + args.join(' '), + ); + } +}); + +test('rejects unknown short options and empty positional repos', () => { + assert.throws(() => parseCliArgs(['-x']), /unknown option: -x/i); + assert.throws(() => parseCliArgs(['']), /repo cannot be empty/i); +}); + test('accepts short help and version flags', () => { assert.deepEqual(parseCliArgs(['-h']), { help: true }); assert.deepEqual(parseCliArgs(['-v']), { version: true }); @@ -207,6 +357,48 @@ test('rejects invalid reasoning and batch settings', () => { ); }); +test('rejects invalid ports', () => { + for (const port of ['-1', '1.5', 'word', '65536']) { + for (const args of [['--port', port], [`--port=${port}`]]) { + assert.throws( + () => parseCliArgs(args), + /--port must be a number from 0 to 65535/i, + args.join(' '), + ); + } + } +}); + +test('rejects unsupported pull request values', () => { + for (const pullRequest of ['0', 'topic', 'https://github.com/acme/widgets']) { + assert.throws( + () => parseCliArgs(['--pr', pullRequest]), + /--pr must be a positive number or a pull request url/i, + pullRequest, + ); + } +}); + +test('rejects conflicting review targets', () => { + for (const args of [ + ['--branch', 'topic', '--pr', '42'], + ['--pr', '42', '--base', 'main'], + ['--pr', '42', '--head', 'topic'], + ['--branch', 'topic', '--head', 'other'], + ['--worktree', '--branch', 'topic'], + ['--worktree', '--pr', '42'], + ['--worktree', '--base', 'main', '--head', 'topic'], + ['--base', 'main'], + ['--head', 'topic'], + ]) { + assert.throws( + () => parseCliArgs(args), + /cannot|must be used together/i, + args.join(' '), + ); + } +}); + test('publishes the diffsplain executable', async () => { const packageJson = JSON.parse( await readFile(new URL('../package.json', import.meta.url), 'utf8'), diff --git a/tests/present-help.test.mjs b/tests/present-help.test.mjs index 28167bc..c00413d 100644 --- a/tests/present-help.test.mjs +++ b/tests/present-help.test.mjs @@ -32,6 +32,35 @@ test('prints the package version with either version flag', async () => { } }); +test('reports missing option values before startup', () => { + for (const option of [ + '--repo', + '--branch', + '--pr', + '--base', + '--head', + '--remote', + '--summaries', + '--output', + '--cache-dir', + '--codex-bin', + '--model', + '--reasoning', + '--batch-size', + '--jobs', + '--port', + '--agent', + ]) { + const result = spawnSync(process.execPath, [script, option], { + encoding: 'utf8', + env: { ...process.env, PATH: '' }, + }); + assert.equal(result.status, 2, `${option}: ${result.stderr}`); + assert.match(result.stderr, new RegExp(`${option} needs a value`)); + assert.match(result.stderr, /diffsplain --help/); + } +}); + test('fails before startup when no coding agent is installed', () => { const result = spawnSync(process.execPath, [script], { encoding: 'utf8',