fix(mcp): report invalid tool arguments#40979
Conversation
@microsoft-github-policy-service agree |
pavelfeldman
left a comment
There was a problem hiding this comment.
I like the intent, but the implementation is below the quality level we would usually accept. Handle zod errors in a nicely-typed manner here, do not declare primitive types.
|
Thanks, updated this to narrow specifically on Validated with:
|
| return String(reason); | ||
| } | ||
|
|
||
| function formatValidationError(toolName: string, error: z.ZodError): string { |
There was a problem hiding this comment.
Could we use Zod's builtin formatter? https://zod.dev/error-formatting
We have custom formatting in the other place because we want to make it look like common CLI args errors, but that doesn't apply here.
There was a problem hiding this comment.
Sounds good, just updated it
| } | ||
|
|
||
| test('reports missing required tool arguments', async () => { | ||
| const response = await createBackend().callTool('browser_validate', {}); |
There was a problem hiding this comment.
can you turn the tests into e2e tests like the rest of the codebase and assert against real tools?
Removed tests for validation errors in JSON mode and nested tool argument paths. Signed-off-by: Pavel Feldman <pavel.feldman@gmail.com>
Test results for "MCP"7207 passed, 1113 skipped Merge workflow run. |
Summary