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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 59 additions & 19 deletions packages/core/src/services/fileReadCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,22 +56,53 @@ export interface FileReadEntry {
/** ms epoch of the last successful write. Undefined if never written. */
lastWriteAt?: number;
/**
* True iff the most recent Read consumed the whole file (no offset /
* limit / pages). Used by the Read fast-path to decide whether a
* follow-up "no-args" Read can return a `file_unchanged` placeholder
* instead of re-emitting the full content. Range-scoped Reads never
* trigger the placeholder, since the model may legitimately ask for a
* different range next time.
* True iff the most recent Read produced the whole file's current
* content: no offset / limit / pages on the request AND the content
* was not truncated by the truncate-tool-output limit. A truncated
* full read records `false` here because the model only saw the
* head of the file.
*
* Sole consumer is the Read fast-path, which uses this flag
* (combined with `lastReadCacheable` and a write-newer-than-read
* check) to decide whether a follow-up "no-args" Read can return
* a `file_unchanged` placeholder.
*
* **`priorReadEnforcement.ts` does NOT consult this flag and must
* not start.** PR #3932 wired it into a `requireFullRead` option
* for WriteFile's overwrite path; PR #4002 removed that wiring
* because the truncate-tool-output limit makes "fully read" an
* impossible precondition on files larger than the limit (issue
* #3945 deadlock). The current contract aligns with Claude Code's
* `readFileState`: any prior read clears enforcement, the
* mtime/size drift check is the safety net. `fileReadCacheDisabled:
* true` is an OPT-OUT (it bypasses the cache and thus enforcement
* entirely so application-level locking can take over) — it is NOT
* an opt-in to stricter behaviour.
*/
lastReadWasFull: boolean;
/**
* True iff the content the most recent Read produced is one we are
* willing to substitute with a `file_unchanged` placeholder. Plain
* text reads set this to true; binary, image, audio, video, PDF, and
* notebook reads set it to false, because the model will likely need
* the structured / multi-modal payload again rather than a stub. The
* cache itself does not interpret this flag — it is a hint produced
* and consumed by the Read tool.
* True iff the most recent Read produced plain-text content — i.e.
* a text payload the Edit / WriteFile tools can mutate as text.
* False for binary, image, audio, video, PDF, and notebook reads,
* which produce structured payloads the mutating tools cannot
* safely alter.
*
* Note: this flag is purely about *content type* (text vs.
* non-text), not about whether the read was complete. Truncation
* is tracked separately on {@link lastReadWasFull}; conflating
* the two caused the issue #3964 regression where a partial /
* truncated text read caused the next Edit to be rejected with
* the misleading "binary / image / audio / video / PDF / notebook
* payload" error.
*
* Two independent consumers read this flag:
* - the ReadFile fast-path uses it (combined with
* `lastReadWasFull`) to decide whether to serve the
* `file_unchanged` placeholder.
* - `priorReadEnforcement.ts` uses it to detect non-text payloads
* and reject Edit / WriteFile against them (re-reading would
* produce the same non-text payload, so the message tells the
* model to use a different mechanism rather than re-read).
*/
lastReadCacheable: boolean;
}
Expand All @@ -93,12 +124,21 @@ export class FileReadCache {
/**
* Record a successful Read of `absPath`.
*
* - `full` — the Read covered the entire file (no offset / limit
* / pages). Only full Reads enable the `file_unchanged` fast-path
* on subsequent reads.
* - `cacheable` — the produced content is suitable for substitution
* with a `file_unchanged` placeholder. Set true for plain text,
* false for binary / image / audio / video / PDF / notebook.
* - `full` — the Read produced the entire current content of
* the file: no offset / limit / pages on the request AND the
* output was not truncated. Pass `false` for ranged reads OR
* for full-request reads whose content was truncated by the
* truncate-tool-output limit; both leave the model without
* sight of every current byte.
* - `cacheable` — the produced content is plain text (vs. binary /
* image / audio / video / PDF / notebook). This flag is purely
* about content type, not about whether the read was complete:
* a partial / truncated text read still records `cacheable: true`
* because the bytes the model saw were text. (Bundling
* truncation into `cacheable` was the issue #3964 regression
* that caused partial reads of `.kt` / `.cpp` / `.py` files to
* be rejected on the next Edit with a misleading "binary
* payload" message.)
*
* The `lastReadWasFull` and `lastReadCacheable` flags are
* **sticky-on-true** when the recorded fingerprint matches the
Expand Down
120 changes: 55 additions & 65 deletions packages/core/src/tools/priorReadEnforcement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,22 +82,30 @@ export type PriorReadVerb = 'editing' | 'overwriting';
* drift, not a "the file genuinely never existed" disappearance
* race. The default (`expectExisting: false`) is the pre-read
* behaviour: ENOENT means "go ahead and create".
* - `requireFullRead`: when true, a partial read (offset / limit /
* pages) of an existing file does NOT satisfy enforcement — only
* a full read does. EditTool can rely on its `old_string` matching
* as a content-derived guard against editing bytes the model never
* saw, so a partial read is acceptable there. WriteFileTool's
* overwrite path replaces the entire file and has no equivalent
* guard: a model that has only seen a slice would necessarily
* hallucinate the rest of the bytes it overwrites (the data-loss
* scenario in issue #2499). Pass `true` from WriteFileTool's
* enforcement call sites; leave unset / `false` for EditTool.
* The flag has no effect when the file does not yet exist
* (ENOENT → `ok: true` for new-file creation regardless).
*
* **Do not re-introduce a `requireFullRead` (or any "stricter for
* WriteFile than Edit") option here.** PR #3932 added one with the
* rationale that WriteFile's overwrite path needs more evidence than
* Edit's `old_string`-matched in-place change; PR #4002 removed it
* because the truncate-tool-output limit makes "fully read" an
* impossible precondition on files larger than the limit, producing
* the deadlock issue #3945 reported. The contract now matches Claude
* Code's `readFileState`: any prior read clears enforcement for both
* tools, the mtime/size drift check is the safety net.
*
* There is no built-in "stricter than this" mode. `fileReadCacheDisabled:
* true` is the OPPOSITE — it bypasses the cache (and thus prior-read
* enforcement) entirely, ceding the safety net to whatever
* application-level overwrite-protection the operator wires up
* (lockfiles, content hashing, atomic temp-file rename, etc.). Users
* who want STRICTER built-in enforcement than the residual #2499 risk
* accepts have no flag here today; file a feature request.
*
* See the docstring on {@link checkPriorRead} for the full rationale
* and the residual #2499 risk it accepts.
*/
export interface CheckPriorReadOptions {
expectExisting?: boolean;
requireFullRead?: boolean;
}

/**
Expand All @@ -110,18 +118,23 @@ export interface CheckPriorReadOptions {
* audio / video / PDF / notebook — those return a structured payload
* the Edit / WriteFile tools cannot mutate as text).
*
* Partial vs full read policy depends on `options.requireFullRead`:
* - default (`requireFullRead !== true`, i.e. EditTool): a partial
* read (offset / limit / pages) counts. The `0 occurrences`
* failure mode in `calculateEdit` already catches a fabricated
* `old_string` that misses the actual bytes, so requiring a full
* read on top of that is over-defence at a real context cost.
* - `requireFullRead: true` (WriteFileTool overwrite): partial reads
* do NOT count. Overwriting replaces the entire file with no
* content-derived guard, so the model must have seen all current
* bytes — issue #2499 (LLM hallucinates content of an unread
* file and clobbers user changes) is exactly the partial-read-
* then-WriteFile case.
* `lastReadCacheable` is purely about content type, not completeness.
* A truncated or partial text read still records `lastReadCacheable:
* true` because the bytes the model saw were text. Whether the model
* has seen *every* byte is recorded on `lastReadWasFull` for the
* Read fast-path; we do NOT consult it for enforcement, because the
* truncate-tool-output limit makes "fully read" an impossible
* precondition on files larger than the limit (issue #3945).
* Aligning with Claude Code's `readFileState`: any prior read clears
* enforcement for both Edit and WriteFile; the mtime/size drift
* check above is the only gate that distinguishes "the model has
* seen current bytes" from "the model has seen older bytes", and it
* fires identically for both tools. Issue #2499 (model hallucinates
* unread bytes on overwrite) is the residual risk this stance
* accepts, mitigated by the drift check. There is no built-in
* stricter mode — `fileReadCacheDisabled: true` is an OPT-OUT (it
* bypasses enforcement entirely so application-level locking can
* take over), not an opt-in to anything stricter.
*
* Stat policy: `ENOENT` means the path disappeared between the
* caller's `fileExists` check and now — a disappearance race that is
Expand Down Expand Up @@ -236,8 +249,7 @@ export async function checkPriorRead(
if (
status.state === 'fresh' &&
status.entry.lastReadAt !== undefined &&
status.entry.lastReadCacheable &&
(!options.requireFullRead || status.entry.lastReadWasFull)
status.entry.lastReadCacheable
) {
return { ok: true };
}
Expand Down Expand Up @@ -286,48 +298,26 @@ export async function checkPriorRead(
displayMessage: `non-text payload; cannot ${verbBare} via this tool.`,
};
}
// fresh + cacheable + partial, but caller demands a full read
// (WriteFile overwrites). The model has seen *some* of this file's
// current bytes, but not all of them — and the operation is about
// to replace every byte. Without this branch a partial-read-then-
// WriteFile would silently destroy content the model never saw,
// re-introducing the issue #2499 data-loss scenario.
if (
status.state === 'fresh' &&
status.entry.lastReadAt !== undefined &&
status.entry.lastReadCacheable &&
options.requireFullRead &&
!status.entry.lastReadWasFull
) {
const raw =
`File ${filePath} has only been partially read in this session ` +
`(prior read used offset / limit / pages). ${verb === 'overwriting' ? 'Overwriting' : 'This operation'} ` +
`replaces the entire file, so the model must have seen all current ` +
`bytes first — not just the slice it has read. Re-read with the ` +
`${ToolNames.READ_FILE} tool without offset / limit / pages, then ` +
`retry ${verb} it.`;
return {
ok: false,
type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ,
rawMessage: raw,
displayMessage: `partial read; full ${ToolNames.READ_FILE} required before ${verb} this file.`,
};
}
// unknown: the model has never read this file in this session.
const verbBare = verb === 'editing' ? 'edit' : 'overwrite';
const verbDisplay =
verb === 'editing' ? 'editing this file' : 'overwriting this file';
const raw = options.requireFullRead
? `File ${filePath} has not been read in this session. ` +
`${verb === 'overwriting' ? 'Overwriting' : 'This operation'} replaces ` +
`the entire file, so the model must have seen all current bytes ` +
`first. Use the ${ToolNames.READ_FILE} tool without offset / limit ` +
`/ pages to load the full content before ${verb} it.`
: `File ${filePath} has not been read in this session. ` +
`Use the ${ToolNames.READ_FILE} tool first to load the current ` +
`content (a partial read with offset / limit is fine — you only ` +
`need to have seen the bytes you intend to ${verbBare}) before ` +
`${verb} it.`;
// Tool-specific guidance on partial reads. Edit can use a partial
// read (the model only needs to have seen `old_string`-bearing
// bytes; the rest of the file passes through untouched). WriteFile
// OVERWRITES — the model is replacing the entire file, so a
// partial read leaves any unseen bytes as collateral damage. The
// mtime/size drift check still catches the worst case (#2499
// hallucinated-bytes risk), but recommending a partial read here
// would actively encourage the foot-gun.
const partialReadGuidance =
verb === 'editing'
? `(a partial read with offset / limit is fine — you only need to have seen the bytes you intend to ${verbBare})`
: `(read the full file — overwriting replaces every byte, so any unseen bytes would be discarded)`;
const raw =
`File ${filePath} has not been read in this session. ` +
Comment thread
wenshao marked this conversation as resolved.
`Use the ${ToolNames.READ_FILE} tool first to load the current ` +
`content ${partialReadGuidance} before ${verb} it.`;
return {
ok: false,
type: ToolErrorType.EDIT_REQUIRES_PRIOR_READ,
Expand Down
94 changes: 94 additions & 0 deletions packages/core/src/tools/read-file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -825,6 +825,100 @@ describe('ReadFileTool', () => {
}
});

it('records partial text reads with lastReadCacheable=true so a follow-up Edit passes enforcement (issue #3964)', async () => {
// Pre-fix, ReadFileToolInvocation derived `cacheable` as
// `string && originalLineCount && !isTruncated`. A partial
// read of a regular text file (offset/limit) sets
// `isTruncated = true`, which collapsed `cacheable` to false
// and recorded the entry as `lastReadCacheable: false`.
// priorReadEnforcement.ts then mistook this for "binary
// payload" on the next Edit and rejected the call with the
// misleading "binary / image / audio / video / PDF /
// notebook payload" error. Decoupling the truncation check
// from `cacheable` (truncation now lives on
// `lastReadWasFull`) means partial text reads correctly
// record as text-cacheable.
const filePath = path.join(tempRootDir, 'partial.kt');
const lines = Array.from({ length: 50 }, (_, i) => `line ${i + 1}`);
await fsp.writeFile(filePath, lines.join('\n'), 'utf-8');

await read({ file_path: filePath, offset: 10, limit: 5 });

const status = fileReadCache.check(fs.statSync(filePath));
expect(status.state).toBe('fresh');
if (status.state === 'fresh') {
expect(status.entry.lastReadAt).toBeDefined();
// The truncation check moved to `lastReadWasFull`: a
// ranged read leaves the model without sight of every
// byte, so this stays false.
expect(status.entry.lastReadWasFull).toBe(false);
// The bytes the model saw were text — Edit must accept
// this read.
expect(status.entry.lastReadCacheable).toBe(true);
}
});

it('records truncated full reads with lastReadCacheable=true (issue #3964)', async () => {
// Symmetric regression for the other arm of issue #3964:
// `read_file(file_path)` without offset/limit but on a file
// larger than the truncate-tool-output limit. Pre-fix the
// truncated content collapsed `cacheable` to false; post-fix
// it stays true (the bytes were text), and only
// `lastReadWasFull` is false (the model only saw the head).
const filePath = path.join(tempRootDir, 'long.cpp');
// Mock Config caps truncate-tool-output-lines at 500.
const bigContent = Array.from(
{ length: 700 },
(_, i) => `line ${i + 1}`,
).join('\n');
await fsp.writeFile(filePath, bigContent, 'utf-8');

const result = await read({ file_path: filePath });
expect(result.returnDisplay).toMatch(/Read lines .* of 700/);

const status = fileReadCache.check(fs.statSync(filePath));
expect(status.state).toBe('fresh');
if (status.state === 'fresh') {
// Truncated → model has not seen every byte.
expect(status.entry.lastReadWasFull).toBe(false);
// But the bytes are text, so Edit (which accepts partial
// reads) must not be rejected as "binary payload".
expect(status.entry.lastReadCacheable).toBe(true);
}
});

it('reads source-code files with binary-looking content as text (encrypted FS, issue #3964)', async () => {
// Frank-Shaw-FS reports `.cpp` source files on Windows
// encrypted / DRM-protected file systems being misclassified
// as binary. The OS surfaces encrypted bytes to `fs.open()`
// random-access reads, so the 4 KB `isBinaryFile` heuristic
// sees nulls / non-printables and concludes binary even
// though the user-visible content is plain text. The
// extension-based override in detectFileType skips the
// content sample for known text extensions; verify that
// routes through `processSingleFileContent` correctly and
// records the read as text-cacheable so a follow-up Edit
// passes prior-read enforcement.
//
// We can't easily simulate a real encrypted volume in a
// unit test, so we approximate by writing nominally text
// content to a `.cpp` file. The test relies on the
// extension override winning over any future content-side
// heuristic — there is no isBinaryFile mocking in scope.
const filePath = path.join(tempRootDir, 'src.cpp');
await fsp.writeFile(filePath, '#include <iostream>\nint main() {}\n');

const result = await read({ file_path: filePath });
expect(typeof result.llmContent).toBe('string');
expect(result.llmContent).toContain('#include');

const status = fileReadCache.check(fs.statSync(filePath));
expect(status.state).toBe('fresh');
if (status.state === 'fresh') {
expect(status.entry.lastReadCacheable).toBe(true);
}
});

it('does not return the placeholder for image files', async () => {
const imagePath = path.join(tempRootDir, 'pic.png');
const pngHeader = Buffer.from([
Expand Down
Loading
Loading