Code Review: PR #301 — CHANGES REQUESTED (delivered as comment; can't request-changes on own PR)

Commit: b2eb64ce0506a70cd5a4690e77ad394238dc6910

Summary

Standardises blocking-hook error messages on a canonical shape (BLOCKED → context → "To unblock:" numbered list → optional escape hatch) and ships AgDR-0039 plus 5 retrofits. The design is good — the contract is clear, the canonical-phrase choice is well-argued, and the option table genuinely engages B/C/D. But the PR is creating the standard by example, so the retrofits need to be exemplary, and several aren't quite there yet.

Checklist Results

• Architecture & Design: Pass — no behavioural changes, stderr-text only
• Code Quality: Fail — see Issues #1 and #2
• Testing: Pass — text-content not asserted by existing tests
• Security: Pass — no new secret-exposure surface
• Performance: N/A
• PR Description & Glossary: Pass — 5 glossary entries, ticket linkage Closes #295, AgDR slug referenced
• Technical Decisions (AgDR):Pass — AgDR-0039 ships with this PR
• Adopter Handbooks: Pass — handbooks/general/commit-message-quality.md (advisory) is satisfied; the single commit (b2eb64c) has a strong subject + multi-paragraph WHY-shaped body

Issues Found

1. ⛔ Word-break defect in check-secrets.sh rendered output

.claude/hooks/check-secrets.sh:49-50:

Pattern source: .claude/hooks/check-secrets.sh (the regex is intentional
ally noisy on the false-positive side — secret leaks are worse than

The word "intentionally" is broken across lines as intentional + ally. The wrap landed mid-word. Every agent and operator who triggers this hook will see broken English in the help text. Especially bad given this PR defines the standard for hook error-message quality. Either re-wrap the paragraph or hyphenate at a syllable boundary (inten-tionally).

2. ⛔ Incorrect override guidance in block-main-push.sh (push case)

.claude/hooks/block-main-push.sh:46-49:

Override (rare — only if your fork's branch model genuinely treats
this branch as a daily-work trunk): add the branch name to
.claude/project-config.json → .git.protected_branches[] to override
the default protection list.

This is backwards. Looking at the config resolution at line 24:

PROTECTED=$(config_get '.git.protected_branches[]' 2>/dev/null | paste -sd'|' -)

The config-block-set list replaces the default. To unblock a push to dev, the operator needs to set .git.protected_branches to a list that excludes dev (e.g. ["main", "master"]) — they should remove the branch from the list, not add it. The current wording would lead any operator who follows the guidance to make the branch more protected, not less. Reword along the lines of: "set .git.protected_branches to a list that excludes this branch (the config replaces the default list rather than appending to it)".

This is the worst possible class of defect for a "concrete next-action" contract — the action is concrete but it actively misdirects.

3. ⛔ Hardcoded dev integration branch in validate-branch-name.sh step 3

.claude/hooks/validate-branch-name.sh:133-134:

     OR start fresh from the integration branch:
       git checkout dev && git checkout -b "feature/GH-XX-your-description"

The hook ships to every adopter including managed projects, which per CLAUDE.md § "Branch model — framework only" stay trunk-based on main (only the framework repo uses dev/main). The example will misdirect operators of managed projects. Suggest one of:

• Read the actual integration branch from .claude/project-config.json → .git.default_base_branch (if such a key exists / can be added)
• Use a placeholder: git checkout <your-integration-branch> and append a one-liner naming the most common choices

Handbook Findings

None. handbooks/general/commit-message-quality.md (advisory) is satisfied — the single commit has a strong WHY-shaped body that explains the failure mode, the Böckeler reference, the canonical-phrase decision, and what's scoped for follow-up.

Suggestions

S1. Shape consistency — block-main-push.sh commit case has a second numbered list

.claude/hooks/block-main-push.sh:71-76 introduces a second numbered list (1–2) for the accidental-commit-recovery path after the primary To unblock: list ends at step 3. The AgDR template (lines 64–69) reserves the numbered-list shape for the canonical To unblock: block and treats the escape hatch as prose. Two numbered lists in one block message dilutes the convention. Options:

• Merge the recovery into the main To unblock: list with conditional phrasing (3. Retry — OR, if you've already committed, see recovery below) and keep the recovery as prose, OR
• Use a second heading (e.g. If you've already committed locally: followed by an unnumbered list)

Either way, the rendered output should have one To unblock: numbered list per block per the AgDR's own contract.

S2. AgDR placeholder needs replacement

docs/agdr/AgDR-0039-hook-self-correction-shape.md:101:

- PR me2resh/apexyard#XXX — the shape definition + 5 hook retrofits (this work)

Replace #XXX with #301 before merge so the Artifacts section is self-referential. Minor.

S3. AgDR claims "17 blocking hooks" — actual count is ~21

grep -l "exit 2" .claude/hooks/*.sh | wc -l → 21. Not all exit 2s are "BLOCKED:"-style blocks (some are arg-parse errors), but the headline number in the AgDR is slightly off. Either reconcile to the precise count of BLOCKED-emitting hooks or soften to "the framework's blocking hooks" without the number.

Verdict

CHANGES REQUESTED

Issues #1 and #2 are blocking — broken English in the rendered help text and reversed-direction override guidance both violate the AgDR's own "concrete next-action" contract. Issue #3 is the same class (concrete-but-wrong) for a smaller surface (only affects managed-project operators). The AgDR itself is solid; the retrofits need to match its quality bar before this lands as the canonical reference example.

Once #1, #2, #3 are fixed (and ideally S1, S2), this is ready. The shape choice is right and the long-term win is real — landing it well matters more than landing it fast.

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

Code Review: PR #301 (re-review)

Commit: 5152b68653ae9101b87c5edf3db3560daf8c6f94

Summary

Re-review at the new HEAD after b2eb64c → 5152b68. The prior review at b2eb64c was REQUEST CHANGES with 3 blocking findings + 2 nits; the fix-commit addresses all five mechanically. Result: clean APPROVE (recording as comment because GitHub disallows self-approval).

Checklist Results

• Architecture & Design: N/A (shell-script hook changes, no domain/application/infrastructure layering)
• Code Quality: Pass
• Testing: Pass (text-only change; author states existing tests assert exit codes, unchanged)
• Security: N/A (no new attack surface; secret regex unchanged)
• Performance: N/A
• PR Description & Glossary: Pass
• Technical Decisions (AgDR): Pass — AgDR-0039 documents the shape decision, options A–D, canonical-phrase rationale
• Adopter Handbooks: N/A (no relevant findings — only architecture/migration-safety.md is blocking, and the diff has no migration files)

Verification of the 5 fixes

Previously-approved aspects (regression check)

• Heredoc terminators (MSG unquoted, left-justified on own line) — intact in all 5 retrofits ✓
• Variable interpolation ($CURRENT_BRANCH, ${PROTECTED//|/, }, $SECRETS_FOUND, ${TYPES//|/, }, ${INNER_PATTERN}) — all inside unquoted MSG heredocs, expansion preserved ✓
• Shape consistency (BLOCKED: line → context → single To unblock: numbered list → optional escape hatch / customisation) — all 5 conform ✓
• AgDR-0039 quality (options A–D table, contract per section, canonical-phrase rationale for To unblock: over To proceed: / To self-correct:) — intact ✓

Issues Found

None.

Suggestions

(Non-blocking, optional follow-up)

• Sub-nuance on the "Customise" block in block-main-push.sh: a pedantic operator could write protected_branches: [] (literal empty array) expecting "disable all protection" and instead hit the empty-falls-back-to-default behaviour at line 26. The PR text doesn't claim [] disables protection — it says "write the array with that branch OMITTED" — so this isn't a misdirection, just a pre-existing config-resolver edge case not surfaced. Worth a one-liner in a follow-up if you want to be exhaustive, but not blocking on this PR.
• Sub-nuance on the commit-case rescue: the prose "(NOT a separate To-unblock — the gate is still the one above)" is correct but slightly meta. A future canonical reader of AgDR-0039 won't have the conversational context of "why is there prose where there could be a list" — the inline reminder is a useful breadcrumb but reads as if it's defending against a critique that exists outside the file. Optional polish: replace with something narrative like "If you've already committed and need to recover, here's the three-command rescue:". Functionally identical; less defensive.
• AgDR-0039 reference list: the second Böckeler reference is dated 2026-05-19. Confirm that's the actual publication date of the cited piece; if back-dated to match this PR's commit date that's fine but worth a note. Not blocking.

Verdict

APPROVED (recorded as comment per GitHub's self-approval restriction)

The 5 fixes are mechanical, correct, and don't regress any previously-approved aspect. This PR is now the canonical example for AgDR-0039's shape — future hook authors can read these 5 retrofitted hooks (check-secrets.sh, block-git-add-all.sh, block-main-push.sh, validate-branch-name.sh, require-active-ticket.sh phrasing tweak) as the template.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 5152b68653ae9101b87c5edf3db3560daf8c6f94

Code Review: PR #301 (re-review at new HEAD)

Commit: 82284f5284d8cc96b9567f2445466c94fe8ff4b8
Previously approved: 5152b68 — invalidated by the two follow-up commits below.

Submitted as a comment because GitHub doesn't allow approving one's own PR. Rex-side verdict is APPROVED; the approval marker covers the merge-gate side.

Summary

CI-fix-only delta on top of the previously-approved 5152b68. Two surgical changes to clear the markdownlint MD025 + lychee timeout failures from the prior run. No functional change to the shape definition or the 5 retrofitted hooks.

Delta vs 5152b68

git diff 5152b686 82284f52 — exactly 2 files, 6 additions, 7 deletions:

Checklist Results

• Architecture & Design: Pass — surgical CI fix, no design surface touched
• Code Quality: Pass — both changes follow established conventions
• Testing: N/A — config / docs only
• Security: Pass — no new code paths, no auth/crypto/secrets
• Performance: N/A
• PR Description & Glossary: Pass — unchanged from the approved version
• Technical Decisions (AgDR): Pass — AgDR-0039 itself is the technical decision, present and linked
• Adopter Handbooks: N/A — no handbooks loaded (no architecture/general/typescript-extension diff)

Issues Found

None.

Verification against the brief

1. AgDR-0039 single H1, no frontmatter — confirmed. Line 1 = # Standardised self-correction guidance shape for blocking hooks. Matches AgDR-0034 / 0035 / 0037 / (post-revert) 0038 sibling convention. MD025 satisfied (grep -cE "^# [^#]" returns 1).
2. .lycheeignore format mirrors the medium.com precedent — confirmed. 3-line comment block explaining the failure mode (per-IP rate limiting trips lychee's burst, works in browsers / cold IPs) + names the citing AgDRs (0037 and 0039). Regex is properly anchored to /articles/exploring-gen-ai/ so it won't over-suppress the broader martinfowler.com domain.
3. No unintended changes — confirmed. Diff against 5152b68 shows only the two files above; nothing else touched.
4. CI green — confirmed via gh pr view 301 --json statusCheckRollup. All four checks (lychee, markdownlint-cli2, Verify Ticket ID, shellcheck .claude/hooks) report SUCCESS on 82284f5.

Notes on the .lycheeignore expansion

The regex https://martinfowler\.com/articles/exploring-gen-ai/.* is correctly scoped — it suppresses only the exploring-gen-ai article series Böckeler authors there, not the entire martinfowler.com domain. If a future AgDR cites a different Fowler article (e.g. one of the canonical refactoring or architecture pieces), that link will still be checked normally. Tight scope, low false-negative surface. The comment explicitly names the failure mode (per-IP rate limiting) and the citing AgDRs (0037 and 0039), so a future reader debugging "why is this Martin Fowler URL silently passing CI?" has the full context in-file.

Verdict

APPROVED — for the new HEAD 82284f5.

The CI-fix-only nature of this delta + the surgical scope + the in-tree precedent for both fixes (AgDR-0034/0035/0037 for the frontmatter convention, the medium.com line for the lychee-ignore shape) means there's nothing new to evaluate beyond confirming the brief's promises. All four promises check out.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 82284f5284d8cc96b9567f2445466c94fe8ff4b8