Skip to content

feat(tools): route the remaining write tools through the guard (U7, #1375) - #1918

Open
easonLiangWorldedtech wants to merge 30 commits into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:fws/u7-tool-wiring
Open

easonLiangWorldedtech wants to merge 30 commits into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:fws/u7-tool-wiring

Conversation

@easonLiangWorldedtech

@easonLiangWorldedtech easonLiangWorldedtech commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Split unit U7 of #1833, under the plan on this issue (5993969784 / 5994039786 / 5994053776). Base is U5 (#1915) per the merge order.

Scope (one gate scope): the remaining write tools — apply_diff, write_to_file, edit, edit_file, search_replace publish through the same guard, so a write that was not earned by a read fails with the standard remediation instead of overwriting.

Content source of record: kind: commit, base 7c291bb08 → head 6768ccfaf, replayed on the current main tip 9af61f87e so this branch carries nothing that main already has.

Budget (own delta, not the stacked view): 539 a+d / 54 changed executable lines. Inside both caps.

Verification at this head: 108 passed, 5 skipped across the five specs; ESLint --max-warnings=0 clean on every file in the unit; Prettier clean; src/eslint-suppressions.json never increased.

The GitHub view also carries the unmerged base, so the numbers above are this unit's own delta.

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 23 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: Zoo-Code-Org/Zoo-Code/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 33a109ff-cf3d-4619-969b-1dff696aca2e
📥 Commits

Reviewing files that changed from the base of the PR and between 91c7d75 and bb9ad00.

📒 Files selected for processing (14)
  • src/core/task/observationRegistry.ts
  • src/core/tools/ApplyDiffTool.ts
  • src/core/tools/ApplyPatchTool.ts
  • src/core/tools/__tests__/applyDiffTool.guardedWrite.spec.ts
  • src/core/tools/__tests__/applyPatchTool.execute.spec.ts
  • src/core/tools/__tests__/guardedWrite.spec.ts
  • src/core/tools/guardedWrite.ts
  • src/integrations/editor/DiffViewProvider.ts
  • src/integrations/editor/__tests__/DiffViewProvider.spec.ts
  • src/services/file-safety/__tests__/safeWriteText.spec.ts
  • src/services/file-safety/safeWriteText.ts
  • src/utils/__tests__/safeWriteJson.lockKey.spec.ts
  • src/utils/__tests__/safeWriteJson.test.ts
  • src/utils/safeWriteJson.ts
📝 Summary

Summary by CodeRabbit

  • Improvements
    • File edits and saves now check for conflicting changes, helping prevent newer on-disk content from being overwritten.
    • File publishing is more resilient, with atomic saves and recovery attempts if a save fails.
    • File reads distinguish clipped content from omitted lines, with notices when either occurs.
  • Bug Fixes
    • Partial or outdated reads are prevented from authorizing certain file replacements.
    • Failed or rejected saves no longer appear as successful edits.

Walkthrough

The pull request adds per-task file-version observations and guarded writes for file tools. It also adds an atomic text publisher and updates safeWriteJson to use it. Read processing now tracks completeness and clipped content.

Changes

Observed and Guarded File Writes

Layer / File(s) Summary
Record file observations and read completeness
src/core/task/Task.ts, src/core/task/observationRegistry.ts, src/core/task/__tests__/observationRegistry.spec.ts, src/core/tools/ReadFileTool.ts, src/core/tools/__tests__/readFileTool.spec.ts, src/integrations/misc/indentation-reader.ts, src/integrations/misc/__tests__/indentation-reader.spec.ts, src/eslint-suppressions.json
Each task owns an observation registry. Stable reads record a version and a completeness value. Read results report clipped content separately from omitted lines. The request loop also pushes an item when the task is paused, even when user content is empty.
Validate and serialize guarded writes
src/core/tools/guardedWrite.ts, src/core/tools/__tests__/guardedWrite.spec.ts
Guarded writes check file observations and version tokens, serialize writes per path, use shared file locks, check cancellation, and refresh observations after publication.
Publish through the diff provider
src/integrations/editor/DiffViewProvider.ts
Diff saves use guarded publication. Preview reads can record observations, and cleanup tracks placeholders, serializes teardown, and closes only the provider’s matching diff view.
Apply guard modes in file tools
src/core/tools/ApplyDiffTool.ts, src/core/tools/ApplyPatchTool.ts, src/core/tools/EditFileTool.ts, src/core/tools/EditTool.ts, src/core/tools/SearchReplaceTool.ts, src/core/tools/WriteToFileTool.ts, src/core/tools/__tests__/*
File tools pass create or edit modes to saves. Patch moves pass source completeness to destination creation. Tests cover successful saves, rejected guards, and error handling.

Atomic Text and JSON Publishing

Layer / File(s) Summary
Stage and atomically publish text
src/services/file-safety/safeWriteText.ts, src/services/file-safety/__tests__/safeWriteText.spec.ts
safeWriteText stages and syncs content before publishing. It handles symlinks, target modes, backups, rollback, and platform-specific durability operations.
Use the text publisher for JSON writes
src/utils/safeWriteJson.ts, src/utils/__tests__/safeWriteJson*.spec.ts
safeWriteJson resolves its lock and publish targets, stages JSON beside the target, and delegates publication and backup recovery to safeWriteText.

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant ReadFileTool
  participant ObservationRegistry
  participant FileTool
  participant DiffViewProvider
  participant guardedWrite
  participant FileSystem
  ReadFileTool->>ObservationRegistry: Record stable version and completeness
  FileTool->>DiffViewProvider: Save content with create or edit kind
  DiffViewProvider->>guardedWrite: Publish content for the task
  guardedWrite->>FileSystem: Check target and version under lock
  FileSystem-->>guardedWrite: Return target state
  guardedWrite->>FileSystem: Publish when guard passes
  guardedWrite->>ObservationRegistry: Refresh observation after publication
Loading
🚥 Pre-merge checks | ✅ 6 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Regression Evidence ⚠️ Warning The changed request-loop branch lacks focused coverage. Task.recursivelyMakeClineRequests now pushes a stack item with empty content when isPaused is true (src/core/task/Task.ts:4696-4705). No t… Add a focused Task.recursivelyMakeClineRequests unit test that sets isPaused with empty userMessageContent and verifies the intended loop behavior. Add the unpaused/empty-content control case to protect the condition’s other branch.
Lifecycle Resource Cleanup ⚠️ Warning The new closeOwnDiffView() cleanup can close a diff owned by another provider. openDiffEditor() reuses an existing diff when its modified path matches (DiffViewProvider.ts:1231-1242). The cleanup … Give each provider's diff a distinct identity and close only that provider's view, or track all providers that share a diff and dispose their listeners and cancel their timers before closing it. Add a same-path, two-provider test that verif…
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: routing the remaining write tools through the guard.
Description check ✅ Passed The description explains the scope, implementation intent, linked issue context, and verification results. It omits some template sections and the checklist, but provides the key information reviewers…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Boundaries ✅ Passed No changed path bypasses approval or access controls. The write tools still validate .rooignore access and protected-file status before publishing; WriteToFileTool also calls askApproval before …
Persistence Integrity ✅ Passed No qualifying persistence-integrity failure was introduced. The new safeWriteText stages content, fsyncs it, and publishes with an atomic rename. With backups enabled, it attempts rollback on pre-co…
Full details: Regression Evidence

Explanation

The changed request-loop branch lacks focused coverage. Task.recursivelyMakeClineRequests now pushes a stack item with empty content when isPaused is true (src/core/task/Task.ts:4696-4705). No task test sets isPaused or exercises this paused-and-empty-content case. Existing Task.spec.ts request-loop tests do not cover it, and ask-queued-message-drain.spec.ts tests ask() behavior rather than the request stack. The guarded-write paths and their rejection cases have focused tests; this finding is limited to the Task loop change.

Full details: Lifecycle Resource Cleanup

Explanation

The new closeOwnDiffView() cleanup can close a diff owned by another provider. openDiffEditor() reuses an existing diff when its modified path matches (DiffViewProvider.ts:1231-1242). The cleanup selects tabs by target path, not provider identity (914-942), and reset() calls it (1451-1467). If two task providers edit the same file, one provider's rejected-save cleanup or reset can close their shared diff while only disposing its own listeners and timer (972-989). The other provider retains listeners and its deferred-scroll callback can still run against the closed editor (1368-1374).

Resolution

Give each provider's diff a distinct identity and close only that provider's view, or track all providers that share a diff and dispose their listeners and cancel their timers before closing it. Add a same-path, two-provider test that verifies one provider's reset or rejected-save cleanup does not leave another provider's listeners or timer active.

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the has-conflicts PR has merge conflicts with the base branch label Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review status

Thanks for contributing. This comment tracks the review sequence and the next action.

Current step: Required CI passed. Waiting for automated review of the latest commit.

If automated review does not start, a maintainer must restart it.

Review-state labels are managed by this workflow; do not edit them manually. community-approved is managed the same way — do not add or remove it manually. It signals a fresh community code approval for the current head as an advisory priority only; maintainer review is still required.

@github-actions github-actions Bot removed the has-conflicts PR has merge conflicts with the base branch label Oct 5, 2026
…ve (U1, issue 1375)

Split unit U1 of PR 1833. Three changes, each with a test that fails without it:
- a caller-supplied staging path is checked for location and file type before anything is written, so an arbitrary path or a symlink cannot be published onto the target;
- a failed parent-directory fsync on POSIX is reported as PostCommitDurabilityError instead of being swallowed, so a successful return never claims durability the filesystem did not grant;
- the staged file and this write's own staging directory are released before RollbackFailureError is thrown.

Focused coverage for resolveLockKey added: canonical parent directory, a dangling-link chain, and termination at the bounded depth on a two-link cycle.
@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

…ishTarget (U1, issue 1375)

The resolver may fall back to the given path only when lstat also reports the path as absent. An EACCES or EIO failure says nothing about whether the path is a link, so falling back would publish through a link we were not allowed to inspect. Focused tests added for both branches.
easonLiangWorldedtech added 2 commits October 5, 2026 22:30
… type-sound

compile failed at the unit head on three points:
- RollbackFailureError needs a string backupPath, but the throw now happens after cleanup, so the
  `string | null` narrowing was lost. The failure is now held as { error, backupPath }.
- The async lstat stand-in is built on the Stats prototype so it satisfies fsSync.Stats.
- The realpath/readlink mocks are typed to the real signatures; the readlink mock answers once
  because only the link path is read.

tsc clean, 50 tests pass, ESLint --max-warnings=0 clean, no suppression change.
easonLiangWorldedtech added 6 commits October 5, 2026 22:47
The any usage this entry covered is gone in the rewritten file, so the count drops 4 -> 3.
eslint --prune-suppressions --max-warnings=0 confirms it.
The read tools record the observed on-disk version through task.observationRegistry, but the field
was only declared in a later unit, so at this head the call dereferences undefined and the mocked
e2e run fails on the read_file smoke tests. The registry is introduced by this unit, so the field
belongs here.

tsc clean on this unit, 11 observationRegistry tests pass, ESLint --max-warnings=0 clean.
The two any usages this entry covered are gone in the rewritten spec, so the count drops 98 -> 96.
eslint --prune-suppressions --max-warnings=0 confirms it.
U6's ApplyPatchTool calls saveChanges with the writeKind argument, so the parameter must exist
before U6 can build. U8 owns that signature, so U8 now lands before U6.
@github-actions github-actions Bot added coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit and removed awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Oct 7, 2026
…nc warning sinks handled

Series alignment with fws/u6-apply-patch-wiring (plans in #41):

1. ApplyDiffTool builds its content from a read, then DiffViewProvider.open() records
   an observation for the path. If the file changed between the tool's read and that
   preview, the preview token is the only entry in the registry, and because the save
   is issued with kind "edit" the partial-observation rule accepts it: the tool's
   stale full-file content is published over the intervening change. open() now
   snapshots the observation that existed BEFORE it touched the registry and an
   edit-kind save restores it, so the compare-and-swap runs against the version the
   caller's content was built on; with no pre-open observation the preview's entry is
   withdrawn (ObservationRegistry#forget) and the unobserved-edit guard rejects the
   save with the re-read remediation.

2. safeWriteText's warning wrapper previously used the caller's callback directly, so
   a throwing sink aborted a committed write and an async sink leaked an unhandled
   rejection (Node's default mode can end the process after a successful write). It
   now catches synchronous throws and attaches a catch handler to a returned promise
   without awaiting it.

Tests ported from u6 (preview rejected with no authorization left behind; legitimate
pre-preview read still publishes; rejecting async sink reported through the fallback).
The safeWriteJson confinement-before-mkdir ordering is not ported here: this unit has
no pre-lock confinement check, and it inherits u6's version at rebase (U8 -> U6 -> U7).
@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

Series alignment with #1915, pushed in 7f70d96a7:

  • Preview authorization: DiffViewProvider.open() snapshots the observation that existed before it records the preview token; saveChanges(…, "edit") restores it so the compare-and-swap runs against the version the tool's content was built on, and with no pre-open observation the preview's entry is withdrawn via ObservationRegistry#forget so the unobserved-edit guard rejects the write instead of publishing stale content over an intervening change.
  • Warning delivery: safeWriteText used the caller's onWarning directly here, so a throwing sink could abort a committed write and an async sink leaked an unhandled rejection. It now catches throws and attaches a catch to a returned promise without awaiting.

Not ported: the safeWriteJson confinement-before-mkdir ordering — this unit has no pre-lock confinement check, and it inherits u6's fixed version at rebase (merge order U8 → U6 → U7).

Local: integrations/editor + services/file-safety + observationRegistry + applyPatchTool.execute = 5 files / 230 tests pass, eslint clean on all five touched files.

Series alignment with fws/u6-apply-patch-wiring. The e2e apply_diff suite drives apply_diff
without a prior read_file, so the only observation available at save time was the one
DiffViewProvider.open() records for the preview. Now that the preview cannot authorize
the save, the guarded save falls back to the unobserved-edit guard and the flow has no
authorization of its own.

ApplyDiffTool reads the file itself to compute the diff, so it observes that read (stat
around the read, observe only when the file did not change underneath it - the same
contract as ApplyPatchTool's hunk read). The save is authorized against the version the
diff was computed against: unchanged file -> the save proceeds; file changed after the
read -> the compare-and-swap rejects and the stale content is not published.
@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

Series alignment with #1915, pushed in 6e8560590: ApplyDiffTool now observes its own hunk read (stat around the read, observe only when the file did not change underneath it — the same contract as ApplyPatchTool). Without it, the e2e apply_diff suite (whose aimock fixture calls apply_diff with no prior read_file) has no authorization of its own once the preview can no longer authorize the save, and the guarded save correctly rejects with the unobserved-edit remediation.

Two unit tests ported (applyDiffTool.guardedWrite.spec.ts): the observation is recorded with completeness unearned; nothing is recorded when the file changed during the read. Local suites green, eslint clean.

@github-actions github-actions Bot removed coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Oct 7, 2026
The CI compile job rejected the new applyDiffTool.guardedWrite.spec.ts code: the task
stub's Pick<> did not list observationRegistry, and the BigIntStats doubles passed to
stat's mockResolvedValueOnce are partial (only the fields versionTokenOfStat reads - a
full BigIntStats cannot be built against the mocked fs, so the double assertion is the
narrowest option).
@github-actions github-actions Bot added coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Oct 7, 2026
@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

All seven required checks are green at e7138ab74 and every review thread is resolved; there is no CodeRabbit review at this head yet.

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 8 minutes.

… edit

Series alignment with fws/u6-apply-patch-wiring. The autosave adoption branch accepted ANY
GuardRejectedError when the buffer was clean. An "edit" save with no pre-open observation
is rejected by the unobserved-edit guard - an authorization verdict, not a moved-token
verdict - so if VS Code autosave had already written the buffer, adoption reported success
AND recorded a partial observation for a file the model never read, which would then
authorize a later targeted publish. Adoption is now limited to saves that were authorized
before open().

Also removes the dead committed flag in safeWriteText (declared and set, never read) and
corrects the comment describing a backup-restore guard that does not exist: the backup is
a copy and is never restored.
@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

Series alignment with #1915, pushed in 92bb178bc:

  • Adoption gate: the autosave adoption branch accepted any GuardRejectedError with a clean buffer. An "edit" save with no pre-open observation is rejected by the unobserved-edit guard — an authorization verdict — so adoption would have reported success and recorded a partial observation for a file the model never read, authorizing a later targeted publish. Adoption is now limited to saves authorized before open().
  • Dead flag: removed the committed local in safeWriteText (declared and set, never read) and corrected the comment that described a backup-restore guard which does not exist.

Test ported (does not adopt an autosaved match for an edit that was never authorized); local suites green, eslint clean.

@github-actions github-actions Bot added coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit and removed coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Oct 7, 2026
…erent version

Series alignment with fws/u6-apply-patch-wiring and fws/u9-task-history-delete. ApplyDiffTool
rewrites an observation only when none exists (its own hunk read is the only authorization
apply_diff can have, since it computes its hunks from that read) or when the prior entry is
on the same version, preserving the completeness the model earned. A prior entry on an older
version is left alone, so content built from a stale read cannot pass the save's
compare-and-swap.

Also repairs the interleaved confinement comment in safeWriteJson (and, where present, the
confineTo doc) so it describes both checks: the declared path before the lock and before any
parent-directory creation, and the resolved publish target again under the lock.
@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

Series alignment with #1915 / #1917, pushed in bb9ad00b0:

  • Observation refresh rule: ApplyDiffTool rewrites an observation only when none exists (its own hunk read is the only authorization apply_diff can have) or when the prior entry is on the same version — preserving the completeness the model earned. A prior entry on an older version is left alone, so content built from a stale read can no longer pass the save's compare-and-swap.
  • Comment/doc repair in safeWriteJson: the interleaved confinement comment is repaired and the confineTo doc now describes both checks.

Local suites green (applyDiffTool.guardedWrite 7 tests, safeWriteJson, integrations/editor), eslint clean.

@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 33 minutes.

@github-actions github-actions Bot added coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit and removed coderabbit-review-active Required CI passed; CodeRabbit review is active awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit labels Oct 7, 2026
easonLiangWorldedtech pushed a commit to easonLiangWorldedtech/Zoo-Code that referenced this pull request Oct 7, 2026
The misc-lane failure (ClineProvider.delegation.spec.ts > keeps directory cleanup and parent
restoration when the child history lock failure is swallowed) is not reachable from these
commits: the delegation spec lives in the misc group (__tests__/**), which does not include
core/**, and the only files this branch adds there are a comment in utils/safeWriteJson.ts
and a new spec under core/webview/__tests__ (core group). The same spec passes on the
sibling branches Zoo-Code-Org#1916 and Zoo-Code-Org#1918 at their current heads, which carry the identical
ApplyDiffTool change. Re-running the lane to confirm.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-coderabbit Waiting for CodeRabbit to approve the latest commit coderabbit-review-active Required CI passed; CodeRabbit review is active

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant