-
Notifications
You must be signed in to change notification settings - Fork 246
feat(compass-assistant): add basic tool calling & placeholder tool cards COMPASS-10144 #7668
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
packages/compass-assistant/src/components/tool-call-message.tsx
Outdated
Show resolved
Hide resolved
| chat.messages = [...chat.messages, contextPrompt]; | ||
| } | ||
|
|
||
| const { enableToolCalling } = preferences.getPreferences(); |
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 is something I'm probably gonna have to check with Sergey when he's back, but the preferences hook isn't working well here - it caches the initial value so it doesn't respond if you toggle the flag from the preferences modal.
This way at least works for now.
| isMyQueriesEnabled; | ||
|
|
||
| const query = useQueryBarQuery(); | ||
| useSyncAssistantGlobalState('currentQuery', query); |
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 have no idea where the best place would be to sync the query. This was the most minimally invasive way I could quickly think of that's guaranteed to be "in sync".
| : 'No output available'; | ||
| const hasOutput = toolCall.state === 'output-available'; | ||
|
|
||
| const isAwaitingApproval = toolCall.state === 'approval-requested'; |
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.
maybe a component which returns a different thing based on a switch statement instead? not too sure
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.
Yeah I think whatever we choose to do here we'll probably be revising it in the database tool calling PR and onwards.
| const editorInitialValueRef = useRef<string>(pipelineText); | ||
| const editorCurrentValueRef = useCurrentValueRef<string>(pipelineText); | ||
|
|
||
| useSyncAssistantGlobalState('currentAggregation', pipelineText); |
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.
In the pipeline as text editor case we happen to already have the pipeline text. So I think this should at least be performant enough?
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 lack context about implications of this so maybe second opinion is good but sgtm
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 think in this case I'm more sure that it is fine. I'm more concerned about the builder path.
| const mapState = (state: RootState) => { | ||
| return { | ||
| stagesIdAndType: state.pipelineBuilder.stageEditor.stagesIdAndType, | ||
| pipelineText: getPipelineTextFromStages( |
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.
In this case I can't think of a better way to calculate the pipeline text. The usual machinery is only accessible from redux land because it uses pipelineBuilder. The stages on the state are of a different type to the ones the aggregation builder and therefore it isn't easy to just factor out a utility function.
And then the builder has all these invalid / partial states. I'm attempting to just skip those stages. Maybe we want just the most recent valid pipeline, maybe we want the partial states and every syntax error so the model can do something with it?
Maybe I'm overthinking it and this is fine for now.
… & settings to the system context prompt
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.
Pull request overview
This PR implements basic tool calling functionality in Compass Assistant, allowing the AI to access current query and aggregation state through a new "get-compass-context" tool. Tool calls are displayed as interactive cards that require user approval before execution.
Key changes:
- Added ToolsController to manage tool registration and execution
- Implemented tool call UI with expandable input/output sections and approval buttons
- Integrated query/aggregation state tracking for the assistant to access
- Updated system prompts to reflect tool calling capabilities
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/compass/src/app/components/home.tsx | Added ToolsControllerProvider wrapper |
| packages/compass-web/src/entrypoint.tsx | Added ToolsControllerProvider and fixed formatting |
| packages/compass-query-bar/src/components/query-bar.tsx | Syncs current query to assistant global state |
| packages/compass-query-bar/package.json | Added compass-assistant dependency |
| packages/compass-generative-ai/src/tools-controller.ts | New ToolsController class for managing tools |
| packages/compass-generative-ai/src/provider.tsx | Added ToolsController provider and context |
| packages/compass-assistant/src/prompts.ts | Updated prompts to include tool calling instructions |
| packages/compass-assistant/src/prompts.spec.ts | Updated test expectations for new prompt format |
| packages/compass-assistant/src/docs-provider-transport.ts | Added tools support to transport layer |
| packages/compass-assistant/src/components/tool-call-message.tsx | New component for rendering tool call cards |
| packages/compass-assistant/src/components/assistant-chat.tsx | Integrated tool call rendering and approval handling |
| packages/compass-assistant/src/compass-assistant-provider.tsx | Wired up ToolsController and preferences |
| packages/compass-assistant/src/compass-assistant-provider.spec.tsx | Added tests for tool calling feature |
| packages/compass-assistant/src/assistant-global-state.tsx | Changed query/aggregation types from object to string |
| packages/compass-assistant/src/@ai-sdk/react/use-chat.ts | Exposed addToolApprovalResponse method |
| packages/compass-aggregations/.../pipeline-builder-ui-workspace/index.tsx | Syncs aggregation pipeline text to assistant global state |
| packages/compass-aggregations/.../pipeline-as-text-workspace/pipeline-editor.tsx | Syncs aggregation pipeline text to assistant global state |
packages/compass-assistant/src/components/tool-call-message.tsx
Outdated
Show resolved
Hide resolved
...gregations/src/components/pipeline-builder-workspace/pipeline-builder-ui-workspace/index.tsx
Outdated
Show resolved
Hide resolved
packages/compass-assistant/src/compass-assistant-provider.spec.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
gagik
left a comment
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.
Some comments, LGTM otherwise
This is copied liberally from Gagik's PoC with minimal changes so far.
To try it out you'll have to toggle the "enable tool calling" feature flag. All this UI will be switched out to use leafygreen's tool card soon once that's ready. For now it is just functional placeholders.
There are a few flows that are probably best tested with e2e tests. Especially since it is centred around the temporary tool call cards.