Code Review: PR #581

Commit: 8aacab0aa9c34c6c20bf3301ff68a06ce6d8c715

Summary

Fixes the extract_pr_number() bug (#568) where gh pr merge $pr --squash 2>&1 | tail -5 (with $pr unexpanded at hook-eval time) caused the merge gate to latch onto the 2 from 2>&1 and block with a fabricated "PR #2". The fix (a) strips redirection tokens (2>&1, &>, >>, >, N>file) from the merge span before any digit search, and (b) requires the first post-merge token to be a bare integer — an unexpanded $var/${VAR} now returns empty so the step-3 gh pr view fallback runs instead. Adds a focused 14-case test_extract_pr.sh.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass — 14 new focused cases, clear comments, gh shimmed to isolate the string-parsing layer
• Security: Pass — verified fail-closed (see below)
• Performance: Pass — N/A (string parsing, no hot path)
• PR Description & Glossary: Pass — Glossary present; Summary bullets are narrative (what + why)
• Summary Bullet Narrative: Pass
• Technical Decisions (AgDR): N/A — bug fix to existing parsing logic, no new dependency/pattern/architecture choice
• Adopter Handbooks: N/A — diff touches only .claude/hooks/*.sh; no migration files (migration-safety blocking handbook does not fire), no domain/application layers, no commits in diff

CRITICAL SECURITY VERIFICATION — fail-closed confirmed

The dangerous direction (empty PR number must NOT let an unverified merge through) was traced through all three merge-gate hooks and verified by running the PR581 hook in isolation:

• block-unreviewed-merge.sh (lines 83–86): empty PR number → exit 2 (BLOCK). This is the load-bearing fail-closed gate.
• block-merge-on-red-ci.sh (lines 59–62): empty PR → exit 0 (deliberately defers — "another hook will handle 'no PR number'").
• require-design-review-for-ui.sh (lines 76–79): empty PR → exit 0 (deliberately defers to block-unreviewed-merge).

All three are wired to the same Bash PreToolUse matcher, so any single exit 2 blocks the merge regardless of hook order. The deferral in the two CI/design hooks is sound because block-unreviewed-merge always fires the hard block on empty.

Live proof (PR581 lib + gh shimmed to fail so the step-3 fallback cannot rescue a PR number):

EMPTY-VAR case: gh pr merge $pr --squash 2>&1 | tail -5
  → "BLOCKED: Could not determine PR number for merge." → EXIT 2  (FAIL-CLOSED)
REAL-DIGIT control: gh pr merge 42 ... 2>&1 | tail -5
  → extracts 42 → blocks on missing Rex/CEO marker → EXIT 2  (correct reason)

Contrast with the old behavior (reproduced live during review): the old extractor turned $pr ... 2>&1 into "PR #2" — a fail-wrong block on a fabricated number. The fix converts that into a clean fail-closed block. No fail-open regression.

Redirection-stripping correctness — no real PR digit dropped

Verified the strip does not eat a real PR number across redirect forms and adjacency:

gh pr merge 42 --squash 2>&1 | tail -5   → 42
gh pr merge 7 2>&1                        → 7
gh pr merge 100 --repo o/r 2>err.log      → 100
gh pr merge 8 --squash >>log 2>&1         → 8
gh pr merge 42>log                        → 42   (no space, adjacent redirect)
gh pr merge 9 --auto --squash 1>out 2>&1  → 9
gh api repos/o/r/pulls/55/merge ... 2>&1  → 55   (URL path wins over the 2)

Note (not a bug): gh pr merge 1>out resolves to PR 1 and gh pr merge 2>out to 2. These are genuinely ambiguous at the shell level — 1>out/2>out is both a positional arg and an fd redirect, and the shell itself would treat the leading digit as the PR number. The extractor matches real shell semantics and the result is a concrete number the SHA/marker checks then validate against GitHub. Not fail-open.

macOS portability — confirmed

• Verified grep -oE '\bmerge\b[[:space:]]+...' works on Darwin (BSD grep supports \b).
• Confirmed the GNU-only \b-in-sed was removed: the only \b-inside-sed occurrences in the file are in comments (lines 106–107) explaining the rationale; no executable sed uses \b. The redirection-strip sed uses only POSIX-portable [0-9]*>&[0-9]*, &>, >>, > with [^[:space:]].

Test results (run during review)

• test_extract_pr.sh (PR581 version): 14 PASS, 0 FAIL
• test_block_unreviewed_merge.sh (working tree): 29 PASS, 0 FAIL

Issues Found

None.

Suggestions

• nit (non-blocking): consider adding the gh pr merge 1>out ambiguity as an explicit documented test case so a future refactor doesn't "fix" it in a way that changes shell-faithful behavior. Not required for merge.

Verdict

APPROVED

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 8aacab0aa9c34c6c20bf3301ff68a06ce6d8c715