Skip to content

fix(branch): honor --gate-review in gx branch finish - #677

Merged
NagyVikt merged 2 commits into
mainfrom
agent/claude/branch-finish-honor-gate-review-2026-07-09-12-52
Jul 9, 2026
Merged

fix(branch): honor --gate-review in gx branch finish#677
NagyVikt merged 2 commits into
mainfrom
agent/claude/branch-finish-honor-gate-review-2026-07-09-12-52

Conversation

@NagyVikt

@NagyVikt NagyVikt commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Problem

gx branch finish passed its argv straight to agent-branch-finish.sh via invokePackageAsset, bypassing src/finish/index.js where runReviewGate lives. Two consequences:

  1. The shell script exits 1 on the unknown --gate-review argument.
  2. Its --via-pr path calls gh pr merge --squash unconditionally right after opening the PR (agent-branch-finish.sh:1267), before the wait/auto fallbacks — i.e. it merges fail-open.

So the workflow every repo's CLAUDE.md documents as the default could never gate, and merged without one. review-gate.js's own header already noted this ("can fail open (it merged PR #610 to main with red preflight tests)") — the gate existed, but only gx ship / gx finish ever ran it.

This is the gap that let lifted.sk-storefront PR #298 merge to production with its review check skipped.

Change

  • Split the gx-level gate flags out of the script's argv and run runReviewGate before invoking the script. It throws on a dirty review, red CI, or a PR GitHub will not merge, so the script — and the merge — never runs.
  • Unrelated flags (--auto-resolve, --no-preflight, …) still reach the script untouched.
  • Resolve the gated base with resolveFinishBaseBranch, matching how the shell resolves an omitted --base (per-branch branch.<name>.guardexBase). Resolving it differently would review one base and merge into another.

Verification

  • test/branch-gate-review.test.js (8 tests): gate runs, flag stripped, fail-closed (a throwing gate never invokes the script), opt-outs, HEAD fallback, per-branch base, inline --branch=/--base=, and a byte-for-byte passthrough guard for callers without the flag.
  • All 8 fail against the pre-fix branch.js.
  • Full suite: 763 pass / 27 fail, 0 new failures vs the main baseline (this repo is baseline-red; the failing set is byte-identical).
  • Verified live: running the fixed gx branch finish --gate-review on a storefront branch refused to merge when the review provider errored — "Refusing to merge."

Known follow-ups (from review, not blocking)

  • options: {} hard-codes provider codex + requireChecks; --review-provider / --allow-no-checks are not parsed on this path and would reach the shell as unknown args.
  • GUARDEX_AUTO_SHIP=1 does not enable the gate here, though args.js documents that it should.

🤖 Generated with Claude Code

NagyVikt and others added 2 commits July 9, 2026 13:12
`gx branch finish` passed its argv straight to agent-branch-finish.sh via
invokePackageAsset, bypassing src/finish/index.js where runReviewGate lives.
Two consequences: the script exits 1 on the unknown `--gate-review` argument,
and its --via-pr path merges the moment the PR opens (the unconditional
`gh pr merge` before the wait/auto fallbacks). So the documented default
workflow could never gate, and merged fail-open.

Split the gx-level gate flags out of the script's argv and run runReviewGate
before invoking the script. It throws on a dirty review, red CI, or a PR
GitHub will not merge, so the script -- and the merge -- never runs. Unrelated
flags (--auto-resolve, --no-preflight, ...) still reach the script untouched.

This is the gap that let lifted.sk-storefront PR #298 merge with its `review`
check skipped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code review caught a wrong-merge-target hazard. The gate resolved the base with
resolveBaseBranch, which only knows the explicit --base, the global config, and
the detected default. agent-branch-finish.sh instead honors the per-branch
branch.<name>.guardexBase when --base is omitted (agent-branch-finish.sh:504),
as does `gx finish` via resolveFinishBaseBranch.

With `branch.agent/x.guardexBase=dev` and no --base, the gate would open and
review a PR against main while the shell merged into dev — reviewing one base
and merging into another. Use resolveFinishBaseBranch so both agree.

Cover it, plus the inline --branch=/--base= form and the no-gate-flag
passthrough that every repo depends on. The suite's git stub now exposes
resolveFinishBaseBranch, so a regression to the old helper fails loudly.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@NagyVikt
NagyVikt merged commit 1925f9d into main Jul 9, 2026
2 of 3 checks passed
@NagyVikt
NagyVikt deleted the agent/claude/branch-finish-honor-gate-review-2026-07-09-12-52 branch July 9, 2026 12:31
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