fix(dashboard): persist a slot create inside the slots suspension

[chenmingwei23]

1. What is the problem?

api_chat_slot_create wraps its whole set-up in state.suspend_slots_push(). That context manager's __exit__ flushes the owed push, and on the coalescing window's leading edge push_slots_update broadcasts synchronously. An exception raised there escapes __exit__ and unwinds the remainder of the handler — which, before this change, included the await save_slot_off_loop(..., force=True) that persists the create's folder filing and pinned title, plus schedule_eager_spawn and the 200.

That write is not a duplicate of anything. For a message-less or freshly re-filed slot, _save_slot_to_history's forced merge is the only durable record of folder_id / a pinned title — its own comment names "the recreate PATCH (folder or pinned title)" among the routes that "persist ONLY through this save, so a folder-only merge would silently drop their acknowledged writes on restart".

Reachability, stated plainly: this is latent, not live. A json.dumps TypeError cannot originate in caller data — a value parsed from a JSON body is JSON-serializable by construction — so it needs a server-constructed object reaching slot state. In #6522 that was a test fixture, and #6522's actual root cause (bare StopIteration from resolve_agent_bindings on an empty config.agents) already landed as #6517 / e48ea4266. No production path is demonstrated here, and this PR does not claim one.

2. Why this issue matters to the user

The two timing branches disagree about what the user is told, and the disagreement falls the wrong way.

• Leading edge (elapsed >= _SLOTS_BROADCAST_INTERVAL_S, i.e. 0.2 s since the last slots push): the flush broadcasts inline, the exception reaches aiohttp, the caller gets a 500 and the durable write never runs.
• Inside the window: the flush is deferred through loop.call_later(_trailing_slots_flush), the handler returns 200, and the same exception surfaces later as a log line through crash_guard._asyncio_exception_handler.

So a busy dashboard gets 200 plus a log, and an idle one — a person doing a single deliberate new-chat, with no other slot activity in the preceding 200 ms — gets the 500. The failure lands on the least-loaded, most deliberate case, which is the worst possible distribution for a phantom error: the create did commit in memory, so the user is told their action failed while the session exists.

And the lost write is the half nothing repairs. A dropped slots frame is cosmetic — four independent readers reconcile it (mount fetchSlots(), reconnect fetchSlots() in useWebSocket.ts, the WS-connect snapshot built from serialize_slots, and the 5 s poll in WorldsPopout). Four independent repair paths is what makes the frame cosmetic; one would not have been enough. None of them repairs a write that never happened: memory holds the new folder, disk holds the old one, and the divergence surfaces on the next restart.

3. How our fix solves it

Move the durable write inside the suspension, ahead of the broadcast. Nothing else.

This is an ordering change with an in-repo precedent, not a new design. session_control.py:1131-1139 — the other path that mints a slot through this same context manager — already does exactly this, with the reason written down: "The whole allocation-to-persist span runs under suspend_slots_push … It also covers the persist and its failure retraction, so a slot whose birth write fails is never broadcast at all." Two create paths, one context manager, and only this one had its persist outside. The fix makes the broken branch agree with the branch that already ships.

Chain from symptom to cause:

• Symptom: POST /api/chat/slots answers 500 and the folder/title the user just chose is absent after a restart.
• Proximate cause: the exception escapes __exit__, so every statement after the with block is skipped.
• Root cause: the durable write was sequenced after a best-effort UI broadcast, so a failure in the optimisation could veto the commit.
• Fix: sequence the write before the broadcast, so the broadcast can only ever cost the frame it owns.

Two deliberate non-changes, both scoped out on purpose:

• The 500 stays. Whether an already-committed create should answer 500 because a best-effort broadcast failed is a failure-semantics decision, and it was declined on this pass with reasons — the broadcast-content half of api_chat_slot_create: broadcast failure during suspend_slots_push unwind 500s an already-successful create #6532 is not addressed here, and folding it in would resurrect it. Closes #6532 therefore covers the ordering defect that is real; the declined half is recorded in the issue, not silently absorbed by this merge.
• No slot retraction, unlike the twin. There the write is the newborn's only record, so an unpersisted slot would vanish on restart and retracting is the lesser evil. Here the slot already has a metadata line and best_effort (the default) logs the failure and marks the slot dirty so the periodic flush retries it — the retry the metadata mutation routes already rely on.

_SLOTS_BROADCAST_INTERVAL_S is untouched, the coalescing is untouched, and what the broadcast contains is untouched.

The comment being replaced argued the write belonged outside because the process-wide suspension would make other clients' slot updates wait behind one session's history lock. That cost is real and the new comment keeps it on the record — it is accepted for the same reason the twin accepts it (which awaits a cross-process metadata write inside its own suspension), and because this span was never await-free anyway: it already suspends on the workspace-conflict probe a few lines above. Coalescing defers other pushes; it does not block their callers.

4. What tests we did

Two tests in test/test_chat_slot_create_folder.py, a file whose docstring already says "The fix is ordering, so the tests assert ordering".

They pin the ordering, not the exception: _broadcast is stubbed to raise on a slots frame, so any future raise out of the flush is covered rather than just today's TypeError. Each waits out _SLOTS_BROADCAST_INTERVAL_S first — without that the flush lands on the deferred trailing timer, the handler answers 200, and the test would prove nothing.

• test_raising_exit_broadcast_cannot_skip_the_folder_write
• test_raising_exit_broadcast_cannot_skip_the_pinned_title_write

Both assert the on-disk metadata line via conversation_log._read_metadata, and both assert the response is still 500 — so the tests also pin that this PR does not swallow the error.

Mutation-verified, with the mutation proven to apply. The handler was reverted with git checkout origin/main -- src/kiro_crew/dashboard/chat_handlers.py, and the revert was confirmed by git diff --stat origin/main on that path returning zero lines before the run. Against that pre-fix tree both tests fail on the disk assertion — assert None == 'f-design' and assert None == 'Pinned' — while the status == 500 assertion passes in both states, which is the evidence that the change moves durability and not the response. The fix patch was then re-applied (git apply --check clean) and both pass.

Suites run (targeted, never the full suite):

• test/test_chat_slot_create_folder.py — 20 passed
• test/test_chat_slot_create_folder.py + test/test_forced_save_history_key_pin.py + test/test_slots_broadcast_coalesce.py — 38 passed
• test/test_open_slots_persistence.py -k "suspend or push_slots" — 4 passed
• test/test_dashboard_chat.py -k "slot_create or create_slot or folder" — 76 passed

Gates on the touched files: black, isort, flake8 clean. mypy reports 2 pre-existing errors in src/kiro_crew/transcribe.py, reproduced identically on an unmodified main checkout — baseline, not from this change.

5. Any other suggestions on the work

• The 500 is still there and still undecided. The declined half needs an owner ruling: catch and log at the flush (making both timing branches converge on what the trailing branch already does), or keep fail-loud. Until then the caller-visible outcome of a broadcast failure remains a function of a 0.2 s timer, which no client can be written against.
• The exit flush can mask the exception that caused it. test_suspend_slots_push_unwinds_and_flushes_on_exception correctly pins that the owed push fires even when the body raises — but it stubs _broadcast, so a failing flush during unwind is unpinned. When both raise, @contextmanager propagates the flush's exception and the body's becomes __context__. That is why api_chat_slot_create 500s (bare StopIteration) when config.agents is empty — main CI red since #6465 #6522 read as a broadcast bug: the 500's top line named json.dumps while the real cause was chained underneath. A diagnostic that names the offending slot key, field and type before re-raising would be semantics-neutral and cheap.
• The other publish-then-await birth sites (18 across 8 files, tracked as Session control: cross-session message delivery needs resolved-binding authorization #4333) are the same class one layer out; this PR does not widen to them.
• Adjacent prior art, checked and found not to apply: PR #6526 names the same symptom (a 500 out of slot create) but approached it through the test's mock config. It was closed by its own author 38 minutes in as "duplicate of test: pin real config values in slot-create inheritance test (#6522) #6533 which applies the same fix with full local verification" — a process closure, not a merits ruling, and all three review lanes had passed it. test: pin real config values in slot-create inheritance test (#6522) #6533 then also closed unmerged, and fix: repair three cross-merge breakages redding main's CI #6517 is what landed. So the area was never ruled not-a-defect, the handler unwind was never touched by any of them, and there is no precedent here for or against this change.

Pattern harvest

Rule candidate: an in-repo sibling already doing the thing correctly is the strongest available authority for a fix's shape — prefer copying a live twin's ordering over reasoning from first principles, and cite the twin by file and line. Here the twin is session_control.py:1131-1139, and those numbers were re-resolved at branch point rather than carried over from where the review had named them, because main had moved in between: a citation that was true when written and false when used is worse than no citation, since it still looks verified.

• When one context manager is used by two paths, the ordering difference between them is the bug report. The evidence that settled this was not the traceback — it was session_control.py doing the same thing correctly, with its reason in a comment. An in-repo sibling that already does it right is the strongest authority a fix's shape can have: it is not one reviewer's judgment against another's, it is the repo's own decision applied consistently.
• "A dropped update is cosmetic" is a claim about the number of independent repair paths, and it should be counted. Here there are four, which is why the frame is genuinely cosmetic. The same sentence with one repair path would have been a correctness bug.
• Separate the messenger from the fault. The failing json.dumps is also what every read path goes through (GET /api/chat/slots, the WS snapshot), so had the poison been real, nothing at the broadcast could have fixed it. Retrying or swallowing the broadcast would have addressed the messenger; the divergence lived at the unwind boundary.
• A negative needs its positive run first. "No production path produces this" was checked against a live gateway journal (2026-09-01T22:54Z → 2026-09-04T16:20Z, 46,794 lines): zero frames through _do_slots_broadcast. That is only worth stating because the same grep does surface real traceback frames from dashboard/state.py — a probe that cannot produce a positive is indistinguishable from an absent signal.
• A mutation that is not proven to apply proves nothing. The revert here was verified by git diff --stat origin/main returning empty on the path before the red run, not by assuming the checkout worked.

Closes #6532

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 1c3bf5ab0cc911509c4cd05ae2410e8461896c2a and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 1c3bf5a

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 1c3bf5ab0cc911509c4cd05ae2410e8461896c2a: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 1c3bf5ab0cc911509c4cd05ae2410e8461896c2a — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real ordering defect (durability sequenced after a best-effort broadcast) fixed at its root, matching the in-repo twin's already-shipped pattern, with the accepted cost documented and the ordering pinned by tests.

[DESIGN-REVIEWED] 1c3bf5a

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 1c3bf5ab0cc911509c4cd05ae2410e8461896c2a — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verifications done. The twin ordering at session_control.py:1131-1139 exists as claimed (persist inside its suspension with retraction), suspend_slots_push's finally re-calls push_slots_update() which broadcasts synchronously on the leading edge (state.py:7578-7585, 7617-7619), and I counted the suspension sites: three total (chat_handlers.py:2523, session_control.py:1139, server.py:4039). The server.py restore site has no acknowledged durable write after its block, so no unfixed sibling remains. The diff is a pure move (the save_slot_off_loop call relocated inside the with, arguments unchanged) plus two ordering tests — no new surface, key, flag, or exported symbol.

First-Principles-Verdict: PASS

A durable write no longer sits behind a best-effort broadcast that can veto it; the fix is a pure move that makes both create paths obey one invariant.

What this change ships

Intent: make a slot create's folder/title survive a restart even when the exit broadcast raises — a FIX.

1. Folder/title chosen at create now reaches disk before the broadcast can fail — justified (derived from api_chat_slot_create: broadcast failure during suspend_slots_push unwind 500s an already-successful create #6532; twin ordering at session_control.py:1131-1139 already ships it).
2. The coalesced slots frame now waits behind the off-loop metadata write — declared cost, quoted from the replaced comment.
3. Broadcast failure still answers 500 — declared non-change, pinned by both tests.
4. Two tests pinning write-before-broadcast ordering, not the exception type — justified.

Sibling count: 3 suspend_slots_push sites grepped; the twin already persists inside, the server.py restore has no post-block durable write — 0 unfixed siblings, so this is the general fix, not a point patch. No new public surface, so consumer count is moot. Reachability is honestly framed as latent, and the provenance is still derived: #6522 evidenced the failing shape and the write it dropped is the only durable record of a recreate's filing (_save_slot_to_history's merge comment confirms). Nothing rides along.

[FIRST-PRINCIPLES-REVIEWED] 1c3bf5a

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 1c3bf5ab0cc911509c4cd05ae2410e8461896c2a — this comment is updated in place on each push.

Review details

The single candidate is a documented, deliberate latency tradeoff. Verified against the code: the suspend_slots_push span opens at line 2523 and already awaits an off-loop voice_runtime_workspace_conflict probe (2705) and a cross-process folder read under tags_write_lock (2679-2680) before the moved write. The span was never await-free, so the added save_slot_off_loop is an incremental cost consciously accepted to close a real correctness hole (a broadcast raise at __exit__ unwinding past the only durable record of a recreate's folder/pinned-title). Frames coalesce, none drop, no caller blocks. The proposed remedy (narrow coalescing to the mutating slot, or split the suspension) requires new machinery and reverting the hunk abandons the fix. It does not clear the 80+ bar as a defect — it is a design decision with a stated tradeoff.

No findings.

[OPUS-REVIEWED] 1c3bf5a

Verdict parsed from the review's SHA-scoped output markers for commit 1c3bf5ab0cc911509c4cd05ae2410e8461896c2a.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 1c3bf5ab0cc911509c4cd05ae2410e8461896c2a: <one-sentence reason>

[dwu96]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: pure ordering fix — moves the existing force=True durable slot save inside the suspend_slots_push span so a raising exit broadcast can no longer unwind past an already-acknowledged folder/title write; call arguments including the expected_history_key rebind pin are moved verbatim, plus two red-first ordering tests.