feat: 支持 SearchExtraTools 能力以替代 Tool Search#442
Merged
claude-code-best merged 9 commits intomainfrom May 9, 2026
Merged
Conversation
… 搜索增强） - 新增 CORE_TOOLS 白名单常量（31 个核心工具），重构 isDeferredTool 为白名单制判定 - 新建 TF-IDF 工具索引模块（toolIndex.ts），复用 localSearch.ts 算法函数 - 新建 ExecuteTool 跨 API provider 统一工具执行入口 - 增强 ToolSearchTool：TF-IDF 搜索路径、discover: 模式、并行搜索合并、文本模式回退 - 新增 27 个单元测试，precheck 零错误通过（4108 tests pass） Co-Authored-By: glm-5.1[1m] <[email protected]>
…ridge 启用时可用 - PushNotificationTool、SendUserFileTool 添加 isEnabled() 使用 isBridgeEnabled() - BriefTool 的 isEnabled() 从 isBriefEnabled() 改为 isBridgeEnabled() - ExecuteTool 添加 isEnabled() 兜底检查，不可用时返回友好错误 - useReplBridge bridge 首次连接时插入 system 消息通知模型新工具可用 - 移除 toolSearch 中 firstParty base URL 白名单检测，默认启用 tool search Co-Authored-By: glm-5.1[1m] <[email protected]>
…vider 通用 - 重命名 ExecuteTool → ExecuteExtraTool，作为一等工具始终可用 - ToolSearchTool 输出改为纯文本（区分 core/deferred），移除 tool_reference blocks - 移除 modelSupportsToolReference() 及相关 GrowthBook 配置 - 移除 API 侧 defer_loading 字段和 tool search beta header 注入 - 简化 system prompt（工具使用指南从 ~120 行压缩到 ~10 行） - extractDiscoveredToolNames 支持文本格式解析（向后兼容旧 session 的 tool_reference） - 更新 promptEngineeringAudit 测试以匹配简化后的 prompt 结构 Co-Authored-By: Claude Opus 4.7 <[email protected]>
移除 deferred 工具的 "discover then include" 逻辑，让 tools 数组在整个会话中 保持稳定（只有 core tools + ToolSearch + ExecuteExtraTool），避免每次发现新 工具时 tools JSON 变化导致 prompt cache 失效。 同时强化工具优先级引导：core tools 优先直接调用，ToolSearch/ExecuteExtraTool 仅作为发现和调用 deferred 工具的最后手段。当模型搜索已加载的 core tool 时， ToolSearch 返回明确的拒绝提示。 Co-Authored-By: glm-5.1[1m] <[email protected]>
将 TeamCreate、TeamDelete、SendMessage 从 CORE_TOOLS 移除， 使其成为 deferred 工具，通过 ToolSearch 按需发现以减少 context token。 swarm 模式下 SendMessage 保持 always loaded，TeamCreate/TeamDelete 在 swarm 未启用时调用返回启用提示。 Co-Authored-By: Claude Opus 4.7 <[email protected]>
- 重命名 toolSearch/ → searchExtraTools/，ToolSearchTool → SearchExtraToolsTool - 强化 ExecuteExtraTool prompt：明确强调必须通过 ExecuteExtraTool 调用 deferred tools - 优化 system prompt 工具优先级引导：core tools 直接调用，deferred tools 必须用 ExecuteExtraTool - available-deferred-tools 标签追加 ExecuteExtraTool 使用说明 - tool_discovery attachment 消息强化 ExecuteExtraTool 调用指引 - 禁用 turn-zero 用户输入工具推荐（频繁弹窗） Co-Authored-By: glm-5-turbo <[email protected]>
- 新增 docs/design/tool-search-design-guide.md，涵盖架构、搜索算法、执行管道、演进历史 - 禁用 getTurnZeroSearchExtraToolsPrefetch，消除用户输入时的频繁弹窗 - inter-turn 发现机制保持不变 Co-Authored-By: glm-5-turbo <[email protected]>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
📝 WalkthroughWalkthroughMigrates ToolSearch to SearchExtraTools, adds ExecuteTool, introduces TF‑IDF indexing and prefetch, updates API filtering/messages, prompts, UI hinting, constants/flags, and comprehensive tests/specs. ChangesSearchExtraTools Migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
getTurnZeroSearchExtraToolsPrefetch 在 commit 2cf18c4 中被禁用（始终返回 null）， 但测试仍期望匹配时返回非 null attachment。更新三个用例全部期望 null。
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/hooks/useReplBridge.tsx (1)
305-325:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis notification can be re-appended on reconnects.
Because reconnect flow sets
replBridgeSessionActiveback to false, this check can fire multiple times and keep adding the same “new tools available” system message. Consider gating once per bridge session (e.g., byreplBridgeSessionId) and keeping copy consistent with existing transcript language.🤖 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 `@src/hooks/useReplBridge.tsx` around lines 305 - 325, The "new tools available" system message is being re-appended on reconnect because the reconnect flow flips replBridgeSessionActive to false; change the gating to be once-per-session by checking and storing a unique replBridgeSessionId instead of just replBridgeSessionActive: read the current replBridgeSessionId from store (or generate/compare the incoming session id used by setAppState), only call setMessages(createSystemMessage(...)) when the session id is new, and update setAppState to persist replBridgeSessionId alongside replBridgeSessionActive/replBridgeConnected; also ensure the message text uses the same transcript wording as other system messages created by createSystemMessage.src/utils/searchExtraTools.ts (1)
497-516: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace the
as anycasts in transcript parsing.
extractDiscoveredToolNames()now relies on(msg as any)for both compact-boundary metadata anddeferred_tools_deltaattachments. That bypasses strict-mode checks in a path fed by persisted message data, so malformed transcript shapes can silently leak non-string entries intodiscoveredTools. Please narrow these branches with explicit message/attachment shapes instead ofany.As per coding guidelines,
**/*.{ts,tsx}: Production code must not useas anytype assertions; useRecord<string, unknown>for objects with unknown structure.🤖 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 `@src/utils/searchExtraTools.ts` around lines 497 - 516, The code uses unsafe casts (msg as any) in extractDiscoveredToolNames; replace them with explicit runtime-narrowing to Record<string, unknown> and validate shapes before using: treat msg as Record<string, unknown>, check msg.type === 'system' && msg.subtype === 'compact_boundary', then read compactMetadata as Record<string, unknown> | undefined, ensure compactMetadata.preCompactDiscoveredTools is an array via Array.isArray and filter items to typeof === 'string' before adding to discoveredTools and counting; similarly for the attachment branch, treat msg.attachment as Record<string, unknown> | undefined, check attachment.type === 'deferred_tools_delta', ensure attachment.addedNames is an array and push only string entries (or default to []), removing all use of as any and replacing with these guarded reads and filters.src/utils/analyzeContext.ts (1)
448-469:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCount discovered deferred tools as loaded, not just invoked tools.
These branches only flip a deferred tool to
isLoadedafter an assistanttool_use, but the request path starts sending that schema as soon as the conversation has discovered it viaSearchExtraTools/deferred_tools_delta. That means the context analyzer undercounts tool tokens between discovery and first invocation. ReusingextractDiscoveredToolNames(messages)here would keep the UI aligned with the actual tool set being sent to the API.Also applies to: 689-709
🤖 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 `@src/utils/analyzeContext.ts` around lines 448 - 469, The code only marks deferred tools as loaded when seeing an assistant 'tool_use' event, but it should also count tools discovered via deferred_tools_delta/SearchExtraTools; update the logic in analyzeContext (the branch using deferredBuiltinTools, isDeferred and loadedToolNames) to seed loadedToolNames with extractDiscoveredToolNames(messages) (add each discovered name into loadedToolNames) before scanning for 'tool_use' blocks; make the same change in the analogous branch later in the file (the other deferredBuiltinTools/isDeferred block) so discovered-but-not-yet-invoked deferred tools are treated as loaded.
🟡 Minor comments (11)
scripts/dev.ts-21-22 (1)
21-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix contradictory inline comment.
Line 21 says development mode, but Line 22 explicitly sets
NODE_ENVtoproduction. This is easy to misread during local debugging.Suggested patch
- // dev 模式使用 development 模式 + // dev 启动脚本显式使用 production 模式（见上方性能原因说明） 'process.env.NODE_ENV': JSON.stringify('production'),🤖 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 `@scripts/dev.ts` around lines 21 - 22, The inline comment in scripts/dev.ts contradicts the env setting: update the code so the comment and the value for 'process.env.NODE_ENV' agree; either change JSON.stringify('production') to JSON.stringify('development') if this is the dev build, or change the comment to state production if the production value is intended—locate the 'process.env.NODE_ENV' entry and make the comment and value consistent.packages/builtin-tools/src/tools/TeamDeleteTool/TeamDeleteTool.ts-92-96 (1)
92-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message should be in English for consistency.
The error message is written in Chinese, which is inconsistent with the rest of the codebase. All other error messages and user-facing text in this repository use English.
🌐 Proposed fix: translate to English
if (!isAgentSwarmsEnabled()) { throw new Error( - 'Agent Teams 功能未启用。请确保未设置 CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED 环境变量。', + 'Agent Teams feature is not enabled. Ensure the CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED environment variable is not set.', ) }🤖 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 `@packages/builtin-tools/src/tools/TeamDeleteTool/TeamDeleteTool.ts` around lines 92 - 96, The thrown Error in the TeamDeleteTool when isAgentSwarmsEnabled() is false uses a Chinese message; change it to an English, consistent message (e.g., "Agent Teams feature is not enabled. Ensure the CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED environment variable is not set."). Update the Error constructed in the conditional that checks isAgentSwarmsEnabled() inside TeamDeleteTool (the throw new Error(...) block) to use this English text so it matches the rest of the codebase.packages/builtin-tools/src/tools/TeamCreateTool/TeamCreateTool.ts-130-134 (1)
130-134:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winError message should be in English for consistency.
The error message is written in Chinese, which is inconsistent with the rest of the codebase. All other error messages and user-facing text in this repository use English.
🌐 Proposed fix: translate to English
if (!isAgentSwarmsEnabled()) { throw new Error( - 'Agent Teams 功能未启用。请确保未设置 CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED 环境变量。', + 'Agent Teams feature is not enabled. Ensure the CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED environment variable is not set.', ) }🤖 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 `@packages/builtin-tools/src/tools/TeamCreateTool/TeamCreateTool.ts` around lines 130 - 134, The thrown Error message inside the isAgentSwarmsEnabled() check uses Chinese text; update the message to English for consistency by replacing the current string in the throw new Error(...) inside TeamCreateTool (the block guarded by isAgentSwarmsEnabled()) with an English message that mentions the Agent Teams feature and the CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED env var (e.g., "Agent Teams feature is not enabled. Ensure the CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED environment variable is not set.").spec/feature_20260508_F001_tool-search/spec-design.md-201-203 (1)
201-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnify naming/paths with SearchExtraTools migration.
Several sections still point to
toolSearch/ToolSearchTool(for example, Line 201 and Line 242), but this PR is explicitly migrating tosearchExtraTools/SearchExtraToolsTool. These references will confuse follow-up work and manual verification.Also applies to: 242-243, 333-334, 419-421, 430-431
🤖 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 `@spec/feature_20260508_F001_tool-search/spec-design.md` around lines 201 - 203, The spec and new file reference the old naming "toolSearch" and type/name "ToolSearchTool" but this PR migrates to "searchExtraTools" and "SearchExtraToolsTool"; locate usages and imports in the new module (e.g., src/services/toolSearch/toolIndex.ts) and all referenced places and rename module/path and type identifiers from toolSearch -> searchExtraTools and ToolSearchTool -> SearchExtraToolsTool (update import paths, exported names, type annotations and any factory/constructor calls), ensure exported symbols and any index/registry entries match the new names and run/adjust tests or imports that consume these symbols so compilation remains green.spec/feature_20260508_F001_tool-search/spec-design.md-248-260 (1)
248-260:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
These fences are unlabeled, which will keep triggering markdownlint (MD040). Please annotate them (e.g.,
text/bash) for consistency.Proposed doc fix
-``` +```text query 输入 │ ... -``` +``` -``` +```text discover:<任务描述> — 纯发现搜索，不触发延迟加载，只返回工具信息 -``` +``` -``` +```text 用户输入提交 │ ... -``` +```Also applies to: 283-285, 339-354
🤖 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 `@spec/feature_20260508_F001_tool-search/spec-design.md` around lines 248 - 260, The fenced code blocks containing diagrams like the one starting with "query 输入", the block with "discover:<任务描述> — 纯发现搜索，不触发延迟加载，只返回工具信息", and the block starting "用户输入提交" are unlabeled and trigger MD040; update each triple-backtick fence to include a language identifier such as text (e.g., ```text) for every opening fence and leave the closing fence unchanged, applying the same fix to the other unlabeled fences noted (around the sections containing those exact block contents).spec/feature_20260508_F001_tool-search/spec-human-verify.md-36-38 (1)
36-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerification commands still target pre-migration paths.
The checklist still uses
ToolSearchToolandsrc/services/toolSearch/...in multiple steps (e.g., Line 68, Line 136, Line 183). These should be updated toSearchExtraToolsToolandsrc/services/searchExtraTools/...so automated/manual acceptance actually checks the new implementation.Also applies to: 68-80, 136-183, 195-196
🤖 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 `@spec/feature_20260508_F001_tool-search/spec-human-verify.md` around lines 36 - 38, Update the verification checklist so all references to the old tool and paths are replaced with the new ones: change every occurrence of ToolSearchTool to SearchExtraToolsTool and every occurrence of src/services/toolSearch to src/services/searchExtraTools, and update the grep targets/expectations to point at the migrated file and function (e.g., the isDeferredTool implementation in the new SearchExtraToolsTool prompt) so the checks still assert presence of CORE_TOOLS.has and the function returning true.spec/feature_20260508_F001_tool-search/spec-plan-1.md-193-195 (1)
193-195:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlan/checklist still mixes old and new module names.
This plan still references
src/services/toolSearch/*andToolSearchToolin core execution and verification steps (e.g., Line 193, Line 450, Line 628). Since this PR is the SearchExtraTools replacement, these should be normalized tosearchExtraTools/SearchExtraToolsTool(and matching test paths) to keep the plan executable.Also applies to: 450-453, 585-590, 628-629
🤖 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 `@spec/feature_20260508_F001_tool-search/spec-plan-1.md` around lines 193 - 195, The plan/checklist still uses the old module and type names; update every occurrence of the module path pattern "src/services/toolSearch/*" and the type/name "ToolSearchTool" to the new standardized names "searchExtraTools" (module) and "SearchExtraToolsTool" (type), and update any test file paths to match the new module naming (e.g., change toolIndex.test and toolIndex.ts references to the equivalent searchExtraTools filenames); ensure core execution and verification steps, examples, and any import/test references (previously referencing ToolSearchTool or src/services/toolSearch) are consistently replaced with SearchExtraToolsTool and searchExtraTools throughout the plan.docs/design/tool-search-design-guide.md-26-34 (1)
26-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPlease label fenced code blocks with a language.
Multiple code fences are missing language tags, which triggers markdownlint MD040. Adding
text/bashconsistently will keep docs lint-clean.Also applies to: 52-57, 61-79, 94-96, 103-106, 110-122, 132-138, 149-153
🤖 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 `@docs/design/tool-search-design-guide.md` around lines 26 - 34, The fenced ASCII diagram and other code blocks (e.g., the "All Tools (60+ built-in + MCP)" table and blocks showing CORE_TOOLS and isDeferredTool()) are missing language tags and trigger markdownlint MD040; update each fenced code block in docs/design/tool-search-design-guide.md (including the blocks called out around the CORE_TOOLS and isDeferredTool() examples and the other ranges noted) to use an appropriate language label such as ```text or ```bash so markdownlint stops flagging MD040 and the blocks render consistently.packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts-77-90 (1)
77-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a generic, consistent unavailable message for disabled tools.
The current message assumes one specific cause (“Remote Control 未连接”) and is language-mixed. Disabled tools can fail for other reasons, so this can mislead users.
Suggested wording
- content: `工具 "${input.tool_name}" 当前不可用：Remote Control 未连接。`, + content: `Tool "${input.tool_name}" is currently unavailable.`,🤖 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 `@packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts` around lines 77 - 90, Replace the mixed-language, cause-specific message in the targetTool.isEnabled() early-return with a generic, consistent unavailable message (e.g. "工具 \"${input.tool_name}\" 当前不可用。") by updating the createUserMessage call in the block that checks targetTool.isEnabled(); ensure you remove the "Remote Control 未连接" clause and keep the rest of the return structure (data.result, tool_name, newMessages) unchanged so callers and payload shape remain the same.spec/feature_20260508_F001_tool-search/spec-plan-2.md-40-545 (1)
40-545:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSync this spec with the renamed SearchExtraTools flow.
Large parts of this plan still describe the pre-rename surface (
ToolSearch,ExecuteTool,src/services/toolSearch/*,EXPERIMENTAL_TOOL_SEARCH), and Task 7 even falls back tonpx tsc. Following this doc now would point future work at the wrong files, flags, and commands for this repo. Please update it to the currentSearchExtraTools/ExecuteExtraTool/src/services/searchExtraTools/*/bunx tscsurface before merging. Based on learnings Runtime must use Bun (not Node.js) for all imports, builds, and execution using Bun APIs.🤖 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 `@spec/feature_20260508_F001_tool-search/spec-plan-2.md` around lines 40 - 545, The spec references old names and paths (ToolSearch, ExecuteTool, EXPRIMENTAL_TOOL_SEARCH, src/services/toolSearch/*, ExecuteTool constants, getTurnZeroToolSearchPrefetch, startToolSearchPrefetch, etc.) but the repo now uses the renamed flow (SearchExtraTools, ExecuteExtraTool, src/services/searchExtraTools/* and the updated feature flag/name); update all occurrences in the plan and code changes to the new symbols and paths (e.g. replace ToolSearch → SearchExtraTools, ExecuteTool → ExecuteExtraTool, toolSearchPrefetch/startToolSearchPrefetch/getTurnZeroToolSearchPrefetch/collectToolSearchPrefetch → corresponding searchExtraTools variants, EXPRIMENTAL_TOOL_SEARCH → SEARCH_EXTRA_TOOLS, and imports from `@claude-code-best/builtin-tools/tools/ToolSearchTool/`* → the new SearchExtraTools module paths), adjust test file names and require/conditional-import code to use Bun-compatible import/require patterns (bunx tsc for TypeScript checks) and ensure all referenced constants (EXECUTE_TOOL_NAME) point to the renamed ExecuteExtraTool constants; keep the described logic but target the renamed modules, flags, and file paths throughout.src/utils/messages.ts-3914-3929 (1)
3914-3929:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle persisted
tool_discoveryattachments when the flag is off.
normalizeAttachmentForAPI()also runs on resumed transcripts. With this handler fully gated, a previously storedtool_discoveryattachment falls through to the unknown-attachment path wheneverEXPERIMENTAL_SEARCH_EXTRA_TOOLSis disabled, which logs an error and silently drops it. Please add an unconditional no-op fallback in the switch so resume behavior stays forward-compatible.Suggested fallback
+ case 'tool_discovery': + 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 `@src/utils/messages.ts` around lines 3914 - 3929, normalizeAttachmentForAPI() currently only handles attachment.type === 'tool_discovery' when feature('EXPERIMENTAL_SEARCH_EXTRA_TOOLS') is true, so resumed transcripts with persisted tool_discovery attachments hit the unknown-attachment path and get dropped; add an unconditional no-op branch for 'tool_discovery' in the switch inside normalizeAttachmentForAPI() that returns an empty result (e.g., []) or otherwise safely ignores the attachment when the flag is off so resumes don't log errors or drop data; keep the existing feature-gated behavior (wrapMessagesInSystemReminder / createUserMessage) intact and only add the unconditional fallback for tool_discovery.
🧹 Nitpick comments (8)
src/services/tokenEstimation.ts (1)
73-129: ⚡ Quick winAdd targeted regression tests for the normalized
tool_resultedge cases.Please add/extend tests to cover: (1)
tool_result.contentwith only tool-reference blocks (expects[tool references]placeholder), and (2) mixed content where only references are removed. This path is easy to regress and directly affects token-count API compatibility.Also applies to: 303-303
🤖 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 `@src/services/tokenEstimation.ts` around lines 73 - 129, Add targeted unit tests for stripSearchExtraToolsFieldsFromMessages to cover two edge cases: (1) a message with a tool_result whose content consists only of tool_reference blocks and should be replaced with a single text block [{ type: 'text', text: '[tool references]' }], and (2) a message with a tool_result containing mixed blocks where only the tool_reference blocks are removed and the remaining content is preserved. Use inputs that exercise the isToolReferenceBlock predicate and assert the normalized message content matches the expected transformed structure and that other message types remain unchanged.src/utils/permissions/classifierDecision.ts (1)
21-21: ⚡ Quick winPrefer importing the tool name from
constants.jsinstead ofprompt.js.This keeps permission/classifier code decoupled from prompt-layer modules and avoids pulling extra prompt text/dependencies into this path.
Suggested change
-import { SEARCH_EXTRA_TOOLS_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/SearchExtraToolsTool/prompt.js' +import { SEARCH_EXTRA_TOOLS_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/SearchExtraToolsTool/constants.js'🤖 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 `@src/utils/permissions/classifierDecision.ts` at line 21, The import of SEARCH_EXTRA_TOOLS_TOOL_NAME in classifierDecision.ts currently comes from the prompt module; change the import source to the tool's constants module (constants.js) so permission/classifier code doesn't pull prompt-layer content—update the import statement that references SEARCH_EXTRA_TOOLS_TOOL_NAME to import from the tool's constants.js instead of prompt.js, leaving the symbol name unchanged.src/hooks/useSearchExtraToolsHint.ts (1)
44-46: ⚡ Quick win
handleSelectcurrently drops the selected tool contextLine 44 accepts
toolNamebut never uses it, so selection behavior is effectively the same as dismiss. Either wiretoolNameinto downstream behavior/state or rename the API to reflect “acknowledge only” semantics.🤖 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 `@src/hooks/useSearchExtraToolsHint.ts` around lines 44 - 46, The handler handleSelect currently ignores its _toolName arg and only calls clearSearchExtraToolsPrefetchResults(), losing the selected-tool context; either consume toolName to update selection or rename the handler to indicate it only dismisses. Fix by removing the underscore and passing toolName into the downstream updater (e.g., call setSelectedTool(toolName) or invoke the existing selection callback) before/after calling clearSearchExtraToolsPrefetchResults(), or rename handleSelect to acknowledgePrefetchWithoutSelection if no selection state exists; reference the handleSelect function and clearSearchExtraToolsPrefetchResults call when making the change.src/constants/__tests__/tools.test.ts (1)
95-100: ⚡ Quick winThis test provides no runtime guarantee of immutability.
ReadonlySetis a compile-time type that is erased at runtime—instanceof Setcannot distinguish aReadonlySetfrom a mutableSet. The test will pass even ifCORE_TOOLSbecomes mutable, creating a false sense of safety. Either remove the test or replace it with a runtime check that would fail on mutation (e.g., attempting a mutating operation and verifying it throws or is prevented).🤖 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 `@src/constants/__tests__/tools.test.ts` around lines 95 - 100, The current test falsely assumes runtime immutability; replace the instanceof check with a mutation attempt against CORE_TOOLS and assert the set does not change: capture the original Array.from(CORE_TOOLS) and size, call a mutating method like CORE_TOOLS.add('___TEST___') or CORE_TOOLS.delete(someValue), then expect the size and Array.from(CORE_TOOLS) to equal the originals so the test fails if CORE_TOOLS becomes mutable. Use the existing test name (e.g., the block referencing CORE_TOOLS) and assert post-mutation equality to detect runtime mutations.src/services/api/openai/index.ts (3)
279-279: 💤 Low valueUpdate log message to match SearchExtraTools terminology.
The log message uses "Tool search" but the code has migrated to "SearchExtraTools". Update for consistency.
- `[OpenAI] Tool search enabled: ${includedDeferredTools}/${deferredToolNames.size} deferred tools included, total tools=${openaiTools.length}`, + `[OpenAI] SearchExtraTools enabled: ${includedDeferredTools}/${deferredToolNames.size} deferred tools included, total tools=${openaiTools.length}`,🤖 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 `@src/services/api/openai/index.ts` at line 279, Update the log message string to use the new "SearchExtraTools" terminology: replace the existing prefix "[OpenAI] Tool search enabled" with "[OpenAI] SearchExtraTools enabled" while keeping the rest of the message and variables (includedDeferredTools, deferredToolNames.size, openaiTools.length) unchanged so the logged content remains consistent.
283-283: 💤 Low valueUpdate log message to match SearchExtraTools terminology.
The log message uses "Tool search" but the code has migrated to "SearchExtraTools". Update for consistency.
- `[OpenAI] Tool search disabled, total tools=${openaiTools.length}`, + `[OpenAI] SearchExtraTools disabled, total tools=${openaiTools.length}`,🤖 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 `@src/services/api/openai/index.ts` at line 283, Update the log string that references the old "Tool search" terminology to use "SearchExtraTools" for consistency: locate the log call that currently emits `[OpenAI] Tool search disabled, total tools=${openaiTools.length}` (references openaiTools) and change its message to `[OpenAI] SearchExtraTools disabled, total tools=${openaiTools.length}` so the wording matches the migrated SearchExtraTools terminology.
196-196: 💤 Low valueUpdate comment to match SearchExtraTools terminology.
The comment still references "tool search" but the code has been migrated to use "SearchExtraTools" (line 197). Update the comment to maintain consistency with the codebase renaming.
- // 3. Check if tool search is enabled (similar to Anthropic path) + // 3. Check if SearchExtraTools is enabled (similar to Anthropic path)🤖 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 `@src/services/api/openai/index.ts` at line 196, Update the outdated comment that says "Check if tool search is enabled (similar to Anthropic path)" to use the project's current terminology "SearchExtraTools" so it reads e.g. "Check if SearchExtraTools is enabled (similar to Anthropic path)"; locate the comment near the SearchExtraTools usage in this file (referenced as SearchExtraTools) and replace the old phrase "tool search" with "SearchExtraTools" for consistency.src/services/api/claude.ts (1)
1410-1417: ⚡ Quick winImport
EXECUTE_TOOL_NAMEand use it instead of hardcoding the tool name in the prepended instructions.The string
ExecuteExtraToolis hardcoded in the ephemeral user-message content at lines 1410–1417 and also appears in the debug log at line 1283. TheEXECUTE_TOOL_NAMEconstant is defined inpackages/builtin-tools/src/tools/ExecuteTool/constants.tsand already imported insrc/constants/tools.tsandsrc/constants/prompts.ts. Using the constant here ensures the prompt stays in sync if the tool is renamed and eliminates duplicate hardcoded strings.Suggested change
+import { EXECUTE_TOOL_NAME } from '@claude-code-best/builtin-tools/tools/ExecuteTool/constants.js' @@ messagesForAPI = [ createUserMessage({ - content: `<available-deferred-tools>\n${deferredToolList}\n</available-deferred-tools>\nTo invoke any tool listed above, use ExecuteExtraTool with {"tool_name": "<name>", "params": {...}}. This is the ONLY way to call deferred tools — do not read source code or analyze implementation, just call ExecuteExtraTool directly.`, + content: `<available-deferred-tools>\n${deferredToolList}\n</available-deferred-tools>\nTo invoke any tool listed above, use ${EXECUTE_TOOL_NAME} with {"tool_name": "<name>", "params": {...}}. This is the ONLY way to call deferred tools — do not read source code or analyze implementation, just call ${EXECUTE_TOOL_NAME} directly.`, isMeta: true, }),🤖 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 `@src/services/api/claude.ts` around lines 1410 - 1417, Replace the hardcoded "ExecuteExtraTool" string with the shared constant EXECUTE_TOOL_NAME: import EXECUTE_TOOL_NAME into this module and update the ephemeral user message constructed in createUserMessage (and the debug log location that also prints the tool name) to use EXECUTE_TOOL_NAME instead of the literal "ExecuteExtraTool", so prompts and logs stay in sync with the tool's canonical name.
🤖 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 `@packages/builtin-tools/src/tools/BriefTool/BriefTool.ts`:
- Around line 152-154: The current isEnabled() replaced isBriefEnabled() with
isBridgeEnabled(), which bypasses opt-in and GrowthBook gates; restore the
unified Brief gate by making isEnabled() require both bridge availability and
Brief-specific checks (i.e., use isBridgeEnabled() && isBriefEnabled()) so the
existing opt-in flags and the tengu_kairos_brief feature flag remain enforced;
if instead the intention is to remove Brief-specific gating, delete the
isBriefEnabled() implementation and update all call sites to avoid confusion.
In `@packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts`:
- Around line 93-109: The permission check currently only blocks when
permResult.behavior === 'deny', allowing non-allow behaviors (e.g.,
approval-needed or undefined) to proceed; update the gating around
targetTool.checkPermissions (the call producing permResult) so that execution
only proceeds when permResult exists and permResult.behavior === 'allow'. For
other cases return early like the deny path: if behavior === 'deny' keep the
existing denial response, and for any non-allow behavior (including undefined)
return a controlled response that prevents execution (e.g., a
permission-denied/approval-required user message referencing input.tool_name and
permResult.message) instead of falling through to execute the tool. Ensure you
modify the condition around the current if (permResult && permResult.behavior
=== 'deny') and related control flow accordingly.
- Around line 140-145: The function mapToolResultToToolResultBlockParam
currently JSON.stringify's the entire content which collapses structured blocks;
update it to follow TeamDeleteTool/TeamCreateTool: return an object with
tool_use_id and type 'tool_result' but set content to an array like [{ type:
'text', text: jsonStringify(content) }] (i.e., preserve structured content by
wrapping a text block containing the stringified data rather than assigning
content: JSON.stringify(content)). Ensure you reference
mapToolResultToToolResultBlockParam and use the same wrapping pattern used in
TeamDeleteTool/TeamCreateTool.
In
`@packages/builtin-tools/src/tools/SearchExtraToolsTool/SearchExtraToolsTool.ts`:
- Around line 441-469: The discover: branch builds textResults/text but then
only returns buildSearchResult(...) with names; change the return so the
discovery payload is actually returned to the caller — pass the constructed text
(variable text) through this branch instead of dropping it. Locate the
discoverMatch block (discoverMatch, discoverQuery, getToolIndex, searchTools,
tfIdfResults, textResults, text, logSearchOutcome) and modify the final return
to include the text payload (e.g., extend buildSearchResult to accept a
text/body parameter and pass text, or return an object containing the existing
fields plus a text field) so the model receives descriptions and schemas.
In `@src/commands/clear/caches.ts`:
- Around line 132-137: The current code only clears the description memoizer via
clearSearchExtraToolsDescriptionCache; also import and invoke the tool index
cache reset from the searchExtraTools service so the TF‑IDF index is cleared on
/clear/resume. Add a dynamic import (matching the existing pattern) of
src/services/searchExtraTools/toolIndex (or the module that exports the index)
and call its exported cache clear/reset function (e.g.,
clearSearchExtraToolsIndexCache or resetToolIndexCache—use the actual exported
name) alongside clearSearchExtraToolsDescriptionCache, ensuring the call is
invoked (and awaited if it returns a promise).
In `@src/constants/prompts.ts`:
- Around line 281-285: The prompt text incorrectly lists CronCreate, CronDelete,
and CronList as directly callable core tools; update the string construction in
src/constants/prompts.ts to remove or rephrase those cron tool names so they are
not advertised as direct-call core tools (leave core tool list to true
always-available tools like
Read/Edit/Write/Glob/Grep/Bash/Agent/WebFetch/WebSearch/AskUserQuestion/NotebookEdit/TaskCreate/TaskUpdate/TaskList/TaskGet/TodoWrite/Skill/Config/LSP/MCPTool
and keep taskToolName handling), or explicitly mark cron operations as
deferred/discovered-only so the model will perform discovery before using
CronCreate/CronDelete/CronList. Ensure references to
${GLOB_TOOL_NAME}/${GREP_TOOL_NAME} and ${BASH_TOOL_NAME} remain unchanged.
- Around line 193-194: The paragraph incorrectly instructs discovering and
invoking skills via SearchExtraTools/ExecuteExtraTool while the rest of the
prompt expects the Skill/DiscoverSkills flow; reconcile this by editing that
paragraph so skills remain on the Skill/DiscoverSkills path (i.e., do NOT
include skills in the "SearchExtraTools first, then ExecuteExtraTool" rule)
and/or add an explicit exception line clarifying that
SearchExtraTools/ExecuteExtraTool apply to deferred/MCP tools only, while Skill
and DiscoverSkills must be used to discover and invoke user-invocable skills
(references: SearchExtraTools, ExecuteExtraTool, Skill, DiscoverSkills).
In `@src/services/searchExtraTools/__tests__/prefetch.test.ts`:
- Around line 12-14: PROJECT_ROOT is being resolved one directory too high (uses
five '..'), causing subprocess cwd to point to the repo parent; change the
PROJECT_ROOT resolve call in the test to go up one less directory (remove one
'..') so it resolves to the repo root (i.e., use four '..' components), leaving
RUNNER_ABS and RUNNER_REL unchanged.
In `@src/services/searchExtraTools/prefetch.ts`:
- Around line 47-50: clearSearchExtraToolsPrefetchResults currently empties
latestPrefetchResult but leaves the dedup set discoveredToolsThisSession intact,
which prevents rediscovery; modify clearSearchExtraToolsPrefetchResults to also
clear discoveredToolsThisSession (and then call notifyPrefetchListeners as
before) so a UI reset truly allows the same tools to be discovered again within
the session; reference the existing symbols latestPrefetchResult,
discoveredToolsThisSession, clearSearchExtraToolsPrefetchResults, and
notifyPrefetchListeners when making the change.
In `@src/services/searchExtraTools/toolIndex.ts`:
- Around line 182-187: The current logic forces pure-CJK queries with fewer
bigrams to zero out because matchingCjk.length is compared against a fixed
CJK_MIN_BIGRAM_MATCHES; change this to compare against a capped threshold based
on the query token count (e.g., threshold = Math.min(CJK_MIN_BIGRAM_MATCHES,
queryCjkTokens.length)) or explicitly allow single-token CJK queries (if
queryCjkTokens.length === 1) so that you only set score = 0 when
matchingCjk.length < threshold; update the block around queryCjkTokens,
matchingCjk, CJK_MIN_BIGRAM_MATCHES, queryAsciiTokens, entry.tfVector and score
accordingly.
In `@src/services/tools/toolExecution.ts`:
- Around line 627-637: Update the deferred-loading hint to instruct callers to
always use SEARCH_EXTRA_TOOLS_TOOL_NAME to discover the tool and then call
ExecuteExtraTool to run it (remove the “OpenAI-compatible models” qualifier and
the old direct-call example); explicitly reference SEARCH_EXTRA_TOOLS_TOOL_NAME
and ExecuteExtraTool, keep the camelCase parameter note, mention that Task tools
(TaskGet, TaskCreate, TaskUpdate, TaskList) must be discovered first and can be
discovered together via
SEARCH_EXTRA_TOOLS_TOOL_NAME("select:TaskGet,TaskCreate,TaskUpdate,TaskList"),
and replace the old example with one showing
SEARCH_EXTRA_TOOLS_TOOL_NAME("select:<tool.name>") → ExecuteExtraTool({ tool:
"<toolDisplayName>", params: { ... } }).
In `@src/skills/bundled/scheduleRemoteAgents.ts`:
- Line 185: The prompt incorrectly instructs calling the RemoteTrigger tool
directly; change the wording to require discovery via SearchExtraTools and
execution via ExecuteExtraTool using the REMOTE_TRIGGER_TOOL_NAME constant (do
not instruct loading + direct use of RemoteTrigger or using curl). Specifically,
replace any guidance that says "load it first with SearchExtraTools
select:${REMOTE_TRIGGER_TOOL_NAME}; auth is handled in-process — do not use
curl" and any direct references to RemoteTrigger with a clear sequence: discover
deferred tools with SearchExtraTools, then invoke ExecuteExtraTool with toolName
set to REMOTE_TRIGGER_TOOL_NAME and appropriate params; ensure the prompt never
instructs calling RemoteTrigger directly.
---
Outside diff comments:
In `@src/hooks/useReplBridge.tsx`:
- Around line 305-325: The "new tools available" system message is being
re-appended on reconnect because the reconnect flow flips
replBridgeSessionActive to false; change the gating to be once-per-session by
checking and storing a unique replBridgeSessionId instead of just
replBridgeSessionActive: read the current replBridgeSessionId from store (or
generate/compare the incoming session id used by setAppState), only call
setMessages(createSystemMessage(...)) when the session id is new, and update
setAppState to persist replBridgeSessionId alongside
replBridgeSessionActive/replBridgeConnected; also ensure the message text uses
the same transcript wording as other system messages created by
createSystemMessage.
In `@src/utils/analyzeContext.ts`:
- Around line 448-469: The code only marks deferred tools as loaded when seeing
an assistant 'tool_use' event, but it should also count tools discovered via
deferred_tools_delta/SearchExtraTools; update the logic in analyzeContext (the
branch using deferredBuiltinTools, isDeferred and loadedToolNames) to seed
loadedToolNames with extractDiscoveredToolNames(messages) (add each discovered
name into loadedToolNames) before scanning for 'tool_use' blocks; make the same
change in the analogous branch later in the file (the other
deferredBuiltinTools/isDeferred block) so discovered-but-not-yet-invoked
deferred tools are treated as loaded.
In `@src/utils/searchExtraTools.ts`:
- Around line 497-516: The code uses unsafe casts (msg as any) in
extractDiscoveredToolNames; replace them with explicit runtime-narrowing to
Record<string, unknown> and validate shapes before using: treat msg as
Record<string, unknown>, check msg.type === 'system' && msg.subtype ===
'compact_boundary', then read compactMetadata as Record<string, unknown> |
undefined, ensure compactMetadata.preCompactDiscoveredTools is an array via
Array.isArray and filter items to typeof === 'string' before adding to
discoveredTools and counting; similarly for the attachment branch, treat
msg.attachment as Record<string, unknown> | undefined, check attachment.type ===
'deferred_tools_delta', ensure attachment.addedNames is an array and push only
string entries (or default to []), removing all use of as any and replacing with
these guarded reads and filters.
---
Minor comments:
In `@docs/design/tool-search-design-guide.md`:
- Around line 26-34: The fenced ASCII diagram and other code blocks (e.g., the
"All Tools (60+ built-in + MCP)" table and blocks showing CORE_TOOLS and
isDeferredTool()) are missing language tags and trigger markdownlint MD040;
update each fenced code block in docs/design/tool-search-design-guide.md
(including the blocks called out around the CORE_TOOLS and isDeferredTool()
examples and the other ranges noted) to use an appropriate language label such
as ```text or ```bash so markdownlint stops flagging MD040 and the blocks render
consistently.
In `@packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts`:
- Around line 77-90: Replace the mixed-language, cause-specific message in the
targetTool.isEnabled() early-return with a generic, consistent unavailable
message (e.g. "工具 \"${input.tool_name}\" 当前不可用。") by updating the
createUserMessage call in the block that checks targetTool.isEnabled(); ensure
you remove the "Remote Control 未连接" clause and keep the rest of the return
structure (data.result, tool_name, newMessages) unchanged so callers and payload
shape remain the same.
In `@packages/builtin-tools/src/tools/TeamCreateTool/TeamCreateTool.ts`:
- Around line 130-134: The thrown Error message inside the
isAgentSwarmsEnabled() check uses Chinese text; update the message to English
for consistency by replacing the current string in the throw new Error(...)
inside TeamCreateTool (the block guarded by isAgentSwarmsEnabled()) with an
English message that mentions the Agent Teams feature and the
CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED env var (e.g., "Agent Teams
feature is not enabled. Ensure the CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED
environment variable is not set.").
In `@packages/builtin-tools/src/tools/TeamDeleteTool/TeamDeleteTool.ts`:
- Around line 92-96: The thrown Error in the TeamDeleteTool when
isAgentSwarmsEnabled() is false uses a Chinese message; change it to an English,
consistent message (e.g., "Agent Teams feature is not enabled. Ensure the
CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS_DISABLED environment variable is not
set."). Update the Error constructed in the conditional that checks
isAgentSwarmsEnabled() inside TeamDeleteTool (the throw new Error(...) block) to
use this English text so it matches the rest of the codebase.
In `@scripts/dev.ts`:
- Around line 21-22: The inline comment in scripts/dev.ts contradicts the env
setting: update the code so the comment and the value for 'process.env.NODE_ENV'
agree; either change JSON.stringify('production') to
JSON.stringify('development') if this is the dev build, or change the comment to
state production if the production value is intended—locate the
'process.env.NODE_ENV' entry and make the comment and value consistent.
In `@spec/feature_20260508_F001_tool-search/spec-design.md`:
- Around line 201-203: The spec and new file reference the old naming
"toolSearch" and type/name "ToolSearchTool" but this PR migrates to
"searchExtraTools" and "SearchExtraToolsTool"; locate usages and imports in the
new module (e.g., src/services/toolSearch/toolIndex.ts) and all referenced
places and rename module/path and type identifiers from toolSearch ->
searchExtraTools and ToolSearchTool -> SearchExtraToolsTool (update import
paths, exported names, type annotations and any factory/constructor calls),
ensure exported symbols and any index/registry entries match the new names and
run/adjust tests or imports that consume these symbols so compilation remains
green.
- Around line 248-260: The fenced code blocks containing diagrams like the one
starting with "query 输入", the block with "discover:<任务描述> —
纯发现搜索，不触发延迟加载，只返回工具信息", and the block starting "用户输入提交" are unlabeled and
trigger MD040; update each triple-backtick fence to include a language
identifier such as text (e.g., ```text) for every opening fence and leave the
closing fence unchanged, applying the same fix to the other unlabeled fences
noted (around the sections containing those exact block contents).
In `@spec/feature_20260508_F001_tool-search/spec-human-verify.md`:
- Around line 36-38: Update the verification checklist so all references to the
old tool and paths are replaced with the new ones: change every occurrence of
ToolSearchTool to SearchExtraToolsTool and every occurrence of
src/services/toolSearch to src/services/searchExtraTools, and update the grep
targets/expectations to point at the migrated file and function (e.g., the
isDeferredTool implementation in the new SearchExtraToolsTool prompt) so the
checks still assert presence of CORE_TOOLS.has and the function returning true.
In `@spec/feature_20260508_F001_tool-search/spec-plan-1.md`:
- Around line 193-195: The plan/checklist still uses the old module and type
names; update every occurrence of the module path pattern
"src/services/toolSearch/*" and the type/name "ToolSearchTool" to the new
standardized names "searchExtraTools" (module) and "SearchExtraToolsTool"
(type), and update any test file paths to match the new module naming (e.g.,
change toolIndex.test and toolIndex.ts references to the equivalent
searchExtraTools filenames); ensure core execution and verification steps,
examples, and any import/test references (previously referencing ToolSearchTool
or src/services/toolSearch) are consistently replaced with SearchExtraToolsTool
and searchExtraTools throughout the plan.
In `@spec/feature_20260508_F001_tool-search/spec-plan-2.md`:
- Around line 40-545: The spec references old names and paths (ToolSearch,
ExecuteTool, EXPRIMENTAL_TOOL_SEARCH, src/services/toolSearch/*, ExecuteTool
constants, getTurnZeroToolSearchPrefetch, startToolSearchPrefetch, etc.) but the
repo now uses the renamed flow (SearchExtraTools, ExecuteExtraTool,
src/services/searchExtraTools/* and the updated feature flag/name); update all
occurrences in the plan and code changes to the new symbols and paths (e.g.
replace ToolSearch → SearchExtraTools, ExecuteTool → ExecuteExtraTool,
toolSearchPrefetch/startToolSearchPrefetch/getTurnZeroToolSearchPrefetch/collectToolSearchPrefetch
→ corresponding searchExtraTools variants, EXPRIMENTAL_TOOL_SEARCH →
SEARCH_EXTRA_TOOLS, and imports from
`@claude-code-best/builtin-tools/tools/ToolSearchTool/`* → the new
SearchExtraTools module paths), adjust test file names and
require/conditional-import code to use Bun-compatible import/require patterns
(bunx tsc for TypeScript checks) and ensure all referenced constants
(EXECUTE_TOOL_NAME) point to the renamed ExecuteExtraTool constants; keep the
described logic but target the renamed modules, flags, and file paths
throughout.
In `@src/utils/messages.ts`:
- Around line 3914-3929: normalizeAttachmentForAPI() currently only handles
attachment.type === 'tool_discovery' when
feature('EXPERIMENTAL_SEARCH_EXTRA_TOOLS') is true, so resumed transcripts with
persisted tool_discovery attachments hit the unknown-attachment path and get
dropped; add an unconditional no-op branch for 'tool_discovery' in the switch
inside normalizeAttachmentForAPI() that returns an empty result (e.g., []) or
otherwise safely ignores the attachment when the flag is off so resumes don't
log errors or drop data; keep the existing feature-gated behavior
(wrapMessagesInSystemReminder / createUserMessage) intact and only add the
unconditional fallback for tool_discovery.
---
Nitpick comments:
In `@src/constants/__tests__/tools.test.ts`:
- Around line 95-100: The current test falsely assumes runtime immutability;
replace the instanceof check with a mutation attempt against CORE_TOOLS and
assert the set does not change: capture the original Array.from(CORE_TOOLS) and
size, call a mutating method like CORE_TOOLS.add('___TEST___') or
CORE_TOOLS.delete(someValue), then expect the size and Array.from(CORE_TOOLS) to
equal the originals so the test fails if CORE_TOOLS becomes mutable. Use the
existing test name (e.g., the block referencing CORE_TOOLS) and assert
post-mutation equality to detect runtime mutations.
In `@src/hooks/useSearchExtraToolsHint.ts`:
- Around line 44-46: The handler handleSelect currently ignores its _toolName
arg and only calls clearSearchExtraToolsPrefetchResults(), losing the
selected-tool context; either consume toolName to update selection or rename the
handler to indicate it only dismisses. Fix by removing the underscore and
passing toolName into the downstream updater (e.g., call
setSelectedTool(toolName) or invoke the existing selection callback)
before/after calling clearSearchExtraToolsPrefetchResults(), or rename
handleSelect to acknowledgePrefetchWithoutSelection if no selection state
exists; reference the handleSelect function and
clearSearchExtraToolsPrefetchResults call when making the change.
In `@src/services/api/claude.ts`:
- Around line 1410-1417: Replace the hardcoded "ExecuteExtraTool" string with
the shared constant EXECUTE_TOOL_NAME: import EXECUTE_TOOL_NAME into this module
and update the ephemeral user message constructed in createUserMessage (and the
debug log location that also prints the tool name) to use EXECUTE_TOOL_NAME
instead of the literal "ExecuteExtraTool", so prompts and logs stay in sync with
the tool's canonical name.
In `@src/services/api/openai/index.ts`:
- Line 279: Update the log message string to use the new "SearchExtraTools"
terminology: replace the existing prefix "[OpenAI] Tool search enabled" with
"[OpenAI] SearchExtraTools enabled" while keeping the rest of the message and
variables (includedDeferredTools, deferredToolNames.size, openaiTools.length)
unchanged so the logged content remains consistent.
- Line 283: Update the log string that references the old "Tool search"
terminology to use "SearchExtraTools" for consistency: locate the log call that
currently emits `[OpenAI] Tool search disabled, total
tools=${openaiTools.length}` (references openaiTools) and change its message to
`[OpenAI] SearchExtraTools disabled, total tools=${openaiTools.length}` so the
wording matches the migrated SearchExtraTools terminology.
- Line 196: Update the outdated comment that says "Check if tool search is
enabled (similar to Anthropic path)" to use the project's current terminology
"SearchExtraTools" so it reads e.g. "Check if SearchExtraTools is enabled
(similar to Anthropic path)"; locate the comment near the SearchExtraTools usage
in this file (referenced as SearchExtraTools) and replace the old phrase "tool
search" with "SearchExtraTools" for consistency.
In `@src/services/tokenEstimation.ts`:
- Around line 73-129: Add targeted unit tests for
stripSearchExtraToolsFieldsFromMessages to cover two edge cases: (1) a message
with a tool_result whose content consists only of tool_reference blocks and
should be replaced with a single text block [{ type: 'text', text: '[tool
references]' }], and (2) a message with a tool_result containing mixed blocks
where only the tool_reference blocks are removed and the remaining content is
preserved. Use inputs that exercise the isToolReferenceBlock predicate and
assert the normalized message content matches the expected transformed structure
and that other message types remain unchanged.
In `@src/utils/permissions/classifierDecision.ts`:
- Line 21: The import of SEARCH_EXTRA_TOOLS_TOOL_NAME in classifierDecision.ts
currently comes from the prompt module; change the import source to the tool's
constants module (constants.js) so permission/classifier code doesn't pull
prompt-layer content—update the import statement that references
SEARCH_EXTRA_TOOLS_TOOL_NAME to import from the tool's constants.js instead of
prompt.js, leaving the symbol name unchanged.
🪄 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: f6d70440-dde2-489e-a632-9cf117e9d547
📒 Files selected for processing (78)
CLAUDE.mddocs/design/tool-search-design-guide.mdpackages/@ant/computer-use-mcp/src/toolCalls.tspackages/builtin-tools/src/index.tspackages/builtin-tools/src/tools/BriefTool/BriefTool.tspackages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.tspackages/builtin-tools/src/tools/ExecuteTool/__tests__/ExecuteTool.runner.tspackages/builtin-tools/src/tools/ExecuteTool/__tests__/ExecuteTool.test.tspackages/builtin-tools/src/tools/ExecuteTool/constants.tspackages/builtin-tools/src/tools/ExecuteTool/prompt.tspackages/builtin-tools/src/tools/PushNotificationTool/PushNotificationTool.tspackages/builtin-tools/src/tools/SearchExtraToolsTool/SearchExtraToolsTool.tspackages/builtin-tools/src/tools/SearchExtraToolsTool/__tests__/SearchExtraToolsTool.test.tspackages/builtin-tools/src/tools/SearchExtraToolsTool/constants.tspackages/builtin-tools/src/tools/SearchExtraToolsTool/prompt.tspackages/builtin-tools/src/tools/SendMessageTool/SendMessageTool.tspackages/builtin-tools/src/tools/SendUserFileTool/SendUserFileTool.tspackages/builtin-tools/src/tools/TeamCreateTool/TeamCreateTool.tspackages/builtin-tools/src/tools/TeamDeleteTool/TeamDeleteTool.tspackages/builtin-tools/src/tools/ToolSearchTool/constants.tspackages/builtin-tools/src/tools/ToolSearchTool/prompt.tspackages/builtin-tools/src/tools/WebFetchTool/WebFetchTool.tsscripts/defines.tsscripts/dev.tsspec/feature_20260508_F001_tool-search/spec-design.mdspec/feature_20260508_F001_tool-search/spec-human-verify.mdspec/feature_20260508_F001_tool-search/spec-plan-1.mdspec/feature_20260508_F001_tool-search/spec-plan-2.mdsrc/Tool.tssrc/commands/clear/caches.tssrc/commands/commit-push-pr.tssrc/components/MessageRow.tsxsrc/components/Messages.tsxsrc/components/SearchExtraToolsHint.tsxsrc/components/__tests__/SearchExtraToolsHint.test.tssrc/components/messages/AttachmentMessage.tsxsrc/components/messages/CollapsedReadSearchContent.tsxsrc/constants/__tests__/tools.test.tssrc/constants/betas.tssrc/constants/promptEngineeringAudit.runner.tssrc/constants/prompts.tssrc/constants/tools.tssrc/hooks/useReplBridge.tsxsrc/hooks/useSearchExtraToolsHint.tssrc/main.tsxsrc/query.tssrc/screens/REPL.tsxsrc/services/PromptSuggestion/speculation.tssrc/services/acp/__tests__/bridge.test.tssrc/services/api/claude.tssrc/services/api/openai/__tests__/queryModelOpenAI.isolated.tssrc/services/api/openai/index.tssrc/services/compact/compact.tssrc/services/compact/sessionMemoryCompact.tssrc/services/searchExtraTools/__tests__/prefetch.runner.tssrc/services/searchExtraTools/__tests__/prefetch.test.tssrc/services/searchExtraTools/__tests__/toolIndex.test.tssrc/services/searchExtraTools/prefetch.tssrc/services/searchExtraTools/toolIndex.tssrc/services/skillSearch/localSearch.tssrc/services/tokenEstimation.tssrc/services/tools/toolExecution.tssrc/skills/bundled/scheduleRemoteAgents.tssrc/skills/bundled/stuck.tssrc/tasks/LocalAgentTask/LocalAgentTask.tsxsrc/tasks/LocalAgentTask/__tests__/LocalAgentTask.test.tssrc/tools.tssrc/utils/analyzeContext.tssrc/utils/api.tssrc/utils/attachments.tssrc/utils/betas.tssrc/utils/cacheWarning.tssrc/utils/claudeInChrome/prompt.tssrc/utils/collapseReadSearch.tssrc/utils/managedEnvConstants.tssrc/utils/messages.tssrc/utils/permissions/classifierDecision.tssrc/utils/searchExtraTools.ts
💤 Files with no reviewable changes (2)
- packages/builtin-tools/src/tools/ToolSearchTool/constants.ts
- packages/builtin-tools/src/tools/ToolSearchTool/prompt.ts
Comment on lines
152
to
154
| isEnabled() { | ||
| return isBriefEnabled() | ||
| return isBridgeEnabled() | ||
| }, |
Contributor
There was a problem hiding this comment.
Critical: Brief enablement logic bypasses opt-in and entitlement checks.
Switching from isBriefEnabled() to isBridgeEnabled() removes all user controls that gate Brief activation:
-
Opt-in requirement removed: Users who haven't explicitly opted in via
--brief,defaultView: 'chat',/brief, orCLAUDE_CODE_BRIEFenv var will now see the tool whenever the bridge is enabled. -
GrowthBook gate bypassed: The
tengu_kairos_brieffeature flag (refreshed every 5 minutes per line 98) is no longer checked, potentially exposing the tool outside the intended experiment cohort. -
Documentation mismatch: Lines 103-126 describe
isBriefEnabled()as the "unified activation gate" that "governs model-facing behavior as a unit: tool availability, system prompt section, tool-deferral bypass, and todo-nag suppression." That function is still exported but no longer used by the tool.
If the intent is to gate on bridge availability in addition to the existing checks, the fix should be:
isEnabled() {
- return isBridgeEnabled()
+ return isBriefEnabled() && isBridgeEnabled()
},If the intent is to replace all Brief-specific gates with a simple bridge check, then isBriefEnabled() should be removed and its call sites updated to prevent confusion.
🤖 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 `@packages/builtin-tools/src/tools/BriefTool/BriefTool.ts` around lines 152 -
154, The current isEnabled() replaced isBriefEnabled() with isBridgeEnabled(),
which bypasses opt-in and GrowthBook gates; restore the unified Brief gate by
making isEnabled() require both bridge availability and Brief-specific checks
(i.e., use isBridgeEnabled() && isBriefEnabled()) so the existing opt-in flags
and the tengu_kairos_brief feature flag remain enforced; if instead the
intention is to remove Brief-specific gating, delete the isBriefEnabled()
implementation and update all call sites to avoid confusion.
Comment on lines
+93
to
+109
| const permResult = await targetTool.checkPermissions?.( | ||
| input.params as Record<string, unknown>, | ||
| context, | ||
| ) | ||
| if (permResult && permResult.behavior === 'deny') { | ||
| return { | ||
| data: { | ||
| result: null, | ||
| tool_name: input.tool_name, | ||
| }, | ||
| newMessages: [ | ||
| createUserMessage({ | ||
| content: `Permission denied for tool "${input.tool_name}": ${permResult.message ?? 'Permission denied'}`, | ||
| }), | ||
| ], | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Permission gating is incomplete for delegated execution.
This path blocks only explicit deny, then executes the target tool. Any non-allow interactive behavior (e.g., approval-needed) can be bypassed here, which weakens permission enforcement for delegated tools.
Suggested guard pattern
- if (permResult && permResult.behavior === 'deny') {
+ if (
+ permResult &&
+ permResult.behavior !== 'allow' &&
+ permResult.behavior !== 'passthrough'
+ ) {
return {
data: {
result: null,
tool_name: input.tool_name,
},
newMessages: [
createUserMessage({
- content: `Permission denied for tool "${input.tool_name}": ${permResult.message ?? 'Permission denied'}`,
+ content: `Tool "${input.tool_name}" requires permission before execution: ${permResult.message ?? 'Permission 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 `@packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts` around lines 93
- 109, The permission check currently only blocks when permResult.behavior ===
'deny', allowing non-allow behaviors (e.g., approval-needed or undefined) to
proceed; update the gating around targetTool.checkPermissions (the call
producing permResult) so that execution only proceeds when permResult exists and
permResult.behavior === 'allow'. For other cases return early like the deny
path: if behavior === 'deny' keep the existing denial response, and for any
non-allow behavior (including undefined) return a controlled response that
prevents execution (e.g., a permission-denied/approval-required user message
referencing input.tool_name and permResult.message) instead of falling through
to execute the tool. Ensure you modify the condition around the current if
(permResult && permResult.behavior === 'deny') and related control flow
accordingly.
Comment on lines
+140
to
+145
| mapToolResultToToolResultBlockParam(content, toolUseID) { | ||
| return { | ||
| tool_use_id: toolUseID, | ||
| type: 'tool_result', | ||
| content: JSON.stringify(content), | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect ToolDef signature and tool_result content expectations:"
fd -i "Tool.ts" -x sh -c 'echo "---- $1 ----"; sed -n "1,260p" "$1"' sh {}
echo
echo "Compare mapToolResultToToolResultBlockParam implementations across tools:"
rg -n "mapToolResultToToolResultBlockParam\\(" --type=ts --type=tsx -C3
echo
echo "Inspect tool_result payload construction callsites:"
rg -n "tool_use_id|type:\\s*'tool_result'|content:" --type=ts --type=tsx -C2Repository: claude-code-best/claude-code
Length of output: 50431
🏁 Script executed:
# Find ToolResultBlockParam type definition
rg -n "type ToolResultBlockParam|interface ToolResultBlockParam" --type=ts --type=tsx -A 10
# Check ExecuteTool implementation context
rg -n "class ExecuteTool|export.*ExecuteTool" --type=ts --type=tsx -B 2 -A 15
# Find where ExecuteTool.mapToolResultToToolResultBlockParam is called
rg -n "mapToolResultToToolResultBlockParam" packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts -B 10 -A 10Repository: claude-code-best/claude-code
Length of output: 697
🏁 Script executed:
# Find ToolResultBlockParam type definition in src/Tool
fd -name "Tool.ts" -o -name "Tool.js" | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && rg "ToolResultBlockParam" {} -B 2 -A 8'
# Also search for content property type in tool_result contexts
rg "type.*ToolResultBlockParam|interface.*ToolResultBlockParam" -A 10Repository: claude-code-best/claude-code
Length of output: 44189
🏁 Script executed:
# Find ToolResultBlockParam type definition from SDK
rg "ToolResultBlockParam" src/Tool.ts -A 5
# Check the ExecuteTool file to understand the full context
fd -name "ExecuteTool.ts" | head -1 | xargs cat | head -200
# Search for examples of structured content blocks
rg "content.*\[" --type=ts --type=tsx | grep -i "tool_result\|type.*text" | head -20Repository: claude-code-best/claude-code
Length of output: 1595
🏁 Script executed:
# Read ExecuteTool.ts file
fd -name "ExecuteTool.ts" -path "*/ExecuteTool/*" | xargs cat | head -250
# Also check what ToolResultBlockParam content type looks like from SDK
# by searching for examples in the codebase
rg "content:.*\[.*type.*text" --type=ts --type=tsx -A 2 | head -30Repository: claude-code-best/claude-code
Length of output: 331
🏁 Script executed:
# Find the ExecuteTool file using git
git ls-files | grep -i executetool
# Read the file
cat packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts | head -250
# Search for examples where content is structured (array with type/text)
rg 'content:.*\[' packages/builtin-tools/src/tools -A 3 | head -40Repository: claude-code-best/claude-code
Length of output: 7617
🏁 Script executed:
# Check what ToolResultBlockParam type actually allows for content
python3 << 'EOF'
import json
# Based on the code patterns, let me search for how content is actually used
# in the codebase to understand what type it should be
import subprocess
result = subprocess.run(
['rg', 'type.*content.*string|.*content.*array|ContentBlock', '--type=ts', '--type=tsx', '-A', '2'],
capture_output=True,
text=True
)
# Look for actual usage patterns
lines = result.stdout.split('\n')
for i, line in enumerate(lines[:100]):
if 'content' in line.lower():
print(line)
EOF
# Check SDK types by looking at how content is used in actual implementations
rg 'content:.*string|content:.*\[' packages/builtin-tools/src/tools -B 3 -A 1 | head -80Repository: claude-code-best/claude-code
Length of output: 6276
🏁 Script executed:
# Look at TeamDeleteTool and TeamCreateTool to see the proper pattern
rg "mapToolResultToToolResultBlockParam" packages/builtin-tools/src/tools/TeamDeleteTool packages/builtin-tools/src/tools/TeamCreateTool -A 10 | head -50
# Check how delegated tool outputs flow through the system
rg "targetResult.*data|tool_result.*content" packages/builtin-tools/src/tools/ExecuteTool -B 3 -A 3
# Verify if ExecuteTool's output type matches what it passes to mapToolResultToToolResultBlockParam
rg "Output.*result|type.*Output" packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts -B 2 -A 2Repository: claude-code-best/claude-code
Length of output: 2745
Preserve structured tool_result content blocks instead of JSON-stringifying.
ExecuteTool wraps delegated tool outputs, but mapToolResultToToolResultBlockParam incorrectly collapses the result into a JSON string (content: JSON.stringify(content)). This prevents downstream processing from interpreting structured content blocks. Follow the pattern used in TeamDeleteTool and TeamCreateTool: return content: [{ type: 'text', text: jsonStringify(data) }] instead.
🤖 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 `@packages/builtin-tools/src/tools/ExecuteTool/ExecuteTool.ts` around lines 140
- 145, The function mapToolResultToToolResultBlockParam currently
JSON.stringify's the entire content which collapses structured blocks; update it
to follow TeamDeleteTool/TeamCreateTool: return an object with tool_use_id and
type 'tool_result' but set content to an array like [{ type: 'text', text:
jsonStringify(content) }] (i.e., preserve structured content by wrapping a text
block containing the stringified data rather than assigning content:
JSON.stringify(content)). Ensure you reference
mapToolResultToToolResultBlockParam and use the same wrapping pattern used in
TeamDeleteTool/TeamCreateTool.
Comment on lines
+441
to
+469
| // Check for discover: prefix — pure discovery search. | ||
| // Returns tool info (name + description + schema) as text, | ||
| // does NOT trigger deferred tool loading. | ||
| const discoverMatch = query.match(/^discover:(.+)$/i) | ||
| if (discoverMatch) { | ||
| const discoverQuery = discoverMatch[1]!.trim() | ||
| const index = await getToolIndex(deferredTools) | ||
| const tfIdfResults = searchTools(discoverQuery, index, max_results) | ||
| const textResults = tfIdfResults.map(r => { | ||
| let line = `**${r.name}** (score: ${r.score.toFixed(2)})\n${r.description}` | ||
| if (r.inputSchema) { | ||
| line += `\nSchema: ${JSON.stringify(r.inputSchema)}` | ||
| } | ||
| return line | ||
| }) | ||
| const text = | ||
| textResults.length > 0 | ||
| ? `Found ${textResults.length} tools:\n${textResults.join('\n\n')}` | ||
| : 'No matching deferred tools found' | ||
| logSearchOutcome( | ||
| tfIdfResults.map(r => r.name), | ||
| 'keyword', | ||
| ) | ||
| return buildSearchResult( | ||
| tfIdfResults.map(r => r.name), | ||
| query, | ||
| deferredTools.length, | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
discover: never returns the discovery payload it builds.
This branch constructs textResults and text, but then returns buildSearchResult(...), which only preserves tool names. In practice discover: behaves like the normal search path, so the model never sees the descriptions or schemas it was told to expect.
🤖 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
`@packages/builtin-tools/src/tools/SearchExtraToolsTool/SearchExtraToolsTool.ts`
around lines 441 - 469, The discover: branch builds textResults/text but then
only returns buildSearchResult(...) with names; change the return so the
discovery payload is actually returned to the caller — pass the constructed text
(variable text) through this branch instead of dropping it. Locate the
discoverMatch block (discoverMatch, discoverQuery, getToolIndex, searchTools,
tfIdfResults, textResults, text, logSearchOutcome) and modify the final return
to include the text payload (e.g., extend buildSearchResult to accept a
text/body parameter and pass text, or return an object containing the existing
fields plus a text field) so the model receives descriptions and schemas.
Comment on lines
+132
to
137
| // Clear SearchExtraTools description cache (full tool prompts, ~500KB for 50 MCP tools) | ||
| void import( | ||
| '@claude-code-best/builtin-tools/tools/ToolSearchTool/ToolSearchTool.js' | ||
| ).then(({ clearToolSearchDescriptionCache }) => | ||
| clearToolSearchDescriptionCache(), | ||
| '@claude-code-best/builtin-tools/tools/SearchExtraToolsTool/SearchExtraToolsTool.js' | ||
| ).then(({ clearSearchExtraToolsDescriptionCache }) => | ||
| clearSearchExtraToolsDescriptionCache(), | ||
| ) |
Contributor
There was a problem hiding this comment.
Clear the TF-IDF index cache here as well.
This reset only clears the description memoizer, but the new src/services/searchExtraTools/toolIndex.ts cache survives. After /clear or resume flows, SearchExtraTools can keep serving stale ranking/input-schema data for the previous session.
Suggested fix
void import(
'@claude-code-best/builtin-tools/tools/SearchExtraToolsTool/SearchExtraToolsTool.js'
).then(({ clearSearchExtraToolsDescriptionCache }) =>
clearSearchExtraToolsDescriptionCache(),
)
+ void import('../../services/searchExtraTools/toolIndex.js').then(
+ ({ clearToolIndexCache }) => clearToolIndexCache(),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Clear SearchExtraTools description cache (full tool prompts, ~500KB for 50 MCP tools) | |
| void import( | |
| '@claude-code-best/builtin-tools/tools/ToolSearchTool/ToolSearchTool.js' | |
| ).then(({ clearToolSearchDescriptionCache }) => | |
| clearToolSearchDescriptionCache(), | |
| '@claude-code-best/builtin-tools/tools/SearchExtraToolsTool/SearchExtraToolsTool.js' | |
| ).then(({ clearSearchExtraToolsDescriptionCache }) => | |
| clearSearchExtraToolsDescriptionCache(), | |
| ) | |
| // Clear SearchExtraTools description cache (full tool prompts, ~500KB for 50 MCP tools) | |
| void import( | |
| '@claude-code-best/builtin-tools/tools/SearchExtraToolsTool/SearchExtraToolsTool.js' | |
| ).then(({ clearSearchExtraToolsDescriptionCache }) => | |
| clearSearchExtraToolsDescriptionCache(), | |
| ) | |
| void import('../../services/searchExtraTools/toolIndex.js').then( | |
| ({ clearToolIndexCache }) => clearToolIndexCache(), | |
| ) |
🤖 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 `@src/commands/clear/caches.ts` around lines 132 - 137, The current code only
clears the description memoizer via clearSearchExtraToolsDescriptionCache; also
import and invoke the tool index cache reset from the searchExtraTools service
so the TF‑IDF index is cleared on /clear/resume. Add a dynamic import (matching
the existing pattern) of src/services/searchExtraTools/toolIndex (or the module
that exports the index) and call its exported cache clear/reset function (e.g.,
clearSearchExtraToolsIndexCache or resetToolIndexCache—use the actual exported
name) alongside clearSearchExtraToolsDescriptionCache, ensuring the call is
invoked (and awaited if it returns a promise).
Comment on lines
+12
to
+14
| const PROJECT_ROOT = resolve(__dirname, '..', '..', '..', '..', '..') | ||
| const RUNNER_ABS = resolve(__dirname, 'prefetch.runner.ts') | ||
| const RUNNER_REL = './' + relative(PROJECT_ROOT, RUNNER_ABS).replace(/\\/g, '/') |
Contributor
There was a problem hiding this comment.
Fix project root calculation for subprocess cwd
Line 12 goes up one directory too far. That sets cwd to the repo parent, which can cause flaky/failed subprocess test execution depending on runner layout.
Proposed fix
-const PROJECT_ROOT = resolve(__dirname, '..', '..', '..', '..', '..')
+const PROJECT_ROOT = resolve(__dirname, '..', '..', '..', '..')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const PROJECT_ROOT = resolve(__dirname, '..', '..', '..', '..', '..') | |
| const RUNNER_ABS = resolve(__dirname, 'prefetch.runner.ts') | |
| const RUNNER_REL = './' + relative(PROJECT_ROOT, RUNNER_ABS).replace(/\\/g, '/') | |
| const PROJECT_ROOT = resolve(__dirname, '..', '..', '..', '..') | |
| const RUNNER_ABS = resolve(__dirname, 'prefetch.runner.ts') | |
| const RUNNER_REL = './' + relative(PROJECT_ROOT, RUNNER_ABS).replace(/\\/g, '/') |
🤖 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 `@src/services/searchExtraTools/__tests__/prefetch.test.ts` around lines 12 -
14, PROJECT_ROOT is being resolved one directory too high (uses five '..'),
causing subprocess cwd to point to the repo parent; change the PROJECT_ROOT
resolve call in the test to go up one less directory (remove one '..') so it
resolves to the repo root (i.e., use four '..' components), leaving RUNNER_ABS
and RUNNER_REL unchanged.
Comment on lines
+47
to
+50
| export function clearSearchExtraToolsPrefetchResults(): void { | ||
| latestPrefetchResult = [] | ||
| notifyPrefetchListeners() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C3 '\bclearSearchExtraToolsPrefetchResults\b'
rg -nP --type=ts -C3 '\bdiscoveredToolsThisSession\b'Repository: claude-code-best/claude-code
Length of output: 6761
clearSearchExtraToolsPrefetchResults leaves discoveredToolsThisSession populated, preventing tool re-discovery after a clear.
After dismissing or selecting a tool hint, clearSearchExtraToolsPrefetchResults() empties the visible results but leaves the dedup set intact. This means the next search call will filter out all previously-shown tools, returning [] even if new tools could be discovered. Since re-clearing only happens via the bounded trim when the set reaches 500 entries (which may never occur), users cannot see the same tools again within a session. Clear the set here to align with the expected behavior of a UI reset:
♻️ Suggested fix
export function clearSearchExtraToolsPrefetchResults(): void {
latestPrefetchResult = []
+ discoveredToolsThisSession.clear()
notifyPrefetchListeners()
}🤖 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 `@src/services/searchExtraTools/prefetch.ts` around lines 47 - 50,
clearSearchExtraToolsPrefetchResults currently empties latestPrefetchResult but
leaves the dedup set discoveredToolsThisSession intact, which prevents
rediscovery; modify clearSearchExtraToolsPrefetchResults to also clear
discoveredToolsThisSession (and then call notifyPrefetchListeners as before) so
a UI reset truly allows the same tools to be discovered again within the
session; reference the existing symbols latestPrefetchResult,
discoveredToolsThisSession, clearSearchExtraToolsPrefetchResults, and
notifyPrefetchListeners when making the change.
Comment on lines
+182
to
+187
| if (queryCjkTokens.length > 0 && score > 0) { | ||
| const matchingCjk = queryCjkTokens.filter(t => entry.tfVector.has(t)) | ||
| if (matchingCjk.length < CJK_MIN_BIGRAM_MATCHES) { | ||
| const hasAsciiMatch = queryAsciiTokens.some(t => entry.tfVector.has(t)) | ||
| if (!hasAsciiMatch) score = 0 | ||
| } |
Contributor
There was a problem hiding this comment.
Short CJK queries get filtered out unconditionally.
When tokenization produces a single CJK bigram, matchingCjk.length can never reach CJK_MIN_BIGRAM_MATCHES, so an otherwise valid pure-CJK match is forced to score 0. Please cap the threshold by the query token count or special-case one-token CJK queries.
Suggested fix
- if (matchingCjk.length < CJK_MIN_BIGRAM_MATCHES) {
+ const minRequiredCjkMatches = Math.min(
+ CJK_MIN_BIGRAM_MATCHES,
+ queryCjkTokens.length,
+ )
+ if (matchingCjk.length < minRequiredCjkMatches) {
const hasAsciiMatch = queryAsciiTokens.some(t => entry.tfVector.has(t))
if (!hasAsciiMatch) score = 0
}🤖 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 `@src/services/searchExtraTools/toolIndex.ts` around lines 182 - 187, The
current logic forces pure-CJK queries with fewer bigrams to zero out because
matchingCjk.length is compared against a fixed CJK_MIN_BIGRAM_MATCHES; change
this to compare against a capped threshold based on the query token count (e.g.,
threshold = Math.min(CJK_MIN_BIGRAM_MATCHES, queryCjkTokens.length)) or
explicitly allow single-token CJK queries (if queryCjkTokens.length === 1) so
that you only set score = 0 when matchingCjk.length < threshold; update the
block around queryCjkTokens, matchingCjk, CJK_MIN_BIGRAM_MATCHES,
queryAsciiTokens, entry.tfVector and score accordingly.
Comment on lines
627
to
637
| `\n\nTool "${toolDisplayName}" is deferred-loading and needs to be discovered before use.\n` + | ||
| `When using OpenAI-compatible models (DeepSeek, Ollama, etc.), follow these steps:\n` + | ||
| `1. First discover the tool with ToolSearch: ${TOOL_SEARCH_TOOL_NAME}("select:${tool.name}")\n` + | ||
| `1. First discover the tool with SearchExtraTools: ${SEARCH_EXTRA_TOOLS_TOOL_NAME}("select:${tool.name}")\n` + | ||
| `2. Then call ${toolDisplayName} tool\n` + | ||
| `\nExample:\n` + | ||
| `${TOOL_SEARCH_TOOL_NAME}("select:${tool.name}") → ${toolDisplayName}({ ... })\n` + | ||
| `${SEARCH_EXTRA_TOOLS_TOOL_NAME}("select:${tool.name}") → ${toolDisplayName}({ ... })\n` + | ||
| `\nImportant notes:\n` + | ||
| `• Use camelCase parameter names (e.g., taskId), not snake_case (task_id)\n` + | ||
| `• All task tools (TaskGet, TaskCreate, TaskUpdate, TaskList) need to be discovered first\n` + | ||
| `• You can discover them all at once: ${TOOL_SEARCH_TOOL_NAME}("select:TaskGet,TaskCreate,TaskUpdate,TaskList")\n` + | ||
| `• You can discover them all at once: ${SEARCH_EXTRA_TOOLS_TOOL_NAME}("select:TaskGet,TaskCreate,TaskUpdate,TaskList")\n` + | ||
| `\nSee docs/openai-task-tools.md for detailed guide.` |
Contributor
There was a problem hiding this comment.
Update this recovery hint to the new ExecuteExtraTool flow.
The message still says this only applies to “OpenAI-compatible models” and then tells the model to call the deferred tool directly after SearchExtraTools. In this PR the deferred path is unified around SearchExtraTools + ExecuteExtraTool, so this guidance can send non-OpenAI models right back into the same failing call pattern instead of the supported recovery path.
🤖 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 `@src/services/tools/toolExecution.ts` around lines 627 - 637, Update the
deferred-loading hint to instruct callers to always use
SEARCH_EXTRA_TOOLS_TOOL_NAME to discover the tool and then call ExecuteExtraTool
to run it (remove the “OpenAI-compatible models” qualifier and the old
direct-call example); explicitly reference SEARCH_EXTRA_TOOLS_TOOL_NAME and
ExecuteExtraTool, keep the camelCase parameter note, mention that Task tools
(TaskGet, TaskCreate, TaskUpdate, TaskList) must be discovered first and can be
discovered together via
SEARCH_EXTRA_TOOLS_TOOL_NAME("select:TaskGet,TaskCreate,TaskUpdate,TaskList"),
and replace the old example with one showing
SEARCH_EXTRA_TOOLS_TOOL_NAME("select:<tool.name>") → ExecuteExtraTool({ tool:
"<toolDisplayName>", params: { ... } }).
| ## What You Can Do | ||
|
|
||
| Use the \`${REMOTE_TRIGGER_TOOL_NAME}\` tool (load it first with \`ToolSearch select:${REMOTE_TRIGGER_TOOL_NAME}\`; auth is handled in-process — do not use curl): | ||
| Use the \`${REMOTE_TRIGGER_TOOL_NAME}\` tool (load it first with \`SearchExtraTools select:${REMOTE_TRIGGER_TOOL_NAME}\`; auth is handled in-process — do not use curl): |
Contributor
There was a problem hiding this comment.
This prompt still tells the model to call RemoteTrigger directly.
Under the new flow, SearchExtraTools only discovers deferred tools; execution must go through ExecuteExtraTool. Telling the model to “load it first” and then use ${REMOTE_TRIGGER_TOOL_NAME} directly will break whenever that tool is not already loaded.
🤖 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 `@src/skills/bundled/scheduleRemoteAgents.ts` at line 185, The prompt
incorrectly instructs calling the RemoteTrigger tool directly; change the
wording to require discovery via SearchExtraTools and execution via
ExecuteExtraTool using the REMOTE_TRIGGER_TOOL_NAME constant (do not instruct
loading + direct use of RemoteTrigger or using curl). Specifically, replace any
guidance that says "load it first with SearchExtraTools
select:${REMOTE_TRIGGER_TOOL_NAME}; auth is handled in-process — do not use
curl" and any direct references to RemoteTrigger with a clear sequence: discover
deferred tools with SearchExtraTools, then invoke ExecuteExtraTool with toolName
set to REMOTE_TRIGGER_TOOL_NAME and appropriate params; ensure the prompt never
instructs calling RemoteTrigger directly.
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.
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
Documentation