fix(dashboard): re-assert queued prompt containment at drain (#5911) - #5978
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound drain-side re-validation, but the admission stamp is a convention spread across ~10 producers, enforced only by a floor that destroys messages. Watch
Suggestions
[DESIGN-REVIEWED] 3262480 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All checks are done. Every main-queue producer is stamped or exempt-kind, First-Principles-Verdict: CONCERNS The fix is sound and cause-level, but ~250 lines of black-baseline reformatting and an allow-side audit row ride along in a security fix. What this change shipsIntent: stop a queued prompt from executing after the session's containment changed between enqueue and drain (TOCTOU, issue #5911) — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 3262480 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've read the instruction set, the untrusted candidate file, and the full diff. The discovery pass recorded no candidates. I independently traced the core logic of this change — the drain-time re-validation ( Working candidate-by-candidate found nothing to falsify (there were none). Extending in Step 2, I actively sought a groundable defect in the changed lines: an unstamped plain producer that could ride past a boundary, a mirror/workspace comparison that fails open, an audit filed under the wrong key, a crash on malformed meta. Each is closed — unmarked entries fail closed against the full constraint set, mirror probe failure refuses delivery, workspace/mirror compare by identity, and the behaviors are pinned by the accompanying tests. No item reaches the (a)/(b)/(c) bar at 80+. No findings. [OPUS-REVIEWED] 3262480 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
2889949 to
472ddc9
Compare
|
472ddc9 to
ecd0265
Compare
|
|
ecd0265 to
39d54aa
Compare
|
|
Dispositions for the Design Review 🟡 CONCERNS on
|
|
Dispositions for the First Principles Review 🟡 CONCERNS on
|
39d54aa to
960a3dc
Compare
|
Dispositions for the GPT review of
|
960a3dc to
30b3bed
Compare
A message queued for a busy chat session was delivered later without re-checking the authorization that admitted it: a target authorized while unlinked could gain a channel or mirror link between enqueue and drain, and the queued prompt then executed and republished to that channel. The gap was caller-independent -- session_send and a human typing into a busy session share the same enqueue_or_run_prompt -> queue_append -> chat_runner drain path. Close it at the drain, once, for every caller: - Tag at enqueue: every producer of plain (user-speech) queue entries stamps the admission-time containment snapshot (linked / mirrored / crew / ephemeral / app / unattended / workspace) on the entry via session_control.containment_meta. Recovery requeues stamp fresh admission context at requeue time -- _queue_recovery (including the fallback-model retry) and the manual continue -- because a recovery replays externally admitted content verbatim under a new queue id. - Re-validate at drain: _start_next_queued_turn sweeps the queue before the dequeue and drops any entry for which a constraint holds NOW that did not hold at admission -- including a workspace change, which swaps the memory/lessons/project context under a waiting prompt -- reusing the session_control helpers (authorize_target's own constraint set, the mirror probe with an explicit fail-closed direction per side). - Audit both outcomes: a dropped entry retracts its queue card (unconditional broadcast), appends a visible transcript notice naming the changed constraint, and writes a denied SEL record under the slot's EFFECTIVE session key; entries that pass re-validation write an allowed SEL record at consumption, matching authorize_target's convention of auditing the permission decision in both directions. Boundaries that keep designed behaviour working: unmarked plain entries -- including unmarked recovery entries -- fail closed against the boolean constraint set; only cron notifications and sub-agent completions are exempt, as runner machinery minted fresh by trusted internal producers that channel-born sessions receive by design; a constraint already held at admission is not a change (channel-born sessions keep draining); entries carrying the authenticated-human provenance flag (never granted to app surfaces -- the manual continue derives it from the request identity) are exempt from the LINKED constraint only, so a user linking their own busy session does not destroy the messages they already typed, while a NEW outbound mirror -- which the message's author does not control -- still drops; and a drain-side mirror-probe failure still refuses delivery but says the state could not be verified instead of asserting a mirror appeared. Closes #5911
30b3bed to
3262480
Compare
|
Disposition for the GPT review of
Span note (recurrence watch): this is the 2nd consecutive blocking finding in |
|
Dispositions for the Design Review 🟡 CONCERNS on
|
|
Dispositions for the First Principles Review 🟡 CONCERNS on
|
…get refusals (#7099) `authorize_target` refuses admission from an inline constraint set; `containment_snapshot` (#5978) re-derives the same predicates so the drain can re-assert them. Two hand-maintained spellings of one set, with nothing tying them together. Drift fails OPEN. A refusal added to `authorize_target` alone is enforced at enqueue and never re-checked at delivery, reopening the enqueue->drain window #5978 closed for that constraint only, with nothing red. The existing `assert snap == {...}` pins the snapshot's shape but is not derived from `authorize_target`, so it catches a new snapshot key and misses a new refusal -- it is red in the safe direction and silent in the dangerous one. Takes option 2 from the issue: a parity test rather than the structural refactor of the admission path, which is a wider change than a test needs to be. The refusal set is read from `authorize_target`'s source via AST rather than hand-listed. A hand-listed copy would be a third spelling of the same set, free to drift from the other two -- the failure this pins, reproduced inside the test that pins it. Two classification tables carry the mapping, so a new refusal must be declared either a containment constraint (with the snapshot key that re-asserts it) or explicitly not one (with a reason). Five tests: the parse finds what it claims to, every refusal is classified, no mapping outlives its refusal, every containment refusal has a snapshot key, and no snapshot key lacks a refusal. Verified by injecting each of the four drift directions and confirming the suite goes red for each, then reverting: an unclassified new refusal (the fail-open case), a containment refusal with no snapshot key, a deleted snapshot key, and a renamed refusal leaving a stale mapping. Test-only; no source file is touched. Refs #5994, #5978
Problem / Motivation
A message queued for a busy chat session is delivered later without re-checking the authorization that admitted it. The window is between enqueue and drain: a target authorized while unlinked can be given a channel or outbound mirror link before its queue drains, and the queued prompt then executes and republishes to that channel.
_has_channel_mirror(session_control.py) documents the mechanism; issue #5911 records it as the maintainer-approved follow-up from the PR #5650needs-a-decisionoverride.The gap is caller-independent:
session_sendand a human typing into a busy session share the sameenqueue_or_run_prompt→queue_append→chat_runnerdrain path, so any fix scoped to one caller leaves the others open.Why it matters
The containment
authorize_targetenforces at decision time (linked_session_target/mirrored_targetare both refusals) is a promise the drain did not keep: a prompt admitted under private-session constraints could surface in front of a Slack/Telegram audience its admission never contemplated. This is a TOCTOU authorization bypass on a security boundary the module itself calls deny-by-default.What changed (motivation → approach → change)
Symptom → root cause: authorization is checked at enqueue but never re-asserted at drain, while the constraints it rests on (channel link, outbound mirror, crew mode, memory mode, workspace) are mutable while the entry waits. The issue explicitly declines the 409-refuse-busy-targets shape — the queue is not the defect; the missing re-validation is. So the fix closes the gap at the drain, once, for every caller, using the seams the issue names:
session_control.containment_meta(queue_append's existingmetachannel — classification by metadata, never content). Producers:enqueue_or_run_prompt(composer,session_send, Slack gateway, workflow injection, crew runtime), the composer hold-queue,queue_for_next_turn, the Slack linked-thread enqueue, requeued steers, the plan-mode "Go", and the spec-builder relay._start_next_queued_turnsweeps the queue before anything reads it (_drop_stale_admissions), recomputes the same constraints —authorize_target's own set: linked, mirrored, crew, ephemeral, app, unattended, and workspace (its seventh refusal,workspace_mismatch;slot.workspaceis mutable under a waiting queue via the agent-switch endpoint) — and drops any entry for which a constraint holds at delivery that did not hold at admission. The mirror is compared by identity (channel type + channel + thread), not just presence: a mirror retargeted to a different channel while the entry waited keeps the boolean true at both ends while substituting the audience, so the sweep drops that too (mirror_retarget). No suspension point sits between the sweep and the dequeue.queue_popbroadcast), appends a visible transcript notice naming the changed constraint, and writes a denied SEL record under the slot's effective session key (a linked slot's turns run underlinked_session_key). The drain decision is a session-control authorization, and SEL records both outcomes of every other authorization in this module — so entries that pass re-validation and are consumed write an allowed SEL record too (audit_queued_allow, one row per drained batch), keeping the drain auditable rather than inferable-by-absence.Boundaries that keep designed behaviour working:
_queue_recovery— the single funnel for all in-turn retries, including the fallback-model retry — and the manual continue) stamps fresh admission context at requeue time and the drain re-validates it like any plain entry — a link appearing during the retry window drops the replay, while channel-born recovery machinery keeps working because its stamp records the link as pre-existing._directive_user_origin, the fail-closed provenance the queue already tracks — the manual continue and the queued plan-mode "Go" both derive it from the request identity, so app surfaces never gain it): a user linking their own busy session must not destroy the messages they already typed —api_chatapplies no linked refusal to composer input. A NEW outbound mirror is never exempt (the message's author does not control mirror links), and a RETARGETED mirror is never exempt for the same reason; crew/ephemeral/app/unattended/workspace all still apply. Requeued steers derive the same provenance (the sole steer producer is the composer branch; app isolation confines app requests to app slots).authorize_target's posture). The probe is tri-state so the notice says the state could not be verified rather than asserting a mirror appeared, and the drop logs at WARNING.The enqueue-side probe failure records not mirrored — the least-authorized admission state — so the two sides fail closed in opposite, correct directions.
Tests
test/test_queue_drain_revalidation.py(27 tests, red-before verified against unpatched source):enqueue_or_run_prompt) andsession_send(send_to_target) paths both stamp the admission snapshot; channel-born enqueue recordslinked: True; requeued steers are stamped and carry human provenance on non-app slots.test/test_dashboard_chat.py::TestPlanActionadds a busy-Go test: a plan approval clicked while the slot runs queues with BOTH the admission stamp and authenticated-human provenance, so a human linking their own session before the drain does not lose the approval (apps never gain the flag — same request-identity split asapi_chat).Four existing test doubles updated to the new
queue_append(meta=...)contract, and the shared helper's in-memory mirror store now returns aChannelLink(SessionStore parity — required for identity comparison). Full backend suite: 68,769 passed; the 16 failures + 2 errors reproduce on pristine main in this environment (AF_UNIX path length, xdist host budget, py-spy absence, etc.).Manual verification
N/A — unit coverage sufficient: the drain sweep, both enqueue paths, and the refusal surface are exercised end-to-end in-process, and the change adds no new external integration.
Screenshots / video
Why no screenshot: backend-only change; no frontend files touched. The only user-visible artifact is a transcript notice row rendered through the existing
noticerole machinery.Related Issues
Closes #5911
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)