feat(pkg/kit): expose runtime add/remove of native Go tools (#74)#75
Conversation
- Add Kit.AddTools, RemoveTools, SetExtraTools, and GetExtraTools - Make native extra tools mutable at runtime under promptOptsMu - Convert promptOptsMu to sync.RWMutex for concurrent reads - Update ReloadExtensions to hold the lock when swapping extension tools - Add unit tests for add/remove/replace, atomic removal, and concurrency - Document the new API in README.md Fixes #74
|
Warning Review limit reached
More reviews will be available in 38 minutes and 14 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdded live native-tool mutation APIs to ChangesRuntime Native Tool Mutation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Connected to Huly®: KIT-76 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/kit/tools_test.go (1)
401-405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the remaining tool names, not just the count.
A bug that leaves some even-numbered tools and drops the same number of odd-numbered tools would still pass this test.
Proposed assertion
// Even-numbered tools were removed; odd ones remain. - got := host.GetToolNames() - if len(got) != n/2 { - t.Errorf("expected %d tools after concurrent mutation, got %d: %v", n/2, len(got), got) + got := sortedStrings(host.GetToolNames()) + want := make([]string, 0, n/2) + for i := 1; i < n; i += 2 { + want = append(want, fmt.Sprintf("tool-%d", i)) + } + if !slices.Equal(got, want) { + t.Errorf("expected odd-numbered tools after concurrent mutation, got %v", got) }🤖 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 `@pkg/kit/tools_test.go` around lines 401 - 405, The concurrent mutation test in GetToolNames only checks the total count, so it can miss cases where even-numbered tools remain and odd-numbered tools are incorrectly removed. Update the assertion around host.GetToolNames to verify the exact remaining tool names, not just len(got), by comparing against the expected odd-numbered names after the removals.
🤖 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 `@pkg/kit/kit.go`:
- Around line 894-897: The extension reload path is overwriting all extra tools,
so runtime-added native tools are being dropped when ReloadExtensions calls
agent.SetExtraTools. Update the reload flow around m.agent.SetExtraTools in
kit.go to keep extension tools separate from tools added via
AddTools/SetExtraTools, then compose both sets while holding promptOptsMu before
setting them back on the agent. Use the existing m.extRunner.RegisteredTools()
and Agent.SetExtraTools behavior to locate the affected code and preserve the
full combined tool list.
- Around line 262-265: SetExtraTools is passing the caller’s slice through
directly, so later mutations to the original tools slice can փոփոխe the live
tool set outside promptOptsMu. Update Kit.SetExtraTools to snapshot the variadic
tools input before calling m.agent.SetExtraTools, using the existing
SetExtraTools path on Kit/agent so the stored tool list is detached from the
caller’s backing array.
- Around line 180-199: In AddTools in kit.go, duplicate tool names passed in the
same call are not using last-write-wins when cur is empty because the second
loop appends the original tool instead of the final replacement. Update the
merge logic so every name in the tools slice resolves to the last Tool with that
Info().Name, using the existing replacements and seen maps, and ensure the
append path for new names also prefers the replacement entry rather than the
earlier duplicate.
- Around line 217-221: The missing-tool error path in kit.go is inconsistent
when cur is empty because it returns names in caller order without deduping or
sorting. Update the early-return branch in the tool lookup logic to apply the
same normalization used elsewhere in the missing-name handling, so the error
from the relevant function in pkg/kit/kit.go always reports a stable, sorted
list of unique tool names.
In `@pkg/kit/tools_test.go`:
- Around line 291-297: The tests using kit.New are not fully hermetic because
the default tool catalog can still be affected by local config, discovered
skills, and context files. Update the Options passed in the relevant kit.New
calls in tools_test.go to also set SkipConfig, NoSkills, and NoContextFiles
alongside Quiet, NoSession, NoExtensions, and DisableCoreTools so the initial
tool catalog stays empty regardless of local environment or KIT_* settings.
In `@README.md`:
- Around line 931-933: The README example for host.RemoveTools ignores the
returned error even though the surrounding text says missing names should
produce one, so update the snippet to handle or surface that error instead of
discarding it. Keep the example centered on host.RemoveTools and make it safe to
copy by showing the returned error being checked, logged, or otherwise
acknowledged in the sample.
---
Nitpick comments:
In `@pkg/kit/tools_test.go`:
- Around line 401-405: The concurrent mutation test in GetToolNames only checks
the total count, so it can miss cases where even-numbered tools remain and
odd-numbered tools are incorrectly removed. Update the assertion around
host.GetToolNames to verify the exact remaining tool names, not just len(got),
by comparing against the expected odd-numbered names after the removals.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b88132a1-a8c0-4a5f-b647-36b97f6f2ed5
📒 Files selected for processing (3)
README.mdpkg/kit/kit.gopkg/kit/tools_test.go
Address CodeRabbit review on #75: - Track runtime native tools separately from extension tools so ReloadExtensions no longer drops AddTools/SetExtraTools entries; recomposeExtraTools rebuilds extension + runtime sets together - Snapshot the variadic slice in SetExtraTools to detach from the caller's backing array - Make AddTools last-write-wins for duplicate names within one call - Normalize (dedupe + sort) the RemoveTools missing-name error on the empty-set path - Handle the RemoveTools error in the README example - Make tool tests hermetic (SkipConfig/NoSkills/NoContextFiles) and assert exact remaining names in the concurrency test
- Add a Runtime native tools section to the SDK overview covering AddTools, RemoveTools, SetExtraTools, and GetExtraTools semantics - Cross-link the custom-tools and ExtraTools option to the new section
1 participant
Description
This PR exposes session-level mutation of the native Go tool set on a live
*Kit, closing the gap between native tools and the already-runtime-mutable MCP / skill subsystems.Before this change, embedders could only add native tools at construction time (
Options.Tools/Options.ExtraTools) or for a single call viaPromptOptions.ExtraTools, which is reverted immediately after the call. Rebuilding the host to swap toolsets loses session state, MCP connections, and the composed system prompt.After this change, native tools can be added, removed, replaced, and read back at runtime:
AddToolsis additive with last-write-wins replacement by tool name.RemoveToolsis atomic and returns an error listing any missing names without changing the tool set. Core tools and MCP tools are unaffected. The new methods share the existingpromptOptsMu(now anRWMutex) so concurrent reads are safe and writes are serialized.Fixes #74
Type of Change
Checklist
go test -race ./pkg/kit/...)Additional Information
Files changed
pkg/kit/kit.go— addsAddTools,RemoveTools,SetExtraTools, andGetExtraTools; convertspromptOptsMutosync.RWMutex; locksReloadExtensionstool swap.pkg/kit/tools_test.go— adds regression and concurrency tests.README.md— documents the new runtime native-tool API.Backward compatibility
Purely additive. No existing signatures or behavior are changed.
Summary by CodeRabbit