Code Review: PR #507

Commit: ed38a63abc20e856b6fc2096d4d4a0adc1e8abcf

Independent review by Rex. NOTE: the build agent's own "Rex" self-review was disregarded (the #494 self-review bug — its guardrail isn't in this fork yet). This verdict was reached by re-running every check from scratch in the worktree. No forged 507-rex.approved marker was found anywhere on disk.

Summary

Wires the framework repo's pre-push-gate.sh to a real check set (4 commands in pre_push.commands) and adds a committed .githooks/pre-push for terminal pushes, both delegating to a shared bin/run-pre-push-checks.sh. Checks: markdownlint, shellcheck, site-counts drift, subpack smoke test. lychee correctly excluded (network-flaky). Also disables MD049/MD060 in .markdownlint.json and documents the core.hooksPath opt-in.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass (ran every check end-to-end)
• Security: Pass (no secrets; tool-missing path fails open by design, acceptable for a local dev convenience gate — CI is the hard backstop)
• Performance: Pass (lychee excluded for latency; checks are fast)
• PR Description & Glossary: Pass (Glossary present, Closes #506, narrative Summary bullets)
• Summary Bullet Narrative: Pass
• Technical Decisions (AgDR): N/A (no new dependency/framework/pattern — wires existing tools)
• Adopter Handbooks: N/A (no migration/domain/TS code in diff; migration-safety handbook not triggered)

Verification performed (ran it, not just read it)

1. Config consumption — jq . valid; pre_push.commands is a [{name, run}] array, exactly the shape pre-push-gate.sh reads via config_get '.pre_push.commands' (cross-checked the hook source). Field names match.
2. Graceful degrade — simulated PATH=/usr/bin:/bin for both the npx and shellcheck commands: each prints an actionable INFO: install message and exits 0 (skip-with-note), no crash. Full runner under stripped PATH also exits 0 and still runs the tool-free checks. AC's graceful-degrade holds.
3. Single source of truth — .githooks/pre-push execs bin/run-pre-push-checks.sh (no duplicated logic; githook can't drift from runner). Exec bit committed (100755). Skip marker <!-- pre-push: skip --> present and identical in both runner and gate; bypass branch verified.
4. End-to-end BLOCK — healthy HEAD: all 4 checks exit 0 (markdownlint 0 errors / 299 files — no false positive). Deliberately broken markdown (SCRATCH_BREAK.md with MD024 dup-heading): .githooks/pre-push BLOCKED (exit 1); the gate's command loop, run against THIS PR's config, BLOCKED (exit 2) catching SCRATCH_BREAK.md:7 MD024. Scratch file reverted; tree clean.
   • One nuance worth recording: when I first fed mock git push JSON to pre-push-gate.sh from inside the worktree, it exited 0. Root cause is NOT a PR defect — _lib-read-config.sh resolves the ops-fork root via resolve_ops_root, which in this multi-fork test layout walks up to the main fork (apexyard/) whose project-config.json has no pre_push.commands. In a normal single-clone deployment the repo root IS the ops fork and the gate reads the 4 commands. Proven by running the gate's exact command loop against the worktree's own config → exit 2 on the broken file.
5. shellcheck — shellcheck --severity=warning .githooks/pre-push bin/run-pre-push-checks.sh → clean (exit 0).
6. Referenced tests — test_site_counts.sh and test_subpack_extraction.sh both exist and pass.

Build agent's 3 self-flagged findings — my disposition

• (a) Runner names only the LAST failing check in the final BLOCKED: summary — confirmed. BUT each failing check still prints its full FAILED: <name> + last-20-lines block to stderr as it runs (lines 78-84), so no failure detail is hidden — only the one-line bottom summary names the last. NIT, non-blocking. Gate still correctly exits 1.
• (b) Command-string duplication between bin/run-pre-push-checks.sh and project-config.json with only a prose "keep in sync" comment, no drift guard — confirmed; the strings are byte-identical today (diffed both). Real maintainability concern but both are correct now and a drift would surface in review/CI. NIT / follow-up, non-blocking. Suggestion: a tiny test asserting the config's markdownlint/shellcheck .run strings equal the runner's *_CMD vars would close the drift window cheaply.
• (c) core.hooksPath docs phrasing — docs are accurate and include the --global caveat (shadows other repos' .git/hooks/). NIT.

Suggestions (non-blocking)

• The .markdownlint.json MD049/MD060 disable is harmless and forward-compatible, but on the locally-pinned engine (markdownlint v0.40.0 via cli2 v0.22.1) neither rule produced any errors — the PR body's "3732 pre-existing errors / pervasive violations" rationale didn't reproduce here. The disable is a reasonable version-defensive guard (CI pins markdownlint-cli2-action@v16); just flagging that the stated error count is engine-version-dependent.
• Consider having pre-push-gate.sh shell out to bin/run-pre-push-checks.sh too (instead of re-reading config and re-implementing the run loop), collapsing the three representations to one. Out of scope for this PR; noting for a follow-up.

Verdict

APPROVED

All acceptance criteria verified by execution: gate does not false-positive on a healthy push, missing tools degrade gracefully without crashing, config is well-formed and consumed correctly, and both push paths block on a real failure. The three self-flagged items are nits, not blockers.

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: ed38a63abc20e856b6fc2096d4d4a0adc1e8abcf