feat(extension): add tool runtime hooks#48
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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds extension-driven tool-call and tool-result mutation support: callback signatures now accept context and return errors, the extension lifecycle can return tool mutations parsed from Lua, the runtime dispatches lifecycle events and applies mutations, and the tool loop is refactored to use these dispatch hooks; tests exercise end-to-end behavior. ChangesTool Lifecycle Dispatch and Extension Mutations
Sequence DiagramsequenceDiagram
participant Client as Client/executeToolCalls
participant Hook as dispatchToolCallHook
participant Dispatch as dispatchToolCallLifecycle
participant Extensions as Lua Extensions
participant Execute as runToolCall
participant ResultHook as dispatchToolResultHook
Client->>Hook: onToolCall(ctx, *ToolCallEvent)
Hook->>Dispatch: delegate dispatch
Dispatch->>Extensions: emit LifecycleToolCall, collect mutations
Extensions->>Dispatch: return ToolCallMutation
Dispatch->>Hook: apply mutation to event.Arguments
Hook->>Execute: execute tool via registry
Execute->>ResultHook: onToolResult(ctx, *ToolEvent)
ResultHook->>Dispatch: delegate dispatch
Dispatch->>Extensions: emit LifecycleToolResult, collect mutations
Extensions->>Dispatch: return ToolResultMutation
Dispatch->>ResultHook: apply mutations to event.Result/DetailsJSON/Error
ResultHook->>Client: return final tool result
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 #48 +/- ##
==========================================
+ Coverage 61.55% 61.65% +0.10%
==========================================
Files 173 173
Lines 17073 17186 +113
==========================================
+ Hits 10509 10596 +87
- Misses 5516 5535 +19
- Partials 1048 1055 +7
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
🧹 Nitpick comments (1)
internal/assistant/tool_lifecycle_test.go (1)
13-75: ⚡ Quick winConvert these lifecycle mutation tests to a table-driven structure.
These are core behavior cases, and table-driven form will make it easier to expand mutation scenarios and regressions without duplicating setup.
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/tool_lifecycle_test.go` around lines 13 - 75, Convert the two tests into table-driven subtests: for TestRuntime_ToolCallLifecycleAppliesArgumentMutation, create a slice of cases each with a name, Lua extension string, initial call (assistant.ToolCallEvent) and expected mutated fields, then loop cases with t.Run(case.name, func(t *testing.T){ t.Parallel(); setup runtime/manager via newTestRuntimeWithManager, loadRuntimeExtension(manager, case.lua), call runtime.DispatchToolCallLifecycleForTest(...), require.NoError(t, err) and assert expected Argument values and ArgumentsJSON using assert.Equal / assert.JSONEq }); do the analogous table-driven refactor for TestRuntime_ToolResultLifecycleAppliesResultMutation using cases that include initial assistant.ToolEvent and expected Result, DetailsJSON and Error, invoking runtime.DispatchToolResultLifecycleForTest and asserting expectations; reuse existing helpers (newTestRuntimeWithManager, loadRuntimeExtension) and existing assertion helpers (require.NoError, assert.Equal, assert.JSONEq) so adding new cases only requires adding entries to the table.
🤖 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 270-272: When a tool error is detected (event.Error != ""),
dispatch the LifecycleToolError through the lifecycle dispatcher in addition to
(or instead of) the runtime event bus so extension handlers (lc.on("tool_error",
...)) are called; call the runtime lifecycle dispatcher method (e.g.,
runtime.lifecycle.Emit or runtime.dispatch) with the same ctx,
extension.LifecycleToolError and toolEventPayload(event) instead of relying only
on runtime.emit(ctx, ...). Ensure you use extension.LifecycleToolError and
toolEventPayload(event) as the type and payload so existing handlers pick it up.
---
Nitpick comments:
In `@internal/assistant/tool_lifecycle_test.go`:
- Around line 13-75: Convert the two tests into table-driven subtests: for
TestRuntime_ToolCallLifecycleAppliesArgumentMutation, create a slice of cases
each with a name, Lua extension string, initial call (assistant.ToolCallEvent)
and expected mutated fields, then loop cases with t.Run(case.name, func(t
*testing.T){ t.Parallel(); setup runtime/manager via newTestRuntimeWithManager,
loadRuntimeExtension(manager, case.lua), call
runtime.DispatchToolCallLifecycleForTest(...), require.NoError(t, err) and
assert expected Argument values and ArgumentsJSON using assert.Equal /
assert.JSONEq }); do the analogous table-driven refactor for
TestRuntime_ToolResultLifecycleAppliesResultMutation using cases that include
initial assistant.ToolEvent and expected Result, DetailsJSON and Error, invoking
runtime.DispatchToolResultLifecycleForTest and asserting expectations; reuse
existing helpers (newTestRuntimeWithManager, loadRuntimeExtension) and existing
assertion helpers (require.NoError, assert.Equal, assert.JSONEq) so adding new
cases only requires adding entries to the table.
🪄 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: f7d815bc-c558-45cd-a235-63ee1d3610ac
📒 Files selected for processing (13)
internal/assistant/client.gointernal/assistant/export_test.gointernal/assistant/lifecycle.gointernal/assistant/runtime.gointernal/assistant/runtime_events.gointernal/assistant/runtime_events_test.gointernal/assistant/runtime_lifecycle_test.gointernal/assistant/runtime_test.gointernal/assistant/tool_lifecycle_test.gointernal/assistant/tool_loop.gointernal/assistant/tool_loop_test.gointernal/extension/lifecycle.gointernal/extension/lifecycle_test.go
💤 Files with no reviewable changes (1)
- internal/assistant/runtime_events.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.
No description provided.