Reject malformed and conflicting CLI input

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');

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'),

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',