📐 refactor: Normalize Stream Events Behind a ChatTransport Type - #16335
Conversation
Add the ChatTransport interface and a ChatEvent union discriminated by
type, covering every frame the client handles today: the created, sync
and final control frames, the step and side-channel { event, data }
frames, legacy content and text frames, attachments, errors, open and
abort. Types only; nothing consumes them yet.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0980b3b404
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| terminalStatus?: 'complete' | 'error' | 'aborted'; | ||
| generationCreatedAt?: number; | ||
| generationProtocolVersion?: number; | ||
| requestMessage?: TMessage | null; |
There was a problem hiding this comment.
When an Assistants run completes, both chatV1.js and chatV2.js emit a requestMessage containing only parentMessageId and thread_id, but TMessage requires fields including messageId, conversationId, text, and isCreatedByUser. Consequently, a transport cannot represent these valid terminal frames as ChatFinalFrame without an assertion, and consumers are falsely told those required fields exist; type this field as Partial<TMessage> | null (as the sync frame already does).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30e25a6. Assistants final frames send only parentMessageId and thread_id (chatV1.js:719, chatV2.js:557); requestMessage/responseMessage are now Partial | null.
| export type ChatErrorData = { | ||
| text?: string; | ||
| message?: string; |
There was a problem hiding this comment.
When packages/api/src/utils/events.ts::handleError is called with a string, the error event parses to a primitive string, while the resumable agent route also emits objects shaped like { error, generationProtocolVersion }; neither value is assignable to this object type. A normalizing transport therefore cannot emit a correctly typed ChatEvent for common server errors without discarding data or casting, so ChatErrorData needs to cover the primitive and resumable error forms in addition to the existing failed-turn object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30e25a6. ChatErrorData now admits the bare string from handleError and { error, generationProtocolVersion } from the agents stream route.
| | ChatSyncFrame | ||
| | ChatFinalFrame | ||
| | ChatEventFrame | ||
| | TContentData |
There was a problem hiding this comment.
userMessageId
When Assistants streams a non-text content part, both AssistantService.addContentData and StreamRunManager.addContentData emit index, type, the content part, messageId, thread_id, and conversationId, but neither emits userMessageId. Since TContentData requires that field, including it directly here excludes these existing wire frames from ChatFrame; use a wire-specific content type with userMessageId optional (or update every producer before exposing this contract).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30e25a6. Added ChatContentFrame with optional userMessageId, since AssistantService.addContentData omits it; ChatFrame and the content event use it.
| responseMessage?: TMessage | null; | ||
| conversation?: Partial<TConversation> | null; | ||
| runMessages?: TMessage[]; | ||
| title?: string; |
There was a problem hiding this comment.
When abortMiddleware.js aborts a conversation object whose title is still empty, it deliberately emits title: null, but this contract permits only a string or omission. That valid abort frame therefore cannot be represented as ChatFinalFrame, and a consumer checking only for undefined can incorrectly treat the runtime value as a string; declare the wire field as title?: string | null.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 30e25a6. title is now string | null, matching abortMiddleware.js:119.
Assistants final frames carry a partial requestMessage, aborts can send
title: null, Assistants content frames omit userMessageId, and the error
event body can be a bare string or { error, generationProtocolVersion }.
Widen the frame types to admit each.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30e25a6c54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** `{ event, data }` for the agent graph's step events, dispatched to the step handler. */ | ||
| export type ChatStepFrame = | ||
| | { event: StepEvents.ON_RUN_STEP; data: Agents.RunStep } | ||
| | { event: StepEvents.ON_AGENT_UPDATE; data: Agents.AgentUpdate } |
There was a problem hiding this comment.
When hide_sequential_outputs is enabled for an intermediate non-tool step, api/server/controllers/agents/callbacks.js emits on_agent_update with data: { runId, message }, while Agents.AgentUpdate requires the unrelated { type, agent_update } content-part shape. This valid wire frame therefore cannot be represented as ChatStepFrame without a cast or lossy conversion; add the legacy payload shape to this event variant or change the producer before publishing this contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26c063b. on_agent_update now admits ChatAgentStatusUpdate ({ runId, message }) from callbacks.js:550.
| | { | ||
| text?: string; | ||
| message?: string; | ||
| error?: string | { message?: string }; |
There was a problem hiding this comment.
For an error raised after SSE headers have been sent, api/server/middleware/error.js::sendError constructs an error body with error: true and passes that object directly to handleError, but this union permits only a string or object in the error property. Fresh evidence in the current head is the unconditional handleError(res, errorMessage) path for that boolean-marked object, so the earlier expansion still leaves a common server error frame unrepresentable as ChatErrorData without a cast.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26c063b. ChatErrorData.error now admits true, as sendError sets once headers are sent (error.js:44).
| export type ChatEventFrame = | ||
| | ChatTitleFrame | ||
| | ChatStepFrame | ||
| | { event: 'attachment'; data: TAttachment } |
There was a problem hiding this comment.
When an agent publishes a run artifact, packages/api/src/agents/files/binding.ts calls emitAttachment(file) with a plain TFile; those published files do not carry the messageId and toolCallId required by TAttachmentMetadata. Both the named attachment SSE path and its resumable { event: 'attachment' } equivalent can therefore carry a valid payload excluded by this variant, so the wire attachment type needs to include plain TFile publications rather than requiring TAttachment universally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26c063b. Attachments are ChatAttachment = TAttachment | TFile, covering plain run-artifact publications.
| | { event: StepEvents.ON_MESSAGE_DELTA; data: Agents.MessageDeltaEvent } | ||
| | { event: StepEvents.ON_REASONING_DELTA; data: Agents.ReasoningDeltaEvent } | ||
| | { event: StepEvents.ON_RUN_STEP_DELTA; data: Agents.RunStepDeltaEvent } | ||
| | { event: StepEvents.ON_RUN_STEP_COMPLETED; data: { result: Agents.ToolEndEvent } } |
There was a problem hiding this comment.
When on_run_step_completed arrives without data.result, api/server/controllers/agents/callbacks.js still forwards the frame if it belongs to the final agent or sequential outputs are visible. Requiring result here excludes those explicitly emitted frames and forces a transport to assert a value that consumers cannot safely dereference; model result as optional or nullable, or stop emitting the result-less variant.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26c063b. on_run_step_completed data is { result?: Agents.ToolEndEvent | null }.
on_agent_update also carries a { runId, message } status line when
sequential outputs are hidden, on_run_step_completed can arrive without
a result, a published run artifact is a plain TFile, and sendError marks
its body with error: true. The frame types now admit each.
danny-avila
added
the
🗺️ Shared Types
2 participants
Summary
The stream hooks tell server frames apart by probing JSON keys (
final,created,event,sync,type,message) plus the namedattachmentanderrorevents, and nothing names that protocol. This first link of a stack, following #16288, #16289 and #16290, adds types only tolibrechat-data-provider.It adds the wire frames as the server writes them, a
ChatEventunion discriminated bytypelikeTMessageContentParts, and aChatTransportinterface. JSDoc notes the AI SDK equivalents (UIMessageChunk,sendMessages,reconnectToStream) without adopting their names. Nothing consumes them yet.Type of change
Testing
Tested environments/configuration:
Automated tests:
cd packages/data-provider && npx tsc --noEmit: cleancd packages/data-provider && npx jest: 51 suites, 2142 passed, 1 skippednpx eslint,npx prettier --check,node scripts/sort-imports.mts --checkon both touched files: cleanScreenshots / recordings
No user-facing change
Risk / compatibility
None. New exported types only.
Checklist