Code Review: PR #524

Commit: `60b3bda91901093b83ee12c215b569a79703e2a6`

Summary

Adds a tier-0 per-worktree ticket marker (`tickets//`) resolved before the existing per-project file and the `current-ticket` fallback, in both `require-active-ticket.sh` and `require-migration-ticket.sh`. Fixes the last-writer-wins collision where two agents fanned out on different tickets within one repo both wrote the shared per-project marker, silently passing subsequent gate checks against the wrong ticket. `/start-ticket` writes the per-worktree path when inside a linked worktree. AgDR-0066 records the decision.

Checklist Results

• ✅ Architecture & Design: Pass
• ✅ Code Quality: Pass
• ✅ Testing: Pass
• ✅ Security: Pass
• ✅ Performance: Pass
• ✅ PR Description & Glossary: Pass
• ✅ Summary Bullet Narrative: Pass (bold-lead + rationale on every bullet)
• ✅ Technical Decisions (AgDR): Pass (AgDR-0066 linked + present)
• ✅ Adopter Handbooks: N/A (no violations — see below)

Verification performed

I checked out the PR head (`60b3bda9`), ran the suite, and probed the edge cases the change hinges on.

1. Tier-0 logic correctness — verified.

• Branch detection: `CLAUDE_WORKTREE_BRANCH` env var OR `git -C branch --show-current` — both paths exercised.
• `/`→`` transform via `${WT_BRANCH//\//}` — correct; `feature/x` resolves to `feature__x`.
• File-vs-directory duality (the load-bearing concern): confirmed safe. Ran adversarial cases against a proper git sandbox (the bare `mktemp` sandbox without `git init` mis-resolves OPS_ROOT and gives false negatives — worth knowing):
  • dir at `tickets/` (worktree mode) + matching tier-0 branch, no fallback → exit 0 (correct pass).
  • dir at `tickets/` + NON-matching branch + no fallback → exit 2 BLOCKED. This is the critical proof: a directory at the tier-1 path reads as "tier-1 absent" via `[ -f ]`, so it never falsely satisfies the per-project tier nor errors.
  • dir at tier-1 + non-matching branch + WITH `current-ticket` fallback → exit 0 (falls through cleanly).

2. No single-agent regression — verified. No worktree branch ⇒ tier-0 lookup misses (no marker written) ⇒ falls straight to the per-project FILE then fallback. Migration hook's restructured `if / elif` with `[ -z "$MARKER" ]` guards preserves the original precedence exactly (manually checked M1 no-marker→block, M2 tier-0 dir resolves, M3 tier-0 dir + wrong branch + no fallback→block, M4 single-agent FILE resolves).

3. Test quality — genuinely proves isolation, not just happy-path. Case #14 (branch-B not satisfied by branch-A's `tickets/myproj/feature__a` marker, no fallback → BLOCKED) is the real isolation assertion — it proves the collision the PR fixes is actually closed, not merely that the happy path works. `15/15` pass locally; `bash -n` + `shellcheck -S error` clean on both hooks.

4. Glossary / bullets / AgDR / commit SHA — all pass. Glossary has 5 well-scoped terms. Summary bullets are narrative (what + why) — no label-only bullets, advisory check clean. AgDR-0066 present and linked, with options-considered table (flock and session-namespace alternatives weighed). Commit message is exemplary (WHY-first body, trade-offs, `Closes #513`, co-authorship).

Handbook Findings

None. The Migration Safety (blocking) handbook triggers on a migration file in the diff with destructive SQL — this PR modifies the migration gate hook, not any migration file, so it's N/A. Clean Architecture and Commit Message Quality: no violations (commit message in fact models the quality handbook well).

Suggestions (non-blocking)

• Write-side / read-side worktree-detection asymmetry. `/start-ticket` (write) gates the per-worktree path on `--git-dir != --git-common-dir` (true linked-worktree only), whereas the read-side hooks treat any non-empty `git branch --show-current` as tier-0-eligible. This is harmless today — in single-agent mode no per-worktree marker exists, so the read-side tier-0 lookup simply misses and falls to tier 1 — but the two sides describe "is this a worktree?" differently. If a future change ever made the read side write-sensitive, the asymmetry could bite. Worth a one-line comment noting the read side is intentionally permissive (tier-0 miss is free).
• AgDR numbering. This takes 0066 while 0065 is absent on `dev` (presumably claimed by another in-flight PR). 0066 is unique so no collision, but with concurrent AgDR authoring the gap is worth a glance at merge time.
• Orphaned per-worktree markers after `git worktree remove` are acknowledged in the AgDR as acceptable (same stale-marker behaviour as today). Fine — flagging only so it's on record.

Verdict

APPROVED

The fix is additive, the file-vs-dir duality is correctly handled by `[ -f ]` throughout, single-agent behaviour is unchanged, and the new isolation test genuinely proves the collision is closed.

🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: `60b3bda91901093b83ee12c215b569a79703e2a6`

Code Review: PR #524 (re-review)

Commit: `4de2c1153019f6379b25e1941a94001494069dab`

Re-review of the single follow-up commit (`4de2c11`) addressing my prior non-blocking nit — the read/write worktree-detection asymmetry. Previously APPROVED at `60b3bda`.

Summary

The new commit replaces branch-presence-based worktree detection with a linked-worktree check that compares the absolute git-dir against the absolute common-dir, and aligns all three sites (`require-active-ticket.sh` read side, `require-migration-ticket.sh` read side, `/start-ticket` write side) on identical logic.

Verification performed

(1) Absolute-form comparison is correct — confirmed empirically on git 2.54.0:

• Main checkout, from a nested subdir (sub/deep): plain forms give /…/main/.git vs ../../.git (would false-positive); absolute forms give /…/main/.git vs /…/main/.git → equal → correctly NOT a worktree. The prior nit is genuinely fixed.
• Real linked worktree, from a nested subdir: absolute git-dir /…/main/.git/worktrees/wt-linked vs common-dir /…/main/.git → differ → correctly detected, branch resolved as feature/x.

(2) Read and write sides use identical detection — confirmed by source inspection:

• require-active-ticket.sh:244-245, require-migration-ticket.sh:215-216, and start-ticket/SKILL.md:152-153 all use --absolute-git-dir vs --path-format=absolute --git-common-dir. The only (correct) difference: the two read-side hooks anchor with git -C \"\$_fdir\" (the edited file's dir, since the hook may run from anywhere), while the skill uses session cwd (it runs interactively inside the worktree). No asymmetry remains.

(3) Test suite passes at PR HEAD — 16/16:
Ran test_require_active_ticket_bash.sh against a worktree checked out at 4de2c11. All 16 pass, including the new case 16 (a real git worktree add with NO CLAUDE_WORKTREE_BRANCH → tier-0 honored purely via git detection), which exercises the symmetric detection path rather than the env-var shortcut.

(4) No regression to existing tiers — confirmed:

• Tier-0 only exit 0s when [ -f \"\$PER_WORKTREE_MARKER\" ] is true (line 254); absent marker falls straight through to tier-1 (per-project FILE) then tier-2 (current-ticket), unchanged.
• bash -n clean; shellcheck -S error clean on both hooks.
• Cases 1-15 (bootstrap exemptions, redirect/python-write detection, .claude/.md exemptions, per-project file, branch-B isolation) all still pass.

Note on git ≥ 2.31 (--path-format=absolute)

`--path-format` landed in git 2.31.0 (March 2021), so the requirement is mild. Not a blocker, and worth recording why: on git < 2.31 the --path-format=absolute call errors → _gcd=\"\", and since _gd is non-empty the _gd != _gcd branch evaluates true even in a main checkout. But because the read and write sides run the identical degraded logic, they stay symmetric: a single-agent flow on ancient git would write and read a tickets/<project>/<branch>-shaped marker, so the gate still functions correctly — just via the per-worktree path instead of the per-project file. Graceful, symmetric degradation rather than a gate bypass. If you want to be belt-and-suspenders, a future tweak could fall back to plain --git-common-dir + realpath when --path-format is unsupported, but that's optional polish, not required.

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass (16/16, new case exercises the real detection path)
• Security: Pass (no auth/secret/crypto surface)
• Performance: Pass (two extra git rev-parse only when no env var and PROJECT resolved)
• PR Description & Glossary: Pass (Glossary present, narrative summary, links #513)
• Technical Decisions (AgDR): Pass (AgDR-0066 present and accurate)
• Adopter Handbooks: N/A (no handbooks loaded for shell-only diff)

Issues Found

None.

Suggestions

• (Optional, non-blocking) Add a --path-format-unsupported fallback for git < 2.31 if you ever need to support very old git; the current symmetric-degradation behaviour is acceptable as-is.

Verdict

APPROVED

The nit from the prior review is correctly resolved, detection is now symmetric across all three sites, and the test suite proves both the no-false-positive and the real-worktree-detection properties.

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