refactor(mcp): client manager hooks into standard AgentLifecycle#1895
refactor(mcp): client manager hooks into standard AgentLifecycle#1895mattzcarey wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: d3e3085 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
agents
@cloudflare/ai-chat
@cloudflare/codemode
create-think
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
cjol
left a comment
There was a problem hiding this comment.
I think the lifecycle hooks are great and work well here. I'm not entirely sure what each represents yet (what's a request vs a turn? Doesn't matter right now, and I know we'll obviously document them prominently eventually) but it feels a good shape.
I'm not so sure about the interface of MCPClientManager but -- a bit like my state PR from yesterday, maybe it's still progress even if it's not the best shape yet.
|
|
||
| The manager is resolved when Agent startup begins, after subclass field initializers run. Replacing `mcp` therefore replaces the default component rather than registering both. | ||
|
|
||
| `MCPClientManager` no longer has a standalone storage-only constructor. It must be attached to an `Agent` through the host-first form above so startup, OAuth callbacks, turn tools, protocol updates, observability, and destruction all use the same lifecycle. |
There was a problem hiding this comment.
I want to think about this for a little bit. Maybe it's the right move, but it makes the interface contract between MCPClientManager and host agent implicit and invites tighter coupling than I'd like.
|
|
||
| // Initialize MCPClientManager AFTER tables are created | ||
| this.mcp = new MCPClientManager(this._ParentClass.name, "0.0.1", { | ||
| const mcpHost: MCPClientManagerHost = { |
There was a problem hiding this comment.
(same point as my other comment, really) This is effectively the object that we're requiring non-Agent hosts to implement in order to use our MCP client, which is quite extensive and also non-specific (e.g. what bindings need to be returned by getEnv?)
Equally I know from experience trying it with Channels (and then State, where I ended up with a similar situation), that this is the bit where extraction from the current spaghetti gets very hairy, and it's what made me want to go in the opposite direction with Channels -- build a neat, isolated interface for the module, and then put all the adapter code inside Agent rather than bending the module to suit the current Agent implementation.
…t-lifecycle # Conflicts: # packages/agents/src/index.ts # packages/agents/src/mcp/client.ts # packages/agents/src/tests/mcp/client-manager.test.ts # packages/agents/src/tests/mcp/session-lifecycle.integration.test.ts # packages/ai-chat/src/index.ts # packages/think/src/think.ts
Collect onDestroy errors and rethrow after every component has run, so a failing component cannot block earlier components from releasing their resources. Document the onStart subscription-timing assumption on MCPClientManager.
Summary
onStart,onRequest,onTurn, andonDestroyMCPClientManagerown its Agent integration: schema setup, HTTP/RPC restoration, OAuth callback interception, tool contribution, state publication, observability forwarding, and cleanupAgent's MCP implementation to lifecycle dispatch plus compatibility delegators foraddMcpServer(),removeMcpServer(), andgetMcpServers()onMcpUpdate, OAuth-provider overrides, and connection behaviorAPI
Most Agents continue using the default
this.mcpwithout configuration. A custom manager is now installed as a normal Agent component field:The previous standalone storage-only constructor,
new MCPClientManager(name, version, { storage }), is removed. Keeping one Agent-owned construction mode avoids constructor discrimination, manual host attachment, fallback OAuth wiring, and a second lifecycle model.Lifecycle behavior
The lifecycle remains internal in this first stage. This PR does not add a raw Durable Object harness, a new MCP package, or a public lifecycle export.
Verification
pnpm --filter agents test:workerspnpm --filter @cloudflare/ai-chat testpnpm --filter @cloudflare/think testpnpm run buildpnpm run checkCI=true pnpm exec nx affected -t testAll checks pass on top of
origin/mainata43df13b.