Conversation
pmbstyle
left a comment
There was a problem hiding this comment.
PR Review: Add full Gmail connector tools 👻
Solid work overall. Clean architecture, good test coverage, proper error handling. Here's my analysis:
✅ What I like
Well-layered architecture — MCP server (API client) cleanly separated from connector tools (proxy layer). The _gmail_tool factory pattern is elegant and consistent.
Robust email building — _build_raw_message handles multipart (text+HTML), proper threading headers (In-Reply-To, References), address normalization with deduplication. Good stuff.
Error handling — GmailApiError with structured reason/message/details, graceful non-JSON fallback, 401 auto-retry with token refresh.
Test coverage — Unit tests for all helpers + integration tests for API methods with mock HTTP clients. The BytesParser roundtrip test for _build_raw_message is especially nice.
Input bounds — max_results capped at 25, batch_get_messages limited to 25 IDs. Smart.
⚠️ Issues worth addressing
1. json parameter shadows built-in module
In GmailApiClient._request(), the json kwarg shadows the json module imported at the top of the file. This works because Python closures, but it's a bug magnet:
async def _request(self, method, path, *, params=None, json=None):Suggest renaming to json_body or json_payload.
2. list_messages auto-hydrates N+1
When list_messages returns 25 message IDs, it immediately fires 25 parallel get_message calls via asyncio.gather. For max_results=25 that's 26 API calls in one method. This could:
- Hit Gmail API rate limits (especially in batch scenarios)
- Make it impossible to get just the IDs without hydration
Consider: add a hydrate=True parameter, or return IDs by default and let callers hydrate as needed.
3. _resolve_label_id has no caching
Every add_label_by_name / remove_label_by_name call fetches ALL labels to resolve the name→ID mapping. If an agent does 5 label operations, that's 5 full label list fetches. A simple LRU cache (or even a dict with TTL) would help.
4. delete_message is permanent with no safety net
The tool permanently deletes email. Even at the tool schema level, there's no confirmation mechanism. Consider:
- Adding a
confirmboolean parameter that must betrue - Or at minimum, return a warning in the description
5. Version skip: 0.4.6 → 0.4.8
Skipping 0.4.7 — intentional or leftover from a squash?
💡 Nice-to-haves (non-blocking)
gmail_send_messageschema has no max length onto/cc/bccarrays — consider capping at ~50 recipients_extract_bodydoesn't handlecharsetparameter in Content-Type — could garble non-UTF8 emails- No
draftsupport — would be useful for review-before-send workflows
Verdict: Ship it ✅
The issues above are follow-ups, not blockers. The core is solid. Well done.
Summary
Testing