Repository navigation
fix(core): coerce numeric string params in SchemaValidator for MCP tools - #4967
Merged
Merged
Conversation
LLMs frequently emit numeric parameters as strings (e.g. `{"depth": "3"}`
instead of `{"depth": 3}`), which strict MCP servers like Playwright reject
with schema validation errors ("params/depth must be number"). The
SchemaValidator already coerces boolean strings and stringified JSON but
was missing numeric coercion.
Add `fixNumericValues()` that converts string values to integers/numbers
when the schema expects integer/number but does not accept string,
consistent with the existing `fixBooleanValues` pattern.
Co-Authored-By: Qwen-Coder (Qwen Code) <[email protected]>
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
|
@qwen-code-ci-bot pls fix it |
- Skip coercion for decimal strings when schema only accepts integer - Prefer parseFloat when number type is accepted (fixes anyOf[integer, number]) - Add tests for integer-only decimal rejection, anyOf coercion, and array elements Co-Authored-By: Qwen-Coder <[email protected]>
wenshao
requested changes
Jun 14, 2026
Collaborator
|
@qwen-code-ci-bot pls fix it |
Replace the syntactic `hasDecimal` guard with a semantic `num % 1 !== 0`
check so whole-number decimal strings ("3.0", "-10.0") coerce to integers
while genuine non-integers ("5.5") are still left for the LLM to
self-correct. Python-based models commonly serialize float(3) as "3.0".
Co-Authored-By: Qwen-Coder <[email protected]>
wenshao
requested changes
Jun 15, 2026
| if (wantsInteger && !wantsNumber && num % 1 !== 0) continue; | ||
|
|
||
| const parsed = wantsNumber ? num : parseInt(trimmed, 10); | ||
| if (Number.isFinite(parsed)) { |
Collaborator
There was a problem hiding this comment.
[Critical] Silent precision loss on large integers. The regex /^-?\d+(\.\d+)?$/ accepts arbitrarily long digit strings, and parseInt silently rounds values exceeding Number.MAX_SAFE_INTEGER (2^53 - 1). Number.isFinite() still passes on the rounded value, so the corrupted number is written into data.
Example: "9999999999999999999" → parseInt yields 10000000000000000000 (off by 1). MCP tool schemas commonly declare 64-bit identifiers (Snowflake IDs, database PKs) as type: "integer". The LLM's correct string representation gets silently corrupted before dispatch — the MCP server receives the wrong ID and operates on the wrong entity.
Suggested change
| if (Number.isFinite(parsed)) { | |
| const parsed = wantsNumber ? num : parseInt(trimmed, 10); | |
| if (wantsInteger && !Number.isSafeInteger(parsed)) continue; | |
| if (Number.isFinite(parsed)) { | |
| data[key] = parsed; | |
| } |
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Well-designed fixNumericValues coercion pass for SchemaValidator. Correctly handles: string→integer, string→number, negative strings, nested objects, arrays of integers, whole-number decimals for integer schemas ("3.0" → 3), and refuses when schema also accepts string. Decimal string for integer-only schema correctly rejected. 12 comprehensive tests covering edge cases. CI green on tests, but review-pr workflow check is failing. Downgraded from Approve to Comment: CI review-pr check failing. — claude-opus-4-6 via Qwen Code /review
wenshao
reviewed
Jun 15, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No new review findings in R3 (same commit as R2). 9 agents + reverse audit: 0 high-confidence findings. tsc 0, eslint 0, 56/56 tests pass. Downgraded from Approve to Comment: CI failing (review-pr). — qwen3.7-max via Qwen Code /review
Collaborator
|
@qwen-code /triage |
Collaborator
✅ Local verification — build + real tests (Linux)Built the head commit Environment: Linux (Debian 13, kernel 6.12), Node v22.22.2, npm 10.9.7. Worktree at Results
A/B — the fix in action (real
|
| Input string | Schema | Outcome |
|---|---|---|
"3" / "-10" / "007" |
integer | ✅ → 3 / -10 / 7 |
"5.5" |
number | ✅ → 5.5 |
"3.0" |
integer | ✅ → 3 (whole-number decimal) |
" 3 " |
integer | ✅ → 3 (surrounding whitespace tolerated) |
["1","2"] |
array items:integer |
✅ → [1,2]; also coerces deeply nested objects |
"42" |
anyOf[string,integer] |
✅ stays "42" (string is accepted → untouched) |
"5.5" |
integer-only | ✅ not coerced → validation fails (LLM self-corrects) |
"+3" / "1e3" / "0x10" / "Infinity" / "NaN" / "3." / ".5" / "abc" |
integer/number | ✅ not coerced (conservative regex) → validation fails |
Notes / two documented edges (neither blocks merge)
- Huge-integer precision:
"99999999999999999999"coerces to1e20(precision lost) and still validates as an integer. Irrelevant for typical MCP params (depths, ports, indices, timeouts); only relevant if a >2^53 ID is modeled asinteger(usually such IDs aretype:string). Pre-PR it was simply rejected, so this is a behavior change in a rare corner. - Tuple arrays not coerced: when
itemsis an array (tuple validation, e.g.items:[{integer},{integer}]), elements are left as strings (getAcceptedTypesfinds no type on the array) → validation still fails. A documented limitation, not a regression; the commonitems:{integer}form works. - Design is safe: coercion runs only as a repair pass after the first
validate()fails, mutating in place exactly like the existingfixBooleanValues/fixStringifiedJsonValues— so already-valid payloads are untouched and there is zero added cost on the happy path.
Recommendation: functionally correct, well-tested, conservative, and safe to merge on these results. The two edges above are worth a mention in the changelog but don't block.
中文版（点击展开）
✅ 本地验证 —— 构建 + 真实测试（Linux）
在独立 git worktree 中以全新 npm ci 构建了头提交 af9aab70，并在 tmux 中跑了真实测试。所有检查通过，三个真实调用方无回归，且与 merge-base 的 A/B 对比证明该修复确实改变了可观察行为。 改动范围小、克制。
环境： Linux（Debian 13，内核 6.12），Node v22.22.2，npm 10.9.7。Worktree 在 af9aab70，与 main 的 merge-base = 4270beb4。
结果
| 检查项 | 命令 | 结果 |
|---|---|---|
| PR 单测 | core → vitest run src/utils/schemaValidator.test.ts |
✅ 56 通过（含 13 个新增 coercion 用例） |
| 调用方回归 | vitest run tools.test.ts ripGrep.test.ts sideQuery.test.ts |
✅ 89 通过（SchemaValidator.validate 的 3 个真实调用方） |
| 类型检查 | core → tsc --noEmit |
✅ 干净 |
| Lint | 对 2 个改动文件跑 eslint |
✅ 干净 |
| 构建 | core → npm run build |
✅ 干净（dist 中含 fixNumericValues） |
| 边界探针 | 10 个对抗用例 | ✅ 10/10（见下表） |
| 与 merge-base A/B | 同一 payload，有/无本 PR | ✅ 行为差异已确认 |
A/B —— 修复实际效果（真实 SchemaValidator.validate）
一个真实的严格 MCP schema（Playwright 风格，draft-2020-12），其数值参数被 LLM 以字符串形式给出：
schema: { index:integer, timeout:number, position:{x:integer,y:integer}, clickCounts:integer[], selector:string }
payload: { selector:"#submit", index:"2", timeout:"5.5", position:{x:"10",y:"20"}, clickCounts:["1","2","3"] }
修复前（merge-base）: validate() → "params/index must be integer" ← 被拒；参数仍是字符串
修复后（PR head）: validate() → null ← 通过
index→2, timeout→5.5, position→{x:10,y:20}, clickCounts→[1,2,3]；selector 保持 "#submit"
用 PR 自带的测试套件去跑 merge-base 的 validator，结果是 9 失败 | 47 通过 —— 这 9 个失败恰好是新增的“应 coerce”用例，而 4 个“反向”用例（不应 coerce / 本就合法 的护栏）在两侧都通过。在 PR head 上 56 个全通过。这干净地证明了 coercion 是真正新增的（且护栏确实起作用）。
边界行为（对抗探针，已在 PR head 确认）
| 输入字符串 | Schema | 结果 |
|---|---|---|
"3" / "-10" / "007" |
integer | ✅ → 3 / -10 / 7 |
"5.5" |
number | ✅ → 5.5 |
"3.0" |
integer | ✅ → 3（整数值的小数写法） |
" 3 " |
integer | ✅ → 3（容忍首尾空白） |
["1","2"] |
array items:integer |
✅ → [1,2]；深层嵌套对象同样会 coerce |
"42" |
anyOf[string,integer] |
✅ 保持 "42"（接受 string → 不动） |
"5.5" |
仅 integer | ✅ 不 coerce → 校验失败（让 LLM 自我纠正） |
"+3" / "1e3" / "0x10" / "Infinity" / "NaN" / "3." / ".5" / "abc" |
integer/number | ✅ 不 coerce（保守正则）→ 校验失败 |
说明 / 两个已记录的边角（均不阻塞合并）
- 超大整数精度：
"99999999999999999999"会 coerce 成1e20（丢精度），并仍按 integer 通过校验。对常见 MCP 参数（depth、port、index、timeout）无影响；只有当 >2^53 的 ID 被建模为integer时才相关（这类 ID 通常是type:string）。修复前是直接被拒，所以这是一个少见角落里的行为变化。 - 元组数组不 coerce： 当
items是数组（元组校验，如items:[{integer},{integer}]）时，元素仍是字符串（getAcceptedTypes在该数组上找不到 type）→ 校验仍失败。这是已记录的限制，并非回归；常见的items:{integer}形式工作正常。 - 设计安全： coercion 仅在第一次
validate()失败后作为修复 pass 运行，原地修改，和已有的fixBooleanValues/fixStringifiedJsonValues完全一致 —— 因此已合法的 payload 不受影响，正常路径零额外开销。
结论： 功能正确、测试充分、保守、基于以上结果可安全合并。上面两个边角值得在 changelog 里提一句，但不阻塞。
wenshao
approved these changes
Jun 15, 2026
wenshao
pushed a commit
that referenced
this pull request
Jun 17, 2026
…4793) * fix(schemaValidator): coerce non-string tool params + harden coercion pipeline Self-hosted LLMs (LMStudio, sglang, vllm) sometimes return number/boolean values for tool params whose schema expects a string (e.g. old_string, content), causing SchemaValidator to reject them outright. Add fixStringValues() — the string-direction counterpart to fixBooleanValues / fixNumericValues — coercing number/boolean/bigint → string only where the schema accepts string. The coercion pipeline is schema-aware and recurses through anyOf/oneOf/allOf, $ref, prefixItems, and additionalProperties, with prototype-pollution guards and depth limits. Coercion runs only after initial validation fails, and each pass skips values whose current type is already accepted (no round-trips). validate() now runs four passes in order: 1. fixBooleanValues "true"/"false" → boolean 2. fixStringValues number/boolean → string (this change) 3. fixStringifiedJsonValues 'JSON' → array/object 4. fixNumericValues "3"/"5.0" → number (#4967, reconciled) Passes 2 and 4 are mutually exclusive per field (string-accepted vs. string-rejected), so neither can undo the other. Addresses review 4496484959: - [Critical] Memoize getAcceptedTypes with a WeakMap keyed on the *resolved* schema object, collapsing branching $ref composition schemas from O(2^depth) to O(depth) (prevents a compact-schema DoS that hung the CLI for minutes). Regression test included. - [Suggestion] Add debugLogger.debug to the nested-array coercion sites. Rebased onto the latest main (was conflicting with #4967). The two unrelated core-suite failures seen locally (anthropicContentGenerator User-Agent, coreToolScheduler truncation) are environmental — this build identifies as 'claude-cli' rather than 'QwenCode' — and pass in QwenLM CI. Closes #2512. Supersedes #2512. Co-Authored-By: Claude <[email protected]> * test(core): use non-coercible value in truncation retry-loop test fixStringValues (prior commit) now coerces { value: 123 } → { value: "123" }, so the "should keep retry counts stable when truncation guidance is toggled" test no longer fails validation and getLastErrorMessage() returns undefined, breaking the toContain assertion in CI. Switch its three turns to { value: {} } (an object, which fixStringValues intentionally does not coerce) — the same change already applied to the three sibling retry-loop tests in this suite. Assertions are unchanged. This was previously misdiagnosed in the prior commit message as a local 'claude-cli' branding issue; it reproduces in QwenLM CI because the coercion is real. Co-Authored-By: Claude <[email protected]> * fix(schemaValidator): restore union-type string guard in fixBooleanValues fixBooleanValues coerced "true"/"false" strings to booleans even when the field's schema also accepted string (e.g. anyOf: [boolean, string]). That silently rewrote a legitimate string value into a boolean, corrupting the tool call — a regression vs main, which skips boolean coercion when string is also accepted. Restore the `!accepted.has('string')` guard across all three boolean coercion paths (scalar field, uniform array items, prefixItems tuple elements), mirroring the guard fixStringValues and fixStringifiedJsonValues already use. Reported by @wenshao in PR #4793 review. Tests: rewrote the two cases that encoded the old over-coercion to assert preservation, and added array/tuple coverage for the new guards. Co-Authored-By: Claude <[email protected]> --------- Co-authored-by: Claude <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds numeric string coercion (
"3"→3) toSchemaValidator.validate(), filling a gap alongside the existing boolean and stringified-JSON coercions. When the schema expectsinteger/numberbut does not acceptstring, clean numeric strings are parsed in-place before re-validation. The newfixNumericValues()helper recurses into nested objects and respectsanyOf/oneOfunion types, matching the style offixBooleanValues().Why it's needed
LLMs frequently emit numeric parameters as strings (e.g.
{"depth": "3"}instead of{"depth": 3}). Strict MCP servers like Playwright validate JSON Schema types and reject these calls outright. The SchemaValidator already coerced booleans and JSON arrays/objects but was missing the numeric case.Real-world session evidence (session
65c4f5fe)In a real user session using Playwright MCP with qwen3.7-max, 4 out of ~20 Playwright tool calls failed due to numeric type mismatches, forcing the model into retry loops and significantly degrading UX:
browser_snapshot{"depth": "3"}params/depth must be numberbrowser_wait_for{"time": "5"}params/time must be numberbrowser_wait_for{"time": "5"}params/time must be numberbrowser_evaluate{"expression": "..."}(missingfunction)params must have required property 'function'The first three are type-mismatch errors that this PR fixes. The fourth (
browser_evaluatewrong parameter name) is a separate issue not addressed here.Additionally, the session also hit other Playwright errors (browser lock contention, file access sandbox restrictions, CSS selector parse failures) — those are unrelated to parameter coercion.
Reviewer Test Plan
How to verify
npx vitest run packages/core/src/utils/schemaValidator.test.ts— all 52 tests should pass (9 new tests for numeric coercion).stringis accepted), nested objects, draft-2020-12 schemas.Evidence (Before & After)
N/A — non-UI change, covered by unit tests.
Tested on
Environment (optional)
N/A — unit tests only.
Risk & Scope
"42"to a number — mitigated by only coercing when the schema does NOT acceptstring.browser_evaluateerror (params must have required property 'function') seen in the same session is a different issue (wrong parameter name, not a type mismatch).Linked Issues
Closes #4966
中文说明
做了什么
在
SchemaValidator.validate()中新增数字字符串强转（"3"→3），补齐已有的布尔值和 JSON 字符串强转逻辑。当 schema 期望integer/number且不接受string时，将合法的数字字符串原地解析后重新校验。新增的fixNumericValues()支持递归嵌套对象，并正确处理anyOf/oneOf联合类型，与fixBooleanValues()风格一致。为什么需要
LLM 经常把数字参数以字符串形式发出（如
{"depth": "3"}而非{"depth": 3}）。严格的 MCP 服务端（如 Playwright）按 JSON Schema 类型校验后直接拒绝。SchemaValidator 已有布尔值和 JSON 数组/对象的强转，唯独缺少数字类型。真实会话证据（session
65c4f5fe）在一个使用 Playwright MCP + qwen3.7-max 的真实用户会话中，约 20 次 Playwright 工具调用中有 4 次因数字类型不匹配而失败，导致模型反复重试，严重影响体验：
browser_snapshot{"depth": "3"}params/depth must be numberbrowser_wait_for{"time": "5"}params/time must be numberbrowser_wait_for{"time": "5"}params/time must be numberbrowser_evaluate{"expression": "..."}(缺少function)params must have required property 'function'前三个是本 PR 修复的类型不匹配错误。第四个（
browser_evaluate参数名错误）是另一个问题，不在本 PR 范围内。此外，该会话还遇到了其他 Playwright 错误（浏览器锁竞争、文件访问沙箱限制、CSS 选择器解析失败）——这些与参数强转无关。
验证方式
npx vitest run packages/core/src/utils/schemaValidator.test.ts— 52 个测试全部通过（含 9 个新增的数字强转测试）。风险与范围
"42"转为数字 — 通过仅在 schema 不接受string时才强转来规避。browser_evaluate的params must have required property 'function'错误是另一个问题（参数名错误，非类型不匹配）。