Uh oh!
There was an error while loading. Please reload this page.
fix: explain a sandbox boundary request closed by a host restart - #1619
Merged
Conversation
Startup recovery already settled a pending sandbox boundary request fail-closed (`deny` with `closureReason: 'host_restarted'`), but the fact stopped at the request row: the interrupted turn recovered as a generic `app_restarted` failure, so a user who never saw the prompt resolved had no way to connect the two. Recovery now keeps the ids it closed and, using the RuntimeEvent ledger as the only durable map from request id to run and turn, re-attributes that turn's failure to `sandbox_boundary_closed_by_restart`. An id must appear in both the closure set and the run's ledger to claim the attribution, so a stalled child run or an ordinary interrupted turn keeps its original class. The active-run overlay also carries sandbox boundary requests and decisions now, alongside the permission facts it already kept, so a pending request stays visible in the read-model view instead of vanishing whenever the messages come from the in-flight projection cache. The fail-closed deny is unchanged; nothing becomes answerable again. Refs #1612
…tart The failed-turn banner reads `sandbox_boundary_closed_by_restart` through the existing `describeTurnErrorClass` seam, checked before the generic prefix list so it never degrades into the "waiting for permission" or "tool failed" catch-alls. Recovery guidance is `retry`, not the `continue` a plain restart gets: the request was denied and its backend generation is gone, so there is nothing to resume into — retrying the turn is the only way to let the agent ask again. Refs #1612
…uests The request row is the earliest durable trace of a boundary prompt — it commits before the matching RuntimeEvent, whose append is fail-open — but it recorded nothing about the work it interrupted. Recovery therefore had to reconstruct that link from the ledger, which a crash in that window erases. Schema 14 adds `turn_id` and `run_id` to `sandbox_boundary_log`, plus `listSandboxBoundaryRestartClosures()`: the settled `denied` + `host_restarted` rows, indexed and re-readable long after they stop being pending. That is what a recovery interrupted before its terminal commit needs on its next attempt. Because the query filters on the settled outcome, a settlement that was a no-op — the request had already been approved or plainly denied — can never be described as a host-restart closure. Legacy rows read back with no provenance. They are long settled, so "not attributable" is the honest answer and no backfill is invented. Refs #1612
The attribution previously rested on the intersection of two volatile things: a `Set<string>` of ids the current recovery pass happened to close, and a matching request event in the RuntimeEvent ledger. Two reachable orderings broke it — a crash between the row commit and the fail-open event append left no ledger event, and a recovery interrupted before the terminal commit could never re-derive the set, because the rows it settled are no longer pending. ToolRuntime now writes the turn and run onto the request itself, and recovery reads the settled closures back from the store. The set, the ledger scan, and the parameter that threaded the set into `recoverAgentRunsFromLedger` are all gone; a closure claims a run by `runId`, or by `turnId` for rows predating run identity. Covered against the real SQLite + JSONL stores across three separate store lifetimes, including a recovery interrupted after settlement, which must attribute on the next attempt and stay a no-op on the one after. Refs #1612
Astro-Hanforce-pushed
the
fix/sandbox-boundary-pending-restart
branch
from
July 29, 2026 15:42
3173db7 to
67728f6CompareUh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When the host restarted while a sandbox boundary request was awaiting the user, the prompt disappeared and the turn ended as a generic failure. Recovery settles every persisted pending request as
denywithclosureReason: 'host_restarted', which is correctly fail-closed and leaves nothing hanging — but a request the user never saw resolved was closed against them silently, and the turn was lost with no explanation.This PR delivers the second half of the issue's expectation: the closure is explained. It does not make the request answerable again — the response authority is keyed to a backend generation, and when the process dies so does the turn's execution context, so that needs a resume capability and is tracked separately.
The attribution is owned by the request row. An earlier revision of this PR matched "ids this recovery pass just closed" against the run's RuntimeEvent ledger, and argued for not touching the storage schema. That was wrong, and two reachable orderings broke it:
pendingrows and the link was lost for good.So provenance now lives where the earliest durable fact already is.
sandbox_boundary_loggainsturn_idandrun_id, written in the sameINSERTas the request.CreateSandboxBoundaryRequest.turnIdis required, so the one real producer cannot forget it. Recovery readslistSandboxBoundaryRestartClosures(sessionId)— rows already settled asdeniedwithhost_restarted— and attributes byrunId, falling back toturnIdonly for pre-provenance rows.This deletes more than it adds: the in-memory
Setof closed ids, the extra parameter threaded intorecoverAgentRunsFromLedger, the ledger scan and its helper, and the boolean-returning wrapper around the settlement are all gone — −47 lines of production logic. Two properties stopped being checks and became structural: mis-attribution is impossible because the query filters on the settled outcome, so a no-op settlement can never enter the set; and idempotence needed no state bit, because on a later pass the run is already terminal andclassifyAgentRunRecoveryreturns nothing.Desktop names the failure instead of showing the generic restart message, and offers
retryrather thanapp_restarted'scontinue, because there is no context left to continue from. The fail-closeddenysemantics are unchanged.Closes#1612. Refs #1607.
Migration
Schema 14 adds the two columns plus a partial index on settled closures. Additive, applies to any v13 database. Legacy rows decode with no provenance and attribute nothing — no backfill, since those requests were settled long ago. There is only one storage path to migrate:
FileSessionStorenever held these rows and throws'Sandbox boundary requests require the SQLite metadata control plane'.Verification
Rebased onto current
main(which now carries #1618 and #1610) and re-run there:@maka/runtime@maka/desktopmain@maka/storage@maka/coreThe two orderings that broke the old design are now regression tests against real stores —
createSessionStore+createAgentRunStore+createRuntimeEventStoreover one temp root, driven through three separate store lifetimes (seed, recover, assert), each opening and closing its own handles, so the durable state really does cross a process boundary:turn_state.Both were verified to be load-bearing: stubbing
listSandboxBoundaryRestartClosuresto return[]fails them withactual: 'app_restarted', and restoring it makes them pass.Also locked: an answered request keeps the generic restart class; a reasonless
endTurndeny never reads as a restart (plus a store-level test that the query returns only thehost_restartedrow out of four settlements); a closure belonging to another turn/run is never lent to this one; the boundary overlay keeps itssandboxBoundaryDecisionhalf, not just the request; request provenance is durable and a reuse that changes it is rejected; and a v13 database migrates with legacy rows left without provenance.npm run typecheck,format:check,lintclean. E2Esandbox-boundary-takeoverandsession-managementpass.Known gap
No cross-restart E2E. Not because the harness cannot relaunch, but because the fixture's boundary prompt is synthesised in memory:
sandboxBoundaryState()injects aSandboxBoundaryRequestEventinto renderer interaction state with no durable row behind it, andseedE2eFixture()re-seeds on every launch, so a second launch would find nothing to recover. A real journey needs durable pending-row seeding, a non-terminal run and ledger for recovery to attribute, first-launch-only seeding, and a stableuserDataDirfixture. That is its own piece of work; the real-store integration tests above cover the durable path in the meantime.Follow-up
"Make a boundary request answerable after a restart" needs its own issue. One constraint worth recording:
permission-response-ipc-boundary.test.tsdeliberately forbids the renderer from turning an ownerless persisted request row into an actionable prompt. That guard is correct — any future design has to establish a new owner for the persisted request and then relax it, not route around it.