-
Notifications
You must be signed in to change notification settings - Fork 757
feat: Create frontend tools framework and integrate to backend ai system #6609
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
Conversation
…nal_tools, rename frontend_tools to additional_tools, remove frontend logic, code, and tools from tool_manager since it gets passed in directly during prompt
…> instead of AnyZodObject which is not available in v4
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
for more information, see https://pre-commit.ci
| export const FRONTEND_TOOL_REGISTRY = new FrontendToolRegistry(); | ||
|
|
||
| /* Register all the frontend tools */ | ||
| FRONTEND_TOOL_REGISTRY.registerAll([ |
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.
could we put this in the constructor (instead of register), then we can confidently do @Memoize on the.getToolSchemas function
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.
Done!
| return this.tools.has(toolName); | ||
| } | ||
|
|
||
| private getTool(toolName: string): StoredTool { |
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.
nit: getToolOrThrow if it may throw
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.
done!
frontend/src/core/ai/tools/base.ts
Outdated
| TIn extends AnyZodObject, | ||
| TOut extends AnyZodObject, | ||
| > { | ||
| public readonly name: string; |
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.
this does not do much being abstract, and instead could just be an interface that others implement
export class MyTool implement AiTool {
public readonly name = "run"
public readonly schema = z.object()
....
}
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.
done!
| }); | ||
| if (FRONTEND_TOOL_REGISTRY.has(toolCall.toolName)) { | ||
| // Invoke the frontend tool | ||
| const response = await FRONTEND_TOOL_REGISTRY.invoke( |
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.
should we validate the mode here? or not needed?
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.
I don't think its needed since it's already hard typed. I'm more worried about the CopilotMode type in the frontend going out of sync with the backend.
| /** should be the same as marimo/_config/config.py > CopilotMode */ | ||
| export type CopilotMode = "manual" | "ask"; | ||
|
|
||
| export interface FrontendToolDefinition { |
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.
you could grab this from import type { components } from "@marimo-team/marimo-api"; or at least extend it:
export interface FrontendToolDefinition extends ToolDefinition {
source: "frontend";
}
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.
Done! I was looking for something like this thanks
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.
@mscolnick can we also do this with CopilotMode?
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.
yep we can as well (but less worried it will get out of sync)
| /** All registered tools */ | ||
| private tools = new Map<string, StoredTool>(); | ||
|
|
||
| registerAll<TIn extends AnyZodObject, TOut extends AnyZodObject>( |
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.
you could simplify this to:
constructor() {
this.tools = new Map(tools.map(tool => [tool.id, tool])
}
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.
done! Nice we were able to get rid of registerAll() and register()
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| if tools: | ||
| create_params["tools"] = convert_to_openai_tools(tools) | ||
| all_tools = tools + additional_tools | ||
| create_params["tools"] = convert_to_openai_tools(all_tools) |
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.
Bug: Frontend Tools Ignored When Backend Tools Unconfigured
Frontend tools (additional_tools) are not sent to the AI provider when backend tools (self.config.tools) are not configured. The if tools: condition prevents additional_tools from being included, which means frontend tools are ignored. This impacts OpenAIProvider, AnthropicProvider, GoogleProvider, and BedrockProvider.
📝 Summary
Create frontend tools framework to manage and send tools to backend via ai body. Update openapi to include ToolDefinition in body. Remove any frontend tool handing from ToolManager.
🔍 Description of Changes
📋 Checklist
Note
Introduce a frontend tools framework and pass tool schemas through chat requests to providers, while removing frontend-tool handling from the backend and updating OpenAPI/types.
AiToolinterface (base.ts),FrontendToolRegistrywith zod validation and memoized schemas (registry.ts), and a sample tool (sample-tool.ts); add tests for registry behavior.FRONTEND_TOOL_REGISTRY.getToolSchemas()in chat request body and handle frontend tool calls client-side before falling back to backend (chat-panel.tsx,chat-utils.ts).zodpath intsconfig.json.stream_completionto acceptadditional_toolsand merge with configured tools for OpenAI, Anthropic, Google, and Bedrock (providers.py).body.toolsto providers inPOST /api/ai/chat; no tools for completion/inline completion (ai.py).tool_manager.py).ToolDefinitionschema and include optionaltoolsinChatRequest; regenerate TS types (api.yaml,src/api.ts).ToolDefinitionin dev OpenAPI generation (commands.py).stream_completionsignature (test_providers.py).Written by Cursor Bugbot for commit d7836e8. This will update automatically on new commits. Configure here.