test: add validation tests for Elasticsearch tool operations#229986
test: add validation tests for Elasticsearch tool operations#229986nehaaprasad wants to merge 5 commits intoelastic:mainfrom
Conversation
botelastic
bot
added
ci:project-deploy-observability
|
💚 CLA has been signed |
|
Pinging @elastic/obs-ai-assistant (Team:Obs AI Assistant) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
@viduni94 Can you take a look? |
viduni94
left a comment
viduni94
left a comment
There was a problem hiding this comment.
Thank you for the PR @naaa760
I've added some comments
| @@ -0,0 +1,426 @@ | |||
| import expect from '@kbn/expect'; | |||
There was a problem hiding this comment.
The existing tests related to this function is located in x-pack/solutions/observability/test/api_integration_deployment_agnostic/apis/ai_assistant/complete/functions/elasticsearch.spec.ts
Would you be able to move the new tests there?
| }); | ||
|
|
||
| await observabilityAIAssistantAPIClient.editor({ | ||
| endpoint: 'POST /internal/observability_ai_assistant/chat', |
There was a problem hiding this comment.
Instead of calling the /chat endpoint, you could use the existing helper function to invoke a chat complete request with the function call. Would you be able to update all the tests to use the invokeChatCompleteWithFunctionRequest helper method?
Example:
const responseBody = await invokeChatCompleteWithFunctionRequest({
connectorId,
observabilityAIAssistantAPIClient,
functionCall: {
name: ELASTICSEARCH_FUNCTION_NAME,
trigger: MessageRole.User,
arguments: JSON.stringify({
method: 'POST',
path: 'traces*/_search',
body: {
size: 0,
aggs: {
services: {
terms: {
field: 'service.name',
},
},
},
},
}),
},
});
| expect(requestBody.messages[0].content).to.eql(SYSTEM_MESSAGE); | ||
| }); | ||
|
|
||
| it('should allow POST requests to _search endpoint', async () => { |
There was a problem hiding this comment.
| it('should allow POST requests to _search endpoint', async () => { | |
| it('should allow POST requests to the _search endpoint', async () => { |
| }); | ||
| }); | ||
|
|
||
| describe('Disallowed Operations', () => { |
There was a problem hiding this comment.
For all tests in this block, would you be able to check whether the function call response has an error that states the request is not allowed?
| }); | ||
| }); | ||
|
|
||
| describe('Edge Cases', () => { |
There was a problem hiding this comment.
| describe('Edge Cases', () => { | |
| describe('Request paths with query parameters', () => { |
| }, | ||
| ]; | ||
|
|
||
| describe('/internal/observability_ai_assistant/chat - Elasticsearch Tool Restrictions', function () { |
There was a problem hiding this comment.
After these tests are moved to the tool: elasticsearch test suite, the describe block can be renamed.
| describe('/internal/observability_ai_assistant/chat - Elasticsearch Tool Restrictions', function () { | |
| describe('tool restrictions', function () { |
…strictions - Replace system message validation with actual function execution testing - Use invokeChatCompleteWithFunctionRequest helper as suggested by reviewer - Test that allowed operations (GET, POST to _search) return success responses - Test that disallowed operations (PUT, DELETE, PATCH, POST to non-search) return proper error messages - Verify actual security restrictions rather than just system configuration - Addresses reviewer feedback about validating function call responses vs system messages
|
@viduni94 |
Thank you for the changes. Would you be able to address the other comments as well? Would you also be able to add some screenshots or screen recordings of testing the helper method in the UI and it to the PR description? |
5 participants
CLOSES: #229975
Summary
This PR addresses the review comments from #229497 by improving the Elasticsearch tool implementation. It extracts the operation validation logic into a well-named helper function
isOperationAllowedand adds comprehensive API tests to ensure only certain operations are allowed through the AI assistant.