diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 32d3aebe2..33847b6c0 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,3 +1,6 @@ * @tanzhenxin @DennisYu07 @gwinthis @LaZzyMan @pomelo-nwu @Mingholy @DragonnZhang # SDK TypeScript package changes require review from Mingholy packages/sdk-typescript/** @Mingholy +# vscode-ide-companion and webui packages require review from yiliang114 +packages/vscode-ide-companion/** @yiliang114 +packages/webui/** @yiliang114 diff --git a/README.md b/README.md index ab598666c..8d7293137 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,7 @@ Qwen Code is an open-source AI agent for the terminal, optimized for [Qwen3-Code #### Linux / macOS ```bash -curl -fsSL https://qwen-code-assets.oss-cn-hangzhou.aliyuncs.com/installation/install-qwen.sh | bash +bash -c "$(curl -fsSL https://qwen-code-assets.oss-cn-hangzhou.aliyuncs.com/installation/install-qwen.sh)" ``` #### Windows (Run as Administrator CMD) diff --git a/docs/developers/tools/shell.md b/docs/developers/tools/shell.md index 8113a9892..5325748b5 100644 --- a/docs/developers/tools/shell.md +++ b/docs/developers/tools/shell.md @@ -110,7 +110,11 @@ You can configure the behavior of the `run_shell_command` tool by modifying your ### Enabling Interactive Commands -To enable interactive commands, you need to set the `tools.shell.enableInteractiveShell` setting to `true`. This will use `node-pty` for shell command execution, which allows for interactive sessions. If `node-pty` is not available, it will fall back to the `child_process` implementation, which does not support interactive commands. +The `tools.shell.enableInteractiveShell` setting controls whether shell commands are executed via `node-pty` (interactive PTY) or the plain `child_process` backend. When enabled, interactive sessions such as `vim`, `git rebase -i`, and TUI programs work correctly. + +This setting defaults to `true` on most platforms. On Windows builds **<= 19041** (before Windows 10 version 2004), it defaults to `false` because older ConPTY implementations have known reliability issues (missing output, hangs). This matches the same cutoff used by VS Code ([microsoft/vscode#123725](https://github.com/microsoft/vscode/issues/123725)). If `node-pty` is not available at runtime, the tool falls back to `child_process` regardless of this setting. + +To explicitly override the default, set the value in `settings.json`: **Example `settings.json`:** diff --git a/integration-tests/hook-integration/hooks.test.ts b/integration-tests/hook-integration/hooks.test.ts index a6c873620..e5c860d4b 100644 --- a/integration-tests/hook-integration/hooks.test.ts +++ b/integration-tests/hook-integration/hooks.test.ts @@ -131,7 +131,7 @@ describe('Hooks System Integration', () => { it('should block tool execution when hook returns block and verify no tool was called', async () => { const blockScript = - 'echo \'{"decision": "block", "reason": "File writing blocked by security policy"}\''; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "block", "reason": "File writing blocked by security policy"}\''; await rig.setup('ups-block-tool', { settings: { @@ -160,6 +160,12 @@ describe('Hooks System Integration', () => { ).rejects.toThrow(/block/i); // Tool should not be called due to blocking hook + const hookInvokeCount = rig + .readFile('hook_invoke_count.txt') + .split('\n') + .filter((line) => line.trim() === 'hook_called').length; + expect(hookInvokeCount).toBeGreaterThan(0); // At least one hook call occurred + const toolLogs = rig.readToolLogs(); const writeFileCalls = toolLogs.filter( (t) => @@ -940,8 +946,9 @@ describe('Hooks System Integration', () => { it('should continue execution when hook returns block decision', async () => { // Stop hook's block decision means "block stopping" (i.e., force continuation) // not "block operation and show error" + // Use background process to write count file, ensuring final output is pure JSON const blockStopScript = - 'echo \'{"decision": "block", "reason": "Stop blocked by security policy"}\''; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "block", "reason": "Stop blocked by security policy"}\''; await rig.setup('stop-block-decision', { settings: { @@ -965,15 +972,25 @@ describe('Hooks System Integration', () => { }); // When Stop hook blocks, agent continues execution normally (with max turns to prevent infinite loop) - const result = await rig.run('Say hello', '--max-session-turns', '2'); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); + const result = await rig.run('Say hello', '--max-session-turns', '3'); + + // Verify that execution completed successfully (not blocked by Stop hook) + // Verify Stop hook was invoked multiple times (indicating multiple rounds) + const hookInvokeCount = rig + .readFile('hook_invoke_count.txt') + .split('\n') + .filter((line) => line.trim() === 'hook_called').length; + expect(hookInvokeCount).toBeGreaterThan(1); + + const toolLogs = rig.readToolLogs(); + const hasActivity = result.length > 0 || toolLogs.length > 0; + expect(hasActivity).toBe(true); }); it('should continue execution with custom reason', async () => { // Stop hook's block decision means "block stopping" (i.e., force continuation) const blockReasonScript = - 'echo \'{"decision": "block", "reason": "Custom block reason: task incomplete"}\''; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "block", "reason": "Custom block reason: task incomplete"}\''; await rig.setup('stop-block-custom-reason', { settings: { @@ -997,46 +1014,19 @@ describe('Hooks System Integration', () => { }); // When Stop hook blocks, agent continues execution normally (with max turns to prevent infinite loop) - const result = await rig.run('Say goodbye', '--max-session-turns', '2'); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); - }); - }); + const result = await rig.run('Say goodbye', '--max-session-turns', '3'); - describe('Continue False', () => { - it('should request continue execution when hook returns continue: false', async () => { - const continueScript = - 'echo \'{"continue": false, "stopReason": "More work needed"}\''; + // Verify that execution completed successfully (not blocked by Stop hook) + // This confirms: 1) Agent could execute after Stop hook blocked, 2) Session terminated normally + const hookInvokeCount = rig + .readFile('hook_invoke_count.txt') + .split('\n') + .filter((line) => line.trim() === 'hook_called').length; + expect(hookInvokeCount).toBeGreaterThan(1); - await rig.setup('stop-continue-false', { - settings: { - hooksConfig: { enabled: true }, - hooks: { - Stop: [ - { - hooks: [ - { - type: 'command', - command: continueScript, - name: 'stop-continue-hook', - timeout: 5000, - }, - ], - }, - ], - }, - trusted: true, - }, - }); - - // When continue: false, agent continues execution normally (with max turns to prevent infinite loop) - const result = await rig.run( - 'Say continue', - '--max-session-turns', - '2', - ); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); + const toolLogs = rig.readToolLogs(); + const hasActivity = result.length > 0 || toolLogs.length > 0; + expect(hasActivity).toBe(true); }); }); @@ -1260,8 +1250,9 @@ describe('Hooks System Integration', () => { // Stop hook's block decision means "block stopping" (i.e., force continuation) const allowScript = 'echo \'{"decision": "allow", "reason": "Stop allowed"}\''; + // Write to a file to count hook invocations, then echo the decision const blockScript = - 'echo \'{"decision": "block", "reason": "Stop blocked by security policy"}\''; + 'echo "hook_called" >> hook_invoke_count.txt; echo \'{"decision": "block", "reason": "Stop blocked by security policy"}\''; await rig.setup('stop-multi-one-blocks', { settings: { @@ -1294,18 +1285,28 @@ describe('Hooks System Integration', () => { const result = await rig.run( 'Say multi stop', '--max-session-turns', - '2', + '3', ); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); + + // Verify that execution completed successfully (not blocked by Stop hook) + // This confirms: 1) Agent could execute after Stop hook blocked, 2) Session terminated normally + const hookInvokeCount = rig + .readFile('hook_invoke_count.txt') + .split('\n') + .filter((line) => line.trim() === 'hook_called').length; + expect(hookInvokeCount).toBeGreaterThan(1); + + const toolLogs = rig.readToolLogs(); + const hasActivity = result.length > 0 || toolLogs.length > 0; + expect(hasActivity).toBe(true); }); it('should continue execution when first sequential stop hook returns block', async () => { // Stop hook's block decision means "block stopping" (i.e., force continuation) const blockScript = - 'echo \'{"decision": "block", "reason": "First hook blocks stop"}\''; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "block", "reason": "First hook blocks stop"}\''; const allowScript = - 'echo \'{"decision": "allow", "reason": "This should not run"}\''; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "allow", "reason": "This should still run"}\''; await rig.setup('stop-seq-first-blocks', { settings: { @@ -1339,18 +1340,28 @@ describe('Hooks System Integration', () => { const result = await rig.run( 'Say sequential stop', '--max-session-turns', - '2', + '3', ); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); + + // Verify that execution completed successfully (not blocked by Stop hook) + // This confirms: 1) Agent could execute after Stop hook blocked, 2) Session terminated normally + const hookInvokeCount = rig + .readFile('hook_invoke_count.txt') + .split('\n') + .filter((line) => line.trim() === 'hook_called').length; + expect(hookInvokeCount).toBeGreaterThan(1); + + const toolLogs = rig.readToolLogs(); + const hasActivity = result.length > 0 || toolLogs.length > 0; + expect(hasActivity).toBe(true); }); it('should continue execution when second sequential stop hook returns block', async () => { // Stop hook's block decision means "block stopping" (i.e., force continuation) const allowScript = - 'echo \'{"decision": "allow", "reason": "First allows"}\''; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "allow", "reason": "First allows"}\''; const blockScript = - 'echo \'{"decision": "block", "reason": "Second hook blocks stop"}\''; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "block", "reason": "Second hook blocks stop"}\''; await rig.setup('stop-seq-second-blocks', { settings: { @@ -1384,10 +1395,20 @@ describe('Hooks System Integration', () => { const result = await rig.run( 'Say seq second blocks', '--max-session-turns', - '2', + '3', ); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); + + // Verify that execution completed successfully (not blocked by Stop hook) + // This confirms: 1) Agent could execute after Stop hook blocked, 2) Session terminated normally + const hookInvokeCount = rig + .readFile('hook_invoke_count.txt') + .split('\n') + .filter((line) => line.trim() === 'hook_called').length; + expect(hookInvokeCount).toBeGreaterThan(1); + + const toolLogs = rig.readToolLogs(); + const hasActivity = result.length > 0 || toolLogs.length > 0; + expect(hasActivity).toBe(true); }); it('should handle multiple stop hooks all returning allow', async () => { @@ -1439,9 +1460,9 @@ describe('Hooks System Integration', () => { it('should handle multiple stop hooks all returning block', async () => { const block1Script = - 'echo {"decision": "block", "reason": "First blocks"}'; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "block", "reason": "First blocks"}\''; const block2Script = - 'echo {"decision": "block", "reason": "Second blocks"}'; + '(echo "hook_called" >> hook_invoke_count.txt &) ; echo \'{"decision": "block", "reason": "Second blocks"}\''; await rig.setup('stop-multi-all-block', { settings: { @@ -1471,138 +1492,18 @@ describe('Hooks System Integration', () => { }); // When Stop hooks block, agent continues execution normally (with max turns to prevent infinite loop) - const result = await rig.run( + const _result = await rig.run( 'Say all block', '--max-session-turns', - '2', + '3', ); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); - }); - it('should handle multiple continue: false from different stop hooks', async () => { - const continue1Script = - 'echo {"continue": false, "stopReason": "First needs more work"}'; - const continue2Script = - 'echo {"continue": false, "stopReason": "Second needs more work"}'; - - await rig.setup('stop-multi-continue-false', { - settings: { - hooksConfig: { enabled: true }, - hooks: { - Stop: [ - { - hooks: [ - { - type: 'command', - command: continue1Script, - name: 'stop-continue-1', - timeout: 5000, - }, - { - type: 'command', - command: continue2Script, - name: 'stop-continue-2', - timeout: 5000, - }, - ], - }, - ], - }, - trusted: true, - }, - }); - - // When continue: false, agent continues execution normally (with max turns to prevent infinite loop) - const result = await rig.run( - 'Say multi continue', - '--max-session-turns', - '2', - ); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); - }); - - it('should handle mixed allow and continue: false in stop hooks', async () => { - const allowScript = - 'echo {"decision": "allow", "reason": "Allow stop"}'; - const continueScript = - 'echo {"continue": false, "stopReason": "Need more work"}'; - - await rig.setup('stop-mixed-allow-continue', { - settings: { - hooksConfig: { enabled: true }, - hooks: { - Stop: [ - { - hooks: [ - { - type: 'command', - command: allowScript, - name: 'stop-allow-hook', - timeout: 5000, - }, - { - type: 'command', - command: continueScript, - name: 'stop-continue-hook', - timeout: 5000, - }, - ], - }, - ], - }, - trusted: true, - }, - }); - - // When continue: false, agent continues execution normally (with max turns to prevent infinite loop) - const result = await rig.run('Say mixed', '--max-session-turns', '2'); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); - }); - - it('should handle block with higher priority than continue: false', async () => { - const blockScript = - 'echo {"decision": "block", "reason": "Security block"}'; - const continueScript = - 'echo {"continue": false, "stopReason": "Need more work"}'; - - await rig.setup('stop-block-vs-continue', { - settings: { - hooksConfig: { enabled: true }, - hooks: { - Stop: [ - { - hooks: [ - { - type: 'command', - command: blockScript, - name: 'stop-block-priority', - timeout: 5000, - }, - { - type: 'command', - command: continueScript, - name: 'stop-continue-lower', - timeout: 5000, - }, - ], - }, - ], - }, - trusted: true, - }, - }); - - // When Stop hook blocks, agent continues execution normally (with max turns to prevent infinite loop) - const result = await rig.run( - 'Say block priority', - '--max-session-turns', - '2', - ); - expect(result).toBeDefined(); - expect(result.length).toBeGreaterThan(0); + // Verify Stop hook was invoked multiple times (indicating multiple rounds) + const hookInvokeCount = rig + .readFile('hook_invoke_count.txt') + .split('\n') + .filter((line) => line.trim() === 'hook_called').length; + expect(hookInvokeCount).toBeGreaterThan(1); }); it('should handle stop hook with error alongside blocking hook', async () => { diff --git a/package-lock.json b/package-lock.json index a9c699f64..8f78bd3f2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@qwen-code/qwen-code", - "version": "0.12.0", + "version": "0.12.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@qwen-code/qwen-code", - "version": "0.12.0", + "version": "0.12.3", "workspaces": [ "packages/*" ], @@ -22,7 +22,6 @@ "@types/mime-types": "^3.0.1", "@types/minimatch": "^5.1.2", "@types/mock-fs": "^4.13.4", - "@types/qrcode-terminal": "^0.12.2", "@types/shell-quote": "^1.7.5", "@types/uuid": "^10.0.0", "@vitest/coverage-v8": "^3.1.1", @@ -4538,13 +4537,6 @@ "dev": true, "license": "MIT" }, - "node_modules/@types/qrcode-terminal": { - "version": "0.12.2", - "resolved": "https://registry.npmjs.org/@types/qrcode-terminal/-/qrcode-terminal-0.12.2.tgz", - "integrity": "sha512-v+RcIEJ+Uhd6ygSQ0u5YYY7ZM+la7GgPbs0V/7l/kFs2uO4S8BcIUEMoP7za4DNIqNnUD5npf0A/7kBhrCKG5Q==", - "dev": true, - "license": "MIT" - }, "node_modules/@types/qs": { "version": "6.14.0", "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.14.0.tgz", @@ -14711,14 +14703,6 @@ "url": "https://github.com/sponsors/sindresorhus" } }, - "node_modules/qrcode-terminal": { - "version": "0.12.0", - "resolved": "https://registry.npmjs.org/qrcode-terminal/-/qrcode-terminal-0.12.0.tgz", - "integrity": "sha512-EXtzRZmC+YGmGlDFbXKxQiMZNwCLEO6BANKXG4iCtSIM0yqc/pappSx3RIKr4r0uh5JsBckOXeKrB3Iz7mdQpQ==", - "bin": { - "qrcode-terminal": "bin/qrcode-terminal.js" - } - }, "node_modules/qs": { "version": "6.13.0", "resolved": "https://registry.npmjs.org/qs/-/qs-6.13.0.tgz", @@ -18800,7 +18784,7 @@ }, "packages/cli": { "name": "@qwen-code/qwen-code", - "version": "0.12.0", + "version": "0.12.3", "dependencies": { "@agentclientprotocol/sdk": "^0.14.1", "@google/genai": "1.30.0", @@ -18825,7 +18809,6 @@ "open": "^10.1.2", "p-limit": "^7.3.0", "prompts": "^2.4.2", - "qrcode-terminal": "^0.12.0", "react": "^19.1.0", "read-package-up": "^11.0.0", "shell-quote": "^1.8.3", @@ -19458,7 +19441,7 @@ }, "packages/core": { "name": "@qwen-code/qwen-code-core", - "version": "0.12.0", + "version": "0.12.3", "hasInstallScript": true, "dependencies": { "@anthropic-ai/sdk": "^0.36.1", @@ -22889,7 +22872,7 @@ }, "packages/test-utils": { "name": "@qwen-code/qwen-code-test-utils", - "version": "0.12.0", + "version": "0.12.3", "dev": true, "license": "Apache-2.0", "devDependencies": { @@ -22901,7 +22884,7 @@ }, "packages/vscode-ide-companion": { "name": "qwen-code-vscode-ide-companion", - "version": "0.12.0", + "version": "0.12.3", "license": "LICENSE", "dependencies": { "@agentclientprotocol/sdk": "^0.14.1", @@ -23149,7 +23132,7 @@ }, "packages/web-templates": { "name": "@qwen-code/web-templates", - "version": "0.12.0", + "version": "0.12.3", "devDependencies": { "@types/react": "^18.2.0", "@types/react-dom": "^18.2.0", @@ -23677,7 +23660,7 @@ }, "packages/webui": { "name": "@qwen-code/webui", - "version": "0.12.0", + "version": "0.12.3", "license": "MIT", "dependencies": { "markdown-it": "^14.1.0" diff --git a/package.json b/package.json index d12e16152..0e6ff1328 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code", - "version": "0.12.0", + "version": "0.12.3", "engines": { "node": ">=20.0.0" }, @@ -13,7 +13,7 @@ "url": "git+https://github.com/QwenLM/qwen-code.git" }, "config": { - "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.12.0" + "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.12.3" }, "scripts": { "start": "cross-env node scripts/start.js", @@ -80,7 +80,6 @@ "@types/mime-types": "^3.0.1", "@types/minimatch": "^5.1.2", "@types/mock-fs": "^4.13.4", - "@types/qrcode-terminal": "^0.12.2", "@types/shell-quote": "^1.7.5", "@types/uuid": "^10.0.0", "@vitest/coverage-v8": "^3.1.1", diff --git a/packages/cli/index.ts b/packages/cli/index.ts index 8e9912f10..3b00b9546 100644 --- a/packages/cli/index.ts +++ b/packages/cli/index.ts @@ -9,8 +9,30 @@ import './src/gemini.js'; import { main } from './src/gemini.js'; import { FatalError } from '@qwen-code/qwen-code-core'; +import { writeStderrLine } from './src/utils/stdioHelpers.js'; // --- Global Entry Point --- + +// Suppress known race condition in @lydell/node-pty on Windows where a +// deferred resize fires after the pty process has already exited. +// Tracking bug: https://github.com/microsoft/node-pty/issues/827 +process.on('uncaughtException', (error) => { + if ( + process.platform === 'win32' && + error instanceof Error && + error.message === 'Cannot resize a pty that has already exited' + ) { + return; + } + + if (error instanceof Error) { + writeStderrLine(error.stack ?? error.message); + } else { + writeStderrLine(String(error)); + } + process.exit(1); +}); + main().catch((error) => { if (error instanceof FatalError) { let errorMessage = error.message; diff --git a/packages/cli/package.json b/packages/cli/package.json index 32073bb5c..940443907 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code", - "version": "0.12.0", + "version": "0.12.3", "description": "Qwen Code", "repository": { "type": "git", @@ -33,7 +33,7 @@ "dist" ], "config": { - "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.12.0" + "sandboxImageUri": "ghcr.io/qwenlm/qwen-code:0.12.3" }, "dependencies": { "@agentclientprotocol/sdk": "^0.14.1", @@ -59,7 +59,6 @@ "open": "^10.1.2", "p-limit": "^7.3.0", "prompts": "^2.4.2", - "qrcode-terminal": "^0.12.0", "react": "^19.1.0", "read-package-up": "^11.0.0", "shell-quote": "^1.8.3", diff --git a/packages/cli/src/acp-integration/service/filesystem.test.ts b/packages/cli/src/acp-integration/service/filesystem.test.ts index 628807fe2..a8683c7c5 100644 --- a/packages/cli/src/acp-integration/service/filesystem.test.ts +++ b/packages/cli/src/acp-integration/service/filesystem.test.ts @@ -13,22 +13,23 @@ const RESOURCE_NOT_FOUND_CODE = -32002; const INTERNAL_ERROR_CODE = -32603; const createFallback = (): FileSystemService => ({ - readTextFile: vi.fn(), - readTextFileWithInfo: vi - .fn() - .mockResolvedValue({ content: '', encoding: 'utf-8', bom: false }), - writeTextFile: vi.fn(), - detectFileBOM: vi.fn().mockResolvedValue(false), + readTextFile: vi.fn().mockResolvedValue({ + content: '', + _meta: { bom: false, encoding: 'utf-8' }, + }), + writeTextFile: vi.fn().mockResolvedValue({ _meta: undefined }), findFiles: vi.fn().mockReturnValue([]), }); describe('AcpFileSystemService', () => { - describe('detectFileBOM', () => { - it('detects BOM through ACP client when content starts with U+FEFF', async () => { + describe('readTextFile', () => { + it('reads through ACP and returns response', async () => { + const mockResponse = { + content: 'hello', + _meta: { bom: false, encoding: 'utf-8' }, + }; const client = { - readTextFile: vi - .fn() - .mockResolvedValue({ content: '\ufeff// BOM file' }), + readTextFile: vi.fn().mockResolvedValue(mockResponse), } as unknown as AgentSideConnection; const svc = new AcpFileSystemService( @@ -38,78 +39,15 @@ describe('AcpFileSystemService', () => { createFallback(), ); - const result = await svc.detectFileBOM('/test/file.txt'); - expect(result).toBe(true); + const result = await svc.readTextFile({ path: '/some/file.txt' }); + + expect(result).toEqual(mockResponse); expect(client.readTextFile).toHaveBeenCalledWith({ - path: '/test/file.txt', + path: '/some/file.txt', sessionId: 'session-1', - limit: 1, }); }); - it('detects no BOM through ACP client when content does not start with U+FEFF', async () => { - const client = { - readTextFile: vi.fn().mockResolvedValue({ content: '// No BOM file' }), - } as unknown as AgentSideConnection; - - const svc = new AcpFileSystemService( - client, - 'session-2', - { readTextFile: true, writeTextFile: true }, - createFallback(), - ); - - const result = await svc.detectFileBOM('/test/file.txt'); - expect(result).toBe(false); - }); - - it('falls back to local filesystem when ACP client fails', async () => { - const client = { - readTextFile: vi.fn().mockRejectedValue(new Error('Network error')), - } as unknown as AgentSideConnection; - - const fallback = createFallback(); - (fallback.detectFileBOM as ReturnType).mockResolvedValue( - true, - ); - - const svc = new AcpFileSystemService( - client, - 'session-3', - { readTextFile: true, writeTextFile: true }, - fallback, - ); - - const result = await svc.detectFileBOM('/test/file.txt'); - expect(result).toBe(true); - expect(fallback.detectFileBOM).toHaveBeenCalledWith('/test/file.txt'); - }); - - it('falls back to local filesystem when readTextFile capability is disabled', async () => { - const client = { - readTextFile: vi.fn(), - } as unknown as AgentSideConnection; - - const fallback = createFallback(); - (fallback.detectFileBOM as ReturnType).mockResolvedValue( - false, - ); - - const svc = new AcpFileSystemService( - client, - 'session-4', - { readTextFile: false, writeTextFile: true }, - fallback, - ); - - const result = await svc.detectFileBOM('/test/file.txt'); - expect(result).toBe(false); - expect(fallback.detectFileBOM).toHaveBeenCalledWith('/test/file.txt'); - expect(client.readTextFile).not.toHaveBeenCalled(); - }); - }); - - describe('readTextFile ENOENT handling', () => { it('converts RESOURCE_NOT_FOUND error to ENOENT', async () => { const resourceNotFoundError = { code: RESOURCE_NOT_FOUND_CODE, @@ -126,7 +64,9 @@ describe('AcpFileSystemService', () => { createFallback(), ); - await expect(svc.readTextFile('/some/file.txt')).rejects.toMatchObject({ + await expect( + svc.readTextFile({ path: '/some/file.txt' }), + ).rejects.toMatchObject({ code: 'ENOENT', errno: -2, path: '/some/file.txt', @@ -149,7 +89,9 @@ describe('AcpFileSystemService', () => { createFallback(), ); - await expect(svc.readTextFile('/some/file.txt')).rejects.toMatchObject({ + await expect( + svc.readTextFile({ path: '/some/file.txt' }), + ).rejects.toMatchObject({ code: INTERNAL_ERROR_CODE, message: 'Internal error', }); @@ -161,8 +103,12 @@ describe('AcpFileSystemService', () => { } as unknown as AgentSideConnection; const fallback = createFallback(); + const fallbackResponse = { + content: 'fallback content', + _meta: { bom: false, encoding: 'utf-8' }, + }; (fallback.readTextFile as ReturnType).mockResolvedValue( - 'fallback content', + fallbackResponse, ); const svc = new AcpFileSystemService( @@ -172,10 +118,12 @@ describe('AcpFileSystemService', () => { fallback, ); - const result = await svc.readTextFile('/some/file.txt'); + const result = await svc.readTextFile({ path: '/some/file.txt' }); - expect(result).toBe('fallback content'); - expect(fallback.readTextFile).toHaveBeenCalledWith('/some/file.txt'); + expect(result).toEqual(fallbackResponse); + expect(fallback.readTextFile).toHaveBeenCalledWith({ + path: '/some/file.txt', + }); expect(client.readTextFile).not.toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/acp-integration/service/filesystem.ts b/packages/cli/src/acp-integration/service/filesystem.ts index 25ad296fb..201c86808 100644 --- a/packages/cli/src/acp-integration/service/filesystem.ts +++ b/packages/cli/src/acp-integration/service/filesystem.ts @@ -7,15 +7,38 @@ import type { AgentSideConnection, FileSystemCapability, + ReadTextFileRequest, + WriteTextFileRequest, + WriteTextFileResponse, } from '@agentclientprotocol/sdk'; import { RequestError } from '@agentclientprotocol/sdk'; import type { - FileReadResult, FileSystemService, + ReadTextFileResponse, } from '@qwen-code/qwen-code-core'; const RESOURCE_NOT_FOUND_CODE = -32002; +function getErrorCode(error: unknown): unknown { + if (error instanceof RequestError) { + return error.code; + } + + if (typeof error === 'object' && error !== null && 'code' in error) { + return (error as { code?: unknown }).code; + } + + return undefined; +} + +function createEnoentError(filePath: string): NodeJS.ErrnoException { + const err = new Error(`File not found: ${filePath}`) as NodeJS.ErrnoException; + err.code = 'ENOENT'; + err.errno = -2; + err.path = filePath; + return err; +} + export class AcpFileSystemService implements FileSystemService { constructor( private readonly connection: AgentSideConnection, @@ -24,82 +47,50 @@ export class AcpFileSystemService implements FileSystemService { private readonly fallback: FileSystemService, ) {} - async readTextFile(filePath: string): Promise { + async readTextFile( + params: Omit, + ): Promise { if (!this.capabilities.readTextFile) { - return this.fallback.readTextFile(filePath); + return this.fallback.readTextFile(params); } - let response: { content: string }; + let response: ReadTextFileResponse; try { response = await this.connection.readTextFile({ - path: filePath, + ...params, sessionId: this.sessionId, }); } catch (error) { - const errorCode = - error instanceof RequestError - ? error.code - : typeof error === 'object' && error !== null && 'code' in error - ? (error as { code?: unknown }).code - : undefined; + const errorCode = getErrorCode(error); if (errorCode === RESOURCE_NOT_FOUND_CODE) { - const err = new Error( - `File not found: ${filePath}`, - ) as NodeJS.ErrnoException; - err.code = 'ENOENT'; - err.errno = -2; - err.path = filePath; - throw err; + throw createEnoentError(params.path); } throw error; } - return response.content; - } - - async readTextFileWithInfo(filePath: string): Promise { - // ACP protocol does not expose encoding metadata; delegate to the local - // fallback which performs a single-pass read with encoding detection. - return this.fallback.readTextFileWithInfo(filePath); + return response; } async writeTextFile( - filePath: string, - content: string, - options?: { bom?: boolean; encoding?: string }, - ): Promise { + params: Omit, + ): Promise { if (!this.capabilities.writeTextFile) { - return this.fallback.writeTextFile(filePath, content, options); + return this.fallback.writeTextFile(params); } - const finalContent = options?.bom ? '\uFEFF' + content : content; + const finalContent = params._meta?.['bom'] + ? '\uFEFF' + params.content + : params.content; await this.connection.writeTextFile({ - path: filePath, + ...params, content: finalContent, sessionId: this.sessionId, }); - } - async detectFileBOM(filePath: string): Promise { - if (this.capabilities.readTextFile) { - try { - const response = await this.connection.readTextFile({ - path: filePath, - sessionId: this.sessionId, - limit: 1, - }); - return ( - response.content.length > 0 && - response.content.codePointAt(0) === 0xfeff - ); - } catch { - // Fall through to fallback if ACP read fails - } - } - return this.fallback.detectFileBOM(filePath); + return { _meta: params._meta }; } findFiles(fileName: string, searchPaths: readonly string[]): string[] { diff --git a/packages/cli/src/commands/mcp/add.ts b/packages/cli/src/commands/mcp/add.ts index 29fe25b88..57c5b3ce2 100644 --- a/packages/cli/src/commands/mcp/add.ts +++ b/packages/cli/src/commands/mcp/add.ts @@ -174,6 +174,7 @@ export const addCommand: CommandModule = { describe: 'Set environment variables (e.g. -e KEY=value)', type: 'array', string: true, + nargs: 1, }) .option('header', { alias: 'H', @@ -181,6 +182,7 @@ export const addCommand: CommandModule = { 'Set HTTP headers for SSE and HTTP transports (e.g. -H "X-Api-Key: abc123" -H "Authorization: Bearer abc123")', type: 'array', string: true, + nargs: 1, }) .option('timeout', { describe: 'Set connection timeout in milliseconds', diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 2234c9ea4..9550932c9 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -348,7 +348,7 @@ describe('Settings Loading and Merging', () => { fileName: 'WORKSPACE_CONTEXT.md', }, mcp: { - allowed: ['server1', 'server2'], + allowed: ['server1', 'server2', 'server3', 'server1', 'server2'], }, }); }); @@ -1474,8 +1474,8 @@ describe('Settings Loading and Merging', () => { const settings = loadSettings(MOCK_WORKSPACE_DIR); expect(settings.merged.mcp).toEqual({ - allowed: ['system-allowed'], - excluded: ['workspace-excluded'], + allowed: ['user-allowed', 'workspace-allowed', 'system-allowed'], + excluded: ['user-excluded', 'workspace-excluded'], }); }); diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 3ce34edc1..0809cf090 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -14,8 +14,6 @@ import { QWEN_DIR, getErrorMessage, Storage, - setDebugLogSession, - sanitizeCwd, createDebugLogger, } from '@qwen-code/qwen-code-core'; import stripJsonComments from 'strip-json-comments'; @@ -476,16 +474,6 @@ export function loadEnvironment(settings: Settings): void { export function loadSettings( workspaceDir: string = process.cwd(), ): LoadedSettings { - // Set up a temporary debug log session for the startup phase. - // This allows migration errors to be logged to file instead of being - // exposed to users via stderr. The Config class will override this - // with the actual session once initialized. - const resolvedWorkspaceDir = path.resolve(workspaceDir); - const sanitizedProjectId = sanitizeCwd(resolvedWorkspaceDir); - setDebugLogSession({ - getSessionId: () => `startup-${sanitizedProjectId}`, - }); - let systemSettings: Settings = {}; let systemDefaultSettings: Settings = {}; let userSettings: Settings = {}; @@ -496,7 +484,7 @@ export function loadSettings( const migratedInMemorScopes = new Set(); // Resolve paths to their canonical representation to handle symlinks - // Note: resolvedWorkspaceDir is already defined at the top of the function + const resolvedWorkspaceDir = path.resolve(workspaceDir); const resolvedHomeDir = path.resolve(homedir()); let realWorkspaceDir = resolvedWorkspaceDir; diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 4a84e8a45..5d6816806 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -998,6 +998,7 @@ const SETTINGS_SCHEMA = { default: undefined as string[] | undefined, description: 'A list of MCP servers to allow.', showInDialog: false, + mergeStrategy: MergeStrategy.CONCAT, }, excluded: { type: 'array', @@ -1007,6 +1008,7 @@ const SETTINGS_SCHEMA = { default: undefined as string[] | undefined, description: 'A list of MCP servers to exclude.', showInDialog: false, + mergeStrategy: MergeStrategy.CONCAT, }, }, }, diff --git a/packages/cli/src/i18n/locales/de.js b/packages/cli/src/i18n/locales/de.js index 9a007a68f..455411096 100644 --- a/packages/cli/src/i18n/locales/de.js +++ b/packages/cli/src/i18n/locales/de.js @@ -745,6 +745,15 @@ export default { "Authentifizierung mit MCP-Server '{{name}}' fehlgeschlagen: {{error}}", "Re-discovering tools from '{{name}}'...": "Werkzeuge von '{{name}}' werden neu erkannt...", + "Discovered {{count}} tool(s) from '{{name}}'.": + "{{count}} Werkzeug(e) von '{{name}}' entdeckt.", + 'Authentication complete. Returning to server details...': + 'Authentifizierung abgeschlossen. Zurück zu den Serverdetails...', + 'Authentication successful.': 'Authentifizierung erfolgreich.', + 'If the browser does not open, copy and paste this URL into your browser:': + 'Falls der Browser sich nicht öffnet, kopieren Sie diese URL und fügen Sie sie in Ihren Browser ein:', + 'Make sure to copy the COMPLETE URL - it may wrap across multiple lines.': + '⚠️ Stellen Sie sicher, dass Sie die VOLLSTÄNDIGE URL kopieren – sie kann über mehrere Zeilen gehen.', // ============================================================================ // Commands - Chat @@ -916,6 +925,8 @@ export default { Disable: 'Deaktivieren', Enable: 'Aktivieren', Authenticate: 'Authentifizieren', + 'Re-authenticate': 'Erneut authentifizieren', + 'Clear Authentication': 'Authentifizierung löschen', disabled: 'deaktiviert', 'Server:': 'Server:', Reconnect: 'Neu verbinden', diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index 768506c06..cbaee7889 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -811,6 +811,15 @@ export default { "Failed to authenticate with MCP server '{{name}}': {{error}}", "Re-discovering tools from '{{name}}'...": "Re-discovering tools from '{{name}}'...", + "Discovered {{count}} tool(s) from '{{name}}'.": + "Discovered {{count}} tool(s) from '{{name}}'.", + 'Authentication complete. Returning to server details...': + 'Authentication complete. Returning to server details...', + 'Authentication successful.': 'Authentication successful.', + 'If the browser does not open, copy and paste this URL into your browser:': + 'If the browser does not open, copy and paste this URL into your browser:', + 'Make sure to copy the COMPLETE URL - it may wrap across multiple lines.': + 'Make sure to copy the COMPLETE URL - it may wrap across multiple lines.', // ============================================================================ // MCP Management Dialog @@ -843,6 +852,8 @@ export default { Enable: 'Enable', Disable: 'Disable', Authenticate: 'Authenticate', + 'Re-authenticate': 'Re-authenticate', + 'Clear Authentication': 'Clear Authentication', 'Server:': 'Server:', 'Command:': 'Command:', 'Working Directory:': 'Working Directory:', diff --git a/packages/cli/src/i18n/locales/ja.js b/packages/cli/src/i18n/locales/ja.js index 3a1bf21c6..02898e8bc 100644 --- a/packages/cli/src/i18n/locales/ja.js +++ b/packages/cli/src/i18n/locales/ja.js @@ -507,6 +507,15 @@ export default { "MCPサーバー '{{name}}' での認証に失敗: {{error}}", "Re-discovering tools from '{{name}}'...": "'{{name}}' からツールを再検出中...", + "Discovered {{count}} tool(s) from '{{name}}'.": + "'{{name}}' から {{count}} 個のツールを検出しました。", + 'Authentication complete. Returning to server details...': + '認証完了。サーバー詳細に戻ります...', + 'Authentication successful.': '認証成功。', + 'If the browser does not open, copy and paste this URL into your browser:': + 'ブラウザが開かない場合は、このURLをコピーしてブラウザに貼り付けてください:', + 'Make sure to copy the COMPLETE URL - it may wrap across multiple lines.': + '⚠️ URL全体をコピーしてください——複数行にまたがる場合があります。', 'Configured MCP servers:': '設定済みMCPサーバー:', Ready: '準備完了', Disconnected: '切断', @@ -655,6 +664,8 @@ export default { Disable: '無効化', Enable: '有効化', Authenticate: '認証', + 'Re-authenticate': '再認証', + 'Clear Authentication': '認証をクリア', disabled: '無効', 'Server:': 'サーバー:', Reconnect: '再接続', diff --git a/packages/cli/src/i18n/locales/pt.js b/packages/cli/src/i18n/locales/pt.js index 37efeda6f..7492fe5f4 100644 --- a/packages/cli/src/i18n/locales/pt.js +++ b/packages/cli/src/i18n/locales/pt.js @@ -751,6 +751,15 @@ export default { "Falha ao autenticar com o servidor MCP '{{name}}': {{error}}", "Re-discovering tools from '{{name}}'...": "Redescobrindo ferramentas de '{{name}}'...", + "Discovered {{count}} tool(s) from '{{name}}'.": + "{{count}} ferramenta(s) descoberta(s) de '{{name}}'.", + 'Authentication complete. Returning to server details...': + 'Autenticação concluída. Retornando aos detalhes do servidor...', + 'Authentication successful.': 'Autenticação bem-sucedida.', + 'If the browser does not open, copy and paste this URL into your browser:': + 'Se o navegador não abrir, copie e cole esta URL no seu navegador:', + 'Make sure to copy the COMPLETE URL - it may wrap across multiple lines.': + '⚠️ Certifique-se de copiar a URL COMPLETA – ela pode ocupar várias linhas.', // ============================================================================ // Commands - Chat @@ -922,6 +931,8 @@ export default { Disable: 'Desativar', Enable: 'Ativar', Authenticate: 'Autenticar', + 'Re-authenticate': 'Reautenticar', + 'Clear Authentication': 'Limpar autenticação', disabled: 'desativado', 'Server:': 'Servidor:', Reconnect: 'Reconectar', diff --git a/packages/cli/src/i18n/locales/ru.js b/packages/cli/src/i18n/locales/ru.js index eaecb4228..66cf2791d 100644 --- a/packages/cli/src/i18n/locales/ru.js +++ b/packages/cli/src/i18n/locales/ru.js @@ -754,6 +754,15 @@ export default { "Не удалось авторизоваться на MCP-сервере '{{name}}': {{error}}", "Re-discovering tools from '{{name}}'...": "Повторное обнаружение инструментов от '{{name}}'...", + "Discovered {{count}} tool(s) from '{{name}}'.": + "Обнаружено {{count}} инструмент(ов) от '{{name}}'.", + 'Authentication complete. Returning to server details...': + 'Аутентификация завершена. Возврат к деталям сервера...', + 'Authentication successful.': 'Аутентификация успешна.', + 'If the browser does not open, copy and paste this URL into your browser:': + 'Если браузер не открылся, скопируйте этот URL и вставьте его в браузер:', + 'Make sure to copy the COMPLETE URL - it may wrap across multiple lines.': + '⚠️ Убедитесь, что скопировали ПОЛНЫЙ URL — он может занимать несколько строк.', // ============================================================================ // Команды - Чат @@ -900,6 +909,8 @@ export default { Disable: 'Отключить', Enable: 'Включить', Authenticate: 'Аутентификация', + 'Re-authenticate': 'Повторная аутентификация', + 'Clear Authentication': 'Очистить аутентификацию', disabled: 'отключен', 'Server:': 'Сервер:', Reconnect: 'Переподключить', diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index 2c078e258..b1086a72d 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -138,7 +138,7 @@ export default { '在所选作用域中未找到主题 "{{themeName}}"。', 'Clear conversation history and free up context': '清除对话历史并释放上下文', 'Compresses the context by replacing it with a summary.': - '通过用摘要替换来压缩上下文', + '通过摘要替换来压缩上下文', 'open full Qwen Code documentation in your browser': '在浏览器中打开完整的 Qwen Code 文档', 'Configuration not available.': '配置不可用', @@ -763,6 +763,15 @@ export default { "认证 MCP 服务器 '{{name}}' 失败:{{error}}", "Re-discovering tools from '{{name}}'...": "正在重新发现 '{{name}}' 的工具...", + "Discovered {{count}} tool(s) from '{{name}}'.": + "从 '{{name}}' 发现了 {{count}} 个工具。", + 'Authentication complete. Returning to server details...': + '认证完成,正在返回服务器详情...', + 'Authentication successful.': '认证成功。', + 'If the browser does not open, copy and paste this URL into your browser:': + '如果浏览器未自动打开,请复制以下 URL 并粘贴到浏览器中:', + 'Make sure to copy the COMPLETE URL - it may wrap across multiple lines.': + '⚠️ 请确保复制完整的 URL —— 它可能跨越多行。', // ============================================================================ // MCP Management Dialog @@ -793,6 +802,8 @@ export default { Enable: '启用', Disable: '禁用', Authenticate: '认证', + 'Re-authenticate': '重新认证', + 'Clear Authentication': '清空认证', disabled: '已禁用', 'Server:': '服务器:', '(disabled)': '(已禁用)', diff --git a/packages/cli/src/ui/auth/AuthDialog.tsx b/packages/cli/src/ui/auth/AuthDialog.tsx index 309e77adf..4469a0759 100644 --- a/packages/cli/src/ui/auth/AuthDialog.tsx +++ b/packages/cli/src/ui/auth/AuthDialog.tsx @@ -345,7 +345,7 @@ export function AuthDialog(): React.JSX.Element { return ( ({ - loadLastSession: vi.fn(), + loadSession: vi.fn(), })); vi.mock('@qwen-code/qwen-code-core', () => { class SessionService { constructor(_cwd: string) {} - async loadLastSession() { - return mockSessionServiceMocks.loadLastSession(); + async loadSession(_sessionId: string) { + return mockSessionServiceMocks.loadSession(); } } @@ -68,13 +68,14 @@ describe('exportCommand', () => { beforeEach(() => { vi.clearAllMocks(); - mockSessionServiceMocks.loadLastSession.mockResolvedValue(mockSessionData); + mockSessionServiceMocks.loadSession.mockResolvedValue(mockSessionData); mockContext = createMockCommandContext({ services: { config: { getWorkingDir: vi.fn().mockReturnValue('/test/dir'), getProjectRoot: vi.fn().mockReturnValue('/test/project'), + getSessionId: vi.fn().mockReturnValue('test-session-id'), }, }, }); @@ -132,7 +133,7 @@ describe('exportCommand', () => { content: expect.stringContaining('export-2025-01-01T00-00-00-000Z.md'), }); - expect(mockSessionServiceMocks.loadLastSession).toHaveBeenCalled(); + expect(mockSessionServiceMocks.loadSession).toHaveBeenCalled(); expect(collectSessionData).toHaveBeenCalledWith( mockSessionData.conversation, expect.anything(), @@ -191,7 +192,7 @@ describe('exportCommand', () => { }); it('should return error when no session is found', async () => { - mockSessionServiceMocks.loadLastSession.mockResolvedValue(undefined); + mockSessionServiceMocks.loadSession.mockResolvedValue(undefined); const mdCommand = exportCommand.subCommands?.find((c) => c.name === 'md'); if (!mdCommand?.action) { @@ -260,7 +261,7 @@ describe('exportCommand', () => { ), }); - expect(mockSessionServiceMocks.loadLastSession).toHaveBeenCalled(); + expect(mockSessionServiceMocks.loadSession).toHaveBeenCalled(); expect(collectSessionData).toHaveBeenCalledWith( mockSessionData.conversation, expect.anything(), @@ -323,7 +324,7 @@ describe('exportCommand', () => { }); it('should return error when no session is found', async () => { - mockSessionServiceMocks.loadLastSession.mockResolvedValue(undefined); + mockSessionServiceMocks.loadSession.mockResolvedValue(undefined); const htmlCommand = exportCommand.subCommands?.find( (c) => c.name === 'html', diff --git a/packages/cli/src/ui/commands/exportCommand.ts b/packages/cli/src/ui/commands/exportCommand.ts index 42af225ac..8edec9f4d 100644 --- a/packages/cli/src/ui/commands/exportCommand.ts +++ b/packages/cli/src/ui/commands/exportCommand.ts @@ -50,9 +50,10 @@ async function exportMarkdownAction( } try { - // Load the current session + // Load the current session using the current session ID const sessionService = new SessionService(cwd); - const sessionData = await sessionService.loadLastSession(); + const sessionId = config.getSessionId(); + const sessionData = await sessionService.loadSession(sessionId); if (!sessionData) { return { @@ -122,9 +123,10 @@ async function exportHtmlAction( } try { - // Load the current session + // Load the current session using the current session ID const sessionService = new SessionService(cwd); - const sessionData = await sessionService.loadLastSession(); + const sessionId = config.getSessionId(); + const sessionData = await sessionService.loadSession(sessionId); if (!sessionData) { return { @@ -194,9 +196,10 @@ async function exportJsonAction( } try { - // Load the current session + // Load the current session using the current session ID const sessionService = new SessionService(cwd); - const sessionData = await sessionService.loadLastSession(); + const sessionId = config.getSessionId(); + const sessionData = await sessionService.loadSession(sessionId); if (!sessionData) { return { @@ -266,9 +269,10 @@ async function exportJsonlAction( } try { - // Load the current session + // Load the current session using the current session ID const sessionService = new SessionService(cwd); - const sessionData = await sessionService.loadLastSession(); + const sessionId = config.getSessionId(); + const sessionData = await sessionService.loadSession(sessionId); if (!sessionData) { return { diff --git a/packages/cli/src/ui/components/Header.test.tsx b/packages/cli/src/ui/components/Header.test.tsx index 99bb053da..72da62aba 100644 --- a/packages/cli/src/ui/components/Header.test.tsx +++ b/packages/cli/src/ui/components/Header.test.tsx @@ -78,7 +78,7 @@ describe('
', () => { it('renders with border around info panel', () => { const { lastFrame } = render(
); - expect(lastFrame()).toContain('╭'); - expect(lastFrame()).toContain('╯'); + expect(lastFrame()).toContain('┌'); + expect(lastFrame()).toContain('┐'); }); }); diff --git a/packages/cli/src/ui/components/Header.tsx b/packages/cli/src/ui/components/Header.tsx index 45fce4385..2d919385f 100644 --- a/packages/cli/src/ui/components/Header.tsx +++ b/packages/cli/src/ui/components/Header.tsx @@ -128,7 +128,7 @@ export const Header: React.FC = ({ {/* Right side: Info panel (flexible width, max 60 in two-column layout) */} ({ useKeypress: vi.fn(), })); -// Mock qrcode-terminal module -vi.mock('qrcode-terminal', () => ({ - default: { - generate: vi.fn(), - }, -})); - -// Mock ink-spinner -vi.mock('ink-spinner', () => ({ - default: ({ type }: { type: string }) => `MockSpinner(${type})`, -})); - // Mock ink-link vi.mock('ink-link', () => ({ default: ({ children }: { children: React.ReactNode; url: string }) => @@ -95,19 +83,17 @@ describe('QwenOAuthProgress', () => { const { lastFrame } = renderComponent(); const output = lastFrame(); - expect(output).toContain('MockSpinner(dots)'); expect(output).toContain('Waiting for Qwen OAuth authentication...'); - expect(output).toContain('(Press ESC or CTRL+C to cancel)'); + expect(output).toContain('Esc to cancel'); }); - it('should render loading state with gray border', () => { + it('should render loading state with single border', () => { const { lastFrame } = renderComponent(); const output = lastFrame(); - // Should not contain auth flow elements - expect(output).not.toContain('Qwen OAuth Authentication'); - expect(output).not.toContain('Please visit this URL to authorize:'); - // Loading state still shows time remaining with default timeout + // Should contain the auth title even in loading state + expect(output).toContain('Qwen OAuth Authentication'); + // Loading state shows time remaining with default timeout expect(output).toContain('Time remaining:'); }); }); @@ -117,44 +103,20 @@ describe('QwenOAuthProgress', () => { const { lastFrame } = renderComponent({ deviceAuth: mockDeviceAuth }); const output = lastFrame(); - // Initially no QR code shown until it's generated, but the status area should be visible - expect(output).toContain('MockSpinner(dots)'); expect(output).toContain('Waiting for authorization'); expect(output).toContain('Time remaining: 5:00'); - expect(output).toContain('(Press ESC or CTRL+C to cancel)'); + expect(output).toContain('Esc to cancel'); }); - it('should display correct URL in Static component when QR code is generated', async () => { - const qrcode = await import('qrcode-terminal'); - const mockGenerate = vi.mocked(qrcode.default.generate); - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let qrCallback: any = null; - mockGenerate.mockImplementation((url, options, callback) => { - qrCallback = callback; - }); - + it('should display correct URL in auth URL display', () => { const customAuth = createMockDeviceAuth({ verification_uri_complete: 'https://custom.com/auth?code=XYZ789', }); - const { lastFrame, rerender } = renderComponent({ + const { lastFrame } = renderComponent({ deviceAuth: customAuth, }); - // Manually trigger the QR code callback - if (qrCallback && typeof qrCallback === 'function') { - qrCallback('Mock QR Code Data'); - } - - rerender( - , - ); - expect(lastFrame()).toContain('https://custom.com/auth?code=XYZ789'); }); @@ -282,10 +244,11 @@ describe('QwenOAuthProgress', () => { />, ); - // Initial state should have no dots - expect(lastFrame()).toContain('Waiting for authorization'); + // Initial state should show '...' (default value) + const initialOutput = lastFrame(); + expect(initialOutput).toContain('Waiting for authorization'); - // Advance by 500ms to add first dot + // Advance by 500ms to cycle animation vi.advanceTimersByTime(500); rerender( { deviceAuth={mockDeviceAuth} />, ); - expect(lastFrame()).toContain('Waiting for authorization.'); + const after500ms = lastFrame(); + expect(after500ms).toContain('Waiting for authorization'); - // Advance by another 500ms to add second dot + // Advance by another 500ms to continue animation vi.advanceTimersByTime(500); rerender( { deviceAuth={mockDeviceAuth} />, ); - expect(lastFrame()).toContain('Waiting for authorization..'); + const after1000ms = lastFrame(); + expect(after1000ms).toContain('Waiting for authorization'); - // Advance by another 500ms to add third dot + // Advance by another 500ms to complete cycle vi.advanceTimersByTime(500); rerender( { deviceAuth={mockDeviceAuth} />, ); - expect(lastFrame()).toContain('Waiting for authorization...'); - - // Advance by another 500ms to reset dots - vi.advanceTimersByTime(500); - rerender( - , - ); - expect(lastFrame()).toContain('Waiting for authorization'); - }); - }); - - describe('QR Code functionality', () => { - it('should generate QR code when deviceAuth is provided', async () => { - const qrcode = await import('qrcode-terminal'); - const mockGenerate = vi.mocked(qrcode.default.generate); - - mockGenerate.mockImplementation((url, options, callback) => { - callback!('Mock QR Code Data'); - }); - - render( - , - ); - - expect(mockGenerate).toHaveBeenCalledWith( - mockDeviceAuth.verification_uri_complete, - { small: true }, - expect.any(Function), - ); - }); - - it('should display QR code in Static component when available', async () => { - const qrcode = await import('qrcode-terminal'); - const mockGenerate = vi.mocked(qrcode.default.generate); - - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let qrCallback: any = null; - mockGenerate.mockImplementation((url, options, callback) => { - qrCallback = callback; - }); - - const { lastFrame, rerender } = render( - , - ); - - // Manually trigger the QR code callback - if (qrCallback && typeof qrCallback === 'function') { - qrCallback('Mock QR Code Data'); - } - - rerender( - , - ); - - const output = lastFrame(); - expect(output).toContain('Or scan the QR code below:'); - expect(output).toContain('Mock QR Code Data'); - }); - - it('should handle QR code generation errors gracefully', async () => { - const qrcode = await import('qrcode-terminal'); - const mockGenerate = vi.mocked(qrcode.default.generate); - mockGenerate.mockImplementation(() => { - throw new Error('QR Code generation failed'); - }); - - const { lastFrame } = render( - , - ); - - // Should not crash and should not show QR code section since QR generation failed - const output = lastFrame(); - expect(output).not.toContain('Or scan the QR code below:'); - }); - - it('should not generate QR code when deviceAuth is null', async () => { - const qrcode = await import('qrcode-terminal'); - const mockGenerate = vi.mocked(qrcode.default.generate); - - render( - , - ); - - expect(mockGenerate).not.toHaveBeenCalled(); + const after1500ms = lastFrame(); + expect(after1500ms).toContain('Waiting for authorization'); }); }); diff --git a/packages/cli/src/ui/components/QwenOAuthProgress.tsx b/packages/cli/src/ui/components/QwenOAuthProgress.tsx index 69d42818d..7655e7915 100644 --- a/packages/cli/src/ui/components/QwenOAuthProgress.tsx +++ b/packages/cli/src/ui/components/QwenOAuthProgress.tsx @@ -5,14 +5,11 @@ */ import type React from 'react'; -import { useState, useEffect, useMemo } from 'react'; +import { useState, useEffect } from 'react'; import { Box, Text } from 'ink'; -import Spinner from 'ink-spinner'; import Link from 'ink-link'; -import qrcode from 'qrcode-terminal'; -import { Colors } from '../colors.js'; +import { theme } from '../semantic-colors.js'; import type { DeviceAuthorizationData } from '@qwen-code/qwen-code-core'; -import { createDebugLogger } from '@qwen-code/qwen-code-core'; import { useKeypress } from '../hooks/useKeypress.js'; import { t } from '../../i18n/index.js'; @@ -30,98 +27,10 @@ interface QwenOAuthProgressProps { authMessage?: string | null; } -const debugLogger = createDebugLogger('QWEN_OAUTH_PROGRESS'); - -/** - * Static QR Code Display Component - * Renders the QR code and URL once and doesn't re-render unless the URL changes - */ -function QrCodeDisplay({ - verificationUrl, - qrCodeData, -}: { - verificationUrl: string; - qrCodeData: string | null; -}): React.JSX.Element | null { - if (!qrCodeData) { - return null; - } - - return ( - - - {t('Qwen OAuth Authentication')} - - - - {t('Please visit this URL to authorize:')} - - - - - {verificationUrl} - - - - - {t('Or scan the QR code below:')} - - - - {qrCodeData} - - - ); -} - -/** - * Dynamic Status Display Component - * Shows the loading spinner, timer, and status messages - */ -function StatusDisplay({ - timeRemaining, - dots, -}: { - timeRemaining: number; - dots: string; -}): React.JSX.Element { - const formatTime = (seconds: number): string => { - const minutes = Math.floor(seconds / 60); - const remainingSeconds = seconds % 60; - return `${minutes}:${remainingSeconds.toString().padStart(2, '0')}`; - }; - - return ( - - - - {t('Waiting for authorization')} - {dots} - - - - - - {t('Time remaining:')} {formatTime(timeRemaining)} - - - {t('(Press ESC or CTRL+C to cancel)')} - - - - ); +function formatTime(seconds: number): string { + const minutes = Math.floor(seconds / 60); + const remainingSeconds = seconds % 60; + return `${minutes}:${remainingSeconds.toString().padStart(2, '0')}`; } export function QwenOAuthProgress({ @@ -133,13 +42,11 @@ export function QwenOAuthProgress({ }: QwenOAuthProgressProps): React.JSX.Element { const defaultTimeout = deviceAuth?.expires_in || 300; // Default 5 minutes const [timeRemaining, setTimeRemaining] = useState(defaultTimeout); - const [dots, setDots] = useState(''); - const [qrCodeData, setQrCodeData] = useState(null); + const [dots, setDots] = useState('...'); useKeypress( (key) => { if (authStatus === 'timeout' || authStatus === 'error') { - // Any key press in timeout or error state should trigger cancel to return to auth dialog onCancel(); } else if (key.name === 'escape' || (key.ctrl && key.name === 'c')) { onCancel(); @@ -148,30 +55,6 @@ export function QwenOAuthProgress({ { isActive: true }, ); - // Generate QR code once when device auth is available - useEffect(() => { - if (!deviceAuth?.verification_uri_complete) { - return; - } - - const generateQR = () => { - try { - qrcode.generate( - deviceAuth.verification_uri_complete, - { small: true }, - (qrcode: string) => { - setQrCodeData(qrcode); - }, - ); - } catch (error) { - debugLogger.error('Failed to generate QR code:', error); - setQrCodeData(null); - } - }; - - generateQR(); - }, [deviceAuth?.verification_uri_complete]); - // Countdown timer useEffect(() => { const timer = setInterval(() => { @@ -187,41 +70,29 @@ export function QwenOAuthProgress({ return () => clearInterval(timer); }, [onTimeout]); - // Animated dots + // Animated dots — cycle through fixed-width patterns to avoid layout shift useEffect(() => { + const dotFrames = ['. ', '.. ', '...']; + let frameIndex = 0; const dotsTimer = setInterval(() => { - setDots((prev) => { - if (prev.length >= 3) return ''; - return prev + '.'; - }); + frameIndex = (frameIndex + 1) % dotFrames.length; + setDots(dotFrames[frameIndex]!); }, 500); return () => clearInterval(dotsTimer); }, []); - // Memoize the QR code display to prevent unnecessary re-renders - const qrCodeDisplay = useMemo(() => { - if (!deviceAuth?.verification_uri_complete) return null; - - return ( - - ); - }, [deviceAuth?.verification_uri_complete, qrCodeData]); - // Handle timeout state if (authStatus === 'timeout') { return ( - + {t('Qwen OAuth Authentication Timeout')} @@ -238,7 +109,7 @@ export function QwenOAuthProgress({ - + {t('Press any key to return to authentication type selection.')} @@ -249,26 +120,26 @@ export function QwenOAuthProgress({ if (authStatus === 'error') { return ( - - Qwen OAuth Authentication Error + + {t('Qwen OAuth Authentication Error')} {authMessage || - 'An error occurred during authentication. Please try again.'} + t('An error occurred during authentication. Please try again.')} - - Press any key to return to authentication type selection. + + {t('Press any key to return to authentication type selection.')} @@ -279,38 +150,61 @@ export function QwenOAuthProgress({ if (!deviceAuth) { return ( - + {t('Qwen OAuth Authentication')} + + + {t('Waiting for Qwen OAuth authentication...')} - - {t('Waiting for Qwen OAuth authentication...')} + {t('Time remaining:')} {formatTime(timeRemaining)} - - - {t('Time remaining:')} {Math.floor(timeRemaining / 60)}: - {(timeRemaining % 60).toString().padStart(2, '0')} - - - {t('(Press ESC or CTRL+C to cancel)')} - + + + {t('Esc to cancel')} ); } return ( - - {/* Static QR Code Display */} - {qrCodeDisplay} + + {t('Qwen OAuth Authentication')} - {/* Dynamic Status Display */} - + + {t('Please visit this URL to authorize:')} + + + + + {deviceAuth.verification_uri_complete} + + + + + + {t('Waiting for authorization')} + {dots} + + + {t('Time remaining:')} {formatTime(timeRemaining)} + + + + + {t('Esc to cancel')} + ); } diff --git a/packages/cli/src/ui/components/ShellInputPrompt.tsx b/packages/cli/src/ui/components/ShellInputPrompt.tsx index ff1f95d7f..a22869f78 100644 --- a/packages/cli/src/ui/components/ShellInputPrompt.tsx +++ b/packages/cli/src/ui/components/ShellInputPrompt.tsx @@ -9,6 +9,7 @@ import type React from 'react'; import { useKeypress } from '../hooks/useKeypress.js'; import { ShellExecutionService } from '@qwen-code/qwen-code-core'; import { keyToAnsi, type Key } from '../hooks/keyToAnsi.js'; +import { keyMatchers, Command } from '../keyMatchers.js'; export interface ShellInputPromptProps { activeShellPtyId: number | null; @@ -33,6 +34,11 @@ export const ShellInputPrompt: React.FC = ({ if (!focus || !activeShellPtyId) { return; } + // Don't forward Ctrl+F to the PTY — it's used to toggle shell focus. + // Without this, the raw ^F control character gets written to the shell. + if (keyMatchers[Command.TOGGLE_SHELL_INPUT_FOCUS](key)) { + return; + } if (key.ctrl && key.shift && key.name === 'up') { ShellExecutionService.scrollPty(activeShellPtyId, -1); return; diff --git a/packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx b/packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx index b9881932d..94910fd72 100644 --- a/packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx +++ b/packages/cli/src/ui/components/mcp/MCPManagementDialog.tsx @@ -25,6 +25,7 @@ import { useConfig } from '../../contexts/ConfigContext.js'; import { getMCPServerStatus, DiscoveredMCPTool, + MCPOAuthTokenStorage, type MCPServerConfig, type AnyDeclarativeTool, type DiscoveredMCPPrompt, @@ -95,16 +96,10 @@ export const MCPManagementDialog: React.FC = ({ let source: 'user' | 'project' | 'extension' = 'user'; if (serverConfig.extensionName) { source = 'extension'; - } - - // Determine the scope of the configuration - let scope: 'user' | 'workspace' | 'extension' = 'user'; - if (serverConfig.extensionName) { - scope = 'extension'; } else if (workspaceSettings.mcpServers?.[name]) { - scope = 'workspace'; + source = 'project'; } else if (userSettings.mcpServers?.[name]) { - scope = 'user'; + source = 'user'; } // Use config.isMcpServerDisabled() to check if server is disabled @@ -115,16 +110,26 @@ export const MCPManagementDialog: React.FC = ({ (t) => !t.name || !t.description, ).length; + // Check if OAuth tokens exist for this server + let hasOAuthTokens = false; + try { + const tokenStorage = new MCPOAuthTokenStorage(); + const credentials = await tokenStorage.getCredentials(name); + hasOAuthTokens = credentials !== null; + } catch { + // Ignore errors when checking token existence + } + serverInfos.push({ name, status, source, - scope, config: serverConfig, toolCount: serverTools.length, invalidToolCount, promptCount: serverPrompts.length, isDisabled, + hasOAuthTokens, }); } @@ -256,6 +261,36 @@ export const MCPManagementDialog: React.FC = ({ } }, [fetchServerData]); + // Clear OAuth authentication tokens and disconnect the server + const handleClearAuth = useCallback(async () => { + if (!config || !selectedServer) return; + + try { + setIsLoading(true); + const tokenStorage = new MCPOAuthTokenStorage(); + await tokenStorage.deleteCredentials(selectedServer.name); + debugLogger.info( + `Cleared OAuth tokens for server '${selectedServer.name}'`, + ); + + // Disconnect the server so it no longer appears as connected + const toolRegistry = config.getToolRegistry(); + if (toolRegistry) { + await toolRegistry.disconnectServer(selectedServer.name); + } + + // Reload to update hasOAuthTokens flag and server status + await reloadServers(); + } catch (error) { + debugLogger.error( + `Error clearing OAuth tokens for server '${selectedServer.name}':`, + error, + ); + } finally { + setIsLoading(false); + } + }, [config, selectedServer, reloadServers]); + // Reconnect server const handleReconnect = useCallback(async () => { if (!config || !selectedServer) return; @@ -343,7 +378,7 @@ export const MCPManagementDialog: React.FC = ({ // Determine the scope based on server configuration location let targetScope: 'user' | 'workspace' = 'user'; - if (server.scope === 'extension') { + if (server.source === 'extension') { // Extension servers should not be disabled through user/workspace settings // Show error message and return debugLogger.warn( @@ -351,7 +386,7 @@ export const MCPManagementDialog: React.FC = ({ ); setIsLoading(false); return; - } else if (server.scope === 'workspace') { + } else if (server.source === 'project') { targetScope = 'workspace'; } @@ -544,6 +579,7 @@ export const MCPManagementDialog: React.FC = ({ onReconnect={handleReconnect} onDisable={handleDisable} onAuthenticate={handleAuthenticate} + onClearAuth={handleClearAuth} onBack={handleNavigateBack} /> ); @@ -576,10 +612,10 @@ export const MCPManagementDialog: React.FC = ({ return ( { + onBack={() => { + handleNavigateBack(); void reloadServers(); }} - onBack={handleNavigateBack} /> ); @@ -601,6 +637,7 @@ export const MCPManagementDialog: React.FC = ({ handleReconnect, handleDisable, handleAuthenticate, + handleClearAuth, handleNavigateBack, handleSelectTool, handleSelectDisableScope, diff --git a/packages/cli/src/ui/components/mcp/steps/AuthenticateStep.tsx b/packages/cli/src/ui/components/mcp/steps/AuthenticateStep.tsx index e4d4e373a..6e0011a77 100644 --- a/packages/cli/src/ui/components/mcp/steps/AuthenticateStep.tsx +++ b/packages/cli/src/ui/components/mcp/steps/AuthenticateStep.tsx @@ -16,13 +16,15 @@ import { MCPOAuthTokenStorage, getErrorMessage, } from '@qwen-code/qwen-code-core'; +import type { OAuthDisplayPayload } from '@qwen-code/qwen-code-core'; import { appEvents, AppEvent } from '../../../../utils/events.js'; type AuthState = 'idle' | 'authenticating' | 'success' | 'error'; +const AUTO_BACK_DELAY_MS = 2000; + export const AuthenticateStep: React.FC = ({ server, - onSuccess, onBack, }) => { const config = useConfig(); @@ -39,9 +41,12 @@ export const AuthenticateStep: React.FC = ({ setMessages([]); setErrorMessage(null); - // Listen for OAuth display messages (same as mcpCommand.ts) - const displayListener = (message: string) => { - setMessages((prev) => [...prev, message]); + // Listen for OAuth display messages - supports both plain strings and + // structured i18n messages ({ key, params }) emitted by the core layer. + const displayListener = (message: OAuthDisplayPayload) => { + const text = + typeof message === 'string' ? message : t(message.key, message.params); + setMessages((prev) => [...prev, text]); }; appEvents.on(AppEvent.OauthDisplayMessage, displayListener); @@ -83,6 +88,16 @@ export const AuthenticateStep: React.FC = ({ }), ]); await toolRegistry.discoverToolsForServer(server.name); + + // Show discovered tool count + const discoveredTools = toolRegistry.getToolsByServer(server.name); + setMessages((prev) => [ + ...prev, + t("Discovered {{count}} tool(s) from '{{name}}'.", { + count: String(discoveredTools.length), + name: server.name, + }), + ]); } // Update the client with the new tools @@ -91,8 +106,12 @@ export const AuthenticateStep: React.FC = ({ await geminiClient.setTools(); } + setMessages((prev) => [ + ...prev, + t('Authentication complete. Returning to server details...'), + ]); + setAuthState('success'); - onSuccess?.(); } catch (error) { setErrorMessage(getErrorMessage(error)); setAuthState('error'); @@ -100,13 +119,22 @@ export const AuthenticateStep: React.FC = ({ isRunning.current = false; appEvents.removeListener(AppEvent.OauthDisplayMessage, displayListener); } - }, [server, config, onSuccess]); + }, [server, config]); useEffect(() => { runAuthentication(); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); + // Auto-navigate back after authentication succeeds + useEffect(() => { + if (authState !== 'success') return; + const timer = setTimeout(() => { + onBack(); + }, AUTO_BACK_DELAY_MS); + return () => clearTimeout(timer); + }, [authState, onBack]); + useKeypress( (key) => { if (key.name === 'escape') { @@ -158,6 +186,11 @@ export const AuthenticateStep: React.FC = ({ {t('Authenticating... Please complete the login in your browser.')} )} + {authState === 'success' && ( + + {t('Authentication successful.')} + + )} ); diff --git a/packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx b/packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx index d23dccf87..3718f5e87 100644 --- a/packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx +++ b/packages/cli/src/ui/components/mcp/steps/ServerDetailStep.tsx @@ -24,7 +24,8 @@ type ServerAction = | 'view-tools' | 'reconnect' | 'toggle-disable' - | 'authenticate'; + | 'authenticate' + | 'clear-auth'; export const ServerDetailStep: React.FC = ({ server, @@ -32,6 +33,7 @@ export const ServerDetailStep: React.FC = ({ onReconnect, onDisable, onAuthenticate, + onClearAuth, onBack, }) => { const statusColor = server @@ -77,15 +79,24 @@ export const ServerDetailStep: React.FC = ({ value: 'toggle-disable', }); - // 待补充准确的认证判断方案,暂时全部开放 + // 已认证的服务器显示"重新认证",未认证的显示"认证" if (!server.isDisabled) { result.push({ key: 'authenticate', - label: t('Authenticate'), + label: server.hasOAuthTokens ? t('Re-authenticate') : t('Authenticate'), value: 'authenticate', }); } + // 只在存储有 OAuth 认证信息时显示“清空认证”选项 + if (!server.isDisabled && server.hasOAuthTokens) { + result.push({ + key: 'clear-auth', + label: t('Clear Authentication'), + value: 'clear-auth', + }); + } + return result; }, [server]); @@ -136,9 +147,9 @@ export const ServerDetailStep: React.FC = ({ - {server.scope === 'user' + {server.source === 'user' ? t('User Settings') - : server.scope === 'workspace' + : server.source === 'project' ? t('Workspace Settings') : t('Extension')} @@ -222,6 +233,9 @@ export const ServerDetailStep: React.FC = ({ case 'authenticate': onAuthenticate?.(); break; + case 'clear-auth': + onClearAuth?.(); + break; default: break; } diff --git a/packages/cli/src/ui/components/mcp/types.ts b/packages/cli/src/ui/components/mcp/types.ts index 34374fa23..82d9ab7ba 100644 --- a/packages/cli/src/ui/components/mcp/types.ts +++ b/packages/cli/src/ui/components/mcp/types.ts @@ -34,8 +34,6 @@ export interface MCPServerDisplayInfo { status: MCPServerStatus; /** 来源类型 */ source: 'user' | 'project' | 'extension'; - /** 配置所在的 scope */ - scope: 'user' | 'workspace' | 'extension'; /** 配置文件路径 */ configPath?: string; /** 服务器配置 */ @@ -50,6 +48,8 @@ export interface MCPServerDisplayInfo { errorMessage?: string; /** 是否被禁用(在排除列表中) */ isDisabled: boolean; + /** 是否存储有 OAuth 认证信息 */ + hasOAuthTokens?: boolean; } /** @@ -134,6 +134,8 @@ export interface ServerDetailStepProps { onDisable?: () => void; /** OAuth 认证回调 */ onAuthenticate?: () => void; + /** 清空认证信息回调 */ + onClearAuth?: () => void; /** 返回回调 */ onBack: () => void; } @@ -180,8 +182,6 @@ export interface ToolDetailStepProps { export interface AuthenticateStepProps { /** 服务器信息 */ server: MCPServerDisplayInfo | null; - /** 认证成功回调 */ - onSuccess?: () => void; /** 返回回调 */ onBack: () => void; } diff --git a/packages/cli/src/ui/components/mcp/utils.test.ts b/packages/cli/src/ui/components/mcp/utils.test.ts index 3b058ba55..155195454 100644 --- a/packages/cli/src/ui/components/mcp/utils.test.ts +++ b/packages/cli/src/ui/components/mcp/utils.test.ts @@ -25,7 +25,6 @@ describe('MCP utils', () => { name: 'server1', status: MCPServerStatus.CONNECTED, source: 'user', - scope: 'user', config: { command: 'cmd1' }, toolCount: 1, promptCount: 0, @@ -35,7 +34,6 @@ describe('MCP utils', () => { name: 'server2', status: MCPServerStatus.CONNECTED, source: 'extension', - scope: 'extension', config: { command: 'cmd2' }, toolCount: 2, promptCount: 0, diff --git a/packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx b/packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx index f52d7aa12..2a19e9328 100644 --- a/packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx +++ b/packages/cli/src/ui/components/messages/AskUserQuestionDialog.test.tsx @@ -174,33 +174,6 @@ describe('', () => { unmount(); }); - it('navigates down with arrow key and selects', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails(); - - const { stdin, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate down to "Blue" - stdin.write('\u001B[B'); // Down arrow - await wait(); - - // Press Enter - stdin.write('\r'); - await wait(); - - expect(onConfirm).toHaveBeenCalledWith( - ToolConfirmationOutcome.ProceedOnce, - { answers: { 0: 'Blue' } }, - ); - unmount(); - }); - it('navigates with number keys', async () => { const onConfirm = vi.fn(); const details = createConfirmationDetails(); @@ -246,56 +219,6 @@ describe('', () => { expect(onConfirm).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); unmount(); }); - - it('does not navigate above first option', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails(); - - const { stdin, unmount } = renderWithProviders( - , - ); - await wait(); - - // Try to go up from first option - stdin.write('\u001B[A'); // Up arrow - await wait(); - - // Should still select the first option - stdin.write('\r'); - await wait(); - - expect(onConfirm).toHaveBeenCalledWith( - ToolConfirmationOutcome.ProceedOnce, - { answers: { 0: 'Red' } }, - ); - unmount(); - }); - - it('does not navigate below last option', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails(); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate way past the last option (3 options + 1 custom input = 4 total) - for (let i = 0; i < 10; i++) { - stdin.write('\u001B[B'); // Down arrow - await wait(); - } - - // Should still render without crashing - expect(lastFrame()).toContain('What is your favorite color?'); - unmount(); - }); }); describe('multi-select interaction', () => { @@ -321,128 +244,9 @@ describe('', () => { expect(lastFrame()).toContain('[✓]'); unmount(); }); - - it('submits multi-select with Space to toggle then Enter to confirm', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [createSingleQuestion({ multiSelect: true })], - }); - - const { stdin, unmount } = renderWithProviders( - , - ); - await wait(); - - // Space to toggle first option - stdin.write(' '); - await wait(); - - // Enter to confirm and submit - stdin.write('\r'); - await wait(); - - expect(onConfirm).toHaveBeenCalledWith( - ToolConfirmationOutcome.ProceedOnce, - { answers: { 0: 'Red' } }, - ); - unmount(); - }); - - it('shows typed custom input text in frame for multi-select question', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [createSingleQuestion({ multiSelect: true })], - }); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Move to "Type something..." input - for (let i = 0; i < 3; i++) { - stdin.write('\u001B[B'); - await wait(); - } - - stdin.write('Orange'); - await wait(); - - expect(lastFrame()).toContain('Orange'); - unmount(); - }); }); describe('multiple questions', () => { - it('auto-advances to next question after selecting an option', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [ - createSingleQuestion({ header: 'Q1' }), - createSingleQuestion({ - header: 'Q2', - question: 'Second question?', - }), - ], - }); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Select first option in Q1 - stdin.write('\r'); - await wait(200); // Wait for auto-advance timeout (150ms) - - // Should now show Q2 - expect(lastFrame()).toContain('Second question?'); - unmount(); - }); - - it('navigates between tabs with left/right arrows', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [ - createSingleQuestion({ header: 'Q1' }), - createSingleQuestion({ - header: 'Q2', - question: 'Second question?', - }), - ], - }); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate right to Q2 - stdin.write('\u001B[C'); // Right arrow - await wait(); - - expect(lastFrame()).toContain('Second question?'); - - // Navigate left back to Q1 - stdin.write('\u001B[D'); // Left arrow - await wait(); - - expect(lastFrame()).toContain('What is your favorite color?'); - unmount(); - }); - it('shows Submit tab for multiple questions', async () => { const onConfirm = vi.fn(); const details = createConfirmationDetails({ @@ -473,80 +277,6 @@ describe('', () => { unmount(); }); - it('submits all answers from Submit tab', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [ - createSingleQuestion({ header: 'Q1' }), - createSingleQuestion({ - header: 'Q2', - question: 'Second question?', - }), - ], - }); - - const { stdin, unmount } = renderWithProviders( - , - ); - await wait(); - - // Answer Q1 - stdin.write('\r'); // Select Red - await wait(200); - - // Answer Q2 - stdin.write('\r'); // Select Red - await wait(200); - - // Now on Submit tab, press Enter to submit - stdin.write('\r'); - await wait(); - - expect(onConfirm).toHaveBeenCalledWith( - ToolConfirmationOutcome.ProceedOnce, - { answers: { 0: 'Red', 1: 'Red' } }, - ); - unmount(); - }); - - it('cancels from Submit tab', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [ - createSingleQuestion({ header: 'Q1' }), - createSingleQuestion({ header: 'Q2' }), - ], - }); - - const { stdin, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate to submit tab - stdin.write('\u001B[C'); // Right - await wait(); - stdin.write('\u001B[C'); // Right - await wait(); - - // Navigate down to Cancel option - stdin.write('\u001B[B'); // Down - await wait(); - - // Press Enter - stdin.write('\r'); - await wait(); - - expect(onConfirm).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); - unmount(); - }); - it('shows unanswered questions as (not answered) in Submit tab', async () => { const onConfirm = vi.fn(); const details = createConfirmationDetails({ @@ -598,286 +328,4 @@ describe('', () => { unmount(); }); }); - - describe('escape from custom input', () => { - it('cancels from custom input with Escape', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails(); - - const { stdin, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate to custom input (3 options, so index 3 is custom input) - stdin.write('\u001B[B'); // Down - await wait(); - stdin.write('\u001B[B'); // Down - await wait(); - stdin.write('\u001B[B'); // Down - now at custom input - await wait(); - - // Press Escape - stdin.write('\u001B'); - await wait(); - - expect(onConfirm).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); - unmount(); - }); - }); - - describe('answered question marker', () => { - it('shows check mark on answered question tab', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [ - createSingleQuestion({ header: 'Q1' }), - createSingleQuestion({ header: 'Q2' }), - ], - }); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Answer Q1 - stdin.write('\r'); // Select Red - await wait(200); - - // Q2 is now active; check that Q1 shows ✓ - expect(lastFrame()).toContain('Q1'); - expect(lastFrame()).toContain('✓'); - unmount(); - }); - }); - - describe('custom input preserves state', () => { - it('preserves typed text when navigating away and back', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails(); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate to custom input (3 options, index 3) - for (let i = 0; i < 3; i++) { - stdin.write('\u001B[B'); // Down - await wait(); - } - - // Type something - stdin.write('Purple'); - await wait(); - - expect(lastFrame()).toContain('Purple'); - - // Navigate away (up to first option) - stdin.write('\u001B[A'); // Up - await wait(); - stdin.write('\u001B[A'); // Up - await wait(); - stdin.write('\u001B[A'); // Up - await wait(); - - // Navigate back to custom input - for (let i = 0; i < 3; i++) { - stdin.write('\u001B[B'); // Down - await wait(); - } - - // Text should still be there - expect(lastFrame()).toContain('Purple'); - unmount(); - }); - - it('does not auto-check custom input in multi-select when navigating back', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [createSingleQuestion({ multiSelect: true })], - }); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate to custom input (index 3) - for (let i = 0; i < 3; i++) { - stdin.write('\u001B[B'); - await wait(); - } - - // Type something - auto-checks - stdin.write('Custom'); - await wait(); - - expect(lastFrame()).toContain('[✓]'); - - // Enter to toggle it off (since auto-check already checked it) - stdin.write('\r'); - await wait(); - - // Should be unchecked now - verify on the custom input line specifically - const afterToggle = lastFrame()!; - const toggledLine = afterToggle - .split('\n') - .find((l) => l.includes('Custom')); - expect(toggledLine).toBeDefined(); - expect(toggledLine).toContain('[ ]'); - expect(toggledLine).not.toContain('[✓]'); - - // Navigate away - stdin.write('\u001B[A'); // Up - await wait(); - - // Navigate back to custom input - stdin.write('\u001B[B'); // Down - await wait(); - - // Should still be unchecked (not auto-checked on remount) - const output = lastFrame()!; - const lines = output.split('\n'); - const customLine = lines.find((l) => l.includes('Custom')); - expect(customLine).toBeDefined(); - expect(customLine).toContain('[ ]'); - expect(customLine).not.toContain('[✓]'); - unmount(); - }); - - it('keeps custom input checked when navigating back if user checked it', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [createSingleQuestion({ multiSelect: true })], - }); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate to custom input (index 3) - for (let i = 0; i < 3; i++) { - stdin.write('\u001B[B'); - await wait(); - } - - // Type something - should auto-check - stdin.write('Custom'); - await wait(); - - // Should already be checked (auto-checked on type) - expect(lastFrame()).toContain('[✓]'); - - // Navigate away - stdin.write('\u001B[A'); // Up - await wait(); - - // Navigate back to custom input - stdin.write('\u001B[B'); // Down - await wait(); - - // Should still be checked - const output = lastFrame()!; - const lines = output.split('\n'); - const customLine = lines.find((l) => l.includes('Custom')); - expect(customLine).toBeDefined(); - expect(customLine).toContain('[✓]'); - unmount(); - }); - - it('auto-checks custom input in multi-select when user types text', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [createSingleQuestion({ multiSelect: true })], - }); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate to custom input (index 3) - for (let i = 0; i < 3; i++) { - stdin.write('\u001B[B'); - await wait(); - } - - // Type something - should auto-check - stdin.write('Hello'); - await wait(); - - const output = lastFrame()!; - const lines = output.split('\n'); - const customLine = lines.find((l) => l.includes('Hello')); - expect(customLine).toBeDefined(); - expect(customLine).toContain('[✓]'); - unmount(); - }); - - it('auto-unchecks custom input in multi-select when text is cleared', async () => { - const onConfirm = vi.fn(); - const details = createConfirmationDetails({ - questions: [createSingleQuestion({ multiSelect: true })], - }); - - const { stdin, lastFrame, unmount } = renderWithProviders( - , - ); - await wait(); - - // Navigate to custom input (index 3) - for (let i = 0; i < 3; i++) { - stdin.write('\u001B[B'); - await wait(); - } - - // Type something - should auto-check - stdin.write('Hi'); - await wait(); - - // Verify auto-check on the custom input line - const afterType = lastFrame()!; - const typedLine = afterType.split('\n').find((l) => l.includes('Hi')); - expect(typedLine).toBeDefined(); - expect(typedLine).toContain('[✓]'); - - // Delete all text (backspace twice) - stdin.write('\x7f'); // backspace - await wait(); - stdin.write('\x7f'); // backspace - await wait(); - - // Should be unchecked now - check the custom input line (option 4) - const afterClear = lastFrame()!; - const clearedLine = afterClear.split('\n').find((l) => l.includes('4.')); - expect(clearedLine).toBeDefined(); - expect(clearedLine).toContain('[ ]'); - expect(clearedLine).not.toContain('[✓]'); - unmount(); - }); - }); }); diff --git a/packages/cli/src/ui/components/shared/text-buffer.ts b/packages/cli/src/ui/components/shared/text-buffer.ts index b07c06706..c68bd1a4b 100644 --- a/packages/cli/src/ui/components/shared/text-buffer.ts +++ b/packages/cli/src/ui/components/shared/text-buffer.ts @@ -1840,7 +1840,7 @@ export function useTextBuffer({ process.env['VISUAL'] ?? process.env['EDITOR'] ?? (process.platform === 'win32' ? 'notepad' : 'vi'); - const tmpDir = fs.mkdtempSync(pathMod.join(os.tmpdir(), 'gemini-edit-')); + const tmpDir = fs.mkdtempSync(pathMod.join(os.tmpdir(), 'qwen-edit-')); const filePath = pathMod.join(tmpDir, 'buffer.txt'); fs.writeFileSync(filePath, text, 'utf8'); diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 42e3964fe..b856dbd99 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -2245,6 +2245,7 @@ describe('useGeminiStream', () => { it('should show a retry countdown and update pending history over time', async () => { vi.useFakeTimers(); try { + let continueToRetryAttempt: (() => void) | undefined; let resolveStream: (() => void) | undefined; mockSendMessageStream.mockReturnValue( (async function* () { @@ -2257,6 +2258,9 @@ describe('useGeminiStream', () => { delayMs: 3000, }, }; + await new Promise((resolve) => { + continueToRetryAttempt = resolve; + }); yield { type: ServerGeminiEventType.Retry, }; @@ -2331,6 +2335,12 @@ describe('useGeminiStream', () => { '2s', ); + continueToRetryAttempt?.(); + + await act(async () => { + await Promise.resolve(); + }); + resolveStream?.(); await act(async () => { @@ -2348,6 +2358,103 @@ describe('useGeminiStream', () => { } }); + it('should clear retry errors after auto-retry succeeds once the countdown has elapsed', async () => { + vi.useFakeTimers(); + try { + let continueAfterCountdown: (() => void) | undefined; + mockSendMessageStream.mockReturnValue( + (async function* () { + yield { + type: ServerGeminiEventType.Retry, + retryInfo: { + message: '[API Error: Rate limit exceeded]', + attempt: 1, + maxRetries: 3, + delayMs: 1000, + }, + }; + await new Promise((resolve) => { + continueAfterCountdown = resolve; + }); + yield { + type: ServerGeminiEventType.Retry, + }; + yield { + type: ServerGeminiEventType.Text, + value: 'Success after retry', + }; + yield { + type: ServerGeminiEventType.Finished, + value: { reason: 'STOP', usageMetadata: undefined }, + }; + })(), + ); + + const { result } = renderHook(() => + useGeminiStream( + new MockedGeminiClientClass(mockConfig), + [], + mockAddItem, + mockConfig, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + false, + () => {}, + () => {}, + () => {}, + () => {}, + 80, + 24, + ), + ); + + act(() => { + void result.current.submitQuery('Trigger retry after countdown'); + }); + + let errorItem = result.current.pendingHistoryItems.find( + (item) => item.type === MessageType.ERROR, + ) as { hint?: string } | undefined; + for (let attempts = 0; attempts < 5 && !errorItem; attempts++) { + await act(async () => { + await Promise.resolve(); + }); + errorItem = result.current.pendingHistoryItems.find( + (item) => item.type === MessageType.ERROR, + ) as { hint?: string } | undefined; + } + expect(errorItem?.hint).toContain('1s'); + + await act(async () => { + await vi.advanceTimersByTimeAsync(1000); + }); + + const staleErrorBeforeRetryCompletes = + result.current.pendingHistoryItems.find( + (item) => item.type === MessageType.ERROR, + ) as { hint?: string } | undefined; + expect(staleErrorBeforeRetryCompletes?.hint).toContain('0s'); + + await act(async () => { + continueAfterCountdown?.(); + await Promise.resolve(); + await Promise.resolve(); + }); + + const remainingError = result.current.pendingHistoryItems.find( + (item) => item.type === MessageType.ERROR, + ); + expect(remainingError).toBeUndefined(); + } finally { + vi.useRealTimers(); + } + }); + it('should memoize pendingHistoryItems', () => { mockUseReactToolScheduler.mockReturnValue([ [], @@ -2527,6 +2634,77 @@ describe('useGeminiStream', () => { expect.any(String), ); }); + + it('should clear static error when starting a new query', async () => { + // First, mock a stream that yields an error (static error without countdown) + mockSendMessageStream.mockReturnValueOnce( + (async function* () { + yield { + type: ServerGeminiEventType.Error, + value: { error: { message: 'First error' } }, + }; + })(), + ); + + const { result } = renderHook(() => + useGeminiStream( + new MockedGeminiClientClass(mockConfig), + [], + mockAddItem, + mockConfig, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + false, + () => {}, + () => {}, + () => {}, + () => {}, + 80, + 24, + ), + ); + + // Submit first query that will fail + await act(async () => { + await result.current.submitQuery('First query'); + }); + + // Verify error appears in pending history items + await waitFor(() => { + const errorItem = result.current.pendingHistoryItems.find( + (item) => item.type === 'error', + ); + expect(errorItem).toBeDefined(); + }); + + // Now mock a successful stream for the second query + mockSendMessageStream.mockReturnValueOnce( + (async function* () { + yield { + type: ServerGeminiEventType.Text, + value: 'Success response', + }; + })(), + ); + + // Submit second query + await act(async () => { + await result.current.submitQuery('Second query'); + }); + + // Verify the error is cleared (no longer in pending history items) + await waitFor(() => { + const errorItem = result.current.pendingHistoryItems.find( + (item) => item.type === 'error', + ); + expect(errorItem).toBeUndefined(); + }); + }); }); describe('Concurrent Execution Prevention', () => { diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 803099d3b..fb35dbcc5 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -1040,7 +1040,8 @@ export const useGeminiStream = ( // Show retry info if available (rate-limit / throttling errors) if (event.retryInfo) { startRetryCountdown(event.retryInfo); - } else if (!pendingRetryCountdownItemRef.current) { + } else { + // The retry attempt is starting now, so any prior retry UI is stale. clearRetryCountdown(); } break; @@ -1081,7 +1082,6 @@ export const useGeminiStream = ( setThought, pendingHistoryItemRef, setPendingHistoryItem, - pendingRetryCountdownItemRef, ], ); @@ -1113,8 +1113,13 @@ export const useGeminiStream = ( if (!options?.isContinuation) { setModelSwitchedFromQuotaError(false); // Commit any pending retry error to history (without hint) since the - // user is starting a new conversation turn - if (pendingRetryCountdownItemRef.current) { + // user is starting a new conversation turn. + // Clear both countdown-based errors AND static errors (those without + // an active countdown timer, e.g. "Press Ctrl+Y to retry"). + if ( + pendingRetryCountdownItemRef.current || + pendingRetryErrorItemRef.current + ) { clearRetryCountdown(); } } @@ -1209,7 +1214,8 @@ export const useGeminiStream = ( } // Only clear auto-retry countdown errors (those with an active timer). // Do NOT clear static error+hint from handleErrorEvent — those should - // remain visible until the user presses Ctrl+Y to retry. + // remain visible until the user presses Ctrl+Y to retry or starts + // a new conversation turn (cleared in submitQuery). if (retryCountdownTimerRef.current) { clearRetryCountdown(); } @@ -1256,6 +1262,7 @@ export const useGeminiStream = ( handleLoopDetectedEvent, clearRetryCountdown, pendingRetryCountdownItemRef, + pendingRetryErrorItemRef, setPendingRetryErrorItem, ], ); @@ -1300,24 +1307,13 @@ export const useGeminiStream = ( return; } - // Commit the error to history (without hint) before clearing - const errorItem = pendingRetryErrorItemRef.current; - if (errorItem) { - addItem({ type: errorItem.type, text: errorItem.text }, Date.now()); - } clearRetryCountdown(); await submitQuery(lastPrompt, { isContinuation: false, skipPreparation: true, }); - }, [ - streamingState, - addItem, - clearRetryCountdown, - submitQuery, - pendingRetryErrorItemRef, - ]); + }, [streamingState, addItem, clearRetryCountdown, submitQuery]); const handleApprovalModeChange = useCallback( async (newApprovalMode: ApprovalMode) => { diff --git a/packages/core/package.json b/packages/core/package.json index 43219cbcc..c66f51a93 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -1,6 +1,6 @@ { "name": "@qwen-code/qwen-code-core", - "version": "0.12.0", + "version": "0.12.3", "description": "Qwen Code Core", "repository": { "type": "git", diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 9feed5ce8..457da8fa9 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -98,6 +98,7 @@ import { // Utils import { shouldAttemptBrowserLaunch } from '../utils/browser.js'; import { FileExclusions } from '../utils/ignorePatterns.js'; +import { shouldDefaultToNodePty } from '../utils/shell-utils.js'; import { WorkspaceContext } from '../utils/workspaceContext.js'; import { isToolEnabled, type ToolName } from '../utils/tool-utils.js'; import { getErrorMessage } from '../utils/errors.js'; @@ -666,7 +667,8 @@ export class Config { this.webSearch = params.webSearch; this.useRipgrep = params.useRipgrep ?? true; this.useBuiltinRipgrep = params.useBuiltinRipgrep ?? true; - this.shouldUseNodePtyShell = params.shouldUseNodePtyShell ?? true; + this.shouldUseNodePtyShell = + params.shouldUseNodePtyShell ?? shouldDefaultToNodePty(); this.skipNextSpeakerCheck = params.skipNextSpeakerCheck ?? true; this.shellExecutionConfig = { terminalWidth: params.shellExecutionConfig?.terminalWidth ?? 80, diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 56dd2786c..fb4cfbfe7 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -358,6 +358,7 @@ describe('Gemini Client (client.ts)', () => { getResumedSessionData: vi.fn().mockReturnValue(undefined), getArenaAgentClient: vi.fn().mockReturnValue(null), getEnableHooks: vi.fn().mockReturnValue(false), + getArenaManager: vi.fn().mockReturnValue(null), getMessageBus: vi.fn().mockReturnValue(undefined), } as unknown as Config; diff --git a/packages/core/src/core/openaiContentGenerator/provider/deepseek.test.ts b/packages/core/src/core/openaiContentGenerator/provider/deepseek.test.ts index 9a69cd326..f4ced4c45 100644 --- a/packages/core/src/core/openaiContentGenerator/provider/deepseek.test.ts +++ b/packages/core/src/core/openaiContentGenerator/provider/deepseek.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type OpenAI from 'openai'; import { DeepSeekOpenAICompatibleProvider } from './deepseek.js'; import type { ContentGeneratorConfig } from '../../contentGenerator.js'; import type { Config } from '../../../config/config.js'; @@ -17,6 +18,7 @@ vi.mock('openai', () => ({ })); describe('DeepSeekOpenAICompatibleProvider', () => { + let provider: DeepSeekOpenAICompatibleProvider; let mockContentGeneratorConfig: ContentGeneratorConfig; let mockCliConfig: Config; @@ -32,6 +34,11 @@ describe('DeepSeekOpenAICompatibleProvider', () => { mockCliConfig = { getCliVersion: vi.fn().mockReturnValue('1.0.0'), } as unknown as Config; + + provider = new DeepSeekOpenAICompatibleProvider( + mockContentGeneratorConfig, + mockCliConfig, + ); }); describe('isDeepSeekProvider', () => { @@ -54,12 +61,102 @@ describe('DeepSeekOpenAICompatibleProvider', () => { }); }); + describe('buildRequest', () => { + const userPromptId = 'prompt-123'; + + it('converts array content into a string', () => { + const originalRequest: OpenAI.Chat.ChatCompletionCreateParams = { + model: 'deepseek-chat', + messages: [ + { + role: 'user', + content: [ + { type: 'text', text: 'Hello' }, + { type: 'text', text: ' world' }, + ], + }, + ], + }; + + const result = provider.buildRequest(originalRequest, userPromptId); + + expect(result.messages).toHaveLength(1); + expect(result.messages?.[0]).toEqual({ + role: 'user', + content: 'Hello\n\n world', + }); + expect(originalRequest.messages?.[0].content).toEqual([ + { type: 'text', text: 'Hello' }, + { type: 'text', text: ' world' }, + ]); + }); + + it('leaves string content unchanged', () => { + const originalRequest: OpenAI.Chat.ChatCompletionCreateParams = { + model: 'deepseek-chat', + messages: [ + { + role: 'user', + content: 'Hello world', + }, + ], + }; + + const result = provider.buildRequest(originalRequest, userPromptId); + + expect(result.messages?.[0].content).toBe('Hello world'); + }); + + it('handles plain string parts in the content array', () => { + const originalRequest = { + model: 'deepseek-chat', + messages: [ + { + role: 'user' as const, + content: [ + 'Hello', + { type: 'text' as const, text: ' world' }, + ] as unknown as OpenAI.Chat.ChatCompletionContentPart[], + }, + ], + }; + + const result = provider.buildRequest(originalRequest, userPromptId); + + expect(result.messages?.[0]).toEqual({ + role: 'user', + content: 'Hello\n\n world', + }); + }); + + it('replaces non-text parts with a placeholder', () => { + const originalRequest: OpenAI.Chat.ChatCompletionCreateParams = { + model: 'deepseek-chat', + messages: [ + { + role: 'user', + content: [ + { type: 'text', text: 'Hello ' }, + { + type: 'image_url', + image_url: { url: 'https://example.com/image.png' }, + }, + ], + }, + ], + }; + + const result = provider.buildRequest(originalRequest, userPromptId); + + expect(result.messages?.[0]).toEqual({ + role: 'user', + content: 'Hello \n\n[Unsupported content type: image_url]', + }); + }); + }); + describe('getDefaultGenerationConfig', () => { it('returns temperature 0', () => { - const provider = new DeepSeekOpenAICompatibleProvider( - mockContentGeneratorConfig, - mockCliConfig, - ); expect(provider.getDefaultGenerationConfig()).toEqual({ temperature: 0, }); diff --git a/packages/core/src/core/openaiContentGenerator/provider/deepseek.ts b/packages/core/src/core/openaiContentGenerator/provider/deepseek.ts index 0e246725f..e34dc724d 100644 --- a/packages/core/src/core/openaiContentGenerator/provider/deepseek.ts +++ b/packages/core/src/core/openaiContentGenerator/provider/deepseek.ts @@ -4,6 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ +import type OpenAI from 'openai'; import type { Config } from '../../../config/config.js'; import type { ContentGeneratorConfig } from '../../contentGenerator.js'; import { DefaultOpenAICompatibleProvider } from './default.js'; @@ -25,6 +26,63 @@ export class DeepSeekOpenAICompatibleProvider extends DefaultOpenAICompatiblePro return baseUrl.toLowerCase().includes('api.deepseek.com'); } + /** + * DeepSeek's API requires message content to be a plain string, not an + * array of content parts. Flatten any text-part arrays into joined strings + * and reject non-text parts that DeepSeek cannot handle. + */ + override buildRequest( + request: OpenAI.Chat.ChatCompletionCreateParams, + userPromptId: string, + ): OpenAI.Chat.ChatCompletionCreateParams { + const baseRequest = super.buildRequest(request, userPromptId); + if (!baseRequest.messages?.length) { + return baseRequest; + } + + const messages = baseRequest.messages.map((message) => { + if (!('content' in message)) { + return message; + } + + const { content } = message; + + if ( + typeof content === 'string' || + content === null || + content === undefined + ) { + return message; + } + + if (!Array.isArray(content)) { + return message; + } + + const text = content + .map((part) => { + if (typeof part === 'string') { + return part; + } + if (part.type === 'text') { + return part.text ?? ''; + } + return `[Unsupported content type: ${part.type}]`; + }) + .join('\n\n'); + + return { + ...message, + content: text, + } as OpenAI.Chat.ChatCompletionMessageParam; + }); + + return { + ...baseRequest, + messages, + }; + } + override getDefaultGenerationConfig(): GenerateContentConfig { return { temperature: 0, diff --git a/packages/core/src/core/tokenLimits.test.ts b/packages/core/src/core/tokenLimits.test.ts index edea10a10..1ba9d4fd1 100644 --- a/packages/core/src/core/tokenLimits.test.ts +++ b/packages/core/src/core/tokenLimits.test.ts @@ -108,11 +108,11 @@ describe('tokenLimit', () => { }); describe('OpenAI', () => { - it('should return 400K for GPT-5.x (latest)', () => { - expect(tokenLimit('gpt-5')).toBe(400000); - expect(tokenLimit('gpt-5-mini')).toBe(400000); - expect(tokenLimit('gpt-5.2')).toBe(400000); - expect(tokenLimit('gpt-5.2-pro')).toBe(400000); + it('should return 272K for GPT-5.x (latest)', () => { + expect(tokenLimit('gpt-5')).toBe(272000); + expect(tokenLimit('gpt-5-mini')).toBe(272000); + expect(tokenLimit('gpt-5.2')).toBe(272000); + expect(tokenLimit('gpt-5.2-pro')).toBe(272000); }); it('should return 128K for legacy GPT (fallback)', () => { @@ -288,8 +288,8 @@ describe('tokenLimit with output type', () => { }); it('should return correct output limits for GLM', () => { - expect(tokenLimit('glm-5', 'output')).toBe(16384); - expect(tokenLimit('glm-4.7', 'output')).toBe(16384); + expect(tokenLimit('glm-5', 'output')).toBe(131072); + expect(tokenLimit('glm-4.7', 'output')).toBe(131072); }); it('should return correct output limits for MiniMax', () => { diff --git a/packages/core/src/core/tokenLimits.ts b/packages/core/src/core/tokenLimits.ts index d038133cb..df911a936 100644 --- a/packages/core/src/core/tokenLimits.ts +++ b/packages/core/src/core/tokenLimits.ts @@ -23,6 +23,7 @@ const LIMITS = { '128k': 131_072, '200k': 200_000, // vendor-declared decimal, used by OpenAI, Anthropic, etc. '256k': 262_144, + '272k': 272_000, // vendor-declared decimal, GPT-5.x input (400K total - 128K output) '400k': 400_000, // vendor-declared decimal, used by OpenAI GPT-5.x '512k': 524_288, '1m': 1_000_000, @@ -87,7 +88,7 @@ const PATTERNS: Array<[RegExp, TokenCount]> = [ // ------------------- // OpenAI // ------------------- - [/^gpt-5/, LIMITS['400k']], // GPT-5.x: 400K + [/^gpt-5/, LIMITS['272k']], // GPT-5.x: 272K input (400K total - 128K output) [/^gpt-/, LIMITS['128k']], // GPT fallback (4o, 4.1, etc.): 128K [/^o\d/, LIMITS['200k']], // o-series (o3, o4-mini, etc.): 200K @@ -171,8 +172,8 @@ const OUTPUT_PATTERNS: Array<[RegExp, TokenCount]> = [ [/^deepseek-chat/, LIMITS['8k']], // Zhipu GLM - [/^glm-5/, LIMITS['16k']], - [/^glm-4\.7/, LIMITS['16k']], + [/^glm-5/, LIMITS['128k']], + [/^glm-4\.7/, LIMITS['128k']], // MiniMax [/^minimax-m2\.5/i, LIMITS['64k']], diff --git a/packages/core/src/ide/ide-client.test.ts b/packages/core/src/ide/ide-client.test.ts index 88788fc57..a483ccb38 100644 --- a/packages/core/src/ide/ide-client.test.ts +++ b/packages/core/src/ide/ide-client.test.ts @@ -40,6 +40,7 @@ vi.mock('node:fs', async (importOriginal) => { readFile: vi.fn(), readdir: vi.fn(), stat: vi.fn(), + unlink: vi.fn(), }, realpathSync: (p: string) => p, existsSync: vi.fn().mockReturnValue(false), @@ -49,10 +50,7 @@ vi.mock('node:dns', async (importOriginal) => { const actual = await importOriginal(); return { ...(actual as object), - promises: { - ...actual.promises, - lookup: vi.fn(), - }, + lookup: vi.fn(), }; }); vi.mock('./process-utils.js'); @@ -84,6 +82,10 @@ describe('IdeClient', () => { // Mock dependencies vi.spyOn(process, 'cwd').mockReturnValue('/test/workspace/sub-dir'); + vi.mocked(fs.existsSync).mockImplementation((filePath: fs.PathLike) => { + const file = String(filePath); + return file !== '/.dockerenv' && file !== '/run/.containerenv'; + }); vi.mocked(detectIde).mockReturnValue(IDE_DEFINITIONS.vscode); vi.mocked(getIdeProcessInfo).mockResolvedValue({ pid: 12345, @@ -218,10 +220,18 @@ describe('IdeClient', () => { vi.mocked(fs.existsSync).mockImplementation( (filePath: fs.PathLike) => filePath === '/.dockerenv', ); - (dns.promises.lookup as unknown as Mock).mockResolvedValue({ - address: '192.168.65.254', - family: 4, - }); + (dns.lookup as unknown as Mock).mockImplementation( + ( + _hostname: string, + callback: ( + err: Error | null, + address?: string, + family?: number, + ) => void, + ) => { + callback(null, '192.168.65.254', 4); + }, + ); mockClient.connect .mockRejectedValueOnce(new Error('localhost unreachable')) .mockResolvedValueOnce(undefined); @@ -248,6 +258,85 @@ describe('IdeClient', () => { delete process.env['QWEN_CODE_IDE_SERVER_PORT']; }); + it('should try a newer lock-file port when the configured port is stale', async () => { + process.env['QWEN_CODE_IDE_SERVER_PORT'] = '1111'; + const primaryConfig = { + port: '1111', + authToken: 'stale-token', + workspacePath: '/test/workspace', + }; + const fallbackConfig = { + port: '2222', + authToken: 'fresh-token', + workspacePath: '/test/workspace', + }; + vi.mocked(fs.promises.readFile).mockImplementation( + async (filePath: fs.PathLike | FileHandle) => { + const file = String(filePath); + if (file === path.join('/home/test', '.qwen', 'ide', '1111.lock')) { + return JSON.stringify(primaryConfig); + } + if (file === path.join('/home/test', '.qwen', 'ide', '2222.lock')) { + return JSON.stringify(fallbackConfig); + } + throw new Error(`unexpected path: ${file}`); + }, + ); + ( + vi.mocked(fs.promises.readdir) as Mock< + (path: fs.PathLike) => Promise + > + ).mockResolvedValue(['1111.lock', '2222.lock']); + ( + vi.mocked(fs.promises.stat) as Mock< + (path: fs.PathLike) => Promise + > + ).mockImplementation(async (filePath: fs.PathLike) => { + const now = Date.now(); + const file = String(filePath); + return { + mtimeMs: file.endsWith('2222.lock') ? now : now - 1000, + } as fs.Stats; + }); + vi.mocked(fs.existsSync).mockImplementation( + (filePath: fs.PathLike) => String(filePath) === '/test/workspace', + ); + mockClient.request.mockResolvedValue({ tools: [] }); + mockClient.connect + .mockRejectedValueOnce(new Error('stale port')) + .mockResolvedValueOnce(undefined); + + const ideClient = await IdeClient.getInstance(); + await ideClient.connect(); + + expect(StreamableHTTPClientTransport).toHaveBeenNthCalledWith( + 1, + new URL('http://127.0.0.1:1111/mcp'), + expect.objectContaining({ + requestInit: { + headers: { + Authorization: 'Bearer stale-token', + }, + }, + }), + ); + expect(StreamableHTTPClientTransport).toHaveBeenNthCalledWith( + 2, + new URL('http://127.0.0.1:2222/mcp'), + expect.objectContaining({ + requestInit: { + headers: { + Authorization: 'Bearer fresh-token', + }, + }, + }), + ); + expect(ideClient.getConnectionStatus().status).toBe( + IDEConnectionStatus.Connected, + ); + delete process.env['QWEN_CODE_IDE_SERVER_PORT']; + }); + it('should connect using stdio when stdio config is in environment variables', async () => { vi.mocked(fs.promises.readFile).mockRejectedValue( new Error('File not found'), @@ -342,6 +431,24 @@ describe('IdeClient', () => { delete process.env['QWEN_CODE_IDE_SERVER_PORT']; }); + it('should not scan the lock directory when the env port lock file exists', async () => { + process.env['QWEN_CODE_IDE_SERVER_PORT'] = '1234'; + const config = { port: '1234', workspacePath: '/test/workspace' }; + vi.mocked(fs.promises.readFile).mockResolvedValue(JSON.stringify(config)); + + const ideClient = await IdeClient.getInstance(); + vi.mocked(fs.promises.readdir).mockClear(); + const result = await ( + ideClient as unknown as { + getConnectionConfigFromFile: () => Promise; + } + ).getConnectionConfigFromFile(); + + expect(result).toEqual(config); + expect(fs.promises.readdir).not.toHaveBeenCalled(); + delete process.env['QWEN_CODE_IDE_SERVER_PORT']; + }); + it('should return undefined if no config files are found', async () => { vi.mocked(fs.promises.readFile).mockRejectedValue(new Error('not found')); @@ -424,6 +531,102 @@ describe('IdeClient', () => { delete process.env['QWEN_CODE_IDE_SERVER_PORT']; }); + it('should keep a live lock file even when it is older than 7 days', async () => { + const liveConfig = { + port: '1000', + workspacePath: '/test/workspace', + ppid: 4242, + }; + const oldTime = Date.now() - 8 * 24 * 60 * 60 * 1000; + + vi.mocked(fs.promises.readFile).mockImplementation( + async (filePath: fs.PathLike | FileHandle) => { + const file = String(filePath); + if (file === path.join('/tmp', 'qwen-code-ide-server-12345.json')) { + throw new Error('not found'); + } + if (file === path.join('/home/test', '.qwen', 'ide', '1000.lock')) { + return JSON.stringify(liveConfig); + } + throw new Error(`unexpected path: ${file}`); + }, + ); + ( + vi.mocked(fs.promises.readdir) as Mock< + (path: fs.PathLike) => Promise + > + ).mockResolvedValue(['1000.lock']); + ( + vi.mocked(fs.promises.stat) as Mock< + (path: fs.PathLike) => Promise + > + ).mockResolvedValue({ mtimeMs: oldTime } as fs.Stats); + vi.spyOn(process, 'kill').mockImplementation(() => true); + + const ideClient = await IdeClient.getInstance(); + const result = await ( + ideClient as unknown as { + getConnectionConfigFromFile: () => Promise; + } + ).getConnectionConfigFromFile(); + + expect(result).toEqual(liveConfig); + expect(fs.promises.unlink).not.toHaveBeenCalled(); + }); + + it('should keep incomplete old lock files when there is no stronger stale signal', async () => { + const latestConfig = { + port: '2000', + workspacePath: '/test/workspace', + }; + const now = Date.now(); + const staleTime = now - 7 * 24 * 60 * 60 * 1000 - 1000; + + vi.mocked(fs.promises.readFile).mockImplementation( + async (filePath: fs.PathLike | FileHandle) => { + const file = String(filePath); + if (file === path.join('/tmp', 'qwen-code-ide-server-12345.json')) { + throw new Error('not found'); + } + if (file === path.join('/home/test', '.qwen', 'ide', '1000.lock')) { + return JSON.stringify({ port: '1000' }); + } + if (file === path.join('/home/test', '.qwen', 'ide', '2000.lock')) { + return JSON.stringify(latestConfig); + } + throw new Error(`unexpected path: ${file}`); + }, + ); + ( + vi.mocked(fs.promises.readdir) as Mock< + (path: fs.PathLike) => Promise + > + ).mockResolvedValue(['1000.lock', '2000.lock']); + ( + vi.mocked(fs.promises.stat) as Mock< + (path: fs.PathLike) => Promise + > + ).mockImplementation(async (filePath: fs.PathLike) => { + const file = String(filePath); + return { + mtimeMs: file.endsWith('1000.lock') ? staleTime : now, + } as fs.Stats; + }); + vi.mocked(fs.existsSync).mockImplementation( + (filePath: fs.PathLike) => String(filePath) === '/test/workspace', + ); + + const ideClient = await IdeClient.getInstance(); + const result = await ( + ideClient as unknown as { + getConnectionConfigFromFile: () => Promise; + } + ).getConnectionConfigFromFile(); + + expect(fs.promises.unlink).not.toHaveBeenCalled(); + expect(result).toEqual(latestConfig); + }); + it('should scan IDE lock directory when env and legacy config are unavailable', async () => { const latestConfig = { port: '2000', @@ -458,9 +661,10 @@ describe('IdeClient', () => { (path: fs.PathLike) => Promise > ).mockImplementation(async (filePath: fs.PathLike) => { + const now = Date.now(); const file = String(filePath); return { - mtimeMs: file.endsWith('2000.lock') ? 2000 : 1000, + mtimeMs: file.endsWith('2000.lock') ? now : now - 1000, } as fs.Stats; }); @@ -509,9 +713,10 @@ describe('IdeClient', () => { (path: fs.PathLike) => Promise > ).mockImplementation(async (filePath: fs.PathLike) => { + const now = Date.now(); const file = String(filePath); return { - mtimeMs: file.endsWith('2000.lock') ? 2000 : 1000, + mtimeMs: file.endsWith('2000.lock') ? now : now - 1000, } as fs.Stats; }); @@ -647,14 +852,18 @@ describe('IdeClient', () => { }); describe('getIdeServerHost', () => { - const dnsLookupMock = dns.promises.lookup as unknown as Mock; + const dnsLookupMock = dns.lookup as unknown as Mock; function mockDnsResolvable(reachable: boolean): void { - if (reachable) { - dnsLookupMock.mockResolvedValue({ address: '192.168.65.254', family: 4 }); - } else { - dnsLookupMock.mockRejectedValue(new Error('ENOTFOUND')); - } + dnsLookupMock.mockImplementation( + (_hostname: string, callback: (err: Error | null) => void) => { + if (reachable) { + callback(null); + } else { + callback(new Error('ENOTFOUND')); + } + }, + ); } beforeEach(() => { @@ -682,7 +891,10 @@ describe('getIdeServerHost', () => { const host = await getIdeServerHost(); expect(host).toBe('host.docker.internal'); - expect(dnsLookupMock).toHaveBeenCalledWith('host.docker.internal'); + expect(dnsLookupMock).toHaveBeenCalledWith( + 'host.docker.internal', + expect.any(Function), + ); }); it('should fall back to 127.0.0.1 when in a container but host.docker.internal is not reachable', async () => { @@ -694,7 +906,10 @@ describe('getIdeServerHost', () => { const host = await getIdeServerHost(); expect(host).toBe('127.0.0.1'); - expect(dnsLookupMock).toHaveBeenCalledWith('host.docker.internal'); + expect(dnsLookupMock).toHaveBeenCalledWith( + 'host.docker.internal', + expect.any(Function), + ); }); it('should detect container via /run/.containerenv', async () => { @@ -727,15 +942,19 @@ describe('getIdeServerHost', () => { vi.mocked(fs.existsSync).mockImplementation( (filePath: fs.PathLike) => filePath === '/.dockerenv', ); - // Simulate dns.promises.lookup that never resolves - dnsLookupMock.mockReturnValue(new Promise(() => {})); + dnsLookupMock.mockImplementation(() => { + // Never call the callback to simulate a hung lookup. + }); const hostPromise = getIdeServerHost(); await vi.advanceTimersByTimeAsync(3000); const host = await hostPromise; expect(host).toBe('127.0.0.1'); - expect(dnsLookupMock).toHaveBeenCalledWith('host.docker.internal'); + expect(dnsLookupMock).toHaveBeenCalledWith( + 'host.docker.internal', + expect.any(Function), + ); }); it('should perform only one DNS lookup when called concurrently', async () => { @@ -746,13 +965,9 @@ describe('getIdeServerHost', () => { // Simulate a slow DNS lookup dnsLookupMock.mockImplementation( - () => - new Promise((resolve) => - setTimeout( - () => resolve({ address: '192.168.65.254', family: 4 }), - 50, - ), - ), + (_hostname: string, callback: (err: Error | null) => void) => { + setTimeout(() => callback(null), 50); + }, ); const promises = Array.from({ length: 5 }, () => getIdeServerHost()); diff --git a/packages/core/src/ide/ide-client.ts b/packages/core/src/ide/ide-client.ts index b4835e30e..d51607eef 100644 --- a/packages/core/src/ide/ide-client.ts +++ b/packages/core/src/ide/ide-client.ts @@ -62,6 +62,19 @@ type ConnectionConfig = { stdio?: StdioConfig; }; +type IdeConnectionConfig = ConnectionConfig & { + workspacePath?: string; + ideInfo?: IdeInfo; + ppid?: number; +}; + +type ParsedConnectionLockFile = { + file: string; + fullPath: string; + mtimeMs: number; + parsed: IdeConnectionConfig; +}; + function getRealPath(path: string): string { try { return fs.realpathSync(path); @@ -85,9 +98,7 @@ export class IdeClient { }; private currentIde: IdeInfo | undefined; private ideProcessInfo: { pid: number; command: string } | undefined; - private connectionConfig: - | (ConnectionConfig & { workspacePath?: string; ideInfo?: IdeInfo }) - | undefined; + private connectionConfig: IdeConnectionConfig | undefined; private authToken: string | undefined; private diffResponses = new Map void>(); private statusListeners = new Set<(state: IDEConnectionState) => void>(); @@ -172,6 +183,10 @@ export class IdeClient { if (connected) { return; } + const fallbackConnected = await this.tryFallbackPorts(); + if (fallbackConnected) { + return; + } } if (this.connectionConfig.stdio) { const connected = await this.establishStdioConnection( @@ -570,10 +585,10 @@ export class IdeClient { } private async getConnectionConfigFromFile(): Promise< - | (ConnectionConfig & { workspacePath?: string; ideInfo?: IdeInfo }) - | undefined + IdeConnectionConfig | undefined > { const portFromEnv = this.getPortFromEnv(); + if (portFromEnv) { try { const ideDir = Storage.getGlobalIdeDir(); @@ -591,37 +606,20 @@ export class IdeClient { return legacyConfig; } - // Scan lock directory as a last resort when neither env var nor legacy - // file is available (e.g. code-server where the env var is not injected). - // Configs are sorted by modification time (most recent first). Pick the - // first one whose workspace matches the current working directory. - if (!portFromEnv) { - const ideDir = Storage.getGlobalIdeDir(); - const configs = await this.getAllConnectionConfigs(ideDir); - if (configs.length > 0) { - debugLogger.debug( - `Discovered ${configs.length} IDE lock file(s) via directory scan`, - ); - const cwd = process.cwd(); - const match = configs.find( - (c) => - c.workspacePath !== undefined && - IdeClient.validateWorkspacePath(c.workspacePath, cwd).isValid, - ); - return match; - } - } - - return undefined; + const ideDir = Storage.getGlobalIdeDir(); + const configs = await this.getAllConnectionConfigs(ideDir); + const cwd = process.cwd(); + return configs.find( + (config) => + config.workspacePath !== undefined && + IdeClient.validateWorkspacePath(config.workspacePath, cwd).isValid, + ); } // Legacy connection files were written in the global temp directory. private async getLegacyConnectionConfig( portFromEnv?: string, - ): Promise< - | (ConnectionConfig & { workspacePath?: string; ideInfo?: IdeInfo }) - | undefined - > { + ): Promise { if (this.ideProcessInfo) { try { const portFile = path.join( @@ -656,15 +654,13 @@ export class IdeClient { protected async getAllConnectionConfigs( ideDir: string, - ): Promise< - ConnectionConfig & Array<{ workspacePath?: string; ideInfo?: IdeInfo }> - > { - const fileRegex = new RegExp('^\\d+\\.lock$'); + ): Promise { + const fileRegex = /^\d+\.lock$/; let lockFiles: string[]; try { - lockFiles = (await fs.promises.readdir(ideDir)).filter((file) => - fileRegex.test(file), - ); + lockFiles = (await fs.promises.readdir(ideDir)) + .map((file) => file.toString()) + .filter((file) => fileRegex.test(file)); } catch (e) { debugLogger.debug('Failed to read IDE connection directory:', e); return []; @@ -677,27 +673,131 @@ export class IdeClient { const stat = await fs.promises.stat(fullPath); const content = await fs.promises.readFile(fullPath, 'utf8'); try { - const parsed = JSON.parse(content); - return { file, mtimeMs: stat.mtimeMs, parsed }; - } catch (e) { - debugLogger.debug('Failed to parse JSON from lock file: ', e); - return { file, mtimeMs: stat.mtimeMs, parsed: undefined }; + return { + file, + fullPath, + mtimeMs: stat.mtimeMs, + parsed: JSON.parse(content) as IdeConnectionConfig, + }; + } catch (error) { + debugLogger.debug('Failed to parse JSON from lock file: ', error); + return undefined; } - } catch (e) { - // If we can't stat/read the file, treat it as very old so it doesn't - // win ties, and skip parsing by returning undefined content. - debugLogger.debug('Failed to read/stat IDE lock file:', e); - return { file, mtimeMs: -Infinity, parsed: undefined }; + } catch (error) { + debugLogger.debug('Failed to read/stat IDE lock file:', error); + return undefined; } }), ); - return fileContents - .filter(({ parsed }) => parsed !== undefined) + const parsedLockFiles = fileContents.filter( + (lockFile): lockFile is ParsedConnectionLockFile => + lockFile !== undefined, + ); + const activeLockFiles = await Promise.all( + parsedLockFiles.map(async (lockFile) => ({ + lockFile, + isStale: await this.cleanupStaleLockFile(lockFile), + })), + ); + + const staleCount = activeLockFiles.filter(({ isStale }) => isStale).length; + if (staleCount > 0) { + debugLogger.debug( + `[cleanupStaleLockFiles] Cleaned up ${staleCount} stale lock file(s)`, + ); + } + + return activeLockFiles + .filter(({ isStale }) => !isStale) + .map(({ lockFile }) => lockFile) .sort((a, b) => b.mtimeMs - a.mtimeMs) .map(({ parsed }) => parsed); } + private async cleanupStaleLockFile({ + file, + fullPath, + parsed, + }: ParsedConnectionLockFile): Promise { + try { + if (parsed.ppid) { + try { + process.kill(parsed.ppid, 0); + return false; + } catch { + debugLogger.debug( + `[cleanupStaleLockFiles] Removing lock file "${file}" - ppid ${parsed.ppid} no longer exists`, + ); + await fs.promises.unlink(fullPath); + return true; + } + } + + if (parsed.workspacePath) { + if (fs.existsSync(parsed.workspacePath)) { + return false; + } + + debugLogger.debug( + `[cleanupStaleLockFiles] Removing lock file "${file}" - workspace doesn't exist`, + ); + await fs.promises.unlink(fullPath); + return true; + } + + return false; + } catch (error) { + debugLogger.debug( + `[cleanupStaleLockFiles] Error checking lock file "${file}":`, + error, + ); + return false; + } + } + + private async tryFallbackPorts(): Promise { + const cwd = process.cwd(); + const currentPort = this.connectionConfig?.port; + const configs = await this.getAllConnectionConfigs( + Storage.getGlobalIdeDir(), + ); + const workspaceMatches: IdeConnectionConfig[] = []; + const otherConfigs: IdeConnectionConfig[] = []; + + for (const config of configs) { + if (!config.port || config.port === currentPort) { + continue; + } + + if ( + config.workspacePath !== undefined && + IdeClient.validateWorkspacePath(config.workspacePath, cwd).isValid + ) { + workspaceMatches.push(config); + } else { + otherConfigs.push(config); + } + } + + for (const config of [...workspaceMatches, ...otherConfigs]) { + const port = config.port; + if (!port) { + continue; + } + if (config.authToken) { + this.authToken = config.authToken; + } + const connected = await this.establishHttpConnection(port); + if (connected) { + this.connectionConfig = config; + return true; + } + } + + return false; + } + private createProxyAwareFetch(ideHost: string) { // Ignore proxy for IDE server host to allow connecting to the ide mcp // server even when HTTP_PROXY is set @@ -929,21 +1029,26 @@ export function _resetCachedIdeServerHost(): void { /** * Check if a hostname is DNS-resolvable, with a timeout guard. + * Uses callback-based dns.lookup() for better compatibility across + * different Node.js environments (e.g., VSCode, Cursor). */ async function isHostResolvable(hostname: string): Promise { - try { - const timeout = new Promise((_, reject) => { - const timer = setTimeout( - () => reject(new Error('DNS lookup timeout')), - DNS_LOOKUP_TIMEOUT_MS, - ); - timer.unref?.(); + return new Promise((resolve) => { + let settled = false; + const timeout = setTimeout(() => { + if (settled) return; + settled = true; + resolve(false); + }, DNS_LOOKUP_TIMEOUT_MS); + timeout.unref?.(); + + dns.lookup(hostname, (err) => { + if (settled) return; + settled = true; + clearTimeout(timeout); + resolve(!err); }); - await Promise.race([dns.promises.lookup(hostname), timeout]); - return true; - } catch { - return false; - } + }); } /** diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index d81079817..21a511eac 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -11,7 +11,6 @@ // Core configuration export * from './config/config.js'; export { Storage } from './config/storage.js'; -export * from './utils/configResolver.js'; // Model configuration export { @@ -60,107 +59,28 @@ export * from './core/nonInteractiveToolExecutor.js'; export * from './core/prompts.js'; export * from './core/tokenLimits.js'; export * from './core/turn.js'; -export * from './core/geminiRequest.js'; -export * from './core/coreToolScheduler.js'; -export * from './core/nonInteractiveToolExecutor.js'; -export * from './tools/tool-names.js'; // ============================================================================ // Tools // ============================================================================ -// Export utilities -export * from './utils/paths.js'; -export * from './utils/schemaValidator.js'; -export * from './utils/errors.js'; -export * from './utils/debugLogger.js'; -export * from './utils/symlink.js'; -export * from './utils/getFolderStructure.js'; -export * from './utils/memoryDiscovery.js'; -export * from './utils/gitIgnoreParser.js'; -export * from './utils/gitUtils.js'; -export * from './utils/editor.js'; -export * from './utils/quotaErrorDetection.js'; -export * from './utils/fileUtils.js'; -export * from './utils/retry.js'; -export * from './utils/shell-utils.js'; -export * from './utils/tool-utils.js'; -export * from './utils/terminalSerializer.js'; -export * from './utils/systemEncoding.js'; -export * from './utils/textUtils.js'; -export * from './utils/formatters.js'; -export * from './utils/generateContentResponseUtilities.js'; -export * from './utils/ripgrepUtils.js'; -export * from './utils/filesearch/fileSearch.js'; -export * from './utils/errorParsing.js'; -export * from './utils/workspaceContext.js'; -export * from './utils/ignorePatterns.js'; -export * from './utils/partUtils.js'; -export * from './utils/subagentGenerator.js'; -export * from './utils/projectSummary.js'; -export * from './utils/promptIdContext.js'; -export * from './utils/thoughtUtils.js'; -export * from './utils/toml-to-markdown-converter.js'; -export * from './utils/yaml-parser.js'; -export * from './utils/environmentContext.js'; - -// Config resolution utilities -export * from './utils/configResolver.js'; - -// Export services -export * from './services/fileDiscoveryService.js'; -export * from './services/gitService.js'; -export * from './services/chatRecordingService.js'; -export * from './services/sessionService.js'; -export * from './services/fileSystemService.js'; - -// Export IDE specific logic -export * from './ide/ide-client.js'; -export * from './ide/ideContext.js'; -export * from './ide/ide-installer.js'; -export { IDE_DEFINITIONS, type IdeInfo } from './ide/detect-ide.js'; -export * from './ide/constants.js'; -export * from './ide/types.js'; - -// Export Shell Execution Service -export * from './services/shellExecutionService.js'; - -// Export base tool definitions -export * from './tools/tools.js'; +// Tool names and registry +export * from './tools/tool-names.js'; export * from './tools/tool-error.js'; export * from './tools/tool-registry.js'; +export * from './tools/tools.js'; -// Export subagents (Phase 1) -export * from './subagents/index.js'; - -// Export shared multi-agent infrastructure -export * from './agents/index.js'; - -// Export skills -export * from './skills/index.js'; - -// Export extension -export * from './extension/index.js'; - -// Export prompt logic -export * from './prompts/mcp-prompts.js'; - -// Export specific tool logic -export * from './tools/read-file.js'; -export * from './tools/ls.js'; -export * from './tools/grep.js'; -export * from './tools/ripGrep.js'; -export * from './tools/glob.js'; +// Individual tools export * from './tools/edit.js'; export * from './tools/exitPlanMode.js'; export * from './tools/glob.js'; export * from './tools/grep.js'; export * from './tools/ls.js'; export * from './tools/lsp.js'; -export * from './tools/memoryTool.js'; export * from './tools/mcp-client.js'; export * from './tools/mcp-client-manager.js'; export * from './tools/mcp-tool.js'; +export * from './tools/memoryTool.js'; export * from './tools/read-file.js'; export * from './tools/ripGrep.js'; export * from './tools/sdk-control-client-transport.js'; @@ -168,9 +88,6 @@ export * from './tools/shell.js'; export * from './tools/skill.js'; export * from './tools/task.js'; export * from './tools/todoWrite.js'; -export * from './tools/tool-error.js'; -export * from './tools/tool-registry.js'; -export * from './tools/tools.js'; export * from './tools/web-fetch.js'; export * from './tools/web-search/index.js'; export * from './tools/write-file.js'; @@ -187,11 +104,21 @@ export * from './services/gitWorktreeService.js'; export * from './services/sessionService.js'; export * from './services/shellExecutionService.js'; +// ============================================================================ +// IDE Support +// ============================================================================ + +export * from './ide/ide-client.js'; +export * from './ide/ideContext.js'; +export * from './ide/ide-installer.js'; +export { IDE_DEFINITIONS, type IdeInfo } from './ide/detect-ide.js'; +export * from './ide/constants.js'; +export * from './ide/types.js'; + // ============================================================================ // LSP Support // ============================================================================ -// LSP support export * from './lsp/constants.js'; export * from './lsp/LspConfigLoader.js'; export * from './lsp/LspConnectionFactory.js'; @@ -207,7 +134,11 @@ export * from './lsp/types.js'; // ============================================================================ export { MCPOAuthProvider } from './mcp/oauth-provider.js'; -export type { MCPOAuthConfig } from './mcp/oauth-provider.js'; +export type { + MCPOAuthConfig, + OAuthDisplayMessage, + OAuthDisplayPayload, +} from './mcp/oauth-provider.js'; export { MCPOAuthTokenStorage } from './mcp/oauth-token-storage.js'; export { KeychainTokenStorage } from './mcp/token-storage/keychain-token-storage.js'; export type { @@ -245,19 +176,24 @@ export { } from './telemetry/types.js'; // ============================================================================ -// Extensions & Subagents +// Extensions, Skills, Subagents & Agents // ============================================================================ export * from './extension/index.js'; export * from './prompts/mcp-prompts.js'; export * from './skills/index.js'; +export * from './subagents/index.js'; +export * from './agents/index.js'; // ============================================================================ // Utilities // ============================================================================ export * from './utils/browser.js'; +export * from './utils/configResolver.js'; +export * from './utils/debugLogger.js'; export * from './utils/editor.js'; +export * from './utils/environmentContext.js'; export * from './utils/errorParsing.js'; export * from './utils/errors.js'; export * from './utils/fileUtils.js'; @@ -268,13 +204,14 @@ export * from './utils/getFolderStructure.js'; export * from './utils/gitIgnoreParser.js'; export * from './utils/gitUtils.js'; export * from './utils/ignorePatterns.js'; +export * from './utils/jsonl-utils.js'; export * from './utils/memoryDiscovery.js'; export { OpenAILogger, openaiLogger } from './utils/openaiLogger.js'; export * from './utils/partUtils.js'; export * from './utils/pathReader.js'; export * from './utils/paths.js'; -export * from './utils/promptIdContext.js'; export * from './utils/projectSummary.js'; +export * from './utils/promptIdContext.js'; export * from './utils/quotaErrorDetection.js'; export * from './utils/readManyFiles.js'; export * from './utils/request-tokenizer/supportedImageFormats.js'; @@ -283,6 +220,7 @@ export * from './utils/ripgrepUtils.js'; export * from './utils/schemaValidator.js'; export * from './utils/shell-utils.js'; export * from './utils/subagentGenerator.js'; +export * from './utils/symlink.js'; export * from './utils/systemEncoding.js'; export * from './utils/terminalSerializer.js'; export * from './utils/textUtils.js'; @@ -291,8 +229,6 @@ export * from './utils/toml-to-markdown-converter.js'; export * from './utils/tool-utils.js'; export * from './utils/workspaceContext.js'; export * from './utils/yaml-parser.js'; -export * from './utils/jsonl-utils.js'; -export * from './utils/symlink.js'; // ============================================================================ // OAuth & Authentication @@ -307,7 +243,10 @@ export * from './qwen/qwenOAuth2.js'; export { makeFakeConfig } from './test-utils/config.js'; export * from './test-utils/index.js'; -// Export hook types and components +// ============================================================================ +// Hooks +// ============================================================================ + export * from './hooks/types.js'; export { HookSystem, HookRegistry } from './hooks/index.js'; export type { HookRegistryEntry } from './hooks/index.js'; diff --git a/packages/core/src/mcp/oauth-provider.ts b/packages/core/src/mcp/oauth-provider.ts index 1d1157c27..a2fca6eec 100644 --- a/packages/core/src/mcp/oauth-provider.ts +++ b/packages/core/src/mcp/oauth-provider.ts @@ -22,8 +22,28 @@ import { export const OAUTH_DISPLAY_MESSAGE_EVENT = 'oauth-display-message' as const; +/** + * Structured display message for i18n support. + * The `key` is the i18n translation key (English text as key). + * The `params` are optional interpolation parameters. + */ +export interface OAuthDisplayMessage { + key: string; + params?: Record; +} + +/** Payload type for OAuth display message events: structured i18n message or plain string. */ +export type OAuthDisplayPayload = string | OAuthDisplayMessage; + const debugLogger = createDebugLogger('MCP_OAUTH'); +// Module-level reference to the active OAuth callback server. +// This ensures that if a new authentication is started before the previous one +// finishes (e.g. user navigated back and re-entered), the old server is closed +// first to avoid EADDRINUSE errors. +let activeCallbackServer: http.Server | null = null; +let activeCallbackTimeout: ReturnType | null = null; + /** * OAuth configuration for an MCP server. */ @@ -195,6 +215,20 @@ export class MCPOAuthProvider { private async startCallbackServer( expectedState: string, ): Promise { + // Close any previously active callback server to avoid EADDRINUSE + if (activeCallbackServer) { + try { + activeCallbackServer.close(); + } catch { + // Ignore errors when closing stale server + } + activeCallbackServer = null; + } + if (activeCallbackTimeout) { + clearTimeout(activeCallbackTimeout); + activeCallbackTimeout = null; + } + return new Promise((resolve, reject) => { const server = http.createServer( async (req: http.IncomingMessage, res: http.ServerResponse) => { @@ -226,6 +260,7 @@ export class MCPOAuthProvider { `); + activeCallbackServer = null; server.close(); reject(new Error(`OAuth error: ${error}`)); return; @@ -240,6 +275,7 @@ export class MCPOAuthProvider { if (state !== expectedState) { res.writeHead(400); res.end('Invalid state parameter'); + activeCallbackServer = null; server.close(); reject(new Error('State mismatch - possible CSRF attack')); return; @@ -257,9 +293,11 @@ export class MCPOAuthProvider { `); + activeCallbackServer = null; server.close(); resolve({ code, state }); } catch (error) { + activeCallbackServer = null; server.close(); reject(error); } @@ -273,9 +311,14 @@ export class MCPOAuthProvider { ); }); + // Track the active server so it can be cleaned up if a new auth starts + activeCallbackServer = server; + // Timeout after 5 minutes - setTimeout( + activeCallbackTimeout = setTimeout( () => { + activeCallbackServer = null; + activeCallbackTimeout = null; server.close(); reject(new Error('OAuth callback timeout')); }, @@ -603,11 +646,17 @@ export class MCPOAuthProvider { events?: EventEmitter, ): Promise { // Helper function to display messages through handler or fallback to debugLogger - const displayMessage = (message: string) => { + const displayMessage = (message: OAuthDisplayPayload) => { if (events) { events.emit(OAUTH_DISPLAY_MESSAGE_EVENT, message); } else { - debugLogger.info(message); + if (typeof message === 'string') { + debugLogger.info(message); + } else { + debugLogger.info( + `[${message.key}]${message.params ? ` ${JSON.stringify(message.params)}` : ''}`, + ); + } } }; @@ -746,13 +795,13 @@ export class MCPOAuthProvider { mcpServerUrl, ); - displayMessage(`→ Opening your browser for OAuth sign-in... - -If the browser does not open, copy and paste this URL into your browser: -${authUrl} - -💡 TIP: Triple-click to select the entire URL, then copy and paste it into your browser. -⚠️ Make sure to copy the COMPLETE URL - it may wrap across multiple lines.`); + displayMessage({ + key: 'If the browser does not open, copy and paste this URL into your browser:', + }); + displayMessage(`\n${authUrl.toString()}\n`); + displayMessage({ + key: 'Make sure to copy the COMPLETE URL - it may wrap across multiple lines.', + }); // Start callback server const callbackPromise = this.startCallbackServer(pkceParams.state); diff --git a/packages/core/src/mcp/oauth-utils.test.ts b/packages/core/src/mcp/oauth-utils.test.ts index 358213510..2e1bd8984 100644 --- a/packages/core/src/mcp/oauth-utils.test.ts +++ b/packages/core/src/mcp/oauth-utils.test.ts @@ -342,4 +342,85 @@ describe('OAuthUtils', () => { expect(result).toBe('https://mcp.alibaba-inc.com/yuque/mcp'); }); }); + + describe('discoverOAuthConfig', () => { + it('should use scopes from protected resource metadata when available', async () => { + // This test verifies the fix for the issue where scopes from + // protected resource metadata were not being used + const mockResourceMetadata: OAuthProtectedResourceMetadata = { + resource: 'https://www.modelscope.cn/mcp-server', + authorization_servers: ['https://www.modelscope.cn'], + scopes_supported: [ + 'openid', + 'profile', + 'list-operational-mcp', + 'manage-mcp-deployment', + ], + }; + + const mockAuthServerMetadata: OAuthAuthorizationServerMetadata = { + issuer: 'https://www.modelscope.cn', + authorization_endpoint: 'https://www.modelscope.cn/oauth/authorize', + token_endpoint: 'https://www.modelscope.cn/oauth/token', + // Note: scopes_supported is NOT present in auth server metadata + }; + + mockFetch + // First call: fetch protected resource metadata + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(mockResourceMetadata), + }) + // Second call: fetch authorization server metadata + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(mockAuthServerMetadata), + }); + + const result = await OAuthUtils.discoverOAuthConfig( + 'https://www.modelscope.cn/mcp-server', + ); + + expect(result).not.toBeNull(); + expect(result!.scopes).toEqual([ + 'openid', + 'profile', + 'list-operational-mcp', + 'manage-mcp-deployment', + ]); + }); + + it('should prefer protected resource scopes over auth server scopes', async () => { + const mockResourceMetadata: OAuthProtectedResourceMetadata = { + resource: 'https://example.com/mcp', + authorization_servers: ['https://auth.example.com'], + scopes_supported: ['mcp-read', 'mcp-write'], + }; + + const mockAuthServerMetadata: OAuthAuthorizationServerMetadata = { + issuer: 'https://auth.example.com', + authorization_endpoint: 'https://auth.example.com/authorize', + token_endpoint: 'https://auth.example.com/token', + scopes_supported: ['read', 'write', 'admin'], // Different scopes + }; + + mockFetch + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(mockResourceMetadata), + }) + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve(mockAuthServerMetadata), + }); + + const result = await OAuthUtils.discoverOAuthConfig( + 'https://example.com/mcp', + ); + + expect(result).not.toBeNull(); + // Should use protected resource scopes, not auth server scopes + expect(result!.scopes).toEqual(['mcp-read', 'mcp-write']); + }); + }); }); diff --git a/packages/core/src/mcp/oauth-utils.ts b/packages/core/src/mcp/oauth-utils.ts index e5d5f3b74..8f60d4f82 100644 --- a/packages/core/src/mcp/oauth-utils.ts +++ b/packages/core/src/mcp/oauth-utils.ts @@ -38,6 +38,7 @@ export interface OAuthProtectedResourceMetadata { resource_signing_alg_values_supported?: string[]; resource_encryption_alg_values_supported?: string[]; resource_encryption_enc_values_supported?: string[]; + scopes_supported?: string[]; } /** @@ -251,6 +252,11 @@ export class OAuthUtils { if (authServerMetadata) { const config = this.metadataToOAuthConfig(authServerMetadata); + // Merge scopes from protected resource metadata (RFC 9728) + // Protected resource scopes take precedence as they define the specific access requirements + if (resourceMetadata.scopes_supported?.length) { + config.scopes = resourceMetadata.scopes_supported; + } if (authServerMetadata.registration_endpoint) { debugLogger.debug( `Dynamic client registration is supported at: ${authServerMetadata.registration_endpoint}`, @@ -325,7 +331,13 @@ export class OAuthUtils { await this.discoverAuthorizationServerMetadata(authServerUrl); if (authServerMetadata) { - return this.metadataToOAuthConfig(authServerMetadata); + const config = this.metadataToOAuthConfig(authServerMetadata); + // Merge scopes from protected resource metadata (RFC 9728) + // Protected resource scopes take precedence as they define the specific access requirements + if (resourceMetadata.scopes_supported?.length) { + config.scopes = resourceMetadata.scopes_supported; + } + return config; } return null; diff --git a/packages/core/src/qwen/qwenOAuth2.test.ts b/packages/core/src/qwen/qwenOAuth2.test.ts index 7ff3207d8..41d06afbe 100644 --- a/packages/core/src/qwen/qwenOAuth2.test.ts +++ b/packages/core/src/qwen/qwenOAuth2.test.ts @@ -91,13 +91,6 @@ vi.mock('./sharedTokenManager.js', () => ({ }, })); -// Mock qrcode-terminal -vi.mock('qrcode-terminal', () => ({ - default: { - generate: vi.fn(), - }, -})); - // Mock open vi.mock('open', () => ({ default: vi.fn(), diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index fe72829e2..66446d7e2 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -14,15 +14,11 @@ vi.mock('../utils/fileUtils.js', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - readFileWithEncoding: vi.fn(), - readFileWithEncodingInfo: vi.fn(), + readFileWithLineAndLimit: vi.fn(), }; }); -import { - readFileWithEncoding, - readFileWithEncodingInfo, -} from '../utils/fileUtils.js'; +import { readFileWithLineAndLimit } from '../utils/fileUtils.js'; describe('StandardFileSystemService', () => { let fileSystem: StandardFileSystemService; @@ -37,58 +33,69 @@ describe('StandardFileSystemService', () => { }); describe('readTextFile', () => { - it('should read file content using readFileWithEncoding', async () => { - const testContent = 'Hello, World!'; - vi.mocked(readFileWithEncoding).mockResolvedValue(testContent); - - const result = await fileSystem.readTextFile('/test/file.txt'); - - expect(readFileWithEncoding).toHaveBeenCalledWith('/test/file.txt'); - expect(result).toBe(testContent); - }); - - it('should propagate readFileWithEncoding errors', async () => { - const error = new Error('ENOENT: File not found'); - vi.mocked(readFileWithEncoding).mockRejectedValue(error); - - await expect(fileSystem.readTextFile('/test/file.txt')).rejects.toThrow( - 'ENOENT: File not found', - ); - }); - }); - - describe('readTextFileWithInfo', () => { - it('should return content, encoding, and bom via readFileWithEncodingInfo', async () => { - const mockResult = { content: 'Hello', encoding: 'utf-8', bom: false }; - vi.mocked(readFileWithEncodingInfo).mockResolvedValue(mockResult); - - const result = await fileSystem.readTextFileWithInfo('/test/file.txt'); - - expect(readFileWithEncodingInfo).toHaveBeenCalledWith('/test/file.txt'); - expect(result).toEqual(mockResult); - }); - - it('should return non-UTF-8 encoding info for GBK file', async () => { - const mockResult = { - content: '你好世界', - encoding: 'gb18030', + it('should read file content and return ReadTextFileResponse', async () => { + vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ + content: 'Hello, World!', bom: false, - }; - vi.mocked(readFileWithEncodingInfo).mockResolvedValue(mockResult); + encoding: 'utf-8', + originalLineCount: 1, + }); - const result = await fileSystem.readTextFileWithInfo('/test/gbk.txt'); + const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); - expect(result.encoding).toBe('gb18030'); - expect(result.bom).toBe(false); - expect(result.content).toBe('你好世界'); + expect(readFileWithLineAndLimit).toHaveBeenCalledWith({ + path: '/test/file.txt', + limit: Infinity, + line: 0, + }); + expect(result.content).toBe('Hello, World!'); + expect(result._meta?.bom).toBe(false); + expect(result._meta?.encoding).toBe('utf-8'); }); - it('should propagate readFileWithEncodingInfo errors', async () => { + it('should pass limit and line params to readFileWithLineAndLimit', async () => { + vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ + content: 'line 5', + bom: false, + encoding: 'utf-8', + originalLineCount: 100, + }); + + const result = await fileSystem.readTextFile({ + path: '/test/file.txt', + limit: 10, + line: 5, + }); + + expect(readFileWithLineAndLimit).toHaveBeenCalledWith({ + path: '/test/file.txt', + limit: 10, + line: 5, + }); + expect(result._meta?.originalLineCount).toBe(100); + }); + + it('should return encoding info for GBK file', async () => { + vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ + content: '你好世界', + bom: false, + encoding: 'gb18030', + originalLineCount: 1, + }); + + const result = await fileSystem.readTextFile({ path: '/test/gbk.txt' }); + + expect(result.content).toBe('你好世界'); + expect(result._meta?.encoding).toBe('gb18030'); + expect(result._meta?.bom).toBe(false); + }); + + it('should propagate readFileWithLineAndLimit errors', async () => { const error = new Error('ENOENT: File not found'); - vi.mocked(readFileWithEncodingInfo).mockRejectedValue(error); + vi.mocked(readFileWithLineAndLimit).mockRejectedValue(error); await expect( - fileSystem.readTextFileWithInfo('/test/file.txt'), + fileSystem.readTextFile({ path: '/test/file.txt' }), ).rejects.toThrow('ENOENT: File not found'); }); }); @@ -97,7 +104,10 @@ describe('StandardFileSystemService', () => { it('should write file content using fs', async () => { vi.mocked(fs.writeFile).mockResolvedValue(); - await fileSystem.writeTextFile('/test/file.txt', 'Hello, World!'); + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'Hello, World!', + }); expect(fs.writeFile).toHaveBeenCalledWith( '/test/file.txt', @@ -109,8 +119,10 @@ describe('StandardFileSystemService', () => { it('should write file with BOM when bom option is true', async () => { vi.mocked(fs.writeFile).mockResolvedValue(); - await fileSystem.writeTextFile('/test/file.txt', 'Hello, World!', { - bom: true, + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'Hello, World!', + _meta: { bom: true }, }); // Verify that fs.writeFile was called with a Buffer that starts with BOM @@ -126,8 +138,10 @@ describe('StandardFileSystemService', () => { it('should write file without BOM when bom option is false', async () => { vi.mocked(fs.writeFile).mockResolvedValue(); - await fileSystem.writeTextFile('/test/file.txt', 'Hello, World!', { - bom: false, + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'Hello, World!', + _meta: { bom: false }, }); expect(fs.writeFile).toHaveBeenCalledWith( @@ -142,8 +156,10 @@ describe('StandardFileSystemService', () => { // Content that includes the BOM character (as readTextFile would return) const contentWithBOM = '\uFEFF' + 'Hello'; - await fileSystem.writeTextFile('/test/file.txt', contentWithBOM, { - bom: true, + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: contentWithBOM, + _meta: { bom: true }, }); // Verify that fs.writeFile was called with a Buffer that has only one BOM @@ -170,11 +186,14 @@ describe('StandardFileSystemService', () => { } expect(bomCount).toBe(1); }); + it('should write file with non-UTF-8 encoding using iconv-lite', async () => { vi.mocked(fs.writeFile).mockResolvedValue(); - await fileSystem.writeTextFile('/test/file.txt', '你好世界', { - encoding: 'gbk', + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: '你好世界', + _meta: { encoding: 'gbk' }, }); // Verify that fs.writeFile was called with a Buffer (iconv-encoded) @@ -186,8 +205,10 @@ describe('StandardFileSystemService', () => { it('should write file as UTF-8 when encoding is utf-8', async () => { vi.mocked(fs.writeFile).mockResolvedValue(); - await fileSystem.writeTextFile('/test/file.txt', 'Hello', { - encoding: 'utf-8', + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'Hello', + _meta: { encoding: 'utf-8' }, }); expect(fs.writeFile).toHaveBeenCalledWith( @@ -200,9 +221,10 @@ describe('StandardFileSystemService', () => { it('should preserve UTF-16LE BOM when writing back a UTF-16LE file', async () => { vi.mocked(fs.writeFile).mockResolvedValue(); - await fileSystem.writeTextFile('/test/file.txt', 'Hello', { - encoding: 'utf-16le', - bom: true, + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'Hello', + _meta: { encoding: 'utf-16le', bom: true }, }); // iconv-lite encodes as UTF-16LE; with bom:true the FF FE BOM is prepended @@ -218,9 +240,10 @@ describe('StandardFileSystemService', () => { it('should not add BOM when writing UTF-16LE file without bom flag', async () => { vi.mocked(fs.writeFile).mockResolvedValue(); - await fileSystem.writeTextFile('/test/file.txt', 'Hello', { - encoding: 'utf-16le', - bom: false, + await fileSystem.writeTextFile({ + path: '/test/file.txt', + content: 'Hello', + _meta: { encoding: 'utf-16le', bom: false }, }); // No BOM prepended — raw iconv-encoded buffer written directly @@ -232,66 +255,4 @@ describe('StandardFileSystemService', () => { expect(!(buf[0] === 0xff && buf[1] === 0xfe)).toBe(true); }); }); - - describe('detectFileBOM', () => { - it('should return true for file with UTF-8 BOM', async () => { - // Create a buffer with BOM - const bomBuffer = Buffer.from([0xef, 0xbb, 0xbf]); - - // Mock fs.open to return a file descriptor that fills buffer with BOM - vi.mocked(fs.open).mockImplementation( - async () => - ({ - read: async (buffer: Buffer, offset: number) => { - // Copy BOM bytes to the buffer - bomBuffer.copy(buffer, offset); - return { bytesRead: 3 }; - }, - close: async () => {}, - }) as unknown as fs.FileHandle, - ); - - const result = await fileSystem.detectFileBOM('/test/file.txt'); - expect(result).toBe(true); - }); - - it('should return false for file without BOM', async () => { - // Mock file without BOM (starts with plain text) - vi.mocked(fs.open).mockImplementation( - async () => - ({ - read: async (buffer: Buffer, offset: number) => { - // Copy plain text bytes ("// ") - const plainText = Buffer.from([0x2f, 0x2f, 0x20]); - plainText.copy(buffer, offset); - return { bytesRead: 3 }; - }, - close: async () => {}, - }) as unknown as fs.FileHandle, - ); - - const result = await fileSystem.detectFileBOM('/test/file.txt'); - expect(result).toBe(false); - }); - - it('should return false for non-existent file', async () => { - vi.mocked(fs.open).mockRejectedValue(new Error('ENOENT')); - - const result = await fileSystem.detectFileBOM('/test/nonexistent.txt'); - expect(result).toBe(false); - }); - - it('should return false for empty file', async () => { - vi.mocked(fs.open).mockImplementation( - async () => - ({ - read: async () => ({ bytesRead: 0 }), - close: async () => {}, - }) as unknown as fs.FileHandle, - ); - - const result = await fileSystem.detectFileBOM('/test/empty.txt'); - expect(result).toBe(false); - }); - }); }); diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 787d68929..a5017621a 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -7,16 +7,26 @@ import fs from 'node:fs/promises'; import * as path from 'node:path'; import { globSync } from 'glob'; -import { - readFileWithEncoding, - readFileWithEncodingInfo, -} from '../utils/fileUtils.js'; -import type { FileReadResult } from '../utils/fileUtils.js'; +import { readFileWithLineAndLimit } from '../utils/fileUtils.js'; import { iconvEncode, iconvEncodingExists, isUtf8CompatibleEncoding, } from '../utils/iconvHelper.js'; +import type { + ReadTextFileRequest, + WriteTextFileRequest, + WriteTextFileResponse, +} from '@agentclientprotocol/sdk'; + +export type ReadTextFileResponse = { + content: string; + _meta?: { + bom?: boolean; + encoding?: string; + originalLineCount?: number; + }; +}; /** * Supported file encodings for new files. @@ -35,43 +45,13 @@ export type FileEncodingType = (typeof FileEncoding)[keyof typeof FileEncoding]; * Interface for file system operations that may be delegated to different implementations */ export interface FileSystemService { - /** - * Read text content from a file - * - * @param filePath - The path to the file to read - * @returns The file content as a string - */ - readTextFile(filePath: string): Promise; + readTextFile( + params: Omit, + ): Promise; - /** - * Read text content from a file, returning both the content and encoding metadata. - * Combines readTextFile + detectFileBOM + detectFileEncoding into a single I/O pass. - * - * @param filePath - The path to the file to read - * @returns The file content, encoding name, and whether a UTF-8 BOM was present - */ - readTextFileWithInfo(filePath: string): Promise; - - /** - * Write text content to a file - * - * @param filePath - The path to the file to write - * @param content - The content to write - * @param options - Optional write options including whether to add BOM - */ writeTextFile( - filePath: string, - content: string, - options?: WriteTextFileOptions, - ): Promise; - - /** - * Detects if a file has UTF-8 BOM (Byte Order Mark). - * - * @param filePath - The path to the file to check - * @returns True if the file has BOM, false otherwise - */ - detectFileBOM(filePath: string): Promise; + params: Omit, + ): Promise; /** * Finds files with a given name within specified search paths. @@ -103,22 +83,6 @@ export interface WriteTextFileOptions { encoding?: string; } -/** - * Detects if a buffer has UTF-8 BOM (Byte Order Mark). - * UTF-8 BOM is the byte sequence EF BB BF. - * - * @param buffer - The buffer to check - * @returns True if the buffer starts with UTF-8 BOM - */ -function hasUTF8BOM(buffer: Buffer): boolean { - return ( - buffer.length >= 3 && - buffer[0] === 0xef && - buffer[1] === 0xbb && - buffer[2] === 0xbf - ); -} - /** * Return the BOM byte sequence for a given encoding name, or null if the * encoding does not use a standard BOM. Used when writing back a file that @@ -148,24 +112,26 @@ function getBOMBytesForEncoding(encoding: string): Buffer | null { * Standard file system implementation */ export class StandardFileSystemService implements FileSystemService { - async readTextFile(filePath: string): Promise { + async readTextFile( + params: Omit, + ): Promise { + const { path, limit, line } = params; // Use encoding-aware reader that handles BOM and non-UTF-8 encodings (e.g. GBK) - return readFileWithEncoding(filePath); - } - - async readTextFileWithInfo(filePath: string): Promise { - // Single I/O pass: returns content, encoding, and BOM flag together, - // eliminating the need for separate detectFileEncoding / detectFileBOM calls. - return readFileWithEncodingInfo(filePath); + const { content, bom, encoding, originalLineCount } = + await readFileWithLineAndLimit({ + path, + limit: limit ?? Number.POSITIVE_INFINITY, + line: line || 0, + }); + return { content, _meta: { bom, encoding, originalLineCount } }; } async writeTextFile( - filePath: string, - content: string, - options?: WriteTextFileOptions, - ): Promise { - const bom = options?.bom ?? false; - const encoding = options?.encoding; + params: Omit, + ): Promise { + const { content, path: filePath, _meta } = params; + const bom = _meta?.['bom'] ?? (false as boolean); + const encoding = _meta?.['encoding'] as string | undefined; // Check if a non-UTF-8 encoding is specified and supported by iconv-lite const isNonUtf8Encoding = @@ -199,27 +165,7 @@ export class StandardFileSystemService implements FileSystemService { } else { await fs.writeFile(filePath, content, 'utf-8'); } - } - - async detectFileBOM(filePath: string): Promise { - let fd: fs.FileHandle | undefined; - try { - // Read only the first 3 bytes to check for BOM - fd = await fs.open(filePath, 'r'); - const buffer = Buffer.alloc(3); - const { bytesRead } = await fd.read(buffer, 0, 3, 0); - - if (bytesRead < 3) { - return false; - } - - return hasUTF8BOM(buffer); - } catch { - // File doesn't exist or can't be read - treat as no BOM - return false; - } finally { - await fd?.close(); - } + return { _meta }; } findFiles(fileName: string, searchPaths: readonly string[]): string[] { diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 1e93076fd..96055840f 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -431,7 +431,30 @@ describe('ShellExecutionService', () => { expect(mockPtySpawn).toHaveBeenCalledWith( 'cmd.exe', - ['/d', '/s', '/c', 'dir "foo bar"'], + '/d /s /c dir "foo bar"', + expect.any(Object), + ); + mockGetShellConfiguration.mockReturnValue({ + executable: 'bash', + argsPrefix: ['-c'], + shell: 'bash', + }); + }); + + it('should use PowerShell on Windows with array args', async () => { + mockPlatform.mockReturnValue('win32'); + mockGetShellConfiguration.mockReturnValue({ + executable: 'powershell.exe', + argsPrefix: ['-NoProfile', '-Command'], + shell: 'powershell', + }); + await simulateExecution('Test-Path "C:\\Temp\\"', (pty) => + pty.onExit.mock.calls[0][0]({ exitCode: 0, signal: null }), + ); + + expect(mockPtySpawn).toHaveBeenCalledWith( + 'powershell.exe', + ['-NoProfile', '-Command', 'Test-Path "C:\\Temp\\"'], expect.any(Object), ); mockGetShellConfiguration.mockReturnValue({ @@ -840,7 +863,7 @@ describe('ShellExecutionService child_process fallback', () => { }); describe('Platform-Specific Behavior', () => { - it('should use cmd.exe and hide window on Windows', async () => { + it('should use cmd.exe with windowsVerbatimArguments on Windows', async () => { mockPlatform.mockReturnValue('win32'); mockGetShellConfiguration.mockReturnValue({ executable: 'cmd.exe', @@ -857,6 +880,34 @@ describe('ShellExecutionService child_process fallback', () => { expect.objectContaining({ detached: false, windowsHide: true, + windowsVerbatimArguments: true, + }), + ); + mockGetShellConfiguration.mockReturnValue({ + executable: 'bash', + argsPrefix: ['-c'], + shell: 'bash', + }); + }); + + it('should use PowerShell without windowsVerbatimArguments on Windows', async () => { + mockPlatform.mockReturnValue('win32'); + mockGetShellConfiguration.mockReturnValue({ + executable: 'powershell.exe', + argsPrefix: ['-NoProfile', '-Command'], + shell: 'powershell', + }); + await simulateExecution('Test-Path "C:\\Temp\\"', (cp) => + cp.emit('exit', 0, null), + ); + + expect(mockCpSpawn).toHaveBeenCalledWith( + 'powershell.exe', + ['-NoProfile', '-Command', 'Test-Path "C:\\Temp\\"'], + expect.objectContaining({ + detached: false, + windowsHide: true, + windowsVerbatimArguments: false, }), ); mockGetShellConfiguration.mockReturnValue({ diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 50cdc3a09..d43d0f190 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -224,15 +224,20 @@ export class ShellExecutionService { ): ShellExecutionHandle { try { const isWindows = os.platform() === 'win32'; - const { executable, argsPrefix } = getShellConfiguration(); + const { executable, argsPrefix, shell } = getShellConfiguration(); const shellArgs = [...argsPrefix, commandToExecute]; // Note: CodeQL flags this as js/shell-command-injection-from-environment. // This is intentional - CLI tool executes user-provided shell commands. + // + // windowsVerbatimArguments must only be true for cmd.exe: it skips + // Node's MSVC CRT escaping, which cmd.exe doesn't understand. For + // PowerShell (.NET), we need the default escaping so that args + // round-trip correctly through CommandLineToArgvW. const child = cpSpawn(executable, shellArgs, { cwd, stdio: ['ignore', 'pipe', 'pipe'], - windowsVerbatimArguments: isWindows, + windowsVerbatimArguments: isWindows && shell === 'cmd', detached: !isWindows, windowsHide: isWindows, env: { @@ -418,8 +423,21 @@ export class ShellExecutionService { try { const cols = shellExecutionConfig.terminalWidth ?? 80; const rows = shellExecutionConfig.terminalHeight ?? 30; - const { executable, argsPrefix } = getShellConfiguration(); - const args = [...argsPrefix, commandToExecute]; + const { executable, argsPrefix, shell } = getShellConfiguration(); + // On Windows with cmd.exe, pass args as a single string instead of + // an array. node-pty's argsToCommandLine re-quotes array elements + // that contain spaces, which mangles user-provided quoted arguments + // for cmd.exe (e.g., `type "hello world"` becomes + // `"type \"hello world\""`). + // + // For PowerShell, keep the array form: argsToCommandLine escapes for + // CommandLineToArgvW round-tripping, which .NET correctly parses. + // The string form breaks quoted paths ending in \ (e.g., "C:\Temp\") + // because CommandLineToArgvW treats \" as an escaped quote. + const args: string[] | string = + os.platform() === 'win32' && shell === 'cmd' + ? [...argsPrefix, commandToExecute].join(' ') + : [...argsPrefix, commandToExecute]; const ptyProcess = ptyInfo.module.spawn(executable, args, { cwd, diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 8b55e28a9..21ee04244 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -280,6 +280,38 @@ describe('EditTool', () => { ); }); + it('should return false and skip confirmation when approval mode is AUTO_EDIT', async () => { + fs.writeFileSync(filePath, 'some old content here'); + (mockConfig.getApprovalMode as Mock).mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + expect(confirmation).toBe(false); + }); + + it('should return false and skip confirmation when approval mode is YOLO', async () => { + fs.writeFileSync(filePath, 'some old content here'); + (mockConfig.getApprovalMode as Mock).mockReturnValue(ApprovalMode.YOLO); + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + expect(confirmation).toBe(false); + }); + it('should return false if old_string is not found', async () => { fs.writeFileSync(filePath, 'some content here'); const params: EditToolParams = { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 61a318190..474a6aace 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -27,7 +27,10 @@ import { ToolNames, ToolDisplayNames } from './tool-names.js'; import { logFileOperation } from '../telemetry/loggers.js'; import { FileOperationEvent } from '../telemetry/types.js'; import { FileOperation } from '../telemetry/metrics.js'; -import { getSpecificMimeType } from '../utils/fileUtils.js'; +import { + getSpecificMimeType, + fileExists as isFilefileExists, +} from '../utils/fileUtils.js'; import { getLanguageFromFilePath } from '../utils/language-detection.js'; import type { ModifiableDeclarativeTool, @@ -133,33 +136,40 @@ class EditToolInvocation implements ToolInvocation { private async calculateEdit(params: EditToolParams): Promise { const replaceAll = params.replace_all ?? false; let currentContent: string | null = null; - let fileExists = false; + let fileExists = await isFilefileExists(params.file_path); let isNewFile = false; let finalNewString = params.new_string; let finalOldString = params.old_string; let occurrences = 0; - let encoding = 'utf-8'; - let bom = false; let error: | { display: string; raw: string; type: ToolErrorType } | undefined = undefined; - - try { - const fileInfo = await this.config - .getFileSystemService() - .readTextFileWithInfo(params.file_path); - // Normalize line endings to LF for consistent processing. - currentContent = fileInfo.content.replace(/\r\n/g, '\n'); - fileExists = true; - // Encoding and BOM are returned from the same I/O pass, avoiding redundant reads. - encoding = fileInfo.encoding; - bom = fileInfo.bom; - } catch (err: unknown) { - if (!isNodeError(err) || err.code !== 'ENOENT') { - // Rethrow unexpected FS errors (permissions, etc.) - throw err; + let useBOM = false; + let detectedEncoding = 'utf-8'; + if (fileExists) { + try { + const fileInfo = await this.config + .getFileSystemService() + .readTextFile({ path: params.file_path }); + if (fileInfo._meta?.bom !== undefined) { + useBOM = fileInfo._meta.bom; + } else { + useBOM = + fileInfo.content.length > 0 && + fileInfo.content.codePointAt(0) === 0xfeff; + } + detectedEncoding = fileInfo._meta?.encoding || 'utf-8'; + // Normalize line endings to LF for consistent processing. + currentContent = fileInfo.content.replace(/\r\n/g, '\n'); + fileExists = true; + // Encoding and BOM are returned from the same I/O pass, avoiding redundant reads. + } catch (err: unknown) { + if (!isNodeError(err) || err.code !== 'ENOENT') { + // Rethrow unexpected FS errors (permissions, etc.) + throw err; + } + fileExists = false; } - fileExists = false; } const normalizedStrings = normalizeEditStrings( @@ -247,8 +257,8 @@ class EditToolInvocation implements ToolInvocation { occurrences, error, isNewFile, - encoding, - bom, + bom: useBOM, + encoding: detectedEncoding, }; } @@ -259,7 +269,8 @@ class EditToolInvocation implements ToolInvocation { async shouldConfirmExecute( abortSignal: AbortSignal, ): Promise { - if (this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT) { + const mode = this.config.getApprovalMode(); + if (mode === ApprovalMode.AUTO_EDIT || mode === ApprovalMode.YOLO) { return false; } @@ -388,18 +399,22 @@ class EditToolInvocation implements ToolInvocation { if (editData.isNewFile) { const useBOM = this.config.getDefaultFileEncoding() === FileEncoding.UTF8_BOM; - await this.config - .getFileSystemService() - .writeTextFile(this.params.file_path, editData.newContent, { + await this.config.getFileSystemService().writeTextFile({ + path: this.params.file_path, + content: editData.newContent, + _meta: { bom: useBOM, - }); + }, + }); } else { - await this.config - .getFileSystemService() - .writeTextFile(this.params.file_path, editData.newContent, { + await this.config.getFileSystemService().writeTextFile({ + path: this.params.file_path, + content: editData.newContent, + _meta: { bom: editData.bom, encoding: editData.encoding, - }); + }, + }); } const fileName = path.basename(this.params.file_path); @@ -581,28 +596,38 @@ Expectation for required parameters: return { getFilePath: (params: EditToolParams) => params.file_path, getCurrentContent: async (params: EditToolParams): Promise => { - try { - return this.config - .getFileSystemService() - .readTextFile(params.file_path); - } catch (err) { - if (!isNodeError(err) || err.code !== 'ENOENT') throw err; + const fileExists = await isFilefileExists(params.file_path); + if (fileExists) { + try { + const { content } = await this.config + .getFileSystemService() + .readTextFile({ path: params.file_path }); + return content; + } catch (err) { + if (!isNodeError(err) || err.code !== 'ENOENT') throw err; + return ''; + } + } else { return ''; } }, getProposedContent: async (params: EditToolParams): Promise => { - try { - const currentContent = await this.config - .getFileSystemService() - .readTextFile(params.file_path); - return applyReplacement( - currentContent, - params.old_string, - params.new_string, - params.old_string === '' && currentContent === '', - ); - } catch (err) { - if (!isNodeError(err) || err.code !== 'ENOENT') throw err; + if (fs.existsSync(params.file_path)) { + try { + const { content: currentContent } = await this.config + .getFileSystemService() + .readTextFile({ path: params.file_path }); + return applyReplacement( + currentContent, + params.old_string, + params.new_string, + params.old_string === '' && currentContent === '', + ); + } catch (err) { + if (!isNodeError(err) || err.code !== 'ENOENT') throw err; + return ''; + } + } else { return ''; } }, diff --git a/packages/core/src/tools/ls.test.ts b/packages/core/src/tools/ls.test.ts index 39a6b7b31..da6273eb1 100644 --- a/packages/core/src/tools/ls.test.ts +++ b/packages/core/src/tools/ls.test.ts @@ -41,6 +41,7 @@ describe('LSTool', () => { respectGitIgnore: true, respectQwenIgnore: true, }), + getTruncateToolOutputLines: () => 1000, storage: { getUserSkillsDir: () => userSkillsBase, }, @@ -100,7 +101,7 @@ describe('LSTool', () => { expect(result.llmContent).toContain('[DIR] subdir'); expect(result.llmContent).toContain('file1.txt'); - expect(result.returnDisplay).toBe('Listed 2 item(s).'); + expect(result.returnDisplay).toBe('Listed 2 item(s)'); }); it('should list files from secondary workspace directory', async () => { @@ -115,7 +116,7 @@ describe('LSTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('secondary-file.txt'); - expect(result.returnDisplay).toBe('Listed 1 item(s).'); + expect(result.returnDisplay).toBe('Listed 1 item(s)'); }); it('should handle empty directories', async () => { @@ -140,7 +141,7 @@ describe('LSTool', () => { expect(result.llmContent).toContain('file1.txt'); expect(result.llmContent).not.toContain('file2.log'); - expect(result.returnDisplay).toBe('Listed 1 item(s).'); + expect(result.returnDisplay).toBe('Listed 1 item(s)'); }); it('should respect gitignore patterns', async () => { @@ -154,7 +155,7 @@ describe('LSTool', () => { expect(result.llmContent).toContain('file1.txt'); expect(result.llmContent).not.toContain('file2.log'); // .git is always ignored by default. - expect(result.returnDisplay).toBe('Listed 2 item(s). (2 git-ignored)'); + expect(result.returnDisplay).toBe('Listed 2 item(s) (2 git-ignored)'); }); it('should respect qwenignore patterns', async () => { @@ -166,7 +167,7 @@ describe('LSTool', () => { expect(result.llmContent).toContain('file1.txt'); expect(result.llmContent).not.toContain('file2.log'); - expect(result.returnDisplay).toBe('Listed 2 item(s). (1 qwen-ignored)'); + expect(result.returnDisplay).toBe('Listed 2 item(s) (1 qwen-ignored)'); }); it('should handle non-directory paths', async () => { @@ -204,7 +205,7 @@ describe('LSTool', () => { typeof result.llmContent === 'string' ? result.llmContent : '' ) .split('\n') - .filter(Boolean); + .filter((l) => l.trim() && l.trim() !== '---'); const entries = lines.slice(1); // Skip header expect(entries[0]).toBe('[DIR] x-dir'); @@ -259,12 +260,70 @@ describe('LSTool', () => { // Should still list the other files expect(result.llmContent).toContain('file1.txt'); expect(result.llmContent).not.toContain('problematic.txt'); - expect(result.returnDisplay).toBe('Listed 1 item(s).'); + expect(result.returnDisplay).toBe('Listed 1 item(s)'); statSpy.mockRestore(); }); }); + describe('truncation', () => { + it('should truncate when entries exceed config line limit', async () => { + const lowLimitConfig = { + ...mockConfig, + getTruncateToolOutputLines: () => 5, + } as unknown as Config; + const lowLimitTool = new LSTool(lowLimitConfig); + + for (let i = 0; i < 10; i++) { + await fs.writeFile( + path.join(tempRootDir, `file${String(i).padStart(2, '0')}.txt`), + `content${i}`, + ); + } + + const invocation = lowLimitTool.build({ path: tempRootDir }); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('[5 items truncated]'); + expect(result.returnDisplay).toBe('Listed 10 item(s) (truncated)'); + }); + + it('should not truncate when entries are within limit', async () => { + for (let i = 0; i < 3; i++) { + await fs.writeFile( + path.join(tempRootDir, `file${i}.txt`), + `content${i}`, + ); + } + + const invocation = lsTool.build({ path: tempRootDir }); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).not.toContain('truncated'); + expect(result.returnDisplay).toBe('Listed 3 item(s)'); + }); + + it('should use singular "entry" when exactly one entry is truncated', async () => { + const lowLimitConfig = { + ...mockConfig, + getTruncateToolOutputLines: () => 2, + } as unknown as Config; + const lowLimitTool = new LSTool(lowLimitConfig); + + for (let i = 0; i < 3; i++) { + await fs.writeFile( + path.join(tempRootDir, `file${i}.txt`), + `content${i}`, + ); + } + + const invocation = lowLimitTool.build({ path: tempRootDir }); + const result = await invocation.execute(abortSignal); + + expect(result.llmContent).toContain('[1 item truncated]'); + }); + }); + describe('getDescription', () => { it('should return shortened relative path', () => { const deeplyNestedDir = path.join(tempRootDir, 'deeply', 'nested'); @@ -319,7 +378,7 @@ describe('LSTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toContain('secondary-file.txt'); - expect(result.returnDisplay).toBe('Listed 1 item(s).'); + expect(result.returnDisplay).toBe('Listed 1 item(s)'); }); }); }); diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index b8edbe163..877a1274b 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -18,6 +18,8 @@ import { createDebugLogger } from '../utils/debugLogger.js'; const debugLogger = createDebugLogger('LS'); +const MAX_ENTRY_COUNT = 100; + /** * Parameters for the LS tool */ @@ -216,12 +218,27 @@ class LSToolInvocation extends BaseToolInvocation { return a.name.localeCompare(b.name); }); - // Create formatted content for LLM - const directoryContent = entries + const totalEntryCount = entries.length; + const entryLimit = Math.min( + MAX_ENTRY_COUNT, + this.config.getTruncateToolOutputLines(), + ); + const truncated = totalEntryCount > entryLimit; + + const entriesToShow = truncated ? entries.slice(0, entryLimit) : entries; + + const directoryContent = entriesToShow .map((entry) => `${entry.isDirectory ? '[DIR] ' : ''}${entry.name}`) .join('\n'); - let resultMessage = `Directory listing for ${this.params.path}:\n${directoryContent}`; + let resultMessage = `Listed ${totalEntryCount} item(s) in ${this.params.path}:\n---\n${directoryContent}`; + + if (truncated) { + const omittedEntries = totalEntryCount - entryLimit; + const entryTerm = omittedEntries === 1 ? 'item' : 'items'; + resultMessage += `\n---\n[${omittedEntries} ${entryTerm} truncated] ...`; + } + const ignoredMessages = []; if (gitIgnoredCount > 0) { ignoredMessages.push(`${gitIgnoredCount} git-ignored`); @@ -233,10 +250,13 @@ class LSToolInvocation extends BaseToolInvocation { resultMessage += `\n\n(${ignoredMessages.join(', ')})`; } - let displayMessage = `Listed ${entries.length} item(s).`; + let displayMessage = `Listed ${totalEntryCount} item(s)`; if (ignoredMessages.length > 0) { displayMessage += ` (${ignoredMessages.join(', ')})`; } + if (truncated) { + displayMessage += ' (truncated)'; + } return { llmContent: resultMessage, diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 9913cba05..e2110810b 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -245,6 +245,22 @@ export class ToolRegistry { } } + /** + * Disconnects an MCP server by removing its tools, prompts, and disconnecting the client. + * Unlike disableMcpServer, this does NOT add the server to the exclusion list. + * @param serverName The name of the server to disconnect. + */ + async disconnectServer(serverName: string): Promise { + // Remove tools from registry + this.removeMcpToolsByServer(serverName); + + // Remove prompts + this.config.getPromptRegistry().removePromptsByServer(serverName); + + // Disconnect the MCP client + await this.mcpClientManager.disconnectServer(serverName); + } + /** * Disables an MCP server by removing its tools, prompts, and disconnecting the client. * Also updates the config's exclusion list. diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index e096b0a72..057eb33dd 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -14,7 +14,7 @@ import { type Mocked, } from 'vitest'; import type { WriteFileToolParams } from './write-file.js'; -import { getCorrectedFileContent, WriteFileTool } from './write-file.js'; +import { WriteFileTool } from './write-file.js'; import { ToolErrorType } from './tool-error.js'; import type { FileDiff, ToolEditConfirmationDetails } from './tools.js'; import { ToolConfirmationOutcome } from './tools.js'; @@ -193,70 +193,6 @@ describe('WriteFileTool', () => { }); }); - describe('getCorrectedFileContent', () => { - it('should return proposed content unchanged for a new file', async () => { - const filePath = path.join(rootDir, 'new_corrected_file.txt'); - const proposedContent = 'Proposed new content.'; - - const result = await getCorrectedFileContent( - mockConfig, - filePath, - proposedContent, - ); - - expect(result.correctedContent).toBe(proposedContent); - expect(result.originalContent).toBe(''); - expect(result.fileExists).toBe(false); - expect(result.error).toBeUndefined(); - }); - - it('should return proposed content unchanged for an existing file', async () => { - const filePath = path.join(rootDir, 'existing_corrected_file.txt'); - const originalContent = 'Original existing content.'; - const proposedContent = 'Proposed replacement content.'; - fs.writeFileSync(filePath, originalContent, 'utf8'); - - const result = await getCorrectedFileContent( - mockConfig, - filePath, - proposedContent, - ); - - expect(result.correctedContent).toBe(proposedContent); - expect(result.originalContent).toBe(originalContent); - expect(result.fileExists).toBe(true); - expect(result.error).toBeUndefined(); - }); - - it('should return error if reading an existing file fails (e.g. permissions)', async () => { - const filePath = path.join(rootDir, 'unreadable_file.txt'); - const proposedContent = 'some content'; - fs.writeFileSync(filePath, 'content', { mode: 0o000 }); - - const readError = new Error('Permission denied'); - vi.spyOn(fsService, 'readTextFile').mockImplementationOnce(() => - Promise.reject(readError), - ); - - const result = await getCorrectedFileContent( - mockConfig, - filePath, - proposedContent, - ); - - expect(fsService.readTextFile).toHaveBeenCalledWith(filePath); - expect(result.correctedContent).toBe(proposedContent); - expect(result.originalContent).toBe(''); - expect(result.fileExists).toBe(true); - expect(result.error).toEqual({ - message: 'Permission denied', - code: undefined, - }); - - fs.chmodSync(filePath, 0o600); - }); - }); - describe('shouldConfirmExecute', () => { const abortSignal = new AbortController().signal; @@ -277,6 +213,26 @@ describe('WriteFileTool', () => { fs.chmodSync(filePath, 0o600); }); + it('should return false and skip confirmation when approval mode is AUTO_EDIT', async () => { + mockConfigInternal.getApprovalMode.mockReturnValue( + ApprovalMode.AUTO_EDIT, + ); + const filePath = path.join(rootDir, 'auto_edit_skip_confirm.txt'); + const params = { file_path: filePath, content: 'content' }; + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute(abortSignal); + expect(confirmation).toBe(false); + }); + + it('should return false and skip confirmation when approval mode is YOLO', async () => { + mockConfigInternal.getApprovalMode.mockReturnValue(ApprovalMode.YOLO); + const filePath = path.join(rootDir, 'yolo_skip_confirm.txt'); + const params = { file_path: filePath, content: 'content' }; + const invocation = tool.build(params); + const confirmation = await invocation.shouldConfirmExecute(abortSignal); + expect(confirmation).toBe(false); + }); + it('should request confirmation with diff for a new file', async () => { const filePath = path.join(rootDir, 'confirm_new_file.txt'); const proposedContent = 'Proposed new content for confirmation.'; @@ -484,7 +440,9 @@ describe('WriteFileTool', () => { /Successfully created and wrote to new file/, ); expect(fs.existsSync(filePath)).toBe(true); - const writtenContent = await fsService.readTextFile(filePath); + const { content: writtenContent } = await fsService.readTextFile({ + path: filePath, + }); expect(writtenContent).toBe(proposedContent); const display = result.returnDisplay as FileDiff; expect(display.fileName).toBe('execute_new_file.txt'); @@ -516,7 +474,9 @@ describe('WriteFileTool', () => { const result = await invocation.execute(abortSignal); expect(result.llmContent).toMatch(/Successfully overwrote file/); - const writtenContent = await fsService.readTextFile(filePath); + const { content: writtenContent } = await fsService.readTextFile({ + path: filePath, + }); expect(writtenContent).toBe(proposedContent); const display = result.returnDisplay as FileDiff; expect(display.fileName).toBe('execute_existing_file.txt'); @@ -528,6 +488,36 @@ describe('WriteFileTool', () => { ); }); + it('should treat metadata ENOENT as new file when readTextFile returned empty content', async () => { + const filePath = path.join(rootDir, 'execute_acp_like_missing_file.txt'); + const proposedContent = 'content from acp-like flow'; + const writeSpy = vi.spyOn(fsService, 'writeTextFile'); + + // Simulate ENOENT: file does not exist, readTextFile throws ENOENT. + const enoentError = new Error('File not found') as NodeJS.ErrnoException; + enoentError.code = 'ENOENT'; + vi.spyOn(fsService, 'readTextFile').mockRejectedValueOnce(enoentError); + + const params = { file_path: filePath, content: proposedContent }; + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.error).toBeUndefined(); + expect(result.llmContent).toMatch( + /Successfully created and wrote to new file/, + ); + expect(writeSpy).toHaveBeenCalledWith({ + path: filePath, + content: proposedContent, + _meta: { + bom: false, + encoding: undefined, + }, + }); + expect(fs.existsSync(filePath)).toBe(true); + expect(fs.readFileSync(filePath, 'utf8')).toBe(proposedContent); + }); + it('should create directory if it does not exist', async () => { const dirPath = path.join(rootDir, 'new_dir_for_write'); const filePath = path.join(dirPath, 'file_in_new_dir.txt'); @@ -757,9 +747,10 @@ describe('WriteFileTool', () => { await invocation.execute(abortSignal); // Verify writeTextFile was called with bom: true - expect(writeSpy).toHaveBeenCalledWith(filePath, newContent, { - bom: true, - encoding: 'utf-8', + expect(writeSpy).toHaveBeenCalledWith({ + path: filePath, + content: newContent, + _meta: { bom: true, encoding: 'utf-8' }, }); // Cleanup @@ -784,9 +775,10 @@ describe('WriteFileTool', () => { await invocation.execute(abortSignal); // Verify writeTextFile was called with bom: false - expect(writeSpy).toHaveBeenCalledWith(filePath, newContent, { - bom: false, - encoding: 'utf-8', + expect(writeSpy).toHaveBeenCalledWith({ + path: filePath, + content: newContent, + _meta: { bom: false, encoding: 'utf-8' }, }); // Cleanup @@ -812,8 +804,10 @@ describe('WriteFileTool', () => { await invocation.execute(abortSignal); // Verify writeTextFile was called with bom: false (default is utf-8) - expect(writeSpy).toHaveBeenCalledWith(filePath, newContent, { - bom: false, + expect(writeSpy).toHaveBeenCalledWith({ + path: filePath, + content: newContent, + _meta: { bom: false, encoding: undefined }, }); // Cleanup @@ -844,8 +838,10 @@ describe('WriteFileTool', () => { await invocation.execute(abortSignal); // Verify writeTextFile was called with bom: true - expect(writeSpy).toHaveBeenCalledWith(filePath, newContent, { - bom: true, + expect(writeSpy).toHaveBeenCalledWith({ + path: filePath, + content: newContent, + _meta: { bom: true, encoding: undefined }, }); // Restore mock diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 4085e3b69..9da02e4d4 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -37,7 +37,10 @@ import { IdeClient } from '../ide/ide-client.js'; import { logFileOperation } from '../telemetry/loggers.js'; import { FileOperationEvent } from '../telemetry/types.js'; import { FileOperation } from '../telemetry/metrics.js'; -import { getSpecificMimeType } from '../utils/fileUtils.js'; +import { + getSpecificMimeType, + fileExists as isFilefileExists, +} from '../utils/fileUtils.js'; import { getLanguageFromFilePath } from '../utils/language-detection.js'; import { createDebugLogger } from '../utils/debugLogger.js'; @@ -68,47 +71,6 @@ export interface WriteFileToolParams { ai_proposed_content?: string; } -interface GetCorrectedFileContentResult { - originalContent: string; - correctedContent: string; - fileExists: boolean; - error?: { message: string; code?: string }; -} - -export async function getCorrectedFileContent( - config: Config, - filePath: string, - proposedContent: string, -): Promise { - let originalContent = ''; - let fileExists = false; - const correctedContent = proposedContent; - - try { - originalContent = await config - .getFileSystemService() - .readTextFile(filePath); - fileExists = true; // File exists and was read - } catch (err) { - if (isNodeError(err) && err.code === 'ENOENT') { - fileExists = false; - originalContent = ''; - } else { - // File exists but could not be read (permissions, etc.) - fileExists = true; // Mark as existing but problematic - originalContent = ''; // Can't use its content - const error = { - message: getErrorMessage(err), - code: isNodeError(err) ? err.code : undefined, - }; - // Return early as we can't proceed with content correction meaningfully - return { originalContent, correctedContent, fileExists, error }; - } - } - - return { originalContent, correctedContent, fileExists }; -} - class WriteFileToolInvocation extends BaseToolInvocation< WriteFileToolParams, ToolResult @@ -135,22 +97,26 @@ class WriteFileToolInvocation extends BaseToolInvocation< override async shouldConfirmExecute( _abortSignal: AbortSignal, ): Promise { - if (this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT) { + const mode = this.config.getApprovalMode(); + if (mode === ApprovalMode.AUTO_EDIT || mode === ApprovalMode.YOLO) { return false; } - const correctedContentResult = await getCorrectedFileContent( - this.config, - this.params.file_path, - this.params.content, - ); - - if (correctedContentResult.error) { - // If file exists but couldn't be read, we can't show a diff for confirmation. - return false; + let originalContent = ''; + const fileExists = await isFilefileExists(this.params.file_path); + if (fileExists) { + try { + const { content } = await this.config + .getFileSystemService() + .readTextFile({ path: this.params.file_path }); + originalContent = content; + } catch (err) { + debugLogger.error( + `Error reading existing file for confirmation: ${getErrorMessage(err)}`, + ); + return false; + } } - - const { originalContent, correctedContent } = correctedContentResult; const relativePath = makeRelative( this.params.file_path, this.config.getTargetDir(), @@ -160,7 +126,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< const fileDiff = Diff.createPatch( fileName, originalContent, // Original content (empty if new file or unreadable) - correctedContent, // Content after potential correction + this.params.content, // Content after potential correction 'Current', 'Proposed', DEFAULT_DIFF_OPTIONS, @@ -169,7 +135,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< const ideClient = await IdeClient.getInstance(); const ideConfirmation = this.config.getIdeMode() && ideClient.isDiffingEnabled() - ? ideClient.openDiff(this.params.file_path, correctedContent) + ? ideClient.openDiff(this.params.file_path, this.params.content) : undefined; const confirmationDetails: ToolEditConfirmationDetails = { @@ -179,7 +145,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< filePath: this.params.file_path, fileDiff, originalContent, - newContent: correctedContent, + newContent: this.params.content, onConfirm: async (outcome: ToolConfirmationOutcome) => { if (outcome === ToolConfirmationOutcome.ProceedAlways) { this.config.setApprovalMode(ApprovalMode.AUTO_EDIT); @@ -201,81 +167,76 @@ class WriteFileToolInvocation extends BaseToolInvocation< const { file_path, content, ai_proposed_content, modified_by_user } = this.params; - const correctedContentResult = await getCorrectedFileContent( - this.config, - file_path, - content, - ); - - if (correctedContentResult.error) { - const errDetails = correctedContentResult.error; - const errorMsg = errDetails.code - ? `Error checking existing file '${file_path}': ${errDetails.message} (${errDetails.code})` - : `Error checking existing file: ${errDetails.message}`; - return { - llmContent: errorMsg, - returnDisplay: errorMsg, - error: { - message: errorMsg, - type: ToolErrorType.FILE_WRITE_FAILURE, - }, - }; - } - - const { - originalContent, - correctedContent: fileContent, - fileExists, - } = correctedContentResult; - // fileExists is true if the file existed (and was readable or unreadable but caught by readError). - // fileExists is false if the file did not exist (ENOENT). - const isNewFile = - !fileExists || - (correctedContentResult.error !== undefined && - !correctedContentResult.fileExists); - - try { - const dirName = path.dirname(file_path); - if (!fs.existsSync(dirName)) { - fs.mkdirSync(dirName, { recursive: true }); - } - - // Check if file exists and has BOM to preserve encoding - // For new files, use the configured default encoding - let useBOM = false; - let detectedEncoding: string | undefined; - if (!isNewFile) { - // Use readTextFileWithInfo for a single I/O pass that returns encoding - // and BOM metadata together, avoiding separate detectFileBOM / detectFileEncoding calls. + let fileExists = await isFilefileExists(file_path); + let originalContent = ''; + let useBOM = false; + let detectedEncoding: string | undefined; + const dirName = path.dirname(file_path); + if (fileExists) { + try { const fileInfo = await this.config .getFileSystemService() - .readTextFileWithInfo(file_path); - useBOM = fileInfo.bom; - detectedEncoding = fileInfo.encoding; - } else { - useBOM = this.config.getDefaultFileEncoding() === FileEncoding.UTF8_BOM; + .readTextFile({ path: file_path }); + if (fileInfo._meta?.bom !== undefined) { + useBOM = fileInfo._meta.bom; + } else { + useBOM = + fileInfo.content.length > 0 && + fileInfo.content.codePointAt(0) === 0xfeff; + } + detectedEncoding = fileInfo._meta?.encoding || 'utf-8'; + originalContent = fileInfo.content; + fileExists = true; // File exists and was read + } catch (err) { + if (isNodeError(err) && err.code === 'ENOENT') { + fileExists = false; + } else { + const error = { + message: getErrorMessage(err), + code: isNodeError(err) ? err.code : undefined, + }; + const errorMsg = error.code + ? `Error checking existing file '${file_path}': ${error.message} (${error.code})` + : `Error checking existing file: ${error.message}`; + return { + llmContent: errorMsg, + returnDisplay: errorMsg, + error: { + message: errorMsg, + type: ToolErrorType.FILE_WRITE_FAILURE, + }, + }; + } } + } - await this.config - .getFileSystemService() - .writeTextFile(file_path, fileContent, { + if (!fileExists) { + fs.mkdirSync(dirName, { recursive: true }); + useBOM = this.config.getDefaultFileEncoding() === FileEncoding.UTF8_BOM; + detectedEncoding = undefined; + } + + try { + await this.config.getFileSystemService().writeTextFile({ + path: file_path, + content, + _meta: { bom: useBOM, encoding: detectedEncoding, - }); + }, + }); // Generate diff for display result const fileName = path.basename(file_path); // If there was a readError, originalContent in correctedContentResult is '', // but for the diff, we want to show the original content as it was before the write if possible. // However, if it was unreadable, currentContentForDiff will be empty. - const currentContentForDiff = correctedContentResult.error - ? '' // Or some indicator of unreadable content - : originalContent; + const currentContentForDiff = originalContent; const fileDiff = Diff.createPatch( fileName, currentContentForDiff, - fileContent, + content, 'Original', 'Written', DEFAULT_DIFF_OPTIONS, @@ -290,7 +251,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< ); const llmSuccessMessageParts = [ - isNewFile + !fileExists ? `Successfully created and wrote to new file: ${file_path}.` : `Successfully overwrote file: ${file_path}.`, ]; @@ -304,9 +265,11 @@ class WriteFileToolInvocation extends BaseToolInvocation< const mimetype = getSpecificMimeType(file_path); const programmingLanguage = getLanguageFromFilePath(file_path); const extension = path.extname(file_path); - const operation = isNewFile ? FileOperation.CREATE : FileOperation.UPDATE; + const operation = !fileExists + ? FileOperation.CREATE + : FileOperation.UPDATE; - const lineCount = fileContent.split('\n').length; + const lineCount = content.split('\n').length; logFileOperation( this.config, new FileOperationEvent( @@ -322,8 +285,8 @@ class WriteFileToolInvocation extends BaseToolInvocation< const displayResult: FileDiff = { fileDiff, fileName, - originalContent: correctedContentResult.originalContent, - newContent: correctedContentResult.correctedContent, + originalContent, + newContent: content, diffStat, }; @@ -458,21 +421,22 @@ export class WriteFileTool return { getFilePath: (params: WriteFileToolParams) => params.file_path, getCurrentContent: async (params: WriteFileToolParams) => { - const correctedContentResult = await getCorrectedFileContent( - this.config, - params.file_path, - params.content, - ); - return correctedContentResult.originalContent; - }, - getProposedContent: async (params: WriteFileToolParams) => { - const correctedContentResult = await getCorrectedFileContent( - this.config, - params.file_path, - params.content, - ); - return correctedContentResult.correctedContent; + const fileExists = await isFilefileExists(params.file_path); + if (fileExists) { + try { + const { content } = await this.config + .getFileSystemService() + .readTextFile({ path: params.file_path }); + return content; + } catch (err) { + if (!isNodeError(err) || err.code !== 'ENOENT') throw err; + return ''; + } + } else { + return ''; + } }, + getProposedContent: async (params: WriteFileToolParams) => params.content, createUpdatedParams: ( _oldContent: string, modifiedProposedContent: string, diff --git a/packages/core/src/utils/fileUtils.test.ts b/packages/core/src/utils/fileUtils.test.ts index 6dc38e4d7..0daa68c4e 100644 --- a/packages/core/src/utils/fileUtils.test.ts +++ b/packages/core/src/utils/fileUtils.test.ts @@ -33,6 +33,7 @@ import { fileExists, } from './fileUtils.js'; import type { Config } from '../config/config.js'; +import { StandardFileSystemService } from '../services/fileSystemService.js'; vi.mock('mime/lite', () => ({ default: { getType: vi.fn() }, @@ -52,10 +53,13 @@ describe('fileUtils', () => { let nonexistentFilePath: string; let directoryPath: string; + const fsService = new StandardFileSystemService(); + const mockConfig = { getTruncateToolOutputThreshold: () => 2500, getTruncateToolOutputLines: () => 500, getTargetDir: () => tempRootDir, + getFileSystemService: () => fsService, } as unknown as Config; beforeEach(() => { @@ -838,7 +842,7 @@ describe('fileUtils', () => { it('should handle read errors for text files', async () => { actualNodeFs.writeFileSync(testTextFilePath, 'content'); // File must exist for initial statSync const readError = new Error('Simulated read error'); - vi.spyOn(fsPromises, 'readFile').mockRejectedValueOnce(readError); + vi.spyOn(fsService, 'readTextFile').mockRejectedValueOnce(readError); const result = await processSingleFileContent( testTextFilePath, diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 05de408ef..41029138e 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -260,6 +260,40 @@ export async function readFileWithEncoding(filePath: string): Promise { return result.content; } +export async function countFileLines(filePath: string): Promise { + const result = await readFileWithEncodingInfo(filePath); + return result.content.split('\n').length; +} + +export async function readFileWithLineAndLimit(params: { + path: string; + limit: number; + line?: number; +}): Promise<{ + content: string; + bom?: boolean; + encoding?: string; + originalLineCount: number; +}> { + const { path: filePath, limit, line } = params; + const { content, encoding, bom } = await readFileWithEncodingInfo(filePath); + const lines = content.split('\n'); + const originalLineCount = lines.length; + const startLine = line || 0; + // Ensure endLine does not exceed originalLineCount + const endLine = Math.min(startLine + limit, originalLineCount); + // Ensure selectedLines doesn't try to slice beyond array bounds if startLine is too high + const actualStartLine = Math.min(startLine, originalLineCount); + const selectedLines = lines.slice(actualStartLine, endLine); + + return { + content: selectedLines.join('\n'), + bom, + encoding, + originalLineCount, + }; +} + /** * Detect the encoding of a file by reading a sample from its beginning. * Returns the encoding name (e.g. 'utf-8', 'gbk', 'shift_jis'). @@ -468,8 +502,8 @@ export interface ProcessedFileReadResult { returnDisplay: string; error?: string; // Optional error message for the LLM if file processing failed errorType?: ToolErrorType; // Structured error type + originalLineCount?: number; // For text files, the total number of lines in the original file isTruncated?: boolean; // For text files, indicates if content was truncated - originalLineCount?: number; // For text files linesShown?: [number, number]; // For text files [startLine, endLine] (1-based for display) } @@ -550,20 +584,18 @@ export async function processSingleFileContent( } case 'text': { // Use BOM-aware reader to avoid leaving a BOM character in content and to support UTF-16/32 transparently - const content = await readFileWithEncoding(filePath); - const lines = content.split('\n').map((line) => line.trimEnd()); - const originalLineCount = lines.length; - + const { content, _meta } = await config + .getFileSystemService() + .readTextFile({ + path: filePath, + limit: limit ?? config.getTruncateToolOutputLines(), + line: offset, + }); + const originalLineCount = + _meta?.originalLineCount ?? (await countFileLines(filePath)); + const selectedLines = content.split('\n').map((line) => line.trimEnd()); const startLine = offset || 0; - const configLineLimit = config.getTruncateToolOutputLines(); const configCharLimit = config.getTruncateToolOutputThreshold(); - const effectiveLimit = limit === undefined ? configLineLimit : limit; - - // Ensure endLine does not exceed originalLineCount - const endLine = Math.min(startLine + effectiveLimit, originalLineCount); - // Ensure selectedLines doesn't try to slice beyond array bounds if startLine is too high - const actualStartLine = Math.min(startLine, originalLineCount); - const selectedLines = lines.slice(actualStartLine, endLine); // Apply character limit truncation let llmContent = ''; @@ -603,11 +635,7 @@ export async function processSingleFileContent( linesIncluded = selectedLines.length; } - // Calculate actual end line shown - const actualEndLine = contentLengthTruncated - ? actualStartLine + linesIncluded - : endLine; - + const actualEndLine = startLine + linesIncluded; const contentRangeTruncated = startLine > 0 || actualEndLine < originalLineCount; const isTruncated = contentRangeTruncated || contentLengthTruncated; @@ -616,7 +644,7 @@ export async function processSingleFileContent( let returnDisplay = ''; if (isTruncated) { returnDisplay = `Read lines ${ - actualStartLine + 1 + startLine + 1 }-${actualEndLine} of ${originalLineCount} from ${relativePathForDisplay}`; if (contentLengthTruncated) { returnDisplay += ' (truncated)'; @@ -628,7 +656,7 @@ export async function processSingleFileContent( returnDisplay, isTruncated, originalLineCount, - linesShown: [actualStartLine + 1, actualEndLine], + linesShown: [startLine + 1, actualEndLine], }; } case 'image': diff --git a/packages/core/src/utils/readManyFiles.test.ts b/packages/core/src/utils/readManyFiles.test.ts index 859753fef..e043eed1c 100644 --- a/packages/core/src/utils/readManyFiles.test.ts +++ b/packages/core/src/utils/readManyFiles.test.ts @@ -12,6 +12,7 @@ import os from 'node:os'; import type { PartListUnion } from '@google/genai'; import { readManyFiles } from './readManyFiles.js'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; +import { StandardFileSystemService } from '../services/fileSystemService.js'; import type { Config } from '../config/config.js'; import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js'; @@ -44,6 +45,7 @@ describe('readManyFiles', () => { getWorkspaceContext: () => createMockWorkspaceContext(rootDir), getTruncateToolOutputLines: () => 1000, getTruncateToolOutputThreshold: () => 2500, + getFileSystemService: () => new StandardFileSystemService(), }) as unknown as Config; async function createTestFile( diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 1f0476866..1c839530f 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -913,3 +913,13 @@ export function isCommandNeedsPermission(command: string): { reason: 'Command requires permission to execute.', }; } + +// ConPTY on Windows builds <= 19041 has known reliability issues (missing +// output, hangs). VS Code uses the same cutoff: microsoft/vscode#123725. +const CONPTY_MIN_WINDOWS_BUILD = 19042; + +export function shouldDefaultToNodePty(): boolean { + if (os.platform() !== 'win32') return true; + const build = parseInt(os.release().split('.')[2] ?? '', 10); + return !isNaN(build) && build >= CONPTY_MIN_WINDOWS_BUILD; +} diff --git a/packages/test-utils/package.json b/packages/test-utils/package.json index e5f087f3c..eadfec8cc 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.12.0", + "version": "0.12.3", "private": true, "main": "src/index.ts", "license": "Apache-2.0", diff --git a/packages/vscode-ide-companion/assets/sidebar-icon.svg b/packages/vscode-ide-companion/assets/sidebar-icon.svg new file mode 100644 index 000000000..51cdab785 --- /dev/null +++ b/packages/vscode-ide-companion/assets/sidebar-icon.svg @@ -0,0 +1,6 @@ + + + diff --git a/packages/vscode-ide-companion/package.json b/packages/vscode-ide-companion/package.json index 79e6193df..22f2a2bc5 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.12.0", + "version": "0.12.3", "publisher": "qwenlm", "icon": "assets/icon.png", "repository": { @@ -28,7 +28,13 @@ "ide companion" ], "activationEvents": [ - "onStartupFinished" + "onStartupFinished", + "onView:qwen-code.chatView.sidebar", + "onView:qwen-code.chatView.secondary", + "onCommand:qwen-code.openChat", + "onCommand:qwen-code.focusChat", + "onCommand:qwen-code.newConversation", + "onCommand:qwen-code.showLogs" ], "contributes": { "jsonValidation": [ @@ -37,6 +43,44 @@ "url": "./schemas/settings.schema.json" } ], + "viewsContainers": { + "activitybar": [ + { + "id": "qwen-code-sidebar", + "title": "Qwen Code", + "icon": "assets/sidebar-icon.svg", + "when": "qwen-code:doesNotSupportSecondarySidebar" + } + ], + "secondarySidebar": [ + { + "id": "qwen-code-secondary", + "title": "Qwen Code", + "icon": "assets/sidebar-icon.svg", + "when": "!qwen-code:doesNotSupportSecondarySidebar" + } + ] + }, + "views": { + "qwen-code-sidebar": [ + { + "type": "webview", + "id": "qwen-code.chatView.sidebar", + "name": "Qwen Code", + "icon": "assets/sidebar-icon.svg", + "when": "qwen-code:doesNotSupportSecondarySidebar" + } + ], + "qwen-code-secondary": [ + { + "type": "webview", + "id": "qwen-code.chatView.secondary", + "name": "Qwen Code", + "icon": "assets/sidebar-icon.svg", + "when": "!qwen-code:doesNotSupportSecondarySidebar" + } + ] + }, "languages": [ { "id": "qwen-diff-editable" @@ -69,6 +113,18 @@ { "command": "qwen-code.login", "title": "Qwen Code: Login" + }, + { + "command": "qwen-code.focusChat", + "title": "Qwen Code: Focus Chat View" + }, + { + "command": "qwen-code.newConversation", + "title": "Qwen Code: New Conversation" + }, + { + "command": "qwen-code.showLogs", + "title": "Qwen Code: Show Logs" } ], "menus": { @@ -113,6 +169,11 @@ "command": "qwen.diff.accept", "key": "cmd+s", "when": "qwen.diff.isVisible" + }, + { + "command": "qwen-code.focusChat", + "key": "ctrl+shift+l", + "mac": "cmd+shift+l" } ] }, diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index abb6e519a..8b4f8d023 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -579,11 +579,9 @@ "type": "object", "properties": { "displayMode": { - "description": "Display mode for multi-agent sessions. \"tmux\" uses tmux panes, \"iterm2\" uses iTerm2 tabs, \"in-process\" runs in the current terminal. Options: in-process, tmux, iterm2", + "description": "Display mode for multi-agent sessions. Currently only \"in-process\" is supported. Options: in-process", "enum": [ - "in-process", - "tmux", - "iterm2" + "in-process" ] }, "arena": { diff --git a/packages/vscode-ide-companion/src/commands/index.test.ts b/packages/vscode-ide-companion/src/commands/index.test.ts new file mode 100644 index 000000000..e05f14272 --- /dev/null +++ b/packages/vscode-ide-companion/src/commands/index.test.ts @@ -0,0 +1,124 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { + focusChatCommand, + openNewChatTabCommand, + registerNewCommands, +} from './index.js'; + +const { + registerCommand, + executeCommand, + showWarningMessage, + showInformationMessage, +} = vi.hoisted(() => ({ + registerCommand: vi.fn( + (_id: string, handler: (...args: unknown[]) => unknown) => ({ + dispose: vi.fn(), + handler, + }), + ), + executeCommand: vi.fn(), + showWarningMessage: vi.fn(), + showInformationMessage: vi.fn(), +})); + +vi.mock('vscode', () => ({ + commands: { + registerCommand, + executeCommand, + }, + window: { + showWarningMessage, + showInformationMessage, + }, + workspace: { + workspaceFolders: [], + }, + Uri: { + joinPath: vi.fn(), + }, +})); + +function getRegisteredHandler(commandId: string) { + const call = registerCommand.mock.calls.find(([id]) => id === commandId); + if (!call) { + throw new Error(`Command ${commandId} was not registered`); + } + return call[1] as (...args: unknown[]) => Promise; +} + +describe('registerNewCommands', () => { + const context = { subscriptions: [] as Array<{ dispose: () => void }> }; + const diffManager = { showDiff: vi.fn() }; + const log = vi.fn(); + + beforeEach(() => { + context.subscriptions = []; + registerCommand.mockClear(); + executeCommand.mockClear(); + showWarningMessage.mockClear(); + showInformationMessage.mockClear(); + }); + + it('openNewChatTab opens a new provider without creating a second session explicitly', async () => { + const provider = { + show: vi.fn().mockResolvedValue(undefined), + createNewSession: vi.fn().mockResolvedValue(undefined), + }; + + registerNewCommands( + context as never, + log, + diffManager as never, + () => [], + () => provider as never, + ); + + await getRegisteredHandler(openNewChatTabCommand)(); + + expect(provider.show).toHaveBeenCalledTimes(1); + expect(provider.createNewSession).not.toHaveBeenCalled(); + }); + + it('focusChat focuses the secondary sidebar when it is supported', async () => { + registerNewCommands( + context as never, + log, + diffManager as never, + () => [], + vi.fn() as never, + undefined, + true, + ); + + await getRegisteredHandler(focusChatCommand)(); + + expect(executeCommand).toHaveBeenCalledWith( + 'qwen-code.chatView.secondary.focus', + ); + }); + + it('focusChat falls back to the primary sidebar when secondary sidebar is unavailable', async () => { + registerNewCommands( + context as never, + log, + diffManager as never, + () => [], + vi.fn() as never, + undefined, + false, + ); + + await getRegisteredHandler(focusChatCommand)(); + + expect(executeCommand).toHaveBeenCalledWith( + 'qwen-code.chatView.sidebar.focus', + ); + }); +}); diff --git a/packages/vscode-ide-companion/src/commands/index.ts b/packages/vscode-ide-companion/src/commands/index.ts index 5f487c6fb..b296c43bd 100644 --- a/packages/vscode-ide-companion/src/commands/index.ts +++ b/packages/vscode-ide-companion/src/commands/index.ts @@ -6,7 +6,12 @@ import * as vscode from 'vscode'; import type { DiffManager } from '../diff-manager.js'; -import type { WebViewProvider } from '../webview/WebViewProvider.js'; +import type { WebViewProvider } from '../webview/providers/WebViewProvider.js'; +import { getErrorMessage } from '../utils/errorMessage.js'; +import { + CHAT_VIEW_ID_SIDEBAR, + CHAT_VIEW_ID_SECONDARY, +} from '../constants/viewIds.js'; type Logger = (message: string) => void; @@ -15,16 +20,36 @@ export const showDiffCommand = 'qwenCode.showDiff'; export const openChatCommand = 'qwen-code.openChat'; export const openNewChatTabCommand = 'qwenCode.openNewChatTab'; export const loginCommand = 'qwen-code.login'; +export const focusChatCommand = 'qwen-code.focusChat'; +export const newConversationCommand = 'qwen-code.newConversation'; +export const showLogsCommand = 'qwen-code.showLogs'; +/** + * Register all Qwen Code chat-related commands. + * + * `openChat` and `newConversation` always open an editor tab, while + * `focusChat` focuses the secondary sidebar (preferred) or primary sidebar. + * + * @param context - VS Code extension context for subscription management + * @param log - Logger function for debug output + * @param diffManager - Diff manager for showing file diffs + * @param getWebViewProviders - Returns all active editor-tab WebView providers + * @param createWebViewProvider - Factory to create a new editor-tab WebView provider + * @param outputChannel - Optional output channel for the showLogs command + * @param supportsSecondarySidebar - Whether the running VS Code supports secondary sidebar + */ export function registerNewCommands( context: vscode.ExtensionContext, log: Logger, diffManager: DiffManager, getWebViewProviders: () => WebViewProvider[], createWebViewProvider: () => WebViewProvider, + outputChannel?: vscode.OutputChannel, + supportsSecondarySidebar = true, ): void { const disposables: vscode.Disposable[] = []; + // Open Chat: show the most recent editor tab or create a new one disposables.push( vscode.commands.registerCommand(openChatCommand, async () => { const providers = getWebViewProviders(); @@ -55,17 +80,18 @@ export function registerNewCommands( log(`[Command] Showing diff for ${absolutePath}`); await diffManager.showDiff(absolutePath, args.oldText, args.newText); } catch (error) { - log(`[Command] Error showing diff: ${error}`); - vscode.window.showErrorMessage(`Failed to show diff: ${error}`); + const errorMsg = getErrorMessage(error); + log(`[Command] Error showing diff: ${errorMsg}`); + vscode.window.showErrorMessage(`Failed to show diff: ${errorMsg}`); } }, ), ); + // Open New Chat Tab: always create a new editor tab disposables.push( vscode.commands.registerCommand(openNewChatTabCommand, async () => { const provider = createWebViewProvider(); - // Session restoration is now disabled by default, so no need to suppress it await provider.show(); }), ); @@ -82,5 +108,39 @@ export function registerNewCommands( } }), ); + + // Focus Chat: bring the active chat view to front. + // Use secondary sidebar when supported; fall back to primary sidebar. + disposables.push( + vscode.commands.registerCommand(focusChatCommand, async () => { + if (supportsSecondarySidebar) { + await vscode.commands.executeCommand(`${CHAT_VIEW_ID_SECONDARY}.focus`); + } else { + await vscode.commands.executeCommand(`${CHAT_VIEW_ID_SIDEBAR}.focus`); + } + }), + ); + + // New Conversation: open a new editor tab for a fresh conversation + disposables.push( + vscode.commands.registerCommand(newConversationCommand, async () => { + const provider = createWebViewProvider(); + await provider.show(); + }), + ); + + // Show Logs: reveal the output channel + disposables.push( + vscode.commands.registerCommand(showLogsCommand, async () => { + if (outputChannel) { + outputChannel.show(true); + } else { + vscode.window.showWarningMessage( + 'Qwen Code Companion log channel is not available.', + ); + } + }), + ); + context.subscriptions.push(...disposables); } diff --git a/packages/vscode-ide-companion/src/constants/viewIds.ts b/packages/vscode-ide-companion/src/constants/viewIds.ts new file mode 100644 index 000000000..b54c6eaa1 --- /dev/null +++ b/packages/vscode-ide-companion/src/constants/viewIds.ts @@ -0,0 +1,17 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * WebviewView IDs for the chat UI host positions. + * These IDs must match the `views` contributions declared in package.json. + * + * Only one of sidebar / secondary is visible at runtime — controlled by the + * `qwen-code:doesNotSupportSecondarySidebar` context key in package.json. + * The secondary sidebar is preferred; the primary sidebar is a fallback for + * VS Code versions that lack secondary sidebar support. + */ +export const CHAT_VIEW_ID_SIDEBAR = 'qwen-code.chatView.sidebar'; +export const CHAT_VIEW_ID_SECONDARY = 'qwen-code.chatView.secondary'; diff --git a/packages/vscode-ide-companion/src/extension.test.ts b/packages/vscode-ide-companion/src/extension.test.ts index ef0d5ad46..72c3d476e 100644 --- a/packages/vscode-ide-companion/src/extension.test.ts +++ b/packages/vscode-ide-companion/src/extension.test.ts @@ -23,6 +23,7 @@ vi.mock('@qwen-code/qwen-code-core/src/ide/detect-ide.js', async () => { }); vi.mock('vscode', () => ({ + version: '1.94.0', window: { createOutputChannel: vi.fn(() => ({ appendLine: vi.fn(), @@ -43,6 +44,9 @@ vi.mock('vscode', () => ({ registerWebviewPanelSerializer: vi.fn(() => ({ dispose: vi.fn(), })), + registerWebviewViewProvider: vi.fn(() => ({ + dispose: vi.fn(), + })), }, workspace: { workspaceFolders: [], @@ -134,6 +138,22 @@ describe('activate', () => { expect(vscode.workspace.onDidGrantWorkspaceTrust).toHaveBeenCalled(); }); + it('should register webview view providers for sidebar and secondary positions', async () => { + await activate(context); + + // Verify registerWebviewViewProvider was called 2 times (sidebar + secondary) + const registerCalls = vi.mocked(vscode.window.registerWebviewViewProvider) + .mock.calls; + expect(registerCalls).toHaveLength(2); + + // Extract view IDs from the calls + const viewIds = registerCalls.map((call) => call[0]); + + // Only sidebar and secondary are registered; panel view was removed + expect(viewIds).toContain('qwen-code.chatView.sidebar'); + expect(viewIds).toContain('qwen-code.chatView.secondary'); + }); + it('should launch the Qwen Code when the user clicks the button', async () => { const showInformationMessageMock = vi .mocked(vscode.window.showInformationMessage) diff --git a/packages/vscode-ide-companion/src/extension.ts b/packages/vscode-ide-companion/src/extension.ts index 14ff4bcae..54b494024 100644 --- a/packages/vscode-ide-companion/src/extension.ts +++ b/packages/vscode-ide-companion/src/extension.ts @@ -14,7 +14,9 @@ import { IDE_DEFINITIONS, type IdeInfo, } from '@qwen-code/qwen-code-core/src/ide/detect-ide.js'; -import { WebViewProvider } from './webview/WebViewProvider.js'; +import { WebViewProvider } from './webview/providers/WebViewProvider.js'; +import { ChatProviderRegistry } from './webview/providers/ChatProviderRegistry.js'; +import { registerChatViewProviders } from './webview/providers/chatViewRegistration.js'; import { registerNewCommands } from './commands/index.js'; import { ReadonlyFileSystemProvider } from './services/readonlyFileSystemProvider.js'; import { isWindows } from './utils/platform.js'; @@ -35,7 +37,7 @@ const HIDE_INSTALLATION_GREETING_IDES: ReadonlySet = new Set([ let ideServer: IDEServer; let logger: vscode.OutputChannel; -let webViewProviders: WebViewProvider[] = []; // Track multiple chat tabs +let chatProviderRegistry: ChatProviderRegistry | null = null; let log: (message: string) => void = () => {}; @@ -125,17 +127,25 @@ export async function activate(context: vscode.ExtensionContext) { ); log('Readonly file system provider registered'); + chatProviderRegistry = new ChatProviderRegistry( + () => new WebViewProvider(context, context.extensionUri), + ); + const diffContentProvider = new DiffContentProvider(); const diffManager = new DiffManager( log, diffContentProvider, - // Delay when any chat tab has a pending permission drawer - () => webViewProviders.some((p) => p.hasPendingPermission()), - // Suppress diffs when active mode is auto or yolo in any chat tab + // Delay when any chat surface has a pending permission drawer + () => + chatProviderRegistry + ?.getPermissionAwareProviders() + .some((p) => p.hasPendingPermission()) ?? false, + // Suppress diffs when active mode is auto or yolo in any chat surface () => { - const providers = webViewProviders.filter( - (p) => typeof p.shouldSuppressDiff === 'function', - ); + const providers = + chatProviderRegistry + ?.getPermissionAwareProviders() + .filter((p) => typeof p.shouldSuppressDiff === 'function') ?? []; if (providers.length === 0) { return false; } @@ -144,11 +154,16 @@ export async function activate(context: vscode.ExtensionContext) { ); // Helper function to create a new WebView provider instance - const createWebViewProvider = (): WebViewProvider => { - const provider = new WebViewProvider(context, context.extensionUri); - webViewProviders.push(provider); - return provider; - }; + const createWebViewProvider = (): WebViewProvider => + chatProviderRegistry!.createEditorProvider(); + + const createViewProvider = (): WebViewProvider => + chatProviderRegistry!.createViewProvider(); + + const supportsSecondarySidebar = registerChatViewProviders({ + context, + createViewProvider, + }); // Register WebView panel serializer for persistence across reloads context.subscriptions.push( @@ -192,8 +207,10 @@ export async function activate(context: vscode.ExtensionContext) { context, log, diffManager, - () => webViewProviders, + () => chatProviderRegistry?.getEditorProviders() ?? [], createWebViewProvider, + logger, + supportsSecondarySidebar, ); context.subscriptions.push( @@ -211,9 +228,10 @@ export async function activate(context: vscode.ExtensionContext) { if (docUri && docUri.scheme === DIFF_SCHEME) { diffManager.acceptDiff(docUri); } - // If WebView is requesting permission, actively select an allow option (prefer once) + // If any chat surface is requesting permission, actively select allow (prefer once) try { - for (const provider of webViewProviders) { + for (const provider of chatProviderRegistry?.getPermissionAwareProviders() ?? + []) { if (provider?.hasPendingPermission()) { provider.respondToPendingPermission('allow'); } @@ -228,9 +246,10 @@ export async function activate(context: vscode.ExtensionContext) { if (docUri && docUri.scheme === DIFF_SCHEME) { diffManager.cancelDiff(docUri); } - // If WebView is requesting permission, actively select reject/cancel + // If any chat surface is requesting permission, actively select reject/cancel try { - for (const provider of webViewProviders) { + for (const provider of chatProviderRegistry?.getPermissionAwareProviders() ?? + []) { if (provider?.hasPendingPermission()) { provider.respondToPendingPermission('cancel'); } @@ -364,11 +383,8 @@ export async function deactivate(): Promise { if (ideServer) { await ideServer.stop(); } - // Dispose all WebView providers - webViewProviders.forEach((provider) => { - provider.dispose(); - }); - webViewProviders = []; + chatProviderRegistry?.disposeAll(); + chatProviderRegistry = null; } catch (err) { const message = err instanceof Error ? err.message : String(err); log(`Failed to stop IDE server during deactivation: ${message}`); diff --git a/packages/vscode-ide-companion/src/package.test.ts b/packages/vscode-ide-companion/src/package.test.ts new file mode 100644 index 000000000..9d7cdaef5 --- /dev/null +++ b/packages/vscode-ide-companion/src/package.test.ts @@ -0,0 +1,27 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +describe('package.json command metadata', () => { + it('describes focusChat as focusing the chat view', () => { + const manifest = JSON.parse( + readFileSync(resolve(import.meta.dirname, '../package.json'), 'utf8'), + ) as { + contributes: { + commands: Array<{ command: string; title: string }>; + }; + }; + + const command = manifest.contributes.commands.find( + (item) => item.command === 'qwen-code.focusChat', + ); + + expect(command?.title).toBe('Qwen Code: Focus Chat View'); + }); +}); diff --git a/packages/vscode-ide-companion/src/services/acpConnection.test.ts b/packages/vscode-ide-companion/src/services/acpConnection.test.ts new file mode 100644 index 000000000..32977171a --- /dev/null +++ b/packages/vscode-ide-companion/src/services/acpConnection.test.ts @@ -0,0 +1,46 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it, vi } from 'vitest'; +import { RequestError } from '@agentclientprotocol/sdk'; + +// AcpConnection imports AcpFileHandler which imports vscode. +// Mock vscode so it can be resolved without the actual VS Code runtime. +vi.mock('vscode', () => ({})); + +import { AcpConnection } from './acpConnection.js'; +import { ACP_ERROR_CODES } from '../constants/acpSchema.js'; + +describe('AcpConnection readTextFile error mapping', () => { + it('maps ENOENT to RESOURCE_NOT_FOUND RequestError', () => { + const conn = new AcpConnection() as unknown as { + mapReadTextFileError: (error: unknown, filePath: string) => unknown; + }; + const enoent = Object.assign(new Error('missing file'), { code: 'ENOENT' }); + + expect(() => + conn.mapReadTextFileError(enoent, '/tmp/missing.txt'), + ).toThrowError( + expect.objectContaining({ + code: ACP_ERROR_CODES.RESOURCE_NOT_FOUND, + }), + ); + }); + + it('keeps non-ENOENT RequestError unchanged', () => { + const conn = new AcpConnection() as unknown as { + mapReadTextFileError: (error: unknown, filePath: string) => unknown; + }; + const requestError = new RequestError( + ACP_ERROR_CODES.INTERNAL_ERROR, + 'Internal error', + ); + + expect(conn.mapReadTextFileError(requestError, '/tmp/file.txt')).toBe( + requestError, + ); + }); +}); diff --git a/packages/vscode-ide-companion/src/services/acpConnection.ts b/packages/vscode-ide-companion/src/services/acpConnection.ts index 8c4994d14..5d263b618 100644 --- a/packages/vscode-ide-companion/src/services/acpConnection.ts +++ b/packages/vscode-ide-companion/src/services/acpConnection.ts @@ -8,6 +8,7 @@ import { ClientSideConnection, ndJsonStream, PROTOCOL_VERSION, + RequestError, } from '@agentclientprotocol/sdk'; import type { Client, @@ -37,6 +38,7 @@ import { spawn } from 'child_process'; import { Readable, Writable } from 'node:stream'; import * as fs from 'node:fs'; import { AcpFileHandler } from './acpFileHandler.js'; +import { ACP_ERROR_CODES } from '../constants/acpSchema.js'; /** * ACP Connection Handler for VSCode Extension @@ -163,7 +165,7 @@ export class AcpConnection { const stream = ndJsonStream(stdin, stdout); - // Build the SDK Client implementation that bridges to our callbacks + // Build the SDK Client implementation that bridges to our callbacks. // eslint-disable-next-line @typescript-eslint/no-this-alias const self = this; this.sdkConnection = new ClientSideConnection( @@ -266,13 +268,17 @@ export class AcpConnection { async readTextFile( params: ReadTextFileRequest, ): Promise { - const result = await self.fileHandler.handleReadTextFile({ - path: params.path, - sessionId: params.sessionId, - line: params.line ?? null, - limit: params.limit ?? null, - }); - return { content: result.content }; + try { + const result = await self.fileHandler.handleReadTextFile({ + path: params.path, + sessionId: params.sessionId, + line: params.line ?? null, + limit: params.limit ?? null, + }); + return { content: result.content }; + } catch (error) { + throw self.mapReadTextFileError(error, params.path); + } }, async writeTextFile( @@ -334,6 +340,22 @@ export class AcpConnection { return this.sdkConnection; } + private mapReadTextFileError(error: unknown, filePath: string): unknown { + const errorCode = + typeof error === 'object' && error !== null && 'code' in error + ? (error as { code?: unknown }).code + : undefined; + + if (errorCode === 'ENOENT') { + throw new RequestError( + ACP_ERROR_CODES.RESOURCE_NOT_FOUND, + `File not found: ${filePath}`, + ); + } + + return error; + } + private resolvePermissionOptionId( request: RequestPermissionRequest, preferredOptionId?: string, diff --git a/packages/vscode-ide-companion/src/services/acpFileHandler.test.ts b/packages/vscode-ide-companion/src/services/acpFileHandler.test.ts index fa87c9ab0..e5c68da8e 100644 --- a/packages/vscode-ide-companion/src/services/acpFileHandler.test.ts +++ b/packages/vscode-ide-companion/src/services/acpFileHandler.test.ts @@ -5,28 +5,91 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { AcpFileHandler } from './acpFileHandler.js'; -import { promises as fs } from 'fs'; -vi.mock('fs', () => ({ - promises: { - readFile: vi.fn(), - writeFile: vi.fn(), - mkdir: vi.fn(), +// Use vi.hoisted so the mocks are accessible inside the vi.mock factory +// (vi.mock calls are hoisted to the top of the file by Vitest). +const { + mockGetText, + mockPositionAt, + mockSave, + mockApplyEdit, + mockOpenTextDocument, + mockCreateDirectory, + mockStatFile, + mockWriteFile, +} = vi.hoisted(() => { + const mockGetText = vi.fn(); + const mockPositionAt = vi.fn((offset: number) => ({ offset })); + const mockSave = vi.fn().mockResolvedValue(true); + const mockApplyEdit = vi.fn().mockResolvedValue(true); + const mockOpenTextDocument = vi.fn().mockResolvedValue({ + getText: mockGetText, + positionAt: mockPositionAt, + isDirty: false, + save: mockSave, + }); + const mockCreateDirectory = vi.fn().mockResolvedValue(undefined); + const mockStatFile = vi.fn(); + const mockWriteFile = vi.fn().mockResolvedValue(undefined); + return { + mockGetText, + mockPositionAt, + mockSave, + mockApplyEdit, + mockOpenTextDocument, + mockCreateDirectory, + mockStatFile, + mockWriteFile, + }; +}); + +vi.mock('vscode', () => ({ + Uri: { + file: (p: string) => ({ fsPath: p, toString: () => p }), + }, + workspace: { + openTextDocument: mockOpenTextDocument, + applyEdit: mockApplyEdit, + fs: { + createDirectory: mockCreateDirectory, + stat: mockStatFile, + writeFile: mockWriteFile, + }, + }, + WorkspaceEdit: class { + replace = vi.fn(); + }, + Range: class { + constructor( + public start: unknown, + public end: unknown, + ) {} }, })); +import { AcpFileHandler } from './acpFileHandler.js'; + describe('AcpFileHandler', () => { let handler: AcpFileHandler; beforeEach(() => { handler = new AcpFileHandler(); vi.clearAllMocks(); + // Restore default implementations after clearAllMocks + mockOpenTextDocument.mockResolvedValue({ + getText: mockGetText, + positionAt: mockPositionAt, + isDirty: false, + save: mockSave, + }); + mockApplyEdit.mockResolvedValue(true); + mockCreateDirectory.mockResolvedValue(undefined); + mockWriteFile.mockResolvedValue(undefined); }); describe('handleReadTextFile', () => { it('returns full content when no line/limit specified', async () => { - vi.mocked(fs.readFile).mockResolvedValue('line1\nline2\nline3\n'); + mockGetText.mockReturnValue('line1\nline2\nline3\n'); const result = await handler.handleReadTextFile({ path: '/test/file.txt', @@ -39,9 +102,7 @@ describe('AcpFileHandler', () => { }); it('uses 1-based line indexing (ACP spec)', async () => { - vi.mocked(fs.readFile).mockResolvedValue( - 'line1\nline2\nline3\nline4\nline5', - ); + mockGetText.mockReturnValue('line1\nline2\nline3\nline4\nline5'); const result = await handler.handleReadTextFile({ path: '/test/file.txt', @@ -54,7 +115,7 @@ describe('AcpFileHandler', () => { }); it('treats line=1 as first line', async () => { - vi.mocked(fs.readFile).mockResolvedValue('first\nsecond\nthird'); + mockGetText.mockReturnValue('first\nsecond\nthird'); const result = await handler.handleReadTextFile({ path: '/test/file.txt', @@ -67,7 +128,7 @@ describe('AcpFileHandler', () => { }); it('defaults to line=1 when line is null but limit is set', async () => { - vi.mocked(fs.readFile).mockResolvedValue('a\nb\nc\nd'); + mockGetText.mockReturnValue('a\nb\nc\nd'); const result = await handler.handleReadTextFile({ path: '/test/file.txt', @@ -80,7 +141,7 @@ describe('AcpFileHandler', () => { }); it('clamps negative line values to 0', async () => { - vi.mocked(fs.readFile).mockResolvedValue('a\nb\nc'); + mockGetText.mockReturnValue('a\nb\nc'); const result = await handler.handleReadTextFile({ path: '/test/file.txt', @@ -95,7 +156,7 @@ describe('AcpFileHandler', () => { it('propagates ENOENT errors', async () => { const err = new Error('ENOENT') as NodeJS.ErrnoException; err.code = 'ENOENT'; - vi.mocked(fs.readFile).mockRejectedValue(err); + mockOpenTextDocument.mockRejectedValue(err); await expect( handler.handleReadTextFile({ @@ -106,12 +167,30 @@ describe('AcpFileHandler', () => { }), ).rejects.toThrow('ENOENT'); }); + + it('normalises VS Code FileNotFound to ENOENT', async () => { + // vscode.FileSystemError.FileNotFound sets code = 'FileNotFound' + const err = new Error('file not found') as NodeJS.ErrnoException; + (err as unknown as Record).code = 'FileNotFound'; + mockOpenTextDocument.mockRejectedValue(err); + + const rejection = handler.handleReadTextFile({ + path: '/missing/file.txt', + sessionId: 'sid', + line: null, + limit: null, + }); + + await expect(rejection).rejects.toThrow('ENOENT'); + await expect(rejection).rejects.toMatchObject({ code: 'ENOENT' }); + }); }); describe('handleWriteTextFile', () => { - it('creates directories and writes file', async () => { - vi.mocked(fs.mkdir).mockResolvedValue(undefined); - vi.mocked(fs.writeFile).mockResolvedValue(undefined); + it('creates directory and uses WorkspaceEdit for existing file', async () => { + // stat resolves → file exists + mockStatFile.mockResolvedValue({}); + mockGetText.mockReturnValue('old content'); const result = await handler.handleWriteTextFile({ path: '/test/dir/file.txt', @@ -120,11 +199,25 @@ describe('AcpFileHandler', () => { }); expect(result).toBeNull(); - expect(fs.mkdir).toHaveBeenCalledWith('/test/dir', { recursive: true }); - expect(fs.writeFile).toHaveBeenCalledWith( - '/test/dir/file.txt', - 'hello', - 'utf-8', + expect(mockCreateDirectory).toHaveBeenCalled(); + expect(mockApplyEdit).toHaveBeenCalled(); + }); + + it('writes bytes directly for new (non-existing) file', async () => { + // stat rejects → file does not exist + mockStatFile.mockRejectedValue(new Error('FileNotFound')); + + const result = await handler.handleWriteTextFile({ + path: '/test/dir/newfile.txt', + content: 'hello', + sessionId: 'sid', + }); + + expect(result).toBeNull(); + expect(mockCreateDirectory).toHaveBeenCalled(); + expect(mockWriteFile).toHaveBeenCalledWith( + expect.objectContaining({ fsPath: '/test/dir/newfile.txt' }), + expect.any(Uint8Array), ); }); }); diff --git a/packages/vscode-ide-companion/src/services/acpFileHandler.ts b/packages/vscode-ide-companion/src/services/acpFileHandler.ts index e41240788..3bf526823 100644 --- a/packages/vscode-ide-companion/src/services/acpFileHandler.ts +++ b/packages/vscode-ide-companion/src/services/acpFileHandler.ts @@ -10,8 +10,9 @@ * Responsible for handling file read and write operations in the ACP protocol */ -import { promises as fs } from 'fs'; import * as path from 'path'; +import * as vscode from 'vscode'; +import { getErrorMessage } from '../utils/errorMessage.js'; /** * ACP File Operation Handler Class @@ -43,9 +44,14 @@ export class AcpFileHandler { }); try { - const content = await fs.readFile(params.path, 'utf-8'); + const uri = vscode.Uri.file(params.path); + // openTextDocument handles encoding detection (BOM, files.encoding setting, + // chardet) and returns properly decoded Unicode text regardless of the + // source encoding (UTF-8, GBK, Shift-JIS, etc.). + const document = await vscode.workspace.openTextDocument(uri); + const content = document.getText(); console.log( - `[ACP] Successfully read file: ${params.path} (${content.length} bytes)`, + `[ACP] Successfully read file: ${params.path} (${content.length} chars)`, ); // Handle line offset and limit. @@ -64,12 +70,25 @@ export class AcpFileHandler { console.log(`[ACP] Returning full file content`); return result; } catch (error) { - const errorMsg = error instanceof Error ? error.message : String(error); + const errorMsg = getErrorMessage(error); console.error(`[ACP] Failed to read file ${params.path}:`, errorMsg); - const nodeError = error as NodeJS.ErrnoException; - if (nodeError?.code === 'ENOENT') { - throw error; + // Detect "file not found" from both Node.js (code === 'ENOENT') and + // VS Code's FileSystemError.FileNotFound (code === 'FileNotFound'). + const errorCode = + typeof error === 'object' && error !== null && 'code' in error + ? (error as { code?: unknown }).code + : undefined; + + if (errorCode === 'ENOENT' || errorCode === 'FileNotFound') { + // Normalise to a Node-style ENOENT so downstream ACP layers + // (mapReadTextFileError → AcpFileSystemService) can recognise it. + const enoent = new Error( + `ENOENT: no such file or directory, open '${params.path}'`, + ) as NodeJS.ErrnoException; + enoent.code = 'ENOENT'; + enoent.path = params.path; + throw enoent; } throw new Error(`Failed to read file '${params.path}': ${errorMsg}`); @@ -97,18 +116,56 @@ export class AcpFileHandler { console.log(`[ACP] Content size: ${params.content.length} bytes`); try { - // Ensure directory exists - const dirName = path.dirname(params.path); - console.log(`[ACP] Ensuring directory exists: ${dirName}`); - await fs.mkdir(dirName, { recursive: true }); + const uri = vscode.Uri.file(params.path); - // Write file - await fs.writeFile(params.path, params.content, 'utf-8'); + // Ensure the parent directory exists. + const dirUri = vscode.Uri.file(path.dirname(params.path)); + console.log(`[ACP] Ensuring directory exists: ${dirUri.fsPath}`); + await vscode.workspace.fs.createDirectory(dirUri); + + // Determine whether the file already exists so we can choose the right + // write strategy. + let fileExists = false; + try { + await vscode.workspace.fs.stat(uri); + fileExists = true; + } catch { + fileExists = false; + } + + if (fileExists) { + // Open the document so VS Code tracks its original encoding, replace + // all content via WorkspaceEdit, then save. VS Code writes back using + // the same encoding it detected on open (e.g. GBK), preserving the + // original encoding without any manual codec work. + const document = await vscode.workspace.openTextDocument(uri); + const edit = new vscode.WorkspaceEdit(); + const fullRange = new vscode.Range( + document.positionAt(0), + document.positionAt(document.getText().length), + ); + edit.replace(uri, fullRange, params.content); + const applied = await vscode.workspace.applyEdit(edit); + if (!applied) { + throw new Error('WorkspaceEdit was not applied'); + } + const updatedDoc = await vscode.workspace.openTextDocument(uri); + if (updatedDoc.isDirty) { + const saved = await updatedDoc.save(); + if (!saved) { + throw new Error(`File could not be saved: ${params.path}`); + } + } + } else { + // New file – write UTF-8 bytes directly. + const bytes = Buffer.from(params.content, 'utf-8'); + await vscode.workspace.fs.writeFile(uri, bytes); + } console.log(`[ACP] Successfully wrote file: ${params.path}`); return null; } catch (error) { - const errorMsg = error instanceof Error ? error.message : String(error); + const errorMsg = getErrorMessage(error); console.error(`[ACP] Failed to write file ${params.path}:`, errorMsg); throw new Error(`Failed to write file '${params.path}': ${errorMsg}`); diff --git a/packages/vscode-ide-companion/src/services/qwenAgentManager.test.ts b/packages/vscode-ide-companion/src/services/qwenAgentManager.test.ts new file mode 100644 index 000000000..440dc2b18 --- /dev/null +++ b/packages/vscode-ide-companion/src/services/qwenAgentManager.test.ts @@ -0,0 +1,56 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it, vi } from 'vitest'; +import { extractSessionListItems } from './qwenAgentManager.js'; + +vi.mock('vscode', () => ({ + window: { + showInformationMessage: vi.fn(), + showWarningMessage: vi.fn(), + showErrorMessage: vi.fn(), + }, +})); + +describe('extractSessionListItems', () => { + it('returns sessions array from the "sessions" field', () => { + const items = extractSessionListItems({ + sessions: [{ sessionId: 'session-1' }], + }); + expect(items).toEqual([{ sessionId: 'session-1' }]); + }); + + it('returns items array from the legacy "items" field', () => { + const items = extractSessionListItems({ + items: [{ sessionId: 'session-2' }], + }); + expect(items).toEqual([{ sessionId: 'session-2' }]); + }); + + it('prefers "sessions" over "items" when both are present', () => { + const items = extractSessionListItems({ + sessions: [{ sessionId: 'from-sessions' }], + items: [{ sessionId: 'from-items' }], + }); + expect(items).toEqual([{ sessionId: 'from-sessions' }]); + }); + + it('returns empty array for null/undefined input', () => { + expect(extractSessionListItems(null)).toEqual([]); + expect(extractSessionListItems(undefined)).toEqual([]); + }); + + it('returns empty array for non-object input', () => { + expect(extractSessionListItems('string')).toEqual([]); + expect(extractSessionListItems(42)).toEqual([]); + }); + + it('returns empty array when neither field is an array', () => { + expect(extractSessionListItems({ sessions: 'not-array' })).toEqual([]); + expect(extractSessionListItems({ items: 123 })).toEqual([]); + expect(extractSessionListItems({})).toEqual([]); + }); +}); diff --git a/packages/vscode-ide-companion/src/services/qwenAgentManager.ts b/packages/vscode-ide-companion/src/services/qwenAgentManager.ts index 4fb044a73..c5a0920d7 100644 --- a/packages/vscode-ide-companion/src/services/qwenAgentManager.ts +++ b/packages/vscode-ide-companion/src/services/qwenAgentManager.ts @@ -36,10 +36,41 @@ import { extractSessionModelState, } from '../utils/acpModelInfo.js'; import { isAuthenticationRequiredError } from '../utils/authErrors.js'; +import { getErrorMessage } from '../utils/errorMessage.js'; import { handleAuthenticateUpdate } from '../utils/authNotificationHandler.js'; export type { ChatMessage, PlanEntry, ToolCallUpdateData }; +/** + * Extract session list items from ACP response. + * Handles both 'sessions' (new) and 'items' (legacy) response shapes. + * @param response - The ACP session/list response + * @returns Array of session items, or empty array if invalid + */ +export function extractSessionListItems( + response: unknown, +): Array> { + if (!response || typeof response !== 'object') { + return []; + } + + const payload = response as { + sessions?: unknown; + items?: unknown; + }; + + // Prefer 'sessions' field, fall back to 'items' for backwards compatibility + if (Array.isArray(payload.sessions)) { + return payload.sessions as Array>; + } + + if (Array.isArray(payload.items)) { + return payload.items as Array>; + } + + return []; +} + /** * Qwen Agent Manager * @@ -413,14 +444,7 @@ export class QwenAgentManager { console.log('[QwenAgentManager] ACP session list response:', response); const res: unknown = response; - let items: Array> = []; - - if (res && typeof res === 'object' && 'sessions' in res) { - const sessionsValue = (res as { sessions?: unknown }).sessions; - items = Array.isArray(sessionsValue) - ? (sessionsValue as Array>) - : []; - } + const items = extractSessionListItems(res); console.log( '[QwenAgentManager] Sessions retrieved via ACP:', @@ -514,14 +538,7 @@ export class QwenAgentManager { ...(cursor !== undefined ? { cursor } : {}), }); const res: unknown = response; - let items: Array> = []; - - if (res && typeof res === 'object' && 'sessions' in res) { - const sessionsValue = (res as { sessions?: unknown }).sessions; - items = Array.isArray(sessionsValue) - ? (sessionsValue as Array>) - : []; - } + const items = extractSessionListItems(res); const mapped = items.map((item) => ({ id: item.sessionId || item.id, @@ -997,8 +1014,7 @@ export class QwenAgentManager { return response; } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); + const errorMessage = getErrorMessage(error); console.error( '[QwenAgentManager] Session load via ACP failed for session:', sessionId, diff --git a/packages/vscode-ide-companion/src/services/qwenConnectionHandler.ts b/packages/vscode-ide-companion/src/services/qwenConnectionHandler.ts index 5e33b548d..60c0b3ac5 100644 --- a/packages/vscode-ide-companion/src/services/qwenConnectionHandler.ts +++ b/packages/vscode-ide-companion/src/services/qwenConnectionHandler.ts @@ -18,6 +18,7 @@ import { extractSessionModeState, extractSessionModelState, } from '../utils/acpModelInfo.js'; +import { getErrorMessage } from '../utils/errorMessage.js'; import type { ModelInfo } from '@agentclientprotocol/sdk'; import type { ApprovalModeValue } from '../types/approvalModeValueTypes.js'; @@ -167,6 +168,8 @@ export class QwenConnectionHandler { authMethod: string, autoAuthenticate: boolean, ): Promise { + let lastError: unknown; + for (let attempt = 1; attempt <= maxRetries; attempt++) { try { console.log( @@ -176,8 +179,8 @@ export class QwenConnectionHandler { console.log('[QwenAgentManager] Session created successfully'); return res; } catch (error) { - const errorMessage = - error instanceof Error ? error.message : String(error); + lastError = error; + const errorMessage = getErrorMessage(error); console.error( `[QwenAgentManager] Session creation attempt ${attempt} failed:`, errorMessage, @@ -221,9 +224,7 @@ export class QwenConnectionHandler { } if (attempt === maxRetries) { - throw new Error( - `Session creation failed after ${maxRetries} attempts: ${errorMessage}`, - ); + throw error; } const delay = Math.min(1000 * Math.pow(2, attempt - 1), 5000); @@ -232,6 +233,10 @@ export class QwenConnectionHandler { } } + if (lastError !== undefined) { + throw lastError; + } + throw new Error('Session creation failed unexpectedly'); } } diff --git a/packages/vscode-ide-companion/src/utils/authErrors.ts b/packages/vscode-ide-companion/src/utils/authErrors.ts index 85d652045..2c4258689 100644 --- a/packages/vscode-ide-companion/src/utils/authErrors.ts +++ b/packages/vscode-ide-companion/src/utils/authErrors.ts @@ -6,13 +6,56 @@ import { ACP_ERROR_CODES } from '../constants/acpSchema.js'; -const AUTH_ERROR_PATTERNS = [ - 'Authentication required', // Standard authentication request message - `(code: ${ACP_ERROR_CODES.AUTH_REQUIRED})`, // RPC error code indicates auth failure - 'Unauthorized', // HTTP unauthorized error - 'Invalid token', // Invalid token - 'Session expired', // Session expired -]; +const CODE_PATTERN = /\(\s*code:\s*(-?\d+)\s*\)/i; + +const toNumericCode = (value: unknown): number | null => { + if (typeof value === 'number' && Number.isFinite(value)) { + return value; + } + if (typeof value === 'string') { + const trimmed = value.trim(); + if (/^-?\d+$/.test(trimmed)) { + return Number.parseInt(trimmed, 10); + } + } + return null; +}; + +const extractCodeFromUnknown = (value: unknown): number | null => { + if (!value) { + return null; + } + + const directCode = toNumericCode(value); + if (directCode !== null) { + return directCode; + } + + if (typeof value === 'string') { + const match = value.match(CODE_PATTERN); + return match?.[1] ? Number.parseInt(match[1], 10) : null; + } + + if (typeof value === 'object') { + const record = value as Record; + const topLevelCode = toNumericCode(record['code']); + if (topLevelCode !== null) { + return topLevelCode; + } + + const nestedCode = extractCodeFromUnknown(record['error']); + if (nestedCode !== null) { + return nestedCode; + } + + const messageCode = extractCodeFromUnknown(record['message']); + if (messageCode !== null) { + return messageCode; + } + } + + return null; +}; /** * Determines if the given error is authentication-related @@ -23,14 +66,6 @@ export const isAuthenticationRequiredError = (error: unknown): boolean => { return false; } - // Extract error message text - const message = - error instanceof Error - ? error.message - : typeof error === 'string' - ? error - : String(error); - - // Match authentication-related errors using predefined patterns - return AUTH_ERROR_PATTERNS.some((pattern) => message.includes(pattern)); + const code = extractCodeFromUnknown(error); + return code === ACP_ERROR_CODES.AUTH_REQUIRED; }; diff --git a/packages/vscode-ide-companion/src/utils/editorGroupUtils.ts b/packages/vscode-ide-companion/src/utils/editorGroupUtils.ts index e3b837778..e855b2dec 100644 --- a/packages/vscode-ide-companion/src/utils/editorGroupUtils.ts +++ b/packages/vscode-ide-companion/src/utils/editorGroupUtils.ts @@ -10,6 +10,7 @@ import { openChatCommand } from '../commands/index.js'; /** * Find the editor group immediately to the left of the Qwen chat webview. * - If the chat webview group is the leftmost group, returns undefined. + * - If no chat webview is found in any editor group, returns undefined. * - Uses the webview tab viewType 'mainThreadWebview-qwenCode.chat'. */ export function findLeftGroupOfChatWebview(): vscode.ViewColumn | undefined { diff --git a/packages/vscode-ide-companion/src/utils/errorMessage.test.ts b/packages/vscode-ide-companion/src/utils/errorMessage.test.ts new file mode 100644 index 000000000..55de1cd0b --- /dev/null +++ b/packages/vscode-ide-companion/src/utils/errorMessage.test.ts @@ -0,0 +1,34 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { getErrorMessage } from './errorMessage.js'; + +describe('getErrorMessage', () => { + it('extracts detailed message from top-level data.details string', () => { + expect( + getErrorMessage({ + data: { + details: 'Detailed error from backend', + }, + }), + ).toBe('Detailed error from backend'); + }); + + it('extracts detailed message from nested error.data.details.message', () => { + expect( + getErrorMessage({ + error: { + data: { + details: { + message: 'Nested detailed error message', + }, + }, + }, + }), + ).toBe('Nested detailed error message'); + }); +}); diff --git a/packages/vscode-ide-companion/src/utils/errorMessage.ts b/packages/vscode-ide-companion/src/utils/errorMessage.ts new file mode 100644 index 000000000..8cd7301b0 --- /dev/null +++ b/packages/vscode-ide-companion/src/utils/errorMessage.ts @@ -0,0 +1,97 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +export function getErrorMessage( + error: unknown, + fallback = 'Unknown error', +): string { + const combineMessageAndDetails = ( + message: string | null, + detailsMessage: string | null, + ): string | null => { + if (message && detailsMessage) { + return message === detailsMessage + ? message + : `${message}: ${detailsMessage}`; + } + return message ?? detailsMessage; + }; + + const extractDetailsMessage = (value: unknown): string | null => { + if (typeof value === 'string' && value) { + return value; + } + + if (typeof value !== 'object' || value === null) { + return null; + } + + const record = value as Record; + const details = record['details']; + if (typeof details === 'string' && details) { + return details; + } + if (typeof details === 'object' && details !== null) { + const detailsRecord = details as Record; + if ( + typeof detailsRecord['message'] === 'string' && + detailsRecord['message'] + ) { + return detailsRecord['message']; + } + try { + return JSON.stringify(details); + } catch { + return null; + } + } + return null; + }; + + if (error instanceof Error && error.message) { + return error.message; + } + if (typeof error === 'string' && error) { + return error; + } + if (typeof error === 'object' && error !== null) { + const record = error as Record; + const topLevelMessage = + typeof record['message'] === 'string' && record['message'] + ? record['message'] + : null; + const topLevelDetailsMessage = extractDetailsMessage(record['data']); + const combinedTopLevelMessage = combineMessageAndDetails( + topLevelMessage, + topLevelDetailsMessage, + ); + if (combinedTopLevelMessage) { + return combinedTopLevelMessage; + } + const nested = record['error']; + if (typeof nested === 'object' && nested !== null) { + const nestedRecord = nested as Record; + const nestedMessage = + typeof nestedRecord['message'] === 'string' && nestedRecord['message'] + ? nestedRecord['message'] + : null; + const nestedDetailsMessage = extractDetailsMessage(nestedRecord['data']); + const combinedNestedMessage = combineMessageAndDetails( + nestedMessage, + nestedDetailsMessage, + ); + if (combinedNestedMessage) { + return combinedNestedMessage; + } + } + try { + return JSON.stringify(error); + } catch { + return fallback; + } + } + return fallback; +} diff --git a/packages/vscode-ide-companion/src/webview/handlers/AuthMessageHandler.ts b/packages/vscode-ide-companion/src/webview/handlers/AuthMessageHandler.ts index ab4b70b2e..0b703da46 100644 --- a/packages/vscode-ide-companion/src/webview/handlers/AuthMessageHandler.ts +++ b/packages/vscode-ide-companion/src/webview/handlers/AuthMessageHandler.ts @@ -6,6 +6,7 @@ import * as vscode from 'vscode'; import { BaseMessageHandler } from './BaseMessageHandler.js'; +import { getErrorMessage } from '../../utils/errorMessage.js'; /** * Auth message handler @@ -67,6 +68,7 @@ export class AuthMessageHandler extends BaseMessageHandler { await vscode.commands.executeCommand('qwen-code.login'); } } catch (error) { + const errorMsg = getErrorMessage(error); console.error('[AuthMessageHandler] Login failed:', error); console.error( '[AuthMessageHandler] Error stack:', @@ -75,7 +77,7 @@ export class AuthMessageHandler extends BaseMessageHandler { this.sendToWebView({ type: 'loginError', data: { - message: `Login failed: ${error instanceof Error ? error.message : String(error)}`, + message: `Login failed: ${errorMsg}`, }, }); } diff --git a/packages/vscode-ide-companion/src/webview/handlers/EditorMessageHandler.ts b/packages/vscode-ide-companion/src/webview/handlers/EditorMessageHandler.ts index 7d82315dc..bb49cc540 100644 --- a/packages/vscode-ide-companion/src/webview/handlers/EditorMessageHandler.ts +++ b/packages/vscode-ide-companion/src/webview/handlers/EditorMessageHandler.ts @@ -7,6 +7,7 @@ import * as vscode from 'vscode'; import { BaseMessageHandler } from './BaseMessageHandler.js'; import { getFileName } from '../utils/webviewUtils.js'; +import { getErrorMessage } from '../../utils/errorMessage.js'; /** * Editor message handler @@ -105,7 +106,9 @@ export class EditorMessageHandler extends BaseMessageHandler { '[EditorMessageHandler] Failed to focus active editor:', error, ); - vscode.window.showErrorMessage(`Failed to focus editor: ${error}`); + vscode.window.showErrorMessage( + `Failed to focus editor: ${getErrorMessage(error)}`, + ); } } } diff --git a/packages/vscode-ide-companion/src/webview/handlers/FileMessageHandler.ts b/packages/vscode-ide-companion/src/webview/handlers/FileMessageHandler.ts index 908de9ca4..4e6e43575 100644 --- a/packages/vscode-ide-companion/src/webview/handlers/FileMessageHandler.ts +++ b/packages/vscode-ide-companion/src/webview/handlers/FileMessageHandler.ts @@ -14,6 +14,7 @@ import { } from '../../utils/editorGroupUtils.js'; import { ReadonlyFileSystemProvider } from '../../services/readonlyFileSystemProvider.js'; import { FileDiscoveryService } from '@qwen-code/qwen-code-core/src/services/fileDiscoveryService.js'; +import { getErrorMessage } from '../../utils/errorMessage.js'; /** * File message handler @@ -118,9 +119,10 @@ export class FileMessageHandler extends BaseMessageHandler { } } catch (error) { console.error('[FileMessageHandler] Failed to attach file:', error); + const errorMsg = getErrorMessage(error); this.sendToWebView({ type: 'error', - data: { message: `Failed to attach file: ${error}` }, + data: { message: `Failed to attach file: ${errorMsg}` }, }); } } @@ -203,9 +205,10 @@ export class FileMessageHandler extends BaseMessageHandler { '[FileMessageHandler] Failed to show context picker:', error, ); + const errorMsg = getErrorMessage(error); this.sendToWebView({ type: 'error', - data: { message: `Failed to show context picker: ${error}` }, + data: { message: `Failed to show context picker: ${errorMsg}` }, }); } } @@ -360,9 +363,10 @@ export class FileMessageHandler extends BaseMessageHandler { '[FileMessageHandler] Failed to get workspace files:', error, ); + const errorMsg = getErrorMessage(error); this.sendToWebView({ type: 'error', - data: { message: `Failed to get workspace files: ${error}` }, + data: { message: `Failed to get workspace files: ${errorMsg}` }, }); } } @@ -422,7 +426,9 @@ export class FileMessageHandler extends BaseMessageHandler { console.log('[FileOperations] File opened successfully:', absolutePath); } catch (error) { console.error('[FileMessageHandler] Failed to open file:', error); - vscode.window.showErrorMessage(`Failed to open file: ${error}`); + vscode.window.showErrorMessage( + `Failed to open file: ${getErrorMessage(error)}`, + ); } } @@ -445,7 +451,9 @@ export class FileMessageHandler extends BaseMessageHandler { }); } catch (error) { console.error('[FileMessageHandler] Failed to open diff:', error); - vscode.window.showErrorMessage(`Failed to open diff: ${error}`); + vscode.window.showErrorMessage( + `Failed to open diff: ${getErrorMessage(error)}`, + ); } } @@ -544,7 +552,7 @@ export class FileMessageHandler extends BaseMessageHandler { error, ); vscode.window.showErrorMessage( - `Failed to create and open temporary file: ${error}`, + `Failed to create and open temporary file: ${getErrorMessage(error)}`, ); } } diff --git a/packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.ts b/packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.ts index 868838a1d..4afac9273 100644 --- a/packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.ts +++ b/packages/vscode-ide-companion/src/webview/handlers/SessionMessageHandler.ts @@ -8,9 +8,8 @@ import * as vscode from 'vscode'; import { BaseMessageHandler } from './BaseMessageHandler.js'; import type { ChatMessage } from '../../services/qwenAgentManager.js'; import type { ApprovalModeValue } from '../../types/approvalModeValueTypes.js'; -import { ACP_ERROR_CODES } from '../../constants/acpSchema.js'; - -const AUTH_REQUIRED_CODE_PATTERN = `(code: ${ACP_ERROR_CODES.AUTH_REQUIRED})`; +import { isAuthenticationRequiredError } from '../../utils/authErrors.js'; +import { getErrorMessage } from '../../utils/errorMessage.js'; /** * Session message handler @@ -101,9 +100,10 @@ export class SessionMessageHandler extends BaseMessageHandler { '[SessionMessageHandler] Failed to open new chat tab:', error, ); + const errorMsg = this.getErrorMessage(error); this.sendToWebView({ type: 'error', - data: { message: `Failed to open new chat tab: ${error}` }, + data: { message: `Failed to open new chat tab: ${errorMsg}` }, }); } break; @@ -221,6 +221,14 @@ export class SessionMessageHandler extends BaseMessageHandler { return 'dismiss'; } + private getErrorMessage(error: unknown): string { + return getErrorMessage(error); + } + + private shouldPromptLogin(error: unknown): boolean { + return isAuthenticationRequiredError(error); + } + /** * Handle send message request */ @@ -279,7 +287,7 @@ export class SessionMessageHandler extends BaseMessageHandler { data: newConv, }); } catch (error) { - const errorMsg = `Failed to create conversation: ${error}`; + const errorMsg = `Failed to create conversation: ${this.getErrorMessage(error)}`; console.error('[SessionMessageHandler]', errorMsg); vscode.window.showErrorMessage(errorMsg); this.sendToWebView({ @@ -367,12 +375,8 @@ export class SessionMessageHandler extends BaseMessageHandler { '[SessionMessageHandler] Failed to create session before sending message:', createErr, ); - const errorMsg = - createErr instanceof Error ? createErr.message : String(createErr); - if ( - errorMsg.includes('Authentication required') || - errorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) - ) { + const errorMsg = this.getErrorMessage(createErr); + if (this.shouldPromptLogin(createErr)) { await this.promptLogin( 'Your login session has expired or is invalid. Please login again to continue using Qwen Code.', ); @@ -413,7 +417,7 @@ export class SessionMessageHandler extends BaseMessageHandler { const err = error as unknown as Error; // Safely convert error to string - const errorMsg = error ? String(error) : 'Unknown error'; + const errorMsg = this.getErrorMessage(error); const lower = errorMsg.toLowerCase(); // Suppress user-cancelled/aborted errors (ESC/Stop button) @@ -435,11 +439,7 @@ export class SessionMessageHandler extends BaseMessageHandler { // Check for session not found error and handle it appropriately if ( errorMsg.includes('Session not found') || - errorMsg.includes('No active ACP session') || - errorMsg.includes('Authentication required') || - errorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) || - errorMsg.includes('Unauthorized') || - errorMsg.includes('Invalid token') + this.shouldPromptLogin(error) ) { // Show a more user-friendly error message for expired sessions await this.promptLogin( @@ -477,7 +477,7 @@ export class SessionMessageHandler extends BaseMessageHandler { this.sendStreamEnd('timeout'); } else { // Handling of Non-Timeout Errors - vscode.window.showErrorMessage(`Error sending message: ${error}`); + vscode.window.showErrorMessage(`Error sending message: ${errorMsg}`); this.sendToWebView({ type: 'error', data: { message: errorMsg }, @@ -524,15 +524,9 @@ export class SessionMessageHandler extends BaseMessageHandler { ); // Safely convert error to string - const errorMsg = error ? String(error) : 'Unknown error'; + const errorMsg = this.getErrorMessage(error); // Check for authentication/session expiration errors - if ( - errorMsg.includes('Authentication required') || - errorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) || - errorMsg.includes('Unauthorized') || - errorMsg.includes('Invalid token') || - errorMsg.includes('No active ACP session') - ) { + if (this.shouldPromptLogin(error)) { // Show a more user-friendly error message for expired sessions await this.promptLogin( 'Your login session has expired or is invalid. Please login again to create a new session.', @@ -546,7 +540,7 @@ export class SessionMessageHandler extends BaseMessageHandler { } else { this.sendToWebView({ type: 'error', - data: { message: `Failed to create new session: ${error}` }, + data: { message: `Failed to create new session: ${errorMsg}` }, }); } } @@ -632,17 +626,8 @@ export class SessionMessageHandler extends BaseMessageHandler { loadError, ); - // Safely convert error to string - const errorMsg = loadError ? String(loadError) : 'Unknown error'; - // Check for authentication/session expiration errors - if ( - errorMsg.includes('Authentication required') || - errorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) || - errorMsg.includes('Unauthorized') || - errorMsg.includes('Invalid token') || - errorMsg.includes('No active ACP session') - ) { + if (this.shouldPromptLogin(loadError)) { // Show a more user-friendly error message for expired sessions await this.promptLogin( 'Your login session has expired or is invalid. Please login again to switch sessions.', @@ -691,18 +676,8 @@ export class SessionMessageHandler extends BaseMessageHandler { createError, ); - // Safely convert error to string - const createErrorMsg = createError - ? String(createError) - : 'Unknown error'; // Check for authentication/session expiration errors in session creation - if ( - createErrorMsg.includes('Authentication required') || - createErrorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) || - createErrorMsg.includes('Unauthorized') || - createErrorMsg.includes('Invalid token') || - createErrorMsg.includes('No active ACP session') - ) { + if (this.shouldPromptLogin(createError)) { // Show a more user-friendly error message for expired sessions await this.promptLogin( 'Your login session has expired or is invalid. Please login again to switch sessions.', @@ -734,15 +709,9 @@ export class SessionMessageHandler extends BaseMessageHandler { console.error('[SessionMessageHandler] Failed to switch session:', error); // Safely convert error to string - const errorMsg = error ? String(error) : 'Unknown error'; + const errorMsg = this.getErrorMessage(error); // Check for authentication/session expiration errors - if ( - errorMsg.includes('Authentication required') || - errorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) || - errorMsg.includes('Unauthorized') || - errorMsg.includes('Invalid token') || - errorMsg.includes('No active ACP session') - ) { + if (this.shouldPromptLogin(error)) { // Show a more user-friendly error message for expired sessions await this.promptLogin( 'Your login session has expired or is invalid. Please login again to switch sessions.', @@ -756,7 +725,7 @@ export class SessionMessageHandler extends BaseMessageHandler { } else { this.sendToWebView({ type: 'error', - data: { message: `Failed to switch session: ${error}` }, + data: { message: `Failed to switch session: ${errorMsg}` }, }); } } @@ -789,15 +758,9 @@ export class SessionMessageHandler extends BaseMessageHandler { console.error('[SessionMessageHandler] Failed to get sessions:', error); // Safely convert error to string - const errorMsg = error ? String(error) : 'Unknown error'; + const errorMsg = this.getErrorMessage(error); // Check for authentication/session expiration errors - if ( - errorMsg.includes('Authentication required') || - errorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) || - errorMsg.includes('Unauthorized') || - errorMsg.includes('Invalid token') || - errorMsg.includes('No active ACP session') - ) { + if (this.shouldPromptLogin(error)) { // Show a more user-friendly error message for expired sessions await this.promptLogin( 'Your login session has expired or is invalid. Please login again to view sessions.', @@ -811,7 +774,7 @@ export class SessionMessageHandler extends BaseMessageHandler { } else { this.sendToWebView({ type: 'error', - data: { message: `Failed to get sessions: ${error}` }, + data: { message: `Failed to get sessions: ${errorMsg}` }, }); } } @@ -891,16 +854,8 @@ export class SessionMessageHandler extends BaseMessageHandler { await this.handleGetQwenSessions(); return; } catch (acpError) { - // Safely convert error to string - const errorMsg = acpError ? String(acpError) : 'Unknown error'; // Check for authentication/session expiration errors - if ( - errorMsg.includes('Authentication required') || - errorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) || - errorMsg.includes('Unauthorized') || - errorMsg.includes('Invalid token') || - errorMsg.includes('No active ACP session') - ) { + if (this.shouldPromptLogin(acpError)) { // Show a more user-friendly error message for expired sessions await this.promptLogin( 'Your login session has expired or is invalid. Please login again to resume sessions.', @@ -920,15 +875,9 @@ export class SessionMessageHandler extends BaseMessageHandler { console.error('[SessionMessageHandler] Failed to resume session:', error); // Safely convert error to string - const errorMsg = error ? String(error) : 'Unknown error'; + const errorMsg = this.getErrorMessage(error); // Check for authentication/session expiration errors - if ( - errorMsg.includes('Authentication required') || - errorMsg.includes(AUTH_REQUIRED_CODE_PATTERN) || - errorMsg.includes('Unauthorized') || - errorMsg.includes('Invalid token') || - errorMsg.includes('No active ACP session') - ) { + if (this.shouldPromptLogin(error)) { // Show a more user-friendly error message for expired sessions await this.promptLogin( 'Your login session has expired or is invalid. Please login again to resume sessions.', @@ -942,7 +891,7 @@ export class SessionMessageHandler extends BaseMessageHandler { } else { this.sendToWebView({ type: 'error', - data: { message: `Failed to resume session: ${error}` }, + data: { message: `Failed to resume session: ${errorMsg}` }, }); } } @@ -960,9 +909,10 @@ export class SessionMessageHandler extends BaseMessageHandler { // No explicit response needed; WebView listens for modeChanged } catch (error) { console.error('[SessionMessageHandler] Failed to set mode:', error); + const errorMsg = this.getErrorMessage(error); this.sendToWebView({ type: 'error', - data: { message: `Failed to set mode: ${error}` }, + data: { message: `Failed to set mode: ${errorMsg}` }, }); } } @@ -982,7 +932,7 @@ export class SessionMessageHandler extends BaseMessageHandler { `Model switched to: ${modelId}`, ); } catch (error) { - const errorMsg = error instanceof Error ? error.message : String(error); + const errorMsg = this.getErrorMessage(error); console.error('[SessionMessageHandler] Failed to set model:', error); vscode.window.showErrorMessage(`Failed to switch model: ${errorMsg}`); this.sendToWebView({ diff --git a/packages/vscode-ide-companion/src/webview/hooks/message/useMessageHandling.ts b/packages/vscode-ide-companion/src/webview/hooks/message/useMessageHandling.ts index 507da7e2a..24c3ce561 100644 --- a/packages/vscode-ide-companion/src/webview/hooks/message/useMessageHandling.ts +++ b/packages/vscode-ide-companion/src/webview/hooks/message/useMessageHandling.ts @@ -161,7 +161,20 @@ export const useMessageHandling = () => { if (idx === null) { idx = next.length; thinkingMessageIndexRef.current = idx; - next.push({ role: 'thinking', content: '', timestamp: Date.now() }); + // Use a timestamp just before the assistant placeholder so thinking + // sorts above the response text when messages are ordered by time. + const assistantIdx = streamingMessageIndexRef.current; + const assistantTs = + assistantIdx !== null && + assistantIdx >= 0 && + assistantIdx < next.length + ? next[assistantIdx].timestamp + : Date.now(); + next.push({ + role: 'thinking', + content: '', + timestamp: assistantTs - 1, + }); } if (idx >= 0 && idx < next.length) { const target = next[idx]; diff --git a/packages/vscode-ide-companion/src/webview/providers/ChatProviderRegistry.test.ts b/packages/vscode-ide-companion/src/webview/providers/ChatProviderRegistry.test.ts new file mode 100644 index 000000000..3820538c8 --- /dev/null +++ b/packages/vscode-ide-companion/src/webview/providers/ChatProviderRegistry.test.ts @@ -0,0 +1,48 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it, vi } from 'vitest'; +import { ChatProviderRegistry } from './ChatProviderRegistry.js'; + +describe('ChatProviderRegistry', () => { + it('tracks editor and view providers separately while exposing a combined list', () => { + const factory = vi + .fn() + .mockReturnValueOnce({ dispose: vi.fn(), kind: 'editor-1' }) + .mockReturnValueOnce({ dispose: vi.fn(), kind: 'view-1' }) + .mockReturnValueOnce({ dispose: vi.fn(), kind: 'editor-2' }); + + const registry = new ChatProviderRegistry(factory); + + const editor1 = registry.createEditorProvider(); + const view1 = registry.createViewProvider(); + const editor2 = registry.createEditorProvider(); + + expect(factory).toHaveBeenCalledTimes(3); + expect(registry.getEditorProviders()).toEqual([editor1, editor2]); + expect(registry.getPermissionAwareProviders()).toEqual([ + editor1, + editor2, + view1, + ]); + }); + + it('disposes all tracked providers and resets internal collections', () => { + const editorDispose = vi.fn(); + const viewDispose = vi.fn(); + const registry = new ChatProviderRegistry(() => ({ dispose: vi.fn() })); + + registry.createEditorProvider({ dispose: editorDispose }); + registry.createViewProvider({ dispose: viewDispose }); + + registry.disposeAll(); + + expect(editorDispose).toHaveBeenCalledTimes(1); + expect(viewDispose).toHaveBeenCalledTimes(1); + expect(registry.getEditorProviders()).toEqual([]); + expect(registry.getPermissionAwareProviders()).toEqual([]); + }); +}); diff --git a/packages/vscode-ide-companion/src/webview/providers/ChatProviderRegistry.ts b/packages/vscode-ide-companion/src/webview/providers/ChatProviderRegistry.ts new file mode 100644 index 000000000..94cacf47d --- /dev/null +++ b/packages/vscode-ide-companion/src/webview/providers/ChatProviderRegistry.ts @@ -0,0 +1,46 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +type DisposableProvider = { + dispose(): void; +}; + +/** + * Tracks chat providers by host type while exposing a combined list for flows + * like permission handling and diff suppression. + */ +export class ChatProviderRegistry { + private editorProviders: T[] = []; + private viewProviders: T[] = []; + + constructor(private readonly createProvider: () => T) {} + + createEditorProvider(provider: T = this.createProvider()): T { + this.editorProviders.push(provider); + return provider; + } + + createViewProvider(provider: T = this.createProvider()): T { + this.viewProviders.push(provider); + return provider; + } + + getEditorProviders(): T[] { + return [...this.editorProviders]; + } + + getPermissionAwareProviders(): T[] { + return [...this.editorProviders, ...this.viewProviders]; + } + + disposeAll(): void { + for (const provider of this.getPermissionAwareProviders()) { + provider.dispose(); + } + this.editorProviders = []; + this.viewProviders = []; + } +} diff --git a/packages/vscode-ide-companion/src/webview/providers/ChatWebviewViewProvider.test.ts b/packages/vscode-ide-companion/src/webview/providers/ChatWebviewViewProvider.test.ts new file mode 100644 index 000000000..a25860eb8 --- /dev/null +++ b/packages/vscode-ide-companion/src/webview/providers/ChatWebviewViewProvider.test.ts @@ -0,0 +1,54 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it, vi } from 'vitest'; +import { ChatWebviewViewProvider } from './ChatWebviewViewProvider.js'; + +vi.mock('vscode', () => ({})); + +describe('ChatWebviewViewProvider', () => { + it('lazily creates the WebViewProvider on first resolveWebviewView call', async () => { + const mockProvider = { + attachToView: vi.fn().mockResolvedValue(undefined), + }; + const factory = vi.fn(() => mockProvider); + + const viewProvider = new ChatWebviewViewProvider(factory as never); + + const mockWebviewView = { + webview: {}, + viewType: 'qwen-code.chatView.sidebar', + }; + + await viewProvider.resolveWebviewView(mockWebviewView as never); + + expect(factory).toHaveBeenCalledTimes(1); + expect(mockProvider.attachToView).toHaveBeenCalledWith( + mockWebviewView, + 'qwen-code.chatView.sidebar', + ); + }); + + it('reuses the same WebViewProvider on subsequent calls', async () => { + const mockProvider = { + attachToView: vi.fn().mockResolvedValue(undefined), + }; + const factory = vi.fn(() => mockProvider); + + const viewProvider = new ChatWebviewViewProvider(factory as never); + + const mockView1 = { webview: {}, viewType: 'sidebar' }; + const mockView2 = { webview: {}, viewType: 'sidebar' }; + + await viewProvider.resolveWebviewView(mockView1 as never); + await viewProvider.resolveWebviewView(mockView2 as never); + + // Factory should only be called once (lazy creation) + expect(factory).toHaveBeenCalledTimes(1); + // But attachToView should be called for each resolve + expect(mockProvider.attachToView).toHaveBeenCalledTimes(2); + }); +}); diff --git a/packages/vscode-ide-companion/src/webview/providers/ChatWebviewViewProvider.ts b/packages/vscode-ide-companion/src/webview/providers/ChatWebviewViewProvider.ts new file mode 100644 index 000000000..ffce1152a --- /dev/null +++ b/packages/vscode-ide-companion/src/webview/providers/ChatWebviewViewProvider.ts @@ -0,0 +1,47 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import type * as vscode from 'vscode'; +import type { WebViewProvider } from './WebViewProvider.js'; + +/** + * Factory function type that lazily creates a WebViewProvider instance. + * The provider is only instantiated when VS Code actually opens the view. + */ +export type WebViewProviderFactory = () => WebViewProvider; + +/** + * WebviewView host for placing the chat UI in sidebar / panel / secondary sidebar. + * + * Accepts a factory function instead of a pre-built WebViewProvider so the + * heavyweight provider (QwenAgentManager, ConversationStore, etc.) is only + * created when VS Code actually opens the view, not at extension startup. + */ +export class ChatWebviewViewProvider implements vscode.WebviewViewProvider { + private webViewProvider: WebViewProvider | null = null; + + /** + * @param createWebViewProvider - Factory that creates a WebViewProvider on demand + */ + constructor(private readonly createWebViewProvider: WebViewProviderFactory) {} + + /** + * Called by VS Code when the webview view becomes visible for the first time. + * Creates the WebViewProvider lazily and attaches the webview. + * + * @param webviewView - The webview view created by VS Code + */ + async resolveWebviewView(webviewView: vscode.WebviewView): Promise { + // Lazily create the provider on first resolve + if (!this.webViewProvider) { + this.webViewProvider = this.createWebViewProvider(); + } + + // Webview options (enableScripts, localResourceRoots) are configured + // inside WebViewProvider.attachToView — no duplication needed here. + await this.webViewProvider.attachToView(webviewView, webviewView.viewType); + } +} diff --git a/packages/vscode-ide-companion/src/webview/MessageHandler.ts b/packages/vscode-ide-companion/src/webview/providers/MessageHandler.ts similarity index 87% rename from packages/vscode-ide-companion/src/webview/MessageHandler.ts rename to packages/vscode-ide-companion/src/webview/providers/MessageHandler.ts index b89c8fd86..a06fd1a3b 100644 --- a/packages/vscode-ide-companion/src/webview/MessageHandler.ts +++ b/packages/vscode-ide-companion/src/webview/providers/MessageHandler.ts @@ -4,13 +4,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { QwenAgentManager } from '../services/qwenAgentManager.js'; -import type { ConversationStore } from '../services/conversationStore.js'; +import type { QwenAgentManager } from '../../services/qwenAgentManager.js'; +import type { ConversationStore } from '../../services/conversationStore.js'; import type { PermissionResponseMessage, AskUserQuestionResponseMessage, -} from '../types/webviewMessageTypes.js'; -import { MessageRouter } from './handlers/MessageRouter.js'; +} from '../../types/webviewMessageTypes.js'; +import { MessageRouter } from '../handlers/MessageRouter.js'; /** * MessageHandler (Refactored Version) diff --git a/packages/vscode-ide-companion/src/webview/PanelManager.ts b/packages/vscode-ide-companion/src/webview/providers/PanelManager.ts similarity index 96% rename from packages/vscode-ide-companion/src/webview/PanelManager.ts rename to packages/vscode-ide-companion/src/webview/providers/PanelManager.ts index 44f1a6ecc..0c02dc3ca 100644 --- a/packages/vscode-ide-companion/src/webview/PanelManager.ts +++ b/packages/vscode-ide-companion/src/webview/providers/PanelManager.ts @@ -224,8 +224,18 @@ export class PanelManager { return; } + // Capture a reference to the current panel so the deferred callback can + // detect if the panel was disposed or replaced before it runs. + const scheduledPanel = this.panel; + // Defer slightly so the tab model is updated after create/reveal setTimeout(() => { + // The panel may have been disposed/replaced before this callback runs. + if (!this.panel || this.panel !== scheduledPanel) { + return; + } + + const panelTitle = this.panel.title; const allTabs = vscode.window.tabGroups.all.flatMap((g) => g.tabs); const match = allTabs.find((t) => { // Type guard for webview tab input @@ -234,7 +244,7 @@ export class PanelManager { !!inp && typeof inp === 'object' && 'viewType' in inp; const isWebview = isWebviewInput(input); const sameViewType = isWebview && input.viewType === 'qwenCode.chat'; - const sameLabel = t.label === this.panel!.title; + const sameLabel = t.label === panelTitle; return !!(sameViewType || sameLabel); }); this.panelTab = match ?? null; diff --git a/packages/vscode-ide-companion/src/webview/providers/WebViewContent.test.ts b/packages/vscode-ide-companion/src/webview/providers/WebViewContent.test.ts new file mode 100644 index 000000000..3c2029fe5 --- /dev/null +++ b/packages/vscode-ide-companion/src/webview/providers/WebViewContent.test.ts @@ -0,0 +1,77 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it, vi } from 'vitest'; +import { WebViewContent } from './WebViewContent.js'; + +vi.mock('vscode', () => ({ + Uri: { + joinPath: vi.fn((_base: unknown, ...parts: string[]) => ({ + fsPath: `/ext/${parts.join('/')}`, + })), + }, +})); + +/** + * Helper: create a minimal mock vscode.Webview + */ +function createMockWebview() { + return { + asWebviewUri: vi.fn((uri: { fsPath: string }) => ({ + toString: () => `https://webview/${uri.fsPath}`, + })), + cspSource: 'https://csp.source', + }; +} + +describe('WebViewContent', () => { + const fakeExtensionUri = { fsPath: '/ext' } as never; + + it('generates HTML when given a raw Webview', () => { + const webview = createMockWebview(); + const html = WebViewContent.generate(webview as never, fakeExtensionUri); + + expect(html).toContain(''); + expect(html).toContain('Qwen Code'); + expect(html).toContain(webview.cspSource); + expect(webview.asWebviewUri).toHaveBeenCalled(); + }); + + it('generates HTML when given a WebviewPanel (has .webview property)', () => { + const webview = createMockWebview(); + const panel = { webview }; + + const html = WebViewContent.generate(panel as never, fakeExtensionUri); + + expect(html).toContain(''); + expect(webview.asWebviewUri).toHaveBeenCalled(); + }); + + it('generates HTML when given a WebviewView (has .webview property)', () => { + const webview = createMockWebview(); + const view = { webview, viewType: 'sidebar' }; + + const html = WebViewContent.generate(view as never, fakeExtensionUri); + + expect(html).toContain(''); + expect(webview.asWebviewUri).toHaveBeenCalled(); + }); + + it('includes the script tag with the correct URI', () => { + const webview = createMockWebview(); + const html = WebViewContent.generate(webview as never, fakeExtensionUri); + + expect(html).toContain('