fix(core): avoid null OpenAPI schema types#6323
Conversation
|
Thanks for the PR! Template looks good ✓ — bilingual, all required sections present, linked issue #6322. Problem: Observed bug with clear evidence. I verified the bug on Direction: Aligned. Tool schemas flow through Approach: Minimal and correct — 3 lines of production code, 3 focused test cases. The fix skips Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 双语、所有必要章节齐全、关联 issue #6322。 问题： 已观测到的 bug，有明确证据。我在 方向： 对齐。工具 schema 在 Anthropic 和 OpenAI 两条内容生成路径中都会经过 方案： 最小化且正确——3 行生产代码、3 个 focused 测试用例。修复在选择 fallback 类型时跳过 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: Before reading the diff, my approach was identical — in the Correctness: The fix handles all cases correctly:
No blockers found. No security issues, no regressions, no AGENTS.md violations. The change is 3 production lines + 3 focused tests — exactly the minimal fix. Downstream consumers: TestingBug reproduction on main: Bug confirmed — Fix verification (simulated): All cases produce correct output. Existing unit tests on main: 12/12 passed. Note: tmux real-scenario testing is not applicable here — 中文说明代码审查独立方案： 在读 diff 之前，我的方案和 PR 完全一致——在 正确性： 修复正确处理了所有情况。无阻塞问题、无安全漏洞、无回归、无 AGENTS.md 违规。3 行生产代码 + 3 个 focused 测试——恰好是最小修复。 下游消费者： 测试Bug 在 main 上已复现：输出 — Qwen Code · qwen3.7-max |
|
This is a clean, minimal bugfix that does exactly what it says. The bug is real — I reproduced it on The test coverage is right — three new tests covering root-level nullable unions, the all-null degenerate case, and nested object properties. Existing tests (12/12) still pass. No regression for non-nullable unions or the existing two-element nullable path. Downstream impact is well-contained: No scope creep, no drive-by refactors, no over-engineering. This is what a good community contribution looks like — focused, well-tested, and solving a real problem. Verdict: Approve. ✅ 中文说明这是一个干净、最小化的 bug 修复，完全符合描述。 Bug 真实存在——我在 测试覆盖合理——3 个新测试覆盖 root-level nullable union、all-null 退化情况和 nested object properties。已有 12 个测试全部通过。不影响非 nullable union 和已有的两元素 nullable 路径。 下游影响可控： 没有范围蔓延、没有顺手重构、没有过度设计。 结论：通过。 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
The 3-line fix correctly addresses the case where the N-element union fallback could emit type: "null" — types.find((t) => t !== 'null') ?? types[0] selects the first concrete type, and nullable: true is set only when null is present. The two new tests cover root-level and nested nullable unions. All 14 schemaConverter tests pass; 464 other test files in packages/core pass (107 unrelated failures are pre-existing CI environment issues with hardcoded $HOME paths).
— qwen3.7-max via Qwen Code /review
| // OpenAPI 3.0 doesn't support type arrays. | ||
| // Ideal fix would be anyOf, but simple fallback is safer for now. | ||
| target['type'] = types[0]; | ||
| target['type'] = types.find((t) => t !== 'null') ?? types[0]; |
There was a problem hiding this comment.
[Suggestion] The ?? types[0] fallback handles the degenerate case where types.find((t) => t !== 'null') returns undefined (e.g., { type: ['null'] }). This is new code introduced by this PR, but no test exercises it.
Without a test, a future refactor could silently break this fallback path. Consider adding a regression test:
it('should fall back to types[0] when all types are null', () => {
const input = { type: ['null'] };
const expected = { type: 'null', nullable: true };
expect(convertSchema(input, 'openapi_30')).toEqual(expected);
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, good catch. I added the focused regression test for the degenerate all-null fallback case:
{ type: ['null'] } -> { type: 'null', nullable: true }This only updates schemaConverter.test.ts; no production code or unrelated tests were changed. I reran the focused validation after repairing the local Windows/WSL optional native dependency state:
Windows / packages/core:
npm run test -- src/utils/schemaConverter.test.ts
Test Files 1 passed; Tests 15 passed
Windows / packages/core:
npm run typecheck
passed
Windows / packages/core:
npm run lint
passed
WSL Ubuntu-22.04 / packages/core:
npm run test -- src/utils/schemaConverter.test.ts
Test Files 1 passed; Tests 15 passed
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
The 3-line fix correctly addresses the case where the N-element union fallback could emit type: "null" — types.find((t) => t !== 'null') ?? types[0] selects the first concrete type, and nullable: true preserves the contract when null was in the union. The three new tests cover the targeted regression paths (root-level nullable union, all-null degenerate, and nested property recursion), and the existing 15-test suite continues to pass. Deterministic checks (tsc, eslint) clean.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/core/src/utils/schemaConverter.ts:50-55 |
The if branch (2-element nullable union) is now dead code — the else branch produces identical output for all inputs satisfying types.length === 2 && types.includes('null'). Two branches must be kept in sync by convention, with no structural enforcement. |
Collapse both branches into one: if (Array.isArray(source['type'])) { const types = source['type'] as string[]; target['type'] = types.find((t) => t !== 'null') ?? types[0]; if (types.includes('null')) { target['nullable'] = true; } } |
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
This PR keeps OpenAPI 3.0 schema conversion from emitting
type: "null"when a nullable JSON Schema union listsnullbefore concrete types. The converter now selects the first non-null fallback type and preserves nullability withnullable: true.Why it's needed
What Problem This Solves
OpenAPI 3.0 compatibility mode must downgrade JSON Schema
typearrays because OpenAPI 3.0 does not support array-valuedtype. Whennullappears before concrete types, the previous fallback could emittype: "null", which is not a valid standalone OpenAPI 3.0 schema type.Changes
The converter now skips
nullwhen choosing the fallback OpenAPI 3.0 schema type, selects the first concrete type instead, and preserves the nullable contract withnullable: true. This keeps the existing lossy fallback behavior for multi-type unions instead of introducing a broaderanyOf/oneOfconversion.Evidence
For
{ "type": ["null", "string", "number"] }, the intended OpenAPI 3.0 output is{ "type": "string", "nullable": true }, not{ "type": "null" }. The focused regression coverage checks root-level nullable unions, nested object property unions, and the reviewer-requested all-null fallback case.Possible call chain / impact
Converted tool schemas, including MCP-style tool parameter schemas, can flow into OpenAPI-compatible tool serialization before the model receives the tool definition. Emitting an invalid OpenAPI schema can plausibly cause provider-side schema validation to reject the tool definition; using a concrete type plus
nullable: truekeeps the schema valid while preserving nullability.Reviewer Test Plan
How to verify
Review the focused unit coverage for root-level and nested nullable unions where
nullappears before concrete types, plus the all-null fallback path. The expected result is that nullable unions pick the first concrete type and setnullable: true, while the degenerate all-null case keeps the current fallback value and remains covered by regression testing.Evidence (Before & After)
Before this fix, the fallback for
{ "type": ["null", "string", "number"] }selected the first array element and could emit{ "type": "null" }. After this fix, the same schema converts to{ "type": "string", "nullable": true }. The nested regression test covers the same fallback inside object properties, and the reviewer-requested test covers{ "type": ["null"] } -> { "type": "null", "nullable": true }.Validation run:
Tested on
Environment (optional)
Node.js v24.15.0, npm 11.12.1. The project requires Node.js >=22. WSL validation initially hit missing Linux optional native dependencies from reusing Windows-installed
node_modules; after installing optional dependencies for the Linux environment, the focused test passed. Windows validation later required restoring the matching Windows optional native packages for Rollup and esbuild after the sharednode_moduleshad been used from WSL.Risk & Scope
anyOf/oneOfpreservation.npm run preflightwas not run locally; validation was limited to focused unit tests, core typecheck, core lint, and WSL focused test.automode is unchanged, and existing two-member nullable conversions continue to use a concrete type withnullable: true.Linked Issues
Fixes #6322
Chinese explanation
What this PR does
这个 PR 避免 OpenAPI 3.0 schema 转换在可空 JSON Schema union 把
null放在具体类型之前时输出type: "null"。现在转换器会选择第一个非null的 fallback 类型，并用nullable: true保留可空语义。Why it's needed
What Problem This Solves
OpenAPI 3.0 兼容模式必须降级 JSON Schema 的
type数组，因为 OpenAPI 3.0 不支持数组形式的type。当null排在具体类型之前时，原来的 fallback 可能输出type: "null"，但这不是 OpenAPI 3.0 支持的独立 schema 类型。Changes
转换器现在在选择 OpenAPI 3.0 fallback 类型时会跳过
null，改为选择第一个具体类型，并通过nullable: true保留可空语义。这个 PR 保留现有的多类型 union 有损 fallback 行为，没有引入更大范围的anyOf/oneOf转换。Evidence
对于
{ "type": ["null", "string", "number"] }，预期的 OpenAPI 3.0 输出应为{ "type": "string", "nullable": true }，而不是{ "type": "null" }。focused 回归覆盖了 root-level 可空 union、object properties 内部的同一条 fallback 路径，以及 reviewer 要求补充的 all-null fallback 情况。Possible call chain / impact
被转换的 tool schema，包括 MCP 风格的工具参数 schema，可能会先进入 OpenAPI-compatible tool serialization，然后模型才会收到工具定义。输出非法 OpenAPI schema 可能导致 provider 侧 schema 校验拒绝工具定义；使用具体类型加
nullable: true可以在保留可空语义的同时维持 schema 合法性。Reviewer Test Plan
How to verify
查看新增的 focused 单测：它覆盖了 root-level 和 nested 两种
null出现在具体类型之前的可空 union，也覆盖了 all-null fallback 路径。期望行为是可空 union 选择第一个具体类型并设置nullable: true，退化的 all-null 情况保留当前 fallback 值且有回归测试保护。Evidence (Before & After)
修改前，
{ "type": ["null", "string", "number"] }的 fallback 会选择数组第一个元素，因此可能输出{ "type": "null" }。修改后，同一个 schema 会转换为{ "type": "string", "nullable": true }。嵌套回归测试也覆盖了 object properties 内部的同一条 fallback 路径；reviewer 要求的测试覆盖了{ "type": ["null"] } -> { "type": "null", "nullable": true }。验证命令：
Tested on
Environment (optional)
Node.js v24.15.0，npm 11.12.1。项目要求 Node.js >=22。WSL 验证一开始因为复用 Windows 安装的
node_modules，缺少 Linux 版 optional native dependencies；补齐 Linux optional dependencies 后，focused test 已通过。后续 Windows 验证也需要恢复同一份共享node_modules里的 Windows 版 Rollup 和 esbuild optional native packages。Risk & Scope
anyOf/oneOfunion 保真改造。npm run preflight；验证范围限于 focused unit tests、core typecheck、core lint 和 WSL focused test。auto模式不变，已有两个成员的 nullable 转换仍然输出具体类型加nullable: true。Linked Issues
Fixes #6322