fix(core): preserve invalid schema length strings - #5312
Conversation
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hey @tt-a1i — thanks for the fix! The bug is real and the code change looks reasonable, but the PR body doesn't follow the PR template.
The template requires these sections: What this PR does, Why it's needed, Reviewer Test Plan (with How to verify, Evidence (Before & After), Tested on), Risk & Scope, Linked Issues, and a 中文说明 translation. Your PR uses custom headings (Summary, Test plan, Demo, AI Assistance Disclosure) that don't match.
Could you reformat the body to follow the template? It helps reviewers and keeps triage consistent. The actual content is fine — just needs to fit the expected structure.
中文说明
@tt-a1i 你好——感谢修复!这个 bug 确实存在,代码改动也合理,但 PR 正文没有按照 PR 模板 的格式来写。
模板要求包含以下章节:What this PR does、Why it's needed、Reviewer Test Plan(含 How to verify、Evidence (Before & After)、Tested on)、Risk & Scope、Linked Issues,以及 中文说明 翻译。你的 PR 使用了自定义标题(Summary、Test plan、Demo、AI Assistance Disclosure),与模板不一致。
请按模板格式重新组织正文内容即可,实际内容没有问题,只需要调整结构。
— Qwen Code · qwen3.7-max
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ On direction: This is a clear correctness fix in the Gemini→OpenAI schema conversion layer. On approach: The fix is tight and minimal — swapping the 中文说明感谢贡献,@tt-a1i! 模板完整 ✓ 方向:这是 Gemini→OpenAI schema 转换层的一个明确的正确性修复。 方案:修复紧凑且最小化——将 — Qwen Code · qwen3.7-max |
Code ReviewThe fix is clean and correct. The core change replaces a loose guard ( One minor observation: No correctness issues, no security concerns, no scope creep. Test ResultsUnit tests (packages/core, The new test covers the key edge cases: Typecheck ( Real-Scenario TestingN/A — this is a non-visible internal logic fix in the schema conversion layer. The bug manifests only in the API request body when tool parameter schemas contain string-valued length/item constraints. There is no CLI-visible behavior to capture via tmux. The unit tests are the authoritative verification. 中文说明代码审查修复干净且正确。核心改动将宽松的判断( 一个小观察: 没有正确性问题,没有安全隐患,没有范围蔓延。 测试结果单元测试(packages/core, 类型检查:✅ 通过 真实场景测试不适用——这是 schema 转换层内部的不可见逻辑修复。该 bug 仅在工具参数 schema 包含字符串型长度/数量约束时出现在 API 请求体中。没有可通过 tmux 捕获的 CLI 可见行为。单元测试是权威验证。 — Qwen Code · qwen3.7-max |
ReflectionThis is a textbook minimal bug fix. The PR author identified a real correctness bug — The test is well-chosen: four edge cases ( I can't find a simpler path. The fix is correct, minimal, and well-tested for what it is. If I had to maintain this in six months, I'd thank the author. ✅ Approving. 中文说明总结这是一个教科书式的最小化 bug 修复。PR 作者发现了一个真实的正确性 bug—— 测试选择得当:四个边界情况( 找不到更简单的方案。修复正确、最小化,且测试充分。如果六个月后需要维护这段代码,我会感谢作者。 ✅ 批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Verification report — PR #5312 (fixes #5310)Verdict: verified, recommend merge. Small, correctly-scoped logic fix. I reverse-audited the guard, ran the converter suite with a base A/B (the new test fails without the fix), and — the decisive part — drove the actual compiled converter (PR vs base) on real Gemini tool schemas, at both the function level and the full request-path that builds the What the PR does
const numberValue = typeof value === 'string' ? Number(value) : NaN;
if (typeof value === 'string' && value.trim() !== '' && Number.isInteger(numberValue)) {
result[key] = numberValue; // exact value, no parseInt truncation
} else {
result[key] = value; // preserve verbatim
}1. Reverse-audit — logic + edge cases
2. Unit suite + base A/B
3. Real-data A/B on the actual compiled converter ⭐Ran the compiled
And the wire schema the backend actually receives (
So the fix propagates end-to-end to the OpenAI request, not just the helper. The nested case shows the converter recurses and base corrupted nested constraints too.
ConclusionTight, well-tested, no regression to valid integers / numbers / numeric constraints, and verified to carry through to the real OpenAI wire schema. LGTM. 🇨🇳 中文版验证报告(点击展开)✅ 验证报告 — PR #5312(修复 #5310)结论:已验证,建议合并。 小而精准的逻辑修复。我审计了判断逻辑、跑了 converter 测试并与 base 做了 A/B(不打补丁时新测试会失败),并且——最关键的一步——用真实编译后的 converter(PR vs base)在真实 Gemini 工具 schema 上跑了对比,既在函数层面,也在构建发给 OpenAI 后端 PR 做了什么
const numberValue = typeof value === 'string' ? Number(value) : NaN;
if (typeof value === 'string' && value.trim() !== '' && Number.isInteger(numberValue)) {
result[key] = numberValue; // 精确值,无 parseInt 截断
} else {
result[key] = value; // 原样保留
}1. 逆向审计 —— 逻辑 + 边界
2. 单元测试 + base A/B
3. 真实数据 A/B —— 真实编译后的 converter ⭐用真实 Gemini 工具 schema,对比运行编译后的
以及后端实际收到的线上 schema(
所以修复会端到端传到 OpenAI 请求,而不只是辅助函数。嵌套用例还表明 converter 会递归、base 连嵌套约束也会破坏。
结论逻辑严谨、测试充分、对合法整数 / 数字 / 数值约束无回归,并已验证能传到真实的 OpenAI 线上 schema。LGTM。 Verification methodology: reverse-audit of the integer guard ( |
✅ Real-build verification — safe to mergeI built the actual 1. Unit tests — 131/131 on PR head
2. Mutation test — the new test isn't vacuousReverted only 3. Compiled A/B — real
|
| property | input | PRE-FIX | FIXED (this PR) |
|---|---|---|---|
badFloat |
minLength: "1.5" |
1 — silently truncated |
"1.5" — preserved |
blank |
maxLength: " " |
null — parseInt(" ")→NaN→null |
" " — preserved |
junk |
minItems: "10px" |
"10px" |
"10px" (same) |
validStr |
minLength: "5" |
5 |
5 — valid coercion still works |
alreadyNum |
minLength: 3 |
3 |
3 — untouched |
The full-pipeline output is identical to the direct one, so the fix survives convertSchema into the final OpenAI tools[].function.parameters.
raw tmux capture (fixed vs pre-fix)
# FIXED
DIRECT convertGeminiToolParametersToOpenAI:
badFloat minLength="1.5" (string)
blank maxLength=" " (string)
junk minItems="10px" (string)
validStr minLength=5 (number)
alreadyNum minLength=3 (number)
# PRE-FIX
DIRECT convertGeminiToolParametersToOpenAI:
badFloat minLength=1 (number)
blank maxLength=null (number)
junk minItems="10px" (string)
validStr minLength=5 (number)
alreadyNum minLength=3 (number)
Reachability & scope (honest framing)
- This runs on the Gemini-format tool → OpenAI conversion (
convertGeminiToolParametersToOpenAI, called fromconvertGeminiToolsToOpenAI), i.e. when talking to an OpenAI-compatible endpoint. MCP tools take the separateparametersJsonSchemabranch and don't reach this code. - Built-in tools declare numeric length constraints, so they're unaffected — which is exactly why there's no regression (numbers and valid integer-strings are untouched / still coerced). The fix is defensive hardening: it stops a Gemini-format tool that declares a string length (
"1.5", blank) from being silently corrupted (→ 1,→ null) in the outgoing tool schema.
Verdict: LGTM — correct, complete, no regression on numeric/valid constraints. 👍
中文说明(完整对应)
✅ 真实构建验证 —— 可以合并
我在隔离 worktree 里从本 PR 的 head(e089cc2dac)构建了真实的 core,并在 tmux 中对编译后的 dist 转换函数做了 A/B(同一棵树,只把 converter.ts 回退到 base)。本 PR 是无 TUI 的核心数据转换,所以"真实运行"是直接跑实际发布的函数及其真实调用链,而非击键。
1. 单元测试 —— PR head 131/131
npx vitest run converter.test.ts → 131 passed。
2. 变异测试 —— 新测试不是空过场
只把 converter.ts 回退到 base(保留本 PR 的测试)再跑:恰好 1 个测试失败(就是新增那个),其余 130 个全过:
× should not truncate non-integer length constraints
(130 个既有测试仍然全过)
3. 编译产物 A/B —— 真实 dist,在 tmux 中运行
把一个含多种长度约束的 schema 同时喂给被改函数和上一层真实流水线(convertGeminiToolsToOpenAI → 最终 OpenAI tool 对象):
| 属性 | 输入 | PRE-FIX | FIXED(本 PR) |
|---|---|---|---|
badFloat |
minLength: "1.5" |
1 —— 被静默截断 |
"1.5" —— 保留 |
blank |
maxLength: " " |
null —— parseInt(" ")→NaN→null |
" " —— 保留 |
junk |
minItems: "10px" |
"10px" |
"10px"(一致) |
validStr |
minLength: "5" |
5 |
5 —— 合法字符串照常转成数字 |
alreadyNum |
minLength: 3 |
3 |
3 —— 不动 |
整条流水线输出与直接调用一致,说明修复能挺过 convertSchema,进入最终的 OpenAI tools[].function.parameters。
可达性与范围(如实说明)
- 这段代码在 Gemini 格式 tool → OpenAI 的转换上(
convertGeminiToolParametersToOpenAI,由convertGeminiToolsToOpenAI调用),即对接 OpenAI 兼容端点时。MCP tool 走的是另一条parametersJsonSchema分支,不经过这里。 - 内置 tool 的长度约束都是数字,所以不受影响 —— 这也正是无回归的原因(数字与合法整数字符串都不变 / 照常转换)。本修复是防御性加固:防止某个声明了字符串长度(
"1.5"、空白)的 Gemini 格式 tool 在外发 schema 中被静默破坏(→ 1、→ null)。
结论:LGTM —— 正确、完整,对数字/合法约束无回归。👍
What this PR does
Tightens the OpenAI schema conversion in
convertGeminiToolParametersToOpenAIfor the string-valued length/item constraintsminLength,maxLength,minItems, andmaxItems. These fields are now only coerced from a string to a number when the string is non-empty (after trimming) and represents an actual integer. The conversion uses the parsedNumber(...)value directly instead ofparseInt(...).Previously the guard accepted any string that satisfied
!isNaN(Number(value))and then wroteparseInt(value, 10), so a value like"1.5"passed the check and was silently truncated to1(and a whitespace-only string like" "coerced to0). With this change, such non-integer strings are left untouched and the original value is preserved.Numeric constraints such as
minimum,maximum, andmultipleOfare not affected by this change.Why it's needed
convertGeminiToolParametersToOpenAI()checkedNumber(value)but then converted withparseInt(value, 10). A value such as"1.5"passes the numeric check but is written as1, silently changing the schema. Length and item constraints should only be converted when the string represents an integer; otherwise the original value must be preserved rather than truncated. See issue #5310.Reviewer Test Plan
How to verify
Repro: pass a tool parameter schema where a length/item constraint is a non-integer string (e.g.
minLength: "1.5",maxLength: " ",minItems: "10px",maxItems: "1.5") throughconvertGeminiToolParametersToOpenAI.1/0. Valid integer strings still convert to numbers."1.5"was silently written as1.Verification commands (run from
packages/coreunless noted):npx vitest run src/core/openaiContentGenerator/converter.test.ts -t "convertGeminiToolParametersToOpenAI"npx vitest run src/core/openaiContentGenerator/converter.test.tsnpm run typecheck --workspace=packages/corenpm run build --workspace=packages/corenpm run lint -- --fix packages/core/src/core/openaiContentGenerator/converter.ts packages/core/src/core/openaiContentGenerator/converter.test.tsnpx prettier --check packages/core/src/core/openaiContentGenerator/converter.ts packages/core/src/core/openaiContentGenerator/converter.test.tsgit diff --checkEvidence (Before & After)
N/A — non-visible logic fix; covered by the unit tests above.
Tested on
✅ tested ·⚠️ not tested · N/A
Environment (optional)
Unit tests only (npm workspaces).
Risk & Scope
convertGeminiToolParametersToOpenAIinpackages/core. Behavior change is limited to how string-valuedminLength/maxLength/minItems/maxItemsare handled (non-integer strings are now preserved instead of truncated).minimum/maximum/multipleOf), no UI changes.Linked Issues
Fixes #5310
中文说明
这个 PR 做了什么
收紧了
convertGeminiToolParametersToOpenAI中对字符串型长度/数量约束minLength、maxLength、minItems、maxItems的转换逻辑。现在只有当字符串在去除首尾空白后非空、且表示一个真正的整数时,才会把它从字符串转换为数字;转换时直接使用解析得到的Number(...)值,而不再使用parseInt(...)。此前的判断条件接受任何满足
!isNaN(Number(value))的字符串,然后写入parseInt(value, 10),因此像"1.5"这样的值能通过检查并被静默截断为1(而像" "这样的纯空白字符串会被转换为0)。改动之后,这类非整数字符串保持不变,原始值得以保留。minimum、maximum、multipleOf等数值约束不受此改动影响。为什么需要它
convertGeminiToolParametersToOpenAI()先检查Number(value),却用parseInt(value, 10)进行转换。像"1.5"这样的值能通过数值检查,但会被写成1,从而静默地改变了 schema。长度和数量约束只应在字符串表示整数时才转换;否则必须保留原始值,而不是截断它。详见 issue #5310。评审者测试计划
如何验证
复现:构造一个工具参数 schema,其中长度/数量约束为非整数字符串(例如
minLength: "1.5"、maxLength: " "、minItems: "10px"、maxItems: "1.5"),传入convertGeminiToolParametersToOpenAI。1/0。合法的整数字符串仍会转换为数字。"1.5"被静默写成1。验证命令(除非另有说明,均在
packages/core目录下执行):npx vitest run src/core/openaiContentGenerator/converter.test.ts -t "convertGeminiToolParametersToOpenAI"npx vitest run src/core/openaiContentGenerator/converter.test.tsnpm run typecheck --workspace=packages/corenpm run build --workspace=packages/corenpm run lint -- --fix packages/core/src/core/openaiContentGenerator/converter.ts packages/core/src/core/openaiContentGenerator/converter.test.tsnpx prettier --check packages/core/src/core/openaiContentGenerator/converter.ts packages/core/src/core/openaiContentGenerator/converter.test.tsgit diff --check证据(前后对比)
N/A — 这是不可见的逻辑修复;已由上述单元测试覆盖。
测试平台
✅ 已测试 ·⚠️ 未测试 · N/A
环境(可选)
仅单元测试(npm workspaces)。
风险与范围
packages/core中convertGeminiToolParametersToOpenAI的长度/数量约束分支。行为变更仅限于字符串型minLength/maxLength/minItems/maxItems的处理方式(非整数字符串现在会被保留而非截断)。minimum/maximum/multipleOf),没有 UI 变更。关联 Issue
Fixes #5310
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.