Code Review: PR #422

Commit: e71cb0d530ebc382d426df1a4251a6dc5d436e22

Summary

Adds two advisory hooks to nudge agents toward MCP search tools (search_docs/search_code) instead of defaulting to grep+Read. remind-mcp-tools.sh fires at session start (only when .mcp.json exists), and suggest-mcp-search.sh fires on PreToolUse for Bash when grep/find targets framework paths. Both are non-blocking (exit 0 always). 13 test cases cover the detection logic.

Checklist Results

• ✅ Architecture & Design: Pass — follows existing advisory-hook pattern (detect-role-trigger.sh shape), no new layers or dependencies introduced
• ✅ Code Quality: Pass — scripts are focused, well-commented, set -u throughout, clean case/pattern matching
• ✅ Testing: Pass — 13 cases (8 should-fire, 5 should-not) cover the key branches; edge cases like non-Bash tool, non-framework path, piped grep all tested
• ✅ Security: Pass — no secrets, no user input passed to eval, no injection surface (jq extracts, grep matches, no dynamic execution)
• ✅ Performance: Pass — early exits on tool_name/command checks keep the hot path fast
• ✅ PR Description & Glossary: Pass — glossary present with MCP, Deferred tool, Advisory hook defined
• ⚠ Summary Bullet Narrative: Pass — all three bullets are narrative with rationale
• ✅ Technical Decisions (AgDR): N/A — no new libraries, frameworks, or architecture patterns; follows the established advisory-hook convention
• ✅ Adopter Handbooks: Pass — no violations detected (migration-safety N/A, clean-architecture N/A for shell scripts, commit-message-quality advisory — no findings)

Issues Found

None.

Suggestions

1. suggest-mcp-search.sh:30-34 — grep -E without -r on framework dirs would not trigger: The case arms match grep -r*, grep --include*, grep -l*, and piped grep. But grep -E "pattern" roles/ (without -r) targeting a framework dir would slip through. Low priority since single-file grep is less of a token concern than recursive grep.

2. remind-mcp-tools.sh:25-31 — walk-up loop for .mcp.json: Correctly walks from REPO_ROOT upward, finding the ops-root .mcp.json even from a workspace clone. Good design.

3. nit: The find pattern on line 37 of suggest-mcp-search.sh requires the extension list (md|yaml|yml|ts|tsx|js|py) — a bare find roles/ -type f without -name wouldn't trigger. Acceptable since bare find without -name is rare in practice.

Verdict

APPROVED

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

Code Review: PR #422

Commit: e71cb0d530ebc382d426df1a4251a6dc5d436e22

Summary

Adds two advisory hooks to enforce MCP search-first behaviour: suggest-mcp-search.sh (PreToolUse on Bash — fires when grep/find targets framework paths, reminding the agent to use MCP search_docs/search_code first) and remind-mcp-tools.sh (SessionStart — emits a ToolSearch load command so MCP tools are in context from the beginning). Both are non-blocking (exit 0 always), matching the established advisory-hook pattern. Wired into .claude/settings.json at the correct hook points. 13 test cases cover the should-fire/should-not-fire matrix.

Checklist Results

• ✅ Architecture & Design: Pass — advisory-only hooks, no domain dependencies, follows established patterns (detect-role-trigger.sh, check-upstream-drift.sh)
• ✅ Code Quality: Pass — set -u, clear comments, clean separation of detection/emission
• ✅ Testing: Pass — 13 cases (8 positive, 5 negative), all passing
• ✅ Security: Pass — no secrets, no eval, no user input passed to commands
• ✅ Performance: Pass — sub-millisecond case/grep-q checks, bounded walk-up loop
• ✅ PR Description & Glossary: Pass — narrative bullets, glossary with 3 terms, testing instructions, issue linked
• ✅ Summary Bullet Narrative: Pass — all bullets are narrative with what+why
• ✅ Technical Decisions (AgDR): N/A — no new dependencies, frameworks, or architectural patterns introduced
• ✅ Adopter Handbooks: Pass — no violations (Clean Architecture Layers, Migration Safety, Commit Message Quality all N/A for this shell-only diff)

Issues Found

None.

Handbook Findings

No violations detected. All three loaded handbooks (clean-architecture-layers, migration-safety, commit-message-quality) are not applicable to this diff — no domain/application/infrastructure layering, no database migrations, and the single commit subject is descriptive.

Suggestions

1. nit: suggest-mcp-search.sh:54 — the "hooks/" pattern in framework_patterns is a bare substring that could false-positive on managed-project paths like webhooks/ or githooks/. Consider tightening to ".claude/hooks/" (the framework's hook location) or keeping it as-is since the advisory cost of a false-positive banner is near zero. Not blocking.

2. nit: Per handbooks/general/commit-message-quality.md — the commit (e71cb0d5) is 225 lines of diff with a subject-only message. An optional body explaining WHY the MCP search-first pattern matters (the deferred-tool friction root cause) would help future git blame readers. The PR description covers this well, so this is purely a commit-hygiene observation. Not blocking.

Verdict

APPROVED (posted as comment — cannot approve own PR)

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

Code Review: PR #422

Commit: 59f9de7037ef0de063fe46a1196355d67d359a65

Summary

Adds two advisory hooks to enforce the MCP search-first pattern: suggest-mcp-search.sh fires on PreToolUse for Bash when grep/find targets framework paths, and remind-mcp-tools.sh emits a ToolSearch load command at session start. Both are wired in settings.json.

Checklist Results

• ✅ Architecture & Design: Pass — advisory hooks follow the established pattern (exit 0 always, stderr banners, no blocking)
• ✅ Code Quality: Pass — clean shell scripts with proper set -u, clear comments, deterministic control flow
• ✅ Testing: Pass — 13 test cases covering both should-fire and should-not-fire scenarios
• ✅ Security: Pass — no secrets, no user input processed unsafely, jq -r for JSON parsing
• ✅ Performance: Pass — early exits on non-matching tools/commands, no expensive operations
• ✅ PR Description & Glossary: Pass — glossary present with MCP, deferred tool, advisory hook definitions
• ⚠ Summary Bullet Narrative: Pass
• ✅ Technical Decisions (AgDR): N/A — no new libraries or architecture patterns; hooks follow the existing advisory shape

Issues Found

None.

Suggestions

1. suggest-mcp-search.sh:29-33 — the case statement catches grep -rn* and grep -r* but grep -r* already subsumes grep -rn* since -rn starts with -r. The grep -rn* branch is unreachable. Harmless but could be simplified.

2. remind-mcp-tools.sh:24-31 — the walk-up loop looking for .mcp.json is a nice touch (handles portfolio layouts), but it will also match an .mcp.json at / if one existed. The [ "$r" != "/" ] guard prevents that, so this is fine — just noting the defensive shape works correctly.

Verdict

APPROVED (posted as comment — cannot approve own PR)

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