fix: undo strict null widening for tool inputs - #939
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStrict-mode schema conversion now records null widening and detects unsupported ChangesOpenAI tool input null restoration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Chat
participant OpenAIAdapter
participant ToolInputNormalizer
participant ToolHandler
Client->>Chat: Start chat with ask_user tool
Chat->>OpenAIAdapter: Send strict tool schema
OpenAIAdapter-->>Chat: Stream tool-call arguments with null values
Chat->>ToolInputNormalizer: Restore null-widened input
ToolInputNormalizer-->>Chat: Return normalized tool input
Chat->>ToolHandler: Execute ask_user
ToolHandler-->>Chat: Return tool result
Chat->>OpenAIAdapter: Request follow-up completion
OpenAIAdapter-->>Client: Stream final assistant text
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2a6c4c8 to
0eeb026
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/openai-base/src/utils/schema-converter.ts`:
- Around line 293-296: Preserve and merge the nullWideningMap returned by
coerceStrictSchema for each supported anyOf variant instead of retaining only
nested.schema. Update both anyOf handling sites near the prop conversion logic
so variant-specific metadata remains associated without marking unrelated
genuinely nullable fields, and add a regression test for an optional nested
property inside anyOf that verifies provider-added nulls are removed during
original-schema validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67373ee5-ff74-40a3-8e8b-9b825e20f5be
📒 Files selected for processing (12)
.changeset/calm-tools-return.mdpackages/openai-base/src/adapters/chat-completions-text.tspackages/openai-base/src/adapters/responses-text.tspackages/openai-base/src/utils/schema-converter.tspackages/openai-base/src/utils/tool-input-normalizer.tspackages/openai-base/tests/chat-completions-text.test.tspackages/openai-base/tests/responses-text.test.tspackages/openai-base/tests/schema-converter.test.tspackages/openai-base/tests/tool-input-normalizer.test.tstesting/e2e/src/routeTree.gen.tstesting/e2e/src/routes/api.openai-strict-tool-null-wire.tstesting/e2e/tests/openai-strict-tool-null-wire.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/openai-base/tests/tool-converter-strict-fallback.test.ts (1)
58-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRelocate tests alongside the source they cover.
The current file tests multiple modules and is located in a dedicated
tests/directory. Consider splitting this file into separate test files colocated with their respective source modules (e.g.,packages/openai-base/src/adapters/responses-tool-converter.test.ts). As per coding guidelines, place unit tests in*.test.tsfiles alongside the source they cover.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openai-base/tests/tool-converter-strict-fallback.test.ts` around lines 58 - 88, Split the tests in tool-converter-strict-fallback.test.ts by the source module or converter they cover, and colocate each resulting *.test.ts file beside its corresponding implementation under src, such as responses-tool-converter.test.ts. Preserve all existing test coverage and behavior while removing the multi-module test file from the dedicated tests directory.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/openai-base/tests/tool-converter-strict-fallback.test.ts`:
- Around line 58-88: Split the tests in tool-converter-strict-fallback.test.ts
by the source module or converter they cover, and colocate each resulting
*.test.ts file beside its corresponding implementation under src, such as
responses-tool-converter.test.ts. Preserve all existing test coverage and
behavior while removing the multi-module test file from the dedicated tests
directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a792ea3c-5b8c-4b77-9473-10228ca296ad
📒 Files selected for processing (4)
packages/openai-base/src/utils/schema-converter.tspackages/openai-base/tests/schema-converter.test.tspackages/openai-base/tests/tool-converter-strict-fallback.test.tspackages/openai-base/tests/tool-input-normalizer.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/openai-base/tests/tool-input-normalizer.test.ts
- packages/openai-base/tests/schema-converter.test.ts
- packages/openai-base/src/utils/schema-converter.ts
38ffc6f to
59525c4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/openai-base/tests/tool-input-normalizer.test.ts (1)
1-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this new unit test alongside
tool-input-normalizer.ts.The current path does not satisfy the repository instruction requiring
*.test.tsfiles alongside their covered source.As per coding guidelines, “Place unit tests in
*.test.tsfiles alongside the source they cover.”<coding_guidelines>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/openai-base/tests/tool-input-normalizer.test.ts` around lines 1 - 100, Move the createToolInputNormalizer unit tests from the tests directory into a tool-input-normalizer.test.ts file alongside tool-input-normalizer.ts, preserving all existing test cases and imports with paths adjusted for the new location.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/openai-base/tests/tool-input-normalizer.test.ts`:
- Around line 1-100: Move the createToolInputNormalizer unit tests from the
tests directory into a tool-input-normalizer.test.ts file alongside
tool-input-normalizer.ts, preserving all existing test cases and imports with
paths adjusted for the new location.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5488cee5-334f-40d1-8389-cb3979cf2856
📒 Files selected for processing (13)
.changeset/calm-tools-return.mdpackages/openai-base/src/adapters/chat-completions-text.tspackages/openai-base/src/adapters/responses-text.tspackages/openai-base/src/utils/schema-converter.tspackages/openai-base/src/utils/tool-input-normalizer.tspackages/openai-base/tests/chat-completions-text.test.tspackages/openai-base/tests/responses-text.test.tspackages/openai-base/tests/schema-converter.test.tspackages/openai-base/tests/tool-converter-strict-fallback.test.tspackages/openai-base/tests/tool-input-normalizer.test.tstesting/e2e/src/routeTree.gen.tstesting/e2e/src/routes/api.openai-strict-tool-null-wire.tstesting/e2e/tests/openai-strict-tool-null-wire.spec.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- testing/e2e/tests/openai-strict-tool-null-wire.spec.ts
- packages/openai-base/src/utils/tool-input-normalizer.ts
- .changeset/calm-tools-return.md
- packages/openai-base/src/adapters/responses-text.ts
- packages/openai-base/tests/responses-text.test.ts
- testing/e2e/src/routeTree.gen.ts
- testing/e2e/src/routes/api.openai-strict-tool-null-wire.ts
- packages/openai-base/src/utils/schema-converter.ts
27e1655 to
26d7fd3
Compare
|
View your CI Pipeline Execution ↗ for commit 831cf62
☁️ Nx Cloud last updated this comment at |
|
Thanks for the PR, @jan-kubica! 🙌 @AlemTuzlak will take a look. Automated pre-review checks
Automated triage — a human review follows. |
69a0c00 to
74baf33
Compare
74baf33 to
1606446
Compare
@tanstack/ai
@tanstack/ai-acp
@tanstack/ai-angular
@tanstack/ai-anthropic
@tanstack/ai-bedrock
@tanstack/ai-byteplus
@tanstack/ai-claude-code
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-snippets
@tanstack/ai-codex
@tanstack/ai-cohere
@tanstack/ai-devtools-core
@tanstack/ai-durable-stream
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-grok-build
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-daytona
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-isolate-quickjs-bun
@tanstack/ai-llmgateway
@tanstack/ai-mcp
@tanstack/ai-memory
@tanstack/ai-mistral
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-opencode
@tanstack/ai-openrouter
@tanstack/ai-perplexity
@tanstack/ai-persistence
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-sandbox
@tanstack/ai-sandbox-cloudflare
@tanstack/ai-sandbox-daytona
@tanstack/ai-sandbox-docker
@tanstack/ai-sandbox-local-process
@tanstack/ai-sandbox-sprites
@tanstack/ai-sandbox-vercel
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-utils
@tanstack/ai-vercel-gateway
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/openai-base
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
Adapter tests now assert the exact tool input, not a partial match. Subclass schema conversion feeds the inverse map so Groq and later overrides stay aligned. The tools page notes optional vs nullable input. Vitest now also runs src colocated tests.
OpenAI-compatible strict tools promote optional fields to required nullable fields. The model can then send
nullfor an omitted optional. TanStack AI used to validate thatnullagainst the original schema, so.optional()failed and the tool did not run.This PR records which nulls the converter added, then removes only those nulls before the tool runs. A genuine
.nullable()nullstays.Changes
NullWideningMapduring the same OpenAI strict-schema conversion that adds nullability.toEqual.Checklist
pnpm run test:pr.docs/for this change, or this change is not user-facing.pnpm changeset), or this PR does not change a published package.Release Impact
Testing
Commands run on the review follow-up:
pnpm --filter @tanstack/openai-base test:lib— 203 passedpnpm --filter @tanstack/ai-groq test:lib— 36 passedpnpm --filter @tanstack/openai-base test:typesandpnpm --filter @tanstack/ai-groq test:types— passedpnpm --filter @tanstack/openai-base test:oxlintandpnpm --filter @tanstack/ai-groq test:oxlint— passed (existinganywarnings only)I did not re-run
pnpm test:pror the e2e spec after the follow-up commit.Manual test:
.optional()and.nullable()fields.null) and sets the nullable field tonull.null.How this PR makes testing easy: adapter unit tests, a normalizer unit test for a custom converter, and
testing/e2e/tests/openai-strict-tool-null-wire.spec.ts.Risk / rollback
Low. If a subclass converter and the inverse map still disagree, an optional
nullcan fail schema validation again. Revert the PR to undo.Public API change
Tool authors do not change call sites. Optional vs nullable input now matches the schema:
Adapter authors who change strict conversion should override
makeStructuredOutputCompatibleWithMapso the inverse map matches the wire schema.@tanstack/openai-basealso exportsmakeStructuredOutputCompatibleWithMap.