feat(extension): harden provider and tool runtime seams#52
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds structured lifecycle diagnostics and refactors tool/provider lifecycle dispatch to use a shared dispatch path that emits diagnostics; it includes diagnostic helpers, updates to extension docs and roadmap, accepts empty-header provider mutations, and adds tests verifying diagnostics for success, noop, and error cases. ChangesTool and Provider Lifecycle Diagnostics
Possibly Related PRs
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 #52 +/- ##
==========================================
+ Coverage 61.65% 61.83% +0.17%
==========================================
Files 173 174 +1
Lines 17186 17250 +64
==========================================
+ Hits 10596 10666 +70
+ Misses 5535 5531 -4
+ Partials 1055 1053 -2
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/assistant/provider_hooks.go (1)
84-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve provider diagnostics on dispatch failures.
On Line 84-89, returning before diagnostics means
before_provider_request_diagnosticis never emitted when handlers fail, sohook_errors/timing/hook_count are dropped in the exact path where diagnostics are most needed.Suggested fix
result, err := runtime.dispatchLifecycle(ctx, extension.LifecycleBeforeProviderRequest, payload) -if err != nil { - return providerHookOutput{}, oops.In("assistant"). - Code("before_provider_request_dispatch_failed"). - Wrapf(err, "dispatch before_provider_request lifecycle") -} +// Build a best-effort output for diagnostics even when dispatch fails. +output := providerHookOutput{ + Payload: providerPayloadFromLifecycle(result.Payload, input.Payload), + Headers: mergeProviderHeaders(input.Headers, result.ProviderRequest.Headers), +} +runtime.emitLifecycleDiagnostics( + ctx, + extension.LifecycleBeforeProviderRequest, + &result, + providerHookDiagnostics(input, output), +) +if err != nil { + return providerHookOutput{}, oops.In("assistant"). + Code("before_provider_request_dispatch_failed"). + Wrapf(err, "dispatch before_provider_request lifecycle") +} ... -output := providerHookOutput{ - Payload: providerPayloadFromLifecycle(result.Payload, input.Payload), - Headers: mergeProviderHeaders(input.Headers, result.ProviderRequest.Headers), -} -runtime.emitLifecycleDiagnostics(...) return output, nilAlso applies to: 100-107
🤖 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_hooks.go` around lines 84 - 89, The dispatch error path for runtime.dispatchLifecycle (when called with extension.LifecycleBeforeProviderRequest) currently returns early and drops the diagnostics; update the error handling around dispatchLifecycle so that when an error occurs you still emit the before_provider_request_diagnostic (including hook_errors, timing, and hook_count) before returning the providerHookOutput{} error (i.e., capture diagnostics from the failed handler invocation and call the same diagnostic emission path used on success), and apply the same change to the analogous after-provider dispatch block (the second dispatchLifecycle call around lines 100-107) so both before and after provider request failures preserve and emit diagnostics.
🤖 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 `@internal/assistant/lifecycle.go`:
- Around line 237-240: When runtime.dispatchLifecycle(ctx,
extension.LifecycleToolCall, payload) returns both a non-nil result and an err,
ensure you call emitLifecycleDiagnostics with that result before returning the
error so hook_errors get sent; update the three call sites around
dispatchLifecycle (the lines using result, err :=
runtime.dispatchLifecycle(...)) to check for err and, if result != nil, call
emitLifecycleDiagnostics(ctx, result) (or the existing emitLifecycleDiagnostics
helper) and only then return the err. Also ensure the same pattern is applied to
the other two locations noted (the blocks at the other dispatchLifecycle calls)
so diagnostics are always emitted on failure paths.
---
Outside diff comments:
In `@internal/assistant/provider_hooks.go`:
- Around line 84-89: The dispatch error path for runtime.dispatchLifecycle (when
called with extension.LifecycleBeforeProviderRequest) currently returns early
and drops the diagnostics; update the error handling around dispatchLifecycle so
that when an error occurs you still emit the before_provider_request_diagnostic
(including hook_errors, timing, and hook_count) before returning the
providerHookOutput{} error (i.e., capture diagnostics from the failed handler
invocation and call the same diagnostic emission path used on success), and
apply the same change to the analogous after-provider dispatch block (the second
dispatchLifecycle call around lines 100-107) so both before and after provider
request failures preserve and emit diagnostics.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac1b81f9-135b-45f0-b20b-8a85acf77378
📒 Files selected for processing (12)
docs/extension-api.mddocs/extension-roadmap.mdinternal/assistant/lifecycle.gointernal/assistant/lifecycle_diagnostics.gointernal/assistant/provider_diagnostics_internal_test.gointernal/assistant/provider_hooks.gointernal/assistant/provider_hooks_internal_test.gointernal/assistant/runtime_events_test.gointernal/assistant/runtime_lifecycle_test.gointernal/assistant/tool_diagnostics_test.gointernal/assistant/tool_lifecycle_test.gointernal/extension/lifecycle.go
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/assistant/provider_diagnostics_internal_test.go (1)
15-114: ⚡ Quick winRefactor the provider diagnostics tests into a table-driven suite.
internal/assistant/provider_diagnostics_internal_test.gohas three standalone tests with largely repeated runtime setup + diagnostics subscription + dispatch; they differ mainly by the Lua handler and expected assertions. Consolidate into one table-driven test with subtests for success, noop handler, and handler error (reusing the existingcollectAssistantDiagnosticshelper).♻️ Suggested refactor shape
func TestRuntime_ProviderRequestHookEmitsDiagnostics(t *testing.T) { t.Parallel() tests := []struct{ name string lua string input providerHookInput assertFn func(t *testing.T, events *[]map[string]any, result *providerHookResult, err error) expectErr bool }{ // success / noop / handler error } for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { runtime := newProviderHookTestRuntime(t, tt.lua) events := collectAssistantDiagnostics(t, runtime.EventBus(), string(extension.LifecycleBeforeProviderRequest)+"_diagnostic") result, err := runtime.dispatchProviderRequestHook(context.Background(), tt.input) if tt.expectErr { require.Error(t, err) } else { require.NoError(t, err) } tt.assertFn(t, events, result, err) }) } }🤖 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 15 - 114, Consolidate the three standalone tests into a single table-driven test (keep the existing TestRuntime_ProviderRequestHookEmitsDiagnostics name) by creating a tests slice describing name, lua (handler), input (providerHookInput), expectErr bool and assertFn func(t,*[]map[string]any,*providerHookResult,error); for each case create runtime via newProviderHookTestRuntime, subscribe with collectAssistantDiagnostics(runtime.EventBus(), string(extension.LifecycleBeforeProviderRequest)+"_diagnostic"), call runtime.dispatchProviderRequestHook and t.Run the subtest (remember tt := tt to capture loop variable), then check err with expectErr and invoke tt.assertFn to perform the existing per-case assertions (success, noop, handler error) so you reuse dispatchProviderRequestHook, collectAssistantDiagnostics and the original assertions in subtests.internal/assistant/tool_lifecycle_test.go (1)
205-208: ⚡ Quick winAssert diagnostic values, not just key presence.
This currently passes even if
hook_errorsis present but empty/malformed. Please also asserthook_count == 1andhook_errorsis non-empty for both diagnostics.Suggested assertion hardening
require.Len(t, *toolCallDiagnostics, 1) require.Contains(t, (*toolCallDiagnostics)[0], "hook_errors") + assert.Equal(t, 1, (*toolCallDiagnostics)[0]["hook_count"]) + require.NotEmpty(t, (*toolCallDiagnostics)[0]["hook_errors"]) require.Len(t, *toolResultDiagnostics, 1) require.Contains(t, (*toolResultDiagnostics)[0], "hook_errors") + assert.Equal(t, 1, (*toolResultDiagnostics)[0]["hook_count"]) + require.NotEmpty(t, (*toolResultDiagnostics)[0]["hook_errors"])🤖 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/tool_lifecycle_test.go` around lines 205 - 208, The tests only assert the presence of the "hook_errors" key; update the assertions on toolCallDiagnostics and toolResultDiagnostics to validate the diagnostic values as well: after confirming length and key presence for the slices referenced by toolCallDiagnostics and toolResultDiagnostics, parse or assert the JSON/structure contains "hook_count == 1" and that "hook_errors" is non-empty (e.g., len > 0 or non-empty string/array) for both diagnostics entries; use the same require.* helpers already imported to fail the test if hook_count is not 1 or hook_errors is empty for the entries in toolCallDiagnostics and toolResultDiagnostics.
🤖 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_diagnostics_internal_test.go`:
- Around line 15-114: Consolidate the three standalone tests into a single
table-driven test (keep the existing
TestRuntime_ProviderRequestHookEmitsDiagnostics name) by creating a tests slice
describing name, lua (handler), input (providerHookInput), expectErr bool and
assertFn func(t,*[]map[string]any,*providerHookResult,error); for each case
create runtime via newProviderHookTestRuntime, subscribe with
collectAssistantDiagnostics(runtime.EventBus(),
string(extension.LifecycleBeforeProviderRequest)+"_diagnostic"), call
runtime.dispatchProviderRequestHook and t.Run the subtest (remember tt := tt to
capture loop variable), then check err with expectErr and invoke tt.assertFn to
perform the existing per-case assertions (success, noop, handler error) so you
reuse dispatchProviderRequestHook, collectAssistantDiagnostics and the original
assertions in subtests.
In `@internal/assistant/tool_lifecycle_test.go`:
- Around line 205-208: The tests only assert the presence of the "hook_errors"
key; update the assertions on toolCallDiagnostics and toolResultDiagnostics to
validate the diagnostic values as well: after confirming length and key presence
for the slices referenced by toolCallDiagnostics and toolResultDiagnostics,
parse or assert the JSON/structure contains "hook_count == 1" and that
"hook_errors" is non-empty (e.g., len > 0 or non-empty string/array) for both
diagnostics entries; use the same require.* helpers already imported to fail the
test if hook_count is not 1 or hook_errors is empty for the entries in
toolCallDiagnostics and toolResultDiagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 351780f8-a228-4520-a463-3b39d9a68c08
📒 Files selected for processing (5)
internal/assistant/lifecycle.gointernal/assistant/provider_diagnostics_internal_test.gointernal/assistant/provider_hooks.gointernal/assistant/provider_hooks_internal_test.gointernal/assistant/tool_lifecycle_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/assistant/provider_hooks_internal_test.go
- internal/assistant/lifecycle.go
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.
Summary
Validation
Note: docs/refactoring/* remain local and are intentionally excluded.