Skip to content

fix(tui): deliver approvals for parked calls in queue-drained turns - #1519

Merged
TheGreatAxios merged 14 commits into
mainfrom
cl-10852-deliver-approvals-for-parked-calls-in-queue-drained-turns
Oct 19, 2026
Merged

TheGreatAxios merged 14 commits into
mainfrom
cl-10852-deliver-approvals-for-parked-calls-in-queue-drained-turns

Conversation

@bwachman

@bwachman bwachman commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • A send that drains the TUI delivery queue (an ask_director wake, mailbox mail, or a prompt typed while busy) returns once send() settles. Presenting and delivering an approval for a call that parked during that turn runs outside the session-queue op, still counted as agent-busy.
  • The operator's decision is delivered through sessionOps. With the drained op waiting on the approval, that delivery was chained behind the op waiting for it, so it never ran. The parked call got approval timed out an hour later, the queue stayed wedged, later messages never reached the conversation, and nothing was logged. This hit approve_worker_call whenever a worker asked its director for a denied call.
  • Turns the operator types while idle are unchanged, since they already ran outside a queue op.
  • A detached decision that fails to settle (the approval surface throws, the reactor never accepts it, or the agent closes) shows a system notice. The drained delivery stays accepted, because its message already reached the agent and restoring it would resend it. The shell's run state is left alone: the run is still open, and settling it idle would strand follow-ups queued behind the turn.
  • When the approval surface throws, the parked call is rejected instead of left parked until the reactor's own timeout, since nothing retries a parked call. A call the reactor already timed out gets no second answer. This applies to operator-typed turns too.
  • This branch also carries fix(session): release a settled approval overlay before delivering #1520 (release a settled approval overlay before delivering) and fix(permission): key worker grants on the dispatched call #1521 (key worker grants on the dispatched call), merged into it after their own reviews.

Verification

  • CI passes on the combined branch (all four test shards, typecheck, lint, build)
  • The new test drives the real delivery routing, submit path, session queue, approval resume and approval deliverer. A wake-drained turn's allow-once decision reaches the agent and the queue tail settles; an operator-typed turn still delivers. The wake case fails on main (no delivery, queue never settles) and passes here
  • Two more cases queue-drain a wake whose decision is lost, one where the approval surface throws and one where the reactor never accepts it. Each shows one notice, keeps the shell's run state, and lets the queue tail settle; the throw also delivers a rejection for the parked call
  • In approval-resume.test.ts, a shared gate failure rejects the parked call once and later retries memoize, and a gate failure after the reactor timed the call out delivers nothing (that test fails with the timeout guard removed)
  • Live, interactive TUI without yolo, on a build of this branch: in a turn started by a worker's ask_director wake, the operator's allow-once on approve_worker_call was delivered and the call ran immediately. On v0.3.40 the same flow never delivered and timed out an hour later. (Its nested prompt for the worker's frozen call was then refused unseen, a separate bug fixed in fix(session): release a settled approval overlay before delivering #1520.)

Fixes CL-10852
Fixes CL-10854
Fixes CL-10855

@linear-code

linear-code Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

CL-10852

CL-10855

CL-10854

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review · Request changes

A queue-drained turn detaches approval delivery so the session-queue op is not waiting on a decision it enqueued behind itself.

Findings

  • src/tui/runner/submit.ts:401 — when detachApproval is set and send returns suspended, approvalResume.handle is started with void runWhileAgentBusy(...).catch(...) and the outer busy callback returns immediately. The try then always returns { status: "accepted" } at line 422, before that task finishes. The .catch only logs the error name and does not call handleSendFailure. createGenerationGatedHop settles the drain from that hop result (src/tui/delivery-queue.ts:651), and settleDrainedDelivery treats accepted as done and does not restore the queued text (src/tui/runtime-bridge.ts:620).

    If the operator allows and delivery then fails (ApprovalDeliveryTimeoutError from src/tui/approval-delivery.ts:151, an overlay throw, or AgentClosedError), the reactor has not accepted the decision, handedOver is never set (src/session/approval-resume.ts:501), and suspendedApprovalRecovery.clear() at line 404 does not run. A later retry will not resend: delivered already holds the correlation (approval-delivery.ts:142), and the next tryResumeOnce drops the candidate as already-attempted (approval-resume.ts:176). The synchronous path at line 415 still rejects into handleSendFailure and does not report accepted.

    The new test only covers an immediate allow-once (bun test src/tui/runner/queued-approval-delivery.test.ts, 2 pass). A since-removed repro — queue-drain a wake whose gate throws — logged approval for a queued turn failed to settle and still settled { status: "accepted" } with nothing delivered. Return the detached failure as not-delivered (or surface it the way handleSendFailure does) instead of accepting the drain, and cover the timeout or throw.

@bwachman

Copy link
Copy Markdown
Contributor Author

Fixed in 1da2213 (test) and 9161a31 (fix). I took your second option: a detached failure now goes through handleSendFailure with its notice on, so it records the run error, posts a system notice, and settles the shell idle. The drain hop passes presentFailureNotice = false because it relies on the queue's own notice, and the queue never sees this failure, so the notice is turned on explicitly here.

I kept the drain accepted rather than not-delivered. By the time the decision is lost, the drained message (usually the ask_director wake) has already reached the agent and run a turn. not-delivered would restore it to the composer, and the operator would resend a wake the parent already acted on. Reporting it on the hop result would also mean awaiting the decision inside the op, which is the deadlock this PR removes.

The new cases cover both the throw and the acceptance timeout (bun test src/tui/runner/queued-approval-delivery.test.ts, 4 pass). On the retry: after an acceptance timeout the deliverer keeps the correlation in delivered on purpose, since the decision may still apply (mayStillApply), and the synchronous path behaves the same way. When the approval surface throws, nothing has been delivered or handed over, so a later tryResumeOnce re-presents it.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review · Request changes

A queue-drained approval that fails now shows a failed-send notice. The drain is still marked accepted, and the decision is not retried.

Findings

  • src/tui/runner/submit.ts:415 — the detached .catch calls handleSendFailure and the function still returns { status: "accepted" } at line 426. settleDrainedDelivery (src/tui/runtime-bridge.ts:620) treats that as final, so the wake is not restored. Restoring it would resend the wake, so leaving it accepted is right for an acceptance timeout, where the decision was already handed over. It is not enough when resolveSuspended throws: deliverDecision never runs, nothing calls tryResumeOnce, and the parked call stays parked until the reactor times it out. The new test locks the drain as accepted (src/tui/runner/queued-approval-delivery.test.ts:258). Re-ask, or explicitly reject the parked call. Do not only print approval overlay failed.

  • src/tui/runner/submit.ts:415 — handleSendFailure sets shell.session.run to idle (submit.ts:356) without draining the boundary. A follow-up already queued while the turn was busy keeps its badge. A later reactor.done hits settleRunToIdle (src/tui/runtime-bridge.ts:1287), which returns immediately because run is no longer busy, so that follow-up never drains. The same idle flip happens on the acceptance-timeout path, while the approved call may still be running. A temporary repro confirmed the badge stays and the later done event does not drain. Do not mark the shell idle from this catch.

  • src/permission/gate.ts:982 — for a scope whose pattern !== null, mintGrant persists the session or project allow before createApprovalDeliverer confirms the reactor accepted it. If acceptance then times out, the grant remains for later matching calls while this parked call is unconfirmed, and the shell is already idle. Allow-once mints nothing and is simply gone. Do not persist that grant until the reactor accepts, or roll it back when acceptance fails.

bun test src/tui/runner/queued-approval-delivery.test.ts — 4 pass, 0 fail. Those tests do not cover a re-ask, the stranded follow-up, or the early grant.

corbits-triage Bot and others added 2 commits October 19, 2026 00:43
…rized-retry-on-the-dispatched-call-so-the

fix(permission): key worker grants on the dispatched call
…oval-overlay-from-aborting-permission

fix(session): release a settled approval overlay before delivering
@bwachman

Copy link
Copy Markdown
Contributor Author

Addressed in 014c058 (tests), b1ceab2 and 9a3d500, on top of the branch after #1520 and #1521 were merged in.

  1. Throwing surface: the parked call is now rejected rather than re-asked. A parked call isn't a stall to the watchdog (it only bounds a silent stream and in-flight wait_agents/ask_director), so nothing would call tryResumeOnce to re-ask. When resolveSuspended throws, settleSuspended delivers a rejection ("approval surface failed") and rethrows so the caller still surfaces it. It skips the rejection when the reactor already timed the call out. The surface resolves aborts and timeouts as outcomes, so only real failures reach this path. This changes the contract in your "a shared rejection re-gates once, delivers once, then memoizes" test (b1ba74e): a shared failure now delivers one rejection, and later retries memoize. I renamed it to match and added a test for the timeout guard.

  2. Run state: the detached catch no longer calls handleSendFailure. It shows the notice only (still silent on an operator abort) and leaves shell.session.run alone. The queued tests now set the shell busy and assert it stays busy.

  3. Early grant: filed as CL-10865 rather than fixed here. It isn't specific to queued turns. Every reactor approval saves its grant in resolveInteractiveAsk before the decision is delivered, and has since at least July, so changing when grants are saved belongs in its own change.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review · Request changes

A thrown approval surface now rejects the parked call, and the shell stays busy so a queued follow-up can still drain. One hole remains in that new catch.

Findings

  • src/session/approval-resume.ts:557 — on a resolveSuspended throw, registerParkedCancel(undefined) runs before timeoutResultFromHistory. If the delivery generation bumps during that history read, the bump's cancel callback is already cleared, so it does not deliver the drop notice. After the await, stillCurrent() is false, deliverDecision returns without sending "approval surface failed", and nothing is delivered. The parked call then sits until the reactor timeout. The in-flight interrupt path is fine, because the bump runs while cancel is still registered. Deliver exactly one rejection when the generation changes during that read.

The previous idle-shell finding is fixed: the queued-turn catch posts a notice and no longer calls handleSendFailure (src/tui/runner/submit.ts:412). A denial and a later reactor timeout do not both become tool results. bun test src/tui/runner/queued-approval-delivery.test.ts src/session/approval-resume.test.ts — 91 pass, 0 fail.

@TheGreatAxios TheGreatAxios left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review · Approve

A thrown approval surface rejects the parked call, the shell stays busy, and a generation change during that failure still drops the call once.

Findings

No findings.

The catch in settleSuspended (src/session/approval-resume.ts:548) matches the success path. After the history read, a timeout sets canReject false, and a stale generation calls dropParked() instead of sending nothing. A current throw still delivers approval surface failed once. A timed-out call is not rejected again. bun test src/session/approval-resume.test.ts src/tui/runner/queued-approval-delivery.test.ts — 94 pass, 0 fail.

@TheGreatAxios
TheGreatAxios merged commit 8baf6aa into main Oct 19, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants