fix(core): avoid LM Studio grammar failure - #10275
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: real and observed, not theoretical — this is the same failure as #10065 ( Direction: aligned — local models through the OpenAI-compatible endpoint are a supported path, and this failure blocks every conversation on grammar-based backends (LM Studio, llama-swap), so it is a P2-level blocker for those users. No comparable entry in peer-CLI changelogs, but the area is clearly relevant. Size: core path ( Approach: the diff itself is minimal and focused — one constraint removed, one regression test, no drive-by edits. But before the code review, one substantive question about whether this is the right constraint to remove. The #10065 analysis fingered Risk: no elevated risk signals — neither changed file matches the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:真实且已观测到,不是理论问题——这与 #10065 是同一个故障( 方向:对齐——OpenAI 兼容端点跑本地模型是受支持的路径,而这个故障会让基于 grammar 的后端(LM Studio、llama-swap)的每一次对话都失败,对这些用户属于 P2 级阻断。同类 CLI 的 changelog 中没有直接对应条目,但该领域显然相关。 规模:核心路径( 方案:diff 本身最小且聚焦——删一个约束、加一个回归测试、无顺手改动。但在进入代码审查前,有一个实质性问题:删的是不是对的约束?#10065 的分析指向的是 风险:无升级风险信号——两个改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent take before reading the diff: given the #10065 record, I would have started by capturing the request body LM Studio rejects and bisecting the tool set (the Judged on its own terms, the diff is clean:
The open question is not code quality but sufficiency — whether this is the construct the llama.cpp grammar converter rejects (Stage 1 covers it). Not verified: that the grammar failure is gone after this change on any environment beyond the author's (macOS only, per the Tested-on table); CI has no LM Studio to reproduce it. That sentence is the trigger, so the remedy goes with it:
CI test evidenceThe PR's own CI has not run: both
The added unit test ( 中文说明代码审查读 diff 之前的独立判断:结合 #10065 的记录,我会先抓取 LM Studio 实际拒绝的请求体,并对工具集做二分(该 issue 记录的 就其本身而言,diff 是干净的:
悬而未决的不是代码质量,而是充分性——这是否正是 llama.cpp grammar 转换器拒绝的结构(见 Stage 1)。未验证:除作者自身环境(按 Tested-on 表仅 macOS)之外,该 grammar 故障在本改动后是否消失;CI 没有 LM Studio 可以复现。这句话本身就是触发条件,所以补救手段一并给出:
CI 测试证据该 PR 自身的 CI 尚未运行:两个 (表格见上方英文部分) 新增的单元测试( — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean, runtime-safe one-line fix for a real observed failure, but whether it removes the construct the grammar converter actually rejects is unresolved, it overlaps maintainer work already in flight on #10065, and no CI has ever run on this commit — needs a human call. Stepping back: the problem is real and multiply corroborated — #10065 carries a full reproduction with a direct-API control, @yiliang114 reproduced it independently against a controlled endpoint, and users hit it on both LM Studio and llama-swap. Judged as code, this diff is the smallest it could be: one advisory constraint removed, one regression test, and I verified nothing at runtime ever depended on the cap ( What keeps this from an approval is not the code but the fit:
⏸️ Deferring to @yiliang114 — you reproduced #10065 and own the fix path, so this is yours to call. Suggested path: (1) connect this PR to #10065 and decide whether the one-line removal is a useful stopgap or should wait for the schema-side fix; (2) approve the fork's workflow runs so CI (including the new test) lands on 中文说明置信度:3/5 —— 对一个真实观测到的故障而言,这是干净且运行时安全的一行修复;但它移除的是否正是 grammar 转换器实际拒绝的结构仍未解决,与 #10065 上正在进行的维护者工作重叠,且该提交从未跑过 CI——需要人来拍板。 退一步看:问题是真实的且有多方佐证——#10065 有完整复现和直连 API 对照,@yiliang114 在受控端点上独立复现,用户在 LM Studio 和 llama-swap 上都遇到了。就代码本身而言,这个 diff 已经是最小形态:删一个纯引导性约束、加一个回归测试;我也核实了运行时从未依赖这个上限( 阻止批准的不是代码,而是契合度:
⏸️ 转交 @yiliang114 —— #10065 由您复现、修复路径由您掌握,此事应由您定夺。建议路径:(1)把本 PR 与 #10065 连通,判断这一行删除是有用的临时缓解,还是应等待 schema 一侧的修复;(2)批准 fork 的工作流运行,让 CI(含新测试)落在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Not explored to full depth (tool budget reached): "agent 6b": end-to-end verification of whether LM Studio/llama.cpp accepts the post-change schema with the remaining nested maxLength constraints (requires a live LM Studio….
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it.
— qwen3.8-max via Qwen Code /review (v0.22.2)
| summary: { | ||
| type: 'string', | ||
| maxLength: 2000, | ||
| description: 'One sentence stating the defect.', |
There was a problem hiding this comment.
[Suggestion] This removes the 2,000-character cap from the schema that BaseDeclarativeTool.validateToolParams validates against (Ajv enforces maxLength), so client-side enforcement of summary length is dropped for every provider, not just the LM Studio endpoint this targets. Verified by probe against the real tool and validator:
summary len=2500: PR schema -> ACCEPTED | base schema -> REJECTED
The codebase already has a layer built for exactly this class of OpenAI-compatible gateway breakage: relaxSchemaForFunctionCalling (packages/core/src/utils/schemaConverter.ts), applied to tool schemas in convertGeminiToolsToOpenAI, whose design note (PR 7315) says the constraint is relaxed on the wire only while client-side validateToolParams still enforces the full source schema. Also note the five sibling constraints in this same item schema (id 64, file 4096, failureScenario 4000, category 64, outcomeNote 1000, plus maxItems: 50) remain, so the change neither preserves the cap where it works nor closes the nested-constraint class that caused this failure.
Consider keeping maxLength: 2000 here and stripping it only at the wire layer for OpenAI-compatible endpoints (extending relaxSchemaForFunctionCalling per the PR 7315 pattern), or moving the cap into validateToolParamValues so client-side enforcement survives — and updating the new test to pin the wire shape accordingly. If the wire-relaxation route is taken, please add the converter/schemaConverter test asserting the relaxed report_findings wire schema carries no maxLength on summary while the source schema keeps it, and confirm it goes red when the relaxation step is removed.
— qwen3.8-max via Qwen Code /review (v0.22.2)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
Not reviewed: reverse audit — stopped before round 1 by the review time budget.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/report-findings.ts:163 — [probe] compaction invariant comment falsified — the retention bound now rides entirely on MAX_RETAINED_AGENT_FIELD_CHARS (measured eviction boundary 22,642/22,643)
— qwen3.8-max via Qwen Code /review (v0.22.2)
| summary: { | ||
| type: 'string', | ||
| maxLength: 2000, | ||
| description: 'One sentence stating the defect.', |
There was a problem hiding this comment.
[Suggestion] R1-1: Still standing — this code is unchanged since the previous round. Removing maxLength: 2000 from the source schema drops client-side (Ajv) enforcement of summary length for every provider, not just the LM Studio endpoint this targets: BaseDeclarativeTool.validateToolParams validates exactly this schema, and the wire-only relaxation layer relaxSchemaForFunctionCalling (packages/core/src/utils/schemaConverter.ts) — the PR 7315 pattern of relaxing constraints on the wire while keeping client-side enforcement — was not used.
The five sibling constraints in this same item schema (id 64, file 4096, failureScenario 4000, category 64, outcomeNote 1000) plus maxItems: 50 remain, so the change neither preserves the cap where it works nor closes the nested-constraint class that caused the LM Studio grammar failure. Round 1 verified by probe against the real tool and validator: summary len=2500: PR schema -> ACCEPTED | base schema -> REJECTED.
Consider keeping maxLength: 2000 here and stripping it only at the wire layer for OpenAI-compatible endpoints (extending relaxSchemaForFunctionCalling per the PR 7315 pattern), or moving the cap into validateToolParamValues so client-side enforcement survives — and updating the new test to pin the wire shape accordingly. If the wire-relaxation route is taken, add the converter/schemaConverter test asserting the relaxed report_findings wire schema carries no maxLength on summary while the source schema keeps it, and confirm it goes red when the relaxation step is removed.
— qwen3.8-max via Qwen Code /review (v0.22.2)
What this PR does
Removes the length constraint from review finding summaries in the tool schema.
Why it is needed
LM Studio using llama.cpp fails before generation when this nested schema constraint is present. Removing it keeps the review tool available and allows new conversations to start normally.
Reviewer Test Plan
How to verify
Use Qwen Code with an LM Studio OpenAI-compatible endpoint and start a new conversation. A simple prompt such as
Hi qwenshould receive a response instead of a grammar parsing error.Evidence (Before & After)
Before: LM Studio returned
Failed to initialize samplers: failed to parse grammar.After: the same request succeeds when the summary length constraint is absent.
Tested on
Environment (optional)
Tested with Qwen Code 0.22.2 against LM Studio’s OpenAI-compatible API using a Qwen 27B model.
Risk & Scope
Linked Issues
None.