diff --git a/src/cli/commands/branch.js b/src/cli/commands/branch.js index 4abbe44..5f9705f 100644 --- a/src/cli/commands/branch.js +++ b/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..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, { diff --git a/test/branch-gate-review.test.js b/test/branch-gate-review.test.js index 9ceb44a..2978866 100644 --- a/test/branch-gate-review.test.js +++ b/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); +});