feat: mirror /note to the channel and close the shared mid-send TOCTOU - #6831
feat: mirror /note to the channel and close the shared mid-send TOCTOU#6831rnoack1 wants to merge 1 commit into
Conversation
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
1 similar comment
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
0bc5b36 to
8df66fa
Compare
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound feature and a well-argued TOCTOU closure, but it deliberately widens Watch
Suggestions
[DESIGN-REVIEWED] 9524c47 |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed 1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands. BLOCKING -- src/kiro_crew/dashboard/chat_handlers.py:10622 -- Await permits accepted note loss during an unbound-slot rebind FINDING -- src/kiro_crew/dashboard/chat_handlers.py:10586 -- [BLOCK-MERGE] 9524c47 Adjudication (Opus 4.8) — is blocking on each finding proportionate?F1 (fenced) — the note-loss it describes is the pre-existing conditional-delivery contract for an UNBOUND slot, not a new silent loss introduced by the awaited save. Harm rung: nominally unbounded (silent data loss), but the loss is signaled and pre-existing.
🏷️ Fenced finding(s) machine-flagged as likely edge caseThe security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)
|
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All verification done. The author's countable claims check out against the base (0 First-Principles-Verdict: CONCERNS Every immediate Not justified as shipped
What this change shipsIntent: a user driving a session from a channel sees the
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 9524c47 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsI've traced both candidates against the actual base-branch source. Candidate 1 (immediate-note mirror retargets on rebind during the durability save): Candidate 2 (thread-less Slack-only binding files a phantom Neither survives falsification. No findings. [OPUS-REVIEWED] 9524c47 |
8df66fa to
3f24eef
Compare
3f24eef to
24252c3
Compare
24252c3 to
5464c9c
Compare
|
Head sha You were right that the description and the code had drifted, and the drift turned out to sit in a different place from the paragraph you quoted. Two functions were made public earlier in this PR at the First Principles reviewer's request, and the description still named the private forms: On the paragraph itself, the text you quoted is not what the description carries. It currently opens:
So the three-state framing and the The audit row is the one item where I read the code differently from you, so here is what I read. The guard at For the sentinel claim, the counts at this head are |
|
Head sha The paragraph you quoted is not what the description carries. The three-state framing is gone: "THREE states", "has THREE", "only two may deliver", "A module-level" and "SEL-audited as" all count zero, checked with whitespace collapsed so a line break cannot hide a match, against a live control ( No sentinel and no isinstance gate in the shared helper's module. The SEL row does exist, and this is the one place I read the code differently from you. You have it as logging the outcome rather than writing an SEL row. The guard is at Two things worth flagging since you last looked, both from your review rather than around it. Your objection was that the description described behaviour the code did not have, and I had introduced a second instance of exactly that, in the other direction. The description still said a held note reaches no channel and called flush-time mirroring the remaining enhancement. That is no longer true: a held note now carries the destinations it was authored for on its held record, and Also worth knowing: three CI lanes are red on this head and none of them are reachable from this branch. |
|
Head sha Correcting my earlier reply. I said the SEL row was the one place I read the code differently from you. That was wrong, and the framing was unfair. I went back and read the commit your review was written against, Taking your items one at a time, with what I read at this head. Two states, not three. One of the three lines you cited now carries the statement itself. No sentinel, no isinstance gate. The audit row now exists, which is the second branch you offered. Your required change allowed either dropping the claim or emitting the row and keeping it. The row is emitted: Where the guard lives, and your line numbers. The guard is One thing worth naming in case it is what you saw. A |
|
Head sha Correcting three coordinates first. My previous reply was written against a head that has since been rebased twice, and three of the numbers I gave you for
The coordinates in One naming point, because it affects searching. The emitter is spelled Your items at this head, each with what I read.
No No The Nothing in the change is owed against these items at this head. If you would rather I re-state any of them against a specific line you are reading, name it and I will read that line rather than paraphrase. |
|
Head sha The paragraph you quoted is gone. Searching the live description, The description does still contain the two tokens Your three code claims, word-boundary counts at this head. In The three lines you cited now hold comments rather than gates.
Nothing is owed on this finding at this head, in code or in prose. No new commit was needed to close it, so the head has not moved since my previous reply. |
|
Head sha The paragraph you quoted is gone from the description. No module-level sentinel. In No The audit row does exist, and this is the one place the code differs from your reading. The guard is A fabricated token returned zero across the same searches, so those zeros are properties of the files rather than of the query. |
|
Head sha Verify without line numbers. Each of these is unique in the file named:
At this head those sit at The paragraph you quoted is gone. In the current description, The sentinel: you were right, and here is the thing that will trip a grep. There is no The audit row: this is where the code differs from your reading. The refusal is not log-only. The guard sits inside A fabricated token returned zero across the same searches, so those zeros are properties of the files rather than of the query. |
|
Head sha The sentinel: correct, and the description already agrees with you. There is no One thing worth flagging, because it will look like a contradiction: a repo-wide grep for The audit row: this half has changed since your review. It is emitted. That was a fair call when you made it. Checking the commit your review was written against, neither the reason string nor the emitter existed there. The row landed in a later round. On the security property, since it is the thing the paragraph was really claiming: it still holds without a third state. The refusal moved rather than being dropped. A captured Use search strings, not line numbers. These have now drifted twice while this was open: the guard and the reason moved five lines between my last reply and this one. A fabricated token returned zero across the same searches, so the zeros above are properties of the files and not of the query. |
|
Head sha The Worth flagging because it looks like a contradiction: a repo-wide grep for The audit row: this half has moved since your review. It is emitted. Your call was correct when you made it. At the commit the review was written against, neither the reason string nor that emitter existed; the row landed in a later round. On the security property, since that is what the paragraph was really claiming: it holds without a third state. The refusal moved rather than being dropped. A captured Use the search strings, not the line numbers. These have drifted on every push since your review, including between my last reply and this one: the guard moved 577 to 571 and the reason 600 to 594. A fabricated token returned zero across the same searches, so the zeros above are facts about the files rather than about the query. |
|
Head sha Re-read at this head:
So the mismatch you named is closed, in the direction you called: the prose came down to what the code does rather than the code growing a sentinel. Line numbers here rot fast. The guard moved 571 to 585 and the reason 594 to 608 since my last reply, because a refactor landed above them in the same file. A fabricated token returned zero across the same searches, so the zeros are facts about the files rather than about the query. |
|
Both halves of this were correct at The authored_link: tuple[ChannelLink, bool] | None = None,with a single gate at The audit_channel_send(
session_key=session_key,
tool_name=EGRESS_TOOL_NAME,
channel_type=None,
outcome="denied",
reason="no_authored_channel_link",
)That sits at the one site that can produce a slot with no binding at authoring time. Five assertions in The net effect is the one your review was asking for. The third case is refused rather than delivered, and the refusal is audited. No sentinel is involved, because the refusal happens upstream of the shared helper rather than inside it. |
|
Settled at head The The guard is named, at the site you identified. The paragraph now says the third case is "refused at the ONE site that can produce it, The SEL row is the half that changed, rather than the prose. At audit_channel_send(
session_key=session_key,
tool_name=EGRESS_TOOL_NAME,
channel_type=None,
outcome="denied",
reason="no_authored_channel_link",
)at The shipped code therefore took both halves: the sentinel is gone from the shared helper, and the refusal is audited at the consumer. The paragraph also carries the point you closed on, that "a future producer that begins passing |
|
Both halves of this are settled at head The The audit_channel_send(
session_key=session_key,
tool_name=EGRESS_TOOL_NAME,
channel_type=None,
outcome="denied",
reason="no_authored_channel_link",
)
On the point underneath the two claims, that a reader is told where the guarantee lives: the description now names the guard site rather than a mechanism. The third case is refused at So the sentinel stayed subtracted and the audit row came back, which is what makes the paragraph's audit sentence true rather than aspirational. |
|
Head sha You wrote: "Rewrite the paragraph to describe what ships ... Drop the At this head:
Three tests that pinned the rows now pin their absence plus the refusal itself, so a row coming back is a failure rather than a silent change. Full suite for the touched modules is green at 792. One item from your review is not mine to close. The tracking issue the RFC front matter wants is a maintainer action, and filing it notifies the watchers, so I have left |
|
Head sha
One thing worth flagging because it came out of your own audit comment on this PR. You noted two remaining references to the symbol the change deletes. Both were in Suite for the touched modules is green at 806. Two items on this PR are not mine to close and I have not acted on them: the maintainer sign-off requested in the description for the |
|
Head sha Every coordinate you named, measured at the current head:
The paragraph was rewritten to match. It now opens " So both halves of your required change are in, and the code took the drop branch rather than the re-add branch you offered as the alternative. |
|
Head sha Your four code claims, each measured at this head with a control so a zero means something:
The paragraph was rewritten to match. It now opens " So the code took the drop branch rather than the emit-the-row alternative you offered, and the prose now describes that. I also swept the other surfaces for the retracted claims so two contradicting statements do not survive: the commit message body carries none of them, the module spec carries neither the sentinel claim nor the row name, and this change adds zero lines mentioning either token anywhere in the diff. |
|
Head sha Your four code observations, each with a control so a zero means something:
On the row, because the thread now carries two answers that look contradictory and are not. You were right originally. The reply saying the row IS emitted at The paragraph now matches. It opens " So the code took the drop branch rather than the emit-the-row alternative you offered, and the prose describes that branch. I also swept the other surfaces so no contradicting copy survives: the commit message body carries none of the retracted tokens, and the module spec carries neither the sentinel claim nor the row name. |
|
Re-confirming at head
Nothing about those four items changed between that reply and this head, so this adds no new evidence, only the current sha. The intervening work was elsewhere: the send-adjacent staleness guard now compares the composite governance answer (ceiling intersected with profile) rather than the ceiling counter alone, plus an import-order fix and a test correction. |
|
Thanks — you were right when you wrote this, and on all three counts. The code had adopted the subtraction and the Description had not been reconciled to it. The prose has since been rewritten; each item below is verified at head The sentinel. There is no The SEL row. Where the third case is handled. The captured- Two things worth flagging since they are adjacent to what you read. No code changed for this reply and the head sha is unchanged — the Description was the stale half, and it was the Description that was corrected. |
|
You were right, and on all three counts. The code had adopted the First Principles subtraction and the Description had not been reconciled to it. That paragraph now describes what ships; verified at head The sentinel. There is no The refusal is caller-side, as you read it. The guard is The SEL row — one correction worth flagging, because it moved after your review. So the paragraph you flagged now claims: two states, no sentinel, the caller-side guard at its single consumer, and an audited refusal with the no-transport exemption named. If you would rather the audit row not be there at all, that is a genuine disagreement between two reviewers rather than something I should settle quietly — say so and I will take it back to the maintainer instead of choosing between you. |
|
Verified at head The SEL audit row exists. There is no three-state sentinel, and the description does not claim one. It states the shared helper is two-state and that no What did change since you reviewed, in case it is what you were looking at: the mid-send retraction and the config-generation/stat-fingerprint bracket were both removed on review feedback, and with them three audit reason codes. The unused Happy to re-check anything else you spot at this head. |
|
Verified at head The prose now describes what ships. The code matches. On the audit trail specifically — you wrote that if the row is wanted, emit it and keep the claim. That is the branch taken: the refusal now files a denied Two things have moved since you reviewed, both worth naming so the diff is not a surprise: a mid-send retraction and a config-generation/stat-fingerprint bracket were removed on review feedback, and the shared helper's continuation re-ask is now unconditional rather than gated — the module's own comment promised that and the code was withholding it. Happy to re-check anything else you spot at this head. |
|
Verified at head The paragraph now describes what ships. It opens The code matches, scoped the way you scoped it. On the audit trail specifically — you wrote that if the row is wanted, emit it and keep the claim. That is the branch taken, so the refusal is now observable in the audit log rather than only in a log line: Two things have moved since you reviewed, both worth naming so the diff is not a surprise. The shared helper's revalidation is now unconditional rather than gated on Happy to re-check anything else you spot at this head. |
|
Head sha 1. The sentinel and the SEL row are absent from the code, as you said. In 2. The description no longer claims three states. It now states the two-state design explicitly and says in as many words that there is no 3. Where the third case actually goes. The state your finding was about — a slot with no binding when the work was authored — is refused at the single site that can produce it, I swept the other surfaces for the same claim so a corrected description does not leave a stale copy standing: the commit body carries it zero times, and of the two docs files this change touches, the only "three-state" hit is |
|
Verified both halves of this at head The One thing worth recording so a later reader does not trip on it: a repo-wide grep for The The description was the defect, and it is corrected at this head. It now states Two boundaries in that paragraph are deliberate and I would rather state them than leave them implied. The no-transport install stays silent, because there the refusal is about the install rather than the request and a row would fire for every note on every session while naming no destination. And a future producer that begins passing |
|
Thanks — this was accurate when you wrote it, and both halves of it have since been addressed. Verified at head Your reading of the code was right. At head there is still no The missing audit trail is the part that changed. You were right that the refusal only logged. It now files a row first: The guard is where you said it should be. The paragraph has been rewritten to match, taking your second option. It now opens Since you noted that re-adding the row re-adds part of what the subtraction removed: what came back is one call to an existing module-level emitter at a single site, not the sentinel or the three-state branch. The emitter is module-level for the reason you would expect — a second refusal site on this leg with a hand-written copy is how a denial stream comes to look complete with one branch missing. |
|
@rnoack1 This PR and #7163 change the same held-note pipeline from opposite ends, and that is the overlap to settle first. Shared files: Two consequences. The reason given here for carrying the dispatch on the record, that the no-argument spelling is asserted by tests outside this change, stops holding once #7163 lands. And the exhaustive record-shape paragraph is wrong in either merge order. #7163 is the further-along side of this seam, since it owns the signature and the list's element classes. Suggestion: land #7163 first, then rebase this PR onto it, restate the docstring to cover both element classes, and make the mirror dispatch skip a Both PRs are yours, so one ordering decision settles this. The audit read this PR at 2830b8e; the head has since moved to dcfca06, and both shared files are still in the diff. Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
A note's context half already reaches the model on any surface, but its visible half broadcast only to the dashboard — so a channel-driven session gained the context with no visible provenance where its user was actually reading. Delivers through the two existing outbound paths and reports `mirroredTo` so a caller can tell delivery from silence, with the Slack leg gated on recipient authorization re-asked after every await and the shared ladder revalidating its binding before it sends.
Problem / Motivation
POST /api/chat/slots/{slot}/notewrites two halves. Only one was surface-aware.The context half is already surface-agnostic:
drain_pending_contextruns inside_run_chat, the one runner every inbound surface shares, so a note reaches the model whether the next message is typed in the dashboard, Slack or Telegram.The visible half was not.
slot.append(broadcast=True)fans out over the dashboard's own SSE/WS and nothing else, and every channel egress lives in the turn loop (user echo, tool stream, assistant reply, approval prompt, auth error) as a separate site the note path never reached.Why it matters
The result is worse-shaped than a missing notification: a session driven from a channel gained an agent that silently knew something its user was never shown. The note had no visible provenance on the surface they were reading, so the next turn referenced something with no record behind it. A missing notice gets noticed; invisible provenance does not.
What changed
Delivery reuses the two existing outbound paths rather than adding a third, matching
chat_compaction_notice— the closest existing analogue, a proactive non-turn notice addressed to whatever conversation a session belongs to.slack_client.post_message. Slack is deliberately absent from thechannel_transportsregistry so it cannot ride the ladder, and it is the one channel bindable two ways: a Slack-born session's origin, or a dashboard slot's_slack_linkedmirror. Both resolve here._resolve_channel_target), walking origin then mirror. This covers the remaining nine transports with no per-channel code, inheriting governance vetting, recipient re-authorization,supports_proactive_sendandchunk_for_transport.Both legs can fire for one note: a dashboard slot can hold a Slack thread link and a non-Slack mirror at once, and each is a conversation with a user in it.
The mirror is dispatched in the background and the response reports nothing about it. An earlier revision returned a
mirroredTofield; First Principles measured zero consumers for it, and I confirmed that at source — the field appeared in 0 files at the base, so it had never shipped and no external caller could depend on it. It was also the only reason the POST waited on a channel at all. Removing it removes the wait, the ceiling that bounded the wait, and both arms that served it. The note's contract stays the transcript line and the context entry. For a note that is NOT held those are committed before the mirror is dispatched. A HELD note is not mirrored at all, which is how that inverse gap is closed. While a turn is runningappendedis false and neither half is committed, so a foreign binding acquired before flush drops BOTH rather than retargeting them; mirroring at POST would therefore have published a channel note asserting content the session never received. The dispatch is guarded byif not deferred:anddeliveryConditionalreports the hold, so no caller is told otherwise. Mirroring held notes at FLUSH time, once their halves land, now ships: the held record carries its authored destinations andflush_deferred_notesdispatches them, across its six invocation sites in three modules (chat_orchestratortwice,chat_runnertwice,chat_handlerstwice, the last two on a slot being torn down) (a different count from the seven resolve-then-send sites inventoried inslack_egress.py) and moving channel egress under it is a behaviour change to a surface this PR does not otherwise touch.One further change rides along and is declared here rather than left for a reader to find:
_source_cap_reachedbuilds its held-context list as[c for c in (n.get("context") for n in slot._deferred_notes) if c is not None], reading each held note'scontextONCE through.get()instead of testing it and then subscripting it. Behaviour is unchanged — the previous comprehension evaluated its guard before the subscript, so it could not raise on a context-less entry either — but the durable hold now admits entries this predicate walks, and a single read is the form that stays correct if that ever stops implying the key is present.Two decisions worth a reviewer's attention
HELD NOTES MIRROR AT FLUSH, not at POST. A note arriving while a turn runs is held, and that is the common case for background senders by the module's own docstring, so it is the case the feature most needs to cover. Mirroring at HOLD time is rejected on orphan-post integrity: neither half is committed while held, so the channel would carry a line the transcript does not yet have, and a foreign rebind before the flush drops both halves and leaves the channel asserting a note that does not exist in the session. An orphan post is a data-integrity defect; a dropped best-effort mirror is not. So the POST dispatch stays guarded by
if not deferred:, pinned bytest_a_held_note_does_not_reach_the_channel, and the held record instead carries the destinations the note was AUTHORED for plus a bound dispatch.flush_deferred_notescalls it once BOTH halves commit, pinned byTestAHeldNoteMirrorsAtFlush. Snapshotting at authoring rather than at flush is what keeps the authored-link guarantee: a channel rebind during the hold makes the send REFUSE on the comparison instead of retargeting to the replacement. Three cases are pinned separately — the dispatch itself, a raising mirror not un-writing a committed note (the suffix-restore must not fire for a delivery failure), and a note dropped for a foreign rebind never dispatching at all. The mirror is carried on the record rather than threaded through a new parameter becauseflush_deferred_notesis called from five sites across three modules and its no-argument spelling is asserted by tests outside this change.An absent snapshot is no longer a licence to deliver.
authored_linkis TWO-state on the shared helper: omitted (an inline caller, which has no authored binding to compare against, which therefore skips only the authored comparison, not the continuation re-ask) or a captured link (compare and refuse on a mismatch). There is no_UNSETsentinel — the third case, a slot with NO binding when the work was authored, is refused at the ONE site that can produce it,chat_note_mirror._deliver_via_transport, whoseif authored_link is None:guard returns without delivering. That placement matters: the helper's walk runs when the TASK runs, so passing an unbound slot through would let it read a binding created after authoring and treat that as the authorized destination. Refusing at the capture site's consumer keeps the shared helper two-state and needing no sentinel to tell "absent" from "captured None". That refusal files a deniedaudit_channel_sendrow under the reason codeunbound_at_authoring, because on an install that HAS channel transports it is a permission decision about a real deliverable surface, and it would otherwise be the only denial on this leg with no SEL record. The exemption is the no-transport install, which stays silent: there the refusal is about the install rather than the request, so a row would fire for every note on every session while naming no destination. The emitter is module-level precisely because a second site refuses on this leg — a hand-written second copy is how a denial stream comes to look complete with one branch missing. A future producer that begins passingauthored_linkmust still carry its own guard for the unbound case; the shared helper does not provide one.Two duplications collapsed.
deliver_to_channel's_walk_ladderclosure was a second hand-written copy ofsnapshot_channel_link's origin→mirror pause-aware loop, so "the snapshot and the delivery agree by construction" actually meant "they agree as long as both copies are edited together" — and a pause-awareness fix applied to one is exactly the divergence the comparison cannot detect. The closure now calls the shared helper.snapshot_slack_linkwas a pure alias of a private_slack_linkwith one consumer; the body now carries the public name and the alias is gone.channel_egress_permittedgains a requiredtool_nameparameter and is public. Slack bypasses the ladder, so without sharing this gate the note's Slack leg would be the one unvetted, unaudited egress. Duplicating a fail-closed gate is how the two copies drift, so this is one function with a parameter rather than two that look alike.tool_nameis a caller identity for the SEL record, never a permission input. It is public because it genuinely has two consumers in two modules — this chain and the compaction notice, which imports it fromslack_egress— so a private name would advertise a boundary the code does not respect.tool_namecarries no default: all three call sites pass it explicitly, and a default would only let a future caller inherit another feature's identity in the SEL record.The pause gates are honoured on both legs. A note is background rather than turn output, so
slack_mirror_is_paused/mirror_is_pausedare not strictly aimed at it — but disconnect is the user saying "not into this conversation", which covers a note about it as much as a reply in it, and the dashboard transcript still carries the line either way.Scope, stated up front. The hardened chain covers the note mirror and nothing else.
api_send_message's Slack leg, the owner DM and the hook DM stay plain-client, and the compaction notice takes the shared gate without the rest of the chain, so three proactive-Slack tiers ship here rather than one. Reading the chain as a repo-wide guarantee would be wrong. The tier table below names every site, and a census test fails if a fourth tier appears unmarked.One caller outside the note feature gains one refusal, and the line is fail-open vs contract-widening.
api_send_messagesharesdeliver_to_channel, and the split is by QUESTION rather than by caller.Asked unconditionally. "Was the permit authorized under the ceiling STILL INSTALLED" needs no captured link: the ceiling is sampled before the target resolve, and that resolve is offloaded because it walks the profile directory, so it is an unbounded await for every caller. Asking this only when an authored link was present was a fail-OPEN — a tightening landing inside that window left the permit authorized under a ceiling no longer installed and part 1 went out on it.
governance_changed_during_resolveandgovernance_changed_before_part_1therefore apply to every caller, both refuse BEFORE part 1 so nothing is delivered, andtest_an_inline_send_refuses_when_governance_tightens_during_resolvepins it.Gated on
authored_link. One question is dispatched-only: "is the live binding the one this WORK WAS AUTHORED FOR" (link_changed_before_dispatch). It compares against the binding a caller captured before its work was queued, and a caller that never captured one has nothing to compare — the parameter is wherelinkcomes from on that path, not a switch.Also asked unconditionally; re-gating is disqualified by measurement, and a maintainer ratification of the widened refusal set is still outstanding. "Is the live binding still the one THIS SEND SELECTED" compares the post-resolve walk against
link, this send's own resolved destination, so every caller holds the comparand (link_changed_during_resolve) — and the per-chunk CONTINUATION re-ask asks the same of every later part. Earlier revisions gated both to keep an LLM-facing tool's mid-send contract out of a note change. That was a fail-OPEN, not a deferral: being inline bounds only the window before part 1, every transport send is an await, so a multi-part inline send spans the same window a dispatched one does and parts 2..N were reaching a conversation already unlinked. A mismatch is always a refusal, never a retarget.test_an_inline_send_refuses_when_the_binding_moves_during_resolveandtest_an_inline_send_stops_remaining_parts_when_unlinkedpin both.send_message's refusal set changed here, not in its own change. Stated plainly because it is the one cross-cutting effect of this PR: the inline/api/send-messageleg now refuses on a superseded governance ceiling, on a binding that moved during its resolve, and between parts once the destination is revoked. Each closes a hole where content reached a conversation the caller had not selected or was no longer permitted; none of them retarget, and none change a permitted send's outcome.No other generation consumer regresses.
governance_answer_generation's signature and contract are byte-identical to base (opaqueint, comparison-only). Its four consumers all use it as an equality comparand —ws.py:777(current != answer_generation, and that file is untouched by this change),state.py:7857(a cache-invalidation field),slack_egress.py:180, andgovernance_ceiling_unchanged(== observed) — so publishing one bump per publication instead of two can only reduce FALSE invalidations, never suppress a real one.test_a_publication_moves_the_generation_exactly_oncepins both directions: one publication moves it once, and a reload that publishes nothing does not move it at all.Tests
test/test_note_channel_mirror.py— 117 tests across both legs: delivery, the session-map fallback for a bare slot, paused mirrors, foreign namespaced channel ids refused at the Slack client, swallowed delivery failures, fail-closed governance degradation, non-proactive transports skipped, a Slack link declining to ride the ladder, origin-before-mirror ordering, both legs firing together, recipient authorization on the Slack leg with its SEL audit of every outcome, mid-send revocation aborting the remaining chunks on both legs, revocation timed inside the mid-send re-resolve await, the awaited mirror's overall bound, and the background dispatch not blocking the POST, the held-note case not reaching the channel at POST and reaching it at flush, and the two earliest Slack refusals — a disconnected thread and a foreign namespaced id — each emitting adeniedSEL row under its own reason code, with a third test asserting the two codes differ so the rows stay filterable. The endpoint tests share one draining helper that cancels and gathers the background mirror it dispatched, so no task outlives the test that spawned it; a dedicated test asserts that directly, because a leaked task otherwise only ever showed up as a stderr line while the suite still reported green. Two further tests cover recipient authorization across surfaces: a telegram-origin direct session linked to an authorized Slack channel must reach the tracked-channel authority rather than being refused by a principal check that cannot apply to it, with a Slack-origin counterpart asserting the principal path is still consulted so the surface gate cannot be too broad.No live channel is required. The Slack leg takes a stub client and the neutral leg drives a fake
MessagingTransportthrough the real send ladder, so the nine transports that are unreachable from many corporate networks are covered by the same assertions as Slack.Negative controls, run per round by neutralising each fix and observing the failure before restoring it. Latest: removing the post-await re-walk in the mid-send path makes the new test fail
assert 2 == 1— part 2 does reach the revoked conversation — and reverting the mirror to unbounded makes the bound test failTimeout >25.0s. Both detect their defect rather than passing vacuously.Local gate results:
check_black_formatting)src/kiro_crew test conftest.py xdist_budget.py)src/kiro_crew/)Affected suites run:
test_gateway_appkit_endpoints,test_cross_surface_mirror,test_channel_compaction_notice,test_channel_compact_failure_binding,test_chat_mirror,test_channel_transport_outbound_authz,test_messaging_transport,test_redaction_mirror_parity,test_slack_mirror_context_leak,test_ci_surface_tests.Echo-loop check
The mirror posts into a thread the gateway also listens on, so inbound must not ingest it as a user turn. Verified at two independent points:
slack/transport.pydropsbot_id/subtype == "bot_message"before authorization, andslack/events.pydoes the same with a SELuntrusted_botdenial. Telegram filters on its ownbot_id. The compaction notice already uses this exact egress.Not traced: the
from_trusted_botbranch inslack/events.py, which is a separate code path.Behaviour change beyond
/noteClosing the caller-supplied-link TOCTOU moved the ladder walk, the pause skip and the post-await revalidation into
handlers/messaging.deliver_to_channel, which is shared. MID-SEND REVALIDATION IS UNCONDITIONAL, and it widensapi_send_message's refusal set deliberately. An earlier revision keyed it onauthored_linkso the inline caller kept its exact prior behaviour; that was wrong, because being inline bounds only the window between the REQUEST and the FIRST part. Every part is an await, so a multi-part inline send spans the same inter-part window a dispatched one does, and its later parts reached a destination the user had already unlinked. The post-resolve re-walk (link_changed_during_resolve) and the per-chunk re-asks therefore run for EVERY caller. Whatauthored_linkstill gates is only the authored-link COMPARISON — "is the live binding the one this work was AUTHORED for" — which needs a captured link and so remains dispatch-only. Forapi_send_messagethis adds seven reason codes, not one: three that can refuse before the first part (a binding or governance ceiling replaced while the permit resolved) and four that fire only on a later part. A send the resolve permitted CAN now be refused before part 1, which the earlier wording denied.deliver_to_channelstill answers with a bool and LOGS which ladder row it selected rather than returning it, so its existing caller is unchanged.That is why
test/test_send_message_targeted.pyis in this diff:test_cron_job_session_key_is_used_verbatimassertedassert_called_once_withon the link getter, which encoded the old single-read behaviour. It now pinscall_args_listso every call is checked for the whole session key — the property that test is actually about — rather than only the first.Bounds and known asymmetries
The mirror is bounded per leg, not per request. Each delivery leg carries its own
_LEG_TIMEOUT_Sand absorbs its own stall or raise, so a wedged channel cannot starve a healthy sibling. That bound now lives entirely inside the mirror: the endpoint dispatches it in the background and never waits, so no outer ceiling exists or is needed. A channel that cannot be reached simply does not receive the note; the failure is logged and SEL-audited rather than reported to the caller.Held-note / rebind window — CLOSED. Two earlier revisions of this description got this wrong in opposite directions: the first claimed "a rebind cannot leave a channel showing a note the agent never receives", the second accepted that it could. Both are withdrawn. A held note is not mirrored at POST, so the window in which the channel held a note the transcript and context did not cannot arise; and it is no longer left unmirrored either, because the flush dispatches it once both halves land. A note whose slot was rebound to another session during the hold is dropped by the rebind guard before the write, and dispatches nothing.
Keying the mirror to the same flush-time condition is the better fix and is what ships.
flush_deferred_notesis a synchronous_ChatSlotmethod with no back-reference toDashboardState, so it cannot resolve destinations itself; the held record carries a bound dispatch instead, which is also why no call site changes spelling. It now has six invocation sites across three modules, one of them a teardown handing its key to a concurrent same-key recreate, where the halves do commit and the key stays open, so provenance there is wanted rather than suppressed.Accepted and deferred — declared, not carried silently
The hardened Slack send lives in its own module, and the note mirror runs it. The chain (governance gate → recipient authorization → coordinate and pause revalidation after every await → chunking → abort-on-revocation) is
_deliver_slack_governedin the newdashboard/slack_egress.py, besidechannel_egress_permittedand_slack_recipient_basis. It began insidechat_compaction_noticebecause that was its first consumer, which homed a cross-feature boundary in a feature-named module — the next caller either misses it or copies it, and a copied egress check stops being re-verified. The note mirror is its ONE consumer. The compaction notice deliberately stays on the barechannel_egress_permittedgate it imports fromslack_egress: adopting the full chain there would widen an existing surface's refusal set, which is a behaviour change to a surface this PR does not otherwise need to touch, so it is deferred alongside the three other proactive Slack sends_deliver_slack_governed's docstring names.test_a_recipient_no_authority_names_is_still_deliveredpins that unhardened posture, so adopting the chain later has to be a reviewed change rather than a silent rider. Slack is deliberately absent fromchannel_transports, so it never reaches_resolve_channel_target's ladder and gets none of that protection for free; this is where it lives instead, in one copy rather than two that drift.The tier map now has a named owner, not a promise. The three proactive-Slack tiers this change leaves in place (the full chain here, the gate-only compaction notice, and plain client for
api_send_message's Slack leg, the owner DM and the hook DM) are pinned by a census test that fails if a new dashboard module sends to Slack unclassified, or if the plain-client tier changes size. The deferred sites are enumerated by symbol indocs/system-specs/modules/messaging.md, alongside the tier map the census test pins executably, so the follow-up is a named inventory rather than a docstring sentence. That inventory deliberately stops at the INVENTORY: it names each deferred sibling and the profile-store cold load, and it prescribes no design for the consolidation. Two earlier revisions went further and both were subtracted at a reviewer's request -- a seven-step forward plan (including a binding-generation counter), because a design nobody is committed to build is speculation, and then the separate RFC document that carried it, because the only parts the census test and the spec actually lean on are the tier map and the symbol-named inventory, and those belong with the module they describe. Filing a tracking item is a maintainer action.The held-note pinning tests are mandatory, not advisory.
TestAHeldNoteMirrorsAtFlushandtest_a_rebound_slot_drops_the_held_note_without_mirroringboth live intest/test_note_channel_mirror.py, which carries zeroskip,skipiforxfailmarkers, and CI's backend job has no deselect list at all (its own comment records thatBACKEND_DESELECTSwas removed deliberately) and never names this file. Every backend shard therefore collects them and neither can be skipped on a runner.There are now TWO revalidation chains, and consolidating them is a declared follow-up rather than work this change withholds.
deliver_to_channelre-walks the transport ladder per chunk;_deliver_slack_governedre-asks_permitted_to_sendper chunk. What is genuinely shareable between them is only the per-chunk loop driver. What is NOT shareable is everything the loop guards: the authority sets differ (Slack sits outsidechannel_transportsand never reaches the ladder at all), the delivery-confirmation predicate differs, and the audit vocabulary differs.handlers/messaging.pydoes not importslack_egress, so the shared piece could not move between those two; it lives in the module both already import, assend_parts_revalidatinginkiro_crew/messaging/renderer.py. That per-chunk loop driver IS extracted here and both chains now run through it. What remains unshared is what the loop guards, which is the list above.Two consequences worth stating plainly:
_is_genuine_slack_link) demanded a thread and a channel, which is right for a note and wrong for a notice: a session bound to a channel with no thread posts top-level, and requiring a thread would have silently dropped its notices. What that check is actually for is refusing another channel's legacy namespaced id, which has nothing to do with threads — so the namespace half is shared and the thread half stays with the note leg as its own local precondition. Pinned by a test that fails when the thread requirement is reinstated.The note mirror passes a pause predicate;
is_pausedis a REQUIRED parameter of the shared send, so no adopter can omit the check by accident. The compaction notice does not call that helper at all, so the question does not arise for it.api_send_message's failure surface IS widened, and that is intended. The shareddeliver_to_channelgained a post-await re-walk and a per-chunk re-ask, and NEITHER is gated: both run for every caller, so the mid-send TOCTOU is closed forapi_send_messagetoo rather than left open. The widening is SEVEN new reason codes, six refusals and one error, none gated onauthored_linkand so all reachable by the inline caller. Three can refuse before the first part goes out at all (link_changed_during_resolve,governance_changed_during_resolve,governance_changed_before_part_1); the other four fire only on a part after the first (not_permitted_mid_send_before_part_N,link_changed_during_recheck_before_part_N,governance_changed_mid_send_before_part_N, and the errorresolve_failed_mid_send_before_part_N). An earlier revision of this description claimed one refusal and claimed nothing permitted at part 1 could be refused; the governance-ceiling sampling added later made both false, and they are corrected here rather than left to read as the smaller change. Exactly one refusal stays dispatched-only, and not by choice:link_changed_before_dispatchcompares against the binding a caller CAPTURED before its work was queued, so a caller that captured none has no comparand. The note mirror is dispatched to a background task and can be overtaken by a rebind, which is why it captures one.test_revalidation_covers_both_callers_and_authored_link_gates_comparisonpins the split by measuring ladder walks on both paths. Named plainly: this is a tightening of a surface this change is not otherwise about. The note mirror is the feature;api_send_messageinherits the closure because the helper both callers share is where the TOCTOU lives, and gating it to the mirror alone would leave the same hole open for the LLM-facing caller.Seven resolve-then-send siblings remain unfixed, named explicitly. The post-await revalidation now covers the shared
deliver_to_channeland, via_deliver_slack_governed, the note mirror's Slack leg. These seven sites across five symbols still send on a link captured before their governance await, with no re-walk (api_chat_slot_mirror_linkcarries three of them):state.py_notify_inbound_unbindchat_mirror.pyapi_chat_slot_mirror_linkchat_mirror.pyapi_chat_slot_mirror_linkchat_mirror.pyapi_chat_slot_mirror_linkslack/gateway.py_deliver_channel_replyhandlers/messaging.py_deliver_channel_dmapi_send_message's Slack legEach was re-verified by SYMBOL at the current tree, not inferred: line citations from the original base had drifted, and one (
state.py) pointed at a function that performs no send. Fixing them is a general change across four more modules and belongs in its own PR; naming them here so the gap is not mistaken for coverage, and so whoever takes it has the inventory.Out of scope
A note's transcript row can be replayed into a new session on a cold start (
is_new and not _provider_has_history), so the model may see it twice in one prompt. That is pre-existing, documented in the endpoint's own docstring, and a deliberate tradeoff — the replay is char-budget bounded, so dropping the queued copy would lose older notes the replay had already trimmed. This change adds no transcript row and neither causes nor worsens it.