fix(agent): rank tool_search by term selectivity, not field precedence - #3865
Conversation
The multi-term fallback matched any candidate sharing any one term, ordered by strongest matched field before number of matched terms. A common word in a tool name therefore outranked a rare word in a description, and with a five-result window the alphabetically-first `list_*` tools filled every slot. Searching `list github issues` returned five unrelated `list_agent_*` tools and reported `miss: false`, so the caller was told the search had succeeded. Weight each term by inverse document frequency and require a candidate to match at least one term that appears in at most half the catalog. A term matching nearly everything can still add score but can no longer be the sole reason a candidate is returned, so a query with no discriminating match now reports a miss instead of returning filler. Also resolve `<namespace>__<tool_id>` on the raw query, before normalization rewrites `_` to a space. Splitting it into words let unrelated platform tools win on the generic halves: `jira__list_projects` surfaced the platform's own `list_projects`, a callable tool in the wrong namespace, which is worse than returning nothing. An id the run is authorized for still resolves by exact name; one it is not resolves to its namespace. One existing expectation changed: a query whose only match on a candidate is a term shared by every candidate no longer returns that candidate. That result was filler by the same mechanism this fixes.
…union Review follow-ups, no behaviour change. Field precedence and score weight were two positional arrays kept in the same order by convention, so reordering one silently mis-weighted the other. Key both off a `ToolSearchMatchField` union instead, which makes the compiler enforce that they stay exhaustive and aligned. Normalize each candidate's name and description once at snapshot time. Matching rescans every candidate once per term to compute document frequency and again to score, so normalizing inside the match check repeated the same string work across the whole catalog on every term. Replace an em dash in a comment with a semicolon, per the public copy rules in AGENTS.md. Extend the canonical-id test to the other ids whose generic half collides with a platform tool: list_comments, list_sites, search_users, and github__list_repos.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3b300dfc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const wholeQueryMatches = rankWholeQueryMatches(query, candidates); | ||
| if (wholeQueryMatches.length > 0) return wholeQueryMatches; |
There was a problem hiding this comment.
Route canonical IDs before normalized phrase matches
When an unavailable canonical ID normalizes to an allowed local tool name, this early return bypasses namespace handling and loads the local tool. For example, jira__list_projects and the valid local ID jira_list_projects both normalize to jira list projects; because src/tool/registry.ts:34-40 permits local IDs without __, the latter becomes a whole-query exact-name match instead of resolving to the Jira namespace catalog. Detect canonical input first, then allow only an actual canonical-name match before falling back to namespace discovery.
Useful? React with 👍 / 👎.
| */ | ||
| const TOOL_SEARCH_MIN_SELECTIVE_IDF = Math.LN2; | ||
| /** Canonical integration tool ids are `<namespace>__<tool_id>` over this segment shape. */ | ||
| const CANONICAL_INTEGRATION_SEGMENT = /^[a-z0-9]+(?:[-_][a-z0-9]+)*$/; |
There was a problem hiding this comment.
Reuse the canonical integration segment contract
This regex is stricter than the authoritative contract in src/integrations/source-policy.ts:76, which accepts any segment matching ^[a-z0-9][a-z0-9_-]*$. Consequently, valid integration IDs with consecutive or trailing separators, such as github__list-issues-, are not recognized when unavailable and fall through to ordinary term splitting rather than namespace discovery. Use the shared parser or exactly preserve its accepted grammar so search does not disagree with authorization and remote-tool validation.
AGENTS.md reference: AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
Addresses both Codex P2 findings on #3865. Route canonical ids first. Normalization maps `jira__list_projects` and the local id `jira_list_projects` onto the same text, and the registry permits any local id without `__`, so the phrase pass could return a same-named local tool for a canonical query. That is the same wrong-namespace failure this PR set out to fix, reached by a different path. Only the real canonical name now satisfies a canonical query. Namespace discovery now requires evidence rather than a name collision: a sibling tool in the same namespace, or a tool that documents the namespace. A local tool merely containing the word is not the integration. Use the authorization layer's `parseIntegrationToolIdentity` instead of a local regex. The local grammar was stricter than the authoritative one and rejected ids with consecutive or trailing separators, so search and authorization disagreed about what a canonical id is. The comment justifying the duplication was also wrong: the runtime already value-imports from that module.
|
Both P2 findings were real and are fixed in Route canonical IDs before normalized phrase matches — confirmed. Canonical ids are now classified before any normalized matching, and only the real canonical name satisfies a canonical query. Fixing that exposed a second hole in the same area that the finding did not name: the namespace fallback still ranked Reuse the canonical integration segment contract — confirmed, and the justification I wrote for duplicating it was factually wrong. My grammar was stricter than Coverage added
42 tests pass. One note for reviewers on automated status here: CodeRabbit reported nothing because this repo is under its 10-star threshold, not because it reviewed and found the PR clean. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23e3e363bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (inverseDocumentFrequency >= TOOL_SEARCH_MIN_SELECTIVE_IDF) matchedSelectiveTerm = true; | ||
| } | ||
| if (!matchedSelectiveTerm) continue; |
There was a problem hiding this comment.
Allow matches in single-candidate catalogs
When the deferred catalog contains one tool and a multiword capability query matches that tool without matching as one contiguous phrase, every matched term has documentFrequency = total = 1, so its IDF is log(2 / 1.5), below this threshold. For example, a sole create_file tool described as Create a project file is reported as a miss for create project, even though both terms match and there is no competing candidate. This breaks capability-phrase discovery whenever only one deferred schema remains unless the model knows its exact name or description phrase, so the selectivity guard needs a single-candidate or equivalent strong-coverage exception.
Useful? React with 👍 / 👎.
…y floor The selectivity floor suppresses filler, which only means something when there is a better candidate to prefer instead. In a single-tool catalog every term matches every candidate by definition, so no term can clear the floor and a certain match was reported as a miss: a sole `create_file` described "Create a project file" missed for the query `create project`. A candidate matching every term in the query is not filler however common those terms are, so full coverage now satisfies the guard on its own. Candidates that match only a subset still need a selective term, so a query naming one rare term alongside common ones is unaffected.
|
Second Codex pass, Allow matches in single-candidate catalogs — real, fixed. Correct, and a regression I introduced. With one candidate every term matches everything, so The floor exists to suppress filler, and filler only means something when there is a better candidate to prefer instead. A candidate matching every term is not filler however common those terms are, so full coverage now satisfies the guard on its own. Candidates matching only a subset still need a selective term, so the original defect stays closed. Covered by a new test using the reported example. Route canonical IDs before normalized phrase matches — already fixed in the commit under review. This is the same finding, verbatim, from the previous pass; it was addressed in The exact example given is covered by a passing test: with both the catalog and a local 43 tests pass. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce8d2c4934
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const field = getMatchedField(canonical.namespace, candidate); | ||
| return field === "description" || field === "parameterDescription"; | ||
| }); | ||
| return rankWholeQueryMatches(canonical.namespace, namespaceCandidates); |
There was a problem hiding this comment.
Normalize canonical namespaces before matching
For a valid canonical ID whose integration segment contains an underscore, such as foo_bar__list_items, parseIntegrationToolIdentity returns the namespace as foo_bar, but every candidate name and description has already normalized underscores to spaces. Consequently the catalog candidate is excluded at line 438, and even a same-namespace sibling admitted through its parsed identity fails this ranking call, so the search incorrectly reports a miss instead of resolving the namespace. Normalize canonical.namespace before using it with getMatchedField and rankWholeQueryMatches.
Useful? React with 👍 / 👎.
Two related holes on the canonical path, both found in review. A query carrying `__` that is not a *valid* canonical id fell through to phrase matching, where normalization collapsed it onto a local id. `jira__list__projects` returned the local `jira_list_projects` rather than the Jira catalog. `__` is the reserved integration namespace separator and `assertLocalToolId` forbids it in local ids, so any query carrying it is asking for an integration tool even when the rest is malformed. Such a query now stays on the namespace path, keyed off the leading segment. Namespace text evidence is also matched normalized. A namespace may itself contain `_`, and every candidate's name and description has already had underscores rewritten to spaces, so an un-normalized `foo_bar` could never match and the search reported a miss. Sibling identity still compares raw ids.
|
Third pass, Route canonical IDs before normalized phrase matches — I was wrong to dismiss this. I said twice that this was a stale repeat, because the exact repro given ( A query carrying The repro was wrong and the conclusion was right. Flagging that plainly because I twice asserted otherwise on this thread. Normalize canonical namespaces before matching — real, fixed. Correct. No integration in the live catalog currently has an underscore in its name, so this was latent rather than user-visible, but the grammar permits it. Coverage added
45 tests pass. |
|
@codex review |
|
Fourth posting of Route canonical IDs before normalized phrase matches, unchanged text. The variant it did catch was fixed in Probed every namespace-shaped input I could construct against a catalog seeded with three deliberately colliding local tools ( Ten of eleven resolve to the catalog and never to a colliding local tool, including the exact repro in the comment. The eleventh is a leading I got this wrong once already on this thread by dismissing the finding twice before probing, so I am deliberately showing the matrix instead of claiming it is a duplicate. If there is a shape missing from that list that reaches a local tool, point at it and I will fix it. Not requesting another review, since the comment has now repeated verbatim across four passes against three different commits including two that changed exactly this code path. Status: 45 tests pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12a92969ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const field = getMatchedField(namespaceTerm, candidate); | ||
| return field === "description" || field === "parameterDescription"; |
There was a problem hiding this comment.
Match canonical namespaces as complete tokens
For canonical queries in short registered namespaces, this reuses substring matching to decide whether an unrelated tool documents the namespace. For example, exa is a real integration in src/integrations/schema.ts, so an unavailable query such as exa__search admits any tool whose description or parameter description contains example; those unrelated tools can then be loaded instead of the integration catalog. Require a complete normalized namespace token, or explicit catalog metadata, on this path.
Useful? React with 👍 / 👎.
Namespaces can be very short: `exa` is a real integration, so substring evidence let `exa__search` admit any tool whose description or parameter description merely contained `example`, and those unrelated tools could be returned instead of the integration catalog. Namespace text evidence now requires a complete normalized token. Boundaries are non-alphanumeric because normalization has already lowercased the text and rewritten underscores to spaces, which also keeps multi-word namespaces such as `foo_bar` working. Checking description evidence directly dropped a guard that field precedence had been providing implicitly, so restore it explicitly: a non-canonical tool carrying the namespace in its name is a normalization coincidence and not the integration, whatever its description happens to mention.
|
Match canonical namespaces as complete tokens — real, fixed in Substring evidence let Fixing it surfaced something the finding did not mention. Checking description evidence directly dropped a guard that field precedence had been providing implicitly: two of my own tests went red because a local tool whose description mentions Jira started qualifying as documenting the namespace. That guard is now explicit rather than incidental. A non-canonical tool carrying the namespace in its name is a normalization coincidence, not the integration, whatever its description happens to mention. Re-ran the full shape matrix against a catalog seeded with All resolve to Route canonical IDs before normalized phrase matches — fifth posting, text unchanged. Evidence in the matrix above and in my previous comment; the repro given passes. Not requesting another review pass on it. 46 tests. |
Problem
tool_searchranked a match on a common term in a tool name above a match on the distinctive term in a tool description, so on any multi-word query the result window filled with alphabetically-first same-prefix tools and reportedmiss: false.Two mechanisms, both in the multi-term fallback:
Pure OR with rank compared before coverage. A candidate matching any single term qualified, and results sorted by strongest matched field first. A tool matching only
listin its name (name rank) therefore beat a tool matchinggithubin its description (description rank), no matter how many terms the latter also matched. With 27 authorized tools whose names containlistand a five-result cap, the window never reached the relevant one.__destroyed before the query is classified. Normalization rewrites_to a space, so a canonical<namespace>__<tool_id>was word-split into its generic halves.jira__list_projectsbecame the termsjira list projects, and the unrelated platform toollist_projectsmatched two of the three in its name and won outright. That is worse than returning noise: the caller receives a callable tool in the wrong namespace.misswas derived from result count rather than relevance, so a caller was told a search had succeeded when nothing relevant was found.Change
Weight terms by inverse document frequency. A term matching few candidates now outweighs one matching many, so a rare namespace term dominates a generic verb.
Require a selective term. A candidate must match at least one term appearing in at most half the catalog. A near-ubiquitous term can still contribute score but can no longer be the sole reason a candidate is returned, so a query with no discriminating match reports a miss instead of returning filler.
Resolve canonical ids before normalization.
<namespace>__<tool_id>is parsed off the raw query. An id the run is authorized for still resolves by exact name through the unchanged whole-query path; one it is not resolves to its namespace.Field precedence and score weight are keyed off a single
ToolSearchMatchFieldunion so the two orderings cannot drift apart, and each candidate's name and description are normalized once at snapshot time rather than on every term comparison.Behaviour change to review
One pre-existing expectation changed. For the query
create_file update_file project file,sandbox_write_fileis no longer returned: its only matched term wasfile, which appears in every candidate and so carries no information about which tool was meant. The other two results and their order are unchanged.That result was filler produced by the same mechanism this PR fixes, so the old expectation encoded the defect. Calling it out explicitly because "existing test updated" is exactly the shape of a test bent to fit an implementation, and it deserves a second opinion rather than my say-so.
Verification
jira__list_projects,jira__list_comments,jira__list_sites,jira__search_users, andgithub__list_repos— every id whose generic half collides with a platform tool.list_projectsquery still resolves to the platform tool; exact-name and single-term queries are unchanged.deno task test:unit3945 passed / 0 failed; typecheck,fmt:check, and lint clean.Note for anyone hitting red CI here: the full unit suite is flaky under parallel execution independently of this change.
mainata4ec5b280fails it too, on a different timing-sensitive test.