Code Review: PR #306

Commit: 0d6a1a5173f381980a1e7054389de92f9578d47e

Summary

Sweeps all 40 bash -c hook wrappers in .claude/settings.json to the canonical v2-anchor-aware shape (.apexyard-fork OR onboarding.yaml, plus the -n "$r" infinite-loop guard); refactors link-custom-skills.sh to source _lib-ops-root.sh with a strict-v1 graceful-degradation fallback; documents the canonical wrapper + hook-internal shapes in AgDR-0041 and cross-links from docs/multi-project.md.

The substance of the refactor is solid — the AgDR is honest, the wrapper shape is correct, the docs match the new contract. There is one blocking issue: the markdownlint CI check is RED, caused by a single MD018 violation in the new AgDR. That has to be fixed before merge per .claude/rules/pr-quality.md § "No Red CI Before Merge".

Checklist Results

• Architecture & Design: Pass — wrapper logic is correct under all three layouts (v2-only, v1-only, no-anchor)
• Code Quality: Pass — mechanical sweep is consistent across all 40 wrappers
• Testing: Pass with note — see Suggestions
• Security: Pass — no secrets, no privilege changes
• Performance: Pass — one extra -f check per ancestor; negligible
• PR Description & Glossary: Pass — 4 glossary entries, Closes #302 present, AgDR referenced by full slug
• Technical Decisions (AgDR):Pass — AgDR-0041 is created and engages with alternatives
• Adopter Handbooks: N/A — diff is shell + JSON wrappers; clean-architecture-layers.md doesn't apply

Issues Found

⛔ BLOCKER — Red CI (markdownlint MD018)

The new docs/agdr/AgDR-0041-sessionstart-v2-anchor-sweep.md:13:1 fails markdownlint with:

MD018/no-missing-space-atx  No space after hash on atx style heading
Context: "#242 (split-portfolio v2) move..."

Line 13 of the AgDR is the second paragraph of ## Context and starts with the literal text #242 (split-portfolio v2) moved …. markdownlint reads #242 at the start of a line as a malformed ATX heading (hash with no space after).

Per .claude/rules/pr-quality.md § "No Red CI Before Merge": "Never merge with red CI — even if the failure is pre-existing or unrelated." This one isn't pre-existing — it ships in the new file.

Fix: rewrite the line so #242 is not at column 1. Two cheap options:

1. Prepend PR or Issue so the line reads PR #242 (split-portfolio v2) moved ….
2. Use the cross-repo form me2resh/apexyard#242 (split-portfolio v2) moved …. This matches the linking style used elsewhere in the AgDR (the Artifacts section already does this).

Either is a one-character/one-word change. After the fix, re-push and CI should go green.

(The same pattern in the Artifacts section — me2resh/apexyard#302 — is fine because the m comes first.)

Audit of the load-bearing claims

The review request asked me to be thorough on (a) shape correctness across all 40 wrappers, (b) AgDR honesty about the options, and (c) docs accuracy. Findings:

(a) Shape correctness — all 40 wrappers swept correctly.

• I verified 40 bash -c entries in .claude/settings.json and all 40 contain .apexyard-fork, onboarding.yaml, AND the [ -n "$r" ] guard. Zero drift.
• Section distribution: 7 SessionStart, 3 PreToolUse/Edit-Write-MultiEdit, 27 PreToolUse/Bash, 1 UserPromptSubmit, 2 PostToolUse. That sums to 40 — matches the headline claim, though the PR body says "29 PreToolUse" (it's 30: 3 + 27). Minor counting nit in prose; the bulk count is right.
• The check-jq-installed.sh SessionStart entry that landed on origin/dev mid-flight is included (line 17 of the new settings.json).
• I executed the new shape against cwd=/tmp (no anchors anywhere up the tree): the loop walks once, reduces r="/tmp" to r="", the -n "$r" guard fires, loop exits cleanly with r=""; exec "/.claude/hooks/<name>.sh" then fails fast with exit 126. Visible failure, no infinite loop. Then ran the OLD legacy shape (no -n "$r" guard, only [ "$r" != / ]) against the same cwd=/tmp: it infinite-loops as predicted. The AgDR's Consequence #3 is grounded in observed behaviour, not theory.

(b) AgDR honesty about options.

• Option A (source the lib from the wrapper): I confirmed bash -c 'echo "0=[$0] dirname0=[$(dirname "$0")]"' prints 0=[bash] dirname0=[.]. The $0=bash claim is technically accurate. Option A's rejection is not just convenient framing — it's a hard property of bash -c. Honest.
• Option B (inline the v2-aware walk-up): chosen. The relaxation from BOTH-of-pair (lib) to either-marker (wrapper) is justified because the wrapper's only job is to locate the hooks dir; the hook itself re-resolves via the lib if it needs strict ops-root semantics. I audited the two SessionStart hooks that DON'T source _lib-ops-root.sh (onboarding-check.sh, check-upstream-drift.sh) and both use git rev-parse --show-toplevel instead of relying on the wrapper's $r. So the laxness has no semantic effect there either. The reasoning holds for every wrapper in the sweep.
• Option D (drop the wrapper walk, exec from $PWD): correctly rejected — breaks when working in workspace/<project>/.
• Gap: the user's review request mentions a different "Option C" — eliminate the wrapper walk via a Claude Code runtime change. The PR body covers it in "Out of scope" ("would need an upstream change to the Claude Code runtime, not a framework change"), but the AgDR's Options table only lists a different Option C (single dispatch script). Worth adding a short row to the AgDR Options table for the upstream-runtime option, even just to record it as deferred — it's the most principled long-term path and an honest options table should name it. Non-blocking nit.

(c) Docs accuracy.

• docs/multi-project.md § "Where session-state files live" now correctly describes the dual-anchor model and removes the wrong "BOTH … AND …" copy that was specific to v1. The "BLOCKED: PR has no recorded code-reviewer approval" troubleshooting line is updated to "at least one of the ops-fork anchors". Accurate.
• One minor precision issue: the new third paragraph says "The same dual-anchor rule applies to the .claude/settings.json hook wrappers themselves". The wrappers use the WEAKER v1 condition (single onboarding.yaml), not the strict pair — the rule is .apexyard-fork OR onboarding.yaml at wrapper level, vs .apexyard-fork OR (onboarding.yaml AND apexyard.projects.yaml) at lib level. Calling them both "the dual-anchor rule" elides that asymmetry. Minor wording suggestion; not blocking.

Handbook Findings

handbooks/architecture/clean-architecture-layers.md (always-load) — diff is shell + JSON wrappers + an AgDR + docs. No domain / application / infrastructure boundary in scope. No findings.

Suggestions (non-blocking)

1. Test coverage gap on link-custom-skills.sh lib-sourcing path. The 6 test cases in test_link_custom_skills.sh build a sandbox that copies in _lib-portfolio-paths.sh and _lib-read-config.sh but not _lib-ops-root.sh. So all 6 cases exercise the graceful-degradation fallback (lines 62–71 of the hook), not the new primary path (lines 57–60, if [ -f "$HOOK_DIR/_lib-ops-root.sh" ]; then . _lib-ops-root.sh; ops_root=$(resolve_ops_root "$PWD") …). The fallback works (the test fixture writes BOTH anchors, so the strict-v1 fallback condition is satisfied), but the actual code path that ships in production isn't pinned by these tests. Two cheap fixes:

   • Copy _lib-ops-root.sh into the sandbox in make_fork() so the helper-sourced path is the one exercised.
   • Add a 7th case that creates a v2-only fork (.apexyard-fork only, no onboarding.yaml) and asserts the hook resolves correctly through the lib.
     The bug this would catch is the helper-sourced path silently returning the wrong dir for a v2-only layout — exactly the failure class this PR is meant to prevent.

2. AgDR Options table: add the upstream-runtime row. As noted under audit (b), the most principled solution is an upstream change to Claude Code that resolves hook paths relative to the fork root. It's acknowledged in the PR body's "Out of scope" but missing from the AgDR Options table. Adding a row ("Option E — push the walk into the Claude Code runtime — Pros: removes all wrappers; Cons: needs upstream cooperation, not a framework-side change. Defer.") would make the Options table feel complete and honest.

3. Docs precision on "dual-anchor rule" asymmetry. The wrapper's v1 acceptance condition (single onboarding.yaml) is a deliberate relaxation from the lib's stricter v1 pair. Worth one sentence in docs/multi-project.md to note that wrappers and the lib accept slightly different shapes, and why (wrapper only needs to find the hooks dir; lib establishes canonical ops-root). Non-blocking.

4. PR body counting nit. "29 PreToolUse" → "30 PreToolUse" (3 Edit-matcher + 27 Bash-matcher = 30). Cosmetic; the headline total of 40 is correct.

Verdict

CHANGES REQUESTED

The substance of the refactor is good and the architectural reasoning in AgDR-0041 holds up to scrutiny. Blocking only on the red CI from the markdownlint MD018 violation in the new AgDR file (one-line fix). Once that's green, I'm happy to re-review and approve.

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

Code Review (re-review): PR #306

Commit: 823b2fc1f63de255f50d334f8f007b18b3e8fd96

Summary

Re-review of the fix-up commit 823b2fc addressing the blocker + 3 non-blocking nits from the previous review at 0d6a1a5. (Cannot --approve own PR; using --comment with explicit APPROVED verdict.)

Checklist Results

• ✅ Architecture & Design: Pass (unchanged from prior review)
• ✅ Code Quality: Pass
• ✅ Testing: Pass — now 7/7 cases, both lib-sourcing branches pinned
• ✅ Security: N/A
• ✅ Performance: N/A
• ✅ PR Description & Glossary: Pass (unchanged from prior review)
• ✅ Technical Decisions (AgDR):Pass — AgDR-0041 now strengthened with Option E
• ✅ Adopter Handbooks: N/A

Fix-up verification

1. MD018 blocker (red CI on prior HEAD) — RESOLVED.

docs/agdr/AgDR-0041-sessionstart-v2-anchor-sweep.md line 13 now reads PR #242 (split-portfolio v2) moved … instead of bare #242 …. CI markdownlint-cli2 SUCCESS on 823b2fc. All five CI checks green (lychee, markdownlint-cli2, Verify Ticket ID, shellcheck).

2. Option E (nit 1: honesty gap) — RESOLVED genuinely.

The new row engages with the runtime-resolution alternative, not lip-service:

• Description names the actual mechanism: "runtime resolves hook paths from the ops-fork root, not from $PWD via bash -c"
• Pros are honest: "Cleanest — no inline walk in any wrapper, no dispatch hop, no lib-sourcing dance. The runtime already knows where the project root is."
• Cons name the right blocker: "Needs an upstream Claude Code change (settings.json semantics + how hook commands are resolved). Out of this PR's scope."
• Defer rationale closes the loop: "Option B is the right shape until the runtime grows that ability."

This is the shape an AgDR-level options-considered row should take — names the cleaner path, names what's blocking it, names when to revisit.

3. Test case 7 (nit 2: fallback path coverage) — RESOLVED, test passes for the right reason.

Verified at HEAD 823b2fc (local worktree run):

PASS: case 1-6 ... (primary lib-sourcing path)
PASS: case 7: graceful-degradation fallback works when _lib-ops-root.sh is absent
Passed: 7  Failed: 0

The case 7 setup is correctly designed:

• make_fork --no-ops-root-lib sets copy_ops_root=0 so the cp "$LIB_OPS_ROOT_SRC" … line is skipped
• Pre-flight assertion [ ! -f "$SB/.claude/hooks/_lib-ops-root.sh" ] exits 1 with an explicit FAIL message if the lib were unexpectedly present (false-positive setup guard)
• Symlink check + "linked 1 custom skill" output check confirms the inline fallback walk-up successfully resolved the ops root via .apexyard-fork and the rest of the hook ran end-to-end

Cases 1-6 now genuinely exercise the lib-sourcing branch (primary path) since make_fork copies _lib-ops-root.sh by default. Together with case 7, both branches of the if [ -f "$HOOK_DIR/_lib-ops-root.sh" ] shape are pinned by tests.

The reviewer-flagged risk ("passes for the wrong reason — some other walk-up resolves the path") doesn't apply: the fallback inline walk-up is the only path that can resolve the ops root when the lib is absent.

4. Doc precision (nit 3) — RESOLVED.

The added callout in docs/multi-project.md § "Where session-state files live" correctly states:

The wrappers accept onboarding.yaml alone as the v1 anchor, while _lib-ops-root.sh (the in-hook resolver) requires BOTH onboarding.yaml AND apexyard.projects.yaml.

Reading the surrounding prose end-to-end — dual-anchor paragraph → wrappers paragraph → laxness callout → "at least one of the ops-fork anchors" recovery instructions — the flow is coherent. The laxness is explained before the recovery instructions mention single-anchor sufficiency, so a reader doesn't trip on "wait, I thought you needed both?". Cross-reference to AgDR-0041 § Decision point 2 anchors the rationale for the future-curious reader.

Issues Found

None.

Suggestions

None — all four points cleanly addressed, CI green, tests strengthened.

Verdict

APPROVED

The fix-up commit cleanly resolves the blocker and substantively addresses all three nits — Option E is honest engagement, case 7 is a real fallback test (not just a "test exists" line item), and the doc callout makes the wrapper-vs-lib asymmetry visible to future-us. Solid response to review feedback; ready to merge once CEO approves.

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