Skip to content

Fix MCP tool discovery for attachment filename schemas - #136

Open
jefflaporte wants to merge 1 commit into
HQBase:mainfrom
jefflaporte:fix/mcp-tool-discovery
Open

jefflaporte wants to merge 1 commit into
HQBase:mainfrom
jefflaporte:fix/mcp-tool-discovery

Conversation

@jefflaporte

@jefflaporte jefflaporte commented Oct 4, 2026 •

Copy link
Copy Markdown

Summary

Closes #135.

ChatGPT can complete OAuth for /mcp/full but discover no tools because add_draft_attachment exports a filename pattern containing \p{Cc} without its required JavaScript Unicode flag. Python regex validators reject the escape, and JavaScript matching without the flag rejects valid filenames.

Replace the property escape with explicit C0/C1 control ranges. This preserves filename validation, including Unicode filenames, while making the exported pattern usable without regex flags. Add a regression test against the actual tools/list response.

Verification

  • pnpm check — 965 unit tests and 225 Worker integration tests passed; two existing tests were skipped. Local Node 26 used NODE_OPTIONS=--no-experimental-webstorage to avoid its native Web Storage conflict with browser tests.
  • pnpm deploy:dry-run.
  • The new regression test failed before the fix and passed afterward. It covers Unicode filenames, forbidden separators and quotes, and every C0/C1 control character.
  • Confirmed on a contributor-owned Cloudflare deployment: ChatGPT discovered all 21 tools after the fix.

Notes

The Biome exception is limited to the intentional control-range exclusion and includes an explanation. Scope is hqbase only; this restores the existing filename contract and does not change storage or authorization. Official HQBase staging has not been run.

Summary by CodeRabbit

  • Bug Fixes
    • Draft attachment filenames now support accented, CJK, and emoji characters while rejecting path separators, quotes, and disallowed control characters.

Use explicit C0 and C1 control ranges so the exported JSON Schema filename pattern works without Unicode regex flags. Preserve Unicode filenames and rejection of unsafe filename characters, and cover the tools/list response with a regression test.
@CLAassistant

CLAassistant commented Oct 4, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9bbb9d45-aa21-4640-aa00-b338f802caad
📥 Commits

Reviewing files that changed from the base of the PR and between a1161f1 and 68d3a34.

📒 Files selected for processing (2)
  • test/integration/worker/mcp.test.ts
  • worker/features/mcp/draft-tools.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

The add_draft_attachment filename pattern now explicitly rejects C0 and C1 control characters. An integration test checks the exported pattern against Unicode filenames, path separators, quotes, and control characters.

Changes

Attachment filename validation

Layer / File(s) Summary
Filename pattern and integration test
worker/features/mcp/draft-tools.ts, test/integration/worker/mcp.test.ts
The pattern replaces the Unicode Cc category with explicit C0 and C1 ranges. The integration test checks valid Unicode filenames and rejects separators, quotes, and control characters.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: bermanto

Merge Risk: ⚪ Minimal · up to 68d3a

No actionable issue remains from this review. Merge after normal checks; official staging validation has not been reported.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 68d3a

The corrected pattern preserves filename restrictions and restores discovery behind existing authentication and draft-access controls. No material security risk introduced or worsened by this change was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — Restored discovery makes existing tools usable by compatible authorized clients; it does not create an unauthenticated attachment path or enlarge attachment ownership. Attachment mutations remain limited by mail:send scope, live draft-access checks, and principal-scoped persistence.

Trust Boundaries and Controls

  • observed — The MCP route authenticates before serving tool requests. Draft tools require mail:send, and attachment creation rechecks draft access before storage. Existing integration assertions also reject use of a read-only token against /mcp/full.

Resilience and Maintainability Implications

  • observed — The existing attachment operation creates metadata, writes the object, verifies the ownership-linked record, and attempts record and object deletion on caught failure. Additions remain explicitly non-idempotent. The PR does not alter these transitions; runtime concurrency guarantees and recovery after interruption or failed cleanup remain unverified.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the MCP tool-discovery issue and the attachment filename schema change.
Description check ✅ Passed The description includes the required Summary, Verification, and Notes sections. It explains the change, reports test and deployment checks, and notes that official staging was not run.
Linked Issues check ✅ Passed Issue #135 requires all 21 tools to be discoverable through /mcp/full and a portable attachment filename pattern. worker/features/mcp/draft-tools.ts replaces \\p{Cc} with explicit C0/C1, separato…
Out of Scope Changes check ✅ Passed The production change is limited to the filename pattern for add_draft_attachment. The integration test supports issue #135 by checking the exported schema pattern and tool discovery. No unrelated c…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChatGPT discovers no MCP Mail actions tools due to incompatible filename regex

2 participants