feat: add PreToolUse hook to gate GroupMe write operations #163
No reviewers
Labels
No labels
domain:backend
domain:devops
domain:frontend
status:approved
status:in-progress
status:needs-fix
status:qa
type:bug
type:devops
type:feature
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
ldraney/claude-custom!163
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "160-block-groupme-send-hook"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Adds a PreToolUse hook that requires explicit user approval before any GroupMe write operation (
send_message,add_member,remove_member). Triggered by the 2026-03-25 data exposure incident where a forked session sent contract data with parent names and emails to the wrong GroupMe group.Changes
hooks/block-groupme-send.sh-- NEW. PreToolUse hook following theblock-mcp-merge.shpattern. Extractsgroup_namefromtool_input, formats a tool-specific summary, returnspermissionDecision: "ask". Each write tool gets a distinct prompt:settings.json-- Added PreToolUse entry matchingmcp__groupme__send_message|mcp__groupme__add_member|mcp__groupme__remove_membertests/test_block_groupme_send.sh-- NEW. 18 assertions across 6 test cases covering all 3 write tool shapes, unknown tool passthrough, text truncation (80 char limit), and missing field defaults.Test Plan
bash tests/test_block_groupme_send.sh-- 18/18 passlist_groups,list_members,get_group) are excluded from the hook matcher -- no approval friction for readssettings.jsonvalidated as valid JSONReview Checklist
block-mcp-merge.shpattern exactlyRelated
QA Review -- APPROVED
Verdict: approved
Review Summary
All 3 files are clean and correct. No issues found.
hooks/block-groupme-send.sh
block-mcp-merge.shpattern exactly: reads stdin, extracts fields with jq, emitspermissionDecision: "ask"exit 0(no interference with read-only tools)head -c 80//operatorsettings.json
send_message|add_member|remove_memberlist_groups,list_members,get_group) correctly excludedtests/test_block_groupme_send.sh
assert_contains,assert_ask,assert_emptyhelpersPR #163 Review
DOMAIN REVIEW
Tech stack: Bash hooks for Claude Code PreToolUse system + JSON (settings.json). Domain expertise applied: shell scripting patterns, Claude Code hook contract, jq usage, MCP tool schema validation.
Hook script (
hooks/block-groupme-send.sh-- 37 lines): Clean, minimal, follows theblock-mcp-merge.shpattern.INPUT=$(cat)for stdin,jq -rfor field extraction,casestatement for tool routing,jq -nfor structured JSON output. Default*case exits 0 (passthrough for unmatched tools). Text truncation viahead -c 80.Settings.json: New PreToolUse entry with pipe-separated matcher for 3 write tools. Correctly placed between the merge gate and the Task gate. JSON structure is valid.
Tests (
tests/test_block_groupme_send.sh-- 144 lines): 18 assertions across 6 test cases. Covers all 3 write tool shapes, unknown tool passthrough, text truncation, and missing field defaults. Well-structured withassert_contains,assert_ask, andassert_emptyhelpers. This is the first test file in the repo -- establishes atests/directory convention.BLOCKERS
1. Hook extracts
group_namebut MCP tools usegroup_idThe hook reads
.tool_input.group_nameon line 11, but all three GroupMe MCP tools (send_message,add_member,remove_member) usegroup_idas the parameter -- a numeric ID, not a human-readable name. Verified against the live MCP tool schemas:send_message:{group_id: string, text: string}-- nogroup_nameadd_member:{group_id: string, nickname: string, ...}-- nogroup_nameremove_member:{group_id: string, membership_id: string}-- nogroup_nameThe PR description states "Deploy order: groupme-mcp (name-based resolution) deploys first, then this hook" -- meaning this hook targets a future MCP schema that does not yet exist. Until the MCP is updated, every permission prompt will show "Send to unknown: ..." because
group_namewill never be present in tool_input. The gate itself still fires (theaskdecision still works), but the user gets zero useful context about which group is being targeted -- defeating the purpose of the incident-response hook.This is a BLOCKER because:
group_nameinputs that will never arrive from the current MCP. All 18 assertions pass, but they test a fiction.Resolution options:
group_idextraction as a fallback: extractgroup_idfrom tool_input, display it in the prompt. At minimum the user sees the numeric ID and can cross-reference.group_name(future) andgroup_id(current) with fallback:GROUP_NAME=$(echo "$INPUT" | jq -r '.tool_input.group_name // .tool_input.group_id // "unknown"'). This works today and tomorrow.Option (c) is recommended -- it is forward-compatible and works immediately.
NITS
No truncation indicator:
head -c 80truncates text silently. Adding...when the text exceeds 80 chars would improve the prompt UX. Minor.create_groupnot gated:mcp__groupme__create_groupis a write operation (creates a new group) but is excluded from the matcher. The incident was about wrong-group targeting, socreate_groupis arguably lower risk. But worth documenting the exclusion rationale.Test 5 fragile JSON construction: The long-text test (lines 112-124) builds JSON via string interpolation with escaped quotes rather than using
jq -n. IfLONG_TEXTcontained quotes or backslashes, the test would break. Low risk since the test text is controlled, but ajq -n --argapproach would be more robust.First
tests/directory in repo: This PR establishes the convention. No issue, just noting it for process awareness -- future hooks should follow this pattern.SOP COMPLIANCE
160-block-groupme-send-hookreferences issue #160PROCESS OBSERVATIONS
DORA impact: This is a Change Failure Rate (CFR) mitigation -- a direct incident response. The hook reduces the probability of data exposure via GroupMe write operations. High-value, low-complexity change.
Deploy dependency risk: The PR explicitly states a deploy ordering dependency (MCP first, then hook). This creates a coordination requirement that is not enforced by any mechanism. If the hook deploys first, it works but provides degraded prompts. Recommend making the hook resilient to both schemas (option c above) to eliminate the ordering dependency entirely.
Test precedent: First test suite in the repo. Good signal for the project's maturity. Consider documenting the test convention (bash tests in
tests/, naming patterntest_*.sh).VERDICT: NOT APPROVED
One blocker: the hook extracts
group_namefrom tool_input, but the current (and only deployed) MCP schema usesgroup_id. The hook will show "unknown" for every group until a not-yet-deployed MCP change lands. Fix with agroup_name // group_idfallback to make the hook work today and forward-compatible with the planned MCP change.QA blocker fixed in
aa8f6d5:Hook fix (
hooks/block-groupme-send.shline 11):Test added (
tests/test_block_groupme_send.sh):group_id-only input shape (nogroup_namepresent)"unknown"default when neither field is presentAll 21 tests pass.
PR #163 Review (Re-review)
DOMAIN REVIEW
Tech stack: Bash shell scripting, jq, Claude Code PreToolUse hooks.
Previous blocker resolution: The prior review flagged that the hook used
group_namewithout agroup_idfallback, meaning the gate would show "unknown" when the MCP server sendsgroup_idinstead ofgroup_name. This is now resolved:The jq alternative operator chain
group_name // group_id // "unknown"correctly handles all three cases: name-based resolution (new MCP), ID-based (backward compat), and missing fields (graceful default). Test 6 explicitly validates this path by sending onlygroup_idand asserting it appears in the reason string.Pattern conformance: Hook follows
block-mcp-merge.shexactly --INPUT=$(cat),jq -rextraction with//defaults,jq -n --argfor safe output construction,exit 0for unknown tools. No deviation from the established pattern.Safety: All values interpolated via jq
--arg(safe from injection). Noeval, no command substitution of user input. No secrets or credentials.Test coverage: 7 test cases with 21 assertions covering all 3 write tools, unknown tool passthrough, text truncation, group_id fallback, and missing fields. PR body says "18 assertions across 6 test cases" -- actual count is 21/7. Minor description mismatch (tests exceed what's claimed, which is fine).
BLOCKERS
None. The previous blocker (group_name without group_id fallback) is resolved.
NITS
create_groupnot gated: The GroupMe MCP exposesmcp__groupme__create_groupas a write operation, but it is not included in the matcher. Creating a group is lower-risk than sending messages to the wrong one (which caused the incident), but it is still a mutation. Consider adding it to the matcher or documenting why it is excluded.Text truncation is byte-level, not character-level:
head -c 80truncates at 80 bytes, which can split multi-byte UTF-8 characters. Since this only affects the human-readable permission prompt and GroupMe messages are typically ASCII-heavy, this is cosmetic at worst.No negative assertion in truncation test: Test 5 asserts the truncated prefix is present but does not assert the tail end of the long text is absent. An
assert_not_containsfor a string that appears only after byte 80 would strengthen the test.PR body test count mismatch: Body says "18 assertions across 6 test cases" but the actual file contains 21 assertions across 7 test cases. Should be updated for accuracy.
SOP COMPLIANCE
160-block-groupme-send-hookreferences #160)Closes #160)PROCESS OBSERVATIONS
Incident-driven hook -- fast response to the 2026-03-25 data exposure. The pattern is well-established (
block-mcp-merge.sh) and this hook follows it faithfully. The deploy ordering note in the PR body (groupme-mcp name-based resolution deploys first, then this hook) shows awareness of the dependency chain.DORA impact: This directly reduces Change Failure Rate by adding a human-in-the-loop gate for GroupMe write operations. The
askpermission decision means no latency added to read-only workflows.VERDICT: APPROVED