Tracking S4b: wire guarded writes into the diff-view save paths #1400

Activity

1. easonLiangWorldedtech

   added a commit that references this issue on Oct 17, 2026

   feat(s4b): guard the legacy diff-editor save path and re-check the ro…

   ...

   22cf25c
2. easonLiangWorldedtech

   added a commit that references this issue on Nov 5, 2026

   revert(utils): drop the out-of-scope safeWriteJson work from the S4b …

   ...

   ab09473
3. easonLiangWorldedtech commented on Nov 9, 2026

   

   easonLiangWorldedtech

   on Nov 9, 2026

   ContributorAuthor

   More actions

   Split plan for PR 1408 — three units, measured widths, merge order, fidelity contracts, and what the split costs that one PR did not

   Position, stated so a reader does not have to infer it from silence. The campaign takes the three-unit split rather than a narrow signed exclusion. The reason is arithmetic: the unit content measures 640 counted executable lines against a 500-line cap, so this PR is over the cap whatever a reviewer prefers, and the alternative remedy needs a maintainer's signature on named lines, which no lane here can write for them. A split is something a lane can build, price and make reviewable. Nothing is pushed at 1408 while the plan is unexecuted, and no split PR is opened yet, because 1910 and 1911 are this chain's queue heads and opening three PRs below them changes what a reviewer sees and in what order. The plan is posted on this tracking issue before any split PR opens. Prepared by s3-1395 at the lead's instruction on 2026-11-09 from measurements already made on 1408.

   1. The widths, and where they came from. Three units were cut locally as three commits on scratch refs, nothing pushed and no PR opened. At base e487cd7b2a: unit 1 "safeWriteText" head 04644bd338, 191 counted executable lines (safeWriteText.ts 152 plus fileLock.ts 39); unit 2 "guardedWrite" head 2fffcfa494, 276 counted lines (guardedWrite.ts 152, observationRegistry.ts 44, DiffViewProvider.ts 48, ApplyPatchTool.ts 32); unit 3 "wiring" head eeb89a3136, 173 counted lines ( DiffViewProvider.ts 48, ApplyDiffTool.ts 39, ApplyPatchTool.ts 41, EditTool.ts 17, SearchReplaceTool.ts 17, ReadFileTool.ts 12, observationRegistry.ts 12, EditFileTool.ts 9, WriteToFileTool.ts 9, Task.ts 4, fileLock.ts 4 ). 191 + 276 + 173 = 640, so the three units partition the counted content exactly and no unit's executable code crosses 500. An earlier cut put ApplyDiffTool.ts and its failureReporting spec in unit 2 and gave 191 / 263 / 186; the current refs above are the re-cut, which moved those two paths whole into unit 3 because three review rows ask about ApplyDiffTool concurrency and the rows and the spec that proves them must travel together. The completeness control for the re-cut: unit 3's tree differs from this PR's head tree by exactly one path, that spec file, and by nothing else.

   2. Contents and merge order. Merge order 1 → 2 → 3, and each unit declares only what it adds rather than re-declaring its parent.

   • Unit 1: src/services/file-safety/safeWriteText.ts and src/services/file-safety/__tests__/safeWriteText.spec.ts. Depends on nothing in this PR.
   • Unit 2: src/core/tools/guardedWrite.ts, src/core/task/observationRegistry.ts, and the three tool specs whose subject is the writer service (checkpoint.spec.ts, applyDiffTool.guarded.spec.ts, applyPatchTool.guarded.spec.ts). Consumes unit 1's writer, so it merges after unit 1.
   • Unit 3: the wiring — DiffViewProvider.ts, ApplyPatchTool.ts, ApplyDiffTool.ts, EditTool.ts, SearchReplaceTool.ts, ReadFileTool.ts, EditFileTool.ts, WriteToFileTool.ts, Task.ts, the three ApplyDiffTool concurrency specs and ApplyDiffTool.spec.ts. Merges after units 1 and 2 because it calls their APIs.
     Each unit re-measures its own width from its own merge base once its parent lands, because a child unit stops re-declaring what its parent already contains; the numbers above are the widths at base e487cd7b2a, which is where they were measured.

   3. Fidelity contract, per unit. What must not change while the cut is made.

   • Unit 1: the publish contract as it stands at c46714d6c2 — staging under a private per-write directory, mode and group captured from the target, the win32 DACL saved and restored around the commit rename, fsync of the file and of the directory, typed post-commit errors and isPublishCommittedError telling the caller whether the content is already on disk. Content ports byte-identical; no behaviour edit in unit 1.
   • Unit 2: the compare-and-swap and observation contract — stale and not-read-yet sentinel errors must reach the tool result the model sees, with the remediation text intact, and a refusal must carry the reason rather than a status code. The concurrency proof set travels whole with the spec that proves it; splitting a proof set from its subject is how a green suite stops meaning anything.
   • Unit 3: this issue's own Scope, unchanged — DiffViewProvider.saveDirectly and DiffViewProvider.saveChanges route through the guard, sentinel errors propagate into the tool catches so the model receives the remediation, the ApplyPatch delete path stays unguarded because deletes do not CAS, userEdits semantics in saveChanges are preserved, and a clean save shows no behaviour change: no new latency, no new say types.

   4. Verification each unit carries, measured on the three cut trees on this box. Root tsc -p tsconfig.json --noEmit exit 0 with 0 error TS rows on all three trees. Lanes from src/: unit 1 core exit 0 (200 files, 4,298 passed / 9 skipped) and misc exit 0 (118 files + 1 skipped, 2,096 passed / 13 skipped); unit 2 core exit 0 (200 files, 4,308 passed / 9 skipped) and misc exit 0 (118 + 1 skipped, 2,106 passed / 13 skipped); unit 3 typecheck exit 0, misc exit 0 (118 + 1 skipped, 2,117 passed / 13 skipped) and core exit 0 (201 files, 4,365 passed / 9 skipped). Disclosed rather than dropped: one earlier run of unit 3's tree exited 1 while reporting the same 201 files and 4,365 tests passing with no failing test file; its cause is not pinned, so it is stated as a teardown transient and each unit re-runs this pair at its own head before it is pushed.

   5. What the split costs that the single PR did not pay, named before anyone builds it.

   • Unit 2 needs a knip exception. guardedWrite.ts ships in unit 2 and its only consumer arrives in unit 3, and knip is a required check here. 1408's current head carries no knip.json change at all (27 files, none of them named knip), so the exception line is a new cost the split introduces; unit 3 must delete it, and a unit that leaves it behind after unit 3 lands is a stale exception.
   • Three pushes, three review cycles and three pre-merge clearances instead of one, and the stack review budget applies per unit.
   • src/eslint-suppressions.json travels with unit 3 (main's refresh touched it); each unit re-reads the per-file counts and must not raise them, and --prune-suppressions rewrites indentation, so the file is restored from its committed blob after any run.
   • Mutation is the unknown half. No preflight can run on this machine: scripts/stryker-diff.mjs line 421 spawns node_modules/.bin/stryker, which is a #!/bin/sh pnpm shim, and spawnSync of it returns status null, error ENOENT, so a unit's mutant count is unknown until CI's mutation-diff row lands. The line cap is satisfied at 191, 276 and 173; the separate 400-mutant cap is not measurable here. The guidance the chain uses is that a unit of new executable code wants to sit nearer 380 to 420 counted lines than 499, so 191 and 276 sit below that band and 173 well below it — comfortable on lines, and still unpriced on mutants until each unit's own CI row arrives.

   6. What must be measured before each unit PR opens, in order. Each unit's width from its own merge base after its parent merges; the mutation row for that unit at that head; whether unit 2 still needs the knip line at its actual head rather than at 2fffcfa494; and the arms list for that head read once by arm name.

   Nothing was pushed by this plan and no split PR was opened. The queue stays 1910, then 1911, then these three.
4. easonLiangWorldedtech commented on Nov 9, 2026

   

   easonLiangWorldedtech

   on Nov 9, 2026

   ContributorAuthor

   More actions

   Correction to the plan I posted above, in my own words and with the measurement that replaces it.

   I wrote that unit 1 is 191 counted executable lines, "safeWriteText.ts 152 plus fileLock.ts 39". The 191 is right and the decomposition is wrong. I built unit 1 at 04644bd33876b1bcc9d829558cc5ae67f41a6903 on its own base e487cd7b2a49 and re-ran the gate's own counter on that tree: it printed baseSha e487cd7b2a49, headSha 04644bd33876 and mergeBase e487cd7b2a49 - mergeBase equal to the base passed, so no first-parent substitution - and it printed 191 changed executable lines in package extension from one file, safeWriteText.ts, with 72 selectors and selectorsHash 0b46021bd21b6ef1 against MAX_CHANGED_LINES 500, headroom 309.

   What makes the decomposition impossible rather than merely unmeasured: src/utils/fileLock.ts is blob 49807ea154d4 at the base, at the unit and at the PR head c46714d6c2. A file whose blob does not change between merge base and head contributes no changed lines, so no split of 191 between that file and safeWriteText.ts exists to argue about. And git diff --name-status from the base to the unit prints two paths, both Added: safeWriteText.ts and its spec, with blobs ecb6e3275221 and b5dfb042bfda, identical to the head's blobs for the same paths - so unit 1 modifies nothing outside those two files. The 152 I quoted is guardedWrite.ts's number, which belongs to unit 2.

   The totals stand: 191 / 276 / 173 summing to 640 at base e487cd7b2a, merge order 1 then 2 then 3, non-cumulative. I re-measured unit 1 only, so units 2 and 3 are inherited here and I am not printing their manifest.baseSha, which is the condition under which one argument can truthfully yield two numbers. Unit 1's verification, in the private worktree wt-s3-1408-u1 (no push): root and src typecheck exit 0 after the package junctions were mirrored; its own spec passes inside the services lane, proven against a control - the same command on the unit's base tree printed the same 4 failing files and the same 53 failing rows, and the only deltas between the two runs are the unit's own +1 file and +46 passing tests; misc lane exit 0; both lane-crossing controls exit 1 with "No test files found"; eslint on the two touched files exits 0 with src/eslint-suppressions.json unchanged and verified byte-identical to its committed blob; prettier --check exit 0; knip exits 1 identically on the unit tree and on the base tree with byte-identical output and zero mentions of safeWriteText. Results are recorded in the sweep ledger as section 73.
5. easonLiangWorldedtech commented on Nov 9, 2026

   

   easonLiangWorldedtech

   on Nov 9, 2026

   ContributorAuthor

   More actions

   Second correction to the plan above, both halves measured in private worktrees on 2026-11-09; nothing was pushed and no split PR was opened.

   Topology, so the numbers are read against the right base. The cut commits are a chain - 04644bd338 (unit 1) <- 2fffcfa494 (unit 2) <- eeb89a3136 (unit 3), with git rev-parse HEAD^ naming the previous commit in each worktree - and all three descend from e487cd7b2a49. Each unit's own merge base is therefore its parent, and the widths are measured there: unit 2 against unit 1 printed baseSha 04644bd33876 / headSha 2fffcfa49468 / mergeBase 04644bd33876 (mergeBase equal to the base passed) and 276 counted executable lines, 37 selectors, hash c151a75724c1c002; unit 3 against unit 2 printed baseSha 2fffcfa49468 / headSha eeb89a3136d9 / mergeBase 2fffcfa49468 and 173 lines, 49 selectors, e8f121e47c671ccf. Two controls close the arithmetic: unit 2 measured against e487cd7b2a49 prints 467, which is 191 + 276, and unit 3 measured against e487cd7b2a49 makes the gate throw - 'extension has 640 changed executable lines (limit 500)' - so the 640 that justifies the split is the counter's number, not my addition.

   The knip exception I said unit 2 needs, unit 2 does not need. The plan says unit 2 must carry a narrow knip line because guardedWrite.ts's consumer arrives in unit 3. That is true of production consumers - the first non-test importer is src/integrations/editor/DiffViewProvider.ts, which imports guardedWrite at line 22 and calls it at 376 and 1254, and that file is unit 3's - but it is not true of the knip arm, and I tested it rather than reasoning from it. On unit 2's tree as cut, knip --treat-config-hints-as-errors (the flag in the root package.json script, run with git on PATH) exits 1 with 373 bytes whose sha256 is byte-identical to the unit-1 and base outputs, and no line mentions guardedWrite: there is no finding to silence. Removing unit 2's own guardedWrite.spec.ts from the tree makes knip print 'Unused files (1) src/core/tools/guardedWrite.ts', which identifies the spec as the thing that satisfies the arm. And adding an ignore entry inside knip.json's workspaces.src object makes it worse, not better: a workspace-level ignore replaces the inherited root ignore list rather than extending it, and the run grew from 373 to 4,923 bytes of output - 37 files that the root config ignores came back as unused, plus a configuration hint telling me to remove the ignore entry because nothing used it.

   So the plan line is amended to: unit 2 carries no knip line and unit 3 deletes nothing, verified on unit 3's tree the same way (exit 1, 373 B, the same sha256, no mention of guardedWrite). If the merge order were reversed the answer would not change on knip grounds - unit 3's own DiffViewProvider.ts keeps guardedWrite visible - though it would not be admissible on size grounds anyway, because unit 3 measured against the series base is the 640 the gate throws on. What unit 3 does carry that unit 2 does not is a suppressions reduction: src/eslint-suppressions.json goes from base blob ae3d504003d7 (unit 1 and unit 2) to c10bf7af4282 (unit 3, identical to the head's), and the single change in it is readFileTool.spec.ts dropping its no-explicit-any count from 96 to 95.

   Both units are built and verified in private worktrees and neither is pushed. Unit 2: root and src typecheck exit 0; core lane exit 0 at 200 files / 4,298 passing, against its parent's 197 / 4,246 (deltas are its own three specs, all passing); eslint on its six touched files exit 0 with the suppressions file unchanged and byte-identical to its committed blob, Task.ts held at 17; prettier exit 0. Unit 3: core lane exit 0 at 201 files / 4,365 passing against unit 2's 200 / 4,298, and the misc lane - the lane whose include glob covers integrations/** and so DiffViewProvider.spec.ts - exit 0 at 118 files / 2,117 passing against unit 2's 2,096; eslint exit 0 on its 18 touched .ts files; prettier exit 0; and lane-crossing controls exit 1 with 'No test files found' on every unit, which is what makes a green lane mean the lane it is. One thing the counter printed that I am not softening: the unit-2 pairing carries an advisory naming src/core/task/Task.ts:4427, whose line is a reasoned mutator-named '// Stryker disable next-line ConditionalExpression, EqualityOperator: equivalent mutants, streamingKey is always defined here...'; it travels with the Task.ts edit rather than with the choice of base, and how CI treats that advisory class I have not measured.