Repository navigation
fix(tui): deliver approvals for parked calls in queue-drained turns - #1519
TheGreatAxios merged 14 commits into
Conversation
TheGreatAxios
left a comment
There was a problem hiding this comment.
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— whendetachApprovalis set andsendreturnssuspended,approvalResume.handleis started withvoid runWhileAgentBusy(...).catch(...)and the outer busy callback returns immediately. Thetrythen always returns{ status: "accepted" }at line 422, before that task finishes. The.catchonly logs the error name and does not callhandleSendFailure.createGenerationGatedHopsettles the drain from that hop result (src/tui/delivery-queue.ts:651), andsettleDrainedDeliverytreatsacceptedas done and does not restore the queued text (src/tui/runtime-bridge.ts:620).If the operator allows and delivery then fails (
ApprovalDeliveryTimeoutErrorfromsrc/tui/approval-delivery.ts:151, an overlay throw, orAgentClosedError), the reactor has not accepted the decision,handedOveris never set (src/session/approval-resume.ts:501), andsuspendedApprovalRecovery.clear()at line 404 does not run. A later retry will not resend:deliveredalready holds the correlation (approval-delivery.ts:142), and the nexttryResumeOncedrops the candidate asalready-attempted(approval-resume.ts:176). The synchronous path at line 415 still rejects intohandleSendFailureand 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 — loggedapproval for a queued turn failed to settleand still settled{ status: "accepted" }with nothing delivered. Return the detached failure asnot-delivered(or surface it the wayhandleSendFailuredoes) instead of accepting the drain, and cover the timeout or throw.
|
Fixed in 1da2213 (test) and 9161a31 (fix). I took your second option: a detached failure now goes through I kept the drain The new cases cover both the throw and the acceptance timeout ( |
TheGreatAxios
left a comment
There was a problem hiding this comment.
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.catchcallshandleSendFailureand 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 whenresolveSuspendedthrows:deliverDecisionnever runs, nothing callstryResumeOnce, 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 printapproval overlay failed. -
src/tui/runner/submit.ts:415—handleSendFailuresetsshell.session.runtoidle(submit.ts:356) without draining the boundary. A follow-up already queued while the turn was busy keeps its badge. A laterreactor.donehitssettleRunToIdle(src/tui/runtime-bridge.ts:1287), which returns immediately becauserunis no longerbusy, 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 whosepattern !== null,mintGrantpersists the session or project allow beforecreateApprovalDelivererconfirms 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.
…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
|
Addressed in 014c058 (tests), b1ceab2 and 9a3d500, on top of the branch after #1520 and #1521 were merged in.
|
TheGreatAxios
left a comment
There was a problem hiding this comment.
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 aresolveSuspendedthrow,registerParkedCancel(undefined)runs beforetimeoutResultFromHistory. 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,deliverDecisionreturns 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
left a comment
There was a problem hiding this comment.
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.
Summary
ask_directorwake, mailbox mail, or a prompt typed while busy) returns oncesend()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.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 gotapproval timed outan hour later, the queue stayed wedged, later messages never reached the conversation, and nothing was logged. This hitapprove_worker_callwhenever a worker asked its director for a denied call.Verification
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)ask_directorwake, the operator's allow-once onapprove_worker_callwas 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