Code Review — PR #285 (commit 1166819)

Verdict: APPROVED (via comment — author cannot self-approve) ✅

Summary

Closes the uniformity hole between Pattern A (heredoc-only) and Pattern B (template-file-driven) ticket-creating skills described in #281. All 7 ticket types now resolve their issue body via portfolio_resolve_template tickets/<name>.md, so the AgDR-0023 path-mirroring contract applies uniformly. Well-scoped, well-tested, ships AgDR-0031 capturing the four orthogonal design choices (location / refactor scope / fallback shape / top-level-template move).

Architecture & Design

• No domain/application/infrastructure layering concerns — markdown + bash only
• Path-mirroring contract preserved — resolver unchanged; only the consumer surface broadens
• Single resolution call shape across all 7 skills (portfolio_resolve_template tickets/<name>.md) — no skill-specific exceptions
• AgDR-0031 dimension-by-dimension rationale follows the existing AgDR shape

Code Quality

• Bash test follows test_portfolio_paths.sh conventions (sandbox fixture, make_fork helper, run_case runner, per-process cache reset)
• Template files use consistent {{placeholder}} Mustache-style across all 5 new files
• Glossary placeholder section added to every ticket template — encourages the PR-quality glossary discipline already enforced on PR bodies (.claude/rules/pr-quality.md)

Testing

• 36 new test cases across 5 case sets: default resolution ×7, single-fork override ×7, split-portfolio sibling override ×7, missing-template fallback ×7 (resolver + WARN-on-stderr wrapper ×2), mixed-override scenario ×1
• Test verifies the fallback contract end-to-end — both resolver behaviour (empty + nonzero) AND a wrapper that emulates how the consuming SKILL.md reacts (FALLBACK output + WARN on stderr)
• Existing test_portfolio_paths.sh (38/38) and .claude/skills/investigation/tests/smoke.sh (18/18) still pass after the path move — verified locally

Security

• No secrets, no input-handling code, no auth/crypto surfaces touched
• Test sandboxes use mktemp -d with pwd -P canonicalisation for macOS compatibility (matches existing pattern in test_portfolio_paths.sh)
• No eval of user-derived input; test snippets are framework-authored heredoc strings

Performance

• N/A — markdown + bash, no hot paths
• Template resolution adds one portfolio_resolve_template call per ticket creation; already cached per-process via _PORTFOLIO_*_CACHE vars

PR Description Quality

• Clear 4-bullet summary
• Ticket linked (Closes #281)
• Testing section names every test that runs + pass counts
• Glossary present with 4 terms (Pattern A vs B, custom-templates path-mirroring, Heredoc fallback, templates/tickets/ subdir)

Technical Decisions (AgDR) — BLOCKING CHECK

• AgDR-0031 at docs/agdr/AgDR-0031-ticket-template-uniformity.md with status: accepted, captures all 4 option dimensions, declares the /update migration step as Out of Scope (sibling ticket)
• Resolution preserves AgDR-0023's contract — no new override semantics, just broader coverage
• No undocumented technical decisions detected in the diff

Handbook Findings

No handbooks under handbooks/architecture/, handbooks/general/, or handbooks/language/<lang>/ triggered by this diff (markdown + bash only — no language-handbook globs match). No custom-handbooks/ resolved.

Observations / Nits

1. /migration consumes two templates (nit). The skill resolves agdr-migration.md (AgDR side, top-level) AND tickets/migration.md (ticket body, in the new subdir). Diff captures this implicitly via the two resolver call sites; a one-line callout in AgDR-0031's Decision section would make it explicit. Non-blocking.
2. AgDR-0017 + AgDR-0027 Artifacts list mentions templates/{spike,investigation}.md (informational). AgDR-0031 correctly calls these out as intentionally-unchanged historical records — AgDRs document state at decision time. The "current paths are documented in templates/README.md and CLAUDE.md" note is the right mitigation.
3. Backward-compat migration step correctly scoped Out (informational). PR body and AgDR-0031 defer the /update flow that detects old top-level overrides to a separate ticket coupling with the existing update-chain ticket. Good scope discipline.

Verdict

APPROVED. Clean, well-tested, well-documented refactor that closes a real adopter-facing UX hole with no behaviour change for adopters who never customise templates. Heredoc-fallback + WARN-on-stderr correctly trades a slightly larger SKILL.md surface for "partial cherry-picks don't break installations" — same trade-off shape as the existing portfolio_validate failure-mode design.