[easonLiangWorldedtech]

This PR routes the interactive editor's save flows - the diff view's saveChanges and saveDirectly - through the guard, and fixes the ordering between a save and a teardown so a cancellation can never resurrect rolled-back content: when a teardown begins while a guarded publish is in flight, the rollback waits for the publish to settle and is provably the last writer; open() serializes with the previous session's finalization so a new edit session cannot start underneath an old cleanup.

The diff view is the highest-traffic write path in the extension. This is the eighth of nine PRs for epic #1375 (chain tracked on #1991).

Related GitHub Issue

Tracking: issue 2023 (this unit's tracking issue; deliberately not a closing reference - the maintainer closes it when the series lands).
Refs: #1991 (chain tracking), #1375 (epic).

Description

What this PR changes:

1. saveChanges publishes through guardedWrite and registers the in-flight publish; a teardown that begins during the publish is honored after the publish settles (the save reports no completed edit rather than claiming one).
2. revertChanges claims the cancellation first, then waits for the in-flight guarded publish before rolling back - the rollback is provably the last writer (verified with a barrier test).
3. open() waits for a prior session's finalization or teardown before starting the new session, and the cancellation claim is cleared with the teardown marker on a clean start.
4. saveDirectly forwards a caller-supplied completeness flag through the direct publish, so a tool-level write can state whether the destination was fully observed.

Why the diff looks bigger than the change: the chain is non-cumulative - this branch is based on main and carries its own copy of the earlier units' content (guarded-write core, observation registry, atomic publish, lock key), which the GitHub diff shows as context. This unit's own delta is 486 lines.

Review focus: src/integrations/editor/DiffViewProvider.ts (saveChanges, revertChanges, open) and the new tests in src/integrations/editor/__tests__/DiffViewProvider.spec.ts and src/core/tools/__tests__/guardedWrite.spec.ts.

Test Procedure

From the repository root, with dependencies installed:

pnpm --dir src exec vitest run --globals integrations/editor/__tests__/DiffViewProvider.spec.ts
pnpm --dir src exec vitest run --globals core/tools/__tests__/guardedWrite.spec.ts
pnpm --dir src exec eslint . --ext=ts --max-warnings=0

Environment: Windows 11, Node 20, unit layer only.

Pre-Submission Checklist

• Scope is one gate scope and matches the unit.
• No .changeset or CHANGELOG changes (AGENTS.md).
• src/eslint-suppressions.json unchanged - no suppression count increased.
• New code lints clean (--max-warnings=0) rather than relying on suppressions.
• Tests added at the lowest layer that would have caught each finding.
• Visual Snapshot item considered: not applicable (no rendered surface).

Visual Snapshots

Not applicable - no rendered surface change; the diff view looks identical.

Videos (interaction / animation only)

Not applicable.

Documentation Updates

No user-facing documentation change: no new setting, no new surface, no model-facing text. No .changeset and no CHANGELOG edit (AGENTS.md).

Additional Notes

• Budget: own delta 486 lines / inside the caps. The mutation gate reports the GitHub diff's changed-line count (1218 against the 500 cap at the previous head) because the unlanded predecessors inflate it; the row clears on rebase after the predecessors merge.
• Concurrency tests were red-first verified against the pre-fix code (each failed with exactly the signature its review row describes), and four negative controls (dropping the rollback wait, the claim check, the completeness override, and the forwarding) each reddened exactly their test; all restores byte-identical.
• Content source of record: the superseded umbrella PR feat(tools): track read completeness and guard the diff-view save path (S4b, #1375) #1833 (closed), replayed onto the current main tip. Split plan of record: comments 5993969784 / 5994039786 / 5994053776 on [Tracking] guarded-write chain (epic 1375): rules, port sources, chain-level rows #1991.
• Merge order for the chain: U1 -> U2 -> U3 -> U4 -> U5 -> U8 -> U6 -> U7 -> U9. This unit's merge-order predecessor is U5 (feat(tools): guarded write core under the shared lock (U5, #1375) #1914).

Verification at this head

• DiffViewProvider spec: 159 passed, 2 failed - both failures are donor-junction artifacts of the local harness (DEFAULT_WRITE_DELAY_MS resolves from a stale junctioned dist; CI test and compile are green at this head). guardedWrite spec: 63 passed.
• Red-first + negative controls as described above; ESLint (the CI command, full src) exit 0; Prettier clean; suppressions unchanged.
• CI: the push at this head fixes the lint-budget red at the previous head (nine as any casts in the new test exceeded the file's suppressed count; replaced with bracket notation and documented double casts, violation count back to the suppressed baseline).

Updated this round

• Pushed fe6ff685d -> 36e3f86b: fixes the open CodeRabbit Major (save-lifecycle race at DiffViewProvider.ts:1096). The publish gate released when guardedWrite() settled, but a rejected save keeps working the same bytes and tabs afterwards: the adoption probe reads the disk and the discard cleanup reloads the buffer and closes the session's views. The whole rejected-save decision is now one registered pass (rejectedSaveCleanupInFlight) that revertChanges() waits on, and the discard cleanup records rejectedSaveCleanupHandled so a cancellation arriving after it finalizes the session without applying or saving originalContent (adoption-success rejections keep rollback-last). Regression tests: cancellation during the probe waits for the decision and keeps the rollback last; cancellation after the cleanup skips the rollback and finalizes exactly once. Negative controls: removing the gate await reddens only the probe test; removing the handled-session check reddens only the skip test.
• Per AGENTS.md the two Stryker directives in DiffViewProvider.ts were removed with this touch. Equivalence notes for the mutation gate: (1) the writeKind default "update" - an empty kind dispatches exactly like "update" in guardedWrite (only "edit" and "create" branch distinctly); the default line itself is unchanged, so no mutant enters the changed-line selector. (2) the saveTask && operand in the adoption condition - GuardRejectedError is only thrown by guardedWrite, which is reached only after the !saveTask check has thrown a plain Error, so whenever the operand is evaluated saveTask is provably non-null; the moved condition lines are killed by the existing adoption-rejection tests (non-guard failure with matching disk, dirty buffer with matching disk, external-writer match).
• Earlier round: 6485f0636 -> fe6ff685d lint-budget fix for the spec casts (no behavior change); description rewritten for human reviewers and linked to issue 2023.

Get in Touch

Reviewer and maintainer questions are welcome as comments on this PR or on #1991.

Re-review round (5a04426)

Cleared the three re-review threads:

1. Typed tests - the moved diagnostic/scroll blocks use bracket
   notation for private members and documented structural doubles
   (as unknown as vscode.TextEditor / TextDocument); the spec's
   no-explicit-any suppression count drops 310 -> 285, no additions.
2. Report counting - the seed-descriptor "reported once" test now
   counts matching console.error calls (exactly 1); the weaker second
   toHaveBeenCalledWith and the stale "failed backup" comment are gone.
   Negative control: a duplicated report reddens exactly this test.
3. Symlink backup (security) - with publishOverLink the backup step
   now lstats the target and takes no content backup for a symbolic
   link: no referent bytes are copied, nothing is renamed back over the
   link, and no copy is left beside it. Two regression tests (link: no
   backup, commit lands; regular file: backup kept and cleaned).
   Negative controls: guard disabled / !== true each redden exactly the
   link test; dropping the lstat operand reddens exactly the
   regular-file test.

Equivalent mutant (no suppression, per AGENTS.md): dropping the
publishOverLink operand of the guard is equivalent, because without
publishOverLink the publish target is dereferenced by
resolvePublishTarget and can never be a symbolic link - the lstat
then always reports a non-link.

DACL-over-link refusal (3624e4f)

Re-review follow-up: a captured-DACL publish whose target is a symbolic
link (publishOverLink + lstat symlink + daclDumpPath set) is now
refused BEFORE the commit rename with a new DaclPublishRefusedError
(exported with the DaclCaptureError/DaclRestoreError family, same
shape): the rename would replace the link itself and the DACL restore
would apply the saved ACEs to the wrong object. The error states the
target is unchanged; neither /restore nor the restrictive fallback
runs. DaclCaptureError does not fit (capture succeeded);
DaclRestoreError does not (no rollback occurred).

Regression test + negative controls: refusal removed -> exactly the new
test reddens; refusal unconditional -> exactly the no-DACL linux
symlink test reddens (that path still publishes with no backup).

The refusal also updates safeWriteJson.lockKey.spec.ts (f23e49f):
its two unscoped symlink publishes previously pinned the old win32
publish-over-link-with-ACL behavior; they now branch on
process.platform (win32: DaclPublishRefusedError, locks still
serialized, /save only, link bytes untouched; linux: original
assertions unchanged).

Scope registration: the safeWriteJson carrier on this unit

The confinement, symlink lock-selection and platform-DACL expectations
in src/utils/safeWriteJson.ts and its tests are deliberate on this
unit, not scope drift. The guarded-write chain's unit map - tracking
issue 1991 - assigns this carrier here: its section "Port source:
folding a lock key the way the lock is placed (owed on U8, PR 1916)"
records the lock-key folding derived from U6 (PR 1915, commit
5131bc0) into U8 (this PR, commit 19aec00), including the owed
mixed-ancestor acceptance test; and the ownership ruling in the same
issue keeps primitive-level ports on the unit whose branch carries
that version of the primitive ("Inherited by any unit whose branch
carries this version of the primitive - U1 and U8 among them").
Removing the carrier from this PR would strand the port the tracking
issue owes on U8.

Walkthrough rows 1-4 (c6b3e86)

1. Out of Scope - sharpened pre-emption. The proposed resolution
("move the safeWriteJson changes to the file-safety unit") would
break the owed-port chain, not restore scope: the safeWriteJson
lock-key folding ON THIS BRANCH IS the port - tracking issue 1991,
section "Port source: folding a lock key the way the lock is placed
(owed on U8, PR 1916)", records it derived from U6 (PR 1915, commit
5131bc0) into U8 (this PR, commit 19aec00), with the owed
mixed-ancestor acceptance test tracked there. The file-safety unit
(U5, PR 1914) is the port SOURCE and merges before this unit in the
declared order; moving this carrier out of U8 strands the port the
tracking issue owes on U8 and deletes the derivation record's target.
Scope is deliberate per the unit map; no move.

2. Security Boundaries - standing TOCTOU window, registered. The
resolve/lock/version-check alias race in replaceIfVersion is the
standing confinement-primitive window, not a defect introduced here:
the check and the publish already run inside ONE acquisition of the
shared advisory lock keyed by resolveLockKey (a symlink alias and
its referent share one lock), and the primitive's docstring states the
atomicity boundary and the no-lock writer explicitly. Closing the
resolve-to-lock alias swap is the directory-identity pinning that is
U5's (PR 1914) content, which merges BEFORE this unit in the declared
order - the row resolves at merge time. Same family as the TOCTOU row
registered on PR 1918 (see record 6106959976 on 1408) and the
primitive-level ruling in tracking issue 1991.

3. Regression Evidence - fixed in c6b3e86. New spec
core/task/__tests__/Task.observationRegistry.spec.ts constructs two
REAL Task instances (same construction path as
Task.dispose.test.ts, startTask: false) and asserts both expose
ObservationRegistry, are NOT the same object, and that an
observation recorded on one is absent from the other. Negative
control: swapping the field initializer for a module-shared registry
reddens exactly this test (1 failed | 13 passed across the two
registry specs). The existing unit spec is untouched. 14/14 green,
eslint/prettier/tsc clean.

4. Lifecycle warning - registered, HOLD family. The
open()/reset() double-await race is the teardown-waiter family
recorded in tracking issue 1991, record 8 ("U9 teardown: the waiter
cannot tell whether the pass it waited for succeeded (PR 1917 / PR
1918 - fix once and port)"). The fix belongs to the U9 teardown
content and is owed there once; the carried copies on this unit
resolve at merge time in the declared order. Not a U8-level defect
per the same primitive-level ruling.

Preview-withdrawal port (82cfbf2)

The sibling unit's Major finding (preview observation surviving on the
non-edit exit paths) applies identically to the DiffViewProvider copy
this unit owns, so the fix is ported here as well (this unit merges
first): open() records the token its preview wrote;
performFinalReset() - the single execution point behind reset() -
forgets the entry while it still holds that token and the preview's
incomplete flag. Trigger-sequence test + two survival tests ported;
negative control: disabling the withdrawal reddens exactly the trigger
test. 166/166 green.

Ownership-based withdrawal (a00c5b7)

The withdrawal now tracks the exact entry object the preview wrote
({ path, entry } identity) instead of version+completeness: a model
partial read while the diff is open replaces the entry with a fresh
object at the same token and flag, and survives reset(). Regression
test + two NCs (each exactly one red). 167/167 green. Ported to the
sibling unit's identical copy (14f8634).

Re-review responses (error-fix re-review wave)

• Publish token surfaced from guardedWrite (Major Data-Integrity): the
  guard now resolves with the token it read back under its lock;
  saveChanges() records it as the rollback baseline and the comparison
  prefers it over the create placeholder, so a session's own publish is never
  mistaken for a peer write and a peer write is never silently overwritten.
  New end-to-end spec case (real save -> peer stat change -> rollback
  conflict); the two chain tests that pinned the old void contract now assert
  the returned token.
• Per-session baseline (Major Data-Integrity): open() and
  performFinalReset() clear the rollback baseline with the other session
  fields; a reused provider cannot inherit the previous session's token and
  report a false conflict. Two regression tests added.
• Conflict path finalizes the session (Minor Stability): a rejected
  rollback restores the evicted preview tabs and claims the finalization
  before the conflict propagates; the peer-publish test now asserts the
  session is finalized after the rejection.

Rollback publishes through a version-bound compare-and-swap (4e4cf7f)

The Persistence Integrity row: the existing-file rollback checked the
disk token and then applied the buffer through VS Code's save path - the
check was not bound to the publish, so a peer write landing after the
check was silently overwritten by originalContent. The rollback now
publishes the original bytes through replaceIfVersion: the version
check and the publish happen inside one acquisition of the shared
advisory lock, with lastWrittenToken as the expected version. A CAS
rejection preserves the peer's bytes and raises RollbackConflictError
naming the disk token it found; on success the buffer is reconciled by
reverting onto the published bytes instead of saving over them. Where
either side of the comparison is missing (never published, or no disk
version evidence) there is nothing to bind the publish to and the save
fallback remains, documented as such.

The create branch no longer saves the dirty diff buffer before the
delete - that save published the cancelled edit's content to disk for a
moment, unbound to the placeholder check - and unlinks only after the
placeholder identity/token check inside the lock.

Tests: the barrier test pauses the rollback's version read, lands a peer
publish inside the pause, and verifies the publish rejects with the
peer's bytes intact and the buffer untouched; a new test pins that the
create rollback deletes without saving first. Negative controls:
replacing the CAS with an unguarded publish reddens exactly the four
tests that pin the rejection contract; removing the buffer reconcile and
re-adding the pre-delete save each redden exactly one test. 174/174
green at this head.

Rollback evidence contract (096abc2)

The 05:51Z Persistence Integrity row implemented in full: every existing-file rollback publishes the original bytes through the atomic compare-and-swap; when the session baseline or the current disk version is unavailable the rollback preserves the disk state and raises RollbackConflictError instead of saving (the unguarded updatedDocument.save() fallback is removed); the create rollback unlinks only on a proven token match under the file lock - unknown ownership (identity-only or failed stat-bracketed open) or a mismatch preserves the path and the created directories and reports the conflict. The buffer is reconciled by reverting onto the CAS-published bytes. Tests: missing-baseline and missing-disk-evidence rollbacks, identity-only create peer replacement, plus the tab-close/ordering tests updated to the revert contract. 177/177 green; three negative controls pin the contract (evidence gate, create ownership, revert-vs-save).

The Security Boundaries alias row remains the registered ApplyDiffTool alias family (lands once at the publish-primitive owner; precedent record 6106959976, issue #1408).

Allowlist binding to the retained target (bf09481)

The 08:21Z Security Boundaries row implemented in full: replaceIfVersion and createIfAbsent take an authorizeTarget callback run under the lock on the RETAINED publish target before the version comparison and the publish; the guarded-write entry re-runs rooIgnoreController.validateAccess on the retained target, so a retargeted symlink or hard-link alias cannot receive approved bytes in an ignored/external file. Both identity locks (referent key + alias-entry key via resolveLinkPathLockKey) are taken in the shared sorted order when distinct. Tests: denial before version comparison, retained-target binding, end-to-end alias-passes/referent-ignored rejection, sorted dual-key acquisition, single-lock fold; four negative controls pin each element. 62/62 guardedWrite, 239/239 with the diff-view suites. The cited retained-target block exists only in this unit's copy; no U-family port applies.

The Regression Evidence row (safeWriteJson multi-lock cleanup test) remains carrier content: the test belongs to the safeWriteJson owner unit (scope registration above).