Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
44 changes: 43 additions & 1 deletion packages/cli/src/acp-integration/service/filesystem.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,39 @@ describe('AcpFileSystemService', () => {
});
});

it('converts core-only read params at the ACP boundary', async () => {
const mockResponse = {
content: 'slice',
_meta: { bom: false, encoding: 'utf-8' },
};
const client = {
readTextFile: vi.fn().mockResolvedValue(mockResponse),
} as unknown as AgentSideConnection;
const signal = new AbortController().signal;

const svc = new AcpFileSystemService(
client,
'session-1',
{ readTextFile: true, writeTextFile: true },
createFallback(),
);

await svc.readTextFile({
path: '/some/file.txt',
line: 0,
limit: 5,
maxOutputBytes: 1024,
signal,
});

expect(client.readTextFile).toHaveBeenCalledWith({
path: '/some/file.txt',
line: 1,
limit: 5,
sessionId: 'session-1',
});
});

it('converts RESOURCE_NOT_FOUND error to ENOENT', async () => {
const resourceNotFoundError = {
code: RESOURCE_NOT_FOUND_CODE,
Expand Down Expand Up @@ -953,11 +986,20 @@ describe('AcpFileSystemService', () => {
fallback,
);

const result = await svc.readTextFile({ path: '/some/file.txt' });
const signal = new AbortController().signal;
const result = await svc.readTextFile({
path: '/some/file.txt',
line: 0,
maxOutputBytes: 2048,
signal,
});

expect(result).toEqual(fallbackResponse);
expect(fallback.readTextFile).toHaveBeenCalledWith({
path: '/some/file.txt',
line: 0,
maxOutputBytes: 2048,
signal,
});
expect(client.readTextFile).not.toHaveBeenCalled();
});
Expand Down
33 changes: 28 additions & 5 deletions packages/cli/src/acp-integration/service/filesystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {
} from '@agentclientprotocol/sdk';
import { RequestError } from '@agentclientprotocol/sdk';
import type {
CoreReadTextFileRequest,
FileSystemService,
ReadTextFileResponse,
} from '@qwen-code/qwen-code-core';
Expand Down Expand Up @@ -105,6 +106,29 @@ async function resolveRealPath(value: string): Promise<string | undefined> {
}
}

function toAcpReadTextFileRequest(
Comment thread
doudouOUC marked this conversation as resolved.
params: CoreReadTextFileRequest,
sessionId: string,
): ReadTextFileRequest {
// `maxOutputBytes`, `signal`, and `stats` are core-local concerns that the
// current ACP schema cannot represent. Keep this boundary explicit if the
// schema grows.
const request: ReadTextFileRequest = {
path: params.path,
sessionId,
};
if (params._meta !== undefined) {
request._meta = params._meta;
}
if (params.limit !== undefined) {
request.limit = params.limit;
}
if (params.line != null) {
request.line = params.line + 1;
}
return request;
}

export class AcpFileSystemService implements FileSystemService {
constructor(
private readonly connection: AgentSideConnection,
Expand All @@ -115,18 +139,17 @@ export class AcpFileSystemService implements FileSystemService {
) {}

async readTextFile(
params: Omit<ReadTextFileRequest, 'sessionId'>,
params: CoreReadTextFileRequest,
): Promise<ReadTextFileResponse> {
if (!this.capabilities.readTextFile) {
return this.fallback.readTextFile(params);
}

let response: ReadTextFileResponse;
try {
response = await this.connection.readTextFile({
...params,
sessionId: this.sessionId,
});
response = await this.connection.readTextFile(
toAcpReadTextFileRequest(params, this.sessionId),
);
} catch (error) {
const errorCode = getErrorCode(error);

Expand Down
12 changes: 6 additions & 6 deletions packages/cli/src/serve/fs/policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ import type { Intent, ResolvedPath } from './paths.js';
* typical source files, small enough that an SSE replay buffer
* doesn't fill on a single read.
*
* Files **above** this cap are refused with `file_too_large` rather
* than truncated — the underlying `readFileWithLineAndLimit`
* reads the whole file into memory before slicing lines, so soft
* truncation past the cap would still OOM the daemon. Files
* **at or below** the cap honor a tighter `opts.maxBytes` via
* post-decode truncation (`enforceReadSize`); that's where the
* Full-snapshot reads above this cap are refused with `file_too_large`
* rather than truncated. `readText` can serve explicit line windows
* from larger files, but the default read/edit contract still needs a
* bounded snapshot for hash stability, SSE buffering, and oldText
* matching. Files at or below the cap honor a tighter `opts.maxBytes`
* via post-decode truncation (`enforceReadSize`); that's where the
* `meta.truncated = true` flag fires.
*
* `enforceReadBytesSize` (the `readBytes` gate) and `edit()` use the
Expand Down
14 changes: 14 additions & 0 deletions packages/cli/src/serve/fs/workspace-file-system.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,20 @@ describe('WorkspaceFileSystem - write/edit', () => {
expect(after).toBe('foo=42\nbar=2\n');
});

it('edit() preserves the tail of files larger than the default range cap', async () => {
const target = path.join(h.workspace, 'large-edit.txt');
const tail = 'tail-marker\n';
const content = `foo=1\n${'body\n'.repeat(6_000)}${tail}`;
await fsp.writeFile(target, content);
const r = await h.fs.resolve('large-edit.txt', 'edit');

const out = await h.fs.edit(r, 'foo=1', 'foo=42');

expect(out.writtenBytes).toBeGreaterThan(25_000);
const after = await fsp.readFile(target, 'utf-8');
expect(after).toBe(`foo=42\n${'body\n'.repeat(6_000)}${tail}`);
});

it('throws parse_error when oldText is not present', async () => {
const target = path.join(h.workspace, 'c.txt');
await fsp.writeFile(target, 'abc');
Expand Down
34 changes: 34 additions & 0 deletions packages/cli/src/ui/hooks/atCommandProcessor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,40 @@ describe('handleAtCommand', () => {
expect(result.toolDisplays![0].status).toBe(ToolCallStatus.Success);
});

it('should attach a truncated text file larger than 10MB', async () => {
const filePath = await createTestFile(
path.join(testRootDir, 'large.log'),
'x'.repeat(11 * 1024 * 1024),
);

const result = await handleAtCommand({
query: `@${filePath}`,
config: mockConfig,
onDebugMessage: mockOnDebugMessage,
messageId: 626,
signal: abortController.signal,
});

const processedText = Array.isArray(result.processedQuery)
? result.processedQuery
.map((part) =>
typeof part === 'string'
? part
: 'text' in part
? part.text
: JSON.stringify(part),
)
.join('')
: '';

expect(processedText).toContain(
'Showing lines 1-1 of at least 1 total lines',
);
expect(processedText).toContain('... [truncated]');
expect(result.shouldProceed).toBe(true);
expect(result.toolDisplays![0].status).toBe(ToolCallStatus.Success);
});

it('should only allow actual temp directory paths outside the workspace', async () => {
const tempParentDir = await fsPromises.mkdtemp(
path.join(os.tmpdir(), 'at-command-temp-'),
Expand Down
69 changes: 68 additions & 1 deletion packages/core/src/services/fileSystemService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ describe('StandardFileSystemService', () => {
expect(readFileWithLineAndLimit).toHaveBeenCalledWith({
path: '/test/file.txt',
limit: Infinity,
line: 0,
});
expect(result.content).toBe('Hello, World!');
expect(result._meta?.bom).toBe(false);
Expand Down Expand Up @@ -116,6 +115,74 @@ describe('StandardFileSystemService', () => {
expect(result._meta?.originalLineCount).toBe(100);
});

it('should preserve explicit line zero for offset reads', async () => {
vi.mocked(readFileWithLineAndLimit).mockResolvedValue({
content: 'line 1',
bom: false,
encoding: 'utf-8',
originalLineCount: 100,
});

await fileSystem.readTextFile({
path: '/test/file.txt',
line: 0,
});

expect(readFileWithLineAndLimit).toHaveBeenCalledWith({
path: '/test/file.txt',
limit: Infinity,
line: 0,
});
});

it('should pass maxOutputBytes and return byte-truncation metadata', async () => {
vi.mocked(readFileWithLineAndLimit).mockResolvedValue({
content: 'partial',
bom: false,
encoding: 'utf-8',
originalLineCount: 100,
truncatedByBytes: true,
});

const result = await fileSystem.readTextFile({
path: '/test/file.txt',
limit: 10,
line: 5,
maxOutputBytes: 128,
});

expect(readFileWithLineAndLimit).toHaveBeenCalledWith({
path: '/test/file.txt',
limit: 10,
line: 5,
maxOutputBytes: 128,
});
expect(result._meta?.truncatedByBytes).toBe(true);
});

it('should pass cached stats to readFileWithLineAndLimit', async () => {
const stats = { size: 123 } as import('node:fs').Stats;
vi.mocked(readFileWithLineAndLimit).mockResolvedValue({
content: 'line 1',
bom: false,
encoding: 'utf-8',
originalLineCount: 1,
});

await fileSystem.readTextFile({
path: '/test/file.txt',
maxOutputBytes: 128,
stats,
});

expect(readFileWithLineAndLimit).toHaveBeenCalledWith({
path: '/test/file.txt',
limit: Infinity,
maxOutputBytes: 128,
stats,
});
});

it('should return encoding info for GBK file', async () => {
vi.mocked(readFileWithLineAndLimit).mockResolvedValue({
content: '你好世界',
Expand Down
57 changes: 43 additions & 14 deletions packages/core/src/services/fileSystemService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import os from 'node:os';
import type { Stats } from 'node:fs';
import * as path from 'node:path';
import { globSync } from 'glob';
import { atomicWriteFile } from '../utils/atomicFileWrite.js';
Expand All @@ -29,10 +30,26 @@ export type ReadTextFileResponse = {
bom?: boolean;
encoding?: string;
originalLineCount?: number;
originalLineCountExact?: boolean;
lineEnding?: LineEnding;
truncatedByBytes?: boolean;
};
};

export type CoreReadTextFileRequest = Omit<
ReadTextFileRequest,
'sessionId' | 'line'
> & {
/**
* Core-local callers use 0-based line offsets. ACP protocol boundaries remain
* 1-based and convert explicitly before remote calls.
*/
line?: number | null;
maxOutputBytes?: number;
signal?: AbortSignal;
stats?: Stats;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking scope note: maxOutputBytes and signal are part of the large-text read behavior, but stats feels more like an internal optimization / TOCTOU cleanup than part of the public FileSystemService read contract.

Could we keep the main PR focused on the large-text range behavior and either avoid threading stats through this shared request type, or split that optimization into a follow-up? The current version is understandable, but it broadens the service API beyond the core bugfix.

};

/**
* Supported file encodings for new files.
*/
Expand All @@ -50,9 +67,7 @@ export type FileEncodingType = (typeof FileEncoding)[keyof typeof FileEncoding];
* Interface for file system operations that may be delegated to different implementations
*/
export interface FileSystemService {
readTextFile(
params: Omit<ReadTextFileRequest, 'sessionId'>,
): Promise<ReadTextFileResponse>;
readTextFile(params: CoreReadTextFileRequest): Promise<ReadTextFileResponse>;

writeTextFile(
params: Omit<WriteTextFileRequest, 'sessionId'>,
Expand Down Expand Up @@ -261,18 +276,32 @@ export function encodeTextFileContent(
*/
export class StandardFileSystemService implements FileSystemService {
async readTextFile(
params: Omit<ReadTextFileRequest, 'sessionId'>,
params: CoreReadTextFileRequest,
): Promise<ReadTextFileResponse> {
const { path, limit, line } = params;
// Use encoding-aware reader that handles BOM and non-UTF-8 encodings (e.g. GBK)
const { content, bom, encoding, originalLineCount } =
await readFileWithLineAndLimit({
path,
limit: limit ?? Number.POSITIVE_INFINITY,
line: line || 0,
});
const lineEnding = detectLineEnding(content);
return { content, _meta: { bom, encoding, originalLineCount, lineEnding } };
const { path, limit, line, maxOutputBytes, signal, stats } = params;
const readResult = await readFileWithLineAndLimit({
path,
limit: limit ?? Number.POSITIVE_INFINITY,
...(line !== undefined && line !== null ? { line } : {}),
...(maxOutputBytes !== undefined ? { maxOutputBytes } : {}),
...(signal !== undefined ? { signal } : {}),
...(stats !== undefined ? { stats } : {}),
});
const detectedLineEnding =
readResult.lineEnding ?? detectLineEnding(readResult.content);
return {
content: readResult.content,
_meta: {
bom: readResult.bom,
encoding: readResult.encoding,
originalLineCount: readResult.originalLineCount,
originalLineCountExact: readResult.originalLineCountExact,
lineEnding: detectedLineEnding,
...(readResult.truncatedByBytes !== undefined
? { truncatedByBytes: readResult.truncatedByBytes }
: {}),
},
};
}

async writeTextFile(
Expand Down
Loading
Loading