forgejo_admin/groupme-mcp

Fork 0

feat: name-based group resolution on all group-scoped tools #4

Merged

forgejo_admin merged 1 commit from 3-name-based-group-resolution into main

2026-03-26 03:51:12 +00:00

forgejo_admin commented

2026-03-26 03:43:26 +00:00

Owner

Summary

Replaces raw group_id parameters with group_name on all 5 group-scoped MCP tools, eliminating the class of error where stale or wrong IDs route messages to the wrong group. A shared _resolve_group() helper resolves names against the live GroupMe API with pagination support.

Changes

src/groupme_mcp/server.py: Added _resolve_group(group_name) helper with paginated list_groups(per_page=100), case-insensitive exact matching, and clear error messages for no-match/ambiguous cases
src/groupme_mcp/tools/messages.py: send_message now accepts group_name; response includes resolved group name, member names, and member count
src/groupme_mcp/tools/members.py: add_member, remove_member, list_members now accept group_name instead of group_id
src/groupme_mcp/tools/groups.py: get_group now accepts group_name instead of group_id
tests/conftest.py: Updated fixture to patch _resolve_group in all tool modules
tests/test_resolve_group.py: New test file with 8 tests covering exact match, case-insensitive match, no-match error, ambiguous error, and pagination
tests/test_messages.py, tests/test_members.py, tests/test_groups.py: Updated all calls to use group_name

Test Plan

28 tests pass (pytest tests/ -v)
ruff format and ruff check clean
Unit tests cover: exact name resolution, case-insensitive matching, no-match with available names listing, ambiguous match with options, pagination beyond first page, pagination stop on incomplete/empty page
Manual verification: deploy and confirm send_message(group_name="Westside Stakeholders", ...) resolves correctly

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #3
claude-custom#160 -- companion PreToolUse hook (depends on this deploying first)

## Summary Replaces raw `group_id` parameters with `group_name` on all 5 group-scoped MCP tools, eliminating the class of error where stale or wrong IDs route messages to the wrong group. A shared `_resolve_group()` helper resolves names against the live GroupMe API with pagination support. ## Changes - `src/groupme_mcp/server.py`: Added `_resolve_group(group_name)` helper with paginated `list_groups(per_page=100)`, case-insensitive exact matching, and clear error messages for no-match/ambiguous cases - `src/groupme_mcp/tools/messages.py`: `send_message` now accepts `group_name`; response includes resolved group name, member names, and member count - `src/groupme_mcp/tools/members.py`: `add_member`, `remove_member`, `list_members` now accept `group_name` instead of `group_id` - `src/groupme_mcp/tools/groups.py`: `get_group` now accepts `group_name` instead of `group_id` - `tests/conftest.py`: Updated fixture to patch `_resolve_group` in all tool modules - `tests/test_resolve_group.py`: New test file with 8 tests covering exact match, case-insensitive match, no-match error, ambiguous error, and pagination - `tests/test_messages.py`, `tests/test_members.py`, `tests/test_groups.py`: Updated all calls to use `group_name` ## Test Plan - [x] 28 tests pass (`pytest tests/ -v`) - [x] `ruff format` and `ruff check` clean - [x] Unit tests cover: exact name resolution, case-insensitive matching, no-match with available names listing, ambiguous match with options, pagination beyond first page, pagination stop on incomplete/empty page - [ ] Manual verification: deploy and confirm `send_message(group_name="Westside Stakeholders", ...)` resolves correctly ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #3 - `claude-custom#160` -- companion PreToolUse hook (depends on this deploying first)

forgejo_admin added 1 commit

2026-03-26 03:43:26 +00:00

feat: replace group_id with group_name on all group-scoped tools 3373fff4fb

Adds _resolve_group() helper that resolves group names against the live
GroupMe API (paginated, per_page=100). All 5 group-scoped tools
(send_message, add_member, remove_member, list_members, get_group) now
accept group_name instead of raw group_id, eliminating wrong-group
errors from stale IDs. send_message response now includes resolved group
name, member names, and member count for confirmation.

Closes #3

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-26 03:44:16 +00:00

Author

Owner

PR Review -- forgejo_admin/groupme-mcp#4

Acceptance Criteria Verification

Criteria	Status
All 5 group-scoped tools accept `group_name`	PASS
Shared `_resolve_group()` in `server.py`	PASS
`_resolve_group()` paginates with `per_page=100`	PASS
Ambiguous/missing name returns helpful error with available groups	PASS
`send_message` response includes group name + member names + count	PASS
Existing tests updated for new interface	PASS

Test Expectations Verification

Test	Status
`send_message` with exact group name resolves correctly	PASS
`add_member` with exact group name resolves correctly	PASS
Ambiguous name returns multiple options without sending	PASS
No match returns error with available group names	PASS
`_resolve_group` handles pagination (>10 groups)	PASS
28 tests pass, ruff clean	PASS

Code Quality

Clean separation: _resolve_group in server.py, consumed by all tool modules
Pagination loop correctly stops on empty page or incomplete page (<100 results)
Case-insensitive exact matching prevents case-mismatch errors
Test fixture design is sound: tool-level tests patch _resolve_group to avoid live API; test_resolve_group.py patches get_client at server level to test the helper directly
No secrets, no unrelated changes, SDK layer untouched

Findings

No issues found.

VERDICT: APPROVED

## PR Review -- forgejo_admin/groupme-mcp#4 ### Acceptance Criteria Verification | Criteria | Status | |----------|--------| | All 5 group-scoped tools accept `group_name` | PASS | | Shared `_resolve_group()` in `server.py` | PASS | | `_resolve_group()` paginates with `per_page=100` | PASS | | Ambiguous/missing name returns helpful error with available groups | PASS | | `send_message` response includes group name + member names + count | PASS | | Existing tests updated for new interface | PASS | ### Test Expectations Verification | Test | Status | |------|--------| | `send_message` with exact group name resolves correctly | PASS | | `add_member` with exact group name resolves correctly | PASS | | Ambiguous name returns multiple options without sending | PASS | | No match returns error with available group names | PASS | | `_resolve_group` handles pagination (>10 groups) | PASS | | 28 tests pass, ruff clean | PASS | ### Code Quality - Clean separation: `_resolve_group` in server.py, consumed by all tool modules - Pagination loop correctly stops on empty page or incomplete page (<100 results) - Case-insensitive exact matching prevents case-mismatch errors - Test fixture design is sound: tool-level tests patch `_resolve_group` to avoid live API; `test_resolve_group.py` patches `get_client` at server level to test the helper directly - No secrets, no unrelated changes, SDK layer untouched ### Findings No issues found. **VERDICT: APPROVED**

forgejo_admin commented

2026-03-26 03:47:23 +00:00

Author

Owner

PR #4 Review

DOMAIN REVIEW

Tech stack: Python 3.12+ / FastMCP / Pydantic / pytest / groupme-sdk. Ruff for linting. Hatchling build. No CI pipeline defined in-repo (no .woodpecker.yml).

Python / MCP quality:

_resolve_group() is a well-designed shared helper with proper pagination, case-insensitive matching, and clear error messages for zero-match and ambiguous-match cases. Good use of ValueError for domain errors.
All 5 group-scoped tools (get_group, send_message, add_member, remove_member, list_members) correctly resolve by name via the shared helper -- no DRY violation.
Type hints are present and correct throughout (list[dict], dict, str | None, Annotated).
Docstrings follow PEP 257 conventions on all public functions and the helper.
The _resolve_group pattern of resolve-then-use-id is clean: resolution happens once at the top of each tool function, the SDK call still uses the raw group_id underneath.
from __future__ import annotations used consistently across all modules.
Error handling pattern is consistent: every tool wraps its body in try/except Exception and delegates to _error_response(), which produces structured JSON for both GroupMeHTTPError and generic exceptions.

Response enrichment on send_message: The enriched response now returns resolved_group.name, resolved_group.id, resolved_group.member_count, and resolved_group.member_names alongside the message data. This is a good UX improvement for agent consumers -- it provides confirmation of which group received the message and who is in it.

Pagination correctness: The pagination loop in _resolve_group (lines 41-48 of server.py) correctly handles three termination conditions: empty batch, batch smaller than page size, and implicit continuation when batch is exactly 100. All three paths are tested.

BLOCKERS

None.

Test coverage: 8 new tests for _resolve_group (exact match, case-insensitive, no-match error, no-match lists available, ambiguous, pagination beyond page 1, stops on incomplete page, stops on empty page). All existing tool tests updated to use group_name. 28 total tests reported passing. Coverage is thorough across happy path, error handling, and edge cases.
Input validation: group_name is a required str parameter on all tools (enforced by Pydantic/FastMCP). The _resolve_group helper validates against live API data. No raw user input reaches SQL or shell.
Secrets: No credentials in code. GroupMeClient() reads GROUPME_ACCESS_TOKEN from environment (handled by the SDK).
DRY: Single _resolve_group helper, no duplicated resolution logic across tools.

NITS

conftest.py _make_resolve_group ignores group_name argument: The stub returned by _make_resolve_group accepts group_name but always returns the same FAKE_GROUP regardless. This is fine for current tests (they all use "Westside Stakeholders" which matches the fixture), but it means the tests do not verify that the correct group name is being passed from the tool to _resolve_group. A more precise stub could assert on the received group_name. Low priority -- the test_resolve_group.py tests cover the real function behavior thoroughly.
per_page=100 magic number: The pagination page size 100 appears twice in _resolve_group (lines 42 and 46 of server.py). Could be extracted to a module-level constant (e.g., _GROUPS_PAGE_SIZE = 100) for clarity. Very minor.
get_group does a double-fetch: get_group calls _resolve_group (which calls list_groups to find the group), then calls get_client().get_group(group_id=...) to fetch it again. The first call already returned the group dict. For the other tools this pattern is necessary (they need the group_id to perform a different operation), but get_group could just return the resolved dict directly. The extra API call is not wrong -- get_group may return fresher/more-detailed data -- but it is worth noting as a minor inefficiency.
No CI pipeline: There is no .woodpecker.yml or equivalent CI config in this repo. Tests are run manually. Not a blocker for this PR, but worth tracking as a future item.

SOP COMPLIANCE

Branch named after issue: 3-name-based-group-resolution references issue #3
PR body has Summary, Changes, Test Plan, Related sections
Related section references parent issue: Closes #3
Related section references companion work: claude-custom#160
No secrets committed
No unnecessary file changes -- all 9 changed files are directly related to the feature
Commit messages are descriptive (based on PR title)
No plan slug referenced in Related -- acceptable for a standalone repo without a pal-e-docs plan

PROCESS OBSERVATIONS

DORA / Deployment Frequency: This is a standalone MCP server repo with no CI pipeline. Deployment is manual. Adding a .woodpecker.yml would improve DF and reduce change failure risk.
Change Failure Risk: Low. The API surface change (group_id -> group_name) is breaking for any existing callers, but the PR body notes a companion hook in claude-custom#160 that depends on this deploying first. The sequencing is documented.
Test quality: The test suite is well-structured with proper separation between unit tests for the resolver (test_resolve_group.py with its own mock_client fixture) and integration-style tests for tool functions (shared conftest.py fixture). The conftest properly patches _resolve_group in each tool module's namespace, preventing live API calls.

VERDICT: APPROVED

## PR #4 Review ### DOMAIN REVIEW **Tech stack**: Python 3.12+ / FastMCP / Pydantic / pytest / groupme-sdk. Ruff for linting. Hatchling build. No CI pipeline defined in-repo (no `.woodpecker.yml`). **Python / MCP quality**: - `_resolve_group()` is a well-designed shared helper with proper pagination, case-insensitive matching, and clear error messages for zero-match and ambiguous-match cases. Good use of `ValueError` for domain errors. - All 5 group-scoped tools (`get_group`, `send_message`, `add_member`, `remove_member`, `list_members`) correctly resolve by name via the shared helper -- no DRY violation. - Type hints are present and correct throughout (`list[dict]`, `dict`, `str | None`, `Annotated`). - Docstrings follow PEP 257 conventions on all public functions and the helper. - The `_resolve_group` pattern of resolve-then-use-id is clean: resolution happens once at the top of each tool function, the SDK call still uses the raw `group_id` underneath. - `from __future__ import annotations` used consistently across all modules. - Error handling pattern is consistent: every tool wraps its body in `try/except Exception` and delegates to `_error_response()`, which produces structured JSON for both `GroupMeHTTPError` and generic exceptions. **Response enrichment on `send_message`**: The enriched response now returns `resolved_group.name`, `resolved_group.id`, `resolved_group.member_count`, and `resolved_group.member_names` alongside the message data. This is a good UX improvement for agent consumers -- it provides confirmation of which group received the message and who is in it. **Pagination correctness**: The pagination loop in `_resolve_group` (lines 41-48 of `server.py`) correctly handles three termination conditions: empty batch, batch smaller than page size, and implicit continuation when batch is exactly 100. All three paths are tested. ### BLOCKERS None. - **Test coverage**: 8 new tests for `_resolve_group` (exact match, case-insensitive, no-match error, no-match lists available, ambiguous, pagination beyond page 1, stops on incomplete page, stops on empty page). All existing tool tests updated to use `group_name`. 28 total tests reported passing. Coverage is thorough across happy path, error handling, and edge cases. - **Input validation**: `group_name` is a required `str` parameter on all tools (enforced by Pydantic/FastMCP). The `_resolve_group` helper validates against live API data. No raw user input reaches SQL or shell. - **Secrets**: No credentials in code. `GroupMeClient()` reads `GROUPME_ACCESS_TOKEN` from environment (handled by the SDK). - **DRY**: Single `_resolve_group` helper, no duplicated resolution logic across tools. ### NITS 1. **`conftest.py` `_make_resolve_group` ignores `group_name` argument**: The stub returned by `_make_resolve_group` accepts `group_name` but always returns the same `FAKE_GROUP` regardless. This is fine for current tests (they all use "Westside Stakeholders" which matches the fixture), but it means the tests do not verify that the *correct* group name is being passed from the tool to `_resolve_group`. A more precise stub could assert on the received `group_name`. Low priority -- the `test_resolve_group.py` tests cover the real function behavior thoroughly. 2. **`per_page=100` magic number**: The pagination page size `100` appears twice in `_resolve_group` (lines 42 and 46 of `server.py`). Could be extracted to a module-level constant (e.g., `_GROUPS_PAGE_SIZE = 100`) for clarity. Very minor. 3. **`get_group` does a double-fetch**: `get_group` calls `_resolve_group` (which calls `list_groups` to find the group), then calls `get_client().get_group(group_id=...)` to fetch it again. The first call already returned the group dict. For the other tools this pattern is necessary (they need the `group_id` to perform a different operation), but `get_group` could just return the resolved dict directly. The extra API call is not wrong -- `get_group` may return fresher/more-detailed data -- but it is worth noting as a minor inefficiency. 4. **No CI pipeline**: There is no `.woodpecker.yml` or equivalent CI config in this repo. Tests are run manually. Not a blocker for this PR, but worth tracking as a future item. ### SOP COMPLIANCE - [x] Branch named after issue: `3-name-based-group-resolution` references issue #3 - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related section references parent issue: `Closes #3` - [x] Related section references companion work: `claude-custom#160` - [x] No secrets committed - [x] No unnecessary file changes -- all 9 changed files are directly related to the feature - [x] Commit messages are descriptive (based on PR title) - [ ] No plan slug referenced in Related -- acceptable for a standalone repo without a pal-e-docs plan ### PROCESS OBSERVATIONS - **DORA / Deployment Frequency**: This is a standalone MCP server repo with no CI pipeline. Deployment is manual. Adding a `.woodpecker.yml` would improve DF and reduce change failure risk. - **Change Failure Risk**: Low. The API surface change (group_id -> group_name) is breaking for any existing callers, but the PR body notes a companion hook in `claude-custom#160` that depends on this deploying first. The sequencing is documented. - **Test quality**: The test suite is well-structured with proper separation between unit tests for the resolver (`test_resolve_group.py` with its own `mock_client` fixture) and integration-style tests for tool functions (shared `conftest.py` fixture). The conftest properly patches `_resolve_group` in each tool module's namespace, preventing live API calls. ### VERDICT: APPROVED