feat(tool): unify extension registry#43
Merged
Merged
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR extends the tool system to support extension-provided tools with custom JSON schemas and makes tool definition selection request-aware. It adds a per-request ChangesExtension Tool Registration & Request-Aware Schemas
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 #43 +/- ##
==========================================
+ Coverage 60.16% 60.35% +0.18%
==========================================
Files 168 170 +2
Lines 16665 16746 +81
==========================================
+ Hits 10027 10107 +80
+ Misses 5628 5627 -1
- Partials 1010 1012 +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: 3
🧹 Nitpick comments (1)
internal/extension/manager_test.go (1)
567-571: ⚡ Quick winAdd a regression case for non-empty
input_schemaregistration.This assertion only covers the default empty-map path. Please add a test case where
lc.register_toolpasses a schema table and assert it is preserved inextension.Tool.InputSchema.As per coding guidelines, "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/extension/manager_test.go` around lines 567 - 571, Add a regression test that exercises lc.register_tool with a non-empty schema and asserts the registered tool preserves it: create a second subtest (or table entry) that calls lc.register_tool passing a non-empty map (e.g., {"foo":"string"}), then inspect the resulting tools slice (same variable used in the file, e.g., tools[0] or the returned tool) and assert extension.Tool.InputSchema equals the original map; update the test around the existing InputSchema empty-map assertion to include this new case so the registration path that sets extension.Tool.InputSchema is covered.
🤖 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/tool_loop.go`:
- Around line 32-42: The nil-registry fallback in executeToolCalls currently
calls tool.NewRegistry("") which loses the request working-directory; change
executeToolCalls signature to accept a cwd string (e.g., add cwd string
parameter) and when registry == nil construct registry = tool.NewRegistry(cwd)
so relative-tool execution preserves the request CWD; update every call site
that invokes executeToolCalls to supply the request.CWD value (or an empty
string if not present) so callers propagate the correct working directory into
tool.NewRegistry.
In `@internal/assistant/tool_registry.go`:
- Around line 17-21: newToolRegistry currently only checks provider == nil but
can receive a typed-nil (non-nil interface) which causes provider.Tools() to
panic; before calling provider.Tools() or registry.RegisterExtensions(provider,
...), detect and treat a typed-nil as nil (for example by using
reflect.ValueOf(provider).IsValid() and reflect.ValueOf(provider).IsNil()) and
return the empty registry like the provider == nil branch; ensure the guard is
placed just before the provider.Tools() call so registry.RegisterExtensions and
provider.Tools() are never invoked on a typed-nil provider.
In `@internal/tool/extension.go`:
- Around line 42-46: The Execute method of ExtensionExecutor returns errors from
executor.runner.ExecuteTool directly; import "github.com/samber/oops" into
internal/tool/extension.go and wrap the returned error using oops with domain
"tool" and code "execute_extension_tool" (include the extension tool name, e.g.
executor.definition.Name) before returning; keep the existing emptyToolResult()
return on error but replace err with the wrapped oops error so callers receive
oops-wrapped context for Execute, referencing ExtensionExecutor.Execute,
executor.runner.ExecuteTool and emptyToolResult().
---
Nitpick comments:
In `@internal/extension/manager_test.go`:
- Around line 567-571: Add a regression test that exercises lc.register_tool
with a non-empty schema and asserts the registered tool preserves it: create a
second subtest (or table entry) that calls lc.register_tool passing a non-empty
map (e.g., {"foo":"string"}), then inspect the resulting tools slice (same
variable used in the file, e.g., tools[0] or the returned tool) and assert
extension.Tool.InputSchema equals the original map; update the test around the
existing InputSchema empty-map assertion to include this new case so the
registration path that sets extension.Tool.InputSchema is covered.
🪄 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: 2808fd6b-2296-4914-92bc-b5080647f58a
📒 Files selected for processing (25)
internal/assistant/anthropic.gointernal/assistant/anthropic_internal_test.gointernal/assistant/client.gointernal/assistant/openai_chat.gointernal/assistant/openai_responses.gointernal/assistant/runtime.gointernal/assistant/tool_loop.gointernal/assistant/tool_loop_test.gointernal/assistant/tool_registry.gointernal/assistant/tool_registry_test.gointernal/assistant/tool_schema.gointernal/extension/manager.gointernal/extension/manager_test.gointernal/extension/types.gointernal/tool/bash.gointernal/tool/edit.gointernal/tool/extension.gointernal/tool/find.gointernal/tool/grep.gointernal/tool/ls.gointernal/tool/read.gointernal/tool/registry.gointernal/tool/registry_test.gointernal/tool/types.gointernal/tool/write.go
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/tool/extension_test.go (1)
57-64: ⚡ Quick winAssert
result.Detailsis nil in the error path.The failing branch returns early after text/error checks, so detail-clearing behavior is not currently verified.
Diff suggestion
if testCase.expectedErr != "" { require.Error(t, err) assert.Contains(t, err.Error(), testCase.expectedErr) oopsErr, ok := oops.AsOops(err) require.True(t, ok) assert.Equal(t, testCase.expectedCode, oopsErr.Code()) assert.Empty(t, result.Text()) + assert.Nil(t, result.Details) return }🤖 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/tool/extension_test.go` around lines 57 - 64, The test's error path currently verifies error text/code and that result.Text() is empty but doesn't assert that result.Details is cleared; update the error branch in the test where testCase.expectedErr != "" (the block checking require.Error, assert.Contains, oops.AsOops, assert.Equal, assert.Empty) to also assert that result.Details() is nil (or empty) before the early return so the detail-clearing behavior is verified.internal/assistant/tool_schema_internal_test.go (1)
59-64: ⚡ Quick winStrengthen clone verification with a nested-mutation assertion.
Current checks only prove top-level map cloning; nested
propertiesaliasing could still regress undetected.Diff suggestion
assertion: func(t *testing.T, schema map[string]any) { t.Helper() assert.Equal(t, customSchema, schema) schema["mutated"] = true assert.NotContains(t, customSchema, "mutated") + + schemaProps, ok := schema[jsonPropertiesKey].(map[string]any) + require.True(t, ok) + schemaProps["message"] = map[string]any{jsonTypeKey: "number"} + + origProps, ok := customSchema[jsonPropertiesKey].(map[string]any) + require.True(t, ok) + assert.Equal(t, map[string]any{jsonTypeKey: "string"}, origProps["message"]) },🤖 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_schema_internal_test.go` around lines 59 - 64, The test currently mutates only the top-level map to prove cloning; add a nested-mutation assertion to ensure deep cloning of the "properties" map: inside the assertion func (the anonymous func using t.Helper(), customSchema and schema) assert that schema["properties"] is a map[string]any (or map[string]interface{}) then mutate a field within that nested map (e.g., set nested["foo"] = "changed") and assert that the same nested key in customSchema["properties"] was not changed; this ensures functions that produce the schema perform a deep copy rather than only a shallow/top-level clone.
🤖 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/tool_schema_internal_test.go`:
- Around line 59-64: The test currently mutates only the top-level map to prove
cloning; add a nested-mutation assertion to ensure deep cloning of the
"properties" map: inside the assertion func (the anonymous func using
t.Helper(), customSchema and schema) assert that schema["properties"] is a
map[string]any (or map[string]interface{}) then mutate a field within that
nested map (e.g., set nested["foo"] = "changed") and assert that the same nested
key in customSchema["properties"] was not changed; this ensures functions that
produce the schema perform a deep copy rather than only a shallow/top-level
clone.
In `@internal/tool/extension_test.go`:
- Around line 57-64: The test's error path currently verifies error text/code
and that result.Text() is empty but doesn't assert that result.Details is
cleared; update the error branch in the test where testCase.expectedErr != ""
(the block checking require.Error, assert.Contains, oops.AsOops, assert.Equal,
assert.Empty) to also assert that result.Details() is nil (or empty) before the
early return so the detail-clearing behavior is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e9ceabc-f7b3-4b7f-9e00-e7be33a5e1a1
📒 Files selected for processing (12)
internal/assistant/anthropic.gointernal/assistant/openai_chat.gointernal/assistant/openai_responses.gointernal/assistant/tool_loop.gointernal/assistant/tool_loop_test.gointernal/assistant/tool_registry.gointernal/assistant/tool_registry_internal_test.gointernal/assistant/tool_schema_internal_test.gointernal/extension/manager_test.gointernal/tool/extension.gointernal/tool/extension_test.gointernal/tool/registry_test.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.