pal-e-docs

Review: Hook + MCP fix: GroupMe name-based resolution + user approval (re-review)

review-357-2026-03-25-v2 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review of board item #357 after scope expansion (comment #7158). Original review: review-357-2026-03-25. The two issues from the first review (all group-scoped tools need name resolution; hook must handle different tool_input shapes) are now addressed. Three new issues found.

Template Completeness

  • [x] Lineage
  • [x] Repo
  • [x] User Story
  • [x] Context
  • [x] File Targets (updated in scope expansion comment)
  • [x] Acceptance Criteria (updated in scope expansion comment)
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related

File Targets

  • [x] hooks/block-groupme-send.sh (NEW) -- confirmed does not exist yet. Pattern file block-mcp-merge.sh verified at claude-custom/hooks/block-mcp-merge.sh (hardlinked to ~/.claude/hooks/). 19-line shell script, clean pattern to follow.
  • [x] settings.json -- verified at claude-custom/settings.json. PreToolUse section exists (lines 50-163). No existing GroupMe matcher. New entry will follow the mcp__forgejo__merge_approved_pr pattern.
  • [x] src/groupme_mcp/tools/messages.py -- verified. send_message takes group_id: str at line 14. Ready for interface change.
  • [x] src/groupme_mcp/tools/members.py -- verified. add_member (line 13), remove_member (line 50), list_members (line 66) all take group_id: str. Ready for interface change.
  • [x] src/groupme_mcp/server.py -- verified. Good location for shared _resolve_group() helper. Currently has get_client(), _error_response(), _ok() helpers. Helper fits the established pattern.
  • [ ] src/groupme_mcp/tools/groups.py -- MISSING from scope. get_group (line 42) also takes raw group_id. See Blast Radius.

Repo Placement

OK. Issue filed on forgejo_admin/claude-custom for hook + settings.json. Checklist correctly identifies two PRs: one on claude-custom, one on groupme-mcp. Both repos exist and are accessible.

Dependencies

No blocking dependencies on the board. Item #357 is in todo with no depends: labels. No related items in in_progress.
Undocumented deployment dependency: The groupme-mcp PR must merge and deploy before the claude-custom PR, because the hook extracts group_name from tool_input -- if the hook deploys while the MCP tools still expose group_id, the permission prompt will show an empty group name.

Acceptance Criteria

Updated criteria from scope expansion are testable. Test commands are real (cd ~/groupme-mcp && pytest tests/). Existing tests confirmed: test_messages.py (3 tests), test_members.py (8 tests), test_groups.py (8 tests) -- all currently pass group_id and will need updating.
Two gaps:
  • Hook unit test dropped. The original issue body's Test Expectations included "Hook test: verify JSON output has permissionDecision: "ask"." The scope expansion's updated acceptance criteria omitted it. Should be restored.
  • No direct test for _resolve_group() helper. Each tool's tests will exercise it implicitly, but an explicit unit test for the ambiguous-name and no-match paths would increase confidence and catch regressions.

Blast Radius

  • get_group is the 5th tool using raw group_id. The scope expansion targets 4 tools but get_group in groups.py (line 42) also takes group_id. An agent could still call get_group(group_id="113983384") with a stale ID. Lower risk (read-only, no message sent to wrong group), but the stale-ID problem remains. Either include it or document the exclusion.
  • Hook excludes list_members -- correct but undocumented. Scope expansion adds list_members to name-based resolution but not to the hook matcher (write tools only: send_message, add_member, remove_member). This is correct -- list_members is read-only. The ticket should state this explicitly so the implementing agent does not add it to the matcher.
  • SDK layer unchanged -- confirmed. All SDK methods (send_message, add_member, remove_member, list_members, get_group) keep their group_id parameter. Resolution is purely MCP-layer. No downstream SDK consumers affected.

Recommendation

Three items before READY:
  • Decide on get_group -- add it to name-based resolution scope (update File Targets to include groups.py), or add an explicit exclusion note with rationale. The stale-ID vulnerability applies to all 5 tools that accept group_id.
  • Restore hook test expectation -- the scope expansion dropped the hook unit test from the original issue body. Re-add to Test Expectations or Updated Acceptance Criteria.
  • Document deployment ordering -- add a Constraints bullet: "groupme-mcp PR must merge and deploy before claude-custom PR, because the hook expects group_name in tool_input."
Minor (non-blocking): explicitly note that list_members is excluded from the hook matcher because it is read-only.