Fix TOCTOU race condition in write-back#1
Open
rileycx wants to merge 2 commits intohunterassembly:mainfrom
Open
Fix TOCTOU race condition in write-back#1rileycx wants to merge 2 commits intohunterassembly:mainfrom
rileycx wants to merge 2 commits intohunterassembly:mainfrom
Conversation
When the file changed between the scan-phase read and the replace-phase re-read (e.g. dev server hot-reload, file watchers, a preceding write), the stored character indices became stale and the substring replacement could overwrite the entire file with just the replacement text. After re-reading the file, we now verify the stored span still exists at the expected index. If the file changed, we re-find the closest match in the current content before replacing — or return an error if the text is no longer present. Also moves normalizeReplacement to use the actual current span rather than the stale one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@rileycx is attempting to deploy a commit to the hunterassembly's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/write.ts (1)
563-613:⚠️ Potential issue | 🟡 MinorReturned line number can be stale after re-location
When
finalIndexchanges,lineshould be recomputed from current content. Returningbest.linecan point to the wrong location.Proposed fix
const before = source.substring(0, finalIndex); const after = source.substring(finalIndex + finalLength); const modified = before + replacement + after; + const finalLine = getLineNumber(source, finalIndex); await writeFile(best.file, modified, "utf-8"); const relativePath = relative(PROJECT_ROOT, best.file); return { success: true, file: relativePath, - line: best.line, + line: finalLine, matchCount: allMatches.length, scannedFileCount, usedFallbackScan, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/write.ts` around lines 563 - 613, The returned best.line can be stale if finalIndex was adjusted when re-finding the original span; recompute the line number from the current source before returning. After finalIndex is finalized (after the occurrences reduce block and before building the return object), compute a new line number e.g. const computedLine = source.substring(0, finalIndex).split(/\r?\n/).length (or equivalent newline-counting logic used elsewhere) and return that (computedLine) instead of best.line in the success response; update any references to line in the returned object to use computedLine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/write.ts`:
- Around line 555-606: The read-modify-write in this block (readFile(...),
computing finalIndex/finalLength, then writeFile(...)) can be clobbered by
concurrent writers; add a conditional write/locking step so the final write only
succeeds if the file version matches the one you read and otherwise retry: after
readFile(best.file) capture a version token (e.g., mtime+size from stat or a
hash of source), then before writeFile check the current version (stat or
re-hash) and if it differs, re-run the span re-find and normalizeReplacement
logic (using adapter.normalizeReplacement with currentSpan) and try again a few
times; alternatively add an adapter-level compare-and-swap API (e.g.,
adapter.compareAndSwap(filePath, expectedVersion, modified) or
adapter.acquireWriteLock(best.file) / release) and use that to guard the final
write; ensure you reference readFile, writeFile, adapter.normalizeReplacement,
finalIndex/finalLength and best.file when implementing the CAS/lock and add a
bounded retry loop to avoid livelock.
---
Outside diff comments:
In `@src/server/write.ts`:
- Around line 563-613: The returned best.line can be stale if finalIndex was
adjusted when re-finding the original span; recompute the line number from the
current source before returning. After finalIndex is finalized (after the
occurrences reduce block and before building the return object), compute a new
line number e.g. const computedLine = source.substring(0,
finalIndex).split(/\r?\n/).length (or equivalent newline-counting logic used
elsewhere) and return that (computedLine) instead of best.line in the success
response; update any references to line in the returned object to use
computedLine.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e2a4a3b-7463-48d4-b165-7d075a4eff9f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/server/__tests__/write.test.tssrc/server/write.ts
Addresses CodeRabbit review comments: - Wrap read-modify-write in a compare-and-swap retry loop (stat-based version check before and after) so concurrent writers can't clobber changes between re-read and writeFile - Recompute returned line number from current source when finalIndex shifts, instead of returning the stale best.line Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/server/write.ts (1)
614-627:⚠️ Potential issue | 🔴 CriticalCAS remains non-atomic; clobber window still exists.
Line 615–616 checks version, but Line 626 writes unconditionally afterward. A concurrent writer between those steps can still be overwritten.
Suggested fix (cooperative file lock around read-modify-write)
-import { readdir, readFile, writeFile, stat } from "node:fs/promises"; +import { readdir, readFile, writeFile, stat, open, unlink } from "node:fs/promises"; // ... for (let attempt = 0; attempt < MAX_CAS_RETRIES; attempt++) { + const lockPath = `${best.file}.hemingway.lock`; + let lockHandle: Awaited<ReturnType<typeof open>> | undefined; + try { + lockHandle = await open(lockPath, "wx"); + } catch { + continue; // someone else is writing; retry + } + + try { const fileStat = await stat(best.file); const versionBefore = `${fileStat.mtimeMs}:${fileStat.size}`; const source = await readFile(best.file, "utf-8"); // ... compute replacement ... const fileStatAfter = await stat(best.file); const versionAfter = `${fileStatAfter.mtimeMs}:${fileStatAfter.size}`; if (versionBefore !== versionAfter) continue; await writeFile(best.file, modified, "utf-8"); // return success... + } finally { + await lockHandle?.close(); + await unlink(lockPath).catch(() => {}); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/write.ts` around lines 614 - 627, The compare-and-swap is non-atomic because after computing fileStatAfter/versionAfter you still call writeFile(best.file, ...) unconditionally, allowing a race; change the sequence in the write loop (symbols: fileStatAfter, versionAfter, versionBefore, writeFile, MAX_CAS_RETRIES, attempt) to perform an atomic replace: write the new content to a temp file in the same directory, fsync the temp file and its parent directory, then atomically rename the temp into best.file only if the version check still passes (re-stat before rename and compare versionBefore/versionAfter), retry up to MAX_CAS_RETRIES on mismatch; alternatively implement a cooperative file lock around the read-modify-write to ensure no concurrent writer can clobber the update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/write.ts`:
- Around line 564-567: The stat/readFile calls around
fileStat/versionBefore/source (stat(best.file) and readFile(best.file)) can
throw and currently reject the promise; wrap these operations in a bounded retry
loop (e.g., 3 attempts with short delays) that retries on transient errors
(EAGAIN, EBUSY, ENOENT transient cases) and otherwise catch terminal fs errors
and return the function's expected structured failure object ({ success: false,
error }) instead of letting the exception escape; implement this around the
block using the existing function's return shape so any terminal error from
stat() or readFile() yields a consistent { success: false, error } response.
---
Duplicate comments:
In `@src/server/write.ts`:
- Around line 614-627: The compare-and-swap is non-atomic because after
computing fileStatAfter/versionAfter you still call writeFile(best.file, ...)
unconditionally, allowing a race; change the sequence in the write loop
(symbols: fileStatAfter, versionAfter, versionBefore, writeFile,
MAX_CAS_RETRIES, attempt) to perform an atomic replace: write the new content to
a temp file in the same directory, fsync the temp file and its parent directory,
then atomically rename the temp into best.file only if the version check still
passes (re-stat before rename and compare versionBefore/versionAfter), retry up
to MAX_CAS_RETRIES on mismatch; alternatively implement a cooperative file lock
around the read-modify-write to ensure no concurrent writer can clobber the
update.
ℹ️ Review info
Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cbd65247-c65a-4ec0-b94c-ce61b8cda573
📒 Files selected for processing (2)
src/server/__tests__/write.test.tssrc/server/write.ts
Comment on lines
+564
to
+567
| const fileStat = await stat(best.file); | ||
| const versionBefore = `${fileStat.mtimeMs}:${fileStat.size}`; | ||
| const source = await readFile(best.file, "utf-8"); | ||
|
|
There was a problem hiding this comment.
Unhandled fs errors can escape the API contract.
Line 564/566/626 can throw (ENOENT, EPERM, transient contention). Right now that rejects the promise instead of returning { success: false, error } like the rest of this function.
Suggested fix (bounded retry + structured failure on terminal fs errors)
for (let attempt = 0; attempt < MAX_CAS_RETRIES; attempt++) {
- const fileStat = await stat(best.file);
- const versionBefore = `${fileStat.mtimeMs}:${fileStat.size}`;
- const source = await readFile(best.file, "utf-8");
+ try {
+ const fileStat = await stat(best.file);
+ const versionBefore = `${fileStat.mtimeMs}:${fileStat.size}`;
+ const source = await readFile(best.file, "utf-8");
- // ... existing logic ...
+ // ... existing logic ...
- await writeFile(best.file, modified, "utf-8");
+ await writeFile(best.file, modified, "utf-8");
+ } catch (err) {
+ if (attempt < MAX_CAS_RETRIES - 1) continue;
+ return {
+ success: false,
+ scannedFileCount,
+ usedFallbackScan,
+ matchCount: allMatches.length,
+ error: `Write failed for ${relative(PROJECT_ROOT, best.file)}: ${err instanceof Error ? err.message : String(err)}`,
+ };
+ }
}Also applies to: 615-627
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/write.ts` around lines 564 - 567, The stat/readFile calls around
fileStat/versionBefore/source (stat(best.file) and readFile(best.file)) can
throw and currently reject the promise; wrap these operations in a bounded retry
loop (e.g., 3 attempts with short delays) that retries on transient errors
(EAGAIN, EBUSY, ENOENT transient cases) and otherwise catch terminal fs errors
and return the function's expected structured failure object ({ success: false,
error }) instead of letting the exception escape; implement this around the
block using the existing function's return shape so any terminal error from
stat() or readFile() yields a consistent { success: false, error } response.
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
1 participant
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.
Problem
The write-back feature has a TOCTOU (time-of-check-time-of-use) race condition that can destroy source files — replacing the entire file contents with just the replacement text.
How to reproduce
Hero.tsx) is now overwritten with justSee Work— all other content is goneRoot cause
In
writeText(), the scan phase reads all source files and stores character indices for text matches. Then, in a separate read, it re-reads the best-match file to perform the replacement. If the file changed between those two reads — which easily happens because the first write triggers a dev server hot-reload or file watcher — the stored indices are stale. The substring replacement then slices at the wrong positions and produces garbage output.Fix
After re-reading the file, we now verify the stored span still exists at the expected index. If it doesn't:
Also moves
normalizeReplacementto use the actual current span rather than the stale one from the scan phase.Single file change:
src/server/write.ts— no changes needed to overlay, index, or write-adapters since all write paths flow throughwriteText().Test plan
src/server/__tests__/write.test.tswith vitest (4 tests, all passing):🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores