fix(gateway): preserve the client sendId through the slot queue drain by CrysisDeu · Pull Request #8853 · kirodotdev/KiroCrew

CrysisDeu · 2026-09-06T00:53:25Z

Problem / Motivation

A dispatched send persists the client-minted meta.sendId on its user row (chat_handlers.py, slot.append("user", …, meta=user_meta)), and a steer does too — accepted (steer_into_running_turn) or requeued (_requeue_unconsumed_steers, #6751). A plain send into a busy slot did not: the busy-slot branch and the sub-agent-hold branch queued the text with only the containment snapshot in the entry's meta, and the drain (_start_next_queued_turn) wrote an id-less user row.

That is the gap the GPT lane blocked #8599 on. ChatEmbed's unconfirmed-send notice needs to prove that its message landed before retiring; with no id on the drained row the only available proof was exact-text equality, which a same-text resend or a cron/sub-agent injection can share — so a genuinely undelivered draft could be silently withdrawn. Both frontend-side workarounds were rejected by GPT and Opus; the server has to keep the id.

What changed

chat_delivery.queue_for_next_turn gains a send_id: str | None = None keyword. When present it is stamped onto the queue entry's meta next to the containment snapshot. Additive: None leaves the entry shape exactly as before.
chat_handlers.api_chat: the busy-slot branch passes normalize_send_id(user_meta.get("sendId")); the sub-agent-hold branch stamps the same key onto its own containment_meta(...) dict. normalize_send_id is the steer path's existing deny-by-default gate (id alphabet, length bound, credential scan) — an unusable value is treated as absent, never truncated or rewritten, since a rewritten id would silently mismatch the client's copy.
chat_runner._start_next_queued_turn: the drain's existing meta union already carries the key onto the row (it is how a merged row names its steer_delivery_ids). One addition for the merge_queued_messages case: when several consumed entries carry distinct ids, the merged row also gets sendIds (every id, queue order). sendId stays present (the union's last writer) so the key is never absent when any entry had one; on the common single-entry row the shape is sendId alone — identical to a dispatched send's row.

No change to the queued: true receipt, the queue_push broadcast, or the queue_id contract.

Tests

test/test_queue_drain_preserve_send_id.py (new, 10 cases):

busy-slot POST with meta.sendId → entry meta carries it; receipt queue_id still names the entry
POST without an id → key absent on the entry (not a falsy value)
unusable ids (empty, outside the alphabet, over SEND_ID_MAX_LEN, non-string) → absent
sub-agent-hold branch → entry carries the id
drained row carries sendId, no sendIds, and no containment plumbing key
drained row for an id-less entry has neither key
merged drain (merge_queued_messages on) of two id-bearing entries + one id-less → sendIds == [a, b], sendId is one of them
queue_for_next_turn seam: id stamped only when given; queue_push payload unchanged

Local: black, isort, flake8, mypy clean on the touched files; the test file is left to CI.

Pattern harvest

Rule candidate: a queue producer that drops request-scoped correlation meta. Pattern: three producers write slot.queue_append(..., meta=containment_meta(...)) for a composer-originated message, and only the steer requeue carried the client's sendId (#6751) — the two plain-send producers built the same meta dict without it. A structural check (semgrep or a unit pin) that every queue_append / queue_insert call reachable from api_chat with a user_meta in scope forwards normalize_send_id(user_meta.get("sendId")) would have caught this at the second producer; alternatively fold the id into containment_meta's caller contract so a producer cannot build the entry meta without deciding about it.

Checklist

One Conventional Commits commit
New deterministic tests cover the behaviour
Self-review completed; code follows project style guidelines
No secrets, credentials, or internal references in the diff

Contribution License Agreement

N/A — repository placeholder only; no OSPO CLA text has been supplied.

A dispatched send persists the client-minted `meta.sendId` on its user row; a send that arrived while the slot was busy (mid-turn, mid-plan, or held behind running sub-agents) went through the slot queue instead, and the queue entry carried only the containment snapshot -- so the row the drain wrote was id-less. A client that has to prove ITS message landed (an unconfirmed-send notice deciding whether to retire) had nothing to match on but text, which a same-text resend or an injection can share. `queue_for_next_turn` gains a `send_id` keyword and both composer-reachable queue producers in `/api/chat` (the busy-slot branch and the sub-agent-hold branch) pass the client's id through `normalize_send_id` -- the same deny-by-default gate the steer path uses, so an unusable value is treated as absent, never rewritten. The drain's existing meta union carries the key onto the row; when `merge_queued_messages` folds several entries into one row, that row now also names every id it stands for as `sendIds`. Additive: a send whose POST carried no usable id stores nothing and the entry / row shape is unchanged. The `queue_push` broadcast is untouched.

github-actions · 2026-09-06T00:56:26Z

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

A real correlation gap (queued sends losing their delivery-proof id), closed additively at the right seam, with the drain contract properly pinned.

Suggestions

The merged-row sendIds list ships with no consumer: website/src reads only meta.sendId (last writer), so non-last senders folded by merge_queued_messages still fall back to content matching. Track the frontend sendIds reader as an explicit follow-up so the persisted key doesn't sit orphaned.

[DESIGN-REVIEWED] e4b38cd

github-actions · 2026-09-06T00:57:28Z

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] e4b38cd

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

github-actions · 2026-09-06T00:58:08Z

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of e4b38cd80a10a9f9b051cb2bc772e35e1df53ff7 — 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. I have what I need for the review.

First-Principles-Verdict: CONCERNS

The fix is real and consumed today, but the sub-agent-hold branch hand-copies the helper this same PR just taught to do the job.

What this change ships

Intent: make a send queued into a busy slot provably deliverable by id, so ChatEmbed's unconfirmed-send notice (#8599) never retires on a text match. A FIX.

Busy-slot queued send's drained row now carries the client's sendId — justified
Sub-agent-hold queued send carries it too — justified, but duplicate of chat_delivery.queue_for_next_turn
queue_for_next_turn gains a send_id keyword — justified, one caller passes it
Merged drained rows gain a new persisted sendIds meta key — declared, zero consumers today
Unusable ids dropped whole via the steer path's existing normalize_send_id gate — justified reuse

The core fix is at cause level: the drain's existing meta union is reused, no new drain branch, and the frontend already reconciles user rows by meta.sendId (chatSlice.ts:173, :3758 — real consumers today, not only pending #8599). Sibling count: of the composer-reachable queue_append producers, both are covered here and the steer requeue was covered by #6751 — zero unfixed siblings.

Watch

sendIds has ZERO consumers (grepped sendIds repo-wide: only this diff's code and tests). Its reader is feat(chat-core): route ChatEmbed's send through the transport wire #8599's merged-row proof; if that PR reads only sendId, this key is dead persisted surface — confirm the reader lands, or defer the key to feat(chat-core): route ChatEmbed's send through the transport wire #8599.

Subtractions

Delete the hold branch's inline block (chat_handlers.py:687-715) and call queue_for_next_turn(..., send_id=...): its redaction chain is byte-identical to sanitize_outbound (chat_delivery.py:106) and its queue_push payload carries the same four keys — the helper has 1 caller and this branch is its second spelling, extended rather than collapsed by this PR (~20 lines removed).

[FIRST-PRINCIPLES-REVIEWED] e4b38cd

github-actions · 2026-09-06T00:58:09Z

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] e4b38cd

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

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

ChatEmbed (the app-sdk's embeddable chat) posted to the bare /api/chat, read its SSE stream as JSON, and called the resulting SyntaxError success; a refused POST rejected the mutation nothing read. Either way the composer had already been cleared, so a failed send looked sent and the text was gone. sendTurn gains an injectable wire (the fetch seam: one POST that resolves on every HTTP status and rejects only when the request never left or the deadline fired); dashboardSendWire is the default, so ChatPane and ChatPage are unchanged. appSendWire re-expresses the scoped AppApi outcomes in that shape (2xx JSON -> ok; 2xx non-JSON -> unknown; non-2xx -> refused with the server's reason via a typed AppApiError; permission denial -> refused with a human sentence; rejected fetch -> transport-error; deadline -> response-late). ChatEmbed sends through sendTurn over that wire. refused / transport-error render an error row and hand the draft back as typed; response-late renders a "Delivery not confirmed" notice and hands the draft back, retired only when the poll shows THIS send's own user row past the send-start point -- by identity only (meta.sendId, or membership in a merged row's meta.sendIds), never by text. The server keeps the id on every path a send can take: a dispatched send persists it on its row, and a send queued behind a busy slot carries it through the queue entry onto the drained row (#8853), so there is no text-matching fallback to false-retire on. unknown does nothing; dispatched / queued let the poll render. mintSendId now lives once in chat-core/transport; ChatPane's and ChatPage's inline minters use it. useChatSession's seed send moves off the bare SSE endpoint onto the same wire (fire-and-forget; the slot is the deliverable). The composer swap (#8631): ChatEmbed's bare <input> + send button becomes the native ChatInput inside a SlotProvider for the embedded slot, narrowed by omission (no upload, voice, agent/model/project chrome, typed command menus, prompt optimizer or slot-approval chrome) with a fail-closed `embedded` preset, a `sending` prop, and sendOnEnter via the new useChatConfig hook. Enter/IME handling, autosize, the running-state send affordance and the composer's a11y contract are now the shared ones; recovered drafts keep their paragraph breaks. Two now-unreferenced app-sdk keys (send, send_message) are removed from every catalog.

CrysisDeu · 2026-09-06T01:38:13Z

First Principles — disposition for e4b38cd80a10a9f9b051cb2bc772e35e1df53ff7

Watch · sendIds has zero consumers in this diff — Confirmed landed downstream. #8599 head d744c972905006aef1aad062ed14279eb93339fc reads it: isOwnUserRow (website/src/app-sdk/ChatEmbed.tsx) accepts a row whose meta.sendId equals the send's id or whose meta.sendIds array contains it, and a test there pins the merged-row case ("a merged queued row that names this send in meta.sendIds is proof"). feat(chat-core): route ChatEmbed's send through the transport wire #8599 is stacked on this PR and declares the dependency in its body, so the key is not dead surface — its one reader is the PR this one exists to unblock.
Subtraction · collapse the sub-agent-hold branch onto queue_for_next_turn(..., send_id=...) — Agreed; deferred, not declined. The two blocks are equivalent (sanitize_outbound is the same two redactions, same four queue_push keys), so the collapse is a pure ~20-line deletion with no behaviour change. This PR is currently all-green and is the gate on feat(chat-core): route ChatEmbed's send through the transport wire #8599's GPT lane; re-pushing for a refactor costs both PRs a full review round. Folding it here is a one-line ask if the maintainer prefers it before merge; otherwise it rides with the next change to that branch of api_chat.

chenmingwei23

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix threading client sendId through the slot queue drain so a queued send's row carries the correlation id, 4 files with a clear root cause.

bolichen97

Approved after a full Tech Lead review of e4b38cd80a10a9f9b051cb2bc772e35e1df53ff7.

Correctness of the id preservation, verified against the code rather than the description:

containment_meta(state, slot) returns a fresh dict per call (session_control.py), so stamping meta["sendId"] on it cannot pollute shared state.
The drain's union (_drained_meta.update(...) over consumed) does carry the entry key onto the row it appends, and the containment plumbing key is still excluded — so the queued row lands with the same meta.sendId shape a dispatched send's row gets from slot.append(..., meta=user_meta).
No id can land on the wrong role: _dequeue_next_message breaks at the first is_system_injection_item, so a merged batch is only composer-originated entries — a cron notification or sub-agent completion can never fold a sendId onto an inject/subagent row.
No loss or collision: the accumulator dedupes, sendIds is written only when several distinct ids were consumed, and sendId stays as the union's last writer so the key is never absent when any entry had one.
No frontend regression. The old id-less drained row reconciled through the else if (meta?.mid) fallback in applyMessageToArray, which requires last.content === content on the tail row; the id-bearing row now goes through reconcileOptimisticEcho, a 50-row backward scan by sendId. That is strictly wider, and the merged-row case never matched the old exact-content fallback either (the row text is [N queued messages merged]…), so nothing that used to reconcile stops reconciling.
normalize_send_id is the right gate to reuse: deny-by-default on alphabet, length, and a redact_credentials scan, and an unusable value is treated as absent rather than rewritten — a rewritten id would silently mismatch the client's copy.
Contracts held: queue_push payload is unchanged (no sendId on the broadcast), the queued: true receipt still names the entry, queue_id untouched.

Checks: PR Readiness success; the PR Hygiene failure at 00:54 is superseded by the 01:16 success on the same head. GPT 5.6, Opus 4.8, Design, UX all PASS; SAST and CodeQL clean. Diff is 4 files / +288-2 with no website/src or electron change, so the UX-review ruleset does not apply.

First Principles CONCERNS treated as advisory, and I agree with the author's disposition: sendIds has its reader in #8599 (the PR this exists to unblock), and collapsing the sub-agent-hold branch onto queue_for_next_turn(..., send_id=...) is a pure deletion worth doing on the next change to that branch rather than costing two PRs a full review round now.

iamwhatever

Tier 1 auto-approve: fix (4 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: queue entries carried only the containment snapshot, so a send that landed while the slot was busy produced an id-less drained row -- the fix stamps the already-normalized client meta.sendId onto the queue entry (both busy-slot and sub-agent-hold branches) and adds sendIds on a merged row, leaving the single-entry shape byte-identical to a dispatched send.

ChatEmbed (the app-sdk's embeddable chat) posted to the bare /api/chat, read its SSE stream as JSON, and called the resulting SyntaxError success; a refused POST rejected the mutation nothing read. Either way the composer had already been cleared, so a failed send looked sent and the text was gone. sendTurn gains an injectable wire (the fetch seam: one POST that resolves on every HTTP status and rejects only when the request never left or the deadline fired); dashboardSendWire is the default, so ChatPane and ChatPage are unchanged. appSendWire re-expresses the scoped AppApi outcomes in that shape (2xx JSON -> ok; 2xx non-JSON -> unknown; non-2xx -> refused with the server's reason via a typed AppApiError; permission denial -> refused with a human sentence; rejected fetch -> transport-error; deadline -> response-late). ChatEmbed sends through sendTurn over that wire. refused / transport-error render an error row and hand the draft back as typed; response-late renders a "Delivery not confirmed" notice and hands the draft back, retired only when the poll shows THIS send's own user row past the send-start point -- by identity only (meta.sendId, or membership in a merged row's meta.sendIds), never by text. The server keeps the id on every path a send can take: a dispatched send persists it on its row, and a send queued behind a busy slot carries it through the queue entry onto the drained row (#8853), so there is no text-matching fallback to false-retire on. unknown does nothing; dispatched / queued let the poll render. mintSendId now lives once in chat-core/transport; ChatPane's and ChatPage's inline minters use it. useChatSession's seed send moves off the bare SSE endpoint onto the same wire (fire-and-forget; the slot is the deliverable). The composer swap (#8631): ChatEmbed's bare <input> + send button becomes the native ChatInput inside a SlotProvider for the embedded slot, narrowed by omission (no upload, voice, agent/model/project chrome, typed command menus, prompt optimizer or slot-approval chrome) with a fail-closed `embedded` preset, a `sending` prop, and sendOnEnter via the new useChatConfig hook. Enter/IME handling, autosize, the running-state send affordance and the composer's a11y contract are now the shared ones; recovered drafts keep their paragraph breaks. Two now-unreferenced app-sdk keys (send, send_message) are removed from every catalog.

CrysisDeu requested a review from a team as a code owner September 6, 2026 00:53

CrysisDeu requested a review from pepmach September 6, 2026 00:53

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

CrysisDeu mentioned this pull request Sep 5, 2026

feat(chat-core): route ChatEmbed's send through the transport wire #8599

Open

5 tasks

chenmingwei23 enabled auto-merge (squash) September 6, 2026 02:03

chenmingwei23 approved these changes Sep 6, 2026

View reviewed changes

chenmingwei23 merged commit 16ec93b into main Sep 6, 2026
71 of 72 checks passed

chenmingwei23 deleted the fix/gateway-queue-drain-preserve-sendid branch September 6, 2026 02:03

github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 6, 2026

bolichen97 approved these changes Sep 6, 2026

View reviewed changes

iamwhatever approved these changes Sep 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix(gateway): preserve the client sendId through the slot queue drain - #8853

fix(gateway): preserve the client sendId through the slot queue drain#8853
chenmingwei23 merged 1 commit into
mainfrom
fix/gateway-queue-drain-preserve-sendid

CrysisDeu commented Sep 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 6, 2026

Uh oh!

github-actions Bot commented Sep 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 6, 2026

Uh oh!

github-actions Bot commented Sep 6, 2026

Uh oh!

CrysisDeu commented Sep 6, 2026

Uh oh!

chenmingwei23 left a comment

Uh oh!

Uh oh!

bolichen97 left a comment

Uh oh!

iamwhatever left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

CrysisDeu commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem / Motivation

What changed

Tests

Related

Pattern harvest

Checklist

Contribution License Agreement

Uh oh!

github-actions Bot commented Sep 6, 2026

Design Review (Fable 5) — ✅ PASS

Suggestions

Uh oh!

github-actions Bot commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GPT 5.6 Review — ✅ no blocking findings

Uh oh!

github-actions Bot commented Sep 6, 2026

First Principles Review (Fable 5) — 🟡 CONCERNS

What this change ships

Watch

Subtractions

Uh oh!

github-actions Bot commented Sep 6, 2026

Opus 4.8 Review — ✅ no blocking findings

Uh oh!

CrysisDeu commented Sep 6, 2026

Uh oh!

chenmingwei23 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bolichen97 left a comment

Choose a reason for hiding this comment

Uh oh!

iamwhatever left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CrysisDeu commented Sep 6, 2026 •

edited

Loading

github-actions Bot commented Sep 6, 2026 •

edited

Loading