Repository navigation
fix(chat): hold a restored queue instead of showing it as work in flight - #23971
Pearcekieser wants to merge 1 commit into
Conversation
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks. The hold restores the rule session.md already pins: restored cards are "handed back as queue CARDS, not dispatched" and "must never be auto-resent". One duplicated line of skill prose is left. Notes
What this change shipsInventory (10 items) — 9 justifiedIntent: after a restart, a chat with messages queued behind a cut turn should look idle, offer Resume, and never send those messages unless the person does. FIX. The change has 11 observable differences; this list keeps the 10 a person is most likely to notice and folds the recovery-gate change into item 1.
[FIRST-PRINCIPLES-REVIEWED] b245065 Description read[DESCRIPTION-READ] 575fa09478de10c3cb455d3bbc36f150d9397c21494b9d65027d58d12ab71c23 That sha256 names the PR description this verdict read. Recompute it with |
UX Review (Opus 5.5, fork) — ✅ PASS (a BLOCK with no Trigger and no Outcome does not block)UX-level review of UX-Verdict: PASS cannot evaluate: missing the crewmate chat's new Held cards and the rowWritten held card with no Edit. The two screenshots only show the main chat pane before and after. Evidence gaps
Notes
[UX-REVIEWED] b245065 Description read[DESCRIPTION-READ] cf647dcf6fecaf9b6d4ce07bbf735aff2aea7de2d8cb6fbab2821278ca2f668b That sha256 names everything this verdict read: the PR description, plus the 5 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the fix is at the root (restored entries are held, and the readers that ask whether work is coming count only ready entries), and screenshots of this revision are attached. Notes
[DESIGN-REVIEWED] b245065 Description read[DESCRIPTION-READ] d7b2a8d9b79ee77d9cc120cb2b6b574d69b656b0af649b203f79df593b420d29 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
GPT 6.1 Review (fork) — ✅ no blocking findings (all downgraded on adjudication)Reviewed Opus 5.5 adjudication downgraded every blocking finding. Downgraded: the remedy each one needs is disproportionate to the harm it prevents. Adjudication downgraded all 2 blocking finding(s): the remedy each one requires is disproportionate to the harm it prevents. Review detailsBLOCKING -- src/kiro_crew/dashboard/chat_api/turn_stop.py:953 -- Preserve the selected card when parking Send now BLOCKING -- src/kiro_crew/dashboard/chat_turn/prompt_assembly.py:104 -- Clear discarded identity before sending a resurrected card Adjudication (Opus 5.5) — is blocking on each finding proportionate?F1 and F2 are both downgraded: neither is worth blocking the merge. F1 (LOW).
F2 (LOW).
|
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
|
self-added: yes
|
|
self-added: yes
|
|
|
b90f6dc to
f674220
Compare
|
Pushed f674220. It carries the two round-1 fixes (dispositions above) and one locale fix for the reds on b90f6dc:
|
|
self-added: yes
|
|
self-added: yes
|
|
self-added: yes
|
|
|
|
f674220 to
f9c1219
Compare
Kept, with the reason. The 409 The id list exists because Discard keeps the row: GPT's round-6 finding (span 9dd41fe0962a) showed a kept row becomes what a later Resume continues, including after another restart reloads it from disk, where the live queue no longer names it. The cost is one metadata read per Resume ( |
|
note (non-blocking): Backend Tests (3.12, 2) on 951347c failed only |
951347c to
2ddc2a9
Compare
Upheld, fixed in the next push. Tests in |
Upheld, fixed in the next push. Test: |
Audit (turn_stop.py:972): upheld, fixed in the next push. The parked branch of the interrupt route now emits Priority (turn_stop.py:953): not changed; the adjudication downgraded it (disproportionate-remedy). The sequence is Resume, then Send now, both while memory preparation is pending. The deferred drain runs them in the order the person pressed them: the recovery Resume put at the queue head first, then the released card. Nothing is lost or auto-sent, and the card does run. Moving the card ahead of a Resume the person also asked for would add a new ordering rule between parking and promotion. |
2ddc2a9 to
75caafb
Compare
Upheld, fixed in the next push. Test: |
Upheld, fixed in the next push. This span's second hit, so the fix closes the race class rather than this ordering. The held, row-written discard now runs in Test: |
Upheld, fixed in the next push. Test in |
50df9c1 to
b245065
Compare
Not changed; downgraded on adjudication again. Same answer as on 2ddc2a9: Resume, then Send now, both while memory preparation is pending, run in the order the person pressed them. Nothing is lost or auto-sent, and the card runs right after the recovery turn. The parked command is now audited ( |
Not changed in this PR; downgraded on adjudication. The trigger takes three faults in a row: a restart between a Discard's committed record and the next queue save, a Send now on the card that restart brought back, and a second interruption of that turn followed by Resume. The cost is that this one Resume continues the earlier request instead of the resent one; nothing runs twice and nothing is lost, since the row stays in the transcript. If it is worth closing, the smaller remedy is at restore rather than at Send now: a restored entry whose id is in |
A queue restored after a restart has no owner: nothing drains it on boot, by design. Every reader still treated a non-empty queue as work about to run. The sidebar spun "In progress", the board filed the chat under Working, the queue card's hourglass kept flipping, and Continue refused with slot_queue_pending, so the restart notice's "resume to continue" had no working control. A later send then ran first and the restored card ran on its own behind it. Restored entries are now held (is_held_queue_entry). The drain passes them by and starts one only when the person names it (Send now). Continue, the completion sound and session status count queue_ready_depth. The slot frame carries queue_held_depth and each held entry meta.held, so the sidebar, board, command center and queue card stop showing a held card as working. The turn marker's metadata merge also writes the post-dequeue queue in the same atomic write as the opener copy. Written apart, a SIGKILL between them restored the consumed entry twice: as the interrupted turn's row and as a queue card.
b245065 to
a14783c
Compare
|
chore (non-blocking): rebased on main, no behaviour change. a14783c is b245065 on main 455e8ec, a clean rebase with no conflicts. It picks up #24142, the fix for flake #23965 that failed Backend Tests shard 2 twice here. Backend shard 5 on b245065 failed |
Problem / Motivation
Goal: After a restart, a chat whose turn was cut while messages were queued shows no work in flight, offers Resume, and never sends the queued messages without the person.
A turn is running and a message is queued behind it. The app restarts. The chat comes back with "This turn was interrupted when the app restarted. Review the partial output above, then resume to continue." and the queued message as a card. Nothing runs (
running: false), yet:POST /continueanswers 409slot_queue_pendingon every press;Why it matters
Every chat that has a queued message when the app restarts (an update, a crash, a watchdog exit) is affected. The UI says work is in progress when nothing is, hides the one control the notice tells the person to use, and later sends words the person typed to a process that no longer exists, out of order. The double-restore case can run the same prompt twice.
Not a goal
open_slots.jsoncadence). Separate cause, listed under Manual verification.What changed (motivation → approach → change)
Cause: a queue restored from disk has no owner. Nothing drains it on boot, by design (session.md: restored entries are handed back as cards, not dispatched). But every reader still asked
queue_depthwhether work was coming, and the drain picked restored entries up after any later turn.Restored entries are now held (
is_held_queue_entry). The drain passes them by, and only the person's Send now on the card starts one. Send now marks the entry released, so the deferred drain after a stopped turn or memory preparation still runs it."Is work coming?" readers now count
queue_ready_depth: Continue's refusal, the completion sound, session status and the members-thread reset. The slot frame carriesqueue_held_depthand each held entrymeta.held(types/index.chat.ts). The store carriesheldthroughwire.tsandqueue.ts(including a card a first pane hydrate keeps), andselectContinuable(selectors.ts) looks past a held card. The sidebar, board, command center and card (website/src/components/QueueStack.tsx) stop showing a held card as working, and the card reads "Held" (new strings in everywebsite/src/i18n/locales/*.jsoncatalog). Held cards no longer count as user intervention for recovery gates. A Resume skips the written rows of held sends and of held sends the person discarded, so it continues the cut request. Both recovery paths apply it: the native-resume preamble and the cold-start replay (build_session_replay(skip_row=...)insrc/kiro_crew/context.py), which drops the rows from the disk transcript and the live window alike. Discard records the entry id in the transcript's session metadata (discarded_queued_sends, newest 64) before it removes the entry, one discard per slot at a time, with a compare-and-swap throughupdate_metadata_if; if the write fails the card stays held and the route answers 503. Agents see the split too: the session-control status and read payloads carryqueue_held_depth,session_statusandsession_read_messageinsrc/kiro_crew/mcp_dashboard_session_runs.pyreport held cards apart from queued ones (_queue_phrase, forwarded on thesrc/kiro_crew/mcp_dashboard.pyfacade), and the goal-conductor skill says held cards do not run on their own.The turn marker's metadata merge now writes the post-dequeue queue atomically with the opener copy. The two land together or neither does: with no retained copy, when another queue writer committed first, or when the turn merged several crewmate sends (the copy holds only the last), the queue key is left to the paired row-and-queue save. Send now parked on memory preparation is answered as accepted and audited as
dashboard_interruptwith outcomeparked, and the client (website/src/hooks/useQueuedMessageActions.ts) releases that latch if a restart hands the card back held. The turn-completion chime (website/src/hooks/websocket/turnCompletion.ts) counts only ready entries too.meta.heldoverrides every content classifier, so a held prompt that happens to read like a system delivery is still an actionable card on every surface. A crewmate chat (ChatPanewithcrewmate) draws held entries as Held cards with Send now instead of sent bubbles, with no typing bubble for them. A held entry whose row is already written (meta.rowWritten) has no Edit, and Cancel discards it: the entry goes, the row stays, nothing returns to the composer.Backwards compatibility
Compatible: new fields are additive (
queue_held_depth,meta.held), and a client that ignores them behaves as before. A gateway without them reads as zero held.Behaviour change: restored cards no longer send themselves after the next turn; they wait for Send now, an edit or a delete. This is the documented session.md contract, now enforced. Session status reports
idleinstead ofqueuedfor a slot whose only queued entries are held.Removes nothing: no field, route or capability is removed. The restored-entry drain path is still reachable, through the card's Send now.
Tests
test/test_restart_held_queue.py: held vs ready counts and projection; restore holds every entry; drain skips held entries, keeps their order, runs a named or released card, never falls through to a held head after a purge, does not park held cards on memory preparation, is not reached by the sub-agent hold's system scan (a restored steer), and does not flush a deferred note into a cron turn behind a held head; Continue runs ahead of held cards, refuses a duplicate press, still refuses on a ready entry; held cards are not user intervention; dequeue and turn marker land in one write, the opener copy and the shortened queue land together or not at all (no copy retained, or a newer committed queue), and a Send now parked on memory preparation is answered as accepted; repeated restarts keep one notice and the same cards.test/test_chat_mode_security_send.py: the four restored-entry security tests now reach restored text through Send now, the only path that runs it.website/src/test/heldQueueAfterRestart.test.tsx: wire and hydrate carryheld(including a crewmate row kept in place); Resume is offered past a held card and withheld behind a ready one; lanes and command center treat a held queue as idle; the card shows "Held" with no hourglass.test/test_chat_slot_facade_contract.pypins the newqueue_held_depthkey.ChatPane.queueActions.test.tsx,useQueuedMessageActions.test.tsx,crewmateBubbles.test.ts: a crewmate's held entry is a Held card with Cancel and no typing bubble; a parked Send now stays latched until a restart re-hydrates the card held.TestResumeNeverContinuesAHeldSend: the interrupted-turn preamble skips the row of a held send and of a discarded one; the cold-start replay skips both rows whether they come from disk or the live window. The cancel test intest/test_dashboard_chat_handlers_coverage.pychecks Discard records the id in session metadata.TestAgentsAreToldHeldIsNotQueued: the agent-facing session tools report held cards apart, and an older gateway's payload still reads as queued.Proof: not provable -- other: the new tests import the held-queue API, so prove.py's revert errors at collection. Run by hand on main c24c7bc with that API shimmed, 14 of the 20 backend tests fail and the other 6 pass, pinning behaviour main already has.
Manual verification
Reproduced on a disposable gateway (fake ACP backend, loopback, throwaway home), never the live install, with a driver script that restarts at each boundary:
running:false, queue_depth:1, sidebar "In progress", hourglass, no Resume,/continue409Not fixed, separate causes: a tab created under 5 s before SIGKILL is not reopened (its transcript, marker and queue are on disk); a force Stop's queue clear rides the 5 s flush.
Screenshots / video
Before (main): "In progress" spinner, animated hourglass, no Resume.
After: "Turn interrupted · Resume", Held card, Resume on the notice and the composer.
Related Issues
Fixes #23970
Closes #16780
Supersedes #20029. Its
meta.restoredprojection and Resume gate are covered here, and its open review blocker (restored cards drain on their own after the recovery turn) is answered by keeping them held.Sibling sweep
Swept:
git grep -nE "queue_depth|len\(slot\._queue\)|if slot\._queue|not slot\._queue" -- src/andgit grep -n "queue_depth" -- website/srcSibling: src/kiro_crew/dashboard/chat_api/continue_turn.py -- changed here
Sibling: src/kiro_crew/dashboard/chat_utils_completion.py -- changed here
Sibling: src/kiro_crew/dashboard/handlers/members_thread_turns.py -- changed here
Sibling: src/kiro_crew/dashboard/session_control_parts/session_status.py -- changed here
Sibling: src/kiro_crew/mcp_dashboard_session_runs.py -- changed here (agent-facing queued count)
Sibling: src/kiro_crew/builtin_skills/goal-conductor/SKILL.md -- changed here
Sibling: website/src/pages/chat/sessionLane.ts -- changed here
Sibling: website/src/pages/chat/command-center/model.ts -- changed here (node state); the options-suppression read keeps raw
queue_depth, since a held card may still be the answerSibling: website/src/lib/closeBusyGate.ts -- not affected -- closing still discards unsent words, so a held card keeps the confirm
Sibling: src/kiro_crew/dashboard/chat_turn/queue_cycle.py -- not affected -- its queue reads feed
_start_next_queued_turn, which now skips held entriesPattern harvest
Rule candidate: review-prompt
Pattern: a predicate that asks "will this run on its own?" reads raw queue/ownership state that a restart can leave without a live owner
Checklist
docs/system-specs/modules/session.md)