Code Review: PR #363

Commit: 543d1823d8678f486ebd61c95cd5422fc39519c8

Summary

Wave 2 PR 3 of #351: /setup --split-portfolio auto-seeds agent-routing.yaml in the private repo (Step 5) and writes the 8th portfolio.agent_routing key into the public fork's .claude/project-config.json (Step 6). Adds advisory Step 7a for single-fork adopters (no auto-seed, gitignore covers leak risk). 3 files, +61/-11. Updated split-portfolio v2 test bumps EXPECTED_KEYS from 7→8 and adds Assertion 1b for the seeded file's presence + agents: key. Test suite passes 11/11 locally.

Checklist Results

• ✅ Architecture & Design: Pass
• ✅ Code Quality: Pass
• ✅ Testing: Pass — Assertion 1b is tight (checks file exists AND grep ^agents:, so touch regression would fail)
• ✅ Security: Pass (no auth/secret/crypto surface)
• ✅ Performance: Pass (one extra cp in a bootstrap-class skill — negligible)
• ✅ PR Description & Glossary: Pass — glossary covers seed-vs-fill, the 8-key list, and the single-fork manual rationale
• ✅ Summary Bullet Narrative: Pass — every bullet names what changed + why (no label-only)
• ✅ Technical Decisions (AgDR):N/A — wiring follows AgDR-0050 § Axis 3 (no new decision introduced)
• ✅ Adopter Handbooks: N/A — no handbooks loaded (diff is markdown + bash test)
• ✅ CI checks: lychee + markdownlint + ticket-id + site-counts all green

Verifications performed against framework artefacts

1. SKILL.md Step 5 bullet shape — the new agent-routing.yaml bullet sits immediately after custom-handbooks/ and mirrors the established bullet shape: bold filename → one-sentence purpose → why-it-matters → cross-ref to the canonical doc → (Added in #351 PR 3.) provenance. Reading flow is consistent with the custom-skills / custom-handbooks / onboarding.yaml bullets above it.

2. SKILL.md Step 6 config-block JSON — agent_routing is the 8th key (after registry, projects_dir, ideas_backlog, onboarding, workspace_dir, custom_skills_dir, custom_handbooks_dir). Path matches the v2 convention ../apexyard-portfolio/agent-routing.yaml. Trailing prose correctly explains both that the key is optional (default ./agent-routing.yaml against ops-fork root) AND that setting it explicitly is the v2 shape.

3. Step 7a (single-fork advisory) — rationale is sound and internally consistent. The framework's .gitignore (verified: line 20 /agent-routing.yaml) already covers the leak risk, and the SessionStart hook apply-agent-routing.sh (verified: lines 97–101) reads via the safe [ -z "$ROUTING_PATH" ] || [ ! -f "$ROUTING_PATH" ] pattern — missing file is a zero-config no-op exit. So Step 7a's "don't auto-seed" choice is properly load-bearing on a graceful-degrade contract that already exists in #357's hook. Contrast with custom-skills/ and custom-handbooks/ in Step 5: those DO need to exist as empty dirs because the symlink/discovery mechanisms walk them at SessionStart; agent-routing.yaml doesn't need that and Step 7a is consistent with the rule "auto-seed only what the framework's read-path requires to exist".

4. Test extension — verified locally:

   • build_pre_setup_v2() seeds an example file with exactly version: 1 + agents: {} — matches the real framework example's first two non-comment lines (verified by grep: lines 51 and 92 of agent-routing.yaml.example).
   • apply_setup_v2() adds the 8th key to the JSON block AND copies the example into the private repo (guarded by [ -f example ] && [ ! -f target ] — idempotent + safe).
   • EXPECTED_KEYS updated 7→8 (verified the array's contents match the JSON block keys).
   • Assertion 1b: [ -f "$PRIVATE_ROUTING" ] && grep -q '^agents:' "$PRIVATE_ROUTING" — would correctly fail on a touch regression because grep wouldn't match.
   • Test ran locally: Passed 11 / Failed 0.

5. docs/multi-project.md update — accurately reflects the new automation. The "Seed from the framework example" prose now leads with the automation + uses "Manual fallback" framing for the split-portfolio command (right for adopters re-running /setup on an already-migrated fork or hitting a corner case). The "What ships next" list correctly drops the PR 3 row, leaving only PR 4 (gated on #348). No false claims about PR 4 status.

6. Scope discipline — 3 files, no drive-by edits. Verified git show --stat: only .claude/skills/setup/SKILL.md, .claude/skills/setup/tests/test_setup_split_portfolio_v2.sh, and docs/multi-project.md touched. No CLAUDE.md / site / AGENTS.md / handbooks / unrelated rules changes.

7. Compatibility with #353 + #357 — verified directly:

   • _lib-portfolio-paths.sh line 292 defines portfolio_agent_routing() reading .portfolio.agent_routing with default ./agent-routing.yaml. The new key written by Step 6 is exactly the one the resolver expects.
   • apply-agent-routing.sh line 97 calls portfolio_agent_routing and (lines 98–101) gracefully exits on missing file. The seeded YAML's shape (version: 1 + agents: {}) matches what the hook's parser expects for a zero-override config — no warnings on a freshly-bootstrapped fork.

Issues Found

None.

Suggestions

None blocking. Minor observations the author may or may not care to action in a follow-up:

• nit (advisory, not blocking): Step 7a's pseudo-code block uses $EDITOR — fine for the advisory, but the single-fork section is the one place where adopters who've never used the framework before see a cp + edit flow without surrounding context. Consider whether a one-line "(or vim / code / your editor of choice)" note is worth adding. Pure ergonomics, not load-bearing.

Verdict

APPROVED

Clean, focused, well-tested. Test PASS at 11/11 with the new Assertion 1b covering the seed correctness. Step 7a's "no auto-seed" rationale is internally consistent with the framework's existing graceful-degrade contract from #357. Documentation in docs/multi-project.md accurately captures the new automation. No scope creep. The 8th key wires cleanly into the resolver shipped in #353.

Ready to merge once CEO records per-PR approval via /approve-merge 363.

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