feat: harden provider seam diagnostics#55
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR refactors provider diagnostics by introducing shared payload builders and integrating them into the unified lifecycle event dispatch pipeline. ChangesProvider Lifecycle Diagnostics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 #55 +/- ##
==========================================
+ Coverage 61.96% 62.02% +0.06%
==========================================
Files 174 174
Lines 17304 17348 +44
==========================================
+ Hits 10722 10760 +38
- Misses 5525 5528 +3
- Partials 1057 1060 +3
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:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/assistant/provider_seam_hardening_test.go (2)
67-70: ⚡ Quick winUse
oops.Wrapfpattern for contextual errors instead of constructing withErrorfhere.This repo’s Go guideline prefers chaining contextual metadata with
Wrapf(err, "message")in oops-enabled packages; please switch this test error construction to that pattern for consistency.As per coding guidelines,
**/*.go: Useoops.In("domain").Code("code").Wrapf(err, "message")for contextual errors in packages that already usesamber/oops.🤖 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_seam_hardening_test.go` around lines 67 - 70, Replace the direct Errorf construction with the oops.Wrapf pattern: create a base error (e.g., errors.New or fmt.Errorf("service unavailable")) and then call oops.In("assistant").Code("responses_status").Wrapf(baseErr, "service unavailable") to attach the contextual metadata; update the symbol providerErr to be assigned from that Wrapf chain (using the same In/Code chain).
17-125: ⚡ Quick winPrefer a table-driven test for these provider diagnostic scenarios.
These cases cover one core behavior surface and share setup/assertion shape; a table-driven structure will reduce duplication and make future scenario additions safer.
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/provider_seam_hardening_test.go` around lines 17 - 125, These three tests (TestRuntime_ProviderLifecycleEmitsDiagnostics, TestRuntime_ProviderErrorDiagnosticsIncludeRetryAndErrorMetadata, TestRuntime_ProviderErrorDiagnosticsCaptureHookErrors) exercise the same provider-diagnostics behavior and should be combined into a single table-driven test: create a slice of cases that configures the staticCompletionClient (result/err), optional extension load (hook error case), expected diagnostic topic (string(extension.LifecycleAfterProviderResponse)+"_diagnostic" or string(extension.LifecycleProviderError)+"_diagnostic"), and expected assertions (hook_count, provider, model, token counts, error metadata, hook_errors, etc.), then loop cases calling newTestRuntimeWithManager, collectRuntimeDiagnosticPayloads, runtime.Prompt and run the appropriate assertions per case; keep helpers newTestRuntimeWithManager, collectRuntimeDiagnosticPayloads and runtime.Prompt as-is and reference their names in the table entries to reduce duplication and make adding scenarios simpler.
🤖 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/provider_seam_hardening_test.go`:
- Around line 67-70: Replace the direct Errorf construction with the oops.Wrapf
pattern: create a base error (e.g., errors.New or fmt.Errorf("service
unavailable")) and then call
oops.In("assistant").Code("responses_status").Wrapf(baseErr, "service
unavailable") to attach the contextual metadata; update the symbol providerErr
to be assigned from that Wrapf chain (using the same In/Code chain).
- Around line 17-125: These three tests
(TestRuntime_ProviderLifecycleEmitsDiagnostics,
TestRuntime_ProviderErrorDiagnosticsIncludeRetryAndErrorMetadata,
TestRuntime_ProviderErrorDiagnosticsCaptureHookErrors) exercise the same
provider-diagnostics behavior and should be combined into a single table-driven
test: create a slice of cases that configures the staticCompletionClient
(result/err), optional extension load (hook error case), expected diagnostic
topic (string(extension.LifecycleAfterProviderResponse)+"_diagnostic" or
string(extension.LifecycleProviderError)+"_diagnostic"), and expected assertions
(hook_count, provider, model, token counts, error metadata, hook_errors, etc.),
then loop cases calling newTestRuntimeWithManager,
collectRuntimeDiagnosticPayloads, runtime.Prompt and run the appropriate
assertions per case; keep helpers newTestRuntimeWithManager,
collectRuntimeDiagnosticPayloads and runtime.Prompt as-is and reference their
names in the table entries to reduce duplication and make adding scenarios
simpler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cbdb0a3-1310-4c57-9e01-f18cdf4b65a7
📒 Files selected for processing (3)
internal/assistant/lifecycle_diagnostics.gointernal/assistant/provider_seam_hardening_test.gointernal/assistant/runtime_events.go



Summary
Validation
Note: docs/refactoring/* remain local/uncommitted and are not part of this PR.