fix: attachment route handler signature (Request vs path params)

[dsh]

Summary

The /attachments/{file_id} endpoint returns 404 for all requests because serve_attachment uses a FastAPI-style function signature instead of the Starlette Request pattern required by FastMCP's custom_route.

Problem

FastMCP's custom_route decorator creates Starlette routes that pass a Request object as the first argument to the handler. The current handler signature:

async def serve_attachment(file_id: str):

receives the Request object as file_id, so get_attachment_metadata() is called with a Request object instead of a UUID string, and always fails.

The health_check handler on line 432 correctly uses (request: Request) — this fix applies the same pattern to serve_attachment.

Fix

-async def serve_attachment(file_id: str):
+async def serve_attachment(request):
     """Serve a stored attachment file."""
     from core.attachment_storage import get_attachment_storage

+    file_id = request.path_params["file_id"]
     storage = get_attachment_storage()

Testing

Verified end-to-end in a Docker deployment with --transport streamable-http --tool-tier complete:

1. Called get_gmail_attachment_content → file saved to disk, download URL returned
2. curl on the download URL → valid PDF returned (55 KB, PDF v1.3)

Summary by CodeRabbit

• Refactor
  • Internal improvements to attachment handling to enhance code maintainability while preserving existing functionality and behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

The serve_attachment route handler method signature was refactored to accept a full request object instead of directly receiving a file_id parameter. The file_id is now extracted from request.path_params. All existing logic, response behavior, and error handling remain unchanged.

Changes

Cohort / File(s)	Summary
Route Handler Refactoring core/server.py	Method signature updated from serve_attachment(file_id: str) to async def serve_attachment(request) with file_id extraction from request.path_params. No functional logic changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Poem

🐰 A path was wrapped in request's fold,
Where file_id secrets now unfold,
From simple strings to objects vast,
The logic shines, forever steadfast,
Same service rendered, just reshown! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing the attachment route handler to use Request vs path params pattern.
Description check	✅ Passed	The description covers the problem, fix, and testing but omits required checklist items and the 'Allow edits from maintainers' section from the template.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[dsh]

This patch and comments were AI-generated but human-reviewed and tested.

[coderabbitai]

🧹 Nitpick comments (1)

core/server.py (1)

440-445: Good fix! Consider adding type annotation for consistency.

The fix correctly aligns with FastMCP's custom_route behavior, which passes a Starlette Request object as the first argument. This matches the pattern used by health_check at line 425.

For consistency with other handlers in this file (health_check, legacy_oauth2_callback), consider adding the type annotation:

🔧 Suggested improvement

 `@server.custom_route`("/attachments/{file_id}", methods=["GET"])
-async def serve_attachment(request):
+async def serve_attachment(request: Request):
     """Serve a stored attachment file."""
     from core.attachment_storage import get_attachment_storage

     file_id = request.path_params["file_id"]

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/server.py` around lines 440 - 445, Add the Starlette Request type
annotation to the serve_attachment handler signature to match other handlers
like health_check: update the serve_attachment parameter to accept request:
Request and ensure Request is imported (e.g., from starlette.requests import
Request) at the top of the file if not already present; keep the function name
serve_attachment and its internal logic unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@core/server.py`:
- Around line 440-445: Add the Starlette Request type annotation to the
serve_attachment handler signature to match other handlers like health_check:
update the serve_attachment parameter to accept request: Request and ensure
Request is imported (e.g., from starlette.requests import Request) at the top of
the file if not already present; keep the function name serve_attachment and its
internal logic unchanged.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcb7bad and b34fc8e.

📒 Files selected for processing (1)

• core/server.py

[taylorwilsdon]

Hm, we actually removed request from that very function a few months back because it was erroring for lack of a request object. Personally, I struggle to see the value of rehosting attachments in general - I run stateless mode and when I pull attachments it just links to the attachment directly in gmail, which is more practical and secure especially from an ediscovery/dlp perspective. Clearly though people have a need so appreciate the PR. Seems like you're right and get_attachment_metadata currently receives a Request object, so we need to parse as such. Thanks!

[taylorwilsdon]

Merged with a test and request object typing