diff --git a/packages/core/src/services/fileReadCache.ts b/packages/core/src/services/fileReadCache.ts index 172b1fd4592..4bac8fb19c5 100644 --- a/packages/core/src/services/fileReadCache.ts +++ b/packages/core/src/services/fileReadCache.ts @@ -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; } @@ -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 diff --git a/packages/core/src/tools/priorReadEnforcement.ts b/packages/core/src/tools/priorReadEnforcement.ts index c5c8bc9f7d0..e7c61eb94f4 100644 --- a/packages/core/src/tools/priorReadEnforcement.ts +++ b/packages/core/src/tools/priorReadEnforcement.ts @@ -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; } /** @@ -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 @@ -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 }; } @@ -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. ` + + `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, diff --git a/packages/core/src/tools/read-file.test.ts b/packages/core/src/tools/read-file.test.ts index 2b2916f508f..fae35b5f541 100644 --- a/packages/core/src/tools/read-file.test.ts +++ b/packages/core/src/tools/read-file.test.ts @@ -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 \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([ diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index b11b579ae15..c528ba634d8 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -215,14 +215,34 @@ class ReadFileToolInvocation extends BaseToolInvocation< } // Record a cache entry so that subsequent identical Reads can hit - // the file_unchanged fast-path. An entry is "cacheable" only when - // - the content is plain text (not binary / image / audio / video - // / PDF / notebook — those need their structured payload), and - // - the read was not truncated. A truncated full Read means the - // model only saw the head of the file; returning a placeholder - // on the next call would falsely imply "you've already seen - // everything", so we force the next call back through the full - // pipeline. + // the file_unchanged fast-path, and so prior-read enforcement on + // Edit / WriteFile can recognise the read. + // + // Two independent flags are recorded: + // + // - `cacheable` — whether the content is plain text (not binary / + // image / audio / video / PDF / notebook). This is the flag + // `priorReadEnforcement.ts` consults to decide whether the + // model has seen a payload that Edit / WriteFile can mutate as + // text. It must NOT include "was the read truncated", because + // a truncated text read still produced text — bundling those + // two concerns is what produced the issue #3964 regression + // where a partial Read of a regular `.kt` / `.cpp` / `.py` + // file caused the next Edit to be rejected with the + // misleading "binary / image / audio / video / PDF / notebook + // payload" error. + // + // - `full` — whether the model has seen every byte of the + // current file. This now gates ONLY the file_unchanged + // fast-path; PR #4002 removed WriteFile's `requireFullRead` + // (the truncate-tool-output limit made "fully read" an + // impossible precondition on files past the limit, deadlocking + // issue #3945). A "full" Read at the request level (no + // offset / limit / pages) only counts as full at the cache + // level if the produced content was not truncated, otherwise + // the model only saw the head and a follow-up `file_unchanged` + // placeholder would falsely imply "you've already seen + // everything". // // The stat we record is the one taken inside `processSingleFileContent` // and surfaced via `result.stats`. The internal stat happens @@ -242,11 +262,10 @@ class ReadFileToolInvocation extends BaseToolInvocation< if (cacheEnabled && (result.stats ?? stats)) { const cacheable = typeof result.llmContent === 'string' && - result.originalLineCount !== undefined && - !result.isTruncated; + result.originalLineCount !== undefined; const recordStats: Stats = result.stats ?? stats!; cache.recordRead(absPath, recordStats, { - full: isFullRead, + full: isFullRead && !result.isTruncated, cacheable, }); } diff --git a/packages/core/src/tools/write-file.test.ts b/packages/core/src/tools/write-file.test.ts index 09805e9a211..a5bc325c71a 100644 --- a/packages/core/src/tools/write-file.test.ts +++ b/packages/core/src/tools/write-file.test.ts @@ -929,14 +929,18 @@ describe('WriteFileTool', () => { fs.unlinkSync(filePath); }); - it('rejects a write when the previous read was ranged (offset/limit)', async () => { - // WriteFile diverges from EditTool here: a partial read counts - // for in-place edits (Edit's `old_string` matching is the - // content-derived guard against editing bytes the model never - // saw), but WriteFile replaces the whole file and has no - // equivalent guard — a slice-only read followed by an - // overwrite would necessarily hallucinate the rest of the - // bytes, which is the issue #2499 data-loss scenario. + it('allows a write after a ranged (offset/limit) read', async () => { + // Aligns WriteFile with EditTool and Claude Code's + // `readFileState`: any prior read clears enforcement. The + // earlier asymmetric stance (full read required for + // overwrite, partial OK for Edit) created a deadlock on + // files larger than the truncate-tool-output limit, where + // `read_file` without offset/limit still produced a + // truncated read and there was no way to satisfy the + // "fully read" precondition (issue #3945). The mtime/size + // drift check is the gate that distinguishes "model has + // seen current bytes" from "model has seen older bytes", + // and it fires identically for Edit and WriteFile. const filePath = path.join(rootDir, 'enforce-ranged.txt'); fs.writeFileSync(filePath, 'unchanged', 'utf-8'); const stats = fs.statSync(filePath); @@ -948,13 +952,49 @@ describe('WriteFileTool', () => { const result = await tool .build({ file_path: filePath, content: 'clobber' }) .execute(abortSignal); - expect(result.error?.type).toBe(ToolErrorType.EDIT_REQUIRES_PRIOR_READ); - // Error message should explain why partial reads are not enough - // for overwrites, not just say "has not been read". - expect(result.error?.message).toMatch( - /only been partially read|replaces the entire file/, - ); - expect(fs.readFileSync(filePath, 'utf-8')).toBe('unchanged'); + expect(result.error).toBeUndefined(); + expect(fs.readFileSync(filePath, 'utf-8')).toBe('clobber'); + + fs.unlinkSync(filePath); + }); + + it('allows a write after a truncated full read (issue #3945 deadlock fix)', async () => { + // Pre-fix, a `read_file` without offset/limit on a file larger + // than the truncate-tool-output limit recorded + // `lastReadWasFull: false` (the model only saw the head), and + // WriteFile's `requireFullRead: true` rejected the follow-up + // overwrite with "only been partially read … re-read without + // offset / limit / pages" — but a re-read produces the same + // truncated state, deadlocking the user. After dropping + // `requireFullRead` (aligning with Claude Code), the truncated + // read is enough to clear enforcement; the mtime/size drift + // check remains the gate that distinguishes "model saw current + // bytes" from "model saw older bytes". + // + // Coverage split: this test seeds the cache directly (mockConfig + // here lacks the `getFileService` / `getTruncateToolOutputLines` + // / `getTruncateToolOutputThreshold` / `getContentGeneratorConfig` + // wiring ReadFileTool needs). The matching ReadFile-side coverage + // that *produces* `{ full: false, cacheable: true }` for a + // truncated full read lives in read-file.test.ts under "records + // truncated full reads with lastReadCacheable=true (issue #3964)". + // A future cache-entry schema change must update both halves to + // keep the deadlock-free guarantee end-to-end. + const filePath = path.join(rootDir, 'enforce-truncated-full.txt'); + fs.writeFileSync(filePath, 'unchanged', 'utf-8'); + const stats = fs.statSync(filePath); + fileReadCache.recordRead(filePath, stats, { + // `full: false` is what a truncated full read records + // (read-file.ts: `full: isFullRead && !result.isTruncated`). + full: false, + cacheable: true, + }); + + const result = await tool + .build({ file_path: filePath, content: 'rewritten' }) + .execute(abortSignal); + expect(result.error).toBeUndefined(); + expect(fs.readFileSync(filePath, 'utf-8')).toBe('rewritten'); fs.unlinkSync(filePath); }); diff --git a/packages/core/src/tools/write-file.ts b/packages/core/src/tools/write-file.ts index 998b9925a50..751c5372ef2 100644 --- a/packages/core/src/tools/write-file.ts +++ b/packages/core/src/tools/write-file.ts @@ -136,15 +136,24 @@ class WriteFileToolInvocation extends BaseToolInvocation< // here, a race window the pre-fix gating left wide open) means // the model is about to clobber bytes it never read → reject. if (!this.config.getFileReadCacheDisabled()) { + // No `requireFullRead`-style option is passed — by design, + // and applies to all 5 checkPriorRead call sites in this file. + // PR #3932 added that option to require a full read before + // overwrite; PR #4002 removed it because the truncate-tool- + // output limit makes "fully read" an impossible precondition + // on large files (issue #3945 deadlock). WriteFile and Edit + // now share the same contract — any prior read clears + // enforcement and mtime/size drift is the safety net. The + // `fileReadCacheDisabled: true` config check above goes the + // OTHER way (skipping `checkPriorRead` entirely so application- + // level locking can take over), it is not an opt-in to + // stricter behaviour. See the docstring on `checkPriorRead` + // for the full rationale and the residual #2499 risk this + // stance accepts. const decision = await checkPriorRead( this.config.getFileReadCache(), this.params.file_path, 'overwriting', - // WriteFile replaces the entire file: a partial read is not - // enough evidence. Edit's `old_string` matching covers the - // "fabricated content" case for in-place edits, but there is - // no equivalent guard on the overwrite path. - { requireFullRead: true }, ); if (!decision.ok) { // Surface the structured ToolErrorType through scheduler. @@ -189,7 +198,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< this.config.getFileReadCache(), this.params.file_path, 'overwriting', - { expectExisting: true, requireFullRead: true }, + { expectExisting: true }, ); if (!postDecision.ok) { debugLogger.warn('post-read TOCTOU rejection (confirmation)', { @@ -263,7 +272,6 @@ class WriteFileToolInvocation extends BaseToolInvocation< this.config.getFileReadCache(), file_path, 'overwriting', - { requireFullRead: true }, ); if (!decision.ok) { return { @@ -327,7 +335,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< this.config.getFileReadCache(), file_path, 'overwriting', - { expectExisting: true, requireFullRead: true }, + { expectExisting: true }, ); if (!postDecision.ok) { debugLogger.warn('post-read TOCTOU rejection (execute)', { @@ -395,12 +403,7 @@ class WriteFileToolInvocation extends BaseToolInvocation< // file from stale bytes. For new-file creation // (`fileExists === false`), ENOENT is the expected pre-write // state (ok:true → writeTextFile creates). - // - // `requireFullRead: true` only matters when stat succeeds - // (file currently exists). On the new-file path the helper - // returns ok:true via ENOENT before consulting this flag, so - // creation is still exempt regardless. - { expectExisting: fileExists, requireFullRead: true }, + { expectExisting: fileExists }, ); if (!writeDecision.ok) { debugLogger.warn('pre-write TOCTOU rejection', { diff --git a/packages/core/src/utils/fileUtils.test.ts b/packages/core/src/utils/fileUtils.test.ts index a1f529cdf3a..dcb4e41aeae 100644 --- a/packages/core/src/utils/fileUtils.test.ts +++ b/packages/core/src/utils/fileUtils.test.ts @@ -845,6 +845,116 @@ describe('fileUtils', () => { // filePathForDetectTest is already a text file by default from beforeEach expect(await detectFileType(filePathForDetectTest)).toBe('text'); }); + + it('returns text for files with a text/* mime even when the content looks binary (issue #3964 encrypted FS)', async () => { + // Frank-Shaw-FS reports `.cpp` / `.c` / `.h` 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. The extension already declares a text + // mime, so we must trust that and skip the content sample. + mockMimeGetType.mockReturnValueOnce('text/x-c'); + const filePath = path.join(tempRootDir, 'encrypted.cpp'); + // Mimic the encrypted-FS sample: leading nulls and high + // bytes that would trip isBinaryFile (>30% non-printable + // and at least one null). + const fakeEncrypted = Buffer.alloc(64); + for (let i = 0; i < fakeEncrypted.length; i++) { + fakeEncrypted[i] = i % 4 === 0 ? 0 : 0xff; + } + actualNodeFs.writeFileSync(filePath, fakeEncrypted); + try { + expect(await detectFileType(filePath)).toBe('text'); + } finally { + actualNodeFs.unlinkSync(filePath); + } + }); + + it('returns text for application/javascript and similar text-like application mimes', async () => { + mockMimeGetType.mockReturnValueOnce('application/javascript'); + expect(await detectFileType('script.js')).toBe('text'); + mockMimeGetType.mockReturnValueOnce('application/json'); + expect(await detectFileType('data.json')).toBe('text'); + mockMimeGetType.mockReturnValueOnce('application/toml'); + expect(await detectFileType('config.toml')).toBe('text'); + }); + + it('returns text for +xml and +json structured-data mime suffixes', async () => { + // Covers e.g. application/atom+xml, application/ld+json, + // application/rls-services+xml (Rust's registered mime). + mockMimeGetType.mockReturnValueOnce('application/rls-services+xml'); + expect(await detectFileType('lib.rs')).toBe('text'); + mockMimeGetType.mockReturnValueOnce('application/ld+json'); + expect(await detectFileType('schema.jsonld')).toBe('text'); + }); + + it('returns text for known source-code extensions even when content looks binary (mime/lite gap)', async () => { + // `mime/lite`'s registry omits most languages: `.py`, `.kt`, + // `.go`, `.rb`, `.swift`, ... all return null. Without a + // curated extension override, an encrypted-volume read whose + // 4 KB sample looks binary would misclassify these as binary + // even though the extension is unambiguously text. + const looksBinary = Buffer.alloc(64); + for (let i = 0; i < looksBinary.length; i++) { + looksBinary[i] = i % 4 === 0 ? 0 : 0xff; + } + for (const ext of ['.py', '.kt', '.go', '.rb', '.swift']) { + mockMimeGetType.mockReturnValueOnce(null); + const filePath = path.join(tempRootDir, `encrypted${ext}`); + actualNodeFs.writeFileSync(filePath, looksBinary); + try { + expect(await detectFileType(filePath)).toBe('text'); + } finally { + actualNodeFs.unlinkSync(filePath); + } + } + }); + + it('returns text for extensionless build/config basenames (Dockerfile, Makefile, go.mod, …)', async () => { + // Build / config / lockfile conventions carry no extension (or + // only an ambiguous one like .mod). `path.extname` returns `''`, + // so the extension allowlist misses them, and an encrypted-volume + // read whose 4 KB sample looks binary would misclassify these as + // binary even though the basename is unambiguously text. + const looksBinary = Buffer.alloc(64); + for (let i = 0; i < looksBinary.length; i++) { + looksBinary[i] = i % 4 === 0 ? 0 : 0xff; + } + for (const basename of [ + 'Dockerfile', + 'Makefile', + 'Jenkinsfile', + 'go.mod', + 'package-lock.json', + '.gitignore', + 'LICENSE', + ]) { + mockMimeGetType.mockReturnValueOnce(null); + const filePath = path.join(tempRootDir, basename); + actualNodeFs.writeFileSync(filePath, looksBinary); + try { + expect(await detectFileType(filePath)).toBe('text'); + } finally { + actualNodeFs.unlinkSync(filePath); + } + } + }); + + it('still classifies files in BINARY_EXTENSIONS as binary even with text-looking content', async () => { + // The extension overrides win-list must not weaken the + // existing binary-extension pre-empt. A `.png` whose first + // bytes happen to be ASCII still gets classified as binary + // because the extension is in BINARY_EXTENSIONS. + mockMimeGetType.mockReturnValueOnce(null); + const filePath = path.join(tempRootDir, 'looksLikeText.png'); + actualNodeFs.writeFileSync(filePath, 'PNGheader plain text'); + try { + expect(await detectFileType(filePath)).toBe('binary'); + } finally { + actualNodeFs.unlinkSync(filePath); + } + }); }); describe('processSingleFileContent', () => { diff --git a/packages/core/src/utils/fileUtils.ts b/packages/core/src/utils/fileUtils.ts index 14b1a8b9dbc..9d4f021c02c 100644 --- a/packages/core/src/utils/fileUtils.ts +++ b/packages/core/src/utils/fileUtils.ts @@ -468,6 +468,247 @@ export type FileType = | 'svg' | 'notebook'; +/** + * `application/*` mime types that the `mime/lite` registry actually + * returns for some extension and that name an unambiguously text + * payload. Trusting these in {@link detectFileType} lets files + * bearing them skip the content-based `isBinaryFile` heuristic — + * that 4 KB sample can produce false positives on UTF-16 / UTF-32 + * without BOM and on encrypted / DRM-protected file systems where + * the OS surfaces encrypted bytes to `fs.open()` reads (the Windows + * scenario in issue #3964). + * + * Scope rule: every entry must be a value `mime/lite` actually emits + * from `getType()` for some file extension. `application/x-sh`, + * `application/x-perl`, `application/x-yaml`, `application/x-tex`, + * `application/x-sql`, `application/graphql`, etc. are real mime + * names that show up in HTTP `Content-Type` contexts but are not in + * the lite registry, so listing them here would be dead code that + * silently activates if the registry is later expanded. The shells / + * tex / sql / graphql extensions reach the text fallback through + * {@link KNOWN_TEXT_EXTENSIONS} below instead. + * + * Anything not in this set still falls through to the content check. + * Mimes ending in `+xml` / `+json` are accepted via suffix match + * rather than enumeration, since structured-data formats keep + * extending those families. + */ +const KNOWN_TEXT_APPLICATION_MIMES: ReadonlySet = new Set([ + 'application/javascript', + 'application/ecmascript', + 'application/node', + 'application/json', + 'application/xml', + 'application/toml', +]); + +/** + * Source-code, config, and markup extensions that `mime/lite` either + * does not register or registers ambiguously, but which are + * unambiguously text in practice. Trusting the extension here means + * a file like `Trigger.kt` or `analysis.py` on an encrypted file + * system whose raw bytes look binary to `isBinaryFile`'s 4 KB + * sample is still classified as text — the fix for the Windows + * scenario in issue #3964. + * + * Scope: only languages and config formats commonly encountered in + * codebases that have been reported in the field, plus a few core + * markup / build formats. Anything more obscure still falls through + * to the content sampler — the goal is "do not lie about a known + * source-code extension", not "be exhaustive". + * + * Maintenance note: `path.extname()` returns `''` for dotfiles + * (`.gitignore`, `.editorconfig`), so this set cannot cover those. + * They go through the content sampler, which handles them fine on + * non-encrypted file systems. Adding a separate basename allowlist + * is a possible future extension if needed. + */ +const KNOWN_TEXT_EXTENSIONS: ReadonlySet = new Set([ + // C / C++ + '.c', + '.cc', + '.cpp', + '.cxx', + '.h', + '.hh', + '.hpp', + '.hxx', + '.inl', + '.tpp', + // Python + '.py', + '.pyi', + '.pyw', + '.pyx', + // Rust + '.rs', + // Go + '.go', + // JVM + '.gradle', + '.groovy', + '.java', + '.kt', + '.kts', + '.sc', + '.scala', + // .NET + '.cs', + '.fs', + '.fsi', + '.fsx', + '.vb', + // Apple platforms + '.m', + '.mm', + '.swift', + // Functional + '.cljc', + '.cljs', + '.clj', + '.edn', + '.erl', + '.ex', + '.exs', + '.hrl', + '.hs', + '.lhs', + '.ml', + '.mli', + // Web frontend (`.tsx` is handled by the early-return at the top + // of detectFileType alongside `.ts` / `.mts` / `.cts` to keep all + // TypeScript-family extensions in one place). + '.astro', + '.jsx', + '.svelte', + '.vue', + // Scripting + '.bash', + '.dart', + '.fish', + '.lua', + '.php', + '.pl', + '.pm', + '.ps1', + '.r', + '.rb', + '.sh', + '.zsh', + // Newer / niche source languages + '.cr', + '.nim', + '.sol', + '.zig', + // Schema / IDL / queries + '.gql', + '.graphql', + '.proto', + '.sql', + '.thrift', + // Markup / typesetting + '.adoc', + '.bib', + '.org', + '.rst', + '.tex', + // Config / build + '.cfg', + '.cmake', + '.conf', + '.containerfile', + '.dockerfile', + '.hcl', + '.ini', + '.mk', + '.nomad', + '.properties', + '.tf', + '.tfvars', + '.toml', +]); + +/** + * Basename-only fallback for files whose name carries no extension + * but is unambiguously text (build / config / lockfile conventions). + * `path.extname('Dockerfile')` / `path.extname('Makefile')` / + * `path.extname('go.mod')` return `''` (or just `'.mod'` for go.mod — + * not enough to disambiguate from binary `.mod` payloads), so the + * extension-only `KNOWN_TEXT_EXTENSIONS` check above misses them and + * an encrypted-volume read whose 4 KB sample looks binary would + * misclassify these as binary. + */ +const KNOWN_TEXT_BASENAMES: ReadonlySet = new Set([ + 'Dockerfile', + 'Containerfile', + 'Makefile', + 'GNUmakefile', + 'Jenkinsfile', + 'Vagrantfile', + 'Rakefile', + 'Gemfile', + 'Procfile', + 'BUILD', + 'WORKSPACE', + 'CMakeLists.txt', // also caught by .txt but pin explicitly + 'go.mod', + 'go.sum', + 'go.work', + 'Cargo.lock', + 'Pipfile', + 'Pipfile.lock', + 'poetry.lock', + 'package-lock.json', + 'yarn.lock', + 'pnpm-lock.yaml', + 'requirements.txt', + '.gitignore', + '.gitattributes', + '.dockerignore', + '.npmignore', + '.editorconfig', + '.env', + '.bashrc', + '.zshrc', + '.profile', + 'LICENSE', + 'COPYING', + 'AUTHORS', + 'CHANGELOG', + 'README', + 'NOTICE', +]); + +/** + * Decide whether a mime registry entry is a text payload that the + * Edit / WriteFile tools can safely mutate as text. Used by {@link + * detectFileType} to avoid running `isBinaryFile` content sampling + * on files whose extension is registered as text — the sampling + * misclassifies UTF-16 without BOM, encrypted / DRM-protected + * volumes, and other plain-text payloads whose first 4 KB happen to + * include nulls / non-printables. + * + * Tradeoff: returning `true` short-circuits `isBinaryFile` entirely, + * including the safety net it provides for *corrupted* text files + * (e.g. a binary blob accidentally saved with a `.txt` / `.md` + * extension via `cat blob.dat > notes.md`). After this fix the + * corrupted-text case is misclassified as text and Edit will see + * garbled string content from `readTextFile`; the corresponding + * `0 occurrences` failure on Edit's `old_string` match is the + * fallback for that population. The encrypted-FS population (issue + * #3964) is the one we are *trying* to serve here, and the + * extension-declared mime is the strongest signal we have for it. + */ +function isTextMime(lookedUpMimeType: string): boolean { + if (lookedUpMimeType.startsWith('text/')) { + return true; + } + if (lookedUpMimeType.endsWith('+xml') || lookedUpMimeType.endsWith('+json')) { + return true; + } + return KNOWN_TEXT_APPLICATION_MIMES.has(lookedUpMimeType); +} + /** * Detects the type of file based on extension and content. * @param filePath Path to the file. @@ -478,8 +719,11 @@ export async function detectFileType(filePath: string): Promise { // The mimetype for various TypeScript extensions (ts, mts, cts, tsx) can be // MPEG transport stream (a video format), but we want to assume these are - // TypeScript files instead. - if (['.ts', '.mts', '.cts'].includes(ext)) { + // TypeScript files instead. `.tsx` is currently absent from the mime/lite + // registry but listed here defensively: if a future registry update mapped + // it to `video/mp2t` (mirroring `.ts`), the `startsWith('video/')` guard + // below would fire before reaching the text fallback. + if (['.ts', '.mts', '.cts', '.tsx'].includes(ext)) { return 'text'; } @@ -505,6 +749,23 @@ export async function detectFileType(filePath: string): Promise { if (lookedUpMimeType === 'application/pdf') { return 'pdf'; } + // Trust the registry for declared text payloads. Skipping the + // `isBinaryFile` content sampler below avoids false positives + // on UTF-16 / UTF-32 without BOM and on encrypted file systems + // (issue #3964 Windows scenario): when the extension already + // declares a text mime, the bytes are text even if the first + // 4 KB look binary on a raw read. + if (isTextMime(lookedUpMimeType)) { + // Log the classification path so future #3964-class + // troubleshooting can tell mime-trust apart from extension + // override and the content-sample fallback below — without + // having to re-derive which fast-path fired by reading the + // code. Cheap at debug level; off by default. + debugLogger.debug( + `detectFileType: ${filePath} → text (mime-trust: ${lookedUpMimeType})`, + ); + return 'text'; + } } // Stricter binary check for common non-text extensions before content check @@ -513,8 +774,31 @@ export async function detectFileType(filePath: string): Promise { return 'binary'; } + // Curated source-code / config / markup extensions. The `mime/lite` + // registry omits most languages (`.py`, `.kt`, `.cpp`, `.go`, ...); + // without this set, an encrypted-volume read whose 4 KB sample + // looks binary would misclassify these as binary even though the + // extension is unambiguously text. Issue #3964 reproduced exactly + // this on `.c` / `.cpp` / `.h` files. + if (KNOWN_TEXT_EXTENSIONS.has(ext)) { + debugLogger.debug( + `detectFileType: ${filePath} → text (extension-override, mime ${lookedUpMimeType ?? 'null'})`, + ); + return 'text'; + } + // Basename-only allowlist for extensionless build / config / lockfiles + // (Dockerfile, Makefile, Jenkinsfile, go.mod, package-lock.json, ...) + // that the extension check above misses. See KNOWN_TEXT_BASENAMES + // for the full list. + if (KNOWN_TEXT_BASENAMES.has(path.basename(filePath))) { + debugLogger.debug( + `detectFileType: ${filePath} → text (basename-override, mime ${lookedUpMimeType ?? 'null'})`, + ); + return 'text'; + } + // Fall back to content-based check if mime type wasn't conclusive for image/pdf - // and it's not a known binary extension. + // and it's not a known binary or known text extension. if (await isBinaryFile(filePath)) { return 'binary'; }