feat(chat-core): route SideChat's send through the transport over a side wire - #8655
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound RFC slice with forced root-cause placement; the rider that reclassifies ChatEmbed's offline sends as "unconfirmed" deserves human eyes. Watch
Suggestions
[DESIGN-REVIEWED] 9b1d664 |
GPT 5.6 Review — 🔴 changes requested (blocking)GPT 5.6 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- website/src/pages/chat/SideChat.tsx:664 -- Client validation is routed through ErrorNotice Adjudication (Opus 4.8) — is blocking on each finding proportionate?F1 anchors to the AUTOSDE rule [ADJUDICATION] 9b1d664 total=1 uphold=1 downgrade=0 False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of All evidence gathered. The PR embeds before/after images hosted at an earlier commit ( UX-Verdict: CONCERNS Sound receipt copy reusing shipped strings, but this revision commits no screenshots — the blind read never ran, so no new state was seen cold. Watch
Evidence gaps
[UX-REVIEWED] 9b1d664 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All verification done: the AUTOSDE rule mandates the validation-hint relabel, the i18n promotion follows the pinned #4240 pattern, capture pages have 227 siblings of precedent, and the transport file confirms the one sibling gap. Final review: First-Principles-Verdict: CONCERNS Every item names its harm, but the PR's own "one POST can't tell never-left from reset-after-accept" rule stops at 2 of 3 wires — the dashboard wire keeps the hazard. What this change shipsIntent: give the side panel's send the same bounded, classified delivery outcomes every other chat surface got, so a hung or ambiguous send stops losing the user's question. A FIX (of receipt-semantics defects) delivered as the RFC-planned adapter.
(More than 10; the rest — Watch
[FIRST-PRINCIPLES-REVIEWED] 9a934ac |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
c32b53f to
e142a64
Compare
e142a64 to
a11d614
Compare
|
GPT — BLOCKING: new send failures bypass Accepted, fixed. The panel's failure strip now renders through the shared |
|
GPT — BLOCKING: accepted turns become retryable when receipt parsing fails ( Accepted, fixed. A |
|
GPT — BLOCKING: deadline during Accepted, fixed (Design raised the same gap). Between the two calls the wire now checks |
|
Design — Suggestion: check Implemented — same fix as GPT's third blocking finding above ( |
|
UX — Suggestion: Implemented. The unconfirmed notice is now a standing notice held until the next submit (a separate |
|
UX — Suggestion: frame the server reason instead of showing it bare ( Implemented. |
|
UX — Suggestion: chip-send Implemented. A chip send ( |
a11d614 to
f98e761
Compare
f98e761 to
aef86f0
Compare
|
GPT — BLOCKING: pre-turn timeout strands an idle send as pending ( Accepted, fixed. The wire now tracks whether |
|
UX — Suggestion: clear the standing notice when the restored draft is emptied ( Implemented. An effect retires |
ChatPage's composer send and its mid-turn steer each hand-rolled the POST to /api/chat: an AbortController and 10 s timer, the resolves-not-rejects trap, readSendReceipt, and a catch that had to tell an abort from a network failure. The steer path was worse -- a separate api.steerChat helper with no deadline whose failure was only a console line. Both now call sendTurn (chat-core transport) and branch on receipt.status; the receipt policy (error row, composer restore, optimistic confirm, card retirement, ask resolution) is unchanged and stays on the surface. sendTurn gains the two flags this endpoint takes that ChatPage sets: steer (inject into the running turn / skip the sub-agent hold) and colorTheme. api.steerChat has no send-path consumer left on this surface (useSceneInteraction still uses it; its own P2 slice). RFC chat-core extraction, P2 (transport), third slice: after ChatPane (#5909), ChatEmbed (#8599) and SideChat (#8655), every user-facing composer send is on sendTurn.
383e9aa to
b4deb25
Compare
ChatPage's composer send and its mid-turn steer each hand-rolled the POST to /api/chat: an AbortController and 10 s timer, the resolves-not-rejects trap, readSendReceipt, and a catch that had to tell an abort from a network failure. The steer path was worse -- a separate api.steerChat helper with no deadline whose failure was only a console line. Both now call sendTurn (chat-core transport) and branch on receipt.status; the receipt policy (error row, composer restore, optimistic confirm, card retirement, ask resolution) is unchanged and stays on the surface. sendTurn gains the two flags this endpoint takes that ChatPage sets: steer (inject into the running turn / skip the sub-agent hold) and colorTheme. api.steerChat has no send-path consumer left on this surface (useSceneInteraction still uses it; its own P2 slice). RFC chat-core extraction, P2 (transport), third slice: after ChatPane (#5909), ChatEmbed (#8599) and SideChat (#8655), every user-facing composer send is on sendTurn.
60c4aa6 to
1d4fdcc
Compare
|
/ai-review override gpt 1bfdada: SideChat.tsx:209's mergeIntoDraft is the current-slot fast path; line 210's else-branch already persists recovery to per-slot store state (sideHandBackText -> releasedText, drained on the slot's next display), so an unmount or slot-switch takes the else branch and no restored text is lost — this is the fix the finding requests, verified present and covered by the unmount/slot-switch tests. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
|
|
|
|
|
|
|
|
|
…ide wire SideChat is the one surface on a different endpoint family (/side/open + /side/turn) with the opposite receipt semantics: the client helpers resolve JSON on 2xx and reject with an ApiError on non-2xx. A per-surface wire re-expresses that in the fetch seam's shape, so sendTurn classifies the side panel's send by the same rule as every other surface (deadline, refused vs unreadable vs transport failure) while the acceptance body (run_id, queue_id, steer_id, pending, demoted, still_queued) passes through untouched and its handling is unchanged. Receipt policy: refused / transport-error roll back the optimistic bubble and hand the text back merged, with the server's reason when there is one; response-late hands the text back under an unconfirmed notice only when no bubble holds a copy (steer / queue), and leaves an idle send's bubble alone; unknown does nothing. Every existing SideChat test passes unmodified.
|
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: |
Problem / Motivation
SideChatwas the third hand-written send path in the chat-core inventory — and the only one on a different endpoint family (POST /side/openthenPOST /side/turn) with the opposite receipt semantics fromPOST /api/chat?ws=1: the client helpers resolve parsed JSON on 2xx and reject with anApiErroron non-2xx. ItsuseMutationre-learned that contract privately (no deadline; a hung request waited forever; a network failure surfaced as the rawTypeErrormessage).Why it matters
Chat-core extraction RFC P2 (transport): one receipt classification, per-surface wires. The P0 inventory concluded SideChat "needs a second receipt adapter — do not flatten" precisely because its endpoint answers differently; this PR is that adapter. After it, ChatPane (#5909), ChatEmbed (#8599) and SideChat all classify sends through
readSendReceiptbehindsendTurn; only ChatPage remains among the surfaces.What changed (motivation → approach → change)
Side wire (
website/src/pages/chat/sideTurnWire.ts, new). ASendWirefor the side endpoint family. It performs the two-call sequence the panel has always made (api.sideOpenthenapi.sideTurn) and re-expresses the helpers' outcomes in the fetch seam's shape the shared classifier reads: a 2xx body → resolvedokwhosejson()yields it (so{ok}→dispatched,{ok, queued}→queued, side-specific fields pass through untouched); a 2xx whose body could not be read (from/side/turnonly) → resolvedokwhosejson()rejects, i.e.unknown— the server accepted the turn, so this must never hand the text back for a duplicate retry. The client'sj()helper collapsed that case into the sameTypeErrora never-sent request throws (a stream cut mid-body is indistinguishable by type from "Failed to fetch"), soapi.sideTurnnow reads through a phase-preservingjAcceptedthat wraps a post-2xx body-read failure inAcceptedBodyUnreadable(identical non-2xx handling; its resolved type is unchanged, and the 10 existing test files that mockapi.sideTurnare untouched). The class lives inapi/apiError.ts(re-exported fromclient.ts) so the app-sdk and the many tests that mockclient.tswholesale share one identity for theinstanceof; it carries a human message (api.client.accepted_body_unreadable, 12 locales + en-XA) because every scoped-API method of every App Kit app now reads through the tagging helper and app code surfacingerr.messagefrom a malformed 2xx must not render blank.j(),jNullable()andjAccepted()share oneacceptOrThrowfor the non-2xx/auth half, so the two receipt semantics cannot drift; that half also guards the error-body read (r.text().catch(() => r.statusText), as the app-sdk'sjsonFetchalready did), because a non-2xx whose body stream is cut is still a refusal — letting it escape as a raw rejection would make the wires below read it as indeterminate and show "delivery unconfirmed" for a real error. The same hazard is closed at the endpoint's other caller — the/sideslash interceptor inChatInput.tsxnow treatsAcceptedBodyUnreadableas accepted instead of restoring the composer — and at the same root cause in the app-sdk:jsonFetchtags a post-2xxres.text()cut or parse failure the same way, andappSendWiremaps that (not a bareSyntaxError) tounknown, so a mid-body stream cut no longer reads astransport-errorin ChatEmbed either — and a rawfetchrejection on that wire is nowresponse-laterather thantransport-error, because a single POST with no response cannot tell "never left" from "accepted, then the connection reset before headers", and the embed already recoversresponse-late(text back under the unconfirmed notice, retired by the poll'ssendIdecho); anApiErrorfrom either call → resolved!okcarrying the error's friendly message (sorefusedkeeps the reason the panel already displayed, andj()'s auth handling / error recording ran exactly as before); a deadline during/side/turn→AbortError→response-late; a deadline before/side/turnwas sent (a stalled/side/open) → a plain error →transport-error, because nothing was accepted and classifying it as unconfirmed would strand an idle send's optimistic bubble for a turn that will never run — and the sequence stops there, so the turn is never dispatched after the receipt was delivered; a raw rejection after/side/turnwas dispatched (connection reset before headers) →AbortError→response-late, because it cannot be told from a request the server took whose answer was lost, and/side/openjust succeeded on the same link — indeterminate, so the text comes back under the unconfirmed notice rather than as a retry-safe failure that would duplicate the turn; anything else (a raw rejection from/side/openitself: offline, DNS) →transport-error. The abort race itself (settleUnderSignal) now lives once inchat-core/transportand both the app-sdk wire and this one use it.steerrides the wire, not the transport contract: only this endpoint family understands it.Adapting the existing
api.sideOpen/api.sideTurncalls rather than new raw-fetch helpers is deliberate: every SideChat test mocks those two functions, so the whole existing suite runs against the new path unmodified.SideChat.tsx.mutationFnbecomes onesendTurn({ message, slot, wire: sideTurnWire(slot, { steer }) })call. The receipt policy lives at the top ofonSuccess, and everything it hands back to the composer (text, error line, standing notice) is addressed to the originating slot: the panel is one instance re-propped across slots, so a receipt that lands after the user switched slots — or after the panel was closed — must not merge into the draft they are writing elsewhere, and must not be lost while the side session is open. The text goes to the store through the channel a cancel's release already uses (sideHandBackTextaccumulates intoslotSide[slot].releasedText, which the panel drains into the composer whenever it next shows that slot), so it survives the component being unmounted; and the status that explains it (framed error line, standing notice) lives in the store too, per slot (SideState.sendStatus, set bysideSendStatus, cleared by the next submit to that slot), so a restored question always reappears with its "check the transcript before resending" warning and slot A's status never renders while slot B is displayed (the store-side bubble rollback was already per-slot). Two boundaries are deliberate: an explicit close of the side session (sideClosedeletesslotSide[slot]) discards any recovery still in flight for it — the session the send belonged to no longer exists, and resurrecting a closed session to hold a failed question would be worse — exactly as a close before the oldonErrordid; and the panel's composer is one draft shared across slots (pre-existing;useComposerDraftis not per-slot), so a hand-back drained on return to slot A merges into whatever the user has typed since, merged, not replaced, with the same semantics as the cancel-release channel it rides — the alternative, discarding one of the two texts, is the loss this channel exists to prevent. Per-slot composer drafts would be a change to the panel's composer model, not to its transport, and is out of this slice's scope:refused/transport-errortake the path the oldonErrortook (roll back the optimistic bubble, hand the text back viamergeIntoDraft— never for a chip send, which did not consume the draft — and show the server's reason framed throughpages.chatPage.send_failed_with_error, or a connection-framed line);response-latehands the text back under a standing "unconfirmed" notice only when no bubble holds a copy (a steer or queued send — composer already cleared) — standing, not on the 8 s transient TTL, because it describes restored text the user may look back at later, and — for the restored-text variant only — retired when the user empties the draft (they checked, it landed, they deleted the copy); the chip variant restored nothing, so an empty composer says nothing about whether it was seen; a chip send gets its own copy ("re-pick the option") and nothing is merged into a draft the user may be mid-writing; an idle send's bubble is left pending (the ChatPane policy: restoring would invite a duplicate);unknowndoes nothing;dispatched/queuedfall through to the acceptance handling, which is byte-for-byte the previousonSuccessbody.onErrorremains only as the unexpected-throw fallback (sendTurnnever rejects). Steer/queue split, optimistic bubble ownership, raw-text correlation for redacted queue cards, demotion notice — all unchanged.Error strip — already
ErrorNoticeon base. An earlier revision of this PR moved the panel's hand-written<div className="text-danger">strip onto the sharedErrorNotice; main landed the same migration in #8743 before this rebase, so that line is now unchanged context in this diff. What this PR still changes about the strip is only what flows into it: the framed reason (pages.chatPage.send_failed_with_error/send_failed_connection) instead of the rawsendMutation.error.message, and the per-slotsendStatus.errorchannel described above.askAgentstays off, with the base's{/* No hand-off: … */}comment kept as-is.Catalogs — shared, core-owned. The three strings this surface needs (
send_failed_connection,delivery_unconfirmed,delivery_unconfirmed_option) were identical in every locale to ChatEmbed'sappSdk.chatEmbed.*copies from #8599. Per the #4240 precedent they live underpages.chatPage.*(siblings ofsend_failed_with_error). Since this branch was cut, #8689 (ChatPage transport) landed the first two of those shared keys on main and pointedChatPane.tsxatsend_failed_connection, so after the rebase this PR adds onlydelivery_unconfirmed_optionto the shared block, pointsChatEmbed.tsxandSideChat.tsxat the three shared keys, deletes the retiredappSdk.chatEmbed.*set in every locale, and extendssendFailedSharedKey.test.ts(the existing #4240 guard) with the same two invariants over these keys (every locale carries the shared keys; no locale or source file carries a retired one).ChatPane.tsxis no longer in this diff.Not done here, deliberately. The plan sketched hoisting an
isOwnUserRowproof-of-delivery helper into chat-core "when the second consumer appears". SideChat turns out not to be one: its optimistic bubble is reconciled by the store against server frames, not by scanning polled rows, so a hoist would have a single consumer (ChatEmbed) and be exactly the zero-second-consumer generalization the review lanes reject. Deferred until a real second consumer exists.RFC placement. P2 (transport), third slice. Stacked on
feat/chat-core-p2-embed(#8599, review-ready) for theSendWireseam andmintSendId; base re-targets tomainwhen #8599 merges. Parallel to #8631 (P3 ChatEmbed composer), which does not touchSideChat.tsx. Remaining P2: ChatPage, then the four app-local senders inventoried in #8599.Tests
SideChat.sendReceipt.test.tsx(new, 14):ApiErrorrefusal → bubble rolled back, text handed back, framed reason rendered throughErrorNotice(role="alert");TypeErrorfrom/side/open(never left) → connection copy and/side/turnis never called; a rawTypeErrorafter/side/turnwas dispatched → text handed back under the standing unconfirmed notice, norole="alert", no connection copy; a refusal from/side/openis a refusal (and/side/turnis never called); late STEER → text back + standing "Delivery not confirmed" notice that survives 20 s; late CHIP send → "re-pick the option", draft untouched; a deadline during/side/openis a failure (bubble rolled back, text restored, connection copy) and never dispatches/side/turn; emptying the restored draft retires the standing notice; an accepted/side/turnwhose body read was cut (AcceptedBodyUnreadable(TypeError)) → no error, no restore, bubble stays; a late chip send with an empty composer keeps its notice; a refusal landing after a slot switch leaves the other slot's draft alone and is handed back on return; a refusal landing after the panel was unmounted lands in the store's per-slot hand-back buffer with its error, and the next panel for that slot shows both; a slot's unconfirmed notice does not follow the user to another slot and is back on return; late IDLE send → bubble stays, composer stays empty, no notice.sendFailedSharedKey.test.ts(+3): shared receipt keys present in every locale; retired surface keys absent from catalogs and source.SideSlashCommand.test.tsx(+1):/side <q>with an accepted-but-unreadable turn does not reportfailed.appSdkScopedApi.test.tsx(+3) /appSendWire.contract.test.ts(+1): the scoped helper tags post-2xx text/parse failures; a never-leftTypeErrorstays raw; the wire classifies a tagged stream cut asunknown.ApiClient.coverage.test.tsx(+3):api.sideTurn— 2xx with a failing body read →AcceptedBodyUnreadablecarrying the cause; never-left → rawTypeError; non-2xx →ApiError.SideChat.steerQueue.test.tsx(73),SideChat.refresh.test.tsx(3),SideChat.multiturn,.close,.imeEnter,.oversizeQuestion,.planExclusion,.thinking,SideSlashCommand.steer— all pass on the new path, including the two that rejectapi.sideTurnand assert the merged draft restore.npm run typecheck, changed-file ESLint clean.Manual verification
Capture harness (
website/capture/side-chat-send-receipt.*+website/scripts/capture-side-chat-send-receipt.mjs): the real SideChat with/side/openstubbed OK and/side/turnrefused with a 409; the script asserts theErrorNoticetext and the restored composer before capturing.Screenshots / video
Visible delta against the current base (#8599 on main, which already renders the strip through
ErrorNoticevia #8743): the refusal line carries the framed reason (Send failed: {{error}}) instead of the raw error message. The BEFORE image below was captured against a pre-#8743 base and therefore also shows the old bare red strip — read it for the copy, not the container; the AFTER image is this PR'srefusedstate as staged by the committed capture harness (website/capture/side-chat-send-receipt.*). The two standing-notice states (response-latewith a restored draft / with a chip re-pick) reuse the panel's existingtext-[12px] text-mutednotice strip with no new markup; their copy and placement are pinned bySideChat.sendReceipt.test.tsxrather than by a screenshot.Light theme
Related Issues
Chat-core extraction RFC, phase P2 (transport). Builds on #5909 and #8599; parallel to #8631. no linked issue: RFC phase work tracked in the RFC, not an issue.
Checklist
sideTurnWireContribution License Agreement
N/A — repository placeholder only; no OSPO CLA text has been supplied.