From 748c888a66ef4e3830da84ffb2ad4d2c606b533e Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Tue, 28 Jul 2026 23:40:54 +0800 Subject: [PATCH 01/16] fix(serve): allow bounded reads of large text files --- .../daemon/07-workspace-filesystem.md | 45 ++-- docs/developers/qwen-serve-protocol.md | 13 +- packages/acp-bridge/src/bridgeFileSystem.ts | 8 +- .../serve/bridge-file-system-adapter.test.ts | 24 +++ packages/cli/src/serve/fs/policy.ts | 15 +- .../serve/fs/workspace-file-system.test.ts | 197 +++++++++++++++++- .../cli/src/serve/fs/workspace-file-system.ts | 169 ++++++++++++++- .../serve/routes/workspace-file-read.test.ts | 26 +++ packages/core/src/index.ts | 1 + .../src/services/fileSystemService.test.ts | 49 +++++ .../core/src/services/fileSystemService.ts | 82 ++++++-- packages/core/src/utils/fileUtils.ts | 20 +- .../core/src/utils/read-text-range.test.ts | 70 +++++++ packages/core/src/utils/read-text-range.ts | 121 +++++++++-- 14 files changed, 749 insertions(+), 91 deletions(-) diff --git a/docs/developers/daemon/07-workspace-filesystem.md b/docs/developers/daemon/07-workspace-filesystem.md index 23543d1d9a5..0f21755a1c0 100644 --- a/docs/developers/daemon/07-workspace-filesystem.md +++ b/docs/developers/daemon/07-workspace-filesystem.md @@ -6,7 +6,7 @@ The daemon never lets HTTP routes or ACP-side agent calls touch the host filesys - **Path resolution** — canonicalize paths and reject anything escaping the bound workspace, including via symlinks. - **Trust gating** — refuse writes when the workspace is not trusted (`untrusted_workspace`). -- **Size & content policy** — read cap (`MAX_READ_BYTES = 256 KiB`), write cap (`MAX_WRITE_BYTES = 5 MiB`), binary detection. +- **Size & content policy** — full-snapshot/output cap (`MAX_READ_BYTES = 256 KiB`), bounded large-text windows, write cap (`MAX_WRITE_BYTES = 5 MiB`), binary detection. - **Atomicity** — write-then-rename with target mode preservation and `0o600` default for new files. - **Audit** — every access / denial emits a structured event for `PermissionAuditRing` / monitoring. - **Typed errors** — closed `FsErrorKind` union mapped to HTTP statuses. @@ -17,7 +17,7 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST - Resolve user-supplied paths into branded `ResolvedPath` values that the rest of the boundary can safely use. - Refuse paths outside the bound workspace (`path_outside_workspace`) and paths whose target is a symlink (`symlink_escape`). -- Refuse reads above `MAX_READ_BYTES`, writes above `MAX_WRITE_BYTES`, and binary files (`binary_file`). +- Refuse full-snapshot reads above `MAX_READ_BYTES`, while allowing finite line windows with output capped at `MAX_READ_BYTES`; refuse writes above `MAX_WRITE_BYTES` and binary files (`binary_file`). - Refuse writes/edits when the workspace is untrusted (`untrusted_workspace`) — gated by `assertTrustedForIntent(trusted, intent)`. - Honor `.gitignore` / `.qwenignore` patterns via `shouldIgnore`. - Perform atomic write-then-rename with target mode preservation; default new file mode is `0o600`. @@ -28,12 +28,12 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST ### Module layout -| File | Purpose | -| ------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `paths.ts` | `canonicalizeWorkspace`, `resolveWithinWorkspace`, `hasSuspiciousPathPattern`, branded `ResolvedPath`, `Intent` union (`read \| write \| list \| stat \| glob`). | -| `policy.ts` | `MAX_READ_BYTES`, `MAX_WRITE_BYTES`, `BINARY_PROBE_BYTES`, `assertTrustedForIntent`, `detectBinary`, `enforceReadBytesSize`, `enforceReadSize`, `enforceWriteSize`, `shouldIgnore`. | -| `audit.ts` | `FS_ACCESS_EVENT_TYPE`, `FS_DENIED_EVENT_TYPE`, `createAuditPublisher`, audit payload types. | -| `errors.ts` | `FsError` class, `isFsError`, `FsErrorKind` union (14 kinds), `FsErrorStatus` union (`400 / 403 / 404 / 409 / 413 / 422 / 500 / 503`). | +| File | Purpose | +| -------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `paths.ts` | `canonicalizeWorkspace`, `resolveWithinWorkspace`, `hasSuspiciousPathPattern`, branded `ResolvedPath`, `Intent` union (`read \| write \| list \| stat \| glob`). | +| `policy.ts` | `MAX_READ_BYTES`, `MAX_WRITE_BYTES`, `BINARY_PROBE_BYTES`, `assertTrustedForIntent`, `detectBinary`, `enforceReadBytesSize`, `enforceReadSize`, `enforceWriteSize`, `shouldIgnore`. | +| `audit.ts` | `FS_ACCESS_EVENT_TYPE`, `FS_DENIED_EVENT_TYPE`, `createAuditPublisher`, audit payload types. | +| `errors.ts` | `FsError` class, `isFsError`, `FsErrorKind` union (14 kinds), `FsErrorStatus` union (`400 / 403 / 404 / 409 / 413 / 422 / 500 / 503`). | | `workspace-file-system.ts` | `createWorkspaceFileSystemFactory`, `WorkspaceFileSystem` (the orchestrator that reads/writes/lists), `WriteMode`, `ContentHash`, `FsEntry`, `FsStat`, `ListOptions`, `GlobOptions`, `ReadTextOptions`, `ReadBytesOptions`, `WriteTextAtomicOptions`. | ### `FsErrorKind` taxonomy @@ -44,7 +44,7 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST | `symlink_escape` | 400 | Target is a symlink (rejected per the conservative PR 18 + PR 20 posture). | | `path_not_found` | 404 | `ENOENT`. | | `binary_file` | 422 | Content sniffed binary on a text route. | -| `file_too_large` | 413 | Above `MAX_READ_BYTES` or `MAX_WRITE_BYTES`. | +| `file_too_large` | 413 | Unbounded/full-snapshot text above `MAX_READ_BYTES`, unsupported large non-UTF-8 text, or a write above `MAX_WRITE_BYTES`. | | `hash_mismatch` | 409 | Optimistic-concurrency `expectedSha256` failed. | | `file_already_exists` | 409 | `mode: 'create'` against an existing file. | | `text_not_found` | 422 | `POST /file/edit`'s search string wasn't in the file. | @@ -139,15 +139,23 @@ sequenceDiagram FS->>FSP: stat(path) FSP-->>FS: stats FS->>FS: reject if not regular file (describeStatKind) - FS->>POL: enforceReadSize(stats.size, opts.maxBytes?)
→ throw file_too_large OR slice plan - FS->>FSP: readFile(path) - FSP-->>FS: buffer - FS->>POL: detectBinary(buffer) + alt file <= 256 KiB + FS->>FSP: open + read stable full snapshot + FSP-->>FS: buffer + FS->>FS: hash full snapshot; apply line/output limits + else file > 256 KiB AND finite limit + FS->>FSP: open stable FileHandle + FS->>FS: stream requested lines from the same inode + FS->>FS: cap output at 256 KiB; omit full-file hash + else unbounded large read + FS-->>R: file_too_large + end + FS->>POL: detectBinary(sample) POL-->>FS: isBinary? - FS->>FS: reject if binary; sha256 hash; truncate to line window + FS->>FS: reject if binary FS->>FS: shouldIgnore? → annotate meta.matchedIgnore FS->>FS: audit fs.access - FS-->>R: { content, sha256, truncated?, meta } + FS-->>R: { content, optional sha256, truncated?, meta } ``` `readText` does not skip or reject reads because of ignore rules. It reads the @@ -217,7 +225,7 @@ flowchart LR | Source | Knob | Effect | | ------------------------------------------------- | --------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------- | | `WorkspaceFileSystemFactoryDeps.trusted: boolean` | Constructor input | Whether writes are allowed; defaults to `true` from `runQwenServe`, `false` from `createServeApp` (with warning). | -| Constant | `MAX_READ_BYTES = 256 KiB` | Read cap; `file_too_large` past this. | +| Constant | `MAX_READ_BYTES = 256 KiB` | Full-snapshot and returned-text cap; larger text requires a finite line limit. | | Constant | `MAX_WRITE_BYTES = 5 MiB` | Write cap; sized below `express.json({ limit: '10mb' })`. | | Constant | `BINARY_PROBE_BYTES = 4096` | Sample size for content-based binary detection. | | Capability tags | `workspace_file_read`, `workspace_file_bytes`, `workspace_file_write` | See [`11-capabilities-versioning.md`](./11-capabilities-versioning.md). | @@ -229,8 +237,9 @@ flowchart LR - **`io_error` vs `permission_denied` are distinct.** Do not conflate them. Monitoring pipelines key on `errorKind` for alerting — folding ENOSPC into permission_denied would page security responders for `df -h` problems. - **New file mode defaults to `0o600`, not umask defaults.** The write syscall's `mode` arg bypasses umask. Agents writing public files should explicitly pass a mode override. - **`createServeApp` default `trusted: false`** silently rejects ACP writes with `untrusted_workspace` for embedders that do not inject a custom `fsFactory` or `bridge`. A one-time stderr warning fires the first time; further callers see no reminder. See [`02-serve-runtime.md`](./02-serve-runtime.md). -- **Read cap is enforced pre-decode.** A file at `MAX_READ_BYTES + 1` is refused even if the request only wants 10 lines — because the underlying `readFileWithLineAndLimit` reads the whole file into memory before slicing. -- **`BridgeFileSystem` adapter MUST replicate both inline-proxy gates** (non-regular-file refusal + buffered-size cap). The inline path is fully bypassed when the adapter is injected. +- **Large text requires a finite line limit.** No-limit reads, line-only reads, and maxBytes-only reads above `MAX_READ_BYTES` remain `file_too_large`. Finite windows stream from an inode-bound handle and never return more than `MAX_READ_BYTES`. +- **Large partial reads omit the full-file hash.** They retain the complete `sizeBytes`; `originalLineCount` is omitted when streaming stops before EOF. +- **`BridgeFileSystem` adapter MUST replicate both inline-proxy gates** (non-regular-file refusal + bounded buffering/streaming). The inline path is fully bypassed when the adapter is injected. ## References diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index d071838a0d7..386b63be338 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -1573,9 +1573,16 @@ Filesystem errors use this JSON shape: #### `GET /file` Reads a text file. Query params: `path` (required), `maxBytes`, `line`, and -`limit`. The daemon rejects binary files and files above the text read cap. -The response includes `hash`, a SHA-256 digest over the raw on-disk bytes for -the whole file, even when `line`, `limit`, or `maxBytes` returned a slice. +`limit`. The daemon rejects binary files. Files above the 256 KiB full-snapshot +cap require a finite `limit`; no-limit, line-only, and maxBytes-only requests +remain `file_too_large`. A finite large-file window is streamed and its returned +UTF-8 content remains capped at 256 KiB. + +For files within the full-snapshot cap, the response includes `hash`, a SHA-256 +digest over the raw on-disk bytes for the whole file, even when `line`, `limit`, +or `maxBytes` returned a slice. Large partial windows omit `hash`, retain the +complete `sizeBytes`, set `truncated: true`, and return +`originalLineCount: null` when the stream stops before EOF. ```json { diff --git a/packages/acp-bridge/src/bridgeFileSystem.ts b/packages/acp-bridge/src/bridgeFileSystem.ts index 951d27ce1ff..baca98051af 100644 --- a/packages/acp-bridge/src/bridgeFileSystem.ts +++ b/packages/acp-bridge/src/bridgeFileSystem.ts @@ -46,10 +46,10 @@ export interface BridgeFileSystem { * / procfs / sysfs entries can produce unbounded data on * read despite reporting `stats.size === 0`). Inline path * throws with `describeStatKind(stats)` in the message. - * 2. Cap the buffered size (the inline path uses - * `READ_FILE_SIZE_CAP = 100 MiB` to defend against a small - * `{ line: 1, limit: 10 }` request against a 500 MB log - * from costing 500 MB of RSS just to return 10 lines). + * 2. Bound memory and returned content. The inline fallback caps + * full buffering at `READ_FILE_SIZE_CAP = 100 MiB`; production + * adapters may instead stream finite line windows from larger + * files, but must retain a hard output cap. */ readText(params: ReadTextFileRequest): Promise; diff --git a/packages/cli/src/serve/bridge-file-system-adapter.test.ts b/packages/cli/src/serve/bridge-file-system-adapter.test.ts index 42abe9b568c..d4a7394e6ac 100644 --- a/packages/cli/src/serve/bridge-file-system-adapter.test.ts +++ b/packages/cli/src/serve/bridge-file-system-adapter.test.ts @@ -247,6 +247,30 @@ describe('createBridgeFileSystemAdapter', () => { expect(response.content).not.toContain('line1'); }); + it('reads a bounded ACP line window from text above MAX_READ_BYTES', async () => { + const { MAX_READ_BYTES } = await import('./fs/policy.js'); + const target = path.join(tmpDir, 'large-window.txt'); + const lines = Array.from( + { length: 4_000 }, + (_, index) => `line${index + 1} ${'x'.repeat(80)}`, + ); + const content = lines.join('\n'); + expect(Buffer.byteLength(content)).toBeGreaterThan(MAX_READ_BYTES); + await fsp.writeFile(target, content, 'utf8'); + + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + const response = await adapter.readText({ + path: target, + sessionId: 'sess:test', + line: 3, + limit: 20, + }); + + expect(response.content).toBe(lines.slice(2, 22).join('\n')); + }); + it('treats null line/limit as undefined (ACP wire compatibility)', async () => { const target = path.join(tmpDir, 'null-window.txt'); await fsp.writeFile(target, 'hello\nworld\n', 'utf8'); diff --git a/packages/cli/src/serve/fs/policy.ts b/packages/cli/src/serve/fs/policy.ts index 0061b6cd98f..71275c795dc 100644 --- a/packages/cli/src/serve/fs/policy.ts +++ b/packages/cli/src/serve/fs/policy.ts @@ -165,14 +165,13 @@ export interface ReadSizeOutcome { * * **Note**: this helper is the *soft* truncation gate that fires * when `opts.maxBytes < fileSize <= MAX_READ_BYTES`. It runs only - * AFTER `readText`'s pre-stat hard-cap check has rejected files - * above `MAX_READ_BYTES` with `file_too_large` (see - * `workspace-file-system.ts:readText`). Within the hard cap the - * caller can opt into a tighter byte ceiling via `opts.maxBytes`, - * and we surface that truncation via `truncated: true` rather - * than throwing — operators want to see a partial config file - * rather than an opaque error when they explicitly opted in to a - * smaller window. + * on the full-snapshot path: unbounded files above `MAX_READ_BYTES` + * are rejected before this helper, while finite line windows use + * the separate streaming path. Within the full-snapshot cap the caller + * can opt into a tighter byte ceiling via `opts.maxBytes`, and we + * surface that truncation via `truncated: true` rather than throwing — + * operators want to see a partial config file rather than an opaque + * error when they explicitly opted in to a smaller window. */ export function enforceReadSize( fileBytes: number, diff --git a/packages/cli/src/serve/fs/workspace-file-system.test.ts b/packages/cli/src/serve/fs/workspace-file-system.test.ts index 47811318389..b6803ede29c 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.test.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.test.ts @@ -9,7 +9,7 @@ import { promises as fsp } from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { createHash, randomBytes } from 'node:crypto'; -import { Ignore } from '@qwen-code/qwen-code-core'; +import { Ignore, StandardFileSystemService } from '@qwen-code/qwen-code-core'; import { FS_ACCESS_EVENT_TYPE, FS_DENIED_EVENT_TYPE, @@ -167,17 +167,16 @@ describe('WorkspaceFileSystem - readText', () => { expect(out.content.length).toBeLessThanOrEqual(1024); }); - it('throws file_too_large when file exceeds MAX_READ_BYTES regardless of opts.maxBytes', async () => { - // Write a file larger than the soft cap and assert the boundary - // refuses BEFORE delegating to lowFs (which would slurp the - // whole file into memory). + it('throws file_too_large for an oversized read without a finite line limit', async () => { const big = path.join(h.workspace, 'huge.txt'); const bytes = (await import('./policy.js')).MAX_READ_BYTES + 1; await fsp.writeFile(big, 'a'.repeat(bytes)); const r = await h.fs.resolve('huge.txt', 'read'); - const err = await h.fs.readText(r).catch((e: unknown) => e); - expect(isFsError(err)).toBe(true); - expect((err as { kind: string }).kind).toBe('file_too_large'); + for (const opts of [{}, { line: 2 }, { maxBytes: 1024 }]) { + const err = await h.fs.readText(r, opts).catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('file_too_large'); + } // Audit was recorded for the denial (P0 silent-failure fix). const denied = h.events.find((e) => e.type === FS_DENIED_EVENT_TYPE); expect(denied).toBeDefined(); @@ -186,6 +185,174 @@ describe('WorkspaceFileSystem - readText', () => { ); }); + it('streams bounded line windows from text above MAX_READ_BYTES', async () => { + const target = path.join(h.workspace, 'large-window.txt'); + const lines = Array.from( + { length: 100 }, + (_, index) => `line-${index + 1} ${'x'.repeat(80)}`, + ); + const maxReadBytes = (await import('./policy.js')).MAX_READ_BYTES; + const prefix = lines.join('\n'); + const filler = 'z'.repeat(maxReadBytes + 1 - Buffer.byteLength(prefix) - 1); + const allLines = [...lines, filler]; + const content = allLines.join('\n'); + expect(Buffer.byteLength(content)).toBe(maxReadBytes + 1); + await fsp.writeFile(target, content); + const r = await h.fs.resolve('large-window.txt', 'read'); + + const first = await h.fs.readText(r, { limit: 20 }); + expect(first.content).toBe(lines.slice(0, 20).join('\n')); + expect(first.meta.sizeBytes).toBe(Buffer.byteLength(content)); + expect(first.meta.truncated).toBe(true); + expect(first.meta.hash).toBeUndefined(); + expect(first.meta.originalLineCount).toBeUndefined(); + + const offset = await h.fs.readText(r, { line: 3, limit: 20 }); + expect(offset.content).toBe(lines.slice(2, 22).join('\n')); + expect(offset.meta.sizeBytes).toBe(Buffer.byteLength(content)); + expect(offset.meta.truncated).toBe(true); + expect(offset.meta.hash).toBeUndefined(); + + const beyondEof = await h.fs.readText(r, { + line: allLines.length + 10, + limit: 20, + }); + expect(beyondEof.content).toBe(''); + expect(beyondEof.meta.originalLineCount).toBe(allLines.length); + expect(beyondEof.meta.truncated).toBe(true); + expect(beyondEof.meta.hash).toBeUndefined(); + }); + + it('rejects oversized binary content even with a finite line limit', async () => { + const target = path.join(h.workspace, 'large-binary.dat'); + const bytes = (await import('./policy.js')).MAX_READ_BYTES + 1; + await fsp.writeFile(target, Buffer.alloc(bytes)); + const r = await h.fs.resolve('large-binary.dat', 'read'); + + const err = await h.fs.readText(r, { limit: 20 }).catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('binary_file'); + }); + + it('maps oversized non-UTF-8 text windows to file_too_large', async () => { + const target = path.join(h.workspace, 'large-utf16.txt'); + const body = Buffer.concat([ + Buffer.from([0xff, 0xfe]), + Buffer.from('中文日志行\n'.repeat(30_000), 'utf16le'), + ]); + const maxReadBytes = (await import('./policy.js')).MAX_READ_BYTES; + expect(body.length).toBeGreaterThan(maxReadBytes); + await fsp.writeFile(target, body); + const r = await h.fs.resolve('large-utf16.txt', 'read'); + + const err = await h.fs.readText(r, { limit: 20 }).catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('file_too_large'); + expect((err as { hint?: string }).hint).toMatch(/convert.*UTF-8/i); + }); + + it('byte-truncates an oversized multibyte line without replacement characters', async () => { + const target = path.join(h.workspace, 'large-unicode.txt'); + const content = '中文🙂'.repeat(40_000); + await fsp.writeFile(target, content, 'utf8'); + const r = await h.fs.resolve('large-unicode.txt', 'read'); + + const out = await h.fs.readText(r, { limit: 1, maxBytes: 7 }); + expect(out.content).toBe('中文'); + expect(out.content).not.toContain('\uFFFD'); + expect(out.meta.truncated).toBe(true); + expect(out.meta.hash).toBeUndefined(); + }); + + it('preserves BOM and CRLF metadata for an oversized bounded window', async () => { + const target = path.join(h.workspace, 'large-crlf.txt'); + const lines = Array.from( + { length: 8_000 }, + (_, index) => `row-${index + 1} ${'x'.repeat(30)}`, + ); + const body = Buffer.concat([ + Buffer.from([0xef, 0xbb, 0xbf]), + Buffer.from(lines.join('\r\n')), + ]); + await fsp.writeFile(target, body); + const r = await h.fs.resolve('large-crlf.txt', 'read'); + + const out = await h.fs.readText(r, { line: 2, limit: 2 }); + // Splitting on LF preserves the selected line's CR terminator, matching + // the existing full-snapshot range behavior. + expect(out.content).toBe(`${lines[1]}\r\n${lines[2]}\r`); + expect(out.meta.bom).toBe(true); + expect(out.meta.encoding).toBe('utf-8'); + expect(out.meta.lineEnding).toBe('crlf'); + expect(out.meta.hash).toBeUndefined(); + }); + + it('rejects a symlink swap while a large range is being read', async () => { + const target = path.join(h.workspace, 'large-swap.txt'); + const moved = path.join(h.workspace, 'large-swap.original.txt'); + const outside = path.join(h.scratch, 'outside.txt'); + const lines = Array.from( + { length: 4_000 }, + (_, index) => `line-${index + 1} ${'x'.repeat(80)}`, + ); + await fsp.writeFile(target, lines.join('\n')); + await fsp.writeFile(outside, 'replacement\n'); + const resolved = await h.fs.resolve('large-swap.txt', 'read'); + const original = StandardFileSystemService.prototype.readTextFileFromHandle; + const readSpy = vi + .spyOn(StandardFileSystemService.prototype, 'readTextFileFromHandle') + .mockImplementation(async function ( + this: StandardFileSystemService, + params, + ) { + const result = await original.call(this, params); + await fsp.rename(target, moved); + await fsp.symlink(outside, target, 'file'); + return result; + }); + + try { + const err = await h.fs + .readText(resolved, { limit: 20 }) + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('symlink_escape'); + } finally { + readSpy.mockRestore(); + } + }); + + it('rejects in-place changes while a large range is being read', async () => { + const target = path.join(h.workspace, 'large-change.txt'); + const lines = Array.from( + { length: 4_000 }, + (_, index) => `line-${index + 1} ${'x'.repeat(80)}`, + ); + await fsp.writeFile(target, lines.join('\n')); + const resolved = await h.fs.resolve('large-change.txt', 'read'); + const original = StandardFileSystemService.prototype.readTextFileFromHandle; + const readSpy = vi + .spyOn(StandardFileSystemService.prototype, 'readTextFileFromHandle') + .mockImplementation(async function ( + this: StandardFileSystemService, + params, + ) { + const result = await original.call(this, params); + await fsp.appendFile(target, '\nchanged'); + return result; + }); + + try { + const err = await h.fs + .readText(resolved, { limit: 20 }) + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('hash_mismatch'); + } finally { + readSpy.mockRestore(); + } + }); + it('throws binary_file when reading binary content', async () => { const bin = path.join(h.workspace, 'bin.dat'); const buf = Buffer.alloc(64); @@ -952,6 +1119,20 @@ describe('WorkspaceFileSystem - write/edit', () => { } }); + it('rejects opts.maxBytes outside the text boundary cap', async () => { + const target = path.join(h.workspace, 'max-bytes.txt'); + await fsp.writeFile(target, 'a\nb\n'); + const r = await h.fs.resolve('max-bytes.txt', 'read'); + const maxReadBytes = (await import('./policy.js')).MAX_READ_BYTES; + for (const bad of [0, -1, 1.5, NaN, maxReadBytes + 1]) { + const err = await h.fs + .readText(r, { maxBytes: bad }) + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('parse_error'); + } + }); + it('records matchedIgnore on edit() audit (parity with readText/writeText)', async () => { const ignore = new Ignore().add(['*.log']); h = await makeHarness({ ignore }); diff --git a/packages/cli/src/serve/fs/workspace-file-system.ts b/packages/cli/src/serve/fs/workspace-file-system.ts index 64507425676..e1b58382984 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.ts @@ -18,6 +18,7 @@ import { glob as globAsync } from 'glob'; // don't repeat the regression. import { + LargeNonUtf8TextError, StandardFileSystemService, decodeBufferWithEncodingInfoAsync, detectLineEnding, @@ -42,6 +43,7 @@ import { type ResolvedPath, } from './paths.js'; import { + BINARY_PROBE_BYTES, MAX_READ_BYTES, assertTrustedForIntent, enforceReadSize, @@ -84,14 +86,14 @@ export interface ReadMeta { } export interface ReadTextOptions { - /** Cap returned bytes; defaults to MAX_READ_BYTES. */ + /** Returned-byte cap in [1, MAX_READ_BYTES]; defaults to MAX_READ_BYTES. */ maxBytes?: number; /** * 1-based starting line for partial reads. `1` returns the file * from its first line. The boundary converts to the 0-based slice * index `readFileWithLineAndLimit` expects internally; SDK - * consumers don't need to adjust. Values < 1 (or undefined) are - * treated as "from the beginning". + * consumers don't need to adjust. Undefined starts from the + * beginning; non-positive or non-integral values are rejected. */ line?: number; /** Maximum number of lines to return. */ @@ -471,7 +473,18 @@ class WorkspaceFileSystemImpl implements WorkspaceFileSystem { `limit must be a positive integer, got ${opts.limit}`, ); } - const snapshot = await readTextSnapshotFromResolvedFile(p, opts); + if ( + opts.maxBytes !== undefined && + (!Number.isSafeInteger(opts.maxBytes) || + opts.maxBytes < 1 || + opts.maxBytes > MAX_READ_BYTES) + ) { + throw new FsError( + 'parse_error', + `maxBytes must be a positive integer in [1, ${MAX_READ_BYTES}], got ${opts.maxBytes}`, + ); + } + const snapshot = await readTextFromResolvedFile(p, opts, this.deps.lowFs); this.deps.generationGuard?.assertOpen(); const ignoreVerdict = this.ignoreVerdict(p, 'file'); const meta = snapshot.meta; @@ -1338,16 +1351,47 @@ function validateWriteTextAtomicOptions(opts: WriteTextAtomicOptions): void { } } -interface TextSnapshot { +interface TextReadOutcome { content: string; + meta: ReadMeta & { sizeBytes: number }; +} + +interface TextSnapshot extends TextReadOutcome { meta: ReadMeta & { hash: ContentHash; sizeBytes: number }; } +async function readTextFromResolvedFile( + p: ResolvedPath, + opts: ReadTextOptions, + lowFs: StandardFileSystemService, +): Promise { + const pre = await fsp.lstat(p as string); + if (pre.isSymbolicLink()) { + throw new FsError('symlink_escape', `path is a symlink: ${p}`, { + hint: 're-resolve the target file instead of reading through a link', + }); + } + if (!pre.isFile()) { + throw new FsError('parse_error', `path is not a regular file: ${p}`); + } + + if (pre.size > MAX_READ_BYTES && opts.limit !== undefined) { + return readLargeTextWindowFromResolvedFile( + p, + pre, + { ...opts, limit: opts.limit }, + lowFs, + ); + } + return readTextSnapshotFromResolvedFile(p, opts, pre); +} + async function readTextSnapshotFromResolvedFile( p: ResolvedPath, opts: ReadTextOptions = {}, + knownPre?: Awaited>, ): Promise { - const pre = await fsp.lstat(p as string); + const pre = knownPre ?? (await fsp.lstat(p as string)); if (pre.isSymbolicLink()) { throw new FsError('symlink_escape', `path is a symlink: ${p}`, { hint: 're-resolve the target file instead of reading through a link', @@ -1357,14 +1401,14 @@ async function readTextSnapshotFromResolvedFile( throw new FsError('parse_error', `path is not a regular file: ${p}`); } // Hard size gate before reading the full raw snapshot. Files above - // this cap should use `readBytesWindow()` with an explicit byte - // window instead of allocating a full decoded text snapshot. + // this cap need a finite text line limit or an explicit + // `readBytesWindow()` byte window instead of a full decoded snapshot. if (pre.size > MAX_READ_BYTES) { throw new FsError( 'file_too_large', `file of ${pre.size} bytes exceeds read cap of ${MAX_READ_BYTES} bytes`, { - hint: 'use readBytes for explicit byte-windowed access on large files', + hint: 'use a finite line limit for large UTF-8 text, or readBytes for explicit byte-windowed access', }, ); } @@ -1416,6 +1460,111 @@ async function readTextSnapshotFromResolvedFile( return { content, meta }; } +async function readLargeTextWindowFromResolvedFile( + p: ResolvedPath, + pre: Awaited>, + opts: ReadTextOptions & { limit: number }, + lowFs: StandardFileSystemService, +): Promise { + const fh = await fsp.open(p as string, 'r'); + let opened: Awaited> | undefined; + let content = ''; + let readMeta: + | Awaited< + ReturnType + >['_meta'] + | undefined; + try { + opened = await fh.stat(); + assertSameFile(pre, opened, p as string, 'read'); + if (opened.size !== pre.size || opened.mtimeMs !== pre.mtimeMs) { + throw new FsError('hash_mismatch', `file changed before read: ${p}`, { + hint: 'retry after re-reading the latest file', + }); + } + + const probe = Buffer.alloc(Math.min(BINARY_PROBE_BYTES, opened.size)); + if (probe.length > 0) { + const { bytesRead } = await fh.read(probe, 0, probe.length, 0); + if (looksBinary(probe.subarray(0, bytesRead))) { + throw new FsError('binary_file', `binary file: ${p}`, { + hint: 'use readBytes for binary content', + }); + } + } + + let result: Awaited< + ReturnType + >; + try { + result = await lowFs.readTextFileFromHandle({ + path: p as string, + fileHandle: fh, + stats: opened, + limit: opts.limit, + line: opts.line !== undefined ? opts.line - 1 : 0, + maxOutputBytes: opts.maxBytes ?? MAX_READ_BYTES, + }); + } catch (err) { + if (err instanceof LargeNonUtf8TextError) { + throw new FsError('file_too_large', err.message, { + cause: err, + hint: 'convert the file to UTF-8 before requesting a large line window', + }); + } + throw err; + } + content = result.content; + readMeta = result._meta; + + const afterRead = await fh.stat(); + assertSameFile(opened, afterRead, p as string, 'read'); + if ( + afterRead.size !== opened.size || + afterRead.mtimeMs !== opened.mtimeMs + ) { + throw new FsError('hash_mismatch', `file changed during read: ${p}`, { + hint: 'retry after re-reading the latest file', + }); + } + } finally { + await fh.close(); + } + + if (opened === undefined) { + throw new FsError('internal_error', `failed to stat opened file: ${p}`); + } + const post = await fsp.lstat(p as string); + if (post.isSymbolicLink()) { + throw new FsError( + 'symlink_escape', + `path was replaced with a symlink during read: ${p}`, + { hint: 'TOCTOU swap detected via post-read lstat' }, + ); + } + assertSameFile(opened, post, p as string, 'read'); + if (post.size !== opened.size || post.mtimeMs !== opened.mtimeMs) { + throw new FsError('hash_mismatch', `file changed during read: ${p}`, { + hint: 'retry after re-reading the latest file', + }); + } + + const meta: TextReadOutcome['meta'] = { + encoding: readMeta?.encoding, + bom: readMeta?.bom, + lineEnding: readMeta?.lineEnding ?? detectLineEnding(content), + sizeBytes: opened.size, + truncated: true, + }; + if ( + readMeta?.originalLineCountExact === true && + readMeta?.originalLineCount !== undefined + ) { + meta.originalLineCount = readMeta.originalLineCount; + } + return { content, meta }; +} + async function readStableRegularFileBuffer( p: string, pre: Awaited>, @@ -1430,7 +1579,7 @@ async function readStableRegularFileBuffer( 'file_too_large', `file of ${opened.size} bytes exceeds read cap of ${MAX_READ_BYTES} bytes`, { - hint: 'use readBytes for explicit byte-windowed access on large files', + hint: 'use a finite line limit for large UTF-8 text, or readBytes for explicit byte-windowed access', }, ); } diff --git a/packages/cli/src/serve/routes/workspace-file-read.test.ts b/packages/cli/src/serve/routes/workspace-file-read.test.ts index f14b81d1133..cd6e65eff30 100644 --- a/packages/cli/src/serve/routes/workspace-file-read.test.ts +++ b/packages/cli/src/serve/routes/workspace-file-read.test.ts @@ -146,6 +146,32 @@ describe('GET /file', () => { }); }); + it('returns a bounded line window for text above MAX_READ_BYTES', async () => { + const { MAX_READ_BYTES } = await import('../fs/policy.js'); + const lines = Array.from( + { length: 4_000 }, + (_, index) => `line-${index + 1} ${'x'.repeat(80)}`, + ); + const content = lines.join('\n'); + expect(Buffer.byteLength(content)).toBeGreaterThan(MAX_READ_BYTES); + await fsp.writeFile(path.join(h.workspace, 'large.txt'), content); + + const res = await request(h.app) + .get('/file?path=large.txt&line=3&limit=20') + .set('Host', loopbackHost()); + + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ + kind: 'file', + path: 'large.txt', + content: lines.slice(2, 22).join('\n'), + sizeBytes: Buffer.byteLength(content), + truncated: true, + originalLineCount: null, + }); + expect(res.body.hash).toBeUndefined(); + }); + it('attaches Cache-Control: no-store and X-Content-Type-Options: nosniff', async () => { await fsp.writeFile(path.join(h.workspace, 'a.txt'), 'x'); const res = await request(h.app) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index a06195d30f5..6254a2b6920 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -263,6 +263,7 @@ export { decodeBufferWithEncodingInfo, encodeTextFileContent, } from './utils/sync-file-encoding.js'; +export { LargeNonUtf8TextError } from './utils/read-text-range.js'; export * from './services/gitWorktreeService.js'; export { DEFAULT_MAX_TOOL_CALLS_PER_TURN } from './services/loopDetectionService.js'; export * from './services/visionBridge/vision-bridge-service.js'; diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index b682825c7ae..1e1c1bbdcba 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -183,6 +183,55 @@ describe('StandardFileSystemService', () => { }); }); + it('should force handle-bound reads through the streaming range path', async () => { + const fileHandle = {} as import('node:fs/promises').FileHandle; + const stats = { size: 300_000 } as import('node:fs').Stats; + vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ + content: 'line 2', + bom: false, + encoding: 'utf-8', + originalLineCount: 3, + originalLineCountExact: false, + truncatedByBytes: false, + }); + + const result = await fileSystem.readTextFileFromHandle({ + path: '/test/large.txt', + fileHandle, + stats, + limit: 1, + line: 1, + maxOutputBytes: 262_144, + }); + + expect(readFileWithLineAndLimit).toHaveBeenCalledWith({ + path: '/test/large.txt', + fileHandle, + forceStreaming: true, + stats, + limit: 1, + line: 1, + maxOutputBytes: 262_144, + }); + expect(result.content).toBe('line 2'); + expect(result._meta?.originalLineCountExact).toBe(false); + }); + + it('should reject unbounded handle reads', async () => { + const fileHandle = {} as import('node:fs/promises').FileHandle; + const stats = { size: 300_000 } as import('node:fs').Stats; + + await expect( + fileSystem.readTextFileFromHandle({ + path: '/test/large.txt', + fileHandle, + stats, + limit: Number.POSITIVE_INFINITY, + }), + ).rejects.toThrow(/positive finite limit/); + expect(readFileWithLineAndLimit).not.toHaveBeenCalled(); + }); + it('should return encoding info for GBK file', async () => { vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ content: '你好世界', diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index ab5860cd941..13e0000262e 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -6,6 +6,7 @@ import os from 'node:os'; import type { Stats } from 'node:fs'; +import type { FileHandle } from 'node:fs/promises'; import * as path from 'node:path'; import { globSync } from 'glob'; import { atomicWriteFile } from '../utils/atomicFileWrite.js'; @@ -47,6 +48,19 @@ export type CoreReadTextFileRequest = Omit< stats?: Stats; }; +/** + * Handle-bound range read used by filesystem security boundaries. The caller + * owns the handle lifecycle and must pass the Stats captured from that handle. + */ +export type CoreReadTextFileHandleRequest = Omit< + CoreReadTextFileRequest, + 'limit' | 'stats' +> & { + fileHandle: FileHandle; + stats: Stats; + limit: number; +}; + /** * Supported file encodings for new files. */ @@ -300,30 +314,21 @@ export class StandardFileSystemService implements FileSystemService { async readTextFile( params: CoreReadTextFileRequest, ): Promise { - 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 } : {}), + return readTextFileStandard(params); + } + + async readTextFileFromHandle( + params: CoreReadTextFileHandleRequest, + ): Promise { + if (!Number.isSafeInteger(params.limit) || params.limit < 1) { + throw new RangeError( + `handle-bound text reads require a positive finite limit, got ${params.limit}`, + ); + } + return readTextFileStandard(params, { + fileHandle: params.fileHandle, + forceStreaming: true, }); - 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( @@ -355,3 +360,34 @@ export class StandardFileSystemService implements FileSystemService { }); } } + +async function readTextFileStandard( + params: CoreReadTextFileRequest, + source?: { fileHandle: FileHandle; forceStreaming: boolean }, +): Promise { + 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 } : {}), + ...(source !== undefined ? source : {}), + }); + 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 } + : {}), + }, + }; +} diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 70a167fce76..915bb627093 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -320,6 +320,8 @@ export async function readFileWithLineAndLimit(params: { maxOutputBytes?: number; signal?: AbortSignal; stats?: import('node:fs').Stats; + fileHandle?: fs.promises.FileHandle; + forceStreaming?: boolean; }): Promise<{ content: string; bom?: boolean; @@ -329,8 +331,20 @@ export async function readFileWithLineAndLimit(params: { lineEnding?: 'crlf' | 'lf'; truncatedByBytes?: boolean; }> { - const { path: filePath, limit, line, maxOutputBytes, signal } = params; - const stats = params.stats ?? (await fs.promises.stat(filePath)); + const { + path: filePath, + limit, + line, + maxOutputBytes, + signal, + fileHandle, + forceStreaming, + } = params; + const stats = + params.stats ?? + (fileHandle !== undefined + ? await fileHandle.stat() + : await fs.promises.stat(filePath)); if ( (line !== undefined && line > 0) || Number.isFinite(limit) || @@ -343,6 +357,8 @@ export async function readFileWithLineAndLimit(params: { maxOutputBytes: normalizeRangeReadByteLimit(maxOutputBytes), stats, ...(signal !== undefined ? { signal } : {}), + ...(fileHandle !== undefined ? { fileHandle } : {}), + ...(forceStreaming !== undefined ? { forceStreaming } : {}), }); } diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index 2812045580b..17b1215b7c0 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -91,6 +91,76 @@ describe('readTextRange', () => { expect(result.truncatedByBytes).toBe(false); }); + it('streams a requested range from a caller-owned file handle', async () => { + const filePath = await writeFile('handle.log', largeUtf8Lines(2_000)); + const fileHandle = await fs.open(filePath, 'r'); + try { + const stats = await fileHandle.stat(); + const result = await readTextRange({ + path: filePath, + fileHandle, + stats, + forceStreaming: true, + offset: 1_500, + limit: 3, + maxOutputBytes: 10_000, + }); + + expect(result.content.split('\n')).toEqual([ + expect.stringContaining('line-1501'), + expect.stringContaining('line-1502'), + expect.stringContaining('line-1503'), + ]); + expect(result.originalLineCountExact).toBe(false); + + const beyondEof = await readTextRange({ + path: filePath, + fileHandle, + stats, + forceStreaming: true, + offset: 10_000, + limit: 3, + maxOutputBytes: 10_000, + }); + expect(beyondEof.content).toBe(''); + expect(beyondEof.originalLineCount).toBe(2_000); + expect(beyondEof.originalLineCountExact).toBe(true); + await expect(fileHandle.stat()).resolves.toMatchObject({ + size: stats.size, + }); + } finally { + await fileHandle.close(); + } + }); + + it('uses only the supplied file handle when the path names another file', async () => { + const originalPath = await writeFile( + 'original.log', + 'safe-one\nsafe-two\nsafe-three\n', + ); + const replacementPath = await writeFile( + 'replacement.log', + 'secret-one\nsecret-two\n', + ); + const fileHandle = await fs.open(originalPath, 'r'); + try { + const result = await readTextRange({ + path: replacementPath, + fileHandle, + stats: await fileHandle.stat(), + forceStreaming: true, + offset: 0, + limit: 2, + maxOutputBytes: 1_024, + }); + + expect(result.content).toBe('safe-one\nsafe-two'); + expect(result.content).not.toContain('secret'); + } finally { + await fileHandle.close(); + } + }); + it('streams a large UTF-8 file from the beginning when no range is provided', async () => { const filePath = await writeFile('large.log', largeUtf8Lines(65_000)); diff --git a/packages/core/src/utils/read-text-range.ts b/packages/core/src/utils/read-text-range.ts index bb89a6c6f83..f1100a8fee1 100644 --- a/packages/core/src/utils/read-text-range.ts +++ b/packages/core/src/utils/read-text-range.ts @@ -5,9 +5,13 @@ */ import { createReadStream, type Stats } from 'node:fs'; -import { stat } from 'node:fs/promises'; +import { stat, type FileHandle } from 'node:fs/promises'; import { TextDecoder } from 'node:util'; -import { detectFileEncoding, readFileWithEncodingInfo } from './fileUtils.js'; +import { + decodeBufferWithEncodingInfoAsync, + detectFileEncoding, + readFileWithEncodingInfo, +} from './fileUtils.js'; import { isUtf8CompatibleEncoding } from './encoding.js'; import { DEFAULT_RANGE_READ_BYTES, @@ -21,6 +25,13 @@ export interface ReadTextRangeRequest { maxOutputBytes: number; signal?: AbortSignal; stats?: Stats; + /** + * Optional caller-owned handle. When present, every read is bound to this + * already-open inode; this function never closes the handle. + */ + fileHandle?: FileHandle; + /** Skip the small-file full-buffer fast path. */ + forceStreaming?: boolean; } export interface ReadTextRangeResult { @@ -51,14 +62,27 @@ export async function readTextRange( request: ReadTextRangeRequest, ): Promise { request.signal?.throwIfAborted(); - const stats = request.stats ?? (await stat(request.path)); + const stats = + request.stats ?? + (request.fileHandle !== undefined + ? await request.fileHandle.stat() + : await stat(request.path)); const maxOutputBytes = normalizeMaxBytes(request.maxOutputBytes); - if (stats.size < TEXT_RANGE_FAST_PATH_MAX_SIZE) { - const { content, encoding, bom } = await readFileWithEncodingInfo( - request.path, - request.signal, - ); + if ( + request.forceStreaming !== true && + stats.size < TEXT_RANGE_FAST_PATH_MAX_SIZE + ) { + const { content, encoding, bom } = + request.fileHandle === undefined + ? await readFileWithEncodingInfo(request.path, request.signal) + : await decodeBufferWithEncodingInfoAsync( + await readFileHandleBuffer( + request.fileHandle, + stats.size, + request.signal, + ), + ); request.signal?.throwIfAborted(); const range = sliceDecodedContent( content, @@ -121,7 +145,10 @@ async function readLargeUtf8Range( request: ReadTextRangeRequest, maxOutputBytes: number, ): Promise { - const encoding = await detectFileEncoding(request.path); + const encoding = + request.fileHandle === undefined + ? await detectFileEncoding(request.path) + : await detectFileHandleEncoding(request.fileHandle, request.signal); if (!isUtf8CompatibleEncoding(encoding)) { throw new LargeNonUtf8TextError(encoding); } @@ -144,10 +171,15 @@ async function readLargeUtf8Range( ignoreBOM: true, }); - const stream = createReadStream(request.path, { - highWaterMark: 512 * 1024, - signal: request.signal, - }); + const pathStream = + request.fileHandle === undefined + ? createReadStream(request.path, { + highWaterMark: 512 * 1024, + signal: request.signal, + }) + : undefined; + const chunks = + pathStream ?? readFileHandleChunks(request.fileHandle!, request.signal); function appendSelected(fragment: string): void { if (fragment.length === 0 || truncatedByBytes) { @@ -184,7 +216,7 @@ async function readLargeUtf8Range( } try { - for await (const rawChunk of stream) { + for await (const rawChunk of chunks) { request.signal?.throwIfAborted(); let chunk = decodeUtf8Chunk(rawChunk as Buffer, { stream: true }); if (firstChunk) { @@ -232,7 +264,9 @@ async function readLargeUtf8Range( } } } finally { - stream.destroy(); + if (pathStream !== undefined && !pathStream.destroyed) { + pathStream.destroy(); + } } if (!stoppedEarly) { @@ -250,6 +284,63 @@ async function readLargeUtf8Range( }; } +async function readFileHandleBuffer( + fileHandle: FileHandle, + size: number, + signal?: AbortSignal, +): Promise { + const buffer = Buffer.alloc(size); + let offset = 0; + while (offset < size) { + signal?.throwIfAborted(); + const read = await fileHandle.read(buffer, offset, size - offset, offset); + if (read.bytesRead === 0) break; + offset += read.bytesRead; + } + signal?.throwIfAborted(); + return offset === buffer.length ? buffer : buffer.subarray(0, offset); +} + +async function* readFileHandleChunks( + fileHandle: FileHandle, + signal?: AbortSignal, +): AsyncGenerator { + const highWaterMark = 512 * 1024; + let position = 0; + while (true) { + signal?.throwIfAborted(); + const buffer = Buffer.allocUnsafe(highWaterMark); + const { bytesRead } = await fileHandle.read( + buffer, + 0, + buffer.length, + position, + ); + signal?.throwIfAborted(); + if (bytesRead === 0) return; + position += bytesRead; + yield buffer.subarray(0, bytesRead); + } +} + +async function detectFileHandleEncoding( + fileHandle: FileHandle, + signal?: AbortSignal, +): Promise { + signal?.throwIfAborted(); + const stats = await fileHandle.stat(); + if (stats.size === 0) return 'utf-8'; + + const sample = Buffer.alloc(Math.min(8192, stats.size)); + const { bytesRead } = await fileHandle.read(sample, 0, sample.length, 0); + signal?.throwIfAborted(); + if (bytesRead === 0) return 'utf-8'; + return ( + (await decodeBufferWithEncodingInfoAsync(sample.subarray(0, bytesRead))) + .encoding ?? 'utf-8' + ); +} + function truncateUtf8( content: string, maxBytes: number, From e784e6d43d6c2eea60fec6872edbd460f059b15c Mon Sep 17 00:00:00 2001 From: jinye Date: Wed, 29 Jul 2026 00:44:25 +0800 Subject: [PATCH 02/16] fix(serve): bound large-text reads by scan cost, not by which knob was set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the bounded large-text read path. Three changes: Gate on any explicit window argument, not on `limit`. Gating on `limit` had the cost model backwards in both directions: `{ line: 900_000_000, limit: 20 }` was admitted despite walking the whole file, while `{ maxBytes: 4096 }` — satisfiable from the first 4 KiB — was refused. A read with no window argument at all still fails, since a caller that believes it holds the whole file may write it back truncated. Add MAX_TEXT_SCAN_BYTES (8 MiB). MAX_READ_BYTES caps what a read returns; nothing capped what it cost. Line offsets are resolved by scanning from byte 0, so a query param could turn into an uninterruptible multi-second scan of an arbitrarily large file — and on Windows hold a read handle for that span, blocking renames and deletes. Past the budget the read is refused with `file_too_large` pointing at readBytes, which reaches any offset in O(1). Tolerate appends on streamed windows. Requiring whole-file size/mtime stability after reading a prefix rejected reads whose returned bytes were still valid, and the case it rejected — tailing a live log — is the one this path exists for. Streamed windows now assert inode identity plus "did not shrink"; truncation and replacement are still rejected. Also: non-UTF-8 large text now returns `binary_file` rather than `file_too_large`, so a client retrying on 413 with a smaller window can't loop forever; and `readFileWithLineAndLimit` throws instead of silently ignoring a caller-supplied `fileHandle` on the by-path fallback. Co-Authored-By: Claude Opus 5 --- .../daemon/07-workspace-filesystem.md | 25 ++-- docs/developers/qwen-serve-protocol.md | 13 +- packages/cli/src/serve/fs/policy.ts | 20 ++++ .../serve/fs/workspace-file-system.test.ts | 98 +++++++++++++-- .../cli/src/serve/fs/workspace-file-system.ts | 113 +++++++++++++----- packages/core/src/index.ts | 5 +- .../src/services/fileSystemService.test.ts | 45 ++++++- .../core/src/services/fileSystemService.ts | 41 ++++++- packages/core/src/utils/fileUtils.ts | 12 ++ .../core/src/utils/read-text-range.test.ts | 61 +++++++++- packages/core/src/utils/read-text-range.ts | 52 +++++++- 11 files changed, 421 insertions(+), 64 deletions(-) diff --git a/docs/developers/daemon/07-workspace-filesystem.md b/docs/developers/daemon/07-workspace-filesystem.md index 0f21755a1c0..b0c9d2e2317 100644 --- a/docs/developers/daemon/07-workspace-filesystem.md +++ b/docs/developers/daemon/07-workspace-filesystem.md @@ -6,7 +6,7 @@ The daemon never lets HTTP routes or ACP-side agent calls touch the host filesys - **Path resolution** — canonicalize paths and reject anything escaping the bound workspace, including via symlinks. - **Trust gating** — refuse writes when the workspace is not trusted (`untrusted_workspace`). -- **Size & content policy** — full-snapshot/output cap (`MAX_READ_BYTES = 256 KiB`), bounded large-text windows, write cap (`MAX_WRITE_BYTES = 5 MiB`), binary detection. +- **Size & content policy** — full-snapshot/output cap (`MAX_READ_BYTES = 256 KiB`), large-text windows bounded in both output and scan cost (`MAX_TEXT_SCAN_BYTES = 8 MiB`), write cap (`MAX_WRITE_BYTES = 5 MiB`), binary detection. - **Atomicity** — write-then-rename with target mode preservation and `0o600` default for new files. - **Audit** — every access / denial emits a structured event for `PermissionAuditRing` / monitoring. - **Typed errors** — closed `FsErrorKind` union mapped to HTTP statuses. @@ -17,7 +17,7 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST - Resolve user-supplied paths into branded `ResolvedPath` values that the rest of the boundary can safely use. - Refuse paths outside the bound workspace (`path_outside_workspace`) and paths whose target is a symlink (`symlink_escape`). -- Refuse full-snapshot reads above `MAX_READ_BYTES`, while allowing finite line windows with output capped at `MAX_READ_BYTES`; refuse writes above `MAX_WRITE_BYTES` and binary files (`binary_file`). +- Refuse full-snapshot reads above `MAX_READ_BYTES`, while allowing explicit windows with output capped at `MAX_READ_BYTES` and scan cost capped at `MAX_TEXT_SCAN_BYTES`; refuse writes above `MAX_WRITE_BYTES` and binary files (`binary_file`). - Refuse writes/edits when the workspace is untrusted (`untrusted_workspace`) — gated by `assertTrustedForIntent(trusted, intent)`. - Honor `.gitignore` / `.qwenignore` patterns via `shouldIgnore`. - Perform atomic write-then-rename with target mode preservation; default new file mode is `0o600`. @@ -31,7 +31,7 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST | File | Purpose | | -------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `paths.ts` | `canonicalizeWorkspace`, `resolveWithinWorkspace`, `hasSuspiciousPathPattern`, branded `ResolvedPath`, `Intent` union (`read \| write \| list \| stat \| glob`). | -| `policy.ts` | `MAX_READ_BYTES`, `MAX_WRITE_BYTES`, `BINARY_PROBE_BYTES`, `assertTrustedForIntent`, `detectBinary`, `enforceReadBytesSize`, `enforceReadSize`, `enforceWriteSize`, `shouldIgnore`. | +| `policy.ts` | `MAX_READ_BYTES`, `MAX_TEXT_SCAN_BYTES`, `MAX_WRITE_BYTES`, `BINARY_PROBE_BYTES`, `assertTrustedForIntent`, `detectBinary`, `enforceReadBytesSize`, `enforceReadSize`, `enforceWriteSize`, `shouldIgnore`. | | `audit.ts` | `FS_ACCESS_EVENT_TYPE`, `FS_DENIED_EVENT_TYPE`, `createAuditPublisher`, audit payload types. | | `errors.ts` | `FsError` class, `isFsError`, `FsErrorKind` union (14 kinds), `FsErrorStatus` union (`400 / 403 / 404 / 409 / 413 / 422 / 500 / 503`). | | `workspace-file-system.ts` | `createWorkspaceFileSystemFactory`, `WorkspaceFileSystem` (the orchestrator that reads/writes/lists), `WriteMode`, `ContentHash`, `FsEntry`, `FsStat`, `ListOptions`, `GlobOptions`, `ReadTextOptions`, `ReadBytesOptions`, `WriteTextAtomicOptions`. | @@ -43,8 +43,8 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST | `path_outside_workspace` | 400 | Resolved path is outside the bound workspace. | | `symlink_escape` | 400 | Target is a symlink (rejected per the conservative PR 18 + PR 20 posture). | | `path_not_found` | 404 | `ENOENT`. | -| `binary_file` | 422 | Content sniffed binary on a text route. | -| `file_too_large` | 413 | Unbounded/full-snapshot text above `MAX_READ_BYTES`, unsupported large non-UTF-8 text, or a write above `MAX_WRITE_BYTES`. | +| `binary_file` | 422 | Content sniffed binary on a text route, or large text in an encoding the text route cannot decode. | +| `file_too_large` | 413 | Windowless/full-snapshot text above `MAX_READ_BYTES`, a line offset beyond `MAX_TEXT_SCAN_BYTES`, or a write above `MAX_WRITE_BYTES`. | | `hash_mismatch` | 409 | Optimistic-concurrency `expectedSha256` failed. | | `file_already_exists` | 409 | `mode: 'create'` against an existing file. | | `text_not_found` | 422 | `POST /file/edit`'s search string wasn't in the file. | @@ -143,11 +143,11 @@ sequenceDiagram FS->>FSP: open + read stable full snapshot FSP-->>FS: buffer FS->>FS: hash full snapshot; apply line/output limits - else file > 256 KiB AND finite limit + else file > 256 KiB AND an explicit window arg FS->>FSP: open stable FileHandle FS->>FS: stream requested lines from the same inode - FS->>FS: cap output at 256 KiB; omit full-file hash - else unbounded large read + FS->>FS: cap output at 256 KiB and scan at 8 MiB; omit full-file hash + else windowless large read FS-->>R: file_too_large end FS->>POL: detectBinary(sample) @@ -225,7 +225,8 @@ flowchart LR | Source | Knob | Effect | | ------------------------------------------------- | --------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------- | | `WorkspaceFileSystemFactoryDeps.trusted: boolean` | Constructor input | Whether writes are allowed; defaults to `true` from `runQwenServe`, `false` from `createServeApp` (with warning). | -| Constant | `MAX_READ_BYTES = 256 KiB` | Full-snapshot and returned-text cap; larger text requires a finite line limit. | +| Constant | `MAX_READ_BYTES = 256 KiB` | Full-snapshot and returned-text cap; larger text requires an explicit window argument. | +| Constant | `MAX_TEXT_SCAN_BYTES = 8 MiB` | Bytes a large-text read may scan to locate a line offset; past it, `file_too_large`. | | Constant | `MAX_WRITE_BYTES = 5 MiB` | Write cap; sized below `express.json({ limit: '10mb' })`. | | Constant | `BINARY_PROBE_BYTES = 4096` | Sample size for content-based binary detection. | | Capability tags | `workspace_file_read`, `workspace_file_bytes`, `workspace_file_write` | See [`11-capabilities-versioning.md`](./11-capabilities-versioning.md). | @@ -237,8 +238,10 @@ flowchart LR - **`io_error` vs `permission_denied` are distinct.** Do not conflate them. Monitoring pipelines key on `errorKind` for alerting — folding ENOSPC into permission_denied would page security responders for `df -h` problems. - **New file mode defaults to `0o600`, not umask defaults.** The write syscall's `mode` arg bypasses umask. Agents writing public files should explicitly pass a mode override. - **`createServeApp` default `trusted: false`** silently rejects ACP writes with `untrusted_workspace` for embedders that do not inject a custom `fsFactory` or `bridge`. A one-time stderr warning fires the first time; further callers see no reminder. See [`02-serve-runtime.md`](./02-serve-runtime.md). -- **Large text requires a finite line limit.** No-limit reads, line-only reads, and maxBytes-only reads above `MAX_READ_BYTES` remain `file_too_large`. Finite windows stream from an inode-bound handle and never return more than `MAX_READ_BYTES`. -- **Large partial reads omit the full-file hash.** They retain the complete `sizeBytes`; `originalLineCount` is omitted when streaming stops before EOF. +- **Large text requires an explicit window argument**, any of `line` / `limit` / `maxBytes`. A read with none of them stays `file_too_large`, because a caller that believes it holds the whole file may write it back truncated. Windows stream from an inode-bound handle and never return more than `MAX_READ_BYTES`. +- **`MAX_READ_BYTES` caps what a read returns; `MAX_TEXT_SCAN_BYTES` caps what it costs.** Line offsets are resolved by scanning from byte 0, so `{ line: 900_000_000, limit: 20 }` returns almost nothing and still walks the file. Past 8 MiB of scanning the read is refused with `file_too_large` pointing at `readBytes`, which reaches any offset in O(1). +- **Streamed windows tolerate appends, not truncation.** The full-snapshot path can demand byte-for-byte stability because it returns the whole file; a prefix window cannot, or every read of a live log fails. The streamed path asserts inode identity plus "did not shrink", so appends pass and truncation / replacement are still rejected. `sizeBytes` reports the size at `open`, describing the snapshot the window was cut from. +- **Large partial reads omit the full-file hash.** `originalLineCount` is omitted when streaming stops before EOF. - **`BridgeFileSystem` adapter MUST replicate both inline-proxy gates** (non-regular-file refusal + bounded buffering/streaming). The inline path is fully bypassed when the adapter is injected. ## References diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 386b63be338..92534a5acfe 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -1574,9 +1574,16 @@ Filesystem errors use this JSON shape: Reads a text file. Query params: `path` (required), `maxBytes`, `line`, and `limit`. The daemon rejects binary files. Files above the 256 KiB full-snapshot -cap require a finite `limit`; no-limit, line-only, and maxBytes-only requests -remain `file_too_large`. A finite large-file window is streamed and its returned -UTF-8 content remains capped at 256 KiB. +cap require at least one explicit window argument (`line`, `limit`, or +`maxBytes`); a request with none of them remains `file_too_large`. Such a +window is streamed, and its returned UTF-8 content stays capped at 256 KiB. + +Line offsets are resolved by scanning from the start of the file, so a window +is also refused with `file_too_large` when reaching it would read more than +8 MiB (`MAX_TEXT_SCAN_BYTES`). Use `GET /file/bytes` to reach a deeper offset +directly. Large text in an encoding the route cannot decode returns +`binary_file`, not `file_too_large` — retrying with a smaller window cannot +help, and `readBytes` is the same remedy that already applies to binary. For files within the full-snapshot cap, the response includes `hash`, a SHA-256 digest over the raw on-disk bytes for the whole file, even when `line`, `limit`, diff --git a/packages/cli/src/serve/fs/policy.ts b/packages/cli/src/serve/fs/policy.ts index 71275c795dc..fe947129af8 100644 --- a/packages/cli/src/serve/fs/policy.ts +++ b/packages/cli/src/serve/fs/policy.ts @@ -32,6 +32,26 @@ import type { Intent, ResolvedPath } from './paths.js'; */ export const MAX_READ_BYTES = 256 * 1024; +/** + * Upper bound on bytes read off disk to locate a line window above + * `MAX_READ_BYTES`. + * + * `MAX_READ_BYTES` caps what a read *returns*; it says nothing about what a + * read *costs*. Line offsets address a byte stream, so `{ line: 900_000_000, + * limit: 20 }` returns almost nothing and still walks the file from byte 0. + * Without this cap a single query param turns into an uninterruptible + * multi-second scan of an arbitrarily large file, and on Windows it holds a + * read handle (opened without `FILE_SHARE_DELETE`) for that entire span, + * blocking renames and deletes of the target. + * + * 8 MiB is ~25 ms at the ~300 MB/s this streams at — small enough that the + * cost is bounded and the handle-hold window stays negligible, large enough + * to cover the head and tail-ish regions agents actually ask for. Requests + * past it get `file_too_large` pointing at `readBytes`, which reaches any + * offset in O(1). + */ +export const MAX_TEXT_SCAN_BYTES = 8 * 1024 * 1024; + /** * Maximum bytes accepted by `writeText` / `edit`. Sized below the * `express.json({ limit: '10mb' })` middleware cap so a request diff --git a/packages/cli/src/serve/fs/workspace-file-system.test.ts b/packages/cli/src/serve/fs/workspace-file-system.test.ts index b6803ede29c..05b3ad45deb 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.test.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.test.ts @@ -167,16 +167,14 @@ describe('WorkspaceFileSystem - readText', () => { expect(out.content.length).toBeLessThanOrEqual(1024); }); - it('throws file_too_large for an oversized read without a finite line limit', async () => { + it('throws file_too_large for an oversized read with no window argument', async () => { const big = path.join(h.workspace, 'huge.txt'); const bytes = (await import('./policy.js')).MAX_READ_BYTES + 1; await fsp.writeFile(big, 'a'.repeat(bytes)); const r = await h.fs.resolve('huge.txt', 'read'); - for (const opts of [{}, { line: 2 }, { maxBytes: 1024 }]) { - const err = await h.fs.readText(r, opts).catch((e: unknown) => e); - expect(isFsError(err)).toBe(true); - expect((err as { kind: string }).kind).toBe('file_too_large'); - } + const err = await h.fs.readText(r).catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('file_too_large'); // Audit was recorded for the denial (P0 silent-failure fix). const denied = h.events.find((e) => e.type === FS_DENIED_EVENT_TYPE); expect(denied).toBeDefined(); @@ -185,6 +183,49 @@ describe('WorkspaceFileSystem - readText', () => { ); }); + it('serves oversized text for any explicit window argument, not just limit', async () => { + // `maxBytes` and `line` bound the response just as much as `limit` does; + // refusing them while admitting a deep `line` had the cost model backwards. + const big = path.join(h.workspace, 'huge-window.txt'); + const maxReadBytes = (await import('./policy.js')).MAX_READ_BYTES; + const line = `${'a'.repeat(99)}\n`; + await fsp.writeFile(big, line.repeat(Math.ceil(maxReadBytes / 100) + 10)); + const r = await h.fs.resolve('huge-window.txt', 'read'); + + const capped = await h.fs.readText(r, { maxBytes: 1024 }); + expect(Buffer.byteLength(capped.content)).toBeLessThanOrEqual(1024); + expect(capped.meta.truncated).toBe(true); + expect(capped.meta.hash).toBeUndefined(); + + const fromLine = await h.fs.readText(r, { line: 2 }); + expect(fromLine.content.startsWith('a'.repeat(99))).toBe(true); + expect(Buffer.byteLength(fromLine.content)).toBeLessThanOrEqual( + maxReadBytes, + ); + expect(fromLine.meta.truncated).toBe(true); + }); + + it('refuses a line offset beyond MAX_TEXT_SCAN_BYTES', async () => { + const { MAX_TEXT_SCAN_BYTES } = await import('./policy.js'); + const big = path.join(h.workspace, 'deep-offset.txt'); + const line = `${'a'.repeat(99)}\n`; + const lineCount = Math.ceil((MAX_TEXT_SCAN_BYTES / 100) * 1.5); + await fsp.writeFile(big, line.repeat(lineCount)); + const r = await h.fs.resolve('deep-offset.txt', 'read'); + + // A shallow window on the same file is still cheap and still works. + const head = await h.fs.readText(r, { limit: 2 }); + expect(head.content.split('\n')).toHaveLength(2); + + // The deep one is refused rather than silently costing a full scan. + const err = await h.fs + .readText(r, { line: lineCount - 5, limit: 2 }) + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('file_too_large'); + expect((err as { hint?: string }).hint).toMatch(/readBytes/); + }); + it('streams bounded line windows from text above MAX_READ_BYTES', async () => { const target = path.join(h.workspace, 'large-window.txt'); const lines = Array.from( @@ -234,7 +275,7 @@ describe('WorkspaceFileSystem - readText', () => { expect((err as { kind: string }).kind).toBe('binary_file'); }); - it('maps oversized non-UTF-8 text windows to file_too_large', async () => { + it('maps oversized non-UTF-8 text windows to binary_file', async () => { const target = path.join(h.workspace, 'large-utf16.txt'); const body = Buffer.concat([ Buffer.from([0xff, 0xfe]), @@ -247,7 +288,10 @@ describe('WorkspaceFileSystem - readText', () => { const err = await h.fs.readText(r, { limit: 20 }).catch((e: unknown) => e); expect(isFsError(err)).toBe(true); - expect((err as { kind: string }).kind).toBe('file_too_large'); + // Not `file_too_large`: shrinking the window can never make a GBK file + // decodable, so a client retrying on 413 would loop forever. 422 with the + // readBytes hint is the same remedy that already works for binary. + expect((err as { kind: string }).kind).toBe('binary_file'); expect((err as { hint?: string }).hint).toMatch(/convert.*UTF-8/i); }); @@ -322,7 +366,7 @@ describe('WorkspaceFileSystem - readText', () => { } }); - it('rejects in-place changes while a large range is being read', async () => { + it('rejects a truncation while a large range is being read', async () => { const target = path.join(h.workspace, 'large-change.txt'); const lines = Array.from( { length: 4_000 }, @@ -338,7 +382,7 @@ describe('WorkspaceFileSystem - readText', () => { params, ) { const result = await original.call(this, params); - await fsp.appendFile(target, '\nchanged'); + await fsp.truncate(target, 1_000); return result; }); @@ -353,6 +397,40 @@ describe('WorkspaceFileSystem - readText', () => { } }); + it('serves a prefix window from a file being appended to during the read', async () => { + // The whole point of the feature: tailing a live log. A prefix window + // does not depend on the tail, so an append must not fail the read. + const target = path.join(h.workspace, 'large-append.txt'); + const lines = Array.from( + { length: 4_000 }, + (_, index) => `line-${index + 1} ${'x'.repeat(80)}`, + ); + await fsp.writeFile(target, lines.join('\n')); + const resolved = await h.fs.resolve('large-append.txt', 'read'); + const original = StandardFileSystemService.prototype.readTextFileFromHandle; + const sizeBefore = (await fsp.stat(target)).size; + const readSpy = vi + .spyOn(StandardFileSystemService.prototype, 'readTextFileFromHandle') + .mockImplementation(async function ( + this: StandardFileSystemService, + params, + ) { + const result = await original.call(this, params); + await fsp.appendFile(target, `\n${'appended '.repeat(50)}`); + return result; + }); + + try { + const out = await h.fs.readText(resolved, { limit: 20 }); + expect(out.content).toBe(lines.slice(0, 20).join('\n')); + // sizeBytes describes the snapshot the window was cut from, not the + // file as it stands after the concurrent append. + expect(out.meta.sizeBytes).toBe(sizeBefore); + } finally { + readSpy.mockRestore(); + } + }); + it('throws binary_file when reading binary content', async () => { const bin = path.join(h.workspace, 'bin.dat'); const buf = Buffer.alloc(64); diff --git a/packages/cli/src/serve/fs/workspace-file-system.ts b/packages/cli/src/serve/fs/workspace-file-system.ts index e1b58382984..bf7cff198de 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.ts @@ -20,6 +20,7 @@ import { glob as globAsync } from 'glob'; import { LargeNonUtf8TextError, StandardFileSystemService, + TextScanBudgetExceededError, decodeBufferWithEncodingInfoAsync, detectLineEnding, encodeTextFileContentAsync, @@ -45,6 +46,7 @@ import { import { BINARY_PROBE_BYTES, MAX_READ_BYTES, + MAX_TEXT_SCAN_BYTES, assertTrustedForIntent, enforceReadSize, enforceWriteSize, @@ -85,6 +87,13 @@ export interface ReadMeta { originalLineCount?: number; } +/** + * Above `MAX_READ_BYTES` at least one of these must be set. Any of them is + * the caller stating it accepts partial content, which is all the streamed + * path returns; with none of them the read is refused rather than silently + * handing back a truncated "whole file". Which one is set does not affect + * cost — that is bounded by `MAX_TEXT_SCAN_BYTES`. + */ export interface ReadTextOptions { /** Returned-byte cap in [1, MAX_READ_BYTES]; defaults to MAX_READ_BYTES. */ maxBytes?: number; @@ -1375,13 +1384,25 @@ async function readTextFromResolvedFile( throw new FsError('parse_error', `path is not a regular file: ${p}`); } - if (pre.size > MAX_READ_BYTES && opts.limit !== undefined) { - return readLargeTextWindowFromResolvedFile( - p, - pre, - { ...opts, limit: opts.limit }, - lowFs, - ); + // Any explicit window argument is the caller stating it accepts partial + // content, which is what the large-file path returns. Gating on `limit` + // alone got this backwards in both directions: `{ line: 900_000_000, + // limit: 20 }` was admitted despite costing a full scan, while + // `{ maxBytes: 4096 }` — satisfiable from the first 4 KiB — was refused. + // Cost is bounded by MAX_TEXT_SCAN_BYTES, not by which knob was set. + // + // A read with no window argument at all still fails: an agent that + // believes it holds the whole file may write it back truncated. The + // omitted `hash` blocks that for `editText`/`writeTextAtomic`, but + // `writeTextOverwrite` takes no hash, so `truncated: true` is the only + // signal on that path — refusing the unbounded read keeps the caller + // from ever being in that position by accident. + const wantsWindow = + opts.limit !== undefined || + opts.maxBytes !== undefined || + opts.line !== undefined; + if (pre.size > MAX_READ_BYTES && wantsWindow) { + return readLargeTextWindowFromResolvedFile(p, pre, opts, lowFs); } return readTextSnapshotFromResolvedFile(p, opts, pre); } @@ -1460,10 +1481,43 @@ async function readTextSnapshotFromResolvedFile( return { content, meta }; } +/** + * Stability check for a streamed *prefix* window. + * + * The full-snapshot path can demand byte-for-byte stability (`size` and + * `mtimeMs` unchanged) because it returns the whole file: any change + * invalidates the result. A line window does not return the whole file, so + * demanding whole-file stability rejects reads whose returned bytes are + * still perfectly valid — and the case it rejects is the one this feature + * exists for. Appending to a log does not change lines 1-20, but under an + * equality check every read of a live log is a coin flip. + * + * So the streamed path asserts only what the returned bytes depend on: the + * inode is unchanged (`assertSameFile`, checked separately) and the file did + * not shrink. Truncation, `>` rewrites, and replacement are still rejected. + * + * The residual gap is a writer that truncates *and* regrows past the + * original size inside one read window while keeping the same inode. That is + * narrower than what an equality check on `mtimeMs` catches, and it is the + * price of supporting append-only files at all. + */ +function assertDidNotShrink( + before: { size: number | bigint }, + after: { size: number | bigint }, + p: ResolvedPath, + reason: string, +): void { + if (toBigInt(after.size) < toBigInt(before.size)) { + throw new FsError('hash_mismatch', `${reason}: ${p}`, { + hint: 'retry after re-reading the latest file', + }); + } +} + async function readLargeTextWindowFromResolvedFile( p: ResolvedPath, pre: Awaited>, - opts: ReadTextOptions & { limit: number }, + opts: ReadTextOptions, lowFs: StandardFileSystemService, ): Promise { const fh = await fsp.open(p as string, 'r'); @@ -1477,11 +1531,7 @@ async function readLargeTextWindowFromResolvedFile( try { opened = await fh.stat(); assertSameFile(pre, opened, p as string, 'read'); - if (opened.size !== pre.size || opened.mtimeMs !== pre.mtimeMs) { - throw new FsError('hash_mismatch', `file changed before read: ${p}`, { - hint: 'retry after re-reading the latest file', - }); - } + assertDidNotShrink(pre, opened, p, 'file was truncated before read'); const probe = Buffer.alloc(Math.min(BINARY_PROBE_BYTES, opened.size)); if (probe.length > 0) { @@ -1501,15 +1551,30 @@ async function readLargeTextWindowFromResolvedFile( path: p as string, fileHandle: fh, stats: opened, - limit: opts.limit, + limit: opts.limit ?? Number.POSITIVE_INFINITY, line: opts.line !== undefined ? opts.line - 1 : 0, maxOutputBytes: opts.maxBytes ?? MAX_READ_BYTES, + maxScanBytes: MAX_TEXT_SCAN_BYTES, }); } catch (err) { + // An encoding the text route can't represent is the same class of + // refusal as sniffed-binary content, and `binary_file` already tells + // clients to fall back to `readBytes` — which is the right remedy for + // a GBK file too. `file_too_large` (413) would send a client that + // retries with a smaller window into a loop it can never exit. if (err instanceof LargeNonUtf8TextError) { + throw new FsError('binary_file', err.message, { + cause: err, + hint: 'convert the file to UTF-8, or use readBytes for the raw bytes', + }); + } + // Budget exhaustion *is* a size refusal: the file is too large to + // reach this line offset by scanning. 413 is correct, and the hint + // names the O(1) alternative so the retry has somewhere to go. + if (err instanceof TextScanBudgetExceededError) { throw new FsError('file_too_large', err.message, { cause: err, - hint: 'convert the file to UTF-8 before requesting a large line window', + hint: `line offsets are resolved by scanning from byte 0 and stop after ${MAX_TEXT_SCAN_BYTES} bytes; use readBytes to reach a deeper offset directly`, }); } throw err; @@ -1519,14 +1584,7 @@ async function readLargeTextWindowFromResolvedFile( const afterRead = await fh.stat(); assertSameFile(opened, afterRead, p as string, 'read'); - if ( - afterRead.size !== opened.size || - afterRead.mtimeMs !== opened.mtimeMs - ) { - throw new FsError('hash_mismatch', `file changed during read: ${p}`, { - hint: 'retry after re-reading the latest file', - }); - } + assertDidNotShrink(opened, afterRead, p, 'file was truncated during read'); } finally { await fh.close(); } @@ -1543,16 +1601,15 @@ async function readLargeTextWindowFromResolvedFile( ); } assertSameFile(opened, post, p as string, 'read'); - if (post.size !== opened.size || post.mtimeMs !== opened.mtimeMs) { - throw new FsError('hash_mismatch', `file changed during read: ${p}`, { - hint: 'retry after re-reading the latest file', - }); - } + assertDidNotShrink(opened, post, p, 'file was truncated during read'); const meta: TextReadOutcome['meta'] = { encoding: readMeta?.encoding, bom: readMeta?.bom, lineEnding: readMeta?.lineEnding ?? detectLineEnding(content), + // Size as of `open`, not as of now: it describes the snapshot the + // returned window was cut from. A file that grew during the read + // reports the smaller, consistent number. sizeBytes: opened.size, truncated: true, }; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 6254a2b6920..ec0196746ac 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -263,7 +263,10 @@ export { decodeBufferWithEncodingInfo, encodeTextFileContent, } from './utils/sync-file-encoding.js'; -export { LargeNonUtf8TextError } from './utils/read-text-range.js'; +export { + LargeNonUtf8TextError, + TextScanBudgetExceededError, +} from './utils/read-text-range.js'; export * from './services/gitWorktreeService.js'; export { DEFAULT_MAX_TOOL_CALLS_PER_TURN } from './services/loopDetectionService.js'; export * from './services/visionBridge/vision-bridge-service.js'; diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 1e1c1bbdcba..ca6481f7551 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -202,6 +202,7 @@ describe('StandardFileSystemService', () => { limit: 1, line: 1, maxOutputBytes: 262_144, + maxScanBytes: 8 * 1024 * 1024, }); expect(readFileWithLineAndLimit).toHaveBeenCalledWith({ @@ -212,12 +213,47 @@ describe('StandardFileSystemService', () => { limit: 1, line: 1, maxOutputBytes: 262_144, + maxScanBytes: 8 * 1024 * 1024, }); expect(result.content).toBe('line 2'); expect(result._meta?.originalLineCountExact).toBe(false); }); - it('should reject unbounded handle reads', async () => { + it('should allow an unbounded limit when both byte bounds are finite', async () => { + const fileHandle = {} as import('node:fs/promises').FileHandle; + const stats = { size: 300_000 } as import('node:fs').Stats; + vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ + content: 'head of file', + originalLineCount: 3, + originalLineCountExact: false, + truncatedByBytes: true, + }); + + const result = await fileSystem.readTextFileFromHandle({ + path: '/test/large.txt', + fileHandle, + stats, + limit: Number.POSITIVE_INFINITY, + maxOutputBytes: 262_144, + maxScanBytes: 8 * 1024 * 1024, + }); + + expect(result.content).toBe('head of file'); + expect(readFileWithLineAndLimit).toHaveBeenCalledWith( + expect.objectContaining({ + limit: Number.POSITIVE_INFINITY, + maxOutputBytes: 262_144, + maxScanBytes: 8 * 1024 * 1024, + }), + ); + }); + + it.each([ + ['maxOutputBytes', { maxOutputBytes: Number.POSITIVE_INFINITY }], + ['maxScanBytes', { maxScanBytes: Number.POSITIVE_INFINITY }], + ['maxOutputBytes', { maxOutputBytes: 0 }], + ['maxScanBytes', { maxScanBytes: -1 }], + ])('should reject a handle read with unbounded %s', async (bound, over) => { const fileHandle = {} as import('node:fs/promises').FileHandle; const stats = { size: 300_000 } as import('node:fs').Stats; @@ -226,9 +262,12 @@ describe('StandardFileSystemService', () => { path: '/test/large.txt', fileHandle, stats, - limit: Number.POSITIVE_INFINITY, + limit: 20, + maxOutputBytes: 262_144, + maxScanBytes: 8 * 1024 * 1024, + ...over, }), - ).rejects.toThrow(/positive finite limit/); + ).rejects.toThrow(new RegExp(`positive finite ${bound}`)); expect(readFileWithLineAndLimit).not.toHaveBeenCalled(); }); diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 13e0000262e..08bf8f8828c 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -51,14 +51,22 @@ export type CoreReadTextFileRequest = Omit< /** * Handle-bound range read used by filesystem security boundaries. The caller * owns the handle lifecycle and must pass the Stats captured from that handle. + * + * Both bounds are required rather than optional: what makes a large-file read + * safe at a boundary is that the *returned* bytes and the *scanned* bytes are + * each capped. A finite `limit` is not one of those bounds — `limit: 20` at + * `line: 900_000_000` still walks the whole file — so it stays optional and + * `maxScanBytes` is what actually keeps the read affordable. */ export type CoreReadTextFileHandleRequest = Omit< CoreReadTextFileRequest, - 'limit' | 'stats' + 'limit' | 'stats' | 'maxOutputBytes' > & { fileHandle: FileHandle; stats: Stats; - limit: number; + limit?: number; + maxOutputBytes: number; + maxScanBytes: number; }; /** @@ -320,14 +328,29 @@ export class StandardFileSystemService implements FileSystemService { async readTextFileFromHandle( params: CoreReadTextFileHandleRequest, ): Promise { - if (!Number.isSafeInteger(params.limit) || params.limit < 1) { + if (!isPositiveSafeInteger(params.maxOutputBytes)) { throw new RangeError( - `handle-bound text reads require a positive finite limit, got ${params.limit}`, + `handle-bound text reads require a positive finite maxOutputBytes, got ${params.maxOutputBytes}`, + ); + } + if (!isPositiveSafeInteger(params.maxScanBytes)) { + throw new RangeError( + `handle-bound text reads require a positive finite maxScanBytes, got ${params.maxScanBytes}`, + ); + } + if ( + params.limit !== undefined && + params.limit !== Number.POSITIVE_INFINITY && + !isPositiveSafeInteger(params.limit) + ) { + throw new RangeError( + `handle-bound text reads require a positive integer limit or Infinity, got ${params.limit}`, ); } return readTextFileStandard(params, { fileHandle: params.fileHandle, forceStreaming: true, + maxScanBytes: params.maxScanBytes, }); } @@ -361,9 +384,17 @@ export class StandardFileSystemService implements FileSystemService { } } +function isPositiveSafeInteger(value: unknown): value is number { + return typeof value === 'number' && Number.isSafeInteger(value) && value >= 1; +} + async function readTextFileStandard( params: CoreReadTextFileRequest, - source?: { fileHandle: FileHandle; forceStreaming: boolean }, + source?: { + fileHandle: FileHandle; + forceStreaming: boolean; + maxScanBytes: number; + }, ): Promise { const { path, limit, line, maxOutputBytes, signal, stats } = params; const readResult = await readFileWithLineAndLimit({ diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 915bb627093..aaa817a4d69 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -322,6 +322,7 @@ export async function readFileWithLineAndLimit(params: { stats?: import('node:fs').Stats; fileHandle?: fs.promises.FileHandle; forceStreaming?: boolean; + maxScanBytes?: number; }): Promise<{ content: string; bom?: boolean; @@ -339,6 +340,7 @@ export async function readFileWithLineAndLimit(params: { signal, fileHandle, forceStreaming, + maxScanBytes, } = params; const stats = params.stats ?? @@ -359,9 +361,19 @@ export async function readFileWithLineAndLimit(params: { ...(signal !== undefined ? { signal } : {}), ...(fileHandle !== undefined ? { fileHandle } : {}), ...(forceStreaming !== undefined ? { forceStreaming } : {}), + ...(maxScanBytes !== undefined ? { maxScanBytes } : {}), }); } + // Below this point the read is satisfied by path, not by descriptor. A + // caller that pinned an inode would silently get bytes from whatever the + // path resolves to now, defeating the reason it opened a handle. + if (fileHandle !== undefined) { + throw new RangeError( + 'readFileWithLineAndLimit cannot honor fileHandle for an unbounded read: pass a finite limit or maxOutputBytes', + ); + } + signal?.throwIfAborted(); const { content, encoding, bom } = await readFileWithEncodingInfo( diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index 17b1215b7c0..ea15cdc8c6c 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -9,7 +9,11 @@ import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; import { iconvEncode } from './iconvHelper.js'; -import { LargeNonUtf8TextError, readTextRange } from './read-text-range.js'; +import { + LargeNonUtf8TextError, + TextScanBudgetExceededError, + readTextRange, +} from './read-text-range.js'; describe('readTextRange', () => { let tempDir: string; @@ -161,6 +165,61 @@ describe('readTextRange', () => { } }); + it('refuses a line offset that cannot be reached within maxScanBytes', async () => { + const filePath = await writeFile('budget.log', largeUtf8Lines(5_000)); + + await expect( + readTextRange({ + path: filePath, + offset: 4_000, + limit: 20, + maxOutputBytes: 262_144, + maxScanBytes: 100_000, + }), + ).rejects.toBeInstanceOf(TextScanBudgetExceededError); + }); + + it('serves a shallow window from a file far larger than maxScanBytes', async () => { + const filePath = await writeFile('budget-head.log', largeUtf8Lines(5_000)); + + const result = await readTextRange({ + path: filePath, + offset: 0, + limit: 3, + maxOutputBytes: 262_144, + maxScanBytes: 100_000, + }); + + expect(result.content.split('\n')).toEqual([ + expect.stringContaining('line-1 '), + expect.stringContaining('line-2 '), + expect.stringContaining('line-3 '), + ]); + }); + + it('does not charge a budget failure to a file that ends within it', async () => { + // The scan reaches EOF on the same chunk that exhausts the budget; the + // window was fully satisfied, so there is nothing to refuse. + const body = largeUtf8Lines(100); + const filePath = await writeFile('budget-exact.log', body); + + const result = await readTextRange({ + path: filePath, + offset: 98, + limit: 10, + maxOutputBytes: 262_144, + maxScanBytes: Buffer.byteLength(body), + forceStreaming: true, + }); + + expect(result.content.split('\n')).toEqual([ + expect.stringContaining('line-99 '), + expect.stringContaining('line-100 '), + ]); + expect(result.originalLineCount).toBe(100); + expect(result.originalLineCountExact).toBe(true); + }); + it('streams a large UTF-8 file from the beginning when no range is provided', async () => { const filePath = await writeFile('large.log', largeUtf8Lines(65_000)); diff --git a/packages/core/src/utils/read-text-range.ts b/packages/core/src/utils/read-text-range.ts index f1100a8fee1..f431aef7d06 100644 --- a/packages/core/src/utils/read-text-range.ts +++ b/packages/core/src/utils/read-text-range.ts @@ -32,6 +32,14 @@ export interface ReadTextRangeRequest { fileHandle?: FileHandle; /** Skip the small-file full-buffer fast path. */ forceStreaming?: boolean; + /** + * Upper bound on bytes read off disk while locating the requested window. + * Line offsets address a byte stream, so a deep `offset` costs a scan from + * byte 0 — this is what keeps that scan from being unbounded. Defaults to + * `Infinity` so non-boundary callers (the `read_file` tool) are unchanged; + * security boundaries must pass a finite value. + */ + maxScanBytes?: number; } export interface ReadTextRangeResult { @@ -58,6 +66,23 @@ export class LargeNonUtf8TextError extends Error { } } +/** + * Raised when locating the requested line window would require reading more + * than `maxScanBytes`. Distinct from `LargeNonUtf8TextError`: the file is + * readable, the *offset* is what cannot be reached affordably. + */ +export class TextScanBudgetExceededError extends Error { + constructor( + readonly scannedBytes: number, + readonly maxScanBytes: number, + ) { + super( + `Locating the requested line window would read more than ${maxScanBytes} bytes (line offsets are resolved by scanning from the start of the file). Use a byte-offset read to reach this part of the file.`, + ); + this.name = 'TextScanBudgetExceededError'; + } +} + export async function readTextRange( request: ReadTextRangeRequest, ): Promise { @@ -68,10 +93,17 @@ export async function readTextRange( ? await request.fileHandle.stat() : await stat(request.path)); const maxOutputBytes = normalizeMaxBytes(request.maxOutputBytes); + const maxScanBytes = request.maxScanBytes ?? Number.POSITIVE_INFINITY; + // The fast path buffers the whole file, so it reads `stats.size` bytes no + // matter how small the window is — a budget that only constrained the + // streaming path would not be a budget. Falling through to streaming lets + // the same bound apply, and raises `TextScanBudgetExceededError` if the + // window really is out of reach. if ( request.forceStreaming !== true && - stats.size < TEXT_RANGE_FAST_PATH_MAX_SIZE + stats.size < TEXT_RANGE_FAST_PATH_MAX_SIZE && + stats.size <= maxScanBytes ) { const { content, encoding, bom } = request.fileHandle === undefined @@ -98,7 +130,7 @@ export async function readTextRange( }; } - return readLargeUtf8Range(request, maxOutputBytes); + return readLargeUtf8Range(request, maxOutputBytes, maxScanBytes); } function normalizeMaxBytes(maxOutputBytes: number): number { @@ -144,6 +176,7 @@ function sliceDecodedContent( async function readLargeUtf8Range( request: ReadTextRangeRequest, maxOutputBytes: number, + maxScanBytes: number, ): Promise { const encoding = request.fileHandle === undefined @@ -166,6 +199,8 @@ async function readLargeUtf8Range( let previousChunkEndedWithCR = false; let originalLineCountExact = true; let stoppedEarly = false; + let scannedBytes = 0; + let budgetExhausted = false; const decoder = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true, @@ -218,6 +253,15 @@ async function readLargeUtf8Range( try { for await (const rawChunk of chunks) { request.signal?.throwIfAborted(); + // Checked on arrival of the *next* chunk rather than after consuming + // the current one: reaching here at all proves there was more file to + // read, which is what separates "budget ran out mid-file" from "the + // file happened to end on the budget". Costs one chunk of overshoot. + if (scannedBytes >= maxScanBytes) { + budgetExhausted = true; + break; + } + scannedBytes += (rawChunk as Buffer).length; let chunk = decodeUtf8Chunk(rawChunk as Buffer, { stream: true }); if (firstChunk) { firstChunk = false; @@ -269,6 +313,10 @@ async function readLargeUtf8Range( } } + if (budgetExhausted) { + throw new TextScanBudgetExceededError(scannedBytes, maxScanBytes); + } + if (!stoppedEarly) { decodeUtf8Chunk(); } From f55c867affdb5d4937f4e6abd705f0f5468a5c44 Mon Sep 17 00:00:00 2001 From: jinye Date: Wed, 29 Jul 2026 02:07:21 +0800 Subject: [PATCH 03/16] refactor(core): thread the descriptor instead of forking text-read helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #7947 pinned large-text reads to one inode by threading a caller-owned FileHandle into readTextRange as an optional field, plus a second field, forceStreaming, to suppress the buffering fast path. Two optional fields produced four combinations: one meaningful, one used by a single test, one unreachable, and — in readFileWithLineAndLimit — one that silently fell through to a by-path read, defeating the reason the caller opened a handle. Unify the two encoding detectors. detectFileEncoding now takes a path or a borrowed handle, so detectFileHandleEncoding is deleted along with the message discrepancy between them: an encoding iconv-lite cannot load now raises LargeNonUtf8TextError naming that encoding rather than deferring to the decoder's generic invalid-utf8 variant. Both still refuse the file, and the Serve boundary maps both to binary_file. Split the reader into readTextRange (path) and readTextRangeFromHandle (always streams, both byte bounds required). The unreachable combination and its untested readFileHandleBuffer are gone, and with no fileHandle parameter left for readFileWithLineAndLimit to ignore, the RangeError guarding that fallthrough is deleted too — the trap can no longer be expressed. CoreReadTextFileHandleRequest drops its required stats field. Nothing downstream read it, and because the ACP request type it extends permits extra properties, TypeScript accepted the dead argument silently. readFileHandleChunks becomes chunksFromHandle(fh, from) — the one seam byte-cursor text paging needs. No observable change at the Serve boundary: its 222 tests pass unmodified. Two fileSystemService tests were deleted rather than repaired; they asserted the arguments readFileWithLineAndLimit received, which is nothing once the handle path stops calling it. Their coverage lives in read-text-range.test.ts against real files and in workspace-file-system.test.ts at the real boundary. 258 production lines in core, net -71 overall. Co-Authored-By: Claude Opus 5 --- ...026-07-29-handle-bound-text-range-reads.md | 164 ++++++++++++++++++ .../cli/src/serve/fs/workspace-file-system.ts | 1 - .../src/services/fileSystemService.test.ts | 91 +++------- .../core/src/services/fileSystemService.ts | 30 ++-- packages/core/src/utils/fileUtils.ts | 70 ++------ .../core/src/utils/read-text-range.test.ts | 43 +++-- packages/core/src/utils/read-text-range.ts | 158 +++++++++-------- 7 files changed, 325 insertions(+), 232 deletions(-) create mode 100644 docs/design/2026-07-29-handle-bound-text-range-reads.md diff --git a/docs/design/2026-07-29-handle-bound-text-range-reads.md b/docs/design/2026-07-29-handle-bound-text-range-reads.md new file mode 100644 index 00000000000..9a844003d2c --- /dev/null +++ b/docs/design/2026-07-29-handle-bound-text-range-reads.md @@ -0,0 +1,164 @@ +# Handle-Bound Text Range Reads + +## Context + +PR #7947 let the Serve workspace filesystem return bounded line windows from +text files above `MAX_READ_BYTES` (256 KiB). To keep those reads pinned to one +inode across validation, binary probing, and streaming, it threaded a +caller-owned `FileHandle` down into `readTextRange` as an optional field, and +added a second optional field, `forceStreaming`, to suppress the buffering fast +path that would otherwise defeat the memory bound. + +Two optional fields on one entry point produced four combinations, of which one +is meaningful, one is unreachable, and one is unsafe: + +| `fileHandle` | `forceStreaming` | Result | +| ------------ | ---------------- | ---------------------------------------------------------------------- | +| unset | unset | ordinary path read | +| unset | set | streams a small file — used by one test | +| set | set | the Serve boundary's read | +| set | unset | buffers the whole file through the handle — **no caller can reach it** | + +The unreachable combination carried a dedicated helper, `readFileHandleBuffer`, +with no test coverage. Separately, `readFileWithLineAndLimit` accepted the same +`fileHandle` but could only honor it on its range branch: an unbounded read fell +through to a by-path `readFileWithEncodingInfo`, silently returning bytes from +whatever the path resolved to at that moment rather than from the pinned inode. +PR #7947's follow-up commit guarded that with a runtime `RangeError`, which +documented the trap without removing it. + +Encoding detection had forked for the same reason. `detectFileEncoding` takes a +path and opens its own descriptor, so the handle path could not use it; a +private `detectFileHandleEncoding` was added alongside, deriving the encoding +name from `decodeBufferWithEncodingInfoAsync(...).encoding` instead of from +chardet directly. The two disagree when chardet names an encoding `iconv-lite` +cannot load: the path variant returns that name, the handle variant returns +`'utf-8'` and defers to the streaming decoder's `fatal: true` failure. Both +refuse the file, with different messages. + +## Goals + +- One encoding detector, usable from a path or a borrowed descriptor. +- No mode flags on the range reader; make the unreachable combination + unrepresentable rather than merely unused. +- Make the by-path fallthrough structurally impossible instead of guarded. +- No observable change at the Serve boundary or in the `read_file` tool. + +## Non-goals + +- Collapsing `decodeBufferWithEncodingInfo` (sync) into its async twin. The sync + variant is a deliberate public-API compatibility shim + ([`lazy-first-use-dependencies.md`](./lazy-first-use-dependencies.md)) pinned + by a parity test. +- Any change to what the Serve boundary returns. This is preparation for + byte-cursor paging, not that feature. + +## Design + +### One detector + +`detectFileEncoding(source: string | FileHandle)`. A supplied handle is +_borrowed_: reads use explicit positions so the caller's file position is +untouched, and the `finally` block closes only a descriptor this function +itself opened. `detectFileHandleEncoding` is deleted, and the open-coded +BOM-to-name switch is replaced with the existing `bomEncodingToName`. + +This makes the handle path slightly stricter, which is the intended direction: +an encoding `iconv-lite` cannot load now raises +`LargeNonUtf8TextError(detected)` naming that encoding, rather than reaching the +decoder and raising the generic `'invalid-utf8'` variant. The refusal is +unchanged; the message improves. The Serve boundary maps both to `binary_file`, +so nothing downstream moves. + +### Two entry points + +```ts +readTextRange(request: ReadTextRangeRequest) // path +readTextRangeFromHandle(fh, request: ReadTextRangeFromHandleRequest) +``` + +The handle variant always streams — there is no flag, because a caller reaches +for a handle precisely when it needs the read bounded, and the buffering fast +path would read the whole file. Its request type has no `path` (nothing for one +to disambiguate) and makes both byte bounds required rather than optional: +`maxOutputBytes` caps what the read returns, `maxScanBytes` caps what it costs, +and a handle-bound read exists because a security boundary needs both. + +`maxScanBytes` stays optional on the path variant, where it defaults to +`Infinity` so the `read_file` tool is unchanged. + +Both delegate to the same streaming implementation, which now takes +`source: string | FileHandle` and selects `createReadStream` or +`chunksFromHandle` accordingly. `readFileHandleBuffer` and the branch that +called it are deleted. + +### The fallthrough disappears + +`readFileWithLineAndLimit` loses `fileHandle`, `forceStreaming`, and +`maxScanBytes` — its single production caller passes none of them. +`StandardFileSystemService.readTextFileFromHandle` now calls +`readTextRangeFromHandle` directly, and the two read paths share a +`toReadTextFileResponse` helper so their metadata shaping cannot drift. With no +`fileHandle` parameter left to ignore, the `RangeError` guard is removed: the +trap it described can no longer be expressed. + +`readTextFileFromHandle` stays off the `FileSystemService` interface, so +`AcpFileSystemService` and the typed fallback mock in `filesystem.test.ts` are +untouched. + +## Blast radius + +- `readTextRange` is not exported from `packages/core/src/index.ts`; only the + two error classes are. The reshaped surface is core-internal. +- `readTextRange` and `readFileWithLineAndLimit` have exactly one production + caller each (`fileUtils.ts`, `fileSystemService.ts`). +- `detectFileEncoding` is public via `export * from './utils/fileUtils.js'`. + Widening a parameter is source-compatible. +- The only cross-package importer of the touched modules is + `packages/cli/src/serve/fs/workspace-file-system.ts`. Its one change is + deleting a `stats:` argument that the handle path no longer accepts — see + below; the `decodeBufferWithEncodingInfoAsync` import it also carries is + untouched. + +`CoreReadTextFileHandleRequest` drops its required `stats` field. The handle +path always streams and the encoding probe does its own `fstat`, so nothing +downstream ever read it — and because the ACP request type it extends permits +extra properties, TypeScript accepted the dead argument silently rather than +flagging it. Removing the field is what surfaced the one CLI line. + +258 production logic lines change in `packages/core`, net −71 lines overall. + +## Testing + +The existing suites are the specification: the whole point is that the Serve +boundary cannot tell. `packages/cli/src/serve/fs/` and the bridge adapter — 222 +tests — pass unmodified, as does the full `packages/core` `src/utils` + +`src/services` run (6078 tests). + +Three tests in `read-text-range.test.ts` moved to `readTextRangeFromHandle`. Two +used `fileHandle` directly. The third used a _path_ with `forceStreaming: true` +to force streaming on a file too small to leave the fast path, so that it could +exercise the budget-at-EOF boundary; with the flag gone, the handle variant is +the only thing that always streams. + +One of the moved tests changed meaning. It previously passed a handle for one +file and a path naming a different file, asserting the handle won — a test for +the confusion the old signature permitted. The handle variant has no `path`, so +that confusion is now unrepresentable and the test would assert nothing. It was +rewritten to cover the property that actually motivated the API: open a handle, +rename another file over the path, and confirm the read still follows the inode. + +Two tests in `fileSystemService.test.ts` were deleted rather than repaired. They +mocked `readFileWithLineAndLimit` and asserted the argument object it received; +since `readTextFileFromHandle` no longer calls it, they could only have been +kept by re-pointing them at a new mock, which would again assert only that one +function passes arguments to another. The behaviour they nominally covered is +tested against real files in `read-text-range.test.ts` and at the real boundary +in `workspace-file-system.test.ts`. The argument-validation tests beside them +are kept — they need no mock. + +## Follow-up + +`chunksFromHandle` gained a `from` parameter that nothing yet passes a non-zero +value for. It is the single seam byte-cursor text paging needs, and that is the +change this refactor exists to enable. diff --git a/packages/cli/src/serve/fs/workspace-file-system.ts b/packages/cli/src/serve/fs/workspace-file-system.ts index bf7cff198de..1b5adf016a0 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.ts @@ -1550,7 +1550,6 @@ async function readLargeTextWindowFromResolvedFile( result = await lowFs.readTextFileFromHandle({ path: p as string, fileHandle: fh, - stats: opened, limit: opts.limit ?? Number.POSITIVE_INFINITY, line: opts.line !== undefined ? opts.line - 1 : 0, maxOutputBytes: opts.maxBytes ?? MAX_READ_BYTES, diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index ca6481f7551..632ffddd492 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -183,71 +183,11 @@ describe('StandardFileSystemService', () => { }); }); - it('should force handle-bound reads through the streaming range path', async () => { - const fileHandle = {} as import('node:fs/promises').FileHandle; - const stats = { size: 300_000 } as import('node:fs').Stats; - vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ - content: 'line 2', - bom: false, - encoding: 'utf-8', - originalLineCount: 3, - originalLineCountExact: false, - truncatedByBytes: false, - }); - - const result = await fileSystem.readTextFileFromHandle({ - path: '/test/large.txt', - fileHandle, - stats, - limit: 1, - line: 1, - maxOutputBytes: 262_144, - maxScanBytes: 8 * 1024 * 1024, - }); - - expect(readFileWithLineAndLimit).toHaveBeenCalledWith({ - path: '/test/large.txt', - fileHandle, - forceStreaming: true, - stats, - limit: 1, - line: 1, - maxOutputBytes: 262_144, - maxScanBytes: 8 * 1024 * 1024, - }); - expect(result.content).toBe('line 2'); - expect(result._meta?.originalLineCountExact).toBe(false); - }); - - it('should allow an unbounded limit when both byte bounds are finite', async () => { - const fileHandle = {} as import('node:fs/promises').FileHandle; - const stats = { size: 300_000 } as import('node:fs').Stats; - vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ - content: 'head of file', - originalLineCount: 3, - originalLineCountExact: false, - truncatedByBytes: true, - }); - - const result = await fileSystem.readTextFileFromHandle({ - path: '/test/large.txt', - fileHandle, - stats, - limit: Number.POSITIVE_INFINITY, - maxOutputBytes: 262_144, - maxScanBytes: 8 * 1024 * 1024, - }); - - expect(result.content).toBe('head of file'); - expect(readFileWithLineAndLimit).toHaveBeenCalledWith( - expect.objectContaining({ - limit: Number.POSITIVE_INFINITY, - maxOutputBytes: 262_144, - maxScanBytes: 8 * 1024 * 1024, - }), - ); - }); - + // Handle-bound reads no longer route through `readFileWithLineAndLimit`, + // so asserting the arguments it was called with would test nothing. The + // behaviour is covered against real files in `read-text-range.test.ts` + // and at the real boundary in `workspace-file-system.test.ts`; only the + // argument validation below needs a unit test, and it needs no mock. it.each([ ['maxOutputBytes', { maxOutputBytes: Number.POSITIVE_INFINITY }], ['maxScanBytes', { maxScanBytes: Number.POSITIVE_INFINITY }], @@ -255,20 +195,35 @@ describe('StandardFileSystemService', () => { ['maxScanBytes', { maxScanBytes: -1 }], ])('should reject a handle read with unbounded %s', async (bound, over) => { const fileHandle = {} as import('node:fs/promises').FileHandle; - const stats = { size: 300_000 } as import('node:fs').Stats; await expect( fileSystem.readTextFileFromHandle({ path: '/test/large.txt', fileHandle, - stats, limit: 20, maxOutputBytes: 262_144, maxScanBytes: 8 * 1024 * 1024, ...over, }), ).rejects.toThrow(new RegExp(`positive finite ${bound}`)); - expect(readFileWithLineAndLimit).not.toHaveBeenCalled(); + }); + + it.each([ + ['a fractional limit', 2.5], + ['a zero limit', 0], + ['a negative limit', -1], + ])('should reject %s on a handle read', async (_label, limit) => { + const fileHandle = {} as import('node:fs/promises').FileHandle; + + await expect( + fileSystem.readTextFileFromHandle({ + path: '/test/large.txt', + fileHandle, + limit, + maxOutputBytes: 262_144, + maxScanBytes: 8 * 1024 * 1024, + }), + ).rejects.toThrow(/positive integer limit or Infinity/); }); it('should return encoding info for GBK file', async () => { diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 08bf8f8828c..9ba6146c5e0 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -11,6 +11,7 @@ import * as path from 'node:path'; import { globSync } from 'glob'; import { atomicWriteFile } from '../utils/atomicFileWrite.js'; import { readFileWithLineAndLimit } from '../utils/fileUtils.js'; +import { readTextRangeFromHandle } from '../utils/read-text-range.js'; import { isUtf8CompatibleEncoding } from '../utils/encoding.js'; import { loadIconvLite, type IconvLite } from '../utils/load-iconv-lite.js'; import { getSystemEncoding } from '../utils/systemEncoding.js'; @@ -63,7 +64,6 @@ export type CoreReadTextFileHandleRequest = Omit< 'limit' | 'stats' | 'maxOutputBytes' > & { fileHandle: FileHandle; - stats: Stats; limit?: number; maxOutputBytes: number; maxScanBytes: number; @@ -347,11 +347,14 @@ export class StandardFileSystemService implements FileSystemService { `handle-bound text reads require a positive integer limit or Infinity, got ${params.limit}`, ); } - return readTextFileStandard(params, { - fileHandle: params.fileHandle, - forceStreaming: true, + const range = await readTextRangeFromHandle(params.fileHandle, { + offset: params.line ?? 0, + limit: params.limit ?? Number.POSITIVE_INFINITY, + maxOutputBytes: params.maxOutputBytes, maxScanBytes: params.maxScanBytes, + ...(params.signal !== undefined ? { signal: params.signal } : {}), }); + return toReadTextFileResponse(range); } async writeTextFile( @@ -390,11 +393,6 @@ function isPositiveSafeInteger(value: unknown): value is number { async function readTextFileStandard( params: CoreReadTextFileRequest, - source?: { - fileHandle: FileHandle; - forceStreaming: boolean; - maxScanBytes: number; - }, ): Promise { const { path, limit, line, maxOutputBytes, signal, stats } = params; const readResult = await readFileWithLineAndLimit({ @@ -404,8 +402,20 @@ async function readTextFileStandard( ...(maxOutputBytes !== undefined ? { maxOutputBytes } : {}), ...(signal !== undefined ? { signal } : {}), ...(stats !== undefined ? { stats } : {}), - ...(source !== undefined ? source : {}), }); + return toReadTextFileResponse(readResult); +} + +/** Shared metadata shaping so both read paths report identically. */ +function toReadTextFileResponse(readResult: { + content: string; + bom?: boolean; + encoding?: string; + originalLineCount: number; + originalLineCountExact?: boolean; + lineEnding?: LineEnding; + truncatedByBytes?: boolean; +}): ReadTextFileResponse { const detectedLineEnding = readResult.lineEnding ?? detectLineEnding(readResult.content); return { diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index aaa817a4d69..85b302110a2 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -320,9 +320,6 @@ export async function readFileWithLineAndLimit(params: { maxOutputBytes?: number; signal?: AbortSignal; stats?: import('node:fs').Stats; - fileHandle?: fs.promises.FileHandle; - forceStreaming?: boolean; - maxScanBytes?: number; }): Promise<{ content: string; bom?: boolean; @@ -332,21 +329,8 @@ export async function readFileWithLineAndLimit(params: { lineEnding?: 'crlf' | 'lf'; truncatedByBytes?: boolean; }> { - const { - path: filePath, - limit, - line, - maxOutputBytes, - signal, - fileHandle, - forceStreaming, - maxScanBytes, - } = params; - const stats = - params.stats ?? - (fileHandle !== undefined - ? await fileHandle.stat() - : await fs.promises.stat(filePath)); + const { path: filePath, limit, line, maxOutputBytes, signal } = params; + const stats = params.stats ?? (await fs.promises.stat(filePath)); if ( (line !== undefined && line > 0) || Number.isFinite(limit) || @@ -359,21 +343,9 @@ export async function readFileWithLineAndLimit(params: { maxOutputBytes: normalizeRangeReadByteLimit(maxOutputBytes), stats, ...(signal !== undefined ? { signal } : {}), - ...(fileHandle !== undefined ? { fileHandle } : {}), - ...(forceStreaming !== undefined ? { forceStreaming } : {}), - ...(maxScanBytes !== undefined ? { maxScanBytes } : {}), }); } - // Below this point the read is satisfied by path, not by descriptor. A - // caller that pinned an inode would silently get bytes from whatever the - // path resolves to now, defeating the reason it opened a handle. - if (fileHandle !== undefined) { - throw new RangeError( - 'readFileWithLineAndLimit cannot honor fileHandle for an unbounded read: pass a finite limit or maxOutputBytes', - ); - } - signal?.throwIfAborted(); const { content, encoding, bom } = await readFileWithEncodingInfo( @@ -403,11 +375,21 @@ export async function readFileWithLineAndLimit(params: { * Detect the encoding of a file by reading a sample from its beginning. * Returns the encoding name (e.g. 'utf-8', 'gbk', 'shift_jis'). * Uses BOM detection first, then UTF-8 validation, then chardet as fallback. + * + * Accepts an already-open handle so a caller that has pinned an inode can be + * told the encoding of *that* inode rather than of whatever the path resolves + * to now. A supplied handle is borrowed: reads go through explicit positions so + * the caller's file position is untouched, and it is never closed here. */ -export async function detectFileEncoding(filePath: string): Promise { - let fh: fs.promises.FileHandle | null = null; +export async function detectFileEncoding( + source: string | fs.promises.FileHandle, +): Promise { + let opened: fs.promises.FileHandle | null = null; try { - fh = await fs.promises.open(filePath, 'r'); + const fh = + typeof source === 'string' + ? (opened = await fs.promises.open(source, 'r')) + : source; const stats = await fh.stat(); if (stats.size === 0) return 'utf-8'; @@ -420,22 +402,7 @@ export async function detectFileEncoding(filePath: string): Promise { // 1. Check for BOM const bom = detectBOM(sample); - if (bom) { - switch (bom.encoding) { - case 'utf8': - return 'utf-8'; - case 'utf16le': - return 'utf-16le'; - case 'utf16be': - return 'utf-16be'; - case 'utf32le': - return 'utf-32le'; - case 'utf32be': - return 'utf-32be'; - default: - return 'utf-8'; - } - } + if (bom) return bomEncodingToName(bom.encoding); // 2. Validate UTF-8 if (isValidUtf8(sample)) return 'utf-8'; @@ -451,9 +418,10 @@ export async function detectFileEncoding(filePath: string): Promise { // If file can't be read, default to UTF-8 return 'utf-8'; } finally { - if (fh) { + // Only what we opened. A borrowed handle outlives this call. + if (opened) { try { - await fh.close(); + await opened.close(); } catch { // Ignore close errors } diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index ea15cdc8c6c..65c6ff4af7d 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -13,6 +13,7 @@ import { LargeNonUtf8TextError, TextScanBudgetExceededError, readTextRange, + readTextRangeFromHandle, } from './read-text-range.js'; describe('readTextRange', () => { @@ -100,14 +101,11 @@ describe('readTextRange', () => { const fileHandle = await fs.open(filePath, 'r'); try { const stats = await fileHandle.stat(); - const result = await readTextRange({ - path: filePath, - fileHandle, - stats, - forceStreaming: true, + const result = await readTextRangeFromHandle(fileHandle, { offset: 1_500, limit: 3, maxOutputBytes: 10_000, + maxScanBytes: Number.MAX_SAFE_INTEGER, }); expect(result.content.split('\n')).toEqual([ @@ -117,14 +115,11 @@ describe('readTextRange', () => { ]); expect(result.originalLineCountExact).toBe(false); - const beyondEof = await readTextRange({ - path: filePath, - fileHandle, - stats, - forceStreaming: true, + const beyondEof = await readTextRangeFromHandle(fileHandle, { offset: 10_000, limit: 3, maxOutputBytes: 10_000, + maxScanBytes: Number.MAX_SAFE_INTEGER, }); expect(beyondEof.content).toBe(''); expect(beyondEof.originalLineCount).toBe(2_000); @@ -137,8 +132,11 @@ describe('readTextRange', () => { } }); - it('uses only the supplied file handle when the path names another file', async () => { - const originalPath = await writeFile( + it('reads the pinned inode after the path is replaced underneath it', async () => { + // The handle variant takes no path, so "read the wrong file" is no longer + // representable. What is still worth pinning down is the property that + // motivated the API: once opened, the read follows the inode, not the name. + const targetPath = await writeFile( 'original.log', 'safe-one\nsafe-two\nsafe-three\n', ); @@ -146,16 +144,15 @@ describe('readTextRange', () => { 'replacement.log', 'secret-one\nsecret-two\n', ); - const fileHandle = await fs.open(originalPath, 'r'); + const fileHandle = await fs.open(targetPath, 'r'); try { - const result = await readTextRange({ - path: replacementPath, - fileHandle, - stats: await fileHandle.stat(), - forceStreaming: true, + await fs.rename(replacementPath, targetPath); + + const result = await readTextRangeFromHandle(fileHandle, { offset: 0, limit: 2, maxOutputBytes: 1_024, + maxScanBytes: Number.MAX_SAFE_INTEGER, }); expect(result.content).toBe('safe-one\nsafe-two'); @@ -200,17 +197,19 @@ describe('readTextRange', () => { it('does not charge a budget failure to a file that ends within it', async () => { // The scan reaches EOF on the same chunk that exhausts the budget; the // window was fully satisfied, so there is nothing to refuse. + // Goes through the handle variant purely because that is the one that + // always streams: this file is far too small to leave the path variant's + // buffering fast path, and the buffered path never consults the budget. const body = largeUtf8Lines(100); const filePath = await writeFile('budget-exact.log', body); + const fileHandle = await fs.open(filePath, 'r'); - const result = await readTextRange({ - path: filePath, + const result = await readTextRangeFromHandle(fileHandle, { offset: 98, limit: 10, maxOutputBytes: 262_144, maxScanBytes: Buffer.byteLength(body), - forceStreaming: true, - }); + }).finally(() => fileHandle.close()); expect(result.content.split('\n')).toEqual([ expect.stringContaining('line-99 '), diff --git a/packages/core/src/utils/read-text-range.ts b/packages/core/src/utils/read-text-range.ts index f431aef7d06..7d33d1980a8 100644 --- a/packages/core/src/utils/read-text-range.ts +++ b/packages/core/src/utils/read-text-range.ts @@ -7,11 +7,7 @@ import { createReadStream, type Stats } from 'node:fs'; import { stat, type FileHandle } from 'node:fs/promises'; import { TextDecoder } from 'node:util'; -import { - decodeBufferWithEncodingInfoAsync, - detectFileEncoding, - readFileWithEncodingInfo, -} from './fileUtils.js'; +import { detectFileEncoding, readFileWithEncodingInfo } from './fileUtils.js'; import { isUtf8CompatibleEncoding } from './encoding.js'; import { DEFAULT_RANGE_READ_BYTES, @@ -25,13 +21,6 @@ export interface ReadTextRangeRequest { maxOutputBytes: number; signal?: AbortSignal; stats?: Stats; - /** - * Optional caller-owned handle. When present, every read is bound to this - * already-open inode; this function never closes the handle. - */ - fileHandle?: FileHandle; - /** Skip the small-file full-buffer fast path. */ - forceStreaming?: boolean; /** * Upper bound on bytes read off disk while locating the requested window. * Line offsets address a byte stream, so a deep `offset` costs a scan from @@ -42,6 +31,23 @@ export interface ReadTextRangeRequest { maxScanBytes?: number; } +/** + * Request shape for {@link readTextRangeFromHandle}. + * + * No `path`: the read is bound to the descriptor, so there is nothing for a + * path to disambiguate. Both byte bounds are required rather than optional — + * a handle-bound read exists because some caller pinned an inode at a security + * boundary, and what makes such a read safe is that the bytes it *returns* and + * the bytes it *scans* are each capped. + */ +export interface ReadTextRangeFromHandleRequest { + offset?: number; + limit?: number; + maxOutputBytes: number; + maxScanBytes: number; + signal?: AbortSignal; +} + export interface ReadTextRangeResult { content: string; originalLineCount: number; @@ -87,11 +93,7 @@ export async function readTextRange( request: ReadTextRangeRequest, ): Promise { request.signal?.throwIfAborted(); - const stats = - request.stats ?? - (request.fileHandle !== undefined - ? await request.fileHandle.stat() - : await stat(request.path)); + const stats = request.stats ?? (await stat(request.path)); const maxOutputBytes = normalizeMaxBytes(request.maxOutputBytes); const maxScanBytes = request.maxScanBytes ?? Number.POSITIVE_INFINITY; @@ -101,20 +103,13 @@ export async function readTextRange( // the same bound apply, and raises `TextScanBudgetExceededError` if the // window really is out of reach. if ( - request.forceStreaming !== true && stats.size < TEXT_RANGE_FAST_PATH_MAX_SIZE && stats.size <= maxScanBytes ) { - const { content, encoding, bom } = - request.fileHandle === undefined - ? await readFileWithEncodingInfo(request.path, request.signal) - : await decodeBufferWithEncodingInfoAsync( - await readFileHandleBuffer( - request.fileHandle, - stats.size, - request.signal, - ), - ); + const { content, encoding, bom } = await readFileWithEncodingInfo( + request.path, + request.signal, + ); request.signal?.throwIfAborted(); const range = sliceDecodedContent( content, @@ -130,7 +125,33 @@ export async function readTextRange( }; } - return readLargeUtf8Range(request, maxOutputBytes, maxScanBytes); + return readLargeUtf8Range( + request.path, + request, + maxOutputBytes, + maxScanBytes, + ); +} + +/** + * Range read bound to a caller-owned descriptor. + * + * Always streams: the buffering fast path would read the whole file, and a + * caller reaches for a handle precisely when it needs the read bounded. The + * handle is borrowed — every read uses an explicit position, and this function + * never closes it. + */ +export async function readTextRangeFromHandle( + fileHandle: FileHandle, + request: ReadTextRangeFromHandleRequest, +): Promise { + request.signal?.throwIfAborted(); + return readLargeUtf8Range( + fileHandle, + request, + normalizeMaxBytes(request.maxOutputBytes), + request.maxScanBytes, + ); } function normalizeMaxBytes(maxOutputBytes: number): number { @@ -174,14 +195,15 @@ function sliceDecodedContent( } async function readLargeUtf8Range( - request: ReadTextRangeRequest, + source: string | FileHandle, + request: { offset?: number; limit?: number; signal?: AbortSignal }, maxOutputBytes: number, maxScanBytes: number, ): Promise { - const encoding = - request.fileHandle === undefined - ? await detectFileEncoding(request.path) - : await detectFileHandleEncoding(request.fileHandle, request.signal); + const encoding = await detectFileEncoding(source); + // Detection is one bounded 8 KiB read, but check here anyway so an abort + // that lands during it is still observed before the streaming loop starts. + request.signal?.throwIfAborted(); if (!isUtf8CompatibleEncoding(encoding)) { throw new LargeNonUtf8TextError(encoding); } @@ -206,15 +228,17 @@ async function readLargeUtf8Range( ignoreBOM: true, }); - const pathStream = - request.fileHandle === undefined - ? createReadStream(request.path, { - highWaterMark: 512 * 1024, - signal: request.signal, - }) - : undefined; - const chunks = - pathStream ?? readFileHandleChunks(request.fileHandle!, request.signal); + let pathStream: ReturnType | undefined; + let chunks: AsyncIterable; + if (typeof source === 'string') { + pathStream = createReadStream(source, { + highWaterMark: 512 * 1024, + signal: request.signal, + }); + chunks = pathStream; + } else { + chunks = chunksFromHandle(source, 0, request.signal); + } function appendSelected(fragment: string): void { if (fragment.length === 0 || truncatedByBytes) { @@ -332,29 +356,21 @@ async function readLargeUtf8Range( }; } -async function readFileHandleBuffer( - fileHandle: FileHandle, - size: number, - signal?: AbortSignal, -): Promise { - const buffer = Buffer.alloc(size); - let offset = 0; - while (offset < size) { - signal?.throwIfAborted(); - const read = await fileHandle.read(buffer, offset, size - offset, offset); - if (read.bytesRead === 0) break; - offset += read.bytesRead; - } - signal?.throwIfAborted(); - return offset === buffer.length ? buffer : buffer.subarray(0, offset); -} - -async function* readFileHandleChunks( +/** + * Sequential chunks off a borrowed descriptor, starting at `from`. + * + * Reads use explicit positions, so the caller's file position is untouched and + * two readers can share one handle. Each chunk is a fresh buffer and stays + * valid after the next iteration — do not "optimize" this into one reused + * buffer without giving the yielded value a documented lifetime. + */ +async function* chunksFromHandle( fileHandle: FileHandle, + from = 0, signal?: AbortSignal, ): AsyncGenerator { const highWaterMark = 512 * 1024; - let position = 0; + let position = from; while (true) { signal?.throwIfAborted(); const buffer = Buffer.allocUnsafe(highWaterMark); @@ -371,24 +387,6 @@ async function* readFileHandleChunks( } } -async function detectFileHandleEncoding( - fileHandle: FileHandle, - signal?: AbortSignal, -): Promise { - signal?.throwIfAborted(); - const stats = await fileHandle.stat(); - if (stats.size === 0) return 'utf-8'; - - const sample = Buffer.alloc(Math.min(8192, stats.size)); - const { bytesRead } = await fileHandle.read(sample, 0, sample.length, 0); - signal?.throwIfAborted(); - if (bytesRead === 0) return 'utf-8'; - return ( - (await decodeBufferWithEncodingInfoAsync(sample.subarray(0, bytesRead))) - .encoding ?? 'utf-8' - ); -} - function truncateUtf8( content: string, maxBytes: number, From a74eeea739a78d98ddf93823392123e5bf363943 Mon Sep 17 00:00:00 2001 From: jinye Date: Wed, 29 Jul 2026 08:31:49 +0800 Subject: [PATCH 04/16] refactor(core): make CoreReadTextFileHandleRequest standalone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-audit follow-up to f55c867a. Two fields survived the reshape that the handle path never reads: - `stats` was documented as required ("must pass the Stats captured from that handle") and nothing downstream read it. The handle path always streams, so it never needs a size to choose a strategy, and the encoding probe does its own fstat. - `path` became dead once readTextRangeFromHandle replaced the path-plus-handle call. Errors are labelled with the path by the Serve boundary that owns it. Neither was caught by the compiler: the ACP ReadTextFileRequest the type derived from permits extra properties, so the CLI kept passing both silently. That is the argument for declaring the type standalone rather than Omit-ing four of six inherited fields and quietly re-admitting the rest. Also record the second behaviour delta of the detector merge in the design doc: detectFileEncoding catches I/O errors and falls back to 'utf-8', where detectFileHandleEncoding let them propagate. The failure is not lost — a handle that fails the 8 KiB probe fails the streaming read immediately after — but a different call now reports it. Co-Authored-By: Claude Opus 5 --- ...026-07-29-handle-bound-text-range-reads.md | 46 ++++++++++++++----- .../cli/src/serve/fs/workspace-file-system.ts | 1 - .../src/services/fileSystemService.test.ts | 2 - .../core/src/services/fileSystemService.ts | 24 ++++++---- 4 files changed, 49 insertions(+), 24 deletions(-) diff --git a/docs/design/2026-07-29-handle-bound-text-range-reads.md b/docs/design/2026-07-29-handle-bound-text-range-reads.md index 9a844003d2c..e3460c38376 100644 --- a/docs/design/2026-07-29-handle-bound-text-range-reads.md +++ b/docs/design/2026-07-29-handle-bound-text-range-reads.md @@ -70,6 +70,15 @@ decoder and raising the generic `'invalid-utf8'` variant. The refusal is unchanged; the message improves. The Serve boundary maps both to `binary_file`, so nothing downstream moves. +A second, smaller delta comes with the merge: `detectFileEncoding` catches all +errors and falls back to `'utf-8'`, whereas `detectFileHandleEncoding` had no +handler and let an I/O failure propagate. The failure is not lost — a handle bad +enough to fail the 8 KiB probe fails the streaming read immediately after, and a +file that is not really UTF-8 is still refused by the `fatal: true` decoder — so +the error surfaces from a different call rather than disappearing. Accepted for +the single fallback policy; noted because it is a real change in which call +reports the problem. + ### Two entry points ```ts @@ -115,18 +124,31 @@ untouched. - `detectFileEncoding` is public via `export * from './utils/fileUtils.js'`. Widening a parameter is source-compatible. - The only cross-package importer of the touched modules is - `packages/cli/src/serve/fs/workspace-file-system.ts`. Its one change is - deleting a `stats:` argument that the handle path no longer accepts — see - below; the `decodeBufferWithEncodingInfoAsync` import it also carries is - untouched. - -`CoreReadTextFileHandleRequest` drops its required `stats` field. The handle -path always streams and the encoding probe does its own `fstat`, so nothing -downstream ever read it — and because the ACP request type it extends permits -extra properties, TypeScript accepted the dead argument silently rather than -flagging it. Removing the field is what surfaced the one CLI line. - -258 production logic lines change in `packages/core`, net −71 lines overall. + `packages/cli/src/serve/fs/workspace-file-system.ts`. Its only change is + dropping two arguments the handle path no longer accepts — see below; the + `decodeBufferWithEncodingInfoAsync` import it also carries is untouched. + +### `CoreReadTextFileHandleRequest` becomes standalone + +It was `Omit & +{...}`, which left two fields the handle path never reads: + +- **`stats`** was documented as required — "must pass the Stats captured from + that handle" — and nothing downstream read it. The handle path always streams, + so it never needs a size to choose a strategy, and the encoding probe does its + own `fstat`. +- **`path`** became dead once `readTextRangeFromHandle` replaced the + path-plus-handle call: the read is bound to the descriptor, and errors are + labelled with the path by the Serve boundary that owns it. + +Neither was caught by the compiler. The ACP `ReadTextFileRequest` this type +derived from permits extra properties, so passing a field the type had removed +raised nothing. That is the argument for declaring the type standalone rather +than deriving it: the `Omit` chain was stripping four of six inherited fields +and quietly re-admitting the rest. + +282 production logic lines change in `packages/core`; across `packages/` the +change is net −68 lines. ## Testing diff --git a/packages/cli/src/serve/fs/workspace-file-system.ts b/packages/cli/src/serve/fs/workspace-file-system.ts index 1b5adf016a0..8f8155ce3fb 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.ts @@ -1548,7 +1548,6 @@ async function readLargeTextWindowFromResolvedFile( >; try { result = await lowFs.readTextFileFromHandle({ - path: p as string, fileHandle: fh, limit: opts.limit ?? Number.POSITIVE_INFINITY, line: opts.line !== undefined ? opts.line - 1 : 0, diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 632ffddd492..303a24f5865 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -198,7 +198,6 @@ describe('StandardFileSystemService', () => { await expect( fileSystem.readTextFileFromHandle({ - path: '/test/large.txt', fileHandle, limit: 20, maxOutputBytes: 262_144, @@ -217,7 +216,6 @@ describe('StandardFileSystemService', () => { await expect( fileSystem.readTextFileFromHandle({ - path: '/test/large.txt', fileHandle, limit, maxOutputBytes: 262_144, diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 9ba6146c5e0..5f4138c87dc 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -51,23 +51,29 @@ export type CoreReadTextFileRequest = Omit< /** * Handle-bound range read used by filesystem security boundaries. The caller - * owns the handle lifecycle and must pass the Stats captured from that handle. + * opens the descriptor, keeps it open for the duration, and closes it; this + * request never transfers ownership. * - * Both bounds are required rather than optional: what makes a large-file read - * safe at a boundary is that the *returned* bytes and the *scanned* bytes are - * each capped. A finite `limit` is not one of those bounds — `limit: 20` at + * Declared standalone rather than derived from {@link CoreReadTextFileRequest}: + * a handle-bound read shares only `line` and `signal` with a path-bound one, so + * an `Omit` chain would strip more than it kept and would keep re-admitting + * fields — `path`, `stats` — that this path has no use for. + * + * Both byte bounds are required rather than optional: what makes a large-file + * read safe at a boundary is that the *returned* bytes and the *scanned* bytes + * are each capped. A finite `limit` is not one of those bounds — `limit: 20` at * `line: 900_000_000` still walks the whole file — so it stays optional and * `maxScanBytes` is what actually keeps the read affordable. */ -export type CoreReadTextFileHandleRequest = Omit< - CoreReadTextFileRequest, - 'limit' | 'stats' | 'maxOutputBytes' -> & { +export interface CoreReadTextFileHandleRequest { fileHandle: FileHandle; + /** 0-based start line, matching {@link CoreReadTextFileRequest}. */ + line?: number | null; limit?: number; maxOutputBytes: number; maxScanBytes: number; -}; + signal?: AbortSignal; +} /** * Supported file encodings for new files. From 63822f6606f91b039e2063ea6f040e63cb6c545c Mon Sep 17 00:00:00 2001 From: Qwen Code Bot Date: Wed, 29 Jul 2026 14:27:32 +0000 Subject: [PATCH 05/16] fix(serve): address review feedback on decoded-output cap, lineEnding, line validation, and docs (#7967) --- .../daemon/07-workspace-filesystem.md | 4 +- packages/cli/src/serve/fs/policy.ts | 14 +++--- .../serve/fs/workspace-file-system.test.ts | 50 +++++++++++++++++++ .../cli/src/serve/fs/workspace-file-system.ts | 17 +++---- .../src/services/fileSystemService.test.ts | 17 +++++++ .../core/src/services/fileSystemService.ts | 9 ++++ packages/core/src/utils/read-text-range.ts | 6 ++- 7 files changed, 97 insertions(+), 20 deletions(-) diff --git a/docs/developers/daemon/07-workspace-filesystem.md b/docs/developers/daemon/07-workspace-filesystem.md index b0c9d2e2317..b5fc5398879 100644 --- a/docs/developers/daemon/07-workspace-filesystem.md +++ b/docs/developers/daemon/07-workspace-filesystem.md @@ -45,7 +45,7 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST | `path_not_found` | 404 | `ENOENT`. | | `binary_file` | 422 | Content sniffed binary on a text route, or large text in an encoding the text route cannot decode. | | `file_too_large` | 413 | Windowless/full-snapshot text above `MAX_READ_BYTES`, a line offset beyond `MAX_TEXT_SCAN_BYTES`, or a write above `MAX_WRITE_BYTES`. | -| `hash_mismatch` | 409 | Optimistic-concurrency `expectedSha256` failed. | +| `hash_mismatch` | 409 | Optimistic-concurrency `expectedSha256` failed, or the file changed during a stable read. | | `file_already_exists` | 409 | `mode: 'create'` against an existing file. | | `text_not_found` | 422 | `POST /file/edit`'s search string wasn't in the file. | | `ambiguous_text_match` | 422 | Multiple matches when exactly one was required. | @@ -71,7 +71,7 @@ This is the injection point for ACP `readTextFile` / `writeTextFile`. Bridge tes Two defensive gates the adapter MUST replicate (because the inline proxy is fully bypassed when the adapter is injected): 1. **Reject non-regular files** — sockets / pipes / char devices / procfs / sysfs entries can stream unbounded data despite `stats.size === 0`. The inline path throws with `describeStatKind(stats)` in the message. -2. **Cap buffered size** at `READ_FILE_SIZE_CAP = 100 MiB`. A tiny `{ line: 1, limit: 10 }` request against a 500 MB log would otherwise cost 500 MB of RSS just to return 10 lines. +2. **Avoid unbounded full-file buffering.** The inline fallback caps a buffered read at `READ_FILE_SIZE_CAP = 100 MiB`. The injected adapter applies the stricter `WorkspaceFileSystem` contract: full snapshots stop at `MAX_READ_BYTES = 256 KiB`, and larger UTF-8 files require a finite window and are streamed from an inode-bound handle. The adapter goes further: it uses `WorkspaceFileSystem.writeTextOverwrite` (PR 18 primitive) for atomic temporary-file-and-rename writes with mode preservation, `0o600` default, and symlink rejection inside a per-path lock. This is a **divergence from the pre-F1 inline proxy** which resolved symlinks and wrote through to their target — agents that relied on writing through symlinked dotfiles now have to address the resolved path directly. diff --git a/packages/cli/src/serve/fs/policy.ts b/packages/cli/src/serve/fs/policy.ts index fe947129af8..c51142693fd 100644 --- a/packages/cli/src/serve/fs/policy.ts +++ b/packages/cli/src/serve/fs/policy.ts @@ -25,10 +25,9 @@ import type { Intent, ResolvedPath } from './paths.js'; * via post-decode truncation (`enforceReadSize`); that's where the * `meta.truncated = true` flag fires. * - * `enforceReadBytesSize` (the `readBytes` gate) and `edit()` use the - * same constant as a hard upper bound — multi-GB files in the - * workspace can no longer reach `fsp.readFile` through any - * boundary path. + * `enforceReadBytesSize` and `edit()` use the same constant as a + * hard upper bound — multi-GB files in the workspace can no longer + * reach `fsp.readFile` through any boundary path. */ export const MAX_READ_BYTES = 256 * 1024; @@ -225,9 +224,10 @@ export function enforceWriteSize( /** * Throw `file_too_large` when `fileBytes` exceeds the hard - * `MAX_READ_BYTES` cap. This is the OOM-defense gate `readBytes` - * runs at stat time — a 5 GB file is rejected before - * `fsp.readFile` allocates the buffer. + * `MAX_READ_BYTES` cap. The production `readBytesWindow` path + * does not call this helper — it reads an explicit bounded window + * from an opened handle — but strict callers and tests can use it + * as a stat-time reject. * * Soft window-read (`opts.maxBytes` truncation) is NOT this * function's job: `readBytes` truncates the returned buffer diff --git a/packages/cli/src/serve/fs/workspace-file-system.test.ts b/packages/cli/src/serve/fs/workspace-file-system.test.ts index 05b3ad45deb..48b7c30d8fa 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.test.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.test.ts @@ -167,6 +167,38 @@ describe('WorkspaceFileSystem - readText', () => { expect(out.content.length).toBeLessThanOrEqual(1024); }); + it('caps decoded UTF-8 bytes when a smaller source encoding expands', async () => { + const smallTarget = path.join(h.workspace, 'expanded-small.txt'); + const smallRaw = Buffer.concat([ + Buffer.from([0xff, 0xfe]), + Buffer.from('中'.repeat(40), 'utf16le'), + ]); + expect(smallRaw.length).toBe(82); + await fsp.writeFile(smallTarget, smallRaw); + const smallResolved = await h.fs.resolve('expanded-small.txt', 'read'); + const small = await h.fs.readText(smallResolved, { maxBytes: 100 }); + expect(Buffer.byteLength(small.content)).toBeLessThanOrEqual(100); + expect(small.content).not.toContain('\uFFFD'); + expect(small.meta.encoding).toBe('utf-16le'); + expect(small.meta.truncated).toBe(true); + + const defaultTarget = path.join(h.workspace, 'expanded-default.txt'); + const defaultRaw = Buffer.concat([ + Buffer.from([0xff, 0xfe]), + Buffer.from('中'.repeat(100_000), 'utf16le'), + ]); + const maxReadBytes = (await import('./policy.js')).MAX_READ_BYTES; + expect(defaultRaw.length).toBeLessThan(maxReadBytes); + await fsp.writeFile(defaultTarget, defaultRaw); + const defaultResolved = await h.fs.resolve('expanded-default.txt', 'read'); + const expanded = await h.fs.readText(defaultResolved); + expect(Buffer.byteLength(expanded.content)).toBeLessThanOrEqual( + maxReadBytes, + ); + expect(expanded.content).not.toContain('\uFFFD'); + expect(expanded.meta.truncated).toBe(true); + }); + it('throws file_too_large for an oversized read with no window argument', async () => { const big = path.join(h.workspace, 'huge.txt'); const bytes = (await import('./policy.js')).MAX_READ_BYTES + 1; @@ -331,6 +363,24 @@ describe('WorkspaceFileSystem - readText', () => { expect(out.meta.hash).toBeUndefined(); }); + it('reports line endings from the selected large-file window', async () => { + const target = path.join(h.workspace, 'large-mixed-endings.txt'); + const lines = Array.from( + { length: 8_000 }, + (_, index) => `row-${index + 1} ${'x'.repeat(30)}`, + ); + const content = `header\r\n${lines.join('\n')}`; + const maxReadBytes = (await import('./policy.js')).MAX_READ_BYTES; + expect(Buffer.byteLength(content)).toBeGreaterThan(maxReadBytes); + await fsp.writeFile(target, content); + const resolved = await h.fs.resolve('large-mixed-endings.txt', 'read'); + + const out = await h.fs.readText(resolved, { line: 2, limit: 2 }); + expect(out.content).toBe(lines.slice(0, 2).join('\n')); + expect(out.content).not.toContain('\r\n'); + expect(out.meta.lineEnding).toBe('lf'); + }); + it('rejects a symlink swap while a large range is being read', async () => { const target = path.join(h.workspace, 'large-swap.txt'); const moved = path.join(h.workspace, 'large-swap.original.txt'); diff --git a/packages/cli/src/serve/fs/workspace-file-system.ts b/packages/cli/src/serve/fs/workspace-file-system.ts index 8f8155ce3fb..d1dd6f84f20 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.ts @@ -1459,14 +1459,13 @@ async function readTextSnapshotFromResolvedFile( hash: hashBuffer(raw), }; - if (sizeOutcome.truncated) { - const buf = Buffer.from(content, 'utf-8'); - if (buf.length > sizeOutcome.bytesToRead) { - content = safeUtf8Truncate(buf, sizeOutcome.bytesToRead).toString( - 'utf-8', - ); - meta.lineEnding = detectLineEnding(content); - } + const outputCap = opts.maxBytes ?? MAX_READ_BYTES; + const decodedBuf = Buffer.from(content, 'utf-8'); + if (decodedBuf.length > outputCap) { + content = safeUtf8Truncate(decodedBuf, outputCap).toString('utf-8'); + meta.lineEnding = detectLineEnding(content); + meta.truncated = true; + } else if (sizeOutcome.truncated) { meta.truncated = true; } @@ -1604,7 +1603,7 @@ async function readLargeTextWindowFromResolvedFile( const meta: TextReadOutcome['meta'] = { encoding: readMeta?.encoding, bom: readMeta?.bom, - lineEnding: readMeta?.lineEnding ?? detectLineEnding(content), + lineEnding: detectLineEnding(content), // Size as of `open`, not as of now: it describes the snapshot the // returned window was cut from. A file that grew during the read // reports the smaller, consistent number. diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 303a24f5865..e1b1d8e7b51 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -224,6 +224,23 @@ describe('StandardFileSystemService', () => { ).rejects.toThrow(/positive integer limit or Infinity/); }); + it.each([ + ['a negative line', -1], + ['a fractional line', 1.5], + ])('should reject %s on a handle read', async (_label, line) => { + const fileHandle = {} as import('node:fs/promises').FileHandle; + + await expect( + fileSystem.readTextFileFromHandle({ + fileHandle, + line, + limit: 20, + maxOutputBytes: 262_144, + maxScanBytes: 8 * 1024 * 1024, + }), + ).rejects.toThrow(/non-negative integer line/); + }); + it('should return encoding info for GBK file', async () => { vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ content: '你好世界', diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index 5f4138c87dc..a23aabeac27 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -353,6 +353,15 @@ export class StandardFileSystemService implements FileSystemService { `handle-bound text reads require a positive integer limit or Infinity, got ${params.limit}`, ); } + if ( + params.line !== undefined && + params.line !== null && + (!Number.isSafeInteger(params.line) || params.line < 0) + ) { + throw new RangeError( + `handle-bound text reads require a non-negative integer line, got ${params.line}`, + ); + } const range = await readTextRangeFromHandle(params.fileHandle, { offset: params.line ?? 0, limit: params.limit ?? Number.POSITIVE_INFINITY, diff --git a/packages/core/src/utils/read-text-range.ts b/packages/core/src/utils/read-text-range.ts index 7d33d1980a8..4aa1a5b2f11 100644 --- a/packages/core/src/utils/read-text-range.ts +++ b/packages/core/src/utils/read-text-range.ts @@ -25,8 +25,10 @@ export interface ReadTextRangeRequest { * Upper bound on bytes read off disk while locating the requested window. * Line offsets address a byte stream, so a deep `offset` costs a scan from * byte 0 — this is what keeps that scan from being unbounded. Defaults to - * `Infinity` so non-boundary callers (the `read_file` tool) are unchanged; - * security boundaries must pass a finite value. + * `Infinity` so non-boundary callers (the `read_file` tool) are unchanged. + * The real security boundary is the handle variant + * ({@link ReadTextRangeFromHandleRequest}), where this field is required; + * no production caller of the path variant sets a finite value. */ maxScanBytes?: number; } From 63d49c415c654630139c8611b4abc6fd3ae96e4e Mon Sep 17 00:00:00 2001 From: "qwen-autofix[bot]" Date: Wed, 29 Jul 2026 17:54:15 +0000 Subject: [PATCH 06/16] fix(serve): scope PR to descriptor threading, revert #7947 behavior reversals (#7967) Maintainer review adjudicated the pending design-reversal Critical: the descriptor-threading refactor is sound and boundary-neutral, but this PR also carried two undisclosed reversals of #7947 that must not ride in on a refactor. Revert both, keeping only the refactor and the disclosed non-UTF-8 mapping: - Restore didFileVersionChange (size + mtime + ctime equality before and after the streamed read) in place of assertDidNotShrink. The size-only guard let a same-size in-place overwrite return torn content and a concurrent append run past the snapshot it opened against; the equality check rejects both with hash_mismatch. Main's three snapshot tests are restored unchanged. - Restore #7947's limit-only large-text admission and drop MAX_TEXT_SCAN_BYTES / maxScanBytes / TextScanBudgetExceededError. The broadened wantsWindow admission and scan budget are a real contract change that belongs in its own PR against #7947, not here. Kept: the descriptor-threading refactor (one detectFileEncoding, two entry points, readTextFileFromHandle/readTextRangeFromHandle, no mode flags) and the disclosed mapping of large non-UTF-8 windows to binary_file rather than file_too_large, which the maintainer explicitly approved. --- ...026-07-29-handle-bound-text-range-reads.md | 38 +-- .../daemon/07-workspace-filesystem.md | 38 +-- docs/developers/qwen-serve-protocol.md | 25 +- .../serve/bridge-file-system-adapter.test.ts | 19 ++ packages/cli/src/serve/fs/policy.ts | 56 +--- .../serve/fs/workspace-file-system.test.ts | 198 +++++++----- .../cli/src/serve/fs/workspace-file-system.ts | 282 ++++++++---------- .../serve/routes/workspace-file-read.test.ts | 18 ++ packages/core/src/index.ts | 5 +- .../src/services/fileSystemService.test.ts | 6 - .../core/src/services/fileSystemService.ts | 16 +- .../core/src/utils/read-text-range.test.ts | 61 ---- packages/core/src/utils/read-text-range.ts | 71 +---- 13 files changed, 361 insertions(+), 472 deletions(-) diff --git a/docs/design/2026-07-29-handle-bound-text-range-reads.md b/docs/design/2026-07-29-handle-bound-text-range-reads.md index e3460c38376..6768b6a15b6 100644 --- a/docs/design/2026-07-29-handle-bound-text-range-reads.md +++ b/docs/design/2026-07-29-handle-bound-text-range-reads.md @@ -89,12 +89,10 @@ readTextRangeFromHandle(fh, request: ReadTextRangeFromHandleRequest) The handle variant always streams — there is no flag, because a caller reaches for a handle precisely when it needs the read bounded, and the buffering fast path would read the whole file. Its request type has no `path` (nothing for one -to disambiguate) and makes both byte bounds required rather than optional: -`maxOutputBytes` caps what the read returns, `maxScanBytes` caps what it costs, -and a handle-bound read exists because a security boundary needs both. - -`maxScanBytes` stays optional on the path variant, where it defaults to -`Infinity` so the `read_file` tool is unchanged. +to disambiguate) and makes `maxOutputBytes` required rather than optional: it +caps what the read returns, and a handle-bound read exists because a security +boundary needs that bound. `limit` stays optional — a caller may read to end of +file — and the borrowing boundary decides which reads to admit. Both delegate to the same streaming implementation, which now takes `source: string | FileHandle` and selects `createReadStream` or @@ -103,8 +101,8 @@ called it are deleted. ### The fallthrough disappears -`readFileWithLineAndLimit` loses `fileHandle`, `forceStreaming`, and -`maxScanBytes` — its single production caller passes none of them. +`readFileWithLineAndLimit` loses `fileHandle` and `forceStreaming` — its single +production caller passes neither. `StandardFileSystemService.readTextFileFromHandle` now calls `readTextRangeFromHandle` directly, and the two read paths share a `toReadTextFileResponse` helper so their metadata shaping cannot drift. With no @@ -117,8 +115,8 @@ untouched. ## Blast radius -- `readTextRange` is not exported from `packages/core/src/index.ts`; only the - two error classes are. The reshaped surface is core-internal. +- `readTextRange` is not exported from `packages/core/src/index.ts`; only + `LargeNonUtf8TextError` is. The reshaped surface is core-internal. - `readTextRange` and `readFileWithLineAndLimit` have exactly one production caller each (`fileUtils.ts`, `fileSystemService.ts`). - `detectFileEncoding` is public via `export * from './utils/fileUtils.js'`. @@ -147,23 +145,19 @@ raised nothing. That is the argument for declaring the type standalone rather than deriving it: the `Omit` chain was stripping four of six inherited fields and quietly re-admitting the rest. -282 production logic lines change in `packages/core`; across `packages/` the -change is net −68 lines. +The change is confined to `packages/core` internals and the single +cross-package call site named above. ## Testing The existing suites are the specification: the whole point is that the Serve -boundary cannot tell. `packages/cli/src/serve/fs/` and the bridge adapter — 222 -tests — pass unmodified, as does the full `packages/core` `src/utils` + -`src/services` run (6078 tests). - -Three tests in `read-text-range.test.ts` moved to `readTextRangeFromHandle`. Two -used `fileHandle` directly. The third used a _path_ with `forceStreaming: true` -to force streaming on a file too small to leave the fast path, so that it could -exercise the budget-at-EOF boundary; with the flag gone, the handle variant is -the only thing that always streams. +boundary cannot tell. `packages/cli/src/serve/fs/` and the bridge adapter pass +against the refactor unchanged apart from one assertion updated for the +deliberate non-UTF-8 → `binary_file` mapping, as does the full `packages/core` +`src/utils` + `src/services` run. -One of the moved tests changed meaning. It previously passed a handle for one +Two tests in `read-text-range.test.ts` moved to `readTextRangeFromHandle`. One +of them changed meaning. It previously passed a handle for one file and a path naming a different file, asserting the handle won — a test for the confusion the old signature permitted. The handle variant has no `path`, so that confusion is now unrepresentable and the test would assert nothing. It was diff --git a/docs/developers/daemon/07-workspace-filesystem.md b/docs/developers/daemon/07-workspace-filesystem.md index b5fc5398879..d19813af798 100644 --- a/docs/developers/daemon/07-workspace-filesystem.md +++ b/docs/developers/daemon/07-workspace-filesystem.md @@ -6,7 +6,7 @@ The daemon never lets HTTP routes or ACP-side agent calls touch the host filesys - **Path resolution** — canonicalize paths and reject anything escaping the bound workspace, including via symlinks. - **Trust gating** — refuse writes when the workspace is not trusted (`untrusted_workspace`). -- **Size & content policy** — full-snapshot/output cap (`MAX_READ_BYTES = 256 KiB`), large-text windows bounded in both output and scan cost (`MAX_TEXT_SCAN_BYTES = 8 MiB`), write cap (`MAX_WRITE_BYTES = 5 MiB`), binary detection. +- **Size & content policy** — full-snapshot/output cap (`MAX_READ_BYTES = 256 KiB`), bounded large-text windows, write cap (`MAX_WRITE_BYTES = 5 MiB`), binary detection. - **Atomicity** — write-then-rename with target mode preservation and `0o600` default for new files. - **Audit** — every access / denial emits a structured event for `PermissionAuditRing` / monitoring. - **Typed errors** — closed `FsErrorKind` union mapped to HTTP statuses. @@ -17,7 +17,7 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST - Resolve user-supplied paths into branded `ResolvedPath` values that the rest of the boundary can safely use. - Refuse paths outside the bound workspace (`path_outside_workspace`) and paths whose target is a symlink (`symlink_escape`). -- Refuse full-snapshot reads above `MAX_READ_BYTES`, while allowing explicit windows with output capped at `MAX_READ_BYTES` and scan cost capped at `MAX_TEXT_SCAN_BYTES`; refuse writes above `MAX_WRITE_BYTES` and binary files (`binary_file`). +- Refuse full-snapshot reads above `MAX_READ_BYTES`, while allowing finite line windows with output capped at `MAX_READ_BYTES`; refuse writes above `MAX_WRITE_BYTES` and binary files (`binary_file`). - Refuse writes/edits when the workspace is untrusted (`untrusted_workspace`) — gated by `assertTrustedForIntent(trusted, intent)`. - Honor `.gitignore` / `.qwenignore` patterns via `shouldIgnore`. - Perform atomic write-then-rename with target mode preservation; default new file mode is `0o600`. @@ -31,7 +31,7 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST | File | Purpose | | -------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `paths.ts` | `canonicalizeWorkspace`, `resolveWithinWorkspace`, `hasSuspiciousPathPattern`, branded `ResolvedPath`, `Intent` union (`read \| write \| list \| stat \| glob`). | -| `policy.ts` | `MAX_READ_BYTES`, `MAX_TEXT_SCAN_BYTES`, `MAX_WRITE_BYTES`, `BINARY_PROBE_BYTES`, `assertTrustedForIntent`, `detectBinary`, `enforceReadBytesSize`, `enforceReadSize`, `enforceWriteSize`, `shouldIgnore`. | +| `policy.ts` | `MAX_READ_BYTES`, `MAX_WRITE_BYTES`, `BINARY_PROBE_BYTES`, `assertTrustedForIntent`, `detectBinary`, `enforceReadBytesSize`, `enforceReadSize`, `enforceWriteSize`, `shouldIgnore`. | | `audit.ts` | `FS_ACCESS_EVENT_TYPE`, `FS_DENIED_EVENT_TYPE`, `createAuditPublisher`, audit payload types. | | `errors.ts` | `FsError` class, `isFsError`, `FsErrorKind` union (14 kinds), `FsErrorStatus` union (`400 / 403 / 404 / 409 / 413 / 422 / 500 / 503`). | | `workspace-file-system.ts` | `createWorkspaceFileSystemFactory`, `WorkspaceFileSystem` (the orchestrator that reads/writes/lists), `WriteMode`, `ContentHash`, `FsEntry`, `FsStat`, `ListOptions`, `GlobOptions`, `ReadTextOptions`, `ReadBytesOptions`, `WriteTextAtomicOptions`. | @@ -44,7 +44,7 @@ The HTTP file routes (`GET /file`, `GET /file/bytes`, `POST /file/write`, `POST | `symlink_escape` | 400 | Target is a symlink (rejected per the conservative PR 18 + PR 20 posture). | | `path_not_found` | 404 | `ENOENT`. | | `binary_file` | 422 | Content sniffed binary on a text route, or large text in an encoding the text route cannot decode. | -| `file_too_large` | 413 | Windowless/full-snapshot text above `MAX_READ_BYTES`, a line offset beyond `MAX_TEXT_SCAN_BYTES`, or a write above `MAX_WRITE_BYTES`. | +| `file_too_large` | 413 | Unbounded/full-snapshot text above `MAX_READ_BYTES`, or a write above `MAX_WRITE_BYTES`. | | `hash_mismatch` | 409 | Optimistic-concurrency `expectedSha256` failed, or the file changed during a stable read. | | `file_already_exists` | 409 | `mode: 'create'` against an existing file. | | `text_not_found` | 422 | `POST /file/edit`'s search string wasn't in the file. | @@ -68,10 +68,10 @@ interface BridgeFileSystem { This is the injection point for ACP `readTextFile` / `writeTextFile`. Bridge tests and Mode A embedded callers can omit it on `BridgeOptions`; `BridgeClient` falls back to its inline `fs.readFile` / `fs.writeFile` proxy (preserves pre-F1 behavior). Production `qwen serve` wires `BridgeFileSystem` through `createBridgeFileSystemAdapter(fsFactory)` (`packages/cli/src/serve/bridge-file-system-adapter.ts`) so agent-side ACP writes pick up the same TOCTOU, symlink, trust-gate, and audit gates the HTTP routes use. -Two defensive gates the adapter MUST replicate (because the inline proxy is fully bypassed when the adapter is injected): +Two defensive properties the adapter MUST preserve (because the inline proxy is fully bypassed when the adapter is injected): 1. **Reject non-regular files** — sockets / pipes / char devices / procfs / sysfs entries can stream unbounded data despite `stats.size === 0`. The inline path throws with `describeStatKind(stats)` in the message. -2. **Avoid unbounded full-file buffering.** The inline fallback caps a buffered read at `READ_FILE_SIZE_CAP = 100 MiB`. The injected adapter applies the stricter `WorkspaceFileSystem` contract: full snapshots stop at `MAX_READ_BYTES = 256 KiB`, and larger UTF-8 files require a finite window and are streamed from an inode-bound handle. +2. **Avoid unbounded full-file buffering.** The inline fallback caps a buffered read at `READ_FILE_SIZE_CAP = 100 MiB`. The injected adapter instead applies the stricter WorkspaceFileSystem contract: full snapshots stop at 256 KiB, while larger UTF-8 files require a finite `limit` and are streamed from an inode-bound handle with at most 256 KiB returned. It must not read an entire 500 MB log merely to return `{ line: 1, limit: 10 }`. The adapter goes further: it uses `WorkspaceFileSystem.writeTextOverwrite` (PR 18 primitive) for atomic temporary-file-and-rename writes with mode preservation, `0o600` default, and symlink rejection inside a per-path lock. This is a **divergence from the pre-F1 inline proxy** which resolved symlinks and wrote through to their target — agents that relied on writing through symlinked dotfiles now have to address the resolved path directly. @@ -142,17 +142,19 @@ sequenceDiagram alt file <= 256 KiB FS->>FSP: open + read stable full snapshot FSP-->>FS: buffer + FS->>POL: detectBinary(buffer) + FS->>FS: reject if binary FS->>FS: hash full snapshot; apply line/output limits - else file > 256 KiB AND an explicit window arg + else file > 256 KiB AND finite limit FS->>FSP: open stable FileHandle + FS->>POL: detectBinary(handle sample) + FS->>FS: reject if binary FS->>FS: stream requested lines from the same inode - FS->>FS: cap output at 256 KiB and scan at 8 MiB; omit full-file hash - else windowless large read + FS->>FS: recheck size + mtime + ctime + device/inode + FS->>FS: cap output at 256 KiB; omit full-file hash + else unbounded large read FS-->>R: file_too_large end - FS->>POL: detectBinary(sample) - POL-->>FS: isBinary? - FS->>FS: reject if binary FS->>FS: shouldIgnore? → annotate meta.matchedIgnore FS->>FS: audit fs.access FS-->>R: { content, optional sha256, truncated?, meta } @@ -225,8 +227,7 @@ flowchart LR | Source | Knob | Effect | | ------------------------------------------------- | --------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------- | | `WorkspaceFileSystemFactoryDeps.trusted: boolean` | Constructor input | Whether writes are allowed; defaults to `true` from `runQwenServe`, `false` from `createServeApp` (with warning). | -| Constant | `MAX_READ_BYTES = 256 KiB` | Full-snapshot and returned-text cap; larger text requires an explicit window argument. | -| Constant | `MAX_TEXT_SCAN_BYTES = 8 MiB` | Bytes a large-text read may scan to locate a line offset; past it, `file_too_large`. | +| Constant | `MAX_READ_BYTES = 256 KiB` | Full-snapshot and returned-text cap; larger text requires a finite line limit. | | Constant | `MAX_WRITE_BYTES = 5 MiB` | Write cap; sized below `express.json({ limit: '10mb' })`. | | Constant | `BINARY_PROBE_BYTES = 4096` | Sample size for content-based binary detection. | | Capability tags | `workspace_file_read`, `workspace_file_bytes`, `workspace_file_write` | See [`11-capabilities-versioning.md`](./11-capabilities-versioning.md). | @@ -238,11 +239,10 @@ flowchart LR - **`io_error` vs `permission_denied` are distinct.** Do not conflate them. Monitoring pipelines key on `errorKind` for alerting — folding ENOSPC into permission_denied would page security responders for `df -h` problems. - **New file mode defaults to `0o600`, not umask defaults.** The write syscall's `mode` arg bypasses umask. Agents writing public files should explicitly pass a mode override. - **`createServeApp` default `trusted: false`** silently rejects ACP writes with `untrusted_workspace` for embedders that do not inject a custom `fsFactory` or `bridge`. A one-time stderr warning fires the first time; further callers see no reminder. See [`02-serve-runtime.md`](./02-serve-runtime.md). -- **Large text requires an explicit window argument**, any of `line` / `limit` / `maxBytes`. A read with none of them stays `file_too_large`, because a caller that believes it holds the whole file may write it back truncated. Windows stream from an inode-bound handle and never return more than `MAX_READ_BYTES`. -- **`MAX_READ_BYTES` caps what a read returns; `MAX_TEXT_SCAN_BYTES` caps what it costs.** Line offsets are resolved by scanning from byte 0, so `{ line: 900_000_000, limit: 20 }` returns almost nothing and still walks the file. Past 8 MiB of scanning the read is refused with `file_too_large` pointing at `readBytes`, which reaches any offset in O(1). -- **Streamed windows tolerate appends, not truncation.** The full-snapshot path can demand byte-for-byte stability because it returns the whole file; a prefix window cannot, or every read of a live log fails. The streamed path asserts inode identity plus "did not shrink", so appends pass and truncation / replacement are still rejected. `sizeBytes` reports the size at `open`, describing the snapshot the window was cut from. -- **Large partial reads omit the full-file hash.** `originalLineCount` is omitted when streaming stops before EOF. -- **`BridgeFileSystem` adapter MUST replicate both inline-proxy gates** (non-regular-file refusal + bounded buffering/streaming). The inline path is fully bypassed when the adapter is injected. +- **Large text requires a finite line limit.** No-limit reads, line-only reads, and maxBytes-only reads above `MAX_READ_BYTES` remain `file_too_large`. Finite windows stream from an inode-bound handle and never return more than `MAX_READ_BYTES`. +- **Streamed windows require a stable file snapshot.** The open handle pins the inode but does not freeze its bytes, so a successful streamed response requires device/inode identity, size, modification time, and change time to remain unchanged through the read. A detected mutation takes precedence over a simultaneous decode failure and returns `hash_mismatch`. +- **Large partial reads omit the full-file hash.** They retain the complete `sizeBytes`; `originalLineCount` is omitted when streaming stops before EOF. +- **`BridgeFileSystem` adapter MUST preserve both inline-proxy safety properties** (non-regular-file refusal + bounded buffering/streaming). The inline path is fully bypassed when the adapter is injected. ## References diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 92534a5acfe..415f1618ef4 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -1574,22 +1574,25 @@ Filesystem errors use this JSON shape: Reads a text file. Query params: `path` (required), `maxBytes`, `line`, and `limit`. The daemon rejects binary files. Files above the 256 KiB full-snapshot -cap require at least one explicit window argument (`line`, `limit`, or -`maxBytes`); a request with none of them remains `file_too_large`. Such a -window is streamed, and its returned UTF-8 content stays capped at 256 KiB. - -Line offsets are resolved by scanning from the start of the file, so a window -is also refused with `file_too_large` when reaching it would read more than -8 MiB (`MAX_TEXT_SCAN_BYTES`). Use `GET /file/bytes` to reach a deeper offset -directly. Large text in an encoding the route cannot decode returns -`binary_file`, not `file_too_large` — retrying with a smaller window cannot -help, and `readBytes` is the same remedy that already applies to binary. +cap require a finite `limit`; no-limit, line-only, and maxBytes-only requests +remain `file_too_large`. A finite large-file window is streamed and its returned +UTF-8 content remains capped at 256 KiB. `maxBytes` always applies to the UTF-8 +response bytes after decoding, including when the source uses another supported +encoding within the full-snapshot cap. Large text in an encoding the route +cannot decode returns `binary_file` rather than `file_too_large` — retrying +with a smaller window cannot help, and `readBytes` is the same remedy that +already applies to binary content. For files within the full-snapshot cap, the response includes `hash`, a SHA-256 digest over the raw on-disk bytes for the whole file, even when `line`, `limit`, or `maxBytes` returned a slice. Large partial windows omit `hash`, retain the complete `sizeBytes`, set `truncated: true`, and return -`originalLineCount: null` when the stream stops before EOF. +`originalLineCount: null` when the stream stops before EOF. A streamed result +is returned only when the file remains stable. Concurrent changes detected by +the post-read device/inode, size, modification-time, and change-time checks +return `hash_mismatch`, including when the same mutation also causes decoding +to fail. Stable binary content remains `binary_file`, and path replacement +retains the existing `symlink_escape` protection. ```json { diff --git a/packages/cli/src/serve/bridge-file-system-adapter.test.ts b/packages/cli/src/serve/bridge-file-system-adapter.test.ts index d4a7394e6ac..d6468db8cd3 100644 --- a/packages/cli/src/serve/bridge-file-system-adapter.test.ts +++ b/packages/cli/src/serve/bridge-file-system-adapter.test.ts @@ -271,6 +271,25 @@ describe('createBridgeFileSystemAdapter', () => { expect(response.content).toBe(lines.slice(2, 22).join('\n')); }); + it('keeps an oversized ACP line-only read behind the snapshot cap', async () => { + const { MAX_READ_BYTES } = await import('./fs/policy.js'); + const target = path.join(tmpDir, 'large-line-only.txt'); + await fsp.writeFile(target, 'x'.repeat(MAX_READ_BYTES + 1), 'utf8'); + const adapter = createBridgeFileSystemAdapter( + buildFactory({ trusted: true }), + ); + + const err = await adapter + .readText({ + path: target, + sessionId: 'sess:test', + line: 2, + }) + .catch((error: unknown) => error); + + expect((err as { kind?: string }).kind).toBe('file_too_large'); + }); + it('treats null line/limit as undefined (ACP wire compatibility)', async () => { const target = path.join(tmpDir, 'null-window.txt'); await fsp.writeFile(target, 'hello\nworld\n', 'utf8'); diff --git a/packages/cli/src/serve/fs/policy.ts b/packages/cli/src/serve/fs/policy.ts index c51142693fd..938cb6adb70 100644 --- a/packages/cli/src/serve/fs/policy.ts +++ b/packages/cli/src/serve/fs/policy.ts @@ -18,39 +18,19 @@ import type { Intent, ResolvedPath } from './paths.js'; * doesn't fill on a single read. * * 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. + * rather than truncated. `readText` can serve finite-limit 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` as a post-decode UTF-8 + * output cap; `enforceReadSize` also supplies source-size truncation metadata. * - * `enforceReadBytesSize` and `edit()` use the same constant as a - * hard upper bound — multi-GB files in the workspace can no longer - * reach `fsp.readFile` through any boundary path. + * `edit()` uses the same constant as a hard upper bound. + * `readBytesWindow` instead binds an fd and returns at most its explicit + * byte window without buffering the whole file. `enforceReadBytesSize` + * remains available to strict callers that do require a full byte snapshot. */ export const MAX_READ_BYTES = 256 * 1024; -/** - * Upper bound on bytes read off disk to locate a line window above - * `MAX_READ_BYTES`. - * - * `MAX_READ_BYTES` caps what a read *returns*; it says nothing about what a - * read *costs*. Line offsets address a byte stream, so `{ line: 900_000_000, - * limit: 20 }` returns almost nothing and still walks the file from byte 0. - * Without this cap a single query param turns into an uninterruptible - * multi-second scan of an arbitrarily large file, and on Windows it holds a - * read handle (opened without `FILE_SHARE_DELETE`) for that entire span, - * blocking renames and deletes of the target. - * - * 8 MiB is ~25 ms at the ~300 MB/s this streams at — small enough that the - * cost is bounded and the handle-hold window stays negligible, large enough - * to cover the head and tail-ish regions agents actually ask for. Requests - * past it get `file_too_large` pointing at `readBytes`, which reaches any - * offset in O(1). - */ -export const MAX_TEXT_SCAN_BYTES = 8 * 1024 * 1024; - /** * Maximum bytes accepted by `writeText` / `edit`. Sized below the * `express.json({ limit: '10mb' })` middleware cap so a request @@ -223,18 +203,14 @@ export function enforceWriteSize( } /** - * Throw `file_too_large` when `fileBytes` exceeds the hard - * `MAX_READ_BYTES` cap. The production `readBytesWindow` path - * does not call this helper — it reads an explicit bounded window - * from an opened handle — but strict callers and tests can use it - * as a stat-time reject. + * Throw `file_too_large` when a strict full-byte-snapshot caller exceeds the + * hard `MAX_READ_BYTES` cap. The production `readBytesWindow` path does not + * use this helper: it reads an explicit bounded window from an opened handle, + * so a multi-GB file never requires a multi-GB allocation. * * Soft window-read (`opts.maxBytes` truncation) is NOT this - * function's job: `readBytes` truncates the returned buffer - * post-read so a caller asking for `maxBytes: 1024` on a 200 KB - * file gets 1 KB back, matching the parameter's window-read - * promise. Mixing the soft window into the hard reject was the - * round-7 reviewer-flagged contract violation. + * function's job. Mixing a bounded window contract into this strict + * full-snapshot gate would make the helper's safety guarantee ambiguous. */ export function enforceReadBytesSize(fileBytes: number): void { if (fileBytes > MAX_READ_BYTES) { @@ -242,7 +218,7 @@ export function enforceReadBytesSize(fileBytes: number): void { 'file_too_large', `file of ${fileBytes} bytes exceeds read limit of ${MAX_READ_BYTES} bytes`, { - hint: 'use readText for capped truncation, or raise the daemon limit', + hint: 'use an explicit bounded readBytesWindow request for large files', }, ); } diff --git a/packages/cli/src/serve/fs/workspace-file-system.test.ts b/packages/cli/src/serve/fs/workspace-file-system.test.ts index 48b7c30d8fa..1a376135b3c 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.test.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.test.ts @@ -177,7 +177,7 @@ describe('WorkspaceFileSystem - readText', () => { await fsp.writeFile(smallTarget, smallRaw); const smallResolved = await h.fs.resolve('expanded-small.txt', 'read'); const small = await h.fs.readText(smallResolved, { maxBytes: 100 }); - expect(Buffer.byteLength(small.content)).toBeLessThanOrEqual(100); + expect(Buffer.byteLength(small.content)).toBe(99); expect(small.content).not.toContain('\uFFFD'); expect(small.meta.encoding).toBe('utf-16le'); expect(small.meta.truncated).toBe(true); @@ -199,14 +199,21 @@ describe('WorkspaceFileSystem - readText', () => { expect(expanded.meta.truncated).toBe(true); }); - it('throws file_too_large for an oversized read with no window argument', async () => { + it('throws file_too_large for an oversized read without a finite line limit', async () => { const big = path.join(h.workspace, 'huge.txt'); const bytes = (await import('./policy.js')).MAX_READ_BYTES + 1; await fsp.writeFile(big, 'a'.repeat(bytes)); const r = await h.fs.resolve('huge.txt', 'read'); - const err = await h.fs.readText(r).catch((e: unknown) => e); - expect(isFsError(err)).toBe(true); - expect((err as { kind: string }).kind).toBe('file_too_large'); + for (const opts of [ + {}, + { line: 2 }, + { maxBytes: 1024 }, + { line: 2, maxBytes: 1024 }, + ]) { + const err = await h.fs.readText(r, opts).catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('file_too_large'); + } // Audit was recorded for the denial (P0 silent-failure fix). const denied = h.events.find((e) => e.type === FS_DENIED_EVENT_TYPE); expect(denied).toBeDefined(); @@ -215,49 +222,6 @@ describe('WorkspaceFileSystem - readText', () => { ); }); - it('serves oversized text for any explicit window argument, not just limit', async () => { - // `maxBytes` and `line` bound the response just as much as `limit` does; - // refusing them while admitting a deep `line` had the cost model backwards. - const big = path.join(h.workspace, 'huge-window.txt'); - const maxReadBytes = (await import('./policy.js')).MAX_READ_BYTES; - const line = `${'a'.repeat(99)}\n`; - await fsp.writeFile(big, line.repeat(Math.ceil(maxReadBytes / 100) + 10)); - const r = await h.fs.resolve('huge-window.txt', 'read'); - - const capped = await h.fs.readText(r, { maxBytes: 1024 }); - expect(Buffer.byteLength(capped.content)).toBeLessThanOrEqual(1024); - expect(capped.meta.truncated).toBe(true); - expect(capped.meta.hash).toBeUndefined(); - - const fromLine = await h.fs.readText(r, { line: 2 }); - expect(fromLine.content.startsWith('a'.repeat(99))).toBe(true); - expect(Buffer.byteLength(fromLine.content)).toBeLessThanOrEqual( - maxReadBytes, - ); - expect(fromLine.meta.truncated).toBe(true); - }); - - it('refuses a line offset beyond MAX_TEXT_SCAN_BYTES', async () => { - const { MAX_TEXT_SCAN_BYTES } = await import('./policy.js'); - const big = path.join(h.workspace, 'deep-offset.txt'); - const line = `${'a'.repeat(99)}\n`; - const lineCount = Math.ceil((MAX_TEXT_SCAN_BYTES / 100) * 1.5); - await fsp.writeFile(big, line.repeat(lineCount)); - const r = await h.fs.resolve('deep-offset.txt', 'read'); - - // A shallow window on the same file is still cheap and still works. - const head = await h.fs.readText(r, { limit: 2 }); - expect(head.content.split('\n')).toHaveLength(2); - - // The deep one is refused rather than silently costing a full scan. - const err = await h.fs - .readText(r, { line: lineCount - 5, limit: 2 }) - .catch((e: unknown) => e); - expect(isFsError(err)).toBe(true); - expect((err as { kind: string }).kind).toBe('file_too_large'); - expect((err as { hint?: string }).hint).toMatch(/readBytes/); - }); - it('streams bounded line windows from text above MAX_READ_BYTES', async () => { const target = path.join(h.workspace, 'large-window.txt'); const lines = Array.from( @@ -320,9 +284,6 @@ describe('WorkspaceFileSystem - readText', () => { const err = await h.fs.readText(r, { limit: 20 }).catch((e: unknown) => e); expect(isFsError(err)).toBe(true); - // Not `file_too_large`: shrinking the window can never make a GBK file - // decodable, so a client retrying on 413 would loop forever. 422 with the - // readBytes hint is the same remedy that already works for binary. expect((err as { kind: string }).kind).toBe('binary_file'); expect((err as { hint?: string }).hint).toMatch(/convert.*UTF-8/i); }); @@ -416,7 +377,7 @@ describe('WorkspaceFileSystem - readText', () => { } }); - it('rejects a truncation while a large range is being read', async () => { + it('rejects in-place changes while a large range is being read', async () => { const target = path.join(h.workspace, 'large-change.txt'); const lines = Array.from( { length: 4_000 }, @@ -432,7 +393,7 @@ describe('WorkspaceFileSystem - readText', () => { params, ) { const result = await original.call(this, params); - await fsp.truncate(target, 1_000); + await fsp.appendFile(target, '\nchanged'); return result; }); @@ -447,35 +408,136 @@ describe('WorkspaceFileSystem - readText', () => { } }); - it('serves a prefix window from a file being appended to during the read', async () => { - // The whole point of the feature: tailing a live log. A prefix window - // does not depend on the tail, so an append must not fail the read. - const target = path.join(h.workspace, 'large-append.txt'); + it('rejects same-size in-place overwrites during a large range read', async () => { + const target = path.join(h.workspace, 'large-overwrite.txt'); const lines = Array.from( - { length: 4_000 }, + { length: 8_000 }, (_, index) => `line-${index + 1} ${'x'.repeat(80)}`, ); await fsp.writeFile(target, lines.join('\n')); - const resolved = await h.fs.resolve('large-append.txt', 'read'); + const fixedTime = new Date('2026-01-01T00:00:00.000Z'); + await fsp.utimes(target, fixedTime, fixedTime); + const before = await fsp.stat(target); + const resolved = await h.fs.resolve('large-overwrite.txt', 'read'); const original = StandardFileSystemService.prototype.readTextFileFromHandle; - const sizeBefore = (await fsp.stat(target)).size; + let changedAfterFirstChunk = false; const readSpy = vi .spyOn(StandardFileSystemService.prototype, 'readTextFileFromHandle') .mockImplementation(async function ( this: StandardFileSystemService, params, ) { - const result = await original.call(this, params); - await fsp.appendFile(target, `\n${'appended '.repeat(50)}`); - return result; + const originalRead = params.fileHandle.read.bind(params.fileHandle); + params.fileHandle.read = (async ( + buffer: Buffer, + offset: number, + length: number, + position: number, + ) => { + const result = await originalRead(buffer, offset, length, position); + if ( + !changedAfterFirstChunk && + length === 512 * 1024 && + position === 0 + ) { + changedAfterFirstChunk = true; + const writer = await fsp.open(target, 'r+'); + try { + await writer.write(Buffer.from('X'), 0, 1, 600_000); + } finally { + await writer.close(); + } + // Restore mtime to prove ctime still detects a same-size overwrite + // that size+mtime checks alone would accept. Pause first so the + // change-time lands in a later timestamp quantum than the pre-read + // snapshot even on coarse-resolution filesystems. + await new Promise((resolve) => setTimeout(resolve, 50)); + await fsp.utimes(target, before.atime, before.mtime); + } + return result; + }) as typeof params.fileHandle.read; + return original.call(this, params); + }); + + try { + const err = await h.fs + .readText(resolved, { line: 7_000, limit: 20 }) + .catch((e: unknown) => e); + expect(changedAfterFirstChunk).toBe(true); + const after = await fsp.stat(target); + expect(after.size).toBe(before.size); + expect(after.mtimeMs).toBe(before.mtimeMs); + expect(after.ctimeMs).not.toBe(before.ctimeMs); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('hash_mismatch'); + } finally { + readSpy.mockRestore(); + } + }); + + it('prioritizes a read-time mutation over the resulting decode error', async () => { + const target = path.join(h.workspace, 'large-invalidated.txt'); + await fsp.writeFile( + target, + `${'a'.repeat(500_000)}\n${'b'.repeat(200_000)}`, + ); + const fixedTime = new Date('2026-01-01T00:00:00.000Z'); + await fsp.utimes(target, fixedTime, fixedTime); + const before = await fsp.stat(target); + const resolved = await h.fs.resolve('large-invalidated.txt', 'read'); + const original = StandardFileSystemService.prototype.readTextFileFromHandle; + let changedAfterFirstChunk = false; + let capturedHandle: import('node:fs/promises').FileHandle | undefined; + const readSpy = vi + .spyOn(StandardFileSystemService.prototype, 'readTextFileFromHandle') + .mockImplementation(async function ( + this: StandardFileSystemService, + params, + ) { + capturedHandle = params.fileHandle; + const originalRead = params.fileHandle.read.bind(params.fileHandle); + params.fileHandle.read = (async ( + buffer: Buffer, + offset: number, + length: number, + position: number, + ) => { + const result = await originalRead(buffer, offset, length, position); + if ( + !changedAfterFirstChunk && + length === 512 * 1024 && + position === 0 + ) { + changedAfterFirstChunk = true; + const writer = await fsp.open(target, 'r+'); + try { + await writer.write(Buffer.from([0xff]), 0, 1, 550_000); + } finally { + await writer.close(); + } + // Pause so the overwrite's change-time lands in a later timestamp + // quantum than the pre-read snapshot even on coarse-resolution + // filesystems; detection here relies on ctime since mtime is + // restored. + await new Promise((resolve) => setTimeout(resolve, 50)); + await fsp.utimes(target, before.atime, before.mtime); + } + return result; + }) as typeof params.fileHandle.read; + return original.call(this, params); }); try { - const out = await h.fs.readText(resolved, { limit: 20 }); - expect(out.content).toBe(lines.slice(0, 20).join('\n')); - // sizeBytes describes the snapshot the window was cut from, not the - // file as it stands after the concurrent append. - expect(out.meta.sizeBytes).toBe(sizeBefore); + const err = await h.fs + .readText(resolved, { line: 2, limit: 1 }) + .catch((e: unknown) => e); + expect(changedAfterFirstChunk).toBe(true); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('hash_mismatch'); + expect(capturedHandle).toBeDefined(); + await expect(capturedHandle!.stat()).rejects.toMatchObject({ + code: 'EBADF', + }); } finally { readSpy.mockRestore(); } diff --git a/packages/cli/src/serve/fs/workspace-file-system.ts b/packages/cli/src/serve/fs/workspace-file-system.ts index d1dd6f84f20..d86e8ece40e 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.ts @@ -20,7 +20,6 @@ import { glob as globAsync } from 'glob'; import { LargeNonUtf8TextError, StandardFileSystemService, - TextScanBudgetExceededError, decodeBufferWithEncodingInfoAsync, detectLineEnding, encodeTextFileContentAsync, @@ -46,7 +45,6 @@ import { import { BINARY_PROBE_BYTES, MAX_READ_BYTES, - MAX_TEXT_SCAN_BYTES, assertTrustedForIntent, enforceReadSize, enforceWriteSize, @@ -87,13 +85,6 @@ export interface ReadMeta { originalLineCount?: number; } -/** - * Above `MAX_READ_BYTES` at least one of these must be set. Any of them is - * the caller stating it accepts partial content, which is all the streamed - * path returns; with none of them the read is refused rather than silently - * handing back a truncated "whole file". Which one is set does not affect - * cost — that is bounded by `MAX_TEXT_SCAN_BYTES`. - */ export interface ReadTextOptions { /** Returned-byte cap in [1, MAX_READ_BYTES]; defaults to MAX_READ_BYTES. */ maxBytes?: number; @@ -1384,25 +1375,13 @@ async function readTextFromResolvedFile( throw new FsError('parse_error', `path is not a regular file: ${p}`); } - // Any explicit window argument is the caller stating it accepts partial - // content, which is what the large-file path returns. Gating on `limit` - // alone got this backwards in both directions: `{ line: 900_000_000, - // limit: 20 }` was admitted despite costing a full scan, while - // `{ maxBytes: 4096 }` — satisfiable from the first 4 KiB — was refused. - // Cost is bounded by MAX_TEXT_SCAN_BYTES, not by which knob was set. - // - // A read with no window argument at all still fails: an agent that - // believes it holds the whole file may write it back truncated. The - // omitted `hash` blocks that for `editText`/`writeTextAtomic`, but - // `writeTextOverwrite` takes no hash, so `truncated: true` is the only - // signal on that path — refusing the unbounded read keeps the caller - // from ever being in that position by accident. - const wantsWindow = - opts.limit !== undefined || - opts.maxBytes !== undefined || - opts.line !== undefined; - if (pre.size > MAX_READ_BYTES && wantsWindow) { - return readLargeTextWindowFromResolvedFile(p, pre, opts, lowFs); + if (pre.size > MAX_READ_BYTES && opts.limit !== undefined) { + return readLargeTextWindowFromResolvedFile( + p, + pre, + { ...opts, limit: opts.limit }, + lowFs, + ); } return readTextSnapshotFromResolvedFile(p, opts, pre); } @@ -1448,7 +1427,8 @@ async function readTextSnapshotFromResolvedFile( startLineIndex, opts.limit ?? Number.POSITIVE_INFINITY, ); - const sizeOutcome = enforceReadSize(raw.length, opts.maxBytes); + const maxOutputBytes = opts.maxBytes ?? MAX_READ_BYTES; + const sizeOutcome = enforceReadSize(raw.length, maxOutputBytes); let content = sliced.content; const meta: TextSnapshot['meta'] = { encoding: decoded.encoding, @@ -1459,13 +1439,13 @@ async function readTextSnapshotFromResolvedFile( hash: hashBuffer(raw), }; - const outputCap = opts.maxBytes ?? MAX_READ_BYTES; - const decodedBuf = Buffer.from(content, 'utf-8'); - if (decodedBuf.length > outputCap) { - content = safeUtf8Truncate(decodedBuf, outputCap).toString('utf-8'); + const output = Buffer.from(content, 'utf-8'); + if (output.length > maxOutputBytes) { + content = safeUtf8Truncate(output, maxOutputBytes).toString('utf-8'); meta.lineEnding = detectLineEnding(content); meta.truncated = true; - } else if (sizeOutcome.truncated) { + } + if (sizeOutcome.truncated) { meta.truncated = true; } @@ -1480,143 +1460,127 @@ async function readTextSnapshotFromResolvedFile( return { content, meta }; } -/** - * Stability check for a streamed *prefix* window. - * - * The full-snapshot path can demand byte-for-byte stability (`size` and - * `mtimeMs` unchanged) because it returns the whole file: any change - * invalidates the result. A line window does not return the whole file, so - * demanding whole-file stability rejects reads whose returned bytes are - * still perfectly valid — and the case it rejects is the one this feature - * exists for. Appending to a log does not change lines 1-20, but under an - * equality check every read of a live log is a coin flip. - * - * So the streamed path asserts only what the returned bytes depend on: the - * inode is unchanged (`assertSameFile`, checked separately) and the file did - * not shrink. Truncation, `>` rewrites, and replacement are still rejected. - * - * The residual gap is a writer that truncates *and* regrows past the - * original size inside one read window while keeping the same inode. That is - * narrower than what an equality check on `mtimeMs` catches, and it is the - * price of supporting append-only files at all. - */ -function assertDidNotShrink( - before: { size: number | bigint }, - after: { size: number | bigint }, - p: ResolvedPath, - reason: string, -): void { - if (toBigInt(after.size) < toBigInt(before.size)) { - throw new FsError('hash_mismatch', `${reason}: ${p}`, { - hint: 'retry after re-reading the latest file', - }); - } -} - async function readLargeTextWindowFromResolvedFile( p: ResolvedPath, pre: Awaited>, - opts: ReadTextOptions, + opts: ReadTextOptions & { limit: number }, lowFs: StandardFileSystemService, ): Promise { const fh = await fsp.open(p as string, 'r'); - let opened: Awaited> | undefined; - let content = ''; - let readMeta: - | Awaited< - ReturnType - >['_meta'] - | undefined; try { - opened = await fh.stat(); + const opened = await fh.stat(); assertSameFile(pre, opened, p as string, 'read'); - assertDidNotShrink(pre, opened, p, 'file was truncated before read'); - - const probe = Buffer.alloc(Math.min(BINARY_PROBE_BYTES, opened.size)); - if (probe.length > 0) { - const { bytesRead } = await fh.read(probe, 0, probe.length, 0); - if (looksBinary(probe.subarray(0, bytesRead))) { - throw new FsError('binary_file', `binary file: ${p}`, { - hint: 'use readBytes for binary content', - }); - } + if (didFileVersionChange(pre, opened)) { + throw new FsError('hash_mismatch', `file changed before read: ${p}`, { + hint: 'retry after re-reading the latest file', + }); } - let result: Awaited< - ReturnType - >; + let result: + | Awaited> + | undefined; + let primaryError: unknown; + let hasPrimaryError = false; try { + const probe = Buffer.alloc(Math.min(BINARY_PROBE_BYTES, opened.size)); + if (probe.length > 0) { + const { bytesRead } = await fh.read(probe, 0, probe.length, 0); + if (looksBinary(probe.subarray(0, bytesRead))) { + throw new FsError('binary_file', `binary file: ${p}`, { + hint: 'use readBytes for binary content', + }); + } + } + result = await lowFs.readTextFileFromHandle({ fileHandle: fh, - limit: opts.limit ?? Number.POSITIVE_INFINITY, + limit: opts.limit, line: opts.line !== undefined ? opts.line - 1 : 0, maxOutputBytes: opts.maxBytes ?? MAX_READ_BYTES, - maxScanBytes: MAX_TEXT_SCAN_BYTES, }); } catch (err) { + hasPrimaryError = true; + primaryError = err; + } + + const afterRead = await fh.stat(); + const post = await fsp.lstat(p as string); + if (post.isSymbolicLink()) { + throw new FsError( + 'symlink_escape', + `path was replaced with a symlink during read: ${p}`, + { hint: 'TOCTOU swap detected via post-read lstat' }, + ); + } + assertSameFile(opened, afterRead, p as string, 'read'); + assertSameFile(opened, post, p as string, 'read'); + if ( + didFileVersionChange(opened, afterRead) || + didFileVersionChange(opened, post) + ) { + throw new FsError('hash_mismatch', `file changed during read: ${p}`, { + hint: 'retry after re-reading the latest file', + }); + } + + if (hasPrimaryError) { // An encoding the text route can't represent is the same class of - // refusal as sniffed-binary content, and `binary_file` already tells - // clients to fall back to `readBytes` — which is the right remedy for - // a GBK file too. `file_too_large` (413) would send a client that - // retries with a smaller window into a loop it can never exit. - if (err instanceof LargeNonUtf8TextError) { - throw new FsError('binary_file', err.message, { - cause: err, + // refusal as sniffed-binary content; `binary_file` already tells + // clients to fall back to `readBytes`. `file_too_large` (413) would + // send a client that retries with a smaller window into a loop it + // can never exit. + if (primaryError instanceof LargeNonUtf8TextError) { + throw new FsError('binary_file', primaryError.message, { + cause: primaryError, hint: 'convert the file to UTF-8, or use readBytes for the raw bytes', }); } - // Budget exhaustion *is* a size refusal: the file is too large to - // reach this line offset by scanning. 413 is correct, and the hint - // names the O(1) alternative so the retry has somewhere to go. - if (err instanceof TextScanBudgetExceededError) { - throw new FsError('file_too_large', err.message, { - cause: err, - hint: `line offsets are resolved by scanning from byte 0 and stop after ${MAX_TEXT_SCAN_BYTES} bytes; use readBytes to reach a deeper offset directly`, - }); - } - throw err; + throw primaryError; } - content = result.content; - readMeta = result._meta; - const afterRead = await fh.stat(); - assertSameFile(opened, afterRead, p as string, 'read'); - assertDidNotShrink(opened, afterRead, p, 'file was truncated during read'); + if (result === undefined) { + throw new FsError( + 'internal_error', + `large text range read returned no result: ${p}`, + ); + } + + const meta: TextReadOutcome['meta'] = { + encoding: result._meta?.encoding, + bom: result._meta?.bom, + lineEnding: detectLineEnding(result.content), + sizeBytes: opened.size, + truncated: true, + }; + if ( + result._meta?.originalLineCountExact === true && + result._meta.originalLineCount !== undefined + ) { + meta.originalLineCount = result._meta.originalLineCount; + } + return { content: result.content, meta }; } finally { await fh.close(); } +} - if (opened === undefined) { - throw new FsError('internal_error', `failed to stat opened file: ${p}`); - } - const post = await fsp.lstat(p as string); - if (post.isSymbolicLink()) { - throw new FsError( - 'symlink_escape', - `path was replaced with a symlink during read: ${p}`, - { hint: 'TOCTOU swap detected via post-read lstat' }, - ); - } - assertSameFile(opened, post, p as string, 'read'); - assertDidNotShrink(opened, post, p, 'file was truncated during read'); - - const meta: TextReadOutcome['meta'] = { - encoding: readMeta?.encoding, - bom: readMeta?.bom, - lineEnding: detectLineEnding(content), - // Size as of `open`, not as of now: it describes the snapshot the - // returned window was cut from. A file that grew during the read - // reports the smaller, consistent number. - sizeBytes: opened.size, - truncated: true, - }; - if ( - readMeta?.originalLineCountExact === true && - readMeta?.originalLineCount !== undefined - ) { - meta.originalLineCount = readMeta.originalLineCount; - } - return { content, meta }; +function didFileVersionChange( + before: { + size: number | bigint; + mtimeMs: number | bigint; + ctimeMs: number | bigint; + }, + after: { + size: number | bigint; + mtimeMs: number | bigint; + ctimeMs: number | bigint; + }, +): boolean { + return ( + after.size !== before.size || + after.mtimeMs !== before.mtimeMs || + after.ctimeMs !== before.ctimeMs + ); } async function readStableRegularFileBuffer( @@ -1747,12 +1711,10 @@ async function atomicWriteTextResolvedFile( const target = input.target as string; const parent = path.dirname(target); const parentStat = await fsp.lstat(parent); - // Defense-in-depth against a parent-symlink swap. A full fix - // requires parent-fd / `openat`-style publish (Node stdlib does - // not expose this) — tracked alongside the fd-based read - // follow-up referenced by `assertInodeStableAfterRead`. This - // guard at least surfaces an obviously-swapped parent before - // we open the temp file or rename through it. + // Defense-in-depth against a parent-symlink swap. A full fix requires + // parent-fd / `openat`-style publish, which Node stdlib does not expose. + // This guard at least surfaces an obviously-swapped parent before we open + // the temp file or rename through it. if (parentStat.isSymbolicLink()) { throw new FsError('symlink_escape', `parent path is a symlink: ${parent}`, { hint: 're-resolve the target after detecting parent-symlink swaps', @@ -2174,20 +2136,12 @@ function safeUtf8Truncate(buf: Buffer, maxBytes: number): Buffer { } /** - * Post-read TOCTOU guard. After reading the file at `p`, re-`lstat` - * to confirm the inode hasn't changed and the path isn't now a - * symlink. Catches the swap-then-leave attack where a regular - * file is replaced with a symlink to outside the workspace - * BETWEEN the boundary's pre-stat and the actual read — the - * pre-stat saw the original (small, regular) file but the read - * followed the swap to wherever the attacker pointed. There's a - * residual race where the attacker swaps back after our read but - * before this check; that window is much smaller than the swap- - * and-leave attack and outside this module's threat model. The proper - * fix is fd-based reading (`fsp.open` + `fileHandle.read`) so the - * fd binds to the inode at open time; that's a follow-up since it - * requires a new variant of `lowFs.readTextFile` that takes a - * FileHandle instead of a path. + * Post-read pathname guard for handle-bound byte reads. The content read is + * already tied to the opened inode; re-`lstat` confirms the requested path + * still names that inode and was not replaced by a symlink before the response + * is emitted. A swap after this final check remains outside the module's + * point-in-time guarantee, but cannot change the bytes already read from the + * original handle. */ async function assertInodeStableAfterRead( p: string, diff --git a/packages/cli/src/serve/routes/workspace-file-read.test.ts b/packages/cli/src/serve/routes/workspace-file-read.test.ts index cd6e65eff30..9c9824cdecb 100644 --- a/packages/cli/src/serve/routes/workspace-file-read.test.ts +++ b/packages/cli/src/serve/routes/workspace-file-read.test.ts @@ -172,6 +172,24 @@ describe('GET /file', () => { expect(res.body.hash).toBeUndefined(); }); + it.each(['', '&line=2', '&maxBytes=1024', '&line=2&maxBytes=1024'])( + 'keeps oversized reads without a finite limit behind the snapshot cap (%s)', + async (query) => { + const { MAX_READ_BYTES } = await import('../fs/policy.js'); + await fsp.writeFile( + path.join(h.workspace, 'large-no-limit.txt'), + 'x'.repeat(MAX_READ_BYTES + 1), + ); + + const res = await request(h.app) + .get(`/file?path=large-no-limit.txt${query}`) + .set('Host', loopbackHost()); + + expect(res.status).toBe(413); + expect(res.body.errorKind).toBe('file_too_large'); + }, + ); + it('attaches Cache-Control: no-store and X-Content-Type-Options: nosniff', async () => { await fsp.writeFile(path.join(h.workspace, 'a.txt'), 'x'); const res = await request(h.app) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ec0196746ac..6254a2b6920 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -263,10 +263,7 @@ export { decodeBufferWithEncodingInfo, encodeTextFileContent, } from './utils/sync-file-encoding.js'; -export { - LargeNonUtf8TextError, - TextScanBudgetExceededError, -} from './utils/read-text-range.js'; +export { LargeNonUtf8TextError } from './utils/read-text-range.js'; export * from './services/gitWorktreeService.js'; export { DEFAULT_MAX_TOOL_CALLS_PER_TURN } from './services/loopDetectionService.js'; export * from './services/visionBridge/vision-bridge-service.js'; diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index e1b1d8e7b51..96e6625082c 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -190,9 +190,7 @@ describe('StandardFileSystemService', () => { // argument validation below needs a unit test, and it needs no mock. it.each([ ['maxOutputBytes', { maxOutputBytes: Number.POSITIVE_INFINITY }], - ['maxScanBytes', { maxScanBytes: Number.POSITIVE_INFINITY }], ['maxOutputBytes', { maxOutputBytes: 0 }], - ['maxScanBytes', { maxScanBytes: -1 }], ])('should reject a handle read with unbounded %s', async (bound, over) => { const fileHandle = {} as import('node:fs/promises').FileHandle; @@ -200,8 +198,6 @@ describe('StandardFileSystemService', () => { fileSystem.readTextFileFromHandle({ fileHandle, limit: 20, - maxOutputBytes: 262_144, - maxScanBytes: 8 * 1024 * 1024, ...over, }), ).rejects.toThrow(new RegExp(`positive finite ${bound}`)); @@ -219,7 +215,6 @@ describe('StandardFileSystemService', () => { fileHandle, limit, maxOutputBytes: 262_144, - maxScanBytes: 8 * 1024 * 1024, }), ).rejects.toThrow(/positive integer limit or Infinity/); }); @@ -236,7 +231,6 @@ describe('StandardFileSystemService', () => { line, limit: 20, maxOutputBytes: 262_144, - maxScanBytes: 8 * 1024 * 1024, }), ).rejects.toThrow(/non-negative integer line/); }); diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index a23aabeac27..b2a8b52b32f 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -59,11 +59,10 @@ export type CoreReadTextFileRequest = Omit< * an `Omit` chain would strip more than it kept and would keep re-admitting * fields — `path`, `stats` — that this path has no use for. * - * Both byte bounds are required rather than optional: what makes a large-file - * read safe at a boundary is that the *returned* bytes and the *scanned* bytes - * are each capped. A finite `limit` is not one of those bounds — `limit: 20` at - * `line: 900_000_000` still walks the whole file — so it stays optional and - * `maxScanBytes` is what actually keeps the read affordable. + * `maxOutputBytes` is required rather than optional: what makes a large-file + * read safe at a boundary is that the *returned* bytes are capped. `limit` + * stays optional — a caller may read to end of file — and the borrowing + * boundary decides which reads to admit. */ export interface CoreReadTextFileHandleRequest { fileHandle: FileHandle; @@ -71,7 +70,6 @@ export interface CoreReadTextFileHandleRequest { line?: number | null; limit?: number; maxOutputBytes: number; - maxScanBytes: number; signal?: AbortSignal; } @@ -339,11 +337,6 @@ export class StandardFileSystemService implements FileSystemService { `handle-bound text reads require a positive finite maxOutputBytes, got ${params.maxOutputBytes}`, ); } - if (!isPositiveSafeInteger(params.maxScanBytes)) { - throw new RangeError( - `handle-bound text reads require a positive finite maxScanBytes, got ${params.maxScanBytes}`, - ); - } if ( params.limit !== undefined && params.limit !== Number.POSITIVE_INFINITY && @@ -366,7 +359,6 @@ export class StandardFileSystemService implements FileSystemService { offset: params.line ?? 0, limit: params.limit ?? Number.POSITIVE_INFINITY, maxOutputBytes: params.maxOutputBytes, - maxScanBytes: params.maxScanBytes, ...(params.signal !== undefined ? { signal: params.signal } : {}), }); return toReadTextFileResponse(range); diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index 65c6ff4af7d..5647518b282 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -11,7 +11,6 @@ import * as path from 'node:path'; import { iconvEncode } from './iconvHelper.js'; import { LargeNonUtf8TextError, - TextScanBudgetExceededError, readTextRange, readTextRangeFromHandle, } from './read-text-range.js'; @@ -105,7 +104,6 @@ describe('readTextRange', () => { offset: 1_500, limit: 3, maxOutputBytes: 10_000, - maxScanBytes: Number.MAX_SAFE_INTEGER, }); expect(result.content.split('\n')).toEqual([ @@ -119,7 +117,6 @@ describe('readTextRange', () => { offset: 10_000, limit: 3, maxOutputBytes: 10_000, - maxScanBytes: Number.MAX_SAFE_INTEGER, }); expect(beyondEof.content).toBe(''); expect(beyondEof.originalLineCount).toBe(2_000); @@ -152,7 +149,6 @@ describe('readTextRange', () => { offset: 0, limit: 2, maxOutputBytes: 1_024, - maxScanBytes: Number.MAX_SAFE_INTEGER, }); expect(result.content).toBe('safe-one\nsafe-two'); @@ -162,63 +158,6 @@ describe('readTextRange', () => { } }); - it('refuses a line offset that cannot be reached within maxScanBytes', async () => { - const filePath = await writeFile('budget.log', largeUtf8Lines(5_000)); - - await expect( - readTextRange({ - path: filePath, - offset: 4_000, - limit: 20, - maxOutputBytes: 262_144, - maxScanBytes: 100_000, - }), - ).rejects.toBeInstanceOf(TextScanBudgetExceededError); - }); - - it('serves a shallow window from a file far larger than maxScanBytes', async () => { - const filePath = await writeFile('budget-head.log', largeUtf8Lines(5_000)); - - const result = await readTextRange({ - path: filePath, - offset: 0, - limit: 3, - maxOutputBytes: 262_144, - maxScanBytes: 100_000, - }); - - expect(result.content.split('\n')).toEqual([ - expect.stringContaining('line-1 '), - expect.stringContaining('line-2 '), - expect.stringContaining('line-3 '), - ]); - }); - - it('does not charge a budget failure to a file that ends within it', async () => { - // The scan reaches EOF on the same chunk that exhausts the budget; the - // window was fully satisfied, so there is nothing to refuse. - // Goes through the handle variant purely because that is the one that - // always streams: this file is far too small to leave the path variant's - // buffering fast path, and the buffered path never consults the budget. - const body = largeUtf8Lines(100); - const filePath = await writeFile('budget-exact.log', body); - const fileHandle = await fs.open(filePath, 'r'); - - const result = await readTextRangeFromHandle(fileHandle, { - offset: 98, - limit: 10, - maxOutputBytes: 262_144, - maxScanBytes: Buffer.byteLength(body), - }).finally(() => fileHandle.close()); - - expect(result.content.split('\n')).toEqual([ - expect.stringContaining('line-99 '), - expect.stringContaining('line-100 '), - ]); - expect(result.originalLineCount).toBe(100); - expect(result.originalLineCountExact).toBe(true); - }); - it('streams a large UTF-8 file from the beginning when no range is provided', async () => { const filePath = await writeFile('large.log', largeUtf8Lines(65_000)); diff --git a/packages/core/src/utils/read-text-range.ts b/packages/core/src/utils/read-text-range.ts index 4aa1a5b2f11..bdaded73b8b 100644 --- a/packages/core/src/utils/read-text-range.ts +++ b/packages/core/src/utils/read-text-range.ts @@ -21,32 +21,21 @@ export interface ReadTextRangeRequest { maxOutputBytes: number; signal?: AbortSignal; stats?: Stats; - /** - * Upper bound on bytes read off disk while locating the requested window. - * Line offsets address a byte stream, so a deep `offset` costs a scan from - * byte 0 — this is what keeps that scan from being unbounded. Defaults to - * `Infinity` so non-boundary callers (the `read_file` tool) are unchanged. - * The real security boundary is the handle variant - * ({@link ReadTextRangeFromHandleRequest}), where this field is required; - * no production caller of the path variant sets a finite value. - */ - maxScanBytes?: number; } /** * Request shape for {@link readTextRangeFromHandle}. * * No `path`: the read is bound to the descriptor, so there is nothing for a - * path to disambiguate. Both byte bounds are required rather than optional — - * a handle-bound read exists because some caller pinned an inode at a security - * boundary, and what makes such a read safe is that the bytes it *returns* and - * the bytes it *scans* are each capped. + * path to disambiguate. `maxOutputBytes` is required rather than optional — a + * handle-bound read exists because some caller pinned an inode at a security + * boundary, and what makes such a read safe is that the bytes it *returns* are + * capped. */ export interface ReadTextRangeFromHandleRequest { offset?: number; limit?: number; maxOutputBytes: number; - maxScanBytes: number; signal?: AbortSignal; } @@ -74,40 +63,14 @@ export class LargeNonUtf8TextError extends Error { } } -/** - * Raised when locating the requested line window would require reading more - * than `maxScanBytes`. Distinct from `LargeNonUtf8TextError`: the file is - * readable, the *offset* is what cannot be reached affordably. - */ -export class TextScanBudgetExceededError extends Error { - constructor( - readonly scannedBytes: number, - readonly maxScanBytes: number, - ) { - super( - `Locating the requested line window would read more than ${maxScanBytes} bytes (line offsets are resolved by scanning from the start of the file). Use a byte-offset read to reach this part of the file.`, - ); - this.name = 'TextScanBudgetExceededError'; - } -} - export async function readTextRange( request: ReadTextRangeRequest, ): Promise { request.signal?.throwIfAborted(); const stats = request.stats ?? (await stat(request.path)); const maxOutputBytes = normalizeMaxBytes(request.maxOutputBytes); - const maxScanBytes = request.maxScanBytes ?? Number.POSITIVE_INFINITY; - // The fast path buffers the whole file, so it reads `stats.size` bytes no - // matter how small the window is — a budget that only constrained the - // streaming path would not be a budget. Falling through to streaming lets - // the same bound apply, and raises `TextScanBudgetExceededError` if the - // window really is out of reach. - if ( - stats.size < TEXT_RANGE_FAST_PATH_MAX_SIZE && - stats.size <= maxScanBytes - ) { + if (stats.size < TEXT_RANGE_FAST_PATH_MAX_SIZE) { const { content, encoding, bom } = await readFileWithEncodingInfo( request.path, request.signal, @@ -127,12 +90,7 @@ export async function readTextRange( }; } - return readLargeUtf8Range( - request.path, - request, - maxOutputBytes, - maxScanBytes, - ); + return readLargeUtf8Range(request.path, request, maxOutputBytes); } /** @@ -152,7 +110,6 @@ export async function readTextRangeFromHandle( fileHandle, request, normalizeMaxBytes(request.maxOutputBytes), - request.maxScanBytes, ); } @@ -200,7 +157,6 @@ async function readLargeUtf8Range( source: string | FileHandle, request: { offset?: number; limit?: number; signal?: AbortSignal }, maxOutputBytes: number, - maxScanBytes: number, ): Promise { const encoding = await detectFileEncoding(source); // Detection is one bounded 8 KiB read, but check here anyway so an abort @@ -223,8 +179,6 @@ async function readLargeUtf8Range( let previousChunkEndedWithCR = false; let originalLineCountExact = true; let stoppedEarly = false; - let scannedBytes = 0; - let budgetExhausted = false; const decoder = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true, @@ -279,15 +233,6 @@ async function readLargeUtf8Range( try { for await (const rawChunk of chunks) { request.signal?.throwIfAborted(); - // Checked on arrival of the *next* chunk rather than after consuming - // the current one: reaching here at all proves there was more file to - // read, which is what separates "budget ran out mid-file" from "the - // file happened to end on the budget". Costs one chunk of overshoot. - if (scannedBytes >= maxScanBytes) { - budgetExhausted = true; - break; - } - scannedBytes += (rawChunk as Buffer).length; let chunk = decodeUtf8Chunk(rawChunk as Buffer, { stream: true }); if (firstChunk) { firstChunk = false; @@ -339,10 +284,6 @@ async function readLargeUtf8Range( } } - if (budgetExhausted) { - throw new TextScanBudgetExceededError(scannedBytes, maxScanBytes); - } - if (!stoppedEarly) { decodeUtf8Chunk(); } From bc4d4c6aae36e7d6f0e5197dc4c428e2718cf46b Mon Sep 17 00:00:00 2001 From: "qwen-autofix[bot]" Date: Wed, 29 Jul 2026 19:53:40 +0000 Subject: [PATCH 07/16] test(core): port deep-read and encoding-probe tests to the handle path (#7967) --- .../core/src/utils/read-text-range.test.ts | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index 5647518b282..594f3ab6310 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -129,6 +129,53 @@ describe('readTextRange', () => { } }); + it('reads a handle-bound range beyond 10 MiB with a byte cap', async () => { + const content = largeUtf8Lines(65_000); + const marker = 'line-60001'; + expect( + Buffer.byteLength(content.slice(0, content.indexOf(marker))), + ).toBeGreaterThan(10 * 1024 * 1024); + const filePath = await writeFile('deep-handle.log', content); + const fileHandle = await fs.open(filePath, 'r'); + try { + const result = await readTextRangeFromHandle(fileHandle, { + offset: 60_000, + limit: 3, + maxOutputBytes: 256, + }); + + expect(result.content).toMatch(/^line-60001 /); + expect(Buffer.byteLength(result.content)).toBeLessThanOrEqual(256); + expect(result.content).not.toContain('\uFFFD'); + expect(result.truncatedByBytes).toBe(true); + expect(result.originalLineCountExact).toBe(false); + } finally { + await fileHandle.close(); + } + }); + + it('accepts handle-bound UTF-8 when the encoding sample splits an emoji', async () => { + const firstLine = `${'a'.repeat(8191)}🙂`; + const filePath = await writeFile( + 'split-encoding-sample.log', + `${firstLine}\n${'b'.repeat(300_000)}`, + ); + const fileHandle = await fs.open(filePath, 'r'); + try { + const result = await readTextRangeFromHandle(fileHandle, { + offset: 0, + limit: 1, + maxOutputBytes: 16_384, + }); + + expect(result.content).toBe(firstLine); + expect(result.encoding).toBe('utf-8'); + expect(result.content).not.toContain('\uFFFD'); + } finally { + await fileHandle.close(); + } + }); + it('reads the pinned inode after the path is replaced underneath it', async () => { // The handle variant takes no path, so "read the wrong file" is no longer // representable. What is still worth pinning down is the property that From 9c0e08d9986b6d0a11719fa4cdf7fdf95a28395a Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Wed, 29 Jul 2026 21:37:45 +0000 Subject: [PATCH 08/16] test(core): pin handle read-to-EOF and unlimited-limit contracts (#7967) --- .../core/src/utils/read-text-range.test.ts | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index 594f3ab6310..00d01335e42 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -205,6 +205,48 @@ describe('readTextRange', () => { } }); + it('reads the full file when limit is omitted or Infinity', async () => { + const filePath = await writeFile('small.txt', 'one\ntwo\nthree\n'); + const fileHandle = await fs.open(filePath, 'r'); + try { + const omitted = await readTextRangeFromHandle(fileHandle, { + maxOutputBytes: 10_000, + }); + expect(omitted.content).toBe('one\ntwo\nthree\n'); + expect(omitted.originalLineCount).toBe(4); + expect(omitted.originalLineCountExact).toBe(true); + expect(omitted.truncatedByBytes).toBe(false); + + const infinite = await readTextRangeFromHandle(fileHandle, { + limit: Number.POSITIVE_INFINITY, + maxOutputBytes: 10_000, + }); + expect(infinite.content).toBe('one\ntwo\nthree\n'); + expect(infinite.originalLineCount).toBe(4); + expect(infinite.originalLineCountExact).toBe(true); + } finally { + await fileHandle.close(); + } + }); + + it('reads to live EOF: bytes appended after open are returned', async () => { + const filePath = await writeFile('append.txt', 'first\n'); + const fileHandle = await fs.open(filePath, 'r'); + try { + await fs.appendFile(filePath, 'second\n'); + + const result = await readTextRangeFromHandle(fileHandle, { + maxOutputBytes: 10_000, + }); + + expect(result.content).toBe('first\nsecond\n'); + expect(result.originalLineCount).toBe(3); + expect(result.originalLineCountExact).toBe(true); + } finally { + await fileHandle.close(); + } + }); + it('streams a large UTF-8 file from the beginning when no range is provided', async () => { const filePath = await writeFile('large.log', largeUtf8Lines(65_000)); From 67ad71f242e8ed1866dd88e36b51442af2222e40 Mon Sep 17 00:00:00 2001 From: "qwen-autofix[bot]" Date: Thu, 30 Jul 2026 06:14:24 +0000 Subject: [PATCH 09/16] docs(serve): disclose handle-read behaviour deltas and add boundary tests (#7967) Address maintainer re-verification (R2). The refactor's code is unchanged; this makes the documentation describe the actual diff and pins the remaining untested handle-path contracts. - Rewrite the design doc to match what the diff does against current main (one detector, two entry points, chunksFromHandle, standalone CoreReadTextFileHandleRequest) instead of the superseded intermediate state, and add a Behaviour deltas section disclosing the read-to-EOF chunk reader, the fresh-buffer-per-chunk change, the limit: Infinity admission, and the binary_file mapping for undecodable large text. - State the handle-layer omitted/infinite limit contract in qwen-serve-protocol.md, reconciled with the unchanged GET /file admission. - Add handle-bound abort tests (pre-aborted and mid-stream) to read-text-range.test.ts and an HTTP-layer test asserting a large non-UTF-8 file requested with a finite limit returns 422 binary_file. --- ...026-07-29-handle-bound-text-range-reads.md | 133 ++++++++++++------ docs/developers/qwen-serve-protocol.md | 5 +- .../serve/routes/workspace-file-read.test.ts | 17 +++ .../core/src/utils/read-text-range.test.ts | 40 ++++++ 4 files changed, 152 insertions(+), 43 deletions(-) diff --git a/docs/design/2026-07-29-handle-bound-text-range-reads.md b/docs/design/2026-07-29-handle-bound-text-range-reads.md index 6768b6a15b6..ac24013e740 100644 --- a/docs/design/2026-07-29-handle-bound-text-range-reads.md +++ b/docs/design/2026-07-29-handle-bound-text-range-reads.md @@ -5,27 +5,16 @@ PR #7947 let the Serve workspace filesystem return bounded line windows from text files above `MAX_READ_BYTES` (256 KiB). To keep those reads pinned to one inode across validation, binary probing, and streaming, it threaded a -caller-owned `FileHandle` down into `readTextRange` as an optional field, and -added a second optional field, `forceStreaming`, to suppress the buffering fast -path that would otherwise defeat the memory bound. - -Two optional fields on one entry point produced four combinations, of which one -is meaningful, one is unreachable, and one is unsafe: - -| `fileHandle` | `forceStreaming` | Result | -| ------------ | ---------------- | ---------------------------------------------------------------------- | -| unset | unset | ordinary path read | -| unset | set | streams a small file — used by one test | -| set | set | the Serve boundary's read | -| set | unset | buffers the whole file through the handle — **no caller can reach it** | - -The unreachable combination carried a dedicated helper, `readFileHandleBuffer`, -with no test coverage. Separately, `readFileWithLineAndLimit` accepted the same -`fileHandle` but could only honor it on its range branch: an unbounded read fell -through to a by-path `readFileWithEncodingInfo`, silently returning bytes from -whatever the path resolved to at that moment rather than from the pinned inode. -PR #7947's follow-up commit guarded that with a runtime `RangeError`, which -documented the trap without removing it. +caller-owned `FileHandle` down into `readTextRange` as an optional field. When +the field was set the read always streamed — the buffering fast path does not +apply to a handle read — through a private `readFileHandleChunks(handle, +sourceSize, signal)` bounded by the size captured at `open`. + +An optional field on a shared entry point expressed the handle path as a branch +inside `readTextRange` rather than as its own operation: the request type still +carried `path` and `stats` that a handle read never uses, and the streaming +implementation chose between `createReadStream` and the handle chunk reader by +testing whether the field was present. Encoding detection had forked for the same reason. `detectFileEncoding` takes a path and opens its own descriptor, so the handle path could not use it; a @@ -39,10 +28,11 @@ refuse the file, with different messages. ## Goals - One encoding detector, usable from a path or a borrowed descriptor. -- No mode flags on the range reader; make the unreachable combination - unrepresentable rather than merely unused. -- Make the by-path fallthrough structurally impossible instead of guarded. -- No observable change at the Serve boundary or in the `read_file` tool. +- No mode flags and no optional handle field on the range reader; the handle + path is its own entry point, so a path read and a handle read can no longer + share a request shape. +- No observable change at the Serve boundary or in the `read_file` tool, apart + from the deliberately disclosed deltas listed below. ## Non-goals @@ -50,8 +40,10 @@ refuse the file, with different messages. variant is a deliberate public-API compatibility shim ([`lazy-first-use-dependencies.md`](./lazy-first-use-dependencies.md)) pinned by a parity test. -- Any change to what the Serve boundary returns. This is preparation for - byte-cursor paging, not that feature. +- Any change to what the Serve boundary admits. The large-file admission + contract (a finite `limit` is required above `MAX_READ_BYTES`) is #7947's and + is not revisited here; this is preparation for byte-cursor paging, not that + feature. ## Design @@ -96,23 +88,68 @@ file — and the borrowing boundary decides which reads to admit. Both delegate to the same streaming implementation, which now takes `source: string | FileHandle` and selects `createReadStream` or -`chunksFromHandle` accordingly. `readFileHandleBuffer` and the branch that -called it are deleted. +`chunksFromHandle` accordingly. The old handle chunk reader +(`readFileHandleChunks`) becomes `chunksFromHandle(fh, from)`: it gains a start +offset and drops the open-time size bound, and allocates a fresh buffer per +chunk instead of reusing one. Both changes are detailed under Behaviour deltas. -### The fallthrough disappears +### The service layer -`readFileWithLineAndLimit` loses `fileHandle` and `forceStreaming` — its single -production caller passes neither. `StandardFileSystemService.readTextFileFromHandle` now calls -`readTextRangeFromHandle` directly, and the two read paths share a -`toReadTextFileResponse` helper so their metadata shaping cannot drift. With no -`fileHandle` parameter left to ignore, the `RangeError` guard is removed: the -trap it described can no longer be expressed. +`readTextRangeFromHandle` directly instead of routing through `readTextRange` +with an optional handle field, and `readTextFile`'s body is extracted to a +module-level `readTextFileStandard`. Both read paths share a +`toReadTextFileResponse` helper — typed structurally rather than as a union of +the two result shapes — so their metadata shaping cannot drift. + +The argument-validation `RangeError` guards are kept, rewired so +`maxOutputBytes` is the required positive-finite field while `limit` becomes +optional and `Infinity`-tolerant (a caller may read to end of file). The +`limit: Infinity` admission this introduces is disclosed under Behaviour deltas. `readTextFileFromHandle` stays off the `FileSystemService` interface, so `AcpFileSystemService` and the typed fallback mock in `filesystem.test.ts` are untouched. +## Behaviour deltas + +The refactor is meant to be unobservable at the Serve boundary, and the +`packages/cli/src/serve/fs/` suites pass against it unchanged apart from the one +assertion noted in Testing. Four changes are nonetheless observable in principle +and are disclosed here rather than left implicit in the diff: + +1. **Handle reads run to live EOF.** #7947 bounded the handle chunk reader by + the size captured at `open` (`while (position < sourceSize)`); this PR reads + until a zero-byte read (`while (true)`). Bytes appended after the handle is + opened now land inside the decoded window. Today this is client-safe because + the Serve boundary re-stats after the read and refuses the whole read with + `hash_mismatch` on any size/mtime/ctime change, so no post-snapshot byte + reaches a client; supporting a file that grows while it is read (live logs) + is the reason to keep it. The exposure is the next caller — byte-cursor + paging — which must re-stat or supply its own end bound. Pinned by + `reads to live EOF: bytes appended after open are returned`. +2. **Fresh buffer per chunk.** #7947 reused one 512 KiB buffer across + iterations, so a yielded view was valid only until the next read. + `chunksFromHandle` allocates a fresh buffer per chunk, so a yielded chunk + stays valid after the generator advances. This is the safer contract. +3. **`limit: Infinity` is admitted at the handle boundary.** #7947 rejected a + non-finite `limit` with a `RangeError`; this PR accepts + `Number.POSITIVE_INFINITY` (and an omitted `limit`) as "read to end of + file," still capped at `maxOutputBytes`. The ACP bridge adapter only filters + `limit > 0`, and `JSON.parse('1e400')` yields `Infinity`, so the path is + reachable from a client; it now resolves with a capped window instead of + escaping as a non-`FsError` `RangeError`. The `GET /file` route's finite-`limit` + admission is unchanged, so a no-limit `GET /file` still returns + `file_too_large`. +4. **Large undecodable text maps to `binary_file`, not `file_too_large`.** A + large file in an encoding the text route cannot decode previously surfaced as + `file_too_large` (413); it is now `binary_file` (422), matching sniffed + binary content. A client that retries a 413 with a smaller window can never + exit that loop for an encoding problem; `readBytes` is the same remedy that + already applies to binary content. Documented in + `docs/developers/daemon/07-workspace-filesystem.md` and + `docs/developers/qwen-serve-protocol.md`, and asserted at the HTTP layer. + ## Blast radius - `readTextRange` is not exported from `packages/core/src/index.ts`; only @@ -122,9 +159,11 @@ untouched. - `detectFileEncoding` is public via `export * from './utils/fileUtils.js'`. Widening a parameter is source-compatible. - The only cross-package importer of the touched modules is - `packages/cli/src/serve/fs/workspace-file-system.ts`. Its only change is - dropping two arguments the handle path no longer accepts — see below; the - `decodeBufferWithEncodingInfoAsync` import it also carries is untouched. + `packages/cli/src/serve/fs/workspace-file-system.ts`. Its only changes are + dropping the two arguments the handle path no longer accepts (`path`, + `stats`) and mapping `LargeNonUtf8TextError` to `binary_file` (Behaviour + deltas #4); the `decodeBufferWithEncodingInfoAsync` import it also carries is + untouched. ### `CoreReadTextFileHandleRequest` becomes standalone @@ -153,8 +192,16 @@ cross-package call site named above. The existing suites are the specification: the whole point is that the Serve boundary cannot tell. `packages/cli/src/serve/fs/` and the bridge adapter pass against the refactor unchanged apart from one assertion updated for the -deliberate non-UTF-8 → `binary_file` mapping, as does the full `packages/core` -`src/utils` + `src/services` run. +deliberate non-UTF-8 → `binary_file` mapping (Behaviour deltas #4), as does the +full `packages/core` `src/utils` + `src/services` run. The HTTP route suite also +gains a case asserting that a large non-UTF-8 file requested with a finite +`limit` returns `422 binary_file`. + +The new handle-path contracts are pinned directly in `read-text-range.test.ts`: +a deep multi-chunk read with a byte cap, a multi-byte character straddling the +8 KiB encoding-probe boundary, read-to-EOF (bytes appended after `open` are +returned), an omitted/`Infinity` `limit` reading the whole file, and aborts +landing before and during a handle-bound stream. Two tests in `read-text-range.test.ts` moved to `readTextRangeFromHandle`. One of them changed meaning. It previously passed a handle for one @@ -177,4 +224,6 @@ are kept — they need no mock. `chunksFromHandle` gained a `from` parameter that nothing yet passes a non-zero value for. It is the single seam byte-cursor text paging needs, and that is the -change this refactor exists to enable. +change this refactor exists to enable. Because the reader now runs to live EOF +(Behaviour deltas #1), that follow-up must either supply its own end bound or +keep re-statting after the read, exactly as the Serve boundary does today. diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index eb1333144a5..7723e184a0b 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -1582,7 +1582,10 @@ Reads a text file. Query params: `path` (required), `maxBytes`, `line`, and `limit`. The daemon rejects binary files. Files above the 256 KiB full-snapshot cap require a finite `limit`; no-limit, line-only, and maxBytes-only requests remain `file_too_large`. A finite large-file window is streamed and its returned -UTF-8 content remains capped at 256 KiB. `maxBytes` always applies to the UTF-8 +UTF-8 content remains capped at 256 KiB. The handle-bound reader underneath +treats an omitted or infinite `limit` as read-to-end-of-file (still capped at +256 KiB); the finite-`limit` admission above is what keeps a no-limit +`GET /file` at `file_too_large`. `maxBytes` always applies to the UTF-8 response bytes after decoding, including when the source uses another supported encoding within the full-snapshot cap. Large text in an encoding the route cannot decode returns `binary_file` rather than `file_too_large` — retrying diff --git a/packages/cli/src/serve/routes/workspace-file-read.test.ts b/packages/cli/src/serve/routes/workspace-file-read.test.ts index 7fccfbe7043..57d857ad74d 100644 --- a/packages/cli/src/serve/routes/workspace-file-read.test.ts +++ b/packages/cli/src/serve/routes/workspace-file-read.test.ts @@ -230,6 +230,23 @@ describe('GET /file', () => { expect(res.body.errorKind).toBe('binary_file'); }); + it('returns 422 binary_file for large non-UTF-8 text even with a finite limit', async () => { + const { MAX_READ_BYTES } = await import('../fs/policy.js'); + const body = Buffer.concat([ + Buffer.from([0xff, 0xfe]), + Buffer.from('中文日志行\n'.repeat(30_000), 'utf16le'), + ]); + expect(body.length).toBeGreaterThan(MAX_READ_BYTES); + await fsp.writeFile(path.join(h.workspace, 'large-utf16.txt'), body); + + const res = await request(h.app) + .get('/file?path=large-utf16.txt&line=1&limit=20') + .set('Host', loopbackHost()); + + expect(res.status).toBe(422); + expect(res.body.errorKind).toBe('binary_file'); + }); + it('truncates content above maxBytes and reports truncated=true', async () => { await fsp.writeFile(path.join(h.workspace, 'big.txt'), 'a'.repeat(2048)); const res = await request(h.app) diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index 00d01335e42..d2240ac4be1 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -447,4 +447,44 @@ describe('readTextRange', () => { await expect(promise).rejects.toThrow(/abort/i); }); + + it('propagates a pre-aborted signal to a handle-bound read', async () => { + const filePath = await writeFile('handle-abort.log', largeUtf8Lines(2_000)); + const fileHandle = await fs.open(filePath, 'r'); + const controller = new AbortController(); + controller.abort(); + try { + await expect( + readTextRangeFromHandle(fileHandle, { + offset: 0, + limit: 3, + maxOutputBytes: 10_000, + signal: controller.signal, + }), + ).rejects.toThrow(/abort/i); + } finally { + await fileHandle.close(); + } + }); + + it('propagates an abort that lands during a handle-bound stream', async () => { + const filePath = await writeFile( + 'handle-abort-stream.log', + largeUtf8Lines(80_000), + ); + const fileHandle = await fs.open(filePath, 'r'); + const controller = new AbortController(); + try { + const promise = readTextRangeFromHandle(fileHandle, { + offset: 70_000, + limit: 10, + maxOutputBytes: 10_000, + signal: controller.signal, + }); + setTimeout(() => controller.abort(), 0); + await expect(promise).rejects.toThrow(/abort/i); + } finally { + await fileHandle.close(); + } + }); }); From cdbdfde09bc66cc2c1fb87f58d385becee7772a0 Mon Sep 17 00:00:00 2001 From: "qwen-autofix[bot]" Date: Thu, 30 Jul 2026 08:28:08 +0000 Subject: [PATCH 10/16] test(core): add positive handle-read test for Infinity and omitted limit (#7967) --- .../src/services/fileSystemService.test.ts | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 96e6625082c..7951a311169 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -58,6 +58,17 @@ vi.mock('../utils/fileUtils.js', async (importOriginal) => { import { readFileWithLineAndLimit } from '../utils/fileUtils.js'; +vi.mock('../utils/read-text-range.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + readTextRangeFromHandle: vi.fn(), + }; +}); + +import { readTextRangeFromHandle } from '../utils/read-text-range.js'; + describe('StandardFileSystemService', () => { let fileSystem: StandardFileSystemService; @@ -235,6 +246,34 @@ describe('StandardFileSystemService', () => { ).rejects.toThrow(/non-negative integer line/); }); + it.each([ + ['Infinity', Number.POSITIVE_INFINITY], + ['omitted', undefined], + ])('should accept %s limit on a handle read', async (_label, limit) => { + vi.mocked(readTextRangeFromHandle).mockResolvedValue({ + content: 'hello', + bom: false, + encoding: 'utf-8', + originalLineCount: 1, + }); + const fileHandle = {} as import('node:fs/promises').FileHandle; + + const result = await fileSystem.readTextFileFromHandle({ + fileHandle, + ...(limit !== undefined ? { limit } : {}), + maxOutputBytes: 262_144, + }); + + expect(result.content).toBe('hello'); + expect(readTextRangeFromHandle).toHaveBeenCalledWith( + fileHandle, + expect.objectContaining({ + limit: Number.POSITIVE_INFINITY, + maxOutputBytes: 262_144, + }), + ); + }); + it('should return encoding info for GBK file', async () => { vi.mocked(readFileWithLineAndLimit).mockResolvedValue({ content: '你好世界', From 1e2203556ff74a680f8b8c3034f6a62aa8923d79 Mon Sep 17 00:00:00 2001 From: "qwen-autofix[bot]" Date: Thu, 30 Jul 2026 08:38:02 +0000 Subject: [PATCH 11/16] fix(core): satisfy ReadTextRangeResult type in handle-read test mock (#7967) --- packages/core/src/services/fileSystemService.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 7951a311169..1a8c6a9723d 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -255,6 +255,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 1, + originalLineCountExact: true, + truncatedByBytes: false, }); const fileHandle = {} as import('node:fs/promises').FileHandle; From dc0604cb1ea68508e83aa4e1c5c8e4d643836a7e Mon Sep 17 00:00:00 2001 From: Qwen Code Bot Date: Thu, 30 Jul 2026 11:44:56 +0000 Subject: [PATCH 12/16] fix(core): bound handle reads to stat size and reject Infinity limit (#7967) --- ...026-07-29-handle-bound-text-range-reads.md | 40 ++++++++----------- .../src/services/fileSystemService.test.ts | 33 ++++++++++----- .../core/src/services/fileSystemService.ts | 27 +++++-------- packages/core/src/utils/fileUtils.ts | 17 +++----- .../core/src/utils/read-text-range.test.ts | 20 +++------- packages/core/src/utils/read-text-range.ts | 22 +++++++--- 6 files changed, 76 insertions(+), 83 deletions(-) diff --git a/docs/design/2026-07-29-handle-bound-text-range-reads.md b/docs/design/2026-07-29-handle-bound-text-range-reads.md index ac24013e740..e0637266a70 100644 --- a/docs/design/2026-07-29-handle-bound-text-range-reads.md +++ b/docs/design/2026-07-29-handle-bound-text-range-reads.md @@ -118,29 +118,22 @@ The refactor is meant to be unobservable at the Serve boundary, and the assertion noted in Testing. Four changes are nonetheless observable in principle and are disclosed here rather than left implicit in the diff: -1. **Handle reads run to live EOF.** #7947 bounded the handle chunk reader by - the size captured at `open` (`while (position < sourceSize)`); this PR reads - until a zero-byte read (`while (true)`). Bytes appended after the handle is - opened now land inside the decoded window. Today this is client-safe because - the Serve boundary re-stats after the read and refuses the whole read with - `hash_mismatch` on any size/mtime/ctime change, so no post-snapshot byte - reaches a client; supporting a file that grows while it is read (live logs) - is the reason to keep it. The exposure is the next caller — byte-cursor - paging — which must re-stat or supply its own end bound. Pinned by - `reads to live EOF: bytes appended after open are returned`. +1. **Handle reads are bounded to the file size at open.** + `readTextRangeFromHandle` stats the descriptor and passes the size as an end + bound to `chunksFromHandle`, which reads `while (position < to)` rather than + to live EOF. Bytes appended after the handle is opened are not returned. + This matches #7947's behaviour and keeps the chunk reader bounded even when + the requested line window is unreachable. Pinned by + `bounds handle reads to the stat size, not live EOF`. 2. **Fresh buffer per chunk.** #7947 reused one 512 KiB buffer across iterations, so a yielded view was valid only until the next read. `chunksFromHandle` allocates a fresh buffer per chunk, so a yielded chunk stays valid after the generator advances. This is the safer contract. -3. **`limit: Infinity` is admitted at the handle boundary.** #7947 rejected a - non-finite `limit` with a `RangeError`; this PR accepts - `Number.POSITIVE_INFINITY` (and an omitted `limit`) as "read to end of - file," still capped at `maxOutputBytes`. The ACP bridge adapter only filters - `limit > 0`, and `JSON.parse('1e400')` yields `Infinity`, so the path is - reachable from a client; it now resolves with a capped window instead of - escaping as a non-`FsError` `RangeError`. The `GET /file` route's finite-`limit` - admission is unchanged, so a no-limit `GET /file` still returns - `file_too_large`. +3. **`limit` stays optional but must be finite.** An omitted `limit` means + "read to end of file," still capped at `maxOutputBytes`. `Infinity` is + rejected by the handle-boundary validator: every production caller already + enforces a positive safe integer before reaching `readTextFileFromHandle`, + so the relaxation bought nothing and was removed per Simplicity First. 4. **Large undecodable text maps to `binary_file`, not `file_too_large`.** A large file in an encoding the text route cannot decode previously surfaced as `file_too_large` (413); it is now `binary_file` (422), matching sniffed @@ -222,8 +215,7 @@ are kept — they need no mock. ## Follow-up -`chunksFromHandle` gained a `from` parameter that nothing yet passes a non-zero -value for. It is the single seam byte-cursor text paging needs, and that is the -change this refactor exists to enable. Because the reader now runs to live EOF -(Behaviour deltas #1), that follow-up must either supply its own end bound or -keep re-statting after the read, exactly as the Serve boundary does today. +`chunksFromHandle` accepts an options object `{ from, to, signal }`. `from` +defaults to 0 and nothing yet passes a non-zero value; it is the seam +byte-cursor text paging needs. `to` defaults to the file size captured at open +(Behaviour deltas #1), so the follow-up inherits the bound automatically. diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 1a8c6a9723d..0e528e7e9e3 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -91,6 +91,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 1, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); @@ -110,6 +112,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 100, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ @@ -132,6 +136,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 100, + originalLineCountExact: true, + truncatedByBytes: false, }); await fileSystem.readTextFile({ @@ -152,6 +158,7 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 100, + originalLineCountExact: true, truncatedByBytes: true, }); @@ -178,6 +185,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 1, + originalLineCountExact: true, + truncatedByBytes: false, }); await fileSystem.readTextFile({ @@ -200,9 +209,9 @@ describe('StandardFileSystemService', () => { // and at the real boundary in `workspace-file-system.test.ts`; only the // argument validation below needs a unit test, and it needs no mock. it.each([ - ['maxOutputBytes', { maxOutputBytes: Number.POSITIVE_INFINITY }], - ['maxOutputBytes', { maxOutputBytes: 0 }], - ])('should reject a handle read with unbounded %s', async (bound, over) => { + ['infinite maxOutputBytes', { maxOutputBytes: Number.POSITIVE_INFINITY }], + ['zero maxOutputBytes', { maxOutputBytes: 0 }], + ])('should reject a handle read with %s', async (_label, over) => { const fileHandle = {} as import('node:fs/promises').FileHandle; await expect( @@ -211,13 +220,14 @@ describe('StandardFileSystemService', () => { limit: 20, ...over, }), - ).rejects.toThrow(new RegExp(`positive finite ${bound}`)); + ).rejects.toThrow(/positive finite maxOutputBytes/); }); it.each([ ['a fractional limit', 2.5], ['a zero limit', 0], ['a negative limit', -1], + ['an Infinity limit', Number.POSITIVE_INFINITY], ])('should reject %s on a handle read', async (_label, limit) => { const fileHandle = {} as import('node:fs/promises').FileHandle; @@ -227,7 +237,7 @@ describe('StandardFileSystemService', () => { limit, maxOutputBytes: 262_144, }), - ).rejects.toThrow(/positive integer limit or Infinity/); + ).rejects.toThrow(/positive integer limit/); }); it.each([ @@ -246,10 +256,7 @@ describe('StandardFileSystemService', () => { ).rejects.toThrow(/non-negative integer line/); }); - it.each([ - ['Infinity', Number.POSITIVE_INFINITY], - ['omitted', undefined], - ])('should accept %s limit on a handle read', async (_label, limit) => { + it('should accept an omitted limit on a handle read', async () => { vi.mocked(readTextRangeFromHandle).mockResolvedValue({ content: 'hello', bom: false, @@ -262,7 +269,6 @@ describe('StandardFileSystemService', () => { const result = await fileSystem.readTextFileFromHandle({ fileHandle, - ...(limit !== undefined ? { limit } : {}), maxOutputBytes: 262_144, }); @@ -270,7 +276,6 @@ describe('StandardFileSystemService', () => { expect(readTextRangeFromHandle).toHaveBeenCalledWith( fileHandle, expect.objectContaining({ - limit: Number.POSITIVE_INFINITY, maxOutputBytes: 262_144, }), ); @@ -282,6 +287,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'gb18030', originalLineCount: 1, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ path: '/test/gbk.txt' }); @@ -763,6 +770,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 3, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); @@ -776,6 +785,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 3, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); diff --git a/packages/core/src/services/fileSystemService.ts b/packages/core/src/services/fileSystemService.ts index b2a8b52b32f..9655b1fcd98 100644 --- a/packages/core/src/services/fileSystemService.ts +++ b/packages/core/src/services/fileSystemService.ts @@ -11,7 +11,10 @@ import * as path from 'node:path'; import { globSync } from 'glob'; import { atomicWriteFile } from '../utils/atomicFileWrite.js'; import { readFileWithLineAndLimit } from '../utils/fileUtils.js'; -import { readTextRangeFromHandle } from '../utils/read-text-range.js'; +import { + readTextRangeFromHandle, + type ReadTextRangeResult, +} from '../utils/read-text-range.js'; import { isUtf8CompatibleEncoding } from '../utils/encoding.js'; import { loadIconvLite, type IconvLite } from '../utils/load-iconv-lite.js'; import { getSystemEncoding } from '../utils/systemEncoding.js'; @@ -337,13 +340,9 @@ export class StandardFileSystemService implements FileSystemService { `handle-bound text reads require a positive finite maxOutputBytes, got ${params.maxOutputBytes}`, ); } - if ( - params.limit !== undefined && - params.limit !== Number.POSITIVE_INFINITY && - !isPositiveSafeInteger(params.limit) - ) { + if (params.limit !== undefined && !isPositiveSafeInteger(params.limit)) { throw new RangeError( - `handle-bound text reads require a positive integer limit or Infinity, got ${params.limit}`, + `handle-bound text reads require a positive integer limit, got ${params.limit}`, ); } if ( @@ -357,7 +356,7 @@ export class StandardFileSystemService implements FileSystemService { } const range = await readTextRangeFromHandle(params.fileHandle, { offset: params.line ?? 0, - limit: params.limit ?? Number.POSITIVE_INFINITY, + ...(params.limit !== undefined ? { limit: params.limit } : {}), maxOutputBytes: params.maxOutputBytes, ...(params.signal !== undefined ? { signal: params.signal } : {}), }); @@ -414,15 +413,9 @@ async function readTextFileStandard( } /** Shared metadata shaping so both read paths report identically. */ -function toReadTextFileResponse(readResult: { - content: string; - bom?: boolean; - encoding?: string; - originalLineCount: number; - originalLineCountExact?: boolean; - lineEnding?: LineEnding; - truncatedByBytes?: boolean; -}): ReadTextFileResponse { +function toReadTextFileResponse( + readResult: ReadTextRangeResult, +): ReadTextFileResponse { const detectedLineEnding = readResult.lineEnding ?? detectLineEnding(readResult.content); return { diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 809bdf0887b..cda11aba878 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -36,7 +36,7 @@ import { import { VISION_BRIDGE_MAX_IMAGES } from '../services/visionBridge/vision-bridge-constants.js'; import type { VisionBridgePdfContinuation } from '../services/visionBridge/vision-bridge-service.js'; import { readNotebookWithMetadata } from './notebook.js'; -import { readTextRange } from './read-text-range.js'; +import { readTextRange, type ReadTextRangeResult } from './read-text-range.js'; import { DEFAULT_RANGE_READ_BYTES, TEXT_RANGE_FAST_PATH_MAX_SIZE, @@ -330,15 +330,7 @@ export async function readFileWithLineAndLimit(params: { maxOutputBytes?: number; signal?: AbortSignal; stats?: import('node:fs').Stats; -}): Promise<{ - content: string; - bom?: boolean; - encoding?: string; - originalLineCount: number; - originalLineCountExact?: boolean; - lineEnding?: 'crlf' | 'lf'; - truncatedByBytes?: boolean; -}> { +}): Promise { const { path: filePath, limit, line, maxOutputBytes, signal } = params; const stats = params.stats ?? (await fs.promises.stat(filePath)); if ( @@ -372,12 +364,15 @@ export async function readFileWithLineAndLimit(params: { const actualStartLine = Math.min(startLine, originalLineCount); const selectedLines = lines.slice(actualStartLine, endLine); + const joined = selectedLines.join('\n'); return { - content: selectedLines.join('\n'), + content: joined, bom, encoding, originalLineCount, originalLineCountExact: true, + truncatedByBytes: false, + lineEnding: joined.includes('\r\n') ? 'crlf' : 'lf', }; } diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index d2240ac4be1..48ebe933dd7 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -205,7 +205,7 @@ describe('readTextRange', () => { } }); - it('reads the full file when limit is omitted or Infinity', async () => { + it('reads the full file when limit is omitted', async () => { const filePath = await writeFile('small.txt', 'one\ntwo\nthree\n'); const fileHandle = await fs.open(filePath, 'r'); try { @@ -216,30 +216,22 @@ describe('readTextRange', () => { expect(omitted.originalLineCount).toBe(4); expect(omitted.originalLineCountExact).toBe(true); expect(omitted.truncatedByBytes).toBe(false); - - const infinite = await readTextRangeFromHandle(fileHandle, { - limit: Number.POSITIVE_INFINITY, - maxOutputBytes: 10_000, - }); - expect(infinite.content).toBe('one\ntwo\nthree\n'); - expect(infinite.originalLineCount).toBe(4); - expect(infinite.originalLineCountExact).toBe(true); } finally { await fileHandle.close(); } }); - it('reads to live EOF: bytes appended after open are returned', async () => { - const filePath = await writeFile('append.txt', 'first\n'); + it('bounds handle reads to the stat size, not live EOF', async () => { + const filePath = await writeFile('bounded.txt', 'first\nsecond\n'); const fileHandle = await fs.open(filePath, 'r'); try { - await fs.appendFile(filePath, 'second\n'); - const result = await readTextRangeFromHandle(fileHandle, { + offset: 10_000, + limit: 3, maxOutputBytes: 10_000, }); - expect(result.content).toBe('first\nsecond\n'); + expect(result.content).toBe(''); expect(result.originalLineCount).toBe(3); expect(result.originalLineCountExact).toBe(true); } finally { diff --git a/packages/core/src/utils/read-text-range.ts b/packages/core/src/utils/read-text-range.ts index bdaded73b8b..2aaba2e1ba7 100644 --- a/packages/core/src/utils/read-text-range.ts +++ b/packages/core/src/utils/read-text-range.ts @@ -100,16 +100,21 @@ export async function readTextRange( * caller reaches for a handle precisely when it needs the read bounded. The * handle is borrowed — every read uses an explicit position, and this function * never closes it. + * + * The read is bounded to the file size captured by an internal stat, so + * bytes appended during the read are not returned. */ export async function readTextRangeFromHandle( fileHandle: FileHandle, request: ReadTextRangeFromHandleRequest, ): Promise { request.signal?.throwIfAborted(); + const stats = await fileHandle.stat(); return readLargeUtf8Range( fileHandle, request, normalizeMaxBytes(request.maxOutputBytes), + stats.size, ); } @@ -157,6 +162,7 @@ async function readLargeUtf8Range( source: string | FileHandle, request: { offset?: number; limit?: number; signal?: AbortSignal }, maxOutputBytes: number, + sourceSize?: number, ): Promise { const encoding = await detectFileEncoding(source); // Detection is one bounded 8 KiB read, but check here anyway so an abort @@ -193,7 +199,10 @@ async function readLargeUtf8Range( }); chunks = pathStream; } else { - chunks = chunksFromHandle(source, 0, request.signal); + chunks = chunksFromHandle(source, { + to: sourceSize, + signal: request.signal, + }); } function appendSelected(fragment: string): void { @@ -300,7 +309,7 @@ async function readLargeUtf8Range( } /** - * Sequential chunks off a borrowed descriptor, starting at `from`. + * Sequential chunks off a borrowed descriptor. * * Reads use explicit positions, so the caller's file position is untouched and * two readers can share one handle. Each chunk is a fresh buffer and stays @@ -309,14 +318,15 @@ async function readLargeUtf8Range( */ async function* chunksFromHandle( fileHandle: FileHandle, - from = 0, - signal?: AbortSignal, + options?: { from?: number; to?: number; signal?: AbortSignal }, ): AsyncGenerator { const highWaterMark = 512 * 1024; + const { from = 0, to = Number.POSITIVE_INFINITY, signal } = options ?? {}; let position = from; - while (true) { + while (position < to) { signal?.throwIfAborted(); - const buffer = Buffer.allocUnsafe(highWaterMark); + const length = Math.min(highWaterMark, to - position); + const buffer = Buffer.allocUnsafe(length); const { bytesRead } = await fileHandle.read( buffer, 0, From 20d606bf66048cad6d6893e6cab645965adab25a Mon Sep 17 00:00:00 2001 From: Qwen Code Bot Date: Thu, 30 Jul 2026 13:59:24 +0000 Subject: [PATCH 13/16] fix(core): satisfy ReadTextRangeResult type in fileSystemService test mocks (#7967) --- .../core/src/services/fileSystemService.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/core/src/services/fileSystemService.test.ts b/packages/core/src/services/fileSystemService.test.ts index 952156b63fc..d1aa3dea445 100644 --- a/packages/core/src/services/fileSystemService.test.ts +++ b/packages/core/src/services/fileSystemService.test.ts @@ -80,6 +80,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 1, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); @@ -99,6 +101,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 100, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ @@ -121,6 +125,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 100, + originalLineCountExact: true, + truncatedByBytes: false, }); await fileSystem.readTextFile({ @@ -141,6 +147,7 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 100, + originalLineCountExact: true, truncatedByBytes: true, }); @@ -167,6 +174,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 1, + originalLineCountExact: true, + truncatedByBytes: false, }); await fileSystem.readTextFile({ @@ -312,6 +321,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'gb18030', originalLineCount: 1, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ path: '/test/gbk.txt' }); @@ -793,6 +804,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 3, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); @@ -806,6 +819,8 @@ describe('StandardFileSystemService', () => { bom: false, encoding: 'utf-8', originalLineCount: 3, + originalLineCountExact: true, + truncatedByBytes: false, }); const result = await fileSystem.readTextFile({ path: '/test/file.txt' }); From b3dafee4efa5d1ed28dd6c96bb300df588715f53 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 30 Jul 2026 16:48:56 +0000 Subject: [PATCH 14/16] refactor(core): reuse detectLineEndingFromContent in whole-file fast path (#7967) Co-authored-by: Qwen-Coder --- packages/core/src/utils/fileUtils.ts | 8 ++++++-- packages/core/src/utils/read-text-range.ts | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index cda11aba878..c8d3a4032ca 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -36,7 +36,11 @@ import { import { VISION_BRIDGE_MAX_IMAGES } from '../services/visionBridge/vision-bridge-constants.js'; import type { VisionBridgePdfContinuation } from '../services/visionBridge/vision-bridge-service.js'; import { readNotebookWithMetadata } from './notebook.js'; -import { readTextRange, type ReadTextRangeResult } from './read-text-range.js'; +import { + readTextRange, + type ReadTextRangeResult, + detectLineEndingFromContent, +} from './read-text-range.js'; import { DEFAULT_RANGE_READ_BYTES, TEXT_RANGE_FAST_PATH_MAX_SIZE, @@ -372,7 +376,7 @@ export async function readFileWithLineAndLimit(params: { originalLineCount, originalLineCountExact: true, truncatedByBytes: false, - lineEnding: joined.includes('\r\n') ? 'crlf' : 'lf', + lineEnding: detectLineEndingFromContent(joined), }; } diff --git a/packages/core/src/utils/read-text-range.ts b/packages/core/src/utils/read-text-range.ts index d341bfdfb6b..8db9247ab91 100644 --- a/packages/core/src/utils/read-text-range.ts +++ b/packages/core/src/utils/read-text-range.ts @@ -749,6 +749,6 @@ function truncateUtf8( }; } -function detectLineEndingFromContent(content: string): 'crlf' | 'lf' { +export function detectLineEndingFromContent(content: string): 'crlf' | 'lf' { return content.includes('\r\n') ? 'crlf' : 'lf'; } From 2558093a5e42386ae330ec5df900409a6978f277 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 31 Jul 2026 08:36:41 +0000 Subject: [PATCH 15/16] fix(core): export ReadTextRangeResult type and cover CRLF detection (#7967) --- packages/core/src/index.ts | 1 + packages/core/src/utils/read-text-range.test.ts | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 1b8482c7a04..c4b7126547a 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -271,6 +271,7 @@ export { LargeNonUtf8TextError, TextScanBudgetExceededError, } from './utils/read-text-range.js'; +export type { ReadTextRangeResult } from './utils/read-text-range.js'; export { isUtf8CompatibleEncoding } from './utils/encoding.js'; export * from './services/gitWorktreeService.js'; export { DEFAULT_MAX_TOOL_CALLS_PER_TURN } from './services/loopDetectionService.js'; diff --git a/packages/core/src/utils/read-text-range.test.ts b/packages/core/src/utils/read-text-range.test.ts index 2789682fcfe..b72450e2362 100644 --- a/packages/core/src/utils/read-text-range.test.ts +++ b/packages/core/src/utils/read-text-range.test.ts @@ -12,6 +12,7 @@ import { iconvEncode } from './iconvHelper.js'; import { CursorNotAtLineBoundaryError, LargeNonUtf8TextError, + detectLineEndingFromContent, readTextCursorWindowFromHandle, readTextRange, readTextRangeFromHandle, @@ -995,3 +996,13 @@ describe('readTextCursorWindowFromHandle', () => { }); }); }); + +describe('detectLineEndingFromContent', () => { + it('reports crlf for small CRLF content', () => { + expect(detectLineEndingFromContent('line1\r\nline2\r\n')).toBe('crlf'); + }); + + it('reports lf when no CRLF is present', () => { + expect(detectLineEndingFromContent('line1\nline2\n')).toBe('lf'); + }); +}); From af5da8ea9de59560058f37854d70c3e71f477eac Mon Sep 17 00:00:00 2001 From: Qwen Code Bot Date: Fri, 31 Jul 2026 09:23:07 +0000 Subject: [PATCH 16/16] test(core): cover whole-file lineEnding and truncatedByBytes shape (#7967) --- packages/core/src/utils/fileUtils.test.ts | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/core/src/utils/fileUtils.test.ts b/packages/core/src/utils/fileUtils.test.ts index 286e68ee484..49876352b99 100644 --- a/packages/core/src/utils/fileUtils.test.ts +++ b/packages/core/src/utils/fileUtils.test.ts @@ -3081,6 +3081,27 @@ describe('fileUtils', () => { ).rejects.toThrow(/abort/i); }); + it('reports lineEnding and truncatedByBytes on an unbounded read', async () => { + actualNodeFs.writeFileSync( + testTextFilePath, + 'alpha\r\nbravo\r\ncharlie\r\n', + ); + + const whole = await readFileWithLineAndLimit({ + path: testTextFilePath, + limit: Number.POSITIVE_INFINITY, + }); + const range = await readFileWithLineAndLimit({ + path: testTextFilePath, + limit: 3, + }); + + expect(whole.lineEnding).toBe('crlf'); + expect(whole.truncatedByBytes).toBe(false); + expect(whole.lineEnding).toBe(range.lineEnding); + expect(whole.truncatedByBytes).toBe(range.truncatedByBytes); + }); + it('should use provided stats when reading with line and byte limits', async () => { actualNodeFs.writeFileSync(testTextFilePath, 'hello\nworld'); const stats = actualNodeFs.statSync(testTextFilePath);