fix(dashboard): carry a chat_message row's identity through the SSE relay

[cixuuz]

Fixes #8045.

The defect

DashboardState._broadcast() feeds one producer note to two doors. The
WebSocket arm carried cls/meta; the SSE arm in handlers/updates.py
rebuilt a four-field subset (slot/role/content/ts) and dropped both.

meta.mid is the per-row delivery identity _ChatSlot.append stamps, and it is
what a client dedups on (isRedeliveredMessage in chatSlice.ts — "a frame with
NO mid is never deduped"). So the SSE door published frames no consumer can
recognise as a redelivery. Same mid-less door #7981 closed; its changed-file set
never included this serialiser, which is why #8045 was filed as the remaining one.

Scope, stated honestly

/api/stream has no in-repo frontend consumer today — the SPA reads
/api/ws. This is contract integrity, not a live rendering symptom; it does
not fix visible duplicate bubbles, because that is not reachable here. The
endpoint is still served, documented, and proxy-allowlisted
(_PROXY_ALLOWED_PREFIXES forwards a peer's api/stream for the remote-crew
view).

The change

One serialiser, and an explicit authorization decision per transport.

chat_message_frame(note, *, include_metadata) lives in state.py's
shared-helpers section beside parse_cls_meta. Both doors call it, so a field
added to the frame cannot reach one transport and miss the other.

include_metadata is a required keyword and names a property of the
transport, not a preference:

door	downstream per-client filter	include_metadata
WS (_broadcast_note)	yes — _send_ws_all → _ws_client_allowed (deny-by-default event scope) → _serialize_for_client	True, gate decides per socket
SSE (api_stream)	none — _broadcast() fans the raw note to every queue	request["is_dashboard_user"] only

That asymmetry is the whole point, and getting it wrong is what GPT 5.6 caught on
b6276c57c: identical shape is not identical safety. meta carries
tool_input, a live oauth_url, and approval_id, so an unconditional pass on
the unfiltered queue would expose it to any app token granted that route
regardless of its slots:* scope. state.py already documents this exact class
on this exact endpoint (GPT #6789, public-repo status leaked onto /api/stream),
and the established remedy is the one applied here: keep enrichment on the door
that filters.

The SSE auth read is the middleware's POSITIVE signal, taken once per connection
and defaulting to False, so a path that bypasses the middleware withholds
rather than publishes. An app token's frame is byte-for-byte the pre-existing
four-field projection — this withholds new metadata, it does not newly restrict
anyone.

cls/meta remain conditional in both directions when included: absent metadata
stays absent rather than becoming null/{} keys a consumer must special-case.

Tests

Four in test/test_dashboard_updates_coverage.py:

1. test_the_sse_chat_message_frame_carries_the_rows_identity — the defect
   repro. Fails KeyError: 'meta' against the unfixed serialiser.
2. test_a_chat_message_without_meta_gains_no_empty_keys — no invented empty
   keys. Passes either way by design; pins the conditional, not the bug.
3. test_an_app_token_stream_never_receives_row_metadata — the security gate.
   Verified by removing the gate: it fails with tool_input: "secret" present.
4. test_neither_chat_message_door_builds_the_frame_by_hand — source-level
   guard that both doors delegate to the shared serialiser.

On (4): it replaces an earlier byte-parity test that First Principles correctly
showed did not guard its own docstring — its "WS payload" called
chat_message_frame directly, so re-inlining the WS arm was invisible to it.
Deleting it was the reviewer's suggestion, but that would drop what Design Review
asked for, so it became a source-level guard instead (the repo's existing pattern
for this, cf. test_no_consumer_hardcodes_its_own_warn_threshold). It is scoped
to each door's own chat_message branch, because a whole-file scan cannot work
here: the helper is defined in state.py, so its own body reads as a
hand-built frame and its def line satisfies a naive "calls the helper" check. I
verified it stays silent on baseline and fires on both re-inlining directions.

Harness fix: _request_with_queue wires is_dashboard_user to a real mapping —
a bare MagicMock returns a truthy .get(), so test 3 would otherwise have
passed on mock truthiness.

Known-divergent siblings (out of scope)

First Principles counted four arms still hand-built and divergent between the two
doors: refresh (WS sends kinds as a list, SSE the raw comma-string) and
update_progress / artifact_update / session_summary (typed WS envelopes vs
the raw-note SSE notification fallback). Real, and tracked by #8044 rather than
widened into here.

Pattern harvest

Rule candidate: review-prompt
Pattern: a second delivery door rebuilds a shared producer note by hand, and enrichment is added to a payload that feeds both a filtered and an unfiltered transport

Two distinct generalizations, and this PR hit both — the second one the hard way.

Duplicated frame construction. One producer, N transports each re-deriving
the payload, so a new field reaches only the doors someone remembered to edit.
Already filed as its own instances: #7981, #8044, #8005.

Enriching a payload that feeds an unfiltered transport. The sharper and more
dangerous one. _broadcast() fans one note to a door that filters per client and
a door that does not, so any field added centrally silently acquires the weaker
of the two authorization models. GPT #6789 was this exact bug on this exact
endpoint; my own first attempt reproduced it while explicitly arguing for
"parity", which is the tell — parity of shape between transports of unequal
privilege is not a safety property. The reviewable question: "does this payload
feed a transport with no per-client gate, and does the field I am adding carry
anything scope-sensitive?"

Why review-prompt over semgrep: both tells are semantic. A dict literal
enumerating a subset of a note is syntactically indistinguishable from a
deliberately narrow envelope, and "this queue has no downstream filter" is a
property of the fan-out, not of the line being edited.

Verification

• pytest test/test_dashboard_updates_coverage.py — 78 passed
• pytest broadcast/state modules (state.py is on that path) — 106 passed
  across test_midless_broadcast_dedup, test_dashboard_state_ws,
  test_dashboard_state_close_characterization,
  test_slots_broadcast_coalesce, test_outbox_notify_broadcast
• mypy src/kiro_crew/ — clean, 1280 files
• flake8 + isort --check-only — clean
• scripts/check_black_formatting.py — passes; both source files are black-clean
  and unbaselined, and no added line of mine is unformatted

Tool versions match the CI pins (pytest 9.0.3, black 26.3.1, flake8 7.1.0,
mypy 1.14.1, isort 6.0.0).

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Verification confirms the change is sound: is_dashboard_user is a positive signal set by token_auth.py (not _app), defaults to False, Any is imported in state.py, and the asymmetry (WS passes True because it filters per-socket downstream; SSE passes the flag because it has no per-app gate) is load-bearing. App tokens receive the same 4-field frame as before — the change is purely additive for dashboard users, with no regression and no metadata leak. Nothing survives falsification.

No findings.

[OPUS-REVIEWED] 7989963

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

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

All checks confirm the design claims: the middleware sets is_dashboard_user positively, the WS door's _ws_client_allowed gate is deny-by-default per socket, the remote-crew proxy authenticates to the peer with a dashboard session cookie (so the one live /api/stream consumer does receive the enriched frame), and app-token SSE frames stay byte-identical. The scoping of the remaining divergent arms to #8044 is honest and tracked. One observation worth passing on as a suggestion: since meta.mid is a random, non-sensitive delivery identity, an app-token SSE frame could carry a mid-only projection without touching the sensitive fields — that would close the "no mid → never deduped" gap for the remaining client class.

Design-Verdict: PASS

One shared serialiser with a per-transport authorization decision is the correct root-cause shape; the security asymmetry is real and correctly preserved.

Suggestions

• meta.mid is a random identity carrying nothing sensitive; projecting {"mid": ...} alone into app-token SSE frames would let that client class dedup redeliveries too, without exposing tool_input/oauth_url — viable here or in Unify append_and_surface's two frame doors: drop extra=, apply re-redaction to reader-suppressed frame #8044.

[DESIGN-REVIEWED] 7989963

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 7989963dc0eab4770329a5244c2f18a97a0ba651 — 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 claims verified. Writing the review.

First-Principles-Verdict: PASS

A filed contract defect fixed at its cause — one serialiser replaces two hand-built frames — with the security asymmetry and every rider declared and counted.

What this change ships

Intent: make /api/stream's chat frames carry the same per-row delivery identity the WebSocket already carries, so a consumer can recognise a redelivery — a FIX.

1. A dashboard user's SSE chat frame now carries cls/meta (the dedup identity) — justified (filed defect Legacy SSE relay strips meta from chat_message frames — last mid-less door after #7981 #8045)
2. An app token's SSE chat frame stays the old four-field shape — justified (SSE fan-out has no per-client gate; verified _ws_client_allowed exists only on the WS side)
3. Both transports now build the frame through one shared serialiser — justified, cause-level (the hand-built divergence is what produced Legacy SSE relay strips meta from chat_message frames — last mid-less door after #7981 #8045)
4. A source-level test fails if either door re-inlines the frame — justified (repo precedent verified: test/test_autocompact_default.py)
5. Three behaviour tests pin identity-carry, no-empty-keys, and app-token withholding — justified
6. Test harness wires is_dashboard_user to a real mapping — justified (kills a mock-truthiness false pass)
7. Spec paragraph updated in the same commit — justified (AGENTS.md mandate)
8. Four sibling arms stay hand-built (refresh, update_progress, artifact_update, session_summary) — declared, counted (I verified all 4 at state.py:7786-7812), deferred to Unify append_and_surface's two frame doors: drop extra=, apply re-redaction to reader-suppressed frame #8044

Every count in the description held up: request["is_dashboard_user"] is the middleware's positive signal (token_auth.py:2437 et al.), isRedeliveredMessage dedups on meta.mid (chatSlice.ts:90), and /api/stream has zero in-repo frontend consumers (grepped api/stream, EventSource, and /proxy/ across website/src — the SPA reads /api/ws; the instance proxy only calls /proxy/api/chat/slots), exactly as the description states. The change is net-subtractive: two hand-built frame constructions deleted, one helper with two consumers passing genuinely different values added.

Subtractions

• When Unify append_and_surface's two frame doors: drop extra=, apply re-redaction to reader-suppressed frame #8044 resolves the remaining divergent arms, weigh retiring the SSE chat_message arm outright rather than maintaining its enriched form forever — its enriched half has 0 counted in-repo consumers today (greps above); the endpoint's other events and the proxy row are what keep the route alive, not this frame.

[FIRST-PRINCIPLES-REVIEWED] 7989963

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/dashboard/state.py:1942 -- docstring names nonexistent _broadcast_note instead of the actual _broadcast WS arm -> Fix: replace it with _broadcast.

[GPT-REVIEWED] 7989963

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

[cixuuz]

Adopted the Design Review suggestion in b6276c57c rather than deferring it.

The reviewer's point was that the frame is now built twice, held identical only by comments, spec prose and one-sided tests — "exactly how #8045 survived #7981". That is the same argument my own Pattern harvest section makes, so shipping the duplication while claiming the class is real would have been inconsistent. Both takes were available (extract a helper, or add a parity test); I did both, because they answer different halves.

• chat_message_frame(note) now lives in state.py's shared-helpers section, beside parse_cls_meta — the precedent the review named, and already there to keep two readers agreeing. The WS arm and the SSE arm both call it, so the duplication is gone rather than merely tested.
• test_the_sse_frame_is_byte_identical_to_the_websocket_frame drives one note through the real SSE handler and asserts the emitted payload equals chat_message_frame for that note. If either door is re-inlined and drifts, it fails.

One consequence worth naming: this does NOT pre-empt #8044. That issue is about append_and_surface's two frame doors, a different pair; this change only unifies the WS/SSE serialiser for chat_message.

Scope check — state.py is on the broadcast path, so I ran it as such, not just the SSE module: 77 in test_dashboard_updates_coverage.py, 7 in test_midless_broadcast_dedup.py, and 99 across test_dashboard_state_ws.py / _close_characterization / _slots_broadcast_coalesce / _outbox_notify_broadcast. mypy clean (1280 files), flake8 + isort clean, black ratchet passes with both source files black-clean and neither baselined.

The GPT 5.6 lane on the previous head reported "review incomplete" — its first run was cancelled, not failed, and it has re-queued on this head; I am not requesting an override for it.

[cixuuz]

Both lanes on b6276c57c are addressed in f0f0387af. Dispositions:

GPT 5.6 — BLOCKING, accepted as correct. Not requesting an override.

The finding is right and it invalidates the premise my previous commit rested on. I had argued "parity" between the two doors; they are not symmetric. The WS door filters per client downstream of the serialiser (_send_ws_all → _ws_client_allowed, deny-by-default event scope → _serialize_for_client), while _broadcast() puts the raw note on every SSE queue with no gate at all. So unifying the serialiser matched the shape and simultaneously moved meta — tool_input, a live oauth_url, approval_id — onto the transport with no authorization.

Confirming it is not novel, which is the part that convinced me fastest: state.py already documents GPT #6789, where enriching a payload that feeds both doors leaked public-repo status onto this same endpoint, and records that the SSE queue "has NO per-app filtering" and that app tokens can be "allowed that route". Same class, same endpoint, second time.

I took the suggested shape (withhold from app tokens) rather than inventing an SSE scope gate, since giving that transport per-slot filtering is an authorization-architecture change a maintainer should own. chat_message_frame now takes a required include_metadata keyword that names the transport's filtering property; api_stream passes request["is_dashboard_user"], defaulting to False so a middleware-bypassing path withholds. An app token's frame is byte-for-byte the pre-existing four-field projection. Verified by deleting the gate: the new test fails with tool_input: "secret" on the wire.

First Principles — CONCERNS, both items accepted.

The third test did not guard its docstring. Correct, and it was my error: its "WS payload" called chat_message_frame directly, so re-inlining the WS arm was invisible to it. I did not take the recommended deletion, because that drops the guard Design Review asked for on the previous head; instead it is now a source-level guard (test_neither_chat_message_door_builds_the_frame_by_hand), following this repo's existing pattern for exactly this shape (cf. test_no_consumer_hardcodes_its_own_warn_threshold). Worth recording for anyone editing it: it must be scoped to each door's own chat_message branch, not the file — chat_message_frame is defined in state.py, so a whole-file scan reads the helper's own body as a hand-built frame and its def line satisfies a naive "calls the helper" check. I hit both of those while writing it. Verified silent on baseline, firing on both re-inlining directions.

Stale description. Also correct — the body said two tests and claimed a from typing import Any hunk that the second commit had already removed. Body rewritten; it now declares four tests and the count of divergent siblings.

The four divergent sibling arms (refresh list-vs-comma-string, and the three typed-WS-envelope-vs-raw-SSE-fallback events) are real and left to #8044, as you noted.

---

One thing neither lane raised, which I think matters more than my delta. The SSE chat_message frame already ships content — the actual message text — for every slot, and that predates this PR. If an app token can be granted /api/stream (which #6789's comment states), then conversation content for all slots already crosses that unfiltered fan-out with no slots:* check; my change would only have added meta on top. I have deliberately not touched that here, because closing it means giving the SSE transport a real per-slot scope gate, which is the architecture decision I just declined to improvise. Flagging it for a maintainer to file — I am happy to open the issue if that is wanted.

[bolichen97]

Approving on the strength of a full readiness audit of every open PR against main, not a
line-by-line reading of this diff — recording that plainly so the next reader knows what this
stamp does and does not cover.

Verified against this exact head SHA:

• readiness: passed present, and PR Readiness — the one required status context on main
  (ruleset protected-branches) — is success on this head.
• No check run on this head is failure, cancelled, timed_out or still in flight. Skipped
  jobs are path-filtered conditionals, none of them required.
• mergeable: true, and the head is not far enough behind main for its green CI to describe a
  base that no longer exists.
• No surviving reviewer CHANGES_REQUESTED: any such review is on an older commit and therefore
  already dismissed by dismiss_stale_reviews_on_push.
• Every issue comment, inline review comment and review thread was read and classified. Nothing
  left is an unresolved human change request — the remainder is bot review-lane output, resolved
  or outdated threads, explicitly non-blocking suggestions, and author status notes.

Auto-merge (squash) is armed, so this lands once every other ruleset requirement is met.