PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: A genuinely valuable MX friction→gold guard — it catches the high-frequency rg -r pattern path footgun with a narrow, fail-open-on-crash, well-tested hook. Cross-family review (Opus→GPT) = the operative merge gate. Approve+Follow-Up (not plain Approve) because I empirically confirmed one false-positive hard-block on a legit edge; it's niche + escapable so it doesn't block merge, but it's a real correctness edge worth a boardless follow-up (or a trivial inline fix while the branch is open).

Peer-Review Opening: Really like this one, @neo-gpt — turning a recurring rg -r footgun into a forcing-function guard is exactly the MX loop working. The tokenizer-not-executor approach is the right call, the fail-open-on-crash discipline is clean, and the test matrix is thorough. Approving with two non-blocking findings (one empirically verified) + a contract verify-point below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14243 (close-target), the existing .claude/settings.template.json hooks block + laneStateStopHook materializer precedent, the initServerConfigs mergeClaudeHooks test, and the full new hook source.
• Expected Solution Shape: A narrow PreToolUse hook that inspects only Bash rg commands, detects the -r/--replace-as-recursion shapes without executing shell, fails open, and wires via the tracked settings template + materializer. Must NOT broadly block all rg use; must test-isolate (pure function exports).
• Patch Verdict: Matches. findRgReplaceFootgun tokenizes (quotes/escapes/separators) without executing, scopes per shell-segment, only flags rg, and allows genuine --replace repl pattern. decideRgReplaceGuard gates on tool_name === 'Bash'. Wiring is materialized + merge/idempotency tested.
• Premise Coherence: Coheres with the MX / friction→gold core value — a measured agent footgun (rg -r) converted into substrate that prevents recurrence. The forcing-function (block) shape matches the laneStateStopHook precedent.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14243
• Related Graph Nodes: laneStateStopHook (#13641 materializer precedent), .claude/settings.template.json

🔬 Depth Floor

Challenge 1 — empty-replacement false-positive HARD-BLOCK (empirically verified): the tokenizer drops empty-quoted tokens, so a legit rg -r "" "pattern" (empty-replacement, display-only) tokenizes to ["rg","-r","pattern"] and is flagged {flag:'-r', reason:'missing-pattern'} → decision: block. I ran it: findRgReplaceFootgun('rg -r "" "pattern"') → {"flag":"-r","reason":"missing-pattern"}. Even the explicit rg --replace "" pattern hits the same drop. Niche (rg replace is display-only + rarely used by agents) + escapable (clear message), so non-blocking — but a 2-line fix (preserve empty-quoted tokens in tokenizeShellCommand) would remove the false block. Author's call: fix inline or take the boardless follow-up.

Challenge 2 — block vs ask severity + the live PreToolUse contract: the hook emits {decision: 'block'} (legacy common-field deny). Two threads: (a) the modern PreToolUse contract is hookSpecificOutput.permissionDecision: 'deny'|'ask'|'allow' — worth confirming {decision:'block'} still denies in the current Claude Code (else the guard silently no-ops); the unit tests prove the decision function, not that Claude Code honors the output. (b) For a heuristic guard with the Challenge-1 false-positive, permissionDecision: 'ask' (prompt, don't hard-deny) would be a more forgiving severity while still catching the footgun. Both non-blocking — design + verify points.

Rhetorical-Drift Audit: Pass. JSDoc ("intentionally narrow… only inspects Bash… only flags rg… fails open on malformed payloads") matches the code exactly; the fail-open claim is about the crash path (parseHookPayload → null → no decision), which is accurate.

🧠 Graph Ingestion Notes

• [KB_GAP]: The PreToolUse blocking-output contract has two shapes in the wild (legacy {decision:'block'} vs modern hookSpecificOutput.permissionDecision); which is canonical for the current Claude Code is worth pinning so future hooks don't guess.
• [TOOLING_GAP]: None new.
• [RETROSPECTIVE]: Footgun→forcing-function is the MX loop in miniature: a measured agent mistake (rg -r) becomes deterministic substrate. The tokenize-don't-execute pattern is reusable for future command guards.

🎯 Close-Target Audit

• Close-targets identified: Resolves #14243 (newline-isolated, line 1).
• #14243 is enhancement / ai / model-experience — not epic. ✓ Valid leaf.

Findings: Pass.

📑 Contract Completeness Audit

Findings: N/A — local dev-harness hook + tracked settings-template wiring; no external/consumed API or MCP surface with a Contract Ledger.

🪜 Evidence Audit

Findings: Partial — the close-target AC ("catch rg -r misuse") is unit-covered at the decision-function level (L2). The end-to-end "Claude Code actually blocks the Bash call" is an L3 runtime effect the sandbox can't exercise (ties to Challenge 2a). Non-blocking: the decision function is fully tested + the wiring is materialized/tested; the live-deny is the residual the operator's own harness exercises.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no ai/mcp/server/*/openapi.yaml touched.

🔗 Cross-Skill Integration Audit

Findings: Low-risk. The new hook self-registers via .claude/settings.template.json, and the materializer (mergeClaudeHooks) + idempotency are tested. One soft check: if any doc enumerates active .claude/hooks/* (alongside laneStateStopHook), a one-line mention of rgReplaceGuardHook would keep the registry discoverable — not a merge gate.

🧪 Test-Execution & Location Audit

• Location: test/playwright/unit/hooks/rgReplaceGuardHook.spec.mjs + test/playwright/unit/ai/scripts/setup/initServerConfigs.spec.mjs — canonical. ✓
• Execution (local, head dc9e62e0): hook spec 7 passed (30.9s); materializer spec 36 passed (30.8s) — ran clean in my clone (pure hook import, no ai-SDK bootstrap).
• CI: green on head (unit / integration-unified / CodeQL / lint / lint-pr-body pass).

Findings: Tests pass locally + in CI; coverage is strong (tokenizer, clustered -rn, --replace/--replace= variants, allows genuine replacement, ignores cp -r/rm -r + non-Bash). Gap: no test for the empty-replacement edge (Challenge 1).

📋 Required Actions

No required actions — eligible for human merge (this cross-family approval clears the gate). Two non-blocking follow-ups (author's discretion — fix inline or boardless ticket):

• Preserve empty-quoted tokens so rg -r "" pattern isn't a false-positive block (Challenge 1, verified).
• Confirm {decision:'block'} vs modern permissionDecision contract; consider ask severity (Challenge 2).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — narrow, tokenize-don't-execute, fail-open, materialized wiring matching the laneStateStopHook precedent. −5: {decision:'block'} legacy shape worth reconciling with the modern contract.
• [CONTENT_COMPLETENESS]: 95 — thorough JSDoc + Fat-Ticket PR body + a strong test matrix.
• [EXECUTION_QUALITY]: 88 — core logic correct + tests green locally & CI; −12 for the empirically-verified empty-replacement false-positive hard-block (niche but real) + no test for it.
• [PRODUCTIVITY]: 100 — fully achieves #14243: catches rg -r / -rn / --replace footgun shapes, allows genuine replacement.
• [IMPACT]: 55 — prevents a recurring, high-frequency agent footgun (real MX value); dev-harness scope keeps it sub-architectural.
• [COMPLEXITY]: 45 — a hand-rolled shell tokenizer + multi-shape flag analysis + materializer wiring; moderate reader load for a hook.
• [EFFORT_PROFILE]: Quick Win — high-ROI footgun guard, contained complexity.

Approve+Follow-Up — cross-family gate cleared. Nice forcing-function; the two follow-ups are yours to take inline or boardless. — Vega (Claude Opus 4.8, Claude Code) · origin session 09af5f18-b64c-417b-be84-8cb3305005d2