fix(runtime): preserve sandbox negotiation across continuations - #4308
fix(runtime): preserve sandbox negotiation across continuations#4308testikun wants to merge 12 commits into
Conversation
272523c to
73e2cef
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Blocking compatibility issue: this PR declares epoch 84 for the sandbox-continuation wire contract, while PR #4321 independently declares the same global epoch 84 for the ScheduledTask Connection-identity wire contract. RUNTIME_HOST_COMPATIBILITY_EPOCH is a single Host/Client interoperability boundary, not a per-feature version. Both branches are based on the old 9249bf3 base and are currently conflicting with main (which is at epoch 83). Please rebase and either compose both closed-shape changes under one epoch-84 ledger entry if they are intended to ship together, or land one at 84 and bump the other to 85 after the first. The stale 78→79 explanation should be updated as part of the same repair. Until this is resolved, the meaning of epoch 84 depends on merge order and clients cannot be given a deterministic compatibility contract.
0dced7d to
14b4b56
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — reviewed 1b986025 for substance. @me2seeks holds the open block on the epoch, so I've stayed off it apart from one factual note at the end.
The problem is real and well-stated: a safe continuation builds a new ToolRuntime, so a Turn that was already denied or had spent its correction budget could start the negotiation over. Rebuilding from digest-validated event lineage plus the durable request log, and keeping ExecutionBoundary as the only thing that grants capability, is the right shape.
P2 — the carried sandboxBoundaryNegotiationState never becomes authority, so it costs more than it earns.
In revalidateContinuationBoundary, the state is re-derived from the lineage and the durable rows, compared against continuation.sandboxBoundaryNegotiationState with isDeepStrictEqual, and on mismatch throws source_replay_changed — then the re-derived value is what's returned and used (runtime-kernel.ts:2871). A second equality check on the same pair sits at :3056.
Since the consumer has to derive it anyway to be safe, the carried copy is a second representation of a fact the consumer already owns. What it adds is a field on RuntimeContinuation, two deep comparisons, and a failure mode — and that failure mode fires precisely in the window this PR documents elsewhere: the request row commits before its RuntimeEvent is appended. A continuation planned before that event lands and revalidated after it lands derives two different states and throws, turning a recoverable timing skew into a hard failure of the Turn. I have not built that race, so treat the reachability as argued rather than demonstrated — but the two derivations are separated in time over an append-only log with a documented commit gap, which is enough to want the check gone rather than tuned.
Dropping the field takes both comparisons and source_replay_changed with it, and RuntimeContinuation stops growing.
If the intent is to catch a planner bug rather than a hostile caller, that is a reasonable thing to want — but then it belongs as an internal invariant assertion at the point of derivation, not as a field the caller supplies. As written the producer of the value and the party it is checked against are the same untrusted input.
Nothing else stood out. projectSandboxBoundaryNegotiation rejecting malformed, legacy, duplicate, and identity-mismatched facts fail-closed reads correctly, and the refusal to infer a correction count from older ledgers without the structured marker is the right call — inferring there would have been the easy mistake.
Evidence boundary: I read the projection, the kernel's revalidation path, and the continuity contract; I did not run the suites and did not review the 534 lines of new tests in detail.
Factual note, not a verdict — that stays with @me2seeks: main is at 87 and this branch is at 88, so the "86 → 87" wording in the description has been overtaken again. Worth refreshing the body whenever you next rebase.
AI-assisted review: drafted with Maka; I verified the re-derivation ordering, both equality checks, and the field's provenance against the branch source myself.
f1099d3 to
58f25c0
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
PR #4308 — 620903d — follow-up review
Summary: Sandbox boundary durable settlement. Exact head 620903de6d6fbe441aeccedfe75931d958882e3b frozen, windows_recovery green, test/package pending, MERGEABLE/BLOCKED. This follows prior 73e2cef 1×P2 NO-GO; current head still exhibits same ordering gap (only typeof decision.revision gate added).
Finding (reproducible, decision-changing):
- P2 — durable settlement applied without authoritative ordering —
packages/core/src/sandbox-boundary.ts:241-405tallies descendant failures byRuntimeEventorder, then415-470appliessqlite-session-metadata-storedurableapproved/deniedsettlement without a comparable sequence number. If Host persisted settlement (session-metadata-store.ts:720-815) beforetool-runtime.ts:2863-2880decision ack is lost, continuation replaysapprovedthen later descendantinvalid/unresolvedfailure is reset at452-466, clearingcorrection budget/finalizationRequested. Existing test717-745covers isolated denial only. Fix: unify on authoritative order or fail-closed when ordering unavailable; add interleaved approved→failure and denied→approval regression.
Gating: hosted windows_recovery SUCCESS, test QUEUED. No current-head formal review before this comment.
Automated review notice: This comment was posted by an automated review agent operated by AstroHan. It is not an independent human review and does not replace one.
简体中文
本条结论来自 @Luna-Deep-Qronos 在 exact head 620903d 的独立复核,已核对 head 未漂移。编排仅同步发布,内容以技术线为准。
6166099 to
6fee964
Compare
6fee964 to
abcfda7
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Reviewed the boundary surface of this change (head abcfda7) with a simplification lens: where does negotiation state get authority, and can any path silently weaken it. The core shape is sound — fail-closed decoding of malformed/legacy/duplicate facts, refusing to infer correction counts from unstructured legacy failures, the durable-settlement ordering guard, and lifting SANDBOX_BOUNDARY_FAILURE_ROUND_LIMIT to core as the single round-limit authority are all the right calls. The earlier carried-state concern is also fully resolved in this revision: the planner no longer carries the projection, and a test pins that.
Two findings remain, both about a second/weaker authority for the same fact rather than about the projection itself — inline:
- P1 on
ai-sdk-backend.ts: the continuation fallback projection is unreachable in production, and fail-open if it ever is reached. - P2 on
runtime-kernel.ts: the durable boundary-log reader silently degrades to an empty log when absent.
Evidence basis: traced the sole production constructor of RuntimeContinuationMetadata, every in-tree SessionStore implementation, and all five invalid-round recording sites. I did not re-run the suites.
Astro-Han
left a comment
There was a problem hiding this comment.
Third pass, on abcfda71. First the good news: the ordering gap from the last two rounds is closed. The guard at sandbox-boundary.ts:445 really does refuse when a durable settlement has no decision ack and any stateful event exists, both callers collapse that to finalization, and there is no path left where a durable row is applied on top of later events. The projection also cannot widen authority: denied and finalizationRequested only short-circuit harder, the two counters only climb, and nothing touches ExecutionBoundary. Carrying the projection through the planner is gone too, with a test pinning it. Merge-tree against main is clean and the epoch guard passes on the merge result.
What I found this time is the other direction: the projection refuses ledger shapes the product itself writes, and every refusal becomes a Turn with zero tools. Three cases, all reproduced by calling projectSandboxBoundaryNegotiation on the built dist:
- Crash during a boundary call. The
function_callis persisted, the response is not.continuation-replay.tsalready handles this shape (unmatched_tool_call, trimmed as the interrupted suffix), butrevalidateContinuationBoundaryfeeds the untrimmed prefix events to the projection, which returnsinvalidatsandbox-boundary.ts:416("has no durable response"). This is the exact path the PR exists for, and it now ends in a text-only Turn.trimmedSuffixEventIdsis already in hand; drop those before projecting, or count a dangling call as one unresolved round. - Denied, then the model asks again. The backend routes the retry to the
invalidrepair tool withsandboxBoundaryAttempt: true, which throwsinvalid_boundary_declaration. The ledger then holds a failure on a call namedinvalid, andisBoundaryAuthorityCallonly knowsrequest_sandbox_boundaryandBash, so the projection hits the "failure has no canonical call" branch that the tests describe as anti-forgery. This is the PR's headline scenario. Let the predicate recogniseINVALID_TOOL_NAMEwithsandboxBoundaryAttempt === true. - Any error on a boundary call without a structured marker.
sandbox-boundary.ts:401treatsisErrorwithoutsandboxFailureas "legacy ledger, reject". That catches every session recorded before this PR, plus sevenrefuseBeforeDispatchexits and the generic catch intool-runtime.tsthat carry no marker today. A user who stops a boundary call and continues lands here. Only a malformedsandboxFailureshould beinvalid; a plain error is a plain error.
The common amplifier is that invalid maps to createSandboxBoundaryFinalizationState(), and the backend then sends an empty tool list from step zero. Failing closed on the negotiation (do not restore budget, withdraw the boundary tools) is right. Failing closed on every tool in the Turn is a regression from the pre-PR behaviour, where the new ToolRuntime simply started clean. I would decouple those two before anything else; it also decides how serious the three cases above are.
Two smaller ones on the durable leg:
- A host restart closes pending requests as
deniedwithoutcomeReason: host_restarted, and the projection reads onlystatus. Nobody denied anything, yet the recovered Turn is permanently denied, and if an approval preceded the restart the ordering guard fires and the Turn has no tools. Either readoutcomeReason, or tell me that a restart-closed Turn is never continued (the recovery pass marks the run failed). If it is never continued, the durable read, the attribution and ordering guards and the three-layerlistSandboxBoundaryRequestsplumbing have no reachable producer, and the PR shrinks to the lineage projection alone. That is the biggest simplification available here, and it hangs on that one fact. - Denied then another failure: live
ToolRuntimefinalizes immediately and stops counting; the projection keeps counting and only finalizes at three. The test atsandbox-boundary.test.ts:505pins the divergence. The recovered Turn ends up looser than the live one it is meant to reproduce.
On me2seeks' two points I agree, and can add: the backend fallback at ai-sdk-backend.ts:1421 projects without durable rows, so it is a second, weaker authority for the same fact; the planner's read at runtime-resume.ts:449 discards the result and exists only to see whether the store throws, while the kernel's read a few seconds later has no catch at all. Make the metadata field required, delete the fallback and the probe, and the ?? [] chain goes with them. Also: reason on the projection result has no reader, and the crash test's new cases are two SessionManagers in one process with hand-written events, so nothing in the suite projects a ledger that a real ToolRuntime wrote. The three cases above all live in that gap.
Epoch: 94 is right and the guard passes, but #4386 also claims 94 alongside #4321; whichever lands first forces the others to renumber, so the body should list both.
Evidence boundary: static read of abcfda71 against main cdb29399; @maka/core built and its sandbox-boundary suite green (30/30); the three refusals and the restart case reproduced against the built projection; the final hop to an empty tool list read from ai-sdk-backend.ts:2084, not observed end to end. No process-level crash run.
AI-assisted review: drafted with Maka; I verified the ordering guard, the three refusal paths, the kernel's untrimmed input and the epoch result myself.
简体中文
前两轮的 ordering 缺口已经关上,投影也不可能放宽权限,这两点可以了结。这轮的问题在反方向:投影拒绝了产品自己会写出的三种 ledger 形状(boundary 调用中途崩溃留下悬空 call;被拒后重试走 invalid 修复工具;boundary 调用报错但没有结构化标记,包括所有本 PR 之前的 session),每次拒绝都变成整个 Turn 零工具。放大器是 invalid 直接映射到 finalization。建议先把「协商 fail-closed」和「全部工具 fail-closed」解耦。另外 host 重启关闭被当成用户拒绝;如果重启关闭的 Turn 根本不会被续跑,整条 durable 读取腿都没有可达生产者,PR 能大幅缩小。me2seeks 的两条同意,backend fallback 和 planner 探针建议删掉。epoch 94 与 #4386、#4321 三方争用,正文要写全。
…he#3731) Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
abcfda7 to
181087c
Compare
Generated-by: OpenAI Codex
|
@me2seeks @Astro-Han Thanks for the detailed review. I addressed the points on the current head
Validation: affected core/storage/runtime/runtime-host builds and typechecks, sandbox-boundary and continuation/resume/session-manager tests, lint, format, and I removed the earlier progress comments so this is the single current status update. Please re-review. |
Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
@testikun @yihanzhu I want to call the direction on this one rather than run a fifth round of line comments.
First, credit where due: ten of the eleven points from last round are closed on 9b3c8104, most exactly as suggested, and core, runtime, lint and format are green locally. The one still open is the host-restart closure (rows settled as deny with outcomeReason: host_restarted are read as user denials; isSandboxBoundaryRestartClosure() exists and is unused). But I no longer think that is the point.
After four rounds, every finding has come from the same place: the PR rebuilds the negotiation from the RuntimeEvent lineage, reads the SQLite boundary log as a second source, reconciles the two, and fails closed on disagreement. Each round found another ledger shape the product itself writes that the reconciliation rejects. That is about 570 production lines, 1,100 test lines and a compatibility-epoch bump, to restore three numbers for a Turn. I think the approach is wrong, and I think the acceptance criteria in #3731 that led here are wrong too, so I am saying this on both.
The fact that matters already has one authority. The Host owns the sandbox-boundary request log: each row carries the Turn, the status and the closure reason, and the Host writes it, not the model. "A denied request cannot be reopened" is one read of that table when a continuation builds its ToolRuntime: a real denial for this Turn means start denied. No lineage projection, no second source, no protocol change, and forgery is not a question because nothing model-generated is read.
Everything else in the PR exists to restore the correction budgets (invalid and unresolved rounds). Their job is to cap a model looping on malformed declarations at three. If a continuation restarts them at zero, the worst case is three more attempts before the same cap; a model cannot cause a continuation on purpose, so "splitting work across segments to reset the budget" is not a path anyone can take. Three attempts are not worth the projection, the reconciliation and an epoch every client has to move past.
So my ask: start over from the boundary log. Read it for the Turn on continuation and restart recovery, treat a restart closure as not a decision, start the new ToolRuntime denied when there is a real denial, and let the budgets begin at zero. Items 3, 4 and 5 of #3731 hold by construction: the live ExecutionBoundary is untouched, the source is Host-written, and the log is already Turn-scoped. I would expect that to be a few dozen lines and one or two tests against a real ToolRuntime. @yihanzhu, that means dropping acceptance item 2 and the "derive from digest-validated lineage" wording from the issue; if there is a reason the budgets must survive a continuation that I am missing, this is the place to say it.
I know this is a hard thing to hear after four rounds of careful fixes, and the work on the ordering guard and the crash harness was genuinely good. It is the shape I am asking to change, not the care.
Evidence boundary: static read of 9b3c8104 against main 92fa5281; @maka/core and @maka/runtime built and their test:dist run; restart shapes reproduced on the built projection.
AI-assisted review: drafted with Maka; I verified the restart paths, the boundary-log ownership and the size split myself.
简体中文
@testikun @yihanzhu 这轮不再逐行提意见,想把方向定下来。十一条关了十条,剩重启关闭那条,但我认为问题不在细节。四轮发现全部来自同一处:从事件流重建协商状态,再和 SQLite 日志对账,对不上就拒绝,每轮都撞上一种产品自己会写出的形状。570 行生产、1100 行测试、一次 epoch,只为恢复三个数。我认为这个解法不对,#3731 里导向它的验收条款也不对。Host 自己写的边界请求日志已经是唯一权威,有 Turn、状态、关闭原因,「拒绝过不能再问」读这张表就够,不改协议、不存在伪造。轮次预算续跑后从零数,最坏多三次尝试,模型无法主动触发 continuation,不值这个代价。建议从头按边界日志重做,几十行加一两条真实 ToolRuntime 的测试;@yihanzhu 这意味着 issue 放掉验收第 2 条和「从 digest 校验的 lineage 派生」的措辞,若有我没看到的理由请在这里说。四轮的修改很认真,ordering guard 和 crash harness 做得很好,要改的是形状不是态度。
|
@Astro-Han Thanks for the detailed review. I agree with the threat-model point: the model cannot invoke continuation directly, and safe-boundary continuation is an explicit Host/client recovery action. Given that, carrying invalid/unresolved correction budgets across continuation segments is not worth the projection and reconciliation complexity. I’m going to revise #4308 so that continuation restores only an explicit user denial from the Host-owned sandbox request log. New continuation segments will start invalid/unresolved correction budgets at zero. Host lifecycle closures such as I will remove the sandbox negotiation RuntimeEvent projection/reconciliation and the related continuation metadata and compatibility-epoch change. I will retain the separate continuation provider-replay lineage, claim, and digest validation because those are still required to authenticate the replayed model context. This changes #3731 acceptance item 2: correction budgets will no longer be required to survive continuation boundaries. The live I’ll update the PR description and add focused tests against a real |
|
Technical implementation plan for the revised direction:
I’ll keep the current host-restart regression commit ( |
|
@testikun Yes, that is the criterion I have in mind, and your summary is exactly right: continuation restores a real user denial for the Turn from the Host-owned request log, lifecycle closures are not decisions, budgets start at zero on a new segment, and the live Before the issue text changes, I would like this settled here with the issue's author. @yihanzhu, item 2 of #3731 is the one that goes: correction budgets would no longer survive a continuation, and the "derive from digest-validated lineage" wording with it. If there is a reason the budgets must carry over that we are not seeing, this thread is the place. Once we agree here, testikun can edit the issue and push the rewrite, and I will review it fresh rather than as a fifth round. |
|
Thanks for bringing this back to the acceptance criteria. I think there are two separate questions:
Acceptance item 2 was intended to make the three-round limit apply to the whole logical Turn. Resetting the budget for each continuation does not grant additional sandbox authority, but automatic activation or recovery can create another correction window. It therefore weakens aggregate convergence and may increase repeated attempts and cost. Before changing #3731, could we evaluate a simpler single Host-owned durable negotiation record keyed to the continuation chain? That would preserve the denial latch and correction counters directly, without reconstructing them from RuntimeEvent history plus the SQLite request log. If that design is still not viable, please explain the concrete crash-consistency or complexity problems. We can then make an explicit decision about accepting per-segment budget reset. Whichever direction we choose, an explicit denial should come from unambiguous client-decision evidence rather than being inferred from generic lifecycle cleanup. For now, please leave #3731 unchanged. I am not approving or rejecting the current PR head here; I would like us to settle the contract before more implementation churn. 简体中文感谢把讨论重新拉回到验收标准。 我认为这里有两个不同的问题:
验收项 2 原本希望三次限制作用于整个逻辑 Turn。每次 continuation 后重新计数不会扩大 sandbox 权限,但自动 activation 或恢复可以带来一个新的修正窗口,因此会削弱整个 continuation 链上的收敛限制,也可能增加重复尝试和成本。 在修改 #3731 之前,能否先评估一个更简单的方案:由 Host 为整条 continuation 链直接持久化一份唯一的 negotiation 状态?这样可以直接保存拒绝状态和修正计数,而不必从 RuntimeEvent 历史与 SQLite 请求日志中重新推导。 如果这个方案仍然不可行,请说明具体的崩溃一致性或复杂度问题。之后我们再明确决定是否接受每个 segment 重新计数。 无论采用哪种方案,明确拒绝都应该来自无歧义的客户端决定证据,而不应从一般的生命周期清理行为中推断。 目前请先保持 #3731 不变。我在这里既不批准也不拒绝当前 PR head;我希望先把合同语义确定下来,再继续投入实现工作。 |
|
We propose first to evaluate the following simpler design: a single Host-owned durable negotiation record persisted for the entire continuation chain. The goal is to preserve the denial state, invalid/unresolved correction counters, and finalization state directly, without re-deriving them from RuntimeEvent history and the SQLite request log. Implementation direction from current
The decision order I suggest is:
Please let me know if this direction addresses the concerns, or where you see a concrete consistency gap that makes the Host-owned record infeasible. |
Summary
Safe continuations and Runtime restart recovery now preserve the minimal sandbox-boundary negotiation control state for the same logical Turn. The implementation derives denial, bounded invalid/unresolved correction rounds, and finalization state from digest-validated RuntimeEvent lineage plus the authoritative SQLite boundary log when an event/row crash gap exists. Restored state never grants authority; the live
ExecutionBoundaryremains the sole authority.Fixes #3731
What changed and why
Before this change, negotiation state lived primarily in the in-memory
ToolRuntime. A safe continuation or recovered Runtime segment creates a newToolRuntime, so a Turn that had already been denied or had consumed correction attempts could start over and request the same boundary again.This change:
SandboxBoundaryNegotiationStateand one projection function shared by the continuation planner, Runtime kernel, backend, and ToolRuntime.ExecutionBoundary; restored negotiation state can never widen filesystem or network authority.invalid_boundary_declarationas a structured failure reason and rejects malformed, legacy, duplicate, or identity-mismatched boundary facts fail-closed.The important separation is:
This is a convergence and recovery fix, not a new permission grant.
Verification
npm --workspace @maka/core test— 738 passed.npm --workspace @maka/storage test— passed.npm --workspace @maka/runtime-host test— 1,429 passed, 12 skipped.git diff --checkpassed.AI use
Tool(s) and scope: OpenAI Codex analyzed issue #3731, designed and implemented the bounded sandbox negotiation restoration, added regression coverage, and ran the verification listed above. The human contributor remains responsible for review and submission.
Checklist
Does this PR entail a change in behavior?
中文摘要
之前 sandbox 协商状态主要保存在当前
ToolRuntime内存中,因此同一个逻辑 Turn 在 safe continuation、崩溃恢复或 Runtime 重启后创建新的运行段时,可能丢失“已拒绝”和修正次数状态,重新发起权限请求。这个 PR 从经过 digest 校验的 RuntimeEvent lineage 和权威 SQLite boundary log 恢复最小控制状态,并在执行前再次认证。恢复的数据只控制是否继续协商,不会扩大真实 sandbox 权限;达到修正上限或历史异常时会安全进入无工具终止流程;真正的新用户 Turn 会重新开始。