feat(messaging): spool inbound messages the shutdown gate refuses, and notice them - #8913
Conversation
Design Review (Fable 5.1) — ✅ PASSDesign-level review of The design rests on real, verified seams ( Design-Verdict: PASS Notice-only replay behind the existing [DESIGN-REVIEWED] f18efe3 |
First Principles Review (Fable 5.1) — 🟡 CONCERNSPremise-level review of I've verified the load-bearing claims. Let me finalize.
First-Principles-Verdict: CONCERNS A clean cause-level fix for silent restart data loss, but 6 of 10 channels still drop refused inbound (deferred to #8912) — a human should see that coverage gap. What this change shipsIntent: stop a message sent during a gateway restart from vanishing silently — on next start, tell the sender it wasn't processed and quote it back. FIX.
Watch
[FIRST-PRINCIPLES-REVIEWED] f18efe3 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsCANDIDATE 1 dies at (a): the failure requires a transport whose No findings. [OPUS-REVIEWED] f18efe3 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
d6e17d3 to
139916b
Compare
|
Agent-writable spool permits forged operator messages — span=582f20a4331e — fixed in Legitimate, and the framing is the part worth agreeing with out loud: the spool is not state, it is a turn source. Every entry names a session key and an allow-listed peer and is replayed as that operator's own message, so a file an agent can write is a way to forge an operator turn — and the replay's allow-list recheck narrows that to ids the agent knows, which is not a boundary. Read matters as much as write: an entry holds the verbatim text of a message the operator sent.
Done, both halves:
Directory-scoped in both, deliberately, and for the reason the Pinned by two tests: |
|
Replay bypasses current route authorization — span=582f20a4331e — fixed in Legitimate on both halves, and the adjudication note named them precisely:
Done, and kept as two separate decisions with two separate owners, which the wording above already implies. For the re-dispatch tier, each transport's For the notice tier, the send is gated on The two stay apart on purpose, in both directions: an inbound turn is not authorized by an egress rule, and a channel whose Four tests: |
|
Concurrent refusal writes overwrite accepted messages — span=582f20a4331e — fixed in Legitimate, and I am fixing it rather than taking the pre-drafted override. The adjudication's FLAG reasoning is sound as far as it goes — the lost entry is ephemeral rescue state the design treats as resend-recoverable — but it undersells one thing: losing a refused message to a race is the exact defect this PR exists to remove, reappearing one layer up. Shipping a rescue buffer that drops messages when two arrive at once would be a poor trade for a lock, and the "deliberate lock-free choice" the note credits was about the happy path, which still takes no lock because it does no write.
Done exactly that. Red-before proven: with the lock removed and a 10 ms window inserted between the read and the write, |
|
Replay changes the accepted message's semantics — span=b07e8c1e84bc — fixed in Legitimate on both halves, and the first one was a comment that was simply false: the code claimed the body was replayed as content while
Both done. On command mode: the replay now reaches On dropped media: an entry with One note on scope, since the finding's line reference points at Telegram: the same rule now covers every channel, because the media decision lives in the shared replay driver rather than per transport. |
Correct, and the mechanism you named is exactly right:
So the comment described behaviour the call site was structurally unable to produce. Fixed by routing the replay through the transport's Pinned at the dispatcher rather than at the outcome: |
Correct, and the load-bearing half of it is the clause about who else runs the gate:
That is what made this unrecoverable by delegation — there was no downstream frame that would have caught it.
|
139916b to
3a30680
Compare
|
Live polling can overtake replay — span=e51cd2330ac2 — fixed in The consequence you named is exactly right, and I had it backwards in my own head until I traced it: I assumed the replay landed on the spooled session key, so the worst case was answering into the old conversation. It does not. The replay reaches the turn through
This part I am pushing back on, and fixing the substance a different way. Gating inbound delivery on the backlog being dispatched puts the boot path behind a full agent turn — context build, model stream, tool approvals — which is the unbounded wait issue #2217 rules out in its own "why the obvious fix does not work" section, and it is what the AUTOSDE What actually goes wrong is narrower than the ordering: it is that a replay can land in a conversation that is no longer the one the message came from. So each transport now compares the session key it would derive now against the one recorded at refusal time ( Three tests: |
|
Shutdown cancellation permanently loses the claimed message — span=582f20a4331e — fixed in Legitimate: I added the
Took the first half, not the requeue. The claim and the dispatch of one entry are now a single coroutine ( Red-before: with the |
|
Replay bypasses WhatsApp group revocation — span=729ff9994e84 — fixed in Legitimate. WhatsApp inherited the base
Done. Three tests: |
Correct, and the trace is exactly right: a non-admitted member's uncaptioned photo in a rules-mode group takes the
Dropped the argument entirely. A route declares its text; empty means media-only (spooled for the notice with a nonzero |
|
First Principles Review 🟡 CONCERNS — two subtractions taken, one rebutted. Move Prune Shrink |
|
Disposition of the GPT 5.6 findings on The three blocking findings all sat on the re-dispatch half, and each round's fix to that half produced the next round's blocker (F1 here is the fix for round 9's shutdown-cancel finding). Rather than add a fourth mechanism, this head removes re-dispatch entirely and makes the replay notice-only, so the spool is a proactive send behind the one egress gate the codebase already owns ( Shielded replay escapes shutdown ( Unconfirmed notice is discarded ( Configured WhatsApp groups lose replay notices ( Weixin |
|
Disposition of the Opus 4.8 finding on Discord |
|
Disposition of the First Principles review on Slack, the motivation's headline loss, ships nothing — acknowledged, deferred to #8912, and the body now says so plainly. This head also shrinks the surface Slack would have to adopt: with re-dispatch removed, adopting a channel is one Drop the Scope change this head makes, for the record: the replay is now notice-only. The re-dispatch half was regenerating a blocking finding per round because it made the spool a second intake path that had to re-derive every channel's inbound gate; a notice is a send behind the one egress gate that already exists. Re-dispatch and WhatsApp group routes are filed as #9144. |
|
Disposition of the GPT 5.6 and First Principles findings on Unsigned pre-upgrade spool records are trusted (
|
|
Disposition of the First Principles review on
Drop Slack unadopted — unchanged: declared and deferred to #8912, as before. |
|
Disposition of the GPT 5.6 and First Principles findings on Failed removal causes a notice storm ( Shrink Replace |
|
Disposition of the GPT 5.6 finding on WhatsApp media spools a transformed prompt ( |
|
Disposition of the GPT 5.6 finding on Discord thread routes inherit DM authorization ( |
|
Disposition of the GPT 5.6 finding on Cancellation can strand the durable write past shutdown ( |
|
Disposition of the GPT 5.6 findings on Restricted messages are persisted to the spool ( Refusal persistence blocks the event loop ( Note on CI: |
|
Disposition of the GPT 5.6 findings on Unauthenticated pre-upgrade spool entries are trusted ( Cancellation leaves the spool write untracked ( The code has converged: rounds 15–17 each fixed a real finding (thread principal, incognito persistence, blocking write), and round 18 regenerates only these two previously-dispositioned fenced residuals. Opus and First Principles are green. |
|
Head The "actively misleading" reply still fires at refusal time (First Principles, Shrink |
|
Disposition of the GPT 5.6 findings on Confirmed sends can silently discard the notice tail ( Cancellation can orphan the refusal write ( |
|
Disposition of the GPT 5.6 findings on Replay bypasses the live governance ceiling ( Cancellation can outlive the durable write ( |
|
Disposition of the Opus 4.8 finding on Revoked-route drop makes an egress permission decision with no SEL audit ( Unsigned pre-upgrade spool records are trusted ( |
|
Disposition of the GPT 5.6 findings on Allowed recipient authorization is not SEL-audited ( Function-local "verbatim" contradicts display-safe + truncation ( |
…d notice them Gateway shutdown gathers channel teardown and SessionManager.close_all() concurrently, so a message the platform has already accepted can be refused by the _closing gate before its turn ever opens. Nothing retried it: the payload was discarded and the user was answered with the channel's generic fault notice (or, on Slack, with silence). The platform said "delivered" and nothing ever answered. Adds a durable inbound spool written ONLY at the refusal point. On the next start the sender is told, in that same conversation, that the message was never processed, quoted back so a resend is one tap. It is deliberately NOT re-driven as a turn. Why notice-only. A re-dispatch half was built and removed during review. Replaying an entry as the operator's own turn makes the spool a second INTAKE path into the model, and every authorization the live path applies at intake -- the peer allow-list, Telegram's forum gate, Discord's thread roster, WhatsApp's group gate, conversation rotation, command interpretation -- has to be re-established on it per channel and kept in step forever; ten review rounds re-derived that surface one gate at a time, and in production Discord's transport has no dispatcher reference at all, so its replay gates were dead code. A notice is a proactive SEND, and a proactive send already has exactly one authorization seam in this codebase, MessagingTransport.may_send_to. Scoping the replay to the notice puts the whole feature behind a gate that already exists and is already owned. Re-dispatch, if wanted, is a separate design owned by the channel dispatch wiring (#9144). Writing only at the refusal point is what makes this small enough to be correct, and it settles four of the five design questions on the issue rather than answering them: ack semantics dissolve (nothing is written on the happy path), nobody is told to resend a message that was answered (a completed turn was never written), the happy path costs zero writes, and platform redelivery is not required (the pass reads our own disk). New shared primitive, src/kiro_crew/messaging/inbound_spool.py: * Combines a COUNT cap with an AGE horizon, which nothing in the tree did. A crash-loop through shutdown would otherwise accumulate a notice storm. Per-entry text is capped too, and truncation is marked rather than silent. * The notice pass is AT-LEAST-ONCE, one entry at a time. peek_next returns the oldest entry WITHOUT removing it; remove_entry runs only after the send is confirmed (a non-empty message id, or any return on a transport whose capabilities.returns_message_id is False -- WeCom and Feishu return "" on success and raise on failure). An unconfirmed send leaves the entry for the next start and the loop moves on, so one dead route cannot park the queue. This direction is safe precisely because the only action is a notice: a crash between the send and the removal costs one repeated line, never a repeated side effect, the opposite of the tradeoff a re-dispatch must make. * The channels governance ceiling is asked before every notice through the same audited vet_and_audit seam every other proactive-send site uses; a denied channel is held, not noticed. * may_send_to(conversation_id, thread_id, principal=) is re-decided at send time. A spooled entry is not a standing grant; a route revoked while the gateway was down is dropped, notice included, and a transport with no gate or one that raises is read as revoked. The principal is passed for a DM route only: a threaded route is authorized by the thread roster alone, because Discord's gate falls from an unknown thread to the DM allow-list on the assumption that a thread route names no principal. * Once per entry per pass: an entry attempted and left on disk (unconfirmed, or noticed but removal failed) is not handed back to the same pass, so an unwritable spool costs one notice per entry, not SPOOL_MAX_ENTRIES. * An entry whose channel is not connected THIS run is left on disk untouched (a startup blip is not the operator disabling the channel); the age horizon bounds it. * The refusal write is off-loop (file lock + disk I/O) and shielded from the caller's cancel: the handler that reached the refusal is a task close_all is about to cancel, and a bare await there would orphan the write. * Not for a restricted session: Telegram and Discord skip the spool for an incognito/temporary conversation, which promised to persist nothing, using the same predicate that gates their durable-history write. * The whole read-modify-write is serialized by a file lock on a dedicated lock file; a read failure raises SpoolUnreadable rather than reading as empty, because every writer rewrites from what it read and the reader unlinks an empty file. Removal is always the atomic replace, never a bare unlink. * A double-spool of the same message collapses on the platform message id ONLY. A body digest would collapse two identical messages on a channel with no id, which is data loss (repeating yourself is ordinary). * Never raises. Written while close_all is already running, so a full disk degrades to today's loss; read on the boot path, so a hand-edited file skips the bad record rather than costing the gateway its start. The spool is an OUTBOUND SOURCE, so it is fenced as a trust boundary. Each entry names a conversation and carries text the notice quotes verbatim, so a file an agent could write is a way to post text of its choosing, as the gateway, into any conversation still authorized for the principal it names -- and an entry holds the verbatim text of a message the operator sent. It lives in its own `inbound-spool` directory, added to security._CREW_SECRET_LEAVES (agent file tools) and sandbox._CREW_HIDDEN_LEAVES (spawned commands); a link planted at the directory, the leaf or the lock is refused before any read, write or unlink; reads open O_NOFOLLOW and fstat for a plain single-linked file; every write is atomic_write(restrict_to_owner=True). Adoption is opt-in per channel via ChannelTurn.inbound_route. The route is declared at the channel's dispatch site because ChannelTurn.conversation_id is a session-attribution id, not a reply target. InboundRoute.text is the message the USER sent, never the turn's prompt and with no fallback to it (WhatsApp's rules mode prepends the group's private rules to the prompt, and the notice quotes the entry). Weixin and WhatsApp capture text and the attachment count BEFORE ingestion (WhatsApp in receive, via a pending_original side table; no fallback to the ingested envelope). A route is declared only where may_send_to can express revocation for it: WhatsApp's answers from dm_policy alone and knows nothing of the group roster, so WhatsApp spools DMs only (#9144). Adopted: Telegram, Discord (with the thread), Weixin, WhatsApp DMs. Remaining channels in #8912; attachments are not carried over (#8911), the notice says so. Refs #2217
|
Head Cancellation can still lose the refused message ( |
cixuuz
left a comment
There was a problem hiding this comment.
Two durability/security gaps remain:
-
record_refusal()shields theto_threadfuture but immediately re-raisesCancelledErrorwithout retaining and awaiting/draining the underlying task. The write therefore becomes unowned during the shutdown race this code is meant to close. Create the task explicitly and drain it before re-raising cancellation. -
record_refusal_sync()and_spool_lock()callmkdir()before the parent/link checks. A pre-planted linked/junctioninbound-spoolparent can be traversed before refusal. Create/open the parent and lock through pinned/no-follow operations, then validate the opened objects.
bolichen97
left a comment
There was a problem hiding this comment.
Tech Lead review — approved.
I verified the three risk areas at f18efe33c405691250c30aecd896dff1eb3c1bf1 rather than from the description.
Duplication on drain. Cannot happen against a fresh inbound message: nothing writes the spool on the happy path, so a live message has no entry to collide with. Every read-modify-write (record_refusal_sync, peek_next, remove_entry) is serialized by _spool_lock on a dedicated lock file — correct, since the spool itself is replaced by rename and a lock on the old inode would not exclude a writer holding the new one. remove_entry matches trace_id and removes exactly one occurrence, so an id-less twin survives. Repeat notices are at-least-once by construction and cost one repeated line, never a repeated turn.
Loss on crash with undrained entries. An entry leaves disk only after delivery_confirmed, or on a revoked route. _read_entries raises SpoolUnreadable instead of collapsing to [], so a transient EIO cannot let the next rewrite erase the queue; removal is always the atomic replace, never a bare unlink. Residual is os._exit landing mid-write of the one message being written, and the 24 h horizon expiring an entry unnoticed — both documented, both bounded.
Bounds. 128 entries newest-wins, 24 h horizon, 16 KiB per entry, applied on write and again on read, and the replay loop is bounded by the same count cap. A crash-loop through shutdown cannot build a notice storm.
Ordering. _read_entries is oldest-first, peek_next returns the oldest actionable entry, _prune slices from the tail. Notices are delivered in arrival order.
On @cixuuz's two points — I read both at source and neither is a defect:
-
record_refusalre-raisingCancelledErrorwithout draining the shielded task is the intended shape.asyncio.to_threaddispatches the write to a non-daemon executor thread that runs to completion independently of the loop and of this coroutine, andrecord_refusal_syncnever raises, so there is no unretrieved-exception path either. Retaining and awaiting it before re-raising would blockclose_all's cancellation on disk I/O inside the shutdown deadline this feature is racing — the cost the shield exists to avoid.test_a_cancelled_refusal_handler_still_lands_its_writepins the behaviour. -
mkdir()ahead of_refuse_links()creates, reads and writes nothing through a pre-planted link:mkdir(parents=True, exist_ok=True)on a symlink to an existing directory is a no-op stat, and on a dangling link it raisesFileExistsErrorinto the best-effortexcept._refuse_linksthen runs before the lock'sO_NOFOLLOWopen and before every read, write and unlink, and the readsfstatthe opened descriptor for a plain single-linked regular file. Hoisting the check above themkdirwould read more cleanly, but there is no window here to exploit — worth a follow-up nit, not a block.
Scope is proportionate: of +2899, 1444 is the test file and 1001 the new module; the production surface outside it is +255/-4 across ten files, almost all opt-in inbound_route declarations. Owning specs updated in the same commit. 63 checks green (55 success, 8 skipped, 0 failures); Design PASS, GPT 5.6 and Opus 4.8 no findings, First Principles CONCERNS advisory on the six unadopted channels, which #8912 tracks.
Merging. Note that #2217 stays open by design — this lands the notice half for four channels.
Problem / Motivation
Gateway shutdown gathers channel teardown and
SessionManager.close_all()concurrently, so a message the platform has already accepted can be refused by the_closinggate before its turn ever opens. Nothing retries it: the payload is discarded and the user is answered with the channel's generic "please try again" (or, on Slack, with silence). The platform says delivered and nothing ever answers.The reorder that looks like the fix does not work, and the issue's own investigation explains why: a channel's
close()awaits its polling task and HTTP session, not the handler tasks it spawned, so it returns while the turn is still pending. Making it actually drain would mean awaiting a complete agent turn, which is unbounded.Why it matters
Every channel, every inbound message, no attachments needed. A user asks a question during a restart and it silently never happens, and the notice they do get on most channels is actively misleading, since it reads as a fault in their message rather than as a restart. Restarts are routine (updates, config reloads, crashes), so this is a recurring silent data loss on the product's own inbound path.
What changed (motivation → approach → change)
Symptom → a refused turn's payload is unrecoverable because the
except SessionClosingErrorbranch is the last frame holding it.Root cause → nothing persists an inbound message, and the refusal point is the only place that both still holds the payload and knows for certain the turn never opened.
Change → a durable inbound spool written only at the refusal point; on the next start the sender is told, in that same conversation, that the message was never processed, quoted back so a resend is one tap.
It is deliberately not re-driven as a turn. See "Why notice-only" below.
Writing only at the refusal point (the narrowing @chenmingwei23 arrived at on the issue) is what makes this small enough to be correct, and it settles four of the five open design questions rather than answering them:
Why notice-only
A re-dispatch half was built and removed during review, and the reason is structural rather than a matter of polish. Replaying an entry as the operator's own turn makes the spool a second intake path into the model, and every authorization the live path applies at intake has to be re-established on it, per channel, and kept in step forever: the peer allow-list, Telegram's forum gate, Discord's thread roster, WhatsApp's group gate, conversation rotation, command interpretation. Ten review rounds re-derived that surface one gate at a time, and the last one found that in production Discord wires
dispatch=_dispatchas a nested function sotransport.dispatcherisNone: every Discord replay gate was dead code.A notice is a proactive send, and a proactive send already has exactly one authorization seam in this codebase:
MessagingTransport.may_send_to. Scoping the replay to the notice puts the whole feature behind a gate that already exists and is already owned, instead of introducing a parallel one. Re-dispatch, if wanted, is a separate design owned by the channel dispatch wiring, filed as #9144.The shared primitive:
src/kiro_crew/messaging/inbound_spool.pyjsonl_util.rotate_jsonl_atgives cap-on-append, the spec-builder tombstones give slice-on-write, the subagent sweep gives an age horizon. Without the pair, a gateway that crash-loops through shutdown accumulates a notice storm for the next start. Per-entry text is capped too, and truncation is marked in the body rather than silent. Both bounds are applied on write and again on read.peek_nextreturns the oldest entry without removing it;remove_entryruns only after the send is confirmed. Confirmation is a non-empty message id, or any return at all on a transport whosecapabilities.returns_message_idisFalse(WeCom and Feishu return""on success and raise on failure; reading that as unconfirmed would re-notice forever). An unconfirmed send (a raise, an empty id) leaves the entry for the next start and the loop moves on, so one dead route cannot park the queue. An entry the pass attempted and left on disk is never handed back to that pass, so a spool that has become unwritable (removal fails after the notice landed) costs one notice per entry, not one per loop iteration. This direction is safe precisely because the only action is a notice: a crash between the send and the removal costs one repeated line, never a repeated side effect, which is the opposite of the tradeoff a re-dispatch must make.channelsgovernance ceiling is asked before every notice, throughvet_and_audit("channels", channel_type, tool_name="inbound_spool.notice", fail_closed=True), the same audited seam the dashboard'schannel.send_messageand the cron fallback legs use. A channel the operator denied while the gateway was down is HELD rather than noticed or dropped: the route is not revoked, the channel is governed off, and the horizon bounds it. A degraded evaluation denies.may_send_to(conversation_id, thread_id, principal=)is re-decided at send time. A spooled entry is not a standing grant; the peer may have left the roster or the thread been revoked while the gateway was down. A revoked route is dropped, notice included, and themay_send_todecision is SEL-audited both ways aschannel.proactive_send_authorize/allowedordenied, the same record the cross-surface proactive send writes (a denial also deletes a stored message; a grant is what puts the gateway's own text into a conversation). A transport with no gate, or one that raises, is read as revoked. The principal is passed for a DM route only: a threaded route (Discord thread, Telegram Topic) is authorized by the thread roster alone, because Discord's gate falls from an unknown thread toprincipal in _allowedon the assumption that a thread route names no principal, and a spooled thread entry names the sender, so passing it would let a still-allowed sender authorize a notice into a revoked thread.to_threadwriters, and the boot pass reads and removes in workers of its own; without the lock they read the same snapshot and the second atomic replace drops the first.asyncio.to_threadrather than on a loop that is racing a shutdown deadline. But the handler that reached the refusal is a taskclose_allis about to cancel, and a bareawaitthere is a cancellation point that would orphan the write;asyncio.shieldkeeps the two apart, so the caller is cancelled and the write is not. An executor already shut down falls back to an inline write rather than dropping the message./incognitoand/temporaryare a promise that the conversation persists nothing, and the spool is a durable file holding the message verbatim. Telegram and Discord ask their own_session_restricted(session_key)(the predicate that already gates the durable-history write) at the refusal point and skip the spool; the message degrades to the pre-feature loss, which is what the user asked for by choosing the mode. Weixin and WhatsApp have no privacy mode.SpoolUnreadablerather than reading as empty: every writer rewrites from what it read and the reader unlinks an empty file, so a transient EIO would otherwise erase the queue. Removal is always the atomic replace, never a bare unlink, which fails routinely on Windows under an AV handle and would re-notice the entry on every start.trace_id), and removal is by one occurrence of it.close_allis already running, so a full disk degrades to today's loss instead of becoming the thing that fails shutdown; and it is read on the boot path, so a hand-edited file skips the bad record rather than costing the gateway its start.The spool is an outbound source, so it is fenced as a trust boundary
Each entry names a conversation and carries text the notice quotes verbatim, so a file an agent could write is a way to post text of its choosing, as the gateway, into any conversation still authorized for the principal it names; and an entry holds the verbatim text of a message the operator sent, so read matters as much as write. The spool therefore lives in its own
inbound-spooldirectory and is added tosecurity._CREW_SECRET_LEAVES(agent file tools) andsandbox._CREW_HIDDEN_LEAVES(spawned commands), directory-scoped because the spool is written by atomic replace through a sibling temp and the lock file beside it is what serializes writers. The fence only holds from the build that ships it, so a link planted before it, at the directory, the leaf or the lock, is refused before any lock, read, write or unlink. Reads openO_NOFOLLOWandfstatthe descriptor for a plain single-linked regular file; every write isatomic_write(restrict_to_owner=True). The quoted body passes throughdisplay_safe_for, so a broadcast mention in the original cannot fire when echoed, and is sized to the transport'smax_message_charswith a visible truncation mark: the notice prefixes the quote, so a message that fit the cap on the way in may not fit now, and a transport that slices to its cap while still returning an id would otherwise confirm a notice whose tail was silently cut. It is not chunked into several messages; the quote is an echo of text the sender still holds and is told to resend.Adoption is opt-in per channel
Via the new
ChannelTurn.inbound_route. A channel that has not declared its reply target is byte-identical to before this PR. The route cannot be derived fromChannelTurn.conversation_id, which is a session attribution id ("weixin:{user}") and is not addressable bysend_message, so each channel declares its own address at the one place holding the normalized envelope.What gets spooled is the message the user sent, never the prompt the turn assembled from it, and there is no fallback to the prompt. WhatsApp's rules mode prepends the group's private operating rules to the model prompt, and the notice quotes the entry; an earlier fallback is exactly how a media-only rules-mode message came to spool the rules. Weixin captures text and the attachment count before ingestion, because ingestion rewrites the text with turn-owned temp paths and clears
inbound.attachments. WhatsApp'sreceivedoes the same into apending_originalside table (keyed likepending_verdicts), and the dispatcher declares no route at all when that entry is absent rather than fall back to the ingestedinbound.text.A route is declared only where
may_send_tocan express revocation for it. Discord's answers from_allowed_threads, so it declares the thread. WhatsApp's answers fromdm_policyalone and knows nothing of the group roster, so a group removed or set tooffwhile the gateway was down would still receive the notice; WhatsApp therefore spools DMs only, and a refused group message degrades exactly as before this seam (#9144).spool_refused_turn(channel_type=, route=)reads the text from the route itself; there is no separate text argument that could diverge from it. Write sites are the refusal points the investigation named: the shared pipeline inmessaging/dispatch.py(covering every adopter), plus Telegram's and Discord's own dispatchers. Discord spools a user message only; a monitor turn's own loop re-fires after the restart.Adopted: Telegram, Discord (with the thread), Weixin, WhatsApp DMs.
What this deliberately does not do
That is why this is
Refs #2217, notCloses.Tests
test/test_inbound_spool.py, 69 tests, plus 3 intest_whatsapp_dispatch.py, 2 intest_whatsapp_transport.py, 2 intest_telegram.pyand 1 intest_discord.py.Red-before, proven by reverting the production change with the tests untouched:
test_a_refused_turn_is_spooled_with_its_routing: with the spool call removed fromdrive_turn'sexcept SessionClosingErrorbranch it fails ona refused message left no durable trace. That assertion is the loss.test_a_confirmed_notice_removes_the_entry: with the pass unwired the spooled message is never answered and the entry never leaves disk.The notice pass: a confirmed notice goes to the right conversation and thread, quotes the message, names the restart, and removes the entry; a dropped attachment and a media-only entry are named in the notice; an unconfirmed send (raise or empty id) keeps the entry and it is noticed on the next start; one dead route does not block the rest; a
returns_message_id=Falsetransport is confirmed on""; a crash after the send and before the removal re-notices rather than losing; a crash on the first entry keeps the rest; a failed removal after a landed notice is reported unconfirmed and noticed once per pass, not once per iteration (red-before-proven); a second pass has nothing to do; entries are noticed oldest first; the quoted body is display-safe (a@channelis defanged); an over-cap quote is truncated visibly under the transport cap rather than sliced by the transport (red-before-proven), and a short quote is untouched; the pass is bounded by the count cap even when nothing confirms.Holding: an unconnected channel's entry stays on disk, never leaves disk during a live send (observed from inside
send_message), keeps its arrival order, is answered when the channel is back, and still expires on the horizon.Authorization at send: a channel denied by the
channelsgovernance profile gets no notice and is held, with the audited seam called once per entry (red-before-proven); a governance evaluation failure denies; the notice is withheld from a revoked conversation and the entry removed (asserting theprincipalis passed), and both the denial and the grant are SEL-audited with the cross-surface record shape (red-before-proven); a transport with no egress gate is refused; a raising gate reads as revoked; a threaded route is gated with no principal while a DM route carries it; against the REALDiscordTransport.may_send_to, a revoked thread gets no notice even from a still-allowed sender (red-before-proven).Dedupe and bounds: the same message spooled three times yields one entry when the platform supplied an id; two identical id-less bodies both survive and are each noticed once; the count cap keeps the newest; the horizon drops a stale entry on read and does not let it survive a later write; an over-cap body is truncated and says so.
Store primitives:
peek_nextreturns the oldest actionable entry without removing it, reports held (unconnected-channel) entries alongside, and skips a trace id the caller has already seen;remove_entrytakes exactly one and is a no-op the second time; the file is removed once drained; a failed unlink of the last entry does not resurrect it (red-before-proven against a bare-unlink removal).Trust boundary (red-before-proven): a symlinked spool directory, a symlinked leaf and a hard-linked leaf are each refused for read and write, the forged record behind them is never read, and the link's target is never mutated; every rewrite in
record_refusal_sync,peek_nextandremove_entryisrestrict_to_owner=True; the directory is in_CREW_SECRET_LEAVESand_CREW_HIDDEN_LEAVESand the spool actually lives in it.Robustness: a cancelled refusal handler still lands its write (the caller is cancelled mid-write, the shielded write completes); an unreadable spool is left untouched and refuses a write rather than overwriting; a write into an unwritable path returns
False; a corrupt line is skipped; an entry with no reply target is refused at parse; a transport fault never escapes the pass; an entry persists exactly the keys the notice reads (nosession_key/chat_typeriding along for a removed design) and a record from the earlier shape still parses.Channel wiring: a Telegram or Discord refusal in a restricted (incognito/temporary) session is NOT spooled while a persistent one is (red-before-proven at both dispatchers); the spooled text is the user message, not the model prompt, with no fallback (the media-only rules-mode case spools no rules); Weixin's
_drivetakes the pre-ingestion originals and its route text has no fallback to the ingested prompt; WhatsApp'sreceivecaptures the original caption and media count before ingestion (red-before-proven through the realreceivewith a fake ingest), the dispatcher's DM route reads them and isNonewithout them, and a group route is never declared; no transport still carries areplay_inboundhook, and__all__is exactlyInboundRoute,replay_spooled,spool_refused_turn.The owning specs are updated in the same commit: the Durable inbound spool section in
docs/system-specs/modules/messaging.mdand the trust-boundary entry insecurity.md.Regression suites run (all green, 3425 tests):
test_inbound_spool,test_messaging_dispatch,test_telegram,test_telegram_sessions,test_telegram_album,test_discord,test_discord_sessions,test_weixin_dispatch,test_whatsapp_dispatch,test_whatsapp_client,test_capability_ledger,test_channel_transport_outbound_authz,test_teams_dispatch,test_webex_dispatch,test_wecom_dispatch,test_imessage_dispatch,test_feishu_dispatch,test_session_drain,test_slack_gateway_coverage,test_security. Local gates green: black, isort, flake8, mypy,check_black_formatting.py,docs_lint.py.Manual verification
N/A: unit coverage sufficient. The behaviour under test is a shutdown race and a boot-path send, both driven deterministically here (the closing gate is invoked at the real seam, and the spool is exercised through its real file paths); reproducing them by hand means racing a live gateway's teardown, which is less reliable evidence than the tests, not more.
Related Issues
Refs #2217
no linked issue: deliberate. This lands the notice half of #2217 for four channels; attachments, six channels, re-dispatch and WhatsApp groups remain, so #2217 must stay open after merge.
Refsrather thanClosesis the accurate trailer here, not a forgotten one.Follow-ups filed for the deliberate gaps:
Pattern harvest
Pattern: a terminal error branch that discards an in-memory payload it is the last holder of. The shape is a handler catching a "this cannot proceed" signal and finalizing the user-visible surface without persisting the input, so the payload is unrecoverable by construction rather than by a bug in any later step.
A second pattern fell out of the ten review rounds and is the reason this PR converged on the shape it has: a persisted payload that is later replayed as a turn creates a second intake path, and every intake gate has to be re-derived on it. The store then needs a credential's fences even though it holds no credential, needs each channel's complete inbound authorization re-applied per channel, and needs those to stay in step with the live path forever. Scoping the replay to a proactive send collapses that to the one egress gate the codebase already owns.
Rule candidate: review-prompt. When an
exceptbranch is the last frame holding an unpersisted user input, ask what recovers it. When a new on-disk store is replayed into the model, ask whether the replay can be expressed as a send through an existing authorization seam instead of as a turn through a new one.Checklist