[split-1066] U6 - fix(write-to-file): run the diff cleanup when handleError rejects #1937

Activity

1. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Scope re-cut for U6 (#1932): which extra files are not U6's, and which one is

   Trigger: CodeRabbit Out of Scope Changes check (Error) on #1932 at 443993e57. Row, verbatim:

   Issue #1937 defines U6 as the single execute() error path and lists two files: WriteToFileTool.ts and writeToFileTool.spec.ts. The PR also changes BaseTool.ts, ClineProvider.ts, DiffViewProvider.ts, and their tests.

   The row's own second remedy is the one we are taking: or update issue #1937 to explicitly define and assign the additional coding scope before merge. This comment is that definition; #1937 now carries the same text in its body. It is per file, because the three files have three different answers, and the reason for each is the owning commit, not the fact that the code already exists.

   1. src/core/tools/BaseTool.ts (+36/-3) - NOT U6. Owned by e3c10401f feat(tools): onParameterParseFailure teardown boundary, which is U3 (#1931). git log -m 036245c5e..HEAD -- src/core/tools/BaseTool.ts returns that commit and the org-main merge and nothing else. It appears in #1932's file list only because the split branches live on the fork, so every unit PR in the chain is opened against main and GitHub shows the cumulative diff through this unit - which is stated in the PR body under Chain position. Merge order is fixed at U12 (#1927) -> U4 (#1929) -> U5 -> U3 (#1931) -> U6 (#1932) -> U7/FINAL (#1928); by the time U6 merges, U3 has merged and this hunk is no longer in U6's diff.

   2. src/core/webview/ClineProvider.ts (+6/-0) - NOT U6. Owned by d172c95ae feat(write-to-file): per-task partial stream state + cleanup primitives, which is U4 (#1929): the writeToFileTool.clearTaskState(task) call before task.dispose(), i.e. the history-task disposal the row names. Same cumulative-diff explanation as above.

   3. src/integrations/editor/DiffViewProvider.ts (+121/-10) and src/integrations/editor/__tests__/DiffViewProvider.spec.ts (+213/-1) - U6's own, and it cannot live outside U6. Owned by 443993e57, a U6 commit. The reason is not that it was written here first:

   • U6's contract is a finally around handleError so the diff cleanup runs even when handleError rejects. The cleanup that finally runs is DiffViewProvider.revertChanges(). Before this hunk, for a new file whose openDiffEditor() had rejected there was no activeDiffEditor and revertChanges() returned early, leaving the empty placeholder and the directories open() had created on disk. The invariant U6 asserts was then only vacuously satisfied at U6's own head: the finally ran, called a function, and that function did nothing. A test of the cleanup runs that cannot distinguish called from did something does not constrain the code U6 is about.
   • The same hunk stops revertChanges() from saving a dirty buffer on the rollback path. Saving unapproved streamed content is the state the execute() error path exists to roll back, so the flag writeApproved and the buffer behaviour are the same invariant seen from two sides; splitting them across units would let one side move without the other.
   • The 213 added spec lines are the tests for exactly that behaviour, so they are in scope by the same argument as the code they pin.

   Assigned to U6 explicitly: src/integrations/editor/DiffViewProvider.ts and src/integrations/editor/__tests__/DiffViewProvider.spec.ts, for the diff-cleanup target of the execute() error path only (rollback of an unapproved stream: placeholder, created directories, and the buffer that must not be saved). Anything in that file beyond the rollback target stays out of U6.

   Head this declaration is written against: 443993e57d65b75c8b1a3a2121af7e495104d538. The follow-up commit that fixes the two remaining rows on #1932 (Persistence Integrity: route the rollback through discardUnapprovedStream() as defined by the earliest unit that defines it, U7/#1928 f8f7ce19a; Lifecycle: the two-window + liveness shape from U4/#1929 61dd05a2c) stays inside the file pair declared above.
2. easonLiangWorldedtech

   added a commit that references this issue on Oct 10, 2026

   fix(write-to-file): cover the pre-streaming window and discard withou…

   ...

   19e8512
3. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Migrated from #41 comment 6093028889 - this note belongs to the split-1066 chain (chain map: #1989), not to that unrelated closed bug (fork-issue numbering trap).

   Scope re-cut for U6 (#1932): which extra files are not U6's, and which one is

   Trigger: CodeRabbit Out of Scope Changes check (Error) on #1932 at 443993e57. Row, verbatim:

   Issue #1937 defines U6 as the single execute() error path and lists two files: WriteToFileTool.ts and writeToFileTool.spec.ts. The PR also changes BaseTool.ts, ClineProvider.ts, DiffViewProvider.ts, and their tests.

   The row's own second remedy is the one we are taking: or update issue #1937 to explicitly define and assign the additional coding scope before merge. This comment is that definition; #1937 now carries the same text in its body. It is per file, because the three files have three different answers, and the reason for each is the owning commit, not the fact that the code already exists.

   1. src/core/tools/BaseTool.ts (+36/-3) - NOT U6. Owned by e3c10401f feat(tools): onParameterParseFailure teardown boundary, which is U3 (#1931). git log -m 036245c5e..HEAD -- src/core/tools/BaseTool.ts returns that commit and the org-main merge and nothing else. It appears in #1932's file list only because the split branches live on the fork, so every unit PR in the chain is opened against main and GitHub shows the cumulative diff through this unit - which is stated in the PR body under Chain position. Merge order is fixed at U12 (#1927) -> U4 (#1929) -> U5 -> U3 (#1931) -> U6 (#1932) -> U7/FINAL (#1928); by the time U6 merges, U3 has merged and this hunk is no longer in U6's diff.

   2. src/core/webview/ClineProvider.ts (+6/-0) - NOT U6. Owned by d172c95ae feat(write-to-file): per-task partial stream state + cleanup primitives, which is U4 (#1929): the writeToFileTool.clearTaskState(task) call before task.dispose(), i.e. the history-task disposal the row names. Same cumulative-diff explanation as above.

   3. src/integrations/editor/DiffViewProvider.ts (+121/-10) and src/integrations/editor/__tests__/DiffViewProvider.spec.ts (+213/-1) - U6's own, and it cannot live outside U6. Owned by 443993e57, a U6 commit. The reason is not that it was written here first:

   • U6's contract is a finally around handleError so the diff cleanup runs even when handleError rejects. The cleanup that finally runs is DiffViewProvider.revertChanges(). Before this hunk, for a new file whose openDiffEditor() had rejected there was no activeDiffEditor and revertChanges() returned early, leaving the empty placeholder and the directories open() had created on disk. The invariant U6 asserts was then only vacuously satisfied at U6's own head: the finally ran, called a function, and that function did nothing. A test of the cleanup runs that cannot distinguish called from did something does not constrain the code U6 is about.
   • The same hunk stops revertChanges() from saving a dirty buffer on the rollback path. Saving unapproved streamed content is the state the execute() error path exists to roll back, so the flag writeApproved and the buffer behaviour are the same invariant seen from two sides; splitting them across units would let one side move without the other.
   • The 213 added spec lines are the tests for exactly that behaviour, so they are in scope by the same argument as the code they pin.

   Assigned to U6 explicitly: src/integrations/editor/DiffViewProvider.ts and src/integrations/editor/__tests__/DiffViewProvider.spec.ts, for the diff-cleanup target of the execute() error path only (rollback of an unapproved stream: placeholder, created directories, and the buffer that must not be saved). Anything in that file beyond the rollback target stays out of U6.

   Head this declaration is written against: 443993e57d65b75c8b1a3a2121af7e495104d538. The follow-up commit that fixes the two remaining rows on #1932 (Persistence Integrity: route the rollback through discardUnapprovedStream() as defined by the earliest unit that defines it, U7/#1928 f8f7ce19a; Lifecycle: the two-window + liveness shape from U4/#1929 61dd05a2c) stays inside the file pair declared above.
4. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Migrated from #41 comment 6094009717 - this note belongs to the split-1066 chain (chain map: #1989), not to that unrelated closed bug (fork-issue numbering trap).
   Port sources recorded for #1932 (p1066/u6-execute-error-path-cleanup) - the unit's rollback fixes are ports, and the chain has to converge on one shape of each primitive:

   hunk	ported from	why
   discardFileTab() restore-then-save before close(tab), saveBufferClean()	#1931 5c0f21219	same primitive, one shape; close()'s 2nd arg is preserveFocus, so the forced close never discarded a dirty tab
   saveChanges(onCommit) commit point + writeCommitted stand-down	#1931 2a9bfabfb	the durable-write fact is what the rollback must obey, not the approval fact
   placeholderPath released after the approved save	#1929 8084eab2e	#1929's Persistence Integrity fix; #1932 carries the same saveChanges()

   Deliberate deviation: the Critical thread also asks to gate revertChanges() on an active edit session. Not taken in #1932 - that guard is #1931's shape of the same method (already filed as a follow-up for #1931's next push). #1932 takes the other half (reset() clears relPath/newContent), which removes the stale-path trigger the thread's reproductions depend on, and its tests record the current guard rather than the desired one.

   Correction to the Major rooignore thread: the two missing-parameter early returns did already call reset(); what none of the three branches did was discard the content an earlier streaming delta had put in the diff view. All three now run the shared discard-then-reset teardown.
5. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Port note: bring the commit-point semantics with the DiffViewProvider half

   The DiffViewProvider content U6 ports from 5c0f21219 (restore buffers before closing) sits in
   the same method as the commit-point signal added in U3 2a9bfabfb:
   saveChanges(diagnosticsEnabled, writeDelayMs, onCommit?) fires onCommit immediately after the
   document write (including the clean-document case, where disk already matches), and
   WriteToFileTool keeps a writeCommitted flag so its catch path does not roll back a write that
   already landed.

   Why it matters here: U6 owns the execute() catch cleanup. If it rolls the diff view back
   without checking the commit point, it undoes a write the user approved. Two further facts about
   the catch path, both verified at U3 head 5ff7bea73:

   • revertChanges() early-returns when !isEditing, and isEditing is set only by open(), so
     on the saveDirectly (prevent-focus-disruption) branch the cleanup is a no-op while the
     caller still reads "reverted". A test that runs only the default branch passes and misses it.
   • reset() clears createdDirs / adoptedCreatedDirs without touching disk, so anything not
     rolled back before the reset is unreachable.

   Port source (per-hunk ownership = the earliest commit defining the hunk): U3 / #1931 commits
   5c0f21219 and 2a9bfabfb. Carry onCommit and writeCommitted in the same positions, or the
   two branches will disagree when they merge.

   Ported from the retired fork tracking item 6093538947 / the U3 note above.