Skip to content

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

Open
CrysisDeu wants to merge 1 commit into
mainfrom
feat/chat-core-p2-embed
Open

feat(chat-core): route ChatEmbed's send through the transport wire#8599
CrysisDeu wants to merge 1 commit into
mainfrom
feat/chat-core-p2-embed

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Depends on #8853 (gateway: preserve the client sendId through the slot queue drain) — merged 2026-09-06 (16ec93bef); this branch is rebased on top of it. The delivery proof below matches a polled user row by identity only; #8853 is what makes a queued send's drained row carry that identity.

Problem / Motivation

ChatEmbed (the app-sdk's embeddable chat) was the third of four hand-written send paths. It posted to the bare /api/chat, which answers with an SSE stream; the scoped AppApi JSON helper buffered that stream and threw SyntaxError, and the embed's .catch called that success. A refused POST (409 slot-agent mismatch, 403 from an app that never granted /api/chat) rejected the mutation, but nothing read sendMutation.isError. Either way the composer had already been cleared, so a failed send looked sent and the text was gone.

Why it matters

The chat-core extraction RFC (P0 contract inventory) named this the worst of the four paths: it is the one that can lose a user's turn with no trace. It is also the one surface that cannot import the dashboard client, because an app's sends must stay a permission the host app grants (allowedApiPaths).

What changed (motivation → approach → change)

Transport (website/src/chat-core/transport/sendTurn.ts). sendTurn was hard-wired to the dashboard client's api.sendChat. It 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 signal fired. dashboardSendWire is the default, so ChatPane (#5909) is unchanged. Receipt classification (readSendReceipt, the abort deadline, the six statuses) stays in the one place.

App-sdk wire (website/src/app-sdk/appSendWire.ts, new). Re-expresses the scoped AppApi.post outcomes in the seam's shape: 2xx JSON → resolved ok; 2xx non-JSON (SyntaxError) → resolved ok whose json() rejects, i.e. unknown — the exact shape the old path swallowed; non-2xx → resolved !ok carrying the body text so the server's reason survives; a scoped-api permission denial → resolved !ok whose body carries a human sentence ("This app isn't allowed to send chat messages.", new key appSdk.chatEmbed.app_not_allowed_to_send, 12 locales + en-XA) as the refused reason — "check your connection" would be advice that can never succeed, and the raw app/path/grant detail is developer vocabulary, so it goes to the console instead — via a typed AppApiPermissionError thrown by the permission check; offline / fetch rejected → transport-error; deadline → AbortErrorresponse-late (the scoped helper takes no signal, so the deadline is honoured at the receipt). The POST targets /api/chat?ws=1; the scoped path check reads only the pathname, so no new permission is required from apps.

To read status and body back without parsing the API <status>: <text> message string, createScopedApi now throws AppApiError (website/src/app-sdk/apiError.ts). It is deliberately not re-exported from the barrel — the vendor stub mirrors the barrel value-for-value, so that would be an SDK surface change. Apps keep seeing the same Error with the same message.

ChatEmbed (website/src/app-sdk/ChatEmbed.tsx). Sends through sendTurn with the app-sdk wire (the agent the embed was mounted with rides on the wire, not the transport contract). Receipt policy: refused / transport-error → an error row at the transcript tail (the existing registry renderer, no new component) — the server's reason framed through pages.chatPage.send_failed_with_error ("Send failed: …", the same core key App.tsx's feedback send uses, so a raw "slot agent mismatch" does not read as the agent erroring mid-work); a reason-less transport failure states its cause ("Couldn't send — check your connection and try again.", new core key appSdk.chatEmbed.send_failed_connection) instead of a bare "Send failed" — and the text handed back via mergeRecoveredDraft as typed (the composer's draft, whitespace and all — the wire carries the trimmed text, the recovery gives back what the user wrote; a paragraph-break join, rendered as such because the composer is now ChatInput's textarea — see the second commit below); response-late → the text handed back under a notice row ("Delivery not confirmed — …", retired only when the poll shows this message's own user row — identified by identity only: the client-minted meta.sendId the embed stamps on every send (mintSendId, now owned once by chat-core/transport — ChatPage's and ChatPane's identical inline minters are replaced by the shared one in this PR), read back off the polled slot detail either as the row's meta.sendId or as a member of a merged row's meta.sendIds. The server keeps that id on every path a send can take — a dispatched send persists it on its row directly, and a send queued behind a busy slot carries it on the queue entry so the drain writes it onto the row (#8853, which this PR depends on) — so there is no text-matching fallback: an id-less row with the same words is some other writer's (an older backend, an injection, a resend) and is never proof (isOwnUserRow) — appended past the transcript length captured when the send started, at which point the restored text is also taken back out of the composer — unless the user has edited it since. Unrelated growth (a prior turn still streaming, a cron/sub-agent injection, a same-text row with another id, an older backend that drops meta) is not proof and leaves both notice and text alone, so a late turn that did land never sits above a standing "not confirmed" with a resendable copy below it, and an undelivered one is never silently withdrawn; new i18n keys appSdk.chatEmbed.delivery_unconfirmed / delivery_unconfirmed_option in all 12 locales + en-XA) — this embed keeps no optimistic bubble, so once the composer cleared the user's text had no visible copy left, which is the one indeterminate outcome the transport contract names as recoverable (ChatPane leaves it alone only because its bubble is still on screen); unknown → nothing (a 2xx was received, restoring could duplicate a running turn); dispatched / queued → the poll renders it. An option chip's direct send never restores into the composer (it did not consume the draft — the same gate ChatPane/ChatPage keep). A host-supplied onSend rejection proves nothing about delivery (a host may have posted and lost the answer), so it is treated as unconfirmed — notice + text handed back — never as a failure that invites a duplicate-turn retry. On a follow-up-chip send the unconfirmed notice uses its own copy (delivery_unconfirmed_option: "…re-pick the option if it is missing") because the draft-clobber gate means nothing was restored. The send-tail row is folded into the top-anchored embed's new-message scroll hash, so a failure/unconfirmed row is announced rather than appended below the fold, and mirrored into a polite role="status" live region so a screen-reader user hears the outcome instead of only noticing the button re-enable. Polling, approvals, follow-up chips and the composer itself are untouched.

useChatSession seed (website/src/app-sdk/useChatSession.ts). The RFC's "four hand-written send paths" undercounted: the app-sdk session hook seeded a new slot through the same bare /api/chat POST with the same swallowed SyntaxError. It now sends the seed through sendTurn over the app-sdk wire, fire-and-forget (the slot is the deliverable; creation must not wait out the transport deadline for an answer it ignores). The receipt is deliberately not acted on — the slot exists either way, and a seed that did not go out is an empty session, not a lost user message — but the SSE-as-JSON root cause is gone from the app-sdk entirely.

RFC placement. Chat-core extraction P2 (transport), second slice. After #5909 (ChatPane), #8689 (ChatPage's send and steer, merged 2026-09-05) and this PR, 3 of 4 user-facing send paths plus the app-sdk seed send are on sendTurn. Corrected inventory — the RFC's "four paths" undercounted; the remaining hand-written POST /api/chat sites off sendTurn are: SideChat (the declared P2 remainder, next PR), plus the app-local and background senders outside app-sdk/ that are NOT converted here and need their own slot in the series: apps/mochi/panel/panelBridge.ts (echoes the user bubble before the POST and never reads the receipt — the same looks-sent class), apps/design-tweak/api.ts (resolves a non-JSON 2xx as {ok: true}), hooks/useSceneInteraction.tsx (re-implements readSendReceipt classification without the deadline), apps/design-critique/api.ts (the identical .catch(SyntaxError) swallow on the SSE endpoint this PR removes from ChatEmbed), App.tsx (the feedback send), and the two non-interactive api.sendChat callers apps/issue-radar/agentSession.ts and apps/auto-improvement/agentSession.ts (no composer to restore into; they need the receipt classification, not the recovery policy). SideChat is #8655 (stacked on this branch; a second receipt adapter for the /side/* reject-semantics endpoint). Rebased onto main after #8689: SendWirePayload now also carries steer / colorTheme so the default dashboard wire forwards ChatPage's flags unchanged; the app-sdk wire ignores them (the server refuses app-authenticated steers). ChatPane's reason-less transport failure still reads bare "Send failed" (ChatPane.tsx:427); it takes the shared cause-stating key in #8655, where that key is promoted to pages.chatPage.*.

Second commit — the composer swap (#8631, merged into this branch 2026-09-05). Reviewed and converged as its own PR (10 rounds, all five lanes) before being merged here so the two land together. ChatEmbed's bare <input> is replaced by the real ChatInput in a SlotProvider with a fail-closed embedded preset — every dashboard capability defaults off, and a source-shape test forbids any = true default except connected — a sending prop (spinner, "Sending…", aria-busy; the button is not disabled while sending so the host's draft-clear does not strand focus), sendOnEnter forwarded via the new hooks/useChatConfig.ts, and ChatEmbed.noDashboardClient.test.tsx, a recording-Proxy invariant that the embedded composer makes no dashboard-client call. This is what lets recovered text keep its paragraph breaks (above) and deletes the bespoke input+button. RFC §4.1 is ratified host-only for ChatEmbed — it mounts under the dashboard store; the store-free ChatInput seam that a standalone npm publish would need is P3 model-layer work, tracked in #8651 and recorded in the RFC so the decision is made, not deferred to publish time. Full history in #8631. Since the merge: an Enter pressed while a send is in flight is acknowledged — the spinner button pulses and "Sending…" is announced once to a live region — instead of silently early-returning. #6825 (optimistic bubble across a slot refetch) is the explicit downstream rebase point: its markSendFailed policy re-bases onto transport callers after this lands; its semantics are not imported here. #6823 / #6307 / #5819 / #4904 are independent.

Tests

  • chatCoreTransport.contract.test.ts (+3): an injected wire receives {message, slot, meta} plus an AbortSignal and the dashboard fetch is not touched; its resolved non-2xx is refused and its rejection transport-error; the deadline fires through it as response-late. 14 pass.
  • appSendWire.contract.test.ts (new, 9): agent + JSON-receipt path on the wire; queued ≠ dispatched; AppApiErrorrefused with server reason (and with a non-JSON HTML 500 body); SyntaxError on 2xx → unknown (the old swallowed shape); permission denial → refused with the human sentence + console detail; rejected fetch → transport-error; deadline → response-late without a signal; a settlement after the deadline does not flip the receipt.
  • ChatEmbed.sendReceipt.test.tsx (new, 24): refused → error row with server reason + text handed back; transport failure → generic copy + handed back; merge with text typed mid-flight; unreadable 2xx → no row, no restore; deadline → notice row + text handed back; dispatched → nothing; next send clears the row; option-chip send does not clobber the draft; onSend rejection → notice + handed back, no error row; timed-out chip send says "re-pick the option", not "text is back"; a failure row in a top-anchored embed triggers the new-message scroll; proven delivery (own user row by meta.sendId) retires the notice and takes back an untouched restore, leaves an edited draft alone, a send the poll showed before the deadline leaves nothing behind, unrelated transcript growth keeps both notice and text, a same-text row with another sendId is not proof, a same-text row with no sendId is not proof either (no text fallback), a merged queued row naming this send in meta.sendIds is proof, a permission denial shows the human sentence, a transport failure names the connection, a refused send hands back the draft as typed (leading/trailing whitespace kept) while the wire carried the trimmed text, an Enter mid-flight pulses the spinner and announces "Sending…" without a second request, and an error row stays whatever arrives.
  • ChatEmbed.test.tsx: send-path expectations updated to /api/chat?ws=1; default receipt {ok: true}. 35 pass.
  • useChatSession.test.ts: seed expectations updated to the JSON-receipt path; a refused seed still yields a ready session.
  • i18n catalog suite (parity, dead keys, key reference, English identity, changed-value QA) passes with the new key.
  • npm run typecheck, changed-file ESLint, and the i18n added-lines gate (0 untranslated) are clean.

Manual verification

Screenshot harnesses: website/capture/chat-embed-send-receipt.* (first commit: real ChatEmbed under the real AppApiProvider, fetch stubbed to refuse the send; asserts the error row and the restored composer before capturing — and now waits for the row's animate-scale-in entrance to finish before the frame is taken, since a first-frame capture showed an empty gap where the row was) and website/capture/chat-embed-composer.* (second commit: the same surface with the mounted ChatInput, light and dark).

Screenshots / video

Frames are of the branch head's surface — the mounted ChatInput — so they show both commits, and the PNGs are committed in this head's tree under temp-screenshots/chat-embed-composer/ (before/after, light/dark). BEFORE is main (bare <input>, and a refused send left it looking exactly like idle: no row, composer empty, text gone).

BEFORE (main): idle AFTER: idle
before idle after idle
AFTER: 409 refused — framed reason row, text back in the composer AFTER: deadline fired (response-late) — notice row
refused late
Permission denial (transport-error receipt), and light theme

denied dark
idle light
refused light

First-commit-only frames (bare input, before the composer swap) — kept for the receipt-row history

refused old
late old

Related Issues

Chat-core extraction RFC, phase P2 (transport). Builds on #5909 and #8689. Depends on #8853 (gateway-side sendId through the queue drain; opened from this PR's GPT finding). Downstream rebase point: #6825. 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 contract is documented at SendWire
  • 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 September 5, 2026 00:07
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 5, 2026 00:07
@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) — ✅ PASS

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

Design-Verdict: PASS

Wire-as-seam keeps receipt classification single-sourced while the scoped AppApi stays the only path an embed can POST through, so the permission boundary survives the unification.

[DESIGN-REVIEWED] f915040

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — 🟡 CONCERNS

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

I have everything I need: blind read reconciled, diff and strings read, screenshots verified (including the shot-08 anomaly), capture harness understood, and no recordings committed. Final review:

UX-Verdict: CONCERNS

Recovery flow reads well cold, but the after/refused-light frame is missing its error row, and the red failure copy leaves the reader with nothing to act on.

Watch

  • temp-screenshots/chat-embed-composer/after/refused-light.png shows NO "Send failed" row where its dark sibling and both before-frames show one — the capture script asserts the row exists, so this is a capture-ordering slip or a real light-theme render miss; if real, a refused send in light theme is silent again. Re-capture and verify. (Low frequency, high impact, every light-theme refusal.)
  • Denied app: "Session ready. Type a message to start." plus a live send button on a screen that can never send — reader: "Those two messages contradict each other and I can't tell which one to believe… I'd just be stuck." The grant is known client-side before the first keystroke; surface the denial up front instead of post-send. (Misconfigured apps only, but a persistent dead-end.)
  • "Send failed: slot agent mismatch" passes raw server vocabulary through — reader: "I don't know what a 'slot agent' is… would not know whether pressing send again is safe or pointless." The 409 body carries code: 'slot_agent'; map known codes to human copy like the permission denial got, and have the error row say the text was handed back (the grey notice says so, the red rows don't — the reader read that asymmetry as two unexplained kinds of failure).

Evidence gaps

  • Refused + light theme, after state: the committed screenshot lacks the error row (above) — a re-captured after/refused-light.png closes it.
  • appSdk.chatEmbed.send_failed_connection ("Couldn't send — check your connection…") appears in no screenshot.
  • delivery_unconfirmed_option (the follow-up-chip "re-pick the option" notice) appears in no screenshot.
  • The composer's in-flight "Sending…" spinner state appears in no screenshot.
  • Notice retirement (proven delivery removes the notice and silently withdraws the untouched restore from the composer) is a state flip static shots can't show; the recordings list is empty — a short recording of send → late notice → poll-proof retirement closes it.

Suggestions

  • delivery_unconfirmed: swap "composer"/"transcript" for on-screen words — reader: "nothing on screen is labelled 'composer'" — e.g. "your text is back in the message box below; check the conversation above before sending it again."

[UX-REVIEWED] 150f90a

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ⚠️ review incomplete

GPT 5.6 did not produce a complete verdict for 150f90abeae967ac23b9b07cc153aa9ab60bcf0a; inspect the workflow logs and re-run it.

This comment is updated in place on each push.

See the GPT 5.6 Review job logs; this commit has no completed GPT verdict.

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

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of f9150405ce085764e3c0040e48814fb872d1c493 — 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 counts are done. Composing the review.

First-Principles-Verdict: CONCERNS

The fix lands at cause level and deletes two duplicate spellings; what rides along is ChatEmbed's new hard requirement on the host Redux store.

What this change ships

Intent: stop an embed's failed chat send from looking sent while the user's text is silently lost — a FIX (routed through the shared transport rather than patched in place).

  1. A refused embed send shows "Send failed: " and returns your text — justified (the reported defect)
  2. A timed-out send shows "Delivery not confirmed", restores the text, retires only on identity-proven delivery — justified
  3. The session hook's slot-seed send rides the same transport — justified (same defect, last sibling: 0 bare /api/chat posts remain, grep post\(['"]/api/chat['"])
  4. The embed composer becomes the real ChatInput (textarea, send-key setting honoured) — declared rider; replaces a second composer spelling
  5. ChatEmbed now only mounts under the dashboard Redux store — undeclared cost of item 4
  6. ChatInput gains a fail-closed embedded flag — justified (app allowedApiPaths boundary); 1 consumer
  7. Send button spins with "Sending…" instead of disabling the field — declared; 1 consumer
  8. Scoped-api errors become typed (AppApiError/AppApiPermissionError), apps see the same message — justified mechanism
  9. Chat-settings liveness collapsed into useChatConfig; sidebar and tool-name readers migrated too — rides along (subtractive, 5 consumers)
  10. Embed strings added/removed across 13 locales; permission denial gets a human sentence — justified

Watch

  • The diff's own note concedes the cost of item 4: ChatEmbed "is not a store-free component -- a standalone publish either ships it as host-only or gives the composer a store-free seam first" (app-sdk/index.ts). The old header said "No Redux dependency." A permanent SDK-surface narrowing traded for composer dedup — a human should ratify that trade, not discover it.
  • useSimplifiedToolNames and ChatSidebar's tagColumnsEnabled migrations are beyond the fix's needs and change behavior (both now also reload on window focus) — subtractive riders, but riders in a fix.
  • The mid-flight "nudge" (scale pulse + keyed role="status" re-announcement, ChatInput.tsx) is the one element of item 7 whose absence costs only a keypress that looks idle while the spinner already shows; 1 consumer.

Subtractions

  • Shrink item 7 to spinner + aria-busy + click guard: drop the sendingNudge state, its 300ms timeout effect, and the keyed sr-only span — the guard already refuses the double-fire, and the spinner already announces the in-flight state (1 consumer: ChatEmbed.tsx:503).

[FIRST-PRINCIPLES-REVIEWED] f915040

@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

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 150f90a

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

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

@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-embed branch from 45b35ec to d40a771 Compare September 5, 2026 00:23
@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-embed branch from d40a771 to 4aead39 Compare September 5, 2026 00:24
@CrysisDeu

CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

GPT 5.6 — disposition for 45b35ecc7 → fixed in 4aead39c4

  • BLOCKING · ChatEmbed.tsx:216 · timed-out send can lose the only copyAccepted, fixed. The finding is right for this surface: ChatPane may leave response-late alone because its optimistic bubble is still on screen, but ChatEmbed keeps no bubble, so once the composer cleared the user's text had no visible copy — exactly the indeterminate outcome sendTurn's contract names as recoverable by such a caller. Now: response-late hands the text back via mergeRecoveredDraft and appends a notice row (not an error — the turn may well be running) with new copy appSdk.chatEmbed.delivery_unconfirmed ("Delivery not confirmed — … check the transcript before sending it again."), added to all 12 locales + en-XA. unknown (a 2xx was received) still does nothing, since restoring there invites a duplicate. Pinned by the rewritten deadline test in ChatEmbed.sendReceipt.test.tsx; screenshot late-dark.png added to the body.

@CrysisDeu

CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

First Principles — disposition for 45b35ecc74aead39c4

  • Subtraction · drop export from dashboardSendWireApplied. Module-private now; sendTurn is its only consumer.
  • Subtraction · drop export from APP_SEND_PATHApplied. Module-private; the contract test asserts the literal /api/chat?ws=1 instead of importing the constant.

@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-embed branch from 4aead39 to 33a10cb Compare September 5, 2026 00:35
@CrysisDeu

CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

UX — disposition for 4aead39c4 → fixed in 33a10cbdd

  • Watch · delivery_unconfirmed misstates composer state on option sendsImplemented. The unconfirmed branch now picks copy by send kind: a composer submit keeps "Your text is back in the composer; check the transcript before sending it again"; a follow-up-chip send gets new appSdk.chatEmbed.delivery_unconfirmed_option — "Delivery not confirmed — the server has not acknowledged this message yet. Check the transcript, then re-pick the option if it is missing." (12 locales + en-XA). Pinned by a new test: a timed-out chip send shows the re-pick copy, never the "back in the composer" sentence, and leaves the user's own draft untouched.

@CrysisDeu

CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

GPT 5.6 — disposition for 4aead39c433a10cbdd

  • BLOCKING · appSendWire.ts:84 / onSend catch · rejection does not prove non-deliveryHalf accepted, fixed for the half that holds.
    • onSend (host endpoint): accepted. A host may have posted and lost the answer; its rejection is opaque. It now takes the unconfirmed path (notice + text handed back, no error row) — never a failure that invites a duplicate. Test updated to pin it.
    • Wire reject(err)transport-error: disputed, override posted. This branch is reached only when the scoped AppApi rejected with something other than AppApiError (an HTTP status) or SyntaxError (a 2xx body) — i.e. the permission check threw before any request left, or fetch itself rejected. That is exactly the dashboard wire's contract merged in feat(chat-core): extract transport sendTurn and adopt it in ChatPane #5909 (sendTurn.ts catch → transport-error, "the send never left"); a received-then-dropped response is a fetch that resolves, which the adapter routes to unknown/refused, not here. Diverging the app wire from the dashboard wire on the same event would re-fork the receipt contract this PR exists to unify.

@CrysisDeu

CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

First Principles — disposition for 4aead39c433a10cbdd

  • Subtraction · drop AppSendWireOptionsApplied. appApiSendWire(api, agent?: string); the interface is gone.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt 33a10cb: the wire's reject→transport-error branch fires only when the scoped api threw before any request left (permission check) or fetch itself rejected — a received-but-dropped response resolves and is classified unknown/refused; this mirrors the dashboard wire contract merged in #5909, and the onSend half of the finding was accepted and fixed in this SHA.

@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 33a10cbddd6a43895ca04cfd8ed7de52c217190c.

the wire's reject→transport-error branch fires only when the scoped api threw before any request left (permission check) or fetch itself rejected — a received-but-dropped response resolves and is classified unknown/refused; this mirrors the dashboard wire contract merged in #5909, and the onSend half of the finding was accepted and fixed in this SHA.

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

@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-embed branch from 33a10cb to 21ba33a Compare September 5, 2026 00:46
@CrysisDeu

CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

UX — disposition for 33a10cbdd → fixed in 21ba33afa

  • Watch · top-anchored embeds can append the failure row below the fold unannouncedImplemented. The send-tail row is now folded into the non-startAtBottom new-message scroll hash (msgHash includes sendTail.role + content.length), so a refused or unconfirmed send triggers the same scrollIntoView a new reply does; startAtBottom embeds were already re-pinned by the ResizeObserver. Pinned by a new test: with scrollIntoView spied, a refused send in a top-anchored embed adds a scroll call.

@CrysisDeu

CrysisDeu commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator Author

First Principles — disposition for 33a10cbdd21ba33afa

  • Watch · useChatSession.ts:113 is a fifth bare /api/chat sibling with the same SSE-swallowAccepted, converted in this PR. The seed now goes through sendTurn over the app-sdk wire (JSON receipt). Receipt deliberately not acted on — the slot exists either way and a missing seed is an empty session, not lost user text — but the swallowed-SyntaxError root cause is gone from the app-sdk. Body's path count corrected ("plus the app-sdk seed send"); useChatSession.test.ts updated.
  • Subtraction · drop AppApiError.statusApplied. Only bodyText is read back; the constructor keeps the status parameter for the message text.

@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-embed branch 2 times, most recently from 85d0b9f to e0be365 Compare September 5, 2026 00:58
@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-embed branch from b4deb25 to b5c6442 Compare September 5, 2026 13:31
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-embed branch from b5c6442 to 5819abf Compare September 5, 2026 16:15
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 5, 2026
@CrysisDeu
CrysisDeu force-pushed the feat/chat-core-p2-embed branch from 5819abf to e3ac2af Compare September 5, 2026 17:27
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026
chenmingwei23 pushed a commit that referenced this pull request Sep 5, 2026
#8689)

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-embed branch from e3ac2af to d744c97 Compare September 6, 2026 01:04
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

GPT 5.6 — disposition for d744c972905006aef1aad062ed14279eb93339fc

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Design — disposition for fe0c36bc1b351281e532aa8f1a6a66f0f552c62a

  • Watch · ChatEmbed's mount contract tightened to store-required; no check that an out-of-tree consumer existsAcknowledged; no out-of-tree mount exists today, and the trade is recorded. Every app-sdk consumer renders inside the dashboard's React tree: the vendor stub (packages/app-sdk) resolves to the host's module at runtime, so an app mounts ChatEmbed under the dashboard's Provider by construction — there is no standalone bundle that could render it outside the store. The standalone-publish case is the RFC §4.1 decision (ratified host-only, 2026-09-05) with the store-free ChatInput seam tracked in chat-core P3: store-free seam for ChatInput so ChatEmbed no longer requires the host store #8651; that issue is the deprecation path if a standalone publish is ever scheduled.
  • Watch · the host-onSend branch mints a sendId no host endpoint can stamp, so a delivered-but-rejected host send keeps a standing noticeAcknowledged; fail-safe by design, and the retirement needs a contract change the host path does not have yet. onSend(text) carries no id, so a host cannot echo one; the PR body names this outcome ("a host may have posted and lost the answer … treated as unconfirmed, never as a failure that invites a duplicate-turn retry"). Giving the host the id (onSend(text, { sendId })) is an SDK signature change and belongs with the host-path seam work in chat-core P3: store-free seam for ChatInput so ChatEmbed no longer requires the host store #8651, not in this slice.
  • Suggestion · hoist SendTail / isOwnUserRow into chat-core before the remaining senders re-derive itAccepted; scheduled for feat(chat-core): route SideChat's send through the transport over a side wire #8655, the first second consumer. Same rule this series used for mintSendId: the helper moves into chat-core/transport when the second consumer appears (SideChat), with ChatEmbed switched to it in the same change, so the count never reaches two hand-written copies.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

UX — disposition for fe0c36bc1b351281e532aa8f1a6a66f0f552c62a

  • Watch · mid-flight acknowledgment is keyboard-only (a mouse click on the spinner button does nothing visible)Accepted as a real asymmetry; deferred to a follow-up rather than re-pushed here. The fix is the one you name — route the click through fireComposer unconditionally so both input paths pulse and re-announce — and it lives in ChatInput so any host that sets sending gets it. This head is all-green after the fix(gateway): preserve the client sendId through the slot queue drain #8853 rebase and feat(chat-core): route SideChat's send through the transport over a side wire #8655 is stacked on it; a re-push costs both PRs a full round for a one-line change with no data-loss consequence (the host still early-returns, so no second request either way). Tracked for the next ChatInput touch.
  • Watch · send_failed_with_error interpolates the server's reason verbatim into an app embedAcknowledged; same framing every core surface uses. The row is "Send failed: ", the key App.tsx's feedback send and ChatPane already use, and the reason is the backend's own human-readable refusal string (chat_handlers.py writes sentences such as "this crew is still running the previous message; send again when it finishes"), not a code. The one machine-shaped case — a permission denial from the scoped API — is replaced by the human sentence appSdk.chatEmbed.app_not_allowed_to_send with the raw detail sent to the console (previous round, pinned by test).
  • Evidence gap · no image under temp-screenshots/ at this HEAD; body frames pinned to 843dff7dAcknowledged; the pinned frames are of this surface. Nothing visible changed between 843dff7d and this head: the rounds since altered only the delivery-proof logic (identity-only isOwnUserRow, no text fallback) and the rebase onto feat(chat-core): route ChatPage's send and steer through the transport #8689/fix(gateway): preserve the client sendId through the slot queue drain #8853, none of which renders. The frames show the mounted ChatInput idle (light/dark), the refused row with the restored draft, the delivery_unconfirmed notice, and the permission-denial row — the states listed as uncovered. The chat-embed-composer / chat-embed-send-receipt capture harnesses are in the tree for a re-capture on request.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

First Principles — disposition for fe0c36bc1b351281e532aa8f1a6a66f0f552c62a

  • Watch · the composer swap is a declared rider that couples the "no Redux" embed to the store; a human should confirm the tradeConfirmed and recorded (unchanged from the prior head). RFC §4.1 is ratified host-only by the series conductor (2026-09-05), the store-free ChatInput seam is tracked as P3 work in chat-core P3: store-free seam for ChatInput so ChatEmbed no longer requires the host store #8651, and the PR body's "Second commit — the composer swap" section states the decision in those words. feat(chat-core): mount the real ChatInput composer in ChatEmbed #8631 converged the swap on its own over ten rounds across all five lanes before it was merged here.
  • Watch · apps/design-critique/api.ts:74 keeps the identical swallowed-SyntaxError POST; 5 hand-written senders remainAcknowledged; inventoried in the body as the series' next slots. No change here — the RFC scope for this slice is the app-sdk paths.
  • Watch · item 8 (tagColumnsEnabled / simplifiedToolNames now also reload on window focus) is a behaviour change the description does not surfaceDeclared here; it is a net-deletion side effect of the shared useChatConfig hook. The three duplicated focus/mc-config-changed listener blocks collapsed into one hook, and the two settings that previously re-read only on mc-config-changed now also re-read on focus. Kept, as the prior round agreed. Not folded into the body on this head because a body edit re-triggers the edited-listening lanes for a full round; it rides into the description with the next push if there is one.
  • Subtraction · defer the composer swap to its own changeDeclined, with the reason on record. The swap already was its own change (feat(chat-core): mount the real ChatInput composer in ChatEmbed #8631) and was merged into this branch deliberately so the two land together: it is what lets a recovered draft keep its paragraph breaks (the recovery path's "as typed" contract in this PR) and it deleted useComposerDraft's duplicate IME/Enter API. Splitting it back out now would re-open a converged review and leave this PR's recovery contract depending on an input it no longer uses.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

@bolichen97 — taking option (b), now complete at head 150f90abeae967ac23b9b07cc153aa9ab60bcf0a:

  • Body rewritten to describe the combined change: a "Second commit — the composer swap (feat(chat-core): mount the real ChatInput composer in ChatEmbed #8631)" section states what ships (the real ChatInput in a fail-closed embedded preset, SlotProvider required, RFC §4.1 ratified host-only with the store-free seam tracked in chat-core P3: store-free seam for ChatInput so ChatEmbed no longer requires the host store #8651), the stale "split out as the next PR" sentence is gone, the "flattened to a space" claim is replaced by what the textarea actually does (paragraph breaks preserved, test asserts 'second\n\nfirst'), and the permission denial is described as the refused receipt it yields. Checklist says one commit, and the branch is one commit.
  • Fresh screenshots committed in the head tree under temp-screenshots/chat-embed-composer/ (16 PNGs: before = main's bare <input>, after = the mounted ChatInput; idle / refused / late / denied, light and dark). The body's frames are pinned to this head's SHA.
  • Since your review: the delivery proof is identity-only (meta.sendId / merged meta.sendIds), which required the gateway to keep the id through the queue drain — landed as fix(gateway): preserve the client sendId through the slot queue drain #8853 (merged, 16ec93bef) and this branch is rebased on it; GPT and Opus lanes pass on this head.

Requesting re-review.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/ai-review override gpt 150f90a: The send-tail is a role:'error' row inside the chat transcript rendered by the registry's ErrorCard, which errors-use-error-notice itself lists under 'Does NOT flag' (the agent is already in that conversation, so a hand-off would be circular); the code is byte-identical to head fe0c36b, which this lane passed with no findings — only PNGs were added.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

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

The send-tail is a role:'error' row inside the chat transcript rendered by the registry's ErrorCard, which errors-use-error-notice itself lists under 'Does NOT flag' (the agent is already in that conversation, so a hand-off would be circular); the code is byte-identical to head fe0c36b, which this lane passed with no findings — only PNGs were added.

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

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

Copy link
Copy Markdown
Collaborator Author

UX — disposition for 150f90abeae967ac23b9b07cc153aa9ab60bcf0a → fixed in f9150405ce085764e3c0040e48814fb872d1c493

  • Watch · after/refused-light.png shows no "Send failed" rowAccepted; reproduced, root-caused, re-captured. Ran the harness locally: the row was in the DOM (the script's own assertion passed) with computed opacity: 0 — it enters through animate-scale-in, and the frame was taken on the animation's first frame (the dark sibling caught it half-faded, which is why it reads so muted). A real render miss is ruled out. scripts/capture-chat-embed-send-receipt.mjs now awaits document.getAnimations() before every frame; all eight after/ frames are re-captured at this head and the light refused frame shows the row at full contrast. Body URLs re-pinned.
  • Watch · denied app shows "Session ready" + a live send button on a screen that can never sendAcknowledged; deferred. The grant is knowable client-side, and surfacing it up front (disable the composer with the human sentence) is the right shape — but it is a new pre-send state on a misconfigured-host path, out of this slice's contract (receipt policy). Recorded for the app-sdk follow-up alongside chat-core P3: store-free seam for ChatInput so ChatEmbed no longer requires the host store #8651.
  • Watch · "Send failed: slot agent mismatch" passes raw server vocabulary through; error rows do not say the text was handed backAcknowledged; deferred. Mapping known backend codes to human copy (as the permission denial already got) is a shared-copy change that belongs in the core pages.chatPage.send_failed_* family so ChatPane/ChatPage change with it — the SideChat slice (feat(chat-core): route SideChat's send through the transport over a side wire #8655) is promoting exactly that key family and is the place for it. The "your text is back" asymmetry between the grey notice and the red row is noted for the same change.
  • Evidence gaps (connection-failure copy, delivery_unconfirmed_option, "Sending…" spinner, retirement recording)Acknowledged. The four committed states are the ones the human reviewer asked for (feat(chat-core): mount the real ChatInput composer in ChatEmbed #8631's option (b)); the remaining states are test-pinned (ChatEmbed.sendReceipt.test.tsx), and the harness accepts ?refuse= scenes so any of them can be added on request. Not extended here to keep this head to the requested evidence.
  • Suggestion · delivery_unconfirmed copy uses "composer"/"transcript"Accepted in principle; deferred to the copy pass above ("your text is back in the message box below; check the conversation above before sending it again" reads better and is the same key family).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

readiness: checking Automated validation is still running

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants