test(core): pin tools.eager Object.prototype-key tolerance and correct related docs - #10479
Merged
Conversation
…t related docs Item 1 of #10400 (a tools.eager entry named after an Object.prototype key crashing PermissionManager.initialize) was already fixed before #10098 merged: resolveToolName only reads own properties of the alias table and the crashing unknown-entry loop was removed. Pin that contract with regression tests so a reintroduction fails CI: - initialize() tolerates constructor/toString/valueOf/... eager entries - resolveToolName returns Object.prototype-keyed names unchanged - config.test.ts: add the third LS x tools.eager cell (enabled and covered by the list -> registered eagerly) Also correct the doc inaccuracies tracked in the issue: - settings.md: permissions.deny blocks MCP tool calls at runtime but does not remove them from the registry (tools.disabled / the per-server excludeTools filter does) - ToolSearch-absent caveat (5 copies): note the resumed-session history re-reveal and tools.visible carve-outs - sdk-typescript docs + README: note the tools.eager exempt families Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
requested review from
LaZzyMan,
doudouOUC,
qqqys,
tanzhenxin and
wenshao
as code owners
August 29, 2026 07:54
Collaborator
Collaborator
|
Thanks for the PR! Re-running the gate at the current head — Template looks good ✓
Moving on to code review. 🔍 中文说明感谢贡献！在当前 head 上重新执行准入检查——自上次检查后新增了评审响应提交 模板完整 ✓
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewRe-run at the current head: this pass covers the delta since the first review (
Not verified: I don't run PR code here, so the tests' green status rests on the PR's own CI below — the right evidence for a tests-only addition. The maintainer-triggered sandboxed Test evidence — the PR's own CICI on this commit has settled — everything green, no pending runs. The two checks that were red at the first commit (
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查在当前 head 上重新审查：本轮覆盖上次审查后的增量（
未验证：本审查不运行 PR 代码，新测试是否通过以下方 PR 自身 CI 为准——对纯测试改动这是正确的证据。维护者触发的沙箱 测试证据——PR 自身 CI该提交的 CI 已结束——全部绿色，无待处理运行。首次提交时变红的两项检查（ — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 5/5 — every claim re-verified against the code at the current head; the review-response commit pins exactly what was asked, and CI is fully green. Stepping back: the first pass already matched this PR one-for-one against my own proposal for closing out #10400's remaining items, and the delta since then only strengthens that read. The one substantive review suggestion (no test pinned the The re-run's skeptical questions were about drift, and they resolve cleanly: main moved under the branch (three additive PRs, none touching tool registration or deferred hiding), the doc copies stayed byte-identical through the merges, the markdown word-diff against the true merge base shows only the documented corrections, and the two checks that were red at the first commit are green on this head. The maintainer's independent A/B verification in this thread reaches the same conclusion. If I'm maintaining this in six months, the pins read like their neighbors and each comment names the mutant it catches — that's what I'd want. No blockers, no nits worth naming. CI has settled on this commit, so approval goes in now, pinned to the reviewed commit — no deferred marker needed. 中文说明置信度：5/5 —— 每一项结论都已在当前 head 的代码上重新核实；评审响应提交恰好钉住了被要求的契约，且 CI 全绿。 退一步看：首轮审查已确认本 PR 与我自己收尾 #10400 剩余条目的方案逐条一致，此后的增量只加强了这一判断。唯一实质性的评审建议（文档新承诺的 本轮复查的怀疑点集中在"漂移"上，且全部干净落地：main 在分支之下有更新（三个增量 PR，均未触及工具注册或 deferred 隐藏），文档副本在合入过程中保持逐字节一致，相对真实合并基的 markdown 逐词 diff 只显示文档所述修正，首次提交时变红的两项检查在该 head 上已转绿。维护者在本线程的独立 A/B 验证也得出相同结论。六个月后维护这些代码时，钉住测试读起来与相邻用例一致，每处注释都点明它能捕获的变异——正是我希望看到的样子。 无阻塞问题，也没有值得一提的小毛病。CI 已在该提交上结束，因此现在提交绑定到被审查提交的批准——无需延迟批准标记。 — Qwen Code · qwen3.8-max Reviewed at |
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. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6a": none.**; "agent 6c": executing the two new test files ( config.test.ts LS cell, permission-manager.test.ts prototype-key tests) — the review worktree has no node_modules / dist ….
Test Plan (not a blocker): src/permissions/permission-manager.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 6a"：none.**；"agent 6c"：executing the two new test files ( config.test.ts LS cell, permission-manager.test.ts prototype-key tests) — the review worktree has no node_modules / dist …。
Test Plan（非阻断）：src/permissions/permission-manager.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Test Plan (not a blocker): src/permissions/permission-manager.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory.
中文说明
Test Plan（非阻断）：src/permissions/permission-manager.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.3)
CanReader
reviewed
Aug 29, 2026
CanReader
left a comment
CanReader
left a comment
There was a problem hiding this comment.
Documentation-and-schema sync PRs are easy to under-review, so the thing worth checking is whether the described behaviour is actually the behaviour, and whether every copy of the text moved together.
The carve-outs being added — demoted tools referenced in resumed session history get their schemas re-sent without a warning, and demoted tools listed in tools.visible are declared up front — are both meaningful exceptions to the sentence they qualify. The prior text told an operator that when ToolSearch is not registered, demoted tools are simply "out of reach for that session and a warning is logged". Someone reading only that would reasonably conclude a resumed session is as crippled as a fresh one, which is the more alarming reading and the wrong one. Correcting it is worth doing.
What I checked is that the copies stayed in step. The same paragraph is duplicated across packages/cli/src/config/settingsSchema.ts, packages/vscode-ide-companion/schemas/settings.schema.json, packages/sdk-typescript/src/types/types.ts, and the two docs pages, and the diff updates all of them with matching wording rather than drifting between the schema description and the prose. That duplication is the real hazard in this area — a settings description that disagrees with the JSON schema is worse than one that is merely stale, since tooling surfaces the schema copy.
One suggestion, aimed at the duplication rather than this diff: five hand-maintained copies of a 900-character description will drift eventually, and nothing currently fails when they do. If there is a plausible way to generate the vscode settings.schema.json description and the SDK type doc comment from settingsSchema.ts, or failing that a test that asserts the strings are byte-identical across the copies, it would turn the next drift into a red test instead of a docs bug someone notices months later. This PR is itself evidence for that — it exists because the copies were collectively behind the behaviour.
Since the diff is text-only I did not exercise the underlying carve-outs; the review above is about consistency and accuracy of the description, not a verification that the two carve-outs behave as written. If they are not already pinned by tests in the tools.eager suite, that would be the more valuable follow-up.
yiliang114
enabled auto-merge
August 30, 2026 07:18
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Followed up on the latest review. The The previous required checks were cancelled by their 60-minute and 20-minute job timeouts rather than test assertion failures. I have merged the latest |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 3a": none** — all planned checks completed within budget.; "agent 3c": executing the new unit tests in packages/core (the vitest prerequisite guard requires npm run build — dist/ is absent in this shared worktree, and a full buil…; "agent 6c": running the three new test files to confirm green — the review worktree has no node_modules or built dist/, and npm ci + npm run build for the monorepo exce….
Test Plan (not a blocker): src/permissions/permission-manager.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
docs/users/configuration/settings.md:491 — [probe] D3-1 settingsSchema.ts tools.core description still overstates permissions.deny, now disagreeing with the settings.md this PR correcteddocs/users/configuration/settings.md:490 — [probe] D3-2 the newly documented contract's first half (denied MCP tools stay registered/listed) is pinned by no test
中文说明
未探索到全部深度（达到工具调用预算）："agent 3a"：none** — all planned checks completed within budget.；"agent 3c"：executing the new unit tests in packages/core (the vitest prerequisite guard requires npm run build — dist/ is absent in this shared worktree, and a full buil…；"agent 6c"：running the three new test files to confirm green — the review worktree has no node_modules or built dist/, and npm ci + npm run build for the monorepo exce…。
Test Plan（非阻断）：src/permissions/permission-manager.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
Independent verification — local build + real-runtime A/BVerdict: every claim in this PR checks out. Tests + docs only, no runtime change; the three new regression pins each catch a real, otherwise-uncaught mutant; the doc corrections match the code. Recommend merge. ✅ Verified at PR head 1. Git archaeology — "item 1 was already fixed before #10098 merged" is exact
2. Test suites (worktree at PR head)The three named 3. Regression-pin efficacy (mutation A/B testing)Each defect class the PR claims to pin was reintroduced into the worktree source, and the suite re-run:
All mutations reverted afterwards; worktree back to clean PR head. 4. Real-runtime A/B (built CLI, isolated
|
| 重新引入的缺陷 | 位置 | 结果 |
|---|---|---|
A — 完整还原 4a51304 旧状态(去掉 Object.hasOwn + 加回 unknown-entry 循环) |
rule-parser.ts + permission-manager.ts |
416 个中 3 个失败:两个新 #10400 测试 + 既有 parser 钉。tolerates … 以一字不差的原始崩溃失败:TypeError: rule.toolName.startsWith is not a function |
| B — 注册分发在 eager 列表激活时无条件降级 LS、无视条目覆盖 | config.ts |
586 个中只有新增第三格测试失败,既有套件全部通过 — 复现了 #10400 item 6 "该 mutant 此前能逃过全部套件"的论断 |
C′ — 仅对 permission-deferred 类工具去掉 tools.visible carve-out |
tool-registry.ts |
60 个中只有新增 visibleTools 测试失败(整体去掉 carve-out 会被 4 个既有测试抓住,但 permission-deferred 类此前无守卫) |
所有突变已回滚,worktree 恢复干净 PR head。
4. 真实运行时 A/B(构建产物 + 隔离 QWEN_HOME)
从 PR head 构建 core+cli;settings.json 的 tools.eager 含全部 8 个 Object.prototype 键 + ReadFile/Shell(见上方截图):
- BEFORE(重新引入缺陷 A 并重建 core):
qwen -p "say hi"在PermissionManager.initialize()内以 issue 中一模一样的 TypeError 崩溃 — 证实这是普通 settings 文件即可触发的真实 CLI 启动崩溃类缺陷。 - AFTER(PR head):同一命令顺利通过
initialize()(仅在故意不可达的 dummy API 端点失败),交互式 TUI 正常启动到提示符。 /tools端到端印证了新 PM 测试的断言 — allowlist 激活、原型键条目无害、Edit/WriteFile/WebFetch等被 deferred、豁免工具不受影响(见第二张截图)。
两点附带观察(均非本 PR 的问题):
- eager 条目走 permission 规则解析器,因此家族语义生效:
ReadFile同时保持 Glob/Grep/ZoomImage eager,Shell保持 Monitor eager — 与文档"与 permission 规则同一解析器"的设计一致。 - 重新引入缺陷 A 时,交互式 TUI 表现为卡死在
Initializing...(崩溃信息只在非交互路径打印)— 同一启动破坏的更隐蔽形态,反而更凸显 PM 层回归钉的价值。
5. 文档修正与代码对照
permissions.deny≠ 移除 MCP 工具:ToolRegistry.registerTool只查isToolDisabled(tools.disabled+ per-server 过滤),从不咨询 PermissionManager(tool-registry.ts:289-300);deny 仅在调用时生效。settings.md 中被修正的tools.core行与迁移行准确。- ToolSearch 缺席时的两个 carve-out:两个调用点(
client.ts:1049、client.ts:2101)都先跑revealDeferredToolsReferencedInHistory,已 reveal 的名字被排除出withheld告警;isDeferredAndHidden与getDeferredToolSummary均排除visibleTools。新增句子与代码完全一致。 - SDK 文档的豁免工具族:与
isExemptFromEagerAllowList(permission-manager.ts:769-778)逐项一致。 - 副本同步:
settingsSchema.ts与settings.schema.json描述字节级一致;docs/developers/sdk-typescript.md与packages/sdk-typescript/README.md对应行字节级一致;4 个改动的文档/schema 文件prettier --check通过。
6. 静态检查
PR head 上 packages/core、packages/cli、packages/sdk-typescript(两份 tsconfig)的 tsc --noEmit 全部干净。
wenshao
approved these changes
Aug 31, 2026
Collaborator
|
@qwen-code /triage |
Collaborator
|
Sandboxed verification: The verification run did not complete, so the phases below may be partial or missing entirely. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 28 passed · 0 failed · 28 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：
|
| Mutant (production file, unmodified by PR) | base arm | head arm | attribution |
|---|---|---|---|
| unmutated control | 1070/1070 green | 1074/1074 green | harness live on both arms |
M1 rule-parser.ts: Object.hasOwn → in (prototype-inheriting lookup, the original bug class) |
KILLED, 1 fail: pre-existing resolveToolName returns unknown names unchanged (expected [Function Object] to be 'constructor') |
KILLED, 3 fails: same pre-existing test + both new (#10400) tests; the PM-level test reproduces the issue's exact crash TypeError: canonicalName.startsWith is not a function |
parser pin pre-existed; PR adds parser breadth + PM-level pin |
M2 permission-manager.ts: drop allowlist coverage check (defer everything unexempt) |
KILLED, 10 | KILLED, 12 | broad mutant; base already pinned alias coverage |
M2e config.ts registerLazy: demote LS whenever eager list active, ignoring entry coverage (the exact mutant the new test's comment names) |
SURVIVES 1070/1070 | KILLED, exactly 1: registers an enabled LS eagerly when tools.eager covers it (#10400) (expected [ 'tool_search', 'task_stop', …(2) ] to include 'list_directory') |
third LS×tools.eager cell unpinned before PR — new test load-bearing |
M3 tool-registry.ts: drop visibleTools clause from isDeferredAndHidden |
KILLED, 3 (ordinary-deferred visibleTools tests) | KILLED, 4 (same 3 + new test) | general carve-out pre-pinned; PR adds permission-deferred variant |
M3b tool-registry.ts: visibleTools rescue excludes permission-deferred tools |
SURVIVES 1070/1070 | KILLED, exactly 1: keeps the tool visible when listed in visibleTools |
permission-deferred×visibleTools combination unpinned before PR — new test load-bearing |
M5 permission-manager.ts initialize(): store raw entry instead of canonical name |
KILLED, 9 | KILLED, 11 | alias canonicalization pre-pinned (control) |
No mutant regressed killed→survived between arms. The two surgical survivors-on-base (M2e, M3b) are each killed on head by exactly the one new test written for them, failing on the intended expected-vs-actual assertion — the vacuity check passes for all three new tests.
Gate witness: evidence/02-head-gate-1074-tests.png (416 + 60 + 598 = 1074 passed across the three changed files).
2. Secondary claims
- Doc corrections are accurate (verified by code trace, quoted per sentence):
- settings.md
tools.core/migration rows: "permissions.deny… (MCP tools stay listed and are rejected at runtime — usetools.disabled/ the per-serverexcludeToolsfilter to remove them outright)". Code: built-in registration consultsgetToolRegistrationStatus()(whole-tool deny →disabled→registerLazyregisters nothing,config.ts:9183-9192); MCP tools are registered viaToolRegistry.discoverTools, which consults onlyConfig.disabledTools/aliases (tool-registry.ts:258-340), never permission deny; deny still applies at call time throughPermissionManager.evaluate()(mcp-tool.ts wirespermissionRulesinto the standard confirmation flow). Accurate. - settings.md
tools.eagercarve-outs: "resumed session history get their schemas re-sent without a warning" =client.ts revealDeferredToolsReferencedInHistory()(runs before reminders;resolveDeferredToolsForReminderthen excludes re-revealed permission-deferred tools from thewithheld/warning list,client.ts:1638-1735); "listed intools.visibleare declared up front" =tool-registry.ts isDeferredAndHidden/getDeferredToolSummaryvisibleTools clauses. Accurate. - sdk-typescript.md + README exempt families: "
tool_search,structured_output, plan-mode lifecycle tools,task_stop,mcp__*andcomputer_use__*" — exactly the six families inisExemptFromEagerAllowList(permission-manager.ts:768-778, withPLAN_LIFECYCLE_TOOLS= exit/enter_plan_mode + ask_user_question). Accurate. - Table churn is padding-only: after normalizing table cells, the content diff is 2 rows in settings.md (plus the migration row), 1 row each in the SDK docs/README — matching the PR's stated prettier re-alignment tradeoff.
- settings.md
- Schema copies in sync:
settingsSchema.ts↔settings.schema.jsontools.eagerdescriptions byte-identical (1183 chars, both carry the carve-out sentence);settings.mdrow carries it too. CI's own generated-artifact gate reproduced exactly:npm run generate:settings-schemaleavesgit status --porcelain packages/vscode-ide-companion/schemas/settings.schema.jsonempty. - Reviewer Test Plan walk: the two-file vitest command passes (plus tool-registry.test.ts, 1074 total); the
QWEN_CODE_DISABLE_CRON=1note is substantively correct (2 failures, byte-identical failing set on base → pre-existing), though one of the two is an eager-allowlist test, not a loop_wakeup test (Finding 2). settingsSchema tests: 48/48 green at head (PR said 47/47 — count drifted with base-tip merges; all green).
3. Findings (non-blocking)
F1 (nice-to-have) — new test code is not prettier-formatted. packages/core/src/tools/tool-registry.test.ts lines 950-952 (inside the PR's new keeps the tool visible when listed in visibleTools test) violate the repo prettier config; npx prettier --check fails on exactly this file of the 9 changed (the other 8 are clean). CI will not fail on it: the Run Prettier CI step is prettier --write . (scripts/lint.js:301, silent, no post-step tree-diff gate) and ESLint passes. But any local npm run format/preflight rewrites it:
- expect(registry.getDeferredToolSummary().map((t) => t.name)).not.toContain(
- 'hidden_by_allowlist',
- );
+ expect(
+ registry.getDeferredToolSummary().map((t) => t.name),
+ ).not.toContain('hidden_by_allowlist');Repro: npx prettier --check packages/core/src/tools/tool-registry.test.ts (exit 1 at head).
F2 (nice-to-have) — test-plan env note misnames one test. Under QWEN_CODE_DISABLE_CRON=1, the two failing tests are initialize registers loop_wakeup when cron is enabled and createToolRegistry registers tools.eager entries eagerly and demotes the rest to deferred (#9827, #10075) — one loop_wakeup test, not two. The substantive claim (pre-existing on clean main; run with the var unset) holds: the failing set is byte-identical at base.
4. Not covered
- Per-commit attribution: checkout is depth-2 (merge commit + base tip + PR head only;
git rev-list HEAD^1..HEAD^2returns 1 vs 7 commits in the snapshot) — the aggregateHEAD^1..HEADdiff was verified; per-commit rows were not individually exercised. - Historical claim "item 1 was already fixed before fix(core): decouple permissions.allow from tool registration via tools.eager (#10075) #10098 merged": needs deeper history than the shallow checkout. The current code has the fix (
Object.hasOwnatrule-parser.ts:310, no unknown-entry crash loop ininitialize()), and the M1/M2e/M3b mutants prove the tests pin it. npm run typecheckin packages/core and packages/cli: not re-run (the PR's own CI covers it; sdk-typescript typecheck was run here, clean, and the gate was proven live by planting a type error → exit 2).- End-to-end live-session verification of the two carve-outs (resumed-history re-send, tools.visible): verified by code trace only; no live resumed-session harness was driven.
- Issue item 5 (post-discovery validation of typo'd
mcp__/computer_use__*eager entries): explicitly deferred by the PR to fix(core):tools.eagerentry named after an Object.prototype key crashes PermissionManager.initialize (from #10098 round-4 rescan) #10400; not part of this diff. - Whole-repo gates (full suite, eslint repo-wide, actionlint/shellcheck/yamllint): not run; yamllint could not be installed in-container (
pip3: Permission deniedduringscripts/lint.js --setup), so that gate is unverified here. Note: while probing gates I rannode scripts/lint.js --prettier, which turned out to beprettier --write .and rewrote the tree (pre-existing drift in unrelated.githubfiles surfaced); the tree was fully restored to HEAD afterwards (git statusclean) and the finding above comes from a read-only--checkrun instead.
5. Methodology
Environment: CI verify container at refs/pull/10479/merge (depth 2), npm ci + npm run build pre-run. All measurements ran vitest from TypeScript source against the repo's installed node_modules (PR leaves lockfile untouched, so no dependency confound). The mutation matrix swapped the three test files between their HEAD^1 and HEAD versions (git show/git checkout), applied each mutant by hand to the production source, ran npx vitest run on the three files per arm (JSON reporter), and restored the tree after every cell; unmutated controls ran on both arms first. Harness, matrix artifacts, and raw logs live in tmp/pr10479-verify-20260831-174058/harness/ (rerun: node harness/verify-assertions.mjs); assertions.json counts only the 28 harness-level scripted checks — vitest per-test totals (1074/1070 etc.) are reported in the table above, not double-counted. Evidence images rendered with scripts/verify-capture.mjs. Doc claims verified by reading the exact production paths named in §2 and by a table-normalized content diff; schema sync by byte-comparison plus the repo's own generate:settings-schema freshness gate.
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/core/src/config/config.test.ts: (cd packages/core) npx --no-install vitest run ./src/config/config.test.ts
file packages/core/src/permissions/permission-manager.test.ts: (cd packages/core) npx --no-install vitest run ./src/permissions/permission-manager.test.ts
file packages/core/src/tools/tool-registry.test.ts: (cd packages/core) npx --no-install vitest run ./src/tools/tool-registry.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/config/config.test.ts: PPPPP
packages/core/src/permissions/permission-manager.test.ts: PPPPP
packages/core/src/tools/tool-registry.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/config/config.test.ts: P (exit 0)
round 1 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 1 · packages/core/src/tools/tool-registry.test.ts: P (exit 0)
round 2 · packages/core/src/config/config.test.ts: P (exit 0)
round 2 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 2 · packages/core/src/tools/tool-registry.test.ts: P (exit 0)
round 3 · packages/core/src/config/config.test.ts: P (exit 0)
round 3 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 3 · packages/core/src/tools/tool-registry.test.ts: P (exit 0)
round 4 · packages/core/src/config/config.test.ts: P (exit 0)
round 4 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 4 · packages/core/src/tools/tool-registry.test.ts: P (exit 0)
round 5 · packages/core/src/config/config.test.ts: P (exit 0)
round 5 · packages/core/src/permissions/permission-manager.test.ts: P (exit 0)
round 5 · packages/core/src/tools/tool-registry.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
approved these changes
Aug 31, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Collaborator
|
Released in v0.23.0. |
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
5 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
Follow-up to #10400. First finding: the item-1 runtime crash (a
tools.eagerentry named after anObject.prototypekey crashingPermissionManager.initialize()at startup) was already fixed before #10098 merged — the issue snapshot was taken at the stale PR head4a51304, while the merged headde60b7ffalready switchedresolveToolNameto own-property lookup (Object.hasOwn) and removed the crashing unknown-entry loop. This PR pins that contract with the regression tests the issue asks for, so a reintroduction fails CI:initialize()must tolerateconstructor/toString/valueOf/hasOwnProperty/isPrototypeOf/propertyIsEnumerable/toLocaleString/__proto__eager entries without throwing,resolveToolNamemust return those names unchanged as strings, and normal alias resolution stays intact. Second finding: it corrects the doc inaccuracies and the test-coverage gap among the remaining issue items, each re-verified against current main.Why it's needed
The "unusable entries never crash
initialize()" contract currently has no PermissionManager-level guard (only a parser-level pin forresolveToolName('constructor')), which is exactly the gap item 1 asks to close. Separately, the docs shipped by #10098 overstate whatpermissions.denydoes for MCP tools (MCP registration only consultstools.disabled; deny rejects calls at runtime), state the ToolSearch-absent caveat absolutely while the code implements two carve-outs (resumed-session history re-reveal andtools.visible), omit thetools.eagerexempt families in the SDK docs/README, andconfig.test.tsleaves the third reachable cell of the LS ×tools.eagermatrix unpinned.Reviewer Test Plan
How to verify
Run
npx vitest run src/permissions/permission-manager.test.ts src/config/config.test.tsinpackages/core— both files pass locally (416 + 580 tests). One environment note: a shell exportingQWEN_CODE_DISABLE_CRON=1breaks two pre-existingloop_wakeupregistration tests (present on clean main too); run with it unset. The new tests aretolerates Object.prototype-keyed entries without crashing (#10400)andreturns Object.prototype-keyed names unchanged (#10400)inpermission-manager.test.ts, andregisters an enabled LS eagerly when tools.eager covers it (#10400)inconfig.test.ts. The doc edits are wording-only; each corrected sentence is quoted in items 2/3/4 of the linked issue, and the schema/settingsSchemacopies were updated in sync (settingsSchema tests pass: 47/47).Evidence (Before & After)
N/A (tests and docs only, no user-visible behavior change)
Tested on
Environment (optional)
Unit tests only (vitest in packages/core and packages/cli), plus
npm run typecheckin packages/core, packages/cli and packages/sdk-typescript — all clean.Risk & Scope
mcp__/computer_use__*eager entries after discovery). The merge-time state deliberately removed the initialize-time unknown-entry warning, so the correct fix is registry-aware post-discovery validation (progressive MCP discovery, alias normalization) and it stays tracked in fix(core):tools.eagerentry named after an Object.prototype key crashes PermissionManager.initialize (from #10098 round-4 rescan) #10400 as follow-up. Item 1's runtime fix already exists on main (see above), so this PR only adds the regression pin for it.Linked Issues
Part of #10400
Remaining #10400 items not covered by this PR: item 1's runtime fix (already on main via the #10098 squash merge — this PR adds only the regression tests the issue requests) and item 5 (post-discovery validation of
mcp__/computer_use__*eager entries — left as follow-up, see Risk & Scope).中文说明
#10400 的后续处理。第一点：item 1 的运行时崩溃（
tools.eager里写了Object.prototype同名项导致PermissionManager.initialize()启动崩溃）在 #10098 合入前就已修复——issue 的代码快照取自旧的 PR head4a51304，而最终合入的 headde60b7ff已把resolveToolName改为只查 own property（Object.hasOwn），并移除了会崩溃的 unknown-entry 循环。本 PR 按 issue 的要求补上回归保护测试钉住该契约，防止回归：initialize()必须能容忍constructor/toString/valueOf/hasOwnProperty/isPrototypeOf/propertyIsEnumerable/toLocaleString/__proto__这些 eager 项而不抛错，resolveToolName必须原样返回这些名字（字符串），正常别名解析不受影响。第二点：修正 issue 其余条目中经当前 main 复核仍成立的文档不准确处和测试覆盖缺口。为什么需要
"不可用条目绝不导致
initialize()崩溃" 这一契约目前在 PermissionManager 层没有守卫（只有 parser 层的resolveToolName('constructor')钉住），这正是 item 1 要求补上的缺口。另外，#10098 带入的文档夸大了permissions.deny对 MCP 工具的作用（MCP 注册只查tools.disabled，deny 仅在调用时拒绝）；ToolSearch 缺席时的 caveat 写得过于绝对，而代码实际实现了两个 carve-out（续会话历史重新暴露、tools.visible）；SDK 文档/README 漏掉了tools.eager的 exempt 工具族；config.test.ts缺 LS ×tools.eager矩阵的第三个可达格。评审验证方式
在
packages/core下运行npx vitest run src/permissions/permission-manager.test.ts src/config/config.test.ts，本地两个文件全部通过（416 + 580 个用例）。环境注意：若 shell 导出了QWEN_CODE_DISABLE_CRON=1，会导致两个与本次改动无关的既有loop_wakeup注册测试失败（干净 main 上同样失败），去掉该变量即可。新增测试为permission-manager.test.ts中的tolerates Object.prototype-keyed entries without crashing (#10400)、returns Object.prototype-keyed names unchanged (#10400)，以及config.test.ts中的registers an enabled LS eagerly when tools.eager covers it (#10400)。文档改动仅为措辞，被修正的原句在 issue 的 item 2/3/4 中有引用，schema 与 settingsSchema 两份副本同步更新（settingsSchema 测试 47/47 通过）。证据（改动前后）
N/A（仅测试与文档，无用户可见行为变化）
测试环境
仅单元测试（packages/core 与 packages/cli 的 vitest），并在 packages/core、packages/cli、packages/sdk-typescript 分别跑了
npm run typecheck，全部干净。风险与范围
mcp__/computer_use__*eager 项告警）。合入版本有意删除了 initialize 期的 unknown-entry 警告，正确做法是 discovery 完成后做 registry-aware 校验（涉及增量 MCP discovery、别名归一化），继续留在 fix(core):tools.eagerentry named after an Object.prototype key crashes PermissionManager.initialize (from #10098 round-4 rescan) #10400 作为后续项。item 1 的运行时修复 main 上已存在（见上），本 PR 只补对应的回归测试。关联 Issue
Part of #10400
本 PR 未覆盖的 #10400 条目：item 1 的运行时修复（已随 #10098 squash 合入 main，本 PR 仅补 issue 要求的回归测试）；item 5（discovery 后校验
mcp__/computer_use__*eager 项，留作后续，见风险与范围）。