feat: add tool search tool - #52
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughツール検索設定、検索カタログ、BM25・正規表現・ファジー検索、MCPサーバ統合、 Changesツール検索機能
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPServer
participant Catalog
participant SearchMethod
Client->>MCPServer: tool_search(query, method, limit)
MCPServer->>Catalog: Search(server, query, method, limit)
Catalog->>SearchMethod: BM25/Regexp/Fuzzy検索
SearchMethod-->>Catalog: ToolDef一覧
Catalog-->>MCPServer: 検索結果
MCPServer-->>Client: 整形済み結果
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/internal/mcpsrv/tool_search_test.go (2)
36-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
connectInMemoryでサーバー側セッションを破棄している点について。
srv.Connect(...)の戻り値（サーバー側*mcp.ServerSession）を_で捨てており、t.Cleanupでもクローズしていません。go-sdkの公式サンプルではdefer serverSession.Close()しているパターンが一般的です。クライアント側クローズで連動して終了する可能性は高いですが、明示的にクローズした方が一貫性があります。🤖 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/internal/mcpsrv/tool_search_test.go` around lines 36 - 47, Update connectInMemory to retain the server-side session returned by srv.Connect and register it for cleanup alongside the client session. Explicitly close the server session through t.Cleanup, while preserving the existing error checks and client connection behavior.
505-509: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
limit_truncatesケースで返却ツール名の中身を検証していません。
wantNames: []string{"list_pets"}を定義していますが、実際のアサーションはrequire.Len(t, gotNames, tt.limit)のみで件数しか検証しておらず、wantNamesの値は使われていません。返却順序が変わって別ツールが1件返っても検知できません。♻️ 修正例
if tt.name == "limit_truncates" { - require.Len(t, gotNames, tt.limit) + require.Equal(t, tt.wantNames, gotNames) } else { require.ElementsMatch(t, tt.wantNames, gotNames) }🤖 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/internal/mcpsrv/tool_search_test.go` around lines 505 - 509, Update the limit_truncates branch in the test assertions to validate gotNames against tt.wantNames, not just its length. Preserve the existing ElementsMatch behavior so the test verifies the returned tool names and detects an incorrect single-item result.pkg/config/config_test.go (1)
89-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThreshold単体の検証になっていない
ToolSearchConfig{Threshold: -1}はDigestMaxToolsが未指定のためゼロ値(0)のままとなり、これ自体がvalidateDigestMaxToolsにより単独でエラーになります。そのため本テストは実質的に「ThresholdとDigestMaxToolsの複合エラー」を検証しており、テスト名が意図する「Thresholdが負の場合のエラー」を単体で検証できていません。toolSearch_test.goの"negative threshold invalid"ケースのようにDigestMaxTools: -1を明示すると意図が明確になります。💡 修正案
func TestConfig_ValidateWithContext_ToolSearch_NegativeThreshold_Invalid(t *testing.T) { cfg := newValidConfigWithServers(Servers{}) - cfg.Gateway.ToolSearch = ToolSearchConfig{Threshold: -1} + cfg.Gateway.ToolSearch = ToolSearchConfig{Threshold: -1, DefaultLimit: 10, DigestMaxTools: -1} err := cfg.ValidateWithContext(t.Context()) require.Error(t, 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 `@pkg/config/config_test.go` around lines 89 - 94, Update TestConfig_ValidateWithContext_ToolSearch_NegativeThreshold_Invalid to set DigestMaxTools to a valid value while keeping Threshold negative, so the test isolates Threshold validation rather than also triggering validateDigestMaxTools.pkg/internal/mcpsrv/tool_search.go (1)
194-201: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLLM 向けレスポンスに
MarshalIndentは不要なオーバーヘッド。
tool_searchの結果は LLM がそのまま消費するペイロードのため、インデント付き JSON は可読性向上より無駄なトークン/バイトコストの方が大きいと考えられます。json.Marshalへ変更してコンパクトにすることを検討してください。♻️ 提案
- data, err := json.MarshalIndent(formatted, "", " ") + data, err := json.Marshal(formatted)🤖 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/internal/mcpsrv/tool_search.go` around lines 194 - 201, Update the JSON serialization in the tool_search result flow to use compact encoding via json.Marshal instead of json.MarshalIndent, while preserving the existing error handling and assignments to result.Content and result.StructuredContent.
🤖 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/internal/mcpsrv/server_manager.go`:
- Around line 134-141: OpenAPI tool registration in the server manager must skip
the reserved "tool_search" name before calling AddTool, matching the backend
filtering behavior. Update the loop over register.ListTools() around catalog.Add
and srv.AddTool to continue without registering that tool, so the later
registerToolSearch implementation remains authoritative.
In `@pkg/internal/toolsearch/regexp.go`:
- Around line 8-13: Update searchRegexp to return nil immediately when query is
empty, before compiling the regular expression, so empty searches produce no
results consistently with BM25 and fuzzy search.
---
Nitpick comments:
In `@pkg/config/config_test.go`:
- Around line 89-94: Update
TestConfig_ValidateWithContext_ToolSearch_NegativeThreshold_Invalid to set
DigestMaxTools to a valid value while keeping Threshold negative, so the test
isolates Threshold validation rather than also triggering
validateDigestMaxTools.
In `@pkg/internal/mcpsrv/tool_search_test.go`:
- Around line 36-47: Update connectInMemory to retain the server-side session
returned by srv.Connect and register it for cleanup alongside the client
session. Explicitly close the server session through t.Cleanup, while preserving
the existing error checks and client connection behavior.
- Around line 505-509: Update the limit_truncates branch in the test assertions
to validate gotNames against tt.wantNames, not just its length. Preserve the
existing ElementsMatch behavior so the test verifies the returned tool names and
detects an incorrect single-item result.
In `@pkg/internal/mcpsrv/tool_search.go`:
- Around line 194-201: Update the JSON serialization in the tool_search result
flow to use compact encoding via json.Marshal instead of json.MarshalIndent,
while preserving the existing error handling and assignments to result.Content
and result.StructuredContent.
🪄 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 Plus
Run ID: e819a4a4-a3a2-47e8-8dba-1c7fdbee9b80
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
go.modpkg/cmd/server.gopkg/config/config.gopkg/config/config_test.gopkg/config/load.gopkg/config/load_test.gopkg/config/toolSearch.gopkg/config/toolSearch_test.gopkg/internal/mcpsrv/mcp_backend_client.gopkg/internal/mcpsrv/mcp_backend_client_test.gopkg/internal/mcpsrv/server_manager.gopkg/internal/mcpsrv/server_manager_test.gopkg/internal/mcpsrv/tool_search.gopkg/internal/mcpsrv/tool_search_test.gopkg/internal/toolsearch/bm25.gopkg/internal/toolsearch/bm25_test.gopkg/internal/toolsearch/catalog.gopkg/internal/toolsearch/catalog_test.gopkg/internal/toolsearch/format.gopkg/internal/toolsearch/format_test.gopkg/internal/toolsearch/fuzzy.gopkg/internal/toolsearch/fuzzy_test.gopkg/internal/toolsearch/regexp.gopkg/internal/toolsearch/regexp_test.gopkg/internal/toolsearch/schema.gopkg/internal/toolsearch/schema_test.gopkg/internal/toolsearch/tokenizer.gopkg/internal/toolsearch/tokenizer_test.gopkg/internal/toolsearch/types.go
nonchan7720
left a comment
nonchan7720
left a comment
There was a problem hiding this comment.
Automated review of the tool_search feature. Found one real correctness bug (OpenAPI-mode tools can silently collide with the synthetic tool_search name) plus two smaller robustness issues and one non-blocking perf nit — see inline comments.
Generated by Claude Code
|
先ほどの自動レビュー概要の日本語版です。
Generated by Claude Code |
…he synthetic tool registerAPI had no guard against an upstream OpenAPI operation named tool_search, unlike the backend-mode registration path. Without it, such a tool would be silently overwritten by registerToolSearch's later AddTool call, making the real operation unreachable while the catalog kept advertising it under a misleading tool_search entry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rZMP1AYvf7ETunqurpG1m
…che BM25 preprocessing
- schema.go: walkSchemaForSearchTexts now also handles tuple-form JSON
Schema items ("items": [{...}, {...}]), which was previously silently
dropped from the search index.
- catalog.go: Catalog.Search normalizes method case before matching, so
"BM25"/"Regexp"/"Fuzzy" no longer error out. Centralizing this in
Catalog.Search also fixes it for every current and future caller.
- catalog.go/bm25.go: Catalog now caches the tokenized bm25Doc set per
server and invalidates it in Add, instead of re-tokenizing the whole
catalog and recomputing IDF/avgdl on every tool_search call.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rZMP1AYvf7ETunqurpG1m
An empty pattern compiles to "(?i)" which matches every string, so searchRegexp was returning the entire catalog for an empty query while bm25/fuzzy both return no results. Short-circuit to nil for consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rZMP1AYvf7ETunqurpG1m
nonchan7720
left a comment
nonchan7720
left a comment
There was a problem hiding this comment.
先ほどの修正コミット（f439644, 3c15df1, 79608c6）に対する、複数の観点（アーキテクチャ／簡潔化／再利用／クロスファイル）からの追加レビューです。
このPRの差分に含まれる範囲では3件見つかりました（詳細はインラインコメント参照）。加えて、このPRの差分外（既存コード）に1件、実害のある競合状態を見つけたため、別途PRコメントで報告します。
その他、より軽微な指摘（config.ToolSearchResultFormat* と toolsearch.ResultFormat* の重複、DigestMaxTools のゼロ値特別扱い、MCPServerOption が実質1オプションしかない点、ToolDef が mcp.Tool と重複した形状を持つ点、registerToolSearch の登録時descriptionが実際には使われず無駄になっている点など）もありましたが、いずれも軽微なスタイル・保守性の提案でブロッカーではないため、今回はインラインコメントを省略しています。
なお、hideToolsMiddleware が catalog.Total()（全 mcpServers 合計）でサーバーごとの表示切り替えを判定している点も一部のfinderから「バグではないか」との指摘がありましたが、toolSearchName/Threshold のドキュメントコメント（tool_search.go:16-18, pkg/config/toolSearch.go:34-40）を確認したところ、「全 mcpServers 合計のツール数が閾値を超えたら」という挙動は明示的に意図された設計でした。誤検知と判断し、指摘からは除外しています。
Generated by Claude Code
|
（このPRの差分外・既存コードの指摘）
func (c *MCPBackendClient) EnsureConnected(ctx context.Context) (rErr error) {
c.mu.Lock()
if c.connected {
c.mu.Unlock()
return nil
}
c.mu.Unlock()
session, err := c.connect(ctx)
...
if err := c.registerTools(ctx, session); err != nil { ... }
c.mu.Lock()
c.session = session
c.connected = true
c.mu.Unlock()
return nil
}check-then-act になっており、 このPRで Generated by Claude Code |
…ten regexp/cache - mcp_backend_client.go: EnsureConnected now holds c.mu for the whole connect+registerTools+state-update sequence instead of releasing it in between, so concurrent first requests to the same backend can no longer both connect and double-register tools, leaking the losing session. Retry-after-failure behavior is preserved since it's still a plain Mutex, not sync.Once. - tool_search.go: extract isReservedToolName as the single place that defines what collides with the synthetic tool_search tool, and use it from both registerAPI (OpenAPI mode) and registerTools (backend mode) instead of duplicating the same == check in both files. - regexp.go: searchRegexp now skips (returns no results) whenever Tokenize(query) yields no tokens, not just for the exact empty string, so symbol-only queries like "." no longer match the entire catalog while bm25/fuzzy return nothing for the same input. - catalog.go: replace the hand-rolled RWMutex double-checked-locking bm25 cache with sync.Map.LoadOrStore, matching the GetOrCreate pattern already used elsewhere in the codebase (pkg/internal/client.InMemoryRegistry). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rZMP1AYvf7ETunqurpG1m
|
先ほど報告した
Generated by Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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)
pkg/config/config.go (1)
49-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Memory.Enabledをストレージ必須判定に反映してください。
Memoryが非nilでもEnabled == falseなら、インメモリストレージは選択されません。現在の条件はMemoryの存在だけで Redis と SQLite の必須検証を無効にするため、有効なストレージバックエンドがない設定を通過させます。c.Memory == nil || !c.Memory.Enabledを条件に含め、Enabled == falseを未選択として扱ってください。MemoryConfig{Enabled: false}と Redis/SQLite 未設定の回帰テストも追加してください。修正例
- validation.Field(&c.Redis, validation.When(c.SQLite == nil && c.Memory == nil, validation.Required)), - validation.Field(&c.SQLite, validation.When(c.Redis == nil && c.Memory == nil, validation.Required)), + validation.Field(&c.Redis, validation.When(c.SQLite == nil && (c.Memory == nil || !c.Memory.Enabled), validation.Required)), + validation.Field(&c.SQLite, validation.When(c.Redis == nil && (c.Memory == nil || !c.Memory.Enabled), validation.Required)),🤖 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/config/config.go` around lines 49 - 50, Update the storage validation conditions in the Redis and SQLite validation fields to treat MemoryConfig as selected only when c.Memory is non-nil and Enabled is true; include c.Memory == nil || !c.Memory.Enabled in both conditions. Add a regression test covering MemoryConfig{Enabled: false} with Redis and SQLite unset, ensuring validation rejects the configuration.
🤖 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 `@README.md`:
- Around line 211-212: Update the README documentation around the tool_search
usage description and the resultFormat table to limit direct tools/call usage
and complete tool-definition claims to resultFormat: default. For resultFormat:
claude, explicitly state that the returned tool_reference blocks must be
expanded by the Claude API before invocation, and clarify the required calling
conditions without changing the format definitions.
---
Outside diff comments:
In `@pkg/config/config.go`:
- Around line 49-50: Update the storage validation conditions in the Redis and
SQLite validation fields to treat MemoryConfig as selected only when c.Memory is
non-nil and Enabled is true; include c.Memory == nil || !c.Memory.Enabled in
both conditions. Add a regression test covering MemoryConfig{Enabled: false}
with Redis and SQLite unset, ensuring validation rejects the configuration.
🪄 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 Plus
Run ID: e5938c33-23cf-4fa7-95d0-7df9024e09fe
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
README.mdgo.modpkg/cmd/server.gopkg/config/config.gopkg/config/config_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/cmd/server.go
- go.mod
- pkg/config/config_test.go
| | `default` | （デフォルト）従来どおり、一致したツールの完全な定義（`name` / `description` / `inputSchema`）の配列を返す | | ||
| | `claude` | [Claude API の Tool Search Tool のカスタム検索実装規約](https://platform.claude.com/docs/en/agents-and-tools/tool-use/tool-search-tool#custom-tool-search-implementation) に準拠した `tool_reference` ブロック（`{"type": "tool_reference", "tool_name": "..."}`）の配列を返す。Claude API 側がこのブロックを完全なツール定義に自動展開する | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
resultFormat: claude の呼び出し条件を明記してください。
README.md Line 190 は、tool_search が完全なツール定義を返し、そのまま tools/call に使えると説明します。一方、Line 211-212 の claude 形式は tool_reference を返します。完全な定義と直接呼び出しの説明は resultFormat: default の場合に限定してください。claude 形式では、参照を展開してから呼び出す必要があることを明記してください。
🤖 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 `@README.md` around lines 211 - 212, Update the README documentation around the
tool_search usage description and the resultFormat table to limit direct
tools/call usage and complete tool-definition claims to resultFormat: default.
For resultFormat: claude, explicitly state that the returned tool_reference
blocks must be expanded by the Claude API before invocation, and clarify the
required calling conditions without changing the format definitions.
2 participants
Summary by CodeRabbit
tool_searchによる検索を提供（BM25・正規表現・ファジー、標準／Claude向け結果形式に対応）。tools/listの表示を最適化し、非表示のツールも検索・実行可能。tool_searchを誤って上書きしないよう改善。