fix(runtime): close the path-replacement window for filesystem mutations (#2600) - #3001
fix(runtime): close the path-replacement window for filesystem mutations (#2600)#3001chinawch007 wants to merge 10 commits into
Conversation
3e10fc8 to
0b45e87
Compare
abb61f6 to
ce0b045
Compare
📝 WalkthroughSummaryThis PR prevents filesystem mutations from acting on a path that changed after authorization. It captures device/inode identity before lock acquisition, validates identity before mutation, and detects replacement or disappearance after mutation. It applies this behavior to Write, Edit, FormatJson, ApplyPatch, and delete operations. The PR also distinguishes pre-dispatch failures from failures after worker dispatch. Post-dispatch failures now report an unknown filesystem outcome instead of implying that the mutation did not occur. Design and scopeThe PR extends the existing filesystem worker and executor flow. It adds the shared, I/O-free The solution is the smallest coherent design shown by the diff. Shared identity and outcome types prevent duplicated rules. Worker dispatch tracking is required to distinguish safe pre-dispatch failures from uncertain post-dispatch failures. Pre- and post-mutation identity checks are required to address both path replacement windows. No test or implementation area is an obvious candidate for deletion without reducing regression coverage. The tests cover authority classification, dispatch behavior, identity compare-and-swap behavior, post-write replacement, missing targets, and smoke-test integration. ValidationThe diff adds unit, protocol, runtime, worker, and smoke tests. The reported validation includes formatting, lint, typecheck, runtime build, and runtime distribution tests. Required checks remain unverified from direct execution evidence. Residual risks include:
Review-relevant risks
These protocol, public-contract, user-visible behavior, and security-relevant changes require independent human review under repository policy. The person performing the merge must review the final diff, and a maintainer makes the final determination. WalkthroughThe change adds filesystem target identity contracts, compare-and-swap checks, dispatch-aware failure classification, and unknown mutation outcomes. Tests cover writes, deletes, edits, patches, formatting, worker failures, and target replacement. ChangesFilesystem mutation authority
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR strengthens filesystem mutation safety and uncertain-outcome handling; the remaining concern is limited to a duplicate test, so no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant FilesystemExecutor
participant Filesystem
participant FilesystemWorkerClient
participant FilesystemWorker
FilesystemExecutor->>Filesystem: Capture target identity before lock wait
FilesystemExecutor->>FilesystemWorkerClient: Send mutation with expectedIdentity
FilesystemWorkerClient->>FilesystemWorker: Dispatch request
FilesystemWorker->>Filesystem: Validate identity and perform mutation
FilesystemWorker->>Filesystem: Verify post-write identity
FilesystemWorker-->>FilesystemWorkerClient: Return result or error
FilesystemWorkerClient-->>FilesystemExecutor: Return result with dispatched state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/runtime/src/filesystem-worker/client.ts (1)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared identity type.
expectedIdentityrestatesFilesystemTargetIdentityinstead of importing it frompackages/runtime/src/filesystem-authority.ts. Structural typing can hide a future contract divergence. Use a type-only import and declareexpectedIdentity?: FilesystemTargetIdentity.As per path instructions, use the existing source of truth for this contract.
Source: Path instructions
packages/runtime/src/__tests__/filesystem-target-identity.test.ts (2)
178-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate pre-write CAS test.
Lines 54-81 already replace the target after identity capture, assert
path_changed, and verify that the replacement remains unchanged. This test repeats the same behavior with weaker assertions.As per path instructions, flag tests that duplicate existing coverage.
Source: Path instructions
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse static filesystem imports in these tests.
packages/runtime/src/__tests__/filesystem-target-identity.test.ts#L47-L50: addstatandreadFileto the existingnode:fs/promisesimport, then remove the dynamic imports in the helpers and assertions.packages/runtime/src/__tests__/filesystem-worker-smoke.test.ts#L85-L94: addlstatto the module-levelnode:fs/promisesimport, then remove the local dynamic import.As per path instructions, flag concrete cases where code can be deleted or simplified.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15a7be29-c845-4400-8afd-efa2a36c5fea
📒 Files selected for processing (13)
packages/runtime/src/__tests__/filesystem-authority-contract.test.tspackages/runtime/src/__tests__/filesystem-mutation-outcome.test.tspackages/runtime/src/__tests__/filesystem-target-identity.test.tspackages/runtime/src/__tests__/filesystem-worker-client.test.tspackages/runtime/src/__tests__/filesystem-worker-linux-smoke.test.tspackages/runtime/src/__tests__/filesystem-worker-smoke.test.tspackages/runtime/src/__tests__/filesystem-worker.test.tspackages/runtime/src/filesystem-authority.tspackages/runtime/src/filesystem-executor.tspackages/runtime/src/filesystem-worker/client.tspackages/runtime/src/filesystem-worker/operations.tspackages/runtime/src/filesystem-worker/process-runner.tspackages/runtime/src/filesystem-worker/protocol.ts
ce0b045 to
c0b8278
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/runtime/src/__tests__/filesystem-target-identity.test.ts (1)
173-195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove this duplicate pre-write CAS test.
Lines 54-81 already replace the target before a
writerequest and assertpath_changed. That test also verifies that the replacement content remains unchanged. This test does not add observable coverage.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e767a2a2-3a9b-4dc8-85fa-e6e4f1cb3143
📒 Files selected for processing (2)
packages/runtime/src/__tests__/filesystem-mutation-outcome.test.tspackages/runtime/src/__tests__/filesystem-target-identity.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/runtime/src/tests/filesystem-mutation-outcome.test.ts
|
Thanks for the fix — the race is real (an external Bash/process can swap the path between lock-key resolution and the actual write, Delete can remove the replacement, and a worker disconnect leaves the outcome unknown), and the three-claim contract (T0 identity capture before entering the write-lock queue, pass-through client, worker-side CAS before write with Conclusion: PASS with two P2s that need an explicit decision or deferral note. P2-1 — the T0-CAS misfires on lock-ordered cooperative changes: the missing↔existing transition returns an internal error where the previous semantics were "last writer wins". Identity is captured unconditionally at T0 (even when the path existed before queuing), forwarded verbatim, but P2-2 — mid-write failures (truncate-then-ENOSPC/EIO) are classified as a clean P3 (optional): the "type-level" tests in AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS（2 个 P2 需显式决策或延后说明）。竞态真实（外部进程可在"锁键解析→实际写入"之间替换路径、Delete 可删掉替换物、worker 断连后结果未知），T0 锁前捕获身份→client 原样透传→worker 写入前 CAS（path_changed）→写后孤儿检查（outcome_unknown）+ dispatched 标志（区分"从未启动"与"跑过丢结果"）三诉求全部真实落地（T0 捕获确在 withFileWriteLock 之前、透传原样、CAS 在写前）。headline 场景（排队期被替换）真实关闭；~500 行生产代码不过量（分类器/dispatched/契约模块都有实重），~60 行测试可删。P2-1：T0-CAS 误伤锁序协作变更——身份在 T0 无条件捕获、targetType 却在 T1（拿锁后)重新派生，同路径 Delete+Write 竞态（Delete 先拿锁 unlink，Write T1 见 missing 但仍带身份 X）命中协议 superRefine（missing 目标不得带身份）→ invalid_request（"Filesystem worker request was invalid."），Create 后排队的 Edit/Write 同理；"删掉重写"是模型合法意图且正是写锁存在的场景。需决策：豁免锁内在途身份（如 T1 重捕获基线或 missing+identity 返回 path_changed 而非 invalid_request）+ 补并发 Delete+Write 测试断言要么干净应用要么有意义的 path_changed，绝不能 invalid_request。P2-2：写入中途失败（O_TRUNC 截断后 ENOSPC/EIO）被分类为干净 rejected——'w' 旗标先截断再写，中途错误映射为普通 filesystem_error 且不在 UNKNOWN_OUTCOME_REASONS 里，模型被告知"什么都没发生"而文件实际已截断/半写——正是 PR 自己定义的 unknown 类且 post-write 检查兜不住（writeFile 抛错后检查不执行）。把"已开始写"的失败映射为 outcome_unknown。P3（可选）：类型级测试近乎同义反复（数组等于自身，~60 行可删）；两处重复 catch/classify 块应抽公共 helper；内部协议错误串直接面向模型应改用户可懂；头注释引用本 PR 不存在的 file-stable-write.ts；Windows 上 stat 若返回恒定 dev/ino（部分文件系统为 0）CAS 静默变 no-op（保护不可验证，smoke 在 Windows skip）；format_json 无变化仍跑 post-write 检查；E2E 从未组合"真实锁队列+真实 worker+排队期替换"；worker_crashed+dispatched=true 粒度粗（读请求前崩溃把所有变更标 outcome_unknown，安全但持续损坏时噪音大）。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Capturing T0 identity before lock wait and conservatively classifying post-dispatch failures are useful pieces, but the mutation itself still reopens the pathname. The implementation can therefore detect corruption after it happens without preventing it, and the local/workspace path bypasses the new authority entirely.
The first-principles/Occam solution is one shared fd/handle-pinned mutation primitive: capture and validate the approved object, perform read/transform/write through that same descriptor, use exclusive creation for an approved missing target, and make both worker and local backends consume that contract. Post-checks then describe host visibility rather than compensate for a second pathname lookup.
Review performed with three Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I reproduced both P1 paths against the latest head and verified green CI.
中文评论
在等待锁前捕获 T0 identity，以及对 post-dispatch failure 做保守分类，都是有效基础；但 mutation 本身仍重新按 pathname 打开。因此当前实现只能在破坏发生后检测，无法阻止 replacement 被写坏；local/workspace 路径还完全绕过了新 authority。
更符合第一性原理和奥卡姆剃刀的方案是单一、共享的 fd/handle-pinned mutation primitive：捕获并验证批准对象，通过同一 descriptor 完成 read/transform/write；批准目标缺失时使用 exclusive create；worker 与 local backend 都消费这一契约。此时 post-check 只负责描述 host 可见性，不再弥补第二次 pathname lookup。
本次审查使用了三位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已在最新 head 上复现两个 P1，并复核绿色 CI。
|
/agentic_review |
Code Review by Qodo
1. CAS does not pin writes
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
c0b8278 to
0bad3c8
Compare
…ions (apache#2600) A filesystem mutation whose worker fails after dispatch may already have landed on disk, but the host reported it as a generic error. The model had no way to tell "the write may have happened" from "nothing ran", so it treated a half-applied mutation as a clean failure. Close that gap for the disconnect concern: - process-runner tracks a `dispatched` flag (set once Node's 'spawn' event fires and stdin is written) and surfaces it on both the resolved result and the rejection error. - client splits the ambiguous launch failures by that flag: `spawn_failed` (never started, nothing could have been written) vs new `worker_io_incomplete` (ran but the result was lost). `aborted` keeps its reason but carries `dispatched` so a pre-flight cancel is distinct from a post-dispatch kill. Worker-response failures are marked dispatched. - protocol gains an `outcome_unknown` error code so the worker can report "I may have applied this before I lost the ability to answer". - the boundary executor converts a mutating op that fails with a post-dispatch reason into ToolOutcomeUnknownError, which already flows to a structured uncertainOutcome result. Reads and pre-flight failures pass through unchanged. This is the first of four commits addressing apache#2600; it independently closes the "post-dispatch unknown outcomes" concern. Generated-by: ZCode
Issue apache#2600 asks for the filesystem-authority contract to live in one place, separate from the individual editing tools. Introduce that module as pure types and a classifier, with no I/O. - FilesystemTargetIdentity: opaque decimal-string dev/ino. String rather than bigint because bigint cannot cross the worker's JSON protocol boundary, and identity is only compared for equality, never used to build a path. - FilesystemTargetDescriptor: a discriminated union so the "no identity to compare" case is an explicit `missing` arm, never an accidentally-absent optional field. A future "skip the identity check" change cannot compile without handling `missing`, which closes the "no identity -> CAS passes" regression class. - FilesystemMutationOutcome: applied | rejected | unknown. - classifyFailedMutationOutcome + UNKNOWN_OUTCOME_REASONS: moved out of filesystem-executor.ts so the executor consumes the contract instead of restating it. The classifier's invariant (every member reason is semantically dispatched, so membership alone suffices; only `aborted` straddles pre-flight/post-dispatch and gates on the flag) is documented at the contract. This is the second of four commits for apache#2600. It is types plus a behaviour- neutral refactor; the descriptor and identity are wired into the worker protocol and the fd-pinned read-modify-write in the following commit. Adds filesystem-authority-contract.test.ts covering the classifier branches and the type-level constraints. Generated-by: ZCode
…pache#2600) Close the queue window for issue apache#2600 concern apache#1: a path replaced while a mutation waits for the write lock must be detected, not silently written. The identity is now captured at lock acquisition (T0) — before the call enters the lock queue — not re-derived inside client.execute after the lock is held (T1). Re-deriving at T1 would sample the post-replacement inode, making the CAS self-fulfilling and re-opening the window. - protocol v6: FilesystemWorkerTargetSchema gains an optional identity {dev, ino} (opaque decimal strings; bigint cannot cross the JSON boundary). superRefine rejects a missing target carrying an identity. - filesystem-executor: writeLockTarget returns {key, canonicalPath}; captureIdentityAtLockAcquisition stats the canonical path at T0; run() receives expectedIdentity and passes it to worker.execute; the FilesystemWorkerExecuteInput carries an expectedIdentity field. - client: deleted captureTargetIdentity (the T1 capture); client.execute uses the caller-supplied expectedIdentity verbatim. - worker assertTargetUnchanged compares the on-disk inode against the T0 identity (follow/entry stat modes match the targetType derivation); a non-missing WRITE target must carry an identity or the request is rejected (reads are exempt — they do not mutate). - post-write orphan check: assertPathStillMatchesIdentity re-stats the path after a write and reports outcome_unknown if the inode no longer matches (the write went to an orphaned inode; the visible file is the replacement). Red-line test verifies the worker receives the T0 identity (before a replacement), proving the capture happens at lock acquisition. Direct unit tests cover the post-write orphan check (match / replaced / disappeared). This is the third of four commits for apache#2600. Generated-by: ZCode
…pache#2600) Commit 3 added the post-write identity check to the write operation only. Extend it symmetrically to edit, format_json, and apply_patch update so a path swapped during any read-modify-write is reported as outcome_unknown, not a misleading success. The delete path is already covered by the T0 identity CAS in assertTargetUnchanged (it runs for every operation, uses lstat for the directory-entry semantics that create/delete use, and rejects with path_changed when the inode mismatches). format_json's invalid-JSON branch already returns ok:false without writing. No additional changes needed for either. Adds red-line tests proving a delete and an edit whose target was replaced after authorisation are rejected (path_changed) and the replacement file is left untouched. This is the fourth and final commit for apache#2600. Generated-by: ZCode
apache#2600) The T0 identity CAS (b51c6a5) made the worker refuse a write mutation on an existing target that carries no identity. The macOS smoke test was updated to pass one, but the Linux smoke test's Edit call was missed — it runs only on linux+bwrap, so local runs never execute it and only CI's Ubuntu runner hit the refusal. Generated-by: ZCode
…e behavior (apache#2600) Two test-quality fixes from review: - The T0 identity test replaced the path inside the worker, i.e. after the lock had been granted — a regression that captured the identity at T1 (post-lock) would still pass, because the replacement happened after any T1 capture point. Rework it to exercise the real queue window: a first mutation blocks inside the worker while holding the path's write lock, a second mutation queues behind it, the path is replaced while the second waits, then the gate releases. Assert the second worker call receives the pre-replacement dev/ino. A T1 capture now samples the replacement's inode and fails the assertion. - The missing-target create test used an empty patch and only asserted the error was not path_changed — it accepted invalid_request, which is exactly what an over-broad mandatory-identity check would throw for missing write targets. Use a valid create patch, assert success, and assert the created file content, so that regression fails loudly. Generated-by: ZCode
…#2600) The previous T0-identity CAS validated the target and then re-opened the pathname, so a swap between validation and the open could still divert the write onto the replacement — detection after the fact, not prevention. The local/workspace path additionally bypassed the identity authority entirely. Introduce file-stable-write.ts, the fd-pinned mutation primitive both backends now consume, and reconcile the cooperative missing↔existing transitions that previously surfaced as invalid_request: - openStableTarget: open the approved object once — 'r+' with O_NOFOLLOW for existing targets (identity validated by fstat on the descriptor, BEFORE any truncation, so a rejected validation leaves the file intact; write-only targets fall back to O_WRONLY), 'wx' for approved-missing targets (a file that appeared in the gap is path_changed, never truncated). writeThroughHandle truncates and writes at position 0 through the pinned descriptor; ENOSPC/EIO/EDQUOT/EFBIG surface as outcome_unknown — a half-written file is not a clean failure (apache#2600 review P2-2). hostVisibilityAfterWrite describes whether the path still resolves to the pinned inode after the write. - worker: write/edit/format_json/apply_patch-update run read/transform/ write through the pinned handle; format_json's invalid-JSON branch still returns ok:false without writing. - local: LocalWorkspaceExecutor gains readModifyWrite (optional on the workspace interface; remote/isolated workspaces keep the path-based fallback, documented as unprotected). Identity capture at lock acquisition no longer depends on a worker being wired (apache#2600 review P1-2), and apply_patch update resolves the existing-target requirement before any create runs. - client: the missing↔existing transitions while queued are reconciled against the T1 reality — a stale identity on a vanished target is dropped so "delete then rewrite" stays a clean apply, and a write whose target appeared while queued fails with a meaningful path_changed, never invalid_request (apache#2600 review P2-1). The duplicated mutation catch blocks collapse into settleMutationFailure, which also maps the primitive's StableWriteFailure codes. - tests: deterministic race for the pin (swap between validation and the write; bytes land on the original inode, replacement untouched), wx gap-creation rejection, both client transition directions, and the lock-serialisation causal barrier moved onto readModifyWrite. The tautological type-level contract tests are removed. Generated-by: ZCode
…#2600) The delete path could remove a replacement: the entry identity was checked and then fs.unlink(path) ran against the pathname, so a rename between the check and the unlink deleted the replacement while the operation reported { ok: true } — a silent unauthorized deletion with no post-operation validation (apache#2600 review: "delete can remove replacement"). POSIX has no atomic compare-and-unlink, so prevention by unlink is impossible; make the capture atomic instead. compareAndDeleteEntry renames the entry to a private unpredictable tombstone in the same directory, verifies the tombstone carries the approved identity (lstat — works for regular files and symlinks alike), and only then unlinks the tombstone. A mismatch means a replacement was installed in the window: it is renamed back to the path — restored, not deleted — and the operation reports path_changed with a message saying so. If the final tombstone unlink fails after a verified match, the entry is preserved at the tombstone and the failure is reported as outcome_unknown, never as success. rename(2) moves the directory entry itself and needs no permission on the file (only write+execute on the parent, exactly like unlink), so deleting read-only/write-only files keeps working. Wired into both backends: the worker's apply_patch delete, and the local executor's applyPatch delete with the approved identity threaded through WorkspaceApplyPatchInput. Tests: plain removal leaves no tombstone; a replacement installed after the check is restored byte-for-byte and reported path_changed; a symlink entry is deleted by its own identity without following the link. Generated-by: ZCode
224d0e3 to
58c3f13
Compare
|
Thanks for your code review feedback. I've updated the code based on your comments and manually resolved all of them, but the CI didn't re-trigger. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier filesystem-authority feedback. I re-reviewed exact head 58c3f137018e3ae2072925745d34d51cb68b81e1: all seven historical threads are resolved/outdated, the T0 identity and fd-pinned mutation direction is substantially stronger, the AI provenance is complete, and there is no UI/UX screenshot requirement.
I found two new delete-path issues and added them inline below. The exact-head Windows sandbox also currently fails because its pre-existing-file smoke write does not pass the newly required identity; that test should lstat the file and supply { dev, ino }. The remaining workflows must finish green after the fixes.
Non-blocking documentation cleanup: the body still says four commits and lists obsolete short SHAs, while the current branch has eight commits.
AI-assisted review disclosure: OpenAI Codex performed the exact-head race, deletion, review-thread, provenance, and CI analysis; I verified the reproductions, severity, smallest fixes, and live GitHub state before posting.
中文说明
旧反馈均已落实，但删除路径还有两个新问题：恢复 tombstone 时可能覆盖并删除并发创建的新 replacement；对目录执行删除会先把目录移进隐藏 tombstone，再因 unlink 失败而让原路径消失。Windows sandbox 也因 smoke test 没传新要求的 identity 而失败。修复后需重跑全绿。
…pache#2600) Two delete-path fixes from review of the compare-and-delete primitive: - The restore could destroy a concurrent newcomer: after the tombstone captured replacement C, another process creating B at the original path meant the rename-based restore atomically overwrote B — the exact class of replacement loss this module exists to prevent. Node exposes no RENAME_NOREPLACE, but link() is natively no-replace: the restore now links the tombstone back and drops the tombstone name, guarding the platform wrinkle where link() follows a symlink source by verifying the restored inode against the tombstone before dropping it. On EEXIST — or any link failure — the tombstone is preserved and the failure reported as outcome_unknown with the location, so nothing is ever lost. - A directory entry was moved into the tombstone and then stranded there: renaming a directory succeeds, but the tombstone unlink cannot (EISDIR/EPERM), so the directory vanished from its path and hid under a stray name where the previous plain unlink simply failed. Directories are now rejected up front, before anything is moved. Also fixes the exact-head Windows sandbox smoke: its pre-existing-file write now supplies the identity captured at lock acquisition (lstat + { dev, ino }), as the boundary executor does in production. Tests: a reoccupied path preserves both the newcomer and the captured entry; a free path restores the captured entry without a tombstone leak; a directory is rejected untouched. Generated-by: ZCode
|
Again, updated code and resolved comment. Please help reviewing, thanks. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on the delete path. Re-reviewed exact head 3edf983d0f42bcdb4ed5efe3c31c824773417f15; the new delta since my last review is one commit over three files, so this pass is scoped to it plus a re-check of the two findings I raised.
Both are addressed in substance: the tombstone restore no longer clobbers a concurrent occupant, and the directory case is refused before anything moves. The Windows smoke test now supplies the identity the write path requires. The remaining problem is that the new no-replace restore only actually restores one entry type. link() cannot recreate a directory at all, and on macOS it dereferences a symlink source, so for those two entry types the restore either cannot run or produces the wrong object — which matters here because compareAndDeleteEntry documents symlink support as a design property and the whole module exists to keep an adversarial swap from losing an entry. Both are inline, plus one diagnostic point.
I have not re-run the suite for this delta; the reproductions below are the link() platform behaviour, checked directly on darwin, and code reading at this head. No CI checks are currently reported for this head.
Review disclosure: this review was prepared with Claude Code, which read the delta, traced the delete call sites and their error classification, and ran the link() platform probe quoted inline. The human contributor reviewed the findings before posting.
| */ | ||
| export async function restoreTombstoneNoReplace(tombstone: string, path: string): Promise<void> { | ||
| try { | ||
| await link(tombstone, path); |
There was a problem hiding this comment.
[P2] Restore the captured entry itself, not whatever it points at. link() dereferences a symlink source on darwin, so when the captured entry is a symlink this creates a hardlink to the symlink's target at the original path rather than restoring the link. I checked the behaviour directly on darwin: linkSync('alink', 'restored') succeeds and lstat('restored').isSymbolicLink() is false with a different inode. The identity guard below then reports outcome_unknown and deliberately touches nothing, so the outcome is the worst of both — the original symlink stays hidden under the tombstone and the path is now occupied by a new regular-file entry aliasing the symlink's target, which any later read or write will silently edit. Note the guard can safely clean up: link() only succeeds when the destination did not exist, so an entry found at the path afterwards is necessarily the one this call created. Either use linkat semantics that do not follow, or detect the symlink case before restoring and unlink what this call created before reporting; add a regression that deletes a symlink whose identity changed in the window and asserts the path is not left holding a foreign entry.
| // the tombstone succeeds for directories, but the tombstone unlink then | ||
| // fails and leaves the directory hidden under the tombstone name — strictly | ||
| // worse than the plain unlink, which failed without moving it. | ||
| const entryBefore = await lstat(input.path); |
There was a problem hiding this comment.
[P2] Make the directory refusal survive the race this module is about. This lstat is a pre-check, not an enforcement point: if the entry becomes a directory between here and the rename below, the directory is captured into the tombstone anyway, the identity comparison fails, and the restore cannot run because link() on a directory fails with EPERM — I confirmed that on darwin. The result is exactly the outcome the check was added to prevent: the path is empty and the directory survives only under the hidden tombstone name, now reported as outcome_unknown. Since the capture is the atomic step, the enforcement belongs after it — check the captured tombstone's type and, for a directory, rename it straight back before any identity comparison — with a regression that forces a directory into the tombstone rather than only asserting the pre-check.
| // worse than the plain unlink, which failed without moving it. | ||
| const entryBefore = await lstat(input.path); | ||
| if (entryBefore.isDirectory()) { | ||
| throw new Error('Refusing to delete a directory through the entry-delete path.'); |
There was a problem hiding this comment.
[P3] Throw the failure type this path's callers understand. A bare Error reaches normalizeOperationError in filesystem-worker/operations.ts, which matches FilesystemOperationError, StableWriteFailure and ApplyPatchRejectedError, then falls through to the generic operationError('filesystem_error', 'Filesystem operation failed.') — so the model is told the operation failed and never learns that a directory was refused or what to do instead. Raise it as a structured failure with its own code so the message survives classification.
…efusal (apache#2600) Three delete-path fixes from review of the no-replace restore: - Restore the captured entry itself, not whatever it points at: link() dereferences a symlink source on darwin (implementation-defined per POSIX; verified directly on darwin), so a link-based restore of a captured symlink would plant a regular-file alias of the TARGET at the path — a foreign entry later reads/writes silently edit, while the original link stays hidden on the tombstone. The restore is now type-aware: symlinks are recreated with symlink(readlink(...)) — natively no-replace (EEXIST), round-trips the target string exactly — and only regular files use link(), which cannot misbehave for them. The inode guard stays deliberately hands-off on mismatch: after a concurrent rename-over, the entry at the path may be a foreign one, and unlinking it would destroy third-party data — the exact loss class this module prevents. - Make the directory refusal survive the race this module is about: the lstat pre-check is not an enforcement point, so a directory swapped in between the check and the capture was moved onto the tombstone and stranded there (link() cannot restore a directory; EPERM). Enforcement now lives after the atomic capture: the captured entry's type is checked first, and a directory is renamed straight back before any identity comparison. The post-capture logic is extracted as deleteCapturedTombstone for the forced-directory regression. - Throw the failure type callers understand: the bare directory-refusal Error fell through normalizeOperationError to a generic filesystem_error, so the model never learned what was refused. The refusal is now a StableWriteFailure with its own is_directory code, added to the worker protocol's error enum so the message survives classification on both backends. Tests: a swapped-in symlink is restored as a symlink pointing at its own target — the path is never left holding a foreign regular file; a directory forced onto the tombstone is renamed back, not stranded; the worker surfaces is_directory with the refusal message end to end. Generated-by: ZCode
|
One more turn, thanks for your attention. |
2 participants
Summary
Closes #2600. Built directly around the three concerns the issue names, plus the "shared filesystem-authority contract, kept separate from individual editing tools" the author asked for. Four commits, each independently healthy (builds and tests green at every step):
Commit 1 —
26f7fa2feat(runtime): surface worker disconnect as unknown outcome for mutationsAddresses "worker disconnect may leave the outcome unknown".
A filesystem mutation whose worker fails after dispatch may already have landed on disk, but the host reported it as a generic error — the model had no way to tell "the write may have happened" from "nothing ran", so it treated a half-applied mutation as a clean failure.
process-runnertracks adispatchedflag (set once Node's'spawn'event fires and stdin is written) and surfaces it on both the resolved result and the rejection error.clientsplits the ambiguous launch failures by that flag:spawn_failed(never started, nothing could have been written) vs newworker_io_incomplete(ran but the result was lost).abortedcarriesdispatchedso a pre-flight cancel (ESC) is a clean cancel, not an unknown outcome — a single ESC would otherwise mark every queued file tooloutcome_unknownwithretrySafe: false.outcome_unknownerror code so the worker can report "I may have applied this before I lost the ability to answer."ToolOutcomeUnknownError, which flows to the existing structureduncertainOutcomeresult. Reads and pre-flight failures pass through unchanged.Commit 2 —
aae3138refactor(runtime): extract filesystem-authority contractImplements "Define a shared filesystem-authority contract … Keep this separate from individual editing tools."
New
packages/runtime/src/filesystem-authority.tsdeclares the contract types, imported by the executor, worker, and workspace-executor alike — types and a pure classifier only, no I/O:The identity is a decimal string, not bigint, because bigint cannot cross the worker's JSON protocol boundary (
JSON.stringifythrows on a BigInt), and identity is only compared for equality. The descriptor is a discriminated union so "the target had no identity to compare" is an explicitmissingarm — a future "skip the identity check" change cannot compile without handling it, which compiles the "no identity → CAS passes" regression class out of existence.Commit 3 —
b51c6a5feat(runtime): capture target identity at lock acquisition for CASAddresses "Write/Edit/FormatJson/ApplyPatch may write to the replacement" — the core concern.
Root cause:
expectedTargetwas computed after the write lock was acquired, not when the lock key was resolved. The lock serializes concurrent Maka tools, but it does nothing while a call is queued, and Bash is not serialized at all — so in the window between lock resolution and the actual write, an external process could replace the path and we mutated whatever we then found.captureIdentityAtLockAcquisitionin the boundary executor. The stat mode matches the targetType derivation: content operationsstat(follow), create/deletelstat(pin the directory entry, so a swapped link is caught against the link's own inode).FilesystemWorkerTargetSchemagains the optionalidentityfield;superRefinerejects a missing target carrying one.client.executereceives the T0 identity and passes it verbatim — it does not re-derive it (re-deriving after the lock is held would sample the post-replacement inode and make the CAS self-fulfilling).assertTargetUnchangedcompares the on-disk inode against the T0 identity and rejects withpath_changed. A non-missing write target must carry an identity or the request is rejected outright — the CAS cannot be silently skipped (reads are exempt; they do not mutate).assertPathStillMatchesIdentity): after a write, if the path's inode no longer matches the one written, the bytes went to an orphaned inode and the visible file is the replacement — reported asoutcome_unknown, not a misleading success.So: a path replaced while queued →
path_changed, the replacement is untouched. A path replaced mid-write →outcome_unknown, the model is told to re-read.Commit 4 —
2d82471feat(runtime): extend post-write orphan check to edit/format/updateExtends the commit-3 post-write check symmetrically to
edit,format_json, andapply_patch update. Delete is already covered by the T0 CAS (it runs for every operation; the symlink entry case useslstaton the link's own inode), andformat_json's invalid-JSON branch already returnsok:falsewithout writing.Honesty about what this does not close
Recorded as documented residuals rather than over-claimed:
lstat-compare +unlinknarrows the window rather than closing it. (We deliberately do notopen()before unlink —open('r+')needs read+write permission on the file whileunlinkonly needs the parent dir, so an open precondition would break deleting read-only/write-only files.)echo x >> file): the inode never changes, so CAS passes; out of scope (content-level detection).file-write-lock.ts, and CAS cannot catch two RMWs on one inode. Left as separate work.path_changed: content matching anoldStringproves the snippet exists, not that it is the same file — retrying would reproduce the exact defect this issue describes (mv other.ts target.tswhere both contain the string). We return a clear "the target was replaced while queued; re-read before editing" and let the model doRead→Edit.New tests added (29 total):
filesystem-mutation-outcome.test.ts(14)worker_crashed,worker_io_incomplete,timeout,response_overflow,invalid_response,response_id_mismatch,response_kind_mismatch,outcome_unknown) converts a mutating op toToolOutcomeUnknownError.spawn_failed(never dispatched) does not convert — nothing could have been written.abortedbefore dispatch is a clean cancel; only after dispatch is it unknown.filesystem-target-identity.test.ts(9)path_changed, and the replacement's content is not overwritten.path_changed, and the replacement file is not deleted.path_changed, and the replacement content is untouched.outcome_unknown; path disappeared →outcome_unknown.filesystem-worker-client.test.ts(3)worker_io_incompletewithdispatched: true.spawn_failedwithdispatched: false.filesystem-authority-contract.test.ts(9)MutationOutcomeis the three-valued set.Verification
Rebased on latest
main; every check run against a clean-mainbaseline to confirm nothing pre-existing was disturbed.npm run format:check— clean (8 formatting nits auto-fixed and folded in)npm run lint— 2304 files, no issuesnpm run typechecknpm --workspace @maka/runtime run build— cleannpm --workspace @maka/runtime run test:dist— 2983 tests, 2971 pass, 12 skipAI use
Select exactly one:
Tool(s) and scope: ZCode (AI coding agent) implemented the runtime, filesystem-worker
protocol, filesystem-authority contract, and test changes; it also designed the
implementation plan through review iterations, resolved rebase conflicts against main,
and ran verification (format, lint, typecheck, build, and the runtime test suite with
clean-main baselines). The human contributor of record set the scope decisions, relayed
external review feedback for evaluation and incorporation, reviewed and approved each
commit, and chose to submit the result. AI review does not constitute the independent
human review this change (runtime behavior) requires.
Checklist
Does this PR entail a change in behavior?