fix: parked approval escalations that leave chats permanently stuck - #39
fix: parked approval escalations that leave chats permanently stuck#39shipdocs wants to merge 3 commits into
Conversation
Approval cards could go missing without a trace: EventBus.publish() awaits escalation_created listeners via asyncio.gather(return_exceptions=True), so any exception inside _mirror_escalation was silently swallowed — no log, no card in any channel, and no pending-escalation future, leaving the parked task with no way to be answered. _mirror_escalation now degrades to the activity channel and logs loudly instead of losing the card. As a safety net for cards that still fail to render, an unambiguous decision word typed in chat (now including "proceed") can resume a parked approval checkpoint directly, without requiring a card click. Also routes the checkpoint-resume progress note through the WorkItem-owned metadata path instead of writing task.metadata directly, which was tripping the metadata-ownership invariant checker on every resume of a work-item-linked task. Separately, the channel outbound-dispatch loop is the sole consumer for every channel; a single send that never returns used to stall delivery for all channels until restart. Outbound sends are now bounded by a configurable dispatch_timeout_seconds.
…on, team chat Sequences the fixes into phases: proactive orphan-checkpoint sweep, giving the Secretary a real delegate-to-team action, and a multi-party team chat built on the existing activity channel and AgentMessage fan-out.
LZH-YS1998
left a comment
There was a problem hiding this comment.
Thanks for investigating these hangs. I am requesting changes because the current PR cannot be merged safely into today's main.
Blocking findings:
-
The approval fix is based on an obsolete control plane and conflicts with the current durable-interaction design. This branch is 19
maincommits behind and GitHub reports it asCONFLICTING. A merge rehearsal has conflicts in the engine, approval parser, Office WS handler, composer, generated UI bundle, and integration tests. Currentmaindeliberately removed_pending_escalations,_handle_ui_escalation, and the other process-local approval paths; cards are rebuilt from durable typed checkpoints and decisions go throughinteraction_reply/submit_checkpoint_decision. Please rebase and preserve that architecture instead of restoring the legacy registry. -
The advertised missing-card text fallback still leaves the task parked.
_mirror_escalation()remembers the live escalation beforeinsert_message(). If that insert fails, a later plainproceedreaches_handle_session_send(), is matched asbackground_pending_escalation, creates another helper/card, and returns without resolving the Future or reaching_maybe_resume_checkpoint(). I reproduced this exact sequence: afterproceed, the pending Future remains unfinished. Please add an end-to-end regression test that fails the original card persistence/broadcast and proves the same durable checkpoint can still be answered. -
Free chat must not implicitly authorize a tool. Mapping words such as
ok,continue, orproceedtoapprove_oncewithout an explicit checkpoint ID can approve the latest pending permission rather than the intended one, especially after restart or with multiple interactions. Currentmainintentionally requires an exact, typed checkpoint decision and has regression tests ensuring ordinary session text does not consume a permission card. Please remove this fallback and use the durable interaction endpoint. -
The copied approval card is not durable/idempotent. The helper card has no deterministic
message_idand omitsapproval_contextand source/ownership metadata. Repeated messages can create duplicate cards; after a restart, clicking such a copy cannot use the deferred-resolution path and may be marked stale. Any projection should be rebuilt from the durable checkpoint with a stable ID, as currentmaindoes. -
Enabling the composer does not implement the queue claimed by the tooltip. The UI now permits unbounded mid-turn sends, but the backend creates background coroutines; task-mode dispatch/classification occurs before the per-task execution lock. This does not guarantee end-to-end FIFO ordering, has no backpressure/queue position, and queued messages can be cancelled after already being persisted. Please remove this from the hang fix or implement/test an explicit per-session queue covering classification, execution, stop/cancel, reconnect, and approval waits.
-
The outbound timeout trades a global hang for silent message loss. Once the shared consumer dequeues a message, timeout drops it permanently with only a log entry. A dropped owner prompt/reply can itself leave a workflow waiting forever.
dispatch_timeout_secondsalso accepts zero/negative values, which makes every outbound delivery time out, andasyncio.wait_foris not a hard bound if a provider suppresses cancellation. This needs channel isolation plus a bounded retry/dead-letter/visible-failure policy and positive config validation, with tests for delivery outcome rather than merely “send was attempted.” -
The PR contains unrelated changes that need separate review. The CLI NDJSON coalescer, project-activation org reload,
sedclassifier extension, and a 254-line future roadmap are unrelated to approval hangs. The coalescer can reorder interleaved delta streams (thinking A,assistant B,thinking Cbecomesthinking AC,assistant B) and has no time/size flush, so--stream-jsonmay stop being observably streaming during long generations. The roadmap also references a non-existentdocs/handover-approval-hang-fix.md. Please split these changes into focused PRs.
Validation performed:
- The PR's 361 directly affected backend tests pass, but none covers the failing missing-card/live-registry sequence above or the composer queue contract.
- Current
main's focused durable-approval/interaction suites pass (36 tests), including card reconstruction without an event/chat projection and explicit tests that legacy process-local approval paths stay removed. - Frontend typecheck, contract tests, and production build pass; there is no composer queue behavior test.
- This PR currently has no CI/status checks reported by GitHub.
Parts of the investigation are still useful, but they should be carried forward separately: the regex-address sed -n classifier looks reasonable, the WorkItem-owned progress update is directionally correct, and the shared outbound consumer really does need isolation. They should not justify merging the current combined branch.
Summary
Chats that hit a tool-approval escalation could get permanently stuck with no way to unblock them — restarting the engine just re-parks on the same escalation forever. Two commits, addressing the same failure class from different angles:
1.
fix(ui): keep approval escalations actionablesed -nexpressions that use regex addresses, avoiding unnecessary approval escalation.2.
fix: recover parked approval escalations and outbound dispatch hangsRoot-caused via a live
.opc/ui_state.dbinspection of a stuck run: for the incident under investigation, zerohuman_escalationcard messages existed anywhere (session or activity channel) even though the task had correctly parked.EventBus.publish()awaits everyescalation_createdlistener throughasyncio.gather(..., return_exceptions=True). Any exception inside_mirror_escalationwas silently swallowed — no log, no card in any channel, and critically no pending-escalationfutureever got registered, so even a later correct reply had nothing live to resolve._mirror_escalationnow catches failures at each resolution step, logs them, and degrades to the project activity channel instead of losing the card outright.task.metadata, butprogress_logis WorkItem-owned metadata — this tripped the metadata-ownership invariant checker (metadata_ownership_conflict/violationwarnings) on every resume of a work-item-linked task. Now routed through the WorkItem-owned path (append_work_item_progress) when the task is linked to a work item.dispatch_timeout_seconds(default 30s).User impact
Read-only inspection commands no longer request approval unnecessarily, users can send a recovery or follow-up message while work is active, pending approvals remain actionable from the chat where the user is working, a card failing to render can no longer strand a task forever, and one hung channel send can no longer stall message delivery for every other channel.
Test plan
tests/test_shell_safety.py,tests/test_ws_handler_escalations.py,tests/test_approval_escalation_reply.py,tests/test_approval_engine.py,tests/test_chat_store_progress_folding.py,tests/test_metadata_ownership.py— all greentests/test_attempt_ledger.py,tests/test_checkpoint_answer_live_dispatcher.py,tests/test_company_recruiter.py,tests/test_company_runtime_suspend_resume.py,tests/test_request_user_input_collab.py,tests/test_run_failure_settlement.py,tests/test_runtime_config_enforcement.py,tests/test_runtime_v2_migration.py,tests/test_self_evolution_pipeline.py,tests/test_stale_checkpoint_selfheal.py— 199 passedtests/test_session_integration.py(192 passed) — new regression: escalation card survives a routing-resolution exceptiontest_stale_checkpoint_selfheal.py), "proceed"/"go ahead"/"continue" normalize to an approval decision (test_approval_escalation_reply.py)npm run test:contracts(9 passed),npm run typecheck,npm run buildwith Node.js 22