Fixes to MCP Client with authentication#173
Conversation
🦋 Changeset detectedLatest commit: 14ba5ca 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 |
cmsparks
force-pushed
the
csparks/auth-client-fixes
branch
2 times, most recently
from
a2f11db to
d608bdb
Compare
threepointone
left a comment
threepointone
left a comment
There was a problem hiding this comment.
this looks good, tentatively approving, just a couple of questions before landing
| `${this.env.HOST}/agents/my-agent/${this.name}/callback` | ||
| ); | ||
| const { id, authUrl } = await this.mcp.connect(url, { | ||
| transport: { authProvider }, |
There was a problem hiding this comment.
should we pass this here, or in the constructor? what benefit to either approach?
There was a problem hiding this comment.
I decided to pass it into the connect method because a transport is intended to be fully owned by a client connection (i.e. two MCP clients can't use the same transport). Because the authProvider is part of the transport options, it would also have to be owned by one client (so each client can have a unique redirect URL, oauth client information, tokens, etc).
I think we'd require some intermediate auth provider creation function if we were to pass it into the constructor.
edit: realized my initial comment was wrong
| if (this.mcp.mcpConnections[id].connectionState === "ready") { | ||
| await this.refreshServerData(); | ||
| } |
There was a problem hiding this comment.
what does this do? should it be running more often?
There was a problem hiding this comment.
It just refreshes the agent's state for the tools, prompts, and resources which get sent to the client. It should be sufficient to just update these upon adding a new server or authenticating to a server.
In the future we should trigger this function if the server sent a notification that the tools/prompts/resources list changed, but I haven't added a nice way to hook into those notifications in the MCPClientManager yet.
2 participants
Agents changes:
AgentsOAuthClientProvider)Demo fixes:
this.namenot being set. Now the callback URL doesn't need to be passed into theMCPClientManager(See MCP Client x Agents Implementation #125)I double checked that the
DurableObjectOAuthClientProviderstill works on the free tier. The SQLite storage backend still supports using the key value storage API, so free users shouldn't have issues here.