Conversation
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. |
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
![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: 9d56547e41
ℹ️ 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".
| const sources = [...(results.organic ?? []), ...(results.topStories ?? [])]; | ||
| return sources.map((source, i) => ({ | ||
| type: 'source-url', | ||
| sourceId: `${attachment.toolCallId}-${i}`, |
There was a problem hiding this comment.
When one message contains search attachments that reuse a provider toolCallId, this resets i for each attachment and produces duplicate sourceId values such as call_0-0 for different URLs. This is a supported case: client/src/hooks/SSE/useAttachmentHandler.ts:13-29 deliberately distinguishes snapshots by agent, turn, and host step, and the integration test at ContentParts.integration.test.tsx:366-398 covers repeated call IDs. Include those ownership fields or a message-wide ordinal so distinct sources retain distinct UI identities.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. Source ids carry a message-wide ordinal across every search attachment, and URLs are deduplicated message-wide. Covered by 'gives distinct sources distinct ids when tool call ids repeat'.
| case ContentTypes.IMAGE_FILE: { | ||
| const { source: _source, ...rest } = metadata; | ||
| return { type: ContentTypes.IMAGE_FILE, ...rest }; |
There was a problem hiding this comment.
When a mapped image_file part is edited through its UI representation, this branch ignores part entirely and restores the original image_file object from metadata. Changes to url, filename, or mediaType therefore disappear on fromUIPart/fromUIMessage, while the adjacent image-URL, video, and audio branches do consume the edited UI fields. Retain the stored image metadata but overwrite its filepath, filename, and type from the UI part.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. The image_file branch now applies the view's url, filename and mediaType over the stored record when they differ, so edits land and untouched parts still round-trip. Covered by 'writes image file edits back while keeping the stored record'.
| default: | ||
| return { | ||
| name: toolCall.name, | ||
| id: toolCall.id ?? toolCall.stepId, |
There was a problem hiding this comment.
When a tool step is announced before the provider assigns an ID, applyToolCallsStep deliberately stores id: '' together with the stable host stepId, but nullish coalescing selects the empty string here instead of that step ID. toToolPart then emits an empty toolCallId because its later fallback has the same problem, so multiple in-flight calls can share the same UI identity and deltas or outputs can be associated with the wrong call. Treat an empty ID as missing and fall back to stepId or the synthetic index ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. An empty tool call id is treated as missing: agents calls fall back to stepId, then to the synthetic tool_call-. Covered by 'falls back from an empty tool call id to the host step id' and the persisted tool scenario, which asserts non-empty, distinct toolCallIds.
| const text = message.parts.reduce( | ||
| (joined, part) => (part.type === 'text' ? joined + part.text : joined), | ||
| '', | ||
| ); |
There was a problem hiding this comment.
fromUIMessage traverses message.parts here to collect text, traverses it again through fromUIParts, and, without a base message, performs separate filter and map passes for files. Thus every base-less conversion makes four O(n) passes over the same potentially large message-parts array even though text, content, and attachment files can be accumulated together; consolidate this work into the reconstruction loop. CLAUDE.mdL165-L168
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. fromUIMessage collects text, content and attachment files in one pass over the parts.
| const { runStepStatus, progress } = toolCall; | ||
| const failed = runStepStatus === 'failed' || runStepStatus === 'cancelled'; | ||
| const hasOutput = output != null && output !== ''; | ||
|
|
||
| let state: UIToolState = complete ? 'input-available' : 'input-streaming'; | ||
| if (failed) { | ||
| state = 'output-error'; | ||
| } else if (hasOutput || runStepStatus === 'completed' || (progress ?? 0) >= 1) { |
There was a problem hiding this comment.
Persisted tool calls can signal terminal failure without a failed runStepStatus: background cancellation is stored as backgroundTask.cancelled, and rejected arguments are stored as inputValidationError. The existing renderer explicitly consumes both markers, but this calculation ignores them and can classify a cancelled or validation-failed call with output/progress as output-available. Include these durable markers when deriving the UI state so restored cancellation and failure are not shown as success.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. getToolFailure reads backgroundTask.cancelled and inputValidationError alongside failed/cancelled run steps, so those calls map to output-error. Covered by 'reports durable failure markers as output-error'.
| let state: UIToolState = complete ? 'input-available' : 'input-streaming'; | ||
| if (failed) { | ||
| state = 'output-error'; | ||
| } else if (hasOutput || runStepStatus === 'completed' || (progress ?? 0) >= 1) { | ||
| state = 'output-available'; |
There was a problem hiding this comment.
When a tool call is paused for human approval, its stored approval object has complete JSON arguments and no output, so this initializes the part as ordinary input-available. The exported part has no approval field or approval lifecycle state, leaving consumers unable to distinguish a blocked call from one ready to execute even though Part.tsx:433-447 renders mandatory approval controls for this exact condition. Surface the approval payload and a distinct pending-approval state in the UI union.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. A stored approval without submitted output maps to the AI SDK approval-requested state with approval { id, requestReason }, and maps back to the stored call. Covered by 'reports a call paused for review as approval-requested'.
| if (!results) { | ||
| return []; | ||
| } | ||
| const sources = [...(results.organic ?? []), ...(results.topStories ?? [])]; |
There was a problem hiding this comment.
A persisted web-search result may place cited links in references rather than organic or topStories; the existing sources renderer handles non-image/non-file reference links as ordinary sources in client/src/components/Web/Sources.tsx:597-636. This selection silently omits those URLs, so a message whose citations are reference-backed produces no corresponding source-url parts. Include link references when building the source list, with appropriate deduplication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. Link-type references are included as sources, deduplicated by URL across the message; image and file references stay out. Covered by 'gives distinct sources distinct ids when tool call ids repeat'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a815932762
ℹ️ 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".
| conversationId: metadata?.conversationId ?? base?.conversationId ?? null, | ||
| parentMessageId: metadata?.parentMessageId ?? base?.parentMessageId ?? null, |
There was a problem hiding this comment.
When a UI edit promotes a message to a root by setting metadata.parentMessageId to null, nullish coalescing restores the old non-null ID from base instead. Since null is a valid TMessage.parentMessageId, distinguish an absent metadata value from an explicit null so reparenting does not silently leave the message under the wrong node. The same issue affects clearing conversationId.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. Identity fields now read metadata when the key is defined, so an explicit null parentMessageId or conversationId wins over the stored value (pickIdentity). Covered by 'keeps an explicit null parent instead of the stored one'.
| const output = typeof part.output === 'string' ? part.output : part.errorText; | ||
| return { | ||
| type: ContentTypes.TOOL_CALL, | ||
| tool_call: { | ||
| type: ToolCallTypes.TOOL_CALL, |
There was a problem hiding this comment.
When fromUIPart receives a hand-built output-error tool part without provider metadata, this stores errorText only as a normal output and records no failure status. Mapping that content back through toUIPart therefore reports output-available, turning a restored failure into a success; preserve a durable failed marker when constructing this tool call.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. A hand-built output-error tool part now stores runStepStatus: 'failed' on the rebuilt call, so it maps back as output-error with its errorText. Covered by 'persists the failure of a hand-built error tool part'.
| if (part == null) { | ||
| continue; | ||
| } | ||
| pushUnique(agentIds, part.agentId); |
There was a problem hiding this comment.
For persisted tool parts whose ownership exists only at tool_call.agentId, this reads only the outer part and omits that agent from metadata.agentIds. That nested fallback is an established storage shape—the ownership logic in packages/data-schemas/src/methods/message.ts explicitly resolves part.agentId ?? part.tool_call?.agentId—so restored multi-agent messages expose incomplete producer metadata unless the inner ID is included.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. Metadata agentIds falls back to tool_call.agentId when the outer part has none, matching the data-schemas ownership resolution. Covered by 'collects agents that own only a nested tool call'.
| const files = attachments.map( | ||
| (part) => | ||
| byPath.get(part.url) ?? { | ||
| filepath: part.url, | ||
| ...(part.filename && { filename: part.filename }), | ||
| type: part.mediaType, |
There was a problem hiding this comment.
When an attachment URL matches a stored file, this returns the base object wholesale, so UI edits to that part's filename or mediaType are silently discarded. Merge the editable UI fields into the matched record while retaining non-UI fields such as file_id; otherwise only newly added attachments can have those fields changed.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. A matched stored file now takes the view's url, filename and mediaType where they differ from the derived values (applyFileEdits), keeping file_id and the rest. Covered by 'writes attachment renames onto the matched stored file'.
| const hasOutput = output != null && output !== ''; | ||
|
|
||
| let state: UIToolState = complete ? 'input-available' : 'input-streaming'; | ||
| if (failed) { | ||
| state = 'output-error'; | ||
| } else if (hasOutput || runStepStatus === 'completed' || (progress ?? 0) >= 1) { |
There was a problem hiding this comment.
For a legacy FunctionToolCall, the contract uses null to mean the output was not submitted, so '' is a valid completed empty result. Excluding the empty string here classifies such a call as input-available and omits its output from the UI part, presenting a completed call as still awaiting execution; use the call variant's null sentinel rather than a blanket non-empty check.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. Output submission is now decided by each variant's own sentinel: a legacy function call counts as submitted when output is not null, so '' is a completed empty result. Covered by 'treats an empty legacy function output as submitted'.
| ...base, | ||
| ...pickMessageFields(metadata), | ||
| messageId: message.id, | ||
| isCreatedByUser: message.role === 'user', |
There was a problem hiding this comment.
A valid UIMessage with role: 'system' is collapsed to the same TMessage shape as an assistant here, and a subsequent toUIMessage returns it as assistant. This is observable for LibreChat's existing system-generated messages, which use sender: 'System' in the assistant and agent error paths; encode that distinction in the reverse mapping and recognize it on the forward mapping, or remove system from the exported role union.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. UIMessage.role is narrowed to 'user' | 'assistant': a TMessage has only isCreatedByUser, so a system role could not survive the reverse mapping, and the type now says so.
| const resolveText = (text: string, contentless: boolean, base?: TMessage) => { | ||
| if (contentless || base?.text === undefined) { | ||
| return text; |
There was a problem hiding this comment.
When no base is supplied, this always uses the concatenated UI text parts, even though the function's own invariant notes that a content-bearing TMessage.text is not necessarily derived from those parts. Thus fromUIMessage(toUIMessage(message)) loses a non-derived value such as the stored summary covered by the adjacent test whenever the message also has tool, reasoning, or differently worded text content; retain that original text in metadata when it differs so the documented base-less rebuild preserves it.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. When a content-bearing message's stored text differs from its joined text parts, toUIMessage carries it in metadata.text and a base-less fromUIMessage restores it. Covered by 'keeps stored text that the parts do not derive without a base'.
| if (text == null || typeof text === 'string') { | ||
| return { value: text ?? '' }; |
There was a problem hiding this comment.
For a valid placeholder such as { type: TEXT } or { type: THINK }, the optional payload is absent, but this normalizes it to ''; fromUIPart(toUIPart(part)) then adds text: '' or think: '' and violates the advertised lossless round-trip. Missing text updates are an actual stream shape (for example, edited content can carry only its index and type), so retain an absence marker instead of conflating it with an explicitly empty payload.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b71feb9. An absent text/think field is recorded as textAbsent and left off again on the way back, instead of becoming ''. Covered by 'keeps a text or think part without its text field'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b71feb9bb2
ℹ️ 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".
| case 'retrieval': | ||
| case 'file_search': | ||
| return { name: toolCall.type, id: toolCall.id || undefined, submitted: false }; |
There was a problem hiding this comment.
For persisted retrieval or file_search calls that carry a top-level output, this branch hard-codes submitted: false and discards the result. That runtime shape is already supported by client/src/components/Chat/Messages/Content/Part.tsx:461-470 and outcome.ts:162-168; without a newer runStepStatus or progress marker, the restored call is therefore reported as input-streaming, and even with a completion marker its output is missing. Preserve the supported output and derive submission from it.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a127f34. retrieval and file_search calls read their top-level output, counting it as submitted and carrying it on the part, matching Part.tsx and outcome.ts. Covered by 'reports a legacy retrieval output as submitted'.
| if (failure) { | ||
| state = 'output-error'; | ||
| } else if (submitted || runStepStatus === 'completed' || (progress ?? 0) >= 1) { | ||
| state = 'output-available'; |
There was a problem hiding this comment.
When an older persisted tool call has an error-prefixed output but no durable failure marker, submitted wins here and produces output-available. Existing rendering explicitly treats those strings as failures via OutputRenderer.tsx:25-27 and ToolCall.tsx:162, so the UI-parts view turns a restored failure into success; include the established output-error predicate when deriving this state.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a127f34. An output that isToolErrorOutput recognises (the tool-call error prefix or 'Error processing tool', the same rule as the client's isError) maps to output-error. The predicate now lives in data-provider errors.ts. Covered by 'reports an error-prefixed output as output-error'.
| } | ||
| } | ||
| const files = attachments.map((part) => { | ||
| const stored = byPath.get(part.url); |
There was a problem hiding this comment.
When a UI edit changes an attachment part's url, looking up the stored record by that already-edited value cannot find the original file. The code then creates a minimal replacement and drops file_id, ownership, storage metadata, and other fields instead of applying the filepath change that applyFileEdits advertises; retain a stable original identity in the part metadata or otherwise match the original occurrence before applying the new URL.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a127f34. Attachment parts carry providerMetadata.librechat { source: 'attachment', filepath }, the path they were viewed with, and reconcileFiles matches on that identity, so an edited url still finds the stored file and keeps file_id. Covered by 'matches an attachment whose url was edited to its stored file'.
| conversationId: message.conversationId, | ||
| parentMessageId: message.parentMessageId, | ||
| ...(message.sender !== undefined && { sender: message.sender }), | ||
| ...(message.model != null && { model: message.model }), |
There was a problem hiding this comment.
For a schema-valid message with model: null, the base-less round trip fromUIMessage(toUIMessage(message)) omits the field because this guard excludes null, even though pickMessageFields can restore it. The fresh evidence in the current head is this newly added != null guard after metadata restoration was introduced; use an undefined-only check so explicit null remains distinguishable from absence.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a127f34. toUIMessage keeps model when it is defined, null included, so a base-less rebuild restores model: null. Covered by 'keeps a null model through a base-less rebuild'.
| }; | ||
|
|
||
| const fromToolPart = (part: UIToolPart): TMessageContentParts => { | ||
| const stored = part.callProviderMetadata?.librechat; |
There was a problem hiding this comment.
When a mapped input-audio part is edited from one audio type to another, this branch reads the new bytes from part.url but always restores the original metadata format. For example, changing both fields to an audio/mp3 data URL still returns { format: 'wav' }, causing downstream consumers to interpret the replacement payload using the wrong encoding; derive the restored format from the edited mediaType or data URL.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a127f34. The restored input_audio format follows an edited audio/* mediaType and falls back to the stored format otherwise. Covered by 'takes the audio format from an edited media type'.
| const mapped = fromUIPart(part); | ||
| if (mapped) { | ||
| content[i] = mapped; | ||
| } else if (part.type === 'step-start') { | ||
| content.length = i + 1; |
There was a problem hiding this comment.
When a valid UI-parts array contains a source-url or attachment file before a later content-bearing part, assigning by the UI index creates an unintended sparse hole—for example, text/source/text becomes content at indexes 0 and 2 despite having no step-start. fromUIMessage repeats the same indexing pattern, so reordered or hand-built messages shift persisted step identities; advance a separate content-slot cursor only for mapped parts and explicit step-start placeholders.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a127f34. Both reverse paths now write through one slot writer: a mapped part takes the next slot, step-start holds one open as a hole, and source-url or attachment parts take none. Covered by 'gives sources and attachments no content slot'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a127f34343
ℹ️ 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".
| const { toolCall, ...partMetadata } = stored; | ||
| return { type: ContentTypes.TOOL_CALL, tool_call: toolCall, ...partMetadata }; | ||
| } | ||
| const output = typeof part.output === 'string' ? part.output : part.errorText; |
There was a problem hiding this comment.
When a valid hand-built tool part has a CodeInterpreterOutputs array in output and no callProviderMetadata, this ternary discards the result because it retains only strings. For example, converting an output-available tool-code_interpreter part with output: [{ logs: 'done' }] produces a stored call with no output, so the result disappears on the next mapping; reconstruct the code-interpreter shape or otherwise retain the array in a durable representation.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9f4f2d. A hand-built tool part with an array output becomes a code_interpreter call carrying the outputs, so the result survives the next mapping. Covered by 'keeps an array output from a hand-built tool part'.
| ...(output !== undefined && { output }), | ||
| ...(part.state === 'output-error' && { runStepStatus: 'failed' as const }), |
There was a problem hiding this comment.
When a hand-built part explicitly has state: 'output-available' but its successful result is empty or omitted, this reverse mapping records no completion marker; an empty string is stored but readToolCall treats it as unsubmitted, while an omitted result is not stored at all. Mapping the rebuilt call again therefore changes it to input-available or input-streaming; persist a completed status for the explicit successful state, just as output-error persists a failed status here.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9f4f2d. A hand-built output-available tool part now records runStepStatus: 'completed', mirroring output-error's 'failed', so it maps back as output-available with or without output. Covered by 'keeps a successful hand-built tool part successful'.
| parts.push(filePart); | ||
| } | ||
| } | ||
| pushSourceParts(parts, message.attachments); |
There was a problem hiding this comment.
When fromUIMessage(toUIMessage(message)) is called without a stored base, this projects only web-search links from message.attachments; the reverse mapper ignores source-url parts and metadata carries no original attachments, so the entire field disappears. Web citation payloads are therefore lost, while memory, workspace, and other non-web attachments disappear without even a UI part; retain the original attachment records in provider metadata for the documented base-less rebuild path.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9f4f2d. toUIMessage carries the stored attachments in metadata.attachments, and a base-less fromUIMessage restores them. Covered by 'keeps attachments and an empty content array', a strict round-trip.
| const steers: UIMessageMetadata['steers'] = []; | ||
| const summaries: UIMessageMetadata['summaries'] = []; | ||
|
|
||
| const contentless = !content || (content.length === 0 && (message.text?.length ?? 0) > 0); |
There was a problem hiding this comment.
For a message with content: [] and nonempty text, this uses the same contentless marker as a message whose content field is absent, and a base-less fromUIMessage therefore rebuilds it with content: undefined. Those shapes are behaviorally distinct—for example, hasEditablePart treats missing content as an editable plain-text message but an empty array as having no editable parts—so metadata must record that the empty array existed and restore it.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9f4f2d. metadata.emptyContent marks a contentless view whose message stored content: [], and a base-less rebuild restores the empty array. Covered by the same strict round-trip test.
| type: part.mediaType, | ||
| }; | ||
| }); | ||
| return files.concat(hidden); |
There was a problem hiding this comment.
When base.files contains an entry without filepath before or between visible files, reconciliation moves that entry to the end because every hidden record is collected separately and appended after the mapped attachments. Thus even an unchanged fromUIMessage(toUIMessage(message), message) transforms [hidden, visible] into [visible, hidden], altering persisted attachment order for downstream consumers that iterate message.files; reconcile visible records into their original slots instead of suffixing all hidden records.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9f4f2d. Files the view never showed keep their stored slots, and visible files fill the remaining slots in view order, so an unchanged round-trip keeps [hidden, visible]. Covered by 'keeps hidden files in their stored positions'.
Adds a UIMessage-shaped view of TMessage modelled on the AI SDK (ai@7.0.114): content parts map one-to-one onto text, reasoning, tool-<name>, file and data-* parts, holes map to step-start, and the reverse restores the stored content so a view round-trips.
An empty content array with text now views as contentless, an edited text part updates the stored text, attachment file parts reconcile message files, and metadata fields restore a message rebuilt without a stored base.
Tool parts fall back from an empty id to the host step id, surface approval pauses and background-cancel or validation failures, and treat an empty legacy function output as submitted. The reverse mapping writes file and attachment edits over the stored records, keeps an explicit null parent, carries non-derived message text and absent text fields, persists hand-built tool failures, and collects text, content and attachments in one pass. Web search sources include link references and take message-wide ids. UIMessage drops the system role it could not round-trip. Adds mock-harness scenarios that view persisted replies, reasoning and tool turns and map them back.
Persisted reasoning is stored inside <think> tags that every renderer strips; the reasoning part now exposes the text alone and keeps the exact wrapper so the reverse mapping restores it. The tool-turn scenario skips mobile, where the composer MCP picker is not offered.
…comes Attachment file parts carry the stored path they were viewed with, so an edited url still matches its file. Reverse slots advance only for content parts, audio takes its format from an edited media type, a null model survives a base-less rebuild, legacy retrieval outputs count as submitted, and error-prefixed outputs map to output-error through isToolErrorOutput, the predicate the tool output renderers use.
…ugh a rebuild A base-less rebuild restores the stored attachments and an explicit empty content array from metadata. Hand-built tool parts record a completed status for output-available and keep an array output as a code interpreter call. Files the view never showed keep their stored positions while visible files fill the others in view order.
a127f34 to
d9f4f2d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9f4f2df80
ℹ️ 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".
| if (runStepStatus === 'failed' || runStepStatus === 'cancelled') { | ||
| return runStepStatus; | ||
| } | ||
| if (typeof output === 'string' && isToolErrorOutput(output)) { |
There was a problem hiding this comment.
When a persisted set_memory or delete_memory call returns failure prose such as Invalid key ... without a failed run-step marker, isToolErrorOutput does not recognize it, so the submitted output maps to output-available. The existing MemoryCall.tsx:16-23 deliberately classifies every non-success memory response as a failure, meaning the UI-parts view reports restored memory failures as successes; include that established memory predicate in failure derivation.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13b2e3e by moving the rule to its owner. Memory failure prose is a client rule (MemoryCall.tsx), and data-provider cannot import it without copying it. toUIMessage/toUIParts/toUIPart now take a resolveToolFailure option, so the client applies its own predicate, and the default stays with the markers stored on the call. Covered by 'applies a failure the caller resolves from its own rules'.
| input: typeof part.input === 'string' ? part.input : JSON.stringify(part.input ?? ''), | ||
| outputs: part.output, | ||
| }, | ||
| ...(part.state === 'output-error' && { runStepStatus: 'failed' as const }), |
There was a problem hiding this comment.
When a hand-built tool-code_interpreter explicitly has state: 'output-available' and output: [], this branch stores an empty outputs array without a completed marker; readToolCall then treats it as unsubmitted and maps it back to input-streaming. Fresh evidence in the current head is that this array-output branch records only the failed state here, while the completed marker at line 602 is unreachable for arrays. Persist runStepStatus: 'completed' for this branch as well.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13b2e3e. The hand-built array-output branch now records runStepStatus: 'completed' for output-available, so an empty outputs array maps back as output-available. Covered by 'marks an empty hand-built array output completed'.
| const { value } = splitText(part.text_delta ?? part.text); | ||
| return { type: 'text', text: value, state: 'streaming' }; |
There was a problem hiding this comment.
When a stream-only text_delta carries agentId, groupId, or streamedIndex—all explicitly allowed by TextDeltaContentPart—this branch returns a bare text part and discards those fields. Mapping a parallel or handoff stream therefore removes the producer/lane identity from the part, and the reverse mapping cannot restore it while normalizing the discriminator to text; retain the content metadata in providerMetadata as the ordinary text branch does.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13b2e3e. A text_delta keeps its ownership fields (agentId, groupId, streamedIndex) in providerMetadata like the text branch, so the reverse mapping restores them onto the text part. Covered by 'keeps ownership metadata on a streamed text delta'.
| if (content && !contentless) { | ||
| for (let i = 0; i < content.length; i++) { | ||
| const part = content[i] as MappableContentPart | undefined; | ||
| const uiPart = toUIPart(part, i); |
There was a problem hiding this comment.
When a detached tool keeps its benign dispatch handle and runStepStatus: 'completed', the actual error or cancellation can exist only in its correlated background_task_status attachment. The established outcome path in client/src/components/Chat/Messages/Content/outcome.ts:125-148 consumes that attachment, but this conversion maps the tool before consulting message.attachments, so the resulting UI part incorrectly reports output-available; correlate those status attachments while constructing message-level tool parts.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 13b2e3e through the same resolveToolFailure option. toUIMessage passes the message, so the client's outcome.ts can correlate background_task_status attachments with the call instead of the mapping copying that logic. Covered by 'applies a failure the caller resolves from its own rules', which asserts the resolver receives the message.
Some tool outcomes are decided by rules the client owns, such as memory failure prose and background status attachments. toUIMessage, toUIParts and toUIPart take a resolveToolFailure option instead of copying those rules. Text deltas keep their ownership metadata, an empty hand-built array output is marked completed, and fromUIMessage documents that a stored message round-trips only with its base.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13b2e3e62c
ℹ️ 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".
A dual-conversation turn seeds type '' placeholder slots, which fell through the part switch and crashed toUIMessage. Any slot without a known type now maps to step-start, and the reverse leaves a hole, the same thing persistence compaction does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16aa523372
ℹ️ 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".
| const byPath = new Map<string, Partial<TFile>>(); | ||
| for (const file of baseFiles ?? []) { | ||
| if (file.filepath) { | ||
| byPath.set(file.filepath, file); |
There was a problem hiding this comment.
Fresh evidence beyond the earlier URL-edit thread is that this Map retains only the last record for each filepath. For a schema-valid base.files containing two entries with the same path but different file_id or metadata, both generated file parts resolve to that final record, so even an unchanged fromUIMessage(toUIMessage(message), message) replaces the first file and corrupts its identity; retain an occurrence or file_id-based identity instead of keying solely by path. CLAUDE.mdL130-L134
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a85fbdf. Attachment parts carry their index in message.files beside the viewed path, and reconcileFiles matches on both. Two stored files sharing a path stay distinct, and a hand-built part claims the first unclaimed match. Covered by 'keeps two stored files that share a path distinct', a strict round-trip.
| if (!contentless) { | ||
| writer.write(part); |
There was a problem hiding this comment.
When a view originated from a plain message without content, metadata.contentless remains true even if a consumer appends a hand-built tool, reasoning, data, or content-backed file part. This guard then skips every slot-backed part, and fromUIMessage returns the base with no content (or its unchanged empty array), silently discarding the addition; only the synthetic text representation should be excluded when deciding whether newly added content must be written.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a85fbdf. A contentless view writes content once it holds a slot-backed part other than text (tool, reasoning, data, content file), so an appended part persists and message text is kept. Covered by 'writes content added to a contentless message'.
| return { input: args, complete: args != null }; | ||
| } | ||
| try { | ||
| return { input: JSON.parse(args) as UIToolInput, complete: true }; |
There was a problem hiding this comment.
When a stored tool call has a valid string argument such as "null", "42", or "true", JSON.parse succeeds but returns a value outside the declared UIToolInput union of strings and objects. The assertion therefore lets toUIPart expose input: null, a number, or a boolean while downstream consumers are type-checked as though that cannot happen, which can cause failures such as object operations on null; validate the parsed value or expand the public union to describe the actual result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a85fbdf. Parsed tool arguments are used only when they are a JSON object; a primitive such as '42' or 'null' stays its string, inside the declared UIToolInput union. Covered by 'keeps primitive JSON arguments as their string'.
…plain messages Attachment parts carry their index in message.files beside the path, so two files sharing a path stay distinct, and a hand-built part claims the first unclaimed match. Content parts added to a view of a plain message are written as content. Tool arguments that parse to a JSON primitive stay their string, inside the declared input type.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a85fbdfb89
ℹ️ 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".
|
|
||
| return { | ||
| type: `tool-${name}`, | ||
| toolCallId: id || `${ContentTypes.TOOL_CALL}-${index}`, |
There was a problem hiding this comment.
When a tool call has neither a provider id nor a stepId, this fallback uses the compacted content index even if preserveStreamedContentIdentity has stamped the outer part with its original streamedIndex. After a sparse streamed response settles into compacted content, the same invocation can therefore change from (for example) tool_call-3 to tool_call-2, defeating the stamp's purpose and causing state keyed by toolCallId to treat it as a different tool; generate the fallback from part.streamedIndex ?? index.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a496143. The fallback id is tool_call-<streamedIndex ?? index>, so a call without provider or step id keeps its identity after compaction. Covered by 'derives a fallback tool call id from the streamed index'.
| if (shape.textAbsent && value === '') { | ||
| return undefined; | ||
| } | ||
| return shape.textObject ? { ...shape.textObject, value } : value; |
There was a problem hiding this comment.
When an object-backed text part has file annotations and its UI text is edited, this unconditionally combines the new value with the original annotations. Their start_index, end_index, and annotated text still describe the old value, so fromUIPart persists citations pointing at unrelated ranges. Carry the original value in metadata and preserve annotations only while it is unchanged, or clear/rebase them when applying an edit.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a496143. Object-form text records the value its annotations index into; if the view's text differs, the annotations are dropped and an untouched part still round-trips strictly. Covered by 'drops annotations once their text is edited'.
…annotations A tool call with neither a provider id nor a step id takes its fallback id from the streamed index the settled message keeps, so its identity survives compaction. Annotations on object-form text are dropped once the view's text no longer matches the value they index into.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4961438ad
ℹ️ 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".
| } | ||
| case ContentTypes.TEXT_DELTA: { | ||
| const { type: _type, text, text_delta: textDelta, ...rest } = part; | ||
| const { value } = splitText(textDelta ?? text); |
There was a problem hiding this comment.
Beyond the earlier outer ownership fix, a supported text_delta whose payload is a TextData object still loses the object's annotations and embedded metadata because only value is retained here. For example, a streaming delta containing a file citation becomes a plain string-backed text part, so the citation disappears from the UI and cannot be restored; carry the selected payload's text shape into providerMetadata as the ordinary text branch does.
Useful? React with 👍 / 👎.
| ...(part.input !== undefined && { | ||
| args: typeof part.input === 'string' ? part.input : JSON.stringify(part.input), | ||
| }), |
There was a problem hiding this comment.
When a hand-built tool part explicitly has state: 'input-available' but its input is an empty or otherwise non-JSON string, this stores only that raw args value. Mapping the result forward again makes parseToolInput reject the string and changes the state to input-streaming, so an invocation declared ready appears to still be loading after a round-trip; persist enough state to distinguish complete raw input from a partial JSON fragment.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
| } else if (part.type !== 'source-url' && part.type !== 'step-start') { | ||
| addedContent = true; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier content-addition fix is that this condition explicitly excludes step-start: when an in-progress view originating from a contentless message adds only a step-start to reserve a content slot, the writer creates the hole but addedContent remains false, so fromUIMessage discards the entire reconstructed array. A subsequent restore therefore loses the reserved index and can shift later streamed parts; count step-start as added slot-backed content even though fromUIPart returns no value for it.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Summary
LibreChat message content is its own vocabulary (
TMessageContentParts, discriminated onContentTypes), so anything that wants to treat a message the way the AI SDK does has to translate every part type by hand. This adds that translation once, inpackages/data-provider, with no consumer changes.toUIPart/toUIPartsmap content to a UI parts union modelled on AI SDKUIMessage.parts(matched againstai@7.0.114):text,reasoning,tool-<name>with aninput-streaming | input-available | output-available | output-errorstate,file,source-url,step-start, anddata-*for agent updates, summaries, activity labels, steers and errors. Parts stay one-to-one with content slots, so the position-equals-step-index invariant holds; a hole maps tostep-startand back.fromUIPart/fromUIPartsrestore the stored content losslessly for everything except the stream-onlytext_delta.toUIMessage/fromUIMessagegive aUIMessageview ofTMessage, withagentIds,groupIds, activity labels, steers and summaries inmetadata.Type of change
Testing
Tested environments/configuration:
Unit level only; nothing consumes the mapping yet.
Automated tests:
packages/data-provider/src/parts.spec.ts: everyContentTypesmember (aRecord<ContentTypes, ...>makes a missing one a type error), round-trips for every lossless type, content arrays recorded by running the stream reducers fromclient/src/hooks/SSE/__tests__/steps.spec.ts(tool completed, streaming and cancelled, out-of-order holes, reasoning, handoff, failed summary, image), 250 generated sparse arrays checking order, holes and round-trip, and the reverse message mapping (empty content with text, edited text, attachment changes, metadata without a stored base)cd packages/data-provider && npx jest: 52 suites, 2197 passed, 1 skippedcd packages/data-provider && npx tsc --noEmit: cleannpm run static-checks -- --against origin/canary: all affected checks passedScreenshots / recordings
No user-facing change.
Risk / compatibility
None. New exports only; no existing type or function changes.
Checklist