Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
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