Drive action resolution through batch sync passes - #378
Conversation
github-actions
Bot
added
kernel
Preview:
|
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Submitted 3 actionable inline review comments. |
|
Submitted 5 actionable inline review comments. |
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.
`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.
Five fixes in the action-sync driver and the approve/reject entry points, from review of the batch apply-through work: - A manual approval was a frontier: any earlier undecided action rode along, attributed to whoever clicked, though they had reviewed only the one card. Two authorities now extend the frontier and nothing else -- the click on that exact action, or an auto-approval rule the user enabled for its kind. An undecided action with neither is a gate; the pass stops below it, because applyActionsThrough would apply it too. A click above a gate is refused through the existing `failure` channel rather than by throwing, so a pass carrying several coalesced clicks still applies the authorized ones. Out-of-order application stays impossible, which dependent actions require. `ManualApproval.frontier` -> `action`, since it no longer names one. - The frontier defaulted to 0, a valid gatekeeper-assigned action ID, so "no frontier" sorted at a real action: a staged veto could be delivered at a frontier that would apply an undecided action 0. It now starts at -1. - awaitActionsSettled() only observed idleness; the caller's write landed a microtask later, long enough for submitAction's own waitUntil pass to start. That pass snapshots the action as pending, applies it externally, then skips it at reconcile because the record now reads rejected -- applied in the world, rejected in the log, permanently. Replaced with withSettled(gk, transition), which runs the transition synchronously in the same turn as the idle check. - The resume loop keyed on `description.awaitDecision`, but a turn suspends only when no auto-approval rule will clear the action, so approving an unrelated gate started an unsolicited turn on a chat that had already ended. Suspension is now recorded on the record (`suspendedTurn`) and read back. Records written before this deploy carry no flag, so a turn suspended at deploy time won't auto-resume; the user can continue it with a message. - One chat's resume reaches getChatContext over RPC and can throw, after the pass has durably approved records from several chats, and approveAction's own pending guard blocks a retry. Each per-chat resume is now isolated and logged. Also make veto durability normative: the driver clears vetoPending for every sent veto on any call that returns, including a `stopped` one, so a gatekeeper that failed an apply before recording a veto would lose the rejection and let a later frontier apply it. No gatekeeper implements applyActionsThrough yet, so stating it costs nothing. `failure` now survives rejection -- the case where the gatekeeper may already have applied the action and will ignore the veto as "already applied" -- so the evidence outlives the decision. The approve path still clears it (an applied action has no failure), as does the cascade path (the gatekeeper deleted the action, so the reason is stale).
ActionLogEntry.failure is the gatekeeper's display-safe reason the last apply attempt stopped at an action -- "specific enough for the user to resolve the problem", per the contract -- and until now it reached no one: nothing in the frontend read it, and useResolveAction collapsed the thrown message into a generic "Failed to approve action" toast. The new ActionFailureNote renders it at the four places a user meets an action: the Activity review list, an expanded history row, and both branches of the chat action card. The toast now surfaces the server's message, matching the rewind handler's existing pattern. This is also how a refused approval explains itself -- the driver writes the "approve <gate> first" reason onto the clicked action -- so the note appears on the card the user just clicked, in the list they are already looking at.
c2151bf to
10514b4
Compare
| } catch (error) { | ||
| return {result: {stopped: { | ||
| at: action, | ||
| reason: error instanceof Error ? error : new Error(String(error)), |
There was a problem hiding this comment.
P1: Legacy applyAction() exceptions do not satisfy the new display-safe stopped.reason contract. Converting the raw exception here causes its message to be persisted as ActionRecord.failure and broadcast/rendered to workspace users; current gatekeepers may include provider or internal details in thrown errors because those errors were previously only failures, not UI data. Use a fixed safe message for this fallback (or another explicitly sanitized channel) rather than treating arbitrary legacy exceptions as user-facing text.
| console.error(`Failed to ${decision} action:`, error) | ||
| toasts.add({ title: `Failed to ${decision} action`, variant: 'error' }) | ||
| toasts.add({ | ||
| title: error instanceof Error && error.message |
There was a problem hiding this comment.
P1: Only ApplyActionsThroughResult.stopped.reason is contractually display-safe, but this now renders every exception message from approveAction()/rejectAction(). Transport, validation, profile lookup, and gatekeeper RPC failures can contain internal or provider-controlled details. Keep the generic toast for arbitrary exceptions and return a typed/display-safe error when the server intentionally needs to explain a blocked or stopped action.
| } | ||
| for (let chatId of chatIds) { | ||
| try { | ||
| await this.#maybeResumeAfterActionDecision(chatId); |
There was a problem hiding this comment.
P1: A follow-up pass can coalesce approvals from different clients, and each waiter receives the same decided set (the driver test explicitly asserts this). Every approving client therefore enters this loop for every affected chat, while #maybeResumeAfterActionDecision() and #resumeSuspendedAgent() use that client interface’s #clientUser for the summary author and getChatContext(). Concurrent collaborators can race and resume another approver’s chat under the wrong user’s model credentials/billing identity, or fail to resume it if that user lacks the model. Resume each chat once with persisted initiator context, or return only the records attributable to each request.
|
Submitted 3 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.