Code Review: PR #527 — APPROVED (posted as comment; cannot self-approve own PR)

Commit: 44604ffb0195944d47a57135af6b4f6922ec95d0

Summary

Turns the ~65-script mechanical-enforcement test corpus into an actual CI gate. Adds a bash-3.2-portable glob-discovery runner (bin/run-hook-tests.sh), a tests.yml workflow (PR + push to dev/main), AgDR-0067 (decision + quarantine-as-ratchet policy), and two in-scope fixes to land the gate green. The PR's own hook test suite check is GREEN at HEAD (PASS=60, FAIL=0, SKIP=5).

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass (this PR is the testing infrastructure)
• Security: Pass (permissions: contents: read, minimal scope)
• Performance: Pass (per-test 120s cap; concurrency cancel-in-progress)
• PR Description & Glossary: Pass
• Summary Bullet Narrative: Pass (narrative, each with rationale)
• Technical Decisions (AgDR):Pass (AgDR-0067 present + linked; Refs #526)
• Adopter Handbooks: N/A (no migration/TS/domain code in a shell-only diff)

Verification performed (against PR HEAD, not local tree)

1. CI green at HEAD — hook test suite check = success (PASS=60/FAIL=0/SKIP=5). The gate fails-on-real-failures by design: set -uo pipefail (not set -e, so failures aggregate rather than abort the loop), rc=$? captured, exit 1 when fail>0.
2. Tracking issue #528 is real + well-formed — OPEN, lists all 5 quarantined tests with root causes and an AC that drains QUARANTINE to empty. A genuine ratchet, not a graveyard.
3. Quarantine legitimacy spot-checked — test_token_efficiency_wave1.sh asserts a 200-char SKILL-description hard cap (matches cited "plan-initiative >200 chars" drift); test_handover_clone_prompt.sh asserts pre-restructure clone-prompt strings ([y / n / later], "Offer the clone-first") which the SKILL has since moved. Both genuine pre-existing drift, not caused by this PR. All 5 SKIP-logged with cited reasons — visible, never silent.
4. test_setup_reset fix aligns to #517 — at PR HEAD onboarding.yaml is NOT tracked (404 via contents API), onboarding.example.yaml exists (4640 bytes) with the \"Your Company Name\" placeholder, and /setup --reset does cp onboarding.example.yaml onboarding.yaml (SKILL.md L80). Test now reads the correct source.
5. CLAUDE.md /handover trim accurate — 35→24 words; preserves the three substantive facts (5-dimension scoring, checklist doc-pick, Next-Steps ticket offer). No information loss.

Runner correctness (focus #1)

• bash 3.2 portable — while IFS= read + < <(...) instead of mapfile; POSIX param expansions. Good.
• Empty-QUARANTINE guard under set -u — is_quarantined short-circuits on ${#QUARANTINE[@]} -gt 0 before iterating, so draining the array in #528 won't trip an unbound-array expansion. Forward-looking + correct.
• timeout fallback — timeout→gtimeout probe, empty default, intentional unquoted word-split (SC2086 disabled). Correct.
• exit codes — aggregates, prints failed list, exits 1 on any failure. Correct.

Issues Found

None blocking.

Suggestions (non-blocking)

• nit: the main run loop for t in \"${TESTS[@]}\" lacks the empty-array guard that is_quarantined and --list have. Under set -u+bash 3.2 a zero-test discovery would expand as unbound. Not reachable today (60+ tests) — but a one-line [ \"${#TESTS[@]}\" -gt 0 ] || { echo \"no tests discovered\"; exit 1; } would turn a silent-empty-pass into a loud failure, the safer direction for a gate.

Verdict

APPROVED (Rex first-pass). Approval marker written for the merge gate. Human approver does the second review before merge.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 44604ffb0195944d47a57135af6b4f6922ec95d0