From 06141cda8dc964877e1cf0772e3715a1ea57f125 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 12 Nov 2025 19:46:05 +0800 Subject: [PATCH 01/10] Refactor: Standardize Tool Naming and Configuration System (#1004) --- docs/cli/configuration.md | 2 +- docs/core/tools-api.md | 28 +-- docs/tools/file-system.md | 8 +- packages/core/src/config/config.test.ts | 201 ++++++++++++++++-- packages/core/src/config/config.ts | 49 +++-- .../core/src/subagents/subagent-manager.ts | 8 +- packages/core/src/tools/edit.ts | 4 +- packages/core/src/tools/exitPlanMode.ts | 5 +- packages/core/src/tools/glob.ts | 4 +- packages/core/src/tools/grep.ts | 4 +- packages/core/src/tools/ls.ts | 5 +- packages/core/src/tools/memoryTool.ts | 5 +- packages/core/src/tools/read-file.ts | 4 +- packages/core/src/tools/read-many-files.ts | 4 +- packages/core/src/tools/shell.ts | 4 +- packages/core/src/tools/task.ts | 4 +- packages/core/src/tools/todoWrite.ts | 5 +- packages/core/src/tools/tool-names.ts | 39 ++++ packages/core/src/tools/web-fetch.ts | 4 +- packages/core/src/tools/web-search/index.ts | 4 +- packages/core/src/tools/write-file.ts | 4 +- packages/core/src/utils/tool-utils.test.ts | 67 +++++- packages/core/src/utils/tool-utils.ts | 105 +++++++++ 23 files changed, 480 insertions(+), 87 deletions(-) diff --git a/docs/cli/configuration.md b/docs/cli/configuration.md index d42c24dd..f1c74e3e 100644 --- a/docs/cli/configuration.md +++ b/docs/cli/configuration.md @@ -578,7 +578,7 @@ Arguments passed directly when running the CLI can override other configurations - Example: `qwen --approval-mode auto-edit` - **`--allowed-tools `**: - A comma-separated list of tool names that will bypass the confirmation dialog. - - Example: `qwen --allowed-tools "ShellTool(git status)"` + - Example: `qwen --allowed-tools "Shell(git status)"` - **`--telemetry`**: - Enables [telemetry](../telemetry.md). - **`--telemetry-target`**: diff --git a/docs/core/tools-api.md b/docs/core/tools-api.md index 4d788a68..70266b88 100644 --- a/docs/core/tools-api.md +++ b/docs/core/tools-api.md @@ -21,7 +21,7 @@ The Qwen Code core (`packages/core`) features a robust system for defining, regi - **Returning Rich Content:** Tools are not limited to returning simple text. The `llmContent` can be a `PartListUnion`, which is an array that can contain a mix of `Part` objects (for images, audio, etc.) and `string`s. This allows a single tool execution to return multiple pieces of rich content. - **Tool Registry (`tool-registry.ts`):** A class (`ToolRegistry`) responsible for: - - **Registering Tools:** Holding a collection of all available built-in tools (e.g., `ReadFileTool`, `ShellTool`). + - **Registering Tools:** Holding a collection of all available built-in tools (e.g., `ListFiles`, `ReadFile`). - **Discovering Tools:** It can also discover tools dynamically: - **Command-based Discovery:** If `tools.toolDiscoveryCommand` is configured in settings, this command is executed. It's expected to output JSON describing custom tools, which are then registered as `DiscoveredTool` instances. - **MCP-based Discovery:** If `mcp.mcpServerCommand` is configured, the registry can connect to a Model Context Protocol (MCP) server to list and register tools (`DiscoveredMCPTool`). @@ -33,20 +33,24 @@ The Qwen Code core (`packages/core`) features a robust system for defining, regi The core comes with a suite of pre-defined tools, typically found in `packages/core/src/tools/`. These include: - **File System Tools:** - - `LSTool` (`ls.ts`): Lists directory contents. - - `ReadFileTool` (`read-file.ts`): Reads the content of a single file. It takes an `absolute_path` parameter, which must be an absolute path. - - `WriteFileTool` (`write-file.ts`): Writes content to a file. - - `GrepTool` (`grep.ts`): Searches for patterns in files. - - `GlobTool` (`glob.ts`): Finds files matching glob patterns. - - `EditTool` (`edit.ts`): Performs in-place modifications to files (often requiring confirmation). - - `ReadManyFilesTool` (`read-many-files.ts`): Reads and concatenates content from multiple files or glob patterns (used by the `@` command in CLI). + - `ListFiles` (`ls.ts`): Lists directory contents. + - `ReadFile` (`read-file.ts`): Reads the content of a single file. It takes an `absolute_path` parameter, which must be an absolute path. + - `WriteFile` (`write-file.ts`): Writes content to a file. + - `ReadManyFiles` (`read-many-files.ts`): Reads and concatenates content from multiple files or glob patterns (used by the `@` command in CLI). + - `Grep` (`grep.ts`): Searches for patterns in files. + - `Glob` (`glob.ts`): Finds files matching glob patterns. + - `Edit` (`edit.ts`): Performs in-place modifications to files (often requiring confirmation). - **Execution Tools:** - - `ShellTool` (`shell.ts`): Executes arbitrary shell commands (requires careful sandboxing and user confirmation). + - `Shell` (`shell.ts`): Executes arbitrary shell commands (requires careful sandboxing and user confirmation). - **Web Tools:** - - `WebFetchTool` (`web-fetch.ts`): Fetches content from a URL. - - `WebSearchTool` (`web-search.ts`): Performs a web search. + - `WebFetch` (`web-fetch.ts`): Fetches content from a URL. + - `WebSearch` (`web-search.ts`): Performs a web search. - **Memory Tools:** - - `MemoryTool` (`memoryTool.ts`): Interacts with the AI's memory. + - `SaveMemory` (`memoryTool.ts`): Interacts with the AI's memory. +- **Planning Tools:** + - `Task` (`task.ts`): Delegates tasks to specialized subagents. + - `TodoWrite` (`todoWrite.ts`): Creates and manages a structured task list. + - `ExitPlanMode` (`exitPlanMode.ts`): Exits plan mode and returns to normal operation. Each of these tools extends `BaseTool` and implements the required methods for its specific functionality. diff --git a/docs/tools/file-system.md b/docs/tools/file-system.md index 7ce38359..93b6ac85 100644 --- a/docs/tools/file-system.md +++ b/docs/tools/file-system.md @@ -4,12 +4,12 @@ Qwen Code provides a comprehensive suite of tools for interacting with the local **Note:** All file system tools operate within a `rootDirectory` (usually the current working directory where you launched the CLI) for security. Paths that you provide to these tools are generally expected to be absolute or are resolved relative to this root directory. -## 1. `list_directory` (ReadFolder) +## 1. `list_directory` (ListFiles) `list_directory` lists the names of files and subdirectories directly within a specified directory path. It can optionally ignore entries matching provided glob patterns. - **Tool name:** `list_directory` -- **Display name:** ReadFolder +- **Display name:** ListFiles - **File:** `ls.ts` - **Parameters:** - `path` (string, required): The absolute path to the directory to list. @@ -59,12 +59,12 @@ Qwen Code provides a comprehensive suite of tools for interacting with the local - **Output (`llmContent`):** A success message, e.g., `Successfully overwrote file: /path/to/your/file.txt` or `Successfully created and wrote to new file: /path/to/new/file.txt`. - **Confirmation:** Yes. Shows a diff of changes and asks for user approval before writing. -## 4. `glob` (FindFiles) +## 4. `glob` (Glob) `glob` finds files matching specific glob patterns (e.g., `src/**/*.ts`, `*.md`), returning absolute paths sorted by modification time (newest first). - **Tool name:** `glob` -- **Display name:** FindFiles +- **Display name:** Glob - **File:** `glob.ts` - **Parameters:** - `pattern` (string, required): The glob pattern to match against (e.g., `"*.py"`, `"src/**/*.js"`). diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 6f07de53..15ef951b 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -45,6 +45,15 @@ import { logRipgrepFallback } from '../telemetry/loggers.js'; import { RipgrepFallbackEvent } from '../telemetry/types.js'; import { ToolRegistry } from '../tools/tool-registry.js'; +function createToolMock(toolName: string) { + const ToolMock = vi.fn(); + Object.defineProperty(ToolMock, 'Name', { + value: toolName, + writable: true, + }); + return ToolMock; +} + vi.mock('fs', async (importOriginal) => { const actual = await importOriginal(); return { @@ -73,23 +82,41 @@ vi.mock('../utils/memoryDiscovery.js', () => ({ })); // Mock individual tools if their constructors are complex or have side effects -vi.mock('../tools/ls'); -vi.mock('../tools/read-file'); -vi.mock('../tools/grep.js'); +vi.mock('../tools/ls', () => ({ + LSTool: createToolMock('list_directory'), +})); +vi.mock('../tools/read-file', () => ({ + ReadFileTool: createToolMock('read_file'), +})); +vi.mock('../tools/grep.js', () => ({ + GrepTool: createToolMock('grep_search'), +})); vi.mock('../tools/ripGrep.js', () => ({ - RipGrepTool: class MockRipGrepTool {}, + RipGrepTool: createToolMock('grep_search'), })); vi.mock('../utils/ripgrepUtils.js', () => ({ canUseRipgrep: vi.fn(), })); -vi.mock('../tools/glob'); -vi.mock('../tools/edit'); -vi.mock('../tools/shell'); -vi.mock('../tools/write-file'); -vi.mock('../tools/web-fetch'); -vi.mock('../tools/read-many-files'); +vi.mock('../tools/glob', () => ({ + GlobTool: createToolMock('glob'), +})); +vi.mock('../tools/edit', () => ({ + EditTool: createToolMock('edit'), +})); +vi.mock('../tools/shell', () => ({ + ShellTool: createToolMock('run_shell_command'), +})); +vi.mock('../tools/write-file', () => ({ + WriteFileTool: createToolMock('write_file'), +})); +vi.mock('../tools/web-fetch', () => ({ + WebFetchTool: createToolMock('web_fetch'), +})); +vi.mock('../tools/read-many-files', () => ({ + ReadManyFilesTool: createToolMock('read_many_files'), +})); vi.mock('../tools/memoryTool', () => ({ - MemoryTool: vi.fn(), + MemoryTool: createToolMock('save_memory'), setGeminiMdFilename: vi.fn(), getCurrentGeminiMdFilename: vi.fn(() => 'QWEN.md'), // Mock the original filename DEFAULT_CONTEXT_FILENAME: 'QWEN.md', @@ -621,7 +648,7 @@ describe('Server Config (config.ts)', () => { it('should register a tool if coreTools contains an argument-specific pattern', async () => { const params: ConfigParameters = { ...baseParams, - coreTools: ['ShellTool(git status)'], + coreTools: ['Shell(git status)'], // Use display name instead of class name }; const config = new Config(params); await config.initialize(); @@ -646,6 +673,89 @@ describe('Server Config (config.ts)', () => { expect(wasReadFileToolRegistered).toBe(false); }); + it('should register a tool if coreTools contains the displayName', async () => { + const params: ConfigParameters = { + ...baseParams, + coreTools: ['Shell'], + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasShellToolRegistered = (registerToolMock as Mock).mock.calls.some( + (call) => call[0] instanceof vi.mocked(ShellTool), + ); + expect(wasShellToolRegistered).toBe(true); + }); + + it('should register a tool if coreTools contains the displayName with argument-specific pattern', async () => { + const params: ConfigParameters = { + ...baseParams, + coreTools: ['Shell(git status)'], + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasShellToolRegistered = (registerToolMock as Mock).mock.calls.some( + (call) => call[0] instanceof vi.mocked(ShellTool), + ); + expect(wasShellToolRegistered).toBe(true); + }); + + it('should register a tool if coreTools contains a legacy tool name alias', async () => { + const params: ConfigParameters = { + ...baseParams, + useRipgrep: false, + coreTools: ['search_file_content'], + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasGrepToolRegistered = (registerToolMock as Mock).mock.calls.some( + (call) => call[0] instanceof vi.mocked(GrepTool), + ); + expect(wasGrepToolRegistered).toBe(true); + }); + + it('should not register a tool if excludeTools contains a legacy display name alias', async () => { + const params: ConfigParameters = { + ...baseParams, + useRipgrep: false, + coreTools: undefined, + excludeTools: ['SearchFiles'], + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasGrepToolRegistered = (registerToolMock as Mock).mock.calls.some( + (call) => call[0] instanceof vi.mocked(GrepTool), + ); + expect(wasGrepToolRegistered).toBe(false); + }); + describe('with minified tool class names', () => { beforeEach(() => { Object.defineProperty( @@ -671,7 +781,27 @@ describe('Server Config (config.ts)', () => { it('should register a tool if coreTools contains the non-minified class name', async () => { const params: ConfigParameters = { ...baseParams, - coreTools: ['ShellTool'], + coreTools: ['Shell'], // Use display name instead of class name + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasShellToolRegistered = ( + registerToolMock as Mock + ).mock.calls.some((call) => call[0] instanceof vi.mocked(ShellTool)); + expect(wasShellToolRegistered).toBe(true); + }); + + it('should register a tool if coreTools contains the displayName', async () => { + const params: ConfigParameters = { + ...baseParams, + coreTools: ['Shell'], }; const config = new Config(params); await config.initialize(); @@ -692,7 +822,28 @@ describe('Server Config (config.ts)', () => { const params: ConfigParameters = { ...baseParams, coreTools: undefined, // all tools enabled by default - excludeTools: ['ShellTool'], + excludeTools: ['Shell'], // Use display name instead of class name + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasShellToolRegistered = ( + registerToolMock as Mock + ).mock.calls.some((call) => call[0] instanceof vi.mocked(ShellTool)); + expect(wasShellToolRegistered).toBe(false); + }); + + it('should not register a tool if excludeTools contains the displayName', async () => { + const params: ConfigParameters = { + ...baseParams, + coreTools: undefined, // all tools enabled by default + excludeTools: ['Shell'], }; const config = new Config(params); await config.initialize(); @@ -712,7 +863,27 @@ describe('Server Config (config.ts)', () => { it('should register a tool if coreTools contains an argument-specific pattern with the non-minified class name', async () => { const params: ConfigParameters = { ...baseParams, - coreTools: ['ShellTool(git status)'], + coreTools: ['Shell(git status)'], // Use display name instead of class name + }; + const config = new Config(params); + await config.initialize(); + + const registerToolMock = ( + (await vi.importMock('../tools/tool-registry')) as { + ToolRegistry: { prototype: { registerTool: Mock } }; + } + ).ToolRegistry.prototype.registerTool; + + const wasShellToolRegistered = ( + registerToolMock as Mock + ).mock.calls.some((call) => call[0] instanceof vi.mocked(ShellTool)); + expect(wasShellToolRegistered).toBe(true); + }); + + it('should register a tool if coreTools contains an argument-specific pattern with the displayName', async () => { + const params: ConfigParameters = { + ...baseParams, + coreTools: ['Shell(git status)'], }; const config = new Config(params); await config.initialize(); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 68e9521a..93a65035 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -81,6 +81,7 @@ import { import { shouldAttemptBrowserLaunch } from '../utils/browser.js'; import { FileExclusions } from '../utils/ignorePatterns.js'; import { WorkspaceContext } from '../utils/workspaceContext.js'; +import { isToolEnabled, type ToolName } from '../utils/tool-utils.js'; // Local config modules import type { FileFilteringOptions } from './constants.js'; @@ -1110,37 +1111,35 @@ export class Config { async createToolRegistry(): Promise { const registry = new ToolRegistry(this, this.eventEmitter); - // helper to create & register core tools that are enabled + const coreToolsConfig = this.getCoreTools(); + const excludeToolsConfig = this.getExcludeTools(); + + // Helper to create & register core tools that are enabled // eslint-disable-next-line @typescript-eslint/no-explicit-any const registerCoreTool = (ToolClass: any, ...args: unknown[]) => { - const className = ToolClass.name; - const toolName = ToolClass.Name || className; - const coreTools = this.getCoreTools(); - const excludeTools = this.getExcludeTools() || []; - // On some platforms, the className can be minified to _ClassName. - const normalizedClassName = className.replace(/^_+/, ''); + const toolName = ToolClass?.Name as ToolName | undefined; + const className = ToolClass?.name ?? 'UnknownTool'; - let isEnabled = true; // Enabled by default if coreTools is not set. - if (coreTools) { - isEnabled = coreTools.some( - (tool) => - tool === toolName || - tool === normalizedClassName || - tool.startsWith(`${toolName}(`) || - tool.startsWith(`${normalizedClassName}(`), + if (!toolName) { + // Log warning and skip this tool instead of crashing + console.warn( + `[Config] Skipping tool registration: ${className} is missing static Name property. ` + + `Tools must define a static Name property to be registered. ` + + `Location: config.ts:registerCoreTool`, ); + return; } - const isExcluded = excludeTools.some( - (tool) => tool === toolName || tool === normalizedClassName, - ); - - if (isExcluded) { - isEnabled = false; - } - - if (isEnabled) { - registry.registerTool(new ToolClass(...args)); + if (isToolEnabled(toolName, coreToolsConfig, excludeToolsConfig)) { + try { + registry.registerTool(new ToolClass(...args)); + } catch (error) { + console.error( + `[Config] Failed to register tool ${className} (${toolName}):`, + error, + ); + throw error; // Re-throw after logging context + } } }; diff --git a/packages/core/src/subagents/subagent-manager.ts b/packages/core/src/subagents/subagent-manager.ts index fdb15881..8dcab0de 100644 --- a/packages/core/src/subagents/subagent-manager.ts +++ b/packages/core/src/subagents/subagent-manager.ts @@ -29,6 +29,7 @@ import { SubagentValidator } from './validation.js'; import { SubAgentScope } from './subagent.js'; import type { Config } from '../config/config.js'; import { BuiltinAgentRegistry } from './builtin-agents.js'; +import { ToolDisplayNamesMigration } from '../tools/tool-names.js'; const QWEN_CONFIG_DIR = '.qwen'; const AGENT_CONFIG_DIR = 'agents'; @@ -632,7 +633,12 @@ export class SubagentManager { // If no exact name match, try to find by display name const displayNameMatch = allTools.find( - (tool) => tool.displayName === toolIdentifier, + (tool) => + tool.displayName === toolIdentifier || + tool.displayName === + (ToolDisplayNamesMigration[ + toolIdentifier as keyof typeof ToolDisplayNamesMigration + ] as string | undefined), ); if (displayNameMatch) { result.push(displayNameMatch.name); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index bd68b105..fa94fda5 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -22,7 +22,7 @@ import type { Config } from '../config/config.js'; import { ApprovalMode } from '../config/config.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; import { ReadFileTool } from './read-file.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { logFileOperation } from '../telemetry/loggers.js'; import { FileOperationEvent } from '../telemetry/types.js'; import { FileOperation } from '../telemetry/metrics.js'; @@ -469,7 +469,7 @@ export class EditTool constructor(private readonly config: Config) { super( EditTool.Name, - 'Edit', + ToolDisplayNames.EDIT, `Replaces text within a file. By default, replaces a single occurrence, but can replace multiple occurrences when \`expected_replacements\` is specified. This tool requires providing significant context around the change to ensure precise targeting. Always use the ${ReadFileTool.Name} tool to examine the file's current content before attempting a text replacement. The user has the ability to modify the \`new_string\` content. If modified, this will be stated in the response. diff --git a/packages/core/src/tools/exitPlanMode.ts b/packages/core/src/tools/exitPlanMode.ts index d39b8bbe..6ff7d176 100644 --- a/packages/core/src/tools/exitPlanMode.ts +++ b/packages/core/src/tools/exitPlanMode.ts @@ -14,6 +14,7 @@ import { import type { FunctionDeclaration } from '@google/genai'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../config/config.js'; +import { ToolDisplayNames, ToolNames } from './tool-names.js'; export interface ExitPlanModeParams { plan: string; @@ -152,12 +153,12 @@ export class ExitPlanModeTool extends BaseDeclarativeTool< ExitPlanModeParams, ToolResult > { - static readonly Name: string = exitPlanModeToolSchemaData.name!; + static readonly Name: string = ToolNames.EXIT_PLAN_MODE; constructor(private readonly config: Config) { super( ExitPlanModeTool.Name, - 'ExitPlanMode', + ToolDisplayNames.EXIT_PLAN_MODE, exitPlanModeToolDescription, Kind.Think, exitPlanModeToolSchemaData.parametersJsonSchema as Record< diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index b717ca3e..29b6cf86 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -9,7 +9,7 @@ import path from 'node:path'; import { glob, escape } from 'glob'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { resolveAndValidatePath } from '../utils/paths.js'; import { type Config } from '../config/config.js'; import { @@ -229,7 +229,7 @@ export class GlobTool extends BaseDeclarativeTool { constructor(private config: Config) { super( GlobTool.Name, - 'FindFiles', + ToolDisplayNames.GLOB, 'Fast file pattern matching tool that works with any codebase size\n- Supports glob patterns like "**/*.js" or "src/**/*.ts"\n- Returns matching file paths sorted by modification time\n- Use this tool when you need to find files by name patterns\n- When you are doing an open ended search that may require multiple rounds of globbing and grepping, use the Agent tool instead\n- You have the capability to call multiple tools in a single response. It is always better to speculatively perform multiple searches as a batch that are potentially useful.', Kind.Search, { diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index f8e19451..df410f0c 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -11,7 +11,7 @@ import { spawn } from 'node:child_process'; import { globStream } from 'glob'; import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { resolveAndValidatePath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { isGitRepository } from '../utils/gitUtils.js'; @@ -522,7 +522,7 @@ export class GrepTool extends BaseDeclarativeTool { constructor(private readonly config: Config) { super( GrepTool.Name, - 'Grep', + ToolDisplayNames.GREP, 'A powerful search tool for finding patterns in files\n\n Usage:\n - ALWAYS use Grep for search tasks. NEVER invoke `grep` or `rg` as a Bash command. The Grep tool has been optimized for correct permissions and access.\n - Supports full regex syntax (e.g., "log.*Error", "function\\s+\\w+")\n - Filter files with glob parameter (e.g., "*.js", "**/*.tsx")\n - Case-insensitive by default\n - Use Task tool for open-ended searches requiring multiple rounds\n', Kind.Search, { diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 16ee1ec7..2aefe443 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -12,6 +12,7 @@ import { makeRelative, shortenPath } from '../utils/paths.js'; import type { Config } from '../config/config.js'; import { DEFAULT_FILE_FILTERING_OPTIONS } from '../config/constants.js'; import { ToolErrorType } from './tool-error.js'; +import { ToolDisplayNames, ToolNames } from './tool-names.js'; /** * Parameters for the LS tool @@ -252,12 +253,12 @@ class LSToolInvocation extends BaseToolInvocation { * Implementation of the LS tool logic */ export class LSTool extends BaseDeclarativeTool { - static readonly Name = 'list_directory'; + static readonly Name = ToolNames.LS; constructor(private config: Config) { super( LSTool.Name, - 'ReadFolder', + ToolDisplayNames.LS, 'Lists the names of files and subdirectories directly within a specified directory path. Can optionally ignore entries matching provided glob patterns.', Kind.Search, { diff --git a/packages/core/src/tools/memoryTool.ts b/packages/core/src/tools/memoryTool.ts index 42c4a661..dc6bebef 100644 --- a/packages/core/src/tools/memoryTool.ts +++ b/packages/core/src/tools/memoryTool.ts @@ -18,6 +18,7 @@ import { Storage } from '../config/storage.js'; import * as Diff from 'diff'; import { DEFAULT_DIFF_OPTIONS } from './diffOptions.js'; import { tildeifyPath } from '../utils/paths.js'; +import { ToolDisplayNames, ToolNames } from './tool-names.js'; import type { ModifiableDeclarativeTool, ModifyContext, @@ -380,11 +381,11 @@ export class MemoryTool extends BaseDeclarativeTool implements ModifiableDeclarativeTool { - static readonly Name: string = memoryToolSchemaData.name!; + static readonly Name: string = ToolNames.MEMORY; constructor() { super( MemoryTool.Name, - 'SaveMemory', + ToolDisplayNames.MEMORY, memoryToolDescription, Kind.Think, memoryToolSchemaData.parametersJsonSchema as Record, diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 99982ca6..a9e47ccf 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -8,7 +8,7 @@ import path from 'node:path'; import { makeRelative, shortenPath } from '../utils/paths.js'; import type { ToolInvocation, ToolLocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; import type { PartUnion } from '@google/genai'; import { @@ -131,7 +131,7 @@ export class ReadFileTool extends BaseDeclarativeTool< constructor(private config: Config) { super( ReadFileTool.Name, - 'ReadFile', + ToolDisplayNames.READ_FILE, `Reads and returns the content of a specified file. If the file is large, the content will be truncated. The tool's response will clearly indicate if truncation has occurred and will provide details on how to read more of the file using the 'offset' and 'limit' parameters. Handles text, images (PNG, JPG, GIF, WEBP, SVG, BMP), and PDF files. For text files, it can read specific line ranges.`, Kind.Read, { diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index c262cf4b..33ea3339 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -6,7 +6,7 @@ import type { ToolInvocation, ToolResult } from './tools.js'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { getErrorMessage } from '../utils/errors.js'; import * as fs from 'node:fs'; import * as path from 'node:path'; @@ -554,7 +554,7 @@ export class ReadManyFilesTool extends BaseDeclarativeTool< super( ReadManyFilesTool.Name, - 'ReadManyFiles', + ToolDisplayNames.READ_MANY_FILES, `Reads content from multiple files specified by paths or glob patterns within a configured target directory. For text files, it concatenates their content into a single string. It is primarily designed for text-based files. However, it can also process image (e.g., .png, .jpg) and PDF (.pdf) files if their file names or extensions are explicitly included in the 'paths' argument. For these explicitly requested non-text files, their data is read and included in a format suitable for model consumption (e.g., base64 encoded). This tool is useful when you need to understand or analyze a collection of files, such as: diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index b5f48672..17e40dbe 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -9,7 +9,7 @@ import path from 'node:path'; import os, { EOL } from 'node:os'; import crypto from 'node:crypto'; import type { Config } from '../config/config.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { ToolErrorType } from './tool-error.js'; import type { ToolInvocation, @@ -429,7 +429,7 @@ export class ShellTool extends BaseDeclarativeTool< constructor(private readonly config: Config) { super( ShellTool.Name, - 'Shell', + ToolDisplayNames.SHELL, getShellToolDescription(), Kind.Execute, { diff --git a/packages/core/src/tools/task.ts b/packages/core/src/tools/task.ts index 06761421..93dff04b 100644 --- a/packages/core/src/tools/task.ts +++ b/packages/core/src/tools/task.ts @@ -5,7 +5,7 @@ */ import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; import type { ToolResult, ToolResultDisplay, @@ -77,7 +77,7 @@ export class TaskTool extends BaseDeclarativeTool { super( TaskTool.Name, - 'Task', + ToolDisplayNames.TASK, 'Delegate tasks to specialized subagents. Loading available subagents...', // Initial description Kind.Other, initialSchema, diff --git a/packages/core/src/tools/todoWrite.ts b/packages/core/src/tools/todoWrite.ts index ffd0bcbb..23deb260 100644 --- a/packages/core/src/tools/todoWrite.ts +++ b/packages/core/src/tools/todoWrite.ts @@ -14,6 +14,7 @@ import * as process from 'process'; import { QWEN_DIR } from '../utils/paths.js'; import type { Config } from '../config/config.js'; +import { ToolDisplayNames, ToolNames } from './tool-names.js'; export interface TodoItem { id: string; @@ -422,12 +423,12 @@ export class TodoWriteTool extends BaseDeclarativeTool< TodoWriteParams, ToolResult > { - static readonly Name: string = todoWriteToolSchemaData.name!; + static readonly Name: string = ToolNames.TODO_WRITE; constructor(private readonly config: Config) { super( TodoWriteTool.Name, - 'TodoWrite', + ToolDisplayNames.TODO_WRITE, todoWriteToolDescription, Kind.Think, todoWriteToolSchemaData.parametersJsonSchema as Record, diff --git a/packages/core/src/tools/tool-names.ts b/packages/core/src/tools/tool-names.ts index 799d0d72..22be9c1b 100644 --- a/packages/core/src/tools/tool-names.ts +++ b/packages/core/src/tools/tool-names.ts @@ -23,4 +23,43 @@ export const ToolNames = { EXIT_PLAN_MODE: 'exit_plan_mode', WEB_FETCH: 'web_fetch', WEB_SEARCH: 'web_search', + LS: 'list_directory', +} as const; + +/** + * Tool display name constants to avoid circular dependencies. + * These constants are used across multiple files and should be kept in sync + * with the actual tool display names. + */ +export const ToolDisplayNames = { + EDIT: 'Edit', + WRITE_FILE: 'WriteFile', + READ_FILE: 'ReadFile', + READ_MANY_FILES: 'ReadManyFiles', + GREP: 'Grep', + GLOB: 'Glob', + SHELL: 'Shell', + TODO_WRITE: 'TodoWrite', + MEMORY: 'SaveMemory', + TASK: 'Task', + EXIT_PLAN_MODE: 'ExitPlanMode', + WEB_FETCH: 'WebFetch', + WEB_SEARCH: 'WebSearch', + LS: 'ListFiles', +} as const; + +// Migration from old tool names to new tool names +// These legacy tool names were used in earlier versions and need to be supported +// for backward compatibility with existing user configurations +export const ToolNamesMigration = { + search_file_content: ToolNames.GREP, // Legacy name from grep tool + replace: ToolNames.EDIT, // Legacy name from edit tool +} as const; + +// Migration from old tool display names to new tool display names +// These legacy display names were used before the tool naming standardization +export const ToolDisplayNamesMigration = { + SearchFiles: ToolDisplayNames.GREP, // Old display name for Grep + FindFiles: ToolDisplayNames.GLOB, // Old display name for Glob + ReadFolder: ToolDisplayNames.LS, // Old display name for ListFiles } as const; diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index 2927e891..7797659e 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -23,7 +23,7 @@ import { ToolConfirmationOutcome, } from './tools.js'; import { DEFAULT_QWEN_MODEL } from '../config/models.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; const URL_FETCH_TIMEOUT_MS = 10000; const MAX_CONTENT_LENGTH = 100000; @@ -196,7 +196,7 @@ export class WebFetchTool extends BaseDeclarativeTool< constructor(private readonly config: Config) { super( WebFetchTool.Name, - 'WebFetch', + ToolDisplayNames.WEB_FETCH, 'Fetches content from a specified URL and processes it using an AI model\n- Takes a URL and a prompt as input\n- Fetches the URL content, converts HTML to markdown\n- Processes the content with the prompt using a small, fast model\n- Returns the model\'s response about the content\n- Use this tool when you need to retrieve and analyze web content\n\nUsage notes:\n - IMPORTANT: If an MCP-provided web fetch tool is available, prefer using that tool instead of this one, as it may have fewer restrictions. All MCP-provided tools start with "mcp__".\n - The URL must be a fully-formed valid URL\n - The prompt should describe what information you want to extract from the page\n - This tool is read-only and does not modify any files\n - Results may be summarized if the content is very large\n - Supports both public and private/localhost URLs using direct fetch', Kind.Fetch, { diff --git a/packages/core/src/tools/web-search/index.ts b/packages/core/src/tools/web-search/index.ts index 2860a333..b9aa83c5 100644 --- a/packages/core/src/tools/web-search/index.ts +++ b/packages/core/src/tools/web-search/index.ts @@ -30,7 +30,7 @@ import type { WebSearchProviderConfig, DashScopeProviderConfig, } from './types.js'; -import { ToolNames } from '../tool-names.js'; +import { ToolNames, ToolDisplayNames } from '../tool-names.js'; class WebSearchToolInvocation extends BaseToolInvocation< WebSearchToolParams, @@ -280,7 +280,7 @@ export class WebSearchTool extends BaseDeclarativeTool< constructor(private readonly config: Config) { super( WebSearchTool.Name, - 'WebSearch', + ToolDisplayNames.WEB_SEARCH, 'Allows searching the web and using results to inform responses. Provides up-to-date information for current events and recent data beyond the training data cutoff. Returns search results formatted with concise answers and source links. Use this tool when accessing information that may be outdated or beyond the knowledge cutoff.', Kind.Search, { diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 560c77e5..c13c9553 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -27,7 +27,7 @@ import { ToolErrorType } from './tool-error.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; -import { ToolNames } from './tool-names.js'; +import { ToolNames, ToolDisplayNames } from './tool-names.js'; import type { ModifiableDeclarativeTool, ModifyContext, @@ -361,7 +361,7 @@ export class WriteFileTool constructor(private readonly config: Config) { super( WriteFileTool.Name, - 'WriteFile', + ToolDisplayNames.WRITE_FILE, `Writes content to a specified file in the local filesystem. The user has the ability to modify \`content\`. If modified, this will be stated in the response.`, diff --git a/packages/core/src/utils/tool-utils.test.ts b/packages/core/src/utils/tool-utils.test.ts index 5527e186..e24585e0 100644 --- a/packages/core/src/utils/tool-utils.test.ts +++ b/packages/core/src/utils/tool-utils.test.ts @@ -5,9 +5,10 @@ */ import { expect, describe, it } from 'vitest'; -import { doesToolInvocationMatch } from './tool-utils.js'; +import { doesToolInvocationMatch, isToolEnabled } from './tool-utils.js'; import type { AnyToolInvocation, Config } from '../index.js'; import { ReadFileTool } from '../tools/read-file.js'; +import { ToolNames } from '../tools/tool-names.js'; describe('doesToolInvocationMatch', () => { it('should not match a partial command prefix', () => { @@ -92,3 +93,67 @@ describe('doesToolInvocationMatch', () => { }); }); }); + +describe('isToolEnabled', () => { + it('enables tool when coreTools is undefined and tool is not excluded', () => { + expect(isToolEnabled(ToolNames.SHELL, undefined, undefined)).toBe(true); + }); + + it('disables tool when excluded by canonical tool name', () => { + expect( + isToolEnabled(ToolNames.SHELL, undefined, ['run_shell_command']), + ).toBe(false); + }); + + it('enables tool when explicitly listed by display name', () => { + expect(isToolEnabled(ToolNames.SHELL, ['Shell'], undefined)).toBe(true); + }); + + it('enables tool when explicitly listed by class name', () => { + expect(isToolEnabled(ToolNames.SHELL, ['ShellTool'], undefined)).toBe(true); + }); + + it('supports class names with leading underscores', () => { + expect(isToolEnabled(ToolNames.SHELL, ['__ShellTool'], undefined)).toBe( + true, + ); + }); + + it('enables tool when coreTools contains a legacy tool name alias', () => { + expect( + isToolEnabled(ToolNames.GREP, ['search_file_content'], undefined), + ).toBe(true); + }); + + it('enables tool when coreTools contains a legacy display name alias', () => { + expect(isToolEnabled(ToolNames.GLOB, ['FindFiles'], undefined)).toBe(true); + }); + + it('enables tool when coreTools contains an argument-specific pattern', () => { + expect( + isToolEnabled(ToolNames.SHELL, ['Shell(git status)'], undefined), + ).toBe(true); + }); + + it('disables tool when not present in coreTools', () => { + expect(isToolEnabled(ToolNames.SHELL, ['Edit'], undefined)).toBe(false); + }); + + it('uses legacy display name aliases when excluding tools', () => { + expect(isToolEnabled(ToolNames.GREP, undefined, ['SearchFiles'])).toBe( + false, + ); + }); + + it('does not treat argument-specific exclusions as matches', () => { + expect( + isToolEnabled(ToolNames.SHELL, undefined, ['Shell(git status)']), + ).toBe(true); + }); + + it('considers excludeTools even when tool is explicitly enabled', () => { + expect(isToolEnabled(ToolNames.SHELL, ['Shell'], ['ShellTool'])).toBe( + false, + ); + }); +}); diff --git a/packages/core/src/utils/tool-utils.ts b/packages/core/src/utils/tool-utils.ts index cd3053ff..fb6f52c7 100644 --- a/packages/core/src/utils/tool-utils.ts +++ b/packages/core/src/utils/tool-utils.ts @@ -6,6 +6,111 @@ import type { AnyDeclarativeTool, AnyToolInvocation } from '../index.js'; import { isTool } from '../index.js'; +import { + ToolNames, + ToolDisplayNames, + ToolNamesMigration, + ToolDisplayNamesMigration, +} from '../tools/tool-names.js'; + +export type ToolName = (typeof ToolNames)[keyof typeof ToolNames]; + +const normalizeIdentifier = (identifier: string): string => + identifier.trim().replace(/^_+/, ''); + +const toolNameKeys = Object.keys(ToolNames) as Array; + +const TOOL_ALIAS_MAP: Map> = (() => { + const map = new Map>(); + + const addAlias = (set: Set, alias?: string) => { + if (!alias) { + return; + } + set.add(normalizeIdentifier(alias)); + }; + + for (const key of toolNameKeys) { + const canonicalName = ToolNames[key]; + const displayName = ToolDisplayNames[key]; + const aliases = new Set(); + + addAlias(aliases, canonicalName); + addAlias(aliases, displayName); + addAlias(aliases, `${displayName}Tool`); + + for (const [legacyName, mappedName] of Object.entries(ToolNamesMigration)) { + if (mappedName === canonicalName) { + addAlias(aliases, legacyName); + } + } + + for (const [legacyDisplay, mappedDisplay] of Object.entries( + ToolDisplayNamesMigration, + )) { + if (mappedDisplay === displayName) { + addAlias(aliases, legacyDisplay); + } + } + + map.set(canonicalName, aliases); + } + + return map; +})(); + +const getAliasSetForTool = (toolName: ToolName): Set => { + const aliases = TOOL_ALIAS_MAP.get(toolName); + if (!aliases) { + return new Set([normalizeIdentifier(toolName)]); + } + return aliases; +}; + +const sanitizeExactIdentifier = (value: string): string => + normalizeIdentifier(value); + +const sanitizePatternIdentifier = (value: string): string => { + const openParenIndex = value.indexOf('('); + if (openParenIndex === -1) { + return normalizeIdentifier(value); + } + return normalizeIdentifier(value.slice(0, openParenIndex)); +}; + +const filterList = (list?: string[]): string[] => + (list ?? []).filter((entry): entry is string => + Boolean(entry && entry.trim()), + ); + +export function isToolEnabled( + toolName: ToolName, + coreTools?: string[], + excludeTools?: string[], +): boolean { + const aliasSet = getAliasSetForTool(toolName); + const matchesIdentifier = (value: string): boolean => + aliasSet.has(sanitizeExactIdentifier(value)); + const matchesIdentifierWithArgs = (value: string): boolean => + aliasSet.has(sanitizePatternIdentifier(value)); + + const filteredCore = filterList(coreTools); + const filteredExclude = filterList(excludeTools); + + if (filteredCore.length === 0) { + return !filteredExclude.some((entry) => matchesIdentifier(entry)); + } + + const isExplicitlyEnabled = filteredCore.some( + (entry) => matchesIdentifier(entry) || matchesIdentifierWithArgs(entry), + ); + + if (!isExplicitlyEnabled) { + return false; + } + + return !filteredExclude.some((entry) => matchesIdentifier(entry)); +} const SHELL_TOOL_NAMES = ['run_shell_command', 'ShellTool']; From d5d96c726aa759b6a582d246f9753205bf043db1 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Wed, 12 Nov 2025 19:46:28 +0800 Subject: [PATCH 02/10] fix: print request errors for logging only in debug mode (#1006) --- .../src/telemetry/qwen-logger/qwen-logger.ts | 25 +++---------------- 1 file changed, 4 insertions(+), 21 deletions(-) diff --git a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts index 09463267..96f796e4 100644 --- a/packages/core/src/telemetry/qwen-logger/qwen-logger.ts +++ b/packages/core/src/telemetry/qwen-logger/qwen-logger.ts @@ -48,7 +48,6 @@ import type { } from './event-types.js'; import type { Config } from '../../config/config.js'; import { safeJsonStringify } from '../../utils/safeJsonStringify.js'; -import { type HttpError, retryWithBackoff } from '../../utils/retry.js'; import { InstallationManager } from '../../utils/installationManager.js'; import { FixedDeque } from 'mnemonist'; import { AuthType } from '../../core/contentGenerator.js'; @@ -288,8 +287,8 @@ export class QwenLogger { const rumPayload = await this.createRumPayload(); // Override events with the ones we're sending rumPayload.events = eventsToSend; - const flushFn = () => - new Promise((resolve, reject) => { + try { + await new Promise((resolve, reject) => { const body = safeJsonStringify(rumPayload); const options = { hostname: USAGE_STATS_HOSTNAME, @@ -311,10 +310,9 @@ export class QwenLogger { res.statusCode && (res.statusCode < 200 || res.statusCode >= 300) ) { - const err: HttpError = new Error( + const err = new Error( `Request failed with status ${res.statusCode}`, ); - err.status = res.statusCode; res.resume(); return reject(err); } @@ -326,26 +324,11 @@ export class QwenLogger { req.end(body); }); - try { - await retryWithBackoff(flushFn, { - maxAttempts: 3, - initialDelayMs: 200, - shouldRetryOnError: (err: unknown) => { - if (!(err instanceof Error)) return false; - const status = (err as HttpError).status as number | undefined; - // If status is not available, it's likely a network error - if (status === undefined) return true; - - // Retry on 429 (Too many Requests) and 5xx server errors. - return status === 429 || (status >= 500 && status < 600); - }, - }); - this.lastFlushTime = Date.now(); return {}; } catch (error) { if (this.config?.getDebugMode()) { - console.error('RUM flush failed after multiple retries.', error); + console.error('RUM flush failed.', error); } // Re-queue failed events for retry From b029f0d2ce3f5e86d0f13a3cc91b9ac19bce5024 Mon Sep 17 00:00:00 2001 From: "fffly.Zzz" <139431781+ffflyZzz@users.noreply.github.com> Date: Thu, 13 Nov 2025 17:41:22 +0800 Subject: [PATCH 03/10] docs: correct YAML list format for tools field in agent template (#1026) Co-authored-by: zhangxiao --- docs/features/subagents.md | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/docs/features/subagents.md b/docs/features/subagents.md index 15b5e273..225384ca 100644 --- a/docs/features/subagents.md +++ b/docs/features/subagents.md @@ -106,7 +106,10 @@ Subagents are configured using Markdown files with YAML frontmatter. This format --- name: agent-name description: Brief description of when and how to use this agent -tools: tool1, tool2, tool3 # Optional +tools: + - tool1 + - tool2 + - tool3 # Optional --- System prompt content goes here. @@ -167,7 +170,11 @@ Perfect for comprehensive test creation and test-driven development. --- name: testing-expert description: Writes comprehensive unit tests, integration tests, and handles test automation with best practices -tools: read_file, write_file, read_many_files, run_shell_command +tools: + - read_file + - write_file + - read_many_files + - run_shell_command --- You are a testing specialist focused on creating high-quality, maintainable tests. @@ -207,7 +214,11 @@ Specialized in creating clear, comprehensive documentation. --- name: documentation-writer description: Creates comprehensive documentation, README files, API docs, and user guides -tools: read_file, write_file, read_many_files, web_search +tools: + - read_file + - write_file + - read_many_files + - web_search --- You are a technical documentation specialist for ${project_name}. @@ -256,7 +267,9 @@ Focused on code quality, security, and best practices. --- name: code-reviewer description: Reviews code for best practices, security issues, performance, and maintainability -tools: read_file, read_many_files +tools: + - read_file + - read_many_files --- You are an experienced code reviewer focused on quality, security, and maintainability. @@ -298,7 +311,11 @@ Optimized for React development, hooks, and component patterns. --- name: react-specialist description: Expert in React development, hooks, component patterns, and modern React best practices -tools: read_file, write_file, read_many_files, run_shell_command +tools: + - read_file + - write_file + - read_many_files + - run_shell_command --- You are a React specialist with deep expertise in modern React development. @@ -339,7 +356,11 @@ Specialized in Python development, frameworks, and best practices. --- name: python-expert description: Expert in Python development, frameworks, testing, and Python-specific best practices -tools: read_file, write_file, read_many_files, run_shell_command +tools: + - read_file + - write_file + - read_many_files + - run_shell_command --- You are a Python expert with deep knowledge of the Python ecosystem. From 0752a31e1e308ab993d70f7dd2dae406450e49b1 Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Thu, 13 Nov 2025 19:01:09 +0800 Subject: [PATCH 04/10] =?UTF-8?q?=F0=9F=8E=AF=20PR:=20Improve=20Edit=20Too?= =?UTF-8?q?l=20Reliability=20with=20Fuzzy=20Matching=20Pipeline=20(#1025)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/tools/file-system.md | 12 +- packages/core/src/tools/edit.test.ts | 76 +--- packages/core/src/tools/edit.ts | 87 ++-- packages/core/src/utils/editHelper.test.ts | 153 +++++++ packages/core/src/utils/editHelper.ts | 499 +++++++++++++++++++++ 5 files changed, 718 insertions(+), 109 deletions(-) create mode 100644 packages/core/src/utils/editHelper.test.ts create mode 100644 packages/core/src/utils/editHelper.ts diff --git a/docs/tools/file-system.md b/docs/tools/file-system.md index 93b6ac85..3c5097df 100644 --- a/docs/tools/file-system.md +++ b/docs/tools/file-system.md @@ -132,7 +132,7 @@ grep_search(pattern="function", glob="*.js", limit=10) ## 6. `edit` (Edit) -`edit` replaces text within a file. By default, replaces a single occurrence, but can replace multiple occurrences when `expected_replacements` is specified. This tool is designed for precise, targeted changes and requires significant context around the `old_string` to ensure it modifies the correct location. +`edit` replaces text within a file. By default it requires `old_string` to match a single unique location; set `replace_all` to `true` when you intentionally want to change every occurrence. This tool is designed for precise, targeted changes and requires significant context around the `old_string` to ensure it modifies the correct location. - **Tool name:** `edit` - **Display name:** Edit @@ -144,12 +144,12 @@ grep_search(pattern="function", glob="*.js", limit=10) **CRITICAL:** This string must uniquely identify the single instance to change. It should include at least 3 lines of context _before_ and _after_ the target text, matching whitespace and indentation precisely. If `old_string` is empty, the tool attempts to create a new file at `file_path` with `new_string` as content. - `new_string` (string, required): The exact literal text to replace `old_string` with. - - `expected_replacements` (number, optional): The number of occurrences to replace. Defaults to `1`. + - `replace_all` (boolean, optional): Replace all occurrences of `old_string`. Defaults to `false`. - **Behavior:** - If `old_string` is empty and `file_path` does not exist, creates a new file with `new_string` as content. - - If `old_string` is provided, it reads the `file_path` and attempts to find exactly one occurrence of `old_string`. - - If one occurrence is found, it replaces it with `new_string`. + - If `old_string` is provided, it reads the `file_path` and attempts to find exactly one occurrence unless `replace_all` is true. + - If the match is unique (or `replace_all` is true), it replaces the text with `new_string`. - **Enhanced Reliability (Multi-Stage Edit Correction):** To significantly improve the success rate of edits, especially when the model-provided `old_string` might not be perfectly precise, the tool incorporates a multi-stage edit correction mechanism. - If the initial `old_string` isn't found or matches multiple locations, the tool can leverage the Qwen model to iteratively refine `old_string` (and potentially `new_string`). - This self-correction process attempts to identify the unique segment the model intended to modify, making the `edit` operation more robust even with slightly imperfect initial context. @@ -158,10 +158,10 @@ grep_search(pattern="function", glob="*.js", limit=10) - `old_string` is not empty, but the `file_path` does not exist. - `old_string` is empty, but the `file_path` already exists. - `old_string` is not found in the file after attempts to correct it. - - `old_string` is found multiple times, and the self-correction mechanism cannot resolve it to a single, unambiguous match. + - `old_string` is found multiple times, `replace_all` is false, and the self-correction mechanism cannot resolve it to a single, unambiguous match. - **Output (`llmContent`):** - On success: `Successfully modified file: /path/to/file.txt (1 replacements).` or `Created new file: /path/to/new_file.txt with provided content.` - - On failure: An error message explaining the reason (e.g., `Failed to edit, 0 occurrences found...`, `Failed to edit, expected 1 occurrences but found 2...`). + - On failure: An error message explaining the reason (e.g., `Failed to edit, 0 occurrences found...`, `Failed to edit because the text matches multiple locations...`). - **Confirmation:** Yes. Shows a diff of the proposed changes and asks for user approval before writing to the file. These file system tools provide a foundation for Qwen Code to understand and interact with your local project context. diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index b695087a..9e41b938 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -425,7 +425,9 @@ describe('EditTool', () => { const invocation = tool.build(params); const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch(/Successfully modified file/); + expect(result.llmContent).toMatch( + /Showing lines \d+-\d+ of \d+ from the edited file:/, + ); expect(fs.readFileSync(filePath, 'utf8')).toBe(newContent); const display = result.returnDisplay as FileDiff; expect(display.fileDiff).toMatch(initialContent); @@ -450,6 +452,9 @@ describe('EditTool', () => { const result = await invocation.execute(new AbortController().signal); expect(result.llmContent).toMatch(/Created new file/); + expect(result.llmContent).toMatch( + /Showing lines \d+-\d+ of \d+ from the edited file:/, + ); expect(fs.existsSync(newFilePath)).toBe(true); expect(fs.readFileSync(newFilePath, 'utf8')).toBe(fileContent); @@ -485,7 +490,7 @@ describe('EditTool', () => { ); }); - it('should return error if multiple occurrences of old_string are found', async () => { + it('should return error if multiple occurrences of old_string are found and replace_all is false', async () => { fs.writeFileSync(filePath, 'multiple old old strings', 'utf8'); const params: EditToolParams = { file_path: filePath, @@ -494,27 +499,27 @@ describe('EditTool', () => { }; const invocation = tool.build(params); const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch( - /Expected 1 occurrence but found 2 for old_string in file/, - ); + expect(result.llmContent).toMatch(/replace_all was not enabled/); expect(result.returnDisplay).toMatch( - /Failed to edit, expected 1 occurrence but found 2/, + /Failed to edit because the text matches multiple locations/, ); }); - it('should successfully replace multiple occurrences when expected_replacements specified', async () => { + it('should successfully replace multiple occurrences when replace_all is true', async () => { fs.writeFileSync(filePath, 'old text\nold text\nold text', 'utf8'); const params: EditToolParams = { file_path: filePath, old_string: 'old', new_string: 'new', - expected_replacements: 3, + replace_all: true, }; const invocation = tool.build(params); const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch(/Successfully modified file/); + expect(result.llmContent).toMatch( + /Showing lines \d+-\d+ of \d+ from the edited file/, + ); expect(fs.readFileSync(filePath, 'utf8')).toBe( 'new text\nnew text\nnew text', ); @@ -535,24 +540,6 @@ describe('EditTool', () => { }); }); - it('should return error if expected_replacements does not match actual occurrences', async () => { - fs.writeFileSync(filePath, 'old text old text', 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - expected_replacements: 3, // Expecting 3 but only 2 exist - }; - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - expect(result.llmContent).toMatch( - /Expected 3 occurrences but found 2 for old_string in file/, - ); - expect(result.returnDisplay).toMatch( - /Failed to edit, expected 3 occurrences but found 2/, - ); - }); - it('should return error if trying to create a file that already exists (empty old_string)', async () => { fs.writeFileSync(filePath, 'Existing content', 'utf8'); const params: EditToolParams = { @@ -568,38 +555,6 @@ describe('EditTool', () => { ); }); - it('should include modification message when proposed content is modified', async () => { - const initialContent = 'Line 1\nold line\nLine 3\nLine 4\nLine 5\n'; - fs.writeFileSync(filePath, initialContent, 'utf8'); - const params: EditToolParams = { - file_path: filePath, - old_string: 'old', - new_string: 'new', - modified_by_user: true, - ai_proposed_content: 'Line 1\nAI line\nLine 3\nLine 4\nLine 5\n', - }; - - (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( - ApprovalMode.AUTO_EDIT, - ); - const invocation = tool.build(params); - const result = await invocation.execute(new AbortController().signal); - - expect(result.llmContent).toMatch( - /User modified the `new_string` content/, - ); - expect((result.returnDisplay as FileDiff).diffStat).toStrictEqual({ - model_added_lines: 1, - model_removed_lines: 1, - model_added_chars: 7, - model_removed_chars: 8, - user_added_lines: 1, - user_removed_lines: 1, - user_added_chars: 8, - user_removed_chars: 7, - }); - }); - it('should not include modification message when proposed content is not modified', async () => { const initialContent = 'This is some old text.'; fs.writeFileSync(filePath, initialContent, 'utf8'); @@ -723,13 +678,12 @@ describe('EditTool', () => { expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_OCCURRENCE_FOUND); }); - it('should return EXPECTED_OCCURRENCE_MISMATCH error', async () => { + it('should return EXPECTED_OCCURRENCE_MISMATCH error when replace_all is false and text is not unique', async () => { fs.writeFileSync(filePath, 'one one two', 'utf8'); const params: EditToolParams = { file_path: filePath, old_string: 'one', new_string: 'new', - expected_replacements: 3, }; const invocation = tool.build(params); const result = await invocation.execute(new AbortController().signal); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index fa94fda5..ec257290 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -34,6 +34,12 @@ import type { } from './modifiable-tool.js'; import { IdeClient } from '../ide/ide-client.js'; import { safeLiteralReplace } from '../utils/textUtils.js'; +import { + countOccurrences, + extractEditSnippet, + maybeAugmentOldStringForDeletion, + normalizeEditStrings, +} from '../utils/editHelper.js'; export function applyReplacement( currentContent: string | null, @@ -77,10 +83,9 @@ export interface EditToolParams { new_string: string; /** - * Number of replacements expected. Defaults to 1 if not specified. - * Use when you want to replace multiple occurrences. + * Replace every occurrence of old_string instead of requiring a unique match. */ - expected_replacements?: number; + replace_all?: boolean; /** * Whether the edit was modified manually by the user. @@ -118,12 +123,12 @@ class EditToolInvocation implements ToolInvocation { * @throws File system errors if reading the file fails unexpectedly (e.g., permissions) */ private async calculateEdit(params: EditToolParams): Promise { - const expectedReplacements = params.expected_replacements ?? 1; + const replaceAll = params.replace_all ?? false; let currentContent: string | null = null; let fileExists = false; let isNewFile = false; - const finalNewString = params.new_string; - const finalOldString = params.old_string; + let finalNewString = params.new_string; + let finalOldString = params.old_string; let occurrences = 0; let error: | { display: string; raw: string; type: ToolErrorType } @@ -144,7 +149,15 @@ class EditToolInvocation implements ToolInvocation { fileExists = false; } - if (params.old_string === '' && !fileExists) { + const normalizedStrings = normalizeEditStrings( + currentContent, + finalOldString, + finalNewString, + ); + finalOldString = normalizedStrings.oldString; + finalNewString = normalizedStrings.newString; + + if (finalOldString === '' && !fileExists) { // Creating a new file isNewFile = true; } else if (!fileExists) { @@ -155,7 +168,13 @@ class EditToolInvocation implements ToolInvocation { type: ToolErrorType.FILE_NOT_FOUND, }; } else if (currentContent !== null) { - occurrences = this.countOccurrences(currentContent, params.old_string); + finalOldString = maybeAugmentOldStringForDeletion( + currentContent, + finalOldString, + finalNewString, + ); + + occurrences = countOccurrences(currentContent, finalOldString); if (params.old_string === '') { // Error: Trying to create a file that already exists error = { @@ -169,13 +188,10 @@ class EditToolInvocation implements ToolInvocation { raw: `Failed to edit, 0 occurrences found for old_string in ${params.file_path}. No edits made. The exact text in old_string was not found. Ensure you're not escaping content incorrectly and check whitespace, indentation, and context. Use ${ReadFileTool.Name} tool to verify.`, type: ToolErrorType.EDIT_NO_OCCURRENCE_FOUND, }; - } else if (occurrences !== expectedReplacements) { - const occurrenceTerm = - expectedReplacements === 1 ? 'occurrence' : 'occurrences'; - + } else if (!replaceAll && occurrences > 1) { error = { - display: `Failed to edit, expected ${expectedReplacements} ${occurrenceTerm} but found ${occurrences}.`, - raw: `Failed to edit, Expected ${expectedReplacements} ${occurrenceTerm} but found ${occurrences} for old_string in file: ${params.file_path}`, + display: `Failed to edit because the text matches multiple locations. Provide more context or set replace_all to true.`, + raw: `Failed to edit. Found ${occurrences} occurrences for old_string in ${params.file_path} but replace_all was not enabled.`, type: ToolErrorType.EDIT_EXPECTED_OCCURRENCE_MISMATCH, }; } else if (finalOldString === finalNewString) { @@ -221,22 +237,6 @@ class EditToolInvocation implements ToolInvocation { }; } - /** - * Counts occurrences of a substring in a string - */ - private countOccurrences(str: string, substr: string): number { - if (substr === '') { - return 0; - } - let count = 0; - let pos = str.indexOf(substr); - while (pos !== -1) { - count++; - pos = str.indexOf(substr, pos + substr.length); // Start search after the current match - } - return count; - } - /** * Handles the confirmation prompt for the Edit tool in the CLI. * It needs to calculate the diff to show the user. @@ -422,12 +422,16 @@ class EditToolInvocation implements ToolInvocation { const llmSuccessMessageParts = [ editData.isNewFile ? `Created new file: ${this.params.file_path} with provided content.` - : `Successfully modified file: ${this.params.file_path} (${editData.occurrences} replacements).`, + : `The file: ${this.params.file_path} has been updated.`, ]; - if (this.params.modified_by_user) { - llmSuccessMessageParts.push( - `User modified the \`new_string\` content to be: ${this.params.new_string}.`, - ); + + const snippetResult = extractEditSnippet( + editData.currentContent, + editData.newContent, + ); + if (snippetResult) { + const snippetText = `Showing lines ${snippetResult.startLine}-${snippetResult.endLine} of ${snippetResult.totalLines} from the edited file:\n\n---\n\n${snippetResult.content}`; + llmSuccessMessageParts.push(snippetText); } return { @@ -470,7 +474,7 @@ export class EditTool super( EditTool.Name, ToolDisplayNames.EDIT, - `Replaces text within a file. By default, replaces a single occurrence, but can replace multiple occurrences when \`expected_replacements\` is specified. This tool requires providing significant context around the change to ensure precise targeting. Always use the ${ReadFileTool.Name} tool to examine the file's current content before attempting a text replacement. + `Replaces text within a file. By default, replaces a single occurrence. Set \`replace_all\` to true when you intend to modify every instance of \`old_string\`. This tool requires providing significant context around the change to ensure precise targeting. Always use the ${ReadFileTool.Name} tool to examine the file's current content before attempting a text replacement. The user has the ability to modify the \`new_string\` content. If modified, this will be stated in the response. @@ -480,7 +484,7 @@ Expectation for required parameters: 3. \`new_string\` MUST be the exact literal text to replace \`old_string\` with (also including all whitespace, indentation, newlines, and surrounding code etc.). Ensure the resulting code is correct and idiomatic. 4. NEVER escape \`old_string\` or \`new_string\`, that would break the exact literal text requirement. **Important:** If ANY of the above are not satisfied, the tool will fail. CRITICAL for \`old_string\`: Must uniquely identify the single instance to change. Include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. If this string matches multiple locations, or does not match exactly, the tool will fail. -**Multiple replacements:** Set \`expected_replacements\` to the number of occurrences you want to replace. The tool will replace ALL occurrences that match \`old_string\` exactly. Ensure the number of replacements matches your expectation.`, +**Multiple replacements:** Set \`replace_all\` to true when you want to replace every occurrence that matches \`old_string\`.`, Kind.Edit, { properties: { @@ -491,7 +495,7 @@ Expectation for required parameters: }, old_string: { description: - 'The exact literal text to replace, preferably unescaped. For single replacements (default), include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. For multiple replacements, specify expected_replacements parameter. If this string is not the exact literal text (i.e. you escaped it) or does not match exactly, the tool will fail.', + 'The exact literal text to replace, preferably unescaped. For single replacements (default), include at least 3 lines of context BEFORE and AFTER the target text, matching whitespace and indentation precisely. If this string is not the exact literal text (i.e. you escaped it) or does not match exactly, the tool will fail.', type: 'string', }, new_string: { @@ -499,11 +503,10 @@ Expectation for required parameters: 'The exact literal text to replace `old_string` with, preferably unescaped. Provide the EXACT text. Ensure the resulting code is correct and idiomatic.', type: 'string', }, - expected_replacements: { - type: 'number', + replace_all: { + type: 'boolean', description: - 'Number of replacements expected. Defaults to 1 if not specified. Use when you want to replace multiple occurrences.', - minimum: 1, + 'Replace all occurrences of old_string (default false).', }, }, required: ['file_path', 'old_string', 'new_string'], diff --git a/packages/core/src/utils/editHelper.test.ts b/packages/core/src/utils/editHelper.test.ts new file mode 100644 index 00000000..79fe78f6 --- /dev/null +++ b/packages/core/src/utils/editHelper.test.ts @@ -0,0 +1,153 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { + countOccurrences, + maybeAugmentOldStringForDeletion, + normalizeEditStrings, +} from './editHelper.js'; + +describe('normalizeEditStrings', () => { + const file = `const one = 1; +const two = 2; +`; + + it('returns literal matches unchanged and trims new_string trailing whitespace', () => { + const result = normalizeEditStrings( + file, + 'const two = 2;', + ' const two = 42; ', + ); + expect(result).toEqual({ + oldString: 'const two = 2;', + newString: ' const two = 42;', + }); + }); + + it('normalizes smart quotes to match on-disk text', () => { + const result = normalizeEditStrings( + "const greeting = 'Don't';\n", + 'const greeting = ‘Don’t’;', + 'const greeting = “Hello”; ', + ); + expect(result).toEqual({ + oldString: "const greeting = 'Don't';", + newString: 'const greeting = “Hello”;', + }); + }); + + it('falls back to original strings when no match is found', () => { + const result = normalizeEditStrings(file, 'missing text', 'replacement'); + expect(result).toEqual({ + oldString: 'missing text', + newString: 'replacement', + }); + }); + + it('still trims new_string when editing a brand-new file', () => { + const result = normalizeEditStrings(null, '', 'new file contents '); + expect(result).toEqual({ + oldString: '', + newString: 'new file contents', + }); + }); + + it('matches unicode dash variants', () => { + const result = normalizeEditStrings( + 'const range = "1-2";\n', + 'const range = "1\u20132";', + 'const range = "3\u20135"; ', + ); + expect(result).toEqual({ + oldString: 'const range = "1-2";', + newString: 'const range = "3\u20135";', + }); + }); + + it('matches when trailing whitespace differs only at line ends', () => { + const result = normalizeEditStrings( + 'value = 1;\n', + 'value = 1; \n', + 'value = 2; \n', + ); + expect(result).toEqual({ + oldString: 'value = 1;\n', + newString: 'value = 2;\n', + }); + }); + + it('treats non-breaking spaces as regular spaces', () => { + const result = normalizeEditStrings( + 'const label = "hello world";\n', + 'const label = "hello\u00a0world";', + 'const label = "hi\u00a0world";', + ); + expect(result).toEqual({ + oldString: 'const label = "hello world";', + newString: 'const label = "hi\u00a0world";', + }); + }); + + it('drops trailing newline from new content when the file lacks it', () => { + const result = normalizeEditStrings( + 'console.log("hi")', + 'console.log("hi")\n', + 'console.log("bye")\n', + ); + expect(result).toEqual({ + oldString: 'console.log("hi")', + newString: 'console.log("bye")', + }); + }); +}); + +describe('countOccurrences', () => { + it('returns zero when substring empty or missing', () => { + expect(countOccurrences('abc', '')).toBe(0); + expect(countOccurrences('abc', 'z')).toBe(0); + }); + + it('counts non-overlapping occurrences', () => { + expect(countOccurrences('aaaa', 'aa')).toBe(2); + }); +}); + +describe('maybeAugmentOldStringForDeletion', () => { + const file = 'console.log("hi")\nconsole.log("bye")\n'; + + it('appends newline when deleting text followed by newline', () => { + expect( + maybeAugmentOldStringForDeletion(file, 'console.log("hi")', ''), + ).toBe('console.log("hi")\n'); + }); + + it('leaves strings untouched when not deleting', () => { + expect( + maybeAugmentOldStringForDeletion( + file, + 'console.log("hi")', + 'replacement', + ), + ).toBe('console.log("hi")'); + }); + + it('does not append newline when file lacks the variant', () => { + expect( + maybeAugmentOldStringForDeletion( + 'console.log("hi")', + 'console.log("hi")', + '', + ), + ).toBe('console.log("hi")'); + }); + + it('no-ops when the old string already ends with a newline', () => { + expect( + maybeAugmentOldStringForDeletion(file, 'console.log("bye")\n', ''), + ).toBe('console.log("bye")\n'); + }); +}); diff --git a/packages/core/src/utils/editHelper.ts b/packages/core/src/utils/editHelper.ts new file mode 100644 index 00000000..6b4a388d --- /dev/null +++ b/packages/core/src/utils/editHelper.ts @@ -0,0 +1,499 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Helpers for reconciling LLM-proposed edits with on-disk text. + * + * The normalization pipeline intentionally stays deterministic: we first try + * literal substring matches, then gradually relax comparison rules (smart + * quotes, em-dashes, trailing whitespace, etc.) until we either locate the + * exact slice from the file or conclude the edit cannot be applied. + */ + +/* -------------------------------------------------------------------------- */ +/* Character-level normalization */ +/* -------------------------------------------------------------------------- */ + +const UNICODE_EQUIVALENT_MAP: Record = { + // Hyphen variations → ASCII hyphen-minus. + '\u2010': '-', + '\u2011': '-', + '\u2012': '-', + '\u2013': '-', + '\u2014': '-', + '\u2015': '-', + '\u2212': '-', + // Curly single quotes → straight apostrophe. + '\u2018': "'", + '\u2019': "'", + '\u201A': "'", + '\u201B': "'", + // Curly double quotes → straight double quote. + '\u201C': '"', + '\u201D': '"', + '\u201E': '"', + '\u201F': '"', + // Whitespace variants → normal space. + '\u00A0': ' ', + '\u2002': ' ', + '\u2003': ' ', + '\u2004': ' ', + '\u2005': ' ', + '\u2006': ' ', + '\u2007': ' ', + '\u2008': ' ', + '\u2009': ' ', + '\u200A': ' ', + '\u202F': ' ', + '\u205F': ' ', + '\u3000': ' ', +}; + +function normalizeBasicCharacters(text: string): string { + if (text === '') { + return text; + } + + let normalized = ''; + for (const char of text) { + normalized += UNICODE_EQUIVALENT_MAP[char] ?? char; + } + return normalized; +} + +/** + * Removes trailing whitespace from each line while keeping the original newline + * separators intact. + */ +function stripTrailingWhitespacePreserveNewlines(text: string): string { + const pieces = text.split(/(\r\n|\n|\r)/); + let result = ''; + + for (let i = 0; i < pieces.length; i++) { + const segment = pieces[i]; + if (segment === undefined) { + continue; + } + + if (i % 2 === 0) { + result += segment.trimEnd(); + } else { + result += segment; + } + } + + return result; +} + +/* -------------------------------------------------------------------------- */ +/* Line-based search helpers */ +/* -------------------------------------------------------------------------- */ + +interface MatchedSliceResult { + slice: string; + removedTrailingFinalEmptyLine: boolean; +} + +/** + * Comparison passes become progressively more forgiving, making it possible to + * match when only trailing whitespace differs. Leading whitespace (indentation) + * is always preserved to avoid matching at incorrect scope levels. + */ +const LINE_COMPARISON_PASSES: Array<(value: string) => string> = [ + (value) => value, + (value) => value.trimEnd(), +]; + +function normalizeLineForComparison(value: string): string { + return normalizeBasicCharacters(value).trimEnd(); +} + +/** + * Finds the first index where {@link pattern} appears within {@link lines} once + * both sequences are transformed in the same way. + */ +function seekSequenceWithTransform( + lines: string[], + pattern: string[], + transform: (value: string) => string, +): number | null { + if (pattern.length === 0) { + return 0; + } + + if (pattern.length > lines.length) { + return null; + } + + outer: for (let i = 0; i <= lines.length - pattern.length; i++) { + for (let p = 0; p < pattern.length; p++) { + if (transform(lines[i + p]) !== transform(pattern[p])) { + continue outer; + } + } + return i; + } + + return null; +} + +function buildLineIndex(text: string): { + lines: string[]; + offsets: number[]; +} { + const lines = text.split('\n'); + const offsets = new Array(lines.length + 1); + let cursor = 0; + + for (let i = 0; i < lines.length; i++) { + offsets[i] = cursor; + cursor += lines[i].length; + if (i < lines.length - 1) { + cursor += 1; // Account for the newline that split() removed. + } + } + offsets[lines.length] = text.length; + + return { lines, offsets }; +} + +/** + * Reconstructs the original characters for the matched lines, optionally + * preserving the newline that follows the final line. + */ +function sliceFromLines( + text: string, + offsets: number[], + lines: string[], + startLine: number, + lineCount: number, + includeTrailingNewline: boolean, +): string { + if (lineCount === 0) { + return includeTrailingNewline ? '\n' : ''; + } + + const startIndex = offsets[startLine] ?? 0; + const lastLineIndex = startLine + lineCount - 1; + const lastLineStart = offsets[lastLineIndex] ?? 0; + let endIndex = lastLineStart + (lines[lastLineIndex]?.length ?? 0); + + if (includeTrailingNewline) { + const nextLineStart = offsets[startLine + lineCount]; + if (nextLineStart !== undefined) { + endIndex = nextLineStart; + } else if (text.endsWith('\n')) { + endIndex = text.length; + } + } + + return text.slice(startIndex, endIndex); +} + +function findLineBasedMatch( + haystack: string, + needle: string, +): MatchedSliceResult | null { + const { lines, offsets } = buildLineIndex(haystack); + const patternLines = needle.split('\n'); + const endsWithNewline = needle.endsWith('\n'); + + if (patternLines.length === 0) { + return null; + } + + const attemptMatch = (candidate: string[]): number | null => { + for (const pass of LINE_COMPARISON_PASSES) { + const idx = seekSequenceWithTransform(lines, candidate, pass); + if (idx !== null) { + return idx; + } + } + return seekSequenceWithTransform( + lines, + candidate, + normalizeLineForComparison, + ); + }; + + let matchIndex = attemptMatch(patternLines); + if (matchIndex !== null) { + return { + slice: sliceFromLines( + haystack, + offsets, + lines, + matchIndex, + patternLines.length, + endsWithNewline, + ), + removedTrailingFinalEmptyLine: false, + }; + } + + if (patternLines.at(-1) === '') { + const trimmedPattern = patternLines.slice(0, -1); + if (trimmedPattern.length === 0) { + return null; + } + matchIndex = attemptMatch(trimmedPattern); + if (matchIndex !== null) { + return { + slice: sliceFromLines( + haystack, + offsets, + lines, + matchIndex, + trimmedPattern.length, + false, + ), + removedTrailingFinalEmptyLine: true, + }; + } + } + + return null; +} + +/* -------------------------------------------------------------------------- */ +/* Slice discovery */ +/* -------------------------------------------------------------------------- */ + +function findMatchedSlice( + haystack: string, + needle: string, +): MatchedSliceResult | null { + if (needle === '') { + return null; + } + + const literalIndex = haystack.indexOf(needle); + if (literalIndex !== -1) { + return { + slice: haystack.slice(literalIndex, literalIndex + needle.length), + removedTrailingFinalEmptyLine: false, + }; + } + + const normalizedHaystack = normalizeBasicCharacters(haystack); + const normalizedNeedleChars = normalizeBasicCharacters(needle); + const normalizedIndex = normalizedHaystack.indexOf(normalizedNeedleChars); + if (normalizedIndex !== -1) { + return { + slice: haystack.slice(normalizedIndex, normalizedIndex + needle.length), + removedTrailingFinalEmptyLine: false, + }; + } + + return findLineBasedMatch(haystack, needle); +} + +/** + * Returns the literal slice from {@link haystack} that best corresponds to the + * provided {@link needle}, or {@code null} when no match is found. + */ +/* -------------------------------------------------------------------------- */ +/* Replacement helpers */ +/* -------------------------------------------------------------------------- */ + +function removeTrailingNewline(text: string): string { + if (text.endsWith('\r\n')) { + return text.slice(0, -2); + } + if (text.endsWith('\n') || text.endsWith('\r')) { + return text.slice(0, -1); + } + return text; +} + +function adjustNewStringForTrailingLine( + newString: string, + removedTrailingLine: boolean, +): string { + return removedTrailingLine ? removeTrailingNewline(newString) : newString; +} + +export interface NormalizedEditStrings { + oldString: string; + newString: string; +} + +/** + * Runs the core normalization pipeline: + * 1. Strip trailing whitespace copied from numbered output. + * 2. Attempt to find the literal text inside {@link fileContent}. + * 3. If found through a relaxed match (smart quotes, line trims, etc.), + * return the canonical slice from disk so later replacements operate on + * exact bytes. + */ +export function normalizeEditStrings( + fileContent: string | null, + oldString: string, + newString: string, +): NormalizedEditStrings { + const trimmedNewString = stripTrailingWhitespacePreserveNewlines(newString); + + if (fileContent === null || oldString === '') { + return { + oldString, + newString: trimmedNewString, + }; + } + + const canonicalOriginal = findMatchedSlice(fileContent, oldString); + if (canonicalOriginal !== null) { + return { + oldString: canonicalOriginal.slice, + newString: adjustNewStringForTrailingLine( + trimmedNewString, + canonicalOriginal.removedTrailingFinalEmptyLine, + ), + }; + } + + return { + oldString, + newString: trimmedNewString, + }; +} + +/** + * When deleting text and the on-disk content contains the same substring with a + * trailing newline, automatically consume that newline so the removal does not + * leave a blank line behind. + */ +export function maybeAugmentOldStringForDeletion( + fileContent: string | null, + oldString: string, + newString: string, +): string { + if ( + fileContent === null || + oldString === '' || + newString !== '' || + oldString.endsWith('\n') + ) { + return oldString; + } + + const candidate = `${oldString}\n`; + return fileContent.includes(candidate) ? candidate : oldString; +} + +/** + * Counts the number of non-overlapping occurrences of {@link substr} inside + * {@link source}. Returns 0 when the substring is empty. + */ +export function countOccurrences(source: string, substr: string): number { + if (substr === '') { + return 0; + } + + let count = 0; + let index = source.indexOf(substr); + while (index !== -1) { + count++; + index = source.indexOf(substr, index + substr.length); + } + return count; +} + +/** + * Result from extracting a snippet showing the edited region. + */ +export interface EditSnippetResult { + /** Starting line number (1-indexed) of the snippet */ + startLine: number; + /** Ending line number (1-indexed) of the snippet */ + endLine: number; + /** Total number of lines in the new content */ + totalLines: number; + /** The snippet content (subset of lines from newContent) */ + content: string; +} + +const SNIPPET_CONTEXT_LINES = 4; +const SNIPPET_MAX_LINES = 1000; + +/** + * Extracts a snippet from the edited file showing the changed region with + * surrounding context. This compares the old and new content line-by-line + * from both ends to locate the changed region. + * + * @param oldContent The original file content before the edit (null for new files) + * @param newContent The new file content after the edit + * @param contextLines Number of context lines to show before and after the change + * @returns Snippet information, or null if no meaningful snippet can be extracted + */ +export function extractEditSnippet( + oldContent: string | null, + newContent: string, +): EditSnippetResult | null { + const newLines = newContent.split('\n'); + const totalLines = newLines.length; + + if (oldContent === null) { + return { + startLine: 1, + endLine: totalLines, + totalLines, + content: newContent, + }; + } + + // No changes case + if (oldContent === newContent || !newContent) { + return null; + } + + const oldLines = oldContent.split('\n'); + + // Find the first line that differs from the start + let firstDiffLine = 0; + const minLength = Math.min(oldLines.length, newLines.length); + + while (firstDiffLine < minLength) { + if (oldLines[firstDiffLine] !== newLines[firstDiffLine]) { + break; + } + firstDiffLine++; + } + + // Find the first line that differs from the end + let oldEndIndex = oldLines.length - 1; + let newEndIndex = newLines.length - 1; + + while (oldEndIndex >= firstDiffLine && newEndIndex >= firstDiffLine) { + if (oldLines[oldEndIndex] !== newLines[newEndIndex]) { + break; + } + oldEndIndex--; + newEndIndex--; + } + + // The changed region in the new content is from firstDiffLine to newEndIndex (inclusive) + // Convert to 1-indexed line numbers + const changeStart = firstDiffLine + 1; + const changeEnd = newEndIndex + 1; + + // If the change region is too large, don't generate a snippet + if (changeEnd - changeStart > SNIPPET_MAX_LINES) { + return null; + } + + // Calculate snippet bounds with context + const snippetStart = Math.max(1, changeStart - SNIPPET_CONTEXT_LINES); + const snippetEnd = Math.min(totalLines, changeEnd + SNIPPET_CONTEXT_LINES); + + const snippetLines = newLines.slice(snippetStart - 1, snippetEnd); + + return { + startLine: snippetStart, + endLine: snippetEnd, + totalLines, + content: snippetLines.join('\n'), + }; +} From 160b64523e4cb17fdcab5a993ba33b02bf1e951e Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Thu, 13 Nov 2025 19:02:53 +0800 Subject: [PATCH 05/10] Add Interactive Approval Mode Dialog (#1012) --- packages/cli/src/config/settingsSchema.ts | 15 +- packages/cli/src/ui/AppContainer.tsx | 18 +- .../ui/commands/approvalModeCommand.test.ts | 476 +----------------- .../src/ui/commands/approvalModeCommand.ts | 423 +--------------- packages/cli/src/ui/commands/types.ts | 3 +- .../src/ui/components/ApprovalModeDialog.tsx | 183 +++++++ .../cli/src/ui/components/DialogManager.tsx | 17 + .../cli/src/ui/components/SettingsDialog.tsx | 65 ++- .../SettingsDialog.test.tsx.snap | 10 - .../__snapshots__/ThemeDialog.test.tsx.snap | 1 - .../cli/src/ui/contexts/UIActionsContext.tsx | 10 +- .../cli/src/ui/contexts/UIStateContext.tsx | 1 + .../cli/src/ui/hooks/slashCommandProcessor.ts | 4 + .../src/ui/hooks/useApprovalModeCommand.ts | 57 +++ packages/cli/src/ui/hooks/useDialogClose.ts | 15 +- packages/cli/src/utils/dialogScopeUtils.ts | 8 +- 16 files changed, 397 insertions(+), 909 deletions(-) create mode 100644 packages/cli/src/ui/components/ApprovalModeDialog.tsx create mode 100644 packages/cli/src/ui/hooks/useApprovalModeCommand.ts diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index b36ee397..70037dfd 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -12,6 +12,7 @@ import type { ChatCompressionSettings, } from '@qwen-code/qwen-code-core'; import { + ApprovalMode, DEFAULT_TRUNCATE_TOOL_OUTPUT_LINES, DEFAULT_TRUNCATE_TOOL_OUTPUT_THRESHOLD, } from '@qwen-code/qwen-code-core'; @@ -830,14 +831,20 @@ const SETTINGS_SCHEMA = { mergeStrategy: MergeStrategy.UNION, }, approvalMode: { - type: 'string', - label: 'Default Approval Mode', + type: 'enum', + label: 'Approval Mode', category: 'Tools', requiresRestart: false, - default: 'default', + default: ApprovalMode.DEFAULT, description: - 'Default approval mode for tool usage. Valid values: plan, default, auto-edit, yolo.', + 'Approval mode for tool usage. Controls how tools are approved before execution.', showInDialog: true, + options: [ + { value: ApprovalMode.PLAN, label: 'Plan' }, + { value: ApprovalMode.DEFAULT, label: 'Default' }, + { value: ApprovalMode.AUTO_EDIT, label: 'Auto Edit' }, + { value: ApprovalMode.YOLO, label: 'YOLO' }, + ], }, discoveryCommand: { type: 'string', diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 2e66610a..fbfc732b 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -53,6 +53,7 @@ import { useQuotaAndFallback } from './hooks/useQuotaAndFallback.js'; import { useEditorSettings } from './hooks/useEditorSettings.js'; import { useSettingsCommand } from './hooks/useSettingsCommand.js'; import { useModelCommand } from './hooks/useModelCommand.js'; +import { useApprovalModeCommand } from './hooks/useApprovalModeCommand.js'; import { useSlashCommandProcessor } from './hooks/slashCommandProcessor.js'; import { useVimMode } from './contexts/VimModeContext.js'; import { useConsoleMessages } from './hooks/useConsoleMessages.js'; @@ -335,6 +336,12 @@ export const AppContainer = (props: AppContainerProps) => { initializationResult.themeError, ); + const { + isApprovalModeDialogOpen, + openApprovalModeDialog, + handleApprovalModeSelect, + } = useApprovalModeCommand(settings, config); + const { setAuthState, authError, @@ -470,6 +477,7 @@ export const AppContainer = (props: AppContainerProps) => { openSettingsDialog, openModelDialog, openPermissionsDialog, + openApprovalModeDialog, quit: (messages: HistoryItem[]) => { setQuittingMessages(messages); setTimeout(async () => { @@ -495,6 +503,7 @@ export const AppContainer = (props: AppContainerProps) => { setCorgiMode, dispatchExtensionStateUpdate, openPermissionsDialog, + openApprovalModeDialog, addConfirmUpdateExtensionRequest, showQuitConfirmation, openSubagentCreateDialog, @@ -939,6 +948,8 @@ export const AppContainer = (props: AppContainerProps) => { const { closeAnyOpenDialog } = useDialogClose({ isThemeDialogOpen, handleThemeSelect, + isApprovalModeDialogOpen, + handleApprovalModeSelect, isAuthDialogOpen, handleAuthSelect, selectedAuthType: settings.merged.security?.auth?.selectedType, @@ -1188,7 +1199,8 @@ export const AppContainer = (props: AppContainerProps) => { showIdeRestartPrompt || !!proQuotaRequest || isSubagentCreateDialogOpen || - isAgentsManagerDialogOpen; + isAgentsManagerDialogOpen || + isApprovalModeDialogOpen; const pendingHistoryItems = useMemo( () => [...pendingSlashCommandHistoryItems, ...pendingGeminiHistoryItems], @@ -1219,6 +1231,7 @@ export const AppContainer = (props: AppContainerProps) => { isSettingsDialogOpen, isModelDialogOpen, isPermissionsDialogOpen, + isApprovalModeDialogOpen, slashCommands, pendingSlashCommandHistoryItems, commandContext, @@ -1313,6 +1326,7 @@ export const AppContainer = (props: AppContainerProps) => { isSettingsDialogOpen, isModelDialogOpen, isPermissionsDialogOpen, + isApprovalModeDialogOpen, slashCommands, pendingSlashCommandHistoryItems, commandContext, @@ -1393,6 +1407,7 @@ export const AppContainer = (props: AppContainerProps) => { () => ({ handleThemeSelect, handleThemeHighlight, + handleApprovalModeSelect, handleAuthSelect, setAuthState, onAuthError, @@ -1428,6 +1443,7 @@ export const AppContainer = (props: AppContainerProps) => { [ handleThemeSelect, handleThemeHighlight, + handleApprovalModeSelect, handleAuthSelect, setAuthState, onAuthError, diff --git a/packages/cli/src/ui/commands/approvalModeCommand.test.ts b/packages/cli/src/ui/commands/approvalModeCommand.test.ts index c52c84fc..f915a63c 100644 --- a/packages/cli/src/ui/commands/approvalModeCommand.test.ts +++ b/packages/cli/src/ui/commands/approvalModeCommand.test.ts @@ -4,492 +4,68 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect } from 'vitest'; import { approvalModeCommand } from './approvalModeCommand.js'; import { type CommandContext, CommandKind, - type MessageActionReturn, + type OpenDialogActionReturn, } from './types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; -import { ApprovalMode } from '@qwen-code/qwen-code-core'; -import { SettingScope, type LoadedSettings } from '../../config/settings.js'; +import type { LoadedSettings } from '../../config/settings.js'; describe('approvalModeCommand', () => { let mockContext: CommandContext; - let setApprovalModeMock: ReturnType; - let setSettingsValueMock: ReturnType; - const originalEnv = { ...process.env }; - const userSettingsPath = '/mock/user/settings.json'; - const projectSettingsPath = '/mock/project/settings.json'; - const userSettingsFile = { path: userSettingsPath, settings: {} }; - const projectSettingsFile = { path: projectSettingsPath, settings: {} }; - - const getModeSubCommand = (mode: ApprovalMode) => - approvalModeCommand.subCommands?.find((cmd) => cmd.name === mode); - - const getScopeSubCommand = ( - mode: ApprovalMode, - scope: '--session' | '--user' | '--project', - ) => getModeSubCommand(mode)?.subCommands?.find((cmd) => cmd.name === scope); beforeEach(() => { - setApprovalModeMock = vi.fn(); - setSettingsValueMock = vi.fn(); - mockContext = createMockCommandContext({ services: { config: { - getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT), - setApprovalMode: setApprovalModeMock, + getApprovalMode: () => 'default', + setApprovalMode: () => {}, }, settings: { merged: {}, - setValue: setSettingsValueMock, - forScope: vi - .fn() - .mockImplementation((scope: SettingScope) => - scope === SettingScope.User - ? userSettingsFile - : scope === SettingScope.Workspace - ? projectSettingsFile - : { path: '', settings: {} }, - ), + setValue: () => {}, + forScope: () => ({}), } as unknown as LoadedSettings, }, - } as unknown as CommandContext); + }); }); - afterEach(() => { - process.env = { ...originalEnv }; - vi.clearAllMocks(); - }); - - it('should have the correct command properties', () => { + it('should have correct metadata', () => { expect(approvalModeCommand.name).toBe('approval-mode'); - expect(approvalModeCommand.kind).toBe(CommandKind.BUILT_IN); expect(approvalModeCommand.description).toBe( 'View or change the approval mode for tool usage', ); + expect(approvalModeCommand.kind).toBe(CommandKind.BUILT_IN); }); - it('should show current mode, options, and usage when no arguments provided', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( + it('should open approval mode dialog when invoked', async () => { + const result = (await approvalModeCommand.action?.( mockContext, '', - )) as MessageActionReturn; + )) as OpenDialogActionReturn; - expect(result.type).toBe('message'); - expect(result.messageType).toBe('info'); - const expectedMessage = [ - 'Current approval mode: default', - '', - 'Available approval modes:', - ' - plan: Plan mode - Analyze only, do not modify files or execute commands', - ' - default: Default mode - Require approval for file edits or shell commands', - ' - auto-edit: Auto-edit mode - Automatically approve file edits', - ' - yolo: YOLO mode - Automatically approve all tools', - '', - 'Usage: /approval-mode [--session|--user|--project]', - ].join('\n'); - expect(result.content).toBe(expectedMessage); + expect(result.type).toBe('dialog'); + expect(result.dialog).toBe('approval-mode'); }); - it('should display error when config is not available', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } + it('should open approval mode dialog with arguments (ignored)', async () => { + const result = (await approvalModeCommand.action?.( + mockContext, + 'some arguments', + )) as OpenDialogActionReturn; - const nullConfigContext = createMockCommandContext({ - services: { - config: null, - }, - } as unknown as CommandContext); - - const result = (await approvalModeCommand.action( - nullConfigContext, - '', - )) as MessageActionReturn; - - expect(result.type).toBe('message'); - expect(result.messageType).toBe('error'); - expect(result.content).toBe('Configuration not available.'); + expect(result.type).toBe('dialog'); + expect(result.dialog).toBe('approval-mode'); }); - it('should change approval mode when valid mode is provided', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - 'plan', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.PLAN); - expect(setSettingsValueMock).not.toHaveBeenCalled(); - expect(result.type).toBe('message'); - expect(result.messageType).toBe('info'); - expect(result.content).toBe('Approval mode changed to: plan'); + it('should not have subcommands', () => { + expect(approvalModeCommand.subCommands).toBeUndefined(); }); - it('should accept canonical auto-edit mode value', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - 'auto-edit', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.AUTO_EDIT); - expect(setSettingsValueMock).not.toHaveBeenCalled(); - expect(result.type).toBe('message'); - expect(result.messageType).toBe('info'); - expect(result.content).toBe('Approval mode changed to: auto-edit'); - }); - - it('should accept auto-edit alias for compatibility', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - 'auto-edit', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.AUTO_EDIT); - expect(setSettingsValueMock).not.toHaveBeenCalled(); - expect(result.content).toBe('Approval mode changed to: auto-edit'); - }); - - it('should display error when invalid mode is provided', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - 'invalid', - )) as MessageActionReturn; - - expect(result.type).toBe('message'); - expect(result.messageType).toBe('error'); - expect(result.content).toContain('Invalid approval mode: invalid'); - expect(result.content).toContain('Available approval modes:'); - expect(result.content).toContain( - 'Usage: /approval-mode [--session|--user|--project]', - ); - }); - - it('should display error when setApprovalMode throws an error', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const errorMessage = 'Failed to set approval mode'; - mockContext.services.config!.setApprovalMode = vi - .fn() - .mockImplementation(() => { - throw new Error(errorMessage); - }); - - const result = (await approvalModeCommand.action( - mockContext, - 'plan', - )) as MessageActionReturn; - - expect(result.type).toBe('message'); - expect(result.messageType).toBe('error'); - expect(result.content).toBe( - `Failed to change approval mode: ${errorMessage}`, - ); - }); - - it('should allow selecting auto-edit with user scope via nested subcommands', async () => { - if (!approvalModeCommand.subCommands) { - throw new Error('approvalModeCommand must have subCommands.'); - } - - const userSubCommand = getScopeSubCommand(ApprovalMode.AUTO_EDIT, '--user'); - if (!userSubCommand?.action) { - throw new Error('--user scope subcommand must have an action.'); - } - - const result = (await userSubCommand.action( - mockContext, - '', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.AUTO_EDIT); - expect(setSettingsValueMock).toHaveBeenCalledWith( - SettingScope.User, - 'approvalMode', - 'auto-edit', - ); - expect(result.content).toBe( - `Approval mode changed to: auto-edit (saved to user settings at ${userSettingsPath})`, - ); - }); - - it('should allow selecting plan with project scope via nested subcommands', async () => { - if (!approvalModeCommand.subCommands) { - throw new Error('approvalModeCommand must have subCommands.'); - } - - const projectSubCommand = getScopeSubCommand( - ApprovalMode.PLAN, - '--project', - ); - if (!projectSubCommand?.action) { - throw new Error('--project scope subcommand must have an action.'); - } - - const result = (await projectSubCommand.action( - mockContext, - '', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.PLAN); - expect(setSettingsValueMock).toHaveBeenCalledWith( - SettingScope.Workspace, - 'approvalMode', - 'plan', - ); - expect(result.content).toBe( - `Approval mode changed to: plan (saved to project settings at ${projectSettingsPath})`, - ); - }); - - it('should allow selecting plan with session scope via nested subcommands', async () => { - if (!approvalModeCommand.subCommands) { - throw new Error('approvalModeCommand must have subCommands.'); - } - - const sessionSubCommand = getScopeSubCommand( - ApprovalMode.PLAN, - '--session', - ); - if (!sessionSubCommand?.action) { - throw new Error('--session scope subcommand must have an action.'); - } - - const result = (await sessionSubCommand.action( - mockContext, - '', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.PLAN); - expect(setSettingsValueMock).not.toHaveBeenCalled(); - expect(result.content).toBe('Approval mode changed to: plan'); - }); - - it('should allow providing a scope argument after selecting a mode subcommand', async () => { - if (!approvalModeCommand.subCommands) { - throw new Error('approvalModeCommand must have subCommands.'); - } - - const planSubCommand = getModeSubCommand(ApprovalMode.PLAN); - if (!planSubCommand?.action) { - throw new Error('plan subcommand must have an action.'); - } - - const result = (await planSubCommand.action( - mockContext, - '--user', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.PLAN); - expect(setSettingsValueMock).toHaveBeenCalledWith( - SettingScope.User, - 'approvalMode', - 'plan', - ); - expect(result.content).toBe( - `Approval mode changed to: plan (saved to user settings at ${userSettingsPath})`, - ); - }); - - it('should support --user plan pattern (scope first)', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - '--user plan', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.PLAN); - expect(setSettingsValueMock).toHaveBeenCalledWith( - SettingScope.User, - 'approvalMode', - 'plan', - ); - expect(result.content).toBe( - `Approval mode changed to: plan (saved to user settings at ${userSettingsPath})`, - ); - }); - - it('should support plan --user pattern (mode first)', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - 'plan --user', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.PLAN); - expect(setSettingsValueMock).toHaveBeenCalledWith( - SettingScope.User, - 'approvalMode', - 'plan', - ); - expect(result.content).toBe( - `Approval mode changed to: plan (saved to user settings at ${userSettingsPath})`, - ); - }); - - it('should support --project auto-edit pattern', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - '--project auto-edit', - )) as MessageActionReturn; - - expect(setApprovalModeMock).toHaveBeenCalledWith(ApprovalMode.AUTO_EDIT); - expect(setSettingsValueMock).toHaveBeenCalledWith( - SettingScope.Workspace, - 'approvalMode', - 'auto-edit', - ); - expect(result.content).toBe( - `Approval mode changed to: auto-edit (saved to project settings at ${projectSettingsPath})`, - ); - }); - - it('should display error when only scope flag is provided', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - '--user', - )) as MessageActionReturn; - - expect(result.type).toBe('message'); - expect(result.messageType).toBe('error'); - expect(result.content).toContain('Missing approval mode'); - expect(setApprovalModeMock).not.toHaveBeenCalled(); - expect(setSettingsValueMock).not.toHaveBeenCalled(); - }); - - it('should display error when multiple scope flags are provided', async () => { - if (!approvalModeCommand.action) { - throw new Error('approvalModeCommand must have an action.'); - } - - const result = (await approvalModeCommand.action( - mockContext, - '--user --project plan', - )) as MessageActionReturn; - - expect(result.type).toBe('message'); - expect(result.messageType).toBe('error'); - expect(result.content).toContain('Multiple scope flags provided'); - expect(setApprovalModeMock).not.toHaveBeenCalled(); - expect(setSettingsValueMock).not.toHaveBeenCalled(); - }); - - it('should surface a helpful error when scope subcommands receive extra arguments', async () => { - if (!approvalModeCommand.subCommands) { - throw new Error('approvalModeCommand must have subCommands.'); - } - - const userSubCommand = getScopeSubCommand(ApprovalMode.DEFAULT, '--user'); - if (!userSubCommand?.action) { - throw new Error('--user scope subcommand must have an action.'); - } - - const result = (await userSubCommand.action( - mockContext, - 'extra', - )) as MessageActionReturn; - - expect(result.type).toBe('message'); - expect(result.messageType).toBe('error'); - expect(result.content).toBe( - 'Scope subcommands do not accept additional arguments.', - ); - expect(setApprovalModeMock).not.toHaveBeenCalled(); - expect(setSettingsValueMock).not.toHaveBeenCalled(); - }); - - it('should provide completion for approval modes', async () => { - if (!approvalModeCommand.completion) { - throw new Error('approvalModeCommand must have a completion function.'); - } - - // Test partial mode completion - const result = await approvalModeCommand.completion(mockContext, 'p'); - expect(result).toEqual(['plan']); - - const result2 = await approvalModeCommand.completion(mockContext, 'a'); - expect(result2).toEqual(['auto-edit']); - - // Test empty completion - should suggest available modes first - const result3 = await approvalModeCommand.completion(mockContext, ''); - expect(result3).toEqual(['plan', 'default', 'auto-edit', 'yolo']); - - const result4 = await approvalModeCommand.completion(mockContext, 'AUTO'); - expect(result4).toEqual(['auto-edit']); - - // Test mode first pattern: 'plan ' should suggest scope flags - const result5 = await approvalModeCommand.completion(mockContext, 'plan '); - expect(result5).toEqual(['--session', '--project', '--user']); - - const result6 = await approvalModeCommand.completion( - mockContext, - 'plan --u', - ); - expect(result6).toEqual(['--user']); - - // Test scope first pattern: '--user ' should suggest modes - const result7 = await approvalModeCommand.completion( - mockContext, - '--user ', - ); - expect(result7).toEqual(['plan', 'default', 'auto-edit', 'yolo']); - - const result8 = await approvalModeCommand.completion( - mockContext, - '--user p', - ); - expect(result8).toEqual(['plan']); - - // Test completed patterns should return empty - const result9 = await approvalModeCommand.completion( - mockContext, - 'plan --user ', - ); - expect(result9).toEqual([]); - - const result10 = await approvalModeCommand.completion( - mockContext, - '--user plan ', - ); - expect(result10).toEqual([]); + it('should not have completion function', () => { + expect(approvalModeCommand.completion).toBeUndefined(); }); }); diff --git a/packages/cli/src/ui/commands/approvalModeCommand.ts b/packages/cli/src/ui/commands/approvalModeCommand.ts index 6cef96c2..5528d86f 100644 --- a/packages/cli/src/ui/commands/approvalModeCommand.ts +++ b/packages/cli/src/ui/commands/approvalModeCommand.ts @@ -7,428 +7,19 @@ import type { SlashCommand, CommandContext, - MessageActionReturn, + OpenDialogActionReturn, } from './types.js'; import { CommandKind } from './types.js'; -import { ApprovalMode, APPROVAL_MODES } from '@qwen-code/qwen-code-core'; -import { SettingScope } from '../../config/settings.js'; - -const USAGE_MESSAGE = - 'Usage: /approval-mode [--session|--user|--project]'; - -const normalizeInputMode = (value: string): string => - value.trim().toLowerCase(); - -const tokenizeArgs = (args: string): string[] => { - const matches = args.match(/(?:"[^"]*"|'[^']*'|[^\s"']+)/g); - if (!matches) { - return []; - } - - return matches.map((token) => { - if ( - (token.startsWith('"') && token.endsWith('"')) || - (token.startsWith("'") && token.endsWith("'")) - ) { - return token.slice(1, -1); - } - return token; - }); -}; - -const parseApprovalMode = (value: string | null): ApprovalMode | null => { - if (!value) { - return null; - } - - const normalized = normalizeInputMode(value).replace(/_/g, '-'); - const matchIndex = APPROVAL_MODES.findIndex( - (candidate) => candidate === normalized, - ); - - return matchIndex === -1 ? null : APPROVAL_MODES[matchIndex]; -}; - -const formatModeDescription = (mode: ApprovalMode): string => { - switch (mode) { - case ApprovalMode.PLAN: - return 'Plan mode - Analyze only, do not modify files or execute commands'; - case ApprovalMode.DEFAULT: - return 'Default mode - Require approval for file edits or shell commands'; - case ApprovalMode.AUTO_EDIT: - return 'Auto-edit mode - Automatically approve file edits'; - case ApprovalMode.YOLO: - return 'YOLO mode - Automatically approve all tools'; - default: - return `${mode} mode`; - } -}; - -const parseApprovalArgs = ( - args: string, -): { - mode: string | null; - scope: 'session' | 'user' | 'project'; - error?: string; -} => { - const trimmedArgs = args.trim(); - if (!trimmedArgs) { - return { mode: null, scope: 'session' }; - } - - const tokens = tokenizeArgs(trimmedArgs); - let mode: string | null = null; - let scope: 'session' | 'user' | 'project' = 'session'; - let scopeFlag: string | null = null; - - // Find scope flag and mode - for (const token of tokens) { - if (token === '--session' || token === '--user' || token === '--project') { - if (scopeFlag) { - return { - mode: null, - scope: 'session', - error: 'Multiple scope flags provided', - }; - } - scopeFlag = token; - scope = token.substring(2) as 'session' | 'user' | 'project'; - } else if (!mode) { - mode = token; - } else { - return { - mode: null, - scope: 'session', - error: 'Invalid arguments provided', - }; - } - } - - if (!mode) { - return { mode: null, scope: 'session', error: 'Missing approval mode' }; - } - - return { mode, scope }; -}; - -const setApprovalModeWithScope = async ( - context: CommandContext, - mode: ApprovalMode, - scope: 'session' | 'user' | 'project', -): Promise => { - const { services } = context; - const { config } = services; - - if (!config) { - return { - type: 'message', - messageType: 'error', - content: 'Configuration not available.', - }; - } - - try { - // Always set the mode in the current session - config.setApprovalMode(mode); - - // If scope is not session, also persist to settings - if (scope !== 'session') { - const { settings } = context.services; - if (!settings || typeof settings.setValue !== 'function') { - return { - type: 'message', - messageType: 'error', - content: - 'Settings service is not available; unable to persist the approval mode.', - }; - } - - const settingScope = - scope === 'user' ? SettingScope.User : SettingScope.Workspace; - const scopeLabel = scope === 'user' ? 'user' : 'project'; - let settingsPath: string | undefined; - - try { - if (typeof settings.forScope === 'function') { - settingsPath = settings.forScope(settingScope)?.path; - } - } catch (_error) { - settingsPath = undefined; - } - - try { - settings.setValue(settingScope, 'approvalMode', mode); - } catch (error) { - return { - type: 'message', - messageType: 'error', - content: `Failed to save approval mode: ${(error as Error).message}`, - }; - } - - const locationSuffix = settingsPath ? ` at ${settingsPath}` : ''; - - const scopeSuffix = ` (saved to ${scopeLabel} settings${locationSuffix})`; - - return { - type: 'message', - messageType: 'info', - content: `Approval mode changed to: ${mode}${scopeSuffix}`, - }; - } - - return { - type: 'message', - messageType: 'info', - content: `Approval mode changed to: ${mode}`, - }; - } catch (error) { - return { - type: 'message', - messageType: 'error', - content: `Failed to change approval mode: ${(error as Error).message}`, - }; - } -}; export const approvalModeCommand: SlashCommand = { name: 'approval-mode', description: 'View or change the approval mode for tool usage', kind: CommandKind.BUILT_IN, action: async ( - context: CommandContext, - args: string, - ): Promise => { - const { config } = context.services; - if (!config) { - return { - type: 'message', - messageType: 'error', - content: 'Configuration not available.', - }; - } - - // If no arguments provided, show current mode and available options - if (!args || args.trim() === '') { - const currentMode = - typeof config.getApprovalMode === 'function' - ? config.getApprovalMode() - : null; - - const messageLines: string[] = []; - - if (currentMode) { - messageLines.push(`Current approval mode: ${currentMode}`); - messageLines.push(''); - } - - messageLines.push('Available approval modes:'); - for (const mode of APPROVAL_MODES) { - messageLines.push(` - ${mode}: ${formatModeDescription(mode)}`); - } - messageLines.push(''); - messageLines.push(USAGE_MESSAGE); - - return { - type: 'message', - messageType: 'info', - content: messageLines.join('\n'), - }; - } - - // Parse arguments flexibly - const parsed = parseApprovalArgs(args); - - if (parsed.error) { - return { - type: 'message', - messageType: 'error', - content: `${parsed.error}. ${USAGE_MESSAGE}`, - }; - } - - if (!parsed.mode) { - return { - type: 'message', - messageType: 'info', - content: USAGE_MESSAGE, - }; - } - - const requestedMode = parseApprovalMode(parsed.mode); - - if (!requestedMode) { - let message = `Invalid approval mode: ${parsed.mode}\n\n`; - message += 'Available approval modes:\n'; - for (const mode of APPROVAL_MODES) { - message += ` - ${mode}: ${formatModeDescription(mode)}\n`; - } - message += `\n${USAGE_MESSAGE}`; - return { - type: 'message', - messageType: 'error', - content: message, - }; - } - - return setApprovalModeWithScope(context, requestedMode, parsed.scope); - }, - subCommands: APPROVAL_MODES.map((mode) => ({ - name: mode, - description: formatModeDescription(mode), - kind: CommandKind.BUILT_IN, - subCommands: [ - { - name: '--session', - description: 'Apply to current session only (temporary)', - kind: CommandKind.BUILT_IN, - action: async ( - context: CommandContext, - args: string, - ): Promise => { - if (args.trim().length > 0) { - return { - type: 'message', - messageType: 'error', - content: 'Scope subcommands do not accept additional arguments.', - }; - } - return setApprovalModeWithScope(context, mode, 'session'); - }, - }, - { - name: '--project', - description: 'Persist for this project/workspace', - kind: CommandKind.BUILT_IN, - action: async ( - context: CommandContext, - args: string, - ): Promise => { - if (args.trim().length > 0) { - return { - type: 'message', - messageType: 'error', - content: 'Scope subcommands do not accept additional arguments.', - }; - } - return setApprovalModeWithScope(context, mode, 'project'); - }, - }, - { - name: '--user', - description: 'Persist for this user on this machine', - kind: CommandKind.BUILT_IN, - action: async ( - context: CommandContext, - args: string, - ): Promise => { - if (args.trim().length > 0) { - return { - type: 'message', - messageType: 'error', - content: 'Scope subcommands do not accept additional arguments.', - }; - } - return setApprovalModeWithScope(context, mode, 'user'); - }, - }, - ], - action: async ( - context: CommandContext, - args: string, - ): Promise => { - if (args.trim().length > 0) { - // Allow users who type `/approval-mode plan --user` via the subcommand path - const parsed = parseApprovalArgs(`${mode} ${args}`); - if (parsed.error) { - return { - type: 'message', - messageType: 'error', - content: `${parsed.error}. ${USAGE_MESSAGE}`, - }; - } - - const normalizedMode = parseApprovalMode(parsed.mode); - if (!normalizedMode) { - return { - type: 'message', - messageType: 'error', - content: `Invalid approval mode: ${parsed.mode}. ${USAGE_MESSAGE}`, - }; - } - - return setApprovalModeWithScope(context, normalizedMode, parsed.scope); - } - - return setApprovalModeWithScope(context, mode, 'session'); - }, - })), - completion: async (_context: CommandContext, partialArg: string) => { - const tokens = tokenizeArgs(partialArg); - const hasTrailingSpace = /\s$/.test(partialArg); - const currentSegment = hasTrailingSpace - ? '' - : tokens.length > 0 - ? tokens[tokens.length - 1] - : ''; - - const normalizedCurrent = normalizeInputMode(currentSegment).replace( - /_/g, - '-', - ); - - const scopeValues = ['--session', '--project', '--user']; - - const normalizeToken = (token: string) => - normalizeInputMode(token).replace(/_/g, '-'); - - const normalizedTokens = tokens.map(normalizeToken); - - if (tokens.length === 0) { - if (currentSegment.startsWith('-')) { - return scopeValues.filter((scope) => scope.startsWith(currentSegment)); - } - return APPROVAL_MODES; - } - - if (tokens.length === 1 && !hasTrailingSpace) { - const originalToken = tokens[0]; - if (originalToken.startsWith('-')) { - return scopeValues.filter((scope) => - scope.startsWith(normalizedCurrent), - ); - } - return APPROVAL_MODES.filter((mode) => - mode.startsWith(normalizedCurrent), - ); - } - - if (tokens.length === 1 && hasTrailingSpace) { - const normalizedFirst = normalizedTokens[0]; - if (scopeValues.includes(tokens[0])) { - return APPROVAL_MODES; - } - if (APPROVAL_MODES.includes(normalizedFirst as ApprovalMode)) { - return scopeValues; - } - return APPROVAL_MODES; - } - - if (tokens.length === 2 && !hasTrailingSpace) { - const normalizedFirst = normalizedTokens[0]; - if (scopeValues.includes(tokens[0])) { - return APPROVAL_MODES.filter((mode) => - mode.startsWith(normalizedCurrent), - ); - } - if (APPROVAL_MODES.includes(normalizedFirst as ApprovalMode)) { - return scopeValues.filter((scope) => - scope.startsWith(normalizedCurrent), - ); - } - return []; - } - - return []; - }, + _context: CommandContext, + _args: string, + ): Promise => ({ + type: 'dialog', + dialog: 'approval-mode', + }), }; diff --git a/packages/cli/src/ui/commands/types.ts b/packages/cli/src/ui/commands/types.ts index e9ee4677..e865c07e 100644 --- a/packages/cli/src/ui/commands/types.ts +++ b/packages/cli/src/ui/commands/types.ts @@ -129,7 +129,8 @@ export interface OpenDialogActionReturn { | 'model' | 'subagent_create' | 'subagent_list' - | 'permissions'; + | 'permissions' + | 'approval-mode'; } /** diff --git a/packages/cli/src/ui/components/ApprovalModeDialog.tsx b/packages/cli/src/ui/components/ApprovalModeDialog.tsx new file mode 100644 index 00000000..eb6441ec --- /dev/null +++ b/packages/cli/src/ui/components/ApprovalModeDialog.tsx @@ -0,0 +1,183 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import type React from 'react'; +import { useCallback, useState } from 'react'; +import { Box, Text } from 'ink'; +import { theme } from '../semantic-colors.js'; +import { ApprovalMode, APPROVAL_MODES } from '@qwen-code/qwen-code-core'; +import { RadioButtonSelect } from './shared/RadioButtonSelect.js'; +import type { LoadedSettings } from '../../config/settings.js'; +import { SettingScope } from '../../config/settings.js'; +import { getScopeMessageForSetting } from '../../utils/dialogScopeUtils.js'; +import { useKeypress } from '../hooks/useKeypress.js'; +import { ScopeSelector } from './shared/ScopeSelector.js'; + +interface ApprovalModeDialogProps { + /** Callback function when an approval mode is selected */ + onSelect: (mode: ApprovalMode | undefined, scope: SettingScope) => void; + + /** The settings object */ + settings: LoadedSettings; + + /** Current approval mode */ + currentMode: ApprovalMode; + + /** Available terminal height for layout calculations */ + availableTerminalHeight?: number; +} + +const formatModeDescription = (mode: ApprovalMode): string => { + switch (mode) { + case ApprovalMode.PLAN: + return 'Analyze only, do not modify files or execute commands'; + case ApprovalMode.DEFAULT: + return 'Require approval for file edits or shell commands'; + case ApprovalMode.AUTO_EDIT: + return 'Automatically approve file edits'; + case ApprovalMode.YOLO: + return 'Automatically approve all tools'; + default: + return `${mode} mode`; + } +}; + +export function ApprovalModeDialog({ + onSelect, + settings, + currentMode, + availableTerminalHeight: _availableTerminalHeight, +}: ApprovalModeDialogProps): React.JSX.Element { + // Start with User scope by default + const [selectedScope, setSelectedScope] = useState( + SettingScope.User, + ); + + // Track the currently highlighted approval mode + const [highlightedMode, setHighlightedMode] = useState( + currentMode || ApprovalMode.DEFAULT, + ); + + // Generate approval mode items with inline descriptions + const modeItems = APPROVAL_MODES.map((mode) => ({ + label: `${mode} - ${formatModeDescription(mode)}`, + value: mode, + key: mode, + })); + + // Find the index of the current mode + const initialModeIndex = modeItems.findIndex( + (item) => item.value === highlightedMode, + ); + const safeInitialModeIndex = initialModeIndex >= 0 ? initialModeIndex : 0; + + const handleModeSelect = useCallback( + (mode: ApprovalMode) => { + onSelect(mode, selectedScope); + }, + [onSelect, selectedScope], + ); + + const handleModeHighlight = (mode: ApprovalMode) => { + setHighlightedMode(mode); + }; + + const handleScopeHighlight = useCallback((scope: SettingScope) => { + setSelectedScope(scope); + }, []); + + const handleScopeSelect = useCallback( + (scope: SettingScope) => { + onSelect(highlightedMode, scope); + }, + [onSelect, highlightedMode], + ); + + const [focusSection, setFocusSection] = useState<'mode' | 'scope'>('mode'); + + useKeypress( + (key) => { + if (key.name === 'tab') { + setFocusSection((prev) => (prev === 'mode' ? 'scope' : 'mode')); + } + if (key.name === 'escape') { + onSelect(undefined, selectedScope); + } + }, + { isActive: true }, + ); + + // Generate scope message for approval mode setting + const otherScopeModifiedMessage = getScopeMessageForSetting( + 'tools.approvalMode', + selectedScope, + settings, + ); + + // Check if user scope is selected but workspace has the setting + const showWorkspacePriorityWarning = + selectedScope === SettingScope.User && + otherScopeModifiedMessage.toLowerCase().includes('workspace'); + + return ( + + + {/* Approval Mode Selection */} + + {focusSection === 'mode' ? '> ' : ' '}Approval Mode{' '} + {otherScopeModifiedMessage} + + + + + + + {/* Scope Selection */} + + + + + + + {/* Warning when workspace setting will override user setting */} + {showWorkspacePriorityWarning && ( + <> + + ⚠ Workspace approval mode exists and takes priority. User-level + change will have no effect. + + + + )} + + + (Use Enter to select, Tab to change focus) + + + + ); +} diff --git a/packages/cli/src/ui/components/DialogManager.tsx b/packages/cli/src/ui/components/DialogManager.tsx index f174a8d0..01d95392 100644 --- a/packages/cli/src/ui/components/DialogManager.tsx +++ b/packages/cli/src/ui/components/DialogManager.tsx @@ -20,6 +20,7 @@ import { WorkspaceMigrationDialog } from './WorkspaceMigrationDialog.js'; import { ProQuotaDialog } from './ProQuotaDialog.js'; import { PermissionsModifyTrustDialog } from './PermissionsModifyTrustDialog.js'; import { ModelDialog } from './ModelDialog.js'; +import { ApprovalModeDialog } from './ApprovalModeDialog.js'; import { theme } from '../semantic-colors.js'; import { useUIState } from '../contexts/UIStateContext.js'; import { useUIActions } from '../contexts/UIActionsContext.js'; @@ -180,6 +181,22 @@ export const DialogManager = ({ onSelect={() => uiActions.closeSettingsDialog()} onRestartRequest={() => process.exit(0)} availableTerminalHeight={terminalHeight - staticExtraHeight} + config={config} + /> + + ); + } + if (uiState.isApprovalModeDialogOpen) { + const currentMode = config.getApprovalMode(); + return ( + + ); diff --git a/packages/cli/src/ui/components/SettingsDialog.tsx b/packages/cli/src/ui/components/SettingsDialog.tsx index b9e1559d..210672bb 100644 --- a/packages/cli/src/ui/components/SettingsDialog.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.tsx @@ -9,11 +9,8 @@ import { Box, Text } from 'ink'; import { theme } from '../semantic-colors.js'; import type { LoadedSettings, Settings } from '../../config/settings.js'; import { SettingScope } from '../../config/settings.js'; -import { - getScopeItems, - getScopeMessageForSetting, -} from '../../utils/dialogScopeUtils.js'; -import { RadioButtonSelect } from './shared/RadioButtonSelect.js'; +import { getScopeMessageForSetting } from '../../utils/dialogScopeUtils.js'; +import { ScopeSelector } from './shared/ScopeSelector.js'; import { getDialogSettingKeys, setPendingSettingValue, @@ -30,6 +27,7 @@ import { getEffectiveValue, } from '../../utils/settingsUtils.js'; import { useVimMode } from '../contexts/VimModeContext.js'; +import { type Config } from '@qwen-code/qwen-code-core'; import { useKeypress } from '../hooks/useKeypress.js'; import chalk from 'chalk'; import { cpSlice, cpLen, stripUnsafeCharacters } from '../utils/textUtils.js'; @@ -43,6 +41,7 @@ interface SettingsDialogProps { onSelect: (settingName: string | undefined, scope: SettingScope) => void; onRestartRequest?: () => void; availableTerminalHeight?: number; + config?: Config; } const maxItemsToShow = 8; @@ -52,6 +51,7 @@ export function SettingsDialog({ onSelect, onRestartRequest, availableTerminalHeight, + config, }: SettingsDialogProps): React.JSX.Element { // Get vim mode context to sync vim mode changes const { vimEnabled, toggleVimEnabled } = useVimMode(); @@ -184,6 +184,21 @@ export function SettingsDialog({ }); } + // Special handling for approval mode to apply to current session + if ( + key === 'tools.approvalMode' && + settings.merged.tools?.approvalMode + ) { + try { + config?.setApprovalMode(settings.merged.tools.approvalMode); + } catch (error) { + console.error( + 'Failed to apply approval mode to current session:', + error, + ); + } + } + // Remove from modifiedSettings since it's now saved setModifiedSettings((prev) => { const updated = new Set(prev); @@ -357,12 +372,6 @@ export function SettingsDialog({ setEditCursorPos(0); }; - // Scope selector items - const scopeItems = getScopeItems().map((item) => ({ - ...item, - key: item.value, - })); - const handleScopeHighlight = (scope: SettingScope) => { setSelectedScope(scope); }; @@ -616,7 +625,11 @@ export function SettingsDialog({ prev, ), ); - } else if (defType === 'number' || defType === 'string') { + } else if ( + defType === 'number' || + defType === 'string' || + defType === 'enum' + ) { if ( typeof defaultValue === 'number' || typeof defaultValue === 'string' @@ -673,6 +686,21 @@ export function SettingsDialog({ selectedScope, ); + // Special handling for approval mode to apply to current session + if ( + currentSetting.value === 'tools.approvalMode' && + settings.merged.tools?.approvalMode + ) { + try { + config?.setApprovalMode(settings.merged.tools.approvalMode); + } catch (error) { + console.error( + 'Failed to apply approval mode to current session:', + error, + ); + } + } + // Remove from global pending changes if present setGlobalPendingChanges((prev) => { if (!prev.has(currentSetting.value)) return prev; @@ -876,19 +904,12 @@ export function SettingsDialog({ {/* Scope Selection - conditionally visible based on height constraints */} {showScopeSelection && ( - - - {focusSection === 'scope' ? '> ' : ' '}Apply To - - item.value === selectedScope, - )} + + )} diff --git a/packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snap index 5e528375..b63948e1 100644 --- a/packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/SettingsDialog.test.tsx.snap @@ -28,7 +28,6 @@ exports[`SettingsDialog > Snapshot Tests > should render default state correctly │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -63,7 +62,6 @@ exports[`SettingsDialog > Snapshot Tests > should render focused on scope select │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -98,7 +96,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with accessibility sett │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -133,7 +130,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with all boolean settin │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -168,7 +164,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with different scope se │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -203,7 +198,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with different scope se │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -238,7 +232,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with file filtering set │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -273,7 +266,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with mixed boolean and │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -308,7 +300,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with tools and security │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ @@ -343,7 +334,6 @@ exports[`SettingsDialog > Snapshot Tests > should render with various boolean se │ Apply To │ │ ● User Settings │ │ Workspace Settings │ -│ System Settings │ │ │ │ (Use Enter to select, Tab to change focus) │ │ │ diff --git a/packages/cli/src/ui/components/__snapshots__/ThemeDialog.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/ThemeDialog.test.tsx.snap index 084525d5..09787eca 100644 --- a/packages/cli/src/ui/components/__snapshots__/ThemeDialog.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/ThemeDialog.test.tsx.snap @@ -6,7 +6,6 @@ exports[`ThemeDialog Snapshots > should render correctly in scope selector mode │ > Apply To │ │ ● 1. User Settings │ │ 2. Workspace Settings │ -│ 3. System Settings │ │ │ │ (Use Enter to apply scope, Tab to select theme) │ │ │ diff --git a/packages/cli/src/ui/contexts/UIActionsContext.tsx b/packages/cli/src/ui/contexts/UIActionsContext.tsx index e6802965..409b4c4c 100644 --- a/packages/cli/src/ui/contexts/UIActionsContext.tsx +++ b/packages/cli/src/ui/contexts/UIActionsContext.tsx @@ -8,7 +8,11 @@ import { createContext, useContext } from 'react'; import { type Key } from '../hooks/useKeypress.js'; import { type IdeIntegrationNudgeResult } from '../IdeIntegrationNudge.js'; import { type FolderTrustChoice } from '../components/FolderTrustDialog.js'; -import { type AuthType, type EditorType } from '@qwen-code/qwen-code-core'; +import { + type AuthType, + type EditorType, + type ApprovalMode, +} from '@qwen-code/qwen-code-core'; import { type SettingScope } from '../../config/settings.js'; import type { AuthState } from '../types.js'; import { type VisionSwitchOutcome } from '../components/ModelSwitchDialog.js'; @@ -19,6 +23,10 @@ export interface UIActions { scope: SettingScope, ) => void; handleThemeHighlight: (themeName: string | undefined) => void; + handleApprovalModeSelect: ( + mode: ApprovalMode | undefined, + scope: SettingScope, + ) => void; handleAuthSelect: ( authType: AuthType | undefined, scope: SettingScope, diff --git a/packages/cli/src/ui/contexts/UIStateContext.tsx b/packages/cli/src/ui/contexts/UIStateContext.tsx index e2fd4cf5..fae2db66 100644 --- a/packages/cli/src/ui/contexts/UIStateContext.tsx +++ b/packages/cli/src/ui/contexts/UIStateContext.tsx @@ -69,6 +69,7 @@ export interface UIState { isSettingsDialogOpen: boolean; isModelDialogOpen: boolean; isPermissionsDialogOpen: boolean; + isApprovalModeDialogOpen: boolean; slashCommands: readonly SlashCommand[]; pendingSlashCommandHistoryItems: HistoryItemWithoutId[]; commandContext: CommandContext; diff --git a/packages/cli/src/ui/hooks/slashCommandProcessor.ts b/packages/cli/src/ui/hooks/slashCommandProcessor.ts index cba3bf7a..45411f94 100644 --- a/packages/cli/src/ui/hooks/slashCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/slashCommandProcessor.ts @@ -48,6 +48,7 @@ interface SlashCommandProcessorActions { openSettingsDialog: () => void; openModelDialog: () => void; openPermissionsDialog: () => void; + openApprovalModeDialog: () => void; quit: (messages: HistoryItem[]) => void; setDebugMessage: (message: string) => void; toggleCorgiMode: () => void; @@ -396,6 +397,9 @@ export const useSlashCommandProcessor = ( case 'subagent_list': actions.openAgentsManagerDialog(); return { type: 'handled' }; + case 'approval-mode': + actions.openApprovalModeDialog(); + return { type: 'handled' }; case 'help': return { type: 'handled' }; default: { diff --git a/packages/cli/src/ui/hooks/useApprovalModeCommand.ts b/packages/cli/src/ui/hooks/useApprovalModeCommand.ts new file mode 100644 index 00000000..f328ded9 --- /dev/null +++ b/packages/cli/src/ui/hooks/useApprovalModeCommand.ts @@ -0,0 +1,57 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useState, useCallback } from 'react'; +import type { ApprovalMode, Config } from '@qwen-code/qwen-code-core'; +import type { LoadedSettings, SettingScope } from '../../config/settings.js'; + +interface UseApprovalModeCommandReturn { + isApprovalModeDialogOpen: boolean; + openApprovalModeDialog: () => void; + handleApprovalModeSelect: ( + mode: ApprovalMode | undefined, + scope: SettingScope, + ) => void; +} + +export const useApprovalModeCommand = ( + loadedSettings: LoadedSettings, + config: Config, +): UseApprovalModeCommandReturn => { + const [isApprovalModeDialogOpen, setIsApprovalModeDialogOpen] = + useState(false); + + const openApprovalModeDialog = useCallback(() => { + setIsApprovalModeDialogOpen(true); + }, []); + + const handleApprovalModeSelect = useCallback( + (mode: ApprovalMode | undefined, scope: SettingScope) => { + try { + if (!mode) { + // User cancelled the dialog + setIsApprovalModeDialogOpen(false); + return; + } + + // Set the mode in the current session and persist to settings + loadedSettings.setValue(scope, 'tools.approvalMode', mode); + config.setApprovalMode( + loadedSettings.merged.tools?.approvalMode ?? mode, + ); + } finally { + setIsApprovalModeDialogOpen(false); + } + }, + [config, loadedSettings], + ); + + return { + isApprovalModeDialogOpen, + openApprovalModeDialog, + handleApprovalModeSelect, + }; +}; diff --git a/packages/cli/src/ui/hooks/useDialogClose.ts b/packages/cli/src/ui/hooks/useDialogClose.ts index 8c944996..06e221ac 100644 --- a/packages/cli/src/ui/hooks/useDialogClose.ts +++ b/packages/cli/src/ui/hooks/useDialogClose.ts @@ -6,13 +6,20 @@ import { useCallback } from 'react'; import { SettingScope } from '../../config/settings.js'; -import type { AuthType } from '@qwen-code/qwen-code-core'; +import type { AuthType, ApprovalMode } from '@qwen-code/qwen-code-core'; export interface DialogCloseOptions { // Theme dialog isThemeDialogOpen: boolean; handleThemeSelect: (theme: string | undefined, scope: SettingScope) => void; + // Approval mode dialog + isApprovalModeDialogOpen: boolean; + handleApprovalModeSelect: ( + mode: ApprovalMode | undefined, + scope: SettingScope, + ) => void; + // Auth dialog isAuthDialogOpen: boolean; handleAuthSelect: ( @@ -57,6 +64,12 @@ export function useDialogClose(options: DialogCloseOptions) { return true; } + if (options.isApprovalModeDialogOpen) { + // Mimic ESC behavior: onSelect(undefined, selectedScope) - keeps current mode + options.handleApprovalModeSelect(undefined, SettingScope.User); + return true; + } + if (options.isEditorDialogOpen) { // Mimic ESC behavior: call onExit() directly options.exitEditorDialog(); diff --git a/packages/cli/src/utils/dialogScopeUtils.ts b/packages/cli/src/utils/dialogScopeUtils.ts index fd4cbbd4..027928ab 100644 --- a/packages/cli/src/utils/dialogScopeUtils.ts +++ b/packages/cli/src/utils/dialogScopeUtils.ts @@ -14,7 +14,11 @@ import { settingExistsInScope } from './settingsUtils.js'; export const SCOPE_LABELS = { [SettingScope.User]: 'User Settings', [SettingScope.Workspace]: 'Workspace Settings', - [SettingScope.System]: 'System Settings', + + // TODO: migrate system settings to user settings + // we don't want to save settings to system scope, it is a troublemaker + // comment it out for now. + // [SettingScope.System]: 'System Settings', } as const; /** @@ -27,7 +31,7 @@ export function getScopeItems() { label: SCOPE_LABELS[SettingScope.Workspace], value: SettingScope.Workspace, }, - { label: SCOPE_LABELS[SettingScope.System], value: SettingScope.System }, + // { label: SCOPE_LABELS[SettingScope.System], value: SettingScope.System }, ]; } From e6d08f0596b068294ca7f606a562efd5321b1c73 Mon Sep 17 00:00:00 2001 From: XinlongWu <43398706+Xinlong-Wu@users.noreply.github.com> Date: Thu, 13 Nov 2025 19:12:10 +0800 Subject: [PATCH 06/10] Change deepseek token limits regex patterns for deepseek-chat (#817) --- packages/core/src/core/tokenLimits.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/core/src/core/tokenLimits.ts b/packages/core/src/core/tokenLimits.ts index f2693075..91471580 100644 --- a/packages/core/src/core/tokenLimits.ts +++ b/packages/core/src/core/tokenLimits.ts @@ -165,9 +165,7 @@ const PATTERNS: Array<[RegExp, TokenCount]> = [ // ------------------- // DeepSeek // ------------------- - [/^deepseek$/, LIMITS['128k']], - [/^deepseek-r1(?:-.*)?$/, LIMITS['128k']], - [/^deepseek-v3(?:\.\d+)?(?:-.*)?$/, LIMITS['128k']], + [/^deepseek(?:-.*)?$/, LIMITS['128k']], // ------------------- // Moonshot / Kimi @@ -211,6 +209,12 @@ const OUTPUT_PATTERNS: Array<[RegExp, TokenCount]> = [ // Qwen3-VL-Plus: 32K max output tokens [/^qwen3-vl-plus$/, LIMITS['32k']], + + // Deepseek-chat: 8k max tokens + [/^deepseek-chat$/, LIMITS['8k']], + + // Deepseek-reasoner: 64k max tokens + [/^deepseek-reasoner$/, LIMITS['64k']], ]; /** From b4eba6584ad96675a9cf6da600b97368ba7e20fc Mon Sep 17 00:00:00 2001 From: tanzhenxin Date: Thu, 13 Nov 2025 20:39:14 +0800 Subject: [PATCH 07/10] chore: pump version to 0.2.2 (#1027) --- package-lock.json | 12 ++++++------ package.json | 4 ++-- packages/cli/package.json | 4 ++-- packages/core/package.json | 2 +- packages/test-utils/package.json | 2 +- packages/vscode-ide-companion/package.json | 2 +- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/package-lock.json b/package-lock.json index 6cbcac5b..296fc29b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@qwen-code/qwen-code", - "version": "0.2.1", + "version": "0.2.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@qwen-code/qwen-code", - "version": "0.2.1", + "version": "0.2.2", "workspaces": [ "packages/*" ], @@ -16024,7 +16024,7 @@ }, "packages/cli": { "name": "@qwen-code/qwen-code", - "version": "0.2.1", + "version": "0.2.2", "dependencies": { "@google/genai": "1.16.0", "@iarna/toml": "^2.2.5", @@ -16139,7 +16139,7 @@ }, "packages/core": { "name": "@qwen-code/qwen-code-core", - "version": "0.2.1", + "version": "0.2.2", "hasInstallScript": true, "dependencies": { "@google/genai": "1.16.0", @@ -16278,7 +16278,7 @@ }, "packages/test-utils": { "name": "@qwen-code/qwen-code-test-utils", - "version": "0.2.1", + "version": "0.2.2", "dev": true, "license": "Apache-2.0", "devDependencies": { @@ -16290,7 +16290,7 @@ }, "packages/vscode-ide-companion": { "name": "qwen-code-vscode-ide-companion", - "version": "0.2.1", + "version": "0.2.2", "license": "LICENSE", "dependencies": { "@modelcontextprotocol/sdk": "^1.15.1", diff --git a/package.json b/package.json index 465eef63..a8b69061 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code", - "version": "0.2.1", + "version": "0.2.2", "engines": { "node": ">=20.0.0" }, @@ -13,7 +13,7 @@ "url": "git+https://github.com/QwenLM/qwen-code.git" }, "config": { - "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.2.1" + "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.2.2" }, "scripts": { "start": "cross-env node scripts/start.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index 9924d7d1..6dce0ccc 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code", - "version": "0.2.1", + "version": "0.2.2", "description": "Qwen Code", "repository": { "type": "git", @@ -25,7 +25,7 @@ "dist" ], "config": { - "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.2.1" + "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.2.2" }, "dependencies": { "@google/genai": "1.16.0", diff --git a/packages/core/package.json b/packages/core/package.json index 5b70b7db..3232a664 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code-core", - "version": "0.2.1", + "version": "0.2.2", "description": "Qwen Code Core", "repository": { "type": "git", diff --git a/packages/test-utils/package.json b/packages/test-utils/package.json index 90b9cb27..512ada66 100644 --- a/packages/test-utils/package.json +++ b/packages/test-utils/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code-test-utils", - "version": "0.2.1", + "version": "0.2.2", "private": true, "main": "src/index.ts", "license": "Apache-2.0", diff --git a/packages/vscode-ide-companion/package.json b/packages/vscode-ide-companion/package.json index 10f8df20..dd86c816 100644 --- a/packages/vscode-ide-companion/package.json +++ b/packages/vscode-ide-companion/package.json @@ -2,7 +2,7 @@ "name": "qwen-code-vscode-ide-companion", "displayName": "Qwen Code Companion", "description": "Enable Qwen Code with direct access to your VS Code workspace.", - "version": "0.2.1", + "version": "0.2.2", "publisher": "qwenlm", "icon": "assets/icon.png", "repository": { From 29261c75e1ada308f5c1ae031747a1ea71e8eb01 Mon Sep 17 00:00:00 2001 From: BlockHand <49096326+BlockHand@users.noreply.github.com> Date: Fri, 14 Nov 2025 10:18:57 +0800 Subject: [PATCH 08/10] feat: openApi configurable window (#1019) --- packages/cli/src/ui/auth/AuthDialog.tsx | 36 ++++++++++++------- .../cli/src/ui/components/OpenAIKeyPrompt.tsx | 12 +++++-- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/ui/auth/AuthDialog.tsx b/packages/cli/src/ui/auth/AuthDialog.tsx index 4104a775..9d9baa89 100644 --- a/packages/cli/src/ui/auth/AuthDialog.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.tsx @@ -78,20 +78,17 @@ export function AuthDialog({ ); const handleAuthSelect = (authMethod: AuthType) => { - const error = validateAuthMethod(authMethod); - if (error) { - if ( - authMethod === AuthType.USE_OPENAI && - !process.env['OPENAI_API_KEY'] - ) { - setShowOpenAIKeyPrompt(true); - setErrorMessage(null); - } else { - setErrorMessage(error); - } - } else { + if (authMethod === AuthType.USE_OPENAI) { + setShowOpenAIKeyPrompt(true); setErrorMessage(null); - onSelect(authMethod, SettingScope.User); + } else { + const error = validateAuthMethod(authMethod); + if (error) { + setErrorMessage(error); + } else { + setErrorMessage(null); + onSelect(authMethod, SettingScope.User); + } } }; @@ -137,10 +134,23 @@ export function AuthDialog({ }, { isActive: true }, ); + const getDefaultOpenAIConfig = () => { + const fromSettings = settings.merged.security?.auth; + const modelSettings = settings.merged.model; + return { + apiKey: fromSettings?.apiKey || process.env['OPENAI_API_KEY'] || '', + baseUrl: fromSettings?.baseUrl || process.env['OPENAI_BASE_URL'] || '', + model: modelSettings?.name || process.env['OPENAI_MODEL'] || '', + }; + }; if (showOpenAIKeyPrompt) { + const defaults = getDefaultOpenAIConfig(); return ( diff --git a/packages/cli/src/ui/components/OpenAIKeyPrompt.tsx b/packages/cli/src/ui/components/OpenAIKeyPrompt.tsx index 365f557e..bc78b8c5 100644 --- a/packages/cli/src/ui/components/OpenAIKeyPrompt.tsx +++ b/packages/cli/src/ui/components/OpenAIKeyPrompt.tsx @@ -13,15 +13,21 @@ import { useKeypress } from '../hooks/useKeypress.js'; interface OpenAIKeyPromptProps { onSubmit: (apiKey: string, baseUrl: string, model: string) => void; onCancel: () => void; + defaultApiKey?: string; + defaultBaseUrl?: string; + defaultModel?: string; } export function OpenAIKeyPrompt({ onSubmit, onCancel, + defaultApiKey, + defaultBaseUrl, + defaultModel, }: OpenAIKeyPromptProps): React.JSX.Element { - const [apiKey, setApiKey] = useState(''); - const [baseUrl, setBaseUrl] = useState(''); - const [model, setModel] = useState(''); + const [apiKey, setApiKey] = useState(defaultApiKey || ''); + const [baseUrl, setBaseUrl] = useState(defaultBaseUrl || ''); + const [model, setModel] = useState(defaultModel || ''); const [currentField, setCurrentField] = useState< 'apiKey' | 'baseUrl' | 'model' >('apiKey'); From f0e21374c1985be60d780dd05521c513f8137674 Mon Sep 17 00:00:00 2001 From: hj C <76840587+seems20@users.noreply.github.com> Date: Fri, 14 Nov 2025 18:09:33 +0800 Subject: [PATCH 09/10] feat: add support for alternative cached_tokens format in OpenAI converter (#1035) Co-authored-by: chenhuanjie --- .../core/openaiContentGenerator/converter.ts | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/core/src/core/openaiContentGenerator/converter.ts b/packages/core/src/core/openaiContentGenerator/converter.ts index 7966f384..1edbdd6e 100644 --- a/packages/core/src/core/openaiContentGenerator/converter.ts +++ b/packages/core/src/core/openaiContentGenerator/converter.ts @@ -23,6 +23,14 @@ import type OpenAI from 'openai'; import { safeJsonParse } from '../../utils/safeJsonParse.js'; import { StreamingToolCallParser } from './streamingToolCallParser.js'; +/** + * Extended usage type that supports both OpenAI standard format and alternative formats + * Some models return cached_tokens at the top level instead of in prompt_tokens_details + */ +interface ExtendedCompletionUsage extends OpenAI.CompletionUsage { + cached_tokens?: number; +} + /** * Tool call accumulator for streaming responses */ @@ -582,7 +590,13 @@ export class OpenAIContentConverter { const promptTokens = usage.prompt_tokens || 0; const completionTokens = usage.completion_tokens || 0; const totalTokens = usage.total_tokens || 0; - const cachedTokens = usage.prompt_tokens_details?.cached_tokens || 0; + // Support both formats: prompt_tokens_details.cached_tokens (OpenAI standard) + // and cached_tokens (some models return it at top level) + const extendedUsage = usage as ExtendedCompletionUsage; + const cachedTokens = + usage.prompt_tokens_details?.cached_tokens ?? + extendedUsage.cached_tokens ?? + 0; // If we only have total tokens but no breakdown, estimate the split // Typically input is ~70% and output is ~30% for most conversations @@ -707,7 +721,13 @@ export class OpenAIContentConverter { const promptTokens = usage.prompt_tokens || 0; const completionTokens = usage.completion_tokens || 0; const totalTokens = usage.total_tokens || 0; - const cachedTokens = usage.prompt_tokens_details?.cached_tokens || 0; + // Support both formats: prompt_tokens_details.cached_tokens (OpenAI standard) + // and cached_tokens (some models return it at top level) + const extendedUsage = usage as ExtendedCompletionUsage; + const cachedTokens = + usage.prompt_tokens_details?.cached_tokens ?? + extendedUsage.cached_tokens ?? + 0; // If we only have total tokens but no breakdown, estimate the split // Typically input is ~70% and output is ~30% for most conversations From 0eeffc6875195023a5ccffb90dd08b65179cebed Mon Sep 17 00:00:00 2001 From: yyyanghj Date: Mon, 17 Nov 2025 10:58:33 +0800 Subject: [PATCH 10/10] feat: add support for Trae editor (#1037) --- .../cli/src/ui/commands/terminalSetupCommand.ts | 2 +- .../cli/src/ui/editors/editorSettingsManager.ts | 1 + packages/cli/src/ui/utils/terminalSetup.ts | 15 ++++++++++++++- packages/core/src/utils/editor.test.ts | 5 +++++ packages/core/src/utils/editor.ts | 10 ++++++++-- 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/ui/commands/terminalSetupCommand.ts b/packages/cli/src/ui/commands/terminalSetupCommand.ts index 09e5240c..31b473c7 100644 --- a/packages/cli/src/ui/commands/terminalSetupCommand.ts +++ b/packages/cli/src/ui/commands/terminalSetupCommand.ts @@ -17,7 +17,7 @@ import { terminalSetup } from '../utils/terminalSetup.js'; export const terminalSetupCommand: SlashCommand = { name: 'terminal-setup', description: - 'Configure terminal keybindings for multiline input (VS Code, Cursor, Windsurf)', + 'Configure terminal keybindings for multiline input (VS Code, Cursor, Windsurf, Trae)', kind: CommandKind.BUILT_IN, action: async (): Promise => { diff --git a/packages/cli/src/ui/editors/editorSettingsManager.ts b/packages/cli/src/ui/editors/editorSettingsManager.ts index 8f5c3710..612e4ff5 100644 --- a/packages/cli/src/ui/editors/editorSettingsManager.ts +++ b/packages/cli/src/ui/editors/editorSettingsManager.ts @@ -25,6 +25,7 @@ export const EDITOR_DISPLAY_NAMES: Record = { vscodium: 'VSCodium', windsurf: 'Windsurf', zed: 'Zed', + trae: 'Trae', }; class EditorSettingsManager { diff --git a/packages/cli/src/ui/utils/terminalSetup.ts b/packages/cli/src/ui/utils/terminalSetup.ts index a7b00d86..af5367f7 100644 --- a/packages/cli/src/ui/utils/terminalSetup.ts +++ b/packages/cli/src/ui/utils/terminalSetup.ts @@ -48,7 +48,7 @@ export interface TerminalSetupResult { requiresRestart?: boolean; } -type SupportedTerminal = 'vscode' | 'cursor' | 'windsurf'; +type SupportedTerminal = 'vscode' | 'cursor' | 'windsurf' | 'trae'; // Terminal detection async function detectTerminal(): Promise { @@ -68,6 +68,11 @@ async function detectTerminal(): Promise { ) { return 'windsurf'; } + + if (process.env['TERM_PRODUCT']?.toLowerCase().includes('trae')) { + return 'trae'; + } + // Check VS Code last since forks may also set VSCODE env vars if (termProgram === 'vscode' || process.env['VSCODE_GIT_IPC_HANDLE']) { return 'vscode'; @@ -86,6 +91,8 @@ async function detectTerminal(): Promise { return 'cursor'; if (parentName.includes('code') || parentName.includes('Code')) return 'vscode'; + if (parentName.includes('trae') || parentName.includes('Trae')) + return 'trae'; } catch (error) { // Continue detection even if process check fails console.debug('Parent process detection failed:', error); @@ -287,6 +294,10 @@ async function configureWindsurf(): Promise { return configureVSCodeStyle('Windsurf', 'Windsurf'); } +async function configureTrae(): Promise { + return configureVSCodeStyle('Trae', 'Trae'); +} + /** * Main terminal setup function that detects and configures the current terminal. * @@ -333,6 +344,8 @@ export async function terminalSetup(): Promise { return configureCursor(); case 'windsurf': return configureWindsurf(); + case 'trae': + return configureTrae(); default: return { success: false, diff --git a/packages/core/src/utils/editor.test.ts b/packages/core/src/utils/editor.test.ts index acc9e1a1..dd3202b8 100644 --- a/packages/core/src/utils/editor.test.ts +++ b/packages/core/src/utils/editor.test.ts @@ -72,6 +72,7 @@ describe('editor utils', () => { { editor: 'neovim', commands: ['nvim'], win32Commands: ['nvim'] }, { editor: 'zed', commands: ['zed', 'zeditor'], win32Commands: ['zed'] }, { editor: 'emacs', commands: ['emacs'], win32Commands: ['emacs.exe'] }, + { editor: 'trae', commands: ['trae'], win32Commands: ['trae'] }, ]; for (const { editor, commands, win32Commands } of testCases) { @@ -171,6 +172,7 @@ describe('editor utils', () => { }, { editor: 'cursor', commands: ['cursor'], win32Commands: ['cursor'] }, { editor: 'zed', commands: ['zed', 'zeditor'], win32Commands: ['zed'] }, + { editor: 'trae', commands: ['trae'], win32Commands: ['trae'] }, ]; for (const { editor, commands, win32Commands } of guiEditors) { @@ -321,6 +323,7 @@ describe('editor utils', () => { 'windsurf', 'cursor', 'zed', + 'trae', ]; for (const editor of guiEditors) { @@ -430,6 +433,7 @@ describe('editor utils', () => { 'windsurf', 'cursor', 'zed', + 'trae', ]; for (const editor of guiEditors) { it(`should not call onEditorClose for ${editor}`, async () => { @@ -481,6 +485,7 @@ describe('editor utils', () => { 'windsurf', 'cursor', 'zed', + 'trae', ]; for (const editor of guiEditors) { it(`should not allow ${editor} in sandbox mode`, () => { diff --git a/packages/core/src/utils/editor.ts b/packages/core/src/utils/editor.ts index 1023abe4..b6328925 100644 --- a/packages/core/src/utils/editor.ts +++ b/packages/core/src/utils/editor.ts @@ -14,7 +14,8 @@ export type EditorType = | 'vim' | 'neovim' | 'zed' - | 'emacs'; + | 'emacs' + | 'trae'; function isValidEditorType(editor: string): editor is EditorType { return [ @@ -26,6 +27,7 @@ function isValidEditorType(editor: string): editor is EditorType { 'neovim', 'zed', 'emacs', + 'trae', ].includes(editor); } @@ -62,6 +64,7 @@ const editorCommands: Record< neovim: { win32: ['nvim'], default: ['nvim'] }, zed: { win32: ['zed'], default: ['zed', 'zeditor'] }, emacs: { win32: ['emacs.exe'], default: ['emacs'] }, + trae: { win32: ['trae'], default: ['trae'] }, }; export function checkHasEditorType(editor: EditorType): boolean { @@ -73,7 +76,9 @@ export function checkHasEditorType(editor: EditorType): boolean { export function allowEditorTypeInSandbox(editor: EditorType): boolean { const notUsingSandbox = !process.env['SANDBOX']; - if (['vscode', 'vscodium', 'windsurf', 'cursor', 'zed'].includes(editor)) { + if ( + ['vscode', 'vscodium', 'windsurf', 'cursor', 'zed', 'trae'].includes(editor) + ) { return notUsingSandbox; } // For terminal-based editors like vim and emacs, allow in sandbox. @@ -115,6 +120,7 @@ export function getDiffCommand( case 'windsurf': case 'cursor': case 'zed': + case 'trae': return { command, args: ['--wait', '--diff', oldPath, newPath] }; case 'vim': case 'neovim':