refactor: remove the duplicate plain pr comment path

packages/client/src/commands/gh.ts

@@ -63,14 +63,22 @@ ${ACTION_LINES}`;
  * Run the `gh` command.
  *
  * @param argv - arguments after `gh`.
- * @returns the process exit code: 0 success, 1 refusal.
+ * @returns the process exit code: 0 success, 1 refusal, and the requested unreachable code
+ *          (1 by default; `pr` passes 3 for runPr's contract).
  */
-export async function runGh(argv: readonly string[]): Promise<number> {
-  if (argv.length === 0 || argv.includes('--help') || argv.includes('-h')) {
+export async function runGh(
+  argv: readonly string[],
+  options: { readonly unreachableExitCode?: number } = {},
+): Promise<number> {
+  if (argv.length === 0) {
     say(GH_USAGE);
-    return argv.length === 0 ? 1 : 0;
+    return 1;
+  }
+  const [domain, command, ...rawRest] = argv;
+  if (domain === '--help' || domain === '-h' || command === '--help' || command === '-h') {
+    say(GH_USAGE);
+    return 0;
   }
-  const [domain, command, ...rest] = argv;
   if (domain === undefined || command === undefined) {
     // A caller with a domain and no command (`gcb run`, `gcb gh pr`) gets the catalog rather than one
     // example: the answer is every action, and the example would only be right for one of them.
@@ -82,11 +90,95 @@ export async function runGh(argv: readonly string[]): Promise<number> {
   try {
     action = getGhAction(actionName);
   } catch {
+    if (rawRest.includes('--help') || rawRest.includes('-h')) {
+      say(GH_USAGE);
+      return 0;
+    }
     fail(`unknown gh action ${JSON.stringify(actionName)}`);
   }
 
+  // A known value-taking flag consumes the next token only when that token is a value. A following
+  // known flag is not taken as a value (so a missing value fails as missing), except a literal
+  // --help/-h: a body that is exactly one of those is data, not a help request. The parser then
+  // sees --body=--help.
+  const valueFlags = new Set([
+    ...action.flags.filter((flag) => flag.type !== 'boolean').map((flag) => flag.name),
+    'repo',
+    'R',
+    'host',
+    'socket',
+  ]);
+
+  const allowed = new Set([
+    ...action.flags.map((flag) => flag.name),
+    'repo',
+    'R',
+    'host',
+    'socket',
+  ]);
+  const knownFlags = new Set([...allowed, 'help']);
+
+  const rest: string[] = [];
+  let missingValue: string | undefined;
+  let help = false;
+  for (let index = 0; index < rawRest.length; index += 1) {
+    const raw = rawRest[index];
+    // Match parseArgs: -h is help everywhere; any other single-dash letter is its long flag. A
+    // single-dash operand belongs to a preceding value flag and was consumed before this point.
+    const argument = raw?.replace(/^-h$/, '--help').replace(/^-(?=[A-Za-z])/, '--');
+    if (argument === undefined) continue;
+    if (argument === '--') {
+      rest.push(...rawRest.slice(index));
+      break;
+    }
+    if (argument === '--help') {
+      help = true;
+      rest.push(argument);
+      continue;
+    }
+    const name = argument.startsWith('--') && !argument.includes('=') ? argument.slice(2) : undefined;
+    if (name === undefined || !valueFlags.has(name)) {
+      rest.push(argument);
+      continue;
+    }
+    const next = rawRest[index + 1];
+    if (next === undefined) {
+      missingValue ??= `${raw} needs a value`;
+      rest.push(argument);
+      continue;
+    }
+    if (next === '--help' || next === '-h') {
+      if (name === 'body') {
+        rest.push(`${argument}=${next}`);
+        index += 1;
+      } else {
+        help = true;
+        rest.push(argument);
+      }
+      continue;
+    }
+    if (next.startsWith('--')) {
+      const nextName = next.slice(2).split('=')[0];
+      if (nextName !== undefined && knownFlags.has(nextName)) {
+        missingValue ??= `${raw} needs a value`;
+      }
+      // Leave both tokens alone: a known flag is a missing value, an unknown one must reach the
+      // unknown-option check rather than becoming a value.
+      rest.push(argument);
+      continue;
+    }
+    rest.push(`${argument}=${next}`);
+    index += 1;
+  }
+  if (help) {
+    say(GH_USAGE);
+    return 0;
+  }
+  if (missingValue !== undefined) fail(missingValue);
+
   const booleans = action.flags.filter((flag) => flag.type === 'boolean').map((flag) => flag.name);
   const args = parseArgs(rest, { booleans });
+
   const host = args.value('host') ?? 'github.com';
   let repo = args.value('repo') ?? args.value('R');
   if (repo === undefined) {
@@ -100,14 +192,8 @@ export async function runGh(argv: readonly string[]): Promise<number> {
   const socketPath = resolveSocketPath(args.value('socket'), process.env);
   if (socketPath === undefined) fail('no broker socket is configured');
 
-  const allowed = new Set([
-    ...action.flags.map((flag) => flag.name),
-    'repo',
-    'R',
-    'host',
-    'socket',
-  ]);
   for (const argument of rest) {
+    if (argument === '--') break;
     const match = /^--([^=]+)/.exec(argument);
     if (match?.[1] !== undefined && !allowed.has(match[1])) {
       fail(`unknown option --${match[1]} for gcb gh ${domain} ${command}`);
@@ -148,7 +234,8 @@ export async function runGh(argv: readonly string[]): Promise<number> {
       action.timeoutSeconds * 1000 + 30_000,
     );
   } catch (error) {
-    fail((error as Error).message);
+    process.stderr.write(`unreachable: ${(error as Error).message}\n`);
+    return options.unreachableExitCode ?? 1;
   }
   if (response.ok !== true) {
     if (response.warning) process.stderr.write(`warning: ${response.warning}\n`);

packages/client/src/commands/pr.ts

@@ -53,7 +53,8 @@ Opening (the default)
 Acting on one
   --number <n>         The pull request number                       [required]
   --status             Report its state, mergeability and the runs for its commit
-  --comment            Post the body as a comment on it (needs only --number and --body/--body-file)
+  --comment            Post an anchored review comment (also needs --file and --line);
+                       for a plain comment use: gcb gh pr comment <number> --body ...
   --file <path>        Anchor that comment to a line of this file, repository-relative
   --line <n>           The line to anchor to, in the new file unless --side says otherwise
   --side <left|right>  Which side of the diff --line counts on (default: right)
@@ -115,6 +116,11 @@ export async function runPr(argv: readonly string[]): Promise<number> {
   if (args.has('close') || args.value('close') !== undefined) {
     fail('--close moved to: gcb gh pr close <number> --repo owner/name');
   }
+  // Before --repo too: a bare `gcb pr --comment` should hear what the flag needs, not that --repo
+  // is missing. An anchored comment (--file and --line) is still this command's to post.
+  if (args.has('comment') && args.value('file') === undefined && args.value('line') === undefined) {
+    fail('--comment only posts an anchored review comment; pass --file and --line');
+  }
 
   const socketPath = resolveSocketPath(args.value('socket'), process.env);
   const host = args.value('host');

packages/client/src/index.ts

@@ -64,7 +64,7 @@ async function run(argv: readonly string[]): Promise<number> {
       if (rest.length === 0) return (await import('./commands/gh.ts')).runGh(['pr']);
       const sub = rest[0];
       if (sub !== undefined && Object.hasOwn(GH_ACTIONS, `pr:${sub}`)) {
-        return (await import('./commands/gh.ts')).runGh(['pr', ...rest]);
+        return (await import('./commands/gh.ts')).runGh(['pr', ...rest], { unreachableExitCode: 3 });
       }
       return (await import('./commands/pr.ts')).runPr(rest);
     }

test/unit/commands.test.ts

@@ -1166,6 +1166,16 @@ describe('the anchor flags, at the CLI boundary', () => {
     assert.match(result.stdout, /Usage: gcb gh/);
   });
 
+  it('refuses a plain --comment and names the anchor requirement', () => {
+    const moved = cli([...base, '--comment', '--body', 'x']);
+    assert.equal(moved.status, 1);
+    assert.match(moved.stderr, /anchored review comment/);
+
+    const bare = cli(['--comment']);
+    assert.equal(bare.status, 1);
+    assert.match(bare.stderr, /anchored review comment/, 'before --repo too');
+  });
+
   it('points --close at the gh action', () => {
     const moved = cli([...base, '--close']);
     assert.equal(moved.status, 1);

test/unit/gh-command.test.ts

@@ -240,6 +240,68 @@ describe('gcb gh', () => {
     }
   });
 
+  it('does not mistake a flag value for a help request', async () => {
+    const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-body-help-'));
+    const socketPath = path.join(root, 'broker.sock');
+    const block: StaticHostConfig = { provider: 'static', allow: ['acme/widget'] };
+    const config: BrokerConfig = {
+      socketPath,
+      socketMode: 0o600,
+      auditPath: null,
+      tokenCacheSkewSeconds: 300,
+      hosts: { 'github.com': block },
+    };
+    const audit: AuditSink = { path: null, record: () => {}, close: () => Promise.resolve() };
+    let seen: unknown;
+    const provider: Provider = {
+      name: 'static',
+      getCredential: async () => ({ username: 'u', password: 'p', expiresAt: Date.now() + 1000 }),
+      runAction: async (request) => {
+        seen = request.values;
+        return { stdout: '', stderr: '', code: 0, signal: null, truncated: false, timedOut: false };
+      },
+    };
+    const broker = await startBroker({ config, audit, providers: new Map([['github.com', provider]]) });
+    try {
+      const result = await runGhCli([
+        'pr', 'comment', '7', '--body', '--help', '--repo', 'acme/widget', '--socket', socketPath,
+      ]);
+      assert.equal(result.status, 0);
+      assert.deepEqual(seen, { number: '7', body: '--help' });
+
+      const shortHelp = await runGhCli([
+        'pr', 'comment', '7', '--body', '-h', '--repo', 'acme/widget', '--socket', socketPath,
+      ]);
+      assert.equal(shortHelp.status, 0);
+      assert.deepEqual(seen, { number: '7', body: '-h' }, 'a body of literally -h is data too');
+    } finally {
+      await broker.close();
+      fs.rmSync(root, { recursive: true, force: true });
+    }
+  });
+
+  it('treats --help as help wherever it sits', async () => {
+    // No broker is involved: each case has to answer before the socket is looked for.
+    for (const argv of [
+      ['--help'],
+      ['pr', 'view', '--help', '7'],
+      ['pr', 'view', '7', '--help'],
+      ['pr', 'view', '7', '-h'],
+      ['pr', 'create', '--title', '--head', '--help'],
+      ['pr', 'create', '--title', '-h', '--head', 'feat', '--body', 'b'],
+    ]) {
+      const result = await runGhCli(argv);
+      assert.equal(result.status, 0, argv.join(' '));
+      assert.match(result.stdout, /Usage: gcb gh/);
+    }
+  });
+
+  it('keeps the pr unreachable exit code', async () => {
+    const result = await runClient(['pr', 'view', '7', '--repo', 'acme/widget', '--socket', '/nonexistent']);
+    assert.equal(result.status, 3);
+    assert.match(result.stderr, /unreachable/);
+  });
+
   it('sends a typed action to the broker and prints its stdout', async () => {
     const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gcb-gh-command-'));
     const socketPath = path.join(root, 'broker.sock');
@@ -280,3 +342,65 @@ describe('bare run', () => {
     assert.match(result.stdout, /Usage: gcb gh/);
   });
 });
+
+describe('gh option values', () => {
+  it('refuses an optional value flag followed by another flag', async () => {
+    const result = await runGhCli([
+      'pr', 'create', '--head', 'feat', '--title', 't', '--body', 'b', '--reviewer', '--draft',
+      '--repo', 'acme/widget', '--socket', '/nonexistent',
+    ]);
+    assert.equal(result.status, 1);
+    assert.match(result.stderr, /--reviewer needs a value/);
+  });
+
+  it('refuses a value flag followed by another flag', async () => {
+    const result = await runGhCli([
+      'pr', 'create', '--head', 'feat', '--title', '--draft', '--body', 'b',
+      '--repo', 'acme/widget', '--socket', '/nonexistent',
+    ]);
+    assert.equal(result.status, 1);
+    assert.match(result.stderr, /--title needs a value/);
+  });
+});
+
+describe('help after a value flag', () => {
+  it('prints help instead of a missing-value error', async () => {
+    const result = await runGhCli([
+      'pr', 'create', '--title', '--help', '--head', 'feat', '--body', 'b',
+      '--repo', 'acme/widget', '--socket', '/nonexistent',
+    ]);
+    assert.equal(result.status, 0);
+    assert.match(result.stdout, /Usage: gcb gh/);
+  });
+});
+
+describe('unknown gh options', () => {
+  it('does not let an unknown flag become a value', async () => {
+    const result = await runGhCli([
+      'pr', 'comment', '7', '--body', 'x', '--bogus', 'y',
+      '--repo', 'acme/widget', '--socket', '/nonexistent',
+    ]);
+    assert.equal(result.status, 1);
+    assert.match(result.stderr, /unknown option --bogus/);
+  });
+});
+
+describe('short gh flags', () => {
+  it('reports a missing -R value using the typed spelling', async () => {
+    const result = await runGhCli(['pr', 'view', '7', '-R']);
+    assert.equal(result.status, 1);
+    assert.match(result.stderr, /-R needs a value/);
+  });
+});
+
+describe('the -- terminator', () => {
+  it('does not treat the token after -- as help', async () => {
+    const result = await runGhCli([
+      'pr', 'view', '7', '--repo', 'acme/widget', '--socket', '/nonexistent',
+      '--', '--help',
+    ]);
+    assert.equal(result.status, 1);
+    assert.doesNotMatch(result.stdout, /Usage: gcb gh/);
+    assert.match(result.stderr, /too many positional arguments/);
+  });
+});

tsconfig.build.base.json

@@ -4,7 +4,6 @@
     "lib": ["ES2023"],
     "module": "nodenext",
     "moduleResolution": "nodenext",
-    "resolveJsonModule": true,
     "types": ["node"],
 
     "strict": true,

tsconfig.json

@@ -4,7 +4,6 @@
     "lib": ["ES2023"],
     "module": "nodenext",
     "moduleResolution": "nodenext",
-    "resolveJsonModule": true,
     "types": ["node"],
 
     "strict": true,