[Tracking] split-1066 chain: rules, port sources, chain-level rows #1989

Activity

1. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

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

   Rule: a provider-state await belongs inside the cleanup-owned boundary

   Two units of the #1066 chain grew the same defect from opposite directions, which is why this is worth a line here rather than two commit messages:

   • U4 (feat(write-to-file): per-task partial stream state + cleanup primitives (split 2/6 of #1066) #1929) wrapped the setup work in a try and then put diffViewProvider.open() / update() after it, at fb21709cd lines 533-549. A rejection there reached BaseTool.handle() with the per-task entry and its TaskAborted listener still attached. Fixed in 61dd05a2c by moving the diff-view work inside the boundary and giving the catch two windows (setup failure releases; diff-view failure keeps the entry and marks it, because releasing at the delta is what lets the next delta replay the operation that just failed).
   • U6 (fix(write-to-file): run diff cleanup when handleError rejects (split 5/6 of #1066) #1932) did the mirror: open() / update() are inside a local try (443993e57), but await provider?.getState() sits outside it at lines 510-511, so the same leak survives on the other side of the same function.

   The rule: any await that happens after per-task state has been registered must be inside the boundary that releases that state - including the awaits that look like read-only setup (providerRef.deref(), getState(), a settings lookup, an isEnabled() read). getState() is not a pure read: it is an await into the extension host and it can reject or be superseded by a task switch.

   Same-shape sites to check beyond U4/U6 (measured at the heads named above, git grep over src/core/tools):

   • U4 feat(write-to-file): per-task partial stream state + cleanup primitives (split 2/6 of #1066) #1929: 16 hit(s)
   • src/core/tools/ApplyDiffTool.ts:130: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:172: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:328: const state = await provider?.getState()
   • src/core/tools/EditFileTool.ts:392: const state = await provider?.getState()
   • src/core/tools/EditTool.ts:167: const state = await provider?.getState()
   • src/core/tools/ExecuteCommandTool.ts:167: const providerState = await provider?.getState()
   • U6 fix(write-to-file): run diff cleanup when handleError rejects (split 5/6 of #1066) #1932: 16 hit(s)
   • src/core/tools/ApplyDiffTool.ts:130: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:172: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:328: const state = await provider?.getState()
   • src/core/tools/EditFileTool.ts:392: const state = await provider?.getState()
   • src/core/tools/EditTool.ts:167: const state = await provider?.getState()
   • src/core/tools/ExecuteCommandTool.ts:167: const providerState = await provider?.getState()
   • U7 fix(write-to-file): clean partial state on missing-param and rooignore denial + integrate the #1066 split series (6/6 + FINAL) #1928: 16 hit(s)
   • src/core/tools/ApplyDiffTool.ts:130: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:172: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:328: const state = await provider?.getState()
   • src/core/tools/EditFileTool.ts:392: const state = await provider?.getState()
   • src/core/tools/EditTool.ts:167: const state = await provider?.getState()
   • src/core/tools/ExecuteCommandTool.ts:167: const providerState = await provider?.getState()
   • U3 feat(tools): onParameterParseFailure teardown boundary (split 4/6 of #1066) #1931: 16 hit(s)
   • src/core/tools/ApplyDiffTool.ts:130: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:172: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:328: const state = await provider?.getState()
   • src/core/tools/EditFileTool.ts:392: const state = await provider?.getState()
   • src/core/tools/EditTool.ts:167: const state = await provider?.getState()
   • src/core/tools/ExecuteCommandTool.ts:167: const providerState = await provider?.getState()
   • U3-fws feat(task): observation registry with read completeness (U3, #1375) #1912: 16 hit(s)
   • src/core/tools/ApplyDiffTool.ts:130: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:172: const state = await provider?.getState()
   • src/core/tools/ApplyPatchTool.ts:328: const state = await provider?.getState()
   • src/core/tools/EditFileTool.ts:392: const state = await provider?.getState()
   • src/core/tools/EditTool.ts:167: const state = await provider?.getState()
   • src/core/tools/ExecuteCommandTool.ts:167: const providerState = await provider?.getState()

   Cross-references: U4 fix 61dd05a2c (this rule's first half), U6 follow-up commit (second half, will cite this comment), U3 #1931 1b09bab32 already carries the liveness-check shape that U6 ports for its Lifecycle row.
2. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

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

   Chain-level row: Persistence Integrity (a rollback path can persist unapproved streamed content)

   This row is red at the same time on #1916 / #1921 / #1929 / #1930, so it is one chain-level defect rather than four per-PR defects. Recording the ownership split here before anyone ports it.

   • Primitive ownership — DiffViewProvider.restorePreStreamBuffer() / saveBufferClean() are owned by fix(write-to-file): capture streaming failure once, report it once (split 3/6 of #1066) #1930 (p1066/u5-streaming-failure-capture). git log --all -S restorePreStreamBuffer -- src/integrations/editor/DiffViewProvider.ts puts the first definition in 6cae369d9 ("fix(diff-view): treat a refused discard as a rollback failure"), and git branch -a --contains 6cae369d9 shows it only on that branch.
   • Semantic baseline — applyEdit() === false means the rollback failed: do not save() the streamed document, restore or close it without saving, and report the failure. That is the rule fix(write-to-file): clean partial state on missing-param and rooignore denial + integrate the #1066 split series (6/6 + FINAL) #1928 already broadcast for DiffViewProvider.discardUnapprovedStream() (its editorFailure channel).
   • Port direction — fix(write-to-file): capture streaming failure once, report it once (split 3/6 of #1066) #1930 → feat(editor): route the diff-view save through the guard (U8, #1375) #1916 / test(webview): ClineProvider parallelMode suite with viewStates pruning edges #1921 / feat(write-to-file): per-task partial stream state + cleanup primitives (split 2/6 of #1066) #1929 / fix(write-to-file): run diff cleanup when handleError rejects (split 5/6 of #1066) #1932.

   Ownership and semantic baseline are two different facts and must be labelled separately. The earliest definer owns the primitive; the semantic baseline may be a rule another unit already broadcast. Marking only the owner makes every unit invent its own variant while porting, which is how one row ends up red on four PRs.

   Shape shipped on #1930: the contract lives in the callee. restorePreStreamBuffer() throws when applyEdit() returns false, so the saveBufferClean() call that follows it cannot run — nothing saveable survives — and the failure reaches revertChanges() → WriteToFileTool.revertDiffChangesBeforeReset(), which already returns it as rollbackError for the caller to report. discardFileTab() is byte-for-byte unchanged, because #1932 depends on its current shape (restore + save hoisted above the tab loop). A refused restore is asserted to reach the user as a failure, not as a successful rollback.
3. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Port source of record for the write_to_file rollback shape

   eb245494a on p1066/u5-streaming-failure-capture is the source of record for the shared rollback shape: revertChanges() requires both relPath and isEditing; reset() clears relPath (and newContent); discardFileTab() restores the buffer to its pre-stream content and saves it clean before closing, because the second argument of tabGroups.close() is preserveFocus and not a force-discard flag; a close the editor refuses now fails the rollback instead of deleting the file underneath an open tab. WriteToFileTool reverts the diff view only while an edit is in progress. 215e32d3e carries the matching eslint suppression prune.

   5c0f21219 on p1066/u3-parse-failure-boundary ports that half verbatim and adds the execute() cleanup hazard report, matching onParameterParseFailure() and cleanupFailedPartialStream().

   Chain-level requirement: port both verbatim to the remaining units in merge order, and record per unit how many tests each negative control kills. The counts differ per unit by design, because the same guard covers several call sites and each unit reaches a different subset of them.
4. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Chain rule: a suppression reduction must ship in the same commit

   The compile job runs the repository lint, and the strict run compares the counts in
   src/eslint-suppressions.json in both directions. A change that removes a suppressed violation -
   replacing an as any cast with bracket notation, for example - makes the stored count too high, and
   the run exits 2 with "There are suppressions left that do not occur anymore" even though the file
   itself reports no violations.

   This bit PR 1930: the first push reported a clean per-file lint and still failed compile, because a
   per-file run cannot see a count drift.

   Acceptance for every unit in this chain:

   • Before pushing, run the full lint the same way CI does, from the src directory:
     eslint . --ext=ts --max-warnings=0. A per-file run is not sufficient.
   • If it exits 2, regenerate with --prune-suppressions, rewrite the JSON with tab indentation, and
     confirm the diff touches only the affected count lines.
   • Ship the pruned file in the same commit as the change that reduced the count, never in a later
     cleanup commit.
   • After merging main into a unit branch, prune again on the merged tree: both sides' counts move
     together, and CI builds the merge commit rather than the branch tip.
5. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   Last edited by easonLiangWorldedtech

   ContributorAuthor

   More actions

   Two chain-level entries for this chain, registered from the local queue.

   1. Series convention, not a defect: the format check script and the root prettier ignore file arrive through the PR merge ref, not through a unit branch's own base, so every unit needs its own pure-formatting commit once it merges current org main. Register it once as a convention, not as one issue per unit. Acceptance: in a worktree checked out with core.autocrlf=false, the format check reports zero offenders at the current merge ref, and only files inside that PR's diff are touched. Shared files converge because the formatter output is deterministic, so ordering between units does not matter.

   2. Fixing one call site does not fix a defect class. The Persistence Integrity row stayed red after one call site was fixed, because the existing-file branch of the same method had its own copy of the unchecked restore-and-save. When a row names a class, enumerate the other call sites and register them - changed only if the row covers them.

   Unit-scope debt for U5 (five unchecked result sites and one await-boundary follow-up) is on issue 1935.

   ---

   Addendum to entry 1, measured on the current base (2026-10-10, unit U7): org main deleted the root prettier ignore file between 09e7326 and d1f5845 - the same batch that added the format check script - so the intent is that the whole tree is formatted, not a subset. Consequence: the compile job (pnpm format:check, i.e. prettier --check over the tree) now reaches 37 files that were previously ignored, and three of them were unit U7's own spec files, which turned that unit's compile job red. The unit-level fallout is already fixed in pull 1928 commit b2b3b0d.

   Two things the chain should settle once, so no later unit rediscovers them:

   • Do not re-add the ignore file inside a unit pull request. Whoever's merge ref lands on the new base carries a separate pure-formatting commit produced by the repository's own prettier version, with a commit message stating that the deviations are not introduced by that pull request. Acceptance for that commit: the diff ignoring whitespace is empty.
   • Re-fetch and re-measure the pull request merge ref before every push. The dirty count is a property of the merge ref GitHub rebuilds, so it can jump from 0 to 37 when the base moves; a previous round's count is not this round's count.

   No action required by any unit that has not merged onto the new base yet.
6. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   Last edited by easonLiangWorldedtech

   ContributorAuthor

   More actions

   Chain-level tooling note for the 1066 split units: org main is not tsc-clean on its own. Merging it brings 12 type errors (webviewMessageHandler.readOriginalContent.spec.ts accounts for 8, plus ClineProvider.spec.ts and WebviewFocusTracker), which moves the merged-tree baseline of every unit from 62 to 74.

   Consequence for how we report: an absolute tsc count is meaningless when upstream carries a dirty baseline. The check has to be an A/B on the same tree - run tsc at HEAD and at HEAD~1, or with and without the change, and require the error sets to be identical by name; this is the same trick as proving a local red with a stash A/B.

   In commit messages write "A/B equivalent, error sets identical by name", not "tsc 0 errors": when the baseline is not zero a bare 0 reads as a claim about the baseline.

   Acceptance: either org main reaches zero with the temporary tsconfig paths mapping for the types package, or every unit records its own merged-tree A/B result instead of an absolute number.
7. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   Chain-level entry from unit U6 (pull 1932): revertChanges() has no "is an edit session active" gate, so a caller that holds no session can still reach the new-file unlink branch.

   Unit U6 removed the stale-path trigger instead of adding the gate: reset() now clears relPath and newContent (commit 694bed4), so the branch is no longer reachable from the path that triggered it here. The shape question is still chain-level because unit U7 carries the gate in a different form (5c0f212), and the two units now differ deliberately.

   Acceptance if the chain wants one shape: the gate exists at the public entry point in every unit that carries the unlink branch, plus a test that revertChanges() with no session is a no-op. A guard that sits only on an internal caller is not a guard.

   Recorded so the next unit does not re-derive the difference between the two units; no action required by U6.
8. easonLiangWorldedtech commented on Oct 10, 2026

   

   easonLiangWorldedtech

   on Oct 10, 2026

   ContributorAuthor

   More actions

   One fail-closed rollback path for the write-to-file stream lifecycle

   Defect: the chain currently has four separate rollback exits that each decide on their own whether to reset per-task stream state - the streaming-failure rollback, the approval-denial revert, the parse-failure release, and the task-abort teardown. Each of them can call reset while a dirty, never-approved buffer is still on disk or still open in a diff view, because closing diff views skips dirty tabs. When the restore or close itself fails, the state is dropped anyway, so a later retry can publish content the user never approved, and the per-task entry plus its abort listener can be retained.

   Acceptance, not bound to a commit:

   1. One shared rollback path is used by all four exits; no exit resets or releases state while a dirty buffer remains.
   2. Any restore, save, close, or delete failure inside that path records an unrecoverable per-task rollback failure.
   3. While that failure is recorded, later writes and saves for the task are blocked, including the approval and parse-failure routes, and the failure is reported exactly once.
   4. The state clears only after the buffer is verified clean, the tab is closed, and the placeholders are removed.
   5. The shape is defined once in the unit that owns the shared diff-view rollback behaviour and ported forward in the declared merge order, re-derived per branch rather than copied byte for byte.

   Negative-control shape: one mutant per production call site, each reddening only the tests naming that behaviour; run each mutant against the pre-fix spec to show the branch was previously uncovered; if a mutant reddens far more tests than the target, assume the mutant is wrong first; restore mutants byte for byte and verify with a hash.

   Related but separate: the parse-failure release hook is never reached on the production malformed-completion path, because the caller emits its own tool result and returns before delegating to the tool handler. That leaves the per-task entry and its abort listener retained and suppresses later previews. Its fix belongs to the unit that introduced the hook, and it must preserve the one-result-per-call behaviour and the existing report ordering.