Repository navigation
safeWriteText's staging-name guard and jsonFileRecovery's leftover prefixes are mutually exclusive #2350
Description
Two write-side contracts in this repository currently disagree about the name of a staging file, and the disagreement is unresolvable from inside a PR that has to satisfy both.
The two contracts, as they read today
- The guarded-write primitive refuses any caller-supplied temp path whose basename does not match its own staging convention —
src/services/file-safety/safeWriteText.ts(present on PR branches, absent at main73931fe8be):
// Ownership convention: a caller-staged file must carry the staging name this
// library's own writers use (.new_<ts>_<rand>.tmp). An arbitrary regular file
// beside the target (e.g. credentials.json) passes every check above, yet
// renaming it onto the target publishes an unrelated file's content and deletes
// its original name. Only files created under the staging convention may be
// published through this parameter.
if (!/^.new_.*.tmp$/.test(path.basename(supplied))) { // :405
throw new StagingPathError(
`Staging file must carry the staging name convention .new_*.tmp, got ${path.basename(supplied)}`,
supplied,
)The behaviour is pinned by its own test: safeWriteText.spec.ts:1696 it("rejects a caller-staged file that does not carry the staging name convention"), asserting the message at :1705.
- Main's JSON recovery recognises leftovers only by a target-prefixed name —
src/utils/jsonFileRecovery.ts:9-19:
function getTempFilePrefix(absoluteFilePath: string, kind: "new" | "bak"): string {
return `.${path.basename(absoluteFilePath)}.${kind}_`
}
function isReplacementLeftover(absoluteFilePath: string, entry: string): boolean {
return (
entry.endsWith(".tmp") &&
(entry.startsWith(getTempFilePrefix(absoluteFilePath, "new")) ||
entry.startsWith(getTempFilePrefix(absoluteFilePath, "bak")))
)
}Why they are mutually exclusive for a caller-staged file
For target settings.json the two accepted shapes are:
| who accepts it | basename | verdict |
|---|---|---|
| recovery only | .settings.json.new_1730000000.tmp |
rejected by the primitive — basename does not start .new_ |
| primitive only | .new_1730000000_a1b2c3.tmp |
invisible to recovery — does not start .settings.json.new_ |
So a caller that stages a file beside the target can satisfy the primitive or be recoverable, not both. This is not theoretical: passing main's own recovery-recognised staging name through the primitive fails its guard with Staging file must carry the staging name convention .new_*.tmp, got .settings.json.new_…tmp (safeWriteText.ts:406-409), which is how 48 tests failed in a conflict resolution that tried to keep main's name for main's recovery.
The reconciliation being used in #1911, and the gap it leaves
The resolution on #1911 stages .new_<basename>_<ts>_<rand>.tmp — anchored so the primitive accepts it, and target-scoped so recovery can never unlink another target's in-flight staging file in the same directory — and extends isReplacementLeftover with a third accepted prefix derived from the target basename. Two things follow that a maintainer should decide on rather than inherit:
- Recovery's accepted set grows from two prefixes to three. It is pinned by tests for the new prefix plus a negative test that a bare
.new_anything.tmpis still not collectable, but the boundary is now a policy choice, not an accident. - Direct
safeWriteTextcallers are still not covered. A unit that writes text through the primitive creates.new_<ts>_<rand>.tmpnames that are not target-scoped, so recovery still will not garbage-collect its leftovers after a crash. Guarded writes in the same series that go through that path therefore leave files a restart cannot clean up.
Asks
- Decide which convention wins, once, in one place: either the primitive accepts the target-prefixed name, or recovery accepts the unscoped name, or both derive the name from a single exported helper (e.g. one
stagingNameFor(targetBasename)used by writer and recovery alike). Today a PR has to invent a third answer. - Pin the accepted-prefix set with a test in
jsonFileRecoverycovering all accepted shapes and at least one rejected look-alike, so the next writer cannot widen it silently. - Note the divergence this creates on the file itself:
src/services/file-safety/safeWriteText.tscurrently exists in four independent, non-ancestor shapes across open PRs — 819 lines at1f8ae7fd0a(feat(file-safety): guarded text publish module + specs (A4, #1375) #1395), 833 lines at0580426aa2(feat(file-safety): canonical advisory lock key (U2, #1375) #1911's head), 446 lines ataa44471e74(feat(tools): guarded write CAS core with per-path FIFO chain (S4a, #1375) #1405), 469 lines ate9490e555d(feat(tools): wire guarded writes into the diff-view save paths (S4b, #1375) #1408) — and main has no version of the file at all. Whichever merges first sets the convention every later PR must satisfy, so this naming decision is best taken before the first of them lands.
No code change is proposed here; this is the record of a contract conflict measured while resolving merge conflicts, filed so the decision has one home.
Activity
fix(safe-write): compare directory identity in the safeWriteText stag…
Merge upstream main 73931fe into fws/u9-task-history-delete
Merge upstream main 73931fe into fws/u9-task-history-delete
Merge upstream main 73931fe into fws/u9-task-history-delete
fix(utils): keep the pre-commit ancestry assertion out of the EPERM r…
Merge upstream main 73931fe into fws/u9-task-history-delete
Mark safeWriteText internal now that nothing in production calls it
Copy the target's mode onto the staged file so the publish cannot wid…
easonLiangWorldedtech commentedon Nov 8, 2026
Series note carried here from #1913 (comment 6717029801), because #2350 decides which primitive main
keeps and that decision is what makes this fix land main-side or in eight lanes.
Measured at #1913 head
8fa7d858c:platform-unit-test (windows-latest)failed with 12 failures inmain's
src/utils/__tests__/safeWriteJson.interruption.test.ts(blobc9c5993f847b…at73931fe8), allexpected 'WRITE_COMPLETED\n' to contain 'EXIT_AT_REPLACEMENT_RENAME'at its line 187, while ubuntu wasgreen. Cause measured on pinned node v22.23.4: the spec's kill point is whole-path equality (child line
40); a lane that grafts path safety publishes to a realpath-resolved target (
fs.realpathexpands anNTFS 8.3 alias,
path.resolvepreserves spelling); on a runner whose TEMP is an alias the injectionnever matches and every row in the file fails. Reproduced locally by pointing TEMP/TMP at
C:\work\ZOO-CO~1\TMP-AL~2\LONG-D~1(12 failed, CI's assertion verbatim) and back at the long spelling(12 passed).
Consequence for the naming/decomposition decision: the fix's canonical home is main-side. Today it exists
in two lane copies (
1fb5fcc47on #1912,f739a5fa3on #1913) and every lane that takes the seam mustcarry it or be red on windows until main does. Merge order decides which text becomes canonical.
Merge the landed u4 head into u5 and record each conflict resolution
Merge the landed u4 head: main's write protocol plus our path-safety …
easonLiangWorldedtech commentedon Nov 8, 2026
Requirement for the guard-core writer, recorded from the #1914 refresh merge (split unit U5-B: branch
vps2f-1914-u5b@120cb057a9f0, builds on #1913's head416179d17de8).The guard-core writer has to carry two refusals. Neither is a rival design; both are the consumer-side application of contracts already ruled on, and I am recording them here because dropping them was the alternative.
(dev, ino)read as bigint stats (file identities on NTFS/ReFS exceedNumber.MAX_SAFE_INTEGER, so a rounded number either hides an alias or rejects a valid staging file). OnlyENOENTmay skip the comparison;EACCES/ELOOP/ENOENT's neighbours mean the comparison could not be made, and that is itself a refusal. Why it cannot be a log line: an alias that reaches the commit leaves the failure handler unlinking the only remaining copy of the content, so a failed write deletes the file it was meant to protect. This is Advisory lock key must be identity-backed, not realpath text: an NTFS 8.3 alias and its long form take different locks #2356's canonical-after-write identity rule applied on the consumer side.icacls /savefails, including after writing a partial dump), or whose access rights cannot be read, must yieldDaclInspectionErrorcarryingphaseandtargetPathbefore anything is committed - not a warning beside a committed file. Publishing without knowing the target's access rights can widen who reads the file, which is the same reasoning behind the_copyTargetModeruling in fix(task-persistence): delete under the canonical lock key (U9, #1375) #1917.Measurement behind the "silently lost" claim:
target itselfandDaclInspectionErroreach return 0 matching lines tree-wide at33e9200900bb(the commit the refresh merged into), so under guard-core's shape the absence would have looked like agreement.Both arrive with tests that exercise the refusal paths: the alias test, the non-
ENOENTreason test, the failed-save test (partial dump unlinked, no rename, oneicaclsattempt), and the inspection test. The inspection test injectsplatform: "win32"and stubsicacls, so it executes on either hosted leg rather than only on the windows one; the hosted windows leg passessafeWriteText.spec.ts. Two pre-existing tests that asserted the opposite behaviour were replaced rather than kept, and that is called out in the unit's commit message.Reversibility: if #2350 rules otherwise for either one, it comes out in a single commit on
vps2f-1914-u5b.easonLiangWorldedtech commentedon Nov 8, 2026
The generation map behind the safeWriteText/guardedWrite convergence (from the #1916 U8 resolution, vps2-f lane)
safeWriteTextexists in incompatible generations;guardedWriteexists in three. Which copy a branch carries is decisive for what can pair with what. Map as shipped on the stack today:retainedTargetoption, ACL/DACL capture + windows publish branches, exportsresolveLinkPathLockKey; json predates main's #2297 trace contractcancellationGenerationlineage,backupoption, noretainedTarget, noresolveLinkPathLockKeyexport; json carriesSafeWriteJsonTraceStage+trace(what main-sidetaskMessages.tsconsumes)canonicalizeForApproval(U7 containment)GuardStaleVersionErrorCAS contract +create/update/editkinds; publishes with{ backup: true };canonicalDirKey/resolveLinkPathLockKeytransplanted as module-local helpersPairing rule (adopted, not advisory): json + its specs and text + its specs travel as ONE generation, always. A branch may adapt a generation's consumers; it may not mix one generation's text with another's json and call the result converged.
The experiment that proves the rule: on #1916 I built the chimera - era
safeWriteTexttrio + 74f5-erasafeWriteJsonquartet - to keep the era surface the era call-site cluster expects. It passedtsc --noEmit, the full ubuntu unit legs locally, and every misc shard locally and on ubuntu CI. It failed only onplatform-unit-test (windows-latest)misc shards: 15 tests insafeWriteText's windows-only branches (3 backup-stat rows, 12 interruption kill-timing rows expectingEXIT_AT_REPLACEMENT_RENAME), because the era windows ACL/publish branches re-route the fs-mock expectations the 74f5 specs encode and shift interruption kill timing. (Code QA run 42459507387, windows job 128665547657.) A reviewer looking at the diff would have attributed that red to the change; it was the pairing. Pinning json+text together at 74f5 and renaming the 43 mock-argument expectations (retainedTarget: true->backup: true) turned the same suite green on every platform.Convergence consequence (#2357 review input): the convergence target is the 25a1a generation, and the sequencing is a human review call, not another push. The U8 tree is queue-proof for it:
guardedWriteneeds onlybackup/resolveLockKey/resolvePublishTarget/PostCommitDurabilityError- all present in the 25a1a generation - and its era-only helpers are already module-local, so once #2357 lands the guardedWrite import swap is mechanical. Taking 25a1ad6 into U8 before #2357 lands was rejected deliberately: it would import the U7-era containment shape mid-stack and re-open pairings already merged (U1-U5).Numbers stated with their bases so they can be compared: local changed-executable-line recount vs the pre-merge branch base 2e00da1 gives types 15/500, webview 401/500, extension 1190/500 on 711ada7; the CI mutation gate computes against the merge base (main, 73931fe) and its reason line reads extension 1797/500 - both over-cap, one advisory class either way.
Merge upstream main (73931fe) into fws/u6-apply-patch-wiring
easonLiangWorldedtech commentedon Nov 8, 2026
Follow-up to the four-generation table above — the CI discriminator fired on the windows leg and it refined the json row, so the table needs this addendum for anyone reviewing the convergence:
What failed.
711ada709(then-final #1916) failed 2 rows on the windows platform-unit misc shard with the reason lineAssertionError: promise resolved "undefined" instead of rejectingatsafeWriteJson.test.ts:675(run 42461215177, job 128670729075). The same row family was already red on74f5eb287's own windows CI — run 42448302148, job 128631851822, 3 failed rows, same file. That rules generation mixing out: json and text were pinned to the same generation there (74f5eb2), and the chimera's 15-row era-text class was gone. The defect is inside the 74f5 generation's test double: the backup-stat injection keyed onos.tmpdir()'s spelling of the leftover path while the writer stats the canonical form of the same file — on windows realpath spells them differently, so the injection missed and the write resolved instead of rejecting.The fix and its provenance.
025fb7d0d(#2359 head) carriessafeWriteJson.tsbyte-identical to74f5eb287(verified via git diff) with a hardenedsafeWriteJson.test.ts(+31/−9: injection keyed on the leftover's dev+ino instead of path spelling, and the fs.stat mock implementation restored in afinallybecause the OS recycles inodes). Its windows platform-unit passed this exact test against the identical implementation — run 42455872567, job 128654644966.What shipped. Test-only adoption across the three carriers, each named in its commit message with the copy source:
281c24565onfws/u5-guard-core(fix commit, parent74f5eb287)680bdc2d142a1bbb86All three now carry a byte-identical copy of that test file, and #2357/#2359 already carried it, so the whole stack holds exactly one copy. The pairing rule from the table above stands untouched — json and its specs travel as one generation; what changed is that the 74f5 json generation is now paired with its own windows-proven spec instead of the version with the path-spelling bug. A full pin bump to
025fb7d0dwas rejected deliberately: it would drag #2359'ssafeWriteText.ts+64/−4 (and the rest of that unit's content) into ancestor branches across the unit boundary, while the json implementation is identical — test-only keeps merge order intact. The generation rows (era /74f5eb287/25a1ad65f/ U8-guardedWrite) and the four-generation pairing rule are unchanged.