Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/cli/src/acp-integration/acpAgent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down
14 changes: 14 additions & 0 deletions packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
findProviderById,
getAllGeminiMdFilenames,
getAutoMemoryRoot,
getUserAutoMemoryRoot,
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
getDefaultBaseUrlForProtocol,
getDefaultModelIds,
getScopedEnvContents,
Expand Down Expand Up @@ -130,6 +131,7 @@ import { pipeline } from 'node:stream/promises';
import * as fs from 'node:fs/promises';
import { createRequire } from 'node:module';
import * as path from 'node:path';
import * as os from 'node:os';
import { createGunzip } from 'node:zlib';
import type { LoadedSettings } from '../config/settings.js';
import {
Expand Down Expand Up @@ -7627,6 +7629,18 @@ class QwenAgent implements Agent {
config.getSessionId(),
this.clientCapabilities.fs,
config.getFileSystemService(),
{
localReadRoots: [
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
config.storage.getProjectTempDir(),
path.join(config.storage.getProjectDir(), 'subagents'),
Storage.getGlobalTempDir(),
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
os.tmpdir(),
getAutoMemoryRoot(config.getTargetDir()),
getUserAutoMemoryRoot(),
...config.storage.getUserSkillsDirs(),
Storage.getUserExtensionsDir(),
],
},
);
config.setFileSystemService(acpFileSystemService);
}
Expand Down
134 changes: 133 additions & 1 deletion packages/cli/src/acp-integration/service/filesystem.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import { describe, expect, it, vi } from 'vitest';
import type { FileSystemService } from '@qwen-code/qwen-code-core';
import { AcpFileSystemService } from './filesystem.js';
import type { AgentSideConnection } from '@agentclientprotocol/sdk';
import os from 'node:os';
import path from 'node:path';

const RESOURCE_NOT_FOUND_CODE = -32002;
const INTERNAL_ERROR_CODE = -32603;
Expand Down Expand Up @@ -73,7 +75,7 @@ describe('AcpFileSystemService', () => {
});
});

it('re-throws other errors unchanged', async () => {
it('preserves code and message for other read errors', async () => {
const otherError = {
code: INTERNAL_ERROR_CODE,
message: 'Internal error',
Expand All @@ -97,6 +99,136 @@ describe('AcpFileSystemService', () => {
});
});

it('normalizes plain object ACP errors to Error instances with the original message', 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({
code: INTERNAL_ERROR_CODE,
message: 'Internal error',
});
expect(String(err)).toContain('Internal error');
expect(String(err)).not.toContain('[object Object]');
});

it('falls back to local reads for allowed local roots when ACP rejects them as outside the workspace', async () => {
const skillRoot = path.join(os.homedir(), '.qwen', 'skills');
const filePath = path.join(
skillRoot,
'dataworks-di-data-processor',
'instructions',
'interaction_norms.md',
);
const pathOutsideWorkspaceError = {
code: INTERNAL_ERROR_CODE,
message: `path escapes workspace: ${filePath}`,
data: {
errorKind: 'path_outside_workspace',
status: 400,
},
};
const client = {
readTextFile: vi.fn().mockRejectedValue(pathOutsideWorkspaceError),
} as unknown as AgentSideConnection;
const fallback = createFallback();
(fallback.readTextFile as ReturnType<typeof vi.fn>).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: filePath });
});

it('does not fall back to local reads outside configured roots', async () => {
const localRoot = path.join(os.tmpdir(), 'acp-local-read-root');
const filePath = path.join(os.tmpdir(), 'outside-local-root.md');
const pathOutsideWorkspaceError = {
code: INTERNAL_ERROR_CODE,
message: `path escapes workspace: ${filePath}`,
data: {
errorKind: 'path_outside_workspace',
status: 400,
},
};
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] },
);

await expect(svc.readTextFile({ path: filePath })).rejects.toMatchObject({
code: INTERNAL_ERROR_CODE,
message: `path escapes workspace: ${filePath}`,
});
expect(fallback.readTextFile).not.toHaveBeenCalled();
});

it('ignores empty configured local read roots', async () => {
const filePath = path.join(process.cwd(), 'outside-workspace.md');
const pathOutsideWorkspaceError = {
code: INTERNAL_ERROR_CODE,
message: `path escapes workspace: ${filePath}`,
data: {
errorKind: 'path_outside_workspace',
status: 400,
},
};
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: [''] },
);

await expect(svc.readTextFile({ path: filePath })).rejects.toMatchObject({
code: INTERNAL_ERROR_CODE,
message: `path escapes workspace: ${filePath}`,
});
expect(fallback.readTextFile).not.toHaveBeenCalled();
});

it('uses fallback when readTextFile capability is disabled', async () => {
const client = {
readTextFile: vi.fn(),
Expand Down
70 changes: 69 additions & 1 deletion packages/cli/src/acp-integration/service/filesystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,19 @@ import type {
FileSystemService,
ReadTextFileResponse,
} from '@qwen-code/qwen-code-core';
import { getErrorMessage } from '../../utils/errors.js';
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
import path from 'node:path';

const RESOURCE_NOT_FOUND_CODE = -32002;
const PATH_OUTSIDE_WORKSPACE_KIND = 'path_outside_workspace';

interface AcpFileSystemServiceOptions {
localReadRoots?: readonly string[];
}
Comment thread
doudouOUC marked this conversation as resolved.
Resolved

function isRecord(value: unknown): value is Record<string, unknown> {
return typeof value === 'object' && value !== null;
}

function getErrorCode(error: unknown): unknown {
if (error instanceof RequestError) {
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
Expand All @@ -31,6 +42,37 @@ function getErrorCode(error: unknown): unknown {
return undefined;
}

function getErrorData(error: unknown): Record<string, unknown> | undefined {
const data = isRecord(error) ? error['data'] : undefined;
return isRecord(data) ? data : undefined;
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
}

function getErrorKind(error: unknown): unknown {
const data = getErrorData(error);
if (data && typeof data['errorKind'] === 'string') {
return data['errorKind'];
}
if (isRecord(error) && typeof error['errorKind'] === 'string') {
return error['errorKind'];
}
return undefined;
}
Comment thread
doudouOUC marked this conversation as resolved.
Resolved

function normalizeError(error: unknown): Error {
if (error instanceof Error) return error;

const normalized = new Error(getErrorMessage(error)) as Error &
Record<string, unknown>;
if (isRecord(error)) {
for (const [key, value] of Object.entries(error)) {
if (key !== 'message') {
normalized[key] = value;
}
}
}
return normalized;
}

function createEnoentError(filePath: string): NodeJS.ErrnoException {
const err = new Error(`File not found: ${filePath}`) as NodeJS.ErrnoException;
err.code = 'ENOENT';
Expand All @@ -39,12 +81,25 @@ function createEnoentError(filePath: string): NodeJS.ErrnoException {
return err;
}
Comment thread
doudouOUC marked this conversation as resolved.
Resolved

function isPathWithinRoot(filePath: string, root: string): boolean {
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
if (!root.trim()) return false;

const relative = path.relative(path.resolve(root), path.resolve(filePath));
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
return (
relative === '' ||
(!relative.startsWith(`..${path.sep}`) &&
relative !== '..' &&
!path.isAbsolute(relative))
);
}

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(
Expand All @@ -67,7 +122,14 @@ export class AcpFileSystemService implements FileSystemService {
throw createEnoentError(params.path);
}

Comment thread
doudouOUC marked this conversation as resolved.
Resolved
throw error;
if (
getErrorKind(error) === PATH_OUTSIDE_WORKSPACE_KIND &&
this.isLocalReadFallbackPath(params.path)
) {
return this.fallback.readTextFile(params);
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
}

throw normalizeError(error);
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
}

return response;
Expand Down Expand Up @@ -97,4 +159,10 @@ export class AcpFileSystemService implements FileSystemService {
findFiles(fileName: string, searchPaths: readonly string[]): string[] {
return this.fallback.findFiles(fileName, searchPaths);
}

private isLocalReadFallbackPath(filePath: string): boolean {
return (this.options.localReadRoots ?? []).some((root) =>
isPathWithinRoot(filePath, root),
);
}
}
14 changes: 14 additions & 0 deletions packages/core/src/utils/errors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,20 @@ describe('getErrorMessage cause unwrapping', () => {
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('stringifies plain objects without a message', () => {
expect(getErrorMessage({ code: -32603 })).toBe('{"code":-32603}');
});
});

describe('isAbortError', () => {
Expand Down
11 changes: 11 additions & 0 deletions packages/core/src/utils/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,17 @@ export function getErrorMessage(error: unknown): string {
}
return error.message;
}
if (error !== null && typeof error === 'object') {
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
const message = (error as { message?: unknown }).message;
if (typeof message === 'string' && message.trim()) {
return message;
}
Comment thread
doudouOUC marked this conversation as resolved.
Resolved
try {
return JSON.stringify(error) ?? String(error);
} catch {
return String(error);
}
}
try {
return String(error);
} catch {
Expand Down
23 changes: 23 additions & 0 deletions packages/core/src/utils/fileUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,29 @@ 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 handle read errors for image/pdf files', async () => {
actualNodeFs.writeFileSync(testImageFilePath, 'content'); // File must exist
mockMimeGetType.mockReturnValue('image/png');
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/utils/fileUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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, '/');
Expand Down
Loading