fix(server): withdraw a stopped turn's pending approvals#22023

Approvability

Verdict: Approved at 71119ed

Macroscope's review found this PR approvable — This is a focused server fix that withdraws pending approvals only when their provider turn is interrupted or cancelled, while preserving completed and failed turn behavior. SQL and in-memory projections plus terminal-status coverage are included, with no schema, configuration, or deployment changes.

^{You can add or adjust custom eligibility rules. Learn more.}

Review in Change Stack →

No actionable comments were generated in the recent review.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

External image Recent review info

External image Run configuration

• Source mode: Auto
• Repository: pingdotgg/t3code
• Branch: main
• File: .coderabbit.config.ts
• Commit: 26c3625
• Layers: Repository TS config
• Review profile: CHILL
• Plan: Advanced
• Run ID: 9101d5df-5c3a-4f25-b630-ae600fe5be07

External image Commits

Reviewing files that changed from the base of the PR and between 7557986e994503e0daae3a3b4e974a41fb01b4ec and f8d2a8dd9ef671412b4a2c4e05901b1aacaadad6.

External image Files selected for processing: 2

• apps/server/src/mcp/threadAccess.ts
• apps/server/src/mcp/toolkits/core.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Written by Claude Opus 5.5 on behalf of @ScottN-PV.

Real-run data for "What I couldn't prove", in case it helps. We did not run this patch.

On main b34501a, a Codex parent used delegate_task to start an approval-required child, which blocked on a shell-command approval. The parent's task_cancel returned cancel_requested, then interrupted. In both of two runs, the approval went from pending to cancelled/not_resumable with the reason "The run was interrupted." That reason comes from the request loop in Orchestrator.settleInterruptedRun (#15442). The reporter's v0.0.46-nightly.20261008.2819 already contains that loop, so it was present when their Codex approval stayed pending.

Not covered: Claude tasks, native subagents, Stop from the UI, timing variations.

Thanks @ScottN-PV, this is useful. I agree: for a Codex parent's task_cancel, settleInterruptedRun already withdraws the approval, so that isn't the gap.

This PR covers a different path. settleInterruptedRun returns early unless the run and its attempt are still running. When a turn ends as interrupted or cancelled through ProviderEventIngestor, main only withdraws user-input requests there, so approvals stay pending. The regression test shows this on main.

I haven't confirmed in code which real scenario reaches that path, so I'm not claiming it explains the reporter's case.

Live data for the "What I couldn't prove" section. On main (7208335), a Codex task_cancel left the child's command approval pending.

Setup: a Supervised Codex parent used delegate_task to start a child, the child blocked on touch approved.txt, and the parent called task_cancel. The child run went to interrupted. The approval stayed pending, with a live Approve/Decline card, until I stopped the server.

Doesn't settleInterruptedRun already handle this, as @ScottN-PV saw? Sometimes. It only runs while the run is still running, and in my run Codex's turn/completed got there first. The provider log shows turn/interrupt, then turn/completed (interrupted), then serverRequest/resolved for the approval, within 19 ms.

A second run backs this up. With a turn-end withdrawal in the ingestor (close to this PR), the same cancel left the approval cancelled with its capability still live. That means the turn-end path withdrew it, not settleInterruptedRun, which writes not_resumable. In other words, a real cancel does reach the path this PR fixes.

Two smaller things from testing:

• Leaving completed turns out is right. I first tried withdrawing approvals on every turn end. A Claude background subagent asked for approval while the main turn ran, and when the main turn completed, its still-live callback was cancelled. The subagent hung with no card until Stop.
• The Codex handler leak you list is easy to close in the adapter. If the adapter settles the pending approval's Deferred with cancel when the turn ends interrupted, Codex gets a reply and the handler fiber finishes. The true result from Deferred.succeed also guards the approve-while-interrupting race. I have this with a failing-first adapter test and can send it as a separate PR once this one lands.

Thanks for writing up the turn-ownership cases so clearly.

Thanks @SudheerNaraharisetty, this is really helpful. The provider-log ordering and your second run show that a real Codex cancel reaches this path. I've linked your comment in the "What I couldn't prove" section of the PR description. Thanks as well for confirming that completed turns should stay out.

@coderabbitai Good catch, fixed. The guard now rechecks approval requests too (still pending, same kind and turn, message-mode left out), and there's a test for an approval answered after the pending requests were read.

@fixfon Thanks for fixing this and adding the regression test. The recheck addresses the race where an approval is answered after the pending requests are read.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

On CodeRabbit's outside-diff note about the respond/cancel race: it's real, and it's the reverse of the case I covered. A late cancel can't overwrite an answer anymore, but a respond that read the request as pending before the interrupt can still commit "resolved" after the cancellation and queue a response effect for a turn that's gone. Approvals only hit this since this PR started cancelling them; questions have the same gap on main.

The fix I have in mind is small: commitCommand re-reads the request inside the transaction and rejects the respond if it's no longer pending or the kind or provider turn changed, and dispatchRuntimeRequestRespond maps that to the existing "is cancelled" error, with a latch test for the interleaving. @juliusmarminge since you're driving this branch now, want me to push that here, or keep it for a follow-up PR?

Keeping this race fix in this PR: the new approval cancellation path needs the response-side transaction check too. I have restarted a focused failing-test step after the previous task failed during the server restart; it will cover response reads pending → cancellation commits → response attempts commit. I will push the fix here after verification.

Fixed the reverse respond/cancel race in 04df7b638c. The response write transaction rechecks pending status, request kind and provider turn before committing events or effects. Stale responses return the existing cancelled-request error; receipt reservation rolls back. Both command and question regression cases failed before the fix and now pass, including exact error, absent receipt, cancelled item and empty outbox assertions. 279 targeted tests and server typecheck passed; both fresh reviewers found no code blockers. Existing client-proof gaps remain disclosed.

@coderabbitai resume

Fixed in f8d2a8dd9e: the dispatch wrapper preserves the tagged sink error as its cause. MCP exposes this specific safe cancellation message; other internal errors remain redacted. 47 focused tests, server typecheck, format and scoped lint passed (one existing lint warning). Real-client proof remains incomplete as disclosed.