Drive action resolution through batch sync passes - #378
Conversation
github-actions
Bot
added
kernel
Preview:
|
|
Four fixes in the batch sync driver and its approval entry point: - The legacy per-action fallback (still the only path any gatekeeper implements) applied every action before persisting any approval. A failure after an apply landed left the record pending, and a replayed legacy applyAction throws on an already-applied action, wedging it forever. Approvals are now persisted as each one lands, through one idempotent approve() the batch reconcile replays harmlessly. - A legacy rejectAction failure cleared vetoPending unconditionally. Only a DO reset -- rolled back, hence provably undelivered -- now keeps the veto staged for the next pass; a settled/unknown action still clears, since it would throw forever. - A cascade invalidation naming an action submitted during the RPC await was discarded, leaving it pending to be recorded approved later though the gatekeeper had deleted it. The action map is refreshed before reconciling when invalidations are reported. - approveAction() returned success whenever the action was no longer pending, including when the same pass cascade-rejected it, which the client displays optimistically as approved. It now throws unless the record ended approved, and resumes awaitDecision turns first so a failure here can't strand another chat.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| // undecided action from the same gatekeeper (attributed to this approver), then continues | ||
| // through any auto-eligible actions the cleared gate unblocked. | ||
| let decided = await this.impl.syncActions( | ||
| action.gatekeeperId, {frontier: action.action, resolvedBy: profile}); |
There was a problem hiding this comment.
P1: This turns the existing singular Approve button into approval for every earlier pending action on the connection. Those earlier actions can belong to other chats and have unrelated side effects; the chat UI shows only the clicked card, and even the activity popover exposes controls before/all without communicating this frontier behavior. A user can therefore authorize actions they have not reviewed. Until the batch UI explicitly presents and confirms the covered set, approveAction(id) should not silently treat the click as authority for earlier manual gates.
| } | ||
| } | ||
| for (let chatId of chatIds) { | ||
| await this.#maybeResumeAfterActionDecision(chatId); |
There was a problem hiding this comment.
P1: The batch has already durably approved actions from every chat before this sequential resume loop runs. If one #maybeResumeAfterActionDecision() rejects (for example, getChatContext() or the profile lookup fails), the remaining chat IDs are skipped. Retrying the original approval cannot recover them because it now fails the initial state !== pending check, and future syncs no longer return those already-decided IDs, so those suspended turns stay stranded. Attempt every affected chat (for example with all-settled handling) before propagating a failure, or persist resumable work.
| } | ||
| // Still pending: the gatekeeper stopped at (or before) this action; surface the | ||
| // display-safe reason so the user can resolve the problem and retry. | ||
| throw new Error(fresh.failure ?? `Failed to apply action: ${id}`); |
There was a problem hiding this comment.
P2: When the gatekeeper stops at an earlier action, the specific failure is stored on that earlier record, while the clicked frontier record has no failure and this returns only Failed to apply action: <id>. The unchanged useResolveAction also discards the RPC message, and no action UI renders the new failure field. Thus the user gets no indication which earlier action blocked the batch or the display-safe reason the contract requires them to resolve. Surface the stopping record/reason through this call or render failures on pending records before enabling frontier approvals.
|
Submitted 3 actionable inline review comments. |
`syncActions` read as a read/mirror operation, but the call sends irreversible external writes. Renamed to `applyDecidedActions`, which also names the union the pass delivers: approvals (manual, then auto-eligible) applied, rejections delivered as vetoes. `settledActionSync` -> `awaitActionsSettled`, and the driver follows so the vocabulary doesn't split at the boundary: `sync` -> `apply`, `#syncOnce` -> `#applyOnce`, `settled` -> `awaitSettled`, `StagedSync` -> `StagedPass`. The class, log component, and event prefix keep "sync": the subsystem is action-sync, its operation is applying decisions. Also log a failed pass. Two callers deliver one through `waitUntil` (rejection, auto-approve opt-in) and never observe the rejection, so a background veto or auto-approval that failed to reach the gatekeeper left no structured trace.
| async awaitSettled(gatekeeperId: number): Promise<void> { | ||
| for (;;) { | ||
| let running = this.#running.get(gatekeeperId); | ||
| if (!running) return; |
There was a problem hiding this comment.
P1: awaitSettled() only observes that the driver is idle; it does not reserve it. Since rejectAction() then awaits this call, a concurrent approval can start an apply pass before the rejection resumes and persists its state. The gatekeeper can apply the action externally, after which reconciliation sees the freshly rejected record and skips marking it approved, leaving the audit log rejected despite the side effect. Serialize the rejection transition through the same per-gatekeeper queue, or hold a reservation through the re-check and write.
| logger.warn("action sync pass failed", { | ||
| event: "action.sync.failed", gatekeeperId, error, | ||
| }); | ||
| slot.reject(error); |
There was a problem hiding this comment.
P1: A failed or lost batch response discards the manual approval intent while leaving the record pending. If the gatekeeper applied the action before the response was lost, the user can next reject that record; the new contract says a veto of an already-applied action is ignored, and this driver then clears vetoPending, so the durable log says rejected even though the effect happened. Persist unresolved approval intent or add an outcome-reconciliation result that distinguishes an already-applied action before accepting a rejection.
| * yet applied). Action IDs listed in `vetoes` are actions the user has rejected. | ||
| * | ||
| * Actions are applied in ascending ID order. Vetoed actions and actions invalidated by a veto | ||
| * become terminal no-ops. Processing stops at the first application failure; a pending in-range |
There was a problem hiding this comment.
P1: The contract says processing stops at the first application failure, but it never requires every supplied veto to be durably processed before applications begin. A compliant ascending implementation can fail applying action 1 before handling veto 2; the caller nevertheless clears veto 2 on any stopped result (actions.ts:259-267), permanently losing the rejection. Require vetoes to be durably accepted first, return acknowledgements, or retain sent vetoes when processing stops.
| for (let recordId of decided) { | ||
| let record = this.impl.storage.actions.get(recordId); | ||
| if (record?.type === "action" && record.caller.from === "agent" && | ||
| record.description.awaitDecision) { |
There was a problem hiding this comment.
P1: This checks the requested awaitDecision, not whether the turn actually suspended. An auto-eligible awaitDecision action blocked behind an earlier manual gate does not latch suspension in submitAction() because willAutoApprove is true, so its agent finishes normally. When the earlier gate is later approved, this batch includes the auto action in decided and starts an unsolicited new agent turn after the original has ended. Persist/check actual suspension state rather than inferring it from the description.
| // chokepoint, and every transition must record the resolving user and whether it was | ||
| // automatic. | ||
| let manualAscending = manualApprovals.toSorted((a, b) => a.frontier - b.frontier); | ||
| let frontier = manualAscending.at(-1)?.frontier ?? 0; |
There was a problem hiding this comment.
P1: 0 is a valid sequential integer under the public contract, but using it as the empty frontier can apply action 0 without approval. With pending action 0 and staged veto 1, firstUndecided ignores action 0 (record.action > frontier), advances the frontier through veto 1, and sends applyActionsThrough(1, [1]); action 0 executes but has no attribution and remains pending locally. Represent an absent frontier below every valid ID, or explicitly require and validate positive action IDs.
|
Submitted 5 actionable inline review comments. |
1 participant
This adds the shared
applyActionsThroughcontract and replaces the overseer’s auto-approval drainer withActionSyncDriver. 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 publicapproveActionandrejectActionRPC signatures remain unchanged. Frontend behavior is intentionally unchanged; the existing per-action controls stay in place, and batch UI work will follow separately. The contract requires idempotency and re-reporting persisted invalidations so a crash cannot misattribute cascade-invalidated actions. Covered by the backend tests and workspace build and lint.