fix(provider): surface incomplete response streams#53
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughSSE parsing now recognizes typed response.* events, tracks completion and terminal failures, extracts provider-specific errors, fails on incomplete typed responses, updates retry rules for stream failures, and adapts tests and diagnostics to use response.completed events. ChangesStream Completion & Error Handling
SSE Stream Failure Tests and Retry Logic
Test Infrastructure Refactoring
Dependencies
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 61.83% 61.96% +0.13%
==========================================
Files 174 174
Lines 17250 17304 +54
==========================================
+ Hits 10666 10722 +56
+ Misses 5531 5525 -6
- Partials 1053 1057 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/assistant/client_test.go (1)
83-118: ⚡ Quick winPrefer table-driven coverage for the new parseSSEResult failure cases.
These three tests are structurally the same and can be consolidated into one table-driven test to reduce duplication and make future SSE error cases easier to add.
As per coding guidelines,
**/*_test.go: "Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs".🤖 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 `@internal/assistant/client_test.go` around lines 83 - 118, Consolidate the three near-identical tests into a single table-driven test (e.g., TestParseSSEResultFailureCases) that iterates over cases each containing a descriptive name, the SSE stream string, and the expected error substring; inside the loop use t.Run(name, func(t *testing.T){ t.Parallel(); _, err := parseSSEResult(strings.NewReader(stream), nil); require.Error(t, err); assert.Contains(t, err.Error(), expected) }) so you keep the same assertions (provider stream closed before completion, server overloaded, provider response incomplete: content_filter) and reduce duplication while still exercising parseSSEResult.internal/assistant/provider_diagnostics_internal_test.go (2)
148-149: ⚡ Quick winStrengthen
hook_errorsassertion shape/content checks.Line 148 currently only verifies key presence. Validate type and non-empty content to avoid false positives in diagnostics regressions.
Proposed fix
require.Contains(t, diagnostic, lifecycleErrorsKey) + hookErrors, ok := diagnostic[lifecycleErrorsKey].([]string) + require.True(t, ok) + require.NotEmpty(t, hookErrors)🤖 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 `@internal/assistant/provider_diagnostics_internal_test.go` around lines 148 - 149, The test only checks that lifecycleErrorsKey exists in diagnostic; update the assertion to also check the value under diagnostic[lifecycleErrorsKey] is the expected type and non-empty: after require.Contains(t, diagnostic, lifecycleErrorsKey) retrieve val := diagnostic[lifecycleErrorsKey], use require.IsType(t, []interface{}{}, val) or the correct expected type and then require.NotEmpty(t, val) (or require.Greater(t, len(val.([]interface{})), 0)) to ensure hook_errors contains a non-empty collection; reference lifecycleErrorsKey and diagnostic to locate and change the assertions.
30-34: ⚡ Quick winStrengthen the lifecycleErrorsKey check (range-variable capture is not applicable)
- The module targets Go
1.26, so the parallel subtest range-variable capture concern doesn’t apply here.- The error-path test only does
require.Contains(t, diagnostic, lifecycleErrorsKey); it can still pass ifdiagnostic[lifecycleErrorsKey]is the wrong type or empty—assert the expected type/contents (e.g., non-empty and exact shape/value) fordiagnostic[lifecycleErrorsKey].🤖 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 `@internal/assistant/provider_diagnostics_internal_test.go` around lines 30 - 34, The existing test only checks require.Contains(t, diagnostic, lifecycleErrorsKey) which can pass for wrong types or empty values; update the assertion (in the test that calls runProviderHookDiagnosticsTest or inside that helper where `diagnostic` is produced) to assert the concrete type and non-empty contents for `diagnostic[lifecycleErrorsKey]` (e.g., use require.IsType(t, []string{}, diagnostic[lifecycleErrorsKey]) or require.IsType(t, map[string]interface{}{}, ...) as appropriate, then require.NotEmpty(t, diagnostic[lifecycleErrorsKey]) and finally assert the expected shape/value with require.Equal or require.Subset for the specific error strings), referencing the `lifecycleErrorsKey` symbol and the `runProviderHookDiagnosticsTest` helper to locate where to add these stronger checks.
🤖 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 `@internal/assistant/client_test.go`:
- Around line 83-118: Consolidate the three near-identical tests into a single
table-driven test (e.g., TestParseSSEResultFailureCases) that iterates over
cases each containing a descriptive name, the SSE stream string, and the
expected error substring; inside the loop use t.Run(name, func(t *testing.T){
t.Parallel(); _, err := parseSSEResult(strings.NewReader(stream), nil);
require.Error(t, err); assert.Contains(t, err.Error(), expected) }) so you keep
the same assertions (provider stream closed before completion, server
overloaded, provider response incomplete: content_filter) and reduce duplication
while still exercising parseSSEResult.
In `@internal/assistant/provider_diagnostics_internal_test.go`:
- Around line 148-149: The test only checks that lifecycleErrorsKey exists in
diagnostic; update the assertion to also check the value under
diagnostic[lifecycleErrorsKey] is the expected type and non-empty: after
require.Contains(t, diagnostic, lifecycleErrorsKey) retrieve val :=
diagnostic[lifecycleErrorsKey], use require.IsType(t, []interface{}{}, val) or
the correct expected type and then require.NotEmpty(t, val) (or
require.Greater(t, len(val.([]interface{})), 0)) to ensure hook_errors contains
a non-empty collection; reference lifecycleErrorsKey and diagnostic to locate
and change the assertions.
- Around line 30-34: The existing test only checks require.Contains(t,
diagnostic, lifecycleErrorsKey) which can pass for wrong types or empty values;
update the assertion (in the test that calls runProviderHookDiagnosticsTest or
inside that helper where `diagnostic` is produced) to assert the concrete type
and non-empty contents for `diagnostic[lifecycleErrorsKey]` (e.g., use
require.IsType(t, []string{}, diagnostic[lifecycleErrorsKey]) or
require.IsType(t, map[string]interface{}{}, ...) as appropriate, then
require.NotEmpty(t, diagnostic[lifecycleErrorsKey]) and finally assert the
expected shape/value with require.Equal or require.Subset for the specific error
strings), referencing the `lifecycleErrorsKey` symbol and the
`runProviderHookDiagnosticsTest` helper to locate where to add these stronger
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faeb4be1-206a-4638-871b-0ef94b8bf463
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modinternal/assistant/client_test.gointernal/assistant/provider_diagnostics_internal_test.gointernal/assistant/retry.gointernal/assistant/retry_test.gointernal/assistant/sse.gointernal/assistant/tool_lifecycle_test.gointernal/assistant/usage_test.go
💤 Files with no reviewable changes (1)
- go.mod
|
Actionable comments posted: 0 |
|
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.