📨 fix: Resolve Stored Responses by Their IDs - #16456
lia-by-librechat[bot] wants to merge 2 commits into
Conversation
|
Review handoff for exact head |
|
Lighthouse CI failed. The last 80 log lines contain the measured budgets and assertion failures. |
|
Review handoff for exact pushed head |
|
@codex review |
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: 385b15018a
ℹ️ 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".
| ? messages.findIndex((message) => message.messageId === responseId) | ||
| : -1; | ||
| if (responseId && lastIndex < 0) return []; | ||
| return (responseId ? messages.slice(0, lastIndex + 1) : messages).filter( |
There was a problem hiding this comment.
When a client branches from an older response, this chronological slice includes unrelated sibling turns. For example, after resp_1 → resp_2, creating resp_3 from resp_1 stores all three in one conversation; subsequently continuing resp_3 slices through resp_2, so the model receives the sibling user prompt and answer even though they are not ancestors of resp_3. Persist and traverse the previous_response_id relationship, or otherwise isolate branches, rather than treating every earlier conversation message as history.
Useful? React with 👍 / 👎.
| previousResponse = await resolveStoredResponse( | ||
| principal.userId, | ||
| request.previous_response_id, | ||
| db, | ||
| ); |
There was a problem hiding this comment.
/api
Move this new validation, database resolution, error mapping, and branching into a TypeScript service in packages/api, leaving this CJS controller to pass request data and invoke it. Keeping the behavior here directly violates the repository boundary for edits to existing /api files and makes the new storage protocol harder to reuse and typecheck.
AGENTS.md reference: AGENTS.md:L64-L68
Useful? React with 👍 / 👎.
| const previousMessages = previousResponse | ||
| ? await loadPreviousMessages( | ||
| conversationId, | ||
| principal.userId, | ||
| previousResponse.message?.messageId, | ||
| ) |
There was a problem hiding this comment.
For a resp_… continuation, resolveStoredResponse serially awaits getMessage and getConvo, and this block then performs getMessages only after both have completed (with admission reads also occurring in between). Thus every continued response now pays at least three sequential Mongo round trips; return or load the conversation and history together, or parallelize the latter reads once the message identifies its conversation. The repository specifically requires message-loading changes to avoid serial database reads, and its CI models 250 ms per query.
AGENTS.md reference: AGENTS.md:L51-L54
Useful? React with 👍 / 👎.
| return (responseId ? messages.slice(0, lastIndex + 1) : messages).filter( | ||
| (message) => | ||
| !message.messageId?.startsWith(RESPONSE_ID_PREFIX) || message.finish_reason === 'stop', |
There was a problem hiding this comment.
If saveConversation or the final updateMessage fails after the input was saved, the request reports response_storage_failed, but this filter removes only the pending_storage assistant row and retains the associated user messages. A later continuation via the supported legacy conversation ID—and any later response-ID continuation created after that—therefore sends the supposedly failed prompt to the model. The commit protocol needs to roll back the input or tag every message in the turn so history selection can exclude the whole uncommitted turn.
Useful? React with 👍 / 👎.
|
|
||
| // Load messages for this conversation | ||
| const messages = await db.getMessages({ conversationId: responseId, user: userId }); | ||
| const { conversation, message } = stored; |
There was a problem hiding this comment.
For the newly supported response-ID lookup, the owning conversation stores the requested agent under agent_id, while the response builder later reads conversation.agentId || conversation.model. Lean conversation documents do not alias agent_id to agentId, so a retrieved response reports the provider model (or unknown) instead of the agent ID that the original POST returned in model. Use conversation.agent_id or persist the original response model with the message.
Useful? React with 👍 / 👎.
|
|
||
| // Load messages for this conversation | ||
| const messages = await db.getMessages({ conversationId: responseId, user: userId }); | ||
| const { conversation, message } = stored; |
There was a problem hiding this comment.
This response-ID path has the specific message, but the returned created_at and completed_at are still derived from the conversation. After later continuations, retrieving an earlier response therefore reports the conversation's original creation time and its latest update time, so the same stored response's completion timestamp changes whenever another turn is added. Use the resolved message's timestamps for response-ID reads; the new StoredResponseMessage contract already exposes createdAt and updatedAt.
Useful? React with 👍 / 👎.
This branch has not been deployed
2 participants
What breaks
POST /api/agents/v1/responsesreturnsresp_…butGET /responses/:idandprevious_response_idlooked for a conversation with that ID. The reply is actually a message in a separate conversation, so clients cannot retrieve or continue the response they were handed.store: truealso passed ExpressreqwheresaveMessageexpects an authenticated{ userId }context, and streamedresponse.completedwas emitted before any required write. Save failures were logged and ignored.What changes
resp_…via the existing indexed, owner-scopedgetMessage, then verify the owner-scoped conversation. Retain authorized conversation-ID reads for existing clients. A returned response ID retrieves only its own reply; continuation replays only through that reply.stoponly once the snapshot exists. Pending or failed writes cannot be retrieved or reused, even on an existing conversation. Read errors fail rather than pretending the history was empty. No automatic retries.response.completedonly after the writes commit. Storage errors emitresponse.failedwith a bounded error and[DONE]. JSON errors return 500 instead of a successful body. Reply announcements remain best-effort after durable storage.Related to AI-1593 and #14466.
Verification
mongodb-memory-servercontract test exercisessaveMessage,saveConvo,updateMessage,getMessage, andgetConvowith pending/committed/other-owner responses.packages/apitypecheck, package build and scoped static checks.Limits
This PR does not change tool execution, subagent discovery, typed history storage, reasoning replay, or Anthropic protocol support. Older conversation-ID requests keep their previous behavior. Pending partial rows are not automatically retried or declared complete. The separate subagent parity slice can land independently; coordinate if it starts changing
responses.jsin the same release window.