Batch action application with per-action veto by ndisidore · Pull Request #528 · cloudflare/cloudflare-os

ndisidore · 2026-09-18T13:14:04Z

This adds the shared applyActionsThrough contract and replaces the overseer’s auto-approval drainer with ActionSyncDriver. The driver resolves pending actions in ordered passes, stages vetoes, records failures, and falls back to the existing per-action calls for gatekeepers that have not migrated. The public approveAction and rejectAction RPC signatures remain unchanged but are now depreciated.

Frontend behavior is intentionally minimal and the primary difference is that action cards now render the gatekeeper's failure reason, and there is distinction between "Denied" and "Invalidated" (new label); the existing per-action controls stay in place, and batch UI application work will follow separately.

Design Decisions

One ordered prefix per call. Frontier and its vetoes in a single RPC. Leaves ordering and cascade reporting where the provider can answer them.
Checkpoint at the provider boundary. Vetoes durable as vetoPending before the call, cleared only on acknowledgement. Same discipline in the legacy path, one checkpoint per acknowledged call.
Ordering is the provider's obligation. Retained actions published in ascending order, so a frontier means what it says. Cheaper than a second reconciliation system in the kernel for late IDs.
A stop leaves the action pending. Bounded, display-safe reason, no resolver attribution. The gatekeeper said why it stopped, not whether the work landed. That reason then gates every unattended path: rules skip it, the card withdraws "Always approve", and a stop outranks a gate when both hold.
Pack authority expires with the invocation. Disposable builder over the authorized pushes only. Cache stays readable for simulation, can't mint packs.
Legacy fallback narrow and temporary. Probe only on the runtime's missing-method error; coded and ordinary failures never replay. Deletable once providers migrate.

github-actions · 2026-09-18T13:16:22Z

Preview: `pr528-feat-action-a-7a61eb44`

https://pr528-feat-action-a-7a61eb44-router.cloudflare-os-previews.workers.dev

Dashboard · deleted when this PR closes

ask-bonk · 2026-09-18T13:26:11Z

Submitted 3 actionable inline findings.

github run

ask-bonk · 2026-09-18T17:41:25Z

Submitted 4 actionable inline findings.

github run

ask-bonk · 2026-09-19T00:03:01Z

Submitted 2 actionable inline findings.

github run

ask-bonk · 2026-09-22T18:27:57Z

Eval runs review

Measured. No cohorts are comparable; all three are marked “eval definition changed.”

github run

ask-bonk · 2026-09-22T19:01:57Z

    // Deny leaves the turn ended, like denyConnectionRequest. The rejected record also prevents a
-    // sibling approval from resuming this turn.
+    // sibling approval from resuming this turn. Rule-approved actions it unblocked apply now.
+    this.#applyDecidedInBackground(action.gatekeeperId);


[P1] Resume with the suspended turn's original user context. This new pass runs through the rejecter's OverseerClientInterface: if Alice's await-decision action is auto-authorized behind a blocker and Bob rejects that blocker, the action applies under Alice's saved rule, but #resumeSuspendedAgent() resolves the original model ID through Bob's #clientUser and attributes the continuation to Bob. Bob may not have that model, leaving Alice's chat suspended, or a matching ID can run with Bob's model config and credentials. Persist/use the suspended turn's initiator user rather than the collaborator who happened to trigger reconciliation.

Hmm so Bob can resume Alice's suspended turn without deciding anything about it. That turn then runs on Bob's model record and Bob's user id. It needs a workspace with several builders who share a connection, a rule, and an awaited action.
main does't really do any tracking here: I could add this with about 15-20 lines of kernel code but for now it may be pragmatic to defer

ask-bonk · 2026-09-22T19:02:00Z

Submitted 1 actionable inline finding.

github run

ask-bonk · 2026-09-22T19:05:46Z

Eval runs review

Measured. No cohorts are comparable; all three are marked “eval definition changed.”

github run

ask-bonk · 2026-09-25T15:09:22Z

LGTM!

github run

ask-bonk · 2026-09-25T15:24:46Z

Eval runs review

Measured. Appointment-desk stayed 10/10, with duration +0.3 s, tool errors +0.6, and cost −$0.00004; project-doc stayed 9/10, with duration −1.5 s, tool errors unchanged at 0, and cost −$0.00004.
Trajectories. In appointment-desk trial 6, turn 1, the candidate used unsupported SQL .one(), incurred five tool errors, and rewrote the server after five failed edits.
Caveats. Ten stochastic trials and candidate run errors that excluded expense-ledger limit attribution.
Verdict. unchanged — pass rates were identical and efficiency changes were small and mixed

github run

Declares the batch action surface on both sides of the kernel boundary. Provider side (gatekeeper.ts): `applyActionsThrough(actionId, vetoes, context)` processes every non-vetoed action through one boundary, with vetoes durable before any application and `stopped`/`invalidatedByVeto` reporting what did not land. `ApplyActionContext` carries the invocation-scoped `GitCache` and `GitPackBuilder` capabilities, so pack building is authorized by one call's non-vetoed prefix and revoked when it returns. `applyAction`/`rejectAction` stay for the per-action fallback every shipping gatekeeper still uses. Workspace side (api.ts): `Overseer.applyActionsThrough(id, vetoes)` takes workspace record IDs and translates them, never exposing provider-local action IDs to the browser. `ACTION_ERROR_CODES` classifies the expected outcomes by `code` rather than message text, on the generic `codedErrorFamily` helper. Declarations and doc comments only; implemented in the commits that follow.

`ActionSyncDriver` (src/actions.ts) owns one serialized pass per connection, replacing the per-action auto-approval module this commit deletes. A queue carries both an explicit batch and a legacy immediate rejection, so a click arriving mid-pass can neither interleave with it nor be lost. An explicit batch authorizes the whole pending prefix at or below its boundary to the calling user, delivers only the vetoes inside that boundary, and leaves later ones staged and indexed for a covering pass. Vetoes are persisted as `vetoPending` before the gatekeeper is called and acknowledged only after it confirms them, so a failed delivery is retried rather than lost, and a rejected push keeps its simulation read grant until the veto lands. Reconciliation applies cascade invalidations before approvals, records `stoppedAt` for the action that halted a pass, and stamps every mutation so the resume replay sees it. Native `applyActionsThrough` is preferred with a per-action fallback pinned to a missing-method error; the fallback checkpoints each veto and aborts before any apply when one fails. `GitPackBuilderImpl` (git-cache.ts) snapshots the invocation's authorized local selectors and revokes retained duplicates when the call ends. Tests: actions.test.ts drives the real driver and client over the production storage schema, subsuming the deleted auto-approval suite; git-push-actions.test.ts proves the Git capability in workerd through `TestGitPackGatekeeper` - real SQLite, facets, RPC and pack streams, including decoded pack contents, mark conversion and capability expiry.

A pass that stops leaves its action pending with the gatekeeper's reason, so the Workshop has to present a pending card that already failed once. `ActionFailureNote` renders that text on both surfaces, Activity distinguishes a cascade invalidation from a direct denial, and `useResolveAction` classifies the expected outcomes by error code instead of message text. `useActions` gains `deliverEntry` so one consumer's throwing listener can neither abort delivery to the others nor abort the mount-time replay before its effect returns the cleanup that releases the shared subscription.

Make the driver's decision queue the sole batch-validation authority, reduce PassResult's blocked/stopped to the booleans callers consume, drop a re-check the vetoPending index already guarantees, resume multi-chat suspensions in parallel, and consolidate duplicated test scaffolding behind makeBatchGatekeeper's parkAt and one parameterized mid-build invalidation test.

The frontier model the batch path relies on assumes a gatekeeper's local action IDs increase: `applyActionsThrough` authorizes by range, so an action submitted below a boundary the user already decided would be applied without a recorded approval. Nothing enforces that, and no shipped gatekeeper takes the batch path yet, so the assumption is untested in practice. Record each connection's highest submitted ID in memory and warn when one arrives out of order. Nothing is rejected: the count tells us whether the promise holds before the first native implementer makes enforcement worthwhile, whereas rejecting now could only fail a submission the legacy path would have handled correctly.

Activity learned to distinguish a cascade invalidation from a direct denial, but the chat card kept mapping every rejected action to "Denied". The same record then read "Invalidated" on one surface and "Denied" on the other, and the chat label attributed a cascade to a decision nobody had made about that action. `actionStatusLabel` is now the single mapping from a record's state and `cascadedFrom` to its display label, and both surfaces read it. Deriving it twice independently is what let them disagree.

`applyActionsThrough` authorizes by range, and `invalidatedByVeto` may name an action above the boundary, so the contract already leans on a gatekeeper's published IDs being a prefix -- but only the in-flight-submission rule said so, and that rule reads as though a lower ID could still appear mid-pass. A gatekeeper that published 3 before 2 would hand the overseer a frontier covering an action it has never seen, or a cascade rejection for a record it cannot hold. State the obligation where it is discharged: `submitAction()` publishes retained actions in ascending ID order, a frontier therefore covers every retained action below it, and an invalidation is reported only once its action's submission has completed. Missing IDs stay legal; nothing requires a contiguous sequence. The cascade-publication test now parks the RPC and publishes action 3 while it is parked, which is the interleaving the contract permits. Writing after the call returned modelled a gatekeeper the contract forbids.

An approval or batch already waiting in a connection's decision queue was planned as though nothing had happened since it was made. When the pass ahead of it stopped, the stale request became authority to retry the action that had just failed: a double-click on Approve reached the provider twice for one intent, the second attempt repeating a side effect whose outcome the first call never reported. A queued batch went further and applied the failed action under a boundary the user chose before the failure existed. The persisted `failure` only holds the automatic path back; an exact click or an explicit batch walks past it by design, which is what a human retry is. Stamp each request with the connection's stop count as it is admitted, and count structured stops on the run that records them. A click authorizes its action only if it was admitted no earlier than that action's latest stop, and a batch is refused when an in-range action it does not veto stopped after the batch was selected. Refusal writes nothing: no veto is staged, no reason is overwritten, no rule is dropped, and the caller gets ACTION_STOPPED rather than a gate error telling them to go and approve something they already did. Vetoing the failed action, or asking again once its reason is on the card, both go through. The state is deliberately ephemeral, per connection, and lives only as long as the run loop that owns the queue. Requests do not survive a restart either, so there is nothing for a durable generation to protect.

A pass can be both blocked and stopped. An undecided gate lowers the frontier and stops planning, but the prefix already authorized under it still goes out, so the gatekeeper can fail on one of those actions and record a reason on its card. `approveAction` checked `blocked` first, so the caller was told an earlier action needed a decision -- true, but the lesser of the two problems, and it sent them the wrong way. Approving the gate then fails again: the failed action now carries `failure`, which disqualifies it from the rule path and makes it a gate of its own. Two round trips, and neither error pointed at the card that explains why. Prefer the stop. It is the deeper blocker, and the only one of the two with a persisted reason -- its copy sends the user to the cards, where the undecided gate is visible as well. `blocked` alone still reports blocked, and the batch wrapper is unaffected: it never reads `blocked`, because an explicit boundary authorizes its whole prefix and cannot gate.

On an unresumed reconnect the cold-open sweep refetches every cached card whose log can still change, racing the action subscription. Its guard only rejected a pending read over a resolved card, so two pending snapshots were treated as interchangeable. They stopped being interchangeable when a pending card gained a `failure`: a read taken before an apply stopped could land after the subscription delivered the stop, silently erasing the reason and leaving an unexplained pending card until something else touched the record or the page reloaded. Reject a read whose change stamp is older than the cached card's. Every mutation stamps `appliedAt`, including the one that records a stop, so it already orders the two snapshots; `createdAt` stands in for a record nothing has touched, which keeps an equally-fresh read applying as before. Resolution monotonicity stays as the separate check, since a resolution delivered without a stamp has no watermark to compare.

`vetoIds` names a selection of records, and a selection is a set. Repeating an id said nothing the first copy had not: multiplicity carries no meaning here, staging is order-independent, and the batch has a single `resolvedBy`, so there is nowhere for a second copy to mean anything. What it did do was read and write the same record once per copy inside the staging transaction. Normalize the argument to a set before validating it. Nothing observable changes -- duplicate staging already converged on identical state, and the vetoes sent to the gatekeeper come from the vetoPendingByGatekeeper index rather than this list, so it never doubled a delivery. Validation is unmoved: `Set` preserves first-occurrence order, so an invalid id still aborts the call at the same place with the same error.

`buildPack()` rechecks the action's lifetime after awaiting the build, so a connection removed mid-build yields GIT_PACK_ACTION_UNAVAILABLE rather than a half-built stream. But the build can also reject: `buildPackForAction` pulls missing objects from the gatekeeper, and removing the connection breaks the very stub that pull is waiting on. The rejection then propagated ahead of the recheck, so the caller saw the pull's failure instead of the coded error the contract tells gatekeepers to expect -- and since they must propagate unknown errors, it bypassed the structured `stopped` handling those codes exist to drive. Recheck on the failure exit too, preferring the lifetime error when the action is gone and rethrowing the build's own failure when it is still live. An invalidated action's pull failure is a consequence of the invalidation, not independent information; a genuine pull failure is, and must not be masked as a lifetime error. The existing mid-build table covered only the build-succeeds half of this guard, so its sibling covers both directions of the other.

A cleanup pass over the branch, removing checks whose failing branch no code path can enter, and docs that restated a rule already stated where it belongs. `isMethodMissing` guarded against a coded Git pack error whose message resembled workerd's method-missing prose. The four pack messages are fixed strings that contain no such text, so the guard, its import, and the test that had to hand-mutate an error's message to reach it are gone. `GitPackBuilderImpl` rechecked `action` and `gatekeeperId` on a record it looked up by a map keyed on exactly those values, populated from this connection's own pending plan; both fields are immutable on a record. The dispose-time map clear was dead behind the `#active` flag. The fourth code, GIT_PACK_ACTION_DECLARES_NO_PUSH, is folded into "not authorized": the constructor filters on `pushedCommits?.length`, so an empty declaration never enters the map, and the contract already tells implementers to omit the field rather than pass `[]`. The next commit closes the one producer that could forward `[]`. `applyThrough` re-read the boundary's connection inside the queue and compared it to the pre-queue read; a record never changes connection. `reject()` re-read the record after the gatekeeper call, but the decision queue holds across that await, so nothing can decide it meanwhile. The legacy veto loop logged and rethrew a failure the run loop already logs and batch callers already receive. Docs: the publish-order rule was stated three times; `applyActionsThrough` now points at the normative text on `submitAction`. The hand-written function types on the pack error exports now match their siblings in api.ts. Tests: the two-scenario pack test is split so a failure names its scenario, the frontend card renderer is shared between the pending and resolved cases, and the RPC-safe `expectGitPackCode` helper gains a comment saying why it must not become `expect().rejects` -- handing an RPC-stub promise to vitest leaves an unhandled rejection behind in workerd.

The kit forwarded a provider's `pushedCommits` whenever it was truthy, and `[]` is truthy. A `describe()` that computed "commits to push" and found none would put `pushedCommits: []` on the wire, which the overseer reads as a push declaration: it verifies no ancestry and marks no objects for it, and the pack builder refuses to build for it, so the action could only stop at apply time with a message that did not name the mistake. Filter on length instead of presence. An empty list is "no git", the same as no key, and now produces the same wire shape. The kit was the only producer that could forward `[]`; the GitHub gatekeeper always declares one commit directly. The existing no-key test is parameterized over the empty list so the truthy hole stays pinned as observable wire behavior.

The offer's own comment says it appears only when enabling a rule would actually apply this action, and it checked three of the conditions for that: a tagged action, on a connection, that the gatekeeper marked auto-approvable. A recorded failure is a fourth, added later -- it disqualifies the action from the rule path, since nothing unattended may retry a side effect whose outcome the gatekeeper never confirmed. So on a stopped card the button still appeared, the confirm dialog still promised application, and enabling the rule left that action pending, with any agent turn awaiting it still suspended. Check the failure in both gates. The rule stays creatable from the auto-approval panel and Approve still works on the card, so nothing is lost except a promise that could not be kept. The backend is untouched: refusing to replay a stopped action unattended is the behaviour the offer was misreporting, not a bug in it.

`ActionSyncStorage` restated the overseer's action schema by hand: a collection plus the two indexes the driver reads. A hand-mirrored shape can drift from the thing it mirrors, and the tests already build their storage from the production `makeOverseerStorage`, so the interface bought nothing the real type does not give. The `applyLegacyAction` hook existed for a narrower reason: the driver had no way to build an action-scoped `GitCacheImpl`, so the overseer supplied the whole call instead of just the cache. Derive the storage type with `Pick`, and give `createGitCache` the optional action id the legacy path needs so the driver can make its own call. Two hooks become one and the legacy call site now reads like the native one above it. Tests lose a seam they only had to route around: `makeDriver` no longer reimplements the apply, `putAction` defers to the shared fixture, and `rejectionOf`/`rejectBatchProbe` replace the try/catch and the hand-written missing-method TypeError that several suites had each spelled out.

The chat card and the Activity row each decided independently whether to offer "Always approve this type", with the same four-part condition written out twice. Keeping two copies in step by hand is what let them drift: when a recorded failure became a reason to withhold the offer, both copies needed the new clause, and the type the confirmation dialog consumes was declared inline in both files as well. `autoApproveTargetOf` now owns that decision, beside `actionStatusLabel`, which is the same move for the same reason. Each caller narrows to an action entry and asks. The rationale for each clause lives with the code that applies it rather than in a comment duplicated next to each copy.

A gatekeeper that has already applied an action cannot honour a veto of it, and the contract told it to ignore the veto silently. The caller then acknowledges a rejection it never got: the record is written `rejected` for work the provider executed, and because `persistRejected` also runs `clearPushMarks`, a Git push additionally loses the `onRemote` proof that makes its objects re-pullable. The gatekeeper knows which it is; nothing asked it. Report those ids in `alreadyApplied` and reconcile them to applied. Reported in the result rather than thrown, matching `invalidatedByVeto`: a throw would abort every other veto and apply in the batch, and since the caller replays its staged vetoes it would throw again on each retry and strand them. The record keeps no resolver, because the pass that applied it lost its response before recording one and this pass only knows the vetoer, and it sheds any `failure` left by an earlier stop, since `ActionLogEntry.failure` is cleared when an action applies and both cards render the note whatever the state. Only ids this call actually sent are honoured, since vetoes beyond the frontier stay staged and undelivered by design. Inert until a gatekeeper implements `applyActionsThrough`, so populating it is an acceptance criterion of the first native port rather than something today's legacy path can exercise.

A veto the gatekeeper refuses because it had already applied the action reconciles the record from rejected to approved, and `vetoRefused` tells the caller why. Only `applyActionsThrough` reads it. `approveAction` and the background rule passes both ride staged vetoes too, and neither reports anything, so the card flips from Denied to Approved with nothing on it to say the user asked for the opposite. Throwing from those routes is not the answer: `approveAction` returns early once the clicked action reads approved, so the check would have to precede that return and fail a click that succeeded, over a reversal on a different card. Mark the record instead, and give the marked state its own label. The flag rides the same path as `cascadedFrom`, the label comes out of the one helper both surfaces already share, and no route needs to learn to report anything. `actionStatusLabel` now covers both states that would otherwise read as a verdict nobody gave: taken down by an earlier rejection, and applied before a rejection could land.

Three gaps around rejecting an action, each reachable from the card. A veto the gatekeeper refuses as already applied reconciles the record to approved, so the resume gate read it as an approval and could resume the turn once its siblings applied. The user asked to stop; a late refusal does not change that. The gate now treats `vetoRefused` like a rejection. Rejecting an action also unblocks any rule-approved actions queued behind it, but nothing ran a pass until the next unrelated trigger, leaving their turns suspended. `rejectAction` now runs the same background pass-and-resume that enabling a rule does, shared as `#applyDecidedInBackground`. A cold reconnect re-fetched only pending cards, so a Denied card whose veto was refused while disconnected kept reading Denied. It now re-fetches every card that is not approved.

ask-bonk · 2026-09-25T16:16:02Z

LGTM!

github run

ask-bonk · 2026-09-25T16:19:48Z

Eval runs review

Measured. No cohorts are comparable; all three are marked “eval definition changed.”

github run

github-actions Bot added workshop/frontend Changes to the Workshop frontend kernel Changes to the Workshop kernel workshop/shared Changes to shared Workshop APIs labels Sep 18, 2026