fix(provider-utils): add comprehensive type tests showing broken behavior #11858
Conversation
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| */ | ||
| export const DEFAULT_EMPTY_INPUT_SCHEMA = { | ||
| type: 'object', | ||
| properties: {}, |
There was a problem hiding this comment.
Default schema when tool.inputSchema is undefined is missing 'additionalProperties: false', causing inconsistent validation behavior compared to other parts of the codebase.
| return tool; | ||
| return { | ||
| ...tool, | ||
| inputSchema: tool.inputSchema ?? DEFAULT_EMPTY_INPUT_SCHEMA, |
There was a problem hiding this comment.
The tool() function assigns a plain object (DEFAULT_EMPTY_INPUT_SCHEMA) to inputSchema instead of wrapping it in a FlexibleSchema, violating the Tool type contract.
| */ | ||
| export const DEFAULT_EMPTY_INPUT_SCHEMA = { | ||
| type: 'object', | ||
| properties: {}, |
There was a problem hiding this comment.
Inconsistent validation behavior: DEFAULT_EMPTY_INPUT_SCHEMA lacks 'additionalProperties: false', causing schema to allow additional properties during validation despite the provider rejecting them
kairosci
changed the title
kairosci
force-pushed
the
fix/5527-default-empty-input-schema
branch
2 times, most recently
from
9a69e3c to
4a741b9
Compare
kairosci
force-pushed
the
fix/5527-default-empty-input-schema
branch
from
4a741b9 to
c88dc6a
Compare
…d add comprehensive type tests
kairosci
changed the title
… to demonstrate breakage
kairosci
changed the title
kairosci
changed the title
kairosci
force-pushed
the
fix/5527-default-empty-input-schema
branch
from
1450d06 to
11e6cea
Compare
|
@lgrammel I have added the requested tests, demonstrating that the type definitions are currently incomplete as the TypeScript check now fails |
|
Since it was asked to prove the problem, I created code to verify the initial hypothesis, which at the moment has no corrections. So as it should be, TypeScript does not pass. (Prettier is to be formatted, but I think it's the least of the problems) |
|
@kairosci thanks for creating the tests. I meant that in this PR, the tests should describe the current behavior (even if wrong), i.e. they should just specify what exists. In the follow up PR that fixes the issue, we would change the tests to show the breakage, and then fix it. This approach will make it very clear in the 2nd PR what behavior was changed and how, making it easier to think about (e.g. re backwards compatibility). Can you adjust this PR such that the tests encode the existing behavior, even if buggy? |
2 participants
Background
The
ToolApprovalRequesttype is currently missingtoolNameandinputproperties. This PR adds comprehensive type tests totool.test-d.tsand explicitly demonstrates the broken behavior by asserting existence of these properties, which causes the tests to fail.Summary
packages/provider-utils/src/types/tool.test-d.tsto be comprehensive for the current state, including tests for:ToolApprovalRequest(failing test to demonstrate missing properties)Manual Verification
Ran
pnpm vitest run packages/provider-utils/src/types/tool.test-d.ts --typecheckand verified that it fails as expected due to missing properties.Checklist