Uh oh!
There was an error while loading. Please reload this page.
feat(runtime): complete Phase 3A file-transaction recovery - #1346
Conversation
likun666661
commented
Jul 22, 2026
这里有一个我认为需要在合并前调整的 blocker:我不接受当前基于目标文件内容中 当前判断: oldMatches===0&&(newString.length===0||newMatches===1)只能描述文件现在长什么样,不能证明当前状态是由这次 Edit 产生的。例如文件原本就包含一次 这不是补几个字符串边界条件可以修好的问题,而是 recovery evidence 本身不足。以当前 live file 内容反推操作因果关系,复杂度和误判面都会持续扩大。 我建议把本地 Write/Edit 收敛为一个细粒度的、可恢复的文件事务:
也就是说,建议删除基于 建议补齐 checkpoint durable 前后、temp write、fsync、replace 前后、replace 后但 T2 前再次崩溃,以及外部 drift 等 crash matrix 测试后再合并。 |
Using RuntimeEvents as the recovery source and committing the reconcile result, synthesized response, and recovery decision in one SQLite bundle makes sense. The file observer is bounded by workspace, file type, size, and UTF-8 checks, and planning and execution share the same recovery contract registry. I would keep those parts. Two blockers remain. First, the production Second, I agree with the existing comment that The atomic SQLite bundle can stay. The missing production wiring and insufficient file evidence should be resolved before merge. |
# Conflicts: # packages/storage/src/__tests__/sqlite-session-metadata-store.test.ts
# Conflicts: # packages/runtime/src/__tests__/session-manager.test.ts # packages/runtime/src/runtime-kernel.ts # packages/runtime/src/runtime-resume.ts
# Conflicts: # packages/storage/src/__tests__/sqlite-session-metadata-store.test.ts
likun666661
commented
Jul 23, 2026
Thanks for the update. I re-reviewed the new head ( I still see three blocking correctness/security issues:
One additional hardening issue: So: the revised architecture resolves my previous objection, but I would still block approval until the three issues above are addressed. |
Uh oh!
There was an error while loading. Please reload this page.
Three additional P2 findings on the current head, separate from the file-transaction issues already covered above:
I’m grouping these in one comment because they do not duplicate the existing blockers and do not need separate inline threads, but each should be covered by a focused regression test in the next revision. |
…e-phase3a # Conflicts: # apps/desktop/src/main/app-lifecycle.ts # packages/cli/src/runtime-bootstrap.ts # packages/runtime/src/session-manager.ts # packages/storage/src/__tests__/sqlite-session-metadata-store.test.ts # packages/storage/src/agent-run-store.ts
zhiiw
commented
Jul 23, 2026
Addressed in
The revision also makes continuation admission atomic per source boundary across store/process instances, keeps the interactive workspace writer exclusive, and prevents cancelled source runs from entering automatic file reconciliation. Validation after merging the latest |
zhiiw
commented
Jul 23, 2026
Follow-up to the review on head
Focused verification on the merged head: 36 file-transaction/ToolRuntime tests passed (31 executed on Windows, 5 POSIX-only mode/hard-link/symlink cases correctly skipped here and retained for Linux/macOS CI), with no failures. |
likun666661
commented
Jul 23, 2026
Thanks for the follow-up. I re-reviewed the current head ( I found two new blocking issues and one Windows cleanup issue in the revised boundary.
The latest typecheck, test, and e2e checks are green, and the earlier corruption/continuation/event-order fixes also look present. I would still block approval on items 1 and 2. |
zhiiw
commented
Jul 24, 2026
Follow-up to the latest blocking review on head 8bfc5ed: the two blockers and the Windows cleanup issue are addressed in 52b9ae9; the branch is now at dde9bb8 after merging current upstream/main and adapting the recovery tests to the new ToolRuntime settlement API.
I also moved the before_replace failpoint ahead of the final state observation and added a drift regression, so an external mutation injected at that boundary parks instead of being overwritten. Verification on the upstream-synced head:
The architecture contract now records the trusted-root rule, no-follow observation protocol, worker-owned finalize ordering, and the requirement that verified cleanup complete before a synthesized response is committed. |
zhiiw
commented
Jul 24, 2026
Final verification update for the latest blocking review: the detailed resolution is in #1346 (comment), and the current head is One Linux-only regression surfaced after that reply: recovery's trusted-path identity check was dereferencing the final path component, so a target replaced by a symlink parked early as Current CI is fully green on the updated head:
The PR is mergeable against current upstream/main with no unresolved review threads. |
Astro-Han
commented
Jul 24, 2026
Thanks for the substantial work here. The overall direction is sound: recovery should be based on durable causal evidence, runtime events should remain the canonical record, SQLite projections should be rebuildable, and reconciliation should be serialized. My main recommendation, however, is to convert #1346 back to Draft and replace it with a small sequence of focused PRs rather than continuing to layer fixes onto this diff. This is not primarily about the line count. The current PR combines several independent correctness claims:
Each of these has a different authority boundary, failure model, and rollback unit. Reviewing them together means proving all of their interactions at once. It also means that a regression in one area could require reverting unrelated, otherwise-correct work. I would keep this PR as the umbrella/design record, mark it Draft, and land the production work through sequential PRs based on the latest
I would not carry the Phase 4 Git carrier, restricted verifier, or speculative retry/reattach modes into these initial PRs until a production consumer requires them. The slices should be divided by independently testable invariants, not mechanically by file or commit. Prefer flat PRs from the latest FindingsP1 — The final drift check and file replacement are not atomicIn Another process can modify the target after the final inspection but before the rename. The recovery operation then silently overwrites that newer external content and reports success. I reproduced this by inserting an external write after the last inspection and before the rename; the external content was replaced by the agent’s prepared content. This is the one finding I would keep at P1 because it can cause irreversible loss of a concurrent external update. Another The commit path needs a real compare-and-swap-style guarantee, such as an exchange/displaced-inode verification strategy that can restore or preserve the competing version. If the platform cannot provide that guarantee, automatic redo should stop safely instead of overwriting the target. P2 — A completed decision can exist without a matching durable outcomeThe public split writers in The resolver then treats a schema-valid This is not currently demonstrated on the normal atomic production path, so I would classify it as P2 rather than P1. It is still a contract hole: a crash, importer, migration, or external caller using the exposed writers can create a state that is interpreted as safely completed even though the provider-facing outcome is missing. There should be one authority for writing the reserved recovery bundle. Storage, rebuild, and resolver validation should reject mismatched execution identity and require:
P2 — Preparation-time business errors bypass normal tool settlementPrepared Write/Edit performs validation before the runtime enters its normal tool-error handling path. Expected business errors—such as a missing Edit target, a missing or non-unique These errors mean “the tool was definitely not dispatched”; they are not effect-uncertain runtime failures. They should settle through the normal provider-facing tool-error path. Only durable commit failures or genuinely uncertain effects should reject execution into recovery handling. P2 — macOS path aliases can silently disable checkpointingThe checkpoint carrier compares lexical and canonical paths in a way that rejects valid aliases such as
That breaks the central invariant: durable T1 evidence can request reconciliation even though no checkpoint was created. The prepared operation also returns the canonicalized path, which changes the existing result contract for callers that supplied an alias. Path identity should be resolved once through a single authority. An unsupported prepared target must fail before T1 is persisted; it must not silently fall back after reconciliation has already been selected. P2 — Atomic replacement changes existing file semantics and drops metadataThe implementation creates a temporary file, applies a mode, and renames it over the target. That preserves content atomicity, but it does not preserve all existing file semantics. On macOS, I verified that extended attributes disappear after a prepared Write. ACLs, ownership, group, Finder/quarantine metadata, and platform-specific streams may also be replaced with those of the temporary file. Read-only-target and parent-directory permission behavior can differ from the previous I would classify this as P2 because it is deterministic but limited to files carrying those attributes or permission arrangements. It nevertheless needs resolution before the prepared-file slice merges: either preserve the required metadata and semantics, or reject ineligible targets before durable preparation. P2 — Continuation cursors include mutable partial snapshots
A partial streaming snapshot is presentation state, not an immutable ledger position. If it is counted as durable history, later continuation or replay can skip, duplicate, or misclassify events. Continuation identity should come exclusively from P2 — Recovery may redo the effect successfully but still fail to convergeIn A following inspection may already prove that the target matches the expected after-state, but the current path can still rethrow instead of finalizing and synthesizing the outcome immediately. Once durable inspection proves the expected after-state, recovery should converge to completed regardless of whether the worker response survived. P2 — Conversation copying does not rewrite recovery evidence referencesConversation copying rewrites outer event IDs but shallow-copies recovery fact payloads. Embedded The copied conversation can therefore deterministically fail evidence validation or appear corrupted even though its outer events were copied successfully. Copying needs an old-to-new event-ID map and typed rewriting of recovery facts. If recovery-bearing conversations are intentionally non-copyable, that restriction should instead be enforced explicitly. Lower-priority follow-upsI would treat the following as P3 or slice-local hardening rather than merge-level P1/P2 findings:
ValidationI built the affected core, storage, and runtime packages and ran both targeted and full runtime tests. The targeted recovery set produced 115 passes and 1 failure. The full runtime suite produced 2,558 passes, 2 failures, and 7 skips. The failures exposed:
I also reproduced the concurrent-overwrite race, loss of macOS extended attributes, and ConclusionThe underlying architecture contains several good ideas, but I do not think the current combined diff is yet the simplest reviewable or safely reversible unit. Converting #1346 to Draft and replacing it with invariant-focused PRs would make approval more meaningful, testing more targeted, and rollback much safer. It would also let us remove prototype and speculative surfaces instead of stabilizing APIs before they have a production consumer. 中文对照感谢为这个 PR 投入的大量工作。整体方向是对的:恢复判断应该建立在持久化的因果证据上;Runtime Event 应该是事实的权威来源;SQLite 中的投影应当可以从事件重建;同一恢复过程中的判断与写入也应该串行完成。 不过,我最主要的建议是:把 #1346 转回 Draft,不要继续在这个大 PR 上叠加修复,而是将生产改动重新整理成一组边界清楚、可以独立审查和回滚的 PR。 原因并不只是改动行数太多。当前 PR 同时要求审查者证明多套彼此独立的正确性:
这些部分的事实权威、失败方式和回滚边界并不相同。把它们放在一次审批里,意味着审查者必须同时证明所有交互都是安全的。以后只要其中一个部分出现回归,也可能被迫回滚其他原本正确的能力。 我建议保留当前 PR 作为总设计和讨论记录,将它转成 Draft,然后从最新 1. Recovery 持久化的唯一权威第一批只引入最小的 recovery fact 模型、唯一的原子 bundle writer、严格的执行身份校验,以及可重建的 SQLite 投影。
2. Continuation 正确性不可变的 Runtime Event 应该是 continuation cursor 和 replay admission 的唯一来源。仍在变化的 partial snapshot 只能用于界面展示和诊断,不能充当账本位置。 如果复制 conversation,还需要同步重写 recovery fact 内部的事件引用和执行身份。 3. 本地文件的预备写入与恢复前两项稳定以后,再接入 Write/Edit 的 checkpoint 和恢复能力。 这一批 PR 需要完整证明下面这条不变量:
它还必须保持现有文件工具的返回结果、路径、权限和元数据语义。 4. CLI owner 和资源生命周期除非文件恢复严格依赖这部分,否则 CLI interactive owner 的调整应当单独提交。资源所有权、初始化失败和退出清理本身就是另一套需要独立证明的不变量。 在真正出现生产消费者以前,我不建议把 Phase 4 Git carrier、restricted verifier 或预设的 retry/reattach 模式带入前面这些 PR。 拆分边界应该依据“能否独立表达和验证一个完整不变量”,而不是机械地按文件或现有 commit 切割。默认从最新 FindingsP1 — 最后的漂移检查与文件替换不是一个原子操作在 另一个进程可以在最后一次检查结束后、 我通过在最后一次检查和 这是我认为唯一应该保留为 P1 的问题,因为它可能不可逆地丢失其他进程刚刚写入的数据。即使在 提交过程需要真正具有 compare-and-swap 性质的保证,例如使用交换操作或检查被替换的 inode,并且在发现竞争写入时保留或恢复外部版本。如果目标平台无法提供这种保证,就应该安全停止自动 redo,而不是冒险覆盖目标文件。 P2 — 没有对应 outcome,也可能写出 completed decision
resolver 随后会把格式合法的 目前还没有证据表明正常的原子生产路径一定会生成这种状态,因此我会把它定为 P2,而不是 P1。但这仍然是一个真实的契约缺口:崩溃、导入器、迁移程序或使用公开 writer 的外部调用者,都可能生成“已经完成,但 provider outcome 不存在”的记录。 保留的 recovery fact 应当只有一个写入权威。Storage、投影重建和 resolver 都应该拒绝执行身份不一致的证据,并强制要求以下因果顺序:
P2 — 准备阶段的普通业务错误绕过了标准工具结算预备 Write/Edit 会在 runtime 进入正常工具错误处理以前执行一部分校验。 因此,Edit 目标不存在、 这些错误表达的是“工具明确没有被执行”,并不属于“副作用是否发生已经无法确定”的恢复故障。它们应该走正常的 provider-facing tool error 结算流程。 只有持久化提交失败,或者副作用状态确实无法确定时,才应该中断 execution 并进入 recovery。 P2 — macOS 路径别名可能静默绕过 checkpointcheckpoint carrier 对词法路径和 canonical path 的比较,会把
这破坏了最核心的不变量:T1 已经持久化并要求 reconcile,但真正恢复时却没有可用的 checkpoint。 另外,预备操作会返回 canonicalized path,这也改变了调用者传入路径别名时的原有返回值契约。 路径身份应该由一个统一的权威解析。如果目标不能使用预备写入,就必须在持久化 T1 之前失败;不能先选择 reconcile,再静默退回没有 checkpoint 的旧路径。 P2 — 原子替换改变了原有文件语义,并丢失元数据当前实现会创建临时文件、设置 mode,然后通过 我在 macOS 上验证过:经过 prepared Write 后,原文件的 extended attributes 会消失。ACL、owner、group、Finder 或 quarantine 元数据,以及其他平台特有的附加数据,也可能被临时文件的属性替代。 只读目标文件、可写父目录等权限组合的行为,也可能与原来的 这个问题是确定发生的,但只影响带有相关元数据或特殊权限配置的文件,因此我倾向于定为 P2。它仍然需要在 prepared-file PR 合并前解决:要么保留必要的元数据和权限语义,要么在持久化 preparation 以前明确拒绝不适合原子替换的目标。 P2 — Continuation cursor 混入了仍会变化的 partial snapshot
正在流式生成的 partial snapshot 属于展示状态,不是不可变账本中的位置。如果把它计入持久历史,后续 continuation 或 replay 就可能跳过事件、重复处理事件,或者错误判断事件所属的 continuation。 Continuation identity 应该只来自 P2 — Redo 已经成功,但响应丢失时不能立即收敛在 后续检查此时可能已经能够证明目标文件等于预期的 after-state,但当前逻辑仍可能继续抛出异常,而不是立即完成 recovery 并合成对应 outcome。 一旦持久化检查已经证明 after-state 成立,恢复过程就应该收敛到 completed,不应再依赖 worker response 是否成功返回。 P2 — 复制 conversation 时没有重写 recovery evidence 引用复制 conversation 时,外层 Runtime Event ID 会被重写,但 recovery fact payload 只是浅复制。内部的 结果是:外层事件复制成功,但复制后的 conversation 会稳定地出现 evidence validation 失败,或者被判断为 recovery 数据损坏。 复制过程需要先建立旧事件 ID 到新事件 ID 的映射,然后按照 recovery fact 的类型重写内部引用。如果产品并不打算支持复制包含 recovery 状态的 conversation,也应该明确拒绝,而不是生成表面成功、内部引用失效的副本。 较低优先级的后续问题下面这些问题更适合标为 P3,或者放到对应 slice 内处理,不需要定成当前 PR 的 P1/P2:
验证结果我构建了受影响的 core、storage 和 runtime package,并分别运行了 recovery 相关的定向测试和完整 runtime 测试。 定向测试结果是 115 个通过、1 个失败。完整 runtime 测试结果是 2,558 个通过、2 个失败、7 个跳过。 失败用例暴露了两个问题:
除此之外,我还独立复现了:
结论这套实现包含多项正确的架构思路,但当前合并后的大 diff 还不是最容易证明正确、最容易独立审批、也最容易安全回滚的交付单元。 将 #1346 转回 Draft,并用一组围绕明确不变量组织的 PR 逐步替代,会让每次审批更有意义、测试范围更清楚、出现回归时也更容易回滚。 这样还可以先删除没有生产消费者的原型和预设抽象,避免在真实需求出现以前,就把不必要的复杂度固化成公共契约。 |
Thanks for the design work and the documentation here — the thinking on file-transaction recovery still has value in the issue discussion. I am closing this because it can no longer be reconnected to current main. It is 1487 commits behind, and 34 of the 76 files it touches no longer exist on main: the whole tool-recovery module ( If you want to continue in this direction, opening a fresh branch against current main — ideally in smaller pieces — would be the way. If the documents under 简体中文感谢你在这条上做的设计和文档整理,file-transaction recovery 这块的思路留在 issue 讨论里仍然有价值。 这条我准备关掉,原因是它已经没法接回现在的 main 了。它比当前 main 落后 1487 个提交,改动涉及的 76 个文件里有 34 个在 main 上已经不存在,其中包括整套 tool-recovery 模块、 如果这个方向还要推进,建议基于当前 main 重开一条,规模也可以拆小一点。文档部分如果还成立,单独提一条 docs PR 会更容易推进。 |
Summary
This PR completes the Phase 3A operation plane for controlled recovery and now includes the file-transaction evidence required to make production
WriteandEditrecovery causal rather than heuristic.WriteandEdit, independent of GitReview blockers resolved
1. Production
Write/Editdid not declarerecoveryMode: 'reconcile'The production-shaped path now marks the canonical builtin
WriteandEditdefinitions asreconcilewhenever a file checkpoint carrier is installed.ToolRuntimetherefore persists the same recovery mode that the registered contracts expect instead of falling back tonever_auto_retry.This is covered by a real builtin-definition →
ToolRuntime→ SQLite dispatch test for both tools. The test reads the durable T1 facts back from SQLite, so it cannot pass by constructing reconcile mode manually.2.
countOccurrencescould not prove that this particular Edit caused the current file stateOccurrence-based applied/not-applied inference has been replaced by a fine-grained file transaction protocol:
missingor content hash), expected-after hash, and transform version.Write/Edittransform used by normal execution; recovery does not carry a second simplified Edit implementation.This protocol does not require Git. Git-backed workspace snapshots remain a separate Phase 3B/4A concern.
Crash and restart coverage
The file transaction suite covers interruption around checkpoint durability, temp write, temp fsync, before/after replace, parent fsync, external drift, oversized files, temp cleanup, and T2 failure.
A production-shaped restart test executes a real builtin
Write, persists checkpoint + T1, atomically replaces the file, injects failure before T2, closes and reopens SQLite, invokes the real SessionManager recovery path, commits the atomic recovery bundle, synthesizes the missing tool response, and completes a continuation throughFakeBackend.Additional recovery fixes
provider_resume_boundary_unsupported. Only the provider replay view rolls back to the latest durable user/tool boundary; the ledger and high-water remain unchanged.recoveredOperationssummaries without requiring hosts to reconstruct them from diagnostics.Safety boundary
Validation
Latest local validation on the split Phase 3A branch:
npm run typecheck— all workspaces passedGitHub CI on the latest upstream-synchronized head passes
typecheck,test, ande2e.