[easonLiangWorldedtech]

This PR routes the remaining write tools - apply_diff, write_to_file, edit, edit_file, search_replace - through the same guard, so after this unit no tool path can clobber a file the model never read: an unearned write fails with the re-read remediation instead of overwriting. A write outside every workspace root is no longer an outright rejection: the tool layer classifies against all workspace folders, asks the user, and forwards the approval to the guard, which re-checks containment against the approved roots before publishing.

It is the seventh of nine PRs for epic #1375 (chain tracked on #1991).

Related GitHub Issue

Tracking: issue 2022 (this unit's tracking issue; deliberately not a closing reference - the maintainer closes it when the series lands).
Refs: #1991 (chain tracking), #1375 (epic).

Description

What this PR changes:

1. apply_diff, write_to_file, edit, edit_file and search_replace all save through DiffViewProvider.saveDirectly() / guardedWrite() with an explicit write kind, so a targeted edit is never treated as a full-file replacement.
2. The tools' own reads (the diff hunk read, the patch hunk read) bracket the read with a bigint fs.stat pair and observe the file only when both tokens match, so the publish is authorized by the exact bytes the tool computed its hunks from; a stat failure leaves the file unobserved and never fails the read.
3. Approved outside-workspace writes are bound to the canonical identity captured BEFORE the approval was asked (approvedCanonicalTarget); the guard only compares and refuses when the capture is missing.
4. Review-round fixes on this unit: an unresolvable workspace root no longer falls back to the lexical-only containment decision, a path that runs through a dangling symlink ancestor is refused instead of re-joined lexically, and the stat-failure branches are covered by tool-level tests.

Why the diff looks bigger than the change: the chain is non-cumulative - this branch is based on main and carries its own copy of the earlier units' content, which the GitHub diff shows as context. This unit's own delta is 539 a+d / 54 changed executable lines.

Review focus: the tool classes named above, src/core/tools/guardedWrite.ts (approval binding), and the specs named in the Test Procedure.

Test Procedure

From the repository root, with dependencies installed:

pnpm --dir src test -- core/tools/__tests__/guardedWrite.spec.ts core/tools/__tests__/applyPatchTool.execute.spec.ts core/tools/__tests__/applyPatchTool.partial.spec.ts core/tools/__tests__/applyDiffTool.guardedWrite.spec.ts core/tools/__tests__/writeToFileTool.spec.ts core/tools/__tests__/editTool.spec.ts core/tools/__tests__/searchAndReplaceTool.spec.ts

Expected: 138 passed, 5 skipped. Then pnpm --dir src test -- integrations/editor/__tests__/DiffViewProvider.spec.ts utils/__tests__/safeWriteJson.test.ts utils/__tests__/safeWriteJson.lockKey.spec.ts - 166 passed, 4 skipped.
Environment: Windows 11, Node 20, unit layer only.

Pre-Submission Checklist

• Scope is one gate scope and matches the unit.
• No .changeset or CHANGELOG changes (AGENTS.md).
• src/eslint-suppressions.json unchanged - no suppression count increased.
• New code lints clean (--max-warnings=0) rather than relying on suppressions.
• Tests added at the lowest layer that would have caught each finding.
• Visual Snapshot item considered: not applicable (no rendered surface).

Visual Snapshots

Not applicable - no rendered surface; the approval prompt is the existing one.

Videos (interaction / animation only)

Not applicable.

Documentation Updates

No user-facing documentation change: the guard is internal behaviour of the write tools, and the model-facing remediation text already existed in earlier units. No .changeset and no CHANGELOG edit (AGENTS.md).

Additional Notes

• Budget: own delta 539 a+d / 54 changed executable lines - inside both caps. The mutation gate reports the GitHub diff's changed-line count (1468 against the 500 cap at head 7f90fc299, 1402 at the previous head) because the unlanded predecessors inflate it; the row clears on rebase after the predecessors merge.
• Negative controls for the approval binding: comparison dropped -> 2 failed; baseline from the post-approval lookup -> 1 failed; missing capture accepted -> 1 failed.
• Content source of record: the superseded umbrella PR feat(tools): track read completeness and guard the diff-view save path (S4b, #1375) #1833 (closed), replayed onto the current main tip. Split plan of record: comments 5993969784 / 5994039786 / 5994053776 on [Tracking] guarded-write chain (epic 1375): rules, port sources, chain-level rows #1991.
• Merge order for the chain: U1 -> U2 -> U3 -> U4 -> U5 -> U8 -> U6 -> U7 -> U9. This unit's merge-order predecessor is U6 (feat(tools): publish apply_patch through the guard (U6, #1375) #1915).

Verification at this head

Verification at this head (7f90fc2)

• Unit specs: applyPatchTool.execute.spec.ts 27 passed (27); DiffViewProvider.spec.ts 158 passed (158); safeWriteText.spec.ts 68 passed (68) - also 68 passed (68) in a worker whose process.platform reports linux; safeWriteJson.test.ts 27 passed / 4 skipped; safeWriteJson.stagingDirectory.spec.ts 7 passed (7).
• Whole src lane measured at 52628eb86: 12859 passed | 45 failed | 41 skipped (the follow-up commit adds one passing case to that total). The same two files fail on the pre-change baseline run of this same worktree - __tests__/dist_assets.spec.ts (40, needs the bundle the local lane skips) and services/rules/__tests__/rules.spec.ts (5, symlink cases on Windows) - so nothing that passed before fails now. Honesty note: local vitest lanes skip the pretest bundling step, so bundle-dependent specs are CI-only.
• ESLint --prune-suppressions --max-warnings=0 clean on all seven files; src/eslint-suppressions.json left byte-identical to the merge commit, because the prune only reformatted it and it was restored from the index. Against the merge base the total is 3301 -> 3299: one reduction (core/tools/__tests__/readFileTool.spec.ts), and none of the files touched this round carries an entry.
• pnpm check-types (root tsc -p tsconfig.json --noEmit plus 11 turbo tasks) exit 0 on a cold cache: 11 cache miss, 0 cached. The pre-push hook re-ran it before this head was pushed.
• Junction pre-flight for split-1833/u7: 262 reparse points under the node_modules roots - 225 pnpm-store links into this worktree, 37 workspace links, 0 links pointing into the main checkout; packages/types/dist and the turbo bin present.
• CI at 7f90fc299, read from the job log (run 41970071166, job 127056022960): the gate derives BASE_SHA as HEAD_SHA^1 of the PR merge commit and fails with Mutation gate failed: extension has 1468 changed executable lines (limit 500) - the same chain budget-cap failure as at the previous head, which reported 1402, so chain paperwork rather than a new defect. The gate's own logic subtests passed in that run and Stryker never started, so there is no reports/mutation/ artifact and no directive was added. compile, e2e-mock, extension-host-visual, webview-visual and both platform-unit-test lanes were still running when this line was written; CodeRabbit's own check is neutral (advisory).

Updated this round

• Registered the two open walkthrough rows at this head (no code change this round; the carried copies are unchanged):
  • TOCTOU row (verifyTarget re-resolves before the publish): registered confinement primitive - see record 6106959976 on feat(tools): wire guarded writes into the diff-view save paths (S4b, #1375) #1408. The carried copy already re-checks the target under the lock immediately before the publish; the full directory-identity pinning is U5's (feat(tools): guarded write core under the shared lock (U5, #1375) #1914) content and merges before this unit in the declared order.
  • Lifecycle row (duplicate teardown after cancellation): carried-copy drift, not a U7 defect. The carried DiffViewProvider/guardedWrite copies predate U8's (feat(editor): route the diff-view save through the guard (U8, #1375) #1916) cancellation ordering and landed-token reconciliation; the rows resolve at merge time because U5 and U8 merge before U7 in the declared order and the diff shrinks to U7's own wiring.
• Next refresh round: a content-union pass over the carried copies (this unit's approval flow + U8's cancellation ordering and landed reconciliation + U5's pinning) is scheduled after the current re-review wave, so the guard is not shipped half-merged under review pressure.

Get in Touch

Reviewer and maintainer questions are welcome as comments on this PR or on #1991.

Stryker directive removal (equivalence notes)

Per AGENTS.md the two Stryker suppression comments in the guarded save
path were removed instead of kept; their equivalence claims:

• writeKind default StringLiteral ("update" -> ""): an empty kind
  dispatches exactly like "update" inside guardedWrite - only "edit"
  and "create" branch distinctly - and the default-parameter line is
  not part of this unit's changed-line selector, so the mutant is
  equivalent / out of scope.
• saveTask operand of the adoption-probe guard (LogicalOperator):
  GuardRejectedError is only thrown by guardedWrite, which is reached
  only after the !saveTask check above has already thrown. Whenever
  the operand is evaluated saveTask is provably non-null, so dropping
  the operand cannot change which branch is taken.

mutation-diff remains advisory for stacked units; maintainers decide any
narrow gate-config exclusion.

Re-review round (41c0295)

• Preview-observation withdrawal on every exit path (Major): open()
  records the token its preview wrote; reset() (reached by every tool catch
  and by revertChanges via finalizeSession) forgets the entry while it
  still holds that token and the preview's incomplete flag - publishes and
  model reads survive. Trigger-sequence test + two survival tests; three
  negative controls, each exactly one red.
• Exact guard verdict in the lock-cleanup test (^Stale version -- the file changed) + deduplicated repeated assertion.

Ownership-based withdrawal port (14f8634)

The sibling unit's follow-up thread found the content-based withdrawal
could delete a model partial read recorded while the diff was open
(same token, same incomplete flag). The copy this unit owns carried the
identical flaw and is fixed identically: open() keeps the exact entry
object its observe() wrote; reset() forgets only while the registry
still stores that object. Regression test ported; NCs each exactly one
red. 153/153 green.

Open document reconciled after the guarded publish (f5fddf6)

The Persistence Integrity row: saveDirectly() commits through
guardedWrite(), which writes the bytes to disk directly, and the diff
removed the awaited document.save() that used to follow the publish -
leaving an already-open target document holding its pre-publish buffer.
A dirty buffer there is a live hazard: VS Code's ordinary save path
would republish those stale bytes over what the guard committed.
saveDirectly now reconciles the open document before returning -
reverting reloads the published bytes and clears the dirty flag - so no
open buffer is left whose text differs from the publish; reverting a
document that already matches the disk is a no-op, and the memory-only
path still never saves the buffer over the publish. Regression test: an
already-open dirty target document is reverted (not saved) by the time
saveDirectly resolves; negative control: removing the reconcile
reddens exactly that test. 154/154 green (240/240 across the guarded
write and tool suites) at this head.

Fail-safe reconcile (c198c2e)

The 04:29Z Persistence Integrity row implemented in full: saveDirectly() requires successful reconciliation after the guarded publish. If revertDocument() returns false or throws, the tab is closed and the document reopened from disk; if the reopened buffer is still dirty the write fails instead of reporting success, and the diagnostics/open path does not run while the target is dirty. Test: revert command rejects and buffer stays dirty -> saveDirectly() rejects, tab closed and reopened from disk (a later save reads the published bytes), stale buffer's own save never runs. Two negative controls (bare revert; tolerate dirty reopen) each redden exactly that test. 155/155 green.

The Security Boundaries TOCTOU row remains the registered check-then-resolve family (lands once at the publish-primitive owner; precedent record 6106959976, issue #1408).

Staging-directory drift guard fix (64dc74c)

platform-unit-test (ubuntu-latest) was red at 40ac71f on exactly one case: core/task/__tests__/Task.spec.ts > 'finalizePartialToolAsk still updates the webview when a later save stage fails' (expected "updateClineMessage" to be called with arguments, Number of calls: 0). Measured with a probe on every realpath call in that window that printed the path it received and its outcome, plus fs.statSync of the same path at the same instant through the never-mocked fs: not one realpath returned ENOENT for the directory - the guard's first walk and both lock-key canonicalizations resolved it, four times, to inode 22517998136858990, and the only ENOENTs were on ui_messages.json itself, the target file that genuinely is not there. The guard's own await fs.stat(approvedCanonical) reported ENOENT for a directory the unmocked fs reported as that same inode, because Task.spec.ts:144 mocks fs/promises with stat: vi.fn().mockRejectedValue({ code: "ENOENT" }) for every path, and it rejected before the second walk, so the drift comparison never happened. The guard was failing on a question it invented, and the only reason our own lane never caught it is that the suite driving this write path mocks stat to fail for every path.

The guard now acts on a single signal: the re-resolution of the approved directory succeeds and lands on a different directory. A probe that cannot answer is not an answer - ENOENT because a component is mid-replacement or because the caller's fs reports the path as missing, EACCES, EPERM, ELOOP - so the write proceeds; the existence probe is gone, and a directory that really is deleted now fails at the staging write itself, still fail-closed with nothing created outside the directory the write was approved for. Confinement keeps the opposite rule on purpose: there the guard has to prove containment, and a probe that cannot answer cannot prove anything. Controls: reverting the fix reddens exactly the two new cannot-answer cases - one of them a unit-scale twin of the CI failure that also asserts the fixed path makes zero fs.stat calls - and reddens the CI case in Task.spec.ts and only it (1 failed | 198 passed); restoring it is green (5 files, 306 passed | 4 skipped) and byte-identical by SHA256. Cross-checked against a scratch merge of this branch's head with upstream/main 6bbc7a0: the CI case is red there without this commit and green with it.

Known property, deliberately not fixed in this PR: if the approved directory is replaced before the guard's first reading, both readings agree and no layer rejects the write - measured byte-for-byte identical before and after this change (the payload publishes at wherever the approved pathname now points, with no staged file left in either spelling and only the advisory lock directory travelling with the renamed directory). The consequence is that the file can be published into a directory the writer's advisory lock does not name. Closing that means safeWriteText binding the publish destination to the directory its own advisory lock names, which is a design change in the publish path that #1916's guardedWrite layer builds on, so it is registered as a follow-up unit rather than folded into a green-lane fix. It is recorded by safeWriteJson.stagingDirectory.spec.ts > 'follows the approved pathname when the directory is replaced before the guard reads it', which asserts readings === 3 so a change in the canonicalization order fails loudly.

Review round r112 fixes (52628eb)

Head 52628eb86. The merge base is now main at e943530c8 (merged in 002b34fa4), so the GitHub diff at this head is 33 files / 12474 insertions / 349 deletions against that base. This unit's own delta is 539 a+d / 54 changed executable lines; the mutation gate charges the whole branch delta against main (it derives BASE_SHA as HEAD_SHA^1 of the PR merge commit), so the chain copies this branch carries sit inside its number, and refreshing onto main did not shrink it.

Five live threads, each fixed and answered in-thread with its control numbers:

1. src/core/tools/ApplyPatchTool.ts:116 (Major, rt4475729779): execute records the exact registry entry behind every observation it creates and forgets it only while the registry still holds that entry; every publish site drops its recorded entry first, so a published file is never withdrawn. Reverted to blob a36944e59b6b: 4 failed | 22 passed; at this head 26 passed (26).
2. src/integrations/editor/DiffViewProvider.ts:765 (Minor, rt4475729791): 765, 962 and 989 now close only this provider diff view via closeOwnDiffView(absolutePath). closeAllDiffViews() stays as the path-unknown fallback inside reset() (1703) and a test pins the fallback itself. Reverted to blob 8afc716b0946: 8 failed | 150 passed; at this head 158 passed (158).
3. src/services/file-safety/safeWriteText.ts:358 (Minor, rt4475668838): dirPath is created with the plain recursive fs.mkdir before _stagingDir(dirPath), so the private 0o700 applies to the staging directory alone and the two writers agree (safeWriteJson.ts:219 already did this). Reverted to blob cb24e0e3172c: 1 failed | 67 passed on the creation-order assertion; at this head 68 passed (68). Windows cannot expose the mode difference itself (measured 666 / 666 in both orders), so the test asserts creation order plus the mode option each call carries.
4. src/utils/__tests__/safeWriteJson.test.ts:646 (Minor, rt4475729798): vi.doUnmock replaces the hoisted vi.unmock, the comment says what actually undoes the capture, and a new case asserts a later import resolves the real proper-lockfile. Rewritten back to vi.unmock: 1 failed | 26 passed | 4 skipped; at this head 27 passed | 4 skipped.
5. src/services/file-safety/__tests__/safeWriteText.spec.ts:692 (Trivial, rt4475668810): the platform skip is gone, and a second case of the same class in the same file lost its skip too; the file runs 68/68 on Windows and 68/68 in a worker reporting linux.

Chain overlap, recorded in-thread: the sites this unit shares with #1916 are the ApplyPatchTool.readFile partial observation and its publish sites, plus the DiffViewProvider teardown; the follow-up that binds a publish destination to the locked directory stays on #1916.

Gates for this round: husky hooks on for both commit and push (lint-staged eslint + prettier, pre-push check-types), never --no-verify; push used --force-with-lease pinned to the freshly read fork head 64dc74cab, read back as 52628eb86. No .changeset, no CHANGELOG edit, no suppression increase, no Stryker directive.

Follow-up inside round r112 (7f90fc299)

CodeRabbit verification reply on rt4475729779 found one non-publishing exit the first cut left open: a move publishes the destination, then a failed fs.unlink of the source is logged and tolerated, so the source stays on disk with its original bytes while its tracking entry had already been dropped - the tool-owned partial observation survived as authority for a later edit. The source entry now leaves the tracking map only when that unlink succeeded (sourceRemoved gates it); the destination entry still leaves unconditionally, because the destination really did publish.

Control: the new case move: withdraws the source observation when its unlink fails after the destination published fails at 52628eb86 (1 failed | 26 passed (27), expected true to be false on the leftover registry entry, and a follow-up guardedWrite(..., "update") to the source rode on it instead of refusing) and passes at 7f90fc299 (27 passed (27)). Reverting only the implementation to blob b78a7ad6ad55 reddens exactly that case. Fixed blobs at this head: implementation 74a22bf7644f, spec ff52ead16c31. GitHub exposes no reopen mutation, so the answer was posted as a reply inside the already-resolved thread.

Charged diff at this head against the merge base e943530c8: 33 files, 12510 insertions / 350 deletions. Suppressions remain byte-identical to the merge commit (--prune-suppressions only reformats it, and it is restored from the index each round).

---

Closes: #2426 (this unit's tracking issue, U7 of the #1375 split, filed 2026-11-11; the note carries this unit's head, its measured price with the base named and the gate state, and the plan post-dates this pull.)