Repository navigation
feat(tools): route the remaining write tools through the guard (U7, #1375) - #1918
easonLiangWorldedtech wants to merge 30 commits into
Conversation
|
Warning Review limit reachedYou'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. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (14)
📝 SummarySummary by CodeRabbit
WalkthroughThe pull request adds per-task file-version observations and guarded writes for file tools. It also adds an atomic text publisher and updates ChangesObserved and Guarded File Writes
Atomic Text and JSON Publishing
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
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
Full details: Regression EvidenceExplanation The changed request-loop branch lacks focused coverage. Full details: Lifecycle Resource CleanupExplanation The new 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 💡
🧪 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 |
Review statusThanks 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. |
f1173b0 to
ba67839
Compare
…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.
ba67839 to
da326b7
Compare
|
@coderabbitai full review |
|
…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.
da326b7 to
a336d59
Compare
… 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.
a336d59 to
a147140
Compare
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.
a147140 to
41b6d11
Compare
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.
…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).
|
Series alignment with #1915, pushed in
Not ported: the Local: |
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.
|
Series alignment with #1915, pushed in Two unit tests ported ( |
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).
|
All seven required checks are green at @coderabbitai full review |
|
… 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.
|
Series alignment with #1915, pushed in
Test ported ( |
…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.
|
Series alignment with #1915 / #1917, pushed in
Local suites green ( |
|
@coderabbitai full review |
|
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.
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_replacepublish 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, base7c291bb08→ head6768ccfaf, replayed on the current main tip9af61f87eso 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=0clean on every file in the unit; Prettier clean;src/eslint-suppressions.jsonnever increased.The GitHub view also carries the unmerged base, so the numbers above are this unit's own delta.