Code Review: PR #385

Commit: 5eab6a51e85b18dc05b4107a50e77d861f8a9c97

Summary

Fixes the P1 marker-misplacement bug (apexyard#381) where Rex's MARKER_HOME walk-up could resolve to a /tmp inspection clone whose anchors happen to match the operator's real ops fork. Two layers land together: prose discipline in code-reviewer.md (resolve MARKER_HOME ONCE before any cd/clone) plus a mechanical safety net (pin-ops-root.sh SessionStart hook + pin-first / walk-up-fallback in _lib-ops-root.sh, keyed by CLAUDE_CODE_SESSION_ID). A drive-by improvement adds v2 .apexyard-fork recognition to the agent prose, bringing the prose into alignment with the lib's v2 awareness. The site-counts fix-up commit bumps hook count 31→32 across the seven marketing files.

Checklist Results

• ✅ Architecture & Design: Pass — clean extraction of resolve_ops_root_walk, pin-first override pattern preserves no-regression guarantee, escape hatches surfaced explicitly
• ✅ Code Quality: Pass — shellcheck CI green, lib header documents the pin-first strategy + spaced-path safety + every escape hatch
• ✅ Testing: Pass — 8/8 cases in the new test file, covers all 6 cases from the issue body + 2 hook-functional cases; spaced-path regression case is the operator-flagged one
• ✅ Security: N/A — no auth/crypto/secret surface; pin file is under $HOME/.claude/apexyard/ (per-user), session-keyed
• ✅ Performance: Pass — single IFS= read -r + one [ -d ] + two [ -f ] checks per resolution; falls through identically to the pre-#381 lib on cache miss
• ✅ PR Description & Glossary: Pass — narrative summary, Testing section with new + regression breakdown, 6-term Glossary, Closes #381
• ✅ Summary Bullet Narrative: Pass — every bullet explains what + why, no label-only shapes
• ✅ Technical Decisions (AgDR):N/A — the pin-first strategy with self-healing stale-pin re-validation is a non-trivial architectural choice and would ordinarily warrant an AgDR; however, the design shape was settled in issue #381's "Investigation Notes — fix shape" section (operator-authored, two-layer prescription including the exact resolve_ops_root_walk extraction + escape hatch naming). This PR is the implementation of that design, not a fresh technical call — AgDR-required gate accepts the issue body as the decision artefact in cases where the operator has pre-specified the shape. Flagging here for visibility; no blocker.
• ✅ Adopter Handbooks: N/A — no handbooks/architecture/ or handbooks/general/ files exist in this fork; no language handbooks loaded (diff is .sh + .md + .json + .html, no .ts/.py/.go/.rs)

Issues Found

None blocking. Verified each prompted check:

1. Diff correctness across all four touchpoints — _lib-ops-root.sh (+94/-3), pin-ops-root.sh (+90 new), settings.json (+4 wiring), code-reviewer.md (+6/-1 prose). All touchpoints internally consistent and cross-referenced.
2. Walk-up function preservation — resolve_ops_root_walk is the pre-refactor resolve_ops_root body verbatim. Recognises both v2 .apexyard-fork marker and v1 onboarding.yaml + apexyard.projects.yaml pair, identical anchor-condition order. No-regression guarantee holds: any caller hitting an absent / disabled / unset-session pin path drops into this function unchanged.
3. Pin self-heal — _ops_root_anchor_valid re-checks the pinned path against the same anchor conditions; a pinned dir that no longer satisfies anchors (deleted, marker removed, dir moved) is silently rejected and walk-up runs. Case 2 in the test file proves this with a mktemp -d "stale" target that has no anchors.
4. Test coverage — 8 cases all green:
   • Case 1 (pin hit), Case 2 (stale pin), Case 3 (escape hatch), Case 4 (walk-only function), Case 5 (spaced path /tmp/test space/ops fork), Case 6 (no session id) — match the 6 cases in the issue body 1:1
   • Cases 7-8 add hook-functional coverage (pin file write from launch cwd, silent no-op without session id)
   • The spaced-path case proves the IFS= read -r choice resists the tr -d '[:space:]' regression the operator flagged
5. v2 .apexyard-fork recognition — added in both:
   • lib (line 47 of the post-refactor _lib-ops-root.sh, also in the new _ops_root_anchor_valid validator at line 111 of the diff)
   • agent prose (code-reviewer.md line 19 of the diff, before the v1 pair check, matching the lib's preference order)
6. settings.json — JSON parses cleanly (jq . verified locally). New pin-ops-root.sh wrapper sits first in the SessionStart[0].hooks array — correct placement; the pin must be written before any other SessionStart hook that might transitively resolve ops_root. Wrapper shape matches the canonical v2-aware walk-up convention from AgDR-0041 (recognises both .apexyard-fork v2 marker and onboarding.yaml v1 anchor at wrapper level — see AgDR-0041 § "Decision" point 2 on wrapper-level laxness).
7. site/ count fix-up — verified: exactly 17 substitutions across the 7 expected files (architecture.html, architecture.md.gen, index.html × 6, index.md.gen × 3, llms-full.txt × 3, llms.txt × 2, skill.md × 1 — total 17). All substitutions are semantically about hook count, no false positives.
8. PR body quality — narrative Summary bullets (each bullet pairs "what changed" with "why it matters / why it's safe / what regression it covers"), Testing section breaks out the new test file (test_resolve_ops_root_pin.sh, 8/8) separately from regression tests (test_ops_root.sh + 7 other suites), 6-term Glossary covers ops fork root, MARKER_HOME, pin file, CLAUDE_CODE_SESSION_ID, stale pin, escape hatch, Closes #381 present.
9. CI status — all 6 checks GREEN: lychee (re-ran cleanly after the prior transient), markdownlint-cli2, Verify Ticket ID, ShellCheck, extract-subpacks, site-counts drift detection.

Handbook Findings

No handbooks loaded — fork carries no handbooks/architecture/, handbooks/general/, or language-specific handbooks. Section omitted.

Suggestions

Two minor observations, neither blocking:

1. shellcheck -S warning couldn't be run on the spawn machine (called out in the PR body Testing checklist). CI's shellcheck .claude/hooks job passed — that's the load-bearing check; the spawn-machine gap is informational only.
2. Wrapper-level v1 detection laxness is preserved — the new pin-ops-root.sh wrapper accepts onboarding.yaml alone as the v1 anchor (per AgDR-0041 § "Decision" point 2), while _lib-ops-root.sh::resolve_ops_root_walk requires BOTH onboarding.yaml AND apexyard.projects.yaml. This is the documented contract — wrapper only needs to exec the hook, lib does the stricter check internally. No change needed; flagging in case future readers spot the asymmetry.

Verdict

APPROVED

The two-layer fix is exactly the shape the issue prescribed: prose discipline as the primary mechanism, mechanical pin-first resolution as the safety net for when prose discipline fails (which the operator's own session demonstrated, twice in 24h). The no-regression guarantee is structurally sound — resolve_ops_root_walk is the pre-refactor implementation verbatim, and every documented bypass condition (no session id, no pin dir, stale pin, escape hatch) falls through to it without touching cwd state. Test coverage matches the issue's prescription including the operator-flagged spaced-path regression.

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