feat(chat): POST /api/chat/slots/{slot}/note -- visible line plus silent next-turn context

[rnoack1]

Problem / Motivation

A background actor — a cron, an app backend, a watcher — sometimes needs to leave a short factual breadcrumb in a chat: "board reconciled, 3 sessions moved to Done". Neither existing channel can do that on its own, and the failure in each direction is silent:

• slot.append() writes a transcript row the user sees, but a live provider holds its own in-memory conversation state and a normal send forwards only the new user message. So the model never sees the row. If the user later asks "what happened while I was away?", the agent has no idea.
• POST /api/chat/slots/{slot}/context puts content where the model will see it (drained and prepended to the next user message), but by design it appends nothing visible. The user has no record that anything happened.

Sending a real message instead is worse: it fires an LLM turn for a fact nobody asked a question about.

There is also a related gap independent of the new endpoint. build_session_replay and compress_thread_history filtered on a hardcoded {"user", "assistant"}, so every inject-role row — cron results and stalled-turn recoveries that already exist today, not just notes — was dropped at a session boundary. A breadcrumb that survives one turn but not the session it was written in is only half useful.

Why it matters

Today a background actor has to choose which half of the audience to fail, and it fails silently either way: write the visible row and the model never learns it happened, or write the context entry and the user has no record at all. The user-visible consequence is asking "what happened while I was away?" and getting a confident answer from an agent that genuinely cannot see the work. The replay gap is the wider half and it is already live: every inject row — cron results and stalled-turn recoveries that exist today, not just new notes — was dropped at a session boundary, so breadcrumbs that survive one turn vanish from the session they were written in.

What changed (motivation → approach → change)

POST /api/chat/slots/{slot}/note does both writes against one slot: a visible role="inject" transcript line (cls="reconcile-note"), and a _pending_context entry on the same channel /context already uses. Both writes always happen: a context-only write is POST /context, which already exists, and no caller wanted a visible-only one.

Four decisions worth calling out:

• RECALL_ROLES. The three role filters now share one frozenset({"user", "assistant", "inject"}) in context.py instead of three copies of a literal, so replay, compression, and the context-builder fallback cannot drift apart. system stays excluded — those are internal thinking/done markers, not visible content.
• The visible half is redacted; the context half is not. role="inject" is rendered to the user and written to the on-disk JSONL, and that sink is not otherwise redacted, so a caller-supplied secret would land in history verbatim. redact_exfiltration_urls then redact_credentials run before the append (URLs first — that pass collapses the whole URL). The context half stays raw, which is the trusted-caller boundary /context already has; this PR does not widen or narrow it.
• The per-source cap protects the queue, not the transcript. At the cap, /note returns 200 with contextSkipped: true and still writes the visible line, rather than 429-ing and losing the audit record too. This matters for the default source: "note" bucket that every sourceless caller shares. /context, which has no visible half, still 429s on its own cap — unchanged.
• Validation is shared, and maxAge is checked unconditionally. drain_pending_context computes injected_at + max_age, so a non-numeric maxAge raises a TypeError on the user's next send — a failure far from the request that caused it. It is now a 400 at the boundary, even on a visible-only note where no entry is enqueued, so a malformed value is never silently ignored. bool is rejected (isinstance(True, int) is True but a boolean TTL is a bug), and so are NaN/Infinity, which slip past a <= 0 check and would make an entry never expire. source is capped at 64 chars with no control characters, so a crafted label cannot break out of the [Background context from "{source}"] frame; it is also trimmed, so a padded label and its bare form share one cap bucket instead of quietly becoming two.

Extracting _validate_content / _validate_source / _validate_max_age / _check_slot_app_ownership / _enqueue_pending_context means /context picks up the maxAge and source guards it never had, and the two entry points cannot drift.

Tests

• test_every_queue_drain_seam_flushes_before_starting_the_successor enumerates the successor-dispatch seams by walking the package AST instead of naming functions, so a new seam that forgets the flush fails. test_the_drain_seam_scan_can_actually_fail is its negative control: it asserts the scan reports a failure both when the flush is missing and when it sits BELOW the drain, so a scan that could never fail would itself fail.

TestNoteEndpoint — 53 tests: both writes; default/explicit/null maxAge; maxAge rejection for non-numeric, boolean, non-positive, NaN and ±Infinity; non-string and over-length content; source newline, control-char, and over-length rejection; empty and whitespace-only source defaulting to note; visible-content redaction; source-at-cap still writing the visible line with contextSkipped: true; an explicit-null maxAge meaning no expiry and agreeing with /context on that; expired entries not holding the per-source cap, with a live-entry control proving the cap still bites; a full queue shedding expired entries rather than evicting a live one; a reset dropping the note's queued copy while keeping a /context entry; app-token denial on an unowned slot plus the allow path on an owned one; 404; empty content; invalid JSON. /context gains two regression tests (its new maxAge guard, and source normalization sharing one cap bucket). test_history_race.py gains four for RECALL_ROLES: inject reaches replay, system still does not, and the filter reads the constant rather than a literal. It gains four more for the replay's per-row inject cap: an oversized row is clipped and keeps its head, a typical row passes through untouched, conversation rows are never clipped, and conversation history survives a 40,000-character inject row.

test_gateway_appkit_endpoints.py + test_history_race.py: 137 passed, 0 failed. Everything covering the four modules this touches — every test_*context*, test_*history*, test_*persist*, test_chat_* suite — 69 files: 2089 passed, 0 failed.

Negative control, because a passing suite is not evidence the tests can fail: replacing the redaction pass with visible_content = content and dropping inject from RECALL_ROLES takes test_gateway_appkit_endpoints.py + test_history_race.py from 137 passed to 6 failed, 131 passed. Both files were then restored and confirmed byte-identical by hash, and the run returns to 137 passed. The four cap tests carry their own controls in both directions: removing the cap takes the preservation test's slot from 120 surviving conversation turns to 49, applying the cap to every role instead of inject alone fails the conversation-row test, and lowering the cap to 100 fails the typical-breadcrumb test.

The ceiling test is bounded by an explicit asyncio.wait_for and both keep their in-flight sleep short, so a ceiling that fails to fire fails the test rather than blocking until the suite cap -- it still completes in ~1s, which is the ceiling firing and not the sleep elapsing. The stage-loop fix adds two tests on the cancellation and ceiling paths; the suites reaching _stage_loop, flush_deferred_notes or slot.task — 36 files — are 2973 passed, 5 skipped, 0 failed, and the no-seed integration and e2e suites are 114 passed, 27 skipped, 0 failed.

flake8, isort, and mypy are clean on all nine changed source files. mypy is also what forced the annotation widening in history.py: recent() hands its roles to _recent_via_tail, so passing a frozenset requires widening set[str] to AbstractSet[str] along the whole chain, not just at the two public entry points. Reverting the three internal helpers reproduces error: Argument 3 to "_recent_via_tail".

Error-code contract

Every error response this endpoint introduces carries a machine-readable code beside the prose, as test/test_error_code_contract.py requires: the dashboard renders res.error verbatim into a localized UI, so an un-coded English sentence is untranslatable by construction. Ids are reused from the vocabulary already in the tree wherever one covered the condition -- invalid_content, empty_content, content_too_long, non_finite_number, value_out_of_range, capacity_reached, invalid_json, slot_not_found -- and four are new where nothing fit: source_not_a_string, source_too_long, invalid_source, invalid_max_age.

One of those choices is deliberate rather than incidental. The app-ownership gate answers 404 precisely so a non-owning token cannot use the status to probe which slots exist, so it returns the same slot_not_found code as a genuine miss. Giving it a distinct code would put back into the body exactly what the status withholds.

error-code-baseline.json is regenerated with python test/test_error_code_contract.py --update, which the ratchet permits only after a count legitimately drops. dashboard/chat_handlers.py moves 74 -> 68: this change codes 16 responses, and 5 previously un-coded inline checks are gone because the shared validators now supersede them. No file's cap is raised in either direction. The regeneration also refreshes the informational _compliant counter (762 -> 790); most of that delta is compliant handlers that landed upstream since the baseline was last snapshotted, not this change.

Two 500s on malformed input (review fix)

Review found two inputs that reached a 500 on an endpoint whose whole point is coded errors, so both are now coded 400s.

A valid JSON scalar is not a parse failure. await request.json() is wrapped, but null, 5, "text" and [] all parse cleanly and then make body.get(...) raise AttributeError past that except into a 500. The shape now gets its own rejection: {"error": "body must be a JSON object", "code": "invalid_body"}. Applied to /context as well as /note — it has the identical hole and reaches the same validators, so fixing only the reported one would have left the sibling crashing. The file already knows this bug class: the guard in api_chat_wait_reply carries a comment describing exactly this, and normalizes to {}. These two endpoints reject instead, because their contract is explicit validation with a code rather than silent coercion.

An arbitrary-precision int passes the numeric check and then overflows the float conversion. isinstance(max_age, (int, float)) admits a 310-digit int, and math.isfinite raises OverflowError converting it — reproduced directly as int too large to convert to float. It now returns the existing non_finite_number 400, the same answer NaN and Infinity already get, since "cannot be represented as a finite float" is the same rejection for the same reason.

Both are new coded responses, so the per-file ratchet is untouched — error-code-baseline.json rebuilds byte-identical, all 62 files and _totals unchanged. Four tests cover them across both endpoints; removing either fix fails exactly its own two.

Two review fixes: null semantics, and a stale-entry cap lockout

maxAge: null meant opposite things on sibling endpoints. On /note an explicit null resolved to the 24h default; on /context it meant no expiry. Same field, same condition, two answers — and this is new App Kit surface, so it is cheaper to settle now than once apps depend on it. The cause is that body.get("maxAge") returns None for an absent key and for a JSON null, so the endpoint could not tell them apart to begin with. It now reads the key through a sentinel: an omitted maxAge takes this endpoint's 24h default, and an explicit null means no expiry, exactly as on /context.

The other direction — making /context adopt the 24h default — was rejected on evidence rather than taste. kirocrew-client-py serializes "maxAge": None unconditionally, so every inject_context() call without a TTL currently means permanent; flipping /context would silently give all of them a 24h expiry, while the client's own buffer filter still treats None as never-expiring, leaving client and server disagreeing about the same entry. It would also remove any way to request a permanent entry over HTTP, since the validator rejects Infinity and <= 0.

Expired entries held the per-source cap, locking a source out of fresh context. _source_cap_reached counted every entry for a source. Expired ones are dropped by drain_pending_context but stay in the list until the next drain, so ten already-dead notes could sit at the cap and skip the context half of every subsequent note — reported as contextSkipped: true with a 200, so the caller is told nothing is wrong. The drain already had the right predicate; it is now a shared context_entry_expired() in chat_runner.py used by both the count and the drain, so the two cannot disagree about which entries are live. It lives there rather than in chat_handlers.py because the import already runs handlers → runner, and the reverse would be a cycle.

Three review fixes: a live-entry eviction, a subtraction, and the double delivery

A full queue evicted a LIVE entry while already-dead ones survived. _enqueue_pending_context capped the 50-slot queue with while len(...) >= _MAX_PENDING_CONTEXT: pop(0), which selects by position. Expired entries are only removed by drain_pending_context, so between drains they sit in the list -- and a permanent entry at index 0 was discarded to make room while 49 dead ones stayed. That silently loses context the caller was told nothing about. It now sheds expired entries first, using the same context_entry_expired() the per-source cap and the drain already share, so all three agree about which entries are live. Reaching this needs at least five distinct sources, since the per-source cap is 10 -- and nothing covered the 50-cap or its eviction before, so the new test is the first of either.

The visible and context booleans are gone; /note always does both writes. They had zero callers anywhere in src/ or website/src/, and visible: false was a second spelling of POST /context -- which already exists, and which 429s at the cap where /note returned 200 with contextSkipped: true. Rather than reconcile two spellings of one operation on brand-new surface, the surface shrinks: the toggles, the nothing_to_write 400, and the boolean type-check go with them. appended is now always true. Five tests that pinned the removed modes are deleted, and the cap contract keeps its own test, which covered it better anyway.

A reset keeps a note's queued context, and may therefore deliver it twice. RECALL_ROLES includes inject, so a session reset replays the visible row while the still-undrained _pending_context entry is prepended to the same first message. An earlier revision dropped the queued copy on that basis. That was wrong: the replay is bounded by a CHARACTER budget, not a message count, so two large notes (content caps at 40,000 chars each) can exceed it -- the older row is trimmed out of the replay while its queued copy has already been discarded, and that note reaches the model in NEITHER form. The copy is kept unconditionally, and the marker and helper the guard keyed on are gone. A duplicate in one prompt is the acceptable failure; a silently missing note is not. A test pins the preserving direction so the guard cannot come back by accident.

The missing-slot 404 was distinguishable from an ownership denial. /note checked slot existence before the ownership gate and answered {"error": "slot not found"}, while a denial answers {"error": "not found"} -- so an app token could tell "this slot is not mine" from "this slot does not exist" and enumerate foreign slot names. Both /note and /context now return the shared _slot_not_found() helper, whose own docstring states the invariant the two must be byte-identical. /context had the same split -- its missing-slot answer differed from the ownership denial it pairs with -- so it is normalised through the same helper rather than giving each endpoint its own denial shape. A test asserts all four responses (missing and denied, on both endpoints) are equal, and the error-code baseline tightened by one as a result.

Owning a slot did not mean owning the session the write lands on. get_or_create_slot sets _app from its caller and, for a name shaped like a channel session stem, resolves linked_session_key from the session map in the same call (state.py:4818) -- so an app can own a slot bound to a channel thread it has no claim on. _check_slot_app_ownership tested ownership alone, and both of its callers write into that session: the visible row lands in the channel's own transcript, and the queued half drains into its next turn. The gate now also requires that the slot still routes to its own dashboard session, which is the second condition _app_cancel_denied already applies to /stop for exactly this reason -- its docstring names the escalation. Dashboard callers are unaffected (no app scope). All three denials in the gate are now single-sourced through _slot_not_found() so they cannot drift apart, since byte-identity is the property being defended. Tested in both directions: an app-owned slot carrying a foreign linked_session_key is refused on /note AND /context with a body equal to the missing-slot answer, and an ordinary app-owned slot with no linked session is still served.

Ownership went stale across the body read. The gate necessarily runs before await request.json(), and that await is a window rather than a formality: linked_session_key is rebound on ALREADY-LIVE slots with no running gate -- a cron completion (cron_inject.py:96), a workflow injection (workflow_inject.py:156) -- so a slow caller could be authorized against its own session and land on another conversation. Both endpoints now re-decide immediately before they touch the slot, through one _reauthorize_after_await helper. It requires the same slot OBJECT and not just the same name, because a delete-and-recreate under one name is a different conversation that would pass an ownership-only re-check, and it runs ahead of the first READ of slot state too, since running and the hold queue belong to whichever conversation the slot now routes to. Two tests drive the race through a stub request, because the rebind has to land during the await and a real client cannot schedule that; a control isolates the identity clause by weakening it to ownership-only, which fails the replacement test while the rebind test still passes.

The app-kit doc described flags the endpoint no longer accepts. docs/app-kit/api-reference.md still listed visible? and context? in the note body with "both false is a 400". Corrected to the shipped shape.

A mid-turn note took the row the replay path skips (review fix)

Posting a note while a turn was already running appended the visible inject line straight away, and that broke an invariant the replay path depends on. When a turn starts on a cold agent, build_session_replay is called with exclude_last_n=1 to drop the user message it has just written -- the comment at chat_runner.py:4445 states the assumption plainly, that exactly one recall-eligible row was appended before the turn fired. inject IS recall-eligible (RECALL_ROLES at context.py:59 is user, assistant, inject), so a note landing mid-turn became the last such row, the exclusion fell on the note instead, and the user's own request was replayed -- sending it twice.

The visible line is now HELD while slot.running or slot._in_stage_execution and written by flush_deferred_notes() from the seams that either end a turn or start the next one. _start_next_queued_turn flushes ABOVE the successor's own user row, for a queued message. The seams that close a cycle without starting one flush at the end: _finish_queue_cycle, and the stage loop's own exit, which covers the paused and cancelled paths and the plan as a whole -- nothing flushes BETWEEN stages, since a note is owed to the next USER turn. That exit seam skips the flush when a turn is running, for the reason given below. Held lines keep their order, and once the next turn appends its own user row the note is no longer last, so exclude_last_n=1 lands correctly again. done is not recall-eligible, so flushing beside it changes nothing.

Rejecting the note instead would have been fewer lines but it refuses the case the endpoint exists for -- a background actor leaving a breadcrumb precisely while the agent is busy. So the note is never dropped: the response now reports appended: false with visibleDeferred: true, and the caller can tell the two apart. The context half is held with it, for a reason a later review round found: it is next-turn by DESIGN but not by MECHANISM. The drain runs inside _run_chat (chat_runner.py:4526), which is reached long after slot.task is assigned (chat_runner.py:3506) -- so a note posted in that window queued its context into the turn already in flight. The note then shaped the request it was written after, and the next turn found the queue empty because the drain clears it. Both halves are now carried on the hold and written together by the flush. The hold is capped at 10 per turn (deferred_notes_full, 429) so one caller cannot park unbounded rows on a long turn. Two orderings matter and a later review round caught both. The cap is decided BEFORE either write: checking it after the context enqueue would 429 the caller while the note's content still reached the next model turn, which is worse than either accepting or rejecting it cleanly. And the flush has to run BEFORE any successor turn is dispatched, because a held note's context half drains inside that successor's own _run_chat (chat_runner.py:4526), so flushing afterwards would let the note shape a turn its visible line appears below. An earlier revision claimed that property while only flushing from the two turn-END sites, and a review round was right that this did not hold: the main dispatch path calls _start_next_queued_turn directly (chat_runner.py:7918) and only reaches _finish_queue_cycle afterwards, and each stage of a plan dispatches its own _run_chat. So a note held on a slot with a queued message had its context drained into that successor while its visible line stayed parked. The flush now sits inside the two dispatch seams themselves, above the row each appends, which is the choke point rather than a list of call sites to remember. A test pins the position rather than the presence: it fails when the flush is moved below the append, and a second one fails if either seam loses its call.

The held note's context reached the turn it was written during (review fix)

Deferring the visible line fixed the replay double-send but left the other half queued immediately, and a review round was right that this is the same bug wearing different clothes. slot.running is true the moment spawn_guarded_turn returns and slot.task = task is set, while the queue is not drained until drain_pending_context runs inside _run_chat. A POST landing between those two points was told its note was deferred -- and its context was handed to the turn already running, then cleared. So the note influenced the message the user sent BEFORE it existed, and the turn it was actually meant for saw nothing. That is a silent loss: the caller got a 200, the visible line did appear later, and only the context went to the wrong turn.

The entry is now built at the POST and carried on the hold, so every rejection the caller could get is still synchronous -- a bad maxAge is a 400, a full per-source bucket still reports contextSkipped, and a full hold is still a 429 -- while the queue write happens in flush_deferred_notes beside the visible line. To keep one definition of what "live" means, context_entry_expired moved down into state.py next to the ceiling it is used with, and the prune-then-evict tail became _ChatSlot.append_pending_context, now shared by /context, /note, and the promotion. Without that move the promotion would have needed its own eviction, and the FIFO-evicts-by-position hazard already documented at the original site would have been reintroduced at a second one. pending counts held entries as well as queued ones, so the field still answers the only question a caller asks of it: what will the model receive. Three tests pin this -- the running turn's drain returns empty, the next turn's drain contains the note, and a capped source holds a line carrying no context so the flush cannot promote a null.

Three review fixes: an unaudited refusal, a bypassed cap, and a flush into a live turn (review fix)

A slot replaced mid-request was refused without an audit record. The ownership gate logs a permission event on each of its three refusals, and the re-check that runs after the body read -- added for the rebind window -- returned the same 404 with no log at all. So an app that posted against a slot at the moment it was replaced left no trace, which is the one case where a trace is most wanted. It now emits the same app_isolation denial its three siblings do, and only when there is an app to name.

Ten held notes each passed a per-source cap that could only see nine of them. The cap counts a source's pending entries, and a note held for the deferred flush is not in the queue yet, so every held entry was invisible to it. With nine entries already queued, ten more notes from the same source were all admitted and the flush then promoted them together -- nineteen entries for a source whose ceiling is ten, and the overflow evicts other sources' context out of the shared fifty-slot queue by position. The cap now counts held entries alongside queued ones, using the same liveness predicate, so the count and the flush cannot disagree.

The stage loop's exit flushed into a turn that was still running. A stage's own turn can start a continuation -- a refusal recovery, for instance -- that owns the task past the stage exit. Flushing there handed that turn the note's context, and a turn drains the queue after its task is assigned, so the note shaped a request written before the note existed and the next turn saw nothing. The exit now flushes only when no turn is running; a running turn writes the note at its own completion, via _start_next_queued_turn if a successor follows it and _finish_queue_cycle if not, so nothing is stranded. The paused and cancelled paths reach that exit with no turn, so they still write as before.

Chatty inject producers were evicting real conversation from the replay (review fix)

Bringing inject into the recalled role set means cron results and stall-recovery rows now compete with user and assistant history for the replay's 80,000-character budget, and that cost had never been measured. Measured across 180 real session transcripts, 87 of which carry inject rows, by building the replay twice per session — once with inject in the role set and once without — and resolving each output back to the exact run of messages that survived the budget. Unbounded, 21 sessions lost conversation from their replay, 198 user and assistant messages in total. The worst slot dropped 44 of its 100 conversation messages so that 6 inject rows could take 17,141 characters, and across the corpus the largest inject contribution to a single replay was 33,718 characters — 42% of a recovery replay spent on cron output. Replay is the recovery path, read after process death or a provider switch, so that is half the history a resumed session gets.

Each inject row is now capped at 2,000 characters in the replay, truncated with the same …[truncated] marker the context-builder's fallback history path already uses — that path caps every message it renders, so the replay was the one recall path with no per-message bound at all. Conversation rows stay uncapped: they are the signal the replay exists to carry, while an inject row only has to record that a cron ran or a note was left. 2,000 sits above the 75th-percentile real inject row (1,525 characters), so typical breadcrumbs pass through whole and only the outsized dumps are clipped. Re-measured with the shipped code on the same corpus: the worst slot's loss falls from 44 conversation messages to 24, the corpus total from 198 to 135, and the largest inject contribution to any one replay from 33,718 characters to 11,163 (14% of the budget) — while slightly more inject rows survive than before (122 against 119), because smaller rows fit.

A held note followed the slot into another session (review fix)

/note is only accepted on a slot that still routes to its own session -- the ownership gate's third condition is effective_session_key(slot) != _history_key_for(slot.key) -> deny (chat_handlers.py:4717), so an accepted note is always authorized against the slot's own dashboard session. But the hold recorded only content, cls and context, and BOTH the transcript path and the next turn's session are resolved from linked_session_key at flush time (chat_utils.py:556 and :580). An unbound slot can acquire a binding while the note waits: cron_inject.py:95 and workflow_inject.py:155 assign linked_session_key when it is empty, with no running gate. Those are exactly the slots the gate admits.

Measured on the live code before the fix, with a slot named cron-job42 (a caller picks its own slot name): the note is authorized against dashboard:cron-job42, the cron's completion binds cron:job42, and the flush then writes the payload and promotes its context entry while the slot resolves to cron:job42 -- content authorized for one conversation addressed by another.

The hold now captures effective_session_key(slot) at the POST, and the flush drops any note whose captured session no longer matches the slot's live one, logging an app_isolation denial. The context half goes with it -- promoting it alone would hand the payload to the other conversation's next turn without the visible line. The reviewer suggested rejecting app-token notes when the note is deferred; binding is preferred because it keeps the away-note case working (that is the whole point of the hold) and because the leak is a property of late resolution rather than of app tokens -- a dashboard-authored hold on a slot that later binds has the same shape, and the rejection would not have covered it. flush_deferred_notes now returns the number actually written rather than the number held.

An already-dead note evicted live context (review fix)

append_pending_context pruned the entries already in the queue, then FIFO-evicted to make room, then appended -- but it never asked whether the INCOMING entry was still alive. At state.py the order was prune, while len(...) >= 50: pop(0), append. So an entry that arrived already expired popped a live entry off index 0 to take a seat the drain would immediately discard: a live entry traded for nothing.

Only the deferred hold can produce that input. An entry's expiry is injectedAt + maxAge, both set when it is built, and maxAge must be positive -- so at the POST it is never already expired. A held note is different: its maxAge can elapse while the turn it is waiting on runs, and the flush promotes it afterwards. Reproduced on the unpatched code with a queue of 50 live entries and one held note whose TTL had passed: 49 live entries left, the dead one seated. The guard now drops an expired entry before anything can evict, so the same input leaves all 50 live entries in place and the note's visible line -- which carries no TTL -- still lands.

Two tests cover it, one on append_pending_context directly and one end-to-end through the flush. Removing the guard fails both, while the neighbouring test for the opposite case (a live entry arriving at a queue full of dead ones) keeps passing -- so the two eviction directions are pinned separately rather than by one shared assertion.

A cancelled or timed-out stage dropped a note it had accepted (review fix)

The stage loop flushed a held note on its way out, but only if not slot.running -- and inside that loop's own finally, slot.running names the loop's own task, which is not done because it is executing that very block. So the guard read true and the flush was skipped. It read true on every exit path where the loop still owned slot.task, and the two that matter are cancellation and the stage ceiling: _bounded_turn cancels the inner turn without awaiting it, so _run_chat's own completion -- the thing that normally clears slot.task -- cannot run in time. On the close path the slot is then saved closed, which makes the drop permanent rather than merely late.

Reproduced before changing anything, by running the loop as a task assigned to slot.task exactly as api_chat_plan_action assigns it: with a note held, cancellation left the note still queued and no inject row in the transcript, and so did a stage that blew its ceiling. The positive control is the same probe with slot.task cleared the way a normal completion clears it -- there the guard fires and the note lands, which is why completion and pause were never affected.

The guard now asks who owns the task rather than whether one is running: it flushes unless a live task that is someone else's owns the slot. That keeps the behaviour the guard was written for -- a stage whose turn left a continuation running still defers, because that continuation drains the queue itself -- while no longer treating the dying loop as a reason to withhold. The reviewer's suggested _cancelled or not slot.running was measured against the same tests and fixes only the cancellation half: the ceiling path leaves _cancelled false, and that test still fails under it.

Two tests, one per path, and both fail with the guard restored while the neighbouring test for the deferring case keeps passing -- so "flush on the way out" and "defer to a live successor" are pinned by separate assertions rather than one shared condition. Neither test waits after the cancellation before asserting, because a later seam clearing slot.task would flush the note and hide the drop.

Notes could starve conversation at the two bounded recall sites, and the fix had to move off the loop thread (review fix)

Widening the recalled role set protected the replay path but left the two recall sites that issue a single bounded query. ConversationLog.recent role-filters and then takes a plain tail slice (history.py:2283-2285), so a run of inject rows longer than the bound IS the entire read: the cold-start fallback in build_session_context passes no max_messages and therefore takes recent's 20-row default, and compress_thread_history is bounded at 100. Reproduced by execution before changing anything, at both sites: with two conversation rows followed by notes past each bound, the fallback's history block ended in nothing but note rows and the compression transcript contained no user or assistant turn at all. Both now read through _recall_rows, which counts an inject quota and a conversation quota separately over the same rows, so notes reach the model without competing with user and assistant turns for the same slots — the discipline _replay_rows already applies on the replay path.

Row quotas alone did not hold, which is the part worth stating plainly: the fallback spends its character budget newest-first and notes ARE the newest rows, so measured across note sizes, 8,000-character notes still evicted every conversation row even with the quota admitting them. The fallback now reserves an inject share of that budget and caps each note row, skipping the ones that spill so the scan keeps looking for conversation rather than stopping at the first note that overruns — the same two-part defence, and the same constants, as the replay path. Each half is independently load-bearing: reverting the quota half fails all three tests, and reverting the budget half fails only the large-note one while the row tests still pass.

That guarantee needs the WHOLE file — a tail slice cannot bound each role independently — so the read cannot be the cheap one, which makes where it runs matter. recent's tail fast path only ever applied when exclude_last_n was 0, and the dashboard's own caller passes 1, so the full parse was already the common case rather than something this change introduced; _read_messages' docstring names the no-blocking-call-on-event-loop anchor itself and records that it is reached on the loop deliberately, with an mtime-guarded lock-free warm path as the existing mitigation. The compression read is now hopped off the loop thread with asyncio.to_thread (context.py:1257), which is safe against that cache's documented contract because _recall_rows only reads the shared cached list and copies out of it. Pinned by a test that records the thread each read runs on and asserts it is never the loop's. The DIRECTION is established — the enclosing function is this module's only coroutine — but the magnitude, a cold multi-megabyte transcript stalling every gateway coroutine, is not measured here.

Rebased onto main, and the baseline hunk this PR briefly carried is gone

For a few hours this PR carried a one-line deletion in .github/black-baseline.txt. The reason: scripts/check_black_formatting.py scopes new offenders to a PR's changed files but computes graduations GLOBALLY (graduated = sorted(baseline - unformatted), :234), so once src/kiro_crew/mcp_gateway/preflight.py was reformatted incidentally by #4295 its stale baseline entry reddened Backend Lint & Type Check on every open PR -- reporting 0 new offender(s), 1 graduated entr(y/ies) to prune, i.e. nothing in this diff was unformatted.

main has since pruned that entry itself (#4323), so on rebasing onto main the identical deletion applied as a no-op and git dropped the hunk. This PR therefore touches NO .github/ file at all, which is strictly better -- a .github/ touch is what routes a PR into the maintainer-label queue. Verified rather than assumed: the black gate was re-run the way CI evaluates it, on the merge commit built from main plus this branch, and passes with no hunk present (rc=0, nothing in scope is unformatted outside the baseline). The rebase brought the branch from 32 commits behind to 0 with no conflicts; the error-code ratchet and the four affected suites (481 tests) pass on the rebased tree.

The 200 promised a delivery the flush can refuse (review fix)

A held note is written only if the slot still routes to the SAME session when the turn ends. An unbound slot can acquire a foreign binding while the note waits -- cron_inject.py and workflow_inject.py assign linked_session_key when it is empty, with no running gate -- and both the transcript path and the next turn's session resolve that binding at flush time rather than at the POST (state.py:2315-2331). The flush then drops BOTH halves rather than retargeting them, because writing them would surface content authorized for one conversation inside another; that drop is deliberate and has its own regression test plus an unchanged-binding control. The defect was not the drop -- it was the acknowledgement in front of it: the POST returned a 200 with visibleDeferred: true and the shipped documentation asserted, in this same diff, that "a held note is not dropped while the gateway stays up". The code and the doc contradicted each other, and the doc's only disclaimer covered a gateway RESTART, which is a different case.

The response now carries deliveryConditional, true exactly when the note is held, so a caller can tell a queued write from a guaranteed one instead of reading 200 as durable delivery; docs/app-kit/api-reference.md no longer claims the note is not dropped, and states the rebind case and where the drop is recorded. Pinned in both directions by a test asserting the immediate path reports false and the held path true; reverting the response field fails it with KeyError: 'deliveryConditional'. The drop test and its unchanged-binding control were NOT touched -- no assertion was weakened, and the accepted-deferral path an ordinary unbound dashboard tab depends on still works.

The reviewer's proposed remedy -- reject deferred notes on an unbound slot -- was not adopted, and the reason is measured rather than argued: linked_session_key is empty for ordinary dashboard tabs, so rejecting on unbound would 4xx /note for the majority of slots for the whole duration of any running turn, which is the away-note case the endpoint exists for. It also contradicts this PR's own control test, which constructs an unbound slot and asserts the deferred note IS accepted and later written. Preserving the note by writing it to the session it was authorized against was considered and rejected on a structural ground: conversation_log hangs off DashboardState (state.py:3060) while flush_deferred_notes is a _ChatSlot method, and _ChatSlot.append resolves both its broadcast and its transcript from the slot's own live binding, so redirecting the write would need cross-session write machinery that does not exist here and would land a row in a file with no live surface showing it.

Closing a slot discarded the held note it had accepted (review fix)

_finish_queue_cycle withholds the flush from an automatic synthesis turn, because a note is owed to the next USER turn rather than to a turn the user never asked for. That withhold assumed a successor exists. It does not when the slot is being torn down: with _pending_synthesis set and the slot already removed from state._slots, will_synthesize was still true, so chat_runner.py:3595 skipped flush_deferred_notes() and the close persisted without the note the POST had acknowledged.

Reproduced by execution before changing anything, and it is a real loss rather than a late delivery: a slot holding one deferred note, _pending_synthesis true, absent from the registry, run through _finish_queue_cycle -- the note was still sitting in _deferred_notes and no inject row ever reached the transcript. will_synthesize now also requires state._slots.get(slot.key) is slot, so a torn-down slot takes the flush path instead. Reverting only that conjunct fails the new test with "the held note was discarded on close" and fails nothing else.

The second effect of the identity test was checked rather than assumed, because it also fires when a key has been REBOUND to a different slot object, which flushes a slot that is mid-teardown. That is safe here: flush_deferred_notes is slot-local -- it resolves its target through effective_session_key(self), which reads only linked_session_key and the slot key, and appends through _ChatSlot.append, which touches the slot's own messages/_pending. Neither reaches back through state._slots, so the repair cannot write a note into another slot's transcript.

Two existing synthesis tests were changed, and the reason is the contract rather than convenience: they construct a slot and set _pending_synthesis WITHOUT registering it in state._slots, which is a state a live slot cannot be in. They now register it. The withhold assertion itself is untouched and still passes -- synthesis continues to withhold the note from a live slot, which is the behaviour the comment at chat_runner.py:3590-3594 exists to protect.

Bulk cleanup archived a slot before writing the note it had accepted (review fix)

POST /api/chat/slots/cleanup bulk-archives inactive tabs. For each one it removes the tab from the registry, writes its final record, and only then cancels any turn still running on it. A note being held for the next turn is flushed by that cancelled turn on its way out -- which is after the final write, into an object no longer in the registry. The note was accepted with a 200 and then never appeared anywhere durable.

Reproduced by execution before anything was changed: a tab stale enough to archive, one held note, a running turn, archived through the endpoint, and the persisted record came back holding only the older user row -- the note absent. The archive itself succeeded, so this was silent loss rather than a failed request. The fix flushes held notes into the record immediately before that final write, so the archive carries them. The flush is idempotent, so the cancelled turn's own later flush is a no-op and the note is not written twice.

The narrower fix was chosen over reordering the cancel ahead of the save, because that ordering is load-bearing for a separate reason the surrounding code documents: the archive pass must not race a concurrent reconcile into resurrecting the tab, and the write is what closes that window. Rejecting notes while a turn runs -- the other option -- was not viable: deferring a note during a running turn is the feature this endpoint adds, so rejecting them removes it.

Verified by negative control: with only the added flush removed, the new test fails on exactly its own assertion and nothing else changes. The suites that own this endpoint were run in full alongside the note suites.

Reviewer dispositions on this revision

The acknowledgement no longer promises durability, which is the part that was actually wrong. A reviewer read POST /note's 200 as a delivery guarantee and proposed reverting the endpoint until the queues survive a restart. The storage observation is correct -- neither queue is persisted, and chat_persistence.py references neither -- but reverting is aimed at the wrong target: _pending_context already exists at main and has always been memory-only, so the context half's volatility is inherited from /context rather than introduced here. What this PR did introduce was a false claim about it. docs/app-kit/api-reference.md said a held note meant "Nothing is lost", and the handler docstring said "The note is not lost" -- both untrue across a restart, and /context's own docs promise nothing of the kind. Both now state that a 200 means accepted for this gateway lifetime, that both halves are dropped by a restart between the acknowledgement and the next turn, and that a caller needing survival must re-post. visibleDeferred: true is documented as ordering against the running turn, not persistence. Persisting the queues stays a follow-up; the limitation is stated below rather than implied.

Follow-up filed as #4094: replace the positional flush invariants with an identity stamp. The design lane is right that this is a tracked-work item rather than a prose note, and it is now an issue rather than prose. It is owed work on the replay path, not on this endpoint: stamp each context entry and the turn with an identity or timestamp, then have drain_pending_context and the replay exclusion filter on that stamp instead of on exclude_last_n=1 and on the drain happening after slot.task is assigned. That collapses the four flush call sites into one filter and removes the discipline a future _run_chat dispatch path has to remember. Until it lands, the ordering is pinned by a test asserting string offsets in inspect.getsource output, which is why the gap is named in the limitations section.

The split recall surface is deliberate scoping, and these are the three surfaces that disagree. RECALL_ROLES widens only the three filters in context.py, which is what dashboard replay reads. Session summaries (handlers/sessions.py:1057), MCP recall (mcp_tools/sessions.py:320) and Discord resume (discord/session_resume.py:548, with a redundant second filter on the same fetched set at :560) each still pass the literal {"user", "assistant"}, so an inject row is invisible to all three. Widening them would change what every existing inject producer contributes to a summary and to a Discord resume -- a behaviour change for cron results and stall recoveries that has nothing to do with this endpoint, and it belongs in its own change.

The deferred hold's durability is a decided tradeoff, not an oversight. The design lane asks for a human decision on shipping the hold unpersisted, and the owner made it: persistence lands as a follow-up change after this merges, and the response shape is not being downgraded in the meantime. The gap itself is stated in the limitations below rather than papered over.

The "required circular-import explanation" on the deferred import is not a repo rule, but the comment was worth adding anyway. No linter or documented convention enforces it: across src/ there are 2,291 function-scoped imports and 189 carrying a # circular import note, and in state.py itself the majority of deferred imports carry no explanation at all. What did justify the line is that the cycle is invisible from inside state.py and two nearby sites importing this exact symbol already name it. Hoisting the import to module scope fails concretely -- ImportError: cannot import name 'BUSY_RECOVERY_PREFIX' from partially initialized module 'kiro_crew.dashboard.state' -- because chat_utils.py:24 imports state at module scope (outside its TYPE_CHECKING block, which closes at line 22). The comment names that direction in one line.

The four-seam flush is a real design observation and it stays a follow-up. The design lane is right that the flush is defended positionally at four dispatch seams and that only inspect.getsource offset assertions stand between that and a future dispatch path missing it, and its suggestion -- funnel dispatch through one choke function -- is the right shape. It is not this PR: the cause-level version changes the replay exclusion and the context drain rather than this endpoint, and doing it here would put a rewrite of the recall path inside a change that adds one route. It is named in the limitations below with the specific mechanism rather than left as a gesture.

Both follow-ups are now filed as issues, so the record no longer depends on this description. The design lane's objection was correct when it was written: hold persistence and the identity-stamp refactor were tracked nowhere but here, and a merged PR's description is not a work queue. They are now #4093 (persist the deferred-note hold so a 200 is not voided by a gateway restart) and #4094 (replace the positional flush invariants with an identity stamp). I searched first rather than filing blind -- no existing open issue owned either, so neither could be cited instead. This paragraph previously stated the opposite, that a description-only record was the honest answer because no ticket existed; that was true at the time and is now superseded.

Release note for existing /context callers: three behaviours change with this PR. The two endpoints share their validators and their ownership gate, so callers of /context see the change even though the new route is /note. A source over 64 characters or containing a control character is now a 400 (source_too_long / invalid_source) where it was previously accepted -- neither limit exists at main. The two denial bodies are now byte-identical: main returned {"error": "slot not found"} for a missing slot and {"error": "not found"} for an app-isolation denial, and both are now the single _slot_not_found() body, deliberately, so no response an unauthorized caller can reach distinguishes the two cases. And an app-scoped request against a slot whose session has been linked elsewhere -- a cron or workflow injection having rebound it -- is now denied rather than served, which is new: main's /context gate did not consult the session binding at all.

On sequencing #4094 and #4093: agreed in direction, and here is the countable trigger for it. Both remaining design items are arguments about ORDER rather than about this diff, and merge order is not mine to set -- so rather than promise it, here is the fact a reviewer can enforce against. This PR introduces the hold mechanism from nothing: flush_deferred_notes has zero call sites at main and four here, plus the definition of the method itself. The four calls are chat_runner.py:3726 (in _start_next_queued_turn), chat_runner.py:3991 (in _finish_queue_cycle), chat_orchestrator.py:701 (the _stage_loop exit) and chat_handlers.py:3468 (in api_chat_slots_cleanup, the teardown flush); the method is defined at state.py:2505, which is not a dispatch seam and is not counted as one. So the lane's "scaffolding" reading is fair, not harsh. Two earlier revisions of this paragraph carried a higher count and line numbers that later rebases moved: the inflated count came from counting added diff lines that name the symbol, one of which is the definition. Every coordinate above was re-measured at this head by sweeping all of src/ for the symbol, with a negative control confirming the sweep returns zero for a name that does not exist. I would rather state it that way than claim a sequencing commitment I cannot keep. On #4093, the contract risk is the reason the acknowledgement was reworded rather than left alone: the App Kit reference and the handler docstring now both say a 200 means accepted for this gateway lifetime and not durable delivery, so a caller reading the documented contract is not misled while the follow-up is outstanding. That reduces the mis-assumption the lane is worried about; it does not remove it, which is why #4093 is open rather than closed.

The /context callers are audited, and no in-repo caller can trip the new source 400s. The design lane asked for this before merge rather than after, so here is what I searched and what it found. Two layers forward source unchanged without validating or truncating it -- inject_context in the Python client and chatSlotContext in the web client -- so the only values that matter are what their callers pass. There are three production call sites, all passing a hardcoded literal: papyrus-co-author (17 characters) and artifact-companion (18, at two sites). The documented examples use watch (5) and the client's own default is None, which the handler defaults for you. The longest real value is 18 characters against a 64 limit, and none contains a control character, so zero of them change behaviour. One caveat on method: my first sweep grepped for the endpoint path as a literal and MISSED the web client, which builds the URL by concatenation; a second sweep for concatenated builders found it and confirmed it is the only one, which is why the count above is three and not two. I also proved the check discriminates rather than trivially passing everything: a 65-character label is rejected as source_too_long, an embedded newline and an embedded tab are each rejected as invalid_source, and a label of exactly 64 characters passes.

No deprecation window is offered, and that is deliberate rather than an oversight. An accept-with-warning path would keep alive the exact input the limit exists to reject. drain_pending_context interpolates the label straight into a frame the model reads -- [Background context from "<source>"] on its own line, closed by [End of background context] -- so a label containing a newline lets a caller add arbitrary lines between that opening delimiter and the content it is supposed to introduce. That frame and its unvalidated interpolation both predate this PR and are untouched by it; what changes is that the label can no longer break it. An over-64-character label has no legitimate use either, since the value's other job is to key a per-source cap bucket. The honest limit of this audit is that it covers callers in this repository: an installed third-party app passes its own label and cannot be enumerated from here, so such an app using an oversized or control-character label would receive a 400 on upgrade. That is the trade being made knowingly, and it is why the constraint is stated in the App Kit API reference rather than only here.

The /context behaviour changes are documented in the App Kit API reference. The design lane asked for them surfaced where a consumer would look, and docs/app-kit/api-reference.md is that place: it now states the source/maxAge/content constraints AND the ownership refusal semantics in present tense, directly beside the endpoints they govern. CHANGELOG.md is written at version-bump time (CONTRIBUTING.md, "Update CHANGELOG.md ... as part of the release"), and this PR carries no version bump, so it does not touch that file.

The RECALL_ROLES suggestion is declined, and the reason is scope rather than disagreement. The lane is right about the mechanism: a named constant exists precisely so a role set is not re-spelled, and three callers still pass the {"user", "assistant"} literal, which does defeat part of its purpose. Those callers are session summaries (handlers/sessions.py:1057), MCP recall (mcp_tools/sessions.py:320) and Discord resume (discord/session_resume.py:548, with a redundant re-filter of the same fetched set at :560) -- read back at the current tree to confirm each still reads as cited. None of those three files is in this PR's changed set, so it is not the mechanical substitution it looks like: switching them changes what every existing inject producer contributes to a summary and to a Discord resume, which is a behaviour change for cron results and stalled-turn recovery rows that has nothing to do with this endpoint. Widening context.py alone was the deliberate scope; the other three deserve their own change with their own reviewers.

A shared test-suite failure in TestLinkTimeBackfill is not reachable from this diff, and its different-looking assertion is the same root cause. A CI run flagged test/test_slack_options_lifecycle.py::TestLinkTimeBackfill::test_only_the_newest_reply_is_answerable failing with assert 0 == 2, where sibling failures of that class report AttributeError: 'NoneType' object has no attribute 'args'. Those are the same fault seen from two angles: the test reads post_blocks.await_args_list and asserts its length is 2, so 0 means the mock was never awaited at all -- and on a never-awaited mock the other members' post_blocks.await_args is None, which is where their AttributeError comes from. So the differing text is which attribute each member happens to touch first, not a different defect. This PR is not on that path: the handler is api_chat_slot_slack_link in chat_slack.py, and its route is registered in routes/sessions.py and routes/taskrunner.py -- none of those three files is in this changed set, which touches only routes/chat.py and only to add the note route. The two methods the test drives, _ChatSlot.append and _ChatSlot.drain, are untouched: state.py is purely additive here (+109/-0) and its new methods are called from nowhere on this path. Across every added and removed line under src/, this diff adds or removes zero lines mentioning post_blocks, and its single textual match for get_or_create_slot is inside a docstring rather than executable code. The class also fails on changes that could not possibly reach a Python test: PRs #2822 (33 files), #3240 (23) and #4116 (12) each contain zero .py files, counted from the file lists rather than assumed. Locally the named member passes alone, the whole TestLinkTimeBackfill class passes (6), and the whole file passes (78). The suspected mechanism is an order-dependent interaction with a pending task leaking from an earlier test in the same parallel worker; that is neither confirmed nor this PR's to fix, and no change was made to that test class from here.

Three review findings, all real, all fixed (review fix)

Capped inject rows could spend the whole replay budget and evict every conversation row. The row quota added last round decides which rows are SELECTED; the character budget is spent afterwards, newest-first, and was shared. With the quota derived as _REPLAY_BUDGET_CHARS // _REPLAY_INJECT_CAP_CHARS that permitted 40 rows at the 2,000-char ceiling against an 80,000-char budget -- exactly enough to exhaust it, so the render loop hit its break before reaching a single user or assistant row. Measured on the pre-fix code: inject rows took 78,858 of 80,000 characters and the replay contained no conversation at all. Two changes close it. inject rows now get a reserved SHARE of the budget (_REPLAY_INJECT_BUDGET_DIVISOR, one quarter), and when an older inject row would overspend that share it is skipped while the scan continues looking for conversation rather than stopping. The row quota is now derived from that share instead of the whole budget, so it is 10 rather than 40 and the two bounds agree by construction. Conversation keeps at least 60,000 characters whatever the breadcrumb volume.

A held note was handed to automatic synthesis instead of the next user turn. _finish_queue_cycle flushed unconditionally and then, in the same function, dispatched _run_pending_synthesis when synthesis was armed. So the note's context half drained into an automatic turn the user never asked for -- which defeats the hold for exactly the case the hold exists to serve. The flush is now withheld when that dispatch is about to happen, and lands on the following cycle instead. This cannot lose a note: the user-turn seams flush independently at their own dispatch sites, which is what the inspect.getsource offset test pins, and that test still passes untouched because it pins _start_next_queued_turn and _stage_loop rather than this function.

A leading or trailing control character was trimmed away instead of rejected. _validate_source ran _SOURCE_CTRL_RE against the value AFTER _normalize_source, which returns source.strip(). Because strip() removes the whitespace-class control characters, a source of "watch\n" or "\twatch" was silently accepted with a 200 while the documented contract says no control characters or newlines. The check now runs before normalization. Space padding still trims, so the shared per-source cap bucket for a padded label and its bare form is unaffected -- a space is outside the control-character class. Worth stating plainly, because it bounds the severity: the frame drain_pending_context builds was never breakable this way, since the value that reaches it is the stripped one. This was a contract defect, not an injection hole.

Answers to the design review's three sequencing points

The four flush seams stay, and here is the honest reason. The lane is right that the ordering is defended by position and pinned by source-offset assertions rather than by a structural invariant, and right that landing the identity stamp (#4094) first would have made this endpoint smaller. The seams stay because the alternative is to rewrite the replay exclusion and the context drain inside a change that adds one route, which is a larger blast radius on the path every turn already uses. What has changed this round is that the seam count is no longer the only guard: the synthesis withhold above means the automatic-turn case is handled by a condition rather than by ordering. The countable trigger has now fired, and it fired inside this PR rather than a later one: the archival flush described above is the fourth call site. It is not a new dispatch path competing for the same ordering -- it repairs silent loss on a teardown path that already existed -- but the trigger was stated as a count a reviewer could enforce, so it counts. #4094 stays the right follow-up, with those four call sites to fold into one filter.

The 200 plus visibleDeferred: true response shape is intended to be stable, and persistence will not change it. #4093 makes the hold durable; it does not alter the field or its meaning. A caller reading visibleDeferred: true today learns that the visible line is ordered after the running turn, and that stays true once the queue survives a restart. What changes is only how much a restart can take away, which the reference already states: a 200 means accepted for this gateway lifetime, not durable delivery. So a third-party app coding against this shape now is not inheriting a compat problem; it is inheriting a guarantee that gets stronger.

The split-brain inject recall is deliberate and is stated as user-visible in the limitations below. Replay sees notes because RECALL_ROLES widened the three filters in context.py; session summaries, MCP recall and Discord resume still pass the literal {"user", "assistant"} and do not. The lane's reading is correct: until those three are widened, "what happened while I was away?" answers differently depending on which surface is asked. That is named in the limitations section rather than left implicit, and widening the other three is its own change because it alters what every existing inject producer contributes to a summary and to a Discord resume.

Correction on "the only structural guard is a source-offset test". The design review says the flush ordering is defended solely by the inspect.getsource string-offset assertion. That is not accurate: three tests execute the real functions and assert behaviour. test/test_chat_runner_coverage.py:1393 runs the real _start_next_queued_turn -- only spawn_guarded_turn and _run_chat are patched, not the flush -- and asserts the actual row order in slot.messages (contents.index("held") < roles.index("user")) plus that the hold is emptied. test/test_chat_runner_coverage.py:1494 and :1518 run the real _finish_queue_cycle and assert the flush is not called and called exactly once respectively. That pair matters most, because _finish_queue_cycle is not named by the source-offset test at all, so for that seam the behavioural tests are the only guard -- the reverse of the review's claim. The other half of the point stands and this description already concedes it: nothing enumerates dispatch SITES, so a fifth one is invisible to both kinds of guard. That gap is about site enumeration, not assertion technique, so replacing the source-offset test would not close it.

Rebased onto main, and the one file that conflicted

GitHub reported this branch CONFLICTING against main, which stops every check from re-running, so no verdict on the PR was current. The branch was 53 commits behind. It is now rebased onto main at a83c67ef2, still as a single commit.

Exactly one of the sixteen files conflicted: error-code-baseline.json. The other fifteen merged without conflict, including the shared dashboard plumbing (chat_handlers.py, chat_runner.py, state.py) that a conflict here would most plausibly have touched. Nothing this branch adds had since been added on main, so nothing was dropped as a duplicate.

Both conflict hunks were generated counters and nothing else: the _totals.missing_code and _compliant totals, and the dashboard/chat_handlers.py entry. That file is generated (its own _comment says to regenerate it with python test/test_error_code_contract.py --update), so neither side of the conflict was the right answer for the merged tree -- main said 1390 missing and 819 compliant, this branch said 1385 and 820. It was resolved by taking main's side and then regenerating from the merged source, which produced 1384 missing, 844 compliant, and 67 for dashboard/chat_handlers.py. That number is arithmetically consistent: main's 1390 minus the six responses this branch fixes in chat_handlers.py (73 down to 67) is exactly 1384, so the ratchet moves down and no number was raised to make CI pass.

Re-verified on the rebased tree rather than assumed: test_error_code_contract.py passes all six tests, including test_baseline_is_not_stale, which independently confirms the regenerated file matches the committed source. The four test files this change touches pass 475 tests. Because a rebase across this plumbing can compile cleanly and still change behaviour, a wider sweep of the 138 test files covering chat, state, history, context and dashboard code was also run: 4611 passed, 0 failed.

One claim in this description was checked and corrected while re-verifying citations after the rebase: the Discord resume call site is src/kiro_crew/discord/session_resume.py, not session_resume.py -- the directory was missing. Both line numbers were correct and are unchanged (:548 passes the role set positionally, :560 re-filters the same fetched set), and that file is not in this PR's changed set. Re-auditing every file:line citation in this description against the rebased tree turned up three more that were already wrong before the rebase and one the rebase itself moved: the ownership gate is at chat_handlers.py:4717 (cited as :4464), the runner's turn-start flush seam, whose coordinate at this head is the one given in the sequencing paragraph above, the main dispatch's _start_next_queued_turn call at chat_runner.py:7918 (cited as :7813), and get_or_create_slot's linked_session_key parameter moved to state.py:4818 from :4564 when the rebase added lines above it. All four are corrected above; every other citation was re-checked and is unchanged. The flush_deferred_notes seam count also still holds after the rebase: zero call sites on main, four here, so no fifth dispatch seam appeared while this branch was behind.

Two GPT 5.6 findings: a note leaking into a plan's next stage, and a silently killed turn (review fix)

A held note was released into a plan's next stage. A note is owed to the next USER turn and every stage of a plan is automatic, but neither runner flush seam consulted _in_stage_execution. _finish_queue_cycle runs per stage -- from inside each stage's own _run_chat finally, while that flag is still set -- so it fed stage N+1. The leak reaches the other seam first, though: a plain queued user message carries no origin kind, so _start_next_queued_turn flushed at its top, ABOVE the in_stage gate that then holds that message back, releasing the note while no user turn started at all. Both seams now require not slot._in_stage_execution, and _stage_loop's exit flush delivers the note once the plan ends -- so this delays delivery rather than losing it. Guarding only the reported line would have changed nothing on the queued-message path.

A failed archive restored a slot whose turn it had already killed. Bulk cleanup cancels a running turn BEFORE the archival save, deliberately, so the doomed turn cannot drain the note's context half -- which means a save that then fails rolls the slot back with that turn already dead. running is derived from the task, so a completed cancel reads False: the tab came back looking idle and dispatchable with that turn's output gone and nothing on the rollback path saying so. The rollback now drops the dead task and appends an error row. Reverting the cancel was the suggested remedy and is not viable -- the same hunk added the bulk path's only flush_deferred_notes(), which TestCleanupPersistsHeldNotes pins in both directions. Reporting is scoped to a task that actually finished; one outliving the shielded wait is still running, so the restore loses nothing there.

Three tests, each carrying its own control: the two stage seams (control -- the same fixture off-plan still flushes) and the failed-archive rollback. All three fail at the parent commit for their stated reason.

Limitations, honestly

• The flush is defended positionally, at four seams. The hold exists because the replay exclusion counts rows (exclude_last_n=1 assumes exactly one recall-eligible row precedes the turn) and the context drain runs inside the turn after slot.task is assigned. Defending both by position needs a flush call at four dispatch/exit sites, and the ordering is pinned only where a seam appends its own user row: one test asserts string offsets in inspect.getsource output for _start_next_queued_turn and for the stage loop's single exit call, which that test pins by asserting the count of calls in the loop's source is exactly one, while _finish_queue_cycle is pinned by nothing at all, since it appends no row of its own to be above. A test now enumerates those sites rather than naming functions: it walks every module in the package for a function that drains the queue to start a successor turn, and fails unless a flush lands above that drain, so a new dispatch path that skips the flush turns red wherever it is added. That was verified by adding such a path to chat_runner.py and running both guards: the older source-offset assertion still passed, while the enumerating one failed and named the offending function. What is still positional is the ORDERING -- the flush is correct because of where the call sits, not because the row carries an identity -- and that is what Replace the positional flush invariants behind the mid-turn note hold with an identity stamp #4094 replaces. The cause-level fix is to stamp context entries and the turn with an identity or timestamp and have the drain and the replay exclusion filter on that instead of on position, which collapses the hold-and-flush machinery into a filter and removes the choke-point discipline. That is a change to the replay exclusion and the drain, not to this endpoint, so it is named here rather than attempted.

• A deferred note is acknowledged but not durable. _deferred_notes is in-memory, like _pending_context before it, so a gateway restart between the 200 and the flush drops both halves of a note the caller was told would appear. That is wider than /context's inherited volatility, because here the response explicitly promises a transcript line (visibleDeferred: true). Persisting the hold would need it written to the slot's own metadata and replayed on restore, which is a bigger change than this endpoint should carry; it is a real gap rather than an accepted one, and it is filed as Persist the deferred-note hold so a 200 is not voided by a gateway restart #4093.

• Three other callers of the same recall API still pass the literal role set, so a note does not reach them. The RECALL_ROLES widening covers the three filters in context.py, which is what dashboard replay reads. Summaries (handlers/sessions.py:1057), MCP recall (mcp_tools/sessions.py:320) and Discord resume (discord/session_resume.py:548) each pass {"user", "assistant"} directly, so an inject row is invisible to all three. Deferred deliberately: widening them changes what every existing inject producer contributes to a summary and to a Discord resume, which is a behaviour change for cron results and stall recoveries that has nothing to do with this endpoint and deserves its own change.

• The second ownership gate could be removed by reading the body first, and is kept anyway. One gate after the read would close the rebind window with less machinery. It would also move the body-shape rejections above the gate, so a foreign app would get invalid_json or invalid_body where it now gets the same 404 as a slot that does not exist -- reintroducing exactly the response-shape oracle the unified 404 body closes. The gate runs before the read so that no response an unauthorized caller can reach varies with the request.

• The context half is not redacted. That is inherited from /context and unchanged here, but /note gives it a second caller, so it is worth naming rather than leaving implied. Both are behind the token-auth middleware; the trusted-caller assumption is the same one /context has always made.

• Notes should be declarative. An interrogative note is not rejected — it rides along as background context and may get answered on some later unrelated turn, which reads as a non sequitur. This is a documented convention, not an enforced one; validating "is this a question" is not a job for a regex.

• No frontend change. role="inject" is already a first-class rendered role, so the visible line needs nothing new client-side. There is no persistence change: an inject row's cls is deliberately not persisted (chat_persistence.py keeps it for role == "system" only), which is the invariant meta.injectKind exists to satisfy.

• A note used to reach the model twice in one prompt. Fixed -- see the review-fix section above. What remains is the deliberate asymmetry that made it fixable: the replayed row is redacted and the queued copy was raw, so a note carrying a secret now reaches the model in its redacted form only.

• RECALL_ROLES widens replay for all existing inject producers, not only notes — cron results and stalled-turn recovery rows now survive a session boundary too. That is the intended fix, but it is a behaviour change beyond this endpoint and reviewers should weigh it as one. What it costs is now measured and bounded (see the review-fix section above): a recalled inject row can take at most 2,000 characters of the replay, so a slot with N such rows in its recent history gives up roughly N x 2,000 characters of older conversation. The residual is inherent rather than a defect — recalling a breadcrumb costs a breadcrumb's worth of budget — and the measured worst case is 24 messages of a 100-message replay. Two sibling recall paths displace by message count rather than characters and were deliberately left alone: thread-history compression takes the most recent 100 messages and the context-builder's fallback the most recent 20, so an inject row there costs one of those slots rather than a share of a character budget. On the same corpus that is at most 10 of 100 and 4 of 20. The fallback path already caps every message it renders and the compression path feeds a summarizer that re-bounds its own output, so neither has the unbounded-eviction shape the replay had.

The /context tightening stays in this PR, and the reason is the shared gate

The design lane's remaining suggestion is that the three behaviour changes to the existing POST /api/chat/slots/{slot}/context endpoint -- the new source_too_long / invalid_source 400s, the deliberately identical 404 body for a missing slot and an app-isolation denial, and the denial of an app-scoped request against a slot whose session is linked elsewhere -- would have been easier to review as their own PR, so the compatibility story could be judged on its own. That is a fair reading and it is worth saying why it was not done that way, rather than leaving it unanswered.

The coupling is the change, not an accident of packaging. /note and /context share one set of validators and one ownership gate (chat_handlers.py:4717) precisely so the two routes cannot drift on what they accept or on who is allowed to write to a slot. Splitting them would mean either landing the shared gate twice and reconciling it later, or landing /note against a copy of the old validators and then rewriting both -- and in both orderings a reviewer reads the compatibility change without the reason it exists. The unified 404 in particular is only defensible next to /note: it exists so that no response an unauthorized caller can reach distinguishes "no such slot" from "not yours", which is a property of the pair rather than of either route.

What the suggestion does earn is discoverability, and that is handled rather than dismissed: all three are documented in docs/app-kit/api-reference.md beside the endpoint they govern -- the input constraints as a Constraints list and the two refusal behaviours as an Ownership list, both in present tense -- so a /context caller auditing the reference meets them where they already read about the endpoint.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed c2824125a45a906a40c697e55714bff301c0240d via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c282412

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of c2824125a45a906a40c697e55714bff301c0240d via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Real problem, working solution — but it defends two mutable invariants with guards at every seam instead of fixing either, and its own review history proves that shape leaks.

Watch

• The hold/flush machinery exists only because the replay exclusion counts rows ("exclude_last_n=1 assumes exactly one recall-eligible row") and the context drain is positional. The description's ~15 review-fix rounds are each a missed seam in that defense — stage loops, synthesis, bulk cleanup, teardown, cancellation — and the author concedes the identity-stamp refactor (Replace the positional flush invariants behind the mid-turn note hold with an identity stamp #4094) "would have made this endpoint smaller." Shipping the scaffolding first means every future dispatch or teardown path must remember the flush-above-drain discipline; the AST seam-enumeration test mitigates this but only for paths that drain via the two named helpers. Replace the positional flush invariants behind the mid-turn note hold with an identity stamp #4094 should land soon, not eventually.
• Same pattern on the session-rebind axis: because linked_session_key is rebound with no gate ("a cron result or workflow injection claims an empty linked_session_key with no running gate"), the noteSession stamp is now checked at three independent late-resolution seams (flush, drain, persistence save). A fourth consumer of slot content that forgets the check silently leaks cross-session — nothing enumerates those seams the way the drain seams are enumerated.

Suggestions

• Consider gating the rebind itself — refuse to claim a slot holding stamped notes/pending context — which would collapse the three distributed drop-checks into one condition at the two binding sites.

[DESIGN-REVIEWED] c282412

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed c2824125a45a906a40c697e55714bff301c0240d via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

The sole candidate (chat_handlers.py:3466, broad except (asyncio.CancelledError, ...)) does not survive falsification. The removed.task.cancel() on the line above means the shielded task itself raises CancelledError, so catching it is the intended path (the code comment and the parallel TimeoutError branch confirm this is deliberate handling of the turn's own cancellation, not a leak). The only genuinely-suppressed case the candidate posits — a cancellation delivered to the cleanup coroutine itself — leads to completing an idempotent best-effort archive/flush, which is desirable on teardown, not an observable wrong outcome; there is no crash, data loss, corruption, or security consequence. (c) resolves to "delayed unwind," which is exactly the "could/might" the bar rejects, and the candidate's own confidence is "low."

[OPUS-REVIEWED] c282412

[github-actions]

First Principles Review (Fable 5, fork) — ⏭️ skipped

Revision c2824125a45a906a40c697e55714bff301c0240d ships no reviewable capability, so there is nothing to inventory. Advisory — does not block merge.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[bolichen97]

Description / diff consistency check

The limitations section asserts "There is no persistence change" and that chat_persistence.py references neither of the new concepts. The diff adds 60 lines to src/kiro_crew/dashboard/chat_persistence.py:

• _save_slot_to_history now snapshot-retries the routing
• note rows are filtered out of the save snapshot via a new _note_authorized_elsewhere check
• a new note_save_drop SEL denied audit event is emitted on the periodic save path

That is a real behavior change on a path the description says is untouched. The accounting is also stale: the body says "sixteen files" and state.py "+109/-0"; actual is 15 files and state.py +177/-0. There is an unmentioned cosmetic reformat of the source-links route in routes/chat.py too.

The core /note endpoint, RECALL_ROLES, caps and flush seams all check out — it is just the persistence claim that needs correcting.

Everything else in this PR lines up with its description; this is only about the description matching the diff, so a reviewer approves what actually ships.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #6813 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6813: KEEP. Ancestor feature rather than competing work; nothing about it makes 6813 redundant. Files: src/kiro_crew/dashboard/chat_handlers.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.