fix(core): accept partial reads in prior-read enforcement#3932
Merged
Conversation
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
commented
May 7, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
wenshao
commented
May 7, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Critical] Two-commit structure — intermediate state reintroduces data-loss scenario (issue #2499)
The PR contains two commits: afc1b917 unconditionally drops lastReadWasFull from checkPriorRead (allowing partial-read-then-WriteFile overwrite), and 503fc0b0 restores WriteFile's protection via the new requireFullRead option. If these commits are ever separated — cherry-pick, bisect, or merge-commit strategy that preserves individual commits — the first commit alone permits the exact data-loss path this PR exists to prevent.
Suggested fix: Squash into a single commit before merging, or reorder so requireFullRead + WriteFile wiring lands first, then the Edit relaxation.
— glm-5.1 via Qwen Code /review
…quired for WriteFile PR #3774 introduced a `lastReadWasFull` requirement to `checkPriorRead` that forced models to re-read multi-thousand-line files just to make a single-line edit. 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 — but only for Edit, not for WriteFile. WriteFile is asymmetric: it replaces the entire file and has no content-derived guard equivalent to `old_string` matching. A model that has only seen a slice via `read_file(offset, limit)` followed by a `WriteFile` would necessarily hallucinate the rest of the bytes — the issue #2499 data-loss scenario PR #3774 was opened to address. Split the policy along that line. `checkPriorRead` gains a `requireFullRead?: boolean` option. WriteFileTool's 5 enforcement call sites pass `true`; EditTool's 3 leave it unset (default `false`): - EditTool: partial read counts (old_string is the floor) - WriteFileTool overwrite: full read required - Either: new-file creation exempt (ENOENT → ok:true before requireFullRead is consulted); `fileReadCacheDisabled` escape hatch unchanged A dedicated `fresh + cacheable + partial + requireFullRead` rejection branch surfaces a clear "only been partially read … overwriting replaces the entire file" message instead of falling through to the generic "has not been read" wording. The `unknown` branch's wording also varies by `requireFullRead` so the read instruction matches the operation's actual requirement. For comparison, Claude Code's `readFileState` enforcement treats partial and full reads identically for both Edit and WriteFile. This PR is stricter on WriteFile (full read required) and identical on Edit (partial OK). Issue #2499 is empirical evidence that the partial-read-then-overwrite case is real on at least some model populations, so the additional WriteFile constraint is justified. Single-commit shape (versus the earlier afc1b91 / 503fc0b split) to avoid an intermediate state in which Edit's relaxation has landed but WriteFile is still on the relaxed path: cherry-picks or bisect walks crossing such a boundary would re-introduce the issue #2499 data-loss case. Tests: edit.test.ts ranged-read test inverted to "allows after ranged read"; write-file.test.ts ranged-read test asserts the new partial / full-required message. Three error-message regex matchers updated from /fully read/ to /read/. 198 / 198 prior-read-related tests pass; tsc --noEmit clean.
503fc0b to
a182029
Compare
Collaborator
Author
|
@wenshao re: glm-5.1 review — accepted. Squashed to a single commit The single commit's message also explicitly calls out why it is one commit, so a future split is less likely. 198/198 prior-read tests still pass against the squashed tree; CI is re-running. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts prior-read enforcement for file mutations so that partial reads are accepted for in-place Edit, while full reads are still required before WriteFile overwrites an existing file. This aligns enforcement with the asymmetric safety properties of the tools (Edit has old_string matching as a guard; WriteFile overwrite does not), and directly targets the data-loss scenario described in issue #2499.
Changes:
- Adds
requireFullRead?: booleantocheckPriorReadso callers can require a full-file read when necessary. - Updates
WriteFileToolto passrequireFullRead: trueat all overwrite enforcement points (including TOCTOU rechecks). - Updates tests to reflect the new policy: Edit allows ranged reads; WriteFile rejects ranged reads with a dedicated, clearer error message.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/core/src/tools/priorReadEnforcement.ts | Adds requireFullRead option, adjusts acceptance logic, and introduces a dedicated rejection path/message for “partial read + overwrite requires full read”. |
| packages/core/src/tools/write-file.ts | Requires full reads for all overwrite enforcement call sites by passing requireFullRead: true (including post-read and pre-write rechecks). |
| packages/core/src/tools/write-file.test.ts | Updates enforcement expectations and asserts the new “partial read is insufficient for overwrite” message. |
| packages/core/src/tools/edit.test.ts | Updates enforcement expectations so ranged reads satisfy prior-read enforcement for edits and validates the edit succeeds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tanzhenxin
approved these changes
May 8, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
This is a small, well-scoped fix for a real UX regression: forcing a full re-read of a multi-thousand-line file just to make a one-line edit is a meaningful context cost, and old_string matching already gives Edit a content-derived guard against fabrication. Splitting the policy — Edit accepts partial reads, WriteFile keeps the full-read requirement — lines up with the actual asymmetry between the two operations. The retained guard on WriteFile still closes the issue #2499 hole.
The reasoning is documented in code, all WriteFile call sites (including the post-read TOCTOU re-checks) opt into the stricter mode, and the inode-keyed cache plus mtime/size mismatch handle the delete-then-recreate edge case correctly. Error prose is well-tailored — the Edit-side message explicitly tells the model that a ranged read is fine, and the new partial-read branch names the exact remediation.
Verdict
APPROVE — Correct, well-scoped, and clearly documented.
DragonnZhang
pushed a commit
that referenced
this pull request
May 8, 2026
…quired for WriteFile (#3932) PR #3774 introduced a `lastReadWasFull` requirement to `checkPriorRead` that forced models to re-read multi-thousand-line files just to make a single-line edit. 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 — but only for Edit, not for WriteFile. WriteFile is asymmetric: it replaces the entire file and has no content-derived guard equivalent to `old_string` matching. A model that has only seen a slice via `read_file(offset, limit)` followed by a `WriteFile` would necessarily hallucinate the rest of the bytes — the issue #2499 data-loss scenario PR #3774 was opened to address. Split the policy along that line. `checkPriorRead` gains a `requireFullRead?: boolean` option. WriteFileTool's 5 enforcement call sites pass `true`; EditTool's 3 leave it unset (default `false`): - EditTool: partial read counts (old_string is the floor) - WriteFileTool overwrite: full read required - Either: new-file creation exempt (ENOENT → ok:true before requireFullRead is consulted); `fileReadCacheDisabled` escape hatch unchanged A dedicated `fresh + cacheable + partial + requireFullRead` rejection branch surfaces a clear "only been partially read … overwriting replaces the entire file" message instead of falling through to the generic "has not been read" wording. The `unknown` branch's wording also varies by `requireFullRead` so the read instruction matches the operation's actual requirement. For comparison, Claude Code's `readFileState` enforcement treats partial and full reads identically for both Edit and WriteFile. This PR is stricter on WriteFile (full read required) and identical on Edit (partial OK). Issue #2499 is empirical evidence that the partial-read-then-overwrite case is real on at least some model populations, so the additional WriteFile constraint is justified. Single-commit shape (versus the earlier afc1b91 / 503fc0b split) to avoid an intermediate state in which Edit's relaxation has landed but WriteFile is still on the relaxed path: cherry-picks or bisect walks crossing such a boundary would re-introduce the issue #2499 data-loss case. Tests: edit.test.ts ranged-read test inverted to "allows after ranged read"; write-file.test.ts ranged-read test asserts the new partial / full-required message. Three error-message regex matchers updated from /fully read/ to /read/. 198 / 198 prior-read-related tests pass; tsc --noEmit clean.
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 8, 2026
…quired for WriteFile (QwenLM#3932) PR QwenLM#3774 introduced a `lastReadWasFull` requirement to `checkPriorRead` that forced models to re-read multi-thousand-line files just to make a single-line edit. 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 — but only for Edit, not for WriteFile. WriteFile is asymmetric: it replaces the entire file and has no content-derived guard equivalent to `old_string` matching. A model that has only seen a slice via `read_file(offset, limit)` followed by a `WriteFile` would necessarily hallucinate the rest of the bytes — the issue QwenLM#2499 data-loss scenario PR QwenLM#3774 was opened to address. Split the policy along that line. `checkPriorRead` gains a `requireFullRead?: boolean` option. WriteFileTool's 5 enforcement call sites pass `true`; EditTool's 3 leave it unset (default `false`): - EditTool: partial read counts (old_string is the floor) - WriteFileTool overwrite: full read required - Either: new-file creation exempt (ENOENT → ok:true before requireFullRead is consulted); `fileReadCacheDisabled` escape hatch unchanged A dedicated `fresh + cacheable + partial + requireFullRead` rejection branch surfaces a clear "only been partially read … overwriting replaces the entire file" message instead of falling through to the generic "has not been read" wording. The `unknown` branch's wording also varies by `requireFullRead` so the read instruction matches the operation's actual requirement. For comparison, Claude Code's `readFileState` enforcement treats partial and full reads identically for both Edit and WriteFile. This PR is stricter on WriteFile (full read required) and identical on Edit (partial OK). Issue QwenLM#2499 is empirical evidence that the partial-read-then-overwrite case is real on at least some model populations, so the additional WriteFile constraint is justified. Single-commit shape (versus the earlier afc1b91 / 503fc0b split) to avoid an intermediate state in which Edit's relaxation has landed but WriteFile is still on the relaxed path: cherry-picks or bisect walks crossing such a boundary would re-introduce the issue QwenLM#2499 data-loss case. Tests: edit.test.ts ranged-read test inverted to "allows after ranged read"; write-file.test.ts ranged-read test asserts the new partial / full-required message. Three error-message regex matchers updated from /fully read/ to /read/. 198 / 198 prior-read-related tests pass; tsc --noEmit clean.
B-A-M-N
pushed a commit
to B-A-M-N/qwen-code
that referenced
this pull request
May 8, 2026
…quired for WriteFile (QwenLM#3932) PR QwenLM#3774 introduced a `lastReadWasFull` requirement to `checkPriorRead` that forced models to re-read multi-thousand-line files just to make a single-line edit. 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 — but only for Edit, not for WriteFile. WriteFile is asymmetric: it replaces the entire file and has no content-derived guard equivalent to `old_string` matching. A model that has only seen a slice via `read_file(offset, limit)` followed by a `WriteFile` would necessarily hallucinate the rest of the bytes — the issue QwenLM#2499 data-loss scenario PR QwenLM#3774 was opened to address. Split the policy along that line. `checkPriorRead` gains a `requireFullRead?: boolean` option. WriteFileTool's 5 enforcement call sites pass `true`; EditTool's 3 leave it unset (default `false`): - EditTool: partial read counts (old_string is the floor) - WriteFileTool overwrite: full read required - Either: new-file creation exempt (ENOENT → ok:true before requireFullRead is consulted); `fileReadCacheDisabled` escape hatch unchanged A dedicated `fresh + cacheable + partial + requireFullRead` rejection branch surfaces a clear "only been partially read … overwriting replaces the entire file" message instead of falling through to the generic "has not been read" wording. The `unknown` branch's wording also varies by `requireFullRead` so the read instruction matches the operation's actual requirement. For comparison, Claude Code's `readFileState` enforcement treats partial and full reads identically for both Edit and WriteFile. This PR is stricter on WriteFile (full read required) and identical on Edit (partial OK). Issue QwenLM#2499 is empirical evidence that the partial-read-then-overwrite case is real on at least some model populations, so the additional WriteFile constraint is justified. Single-commit shape (versus the earlier afc1b91 / 503fc0b split) to avoid an intermediate state in which Edit's relaxation has landed but WriteFile is still on the relaxed path: cherry-picks or bisect walks crossing such a boundary would re-introduce the issue QwenLM#2499 data-loss case. Tests: edit.test.ts ranged-read test inverted to "allows after ranged read"; write-file.test.ts ranged-read test asserts the new partial / full-required message. Three error-message regex matchers updated from /fully read/ to /read/. 198 / 198 prior-read-related tests pass; tsc --noEmit clean.
4 tasks
This was referenced May 9, 2026
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
May 9, 2026
… Code PR QwenLM#3932 deliberately diverged from Claude Code's `readFileState` by keeping `requireFullRead: true` on WriteFile's prior-read enforcement, citing issue QwenLM#2499 (LLM hallucinates content of an unread file and clobbers user changes) as evidence that the asymmetric stance was justified. In practice that stance leaves a hard deadlock: when a file is larger than `truncate-tool-output- lines`, `read_file` without offset/limit still records `lastReadWasFull: false` (the model only saw the head), and the "only been partially read … re-read without offset / limit / pages" rejection sends the model back to the same truncated read with no escape — the exact deadlock issue QwenLM#3945 reported. Drop the `requireFullRead` option from `checkPriorRead` and remove all 5 `requireFullRead: true` call sites in WriteFileTool. After this change the contract is identical to Claude Code's: any prior read of an existing file clears enforcement; the mtime/size drift check is the only gate that distinguishes "the model has seen current bytes" from "the model has seen older bytes", and it fires identically for Edit and WriteFile. The residual QwenLM#2499 risk is acknowledged in the docstring: a model that reads only a slice and then overwrites would necessarily hallucinate the rest of the bytes. Mitigations: - `fileReadCacheDisabled: true` for users who want stricter behaviour (existing escape hatch, unchanged). - The mtime/size drift check still rejects Writes against bytes the model saw at fingerprint X if disk has moved to Y. Cleanup: drops the dedicated "fresh + cacheable + partial + requireFullRead" rejection branch and the `requireFullRead`-aware wording variant in the `unknown` branch — both unreachable now. Tests: - `write-file.test.ts:932` inverted from "rejects a write when the previous read was ranged" to "allows a write after a ranged read", matching the equivalent `edit.test.ts:1077`. - New `write-file.test.ts:961` regression for the issue QwenLM#3945 deadlock: a `recordRead({ full: false, cacheable: true })` entry (what a truncated full read produces) clears WriteFile enforcement. - 7393 / 7398 (5 skipped, all pre-existing) on the full core suite. `tsc --noEmit` clean.
wenshao
added a commit
to wenshao/qwen-code
that referenced
this pull request
May 9, 2026
Three sites a future contributor might naturally try to "tighten up" back into the deadlock-prone shape, now carrying explicit guard comments that name the prior PR (QwenLM#3932), the issue it broke (QwenLM#3945), and the residual risk this stance accepts (QwenLM#2499): - `priorReadEnforcement.ts:CheckPriorReadOptions` — interface-level note: do not re-introduce `requireFullRead` (or any "stricter for WriteFile than Edit") option here. References the function docstring for the full rationale. - `fileReadCache.ts:lastReadWasFull` — field-level note: sole consumer is the Read fast-path; `priorReadEnforcement` does not consult this and must not start. - `write-file.ts` first checkPriorRead call site — anchor comment that explains why no extra option is passed and applies to all 5 call sites in the file. No code changes; test suite unchanged at 7393 / 7398 (5 pre-existing skips); `tsc --noEmit` clean.
wenshao
added a commit
that referenced
this pull request
May 10, 2026
+ #3945 (#4002) * fix(core): decouple cacheable flag from truncation in FileReadCache PR #3774 introduced prior-read enforcement that consults `lastReadCacheable` to discriminate text from binary / image / PDF / notebook payloads. ReadFileToolInvocation derived `cacheable` as `string && originalLineCount && !isTruncated`, conflating two unrelated concerns: "is the content text" and "did we see all the bytes". A partial read (offset/limit) or a truncated full read of a regular `.kt` / `.cpp` / `.py` source file therefore set `cacheable: false`, and priorReadEnforcement.ts mistook that for a non-text payload and rejected the next Edit with the misleading "binary / image / audio / video / PDF / notebook payload" error. PR #3932 split prior-read enforcement so Edit accepts partial reads (`lastReadWasFull`-relaxed for Edit, kept for WriteFile), but the `lastReadCacheable` conflation persisted, so partial / truncated text reads still hit the binary-payload rejection on Edit. Issue #3964 is the resulting field reports: .kt / .cpp / .py / .ts files on both Linux and Windows misclassified as binary across 0.15.7-0.15.9. Decouple the two concerns: - `cacheable` is now purely about content type. A partial or truncated text read records `cacheable: true` because the bytes the model saw were text. - Truncation gating moves to `full`. A request-level full read (no offset/limit/pages) only counts as full at the cache level when the produced content was not truncated; otherwise the model only saw the head of the file. The fast-path `file_unchanged` placeholder still requires both `lastReadWasFull && lastReadCacheable`, so its semantics are unchanged — a truncated full read now fails the AND on the moved flag instead of the original. WriteFile's `requireFullRead` still rejects partial or truncated text reads; it now reports the accurate "partial read" error instead of the wrong "binary payload" message. Also fixes issue #3945 (edit tool unusable for large files) as a side effect: the truncated-full case there hit the same misclassified path before the rejection wording could even surface the truncation question. Tests: 2 regression tests added in read-file.test.ts (partial .kt read and truncated full .cpp read both record `lastReadCacheable: true`). Existing 7386 / 7391 (5 skipped) core tests pass; tsc --noEmit clean. Issue #3964 also reports a separate scenario on Windows encrypted/DRM-protected file systems where .cpp source files are misclassified by `isBinaryFile`'s 4KB content sampling. That path is content-detection-side, not cache-side, and is left to a follow-up (extension- or mime-based override of the content sample for known text types). * fix(core): trust extension/mime over isBinaryFile sampling for known text Issue #3964's first report (Frank-Shaw-FS) describes `.cpp` / `.c` / `.h` source files on a Windows encrypted / DRM-protected file system being misclassified as binary. The OS surfaces encrypted bytes to `fs.open()` random-access reads, so `isBinaryFile`'s 4 KB sample sees nulls / non-printable characters and concludes binary — even though the higher-level `readFile` returns the decrypted text and the extension declares the file as text. Layer-2 fix on top of the cache-side decoupling: change `detectFileType` to trust the registry / curated extension list *before* running the content sample, so a known text extension is not subject to false positives from raw-bytes sampling. - Trust mime types declared as text: `text/*`, `application/*` text-likes (`application/javascript`, `application/json`, `application/toml`, ...), and any mime ending in `+xml` / `+json`. - Trust a curated set of source-code / config / markup extensions whose `mime/lite` registry coverage is patchy (`.py`, `.kt`, `.go`, `.rb`, `.swift`, `.scala`, `.rs`, `.proto`, `.graphql`, `.toml`, `.hcl`, `.tf`, ...). The list is restricted to extensions we have observed to be misclassified by `isBinaryFile` in the field; obscure extensions still go through the content sampler. Order in `detectFileType`: 1. Hardcoded `.ts` / `.svg` / `.ipynb` 2. Mime check (image / audio / video / pdf / declared-text) 3. `BINARY_EXTENSIONS` pre-empt (so `.png` with text-looking content stays binary) 4. Curated text extension override (for mime-less source code) 5. `isBinaryFile` content sampler (final fallback for unrecognised extensions) 6. Default text Tests: 5 new cases in `fileUtils.test.ts` and 1 end-to-end in `read-file.test.ts` covering: text mime override on binary-looking content, application/* text mimes, `+xml` / `+json` suffix match, curated extension override on `.py` / `.kt` / `.go` / `.rb` / `.swift`, and the `BINARY_EXTENSIONS` pre-empt still winning over the new override (a `.png` whose first bytes happen to be ASCII text stays binary). Full core suite passes (7392 / 7397, 5 pre- existing skips); `tsc --noEmit` clean. Together with the earlier commit, this PR closes both arms of issue #3964: the cache-side `cacheable` conflation that affected partial / truncated reads, and the content-detection-side false positive on encrypted file systems. * fix(core): tighten detectFileType after self-review on #4002 Three follow-ups flagged by `/review` on #4002: 1. `KNOWN_TEXT_APPLICATION_MIMES` had 10 dead entries — names like `application/x-sh`, `application/x-perl`, `application/x-yaml`, `application/x-tex`, `application/x-sql`, `application/graphql` are real mimes seen in HTTP `Content-Type` contexts but are not in `mime/lite`'s registry, so `mime.getType()` never returns them and the entries are unreachable. Strip the set to the 6 values the registry actually emits (`javascript`, `ecmascript`, `node`, `json`, `xml`, `toml`); the shells / tex / sql / graphql extensions reach the text fallback through `KNOWN_TEXT_EXTENSIONS` instead. Add a scope rule in the docstring so future additions stay aligned with what mime/lite actually emits. 2. The early-return at the top of `detectFileType` listed `.ts / .mts / .cts / .tsx` in its comment but the array only contained `.ts / .mts / .cts`. `.tsx` was reaching the text verdict via `KNOWN_TEXT_EXTENSIONS`, which works today but would break if a future `mime/lite` update mapped `.tsx` to `video/mp2t` (mirroring `.ts`): the `startsWith('video/')` guard would fire before the text fallback. Move `.tsx` up to the early-return array so the comment matches the code and the defence is consistent across the TypeScript family. Drop the duplicate listing in `KNOWN_TEXT_EXTENSIONS`. 3. `isTextMime()` short-circuits `isBinaryFile` for any `text/*` mime, which is the necessary tradeoff for the encrypted-FS fix but removes the safety net for *corrupted* text files (a binary blob saved with a `.txt` / `.md` extension via redirection). Document the tradeoff explicitly with a concrete counter-example and call out that Edit's `0 occurrences` failure mode is the fallback for the corrupted-text population. Tests: 261 / 262 (1 skipped, pre-existing) on `fileUtils.test.ts` + `read-file.test.ts` + `edit.test.ts` + `write-file.test.ts`. `tsc --noEmit` clean. * fix(core): drop full-read requirement on WriteFile, align with Claude Code PR #3932 deliberately diverged from Claude Code's `readFileState` by keeping `requireFullRead: true` on WriteFile's prior-read enforcement, citing issue #2499 (LLM hallucinates content of an unread file and clobbers user changes) as evidence that the asymmetric stance was justified. In practice that stance leaves a hard deadlock: when a file is larger than `truncate-tool-output- lines`, `read_file` without offset/limit still records `lastReadWasFull: false` (the model only saw the head), and the "only been partially read … re-read without offset / limit / pages" rejection sends the model back to the same truncated read with no escape — the exact deadlock issue #3945 reported. Drop the `requireFullRead` option from `checkPriorRead` and remove all 5 `requireFullRead: true` call sites in WriteFileTool. After this change the contract is identical to Claude Code's: any prior read of an existing file clears enforcement; the mtime/size drift check is the only gate that distinguishes "the model has seen current bytes" from "the model has seen older bytes", and it fires identically for Edit and WriteFile. The residual #2499 risk is acknowledged in the docstring: a model that reads only a slice and then overwrites would necessarily hallucinate the rest of the bytes. Mitigations: - `fileReadCacheDisabled: true` for users who want stricter behaviour (existing escape hatch, unchanged). - The mtime/size drift check still rejects Writes against bytes the model saw at fingerprint X if disk has moved to Y. Cleanup: drops the dedicated "fresh + cacheable + partial + requireFullRead" rejection branch and the `requireFullRead`-aware wording variant in the `unknown` branch — both unreachable now. Tests: - `write-file.test.ts:932` inverted from "rejects a write when the previous read was ranged" to "allows a write after a ranged read", matching the equivalent `edit.test.ts:1077`. - New `write-file.test.ts:961` regression for the issue #3945 deadlock: a `recordRead({ full: false, cacheable: true })` entry (what a truncated full read produces) clears WriteFile enforcement. - 7393 / 7398 (5 skipped, all pre-existing) on the full core suite. `tsc --noEmit` clean. * docs(core): add anti-regression notes locking in the WriteFile relax Three sites a future contributor might naturally try to "tighten up" back into the deadlock-prone shape, now carrying explicit guard comments that name the prior PR (#3932), the issue it broke (#3945), and the residual risk this stance accepts (#2499): - `priorReadEnforcement.ts:CheckPriorReadOptions` — interface-level note: do not re-introduce `requireFullRead` (or any "stricter for WriteFile than Edit") option here. References the function docstring for the full rationale. - `fileReadCache.ts:lastReadWasFull` — field-level note: sole consumer is the Read fast-path; `priorReadEnforcement` does not consult this and must not start. - `write-file.ts` first checkPriorRead call site — anchor comment that explains why no extra option is passed and applies to all 5 call sites in the file. No code changes; test suite unchanged at 7393 / 7398 (5 pre-existing skips); `tsc --noEmit` clean. * fix(core): #4002 review wave — basename allowlist + correct stale comments 3 #4002 review threads addressed: - fileUtils.ts: added KNOWN_TEXT_BASENAMES allowlist for extensionless build / config / lockfiles (Dockerfile, Containerfile, Makefile, GNUmakefile, Jenkinsfile, Vagrantfile, Rakefile, Gemfile, Procfile, BUILD, WORKSPACE, CMakeLists.txt, 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). `path.extname('Dockerfile')` returns `''`, so the KNOWN_TEXT_EXTENSIONS check above misses these — an encrypted-volume read whose 4 KB sample looks binary would misclassify them as binary. Regression test pinned with fake-encrypted bytes for Dockerfile / Makefile / Jenkinsfile / go.mod / package-lock.json / .gitignore / LICENSE. - priorReadEnforcement.ts: rewrote two misleading comments that pointed users to `fileReadCacheDisabled: true` for "stricter behaviour". That setting actually DISABLES enforcement entirely (skips checkPriorRead). Updated to make the opt-out semantics explicit and clarify that there is no built-in stricter mode — users who want stricter built-in enforcement than the residual #2499 risk accepts have no flag here today and should file a feature request. - read-file.ts: updated the `lastReadWasFull` comment to reflect that PR #4002 removed WriteFile's `requireFullRead`. The flag now gates ONLY the `file_unchanged` fast-path; the stale "and WriteFile's full-read requirement" wording would have confused future readers into thinking WriteFile still consults `lastReadWasFull`. Tests: 89/89 fileUtils.test.ts pass; tsc + ESLint clean. * fix(core): split priorReadEnforcement guidance — partial OK for edit, full for overwrite #4002 review: shared "never read" error said `(a partial read with offset / limit is fine — you only need to have seen the bytes you intend to edit/overwrite)` for BOTH Edit and WriteFile. For Edit this is correct — the model only needs to have seen the `old_string`-bearing bytes; the rest passes through untouched. For WriteFile this is misleading: overwriting replaces EVERY byte, 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 in the WriteFile guidance would actively encourage the footgun. Fix: branch the partial-read guidance on `verb`. Edit keeps the current "partial OK" text. WriteFile gets `(read the full file — overwriting replaces every byte, so any unseen bytes would be discarded)`. 120/120 edit + write-file tests pass; tsc + ESLint clean. * docs(core): finish #4002 review wave — drop two stale "fileReadCacheDisabled is escape hatch" mentions cc30278 + c6e2bde addressed 4 of the 6 #4002 review threads but left two prior occurrences of the misleading "fileReadCacheDisabled: true is the escape hatch for users who want stricter behaviour" wording untouched. The flag actually goes the OPPOSITE way (skips checkPriorRead entirely so application-level locking can take over), so describing it as a "stricter" escape hatch is exactly the guidance the c6e2bde review thread asked us to stop giving. Files updated: - fileReadCache.ts:lastReadWasFull docstring — replaces the "stricter behaviour" sentence with the same opt-out / opt-in distinction c6e2bde used in priorReadEnforcement.ts. - write-file.ts anchor comment for all 5 checkPriorRead call sites — replaces the "fileReadCacheDisabled: true is the escape hatch" sentence with an explicit note on the opt-out direction matching the docstring. Plus a coverage-split comment on the issue #3945 deadlock-free regression test in write-file.test.ts (review thread #6 from glm-5.1: pointed out the test seeds the cache directly rather than driving a full ReadFile→WriteFile pipeline). A real integration test would need ReadFile-side mockConfig plumbing (`getFileService`, `getTruncateToolOutputLines`, etc.) ported into write-file.test.ts; the comment captures the link to read-file.test.ts's matching cache-population assertion so a future cache-entry schema change has to update both halves to keep the end-to-end guarantee. Tests: 295 / 296 (1 pre-existing skip) on the affected files; tsc --noEmit clean. * chore(core): add debug logs to detectFileType text fast-paths #4002 review (DeepSeek): the new text-classification branches returned `'text'` without logging which path fired, leaving future #3964-class troubleshooting unable to tell mime-trust from extension-override from basename-override from the content-sample fallback without re-deriving by code reading. Add `debugLogger.debug` calls on the three new fast-path branches: mime trust (`isTextMime` match), extension override (`KNOWN_TEXT_EXTENSIONS`), and basename override (`KNOWN_TEXT_BASENAMES`). Each log includes the path, the chosen classification, and the looked-up mime when relevant — enough to disambiguate the four classification paths from a single line. Off by default (`debug` level on the `FILE_UTILS` logger). Older branches (image / audio / video / pdf / hardcoded TS / SVG / ipynb / BINARY_EXTENSIONS / isBinaryFile / default text) keep their existing silent behaviour: they predate the issue this is paving for and adding logs there would be scope creep. Tests: 89 / 89 fileUtils.test.ts pass; tsc --noEmit clean.
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
…quired for WriteFile (QwenLM#3932) PR QwenLM#3774 introduced a `lastReadWasFull` requirement to `checkPriorRead` that forced models to re-read multi-thousand-line files just to make a single-line edit. 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 — but only for Edit, not for WriteFile. WriteFile is asymmetric: it replaces the entire file and has no content-derived guard equivalent to `old_string` matching. A model that has only seen a slice via `read_file(offset, limit)` followed by a `WriteFile` would necessarily hallucinate the rest of the bytes — the issue QwenLM#2499 data-loss scenario PR QwenLM#3774 was opened to address. Split the policy along that line. `checkPriorRead` gains a `requireFullRead?: boolean` option. WriteFileTool's 5 enforcement call sites pass `true`; EditTool's 3 leave it unset (default `false`): - EditTool: partial read counts (old_string is the floor) - WriteFileTool overwrite: full read required - Either: new-file creation exempt (ENOENT → ok:true before requireFullRead is consulted); `fileReadCacheDisabled` escape hatch unchanged A dedicated `fresh + cacheable + partial + requireFullRead` rejection branch surfaces a clear "only been partially read … overwriting replaces the entire file" message instead of falling through to the generic "has not been read" wording. The `unknown` branch's wording also varies by `requireFullRead` so the read instruction matches the operation's actual requirement. For comparison, Claude Code's `readFileState` enforcement treats partial and full reads identically for both Edit and WriteFile. This PR is stricter on WriteFile (full read required) and identical on Edit (partial OK). Issue QwenLM#2499 is empirical evidence that the partial-read-then-overwrite case is real on at least some model populations, so the additional WriteFile constraint is justified. Single-commit shape (versus the earlier afc1b91 / 503fc0b split) to avoid an intermediate state in which Edit's relaxation has landed but WriteFile is still on the relaxed path: cherry-picks or bisect walks crossing such a boundary would re-introduce the issue QwenLM#2499 data-loss case. Tests: edit.test.ts ranged-read test inverted to "allows after ranged read"; write-file.test.ts ranged-read test asserts the new partial / full-required message. Three error-message regex matchers updated from /fully read/ to /read/. 198 / 198 prior-read-related tests pass; tsc --noEmit clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
PR #3774 introduced a
lastReadWasFullrequirement tocheckPriorRead: in addition to a fresh, cacheable cache entry, the most recent read had to have consumed the entire file (nooffset/limit/pages). That over-constrains the in-place edit case. For a multi-thousand-line file where the edit only touches a single line, the model is forced to re-read the whole file even when the bytes the edit touches are already in its context fromread_file(offset, limit).But — as a code review of the first revision of this PR pointed out —
EditandWriteFileare not symmetric:Editrequiresold_stringto match the file's current bytes; a fabricated string that misses what is actually on disk is caught by the0 occurrences foundfailure mode incalculateEdit. So a partial read is acceptable for in-place edits.WriteFileoverwrites the entire file. There is no equivalent content-derived guard. A model that has only seen a slice viaread_file(offset, limit)followed by aWriteFilewould necessarily hallucinate the rest of the bytes — which is issue Agent overwrites files using write_file without reading existing content, leading to data loss. #2499 verbatim (the data-loss scenario PR feat(core): enforce prior read before Edit / WriteFile mutates a file #3774 was opened to address).This PR splits the policy along that line.
Change
checkPriorReadgains arequireFullRead?: booleanoption (CheckPriorReadOptions). Whentrue, afresh + cacheable + partialcache entry is rejected; only a full read clears enforcement.EditToolrequireFullReadnot set (defaultfalse)WriteFileTooloverwriterequireFullRead: truefrom all 5 call sitesok: truebeforerequireFullReadis consulted)fileReadCacheDisabled === trueA new dedicated rejection branch handles
fresh + cacheable + partial + requireFullReadso the model gets a clear "only been partially read … overwriting replaces the entire file" message rather than the generic "has not been read". Theunknownbranch's wording also varies byrequireFullReadso the read instruction matches the operation's actual requirement.Comparison with Claude Code
Claude Code's
readFileStateenforcement (spec03_tools.md:484, 532) treats partial and full reads identically for both Edit and WriteFile. This PR is stricter on the WriteFile path (full read required) and identical on the Edit path (partial OK). The asymmetric stance reflects the asymmetric guards: Claude Code'sWritetool ships with the same risk surface, but issue #2499 in this repo is empirical evidence that the partial-read-then-overwrite hallucination case is real on at least some model populations, so the additional WriteFile constraint is justified here.Risk
(mtime, size)fingerprint risk PR feat(core): enforce prior read before Edit / WriteFile mutates a file #3774's "Known unmitigated" section flagged remains: a writer that produces an identical-length payload in the same mtime tick can passcache.check === fresh. Unchanged by this PR; mitigation deferred to a follow-up content-hash on the read pipeline.0 occurrencesfloor catches fabricatedold_strings, but a fabrication that coincidentally matches a region outside the partial-read window would still go through. The single-occurrence-or-replace_allrequirement narrows but does not close this. Operators who want strict full-read protection on Edit too can setfileReadCacheDisabled: true.Test plan
npx vitest run src/tools/edit.test.ts src/tools/write-file.test.ts src/tools/read-file.test.ts src/services/fileReadCache.test.ts— 198 / 198 passtsc --noEmit -p packages/core/tsconfig.json— cleanedit.test.ts:1022) inverted from "rejects" to "allows after ranged read" with passing assertion on resulting byteswrite-file.test.ts:858) restored to "rejects" with assertion on the new "partially read / replaces the entire file" message/fully read/to/read/Write→partial-Read→Editregression test description updated; behaviour unchangedNotes for reviewers
requireFullReadso a model reading the message gets the read instruction matching the operation it just attempted (full read for overwrite, partial-OK hint for edit).lastReadWasFullis still consulted byread-file.tsfor thefile_unchangedfast-path (decides whether a follow-up no-args Read can return afile_unchangedplaceholder). Unchanged and unrelated to enforcement.FileReadCache.recordRead's sticky-on-true behaviour for matching fingerprints remains in place; with this PR it serves thefile_unchangedfast-path and keeps the WriteFile-overwrite path passing once a full read has been recorded against the current bytes.Commits
afc1b917— first pass: droplastReadWasFullfor both Edit and WriteFile503fc0b0— review fix: re-introduce full-read requirement for WriteFile only, viarequireFullReadoption