diff --git a/package.json b/package.json index 72d9016af2..faa3d0d0c6 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "build:sandbox": "node scripts/build_sandbox.js", "bundle": "npm run generate && node esbuild.config.js && node scripts/copy_bundle_assets.js", "test": "cross-env NODE_OPTIONS=\"--max-old-space-size=3072\" npm run test --workspaces --if-present --parallel", - "test:ci": "cross-env NODE_OPTIONS=\"--max-old-space-size=3072\" npm run test:ci --workspaces --if-present --parallel && npm run test:scripts", + "test:ci": "cross-env NODE_OPTIONS=\"--max-old-space-size=3072\" npm run test:ci --workspaces --if-present --parallel && npm run test:scripts && npm run check:serve-fast-path-bundle", "test:scripts": "vitest run --config ./scripts/tests/vitest.config.ts", "test:e2e": "cross-env VERBOSE=true KEEP_OUTPUT=true npm run test:integration:sandbox:none", "test:integration:all": "npm run test:integration:sandbox:none && npm run test:integration:sandbox:docker && npm run test:integration:sandbox:podman", diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 355c3843db..6b0d739bf9 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -294,6 +294,7 @@ vi.mock('@qwen-code/qwen-code-core', () => ({ getAutoMemoryRoot: vi.fn( (projectRoot: string) => `${projectRoot}/.qwen/memory`, ), + getUserAutoMemoryRoot: vi.fn(() => '/tmp/user-memory'), QwenOAuth2Event: {}, qwenOAuth2Events: { on: vi.fn(), off: vi.fn() }, MCPDiscoveryState: { @@ -347,6 +348,8 @@ vi.mock('@qwen-code/qwen-code-core', () => ({ SessionService: vi.fn(), Storage: { getGlobalQwenDir: vi.fn(() => '/tmp/qwen-global-test'), + getGlobalTempDir: vi.fn(() => '/tmp/qwen-global-temp'), + getUserExtensionsDir: vi.fn(() => '/tmp/qwen-extensions'), }, parseRule: vi.fn((raw: string) => { const trimmed = raw.trim(); @@ -597,6 +600,7 @@ import { } from '../config/permission-settings.js'; import { loadCliConfig } from '../config/config.js'; import { createLoadedSettingsAdapter } from '../config/loadedSettingsAdapter.js'; +import { AcpFileSystemService } from './service/filesystem.js'; import { Session, buildAvailableCommandsSnapshot } from './session/Session.js'; import { SERVE_STATUS_EXT_METHODS, @@ -1215,6 +1219,78 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); + it('configures ACP file system fallback roots for read_file allowed local roots', async () => { + const fsCapabilities = { readTextFile: true, writeTextFile: true }; + const fallbackFileSystem = {}; + const innerConfig = { + ...makeInnerConfig(), + getTargetDir: vi.fn().mockReturnValue('/project'), + getSessionId: vi.fn().mockReturnValue('session-with-fs'), + getFileSystemService: vi.fn().mockReturnValue(fallbackFileSystem), + setFileSystemService: vi.fn(), + storage: { + getProjectTempDir: vi.fn().mockReturnValue('/project/.qwen/tmp'), + getProjectDir: vi.fn().mockReturnValue('/project'), + getUserSkillsDirs: vi.fn().mockReturnValue(['/home/test/.qwen/skills']), + }, + }; + vi.mocked(loadSettings).mockReturnValue(makeSessionSettings()); + vi.mocked(loadCliConfig).mockResolvedValue( + innerConfig as unknown as Config, + ); + vi.mocked(Session).mockImplementation( + () => + ({ + getId: vi.fn().mockReturnValue('session-with-fs'), + getConfig: vi.fn().mockReturnValue(innerConfig), + sendAvailableCommandsUpdate: vi.fn().mockResolvedValue(undefined), + replayHistory: vi.fn().mockResolvedValue(undefined), + installRewriter: vi.fn(), + startCronScheduler: vi.fn(), + dispose: vi.fn(), + }) as unknown as InstanceType, + ); + + const agentPromise = runAcpAgent( + mockConfig, + makeSessionSettings(), + mockArgv, + ); + await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined()); + + const fakeConn = { + get closed() { + return mockConnectionState.promise; + }, + } as AgentSideConnectionLike; + const agent = capturedAgentFactory!(fakeConn) as AgentLike; + + await agent.initialize({ clientCapabilities: { fs: fsCapabilities } }); + await agent.newSession({ cwd: '/project', mcpServers: [] }); + + expect(AcpFileSystemService).toHaveBeenCalledWith( + fakeConn, + 'session-with-fs', + fsCapabilities, + fallbackFileSystem, + { + localReadRoots: [ + '/project/.qwen/tmp', + path.join('/project', 'subagents'), + '/tmp/qwen-global-temp', + '/project/.qwen/memory', + '/tmp/user-memory', + '/home/test/.qwen/skills', + '/tmp/qwen-extensions', + ], + }, + ); + expect(innerConfig.setFileSystemService).toHaveBeenCalled(); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('does not return discontinued qwen-oauth as the only ACP auth option', async () => { vi.mocked(buildAuthMethods).mockReturnValue([ { diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 6243ae73cf..606c1cecc9 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -20,6 +20,7 @@ import { findProviderById, getAllGeminiMdFilenames, getAutoMemoryRoot, + getUserAutoMemoryRoot, getDefaultBaseUrlForProtocol, getDefaultModelIds, getScopedEnvContents, @@ -7651,6 +7652,19 @@ class QwenAgent implements Agent { config.getSessionId(), this.clientCapabilities.fs, config.getFileSystemService(), + { + // SYNC: Mirrors ReadFileTool's default allowed local roots, including + // auto-memory roots, so ACP-local read fallback follows the same policy. + localReadRoots: [ + config.storage.getProjectTempDir(), + path.join(config.storage.getProjectDir(), 'subagents'), + Storage.getGlobalTempDir(), + getAutoMemoryRoot(config.getTargetDir()), + getUserAutoMemoryRoot(), + ...config.storage.getUserSkillsDirs(), + Storage.getUserExtensionsDir(), + ], + }, ); config.setFileSystemService(acpFileSystemService); } diff --git a/packages/cli/src/acp-integration/service/filesystem.test.ts b/packages/cli/src/acp-integration/service/filesystem.test.ts index c6b60abae5..d53c512a45 100644 --- a/packages/cli/src/acp-integration/service/filesystem.test.ts +++ b/packages/cli/src/acp-integration/service/filesystem.test.ts @@ -4,13 +4,66 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const mockDebugLogger = vi.hoisted(() => ({ + debug: vi.fn(), + warn: vi.fn(), +})); + +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + createDebugLogger: vi.fn(() => mockDebugLogger), + }; +}); + +vi.mock('node:fs/promises', { spy: true }); + import type { FileSystemService } from '@qwen-code/qwen-code-core'; import { AcpFileSystemService } from './filesystem.js'; import type { AgentSideConnection } from '@agentclientprotocol/sdk'; +import { promises as fs } from 'node:fs'; +import { realpath as fsRealpath } from 'node:fs/promises'; +import os from 'node:os'; +import path from 'node:path'; const RESOURCE_NOT_FOUND_CODE = -32002; const INTERNAL_ERROR_CODE = -32603; +type LocalReadFallbackErrorKind = 'path_outside_workspace' | 'symlink_escape'; + +async function withTempRoot( + callback: (tempRoot: string) => Promise, +): Promise { + const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'acp-local-read-')); + + try { + return await callback(tempRoot); + } finally { + await fs.rm(tempRoot, { recursive: true, force: true }); + } +} + +function createLocalReadFallbackError( + filePath: string, + errorKind: LocalReadFallbackErrorKind = 'path_outside_workspace', +) { + const reason = + errorKind === 'symlink_escape' + ? 'path escapes workspace via symlink' + : 'path escapes workspace'; + + return { + code: INTERNAL_ERROR_CODE, + message: `${reason}: ${filePath}`, + data: { + errorKind, + status: 400, + }, + }; +} const createFallback = (): FileSystemService => ({ readTextFile: vi.fn().mockResolvedValue({ @@ -22,6 +75,12 @@ const createFallback = (): FileSystemService => ({ }); describe('AcpFileSystemService', () => { + beforeEach(() => { + mockDebugLogger.debug.mockClear(); + mockDebugLogger.warn.mockClear(); + vi.mocked(fsRealpath).mockClear(); + }); + describe('readTextFile', () => { it('reads through ACP and returns response', async () => { const mockResponse = { @@ -73,7 +132,7 @@ describe('AcpFileSystemService', () => { }); }); - it('re-throws other errors unchanged', async () => { + it('preserves message for other read errors', async () => { const otherError = { code: INTERNAL_ERROR_CODE, message: 'Internal error', @@ -92,8 +151,784 @@ describe('AcpFileSystemService', () => { await expect( svc.readTextFile({ path: '/some/file.txt' }), ).rejects.toMatchObject({ + message: 'Internal error', + }); + }); + + it('normalizes plain object ACP errors without exposing numeric codes as Node error codes', async () => { + const otherError = { code: INTERNAL_ERROR_CODE, message: 'Internal error', + }; + const client = { + readTextFile: vi.fn().mockRejectedValue(otherError), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-2b', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + const err = await svc + .readTextFile({ path: '/some/file.txt' }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: otherError, + message: 'Internal error', + }); + expect(Object.prototype.hasOwnProperty.call(err, 'code')).toBe(false); + expect(String(err)).toContain('Internal error'); + expect(String(err)).not.toContain('[object Object]'); + }); + + it('passes Error instances through without wrapping them', async () => { + const upstreamError = new Error('upstream failure'); + const client = { + readTextFile: vi.fn().mockRejectedValue(upstreamError), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-2b-error', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + await expect(svc.readTextFile({ path: '/some/file.txt' })).rejects.toBe( + upstreamError, + ); + }); + + it('does not copy unsafe properties from plain object ACP errors', async () => { + const otherError: Record = { + code: 'ABORT_ERR', + message: 'Internal error', + stack: 'Remote ACP stack', + name: 'AbortError', + constructor: 'RemoteConstructor', + toString: 'not callable', + valueOf: 'not callable', + }; + Object.defineProperty(otherError, '__proto__', { + value: { remotePrototype: true }, + enumerable: true, + }); + const client = { + readTextFile: vi.fn().mockRejectedValue(otherError), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-2b-stack', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + const err = await svc + .readTextFile({ path: '/some/file.txt' }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect((err as Error).name).toBe('Error'); + expect((err as Error).stack).toContain('Internal error'); + expect(Object.prototype.hasOwnProperty.call(err, 'code')).toBe(false); + expect(Object.prototype.hasOwnProperty.call(err, 'constructor')).toBe( + false, + ); + expect(Object.prototype.hasOwnProperty.call(err, 'toString')).toBe(false); + expect(Object.prototype.hasOwnProperty.call(err, 'valueOf')).toBe(false); + expect(Object.prototype.hasOwnProperty.call(err, '__proto__')).toBe( + false, + ); + expect( + (err as Record)['remotePrototype'], + ).toBeUndefined(); + expect(String(err)).toContain('Internal error'); + }); + + it('does not copy array entries onto normalized ACP errors', async () => { + const client = { + readTextFile: vi.fn().mockRejectedValue(['Internal error']), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-2b-array', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + const err = await svc + .readTextFile({ path: '/some/file.txt' }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(Object.prototype.hasOwnProperty.call(err, '0')).toBe(false); + }); + + it('includes cause details from plain object ACP errors', async () => { + const otherError = { + code: INTERNAL_ERROR_CODE, + message: 'fetch failed', + cause: { code: 'ECONNREFUSED' }, + }; + const client = { + readTextFile: vi.fn().mockRejectedValue(otherError), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-2b-cause', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + const err = await svc + .readTextFile({ path: '/some/file.txt' }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: otherError, + message: 'fetch failed (cause: ECONNREFUSED)', + }); + }); + + it('falls back to local reads for allowed local roots when ACP rejects them as outside the workspace', async () => { + await withTempRoot(async (tempRoot) => { + const skillRoot = path.join(tempRoot, 'skills'); + const filePath = path.join( + skillRoot, + 'dataworks-di-data-processor', + 'instructions', + 'interaction_norms.md', + ); + await fs.mkdir(path.dirname(filePath), { recursive: true }); + await fs.writeFile(filePath, 'skill instructions', 'utf8'); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + (fallback.readTextFile as ReturnType).mockResolvedValue({ + content: 'skill instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }); + + const svc = new AcpFileSystemService( + client, + 'session-2c', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [skillRoot] }, + ); + + await expect(svc.readTextFile({ path: filePath })).resolves.toEqual({ + content: 'skill instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }); + expect(fallback.readTextFile).toHaveBeenCalledWith({ + path: await fs.realpath(filePath), + }); + }); + }); + + it.skipIf(process.platform === 'win32')( + 'uses the resolved real path for local read fallback', + async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const realFilePath = path.join(localRoot, 'instructions.md'); + const symlinkPath = path.join(localRoot, 'instructions-link.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.writeFile(realFilePath, 'instructions', 'utf8'); + await fs.symlink(realFilePath, symlinkPath); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(symlinkPath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + (fallback.readTextFile as ReturnType).mockResolvedValue( + { + content: 'instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }, + ); + + const svc = new AcpFileSystemService( + client, + 'session-2c-real-fallback-path', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + await expect( + svc.readTextFile({ path: symlinkPath }), + ).resolves.toEqual({ + content: 'instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }); + expect(fallback.readTextFile).toHaveBeenCalledWith({ + path: await fs.realpath(realFilePath), + }); + }); + }, + ); + + it('does not use top-level errorKind fields for local read fallback', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const filePath = path.join(localRoot, 'instructions.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.writeFile(filePath, 'instructions', 'utf8'); + + const topLevelErrorKindError = { + code: INTERNAL_ERROR_CODE, + message: `top-level errorKind only: ${filePath}`, + errorKind: 'path_outside_workspace', + }; + const client = { + readTextFile: vi.fn().mockRejectedValue(topLevelErrorKindError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + + const svc = new AcpFileSystemService( + client, + 'session-2c-top-level-kind', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + const err = await svc + .readTextFile({ path: filePath }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: topLevelErrorKindError, + message: `top-level errorKind only: ${filePath}`, + }); + expect(Object.prototype.hasOwnProperty.call(err, 'code')).toBe(false); + expect(fallback.readTextFile).not.toHaveBeenCalled(); + }); + }); + + it.skipIf(process.platform === 'win32')( + 'does not follow symlink paths that resolve outside configured local roots', + async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'allowed'); + const outsideRoot = path.join(tempRoot, 'outside'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.mkdir(outsideRoot, { recursive: true }); + + const outsideFile = path.join(outsideRoot, 'secret.md'); + const symlinkPath = path.join(localRoot, 'secret.md'); + await fs.writeFile(outsideFile, 'secret', 'utf8'); + await fs.symlink(outsideFile, symlinkPath); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(symlinkPath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + + const svc = new AcpFileSystemService( + client, + 'session-2d', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + const err = await svc + .readTextFile({ path: symlinkPath }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: pathOutsideWorkspaceError, + message: `path escapes workspace: ${symlinkPath}`, + }); + expect(Object.prototype.hasOwnProperty.call(err, 'code')).toBe(false); + expect(fallback.readTextFile).not.toHaveBeenCalled(); + }); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'allows local roots and files that resolve to the same real path tree', + async () => { + await withTempRoot(async (tempRoot) => { + const realRoot = path.join(tempRoot, 'real-root'); + const rootAlias = path.join(tempRoot, 'root-alias'); + const filePath = path.join(realRoot, 'instructions.md'); + await fs.mkdir(realRoot, { recursive: true }); + await fs.writeFile(filePath, 'instructions', 'utf8'); + await fs.symlink(realRoot, rootAlias, 'dir'); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + (fallback.readTextFile as ReturnType).mockResolvedValue( + { + content: 'instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }, + ); + + const svc = new AcpFileSystemService( + client, + 'session-2d-realpath', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [rootAlias] }, + ); + + await expect(svc.readTextFile({ path: filePath })).resolves.toEqual({ + content: 'instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }); + expect(fallback.readTextFile).toHaveBeenCalledWith({ + path: await fs.realpath(filePath), + }); + }); + }, + ); + + it('falls back to local reads for allowed local roots when ACP rejects them as symlink escapes', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const filePath = path.join(localRoot, 'instructions.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.writeFile(filePath, 'instructions', 'utf8'); + + const symlinkEscapeError = createLocalReadFallbackError( + filePath, + 'symlink_escape', + ); + const client = { + readTextFile: vi.fn().mockRejectedValue(symlinkEscapeError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + (fallback.readTextFile as ReturnType).mockResolvedValue({ + content: 'instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }); + + const svc = new AcpFileSystemService( + client, + 'session-2d-symlink', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + await expect(svc.readTextFile({ path: filePath })).resolves.toEqual({ + content: 'instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }); + expect(fallback.readTextFile).toHaveBeenCalledWith({ + path: await fs.realpath(filePath), + }); + }); + }); + + it('preserves the original ACP error when local read fallback fails', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const filePath = path.join(localRoot, 'instructions.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.writeFile(filePath, 'instructions', 'utf8'); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + const fallbackError = new Error('local read failed'); + (fallback.readTextFile as ReturnType).mockRejectedValue( + fallbackError, + ); + + const svc = new AcpFileSystemService( + client, + 'session-2d-fallback-fail', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + const err = await svc + .readTextFile({ path: filePath }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + message: `Local fallback read failed for ${filePath}: local read failed (original ACP error: path escapes workspace: ${filePath})`, + cause: { + fallbackError, + acpError: pathOutsideWorkspaceError, + }, + }); + }); + }); + + it('re-throws ENOENT from local read fallback without wrapping it', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const filePath = path.join(localRoot, 'instructions.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.writeFile(filePath, 'instructions', 'utf8'); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + const fallbackError = Object.assign( + new Error(`File not found: ${filePath}`), + { + code: 'ENOENT', + errno: -2, + path: filePath, + }, + ); + (fallback.readTextFile as ReturnType).mockRejectedValue( + fallbackError, + ); + + const svc = new AcpFileSystemService( + client, + 'session-2d-fallback-enoent', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + const err = await svc + .readTextFile({ path: filePath }) + .catch((e: unknown) => e); + + expect(err).toBe(fallbackError); + expect(err).toMatchObject({ + code: 'ENOENT', + errno: -2, + path: filePath, + }); + expect(fallback.readTextFile).toHaveBeenCalledWith({ + path: await fs.realpath(filePath), + }); + expect(mockDebugLogger.warn).not.toHaveBeenCalled(); + }); + }); + + it('does not fall back to local reads for missing files under allowed local roots', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const filePath = path.join(localRoot, 'missing.md'); + await fs.mkdir(localRoot, { recursive: true }); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + + const svc = new AcpFileSystemService( + client, + 'session-2d-missing-local-file', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + const err = await svc + .readTextFile({ path: filePath }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: pathOutsideWorkspaceError, + message: `path escapes workspace: ${filePath}`, + }); + expect(fallback.readTextFile).not.toHaveBeenCalled(); + }); + }); + + it('does not fall back to local reads outside configured roots', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'allowed'); + const outsideRoot = path.join(tempRoot, 'outside'); + const filePath = path.join(outsideRoot, 'outside-local-root.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.mkdir(outsideRoot, { recursive: true }); + await fs.writeFile(filePath, 'outside local root', 'utf8'); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + + const svc = new AcpFileSystemService( + client, + 'session-2e', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + const err = await svc + .readTextFile({ path: filePath }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: pathOutsideWorkspaceError, + message: `path escapes workspace: ${filePath}`, + }); + expect(Object.prototype.hasOwnProperty.call(err, 'code')).toBe(false); + expect(fallback.readTextFile).not.toHaveBeenCalled(); + }); + }); + + it('logs when a local read fallback is eligible but skipped', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'allowed'); + const outsideRoot = path.join(tempRoot, 'outside'); + const filePath = path.join(outsideRoot, 'outside-local-root.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.mkdir(outsideRoot, { recursive: true }); + await fs.writeFile(filePath, 'outside local root', 'utf8'); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-2e-skipped-log', + { readTextFile: true, writeTextFile: true }, + createFallback(), + { localReadRoots: [localRoot] }, + ); + + await svc.readTextFile({ path: filePath }).catch(() => undefined); + + expect(mockDebugLogger.debug).toHaveBeenCalledWith( + 'Local read fallback skipped - no safe local path', + { + path: filePath, + errorKind: 'path_outside_workspace', + }, + ); + }); + }); + + it('resolves configured local read roots for each fallback read', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const firstFilePath = path.join(localRoot, 'first.md'); + const secondFilePath = path.join(localRoot, 'second.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.writeFile(firstFilePath, 'first', 'utf8'); + await fs.writeFile(secondFilePath, 'second', 'utf8'); + + const client = { + readTextFile: vi + .fn() + .mockRejectedValueOnce(createLocalReadFallbackError(firstFilePath)) + .mockRejectedValueOnce( + createLocalReadFallbackError(secondFilePath), + ), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + (fallback.readTextFile as ReturnType) + .mockResolvedValueOnce({ + content: 'first', + _meta: { bom: false, encoding: 'utf-8' }, + }) + .mockResolvedValueOnce({ + content: 'second', + _meta: { bom: false, encoding: 'utf-8' }, + }); + + const svc = new AcpFileSystemService( + client, + 'session-2e-root-cache', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + await svc.readTextFile({ path: firstFilePath }); + await svc.readTextFile({ path: secondFilePath }); + + const resolvedLocalRoot = path.resolve(localRoot); + const localRootRealpathCalls = vi + .mocked(fsRealpath) + .mock.calls.filter(([value]) => value === resolvedLocalRoot); + expect(localRootRealpathCalls).toHaveLength(2); + }); + }); + + it('allows lazily-created local read roots on later fallback reads', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const firstFilePath = path.join(localRoot, 'missing.md'); + const secondFilePath = path.join(localRoot, 'instructions.md'); + + const client = { + readTextFile: vi + .fn() + .mockRejectedValueOnce(createLocalReadFallbackError(firstFilePath)) + .mockRejectedValueOnce( + createLocalReadFallbackError(secondFilePath), + ), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + (fallback.readTextFile as ReturnType).mockResolvedValue({ + content: 'instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }); + + const svc = new AcpFileSystemService( + client, + 'session-2e-lazy-root', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + await svc.readTextFile({ path: firstFilePath }).catch(() => undefined); + expect(fallback.readTextFile).not.toHaveBeenCalled(); + + await fs.mkdir(localRoot, { recursive: true }); + await fs.writeFile(secondFilePath, 'instructions', 'utf8'); + + await expect( + svc.readTextFile({ path: secondFilePath }), + ).resolves.toEqual({ + content: 'instructions', + _meta: { bom: false, encoding: 'utf-8' }, + }); + expect(fallback.readTextFile).toHaveBeenCalledWith({ + path: await fs.realpath(secondFilePath), + }); + }); + }); + + it('logs and excludes local read roots when realpath fails with non-ENOENT', async () => { + await withTempRoot(async (tempRoot) => { + const localRoot = path.join(tempRoot, 'skills'); + const filePath = path.join(localRoot, 'instructions.md'); + await fs.mkdir(localRoot, { recursive: true }); + await fs.writeFile(filePath, 'instructions', 'utf8'); + + const actualFsPromises = + await vi.importActual( + 'node:fs/promises', + ); + vi.mocked(fsRealpath).mockImplementation(async (value) => { + if (value === path.resolve(localRoot)) { + const err = new Error('permission denied') as NodeJS.ErrnoException; + err.code = 'EACCES'; + throw err; + } + return actualFsPromises.realpath(value); + }); + + try { + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + + const svc = new AcpFileSystemService( + client, + 'session-2e-root-realpath-failure', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [localRoot] }, + ); + + const err = await svc + .readTextFile({ path: filePath }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: pathOutsideWorkspaceError, + message: `path escapes workspace: ${filePath}`, + }); + expect(fallback.readTextFile).not.toHaveBeenCalled(); + expect(mockDebugLogger.warn).toHaveBeenCalledWith( + 'realpath failed during ACP local read fallback check', + { + path: localRoot, + error: 'permission denied', + }, + ); + } finally { + vi.mocked(fsRealpath).mockRestore(); + } + }); + }); + + it('ignores empty configured local read roots', async () => { + await withTempRoot(async (tempRoot) => { + const filePath = path.join(tempRoot, 'outside-workspace.md'); + await fs.writeFile(filePath, 'outside workspace', 'utf8'); + + const pathOutsideWorkspaceError = + createLocalReadFallbackError(filePath); + const client = { + readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError), + } as unknown as AgentSideConnection; + const fallback = createFallback(); + + const svc = new AcpFileSystemService( + client, + 'session-2f', + { readTextFile: true, writeTextFile: true }, + fallback, + { localReadRoots: [''] }, + ); + + const err = await svc + .readTextFile({ path: filePath }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: pathOutsideWorkspaceError, + message: `path escapes workspace: ${filePath}`, + }); + expect(Object.prototype.hasOwnProperty.call(err, 'code')).toBe(false); + expect(fallback.readTextFile).not.toHaveBeenCalled(); }); }); @@ -236,5 +1071,66 @@ describe('AcpFileSystemService', () => { }); expect(client.writeTextFile).not.toHaveBeenCalled(); }); + + it('normalizes plain object ACP write errors without exposing numeric codes as Node error codes', async () => { + const writeError = { + code: INTERNAL_ERROR_CODE, + message: 'Write failed', + }; + const client = { + writeTextFile: vi.fn().mockRejectedValue(writeError), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-8', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + const err = await svc + .writeTextFile({ + path: '/some/file.txt', + content: 'hello', + }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Error); + expect(err).toMatchObject({ + cause: writeError, + message: 'Write failed', + }); + expect(Object.prototype.hasOwnProperty.call(err, 'code')).toBe(false); + expect(String(err)).toContain('Write failed'); + expect(String(err)).not.toContain('[object Object]'); + }); + + it('converts RESOURCE_NOT_FOUND write errors to ENOENT', async () => { + const resourceNotFoundError = { + code: RESOURCE_NOT_FOUND_CODE, + message: 'File not found', + }; + const client = { + writeTextFile: vi.fn().mockRejectedValue(resourceNotFoundError), + } as unknown as AgentSideConnection; + + const svc = new AcpFileSystemService( + client, + 'session-9', + { readTextFile: true, writeTextFile: true }, + createFallback(), + ); + + await expect( + svc.writeTextFile({ + path: '/some/file.txt', + content: 'hello', + }), + ).rejects.toMatchObject({ + code: 'ENOENT', + errno: -2, + path: '/some/file.txt', + }); + }); }); }); diff --git a/packages/cli/src/acp-integration/service/filesystem.ts b/packages/cli/src/acp-integration/service/filesystem.ts index 1990c26503..95fb1d14b6 100644 --- a/packages/cli/src/acp-integration/service/filesystem.ts +++ b/packages/cli/src/acp-integration/service/filesystem.ts @@ -16,21 +16,64 @@ import type { FileSystemService, ReadTextFileResponse, } from '@qwen-code/qwen-code-core'; +import { + createDebugLogger, + getErrorMessage, + isSubpath, +} from '@qwen-code/qwen-code-core'; +import { realpath } from 'node:fs/promises'; +import path from 'node:path'; const RESOURCE_NOT_FOUND_CODE = -32002; +const PATH_OUTSIDE_WORKSPACE_KIND = 'path_outside_workspace'; +const SYMLINK_ESCAPE_KIND = 'symlink_escape'; +const LOCAL_READ_FALLBACK_ERROR_KINDS = new Set([ + PATH_OUTSIDE_WORKSPACE_KIND, + SYMLINK_ESCAPE_KIND, +]); +const debugLogger = createDebugLogger('ACP_FILE_SYSTEM'); + +interface AcpFileSystemServiceOptions { + localReadRoots?: readonly string[]; +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} 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; + if (isRecord(error)) { + return error['code']; } return undefined; } +function getErrorData(error: unknown): Record | undefined { + const data = isRecord(error) ? error['data'] : undefined; + return isRecord(data) ? data : undefined; +} + +function getErrorKind(error: unknown): string | undefined { + const data = getErrorData(error); + if (data && typeof data['errorKind'] === 'string') { + return data['errorKind']; + } + return undefined; +} + +function normalizeError(error: unknown): Error { + if (error instanceof Error) return error; + + return new Error(getErrorMessage(error), { + cause: error, + }); +} + function createEnoentError(filePath: string): NodeJS.ErrnoException { const err = new Error(`File not found: ${filePath}`) as NodeJS.ErrnoException; err.code = 'ENOENT'; @@ -39,12 +82,36 @@ function createEnoentError(filePath: string): NodeJS.ErrnoException { return err; } +function isLocalReadFallbackErrorKind(errorKind: unknown): boolean { + return ( + typeof errorKind === 'string' && + LOCAL_READ_FALLBACK_ERROR_KINDS.has(errorKind) + ); +} + +async function resolveRealPath(value: string): Promise { + if (!value.trim()) return undefined; + + try { + return await realpath(path.resolve(value)); + } catch (error) { + if (getErrorCode(error) !== 'ENOENT') { + debugLogger.warn('realpath failed during ACP local read fallback check', { + path: value, + error: getErrorMessage(error), + }); + } + return undefined; + } +} + export class AcpFileSystemService implements FileSystemService { constructor( private readonly connection: AgentSideConnection, private readonly sessionId: string, private readonly capabilities: FileSystemCapability, private readonly fallback: FileSystemService, + private readonly options: AcpFileSystemServiceOptions = {}, ) {} async readTextFile( @@ -67,7 +134,50 @@ export class AcpFileSystemService implements FileSystemService { throw createEnoentError(params.path); } - throw error; + const errorKind = getErrorKind(error); + const shouldTryLocalReadFallback = + isLocalReadFallbackErrorKind(errorKind); + const fallbackPath = shouldTryLocalReadFallback + ? await this.getLocalReadFallbackPath(params.path) + : undefined; + if (shouldTryLocalReadFallback && !fallbackPath) { + debugLogger.debug('Local read fallback skipped - no safe local path', { + path: params.path, + errorKind, + }); + } + if (shouldTryLocalReadFallback && fallbackPath) { + debugLogger.debug('Falling back to local read after ACP error', { + path: params.path, + resolvedPath: fallbackPath, + errorKind, + error: getErrorMessage(error), + }); + try { + return await this.fallback.readTextFile({ + ...params, + path: fallbackPath, + }); + } catch (fallbackError) { + if (getErrorCode(fallbackError) === 'ENOENT') { + throw fallbackError; + } + + debugLogger.warn('Local read fallback failed after ACP error', { + path: params.path, + resolvedPath: fallbackPath, + errorKind, + originalError: getErrorMessage(error), + fallbackError: getErrorMessage(fallbackError), + }); + throw new Error( + `Local fallback read failed for ${params.path}: ${getErrorMessage(fallbackError)} (original ACP error: ${getErrorMessage(error)})`, + { cause: { fallbackError, acpError: error } }, + ); + } + } + + throw normalizeError(error); } return response; @@ -85,11 +195,18 @@ export class AcpFileSystemService implements FileSystemService { ? '\uFEFF' + params.content : params.content; - await this.connection.writeTextFile({ - ...params, - content: finalContent, - sessionId: this.sessionId, - }); + try { + await this.connection.writeTextFile({ + ...params, + content: finalContent, + sessionId: this.sessionId, + }); + } catch (error) { + if (getErrorCode(error) === RESOURCE_NOT_FOUND_CODE) { + throw createEnoentError(params.path); + } + throw normalizeError(error); + } return { _meta: params._meta }; } @@ -97,4 +214,26 @@ export class AcpFileSystemService implements FileSystemService { findFiles(fileName: string, searchPaths: readonly string[]): string[] { return this.fallback.findFiles(fileName, searchPaths); } + + private async getResolvedLocalReadRoots(): Promise { + const roots = await Promise.all( + (this.options.localReadRoots ?? []).map(resolveRealPath), + ); + return roots.filter((root): root is string => Boolean(root)); + } + + private async getLocalReadFallbackPath( + filePath: string, + ): Promise { + const normalizedFilePath = path.resolve(filePath); + const realFilePath = await resolveRealPath(normalizedFilePath); + if (!realFilePath) return undefined; + + for (const realRoot of await this.getResolvedLocalReadRoots()) { + if (isSubpath(realRoot, realFilePath)) { + return realFilePath; + } + } + return undefined; + } } diff --git a/packages/core/src/tools/edit.test.ts b/packages/core/src/tools/edit.test.ts index 3c1c126eac..15fb31ddcb 100644 --- a/packages/core/src/tools/edit.test.ts +++ b/packages/core/src/tools/edit.test.ts @@ -451,6 +451,30 @@ describe('EditTool', () => { ).rejects.toThrow(); }); + it('should surface plain object read errors without object stringification', async () => { + fs.writeFileSync(filePath, 'some old content here'); + seedPriorRead(filePath); + vi.spyOn(fsService, 'readTextFile').mockRejectedValueOnce({ + message: 'Plain object read error', + }); + + const params: EditToolParams = { + file_path: filePath, + old_string: 'old', + new_string: 'new', + }; + const invocation = tool.build(params); + const err = await invocation + .getConfirmationDetails(new AbortController().signal) + .catch((error: unknown) => error); + + expect(err).toBeInstanceOf(Error); + expect((err as Error).message).toContain( + 'Error preparing edit: Plain object read error', + ); + expect((err as Error).message).not.toContain('[object Object]'); + }); + it('should request confirmation for creating a new file (empty old_string)', async () => { const newFileName = 'new_file.txt'; const newFilePath = path.join(rootDir, newFileName); @@ -1074,6 +1098,28 @@ describe('EditTool', () => { expect(result.error?.type).toBe(ToolErrorType.EDIT_NO_CHANGE); }); + it('should return EDIT_PREPARATION_FAILURE with plain object read error messages', async () => { + fs.writeFileSync(filePath, 'content', 'utf8'); + seedPriorRead(filePath); + vi.spyOn(fsService, 'readTextFile').mockRejectedValueOnce({ + message: 'Plain object read error', + }); + + const params: EditToolParams = { + file_path: filePath, + old_string: 'content', + new_string: 'new content', + }; + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); + + expect(result.error?.type).toBe(ToolErrorType.EDIT_PREPARATION_FAILURE); + expect(result.llmContent).toContain( + 'Error preparing edit: Plain object read error', + ); + expect(result.llmContent).not.toContain('[object Object]'); + }); + it('should throw INVALID_PARAMETERS error for relative path', async () => { const params: EditToolParams = { file_path: 'relative/path.txt', @@ -1100,6 +1146,29 @@ describe('EditTool', () => { const result = await invocation.execute(new AbortController().signal); expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); }); + + it('should surface plain object write error messages without object stringification', async () => { + fs.writeFileSync(filePath, 'content', 'utf8'); + seedPriorRead(filePath); + + vi.spyOn(fsService, 'writeTextFile').mockRejectedValueOnce({ + message: 'Plain object edit error', + }); + + const params: EditToolParams = { + file_path: filePath, + old_string: 'content', + new_string: 'new content', + }; + const invocation = tool.build(params); + const result = await invocation.execute(new AbortController().signal); + + expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); + expect(result.llmContent).toContain( + 'Error executing edit: Plain object edit error', + ); + expect(result.llmContent).not.toContain('[object Object]'); + }); }); describe('getDescription', () => { diff --git a/packages/core/src/tools/edit.ts b/packages/core/src/tools/edit.ts index 0542d3aa1b..49f03753e5 100644 --- a/packages/core/src/tools/edit.ts +++ b/packages/core/src/tools/edit.ts @@ -18,7 +18,7 @@ import type { PermissionDecision } from '../permissions/types.js'; import { BaseDeclarativeTool, Kind, ToolConfirmationOutcome } from './tools.js'; import { ToolErrorType } from './tool-error.js'; import { makeRelative, shortenPath, unescapePath } from '../utils/paths.js'; -import { isNodeError } from '../utils/errors.js'; +import { getErrorMessage, isNodeError } from '../utils/errors.js'; import type { Config } from '../config/config.js'; import { ApprovalMode } from '../config/config.js'; import { isAnyAutoMemPath, isTeamAutoMemPath } from '../memory/paths.js'; @@ -416,7 +416,7 @@ class EditToolInvocation implements ToolInvocation { if (abortSignal.aborted) { throw error; } - const errorMsg = error instanceof Error ? error.message : String(error); + const errorMsg = getErrorMessage(error); throw new Error(`Error preparing edit: ${errorMsg}`); } @@ -484,7 +484,7 @@ class EditToolInvocation implements ToolInvocation { if (signal.aborted) { throw error; } - const errorMsg = error instanceof Error ? error.message : String(error); + const errorMsg = getErrorMessage(error); return { llmContent: `Error preparing edit: ${errorMsg}`, returnDisplay: `Error preparing edit: ${errorMsg}`, @@ -725,7 +725,7 @@ class EditToolInvocation implements ToolInvocation { returnDisplay: displayResult, }; } catch (error) { - const errorMsg = error instanceof Error ? error.message : String(error); + const errorMsg = getErrorMessage(error); return { llmContent: `Error executing edit: ${errorMsg}`, returnDisplay: `Error writing file: ${errorMsg}`, diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index f90287a75d..08fe5edcb8 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -13,6 +13,7 @@ import os from 'node:os'; import fs from 'node:fs'; import fsp from 'node:fs/promises'; import type { Config } from '../config/config.js'; +import { Storage } from '../config/storage.js'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import { FileReadCache } from '../services/fileReadCache.js'; import { StandardFileSystemService } from '../services/fileSystemService.js'; @@ -114,7 +115,7 @@ describe('ReadFileTool', () => { expect(typeof result).not.toBe('string'); }); - it('should allow access to files in OS temp directory', () => { + it('should build an invocation for files in the OS temp directory', () => { const params: ReadFileToolParams = { file_path: path.join(os.tmpdir(), 'pr-review-context.md'), }; @@ -212,6 +213,37 @@ describe('ReadFileTool', () => { expect(permission).toBe('allow'); }); + it('should return allow for paths within the global qwen temp directory', async () => { + const params: ReadFileToolParams = { + file_path: path.join(Storage.getGlobalTempDir(), 'temp-file.txt'), + }; + const invocation = tool.build(params); + const permission = await invocation.getDefaultPermission(); + expect(permission).toBe('allow'); + }); + + it('should return allow for paths within the user extensions directory', async () => { + const params: ReadFileToolParams = { + file_path: path.join( + Storage.getUserExtensionsDir(), + 'my-ext', + 'index.js', + ), + }; + const invocation = tool.build(params); + const permission = await invocation.getDefaultPermission(); + expect(permission).toBe('allow'); + }); + + it('should return ask for paths directly under the OS temp directory', async () => { + const params: ReadFileToolParams = { + file_path: path.join(os.tmpdir(), 'pr-review-context.md'), + }; + const invocation = tool.build(params); + const permission = await invocation.getDefaultPermission(); + expect(permission).toBe('ask'); + }); + it('should return allow for paths within the subagent transcripts dir', async () => { const params: ReadFileToolParams = { file_path: path.join( @@ -648,7 +680,7 @@ describe('ReadFileTool', () => { expect(result.returnDisplay).toBe(''); }); - it('should successfully read files from OS temp directory', async () => { + it('should read OS temp files after the invocation is executed', async () => { const osTempFile = await fsp.mkdtemp( path.join(os.tmpdir(), 'read-file-test-'), ); diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 5f31ce4f2c..ff75476bdb 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import os from 'node:os'; import path from 'node:path'; import fs from 'node:fs/promises'; import type { Stats } from 'node:fs'; @@ -99,20 +98,21 @@ class ReadFileToolInvocation extends BaseToolInvocation< } /** - * Returns 'ask' for paths outside the workspace/temp/userSkills directories, - * so that external file reads require user confirmation. + * Returns 'ask' for paths outside the workspace/qwen-managed temp/userSkills + * directories, so that external file reads require user confirmation. */ override async getDefaultPermission(): Promise { const filePath = path.resolve(this.params.file_path); const workspaceContext = this.config.getWorkspaceContext(); + // SYNC: Keep these roots and the auto-memory check below aligned with + // AcpAgent.setupFileSystem's localReadRoots. const allowedRoots = [ this.config.storage.getProjectTempDir(), // Background subagent transcripts live under /subagents/ and // are advertised to the model as polling targets via read_file. path.join(this.config.storage.getProjectDir(), 'subagents'), Storage.getGlobalTempDir(), - os.tmpdir(), ...this.config.storage.getUserSkillsDirs(), Storage.getUserExtensionsDir(), ]; diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 7648777164..a8a265badd 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -846,6 +846,51 @@ describe('WriteFileTool', () => { 'Error writing to file: Generic write error', ); }); + + it('should include cause details for non-Node write errors', async () => { + const filePath = path.join(rootDir, 'write_error_with_cause.txt'); + const content = 'test content'; + + vi.restoreAllMocks(); + + const cause = Object.assign(new Error(''), { code: 'ECONNREFUSED' }); + vi.spyOn(fsService, 'writeTextFile').mockRejectedValueOnce( + new TypeError('fetch failed', { cause }), + ); + + const params = { file_path: filePath, content }; + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); + expect(result.llmContent).toContain( + 'Error writing to file: fetch failed (cause: ECONNREFUSED)', + ); + expect(result.returnDisplay).toContain( + 'Error writing to file: fetch failed (cause: ECONNREFUSED)', + ); + }); + + it('should surface plain object write error messages without object stringification', async () => { + const filePath = path.join(rootDir, 'plain_object_error_file.txt'); + const content = 'test content'; + + vi.restoreAllMocks(); + + vi.spyOn(fsService, 'writeTextFile').mockRejectedValueOnce({ + message: 'Plain object write error', + }); + + const params = { file_path: filePath, content }; + const invocation = tool.build(params); + const result = await invocation.execute(abortSignal); + + expect(result.error?.type).toBe(ToolErrorType.FILE_WRITE_FAILURE); + expect(result.llmContent).toContain( + 'Error writing to file: Plain object write error', + ); + expect(result.llmContent).not.toContain('[object Object]'); + }); }); describe('BOM preservation (Issue #1672)', () => { diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 742e61ccbf..c0a0951b6e 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -612,10 +612,8 @@ class WriteFileToolInvocation extends BaseToolInvocation< if (this.config.getDebugMode() && error.stack) { debugLogger.debug('Write file error stack:', error.stack); } - } else if (error instanceof Error) { - errorMsg = `Error writing to file: ${error.message}`; } else { - errorMsg = `Error writing to file: ${String(error)}`; + errorMsg = `Error writing to file: ${getErrorMessage(error)}`; } return { @@ -689,9 +687,9 @@ The user has the ability to modify \`content\`. If modified, this will be stated } } } catch (statError: unknown) { - return `Error accessing path properties for validation: ${filePath}. Reason: ${ - statError instanceof Error ? statError.message : String(statError) - }`; + return `Error accessing path properties for validation: ${filePath}. Reason: ${getErrorMessage( + statError, + )}`; } const teamMemoryError = checkTeamMemorySecrets( diff --git a/packages/core/src/utils/errors.test.ts b/packages/core/src/utils/errors.test.ts index 36a098ca6a..9ddf157b7e 100644 --- a/packages/core/src/utils/errors.test.ts +++ b/packages/core/src/utils/errors.test.ts @@ -38,10 +38,100 @@ describe('getErrorMessage cause unwrapping', () => { expect(getErrorMessage(err)).toBe('outer (cause: inner detail)'); }); + it('bounds Error messages that include long cause details', () => { + const expectedPrefix = 'outer (cause: '; + const err = new Error('outer', { + cause: { message: 'x'.repeat(2000) }, + }); + const message = getErrorMessage(err); + + expect(message).toBe( + `${expectedPrefix}${'x'.repeat(1000 - expectedPrefix.length - 3)}...`, + ); + expect(message.length).toBe(1000); + }); + it('does not append a redundant cause equal to the message', () => { const err = new Error('same', { cause: new Error('same') }); expect(getErrorMessage(err)).toBe('same'); }); + + it('uses the message from plain error-like objects', () => { + expect( + getErrorMessage({ + code: -32603, + message: 'path escapes workspace: /root/.qwen/skills/example.md', + data: { errorKind: 'path_outside_workspace' }, + }), + ).toBe('path escapes workspace: /root/.qwen/skills/example.md'); + }); + + it('surfaces cause details from plain error-like objects', () => { + expect( + getErrorMessage({ + message: 'fetch failed', + cause: { code: 'ECONNREFUSED' }, + }), + ).toBe('fetch failed (cause: ECONNREFUSED)'); + }); + + it('surfaces message and numeric code from plain object causes', () => { + expect( + getErrorMessage({ + message: 'fetch failed', + cause: { code: -32603, message: 'connection refused' }, + }), + ).toBe('fetch failed (cause: -32603: connection refused)'); + }); + + it('surfaces message-only plain object causes', () => { + expect( + getErrorMessage({ + message: 'fetch failed', + cause: { message: 'connection refused' }, + }), + ).toBe('fetch failed (cause: connection refused)'); + }); + + it('bounds long messages from plain error-like objects', () => { + const message = getErrorMessage({ message: 'x'.repeat(2000) }); + + expect(message).toBe(`${'x'.repeat(997)}...`); + }); + + it('stringifies plain objects without a message', () => { + expect(getErrorMessage({ code: -32603 })).toBe('{"code":-32603}'); + }); + + it('bounds stringified plain objects without a message', () => { + const message = getErrorMessage({ detail: 'x'.repeat(2000) }); + + expect(message.length).toBeLessThanOrEqual(1000); + expect(message).toContain('"detail"'); + }); + + it('uses plain object code when JSON stringification fails', () => { + const circular: Record = { code: -32603 }; + circular['self'] = circular; + + expect(getErrorMessage(circular)).toBe('-32603'); + }); + + it('uses String formatting when circular plain objects have no error details', () => { + const circular: Record = {}; + circular['self'] = circular; + + expect(getErrorMessage(circular)).toBe('[object Object]'); + }); + + it('uses String formatting for arrays', () => { + expect(getErrorMessage([1, 2, 3])).toBe('1,2,3'); + }); + + it('uses String formatting for null and undefined', () => { + expect(getErrorMessage(null)).toBe('null'); + expect(getErrorMessage(undefined)).toBe('undefined'); + }); }); describe('isAbortError', () => { diff --git a/packages/core/src/utils/errors.ts b/packages/core/src/utils/errors.ts index 114aa8d291..c5d2e99002 100644 --- a/packages/core/src/utils/errors.ts +++ b/packages/core/src/utils/errors.ts @@ -10,6 +10,8 @@ interface GaxiosError { }; } +const MAX_STRINGIFIED_ERROR_MESSAGE_LENGTH = 1000; + export function isNodeError(error: unknown): error is NodeJS.ErrnoException { return error instanceof Error && 'code' in error; } @@ -72,22 +74,69 @@ function describeSingleError(err: unknown): string | undefined { } return msg || codeStr || (err.name !== 'Error' ? err.name : undefined); } - if (err && typeof err === 'object' && 'code' in err) { - const code = (err as { code?: unknown }).code; - if (typeof code === 'string' && code) return code; + if (err && typeof err === 'object' && !Array.isArray(err)) { + const rec = err as Record; + const code = rec['code']; + const codeStr = + typeof code === 'string' && code + ? code + : typeof code === 'number' + ? String(code) + : undefined; + const message = rec['message']; + const msg = + typeof message === 'string' && message.trim() + ? message.trim() + : undefined; + if (msg && codeStr && !msg.includes(codeStr)) { + return `${codeStr}: ${msg}`; + } + return msg || codeStr; } const str = String(err); return str && str !== '[object Object]' ? str : undefined; } +function truncateStringifiedErrorMessage(message: string): string { + if (message.length <= MAX_STRINGIFIED_ERROR_MESSAGE_LENGTH) { + return message; + } + return `${message.slice(0, MAX_STRINGIFIED_ERROR_MESSAGE_LENGTH - 3)}...`; +} + export function getErrorMessage(error: unknown): string { if (error instanceof Error) { const detail = describeErrorCause(error.cause); if (detail && detail !== error.message) { - return `${error.message} (cause: ${detail})`; + return truncateStringifiedErrorMessage( + `${error.message} (cause: ${detail})`, + ); } return error.message; } + if (error !== null && typeof error === 'object' && !Array.isArray(error)) { + const { message, cause } = error as { + message?: unknown; + cause?: unknown; + }; + if (typeof message === 'string' && message.trim()) { + const detail = describeErrorCause(cause); + const result = + detail && detail !== message + ? `${message} (cause: ${detail})` + : message; + return truncateStringifiedErrorMessage(result); + } + try { + const serialized = JSON.stringify(error); + return serialized + ? truncateStringifiedErrorMessage(serialized) + : String(error); + } catch { + const detail = describeSingleError(error); + return detail ? truncateStringifiedErrorMessage(detail) : String(error); + } + } try { return String(error); } catch { diff --git a/packages/core/src/utils/fileUtils.test.ts b/packages/core/src/utils/fileUtils.test.ts index 9bbcc60704..a7844d47de 100644 --- a/packages/core/src/utils/fileUtils.test.ts +++ b/packages/core/src/utils/fileUtils.test.ts @@ -1055,6 +1055,50 @@ describe('fileUtils', () => { expect(result.returnDisplay).toContain('Simulated read error'); }); + it('should surface messages from plain object text read errors', async () => { + actualNodeFs.writeFileSync(testTextFilePath, 'content'); + vi.spyOn(fsService, 'readTextFile').mockRejectedValueOnce({ + code: -32603, + message: + 'path escapes workspace: /root/.qwen/skills/dataworks-di-data-processor/instructions/interaction_norms.md', + data: { + errorKind: 'path_outside_workspace', + status: 400, + }, + }); + + const result = await processSingleFileContent( + testTextFilePath, + mockConfig, + ); + + expect(result.error).toContain('path escapes workspace'); + expect(result.returnDisplay).toContain('path escapes workspace'); + expect(result.error).not.toContain('[object Object]'); + expect(result.returnDisplay).not.toContain('[object Object]'); + }); + + it('should surface messages from plain object notebook read errors', async () => { + const notebookPath = path.join(tempRootDir, 'analysis.ipynb'); + actualNodeFs.writeFileSync(notebookPath, '{}'); + vi.spyOn(fs.promises, 'readFile').mockRejectedValueOnce({ + code: -32603, + message: 'notebook is outside allowed roots', + data: { + errorKind: 'path_outside_workspace', + status: 400, + }, + }); + + const result = await processSingleFileContent(notebookPath, mockConfig); + + expect(result.error).toContain('notebook is outside allowed roots'); + expect(result.returnDisplay).toContain('Error reading notebook'); + expect(result.llmContent).toContain('notebook is outside allowed roots'); + expect(result.error).not.toContain('[object Object]'); + expect(result.llmContent).not.toContain('[object Object]'); + }); + it('should handle read errors for image/pdf files', async () => { actualNodeFs.writeFileSync(testImageFilePath, 'content'); // File must exist mockMimeGetType.mockReturnValue('image/png'); diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 6e36c60c6d..3dec25cf4b 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -18,7 +18,7 @@ import { ToolErrorType } from '../tools/tool-error.js'; import { BINARY_EXTENSIONS } from './ignorePatterns.js'; import type { Config } from '../config/config.js'; import { createDebugLogger } from './debugLogger.js'; -import { isNodeError } from './errors.js'; +import { getErrorMessage, isNodeError } from './errors.js'; import type { InputModalities } from '../core/contentGenerator.js'; import { detectEncodingFromBuffer } from './systemEncoding.js'; import { extractPDFText, parsePDFPageRange } from './pdf.js'; @@ -1222,7 +1222,7 @@ export async function processSingleFileContent( stats, }; } catch (e: unknown) { - const msg = e instanceof Error ? e.message : String(e); + const msg = getErrorMessage(e); return { llmContent: `Error parsing notebook ${relativePathForDisplay}: ${msg}`, returnDisplay: `Error reading notebook: ${relativePathForDisplay}`, @@ -1242,7 +1242,7 @@ export async function processSingleFileContent( } } } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); + const errorMessage = getErrorMessage(error); const displayPath = path .relative(rootDirectory, filePath) .replace(/\\/g, '/'); diff --git a/scripts/check-serve-fast-path-bundle.js b/scripts/check-serve-fast-path-bundle.js index e05c8e2419..c8be9c9731 100644 --- a/scripts/check-serve-fast-path-bundle.js +++ b/scripts/check-serve-fast-path-bundle.js @@ -11,7 +11,7 @@ import { fileURLToPath } from 'node:url'; const DEFAULT_METAFILE_PATH = resolve('dist/esbuild.json'); const METAFILE_BUILD_COMMAND = - 'npm run build -- --cli-only && npx cross-env DEV=true npm run bundle'; + 'npm run build -- --cli-only && cross-env DEV=true npm run bundle'; const SERVE_PRE_LISTEN_ROOTS = [ { label: 'serve fast path entry', diff --git a/scripts/tests/package-scripts.test.js b/scripts/tests/package-scripts.test.js new file mode 100644 index 0000000000..366db5758a --- /dev/null +++ b/scripts/tests/package-scripts.test.js @@ -0,0 +1,25 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const root = path.resolve(__dirname, '../..'); + +describe('package scripts', () => { + it('runs the serve fast-path bundle check in CI tests', () => { + const packageJson = JSON.parse( + readFileSync(path.join(root, 'package.json'), 'utf8'), + ); + + expect(packageJson.scripts['test:ci']).toContain( + 'npm run check:serve-fast-path-bundle', + ); + }); +}); diff --git a/scripts/tests/serve-fast-path-bundle-check.test.js b/scripts/tests/serve-fast-path-bundle-check.test.js index 4c467e29c3..dab1731bd8 100644 --- a/scripts/tests/serve-fast-path-bundle-check.test.js +++ b/scripts/tests/serve-fast-path-bundle-check.test.js @@ -238,7 +238,7 @@ describe('serve fast-path bundle check', () => { /Could not find bundled outputs for serve pre-listen roots/, ); expect(() => findServeFastPathBundleOffenders(metafile)).toThrow( - /npm run build -- --cli-only && npx cross-env DEV=true npm run bundle/, + /npm run build -- --cli-only && cross-env DEV=true npm run bundle/, ); }); @@ -313,7 +313,7 @@ describe('serve fast-path bundle check', () => { metafilePath: join(tempDir, 'dist', 'esbuild.json'), }), ).toThrow( - /npm run build -- --cli-only && npx cross-env DEV=true npm run bundle/, + /npm run build -- --cli-only && cross-env DEV=true npm run bundle/, ); } finally { rmSync(tempDir, { recursive: true, force: true }); @@ -331,7 +331,7 @@ describe('serve fast-path bundle check', () => { /Invalid esbuild metafile at .*dist[/\\]esbuild\.json/, ); expect(() => checkServeFastPathBundle({ metafilePath })).toThrow( - /Run `npm run build -- --cli-only && npx cross-env DEV=true npm run bundle` to regenerate it/, + /Run `npm run build -- --cli-only && cross-env DEV=true npm run bundle` to regenerate it/, ); } finally { rmSync(tempDir, { recursive: true, force: true });