fix(runtime): distinguish unchecked writes from T0-missing races in the filesystem worker - #3487
Conversation
9fc450b to
51b327f
Compare
…S behavior tests Review follow-ups (#3487, Opus5/Luna): - `operationAccess` (write | apply_patch | edit | format_json) now lives once in the shared protocol module; the client, the worker and the executor's T0-marker decision all call it, so the set cannot drift — the executor previously hand-copied a third list that silently dropped apply_patch onto 'unchecked'. - Worker-level behavior tests: an apply_patch update against a swapped inode fails path_changed and leaves the replacement content untouched (delete and edit already had such tests; update was the gap). - Format the smoke-test fixtures so the exact-head format check passes. Generated-by: DSv4F-AstroHan
…he filesystem worker Fixes #3484. expectedIdentity was optional, so a caller without a T0 snapshot was indistinguishable from a T0-missing target that appeared while queued; every write to an existing file without an identity failed with path_changed, breaking verify-windows-sandbox-e2e and blocking the Release Windows check on four PRs. - FilesystemWorkerExpectedIdentity is now required: {dev,ino} | 'missing' | 'unchecked'. Every caller must decide explicitly which CAS contract it participates in. - The wire request carries the T0 marker explicitly (protocol v7) so the worker can tell 'created while queued' from 'no CAS snapshot'. - Worker: t0 'missing' + existing target fails path_changed; t0 'existing' without identity fails invalid_request (buggy caller); t0 'unchecked' skips CAS. Identity-present targets still CAS. - openStableTarget opens an existing target without an identity when the caller is 'unchecked' (no comparison), keeping the create path exclusive (wx) unchanged. - Callers: FilesystemExecutor passes its captured identity or 'missing' for mutations and 'unchecked' for reads; the runtime-host read-only adapter and verify-windows-sandbox-e2e pass 'unchecked'. - Tests: unchecked write to an existing target passes and carries no identity; 'missing' with an existing target still fails path_changed; CAS mismatch still fails; T0-missing marker is preserved on the wire. Generated-by: DSv4F-AstroHan
… worker branch The T0 marker computation in the executor aligned with `mutates` (write/edit/format_json) instead of the worker client's `operationAccess` (write/apply_patch/edit/format_json). apply_patch therefore fell into the 'unchecked' branch, silently dropping the identity captured by applyPatch() and disabling the queue-window CAS on the main editing channel (create/update/delete). Add apply_patch to the mutation list, point the comment at operationAccess as the authority, and cover the forwarding with an executor-level regression test. Generated-by: DSv4F-AstroHan
…d three-state field
Review finding (P2, Opus5 + Muse converged): the previous wire carried a
required `t0` enum plus an optional `identity` plus two cross-field
superRefines — two models for one concept, with the looser wire able to
express illegal combinations that only runtime constraints rejected.
The wire now carries a single required three-state `identity`
({dev,ino} | 'missing' | 'unchecked') mirroring the client input, so an
illegal state cannot be expressed on the wire at all. The worker treats
'missing' with an existing target as created-while-queued (path_changed),
CASes objects, and skips everything for 'unchecked'. The "buggy caller
omitted identity" invalid_request guard disappears because the wire can
no longer express that state.
Generated-by: DSv4F-AstroHan
…S behavior tests Review follow-ups (#3487, Opus5/Luna): - `operationAccess` (write | apply_patch | edit | format_json) now lives once in the shared protocol module; the client, the worker and the executor's T0-marker decision all call it, so the set cannot drift — the executor previously hand-copied a third list that silently dropped apply_patch onto 'unchecked'. - Worker-level behavior tests: an apply_patch update against a swapped inode fails path_changed and leaves the replacement content untouched (delete and edit already had such tests; update was the gap). - Format the smoke-test fixtures so the exact-head format check passes. Generated-by: DSv4F-AstroHan
5ac0d37 to
e405a34
Compare
The rebase conflict on verify-windows-sandbox-e2e.mjs was resolved with the branch's own 'unchecked' variant, which would have reverted the #3457 hardening (the script now captures a real T0 identity via stat and passes it). Restore the main version: the script verifies relay AND the packaged CAS path; 'unchecked' would silently skip the identity comparison on the release lane. Generated-by: DSv4F-AstroHan
M4n5ter
left a comment
There was a problem hiding this comment.
English
Reviewed exact head 89db39fbf03c77ccb6ab5df55b954db05bc77942.
I found two issues worth addressing before merge.
[Blocking] Migrate the JavaScript release callers to the required v7 identity contract
packages/runtime/src/filesystem-worker/protocol.ts:59-83 now requires expectedTarget.identity, but two supported JavaScript release callers still construct requests without it:
scripts/verify-macos-arm64-dmg.mjs:49-60constructs a raw v7 request for a missing target withoutidentity: 'missing'.scripts/verify-windows-sandbox-e2e.mjs:91-92forwards an optionalexpectedIdentity. The seeded write supplies its captured inode, but Read, Glob, and Grep at lines 126, 135, and 148 omit it.
For the Windows calls, client.ts:328-331 consequently places identity: undefined in the request object. JSON.stringify removes that property, and the packaged worker rejects the request as invalid_request under the required v7 schema. The verifier therefore terminates at its first Read. The macOS raw request is rejected for the same reason.
The current green checks do not cover these paths. windows_sandbox_w0_protocol runs the TypeScript worker smoke, not verify-windows-sandbox-e2e.mjs, and this PR did not trigger the path-filtered Release Windows workflow.
The minimum repair is to add identity: 'missing' to the macOS raw request and explicitly supply the correct marker from every Windows call.
The cleaner root model is narrower: CAS identity is a mutation contract. Have the client derive wire 'unchecked' for read operations through operationAccess, while requiring and runtime-validating an explicit T0 state only for mutations. That removes meaningless state from read callers and prevents untyped JavaScript reads from recreating this failure. The Windows mutation probes should still explicitly pass their captured identity or 'missing'.
Please run the packaged Release Windows verifier after the repair; a small request-shape regression for the macOS raw smoke would also close the currently untested boundary.
[Important] Preserve the creation diff for worker-backed new-file writes
packages/runtime/src/filesystem-worker/operations.ts:164-173 still identifies a new file using the old optional-field test:
if (!expectedTarget?.identity) {
previous = 'new';
}Under v7, a T0-missing target carries the truthy value 'missing'. The worker therefore creates the file with a write-only wx descriptor, tries to read that descriptor, catches the failure as previous = 'unknown', and omits the diff entirely.
This affects the normal managed Write path. builtin-tools.ts:416-418 converts the result from a reviewable file_diff containing --- /dev/null into only a file_write descriptor, so users lose the new file's change review.
Use the target fact as the authority—for example, expectedTarget?.targetType === 'missing'—and add one worker behavior assertion that a missing-target Write returns a /dev/null creation diff.
Simplify audit
filesystem-executor.ts:125-130 still maintains a second handwritten write-kind list in mutates(), while this PR introduces operationAccess() as the shared authority. Line 270 can use operationAccess(call.operation.kind) directly and delete mutates(). This removes the same class of classification drift without introducing another abstraction.
I did not find any changed tests that should be deleted: the executor forwarding test and worker CAS tests protect distinct stable boundaries.
The three-state wire direction and the fd-pinned CAS paths otherwise look sound, including apply_patch forwarding, inode replacement rejection, exclusive creation, and compare-and-delete.
简体中文
已审查 exact head 89db39fbf03c77ccb6ab5df55b954db05bc77942。
我发现两项值得在合并前处理的问题。
[Blocking] 将 JavaScript release callers 完整迁移到 v7 必填 identity 契约
packages/runtime/src/filesystem-worker/protocol.ts:59-83 现在要求 expectedTarget.identity 必填,但两个受支持的 JavaScript release caller 仍会构造不含该字段的请求:
scripts/verify-macos-arm64-dmg.mjs:49-60为缺失目标直接构造 raw v7 request,却没有传入identity: 'missing'。scripts/verify-windows-sandbox-e2e.mjs:91-92转发可省略的expectedIdentity。预先创建的 write 传入了真实 inode,但 lines 126、135、148 的 Read、Glob、Grep 都没有传值。
Windows 路径中,client.ts:328-331 因此会在 request object 中产生 identity: undefined。JSON.stringify 会删除该属性,packaged worker 随后按照 v7 required schema 将请求拒绝为 invalid_request,verifier 会在第一个 Read 处直接终止。macOS raw request 也会因同一原因被拒绝。
当前绿色 CI 没有覆盖这些路径。windows_sandbox_w0_protocol 运行的是 TypeScript worker smoke,不是 verify-windows-sandbox-e2e.mjs;本 PR 也没有触发带 path filter 的 Release Windows workflow。
最小修复是为 macOS raw request 增加 identity: 'missing',并让 Windows 的每个调用显式传入正确 marker。
更根因化且更简洁的模型是:CAS identity 只属于 mutation contract。client 应通过 operationAccess 为 read operation 自行生成 wire 'unchecked',只要求 mutation caller 提供并在运行时校验显式 T0 state。这样可以从 read caller 中删除无意义状态,并避免未类型检查的 JavaScript read 再次制造同类失败。Windows 的 mutation probe 仍应明确传入捕获的 identity 或 'missing'。
修复后请运行实际 packaged Release Windows verifier;同时可以用一个很小的 request-shape regression 封住 macOS raw smoke 当前未测试的边界。
[Important] 保留 worker-backed 新建文件 Write 的 creation diff
packages/runtime/src/filesystem-worker/operations.ts:164-173 仍沿用旧的 optional-field 判断来识别新文件:
if (!expectedTarget?.identity) {
previous = 'new';
}在 v7 中,T0 不存在的目标会携带 truthy 的 'missing'。worker 因此先通过 write-only wx descriptor 创建文件,随后尝试读取该 descriptor;读取失败被降级为 previous = 'unknown',最终完全省略 diff。
这会影响正常的 managed Write 路径。builtin-tools.ts:416-418 会将本应包含 --- /dev/null、可供用户审查的 file_diff 降级为只有基本信息的 file_write,用户无法再看到新文件的 change review。
应以目标事实作为 authority,例如判断 expectedTarget?.targetType === 'missing',并增加一条 worker 行为断言,确认 missing-target Write 会返回 /dev/null creation diff。
Simplify audit
filesystem-executor.ts:125-130 仍在 mutates() 中维护第二份手写 write-kind 列表,而本 PR 已引入 operationAccess() 作为共享 authority。line 270 可以直接使用 operationAccess(call.operation.kind) 并删除 mutates()。这能在不新增抽象的情况下消除同类分类漂移。
没有发现应该删除的 changed tests:executor forwarding test 与 worker CAS tests 分别保护不同的稳定边界。
除此之外,三态 wire contract 与 fd-pinned CAS 的整体方向成立;apply_patch identity forwarding、inode replacement rejection、exclusive create 和 compare-and-delete 路径均未发现新的实质问题。
…ct (#3487) Three findings from the external maintainer review: 1. JavaScript callers were not migrated and the required-field argument gave them no protection. The client now auto-generates the wire 'unchecked' identity for reads (callers cannot get reads wrong, even from plain .mjs that bypass TypeScript) and enforces at runtime that write operations carry an explicit identity contract — a JS caller that omits it fails loudly instead of silently skipping the queue-window CAS. verify-macos-arm64-dmg.mjs's raw v7 write request gains the required identity: 'missing'. 2. Creation diff was lost: the truthy 'missing' string made the old `!expectedTarget?.identity` test fail, collapsing new-file writes into unknown and hiding the `--- /dev/null` diff. The worker now derives "approved missing" from targetType, with a behavior test asserting a missing-target write reports a creation diff. 3. mutates() removed; the executor's mutation gate and the T0-marker decision now use the single operationAccess authority. Verification: runtime+runtime-host typecheck clean; focused suites 71/71; full @maka/runtime test:dist 3080 pass / 0 fail; format:check clean; request shapes for both verifier scripts validated against the worker's schema (write/'missing' and read/'unchecked' parse; the pre-fix no-identity shape is rejected). The packaged Release Windows verifier and the macOS DMG verifier cannot run on this machine (no Windows runner, no packaged artifacts); their request construction is covered by the schema checks above. Generated-by: DSv4F-AstroHan
The client now rejects a write without an explicit expectedIdentity at runtime (#3487). Two intentionally-failing cases in verify-windows-sandbox-e2e.mjs wrote without one, so they were rejected by the parameter validation with invalid_request instead of reaching the sandbox path they were meant to exercise (parent-entry fail-closed, and the workspace-boundary path_denied). Pass 'missing' — the truthful T0 state for targets that do not exist — so the intended checks run again. Generated-by: DSv4F-AstroHan
jackwener
left a comment
There was a problem hiding this comment.
Independent review of d9831155c. Did not treat author self-review as evidence.
-
Problem. Optional
expectedIdentitymade “no T0 snapshot” look like “T0 missing”. Writes then failedpath_changedon existing files, and!identitylater flipped when'missing'became a truthy string. -
Solution. One required three-state wire field
{dev,ino} | 'missing' | 'unchecked'.operationAccessis the single write-kind list (includesapply_patch). Client rejects a write with no identity at runtime; reads send'unchecked'themselves. Worker CAS: object compares,'missing'+ T1-existing failspath_changed,'unchecked'skips.openStableTargettreats existing+no-object as unchecked open, missing as exclusivewx. -
Occam. One field, not a flag plus an identity. Checked the previous failure modes:
!identityon the wire is gone; apply_patch is a write; e2e negative cases pass a real T0 so they hit the permission gate, not identity validation.
Approve.
Summary
FilesystemWorkerExecuteInput.expectedIdentitywas optional. A caller that never captured a T0 snapshot (e.g.verify-windows-sandbox-e2e.mjs, which owns every path it writes) was indistinguishable from a caller whose T0 saw a missing target that then appeared while queued — so every write to an existing file without an identity failedpath_changed. This blocked the Release Windows check on four PRs (#3454 #3434 #3431 #3397) and could break any directFilesystemWorkerClientcaller.The fix makes the caller's T0 observation explicit and required, and carries it on the wire:
FilesystemWorkerExpectedIdentity = { dev, ino } | 'missing' | 'unchecked'(required). Every caller must decide which CAS contract it participates in — an absent field is no longer silently accepted.expectedTarget.identityis now one required three-state field (protocol v7):{dev, ino} | 'missing' | 'unchecked'— illegal combinations are unrepresentable on the wire. The worker fails a write only whenidentity === 'missing'and the target exists at execution time;'unchecked'skips CAS; identity objects still compare-and-swap.openStableTargetopens an existing target without an identity when the caller is'unchecked'(no comparison), leaving the create path (wx) exclusive and unchanged.FilesystemExecutorpasses its captured identity or'missing'for mutations and'unchecked'for reads; the runtime-host read-only adapter and the Windows sandbox e2e script pass'unchecked'.The #3001 race protection is not weakened: identity-present writes still CAS against the on-disk inode, and the "T0 missing, T1 existing" race still fails
path_changed.Fixes #3484
Verification
Ran (worktree
fix/3484-filesystem-worker-identityonorigin/mainf2722fd):npx tsc -p packages/runtime/tsconfig.json --noEmit— cleannpx tsc -p packages/runtime-host/tsconfig.json --noEmit— cleannpm run test:distin@maka/runtime— 3073 tests, 3060 pass, 0 fail, 13 skipped (opt-in docker/sandbox suites)'unchecked'write to an existing target passes and carries no identity on the wire'missing'with a T1-existing target still failspath_changed(client and worker paths)path_changednode --check scripts/verify-windows-sandbox-e2e.mjs— syntax clean (full Windows repro needs the Windows sandbox lane; unit-level equivalent covered by the new client test)npm run format:check— clean across the repo (1591 files);biome checkon all changed files — cleanpath_changedwith the replacement content untouchedNot run: the Windows-packaged release lane (no Windows runner here); the opt-in docker/Linux-sandbox suites.
AI use
Select exactly one:
Tool(s) and scope: DeepSeek V4 Flash (AstroHan-DSv4F, Pi runtime) — designed and implemented the fix, updated callers and tests. Commit carries the
Generated-bytrailer.Checklist
Does this PR entail a change in behavior?