[split-1066] U7 - fix(write-to-file): clean partial state on missing-param and rooignore denial #1938

Activity

1. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

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

   Follow-up: closeAllDiffViews() must run after the buffer restore inside discardUnapprovedStream()

   Found while fixing the Persistence Integrity row on #1932 (shipped in 19e85126d, where the primitive is ported). It is not a regression - the primitive is new in U7/#1928, so nothing on main behaves differently - which is why it is filed here instead of being pushed under a review that is already in flight.

   The defect. In the primitive as defined by U7/#1928 f8f7ce19a (and therefore in U4/#1929 61dd05a2c), the editor work runs in this order:

   1. disposeActiveEditorListener(), cancelDeferredScroll()
   2. await this.closeAllDiffViews()
   3. if the buffer is dirty: empty it (create) or restore the pre-stream content (modify), then save the emptied placeholder
   4. closeFileTab(absolutePath), then unlink the placeholder and remove the created directories

   A vscode.diff tab is dirty exactly while its modified side holds the streamed content, and closeAllDiffViews() deliberately skips dirty tabs (closing one would prompt to save). So step 2 skips the only tab that matters, step 3 makes the buffer clean, and step 4 matches TabInputText tabs - which a diff tab is not (openDiffEditor() creates a TabInputTextDiff). Net effect: the discard unlinks the file underneath a diff tab that is still open, now clean and empty.

   Measured, not argued. The regression test on #1932 (closes the dirty vscode.diff tab that only TabInputTextDiff matches, before the unlink) stubs nothing of the provider's own tab handling and models the tab's isDirty as a getter over the document's dirty flag. Against the U7 order it fails with expected -1 to be greater than -1 on callOrder.indexOf("closeTab") - the tab is never closed. Against the corrected order it passes, and the negative control (moving the call back before the restore) turns exactly 3 tests red, including that one.

   The fix is one line: move await this.closeAllDiffViews() from before the dirty-buffer block to just before closeFileTab(absolutePath). The buffer is emptied first, so the tab is clean when it is closed and no save prompt is possible - which is the invariant closeAllDiffViews()'s dirty check exists to protect.

   Where it has to land. U7/#1928 f8f7ce19a (the unit that defines the primitive) and U4/#1929 61dd05a2c (which ports it). Each is a one-line move plus two expect(callOrder) assertions that currently expect closeDiffViews first:

   • expect(callOrder).toEqual(["closeDiffViews", "applyEdit", "save", "closeFileTab"]) → ["applyEdit", "save", "closeDiffViews", "closeFileTab"]
   • expect(callOrder).toEqual(["closeDiffViews", "applyEdit", "closeFileTab"]) → ["applyEdit", "closeDiffViews", "closeFileTab"]

   Landing timing (lead's call): both units have a review request in flight (f8f7ce19a at 02:22, 61dd05a2c at 02:47), so pushing now would void two reviews for a non-regression. Carry this with the next push either unit makes for any other reason; if both go green without it, fix it in one follow-up PR after the chain merges. U6/#1932 already carries the corrected order, so the chain converges either way.
2. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   Last edited by easonLiangWorldedtech

   ContributorAuthor

   More actions

   Head is missing the rollback batch

   The head of PR 1928 does not yet carry the cleanup and rollback fixes that landed on the sibling units:

   • eb245494a (branch p1066/u5-streaming-failure-capture): revertChanges() requires both relPath and isEditing, reset() clears relPath, discardFileTab() restores the buffer and saves it clean before closing instead of passing true to tabGroups.close() as a force-discard flag, and a refused close now fails the rollback.
   • 5c0f21219 (branch p1066/u3-parse-failure-boundary): the same shape plus the execute() cleanup hazard report.

   Without it this unit's diff is incomplete and the merge will conflict. Bring the batch over in the same push as the cancellation change, and re-run this unit's specs, type check, and negative controls afterwards.
3. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Follow-up for the second half of the Persistence Integrity pre-merge checklist row on pull 1928 (unit U7, early-return and denial cleanup).

   Shipped with the fix: a missing provider is treated as a failed metadata stage and pendingTaskMetadataRepair is retained instead of being cleared.

   Still open: in-flight saveClineMessages and persistTaskMetadata calls are not tracked, so disposeOnce can conclude there is nothing pending while a stage is still running.

   Acceptance: disposeOnce awaits every in-flight metadata or message write, or the writes are serialised behind one promise chain, and a test proves that a write started just before disposal still lands and still sets pendingTaskMetadataRepair when it fails.

   Owner: unit U7. Recorded locally first, back-filed here so the argued half of the row has a scoped target.
4. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Record, no action required: persistTaskMetadata() carried an unreachable trailing return true after an unconditional throw/return path. Removing it (commit f873a3d) moves no test - all 180 tests in the affected file stay green with the statement restored - which is the same fact the surviving BooleanLiteral mutant at that line reports.

   Registered so the mutation advisory for that line is not re-derived as an untested behaviour change: the statement was dead code, and no test can distinguish its removal.