diff --git a/.gitignore b/.gitignore index 493296158..01d4592b2 100644 --- a/.gitignore +++ b/.gitignore @@ -83,3 +83,6 @@ integration-tests/terminal-capture/scenarios/screenshots/ # storybook *storybook.log storybook-static + +# Dev symlink: qc-helper bundled skill docs (created by scripts/dev.js) +packages/core/src/skills/bundled/qc-helper/docs diff --git a/.qwen/skills/qwen-code-claw/SKILL.md b/.qwen/skills/qwen-code-claw/SKILL.md index f9a7b6a17..3a4b6e467 100644 --- a/.qwen/skills/qwen-code-claw/SKILL.md +++ b/.qwen/skills/qwen-code-claw/SKILL.md @@ -150,6 +150,59 @@ If every permission request is denied/cancelled and none are approved, `acpx` ex 4. Use `--format json` for automation and script integration 5. Use `--cwd` to manage context across multiple projects +## QwenCode Reference + +### CLI Commands + +| Command | Description | +| ----------- | ------------------------------- | +| `/help` | Show available commands | +| `/clear` | Clear conversation history | +| `/compress` | Compress history to save tokens | +| `/stats` | Show session info | +| `/auth` | Configure authentication | +| `/exit` | Exit Qwen Code | + +Full reference: https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/features/commands.md + +### Configuration + +Config files (highest priority first): CLI args > env vars > system > project (`.qwen/settings.json`) > user (`~/.qwen/settings.json`) > defaults. Format: JSONC with env var interpolation. + +Key settings: + +| Setting | Description | +| ---------------------------- | ----------------------------------------- | +| `model.name` | Model to use (e.g. `qwen-max`) | +| `tools.approvalMode` | `plan` / `default` / `auto_edit` / `yolo` | +| `permissions.allow/ask/deny` | Tool permission rules | +| `mcpServers.*` | MCP server configurations | + +Full reference: https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/configuration/settings.md + +### Authentication + +Supports Qwen OAuth (browser-based, 1000 free requests/day) and OpenAI-compatible API keys. + +Full reference: https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/configuration/auth.md + +### Model Providers + +Configure custom model providers via `modelProviders` in settings or environment variables (`OPENAI_API_KEY`, `OPENAI_BASE_URL`, `OPENAI_MODEL`). + +Full reference: https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/configuration/model-providers.md + +### Key Features + +| Feature | Description | Docs | +| ------------- | ----------------------------------------- | ------------------------------------------------------------------------------------------------------- | +| Approval Mode | Control tool execution permissions | https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/features/approval-mode.md | +| MCP | Model Context Protocol server integration | https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/features/mcp.md | +| Skills | Reusable skill system via `/skill` | https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/features/skills.md | +| Sub-agents | Delegate tasks to specialized agents | https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/features/sub-agents.md | +| Sandbox | Secure code execution environment | https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/features/sandbox.md | +| Headless | Non-interactive / CI mode | https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/docs/users/features/headless.md | + ## ACPX Reference ### Built-in Agent Registry diff --git a/README.md b/README.md index 7658b7ef0..5c0d83ab8 100644 --- a/README.md +++ b/README.md @@ -98,12 +98,22 @@ Your browser does not support the video tag. +## šŸ¦ž Use Qwen Code for Coding Tasks in Claw + +Copy the prompt below and paste it into your agent: + +``` +Use Qwen Code as a coding agent. First, fetch and read this skill reference: +https://raw.githubusercontent.com/QwenLM/qwen-code/refs/heads/main/.qwen/skills/qwen-code-claw/SKILL.md +Then follow the instructions inside to install, authenticate, and use Qwen Code via acpx for all coding tasks. +``` + ## Authentication Qwen Code supports two authentication methods: - **Qwen OAuth (recommended & free)**: sign in with your `qwen.ai` account in a browser. -- **API-KEY**: use an API key to connect to any supported provider (OpenAI, Anthropic, Google GenAI, Alibaba Cloud Bailian, and other compatible endpoints). +- **API-KEY**: use an API key to connect to any supported provider (OpenAI, Anthropic, Google GenAI, Alibaba Cloud ModelStudio, and other compatible endpoints). #### Qwen OAuth (recommended) @@ -121,7 +131,7 @@ Choose **Qwen OAuth** and complete the browser flow. Your credentials are cached Use this if you want more flexibility over which provider and model to use. Supports multiple protocols: -- **OpenAI-compatible**: Alibaba Cloud Bailian, ModelScope, OpenAI, OpenRouter, and other OpenAI-compatible providers +- **OpenAI-compatible**: Alibaba Cloud ModelStudio, ModelScope, OpenAI, OpenRouter, and other OpenAI-compatible providers - **Anthropic**: Claude models - **Google GenAI**: Gemini models @@ -183,7 +193,7 @@ Use the `/model` command at any time to switch between all configured models. ##### More Examples
-Coding Plan (Alibaba Cloud Bailian) — fixed monthly fee, higher quotas +Coding Plan (Alibaba Cloud ModelStudio) — fixed monthly fee, higher quotas ```json { @@ -193,7 +203,7 @@ Use the `/model` command at any time to switch between all configured models. "id": "qwen3.5-plus", "name": "qwen3.5-plus (Coding Plan)", "baseUrl": "https://coding.dashscope.aliyuncs.com/v1", - "description": "qwen3.5-plus with thinking enabled from Bailian Coding Plan", + "description": "qwen3.5-plus with thinking enabled from ModelStudio Coding Plan", "envKey": "BAILIAN_CODING_PLAN_API_KEY", "generationConfig": { "extra_body": { @@ -205,14 +215,14 @@ Use the `/model` command at any time to switch between all configured models. "id": "qwen3-coder-plus", "name": "qwen3-coder-plus (Coding Plan)", "baseUrl": "https://coding.dashscope.aliyuncs.com/v1", - "description": "qwen3-coder-plus from Bailian Coding Plan", + "description": "qwen3-coder-plus from ModelStudio Coding Plan", "envKey": "BAILIAN_CODING_PLAN_API_KEY" }, { "id": "qwen3-coder-next", "name": "qwen3-coder-next (Coding Plan)", "baseUrl": "https://coding.dashscope.aliyuncs.com/v1", - "description": "qwen3-coder-next with thinking enabled from Bailian Coding Plan", + "description": "qwen3-coder-next with thinking enabled from ModelStudio Coding Plan", "envKey": "BAILIAN_CODING_PLAN_API_KEY", "generationConfig": { "extra_body": { @@ -224,7 +234,7 @@ Use the `/model` command at any time to switch between all configured models. "id": "glm-4.7", "name": "glm-4.7 (Coding Plan)", "baseUrl": "https://coding.dashscope.aliyuncs.com/v1", - "description": "glm-4.7 with thinking enabled from Bailian Coding Plan", + "description": "glm-4.7 with thinking enabled from ModelStudio Coding Plan", "envKey": "BAILIAN_CODING_PLAN_API_KEY", "generationConfig": { "extra_body": { @@ -236,7 +246,7 @@ Use the `/model` command at any time to switch between all configured models. "id": "kimi-k2.5", "name": "kimi-k2.5 (Coding Plan)", "baseUrl": "https://coding.dashscope.aliyuncs.com/v1", - "description": "kimi-k2.5 with thinking enabled from Bailian Coding Plan", + "description": "kimi-k2.5 with thinking enabled from ModelStudio Coding Plan", "envKey": "BAILIAN_CODING_PLAN_API_KEY", "generationConfig": { "extra_body": { @@ -260,7 +270,7 @@ Use the `/model` command at any time to switch between all configured models. } ``` -> Subscribe to the Coding Plan and get your API key at [Alibaba Cloud Bailian](https://modelstudio.console.aliyun.com/?tab=dashboard#/efm/coding_plan). +> Subscribe to the Coding Plan and get your API key at [Alibaba Cloud ModelStudio(Beijing)](https://bailian.console.aliyun.com/cn-beijing?tab=coding-plan#/efm/coding-plan-index) or [Alibaba Cloud ModelStudio(intl)](https://modelstudio.console.alibabacloud.com/?tab=coding-plan#/efm/coding-plan-index).
diff --git a/docs/users/configuration/auth.md b/docs/users/configuration/auth.md index 445e42bc5..8f43729cb 100644 --- a/docs/users/configuration/auth.md +++ b/docs/users/configuration/auth.md @@ -37,16 +37,16 @@ qwen auth qwen-oauth Use this if you want predictable costs with diverse model options and higher usage quotas. - **How it works**: Subscribe to the Coding Plan with a fixed monthly fee, then configure Qwen Code to use the dedicated endpoint and your subscription API key. -- **Requirements**: Obtain an active Coding Plan subscription from [Aliyun Bailian](https://bailian.console.aliyun.com/?tab=model#/efm/coding_plan) or [Alibaba Cloud](https://bailian.console.alibabacloud.com/?tab=model#/efm/coding_plan), depending on the region of your account. +- **Requirements**: Obtain an active Coding Plan subscription from [Alibaba Cloud ModelStudio(Beijing)](https://bailian.console.aliyun.com/cn-beijing?tab=coding-plan#/efm/coding-plan-index) or [Alibaba Cloud ModelStudio(intl)](https://modelstudio.console.alibabacloud.com/?tab=coding-plan#/efm/coding-plan-index), depending on the region of your account. - **Benefits**: Diverse model options, higher usage quotas, predictable monthly costs, access to a wide range of models (Qwen, GLM, Kimi, Minimax and more). -- **Cost & quota**: View [Aliyun Bailian Coding Plan documentation](https://bailian.console.aliyun.com/cn-beijing/?tab=doc#/doc/?type=model&url=3005961). +- **Cost & quota**: View Aliyun ModelStudio Coding Plan documentation[Beijing](https://bailian.console.aliyun.com/cn-beijing/?tab=doc#/doc/?type=model&url=3005961)[intl](https://modelstudio.console.alibabacloud.com/?tab=doc#/doc/?type=model&url=2840914). Alibaba Cloud Coding Plan is available in two regions: -| Region | Console URL | -| -------------------------------- | ---------------------------------------------------------------------------- | -| Aliyun Bailian (aliyun.com) | [bailian.console.aliyun.com](https://bailian.console.aliyun.com) | -| Alibaba Cloud (alibabacloud.com) | [bailian.console.alibabacloud.com](https://bailian.console.alibabacloud.com) | +| Region | Console URL | +| ---------------------------- | ---------------------------------------------------------------------------- | +| Aliyun ModelStudio (Beijing) | [bailian.console.aliyun.com](https://bailian.console.aliyun.com) | +| Alibaba Cloud (intl) | [bailian.console.alibabacloud.com](https://bailian.console.alibabacloud.com) | ### Interactive setup diff --git a/docs/users/configuration/model-providers.md b/docs/users/configuration/model-providers.md index bcfc2cc75..83a66e8de 100644 --- a/docs/users/configuration/model-providers.md +++ b/docs/users/configuration/model-providers.md @@ -51,6 +51,10 @@ This auth type supports not only OpenAI's official API but also any OpenAI-compa ```json { + "env": { + "OPENAI_API_KEY": "sk-your-actual-openai-key-here", + "OPENROUTER_API_KEY": "sk-or-your-actual-openrouter-key-here" + }, "modelProviders": { "openai": [ { @@ -117,6 +121,9 @@ This auth type supports not only OpenAI's official API but also any OpenAI-compa ```json { + "env": { + "ANTHROPIC_API_KEY": "sk-ant-your-actual-anthropic-key-here" + }, "modelProviders": { "anthropic": [ { @@ -157,6 +164,9 @@ This auth type supports not only OpenAI's official API but also any OpenAI-compa ```json { + "env": { + "GEMINI_API_KEY": "AIza-your-actual-gemini-key-here" + }, "modelProviders": { "gemini": [ { @@ -191,6 +201,11 @@ Most local inference servers (vLLM, Ollama, LM Studio, etc.) provide an OpenAI-c ```json { + "env": { + "OLLAMA_API_KEY": "ollama", + "VLLM_API_KEY": "not-needed", + "LMSTUDIO_API_KEY": "lm-studio" + }, "modelProviders": { "openai": [ { @@ -255,6 +270,27 @@ export VLLM_API_KEY="not-needed" > > The `extra_body` parameter is **only supported for OpenAI-compatible providers** (`openai`, `qwen-oauth`). It is ignored for Anthropic, and Gemini providers. +> [!note] +> +> **About `envKey`**: The `envKey` field specifies the **name of an environment variable**, not the actual API key value. For the configuration to work, you need to ensure the corresponding environment variable is set with your real API key. There are two ways to do this: +> +> - **Option 1: Using a `.env` file** (recommended for security): +> ```bash +> # ~/.qwen/.env (or project root) +> OPENAI_API_KEY=sk-your-actual-key-here +> ``` +> Be sure to add `.env` to your `.gitignore` to prevent accidentally committing secrets. +> - **Option 2: Using the `env` field in `settings.json`** (as shown in the examples above): +> ```json +> { +> "env": { +> "OPENAI_API_KEY": "sk-your-actual-key-here" +> } +> } +> ``` +> +> Each provider example includes an `env` field to illustrate how the API key should be configured. + ## Alibaba Cloud Coding Plan Alibaba Cloud Coding Plan provides a pre-configured set of Qwen models optimized for coding tasks. This feature is available for users with Alibaba Cloud Coding Plan API access and offers a simplified setup experience with automatic model configuration updates. diff --git a/integration-tests/list_directory.test.ts b/integration-tests/list_directory.test.ts index 6d3cc37ad..a60945ba4 100644 --- a/integration-tests/list_directory.test.ts +++ b/integration-tests/list_directory.test.ts @@ -29,7 +29,7 @@ describe('list_directory', () => { 50, // check every 50ms ); - const prompt = `Can you list the files in the current directory.`; + const prompt = `Use the list_directory tool to list the files in the current directory.`; const result = await rig.run(prompt); diff --git a/integration-tests/sdk-typescript/tool-control.test.ts b/integration-tests/sdk-typescript/tool-control.test.ts index b0366a9b8..c4b48fc82 100644 --- a/integration-tests/sdk-typescript/tool-control.test.ts +++ b/integration-tests/sdk-typescript/tool-control.test.ts @@ -173,7 +173,9 @@ describe('Tool Control Parameters (E2E)', () => { if (writeFileResults.length > 0) { // Tool was called but should have permission declined message for (const result of writeFileResults) { - expect(result.content).toMatch(/permission.*declined/i); + expect(result.content).toMatch( + /permission.*(?:declined|denied)|denied.*permission/i, + ); } } @@ -221,14 +223,18 @@ describe('Tool Control Parameters (E2E)', () => { const listDirResults = findToolResults(messages, 'list_directory'); if (listDirResults.length > 0) { for (const result of listDirResults) { - expect(result.content).toMatch(/permission.*declined/i); + expect(result.content).toMatch( + /permission.*(?:declined|denied)|denied.*permission/i, + ); } } const shellResults = findToolResults(messages, 'run_shell_command'); if (shellResults.length > 0) { for (const result of shellResults) { - expect(result.content).toMatch(/permission.*declined/i); + expect(result.content).toMatch( + /permission.*(?:declined|denied)|denied.*permission/i, + ); } } } finally { @@ -263,7 +269,9 @@ describe('Tool Control Parameters (E2E)', () => { // All shell commands should have permission declined const shellResults = findToolResults(messages, 'run_shell_command'); for (const result of shellResults) { - expect(result.content).toMatch(/permission.*declined/i); + expect(result.content).toMatch( + /permission.*(?:declined|denied)|denied.*permission/i, + ); } } finally { await q.close(); @@ -303,7 +311,9 @@ describe('Tool Control Parameters (E2E)', () => { if (writeFileResults.length > 0) { // Tool was called but should have permission declined message (exclude takes priority) for (const result of writeFileResults) { - expect(result.content).toMatch(/permission.*declined/i); + expect(result.content).toMatch( + /permission.*(?:declined|denied)|denied.*permission/i, + ); } } @@ -360,7 +370,9 @@ describe('Tool Control Parameters (E2E)', () => { ); if (envReadResults.length > 0) { for (const result of envReadResults) { - expect(result.content).toMatch(/permission.*declined/i); + expect(result.content).toMatch( + /permission.*(?:declined|denied)|denied.*permission/i, + ); } } } finally { @@ -417,7 +429,9 @@ describe('Tool Control Parameters (E2E)', () => { ); if (srcEditResults.length > 0) { for (const result of srcEditResults) { - expect(result.content).toMatch(/permission.*declined/i); + expect(result.content).toMatch( + /permission.*(?:declined|denied)|denied.*permission/i, + ); } } @@ -469,7 +483,8 @@ describe('Tool Control Parameters (E2E)', () => { const rmResults = results.filter((r) => { return ( r.content.includes('permission') || - r.content.includes('declined') + r.content.includes('declined') || + r.content.includes('denied') ); }); expect(rmResults.length).toBeGreaterThan(0); @@ -1089,7 +1104,9 @@ describe('Tool Control Parameters (E2E)', () => { const shellResults = findToolResults(messages, 'run_shell_command'); if (shellResults.length > 0) { for (const result of shellResults) { - expect(result.content).toMatch(/permission.*declined/i); + expect(result.content).toMatch( + /permission.*(?:declined|denied)|denied.*permission/i, + ); } } } finally { diff --git a/package-lock.json b/package-lock.json index 57923129d..45a70b18b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@qwen-code/qwen-code", - "version": "0.13.0", + "version": "0.13.2", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@qwen-code/qwen-code", - "version": "0.13.0", + "version": "0.13.2", "workspaces": [ "packages/*", "packages/channels/base", @@ -19028,7 +19028,7 @@ }, "packages/cli": { "name": "@qwen-code/qwen-code", - "version": "0.13.0", + "version": "0.13.2", "dependencies": { "@agentclientprotocol/sdk": "^0.14.1", "@google/genai": "1.30.0", @@ -19689,7 +19689,7 @@ }, "packages/core": { "name": "@qwen-code/qwen-code-core", - "version": "0.13.0", + "version": "0.13.2", "hasInstallScript": true, "dependencies": { "@anthropic-ai/sdk": "^0.36.1", @@ -20320,7 +20320,7 @@ }, "packages/sdk-typescript": { "name": "@qwen-code/sdk", - "version": "0.1.4", + "version": "0.1.6", "license": "Apache-2.0", "dependencies": { "@modelcontextprotocol/sdk": "^1.25.1", @@ -23122,7 +23122,7 @@ }, "packages/test-utils": { "name": "@qwen-code/qwen-code-test-utils", - "version": "0.13.0", + "version": "0.13.2", "dev": true, "license": "Apache-2.0", "devDependencies": { @@ -23134,7 +23134,7 @@ }, "packages/vscode-ide-companion": { "name": "qwen-code-vscode-ide-companion", - "version": "0.13.0", + "version": "0.13.2", "license": "LICENSE", "dependencies": { "@agentclientprotocol/sdk": "^0.14.1", @@ -23382,7 +23382,7 @@ }, "packages/web-templates": { "name": "@qwen-code/web-templates", - "version": "0.13.0", + "version": "0.13.2", "devDependencies": { "@types/react": "^18.2.0", "@types/react-dom": "^18.2.0", @@ -23910,7 +23910,7 @@ }, "packages/webui": { "name": "@qwen-code/webui", - "version": "0.13.0", + "version": "0.13.2", "license": "MIT", "dependencies": { "markdown-it": "^14.1.0" diff --git a/package.json b/package.json index c5dc43258..e5c49cae2 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code", - "version": "0.13.0", + "version": "0.13.2", "engines": { "node": ">=20.0.0" }, @@ -18,7 +18,7 @@ "url": "git+https://github.com/QwenLM/qwen-code.git" }, "config": { - "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.13.0" + "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.13.2" }, "scripts": { "start": "cross-env node scripts/start.js", diff --git a/packages/cli/package.json b/packages/cli/package.json index bee927ccb..221d092bf 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code", - "version": "0.13.0", + "version": "0.13.2", "description": "Qwen Code", "repository": { "type": "git", @@ -33,7 +33,7 @@ "dist" ], "config": { - "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.13.0" + "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.13.2" }, "dependencies": { "@agentclientprotocol/sdk": "^0.14.1", diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index dbdab8de4..b78a8a0d6 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -57,7 +57,7 @@ import { buildAuthMethods } from './authMethods.js'; import { AcpFileSystemService } from './service/filesystem.js'; import { Readable, Writable } from 'node:stream'; import type { LoadedSettings } from '../config/settings.js'; -import { SettingScope } from '../config/settings.js'; +import { loadSettings, SettingScope } from '../config/settings.js'; import type { ApprovalModeValue } from './session/types.js'; import { z } from 'zod'; import type { CliArgs } from '../config/config.js'; @@ -223,30 +223,18 @@ class QwenAgent implements Agent { return sessionService.sessionExists(params.sessionId); }, ); - if (!exists) { - throw RequestError.invalidParams( - undefined, - `Session not found for id: ${params.sessionId}`, - ); - } const config = await this.newSessionConfig( params.cwd, params.mcpServers, params.sessionId, + exists, ); await this.ensureAuthenticated(config); this.setupFileSystem(config); const sessionData = config.getResumedSessionData(); - if (!sessionData) { - throw RequestError.internalError( - undefined, - `Failed to load session data for id: ${params.sessionId}`, - ); - } - - await this.createAndStoreSession(config, sessionData.conversation); + await this.createAndStoreSession(config, sessionData?.conversation); const modesData = this.buildModesData(config); const availableModels = this.buildAvailableModels(config); @@ -380,7 +368,9 @@ class QwenAgent implements Agent { cwd: string, mcpServers: McpServer[], sessionId?: string, + resume?: boolean, ): Promise { + this.settings = loadSettings(cwd); const mergedMcpServers = { ...this.settings.merged.mcpServers }; for (const server of mcpServers) { @@ -402,11 +392,11 @@ class QwenAgent implements Agent { const settings = { ...this.settings.merged, mcpServers: mergedMcpServers }; const argvForSession = { ...this.argv, - resume: sessionId, + ...(resume ? { resume: sessionId } : { sessionId }), continue: false, }; - const config = await loadCliConfig(settings, argvForSession, cwd); + const config = await loadCliConfig(settings, argvForSession, cwd, []); await config.initialize(); return config; } diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 9715d765c..4b5229321 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -34,6 +34,7 @@ describe('Session', () => { let currentAuthType: AuthType; let switchModelSpy: ReturnType; let getAvailableCommandsSpy: ReturnType; + let mockToolRegistry: { getTool: ReturnType }; beforeEach(() => { currentModel = 'qwen3-code-plus'; @@ -50,7 +51,7 @@ describe('Session', () => { addHistory: vi.fn(), } as unknown as GeminiChat; - const toolRegistry = { getTool: vi.fn() }; + mockToolRegistry = { getTool: vi.fn() }; const fileService = { shouldGitIgnoreFile: vi.fn().mockReturnValue(false) }; mockConfig = { @@ -65,8 +66,9 @@ describe('Session', () => { getChatRecordingService: vi.fn().mockReturnValue({ recordUserMessage: vi.fn(), recordUiTelemetryEvent: vi.fn(), + recordToolResult: vi.fn(), }), - getToolRegistry: vi.fn().mockReturnValue(toolRegistry), + getToolRegistry: vi.fn().mockReturnValue(mockToolRegistry), getFileService: vi.fn().mockReturnValue(fileService), getFileFilteringRespectGitIgnore: vi.fn().mockReturnValue(true), getEnableRecursiveFileSearch: vi.fn().mockReturnValue(false), @@ -275,5 +277,204 @@ describe('Session', () => { expect.any(Function), ); }); + + it('hides allow-always options when confirmation already forbids them', async () => { + const executeSpy = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const onConfirmSpy = vi.fn().mockResolvedValue(undefined); + const invocation = { + params: { path: '/tmp/file.txt' }, + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type: 'info', + title: 'Need permission', + prompt: 'Allow?', + hideAlwaysAllow: true, + onConfirm: onConfirmSpy, + }), + getDescription: vi.fn().mockReturnValue('Inspect file'), + toolLocations: vi.fn().mockReturnValue([]), + execute: executeSpy, + }; + const tool = { + name: 'read_file', + kind: core.Kind.Read, + build: vi.fn().mockReturnValue(invocation), + }; + + mockToolRegistry.getTool.mockReturnValue(tool); + mockConfig.getApprovalMode = vi + .fn() + .mockReturnValue(ApprovalMode.DEFAULT); + mockConfig.getPermissionManager = vi.fn().mockReturnValue(null); + mockChat.sendMessageStream = vi.fn().mockResolvedValue( + (async function* () { + yield { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-1', + name: 'read_file', + args: { path: '/tmp/file.txt' }, + }, + ], + }, + }; + })(), + ); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'run tool' }], + }); + + expect(mockClient.requestPermission).toHaveBeenCalledWith( + expect.objectContaining({ + options: [ + expect.objectContaining({ kind: 'allow_once' }), + expect.objectContaining({ kind: 'reject_once' }), + ], + }), + ); + const options = (mockClient.requestPermission as ReturnType) + .mock.calls[0][0].options as Array<{ kind: string }>; + expect(options.some((option) => option.kind === 'allow_always')).toBe( + false, + ); + }); + + it('returns permission error for disabled tools (L1 isToolEnabled check)', async () => { + const executeSpy = vi.fn(); + const invocation = { + params: { path: '/tmp/file.txt' }, + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type: 'info', + title: 'Need permission', + prompt: 'Allow?', + onConfirm: vi.fn(), + }), + getDescription: vi.fn().mockReturnValue('Write file'), + toolLocations: vi.fn().mockReturnValue([]), + execute: executeSpy, + }; + const tool = { + name: 'write_file', + kind: core.Kind.Edit, + build: vi.fn().mockReturnValue(invocation), + }; + + mockToolRegistry.getTool.mockReturnValue(tool); + mockConfig.getApprovalMode = vi + .fn() + .mockReturnValue(ApprovalMode.DEFAULT); + // Mock a PermissionManager that denies the tool + mockConfig.getPermissionManager = vi.fn().mockReturnValue({ + isToolEnabled: vi.fn().mockResolvedValue(false), + }); + mockChat.sendMessageStream = vi.fn().mockResolvedValue( + (async function* () { + yield { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-denied', + name: 'write_file', + args: { path: '/tmp/file.txt' }, + }, + ], + }, + }; + })(), + ); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'write something' }], + }); + + // Tool should NOT have been executed + expect(executeSpy).not.toHaveBeenCalled(); + // No permission dialog should have been opened + expect(mockClient.requestPermission).not.toHaveBeenCalled(); + }); + + it('respects permission-request hook allow decisions without opening ACP permission dialog', async () => { + const hookSpy = vi + .spyOn(core, 'firePermissionRequestHook') + .mockResolvedValue({ + hasDecision: true, + shouldAllow: true, + updatedInput: { path: '/tmp/updated.txt' }, + denyMessage: undefined, + }); + const executeSpy = vi.fn().mockResolvedValue({ + llmContent: 'ok', + returnDisplay: 'ok', + }); + const onConfirmSpy = vi.fn().mockResolvedValue(undefined); + const invocation = { + params: { path: '/tmp/original.txt' }, + getDefaultPermission: vi.fn().mockResolvedValue('ask'), + getConfirmationDetails: vi.fn().mockResolvedValue({ + type: 'info', + title: 'Need permission', + prompt: 'Allow?', + onConfirm: onConfirmSpy, + }), + getDescription: vi.fn().mockReturnValue('Inspect file'), + toolLocations: vi.fn().mockReturnValue([]), + execute: executeSpy, + }; + const tool = { + name: 'read_file', + kind: core.Kind.Read, + build: vi.fn().mockReturnValue(invocation), + }; + + mockToolRegistry.getTool.mockReturnValue(tool); + mockConfig.getApprovalMode = vi + .fn() + .mockReturnValue(ApprovalMode.DEFAULT); + mockConfig.getPermissionManager = vi.fn().mockReturnValue(null); + mockConfig.getEnableHooks = vi.fn().mockReturnValue(true); + mockConfig.getMessageBus = vi.fn().mockReturnValue({}); + mockChat.sendMessageStream = vi.fn().mockResolvedValue( + (async function* () { + yield { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'call-2', + name: 'read_file', + args: { path: '/tmp/original.txt' }, + }, + ], + }, + }; + })(), + ); + + try { + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'run tool' }], + }); + } finally { + hookSpy.mockRestore(); + } + + expect(mockClient.requestPermission).not.toHaveBeenCalled(); + expect(onConfirmSpy).toHaveBeenCalledWith( + core.ToolConfirmationOutcome.ProceedOnce, + ); + expect(invocation.params).toEqual({ path: '/tmp/updated.txt' }); + expect(executeSpy).toHaveBeenCalled(); + }); }); }); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index f9e47cdae..fd009dddf 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -36,6 +36,13 @@ import { readManyFiles, Storage, ToolNames, + buildPermissionCheckContext, + evaluatePermissionRules, + fireNotificationHook, + firePermissionRequestHook, + injectPermissionRulesIfMissing, + NotificationType, + persistPermissionOutcome, } from '@qwen-code/qwen-code-core'; import { RequestError } from '@agentclientprotocol/sdk'; @@ -43,7 +50,6 @@ import type { AvailableCommand, ContentBlock, EmbeddedResourceResource, - PermissionOption, PromptRequest, PromptResponse, RequestPermissionRequest, @@ -54,7 +60,6 @@ import type { SetSessionModeResponse, SetSessionModelRequest, SetSessionModelResponse, - ToolCallContent, AgentSideConnection, } from '@agentclientprotocol/sdk'; import type { LoadedSettings } from '../../config/settings.js'; @@ -79,6 +84,10 @@ import { ToolCallEmitter } from './emitters/ToolCallEmitter.js'; import { PlanEmitter } from './emitters/PlanEmitter.js'; import { MessageEmitter } from './emitters/MessageEmitter.js'; import { SubAgentTracker } from './SubAgentTracker.js'; +import { + buildPermissionRequestContent, + toPermissionOptions, +} from './permissionUtils.js'; const debugLogger = createDebugLogger('SESSION'); @@ -487,13 +496,34 @@ export class Session implements SessionContext { await this.sendUpdate(update); } + private async resolveIdeDiffForOutcome( + confirmationDetails: ToolCallConfirmationDetails, + outcome: ToolConfirmationOutcome, + ): Promise { + if ( + confirmationDetails.type !== 'edit' || + !confirmationDetails.ideConfirmation + ) { + return; + } + + const { IdeClient } = await import('@qwen-code/qwen-code-core'); + const ideClient = await IdeClient.getInstance(); + const cliOutcome = + outcome === ToolConfirmationOutcome.Cancel ? 'rejected' : 'accepted'; + await ideClient.resolveDiffFromCli( + confirmationDetails.filePath, + cliOutcome as 'accepted' | 'rejected', + ); + } + private async runTool( abortSignal: AbortSignal, promptId: string, fc: FunctionCall, ): Promise { const callId = fc.id ?? `${fc.name}-${Date.now()}`; - const args = (fc.args ?? {}) as Record; + let args = (fc.args ?? {}) as Record; const startTime = Date.now(); @@ -526,19 +556,49 @@ export class Session implements SessionContext { ]; }; + const earlyErrorResponse = async ( + error: Error, + toolName = fc.name ?? 'unknown_tool', + ) => { + if (toolName !== TodoWriteTool.Name) { + await this.toolCallEmitter.emitError(callId, toolName, error); + } + + const errorParts = errorResponse(error); + this.config.getChatRecordingService()?.recordToolResult(errorParts, { + callId, + status: 'error', + resultDisplay: undefined, + error, + errorType: undefined, + }); + return errorParts; + }; + if (!fc.name) { - return errorResponse(new Error('Missing function name')); + return earlyErrorResponse(new Error('Missing function name')); } const toolRegistry = this.config.getToolRegistry(); const tool = toolRegistry.getTool(fc.name as string); if (!tool) { - return errorResponse( + return earlyErrorResponse( new Error(`Tool "${fc.name}" not found in registry.`), ); } + // ---- L1: Tool enablement check ---- + const pm = this.config.getPermissionManager?.(); + if (pm && !(await pm.isToolEnabled(fc.name as string))) { + return earlyErrorResponse( + new Error( + `Qwen Code requires permission to use "${fc.name}", but that permission was declined.`, + ), + fc.name, + ); + } + // Detect TodoWriteTool early - route to plan updates instead of tool_call events const isTodoWriteTool = tool.name === TodoWriteTool.Name; const isAgentTool = tool.name === AgentTool.Name; @@ -577,127 +637,238 @@ export class Session implements SessionContext { ); } - // Use the new permission flow: getDefaultPermission + getConfirmationDetails - // ask_user_question must always go through confirmation even in YOLO mode - // so the user always has a chance to respond to questions. + // L3→L4→L5 Permission Flow (aligned with coreToolScheduler) + // + // L3: Tool's intrinsic default permission + // L4: PermissionManager rule override + // L5: ApprovalMode override (YOLO / AUTO_EDIT / PLAN) + // + // AUTO_EDIT auto-approval is handled HERE, same as coreToolScheduler. + // The VS Code extension is just a UI layer for requestPermission. const isAskUserQuestionTool = fc.name === ToolNames.ASK_USER_QUESTION; + + // ---- L3: Tool's default permission ---- + // In YOLO mode, force 'allow' for everything except ask_user_question. const defaultPermission = this.config.getApprovalMode() !== ApprovalMode.YOLO || isAskUserQuestionTool ? await invocation.getDefaultPermission() : 'allow'; - const needsConfirmation = defaultPermission === 'ask'; + // ---- L4: PermissionManager override (if relevant rules exist) ---- + const toolParams = invocation.params as Record; + const pmCtx = buildPermissionCheckContext( + fc.name, + toolParams, + this.config.getTargetDir?.() ?? '', + ); + const { finalPermission, pmForcedAsk } = await evaluatePermissionRules( + pm, + defaultPermission, + pmCtx, + ); - // Check for plan mode enforcement - block non-read-only tools - // but allow ask_user_question so users can answer clarification questions - const isPlanMode = this.config.getApprovalMode() === ApprovalMode.PLAN; + const needsConfirmation = finalPermission === 'ask'; + + // ---- L5: ApprovalMode overrides ---- + const approvalMode = this.config.getApprovalMode(); + const isPlanMode = approvalMode === ApprovalMode.PLAN; + + // PLAN mode: block non-read-only tools if ( isPlanMode && !isExitPlanModeTool && !isAskUserQuestionTool && needsConfirmation ) { - // In plan mode, block any tool that requires confirmation (write operations) - return errorResponse( + return earlyErrorResponse( new Error( `Plan mode is active. The tool "${fc.name}" cannot be executed because it modifies the system. ` + 'Please use the exit_plan_mode tool to present your plan and exit plan mode before making changes.', ), + fc.name, ); } - if (defaultPermission === 'deny') { - return errorResponse( + if (finalPermission === 'deny') { + return earlyErrorResponse( new Error( - `Tool "${fc.name}" is denied: command substitution is not allowed for security reasons.`, + defaultPermission === 'deny' + ? `Tool "${fc.name}" is denied: command substitution is not allowed for security reasons.` + : `Tool "${fc.name}" is denied by permission rules.`, ), + fc.name, ); } + let didRequestPermission = false; + if (needsConfirmation) { const confirmationDetails = await invocation.getConfirmationDetails(abortSignal); - const content: ToolCallContent[] = []; - if (confirmationDetails.type === 'edit') { - content.push({ - type: 'diff', - path: confirmationDetails.filePath || confirmationDetails.fileName, - oldText: confirmationDetails.originalContent, - newText: confirmationDetails.newContent, - }); - } + // Centralised rule injection (for display and persistence) + injectPermissionRulesIfMissing(confirmationDetails, pmCtx); - // Add plan content for exit_plan_mode - if (confirmationDetails.type === 'plan') { - content.push({ - type: 'content', - content: { - type: 'text', - text: confirmationDetails.plan, - }, - }); - } + const messageBus = this.config.getMessageBus?.(); + const hooksEnabled = this.config.getEnableHooks?.() ?? false; + let hookHandled = false; - // Map tool kind, using switch_mode for exit_plan_mode per ACP spec - const mappedKind = this.toolCallEmitter.mapToolKind(tool.kind, fc.name); + if (hooksEnabled && messageBus) { + const hookResult = await firePermissionRequestHook( + messageBus, + fc.name, + args, + String(approvalMode), + ); - const params: RequestPermissionRequest = { - sessionId: this.sessionId, - options: toPermissionOptions(confirmationDetails), - toolCall: { - toolCallId: callId, - status: 'pending', - title: invocation.getDescription(), - content, - locations: invocation.toolLocations(), - kind: mappedKind, - rawInput: args, - }, - }; + if (hookResult.hasDecision) { + hookHandled = true; + if (hookResult.shouldAllow) { + if (hookResult.updatedInput) { + args = hookResult.updatedInput; + invocation.params = + hookResult.updatedInput as typeof invocation.params; + } - const output = (await this.client.requestPermission( - params, - )) as RequestPermissionResponse & { - answers?: Record; - }; - const outcome = - output.outcome.outcome === 'cancelled' - ? ToolConfirmationOutcome.Cancel - : z - .nativeEnum(ToolConfirmationOutcome) - .parse(output.outcome.optionId); - - await confirmationDetails.onConfirm(outcome, { - answers: output.answers, - }); - - // After exit_plan_mode confirmation, send current_mode_update notification - if (isExitPlanModeTool && outcome !== ToolConfirmationOutcome.Cancel) { - await this.sendCurrentModeUpdateNotification(outcome); - } - - switch (outcome) { - case ToolConfirmationOutcome.Cancel: - return errorResponse( - new Error(`Tool "${fc.name}" was canceled by the user.`), - ); - case ToolConfirmationOutcome.ProceedOnce: - case ToolConfirmationOutcome.ProceedAlways: - case ToolConfirmationOutcome.ProceedAlwaysProject: - case ToolConfirmationOutcome.ProceedAlwaysUser: - case ToolConfirmationOutcome.ProceedAlwaysServer: - case ToolConfirmationOutcome.ProceedAlwaysTool: - case ToolConfirmationOutcome.ModifyWithEditor: - break; - default: { - const resultOutcome: never = outcome; - throw new Error(`Unexpected: ${resultOutcome}`); + await this.resolveIdeDiffForOutcome( + confirmationDetails, + ToolConfirmationOutcome.ProceedOnce, + ); + await confirmationDetails.onConfirm( + ToolConfirmationOutcome.ProceedOnce, + ); + } else { + return earlyErrorResponse( + new Error( + hookResult.denyMessage || + `Permission denied by hook for "${fc.name}"`, + ), + fc.name, + ); + } } } - } else if (!isTodoWriteTool) { - // Skip tool_call event for TodoWriteTool - use ToolCallEmitter + + // AUTO_EDIT mode: auto-approve edit and info tools + // (same as coreToolScheduler L5 — NOT delegated to the extension) + if ( + approvalMode === ApprovalMode.AUTO_EDIT && + (confirmationDetails.type === 'edit' || + confirmationDetails.type === 'info') + ) { + // Auto-approve, skip requestPermission. + // didRequestPermission stays false → emitStart below. + } else if (!hookHandled) { + // Show permission dialog via ACP requestPermission + didRequestPermission = true; + const content = buildPermissionRequestContent(confirmationDetails); + + // Map tool kind, using switch_mode for exit_plan_mode per ACP spec + const mappedKind = this.toolCallEmitter.mapToolKind( + tool.kind, + fc.name, + ); + + if (hooksEnabled && messageBus) { + void fireNotificationHook( + messageBus, + `Qwen Code needs your permission to use ${fc.name}`, + NotificationType.PermissionPrompt, + 'Permission needed', + ); + } + + const params: RequestPermissionRequest = { + sessionId: this.sessionId, + options: toPermissionOptions(confirmationDetails, pmForcedAsk), + toolCall: { + toolCallId: callId, + status: 'pending', + title: invocation.getDescription(), + content, + locations: invocation.toolLocations(), + kind: mappedKind, + rawInput: args, + }, + }; + + const output = (await this.client.requestPermission( + params, + )) as RequestPermissionResponse & { + answers?: Record; + }; + const outcome = + output.outcome.outcome === 'cancelled' + ? ToolConfirmationOutcome.Cancel + : z + .nativeEnum(ToolConfirmationOutcome) + .parse(output.outcome.optionId); + + await this.resolveIdeDiffForOutcome(confirmationDetails, outcome); + + await confirmationDetails.onConfirm(outcome, { + answers: output.answers, + }); + + // Persist permission rules when user explicitly chose "Always Allow". + // This branch is only reached for tools that went through + // requestPermission (user saw dialog and made a choice). + // AUTO_EDIT auto-approved tools never reach here. + if ( + outcome === ToolConfirmationOutcome.ProceedAlways || + outcome === ToolConfirmationOutcome.ProceedAlwaysProject || + outcome === ToolConfirmationOutcome.ProceedAlwaysUser + ) { + await persistPermissionOutcome( + outcome, + confirmationDetails, + this.config.getOnPersistPermissionRule?.(), + this.config.getPermissionManager?.(), + { answers: output.answers }, + ); + } + + // After exit_plan_mode confirmation, send current_mode_update + if ( + isExitPlanModeTool && + outcome !== ToolConfirmationOutcome.Cancel + ) { + await this.sendCurrentModeUpdateNotification(outcome); + } + + // After edit tool ProceedAlways, notify the client about mode change + if ( + confirmationDetails.type === 'edit' && + outcome === ToolConfirmationOutcome.ProceedAlways + ) { + await this.sendCurrentModeUpdateNotification(outcome); + } + + switch (outcome) { + case ToolConfirmationOutcome.Cancel: + return errorResponse( + new Error(`Tool "${fc.name}" was canceled by the user.`), + ); + case ToolConfirmationOutcome.ProceedOnce: + case ToolConfirmationOutcome.ProceedAlways: + case ToolConfirmationOutcome.ProceedAlwaysProject: + case ToolConfirmationOutcome.ProceedAlwaysUser: + case ToolConfirmationOutcome.ProceedAlwaysServer: + case ToolConfirmationOutcome.ProceedAlwaysTool: + case ToolConfirmationOutcome.ModifyWithEditor: + break; + default: { + const resultOutcome: never = outcome; + throw new Error(`Unexpected: ${resultOutcome}`); + } + } + } + } + + if (!didRequestPermission && !isTodoWriteTool) { + // Auto-approved (L3 allow / L4 PM allow / L5 YOLO|AUTO_EDIT) + // → emit tool_call start notification const startParams: ToolCallStartParams = { callId, toolName: fc.name, @@ -1041,113 +1212,3 @@ export class Session implements SessionContext { } } } - -// ============================================================================ -// Helper functions -// ============================================================================ - -const basicPermissionOptions = [ - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: 'Allow', - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: 'Reject', - kind: 'reject_once', - }, -] as const; - -function toPermissionOptions( - confirmation: ToolCallConfirmationDetails, -): PermissionOption[] { - switch (confirmation.type) { - case 'edit': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlways, - name: 'Allow All Edits', - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'exec': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project: ${confirmation.rootCommand}`, - kind: 'allow_always', - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user: ${confirmation.rootCommand}`, - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'mcp': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project: ${confirmation.toolName}`, - kind: 'allow_always', - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user: ${confirmation.toolName}`, - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'info': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project`, - kind: 'allow_always', - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user`, - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'plan': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlways, - name: `Yes, and auto-accept edits`, - kind: 'allow_always', - }, - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: `Yes, and manually approve edits`, - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: `No, keep planning (esc)`, - kind: 'reject_once', - }, - ]; - case 'ask_user_question': - return [ - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: 'Submit', - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: 'Cancel', - kind: 'reject_once', - }, - ]; - default: { - const unreachable: never = confirmation; - throw new Error(`Unexpected: ${unreachable}`); - } - } -} diff --git a/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts b/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts index 4c4025c82..573a9afee 100644 --- a/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts +++ b/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts @@ -488,6 +488,9 @@ describe('SubAgentTracker', () => { await vi.waitFor(() => { expect(respondSpy).toHaveBeenCalledWith( ToolConfirmationOutcome.ProceedOnce, + { + answers: undefined, + }, ); }); }); @@ -528,7 +531,58 @@ describe('SubAgentTracker', () => { eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); await vi.waitFor(() => { - expect(respondSpy).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); + expect(respondSpy).toHaveBeenCalledWith( + ToolConfirmationOutcome.Cancel, + { + answers: undefined, + }, + ); + }); + }); + + it('should forward answers payload from ACP permission responses', async () => { + requestPermissionSpy.mockResolvedValue({ + outcome: { + outcome: 'selected', + optionId: ToolConfirmationOutcome.ProceedOnce, + }, + answers: { + answer: 'yes', + }, + }); + tracker.setup(eventEmitter, abortController.signal); + + const respondSpy = vi.fn().mockResolvedValue(undefined); + const confirmationDetails = { + type: 'ask_user_question', + title: 'Question', + questions: [ + { + question: 'Continue?', + header: 'Question', + options: [], + multiSelect: false, + }, + ], + } as unknown as AgentApprovalRequestEvent['confirmationDetails']; + const event = createApprovalEvent({ + name: 'ask_user_question', + callId: 'call-ask', + confirmationDetails, + respond: respondSpy, + }); + + eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); + + await vi.waitFor(() => { + expect(respondSpy).toHaveBeenCalledWith( + ToolConfirmationOutcome.ProceedOnce, + { + answers: { + answer: 'yes', + }, + }, + ); }); }); diff --git a/packages/cli/src/acp-integration/session/SubAgentTracker.ts b/packages/cli/src/acp-integration/session/SubAgentTracker.ts index 7a904e9e6..133339fad 100644 --- a/packages/cli/src/acp-integration/session/SubAgentTracker.ts +++ b/packages/cli/src/acp-integration/session/SubAgentTracker.ts @@ -26,44 +26,15 @@ import { ToolCallEmitter } from './emitters/ToolCallEmitter.js'; import { MessageEmitter } from './emitters/MessageEmitter.js'; import type { AgentSideConnection, - PermissionOption, RequestPermissionRequest, - ToolCallContent, } from '@agentclientprotocol/sdk'; +import { + buildPermissionRequestContent, + toPermissionOptions, +} from './permissionUtils.js'; const debugLogger = createDebugLogger('ACP_SUBAGENT_TRACKER'); -/** - * Permission option kind type matching ACP schema. - */ -type PermissionKind = - | 'allow_once' - | 'reject_once' - | 'allow_always' - | 'reject_always'; - -/** - * Configuration for permission options displayed to users. - */ -interface PermissionOptionConfig { - optionId: ToolConfirmationOutcome; - name: string; - kind: PermissionKind; -} - -const basicPermissionOptions: readonly PermissionOptionConfig[] = [ - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: 'Allow', - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: 'Reject', - kind: 'reject_once', - }, -] as const; - /** * Tracks and emits events for sub-agent tool calls within AgentTool execution. * @@ -219,24 +190,6 @@ export class SubAgentTracker { if (abortSignal.aborted) return; const state = this.toolStates.get(event.callId); - const content: ToolCallContent[] = []; - - // Handle edit confirmation type - show diff - if (event.confirmationDetails.type === 'edit') { - const editDetails = event.confirmationDetails as unknown as { - type: 'edit'; - fileName: string; - filePath: string; - originalContent: string | null; - newContent: string; - }; - content.push({ - type: 'diff', - path: editDetails.filePath || editDetails.fileName, - oldText: editDetails.originalContent ?? '', - newText: editDetails.newContent, - }); - } // Build permission request const fullConfirmationDetails = { @@ -251,12 +204,12 @@ export class SubAgentTracker { const params: RequestPermissionRequest = { sessionId: this.ctx.sessionId, - options: this.toPermissionOptions(fullConfirmationDetails), + options: toPermissionOptions(fullConfirmationDetails), toolCall: { toolCallId: event.callId, status: 'pending', title, - content, + content: buildPermissionRequestContent(fullConfirmationDetails), locations, kind, rawInput: state?.args, @@ -274,7 +227,9 @@ export class SubAgentTracker { .parse(output.outcome.optionId); // Respond to subagent with the outcome - await event.respond(outcome); + await event.respond(outcome, { + answers: 'answers' in output ? output.answers : undefined, + }); } catch (error) { // If permission request fails, cancel the tool call debugLogger.error( @@ -324,92 +279,4 @@ export class SubAgentTracker { ); }; } - - /** - * Converts confirmation details to permission options for the client. - */ - private toPermissionOptions( - confirmation: ToolCallConfirmationDetails, - ): PermissionOption[] { - const hideAlwaysAllow = - 'hideAlwaysAllow' in confirmation && confirmation.hideAlwaysAllow; - switch (confirmation.type) { - case 'edit': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlways, - name: 'Allow All Edits', - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - case 'exec': - return [ - ...(hideAlwaysAllow - ? [] - : [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project: ${(confirmation as { rootCommand?: string }).rootCommand ?? 'command'}`, - kind: 'allow_always' as const, - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user: ${(confirmation as { rootCommand?: string }).rootCommand ?? 'command'}`, - kind: 'allow_always' as const, - }, - ]), - ...basicPermissionOptions, - ]; - case 'mcp': - return [ - ...(hideAlwaysAllow - ? [] - : [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: `Always Allow in project: ${(confirmation as { toolName?: string }).toolName ?? 'tool'}`, - kind: 'allow_always' as const, - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: `Always Allow for user: ${(confirmation as { toolName?: string }).toolName ?? 'tool'}`, - kind: 'allow_always' as const, - }, - ]), - ...basicPermissionOptions, - ]; - case 'info': - return [ - ...(hideAlwaysAllow - ? [] - : [ - { - optionId: ToolConfirmationOutcome.ProceedAlwaysProject, - name: 'Always Allow in project', - kind: 'allow_always' as const, - }, - { - optionId: ToolConfirmationOutcome.ProceedAlwaysUser, - name: 'Always Allow for user', - kind: 'allow_always' as const, - }, - ]), - ...basicPermissionOptions, - ]; - case 'plan': - return [ - { - optionId: ToolConfirmationOutcome.ProceedAlways, - name: 'Always Allow Plans', - kind: 'allow_always', - }, - ...basicPermissionOptions, - ]; - default: { - // Fallback for unknown types - return [...basicPermissionOptions]; - } - } - } } diff --git a/packages/cli/src/acp-integration/session/permissionUtils.test.ts b/packages/cli/src/acp-integration/session/permissionUtils.test.ts new file mode 100644 index 000000000..743049f2e --- /dev/null +++ b/packages/cli/src/acp-integration/session/permissionUtils.test.ts @@ -0,0 +1,54 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { ToolConfirmationOutcome } from '@qwen-code/qwen-code-core'; +import { toPermissionOptions } from './permissionUtils.js'; + +describe('permissionUtils', () => { + describe('toPermissionOptions', () => { + it('uses permissionRules for exec always-allow labels when available', () => { + const options = toPermissionOptions({ + type: 'exec', + title: 'Confirm Shell Command', + command: 'git add package.json', + rootCommand: 'git', + permissionRules: ['Bash(git add *)'], + onConfirm: async () => undefined, + }); + + expect(options).toContainEqual( + expect.objectContaining({ + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: 'Always Allow in project: git add *', + }), + ); + expect(options).toContainEqual( + expect.objectContaining({ + optionId: ToolConfirmationOutcome.ProceedAlwaysUser, + name: 'Always Allow for user: git add *', + }), + ); + }); + + it('falls back to rootCommand when exec permissionRules are unavailable', () => { + const options = toPermissionOptions({ + type: 'exec', + title: 'Confirm Shell Command', + command: 'git add package.json', + rootCommand: 'git', + onConfirm: async () => undefined, + }); + + expect(options).toContainEqual( + expect.objectContaining({ + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: 'Always Allow in project: git', + }), + ); + }); + }); +}); diff --git a/packages/cli/src/acp-integration/session/permissionUtils.ts b/packages/cli/src/acp-integration/session/permissionUtils.ts new file mode 100644 index 000000000..06434b4a0 --- /dev/null +++ b/packages/cli/src/acp-integration/session/permissionUtils.ts @@ -0,0 +1,208 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ToolCallConfirmationDetails } from '@qwen-code/qwen-code-core'; +import { ToolConfirmationOutcome } from '@qwen-code/qwen-code-core'; +import type { + PermissionOption, + ToolCallContent, +} from '@agentclientprotocol/sdk'; + +const basicPermissionOptions = [ + { + optionId: ToolConfirmationOutcome.ProceedOnce, + name: 'Allow', + kind: 'allow_once', + }, + { + optionId: ToolConfirmationOutcome.Cancel, + name: 'Reject', + kind: 'reject_once', + }, +] as const satisfies readonly PermissionOption[]; + +function supportsHideAlwaysAllow( + confirmation: ToolCallConfirmationDetails, +): confirmation is Exclude< + ToolCallConfirmationDetails, + { type: 'ask_user_question' } +> { + return confirmation.type !== 'ask_user_question'; +} + +function filterAlwaysAllowOptions( + confirmation: ToolCallConfirmationDetails, + options: PermissionOption[], + forceHideAlwaysAllow = false, +): PermissionOption[] { + const hideAlwaysAllow = + forceHideAlwaysAllow || + (supportsHideAlwaysAllow(confirmation) && + confirmation.hideAlwaysAllow === true); + return hideAlwaysAllow + ? options.filter((option) => option.kind !== 'allow_always') + : options; +} + +function formatExecPermissionScopeLabel( + confirmation: Extract, +): string { + const permissionRules = confirmation.permissionRules ?? []; + const bashRules = permissionRules + .map((rule) => { + const match = /^Bash\((.*)\)$/.exec(rule.trim()); + return match?.[1]?.trim() || undefined; + }) + .filter((rule): rule is string => Boolean(rule)); + + const uniqueRules = [...new Set(bashRules)]; + if (uniqueRules.length === 1) { + return uniqueRules[0]; + } + if (uniqueRules.length > 1) { + return uniqueRules.join(', '); + } + return confirmation.rootCommand; +} + +export function buildPermissionRequestContent( + confirmation: ToolCallConfirmationDetails, +): ToolCallContent[] { + const content: ToolCallContent[] = []; + + if (confirmation.type === 'edit') { + content.push({ + type: 'diff', + path: confirmation.filePath ?? confirmation.fileName, + oldText: confirmation.originalContent ?? '', + newText: confirmation.newContent, + }); + } + + if (confirmation.type === 'plan') { + content.push({ + type: 'content', + content: { + type: 'text', + text: confirmation.plan, + }, + }); + } + + return content; +} + +export function toPermissionOptions( + confirmation: ToolCallConfirmationDetails, + forceHideAlwaysAllow = false, +): PermissionOption[] { + switch (confirmation.type) { + case 'edit': + return filterAlwaysAllowOptions( + confirmation, + [ + { + optionId: ToolConfirmationOutcome.ProceedAlways, + name: 'Allow All Edits', + kind: 'allow_always', + }, + ...basicPermissionOptions, + ], + forceHideAlwaysAllow, + ); + case 'exec': { + const label = formatExecPermissionScopeLabel(confirmation); + return filterAlwaysAllowOptions( + confirmation, + [ + { + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: `Always Allow in project: ${label}`, + kind: 'allow_always', + }, + { + optionId: ToolConfirmationOutcome.ProceedAlwaysUser, + name: `Always Allow for user: ${label}`, + kind: 'allow_always', + }, + ...basicPermissionOptions, + ], + forceHideAlwaysAllow, + ); + } + case 'mcp': + return filterAlwaysAllowOptions( + confirmation, + [ + { + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: `Always Allow in project: ${confirmation.toolName}`, + kind: 'allow_always', + }, + { + optionId: ToolConfirmationOutcome.ProceedAlwaysUser, + name: `Always Allow for user: ${confirmation.toolName}`, + kind: 'allow_always', + }, + ...basicPermissionOptions, + ], + forceHideAlwaysAllow, + ); + case 'info': + return filterAlwaysAllowOptions( + confirmation, + [ + { + optionId: ToolConfirmationOutcome.ProceedAlwaysProject, + name: 'Always Allow in project', + kind: 'allow_always', + }, + { + optionId: ToolConfirmationOutcome.ProceedAlwaysUser, + name: 'Always Allow for user', + kind: 'allow_always', + }, + ...basicPermissionOptions, + ], + forceHideAlwaysAllow, + ); + case 'plan': + return [ + { + optionId: ToolConfirmationOutcome.ProceedAlways, + name: 'Yes, and auto-accept edits', + kind: 'allow_always', + }, + { + optionId: ToolConfirmationOutcome.ProceedOnce, + name: 'Yes, and manually approve edits', + kind: 'allow_once', + }, + { + optionId: ToolConfirmationOutcome.Cancel, + name: 'No, keep planning (esc)', + kind: 'reject_once', + }, + ]; + case 'ask_user_question': + return [ + { + optionId: ToolConfirmationOutcome.ProceedOnce, + name: 'Submit', + kind: 'allow_once', + }, + { + optionId: ToolConfirmationOutcome.Cancel, + name: 'Cancel', + kind: 'reject_once', + }, + ]; + default: { + const unreachable: never = confirmation; + throw new Error(`Unexpected: ${unreachable}`); + } + } +} diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 5b5436006..32131e47a 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -33,8 +33,8 @@ import { } from '@qwen-code/qwen-code-core'; import { extensionsCommand } from '../commands/extensions.js'; import { hooksCommand } from '../commands/hooks.js'; -import type { Settings, LoadedSettings } from './settings.js'; -import { SettingScope } from './settings.js'; +import type { Settings } from './settings.js'; +import { loadSettings, SettingScope } from './settings.js'; import { authCommand } from '../commands/auth.js'; import { resolveCliGenerationConfig, @@ -708,7 +708,6 @@ export async function loadCliConfig( argv: CliArgs, cwd: string = process.cwd(), overrideExtensions?: string[], - loadedSettings?: LoadedSettings, ): Promise { const debugMode = isDebugMode(argv); @@ -1046,20 +1045,19 @@ export async function loadCliConfig( deny: mergedDeny.length > 0 ? mergedDeny : undefined, }, // Permission rule persistence callback (writes to settings files). - onPersistPermissionRule: loadedSettings - ? async (scope, ruleType, rule) => { - const settingScope = - scope === 'project' ? SettingScope.Workspace : SettingScope.User; - const key = `permissions.${ruleType}`; - const currentRules: string[] = - loadedSettings.forScope(settingScope).settings.permissions?.[ - ruleType - ] ?? []; - if (!currentRules.includes(rule)) { - loadedSettings.setValue(settingScope, key, [...currentRules, rule]); - } - } - : undefined, + onPersistPermissionRule: async (scope, ruleType, rule) => { + const currentSettings = loadSettings(cwd); + const settingScope = + scope === 'project' ? SettingScope.Workspace : SettingScope.User; + const key = `permissions.${ruleType}`; + const currentRules: string[] = + currentSettings.forScope(settingScope).settings.permissions?.[ + ruleType + ] ?? []; + if (!currentRules.includes(rule)) { + currentSettings.setValue(settingScope, key, [...currentRules, rule]); + } + }, toolDiscoveryCommand: settings.tools?.discoveryCommand, toolCallCommand: settings.tools?.callCommand, mcpServerCommand: settings.mcp?.serverCommand, diff --git a/packages/cli/src/constants/alibabaStandardApiKey.ts b/packages/cli/src/constants/alibabaStandardApiKey.ts new file mode 100644 index 000000000..cb1c6170c --- /dev/null +++ b/packages/cli/src/constants/alibabaStandardApiKey.ts @@ -0,0 +1,24 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +export type AlibabaStandardRegion = + | 'cn-beijing' + | 'sg-singapore' + | 'us-virginia' + | 'cn-hongkong'; + +export const DASHSCOPE_STANDARD_API_KEY_ENV_KEY = 'DASHSCOPE_API_KEY'; + +export const ALIBABA_STANDARD_API_KEY_ENDPOINTS: Record< + AlibabaStandardRegion, + string +> = { + 'cn-beijing': 'https://dashscope.aliyuncs.com/compatible-mode/v1', + 'sg-singapore': 'https://dashscope-intl.aliyuncs.com/compatible-mode/v1', + 'us-virginia': 'https://dashscope-us.aliyuncs.com/compatible-mode/v1', + 'cn-hongkong': + 'https://cn-hongkong.dashscope.aliyuncs.com/compatible-mode/v1', +}; diff --git a/packages/cli/src/constants/codingPlan.ts b/packages/cli/src/constants/codingPlan.ts index 87be46542..8e7621861 100644 --- a/packages/cli/src/constants/codingPlan.ts +++ b/packages/cli/src/constants/codingPlan.ts @@ -54,7 +54,7 @@ export function generateCodingPlanTemplate( return [ { id: 'qwen3.5-plus', - name: '[Bailian Coding Plan] qwen3.5-plus', + name: '[ModelStudio Coding Plan] qwen3.5-plus', baseUrl: 'https://coding.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -66,7 +66,7 @@ export function generateCodingPlanTemplate( }, { id: 'glm-5', - name: '[Bailian Coding Plan] glm-5', + name: '[ModelStudio Coding Plan] glm-5', baseUrl: 'https://coding.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -78,7 +78,7 @@ export function generateCodingPlanTemplate( }, { id: 'kimi-k2.5', - name: '[Bailian Coding Plan] kimi-k2.5', + name: '[ModelStudio Coding Plan] kimi-k2.5', baseUrl: 'https://coding.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -90,7 +90,7 @@ export function generateCodingPlanTemplate( }, { id: 'MiniMax-M2.5', - name: '[Bailian Coding Plan] MiniMax-M2.5', + name: '[ModelStudio Coding Plan] MiniMax-M2.5', baseUrl: 'https://coding.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -102,7 +102,7 @@ export function generateCodingPlanTemplate( }, { id: 'qwen3-coder-plus', - name: '[Bailian Coding Plan] qwen3-coder-plus', + name: '[ModelStudio Coding Plan] qwen3-coder-plus', baseUrl: 'https://coding.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -111,7 +111,7 @@ export function generateCodingPlanTemplate( }, { id: 'qwen3-coder-next', - name: '[Bailian Coding Plan] qwen3-coder-next', + name: '[ModelStudio Coding Plan] qwen3-coder-next', baseUrl: 'https://coding.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -120,7 +120,7 @@ export function generateCodingPlanTemplate( }, { id: 'qwen3-max-2026-01-23', - name: '[Bailian Coding Plan] qwen3-max-2026-01-23', + name: '[ModelStudio Coding Plan] qwen3-max-2026-01-23', baseUrl: 'https://coding.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -132,7 +132,7 @@ export function generateCodingPlanTemplate( }, { id: 'glm-4.7', - name: '[Bailian Coding Plan] glm-4.7', + name: '[ModelStudio Coding Plan] glm-4.7', baseUrl: 'https://coding.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -145,11 +145,11 @@ export function generateCodingPlanTemplate( ]; } - // Global region uses Bailian Coding Plan branding for Global/Intl + // Global region uses ModelStudio Coding Plan branding for Global/Intl return [ { id: 'qwen3.5-plus', - name: '[Bailian Coding Plan for Global/Intl] qwen3.5-plus', + name: '[ModelStudio Coding Plan for Global/Intl] qwen3.5-plus', baseUrl: 'https://coding-intl.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -161,7 +161,7 @@ export function generateCodingPlanTemplate( }, { id: 'qwen3-coder-plus', - name: '[Bailian Coding Plan for Global/Intl] qwen3-coder-plus', + name: '[ModelStudio Coding Plan for Global/Intl] qwen3-coder-plus', baseUrl: 'https://coding-intl.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -170,7 +170,7 @@ export function generateCodingPlanTemplate( }, { id: 'qwen3-coder-next', - name: '[Bailian Coding Plan for Global/Intl] qwen3-coder-next', + name: '[ModelStudio Coding Plan for Global/Intl] qwen3-coder-next', baseUrl: 'https://coding-intl.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -179,7 +179,7 @@ export function generateCodingPlanTemplate( }, { id: 'qwen3-max-2026-01-23', - name: '[Bailian Coding Plan for Global/Intl] qwen3-max-2026-01-23', + name: '[ModelStudio Coding Plan for Global/Intl] qwen3-max-2026-01-23', baseUrl: 'https://coding-intl.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -191,7 +191,7 @@ export function generateCodingPlanTemplate( }, { id: 'glm-4.7', - name: '[Bailian Coding Plan for Global/Intl] glm-4.7', + name: '[ModelStudio Coding Plan for Global/Intl] glm-4.7', baseUrl: 'https://coding-intl.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -203,7 +203,7 @@ export function generateCodingPlanTemplate( }, { id: 'glm-5', - name: '[Bailian Coding Plan for Global/Intl] glm-5', + name: '[ModelStudio Coding Plan for Global/Intl] glm-5', baseUrl: 'https://coding-intl.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -215,7 +215,7 @@ export function generateCodingPlanTemplate( }, { id: 'MiniMax-M2.5', - name: '[Bailian Coding Plan for Global/Intl] MiniMax-M2.5', + name: '[ModelStudio Coding Plan for Global/Intl] MiniMax-M2.5', baseUrl: 'https://coding-intl.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { @@ -227,7 +227,7 @@ export function generateCodingPlanTemplate( }, { id: 'kimi-k2.5', - name: '[Bailian Coding Plan for Global/Intl] kimi-k2.5', + name: '[ModelStudio Coding Plan for Global/Intl] kimi-k2.5', baseUrl: 'https://coding-intl.dashscope.aliyuncs.com/v1', envKey: CODING_PLAN_ENV_KEY, generationConfig: { diff --git a/packages/cli/src/gemini.tsx b/packages/cli/src/gemini.tsx index 3d0e180db..a302e6caf 100644 --- a/packages/cli/src/gemini.tsx +++ b/packages/cli/src/gemini.tsx @@ -360,7 +360,6 @@ export async function main() { argv, process.cwd(), argv.extensions, - settings, ); // Register cleanup for MCP clients as early as possible diff --git a/packages/cli/src/i18n/locales/de.js b/packages/cli/src/i18n/locales/de.js index 42070c5a1..cb3229a2b 100644 --- a/packages/cli/src/i18n/locales/de.js +++ b/packages/cli/src/i18n/locales/de.js @@ -1784,8 +1784,8 @@ export default { // Auth Dialog - View Titles and Labels // ============================================================================ 'Coding Plan': 'Coding Plan', - "Paste your api key of Bailian Coding Plan and you're all set!": - 'Fügen Sie Ihren Bailian Coding Plan API-Schlüssel ein und Sie sind bereit!', + "Paste your api key of ModelStudio Coding Plan and you're all set!": + 'Fügen Sie Ihren ModelStudio Coding Plan API-Schlüssel ein und Sie sind bereit!', Custom: 'Benutzerdefiniert', 'More instructions about configuring `modelProviders` manually.': 'Weitere Anweisungen zur manuellen Konfiguration von `modelProviders`.', diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index 8024ebb80..3178ea533 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -1833,8 +1833,8 @@ export default { // Auth Dialog - View Titles and Labels // ============================================================================ 'Coding Plan': 'Coding Plan', - "Paste your api key of Bailian Coding Plan and you're all set!": - "Paste your api key of Bailian Coding Plan and you're all set!", + "Paste your api key of ModelStudio Coding Plan and you're all set!": + "Paste your api key of ModelStudio Coding Plan and you're all set!", Custom: 'Custom', 'More instructions about configuring `modelProviders` manually.': 'More instructions about configuring `modelProviders` manually.', diff --git a/packages/cli/src/i18n/locales/ja.js b/packages/cli/src/i18n/locales/ja.js index 7f5e6dbb9..ac5f59111 100644 --- a/packages/cli/src/i18n/locales/ja.js +++ b/packages/cli/src/i18n/locales/ja.js @@ -1285,8 +1285,8 @@ export default { // Auth Dialog - View Titles and Labels // ============================================================================ 'Coding Plan': 'Coding Plan', - "Paste your api key of Bailian Coding Plan and you're all set!": - 'Bailian Coding Plan恮APIć‚­ćƒ¼ć‚’č²¼ć‚Šä»˜ć‘ć‚‹ć ć‘ć§ęŗ–å‚™å®Œäŗ†ć§ć™ļ¼', + "Paste your api key of ModelStudio Coding Plan and you're all set!": + 'ModelStudio Coding Plan恮APIć‚­ćƒ¼ć‚’č²¼ć‚Šä»˜ć‘ć‚‹ć ć‘ć§ęŗ–å‚™å®Œäŗ†ć§ć™ļ¼', Custom: 'ć‚«ć‚¹ć‚æćƒ ', 'More instructions about configuring `modelProviders` manually.': '`modelProviders`ć‚’ę‰‹å‹•ć§čØ­å®šć™ć‚‹ę–¹ę³•ć®č©³ē“°ćÆć“ć”ć‚‰ć€‚', diff --git a/packages/cli/src/i18n/locales/pt.js b/packages/cli/src/i18n/locales/pt.js index bafabb6f6..993cd8d8c 100644 --- a/packages/cli/src/i18n/locales/pt.js +++ b/packages/cli/src/i18n/locales/pt.js @@ -1777,8 +1777,8 @@ export default { // Auth Dialog - View Titles and Labels // ============================================================================ 'Coding Plan': 'Coding Plan', - "Paste your api key of Bailian Coding Plan and you're all set!": - 'Cole sua chave de API do Bailian Coding Plan e pronto!', + "Paste your api key of ModelStudio Coding Plan and you're all set!": + 'Cole sua chave de API do ModelStudio Coding Plan e pronto!', Custom: 'Personalizado', 'More instructions about configuring `modelProviders` manually.': 'Mais instruƧƵes sobre como configurar `modelProviders` manualmente.', diff --git a/packages/cli/src/i18n/locales/ru.js b/packages/cli/src/i18n/locales/ru.js index 51eadf956..bb7e8968f 100644 --- a/packages/cli/src/i18n/locales/ru.js +++ b/packages/cli/src/i18n/locales/ru.js @@ -1711,8 +1711,8 @@ export default { // Auth Dialog - View Titles and Labels // ============================================================================ 'Coding Plan': 'Coding Plan', - "Paste your api key of Bailian Coding Plan and you're all set!": - 'Š’ŃŃ‚Š°Š²ŃŒŃ‚Šµ ваш API-ŠŗŠ»ŃŽŃ‡ Bailian Coding Plan Šø всё готово!', + "Paste your api key of ModelStudio Coding Plan and you're all set!": + 'Š’ŃŃ‚Š°Š²ŃŒŃ‚Šµ ваш API-ŠŗŠ»ŃŽŃ‡ ModelStudio Coding Plan Šø всё готово!', Custom: 'ŠŸŠ¾Š»ŃŒŠ·Š¾Š²Š°Ń‚ŠµŠ»ŃŒŃŠŗŠøŠ¹', 'More instructions about configuring `modelProviders` manually.': 'Š”Š¾ŠæŠ¾Š»Š½ŠøŃ‚ŠµŠ»ŃŒŠ½Ń‹Šµ ŠøŠ½ŃŃ‚Ń€ŃƒŠŗŃ†ŠøŠø по Ń€ŃƒŃ‡Š½Š¾Š¹ настройке `modelProviders`.', diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index c646fc044..ad755b721 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -1650,7 +1650,7 @@ export default { // ============================================================================ 'API-KEY': 'API-KEY', 'Coding Plan': 'Coding Plan', - "Paste your api key of Bailian Coding Plan and you're all set!": + "Paste your api key of ModelStudio Coding Plan and you're all set!": 'ē²˜č““ę‚Øēš„ē™¾ē‚¼ Coding Plan API Keyļ¼Œå³åÆå®Œęˆč®¾ē½®ļ¼', Custom: 'č‡Ŗå®šä¹‰', 'More instructions about configuring `modelProviders` manually.': diff --git a/packages/cli/src/services/prompt-processors/shellProcessor.ts b/packages/cli/src/services/prompt-processors/shellProcessor.ts index f499c2713..679e1d0c6 100644 --- a/packages/cli/src/services/prompt-processors/shellProcessor.ts +++ b/packages/cli/src/services/prompt-processors/shellProcessor.ts @@ -133,12 +133,12 @@ export class ShellProcessor implements IPromptProcessor { // Security check on the final, escaped command string. const { allAllowed, disallowedCommands, blockReason, isHardDenial } = - checkCommandPermissions(command, config, sessionShellAllowlist); + await checkCommandPermissions(command, config, sessionShellAllowlist); // Determine if this command is explicitly auto-approved via PermissionManager const pm = config.getPermissionManager?.(); const isAllowedBySettings = pm - ? pm.isCommandAllowed(command) === 'allow' + ? (await pm.isCommandAllowed(command)) === 'allow' : false; if (!allAllowed) { diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index 4e8091378..07397989a 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -190,6 +190,8 @@ describe('AppContainer State Management', () => { isAuthDialogOpen: false, isAuthenticating: false, handleAuthSelect: vi.fn(), + handleCodingPlanSubmit: vi.fn(), + handleAlibabaStandardSubmit: vi.fn(), openAuthDialog: vi.fn(), cancelAuthentication: vi.fn(), }); diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index b32bd89ad..37dc32518 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -457,6 +457,7 @@ export const AppContainer = (props: AppContainerProps) => { qwenAuthState, handleAuthSelect, handleCodingPlanSubmit, + handleAlibabaStandardSubmit, openAuthDialog, cancelAuthentication, } = useAuthCommand(settings, config, historyManager.addItem, refreshStatic); @@ -1691,6 +1692,7 @@ export const AppContainer = (props: AppContainerProps) => { onAuthError, cancelAuthentication, handleCodingPlanSubmit, + handleAlibabaStandardSubmit, handleEditorSelect, exitEditorDialog, closeSettingsDialog, @@ -1748,6 +1750,7 @@ export const AppContainer = (props: AppContainerProps) => { onAuthError, cancelAuthentication, handleCodingPlanSubmit, + handleAlibabaStandardSubmit, handleEditorSelect, exitEditorDialog, closeSettingsDialog, diff --git a/packages/cli/src/ui/auth/AuthDialog.test.tsx b/packages/cli/src/ui/auth/AuthDialog.test.tsx index 90b15c968..561d5b0b2 100644 --- a/packages/cli/src/ui/auth/AuthDialog.test.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.test.tsx @@ -32,6 +32,9 @@ const createMockUIActions = (overrides: Partial = {}): UIActions => { // AuthDialog only uses handleAuthSelect const baseActions = { handleAuthSelect: vi.fn(), + handleCodingPlanSubmit: vi.fn(), + handleAlibabaStandardSubmit: vi.fn(), + onAuthError: vi.fn(), handleRetryLastPrompt: vi.fn(), } as Partial; diff --git a/packages/cli/src/ui/auth/AuthDialog.tsx b/packages/cli/src/ui/auth/AuthDialog.tsx index 4469a0759..c82524011 100644 --- a/packages/cli/src/ui/auth/AuthDialog.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.tsx @@ -13,6 +13,7 @@ import { theme } from '../semantic-colors.js'; import { useKeypress } from '../hooks/useKeypress.js'; import { DescriptiveRadioButtonSelect } from '../components/shared/DescriptiveRadioButtonSelect.js'; import { ApiKeyInput } from '../components/ApiKeyInput.js'; +import { TextInput } from '../components/shared/TextInput.js'; import { useUIState } from '../contexts/UIStateContext.js'; import { useUIActions } from '../contexts/UIActionsContext.js'; import { useConfig } from '../contexts/ConfigContext.js'; @@ -21,6 +22,10 @@ import { CodingPlanRegion, isCodingPlanConfig, } from '../../constants/codingPlan.js'; +import { + ALIBABA_STANDARD_API_KEY_ENDPOINTS, + type AlibabaStandardRegion, +} from '../../constants/alibabaStandardApiKey.js'; const MODEL_PROVIDERS_DOCUMENTATION_URL = 'https://qwenlm.github.io/qwen-code-docs/en/users/configuration/model-providers/'; @@ -39,15 +44,39 @@ function parseDefaultAuthType( // Main menu option type type MainOption = typeof AuthType.QWEN_OAUTH | 'CODING_PLAN' | 'API_KEY'; +type ApiKeyOption = 'ALIBABA_STANDARD_API_KEY' | 'CUSTOM_API_KEY'; // View level for navigation -type ViewLevel = 'main' | 'region-select' | 'api-key-input' | 'custom-info'; +type ViewLevel = + | 'main' + | 'region-select' + | 'api-key-input' + | 'api-key-type-select' + | 'alibaba-standard-region-select' + | 'alibaba-standard-api-key-input' + | 'alibaba-standard-model-id-input' + | 'custom-info'; + +const ALIBABA_STANDARD_MODEL_IDS_PLACEHOLDER = 'qwen3.5-plus,glm-5,kimi-k2.5'; +const ALIBABA_STANDARD_API_DOCUMENTATION_URLS: Record< + AlibabaStandardRegion, + string +> = { + 'cn-beijing': 'https://bailian.console.aliyun.com/cn-beijing?tab=api#/api', + 'sg-singapore': + 'https://modelstudio.console.alibabacloud.com/ap-southeast-1?tab=api#/api/?type=model&url=2712195', + 'us-virginia': + 'https://modelstudio.console.alibabacloud.com/us-east-1?tab=api#/api/?type=model&url=2712195', + 'cn-hongkong': + 'https://modelstudio.console.alibabacloud.com/cn-hongkong?tab=api#/api/?type=model&url=2712195', +}; export function AuthDialog(): React.JSX.Element { const { pendingAuthType, authError } = useUIState(); const { handleAuthSelect: onAuthSelect, handleCodingPlanSubmit, + handleAlibabaStandardSubmit, onAuthError, } = useUIActions(); const config = useConfig(); @@ -58,6 +87,18 @@ export function AuthDialog(): React.JSX.Element { const [region, setRegion] = useState( CodingPlanRegion.CHINA, ); + const [alibabaStandardRegionIndex, setAlibabaStandardRegionIndex] = + useState(0); + const [apiKeyTypeIndex, setApiKeyTypeIndex] = useState(0); + const [alibabaStandardRegion, setAlibabaStandardRegion] = + useState('cn-beijing'); + const [alibabaStandardApiKey, setAlibabaStandardApiKey] = useState(''); + const [alibabaStandardApiKeyError, setAlibabaStandardApiKeyError] = useState< + string | null + >(null); + const [alibabaStandardModelId, setAlibabaStandardModelId] = useState(''); + const [alibabaStandardModelIdError, setAlibabaStandardModelIdError] = + useState(null); // Main authentication entries (flat three-option layout) const mainItems = [ @@ -124,21 +165,87 @@ export function AuthDialog(): React.JSX.Element { }, ]; + const alibabaStandardRegionItems = [ + { + key: 'cn-beijing', + title: t('China (Beijing)'), + label: t('China (Beijing)'), + description: ( + + Endpoint: {ALIBABA_STANDARD_API_KEY_ENDPOINTS['cn-beijing']} + + ), + value: 'cn-beijing' as AlibabaStandardRegion, + }, + { + key: 'sg-singapore', + title: t('Singapore'), + label: t('Singapore'), + description: ( + + Endpoint: {ALIBABA_STANDARD_API_KEY_ENDPOINTS['sg-singapore']} + + ), + value: 'sg-singapore' as AlibabaStandardRegion, + }, + { + key: 'us-virginia', + title: t('US (Virginia)'), + label: t('US (Virginia)'), + description: ( + + Endpoint: {ALIBABA_STANDARD_API_KEY_ENDPOINTS['us-virginia']} + + ), + value: 'us-virginia' as AlibabaStandardRegion, + }, + { + key: 'cn-hongkong', + title: t('China (Hong Kong)'), + label: t('China (Hong Kong)'), + description: ( + + Endpoint: {ALIBABA_STANDARD_API_KEY_ENDPOINTS['cn-hongkong']} + + ), + value: 'cn-hongkong' as AlibabaStandardRegion, + }, + ]; + + const apiKeyTypeItems = [ + { + key: 'ALIBABA_STANDARD_API_KEY', + title: t('Alibaba Cloud ModelStudio Standard API Key'), + label: t('Alibaba Cloud ModelStudio Standard API Key'), + description: t('Quick setup for Model Studio (China/International)'), + value: 'ALIBABA_STANDARD_API_KEY' as ApiKeyOption, + }, + { + key: 'CUSTOM_API_KEY', + title: t('Custom API Key'), + label: t('Custom API Key'), + description: t( + 'For other OpenAI / Anthropic / Gemini-compatible providers', + ), + value: 'CUSTOM_API_KEY' as ApiKeyOption, + }, + ]; + // Map an AuthType to the corresponding main menu option. - // QWEN_OAUTH maps directly; any other auth type maps to CODING_PLAN only - // if the current config actually uses a Coding Plan baseUrl+envKey, - // otherwise it maps to API_KEY. + // QWEN_OAUTH maps directly; USE_OPENAI maps to: + // - CODING_PLAN when current config matches coding plan + // - API_KEY for other OpenAI / Anthropic / Gemini-compatible configs const contentGenConfig = config.getContentGeneratorConfig(); const isCurrentlyCodingPlan = isCodingPlanConfig( contentGenConfig?.baseUrl, contentGenConfig?.apiKeyEnvKey, ) !== false; - const authTypeToMainOption = (authType: AuthType): MainOption => { if (authType === AuthType.QWEN_OAUTH) return AuthType.QWEN_OAUTH; - if (authType === AuthType.USE_OPENAI && isCurrentlyCodingPlan) + if (authType === AuthType.USE_OPENAI && isCurrentlyCodingPlan) { return 'CODING_PLAN'; + } return 'API_KEY'; }; @@ -180,8 +287,7 @@ export function AuthDialog(): React.JSX.Element { } if (value === 'API_KEY') { - // Navigate directly to custom API key info - setViewLevel('custom-info'); + setViewLevel('api-key-type-select'); return; } @@ -189,6 +295,20 @@ export function AuthDialog(): React.JSX.Element { await onAuthSelect(value); }; + const handleApiKeyTypeSelect = async (value: ApiKeyOption) => { + setErrorMessage(null); + onAuthError(null); + + if (value === 'ALIBABA_STANDARD_API_KEY') { + setAlibabaStandardModelIdError(null); + setAlibabaStandardApiKeyError(null); + setViewLevel('alibaba-standard-region-select'); + return; + } + + setViewLevel('custom-info'); + }; + const handleRegionSelect = async (selectedRegion: CodingPlanRegion) => { setErrorMessage(null); onAuthError(null); @@ -196,6 +316,17 @@ export function AuthDialog(): React.JSX.Element { setViewLevel('api-key-input'); }; + const handleAlibabaStandardRegionSelect = async ( + selectedRegion: AlibabaStandardRegion, + ) => { + setErrorMessage(null); + onAuthError(null); + setAlibabaStandardApiKeyError(null); + setAlibabaStandardModelIdError(null); + setAlibabaStandardRegion(selectedRegion); + setViewLevel('alibaba-standard-api-key-input'); + }; + const handleApiKeyInputSubmit = async (apiKey: string) => { setErrorMessage(null); @@ -208,14 +339,59 @@ export function AuthDialog(): React.JSX.Element { await handleCodingPlanSubmit(apiKey, region); }; + const handleAlibabaStandardApiKeySubmit = () => { + const trimmedKey = alibabaStandardApiKey.trim(); + if (!trimmedKey) { + setAlibabaStandardApiKeyError(t('API key cannot be empty.')); + return; + } + + setAlibabaStandardApiKeyError(null); + if (!alibabaStandardModelId.trim()) { + setAlibabaStandardModelId(ALIBABA_STANDARD_MODEL_IDS_PLACEHOLDER); + } + setViewLevel('alibaba-standard-model-id-input'); + }; + + const handleAlibabaStandardModelSubmit = () => { + const trimmedApiKey = alibabaStandardApiKey.trim(); + const trimmedModelIds = alibabaStandardModelId.trim(); + if (!trimmedApiKey) { + setAlibabaStandardApiKeyError(t('API key cannot be empty.')); + setViewLevel('alibaba-standard-api-key-input'); + return; + } + if (!trimmedModelIds) { + setAlibabaStandardModelIdError(t('Model IDs cannot be empty.')); + return; + } + + setAlibabaStandardModelIdError(null); + void handleAlibabaStandardSubmit( + trimmedApiKey, + alibabaStandardRegion, + trimmedModelIds, + ); + }; + const handleGoBack = () => { setErrorMessage(null); onAuthError(null); - if (viewLevel === 'region-select' || viewLevel === 'custom-info') { + if (viewLevel === 'region-select') { setViewLevel('main'); } else if (viewLevel === 'api-key-input') { setViewLevel('region-select'); + } else if (viewLevel === 'api-key-type-select') { + setViewLevel('main'); + } else if (viewLevel === 'custom-info') { + setViewLevel('api-key-type-select'); + } else if (viewLevel === 'alibaba-standard-region-select') { + setViewLevel('api-key-type-select'); + } else if (viewLevel === 'alibaba-standard-api-key-input') { + setViewLevel('alibaba-standard-region-select'); + } else if (viewLevel === 'alibaba-standard-model-id-input') { + setViewLevel('alibaba-standard-api-key-input'); } }; @@ -232,6 +408,15 @@ export function AuthDialog(): React.JSX.Element { handleGoBack(); return; } + if ( + viewLevel === 'api-key-type-select' || + viewLevel === 'alibaba-standard-region-select' || + viewLevel === 'alibaba-standard-api-key-input' || + viewLevel === 'alibaba-standard-model-id-input' + ) { + handleGoBack(); + return; + } // For main view, use existing logic if (errorMessage) { @@ -304,6 +489,135 @@ export function AuthDialog(): React.JSX.Element { ); + const renderApiKeyTypeSelectView = () => ( + <> + + { + const index = apiKeyTypeItems.findIndex( + (item) => item.value === value, + ); + setApiKeyTypeIndex(index); + }} + itemGap={1} + /> + + + + {t('Enter to select, ↑↓ to navigate, Esc to go back')} + + + + ); + + const renderAlibabaStandardRegionSelectView = () => ( + <> + + { + const index = alibabaStandardRegionItems.findIndex( + (item) => item.value === value, + ); + setAlibabaStandardRegionIndex(index); + }} + itemGap={1} + /> + + + + {t('Enter to select, ↑↓ to navigate, Esc to go back')} + + + + ); + + const renderAlibabaStandardApiKeyInputView = () => ( + + + + Endpoint: {ALIBABA_STANDARD_API_KEY_ENDPOINTS[alibabaStandardRegion]} + + + + {t('Documentation')}: + + + + + {ALIBABA_STANDARD_API_DOCUMENTATION_URLS[alibabaStandardRegion]} + + + + + { + setAlibabaStandardApiKey(value); + if (alibabaStandardApiKeyError) { + setAlibabaStandardApiKeyError(null); + } + }} + onSubmit={handleAlibabaStandardApiKeySubmit} + placeholder="sk-..." + /> + + {alibabaStandardApiKeyError && ( + + {alibabaStandardApiKeyError} + + )} + + + {t('Enter to submit, Esc to go back')} + + + + ); + + const renderAlibabaStandardModelIdInputView = () => ( + + + + {t( + 'You can enter multiple model IDs, separated by commas. Examples: qwen3.5-plus,glm-5,kimi-k2.5', + )} + + + + { + setAlibabaStandardModelId(value); + if (alibabaStandardModelIdError) { + setAlibabaStandardModelIdError(null); + } + }} + onSubmit={handleAlibabaStandardModelSubmit} + placeholder={ALIBABA_STANDARD_MODEL_IDS_PLACEHOLDER} + /> + + {alibabaStandardModelIdError && ( + + {alibabaStandardModelIdError} + + )} + + + {t('Enter to submit, Esc to go back')} + + + + ); + // Render custom mode info const renderCustomInfoView = () => ( <> @@ -336,8 +650,18 @@ export function AuthDialog(): React.JSX.Element { return t('Select Region for Coding Plan'); case 'api-key-input': return t('Enter Coding Plan API Key'); + case 'api-key-type-select': + return t('Select API Key Type'); case 'custom-info': return t('Custom Configuration'); + case 'alibaba-standard-region-select': + return t( + 'Select Region for Alibaba Cloud ModelStudio Standard API Key', + ); + case 'alibaba-standard-api-key-input': + return t('Enter Alibaba Cloud ModelStudio Standard API Key'); + case 'alibaba-standard-model-id-input': + return t('Enter Model IDs'); default: return t('Select Authentication Method'); } @@ -356,6 +680,13 @@ export function AuthDialog(): React.JSX.Element { {viewLevel === 'main' && renderMainView()} {viewLevel === 'region-select' && renderRegionSelectView()} {viewLevel === 'api-key-input' && renderApiKeyInputView()} + {viewLevel === 'api-key-type-select' && renderApiKeyTypeSelectView()} + {viewLevel === 'alibaba-standard-region-select' && + renderAlibabaStandardRegionSelectView()} + {viewLevel === 'alibaba-standard-api-key-input' && + renderAlibabaStandardApiKeyInputView()} + {viewLevel === 'alibaba-standard-model-id-input' && + renderAlibabaStandardModelIdInputView()} {viewLevel === 'custom-info' && renderCustomInfoView()} {(authError || errorMessage) && ( diff --git a/packages/cli/src/ui/auth/useAuth.ts b/packages/cli/src/ui/auth/useAuth.ts index 283a0d155..86c3857aa 100644 --- a/packages/cli/src/ui/auth/useAuth.ts +++ b/packages/cli/src/ui/auth/useAuth.ts @@ -36,6 +36,11 @@ import { CODING_PLAN_ENV_KEY, } from '../../constants/codingPlan.js'; import { backupSettingsFile } from '../../utils/settingsUtils.js'; +import { + ALIBABA_STANDARD_API_KEY_ENDPOINTS, + DASHSCOPE_STANDARD_API_KEY_ENV_KEY, + type AlibabaStandardRegion, +} from '../../constants/alibabaStandardApiKey.js'; export type { QwenAuthState } from '../hooks/useQwenAuth.js'; @@ -421,6 +426,134 @@ export const useAuthCommand = ( [settings, config, handleAuthFailure, addItem, onAuthChange], ); + /** + * Handle Alibaba Cloud standard API key flow. + * Persists key to env.DASHSCOPE_API_KEY and creates a modelProviders.openai entry. + */ + const handleAlibabaStandardSubmit = useCallback( + async ( + apiKey: string, + region: AlibabaStandardRegion, + modelIdsInput: string, + ) => { + try { + setIsAuthenticating(true); + setAuthError(null); + + const trimmedApiKey = apiKey.trim(); + const modelIds = modelIdsInput + .split(',') + .map((id) => id.trim()) + .filter( + (id, index, array) => id.length > 0 && array.indexOf(id) === index, + ); + if (!trimmedApiKey) { + throw new Error(t('API key cannot be empty.')); + } + if (modelIds.length === 0) { + throw new Error(t('Model IDs cannot be empty.')); + } + + const baseUrl = ALIBABA_STANDARD_API_KEY_ENDPOINTS[region]; + const persistScope = getPersistScopeForModelSelection(settings); + + const settingsFile = settings.forScope(persistScope); + backupSettingsFile(settingsFile.path); + + settings.setValue( + persistScope, + `env.${DASHSCOPE_STANDARD_API_KEY_ENV_KEY}`, + trimmedApiKey, + ); + process.env[DASHSCOPE_STANDARD_API_KEY_ENV_KEY] = trimmedApiKey; + + const newConfigs: ProviderModelConfig[] = modelIds.map((modelId) => ({ + id: modelId, + name: `[ModelStudio Standard] ${modelId}`, + baseUrl, + envKey: DASHSCOPE_STANDARD_API_KEY_ENV_KEY, + })); + + const existingConfigs = + ( + settings.merged.modelProviders as ModelProvidersConfig | undefined + )?.[AuthType.USE_OPENAI] || []; + + const nonAlibabaStandardConfigs = existingConfigs.filter( + (existing) => + !( + existing.envKey === DASHSCOPE_STANDARD_API_KEY_ENV_KEY && + typeof existing.baseUrl === 'string' && + Object.values(ALIBABA_STANDARD_API_KEY_ENDPOINTS).includes( + existing.baseUrl, + ) + ), + ); + + const updatedConfigs = [...newConfigs, ...nonAlibabaStandardConfigs]; + + settings.setValue( + persistScope, + `modelProviders.${AuthType.USE_OPENAI}`, + updatedConfigs, + ); + settings.setValue( + persistScope, + 'security.auth.selectedType', + AuthType.USE_OPENAI, + ); + settings.setValue(persistScope, 'model.name', modelIds[0]); + + const updatedModelProviders: ModelProvidersConfig = { + ...(settings.merged.modelProviders as + | ModelProvidersConfig + | undefined), + [AuthType.USE_OPENAI]: updatedConfigs, + }; + config.reloadModelProvidersConfig(updatedModelProviders); + await config.refreshAuth(AuthType.USE_OPENAI); + + setAuthError(null); + setAuthState(AuthState.Authenticated); + setPendingAuthType(undefined); + setIsAuthDialogOpen(false); + setIsAuthenticating(false); + onAuthChange?.(); + + addItem( + { + type: MessageType.INFO, + text: t( + 'Alibaba Cloud ModelStudio Standard API Key successfully entered. Settings updated with env.DASHSCOPE_API_KEY and {{modelCount}} model(s).', + { modelCount: String(modelIds.length) }, + ), + }, + Date.now(), + ); + + addItem( + { + type: MessageType.INFO, + text: t( + 'You can use /model to see new ModelStudio Standard models and switch between them.', + ), + }, + Date.now(), + ); + + const authEvent = new AuthEvent( + AuthType.USE_OPENAI, + 'manual', + 'success', + ); + logAuth(config, authEvent); + } catch (error) { + handleAuthFailure(error); + } + }, + [settings, config, handleAuthFailure, addItem, onAuthChange], + ); + /** /** * We previously used a useEffect to trigger authentication automatically when @@ -472,6 +605,7 @@ export const useAuthCommand = ( qwenAuthState, handleAuthSelect, handleCodingPlanSubmit, + handleAlibabaStandardSubmit, openAuthDialog, cancelAuthentication, }; diff --git a/packages/cli/src/ui/commands/clearCommand.test.ts b/packages/cli/src/ui/commands/clearCommand.test.ts index 1eb4f4707..61e66b53e 100644 --- a/packages/cli/src/ui/commands/clearCommand.test.ts +++ b/packages/cli/src/ui/commands/clearCommand.test.ts @@ -140,6 +140,71 @@ describe('clearCommand', () => { expect(mockContext.ui.clear).toHaveBeenCalledTimes(1); }); + it('should clear UI before resetChat for immediate responsiveness', async () => { + if (!clearCommand.action) { + throw new Error('clearCommand must have an action.'); + } + + const callOrder: string[] = []; + (mockContext.ui.clear as ReturnType).mockImplementation( + () => { + callOrder.push('ui.clear'); + }, + ); + mockResetChat.mockImplementation(async () => { + callOrder.push('resetChat'); + }); + + await clearCommand.action(mockContext, ''); + + // ui.clear should be called before resetChat for immediate UI feedback + const clearIndex = callOrder.indexOf('ui.clear'); + const resetIndex = callOrder.indexOf('resetChat'); + expect(clearIndex).toBeGreaterThanOrEqual(0); + expect(resetIndex).toBeGreaterThanOrEqual(0); + expect(clearIndex).toBeLessThan(resetIndex); + }); + + it('should not await hook events (fire-and-forget)', async () => { + if (!clearCommand.action) { + throw new Error('clearCommand must have an action.'); + } + + // Make hooks take a long time - they should not block + let sessionEndResolved = false; + let sessionStartResolved = false; + mockFireSessionEndEvent.mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => { + sessionEndResolved = true; + resolve(undefined); + }, 5000); + }), + ); + mockFireSessionStartEvent.mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => { + sessionStartResolved = true; + resolve(undefined); + }, 5000); + }), + ); + + await clearCommand.action(mockContext, ''); + + // The action should complete immediately without waiting for hooks + expect(mockContext.ui.clear).toHaveBeenCalledTimes(1); + expect(mockResetChat).toHaveBeenCalledTimes(1); + // Hooks should have been called but not necessarily resolved + expect(mockFireSessionEndEvent).toHaveBeenCalled(); + expect(mockFireSessionStartEvent).toHaveBeenCalled(); + // Hooks should NOT have resolved yet since they have 5s timeouts + expect(sessionEndResolved).toBe(false); + expect(sessionStartResolved).toBe(false); + }); + it('should not attempt to reset chat if config service is not available', async () => { if (!clearCommand.action) { throw new Error('clearCommand must have an action.'); diff --git a/packages/cli/src/ui/commands/clearCommand.ts b/packages/cli/src/ui/commands/clearCommand.ts index ce3b78066..571ee5c6c 100644 --- a/packages/cli/src/ui/commands/clearCommand.ts +++ b/packages/cli/src/ui/commands/clearCommand.ts @@ -27,14 +27,13 @@ export const clearCommand: SlashCommand = { const { config } = context.services; if (config) { - // Fire SessionEnd event before clearing (current session ends) - try { - await config - .getHookSystem() - ?.fireSessionEndEvent(SessionEndReason.Clear); - } catch (err) { - config.getDebugLogger().warn(`SessionEnd hook failed: ${err}`); - } + // Fire SessionEnd event (non-blocking to avoid UI lag) + config + .getHookSystem() + ?.fireSessionEndEvent(SessionEndReason.Clear) + .catch((err) => { + config.getDebugLogger().warn(`SessionEnd hook failed: ${err}`); + }); const newSessionId = config.startNewSession(); @@ -54,6 +53,9 @@ export const clearCommand: SlashCommand = { context.session.startNewSession(newSessionId); } + // Clear UI first for immediate responsiveness + context.ui.clear(); + const geminiClient = config.getGeminiClient(); if (geminiClient) { context.ui.setDebugMessage( @@ -66,22 +68,20 @@ export const clearCommand: SlashCommand = { context.ui.setDebugMessage(t('Starting a new session and clearing.')); } - // Fire SessionStart event after clearing (new session starts) - try { - await config - .getHookSystem() - ?.fireSessionStartEvent( - SessionStartSource.Clear, - config.getModel() ?? '', - String(config.getApprovalMode()) as PermissionMode, - ); - } catch (err) { - config.getDebugLogger().warn(`SessionStart hook failed: ${err}`); - } + // Fire SessionStart event (non-blocking to avoid UI lag) + config + .getHookSystem() + ?.fireSessionStartEvent( + SessionStartSource.Clear, + config.getModel() ?? '', + String(config.getApprovalMode()) as PermissionMode, + ) + .catch((err) => { + config.getDebugLogger().warn(`SessionStart hook failed: ${err}`); + }); } else { context.ui.setDebugMessage(t('Starting a new session and clearing.')); + context.ui.clear(); } - - context.ui.clear(); }, }; diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index e3c9ed1e1..cc70a8809 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -78,6 +78,12 @@ export const ToolConfirmationMessage: React.FC< }, [config]); const handleConfirm = async (outcome: ToolConfirmationOutcome) => { + // Call onConfirm before resolving the IDE diff so that the CLI outcome + // (e.g. ProceedAlways) is processed first. resolveDiffFromCli would + // otherwise trigger the scheduler's ideConfirmation .then() handler + // with ProceedOnce, racing with the intended CLI outcome. + onConfirm(outcome); + if (confirmationDetails.type === 'edit') { if (config.getIdeMode() && isDiffingEnabled) { const cliOutcome = @@ -88,7 +94,6 @@ export const ToolConfirmationMessage: React.FC< ); } } - onConfirm(outcome); }; const isTrustedFolder = config.isTrustedFolder(); diff --git a/packages/cli/src/ui/contexts/KeypressContext.tsx b/packages/cli/src/ui/contexts/KeypressContext.tsx index 97db27563..e81747957 100644 --- a/packages/cli/src/ui/contexts/KeypressContext.tsx +++ b/packages/cli/src/ui/contexts/KeypressContext.tsx @@ -540,7 +540,16 @@ export function KeypressProvider({ } }; + // Matches terminal query responses (DA1, DA2, Kitty protocol query) + // that may arrive late from startup detection in kittyProtocolDetector. + // These are never valid user input. + // eslint-disable-next-line no-control-regex + const TERMINAL_RESPONSE_RE = /^\x1b\[[?>][\d;]*[uc]$/; + const handleKeypress = async (_: unknown, key: Key) => { + if (TERMINAL_RESPONSE_RE.test(key.sequence)) { + return; + } if (key.sequence === FOCUS_IN || key.sequence === FOCUS_OUT) { return; } diff --git a/packages/cli/src/ui/contexts/UIActionsContext.tsx b/packages/cli/src/ui/contexts/UIActionsContext.tsx index 4228149bc..e1a1010b9 100644 --- a/packages/cli/src/ui/contexts/UIActionsContext.tsx +++ b/packages/cli/src/ui/contexts/UIActionsContext.tsx @@ -16,6 +16,7 @@ import { } from '@qwen-code/qwen-code-core'; import { type SettingScope } from '../../config/settings.js'; import { type CodingPlanRegion } from '../../constants/codingPlan.js'; +import { type AlibabaStandardRegion } from '../../constants/alibabaStandardApiKey.js'; import type { AuthState } from '../types.js'; import { type ArenaDialogType } from '../hooks/useArenaCommand.js'; // OpenAICredentials type (previously imported from OpenAIKeyPrompt) @@ -45,6 +46,11 @@ export interface UIActions { apiKey: string, region?: CodingPlanRegion, ) => Promise; + handleAlibabaStandardSubmit: ( + apiKey: string, + region: AlibabaStandardRegion, + modelIdsInput: string, + ) => Promise; setAuthState: (state: AuthState) => void; onAuthError: (error: string | null) => void; cancelAuthentication: () => void; diff --git a/packages/cli/src/ui/hooks/useCommandCompletion.test.ts b/packages/cli/src/ui/hooks/useCommandCompletion.test.ts index 659b99db0..fed160343 100644 --- a/packages/cli/src/ui/hooks/useCommandCompletion.test.ts +++ b/packages/cli/src/ui/hooks/useCommandCompletion.test.ts @@ -417,6 +417,95 @@ describe('useCommandCompletion', () => { }); }); + describe('Completion mode detection', () => { + it('should switch to AT mode when typing @ after a slash command (#2518)', async () => { + setupMocks({ + atSuggestions: [{ label: 'src/file.txt', value: 'src/file.txt' }], + }); + + const text = '/qc:create-issue @file'; + renderHook(() => + useCommandCompletion( + useTextBufferForTest(text), + testDirs, + testRootDir, + [], + mockCommandContext, + false, + mockConfig, + ), + ); + + await waitFor(() => { + expect(useAtCompletion).toHaveBeenLastCalledWith( + expect.objectContaining({ + enabled: true, + pattern: 'file', + }), + ); + }); + }); + + it('should remain in SLASH mode when no @ is typed after slash command', async () => { + setupMocks({ + slashSuggestions: [{ label: 'help', value: 'help' }], + }); + + const text = '/help'; + renderHook(() => + useCommandCompletion( + useTextBufferForTest(text), + testDirs, + testRootDir, + [], + mockCommandContext, + false, + mockConfig, + ), + ); + + await waitFor(() => { + expect(useSlashCompletion).toHaveBeenLastCalledWith( + expect.objectContaining({ + enabled: true, + query: '/help', + }), + ); + }); + }); + + it('should complete a file path when @ appears after a slash command', async () => { + setupMocks({ + atSuggestions: [{ label: 'src/index.ts', value: 'src/index.ts' }], + }); + + const text = '/review @src/ind'; + const { result } = renderHook(() => { + const textBuffer = useTextBufferForTest(text); + const completion = useCommandCompletion( + textBuffer, + testDirs, + testRootDir, + [], + mockCommandContext, + false, + mockConfig, + ); + return { ...completion, textBuffer }; + }); + + await waitFor(() => { + expect(result.current.suggestions.length).toBe(1); + }); + + act(() => { + result.current.handleAutocomplete(0); + }); + + expect(result.current.textBuffer.text).toBe('/review @src/index.ts '); + }); + }); + describe('handleAutocomplete', () => { it('should complete a partial command', async () => { setupMocks({ diff --git a/packages/cli/src/ui/hooks/useCommandCompletion.tsx b/packages/cli/src/ui/hooks/useCommandCompletion.tsx index cb5d9f276..c78e9e46e 100644 --- a/packages/cli/src/ui/hooks/useCommandCompletion.tsx +++ b/packages/cli/src/ui/hooks/useCommandCompletion.tsx @@ -74,15 +74,9 @@ export function useCommandCompletion( const { completionMode, query, completionStart, completionEnd } = useMemo(() => { const currentLine = buffer.lines[cursorRow] || ''; - if (cursorRow === 0 && isSlashCommand(currentLine.trim())) { - return { - completionMode: CompletionMode.SLASH, - query: currentLine, - completionStart: 0, - completionEnd: currentLine.length, - }; - } + // Check for @ completion first, so that typing @ after a slash command + // still triggers file search (see #2518). const codePoints = toCodePoints(currentLine); for (let i = cursorCol - 1; i >= 0; i--) { const char = codePoints[i]; @@ -121,6 +115,15 @@ export function useCommandCompletion( } } + if (cursorRow === 0 && isSlashCommand(currentLine.trim())) { + return { + completionMode: CompletionMode.SLASH, + query: currentLine, + completionStart: 0, + completionEnd: currentLine.length, + }; + } + return { completionMode: CompletionMode.IDLE, query: null, diff --git a/packages/cli/src/ui/utils/kittyProtocolDetector.ts b/packages/cli/src/ui/utils/kittyProtocolDetector.ts index 3355330a6..a46390603 100644 --- a/packages/cli/src/ui/utils/kittyProtocolDetector.ts +++ b/packages/cli/src/ui/utils/kittyProtocolDetector.ts @@ -37,11 +37,20 @@ export async function detectAndEnableKittyProtocol(): Promise { const onTimeout = () => { timeoutId = undefined; process.stdin.removeListener('data', handleData); - if (!originalRawMode) { - process.stdin.setRawMode(false); - } - detectionComplete = true; - resolve(false); + + // Keep a drain handler briefly to consume any late-arriving terminal + // responses that would otherwise leak into the application input. + const drainHandler = () => {}; + process.stdin.on('data', drainHandler); + + setTimeout(() => { + process.stdin.removeListener('data', drainHandler); + if (!originalRawMode) { + process.stdin.setRawMode(false); + } + detectionComplete = true; + resolve(false); + }, 100); }; const handleData = (data: Buffer) => { diff --git a/packages/core/package.json b/packages/core/package.json index cca5ef21c..d42076816 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code-core", - "version": "0.13.0", + "version": "0.13.2", "description": "Qwen Code Core", "repository": { "type": "git", diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 5b1e62fb5..aefe25ea1 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -1582,4 +1582,37 @@ describe('Model Switching and Config Updates', () => { const updatedConfig = config.getContentGeneratorConfig(); expect(updatedConfig['contextWindowSize']).toBe(128_000); }); + + describe('hasHooksForEvent', () => { + it('should return false when hookSystem is not initialized', () => { + const config = new Config(baseParams); + expect(config.hasHooksForEvent('Stop')).toBe(false); + }); + + it('should delegate to hookSystem.hasHooksForEvent when hookSystem exists', () => { + const config = new Config(baseParams); + const mockHasHooksForEvent = vi.fn().mockReturnValue(true); + const mockHookSystem = { + hasHooksForEvent: mockHasHooksForEvent, + }; + // @ts-expect-error - accessing private for testing + config['hookSystem'] = mockHookSystem; + + expect(config.hasHooksForEvent('UserPromptSubmit')).toBe(true); + expect(mockHasHooksForEvent).toHaveBeenCalledWith('UserPromptSubmit'); + }); + + it('should return false when hookSystem has no hooks for the event', () => { + const config = new Config(baseParams); + const mockHasHooksForEvent = vi.fn().mockReturnValue(false); + const mockHookSystem = { + hasHooksForEvent: mockHasHooksForEvent, + }; + // @ts-expect-error - accessing private for testing + config['hookSystem'] = mockHookSystem; + + expect(config.hasHooksForEvent('Stop')).toBe(false); + expect(mockHasHooksForEvent).toHaveBeenCalledWith('Stop'); + }); + }); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index a69e4d29b..c2d131991 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1769,6 +1769,15 @@ export class Config { return this.hookSystem; } + /** + * Fast-path check: returns true only when hooks are enabled AND there are + * registered hooks for the given event name. Callers can use this to skip + * expensive MessageBus round-trips when no hooks are configured. + */ + hasHooksForEvent(eventName: string): boolean { + return this.hookSystem?.hasHooksForEvent(eventName) ?? false; + } + /** * Check if hooks are enabled. */ @@ -2111,7 +2120,7 @@ export class Config { // 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 registerCoreTool = async (ToolClass: any, ...args: unknown[]) => { const toolName = ToolClass?.Name as ToolName | undefined; const className = ToolClass?.name ?? 'UnknownTool'; @@ -2127,7 +2136,7 @@ export class Config { // PermissionManager handles both the coreTools allowlist (registry-level) // and deny rules (runtime-level) in a single check. const pmEnabled = this.permissionManager - ? this.permissionManager.isToolEnabled(toolName) + ? await this.permissionManager.isToolEnabled(toolName) : true; // Should never reach here after initialize(), but safe default. if (pmEnabled) { @@ -2143,10 +2152,10 @@ export class Config { } }; - registerCoreTool(AgentTool, this); - registerCoreTool(SkillTool, this); - registerCoreTool(LSTool, this); - registerCoreTool(ReadFileTool, this); + await registerCoreTool(AgentTool, this); + await registerCoreTool(SkillTool, this); + await registerCoreTool(LSTool, this); + await registerCoreTool(ReadFileTool, this); if (this.getUseRipgrep()) { let useRipgrep = false; @@ -2157,7 +2166,7 @@ export class Config { errorString = getErrorMessage(error); } if (useRipgrep) { - registerCoreTool(RipGrepTool, this); + await registerCoreTool(RipGrepTool, this); } else { // Log for telemetry logRipgrepFallback( @@ -2168,30 +2177,30 @@ export class Config { errorString || 'ripgrep is not available', ), ); - registerCoreTool(GrepTool, this); + await registerCoreTool(GrepTool, this); } } else { - registerCoreTool(GrepTool, this); + await registerCoreTool(GrepTool, this); } - registerCoreTool(GlobTool, this); - registerCoreTool(EditTool, this); - registerCoreTool(WriteFileTool, this); - registerCoreTool(ShellTool, this); - registerCoreTool(MemoryTool); - registerCoreTool(TodoWriteTool, this); - registerCoreTool(AskUserQuestionTool, this); - !this.sdkMode && registerCoreTool(ExitPlanModeTool, this); - registerCoreTool(WebFetchTool, this); + await registerCoreTool(GlobTool, this); + await registerCoreTool(EditTool, this); + await registerCoreTool(WriteFileTool, this); + await registerCoreTool(ShellTool, this); + await registerCoreTool(MemoryTool); + await registerCoreTool(TodoWriteTool, this); + await registerCoreTool(AskUserQuestionTool, this); + !this.sdkMode && (await registerCoreTool(ExitPlanModeTool, this)); + await registerCoreTool(WebFetchTool, this); // Conditionally register web search tool if web search provider is configured // buildWebSearchConfig ensures qwen-oauth users get dashscope provider, so // if tool is registered, config must exist if (this.getWebSearchConfig()) { - registerCoreTool(WebSearchTool, this); + await registerCoreTool(WebSearchTool, this); } if (this.isLspEnabled() && this.getLspClient()) { // Register the unified LSP tool - registerCoreTool(LspTool, this); + await registerCoreTool(LspTool, this); } if (!options?.skipDiscovery) { diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 9527ef071..3c181ba8f 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -363,6 +363,7 @@ describe('Gemini Client (client.ts)', () => { getEnableHooks: vi.fn().mockReturnValue(false), getArenaManager: vi.fn().mockReturnValue(null), getMessageBus: vi.fn().mockReturnValue(undefined), + hasHooksForEvent: vi.fn().mockReturnValue(false), getHookSystem: vi.fn().mockReturnValue(undefined), getDebugLogger: vi.fn().mockReturnValue({ debug: vi.fn(), @@ -2384,6 +2385,105 @@ Other open files: expect(client['sessionTurnCount']).toBe(turnCountBefore); }); }); + + describe('hooks fast-path optimization', () => { + let mockChat: Partial; + + beforeEach(() => { + vi.spyOn(client, 'tryCompressChat').mockResolvedValue({ + originalTokenCount: 0, + newTokenCount: 0, + compressionStatus: CompressionStatus.COMPRESSED, + }); + + const mockStream = (async function* () { + yield { type: 'content', value: 'Hello' }; + })(); + mockTurnRunFn.mockReturnValue(mockStream); + + mockChat = { + addHistory: vi.fn(), + getHistory: vi.fn().mockReturnValue([]), + stripThoughtsFromHistory: vi.fn(), + }; + client['chat'] = mockChat as GeminiChat; + }); + + it('should skip messageBus.request for UserPromptSubmit when hasHooksForEvent returns false', async () => { + // Enable hooks and provide messageBus + const mockMessageBus = { + request: vi.fn(), + response: vi.fn(), + }; + vi.spyOn(client['config'], 'getEnableHooks').mockReturnValue(true); + vi.spyOn(client['config'], 'getMessageBus').mockReturnValue( + mockMessageBus as unknown as ReturnType, + ); + vi.spyOn(client['config'], 'hasHooksForEvent').mockReturnValue(false); + + const stream = client.sendMessageStream( + [{ text: 'Hi' }], + new AbortController().signal, + 'prompt-hooks-1', + ); + for await (const _ of stream) { + // consume stream + } + + // messageBus.request should NOT be called because hasHooksForEvent returned false + expect(mockMessageBus.request).not.toHaveBeenCalled(); + }); + + it('should skip messageBus.request for Stop when hasHooksForEvent returns false', async () => { + const mockMessageBus = { + request: vi.fn(), + response: vi.fn(), + }; + vi.spyOn(client['config'], 'getEnableHooks').mockReturnValue(true); + vi.spyOn(client['config'], 'getMessageBus').mockReturnValue( + mockMessageBus as unknown as ReturnType, + ); + vi.spyOn(client['config'], 'hasHooksForEvent').mockReturnValue(false); + + const stream = client.sendMessageStream( + [{ text: 'Hi' }], + new AbortController().signal, + 'prompt-hooks-2', + ); + for await (const _ of stream) { + // consume stream + } + + // messageBus.request should NOT be called for Stop hook either + expect(mockMessageBus.request).not.toHaveBeenCalled(); + }); + + it('should not skip hooks when hasHooksForEvent returns true', async () => { + const mockMessageBus = { + request: vi.fn().mockResolvedValue({ modifiedPrompt: undefined }), + response: vi.fn(), + }; + vi.spyOn(client['config'], 'getEnableHooks').mockReturnValue(true); + vi.spyOn(client['config'], 'getMessageBus').mockReturnValue( + mockMessageBus as unknown as ReturnType, + ); + vi.spyOn(client['config'], 'hasHooksForEvent').mockImplementation( + (event: string) => event === 'UserPromptSubmit', + ); + + const stream = client.sendMessageStream( + [{ text: 'Hi' }], + new AbortController().signal, + 'prompt-hooks-3', + ); + for await (const _ of stream) { + // consume stream + } + + // messageBus.request SHOULD be called for UserPromptSubmit + expect(mockMessageBus.request).toHaveBeenCalled(); + }); + }); }); describe('generateContent', () => { diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index dfbcc38ea..43c6f556f 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -465,7 +465,12 @@ export class GeminiClient { // Fire UserPromptSubmit hook through MessageBus (only if hooks are enabled) const hooksEnabled = this.config.getEnableHooks(); const messageBus = this.config.getMessageBus(); - if (messageType !== SendMessageType.Retry && hooksEnabled && messageBus) { + if ( + messageType !== SendMessageType.Retry && + hooksEnabled && + messageBus && + this.config.hasHooksForEvent('UserPromptSubmit') + ) { const promptText = partToString(request); const response = await messageBus.request< HookExecutionRequest, @@ -675,14 +680,15 @@ export class GeminiClient { return turn; } } - // Fire Stop hook through MessageBus (only if hooks are enabled) + // Fire Stop hook through MessageBus (only if hooks are enabled and registered) // This must be done before any early returns to ensure hooks are always triggered if ( hooksEnabled && messageBus && !turn.pendingToolCalls.length && signal && - !signal.aborted + !signal.aborted && + this.config.hasHooksForEvent('Stop') ) { // Get response text from the chat history const history = this.getHistory(); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 7279d452b..959c86e99 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -49,7 +49,12 @@ import type { PartListUnion, } from '@google/genai'; import { ToolNames } from '../tools/tool-names.js'; -import { buildPermissionRules } from '../permissions/rule-parser.js'; +import { + buildPermissionCheckContext, + evaluatePermissionRules, + injectPermissionRulesIfMissing, + persistPermissionOutcome, +} from './permission-helpers.js'; import { getResponseTextFromParts } from '../utils/generateContentResponseUtilities.js'; import type { ModifyContext } from '../tools/modifiable-tool.js'; import { @@ -57,7 +62,6 @@ import { modifyWithEditor, } from '../tools/modifiable-tool.js'; import * as Diff from 'diff'; -import * as path from 'node:path'; import levenshtein from 'fast-levenshtein'; import { getPlanModeSystemReminder } from './prompts.js'; import { ShellToolInvocation } from '../tools/shell.js'; @@ -695,122 +699,125 @@ export class CoreToolScheduler { } const requestsToProcess = Array.isArray(request) ? request : [request]; - const newToolCalls: ToolCall[] = requestsToProcess.map( - (reqInfo): ToolCall => { - // Check if the tool is excluded due to permissions/environment restrictions - // This check should happen before registry lookup to provide a clear permission error - const pm = this.config.getPermissionManager?.(); - if (pm && !pm.isToolEnabled(reqInfo.name)) { - const matchingRule = pm.findMatchingDenyRule({ - toolName: reqInfo.name, - }); - const ruleInfo = matchingRule - ? ` Matching deny rule: "${matchingRule}".` - : ''; - const permissionErrorMessage = `Qwen Code requires permission to use "${reqInfo.name}", but that permission was declined.${ruleInfo}`; - return { - status: 'error', - request: reqInfo, - response: createErrorResponse( - reqInfo, - new Error(permissionErrorMessage), - ToolErrorType.EXECUTION_DENIED, - ), - durationMs: 0, - }; - } + const newToolCalls: ToolCall[] = []; + for (const reqInfo of requestsToProcess) { + // Check if the tool is excluded due to permissions/environment restrictions + // This check should happen before registry lookup to provide a clear permission error + const pm = this.config.getPermissionManager?.(); + if (pm && !(await pm.isToolEnabled(reqInfo.name))) { + const matchingRule = pm.findMatchingDenyRule({ + toolName: reqInfo.name, + }); + const ruleInfo = matchingRule + ? ` Matching deny rule: "${matchingRule}".` + : ''; + const permissionErrorMessage = `Qwen Code requires permission to use "${reqInfo.name}", but that permission was declined.${ruleInfo}`; + newToolCalls.push({ + status: 'error', + request: reqInfo, + response: createErrorResponse( + reqInfo, + new Error(permissionErrorMessage), + ToolErrorType.EXECUTION_DENIED, + ), + durationMs: 0, + }); + continue; + } - // Legacy fallback: check getPermissionsDeny() when PM is not available - if (!pm) { - const excludeTools = - this.config.getPermissionsDeny?.() ?? undefined; - if (excludeTools && excludeTools.length > 0) { - const normalizedToolName = reqInfo.name.toLowerCase().trim(); - const excludedMatch = excludeTools.find( - (excludedTool) => - excludedTool.toLowerCase().trim() === normalizedToolName, - ); - if (excludedMatch) { - const permissionErrorMessage = `Qwen Code requires permission to use ${excludedMatch}, but that permission was declined.`; - return { - status: 'error', - request: reqInfo, - response: createErrorResponse( - reqInfo, - new Error(permissionErrorMessage), - ToolErrorType.EXECUTION_DENIED, - ), - durationMs: 0, - }; - } + // Legacy fallback: check getPermissionsDeny() when PM is not available + if (!pm) { + const excludeTools = this.config.getPermissionsDeny?.() ?? undefined; + if (excludeTools && excludeTools.length > 0) { + const normalizedToolName = reqInfo.name.toLowerCase().trim(); + const excludedMatch = excludeTools.find( + (excludedTool) => + excludedTool.toLowerCase().trim() === normalizedToolName, + ); + if (excludedMatch) { + const permissionErrorMessage = `Qwen Code requires permission to use ${excludedMatch}, but that permission was declined.`; + newToolCalls.push({ + status: 'error', + request: reqInfo, + response: createErrorResponse( + reqInfo, + new Error(permissionErrorMessage), + ToolErrorType.EXECUTION_DENIED, + ), + durationMs: 0, + }); + continue; } } + } - const toolInstance = this.toolRegistry.getTool(reqInfo.name); - if (!toolInstance) { - // Tool is not in registry and not excluded - likely hallucinated or typo - const errorMessage = this.getToolNotFoundMessage(reqInfo.name); - return { - status: 'error', - request: reqInfo, - response: createErrorResponse( - reqInfo, - new Error(errorMessage), - ToolErrorType.TOOL_NOT_REGISTERED, - ), - durationMs: 0, - }; - } + const toolInstance = this.toolRegistry.getTool(reqInfo.name); + if (!toolInstance) { + // Tool is not in registry and not excluded - likely hallucinated or typo + const errorMessage = this.getToolNotFoundMessage(reqInfo.name); + newToolCalls.push({ + status: 'error', + request: reqInfo, + response: createErrorResponse( + reqInfo, + new Error(errorMessage), + ToolErrorType.TOOL_NOT_REGISTERED, + ), + durationMs: 0, + }); + continue; + } - const invocationOrError = this.buildInvocation( - toolInstance, - reqInfo.args, - ); - if (invocationOrError instanceof Error) { - const error = reqInfo.wasOutputTruncated - ? new Error( - `${invocationOrError.message} ${TRUNCATION_PARAM_GUIDANCE}`, - ) - : invocationOrError; - return { - status: 'error', - request: reqInfo, - tool: toolInstance, - response: createErrorResponse( - reqInfo, - error, - ToolErrorType.INVALID_TOOL_PARAMS, - ), - durationMs: 0, - }; - } - - // Reject file-modifying calls when truncated to prevent - // writing incomplete content. - if (reqInfo.wasOutputTruncated && toolInstance.kind === Kind.Edit) { - const truncationError = new Error(TRUNCATION_EDIT_REJECTION); - return { - status: 'error', - request: reqInfo, - tool: toolInstance, - response: createErrorResponse( - reqInfo, - truncationError, - ToolErrorType.OUTPUT_TRUNCATED, - ), - durationMs: 0, - }; - } - - return { - status: 'validating', + const invocationOrError = this.buildInvocation( + toolInstance, + reqInfo.args, + ); + if (invocationOrError instanceof Error) { + const error = reqInfo.wasOutputTruncated + ? new Error( + `${invocationOrError.message} ${TRUNCATION_PARAM_GUIDANCE}`, + ) + : invocationOrError; + newToolCalls.push({ + status: 'error', request: reqInfo, tool: toolInstance, - invocation: invocationOrError, - startTime: Date.now(), - }; - }, - ); + response: createErrorResponse( + reqInfo, + error, + ToolErrorType.INVALID_TOOL_PARAMS, + ), + durationMs: 0, + }); + continue; + } + + // Reject file-modifying calls when truncated to prevent + // writing incomplete content. + if (reqInfo.wasOutputTruncated && toolInstance.kind === Kind.Edit) { + const truncationError = new Error(TRUNCATION_EDIT_REJECTION); + newToolCalls.push({ + status: 'error', + request: reqInfo, + tool: toolInstance, + response: createErrorResponse( + reqInfo, + truncationError, + ToolErrorType.OUTPUT_TRUNCATED, + ), + durationMs: 0, + }); + continue; + } + + newToolCalls.push({ + status: 'validating', + request: reqInfo, + tool: toolInstance, + invocation: invocationOrError, + startTime: Date.now(), + }); + } this.toolCalls = this.toolCalls.concat(newToolCalls); this.notifyToolCallsUpdate(); @@ -842,66 +849,14 @@ export class CoreToolScheduler { // ---- L4: PermissionManager override (if relevant rules exist) ---- const pm = this.config.getPermissionManager?.(); - let finalPermission = defaultPermission; - let pmForcedAsk = false; - - // Build invocation context from tool params. - // This is used both by the PM evaluation below and later by - // centralized permission-rule generation (Always Allow). const toolParams = invocation.params as Record; - const shellCommand = - 'command' in toolParams ? String(toolParams['command']) : undefined; - // Extract file path — tools use 'file_path' or 'path' - // (LS / grep / glob). - let invocationFilePath = - typeof toolParams['file_path'] === 'string' - ? toolParams['file_path'] - : undefined; - if ( - invocationFilePath === undefined && - typeof toolParams['path'] === 'string' - ) { - // LS uses absolute paths; grep/glob may be relative to targetDir. - invocationFilePath = path.isAbsolute(toolParams['path']) - ? toolParams['path'] - : path.resolve(this.config.getTargetDir(), toolParams['path']); - } - let invocationDomain: string | undefined; - if (typeof toolParams['url'] === 'string') { - try { - invocationDomain = new URL(toolParams['url']).hostname; - } catch { - // malformed URL — leave domain undefined - } - } - // Generic specifier for literal matching (Skill name, Task subagent type, etc.) - const literalSpecifier = - typeof toolParams['skill'] === 'string' - ? toolParams['skill'] - : typeof toolParams['subagent_type'] === 'string' - ? toolParams['subagent_type'] - : undefined; - const pmCtx = { - toolName: reqInfo.name, - command: shellCommand, - filePath: invocationFilePath, - domain: invocationDomain, - specifier: literalSpecifier, - }; - - if (pm && defaultPermission !== 'deny') { - if (pm.hasRelevantRules(pmCtx)) { - const pmDecision = pm.evaluate(pmCtx); - if (pmDecision !== 'default') { - finalPermission = pmDecision; - // If PM explicitly forces 'ask', adding allow rules won't help - // because ask has higher priority. Hide "Always allow" options. - if (pmDecision === 'ask') { - pmForcedAsk = true; - } - } - } - } + const pmCtx = buildPermissionCheckContext( + reqInfo.name, + toolParams, + this.config.getTargetDir?.() ?? '', + ); + const { finalPermission, pmForcedAsk } = + await evaluatePermissionRules(pm, defaultPermission, pmCtx); // ---- L5: Final decision based on permission + ApprovalMode ---- const approvalMode = this.config.getApprovalMode(); @@ -977,19 +932,7 @@ export class CoreToolScheduler { await invocation.getConfirmationDetails(signal); // ── Centralised rule injection ────────────────────────────────── - // If the tool did not provide its own permissionRules (e.g. Shell - // and WebFetch already do), generate minimum-scope rules from - // the invocation context so that "Always Allow" persists a - // properly scoped rule rather than nothing. - // Only exec/mcp/info types support the permissionRules field. - if ( - (confirmationDetails.type === 'exec' || - confirmationDetails.type === 'mcp' || - confirmationDetails.type === 'info') && - !confirmationDetails.permissionRules - ) { - confirmationDetails.permissionRules = buildPermissionRules(pmCtx); - } + injectPermissionRulesIfMissing(confirmationDetails, pmCtx); // AUTO_EDIT mode: auto-approve edit-like and info tools if ( @@ -1033,6 +976,17 @@ export class CoreToolScheduler { confirmationDetails.ideConfirmation ) { confirmationDetails.ideConfirmation.then((resolution) => { + // Guard: skip if the tool was already handled (e.g. by CLI + // confirmation). Without this check, resolveDiffFromCli + // triggers this handler AND the CLI's onConfirm, causing a + // race where ProceedOnce overwrites ProceedAlways. + const still = this.toolCalls.find( + (c) => + c.request.callId === reqInfo.callId && + c.status === 'awaiting_approval', + ); + if (!still) return; + if (resolution.status === 'accepted') { this.handleConfirmationResponse( reqInfo.callId, @@ -1197,6 +1151,11 @@ export class CoreToolScheduler { (c) => c.request.callId === callId && c.status === 'awaiting_approval', ); + // Guard: if the tool is no longer awaiting approval (already handled by + // another confirmation path, e.g. IDE vs CLI race), skip to avoid double + // processing and potential re-execution. + if (!toolCall) return; + await originalOnConfirm(outcome, payload); if ( @@ -1205,37 +1164,13 @@ export class CoreToolScheduler { outcome === ToolConfirmationOutcome.ProceedAlwaysUser ) { // Persist permission rules for Project/User scope outcomes - if ( - outcome === ToolConfirmationOutcome.ProceedAlwaysProject || - outcome === ToolConfirmationOutcome.ProceedAlwaysUser - ) { - const scope = - outcome === ToolConfirmationOutcome.ProceedAlwaysProject - ? 'project' - : 'user'; - // Read permissionRules from the stored confirmation details first, - // falling back to payload for backward compatibility. - const details = (toolCall as WaitingToolCall | undefined) - ?.confirmationDetails; - const detailsRules = (details as Record | undefined)?.[ - 'permissionRules' - ] as string[] | undefined; - const payloadRules = payload?.permissionRules; - const rules = payloadRules ?? detailsRules ?? []; - const persistFn = this.config.getOnPersistPermissionRule?.(); - const pm = this.config.getPermissionManager?.(); - if (rules.length > 0) { - for (const rule of rules) { - // 1. Persist to disk (settings.json) - if (persistFn) { - await persistFn(scope, 'allow', rule); - } - // 2. Immediately update in-memory PermissionManager so the - // new rule takes effect without restart. - pm?.addPersistentRule(rule, 'allow'); - } - } - } + await persistPermissionOutcome( + outcome, + (toolCall as WaitingToolCall).confirmationDetails, + this.config.getOnPersistPermissionRule?.(), + this.config.getPermissionManager?.(), + payload, + ); await this.autoApproveCompatiblePendingTools(signal, callId); } @@ -1738,50 +1673,20 @@ export class CoreToolScheduler { // Re-run L3→L4 to see if the tool can now be auto-approved const defaultPermission = await pendingTool.invocation.getDefaultPermission(); - let finalPermission = defaultPermission; - - // L4: PM override - const pm = this.config.getPermissionManager?.(); - if (pm && defaultPermission !== 'deny') { - const params = pendingTool.invocation.params as Record< - string, - unknown - >; - const shellCommand = - 'command' in params ? String(params['command']) : undefined; - const filePath = - typeof params['file_path'] === 'string' - ? params['file_path'] - : undefined; - let domain: string | undefined; - if (typeof params['url'] === 'string') { - try { - domain = new URL(params['url']).hostname; - } catch { - // malformed URL - } - } - // Generic specifier for literal matching (Skill name, Task subagent type, etc.) - const literalSpecifier = - typeof params['skill'] === 'string' - ? params['skill'] - : typeof params['subagent_type'] === 'string' - ? params['subagent_type'] - : undefined; - const pmCtx = { - toolName: pendingTool.request.name, - command: shellCommand, - filePath, - domain, - specifier: literalSpecifier, - }; - if (pm.hasRelevantRules(pmCtx)) { - const pmDecision = pm.evaluate(pmCtx); - if (pmDecision !== 'default') { - finalPermission = pmDecision; - } - } - } + const toolParams = pendingTool.invocation.params as Record< + string, + unknown + >; + const pmCtx = buildPermissionCheckContext( + pendingTool.request.name, + toolParams, + this.config.getTargetDir?.() ?? '', + ); + const { finalPermission } = await evaluatePermissionRules( + this.config.getPermissionManager?.(), + defaultPermission, + pmCtx, + ); if (finalPermission === 'allow') { this.setToolCallOutcome( diff --git a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts index 06be16ea5..b39e65357 100644 --- a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts +++ b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.test.ts @@ -66,6 +66,7 @@ const createConfig = (overrides: Record = {}): Config => { return { getContentGeneratorConfig: () => configContent, getAuthType: () => configContent.authType as AuthType | undefined, + getWorkingDir: () => process.cwd(), } as Config; }; diff --git a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts index 61fc885e9..4f4b00138 100644 --- a/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts +++ b/packages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts @@ -62,7 +62,10 @@ export class LoggingContentGenerator implements ContentGenerator { // Extract fields needed for initialization from passed config // (config.getContentGeneratorConfig() may not be available yet during refreshAuth) if (generatorConfig.enableOpenAILogging) { - this.openaiLogger = new OpenAILogger(generatorConfig.openAILoggingDir); + this.openaiLogger = new OpenAILogger( + generatorConfig.openAILoggingDir, + config.getWorkingDir(), + ); this.schemaCompliance = generatorConfig.schemaCompliance; } } diff --git a/packages/core/src/core/permission-helpers.ts b/packages/core/src/core/permission-helpers.ts new file mode 100644 index 000000000..82d960bd6 --- /dev/null +++ b/packages/core/src/core/permission-helpers.ts @@ -0,0 +1,207 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Shared permission-evaluation and persistence helpers. + * + * These are used by both `coreToolScheduler` (CLI mode) and the ACP + * `Session` (VS Code / webui mode) so that the L3→L4→L5 permission flow + * and the "Always Allow" persistence logic stay in sync. + */ + +import * as path from 'node:path'; +import type { PermissionCheckContext } from '../permissions/types.js'; +import type { PermissionManager } from '../permissions/permission-manager.js'; +import type { + ToolCallConfirmationDetails, + ToolConfirmationPayload, +} from '../tools/tools.js'; +import { ToolConfirmationOutcome } from '../tools/tools.js'; +import { buildPermissionRules } from '../permissions/rule-parser.js'; + +// ───────────────────────────────────────────────────────────────────────────── +// Context building +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Build a {@link PermissionCheckContext} from raw tool invocation parameters. + * + * Extracts `command`, `filePath`, `domain`, and `specifier` fields from the + * tool's params, resolving relative paths against `targetDir`. + */ +export function buildPermissionCheckContext( + toolName: string, + toolParams: Record, + targetDir: string, +): PermissionCheckContext { + const command = + 'command' in toolParams ? String(toolParams['command']) : undefined; + + // Extract file path — tools use 'file_path' or 'path' (LS / grep / glob). + let filePath = + typeof toolParams['file_path'] === 'string' + ? toolParams['file_path'] + : undefined; + if (filePath === undefined && typeof toolParams['path'] === 'string') { + // LS uses absolute paths; grep/glob may be relative to targetDir. + filePath = path.isAbsolute(toolParams['path']) + ? toolParams['path'] + : path.resolve(targetDir, toolParams['path']); + } + + let domain: string | undefined; + if (typeof toolParams['url'] === 'string') { + try { + domain = new URL(toolParams['url']).hostname; + } catch { + // malformed URL — leave domain undefined + } + } + + // Generic specifier for literal matching (Skill name, Task subagent type, etc.) + const specifier = + typeof toolParams['skill'] === 'string' + ? toolParams['skill'] + : typeof toolParams['subagent_type'] === 'string' + ? toolParams['subagent_type'] + : undefined; + + return { toolName, command, filePath, domain, specifier }; +} + +// ───────────────────────────────────────────────────────────────────────────── +// PM evaluation +// ───────────────────────────────────────────────────────────────────────────── + +/** Result of {@link evaluatePermissionRules}. */ +export interface PermissionEvalResult { + /** The final permission after PM override. */ + finalPermission: string; + /** + * `true` when PM explicitly forces `'ask'`. In that case "Always Allow" + * buttons should be hidden because allow rules can never override the + * higher-priority ask rule. + */ + pmForcedAsk: boolean; +} + +/** + * L4 — evaluate {@link PermissionManager} rules against the given context. + * + * Returns the final permission decision and whether PM forced 'ask'. + * When `defaultPermission` is already `'deny'`, PM evaluation is skipped. + */ +export async function evaluatePermissionRules( + pm: PermissionManager | null | undefined, + defaultPermission: string, + pmCtx: PermissionCheckContext, +): Promise { + let finalPermission = defaultPermission; + let pmForcedAsk = false; + + if (pm && defaultPermission !== 'deny') { + if (pm.hasRelevantRules(pmCtx)) { + const pmDecision = await pm.evaluate(pmCtx); + if (pmDecision !== 'default') { + finalPermission = pmDecision; + // If PM explicitly forces 'ask', adding allow rules won't help + // because ask has higher priority. Hide "Always allow" options. + if (pmDecision === 'ask' && pm.hasMatchingAskRule(pmCtx)) { + pmForcedAsk = true; + } + } + } + } + + return { finalPermission, pmForcedAsk }; +} + +// ───────────────────────────────────────────────────────────────────────────── +// Centralised rule injection +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Inject centralized permission rules into confirmation details when the tool + * doesn't provide its own. This ensures "Always Allow" persists a properly + * scoped rule rather than nothing. + * + * Only `exec` / `mcp` / `info` types support the `permissionRules` field. + * Mutates `confirmationDetails` in place. + */ +export function injectPermissionRulesIfMissing( + confirmationDetails: ToolCallConfirmationDetails, + pmCtx: PermissionCheckContext, +): void { + if ( + (confirmationDetails.type === 'exec' || + confirmationDetails.type === 'mcp' || + confirmationDetails.type === 'info') && + !confirmationDetails.permissionRules + ) { + confirmationDetails.permissionRules = buildPermissionRules(pmCtx); + } +} + +// ───────────────────────────────────────────────────────────────────────────── +// Permission persistence +// ───────────────────────────────────────────────────────────────────────────── + +/** + * Persist permission rules for `ProceedAlwaysProject` / `ProceedAlwaysUser` + * outcomes. + * + * Reads rules from `confirmationDetails.permissionRules` (set by the tool or + * by {@link injectPermissionRulesIfMissing}), falling back to + * `payload.permissionRules` for backward compatibility. + * + * Writes to disk via `persistFn` and updates the in-memory + * {@link PermissionManager}. No-op for other outcomes. + */ +export async function persistPermissionOutcome( + outcome: ToolConfirmationOutcome, + confirmationDetails: ToolCallConfirmationDetails, + persistFn: + | (( + scope: 'project' | 'user', + ruleType: 'allow' | 'ask' | 'deny', + rule: string, + ) => Promise) + | undefined, + pm: PermissionManager | null | undefined, + payload?: ToolConfirmationPayload, +): Promise { + if ( + outcome !== ToolConfirmationOutcome.ProceedAlwaysProject && + outcome !== ToolConfirmationOutcome.ProceedAlwaysUser + ) { + return; + } + + const scope = + outcome === ToolConfirmationOutcome.ProceedAlwaysProject + ? 'project' + : 'user'; + + // Read permissionRules from the stored confirmation details first, + // falling back to payload for backward compatibility. + const detailsRules = ( + confirmationDetails as unknown as Record + )?.['permissionRules'] as string[] | undefined; + const payloadRules = payload?.permissionRules; + const rules = payloadRules ?? detailsRules ?? []; + + if (rules.length > 0) { + for (const rule of rules) { + // 1. Persist to disk (settings.json) + if (persistFn) { + await persistFn(scope, 'allow', rule); + } + // 2. Immediately update in-memory PermissionManager so the + // new rule takes effect without restart. + pm?.addPersistentRule(rule, 'allow'); + } + } +} diff --git a/packages/core/src/hooks/hookSystem.test.ts b/packages/core/src/hooks/hookSystem.test.ts index cc09289de..0bdbbaf05 100644 --- a/packages/core/src/hooks/hookSystem.test.ts +++ b/packages/core/src/hooks/hookSystem.test.ts @@ -65,6 +65,7 @@ describe('HookSystem', () => { initialize: vi.fn().mockResolvedValue(undefined), setHookEnabled: vi.fn(), getAllHooks: vi.fn().mockReturnValue([]), + getHooksForEvent: vi.fn().mockReturnValue([]), } as unknown as HookRegistry; mockHookRunner = { @@ -186,6 +187,52 @@ describe('HookSystem', () => { }); }); + describe('hasHooksForEvent', () => { + it('should return false when no hooks are registered for the event', () => { + vi.mocked(mockHookRegistry.getHooksForEvent).mockReturnValue([]); + + expect(hookSystem.hasHooksForEvent('Stop')).toBe(false); + expect(mockHookRegistry.getHooksForEvent).toHaveBeenCalledWith('Stop'); + }); + + it('should return true when hooks are registered for the event', () => { + vi.mocked(mockHookRegistry.getHooksForEvent).mockReturnValue([ + { + config: { + type: HookType.Command, + command: 'echo test', + source: HooksConfigSource.Project, + }, + source: HooksConfigSource.Project, + eventName: HookEventName.Stop, + enabled: true, + }, + ]); + + expect(hookSystem.hasHooksForEvent('Stop')).toBe(true); + }); + + it('should check the correct event name for UserPromptSubmit', () => { + vi.mocked(mockHookRegistry.getHooksForEvent).mockReturnValue([]); + + hookSystem.hasHooksForEvent('UserPromptSubmit'); + + expect(mockHookRegistry.getHooksForEvent).toHaveBeenCalledWith( + 'UserPromptSubmit', + ); + }); + + it('should check the correct event name for SessionEnd', () => { + vi.mocked(mockHookRegistry.getHooksForEvent).mockReturnValue([]); + + hookSystem.hasHooksForEvent('SessionEnd'); + + expect(mockHookRegistry.getHooksForEvent).toHaveBeenCalledWith( + 'SessionEnd', + ); + }); + }); + describe('fireStopEvent', () => { it('should fire stop event and return output', async () => { const mockResult = { diff --git a/packages/core/src/hooks/hookSystem.ts b/packages/core/src/hooks/hookSystem.ts index f37d5c712..03d6eebfc 100644 --- a/packages/core/src/hooks/hookSystem.ts +++ b/packages/core/src/hooks/hookSystem.ts @@ -22,6 +22,7 @@ import type { PreCompactTrigger, NotificationType, PermissionSuggestion, + HookEventName, } from './types.js'; const debugLogger = createDebugLogger('TRUSTED_HOOKS'); @@ -87,6 +88,17 @@ export class HookSystem { return this.hookRegistry.getAllHooks(); } + /** + * Check if there are any enabled hooks registered for a specific event. + * This is a fast-path check to avoid expensive MessageBus round-trips + * when no hooks are configured for a given event. + */ + hasHooksForEvent(eventName: string): boolean { + return ( + this.hookRegistry.getHooksForEvent(eventName as HookEventName).length > 0 + ); + } + async fireUserPromptSubmitEvent( prompt: string, signal?: AbortSignal, diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8a498b912..66359a865 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -55,6 +55,7 @@ export * from './output/types.js'; export * from './core/client.js'; export * from './core/contentGenerator.js'; export * from './core/coreToolScheduler.js'; +export * from './core/permission-helpers.js'; export * from './core/geminiChat.js'; export * from './core/geminiRequest.js'; export * from './core/logger.js'; @@ -259,5 +260,6 @@ export type { HookRegistryEntry } from './hooks/index.js'; // Export hook triggers for notification hooks export { fireNotificationHook, + firePermissionRequestHook, type NotificationHookResult, } from './core/toolHookTriggers.js'; diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 94a5126ba..5b1048d5c 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -28,12 +28,12 @@ import type { PermissionManagerConfig } from './permission-manager.js'; // ─── resolveToolName ───────────────────────────────────────────────────────── describe('resolveToolName', () => { - it('resolves canonical names', () => { + it('resolves canonical names', async () => { expect(resolveToolName('run_shell_command')).toBe('run_shell_command'); expect(resolveToolName('read_file')).toBe('read_file'); }); - it('resolves display-name aliases', () => { + it('resolves display-name aliases', async () => { expect(resolveToolName('Shell')).toBe('run_shell_command'); expect(resolveToolName('ShellTool')).toBe('run_shell_command'); expect(resolveToolName('Bash')).toBe('run_shell_command'); @@ -43,25 +43,25 @@ describe('resolveToolName', () => { expect(resolveToolName('WriteFileTool')).toBe('write_file'); }); - it('resolves "Read" and "Edit" meta-categories', () => { + it('resolves "Read" and "Edit" meta-categories', async () => { expect(resolveToolName('Read')).toBe('read_file'); expect(resolveToolName('Edit')).toBe('edit'); expect(resolveToolName('Write')).toBe('write_file'); }); - it('resolves Agent category', () => { + it('resolves Agent category', async () => { expect(resolveToolName('Agent')).toBe('agent'); expect(resolveToolName('agent')).toBe('agent'); expect(resolveToolName('AgentTool')).toBe('agent'); }); - it('resolves legacy task aliases to agent', () => { + it('resolves legacy task aliases to agent', async () => { expect(resolveToolName('task')).toBe('agent'); expect(resolveToolName('Task')).toBe('agent'); expect(resolveToolName('TaskTool')).toBe('agent'); }); - it('returns unknown names unchanged', () => { + it('returns unknown names unchanged', async () => { expect(resolveToolName('my_mcp_tool')).toBe('my_mcp_tool'); expect(resolveToolName('mcp__server__tool')).toBe('mcp__server__tool'); }); @@ -70,11 +70,11 @@ describe('resolveToolName', () => { // ─── getSpecifierKind ──────────────────────────────────────────────────────── describe('getSpecifierKind', () => { - it('returns "command" for shell tools', () => { + it('returns "command" for shell tools', async () => { expect(getSpecifierKind('run_shell_command')).toBe('command'); }); - it('returns "path" for file read/edit tools', () => { + it('returns "path" for file read/edit tools', async () => { expect(getSpecifierKind('read_file')).toBe('path'); expect(getSpecifierKind('edit')).toBe('path'); expect(getSpecifierKind('write_file')).toBe('path'); @@ -83,11 +83,11 @@ describe('getSpecifierKind', () => { expect(getSpecifierKind('list_directory')).toBe('path'); }); - it('returns "domain" for web fetch tools', () => { + it('returns "domain" for web fetch tools', async () => { expect(getSpecifierKind('web_fetch')).toBe('domain'); }); - it('returns "literal" for other tools', () => { + it('returns "literal" for other tools', async () => { expect(getSpecifierKind('Agent')).toBe('literal'); expect(getSpecifierKind('task')).toBe('literal'); expect(getSpecifierKind('mcp__server')).toBe('literal'); @@ -97,22 +97,22 @@ describe('getSpecifierKind', () => { // ─── toolMatchesRuleToolName ───────────────────────────────────────────────── describe('toolMatchesRuleToolName', () => { - it('exact match', () => { + it('exact match', async () => { expect(toolMatchesRuleToolName('read_file', 'read_file')).toBe(true); expect(toolMatchesRuleToolName('edit', 'edit')).toBe(true); }); - it('"Read" (read_file) covers grep_search, glob, list_directory', () => { + it('"Read" (read_file) covers grep_search, glob, list_directory', async () => { expect(toolMatchesRuleToolName('read_file', 'grep_search')).toBe(true); expect(toolMatchesRuleToolName('read_file', 'glob')).toBe(true); expect(toolMatchesRuleToolName('read_file', 'list_directory')).toBe(true); }); - it('"Edit" (edit) covers write_file', () => { + it('"Edit" (edit) covers write_file', async () => { expect(toolMatchesRuleToolName('edit', 'write_file')).toBe(true); }); - it('does not cross categories', () => { + it('does not cross categories', async () => { expect(toolMatchesRuleToolName('read_file', 'edit')).toBe(false); expect(toolMatchesRuleToolName('edit', 'read_file')).toBe(false); expect(toolMatchesRuleToolName('read_file', 'run_shell_command')).toBe( @@ -124,7 +124,7 @@ describe('toolMatchesRuleToolName', () => { // ─── parseRule ─────────────────────────────────────────────────────────────── describe('parseRule', () => { - it('parses a simple tool name', () => { + it('parses a simple tool name', async () => { const r = parseRule('ShellTool'); expect(r.raw).toBe('ShellTool'); expect(r.toolName).toBe('run_shell_command'); @@ -132,59 +132,59 @@ describe('parseRule', () => { expect(r.specifierKind).toBeUndefined(); }); - it('parses Bash alias (Claude Code compat)', () => { + it('parses Bash alias (Claude Code compat)', async () => { const r = parseRule('Bash'); expect(r.toolName).toBe('run_shell_command'); }); - it('parses a shell tool with a specifier', () => { + it('parses a shell tool with a specifier', async () => { const r = parseRule('Bash(git *)'); expect(r.toolName).toBe('run_shell_command'); expect(r.specifier).toBe('git *'); expect(r.specifierKind).toBe('command'); }); - it('parses Read with path specifier', () => { + it('parses Read with path specifier', async () => { const r = parseRule('Read(./secrets/**)'); expect(r.toolName).toBe('read_file'); expect(r.specifier).toBe('./secrets/**'); expect(r.specifierKind).toBe('path'); }); - it('parses Edit with path specifier', () => { + it('parses Edit with path specifier', async () => { const r = parseRule('Edit(/src/**/*.ts)'); expect(r.toolName).toBe('edit'); expect(r.specifier).toBe('/src/**/*.ts'); expect(r.specifierKind).toBe('path'); }); - it('parses WebFetch with domain specifier', () => { + it('parses WebFetch with domain specifier', async () => { const r = parseRule('WebFetch(domain:example.com)'); expect(r.toolName).toBe('web_fetch'); expect(r.specifier).toBe('domain:example.com'); expect(r.specifierKind).toBe('domain'); }); - it('parses Agent with literal specifier', () => { + it('parses Agent with literal specifier', async () => { const r = parseRule('Agent(Explore)'); expect(r.toolName).toBe('agent'); expect(r.specifier).toBe('Explore'); expect(r.specifierKind).toBe('literal'); }); - it('handles unknown tools without specifier', () => { + it('handles unknown tools without specifier', async () => { const r = parseRule('mcp__my_server__my_tool'); expect(r.toolName).toBe('mcp__my_server__my_tool'); expect(r.specifier).toBeUndefined(); }); - it('handles legacy :* suffix (deprecated)', () => { + it('handles legacy :* suffix (deprecated)', async () => { const r = parseRule('Bash(git:*)'); expect(r.toolName).toBe('run_shell_command'); expect(r.specifier).toBe('git *'); }); - it('handles malformed pattern (no closing paren)', () => { + it('handles malformed pattern (no closing paren)', async () => { const r = parseRule('Bash(git status'); expect(r.specifier).toBeUndefined(); }); @@ -193,7 +193,7 @@ describe('parseRule', () => { // ─── parseRules ────────────────────────────────────────────────────────────── describe('parseRules', () => { - it('filters empty strings', () => { + it('filters empty strings', async () => { const rules = parseRules(['ShellTool', '', ' ', 'ReadFileTool']); expect(rules).toHaveLength(2); }); @@ -204,48 +204,48 @@ describe('parseRules', () => { describe('matchesCommandPattern', () => { // Basic prefix matching (no wildcards) describe('prefix matching without glob', () => { - it('exact match', () => { + it('exact match', async () => { expect(matchesCommandPattern('git', 'git')).toBe(true); }); - it('prefix + space', () => { + it('prefix + space', async () => { expect(matchesCommandPattern('git', 'git status')).toBe(true); expect(matchesCommandPattern('git commit', 'git commit -m "test"')).toBe( true, ); }); - it('does not match as substring', () => { + it('does not match as substring', async () => { expect(matchesCommandPattern('git', 'gitcommit')).toBe(false); }); }); // Wildcard at tail describe('wildcard at tail', () => { - it('matches any arguments', () => { + it('matches any arguments', async () => { expect(matchesCommandPattern('git *', 'git status')).toBe(true); expect(matchesCommandPattern('git *', 'git commit -m "test"')).toBe(true); expect(matchesCommandPattern('npm run *', 'npm run build')).toBe(true); }); - it('space-star requires word boundary (ls * does not match lsof)', () => { + it('space-star requires word boundary (ls * does not match lsof)', async () => { expect(matchesCommandPattern('ls *', 'ls -la')).toBe(true); expect(matchesCommandPattern('ls *', 'lsof')).toBe(false); }); - it('no-space-star allows prefix matching (ls* matches lsof)', () => { + it('no-space-star allows prefix matching (ls* matches lsof)', async () => { expect(matchesCommandPattern('ls*', 'ls -la')).toBe(true); expect(matchesCommandPattern('ls*', 'lsof')).toBe(true); }); - it('does not match different command', () => { + it('does not match different command', async () => { expect(matchesCommandPattern('git *', 'echo hello')).toBe(false); }); }); // Wildcard at head describe('wildcard at head', () => { - it('matches any command ending with pattern', () => { + it('matches any command ending with pattern', async () => { expect(matchesCommandPattern('* --version', 'node --version')).toBe(true); expect(matchesCommandPattern('* --version', 'npm --version')).toBe(true); expect(matchesCommandPattern('* --help *', 'npm --help install')).toBe( @@ -253,21 +253,21 @@ describe('matchesCommandPattern', () => { ); }); - it('does not match non-matching suffix', () => { + it('does not match non-matching suffix', async () => { expect(matchesCommandPattern('* --version', 'node --help')).toBe(false); }); }); // Wildcard in middle describe('wildcard in middle', () => { - it('matches middle segments', () => { + it('matches middle segments', async () => { expect(matchesCommandPattern('git * main', 'git checkout main')).toBe( true, ); expect(matchesCommandPattern('git * main', 'git merge main')).toBe(true); }); - it('does not match different suffix', () => { + it('does not match different suffix', async () => { expect(matchesCommandPattern('git * main', 'git checkout dev')).toBe( false, ); @@ -276,19 +276,19 @@ describe('matchesCommandPattern', () => { // Word boundary rule: space before * matters describe('word boundary rule (space before *)', () => { - it('Bash(ls *): matches "ls -la" but NOT "lsof"', () => { + it('Bash(ls *): matches "ls -la" but NOT "lsof"', async () => { expect(matchesCommandPattern('ls *', 'ls -la')).toBe(true); expect(matchesCommandPattern('ls *', 'ls')).toBe(true); // "ls" alone expect(matchesCommandPattern('ls *', 'lsof')).toBe(false); }); - it('Bash(ls*): matches both "ls -la" and "lsof"', () => { + it('Bash(ls*): matches both "ls -la" and "lsof"', async () => { expect(matchesCommandPattern('ls*', 'ls -la')).toBe(true); expect(matchesCommandPattern('ls*', 'lsof')).toBe(true); expect(matchesCommandPattern('ls*', 'ls')).toBe(true); }); - it('Bash(npm *): matches "npm run" but NOT "npmx"', () => { + it('Bash(npm *): matches "npm run" but NOT "npmx"', async () => { expect(matchesCommandPattern('npm *', 'npm run build')).toBe(true); expect(matchesCommandPattern('npm *', 'npmx install')).toBe(false); }); @@ -307,13 +307,13 @@ describe('matchesCommandPattern', () => { // These tests verify that matchesCommandPattern works correctly on // individual simple commands (the sub-commands after splitting). describe('simple command matching (no operators)', () => { - it('matches when no operators are present', () => { + it('matches when no operators are present', async () => { expect( matchesCommandPattern('git *', 'git commit -m "hello world"'), ).toBe(true); }); - it('operators inside quotes are not boundaries for splitCompoundCommand', () => { + it('operators inside quotes are not boundaries for splitCompoundCommand', async () => { // "echo 'a && b'" → the && is inside quotes, not an operator expect(matchesCommandPattern('echo *', "echo 'a && b'")).toBe(true); }); @@ -321,24 +321,24 @@ describe('matchesCommandPattern', () => { // Special: lone * matches any command describe('lone wildcard', () => { - it('* matches any single command', () => { + it('* matches any single command', async () => { expect(matchesCommandPattern('*', 'anything here')).toBe(true); }); }); // Exact command match with specifier describe('exact command specifier', () => { - it('Bash(npm run build) matches exact command', () => { + it('Bash(npm run build) matches exact command', async () => { expect(matchesCommandPattern('npm run build', 'npm run build')).toBe( true, ); }); - it('Bash(npm run build) also matches with trailing args (prefix)', () => { + it('Bash(npm run build) also matches with trailing args (prefix)', async () => { expect( matchesCommandPattern('npm run build', 'npm run build --verbose'), ).toBe(true); }); - it('Bash(npm run build) does not match different command', () => { + it('Bash(npm run build) does not match different command', async () => { expect(matchesCommandPattern('npm run build', 'npm run test')).toBe( false, ); @@ -349,59 +349,59 @@ describe('matchesCommandPattern', () => { // ─── splitCompoundCommand ──────────────────────────────────────────────────── describe('splitCompoundCommand', () => { - it('simple command returns single-element array', () => { + it('simple command returns single-element array', async () => { expect(splitCompoundCommand('git status')).toEqual(['git status']); }); - it('splits on &&', () => { + it('splits on &&', async () => { expect(splitCompoundCommand('git status && rm -rf /')).toEqual([ 'git status', 'rm -rf /', ]); }); - it('splits on ||', () => { + it('splits on ||', async () => { expect(splitCompoundCommand('git push || echo failed')).toEqual([ 'git push', 'echo failed', ]); }); - it('splits on ;', () => { + it('splits on ;', async () => { expect(splitCompoundCommand('echo hello; echo world')).toEqual([ 'echo hello', 'echo world', ]); }); - it('splits on |', () => { + it('splits on |', async () => { expect(splitCompoundCommand('git log | grep fix')).toEqual([ 'git log', 'grep fix', ]); }); - it('handles three-part compound', () => { + it('handles three-part compound', async () => { expect(splitCompoundCommand('a && b && c')).toEqual(['a', 'b', 'c']); }); - it('handles mixed operators', () => { + it('handles mixed operators', async () => { expect(splitCompoundCommand('a && b | c; d')).toEqual(['a', 'b', 'c', 'd']); }); - it('does not split on operators inside single quotes', () => { + it('does not split on operators inside single quotes', async () => { expect(splitCompoundCommand("echo 'a && b'")).toEqual(["echo 'a && b'"]); }); - it('does not split on operators inside double quotes', () => { + it('does not split on operators inside double quotes', async () => { expect(splitCompoundCommand('echo "a && b"')).toEqual(['echo "a && b"']); }); - it('handles escaped characters', () => { + it('handles escaped characters', async () => { expect(splitCompoundCommand('echo a \\&& b')).toEqual(['echo a \\&& b']); }); - it('trims whitespace around sub-commands', () => { + it('trims whitespace around sub-commands', async () => { expect(splitCompoundCommand(' git status && rm -rf / ')).toEqual([ 'git status', 'rm -rf /', @@ -415,13 +415,13 @@ describe('resolvePathPattern', () => { const projectRoot = '/project'; const cwd = '/project/subdir'; - it('// prefix → absolute from filesystem root', () => { + it('// prefix → absolute from filesystem root', async () => { expect( resolvePathPattern('//Users/alice/secrets/**', projectRoot, cwd), ).toBe('/Users/alice/secrets/**'); }); - it('~/ prefix → relative to home directory', () => { + it('~/ prefix → relative to home directory', async () => { const result = resolvePathPattern('~/Documents/*.pdf', projectRoot, cwd); expect(result).toContain('Documents/*.pdf'); // On POSIX systems the home dir starts with '/'; on Windows it may look like @@ -431,25 +431,25 @@ describe('resolvePathPattern', () => { expect(result.startsWith(normalizedHome)).toBe(true); }); - it('/ prefix → relative to project root (NOT absolute)', () => { + it('/ prefix → relative to project root (NOT absolute)', async () => { expect(resolvePathPattern('/src/**/*.ts', projectRoot, cwd)).toBe( '/project/src/**/*.ts', ); }); - it('./ prefix → relative to cwd', () => { + it('./ prefix → relative to cwd', async () => { expect(resolvePathPattern('./secrets/**', projectRoot, cwd)).toBe( '/project/subdir/secrets/**', ); }); - it('no prefix → relative to cwd', () => { + it('no prefix → relative to cwd', async () => { expect(resolvePathPattern('*.env', projectRoot, cwd)).toBe( '/project/subdir/*.env', ); }); - it('/Users/alice/file is relative to project root, NOT absolute', () => { + it('/Users/alice/file is relative to project root, NOT absolute', async () => { // This is a gotcha from the Claude Code docs expect(resolvePathPattern('/Users/alice/file', projectRoot, cwd)).toBe( '/project/Users/alice/file', @@ -463,7 +463,7 @@ describe('matchesPathPattern', () => { const projectRoot = '/project'; const cwd = '/project'; - it('matches dotfiles (e.g. .env)', () => { + it('matches dotfiles (e.g. .env)', async () => { expect(matchesPathPattern('.env', '/project/.env', projectRoot, cwd)).toBe( true, ); @@ -472,7 +472,7 @@ describe('matchesPathPattern', () => { ); }); - it('** matches recursively across directories', () => { + it('** matches recursively across directories', async () => { expect( matchesPathPattern( './secrets/**', @@ -483,7 +483,7 @@ describe('matchesPathPattern', () => { ).toBe(true); }); - it('* matches single directory only', () => { + it('* matches single directory only', async () => { expect( matchesPathPattern( '/src/*.ts', @@ -502,7 +502,7 @@ describe('matchesPathPattern', () => { ).toBe(false); }); - it('/docs/** matches under project root docs', () => { + it('/docs/** matches under project root docs', async () => { expect( matchesPathPattern( '/docs/**', @@ -521,7 +521,7 @@ describe('matchesPathPattern', () => { ).toBe(false); }); - it('//tmp/scratch.txt matches absolute path', () => { + it('//tmp/scratch.txt matches absolute path', async () => { expect( matchesPathPattern( '//tmp/scratch.txt', @@ -532,7 +532,7 @@ describe('matchesPathPattern', () => { ).toBe(true); }); - it('does not match unrelated paths', () => { + it('does not match unrelated paths', async () => { expect( matchesPathPattern( './secrets/**', @@ -547,13 +547,13 @@ describe('matchesPathPattern', () => { // ─── matchesDomainPattern ──────────────────────────────────────────────────── describe('matchesDomainPattern', () => { - it('matches exact domain', () => { + it('matches exact domain', async () => { expect(matchesDomainPattern('domain:example.com', 'example.com')).toBe( true, ); }); - it('matches subdomain', () => { + it('matches subdomain', async () => { expect(matchesDomainPattern('domain:example.com', 'sub.example.com')).toBe( true, ); @@ -562,19 +562,19 @@ describe('matchesDomainPattern', () => { ).toBe(true); }); - it('does not match different domain', () => { + it('does not match different domain', async () => { expect(matchesDomainPattern('domain:example.com', 'notexample.com')).toBe( false, ); }); - it('is case-insensitive', () => { + it('is case-insensitive', async () => { expect(matchesDomainPattern('domain:Example.COM', 'example.com')).toBe( true, ); }); - it('handles missing prefix', () => { + it('handles missing prefix', async () => { expect(matchesDomainPattern('example.com', 'example.com')).toBe(true); }); }); @@ -583,26 +583,26 @@ describe('matchesDomainPattern', () => { describe('matchesRule', () => { // Basic tool name matching - it('simple tool-name rule matches any invocation', () => { + it('simple tool-name rule matches any invocation', async () => { const rule = parseRule('ShellTool'); expect(matchesRule(rule, 'run_shell_command')).toBe(true); expect(matchesRule(rule, 'run_shell_command', 'git status')).toBe(true); }); - it('does not match a different tool', () => { + it('does not match a different tool', async () => { const rule = parseRule('ShellTool'); expect(matchesRule(rule, 'read_file')).toBe(false); }); // Shell command specifier - it('specifier rule requires a command for shell tools', () => { + it('specifier rule requires a command for shell tools', async () => { const rule = parseRule('Bash(git *)'); expect(matchesRule(rule, 'run_shell_command')).toBe(false); // no command expect(matchesRule(rule, 'run_shell_command', 'git status')).toBe(true); expect(matchesRule(rule, 'run_shell_command', 'echo hello')).toBe(false); }); - it('matchesRule checks individual simple commands (compound splitting is at PM level)', () => { + it('matchesRule checks individual simple commands (compound splitting is at PM level)', async () => { const rule = parseRule('Bash(git *)'); // matchesRule receives a simple command (already split by PM) expect(matchesRule(rule, 'run_shell_command', 'git status')).toBe(true); @@ -610,7 +610,7 @@ describe('matchesRule', () => { }); // Meta-category matching: Read - it('Read rule matches grep_search, glob, list_directory', () => { + it('Read rule matches grep_search, glob, list_directory', async () => { const rule = parseRule('Read'); expect(matchesRule(rule, 'read_file')).toBe(true); expect(matchesRule(rule, 'grep_search')).toBe(true); @@ -620,7 +620,7 @@ describe('matchesRule', () => { }); // Meta-category matching: Edit - it('Edit rule matches edit and write_file', () => { + it('Edit rule matches edit and write_file', async () => { const rule = parseRule('Edit'); expect(matchesRule(rule, 'edit')).toBe(true); expect(matchesRule(rule, 'write_file')).toBe(true); @@ -628,7 +628,7 @@ describe('matchesRule', () => { }); // File path matching - it('Read with path specifier requires filePath', () => { + it('Read with path specifier requires filePath', async () => { const rule = parseRule('Read(.env)'); const pathCtx = { projectRoot: '/project', cwd: '/project' }; // No filePath → no match @@ -656,7 +656,7 @@ describe('matchesRule', () => { ).toBe(false); }); - it('Edit path specifier matches write_file too', () => { + it('Edit path specifier matches write_file too', async () => { const rule = parseRule('Edit(/src/**/*.ts)'); const pathCtx = { projectRoot: '/project', cwd: '/project' }; expect( @@ -682,7 +682,7 @@ describe('matchesRule', () => { }); // WebFetch domain matching - it('WebFetch domain specifier', () => { + it('WebFetch domain specifier', async () => { const rule = parseRule('WebFetch(domain:example.com)'); expect( matchesRule(rule, 'web_fetch', undefined, undefined, 'example.com'), @@ -698,7 +698,7 @@ describe('matchesRule', () => { }); // Agent literal matching - it('Agent literal specifier', () => { + it('Agent literal specifier', async () => { const rule = parseRule('Agent(Explore)'); // Agent is an alias for 'task'; specifier matches via the specifier field expect( @@ -727,26 +727,26 @@ describe('matchesRule', () => { }); // MCP tool matching - it('MCP tool exact match', () => { + it('MCP tool exact match', async () => { const rule = parseRule('mcp__puppeteer__puppeteer_navigate'); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_navigate')).toBe(true); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_click')).toBe(false); }); - it('MCP server-level match (2-part pattern)', () => { + it('MCP server-level match (2-part pattern)', async () => { const rule = parseRule('mcp__puppeteer'); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_navigate')).toBe(true); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_click')).toBe(true); expect(matchesRule(rule, 'mcp__other__tool')).toBe(false); }); - it('MCP wildcard match', () => { + it('MCP wildcard match', async () => { const rule = parseRule('mcp__puppeteer__*'); expect(matchesRule(rule, 'mcp__puppeteer__puppeteer_navigate')).toBe(true); expect(matchesRule(rule, 'mcp__other__tool')).toBe(false); }); - it('MCP intra-segment wildcard match (e.g. mcp__chrome__use_*)', () => { + it('MCP intra-segment wildcard match (e.g. mcp__chrome__use_*)', async () => { const rule = parseRule('mcp__chrome__use_*'); expect(matchesRule(rule, 'mcp__chrome__use_browser')).toBe(true); expect(matchesRule(rule, 'mcp__chrome__use_context')).toBe(true); @@ -792,25 +792,25 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('returns deny for a denied tool', () => { - expect(pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); + it('returns deny for a denied tool', async () => { + expect(await pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); }); - it('returns ask for an ask-rule tool', () => { - expect(pm.evaluate({ toolName: 'write_file' })).toBe('ask'); + it('returns ask for an ask-rule tool', async () => { + expect(await pm.evaluate({ toolName: 'write_file' })).toBe('ask'); }); - it('returns allow for an allow-rule tool', () => { - expect(pm.evaluate({ toolName: 'read_file' })).toBe('allow'); + it('returns allow for an allow-rule tool', async () => { + expect(await pm.evaluate({ toolName: 'read_file' })).toBe('allow'); }); - it('returns default for unmatched tool', () => { + it('returns default for unmatched tool', async () => { // Note: 'glob' is covered by ReadFileTool via Read meta-category, // so use a tool not in any rule or meta-category - expect(pm.evaluate({ toolName: 'agent' })).toBe('default'); + expect(await pm.evaluate({ toolName: 'agent' })).toBe('default'); }); - it('deny takes precedence over ask and allow', () => { + it('deny takes precedence over ask and allow', async () => { const pm2 = new PermissionManager( makeConfig({ permissionsAllow: ['run_shell_command'], @@ -819,10 +819,12 @@ describe('PermissionManager', () => { }), ); pm2.initialize(); - expect(pm2.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); + expect(await pm2.evaluate({ toolName: 'run_shell_command' })).toBe( + 'deny', + ); }); - it('ask takes precedence over allow', () => { + it('ask takes precedence over allow', async () => { const pm2 = new PermissionManager( makeConfig({ permissionsAllow: ['write_file'], @@ -830,7 +832,7 @@ describe('PermissionManager', () => { }), ); pm2.initialize(); - expect(pm2.evaluate({ toolName: 'write_file' })).toBe('ask'); + expect(await pm2.evaluate({ toolName: 'write_file' })).toBe('ask'); }); }); @@ -845,33 +847,51 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('allows a matching allowed command', () => { + it('allows a matching allowed command', async () => { expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'git status' }), + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git status', + }), ).toBe('allow'); }); - it('denies a matching denied command', () => { + it('denies a matching denied command', async () => { expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'rm -rf /' }), + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'rm -rf /', + }), ).toBe('deny'); }); - it('returns default for an unmatched command', () => { + it('resolves default to allow for readonly commands, ask for others', async () => { + // 'echo' is a readonly command, so it resolves to 'allow' expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'echo hello' }), - ).toBe('default'); + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'echo hello', + }), + ).toBe('allow'); + // 'npm install' is not readonly, so it resolves to 'ask' + expect( + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'npm install', + }), + ).toBe('ask'); }); - it('isCommandAllowed delegates to evaluate', () => { - expect(pm.isCommandAllowed('git commit')).toBe('allow'); - expect(pm.isCommandAllowed('rm -rf /')).toBe('deny'); - expect(pm.isCommandAllowed('ls')).toBe('default'); + it('isCommandAllowed delegates to evaluate', async () => { + expect(await pm.isCommandAllowed('git commit')).toBe('allow'); + expect(await pm.isCommandAllowed('rm -rf /')).toBe('deny'); + // 'ls' is readonly, resolves to 'allow' when no rule matches + expect(await pm.isCommandAllowed('ls')).toBe('allow'); }); }); describe('compound command evaluation', () => { - it('all sub-commands allowed → allow', () => { + it('all sub-commands allowed → allow', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(safe-cmd *)', 'Bash(one-cmd *)'], @@ -879,29 +899,30 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'safe-cmd arg1 && one-cmd arg2', }), ).toBe('allow'); }); - it('one sub-command unmatched → default (most restrictive)', () => { + it('one sub-command unmatched (non-readonly) → ask (resolved from default)', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(safe-cmd *)'], }), ); pm.initialize(); + // 'two-cmd' is unknown/non-readonly, so its default permission is 'ask' expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'safe-cmd && two-cmd', }), - ).toBe('default'); + ).toBe('ask'); }); - it('one sub-command denied → deny', () => { + it('one sub-command denied → deny', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(safe-cmd *)'], @@ -910,14 +931,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'safe-cmd && evil-cmd rm-all', }), ).toBe('deny'); }); - it('one sub-command ask + one allow → ask', () => { + it('one sub-command ask + one allow → ask', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)'], @@ -926,14 +947,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git status && npm publish', }), ).toBe('ask'); }); - it('pipe compound: all matched → allow', () => { + it('pipe compound: all matched → allow', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)', 'Bash(grep *)'], @@ -941,29 +962,30 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git log | grep fix', }), ).toBe('allow'); }); - it('pipe compound: second unmatched → default', () => { + it('pipe compound: second unmatched but readonly → allow (resolved from default)', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)'], }), ); pm.initialize(); + // 'grep' is a readonly command, so its default permission is 'allow' expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git log | grep fix', }), - ).toBe('default'); + ).toBe('allow'); }); - it('semicolon compound: deny in second → deny', () => { + it('semicolon compound: deny in second → deny', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(echo *)'], @@ -972,14 +994,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'echo hello; rm -rf /', }), ).toBe('deny'); }); - it('|| compound: all allowed → allow', () => { + it('|| compound: all allowed → allow', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)', 'Bash(echo *)'], @@ -987,14 +1009,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git push || echo failed', }), ).toBe('allow'); }); - it('operators inside quotes: treated as single command', () => { + it('operators inside quotes: treated as single command', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(echo *)'], @@ -1002,14 +1024,14 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: "echo 'a && b'", }), ).toBe('allow'); }); - it('three-part compound: all must pass', () => { + it('three-part compound: all must pass', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)', 'Bash(npm *)', 'Bash(echo *)'], @@ -1017,29 +1039,30 @@ describe('PermissionManager', () => { ); pm.initialize(); expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git add . && npm test && echo done', }), ).toBe('allow'); }); - it('three-part compound: one unmatched → default', () => { + it('three-part compound: one unmatched (non-readonly) → ask (resolved from default)', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(git *)', 'Bash(echo *)'], }), ); pm.initialize(); + // 'npm test' is not readonly, so its default permission is 'ask' expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'run_shell_command', command: 'git add . && npm test && echo done', }), - ).toBe('default'); + ).toBe('ask'); }); - it('isCommandAllowed also handles compound commands', () => { + it('isCommandAllowed also handles compound commands', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['Bash(safe-cmd *)', 'Bash(one-cmd *)'], @@ -1047,9 +1070,16 @@ describe('PermissionManager', () => { }), ); pm.initialize(); - expect(pm.isCommandAllowed('safe-cmd a && one-cmd b')).toBe('allow'); - expect(pm.isCommandAllowed('safe-cmd a && unknown-cmd')).toBe('default'); - expect(pm.isCommandAllowed('safe-cmd a && evil-cmd b')).toBe('deny'); + expect(await pm.isCommandAllowed('safe-cmd a && one-cmd b')).toBe( + 'allow', + ); + // 'unknown-cmd' is not readonly, resolves to 'ask' + expect(await pm.isCommandAllowed('safe-cmd a && unknown-cmd')).toBe( + 'ask', + ); + expect(await pm.isCommandAllowed('safe-cmd a && evil-cmd b')).toBe( + 'deny', + ); }); }); @@ -1066,39 +1096,42 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('denies reading a denied file', () => { + it('denies reading a denied file', async () => { expect( - pm.evaluate({ toolName: 'read_file', filePath: '/project/.env' }), + await pm.evaluate({ toolName: 'read_file', filePath: '/project/.env' }), ).toBe('deny'); }); - it('denies editing in a denied directory', () => { + it('denies editing in a denied directory', async () => { expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'edit', filePath: '/project/src/generated/code.ts', }), ).toBe('deny'); }); - it('allows reading in an allowed directory', () => { + it('allows reading in an allowed directory', async () => { expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'read_file', filePath: '/project/docs/readme.md', }), ).toBe('allow'); }); - it('Read deny applies to grep_search too (meta-category)', () => { + it('Read deny applies to grep_search too (meta-category)', async () => { expect( - pm.evaluate({ toolName: 'grep_search', filePath: '/project/.env' }), + await pm.evaluate({ + toolName: 'grep_search', + filePath: '/project/.env', + }), ).toBe('deny'); }); - it('returns default for unmatched path', () => { + it('returns default for unmatched path', async () => { expect( - pm.evaluate({ + await pm.evaluate({ toolName: 'read_file', filePath: '/project/src/index.ts', }), @@ -1117,89 +1150,89 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('allows fetch to allowed domain', () => { - expect(pm.evaluate({ toolName: 'web_fetch', domain: 'github.com' })).toBe( - 'allow', - ); - }); - - it('allows fetch to subdomain of allowed domain', () => { + it('allows fetch to allowed domain', async () => { expect( - pm.evaluate({ toolName: 'web_fetch', domain: 'api.github.com' }), + await pm.evaluate({ toolName: 'web_fetch', domain: 'github.com' }), ).toBe('allow'); }); - it('denies fetch to denied domain', () => { - expect(pm.evaluate({ toolName: 'web_fetch', domain: 'evil.com' })).toBe( - 'deny', - ); + it('allows fetch to subdomain of allowed domain', async () => { + expect( + await pm.evaluate({ toolName: 'web_fetch', domain: 'api.github.com' }), + ).toBe('allow'); }); - it('returns default for unmatched domain', () => { + it('denies fetch to denied domain', async () => { expect( - pm.evaluate({ toolName: 'web_fetch', domain: 'example.com' }), + await pm.evaluate({ toolName: 'web_fetch', domain: 'evil.com' }), + ).toBe('deny'); + }); + + it('returns default for unmatched domain', async () => { + expect( + await pm.evaluate({ toolName: 'web_fetch', domain: 'example.com' }), ).toBe('default'); }); }); describe('isToolEnabled', () => { - it('returns false for deny-ruled tools', () => { + it('returns false for deny-ruled tools', async () => { pm = new PermissionManager( makeConfig({ permissionsDeny: ['ShellTool'] }), ); pm.initialize(); - expect(pm.isToolEnabled('run_shell_command')).toBe(false); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); }); - it('returns true for tools with only specifier deny rules', () => { + it('returns true for tools with only specifier deny rules', async () => { pm = new PermissionManager( makeConfig({ permissionsDeny: ['Bash(rm *)'] }), ); pm.initialize(); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); }); - it('excludeTools passed via permissionsDeny disables the tool', () => { + it('excludeTools passed via permissionsDeny disables the tool', async () => { pm = new PermissionManager( makeConfig({ permissionsDeny: ['run_shell_command'] }), ); pm.initialize(); - expect(pm.isToolEnabled('run_shell_command')).toBe(false); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); }); - it('coreTools allowlist: listed tool is enabled', () => { + it('coreTools allowlist: listed tool is enabled', async () => { pm = new PermissionManager( makeConfig({ coreTools: ['read_file', 'Bash'] }), ); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); // Bash resolves to run_shell_command + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); // Bash resolves to run_shell_command }); - it('coreTools allowlist: unlisted tool is disabled', () => { + it('coreTools allowlist: unlisted tool is disabled', async () => { pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] })); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(false); - expect(pm.isToolEnabled('edit')).toBe(false); + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); + expect(await pm.isToolEnabled('edit')).toBe(false); }); - it('coreTools with specifier: tool-level check strips specifier', () => { + it('coreTools with specifier: tool-level check strips specifier', async () => { // "Bash(ls -l)" should register run_shell_command (specifier only affects runtime) pm = new PermissionManager(makeConfig({ coreTools: ['Bash(ls -l)'] })); pm.initialize(); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); - expect(pm.isToolEnabled('read_file')).toBe(false); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); + expect(await pm.isToolEnabled('read_file')).toBe(false); }); - it('empty coreTools: all tools enabled (no whitelist restriction)', () => { + it('empty coreTools: all tools enabled (no whitelist restriction)', async () => { pm = new PermissionManager(makeConfig({ coreTools: [] })); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); }); - it('coreTools allowlist + deny rule: deny takes precedence for listed tools', () => { + it('coreTools allowlist + deny rule: deny takes precedence for listed tools', async () => { pm = new PermissionManager( makeConfig({ coreTools: ['read_file', 'Bash'], @@ -1207,19 +1240,19 @@ describe('PermissionManager', () => { }), ); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(false); // in list but denied + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(false); // in list but denied }); - it('permissionsAllow alone does NOT restrict unlisted tools (not a whitelist)', () => { + it('permissionsAllow alone does NOT restrict unlisted tools (not a whitelist)', async () => { // This verifies the previous incorrect behavior is gone: permissionsAllow // only means "auto-approve", it does NOT block unlisted tools. pm = new PermissionManager( makeConfig({ permissionsAllow: ['read_file'] }), ); pm.initialize(); - expect(pm.isToolEnabled('read_file')).toBe(true); - expect(pm.isToolEnabled('run_shell_command')).toBe(true); // not denied, just unreviewed + expect(await pm.isToolEnabled('read_file')).toBe(true); + expect(await pm.isToolEnabled('run_shell_command')).toBe(true); // not denied, just unreviewed }); }); @@ -1229,38 +1262,48 @@ describe('PermissionManager', () => { pm.initialize(); }); - it('addSessionAllowRule enables auto-approval for that pattern', () => { + it('addSessionAllowRule enables auto-approval for that pattern', async () => { + // Use 'git commit' which is not readonly, so it resolves to 'ask' by default expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'git status' }), - ).toBe('default'); + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git commit', + }), + ).toBe('ask'); pm.addSessionAllowRule('Bash(git *)'); expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'git status' }), + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git commit', + }), ).toBe('allow'); }); - it('session deny rules override allow rules', () => { + it('session deny rules override allow rules', async () => { pm.addSessionAllowRule('run_shell_command'); pm.addSessionDenyRule('run_shell_command'); - expect(pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); + expect(await pm.evaluate({ toolName: 'run_shell_command' })).toBe('deny'); }); }); describe('allowedTools via permissionsAllow', () => { - it('allow rule auto-approves matching tools/commands', () => { + it('allow rule auto-approves matching tools/commands', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['ReadFileTool', 'Bash(git *)'] }), ); pm.initialize(); - expect(pm.evaluate({ toolName: 'read_file' })).toBe('allow'); + expect(await pm.evaluate({ toolName: 'read_file' })).toBe('allow'); expect( - pm.evaluate({ toolName: 'run_shell_command', command: 'git status' }), + await pm.evaluate({ + toolName: 'run_shell_command', + command: 'git status', + }), ).toBe('allow'); }); }); describe('listRules', () => { - it('returns all rules with type and scope', () => { + it('returns all rules with type and scope', async () => { pm = new PermissionManager( makeConfig({ permissionsAllow: ['ReadFileTool'], @@ -1278,37 +1321,67 @@ describe('PermissionManager', () => { expect(sessionAllow?.rule.toolName).toBe('run_shell_command'); }); }); + + describe('hasMatchingAskRule', () => { + it('returns false when shell ask comes only from default permission fallback', async () => { + pm = new PermissionManager( + makeConfig({ permissionsAllow: ['Bash(git add *)'] }), + ); + pm.initialize(); + + expect( + pm.hasMatchingAskRule({ + toolName: 'run_shell_command', + command: 'git add file && git commit -m "msg"', + }), + ).toBe(false); + }); + + it('returns true when an explicit ask rule matches a shell sub-command', async () => { + pm = new PermissionManager( + makeConfig({ permissionsAsk: ['Bash(git commit *)'] }), + ); + pm.initialize(); + + expect( + pm.hasMatchingAskRule({ + toolName: 'run_shell_command', + command: 'git add file && git commit -m "msg"', + }), + ).toBe(true); + }); + }); }); // ─── getRuleDisplayName ────────────────────────────────────────────────────── describe('getRuleDisplayName', () => { - it('maps read tools to "Read" meta-category', () => { + it('maps read tools to "Read" meta-category', async () => { expect(getRuleDisplayName('read_file')).toBe('Read'); expect(getRuleDisplayName('grep_search')).toBe('Read'); expect(getRuleDisplayName('glob')).toBe('Read'); expect(getRuleDisplayName('list_directory')).toBe('Read'); }); - it('maps edit tools to "Edit" meta-category', () => { + it('maps edit tools to "Edit" meta-category', async () => { expect(getRuleDisplayName('edit')).toBe('Edit'); expect(getRuleDisplayName('write_file')).toBe('Edit'); }); - it('maps shell to "Bash"', () => { + it('maps shell to "Bash"', async () => { expect(getRuleDisplayName('run_shell_command')).toBe('Bash'); }); - it('maps web_fetch to "WebFetch"', () => { + it('maps web_fetch to "WebFetch"', async () => { expect(getRuleDisplayName('web_fetch')).toBe('WebFetch'); }); - it('maps agent to "Agent" and skill to "Skill"', () => { + it('maps agent to "Agent" and skill to "Skill"', async () => { expect(getRuleDisplayName('agent')).toBe('Agent'); expect(getRuleDisplayName('skill')).toBe('Skill'); }); - it('returns the canonical name for unknown tools (e.g. MCP)', () => { + it('returns the canonical name for unknown tools (e.g. MCP)', async () => { expect(getRuleDisplayName('mcp__server__tool')).toBe('mcp__server__tool'); }); }); @@ -1317,7 +1390,7 @@ describe('getRuleDisplayName', () => { describe('buildPermissionRules', () => { describe('path-based tools (Read/Edit)', () => { - it('generates Read rule scoped to parent directory for read_file', () => { + it('generates Read rule scoped to parent directory for read_file', async () => { const rules = buildPermissionRules({ toolName: 'read_file', filePath: '/Users/alice/.secrets', @@ -1326,7 +1399,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Read(//Users/alice/**)']); }); - it('generates Read rule with directory as-is for grep_search', () => { + it('generates Read rule with directory as-is for grep_search', async () => { const rules = buildPermissionRules({ toolName: 'grep_search', filePath: '/external/dir', @@ -1335,7 +1408,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Read(//external/dir/**)']); }); - it('generates Read rule with directory as-is for glob', () => { + it('generates Read rule with directory as-is for glob', async () => { const rules = buildPermissionRules({ toolName: 'glob', filePath: '/tmp/data', @@ -1343,7 +1416,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Read(//tmp/data/**)']); }); - it('generates Read rule with directory as-is for list_directory', () => { + it('generates Read rule with directory as-is for list_directory', async () => { const rules = buildPermissionRules({ toolName: 'list_directory', filePath: '/home/user/docs', @@ -1351,7 +1424,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Read(//home/user/docs/**)']); }); - it('generates Edit rule scoped to parent directory for edit', () => { + it('generates Edit rule scoped to parent directory for edit', async () => { const rules = buildPermissionRules({ toolName: 'edit', filePath: '/external/file.ts', @@ -1360,7 +1433,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Edit(//external/**)']); }); - it('generates Edit rule scoped to parent directory for write_file', () => { + it('generates Edit rule scoped to parent directory for write_file', async () => { const rules = buildPermissionRules({ toolName: 'write_file', filePath: '/tmp/output.txt', @@ -1368,14 +1441,14 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Edit(//tmp/**)']); }); - it('falls back to bare display name when no filePath', () => { + it('falls back to bare display name when no filePath', async () => { const rules = buildPermissionRules({ toolName: 'read_file' }); expect(rules).toEqual(['Read']); }); }); describe('generated rules round-trip through parseRule and matchesRule', () => { - it('Read rule for external file covers the containing directory', () => { + it('Read rule for external file covers the containing directory', async () => { const rules = buildPermissionRules({ toolName: 'read_file', filePath: '/Users/alice/.secrets', @@ -1425,7 +1498,7 @@ describe('buildPermissionRules', () => { ).toBe(false); }); - it('Read rule also matches other read-family tools on the same path', () => { + it('Read rule also matches other read-family tools on the same path', async () => { const rules = buildPermissionRules({ toolName: 'grep_search', filePath: '/external/dir', @@ -1459,7 +1532,7 @@ describe('buildPermissionRules', () => { }); describe('domain-based tools', () => { - it('generates WebFetch rule with domain specifier', () => { + it('generates WebFetch rule with domain specifier', async () => { const rules = buildPermissionRules({ toolName: 'web_fetch', domain: 'example.com', @@ -1467,14 +1540,14 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['WebFetch(example.com)']); }); - it('falls back to bare display name when no domain', () => { + it('falls back to bare display name when no domain', async () => { const rules = buildPermissionRules({ toolName: 'web_fetch' }); expect(rules).toEqual(['WebFetch']); }); }); describe('command-based tools', () => { - it('generates Bash rule with command specifier', () => { + it('generates Bash rule with command specifier', async () => { const rules = buildPermissionRules({ toolName: 'run_shell_command', command: 'git status', @@ -1482,14 +1555,14 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Bash(git status)']); }); - it('falls back to bare display name when no command', () => { + it('falls back to bare display name when no command', async () => { const rules = buildPermissionRules({ toolName: 'run_shell_command' }); expect(rules).toEqual(['Bash']); }); }); describe('literal-specifier tools', () => { - it('generates Skill rule with specifier', () => { + it('generates Skill rule with specifier', async () => { const rules = buildPermissionRules({ toolName: 'skill', specifier: 'Explore', @@ -1497,7 +1570,7 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Skill(Explore)']); }); - it('generates Agent rule with specifier', () => { + it('generates Agent rule with specifier', async () => { const rules = buildPermissionRules({ toolName: 'agent', specifier: 'research', @@ -1505,14 +1578,14 @@ describe('buildPermissionRules', () => { expect(rules).toEqual(['Agent(research)']); }); - it('falls back to bare display name when no specifier', () => { + it('falls back to bare display name when no specifier', async () => { const rules = buildPermissionRules({ toolName: 'skill' }); expect(rules).toEqual(['Skill']); }); }); describe('unknown / MCP tools', () => { - it('uses the canonical name as display for MCP tools', () => { + it('uses the canonical name as display for MCP tools', async () => { const rules = buildPermissionRules({ toolName: 'mcp__puppeteer__navigate', }); diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index f10d1d4a3..e17fac881 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -14,6 +14,8 @@ import { import type { PathMatchContext } from './rule-parser.js'; import { extractShellOperations } from './shell-semantics.js'; import type { ShellOperation } from './shell-semantics.js'; +import { isShellCommandReadOnlyAST } from '../utils/shellAstParser.js'; +import { detectCommandSubstitution } from '../utils/shell-utils.js'; import type { PermissionCheckContext, PermissionDecision, @@ -153,12 +155,12 @@ export class PermissionManager { * @param ctx - The context containing the tool name and optional command. * @returns A PermissionDecision indicating how to handle this tool call. */ - evaluate(ctx: PermissionCheckContext): PermissionDecision { - const { command } = ctx; + async evaluate(ctx: PermissionCheckContext): Promise { + const { command, toolName } = ctx; // For shell commands, split compound commands and evaluate each // sub-command independently, then return the most restrictive result. - // Priority order (most to least restrictive): deny > ask > default > allow + // Priority order (most to least restrictive): deny > ask > allow if (command !== undefined) { const subCommands = splitCompoundCommand(command); if (subCommands.length > 1) { @@ -166,7 +168,20 @@ export class PermissionManager { } } - return this.evaluateSingle(ctx); + const decision = this.evaluateSingle(ctx); + + // For shell commands, resolve 'default' to actual permission using AST analysis + // This ensures 'default' is never returned for shell commands - they always get + // a concrete permission (deny/ask/allow) based on the command's readonly status. + if ( + decision === 'default' && + toolName === 'run_shell_command' && + command !== undefined + ) { + return this.resolveDefaultPermission(command); + } + + return decision; } /** @@ -295,32 +310,46 @@ export class PermissionManager { * Evaluate a compound command by splitting it into sub-commands, * evaluating each independently, and returning the most restrictive result. * - * Restriction order: deny > ask > default > allow + * Restriction order: deny > ask > allow * - * Example: with rules `allow: [safe-cmd *, one-cmd *]` - * - "safe-cmd && one-cmd" → both allow → allow - * - "safe-cmd && two-cmd" → allow + default → default - * - "safe-cmd && evil-cmd" (deny: [evil-cmd]) → allow + deny → deny + * When a sub-command returns 'default' (no rule matches), it is resolved to + * the actual default permission using AST analysis: + * - Command substitution detected → 'deny' + * - Read-only command (cd, ls, git status, etc.) → 'allow' + * - Otherwise → 'ask' + * + * Example: with rules `allow: [git checkout *]` + * - "cd /path && git checkout -b feature" → allow (cd) + allow (rule) → allow + * - "rm /path && git checkout -b feature" → ask (rm) + allow (rule) → ask + * - "evil-cmd && git checkout" (deny: [evil-cmd]) → deny + allow → deny */ - private evaluateCompoundCommand( + private async evaluateCompoundCommand( ctx: PermissionCheckContext, subCommands: string[], - ): PermissionDecision { - const PRIORITY: Record = { + ): Promise { + // Type for resolved decisions (excludes 'default' since it's resolved) + type ResolvedDecision = 'allow' | 'ask' | 'deny'; + const PRIORITY: Record = { deny: 3, ask: 2, - default: 1, allow: 0, }; - let mostRestrictive: PermissionDecision = 'allow'; + let mostRestrictive: ResolvedDecision = 'allow'; for (const subCmd of subCommands) { const subCtx: PermissionCheckContext = { ...ctx, command: subCmd, }; - const decision = this.evaluateSingle(subCtx); + const rawDecision = this.evaluateSingle(subCtx); + + // Resolve 'default' to actual permission using AST analysis + // (same logic as ShellToolInvocation.getDefaultPermission) + const decision: ResolvedDecision = + rawDecision === 'default' + ? await this.resolveDefaultPermission(subCmd) + : (rawDecision as ResolvedDecision); if (PRIORITY[decision] > PRIORITY[mostRestrictive]) { mostRestrictive = decision; @@ -335,6 +364,34 @@ export class PermissionManager { return mostRestrictive; } + /** + * Resolve 'default' permission to actual permission using AST analysis. + * This mirrors the logic in ShellToolInvocation.getDefaultPermission(). + * + * @param command - The shell command to analyze. + * @returns 'deny' for command substitution, 'allow' for read-only, 'ask' otherwise. + */ + private async resolveDefaultPermission( + command: string, + ): Promise<'allow' | 'ask' | 'deny'> { + // Security: command substitution ($(), ``, <(), >()) → deny + if (detectCommandSubstitution(command)) { + return 'deny'; + } + + // AST-based read-only detection + try { + const isReadOnly = await isShellCommandReadOnlyAST(command); + if (isReadOnly) { + return 'allow'; + } + } catch { + // AST check failed, fall back to 'ask' + } + + return 'ask'; + } + // --------------------------------------------------------------------------- // Registry-level helper // --------------------------------------------------------------------------- @@ -347,7 +404,7 @@ export class PermissionManager { * `"Bash(rm -rf *)"` do NOT remove the tool from the registry – they only * deny specific invocations at runtime. */ - isToolEnabled(toolName: string): boolean { + async isToolEnabled(toolName: string): Promise { const canonicalName = resolveToolName(toolName); // If a coreTools allowlist is active, only explicitly listed tools are @@ -361,7 +418,7 @@ export class PermissionManager { // evaluate({ toolName }) without a command will only match rules that have // no specifier, which is the correct registry-level check. - const decision = this.evaluate({ toolName: canonicalName }); + const decision = await this.evaluate({ toolName: canonicalName }); return decision !== 'deny'; } @@ -412,7 +469,7 @@ export class PermissionManager { * @param command - The shell command to evaluate. * @returns The PermissionDecision for this command. */ - isCommandAllowed(command: string): PermissionDecision { + async isCommandAllowed(command: string): Promise { return this.evaluate({ toolName: 'run_shell_command', command, @@ -447,6 +504,15 @@ export class PermissionManager { hasRelevantRules(ctx: PermissionCheckContext): boolean { const { toolName, command, filePath, domain, specifier } = ctx; + if (ctx.toolName === 'run_shell_command' && command !== undefined) { + const subCommands = splitCompoundCommand(command); + if (subCommands.length > 1) { + return subCommands.some((subCmd) => + this.hasRelevantRules({ ...ctx, command: subCmd }), + ); + } + } + const pathCtx: PathMatchContext | undefined = this.config.getProjectRoot && this.config.getCwd ? { @@ -502,6 +568,69 @@ export class PermissionManager { return false; } + /** + * Returns true when the invocation is matched by an explicit `ask` rule. + * + * This is intentionally narrower than `evaluate(ctx) === 'ask'`. Shell + * commands can resolve to `ask` simply because they are non-read-only and no + * explicit allow/deny rule matched. That fallback should still allow users to + * create new allow rules, so callers must only hide "Always allow" when a + * real ask rule matched. + */ + hasMatchingAskRule(ctx: PermissionCheckContext): boolean { + const { toolName, command, filePath, domain, specifier } = ctx; + + if (ctx.toolName === 'run_shell_command' && command !== undefined) { + const subCommands = splitCompoundCommand(command); + if (subCommands.length > 1) { + return subCommands.some((subCmd) => + this.hasMatchingAskRule({ ...ctx, command: subCmd }), + ); + } + } + + const pathCtx: PathMatchContext | undefined = + this.config.getProjectRoot && this.config.getCwd + ? { + projectRoot: this.config.getProjectRoot(), + cwd: this.config.getCwd(), + } + : undefined; + + const matchArgs = [ + toolName, + command, + filePath, + domain, + pathCtx, + specifier, + ] as const; + + const askRules = [...this.sessionRules.ask, ...this.persistentRules.ask]; + + if (askRules.some((rule) => matchesRule(rule, ...matchArgs))) { + return true; + } + + if (ctx.toolName === 'run_shell_command' && ctx.command !== undefined) { + const cwd = pathCtx?.cwd ?? process.cwd(); + const ops = extractShellOperations(ctx.command, cwd); + return ops.some((op) => { + const opMatchArgs = [ + op.virtualTool, + undefined, + op.filePath, + op.domain, + pathCtx, + undefined, + ] as const; + return askRules.some((rule) => matchesRule(rule, ...opMatchArgs)); + }); + } + + return false; + } + // --------------------------------------------------------------------------- // Session rule management // --------------------------------------------------------------------------- diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 7811a96ed..04d4293e8 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -10,6 +10,8 @@ import { StandardFileSystemService, needsUtf8Bom, resetUtf8BomCache, + detectLineEnding, + ensureCrlfLineEndings, } from './fileSystemService.js'; const mockPlatform = vi.hoisted(() => vi.fn().mockReturnValue('linux')); @@ -448,4 +450,144 @@ describe('StandardFileSystemService', () => { expect(needsUtf8Bom('/test/script.ps1')).toBe(true); }); }); + + describe('detectLineEnding', () => { + it('should detect CRLF line endings', () => { + expect(detectLineEnding('line1\r\nline2\r\n')).toBe('crlf'); + }); + + it('should detect LF line endings', () => { + expect(detectLineEnding('line1\nline2\n')).toBe('lf'); + }); + + it('should return lf for content with no line endings', () => { + expect(detectLineEnding('single line')).toBe('lf'); + }); + + it('should return lf for empty content', () => { + expect(detectLineEnding('')).toBe('lf'); + }); + + it('should detect CRLF even in mixed content', () => { + expect(detectLineEnding('line1\r\nline2\nline3')).toBe('crlf'); + }); + }); + + describe('ensureCrlfLineEndings', () => { + it('should convert LF to CRLF', () => { + expect(ensureCrlfLineEndings('line1\nline2\n')).toBe( + 'line1\r\nline2\r\n', + ); + }); + + it('should not double-convert existing CRLF', () => { + expect(ensureCrlfLineEndings('line1\r\nline2\r\n')).toBe( + 'line1\r\nline2\r\n', + ); + }); + + it('should handle mixed line endings', () => { + expect(ensureCrlfLineEndings('line1\r\nline2\nline3\r\n')).toBe( + 'line1\r\nline2\r\nline3\r\n', + ); + }); + + it('should handle content with no line endings', () => { + expect(ensureCrlfLineEndings('single line')).toBe('single line'); + }); + }); + + describe('writeTextFile with lineEnding preservation', () => { + it('should convert LF to CRLF when lineEnding is crlf', async () => { + vi.mocked(fs.writeFile).mockResolvedValue(); + + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'line1\nline2\n', + _meta: { lineEnding: 'crlf' }, + }); + + expect(fs.writeFile).toHaveBeenCalledWith( + '/test/file.txt', + 'line1\r\nline2\r\n', + 'utf-8', + ); + }); + + it('should not convert line endings when lineEnding is lf', async () => { + vi.mocked(fs.writeFile).mockResolvedValue(); + + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'line1\nline2\n', + _meta: { lineEnding: 'lf' }, + }); + + expect(fs.writeFile).toHaveBeenCalledWith( + '/test/file.txt', + 'line1\nline2\n', + 'utf-8', + ); + }); + + it('should not convert line endings when lineEnding is not specified', async () => { + vi.mocked(fs.writeFile).mockResolvedValue(); + + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'line1\nline2\n', + }); + + expect(fs.writeFile).toHaveBeenCalledWith( + '/test/file.txt', + 'line1\nline2\n', + 'utf-8', + ); + }); + + it('should preserve CRLF for non-bat files on non-Windows when lineEnding is crlf', async () => { + mockPlatform.mockReturnValue('linux'); + vi.mocked(fs.writeFile).mockResolvedValue(); + + await fileSystem.writeTextFile({ + path: '/test/file.cs', + content: 'using System;\nclass Foo {}\n', + _meta: { lineEnding: 'crlf' }, + }); + + expect(fs.writeFile).toHaveBeenCalledWith( + '/test/file.cs', + 'using System;\r\nclass Foo {}\r\n', + 'utf-8', + ); + }); + }); + + describe('readTextFile with lineEnding detection', () => { + it('should detect CRLF line ending in file content', async () => { + vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ + content: 'line1\r\nline2\r\n', + bom: false, + encoding: 'utf-8', + originalLineCount: 3, + }); + + const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); + + expect(result._meta?.lineEnding).toBe('crlf'); + }); + + it('should detect LF line ending in file content', async () => { + vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ + content: 'line1\nline2\n', + bom: false, + encoding: 'utf-8', + originalLineCount: 3, + }); + + const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); + + expect(result._meta?.lineEnding).toBe('lf'); + }); + }); }); diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 6d2022c75..e17d9288b 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -21,12 +21,15 @@ import type { WriteTextFileResponse, } from '@agentclientprotocol/sdk'; +export type LineEnding = 'crlf' | 'lf'; + export type ReadTextFileResponse = { content: string; _meta?: { bom?: boolean; encoding?: string; originalLineCount?: number; + lineEnding?: LineEnding; }; }; @@ -148,10 +151,20 @@ function needsCrlfLineEndings(filePath: string): boolean { /** * Ensures content uses CRLF line endings. First normalizes any existing - * \r\n to \n to avoid double-conversion, then converts all \n to \r\n. + * CRLF to LF to avoid double-conversion, then converts all LF to CRLF. */ -function ensureCrlfLineEndings(content: string): string { - return content.replace(/\r\n/g, '\n').replace(/\n/g, '\r\n'); +export function ensureCrlfLineEndings(content: string): string { + // First normalize CRLF to LF to avoid double-conversion, then convert all LF to CRLF + return content.split('\r\n').join('\n').split('\n').join('\r\n'); +} + +/** + * Detects whether the content uses CRLF or LF line endings. + * Returns 'crlf' if the content contains at least one CRLF sequence, + * 'lf' otherwise (including for content with no line endings). + */ +export function detectLineEnding(content: string): LineEnding { + return content.includes('\r\n') ? 'crlf' : 'lf'; } /** @@ -194,15 +207,21 @@ export class StandardFileSystemService implements FileSystemService { limit: limit ?? Number.POSITIVE_INFINITY, line: line || 0, }); - return { content, _meta: { bom, encoding, originalLineCount } }; + const lineEnding = detectLineEnding(content); + return { content, _meta: { bom, encoding, originalLineCount, lineEnding } }; } async writeTextFile( params: Omit, ): Promise { const { path: filePath, _meta } = params; - // Convert LF to CRLF for file types that require it (e.g. .bat, .cmd) - const content = needsCrlfLineEndings(filePath) + const lineEnding = _meta?.['lineEnding'] as string | undefined; + // Convert LF to CRLF when: + // 1. The file type requires it (e.g. .bat, .cmd on Windows), OR + // 2. The original file used CRLF line endings (preserve original style) + const shouldUseCrlf = + needsCrlfLineEndings(filePath) || lineEnding === 'crlf'; + const content = shouldUseCrlf ? ensureCrlfLineEndings(params.content) : params.content; const bom = _meta?.['bom'] ?? (false as boolean); diff --git a/packages/core/src/skills/bundled/qc-helper/SKILL.md b/packages/core/src/skills/bundled/qc-helper/SKILL.md new file mode 100644 index 000000000..14fbaa152 --- /dev/null +++ b/packages/core/src/skills/bundled/qc-helper/SKILL.md @@ -0,0 +1,151 @@ +--- +name: qc-helper +description: Answer any question about Qwen Code usage, features, configuration, and troubleshooting by referencing the official user documentation. Also helps users view or modify their settings.json. Invoke with `/qc-helper` followed by a question, e.g. `/qc-helper how do I configure MCP servers?` or `/qc-helper change approval mode to yolo`. +allowedTools: + - read_file + - edit_file + - grep_search + - glob + - read_many_files +--- + +# Qwen Code Helper + +You are a helpful assistant for **Qwen Code** — an AI coding agent for the terminal. Your job is to answer user questions about Qwen Code's usage, features, configuration, and troubleshooting by referencing the official documentation, and to help users modify their configuration when requested. + +## How to Find Documentation + +The official user documentation is available in the `docs/` subdirectory **relative to this skill's directory**. Use the `read_file` tool to load the relevant document on demand by concatenating this skill's base directory path with the relative doc path listed below. + +> **Example**: If the user asks about MCP servers, read `docs/features/mcp.md` (relative to this skill's directory). + +--- + +## Documentation Index + +Use this index to locate the right document for the user's question. Load only the docs that are relevant — do not read everything at once. + +### Getting Started + +| Topic | Doc Path | +| ----------------- | ------------------------- | +| Product overview | `docs/overview.md` | +| Quick start guide | `docs/quickstart.md` | +| Common workflows | `docs/common-workflow.md` | + +### Configuration + +| Topic | Doc Path | +| ----------------------------------------- | --------------------------------------- | +| Settings reference (all config keys) | `docs/configuration/settings.md` | +| Authentication setup | `docs/configuration/auth.md` | +| Model providers (OpenAI-compatible, etc.) | `docs/configuration/model-providers.md` | +| .qwenignore file | `docs/configuration/qwen-ignore.md` | +| Themes | `docs/configuration/themes.md` | +| Memory | `docs/configuration/memory.md` | +| Trusted folders | `docs/configuration/trusted-folders.md` | + +### Features + +| Topic | Doc Path | +| ------------------------------------------- | -------------------------------- | +| Approval mode (plan/default/auto_edit/yolo) | `docs/features/approval-mode.md` | +| MCP (Model Context Protocol) | `docs/features/mcp.md` | +| Skills system | `docs/features/skills.md` | +| Sub-agents | `docs/features/sub-agents.md` | +| Sandbox / security | `docs/features/sandbox.md` | +| Slash commands | `docs/features/commands.md` | +| Headless / non-interactive mode | `docs/features/headless.md` | +| LSP integration | `docs/features/lsp.md` | +| Checkpointing | `docs/features/checkpointing.md` | +| Token caching | `docs/features/token-caching.md` | +| Language / i18n | `docs/features/language.md` | +| Arena mode | `docs/features/arena.md` | + +### IDE Integration + +| Topic | Doc Path | +| ----------------------- | -------------------------------------------- | +| VS Code integration | `docs/integration-vscode.md` | +| Zed IDE integration | `docs/integration-zed.md` | +| JetBrains integration | `docs/integration-jetbrains.md` | +| GitHub Actions | `docs/integration-github-action.md` | +| IDE companion spec | `docs/ide-integration/ide-companion-spec.md` | +| IDE integration details | `docs/ide-integration/ide-integration.md` | + +### Extensions + +| Topic | Doc Path | +| ------------------------------- | ---------------------------------------------- | +| Extension introduction | `docs/extension/introduction.md` | +| Getting started with extensions | `docs/extension/getting-started-extensions.md` | +| Releasing extensions | `docs/extension/extension-releasing.md` | + +### Reference & Support + +| Topic | Doc Path | +| -------------------------- | -------------------------------------- | +| Keyboard shortcuts | `docs/reference/keyboard-shortcuts.md` | +| Troubleshooting | `docs/support/troubleshooting.md` | +| Uninstall guide | `docs/support/Uninstall.md` | +| Terms of service & privacy | `docs/support/tos-privacy.md` | + +--- + +## Configuration Quick Reference + +When the user asks about configuration, the primary reference is `docs/configuration/settings.md`. Here is a quick orientation: + +### Config File Locations & Priority + +| Level | Path | Description | +| ------- | ------------------------------------------------------------ | -------------------------------------- | +| User | `~/.qwen/settings.json` | Personal global config | +| Project | `/.qwen/settings.json` | Project-specific, overrides user level | +| System | macOS: `/Library/Application Support/QwenCode/settings.json` | Admin-level config | + +**Priority** (highest to lowest): CLI args > env vars > system settings > project settings > user settings > defaults + +**Format**: JSON with Comments (supports `//` and `/* */`), with environment variable interpolation (`$VAR` or `${VAR}`) + +### Common Config Categories + +| Category | Key Config Keys | Reference | +| ------------- | ----------------------------------------------------------------------------- | ------------------------------------------------------------------------- | +| Permissions | `permissions.allow/ask/deny` | `docs/configuration/settings.md`, `docs/features/approval-mode.md` | +| MCP Servers | `mcpServers.*`, `mcp.*` | `docs/configuration/settings.md`, `docs/features/mcp.md` | +| Tool Approval | `tools.approvalMode` | `docs/configuration/settings.md`, `docs/features/approval-mode.md` | +| Model | `model.name`, `modelProviders` | `docs/configuration/settings.md`, `docs/configuration/model-providers.md` | +| General/UI | `general.*`, `ui.*`, `ide.*`, `output.*` | `docs/configuration/settings.md` | +| Context | `context.*` | `docs/configuration/settings.md` | +| Advanced | `hooks`, `env`, `webSearch`, `security`, `privacy`, `telemetry`, `advanced.*` | `docs/configuration/settings.md` | + +--- + +## Workflow + +### Answering Questions + +1. **Identify the topic** from the user's question using the Documentation Index above +2. **Use `read_file`** to load the relevant doc(s) — only load what you need +3. **Provide a clear, concise answer** grounded in the documentation content +4. If the docs don't cover the question, say so honestly and suggest where to look + +### Helping with Configuration Changes + +When the user wants to modify their configuration: + +1. **Read the relevant doc** to understand the config key, its type, allowed values, and defaults +2. **Ask which config level** to modify if not specified: user (`~/.qwen/settings.json`) or project (`.qwen/settings.json`) +3. **Use `read_file`** to check the current content of the target settings file +4. **Use `edit_file`** to apply the change with correct JSON syntax +5. **After every configuration change**, you MUST remind the user: + +> **Note: Most configuration changes require restarting Qwen Code (`/exit` then re-launch) to take effect.** Only a few settings (like `permissions`) are picked up dynamically. + +### Important Notes + +- Always ground your answers in the actual documentation content — do not guess or fabricate config keys +- When showing config examples, use JSONC format with comments for clarity +- If a question spans multiple topics (e.g., "How do I set up MCP with sandbox?"), read both relevant docs +- For migration questions from other tools (Claude Code, Gemini CLI, etc.), check `docs/configuration/settings.md` for equivalent config keys diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index c67520385..71ff12209 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -922,5 +922,48 @@ describe('EditTool', () => { expect(params.old_string).toBe(initialContent); expect(params.new_string).toBe(modifiedContent); }); + + it('should not call ideClient.openDiff in AUTO_EDIT mode', async () => { + const initialContent = 'some old content here'; + fs.writeFileSync(filePath, initialContent); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.AUTO_EDIT, + ); + + const invocation = tool.build(params); + const confirmation = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + + expect(ideClient.openDiff).not.toHaveBeenCalled(); + expect(confirmation).toBeDefined(); + expect((confirmation as any).ideConfirmation).toBeUndefined(); + }); + + it('should not call ideClient.openDiff in YOLO mode', async () => { + const initialContent = 'some old content here'; + fs.writeFileSync(filePath, initialContent); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + (mockConfig.getApprovalMode as Mock).mockReturnValueOnce( + ApprovalMode.YOLO, + ); + + const invocation = tool.build(params); + const confirmation = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + + expect(ideClient.openDiff).not.toHaveBeenCalled(); + expect((confirmation as any).ideConfirmation).toBeUndefined(); + }); }); }); diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index e5b1480b9..e41ec3a55 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -21,7 +21,12 @@ import { makeRelative, shortenPath } from '../utils/paths.js'; import { isNodeError } from '../utils/errors.js'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../config/config.js'; -import { FileEncoding, needsUtf8Bom } from '../services/fileSystemService.js'; +import { + FileEncoding, + needsUtf8Bom, + detectLineEnding, +} from '../services/fileSystemService.js'; +import type { LineEnding } from '../services/fileSystemService.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; import { ReadFileTool } from './read-file.js'; import { ToolNames, ToolDisplayNames } from './tool-names.js'; @@ -113,6 +118,8 @@ interface CalculatedEdit { encoding: string; /** Whether the existing file has a UTF-8 BOM */ bom: boolean; + /** Original line ending style of the existing file */ + lineEnding: LineEnding; } class EditToolInvocation implements ToolInvocation { @@ -144,6 +151,7 @@ class EditToolInvocation implements ToolInvocation { | undefined = undefined; let useBOM = false; let detectedEncoding = 'utf-8'; + let detectedLineEnding: LineEnding = 'lf'; if (fileExists) { try { const fileInfo = await this.config @@ -157,6 +165,8 @@ class EditToolInvocation implements ToolInvocation { fileInfo.content.codePointAt(0) === 0xfeff; } detectedEncoding = fileInfo._meta?.encoding || 'utf-8'; + // Detect original line ending style before normalizing + detectedLineEnding = detectLineEnding(fileInfo.content); // Normalize line endings to LF for consistent processing. currentContent = fileInfo.content.replace(/\r\n/g, '\n'); fileExists = true; @@ -257,6 +267,7 @@ class EditToolInvocation implements ToolInvocation { isNewFile, bom: useBOM, encoding: detectedEncoding, + lineEnding: detectedLineEnding, }; } @@ -297,9 +308,13 @@ class EditToolInvocation implements ToolInvocation { 'Proposed', DEFAULT_DIFF_OPTIONS, ); + const approvalMode = this.config.getApprovalMode(); const ideClient = await IdeClient.getInstance(); const ideConfirmation = - this.config.getIdeMode() && ideClient.isDiffingEnabled() + this.config.getIdeMode() && + ideClient.isDiffingEnabled() && + approvalMode !== ApprovalMode.AUTO_EDIT && + approvalMode !== ApprovalMode.YOLO ? ideClient.openDiff(this.params.file_path, editData.newContent) : undefined; @@ -414,6 +429,7 @@ class EditToolInvocation implements ToolInvocation { _meta: { bom: editData.bom, encoding: editData.encoding, + lineEnding: editData.lineEnding, }, }); } diff --git a/packages/core/src/tools/glob.test.ts b/packages/core/src/tools/glob.test.ts index 24be79d2e..67769a6e9 100644 --- a/packages/core/src/tools/glob.test.ts +++ b/packages/core/src/tools/glob.test.ts @@ -491,6 +491,44 @@ describe('GlobTool', () => { expect(result.llmContent).toContain('Found 3 file(s)'); // fileA.txt, FileB.TXT, b.notignored.txt expect(result.llmContent).not.toContain('a.qwenignored.txt'); }); + + it('should respect .gitignore when searching a subdirectory (path option)', async () => { + // This tests the regression fix: relativePaths must be computed relative + // to projectRoot, not to searchDir, so that gitignore rules rooted at + // projectRoot are evaluated against the correct paths. + await fs.writeFile(path.join(tempRootDir, '.gitignore'), '*.secret'); + await fs.writeFile(path.join(tempRootDir, 'sub', 'visible.txt'), 'ok'); + await fs.writeFile( + path.join(tempRootDir, 'sub', 'hidden.secret'), + 'should be ignored', + ); + + const subDirTool = new GlobTool(mockConfig); + const params: GlobToolParams = { pattern: '*', path: 'sub' }; + const invocation = subDirTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('visible.txt'); + expect(result.llmContent).not.toContain('hidden.secret'); + }); + + it('should respect .qwenignore when searching a subdirectory (path option)', async () => { + await fs.writeFile(path.join(tempRootDir, '.qwenignore'), '*.secret'); + await fs.writeFile(path.join(tempRootDir, 'sub', 'visible.txt'), 'ok'); + await fs.writeFile( + path.join(tempRootDir, 'sub', 'hidden.secret'), + 'should be ignored', + ); + + // Recreate to pick up .qwenignore + const subDirTool = new GlobTool(mockConfig); + const params: GlobToolParams = { pattern: '*', path: 'sub' }; + const invocation = subDirTool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('visible.txt'); + expect(result.llmContent).not.toContain('hidden.secret'); + }); }); describe('file count truncation', () => { diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 44edae4e6..ab6b6d80a 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -144,11 +144,13 @@ class GlobToolInvocation extends BaseToolInvocation< signal, })) as GlobPath[]; - // Filter using paths relative to the search directory so that - // .gitignore / .qwenignore patterns match correctly regardless of - // which workspace directory the file belongs to. + // Filter using paths relative to the project root (the base that + // FileDiscoveryService uses for .gitignore / .qwenignore evaluation). + // Using searchDir-relative paths would cause ignore rules to be + // evaluated against incorrect paths when searchDir != projectRoot. + const projectRoot = this.config.getTargetDir(); const relativePaths = entries.map((p) => - path.relative(searchDir, p.fullpath()), + path.relative(projectRoot, p.fullpath()), ); const { filteredPaths } = this.fileService.filterFilesWithReport( @@ -163,7 +165,7 @@ class GlobToolInvocation extends BaseToolInvocation< const filteredAbsolutePaths = new Set( filteredPaths.map((p) => - normalizePathForComparison(path.resolve(searchDir, p)), + normalizePathForComparison(path.resolve(projectRoot, p)), ), ); diff --git a/packages/core/src/tools/grep.test.ts b/packages/core/src/tools/grep.test.ts index 868cecd78..4af6d8d7c 100644 --- a/packages/core/src/tools/grep.test.ts +++ b/packages/core/src/tools/grep.test.ts @@ -399,6 +399,33 @@ describe('GrepTool', () => { await fs.rm(secondDir, { recursive: true, force: true }); }); + + it('should deduplicate matches from overlapping workspace directories', async () => { + // This tests the fix: when workspace dirs overlap (parent + child), + // the same file should appear only once in the results. + const subDir = path.join(tempRootDir, 'sub'); + + const multiDirConfig = { + getTargetDir: () => tempRootDir, + getWorkspaceContext: () => + createMockWorkspaceContext(tempRootDir, [subDir]), + getFileExclusions: () => ({ + getGlobExcludes: () => [], + }), + getTruncateToolOutputThreshold: () => 25000, + getTruncateToolOutputLines: () => 1000, + } as unknown as Config; + + const multiDirGrepTool = new GrepTool(multiDirConfig); + // 'sub dir' exists only in sub/fileC.txt — a file that lives under both + // tempRootDir and subDir, so without deduplication it would appear twice. + const params: GrepToolParams = { pattern: 'sub dir' }; + const invocation = multiDirGrepTool.build(params); + const result = await invocation.execute(abortSignal); + + // sub/fileC.txt (or its absolute path equivalent) should appear only once + expect(result.llmContent).toContain('Found 1 match'); + }); }); describe('getDescription', () => { diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 6e16348d9..4f927a167 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -121,7 +121,7 @@ class GrepToolInvocation extends BaseToolInvocation< } // Perform grep search across all directories - const rawMatches: GrepMatch[] = []; + let rawMatches: GrepMatch[] = []; for (const searchDir of searchDirs) { const matches = await this.performGrepSearch({ pattern: this.params.pattern, @@ -142,6 +142,18 @@ class GrepToolInvocation extends BaseToolInvocation< rawMatches.push(...matches); } + // Deduplicate matches that might appear from overlapping workspace + // directories (e.g. parent + child both in workspace dirs). + if (searchDirs.length > 1) { + const seen = new Set(); + rawMatches = rawMatches.filter((match) => { + const key = `${match.filePath}:${match.lineNumber}`; + if (seen.has(key)) return false; + seen.add(key); + return true; + }); + } + const filterDescription = this.params.glob ? ` (filter: "${this.params.glob}")` : ''; diff --git a/packages/core/src/tools/ripGrep.test.ts b/packages/core/src/tools/ripGrep.test.ts index 5edbc680a..118ed9791 100644 --- a/packages/core/src/tools/ripGrep.test.ts +++ b/packages/core/src/tools/ripGrep.test.ts @@ -544,6 +544,36 @@ describe('RipGrepTool', () => { await fs.rm(secondDir, { recursive: true, force: true }); }); + + it('should deduplicate matches from overlapping workspace directories', async () => { + // This tests the fix: when ripgrep receives overlapping search paths + // (e.g. /parent and /parent/sub), it may report the same file twice. + // The deduplication layer must remove duplicates. + const subDir = path.join(tempRootDir, 'sub'); + + const multiDirConfig = { + ...mockConfig, + getWorkspaceContext: () => + createMockWorkspaceContext(tempRootDir, [subDir]), + } as unknown as Config; + + const multiDirGrepTool = new RipGrepTool(multiDirConfig); + + // Simulate ripgrep returning the same file:line twice (once from each search root) + const dupLine = `${path.join(subDir, 'fileC.txt')}:1:hello world`; + (runRipgrep as Mock).mockResolvedValue({ + stdout: `${dupLine}${EOL}${dupLine}${EOL}`, + truncated: false, + error: undefined, + }); + + const params: RipGrepToolParams = { pattern: 'hello' }; + const invocation = multiDirGrepTool.build(params); + const result = await invocation.execute(abortSignal); + + // Despite two identical lines in the raw output, only 1 match should be reported. + expect(result.llmContent).toContain('Found 1 match'); + }); }); describe('abort signal handling', () => { diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 19a98af80..a023d9d61 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -106,7 +106,27 @@ class GrepToolInvocation extends BaseToolInvocation< } // Split into lines and count total matches - const allLines = rawOutput.split('\n').filter((line) => line.trim()); + let allLines = rawOutput.split('\n').filter((line) => line.trim()); + + // Deduplicate lines from potentially overlapping workspace directories. + // ripgrep reports the same file twice when given paths like /a and /a/sub. + if (searchPaths.length > 1) { + const seen = new Set(); + allLines = allLines.filter((line) => { + // ripgrep output format: filepath:linenum:content + const firstColon = line.indexOf(':'); + if (firstColon !== -1) { + const secondColon = line.indexOf(':', firstColon + 1); + if (secondColon !== -1) { + const key = line.substring(0, secondColon); + if (seen.has(key)) return false; + seen.add(key); + } + } + return true; + }); + } + const totalMatches = allLines.length; const matchTerm = totalMatches === 1 ? 'match' : 'matches'; diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 73047dbea..a8748e375 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -37,6 +37,7 @@ import * as crypto from 'node:crypto'; import { ToolErrorType } from './tool-error.js'; import { OUTPUT_UPDATE_INTERVAL_MS } from './shell.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; +import { PermissionManager } from '../permissions/permission-manager.js'; describe('ShellTool', () => { let shellTool: ShellTool; @@ -49,6 +50,8 @@ describe('ShellTool', () => { mockConfig = { getCoreTools: vi.fn().mockReturnValue([]), + getPermissionsAllow: vi.fn().mockReturnValue([]), + getPermissionsAsk: vi.fn().mockReturnValue([]), getPermissionsDeny: vi.fn().mockReturnValue([]), getDebugMode: vi.fn().mockReturnValue(false), getTargetDir: vi.fn().mockReturnValue('/test/dir'), @@ -61,6 +64,7 @@ describe('ShellTool', () => { }, getTruncateToolOutputThreshold: vi.fn().mockReturnValue(0), getTruncateToolOutputLines: vi.fn().mockReturnValue(0), + getPermissionManager: vi.fn().mockReturnValue(undefined), getGeminiClient: vi.fn(), getGitCoAuthor: vi.fn().mockReturnValue({ enabled: true, @@ -91,21 +95,21 @@ describe('ShellTool', () => { }); describe('isCommandAllowed', () => { - it('should allow a command if no restrictions are provided', () => { + it('should allow a command if no restrictions are provided', async () => { (mockConfig.getCoreTools as Mock).mockReturnValue(undefined); (mockConfig.getPermissionsDeny as Mock).mockReturnValue(undefined); - expect(isCommandAllowed('ls -l', mockConfig).allowed).toBe(true); + expect((await isCommandAllowed('ls -l', mockConfig)).allowed).toBe(true); }); - it('should block a command with command substitution using $()', () => { - expect(isCommandAllowed('echo $(rm -rf /)', mockConfig).allowed).toBe( - false, - ); + it('should block a command with command substitution using $()', async () => { + expect( + (await isCommandAllowed('echo $(rm -rf /)', mockConfig)).allowed, + ).toBe(false); }); }); describe('build', () => { - it('should return an invocation for a valid command', () => { + it('should return an invocation for a valid command', async () => { const invocation = shellTool.build({ command: 'ls -l', is_background: false, @@ -113,13 +117,13 @@ describe('ShellTool', () => { expect(invocation).toBeDefined(); }); - it('should throw an error for an empty command', () => { + it('should throw an error for an empty command', async () => { expect(() => shellTool.build({ command: ' ', is_background: false }), ).toThrow('Command cannot be empty.'); }); - it('should throw an error for a relative directory path', () => { + it('should throw an error for a relative directory path', async () => { expect(() => shellTool.build({ command: 'ls', @@ -129,7 +133,7 @@ describe('ShellTool', () => { ).toThrow('Directory must be an absolute path.'); }); - it('should throw an error for a directory outside the workspace', () => { + it('should throw an error for a directory outside the workspace', async () => { (mockConfig.getWorkspaceContext as Mock).mockReturnValue( createMockWorkspaceContext('/test/dir', ['/another/workspace']), ); @@ -144,7 +148,7 @@ describe('ShellTool', () => { ); }); - it('should throw an error for a directory within the user skills directory', () => { + it('should throw an error for a directory within the user skills directory', async () => { expect(() => shellTool.build({ command: 'ls', @@ -156,7 +160,7 @@ describe('ShellTool', () => { ); }); - it('should throw an error for the user skills directory itself', () => { + it('should throw an error for the user skills directory itself', async () => { expect(() => shellTool.build({ command: 'ls', @@ -168,7 +172,7 @@ describe('ShellTool', () => { ); }); - it('should resolve directory path before checking user skills directory', () => { + it('should resolve directory path before checking user skills directory', async () => { expect(() => shellTool.build({ command: 'ls', @@ -180,7 +184,7 @@ describe('ShellTool', () => { ); }); - it('should return an invocation for a valid absolute directory path', () => { + it('should return an invocation for a valid absolute directory path', async () => { (mockConfig.getWorkspaceContext as Mock).mockReturnValue( createMockWorkspaceContext('/test/dir', ['/another/workspace']), ); @@ -192,7 +196,7 @@ describe('ShellTool', () => { expect(invocation).toBeDefined(); }); - it('should include background indicator in description when is_background is true', () => { + it('should include background indicator in description when is_background is true', async () => { const invocation = shellTool.build({ command: 'npm start', is_background: true, @@ -200,7 +204,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).toContain('[background]'); }); - it('should not include background indicator in description when is_background is false', () => { + it('should not include background indicator in description when is_background is false', async () => { const invocation = shellTool.build({ command: 'npm test', is_background: false, @@ -209,7 +213,7 @@ describe('ShellTool', () => { }); describe('is_background parameter coercion', () => { - it('should accept string "true" as boolean true', () => { + it('should accept string "true" as boolean true', async () => { const invocation = shellTool.build({ command: 'npm run dev', is_background: 'true' as unknown as boolean, @@ -218,7 +222,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).toContain('[background]'); }); - it('should accept string "false" as boolean false', () => { + it('should accept string "false" as boolean false', async () => { const invocation = shellTool.build({ command: 'npm run build', is_background: 'false' as unknown as boolean, @@ -227,7 +231,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).not.toContain('[background]'); }); - it('should accept string "True" as boolean true', () => { + it('should accept string "True" as boolean true', async () => { const invocation = shellTool.build({ command: 'npm run dev', is_background: 'True' as unknown as boolean, @@ -236,7 +240,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).toContain('[background]'); }); - it('should accept string "False" as boolean false', () => { + it('should accept string "False" as boolean false', async () => { const invocation = shellTool.build({ command: 'npm run build', is_background: 'False' as unknown as boolean, @@ -459,13 +463,13 @@ describe('ShellTool', () => { expect(result.error?.message).toBe('command failed'); }); - it('should throw an error for invalid parameters', () => { + it('should throw an error for invalid parameters', async () => { expect(() => shellTool.build({ command: '', is_background: false }), ).toThrow('Command cannot be empty.'); }); - it('should throw an error for invalid directory', () => { + it('should throw an error for invalid directory', async () => { expect(() => shellTool.build({ command: 'ls', @@ -967,7 +971,31 @@ describe('ShellTool', () => { expect(details.permissionRules).toEqual(['Bash(npm run *)']); }); - it('should throw an error if validation fails', () => { + it('should exclude already-allowed sub-commands from confirmation details in compound commands', async () => { + const pm = new PermissionManager({ + getPermissionsAllow: () => ['Bash(git add *)'], + getPermissionsAsk: () => [], + getPermissionsDeny: () => [], + getProjectRoot: () => '/test/dir', + getCwd: () => '/test/dir', + }); + pm.initialize(); + (mockConfig.getPermissionManager as Mock).mockReturnValue(pm); + + const invocation = shellTool.build({ + command: 'git add /tmp/file && git commit -m "msg"', + is_background: false, + }); + + const details = (await invocation.getConfirmationDetails( + new AbortController().signal, + )) as { rootCommand: string; permissionRules: string[] }; + + expect(details.rootCommand).toBe('git'); + expect(details.permissionRules).toEqual(['Bash(git commit *)']); + }); + + it('should throw an error if validation fails', async () => { expect(() => shellTool.build({ command: '', is_background: false }), ).toThrow(); @@ -975,13 +1003,13 @@ describe('ShellTool', () => { }); describe('getDescription', () => { - it('should return the windows description when on windows', () => { + it('should return the windows description when on windows', async () => { vi.mocked(os.platform).mockReturnValue('win32'); const shellTool = new ShellTool(mockConfig); expect(shellTool.description).toMatchSnapshot(); }); - it('should return the non-windows description when not on windows', () => { + it('should return the non-windows description when not on windows', async () => { vi.mocked(os.platform).mockReturnValue('linux'); const shellTool = new ShellTool(mockConfig); expect(shellTool.description).toMatchSnapshot(); @@ -1026,7 +1054,7 @@ describe('ShellTool', () => { }); describe('timeout parameter', () => { - it('should validate timeout parameter correctly', () => { + it('should validate timeout parameter correctly', async () => { // Valid timeout expect(() => { shellTool.build({ @@ -1091,7 +1119,7 @@ describe('ShellTool', () => { }).toThrow('params/timeout must be number'); }); - it('should include timeout in description for foreground commands', () => { + it('should include timeout in description for foreground commands', async () => { const invocation = shellTool.build({ command: 'npm test', is_background: false, @@ -1101,7 +1129,7 @@ describe('ShellTool', () => { expect(invocation.getDescription()).toBe('npm test [timeout: 30000ms]'); }); - it('should not include timeout in description for background commands', () => { + it('should not include timeout in description for background commands', async () => { const invocation = shellTool.build({ command: 'npm start', is_background: true, diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 3d38eaf4b..0300f7bec 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -37,7 +37,6 @@ import { getCommandRoots, splitCommands, stripShellWrapper, - detectCommandSubstitution, } from '../utils/shell-utils.js'; import { createDebugLogger } from '../utils/debugLogger.js'; import { @@ -92,18 +91,12 @@ export class ShellToolInvocation extends BaseToolInvocation< /** * AST-based permission check for the shell command. - * - Command substitution → 'deny' (security) * - Read-only commands (via AST analysis) → 'allow' * - All other commands → 'ask' */ override async getDefaultPermission(): Promise { const command = stripShellWrapper(this.params.command); - // Security: command substitution ($(), ``, <(), >()) → deny - if (detectCommandSubstitution(command)) { - return 'deny'; - } - // AST-based read-only detection try { const isReadOnly = await isShellCommandReadOnlyAST(command); @@ -127,25 +120,40 @@ export class ShellToolInvocation extends BaseToolInvocation< _abortSignal: AbortSignal, ): Promise { const command = stripShellWrapper(this.params.command); + const pm = this.config.getPermissionManager?.(); // Split compound command and filter out already-allowed (read-only) sub-commands const subCommands = splitCommands(command); - const nonReadOnlySubCommands: string[] = []; + const confirmableSubCommands: string[] = []; for (const sub of subCommands) { + let isReadOnly = false; try { - const isReadOnly = await isShellCommandReadOnlyAST(sub); - if (!isReadOnly) { - nonReadOnlySubCommands.push(sub); - } + isReadOnly = await isShellCommandReadOnlyAST(sub); } catch { - nonReadOnlySubCommands.push(sub); // conservative: include if check fails + // conservative: treat unknown commands as requiring confirmation } + + if (isReadOnly) { + continue; + } + + if (pm) { + try { + if ((await pm.isCommandAllowed(sub)) === 'allow') { + continue; + } + } catch (e) { + debugLogger.warn('PermissionManager command check failed:', e); + } + } + + confirmableSubCommands.push(sub); } // Fallback to all sub-commands if everything was filtered out (shouldn't // normally happen since getDefaultPermission already returned 'ask'). const effectiveSubCommands = - nonReadOnlySubCommands.length > 0 ? nonReadOnlySubCommands : subCommands; + confirmableSubCommands.length > 0 ? confirmableSubCommands : subCommands; const rootCommands = [ ...new Set( @@ -583,18 +591,10 @@ ${processGroupNote} } function getCommandDescription(): string { - const cmd_substitution_warning = - '\n*** WARNING: Command substitution using $(), `` ` ``, <(), or >() is not allowed for security reasons.'; if (os.platform() === 'win32') { - return ( - 'Exact command to execute as `cmd.exe /c `' + - cmd_substitution_warning - ); + return 'Exact command to execute as `cmd.exe /c `'; } else { - return ( - 'Exact bash command to execute as `bash -c `' + - cmd_substitution_warning - ); + return 'Exact bash command to execute as `bash -c `'; } } @@ -647,9 +647,9 @@ export class ShellTool extends BaseDeclarativeTool< protected override validateToolParamValues( params: ShellToolParams, ): string | null { - // NOTE: Permission checks (command substitution, read-only detection, PM rules) - // are now handled at L3 (getDefaultPermission) and L4 (PM override) in - // coreToolScheduler. This method only performs pure parameter validation. + // NOTE: Permission checks (read-only detection, PM rules) are handled at + // L3 (getDefaultPermission) and L4 (PM override) in coreToolScheduler. + // This method only performs pure parameter validation. if (!params.command.trim()) { return 'Command cannot be empty.'; } diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index f4808cdc0..407d187b1 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -321,6 +321,36 @@ describe('WriteFileTool', () => { expect(mockIdeClient.openDiff).not.toHaveBeenCalled(); }); + it('should not call openDiff in AUTO_EDIT mode', async () => { + mockConfigInternal.getApprovalMode.mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + const filePath = path.join(rootDir, 'ide_auto_edit_file.txt'); + const params = { file_path: filePath, content: 'test' }; + const invocation = tool.build(params); + + const confirmation = (await invocation.getConfirmationDetails( + abortSignal, + )) as ToolEditConfirmationDetails; + + expect(mockIdeClient.openDiff).not.toHaveBeenCalled(); + expect(confirmation.ideConfirmation).toBeUndefined(); + }); + + it('should not call openDiff in YOLO mode', async () => { + mockConfigInternal.getApprovalMode.mockReturnValue(ApprovalMode.YOLO); + const filePath = path.join(rootDir, 'ide_yolo_file.txt'); + const params = { file_path: filePath, content: 'test' }; + const invocation = tool.build(params); + + const confirmation = (await invocation.getConfirmationDetails( + abortSignal, + )) as ToolEditConfirmationDetails; + + expect(mockIdeClient.openDiff).not.toHaveBeenCalled(); + expect(confirmation.ideConfirmation).toBeUndefined(); + }); + it('should update params.content with IDE content when onConfirm is called', async () => { const filePath = path.join(rootDir, 'ide_onconfirm_file.txt'); const params = { file_path: filePath, content: 'original-content' }; @@ -740,7 +770,7 @@ describe('WriteFileTool', () => { expect(writeSpy).toHaveBeenCalledWith({ path: filePath, content: newContent, - _meta: { bom: true, encoding: 'utf-8' }, + _meta: { bom: true, encoding: 'utf-8', lineEnding: 'lf' }, }); // Cleanup @@ -768,7 +798,7 @@ describe('WriteFileTool', () => { expect(writeSpy).toHaveBeenCalledWith({ path: filePath, content: newContent, - _meta: { bom: false, encoding: 'utf-8' }, + _meta: { bom: false, encoding: 'utf-8', lineEnding: 'lf' }, }); // Cleanup diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 1f1a30cdd..caaa3aaa4 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -25,7 +25,12 @@ import { ToolConfirmationOutcome, } from './tools.js'; import { ToolErrorType } from './tool-error.js'; -import { FileEncoding, needsUtf8Bom } from '../services/fileSystemService.js'; +import { + FileEncoding, + needsUtf8Bom, + detectLineEnding, +} from '../services/fileSystemService.js'; +import type { LineEnding } from '../services/fileSystemService.js'; import { makeRelative, shortenPath } from '../utils/paths.js'; import { getErrorMessage, isNodeError } from '../utils/errors.js'; import { DEFAULT_DIFF_OPTIONS, getDiffStat } from './diffOptions.js'; @@ -138,9 +143,13 @@ class WriteFileToolInvocation extends BaseToolInvocation< DEFAULT_DIFF_OPTIONS, ); + const approvalMode = this.config.getApprovalMode(); const ideClient = await IdeClient.getInstance(); const ideConfirmation = - this.config.getIdeMode() && ideClient.isDiffingEnabled() + this.config.getIdeMode() && + ideClient.isDiffingEnabled() && + approvalMode !== ApprovalMode.AUTO_EDIT && + approvalMode !== ApprovalMode.YOLO ? ideClient.openDiff(this.params.file_path, this.params.content) : undefined; @@ -177,6 +186,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< let originalContent = ''; let useBOM = false; let detectedEncoding: string | undefined; + let detectedLineEnding: LineEnding | undefined; const dirName = path.dirname(file_path); if (fileExists) { try { @@ -191,6 +201,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< fileInfo.content.codePointAt(0) === 0xfeff; } detectedEncoding = fileInfo._meta?.encoding || 'utf-8'; + detectedLineEnding = detectLineEnding(fileInfo.content); originalContent = fileInfo.content; fileExists = true; // File exists and was read } catch (err) { @@ -239,6 +250,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< _meta: { bom: useBOM, encoding: detectedEncoding, + lineEnding: detectedLineEnding, }, }); diff --git a/packages/core/src/utils/openaiLogger.test.ts b/packages/core/src/utils/openaiLogger.test.ts index 9d3387e4b..4aa545e10 100644 --- a/packages/core/src/utils/openaiLogger.test.ts +++ b/packages/core/src/utils/openaiLogger.test.ts @@ -387,4 +387,86 @@ describe('OpenAILogger', () => { expect(logPath).toContain(specialPath); }); }); + + describe('cwd parameter', () => { + it('should use provided cwd for default log directory instead of process.cwd()', async () => { + const customCwd = path.join(testTempDir, 'project-root'); + await fs.mkdir(customCwd, { recursive: true }); + const logger = new OpenAILogger(undefined, customCwd); + await logger.initialize(); + + const request = { test: 'request' }; + const response = { test: 'response' }; + + const logPath = await logger.logInteraction(request, response); + const expectedDir = path.join(customCwd, 'logs', 'openai'); + createdDirs.push(expectedDir); + + expect(logPath).toContain(expectedDir); + }); + + it('should resolve relative customLogDir against provided cwd', async () => { + const customCwd = path.join(testTempDir, 'project-root-2'); + await fs.mkdir(customCwd, { recursive: true }); + const relativeDir = 'my-logs'; + const logger = new OpenAILogger(relativeDir, customCwd); + await logger.initialize(); + + const request = { test: 'request' }; + const response = { test: 'response' }; + + const logPath = await logger.logInteraction(request, response); + const expectedDir = path.resolve(customCwd, relativeDir); + createdDirs.push(expectedDir); + + expect(logPath).toContain(expectedDir); + }); + + it('should not use cwd when customLogDir is an absolute path', async () => { + const customCwd = path.join(testTempDir, 'project-root-3'); + const absoluteLogDir = path.join(testTempDir, 'absolute-logs'); + const logger = new OpenAILogger(absoluteLogDir, customCwd); + await logger.initialize(); + + const request = { test: 'request' }; + const response = { test: 'response' }; + + const logPath = await logger.logInteraction(request, response); + createdDirs.push(absoluteLogDir); + + expect(logPath).toContain(absoluteLogDir); + expect(logPath).not.toContain(customCwd); + }); + + it('should not use cwd when customLogDir starts with ~', async () => { + const customCwd = path.join(testTempDir, 'project-root-4'); + const logger = new OpenAILogger('~/test-openai-logs', customCwd); + await logger.initialize(); + + const request = { test: 'request' }; + const response = { test: 'response' }; + + const logPath = await logger.logInteraction(request, response); + const expectedDir = path.join(os.homedir(), 'test-openai-logs'); + createdDirs.push(expectedDir); + + expect(logPath).toContain(expectedDir); + expect(logPath).not.toContain(customCwd); + }); + + it('should fall back to process.cwd() when cwd is not provided', async () => { + const relativeDir = 'test-relative-logs'; + const logger = new OpenAILogger(relativeDir); + await logger.initialize(); + + const request = { test: 'request' }; + const response = { test: 'response' }; + + const logPath = await logger.logInteraction(request, response); + const expectedDir = path.resolve(process.cwd(), relativeDir); + createdDirs.push(expectedDir); + + expect(logPath).toContain(expectedDir); + }); + }); }); diff --git a/packages/core/src/utils/openaiLogger.ts b/packages/core/src/utils/openaiLogger.ts index c6a56ee0a..43028de2c 100644 --- a/packages/core/src/utils/openaiLogger.ts +++ b/packages/core/src/utils/openaiLogger.ts @@ -22,8 +22,12 @@ export class OpenAILogger { /** * Creates a new OpenAI logger * @param customLogDir Optional custom log directory path (supports relative paths, absolute paths, and ~ expansion) + * @param cwd Optional working directory for resolving relative paths. Defaults to process.cwd(). + * In ACP mode, process.cwd() may be '/' (filesystem root), so callers should + * pass the project working directory from Config.getWorkingDir(). */ - constructor(customLogDir?: string) { + constructor(customLogDir?: string, cwd?: string) { + const baseCwd = cwd || process.cwd(); if (customLogDir) { // Resolve relative paths to absolute paths // Handle ~ expansion @@ -31,12 +35,12 @@ export class OpenAILogger { if (customLogDir === '~' || customLogDir.startsWith('~/')) { resolvedPath = path.join(os.homedir(), customLogDir.slice(1)); } else if (!path.isAbsolute(customLogDir)) { - // If it's a relative path, resolve it relative to current working directory - resolvedPath = path.resolve(process.cwd(), customLogDir); + // If it's a relative path, resolve it relative to provided working directory + resolvedPath = path.resolve(baseCwd, customLogDir); } this.logDir = path.normalize(resolvedPath); } else { - this.logDir = path.join(process.cwd(), 'logs', 'openai'); + this.logDir = path.join(baseCwd, 'logs', 'openai'); } } diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 8224f9950..0cdeaee88 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -55,82 +55,82 @@ afterEach(() => { }); describe('isCommandAllowed', () => { - it('should allow a command if no restrictions are provided', () => { - const result = isCommandAllowed('ls -l', config); + it('should allow a command if no restrictions are provided', async () => { + const result = await isCommandAllowed('ls -l', config); expect(result.allowed).toBe(true); }); - it('should allow a command if it is in the global allowlist', () => { + it('should allow a command if it is in the global allowlist', async () => { config.getCoreTools = () => ['ShellTool(ls)']; - const result = isCommandAllowed('ls -l', config); + const result = await isCommandAllowed('ls -l', config); expect(result.allowed).toBe(true); }); - it('should block a command if it is not in a strict global allowlist', () => { + it('should block a command if it is not in a strict global allowlist', async () => { config.getCoreTools = () => ['ShellTool(ls -l)']; - const result = isCommandAllowed('rm -rf /', config); + const result = await isCommandAllowed('rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command(s) not in the allowed commands list. Disallowed commands: "rm -rf /"`, ); }); - it('should block a command if it is in the blocked list', () => { + it('should block a command if it is in the blocked list', async () => { config.getPermissionsDeny = () => ['ShellTool(rm -rf /)']; - const result = isCommandAllowed('rm -rf /', config); + const result = await isCommandAllowed('rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command 'rm -rf /' is blocked by configuration`, ); }); - it('should prioritize the blocklist over the allowlist', () => { + it('should prioritize the blocklist over the allowlist', async () => { config.getCoreTools = () => ['ShellTool(rm -rf /)']; config.getPermissionsDeny = () => ['ShellTool(rm -rf /)']; - const result = isCommandAllowed('rm -rf /', config); + const result = await isCommandAllowed('rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command 'rm -rf /' is blocked by configuration`, ); }); - it('should allow any command when a wildcard is in coreTools', () => { + it('should allow any command when a wildcard is in coreTools', async () => { config.getCoreTools = () => ['ShellTool']; - const result = isCommandAllowed('any random command', config); + const result = await isCommandAllowed('any random command', config); expect(result.allowed).toBe(true); }); - it('should block any command when a wildcard is in excludeTools', () => { + it('should block any command when a wildcard is in excludeTools', async () => { config.getPermissionsDeny = () => ['run_shell_command']; - const result = isCommandAllowed('any random command', config); + const result = await isCommandAllowed('any random command', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( 'Shell tool is globally disabled in configuration', ); }); - it('should block a command on the blocklist even with a wildcard allow', () => { + it('should block a command on the blocklist even with a wildcard allow', async () => { config.getCoreTools = () => ['ShellTool']; config.getPermissionsDeny = () => ['ShellTool(rm -rf /)']; - const result = isCommandAllowed('rm -rf /', config); + const result = await isCommandAllowed('rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command 'rm -rf /' is blocked by configuration`, ); }); - it('should allow a chained command if all parts are on the global allowlist', () => { + it('should allow a chained command if all parts are on the global allowlist', async () => { config.getCoreTools = () => [ 'run_shell_command(echo)', 'run_shell_command(ls)', ]; - const result = isCommandAllowed('echo "hello" && ls -l', config); + const result = await isCommandAllowed('echo "hello" && ls -l', config); expect(result.allowed).toBe(true); }); - it('should block a chained command if any part is blocked', () => { + it('should block a chained command if any part is blocked', async () => { config.getPermissionsDeny = () => ['run_shell_command(rm)']; - const result = isCommandAllowed('echo "hello" && rm -rf /', config); + const result = await isCommandAllowed('echo "hello" && rm -rf /', config); expect(result.allowed).toBe(false); expect(result.reason).toBe( `Command 'rm -rf /' is blocked by configuration`, @@ -138,20 +138,20 @@ describe('isCommandAllowed', () => { }); describe('command substitution', () => { - it('should block command substitution using `$(...)`', () => { - const result = isCommandAllowed('echo $(rm -rf /)', config); + it('should block command substitution using `$(...)`', async () => { + const result = await isCommandAllowed('echo $(rm -rf /)', config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should block command substitution using `<(...)`', () => { - const result = isCommandAllowed('diff <(ls) <(ls -a)', config); + it('should block command substitution using `<(...)`', async () => { + const result = await isCommandAllowed('diff <(ls) <(ls -a)', config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should block command substitution using `>(...)`', () => { - const result = isCommandAllowed( + it('should block command substitution using `>(...)`', async () => { + const result = await isCommandAllowed( 'echo "Log message" > >(tee log.txt)', config, ); @@ -159,20 +159,20 @@ describe('isCommandAllowed', () => { expect(result.reason).toContain('Command substitution'); }); - it('should block command substitution using backticks', () => { - const result = isCommandAllowed('echo `rm -rf /`', config); + it('should block command substitution using backticks', async () => { + const result = await isCommandAllowed('echo `rm -rf /`', config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should allow substitution-like patterns inside single quotes', () => { + it('should allow substitution-like patterns inside single quotes', async () => { config.getCoreTools = () => ['ShellTool(echo)']; - const result = isCommandAllowed("echo '$(pwd)'", config); + const result = await isCommandAllowed("echo '$(pwd)'", config); expect(result.allowed).toBe(true); }); describe('heredocs', () => { - it('should allow substitution-like content in a quoted heredoc delimiter', () => { + it('should allow substitution-like content in a quoted heredoc delimiter', async () => { const cmd = [ "cat <<'EOF' > user_session.md", '```', @@ -182,55 +182,55 @@ describe('isCommandAllowed', () => { 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); - it('should block command substitution in an unquoted heredoc body', () => { + it('should block command substitution in an unquoted heredoc body', async () => { const cmd = [ 'cat < user_session.md', "'$(rm -rf /)'", 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should block backtick command substitution in an unquoted heredoc body', () => { + it('should block backtick command substitution in an unquoted heredoc body', async () => { const cmd = ['cat < user_session.md', '`rm -rf /`', 'EOF'].join( '\n', ); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should allow escaped command substitution in an unquoted heredoc body', () => { + it('should allow escaped command substitution in an unquoted heredoc body', async () => { const cmd = [ 'cat < user_session.md', '\\$(rm -rf /)', 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); - it('should support tab-stripping heredocs (<<-)', () => { + it('should support tab-stripping heredocs (<<-)', async () => { const cmd = [ "cat <<-'EOF' > user_session.md", '\t$(rm -rf /)', '\tEOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); - it('should block command substitution split by line continuation in an unquoted heredoc body', () => { + it('should block command substitution split by line continuation in an unquoted heredoc body', async () => { const cmd = [ 'cat < user_session.md', '$\\', @@ -238,12 +238,12 @@ describe('isCommandAllowed', () => { 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should allow escaped command substitution split by line continuation in an unquoted heredoc body', () => { + it('should allow escaped command substitution split by line continuation in an unquoted heredoc body', async () => { const cmd = [ 'cat < user_session.md', '\\$\\', @@ -251,36 +251,39 @@ describe('isCommandAllowed', () => { 'EOF', ].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); }); describe('comments', () => { - it('should ignore heredoc operators inside comments', () => { + it('should ignore heredoc operators inside comments', async () => { const cmd = ["# Fake heredoc <<'EOF'", '$(rm -rf /)', 'EOF'].join('\n'); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); - it('should allow command substitution patterns inside full-line comments', () => { + it('should allow command substitution patterns inside full-line comments', async () => { const cmd = ['# Note: $(rm -rf /) is dangerous', 'echo hello'].join( '\n', ); - const result = isCommandAllowed(cmd, config); + const result = await isCommandAllowed(cmd, config); expect(result.allowed).toBe(true); }); - it('should allow command substitution patterns inside inline comments', () => { - const result = isCommandAllowed('echo hello # $(rm -rf /)', config); + it('should allow command substitution patterns inside inline comments', async () => { + const result = await isCommandAllowed( + 'echo hello # $(rm -rf /)', + config, + ); expect(result.allowed).toBe(true); }); - it('should not treat # inside a word as a comment starter', () => { - const result = isCommandAllowed('echo foo#$(rm -rf /)', config); + it('should not treat # inside a word as a comment starter', async () => { + const result = await isCommandAllowed('echo foo#$(rm -rf /)', config); expect(result.allowed).toBe(false); expect(result.reason).toContain('Command substitution'); }); @@ -290,17 +293,17 @@ describe('isCommandAllowed', () => { describe('checkCommandPermissions', () => { describe('in "Default Allow" mode (no sessionAllowlist)', () => { - it('should return a detailed success object for an allowed command', () => { - const result = checkCommandPermissions('ls -l', config); + it('should return a detailed success object for an allowed command', async () => { + const result = await checkCommandPermissions('ls -l', config); expect(result).toEqual({ allAllowed: true, disallowedCommands: [], }); }); - it('should return a detailed failure object for a blocked command', () => { + it('should return a detailed failure object for a blocked command', async () => { config.getPermissionsDeny = () => ['ShellTool(rm)']; - const result = checkCommandPermissions('rm -rf /', config); + const result = await checkCommandPermissions('rm -rf /', config); expect(result).toEqual({ allAllowed: false, disallowedCommands: ['rm -rf /'], @@ -309,9 +312,9 @@ describe('checkCommandPermissions', () => { }); }); - it('should return a detailed failure object for a command not on a strict allowlist', () => { + it('should return a detailed failure object for a command not on a strict allowlist', async () => { config.getCoreTools = () => ['ShellTool(ls)']; - const result = checkCommandPermissions('git status && ls', config); + const result = await checkCommandPermissions('git status && ls', config); expect(result).toEqual({ allAllowed: false, disallowedCommands: ['git status'], @@ -322,8 +325,8 @@ describe('checkCommandPermissions', () => { }); describe('in "Default Deny" mode (with sessionAllowlist)', () => { - it('should allow a command on the sessionAllowlist', () => { - const result = checkCommandPermissions( + it('should allow a command on the sessionAllowlist', async () => { + const result = await checkCommandPermissions( 'ls -l', config, new Set(['ls -l']), @@ -331,8 +334,8 @@ describe('checkCommandPermissions', () => { expect(result.allAllowed).toBe(true); }); - it('should block a command not on the sessionAllowlist or global allowlist', () => { - const result = checkCommandPermissions( + it('should block a command not on the sessionAllowlist or global allowlist', async () => { + const result = await checkCommandPermissions( 'rm -rf /', config, new Set(['ls -l']), @@ -344,9 +347,9 @@ describe('checkCommandPermissions', () => { expect(result.disallowedCommands).toEqual(['rm -rf /']); }); - it('should allow a command on the global allowlist even if not on the session allowlist', () => { + it('should allow a command on the global allowlist even if not on the session allowlist', async () => { config.getCoreTools = () => ['ShellTool(git status)']; - const result = checkCommandPermissions( + const result = await checkCommandPermissions( 'git status', config, new Set(['ls -l']), @@ -354,9 +357,9 @@ describe('checkCommandPermissions', () => { expect(result.allAllowed).toBe(true); }); - it('should allow a chained command if parts are on different allowlists', () => { + it('should allow a chained command if parts are on different allowlists', async () => { config.getCoreTools = () => ['ShellTool(git status)']; - const result = checkCommandPermissions( + const result = await checkCommandPermissions( 'git status && git commit', config, new Set(['git commit']), @@ -364,9 +367,9 @@ describe('checkCommandPermissions', () => { expect(result.allAllowed).toBe(true); }); - it('should block a command on the sessionAllowlist if it is also globally blocked', () => { + it('should block a command on the sessionAllowlist if it is also globally blocked', async () => { config.getPermissionsDeny = () => ['run_shell_command(rm)']; - const result = checkCommandPermissions( + const result = await checkCommandPermissions( 'rm -rf /', config, new Set(['rm -rf /']), @@ -375,9 +378,9 @@ describe('checkCommandPermissions', () => { expect(result.blockReason).toContain('is blocked by configuration'); }); - it('should block a chained command if one part is not on any allowlist', () => { + it('should block a chained command if one part is not on any allowlist', async () => { config.getCoreTools = () => ['run_shell_command(echo)']; - const result = checkCommandPermissions( + const result = await checkCommandPermissions( 'echo "hello" && rm -rf /', config, new Set(['echo']), @@ -389,101 +392,101 @@ describe('checkCommandPermissions', () => { }); describe('getCommandRoots', () => { - it('should return a single command', () => { + it('should return a single command', async () => { expect(getCommandRoots('ls -l')).toEqual(['ls']); }); - it('should handle paths and return the binary name', () => { + it('should handle paths and return the binary name', async () => { expect(getCommandRoots('/usr/local/bin/node script.js')).toEqual(['node']); }); - it('should return an empty array for an empty string', () => { + it('should return an empty array for an empty string', async () => { expect(getCommandRoots('')).toEqual([]); }); - it('should handle a mix of operators', () => { + it('should handle a mix of operators', async () => { const result = getCommandRoots('a;b|c&&d||e&f'); expect(result).toEqual(['a', 'b', 'c', 'd', 'e', 'f']); }); - it('should correctly parse a chained command with quotes', () => { + it('should correctly parse a chained command with quotes', async () => { const result = getCommandRoots('echo "hello" && git commit -m "feat"'); expect(result).toEqual(['echo', 'git']); }); - it('should split on Unix newlines (\\n)', () => { + it('should split on Unix newlines (\\n)', async () => { const result = getCommandRoots('grep pattern file\ncurl evil.com'); expect(result).toEqual(['grep', 'curl']); }); - it('should split on Windows newlines (\\r\\n)', () => { + it('should split on Windows newlines (\\r\\n)', async () => { const result = getCommandRoots('grep pattern file\r\ncurl evil.com'); expect(result).toEqual(['grep', 'curl']); }); - it('should handle mixed newlines and operators', () => { + it('should handle mixed newlines and operators', async () => { const result = getCommandRoots('ls\necho hello && cat file\r\nrm -rf /'); expect(result).toEqual(['ls', 'echo', 'cat', 'rm']); }); - it('should not split on newlines inside quotes', () => { + it('should not split on newlines inside quotes', async () => { const result = getCommandRoots('echo "line1\nline2"'); expect(result).toEqual(['echo']); }); - it('should treat escaped newline as line continuation (not a separator)', () => { + it('should treat escaped newline as line continuation (not a separator)', async () => { const result = getCommandRoots('grep pattern\\\nfile'); expect(result).toEqual(['grep']); }); - it('should filter out empty segments from consecutive newlines', () => { + it('should filter out empty segments from consecutive newlines', async () => { const result = getCommandRoots('ls\n\ngrep foo'); expect(result).toEqual(['ls', 'grep']); }); - it('should not treat file descriptor redirection as a command separator', () => { + it('should not treat file descriptor redirection as a command separator', async () => { const result = getCommandRoots('npm run build 2>&1 | head -100'); expect(result).toEqual(['npm', 'head']); }); - it('should not treat >| redirection as a pipeline separator', () => { + it('should not treat >| redirection as a pipeline separator', async () => { const result = getCommandRoots('echo hello >| out.txt'); expect(result).toEqual(['echo']); }); }); describe('stripShellWrapper', () => { - it('should strip sh -c with quotes', () => { + it('should strip sh -c with quotes', async () => { expect(stripShellWrapper('sh -c "ls -l"')).toEqual('ls -l'); }); - it('should strip bash -c with extra whitespace', () => { + it('should strip bash -c with extra whitespace', async () => { expect(stripShellWrapper(' bash -c "ls -l" ')).toEqual('ls -l'); }); - it('should strip zsh -c without quotes', () => { + it('should strip zsh -c without quotes', async () => { expect(stripShellWrapper('zsh -c ls -l')).toEqual('ls -l'); }); - it('should strip cmd.exe /c', () => { + it('should strip cmd.exe /c', async () => { expect(stripShellWrapper('cmd.exe /c "dir"')).toEqual('dir'); }); - it('should not strip anything if no wrapper is present', () => { + it('should not strip anything if no wrapper is present', async () => { expect(stripShellWrapper('ls -l')).toEqual('ls -l'); }); }); describe('escapeShellArg', () => { describe('POSIX (bash)', () => { - it('should use shell-quote for escaping', () => { + it('should use shell-quote for escaping', async () => { mockQuote.mockReturnValueOnce("'escaped value'"); const result = escapeShellArg('raw value', 'bash'); expect(mockQuote).toHaveBeenCalledWith(['raw value']); expect(result).toBe("'escaped value'"); }); - it('should handle empty strings', () => { + it('should handle empty strings', async () => { const result = escapeShellArg('', 'bash'); expect(result).toBe(''); expect(mockQuote).not.toHaveBeenCalled(); @@ -492,39 +495,39 @@ describe('escapeShellArg', () => { describe('Windows', () => { describe('when shell is cmd.exe', () => { - it('should wrap simple arguments in double quotes', () => { + it('should wrap simple arguments in double quotes', async () => { const result = escapeShellArg('search term', 'cmd'); expect(result).toBe('"search term"'); }); - it('should escape internal double quotes by doubling them', () => { + it('should escape internal double quotes by doubling them', async () => { const result = escapeShellArg('He said "Hello"', 'cmd'); expect(result).toBe('"He said ""Hello"""'); }); - it('should handle empty strings', () => { + it('should handle empty strings', async () => { const result = escapeShellArg('', 'cmd'); expect(result).toBe(''); }); }); describe('when shell is PowerShell', () => { - it('should wrap simple arguments in single quotes', () => { + it('should wrap simple arguments in single quotes', async () => { const result = escapeShellArg('search term', 'powershell'); expect(result).toBe("'search term'"); }); - it('should escape internal single quotes by doubling them', () => { + it('should escape internal single quotes by doubling them', async () => { const result = escapeShellArg("It's a test", 'powershell'); expect(result).toBe("'It''s a test'"); }); - it('should handle double quotes without escaping them', () => { + it('should handle double quotes without escaping them', async () => { const result = escapeShellArg('He said "Hello"', 'powershell'); expect(result).toBe('\'He said "Hello"\''); }); - it('should handle empty strings', () => { + it('should handle empty strings', async () => { const result = escapeShellArg('', 'powershell'); expect(result).toBe(''); }); @@ -539,7 +542,7 @@ describe('getShellConfiguration', () => { process.env = originalEnv; }); - it('should return bash configuration on Linux', () => { + it('should return bash configuration on Linux', async () => { mockPlatform.mockReturnValue('linux'); const config = getShellConfiguration(); expect(config.executable).toBe('bash'); @@ -547,7 +550,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('bash'); }); - it('should return bash configuration on macOS (darwin)', () => { + it('should return bash configuration on macOS (darwin)', async () => { mockPlatform.mockReturnValue('darwin'); const config = getShellConfiguration(); expect(config.executable).toBe('bash'); @@ -566,7 +569,7 @@ describe('getShellConfiguration', () => { process.env = originalEnv; }); - it('should return cmd.exe configuration by default', () => { + it('should return cmd.exe configuration by default', async () => { delete process.env['ComSpec']; delete process.env['MSYSTEM']; delete process.env['TERM']; @@ -576,7 +579,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('cmd'); }); - it('should respect ComSpec for cmd.exe', () => { + it('should respect ComSpec for cmd.exe', async () => { const cmdPath = 'C:\\WINDOWS\\system32\\cmd.exe'; process.env['ComSpec'] = cmdPath; delete process.env['MSYSTEM']; @@ -587,7 +590,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('cmd'); }); - it('should return PowerShell configuration if ComSpec points to powershell.exe', () => { + it('should return PowerShell configuration if ComSpec points to powershell.exe', async () => { const psPath = 'C:\\WINDOWS\\System32\\WindowsPowerShell\\v1.0\\powershell.exe'; process.env['ComSpec'] = psPath; @@ -599,7 +602,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('powershell'); }); - it('should return PowerShell configuration if ComSpec points to pwsh.exe', () => { + it('should return PowerShell configuration if ComSpec points to pwsh.exe', async () => { const pwshPath = 'C:\\Program Files\\PowerShell\\7\\pwsh.exe'; process.env['ComSpec'] = pwshPath; delete process.env['MSYSTEM']; @@ -610,7 +613,7 @@ describe('getShellConfiguration', () => { expect(config.shell).toBe('powershell'); }); - it('should be case-insensitive when checking ComSpec', () => { + it('should be case-insensitive when checking ComSpec', async () => { process.env['ComSpec'] = 'C:\\Path\\To\\POWERSHELL.EXE'; delete process.env['MSYSTEM']; delete process.env['TERM']; @@ -624,7 +627,11 @@ describe('getShellConfiguration', () => { it('should return bash configuration when MSYSTEM starts with MINGW', () => { process.env['MSYSTEM'] = 'MINGW64'; const config = getShellConfiguration(); - expect(config.executable).toBe('bash'); + // executable should be bash.exe path (either 'bash' or full path like 'C:\...\bash.exe') + expect( + config.executable.endsWith('bash.exe') || + config.executable === 'bash', + ).toBe(true); expect(config.argsPrefix).toEqual(['-c']); expect(config.shell).toBe('bash'); }); @@ -632,7 +639,10 @@ describe('getShellConfiguration', () => { it('should return bash configuration when MSYSTEM starts with MSYS', () => { process.env['MSYSTEM'] = 'MSYS'; const config = getShellConfiguration(); - expect(config.executable).toBe('bash'); + expect( + config.executable.endsWith('bash.exe') || + config.executable === 'bash', + ).toBe(true); expect(config.argsPrefix).toEqual(['-c']); expect(config.shell).toBe('bash'); }); @@ -641,7 +651,10 @@ describe('getShellConfiguration', () => { delete process.env['MSYSTEM']; process.env['TERM'] = 'xterm-256color-msys'; const config = getShellConfiguration(); - expect(config.executable).toBe('bash'); + expect( + config.executable.endsWith('bash.exe') || + config.executable === 'bash', + ).toBe(true); expect(config.argsPrefix).toEqual(['-c']); expect(config.shell).toBe('bash'); }); @@ -650,7 +663,10 @@ describe('getShellConfiguration', () => { delete process.env['MSYSTEM']; process.env['TERM'] = 'xterm-256color-cygwin'; const config = getShellConfiguration(); - expect(config.executable).toBe('bash'); + expect( + config.executable.endsWith('bash.exe') || + config.executable === 'bash', + ).toBe(true); expect(config.argsPrefix).toEqual(['-c']); expect(config.shell).toBe('bash'); }); @@ -659,7 +675,10 @@ describe('getShellConfiguration', () => { process.env['MSYSTEM'] = 'MINGW64'; process.env['TERM'] = 'xterm'; const config = getShellConfiguration(); - expect(config.executable).toBe('bash'); + expect( + config.executable.endsWith('bash.exe') || + config.executable === 'bash', + ).toBe(true); expect(config.argsPrefix).toEqual(['-c']); expect(config.shell).toBe('bash'); }); @@ -677,7 +696,10 @@ describe('getShellConfiguration', () => { it('should return bash when MSYSTEM is MINGW32', () => { process.env['MSYSTEM'] = 'MINGW32'; const config = getShellConfiguration(); - expect(config.executable).toBe('bash'); + expect( + config.executable.endsWith('bash.exe') || + config.executable === 'bash', + ).toBe(true); expect(config.argsPrefix).toEqual(['-c']); expect(config.shell).toBe('bash'); }); @@ -686,12 +708,12 @@ describe('getShellConfiguration', () => { }); describe('isCommandNeedPermission', () => { - it('returns false for read-only commands', () => { + it('returns false for read-only commands', async () => { const result = isCommandNeedsPermission('ls'); expect(result.requiresPermission).toBe(false); }); - it('returns true for mutating commands with reason', () => { + it('returns true for mutating commands with reason', async () => { const result = isCommandNeedsPermission('rm -rf temp'); expect(result.requiresPermission).toBe(true); expect(result.reason).toContain('requires permission to execute'); @@ -700,13 +722,13 @@ describe('isCommandNeedPermission', () => { describe('checkArgumentSafety', () => { describe('command substitution patterns', () => { - it('should detect $() command substitution', () => { + it('should detect $() command substitution', async () => { const result = checkArgumentSafety('$(whoami)'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('$() command substitution'); }); - it('should detect backtick command substitution', () => { + it('should detect backtick command substitution', async () => { const result = checkArgumentSafety('`whoami`'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain( @@ -714,13 +736,13 @@ describe('checkArgumentSafety', () => { ); }); - it('should detect <() process substitution', () => { + it('should detect <() process substitution', async () => { const result = checkArgumentSafety('<(cat file)'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('<() process substitution'); }); - it('should detect >() process substitution', () => { + it('should detect >() process substitution', async () => { const result = checkArgumentSafety('>(tee file)'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('>() process substitution'); @@ -728,25 +750,25 @@ describe('checkArgumentSafety', () => { }); describe('command separators', () => { - it('should detect semicolon separator', () => { + it('should detect semicolon separator', async () => { const result = checkArgumentSafety('arg1; rm -rf /'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('; command separator'); }); - it('should detect pipe', () => { + it('should detect pipe', async () => { const result = checkArgumentSafety('arg1 | cat file'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('| pipe'); }); - it('should detect && operator', () => { + it('should detect && operator', async () => { const result = checkArgumentSafety('arg1 && ls'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('&& AND operator'); }); - it('should detect || operator', () => { + it('should detect || operator', async () => { const result = checkArgumentSafety('arg1 || ls'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('|| OR operator'); @@ -754,7 +776,7 @@ describe('checkArgumentSafety', () => { }); describe('background execution', () => { - it('should detect background operator', () => { + it('should detect background operator', async () => { const result = checkArgumentSafety('arg1 & ls'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('& background operator'); @@ -762,19 +784,19 @@ describe('checkArgumentSafety', () => { }); describe('input/output redirection', () => { - it('should detect output redirection', () => { + it('should detect output redirection', async () => { const result = checkArgumentSafety('arg1 > file'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('> output redirection'); }); - it('should detect input redirection', () => { + it('should detect input redirection', async () => { const result = checkArgumentSafety('arg1 < file'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('< input redirection'); }); - it('should detect append redirection', () => { + it('should detect append redirection', async () => { const result = checkArgumentSafety('arg1 >> file'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('> output redirection'); @@ -782,45 +804,45 @@ describe('checkArgumentSafety', () => { }); describe('safe inputs', () => { - it('should accept simple arguments', () => { + it('should accept simple arguments', async () => { const result = checkArgumentSafety('arg1 arg2'); expect(result.isSafe).toBe(true); expect(result.dangerousPatterns).toHaveLength(0); }); - it('should accept arguments with numbers', () => { + it('should accept arguments with numbers', async () => { const result = checkArgumentSafety('file123.txt'); expect(result.isSafe).toBe(true); }); - it('should accept arguments with hyphens', () => { + it('should accept arguments with hyphens', async () => { const result = checkArgumentSafety('--flag=value'); expect(result.isSafe).toBe(true); }); - it('should accept arguments with underscores', () => { + it('should accept arguments with underscores', async () => { const result = checkArgumentSafety('my_file_name'); expect(result.isSafe).toBe(true); }); - it('should accept arguments with dots', () => { + it('should accept arguments with dots', async () => { const result = checkArgumentSafety('path/to/file.txt'); expect(result.isSafe).toBe(true); }); - it('should accept empty string', () => { + it('should accept empty string', async () => { const result = checkArgumentSafety(''); expect(result.isSafe).toBe(true); }); - it('should accept arguments with spaces (quoted)', () => { + it('should accept arguments with spaces (quoted)', async () => { const result = checkArgumentSafety('hello world'); expect(result.isSafe).toBe(true); }); }); describe('multiple dangerous patterns', () => { - it('should detect multiple dangerous patterns', () => { + it('should detect multiple dangerous patterns', async () => { const result = checkArgumentSafety('$(whoami); rm -rf / &'); expect(result.isSafe).toBe(false); expect(result.dangerousPatterns).toContain('$() command substitution'); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 1ef1dd09e..8eeb19eaa 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -7,6 +7,7 @@ import type { AnyToolInvocation } from '../index.js'; import type { Config } from '../config/config.js'; import os from 'node:os'; +import path from 'node:path'; import { quote } from 'shell-quote'; import { doesToolInvocationMatch } from './tool-utils.js'; import { isShellCommandReadOnly } from './shellReadOnlyChecker.js'; @@ -38,6 +39,70 @@ export interface ShellConfiguration { shell: ShellType; } +let cachedBashPath: string | undefined; + +/** + * Attempts to find the Git Bash executable path on Windows. + * Checks common installation locations and PATH. + * @returns The path to bash.exe if found, or 'bash' as fallback. + */ +function findGitBashPath(): string { + // Return cached result if available + if (cachedBashPath) { + return cachedBashPath; + } + + // Search in PATH directories + const pathEnv = process.env['PATH'] || ''; + const pathDirs = pathEnv.split(path.delimiter).filter(Boolean); + + for (const dir of pathDirs) { + const bashPath = path.join(dir, 'bash.exe'); + try { + accessSync(bashPath, fsConstants.X_OK); + cachedBashPath = bashPath; + return bashPath; + } catch { + // Continue searching + } + } + + // Check common Git Bash installation locations + const commonPaths = [ + path.join('C:', 'Program Files', 'Git', 'bin', 'bash.exe'), + path.join('C:', 'Program Files', 'Git', 'usr', 'bin', 'bash.exe'), + path.join('C:', 'Program Files (x86)', 'Git', 'bin', 'bash.exe'), + path.join('C:', 'Program Files (x86)', 'Git', 'usr', 'bin', 'bash.exe'), + path.join( + process.env['ProgramFiles'] || path.join('C:', 'Program Files'), + 'Git', + 'bin', + 'bash.exe', + ), + path.join( + process.env['ProgramFiles(x86)'] || + path.join('C:', 'Program Files (x86)'), + 'Git', + 'bin', + 'bash.exe', + ), + ]; + + for (const bashPath of commonPaths) { + try { + accessSync(bashPath, fsConstants.X_OK); + cachedBashPath = bashPath; + return bashPath; + } catch { + // Continue searching + } + } + + // Fallback to 'bash' and let the system handle it + cachedBashPath = 'bash'; + return 'bash'; +} + /** * Determines the appropriate shell configuration for the current platform. * @@ -60,7 +125,7 @@ export function getShellConfiguration(): ShellConfiguration { if (isGitBash) { return { - executable: 'bash', + executable: findGitBashPath(), argsPrefix: ['-c'], shell: 'bash', }; @@ -670,16 +735,16 @@ export function detectCommandSubstitution(command: string): boolean { * presence activates "Default Deny" mode. * @returns An object detailing which commands are not allowed. */ -export function checkCommandPermissions( +export async function checkCommandPermissions( command: string, config: Config, sessionAllowlist?: Set, -): { +): Promise<{ allAllowed: boolean; disallowedCommands: string[]; blockReason?: string; isHardDenial?: boolean; -} { +}> { // Disallow command substitution for security. if (detectCommandSubstitution(command)) { return { @@ -717,7 +782,7 @@ export function checkCommandPermissions( if (isSessionAllowed) continue; } - const decision = pm.isCommandAllowed(cmd); + const decision = await pm.isCommandAllowed(cmd); if (decision === 'deny') { return { @@ -994,12 +1059,15 @@ export function isCommandAvailable(command: string): { return { available: path !== null, error }; } -export function isCommandAllowed( +export async function isCommandAllowed( command: string, config: Config, -): { allowed: boolean; reason?: string } { +): Promise<{ allowed: boolean; reason?: string }> { // By not providing a sessionAllowlist, we invoke "default allow" behavior. - const { allAllowed, blockReason } = checkCommandPermissions(command, config); + const { allAllowed, blockReason } = await checkCommandPermissions( + command, + config, + ); if (allAllowed) { return { allowed: true }; } diff --git a/packages/core/src/utils/shellAstParser.test.ts b/packages/core/src/utils/shellAstParser.test.ts index 0b0e6abe9..506147e6b 100644 --- a/packages/core/src/utils/shellAstParser.test.ts +++ b/packages/core/src/utils/shellAstParser.test.ts @@ -4,12 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ +import path from 'node:path'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { initParser, isShellCommandReadOnlyAST, extractCommandRules, _resetParser, + _resolveWasmPathForTesting, } from './shellAstParser.js'; beforeAll(async () => { @@ -20,6 +22,44 @@ afterAll(() => { _resetParser(); }); +describe('WASM path resolution', () => { + it('resolves bundled WASM relative to the real CLI path when launched via symlink', () => { + const symlinkedCliPath = path.join('/usr', 'bin', 'qwen'); + const realCliPath = path.join( + '/opt', + 'homebrew', + 'lib', + 'node_modules', + '@qwen-code', + 'qwen-code', + 'dist', + 'cli.js', + ); + + const result = _resolveWasmPathForTesting( + 'tree-sitter.wasm', + symlinkedCliPath, + () => realCliPath, + ); + + expect(result).toBe( + path.join( + '/opt', + 'homebrew', + 'lib', + 'node_modules', + '@qwen-code', + 'qwen-code', + 'dist', + 'vendor', + 'tree-sitter', + 'tree-sitter.wasm', + ), + ); + expect(result).not.toContain(path.join('/usr', 'bin', 'vendor')); + }); +}); + // ========================================================================= // isShellCommandReadOnlyAST — mirror all tests from shellReadOnlyChecker.test.ts // ========================================================================= diff --git a/packages/core/src/utils/shellAstParser.ts b/packages/core/src/utils/shellAstParser.ts index 7b5e5d2b2..0f315b2f9 100644 --- a/packages/core/src/utils/shellAstParser.ts +++ b/packages/core/src/utils/shellAstParser.ts @@ -15,6 +15,7 @@ */ import Parser from 'web-tree-sitter'; +import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -22,8 +23,18 @@ import { fileURLToPath } from 'node:url'; // Constants // --------------------------------------------------------------------------- -const __filename_ = fileURLToPath(import.meta.url); -const __dirname_ = path.dirname(__filename_); +const __filename_ = resolveModuleFilePath(fileURLToPath(import.meta.url)); + +function resolveModuleFilePath(moduleFilePath: string): string { + try { + const resolved = fs.realpathSync(moduleFilePath); + // Guard against test environments where `fs` is mocked and realpathSync + // returns undefined rather than throwing. + return typeof resolved === 'string' ? resolved : moduleFilePath; + } catch { + return moduleFilePath; + } +} /** * Root commands considered read-only by default (no sub-command analysis needed @@ -569,10 +580,24 @@ let initPromise: Promise | null = null; * - Bundle (dist/cli.js): vendor at same level (0 levels) */ function resolveWasmPath(filename: string): string { - const inSrcUtils = __filename_.includes(path.join('src', 'utils')); - const levelsUp = !inSrcUtils ? 0 : __filename_.endsWith('.ts') ? 2 : 3; + return resolveWasmPathForModule(filename, __filename_); +} + +function resolveWasmPathForModule( + filename: string, + moduleFilePath: string, + resolvePath: (moduleFilePath: string) => string = resolveModuleFilePath, +): string { + const resolvedModuleFilePath = resolvePath(moduleFilePath); + const moduleDir = path.dirname(resolvedModuleFilePath); + const inSrcUtils = resolvedModuleFilePath.includes(path.join('src', 'utils')); + const levelsUp = !inSrcUtils + ? 0 + : resolvedModuleFilePath.endsWith('.ts') + ? 2 + : 3; return path.join( - __dirname_, + moduleDir, ...Array(levelsUp).fill('..'), 'vendor', 'tree-sitter', @@ -1084,3 +1109,15 @@ export function _resetParser(): void { bashLanguage = null; initPromise = null; } + +/** + * Internal helper exposed for tests. + * @internal + */ +export function _resolveWasmPathForTesting( + filename: string, + moduleFilePath: string, + resolvePath?: (moduleFilePath: string) => string, +): string { + return resolveWasmPathForModule(filename, moduleFilePath, resolvePath); +} diff --git a/packages/sdk-typescript/package.json b/packages/sdk-typescript/package.json index 6215f6a09..21ab24bf2 100644 --- a/packages/sdk-typescript/package.json +++ b/packages/sdk-typescript/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/sdk", - "version": "0.1.4", + "version": "0.1.6", "description": "TypeScript SDK for programmatic access to qwen-code CLI", "main": "./dist/index.cjs", "module": "./dist/index.mjs", diff --git a/packages/test-utils/package.json b/packages/test-utils/package.json index d4d5c1d85..5def9b873 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.13.0", + "version": "0.13.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 31039854c..da87407eb 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.13.0", + "version": "0.13.2", "publisher": "qwenlm", "icon": "assets/icon.png", "repository": { diff --git a/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts b/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts index e5e69e66a..182025917 100644 --- a/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts +++ b/packages/vscode-ide-companion/src/webview/providers/WebViewProvider.ts @@ -256,25 +256,6 @@ export class WebViewProvider { this.agentManager.onPermissionRequest( async (request: RequestPermissionRequest) => { - // Auto-approve in auto/yolo mode (no UI, no diff) - if (this.isAutoMode()) { - const options = request.options || []; - const pick = (substr: string) => - options.find((o) => - (o.optionId || '').toLowerCase().includes(substr), - )?.optionId; - const pickByKind = (k: string) => - options.find((o) => (o.kind || '').toLowerCase().includes(k)) - ?.optionId; - const optionId = - pick('allow_once') || - pickByKind('allow') || - pick('proceed') || - options[0]?.optionId || - 'allow_once'; - return optionId; - } - // Send permission request to WebView this.sendMessageToWebView({ type: 'permissionRequest', diff --git a/packages/web-templates/package.json b/packages/web-templates/package.json index fbedb34d0..f90055b11 100644 --- a/packages/web-templates/package.json +++ b/packages/web-templates/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/web-templates", - "version": "0.13.0", + "version": "0.13.2", "description": "Web templates bundled as embeddable JS/CSS strings", "repository": { "type": "git", diff --git a/packages/webui/package.json b/packages/webui/package.json index da5a463ab..45112a0d8 100644 --- a/packages/webui/package.json +++ b/packages/webui/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/webui", - "version": "0.13.0", + "version": "0.13.2", "description": "Shared UI components for Qwen Code packages", "type": "module", "main": "./dist/index.cjs", diff --git a/scripts/copy_bundle_assets.js b/scripts/copy_bundle_assets.js index 83ca91f3f..96a65a2aa 100644 --- a/scripts/copy_bundle_assets.js +++ b/scripts/copy_bundle_assets.js @@ -72,6 +72,18 @@ if (existsSync(bundledSkillsDir)) { ); } +// Copy user docs into qc-helper bundled skill so it can reference them at runtime. +// The qc-helper skill reads docs from a `docs/` subdirectory relative to its own +// directory. In the esbuild bundle this becomes dist/bundled/qc-helper/docs/. +const userDocsDir = join(root, 'docs', 'users'); +if (existsSync(userDocsDir)) { + const destDocsDir = join(distDir, 'bundled', 'qc-helper', 'docs'); + copyRecursiveSync(userDocsDir, destDocsDir); + console.log('Copied docs/users/ to dist/bundled/qc-helper/docs/'); +} else { + console.warn(`Warning: User docs directory not found at ${userDocsDir}`); +} + console.log('\nāœ… All bundle assets copied to dist/'); /** diff --git a/scripts/dev.js b/scripts/dev.js index 32f4a2280..bcbe27d89 100644 --- a/scripts/dev.js +++ b/scripts/dev.js @@ -17,13 +17,44 @@ import { spawn } from 'node:child_process'; import { dirname, join } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; -import { writeFileSync, mkdtempSync, rmSync } from 'node:fs'; +import { + writeFileSync, + mkdtempSync, + rmSync, + existsSync, + symlinkSync, + mkdirSync, +} from 'node:fs'; import { tmpdir, platform } from 'node:os'; const __dirname = dirname(fileURLToPath(import.meta.url)); const root = join(__dirname, '..'); const cliPackageDir = join(root, 'packages', 'cli'); +// Ensure qc-helper bundled skill can find user docs in dev mode. +// In dev, import.meta.url resolves to the source tree, so the bundled skill +// directory is packages/core/src/skills/bundled/qc-helper/. We create a +// symlink from there to docs/users/ so the skill can read docs at runtime. +const qcHelperDocsLink = join( + root, + 'packages', + 'core', + 'src', + 'skills', + 'bundled', + 'qc-helper', + 'docs', +); +const userDocsTarget = join(root, 'docs', 'users'); +if (existsSync(userDocsTarget) && !existsSync(qcHelperDocsLink)) { + mkdirSync(dirname(qcHelperDocsLink), { recursive: true }); + try { + symlinkSync(userDocsTarget, qcHelperDocsLink); + } catch { + // Symlink may fail on some systems; non-critical for dev + } +} + // Entry point for the CLI const cliEntry = join(cliPackageDir, 'index.ts'); diff --git a/scripts/prepare-package.js b/scripts/prepare-package.js index 02a8fb017..d38bf2e1e 100644 --- a/scripts/prepare-package.js +++ b/scripts/prepare-package.js @@ -41,6 +41,13 @@ if (!fs.existsSync(vendorDir)) { process.exit(1); } +const bundledDocsDir = path.join(distDir, 'bundled', 'qc-helper', 'docs'); +if (!fs.existsSync(bundledDocsDir)) { + console.error(`Error: Bundled docs not found at ${bundledDocsDir}`); + console.error('Please run "npm run bundle" first'); + process.exit(1); +} + // Copy README and LICENSE console.log('Copying documentation files...'); const filesToCopy = ['README.md', 'LICENSE'];