Skip to content

Commit 64dc74c

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(utils): treat an unanswered drift probe as no evidence of drift
platform-unit-test (ubuntu-latest) was red at 40ac71f on exactly one case: core/task/__tests__/Task.spec.ts > 'finalizePartialToolAsk still updates the webview when a later save stage fails' (expected "updateClineMessage" to be called with arguments, Number of calls: 0). Measured by printing every realpath call in that window with its outcome and, at the same instant, running fs.statSync on the same path through the never-mocked fs: not one realpath returned ENOENT for the directory - the guard's first walk and both lock-key canonicalizations resolved it, four times, to inode 22517998136858990 - and the only ENOENTs were on ui_messages.json itself, the target file that genuinely is not there. The failure is the mirror image of a missing directory: the guard's own 'await fs.stat(approvedCanonical)' reported ENOENT for a directory the unmocked fs reported as that same inode, because Task.spec.ts:144 mocks fs/promises with 'stat: vi.fn().mockRejectedValue({ code: "ENOENT" })' for every path. The second walk never ran, so the drift comparison never happened. The guard was failing on a question it invented, and the only reason our own lane never caught it is that the suite which drives this write path mocks stat to fail for every path. The guard now acts on a single signal: the re-resolution of the approved directory succeeds and lands on a different directory. A probe that cannot answer is not an answer - ENOENT because a component is mid-replacement or because the caller's fs reports the path as missing, EACCES, EPERM, ELOOP - and a failing probe is a normal condition of this write path rather than an attack, so _probeApprovedStagingDirectory reports 'no answer' instead of throwing and the write proceeds. The existence probe is gone. A directory that really was deleted then fails at the staging write itself: still fail-closed, still with nothing created outside the directory the write was approved for, and the error names the failed write instead of claiming to be evidence of replacement. The confinement checks above deliberately keep the opposite rule: there the guard has to prove the write is inside the scope, and a probe that cannot answer cannot prove anything. The guard's comment paragraph was duplicated verbatim twice in the previous head (lines 297-301 and 302-306, a chain artifact); it is collapsed here. Controls. Reverting this change reddens exactly the two new cannot-answer cases: 'stages anyway when the guard's probe cannot answer' (EACCES on the re-resolution - the failure the canonicalization walk itself rethrows, so the guard really is out of answers) and 'stages anyway under the task persistence suite's blanket missing stat', which puts Task.spec.ts:144's blanket double over a write whose directory exists and asserts expect(statCalls).toBe(0) (under the old guard that write died after exactly one stat call). Reverted: 2 failed | 12 passed on the two drift specs, and Task.spec.ts 1 failed | 198 passed - the CI case and only it. Restored: 5 spec files, 306 passed | 4 skipped, and the restore is byte-identical by SHA256 on all three touched files. Two cases are labelled in-file as absorbed rather than controls: the re-walk ENOENT case is absorbed by the walk's retry from its nearest existing ancestor, and the real-deletion case is keyed to the second reading, so the old guard had already cleared its probe before the directory went. The re-point case is now decided by the OS rather than by a stub: the approved directory is renamed aside and a junction onto a different directory is planted at the guard's second reading, realpath left unstubbed and the wrapper choosing only when the plant happens - the junction being the portable twin of the POSIX symlink case. safeWriteJson.lockKey.spec.ts's 'refuses to stage when the target's parent no longer names that directory' used to pass off the deleted existence probe (its stub named a path that never existed, so the guard rejected for 'no existing directory'); it now flips on the fourth reading with expect(readings).toBe(4), which is evidence about the re-verification and fails loudly if the guard ever stops taking it. Known property, deliberately not fixed here: when the approved directory is replaced before the guard's first reading, both readings agree and no layer rejects the write - measured byte-for-byte the same outcome before and after this change (payload published at wherever the approved pathname now points, no staged file left in either spelling, only the lock directory travelling with the renamed directory). The file can be published into a directory the writer's lock does not name. Closing that means safeWriteText binding the publish destination to the directory its own advisory lock names, which is a design change in the path #1916's guardedWrite layer builds on, so it is a follow-up unit rather than something to smuggle into a green-lane fix. Gates, all in split-1833/u7 against its own install: tsc -p tsconfig.json --noEmit exit 0; eslint --prune-suppressions --max-warnings=0 on the three files exit 0, with src/eslint-suppressions.json restored to HEAD bytes and the parsed (file,rule)->count delta computed in Node straight from 'git cat-file blob' - headSum 3537, nowSum 3537, 325 keys on both sides, 0 deltas, and the three touched files carrying no suppression entries at all; prettier --write then --check exit 0; root 'pnpm run check-types' exit 0 (10 packages cache hits, zoo-code:check-types ran 'tsc --noEmit' fresh). Cross-checked in a scratch merge of 40ac71f with upstream/main 6bbc7a0: the CI case is green there with this fix and red there without it at the same merge, and the single remaining failure in that tree - main's 'bills a 1h cache write at the 1h rate', expected +0 to be close to 0.6 - reproduces at pure merge content without these three files, so it is the scratch tree's donor-linked install resolving @roo-code/telemetry outside the merge, not this change.
1 parent 40ac71f commit 64dc74c

3 files changed

Lines changed: 304 additions & 90 deletions

File tree

‎src/utils/__tests__/safeWriteJson.lockKey.spec.ts‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -207,11 +207,29 @@ describe("safeWriteJson lock key under a peer commit", () => {
207207
currentLink = ""
208208

209209
// The staging location is chosen from the parent directory resolved under the lock, and the
210-
// staged file is written here in safeWriteJson before safeWriteText runs. Replace that
211-
// directory with a link pointing outside it in between, and the staged JSON lands outside the
212-
// directory this write was approved for - safeWriteText can still reject the publish, but the
213-
// stray file already exists. Canonicalizing the parent is the only thing that can see the swap.
214-
mockedRealpath.mockImplementation(async (t) => (String(t) === dir ? outside : String(t)))
210+
// staged file is written here in safeWriteJson before safeWriteText runs. Replace that directory
211+
// with a link pointing outside it between the guard's approval of it and its re-verification,
212+
// and the staged JSON lands outside the directory this write was approved for - safeWriteText
213+
// can still reject the publish, but the stray file already exists. Canonicalizing the parent is
214+
// the only thing that can see the swap, and it sees it as the re-verification landing somewhere
215+
// the approval did not. (A swap planted before the first reading is invisible to this guard by
216+
// construction: both readings then agree, and safeWriteText's own beside-the-target check is
217+
// what rejects it.)
218+
let readings = 0
219+
mockedRealpath.mockImplementation(async (t) => {
220+
const key = String(t)
221+
if (key !== dir) {
222+
return key
223+
}
224+
// Readings 1 and 2 are the lock-key canonicalization before and after the lock, reading 3
225+
// is the guard approving the directory as it currently resolves, and reading 4 is the guard
226+
// re-verifying it - the one a planted swap has to move. This case used to pass off the guard's
227+
// separate existence probe instead: its stub named a path that never existed, so the guard's
228+
// fs.stat of the resolution failed and the write was rejected for "no existing directory",
229+
// which was never evidence about a re-point. The readings below are what make it evidence.
230+
readings += 1
231+
return readings <= 3 ? dir : outside
232+
})
215233
mockedLstat.mockImplementation(
216234
async () => ({ isSymbolicLink: () => false, isFile: () => true }) as unknown as BigIntStats,
217235
)
@@ -223,6 +241,7 @@ describe("safeWriteJson lock key under a peer commit", () => {
223241
await expect(safeWriteJson(target, { id: "task-1" })).rejects.toThrow(
224242
"the directory this write was approved for",
225243
)
244+
expect(readings).toBe(4) // the guard really did take its second reading of the approved directory
226245

227246
expect(released).toBe(true)
228247
// Nothing was staged: no target, and no .new_*.tmp left behind in the directory.

0 commit comments

Comments
 (0)