-
Notifications
You must be signed in to change notification settings - Fork 470
feat(js/core): Add dynamic-action-provider support #3598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
js/plugins/mcp/src/index.ts
Outdated
ai.defineDynamicActionProvider(options.name, async () => ({ | ||
tool: await mcpHost.getActiveTools(ai), | ||
resource: await mcpHost.getActiveResources(ai), | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a strong opinion but looks like the delta between the old and new APIs is only these 4 lines.
Do we need to deprecate the old API instead of providing dynamic actions as an add on?
createMcpHost({...}).withDynamicActions({ai})
This is not consistent with our defineBlah({ai, ...})
APIs but it will avoid deprecating the old one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It requires the ai parameter and it also makes the name field of the options a required field. It's a breaking change to createMcpHost.
(await registry.lookupAction(`/tool/${name}`)) || | ||
(await registry.lookupAction(`/prompt/${name}`)); | ||
(await registry.lookupAction(`/prompt/${name}`)) || | ||
(await registry.lookupAction(`/dynamic-action-provider/${name}`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would we allow specifying tool names that are only resolvable through DAP without using the DAP ref syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. I don't think that's a good idea. You might have a /dynamic-action-provider/myDap1:tool/fs/read_file that is configured to read stuff from directory A and another one /dynamic-action-provider/myDap2:tool/fs/read_file that is configured to read stuff from directory B. If you just say /tool/fs/read_file it's ambiguous (and we don't know where to find it).
Checklist (if applicable):