Repository navigation
🔁 fix: Keep the Error Card When a Stream Reattached Without Resume State Fails - #17112
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. |
d88d6f6 to
c9721d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9721d1923
ℹ️ 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, | ||
| userMessage, |
There was a problem hiding this comment.
Avoid appending the resumed user row twice
When a bare-status reattach finds lastUserMessage, that exact row is already present in messages, but the submission retains the full array and sets isRegenerate: false. If the reattached stream emits the error this change targets, errorHandler calls mergeErrorMessages, whose normal path returns [...messages, userMessage, errorMessage]; this duplicates the prompt (including its messageId) in the rendered and React Query-cached transcript. Exclude the selected user row from the submission prefix, or otherwise use a merge path that appends only the error for this resume case.
AGENTS.md reference: AGENTS.md:L50-L53
Useful? React with 👍 / 👎.
c9721d1 to
ffcbe71
Compare
ffcbe71 to
79364f3
Compare
Pull Request
Summary
When the stream status route answers a running job without
resumeState(an older server, or a status that omits it),useResumeOnLoadattaches with a minimal submission whose placeholder response has noisCreatedByUserand no parent. If the reattached stream then reports a failure, building the error turn from that placeholder throws a ZodError (isCreatedByUseris required), so the error card is never written and the pane stayssubmitted. The placeholder is now a parented assistant row (isCreatedByUser: false, parented under the last user turn), and the fallback user row is marked as the user's, so the failure renders as an error like any other. Fixes berry-13#223.The fix lands where the resume rules now live: the resume-state helpers (rebuilding a submission from resume state or from a bare status, matching a submission to its resume state, the branch to restore, and the merge of a resumed pair into history) move out of
useResumeOnLoad.tsanduseResumableSSE.tsintopackages/chat/src/core/resumable/resume.ts, and the pending-action appliers they use move fromutils/approval.tstopackages/chat/src/core/pending.ts, whichutils/approval.tsre-exports. Apart from the fix, no behavior changes. Stacked on #17110.Type of change
Testing
Tested environments/configuration:
The failure path needs a status without
resumeState, which current servers do not send, so it is covered by the spec below rather than by hand.Automated tests:
transport.spec.tsx(resume): reattaches through a status withoutresumeState, emits an error on the stream, and expects the chat to settle on that error. It fails on the base with the ZodError and passes with the fix.packages/chat/src/core/__tests__/resume.spec.tsfor the moved resume helpers and the minimal submission.npx jest src/coreinpackages/chat, and inclientthe hooks/SSE, hooks/Chat and hooks/Agents specs,utils/approval.spec.tsandPendingSteers.test.tsx: all pass.npx tsc --noEmitinpackages/chatandclient, eslint, prettier and sort-imports on the changed files: clean.Screenshots / recordings
No user-facing change.
Risk / compatibility
None.
utils/approval.tskeeps exporting the pending-action appliers, now from@librechat/chat.Checklist