fix(rewind): restore upstream TOCTOU ordering + heal sticky failed marker#4216
Merged
Conversation
…rker Two related bugs that landed in #4064: 1. **trackEdit widened the pre-write TOCTOU window**. PR #4064 inserted `await trackEdit(filePath)` between `checkPriorRead` and `writeTextFile` in both `edit.ts` and `write-file.ts`. trackEdit does `stat` + `copyFile` and on large files can take hundreds of milliseconds. The pre-existing comment above `checkPriorRead` acknowledged a stat-then-write race window of "two adjacent syscalls"; the new ordering widened that to "freshness check → potentially-multi-second backup → write", so an external mutation landing during the backup would no longer be detected before the write clobbered it. The fix is the ordering upstream `claude-code/src/tools/FileEditTool` uses, with its own explicit comment on the equivalent block: > These awaits must stay OUTSIDE the critical section below — a > yield between the staleness check and writeTextContent lets > concurrent edits interleave. Moved `trackEdit` to BEFORE `checkPriorRead` in both tools. Backups are idempotent (deterministic `{hash}@v{version}` filename), so running the backup on a path that ends up rejected just leaves an unused-but-correct backup of the pre-edit state, not corrupt state. 2. **The `failed` marker added in d598383 stayed sticky in `trackEdit`**. When `makeSnapshot` recorded `failed: true` for a file (transient I/O error during per-file backup), the next `trackEdit` for that file skipped because the early guard only checked existence, not the failed bit. The pre-edit state never got re-captured even after the underlying I/O recovered, leaving the snapshot marked filesFailed forever for that path. The guard now skips only on confirmed (non-failed) entries: `if (existing && !existing.failed) return;`. The re-check guard at the end is updated symmetrically so the fresh backup actually replaces the failed entry (instead of being silently dropped by the "already present" branch). Tests: - `edit.test.ts`: pin the TOCTOU ordering via spy on `fileReadCache.check` and `mockFileHistoryService.trackEdit`. Verifies `trackEdit` is called before the last `cache.check` (the pre-write freshness guard). Same pattern in `write-file.test.ts`. Both tests fail on the old ordering (verified by stashing the production change). - `fileHistoryService.test.ts`: `heals a failed entry on the next trackEdit attempt` — sabotage storage so makeSnapshot records failed, restore storage, run trackEdit, assert the entry is now non-failed with a real backupFileName.
Contributor
📋 Review SummaryThis PR fixes two related bugs introduced in #4064: (1) restores the correct TOCTOU ordering by moving 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes rewind file-history behavior by moving backups before final freshness checks and allowing failed backup markers to be retried/healed.
Changes:
- Moves
trackEditbefore pre-write TOCTOU checks in edit and write-file tools. - Updates file-history tracking to retry when the current snapshot entry is marked
failed. - Adds regression tests for ordering and failed-marker healing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/core/src/tools/write-file.ts |
Reorders file-history backup before final overwrite freshness check. |
packages/core/src/tools/write-file.test.ts |
Adds ordering regression test for write-file backups. |
packages/core/src/tools/edit.ts |
Reorders file-history backup before final edit freshness check. |
packages/core/src/tools/edit.test.ts |
Adds ordering regression test for edit backups. |
packages/core/src/services/fileHistoryService.ts |
Allows trackEdit to replace failed backup entries. |
packages/core/src/services/fileHistoryService.test.ts |
Adds regression coverage for healing failed backup entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+391
to
+392
| try { | ||
| await this.config.getFileHistoryService().trackEdit(file_path); |
Comment on lines
+500
to
+503
| try { | ||
| await this.config | ||
| .getFileHistoryService() | ||
| .trackEdit(this.params.file_path); |
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. |
…eal test
Address review feedback from code-reviewer agent:
- The TOCTOU ordering tests previously asserted ordering via
`callOrder.lastIndexOf('cache.check')`, which would silently degrade
to a tautology if a future refactor added any `cache.check` call
AFTER the pre-write check (e.g. the deferred atomic-write + content-
hash post-check explicitly mentioned in the comment above the
freshness guard).
Switch to a behavioral assertion that directly tests the invariant:
install a `trackEdit` mock that mutates the file on disk (bumps
mtime) before returning. The pre-write `checkPriorRead` must catch
the in-trackEdit mutation — that only happens if `trackEdit` runs
BEFORE the pre-write check. The broken ordering would run pre-write
check first (passing on pre-mutation stats), then trackEdit (which
mutates), then write (which clobbers the external mutation).
Asserting on `result.error?.type === FILE_CHANGED_SINCE_READ`
exercises the actual race protection the fix exists to preserve,
and survives any future refactor that keeps the invariant intact
regardless of how many `cache.check` calls happen on the way.
Verified by reverting just the production change to HEAD~1: both
behavioral tests fail under the broken ordering (file is silently
overwritten, no FILE_CHANGED_SINCE_READ error), and pass under the
fix.
- The heal test (`heals a failed entry on the next trackEdit attempt`)
previously asserted `backupFileName !== null` but not that the file
on disk at that name actually contained the current content. Add a
`readFile + expect(...).toBe('p2-content')` assertion so a regression
that wrongly reuses `previous.backupFileName` (pointing to
`p1-content`) fails loudly instead of silently passing.
wenshao
reviewed
May 16, 2026
| @@ -476,6 +476,35 @@ class EditToolInvocation implements ToolInvocation<EditToolParams, ToolResult> { | |||
| } | |||
|
|
|||
| try { | |||
Collaborator
There was a problem hiding this comment.
[Suggestion] The trackEdit backup block (lines 478-501) is nearly identical to the one in write-file.ts (lines 371-395) — ~23 lines each with the same upstream design rationale comment. The only differences are the file-path variable (this.params.file_path vs file_path) and the last sentence of the comment.
This duplication is a maintenance risk: the sort ordering constraint already regressed once (in #4064), and any future update to the design rationale or the trackEdit signature must be applied in two places.
Consider extracting a shared helper (e.g. trackEditBestEffort(path)) on FileHistoryService or a utility that encapsulates the try/catch and the "best-effort; never block core tool operations" semantics. The authoritative upstream comment can live on that helper as a single source of truth.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
本地真实场景测试报告（tmux）Head: 修复 #1：TOCTOU ordering原 #4064 在 新的 ordering（
修复 #2：sticky
|
| 测试 | 关键 |
|---|---|
fileHistoryService.test.ts heals a failed entry on the next trackEdit attempt |
制造 makeSnapshot failure 后 trackEdit 必须 retry；额外读 backup 文件内容确认是 p2-content 而非 p1-content，防 reuse previous.backupFileName 回归 |
edit.test.ts backs up before the pre-write freshness check (TOCTOU ordering) |
trackEdit mock 内 bump mtime → pre-write check 必须捕获并 reject。behavioral 断言（result.error.type === FILE_CHANGED_SINCE_READ），不是 call-order proxy |
write-file.test.ts 同上 |
镜像版 |
tmux 真实端到端
1. 工具改 foo.txt "initial content" → "CHANGED VIA TOOL"
2. backup 真实生成在 ~/.qwen/file-history/<session>/67c68ab663359af7@v1
3. /rewind → 选 turn → "Restore code and conversation (+1 -1 in 1 file)"
4. cat foo.txt → "initial content" ← 文件确实恢复了
证明 trackEdit 在 write 之前抓到了 pre-edit 内容，TOCTOU 修复在真实数据通路上 work。
测试 & CI
| 项 | 结果 |
|---|---|
tsc --noEmit -p packages/core |
0 错 |
| Targeted tests | 153/153 pass (fileHistoryService 29 + edit 77 + write-file 47) |
| GitHub CI | ✅ 全绿（Lint, Test ubuntu/macos/windows, CodeQL, Coverage） |
备注
两个 bug 都是我前面 approve PR #4064 时漏掉的 follow-up：TOCTOU 我当时没意识到 trackEdit 撑大了 race window；sticky failed 我在 makeSnapshot 路径修了但 trackEdit 路径漏了（PR #4064 commit ac07f63e 只动了 makeSnapshot 的 no-change fast-path 守卫，trackEdit 的对应守卫继续 sticky）。这次修得彻底，commit message 引用 upstream claude-code 的"these awaits must stay OUTSIDE the critical section"原文作为设计意图依据。
LGTM ✅
wenshao
approved these changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
已发布详细测试报告（见 issue comment）。两个 #4064 follow-up bug 都修了（TOCTOU ordering + sticky failed marker heal），3 条 behavioral 单测加强，tmux 真测 /rewind 全流程能把 foo.txt 从 'CHANGED VIA TOOL' 恢复回 'initial content'；153/153 单测过，tsc 0 错，CI 全绿。LGTM ✅
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.
Two related bugs that landed in #4064.
Summary
edit.tsandwrite-file.tswas widened: PR feat(rewind): add file restoration support to /rewind command #4064 insertedawait trackEdit(...)between the pre-writecheckPriorReadand the actualwriteTextFile. Move it to BEFOREcheckPriorReadto restore the upstream-aligned ordering. This is the headline fix — details below.failedmarker intrackEditwas sticky: the marker added in d598383 to surface per-file backup failures was not cleared whentrackEditran again. Allow the nexttrackEditto overwrite a failed entry so the heal path actually records a fresh backup.Why #1 (TOCTOU) is done this way
This part of the PR is intentionally a literal port of upstream
claude-code's ordering, because the upstream maintainers wrote out the exact constraint we lost.The relevant upstream file is
src/tools/FileEditTool/FileEditTool.ts. It contains this block, with a comment that is the design intent for this section:Two upstream invariants:
trackEditruns BEFORE the staleness check (the cited comment about "awaits outside the critical section").awaitbetween the staleness check and the write (the "preserve atomicity" comment).When we ported the rewind feature in #4064 we kept the staleness check (
checkPriorRead) and the write (writeTextFile), but we inserted the newawait trackEdit(...)between them. BecausetrackEditdoes astat+copyFile, on large files it takes hundreds of milliseconds to multiple seconds. The pre-existing comment abovecheckPriorReadalready acknowledged a stat-then-write race window of "two adjacent syscalls"; the new ordering silently grew that window into "freshness check → potentially-multi-second backup → write", so an external mutation landing inside the backup window would no longer be detected before the write clobbered it.The fix is the same shape upstream uses: move
trackEditto beforecheckPriorRead. Backups are idempotent (deterministic{hash}@v{version}filename keyed off path + version), so doing the backup on a path the freshness check later rejects just leaves an unused-but-correct backup of the pre-edit state — not corrupt state. The nextmakeSnapshotwill reuse it if the file is unchanged. The inline comments on bothedit.tsandwrite-file.tsquote the upstream comment verbatim so the design rationale doesn't get re-lost the next time someone reaches for that section.This is purely about restoring the upstream-designed ordering. It does not couple to any of the other open follow-ups (#4173, #4187, #4204).
#2 (failed marker heal) — short version
makeSnapshotrecords{ failed: true, backupFileName: previous?.backupFileName, ... }when the per-file backup throws (added in d598383, fixes silent-data-loss). ButtrackEdit's early guard only checked entry existence, not the failed bit:Once an entry is marked
failed, the nexttrackEditfor that file — typically about to be triggered by a tool — would skip without attempting a new backup. The pre-edit state never got captured even after the underlying I/O recovered, so the snapshot kept reportingfilesFaileduntil the file content drifted.Now:
The re-check guard at the end is updated symmetrically so the fresh backup actually replaces the failed entry instead of being silently dropped by the "already present" branch.
Test plan
packages/core/src/tools/edit.test.ts— newbacks up before the pre-write freshness check (TOCTOU ordering): pass-through spy onfileReadCache.checkandmockFileHistoryService.trackEditrecords each invocation into a shared array; assertstrackEditis called BEFORE the lastcache.check(the pre-write freshness guard). Same shape inpackages/core/src/tools/write-file.test.ts. Both tests fail on the old ordering (verified by stashing the production change and re-running).packages/core/src/services/fileHistoryService.test.ts— newheals a failed entry on the next trackEdit attempt: sabotage storage somakeSnapshotrecordsfailed: true, restore storage, runtrackEditagain, assert the entry is now non-failed with a realbackupFileName !== null.🤖 Generated with Qwen Code