fix(branch): forward --review-provider to the merge gate

src/cli/commands/branch.js

@@ -11,23 +11,47 @@ const {
 const { runReviewGate } = require('../../finish/review-gate');
 const { finish, merge } = require('./finish');
 
-// `--gate-review` and its opt-outs are gx-level flags. agent-branch-finish.sh
-// does not parse them (it exits 1 on the unknown argument), and its --via-pr
-// path merges the moment the PR opens, so the shell cannot enforce the gate
-// itself. Pull the flags out of the script's argv and honor them here.
+const REVIEW_PROVIDERS = ['codex', 'claude'];
+
+// `--gate-review`, its opt-outs, and `--review-provider` are gx-level flags.
+// agent-branch-finish.sh does not parse them (it exits 1 on the unknown
+// argument), and its --via-pr path merges the moment the PR opens, so the shell
+// cannot enforce the gate itself. Pull the flags out of the script's argv and
+// honor them here.
 function splitGateReviewFlags(args) {
   const scriptArgs = [];
   let gateReview = false;
-  for (const arg of args) {
+  let reviewProvider;
+  for (let index = 0; index < args.length; index += 1) {
+    const arg = args[index];
     if (arg === '--gate-review') {
       gateReview = true;
     } else if (arg === '--no-gate-review' || arg === '--skip-review-gate') {
       gateReview = false;
+    } else if (arg === '--review-provider') {
+      // Consume the value too — leaving it behind would hand the script a bare
+      // "claude" positional and it would exit 1. A missing value becomes "" so
+      // the check below rejects it, matching args.js: falling back to the
+      // default here would gate with codex a caller who asked for claude.
+      reviewProvider = args[index + 1] ?? '';
+      index += 1;
+    } else if (arg.startsWith('--review-provider=')) {
+      reviewProvider = arg.slice('--review-provider='.length);
     } else {
       scriptArgs.push(arg);
     }
   }
-  return { gateReview, scriptArgs };
+
+  if (reviewProvider !== undefined) {
+    reviewProvider = String(reviewProvider).trim().toLowerCase();
+    // Fail closed on a typo rather than silently falling back to the default: a
+    // caller who named a provider must not quietly get a different one.
+    if (!REVIEW_PROVIDERS.includes(reviewProvider)) {
+      throw new Error(`--review-provider requires a value of ${REVIEW_PROVIDERS.join('|')}`);
+    }
+  }
+
+  return { gateReview, reviewProvider, scriptArgs };
 }
 
 // Read `--flag value` or `--flag=value` out of an argv array.
@@ -49,7 +73,7 @@ function branch(rawArgs) {
   if (subcommand === 'finish') {
     const { target, passthrough } = extractTargetedArgs(rest);
     const repoRoot = resolveRepoRoot(target);
-    const { gateReview, scriptArgs } = splitGateReviewFlags(passthrough);
+    const { gateReview, reviewProvider, scriptArgs } = splitGateReviewFlags(passthrough);
     // Fail-closed: runReviewGate throws on a dirty review, red CI, or a PR
     // GitHub will not merge. Throwing here means the script never runs, so
     // the merge never happens.
@@ -62,7 +86,9 @@ function branch(rawArgs) {
         // honors branch.<name>.guardexBase. Resolving differently would gate one
         // base and merge into another.
         baseBranch: resolveFinishBaseBranch(repoRoot, gatedBranch, readFlagValue(scriptArgs, '--base')),
-        options: {},
+        // review-gate.js falls back to its own default when this is undefined,
+        // which keeps a bare --gate-review behaving exactly as before.
+        options: { reviewProvider },
       });
     }
     invokePackageAsset('branchFinish', scriptArgs, {

test/branch-gate-review.test.js

@@ -133,3 +133,55 @@ test('branch finish without any gate flag is an unchanged passthrough', () => {
   assert.equal(calls.gate.length, 0, 'no gate without the flag');
   assert.deepEqual(calls.script[0].args, argv, 'argv must pass through untouched');
 });
+
+// `gx ship` / `gx finish` honor `--review-provider` (args.js -> finish/index.js
+// passes the whole options object to the gate), but `gx branch finish` handed
+// the gate a bare `options: {}`, so it always ran review-gate.js's `codex`
+// default. On a machine where codex is unavailable the gate then fails closed on
+// every run and the only way forward is --skip-review-gate — the gate turned off
+// by the very mechanism meant to enforce it.
+test('branch finish --gate-review forwards the review provider to the gate', () => {
+  const { branch, calls } = loadBranchWithStubs();
+
+  branch(['finish', '--branch', 'agent/claude/p', '--via-pr', '--gate-review', '--review-provider', 'claude']);
+
+  assert.equal(calls.gate[0].options.reviewProvider, 'claude');
+  const argv = calls.script[0].args;
+  assert.ok(!argv.includes('--review-provider'), 'agent-branch-finish.sh exits 1 on the unknown flag');
+  assert.ok(!argv.includes('claude'), 'the provider value must not leak into the script argv either');
+  assert.deepEqual(argv, ['--branch', 'agent/claude/p', '--via-pr']);
+});
+
+test('branch finish --gate-review reads the inline --review-provider= form', () => {
+  const { branch, calls } = loadBranchWithStubs();
+
+  branch(['finish', '--via-pr', '--gate-review', '--review-provider=claude']);
+
+  assert.equal(calls.gate[0].options.reviewProvider, 'claude');
+  assert.deepEqual(calls.script[0].args, ['--via-pr']);
+});
+
+// Fail closed on a typo rather than silently falling back to codex: a caller who
+// asked for a specific provider must not get a different one.
+test('branch finish rejects an unknown review provider before the script runs', () => {
+  const { branch, calls } = loadBranchWithStubs();
+
+  assert.throws(
+    () => branch(['finish', '--via-pr', '--gate-review', '--review-provider', 'gpt']),
+    /codex\|claude/,
+  );
+  assert.equal(calls.script.length, 0, 'the merge must never run on a bad provider');
+});
+
+// args.js throws when the value is missing ("--review-provider requires a value
+// of codex|claude"). branch.js must match: a silent fall back to the default is
+// how a caller who asked for claude ends up gated by codex without being told.
+test('branch finish rejects --review-provider with no value', () => {
+  const { branch, calls } = loadBranchWithStubs();
+
+  assert.throws(
+    () => branch(['finish', '--via-pr', '--gate-review', '--review-provider']),
+    /codex\|claude/,
+  );
+  assert.equal(calls.script.length, 0);
+});