feat(file-safety): guarded text publish module + specs (A4, #1375) - #1395

The commit is a rename, so the published inode is a new one: it takes this
process's primary group, or the directory's gid when the directory is setgid.
A shared 0o664 file owned by group devs therefore lost group write access for
every member whose primary group is not devs after one direct save, even though
fchmodSync restored the mode - the same regression the mode fix exists to
prevent, so the group has to survive too.

The gid is taken from the same stat call that reads the mode and applied to the
descriptor with fchownSync(fd, -1, gid) before the chmod, because chown can
clear the setgid bits the chmod has just set. It is best effort: the call only
succeeds when this process belongs to that group, which is the case that
matters, and the mode preservation still stands when it does not. Both the
staging branch and the caller-staged branch do it. Windows is skipped: it
carries no POSIX gid to preserve, its identity is the DACL.

…ert the result

- The persistent staging-release failure test captures the staged path from the
  publish call and asserts that both unlink attempts and the warning name that
  exact path, instead of counting attempts and matching a message prefix.
- The real-filesystem residue filter now also matches the Windows DACL dump,
  which is written next to the target and only ever appears on the
  windows-latest run, so a leaked dump can no longer pass a suite whose claim
  is the absence of residue.
- The saveDirectly missing-target test asserts the returned finalContent, so a
  stale result cannot pass on call counts alone.

…o test

The first write makes exactly one /save and one /restore: the 1300 exit tells
the restore it is the missing privilege, so the transient retry must not run.
A greater-than-zero assertion could not tell that apart; letting a privilege
failure take the retry as well now reddens exactly this test.

…er the commit

Convergence port of the chain's first-unit shape into this branch's diverged
safeWriteText copy, so the Security Boundaries row at DiffViewProvider.ts:1174
can clear without adding a second mechanism at the call site.

- safeWriteText: a DACL restore that fails after the commit rename now records
  a typed DaclRestoreError and is raised after the rollback handler, the same
  position PublishNotDurableError already uses: the content is committed, so
  this is not a failure to roll back, and the backup copy - the only artifact
  still carrying the target's original security descriptor - is retained.
  Warn-and-resolve is gone: no caller can observe success for a publish whose
  DACL was not restored. The missing-privilege memo keeps its semantics; the
  first publish that loses its DACL still reports the typed error.
- Capture stays fail-closed before the commit (DaclCaptureError unchanged);
  the capture-abort test now also pins that the commit rename never ran.
- safeWriteJson: DaclRestoreError joins PublishNotDurableError in the
  commit-landed classification, so the landed publish's staged path is not
  treated as a leftover temp file, and the error still rethrows.
- saveDirectly needs no production change: it awaits the primitive directly,
  so the typed error surfaces on the direct-save path; a new spec pins that.

Tests pin the four contract points with negative controls (mutants restored
byte-for-byte): typed error to the caller - 4 tests red under the throw
mutant, 0 red in the pre-fix spec; backup retention on restore failure -
1 red, already pinned pre-fix, no regression; direct-save surfacing - 1 red,
0 red pre-fix; fail-closed capture - 7 red, pinned pre-fix and now.

Local Windows note: icacls /restore exits 1300 on this non-elevated host, so
the two real-icacls integration tests (safeWriteText.integration.spec.ts
"replaces an existing file and leaves no residue", safeWriteJson.test.ts
"should successfully write a new file") now report DaclRestoreError locally
where they previously only warned; CI windows-latest runs elevated and is the
arbiter, as with the first unit's equivalent suite.

…describe the state they claim

Two review findings said the same thing about different tests: an assertion that
counts the ABSENCE of an unlink passes just as well when the artifact was never
created, so neither test proved the fact it named.

The backup-retention test counted unlinks whose path contained ".bak". With no
backup copy at all that count is zero too, so the test passed on a publish that
never made the backup it was claiming to keep. It now captures the destination
passed to the copy call, asserts the copy really copied the target to that
destination, and only then asserts that nothing unlinked that destination.

The safeWriteJson leftover test mocked a DaclRestoreError thrown without the
commit rename having happened, so it described a write that never committed -
where unlinking the staged file would be correct - while claiming to cover the
post-commit case. The mock now performs the rename the real failure arrives
behind, and the test asserts the committed bytes are at the target and the
staged name is gone before asserting the catch path leaves it alone.

Negative controls: skipping the backup copy left the old backup-retention
assertion green and reddens the new one; dropping the DaclRestoreError arm of
the catch classification reddens the leftover test in both shapes; running the
mock without the commit reddens the new post-commit assertions.

…abled

Once a restore exits 1300 the process memo disables the save/restore pair, and
the skip also silenced the condition it was skipping. A later write to an
existing target still renames a staged file over it, and a rename does not hand
the published file the descriptor of the file it replaced, so those writes
replaced the target's access rights, preserved nothing, and resolved as an
ordinary successful save. The module's own contract says the opposite: no caller
may observe success for a publish whose DACL was not restored.

The memo is a cost decision, so it stays a cost decision. Step 2b probes the
target with one access() - no process spawn, so the memo still buys zero icacls
calls per write - and records that a descriptor is about to be replaced with
nothing saved for it. Step 5b turns that into the post-commit error a failed
restore already uses, worded for the condition it reports, before step 6 so the
backup copy that still carries the original descriptor is retained. A brand-new
target is not a loss - it takes a default descriptor either way - so the report
is scoped to writes that replaced something, and a non-ENOENT probe error still
propagates instead of being read as permission to succeed.

The narrowing from the earlier round is untouched: the memo is still set only by
exit 1300, so a transient restore failure still retries and still captures and
restores the DACL on later writes.

Reported through the existing error type rather than a new one on purpose. The
caller-side fact is identical - the content landed and the access rights changed
- and safeWriteJson already classifies DaclRestoreError as a landed commit it
must not unlink, while DiffViewProvider surfaces it. A second class would have
to be added to that classification for no change in what any caller can do.

Chain-level row registered on Org#1991 (comment 6108998842). The staging
directory carried one shared, predictable name, .file-safety-staging. mkdirSync
with recursive:true accepts an existing entry of the requested name without
creating anything, and the permission repair uses chmodSync, which follows a
link. A repository that ships that name as a symlink to a 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 it
before the commit rename; a committed plain directory at that name is repaired
and released by a write that never created it.

The chain's single shape is a per-write unique staging-directory name, which
removes the hazard by construction: the entry a write creates, chmods and writes
into is one it generated. Re-derived for this branch's call shape - the guard,
the abandon-on-reject cleanup and the ownership check stay, because the entry at
a generated name still has to be the one this call made.

The generated name starts with the shared name, so a substring probe reports a
match either way: the new tests compare full names and pin the generated shape
end to end. Negative control - reverting the name to the shared fixed name
reddens exactly the three new tests and nothing else.

…d root

The apply-diff save path approved the lexical tool path and then let
safeWriteText publish wherever the path resolved: a dangling in-workspace
link approved.txt -> /tmp/outside/new/secret.txt passed the in-workspace
allowlist and auto-approval, and the primitive then CREATED the outside
parent (mkdir of the referent directory) and renamed the content there.
The bytes landed outside the workspace the approval displayed.

safeWriteText now takes an optional authorizedRoot. When set, a requested
path that resolves lexically inside the root must also publish inside it:
the retained target (resolvePublishTarget's result, before any mkdir) is
checked against the canonicalized root and the write is refused with
nothing created or written when it escaped. Requested paths that are
themselves outside the root stay unconstrained - the tool layer's own
approval flow governs outside-workspace writes - and a root that cannot
be inspected fails closed (non-ENOENT realpath errors propagate).
DiffViewProvider.saveDirectly passes the task cwd as the root, so the
guarded save path can no longer spend an inside-path approval on an
outside target.

Tests: dangling-link and existing-symlink escapes refused before mkdir
and rename, inside referents published, outside requested paths and the
no-root case unchanged, lexical-root fallback for a not-yet-created root,
EACCES on root inspection propagated. Negative controls: removing the
refusal reddens exactly the three escape tests, dropping the
inside-requested condition reddens exactly the outside-requested test,
removing the ENOENT fallback reddens exactly the fallback test, and
dropping the saveDirectly wiring reddens exactly the four wiring
assertions. 95/96 unit (1 pre-existing skip), 75/75 diff-view; the one
integration failure is the pre-existing win32 icacls/restore host
limitation, identical on the clean tree.

… in review

- _isWithinRoot treated any leading ".." as an escape, so a sibling
  entry legitimately named "..config" was misclassified as outside the
  root: a write whose target carried such a name was refused even
  though it sits inside the root. Only a full ".." segment escapes.
- The inside-request test compared the lexical requested path only
  against the canonicalized root. A workspace opened through a symlink
  (cwd /tmp/ws resolving to /private/tmp/ws) makes every inside request
  look outside, skipping the guard exactly where it matters: a dangling
  link under the symlinked cwd could still publish to /etc/x. The test
  now matches both the canonical and the lexical root for the request
  and the target, so an inside request is inside under either and only
  a target outside both is refused.

Tests: an in-root "..config" target publishes through an inside link;
a symlinked workspace still refuses the dangling-link escape. Negative
controls: reverting either fix reddens exactly its test. 97/98 (1
pre-existing skip).

…r the main refresh (lint gate)

…t boundary and staging

- authorizedRoot bypass (Major): a NEW file under a symlinked parent
  directory escaped the root unchecked - the retained target of a
  not-yet-existing path is lexical, so root/sublink/new.txt looked
  inside while the bytes landed at /outside/new.txt through the
  committed parent symlink. The inside-target test now canonicalizes
  the target's nearest existing ancestor and re-appends the missing
  tail, so the check sees where the bytes land, not where the name
  sits. Regression test: dangling write under a parent link out of the
  root is refused before mkdir. Negative control: reverting to the
  lexical target reddens exactly that test.
- staging recovery leak (Minor): the ENOENT reopen recovery called
  _stagingDir() again, which generates a FRESH random name - the
  recreated directory did not contain tempPath, so the reopen failed
  again on a real filesystem and the new directory was orphaned. The
  recovery now recreates the same stagingDir path and verifies it is a
  directory before reopening. The recovery test now pins that the
  recreated path equals the temp file's parent; reverting the fix
  reddens exactly that test.
- suite isolation (Minor): the two top-level suites without a
  beforeEach inherited mock defaults from the main suite's beforeEach,
  so a filtered run (-t) crashed with TypeError. The defaults moved to
  a shared _restoreFsDefaults() called by every suite; both filtered
  runs now pass standalone and fail without the beforeEach.

… saveDirectly

saveDirectly ran createDirectoriesForFile before safeWriteText's
authorizedRoot check, and createDirectoriesForFile follows symlinks: an
approved inside path whose intermediate link points outside the workspace
mutated the outside filesystem before the publish was rejected. Export the
canonical ancestor check from safeWriteText and run it first, so the
allowlist decision has no filesystem side effects (Security Boundaries
row).

…d lock the referent

Review follow-ups: (1) a host whose icacls /restore exits 1300 now publishes successfully with one process warning instead of throwing DaclRestoreError after the rename on every overwrite; non-privilege restore failures still throw; (2) DiffViewProvider.saveDirectly treats PublishNotDurableError and DaclRestoreError as a completed save with a warning so the tool layer does not retry content already on disk; (3) file locks canonicalise through resolveRealPath (nearest existing ancestor, case preserved) so TaskHistoryStore and safeWriteJson serialize on the same lock through a symlinked ancestor; (4) safeWriteText reuses WorkspacePathResolver.resolveRealPath via a caseFold option instead of a private copy.

Review follow-up: the durability fsyncs (staged file and parent directory) now use the async fs.fsync form instead of fsyncSync, so the ~7 ms syscall no longer blocks the extension host event loop; this is the same syscall FileHandle.sync() uses internally. fsync failures still reject before the rename, and ordering assertions still prove file-fsync-before-close and directory-fsync-after-rename. Tests 98 passed.

Review follow-up: no production caller passes backup: true, and the option added a copy/retain/cleanup path with its own ~20 tests. Remove it until a caller needs the recovery copy: the primitive keeps the single atomic rename, staging fsync, DACL capture/restore and the dump cleanup. Tests and comments are pruned accordingly (the DACL retry, lost-DACL and ordering coverage stays). Related suites 200 passed, 8 skipped.

Windows CI returns an 8.3 short os.tmpdir() path, but resolveRealPath returns the long form, so the lock-path assertions now compare against the realpath of the existing ancestor instead of path.resolve. 3 tests passed locally.

…s for cache state

Review follow-up: add isPublishCommittedError(error) for PublishNotDurableError/DaclRestoreError and use it where a consumer keeps state derived from the write. CacheManager.clearCacheFile now clears the in-memory hashes when the empty cache committed but its durability/ACL verification failed (the file on disk is the new one); DiffViewProvider reuses the shared guard. safeWriteText keeps the exact-commit failure contract. Adds slice tests: guard classification, staging mkdirSync EACCES/ENOSPC without open/rename, and the committed-clear cache path. Related suites 205 passed, 8 skipped.

Review follow-ups: the alias/referent lock test now asserts the ELOCKED failure kind (lock contention, not an unrelated lockfile error) and releases both the holder and, should it ever acquire, the contending lock from a finally block so a failing assertion cannot strand the lock. The committed-clear cache test keeps the PublishNotDurableError instance and asserts the warning receives the exact message prefix plus that instance. fileLock + cache-manager specs 15 passed.

withFileLock canonicalised the path before handing it to the operation, so a
caller whose own path arrives through an 8.3 alias or a junctioned ancestor ran
its body against a different string for the same file. The lock key and the
artefact stay on resolveFileLockPath - an alias and its referent still contend -
and only the path the operation sees changes, because the delete path compares
what it receives against names it built from its own string: the backup guard and
the leftover-prefix match in jsonFileRecovery are string comparisons, which is
why TaskHistoryStore.deleteSemantics.spec.ts loses three rows under an aliased
TEMP (two at :145 opening the caller-spelled history file, one at :255 on batch-b)
and passes 10/10 under a plain TEMP with the same code.

The added row in fileLock.spec.ts pins the caller's spelling contract for both
spellings of one referent. Its fixture is a junction (a directory symlink off
win32), so it discriminates spelling divergence generally rather than the 8.3
case specifically: it also reddens under a plain TEMP on the head implementation,
and 3-failed-under-alias against 10-passed-under-plain on the deletion spec is the
pair that isolates the 8.3 alias reason. The identity-backed key used on the
2368 lane is deliberately not ported here: resolveFileLockPath already returns one
string for both spellings on this branch, stat identity is one referent across
three spellings, and the alias/referent contention row passes under an aliased
TEMP with and without this change, so an identity key here would be code no test
in this lane can turn red.

Measured on Windows with node v22.23.4 (.tmp-node22), vitest.core.config.ts for
core/** and vitest.unit.config.ts for utils/** and services/**, run from src, with
TEMP pointing at a scratch root whose 8.3 name was read from the parent directory
listing: head implementation - deleteSemantics 3 failed | 7 passed under alias,
10 passed under plain; with the fix - 10 passed under both, fileLock.spec.ts 4
passed under both, jsonFileRecovery 10 passed, safeWriteJson 53 passed, whole
core/task-persistence 208 passed, whole utils 869 passed | 1 skipped, whole
services/file-safety 99 passed | 1 skipped. Swapped in main's fileLock blob
49807ea on this tree: deleteSemantics 10 passed under both, safeWriteJson 53,
jsonFileRecovery 10. This file dropped onto main at 5319fe9: deleteSemantics
10 passed under both, safeWriteJson 53 under both, jsonFileRecovery 10 under
alias. Hosted eslint from src exits 0, per-file eslint --prune-suppressions exits
0 on both files with src/eslint-suppressions.json restored byte-for-byte, repo
root prettier --check . exits 0, and tsc --noEmit from src reports no errors.

…ew rows

Every row this commit acts on is a spec row: no production line moves, so the
mutation gate's inputs are unchanged (the gate's own selectFromGit still counts
359 executable changed lines of the 500 cap against base 73931fe).

Rows answered by code (review comment ids, in thread order):
- 4510971744 DiffViewProvider.spec.ts: pin the target paths handed to the
  authorization preflight, which otherwise rejects whatever it is given.
- 4510971771 hold the staged-file fsync callback open and assert closeSync and
  the commit rename are waiting, then release it and assert both run.
- 4510971780 hold fs.rename pending and assert only /save has run, then resolve
  it and assert /restore follows: the DACL restore cannot precede the commit.
- 4510971785 add the split-character short write ('é' is 1 character, 2 bytes) so
  a loop bounded by content.length cannot pass.
- 4510971793 assert the last write precedes the first fsync, not just its arguments.
- 4510971800 assert fs.rename never ran on both the writeSync- and fsync-failure paths.
- 4510971809 inject fsync failures through the callback for the staged file (pre-
  commit, nothing published) and the parent directory (post-commit, not durable).
- 4510971818 hold each parent-directory preflight pending and assert mkdirSync,
  openSync and rename never started, before and after the rejection.
- 4510971827 assert the rollback and cleanup never unlink the target's own path.
- 4510971833 judge the staged "w" open apart from the post-commit directory open.
- 4510971849 race the lock-entered signal with the contending acquisition so a
  failed acquisition surfaces as its own error instead of a 15s test timeout.

Measured on node v22.23.4 in this worktree: services lane 104 passed / 1 skipped
(POSIX-only case) for safeWriteText.spec.ts; misc lane 82 passed for fileLock.spec
and DiffViewProvider.spec.ts; mapped tsc -p src/tsconfig.json --noEmit exit 0;
eslint --prune-suppressions --max-warnings=0 exit 0 for each file with
src/eslint-suppressions.json restored byte-identically.

…om safeWriteText

The replay onto main applied the cache-state commit's diff against main's copy of src/services/code-index/cache-manager.ts, where the import line is still the single combined import from utils/safeWriteJson. In the branch the diff was authored against, that line had already been rewritten by the branch's own merge of upstream main, commit 1f8ae7f, which a replay drops by design because rebase replays non-merge commits only. The result was a combined import that asks utils/safeWriteJson for isPublishCommittedError, while this unit moved that symbol into services/file-safety/safeWriteText.

Measured rather than inferred: main's blob for this path is 7807aac and the merge base's is the same 7807aac, so main carried no change here at all, while the unit's head carries b1062de. Under the rule that trunk's blob is taken where the unit only carried trunk content and the unit's own content is kept where main does not carry it, this path belongs to the unit, so the file is restored by taking the unit head's blob byte for byte rather than by writing the line again.

After this the replayed tree is byte-identical to the tree the textual merge produced, 48b78e8, over 11 files differing from main and no others.

…and staging no-ops

Four review findings on this unit's own specs, each verified against the code at daee64d before being changed:

- DiffViewProvider.spec.ts: no test handed saveDirectly an ordinary publish failure, so a catch that swallowed every error would have passed the suite. The new case rejects with a plain Error and asserts the rejection propagates, the document is not opened, and no committed-write warning is logged.
- safeWriteText.spec.ts line 649: the comment said the restore is retried once while the assertion checked greater than 0, which is the shape that lets a removed retry pass. Production retries a non-privilege restore failure exactly once, so the count is now asserted exactly at 2.
- safeWriteText.spec.ts foreign-uid staging test: the comment claimed nothing is staged and nothing is published but no assertion backed it, so openSync, rename and rmdirSync are now asserted not to have been called.
- safeWriteText.spec.ts parent-directory-creation block: its beforeEach hand-copied the shared _restoreFsDefaults helper, had already lost the fs.lstat default the helper sets and repeated two mocks the helper makes, while four sibling top-level suites call the helper. It now calls the helper and keeps only the two defaults specific to that block, and the describe title no longer cites safeWriteText.ts:288-290, which at this head is the doc comment of _restoreDaclWindows while the parent-directory mkdir and access live at 471-488.

Negative controls, all run on this machine against the production code the specs pin: disabling the single restore retry turns the exact-count assertion red with expected 1 to be 2; widening the DiffViewProvider catch to swallow ordinary publish failures turns the new case red with promise resolved instead of rejecting; and the foreign-uid case, which is skipped on win32 because production reads process.getuid at safeWriteText.ts:192 and this host has none, was run once with the skip removed and getuid stubbed, where it passes, and once more with the ownership throw neutered, where it fails. Baseline over both spec files: 2 files passed, 183 passed and 1 skipped. ESLint over both changed files exits 0 with no findings, and eslint-suppressions.json is untouched.

…it performs

The review asked whether a host that cannot restore DACLs should keep throwing
DaclRestoreError on every later overwrite once the rename has landed. Measured at
this head it does not. DaclRestoreError is constructed at exactly one site, in the
branch that handles restore failures which are not the missing-privilege limit, and
that whole step runs only when a dump was saved. Once the memo is set the save is
skipped, so no later overwrite reaches the throw at all. The first privilege-limited
restore already degrades the way the question asks: one warning is logged, the
publish resolves, and a case in this spec asserts both halves of that. The defect was
never a throw that should have been a warning.

What was wrong is the comment above the memo. It claimed skipping "must not change
what a publish reports" and that a later call "reports the condition instead of
resolving quietly". Neither holds: the save step is skipped too, so nothing is
recorded or reported per call, and the spec asserts that no second warning is
emitted. The comment now states the design, including that a later write on such a
host does not throw for this reason because the restore step requires a saved dump.

The icacls command case was platform-skipped, so no CI runner ever asserted the two
command shapes, and its title promised a copy of the target DACL onto the staging
file while the case asserts command names, arguments and a call count. The skip is
dropped, because the step is driven by the platform option and every neighbouring
case in the same describe mocks the platform, and the title now names what the
assertions check.

The durability report had the mirror-image gap. Two cases already assert that an
unconfirmed post-commit directory fsync fails the publish with PublishNotDurableError,
and both assert only the class name. The sentence that tells a caller which barrier
went unconfirmed was pinned by nothing: the string this file publishes as its reason
appears nowhere in the spec, and the helper that reports a rejection name together
with its wording was defined and never called. A case now drives the directory fsync
failure through its callback and asserts the report, so the reason is pinned too.

Controls, each named by the assertion that fails on this tree:
- degrade path removed, so a missing-privilege restore failure is treated like any
  other: "skips the DACL save and restore after a restore reports the missing
  privilege and still succeeds" fails, "promise rejected DaclRestoreError ...
  instead of resolving".
- every restore failure degraded, including ones where durability is unproven: 13
  failures, among them "keeps a non-privilege restore failure as the typed
  post-commit error" and "win32 DACL: a failed restore is a DaclRestoreError".
- reason recorded for the unconfirmed barrier replaced with undefined: the new case
  fails, "expected Published /tmp/test-dir/target.txt but could not confirm it is
  durable: ... to contain EIO: directory fsync callback failed". Before this commit
  the same control left the suite green, which is the hole the added case closes.
- durability throw removed outright: 3 failures, the two pre-existing name cases and
  the new one, each "expected resolved to be PublishNotDurableError".

Measured on this un-elevated host: icacls /save exits 0 and icacls /restore exits 1300
twice, which is the condition the memo exists for, so the memo path is reachable here.

Legs: the touched directories pass prettier --check; repository-root prettier --check
exits 1 on 24 files, none of them touched here, and the head blobs of both touched
files are prettier-clean, so the root red is not this change; eslint --prune-suppressions
--max-warnings=0 exits 0 on both touched files and neither has an entry in
src/eslint-suppressions.json, 0 before and 0 after, with fileLock.ts likewise 0 and 0
and untouched here, and the pruner re-indents all 1326 lines of that file to two spaces
with every count unchanged, which is reverted rather than committed; the declared
tsc --noEmit reports the same 10 errors in 6 files with and without this change, none
of them in services/file-safety, the failures coming from a package store junction
whose packages/types predates this head because no pnpm is installed on this box;
vitest services lane on this spec is 105 passed and 1 skipped, with the formerly
skipped case now executing, and the remaining skip is the one that is genuinely
host-bound.