pal-e-docs

Review: Hook + MCP fix: GroupMe send_message requires name-based resolution + user approval

review-357-2026-03-25 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related
  • [x] Type (extra section, fine)
All required template sections present. Well-written incident context with root cause analysis.

File Targets

  • [x] hooks/block-groupme-send.sh — NEW file. Reference pattern hooks/block-mcp-merge.sh confirmed at /home/ldraney/claude-custom/hooks/block-mcp-merge.sh (19 lines, uses jq + permissionDecision: "ask"). Pattern is solid and appropriate.
  • [x] settings.json — Confirmed at /home/ldraney/claude-custom/settings.json. PreToolUse section exists (lines 50-163) with clear pattern for adding new matcher entries. No existing GroupMe matcher present.
  • [x] src/groupme_mcp/tools/messages.py — Confirmed at /home/ldraney/groupme-mcp/src/groupme_mcp/tools/messages.py. Currently accepts group_id: str parameter (line 14). send_message calls get_client().send_message(group_id=group_id, text=text).
  • [x] list_groups() function confirmed in src/groupme_mcp/tools/groups.py (line 29) — calls get_client().list_groups() with no arguments. Available for live resolution.
  • [x] Tests directory confirmed: tests/test_messages.py, tests/test_groups.py, tests/test_members.py exist.
  • [ ] add_member and remove_member — ISSUE: Both also take raw group_id (confirmed in members.py lines 14 and 51). The hook covers all three tools, but the MCP interface fix in File Targets only mentions messages.py. Same stale-ID vulnerability exists for member operations. See Blast Radius.

Repo Placement

OK. Two repos correctly identified:
  • forgejo_admin/claude-custom — hook + settings.json (confirmed on Forgejo)
  • forgejo_admin/groupme-mcp — tool interface change (confirmed on Forgejo)
Forgejo issue is filed on claude-custom. Since work spans two repos, the checklist correctly calls for two PRs. The groupme-sdk exclusion is correct — SDK keeps raw group_id, MCP resolves above it.
Note: The Forgejo issue lives on claude-custom only. A second Forgejo issue on groupme-mcp would improve traceability for the MCP-side PR, but is not strictly required since the checklist already calls out both PRs.

Dependencies

  • No blocking dependencies found on the board. Board item #357 is in todo column with no depends: label.
  • Board item #98 (Context intelligence) is in_progress — no conflict.
  • The groupme-sdk package must already be published to Forgejo PyPI for groupme-mcp to import list_groups from it. The ticket's Constraints section notes "Forgejo PyPI for groupme-sdk dependency" — this is documented.
  • No undocumented cross-repo dependencies found.

Acceptance Criteria

  • [x] "send_message accepts group_name, not raw group_id" — Testable, clear.
  • [x] "Ambiguous or missing group name returns helpful error" — Testable.
  • [x] "PreToolUse hook fires on every send_message, add_member, remove_member" — Testable via hook test.
  • [x] "Hook shows group name and message text in permission prompt" — Testable, but note: add_member/remove_member don't have a text field. Hook extraction logic needs to handle different tool_input shapes.
  • [ ] "Hook works under --dangerously-skip-permissions" — ISSUE: This criterion is misleading. Claude Code hooks ALWAYS run regardless of permission flags — skipDangerousModePermissionPrompt only skips the dangerous-mode confirmation, not hooks. The criterion is technically true by default, but suggests the agent needs to verify something special. Recommend rewording to: "Hook fires even when skipDangerousModePermissionPrompt is true in settings.json" or removing it (it's inherent to hook semantics).
  • [x] "Forked/resumed sessions cannot bypass the gate" — True by design (hooks are settings.json-level, not session-level).

Blast Radius

  • add_member and remove_member also take raw group_id. The hook gates all three (good), but the MCP interface fix only targets send_message. If the goal is to eliminate stale-ID risk, add_member, remove_member, and list_members should also resolve by name. An agent could still call add_member(group_id="113983384") with a wrong ID — the hook would prompt, but the prompt would show a raw ID instead of a name, defeating the "name-based resolution" goal.
  • get_group also takes raw group_id (line 43 of groups.py) — lower risk since it's read-only, but inconsistent if send_message moves to name-based.
  • Hook tool_input extraction must handle multiple shapes: send_message has group_name + text; add_member has group_name + nickname + contact fields; remove_member has group_name + membership_id. The ticket's hook description only mentions extracting group_name and text.
  • Existing tests will break. tests/test_messages.py calls send_message(group_id="12345", text=...) in 3 tests. These need updating as part of the MCP change.

Recommendation

Two issues to address before READY:
  • Extend name-based resolution to add_member and remove_member (not just send_message). Add to File Targets: members.py — MODIFY to accept group_name instead of group_id. Alternatively, scope this as a deliberate Phase 2 follow-up and document the decision in the ticket.
  • Update hook description to note it must handle different tool_input shapes (not all tools have a text field). The hook should extract group_name from all three tools, and show context-appropriate details (text for messages, nickname for add_member, membership_id for remove_member).
Minor nit: Reword or remove the "Hook works under --dangerously-skip-permissions" acceptance criterion — it's inherently true for all hooks and may confuse the implementing agent.