fix: rebuild stream context after sanitizing retry history#36
fix: rebuild stream context after sanitizing retry history#36sichengchen merged 2 commits intomainfrom
Conversation
When a retryable provider error occurs, sanitizeHistoryForRetry assigns a new array to this.messages, but the retry loop kept reusing the context object created before the loop. This meant context.messages still pointed to the old unsanitized array, so retries resent the same malformed history. Update context.messages after sanitization so retries use the cleaned history. Adds regression test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: 4cecc98250
ℹ️ 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".
src/engine/agent/agent.test.ts
Outdated
| stream: async function* (_model: any, context: any) { | ||
| callCount++; | ||
| // Capture a snapshot of context.messages for each call | ||
| capturedMessages.push([...context.messages]); |
There was a problem hiding this comment.
capturedMessages.push([...context.messages]) snapshots message contents instead of preserving the array reference, and the test later only checks for errorMessage fields. In this code path the retryable provider error is not appended to this.messages before sanitization (agent.ts keeps only the user message), so sanitized and unsanitized histories are content-identical; this test will pass even if context.messages = this.messages is removed, leaving the stale-reference bug unguarded.
Useful? React with 👍 / 👎.
The previous test snapshot content with spread, which was identical before and after sanitization (the error is never pushed to history). Now captures the raw array reference and asserts the retry receives a different reference, which only passes when context.messages is reassigned after sanitizeHistoryForRetry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Agent.chat(),sanitizeHistoryForRetry()assigns a new array tothis.messages, but thecontextobject passed tostream()still held a reference to the old array. Retries resent the unsanitized history, so errors like Geminithought_signaturefailures repeated on every attempt instead of recovering.context.messages = this.messagesafter sanitization so retries use the cleaned history.Test plan
bun test src/engine/agent/agent.test.ts— 4/4 pass (including new regression test)bun run typecheck— no new errors🤖 Generated with Claude Code