[Tracking] guarded-write chain (epic 1375): rules, port sources, chain-level rows

Part of the split-plan bookkeeping for the guarded-write (epic 1375, units U1 to U9) chain.

What belongs here

chain-level rules and design decisions that no single unit owns
port sources and the per-unit derivation notes for content that is ported rather than rebased
chain-level review rows that CodeRabbit raises against a unit but whose cause spans units
units U1 to U9 currently have no per-unit issue in this repository, so until one exists a unit-scope note also belongs here and must name its unit in the first line

What does not belong here

unit-scope defects: those go on that unit's own issue
anything that requires a maintainer decision: raise it in the PR thread instead

Writing rules for this issue

no @mention in any comment here: mentions notify people who are not subscribed to this issue
do not reference a fork-repo issue number here; a bare number resolves against this repository and silently points at something unrelated
when recording an upstream pull request, write it without a leading hash (for example upstream PR 1066)
prefer editing an existing comment over posting a new one: edits notify nobody and recompute cross-references

Chain-level row: a rollback that persists unapproved streamed content (defect class, not one call site).

Unit: U8 (PR 1916) of this chain - no unit issue exists for this chain yet, so this note is labelled by unit on the first line.

The class: a rollback path awaits workspace.applyEdit() or TextDocument.save(), discards the boolean, and continues as if the operation succeeded. When the editor refuses the restore, the buffer still holds content the user never approved and the following save writes it to an existing file, while the caller reports a completed rollback.

Acceptance for any unit on this chain that carries the row: a refused restore must fail the rollback and must not be followed by a save; a save that resolves false must propagate; a test must show the user does not receive a successful rollback. Fix it in the callee so every caller inherits the contract, rather than adding another inline check at each call site.

Reference: tracking comment 6094179445 records the primitive ownership and the semantic baseline for the same class on the split chain; the shape shipped there is commit 9c54765 on PR 1930.

Unit U6 (PR 1915) - chain-level defect in the shared write primitive, inherited by every unit that carries it.

A confined publication did not pin the directories the write itself created. The caller can only record the identity of directories that already exist, and a recursive mkdir reports nothing it made, so the pre-commit re-check walked a list that omitted exactly the components that appeared during the write. A local process that replaces one of those with a link to outside the scope sends the commit somewhere the confinement decision never made, while every recorded identity still matches.

Fixed on U6 in commit 3289a72: after the mkdir the primitive records the identity of each directory it created and the re-check walks the recorded list plus those; the stat-with-mapping is shared by both call sites so no second branch set appears.

Inherited by any unit whose branch carries this version of the primitive - U1 and U8 among them. Ports are owed, not copied: each unit's call shape differs, so a port needs its own red-first test and its own negative controls. Carry a port when that unit is pushed for some other legitimate reason; do not push a unit solely to deliver it.

Acceptance for a port: (1) a confined write whose parent chain is created by that write, with one component swapped for an out-of-scope link after the mkdir, fails with the ancestor-replaced error and renames nothing; (2) the same shape without the swap publishes normally; (3) a swap of a pre-existing ancestor still fails. Negative controls, one per half of the fix: skipping the recording turns test 1 red, and recording without feeding the list to the re-check also turns it red.

Port source: folding a lock key the way the lock is placed.

Source: U6 (PR 1915) commit 5131bc0. Derived into U8 (PR 1916) commit 19aec00 - not byte-identical, because U8 computed the same-identity comparison inline while U6 has it as a helper.

The rule, which is what a port must reproduce rather than the code shape: the advisory lock is the entry <path>.lock beside the file, so a key must be folded the way that entry is named. On Windows the filesystem folds two spellings of one directory entry twice over - case anywhere, and short (8.3) names inside a component, which is how a CI agent hands out its runner profile directory - while a string comparison folds only case. So the fold canonicalises the deepest EXISTING ancestor and appends the segments below it, case-folded on Windows, and the walk starts at the parent, never at the file: resolving the final component through a symlink folds a link together with its referent, and those are two different lock entries.

Owed on U8: the mixed-ancestor acceptance test (an existing ancestor spelled short with a tail that does not exist yet) has not been derived there, because in U8 the same-identity comparison is only consulted when publishing over a link. The test must be re-derived against that shape, not copied.

Verification limit recorded on the U8 commit: the four windows failures that motivated the port do not reproduce on a host whose temp directory has no short-name ancestor, so only CI exercises them.

Unit U6 (PR 1915) - owed change: stop accepting a bare staging path.

The text writer still accepts a caller-supplied staging path by prefix alone, for compatibility, and the wording says it is checked rather than trusted. Verified at the head of U6: no production caller passes a bare path - the JSON writer takes the handle from the staging-file API and hands the handle down. So the compatibility acceptance has no user, and it keeps a bypass in the primitive: a prefix check on a path a caller invented is not the opaque capability the design relies on, because the caller can name a path it did not create.

Acceptance: (1) a bare path is refused with an error that names the handle API; (2) the handle path still works unchanged; (3) three assertions that the handle cannot be forged by naming a path - a path this call did not create, a link at the staging location, and the target itself.

Negative control: restore the prefix acceptance and drop the handle identity check, and the unforgeability assertions go red.

Chain-level registrations from the frozen fork tracking item

1. A refused edit during rollback is treated as a no-op and the streamed document is still
saved. Units affected: the unit that owns PR 1916 in this chain, and the same primitive as
consumed by PR 1921, PR 1929 and PR 1932 in the other two chains.

Acceptance: restorePreStreamBuffer() throws when applyEdit() returns false; document.save()
and the tab close are not called; the failure reaches the layer that reports it (revertChanges
-> revertDiffChangesBeforeReset -> rollbackError); a test asserts the user does not receive a
"successful rollback". For PR 1932 additionally: the discardFileTab() shape (restore and save
hoisted above the tab loop) must stay intact, and prettier whitespace changes do not affect it.

Two facts that are different and must not be merged into one attribution: the shared primitive is
DiffViewProvider.restorePreStreamBuffer() / saveBufferClean(), whose primitive owner is PR
1930 (commit e41ebe0), with port direction 1930 -> 1916 / 1921 / 1929 / 1932; the semantic
baseline is PR 1928 discardUnapprovedStream() and its editorFailure channel, i.e. a false
return from applyEdit() means the rollback failed. The baseline was already broadcast, so it is
not a new rule. Recorded while the old home was frozen as tracking item 6093818950.

This is the same defect class as the rollback fix in 9c54765: fixing one call site does not fix
the class. The sites were registered rather than swept to keep each row's scope honest. When any
of them is fixed, reuse the existing callee-owned contract in the shape above instead of adding
another inline check.

2. Port owed: a confined write must pin the directories it creates before it commits.
A confined write did not pin the directories it created, so a component swapped for an
out-of-scope link between the mkdir and the commit escaped confinement. Fixed in the unit that
owns PR 1928-era work on the shared primitive src/services/file-safety/safeWriteText.ts
(commit "pin the directories a confined write creates before it commits"). Acceptance: swap after
mkdir raises AncestorReplacedError and no rename happens; no swap publishes; a swap of an
existing ancestor still fails. Negative controls: skipping the recording reddens it; recording
without feeding the re-check also reddens it. Port owed to the units behind U6 in this chain
(re-derive per unit, not byte-identical) - carry it whenever that unit is legitimately pushed for
another reason.

Unit: U9 (DiffViewProvider teardown), carried in PR 1917 and PR 1918 - fix once and port.

runTeardown cannot tell a waiter whether the pass it waited for actually succeeded.

The tracked promise is resolved by release() in the finally block, so it settles successfully whether the owner's cleanup or finalize returned or threw. The waiter's await this.teardownInFlight therefore resolves in both cases and it returns false - "not my pass" - with no signal that the pass it just waited through left the document mid-teardown. The owner's own caller does see the throw, so the failure is not swallowed outright; what is lost is the waiter's ability to react, which is exactly the case a cancellation lands in: the second caller arrives, waits, and walks away believing the buffer was reverted and the tabs closed.

The two structural halves of the original note are already in place and should not be re-derived: finalize now runs inside the tracked promise, so a cancellation arriving after the callback but before the restore and reset no longer starts a second full pass, and the gate is held until both settle.

Acceptance. A test where the owner's cleanup rejects and the waiter is already parked: the waiter must either see the failure or run the recovery itself - asserting only that the owner rejects is not enough, because that half already holds. Negative control: make the waiter's path indistinguishable again (resolve the gate unconditionally and drop whatever channel carries the outcome) and that test must fail.

Two notes closed before they were ever registered here, so nobody re-derives them - both were queued against this chain and are fixed at the current head:

ApplyPatchTool refreshing a stale observation: the hunk read now observes only when there is no prior entry or the prior version equals the token just read, which is the same stale policy ApplyDiffTool applies, and the comment at the site states why an older observation must not be refreshed.
PostCommitDurabilityError leaving the backup behind: the delete path catches it and unlinks error.backupPath before rethrowing.

Both verified by reading the two files at the head of the U9 branch on 2026-10-10, not from the queue entry.

Unit: U3 (PR 1912) of this chain - the split of PR 1833 under epic 1375. Two records, neither a code defect; registered here because this chain has no per-unit issue.

1. The Linked Issues check reports an error because the branch is based directly on the repository default branch, not on a preceding unit. CodeRabbit therefore sees the whole stack (13 files, roughly 3.6k added lines, 39 commits) instead of one unit's delta, and its linked-issues validation reports an error for a pull request whose diff spans requirements owned by other units. Handling chosen: the pull request body carries a requirement-to-unit table so a reviewer can tell which requirement this unit serves. Acceptance: when the earlier units of this chain merge, the row disappears without any code change - if it does not, the cause is something else and this record is wrong.

2. A review finding overstated the effect of a cleanup on a committed publication. The finding said the cleanup path can delete the already-committed target. On the POSIX path it cannot: the staged name is gone once the rename lands, so the unlink reaches a name that no longer exists and the only observable effect was a false leftover warning. The shape criticism was still correct - a cleanup that reaches for a path it may not own needs its ownership stated, not inferred - so the guard stayed. Acceptance: none required. Recorded so the next reviewer of a sibling unit does not re-derive the overstated consequence, and so a future row about the same shape is judged on the shape, not on the effect that turned out not to exist.

Both records came out of the round that produced PR 1912; nothing here needs a maintainer decision.

Unit: U4 (PR 1913) — ownership registration for the shared primitive src/services/file-safety/safeWriteText.ts

A read-only check of this issue found no existing ownership entry for the safeWriteText primitive, so the three pre-merge rows raised on PR 1913 that land on that primitive had no owner. Registering the ownership here so the same block of code is not fixed twice.

Rows in question, as raised on PR 1913 at head c36fd64:

Security Boundaries — a symlink race requires confineTo to be carried into safeWriteText and enforced at the final publish action.
Persistence Integrity — the committed semantics at safeWriteText.ts:509-511.
Lifecycle Resource Cleanup — the swallowed backup unlink at safeWriteText.ts:553-558.

Ruling: these are primitive-level, not U4-level. The primitive already has one fixed defect in this chain (the pin-before-commit escape, fixed in U6, with the port owed to U1 and U8 and re-derived per unit rather than copied byte for byte), so a second unit editing the same block independently is exactly the double fix the chain rule forbids.

Ownership: the primitive fix belongs to the earliest unit in the declared merge order that can carry it, and ports forward from there. U4 does not fix these three rows; it references this comment and resolves its threads.

Acceptance for whoever takes the primitive fix:

symlink race: swap an ancestor for an out-of-scope link between the last check and the publish action -> AncestorReplacedError and no rename; no swap -> publishes.
committed semantics: a failure after the commit point must not be reported as a rollback, and must not leave the caller believing the previous content is back.
swallowed backup unlink: ENOENT and ENOTEMPTY stay silent, every other error is surfaced, and the loop continues.

Negative control shape per row: mutate the exact production line, expect exactly the tests that name that behaviour to go red, and run the same mutant against the pre-fix spec to prove the branch was uncovered. A mutant that reddens far more tests than the target means the mutant itself is wrong.

Note for the reader: an open-thread count on a PR and the number of rows in that PR's pre-merge table are two different measurements and must not be used interchangeably.

Unit U7 (pull request 1405) - owed change: the symlinked-ancestor walk is not bounded by the authorized scope.

Single record for this defect. An earlier duplicate carried the same content under another comment id; that duplicate was deleted, so this comment holds the record rather than pointing at a comment that no longer exists.

Defect. The ancestor refusal in the JSON writer walks every directory above the target and refuses the write if any of them is a symlink. The walk has no lower bound: it keeps going past the root the caller actually authorized, so a symlink anywhere above that root - a linked workspace root, a linked temporary directory on a developer machine or a CI agent profile - refuses writes that the caller opted into for a reason. The refusal exists to stop a link redirecting a payload out of the scope; applied above the authorized root it stops the write without protecting anything, because nothing above that root is part of the containment decision.

Acceptance.

A write whose ancestor chain contains a symlink strictly above the authorized scope root publishes normally, with the payload at the named path.
A symlink strictly inside the authorized scope still refuses, with the same error the refusal gives today.
The credential-carrying case keeps its current behaviour: a link at the final component is an error, and a link inside the scope is an error.
The walk stops at the scope root rather than at the filesystem root, and the code says which root it stopped at.

Negative control. Red first, before the change: the test for case 1 must fail against the current walk. Then green with the bound in place. Then red again with the load-bearing piece removed - drop the lower bound so the walk runs to the filesystem root again, and case 1 must fail. A second control for the other half: widen the bound so it also ignores links inside the scope, and case 2 must fail. Both mutants restored byte-exact.

Notes for whoever takes it. Heavy lift, deliberately not bundled with the comment and documentation changes already landed on that unit. Not tied to any commit: carry it on the unit that legitimately pushes this primitive next, re-derived against that unit's call shape rather than copied. Related chain facts on this issue: the lock key and the publication destination are different paths, and the fold of a lock key has to match the way the lock entry is named.

Fail-closed DACL restore must reach the old-generation copies of the publish primitive

Defect: in the old-generation copies of src/services/file-safety/safeWriteText.ts the DACL restore runs after the commit rename and, on failure, only records a flag and warns, so a replacement is published even though the captured DACL could not be restored. The caller gets no signal: the primitive returns success and the only post-commit throw is PublishNotDurableError. A call-site preflight would be a second mechanism duplicating the primitive's own capability logic, and requesting a backup does not fail closed because the replacement is still published.

Ownership: the chain already carries the correct shape. The earliest unit in merge order implements a typed DaclRestoreError thrown after the commit rename when the restore fails, plus a fail-closed inspection error before the commit, and its own pre-merge row passes with that shape. The old-generation branches carry independently re-derived copies that predate it, so the fix is a convergence port of one shape, not a new design.

Acceptance, not bound to a commit:

A restore failure after the commit rename reaches the caller as a typed error, and no caller can observe success for a publish whose DACL was not restored.
The branch converges on the chain's single shape rather than adding a second mechanism in the caller.
The direct-save path surfaces the same failure instead of swallowing it.
Backup retention on restore failure does not regress, and capture stays fail-closed before the commit.
Every branch carrying its own copy of the block gets the port re-derived for that branch, not 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.

Reference: the PR-level registration on the first affected pull request carries the blame evidence and the same acceptance list.

undefined

Correction after two units fixed this independently: the two copies diverged. The unit that merges earlier in the declared order rejects an empty or whitespace-only declared root with the confinement error naming that root, before the lock key, the lock, directory creation or staging, because the helper's stance is that a failure must surface rather than be decided from a guess about the caller. The later copy currently resolves an empty string to the process working directory instead. The canonical shape is therefore the rejecting one, and the later copy must be re-derived to it when the earlier one lands, since merging the later copy as it stands would silently replace reject semantics with working-directory semantics in the landed default. Until then the later unit stays as it is rather than spending another review cycle, because it cannot merge before the earlier one anyway.

Publish must not follow a swapped ancestor link when a caller asks for symlink refusal

Defect: a caller can ask that a write refuse symlinked ancestors. The refusal walks the ancestors once, before anything is resolved, staged or locked, while the two later checks look only at the final component and publication is path based - the directory creation and the rename both take a path. An ancestor replaced by a link after the walk therefore redirects the commit, so the control the caller asked for is not what the caller gets. The runtime has no renameat equivalent, so this is a design decision in the primitive rather than a local patch: either publish through handles with no-follow semantics, or stop claiming the protection for paths that cannot be proven stable after the check.

Acceptance, not bound to a commit:

Either publication is handle relative and no-follow for the directory creation and the rename, or the control is removed from the credential path so it no longer claims protection it cannot prove.
The chosen boundary is documented on the option, including what it does not cover.
A test swaps an ancestor after the final component check and shows the outcome the option now promises, red before the change.
The shape lands in the earliest unit in the declared merge order and is re-derived per branch when ported forward.
Callers that pass the option keep their existing behaviour for ancestors that do not change under them.

Negative-control shape: one mutant per production call site, each reddening only the test naming that behaviour, plus a run of the new test against the pre-fix production file; restore byte for byte and verify with a hash.

Ownership correction measured at the earliest unit head: the primitive in the earliest unit of the declared merge order has no refusal option, no confinement option, no ancestor walk and no external callers, so the claim this registration narrows is vacuously true there and no change at that head can produce a test that is red first. The earliest unit that actually carries the control is the one whose helper resolves the caller path against a declared scope root and walks ancestors once before anything is staged or locked, and that is where the cheap half belongs - document precisely what the control does not cover, add a test that swaps an ancestor after the final component check and is red before the change, and decide whether the credential path keeps the claim. Handle-relative no-follow publication is not available in this runtime, so the narrowing is the only implementable half. Correct the owner from the earliest unit in merge order to the earliest unit that carries the control, and port forward re-derived per branch.

Carrier correction measured branch by branch: the confinement control is not new to the unit that first documented the window. The unit before it in the declared merge order already carries the identical control - the same scope-root resolution with the missing-ancestor walk, the same pre-lock and in-lock checks in the same order - and the same credential call sites that pass it. The two earliest units carry no such control at all. The documentation narrowing therefore has to be ported to that earlier unit and re-derived: where that copy still decides from a truthiness test, keep the boundary wording and drop the sentence about a declared but empty root, which only exists in the later copy.

Decision request: one canonical write lock or an accepted residual window

Two chain registrations converge on the same end state and neither can be closed inside a single unit. The runtime has no atomic compare-and-swap rename, so a pre-rename verification callback followed by a plain rename cannot be made sufficient by any unit, and a symlink refusal that walks ancestors once cannot be made sufficient while publication is path based. The only shapes that make both claims true are a single lock that every writer in the repository honours, including writers outside the extension host, or an explicit, documented residual window on both controls. A unit-level change cannot choose between those, and a registered row stays red at every head until one of them is chosen, because a citation does not clear a pre-merge row - only code at the head does. This is a request for that choice, with the consequence recorded: until it is made, the units that wire these controls cannot reach an approving review at their own head.

Chain-level row: a shared fixed staging-directory name lets a workspace-shipped symlink redirect the permission repair outside the workspace.

The old-generation copies of the publish primitive name the staging directory with one shared,
predictable name (.file-safety-staging) inside the target directory. mkdirSync with
recursive: true accepts an existing entry of the requested name without creating anything, and
the permission repair uses chmodSync, which follows a symlink. A repository that ships a
directory entry named .file-safety-staging as a symlink to any directory the user owns therefore
has that outside directory set to 0o700 by the first confined write into the containing
directory, and the staged temp file is created in the outside directory before the commit rename.

The chain's single shape is a per-write unique staging-directory name, which removes the hazard by
construction: the entry the write creates, chmods and writes into is one it made. Every unit branch
of the declared merge order (U1 through U9) already carries that shape.

Carriers of the defect. The old-generation copies on pull request 1395 and pull request 1405
still use the shared fixed name. Pull request 1408 carried the convergence port in commit 87ee84e
(raised as inline finding 4240865298 against its head); this note records the port still owed on
the two earlier old-generation branches.

Acceptance for a port, not bound to a commit:

Two self-staged writes into one directory receive different staging names, each of the generated
shape, and each write's chmod and staged file land inside its own directory.
A pre-existing entry named .file-safety-staging - plain directory or symlink - is never
chmod-ed or written into by this layer.
The test is red before the change on that branch.

Negative-control shape. One mutant: revert the staging-directory name to the shared fixed name
and confirm exactly the new ownership test reddens and nothing else; restore byte for byte and
verify with a hash. Ports are owed, not copied: carry a port when that unit is pushed for some
other legitimate reason, and re-derive it against that branch's call shape.

fix: order cancellation against the guarded publish and session restart

test(diffview): keep the open() serialization test inside the lint su…

Unit tracking issues for the fws chain (one per PR, so every PR now has a linked issue in its sidebar): U1 #1910 -> #2016, U2 #1911 -> #2017, U3 #1912 -> #2018, U4 #1913 -> #2019, U5 #1914 -> #2020, U6 #1915 -> #2021, U7 #1918 -> #2022, U8 #1916 -> #2023, U9 #1917 -> #2024.

All nine PR descriptions were rewritten for human reviewers following maintainer feedback on #1910: each now leads with what the PR does and why in plain terms, links its unit tracking issue with a Closes line (U9 additionally closes the epic as the last unit), and keeps the measured verification numbers under Test Procedure / Verification at this head.

Navigation Menu

Repository navigation

[Tracking] guarded-write chain (epic 1375): rules, port sources, chain-level rows #1991

What belongs here

What does not belong here

Writing rules for this issue

Chain-level registrations from the frozen fork tracking item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[Tracking] guarded-write chain (epic 1375): rules, port sources, chain-level rows #1991

Description

What belongs here

What does not belong here

Writing rules for this issue

Activity

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 10, 2026

Chain-level registrations from the frozen fork tracking item

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 10, 2026

easonLiangWorldedtech commented on Oct 11, 2026

easonLiangWorldedtech commented on Oct 11, 2026

easonLiangWorldedtech commented on Oct 11, 2026

easonLiangWorldedtech commented on Oct 11, 2026

easonLiangWorldedtech commented on Oct 12, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions