Code Review: PR #305

Commit: edee2f4119f8bcd80a2aa4a93ad098900febb74c

Summary

Adds /codify-rule skill — interactive operator-curated capture for turning human / Copilot / second-pass review comments that caught a Rex-miss into draft handbook entries. Routes to one of four buckets (domain / architecture / general / language) per the existing handbooks/README.md + handbooks/domain/README.md conventions, gates every write on Y/edit/N approval, ships a _Source: PR #N comment by @author on YYYY-MM-DD_ audit footer on every entry, and offers the split-portfolio private layer when portfolio_custom_handbooks_dir resolves outside the ops fork. Stage 2 of the three-stage Rex domain-handbooks plan tracked by #293.

Five files / +851 / -2: SKILL.md (436 lines), 48-assertion smoke test (297 lines), AgDR-0040 (114 lines), CLAUDE.md row + count bump 50→51, docs/multi-project.md skill-behaviour row.

Checklist Results

• Architecture & Design: Pass

• Code Quality: Pass

• Testing: Pass (with two over-pin notes — see below)

• Security: Pass

• Performance: Pass (N/A — interactive skill, no hot path)

• PR Description & Glossary: Pass (5 glossary entries, Closes #296, AgDR-0040 referenced)

• Technical Decisions (AgDR): Pass (AgDR-0040 ships in the same PR, four-section shape verified)

• Adopter Handbooks: Pass (clean-architecture-layers.md and commit-message-quality.md load; neither flagged — SKILL.md is doc-only)

Issues Found

None blocking. Six observations worth surfacing — all advisory.

1. The "Open question" you flagged in point #10 is already resolved by the existing hook — no follow-up needed. require-active-ticket.sh lines 100-110 exempt any path matching *.md from the active-ticket gate (in addition to .claude/*, docs/*, etc.). Since /codify-rule only writes .md files under handbooks/, the gate never fires on its writes. No bootstrap-skills addition needed; the skill works for a reviewer with or without an active ticket marker. Recommend either (a) noting this explicitly in the SKILL.md ("/codify-rule writes .md files only, which satisfies require-active-ticket.sh's *.md exemption — no active-ticket marker required") or (b) just deleting the open question from the PR description so future readers don't re-litigate it. Either is fine.

2. SKILL.md reaches into a private helper API. Line 28 sources _portfolio_root (underscore-prefixed = internal-only convention) directly. The function is exported but the leading underscore signals "framework-internal — public surface is portfolio_registry, portfolio_projects_dir, portfolio_custom_handbooks_dir, etc.". Two cleaner shapes: (a) git rev-parse --show-toplevel + walk up for the .apexyard-fork / legacy anchor (duplicates the lib's logic — not great); (b) add a public portfolio_root() wrapper that just re-exports _portfolio_root. Pragmatic non-blocker — file as a follow-up cleanup if you want.

3. Smoke-test over-pinning #1 — exact phrasing of the Y/N gate prompt. Line 574: assert_grep "yes / edit / no choice" "yes / edit / no" "$skill_md". A harmless rewrite to yes / edit / no?, (y / e / n), or Y / edit / N would break the test without changing skill behaviour. Recommend loosening to yes.*edit.*no (allowing for separator variation) or asserting the three-choice semantics another way.

4. Smoke-test over-pinning #2 — exact _Source: footer literal. Line 721 pins the EXACT footer template _Source: PR #{pr_number} comment by @{comment_author} on {comment_date}_. If anyone reformats (drop the @, swap to **Source**: markdown, add a leading newline), the test breaks. This one is somewhat defensible — the footer IS the load-bearing audit-trail contract — but a slightly looser shape that just asserts "PR #" + "comment by" + "@" + date-placeholder appear in the same line would be more robust to harmless cosmetic edits.

5. Smoke-test contract gap — --blocking flag adds ENFORCEMENT: blocking marker is NOT pinned. Three separate assertions verify advisory default, --blocking flag, and ENFORCEMENT: blocking each appear somewhere in the file, but no assertion pins the relationship: "--blocking flag causes the literal ENFORCEMENT: blocking line to be prepended to the file body." A rewrite that decoupled the flag from the marker (e.g. accidentally renaming the marker to BLOCKING) would still pass the existing assertions. Consider one more assertion that pins the co-location.

6. AgDR option D conflates "no skill" with "manual cp". Honest but a touch unfair to the alternative — the existing handbooks/README.md § "Adding a new handbook" specifically says "No skill scaffolding — handbook authoring is intentionally raw markdown editing in v1. If authoring friction shows up, file a ticket for a /handbook skill." The decision rationale could acknowledge that authoring friction has now shown up (#296 is that ticket — partially) AND that /codify-rule is specifically the capture flow that's distinct from the authoring flow. Mostly stylistic; doesn't change the verdict.

Handbook Findings

Advisory — Clean Architecture Layers — handbooks/architecture/clean-architecture-layers.md

• No findings. The SKILL.md is a documentation file; it doesn't introduce domain/application/infrastructure layering concerns.

Advisory — Commit Message Quality — handbooks/general/commit-message-quality.md

• No findings. PR title feat(#296): /codify-rule — turn human review comments into draft handbook entries reads as a why-not-what summary.

No language handbooks load — the diff is markdown-only, no .ts/.py/.go/.rs files. Migration-safety handbook is N/A — no schema migrations.

Specific contract verifications (per your focus list)

1. PR-resolution inputs (3 paths): SKILL.md step 1 covers full-URL parsing → --pr <N> → current-branch open PR, in that explicit fall-through order. Correct.

2. Operator-approval gate: Step 8 explicitly says "No file is written until the operator explicitly says yes" and the Y/edit/no fork is clearly defined. Re-show + re-ask on edit. Cancel on no. Solid.

3. Re-run on existing slug: Step 10 — Append / Overwrite / Cancel. For append on domain handbooks, frontmatter paths: is merged by union + dedupe (line 377). Sensible default; matches the "compounding" intent of the bucket.

4. Bucket routing — frontmatter rules: Verified against handbooks/README.md ("Flat markdown. No YAML frontmatter required") and handbooks/domain/README.md ("opt-in paths: frontmatter field"). The SKILL.md step 7's three template variants (domain-with-paths / domain-without-paths / architecture-general-language-no-frontmatter) match the conventions exactly. Rule #3 in the Rules section ("paths: frontmatter is domain-only. Other buckets stay frontmatter-free per handbooks/README.md § File format") is correct.

5. Split-portfolio prompt (step 9): Uses portfolio_custom_handbooks_dir; defaults to public when the resolved dir equals <ops_root>/custom-handbooks (i.e. the single-fork default) per the condition "exists AND is different from <ops_root>/custom-handbooks". This is a thoughtful guard — split-portfolio adopters with a real private layer get the choice; single-fork adopters with no private layer never see the prompt. Correct.

6. AgDR-0040 quality: Has all four sections (Context, Options Considered, Decision, Consequences). "In the context of … facing … I decided … to achieve … accepting …" opener present and well-formed. Options A/B/C/D engage honestly: A (operator-curated, chosen) vs B (auto-promote on hook — rejected for noise) vs C (hybrid queue — rejected for v1 simplicity) vs D (no skill, manual cp — rejected for friction + missing source-attribution). No YAML frontmatter — consistent with AgDRs 0034/0035/0037/0038/0039.

7. Source footer: _Source: PR #N comment by @author on YYYY-MM-DD_ + _See: <comment-url>_. Captured from pr_owner_repo, pr_number, comment_author, comment_date, comment_url. When the comment isn't fetchable via gh api (freeform paste), the URL degrades to the PR URL — acknowledged in step 1. Audit trail is complete.

8. CLAUDE.md row placement: Inserted between /spike-close and /investigation. That's a reasonable position — /spike-close is also a "post-PR / disposition / close-the-loop" skill. Count bump 50→51 correct in both the layer table ("50 slash commands" → "51 slash commands") and the section header ("### Available skills (50)" → "### Available skills (51)"). docs/multi-project.md row sits in the architecture-doc family between /pdf and the legacy-skills-not-portfolio-aware paragraph; matches the surrounding row density (long, dense, includes invocation shape + key flag + AgDR ref).

9. Glossary: 5 entries in the PR body — Rex-miss, Steering loop, Capture, paths: frontmatter, Custom-handbooks layer. All load-bearing; none are filler.

10. Closes #296: Present at the end of the PR body. Bare #N notation — per .claude/rules/ticket-vocabulary.md § "Fork → upstream PRs: bare #N is the right notation", correct for fork→upstream auto-close to fire.

Suggestions

• Add a single line in SKILL.md noting that *.md writes satisfy the active-ticket gate (closes your point #10 in the agent prompt).

• Consider adding one more smoke assertion that pins --blocking ↔ ENFORCEMENT: blocking co-location, not just both-words-appear-somewhere.

• If the underscore-prefixed _portfolio_root reach bothers you in code review, file a 1-line follow-up to add a public portfolio_root() wrapper to _lib-portfolio-paths.sh. Not a blocker.

Verdict

APPROVED

The skill closes the loop on the Stage 2 ticket (#296) cleanly. The capture flow is well-shaped (resolution → comment → file:line → bucket routing → operator gate → write), the audit-trail footer is built in by construction, the four-bucket routing honours the existing handbook conventions correctly, and the AgDR engages honestly with the obvious alternatives (auto-promote, hybrid queue, no-skill). Smoke test has two minor over-pin spots and one missing contract assertion — all easy follow-ups, none blocking.

This is the steering-loop closer for the harness. The UX is right: every entry is operator-approved before any file is written, source attribution is automatic, re-runs handle the compounding case. The handbook layer can now grow on Rex's actual misses rather than only the rules an operator thought to write proactively.

Reviewed by Rex (Code Reviewer Agent)

Reviewed commit: edee2f4119f8bcd80a2aa4a93ad098900febb74c

Code Review: PR #305 (fix-up re-review) — APPROVED

Commit: 6e128892bf328af261b9b92a540cddfb715fce6c
Prior approval: edee2f4119f8bcd80a2aa4a93ad098900febb74c (3 non-blocking nits)

Note: --approve blocked (self-PR); posting as --comment. Verdict is APPROVED.

Scope of this re-review

Fix-up commit only — +30 / -4 to .claude/skills/codify-rule/tests/smoke.sh. Did not re-audit the contracts already verified at edee2f4.

Focus 1 — loosened footer assertion

The three sub-assertions correctly catch removal of any one ingredient from the spec entirely:

• Source.*pr_number|Source.*PR #\{ — matches the template literal at line 279
• comment_author — matches at line 279
• comment_date — matches at line 279

Trade-off noted, not blocking: the three independent assert_grep calls don't verify the ingredients appear in the same template line — a future spec that scatters them across multiple lines or describes them in prose only would still pass. comment_author and comment_date also appear in prose on lines 18, 850, 893, so an ingredient could be removed from the template literal while still passing the assertion if the string survives in surrounding prose. This is the deliberate cost of loosening to allow harmless template-shape changes, which is exactly what was asked for. The "ingredient removed entirely from the spec" failure mode is still caught — that's the load-bearing contract.

Focus 2 — 3-line co-location threshold

Not brittle for the current spec. Two co-located pairs exist in SKILL.md:

• Lines 303 ↔ 304: --blocking and ENFORCEMENT: blocking are 1 line apart
• Line 409: both phrases on the same line (diff = 0)

The algorithm checks all pairs and accepts on first match (break 2), so as long as at least one co-located instance exists, the test passes. A future rewrite would have to scatter all instances past 3 lines for the test to fail — unlikely without intent. Threshold is well-tuned.

Minor nit (non-blocking): the assertion is silently skipped if grep -nE returns empty (the outer if has no else FAIL). Consistent with the file's defensive style — section 5 above already pins that both phrases exist — so the gap is covered transitively.

Focus 3 — No regression

Single file changed in the fix-up commit: .claude/skills/codify-rule/tests/smoke.sh. No other files touched. The PR body says +35 / -4; the raw diff is +30 / -4 — the delta is whitespace/header lines, substance matches.

Checklist Results

• Architecture & Design: Pass (test-only change)
• Code Quality: Pass
• Testing: Pass — test infra is the artifact being modified
• Security: Pass (N/A)
• Performance: Pass (N/A)
• PR Description & Glossary: Pass (verified at edee2f4)
• Technical Decisions (AgDR):Pass — N/A for fix-up; AgDR-0040 covers the skill
• Adopter Handbooks: N/A

Issues Found

None.

Suggestions

• (carried from prior review, unchanged) SKILL.md still sources _portfolio_root — underscore-prefixed internal API. Pragmatic non-blocker; treat as optional follow-up.
• (new nit) /tmp/codify_enforce_lines.txt and /tmp/codify_flag_lines.txt are not race-safe. If the test ever runs in parallel, the temp files collide. Add $$ to the filename if that ever becomes a concern.

Verdict

APPROVED

The fix-up commit cleanly addresses both nits and adds a meaningful new contract (co-location pin) that closes the failure mode I flagged in the prior review.

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: 6e128892bf328af261b9b92a540cddfb715fce6c