refactor(email-core,provider-microsoft,provider-gmail,email-mcp): harden download_attachment (#68)#69
Merged
stevenobiajulu merged 1 commit intomainfrom May 2, 2026
Conversation
…den download_attachment Addresses all 5 items from peer review of #67 (issue #68): 1. MCP transport: download_attachment now returns embedded `resource` content (typed binary blob) alongside text metadata, instead of stuffing base64 inside the JSON-stringified text envelope. URI scheme: attachment://<mailbox>/<messageId>/<attachmentId>. 2. URL encoding: added `encodeGraphPathId` helper in the Microsoft provider and applied it at every path-segment ID interpolation (15+ sites). Real Graph IDs containing `/`, `+`, `=` no longer 400/404 the request. 3. Base64 validation: Microsoft provider now strips whitespace, validates the base64 character set, and compares decoded length against Graph's reported `size`. Mismatches throw rather than silently returning truncated bytes. 4. original_filename: list_attachments and download_attachment outputs now carry both `filename` (sanitized) and `original_filename` (raw). Preserves non-ASCII names like `Räsumé.pdf` for international users. 5. Provider contract reshape: EmailAttachmentHandler.downloadAttachment now returns `{content, filename, mimeType, size}` instead of just `Buffer`. Single round-trip; fresh metadata on every call. Race-deleted attachments surface as ATTACHMENT_NOT_FOUND via new `AttachmentNotFoundError` sentinel (mapped from Graph 404 and Gmail 404). Gmail provider also gets filename-fallback parity with collectPayloadContent (filename → Content-ID → attachment-<path>) so CID-only inline parts don't regress to empty names. Tested: 610 passing (was 580). Added URL-encoding tests for downloadAttachment, listAttachments, getMessage; malformed-base64 + size-mismatch tests; Gmail 404 mapping (message + attachment endpoints); CID and bare-attachment filename fallbacks; non-ASCII filename round-trip; new MCP resource-content shape. Pre-PR checks pass: build, lint, test:run, check:spec-coverage (186/186), check:server-json, check:gemini-extension-manifest. Reviewed by Gemini and Codex via /peer-review. Both pre-implementation (framing scope) and post-implementation (catching the base64-whitespace bug and the Gmail 404 / filename-fallback gaps). Closes: #68 Ref: #67
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardening pass on the
download_attachmentMCP tool added in #67. Addresses all 5 items raised by Gemini + Codex peer review (issue #68).download_attachmentreturns[text, resource]content array — bytes flow as a typedresource.blobwith URIattachment://<mailbox>/<messageId>/<attachmentId>encodeGraphPathIdhelper applied at every path-segment ID interpolation (15+ sites); folder names toosizemismatch checklist_attachmentsanddownload_attachmentnow return bothfilename(sanitized) andoriginal_filename(raw)downloadAttachmentprovider contract too narrow{content, filename, mimeType, size}— single round-trip, fresh metadata, race-deleted attachments map cleanly toATTACHMENT_NOT_FOUNDvia newAttachmentNotFoundErrorsentinel (Graph 404 + Gmail 404 both mapped)Gmail provider also gets filename-fallback parity with
collectPayloadContent(filename → Content-ID →attachment-<path>) so CID-only inline parts don't regress to empty names after the contract reshape.Test plan
npm run build— clean across all 4 packagesnpm run lint --workspaces --if-present— cleannpm run test:run— 610 passing (was 580 before; +30 added)npm run check:spec-coverage— 186/186npm run check:server-json,check:gemini-extension-manifest— passNew tests
downloadAttachment,listAttachments,getMessage(cross-method verification with/,+,=); malformed-base64 →GraphApiError; size-mismatch →GraphApiError; 404 →AttachmentNotFoundError; happy-path uses new return shape.AttachmentNotFoundError; 404 on message endpoint (forpart:*synthetic ids) →AttachmentNotFoundError; non-404 errors propagate unchanged; CID-only inline part filename fallback; bare-attachmentattachment-<path>fallback.original_filename; explicitAttachmentNotFoundErrormapping; updated existing tests for new contract shape.download_attachmentemits text + resource content items on success; failure path stays single text envelope.Reviewed by
Gemini and Codex via
/peer-reviewCLI, both pre-implementation (scope framing) and post-implementation. Critical fixes from the post-implementation review:contentBytes(whitespace now stripped before validation)getMessage+getAttachment404 mapping added)collectPayloadContentrestored)Both reviewers endorsed keeping:
AttachmentNotSupportedErrorsentinel, ODatamicrosoft.graph.fileAttachment/contentBytescast, runtimeNOT_SUPPORTEDfor providers without the capability, and thehandleToolCallstring special-case fordownload_attachment(acceptable for v1.1; refactor to a generic content-formatter hook deferred).Closes #68. Ref #67.