Skip to content

fix(dashboard): re-assert queued prompt containment at drain (#5911) - #5978

Merged
kyleseaman merged 1 commit into
mainfrom
fix/queue-drain-revalidate-5911
Aug 26, 2026
Merged

fix(dashboard): re-assert queued prompt containment at drain (#5911)#5978
kyleseaman merged 1 commit into
mainfrom
fix/queue-drain-revalidate-5911

Conversation

@NicholasRBowers

@NicholasRBowers NicholasRBowers commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

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 #5650 needs-a-decision override.

The gap is caller-independent: session_send and a human typing into a busy session share the same enqueue_or_run_promptqueue_appendchat_runner drain path, so any fix scoped to one caller leaves the others open.

Why it matters

The containment authorize_target enforces at decision time (linked_session_target / mirrored_target are 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:

  1. Tag at enqueue. Every producer of plain (user-speech) queue entries stamps the admission-time containment snapshot on the entry via session_control.containment_meta (queue_append's existing meta channel — 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.
  2. Re-validate at drain. _start_next_queued_turn sweeps 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.workspace is 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.
  3. Loud refusal, audited both ways. A drop retracts the frontend queue card (unconditional queue_pop broadcast), 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 under linked_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:

  • Unmarked plain entries fail closed against the boolean constraint set, so an untagged producer can never ride a queued prompt past a boundary the tagged paths respect. Workspace is compared only when the entry recorded one — there is no least-authorized workspace to assume.
  • Structural exemption is narrow: cron notifications and sub-agent completions only — runner machinery minted fresh by trusted internal producers, which channel-born sessions receive by design. Synthetic-recovery entries are NOT exempt: a recovery replays externally admitted content verbatim under a fresh queue id, so every recovery producer (_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.
  • A constraint already held at admission is not a change: channel-born sessions keep draining their own thread's messages.
  • Authenticated-human entries are exempt from the LINKED constraint only (_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_chat applies 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).
  • A drain-side mirror-probe failure refuses delivery unconditionally — even for an entry admitted under a mirror, because the audience may have been retargeted since admission and an unreadable store leaves no identity to compare (fail closed, matching 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):

  • Human-typed (enqueue_or_run_prompt) and session_send (send_to_target) paths both stamp the admission snapshot; channel-born enqueue records linked: True; requeued steers are stamped and carry human provenance on non-app slots.
  • Queued-then-linked and queued-then-mirrored drop with a visible notice and a denied SEL record filed under the effective session key; a mirror retargeted to a different channel while queued drops (directive provenance does not exempt it) while an unchanged admitted mirror still drains; queued-then-unchanged drains normally (including linked-from-birth).
  • Workspace change drops a marked entry; unmarked entries skip the workspace comparison; unmarked entries fail closed on held boolean constraints and survive in unconstrained slots; cron and sub-agent kinds are exempt; an unmarked recovery entry fails closed, and a stamped recovery entry follows its own admission (channel-born recovery survives, link-during-retry drops).
  • Directive-origin entries survive linking but still drop on a workspace change; the card-retraction broadcast fires without a placeholder row; probe failure records opposite fail-closed directions per side and produces the "could not be verified" wording; malformed snapshots degrade to the fail-closed baseline; a surviving entry's snapshot is stripped from the persisted transcript row; a consumed (re-validated) batch emits exactly one allowed SEL record.

test/test_dashboard_chat.py::TestPlanAction adds 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 as api_chat).

Four existing test doubles updated to the new queue_append(meta=...) contract, and the shared helper's in-memory mirror store now returns a ChannelLink (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 notice role machinery.

Related Issues

Closes #5911

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — module docstrings updated where behaviour changed
  • No secrets, credentials, or internal references in the diff

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

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

  • The stamp invariant is distributed: every plain-entry producer must remember containment_meta(...) at enqueue, and this PR itself had to chase nine call sites (spec_builder's comment calls its stamp "load-bearing" because an unmarked entry in an app slot always drops). A future producer that forgets works fine in an unconstrained dashboard slot and silently destroys user speech in linked/app/crew/ephemeral slots — with a notice claiming the constraint "appeared" when it was there all along. Fail-closed is the right direction for the boundary, but the residual failure mode is quiet message loss found only in constrained contexts.

Suggestions

  • Make the stamp structurally unforgettable: require the containment snapshot (or an explicit exemption kind) as a queue_append/queue_insert parameter, so an unstamped producer fails at development time instead of dropping entries at drain.

[DESIGN-REVIEWED] 3262480

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3262480

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 32624801a035e9f8625025b142e47a92590d34b9: <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 Aug 26, 2026
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 32624801a035e9f8625025b142e47a92590d34b9 — 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.

All checks are done. Every main-queue producer is stamped or exempt-kind, authorize_target does log allowed outcomes at 4 sites (the claimed precedent is real), _AUDIENCE_CONSTRAINTS has exactly one member and one consumer, and the side-conversation queue drains through its own path outside this defect's scope. Here is the review.

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 ships

Intent: stop a queued prompt from executing after the session's containment changed between enqueue and drain (TOCTOU, issue #5911) — a FIX.

  1. Drain drops queued prompts whose containment newly holds — justified (named boundary, maintainer-approved issue)
  2. Every plain-queue producer stamps admission-time containment — justified (mechanism of the fix)
  3. Drop is loud: card retracted, notice, denied SEL record — justified
  4. Mirror compared by identity, catching retargets — justified (same cause; boolean cannot see it)
  5. Workspace change drops the entry — justified (re-asserts authorize_target's own workspace_mismatch)
  6. Unmarked entries fail closed — justified (closes the untagged-producer hole at the cause)
  7. Continue/busy-Go now carry caller provenance — rides along, but required so the fix doesn't destroy those flows
  8. Consumed entries write an "allowed" SEL row per batch — declared rider
  9. Fallback-model retry rerouted through _queue_recovery — justified (retries must carry stamps)
  10. chat_orchestrator.py + 3 test files reformatted, pruned from black baseline — rides along, unrelated

Watch

  • audit_queued_allow (session_control.py) is not part of the reported defect: the deny record alone closes it. Its precedent is real — 4 outcome="allowed" sites exist in the module — but it adds one SEL row per drained batch on every busy-session queue, forever, to make the drain "auditable rather than inferable-by-absence." A human should weigh that volume.
  • The sweep's stamped/exempt coverage was verified by count: all queue_append/queue_insert producers in src/ are stamped, exempt-kind (cron/subagent), or on the separate side-conversation drain; the fail-closed default backstops any future miss.

Subtractions

  • Defer the black reformatting (chat_orchestrator.py, test_handler_link_intercept.py, test_slack_handler_coverage.py, test_slack_handler_more_coverage.py + baseline prunes) to its own commit — AGENTS.md itself says baseline formatting goes "in its own commit," and it is ~250 lines of noise in a security diff.
  • Drop _AUDIENCE_CONSTRAINTS — a frozenset with one member and one consumer (session_control.py:419); compare name == "linked" directly.

[FIRST-PRINCIPLES-REVIEWED] 3262480

@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I'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 (_drop_stale_admissions), the containment snapshot/compare (containment_snapshot, newly_held_constraints), the fail-closed directions (probe failure inverted between enqueue and drain), the directive-user-origin exemption scoped to linked only, the mirror-identity retarget comparison, the exempt kinds, the persisted-meta stripping, and every stamped producer.

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 32624801a035e9f8625025b142e47a92590d34b9.

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

@NicholasRBowers
NicholasRBowers force-pushed the fix/queue-drain-revalidate-5911 branch from 2889949 to 472ddc9 Compare August 26, 2026 02:42
@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 Aug 26, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • Recovery retries bypass containment revalidation (chat_runner.py, _drop_stale_admissions orchestration exemption) — fixed in 472ddc9.

The finding holds: a synthetic-recovery entry replays externally admitted content verbatim under a fresh queue id, so the blanket orchestration exemption let a retry execute after a link/mirror appeared during the recovery window. The proposed remedy (revalidate recovery entries fail-closed) is applied with one refinement so it does not destroy recovery machinery in channel-born sessions: the exemption is narrowed to cron notifications and sub-agent completions only, and every recovery producer now stamps fresh admission context at requeue time — _queue_recovery (the single funnel for all 14 in-turn recovery inserts, which also carries the turn's directive provenance forward) and the manual continue handler. An unmarked recovery entry fails closed exactly as demanded; a stamped one follows its own admission, so a channel-born session's recoveries keep draining (linked recorded pre-existing) while a link appearing during the retry window drops the replay with the visible notice + SEL record. Locked in by test_unmarked_recovery_entry_fails_closed and test_stamped_recovery_entry_follows_its_admission.

@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 Aug 26, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/queue-drain-revalidate-5911 branch from 472ddc9 to ecd0265 Compare August 26, 2026 03:04
@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 Aug 26, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • Model-fallback recovery re-queue is unstamped, so the drain destroys it in channel-linked/unattended sessions (chat_runner.py, fallback-swap branch) — fixed in ecd0265.

The finding holds: this was the one recovery producer calling slot.queue_insert directly instead of _queue_recovery, so the retry carried no admission stamp and the fail-closed re-check would discard it in exactly the long-running linked/unattended sessions most likely to hit throttle fallback. Routed through _queue_recovery as proposed, matching the same-model retry directly above it, which also carries the turn's directive provenance and consumption-settlement callbacks forward. The four CI test failures on this head (TestAcpProcessDiedRecovery ×3, TestRunnerWiring ×1) were whole-dict queue-shape assertions that now also expect the admission stamp; updated to echo it the same way they echo the generated id.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • App Continue requests gain human provenance (chat_handlers.py, api_chat_slot_continue) — fixed in ecd0265.

The finding holds: the manual-continue insert set directive_user_origin=True unconditionally, so an app hitting Continue on its own slot would mint the authenticated-human provenance that gates session-mutating effects. Now derived from the request identity exactly as api_chat does — not bool(request.get("app", "")) — so a dashboard human keeps the flag and an app surface never gains it.

  • Slack messages bypass new-mirror revalidation (_AUDIENCE_CONSTRAINTS includes "mirrored") — fixed in ecd0265.

The finding holds: directive content can be authored by any allowed human in a linked thread, while only the session owner adds outbound mirror links — author and audience-controller are different people, so exempting mirrored for directive entries reopened the republication window for a mirror added after enqueue. The exemption is narrowed to linked only (where the author typed into the session's own surface and the linker IS that surface's owner); a NEW outbound mirror now drops directive entries like everything else. Locked in by test_directive_user_origin_exempts_linked_only, which asserts the linked survival, the new-mirror drop, and the workspace drop on the same directive entry.

@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 Aug 26, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/queue-drain-revalidate-5911 branch from ecd0265 to 39d54aa Compare August 26, 2026 03:26
@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 Aug 26, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • Successful drain authorization is not audited (chat_runner.py, _drop_stale_admissions / drain consumption) — fixed in 39d54aa.

The finding holds by the module's own convention: session_control audits allowed outcomes on every other permission decision (authorize_target's operations log both directions), and the drain re-validation only logged denies. Entries that pass re-validation now write an allowed SEL row via audit_queued_allow, sharing the deny path's writer (queue_drain_revalidation, effective session key, queue ids in metadata). Deliberately emitted at CONSUMPTION — the moment the entries actually become a turn — rather than per sweep pass, so an entry that waits across several drains produces one row when it executes instead of one per re-check; exempt cron/sub-agent kinds were never subject to the decision and are not counted as one. Locked in by test_consumed_entry_emits_an_allowed_audit. (Span note: second blocking round touching chat_runner:_drop_stale_admissions — round 1 was the recovery exemption; if a third lands here the next step is a restructure round, not another point fix.)

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 26, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Aug 26, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Dispositions for the Design Review 🟡 CONCERNS on 39d54aa52:

  • Watch: containment_snapshot re-derives authorize_target's seven refusals as a second hand-maintained spelling; an eighth refusal would fail open at drainaccepted-and-deferred.

    The concern holds: the two constraint sets are pinned only by discipline today, and drift fails open for the new constraint. Converging them (shared per-slot predicate table both consume) is a structural refactor of authorize_target's admission path — wider than this PR's fix and re-arming every reviewer on untouched admission code if folded in here. Deferred to Converge authorize_target and containment_snapshot onto one spelling of the containment constraint set #5994, which names both acceptable shapes (shared predicate table preferred; parity-pinning test as the floor) and cites the exact sites.

  • Suggestion: auto-stamp at the queue_append/queue_insert chokepoint via a _ChatSlot snapshot provider, eliminating the nine per-producer stamp sites and their deferred importsaccepted-and-deferred.

    Sound direction and it composes with the predicate-table refactor, so it is folded into the same follow-up (Converge authorize_target and containment_snapshot onto one spelling of the containment constraint set #5994) rather than a second issue. In the interim the failure direction of a forgotten stamp is closed-not-open: unmarked plain entries fail closed against the boolean constraint set, and the drop is loud (transcript notice + SEL record), so a missing stamp is a visible functional regression rather than a silent bypass.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Dispositions for the First Principles Review 🟡 CONCERNS on 39d54aa52:

  • Watch/Subtraction: audit_queued_allow is undeclared in the PR description, and the allow rows could be deferred until someone names a readerfixed (declaration) / rebutted (removal).

    The declaration gap was real and is fixed: the PR body's "Loud refusal" item now declares the allow-side SEL record, its rationale (SEL records both outcomes of every other authorization in this module), and its cost shape (one row per drained batch, not per entry), and the Tests section names the test that locks it in. Removing the audit is rebutted on adjudication grounds: the GPT lane's round-3 blocking finding was precisely that the allow side of the drain decision was unaudited (see the target=gpt head=ecd02656d disposition above), and it was closed by adding audit_queued_allow. Subtracting it would re-open an adjudicated blocker to satisfy an advisory subtraction; the batch-level granularity already implements the "minimal surface" instinct behind the suggestion.

  • Watch: second spelling of the constraint set (containment_snapshot vs authorize_target inline reads); Subtraction: converge authorize_target onto the snapshotaccepted-and-deferred.

    Same finding as the Design Review watch, same disposition: legitimate, drift fails open, and the convergence is deferred to Converge authorize_target and containment_snapshot onto one spelling of the containment constraint set #5994 because rewriting authorize_target's admission path inside this green single-commit fix widens the diff beyond the PR's purpose. The follow-up names the shared-predicate-table shape (which is exactly this subtraction, in either direction of convergence) and the parity-test floor.

  • Rides along: three test files reformatted wholesale and pruned from the black baseline (AGENTS.md: baseline formatting goes in its own commit)rebutted.

    Each of the three files is substantively modified by this PR (queue-shape assertions updated to expect the containment stamp), and the repo's black gate requires touched files to be formatted — so the reformat is a consequence of touching them, not an independent formatting sweep. A separate formatting commit is structurally unavailable here: this repo enforces one commit per PR, and splitting the reformat into its own PR would leave this PR's CI red on the black gate until the other landed. The baseline prune is the mechanical companion of formatting a file (a formatted file must leave the exemption list or the ratchet check fails).

@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 Aug 26, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/queue-drain-revalidate-5911 branch from 39d54aa to 960a3dc Compare August 26, 2026 04:24
@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 Aug 26, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Dispositions for the GPT review of 39d54aa52 (both addressed in 960a3dc2e):

  • Queued plan approvals lose human provenance (chat_orchestrator.py, busy-Go queue_append)fixed in 960a3dc2e.

    Verified legitimate: the busy-Go enqueue was the one authenticated-human producer stamping containment without directive_user_origin, so a human clicking Go and then linking their own session would have their explicit approval dropped at drain — the exact user-speech-destruction case the linked-constraint exemption exists for. Fixed with the same request-identity derivation as api_chat and the manual continue (not bool(request.get("app", "")) — the auth middleware stamps request["app"] for app-token callers, so app surfaces never gain the flag). Locked in by test_busy_go_queues_with_stamp_and_human_provenance (test_dashboard_chat.py::TestPlanAction).

  • FINDING: relinking mirror A to B leaves mirrored true, delivering queued content to audience B (session_control.py)fixed in 960a3dc2e.

    Advisory, but verified real and in-scope: set_mirror_link supports rebinding to another location, and the boolean snapshot cannot see an audience substitution that keeps mirrored true across the wait. The probe now returns the mirror's identity (channel type + channel + thread, tri-state preserved: "" = none, None = store unreadable), the snapshot records mirror_identity when the probe answered, and the drain compares it like workspace — a changed identity drops the entry as mirror_retarget with its own notice wording, never exempt for directive entries (the author does not control mirror links). Probe failure omits the key, matching the boolean's adjudicated treatment of an admission-mirrored entry under a failed drain probe (unverifiable ≠ evidence of change). Locked in by test_mirror_retargeted_while_queued_drops and the false-drop guard test_mirror_unchanged_identity_still_drains; the shared test helper's mirror store now returns a ChannelLink for SessionStore parity.

@NicholasRBowers
NicholasRBowers force-pushed the fix/queue-drain-revalidate-5911 branch from 960a3dc to 30b3bed Compare August 26, 2026 04:38
@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 Aug 26, 2026
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
@NicholasRBowers
NicholasRBowers force-pushed the fix/queue-drain-revalidate-5911 branch from 30b3bed to 3262480 Compare August 26, 2026 04:55
@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 Aug 26, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Disposition for the GPT review of 30b3bed46 (addressed in 32624801a):

  • Mirror probe failure permits unverified queued delivery (session_control.py, newly_held_constraints)fixed in 32624801a.

    Verified legitimate as a posture inconsistency: an entry admitted under a mirror delivered at drain even when the probe failed, because mirrored True→True read as "not a change" and the probe-failure snapshot carries no identity to compare — so an unverifiable (possibly retargeted) audience received queued content, contrary to authorize_target's refuse-on-unreadable rule. mirror_unverified now forces the mirror constraint regardless of the admission snapshot; the notice keeps the "could not be verified" wording. Locked in by an extended test_snapshot_probe_failure_directions (unit: admitted-mirrored + unverified ⇒ constraint) and the new end-to-end test_admitted_mirrored_entry_drops_on_drain_probe_failure. For the record, the Opus lane dropped this same candidate on reachability (get_mirror_link is an in-memory lookup with no transient failure mode); the fix is kept because it costs nothing on the healthy path and removes the posture inconsistency either way.

Span note (recurrence watch): this is the 2nd consecutive blocking finding in session_control.py:newly_held_constraints (round 4: mirror identity/retarget; round 5: probe-failure fail-open). One more blocking finding in this span triggers a restructure round (single invariant over the constraint-comparison table) instead of another point patch, per the same-span stall rule. The broader constraint-set convergence is already tracked in #5994.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 26, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Dispositions for the Design Review 🟡 CONCERNS on 32624801a:

  • Watch: the stamp invariant is distributed across producers; a future producer that forgets silently destroys user speech in constrained slots. Suggestion: require the snapshot (or an explicit exemption kind) as a queue_append/queue_insert parameter so an unstamped producer fails at development timeaccepted-and-deferred (already tracked).

    Same structural concern as this lane's previous verdict, now with a sharper shape (required parameter vs. auto-stamp provider) — both are the chokepoint design already deferred to Converge authorize_target and containment_snapshot onto one spelling of the containment constraint set #5994, whose body explicitly folds the enqueue-chokepoint stamping into the shared-predicate-table refactor; I have noted the required-parameter variant there as the stricter option. Deferred rather than folded in here because changing queue_append's signature touches every producer including the exempt ones — a wider admission-path refactor than this fix's scope, on a PR that is otherwise green. Interim risk posture is as the verdict itself states: the residual failure direction is closed-not-open, and a drop is loud (transcript notice + denied SEL row), so a forgotten stamp is a visible functional bug rather than a silent boundary bypass.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Dispositions for the First Principles Review 🟡 CONCERNS on 32624801a:

  • Watch: audit_queued_allow adds one SEL row per drained batch forever; a human should weigh that volumeneeds-a-decision (put to the maintainer here).

    The capability is declared in the PR body with its rationale (SEL records both outcomes of every other authorization in this module) and its cost shape (one row per drained batch, not per entry), and it closed a GPT blocking finding on ecd02656d ("allow side of the drain decision unaudited"). Whether that audit volume is wanted long-term is genuinely the maintainer's call: @NicholasRBowers — keep the allow-side row (current state, symmetric with authorize_target's convention), or drop to deny-only after merge? Removing it later is a two-line change plus one test; nothing else depends on it.

  • Watch: stamped/exempt coverage verified by count; fail-closed default backstops future missesrebutted as a concern (it is the verification we rely on).

    This bullet records the review's own verification rather than demanding a change: all producers are stamped, exempt-kind, or on the separate drain, and an unmarked future producer fails closed with a loud drop. The structural elimination of the per-producer obligation is tracked in Converge authorize_target and containment_snapshot onto one spelling of the containment constraint set #5994.

  • Subtraction: defer the black reformatting + baseline prunes to their own commit (~250 lines of noise in a security diff)rebutted (same finding as this lane's previous verdict, same structural reason).

    Every reformatted file is substantively modified by this PR, the repo's black ratchet requires touched files formatted and pruned from the baseline (CI enforced it at 960a3dc2e, where the missing prune was a red check), and the repo's one-commit-per-PR rule leaves no second commit to put it in. A separate formatting PR would leave this PR red on the black gate until it landed.

  • Subtraction: drop _AUDIENCE_CONSTRAINTS (one member, one consumer) and compare name == "linked" directlyrebutted (proportionality).

    Legitimate simplification instinct, but the named frozenset is where the exemption's load-bearing rationale lives (the docstring-adjacent comment explaining why linked is exempt for directive entries and mirrored deliberately is not — the exact distinction two review rounds interrogated). Inlining the literal saves three lines and deletes the named anchor that documents the policy; the set also gives Converge authorize_target and containment_snapshot onto one spelling of the containment constraint set #5994's predicate-table refactor a seam to converge on. Keeping correct, documented code unchanged.

@kyleseaman
kyleseaman merged commit 7f64e10 into main Aug 26, 2026
73 of 75 checks passed
@kyleseaman
kyleseaman deleted the fix/queue-drain-revalidate-5911 branch August 26, 2026 17:16
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 26, 2026
bolichen97 pushed a commit that referenced this pull request Aug 30, 2026
…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
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.

Queued prompts drain without re-validating the authorization that admitted them

2 participants