fix(core): normalize tool schemas for grammar-based providers - #10080
fix(core): normalize tool schemas for grammar-based providers#10080yiliang114 wants to merge 25 commits into
Conversation
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with evidence. #10065 carries a full reproduction — LM Studio 0.4.21 rejecting every request with Direction: aligned. llama.cpp-based runtimes (LM Studio) are a supported OpenAI-compatible provider path, and because llama.cpp compiles all tool schemas into one grammar, one valid-but-unsupported shape breaks the entire request — a real, reported user failure, not a theoretical one. Claude Code CHANGELOG: no direct reference found, but the area (local/OpenAI-compatible provider compatibility) is clearly relevant. Size: core paths touched. 108 production lines (converter.ts 9, schemaConverter.ts 43, schemaValidator.ts 56), 181 test lines, 27 docs lines — no thresholds in play. Approach: scope feels right, and it matches the minimal version of this fix: relax only the outbound wire copy, at the single OpenAI conversion boundary, gated on "the source schema is still enforced locally" so no constraint is dropped from both layers at once. The original schema keeps driving client-side Risk: one high-risk-path match — Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,证据充分。#10065 提供了完整复现——LM Studio 0.4.21 以 方向:对齐。基于 llama.cpp 的 runtime(LM Studio)是受支持的 OpenAI-compatible provider 路径;由于 llama.cpp 会把所有工具 schema 编译成一个 grammar,任何一个合法但不受支持的形态都会导致整个请求失败——这是真实的、有报告的用户故障,不是理论问题。Claude Code CHANGELOG:未找到直接引用,但该领域(本地 / OpenAI-compatible provider 兼容性)明显相关。 规模:触及核心路径。生产代码 108 行(converter.ts 9 行、schemaConverter.ts 43 行、schemaValidator.ts 56 行),测试 181 行,文档 27 行——未触及任何阈值。 方案:范围合理,且就是该修复的最小形态:只在唯一的 OpenAI 转换边界放宽出站 wire 副本,并以"本地校验仍能强制执行原始 schema"为门槛,确保不会有任何约束在两层同时失效。原始 schema 继续驱动客户端 风险:命中一个高风险路径—— 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code review — no blockersThe approach is the right one and the diff is surgical. What I verified against the code at this head:
Test evidence — this PR's own CI at the reviewed commitUnit suite is green on the head commit; the macOS/Windows unit legs and the sandboxed CLI integration leg are skipped — the same fork-PR pattern noted in earlier rounds, not something this diff caused.
From the ubuntu unit job log — the three changed suites ran and passed at this head: Sandboxed verification would settle part of the central claim: 中文说明代码审查——无阻塞项方案正确,diff 精准。在当前 head 上核实到的内容:
测试证据——本 PR 自身在所审 commit 上的 CI单测套件在 head commit 上为绿;macOS/Windows 单测与带沙箱的 CLI 集成腿被跳过——与之前各轮记录的 fork PR 既有模式一致,并非本 diff 导致。上方表格中的 CI 结论来自 GitHub 官方 check 数据;ubuntu 单测任务日志显示三个改动的测试套件均在该 head 上运行并通过(71 个测试文件全部通过,1917 通过 / 11 跳过)。 沙箱验证可以落实核心主张的一部分: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — solid, surgical, well-tested; the only remaining gap is live llama.cpp evidence, which CI cannot produce. This landed in good shape. The diff is the minimal version of the fix I would have proposed: relax only the wire copy, at the single OpenAI conversion boundary, and only when local validation still enforces the source schema — so no constraint ever disappears from both layers. The Context for anyone reading the thread: the current diff is only the schema-normalization phase. The earlier rounds of findings (empty-allowlist handling in config/permissions/scheduler) targeted code that is no longer in this PR — that phase landed separately, and the standing CHANGES_REQUESTED reviews predate this head and this diff shape. Reviewing the seven files here as they stand, I found nothing blocking; the two minor observations are in the Stage 2 note. CI is green at the reviewed commit (ubuntu unit suite, no-AK integration, desktop shell, web-shell smoke, secret scan). The skipped macOS/Windows/integration legs are the fork-PR pattern seen all along this PR, not a signal from this change. The honest caveat stands: nobody has watched LM Studio 0.4.21 accept a relaxed request yet — the author had no endpoint and the runners have none. That makes the first real smoke against #10065's repro the one thing left to do, not a reason to hold a clearly-scoped, safely-gated fix. Approving, pinned to the reviewed commit. 中文说明置信度:4/5 —— 扎实、精准、测试充分;唯一的缺口是真实 llama.cpp 环境下的证据,而这是 CI 无法提供的。 这个 PR 的状态很好。diff 就是我会提出的最小修复形态:只在唯一的 OpenAI 转换边界放宽 wire 副本,并且只在本地校验仍强制执行原始 schema 时才放宽——任何约束都不会在两层同时消失。 给阅读此线程的人的背景:当前 diff 只包含 schema 规范化阶段。更早各轮的发现(config/permissions/scheduler 中的空 allowlist 处理)针对的代码已不在本 PR 中——那个阶段已另行合入,现存的 CHANGES_REQUESTED 评审早于当前 head 与当前 diff 形态。就这 7 个文件的现状审查,未发现阻塞项;两条次要观察见 Stage 2 评论。 所审 commit 上 CI 为绿(ubuntu 单测、no-AK 集成、desktop shell、web-shell smoke、secret scan)。被跳过的 macOS/Windows/集成腿是本 PR 一路以来的 fork PR 既有模式,不是本次改动的信号。诚实的保留意见仍在:还没有人真正看到 LM Studio 0.4.21 接受放宽后的请求——作者没有 endpoint,runner 也没有。因此按 #10065 复现步骤做第一次真实 smoke 是唯一剩下的事,但这不足以成为扣留一个范围清晰、门槛安全的修复的理由。 已批准,锚定在所审 commit。 — Qwen Code · qwen3.8-max Reviewed at |
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
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查:反向审计——评审时间预算不足,未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.22.0)
…ion messages Address review feedback on the empty coreTools allowlist semantic (QwenLM#10065): - Gate ToolRegistry.registerTool on an explicitly empty coreTools allowlist so MCP-discovered tools — both the legacy McpClient.discover() path and the pooled SessionMcpView.applyTools path — are not registered and advertised under `tools.core: []` only to be rejected at the runtime gate (advertised-then-rejected). - Attribute scheduler rejections to the empty coreTools allowlist instead of the misleading "permission was declined" text or the permissions.allow remediation advice that can never succeed while the empty gate is armed. - Treat a bare `--core-tools` flag (yargs yields []) as absent in loadCliConfig so a forgotten flag value cannot silently tool-free a session; settings `tools.core: []` keeps the empty-allowlist semantic. - Update the stale JSDoc/docstring/docs statements that claimed non-core tools and structured_output bypass the allowlist unconditionally — an explicitly empty list disables every tool. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Closeout for the 08:15Z review round on 9545e63 — all 6 threads verified and reconciled (single non-force push 9545e63..7e7a12b):
Verification: tool-registry + permission-manager suites 470/470, coreToolScheduler 380 pass (4 failures are pre-existing "Plan shell routing" cases, reproduced on pristine 9545e63), cli config.test.ts 358/358; core+cli tsc clean, eslint+prettier clean on all 8 touched files. All 6 threads replied + resolved. |
doudouOUC
left a comment
There was a problem hiding this comment.
Re-review: fix commit verified (7e7a12b)
The previous CHANGES_REQUESTED round (at 9545e63) raised 6 findings. I verified the fix commit at HEAD 7e7a12b against each one:
Previous findings — all resolved
| # | Finding | Verdict |
|---|---|---|
| R1-1 (C) | MCP tools bypass the empty coreTools allowlist | Fixed — guard at covers the three MCP paths (legacy , pooled , ). The optional-call + typeof-function guard is consistent with the project's shim-safety pattern. |
| R1-2 (S) | Bare silently disables all tools | Fixed — in treats an argv-sourced as absent, while still passes through. Three new config tests pin the three-way semantics. |
| R1-3 (S) | Misleading "permission was declined" attribution | Fixed — adds an branch before the advice, producing a message that names . Two new tests verify both the covered and uncovered tool paths. |
| R1-4 (S) | Four stale doc statements | Fixed — JSDoc, docstring, comment, and all state the empty-list exception. |
| R1-5 (S) | advice can never succeed | Fixed — the empty-allowlist branch in is ordered before the allow-advice fallback, and the check in is ordered before the non-core exemption. |
| R1-6 (S) | SDK transports drop an explicitly empty list | Deferred to #10138 — spans three SDK packages + a forward-vs-document decision. Reasonable. |
New HEAD review
The fix commit is clean, correctly scoped, and well-tested (8 new tests across 4 test files). No new issues found.
Consistency checks:
- , , and the empty-allowlist gate are mutually consistent for all three states (, empty , non-empty ).
- The bare-flag → absent mapping in correctly preserves the / override.
- accurately documents that an empty allowlist disables the synthetic tool, and the code enforces it (the empty-allowlist check in is placed before the exemption).
One pre-existing note (non-blocking, already flagged by triage): accepts through the guard and would crash on . This is a schema-invalid config edge case; the triage bot flagged it as a non-blocking nit. The fix () is trivial but the author reasonably chose to keep the diff minimal. Not a blocker.
CI: The Linux unit suite (including the new tests) is green at the old HEAD; the fix commit is a follow-up push on the same branch and the PR remains mergeable. The macOS/Windows lane removal (#10059) and integration-test skip (merge-queue-only) are infra design, not failures.
Verdict: All 6 previous findings resolved. No new issues. The fix is correct, well-scoped, and properly tested. — Qwen Code /review (v0.22.0)
doudouOUC
left a comment
There was a problem hiding this comment.
Re-review: fix commit verified (7e7a12b)
The previous CHANGES_REQUESTED round (at 9545e63) raised 6 findings. I verified the fix commit at HEAD 7e7a12b against each one:
Previous findings — all resolved
| # | Finding | Verdict |
|---|---|---|
| R1-1 (C) | MCP tools bypass the empty coreTools allowlist | Fixed — registerTool() guard in tool-registry.ts covers the three MCP paths (legacy McpClient.discover(), pooled SessionMcpView.applyTools, discoverAndRegisterToolsFromCommand). The optional-call + typeof-function guard is consistent with the project's shim-safety pattern. |
| R1-2 (S) | Bare --core-tools silently disables all tools | Fixed — argv.coreTools?.length in config.ts treats an argv-sourced [] as absent, while settings.tools?.core still passes through. Three new config tests pin the three-way semantics. |
| R1-3 (S) | Misleading "permission was declined" attribution | Fixed — coreToolScheduler.ts adds an isCoreToolsAllowListEmpty() branch before the permissions.allow advice, producing a message that names tools.core. Two new tests verify both the covered and uncovered tool paths. |
| R1-4 (S) | Four stale doc statements | Fixed — isToolEnabled JSDoc, CORE_TOOLS docstring, initialize() comment, and structured-output.md all state the empty-list exception. |
| R1-5 (S) | permissions.allow advice can never succeed | Fixed — the empty-allowlist branch in coreToolScheduler.ts is ordered before the allow-advice fallback, and the isCoreToolsAllowListEmpty() check in isToolEnabled() is ordered before the non-core exemption. |
| R1-6 (S) | SDK transports drop an explicitly empty list | Deferred to #10138 — spans three SDK packages + a forward-vs-document decision. Reasonable. |
New HEAD review
The fix commit is clean, correctly scoped, and well-tested (8 new tests across 4 test files). No new issues found.
Consistency checks:
- initialize(), isCoreToolsAllowListEmpty(), and the isToolEnabled() empty-allowlist gate are mutually consistent for all three coreToolsAllowList states (null, empty Set, non-empty Set).
- The --core-tools bare-flag -> absent mapping in config.ts correctly preserves the bareMode/safeMode override.
- structured-output.md accurately documents that an empty allowlist disables the synthetic tool, and the code enforces it (the empty-allowlist check in isToolEnabled() is placed before the isCoreTool() exemption).
One pre-existing note (non-blocking, already flagged by triage): initialize() accepts null through the !== undefined guard and would crash on null.map(...). This is a schema-invalid config edge case; the triage bot flagged it as a non-blocking nit. The fix (Array.isArray(rawCoreTools)) is trivial but the author reasonably chose to keep the diff minimal. Not a blocker.
CI: The Linux unit suite (including the new tests) is green at the old HEAD; the fix commit is a follow-up push on the same branch and the PR remains mergeable. The macOS/Windows lane removal (#10059) and integration-test skip (merge-queue-only) are infra design, not failures.
Verdict: All 6 previous findings resolved. No new issues. The fix is correct, well-scoped, and properly tested. — Qwen Code /review (v0.22.0)
doudouOUC
left a comment
There was a problem hiding this comment.
Re-review: fix commit verified (7e7a12b)
The previous CHANGES_REQUESTED round (at 9545e63) raised 6 findings. I verified the fix commit at HEAD 7e7a12b against each one:
Previous findings — all resolved
| # | Finding | Verdict |
|---|---|---|
| R1-1 (C) | MCP tools bypass the empty coreTools allowlist | Fixed — registerTool() guard in tool-registry.ts covers the three MCP paths (legacy McpClient.discover(), pooled SessionMcpView.applyTools, discoverAndRegisterToolsFromCommand). The optional-call + typeof-function guard is consistent with the project's shim-safety pattern. |
| R1-2 (S) | Bare --core-tools silently disables all tools | Fixed — argv.coreTools?.length in config.ts treats an argv-sourced [] as absent, while settings.tools?.core still passes through. Three new config tests pin the three-way semantics. |
| R1-3 (S) | Misleading "permission was declined" attribution | Fixed — coreToolScheduler.ts adds an isCoreToolsAllowListEmpty() branch before the permissions.allow advice, producing a message that names tools.core. Two new tests verify both the covered and uncovered tool paths. |
| R1-4 (S) | Four stale doc statements | Fixed — isToolEnabled JSDoc, CORE_TOOLS docstring, initialize() comment, and structured-output.md all state the empty-list exception. |
| R1-5 (S) | permissions.allow advice can never succeed | Fixed — the empty-allowlist branch in coreToolScheduler.ts is ordered before the allow-advice fallback, and the isCoreToolsAllowListEmpty() check in isToolEnabled() is ordered before the non-core exemption. |
| R1-6 (S) | SDK transports drop an explicitly empty list | Deferred to #10138 — spans three SDK packages + a forward-vs-document decision. Reasonable. |
New HEAD review
The fix commit is clean, correctly scoped, and well-tested (8 new tests across 4 test files). No new issues found.
Consistency checks:
- initialize(), isCoreToolsAllowListEmpty(), and the isToolEnabled() empty-allowlist gate are mutually consistent for all three coreToolsAllowList states (null, empty Set, non-empty Set).
- The --core-tools bare-flag -> absent mapping in config.ts correctly preserves the bareMode/safeMode override.
- structured-output.md accurately documents that an empty allowlist disables the synthetic tool, and the code enforces it (the empty-allowlist check in isToolEnabled() is placed before the isCoreTool() exemption).
One pre-existing note (non-blocking, already flagged by triage): initialize() accepts null through the !== undefined guard and would crash on null.map(...). This is a schema-invalid config edge case; the triage bot flagged it as a non-blocking nit. The fix (Array.isArray(rawCoreTools)) is trivial but the author reasonably chose to keep the diff minimal. Not a blocker.
CI: The Linux unit suite (including the new tests) is green at the old HEAD; the fix commit is a follow-up push on the same branch and the PR remains mergeable. The macOS/Windows lane removal (#10059) and integration-test skip (merge-queue-only) are infra design, not failures.
Verdict: All 6 previous findings resolved. No new issues. The fix is correct, well-scoped, and properly tested. — Qwen Code /review (v0.22.0)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
仅完成部分审查,审查缺口已披露。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max via Qwen Code /review (v0.22.0)
A settings file with `"tools": { "core": null }` — the common JSON idiom
for clearing the deprecated key — crashed startup with a TypeError
inside `PermissionManager.initialize()` because the rewritten guard
admitted `null` where the pre-PR code was null-tolerant. Only real
arrays activate the allowlist now, so `null`/non-array values keep the
"no restriction" semantic while `tools.core: []` still disables every
tool. Also route the empty-allowlist gate in `isToolEnabled()` through
`isCoreToolsAllowListEmpty()` so the file has one source of truth, and
correct the JSDoc and scheduler rejection message that attributed the
empty state to `--core-tools` — no flag form can produce it now that
`loadCliConfig` treats a bare/valueless flag as absent.
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
|
CAP-4 closeout round — pushed 7e7a12b..762f662 to the fork branch. Fixed in 762f662 (threads resolved):
Deferred to a follow-up round (replied, left unresolved): cli precedence test gap (#2165), startup warning for an empty allowlist (#312), Verification: permission-manager.test.ts 413/413 green; |
Extend the empty coreTools allowlist test to assert structured_output and an mcp__ tool name are disabled under `coreTools: []`, so an exemption mutant in isToolEnabled can no longer pass the suite (QwenLM#10065). Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Add a precedence test asserting a valued --core-tools flag wins over settings tools.core on the resolved coreTools ternary; the current suite left a settings-over-argv mutant undetected (QwenLM#10065). Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
An explicit `tools.core: []` now disables every tool for the session, but until now with zero user-visible signal. Emit one startup notice on stderr next to the existing safe-mode --core-tools warning so a tool-free session is recognizable, and cover the notice in the config tests (QwenLM#10065). Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/core/coreToolScheduler.ts:2487 — [probe] Auto-injected deny rules shadow the empty-allowlist attribution in non-interactive mode
Convergence: round 3 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 7 (7 new). Findings keep coming back to the same files: packages/cli/src/config/config.ts (findings in round 2; 4 more now); packages/core/src/core/coreToolScheduler.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 3 轮发布了 5 条行内评论,其中 5 条是首次提出;上一轮发布了 7 条(其中 7 条首次提出)。发现反复回到同一批文件:packages/cli/src/config/config.ts(第 2 轮已出过发现,本轮又有 4 条);packages/core/src/core/coreToolScheduler.ts(第 2 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the PR has merge conflicts, so refs/pull/10080/merge is unavailable — resolve conflicts and re-run. 中文 — 判定:
|
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the PR has merge conflicts, so refs/pull/10080/merge is unavailable — resolve conflicts and re-run. 中文 — 判定:
|
|
Triage re-run completed without a new review. The bot already has a review of its own on 机器人在 The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
…s.eager (QwenLM#10075) (QwenLM#10098) * fix(core): decouple permissions.allow from tool registration via tools.eager (QwenLM#10075) `permissions.allow` was overloaded in QwenLM#9829 to double as a registry-level allowlist, so configuring it purely for auto-approval silently reshaped the model's toolset — the QwenLM#10075 report (`edit` / `write_file` vanishing in 0.22.1). QwenLM#10082 softened the symptom (uncovered tools became `deferred` rather than unregistered) but kept the conflation: one key still decided both "don't ask me about this" and "don't send this schema". Split the two jobs instead of trading them off: - `permissions.allow` (and `ask`) go back to PURE auto-approval. They never remove, demote, or hide a tool. This is the actual fix for QwenLM#10075. - A dedicated `settings.tools.eager` allowlist now drives the eager/deferred split, preserving QwenLM#9827: when set, only the named tools' schemas ride in the initial model request and everything else is deferred — still registered, still in `/tools`, still loadable via `tool_search`. QwenLM#10082's three-state registration machinery is kept as-is; only its input changes, so no multi-layer deferred wiring is reverted. QwenLM#9829's rule-parser alias map is likewise untouched — the rollback is scoped to the one part of that PR which caused the regression. Also fixed along the way: - Command-discovered tools (`tools.discoveryCommand`) were hard-DROPPED when the allowlist did not cover them, which reproduced QwenLM#10075's silent disappearance for that registration path. They now defer like built-ins. - `isLsToolEnabled()` no longer treats permission rules as a registration signal (reverting the QwenLM#9829 side-effect). - `/tools` marks deferred tools "(on demand)" with a footnote, so an active `tools.eager` list is visible instead of silently reshaping the toolset — the user-facing half of the original report. Migration: users who set `permissions.allow` for schema-shrinking (llama.cpp, Docker Model Runner) move that list to `tools.eager`; the shape and entries are unchanged. `tools.core` no longer migrates to `permissions.allow`. * fix(core): make an explicitly empty tools.eager list active, not ignored `tools.eager: []` collapsed into "unset", so an empty list silently meant "no restriction" — the opposite of the convention QwenLM#10080 establishes for the sibling `tools.core` key, where an explicit `[]` is an active-but-empty allowlist and only `undefined` means unrestricted. Two knobs in the same `tools.*` namespace reading empty-array backwards from one another is a trap, and QwenLM#10138 shows how expensive that divergence gets once transports start dropping the empty case. Activation is now `Array.isArray()`, matching QwenLM#10080. An explicitly empty `tools.eager` defers every non-exempt tool, which is a gentler answer to the constrained-decoding failures behind QwenLM#10065 than `tools.core: []`: the eager request carries almost no tool schemas, but nothing is disabled — every tool stays registered and reachable via `tool_search`. Two supporting changes were needed to make the empty case reachable at all, the same class of bug QwenLM#10138 reports for coreTools: - `Config.getEagerTools()` returns `readonly string[] | undefined` instead of collapsing an absent list to `[]`. - The CLI guards with `Array.isArray(settings.tools?.eager)` before calling `normalizeDisabledToolList`, which maps `undefined` to `[]` and would otherwise erase the distinction on the way into core. Malformed-only lists (`['', 'Bash(unbalanced']`) now leave the allowlist active but matching nothing, so everything defers. Deferring more than intended is recoverable — ToolSearch still reaches every tool — whereas ignoring a configured list resends exactly the schemas the user asked to keep out. * docs: qualify schema-shrink advice and registration claims (QwenLM#10075) Address review findings R1-2, R1-4, R1-7, R1-13: state that coreTools is the only allowlist-style registration gate (whole-tool deny/exclude rules and tools.disabled also deregister), qualify the schema-shrink advice to whole-tool rules with MCP tools exempt, and correct the deprecated tools.core migration guidance (permissions.allow is pure auto-approval and cannot reproduce the fail-closed allowlist restriction). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(core): cover the two behaviours this PR changed but left unpinned R1-17 (adapted) — the `QwenLM#9827` test sweep removed the only guard that `Config.initialize()` tolerates non-string entries in a settings array. The path that guard protected is gone with `isLsToolEnabled`'s revert, so it is re-pointed at the equivalent new surface: settings load performs no element-type validation, so `tools.eager` holding `null` or a number must be filtered rather than crash registry construction. R1-18 — `isLsToolEnabled()`'s reverted behaviour had no test. The eight deleted tests only covered the positive allowlist path that no longer exists; nothing pinned the negative. Added a test asserting that `permissions.allow` / `permissions.ask` coverage no longer opts `list_directory` into registration, so only `tools.listDirectory.enabled` or `coreTools` can open that gate. The docs findings from the same review (R1-1/R1-2/R1-4/R1-5/R1-12) are already fixed by e3e2fce on this branch, which also covers the specifier-rule and MCP deny-exemption nuances; nothing further is needed there. Deliberately not addressed: R1-15/R1-16 concern an integration-test file this revision does not touch. R1-19 (scheduler if/else ordering) and R1-22 (`removePersistentRule` coverage) are pre-existing gaps in files QwenLM#10080 also edits, so adding tests there now maximises the conflict between the two PRs for marginal gain — better done once both have landed. * fix(core): complement review round — missing schema, stale CLI tests, shim delegation witnesses (QwenLM#10075) Complements e3e2fce / 2620d9d (docs qualifications and the R1-17/R1-18 witnesses): - Regenerate settings.schema.json (the tools.eager entry was missing, which failed the "Check settings schema is up-to-date" CI gate). - Rewrite the stale loadCliConfig "registry allowlist wiring" describe: the PR removed Config.getRegistryAllowList(), leaving five assertions calling a method that no longer exists. Re-point them at the tools.eager wiring, including the explicitly-empty-list case. - Restore shim delegation tests for isToolDisabledByCoreToolsAllowList and getToolRegistrationStatus on both scoped PermissionManager shims (R1-21). - Re-add the coreTools x permissions.allow combination witness (R1-23) and the ask-never-affects-registration witness (R1-25). - Freshen stale "permissions.allow registry allowlist" comments now that tools.eager owns the demotion (tool-registry.ts, agent-core.ts, tool-registry.test.ts, config.test.ts) and rename the integration-test describe that still carried the old framing. * fix(test): align the QwenLM#10075 integration suite with the decoupled semantics The suite still encoded the intermediate demote-via-permissions.allow design: it asserted uncovered tools stay OUT of the eager request under a bare permissions.allow configuration. Under the final decoupling permissions.allow never gates the registry, so without tools.eager the uncovered tools ride in the eager request — the no-AK gate failed on 'excluded schemas' expectations this revision no longer satisfies. Rewrite the two witnesses: - bare permissions.allow: uncovered tools stay registered, ADVERTISED in the eager request, and execute through the normal approval flow. - tools.eager set: unlisted tools are deferred (absent from the eager request) yet tool_search still discovers and loads them — the QwenLM#9827 schema-shrink now pinned to its real knob. * fix(core): preserve deferred discovered tools across registries * fix(cli): let tools.eager replace across scopes * fix(core): preserve tools.eager without tool search * fix(core): report the two states where tools.eager quietly changes shape Neither state is wrong to be in — both are just invisible today, and an invisible reshaping of the toolset is what QwenLM#10075 was reported for. - No ToolSearch. Holding demoted tools back (rather than revealing them) keeps the allowlist honest, but with `tools.toolSearch.enabled: false`, a `tool_search` deny rule, or the automatic DeepSeek opt-out there is nothing left to load them on demand: they are unreachable until restart while still listed in `/tools`. Warn once, naming the tools and the ways out. - Unusable entries. A misspelt `tools.eager` entry is dropped per-entry and can narrow the eager set to nothing, deferring the whole toolset. Keeping the list active is right — ignoring it would resend the schemas the user asked to withhold — but nothing said which entries fell out. Log them, with how many survived. * fix(cli): stop /tools promising a tool_search load that cannot happen The "(on demand)" footnote tells the user the model "loads them via tool_search when needed". When `tool_search` is not registered that is false, and it is false in exactly the session where the distinction bites: tools held back by `tools.eager` have no loading path left. Pass the registry's answer through to the view and say so instead. * docs: document the tools.eager/tool_search pairing, retire two stale claims - settings.md, settingsSchema and the VS Code schema now state what happens when ToolSearch is off (schemas still withheld, nothing left to load them back) and that unusable entries are dropped with a warning while the rest of the list stays active. Both were discoverable only by reading the code. - `preloadDeferredToolsWithinBudget` still credited the demotion to the `permissions.allow` registry allowlist; every other comment in that file moved to `tools.eager` when the key changed. - `migrateLegacyPermissions` is dead code (no call site anywhere) whose `tools.core` → `permissions.allow` arm encodes exactly the conflation this PR removes. Say so at the definition, so wiring it up later cannot quietly delete a user's allowlist and replace it with a no-op. * fix(core): resolve registration-gate PM through getPermissionManager so scoped agent shims participate (QwenLM#10075) registerLazy and registerImageGenerationTool read the permissionManager field, which on Object.create-derived configs (skill-review / managed-memory agent shims installed via deriveConfig) resolves through the prototype chain to the base manager — bypassing the getPermissionManager override. With an active tools.eager allowlist omitting the file tools, the base manager demoted them to deferred and prepareTools stripped them from the forked agent's explicit tool list. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs(settings): scope the tools.core fail-closed promise to the core tool set (QwenLM#10075) isToolDisabledByCoreToolsAllowList only disables names in the hardcoded CORE_TOOLS set; built-ins outside it (agent, skill, plan-mode lifecycle, goal tools, task_stop, send_message, list_agents, tool_search, MCP) bypass the allowlist by design. The row claimed 'all tools not in the list are disabled'. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs(sdk): caveat the tool_search loadability promise for sessions without ToolSearch (QwenLM#10075) When ToolSearch is not registered (tools.toolSearch.enabled: false, a tool_search deny rule, or the automatic DeepSeek opt-out), resolveDeferredToolsForReminder withholds every permission-deferred tool for the whole session. Mirror the settings-schema caveat in the coreTools and allowedTools JSDoc blocks. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): surface tools.eager warnings on the console in default runs (QwenLM#10075) The dropped-entries and unreachable-tools warnings were emitted through debugLogger.warn, which writes nothing unless QWEN_DEBUG_LOG_FILE is set — the CLI only auto-sets that in --debug mode, so both warnings were invisible in default runs while the docs promise them. Emit them via console.warn, matching the resolveCronRecurringMaxAgeDays precedent for operator-facing breadcrumbs, and re-pin the existing tests to the visible channel. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): exempt history-revealed tools from the tools.eager unreachable warning (QwenLM#10075) The withheld list feeding the 'unreachable until restart' warning filtered only on isPermissionDeferred. At both call sites revealDeferredToolsReferencedInHistory runs first and re-exposes resume-referenced tools regardless of deferral reason, so a history-revealed tool's schema IS in the declarations while the warning still declared it unreachable. Skip already-revealed tools when building the list and pin the behavior with a regression test. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): warn per tool when tools.eager withholds late-registered tools (QwenLM#10075) The unreachable warning used a boolean latch that froze the first withheld snapshot. A tool registered mid-session (e.g. image_gen via /model --image) joins the permission-deferred set after the latch fires, and every later setTools() re-run withholds it again with zero diagnostic. Track already-warned names in a set and warn on the delta, preserving the anti-spam intent while naming late arrivals. Regression test covers the growing-summary path. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): warn when tools.eager normalization drops entries (QwenLM#10075) normalizeDisabledToolList strips empty/whitespace-only and non-string entries before PermissionManager.initialize() sees the list, so the dropped-entries warning there can never fire for that class on the real CLI path. Worse, tools.eager: [""] collapses to [] — the active defer-everything allowlist — silently demoting every non-exempt eager-by-default tool with zero log signal. Warn at normalization so the collapse always leaves a signal; the fail-closed [] itself is unchanged. CLI-level test pins the warning and goes red if the guard is removed. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): warn when a tools.eager entry matches no registered tool (QwenLM#10075) The dropped-entries warning only fires for syntactically unusable entries; a valid-but-unknown name like tools.eager: ["read_flie"] passes parseRule and resolveToolName unchanged, lands in canonicalNames with no diagnostic, and activates an allowlist matching nothing — silently deferring every non-exempt eager-by-default tool, the exact typo class the warning's rationale names. Warn once per initialize() for entries the alias table cannot resolve, keeping the mcp__/computer_use__ discovery-name pass-through untouched. Tests pin the diagnostic for the typo class and the pass-through silence; removing the check turns them red. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(core): pin tools.eager gate ordering and no-promotion invariants (QwenLM#10075) Arm the surviving coreTools branch in 'cites a matching deny rule when one exists (QwenLM#9827)' so the test pins the deny-over-coreTools if/else-if ordering with both gates armed — swapping the branches turns it red locally. Add the missing ToolNames.LS negative assertions to the eager-wiring registry test and a companion case with lsToolEnabled: true where the eager list omits LS, pinning that tools.eager demotes via registerPermissionDeferredFactory and never promotes a disabled-by-default tool into existence. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(core): give memory-scoped PM stub its registration-time surface (QwenLM#10075) The forkedAgent bound-tool isolation test stubbed the upstream getPermissionManager override as a bare marker object. Now that createToolRegistry resolves the permission manager through the getter and consults getToolRegistrationStatus at registration time, the bare marker throws inside registerLazy's guard and every bare-mode factory is skipped, so ensureTool(Edit) resolved undefined in CI. Give the stub the getToolRegistrationStatus surface that production memory-scoped PMs always implement (MemoryScopedPermissionManager in memory-scoped-agent-config.ts). Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com> * fix(core): close tools.eager review gaps --------- Co-authored-by: yiliang114 <jinjing.zzj@gmail.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Resolve conflicts against the QwenLM#10075/QwenLM#10098 permissions overhaul: keep the empty tools.core gate and isCoreToolsAllowListEmpty(), drop the removed permissions.allow registry-allowlist API, adopt the tools.eager naming, and map getRegistryAllowList to getEagerTools in the new tool-registry tests. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- scoped PermissionManager shims miss the isCoreToolsAllowListEmpty delegation (scheduler attribution at coreToolScheduler.ts:2490 and the registerTool empty gate) — already recorded in the round-4 deferral list (review 5038829057)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/tool-registry.ts:313 — [review] registerTool shim guard has no testpackages/core/src/core/coreToolScheduler.ts:2491 — [review] deny-over-empty-allowlist ordering untestedpackages/core/src/tools/tool-registry.ts:301 — [review] No registry-boundary test for built-ins under []
Convergence: round 5 posted 8 inline comment(s), 4 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: packages/cli/src/config/config.ts (findings in round 4; 1 more now); packages/core/src/permissions/permission-manager.ts (findings in round 4; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 3 条(原文未翻译,列表见上方英文部分)。
收敛情况:第 5 轮发布了 8 条行内评论,其中 4 条是首次提出;上一轮发布了 4 条(其中 4 条首次提出)。发现反复回到同一批文件:packages/cli/src/config/config.ts(第 4 轮已出过发现,本轮又有 1 条);packages/core/src/permissions/permission-manager.ts(第 4 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。(仅为观察——本轮评审未因此扣留任何内容。)
— qwen3.8-max via Qwen Code /review (v0.22.2)
Settings loading performs no element validation (the schema declares only
type: array), so a hand-edited "tools": { "core": [42] } — or a mixed
["read_file", 42] — flowed raw into Config.coreTools and
resolvedCoreTools. The non-interactive exclusion path (isToolEnabled ->
filterList -> entry.trim()) then threw TypeError: entry.trim is not a
function inside loadCliConfig, and isLsToolEnabled() -> parseRule(42)
would crash tool-registry construction — the exact input shape the
coreTools ternary's own comment promises is normalized away (QwenLM#10080).
Normalize once in a settingsCoreTools const consumed by both read sites:
non-array values still resolve to ABSENT (undefined), non-string entries
are dropped, and empty/whitespace strings are kept so they collapse to
the explicit empty allowlist (with both QwenLM#10065 diagnostics) in
PermissionManager.initialize(). Adds a regression test covering [42] and
the mixed shape; it fails with TypeError on the unpatched code.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Under tools.core: [] the empty allowlist disables every tool, so
client.setTools() wraps ZERO declarations as [{ functionDeclarations: [] }].
The pipeline guard only checked the wrapper's length (1 for that shape),
so convertLlmToolsToOpenAI returned [] and buildRequest still shipped
"tools": [] on the wire — which some providers reject, and which defeats
the actual tool-free session the empty-allowlist semantic promises
(QwenLM#10065/QwenLM#10080). Guard on the converted declarations instead: when the
conversion yields no tools the field (and the tool_choice mapping that
only makes sense alongside tools) is omitted entirely.
Adds a pipeline test for the wrapped-empty shape client.setTools()
actually produces; it fails (tools: [] reaches the wire) on the
unpatched code.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…ager
The collapse filter in initialize() explicitly handles non-string array
entries ("non-string garbage" per its own comment), but no test passed a
non-string entry inside the array. Settings loading performs no element
validation, so a hand-edited "core": [42] genuinely reaches this site;
a future simplification dropping the typeof half of the filter would turn
that config into a startup crash (parseRule(42) -> raw.trim()) with no
test going red (QwenLM#10080).
Add cases beside the all-empty-string collapse test: [42] collapses to
the explicit empty allowlist and a mixed [read_file, 42] keeps its named
tool. Removing the typeof guard makes the new test fail with
TypeError: t.trim is not a function.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The comment asserted safe mode resolves coreTools to a built-in safe-tools list, but no such list exists anywhere in the codebase (repo-wide search for SAFE_TOOLS/safeTools/safe-tools matches only the comment itself): loadCliConfig resolves coreTools to undefined via the bareMode || safeMode arm of the ternary. Reword the comment to describe the actual mechanism so future edits of this test do not assert a non-existent safe list (QwenLM#10080). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R5-3 stale tools.core descriptions across eight sites (settings.md:367/490, settingsSchema.ts:2751, settings.schema.json:1300, four code/test comments) — already reported (comment 3883041117), re-flagged this round by three agents; still op…
- R4-2 notice predicate re-implements the De Morgan complement of the PermissionManager collapse filter — already reported (comments 3870090480, 3883041136), re-derived by this round's altitude agent; still open
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent 5": executing the three focused vitest suites at HEAD (config.test.ts, pipeline.test.ts, permission-manager.test.ts) — neither the worktree nor the parent checkout ….
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/core/openaiContentGenerator/pipeline.ts:1095 — [probe] tool_choice omission under the wrapped-empty guard has no test pinpackages/cli/src/config/config.ts:1800 — [probe] whitespace-only tools.core entry fires no startup notice in any testpackages/core/src/permissions/permission-manager.ts:238 — [review] R4-1 entry whose parsed tool name is empty still enters the allowlistpackages/cli/src/config/config.ts:1800 — [review] R4-2 predicate re-implements the De Morgan complement of the collapse filterdocs/users/features/structured-output.md:267 — [review] R5-3 eight other tools.core descriptions still assert the old opposite semanticpackages/core/src/core/coreToolScheduler.ts:2482 — [review] R5-4 ordering comment references two branches that no longer exist
中文说明
仅完成部分审查,审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度(达到工具调用预算):"agent 5":executing the three focused vitest suites at HEAD (config.test.ts, pipeline.test.ts, permission-manager.test.ts) — neither the worktree nor the parent checkout …。
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 6 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
A non-empty raw string can still carry no tool name: `"()"` and mangled
specifiers like `"(ls -l)"` parse to `toolName: ''`, and unbalanced-paren
entries parse to `invalid` rules. The normalization filter only checked the
raw string, so such entries entered the allowlist as `{''}` — size 1,
matching nothing — with `isCoreToolsAllowListEmpty()` false: every core
tool silently disabled while non-core tools kept their historical bypass,
and the CLI startup notice never fired (QwenLM#10080).
Export `isNamelessCoreToolsEntry` as the single shared classification
(non-string, blank, `rule.invalid`, or empty parsed tool name) and use it
in the gate, mirroring the eager-allowlist path which already drops
`rule.invalid` entries.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…otice The startup notice re-implemented the De Morgan complement of the collapse filter in PermissionManager.initialize() inline, in a different package, coupled only by a comment. Fixing the gate alone (dropping entries without a usable parsed name) would flip `tools.core: ["()"]` to the empty allowlist while this un-mirrored predicate still suppressed the notice — reproducing the exact silent tool-free session QwenLM#10065 set out to prevent (QwenLM#10080). Use the core-exported isNamelessCoreToolsEntry in the notice predicate; config.ts already imports from @qwen-code/qwen-code-core, so no new dependency edge is needed. The argv-wins guard stays unchanged: an empty allowlist from a valued --core-tools flag keeps its existing, test-pinned treatment. Also aligns the neighbouring tools.eager comment, which still claimed a tools.core empty list is treated as unset. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Gate side: `["()"]`, `["(ls -l)"]`, `["Bash(ls"]` and mixed all-nameless
lists must collapse to the explicit empty allowlist —
isCoreToolsAllowListEmpty() true, every tool (core, MCP, synthetic)
disabled — instead of a size-1 {''} allowlist matching nothing. Removing
the isNamelessCoreToolsEntry filter from initialize() turns these red.
Notice side: the startup warning fires for `["()"]` / `["(ls -l)"]`
exactly like for [] and does not fire for a list still naming a tool.
Also pins the helper directly so neither call site can rewind to a
private inline predicate while the other stays.
Touches the two tools.eager test comments that still claimed a
tools.core empty list is treated as unset.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…semantic The semantic flip documented in structured-output.md — an explicitly empty tools.core list (or one whose entries carry no tool name) is an ACTIVE allowlist that disables every registered tool — left every other description of the key asserting the opposite, pre-QwenLM#10065 behavior. An operator clearing this deprecated key followed the settings reference, the migration table, or the /settings schema and got a session where every tool is disabled while the docs promised [] "disables nothing". Rewrite the remaining sites to the new contract (only an omitted / null / non-array value means no restriction) and regenerate the vscode-ide-companion schema from settingsSchema.ts. The tools.eager comments in core config.ts kept their active-empty contrast but named the wrong effect for core. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The empty-allowlist branch comment justified its ordering by referencing two branches that no longer exist: the QwenLM#10075 merge deleted the permissions.allow remediation branch, leaving only the coreTools-miss branch and the generic "permission was declined" fallback. Rewrite the rationale to name the branches that actually follow, so a maintainer does not conclude the allowlist-advice branch was accidentally deleted and re-add a remediation an allow rule can never satisfy. Also drop the isPermissionsAllowListActive / isCoveredByAllowOrAskRule mock properties from the two new tests — they exist nowhere in production code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 457 passed · 0 failed · 457 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:457 通过 · 0 失败 · 457 总计 抖动门:✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #10080 Deep Verification —
|
| cell | environment | observable oracle | result |
|---|---|---|---|
base (c13aa351a0, worktree) |
tsx source import; shared node_modules (lockfile untouched by PR) + packages/core/node_modules symlink (deps-only, no @qwen-code links) |
hazard count over 13 real tools + 5 fixtures | 18 hazards — 10 from production/default shapes (expected-broken control: PASS) |
head (96eeb285d2 merge) |
same | same | 5 hazards, all inside the deliberately conservative fixtures; 0 from production schemas |
The flip: report_findings wire schema loses maxLength 2000/4000/4096 (keeps 64/1000); send_message 65536, loop_wakeup/task_create/task_update 10000, read_mcp_resource 4096 all removed; cron_list's closed-empty object and the MCP default {type:'object',properties:{}} (the exact fallback mcp-client.ts gives tools with no inputSchema) normalize to {type:'object'}; the 2020-12 fixture's maxLength:1999 survives. Witness: 01-ab-hazard-cells-base-vs-head.png.
Secondary claims, verified:
- Gate correctness (
canEnforce) — draft-07 schema using 2020-12prefixItems→false(wire keeps constraints); identical schema with 2020-12$schema→true; unresolvable external$ref→false; non-object inputs →false. Converter cells confirm the wire consequence in each direction (mcp_2019_unenforceable,mcp_draft07_prefixItems,without_local_schema,legacy_gemini_onlyall keep hazards on head). Witness:02-boundary-ladder-and-siblings.png. - Source-schema immutability + client-side enforcement — deep snapshot equality before/after conversion on both arms;
ReportFindingsTool.validateToolParamsstill rejects a 2500-charsummary(sourcemaxLength: 2000) on both arms while the head wire copy no longer carries the limit. - No behavior change with the flag off — default-arg
relaxSchemaForFunctionCallingkeepsadditionalProperties:falseandmaxLength:65536on both arms;compileStrictaccept/reject identical on both arms (tuple schema accepted, unknown keyword rejected).
Mutation matrix (all rows as expected, witness 03-mutation-matrix.png):
| row | mutation | suite | expected | got |
|---|---|---|---|---|
| C×3 | unmutated controls | 3 files | green | green (33, 133, 1+223 skipped) |
| M1 | gate := true |
converter.test.ts | red | red — only relaxes grammar constraints backed by local validation |
| M2 | gate := false |
converter.test.ts | red | red — same test |
| M3 | >= 2000 → > 2000 |
schemaConverter.test.ts | red | red — removes grammar-hostile empty objects and repetition limits |
| M4 | remove empty-properties drop branch |
schemaConverter.test.ts | red | red — same test |
| M5 | canEnforce := true |
schemaValidator.test.ts | red | red — requires the runtime dialect to recognize the full vocabulary |
| M6 | neutral comment-only change | schemaConverter.test.ts | green | green |
No survivors; each red is the intended assertion (behavioral toEqual/toBe mismatch, not an import break), and M6 proves the runner does not red-shift. Gate + filter hunks are individually observable (M2 vs M4), so no layered-guard masking.
Targeted gates (head): schemaConverter.test.ts 33 passed, schemaValidator.test.ts 133 passed, converter.test.ts 224 passed (390 total); tsc --noEmit exit 0; eslint on the 6 changed files exit 0 (lint liveness proven: planted any/unused-var probe was reported, then removed). Reviewer Test Plan steps 1–5 each map to executed evidence above (step 1 = gate run; steps 2–5 = harness 01/02 cells and the PR's own pinned tests).
Findings
None blocking. Ordered by severity:
- Suggestion — deleted test left one shape unpinned. The removed test "keeps additionalProperties:false when there are no properties to promote" would still have passed on head (measured: flag-off
{type:'object',additionalProperties:false}→ unchanged), so its removal was not forced by the behavior change. After the removal, no head test pins the no-properties-key shape under flag-off — the converter'swithout_local_schemacell pins the siblingproperties:{}shape, and the newremoves grammar-hostile…test covers flag-on only. A future edit making the flag default-true (or dropping therelaxGrammarConstraints &&conjunct) would silently move that shape with no test going red. Non-blocking: suggest restoring the one-line test beside the new ones. Repro:relaxSchemaForFunctionCalling({type:'object',additionalProperties:false})keepsadditionalProperties:falseon head today. - Low / pre-existing — residual sibling hazard classes stay on the wire, gate correctly closed. String-typed limits (
maxLength: "3000") and negative limits (maxLength: -5) violate Ajv's meta-schema, socanEnforcereturns false (measured) and the constraints stay on the wire — consistent with the PR's documented principle (no local enforcement ⇒ no wire relaxation) and identical to base exposure, i.e. not a regression. But such a malformed MCP schema can still trigger a whole-request grammar failure on llama.cpp-based runtimes; the PR's stated scope (the two documented upstream failure classes) does not cover them. Naming it so the description's "no matching wire hazards" is read as "no hazards of the two addressed classes". - Note — shared strict Ajv instances,
$idcollision fails closed.canEnforcecompiles into module-level shared instances; a second schema carrying the same$idthrows on compile →canEnforcefalse → no relaxation for that tool (wire identical to base). Measured: first calltrue, secondfalse. Conservative direction, no regression; only relevant to providers that would have benefited from relaxation. - Note —
strictTuples: falsein the extractedstrictCompileOptionsis cosmetic-only. Base logsstrict mode: "items" is 1-tuple…warnings fromcompileStrict; head suppresses them. Accept/reject outcomes measured identical on both arms (tuple schema accepted; unknown keyword rejected).
Not covered
- Live llama.cpp / LM Studio smoke test — no such runtime exists in this container. The harness reproduces the wire shape the upstream issues document as unparseable, not an actual grammar-compilation failure; the oracle is the hazard-shape predicate from llama.cpp #25923 / #27087, not llama.cpp itself.
- Per-commit attribution — depth-2 shallow checkout: 1 of the snapshot's 25 commits reachable (
git rev-list HEAD^1..HEAD^2= head OID only). Verified the aggregateHEAD^1..HEADdiff instead. The other 24 commits'#10065content is already in the base tip and absent from the effective diff. - Tools not constructed for the scan — 13 real tools were constructed (every source file carrying limits ≥ 2000, plus closed-shape candidates); a static sweep for inline paramless closed schemas in the remaining tool files found none.
- Windows / live-endpoint rows of the PR's test table — CI-gated, out of reach here.
- Mutation matrix was executed twice (logged run + live capture re-run); both exited 0 with 9/9 rows as expected.
Methodology
Environment: node:22-bookworm-class CI container, Node v22.23.2, npm ci + npm run build pre-done at the merge commit. A/B drove the real TS source of each tree via the repo's tsx (no rebuild needed — the change is pure source; lockfile untouched, so shared node_modules is a clean control; the base worktree additionally needed the deps-only packages/core/node_modules and dist symlinks for resolution and the vitest guard — asserted deps-only, no @qwen-code links inside). Harnesses 01/02 (harness/*.mjs) import the tree under test parameterized by --tree, encode per-arm expectations so predicted base failures count as passes, and wrote raw logs to logs/. Mutation matrix ran in a scratch worktree of the merge commit with exact-string patches, git checkout -- restoration after each row, and a final clean-tree check. Gates: workspace vitest runs cited by exact counts; lint liveness proven by a planted-then-removed violation. Captures produced with scripts/verify-capture.mjs.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/core/src/core/openaiContentGenerator/converter.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/openaiContentGenerator/converter.test.ts
file packages/core/src/utils/schemaConverter.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/schemaConverter.test.ts
file packages/core/src/utils/schemaValidator.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/schemaValidator.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/core/openaiContentGenerator/converter.test.ts: PPPPP
packages/core/src/utils/schemaConverter.test.ts: PPPPP
packages/core/src/utils/schemaValidator.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/core/openaiContentGenerator/converter.test.ts: P (exit 0)
round 1 · packages/core/src/utils/schemaConverter.test.ts: P (exit 0)
round 1 · packages/core/src/utils/schemaValidator.test.ts: P (exit 0)
round 2 · packages/core/src/core/openaiContentGenerator/converter.test.ts: P (exit 0)
round 2 · packages/core/src/utils/schemaConverter.test.ts: P (exit 0)
round 2 · packages/core/src/utils/schemaValidator.test.ts: P (exit 0)
round 3 · packages/core/src/core/openaiContentGenerator/converter.test.ts: P (exit 0)
round 3 · packages/core/src/utils/schemaConverter.test.ts: P (exit 0)
round 3 · packages/core/src/utils/schemaValidator.test.ts: P (exit 0)
round 4 · packages/core/src/core/openaiContentGenerator/converter.test.ts: P (exit 0)
round 4 · packages/core/src/utils/schemaConverter.test.ts: P (exit 0)
round 4 · packages/core/src/utils/schemaValidator.test.ts: P (exit 0)
round 5 · packages/core/src/core/openaiContentGenerator/converter.test.ts: P (exit 0)
round 5 · packages/core/src/utils/schemaConverter.test.ts: P (exit 0)
round 5 · packages/core/src/utils/schemaValidator.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
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
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Not explored to full depth (tool budget reached): "agent 1a": none — all planned checks were completed; the only unperformed check (running the three test files) was blocked by the absence of node_modules in this environ….
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/schemaConverter.test.ts:499 — [probe] Deleted test still passed; flag-off additionalProperties:false retention is now unpinnedpackages/core/src/utils/schemaConverter.ts:194 — [probe] JSDoc states the no-declared-properties additionalProperties removal unconditionally, but it is flag-gatedpackages/core/src/utils/schemaConverter.ts:246 — [probe] hasNoDeclaredProperties misses properties declared via allOf/anyOf/oneOf/patternPropertiespackages/core/src/core/openaiContentGenerator/converter.ts:374 — [review] Withholding of grammar relaxation is silent — no diagnostic names the offending toolpackages/core/src/utils/schemaValidator.ts:64 — [probe] Strict compilation is implemented twice; the two constructions can drift silentlypackages/core/src/core/openaiContentGenerator/converter.ts:374 — [probe] Gate retains grammar-hostile shapes for schemas the runtime cannot compile at allpackages/core/src/utils/schemaConverter.ts:261 — [probe] Length-limit branch guard never tested flag-off; the mutant survives the whole suitepackages/core/src/utils/schemaValidator.ts:128 — [probe] Shared Ajv instances retain ~8.6KB per distinct schema object for process lifetime
中文说明
无阻断问题。LGTM!✅
未探索到全部深度(达到工具调用预算):"agent 1a":none — all planned checks were completed; the only unperformed check (running the three test files) was blocked by the absence of node_modules in this environ…。
收敛姿态下延后(第 7 轮,非阻断)——已记录,本轮不要求修改:共 8 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Local real-stack verification of this PRI rebuilt the failure from #10065 on a real grammar-compiling runtime and ran the PR head against it. Summary: the fix works — the exact 400 disappears — but Bench
1. The reported failure reproduces on the merge base, and this PR removes itBoth runs send the same 23 tools; only the wire copy of the schemas differs. 2. Root cause pinned to one value in one built-in toolSweeping each keyword alone against the live server, then bisecting the captured 23-tool payload: The single trigger in today's tool set is Two notes on the PR description:
3. The
|
| 运行时 | 真实 llama.cpp llama-server b10621(c1d0e7a00),--jinja -c 32768,Qwen2.5-0.5B-Instruct-Q4_K_M.gguf |
| 客户端 | 打包后的 CLI(npm run build && npm run bundle → dist/cli.js),隔离 HOME,OPENAI_BASE_URL 指向一个逐字节记录的 tee 代理 |
| 代码树 | base = merge-base e5cb60ad4,head = 60d4157a2 |
| 提示词 | こんにちは!——报告者本人的输入 |
1. 故障在 merge base 上可复现,本 PR 消除了它
两次运行发送的是同样的 23 个工具,只有 schema 的出站副本不同。(见上方第 1 张图)
2. 根因精确定位到某个内置工具里的一个数值
对每个关键字单独扫描真实服务端,再对抓到的 23 工具载荷做二分:(见上方第 2 张图)
当前工具集里唯一的触发点是 report_findings → findings[].summary → maxLength: 2000。只删掉这一个数字,整个请求就变绿;把它改成 1999 或 2001 同样变绿。这正是 llama.cpp #27087 里的差一错误。
关于 PR 描述的两点:
- 在 b10621 上,
maxLength取 4096 / 10000 / 65536 并不会失败——它们会被 clamp 成无上界。描述暗示它们属于同一失败类。在 #25923 描述的旧引擎上(clamp 合入之前)确实会失败,而 LM Studio 0.4.21 装的正是旧引擎,所以照样删掉是对的——但正文里的论证只对旧引擎成立,对当前 master 不成立。 - 修复的空对象那一半(
properties: {}/additionalProperties: false)在 b10621 上无法复现——我试过的所有形态都返回 200。它只针对旧引擎。无害,但这里没有实测证据支撑。
3. >= 2000 这个阈值漏掉了一个值
maxItems: 1999 在真实服务端上失败,而本 PR 会把它保留在 wire 上(见上图 maxItems 一行,失败集合恰好是 {1999, 2000, 2001})。目前没有工具用到这个值,所以今天不会坏——但 MCP server 可以送来一个。我实测到的所有失败值,用 >= 1999 都能覆盖。
4. 缺陷——canEnforce() 的模块级 Ajv 单例在 $id 上撞车
ajvStrictDefault / ajvStrict2020 是模块级的,而且 canEnforce() 还会编译进共享的运行时 ajvDefault / ajv2020。Ajv 会按 $id 注册 schema;第二个带相同 $id 的不同对象会抛 schema with key or id "…" already exists,而那个空 catch 把它变成了 false。(见上方第 3 张图)
两个后果,都通过本 PR 自己的代码路径复现:
- 修复被静默关掉。 两个 schema 共享同一个
$id的工具:第一个被放宽,第二个保留maxLength: 2000,请求以本 PR 要修的那个错误 400。 - 它比以前更早地波及兜底。
validate()会吞掉编译失败并返回null(视为合法),所以撞车的$id会完全跳过参数校验——50 字符的值能通过maxLength: 10的 schema。这里要为本 PR 说句公道话:共享注册表这个隐患早于本 PR——在main上,同一个$id的第二次validate()就已经会跳过了。变化的是波及范围:canEnforce()在构造请求时就为每个工具占用$id,无论该工具是否会被调用,于是重建后的 schema 对象第一次validate()就可能跳过。这削弱了"原始 schema 仍然在客户端工具参数校验中执行全部约束"这一承重论断。
内置工具不受影响:parametersJsonSchema 是稳定引用(tools.ts:247),Ajv 的 WeakMap 缓存会命中。撞车需要顶层 $id 加上第二个 schema 对象——同一模板生成的两个工具,或者 MCP 重连/刷新后重建的注册表。嵌套 $id 不会撞。
建议的修法(已在仓库的 ajv@8.20.0 上本地验证):new Ajv({ …, addUsedSchema: false }) 能让 compile() 对带 $id 的 schema 幂等——建议同时用在两个新的 strict 实例和共享的 ajvOptions 上。补一个"两个声明共享一个 $id"的回归用例即可钉住。
同一根因的次要影响:schema 对象每次新建时,转换开销从 0.31 ms 变成 15.9 ms(每次 23 工具请求,200 次迭代),因为什么都缓存不上。用内置工具的稳定对象则是 0.06 ms 对 0.07 ms——没有可测代价。
5. 站得住的部分
(见上方第 4 张图)
- 声明的安全性质是真的。 源 schema 仍然会拒绝 wire 副本不再声明的东西:2001 字符的
report_findings.summary、cron_list的多余参数、65537 字符的send_message.message。 - 新增测试是有辨别力的。 对新增生产代码做了 10 个变异体——包括
>= 2000边界的两个方向、canBeObject的各个子句、以及canEnforce那道门——10/10 全被杀死。 - 没有回归。
packages/core:617 个文件 / 22182 个用例通过。npm run typecheck干净。packages/cli:systemController.test.ts(get_usage_info)有 2 个失败,是既有问题——在 merge base 上以完全相同的方式失败,与本改动无关。 - 加进现已共享的
strictCompileOptions的strictTuples: false不改变compileStrict()的结果(Ajv 对它的默认值是log而非throw)——base 与 head 返回的消息完全一致。 - 被删掉的用例
keeps additionalProperties:false when there are no properties to promote不构成覆盖损失:默认路径现在由converter.test.ts里的unsupported/without_local_schema用例钉住(变异体 M4 可证)。
结论
改动在实质上是正确的,端到端证据也很干净:同样 23 个工具,对真实 llama.cpp 从 400 变成 200。我希望 §4 在合入前修掉——它只是一个选项加一个用例,不修的话这个修复对 MCP 工具是顺序相关的,还可能静默关掉本地参数校验。§3(>= 1999)是一个字符的改动,建议一并带上。§2 只是描述措辞问题。
本次未覆盖:LM Studio 0.4.21 本体(本地没有可用的二进制)、Windows,以及空对象放宽(在当前 llama.cpp 上没有活体复现)。







What this PR does
This PR keeps the complete tool set enabled for OpenAI-compatible providers and normalizes only the outbound copy of each tool schema before it is sent.
minLength,maxLength,minItems, andmaxItemsvalues at or above 2000 are omitted from the wire copy so they cannot generate rejected repetition rules.Why it's needed
Before this change, LM Studio and other llama.cpp-based OpenAI-compatible runtimes could reject the whole request with
Failed to initialize samplers: failed to parse grammar. These runtimes compile all tool schemas into one grammar, so one valid but unsupported empty-object or repetition shape breaks every tool in the request.Disabling every tool avoids grammar construction, but it also removes the functionality users need. After this change, tools stay available and only the provider-facing schema copy is relaxed. This addresses the two concrete upstream failure classes documented in llama.cpp #25923 and the exact 2000 boundary documented in llama.cpp #27087.
Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/utils/schemaConverter.test.tsin a built checkout.typeisobject, omitted, or includesobjectin a type array.min/maxconstraints at 2000 or above are removed.parametersJsonSchema, retain the empty-object and repetition constraints on the wire.Evidence (Before & After)
N/A for UI. Before: the production tool set contains closed empty-object schemas and limits such as 4096, 10000, and 65536 that match the upstream grammar failures. After: a recursive scan of the converted production schemas reports no matching wire hazards, while the source schemas remain unchanged.
Tested on
Environment (optional)
macOS, Node.js v22.22.0. A live LM Studio 0.4.21 endpoint was not available locally.
Risk & Scope
tools.coresemantics are unchanged frommain.Linked Issues
Fixes #10065
中文说明
本 PR 做了什么
这个 PR 会为 OpenAI-compatible provider 保留完整工具集,只在发送前规范化每个工具 schema 的出站副本。
minLength、maxLength、minItems和maxItems,避免生成被 grammar 引擎拒绝的重复规则。为什么需要
修改前,LM Studio 以及其他基于 llama.cpp 的 OpenAI-compatible runtime 可能直接拒绝整个请求,并返回
Failed to initialize samplers: failed to parse grammar。这些 runtime 会把请求中的全部工具 schema 编译成一个 grammar,因此只要其中一个合法但不受支持的空对象或重复次数形态存在,所有工具都会一起失效。完全关闭工具虽然能绕过 grammar 构建,但也会移除用户需要的功能。修改后,工具仍然可用,只放宽 provider 看到的 schema 副本。这个方案覆盖了 llama.cpp #25923 记录的两个具体失败类型,以及 llama.cpp #27087 记录的精确 2000 边界问题。
Reviewer Test Plan
如何验证
cd packages/core && npx vitest run src/utils/schemaConverter.test.ts。type为object、省略type、或 type 数组包含object时都会被正确规范化。min/max约束都会被移除。parametersJsonSchema的声明,会在 wire schema 中保留空对象和重复次数约束。证据(修改前后)
非 UI 变更,不适用截图。修改前:生产工具集中包含 closed empty-object schema,以及 4096、10000、65536 等与上游 grammar 故障一致的限制。修改后:递归扫描转换后的生产 schema,不再发现同类 wire hazard,同时 source schema 保持不变。
测试平台
环境(可选)
macOS,Node.js v22.22.0。本地没有可用的 LM Studio 0.4.21 endpoint。
风险与范围
tools.core语义与main保持一致。关联 Issue
Fixes #10065