refactor(runtime): persist durable Tool Result projections - #4287
refactor(runtime): persist durable Tool Result projections#4287Astro-Han wants to merge 29 commits into
Conversation
9d41dd3 to
4d7007a
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
484f4e9 to
b2b2ae8
Compare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed the latest head b2b2ae86d. No P0 or P1 — approving. I found nothing worth filing at P2 or P3, so there are no inline comments. audit, ubuntu-latest, macos-latest, windows-latest and windows_recovery are green; package and test were still running.
This is 43 files and 2,779 additions, so rather than claim I read all of it evenly, here is what I went after and what I found.
The allocation bound is done properly. 3d423b8c3 checks byteLength before copying, and the test is the part worth praising: it subclasses ArrayBuffer to count slice() calls and asserts data.copies === 0, plus passes a planning callback that throws if reached. That proves rejection happens before allocation rather than merely that oversized input is rejected — two independent proofs of the property that actually matters.
The same discipline holds across the module. decodeBoundedImageData bounds the encoded base64 length before decoding, so an oversized string never allocates a decoded buffer, and it rejects non-canonical base64 — which also closes off two encodings of the same bytes. Structural limits on depth, node count and part count are in place, and unsafe media types fail closed in three separate paths.
The artifact protection is the right shape. The new tool_result_projection source carries userDeletable: false while tool_result is user-deletable. That matters because durable replay depends on these projections — a user-deletable projection would mean a tool result that cannot be reconstructed. sharedReadable: true keeps shared-session reads working. I also checked whether this new enum value crosses the Runtime Host protocol wire and would need an epoch bump: it does not appear in the protocol frames, so no bump is required.
The session-manager.ts removal is safe, and I checked the call chain rather than the symbol. Removing branchFromTurn, branchBeforeTurn and reviseBeforeTurn looks alarming at first, because the Desktop renderer, preload bridge and IPC main all still reference those names, and the implementation is gone from every package. It is fine: the live path is sessions:branchFromTurn → deps.client.copySession("branch", ...) → Host → conversation-copy.ts, which never went through those SessionManager methods. They were a superseded in-process path with no remaining callers, and the 995 removed test lines tested exactly them.
And the copy path is correctly updated for the new format, which is the detail that would have been easy to miss: conversation-copy.ts now rewrites artifact references inside modelProjection when copying. Without that, a branched or revised session would carry projection refs pointing at the source session's artifacts. It has 22 tests.
Scope of this review. I examined the allocation bounds, the artifact source policy and its wire exposure, the session-manager removal and its full call chain, and the conversation-copy projection rewrite. I did not line-by-line review tool-runtime.ts (+266), ai-sdk-backend.ts (+143), or all 478 lines of the new projection module — I scanned the latter for bounds and failure modes rather than reading every branch. Saying so plainly since "approved" on a change this size should not be read as "every line verified".
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head and found one P1 blocker in the durable tool-result settlement path. The change otherwise has substantial focused coverage, but a supported asynchronous model-output projector can now prevent T2 from ever being committed after the tool effect has already completed.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| ? Object.freeze(coerceResultContent(result)) | ||
| : coerceResultContent(result); | ||
| const projected = this.projectToolResult(tool, turnId, toolUseId, executionArgs, result); | ||
| const modelProjection = isPromiseLike(projected) ? await projected : projected; |
There was a problem hiding this comment.
P1 — Bound or abort asynchronous projection before T2.
MakaTool.toModelOutput is explicitly allowed to return a PromiseLike, but this await has no timeout or abort race and now occurs before commitOutcome. A projector that never settles leaves an already-completed tool effect with only T1 durable: in an exact-head production-path probe, the implementation returned, commitToolPrepared ran once, commitToolOutcome stayed at zero, and both settleToolCall and endTurn("aborted") remained pending. Before this change, the raw settlement path committed T2 before model-output materialization.
Please make projection total at this boundary (for example, race async projection against turn abort or a bounded timeout and commit the deterministic failure sentinel), or narrow and enforce the contract to synchronous projectors, so a projector cannot prevent a completed outcome from reaching T2.
There was a problem hiding this comment.
I independently reproduced this on exact head b2b2ae86dd2a00312e73dd1fb3090ff07365a4cc. A toModelOutput PromiseLike that never settles allowed the implementation to finish after one T1 commit, but T2 remained at zero; aborting the turn still left settleToolCall pending. This confirms the completed tool effect can be stranded indefinitely behind the projector. The projector therefore needs a bounded/abort-aware fallback to the deterministic failure projection, or the contract must be narrowed and enforced as synchronous.
Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
M4n5ter
left a comment
There was a problem hiding this comment.
Review of exact head b2b2ae86dd2a00312e73dd1fb3090ff07365a4cc: changes are required. I independently reproduced the unresolved P1 on tool-runtime.ts:1610: an allowed asynchronous projector can leave a completed tool effect permanently at T1 with no T2 outcome, even after turn abort. Because that P1 blocks approval, I am not approving this head. I left the separate artifact-lifecycle P2 inline below. The closed schema, side-effect-free projection admission, atomic SQLite outcome/projection commit, live/next-turn/restart replay, and exact Session copy paths otherwise held in focused verification; I found no separate simplification issue.
Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
| const persisted = new Set<string>(); | ||
| for (const artifact of prepared.artifacts) { | ||
| if (persisted.has(artifact.ref.relativePath)) continue; | ||
| await artifact.persist(); |
There was a problem hiding this comment.
P2 — Reclaim partially published projection artifacts. This loop publishes each protected artifact separately. If a later persist() rejects, the function returns the failure sentinel while earlier writes remain live, hidden, and non-user-deletable; the same orphan also remains if the later RuntimeStore T2 transaction fails. A two-artifact probe where the second write rejected produced persisted: [1] plus the failure projection, with no durable projection referencing artifact 1. Independent failing operations can therefore accumulate inaccessible disk usage until the whole Session is purged. Please add batch/staged publication, finalize ownership with T2, or persist enough ownership for deterministic garbage collection. This is non-blocking under the current P0/P1 approval gate, but it is a real lifecycle leak.
Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head b2b2ae86 (verified unchanged at review time; all checks completed/success on this head, including test 25m50s and package 23m41s).
Durable Tool Result projection foundation, verified rather than assumed:
- The schema is closed, versioned, and bounded at the Core layer: 256 KiB / 64 parts / depth 32 / 20k nodes, exact-shape validation per kind, one frozen failure sentinel.
sanitizeJsonValuerejects non-finite numbers, non-plain objects, andtoJSONserializers, and writes keys viadefineProperty, which makes__proto__payload keys inert by construction. Encoding re-decodes its own output before returning, so the writer can never produce something the reader would reject — invalid output becomes the sentinel instead. - Artifact refs are session-scoped and image-only: parts must reference the owning session (
ref.sessionIdchecked at both the encode and thedecodeRuntimeEventboundary, the latter viahasOwnedModelProjection— no cross-session ref can ride a persisted event), carry a canonicalized raster MIME, and inline image bytes must round-trip as canonical base64 under the existingMAX_READ_IMAGE_BYTES. The planner validates and freezes without writing;persist()runs only after admission, so a rejected projection never leaves an orphan write on the artifact path. - Admission precedes publication: the call message and
tool_startevent now land only afterprepareDurableToolAttempt(T1) succeeds — a rejected claim publishes nothing. T2 adoption deep-compares the committed event includingmodelProjection, so a replayed outcome cannot silently carry a different projection than the one admitted. - Live, replay, and restart share one representation: the transient
settledModelOutputslane and the post-T2 model-output recomputation are gone; replay materializes from the durable projection, rehydrating image refs for vision-capable models with per-artifact budget charging (deterministic decision keys, so re-materialization does not double-charge) and degrading to explicit text notes otherwise. Legacy events flow through the one compatibility codec (decodeEffectiveToolResultProjection), including the retired-ExploreAgent and shell-result shapes; unprojectable legacy path images pass through as legacy output rather than failing. - Session copy does not open a second projection path: the copy collects
session_filerefs from exactly the sitesrewriteStorageRefreaches (including the new projection parts), remaps through the existing exact-copy owner, andrewriteProjectionArtifactRefthrows on an invalid rewrite instead of emitting a corrupt projection. - Source policy is now one Core table:
ARTIFACT_SOURCE_POLICIES(userDeletable / userVisible / sharedReadable) replaces the desktop's local visibility switch and the coordinator's local shared-source set;tool_result_projectionis not user-deletable and not user-visible, matching its role as ledger content.
Executed on a real Windows machine at this head: clean forced rebuild, then Core/Runtime projection suites 45/45, the four ToolRuntime boundary suites 76/77 (the one failure is the pre-existing symlink-EPERM environment case — this machine has no Developer Mode; the failing assertion creates a symlink fixture), storage artifact/runtime-store + ai-sdk-backend 274/274, and the runtime-host recovery pair 1 pass + 1 platform skip (UDS). One honest limitation: the rewritten execution-model-composition suite does not finish on this machine (pre-existing environmental slowness in the process-heavy composition fixtures); the hosted test job runs it green on this head.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
durable Tool Result 投影地基,全部核实而非假设:Core 层 schema 封闭有界(256KiB/64 段/深度 32/2 万节点),sanitizeJson 拒绝非有限数/非纯对象/toJSON,defineProperty 逐键写使 proto 键天然失效;编码后自解码自检,非法输出收编到唯一 sentinel。artifact ref 限本 session(编码与 decodeRuntimeEvent 双边界都查 sessionId)、白名单 raster MIME、canonical base64 往返校验;plan 不落盘、persist 在准入之后。T1 先准入后发布(call 消息/事件只在 claim 成功后落地);T2 adopt 深比对含 modelProjection。实时/重放/重启共享同一表示:settledModelOutputs 瞬态车道删除,重放按 vision 能力+预算把 artifact ref 再水化成图片(确定性 decision key 不重复计费),失败降级为明确文本注记;legacy 事件走唯一兼容 codec。session copy 只 remap ref 不另开投影路径,非法 rewrite 直接抛。artifact 来源策略收成 Core 一张三轴表。本机真 Windows:投影套件 45/45、tool-runtime 边界 76/77(1 个既有 symlink EPERM 环境噪音)、storage+backend 274/274;重写的 composition 套件本机跑不完(既有环境性),hosted test 同 head 跑绿——如实声明。
Summary
Tool Results previously had no single durable, provider-neutral model-visible representation. Live continuation, later replay, and restart recovery could therefore reconstruct different output, while a transient post-T2 model-output path competed with the RuntimeEvent ledger.
This PR adds the PR 1 foundation from #4283:
The final diff is 43 files (
+2779/-1578): production and other source is+1225/-463, while tests are+1554/-1115. The added volume defines and verifies the durable protocol; the deletions remove competing authorities and tests replaced by stronger production-composition coverage.Refs #4283
Lifecycle
flowchart TD A[Provider tool call] --> B{RuntimeStore T1 claim} B -- rejected --> R[Stop before call publication or execution] B -- accepted --> C[Runtime publishes the admitted call and executes once] C --> D{Storage plans exact artifact refs without writes} D -- planning fails --> F[Core-defined deterministic failure projection] D -- planned --> E{Core admits one closed and bounded projection} E -- rejected --> F E -- admitted --> G{Storage persists protected projection artifacts} G -- persistence fails --> F G -- persisted --> H[Canonical durable projection] F --> I{RuntimeStore T2 atomic commit} H --> I I -- fails --> J[Fail-stop; T1 prevents repeated effects] I -- committed --> K[Effective-history reducer] K --> L[Live continuation] K --> M[Next-Turn replay] K --> N[Cold restart]Core is the only compatibility/schema authority; Runtime does not recalculate projection semantics during replay. Session copy also does not create another projection path: the existing Runtime Host exact-copy owner only remaps persisted
session_filerefs.Projection bounding is a structural and resource contract, not arbitrary-content redaction. Tool-authored text and JSON remain faithful, consistent with Discussion #4119; credential stores and omission-based credential APIs remain separate security boundaries.
Verification
npm --workspace @maka/core run test:dist— 739 passednpm --workspace @maka/storage run test:dist— 1015 passed, 7 skippednpm --workspace @maka/runtime run test:dist— 3093 passed, 13 skippednpm --workspace @maka/runtime-host run test:dist— 1444 passed, 12 skippedgit diff --checkpassedReview focus
This remains the durable projection foundation. Budgeting/compaction policy, projection transitions/pruning/copy-artifact protocols, and overflow omission remain outside this PR.
Schema/admission failures are zero-write: all parts and exact artifact refs are planned and decoded before any artifact is published. Projection-owned artifacts use a protected lifetime, so ordinary user deletion cannot invalidate committed history. Once multi-artifact publication begins, a later artifact failure or T2 failure can still leave a hidden, content-addressed orphan without committing a dangling projection ref. Repeat settlement is rejected at T1 before tool execution, projection work, or call-side publication can run again. Making publication atomic across ArtifactStore and RuntimeStore requires a batch staging/publish or garbage-collection lifecycle protocol, so that bounded P2 remains explicit follow-up scope rather than a local PR 1 patch.
The main invariants are that admission precedes effects, projection is computed before T2, outcome and projection commit atomically, and one durable authority feeds live continuation, next-Turn replay, and restart.
AI use
Select exactly one:
Tool(s) and scope: Codex implemented the schema, codec, Runtime and Host integration, tests, validation, simplification, and review-driven corrections.
Checklist
Does this PR entail a change in behavior?