feat(runtime): add turn-scoped tool_search activation - #3765
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found blocking issues.
[P2] Client Capability search loses human meaning
Tool offer label/description are stored but not indexed; searching "schedule a calendar meeting" fails to activate an opaque remote/invoke tool whose capability is only in those fields, leaving it invisible to both search and model.
[P2] Per-search 64 KiB limit allows unbounded Turn-owned activation
The 64 KiB schema cap resets per search, but results are unioned into a persistent activeTools set. Three ~40k activations reach 120k, eventually exhausting context budget without compaction. The budget should be tracked cumulatively per Turn scope.
Checks on ca04b00d9f are test/audit/package: success.
简体中文
存在搜索信息与预算两项阻断。|
On the second point: the 64 KiB ceiling is intentionally per search, not cumulative per turn. Discussion #3621 and issue #3752 explicitly settled on monotonic turn-scoped accumulation with no turn-wide activation budget or unload operation; the model may make repeated searches to expand the active set, while each individual search is bounded by result count and schema bytes. The binding ceiling remains the final upper bound. A cumulative cap would introduce a new ordering-dependent failure mode: an earlier approximate search could consume the budget and prevent a later, more precise search from activating the required tool. That is a different contract from the agreed first slice. Context capacity is still measured on every provider projection and handled by the existing capacity/compaction path. The first point is valid. I will update Client Capability search documents to include the offer label/description while keeping the initial model-facing inventory limited to group ids and canonical tool names. |
|
The first point is fixed in 838516e. Search documents now include the owning group id, label, and description in addition to the canonical tool name and callable description. The initial tool_search inventory remains intentionally limited to group ids and canonical names. Coverage includes:
Build, typecheck, both focused tests, formatting, and diff checks pass. |
Astro-Han
left a comment
There was a problem hiding this comment.
Update on 838516e22d:
The previous search-semantics P2 is now closed (offer label/description only enter the per-member document, not initial inventory). The remaining P2 persists: per-search schemaChars resets while Turn-scoped activation is monotonic — exercising the limit three times reaches 120k and would exhaust budget without per-Turn accounting.
No new P0/P1; two P3 candidates remain for Map<MakaTool> → Set<string> and duplicated direct/searchable authority, plus a pending decision on MiniSearch value vs cost.
Checks on 838516e22d are still running — code is NO-GO due to remaining P2.
简体中文
一处语义已闭合,预算阻断仍在。|
The remaining points do not identify implementation defects. They mix the accepted product contract, state representation, and executable authority into one review concern. 1. Per-search schema budgetThe absence of a cumulative Turn budget is intentional and explicitly specified in #3621/#3752. Activation is monotonic within the Turn, there is no unload operation, and each search is independently bounded. The model is allowed to refine or expand the active surface until the binding ceiling. A cumulative cap would be a semantic change, not a safety fix. It makes activation order-dependent: an approximate early search could consume the Turn allowance and prevent a later precise search from activating the required tool. That is exactly why the agreed contract uses per-call limits plus the binding ceiling. The claim that this exhausts context “without compaction” also does not match the implementation. Every provider projection measures the complete currently visible schema surface, and the existing mid-turn capacity path includes schema growth from tool_search. packages/runtime/src/tests/mid-turn-capacity-backend.test.ts has a regression case named “the trigger counts same-turn tool-schema growth from tool_search”. If activation reaches the complete bound surface, that is the intentional ceiling, not unbounded state. Changing this contract requires reopening the design decision with evidence from evaluation; it is not a P2 implementation bug in this PR. 2. Map<string, MakaTool> versus SetThese types encode different state. A Set records only membership. The Map records the Turn-owned resolved binding associated with each activated name. Replacing it with a Set discards that information and requires the projection path to resolve names back through a backend-owned registry. The current Map is also the exact state shape agreed in #3752. There is no demonstrated correctness, memory, or performance problem here. A representation-only rewrite that throws away the resolved value is not required for this PR. 3. Direct and searchable are not duplicate executable authoritiesThere is one executable authority: the tools actually bound to the run.
ToolAvailabilityRuntime intersects every group with the known bound-tool set, ignores unknown members, prevents group metadata from deferring the fixed direct baseline, and can never create a binding. These are policy projections over one authority, not competing authorities. Removing either visibility classification would not simplify authority; it would remove required policy. 4. MiniSearch value and dependency costMiniSearch is not an arbitrary fuzzy-search dependency. It implements BM25+ scoring, which is the retrieval model selected by OpenAI Codex for its current tool_search implementation. Codex main at 9c9675d3d0:
Reference: https://github.com/openai/codex/blob/9c9675d3d0/codex-rs/core/src/tools/handlers/tool_search.rs Maka follows the same architecture in TypeScript: backend-scoped cached index plus Turn-scoped activation. MiniSearch 7.2.0 provides BM25+, has zero runtime dependencies, is MIT licensed, is exact-pinned, has generated ASF notices, and passes dependency audit/signature checks. Reimplementing BM25 ranking and indexing locally would add code and maintenance risk without removing the underlying algorithmic cost. The Client Capability P2 was valid and is fixed. The remaining cumulative-budget objection contradicts the accepted contract, while the Map, classification, and MiniSearch points do not establish defects. Please close the remaining P2 unless there is new evidence that the implemented behavior differs from #3752. |
|
Agreed on all four — closing the remaining P2. The cumulative-budget point was mine to withdraw. One thing I want recorded but not fixed here: when activation grows the surface enough to trigger compaction, the cost is paid in transcript, not in a failed search. That follows from the agreed contract rather than from anything in this PR — I will track it as something to measure once Thanks for the sourced replies, and for closing the search-document gap. 简体中文四条我都同意,剩下那条 P2 关掉。 累计预算那条该由我撤回: 有一点我想记下来但不在这里改:当激活确实把可见面撑到触发压缩时,代价是付在转录上,而不是一次失败的搜索。这来自已定的契约,不是本 PR 的问题——等 谢谢你带出处的回复,也谢谢补上搜索文档那个缺口。 |
|
I read the full diff this time rather than only the thread. One thing I would like fixed before approving, plus a few deletions that are cheap to take while this area is open. [P2] Already-active tools consume the next search's limit and byte budgetIn the connector for (const name of ranked) {
if (activated.length >= limit) break;
const tool = this.toolsByName.get(name);
if (!tool || !this.searchableNames.has(name)) continue;
const chars = toolSchemaCharsForDiagnostics([tool], [tool.name]);
if (schemaChars + chars > TOOL_SEARCH_MAX_SCHEMA_CHARS) continue;
activated.push(name);
schemaChars += chars;
}
const newlyActivated = activated.filter((name) => !activeTools.has(name));When the model refines a search, the default One line closes it: skip already-active names before the limit and budget checks, or union them into the response without charging them. This is not the cumulative-Turn-budget point from the earlier thread — I accepted that one. This is the existing per-search budget, and the two remaining notes below are on the same branch. [P3] A single tool larger than 64 KiB can never be activated
[P3] Relevance order is silently broken by sizeThe budget check uses These three share one cause: Deletions still available in this area[P1] [P2] [P2] One thing worth deciding, not fixing here
The fields cannot simply go, since What this gets rightThe persisted surfaces are handled properly: Fix the P2 with a test on that branch and I am happy to approve. 简体中文这次我完整读了 diff,不只是跟着讨论走。有一处希望在 approve 之前修掉,另外趁这块打开顺手可以删几处。 [P2] 已激活的工具会吃掉下一次搜索的名额和字节预算连接器 模型细化搜索时,默认 一行即可:在 limit 与预算判断之前跳过已激活的名字,或者把它们并入返回但不计名额与预算。 这不是之前那条「跨搜索累计预算」——那条我已经接受了。这里说的是现有的 per-search 预算,下面两条也在同一个分支上。 [P3] 单个超过 64 KiB 的工具永远激活不了
[P3] 相关性顺序被体积静默打破预算判断用的是 这三条同一个根因: 这块还能继续删的部分[P1] [P2] [P2] 一件值得定、但不必在这里改的事
字段本身不能直接删, 做得好的地方持久化面处理得规范: 把那条 P2 修掉并给这条分支补个测试,我这边就可以 approve。 |
|
Addressed in c28cb8d. Search expansion and schema budget
Added regression coverage for:
Deletions
I left the search-mode diagnostic shape unchanged as suggested; that deserves a separate measurement/design issue rather than expanding this PR. Validation:
|
|
Thanks for the fast turnaround — I read [P2]
|
Astro-Han
left a comment
There was a problem hiding this comment.
Approving. My comments above are non-blocking: the P2 on schema_too_large is worth taking as a follow-up, and the P3 items are opportunistic.
|
Took the follow-up P2 in da7259a. schema_too_large now records the blocked candidate and continues, so a permanently oversized match cannot starve smaller later results. schema_budget_exhausted still breaks to preserve the ranked prefix for candidates that could fit in a fresh search. The regression test now puts a smaller match behind the oversized top result and verifies that the result reports the oversized tool while activating the smaller one. Validated with format check, build:test, typecheck, the complete tool-availability suite, and diff check. |
Summary
tool_searchcontractTurnScope.activeToolsload_toolsdecoding only for transcript compatibility, never for cross-turn activationThis implements the first slice agreed in Discussion #3621.
Closes #3752.
Testing
npm run format:checknpm run build:testnpm run typechecknpm run check:third-party-noticesnpm run check:cli-third-party-noticesnpm audit --omit=dev --audit-level=moderatenpm audit signaturesThe complete local workspace test runner still encounters existing platform-sensitive failures in the macOS Bash executable-root assertion and Desktop deadline tests. The affected tool-search suites pass independently.
AI assistance disclosure
Maka helped implement the agreed design, migrate tests and documentation, and run validation. I reviewed the resulting behavior and diff.