vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-04-29 04:00:36 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 24

fix(tool-registry): add lazy factory registration with inflight concurrency dedup (#3297)

Browse source 
Closes #3221.

Introduces a lazy factory API on ToolRegistry (registerFactory,
ensureTool, warmAll, getAllToolNames) as infrastructure for future
esbuild code-splitting (#3226). With the current single-bundle build,
the lazy API does not change startup time on its own — the primary
immediate value is fixing three pre-existing bugs uncovered while
designing it.

Bug fixes:

- Concurrent instantiation (P0): the original ensureTool had no
  concurrency protection around `await factory()` — two concurrent
  calls for the same tool both passed the cache check and each ran the
  factory, producing two instances. AgentTool and SkillTool register
  SubagentManager listeners in their constructors, so the extra
  instance leaked listeners. Fix: a per-name `inflight: Map<string,
  Promise<Tool>>` so concurrent ensureTool() calls share a single
  promise. On factory rejection the inflight entry is cleared so a
  subsequent call can retry.

- stop() resource leak: stop() only disposed tools already in
  `this.tools`; tools still loading in `inflight` when stop() ran
  finished afterward and were never disposed. Fix: await
  Promise.allSettled(inflight.values()) before the dispose loop.

- Cache hit left stale factory: ensureTool's cache-hit branch did not
  delete the factory entry, so warmAll() would re-invoke the factory
  for an already-loaded tool. Fix: delete the factory on cache hit.

Additional hardening in response to review feedback:

- warmAll({ strict?: boolean }): strict mode re-throws the first
  factory failure rather than swallowing it. Config.initialize() uses
  strict: true so a broken built-in tool fails startup fast instead of
  silently leaving a partially initialized registry; runtime-path
  callers (GeminiChat, agent runtime, etc.) continue to use the
  non-strict default and log failures via debugLogger.
- getAllTools() and getFunctionDeclarationsFiltered() emit a debug
  warning when called while unloaded factories remain, nudging callers
  toward warmAll() without hard-breaking existing code paths.
- copyDiscoveredToolsFrom() now iterates source.tools.values()
  directly instead of source.getAllTools() — the copy path deals only
  with already-discovered MCP/command tools and should not trigger the
  unloaded-factory warning.
- MemoryTool and SkillTool config parsing was extracted into
  memory-config.ts and skill-utils.ts so a factory can resolve tool
  metadata without importing the tool module.

Tests:

- tool-registry.test.ts adds 128 lines covering: concurrent ensureTool
  runs the factory exactly once, warmAll and ensureTool overlap,
  retries succeed after a prior factory failure, stop() disposes tools
  that finish loading after stop was called, and warmAll strict vs
  default behavior.
- 33 existing call sites across cli, core, agents, and subagents were
  updated to await warmAll() before bulk tool access.
This commit is contained in:
jinye 2026-04-18 10:31:50 +08:00 • committed by GitHub
parent 5facd8738b
commit 9f4734e84d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
35 changed files with 739 additions and 330 deletions
Download patch file Download diff file Expand all files Collapse all files

74
packages/cli/src/config/config.test.ts
View file

@ -8,9 +8,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import * as os from 'node:os';
import * as path from 'node:path';
import {
ShellTool,
EditTool,
WriteFileTool,
ToolNames,
DEFAULT_QWEN_MODEL,
OutputFormat,
NativeLspService,
@ -1020,7 +1018,11 @@ describe('loadCliConfig telemetry', () => {
});
describe('mergeExcludeTools', () => {
const defaultExcludes = [ShellTool.Name, EditTool.Name, WriteFileTool.Name];
const defaultExcludes = [
ToolNames.SHELL,
ToolNames.EDIT,
ToolNames.WRITE_FILE,
];
const originalIsTTY = process.stdin.isTTY;
beforeEach(() => {
@ -1083,9 +1085,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).toContain(ShellTool.Name);
expect(excludedTools).toContain(EditTool.Name);
expect(excludedTools).toContain(WriteFileTool.Name);
expect(excludedTools).toContain(ToolNames.SHELL);
expect(excludedTools).toContain(ToolNames.EDIT);
expect(excludedTools).toContain(ToolNames.WRITE_FILE);
});
it('should exclude all interactive tools in non-interactive mode with plan approval mode', async () => {
@ -1102,9 +1104,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).toContain(ShellTool.Name);
expect(excludedTools).toContain(EditTool.Name);
expect(excludedTools).toContain(WriteFileTool.Name);
expect(excludedTools).toContain(ToolNames.SHELL);
expect(excludedTools).toContain(ToolNames.EDIT);
expect(excludedTools).toContain(ToolNames.WRITE_FILE);
});
it('should exclude all interactive tools in non-interactive mode with explicit default approval mode', async () => {
@ -1122,9 +1124,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).toContain(ShellTool.Name);
expect(excludedTools).toContain(EditTool.Name);
expect(excludedTools).toContain(WriteFileTool.Name);
expect(excludedTools).toContain(ToolNames.SHELL);
expect(excludedTools).toContain(ToolNames.EDIT);
expect(excludedTools).toContain(ToolNames.WRITE_FILE);
});
it('should not exclude a tool explicitly allowed in tools.allowed', async () => {
@ -1132,16 +1134,16 @@ describe('Approval mode tool exclusion logic', () => {
const argv = await parseArguments();
const settings: Settings = {
tools: {
allowed: [ShellTool.Name],
allowed: [ToolNames.SHELL],
},
};
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).not.toContain(ShellTool.Name);
expect(excludedTools).toContain(EditTool.Name);
expect(excludedTools).toContain(WriteFileTool.Name);
expect(excludedTools).not.toContain(ToolNames.SHELL);
expect(excludedTools).toContain(ToolNames.EDIT);
expect(excludedTools).toContain(ToolNames.WRITE_FILE);
});
it('should not exclude a tool explicitly allowed in tools.core', async () => {
@ -1149,16 +1151,16 @@ describe('Approval mode tool exclusion logic', () => {
const argv = await parseArguments();
const settings: Settings = {
tools: {
core: [ShellTool.Name],
core: [ToolNames.SHELL],
},
};
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).not.toContain(ShellTool.Name);
expect(excludedTools).toContain(EditTool.Name);
expect(excludedTools).toContain(WriteFileTool.Name);
expect(excludedTools).not.toContain(ToolNames.SHELL);
expect(excludedTools).toContain(ToolNames.EDIT);
expect(excludedTools).toContain(ToolNames.WRITE_FILE);
});
it('should exclude only shell tools in non-interactive mode with auto-edit approval mode', async () => {
@ -1176,9 +1178,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).toContain(ShellTool.Name);
expect(excludedTools).not.toContain(EditTool.Name);
expect(excludedTools).not.toContain(WriteFileTool.Name);
expect(excludedTools).toContain(ToolNames.SHELL);
expect(excludedTools).not.toContain(ToolNames.EDIT);
expect(excludedTools).not.toContain(ToolNames.WRITE_FILE);
});
it('should exclude no interactive tools in non-interactive mode with yolo approval mode', async () => {
@ -1196,9 +1198,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).not.toContain(ShellTool.Name);
expect(excludedTools).not.toContain(EditTool.Name);
expect(excludedTools).not.toContain(WriteFileTool.Name);
expect(excludedTools).not.toContain(ToolNames.SHELL);
expect(excludedTools).not.toContain(ToolNames.EDIT);
expect(excludedTools).not.toContain(ToolNames.WRITE_FILE);
});
it('should exclude no interactive tools in non-interactive mode with legacy yolo flag', async () => {
@ -1209,9 +1211,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).not.toContain(ShellTool.Name);
expect(excludedTools).not.toContain(EditTool.Name);
expect(excludedTools).not.toContain(WriteFileTool.Name);
expect(excludedTools).not.toContain(ToolNames.SHELL);
expect(excludedTools).not.toContain(ToolNames.EDIT);
expect(excludedTools).not.toContain(ToolNames.WRITE_FILE);
});
it('should not exclude interactive tools in interactive mode regardless of approval mode', async () => {
@ -1234,9 +1236,9 @@ describe('Approval mode tool exclusion logic', () => {
const config = await loadCliConfig(settings, argv, undefined, []);
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).not.toContain(ShellTool.Name);
expect(excludedTools).not.toContain(EditTool.Name);
expect(excludedTools).not.toContain(WriteFileTool.Name);
expect(excludedTools).not.toContain(ToolNames.SHELL);
expect(excludedTools).not.toContain(ToolNames.EDIT);
expect(excludedTools).not.toContain(ToolNames.WRITE_FILE);
}
});
@ -1255,9 +1257,9 @@ describe('Approval mode tool exclusion logic', () => {
const excludedTools = config.getPermissionsDeny();
expect(excludedTools).toContain('custom_tool'); // From settings
expect(excludedTools).toContain(ShellTool.Name); // From approval mode
expect(excludedTools).not.toContain(EditTool.Name); // Should be allowed in auto-edit
expect(excludedTools).not.toContain(WriteFileTool.Name); // Should be allowed in auto-edit
expect(excludedTools).toContain(ToolNames.SHELL); // From approval mode
expect(excludedTools).not.toContain(ToolNames.EDIT); // Should be allowed in auto-edit
expect(excludedTools).not.toContain(ToolNames.WRITE_FILE); // Should be allowed in auto-edit
});
it('should throw an error for invalid approval mode values in loadCliConfig', async () => {

12
packages/cli/src/config/config.ts
View file

@ -24,9 +24,7 @@ import {
type ResumedSessionData,
type LspClient,
type ToolName,
EditTool,
ShellTool,
WriteFileTool,
ToolNames,
NativeLspClient,
createDebugLogger,
NativeLspService,
@ -955,13 +953,13 @@ export async function loadCliConfig(
case ApprovalMode.PLAN:
case ApprovalMode.DEFAULT:
// Deny all write/execute tools unless explicitly allowed.
denyUnlessAllowed(ShellTool.Name as ToolName);
denyUnlessAllowed(EditTool.Name as ToolName);
denyUnlessAllowed(WriteFileTool.Name as ToolName);
denyUnlessAllowed(ToolNames.SHELL as ToolName);
denyUnlessAllowed(ToolNames.EDIT as ToolName);
denyUnlessAllowed(ToolNames.WRITE_FILE as ToolName);
break;
case ApprovalMode.AUTO_EDIT:
// Only shell requires a prompt in auto-edit mode.
denyUnlessAllowed(ShellTool.Name as ToolName);
denyUnlessAllowed(ToolNames.SHELL as ToolName);
break;
case ApprovalMode.YOLO:
// No extra denials for YOLO mode.