Skip to content

feat(chat-core): route SideChat's send through the transport over a side wire - #8655

Merged
iamwhatever merged 1 commit into
feat/chat-core-p2-embedfrom
feat/chat-core-p2-sidechat
Sep 7, 2026
Merged

feat(chat-core): route SideChat's send through the transport over a side wire#8655
iamwhatever merged 1 commit into
feat/chat-core-p2-embedfrom
feat/chat-core-p2-sidechat

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

SideChat was the third hand-written send path in the chat-core inventory — and the only one on a different endpoint family (POST /side/open then POST /side/turn) with the opposite receipt semantics from POST /api/chat?ws=1: the client helpers resolve parsed JSON on 2xx and reject with an ApiError on non-2xx. Its useMutation re-learned that contract privately (no deadline; a hung request waited forever; a network failure surfaced as the raw TypeError message).

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 readSendReceipt behind sendTurn; only ChatPage remains among the surfaces.

What changed (motivation → approach → change)

Side wire (website/src/pages/chat/sideTurnWire.ts, new). A SendWire for the side endpoint family. It performs the two-call sequence the panel has always made (api.sideOpen then api.sideTurn) and re-expresses the helpers' outcomes in the fetch seam's shape the shared classifier reads: a 2xx body → resolved ok whose json() yields it (so {ok}dispatched, {ok, queued}queued, side-specific fields pass through untouched); a 2xx whose body could not be read (from /side/turn only) → resolved ok whose json() rejects, i.e. unknown — the server accepted the turn, so this must never hand the text back for a duplicate retry. The client's j() helper collapsed that case into the same TypeError a never-sent request throws (a stream cut mid-body is indistinguishable by type from "Failed to fetch"), so api.sideTurn now reads through a phase-preserving jAccepted that wraps a post-2xx body-read failure in AcceptedBodyUnreadable (identical non-2xx handling; its resolved type is unchanged, and the 10 existing test files that mock api.sideTurn are untouched). The class lives in api/apiError.ts (re-exported from client.ts) so the app-sdk and the many tests that mock client.ts wholesale share one identity for the instanceof; 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 surfacing err.message from a malformed 2xx must not render blank. j(), jNullable() and jAccepted() share one acceptOrThrow for 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's jsonFetch already 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 /side slash interceptor in ChatInput.tsx now treats AcceptedBodyUnreadable as accepted instead of restoring the composer — and at the same root cause in the app-sdk: jsonFetch tags a post-2xx res.text() cut or parse failure the same way, and appSendWire maps that (not a bare SyntaxError) to unknown, so a mid-body stream cut no longer reads as transport-error in ChatEmbed either — and a raw fetch rejection on that wire is now response-late rather than transport-error, because a single POST with no response cannot tell "never left" from "accepted, then the connection reset before headers", and the embed already recovers response-late (text back under the unconfirmed notice, retired by the poll's sendId echo); an ApiError from either call → resolved !ok carrying the error's friendly message (so refused keeps the reason the panel already displayed, and j()'s auth handling / error recording ran exactly as before); a deadline during /side/turnAbortErrorresponse-late; a deadline before /side/turn was 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/turn was dispatched (connection reset before headers) → AbortErrorresponse-late, because it cannot be told from a request the server took whose answer was lost, and /side/open just 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/open itself: offline, DNS) → transport-error. The abort race itself (settleUnderSignal) now lives once in chat-core/transport and both the app-sdk wire and this one use it. steer rides the wire, not the transport contract: only this endpoint family understands it.

Adapting the existing api.sideOpen/api.sideTurn calls 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. mutationFn becomes one sendTurn({ message, slot, wire: sideTurnWire(slot, { steer }) }) call. The receipt policy lives at the top of onSuccess, 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 (sideHandBackText accumulates into slotSide[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 by sideSendStatus, 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 (sideClose deletes slotSide[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 old onError did; and the panel's composer is one draft shared across slots (pre-existing; useComposerDraft is 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-error take the path the old onError took (roll back the optimistic bubble, hand the text back via mergeIntoDraft — never for a chip send, which did not consume the draft — and show the server's reason framed through pages.chatPage.send_failed_with_error, or a connection-framed line); response-late hands 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); unknown does nothing; dispatched / queued fall through to the acceptance handling, which is byte-for-byte the previous onSuccess body. onError remains only as the unexpected-throw fallback (sendTurn never rejects). Steer/queue split, optimistic bubble ownership, raw-text correlation for redacted queue cards, demotion notice — all unchanged.

Error strip — already ErrorNotice on base. An earlier revision of this PR moved the panel's hand-written <div className="text-danger"> strip onto the shared ErrorNotice; 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 raw sendMutation.error.message, and the per-slot sendStatus.error channel described above. askAgent stays 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's appSdk.chatEmbed.* copies from #8599. Per the #4240 precedent they live under pages.chatPage.* (siblings of send_failed_with_error). Since this branch was cut, #8689 (ChatPage transport) landed the first two of those shared keys on main and pointed ChatPane.tsx at send_failed_connection, so after the rebase this PR adds only delivery_unconfirmed_option to the shared block, points ChatEmbed.tsx and SideChat.tsx at the three shared keys, deletes the retired appSdk.chatEmbed.* set in every locale, and extends sendFailedSharedKey.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.tsx is no longer in this diff.

Not done here, deliberately. The plan sketched hoisting an isOwnUserRow proof-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 the SendWire seam and mintSendId; base re-targets to main when #8599 merges. Parallel to #8631 (P3 ChatEmbed composer), which does not touch SideChat.tsx. Remaining P2: ChatPage, then the four app-local senders inventoried in #8599.

Tests

  • SideChat.sendReceipt.test.tsx (new, 14): ApiError refusal → bubble rolled back, text handed back, framed reason rendered through ErrorNotice (role="alert"); TypeError from /side/open (never left) → connection copy and /side/turn is never called; a raw TypeError after /side/turn was dispatched → text handed back under the standing unconfirmed notice, no role="alert", no connection copy; a refusal from /side/open is a refusal (and /side/turn is 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/open is 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/turn whose 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 report failed.
  • appSdkScopedApi.test.tsx (+3) / appSendWire.contract.test.ts (+1): the scoped helper tags post-2xx text/parse failures; a never-left TypeError stays raw; the wire classifies a tagged stream cut as unknown.
  • ApiClient.coverage.test.tsx (+3): api.sideTurn — 2xx with a failing body read → AcceptedBodyUnreadable carrying the cause; never-left → raw TypeError; non-2xx → ApiError.
  • Compatibility evidence — unmodified: 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 reject api.sideTurn and assert the merged draft restore.
  • i18n: catalog parity, dead-keys, key-reference, English-identity, changed-value QA and all 11 locale style gates pass; added-lines gate 0.
  • 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/open stubbed OK and /side/turn refused with a 409; the script asserts the ErrorNotice text and the restored composer before capturing.

Screenshots / video

Visible delta against the current base (#8599 on main, which already renders the strip through ErrorNotice via #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's refused state as staged by the committed capture harness (website/capture/side-chat-send-receipt.*). The two standing-notice states (response-late with a restored draft / with a chip re-pick) reuse the panel's existing text-[12px] text-muted notice strip with no new markup; their copy and placement are pinned by SideChat.sendReceipt.test.tsx rather than by a screenshot.

BEFORE AFTER
before after
Light theme

before light
after light

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

  • At most two commits; this PR has one Conventional Commits commit
  • Existing relevant tests pass and new deterministic tests cover the behavior
  • Self-review completed; code follows project style guidelines
  • Documentation updated if applicable; the wire's contract is documented at sideTurnWire
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

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

@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 5, 2026 05:03
@CrysisDeu
CrysisDeu requested review from bolichen97 and removed request for a team September 5, 2026 05:03
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

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

  • appSendWire now maps every raw fetch rejection to response-late ("a raw fetch rejection on that wire is now response-late rather than transport-error"). Cause: one POST can't distinguish never-left from reset-after-accept → mechanism: plain offline — by far the common rejection — is classified as indeterminate → consequence: ChatEmbed users who are simply offline see "Delivery not confirmed — check the transcript" instead of a clear connection failure, and the poll that would retire the notice is also down, so the vague framing stands. The duplicate-turn tradeoff is deliberate and text is restored either way, but this degrades an already-shipped surface's common failure mode inside a SideChat PR — confirm it's intended product behavior, not just contract hygiene.
  • createScopedApi's reader now throws AcceptedBodyUnreadable for a malformed 2xx on all App Kit scoped calls, not just sends — an SDK-visible error-identity change (app code branching on instanceof SyntaxError/TypeError changes behavior). Placement is forced (only the fetch site knows the phase) and the message is non-blank, but it's a public-surface contract shift riding a transport PR.

Suggestions

  • In appSendWire, treat navigator.onLine === false as transport-error: a browser that knows it's offline proves "never left," recovering the clear failure copy for the common case at no duplicate-turn risk.

[DESIGN-REVIEWED] 9b1d664

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 9b1d664f0adaa49060699efe06cefb92002e38c9. Adjudication upheld 1 of 1 blocking finding(s); those 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
: (sendStatus?.error ?? localError)
Oversized draft -> setLocalError(question_too_long) -> displayError -> validation hint is rendered as a failed-operation alert.
Anchor: errors-use-error-notice
Fix: Render localError separately as neutral validation text; reserve ErrorNotice for send failures.
[BLOCK-MERGE] 9b1d664
[GPT-REVIEWED] 9b1d664

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

F1 anchors to the AUTOSDE rule errors-use-error-notice, which carries blocking: true at website/AUTOSDE.yaml:527. The rule text (:550-552) explicitly excludes client-side validation hints from ErrorNotice. Confirmed the path: setLocalError(question_too_long) at SideChat.tsx:646 (a validation hint on an oversized draft) → localError folded into displayError at :664 → rendered through <ErrorNotice> at :739. This is exactly the case the blocking rule forbids; the rule's flag is authoritative and outranks weighing.

[ADJUDICATION] 9b1d664 total=1 uphold=1 downgrade=0
UPHOLD F1 website/src/pages/chat/SideChat.tsx:664 reason=autosde-blocking-rule
[GPT-ADJUDICATED] 9b1d664

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 9b1d664f0adaa49060699efe06cefb92002e38c9: <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 Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

All evidence gathered. The PR embeds before/after images hosted at an earlier commit (e142a647...) but commits no images at this HEAD, so the blind read had nothing to read. The copy itself (shared delivery_unconfirmed*, send_failed_connection, framed send_failed_with_error) is established shipped copy re-keyed and applied to SideChat; no lens-13 hard swap exists (both notice strips reuse the panel's existing markup). Final review:

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

  • The standing "Delivery not confirmed — …check the transcript before sending it again" notice renders in the panel's muted 12px strip (text-[12px] text-muted, SideChat.tsx); its whole job is to stop a duplicate resend, and users who overlook muted chrome will resend. Rare (deadline timeouts) but the notice is the only guard, every time. Smallest fix: give the standing variant a warning-toned treatment distinct from the transient demotion notice.

Evidence gaps

  • Blind read not performed: no image is committed under temp-screenshots/ at HEAD 9b1d664; the PR's before/after links point at earlier commit e142a64. Re-run the committed capture harness (website/scripts/capture-side-chat-send-receipt.mjs) and commit the refused-state captures at this HEAD.
  • The two standing-notice states (pages.chatPage.delivery_unconfirmed with a restored draft; delivery_unconfirmed_option after a chip send) appear in no screenshot — the PR pins them by test only; a capture of each would close this.

[UX-REVIEWED] 9b1d664

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 9a934acf388c383a521dba3a2ce08490e174f89a — 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 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 ships

Intent: 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.

  1. Side-panel sends now time out at 10s instead of hanging forever — justified
  2. Refused side send shows the server's reason, framed, text handed back — justified
  3. Unconfirmed side send restores text under a standing notice — justified
  4. Accepted-but-unreadable reply now does nothing (no duplicate-inviting retry) — justified
  5. Handed-back text and its status survive slot switches and unmount, per-slot — justified
  6. Any side draft now survives a tab switch (parked on unmount) — declared, rides along
  7. ChatEmbed offline copy changed: "Delivery not confirmed" replaces "check your connection" — declared, rides along
  8. /side slash command no longer restores composer on unreadable 2xx — justified
  9. Oversize-question hint demoted from error alert to status text — mandated (AUTOSDE errors-use-error-notice excludes validation hints)
  10. Send-receipt strings promoted to shared pages.chatPage.* keys, embed copies deleted — justified (pinned Promote framed send_failed catalog entry out of the designTweak app namespace #4240 pattern)

(More than 10; the rest — AcceptedBodyUnreadable/jAccepted, settleUnderSignal dedup, capture page — are the mechanisms behind the above.)

Watch

  • One counted unfixed sibling of the reclassification's root cause. appSendWire and sideTurnWire now map a raw post-dispatch rejection to response-late because "a single POST with no response cannot tell 'never left' from 'accepted, then the connection reset before headers'" — but dashboardSendWire (sendTurn.ts:133), the same single POST, still falls to transport-error (sendTurn.ts:200), whose contract comment still claims "The send never left, so restore-and-report is safe" (sendTurn.ts:52-53). Grepped the three SendWire implementations: 2 fixed, 1 not. ChatPane keeps the duplicate-turn invitation the two fixed wires closed, and the contract doc now asserts something the PR argues is false. Genuinely larger blast radius (ChatPane/ChatPage policy), so accepted-and-deferred — but it should be a named follow-up, not silence.
  • Item 7 changes shipped ChatEmbed behavior in a SideChat-titled PR: the common, plainly-offline case now reads as indeterminate ("Delivery not confirmed", ChatEmbed.sendReceipt.test.tsx:1569-1585) to protect the rare reset-after-accept case. Declared and protocol-derived, but the common-case copy regression is a trade a human should consciously accept.

[FIRST-PRINCIPLES-REVIEWED] 9a934ac

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 9a934ac

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

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

@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-sidechat branch from c32b53f to e142a64 Compare September 5, 2026 05:25
@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 Sep 5, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-sidechat branch from e142a64 to a11d614 Compare September 5, 2026 05:26
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

GPT — BLOCKING: new send failures bypass ErrorNotice (c32b53f15 → fixed in a11d614ca)

Accepted, fixed. The panel's failure strip now renders through the shared ErrorNotice (inline variant, role="alert") with askAgent off and a {/* No hand-off: the failed question was just merged back into the composer draft below, and the hand-off navigates away, destroying that draft. */} comment, per errors-use-error-notice. Tests assert the alert role and framed text.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

GPT — BLOCKING: accepted turns become retryable when receipt parsing fails (c32b53f15 → fixed in a11d614ca)

Accepted, fixed. A SyntaxError raised by api.sideTurn (i.e. a 2xx whose body would not parse) is now resolved as { ok: true, json: () => reject } so sendTurn classifies it unknown — no error, no restore, bubble stays. Scoped to the turn phase only: the wire tags errors from /side/turn so the same SyntaxError from /side/open is not misread as an accepted turn. New test: unreadable 2xx → no alert, composer stays empty, optimistic bubble stays.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

GPT — BLOCKING: deadline during /side/open still sends the turn later (c32b53f15 → fixed in a11d614ca)

Accepted, fixed (Design raised the same gap). Between the two calls the wire now checks settled || signal.aborted and returns without dispatching /side/turn once the deadline has delivered a response-late receipt. New test: /side/open never resolves → after the deadline the text is back with the unconfirmed notice and api.sideTurn was never called.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Design — Suggestion: check signal.aborted before launching api.sideTurn (c32b53f15a11d614ca)

Implemented — same fix as GPT's third blocking finding above (settled || signal.aborted gate between /side/open and /side/turn, test-pinned).

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX — Suggestion: delivery_unconfirmed should not auto-dismiss on the 8 s TTL (c32b53f15a11d614ca)

Implemented. The unconfirmed notice is now a standing notice held until the next submit (a separate standingNotice state; the transient localNotice TTL is unchanged for the steer-demoted case). Test advances 20 s past the deadline and asserts the notice is still there.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX — Suggestion: frame the server reason instead of showing it bare (c32b53f15a11d614ca)

Implemented. refused now renders pages.chatPage.send_failed_with_error ("Send failed: side turn already in flight"), the same core framing key ChatEmbed and App.tsx use; the reason-less transport case keeps the connection-framed line. Screenshot after/refused-dark.png shows it.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX — Suggestion: chip-send response-late needs its own copy and must not merge into a live draft (c32b53f15a11d614ca)

Implemented. A chip send (override: true) that hits the deadline now shows pages.chat.sideChat.delivery_unconfirmed_option ("…re-pick the option if it is missing") and merges nothing into the draft; the same gate now also keeps a refused chip send from merging its label. New test: chip send times out → draft untouched, re-pick copy shown, "back in the composer" absent.

@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 Sep 5, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-sidechat branch from a11d614 to f98e761 Compare September 5, 2026 05:41
@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 Sep 5, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-sidechat branch from f98e761 to aef86f0 Compare September 5, 2026 05:43
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

GPT — BLOCKING: pre-turn timeout strands an idle send as pending (a11d614ca → fixed in aef86f075)

Accepted, fixed. The wire now tracks whether /side/turn was ever sent. A deadline that fires before it (a stalled /side/open) rejects with a plain PreTurnTimeouttransport-error → the panel rolls back the optimistic bubble, restores the text and shows the connection copy; a deadline during /side/turn keeps AbortErrorresponse-late. The sequence still stops after the deadline, so the turn is never dispatched late. Test rewritten: idle send, /side/open never resolves → after the deadline the bubble is gone, text is back, alert shows, no "unconfirmed" notice, api.sideTurn never called.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX — Suggestion: clear the standing notice when the restored draft is emptied (a11d614caaef86f075)

Implemented. An effect retires standingNotice as soon as the draft is empty, so the strip never asserts text that is no longer there. New test: steer times out → notice shown → user clears the composer → notice gone.

@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 Sep 5, 2026
CrysisDeu added a commit that referenced this pull request Sep 5, 2026
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.
@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 Sep 5, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-embed branch from 383e9aa to b4deb25 Compare September 5, 2026 08:16
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 5, 2026
CrysisDeu added a commit that referenced this pull request Sep 5, 2026
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.
@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-sidechat branch from 60c4aa6 to 1d4fdcc Compare September 5, 2026 08:40
@github-actions github-actions Bot removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision labels Sep 5, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/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.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 1bfdadaf3a4aed40e06b20e077e92e5ba5ec0a4a.

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.

This decision applies only to this commit. A new push requires a new judgment.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • GPT — F1 (adjudicated advisory): validation and failures share one error channel — span=97715118fe50 — rebutted (holds narrowly; remedy disproportionate — agreeing with the Opus adjudication)

The precedence at SideChat.tsx:647 is the base's shape, not this PR's: on d744c9729 displayError was already sendErr ? sendErr.message : localError, so a mutation-level failure already outranked a validation/queue-op localError. This PR inserts the per-slot store channel (sendStatus.error) between the two, because the framed refusal has to survive a slot switch / unmount and localError cannot (it is component state).
Both channels, plus the store one, are cleared together on every composer submit (onMutate, lines 282–284: setLocalError(null) / sideSendStatus({error: null, notice: null})), so the stale-outranks-fresh window closes on the user's next attempt; worst case is a visible, self-correcting wrong error string — no data loss, no security dimension.
A separate validation lane with reordered precedence is new state carried by every future reader of this composer, for a one-off recoverable UX degradation; that is the gold-plating the adjudication declined. Left as-is.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • First Principles — Watch: the "Error strip → ErrorNotice" section ships nothing at this revisionfixed (description, no code change)

Correct. The strip migration was in this PR's earlier revisions; main landed the same change in #8743 before the rebase onto #8599's new head (d744c9729), so the ErrorNotice line is now unchanged context. The body section is rewritten as "Error strip — already ErrorNotice on base", stating what this PR still changes about the strip (the framed reason and the per-slot sendStatus.error channel feeding it) and nothing more. The Catalogs paragraph was reconciled the same way: #8689 landed two of the three shared keys and the ChatPane.tsx change on main, so the body now says this diff adds only delivery_unconfirmed_option and no longer touches ChatPane.tsx.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • UX — Watch: the standing "Delivery not confirmed" notice renders in the muted 12px striprebutted (design choice, made deliberately in an earlier round)

The strip's weight was chosen in the round that made this notice standing (UX suggestion on c32b53f15a11d614ca): the red ErrorNotice strip means "your send failed, act"; this notice means the opposite — the send may well have landed, so do not resend without checking. Rendering it in error weight is the wrong signal and, on this panel, would invite the duplicate turn the copy exists to prevent. The notice is also not transient: it is store-held per slot, survives slot switches and unmount, and is retired only when the user empties the restored draft (pinned by SideChat.sendReceipt.test.tsx: "standing notice survives 20 s", "back on return to the slot"). What it gains over a transient notice is persistence, which is the property the copy needs; colour weight is not. Left as-is.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • UX — Evidence gaps: no screenshot of the two standing-notice states; BEFORE/AFTER documents a change already in the basefixed for the description, rebutted for the missing captures

The BEFORE/AFTER prose was stale after the rebase: #8743 moved the strip to ErrorNotice on main, so the BEFORE image now also shows a base difference. The Screenshots section is rewritten to say exactly that — read BEFORE for the copy (raw error message → framed Send failed: {{error}}), not the container — and the "What changed" section no longer claims the container migration. The images remain the committed refused capture from the harness this PR adds (website/capture/side-chat-send-receipt.*, SHA-pinned; both URLs verified 200).
On the two response-late notice states: they add no markup — both render through the panel's pre-existing text-[12px] text-muted notice strip (present on base at SideChat.tsx:620), only the text differs, and that text plus its placement/persistence is pinned by five cases in SideChat.sendReceipt.test.tsx. A screenshot of a pre-existing strip carrying a new sentence would document the string, which the catalog already does; staging a late receipt in the capture harness would also mean adding a fake-timer path to it for one image. Not added.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • UX — Suggestion: api.client.accepted_body_unreadable is thrown from every scoped App Kit call, including reads; scope the acceptance wording to send pathsrebutted (holds, disproportionate)

Confirmed: app-sdk/index.ts jsonFetch tags a post-2xx body-read or parse failure for every method (AcceptedBodyUnreadable, lines 442/450), so a GET can surface this sentence. Two reasons to keep one string:

  1. It is not wrong on a GET. "The server accepted the request" is literally what a 2xx means for any method, and "its reply could not be read" is the condition. What it replaces on the same path is the raw TypeError: network error / SyntaxError: Unexpected token d in JSON message the app would otherwise render from err.message (the blank-message concern raised in the Design lane on 9020b6023 and fixed in 35861c846) — the new line is strictly more readable than the one it displaced, on every method.
  2. The wording exists because jsonFetch has no way to know whether a call is a "send": it is a generic scoped fetch and the send-vs-read meaning lives in the caller (appSendWire). Making the sentence method-aware means either a second catalog key × 13 locales for the same technical condition, or a caller-supplied phrasing hook on the scoped API — both for a rare path (a 2xx whose body stream was cut) whose action for the user is the same regardless of method: retry the read. Left as one core-owned sentence; an app that wants friendlier read copy already catches AcceptedBodyUnreadable by identity.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • GPT — BLOCKING (security-class): a post-dispatch connection reset was classified as retry-safe transport-error — span=17b2e66d53de — fixed in 367a987a0

Accepted. At sideTurnWire.ts:96 a raw (non-ApiError, non-AcceptedBodyUnreadable) rejection after api.sideTurn had been dispatched fell through to throw err, which sendTurn reads as transport-error → the panel rolled back and restored the prompt with the connection copy, inviting a resend of a turn the server may already be running. /side/open succeeding milliseconds earlier on the same link makes "never left" the unlikely branch, so the honest label is indeterminate.
Fix: when turnStarted is set, the wire now rethrows a DOMException('AbortError'), so sendTurn yields response-late — the same path a deadline during /side/turn takes: an idle send's bubble stays pending, a steer/queued send hands the text back under the standing "Delivery not confirmed — check the transcript before sending it again" notice, and a chip send says to re-pick. The header contract gained the new bullet; transport-error is now reachable only from /side/open itself (offline, DNS).
Tests (SideChat.sendReceipt.test.tsx): the old "transport failure → connection copy" case was pinning the condemned behaviour and now rejects /side/open instead (asserting /side/turn is never called); a new case rejects /side/turn with a raw TypeError on a steer and asserts the text is back under the unconfirmed notice with no role="alert" and no connection copy.
Not widened: the /side slash interceptor in ChatInput.tsx (pre-existing, base behaviour) still treats a raw post-dispatch rejection as failed and restores the composer; it has no standing-notice channel to hand text back under, so the analogous change there is a UX decision for that surface, not a transport fix, and is out of this slice.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • GPT — BLOCKING (security-class): appSendWire.ts:83 classified a lost response as retry-safe transport-error — span=e392cec5d60e — fixed in 291103cb1

Accepted — the sibling of the sideTurnWire finding fixed one round earlier, and this PR's diff owns the handler (it rewrote it onto settleUnderSignal). The scoped helper already turns every response the server did send into AppApiError / AcceptedBodyUnreadable / AppApiPermissionError, so the only thing reaching the fall-through is fetch itself rejecting: "never left" (offline, DNS) or "the server took the POST and the connection reset before headers". One POST, no earlier call on the same link to weigh the odds, and sendId is echo correlation, not server-side dedup (a resend mints a new one) — so the adjudication's "no dedup path" holds.
Fix: the fall-through now rethrows DOMException('AbortError'), so sendTurn yields response-late. ChatEmbed already owns that outcome correctly: the text is handed back under the standing "Delivery not confirmed" notice (never the "check your connection" error row), and the poll's sendId echo retires the notice if the turn did run. transport-error is no longer produced by this wire; the header contract and ChatEmbed's policy comment say so.
Tests: appSendWire.contract.test.ts — the "rejected fetch → transport-error" case now pins response-late; ChatEmbed.sendReceipt.test.tsx — the "transport-level failure → connection error row" case now pins zero error rows, one unconfirmed notice, text restored. SideChat.sendReceipt.test.tsx keeps its /side/open never-left case, where the two-call sequence can still tell the branches apart.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • GPT — BLOCKING (errors-use-error-notice): a non-2xx whose body read fails escaped acceptOrThrow as a raw rejection and the wires read it as response-late — span=97715118fe50 — fixed in f47f1094c

Accepted. This is the one gap the two previous fixes opened: once a raw post-dispatch rejection is (correctly) indeterminate, every path that can leak a raw error after the server has already answered must be closed, and acceptOrThrow had one — await r.text() on a non-2xx ran before the ApiError wrap, so a cut error-body stream escaped as a TypeError, which sideTurnWire/appSendWire now classify response-late. A real refusal would then show as "Delivery not confirmed" (or leave an idle bubble pending) instead of reaching ErrorNotice.
Fix (in the helper, as the finding suggests): acceptOrThrow reads the error body with r.text().catch(() => r.statusText) — the status is the verdict, the body only its explanation — so a non-2xx is always an ApiError (status preserved, status line as the fallback reason) and the wires' refusal() branch always fires. This mirrors the guard the app-sdk's jsonFetch already had (res.text().catch(() => res.statusText)), so the two receipt clients agree.
Test: ApiClient.coverage.test.tsx gains "a non-2xx whose body read fails STILL rejects with ApiError" (409, text() throws → ApiError with status === 409).
Inventory of raw-rejection sources after this head, so the class is closed rather than chased: fetch() itself rejecting (no response — indeterminate by construction, handled as response-late in both wires); 2xx body unreadable (AcceptedBodyUnreadableunknown); non-2xx body unreadable (now ApiErrorrefused). checkSessionExpired / removeAuthBanner are synchronous DOM/state helpers with no I/O. No other await sits between "response received" and "classified".

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • GPT — BLOCKING (fenced, Opus: FLAG): a stale-idle send whose receipt is late/unreadable can briefly show its question twice (optimistic bubble + queue card) — span=97715118fe50 — needs-a-decision

Verdict flip on an identical diff. 95e7707a8 is a pure rebase of f47f1094c (the #8599 amend it absorbed added screenshots only; git diff between the two heads against their respective bases is byte-identical). GPT 5.6 passed f47f1094c with no blocking findings and blocks 95e7707a8 — the lane is non-deterministic run to run. This is also the third round a blocking finding has landed in this span (ec8984e20 → adjudicated advisory; 291103cb1 → fixed via acceptOrThrow; now this), so per the review loop's stall rule I am not pushing another patch here.
The finding, as Opus verified it: the optimistic bubble is appended only when the client guesses idle (optimistic: !isBusy); if that guess is stale (reload before the first frame restores streaming), the server queues instead, and if the HTTP receipt is simultaneously response-late/unknown while the WS channel still delivers the queue card, the bubble is not rolled back and the question shows as both running and queued. Opus's own adjudication: the duplicate is transient — the server's user echo reconciles the bubble by content when the queued entry drains (chatSlice.ts:4450-4457), the card is removed on drain, reload rebuilds from server history; no text lost, no corruption. Opus FLAGGED it (harm LOW, triple coincidence, "a human would plausibly accept the residual risk") — a fenced finding it cannot downgrade itself.
Why not fix in this PR: the remedy GPT names — reconcile the WS queue-card push against the in-flight optimistic bubble and roll it back — lives in the store's queue-card frame handling, not in the transport adapter this slice adds; the same stale-idle mis-guess pre-exists on base (the optimistic: !isBusy heuristic and its reload caveat are unchanged by this PR), and the queued HTTP branch already rolls the bubble back. Widening here would make this the fourth SideChat.tsx iteration on a cosmetic residual.
Decision requested from a maintainer: either (a) accept the residual as Opus's FLAG recommends — /ai-review override gpt 95e7707a8d593f51d614f3cf78154b7559153142: cosmetic self-correcting duplicate row requiring stale-idle + late/unreadable receipt + working WS; Opus FLAG, no data loss; queue-card/bubble reconciliation belongs to the store, not this transport slice — or (b) rule that the reconciliation belongs in this PR, in which case I will implement it as one more round.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author
  • GPT — BLOCKING (residual/crash-data-loss-corruption): SideChat.tsx:209 recovered text merges into an uncontrolled local draft and is lost when the panel unmountsfixed in 9b1d664f0

Accepted. Opus upheld it (UPHOLD-FENCED: ordinary flow, no rarity argument), and the trace holds: useComposerDraft is uncontrolled here, so the mounted-branch merge in recoverFor lands in component state, and an activity-tab switch unmounts SideChat (ActivityViewer.tsx, effectiveTab === 'side'). The standing notice, which lives in the store per slot, would then reappear over an empty box.
Root is shared with base — base's own cancel-release path (releasedTextmergeIntoDraftsideReleaseConsumed) drains into the same local draft and loses it the same way — but this PR is the diff that added the "your text is back in the composer" contract, so it owns closing the gap.
Fix: on unmount, park a non-empty draft into the store via the existing sideHandBackText channel (the same releasedText field the mount-time drain already reads), keyed to the slot the panel was showing. No new store field, no new reducer. An explicit close is still a discard: sideClose deletes the slot's side state and sideHandBackText no-ops on a missing slot.
Tests (SideChat.sendReceipt.test.tsx): a refused send whose text was merged into the mounted composer → unmount → releasedText holds it → remount drains it back with the failure line and clears the store copy; an empty composer parks nothing.

…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.
@CrysisDeu

CrysisDeu commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author
  • GPT — BLOCKING (errors-use-error-notice): SideChat.tsx:664 routes the over-limit validation hint (question_too_long) through ErrorNotice — span=97715118fe50 — fixed in 9a934acf3

Accepted. The rule's own text excludes client-side validation hints, and question_too_long is one: the send is never attempted, nothing failed. The routing pre-exists on base (localErrordisplayErrorErrorNotice), but this PR rewrote that exact displayError line, so it owns it.
Fix: a separate validationHint state carries only the over-limit message and renders as neutral helper text in the muted strip (role="status"), cleared on any draft edit. localError keeps its two real-failure producers (queue_cancel_failed, queue_edit_failed) and still renders through ErrorNotice, so the hand-off surface for actual failures is unchanged.
Test (SideChat.oversizeQuestion.test.tsx): the refusal renders with role="status", no role="alert" is present, and an edit retires the hint. The three existing text-content pins are unchanged.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 9a934ac

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

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.

3 participants