Skip to content

Conversation

@mattzcarey
Copy link
Contributor

@mattzcarey mattzcarey commented Nov 14, 2025

part one in a long saga. This moves all the mcp storage functions to the client manager, with an interface to try and make it less platform specific.

also cleans up the _connectMcpServersInternal and mcp.connect into..

  • mcp.registerServer()
  • mcp.connectToServer()

tested with: https://search-mcp.parallel.ai/mcp on https://workers-ai-playground.mattzcarey.workers.dev/

Other fun stuff here:

  • reasoning card for playground
  • updating to qwen 3 as default

@changeset-bot
Copy link

changeset-bot bot commented Nov 14, 2025

🦋 Changeset detected

Latest commit: 0966d6c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
agents Minor
hono-agents Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@claude
Copy link

claude bot commented Nov 14, 2025

Claude Code Review

Architecture Issues

Critical: Race condition in OAuth flow (index.ts:482-487)
The OAuth callback handling fires onServerStateChanged which triggers a broadcast, then establishConnection is called in the background which fires another onServerStateChanged. However, establishConnection can fail after the broadcast, leaving the UI showing stale state. The background connection should complete before broadcasting, or the error state should trigger another broadcast.

Storage timing issue (client.ts:600-602)
clearAuthUrl is called immediately after completeAuthorization, but if establishConnection fails later, the auth_url is already cleared. This breaks reconnection attempts since the system thinks OAuth is complete when it isn't. Consider clearing auth_url only after establishConnection succeeds.

Code Quality

Missing error handling in restoreConnectionsFromStorage (client.ts:223)
Connection restoration catches errors but continues silently. Failed restorations should be tracked so the UI can show which servers need attention.

Inconsistent state machine (client.ts:160-196)
The logic skips servers in various states but doesn't document why. A server in "authenticating" state during restore likely indicates an incomplete OAuth flow that should be cleaned up or retried.

Type safety gap (client-storage.ts:79-85)
AgentMCPClientStorage constructor takes sql with a signature that returns T[], but the SQL operations don't validate that the template strings are actually SQL. Consider a more type-safe SQL builder or at least document the expected usage pattern.

Testing Coverage

The test file (client-manager.test.ts) has grown significantly (+1757 lines) but I cannot verify test quality without seeing the full file. Ensure tests cover:

  • The OAuth race condition scenarios
  • Failed connection restoration
  • Concurrent registerServer/connectToServer calls
  • Storage adapter error cases

Minor Issues

  • client.ts:166-167: Log message could be more actionable - suggest what user should do
  • index.ts:1322: mcp.dispose() called but no check if already disposed
  • client-storage.ts:22,26: Return types use Promise<T | undefined> | undefined - this dual async/sync pattern is confusing, pick one

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 14, 2025

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/agents@652

commit: 0966d6c

threepointone pushed a commit to cloudflare/cloudflare-docs that referenced this pull request Nov 14, 2025
This PR introduces a storage adapter architecture for the MCP client manager,
allowing custom storage backends for MCP server configurations.

Changes:
- Created advanced-client-api.mdx documenting the MCPStorageAdapter interface
- Added security section to oauth-mcp-client.mdx about callback URL clearing
- Updated mcp-client-api.mdx to link to advanced configuration guide

These docs explain the new storage adapter pattern, OAuth security improvements,
and provide examples for implementing custom storage backends.

Related: cloudflare/agents#652
threepointone pushed a commit to cloudflare/cloudflare-docs that referenced this pull request Nov 14, 2025
Documents the MCP client manager refactoring that introduces a storage
adapter interface and implements automatic OAuth credential cleanup for
enhanced security.

Changes:
- Add new mcp-storage.mdx explaining storage adapter architecture
- Document automatic OAuth credential cleanup after authentication
- Add security notes about replay attack prevention
- Document automatic connection restoration after hibernation
- Update OAuth guide with security information

Related PR: cloudflare/agents#652

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@threepointone
Copy link
Contributor

📚 Documentation Sync

I've created a documentation PR to sync these changes to cloudflare-docs:

Docs PR: cloudflare/cloudflare-docs#26535

Documentation Changes

This PR introduces a storage adapter architecture for the MCP client manager. The documentation includes:

  1. New Advanced Configuration Guide (advanced-client-api.mdx)

    • Explains the MCPStorageAdapter interface
    • Provides examples for implementing custom storage backends (KV, R2, external databases)
    • Documents the connection restoration process after hibernation
    • Details performance considerations and caching strategies
  2. OAuth Security Section (updated oauth-mcp-client.mdx)

    • Documents the security improvement where callback URLs are cleared after successful OAuth
    • Explains state parameter validation
    • Notes on secure token storage
  3. API Reference Updates (updated mcp-client-api.mdx)

    • Added link to advanced configuration guide
    • Noted the modular storage adapter architecture

Why These Docs Matter

This refactoring introduces a breaking change in how MCPClientManager is initialized (now requires a storage adapter in options). While the Agent class handles this transparently for most users, advanced users who might want custom storage backends (e.g., storing MCP configs in KV instead of SQL) now have a documented path forward.

The OAuth security improvements are also worth documenting for user awareness.


This is an automated sync from the intelligent documentation workflow.

@threepointone
Copy link
Contributor

📚 Documentation sync

Documentation has been updated in cloudflare-docs to reflect these changes:

Docs PR: cloudflare/cloudflare-docs#26535

Changes documented

  • New page: mcp-storage.mdx - Explains the storage adapter architecture and security considerations
  • Updated: mcp-client-api.mdx - Added security notes about OAuth credential cleanup
  • Updated: oauth-mcp-client.mdx - Added security information about automatic credential cleanup

The documentation covers:

  • Storage adapter interface for flexible MCP server configuration persistence
  • Automatic OAuth credential cleanup after successful authentication (security improvement)
  • Connection restoration behavior after Agent hibernation

🤖 Auto-generated comment

threepointone pushed a commit to cloudflare/cloudflare-docs that referenced this pull request Nov 14, 2025
- Add new storage-adapter.mdx page documenting the MCPStorageAdapter interface
- Update mcp-client-api.mdx to reference storage adapter pattern
- Document breaking change: MCPClientManager now requires storage option
- Include migration guide for direct MCPClientManager usage
- Document security improvements in OAuth credential handling

Related to cloudflare/agents#652
@threepointone
Copy link
Contributor

Documentation Updated

The documentation for this PR has been updated in cloudflare-docs#26535.

Documentation changes:

  • Added new Storage Adapter page documenting the MCPStorageAdapter interface
  • Updated MCP Client API reference to reference storage adapter pattern
  • Documented breaking change: MCPClientManager now requires storage option
  • Included migration guide for direct MCPClientManager usage
  • Documented security improvements in OAuth credential handling

Please review the documentation PR when you have a chance.

@mattzcarey mattzcarey marked this pull request as draft November 14, 2025 18:48
@threepointone
Copy link
Contributor

📚 Documentation sync completed

The documentation for this PR has been updated in the cloudflare-docs repository.

Documentation PR: cloudflare/cloudflare-docs#26535

Updated documentation:

Key changes documented:

  • Storage adapter pattern for decoupling MCP connection management from storage
  • Automatic connection restoration after Agent hibernation
  • OAuth security improvements (callback URL clearing to prevent replay attacks)
  • Custom storage backend implementation guide
  • Connection state management and efficient callback routing

The documentation is ready for review alongside this PR.

@mattzcarey mattzcarey marked this pull request as ready for review November 14, 2025 23:29
Copy link
Contributor

@deathbyknowledge deathbyknowledge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

paired and reviewd, ping when done and let's land this

Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stampung pending a descriptive changeset

@threepointone threepointone merged commit c07b2c0 into main Nov 18, 2025
4 of 5 checks passed
@threepointone threepointone deleted the feat/mcp-storage-adapter branch November 18, 2025 19:25
@threepointone threepointone mentioned this pull request Nov 18, 2025
naji247 pushed a commit to MCPCat/agents that referenced this pull request Nov 20, 2025
* move storage to the client manager

* fix: url missing after page load

* fix: dots

* fix: callback handling is now in client manager with 2 stage storage.

* some auth edge cases

* make storage sync for the moment to reduce complexity

* better default states

* cancel button on frontend

* remove openai from agents package.json

* unify mcp storage

* fix lock file

* fix: tests

* feat: more refactor

* tools tests

* update x402

* fix: types

* update default model in playground

* reasoning card

* Apply suggestion from @mattzcarey

* fix: from review

* add smooth scrolling

* fix: calling restore onstart and json schema the rest of the time

* fix: oauth tests

* remove smooth scroll :(

* make tests smaller

* rename to client for review

* revert changes to mcp.connect()

* make storage async

* fix readonly nit

* revert remove openai ai sdk provider

* remove public storage api

* add todos for ensureJsonSchema

* fix: mcp server states and unify observability to the mcp client manager class.

* remove callback cache

* dead

* createConnection. in memory method

* add tests

* changeset

* formatting

* claude comments

* remove redundant checks
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.

3 participants