Skip to content

feat(messaging): spool inbound messages the shutdown gate refuses, and notice them - #8913

Merged
bolichen97 merged 1 commit into
mainfrom
fix/inbound-shutdown-spool-2217
Sep 8, 2026
Merged

feat(messaging): spool inbound messages the shutdown gate refuses, and notice them#8913
bolichen97 merged 1 commit into
mainfrom
fix/inbound-shutdown-spool-2217

Conversation

@iamwhatever

@iamwhatever iamwhatever commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

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 _closing gate 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 SessionClosingError branch 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:

  • Ack semantics dissolve: nothing is written on the happy path, so there is no general "what counts as handled" protocol to design. Every entry that exists is a turn provably refused before it opened.
  • Nobody is told to resend an answered message: a turn that completed just before exit was never written.
  • Cost is zero when nothing is wrong: no write, no lock, no fsync on the path every message takes.
  • Platform redelivery is not required: the pass reads our own disk. That matters because, as the issue's own correction establishes, only Webex genuinely defers its ack; the other nine channels ack before the turn runs.

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=_dispatch as a nested function so transport.dispatcher is None: 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.py

  • Combines a count cap with an age horizon, which nothing in the tree did: jsonl_util.rotate_jsonl_at gives 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.
  • 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. Confirmation is a non-empty message id, or any return at all on a transport whose capabilities.returns_message_id is False (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.
  • The channels governance ceiling is asked before every notice, through vet_and_audit("channels", channel_type, tool_name="inbound_spool.notice", fail_closed=True), the same audited seam the dashboard's channel.send_message and 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 the may_send_to decision is SEL-audited both ways as channel.proactive_send_authorize / allowed or denied, 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 to principal in _allowed on 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.
  • 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 still bounds it.
  • The whole read-modify-write is serialized by a file lock on a dedicated lock file (the spool itself is replaced by rename, so a lock on the old inode would not exclude a writer that opened the new one). Two messages refused in the same shutdown are two concurrent to_thread writers, 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.
  • The refusal write is off-loop and shielded from the caller's cancel. It takes a file lock and does disk I/O, so it belongs in asyncio.to_thread rather than on a loop that is racing a shutdown deadline. But the handler that reached the refusal is a task close_all is about to cancel, and a bare await there is a cancellation point that would orphan the write; asyncio.shield keeps 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.
  • Not for a restricted session. /incognito and /temporary are 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.
  • A read failure raises SpoolUnreadable rather 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.
  • A double-spool collapses on the platform message id only. A body digest looks like the obvious fallback for a channel that has none, and is a data-loss bug: repeating yourself is ordinary, and two identical messages hash the same, so one accepted message would be silently discarded. The digest survives only as a log label (trace_id), and removal is by one occurrence of it.
  • Never raises. It is written while close_all is 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-spool directory and is added to security._CREW_SECRET_LEAVES (agent file tools) and sandbox._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 open O_NOFOLLOW and fstat the descriptor for a plain single-linked regular file; every write is atomic_write(restrict_to_owner=True). The quoted body passes through display_safe_for, so a broadcast mention in the original cannot fire when echoed, and is sized to the transport's max_message_chars with 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 from ChannelTurn.conversation_id, which is a session attribution id ("weixin:{user}") and is not addressable by send_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's receive does the same into a pending_original side table (keyed like pending_verdicts), and the dispatcher declares no route at all when that entry is absent rather than fall back to the ingested inbound.text.

A route is declared only where may_send_to can express revocation for it. Discord's answers from _allowed_threads, so it declares the thread. WhatsApp's answers from dm_policy alone and knows nothing of the group roster, so a group removed or set to off while 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 in messaging/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, not Closes.

Tests

test/test_inbound_spool.py, 69 tests, plus 3 in test_whatsapp_dispatch.py, 2 in test_whatsapp_transport.py, 2 in test_telegram.py and 1 in test_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 from drive_turn's except SessionClosingError branch it fails on a 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=False transport 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 @channel is 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 channels governance 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 the principal is 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 REAL DiscordTransport.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_next returns the oldest actionable entry without removing it, reports held (unconnected-channel) entries alongside, and skips a trace id the caller has already seen; remove_entry takes 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_next and remove_entry is restrict_to_owner=True; the directory is in _CREW_SECRET_LEAVES and _CREW_HIDDEN_LEAVES and 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 (no session_key / chat_type riding 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 _drive takes the pre-ingestion originals and its route text has no fallback to the ingested prompt; WhatsApp's receive captures the original caption and media count before ingestion (red-before-proven through the real receive with a fake ingest), the dispatcher's DM route reads them and is None without them, and a group route is never declared; no transport still carries a replay_inbound hook, and __all__ is exactly InboundRoute, 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.md and the trust-boundary entry in security.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. Refs rather than Closes is 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 except branch 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

  • At most two commits (one is the norm), with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable): the module docstring carries the design and the tradeoffs
  • No secrets, credentials, or internal references in the diff

@iamwhatever
iamwhatever requested a review from a team as a code owner September 6, 2026 07:07
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5.1) — ✅ PASS

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

The design rests on real, verified seams (may_send_to exists on every transport with the exact signature the module uses; the Discord thread/DM principal-withholding reasoning matches discord/transport.py). Docs are updated in the same commit, the trust boundary is fenced in both _CREW_SECRET_LEAVES and sandbox._CREW_HIDDEN_LEAVES, and every design question the PR raises is settled from the diff.

Design-Verdict: PASS

Notice-only replay behind the existing may_send_to/vet_and_audit seams, opt-in per channel, best-effort at every fallible point — a sound, proportionate close of a real accepted-then-dropped loss.

[DESIGN-REVIEWED] f18efe3

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5.1) — 🟡 CONCERNS

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

I've verified the load-bearing claims. Let me finalize.

  • delivery_confirmed (transport.py:208) and may_send_to (all 11 transports) pre-exist — the notice reuses existing seams, not second spellings.
  • No prior inbound-persistence/redelivery mechanism exists (spool appears only in the new code) — not a duplicate.
  • inbound_route: 1 consumer (drive_turn), 4 producers — genuine multi-channel use, not speculative.
  • Channels total 10; adopted 4 (Telegram, Discord, Weixin, WhatsApp DMs); 6 unadopted (Slack, Teams, Webex, WeCom, iMessage, Feishu), tracked in Adopt the inbound shutdown spool on the remaining channels (Slack, Teams, Webex, WeCom, iMessage, Feishu) #8912.
  • The speculative session_key/chat_type fields and the re-dispatch half were already removed (pinned by tests) — no zero-consumer surface remains.

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 ships

Intent: 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.

  1. Refused-during-shutdown message is spooled and the sender gets a "was restarting, resend it" notice next start — justified (the fix, cause-level: persisted at the one point holding the payload).
  2. New persisted inbound-spool/refused.jsonl state — justified.
  3. Boot-time proactive notice re-authorized via may_send_to + channels governance — justified (egress/network boundary, reuses existing seams).
  4. Telegram adopts at refusal point — justified.
  5. Discord adopts, incl. thread route — justified.
  6. Weixin adopts, captures pre-ingestion text/count — justified.
  7. WhatsApp DMs adopt; groups deferred Inbound spool follow-up: re-dispatch as a turn, and WhatsApp group routes #9144 — justified.
  8. Restricted (incognito/temporary) sessions skip the spool — justified (persistence promise).
  9. Count cap + age horizon + text cap — justified (crash-loop storm).
  10. inbound-spool fenced on _CREW_SECRET_LEAVES + _CREW_HIDDEN_LEAVES — justified (keystone invariant).
    (More items exist — docs, dedupe key, shield write; list capped at the 10 most noticeable.)

Watch

[FIRST-PRINCIPLES-REVIEWED] f18efe3

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] f18efe3

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

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

CANDIDATE 1 dies at (a): the failure requires a transport whose max_message_chars is below ~135, and every adopted transport (Telegram 4000, Discord 1900, Weixin/WhatsApp 4096) — plus every other production transport — sets caps in the thousands. The tiny caps that exist (10, 60, 90) are all test-only fabrications, never a shipped transport the replay pass would reach. The condition does not occur in practice, so the sizing guarantee is not observably violated on any real path. No other candidate exists, and nothing new grounds to the 80+ bar.

No findings.

[OPUS-REVIEWED] f18efe3

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

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@iamwhatever
iamwhatever force-pushed the fix/inbound-shutdown-spool-2217 branch from d6e17d3 to 139916b Compare September 6, 2026 07:57
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Agent-writable spool permits forged operator messages — span=582f20a4331e — fixed in 139916b5a.

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.

Fix: Fence the entire spool directory from agent reads/writes and sandbox access.

Done, both halves:

  • The spool moved to its own inbound-spool directory under the crew home and is added to security._CREW_SECRET_LEAVES — the agent file-tool floor.
  • Added to sandbox._CREW_HIDDEN_LEAVES — masked in every agent sandbox, so a spawned command cannot reach it either.

Directory-scoped in both, deliberately, and for the reason the whatsapp and apps/aws-control/data entries already are: the spool is written by atomic replace and claimed by rename, both through a sibling name in the same directory, so fencing only the final leaf would leave a writable path to the same bytes — and the lock file beside it is what serializes two concurrent refusals. The gateway opens all of it directly rather than through those gates, so spooling and replay keep working.

Pinned by two tests: test_the_spool_directory_is_fenced_from_agent_file_tools (which also asserts the spool actually lives in the fenced directory rather than merely being named after it) and test_the_spool_directory_is_masked_in_agent_sandboxes.

@iamwhatever

iamwhatever commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Replay bypasses current route authorization — span=582f20a4331e — fixed in 139916b5a.

Legitimate on both halves, and the adjudication note named them precisely: authorize checks only the user allow-list, so Telegram's replay re-authorized the sender and never the Topic; and the fallback send_message authorized nothing at all.

Fix: Reapply each channel's complete live route gate, and require may_send_to(..., principal=entry.user_id) before fallback sends.

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 replay_inbound now re-applies its own complete inbound gate. Telegram additionally calls forum_gate_outcome with the same construction-time frozen allow-list receive passes, so a replay cannot be admitted by a gate the live path would refuse. handle_message does not re-run that gate either, which is why it belongs at the replay rather than being left to the dispatcher.

For the notice tier, the send is gated on may_send_to(conversation_id, thread_id, principal=entry.user_id), and it fails closed on a transport that cannot answer, since a spool entry is the one input in this path that did not arrive from the platform this run.

The two stay apart on purpose, in both directions: an inbound turn is not authorized by an egress rule, and a channel whose may_send_to is narrower than its inbound policy must not have legitimate replays silently dropped by it — Telegram is exactly that shape, since may_send_to answers a DM from self._allowed while its inbound policy also admits allow-listed forum Topics.

Four tests: test_telegram_replay_reapplies_the_forum_gate (a de-allow-listed Topic is refused and never dispatched), test_the_notice_is_withheld_from_a_revoked_conversation (asserting the principal is passed, or a transport that authorizes by peer cannot decide), test_a_transport_with_no_egress_gate_is_refused_the_notice, and test_the_egress_gate_does_not_block_a_re_dispatch.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Concurrent refusal writes overwrite accepted messages — span=582f20a4331e — fixed in 139916b5a.

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.

Fix: Serialize the complete read-modify-write transaction with a file lock.

Done exactly that. platform_compat.file_lock spans the read and the atomic replace in record_refusal_sync, and the same lock spans the claim in claim_spooled so a claim cannot land between a refusal's read and its replace. It is a dedicated lock file, not the spool itself: the spool is replaced by rename, so a lock held on the old inode would not exclude a writer that opened the new one. file_lock fails closed, and the module is best-effort throughout, so a lock that cannot be taken degrades the message to the pre-feature drop rather than to a torn file.

Red-before proven: with the lock removed and a 10 ms window inserted between the read and the write, test_two_concurrent_refusals_both_survive fails on a concurrent refusal overwrote another message; with the lock, 24 messages written from 8 threads all survive.

@iamwhatever

iamwhatever commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Replay changes the accepted message's semantics — span=b07e8c1e84bc — fixed in 139916b5a.

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 DispatchFn = Callable[[InboundMessage], Awaitable[None]] cannot carry the flag at all, so interpret_commands sat at its True default. Taking the override here would have shipped a lie in a comment next to the behaviour it misdescribes.

Fix: Replay Telegram/Discord with command interpretation disabled; preserve the original attachment count and use notice-only replay when media was dropped.

Both done.

On command mode: the replay now reaches handle_message(msg, interpret_commands=False) through the transport's dispatcher rather than through _dispatch, which is the only route with the real signature. Weixin gained the same parameter Teams, Webex, Telegram and Discord already had, so it is no longer the odd channel out. Tests assert the flag's value at the dispatcher for all three implementing channels, using a spooled /new and a spooled !stop as the bodies.

On dropped media: an entry with attachments_dropped > 0 is never re-dispatched and goes straight to the notice, which names the count. The reason is the sharper half of your finding — those channels inline the turn-owned temp paths into the prompt text, so re-driving that text after a restart hands the model paths to deleted files. test_an_entry_that_dropped_attachments_is_never_re_dispatched pins it.

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.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • The replay's "as CONTENT" comment contradicted the dispatch it performed (span=18d9f8cacbf0) — fixed in 139916b5a.

Correct, and the mechanism you named is exactly right:

DispatchFn = Callable[[InboundMessage], Awaitable[None]] cannot pass the flag

So the comment described behaviour the call site was structurally unable to produce. Fixed by routing the replay through the transport's dispatcher — the only reference with the real handle_message signature — and passing interpret_commands=False explicitly. Weixin gained the same parameter the other four channels already had, so the flag exists to be passed on every implementing channel rather than only where it happened to be declared.

Pinned at the dispatcher rather than at the outcome: test_telegram_replay_disables_command_interpretation (body /new), test_discord_replay_carries_the_thread_and_disables_commands (body !stop), test_weixin_replay_uses_the_peer_and_disables_commands, plus test_weixin_dispatch_honours_the_content_only_flag on the signature itself.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • replay_inbound never called forum_gate_outcome, the forum chat-type authZ boundary (span=18d9f8cacbf0) — fixed in 139916b5a.

Correct, and the load-bearing half of it is the clause about who else runs the gate:

the forum chat-type authZ boundary that both receive and on_callback enforce and that handle_message does not re-run

That is what made this unrecoverable by delegation — there was no downstream frame that would have caught it. replay_inbound now calls forum_gate_outcome itself, with the same construction-time frozen allow-list receive passes rather than live cfg, so a replay cannot be admitted by a gate the live inbound path would refuse. A refusal logs a SEL denied_forum_not_allowed under its own telegram_transport.replay_inbound operation, so a dropped replay is distinguishable in the audit trail from a dropped live message.

test_telegram_replay_reapplies_the_forum_gate builds exactly your case — a spooled supergroup Topic whose chat id is no longer allow-listed — and asserts the dispatcher is never reached.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026
@iamwhatever
iamwhatever force-pushed the fix/inbound-shutdown-spool-2217 branch from 139916b to 3a30680 Compare September 6, 2026 08:41
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Live polling can overtake replay — span=e51cd2330ac2 — fixed in 3a306809f, by a different mechanism than the one proposed.

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 handle_message, which derives the key from live ConversationState — so after a /new the pre-restart question is injected into the fresh conversation the user just started, which is your "old content enters the new conversation" verbatim. The spooled session_key was, until this commit, decorative.

Fix: Gate inbound polling until the backlog is claimed and dispatched.

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 no-new-work-on-gateway-boot-path rule exists to prevent (the same rule Opus checked this hunk against and cleared precisely because the work is detached).

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 (conversation_moved_on), and declines the re-dispatch on a mismatch. The entry falls through to the restart notice, which is the honest outcome — the user is told the earlier message was never processed and can resend it into whichever conversation they actually want it in. That closes the stated consequence without ordering the boot path behind a model turn, and it makes the spooled key load-bearing.

Three tests: test_a_rotated_conversation_is_not_re_dispatched_into, test_an_entry_with_no_recorded_key_is_treated_as_unmoved (nothing to compare must not mean refuse-everything), and test_telegram_replay_declines_a_rotated_conversation at the transport.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 6, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Shutdown cancellation permanently loses the claimed message — span=582f20a4331e — fixed in 35673847d.

Legitimate: I added the _shutdown cancel last round to bound the os._exit race and, in doing so, created a cleaner version of the same loss — a CancelledError landing between claim_next (entry already off disk) and _replay_one strands the entry with nothing to requeue it.

Fix: Make claiming cancellation-safe and requeue any claimed, undispatched entry before propagating cancellation.

Took the first half, not the requeue. The claim and the dispatch of one entry are now a single coroutine (_claim_and_replay) awaited under asyncio.shield, so a cancel can land only between entries: the in-flight one finishes its dispatch or notice, and the next iteration's claim never happens. A requeue-on-cancel would reintroduce the in-memory copy the previous round removed, and a finally does not run under os._exit anyway. The shielded work is bounded — one turn hand-off or one send — so the 1-second wait_for in _shutdown is the right budget: it returns after the hand-off, not after the model turn.

Red-before: with the shield removed, test_a_cancel_mid_replay_finishes_the_claimed_entry fails with the in-flight entry was dropped by the cancel (dispatched [] instead of ['message 0']); the test also asserts the unclaimed second entry is still on disk.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Replay bypasses WhatsApp group revocation — span=729ff9994e84 — fixed in 35673847d.

Legitimate. WhatsApp inherited the base replay_inbound (UNSUPPORTED → notice tier), and the notice tier's gate is may_send_to, which answers from dm_policy — under open that is True for every conversation. So a group the operator removed from the roster, or set to off, while the gateway was down still received the quoted notice. This is the WhatsApp instance of the same class as the Telegram forum and Discord thread findings: the channel's live inbound authority for a group route is not the peer policy.

Fix: Reapply WhatsApp's live group gate during replay and return REFUSED for removed or disabled groups.

Done. WhatsAppTransport.replay_inbound now re-runs group_gate.evaluate for a group JID (with the most permissive framing, so respond=False means the group is gone, not that this message would have been ignored) and returns REFUSED with a SEL denied_group_not_configured record; a DM peer is re-checked through authorize. Every authorized answer stays UNSUPPORTED, because this transport genuinely cannot rehydrate an inbound it would trust — its dispatch reads per-message side tables a rebuilt envelope has no row in — so the notice remains the only action it ever takes.

Three tests: test_whatsapp_replay_refuses_a_group_no_longer_configured, test_whatsapp_replay_never_redrives_but_notices_a_live_group, test_whatsapp_replay_reauthorizes_a_dm_peer.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author
  • fallback_text=turn.user_text reintroduces the string the text split excludes (span=6fd8766f4a8d) — fixed in 35673847d.

Correct, and the trace is exactly right: a non-admitted member's uncaptioned photo in a rules-mode group takes the _may_fetch_media-denied path, so inbound.text == "" and there are no attachments; user_text is build_silence_contract(rules) + "\n\n" + ""; and text or fallback_text resolves to the rules. The fallback I added to spare Weixin from restating an identical string was the disclosure vector.

Fix: fall back to turn.user_text only when the route declared no text field at all, or drop the fallback_text argument

Dropped the argument entirely. A route declares its text; empty means media-only (spooled for the notice with a nonzero attachments_dropped) or nothing to spool. Both shared-pipeline adopters already set text explicitly, so nothing else moved. test_the_turn_prompt_is_never_a_fallback_for_a_route_with_no_text and test_a_media_only_rules_mode_message_spools_no_rules (your exact case) pin it; the former asserts no spool file exists at all.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

First Principles Review 🟡 CONCERNS — two subtractions taken, one rebutted.

Move ReplayOutcome into transport.py and delete the bool-coercion machinerytaken, and this is the best finding of the run. The cited constraint ("transport.py stays stdlib-only") never bound an Enum, so the entire of() / overridden= / _replay_is_overridden apparatus existed only to police a bool that the ABC default was forced to return because the enum lived one module over. Moving the enum lets the default return ReplayOutcome.UNSUPPORTED directly, and a plain isinstance in _route_verdict keeps the notice-into-revoked-route leak closed with nothing left to police. Three functions and two tests deleted; inbound_spool re-exports the enum so the four transports' imports are unchanged in meaning (they now import from transport).

Prune __all__ to production consumerstaken. Down to InboundRoute, ReplayOutcome, ReplayReport, SpooledInbound, conversation_moved_on, replay_spooled, spool_refused_turn. The store primitives, error types and path are module-internal, reached by tests through the module.

Shrink ReplayReport to the counters the log line usesrebutted as disproportional. You are right the gateway discards the return value; the report's consumers are the log summary and the 72 tests, which assert on which entry went where (dispatched vs notified vs held vs dropped) rather than on counts. Collapsing it to four integers would remove exactly the observability that let each of the last five rounds' findings be proven red-before. Four lists of short strings is not a cost worth that trade.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 findings on 4e525475d, against head 18e00d18b.

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 (may_send_to). Follow-up for re-dispatch and WhatsApp groups: #9144.

Shielded replay escapes shutdown (inbound_spool.py:993, span=582f20a4331e)resolved by removal. There is no claim-before-dispatch any more: peek_next returns the entry without removing it and remove_entry runs only after the send is confirmed, so the pass is at-least-once. A cancel or os._exit at any point leaves the entry on disk for the next start; the worst case is one repeated notice line, never a lost message. asyncio.shield, _claim_and_replay and claim_next are deleted. Pinned by test_a_crash_between_send_and_removal_re_notices and test_a_crash_on_the_first_entry_keeps_the_rest.

Unconfirmed notice is discarded (inbound_spool.py:914)taken. _delivery_confirmed requires a non-empty message id, or any return on a transport whose capabilities.returns_message_id is False (WeCom/Feishu return "" on success). An unconfirmed send (raise or empty id) keeps the entry, is reported as unconfirmed, and the loop moves on so one dead route cannot park the queue. Pinned by test_the_notice_is_delivered_at_least_once, test_an_unconfirmed_entry_does_not_block_the_rest, test_a_transport_that_returns_no_ids_is_still_confirmed.

Configured WhatsApp groups lose replay notices (whatsapp/transport.py:327)taken, in the honest direction. You are right that may_send_to under the default dm_policy="self" refuses every group jid, and the fix you name ("carry the live group-gate verdict into notice authorization") is exactly the kind of parallel gate this head stops building. WhatsApp now declares a route for DMs only (is_group_jid → inbound_route=None), so a refused group message degrades exactly as before this seam instead of being spooled and then silently dropped. Teaching may_send_to to consult the group gate is the principled fix and is filed as #9144. Pinned by test_whatsapp_declares_a_route_for_dms_only.

Weixin original_text or text (weixin/transport_dispatch.py:329)taken. text=original_text, no fallback; the ingested form inlines temp paths. Pinned by test_weixin_route_text_has_no_fallback_to_the_ingested_prompt.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the Opus 4.8 finding on 4e525475d, against head 18e00d18b.

Discord dispatcher is None in production (discord/transport.py:317, span=6fd8766f4a8d)confirmed, and it is the finding that decided the shape of this head. discord/gateway.py:153 wires dispatch=_dispatch as a nested function, so every Discord replay gate was dead code and the tests passed only because they bound the method themselves. Rather than fix the wiring in a file this PR does not touch, this head removes re-dispatch entirely: replay_inbound, ReplayOutcome and all four transport overrides are deleted, and the replay is a notice gated on may_send_to(conversation_id, thread_id, principal=user_id). For Discord that gate reads _allowed_threads directly, so a thread revoked while the gateway was down is refused at the one place the send actually happens, with no dispatcher reference needed. Pinned by test_the_thread_is_part_of_the_egress_decision and test_no_transport_still_carries_a_replay_hook. Re-dispatch, if wanted, belongs in the channel dispatch wiring next to _dispatch where the live gates already run: #9144.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the First Principles review on 4e525475d, against head 18e00d18b.

Slack, the motivation's headline loss, ships nothingacknowledged, 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 InboundRoute declaration at its refusal site plus its existing may_send_to, with no replay_inbound override to write. That is the shape #8912 will land against. The three Slack refusal sites you enumerate are the right list.

Drop the text parameter from spool_refused_turn and read route.textagreed; landing in the next push rather than this one so the CI run on 18e00d18b is not evicted mid-review. All three callers do pass the value they put in the route, and the duplicate spelling can only diverge.

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.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 and First Principles findings on 18e00d18b, against head 2847b284e.

Unsigned pre-upgrade spool records are trusted (inbound_spool.py:548)rebutted as a residual the fence already accepts; requesting the human override. The adjudicator's own evidence record is the rebuttal: the only writer able to plant a record is a build that predates both the module and its fence (_CREW_SECRET_LEAVES, _CREW_HIDDEN_LEAVES); a planted record can only produce ONE restart notice, into a conversation may_send_to still authorizes for the principal it names, with the body run through display_safe_for; and the precondition — same-UID code execution on a legacy build — already lets the attacker send messages directly, so the marginal capability is nil. Signing the spool with a gateway-held secret would add a key-management surface to a bounded rescue buffer to close a path an attacker who has the key material's UID does not need. This is the same residual every other same-UID-writable state file under the crew home carries, and the fence is the mechanism the tree uses for it.

security.py comment says "TURN SOURCE" (security.py:8253)taken. Both the security.py and sandbox.py fence comments now describe the spool as an OUTBOUND source: an entry an agent could write is posted verbatim as a gateway-authored notice into the conversation it names. The "claimed by rename" clause is gone too, since nothing claims any more.

session_key and chat_type persisted with zero consumers (First Principles #7)taken. Both fields are removed from InboundRoute / SpooledInbound; a record from the earlier shape still parses (unknown keys ignored). Pinned by test_an_entry_records_only_what_the_notice_reads, which asserts the exact key set.

text parameter duplicating route.text (First Principles #8)taken, as promised last round. spool_refused_turn(channel_type=, route=) reads the text from the route; the three callers no longer spell it twice.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the First Principles review on 2847b284e, against head 4de9a1160.

_delivery_confirmed re-spells messaging.transport.delivery_confirmedtaken. The local helper is deleted; _notify_one calls the shared predicate with transport.capabilities. Its docstring is right that it exists so every proactive-send site asks the question the same way, and the WeCom/Feishu convention is exactly the thing a local copy would eventually forget. test_a_transport_that_returns_no_ids_is_still_confirmed still pins the behaviour through the shared path.

Drop path= from the public spool_refused_turntaken. Zero callers passed it; the store primitives keep theirs for tests.

Slack unadopted — unchanged: declared and deferred to #8912, as before.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 and First Principles findings on 4de9a1160, against head ace1262b1.

Failed removal causes a notice storm (inbound_spool.py:863)taken; this one is real. remove_entry returning False left the entry on disk and actionable, so the bounded loop would re-notice it up to SPOOL_MAX_ENTRIES times in one pass. The pass now remembers every entry it attempted and LEFT ON DISK (unconfirmed send, or notice landed but removal failed) and never hands it back to itself; the failed-removal case is reported as unconfirmed and the next start retries. A removed entry is deliberately not remembered, because two identical id-less bodies share a trace_id and forgetting the removed one is what lets its twin be noticed in the same pass. Red-before-proven: test_a_failed_removal_notices_once_per_pass_not_once_per_iteration fails with the guard removed (2 entries, 128 sends).

Shrink __all__ to InboundRoute, replay_spooled, spool_refused_turn (First Principles)taken; test_no_transport_still_carries_a_replay_hook now pins the exact list.

Replace peek_next's claimable callback with the two values its one consumer computes (First Principles)taken. peek_next(connected=, skip=) returns (entry, held_trace_ids), so the driver reports held entries without a second read and the predicate closure is gone.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 finding on ace1262b1, against head 167c2e46e.

WhatsApp media spools a transformed prompt (whatsapp/transport_dispatch.py:357)taken; real. WhatsAppTransport.receive rewrites msg.text with append_attachment_context (temp paths) before dispatch, so the DM route built from inbound.text would have spooled, and the restart notice quoted, on-disk paths to files that no longer exist — the same defect Weixin already had fixed with original_text. WhatsApp now captures (text as sent, media count) in receive BEFORE ingestion, in a pending_original side table keyed and scoped exactly like pending_verdicts / pending_operator / pending_message_id; the dispatcher's DM route reads it. There is deliberately NO fallback to inbound.text when the entry is absent — that fallback is the disclosure class this table exists to prevent — so an envelope that did not come through receive is not spooled. Red-before-proven through the real receive with a fake ingest (test_the_original_caption_survives_ingestion: fails with the capture removed); the dispatcher half is pinned by test_dm_route_spools_the_pre_ingestion_original_not_the_prompt, test_dm_route_is_not_declared_without_a_captured_original, test_group_route_is_never_declared.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 finding on 167c2e46e, against head 400772f2c.

Discord thread routes inherit DM authorization (inbound_spool.py:772)taken; real and not rare. DiscordTransport.may_send_to falls from a thread not in _allowed_threads to principal in _allowed on the documented assumption that a thread route names no principal; a spooled thread entry names the sender, and after a restart an auto-created thread is not in the roster, so a still-allowed sender would have authorized a notice into a revoked thread. _route_authorized now passes the principal for a DM route only; a threaded route (Discord thread, Telegram Topic — whose thread arm ignores the principal anyway) is authorized by the thread roster and nothing else. Red-before-proven against the REAL DiscordTransport (test_a_revoked_discord_thread_gets_no_notice_even_from_an_allowed_sender: with principal=entry.user_id for the thread, the notice lands; with it withheld, the route is dropped). test_a_threaded_route_is_authorized_by_the_thread_roster_alone pins the two call shapes.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 finding on 400772f2c, against head 61d494691.

Cancellation can strand the durable write past shutdown (inbound_spool.py:627)taken, by removing the mechanism rather than tracking it. You are right that await asyncio.to_thread(record_refusal_sync, ...) is a cancellation point in a handler close_all is about to cancel, and that a cancel there abandons a worker whose write os._exit then kills mid-file; the executor-shutdown RuntimeError fallback covered a different failure of the same shape. Rather than track spool-write futures and drain them in _shutdown (a second lifecycle for one bounded file), record_refusal now writes inline on the loop: a synchronous write has no await point, so once the refusal branch is entered the entry is on disk before control can leave it. The cost is a few milliseconds of loop time on a path that only runs while the gateway is already shutting down. Pinned structurally by test_the_refusal_write_has_no_await_point (the loss it prevents is only observable under a real os._exit, which a unit test cannot stage); it fails against the previous to_thread shape. The lock rationale is updated to name the concurrency that remains (boot-pass to_thread reads/removes vs. a late inline refusal write).

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 findings on 61d494691, against head cb5cbddc1.

Restricted messages are persisted to the spool (telegram/transport_dispatch.py:1119, Discord :913)taken; real, and the one I should have caught. An incognito or temporary conversation promised to persist nothing, and the spool is a durable file holding the message verbatim. Both dispatchers now ask their own _session_restricted(session_key) (the predicate that already gates the durable-history write, including the resumed-dashboard: case privacy_mode.is_restricted cannot see) 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. Red-before-proven at both dispatchers: test_a_shutdown_refusal_is_not_spooled_for_a_restricted_session in test_telegram.py and test_discord.py (each also pins that a persistent session IS spooled, so the gate cannot regress into "never spool").

Refusal persistence blocks the event loop (inbound_spool.py:637, AUTOSDE no-blocking-call-on-event-loop)taken. Last round's inline write was my answer to your cancellation finding; you are right that it trades one defect for another. The write is back in asyncio.to_thread and wrapped in asyncio.shield, which is what separates the two concerns: the handler task may be cancelled, the write is not, and nothing blocks the loop. The executor-shutdown RuntimeError case still falls back to an inline write rather than dropping the message. The remaining residual is os._exit landing while the worker is mid-write, which no in-process shape closes and which your round-10 adjudication already FLAGged as a bounded one-message residual. Pinned by test_a_cancelled_refusal_handler_still_lands_the_write: the caller is cancelled while the worker is parked mid-write, and the entry is on disk once released.

Note on CI: test_security.py::test_chained_cd_expansions_do_not_blow_up_the_gate is red on this head and on origin/main alike — #9089 moved _dir_holds_sensitive_leaf out of security.py and the test still patches it there. This PR's only security.py change is the "inbound-spool" leaf string.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 findings on cb5cbddc1 (no code change; both are fenced residuals already dispositioned, restated here so the human override has one place to read).

Unauthenticated pre-upgrade spool entries are trusted (inbound_spool.py:519)rebutted as an accepted residual; requesting the human override (same finding as round 11, 18e00d18b). The only writer able to plant a record is a build predating both the module and its fence (_CREW_SECRET_LEAVES, _CREW_HIDDEN_LEAVES); a planted record yields ONE restart notice, into a conversation may_send_to still authorizes for the principal it names (a DM route; a thread route gets no principal at all), with the body defanged through display_safe_for; and the precondition — same-UID code execution on a legacy build — already lets the attacker send messages directly. Signing the spool with a gateway-held key adds a key-management surface to a bounded rescue buffer to close a path the attacker does not need. Your adjudicator's own pre-drafted rationale says the same.

Cancellation leaves the spool write untracked (inbound_spool.py:641)rebutted as an accepted residual; requesting the human override (round 10's F1, FLAGged by your adjudicator then and now). The write is off-loop (your round-16 AUTOSDE finding) and shielded from the caller's cancel (your round-15 finding). What remains is os._exit landing during the sub-second window of one bounded atomic_write, which cannot tear the file and loses at most the one message being written — the documented pre-feature behaviour for exactly that message. Your own fix line concedes that "retain and await before propagating cancellation" cannot beat a hard exit either; it would add a lifecycle to defer a residual it cannot remove.

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.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Head 637e1eee5 is cb5cbddc1 rebased onto cbdd4a569 (#9182), which repairs the main-inherited test_security.py red; the PR diff is unchanged. Two GPT fenced residuals remain open for the human override (see the disposition on cb5cbddc1).

The "actively misleading" reply still fires at refusal time (First Principles, cb5cbddc1)rebutted as out of scope, deliberately. The channel's refusal-time notice is that channel's own string, decided per channel before this PR, and on Slack it is silence; rewording it is a per-channel copy change that #8912's adoption of each channel is the right place to carry, since the same pass has to decide what each channel says at refusal time AND declare its route. This PR fixes the loss (the message is recovered and the user is told, accurately, on the next start) rather than the wording of the moment-of-refusal reply, which for the four adopted channels is now followed by the accurate notice.

Shrink ReplayReport to counters / drop the returns (First Principles)rebutted, as in round 5. The report's consumers are the log summary and the tests, which assert on WHICH entry went where (notified vs dropped vs held vs unconfirmed); every red-before proof in rounds 12–17 leaned on exactly that. Four lists of short strings is not a cost worth that trade, and spool_refused_turn's bool is what the store tests assert against.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 findings on 637e1eee5, against head 100df0e81.

Confirmed sends can silently discard the notice tail (inbound_spool.py:828)taken, proportionately. The notice prefixes the quote, so a message that fit max_message_chars on the way in may not fit with the prefix and > markers, and a transport that slices to its cap and still returns an id would confirm a notice whose tail was cut. _quote now sizes the (already defanged) quote to capabilities.max_message_chars and truncates it VISIBLY ([…] (too long to quote in full — the rest is still yours to resend)) before the send. Not chunked into several confirmed messages, as the fix line proposes: the quote is an echo of text the sender still holds and is explicitly told to resend, so a marked truncation is an honest notice while a multi-part send with per-chunk confirmation is a delivery protocol for an echo. Red-before-proven: test_an_over_cap_quote_is_truncated_visibly_not_sliced_by_the_transport (1900-char message on a 1900-cap transport) fails without the sizing; test_a_short_quote_is_not_truncated pins the common case.

Cancellation can orphan the refusal write (inbound_spool.py:641)rebutted as an accepted residual; requesting the human override (unchanged from cb5cbddc1; your adjudicator's pre-drafted rationale is the one I would post). The shield keeps the write running under the caller's cancel; only os._exit racing a sub-second atomic_write remains, which cannot tear the file and loses at most the one message being written, and your own fix line concedes a drain-before-cancel cannot beat a hard exit either.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 findings on 100df0e81, against head 674d1a1c8.

Replay bypasses the live governance ceiling (inbound_spool.py:845)taken; real. The notice is a proactive send on a network surface, and every other proactive-send site asks the channels policy before sending; may_send_to answers only about the route, and a transport being connected says nothing about whether the operator still permits writing to it. The pass now asks vet_and_audit("channels", channel_type, tool_name="inbound_spool.notice", fail_closed=True) — the same audited seam as _vet_channel_send — per entry, off-loop. A denied channel is HELD rather than dropped: the route is not revoked, the channel is governed off, and the operator may loosen the policy before the horizon expires the entry. A degraded evaluation denies. Red-before-proven: test_a_channel_denied_by_governance_gets_no_notice_and_is_held (asserts the seam is called once with the expected scope/item/tool and the entry stays on disk) and test_a_governance_evaluation_failure_denies.

Cancellation can outlive the durable write (inbound_spool.py:642)rebutted as an accepted residual; requesting the human override (unchanged; your adjudicator FLAGs it with a complete rarity + recovery record and a pre-drafted rationale). The shield keeps the write running under the caller's cancel; only os._exit racing a sub-second atomic_write remains, and the proposed await-before-propagate reintroduces the shutdown stall the shield exists to avoid while still not beating a hard exit.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the Opus 4.8 finding on 674d1a1c8, against head c5382bfbc.

Revoked-route drop makes an egress permission decision with no SEL audit (inbound_spool.py:1021)taken. You are right that this is a permission decision (the transport's may_send_to) that also deletes a stored user message, and that the cross-surface proactive send audits the identical refusal. The _route_authorized-False branch now emits sel().log_api_access(operation="channel.proactive_send_authorize", outcome="denied", source=<channel>, caller=<user_id>, resources="inbound-spool -> <channel>:<conversation>"), mirroring chat_runner._resolve_channel_target, with the same best-effort guard so a SEL failure cannot turn the drop into a raise on the boot path. Red-before-proven by test_a_revoked_route_drop_is_audited.

Unsigned pre-upgrade spool records are trusted (inbound_spool.py:530)rebutted as an accepted residual; requesting the human override (same finding as rounds 11 and 17; your adjudicator FLAGs it with a pre-drafted rationale each time). The only writer able to plant a record is a build predating the module and its fence; the payoff is one defanged, size-capped restart notice into a may_send_to-authorized route on a governance-permitted channel; and the precondition already lets the attacker send messages directly.

@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Disposition of the GPT 5.6 findings on c5382bfbc, against head 1c4388ce7.

Allowed recipient authorization is not SEL-audited (inbound_spool.py:819)taken. Both outcomes of the may_send_to decision now go through one _audit_route(entry, outcome) helper (channel.proactive_send_authorize / allowed or denied), matching the cross-surface proactive send's record shape; a denial also deletes a stored message and a grant is what puts the gateway's own text into a conversation, so both are observable. Pinned by test_an_allowed_route_is_audited_too alongside the denial test.

Function-local vet_and_audit import (inbound_spool.py:842)taken; moved to module scope (messaging/identity.py already imports governance_profiles at top level, so there is no cycle to work around). The two governance tests patch the bound name in inbound_spool accordingly.

"verbatim" contradicts display-safe + truncation (inbound_spool.py:43)taken; the module docstring and TEXT_CAP comment now say the quote is display-safe and size-capped, otherwise as sent.

…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
@iamwhatever

Copy link
Copy Markdown
Collaborator Author

Head f18efe33c is 1c4388ce7 rebased onto current main (44 commits, incl. #9183 which split security.py into a package): the "inbound-spool" fence leaf moved with it to src/kiro_crew/security/paths.py::_CREW_SECRET_LEAVES, re-exported from kiro_crew.security as before. No other change to the PR diff.

Cancellation can still lose the refused message (inbound_spool.py:645)rebutted as an accepted residual; requesting the human override (unchanged since cb5cbddc1; your adjudicator FLAGged it in rounds 10, 17, 18, 20 and 21 with a pre-drafted rationale, and errored out entirely on 1c4388ce7). The shield keeps the write running under the caller's cancel; only os._exit racing a sub-second atomic_write remains, and the proposed await-before-propagate reintroduces the shutdown stall the shield exists to avoid while still not beating a hard exit.

@cixuuz cixuuz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two durability/security gaps remain:

  1. record_refusal() shields the to_thread future but immediately re-raises CancelledError without 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.

  2. record_refusal_sync() and _spool_lock() call mkdir() before the parent/link checks. A pre-planted linked/junction inbound-spool parent can be traversed before refusal. Create/open the parent and lock through pinned/no-follow operations, then validate the opened objects.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. record_refusal re-raising CancelledError without draining the shielded task is the intended shape. asyncio.to_thread dispatches the write to a non-daemon executor thread that runs to completion independently of the loop and of this coroutine, and record_refusal_sync never raises, so there is no unretrieved-exception path either. Retaining and awaiting it before re-raising would block close_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_write pins the behaviour.

  2. 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 raises FileExistsError into the best-effort except. _refuse_links then runs before the lock's O_NOFOLLOW open and before every read, write and unlink, and the reads fstat the opened descriptor for a plain single-linked regular file. Hoisting the check above the mkdir would 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants