Skip to content

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

Merged
NagyVikt merged 2 commits into
mainfrom
agent/claude/gate-review-provider
Aug 6, 2026
Merged

fix(branch): forward --review-provider to the merge gate#689
NagyVikt merged 2 commits into
mainfrom
agent/claude/gate-review-provider

Conversation

@NagyVikt

@NagyVikt NagyVikt commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

gx branch finish --gate-review passed options: {} to runReviewGate, so the gate always used review-gate.js's codex default and ignored --review-provider. gx ship / gx finish honor the flag; gx branch finish did not.

Where codex is unavailable this makes the gate unusable — it fails closed on every run and the only way forward is --skip-review-gate, i.e. the gate disabled by the mechanism meant to enforce it. Same failure mode #677 was added to prevent.

Changes

  • splitGateReviewFlags consumes --review-provider <v> and --review-provider=<v> (the value too — agent-branch-finish.sh exits 1 on the unknown flag and would otherwise see a bare claude positional).
  • Unknown provider throws before the script runs, so a typo cannot silently downgrade to the default.
  • Bare --gate-review unchanged: reviewProvider is undefined and review-gate.js applies its own default.

Test plan

  • 3 new tests in test/branch-gate-review.test.js (flag form, inline form, invalid value). RED before the fix, GREEN after: 11/11.
  • Full suite: 802 tests, 41 failing — byte-identical failing set to main (verified by stashing the change and re-running; diff empty). No new failures.

NagyVikt added 2 commits August 6, 2026 21:30
`gx ship` / `gx finish` honor `--review-provider` — args.js parses it and
finish/index.js hands the whole options object to runReviewGate. `gx branch
finish --gate-review` passed a bare `options: {}`, so the gate always ran
review-gate.js's `codex` default no matter what the caller asked for.

On a machine where codex is unavailable that makes the gate unusable: it fails
closed on every run, and the only way to land anything is --skip-review-gate —
the gate switched off by the very mechanism meant to enforce it. Which is the
same failure mode the --gate-review routing (#677) was added to prevent.

The flag and its value are consumed here rather than passed through, because
agent-branch-finish.sh exits 1 on the unknown argument (and would otherwise see
a bare "claude" positional). An unknown provider throws before the script runs,
so a typo cannot silently downgrade to the default.

A bare --gate-review is unchanged: reviewProvider is undefined and
review-gate.js applies its own default.
args.js throws "requires a value of codex|claude" when the value is missing;
branch.js silently left reviewProvider undefined, so the gate fell back to its
codex default. A caller who asked for claude and mistyped would be gated by
codex without being told — the same silent-fallback failure this change set out
to remove.

A missing value now becomes "" so the existing validation rejects it.

@NagyVikt NagyVikt left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — adversarial pass over the diff

Decision: APPROVE. 0 CRITICAL, 0 HIGH. One MEDIUM was found during the pass and is fixed in b411c18, before merge.

Found and fixed in this PR

--review-provider with no value fell back silently (b411c18)
args.js throws --review-provider requires a value of codex|claude when the value is missing. The first commit left reviewProvider as undefined in that case, so the gate quietly used its codex default — a caller who asked for claude and mistyped would be gated by codex with no signal. That is the same silent-fallback failure this PR exists to remove. A missing value now becomes "" so the existing validation rejects it.

Checked, no finding

  • Argv consumption — the index += 1 inside the --review-provider branch correctly skips the value; the loop increment then moves past it. Pinned by deepEqual assertions that neither the flag nor its value reaches the script argv (agent-branch-finish.sh exits 1 on both).
  • Backward compatibility — a bare --gate-review leaves reviewProvider undefined and review-gate.js applies its own default, so existing callers are unchanged. Pinned by the pre-existing tests, all still green.
  • Fail-closed ordering — validation throws inside splitGateReviewFlags, i.e. before invokePackageAsset('branchFinish', ...), so a bad provider cannot reach the merge. Asserted (calls.script.length === 0).
  • Inline form--review-provider=claude handled; --review-provider= (empty) throws.
  • Security / injection — the value is compared against a fixed allowlist and never interpolated into a shell command.

Known, deliberate divergence (LOW)

branch.js normalises with .trim().toLowerCase(); args.js compares the raw token. So --review-provider CLAUDE is accepted here and rejected by gx ship. The divergence is a superset — it can never select a provider the caller did not ask for — so it is left as-is rather than tightening a working path. Worth unifying if the two parsers are ever consolidated.

Validation

Check Result
Targeted tests (test/branch-gate-review.test.js) 12/12 pass — 4 new (flag form, inline form, invalid value, missing value). RED before the fix.
Full suite 802 tests, 41 failing — failing set byte-identical to main, verified by stashing the change and re-running; diff of the sorted not ok names is empty. No new failures.
CI No checks registered on this PR.

Disclosure

This is a self-review by the change's author, not an independent one. It is recorded here because the repo has no required checks and no review bot on this PR, so nothing else would document the reasoning.

@NagyVikt
NagyVikt merged commit f501b2c into main Aug 6, 2026
1 of 3 checks passed
@NagyVikt
NagyVikt deleted the agent/claude/gate-review-provider branch August 6, 2026 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant