Code Review: PR #289

Commit: 4c46d8b35f929620784d397bca43d7f2a47994fc

Summary

Introduces _lib-tracker.sh — a tracker-dispatch library that routes ticket-existence verification through the configured CLI (gh / linear / jira / asana / custom / none) instead of hardcoding gh issue view. Four consumers (validate-pr-create.sh, verify-commit-refs.sh, validate-branch-name.sh, /start-ticket) refactor onto the lib; the default tracker.kind = gh preserves today's behaviour byte-for-byte. Closed-state recognition widens to a case-insensitive set covering non-GH workflow vocabulary (closed|done|cancelled|canceled|resolved|completed). Backed by AgDR-0033, 17 new tests, and updates to 5 existing test fixtures that copy the new lib into their sandboxes.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass
• Performance: Pass
• PR Description & Glossary: Pass
• Technical Decisions (AgDR):Pass — AgDR-0033 linked and exhaustive
• Adopter Handbooks: N/A — diff is shell + JSON config, no handbook applies

Issues Found

None blocking.

Suggestions

nit (low priority): tracker_owner_repo_param docstring vs implementation mismatch. The header docs at .claude/hooks/_lib-tracker.sh:16 claim "(gh: 'owner/repo'; others: empty)" but the implementation (tracker_owner_repo_param() { echo "$slug"; }, line 126-129) always echoes the slug unchanged regardless of tracker_kind. The function is also unreferenced by any consumer in this PR (grep found zero call sites). Either kill the function as dead code, or make it kind-aware per the docstring so it's ready for use. Not blocking — both consumers pass --repo directly via the view_command template's {owner_repo} substitution today.

Verified open notes from the author:

1. Mock-gh shim emits {number, state} while the new default view_command requests state,title,url,labels — the adapter's // "" defaults absorb the missing fields cleanly. Confirmed by running all five updated existing test files (8+7+12+13+8 = 48 cases, all passing) plus the new test suite (17/17). Not a real risk.
2. Per-tracker CLI auth left as operator responsibility — explicitly called out in AgDR § Consequences. Fine to defer; the hooks emit a clear blocked-message that points the operator at their CLI's docs.

Detailed observations

Shell quality (manual review — shellcheck not installed locally). _lib-tracker.sh (333 lines) follows the same shape as the existing _lib-portfolio-paths.sh / _lib-read-config.sh siblings: per-process caches, idempotent helpers, fallback values when config is missing. The _tracker_substitute helper uses bash 3.2-portable ${var//pat/repl} instead of sed, matching the macOS-compatibility convention. eval "$cmd" at line 287 is a code-injection surface in theory, but the input is the operator's own view_command template from their own project-config.json — same trust model as tracker.kind = custom accepting arbitrary commands. Acceptable.

Adapter normalisation (deep-read on gh and jira).

• _tracker_normalise_gh (lines 149-164): handles labels as [{name, ...}] OR plain strings; // "" defaults on every field. Verified echo '{"number":194,"state":"OPEN"}' | jq ... produces {state:"OPEN", title:"", url:"", labels:[]} cleanly — explains why the existing mock-gh shim still passes.
• _tracker_normalise_jira (lines 193-203): correctly reads .fields.status.name (REST shape from ankitpokhrel/jira-cli --raw), falls back to .status / .title / .url, casts all to string with | tostring (catches the case where Jira returns a numeric status code). Labels handle both [string] and [{name}] shapes.

Backward-compat. tracker.kind = gh with the default view_command = "gh issue view {id} --repo {owner_repo} --json state,title,url,labels" produces a call shape that's identical to the pre-PR direct call. Verified by running test_validate_pr_create_head.sh (8/8 pass) and test_validate_pr_create_upstream.sh (7/7 pass — both exercise the upstream-fallback path from #207). Zero behaviour change for the default adopter.

Closed-state set. closed|done|cancelled|canceled|resolved|completed covers GH ("CLOSED"), Linear ("Done"/"Cancelled"/"Completed"), Jira ("Done"/"Closed"/"Resolved"/"Cancelled"), Asana (derived "Closed" from .completed). UK + US spellings both present. Sensible scope — won't accidentally flag "In Review" or "Backlog" states.

validate-branch-name.sh regex extraction. Lines 91-99 strip the ^...$ anchors and outer parens via parameter expansion + a simple case-glob match. Works correctly for the default pattern ^(#[0-9]+|GH-[0-9]+|[A-Z]{2,10}-[0-9]+)$ and the strict-Linear pattern ^[A-Z]+-[0-9]+$ (verified by test case 7). Best-effort approach is fine — extreme patterns with nested top-level groups are out of scope and unlikely.

AgDR-0033 completeness. Hits every section in the template (Context, Options, Decision, Consequences, Artifacts). Options table includes the rejected alternatives (inline case blocks in each consumer, fully scripted runtime config) with honest pros/cons. Consequences section explicitly names what's not solved (per-tracker auth) and explains why (none exists as the documented escape hatch for trackers with no CLI). Mirrors the design-doc shape of _lib-portfolio-paths.sh as intended.

Test coverage. 17 cases in the new test file: default-GH regression × 2 (valid title passes, fabricated #N blocks), Linear × 3 (valid/closed/missing), Jira × 2 (valid/closed-Resolved), none × 2 (PR + commit hooks short-circuit), custom × 1 (operator-supplied template), branch-name × 2 (strict Linear pattern + default regression), library-API smoke × 4 (gh/linear/jira/none normalisation). Comprehensive coverage of every code path the PR adds.

Verdict

APPROVED

(Submitted as --comment because GitHub blocks self-approval on this PR — the verdict for the merge-gate is APPROVED. Approval marker written.)

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: 4c46d8b35f929620784d397bca43d7f2a47994fc

Code Review: PR #289 (re-review at new HEAD)

Commit: d99f4033a0d16c723e71830039132d482cabb5da
Previous reviewed commit: 4c46d8b35f929620784d397bca43d7f2a47994fc

Summary

Follow-up commit fix(#283): CI green — markdownlint MD056 + shellcheck SC2164 lands two cosmetic lint fixes on top of the previously-approved tracker-aware-hooks work. Two files touched, +4 / -4 lines, zero functional change.

Diff verification

Confirmed via gh api repos/me2resh/apexyard/commits/d99f4033...:

1. docs/agdr/AgDR-0033-tracker-abstraction.md:14 — escaped the pipe inside the inline code span (`[A-Z]+-[0-9]+ | #[0-9]+` → `[A-Z]+-[0-9]+ \| #[0-9]+`) so the markdown table parser stops reading the row as 4 cells. MD056 fix, correct.
2. .claude/hooks/tests/test_tracker_aware_hooks.sh:454, 467, 484 — added || exit 1 to three cd "$SB" calls inside subshells. SC2164 fix, correct. Subshell cd failure now propagates rather than silently continuing on the wrong working dir — defensible safety improvement on top of the lint compliance.

Checklist Results

• Architecture & Design: Pass (no change from prior review)
• Code Quality: Pass (no change from prior review)
• Testing: Pass — operator confirms 17/17 still pass locally
• Security: Pass (no change from prior review)
• Performance: Pass (no change from prior review)
• PR Description & Glossary: Pass — Glossary intact, AgDR-0033 still linked, Closes #283 preserved
• Technical Decisions (AgDR):Pass — AgDR-0033 unchanged structurally; the markdown escape is purely renderer-facing
• Adopter Handbooks: N/A (no handbook diff-match on the two files in this commit)

Issues Found

None. This is a pure CI-green follow-up — two surgical fixes to clear markdownlint MD056 and shellcheck SC2164. The previous review's findings on substance still stand; nothing in this commit invalidates the approval.

Suggestions

None.

Verdict

APPROVED

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

Code Review: PR #289 (re-review)

Commit: bfc0f342561d96cd8d9461b81c4b67b47c1cb573

Re-review of the new HEAD after the routine chore(#283): merge origin/dev commit. Previous review at d99f4033 was clean; this review focuses on the merge conflict resolution.

Summary

Single new commit on the PR branch since last review: a merge of origin/dev (parent 54e9978 — #291 / #288 /feature-diagram) into the feature branch. The only file-level conflict was .claude/project-config.defaults.json, where both sides had added a new top-level key after the portfolio block:

• HEAD (#283): tracker block — tracker dispatch (AgDR-0033)
• dev (#287 via #284): pdf block — /pdf skill config (AgDR-0034)

Resolution keeps both blocks as additive sibling top-level keys.

Conflict resolution — verified

• JSON parses cleanly (jq -e . < project-config.defaults.json succeeds).
• Top-level keys after merge: _comment, _schema_version, agdr_trigger_dep_files, agdr_trigger_paths, branch, commit, git, leak_protection, pdf, portfolio, pr, pre_push, review_markers, ticket, tracker — both new keys present, none lost from either side.
• Delimiters correct: portfolio closes with },, tracker closes with },, pdf is the last key and closes with } (no trailing comma) — JSON-compliant.
• No semantic clash: tracker is consumed by _lib-tracker.sh + validate-* hooks; pdf is consumed by the /pdf skill. Different consumers, no overlap.

Other content from origin/dev

The merge brought in the other commits sitting on dev since branch-off (#285, #286, #287, #288, #291 — feature-diagram skill, /pdf skill, ticket templates, update chain migrations, AgDRs 0031/0032/0034/0035). All passive framework content from already-merged PRs; the feature branch accepts these untouched. git diff d99f4033..bfc0f342 -- <pr-289-files> shows only the 7-line additive change to project-config.defaults.json — every other PR-283 file is byte-identical to the previously-approved version.

Regression check

• test_tracker_aware_hooks.sh: 17 passed, 0 failed.
• test_validate_pr_create_head.sh: 8/0.
• test_validate_pr_create_upstream.sh: 7/0.
• test_verify_commit_refs_upstream.sh: 12/0.
• test_single_closes_per_pr.sh: 13/0.
• test_validate_pr_required_sections.sh: 8/0.
• CI checks on bfc0f34: all four (Verify Ticket ID, lychee, markdownlint-cli2, shellcheck .claude/hooks) green.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass
• Security: Pass (no auth / crypto / secrets / user-data paths touched in the merge resolution)
• Performance: Pass
• PR Description & Glossary: Pass (unchanged from previous review)
• Technical Decisions (AgDR):Pass (AgDR-0033 unchanged)
• Adopter Handbooks: N/A (no handbooks configured)

Issues Found

None.

Verdict

APPROVED (recorded as comment because GitHub blocks self-approval; approval marker written to .claude/session/reviews/289-rex.approved per framework merge-gate convention)

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