-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Refactor agent tools #17927
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: master
Are you sure you want to change the base?
Refactor agent tools #17927
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2e7c3917
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const description = `${formatActionLabel(action)} in "${tableName}". ${def.description}` | ||
| return { | ||
| name: `${tableId}.${action}`, | ||
| readableName: `${tableName}.${action}`, |
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.
Use tool names that OpenAI accepts
Row tool names are now built as ${tableId}.${action}. That introduces dots in every tool name and can also introduce % from buildExternalTableId(... encodeURIComponent(tableName)) for external tables. OpenAI function/tool names are restricted to [A-Za-z0-9_-] and ≤64 chars; names with . or % are rejected, which means tool registration fails and the agent can’t call any row tools for those tables. Consider sanitizing the runtime tool name (keep readableName for display) and mapping back to the real table id/action in the execute handler.
Useful? React with 👍 / 👎.
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.
1 issue found across 32 files
Confidence score: 3/5
- There is a concrete regression risk:
packages/builder/src/components/common/TopBar.svelteswitchesiconfromexport lettoexport const, which will ignore parent-provided props and likely break icons in the UI. - Severity is moderate (6/10) with high confidence, so this change carries some user-facing impact despite being localized.
- Pay close attention to
packages/builder/src/components/common/TopBar.svelte-iconprop binding is removed and may break parent-provided icons.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/builder/src/components/common/TopBar.svelte">
<violation number="1" location="packages/builder/src/components/common/TopBar.svelte:17">
P2: `icon` is no longer a component prop. Svelte only treats `export let` as props; switching to `export const` means parent-provided `icon` values will be ignored. Use `export let` to keep the prop binding.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| $url | ||
|
|
||
| export let icon: string | ||
| export const icon: string | undefined = undefined |
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.
P2: icon is no longer a component prop. Svelte only treats export let as props; switching to export const means parent-provided icon values will be ignored. Use export let to keep the prop binding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/builder/src/components/common/TopBar.svelte, line 17:
<comment>`icon` is no longer a component prop. Svelte only treats `export let` as props; switching to `export const` means parent-provided `icon` values will be ignored. Use `export let` to keep the prop binding.</comment>
<file context>
@@ -14,7 +14,7 @@
$url
- export let icon: string
+ export const icon: string | undefined = undefined
export let breadcrumbs: Breadcrumb[]
export let showPublish = true
</file context>
| export const icon: string | undefined = undefined | |
| export let icon: string | undefined |
Description
This PR totally refactors how we handle tools within our agent. Previously you would need to tell the agent it would need to list tables, then get the table from that list then get row / create row etc. Now, we have a tool per each action per table, reducing the complexity and also reducing the amount of tools needing to be added to any one agent configuration.
This also fixes the bug where you were unable to add an external datasource as a tool. This is now possible, although to display the external datasource icons within the Codemirror box I had to add them as actual svgs, Codemirror doesn't accept svelte components which they were stored as previously (those still exist, i just copied them to SVG as well).
Fixes a few other miscellaneous bugs and cleans up

config.svelte`## Screenshots
Summary by cubic
Refactored agent tools to generate table-scoped actions (e.g., TableName.create_row) and added full support for external datasource tools with icons in the editor. This simplifies tool selection and fixes the bug that blocked adding external datasources as tools.
New Features
Refactors
Written for commit de2e7c3. Summary will update on new commits.