Improve Claude stream completion handling - #1030
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughClaude SSE handling now distinguishes completion, refusal, token-limit, abort, and stream-failure outcomes. Successful answers are finalized explicitly, incomplete responses are rejected, cleanup is enforced, tests cover termination scenarios, and localized messages were added in 13 locales. ChangesClaude stream completion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeAPI
participant SSEStream
participant ConversationStorage
participant Port
SSEStream->>ClaudeAPI: Send message_delta and message_stop events
ClaudeAPI->>ClaudeAPI: Classify stop reason and stream state
ClaudeAPI->>ConversationStorage: Persist successful response
ClaudeAPI->>Port: Post completed response
SSEStream->>ClaudeAPI: Report stream end or error
ClaudeAPI->>ClaudeAPI: Abort or propagate incomplete stream failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix Claude SSE completion detection and surface token-limit/refusal errors
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
There was a problem hiding this comment.
Pull request overview
This PR improves Anthropic Claude API streaming completion handling so the extension can reliably distinguish successful completions from token-limit truncations, refusals, transport interruptions, and early EOFs—avoiding false “success” states (and duplicate terminal messages) seen with Claude Opus 5.
Changes:
- Track
stop_reason(message_delta) and requiremessage_stopbefore treating a stream as completed; surfacemax_tokensandrefusalas explicit errors. - Preserve “abort” behavior and ignore late response-stream interruptions after a confirmed successful completion.
- Add regression tests for incomplete responses, stream/transport failures, callback cleanup failures, and abort flows; add i18n keys for the new Claude-specific errors.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/services/apis/claude-api.mjs |
Implements stop-reason tracking, message_stop-gated completion, and refined error/abort handling for Claude SSE streams. |
tests/unit/services/apis/claude-api.test.mjs |
Adds targeted regression coverage for the new completion/error behaviors (max_tokens/refusal, EOF, stream failure, callback errors, abort). |
src/_locales/en/main.json |
Adds new Claude error strings as i18n keys. |
src/_locales/de/main.json |
Adds translations for the new Claude error strings. |
src/_locales/es/main.json |
Adds translations for the new Claude error strings. |
src/_locales/fr/main.json |
Adds translations for the new Claude error strings. |
src/_locales/in/main.json |
Adds translations for the new Claude error strings. |
src/_locales/it/main.json |
Adds translations for the new Claude error strings. |
src/_locales/ja/main.json |
Adds translations for the new Claude error strings. |
src/_locales/ko/main.json |
Adds translations for the new Claude error strings. |
src/_locales/pt/main.json |
Adds translations for the new Claude error strings. |
src/_locales/ru/main.json |
Adds translations for the new Claude error strings. |
src/_locales/tr/main.json |
Adds translations for the new Claude error strings. |
src/_locales/zh-hans/main.json |
Adds translations for the new Claude error strings. |
src/_locales/zh-hant/main.json |
Adds translations for the new Claude error strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review by Qodo
Context used✅ Compliance rules (platform):
6 rules 1. Claude strings exceed 100 columns
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| "Claude reached the response token limit. Increase Max Response Token Length and try again.": "Claude reached the response token limit. Increase Max Response Token Length and try again.", | ||
| "Claude declined to respond to this request.": "Claude declined to respond to this request.", | ||
| "Claude response stream ended before completion.": "Claude response stream ended before completion.", |
There was a problem hiding this comment.
1. Locale json lines exceed 100 📘 Rule violation ⚙ Maintainability
New localization entries add very long single-line JSON mappings that exceed the 100-character line-length limit. This reduces readability and violates the project’s max line length requirement.
Agent Prompt
## Issue description
New locale entries are written as single-line JSON key/value pairs whose physical line length exceeds 100 characters.
## Issue Context
These keys/values are full sentences (often duplicated as key and value), which makes each JSON line very long in multiple `src/_locales/*/main.json` files.
## Fix Focus Areas
- src/_locales/en/main.json[97-99]
- src/_locales/de/main.json[90-92]
Potential approaches:
- Reformat JSON entries so the key and value are on separate lines (only helps if each individual string stays <= 100 chars).
- Shorten the user-facing strings (and keep them consistent with any code that matches on these messages).
- If long translations are expected, add an explicit repo-wide exemption for `src/_locales/**/main.json` in the relevant line-length/linting configuration.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/apis/claude-api.mjs`:
- Around line 91-95: In the successful completion path, set
completedSuccessfully to true before calling pushRecord and before
port.postMessage. Keep the existing persistence and notification behavior
unchanged so the completion flag remains true if either side effect fails after
the response has been recorded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a65031d-7675-4dc9-bcab-b0d7d7534bda
📒 Files selected for processing (15)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/services/apis/claude-api.mjstests/unit/services/apis/claude-api.test.mjs
d015a29 to
02d0533
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/unit/services/apis/claude-api.test.mjs:19
- The new
setupCompletionTest()helper seeds config using legacy storage keys (customClaudeApiUrl,claudeApiKey). These tests are about Claude API streaming behavior, so tying them to config-migration behavior makes them more brittle (they’ll start failing if/when the migration cleanup is removed). Prefer setting the current config keys directly.
const setupCompletionTest = (modelName = 'claudeOpus5Api') => {
setStorage({
customClaudeApiUrl: 'https://api.anthropic.com',
claudeApiKey: 'sk-ant-test',
maxConversationContextLength: 3,
maxResponseTokenLength: 100,
temperature: 0.5,
})
![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: 02d0533a8a
ℹ️ 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 (!completedSuccessfully) { | ||
| throw new Error('Claude response stream ended before completion.') |
There was a problem hiding this comment.
When Anthropic accepts the request but later emits an SSE error event, such as overloaded_error, onMessage ignores that event and this fallback reports only “Claude response stream ended before completion.” This masks the provider's actionable error type/message during overloads and other mid-stream failures; capture data.error and surface it before applying the generic EOF fallback.
Useful? React with 👍 / 👎.
| if (stopReason === 'refusal') { | ||
| completionError = new Error('Claude declined to respond to this request.') | ||
| return | ||
| } |
There was a problem hiding this comment.
When Anthropic returns another non-final stop reason such as pause_turn, this blacklist falls through at message_stop, saves the partial answer, and reports success. A paused turn must be continued rather than committed as a completed response, so success should be limited to terminal reasons such as end_turn and stop_sequence, with pause_turn handled explicitly.
Useful? React with 👍 / 👎.
|
/agentic_review |
| if (data?.type === 'message_stop') { | ||
| if (stopReason === 'max_tokens') { | ||
| completionError = new Error( | ||
| 'Claude reached the response token limit. Increase Max Response Token Length and try again.', |
There was a problem hiding this comment.
1. Claude strings exceed 100 columns 📘 Rule violation ⚙ Maintainability
The new Claude error string and SSE fixtures contain physical lines longer than 100 characters. These additions violate the source line-length limit.
Agent Prompt
## Issue description
New Claude error and test fixture strings exceed the 100-character physical line limit.
## Issue Context
Preserve the exact runtime string values by wrapping them with concatenation or another formatting approach that does not introduce whitespace.
## Fix Focus Areas
- src/services/apis/claude-api.mjs[83-83]
- tests/unit/services/apis/claude-api.test.mjs[234-253]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 02d0533 |
02d0533 to
a33cc9d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/services/apis/claude-api.test.mjs (1)
499-514: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
finallycleanup actually ran.This is the only test covering the new
try/finallyinonEnd, but it stops at the rejection. Add an assertion thatonDisconnectwas still removed so a regression that drops thefinallywould fail here.♻️ Proposed addition
await assert.rejects(generateAnswersWithClaudeApi(port, 'Q', session), /listener cleanup failed/) + assert.equal(port.listenerCounts().onDisconnect, 0) + assert.deepEqual(session.conversationRecords, [{ question: 'Q', answer: 'Answer' }]) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/services/apis/claude-api.test.mjs` around lines 499 - 514, Extend the test for generateAnswersWithClaudeApi to assert that the onDisconnect listener was removed after the callback cleanup error. Keep the existing rejection assertion and verify the cleanup state after it, so the test fails if the onEnd finally block does not execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/services/apis/claude-api.test.mjs`:
- Around line 499-514: Extend the test for generateAnswersWithClaudeApi to
assert that the onDisconnect listener was removed after the callback cleanup
error. Keep the existing rejection assertion and verify the cleanup state after
it, so the test fails if the onEnd finally block does not execute.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a7141905-0d65-4694-931a-766dc0669e72
📒 Files selected for processing (15)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/services/apis/claude-api.mjstests/unit/services/apis/claude-api.test.mjs
🚧 Files skipped from review as they are similar to previous changes (11)
- src/_locales/en/main.json
- src/_locales/fr/main.json
- src/_locales/ko/main.json
- src/_locales/ja/main.json
- src/_locales/ru/main.json
- src/_locales/pt/main.json
- src/_locales/zh-hans/main.json
- src/_locales/zh-hant/main.json
- src/_locales/it/main.json
- src/_locales/de/main.json
- src/_locales/in/main.json
|
Code review by qodo was updated up to the latest commit a33cc9d |
Reject token, context, refusal, and nonterminal stop conditions instead of treating incomplete Claude responses as successful completions. Stop processing once a response completes and surface streamed API errors without undoing completed responses. Add localized completion errors and regression coverage for stop reasons, provider errors, transport failures, callbacks, and aborts. Follow-up to ChatGPTBox-dev#1026.
a33cc9d to
46786e0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/apis/claude-api.mjs (1)
137-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrdering nuance worth documenting:
wasAbortedshort-circuits beforecompletionError.Today this is safe because a
completionErroralways throws synchronously out ofonMessage, sofetchSSErejects viaonErrorwithout ever invokingonEnd(true)(tests attests/unit/services/apis/claude-api.test.mjsLines 542-581 pinreadCount === 1). But if that throw path is ever softened (e.g.onErrorswallowing instead of rethrowing), thecontroller.abort()on Line 105 would surface asonEnd(true)and this earlyreturnwould silently discard the token-limit/refusal error. A short comment here recording that invariant would protect the ordering against future refactors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/apis/claude-api.mjs` around lines 137 - 148, Document the ordering invariant at the early wasAborted return in the stream completion handling: it must remain before completionError because completionError currently rejects synchronously through onMessage/onError, preventing onEnd(true) from masking it. Add a concise comment near wasAborted that warns future changes must preserve this behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/services/apis/claude-api.mjs`:
- Around line 137-148: Document the ordering invariant at the early wasAborted
return in the stream completion handling: it must remain before completionError
because completionError currently rejects synchronously through
onMessage/onError, preventing onEnd(true) from masking it. Add a concise comment
near wasAborted that warns future changes must preserve this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5b24a81-ffb0-457e-9c96-4eeb1e4e1769
📒 Files selected for processing (15)
src/_locales/de/main.jsonsrc/_locales/en/main.jsonsrc/_locales/es/main.jsonsrc/_locales/fr/main.jsonsrc/_locales/in/main.jsonsrc/_locales/it/main.jsonsrc/_locales/ja/main.jsonsrc/_locales/ko/main.jsonsrc/_locales/pt/main.jsonsrc/_locales/ru/main.jsonsrc/_locales/tr/main.jsonsrc/_locales/zh-hans/main.jsonsrc/_locales/zh-hant/main.jsonsrc/services/apis/claude-api.mjstests/unit/services/apis/claude-api.test.mjs
🚧 Files skipped from review as they are similar to previous changes (11)
- src/_locales/ko/main.json
- src/_locales/fr/main.json
- src/_locales/zh-hant/main.json
- src/_locales/zh-hans/main.json
- src/_locales/ru/main.json
- src/_locales/de/main.json
- src/_locales/in/main.json
- src/_locales/it/main.json
- src/_locales/es/main.json
- src/_locales/en/main.json
- src/_locales/pt/main.json
|
Code review by qodo was updated up to the latest commit 46786e0 |
Follow-up to #1026.
What changed
max_tokensandrefusalstop reasons instead of treating them as successful completions.message_stopbefore accepting a Claude response as complete.Why
Claude Opus 5 can consume the configured response token limit while producing little or no visible text. The previous SSE handling treated these responses as successful and could also emit a duplicate completion message.
Validation
npm run prettynpm run lintnpm test(949/949 passing)npm run buildand expected build artifactsgit diff --checkLive Anthropic API smoke testing was not run because API credentials were unavailable.
Summary by CodeRabbit