Code Review: PR #582 — VERDICT: CHANGES REQUESTED

Posted as a comment because GitHub blocks "request changes" on a self-authored PR (single-account fork). Treat this as CHANGES REQUESTED — do not merge until exemption #2 is tightened.

Commit: 25a889ffc2f821204ca5c5f91d3ce7e25e9c2b1a

Summary

Adds three target-aware exemptions to the Bash-write path of require-active-ticket.sh so harmless writes stop requiring an active ticket (#569): (1) deletion-only rm via a new bash_command_is_deletion_only helper; (2) shell-variable targets ($VAR/${VAR}); (3) absolute paths outside the repo root. Both test suites are green locally — test_detect_bash_write.sh 97/97, test_require_active_ticket_bash.sh 26/26.

I reproduced the fix in an isolated sandbox faithful to the test harness (the worktree itself can't be used directly — resolve_ops_root walks up to the real fork and inherits the live session's current-ticket, so every command falsely passes; the sandbox with its own fork anchors is the authoritative environment).

Checklist Results

• Architecture & Design: Pass
• Code Quality: Pass
• Testing: Pass (suites green; security regression test missing — see below)
• Security: Fail — exemption #2 opens a trivially-reachable gate bypass
• Performance: Pass
• PR Description & Glossary: Pass (clear summary, narrative bullets, glossary present)
• Summary Bullet Narrative: Pass
• Technical Decisions (AgDR):N/A (bugfix to existing control; no new library/pattern)
• Adopter Handbooks: N/A (diff is shell only; no language handbooks fire; blocking migration-safety handbook doesn't apply)

Issues Found

BLOCKING — Exemption #2 ($VAR targets) opens a meaningful bypass of the ticket gate

This is a security control (#151 closed the Bash-write bypass surface deliberately). Exemption #2 reopens part of it. The rule is:

case "$FILE_PATH" in
  \$*) exit 0 ;;
esac

It fires whenever the extracted target begins with $ — not only when the target IS a bare variable. I confirmed via bash_extract_write_target that the extractor returns the literal token, so:

All four were verified in the sandbox (rc=0 where 2 was expected for the bottom three). An agent under the ticket gate can now write to any tracked source file by prefixing the path with a shell variable. That defeats the control the hook exists to enforce.

Two things make this worse than a borderline call:

1. It's strictly broader than the bug requires. Per #569, the bug only manifests when an active-ticket marker exists but the var-target can't be mapped to a project. Exemption #2 fires regardless of ticket state — it removes the gate for var-prefixed targets even in the no-ticket case. The fix over-reaches the bug.
2. #569 itself offered the safe fix. Suggestion (b) in the issue — "resolve the write target to an absolute path before the exempt-path case match so .claude/ is recognised regardless of cwd" — fixes the real annoyance (cat > "$CEO" where $CEO is a .claude/ path) without opening the source tree. The PR picked suggestion (a) ("treat unresolved/variable target as non-blocking"), the fail-open option. A security gate should fail closed: if you can't resolve the target, block (pre-PR behaviour), don't exempt.

Requested change (pick one):

• Preferred: drop exemption #2 and instead resolve $VAR-bearing targets at hook time (expand against the live env — e.g. an eval-free ${!name} lookup, or envsubst-style expansion) before the exempt-path case. Then a $CEO pointing into .claude/ is recognised as exempt while $PWD/src/app.ts resolves to a tracked path and stays blocked. This is #569 suggestion (b).
• Acceptable fallback: narrow the variable exemption to a bare, whole-target variable only (^\$\{?[A-Za-z_][A-Za-z0-9_]*\}?$ — no trailing /...), AND only when an active ticket marker is present for the session (matching the bug's actual precondition). A var with a path tail ($D/app.ts, $PWD/src/...) must fall through to the gate.

Either way, add regression cases to test_require_active_ticket_bash.sh asserting echo x > $PWD/src/app.ts and echo x > $D/app.ts are BLOCKED without a ticket. The current suite tests only the bare-$CEO happy path — which is exactly why this slipped through.

Verified (no issues)

• echo x > src/app.ts (no ticket) → BLOCKED (rc=2). Case 24 confirmed independently in the sandbox.
• rm old.ts && echo x > src/app.ts → BLOCKED — bash_command_is_deletion_only correctly returns 1 when a content-writer rides alongside rm. Exemption #1 is sound.
• Exemption #3 (absolute outside repo): an absolute path into the repo (src/deep/y.ts, sandbox-abs src/x.ts) is correctly relativized and BLOCKED — the REL_PATH = FILE_PATH guard only exempts paths that don't strip under REPO_ROOT. Exemption #3 is sound.
• cat > /tmp/..., rm -f ..., echo x > .claude/session/... all correctly ALLOWED.
• Deletion-only helper negative class (cp/mv/dd/install, rm+redirect, rm+tee) all correctly return "not deletion-only".

Suggestions (non-blocking)

• The fail-open vs fail-closed choice for an unresolvable security-gate target is a genuine design decision with a tradeoff. Consider a short AgDR (/decide) recording the rationale — future readers will hit this exact question.

Verdict

CHANGES REQUESTED

Exemptions #1 (deletion-only) and #3 (absolute-outside-repo) are good and well-tested. Exemption #2 as written lets any source write through a $-prefixed path and must be tightened (resolve the variable, or narrow to bare-var + ticket-present) with a blocking regression test before merge.

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

Code Review: PR #582 (re-review) — Verdict: APPROVED

(Posted as a comment — GitHub blocks self-approval; verdict is APPROVED and the Rex marker is written.)

Commit: b5c7efef641174b9cd321dc78e042b9e87bb30ab

Summary

Re-review after b5c7efe, which closes the fail-open bypass I flagged previously. The earlier blanket $*) exit 0 exemption let any write whose target began with $ skip the ticket gate — including echo x > $PWD/src/app.ts and > $D/app.ts, where the variable carried a path tail into tracked source. That was fail-open on a security gate.

The fix removes the blanket exemption and narrows it to two safe cases:

1. A temp-dir variable target ($TMPDIR/$TMP prefix).
2. A bare whole-target variable ($CEO, ${marker}), matched by the anchored regex ^\$\{?[A-Za-z_][A-Za-z0-9_]*\}?$ — no path/extension/trailing tail permitted.

A variable WITH a path tail is neither bare nor absolute, so it falls through to the ticket gate and blocks — the safe direction. The author deliberately does not expand $PWD/$HOME (added nothing for blocking and tripped a /var↔/private/var symlink mismatch); the var+tail case blocks regardless, which is what matters.

Verification (ran, not just read)

Test suites — both green on PR HEAD:

• test_require_active_ticket_bash.sh → 29/29 PASS
• test_detect_bash_write.sh → 97/97 PASS

Independent manual probes (test-equivalent sandbox, no active ticket unless noted):

• echo x > $PWD/src/app.ts → BLOCK (rc 2) ✓
• echo x > $D/app.ts → BLOCK (rc 2) ✓
• echo x > "$HOME/work/src/x.ts" → BLOCK (rc 2) ✓
• cat > "$CEO" → ALLOW (rc 0) ✓
• cat > "${marker}" → ALLOW (rc 0) ✓
• echo x > src/app.ts (no ticket) → BLOCK ✓ ; (with active ticket) → ALLOW ✓

Bare-var regex adversarial probe — cannot be tricked into matching a path-tail form. All of $D/app.ts, $PWD/src/app.ts, ${marker}/app.ts, $VAR.ts, ${VAR}x.ts, $VAR-tail, ${VAR}/sub, $VAR/, prefix$VAR, $VAR$X, $1, ${1} fall through to the gate. Only pure $NAME / ${NAME} tokens match. The ^...$ anchors hold.

The bypass is closed. Fix is minimal, well-commented at the decision point, and the regression tests pin the exact bypass shapes.

Checklist Results

• ✅ Architecture & Design: Pass — narrows an over-broad exemption; correct fail-closed default
• ✅ Code Quality: Pass — anchored regex, clear inline rationale at the exemption
• ✅ Testing: Pass — 29/29 + 97/97; regression cases pin the bypass shapes
• ✅ Security: Pass — fail-open hole closed; var+tail now blocks
• ✅ Performance: Pass — N/A (hook-time string matching)
• ✅ PR Description & Glossary: Pass — Summary, Testing, Refs #569, Glossary all present
• ⚠ Summary Bullet Narrative: Pass — bullets are narrative with rationale
• ✅ Technical Decisions (AgDR): N/A — gate hardening, no new dependency/architecture decision
• ✅ Adopter Handbooks: N/A — diff is shell hooks/tests; architecture/general handbooks don't trigger (no DB migration, no app-layer code; no shell language handbook)

Minor note (non-blocking): the PR body still states "26/26" for the ticket-bash suite, but it now reports 29/29 after the three review-fix regression cases were added. Worth syncing the body number on a future edit; does not affect the verdict.

Issues Found

None.

Verdict

APPROVED

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