Code Review: PR #486

Commit: 497d73774fce42cc46c52e451c1be08c8a3ef488

Summary

Repo-qualifies review markers so same-numbered PRs across managed repos no longer collide. Marker scheme moves from reviews/<pr>-<role>.approved to reviews/<owner>__<repo>__<pr>-<role>.approved via a new single-source-of-truth helper _lib-review-markers.sh::review_marker_path. All 4 gate hooks (readers) and all 6 writers (4 skills + 2 agents) were migrated. _lib-extract-pr.sh gains a sibling extract_repo_from_command. AgDR-0060 records the decision.

Checklist Results

• Architecture & Design: Pass — clean single-source-of-truth helper; readers and writers share it
• Code Quality: Pass — shellcheck -S warning clean on all 6 files
• Testing: Pass — 25 / 19 / 9 across the three suites; cross-repo isolation genuinely proven
• Security: Pass — gate strength unchanged; no weakening
• Performance: Pass — no hot-path concern
• PR Description & Glossary: Pass — Glossary present, AgDR linked, Closes #485
• Summary Bullet Narrative: Pass — all bullets are narrative (what + why)
• Technical Decisions (AgDR): Pass — AgDR-0060 linked via <!-- agdr: -->, covers the decision + options + backward-compat
• Adopter Handbooks: N/A — no handbooks triggered (shell-only diff)

The load-bearing property — reader/writer agreement — VERIFIED

Every reader and writer computes the SAME path for the same (repo, pr, role):

• Readers (block-unreviewed-merge.sh, require-design-review-for-ui.sh, require-architecture-review.sh, warn-stale-review-markers.sh) source _lib-review-markers.sh and call review_marker_path "${CMD_REPO:-unknown}" <pr> <role> "$MARKER_HOME". CMD_REPO comes from --repo / the gh api .../pulls/N/merge URL path / extract_repo_from_command fallback.
• Writers (skills /approve-merge /approve-design /approve-architecture /design-review; agents code-reviewer solution-architect) source the same lib and call review_marker_path "$PR_REPO" <pr> <role> "$MARKER_HOME" where PR_REPO=$(gh pr view <pr> --json headRepository --jq '.headRepository.nameWithOwner').

Reader fallback #3 and every writer both derive the repo from headRepository.nameWithOwner → identical string → identical path. No reader/writer mismatch.

Repo resolution — VERIFIED

• extract_repo_from_command correctly extracts the repo for BOTH gh pr merge --repo owner/repo and gh api repos/owner/repo/pulls/N/merge, with a gh pr view last-resort fallback.
• extract_pr_number contract is unchanged (additive sibling function — no disturbance to the widely-used parser).
• Unresolvable-repo case is fail-closed: a blank repo resolves to unknown__<pr>-..., a path no writer ever writes → the gate BLOCKS rather than silently passing. Confirmed by direct path-construction test.

Gate strength — UNCHANGED (purely additive)

Two-marker requirement (rex + ceo), structured-CEO validation (sha= / approved_by=user / skill_version>=2), and the SHA-vs-PR-HEAD match are all structurally intact in block-unreviewed-merge.sh. The repo qualifier only narrows which path satisfies the gate. The #426 inline compound-command path matches BOTH the new qualified basename and the legacy bare-number form (belt-and-suspenders), backstopped by the SHA check.

Regression tests — genuinely prove isolation

Ran all three suites in the worktree:

test_block_unreviewed_merge.sh       PASS 25  FAIL 0
test_require_architecture_review.sh  PASS 19  FAIL 0
test_warn_stale_review_markers.sh    PASS 9   FAIL 0
shellcheck -S warning (6 files)      clean (exit 0)
test_site_counts.sh                  PASS (hooks=38 — _lib-* excluded, no drift)

The R1 case writes markers under org-a/project-a, drives a merge for org-b/project-b PR #100, and asserts BLOCKED (rc=2) — the real isolation proof. R2 proves correct-repo markers still pass (rc=0). R3 proves distinct coexisting files. Not vacuous.

Backward-compat & counts

New-scheme-only is safe: markers are gitignored ephemeral session state, and the SHA backstop prevents any stale cross-repo marker from producing a wrong merge. The architecture-gate BLOCKED message prints the exact qualified $APPROVAL path so a stale-marker situation is self-explanatory. _lib-review-markers.sh is excluded from the hook count (site-counts green at 38). CLAUDE.md is not touched by this PR and needs no count change.

Suggestions (non-blocking nits — cosmetic legacy-path strings, no correctness impact)

1. require-design-review-for-ui.sh:178-179 — the unblock instructions tell the user to write the legacy ${PR_NUMBER}-design.approved path. The gate itself checks the qualified $APPROVAL, so a user who follows these instructions writes a marker the gate won't find. Worth updating the heredoc to print ${APPROVAL} (as require-architecture-review.sh already does at line 169) for consistency.
2. auto-code-review.sh:61 — reminder text names the legacy ${PR_NUMBER}-rex.approved path. Purely informational (Rex computes the correct path itself), but slightly misleading now.
3. code-reviewer.md:578 — the od -c example still references the legacy basename. Cosmetic.

None of these weaken the gate or break reader/writer agreement; they're follow-up polish.

Verdict

APPROVED

The reader/writer paths agree, the gate is unweakened (additive only), the unresolvable-repo case is fail-closed, and the regression tests genuinely prove cross-repo isolation. Markers recorded at both the legacy (486-rex.approved) and qualified (me2resh__apexyard__486-rex.approved) paths so the merge works under either scheme at merge time.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 497d73774fce42cc46c52e451c1be08c8a3ef488