From 9545e63bd569859c1cee1fce6122dda82be6d322 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Wed, 26 Aug 2026 11:54:28 +0800 Subject: [PATCH 01/29] fix(core): honor empty core tool allowlists Co-authored-by: Qwen-Coder Co-authored-by: Qwen-Coder --- packages/cli/src/config/config.test.ts | 14 ++++++++++++ .../permissions/permission-manager.test.ts | 11 +++++++++- .../src/permissions/permission-manager.ts | 22 ++++++++++++++----- 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 81b8ee64e6c..a2e7651ebde 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3025,6 +3025,20 @@ describe('Approval mode tool exclusion logic', () => { expect(excludedTools).toContain(ToolNames.WRITE_FILE); }); + it('should preserve an explicitly empty tools.core allowlist', async () => { + process.argv = ['node', 'script.js', '-p', 'test']; + const argv = await parseArguments(); + const settings: Settings = { + tools: { + core: [], + }, + }; + + const config = await loadCliConfig(settings, argv, undefined, []); + + expect(config.getCoreTools()).toEqual([]); + }); + it('should exclude only shell tools in non-interactive mode with auto-edit approval mode', async () => { process.argv = [ 'node', diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 37694cd08e9..997346c3302 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -2567,11 +2567,20 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('read_file')).toBe(false); }); - it('empty coreTools: all tools enabled (no whitelist restriction)', async () => { + it('empty coreTools disables every tool', async () => { pm = new PermissionManager(makeConfig({ coreTools: [] })); pm.initialize(); + expect(await pm.isToolEnabled('read_file')).toBe(false); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); + expect(await pm.isToolEnabled('agent')).toBe(false); + }); + + it('undefined coreTools keeps tools enabled', async () => { + pm = new PermissionManager(makeConfig()); + pm.initialize(); expect(await pm.isToolEnabled('read_file')).toBe(true); expect(await pm.isToolEnabled('run_shell_command')).toBe(true); + expect(await pm.isToolEnabled('agent')).toBe(true); }); it('coreTools allowlist + deny rule: deny takes precedence for listed tools', async () => { diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index f6aa226b820..60b161b7471 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -232,8 +232,10 @@ export class PermissionManager { // Each entry may be a bare name ("Bash", "read_file") or include a specifier // ("Bash(ls -l)") – we normalise to canonical tool names and ignore specifiers // because the registry check is at the tool level, not the invocation level. + // An explicitly configured empty list is still an active allowlist: it + // disables every core tool. `undefined` alone means no restriction. const rawCoreTools = this.config.getCoreTools?.(); - if (rawCoreTools && rawCoreTools.length > 0) { + if (rawCoreTools !== undefined) { this.coreToolsAllowList = new Set( rawCoreTools.map((t) => parseRule(t).toolName), ); @@ -794,7 +796,14 @@ export class PermissionManager { return false; } - // Non-core tools bypass coreTools allowlist check + // An explicitly empty coreTools allowlist means no tools. Apply this before + // the historical non-core exemption so `tools.core: []` is an actual + // tool-free configuration rather than silently sending every schema. + if (this.coreToolsAllowList?.size === 0) { + return false; + } + + // Non-core tools bypass a non-empty coreTools allowlist check. if (!this.isCoreTool(canonicalName)) { const decision = await this.evaluate({ toolName: canonicalName }); return decision !== 'deny'; @@ -803,10 +812,11 @@ export class PermissionManager { // Core tools: if a coreTools allowlist is active, only explicitly listed // tools are registered. This mirrors the legacy `tools.core` whitelist // semantic: any tool NOT in the allowlist is excluded from the registry. - if (this.coreToolsAllowList !== null && this.coreToolsAllowList.size > 0) { - if (!this.coreToolsAllowList.has(canonicalName)) { - return false; - } + if ( + this.coreToolsAllowList !== null && + !this.coreToolsAllowList.has(canonicalName) + ) { + return false; } // evaluate({ toolName }) without a command will only match rules that have From 7e7a12b929999acd6cd3e989dcef99aaeb75797a Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Wed, 26 Aug 2026 18:50:11 +0800 Subject: [PATCH 02/29] fix(core): enforce empty coreTools allowlist for MCP tools and rejection messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on the empty coreTools allowlist semantic (#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 --- docs/users/features/structured-output.md | 12 +- packages/cli/src/config/config.test.ts | 28 +++++ packages/cli/src/config/config.ts | 10 +- .../core/src/core/coreToolScheduler.test.ts | 108 ++++++++++++++++++ packages/core/src/core/coreToolScheduler.ts | 15 +++ .../src/permissions/permission-manager.ts | 39 ++++++- packages/core/src/tools/tool-registry.test.ts | 51 +++++++++ packages/core/src/tools/tool-registry.ts | 24 ++++ 8 files changed, 277 insertions(+), 10 deletions(-) diff --git a/docs/users/features/structured-output.md b/docs/users/features/structured-output.md index 78c7e30eee1..4a8ded36aec 100644 --- a/docs/users/features/structured-output.md +++ b/docs/users/features/structured-output.md @@ -259,9 +259,15 @@ synthetic tool is registered when the CLI parses its arguments, so: ## Permission gating -`structured_output` deliberately bypasses the `--core-tools` allowlist: -the tool only exists when `--json-schema` is set, so excluding it -would leave the run with no terminal contract. +`structured_output` deliberately bypasses a NON-EMPTY `--core-tools` +allowlist: the tool only exists when `--json-schema` is set, so +excluding it would leave the run with no terminal contract. + +An explicitly EMPTY allowlist (`tools.core: []`) is the deliberate +"no tools at all" configuration: it disables `structured_output` too, +so a `--json-schema` run under `tools.core: []` cannot complete — the +model has no terminal contract to end the run. Remove the setting or +list the tools you need if you want structured output to finish. Explicit `permissions.deny` rules and `--exclude-tools` settings DO take effect — both use the same deny mechanism and both prevent diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index a2e7651ebde..04bde53f40f 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3039,6 +3039,34 @@ describe('Approval mode tool exclusion logic', () => { expect(config.getCoreTools()).toEqual([]); }); + it('should treat a bare --core-tools flag with no values as absent', async () => { + // yargs yields `[]` for the bare flag. Treating that argv-sourced + // empty list as "disable every tool" would silently tool-free the + // session on a forgotten flag value; only `tools.core: []` in + // settings should carry the empty-allowlist semantic (#10065). + process.argv = ['node', 'script.js', '--core-tools', '-p', 'test']; + const argv = await parseArguments(); + const settings: Settings = {}; + + const config = await loadCliConfig(settings, argv, undefined, []); + + expect(config.getCoreTools()).toBeUndefined(); + }); + + it('should fall back to settings tools.core when --core-tools has no values', async () => { + process.argv = ['node', 'script.js', '--core-tools', '-p', 'test']; + const argv = await parseArguments(); + const settings: Settings = { + tools: { + core: ['read_file'], + }, + }; + + const config = await loadCliConfig(settings, argv, undefined, []); + + expect(config.getCoreTools()).toEqual(['read_file']); + }); + it('should exclude only shell tools in non-interactive mode with auto-edit approval mode', async () => { process.argv = [ 'node', diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 6379739b117..ac359c1bcf0 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -2153,10 +2153,18 @@ export async function loadCliConfig( systemPrompt: argv.systemPrompt, appendSystemPrompt: argv.appendSystemPrompt, // Legacy fields – kept for backward compatibility with getCoreTools() etc. + // A bare `--core-tools` flag with no values yields `[]` from yargs. + // Treat an argv-sourced empty list as ABSENT, not as "disable every + // tool": only an explicit `tools.core: []` in settings (or a valued + // flag) should carry the empty-allowlist semantic, so a forgotten + // flag value — or a script expanding an unset variable into the flag + // — cannot silently flip a session to tool-free (#10065). coreTools: bareMode || safeMode ? undefined - : argv.coreTools || settings.tools?.core || undefined, + : argv.coreTools?.length + ? argv.coreTools + : settings.tools?.core, allowedTools: bareMode || safeMode ? argv.allowedTools || undefined diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 0c26ac8ee5c..041020b79a7 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -3770,6 +3770,114 @@ describe('CoreToolScheduler', () => { expect(execute).not.toHaveBeenCalled(); }); + it('attributes the rejection to the empty tools.core allowlist instead of promising a permissions.allow fix (#10065)', async () => { + // With an active `permissions.allow` allowlist AND `tools.core: []`, + // an UNCOVERED tool used to get the "add a permissions.allow rule" + // advice — which can never succeed because the empty coreTools gate + // rejects the tool even after it becomes covered. The message must + // name `tools.core` instead (#10065). + const execute = vi.fn().mockResolvedValue({ + llmContent: 'sent', + returnDisplay: 'sent', + }); + const toolsByName = new Map([ + [ToolNames.EDIT, new MockTool({ name: ToolNames.EDIT, execute })], + ]); + const permissionManager = { + isToolEnabled: vi.fn().mockResolvedValue(false), + findMatchingDenyRule: vi.fn().mockReturnValue(undefined), + isCoreToolsAllowListEmpty: vi.fn().mockReturnValue(true), + isPermissionsAllowListActive: vi.fn().mockReturnValue(true), + isCoveredByAllowOrAskRule: vi.fn().mockReturnValue(false), + }; + const { scheduler, onAllToolCallsComplete } = + createSchedulerForLegacyToolTests({ + toolsByName, + permissionManager, + }); + + await scheduler.schedule( + [ + { + callId: 'empty-core-tools-uncovered', + name: ToolNames.EDIT, + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-empty-core-tools-uncovered', + }, + ], + new AbortController().signal, + ); + + expect(onAllToolCallsComplete).toHaveBeenCalled(); + const completedCalls = onAllToolCallsComplete.mock + .calls[0][0] as ToolCall[]; + const completedCall = completedCalls[0]; + expect(completedCall.status).toBe('error'); + if (completedCall.status === 'error') { + expect(completedCall.response.errorType).toBe( + ToolErrorType.EXECUTION_DENIED, + ); + const message = completedCall.response.error?.message ?? ''; + expect(message).toContain('tools.core'); + expect(message).toContain('"edit"'); + expect(message).not.toContain('permissions.allow'); + expect(message).not.toContain('permission was declined'); + } + expect(execute).not.toHaveBeenCalled(); + }); + + it('names tools.core for a covered tool rejected by the empty allowlist instead of the generic declined message (#10065)', async () => { + // Covered by an allow rule but still rejected by `tools.core: []`: + // the generic "permission was declined" text is factually wrong + // (nothing was ever asked or declined), so the attribution must + // name the empty allowlist (#10065). + const execute = vi.fn().mockResolvedValue({ + llmContent: 'sent', + returnDisplay: 'sent', + }); + const toolsByName = new Map([ + [ToolNames.EDIT, new MockTool({ name: ToolNames.EDIT, execute })], + ]); + const permissionManager = { + isToolEnabled: vi.fn().mockResolvedValue(false), + findMatchingDenyRule: vi.fn().mockReturnValue(undefined), + isCoreToolsAllowListEmpty: vi.fn().mockReturnValue(true), + isPermissionsAllowListActive: vi.fn().mockReturnValue(true), + isCoveredByAllowOrAskRule: vi.fn().mockReturnValue(true), + }; + const { scheduler, onAllToolCallsComplete } = + createSchedulerForLegacyToolTests({ + toolsByName, + permissionManager, + }); + + await scheduler.schedule( + [ + { + callId: 'empty-core-tools-covered', + name: ToolNames.EDIT, + args: {}, + isClientInitiated: false, + prompt_id: 'prompt-empty-core-tools-covered', + }, + ], + new AbortController().signal, + ); + + expect(onAllToolCallsComplete).toHaveBeenCalled(); + const completedCalls = onAllToolCallsComplete.mock + .calls[0][0] as ToolCall[]; + const completedCall = completedCalls[0]; + expect(completedCall.status).toBe('error'); + if (completedCall.status === 'error') { + const message = completedCall.response.error?.message ?? ''; + expect(message).toContain('tools.core'); + expect(message).not.toContain('permission was declined'); + } + expect(execute).not.toHaveBeenCalled(); + }); + it('keeps the legacy declined message when the tool is disabled without an active allowlist (#9827)', async () => { const execute = vi.fn().mockResolvedValue({ llmContent: 'sent', diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index d86ae260042..ec6d1bcb2ae 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -2473,6 +2473,21 @@ export class CoreToolScheduler { let permissionErrorMessage: string; if (matchingRule) { permissionErrorMessage = `Qwen Code requires permission to use "${reqInfo.name}", but that permission was declined. Matching deny rule: "${matchingRule}".`; + } else if ( + // An explicitly empty coreTools allowlist (`tools.core: []`) + // rejects EVERY tool, so the generic "permission was + // declined" text (nothing was ever asked or declined) and + // the `permissions.allow` remediation below are both wrong: + // adding an allow rule can never satisfy the empty gate. + // Name the responsible knob instead (#10065). The optional + // call keeps scoped PermissionManager shims from throwing + // until they grow the method; this branch sits ahead of the + // allowlist-advice branch because the empty gate rejects + // covered and uncovered tools alike. + typeof pm.isCoreToolsAllowListEmpty === 'function' && + pm.isCoreToolsAllowListEmpty() + ) { + permissionErrorMessage = `"${reqInfo.name}" is disabled because the core tools allowlist (settings tools.core / --core-tools) is explicitly empty. Remove the setting or list the tool there to re-enable it.`; } else if ( pm.isPermissionsAllowListActive() && // Only attribute the miss to `permissions.allow` when the tool diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index 60b161b7471..d1f8021961c 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -233,7 +233,9 @@ export class PermissionManager { // ("Bash(ls -l)") – we normalise to canonical tool names and ignore specifiers // because the registry check is at the tool level, not the invocation level. // An explicitly configured empty list is still an active allowlist: it - // disables every core tool. `undefined` alone means no restriction. + // disables every registered tool — core and non-core (MCP, Skill, + // Agent, synthetic system tools) alike (see `isToolEnabled`). + // `undefined` alone means no restriction. const rawCoreTools = this.config.getCoreTools?.(); if (rawCoreTools !== undefined) { this.coreToolsAllowList = new Set( @@ -657,8 +659,11 @@ export class PermissionManager { * `structured_output` (registered only when `--json-schema` is set). * Excluding `structured_output` from `--core-tools` would leave a * `--json-schema` run with no terminal contract, so the synthetic - * tool stays available regardless of the allowlist (deny rules still - * apply). + * tool stays available regardless of a NON-EMPTY allowlist (deny + * rules still apply). An explicitly EMPTY allowlist (`tools.core: []`) + * is the deliberate "no tools at all" configuration and disables even + * these synthetic tools, so a `--json-schema` run under it cannot + * complete (#10065). */ private static readonly CORE_TOOLS = new Set([ 'read_file', @@ -711,6 +716,11 @@ export class PermissionManager { * Determine whether a tool should be present in the tool registry. * * A tool is disabled (returns false) when: + * - the coreTools allowlist is explicitly empty (`tools.core: []`) — the + * deliberate "no tools at all" configuration disables EVERY tool, + * non-core tools included (#10065), or + * - a non-empty coreTools allowlist is active and the tool is a core + * tool not in the list, or * - the `permissions.allow` registry allowlist is active and the tool is * not covered by any allow or ask rule (see * `isPermissionsAllowListActive`; ask rules keep their tool registered @@ -724,9 +734,10 @@ export class PermissionManager { * the tool in the registry — the allowlist is tool-level, not * invocation-level. * - * Non-core tools (MCP, Skill, Agent, etc.) skip the coreTools allowlist - * check because they are dynamically discovered or essential for system - * operation, but they ARE subject to the `permissions.allow` registry + * Non-core tools (MCP, Skill, Agent, etc.) skip a NON-EMPTY coreTools + * allowlist check because they are dynamically discovered or essential + * for system operation (an explicitly empty allowlist still disables + * them, see above), but they ARE subject to the `permissions.allow` registry * allowlist (except MCP tools, `structured_output`, and the * `computer_use__*` family, see below) — that is the documented migration * semantic of the legacy `tools.core` whitelist ("unlisted tools are @@ -834,6 +845,22 @@ export class PermissionManager { return this.permissionsAllowListActive; } + /** + * Whether the coreTools allowlist is explicitly empty (`tools.core: []` + * in settings or a bare/valueless `--core-tools` resolved to a list). + * When true, `isToolEnabled()` rejects EVERY tool — core and non-core + * alike — so user-facing remediation advice must name this knob instead + * of promising re-enablement through other gates (e.g. adding a + * `permissions.allow` rule can never satisfy an empty coreTools + * allowlist, #10065). `undefined` coreTools (no restriction) and + * non-empty allowlists both return false. + */ + isCoreToolsAllowListEmpty(): boolean { + return ( + this.coreToolsAllowList !== null && this.coreToolsAllowList.size === 0 + ); + } + /** * All allow rules currently in force: persistent + session + any rules * the AUTO-mode strip moved to the stash (they are configured rules, diff --git a/packages/core/src/tools/tool-registry.test.ts b/packages/core/src/tools/tool-registry.test.ts index 7b3f1e932af..d8f74f3184b 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -161,6 +161,57 @@ describe('ToolRegistry', () => { expect(toolRegistry.getTool('mock-tool')).toBe(tool); }); + it('skips registration when tools.core is an explicitly empty allowlist (#10065)', () => { + // Built-ins are gated at `registerLazy` and command-discovered tools + // at the async discovery gate, but MCP-discovered tools flow only + // through `registerTool` (both the legacy `McpClient.discover()` + // path and the pooled `SessionMcpView.applyTools` path end here). + // Without this guard `tools.core: []` would keep them registered — + // advertised to the model, then every invocation rejected at the + // runtime gate. + const pm = new PermissionManager({ + getPermissionsAllow: () => [], + getPermissionsAsk: () => [], + getPermissionsDeny: () => [], + getCoreTools: () => [], + getRegistryAllowList: () => [], + getProjectRoot: () => '/test/dir', + getCwd: () => '/test/dir', + getApprovalMode: () => 'default', + }); + pm.initialize(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(true); + vi.spyOn(config, 'getPermissionManager').mockReturnValue(pm); + + const tool = new MockTool({ name: 'mcp__srv__foo' }); + toolRegistry.registerTool(tool); + + expect(toolRegistry.getTool('mcp__srv__foo')).toBeUndefined(); + }); + + it('still registers tools when the coreTools allowlist is unset (#10065)', () => { + // `undefined` coreTools means "no restriction" — the empty-allowlist + // guard must not fire for it. + const pm = new PermissionManager({ + getPermissionsAllow: () => [], + getPermissionsAsk: () => [], + getPermissionsDeny: () => [], + getCoreTools: () => undefined, + getRegistryAllowList: () => [], + getProjectRoot: () => '/test/dir', + getCwd: () => '/test/dir', + getApprovalMode: () => 'default', + }); + pm.initialize(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(false); + vi.spyOn(config, 'getPermissionManager').mockReturnValue(pm); + + const tool = new MockTool({ name: 'mcp__srv__foo' }); + toolRegistry.registerTool(tool); + + expect(toolRegistry.getTool('mcp__srv__foo')).toBe(tool); + }); + it('renames an MCP tool whose name shadows a registered lazy factory', async () => { // The synthetic `structured_output` tool registers via // `registerFactory` (lazy). Without this guard, an MCP server diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 362727b279e..e381f97103a 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -289,6 +289,30 @@ export class ToolRegistry { ); return; } + // An explicitly empty coreTools allowlist (`tools.core: []`) disables + // every tool. Built-ins are already skipped at `registerLazy` + // (config.ts) and command-discovered tools at the async + // `isToolEnabled` gate below in `discoverAndRegisterToolsFromCommand`, + // but MCP-discovered tools only flow through `registerTool` — both the + // legacy `McpClient.discover()` path and the pooled + // `SessionMcpView.applyTools` path end here. Without this guard they + // would stay registered and advertised under `[]`, then every call + // would be rejected at the runtime gate: advertised-then-rejected, + // the exact failure mode the empty allowlist is meant to remove + // (#10065). The optional call keeps scoped PermissionManager shims + // (installed via `as unknown as PermissionManager`) from throwing + // until they grow the method. + const pmForEmptyGate = this.config.getPermissionManager?.(); + if ( + pmForEmptyGate && + typeof pmForEmptyGate.isCoreToolsAllowListEmpty === 'function' && + pmForEmptyGate.isCoreToolsAllowListEmpty() + ) { + debugLogger.info( + `Tool "${tool.name}" skipped: tools.core is an explicitly empty allowlist (#10065).`, + ); + return; + } // A name collision can happen against either the eager `tools` map // (already-instantiated tools) or the lazy `factories` map (registered // but not yet constructed — `structured_output` lives here when From 762f662e70f7132b548f4e675b1e1cd788937798 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Wed, 26 Aug 2026 20:57:44 +0800 Subject: [PATCH 03/29] fix(core): keep coreTools allowlist null-tolerant and single-sourced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/core/src/core/coreToolScheduler.ts | 2 +- .../permissions/permission-manager.test.ts | 22 +++++++++++++++++ .../src/permissions/permission-manager.ts | 24 +++++++++++-------- 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index ec6d1bcb2ae..e5e0def3dcd 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -2487,7 +2487,7 @@ export class CoreToolScheduler { typeof pm.isCoreToolsAllowListEmpty === 'function' && pm.isCoreToolsAllowListEmpty() ) { - permissionErrorMessage = `"${reqInfo.name}" is disabled because the core tools allowlist (settings tools.core / --core-tools) is explicitly empty. Remove the setting or list the tool there to re-enable it.`; + permissionErrorMessage = `"${reqInfo.name}" is disabled because the core tools allowlist (settings tools.core) is explicitly empty. Remove the setting or list the tool there to re-enable it.`; } else if ( pm.isPermissionsAllowListActive() && // Only attribute the miss to `permissions.allow` when the tool diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 997346c3302..9f5f946b1b7 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -2583,6 +2583,28 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('agent')).toBe(true); }); + it('null or non-array coreTools is unrestricted, not an empty allowlist', async () => { + // `"tools": { "core": null }` is the common JSON idiom for clearing + // the deprecated key, and settings loading performs no type + // validation, so null (or any non-array value) reaches + // `initialize()` raw. It must not throw and must behave like "no + // allowlist", not like the tool-disabling explicit `[]`. + pm = new PermissionManager( + makeConfig({ coreTools: null as unknown as string[] }), + ); + expect(() => pm.initialize()).not.toThrow(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(false); + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('agent')).toBe(true); + + pm = new PermissionManager( + makeConfig({ coreTools: {} as unknown as string[] }), + ); + expect(() => pm.initialize()).not.toThrow(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(false); + expect(await pm.isToolEnabled('read_file')).toBe(true); + }); + it('coreTools allowlist + deny rule: deny takes precedence for listed tools', async () => { pm = new PermissionManager( makeConfig({ diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index d1f8021961c..09326cad0cd 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -235,9 +235,11 @@ export class PermissionManager { // An explicitly configured empty list is still an active allowlist: it // disables every registered tool — core and non-core (MCP, Skill, // Agent, synthetic system tools) alike (see `isToolEnabled`). - // `undefined` alone means no restriction. + // Only an array activates the allowlist: `undefined`, `null`, or any + // non-array value (e.g. `"tools": { "core": null }`, the common JSON + // idiom for clearing this deprecated key) means no restriction. const rawCoreTools = this.config.getCoreTools?.(); - if (rawCoreTools !== undefined) { + if (Array.isArray(rawCoreTools)) { this.coreToolsAllowList = new Set( rawCoreTools.map((t) => parseRule(t).toolName), ); @@ -810,7 +812,7 @@ export class PermissionManager { // An explicitly empty coreTools allowlist means no tools. Apply this before // the historical non-core exemption so `tools.core: []` is an actual // tool-free configuration rather than silently sending every schema. - if (this.coreToolsAllowList?.size === 0) { + if (this.isCoreToolsAllowListEmpty()) { return false; } @@ -847,13 +849,15 @@ export class PermissionManager { /** * Whether the coreTools allowlist is explicitly empty (`tools.core: []` - * in settings or a bare/valueless `--core-tools` resolved to a list). - * When true, `isToolEnabled()` rejects EVERY tool — core and non-core - * alike — so user-facing remediation advice must name this knob instead - * of promising re-enablement through other gates (e.g. adding a - * `permissions.allow` rule can never satisfy an empty coreTools - * allowlist, #10065). `undefined` coreTools (no restriction) and - * non-empty allowlists both return false. + * in settings). A bare/valueless `--core-tools` never produces this + * state — `loadCliConfig` treats an argv-sourced empty list as absent — + * and neither do `null`/non-array values, which `initialize()` treats + * as no restriction. When true, `isToolEnabled()` rejects EVERY tool — + * core and non-core alike — so user-facing remediation advice must name + * this knob instead of promising re-enablement through other gates + * (e.g. adding a `permissions.allow` rule can never satisfy an empty + * coreTools allowlist, #10065). `undefined`/`null`/non-array coreTools + * (no restriction) and non-empty allowlists both return false. */ isCoreToolsAllowListEmpty(): boolean { return ( From c548c0faa8b875995ad42ead7c63e3cad8c3f7a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Wed, 26 Aug 2026 22:20:15 +0800 Subject: [PATCH 04/29] test(core): pin structured_output and mcp tools under empty coreTools 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 (#10065). Co-authored-by: Qwen-Coder --- packages/core/src/permissions/permission-manager.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 9f5f946b1b7..e55a15fce69 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -2573,6 +2573,12 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('read_file')).toBe(false); expect(await pm.isToolEnabled('run_shell_command')).toBe(false); expect(await pm.isToolEnabled('agent')).toBe(false); + // The empty gate must also win against the `isToolEnabled` + // exemptions: `structured_output` bypasses the permissions.allow + // gate and `mcp__*` names bypass non-empty coreTools allowlists, + // so pin both as disabled under `[]` (#10065). + expect(await pm.isToolEnabled('structured_output')).toBe(false); + expect(await pm.isToolEnabled('mcp__server__tool')).toBe(false); }); it('undefined coreTools keeps tools enabled', async () => { From e37a11af117e35bf6c1d4d4a20f0bff98fe3ad15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Wed, 26 Aug 2026 22:21:02 +0800 Subject: [PATCH 05/29] test(cli): cover argv --core-tools precedence over settings tools.core 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 (#10065). Co-authored-by: Qwen-Coder --- packages/cli/src/config/config.test.ts | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 04bde53f40f..5d0ec8a9f4d 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3067,6 +3067,30 @@ describe('Approval mode tool exclusion logic', () => { expect(config.getCoreTools()).toEqual(['read_file']); }); + it('should prefer --core-tools over settings tools.core when both are set', async () => { + // argv-over-settings precedence on the resolved coreTools ternary: + // a valued --core-tools flag must win over settings tools.core + // instead of being overridden by it (#10065). + process.argv = [ + 'node', + 'script.js', + '--core-tools', + 'web_fetch', + '-p', + 'test', + ]; + const argv = await parseArguments(); + const settings: Settings = { + tools: { + core: ['read_file'], + }, + }; + + const config = await loadCliConfig(settings, argv, undefined, []); + + expect(config.getCoreTools()).toEqual(['web_fetch']); + }); + it('should exclude only shell tools in non-interactive mode with auto-edit approval mode', async () => { process.argv = [ 'node', From 9f9420bac7d24ad75a29908c88815c98f0704843 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Wed, 26 Aug 2026 22:21:33 +0800 Subject: [PATCH 06/29] fix(cli): warn at startup when tools.core is an empty allowlist 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 (#10065). Co-authored-by: Qwen-Coder --- packages/cli/src/config/config.test.ts | 30 ++++++++++++++++++++++++++ packages/cli/src/config/config.ts | 16 ++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 5d0ec8a9f4d..5941a210d9c 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3091,6 +3091,36 @@ describe('Approval mode tool exclusion logic', () => { expect(config.getCoreTools()).toEqual(['web_fetch']); }); + it('should warn at startup when tools.core is an explicitly empty allowlist', async () => { + process.argv = ['node', 'script.js', '-p', 'test']; + const argv = await parseArguments(); + const emptySettings: Settings = { + tools: { + core: [], + }, + }; + + mockWriteStderrLine.mockClear(); + const config = await loadCliConfig(emptySettings, argv, undefined, []); + expect(config.getCoreTools()).toEqual([]); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + + // A non-empty allowlist must not trigger the notice. + const valuedSettings: Settings = { + tools: { + core: ['read_file'], + }, + }; + + mockWriteStderrLine.mockClear(); + await loadCliConfig(valuedSettings, argv, undefined, []); + expect(mockWriteStderrLine).not.toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + }); + it('should exclude only shell tools in non-interactive mode with auto-edit approval mode', async () => { process.argv = [ 'node', diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index ac359c1bcf0..3e377998ac9 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -1767,6 +1767,22 @@ export async function loadCliConfig( '⚠ Safe mode: --core-tools flag is ignored (settings-sourced core tools are also disabled).\n', ); } + // An explicit `tools.core: []` in settings disables every tool for the + // session (the empty-allowlist semantic of #10065). Give the user one + // visible startup notice instead of failing every tool call silently; + // the argv flag wins over settings when valued, so only the settings + // path can resolve to the empty allowlist here. + if ( + !bareMode && + !safeMode && + !(argv.coreTools && argv.coreTools.length > 0) && + Array.isArray(settings.tools?.core) && + settings.tools.core.length === 0 + ) { + writeStderrLine( + '⚠ tools.core is an empty allowlist: all tools are disabled for this session. Add tool names to tools.core or remove the key to re-enable tools (#10065).\n', + ); + } const resolvedCoreTools: string[] = [ ...(bareMode || safeMode ? [] : (argv.coreTools ?? [])), ...(bareMode || safeMode ? [] : (settings.tools?.core ?? [])), From 1ac1ecd8a0c4a350845723d2c684f8dfdde9af00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Thu, 27 Aug 2026 14:08:17 +0800 Subject: [PATCH 07/29] fix(core): normalize empty/non-array tools.core; restart caveat + notice pins (#10065) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix(core): normalize empty and non-array tools.core values (#10065) Drop name-less entries (empty string, whitespace-only, non-string) when building the coreTools allowlist in PermissionManager.initialize(), so a settings `"core": [""]` collapses to the explicit EMPTY allowlist — isCoreToolsAllowListEmpty() true, the scheduler names tools.core, and the startup notice fires for all-empty-string lists too — instead of a size-1 allowlist that matches nothing and silently disables every tool. The `--core-tools` coerce filters empty entries after trim, so `--core-tools ""` / `--core-tools ,` (e.g. a script expanding an unset variable into the flag) take the absent-flag path instead of yielding a [""] allowlist. The coreTools ternary's settings leg is Array.isArray-guarded so a hand-edited non-array `"core": ""` normalizes to undefined instead of flowing raw into Config.coreTools, where getCoreTools()?.some(...) would throw during tool-registry construction. Co-authored-by: Qwen-Coder fix(core): add restart caveat to empty tools.core message; pin notice suppressors (#10065) The scheduler's empty-allowlist rejection message now carries the restart caveat ("..., and restart, to re-enable it") like its sibling messages: coreToolsAllowList is snapshotted once in PermissionManager.initialize() and the settings schema marks tools.core requiresRestart, so editing settings mid-session cannot re-enable tools until restart. Pinned by expect(message).toContain('restart') in the first #10065 scheduler test. Also add discriminating tests for the empty-allowlist startup notice's three suppression conjuncts, each red under the corresponding one-conjunct mutation: a valued --core-tools over an empty settings list, safe mode, and bare mode each suppress the notice, while a bare --core-tools flag (treated as absent) over `tools.core: []` still fires it. Co-authored-by: Qwen-Coder --- packages/cli/src/config/config.test.ts | 163 ++++++++++++++++++ packages/cli/src/config/config.ts | 33 +++- .../core/src/core/coreToolScheduler.test.ts | 5 + packages/core/src/core/coreToolScheduler.ts | 2 +- .../permissions/permission-manager.test.ts | 25 +++ .../src/permissions/permission-manager.ts | 15 +- 6 files changed, 235 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index a1146fa2e9e..a2efcf9865f 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3121,6 +3121,169 @@ describe('Approval mode tool exclusion logic', () => { ); }); + it('should treat empty-string --core-tools values as absent, not as a [""] allowlist', async () => { + // A script expanding an unset variable (`qwen --core-tools "$TOOLS"`) + // hands yargs `[""]`; `--core-tools ,` yields `["", ""]`. Neither + // carries a tool name, so both must collapse to the absent-flag path + // instead of a one-entry allowlist that matches nothing and silently + // disables every tool with neither diagnostic firing (#10065). + for (const emptyValue of ['', ',']) { + process.argv = [ + 'node', + 'script.js', + '--core-tools', + emptyValue, + '-p', + 'test', + ]; + const argv = await parseArguments(); + const emptySettings: Settings = {}; + + mockWriteStderrLine.mockClear(); + const config = await loadCliConfig(emptySettings, argv, undefined, []); + expect(config.getCoreTools()).toBeUndefined(); + expect(mockWriteStderrLine).not.toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + } + + // Valued entries survive the empty-entry filter. + process.argv = [ + 'node', + 'script.js', + '--core-tools', + 'read_file,', + '-p', + 'test', + ]; + const argv = await parseArguments(); + const settings: Settings = {}; + + const config = await loadCliConfig(settings, argv, undefined, []); + expect(config.getCoreTools()).toEqual(['read_file']); + }); + + it('should warn for an all-empty-string tools.core exactly like for []', async () => { + // `"core": [""]` (an unset-variable expansion or hand-edit written + // into settings) carries no tool name: PermissionManager.initialize() + // collapses it to the explicit empty allowlist, so the startup notice + // must fire for it too (#10065). A list still naming a tool must not. + process.argv = ['node', 'script.js', '-p', 'test']; + const argv = await parseArguments(); + const emptyEntrySettings: Settings = { + tools: { + core: [''], + }, + }; + + mockWriteStderrLine.mockClear(); + await loadCliConfig(emptyEntrySettings, argv, undefined, []); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + + const valuedSettings: Settings = { + tools: { + core: ['', 'read_file'], + }, + }; + + mockWriteStderrLine.mockClear(); + await loadCliConfig(valuedSettings, argv, undefined, []); + expect(mockWriteStderrLine).not.toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + }); + + it('should treat a non-array tools.core value as absent', async () => { + // Settings loading performs no type validation, so a hand-edited + // `"core": ""` reaches the coreTools ternary raw; it must normalize + // to ABSENT (undefined) instead of flowing into Config.coreTools, + // where `getCoreTools()?.some(...)` would throw during tool-registry + // construction (#10065). + process.argv = ['node', 'script.js', '-p', 'test']; + const argv = await parseArguments(); + const settings: Settings = { + tools: { + core: '' as unknown as string[], + }, + }; + + const config = await loadCliConfig(settings, argv, undefined, []); + expect(config.getCoreTools()).toBeUndefined(); + }); + + it('should suppress the empty-allowlist notice when a valued --core-tools overrides the empty settings list', async () => { + // A valued argv flag wins over settings (its tools ARE registered), + // so the "all tools are disabled" notice would be factually wrong + // here (#10065). + process.argv = [ + 'node', + 'script.js', + '--core-tools', + 'web_fetch', + '-p', + 'test', + ]; + const argv = await parseArguments(); + const settings: Settings = { + tools: { + core: [], + }, + }; + + mockWriteStderrLine.mockClear(); + const config = await loadCliConfig(settings, argv, undefined, []); + expect(config.getCoreTools()).toEqual(['web_fetch']); + expect(mockWriteStderrLine).not.toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + }); + + it('should suppress the empty-allowlist notice in safe mode and bare mode', async () => { + // Neither mode honours settings.tools.core — safe mode forces the + // CLI-level coreTools to undefined and bare mode replaces it with + // the built-in bare toolset — so the notice advice could never + // re-enable tools there. + const settings: Settings = { + tools: { + core: [], + }, + }; + + for (const flag of ['--safe-mode', '--bare']) { + process.argv = ['node', 'script.js', flag, '-p', 'test']; + const argv = await parseArguments(); + + mockWriteStderrLine.mockClear(); + await loadCliConfig(settings, argv, undefined, []); + // Note: safe mode resolves coreTools to the built-in safe-tools + // list (not undefined), so only the notice suppression is + // asserted here. + expect(mockWriteStderrLine).not.toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + } + }); + + it('should still warn for a bare --core-tools flag over an empty settings allowlist', async () => { + // A bare flag is treated as ABSENT, so the empty settings list + // carries the session semantic and the notice must fire (#10065). + process.argv = ['node', 'script.js', '--core-tools', '-p', 'test']; + const argv = await parseArguments(); + const settings: Settings = { + tools: { + core: [], + }, + }; + + mockWriteStderrLine.mockClear(); + await loadCliConfig(settings, argv, undefined, []); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + }); + it('should exclude only shell tools in non-interactive mode with auto-edit approval mode', async () => { process.argv = [ 'node', diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index d68f30eece3..0578d011ac9 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -917,8 +917,18 @@ export async function parseArguments(): Promise { type: 'array', string: true, description: 'Core tool paths', + // Empty entries carry no tool name (e.g. a script expanding an + // unset variable — `qwen --core-tools "$TOOLS"` — hands yargs + // `[""]`); drop them so the flag collapses to the absent-flag + // path instead of a one-entry allowlist that matches nothing + // and silently disables every tool (#10065). coerce: (tools: string[]) => - tools.flatMap((tool) => tool.split(',').map((t) => t.trim())), + tools.flatMap((tool) => + tool + .split(',') + .map((t) => t.trim()) + .filter((t) => t !== ''), + ), }) .option('exclude-tools', { type: 'array', @@ -1777,13 +1787,17 @@ export async function loadCliConfig( // session (the empty-allowlist semantic of #10065). Give the user one // visible startup notice instead of failing every tool call silently; // the argv flag wins over settings when valued, so only the settings - // path can resolve to the empty allowlist here. + // path can resolve to the empty allowlist here. An all-empty-string + // list (`"core": [""]`, e.g. an unset-variable expansion written into + // settings) carries no tool name either and collapses to the same + // empty allowlist in `PermissionManager.initialize()`, so the notice + // covers it too (#10065). if ( !bareMode && !safeMode && !(argv.coreTools && argv.coreTools.length > 0) && Array.isArray(settings.tools?.core) && - settings.tools.core.length === 0 + settings.tools.core.every((t) => typeof t !== 'string' || t.trim() === '') ) { writeStderrLine( '⚠ tools.core is an empty allowlist: all tools are disabled for this session. Add tool names to tools.core or remove the key to re-enable tools (#10065).\n', @@ -2182,13 +2196,22 @@ export async function loadCliConfig( // tool": only an explicit `tools.core: []` in settings (or a valued // flag) should carry the empty-allowlist semantic, so a forgotten // flag value — or a script expanding an unset variable into the flag - // — cannot silently flip a session to tool-free (#10065). + // — cannot silently flip a session to tool-free (#10065). The + // settings leg is type-guarded because settings loading performs no + // validation: a hand-edited non-array `"core": ""` must normalize to + // ABSENT (as the pre-#10065 `||` chain did) instead of flowing raw + // into Config.coreTools, where `getCoreTools()?.some(...)` would + // throw during tool-registry construction; empty-string entries are + // dropped where the allowlist is built (PermissionManager.initialize, + // #10065). coreTools: bareMode || safeMode ? undefined : argv.coreTools?.length ? argv.coreTools - : settings.tools?.core, + : Array.isArray(settings.tools?.core) + ? settings.tools.core + : undefined, allowedTools: bareMode || safeMode ? argv.allowedTools || undefined diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 8f870e577a3..8cd276acc84 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -3821,6 +3821,11 @@ describe('CoreToolScheduler', () => { const message = completedCall.response.error?.message ?? ''; expect(message).toContain('tools.core'); expect(message).toContain('"edit"'); + // The gate is restart-scoped (coreToolsAllowList is snapshotted in + // initialize(), `tools.core` is requiresRestart), so the advice + // must carry the restart caveat like its sibling messages + // (#10065). + expect(message).toContain('restart'); expect(message).not.toContain('permissions.allow'); expect(message).not.toContain('permission was declined'); } diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 2564a5ff948..fa476c4e133 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -2490,7 +2490,7 @@ export class CoreToolScheduler { typeof pm.isCoreToolsAllowListEmpty === 'function' && pm.isCoreToolsAllowListEmpty() ) { - permissionErrorMessage = `"${reqInfo.name}" is disabled because the core tools allowlist (settings tools.core) is explicitly empty. Remove the setting or list the tool there to re-enable it.`; + permissionErrorMessage = `"${reqInfo.name}" is disabled because the core tools allowlist (settings tools.core) is explicitly empty. Remove the setting or list the tool there, and restart, to re-enable it.`; } else if ( // The legacy `coreTools` allowlist (`--core-tools` / settings // `tools.core`) keeps its hard-disable semantic: an unlisted diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 5f198e2bf64..bb33483e3bd 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -2581,6 +2581,31 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('mcp__server__tool')).toBe(false); }); + it('all-empty-string coreTools collapses to the empty allowlist, not a [""] allowlist', async () => { + // `"core": [""]` (e.g. an unset-variable expansion written into + // settings) carries no tool name; it must behave exactly like the + // explicit `[]` — `isCoreToolsAllowListEmpty()` true and every tool + // rejected — instead of a size-1 allowlist matching nothing that + // silently disables tools while neither diagnostic fires (#10065). + pm = new PermissionManager(makeConfig({ coreTools: [''] })); + pm.initialize(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(true); + expect(await pm.isToolEnabled('read_file')).toBe(false); + expect(await pm.isToolEnabled('agent')).toBe(false); + expect(await pm.isToolEnabled('structured_output')).toBe(false); + + // Whitespace-only entries carry no name either. + pm = new PermissionManager(makeConfig({ coreTools: [' '] })); + pm.initialize(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(true); + + // A list still naming a tool keeps its non-empty semantic. + pm = new PermissionManager(makeConfig({ coreTools: ['', 'read_file'] })); + pm.initialize(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(false); + expect(await pm.isToolEnabled('read_file')).toBe(true); + }); + it('undefined coreTools keeps tools enabled', async () => { pm = new PermissionManager(makeConfig()); pm.initialize(); diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index 0f6705a12b2..04bd2bb73cf 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -256,10 +256,19 @@ export class PermissionManager { // Only an array activates the allowlist: `undefined`, `null`, or any // non-array value (e.g. `"tools": { "core": null }`, the common JSON // idiom for clearing this deprecated key) means no restriction. + // Entries that carry no tool name — the empty string, whitespace-only + // strings, non-string garbage — are dropped: an all-empty-string list + // (`[""]`, e.g. an unset-variable expansion written into settings) + // collapses to the explicit EMPTY allowlist with both #10065 + // diagnostics instead of a size-1 allowlist that matches nothing and + // silently disables every tool while `isCoreToolsAllowListEmpty()` + // stays false (#10065). const rawCoreTools = this.config.getCoreTools?.(); if (Array.isArray(rawCoreTools)) { this.coreToolsAllowList = new Set( - rawCoreTools.map((t) => parseRule(t).toolName), + rawCoreTools + .filter((t) => typeof t === 'string' && t.trim() !== '') + .map((t) => parseRule(t).toolName), ); } @@ -924,7 +933,9 @@ export class PermissionManager { * in settings). A bare/valueless `--core-tools` never produces this * state — `loadCliConfig` treats an argv-sourced empty list as absent — * and neither do `null`/non-array values, which `initialize()` treats - * as no restriction. When true, `isToolEnabled()` rejects EVERY tool — + * as no restriction; an all-empty-string list (`[""]`) does, because + * name-less entries are dropped when the allowlist is built (#10065). + * When true, `isToolEnabled()` rejects EVERY tool — * core and non-core alike — so user-facing remediation advice must name * this knob instead of promising re-enablement through other gates * (e.g. adding a `permissions.allow` rule can never satisfy an empty From 1d830f68d4192189fe1e2f01b9516670a3af0c11 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 04:06:59 +0800 Subject: [PATCH 08/29] fix(cli): drop non-string tools.core entries at the settings boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (#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 #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 --- packages/cli/src/config/config.test.ts | 38 ++++++++++++++++++++++++++ packages/cli/src/config/config.ts | 37 +++++++++++++++++-------- 2 files changed, 64 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index a9c81e3574a..dd568f2fd8f 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3207,6 +3207,44 @@ describe('Approval mode tool exclusion logic', () => { expect(config.getCoreTools()).toBeUndefined(); }); + it('should drop non-string tools.core entries instead of crashing startup', async () => { + // Settings loading performs no element validation (the schema declares + // only `type: 'array'`), so a hand-edited `"core": [42]` — or a mixed + // `["read_file", 42]` — reaches loadCliConfig raw. Without the + // boundary filter, the non-interactive exclusion path (`isToolEnabled` + // → `filterList` → `entry.trim()`) throws a TypeError inside + // loadCliConfig, and `isLsToolEnabled()` → `parseRule(42)` would + // crash tool-registry construction. The entries must be dropped, and + // an all-non-string list collapses to the explicit empty allowlist + // with the #10065 startup notice (#10080). + process.argv = ['node', 'script.js', '-p', 'test']; + const argv = await parseArguments(); + + mockWriteStderrLine.mockClear(); + const config = await loadCliConfig( + { tools: { core: [42 as unknown as string] } }, + argv, + undefined, + [], + ); + expect(config.getCoreTools()).toEqual([]); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + + mockWriteStderrLine.mockClear(); + const mixed = await loadCliConfig( + { tools: { core: ['read_file', 42 as unknown as string] } }, + argv, + undefined, + [], + ); + expect(mixed.getCoreTools()).toEqual(['read_file']); + expect(mockWriteStderrLine).not.toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + }); + it('should suppress the empty-allowlist notice when a valued --core-tools overrides the empty settings list', async () => { // A valued argv flag wins over settings (its tools ARE registered), // so the "all tools are disabled" notice would be factually wrong diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index ef9a73d1108..2d25ea6a124 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -1803,9 +1803,25 @@ export async function loadCliConfig( '⚠ tools.core is an empty allowlist: all tools are disabled for this session. Add tool names to tools.core or remove the key to re-enable tools (#10065).\n', ); } + // Settings loading performs no element validation (the schema declares only + // `type: 'array'`), so a hand-edited `"core": [42]` — or a mixed + // `["read_file", 42]` — would otherwise reach Config.coreTools and + // resolvedCoreTools raw, where `parseRule(42)`/`entry.trim()` throw a + // TypeError during tool-registry construction instead of the entry being + // dropped (#10080). Only an ARRAY activates the allowlist (a non-array + // normalizes to ABSENT/undefined, as the pre-#10065 `||` chain did); drop + // non-string entries at the boundary. Empty/whitespace strings are KEPT so + // they collapse to the explicit empty allowlist (with both #10065 + // diagnostics) in PermissionManager.initialize() rather than being treated + // as ABSENT. + const settingsCoreTools: string[] | undefined = Array.isArray( + settings.tools?.core, + ) + ? settings.tools.core.filter((t) => typeof t === 'string') + : undefined; const resolvedCoreTools: string[] = [ ...(bareMode || safeMode ? [] : (argv.coreTools ?? [])), - ...(bareMode || safeMode ? [] : (settings.tools?.core ?? [])), + ...(bareMode || safeMode ? [] : (settingsCoreTools ?? [])), ]; const mergedAllow: string[] = [ ...(bareMode || safeMode ? [] : (settings.permissions?.allow ?? [])), @@ -2237,21 +2253,20 @@ export async function loadCliConfig( // flag) should carry the empty-allowlist semantic, so a forgotten // flag value — or a script expanding an unset variable into the flag // — cannot silently flip a session to tool-free (#10065). The - // settings leg is type-guarded because settings loading performs no - // validation: a hand-edited non-array `"core": ""` must normalize to - // ABSENT (as the pre-#10065 `||` chain did) instead of flowing raw - // into Config.coreTools, where `getCoreTools()?.some(...)` would - // throw during tool-registry construction; empty-string entries are - // dropped where the allowlist is built (PermissionManager.initialize, - // #10065). + // settings leg reads `settingsCoreTools` because settings loading + // performs no element validation: a hand-edited non-array `"core": ""` + // normalizes to ABSENT (as the pre-#10065 `||` chain did), and non-string + // array entries (`"core": [42]`) are dropped — either shape flowing raw + // into Config.coreTools would make `getCoreTools()?.some(...)` throw + // during tool-registry construction (#10080). Empty-string entries are + // kept there and dropped where the allowlist is built + // (PermissionManager.initialize, #10065). coreTools: bareMode || safeMode ? undefined : argv.coreTools?.length ? argv.coreTools - : Array.isArray(settings.tools?.core) - ? settings.tools.core - : undefined, + : settingsCoreTools, allowedTools: bareMode || safeMode ? argv.allowedTools || undefined From dbed630a76c0db65c157c1745a53361fd5b92508 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 04:07:22 +0800 Subject: [PATCH 09/29] fix(core): omit empty tools from OpenAI-compatible wire requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (#10065/#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 --- .../openaiContentGenerator/pipeline.test.ts | 45 ++++++++++++++++++ .../core/openaiContentGenerator/pipeline.ts | 46 +++++++++++-------- 2 files changed, 73 insertions(+), 18 deletions(-) diff --git a/packages/core/src/core/openaiContentGenerator/pipeline.test.ts b/packages/core/src/core/openaiContentGenerator/pipeline.test.ts index a46a564fb77..9ab61cab6f1 100644 --- a/packages/core/src/core/openaiContentGenerator/pipeline.test.ts +++ b/packages/core/src/core/openaiContentGenerator/pipeline.test.ts @@ -643,6 +643,51 @@ describe('ContentGenerationPipeline', () => { expect(apiCall.tools).toBeUndefined(); }); + it('should skip the wrapped-empty tools shape produced by client.setTools() on an empty registry', async () => { + // Arrange — the #10065 empty allowlist disables every tool, so + // client.setTools() wraps ZERO declarations as + // `[{ functionDeclarations: [] }]`. The wrapper's `.length` is 1, so + // a length-only guard would still ship `tools: []` on the wire, + // which some providers reject (#10080). The converted declarations + // are empty, so the field must be omitted entirely. + const request: GenerateContentParameters = { + model: 'test-model', + contents: [{ parts: [{ text: 'Hello' }], role: 'user' }], + config: { tools: [{ functionDeclarations: [] }] }, + }; + const userPromptId = 'test-prompt-id'; + + const mockMessages = [ + { role: 'user', content: 'Hello' }, + ] as OpenAI.Chat.ChatCompletionMessageParam[]; + const mockOpenAIResponse = { + id: 'response-id', + choices: [{ message: { content: 'Response' }, finish_reason: 'stop' }], + } as OpenAI.Chat.ChatCompletion; + const mockLlmResponse = new GenerateContentResponse(); + + (mockConverter.convertLlmRequestToOpenAI as Mock).mockReturnValue( + mockMessages, + ); + (mockConverter.convertLlmToolsToOpenAI as Mock).mockResolvedValue([]); + (mockConverter.convertOpenAIResponseToLlm as Mock).mockReturnValue( + mockLlmResponse, + ); + (mockClient.chat.completions.create as Mock).mockResolvedValue( + mockOpenAIResponse, + ); + + // Act + await pipeline.execute(request, userPromptId); + + // Assert — conversion ran (wrapper is non-empty) but the empty + // result must not reach the wire. + expect(mockConverter.convertLlmToolsToOpenAI).toHaveBeenCalled(); + const apiCall = (mockClient.chat.completions.create as Mock).mock + .calls[0][0]; + expect(apiCall.tools).toBeUndefined(); + }); + it('should override enable_thinking when thinkingConfig disables it', async () => { // Arrange — provider injects enable_thinking: true via extra_body // (e.g. user configured `enableThinking: true` via setup wizard, diff --git a/packages/core/src/core/openaiContentGenerator/pipeline.ts b/packages/core/src/core/openaiContentGenerator/pipeline.ts index 63ab4db3c6e..24b2ab719e6 100644 --- a/packages/core/src/core/openaiContentGenerator/pipeline.ts +++ b/packages/core/src/core/openaiContentGenerator/pipeline.ts @@ -1079,25 +1079,35 @@ export class ContentGenerationPipeline { } // Add tools if present and non-empty. - // Some providers reject tools: [] (empty array), so skip when there are no tools. + // Some providers reject tools: [] (empty array), so skip when there are + // no tools. Guard on the CONVERTED declarations, not the wrapper length: + // client.setTools() always wraps declarations as `[{ functionDeclarations }]` + // even when the registry is empty (the #10065 empty allowlist disables + // every tool), so the wrapper's own `.length` is 1 while it carries zero + // declarations — without this guard an empty allowlist would still ship + // `tools: []` on the wire (#10080). if (request.config?.tools && request.config.tools.length > 0) { - baseRequest.tools = await OpenAIContentConverter.convertLlmToolsToOpenAI( - request.config.tools, - this.contentGeneratorConfig.schemaCompliance ?? 'auto', - ); - - // Map Gemini-style toolConfig.functionCallingConfig.mode to OpenAI's - // tool_choice so structured side queries (e.g. the AUTO-mode - // classifier's respond_in_schema) can force the model to emit a tool - // call instead of free-texting. Without this, thinking-heavy models - // may consume the tiny output budget on reasoning and skip the tool. - const fcMode = request.config?.toolConfig?.functionCallingConfig?.mode; - if (fcMode === 'ANY') { - (baseRequest as unknown as Record)['tool_choice'] = - 'required'; - } else if (fcMode === 'NONE') { - (baseRequest as unknown as Record)['tool_choice'] = - 'none'; + const convertedTools = + await OpenAIContentConverter.convertLlmToolsToOpenAI( + request.config.tools, + this.contentGeneratorConfig.schemaCompliance ?? 'auto', + ); + if (convertedTools.length > 0) { + baseRequest.tools = convertedTools; + + // Map Gemini-style toolConfig.functionCallingConfig.mode to OpenAI's + // tool_choice so structured side queries (e.g. the AUTO-mode + // classifier's respond_in_schema) can force the model to emit a tool + // call instead of free-texting. Without this, thinking-heavy models + // may consume the tiny output budget on reasoning and skip the tool. + const fcMode = request.config?.toolConfig?.functionCallingConfig?.mode; + if (fcMode === 'ANY') { + (baseRequest as unknown as Record)['tool_choice'] = + 'required'; + } else if (fcMode === 'NONE') { + (baseRequest as unknown as Record)['tool_choice'] = + 'none'; + } } } From dca5b970f65a90f1fec681e0c50980a3490ca26a Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 04:07:46 +0800 Subject: [PATCH 10/29] test(core): pin non-string tools.core entry handling in PermissionManager 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 (#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 --- .../permissions/permission-manager.test.ts | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 9c96446b8ea..7d251b742e5 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -2605,6 +2605,33 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('read_file')).toBe(true); }); + it('non-string coreTools entries are dropped without throwing', async () => { + // Settings loading performs no element validation (the schema + // declares only `type: 'array'`), so a hand-edited `"core": [42]` + // — or a mixed `["read_file", 42]` — reaches `initialize()` raw. + // The `typeof t === 'string'` half of the filter must drop the + // garbage instead of letting `parseRule(42)` throw while the + // allowlist is built (#10080). + pm = new PermissionManager( + makeConfig({ coreTools: [42 as unknown as string] }), + ); + expect(() => pm.initialize()).not.toThrow(); + // An all-non-string list names nothing, so it collapses to the + // explicit empty allowlist exactly like `[]` / `[""]` — both + // #10065 diagnostics stay reachable. + expect(pm.isCoreToolsAllowListEmpty()).toBe(true); + expect(await pm.isToolEnabled('read_file')).toBe(false); + + // A mixed list keeps its named tool and drops the garbage entry. + pm = new PermissionManager( + makeConfig({ coreTools: ['read_file', 42 as unknown as string] }), + ); + expect(() => pm.initialize()).not.toThrow(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(false); + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); + }); + it('undefined coreTools keeps tools enabled', async () => { pm = new PermissionManager(makeConfig()); pm.initialize(); From 7b7435017e4308c7177320954eed5c5024cf8f28 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 04:08:04 +0800 Subject: [PATCH 11/29] docs(cli): correct safe-mode coreTools comment in config test 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 (#10080). Co-authored-by: Qwen-Coder --- packages/cli/src/config/config.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index dd568f2fd8f..8b921b95b12 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3289,9 +3289,9 @@ describe('Approval mode tool exclusion logic', () => { mockWriteStderrLine.mockClear(); await loadCliConfig(settings, argv, undefined, []); - // Note: safe mode resolves coreTools to the built-in safe-tools - // list (not undefined), so only the notice suppression is - // asserted here. + // Note: safe mode resolves coreTools to `undefined` via the ternary's + // `bareMode || safeMode` arm (there is no built-in safe-tools list), + // so only the notice suppression is asserted here. expect(mockWriteStderrLine).not.toHaveBeenCalledWith( expect.stringContaining('tools.core is an empty allowlist'), ); From e073c9d48cc76f409b69cd96a25f043501539b73 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 06:35:45 +0800 Subject: [PATCH 12/29] fix(core): drop tools.core entries without a usable parsed name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (#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 --- packages/core/src/permissions/index.ts | 5 ++- .../src/permissions/permission-manager.ts | 39 +++++++++++++++---- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/packages/core/src/permissions/index.ts b/packages/core/src/permissions/index.ts index a13c72ed9dc..bddbe24d0c5 100644 --- a/packages/core/src/permissions/index.ts +++ b/packages/core/src/permissions/index.ts @@ -6,7 +6,10 @@ export * from './types.js'; export * from './rule-parser.js'; -export { PermissionManager } from './permission-manager.js'; +export { + PermissionManager, + isNamelessCoreToolsEntry, +} from './permission-manager.js'; export type { PermissionManagerConfig } from './permission-manager.js'; export { extractShellOperations } from './shell-semantics.js'; export type { ShellOperation } from './shell-semantics.js'; diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index 6291d1a1de8..0f7d99a2561 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -123,6 +123,27 @@ export interface PermissionManagerConfig { getEagerTools?(): readonly string[] | undefined; } +/** + * Classifies a raw `tools.core` entry as name-less: it carries no usable + * tool name. Non-string entries, empty/whitespace-only strings, malformed + * rules (unbalanced parens parse to `invalid`), and entries whose tool + * part parses to the empty string (`"()"`, `"(ls -l)"`) are all name-less. + * + * This is the single shared classification behind BOTH faces of the + * empty-allowlist collapse: the gate in `PermissionManager.initialize()` + * below and the CLI startup notice in `packages/cli/src/config/config.ts`. + * They must agree on what counts as name-less, or the notice can stay + * silent for a list the gate collapses to the empty allowlist — exactly + * the silent tool-free session #10065 set out to diagnose (#10080). + */ +export function isNamelessCoreToolsEntry(entry: unknown): boolean { + if (typeof entry !== 'string' || entry.trim() === '') { + return true; + } + const rule = parseRule(entry); + return rule.invalid || rule.toolName === ''; +} + /** * Manages tool and command permissions by evaluating a set of * prioritised rules against allow / ask / deny lists. @@ -224,17 +245,21 @@ export class PermissionManager { // non-array value (e.g. `"tools": { "core": null }`, the common JSON // idiom for clearing this deprecated key) means no restriction. // Entries that carry no tool name — the empty string, whitespace-only - // strings, non-string garbage — are dropped: an all-empty-string list - // (`[""]`, e.g. an unset-variable expansion written into settings) - // collapses to the explicit EMPTY allowlist with both #10065 - // diagnostics instead of a size-1 allowlist that matches nothing and - // silently disables every tool while `isCoreToolsAllowListEmpty()` - // stays false (#10065). + // strings, non-string garbage, malformed rules, and entries whose tool + // part parses to the empty string (`"()"`, `"(ls -l)"`, e.g. an + // unset-variable or template expansion written into settings) — are + // dropped via the shared `isNamelessCoreToolsEntry` classification, so + // a list made up only of such entries (`[""]`, `["()"]`) collapses to + // the explicit EMPTY allowlist with both #10065 diagnostics instead of + // a size-1 allowlist that matches nothing and silently disables every + // tool while `isCoreToolsAllowListEmpty()` stays false (#10065). The + // CLI startup notice uses the same helper, so notice and gate cannot + // drift apart (#10080). const rawCoreTools = this.config.getCoreTools?.(); if (Array.isArray(rawCoreTools)) { this.coreToolsAllowList = new Set( rawCoreTools - .filter((t) => typeof t === 'string' && t.trim() !== '') + .filter((t) => !isNamelessCoreToolsEntry(t)) .map((t) => parseRule(t).toolName), ); } From 63800322a627a774973c87c1947619656d98531d Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 06:35:59 +0800 Subject: [PATCH 13/29] fix(cli): share core's name-less tools.core classification with the notice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #10065 set out to prevent (#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 --- packages/cli/src/config/config.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 2d25ea6a124..4127f49d900 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -34,6 +34,7 @@ import { createDebugLogger, NativeLspService, isBareMode, + isNamelessCoreToolsEntry, isTruthy, isSafeModeEnv, isToolEnabled, @@ -1787,17 +1788,19 @@ export async function loadCliConfig( // session (the empty-allowlist semantic of #10065). Give the user one // visible startup notice instead of failing every tool call silently; // the argv flag wins over settings when valued, so only the settings - // path can resolve to the empty allowlist here. An all-empty-string - // list (`"core": [""]`, e.g. an unset-variable expansion written into - // settings) carries no tool name either and collapses to the same - // empty allowlist in `PermissionManager.initialize()`, so the notice - // covers it too (#10065). + // path can resolve to the empty allowlist here. Entries carrying no + // tool name — empty/whitespace strings, non-string garbage, malformed + // rules, and mangled specifiers like `"()"` (e.g. an unset-variable or + // template expansion written into settings) — collapse to the same + // empty allowlist in `PermissionManager.initialize()`; the gate and + // this notice share the `isNamelessCoreToolsEntry` classification from + // core so they cannot drift apart (#10065, #10080). if ( !bareMode && !safeMode && !(argv.coreTools && argv.coreTools.length > 0) && Array.isArray(settings.tools?.core) && - settings.tools.core.every((t) => typeof t !== 'string' || t.trim() === '') + settings.tools.core.every((t) => isNamelessCoreToolsEntry(t)) ) { writeStderrLine( '⚠ tools.core is an empty allowlist: all tools are disabled for this session. Add tool names to tools.core or remove the key to re-enable tools (#10065).\n', @@ -1888,7 +1891,9 @@ export async function loadCliConfig( // // An explicitly empty array must survive as an empty array, not collapse // into "unset": `[]` is an active allowlist naming nothing (defer - // everything). `tools.core` differs: its empty list is treated as unset. + // everything). `tools.core` differs only in effect: its explicitly + // empty list stays active and disables every tool instead of + // deferring (#10065). // `normalizeDisabledToolList` maps undefined to `[]`, // so the Array.isArray guard has to come first — without it, absent and // explicitly-empty would reach core as the same value, which is exactly From 332d2e5885a12f0654b1b136acb17f8820b915cb Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 06:36:15 +0800 Subject: [PATCH 14/29] test: pin name-less tools.core collapse on gate and startup notice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/cli/src/config/config.test.ts | 41 +++++++++++++- .../permissions/permission-manager.test.ts | 55 ++++++++++++++++++- 2 files changed, 92 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 8b921b95b12..d7ffd09fc75 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3189,6 +3189,42 @@ describe('Approval mode tool exclusion logic', () => { ); }); + it('should warn for name-less tools.core entries exactly like for []', async () => { + // `"core": ["()"]` and similar mangled specifiers are non-empty + // strings whose parsed tool name is empty; PermissionManager + // collapses them to the explicit empty allowlist, so the startup + // notice must fire for them too — gate and notice share the + // isNamelessCoreToolsEntry classification (#10065, #10080). A list + // still naming a tool must not warn. + process.argv = ['node', 'script.js', '-p', 'test']; + const argv = await parseArguments(); + for (const core of [['()'], ['(ls -l)']]) { + const namelessSettings: Settings = { + tools: { + core, + }, + }; + + mockWriteStderrLine.mockClear(); + await loadCliConfig(namelessSettings, argv, undefined, []); + expect(mockWriteStderrLine).toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + } + + const valuedSettings: Settings = { + tools: { + core: ['()', 'read_file'], + }, + }; + + mockWriteStderrLine.mockClear(); + await loadCliConfig(valuedSettings, argv, undefined, []); + expect(mockWriteStderrLine).not.toHaveBeenCalledWith( + expect.stringContaining('tools.core is an empty allowlist'), + ); + }); + it('should treat a non-array tools.core value as absent', async () => { // Settings loading performs no type validation, so a hand-edited // `"core": ""` reaches the coreTools ternary raw; it must normalize @@ -4412,8 +4448,9 @@ describe('loadCliConfig tools.eager wiring (#9827, #10075)', () => { }; const config = await loadCliConfig(settings, argv, undefined, []); - // `[]` is an active allowlist naming nothing (defer everything), unlike - // `tools.core`, where an empty list is treated as unset. + // `[]` is an active allowlist naming nothing (defer everything); `tools.core` + // differs only in effect: its explicitly empty list stays active and + // disables every tool instead of deferring (#10065). expect(config.getEagerTools()).toEqual([]); }); diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 7d251b742e5..86c315a5538 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -26,7 +26,10 @@ import { buildHumanReadableRuleLabel, TOOL_NAME_ALIASES, } from './rule-parser.js'; -import { PermissionManager } from './permission-manager.js'; +import { + PermissionManager, + isNamelessCoreToolsEntry, +} from './permission-manager.js'; import type { PermissionManagerConfig } from './permission-manager.js'; import { normalizeToolNameForProvider } from '../utils/tool-name-utils.js'; import { ToolNames, ToolDisplayNames } from '../tools/tool-names.js'; @@ -2605,6 +2608,33 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('read_file')).toBe(true); }); + it('coreTools entries without a usable parsed name collapse to the empty allowlist', async () => { + // 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. They must be + // dropped exactly like empty strings — collapsing such a list to the + // explicit empty allowlist with both #10065 diagnostics — instead of + // forming a size-1 `{''}` allowlist that matches nothing while + // `isCoreToolsAllowListEmpty()` stays false and every core tool is + // silently disabled (#10080). + for (const coreTools of [['()'], ['(ls -l)'], ['Bash(ls'], ['', '()']]) { + pm = new PermissionManager(makeConfig({ coreTools })); + pm.initialize(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(true); + expect(await pm.isToolEnabled('read_file')).toBe(false); + expect(await pm.isToolEnabled('mcp__server__tool')).toBe(false); + } + + // A list still naming a real tool keeps its non-empty semantic even + // when mixed with name-less entries. + pm = new PermissionManager( + makeConfig({ coreTools: ['()', 'read_file'] }), + ); + pm.initialize(); + expect(pm.isCoreToolsAllowListEmpty()).toBe(false); + expect(await pm.isToolEnabled('read_file')).toBe(true); + }); + it('non-string coreTools entries are dropped without throwing', async () => { // Settings loading performs no element validation (the schema // declares only `type: 'array'`), so a hand-edited `"core": [42]` @@ -2922,7 +2952,8 @@ describe('PermissionManager', () => { it('an explicitly empty list is active and defers everything', async () => { // `[]` is an active allowlist that names nothing; `tools.core` differs - // because its empty list is treated as unset. + // only in effect: its explicitly empty list stays active and disables + // every tool instead of deferring (#10065). // This is the gentler answer for constrained-decoding backends: the // eager request carries almost no tool schemas, but every tool is // still registered and reachable via ToolSearch. @@ -4385,3 +4416,23 @@ describe('matchesRule — param matcher type guards', () => { ).toBe(true); }); }); +describe('isNamelessCoreToolsEntry', () => { + // The helper is the single shared classification behind both the gate in + // PermissionManager.initialize() and the CLI startup notice; pin it + // directly so either call site rewinding to a private inline predicate + // cannot hide behind the other (#10080). + it('classifies non-strings, blank strings, and unparseable entries as name-less', () => { + expect(isNamelessCoreToolsEntry(123)).toBe(true); + expect(isNamelessCoreToolsEntry('')).toBe(true); + expect(isNamelessCoreToolsEntry(' ')).toBe(true); + expect(isNamelessCoreToolsEntry('()')).toBe(true); + expect(isNamelessCoreToolsEntry('(ls -l)')).toBe(true); + expect(isNamelessCoreToolsEntry('Bash(ls')).toBe(true); + }); + + it('classifies entries naming a tool as usable', () => { + expect(isNamelessCoreToolsEntry('read_file')).toBe(false); + expect(isNamelessCoreToolsEntry('Bash(npm test)')).toBe(false); + expect(isNamelessCoreToolsEntry(' mcp__server__tool ')).toBe(false); + }); +}); From fbf098a0453ebffaf641fefddef67d95c11d75fe Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 06:36:31 +0800 Subject: [PATCH 15/29] docs: align tools.core empty-list descriptions with the #10065 semantic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-#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 --- docs/users/configuration/settings.md | 12 ++++++------ packages/cli/src/config/settingsSchema.ts | 2 +- packages/core/src/config/config.ts | 4 +++- .../schemas/settings.schema.json | 2 +- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/docs/users/configuration/settings.md b/docs/users/configuration/settings.md index b18ab489ce6..1c7133e5461 100644 --- a/docs/users/configuration/settings.md +++ b/docs/users/configuration/settings.md @@ -364,7 +364,7 @@ If you are experiencing performance issues with file searching (e.g., with `@` c | `tools.shell.enableInteractiveShell` | boolean | Use `node-pty` for an interactive shell experience. Fallback to `child_process` still applies. | `true` | | | `tools.shell.defaultTimeoutMs` | number | Default timeout, in milliseconds, for foreground shell commands started by the agent. A per-call timeout on the shell tool overrides this. When unset, foreground commands time out after 120000 ms (2 minutes). Set to 0 to disable the timeout. | `undefined` | | | `tools.shell.heartbeatIntervalMs` | number | Interval, in milliseconds, between liveness heartbeats emitted while a foreground shell command produces no output. Heartbeats are forwarded to ACP clients and stream-json consumers so they can tell a silent command from a dead session. When unset, heartbeats fire every 10000 ms (10 seconds). Set to 0 to disable heartbeats. | `undefined` | | -| `tools.core` | array of strings | **Deprecated.** Will be removed in next version. A non-empty list restricts the core tool set (file, shell, search and related built-ins) to an allowlist: core tools not in the list are disabled (fail-closed). Tools outside that set — dynamically discovered tools (MCP, skill) and synthetic/system built-ins such as `agent`, `list_agents`, plan-mode lifecycle tools, goal tools, `task_stop`, `send_message` and `tool_search` — bypass the allowlist by design; use `permissions.deny` to remove a tool outright. An empty list (`[]`) is treated as unset and disables nothing. `permissions.allow` cannot reproduce this restriction — it is pure auto-approval (#10075). Use `tools.eager` to restrict which eager-by-default tool schemas are sent initially (unlisted tools are deferred, not disabled — they stay loadable via `tool_search`), and `permissions.deny` to block tools outright. | `undefined` | | +| `tools.core` | array of strings | **Deprecated.** Will be removed in next version. A non-empty list restricts the core tool set (file, shell, search and related built-ins) to an allowlist: core tools not in the list are disabled (fail-closed). Tools outside that set — dynamically discovered tools (MCP, skill) and synthetic/system built-ins such as `agent`, `list_agents`, plan-mode lifecycle tools, goal tools, `task_stop`, `send_message` and `tool_search` — bypass the allowlist by design; use `permissions.deny` to remove a tool outright. An explicitly empty list (`[]`, or one whose entries carry no tool name) is an ACTIVE allowlist that disables every registered tool — built-ins, MCP, and synthetic tools alike (#10065); omit the key or set it to `null` for no restriction. `permissions.allow` cannot reproduce this restriction — it is pure auto-approval (#10075). Use `tools.eager` to restrict which eager-by-default tool schemas are sent initially (unlisted tools are deferred, not disabled — they stay loadable via `tool_search`), and `permissions.deny` to block tools outright. | `undefined` | | | `tools.exclude` | array of strings | **Deprecated.** Use `permissions.deny` instead. Tool names to exclude from discovery. Not automatically migrated; the legacy setting remains honoured at startup. | `undefined` | | | `tools.disabled` | array of strings | Tool names hidden from the registry entirely. Unlike `permissions.deny` (which blocks calls at runtime), disabled tools are never registered, so they do not appear in `/tools` and cannot be discovered or called by the model. For example, `["enter_plan_mode"]` prevents the model from switching into plan mode on its own. Merged as a union across scopes. | `undefined` | | | `tools.visible` | array of strings | Deferred tool names made visible at startup without requiring `tool_search`. Listed tools appear alongside core tools in the initial session. Merged as a union across scopes. | `undefined` | | @@ -483,11 +483,11 @@ Permission rules for `Read`, `Edit`, and `WebFetch` are also enforced when the a **Migrating from legacy settings:** -| Legacy setting | Equivalent `permissions` rule | Notes | -| --------------- | ------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `tools.allowed` | `permissions.allow` | Not automatically migrated; still honoured at startup | -| `tools.exclude` | `permissions.deny` | Not automatically migrated; still honoured at startup | -| `tools.core` | `tools.eager` (+ `permissions.deny`) | Not auto-migrated to `permissions.allow`, which is pure auto-approval and cannot reproduce the allowlist restriction (#10075). `tools.eager` defers unlisted eager-by-default tools (they stay loadable via `tool_search`); `permissions.deny` removes tools entirely. Neither preserves a non-empty `tools.core` allowlist's fail-closed guarantee over the core tool set: a built-in added in a future release registers until explicitly denied, so a deny list must be re-audited per release. An empty `tools.core` list is treated as unset and disables nothing. | +| Legacy setting | Equivalent `permissions` rule | Notes | +| --------------- | ------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `tools.allowed` | `permissions.allow` | Not automatically migrated; still honoured at startup | +| `tools.exclude` | `permissions.deny` | Not automatically migrated; still honoured at startup | +| `tools.core` | `tools.eager` (+ `permissions.deny`) | Not auto-migrated to `permissions.allow`, which is pure auto-approval and cannot reproduce the allowlist restriction (#10075). `tools.eager` defers unlisted eager-by-default tools (they stay loadable via `tool_search`); `permissions.deny` removes tools entirely. Neither preserves a non-empty `tools.core` allowlist's fail-closed guarantee over the core tool set: a built-in added in a future release registers until explicitly denied, so a deny list must be re-audited per release. An explicitly empty `tools.core` list (`[]`, or one whose entries carry no tool name) is an active allowlist that disables every tool (#10065); omit the key for no restriction. | **Example configuration:** diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 34d6822df18..df51bf3bfd5 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -2748,7 +2748,7 @@ const SETTINGS_SCHEMA = { requiresRestart: true, default: undefined as string[] | undefined, description: - 'Deprecated. permissions.allow cannot reproduce this registration restriction because it only auto-approves calls. Use tools.eager to defer unlisted eager-by-default tools or permissions.deny to remove tools. An empty list is treated as unset and disables nothing.', + 'Deprecated. permissions.allow cannot reproduce this registration restriction because it only auto-approves calls. Use tools.eager to defer unlisted eager-by-default tools or permissions.deny to remove tools. An explicitly empty list ([]) is an active allowlist that disables every tool; omit the key or set it to null for no restriction (#10065).', showInDialog: false, }, allowed: { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index c14fecf8533..812ce38626d 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2584,7 +2584,9 @@ export class Config { ); // An explicitly empty array is preserved as an ACTIVE-but-empty // allowlist (defer everything); only `undefined` means "no - // restriction". `tools.core` differs: its empty list is treated as unset. + // restriction". `tools.core` differs only in effect: its explicitly + // empty list stays active and disables every tool instead of + // deferring (#10065). this.eagerTools = params.eagerTools === undefined ? undefined diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index 963bf8754ef..7c76add97e9 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -1297,7 +1297,7 @@ } }, "core": { - "description": "Deprecated. permissions.allow cannot reproduce this registration restriction because it only auto-approves calls. Use tools.eager to defer unlisted eager-by-default tools or permissions.deny to remove tools. An empty list is treated as unset and disables nothing.", + "description": "Deprecated. permissions.allow cannot reproduce this registration restriction because it only auto-approves calls. Use tools.eager to defer unlisted eager-by-default tools or permissions.deny to remove tools. An explicitly empty list ([]) is an active allowlist that disables every tool; omit the key or set it to null for no restriction (#10065).", "type": "array", "items": { "type": "string" From 70d0ac0199c95c1efe9da5667003c683c3eb7f3b Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Sat, 29 Aug 2026 06:36:46 +0800 Subject: [PATCH 16/29] fix(core): refresh empty-gate ordering comment, drop dead test mocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The empty-allowlist branch comment justified its ordering by referencing two branches that no longer exist: the #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 --- .../core/src/core/coreToolScheduler.test.ts | 4 ---- packages/core/src/core/coreToolScheduler.ts | 18 +++++++++--------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 2664abb2fde..e1c673ab843 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -3724,8 +3724,6 @@ describe('CoreToolScheduler', () => { isToolEnabled: vi.fn().mockResolvedValue(false), findMatchingDenyRule: vi.fn().mockReturnValue(undefined), isCoreToolsAllowListEmpty: vi.fn().mockReturnValue(true), - isPermissionsAllowListActive: vi.fn().mockReturnValue(true), - isCoveredByAllowOrAskRule: vi.fn().mockReturnValue(false), }; const { scheduler, onAllToolCallsComplete } = createSchedulerForLegacyToolTests({ @@ -3785,8 +3783,6 @@ describe('CoreToolScheduler', () => { isToolEnabled: vi.fn().mockResolvedValue(false), findMatchingDenyRule: vi.fn().mockReturnValue(undefined), isCoreToolsAllowListEmpty: vi.fn().mockReturnValue(true), - isPermissionsAllowListActive: vi.fn().mockReturnValue(true), - isCoveredByAllowOrAskRule: vi.fn().mockReturnValue(true), }; const { scheduler, onAllToolCallsComplete } = createSchedulerForLegacyToolTests({ diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 13da563c9a1..054776b5f47 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -2476,18 +2476,18 @@ export class CoreToolScheduler { permissionErrorMessage = `Qwen Code requires permission to use "${reqInfo.name}", but that permission was declined. Matching deny rule: "${matchingRule}".`; } else if ( // An explicitly empty coreTools allowlist (`tools.core: []`) - // rejects EVERY tool, so the generic "permission was - // declined" text (nothing was ever asked or declined) and - // the `permissions.allow` remediation below are both wrong: - // adding an allow rule can never satisfy the empty gate. + // rejects EVERY tool, so both branches that follow are wrong + // here: the coreTools-miss branch advises adding the tool to + // the core tools list, but for a deliberately tool-free + // session the remedy is removing the empty setting — and the + // generic "permission was declined" fallback implies + // something was asked and declined, which never happened. // Name the responsible knob instead (#10065). The optional // call keeps scoped PermissionManager shims from throwing // until they grow the method; this branch sits ahead of the - // coreTools-miss and allowlist-advice branches because the - // empty gate rejects listed-or-not, covered-or-not tools - // alike (`isToolDisabledByCoreToolsAllowList` is also true - // under `[]`, but "add it to the core tools list" is the - // wrong remedy for a deliberately tool-free session). + // coreTools-miss branch because the empty gate rejects + // listed-or-not tools alike (`isToolDisabledByCoreToolsAllowList` + // is also true under `[]`). typeof pm.isCoreToolsAllowListEmpty === 'function' && pm.isCoreToolsAllowListEmpty() ) { From 6d3c84eecf42172cfd5c7eb3ab449ba153114e42 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 29 Aug 2026 07:50:29 +0800 Subject: [PATCH 17/29] fix(core): normalize grammar-sensitive tool schemas Co-authored-by: Qwen-Coder --- ...penai-tool-schema-grammar-compatibility.md | 25 ++ docs/users/configuration/settings.md | 12 +- docs/users/features/structured-output.md | 12 +- packages/cli/src/config/config.test.ts | 338 +----------------- packages/cli/src/config/config.ts | 75 +--- packages/cli/src/config/settingsSchema.ts | 2 +- packages/core/src/config/config.ts | 4 +- .../core/src/core/coreToolScheduler.test.ts | 109 ------ packages/core/src/core/coreToolScheduler.ts | 18 - .../openaiContentGenerator/pipeline.test.ts | 45 --- .../core/openaiContentGenerator/pipeline.ts | 46 +-- packages/core/src/permissions/index.ts | 5 +- .../permissions/permission-manager.test.ts | 146 +------- .../src/permissions/permission-manager.ts | 125 +------ packages/core/src/tools/tool-registry.test.ts | 51 --- packages/core/src/tools/tool-registry.ts | 24 -- .../core/src/utils/schemaConverter.test.ts | 54 ++- packages/core/src/utils/schemaConverter.ts | 31 +- .../schemas/settings.schema.json | 2 +- 19 files changed, 159 insertions(+), 965 deletions(-) create mode 100644 docs/design/openai-tool-schema-grammar-compatibility.md diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md new file mode 100644 index 00000000000..4976daf444f --- /dev/null +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -0,0 +1,25 @@ +# OpenAI Tool Schema Grammar Compatibility + +## Problem + +Some OpenAI-compatible runtimes compile every function schema in a request into one grammar. Older llama.cpp builds reject empty `properties` maps and large string or array repetition limits, so one valid but unsupported schema prevents the entire request from starting. Qwen Code ships tools with both shapes and can also receive them from MCP servers. + +Disabling all tools avoids grammar construction but also removes the functionality the user asked the agent to use. It is therefore a diagnostic fallback, not the primary fix. + +## Design + +Keep the registered tool set unchanged. Before an OpenAI-compatible request is sent, recursively relax only the wire copy of each schema: + +- omit empty `properties` maps and the same level's `additionalProperties: false` constraint; +- omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above the grammar repetition boundary of 2000; +- preserve smaller limits and all other supported constraints. + +The original schema remains attached to the tool and continues to drive client-side parameter validation. The provider receives a schema it can compile, while Qwen Code still rejects tool calls that violate the original limits. + +## Compatibility + +This applies to both built-in tools and MCP-provided schemas because they share the same OpenAI conversion boundary. Native Gemini requests are unchanged. Providers that accept the original constraints receive a slightly relaxed wire schema, but local validation preserves their behavior. + +## Verification + +Unit coverage exercises recursive empty objects, the 1999/2000 boundary, the actual OpenAI tool converter, and source-schema immutability. A live LM Studio smoke remains useful when that runtime is available, but the regression test pins the exact request shapes that caused grammar initialization to fail. diff --git a/docs/users/configuration/settings.md b/docs/users/configuration/settings.md index 1c7133e5461..b18ab489ce6 100644 --- a/docs/users/configuration/settings.md +++ b/docs/users/configuration/settings.md @@ -364,7 +364,7 @@ If you are experiencing performance issues with file searching (e.g., with `@` c | `tools.shell.enableInteractiveShell` | boolean | Use `node-pty` for an interactive shell experience. Fallback to `child_process` still applies. | `true` | | | `tools.shell.defaultTimeoutMs` | number | Default timeout, in milliseconds, for foreground shell commands started by the agent. A per-call timeout on the shell tool overrides this. When unset, foreground commands time out after 120000 ms (2 minutes). Set to 0 to disable the timeout. | `undefined` | | | `tools.shell.heartbeatIntervalMs` | number | Interval, in milliseconds, between liveness heartbeats emitted while a foreground shell command produces no output. Heartbeats are forwarded to ACP clients and stream-json consumers so they can tell a silent command from a dead session. When unset, heartbeats fire every 10000 ms (10 seconds). Set to 0 to disable heartbeats. | `undefined` | | -| `tools.core` | array of strings | **Deprecated.** Will be removed in next version. A non-empty list restricts the core tool set (file, shell, search and related built-ins) to an allowlist: core tools not in the list are disabled (fail-closed). Tools outside that set — dynamically discovered tools (MCP, skill) and synthetic/system built-ins such as `agent`, `list_agents`, plan-mode lifecycle tools, goal tools, `task_stop`, `send_message` and `tool_search` — bypass the allowlist by design; use `permissions.deny` to remove a tool outright. An explicitly empty list (`[]`, or one whose entries carry no tool name) is an ACTIVE allowlist that disables every registered tool — built-ins, MCP, and synthetic tools alike (#10065); omit the key or set it to `null` for no restriction. `permissions.allow` cannot reproduce this restriction — it is pure auto-approval (#10075). Use `tools.eager` to restrict which eager-by-default tool schemas are sent initially (unlisted tools are deferred, not disabled — they stay loadable via `tool_search`), and `permissions.deny` to block tools outright. | `undefined` | | +| `tools.core` | array of strings | **Deprecated.** Will be removed in next version. A non-empty list restricts the core tool set (file, shell, search and related built-ins) to an allowlist: core tools not in the list are disabled (fail-closed). Tools outside that set — dynamically discovered tools (MCP, skill) and synthetic/system built-ins such as `agent`, `list_agents`, plan-mode lifecycle tools, goal tools, `task_stop`, `send_message` and `tool_search` — bypass the allowlist by design; use `permissions.deny` to remove a tool outright. An empty list (`[]`) is treated as unset and disables nothing. `permissions.allow` cannot reproduce this restriction — it is pure auto-approval (#10075). Use `tools.eager` to restrict which eager-by-default tool schemas are sent initially (unlisted tools are deferred, not disabled — they stay loadable via `tool_search`), and `permissions.deny` to block tools outright. | `undefined` | | | `tools.exclude` | array of strings | **Deprecated.** Use `permissions.deny` instead. Tool names to exclude from discovery. Not automatically migrated; the legacy setting remains honoured at startup. | `undefined` | | | `tools.disabled` | array of strings | Tool names hidden from the registry entirely. Unlike `permissions.deny` (which blocks calls at runtime), disabled tools are never registered, so they do not appear in `/tools` and cannot be discovered or called by the model. For example, `["enter_plan_mode"]` prevents the model from switching into plan mode on its own. Merged as a union across scopes. | `undefined` | | | `tools.visible` | array of strings | Deferred tool names made visible at startup without requiring `tool_search`. Listed tools appear alongside core tools in the initial session. Merged as a union across scopes. | `undefined` | | @@ -483,11 +483,11 @@ Permission rules for `Read`, `Edit`, and `WebFetch` are also enforced when the a **Migrating from legacy settings:** -| Legacy setting | Equivalent `permissions` rule | Notes | -| --------------- | ------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `tools.allowed` | `permissions.allow` | Not automatically migrated; still honoured at startup | -| `tools.exclude` | `permissions.deny` | Not automatically migrated; still honoured at startup | -| `tools.core` | `tools.eager` (+ `permissions.deny`) | Not auto-migrated to `permissions.allow`, which is pure auto-approval and cannot reproduce the allowlist restriction (#10075). `tools.eager` defers unlisted eager-by-default tools (they stay loadable via `tool_search`); `permissions.deny` removes tools entirely. Neither preserves a non-empty `tools.core` allowlist's fail-closed guarantee over the core tool set: a built-in added in a future release registers until explicitly denied, so a deny list must be re-audited per release. An explicitly empty `tools.core` list (`[]`, or one whose entries carry no tool name) is an active allowlist that disables every tool (#10065); omit the key for no restriction. | +| Legacy setting | Equivalent `permissions` rule | Notes | +| --------------- | ------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `tools.allowed` | `permissions.allow` | Not automatically migrated; still honoured at startup | +| `tools.exclude` | `permissions.deny` | Not automatically migrated; still honoured at startup | +| `tools.core` | `tools.eager` (+ `permissions.deny`) | Not auto-migrated to `permissions.allow`, which is pure auto-approval and cannot reproduce the allowlist restriction (#10075). `tools.eager` defers unlisted eager-by-default tools (they stay loadable via `tool_search`); `permissions.deny` removes tools entirely. Neither preserves a non-empty `tools.core` allowlist's fail-closed guarantee over the core tool set: a built-in added in a future release registers until explicitly denied, so a deny list must be re-audited per release. An empty `tools.core` list is treated as unset and disables nothing. | **Example configuration:** diff --git a/docs/users/features/structured-output.md b/docs/users/features/structured-output.md index 4a8ded36aec..78c7e30eee1 100644 --- a/docs/users/features/structured-output.md +++ b/docs/users/features/structured-output.md @@ -259,15 +259,9 @@ synthetic tool is registered when the CLI parses its arguments, so: ## Permission gating -`structured_output` deliberately bypasses a NON-EMPTY `--core-tools` -allowlist: the tool only exists when `--json-schema` is set, so -excluding it would leave the run with no terminal contract. - -An explicitly EMPTY allowlist (`tools.core: []`) is the deliberate -"no tools at all" configuration: it disables `structured_output` too, -so a `--json-schema` run under `tools.core: []` cannot complete — the -model has no terminal contract to end the run. Remove the setting or -list the tools you need if you want structured output to finish. +`structured_output` deliberately bypasses the `--core-tools` allowlist: +the tool only exists when `--json-schema` is set, so excluding it +would leave the run with no terminal contract. Explicit `permissions.deny` rules and `--exclude-tools` settings DO take effect — both use the same deny mechanism and both prevent diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index d7ffd09fc75..06ce41a1f1f 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -3019,339 +3019,6 @@ describe('Approval mode tool exclusion logic', () => { expect(excludedTools).toContain(ToolNames.WRITE_FILE); }); - it('should preserve an explicitly empty tools.core allowlist', async () => { - process.argv = ['node', 'script.js', '-p', 'test']; - const argv = await parseArguments(); - const settings: Settings = { - tools: { - core: [], - }, - }; - - const config = await loadCliConfig(settings, argv, undefined, []); - - expect(config.getCoreTools()).toEqual([]); - }); - - it('should treat a bare --core-tools flag with no values as absent', async () => { - // yargs yields `[]` for the bare flag. Treating that argv-sourced - // empty list as "disable every tool" would silently tool-free the - // session on a forgotten flag value; only `tools.core: []` in - // settings should carry the empty-allowlist semantic (#10065). - process.argv = ['node', 'script.js', '--core-tools', '-p', 'test']; - const argv = await parseArguments(); - const settings: Settings = {}; - - const config = await loadCliConfig(settings, argv, undefined, []); - - expect(config.getCoreTools()).toBeUndefined(); - }); - - it('should fall back to settings tools.core when --core-tools has no values', async () => { - process.argv = ['node', 'script.js', '--core-tools', '-p', 'test']; - const argv = await parseArguments(); - const settings: Settings = { - tools: { - core: ['read_file'], - }, - }; - - const config = await loadCliConfig(settings, argv, undefined, []); - - expect(config.getCoreTools()).toEqual(['read_file']); - }); - - it('should prefer --core-tools over settings tools.core when both are set', async () => { - // argv-over-settings precedence on the resolved coreTools ternary: - // a valued --core-tools flag must win over settings tools.core - // instead of being overridden by it (#10065). - process.argv = [ - 'node', - 'script.js', - '--core-tools', - 'web_fetch', - '-p', - 'test', - ]; - const argv = await parseArguments(); - const settings: Settings = { - tools: { - core: ['read_file'], - }, - }; - - const config = await loadCliConfig(settings, argv, undefined, []); - - expect(config.getCoreTools()).toEqual(['web_fetch']); - }); - - it('should warn at startup when tools.core is an explicitly empty allowlist', async () => { - process.argv = ['node', 'script.js', '-p', 'test']; - const argv = await parseArguments(); - const emptySettings: Settings = { - tools: { - core: [], - }, - }; - - mockWriteStderrLine.mockClear(); - const config = await loadCliConfig(emptySettings, argv, undefined, []); - expect(config.getCoreTools()).toEqual([]); - expect(mockWriteStderrLine).toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - - // A non-empty allowlist must not trigger the notice. - const valuedSettings: Settings = { - tools: { - core: ['read_file'], - }, - }; - - mockWriteStderrLine.mockClear(); - await loadCliConfig(valuedSettings, argv, undefined, []); - expect(mockWriteStderrLine).not.toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - }); - - it('should treat empty-string --core-tools values as absent, not as a [""] allowlist', async () => { - // A script expanding an unset variable (`qwen --core-tools "$TOOLS"`) - // hands yargs `[""]`; `--core-tools ,` yields `["", ""]`. Neither - // carries a tool name, so both must collapse to the absent-flag path - // instead of a one-entry allowlist that matches nothing and silently - // disables every tool with neither diagnostic firing (#10065). - for (const emptyValue of ['', ',']) { - process.argv = [ - 'node', - 'script.js', - '--core-tools', - emptyValue, - '-p', - 'test', - ]; - const argv = await parseArguments(); - const emptySettings: Settings = {}; - - mockWriteStderrLine.mockClear(); - const config = await loadCliConfig(emptySettings, argv, undefined, []); - expect(config.getCoreTools()).toBeUndefined(); - expect(mockWriteStderrLine).not.toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - } - - // Valued entries survive the empty-entry filter. - process.argv = [ - 'node', - 'script.js', - '--core-tools', - 'read_file,', - '-p', - 'test', - ]; - const argv = await parseArguments(); - const settings: Settings = {}; - - const config = await loadCliConfig(settings, argv, undefined, []); - expect(config.getCoreTools()).toEqual(['read_file']); - }); - - it('should warn for an all-empty-string tools.core exactly like for []', async () => { - // `"core": [""]` (an unset-variable expansion or hand-edit written - // into settings) carries no tool name: PermissionManager.initialize() - // collapses it to the explicit empty allowlist, so the startup notice - // must fire for it too (#10065). A list still naming a tool must not. - process.argv = ['node', 'script.js', '-p', 'test']; - const argv = await parseArguments(); - const emptyEntrySettings: Settings = { - tools: { - core: [''], - }, - }; - - mockWriteStderrLine.mockClear(); - await loadCliConfig(emptyEntrySettings, argv, undefined, []); - expect(mockWriteStderrLine).toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - - const valuedSettings: Settings = { - tools: { - core: ['', 'read_file'], - }, - }; - - mockWriteStderrLine.mockClear(); - await loadCliConfig(valuedSettings, argv, undefined, []); - expect(mockWriteStderrLine).not.toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - }); - - it('should warn for name-less tools.core entries exactly like for []', async () => { - // `"core": ["()"]` and similar mangled specifiers are non-empty - // strings whose parsed tool name is empty; PermissionManager - // collapses them to the explicit empty allowlist, so the startup - // notice must fire for them too — gate and notice share the - // isNamelessCoreToolsEntry classification (#10065, #10080). A list - // still naming a tool must not warn. - process.argv = ['node', 'script.js', '-p', 'test']; - const argv = await parseArguments(); - for (const core of [['()'], ['(ls -l)']]) { - const namelessSettings: Settings = { - tools: { - core, - }, - }; - - mockWriteStderrLine.mockClear(); - await loadCliConfig(namelessSettings, argv, undefined, []); - expect(mockWriteStderrLine).toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - } - - const valuedSettings: Settings = { - tools: { - core: ['()', 'read_file'], - }, - }; - - mockWriteStderrLine.mockClear(); - await loadCliConfig(valuedSettings, argv, undefined, []); - expect(mockWriteStderrLine).not.toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - }); - - it('should treat a non-array tools.core value as absent', async () => { - // Settings loading performs no type validation, so a hand-edited - // `"core": ""` reaches the coreTools ternary raw; it must normalize - // to ABSENT (undefined) instead of flowing into Config.coreTools, - // where `getCoreTools()?.some(...)` would throw during tool-registry - // construction (#10065). - process.argv = ['node', 'script.js', '-p', 'test']; - const argv = await parseArguments(); - const settings: Settings = { - tools: { - core: '' as unknown as string[], - }, - }; - - const config = await loadCliConfig(settings, argv, undefined, []); - expect(config.getCoreTools()).toBeUndefined(); - }); - - it('should drop non-string tools.core entries instead of crashing startup', async () => { - // Settings loading performs no element validation (the schema declares - // only `type: 'array'`), so a hand-edited `"core": [42]` — or a mixed - // `["read_file", 42]` — reaches loadCliConfig raw. Without the - // boundary filter, the non-interactive exclusion path (`isToolEnabled` - // → `filterList` → `entry.trim()`) throws a TypeError inside - // loadCliConfig, and `isLsToolEnabled()` → `parseRule(42)` would - // crash tool-registry construction. The entries must be dropped, and - // an all-non-string list collapses to the explicit empty allowlist - // with the #10065 startup notice (#10080). - process.argv = ['node', 'script.js', '-p', 'test']; - const argv = await parseArguments(); - - mockWriteStderrLine.mockClear(); - const config = await loadCliConfig( - { tools: { core: [42 as unknown as string] } }, - argv, - undefined, - [], - ); - expect(config.getCoreTools()).toEqual([]); - expect(mockWriteStderrLine).toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - - mockWriteStderrLine.mockClear(); - const mixed = await loadCliConfig( - { tools: { core: ['read_file', 42 as unknown as string] } }, - argv, - undefined, - [], - ); - expect(mixed.getCoreTools()).toEqual(['read_file']); - expect(mockWriteStderrLine).not.toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - }); - - it('should suppress the empty-allowlist notice when a valued --core-tools overrides the empty settings list', async () => { - // A valued argv flag wins over settings (its tools ARE registered), - // so the "all tools are disabled" notice would be factually wrong - // here (#10065). - process.argv = [ - 'node', - 'script.js', - '--core-tools', - 'web_fetch', - '-p', - 'test', - ]; - const argv = await parseArguments(); - const settings: Settings = { - tools: { - core: [], - }, - }; - - mockWriteStderrLine.mockClear(); - const config = await loadCliConfig(settings, argv, undefined, []); - expect(config.getCoreTools()).toEqual(['web_fetch']); - expect(mockWriteStderrLine).not.toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - }); - - it('should suppress the empty-allowlist notice in safe mode and bare mode', async () => { - // Neither mode honours settings.tools.core — safe mode forces the - // CLI-level coreTools to undefined and bare mode replaces it with - // the built-in bare toolset — so the notice advice could never - // re-enable tools there. - const settings: Settings = { - tools: { - core: [], - }, - }; - - for (const flag of ['--safe-mode', '--bare']) { - process.argv = ['node', 'script.js', flag, '-p', 'test']; - const argv = await parseArguments(); - - mockWriteStderrLine.mockClear(); - await loadCliConfig(settings, argv, undefined, []); - // Note: safe mode resolves coreTools to `undefined` via the ternary's - // `bareMode || safeMode` arm (there is no built-in safe-tools list), - // so only the notice suppression is asserted here. - expect(mockWriteStderrLine).not.toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - } - }); - - it('should still warn for a bare --core-tools flag over an empty settings allowlist', async () => { - // A bare flag is treated as ABSENT, so the empty settings list - // carries the session semantic and the notice must fire (#10065). - process.argv = ['node', 'script.js', '--core-tools', '-p', 'test']; - const argv = await parseArguments(); - const settings: Settings = { - tools: { - core: [], - }, - }; - - mockWriteStderrLine.mockClear(); - await loadCliConfig(settings, argv, undefined, []); - expect(mockWriteStderrLine).toHaveBeenCalledWith( - expect.stringContaining('tools.core is an empty allowlist'), - ); - }); - it('should exclude only shell tools in non-interactive mode with auto-edit approval mode', async () => { process.argv = [ 'node', @@ -4448,9 +4115,8 @@ describe('loadCliConfig tools.eager wiring (#9827, #10075)', () => { }; const config = await loadCliConfig(settings, argv, undefined, []); - // `[]` is an active allowlist naming nothing (defer everything); `tools.core` - // differs only in effect: its explicitly empty list stays active and - // disables every tool instead of deferring (#10065). + // `[]` is an active allowlist naming nothing (defer everything), unlike + // `tools.core`, where an empty list is treated as unset. expect(config.getEagerTools()).toEqual([]); }); diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 4127f49d900..a724ecb2570 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -34,7 +34,6 @@ import { createDebugLogger, NativeLspService, isBareMode, - isNamelessCoreToolsEntry, isTruthy, isSafeModeEnv, isToolEnabled, @@ -918,18 +917,8 @@ export async function parseArguments(): Promise { type: 'array', string: true, description: 'Core tool paths', - // Empty entries carry no tool name (e.g. a script expanding an - // unset variable — `qwen --core-tools "$TOOLS"` — hands yargs - // `[""]`); drop them so the flag collapses to the absent-flag - // path instead of a one-entry allowlist that matches nothing - // and silently disables every tool (#10065). coerce: (tools: string[]) => - tools.flatMap((tool) => - tool - .split(',') - .map((t) => t.trim()) - .filter((t) => t !== ''), - ), + tools.flatMap((tool) => tool.split(',').map((t) => t.trim())), }) .option('exclude-tools', { type: 'array', @@ -1784,47 +1773,9 @@ export async function loadCliConfig( '⚠ Safe mode: --core-tools flag is ignored (settings-sourced core tools are also disabled).\n', ); } - // An explicit `tools.core: []` in settings disables every tool for the - // session (the empty-allowlist semantic of #10065). Give the user one - // visible startup notice instead of failing every tool call silently; - // the argv flag wins over settings when valued, so only the settings - // path can resolve to the empty allowlist here. Entries carrying no - // tool name — empty/whitespace strings, non-string garbage, malformed - // rules, and mangled specifiers like `"()"` (e.g. an unset-variable or - // template expansion written into settings) — collapse to the same - // empty allowlist in `PermissionManager.initialize()`; the gate and - // this notice share the `isNamelessCoreToolsEntry` classification from - // core so they cannot drift apart (#10065, #10080). - if ( - !bareMode && - !safeMode && - !(argv.coreTools && argv.coreTools.length > 0) && - Array.isArray(settings.tools?.core) && - settings.tools.core.every((t) => isNamelessCoreToolsEntry(t)) - ) { - writeStderrLine( - '⚠ tools.core is an empty allowlist: all tools are disabled for this session. Add tool names to tools.core or remove the key to re-enable tools (#10065).\n', - ); - } - // Settings loading performs no element validation (the schema declares only - // `type: 'array'`), so a hand-edited `"core": [42]` — or a mixed - // `["read_file", 42]` — would otherwise reach Config.coreTools and - // resolvedCoreTools raw, where `parseRule(42)`/`entry.trim()` throw a - // TypeError during tool-registry construction instead of the entry being - // dropped (#10080). Only an ARRAY activates the allowlist (a non-array - // normalizes to ABSENT/undefined, as the pre-#10065 `||` chain did); drop - // non-string entries at the boundary. Empty/whitespace strings are KEPT so - // they collapse to the explicit empty allowlist (with both #10065 - // diagnostics) in PermissionManager.initialize() rather than being treated - // as ABSENT. - const settingsCoreTools: string[] | undefined = Array.isArray( - settings.tools?.core, - ) - ? settings.tools.core.filter((t) => typeof t === 'string') - : undefined; const resolvedCoreTools: string[] = [ ...(bareMode || safeMode ? [] : (argv.coreTools ?? [])), - ...(bareMode || safeMode ? [] : (settingsCoreTools ?? [])), + ...(bareMode || safeMode ? [] : (settings.tools?.core ?? [])), ]; const mergedAllow: string[] = [ ...(bareMode || safeMode ? [] : (settings.permissions?.allow ?? [])), @@ -1891,9 +1842,7 @@ export async function loadCliConfig( // // An explicitly empty array must survive as an empty array, not collapse // into "unset": `[]` is an active allowlist naming nothing (defer - // everything). `tools.core` differs only in effect: its explicitly - // empty list stays active and disables every tool instead of - // deferring (#10065). + // everything). `tools.core` differs: its empty list is treated as unset. // `normalizeDisabledToolList` maps undefined to `[]`, // so the Array.isArray guard has to come first — without it, absent and // explicitly-empty would reach core as the same value, which is exactly @@ -2252,26 +2201,10 @@ export async function loadCliConfig( systemPrompt: argv.systemPrompt, appendSystemPrompt: argv.appendSystemPrompt, // Legacy fields – kept for backward compatibility with getCoreTools() etc. - // A bare `--core-tools` flag with no values yields `[]` from yargs. - // Treat an argv-sourced empty list as ABSENT, not as "disable every - // tool": only an explicit `tools.core: []` in settings (or a valued - // flag) should carry the empty-allowlist semantic, so a forgotten - // flag value — or a script expanding an unset variable into the flag - // — cannot silently flip a session to tool-free (#10065). The - // settings leg reads `settingsCoreTools` because settings loading - // performs no element validation: a hand-edited non-array `"core": ""` - // normalizes to ABSENT (as the pre-#10065 `||` chain did), and non-string - // array entries (`"core": [42]`) are dropped — either shape flowing raw - // into Config.coreTools would make `getCoreTools()?.some(...)` throw - // during tool-registry construction (#10080). Empty-string entries are - // kept there and dropped where the allowlist is built - // (PermissionManager.initialize, #10065). coreTools: bareMode || safeMode ? undefined - : argv.coreTools?.length - ? argv.coreTools - : settingsCoreTools, + : argv.coreTools || settings.tools?.core || undefined, allowedTools: bareMode || safeMode ? argv.allowedTools || undefined diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index df51bf3bfd5..34d6822df18 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -2748,7 +2748,7 @@ const SETTINGS_SCHEMA = { requiresRestart: true, default: undefined as string[] | undefined, description: - 'Deprecated. permissions.allow cannot reproduce this registration restriction because it only auto-approves calls. Use tools.eager to defer unlisted eager-by-default tools or permissions.deny to remove tools. An explicitly empty list ([]) is an active allowlist that disables every tool; omit the key or set it to null for no restriction (#10065).', + 'Deprecated. permissions.allow cannot reproduce this registration restriction because it only auto-approves calls. Use tools.eager to defer unlisted eager-by-default tools or permissions.deny to remove tools. An empty list is treated as unset and disables nothing.', showInDialog: false, }, allowed: { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 812ce38626d..c14fecf8533 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2584,9 +2584,7 @@ export class Config { ); // An explicitly empty array is preserved as an ACTIVE-but-empty // allowlist (defer everything); only `undefined` means "no - // restriction". `tools.core` differs only in effect: its explicitly - // empty list stays active and disables every tool instead of - // deferring (#10065). + // restriction". `tools.core` differs: its empty list is treated as unset. this.eagerTools = params.eagerTools === undefined ? undefined diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index e1c673ab843..2a0b19e627e 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -3707,115 +3707,6 @@ describe('CoreToolScheduler', () => { expect(execute).not.toHaveBeenCalled(); }); - it('attributes the rejection to the empty tools.core allowlist instead of promising a permissions.allow fix (#10065)', async () => { - // With an active `permissions.allow` allowlist AND `tools.core: []`, - // an UNCOVERED tool used to get the "add a permissions.allow rule" - // advice — which can never succeed because the empty coreTools gate - // rejects the tool even after it becomes covered. The message must - // name `tools.core` instead (#10065). - const execute = vi.fn().mockResolvedValue({ - llmContent: 'sent', - returnDisplay: 'sent', - }); - const toolsByName = new Map([ - [ToolNames.EDIT, new MockTool({ name: ToolNames.EDIT, execute })], - ]); - const permissionManager = { - isToolEnabled: vi.fn().mockResolvedValue(false), - findMatchingDenyRule: vi.fn().mockReturnValue(undefined), - isCoreToolsAllowListEmpty: vi.fn().mockReturnValue(true), - }; - const { scheduler, onAllToolCallsComplete } = - createSchedulerForLegacyToolTests({ - toolsByName, - permissionManager, - }); - - await scheduler.schedule( - [ - { - callId: 'empty-core-tools-uncovered', - name: ToolNames.EDIT, - args: {}, - isClientInitiated: false, - prompt_id: 'prompt-empty-core-tools-uncovered', - }, - ], - new AbortController().signal, - ); - - expect(onAllToolCallsComplete).toHaveBeenCalled(); - const completedCalls = onAllToolCallsComplete.mock - .calls[0][0] as ToolCall[]; - const completedCall = completedCalls[0]; - expect(completedCall.status).toBe('error'); - if (completedCall.status === 'error') { - expect(completedCall.response.errorType).toBe( - ToolErrorType.EXECUTION_DENIED, - ); - const message = completedCall.response.error?.message ?? ''; - expect(message).toContain('tools.core'); - expect(message).toContain('"edit"'); - // The gate is restart-scoped (coreToolsAllowList is snapshotted in - // initialize(), `tools.core` is requiresRestart), so the advice - // must carry the restart caveat like its sibling messages - // (#10065). - expect(message).toContain('restart'); - expect(message).not.toContain('permissions.allow'); - expect(message).not.toContain('permission was declined'); - } - expect(execute).not.toHaveBeenCalled(); - }); - - it('names tools.core for a covered tool rejected by the empty allowlist instead of the generic declined message (#10065)', async () => { - // Covered by an allow rule but still rejected by `tools.core: []`: - // the generic "permission was declined" text is factually wrong - // (nothing was ever asked or declined), so the attribution must - // name the empty allowlist (#10065). - const execute = vi.fn().mockResolvedValue({ - llmContent: 'sent', - returnDisplay: 'sent', - }); - const toolsByName = new Map([ - [ToolNames.EDIT, new MockTool({ name: ToolNames.EDIT, execute })], - ]); - const permissionManager = { - isToolEnabled: vi.fn().mockResolvedValue(false), - findMatchingDenyRule: vi.fn().mockReturnValue(undefined), - isCoreToolsAllowListEmpty: vi.fn().mockReturnValue(true), - }; - const { scheduler, onAllToolCallsComplete } = - createSchedulerForLegacyToolTests({ - toolsByName, - permissionManager, - }); - - await scheduler.schedule( - [ - { - callId: 'empty-core-tools-covered', - name: ToolNames.EDIT, - args: {}, - isClientInitiated: false, - prompt_id: 'prompt-empty-core-tools-covered', - }, - ], - new AbortController().signal, - ); - - expect(onAllToolCallsComplete).toHaveBeenCalled(); - const completedCalls = onAllToolCallsComplete.mock - .calls[0][0] as ToolCall[]; - const completedCall = completedCalls[0]; - expect(completedCall.status).toBe('error'); - if (completedCall.status === 'error') { - const message = completedCall.response.error?.message ?? ''; - expect(message).toContain('tools.core'); - expect(message).not.toContain('permission was declined'); - } - expect(execute).not.toHaveBeenCalled(); - }); - it('attributes a rejection by the legacy coreTools allowlist to core tools (#10075)', async () => { // Since #10075 an uncovered `permissions.allow` tool is deferred (still // registered and callable), never rejected at call time — so a diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 054776b5f47..3a6a9b2b95f 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -2474,24 +2474,6 @@ export class CoreToolScheduler { let permissionErrorMessage: string; if (matchingRule) { permissionErrorMessage = `Qwen Code requires permission to use "${reqInfo.name}", but that permission was declined. Matching deny rule: "${matchingRule}".`; - } else if ( - // An explicitly empty coreTools allowlist (`tools.core: []`) - // rejects EVERY tool, so both branches that follow are wrong - // here: the coreTools-miss branch advises adding the tool to - // the core tools list, but for a deliberately tool-free - // session the remedy is removing the empty setting — and the - // generic "permission was declined" fallback implies - // something was asked and declined, which never happened. - // Name the responsible knob instead (#10065). The optional - // call keeps scoped PermissionManager shims from throwing - // until they grow the method; this branch sits ahead of the - // coreTools-miss branch because the empty gate rejects - // listed-or-not tools alike (`isToolDisabledByCoreToolsAllowList` - // is also true under `[]`). - typeof pm.isCoreToolsAllowListEmpty === 'function' && - pm.isCoreToolsAllowListEmpty() - ) { - permissionErrorMessage = `"${reqInfo.name}" is disabled because the core tools allowlist (settings tools.core) is explicitly empty. Remove the setting or list the tool there, and restart, to re-enable it.`; } else if ( // The legacy `coreTools` allowlist (`--core-tools` / settings // `tools.core`) keeps its hard-disable semantic: an unlisted diff --git a/packages/core/src/core/openaiContentGenerator/pipeline.test.ts b/packages/core/src/core/openaiContentGenerator/pipeline.test.ts index 9ab61cab6f1..a46a564fb77 100644 --- a/packages/core/src/core/openaiContentGenerator/pipeline.test.ts +++ b/packages/core/src/core/openaiContentGenerator/pipeline.test.ts @@ -643,51 +643,6 @@ describe('ContentGenerationPipeline', () => { expect(apiCall.tools).toBeUndefined(); }); - it('should skip the wrapped-empty tools shape produced by client.setTools() on an empty registry', async () => { - // Arrange — the #10065 empty allowlist disables every tool, so - // client.setTools() wraps ZERO declarations as - // `[{ functionDeclarations: [] }]`. The wrapper's `.length` is 1, so - // a length-only guard would still ship `tools: []` on the wire, - // which some providers reject (#10080). The converted declarations - // are empty, so the field must be omitted entirely. - const request: GenerateContentParameters = { - model: 'test-model', - contents: [{ parts: [{ text: 'Hello' }], role: 'user' }], - config: { tools: [{ functionDeclarations: [] }] }, - }; - const userPromptId = 'test-prompt-id'; - - const mockMessages = [ - { role: 'user', content: 'Hello' }, - ] as OpenAI.Chat.ChatCompletionMessageParam[]; - const mockOpenAIResponse = { - id: 'response-id', - choices: [{ message: { content: 'Response' }, finish_reason: 'stop' }], - } as OpenAI.Chat.ChatCompletion; - const mockLlmResponse = new GenerateContentResponse(); - - (mockConverter.convertLlmRequestToOpenAI as Mock).mockReturnValue( - mockMessages, - ); - (mockConverter.convertLlmToolsToOpenAI as Mock).mockResolvedValue([]); - (mockConverter.convertOpenAIResponseToLlm as Mock).mockReturnValue( - mockLlmResponse, - ); - (mockClient.chat.completions.create as Mock).mockResolvedValue( - mockOpenAIResponse, - ); - - // Act - await pipeline.execute(request, userPromptId); - - // Assert — conversion ran (wrapper is non-empty) but the empty - // result must not reach the wire. - expect(mockConverter.convertLlmToolsToOpenAI).toHaveBeenCalled(); - const apiCall = (mockClient.chat.completions.create as Mock).mock - .calls[0][0]; - expect(apiCall.tools).toBeUndefined(); - }); - it('should override enable_thinking when thinkingConfig disables it', async () => { // Arrange — provider injects enable_thinking: true via extra_body // (e.g. user configured `enableThinking: true` via setup wizard, diff --git a/packages/core/src/core/openaiContentGenerator/pipeline.ts b/packages/core/src/core/openaiContentGenerator/pipeline.ts index 24b2ab719e6..63ab4db3c6e 100644 --- a/packages/core/src/core/openaiContentGenerator/pipeline.ts +++ b/packages/core/src/core/openaiContentGenerator/pipeline.ts @@ -1079,35 +1079,25 @@ export class ContentGenerationPipeline { } // Add tools if present and non-empty. - // Some providers reject tools: [] (empty array), so skip when there are - // no tools. Guard on the CONVERTED declarations, not the wrapper length: - // client.setTools() always wraps declarations as `[{ functionDeclarations }]` - // even when the registry is empty (the #10065 empty allowlist disables - // every tool), so the wrapper's own `.length` is 1 while it carries zero - // declarations — without this guard an empty allowlist would still ship - // `tools: []` on the wire (#10080). + // Some providers reject tools: [] (empty array), so skip when there are no tools. if (request.config?.tools && request.config.tools.length > 0) { - const convertedTools = - await OpenAIContentConverter.convertLlmToolsToOpenAI( - request.config.tools, - this.contentGeneratorConfig.schemaCompliance ?? 'auto', - ); - if (convertedTools.length > 0) { - baseRequest.tools = convertedTools; - - // Map Gemini-style toolConfig.functionCallingConfig.mode to OpenAI's - // tool_choice so structured side queries (e.g. the AUTO-mode - // classifier's respond_in_schema) can force the model to emit a tool - // call instead of free-texting. Without this, thinking-heavy models - // may consume the tiny output budget on reasoning and skip the tool. - const fcMode = request.config?.toolConfig?.functionCallingConfig?.mode; - if (fcMode === 'ANY') { - (baseRequest as unknown as Record)['tool_choice'] = - 'required'; - } else if (fcMode === 'NONE') { - (baseRequest as unknown as Record)['tool_choice'] = - 'none'; - } + baseRequest.tools = await OpenAIContentConverter.convertLlmToolsToOpenAI( + request.config.tools, + this.contentGeneratorConfig.schemaCompliance ?? 'auto', + ); + + // Map Gemini-style toolConfig.functionCallingConfig.mode to OpenAI's + // tool_choice so structured side queries (e.g. the AUTO-mode + // classifier's respond_in_schema) can force the model to emit a tool + // call instead of free-texting. Without this, thinking-heavy models + // may consume the tiny output budget on reasoning and skip the tool. + const fcMode = request.config?.toolConfig?.functionCallingConfig?.mode; + if (fcMode === 'ANY') { + (baseRequest as unknown as Record)['tool_choice'] = + 'required'; + } else if (fcMode === 'NONE') { + (baseRequest as unknown as Record)['tool_choice'] = + 'none'; } } diff --git a/packages/core/src/permissions/index.ts b/packages/core/src/permissions/index.ts index bddbe24d0c5..a13c72ed9dc 100644 --- a/packages/core/src/permissions/index.ts +++ b/packages/core/src/permissions/index.ts @@ -6,10 +6,7 @@ export * from './types.js'; export * from './rule-parser.js'; -export { - PermissionManager, - isNamelessCoreToolsEntry, -} from './permission-manager.js'; +export { PermissionManager } from './permission-manager.js'; export type { PermissionManagerConfig } from './permission-manager.js'; export { extractShellOperations } from './shell-semantics.js'; export type { ShellOperation } from './shell-semantics.js'; diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 86c315a5538..c18afd6cdda 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -26,10 +26,7 @@ import { buildHumanReadableRuleLabel, TOOL_NAME_ALIASES, } from './rule-parser.js'; -import { - PermissionManager, - isNamelessCoreToolsEntry, -} from './permission-manager.js'; +import { PermissionManager } from './permission-manager.js'; import type { PermissionManagerConfig } from './permission-manager.js'; import { normalizeToolNameForProvider } from '../utils/tool-name-utils.js'; import { ToolNames, ToolDisplayNames } from '../tools/tool-names.js'; @@ -2569,127 +2566,11 @@ describe('PermissionManager', () => { expect(await pm.isToolEnabled('read_file')).toBe(false); }); - it('empty coreTools disables every tool', async () => { + it('empty coreTools: all tools enabled (no whitelist restriction)', async () => { pm = new PermissionManager(makeConfig({ coreTools: [] })); pm.initialize(); - expect(await pm.isToolEnabled('read_file')).toBe(false); - expect(await pm.isToolEnabled('run_shell_command')).toBe(false); - expect(await pm.isToolEnabled('agent')).toBe(false); - // The empty gate must also win against the `isToolEnabled` - // exemptions: `structured_output` bypasses the permissions.allow - // gate and `mcp__*` names bypass non-empty coreTools allowlists, - // so pin both as disabled under `[]` (#10065). - expect(await pm.isToolEnabled('structured_output')).toBe(false); - expect(await pm.isToolEnabled('mcp__server__tool')).toBe(false); - }); - - it('all-empty-string coreTools collapses to the empty allowlist, not a [""] allowlist', async () => { - // `"core": [""]` (e.g. an unset-variable expansion written into - // settings) carries no tool name; it must behave exactly like the - // explicit `[]` — `isCoreToolsAllowListEmpty()` true and every tool - // rejected — instead of a size-1 allowlist matching nothing that - // silently disables tools while neither diagnostic fires (#10065). - pm = new PermissionManager(makeConfig({ coreTools: [''] })); - pm.initialize(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(true); - expect(await pm.isToolEnabled('read_file')).toBe(false); - expect(await pm.isToolEnabled('agent')).toBe(false); - expect(await pm.isToolEnabled('structured_output')).toBe(false); - - // Whitespace-only entries carry no name either. - pm = new PermissionManager(makeConfig({ coreTools: [' '] })); - pm.initialize(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(true); - - // A list still naming a tool keeps its non-empty semantic. - pm = new PermissionManager(makeConfig({ coreTools: ['', 'read_file'] })); - pm.initialize(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(false); - expect(await pm.isToolEnabled('read_file')).toBe(true); - }); - - it('coreTools entries without a usable parsed name collapse to the empty allowlist', async () => { - // 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. They must be - // dropped exactly like empty strings — collapsing such a list to the - // explicit empty allowlist with both #10065 diagnostics — instead of - // forming a size-1 `{''}` allowlist that matches nothing while - // `isCoreToolsAllowListEmpty()` stays false and every core tool is - // silently disabled (#10080). - for (const coreTools of [['()'], ['(ls -l)'], ['Bash(ls'], ['', '()']]) { - pm = new PermissionManager(makeConfig({ coreTools })); - pm.initialize(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(true); - expect(await pm.isToolEnabled('read_file')).toBe(false); - expect(await pm.isToolEnabled('mcp__server__tool')).toBe(false); - } - - // A list still naming a real tool keeps its non-empty semantic even - // when mixed with name-less entries. - pm = new PermissionManager( - makeConfig({ coreTools: ['()', 'read_file'] }), - ); - pm.initialize(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(false); - expect(await pm.isToolEnabled('read_file')).toBe(true); - }); - - it('non-string coreTools entries are dropped without throwing', async () => { - // Settings loading performs no element validation (the schema - // declares only `type: 'array'`), so a hand-edited `"core": [42]` - // — or a mixed `["read_file", 42]` — reaches `initialize()` raw. - // The `typeof t === 'string'` half of the filter must drop the - // garbage instead of letting `parseRule(42)` throw while the - // allowlist is built (#10080). - pm = new PermissionManager( - makeConfig({ coreTools: [42 as unknown as string] }), - ); - expect(() => pm.initialize()).not.toThrow(); - // An all-non-string list names nothing, so it collapses to the - // explicit empty allowlist exactly like `[]` / `[""]` — both - // #10065 diagnostics stay reachable. - expect(pm.isCoreToolsAllowListEmpty()).toBe(true); - expect(await pm.isToolEnabled('read_file')).toBe(false); - - // A mixed list keeps its named tool and drops the garbage entry. - pm = new PermissionManager( - makeConfig({ coreTools: ['read_file', 42 as unknown as string] }), - ); - expect(() => pm.initialize()).not.toThrow(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(false); - expect(await pm.isToolEnabled('read_file')).toBe(true); - expect(await pm.isToolEnabled('run_shell_command')).toBe(false); - }); - - it('undefined coreTools keeps tools enabled', async () => { - pm = new PermissionManager(makeConfig()); - pm.initialize(); expect(await pm.isToolEnabled('read_file')).toBe(true); expect(await pm.isToolEnabled('run_shell_command')).toBe(true); - expect(await pm.isToolEnabled('agent')).toBe(true); - }); - - it('null or non-array coreTools is unrestricted, not an empty allowlist', async () => { - // `"tools": { "core": null }` is the common JSON idiom for clearing - // the deprecated key, and settings loading performs no type - // validation, so null (or any non-array value) reaches - // `initialize()` raw. It must not throw and must behave like "no - // allowlist", not like the tool-disabling explicit `[]`. - pm = new PermissionManager( - makeConfig({ coreTools: null as unknown as string[] }), - ); - expect(() => pm.initialize()).not.toThrow(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(false); - expect(await pm.isToolEnabled('read_file')).toBe(true); - expect(await pm.isToolEnabled('agent')).toBe(true); - - pm = new PermissionManager( - makeConfig({ coreTools: {} as unknown as string[] }), - ); - expect(() => pm.initialize()).not.toThrow(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(false); - expect(await pm.isToolEnabled('read_file')).toBe(true); }); it('coreTools allowlist + deny rule: deny takes precedence for listed tools', async () => { @@ -2952,8 +2833,7 @@ describe('PermissionManager', () => { it('an explicitly empty list is active and defers everything', async () => { // `[]` is an active allowlist that names nothing; `tools.core` differs - // only in effect: its explicitly empty list stays active and disables - // every tool instead of deferring (#10065). + // because its empty list is treated as unset. // This is the gentler answer for constrained-decoding backends: the // eager request carries almost no tool schemas, but every tool is // still registered and reachable via ToolSearch. @@ -4416,23 +4296,3 @@ describe('matchesRule — param matcher type guards', () => { ).toBe(true); }); }); -describe('isNamelessCoreToolsEntry', () => { - // The helper is the single shared classification behind both the gate in - // PermissionManager.initialize() and the CLI startup notice; pin it - // directly so either call site rewinding to a private inline predicate - // cannot hide behind the other (#10080). - it('classifies non-strings, blank strings, and unparseable entries as name-less', () => { - expect(isNamelessCoreToolsEntry(123)).toBe(true); - expect(isNamelessCoreToolsEntry('')).toBe(true); - expect(isNamelessCoreToolsEntry(' ')).toBe(true); - expect(isNamelessCoreToolsEntry('()')).toBe(true); - expect(isNamelessCoreToolsEntry('(ls -l)')).toBe(true); - expect(isNamelessCoreToolsEntry('Bash(ls')).toBe(true); - }); - - it('classifies entries naming a tool as usable', () => { - expect(isNamelessCoreToolsEntry('read_file')).toBe(false); - expect(isNamelessCoreToolsEntry('Bash(npm test)')).toBe(false); - expect(isNamelessCoreToolsEntry(' mcp__server__tool ')).toBe(false); - }); -}); diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index 0f7d99a2561..7a01171af02 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -51,9 +51,8 @@ const debugLogger = createDebugLogger('PERMISSIONS'); * built-in tools not named in an active `settings.tools.eager` * allowlist: their schemas stay out of the eager request (#9827) without * the tools silently disappearing from the session (#10075). - * - `disabled`: not registered at all (whole-tool deny rule, or excluded - * by the legacy `coreTools` allowlist — unlisted, or any tool under an - * explicitly empty `tools.core: []`, #10065). + * - `disabled`: not registered at all (whole-tool deny rule, or unlisted in + * the legacy `coreTools` allowlist). */ export type ToolRegistrationStatus = 'registered' | 'deferred' | 'disabled'; @@ -123,27 +122,6 @@ export interface PermissionManagerConfig { getEagerTools?(): readonly string[] | undefined; } -/** - * Classifies a raw `tools.core` entry as name-less: it carries no usable - * tool name. Non-string entries, empty/whitespace-only strings, malformed - * rules (unbalanced parens parse to `invalid`), and entries whose tool - * part parses to the empty string (`"()"`, `"(ls -l)"`) are all name-less. - * - * This is the single shared classification behind BOTH faces of the - * empty-allowlist collapse: the gate in `PermissionManager.initialize()` - * below and the CLI startup notice in `packages/cli/src/config/config.ts`. - * They must agree on what counts as name-less, or the notice can stay - * silent for a list the gate collapses to the empty allowlist — exactly - * the silent tool-free session #10065 set out to diagnose (#10080). - */ -export function isNamelessCoreToolsEntry(entry: unknown): boolean { - if (typeof entry !== 'string' || entry.trim() === '') { - return true; - } - const rule = parseRule(entry); - return rule.invalid || rule.toolName === ''; -} - /** * Manages tool and command permissions by evaluating a set of * prioritised rules against allow / ask / deny lists. @@ -238,29 +216,10 @@ export class PermissionManager { // Each entry may be a bare name ("Bash", "read_file") or include a specifier // ("Bash(ls -l)") – we normalise to canonical tool names and ignore specifiers // because the registry check is at the tool level, not the invocation level. - // An explicitly configured empty list is still an active allowlist: it - // disables every registered tool — core and non-core (MCP, Skill, - // Agent, synthetic system tools) alike (see `isToolEnabled`). - // Only an array activates the allowlist: `undefined`, `null`, or any - // non-array value (e.g. `"tools": { "core": null }`, the common JSON - // idiom for clearing this deprecated key) means no restriction. - // Entries that carry no tool name — the empty string, whitespace-only - // strings, non-string garbage, malformed rules, and entries whose tool - // part parses to the empty string (`"()"`, `"(ls -l)"`, e.g. an - // unset-variable or template expansion written into settings) — are - // dropped via the shared `isNamelessCoreToolsEntry` classification, so - // a list made up only of such entries (`[""]`, `["()"]`) collapses to - // the explicit EMPTY allowlist with both #10065 diagnostics instead of - // a size-1 allowlist that matches nothing and silently disables every - // tool while `isCoreToolsAllowListEmpty()` stays false (#10065). The - // CLI startup notice uses the same helper, so notice and gate cannot - // drift apart (#10080). const rawCoreTools = this.config.getCoreTools?.(); - if (Array.isArray(rawCoreTools)) { + if (rawCoreTools && rawCoreTools.length > 0) { this.coreToolsAllowList = new Set( - rawCoreTools - .filter((t) => !isNamelessCoreToolsEntry(t)) - .map((t) => parseRule(t).toolName), + rawCoreTools.map((t) => parseRule(t).toolName), ); } @@ -277,8 +236,7 @@ export class PermissionManager { // activates it: `undefined`, `null`, or any non-array value means no // restriction, while an explicitly empty array is an active allowlist // that names nothing and therefore defers every non-exempt tool. - // `tools.core` differs: its explicitly empty list stays active and - // disables every tool instead of deferring (#10065). + // `tools.core` differs: its empty list is treated as unset. // // Entries are parsed with the same rule parser the permission rules // use so alias forms (`ListFiles`) and stray specifiers @@ -708,11 +666,8 @@ export class PermissionManager { * `structured_output` (registered only when `--json-schema` is set). * Excluding `structured_output` from `--core-tools` would leave a * `--json-schema` run with no terminal contract, so the synthetic - * tool stays available regardless of a NON-EMPTY allowlist (deny - * rules still apply). An explicitly EMPTY allowlist (`tools.core: []`) - * is the deliberate "no tools at all" configuration and disables even - * these synthetic tools, so a `--json-schema` run under it cannot - * complete (#10065). + * tool stays available regardless of the allowlist (deny rules still + * apply). */ private static readonly CORE_TOOLS = new Set([ 'read_file', @@ -768,11 +723,8 @@ export class PermissionManager { * is still registered — it is merely hidden from the eager model request * and loadable via ToolSearch — so a call to it must flow through the * normal approval evaluation, not a permission error (#10075). Only - * `disabled` tools return `false`: a whole-tool deny rule, or the legacy - * `coreTools` allowlist — a core tool missing from a non-empty list, or - * EVERY tool (non-core included) when the list is explicitly empty - * (`tools.core: []`, the deliberate "no tools at all" configuration, - * #10065). + * `disabled` tools (whole-tool deny rule, or unlisted in the legacy + * `coreTools` allowlist) return `false`. * * Specifier-based deny rules such as `"Bash(rm -rf *)"` never disable the * tool — they only deny specific invocations at runtime. Likewise, @@ -780,14 +732,12 @@ export class PermissionManager { * for allowlist membership — the allowlist is tool-level, not * invocation-level. * - * Non-core tools (MCP, Skill, Agent, etc.) skip the NON-EMPTY coreTools - * allowlist check because they are dynamically discovered or essential - * for system operation (an explicitly empty allowlist still disables - * them, see {@link isToolDisabledByCoreToolsAllowList}). The - * `settings.tools.eager` allowlist does apply to them (except the - * exempt families, see {@link getToolRegistrationStatus}), which is - * how e.g. `send_message` / `update_goal` schemas are kept out of the - * eager model request (#9827) — but it only ever demotes them to + * Non-core tools (MCP, Skill, Agent, etc.) skip the coreTools allowlist + * check because they are dynamically discovered or essential for system + * operation. The `settings.tools.eager` allowlist does apply to them + * (except the exempt families, see {@link getToolRegistrationStatus}), + * which is how e.g. `send_message` / `update_goal` schemas are kept out + * of the eager model request (#9827) — but it only ever demotes them to * `deferred`, so this method still reports them enabled. */ async isToolEnabled(toolName: string): Promise { @@ -800,25 +750,13 @@ export class PermissionManager { * demotes unlisted tools to `deferred` — the legacy coreTools knob keeps * its documented hard-disable semantic: an unlisted core tool is not * registered at all. - * - * An explicitly EMPTY allowlist (`tools.core: []`) is the deliberate - * "no tools at all" configuration and excludes EVERY tool — non-core - * tools (MCP, Skill, Agent, synthetic system tools) included — so the - * documented empty allowlist yields an actual tool-free session instead - * of silently sending every schema (#10065). Only a NON-EMPTY allowlist - * keeps the historical non-core bypass. `undefined`/`null`/non-array - * coreTools mean no restriction (see `initialize()`). */ isToolDisabledByCoreToolsAllowList(toolName: string): boolean { - if (this.coreToolsAllowList === null) { - return false; - } - if (this.isCoreToolsAllowListEmpty()) { - return true; - } const canonicalName = resolveToolName(toolName); return ( this.isCoreTool(canonicalName) && + this.coreToolsAllowList !== null && + this.coreToolsAllowList.size > 0 && !this.coreToolsAllowList.has(canonicalName) ); } @@ -905,9 +843,7 @@ export class PermissionManager { * (deny always wins over eager-allowlist membership), or the legacy * `coreTools` allowlist, whose documented semantic is hard exclusion * and which — unlike `tools.eager` — predates the deferred demotion and - * is set deliberately (#9827). That gate runs BEFORE the allowlist - * exemptions so an explicitly empty `tools.core: []` disables even the - * exempt families (MCP, `structured_output`, ...) (#10065). + * is set deliberately (#9827). */ async getToolRegistrationStatus( toolName: string, @@ -923,10 +859,7 @@ export class PermissionManager { return 'disabled'; } - // The legacy coreTools allowlist keeps its hard-disable semantic — and - // an explicitly empty one disables every tool, exempt families - // included, so `tools.core: []` is an actual tool-free configuration - // rather than silently sending every schema (#10065). + // The legacy coreTools allowlist keeps its hard-disable semantic. if (this.isToolDisabledByCoreToolsAllowList(canonicalName)) { return 'disabled'; } @@ -953,26 +886,6 @@ export class PermissionManager { return this.eagerToolAllowList !== null; } - /** - * Whether the coreTools allowlist is explicitly empty (`tools.core: []` - * in settings). A bare/valueless `--core-tools` never produces this - * state — `loadCliConfig` treats an argv-sourced empty list as absent — - * and neither do `null`/non-array values, which `initialize()` treats - * as no restriction; an all-empty-string list (`[""]`) does, because - * name-less entries are dropped when the allowlist is built (#10065). - * When true, `isToolEnabled()` rejects EVERY tool — - * core and non-core alike — so user-facing remediation advice must name - * this knob instead of promising re-enablement through other gates - * (e.g. adding a `permissions.allow` rule can never satisfy an empty - * coreTools allowlist, #10065). `undefined`/`null`/non-array coreTools - * (no restriction) and non-empty allowlists both return false. - */ - isCoreToolsAllowListEmpty(): boolean { - return ( - this.coreToolsAllowList !== null && this.coreToolsAllowList.size === 0 - ); - } - /** * Find the first deny rule that matches the given context. * Returns the raw rule string if found, or undefined if no deny rule matches. diff --git a/packages/core/src/tools/tool-registry.test.ts b/packages/core/src/tools/tool-registry.test.ts index 5cf8481d5b6..2efe0178fd9 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -161,57 +161,6 @@ describe('ToolRegistry', () => { expect(toolRegistry.getTool('mock-tool')).toBe(tool); }); - it('skips registration when tools.core is an explicitly empty allowlist (#10065)', () => { - // Built-ins are gated at `registerLazy` and command-discovered tools - // at the async discovery gate, but MCP-discovered tools flow only - // through `registerTool` (both the legacy `McpClient.discover()` - // path and the pooled `SessionMcpView.applyTools` path end here). - // Without this guard `tools.core: []` would keep them registered — - // advertised to the model, then every invocation rejected at the - // runtime gate. - const pm = new PermissionManager({ - getPermissionsAllow: () => [], - getPermissionsAsk: () => [], - getPermissionsDeny: () => [], - getCoreTools: () => [], - getEagerTools: () => [], - getProjectRoot: () => '/test/dir', - getCwd: () => '/test/dir', - getApprovalMode: () => 'default', - }); - pm.initialize(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(true); - vi.spyOn(config, 'getPermissionManager').mockReturnValue(pm); - - const tool = new MockTool({ name: 'mcp__srv__foo' }); - toolRegistry.registerTool(tool); - - expect(toolRegistry.getTool('mcp__srv__foo')).toBeUndefined(); - }); - - it('still registers tools when the coreTools allowlist is unset (#10065)', () => { - // `undefined` coreTools means "no restriction" — the empty-allowlist - // guard must not fire for it. - const pm = new PermissionManager({ - getPermissionsAllow: () => [], - getPermissionsAsk: () => [], - getPermissionsDeny: () => [], - getCoreTools: () => undefined, - getEagerTools: () => [], - getProjectRoot: () => '/test/dir', - getCwd: () => '/test/dir', - getApprovalMode: () => 'default', - }); - pm.initialize(); - expect(pm.isCoreToolsAllowListEmpty()).toBe(false); - vi.spyOn(config, 'getPermissionManager').mockReturnValue(pm); - - const tool = new MockTool({ name: 'mcp__srv__foo' }); - toolRegistry.registerTool(tool); - - expect(toolRegistry.getTool('mcp__srv__foo')).toBe(tool); - }); - it('renames an MCP tool whose name shadows a registered lazy factory', async () => { // The synthetic `structured_output` tool registers via // `registerFactory` (lazy). Without this guard, an MCP server diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 2351097898f..65b77409697 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -298,30 +298,6 @@ export class ToolRegistry { ); return; } - // An explicitly empty coreTools allowlist (`tools.core: []`) disables - // every tool. Built-ins are already skipped at `registerLazy` - // (config.ts) and command-discovered tools at the async - // `isToolEnabled` gate below in `discoverAndRegisterToolsFromCommand`, - // but MCP-discovered tools only flow through `registerTool` — both the - // legacy `McpClient.discover()` path and the pooled - // `SessionMcpView.applyTools` path end here. Without this guard they - // would stay registered and advertised under `[]`, then every call - // would be rejected at the runtime gate: advertised-then-rejected, - // the exact failure mode the empty allowlist is meant to remove - // (#10065). The optional call keeps scoped PermissionManager shims - // (installed via `as unknown as PermissionManager`) from throwing - // until they grow the method. - const pmForEmptyGate = this.config.getPermissionManager?.(); - if ( - pmForEmptyGate && - typeof pmForEmptyGate.isCoreToolsAllowListEmpty === 'function' && - pmForEmptyGate.isCoreToolsAllowListEmpty() - ) { - debugLogger.info( - `Tool "${tool.name}" skipped: tools.core is an explicitly empty allowlist (#10065).`, - ); - return; - } // A name collision can happen against either the eager `tools` map // (already-instantiated tools) or the lazy `factories` map (registered // but not yet constructed — `structured_output` lives here when diff --git a/packages/core/src/utils/schemaConverter.test.ts b/packages/core/src/utils/schemaConverter.test.ts index f3acc3010c0..34f0b19e365 100644 --- a/packages/core/src/utils/schemaConverter.test.ts +++ b/packages/core/src/utils/schemaConverter.test.ts @@ -335,11 +335,11 @@ describe('relaxSchemaForFunctionCalling', () => { ).toBe(false); }); - it('keeps additionalProperties:false when there are no properties to promote', () => { - const empty = { type: 'object', additionalProperties: false }; - expect(relaxSchemaForFunctionCalling(empty)['additionalProperties']).toBe( - false, - ); + it('keeps additionalProperties:false when no empty properties map is declared', () => { + const schema = { type: 'object', additionalProperties: false }; + expect( + relaxSchemaForFunctionCalling(schema)['additionalProperties'], + ).toBe(false); }); it('relaxes nested object levels independently', () => { @@ -503,6 +503,50 @@ describe('relaxSchemaForFunctionCalling', () => { }); }); + it('removes grammar-hostile empty objects and repetition limits', () => { + const schema = { + type: 'object', + properties: { + empty: { + type: 'object', + properties: {}, + additionalProperties: false, + }, + bounded: { type: 'string', maxLength: 1999 }, + long: { type: 'string', maxLength: 2000 }, + padded: { type: 'string', minLength: 2000 }, + many: { + type: 'array', + maxItems: 2000, + items: { type: 'string' }, + }, + largeBatch: { + type: 'array', + minItems: 2000, + items: { type: 'string' }, + }, + }, + }; + + expect(relaxSchemaForFunctionCalling(schema)).toEqual({ + type: 'object', + properties: { + empty: { type: 'object' }, + bounded: { type: 'string', maxLength: 1999 }, + long: { type: 'string' }, + padded: { type: 'string' }, + many: { + type: 'array', + items: { type: 'string' }, + }, + largeBatch: { + type: 'array', + items: { type: 'string' }, + }, + }, + }); + }); + it('never treats schema-map keys as schema keywords', () => { const namedUniqueItems = { uniqueItems: { type: 'string' } }; const schema = { diff --git a/packages/core/src/utils/schemaConverter.ts b/packages/core/src/utils/schemaConverter.ts index db351a89040..e3e1f3aa190 100644 --- a/packages/core/src/utils/schemaConverter.ts +++ b/packages/core/src/utils/schemaConverter.ts @@ -191,14 +191,17 @@ function toOpenAPI30(schema: Record): Record { * client-side validation error until loop detection kills the run. * * The relaxation is deliberately surgical: - * - `additionalProperties: false` is removed ONLY on object levels that - * declare optional properties (some `properties` key missing from - * `required`). Levels where every property is required keep the - * constraint — there is nothing for a gateway to promote. + * - `additionalProperties: false` is removed on object levels that declare + * optional properties (some `properties` key missing from `required`) or + * an empty `properties` map. Levels where every property is required keep + * the constraint — there is nothing for a gateway to promote. * - `$schema` / `$id` metadata is dropped at every schema level (some * gateways reject unknown keywords). * - `uniqueItems` is dropped at every schema level because some * OpenAI-compatible function-calling endpoints reject it. + * - Empty object declarations and string / array length limits at or above + * 2000 are dropped because grammar-based endpoints can turn them into + * invalid or rejected repetition rules. * - Other constraints pass through untouched; client-side * `validateToolParams` still enforces the full source schema, so the * constraint is relaxed on the wire only. @@ -229,15 +232,33 @@ export function relaxSchemaForFunctionCalling( properties !== null && !Array.isArray(properties) && Object.keys(properties).some((key) => !required.includes(key)); + const hasEmptyProperties = + typeof properties === 'object' && + properties !== null && + !Array.isArray(properties) && + Object.keys(properties).length === 0; for (const [key, value] of Object.entries(source)) { if (key === '$schema' || key === '$id' || key === 'uniqueItems') { continue; } + if (key === 'properties' && hasEmptyProperties) { + continue; + } + if ( + (key === 'minLength' || + key === 'maxLength' || + key === 'minItems' || + key === 'maxItems') && + typeof value === 'number' && + value >= 2000 + ) { + continue; + } if ( key === 'additionalProperties' && value === false && - hasOptionalProperties + (hasOptionalProperties || hasEmptyProperties) ) { continue; } diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index 7c76add97e9..963bf8754ef 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -1297,7 +1297,7 @@ } }, "core": { - "description": "Deprecated. permissions.allow cannot reproduce this registration restriction because it only auto-approves calls. Use tools.eager to defer unlisted eager-by-default tools or permissions.deny to remove tools. An explicitly empty list ([]) is an active allowlist that disables every tool; omit the key or set it to null for no restriction (#10065).", + "description": "Deprecated. permissions.allow cannot reproduce this registration restriction because it only auto-approves calls. Use tools.eager to defer unlisted eager-by-default tools or permissions.deny to remove tools. An empty list is treated as unset and disables nothing.", "type": "array", "items": { "type": "string" From 75d7a664f1b38c51b97341baded76cfa6169d4ef Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 29 Aug 2026 07:54:59 +0800 Subject: [PATCH 18/29] fix(core): relax closed zero-property schemas Co-authored-by: Qwen-Coder --- docs/design/openai-tool-schema-grammar-compatibility.md | 2 +- packages/core/src/utils/schemaConverter.test.ts | 8 ++++---- packages/core/src/utils/schemaConverter.ts | 9 ++++++--- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md index 4976daf444f..c87ab0c3496 100644 --- a/docs/design/openai-tool-schema-grammar-compatibility.md +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -10,7 +10,7 @@ Disabling all tools avoids grammar construction but also removes the functionali Keep the registered tool set unchanged. Before an OpenAI-compatible request is sent, recursively relax only the wire copy of each schema: -- omit empty `properties` maps and the same level's `additionalProperties: false` constraint; +- omit empty `properties` maps and `additionalProperties: false` on objects with zero declared properties; - omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above the grammar repetition boundary of 2000; - preserve smaller limits and all other supported constraints. diff --git a/packages/core/src/utils/schemaConverter.test.ts b/packages/core/src/utils/schemaConverter.test.ts index 34f0b19e365..85fea007a62 100644 --- a/packages/core/src/utils/schemaConverter.test.ts +++ b/packages/core/src/utils/schemaConverter.test.ts @@ -335,11 +335,9 @@ describe('relaxSchemaForFunctionCalling', () => { ).toBe(false); }); - it('keeps additionalProperties:false when no empty properties map is declared', () => { + it('removes additionalProperties:false when no properties are declared', () => { const schema = { type: 'object', additionalProperties: false }; - expect( - relaxSchemaForFunctionCalling(schema)['additionalProperties'], - ).toBe(false); + expect(relaxSchemaForFunctionCalling(schema)).toEqual({ type: 'object' }); }); it('relaxes nested object levels independently', () => { @@ -512,6 +510,7 @@ describe('relaxSchemaForFunctionCalling', () => { properties: {}, additionalProperties: false, }, + closed: { type: 'object', additionalProperties: false }, bounded: { type: 'string', maxLength: 1999 }, long: { type: 'string', maxLength: 2000 }, padded: { type: 'string', minLength: 2000 }, @@ -532,6 +531,7 @@ describe('relaxSchemaForFunctionCalling', () => { type: 'object', properties: { empty: { type: 'object' }, + closed: { type: 'object' }, bounded: { type: 'string', maxLength: 1999 }, long: { type: 'string' }, padded: { type: 'string' }, diff --git a/packages/core/src/utils/schemaConverter.ts b/packages/core/src/utils/schemaConverter.ts index e3e1f3aa190..c3fc9e66ce2 100644 --- a/packages/core/src/utils/schemaConverter.ts +++ b/packages/core/src/utils/schemaConverter.ts @@ -193,8 +193,8 @@ function toOpenAPI30(schema: Record): Record { * The relaxation is deliberately surgical: * - `additionalProperties: false` is removed on object levels that declare * optional properties (some `properties` key missing from `required`) or - * an empty `properties` map. Levels where every property is required keep - * the constraint — there is nothing for a gateway to promote. + * no declared properties. Levels where every property is required keep the + * constraint — there is nothing for a gateway to promote. * - `$schema` / `$id` metadata is dropped at every schema level (some * gateways reject unknown keywords). * - `uniqueItems` is dropped at every schema level because some @@ -237,6 +237,9 @@ export function relaxSchemaForFunctionCalling( properties !== null && !Array.isArray(properties) && Object.keys(properties).length === 0; + const hasNoDeclaredProperties = + hasEmptyProperties || + (source['type'] === 'object' && properties === undefined); for (const [key, value] of Object.entries(source)) { if (key === '$schema' || key === '$id' || key === 'uniqueItems') { @@ -258,7 +261,7 @@ export function relaxSchemaForFunctionCalling( if ( key === 'additionalProperties' && value === false && - (hasOptionalProperties || hasEmptyProperties) + (hasOptionalProperties || hasNoDeclaredProperties) ) { continue; } From 117b1c6f2b4c35a5be577cddb92b046a7557aa19 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 29 Aug 2026 07:56:47 +0800 Subject: [PATCH 19/29] fix(core): cover object-capable empty schemas Co-authored-by: Qwen-Coder --- .../openai-tool-schema-grammar-compatibility.md | 2 +- packages/core/src/utils/schemaConverter.test.ts | 12 +++++++----- packages/core/src/utils/schemaConverter.ts | 8 ++++++-- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md index c87ab0c3496..0a0e1adaf9c 100644 --- a/docs/design/openai-tool-schema-grammar-compatibility.md +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -10,7 +10,7 @@ Disabling all tools avoids grammar construction but also removes the functionali Keep the registered tool set unchanged. Before an OpenAI-compatible request is sent, recursively relax only the wire copy of each schema: -- omit empty `properties` maps and `additionalProperties: false` on objects with zero declared properties; +- omit empty `properties` maps and `additionalProperties: false` on object-capable schemas with zero declared properties; - omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above the grammar repetition boundary of 2000; - preserve smaller limits and all other supported constraints. diff --git a/packages/core/src/utils/schemaConverter.test.ts b/packages/core/src/utils/schemaConverter.test.ts index 85fea007a62..d05b986b0d2 100644 --- a/packages/core/src/utils/schemaConverter.test.ts +++ b/packages/core/src/utils/schemaConverter.test.ts @@ -335,11 +335,6 @@ describe('relaxSchemaForFunctionCalling', () => { ).toBe(false); }); - it('removes additionalProperties:false when no properties are declared', () => { - const schema = { type: 'object', additionalProperties: false }; - expect(relaxSchemaForFunctionCalling(schema)).toEqual({ type: 'object' }); - }); - it('relaxes nested object levels independently', () => { const nested = { type: 'object', @@ -511,6 +506,11 @@ describe('relaxSchemaForFunctionCalling', () => { additionalProperties: false, }, closed: { type: 'object', additionalProperties: false }, + typelessClosed: { additionalProperties: false }, + nullableClosed: { + type: ['object', 'null'], + additionalProperties: false, + }, bounded: { type: 'string', maxLength: 1999 }, long: { type: 'string', maxLength: 2000 }, padded: { type: 'string', minLength: 2000 }, @@ -532,6 +532,8 @@ describe('relaxSchemaForFunctionCalling', () => { properties: { empty: { type: 'object' }, closed: { type: 'object' }, + typelessClosed: {}, + nullableClosed: { type: ['object', 'null'] }, bounded: { type: 'string', maxLength: 1999 }, long: { type: 'string' }, padded: { type: 'string' }, diff --git a/packages/core/src/utils/schemaConverter.ts b/packages/core/src/utils/schemaConverter.ts index c3fc9e66ce2..a8cc391fa7d 100644 --- a/packages/core/src/utils/schemaConverter.ts +++ b/packages/core/src/utils/schemaConverter.ts @@ -237,9 +237,13 @@ export function relaxSchemaForFunctionCalling( properties !== null && !Array.isArray(properties) && Object.keys(properties).length === 0; + const type = source['type']; + const canBeObject = + type === undefined || + type === 'object' || + (Array.isArray(type) && type.includes('object')); const hasNoDeclaredProperties = - hasEmptyProperties || - (source['type'] === 'object' && properties === undefined); + hasEmptyProperties || (canBeObject && properties === undefined); for (const [key, value] of Object.entries(source)) { if (key === '$schema' || key === '$id' || key === 'uniqueItems') { From 87a39979a663e83473a816d87e25b2e4cc0a144b Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 29 Aug 2026 08:23:34 +0800 Subject: [PATCH 20/29] fix(core): guard schema relaxation with local validation Co-authored-by: Qwen-Coder --- ...penai-tool-schema-grammar-compatibility.md | 2 + .../openaiContentGenerator/converter.test.ts | 62 +++++++++++++++++++ .../core/openaiContentGenerator/converter.ts | 9 ++- .../core/src/utils/schemaConverter.test.ts | 2 +- packages/core/src/utils/schemaConverter.ts | 17 +++-- packages/core/src/utils/schemaValidator.ts | 18 ++++++ 6 files changed, 103 insertions(+), 7 deletions(-) diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md index 0a0e1adaf9c..d4f17a9b6f0 100644 --- a/docs/design/openai-tool-schema-grammar-compatibility.md +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -14,6 +14,8 @@ Keep the registered tool set unchanged. Before an OpenAI-compatible request is s - omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above the grammar repetition boundary of 2000; - preserve smaller limits and all other supported constraints. +Apply these grammar-specific relaxations only when the source schema compiles with the same validator used before tool execution. If local validation cannot enforce the source schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. + The original schema remains attached to the tool and continues to drive client-side parameter validation. The provider receives a schema it can compile, while Qwen Code still rejects tool calls that violate the original limits. ## Compatibility diff --git a/packages/core/src/core/openaiContentGenerator/converter.test.ts b/packages/core/src/core/openaiContentGenerator/converter.test.ts index 9ca188e8ef5..85cbd98919e 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.test.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.test.ts @@ -5895,6 +5895,68 @@ describe('OpenAIContentConverter', () => { expect(parametersJsonSchema.properties.blockedBy.uniqueItems).toBe(true); }); + it('only relaxes grammar constraints backed by local validation', async () => { + const supportedSchema = { + type: 'object', + properties: {}, + additionalProperties: false, + }; + const unsupportedSchema = { + $schema: 'https://json-schema.org/draft/2019-09/schema', + type: 'object', + properties: {}, + additionalProperties: false, + }; + const tools = [ + { + functionDeclarations: [ + { name: 'supported', parametersJsonSchema: supportedSchema }, + { name: 'unsupported', parametersJsonSchema: unsupportedSchema }, + { + name: 'without_local_schema', + parameters: { + type: Type.OBJECT, + properties: {}, + additionalProperties: false, + }, + }, + ], + }, + ] as Tool[]; + + const result = await converter.convertLlmToolsToOpenAI(tools); + + expect(result.map(({ function: declaration }) => declaration)).toEqual([ + { name: 'supported', description: '', parameters: { type: 'object' } }, + { + name: 'unsupported', + description: '', + parameters: { + type: 'object', + properties: {}, + additionalProperties: false, + }, + }, + { + name: 'without_local_schema', + description: '', + parameters: { + type: 'object', + properties: {}, + additionalProperties: false, + }, + }, + ]); + expect(supportedSchema).toEqual({ + type: 'object', + properties: {}, + additionalProperties: false, + }); + expect(unsupportedSchema.$schema).toBe( + 'https://json-schema.org/draft/2019-09/schema', + ); + }); + it('should convert Gemini tools with parameters field', async () => { const llmTools = [ { diff --git a/packages/core/src/core/openaiContentGenerator/converter.ts b/packages/core/src/core/openaiContentGenerator/converter.ts index 225d3d42209..d3ab71d9004 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.ts @@ -41,6 +41,7 @@ import { import { InvalidStreamError } from '../invalid-stream-error.js'; import { normalizeMcpToolName } from '../../utils/tool-name-utils.js'; import { setGenAiUsageProvenance } from '../../telemetry/gen-ai-usage.js'; +import { SchemaValidator } from '../../utils/schemaValidator.js'; const debugLogger = createDebugLogger('CONVERTER'); const SPLIT_TOOL_MEDIA_TEXT = '(attached media from previous tool call)'; @@ -370,6 +371,9 @@ export async function convertLlmToolsToOpenAI( } if (parameters) { + const canValidateLocally = + func.parametersJsonSchema !== undefined && + SchemaValidator.canCompile(func.parametersJsonSchema); parameters = convertSchema(parameters, schemaCompliance); // #7315: gateways enforcing OpenAI's structured-output contract // promote every property to required when an object level has @@ -377,7 +381,10 @@ export async function convertLlmToolsToOpenAI( // mutually exclusive optional fields (Agent working_dir vs // isolation). Relax the wire schema; client-side // validateToolParams still enforces the source schema. - parameters = relaxSchemaForFunctionCalling(parameters); + parameters = relaxSchemaForFunctionCalling( + parameters, + canValidateLocally, + ); } openAITools.push({ diff --git a/packages/core/src/utils/schemaConverter.test.ts b/packages/core/src/utils/schemaConverter.test.ts index d05b986b0d2..0e110974e6f 100644 --- a/packages/core/src/utils/schemaConverter.test.ts +++ b/packages/core/src/utils/schemaConverter.test.ts @@ -527,7 +527,7 @@ describe('relaxSchemaForFunctionCalling', () => { }, }; - expect(relaxSchemaForFunctionCalling(schema)).toEqual({ + expect(relaxSchemaForFunctionCalling(schema, true)).toEqual({ type: 'object', properties: { empty: { type: 'object' }, diff --git a/packages/core/src/utils/schemaConverter.ts b/packages/core/src/utils/schemaConverter.ts index a8cc391fa7d..57a2c17777f 100644 --- a/packages/core/src/utils/schemaConverter.ts +++ b/packages/core/src/utils/schemaConverter.ts @@ -199,9 +199,9 @@ function toOpenAPI30(schema: Record): Record { * gateways reject unknown keywords). * - `uniqueItems` is dropped at every schema level because some * OpenAI-compatible function-calling endpoints reject it. - * - Empty object declarations and string / array length limits at or above - * 2000 are dropped because grammar-based endpoints can turn them into - * invalid or rejected repetition rules. + * - When the source schema can be validated locally, empty object declarations + * and string / array length limits at or above 2000 are dropped because + * grammar-based endpoints can turn them into invalid or rejected rules. * - Other constraints pass through untouched; client-side * `validateToolParams` still enforces the full source schema, so the * constraint is relaxed on the wire only. @@ -210,6 +210,7 @@ function toOpenAPI30(schema: Record): Record { */ export function relaxSchemaForFunctionCalling( schema: Record, + relaxGrammarConstraints = false, ): Record { const relax = (obj: unknown): unknown => { if (typeof obj !== 'object' || obj === null) { @@ -249,10 +250,15 @@ export function relaxSchemaForFunctionCalling( if (key === '$schema' || key === '$id' || key === 'uniqueItems') { continue; } - if (key === 'properties' && hasEmptyProperties) { + if ( + relaxGrammarConstraints && + key === 'properties' && + hasEmptyProperties + ) { continue; } if ( + relaxGrammarConstraints && (key === 'minLength' || key === 'maxLength' || key === 'minItems' || @@ -265,7 +271,8 @@ export function relaxSchemaForFunctionCalling( if ( key === 'additionalProperties' && value === false && - (hasOptionalProperties || hasNoDeclaredProperties) + (hasOptionalProperties || + (relaxGrammarConstraints && hasNoDeclaredProperties)) ) { continue; } diff --git a/packages/core/src/utils/schemaValidator.ts b/packages/core/src/utils/schemaValidator.ts index 06af19c900c..8f93e4de3ea 100644 --- a/packages/core/src/utils/schemaValidator.ts +++ b/packages/core/src/utils/schemaValidator.ts @@ -89,6 +89,24 @@ function getValidator(schema: AnySchema): Ajv { * Supports both draft-07 (default) and draft-2020-12 schemas. */ export class SchemaValidator { + static canCompile(schema: unknown): boolean { + if ( + (typeof schema !== 'object' || + schema === null || + Array.isArray(schema)) && + typeof schema !== 'boolean' + ) { + return false; + } + + try { + getValidator(schema as AnySchema).compile(schema as AnySchema); + return true; + } catch { + return false; + } + } + /** * Strictly compiles a schema. Returns an error message if the schema is * malformed or uses unsupported draft/features for our Ajv configuration From 60d4157a2fa18148a65fe62a66159ae1872c3faf Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Sat, 29 Aug 2026 09:03:00 +0800 Subject: [PATCH 21/29] fix(core): require enforceable schemas before relaxation Co-authored-by: Qwen-Coder --- ...penai-tool-schema-grammar-compatibility.md | 2 +- .../openaiContentGenerator/converter.test.ts | 28 +++++++++++ .../core/openaiContentGenerator/converter.ts | 2 +- .../core/src/utils/schemaValidator.test.ts | 31 ++++++++++++ packages/core/src/utils/schemaValidator.ts | 50 +++++++++++++------ 5 files changed, 96 insertions(+), 17 deletions(-) diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md index d4f17a9b6f0..d1d3dbaa666 100644 --- a/docs/design/openai-tool-schema-grammar-compatibility.md +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -14,7 +14,7 @@ Keep the registered tool set unchanged. Before an OpenAI-compatible request is s - omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above the grammar repetition boundary of 2000; - preserve smaller limits and all other supported constraints. -Apply these grammar-specific relaxations only when the source schema compiles with the same validator used before tool execution. If local validation cannot enforce the source schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. +Apply these grammar-specific relaxations only when the source schema both compiles with the validator used before tool execution and passes strict vocabulary compilation for that validator's selected dialect. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. The original schema remains attached to the tool and continues to drive client-side parameter validation. The provider receives a schema it can compile, while Qwen Code still rejects tool calls that violate the original limits. diff --git a/packages/core/src/core/openaiContentGenerator/converter.test.ts b/packages/core/src/core/openaiContentGenerator/converter.test.ts index 85cbd98919e..c13613fa9b5 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.test.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.test.ts @@ -5907,11 +5907,30 @@ describe('OpenAIContentConverter', () => { properties: {}, additionalProperties: false, }; + const unsupportedVocabularySchema = { + type: 'object', + properties: { + tuple: { + type: 'array', + prefixItems: [ + { + type: 'object', + properties: {}, + additionalProperties: false, + }, + ], + }, + }, + }; const tools = [ { functionDeclarations: [ { name: 'supported', parametersJsonSchema: supportedSchema }, { name: 'unsupported', parametersJsonSchema: unsupportedSchema }, + { + name: 'unsupported_vocabulary', + parametersJsonSchema: unsupportedVocabularySchema, + }, { name: 'without_local_schema', parameters: { @@ -5937,6 +5956,11 @@ describe('OpenAIContentConverter', () => { additionalProperties: false, }, }, + { + name: 'unsupported_vocabulary', + description: '', + parameters: unsupportedVocabularySchema, + }, { name: 'without_local_schema', description: '', @@ -5955,6 +5979,10 @@ describe('OpenAIContentConverter', () => { expect(unsupportedSchema.$schema).toBe( 'https://json-schema.org/draft/2019-09/schema', ); + expect( + unsupportedVocabularySchema.properties.tuple.prefixItems[0] + .additionalProperties, + ).toBe(false); }); it('should convert Gemini tools with parameters field', async () => { diff --git a/packages/core/src/core/openaiContentGenerator/converter.ts b/packages/core/src/core/openaiContentGenerator/converter.ts index d3ab71d9004..131503b1c96 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.ts @@ -373,7 +373,7 @@ export async function convertLlmToolsToOpenAI( if (parameters) { const canValidateLocally = func.parametersJsonSchema !== undefined && - SchemaValidator.canCompile(func.parametersJsonSchema); + SchemaValidator.canEnforce(func.parametersJsonSchema); parameters = convertSchema(parameters, schemaCompliance); // #7315: gateways enforcing OpenAI's structured-output contract // promote every property to required when an object level has diff --git a/packages/core/src/utils/schemaValidator.test.ts b/packages/core/src/utils/schemaValidator.test.ts index 84ad827d808..2e8a018c465 100644 --- a/packages/core/src/utils/schemaValidator.test.ts +++ b/packages/core/src/utils/schemaValidator.test.ts @@ -817,6 +817,37 @@ describe('SchemaValidator', () => { }); }); + describe('canEnforce', () => { + it('requires the runtime dialect to recognize the full vocabulary', () => { + const tupleKeywords = { + prefixItems: [{ type: 'string' }], + minItems: 1, + maxItems: 1, + items: false, + }; + + expect( + SchemaValidator.canEnforce({ + type: 'array', + ...tupleKeywords, + }), + ).toBe(false); + expect( + SchemaValidator.canEnforce({ + $schema: 'https://json-schema.org/draft/2020-12/schema', + type: 'array', + ...tupleKeywords, + }), + ).toBe(true); + expect( + SchemaValidator.canEnforce({ + type: 'object', + properties: { value: { type: 'string' } }, + }), + ).toBe(true); + }); + }); + describe('non-string to string coercion', () => { const schema = { type: 'object', diff --git a/packages/core/src/utils/schemaValidator.ts b/packages/core/src/utils/schemaValidator.ts index 8f93e4de3ea..25aefe22c06 100644 --- a/packages/core/src/utils/schemaValidator.ts +++ b/packages/core/src/utils/schemaValidator.ts @@ -46,12 +46,24 @@ const ajvOptions = { }, }; +const strictCompileOptions = { + strictSchema: true, + strictRequired: false, + strictTypes: false, + strictTuples: false, + validateFormats: false, + allowUnionTypes: true, +}; + // Draft-07 validator (default) const ajvDefault: Ajv = new AjvClass(ajvOptions); // Draft-2020-12 validator for MCP servers using rmcp const ajv2020: Ajv = new Ajv2020Class(ajvOptions); +const ajvStrictDefault: Ajv = new AjvClass(strictCompileOptions); +const ajvStrict2020: Ajv = new Ajv2020Class(strictCompileOptions); + // eslint-disable-next-line @typescript-eslint/no-explicit-any const addFormatsFunc = (addFormats as any).default || addFormats; addFormatsFunc(ajvDefault); @@ -84,23 +96,38 @@ function getValidator(schema: AnySchema): Ajv { return ajvDefault; } +function getStrictValidator(schema: AnySchema): Ajv { + return typeof schema === 'object' && + schema !== null && + '$schema' in schema && + isDraft2020Uri(schema.$schema) + ? ajvStrict2020 + : ajvStrictDefault; +} + /** * Simple utility to validate objects against JSON Schemas. * Supports both draft-07 (default) and draft-2020-12 schemas. */ export class SchemaValidator { - static canCompile(schema: unknown): boolean { + /** + * Returns true only when the runtime validator recognizes the schema's full + * vocabulary. This prevents lenient compilation from being mistaken for + * local enforcement when deciding whether wire constraints may be relaxed. + */ + static canEnforce(schema: unknown): boolean { if ( - (typeof schema !== 'object' || - schema === null || - Array.isArray(schema)) && - typeof schema !== 'boolean' + typeof schema !== 'object' || + schema === null || + Array.isArray(schema) ) { return false; } try { - getValidator(schema as AnySchema).compile(schema as AnySchema); + const anySchema = schema as AnySchema; + getStrictValidator(anySchema).compile(anySchema); + getValidator(anySchema).compile(anySchema); return true; } catch { return false; @@ -135,18 +162,11 @@ export class SchemaValidator { // or anything using a custom `format`. Keep typo detection; // tolerate the looser-but-still-spec-valid patterns users actually // ship in `--json-schema`. - const strictOptions = { - strictSchema: true, // catches unknown keywords (typos) - strictRequired: false, // allow `required` without `properties` - strictTypes: false, // allow inferred / partial type info - validateFormats: false, // unknown `format` values don't fail - allowUnionTypes: true, // type: ["a","b"] - }; const strictAjv: Ajv = isDraft2020Uri( (schema as { $schema?: unknown }).$schema, ) - ? new Ajv2020Class(strictOptions) - : new AjvClass(strictOptions); + ? new Ajv2020Class(strictCompileOptions) + : new AjvClass(strictCompileOptions); addFormatsFunc(strictAjv); try { strictAjv.compile(schema as AnySchema); From e1d34bb6a827ae56451f28dc5dca9c9e15a9f5fb Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Mon, 31 Aug 2026 01:53:26 +0800 Subject: [PATCH 22/29] fix(core): make schema compilation idempotent for $id-bearing schemas Ajv registers compiled schemas by their top-level `$id`. Because the shared Ajv instances in SchemaValidator are module-level, a second distinct schema object carrying the same `$id` (two MCP tools generated from one template, or a registry rebuilt after an MCP reconnect/refresh) made compile() throw "schema with key or id ... already exists", and the bare catch silently disabled wire-constraint relaxation and runtime validation for that schema. Set `addUsedSchema: false` on the shared runtime Ajv options (ajvOptions) and the strict compile options (strictCompileOptions) so compile() is idempotent for `$id`-bearing schemas. Adds a regression test: two distinct schemas sharing one top-level `$id` must both pass canEnforce and still enforce constraints via validate. Co-authored-by: Qwen-Coder --- .../core/src/utils/schemaValidator.test.ts | 26 +++++++++++++++++++ packages/core/src/utils/schemaValidator.ts | 11 ++++++++ 2 files changed, 37 insertions(+) diff --git a/packages/core/src/utils/schemaValidator.test.ts b/packages/core/src/utils/schemaValidator.test.ts index 2e8a018c465..6c921c45b78 100644 --- a/packages/core/src/utils/schemaValidator.test.ts +++ b/packages/core/src/utils/schemaValidator.test.ts @@ -846,6 +846,32 @@ describe('SchemaValidator', () => { }), ).toBe(true); }); + + it('enforces two distinct schemas that share one top-level $id', () => { + const sharedId = 'https://qwen-code.test/schemaValidator/duplicate-id'; + const schemaA = { + $id: sharedId, + type: 'object', + properties: { value: { type: 'string', maxLength: 10 } }, + }; + // A DISTINCT schema object reusing the same top-level $id (e.g. two + // tools generated from one template, or a registry rebuilt after an + // MCP reconnect/refresh). + const schemaB = { + $id: sharedId, + type: 'object', + properties: { value: { type: 'string', maxLength: 10 } }, + }; + + expect(SchemaValidator.canEnforce(schemaA)).toBe(true); + expect(SchemaValidator.canEnforce(schemaB)).toBe(true); + + // Validation must still enforce constraints on the rebuilt schema + // object instead of silently skipping it. + expect( + SchemaValidator.validate(schemaB, { value: 'a'.repeat(50) }), + ).not.toBeNull(); + }); }); describe('non-string to string coercion', () => { diff --git a/packages/core/src/utils/schemaValidator.ts b/packages/core/src/utils/schemaValidator.ts index 25aefe22c06..06c98fec245 100644 --- a/packages/core/src/utils/schemaValidator.ts +++ b/packages/core/src/utils/schemaValidator.ts @@ -20,6 +20,13 @@ const Ajv2020Class = (Ajv2020Pkg as any).default || Ajv2020Pkg; const debugLogger = createDebugLogger('SchemaValidator'); const ajvOptions = { + // Don't register compiled schemas by `$id`. These instances are + // module-level and shared; if a second, distinct schema object carries + // the same top-level `$id` (two MCP tools generated from one template, + // or a registry rebuilt after an MCP reconnect/refresh), Ajv would throw + // "schema with key or id ... already exists" and the bare catch would + // silently turn enforcement/relaxation off for that schema. + addUsedSchema: false, // See: https://ajv.js.org/options.html#strict-mode-options // strictSchema defaults to true and prevents use of JSON schemas that // include unrecognized keywords. The JSON schema spec specifically allows @@ -47,6 +54,10 @@ const ajvOptions = { }; const strictCompileOptions = { + // Same rationale as `ajvOptions`: keep compile() idempotent for + // `$id`-bearing schemas so a distinct schema object reusing a `$id` + // doesn't collide in the shared strict instances. + addUsedSchema: false, strictSchema: true, strictRequired: false, strictTypes: false, From 75b030aa16b39478daa69129f1f68d9824befe43 Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Mon, 31 Aug 2026 03:32:55 +0800 Subject: [PATCH 23/29] fix(core): restore $id-mediated $ref resolution while keeping compile idempotent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous round set `addUsedSchema: false` on the shared Ajv options to stop duplicate-`$id` collisions, but that also removed `$id` registration, so `$ref`s that resolve through an absolute `$id` URI no longer compile: schemas recursing through their own top-level `$id` and cross-document references throw "can't resolve reference", landing in the bare catch — validate() silently skips validation, canEnforce() returns false, and compileStrict() rejects previously-accepted recursive `--json-schema`. Keep registration enabled (Ajv's default) and instead evict a stale top-level `$id` registration before compiling on the shared module-level instances: when a DISTINCT schema object reuses an already-registered `$id` (two MCP tools from one template, or a registry rebuilt after an MCP reconnect/refresh), the old entry is removed first so compile() no longer throws "schema with key or id ... already exists". The same-object fast path is preserved: when the registered schema is the one being compiled, the registration is left untouched and Ajv's object-identity cache returns the compiled validator. compileStrict() builds a fresh instance per call, so it needs no eviction. Adds regression tests: self-recursive and cross-document `$id` refs compile and enforce (non-coercible payloads error instead of passing a silently-skipped validate), and repeated compilation of same/distinct schemas sharing one `$id` stays idempotent without cross-pollution. Co-authored-by: Qwen-Coder --- .../core/src/utils/schemaValidator.test.ts | 119 ++++++++++++++++++ packages/core/src/utils/schemaValidator.ts | 53 ++++++-- 2 files changed, 159 insertions(+), 13 deletions(-) diff --git a/packages/core/src/utils/schemaValidator.test.ts b/packages/core/src/utils/schemaValidator.test.ts index 6c921c45b78..882285e4d7a 100644 --- a/packages/core/src/utils/schemaValidator.test.ts +++ b/packages/core/src/utils/schemaValidator.test.ts @@ -874,6 +874,125 @@ describe('SchemaValidator', () => { }); }); + describe('$id registration vs. idempotent compilation', () => { + // Both invariants must hold at the same time: + // 1. `$id` registration stays enabled so `$ref`s that resolve via an + // absolute `$id` URI (self-recursive and cross-document references) + // still compile — silently skipping them makes validate() return + // null and lets malformed tool arguments through. + // 2. A distinct schema object reusing an already-registered `$id` + // (two MCP tools generated from one template, or a registry rebuilt + // after an MCP reconnect/refresh) must not make compile() throw + // "schema with key or id ... already exists". + const selfRefSchema = { + $id: 'https://qwen-code.test/schemaValidator/self-ref', + type: 'object', + properties: { + name: { type: 'string' }, + child: { $ref: 'https://qwen-code.test/schemaValidator/self-ref' }, + }, + }; + + it('compiles and enforces schemas that recurse through their own $id', () => { + expect(SchemaValidator.canEnforce(selfRefSchema)).toBe(true); + expect(SchemaValidator.compileStrict(selfRefSchema)).toBeNull(); + + // An array is not coercible to string, so real enforcement (not the + // coercion passes) is what turns this into an error. When compilation + // is silently skipped, validate() returns null instead. + expect( + SchemaValidator.validate(selfRefSchema, { + name: 'root', + child: { name: [1] }, + }), + ).not.toBeNull(); + + // A conforming nested payload still passes. + expect( + SchemaValidator.validate(selfRefSchema, { + name: 'root', + child: { name: 'leaf', child: { name: 'deep' } }, + }), + ).toBeNull(); + }); + + it('compiles and enforces schemas with a bare self $ref', () => { + const bareSelfRef = { + $id: 'https://qwen-code.test/schemaValidator/bare-self-ref', + type: 'object', + properties: { + name: { type: 'string' }, + child: { $ref: '#' }, + }, + }; + expect(SchemaValidator.canEnforce(bareSelfRef)).toBe(true); + expect( + SchemaValidator.validate(bareSelfRef, { child: { name: [1] } }), + ).not.toBeNull(); + }); + + it('resolves cross-document $refs through a registered $id', () => { + const leaf = { + $id: 'https://qwen-code.test/schemaValidator/xdoc-leaf', + type: 'object', + properties: { v: { type: 'string' } }, + }; + // Registers `leaf` under its `$id` in the shared instances. + expect(SchemaValidator.canEnforce(leaf)).toBe(true); + + const tree = { + type: 'object', + properties: { + leaf: { $ref: 'https://qwen-code.test/schemaValidator/xdoc-leaf' }, + }, + }; + expect(SchemaValidator.canEnforce(tree)).toBe(true); + expect( + SchemaValidator.validate(tree, { leaf: { v: [1] } }), + ).not.toBeNull(); + expect(SchemaValidator.validate(tree, { leaf: { v: 'ok' } })).toBeNull(); + }); + + it('recompiling same-$id schemas is idempotent without cross-pollution', () => { + const sharedId = 'https://qwen-code.test/schemaValidator/repeat-compile'; + const schema1 = { + $id: sharedId, + type: 'object', + properties: { value: { type: 'string', maxLength: 5 } }, + }; + + // Same object compiled repeatedly across the shared instances. + expect( + SchemaValidator.validate(schema1, { value: 'a'.repeat(50) }), + ).not.toBeNull(); + expect(SchemaValidator.validate(schema1, { value: 'ok' })).toBeNull(); + expect(SchemaValidator.canEnforce(schema1)).toBe(true); + expect(SchemaValidator.canEnforce(schema1)).toBe(true); + + // A distinct object reusing the same $id with a different constraint + // must compile (no "already exists" collision) and enforce ITS OWN + // constraints rather than the displaced schema's. + const schema2 = { + $id: sharedId, + type: 'object', + properties: { value: { type: 'string', maxLength: 2 } }, + }; + expect(SchemaValidator.canEnforce(schema2)).toBe(true); + // maxLength 2 rejects 'abc'. + expect( + SchemaValidator.validate(schema2, { value: 'abc' }), + ).not.toBeNull(); + expect(SchemaValidator.validate(schema2, { value: 'ab' })).toBeNull(); + + // The displaced schema object can still be recompiled afterwards and + // enforces its own (looser) constraint again. + expect( + SchemaValidator.validate(schema1, { value: 'abcdef' }), + ).not.toBeNull(); + expect(SchemaValidator.validate(schema1, { value: 'abc' })).toBeNull(); + }); + }); + describe('non-string to string coercion', () => { const schema = { type: 'object', diff --git a/packages/core/src/utils/schemaValidator.ts b/packages/core/src/utils/schemaValidator.ts index 06c98fec245..8c8a0d6be80 100644 --- a/packages/core/src/utils/schemaValidator.ts +++ b/packages/core/src/utils/schemaValidator.ts @@ -20,13 +20,6 @@ const Ajv2020Class = (Ajv2020Pkg as any).default || Ajv2020Pkg; const debugLogger = createDebugLogger('SchemaValidator'); const ajvOptions = { - // Don't register compiled schemas by `$id`. These instances are - // module-level and shared; if a second, distinct schema object carries - // the same top-level `$id` (two MCP tools generated from one template, - // or a registry rebuilt after an MCP reconnect/refresh), Ajv would throw - // "schema with key or id ... already exists" and the bare catch would - // silently turn enforcement/relaxation off for that schema. - addUsedSchema: false, // See: https://ajv.js.org/options.html#strict-mode-options // strictSchema defaults to true and prevents use of JSON schemas that // include unrecognized keywords. The JSON schema spec specifically allows @@ -54,10 +47,6 @@ const ajvOptions = { }; const strictCompileOptions = { - // Same rationale as `ajvOptions`: keep compile() idempotent for - // `$id`-bearing schemas so a distinct schema object reusing a `$id` - // doesn't collide in the shared strict instances. - addUsedSchema: false, strictSchema: true, strictRequired: false, strictTypes: false, @@ -116,6 +105,39 @@ function getStrictValidator(schema: AnySchema): Ajv { : ajvStrictDefault; } +/** + * Evicts a stale `$id` registration from a shared Ajv instance before + * compiling, so that a DISTINCT schema object reusing the same top-level + * `$id` (two MCP tools generated from one template, or a registry rebuilt + * after an MCP reconnect/refresh) cannot make compile() throw + * "schema with key or id ... already exists" — which the bare catch in + * validate() would turn into silently-skipped validation. + * + * `$id` registration itself must stay enabled (Ajv's default + * `addUsedSchema: true`): `$ref`s that resolve through an absolute `$id` + * URI — a schema recursing through its own top-level `$id`, or a + * cross-document reference to another registered schema — fail to compile + * without it. + * + * The same-object fast path is preserved: Ajv caches compiled schemas by + * object identity, so when the schema registered under the `$id` IS the + * one being compiled, the registration is left untouched and compile() + * returns the cached validator. + */ +function evictStaleSchemaId(validator: Ajv, schema: AnySchema): void { + if (typeof schema !== 'object' || schema === null) { + return; + } + const id = (schema as { $id?: unknown }).$id; + if (typeof id !== 'string') { + return; + } + const registered = validator.getSchema(id); + if (registered && registered.schema !== schema) { + validator.removeSchema(id); + } +} + /** * Simple utility to validate objects against JSON Schemas. * Supports both draft-07 (default) and draft-2020-12 schemas. @@ -137,8 +159,12 @@ export class SchemaValidator { try { const anySchema = schema as AnySchema; - getStrictValidator(anySchema).compile(anySchema); - getValidator(anySchema).compile(anySchema); + const strictValidator = getStrictValidator(anySchema); + evictStaleSchemaId(strictValidator, anySchema); + strictValidator.compile(anySchema); + const runtimeValidator = getValidator(anySchema); + evictStaleSchemaId(runtimeValidator, anySchema); + runtimeValidator.compile(anySchema); return true; } catch { return false; @@ -208,6 +234,7 @@ export class SchemaValidator { // This matches LenientJsonSchemaValidator behavior in mcp-client.ts. let validate; try { + evictStaleSchemaId(validator, anySchema); validate = validator.compile(anySchema); } catch (error) { // Schema compilation failed (unsupported version, invalid $ref, etc.) From 0700c6ed3336d088d99b28b754ccc3c9c9fb0d2a Mon Sep 17 00:00:00 2001 From: "jinjing.zzj" Date: Mon, 31 Aug 2026 09:12:22 +0800 Subject: [PATCH 24/29] fix(core): make shared-$id schema eviction collision-triggered and poison-safe evictStaleSchemaId ran ahead of every compile and passed the raw $id to removeSchema, which left four holes in the shared Ajv instances: - Ajv registers schemas under normalizeId($id) (trailing # / #/ stripped) while string-form removeSchema deletes the raw key, so eviction was a silent no-op for trailing-# $ids; the next compile hit "already exists" and validate() silently skipped enforcement. - Evicting before compile removed a shared registration that a failing compile then left replaced by the broken schema, poisoning every $ref resolving through that $id for the process lifetime. - The eviction probe used getSchema(), which compiles the registered schema; a poison registration re-threw inside the probe on every call, aborting eviction forever. - Nothing guarded Ajv's meta-schema registrations, so a vended schema whose $id equals a meta-schema URI could replace the shared meta-schema and make every later compile meta-validate against the attacker body. Eviction now happens only on an actual duplicate-$id compile collision (compileWithEviction), removes via the normalized key/schema object, wraps the probe so poison registrations are dropped instead of re-thrown, refuses to evict meta-schema registrations, clears the failed attempt's identity-cache entry before retrying (the retry would otherwise bypass _checkUnique and the $id re-registration), and restores the displaced registration when the post-eviction compile fails. Adds regression tests pinning all four invariants; each goes red when the corresponding fix arm is reverted. Co-authored-by: Qwen-Coder --- .../core/src/utils/schemaValidator.test.ts | 128 +++++++++++++++ packages/core/src/utils/schemaValidator.ts | 146 +++++++++++++++--- 2 files changed, 256 insertions(+), 18 deletions(-) diff --git a/packages/core/src/utils/schemaValidator.test.ts b/packages/core/src/utils/schemaValidator.test.ts index 882285e4d7a..3a5e523a0e9 100644 --- a/packages/core/src/utils/schemaValidator.test.ts +++ b/packages/core/src/utils/schemaValidator.test.ts @@ -993,6 +993,134 @@ describe('SchemaValidator', () => { }); }); + describe('shared-$id eviction safety', () => { + it('evicts stale registrations whose $id ends in # or #/', () => { + for (const suffix of ['#', '#/']) { + const sharedId = `https://qwen-code.test/schemaValidator/dup-hash${suffix}`; + const first = { + $id: sharedId, + type: 'object', + properties: { value: { type: 'string', maxLength: 3 } }, + }; + // A distinct object reusing the same trailing-# $id: Ajv registers + // under the normalized id (trailing `#`/`#/` stripped), so eviction + // must remove the normalized registration rather than the raw key. + const second = { + $id: sharedId, + type: 'object', + properties: { value: { type: 'string', maxLength: 3 } }, + }; + expect(SchemaValidator.canEnforce(first)).toBe(true); + expect(SchemaValidator.canEnforce(second)).toBe(true); + expect( + SchemaValidator.validate(second, { value: 'a'.repeat(50) }), + ).not.toBeNull(); + expect(SchemaValidator.validate(second, { value: 'ok' })).toBeNull(); + } + }); + + it('does not poison a shared $id when the colliding schema fails to compile', () => { + const sharedId = + 'https://qwen-code.test/schemaValidator/collision-poison'; + const good = { + $id: sharedId, + type: 'object', + properties: { v: { type: 'string' } }, + }; + expect(SchemaValidator.canEnforce(good)).toBe(true); + + // An uncompilable body reusing the registered $id: its failed compile + // must not leave the shared instances registered to it, or every + // $ref resolving through sharedId would fail forever. + const broken = { + $id: sharedId, + $ref: 'https://qwen-code.test/schemaValidator/collision-poison-missing', + }; + expect(SchemaValidator.canEnforce(broken)).toBe(false); + + const referrer = { + type: 'object', + properties: { leaf: { $ref: sharedId } }, + }; + expect(SchemaValidator.canEnforce(referrer)).toBe(true); + expect( + SchemaValidator.validate(referrer, { leaf: { v: [1] } }), + ).not.toBeNull(); + expect( + SchemaValidator.validate(referrer, { leaf: { v: 'ok' } }), + ).toBeNull(); + }); + + it('evicts a poison registration without re-throwing through the probe', () => { + const sharedId = 'https://qwen-code.test/schemaValidator/poison-probe'; + const poison = { + $id: sharedId, + $ref: 'https://qwen-code.test/schemaValidator/poison-probe-missing', + }; + // Registers `poison` under sharedId, then fails to compile it; the + // registration stays behind and its compile error must not abort + // eviction for the valid sibling below. + expect(SchemaValidator.validate(poison, {})).toBeNull(); + + const sibling = { + $id: sharedId, + type: 'object', + properties: { value: { type: 'string', maxLength: 2 } }, + }; + expect(SchemaValidator.canEnforce(sibling)).toBe(true); + expect( + SchemaValidator.validate(sibling, { value: 'abc' }), + ).not.toBeNull(); + expect(SchemaValidator.validate(sibling, { value: 'ab' })).toBeNull(); + }); + + it('never evicts Ajv meta-schema registrations', () => { + // Each attacker body must be valid against ITSELF: Ajv meta-validates + // a schema against whatever is registered under the meta URI, so an + // attacker that replaced the meta-schema but failed its own check + // would be evicted again and the poisoning would not stick. A + // self-consistent attacker that demands a marker property rejects + // every normal schema afterwards. + const marker = '__schemaValidatorMetaPoison'; + const legs = [ + { + attacker: { + $id: 'http://json-schema.org/draft-07/schema', + type: 'object', + required: [marker], + [marker]: true, + }, + probe: { + type: 'object', + properties: { v: { type: 'string', maxLength: 3 } }, + }, + }, + { + attacker: { + $schema: 'https://json-schema.org/draft/2020-12/schema', + $id: 'https://json-schema.org/draft/2020-12/schema', + type: 'object', + required: [marker], + [marker]: true, + }, + probe: { + $schema: 'https://json-schema.org/draft/2020-12/schema', + type: 'object', + properties: { v: { type: 'string', maxLength: 3 } }, + }, + }, + ]; + for (const { attacker, probe } of legs) { + expect(SchemaValidator.canEnforce(attacker)).toBe(false); + expect(SchemaValidator.validate(attacker, {})).toBeNull(); + // The shared meta-schema must survive the attacker: a fresh schema + // still compiles and still enforces. + expect(SchemaValidator.canEnforce(probe)).toBe(true); + expect(SchemaValidator.validate(probe, { v: 'aaaa' })).not.toBeNull(); + } + }); + }); + describe('non-string to string coercion', () => { const schema = { type: 'object', diff --git a/packages/core/src/utils/schemaValidator.ts b/packages/core/src/utils/schemaValidator.ts index 8c8a0d6be80..440a729f94a 100644 --- a/packages/core/src/utils/schemaValidator.ts +++ b/packages/core/src/utils/schemaValidator.ts @@ -106,12 +106,63 @@ function getStrictValidator(schema: AnySchema): Ajv { } /** - * Evicts a stale `$id` registration from a shared Ajv instance before - * compiling, so that a DISTINCT schema object reusing the same top-level + * Ajv meta-schema registrations must never be evicted: `_addSchema` + * registers a schema under its `$id` BEFORE meta-validation runs, so a + * vended schema whose `$id` equals a meta-schema URI would otherwise + * replace the shared meta-schema and every later compile in the process + * would meta-validate against the attacker body. The alias is what Ajv's + * draft classes keep in `refs` for the default meta-schema. + */ +const WELL_KNOWN_META_SCHEMA_IDS: ReadonlySet = new Set([ + 'http://json-schema.org/draft-06/schema', + 'http://json-schema.org/draft-07/schema', + 'https://json-schema.org/draft/2019-09/schema', + 'https://json-schema.org/draft/2020-12/schema', + 'http://json-schema.org/schema', +]); + +/** Mirrors Ajv's `normalizeId` (strips a trailing `#` or `#/`). */ +function normalizeSchemaId(id: string): string { + return id.replace(/#\/?$/, ''); +} + +function isMetaSchemaRegistration( + validator: Ajv, + normalizedId: string, +): boolean { + const defaultMeta = validator.defaultMeta(); + if ( + typeof defaultMeta === 'string' && + normalizeSchemaId(defaultMeta) === normalizedId + ) { + return true; + } + return WELL_KNOWN_META_SCHEMA_IDS.has(normalizedId); +} + +function isDuplicateSchemaIdError(error: unknown): boolean { + return error instanceof Error && error.message.includes('already exists'); +} + +/** + * Evicts a stale `$id` registration from a shared Ajv instance, but ONLY + * when compile() has actually reported the duplicate-`$id` collision (see + * {@link compileWithEviction}) — evicting ahead of compile would remove a + * shared registration that a subsequent compile failure then leaves + * replaced by the broken schema. + * + * Returns the displaced schema object (when a DIFFERENT, previously + * compiled schema was registered under the `$id`) so the caller can + * restore it if the post-eviction compile fails, plus whether anything + * was actually evicted — when nothing was (meta-schema guard), a retry + * would bypass `_checkUnique` through the failed attempt's identity-cache + * entry and must not happen. + * + * Eviction exists so a DISTINCT schema object reusing the same top-level * `$id` (two MCP tools generated from one template, or a registry rebuilt - * after an MCP reconnect/refresh) cannot make compile() throw - * "schema with key or id ... already exists" — which the bare catch in - * validate() would turn into silently-skipped validation. + * after an MCP reconnect/refresh) cannot make compile() throw "schema + * with key or id ... already exists" — which the bare catch in validate() + * would turn into silently-skipped validation. * * `$id` registration itself must stay enabled (Ajv's default * `addUsedSchema: true`): `$ref`s that resolve through an absolute `$id` @@ -124,17 +175,81 @@ function getStrictValidator(schema: AnySchema): Ajv { * one being compiled, the registration is left untouched and compile() * returns the cached validator. */ -function evictStaleSchemaId(validator: Ajv, schema: AnySchema): void { +function evictStaleSchemaId( + validator: Ajv, + schema: AnySchema, +): { evicted: boolean; displaced?: AnySchema } { if (typeof schema !== 'object' || schema === null) { - return; + return { evicted: false }; } const id = (schema as { $id?: unknown }).$id; if (typeof id !== 'string') { - return; + return { evicted: false }; + } + const key = normalizeSchemaId(id); + if (isMetaSchemaRegistration(validator, key)) { + return { evicted: false }; + } + let registered: ReturnType | undefined; + try { + registered = validator.getSchema(key); + } catch { + // A registered schema that cannot compile is stale by definition. + // Remove it by the normalized key: string-form removeSchema deletes + // the raw key, so the unnormalized `$id` would be a silent no-op for + // ids ending in `#` or `#/`. + validator.removeSchema(key); + return { evicted: true }; } - const registered = validator.getSchema(id); if (registered && registered.schema !== schema) { - validator.removeSchema(id); + // Object-form removeSchema normalizes the schema's `$id` itself; + // passing the raw `$id` would miss ids ending in `#` or `#/`. + validator.removeSchema(registered.schema); + return { evicted: true, displaced: registered.schema }; + } + return { evicted: false }; +} + +/** + * Compiles `schema`, evicting a stale same-`$id` registration only when + * Ajv reports the actual duplicate-`$id` collision, and restoring the + * displaced registration when the post-eviction compile fails (Ajv's + * `_addSchema` registers the schema under its `$id` before the compile + * error surfaces, so a failed compile would otherwise poison the shared + * instance for every `$ref` resolving through that `$id`). + */ +function compileWithEviction( + validator: Ajv, + schema: AnySchema, +): ReturnType { + try { + return validator.compile(schema); + } catch (error) { + if (!isDuplicateSchemaIdError(error)) { + throw error; + } + const { evicted, displaced } = evictStaleSchemaId(validator, schema); + if (!evicted) { + // Nothing was removed (e.g. the `$id` is a meta-schema URI): the + // duplicate collision stands. + throw error; + } + // The failed attempt left an identity-cache entry that would let the + // retry bypass both `_checkUnique` and the `$id` re-registration. + validator.removeSchema(schema); + try { + return validator.compile(schema); + } catch (retryError) { + if (displaced !== undefined) { + try { + validator.removeSchema(schema); + validator.compile(displaced); + } catch { + // Best effort: keep surfacing the original compile error. + } + } + throw retryError; + } } } @@ -159,12 +274,8 @@ export class SchemaValidator { try { const anySchema = schema as AnySchema; - const strictValidator = getStrictValidator(anySchema); - evictStaleSchemaId(strictValidator, anySchema); - strictValidator.compile(anySchema); - const runtimeValidator = getValidator(anySchema); - evictStaleSchemaId(runtimeValidator, anySchema); - runtimeValidator.compile(anySchema); + compileWithEviction(getStrictValidator(anySchema), anySchema); + compileWithEviction(getValidator(anySchema), anySchema); return true; } catch { return false; @@ -234,8 +345,7 @@ export class SchemaValidator { // This matches LenientJsonSchemaValidator behavior in mcp-client.ts. let validate; try { - evictStaleSchemaId(validator, anySchema); - validate = validator.compile(anySchema); + validate = compileWithEviction(validator, anySchema); } catch (error) { // Schema compilation failed (unsupported version, invalid $ref, etc.) // Skip validation rather than blocking tool usage. From 2261b547706194793c6f74d37b8ceb98c7d64e8b Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 1 Sep 2026 16:51:48 +0800 Subject: [PATCH 25/29] fix(core): isolate schema grammar validation Co-authored-by: Qwen-Coder --- ...penai-tool-schema-grammar-compatibility.md | 6 +- .../openaiContentGenerator/converter.test.ts | 40 +++ .../core/openaiContentGenerator/converter.ts | 2 +- .../core/src/utils/schemaConverter.test.ts | 12 +- packages/core/src/utils/schemaConverter.ts | 4 +- .../core/src/utils/schemaValidator.test.ts | 304 ------------------ packages/core/src/utils/schemaValidator.ts | 206 +----------- 7 files changed, 62 insertions(+), 512 deletions(-) diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md index d1d3dbaa666..f536dd9dda2 100644 --- a/docs/design/openai-tool-schema-grammar-compatibility.md +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -11,10 +11,10 @@ Disabling all tools avoids grammar construction but also removes the functionali Keep the registered tool set unchanged. Before an OpenAI-compatible request is sent, recursively relax only the wire copy of each schema: - omit empty `properties` maps and `additionalProperties: false` on object-capable schemas with zero declared properties; -- omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above the grammar repetition boundary of 2000; +- omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above 1999, the lowest failing boundary measured across the four keywords; - preserve smaller limits and all other supported constraints. -Apply these grammar-specific relaxations only when the source schema both compiles with the validator used before tool execution and passes strict vocabulary compilation for that validator's selected dialect. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. +Apply these grammar-specific relaxations only when the source schema passes the existing isolated strict compilation for its selected dialect. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. The original schema remains attached to the tool and continues to drive client-side parameter validation. The provider receives a schema it can compile, while Qwen Code still rejects tool calls that violate the original limits. @@ -24,4 +24,4 @@ This applies to both built-in tools and MCP-provided schemas because they share ## Verification -Unit coverage exercises recursive empty objects, the 1999/2000 boundary, the actual OpenAI tool converter, and source-schema immutability. A live LM Studio smoke remains useful when that runtime is available, but the regression test pins the exact request shapes that caused grammar initialization to fail. +Unit coverage exercises recursive empty objects, the 1998/1999 boundary, the actual OpenAI tool converter, and source-schema immutability. A live LM Studio smoke remains useful when that runtime is available, but the regression test pins the exact request shapes that caused grammar initialization to fail. diff --git a/packages/core/src/core/openaiContentGenerator/converter.test.ts b/packages/core/src/core/openaiContentGenerator/converter.test.ts index c13613fa9b5..8351b6119ea 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.test.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.test.ts @@ -5985,6 +5985,46 @@ describe('OpenAIContentConverter', () => { ).toBe(false); }); + it('isolates grammar checks for schemas that share a top-level $id', async () => { + const sharedId = 'https://qwen-code.test/shared-tool-schema'; + const makeSchema = () => ({ + $id: sharedId, + type: 'object', + properties: { + value: { type: 'string', maxLength: 1999 }, + }, + }); + const tools = [ + { + functionDeclarations: [ + { name: 'first', parametersJsonSchema: makeSchema() }, + { name: 'second', parametersJsonSchema: makeSchema() }, + ], + }, + ] as Tool[]; + + const result = await converter.convertLlmToolsToOpenAI(tools); + + expect(result.map(({ function: declaration }) => declaration)).toEqual([ + { + name: 'first', + description: '', + parameters: { + type: 'object', + properties: { value: { type: 'string' } }, + }, + }, + { + name: 'second', + description: '', + parameters: { + type: 'object', + properties: { value: { type: 'string' } }, + }, + }, + ]); + }); + it('should convert Gemini tools with parameters field', async () => { const llmTools = [ { diff --git a/packages/core/src/core/openaiContentGenerator/converter.ts b/packages/core/src/core/openaiContentGenerator/converter.ts index 131503b1c96..ef712f9fb94 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.ts @@ -373,7 +373,7 @@ export async function convertLlmToolsToOpenAI( if (parameters) { const canValidateLocally = func.parametersJsonSchema !== undefined && - SchemaValidator.canEnforce(func.parametersJsonSchema); + SchemaValidator.compileStrict(func.parametersJsonSchema) === null; parameters = convertSchema(parameters, schemaCompliance); // #7315: gateways enforcing OpenAI's structured-output contract // promote every property to required when an object level has diff --git a/packages/core/src/utils/schemaConverter.test.ts b/packages/core/src/utils/schemaConverter.test.ts index 0e110974e6f..f893dabd583 100644 --- a/packages/core/src/utils/schemaConverter.test.ts +++ b/packages/core/src/utils/schemaConverter.test.ts @@ -511,17 +511,17 @@ describe('relaxSchemaForFunctionCalling', () => { type: ['object', 'null'], additionalProperties: false, }, - bounded: { type: 'string', maxLength: 1999 }, - long: { type: 'string', maxLength: 2000 }, - padded: { type: 'string', minLength: 2000 }, + bounded: { type: 'string', maxLength: 1998 }, + long: { type: 'string', maxLength: 1999 }, + padded: { type: 'string', minLength: 1999 }, many: { type: 'array', - maxItems: 2000, + maxItems: 1999, items: { type: 'string' }, }, largeBatch: { type: 'array', - minItems: 2000, + minItems: 1999, items: { type: 'string' }, }, }, @@ -534,7 +534,7 @@ describe('relaxSchemaForFunctionCalling', () => { closed: { type: 'object' }, typelessClosed: {}, nullableClosed: { type: ['object', 'null'] }, - bounded: { type: 'string', maxLength: 1999 }, + bounded: { type: 'string', maxLength: 1998 }, long: { type: 'string' }, padded: { type: 'string' }, many: { diff --git a/packages/core/src/utils/schemaConverter.ts b/packages/core/src/utils/schemaConverter.ts index 57a2c17777f..c9260432392 100644 --- a/packages/core/src/utils/schemaConverter.ts +++ b/packages/core/src/utils/schemaConverter.ts @@ -200,7 +200,7 @@ function toOpenAPI30(schema: Record): Record { * - `uniqueItems` is dropped at every schema level because some * OpenAI-compatible function-calling endpoints reject it. * - When the source schema can be validated locally, empty object declarations - * and string / array length limits at or above 2000 are dropped because + * and string / array length limits at or above 1999 are dropped because * grammar-based endpoints can turn them into invalid or rejected rules. * - Other constraints pass through untouched; client-side * `validateToolParams` still enforces the full source schema, so the @@ -264,7 +264,7 @@ export function relaxSchemaForFunctionCalling( key === 'minItems' || key === 'maxItems') && typeof value === 'number' && - value >= 2000 + value >= 1999 ) { continue; } diff --git a/packages/core/src/utils/schemaValidator.test.ts b/packages/core/src/utils/schemaValidator.test.ts index 3a5e523a0e9..84ad827d808 100644 --- a/packages/core/src/utils/schemaValidator.test.ts +++ b/packages/core/src/utils/schemaValidator.test.ts @@ -817,310 +817,6 @@ describe('SchemaValidator', () => { }); }); - describe('canEnforce', () => { - it('requires the runtime dialect to recognize the full vocabulary', () => { - const tupleKeywords = { - prefixItems: [{ type: 'string' }], - minItems: 1, - maxItems: 1, - items: false, - }; - - expect( - SchemaValidator.canEnforce({ - type: 'array', - ...tupleKeywords, - }), - ).toBe(false); - expect( - SchemaValidator.canEnforce({ - $schema: 'https://json-schema.org/draft/2020-12/schema', - type: 'array', - ...tupleKeywords, - }), - ).toBe(true); - expect( - SchemaValidator.canEnforce({ - type: 'object', - properties: { value: { type: 'string' } }, - }), - ).toBe(true); - }); - - it('enforces two distinct schemas that share one top-level $id', () => { - const sharedId = 'https://qwen-code.test/schemaValidator/duplicate-id'; - const schemaA = { - $id: sharedId, - type: 'object', - properties: { value: { type: 'string', maxLength: 10 } }, - }; - // A DISTINCT schema object reusing the same top-level $id (e.g. two - // tools generated from one template, or a registry rebuilt after an - // MCP reconnect/refresh). - const schemaB = { - $id: sharedId, - type: 'object', - properties: { value: { type: 'string', maxLength: 10 } }, - }; - - expect(SchemaValidator.canEnforce(schemaA)).toBe(true); - expect(SchemaValidator.canEnforce(schemaB)).toBe(true); - - // Validation must still enforce constraints on the rebuilt schema - // object instead of silently skipping it. - expect( - SchemaValidator.validate(schemaB, { value: 'a'.repeat(50) }), - ).not.toBeNull(); - }); - }); - - describe('$id registration vs. idempotent compilation', () => { - // Both invariants must hold at the same time: - // 1. `$id` registration stays enabled so `$ref`s that resolve via an - // absolute `$id` URI (self-recursive and cross-document references) - // still compile — silently skipping them makes validate() return - // null and lets malformed tool arguments through. - // 2. A distinct schema object reusing an already-registered `$id` - // (two MCP tools generated from one template, or a registry rebuilt - // after an MCP reconnect/refresh) must not make compile() throw - // "schema with key or id ... already exists". - const selfRefSchema = { - $id: 'https://qwen-code.test/schemaValidator/self-ref', - type: 'object', - properties: { - name: { type: 'string' }, - child: { $ref: 'https://qwen-code.test/schemaValidator/self-ref' }, - }, - }; - - it('compiles and enforces schemas that recurse through their own $id', () => { - expect(SchemaValidator.canEnforce(selfRefSchema)).toBe(true); - expect(SchemaValidator.compileStrict(selfRefSchema)).toBeNull(); - - // An array is not coercible to string, so real enforcement (not the - // coercion passes) is what turns this into an error. When compilation - // is silently skipped, validate() returns null instead. - expect( - SchemaValidator.validate(selfRefSchema, { - name: 'root', - child: { name: [1] }, - }), - ).not.toBeNull(); - - // A conforming nested payload still passes. - expect( - SchemaValidator.validate(selfRefSchema, { - name: 'root', - child: { name: 'leaf', child: { name: 'deep' } }, - }), - ).toBeNull(); - }); - - it('compiles and enforces schemas with a bare self $ref', () => { - const bareSelfRef = { - $id: 'https://qwen-code.test/schemaValidator/bare-self-ref', - type: 'object', - properties: { - name: { type: 'string' }, - child: { $ref: '#' }, - }, - }; - expect(SchemaValidator.canEnforce(bareSelfRef)).toBe(true); - expect( - SchemaValidator.validate(bareSelfRef, { child: { name: [1] } }), - ).not.toBeNull(); - }); - - it('resolves cross-document $refs through a registered $id', () => { - const leaf = { - $id: 'https://qwen-code.test/schemaValidator/xdoc-leaf', - type: 'object', - properties: { v: { type: 'string' } }, - }; - // Registers `leaf` under its `$id` in the shared instances. - expect(SchemaValidator.canEnforce(leaf)).toBe(true); - - const tree = { - type: 'object', - properties: { - leaf: { $ref: 'https://qwen-code.test/schemaValidator/xdoc-leaf' }, - }, - }; - expect(SchemaValidator.canEnforce(tree)).toBe(true); - expect( - SchemaValidator.validate(tree, { leaf: { v: [1] } }), - ).not.toBeNull(); - expect(SchemaValidator.validate(tree, { leaf: { v: 'ok' } })).toBeNull(); - }); - - it('recompiling same-$id schemas is idempotent without cross-pollution', () => { - const sharedId = 'https://qwen-code.test/schemaValidator/repeat-compile'; - const schema1 = { - $id: sharedId, - type: 'object', - properties: { value: { type: 'string', maxLength: 5 } }, - }; - - // Same object compiled repeatedly across the shared instances. - expect( - SchemaValidator.validate(schema1, { value: 'a'.repeat(50) }), - ).not.toBeNull(); - expect(SchemaValidator.validate(schema1, { value: 'ok' })).toBeNull(); - expect(SchemaValidator.canEnforce(schema1)).toBe(true); - expect(SchemaValidator.canEnforce(schema1)).toBe(true); - - // A distinct object reusing the same $id with a different constraint - // must compile (no "already exists" collision) and enforce ITS OWN - // constraints rather than the displaced schema's. - const schema2 = { - $id: sharedId, - type: 'object', - properties: { value: { type: 'string', maxLength: 2 } }, - }; - expect(SchemaValidator.canEnforce(schema2)).toBe(true); - // maxLength 2 rejects 'abc'. - expect( - SchemaValidator.validate(schema2, { value: 'abc' }), - ).not.toBeNull(); - expect(SchemaValidator.validate(schema2, { value: 'ab' })).toBeNull(); - - // The displaced schema object can still be recompiled afterwards and - // enforces its own (looser) constraint again. - expect( - SchemaValidator.validate(schema1, { value: 'abcdef' }), - ).not.toBeNull(); - expect(SchemaValidator.validate(schema1, { value: 'abc' })).toBeNull(); - }); - }); - - describe('shared-$id eviction safety', () => { - it('evicts stale registrations whose $id ends in # or #/', () => { - for (const suffix of ['#', '#/']) { - const sharedId = `https://qwen-code.test/schemaValidator/dup-hash${suffix}`; - const first = { - $id: sharedId, - type: 'object', - properties: { value: { type: 'string', maxLength: 3 } }, - }; - // A distinct object reusing the same trailing-# $id: Ajv registers - // under the normalized id (trailing `#`/`#/` stripped), so eviction - // must remove the normalized registration rather than the raw key. - const second = { - $id: sharedId, - type: 'object', - properties: { value: { type: 'string', maxLength: 3 } }, - }; - expect(SchemaValidator.canEnforce(first)).toBe(true); - expect(SchemaValidator.canEnforce(second)).toBe(true); - expect( - SchemaValidator.validate(second, { value: 'a'.repeat(50) }), - ).not.toBeNull(); - expect(SchemaValidator.validate(second, { value: 'ok' })).toBeNull(); - } - }); - - it('does not poison a shared $id when the colliding schema fails to compile', () => { - const sharedId = - 'https://qwen-code.test/schemaValidator/collision-poison'; - const good = { - $id: sharedId, - type: 'object', - properties: { v: { type: 'string' } }, - }; - expect(SchemaValidator.canEnforce(good)).toBe(true); - - // An uncompilable body reusing the registered $id: its failed compile - // must not leave the shared instances registered to it, or every - // $ref resolving through sharedId would fail forever. - const broken = { - $id: sharedId, - $ref: 'https://qwen-code.test/schemaValidator/collision-poison-missing', - }; - expect(SchemaValidator.canEnforce(broken)).toBe(false); - - const referrer = { - type: 'object', - properties: { leaf: { $ref: sharedId } }, - }; - expect(SchemaValidator.canEnforce(referrer)).toBe(true); - expect( - SchemaValidator.validate(referrer, { leaf: { v: [1] } }), - ).not.toBeNull(); - expect( - SchemaValidator.validate(referrer, { leaf: { v: 'ok' } }), - ).toBeNull(); - }); - - it('evicts a poison registration without re-throwing through the probe', () => { - const sharedId = 'https://qwen-code.test/schemaValidator/poison-probe'; - const poison = { - $id: sharedId, - $ref: 'https://qwen-code.test/schemaValidator/poison-probe-missing', - }; - // Registers `poison` under sharedId, then fails to compile it; the - // registration stays behind and its compile error must not abort - // eviction for the valid sibling below. - expect(SchemaValidator.validate(poison, {})).toBeNull(); - - const sibling = { - $id: sharedId, - type: 'object', - properties: { value: { type: 'string', maxLength: 2 } }, - }; - expect(SchemaValidator.canEnforce(sibling)).toBe(true); - expect( - SchemaValidator.validate(sibling, { value: 'abc' }), - ).not.toBeNull(); - expect(SchemaValidator.validate(sibling, { value: 'ab' })).toBeNull(); - }); - - it('never evicts Ajv meta-schema registrations', () => { - // Each attacker body must be valid against ITSELF: Ajv meta-validates - // a schema against whatever is registered under the meta URI, so an - // attacker that replaced the meta-schema but failed its own check - // would be evicted again and the poisoning would not stick. A - // self-consistent attacker that demands a marker property rejects - // every normal schema afterwards. - const marker = '__schemaValidatorMetaPoison'; - const legs = [ - { - attacker: { - $id: 'http://json-schema.org/draft-07/schema', - type: 'object', - required: [marker], - [marker]: true, - }, - probe: { - type: 'object', - properties: { v: { type: 'string', maxLength: 3 } }, - }, - }, - { - attacker: { - $schema: 'https://json-schema.org/draft/2020-12/schema', - $id: 'https://json-schema.org/draft/2020-12/schema', - type: 'object', - required: [marker], - [marker]: true, - }, - probe: { - $schema: 'https://json-schema.org/draft/2020-12/schema', - type: 'object', - properties: { v: { type: 'string', maxLength: 3 } }, - }, - }, - ]; - for (const { attacker, probe } of legs) { - expect(SchemaValidator.canEnforce(attacker)).toBe(false); - expect(SchemaValidator.validate(attacker, {})).toBeNull(); - // The shared meta-schema must survive the attacker: a fresh schema - // still compiles and still enforces. - expect(SchemaValidator.canEnforce(probe)).toBe(true); - expect(SchemaValidator.validate(probe, { v: 'aaaa' })).not.toBeNull(); - } - }); - }); - describe('non-string to string coercion', () => { const schema = { type: 'object', diff --git a/packages/core/src/utils/schemaValidator.ts b/packages/core/src/utils/schemaValidator.ts index 440a729f94a..06af19c900c 100644 --- a/packages/core/src/utils/schemaValidator.ts +++ b/packages/core/src/utils/schemaValidator.ts @@ -46,24 +46,12 @@ const ajvOptions = { }, }; -const strictCompileOptions = { - strictSchema: true, - strictRequired: false, - strictTypes: false, - strictTuples: false, - validateFormats: false, - allowUnionTypes: true, -}; - // Draft-07 validator (default) const ajvDefault: Ajv = new AjvClass(ajvOptions); // Draft-2020-12 validator for MCP servers using rmcp const ajv2020: Ajv = new Ajv2020Class(ajvOptions); -const ajvStrictDefault: Ajv = new AjvClass(strictCompileOptions); -const ajvStrict2020: Ajv = new Ajv2020Class(strictCompileOptions); - // eslint-disable-next-line @typescript-eslint/no-explicit-any const addFormatsFunc = (addFormats as any).default || addFormats; addFormatsFunc(ajvDefault); @@ -96,192 +84,11 @@ function getValidator(schema: AnySchema): Ajv { return ajvDefault; } -function getStrictValidator(schema: AnySchema): Ajv { - return typeof schema === 'object' && - schema !== null && - '$schema' in schema && - isDraft2020Uri(schema.$schema) - ? ajvStrict2020 - : ajvStrictDefault; -} - -/** - * Ajv meta-schema registrations must never be evicted: `_addSchema` - * registers a schema under its `$id` BEFORE meta-validation runs, so a - * vended schema whose `$id` equals a meta-schema URI would otherwise - * replace the shared meta-schema and every later compile in the process - * would meta-validate against the attacker body. The alias is what Ajv's - * draft classes keep in `refs` for the default meta-schema. - */ -const WELL_KNOWN_META_SCHEMA_IDS: ReadonlySet = new Set([ - 'http://json-schema.org/draft-06/schema', - 'http://json-schema.org/draft-07/schema', - 'https://json-schema.org/draft/2019-09/schema', - 'https://json-schema.org/draft/2020-12/schema', - 'http://json-schema.org/schema', -]); - -/** Mirrors Ajv's `normalizeId` (strips a trailing `#` or `#/`). */ -function normalizeSchemaId(id: string): string { - return id.replace(/#\/?$/, ''); -} - -function isMetaSchemaRegistration( - validator: Ajv, - normalizedId: string, -): boolean { - const defaultMeta = validator.defaultMeta(); - if ( - typeof defaultMeta === 'string' && - normalizeSchemaId(defaultMeta) === normalizedId - ) { - return true; - } - return WELL_KNOWN_META_SCHEMA_IDS.has(normalizedId); -} - -function isDuplicateSchemaIdError(error: unknown): boolean { - return error instanceof Error && error.message.includes('already exists'); -} - -/** - * Evicts a stale `$id` registration from a shared Ajv instance, but ONLY - * when compile() has actually reported the duplicate-`$id` collision (see - * {@link compileWithEviction}) — evicting ahead of compile would remove a - * shared registration that a subsequent compile failure then leaves - * replaced by the broken schema. - * - * Returns the displaced schema object (when a DIFFERENT, previously - * compiled schema was registered under the `$id`) so the caller can - * restore it if the post-eviction compile fails, plus whether anything - * was actually evicted — when nothing was (meta-schema guard), a retry - * would bypass `_checkUnique` through the failed attempt's identity-cache - * entry and must not happen. - * - * Eviction exists so a DISTINCT schema object reusing the same top-level - * `$id` (two MCP tools generated from one template, or a registry rebuilt - * after an MCP reconnect/refresh) cannot make compile() throw "schema - * with key or id ... already exists" — which the bare catch in validate() - * would turn into silently-skipped validation. - * - * `$id` registration itself must stay enabled (Ajv's default - * `addUsedSchema: true`): `$ref`s that resolve through an absolute `$id` - * URI — a schema recursing through its own top-level `$id`, or a - * cross-document reference to another registered schema — fail to compile - * without it. - * - * The same-object fast path is preserved: Ajv caches compiled schemas by - * object identity, so when the schema registered under the `$id` IS the - * one being compiled, the registration is left untouched and compile() - * returns the cached validator. - */ -function evictStaleSchemaId( - validator: Ajv, - schema: AnySchema, -): { evicted: boolean; displaced?: AnySchema } { - if (typeof schema !== 'object' || schema === null) { - return { evicted: false }; - } - const id = (schema as { $id?: unknown }).$id; - if (typeof id !== 'string') { - return { evicted: false }; - } - const key = normalizeSchemaId(id); - if (isMetaSchemaRegistration(validator, key)) { - return { evicted: false }; - } - let registered: ReturnType | undefined; - try { - registered = validator.getSchema(key); - } catch { - // A registered schema that cannot compile is stale by definition. - // Remove it by the normalized key: string-form removeSchema deletes - // the raw key, so the unnormalized `$id` would be a silent no-op for - // ids ending in `#` or `#/`. - validator.removeSchema(key); - return { evicted: true }; - } - if (registered && registered.schema !== schema) { - // Object-form removeSchema normalizes the schema's `$id` itself; - // passing the raw `$id` would miss ids ending in `#` or `#/`. - validator.removeSchema(registered.schema); - return { evicted: true, displaced: registered.schema }; - } - return { evicted: false }; -} - -/** - * Compiles `schema`, evicting a stale same-`$id` registration only when - * Ajv reports the actual duplicate-`$id` collision, and restoring the - * displaced registration when the post-eviction compile fails (Ajv's - * `_addSchema` registers the schema under its `$id` before the compile - * error surfaces, so a failed compile would otherwise poison the shared - * instance for every `$ref` resolving through that `$id`). - */ -function compileWithEviction( - validator: Ajv, - schema: AnySchema, -): ReturnType { - try { - return validator.compile(schema); - } catch (error) { - if (!isDuplicateSchemaIdError(error)) { - throw error; - } - const { evicted, displaced } = evictStaleSchemaId(validator, schema); - if (!evicted) { - // Nothing was removed (e.g. the `$id` is a meta-schema URI): the - // duplicate collision stands. - throw error; - } - // The failed attempt left an identity-cache entry that would let the - // retry bypass both `_checkUnique` and the `$id` re-registration. - validator.removeSchema(schema); - try { - return validator.compile(schema); - } catch (retryError) { - if (displaced !== undefined) { - try { - validator.removeSchema(schema); - validator.compile(displaced); - } catch { - // Best effort: keep surfacing the original compile error. - } - } - throw retryError; - } - } -} - /** * Simple utility to validate objects against JSON Schemas. * Supports both draft-07 (default) and draft-2020-12 schemas. */ export class SchemaValidator { - /** - * Returns true only when the runtime validator recognizes the schema's full - * vocabulary. This prevents lenient compilation from being mistaken for - * local enforcement when deciding whether wire constraints may be relaxed. - */ - static canEnforce(schema: unknown): boolean { - if ( - typeof schema !== 'object' || - schema === null || - Array.isArray(schema) - ) { - return false; - } - - try { - const anySchema = schema as AnySchema; - compileWithEviction(getStrictValidator(anySchema), anySchema); - compileWithEviction(getValidator(anySchema), anySchema); - return true; - } catch { - return false; - } - } - /** * Strictly compiles a schema. Returns an error message if the schema is * malformed or uses unsupported draft/features for our Ajv configuration @@ -310,11 +117,18 @@ export class SchemaValidator { // or anything using a custom `format`. Keep typo detection; // tolerate the looser-but-still-spec-valid patterns users actually // ship in `--json-schema`. + const strictOptions = { + strictSchema: true, // catches unknown keywords (typos) + strictRequired: false, // allow `required` without `properties` + strictTypes: false, // allow inferred / partial type info + validateFormats: false, // unknown `format` values don't fail + allowUnionTypes: true, // type: ["a","b"] + }; const strictAjv: Ajv = isDraft2020Uri( (schema as { $schema?: unknown }).$schema, ) - ? new Ajv2020Class(strictCompileOptions) - : new AjvClass(strictCompileOptions); + ? new Ajv2020Class(strictOptions) + : new AjvClass(strictOptions); addFormatsFunc(strictAjv); try { strictAjv.compile(schema as AnySchema); @@ -345,7 +159,7 @@ export class SchemaValidator { // This matches LenientJsonSchemaValidator behavior in mcp-client.ts. let validate; try { - validate = compileWithEviction(validator, anySchema); + validate = validator.compile(anySchema); } catch (error) { // Schema compilation failed (unsupported version, invalid $ref, etc.) // Skip validation rather than blocking tool usage. From 926e1a985d0810e0ffab7493322002da73fbe41a Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 1 Sep 2026 16:59:01 +0800 Subject: [PATCH 26/29] fix(core): keep identified schemas constrained Co-authored-by: Qwen-Coder --- .../design/openai-tool-schema-grammar-compatibility.md | 2 +- .../src/core/openaiContentGenerator/converter.test.ts | 10 +++++++--- .../core/src/core/openaiContentGenerator/converter.ts | 8 ++++++-- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md index f536dd9dda2..45b8d81f6e5 100644 --- a/docs/design/openai-tool-schema-grammar-compatibility.md +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -14,7 +14,7 @@ Keep the registered tool set unchanged. Before an OpenAI-compatible request is s - omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above 1999, the lowest failing boundary measured across the four keywords; - preserve smaller limits and all other supported constraints. -Apply these grammar-specific relaxations only when the source schema passes the existing isolated strict compilation for its selected dialect. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. +Apply these grammar-specific relaxations only when the source schema passes the existing isolated strict compilation for its selected dialect and has no top-level `$id`. Schemas with a top-level `$id` keep their constraints because runtime validation uses a shared schema registry where duplicate IDs can prevent enforcement. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. The original schema remains attached to the tool and continues to drive client-side parameter validation. The provider receives a schema it can compile, while Qwen Code still rejects tool calls that violate the original limits. diff --git a/packages/core/src/core/openaiContentGenerator/converter.test.ts b/packages/core/src/core/openaiContentGenerator/converter.test.ts index 8351b6119ea..5c3a1d3065a 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.test.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.test.ts @@ -5985,7 +5985,7 @@ describe('OpenAIContentConverter', () => { ).toBe(false); }); - it('isolates grammar checks for schemas that share a top-level $id', async () => { + it('keeps grammar constraints for schemas with a top-level $id', async () => { const sharedId = 'https://qwen-code.test/shared-tool-schema'; const makeSchema = () => ({ $id: sharedId, @@ -6011,7 +6011,9 @@ describe('OpenAIContentConverter', () => { description: '', parameters: { type: 'object', - properties: { value: { type: 'string' } }, + properties: { + value: { type: 'string', maxLength: 1999 }, + }, }, }, { @@ -6019,7 +6021,9 @@ describe('OpenAIContentConverter', () => { description: '', parameters: { type: 'object', - properties: { value: { type: 'string' } }, + properties: { + value: { type: 'string', maxLength: 1999 }, + }, }, }, ]); diff --git a/packages/core/src/core/openaiContentGenerator/converter.ts b/packages/core/src/core/openaiContentGenerator/converter.ts index ef712f9fb94..2d2df21203d 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.ts @@ -371,9 +371,13 @@ export async function convertLlmToolsToOpenAI( } if (parameters) { + const sourceSchema = func.parametersJsonSchema; const canValidateLocally = - func.parametersJsonSchema !== undefined && - SchemaValidator.compileStrict(func.parametersJsonSchema) === null; + typeof sourceSchema === 'object' && + sourceSchema !== null && + !Array.isArray(sourceSchema) && + !('$id' in sourceSchema) && + SchemaValidator.compileStrict(sourceSchema) === null; parameters = convertSchema(parameters, schemaCompliance); // #7315: gateways enforcing OpenAI's structured-output contract // promote every property to required when an object level has From 65c4ed0c8aeb2ea43e3b9468d6b5edb388d6cc56 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 1 Sep 2026 17:02:41 +0800 Subject: [PATCH 27/29] fix(core): reject nested schema identifiers Co-authored-by: Qwen-Coder --- ...penai-tool-schema-grammar-compatibility.md | 2 +- .../openaiContentGenerator/converter.test.ts | 25 ++++++++++++++----- .../core/openaiContentGenerator/converter.ts | 14 ++++++++++- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md index 45b8d81f6e5..71009b12c9a 100644 --- a/docs/design/openai-tool-schema-grammar-compatibility.md +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -14,7 +14,7 @@ Keep the registered tool set unchanged. Before an OpenAI-compatible request is s - omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above 1999, the lowest failing boundary measured across the four keywords; - preserve smaller limits and all other supported constraints. -Apply these grammar-specific relaxations only when the source schema passes the existing isolated strict compilation for its selected dialect and has no top-level `$id`. Schemas with a top-level `$id` keep their constraints because runtime validation uses a shared schema registry where duplicate IDs can prevent enforcement. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. +Apply these grammar-specific relaxations only when the source schema passes the existing isolated strict compilation for its selected dialect and contains no `$id`. Schemas containing `$id` keep their constraints because runtime validation uses a shared schema registry where duplicate IDs can prevent enforcement. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. The original schema remains attached to the tool and continues to drive client-side parameter validation. The provider receives a schema it can compile, while Qwen Code still rejects tool calls that violate the original limits. diff --git a/packages/core/src/core/openaiContentGenerator/converter.test.ts b/packages/core/src/core/openaiContentGenerator/converter.test.ts index 5c3a1d3065a..9274c64e4f7 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.test.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.test.ts @@ -5985,20 +5985,30 @@ describe('OpenAIContentConverter', () => { ).toBe(false); }); - it('keeps grammar constraints for schemas with a top-level $id', async () => { + it('keeps grammar constraints for schemas containing $id', async () => { const sharedId = 'https://qwen-code.test/shared-tool-schema'; - const makeSchema = () => ({ + const topLevelSchema = { $id: sharedId, type: 'object', properties: { value: { type: 'string', maxLength: 1999 }, }, - }); + }; + const nestedSchema = { + type: 'object', + properties: { + value: { + $id: sharedId, + type: 'string', + maxLength: 1999, + }, + }, + }; const tools = [ { functionDeclarations: [ - { name: 'first', parametersJsonSchema: makeSchema() }, - { name: 'second', parametersJsonSchema: makeSchema() }, + { name: 'first', parametersJsonSchema: topLevelSchema }, + { name: 'second', parametersJsonSchema: nestedSchema }, ], }, ] as Tool[]; @@ -6012,7 +6022,10 @@ describe('OpenAIContentConverter', () => { parameters: { type: 'object', properties: { - value: { type: 'string', maxLength: 1999 }, + value: { + type: 'string', + maxLength: 1999, + }, }, }, }, diff --git a/packages/core/src/core/openaiContentGenerator/converter.ts b/packages/core/src/core/openaiContentGenerator/converter.ts index 2d2df21203d..19bc8706933 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.ts @@ -332,6 +332,18 @@ export function convertLlmToolParametersToOpenAI( * Handles both Gemini tools (using 'parameters' field) and MCP tools * (using 'parametersJsonSchema' field). */ +function containsSchemaId(value: unknown): boolean { + if (typeof value !== 'object' || value === null) { + return false; + } + if (Array.isArray(value)) { + return value.some(containsSchemaId); + } + return Object.entries(value).some( + ([key, nested]) => key === '$id' || containsSchemaId(nested), + ); +} + export async function convertLlmToolsToOpenAI( llmTools: ToolListUnion, schemaCompliance: SchemaComplianceMode = 'auto', @@ -376,7 +388,7 @@ export async function convertLlmToolsToOpenAI( typeof sourceSchema === 'object' && sourceSchema !== null && !Array.isArray(sourceSchema) && - !('$id' in sourceSchema) && + !containsSchemaId(sourceSchema) && SchemaValidator.compileStrict(sourceSchema) === null; parameters = convertSchema(parameters, schemaCompliance); // #7315: gateways enforcing OpenAI's structured-output contract From c45f69053048411c32ea2635bee79be14f36e0d3 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 1 Sep 2026 17:12:42 +0800 Subject: [PATCH 28/29] fix(core): avoid schema identifier false positives Co-authored-by: Qwen-Coder --- ...penai-tool-schema-grammar-compatibility.md | 2 +- .../openaiContentGenerator/converter.test.ts | 25 +++++-------------- .../core/openaiContentGenerator/converter.ts | 14 +---------- 3 files changed, 8 insertions(+), 33 deletions(-) diff --git a/docs/design/openai-tool-schema-grammar-compatibility.md b/docs/design/openai-tool-schema-grammar-compatibility.md index 71009b12c9a..45b8d81f6e5 100644 --- a/docs/design/openai-tool-schema-grammar-compatibility.md +++ b/docs/design/openai-tool-schema-grammar-compatibility.md @@ -14,7 +14,7 @@ Keep the registered tool set unchanged. Before an OpenAI-compatible request is s - omit `minLength`, `maxLength`, `minItems`, and `maxItems` values at or above 1999, the lowest failing boundary measured across the four keywords; - preserve smaller limits and all other supported constraints. -Apply these grammar-specific relaxations only when the source schema passes the existing isolated strict compilation for its selected dialect and contains no `$id`. Schemas containing `$id` keep their constraints because runtime validation uses a shared schema registry where duplicate IDs can prevent enforcement. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. +Apply these grammar-specific relaxations only when the source schema passes the existing isolated strict compilation for its selected dialect and has no top-level `$id`. Schemas with a top-level `$id` keep their constraints because runtime validation uses a shared schema registry where duplicate IDs can prevent enforcement. If local validation cannot enforce the complete schema, keep its grammar constraints on the wire rather than broadening both enforcement layers. The original schema remains attached to the tool and continues to drive client-side parameter validation. The provider receives a schema it can compile, while Qwen Code still rejects tool calls that violate the original limits. diff --git a/packages/core/src/core/openaiContentGenerator/converter.test.ts b/packages/core/src/core/openaiContentGenerator/converter.test.ts index 9274c64e4f7..5c3a1d3065a 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.test.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.test.ts @@ -5985,30 +5985,20 @@ describe('OpenAIContentConverter', () => { ).toBe(false); }); - it('keeps grammar constraints for schemas containing $id', async () => { + it('keeps grammar constraints for schemas with a top-level $id', async () => { const sharedId = 'https://qwen-code.test/shared-tool-schema'; - const topLevelSchema = { + const makeSchema = () => ({ $id: sharedId, type: 'object', properties: { value: { type: 'string', maxLength: 1999 }, }, - }; - const nestedSchema = { - type: 'object', - properties: { - value: { - $id: sharedId, - type: 'string', - maxLength: 1999, - }, - }, - }; + }); const tools = [ { functionDeclarations: [ - { name: 'first', parametersJsonSchema: topLevelSchema }, - { name: 'second', parametersJsonSchema: nestedSchema }, + { name: 'first', parametersJsonSchema: makeSchema() }, + { name: 'second', parametersJsonSchema: makeSchema() }, ], }, ] as Tool[]; @@ -6022,10 +6012,7 @@ describe('OpenAIContentConverter', () => { parameters: { type: 'object', properties: { - value: { - type: 'string', - maxLength: 1999, - }, + value: { type: 'string', maxLength: 1999 }, }, }, }, diff --git a/packages/core/src/core/openaiContentGenerator/converter.ts b/packages/core/src/core/openaiContentGenerator/converter.ts index 19bc8706933..2d2df21203d 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.ts @@ -332,18 +332,6 @@ export function convertLlmToolParametersToOpenAI( * Handles both Gemini tools (using 'parameters' field) and MCP tools * (using 'parametersJsonSchema' field). */ -function containsSchemaId(value: unknown): boolean { - if (typeof value !== 'object' || value === null) { - return false; - } - if (Array.isArray(value)) { - return value.some(containsSchemaId); - } - return Object.entries(value).some( - ([key, nested]) => key === '$id' || containsSchemaId(nested), - ); -} - export async function convertLlmToolsToOpenAI( llmTools: ToolListUnion, schemaCompliance: SchemaComplianceMode = 'auto', @@ -388,7 +376,7 @@ export async function convertLlmToolsToOpenAI( typeof sourceSchema === 'object' && sourceSchema !== null && !Array.isArray(sourceSchema) && - !containsSchemaId(sourceSchema) && + !('$id' in sourceSchema) && SchemaValidator.compileStrict(sourceSchema) === null; parameters = convertSchema(parameters, schemaCompliance); // #7315: gateways enforcing OpenAI's structured-output contract From 337e4aef029e5275ba37ad93c37f8263d029202a Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Wed, 2 Sep 2026 19:37:54 +0800 Subject: [PATCH 29/29] perf(core): cache tool schema validation Co-authored-by: Qwen-Coder --- .../openaiContentGenerator/converter.test.ts | 28 ++++++++++++++++++- .../core/openaiContentGenerator/converter.ts | 14 +++++++++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/packages/core/src/core/openaiContentGenerator/converter.test.ts b/packages/core/src/core/openaiContentGenerator/converter.test.ts index 8b66ffed288..1a59786b5b8 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.test.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { OpenAIContentConverter } from './converter.js'; import { StreamingToolCallParser } from './streamingToolCallParser.js'; import { TaggedThinkingParser } from './taggedThinkingParser.js'; @@ -24,6 +24,7 @@ import { convertToFunctionResponse } from '../coreToolScheduler.js'; import { getToolCallPreparations } from '../tool-call-preparation.js'; import { isOpenAIReasoningThoughtPart } from '../../utils/thoughtUtils.js'; import { getGenAiUsageProvenance } from '../../telemetry/gen-ai-usage.js'; +import { SchemaValidator } from '../../utils/schemaValidator.js'; describe('OpenAIContentConverter', () => { let converter: typeof OpenAIContentConverter; @@ -6035,6 +6036,31 @@ describe('OpenAIContentConverter', () => { }); describe('convertLlmToolsToOpenAI', () => { + it('compiles a stable tool schema only once', async () => { + const parametersJsonSchema = { + type: 'object', + properties: { + value: { type: 'string', maxLength: 1999 }, + }, + }; + const tools = [ + { + functionDeclarations: [ + { name: 'stable', parametersJsonSchema }, + ], + }, + ] as Tool[]; + const compileStrict = vi.spyOn(SchemaValidator, 'compileStrict'); + + try { + await converter.convertLlmToolsToOpenAI(tools); + await converter.convertLlmToolsToOpenAI(tools); + expect(compileStrict).toHaveBeenCalledTimes(1); + } finally { + compileStrict.mockRestore(); + } + }); + it('removes uniqueItems from function-calling wire schemas', async () => { const parametersJsonSchema = { type: 'object', diff --git a/packages/core/src/core/openaiContentGenerator/converter.ts b/packages/core/src/core/openaiContentGenerator/converter.ts index afe968825f6..3068fa05f73 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.ts @@ -335,6 +335,18 @@ export function convertLlmToolParametersToOpenAI( * Handles both Gemini tools (using 'parameters' field) and MCP tools * (using 'parametersJsonSchema' field). */ +const grammarSchemaValidationCache = new WeakMap(); + +function isStrictlyValidSchema(schema: object): boolean { + const cached = grammarSchemaValidationCache.get(schema); + if (cached !== undefined) { + return cached; + } + const valid = SchemaValidator.compileStrict(schema) === null; + grammarSchemaValidationCache.set(schema, valid); + return valid; +} + export async function convertLlmToolsToOpenAI( llmTools: ToolListUnion, schemaCompliance: SchemaComplianceMode = 'auto', @@ -380,7 +392,7 @@ export async function convertLlmToolsToOpenAI( sourceSchema !== null && !Array.isArray(sourceSchema) && !('$id' in sourceSchema) && - SchemaValidator.compileStrict(sourceSchema) === null; + isStrictlyValidSchema(sourceSchema); parameters = convertSchema(parameters, schemaCompliance); // #7315: gateways enforcing OpenAI's structured-output contract // promote every property to required when an object level has