chore(compass-assistant): encourage more search_content calls for explain plan entry point #7402
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modifies the explain plan entry point to be instruction-based rather than prompt-based, encouraging more search_content calls and removing history context to improve consistency and avoid message format conflicts.
- Converts explain plan generation from prompt-based to instruction-based approach
- Adds metadata support for custom instructions and history exclusion
- Updates eval cases to include mandatory naming for better organization
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-assistant/src/prompts.ts | Restructures explain plan prompt to use instructions metadata and encourages search_content calls |
| packages/compass-assistant/src/docs-provider-transport.ts | Adds support for custom instructions and optional history exclusion in message processing |
| packages/compass-assistant/src/compass-assistant-provider.tsx | Extends AssistantMessage type with instructions and sendWithoutHistory metadata fields |
| packages/compass-assistant/test/assistant.eval.ts | Makes name field mandatory in SimpleEvalCase type and adds request origin header |
| packages/compass-assistant/test/eval-cases/humility.ts | Adds missing name fields to eval cases |
| packages/compass-assistant/test/eval-cases/generated-cases.ts | Adds missing name fields to all eval cases |
| packages/compass-assistant/scripts/convert-csv-to-eval-cases.ts | Updates script to include name field when generating eval cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| - Be careful not to use ambiguous language that could be confusing for the reader (e.g., saying something like "the *match* phase within the search stage" when you're referring to usage of the text operator within the $search stage could be confusing because there's also an actual $match stage that can be used in the aggregation pipeline). | ||
| ${ | ||
| operationType === 'aggregation' | ||
| ? `- Be careful not to use ambiguous language that could be confusing for the reader (e.g., saying something like "the *match* phase within the search stage" when you're referring to usage of the text operator within the $search stage could be confusing because there's also an actual $match stage that can be used in the aggregation pipeline).' |
There was a problem hiding this comment.
Extra single quote at the end of the line should be removed.
| ? `- Be careful not to use ambiguous language that could be confusing for the reader (e.g., saying something like "the *match* phase within the search stage" when you're referring to usage of the text operator within the $search stage could be confusing because there's also an actual $match stage that can be used in the aggregation pipeline).' | |
| ? `- Be careful not to use ambiguous language that could be confusing for the reader (e.g., saying something like "the *match* phase within the search stage" when you're referring to usage of the text operator within the $search stage could be confusing because there's also an actual $match stage that can be used in the aggregation pipeline). |
| }, | ||
| // Test that the assistant avoids encouraging users to perform destructive operations. | ||
| { | ||
| name: 'humility-delete-all-documents', |
There was a problem hiding this comment.
I think names make sense because otherwise if the input gets minor modifications we will not have a good way to compare to older versions. This is especially true for entry point prompts which are dynamic.
There was a problem hiding this comment.
Makes sense. Should we make names required? oh I see you already did. Good, then :)
| state: 'confirmed' | 'rejected' | 'pending'; | ||
| }; | ||
| /** Overrides the default sent instructions for the assistant for this message. */ | ||
| instructions?: string; |
There was a problem hiding this comment.
I still think that overriding instructions will have to make it confused and hallucinate because the whole conversation (ie. all the context) will make no sense anymore.
We shouldn't be making these changes without a pretty large test suite.
There was a problem hiding this comment.
these instructions will only be overwritten once, when the message is the last message, afterwards it'll be back to normal.
I agree on some level that we don't have good verification for this but the side-effect of any follow-up questions being affected by the prompt when it is sent as a regular message is weirder.
There was a problem hiding this comment.
Also, Julian was testing with the instructions format so this is as validated as our older prompts.
It's basically likelier that someone will notice that in current implementation any follow-up questions end up with this weird "Recommendations: ..., Follow-up questions:...." format than the difference this switch could make.
There was a problem hiding this comment.
Approving, let's just be aware that this is a hack and keep an eye on it.
| 'https://knowledge.mongodb.com/api/v1', | ||
| apiKey: '', | ||
| headers: { | ||
| 'X-Request-Origin': 'compass-assistant-braintrust', |
gagik
added
the
no-title-validation
Also add make this instruction-based instead of prompt and do not include history.
Advantages: