feat(chat-core): extract transport sendTurn and adopt it in ChatPane

[CrysisDeu]

Why no screenshot: This changes only transport receipt handling and deterministic tests; no rendered component, layout, style, motion, or interaction affordance changes.

Problem / Motivation

ChatPane had two send sites that independently interpreted the same HTTP receipt. The composer and the question-card fallback could drift on refused, unreadable, queued, aborted, and transport-error outcomes; the fallback also had no bounded response contract.

Why it matters

Receipt drift can silently lose a user's turn or recover an already-delivered turn and invite a duplicate. A shared transport seam makes delivery classification consistent while leaving each surface responsible for its own recovery policy.

What changed (motivation → approach → change)

A new chat-core sendTurn transport wraps the existing readSendReceipt parser and normalizes dispatched, queued, refused, unknown, response-late, and transport-error outcomes. It owns the 10-second abort deadline and never rejects.

Both ChatPane send paths now consume that contract. The normal composer preserves current-main optimistic confirmation, including the server-provided message ID, follow-up options, pending questions, and queue behavior. It leaves unknown and response-late sends pending rather than restoring potentially delivered content. The question-card fallback deliberately restores on response-late because the card has already removed the only visible copy; a deterministic policy test now pins that caller-specific choice.

The server now rejects an empty wire message before every dispatch branch, so the obsolete client-side queued-but-dropped workaround is replaced by a refusal-path assertion.

The rebase preserves current-main behavior and does not import #6825's separate markSendFailed policy. #6825 is the explicit follow-up that must rebase its failure behavior onto this transport caller after this PR lands. #6823 follow-up options, #6307 ACP/rate-limit behavior, #5819 cross-session send, and #4904 fork-merge UI remain independently owned.

Tests

• Eleven relevant transport, ChatPane, PendingQuestionCard, delivery, and ChatPage files: 155 passed in one run.
• Focused post-review transport and ChatPane policy run: 33 passed.
• npm typecheck passed.
• ESLint passed for all four changed files.
• Production build passed before the final comment/test-only review amendment.
• Fresh merge-tree against current main passed; git diff validation is clean.
• No retries, sleeps, real-time waits, increased timeouts, relaxed assertions, or warning filters were added. Deadline tests use fake timers; the new caller-policy test injects AbortError directly.

Manual verification

N/A — pure transport and receipt-policy behavior with no rendered, layout, or styling delta; deterministic automated coverage exercises the fetch boundary and caller policy.

Related Issues

Chat-core extraction phase P2. Follow-up semantic rebase point: #6825.

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; transport behavior is documented at the contract
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

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

Body-read deadline follow-up

The GPT review found one valid residual path: a 2xx response can deliver its headers and then stall while response.json() reads the body. The abort is intentionally classified as unreadable by the shared parser, but the transport must preserve that the deadline fired. sendTurn now returns response-late when the parser reports unknown after its controller has aborted, so card/composer callers can recover their only visible input. A deterministic fake-timer test holds the body read open until abort and pins this contract.

Additional validation:

• chatCoreTransport.contract.test.ts: 11 passed
• transport + ChatPane focused suite: 34 passed
• changed-file ESLint: 0 errors
• npm run typecheck

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3033462

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

The design checks out: the removed client-side workaround is backed by a server guard already at the base commit, and the divergent response-late policies (composer holds, card restores) are deliberate, documented, and pinned by tests. The remaining question is scope: sendTurn calls itself "the one send implementation," but ChatPage.tsx, App.tsx, useSceneInteraction, and two app agentSessions still hand-roll the same receipt.

Design-Verdict: PASS

A real drift bug, fixed at its root by one shared receipt contract, with each caller's recovery policy kept explicit and test-pinned.

Watch

• sendTurn documents itself as "the one send implementation," but ChatPage.send, App.tsx, useSceneInteraction, and both app agentSessions still hand-roll readSendReceipt — the receipt-drift class this PR eliminates in ChatPane persists at the largest send site until those adopt the transport. fix(chat): keep the optimistic user bubble across a slot refetch #6825 covers ChatPane failure policy only; make sure the remaining migrations are tracked, or the "one implementation" claim rots into a third interpretation surface.

[DESIGN-REVIEWED] 3033462

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 3033462f8171e3881bea23b33b02c726cad9620b — 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 checks done. The server-side empty-message guard is base behavior (chat_handlers.py:504, untouched by the diff), so the client workaround removal deletes dead code. sendTurn has 2 real consumers; the hand-rolled receipt-interpretation pattern it exists to end has 5 remaining non-test sites. Final review:

First-Principles-Verdict: CONCERNS

Cause-level consolidation that earns its place — but the nearest sibling, ChatPage.send, hand-rolls the identical contract and is absent from the named follow-ups.

What this change ships

Intent: make ChatPane's two send paths read one HTTP receipt identically so a turn is neither silently lost nor duplicated — an ADDITION (consolidation) with declared behavior changes.

1. One shared transport classifies every send outcome for both ChatPane paths — justified, cause-level
2. Question-card fallback send now aborts after 10s instead of hanging forever — justified, declared
3. A late receipt on the card fallback now restores the answer with an error row — justified, test-pinned
4. File-only send failure now comes from the server refusal; client workaround deleted — justified deletion (guard is base behavior, chat_handlers.py:504)
5. Composer send outcomes unchanged (verified branch-by-branch against the removed code) — justified
6. New top-level chat-core/ tree holding one file — inherited placement, plan-only justification
7. Receipt-contract test suite at the real fetch seam — justified

Watch

• Two live spellings of the receipt contract now coexist: ChatPage.tsx:4830-4886 hand-rolls the same abort deadline, readSendReceipt, confirmedDelivered, and mid extraction that sendTurn owns. Grep readSendReceipt outside tests: 5 unfixed sites (ChatPage.tsx:4832, App.tsx:2718, useSceneInteraction.tsx:356, apps/auto-improvement/lib/agentSession.ts:251, apps/issue-radar/lib/agentSession.ts:294). The description names five follow-ups but not ChatPage — the site most able to drift.

Subtractions

• Shrink the new namespace: move sendTurn.ts into utils/ beside its only dependency sendDelivery.ts — chat-core/transport/ holds 1 file, and its justification ("extraction phase P2") lives only in PR text, not in this repository.

[FIRST-PRINCIPLES-REVIEWED] 3033462

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 3033462

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

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

[github-actions]

UX Review (Fable 5) — 🟡 CONCERNS

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

All evidence gathered. The diff is a transport refactor with one genuinely new user-facing path: the question-card fallback now recovers on the 10-second deadline, but routes it through the generic connection-failure copy.

UX-Verdict: CONCERNS

The new response-late recovery on the question card asserts "Couldn't send — check your connection and try again" when the code knows delivery is indeterminate.

Watch

• onFallbackSend maps response-late to fail(receipt.reason) → reason is undefined → the error row renders pages.chatPage.send_failed: "Couldn't send — check your connection and try again." The transport's own contract says this status means "delivery is indeterminate" (the send may well have landed), so the copy misnames the cause and its "try again" actively steers the user into the duplicate answer the PR's comment calls the worst case. Rare (deadline fire on a card fallback) × moderate impact (misdirected retry, duplicated answer to the agent) × every occurrence. Smallest fix: a distinct catalog string for this status — "No confirmation arrived — your answer is back in the composer; it may still have been delivered."

Suggestions

• The composer path's refused now surfaces the raw server string (receipt.reason = "message is required") for a file-only send; a user who attached a file did provide content — a string naming the actual gap ("Add a message to send with your file") would keep the label honest.

[UX-REVIEWED] 3033462

[CrysisDeu]

First-principles CONCERNS — addressed in 08b4933.

• abortMs, colorTheme, steer: confirmed zero consumers (the "test seam" claim for abortMs was indeed wrong — the contract tests drive the deadline via fake timers + the shared SEND_ABORT_MS constant). All three options are removed; SendTurnOptions is now exactly the surface ChatPane passes (message, slot, meta). They can be re-added with their first real consumer (ChatEmbed/ChatPage adoption).
• Remaining hand-rolled send sites (incl. ChatPage): accepted-and-deferred per the seam-extraction sequencing the design review endorsed; ChatPage is on the adoption list alongside ChatEmbed/SideChat in the RFC's P2 slices.

[CrysisDeu]

/ai-review override gpt 08b4933: Deliberate duplicate-over-loss tradeoff, not a defect — on response-late the question card is already cleared, so NOT restoring silently loses the user's answer (the strictly worse outcome); the divergence is documented at the call site and in the PR body, and this PR's own Design Review explicitly endorsed it.

[github-actions]

Human judgment recorded

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

Deliberate duplicate-over-loss tradeoff, not a defect — on response-late the question card is already cleared, so NOT restoring silently loses the user's answer (the strictly worse outcome); the divergence is documented at the call site and in the PR body, and this PR's own Design Review explicitly endorsed it.

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

[CrysisDeu]

First-principles CONCERNS — dropped status is dead code — fixed in a1f1e55.

• dropped removed entirely (status, guard, its contract test, and the PR-body claims). Verified the premise directly: chat_handlers.py refuses an empty wire text ABOVE every dispatch branch (its comment even names the old below-the-busy-branch placement as the cause of the false queued: true receipt), so no current backend can produce the shape the guard watched for. The receipt enum is now six outcomes, all reachable.

(Split 2026-09-04 from a combined record so each disposition carries exactly one finding; the UX-lane items now live in their own comments.)

[CrysisDeu]

First-principles CONCERNS (round 3) — remaining api.sendChat call sites — accepted-and-deferred.

• Remaining api.sendChat call sites (ChatPage / App / scene / app agentSessions): accepted-and-deferred. This PR is the RFC's P2 slice — extract sendTurn and adopt it in ChatPane, the narrowest mirror. The other call sites are additional adoption targets beyond the RFC's named ChatEmbed/SideChat slices and will each be migrated in their own PR so the receipt-contract change stays reviewable per surface.

[CrysisDeu]

First-principles CONCERNS (round 3) — undeclared removal of the client-side drop guard — fixed in c16411b.

• Undeclared removal: the PR body's "What changed" now explicitly declares the client-side drop-guard removal and why (server refuses empty wire text at cause level).

[CrysisDeu]

UX CONCERNS — copy for the dropped status — fixed in a1f1e55 (moot).

• UX copy for dropped: moot — the dropped status no longer exists. It was removed entirely in a1f1e55 (status, guard, contract test) because chat_handlers.py refuses empty wire text above every dispatch branch, so no surface can ever render that copy.

[CrysisDeu]

UX CONCERNS — response-late copy blames the connection — accepted-and-deferred.

• UX copy for response-late (distinct "no confirmation received" string instead of connection-blaming copy): accepted as a follow-up, not folded in here. Doing it right means a reason key on the receipt mapped to new catalog strings across the 11 dashboard locales — a user-visible copy change that belongs in its own reviewable PR, not inside a no-visual-delta transport refactor. The suggested shape (transport populates a per-status reason key, surfaces map to catalog) is the design we'll take into the ChatEmbed/SideChat adoption slice.

[CrysisDeu]

First-principles CONCERNS (round 3) — duplicate spelling of the delivery rule — fixed in c16411b.

• Duplicate spelling of the delivery rule: sendTurn now calls the shared confirmedDelivered(body) instead of re-deriving ok && !queued inline.

[CrysisDeu]

First-principles CONCERNS (round 3) — SEND_ABORT_MS export — rebutted.

• SEND_ABORT_MS export: retained — the contract tests drive the deadline through it, and inlining the constant would leave the deadline unpinnable at the seam. Removing the export would not shrink the surface; it would only make the 10s deadline untestable.

[bolichen97]

Approving — the extraction is behavior-preserving where it claims to be, and the one deliberate behavior change is scoped, explained, and pinned by a test.

Status-by-status equivalence check against the deleted ChatPane code:

• refused -> reportFailedSend(body.error): same, and receipt.reason carries the same body.error narrowing.
• unknown -> silent: same, and correctly still silent, since a 2xx may already have started a turn.
• transport-error -> reportFailedSend() with no reason: matches the old non-abort .catch leg, including the "no body to quote" rationale.
• response-late -> silent in the composer: matches the old AbortError early return.
• dispatched == confirmedDelivered(body) == ok && !queued (verified in sendDelivery.ts), so both the confirmOptimisticSend gate and the retireStatelessQuestion gate keep their exact previous predicate — the old site spelled the latter body.ok && !body.queued, which is the same test.
• resolveAskAfterSend(receipt.body, ...) still receives the raw acceptance body, so card/ask logic is untouched.

The one deletion that is not a pure move — the body.queued && !llm.trim() attachment-only workaround — is genuinely obsolete rather than dropped: chat_handlers.py now carries the hoisted if not message: return 400 message_required guard ABOVE every dispatch branch (steer/queue, crew, subagent-hold, new turn), and that guard is already on main, not introduced here. A 400 is non-2xx, so readSendReceipt classifies it refused and the same reportFailedSend recovery fires — now with the server's reason instead of a bare failure. ChatPane.dirSend.test.tsx adds a case pinning exactly that refusal path, so the removed client workaround is replaced by an assertion rather than by nothing.

The intentional divergence is correct and correctly localized: the question-card fallback restores on response-late while the composer does not. The card has already destroyed the only visible copy of the answer, so a silently lost answer has no other trace, and the worst case is a visible duplicate the user can delete; the composer keeps its optimistic row pending to avoid inviting a duplicate turn. sendTurn distinguishes this from a merely malformed 2xx by checking controller.signal.aborted when the parser says unknown, which is the right way to preserve the deadline signal through a parser that folds an abort-during-response.json() into unknown.

sendTurn never rejects (every leg returns a receipt, clearTimeout in finally), so the old if (!res) guard in the fallback is subsumed: a throw from readSendReceipt lands in the catch as transport-error, which fails and restores exactly as before.

Truly no visual delta: the only ChatPane edits are the two send callbacks and their comments — no JSX element, class, prop, icon, ordering, or affordance changes anywhere in the diff.

Test coverage matches the seam: 11 transport contract tests cover all six statuses plus the pre-deadline non-fire, the unreadable-non-2xx and unreadable-2xx split, and the wire shape (/api/chat?ws=1, slot, meta); ChatPane tests pin the server-provided mid, the card late-restore policy, and the empty-wire refusal.

All checks pass (57 pass, 7 skipped, 0 failing).

Non-blocking, pre-existing on main and not this PR's to fix: the confirmedDelivered docstring in sendDelivery.ts still describes the busy branch as queueing only non-empty text while answering {ok, queued} either way — the hoisted server guard has made that paragraph stale. Worth a follow-up sweep now that the client workaround it justified is gone.