feat(discover): pluggable ToolRanker with BM25 fallback and compare mode - #185
Conversation
The tinytools submodule is updated to a newer commit, incorporating upstream fixes and improvements. Auto-committed-on: macbook
The discovery module previously relied on a default feature flag that was not enabled in all build configurations, causing tools to be missing from the registry. This change makes the module unconditionally compiled, ensuring discovery works regardless of feature selection. Auto-committed-on: macbook
The file previously lacked a trailing newline, which could cause issues with some tooling and version control systems. This change adds the newline to ensure the file ends properly. Auto-committed-on: macbook
The discovery bridge now returns ranking metadata alongside search results, including which ranker was used, confidence scores, fallback reasons, shadow rankings, and latency. This information is surfaced in the `ToolSearched` event so callers can observe and audit how deferred tool discovery ranked results. The schema lookup also now pairs each deferred tool with its family name, and the BM25 index types are re-exported from the `rank` module to keep the public API aligned with the underlying crate. Auto-committed-on: macbook
The change makes `answer_tool_search` async and updates its return type to include ranking information, allowing tests to verify ranker behavior and fallback logic. It also adds tests for host ranker integration, including failure fallback to BM25 and compare mode behavior. Auto-committed-on: macbook
ToolDiscoveryPolicy gains a host ranker (any tinytools::ToolRanker) and a DiscoveryRankMode; DeferredCatalog::rank serves it, falls back to BM25 on failure or an empty answer, and in Compare mode reports the BM25 ranking alongside. answer_tool_search is async and returns a SearchAnswer; ToolSearched carries the ranker, top confidence, fallback reason, shadow ranking and latency. The BM25 index moved to tinytools::rank and is re-exported from its old path; the catalogue carries Tool::family. Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
✨ Finishing Touches📝 Generate docstrings
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The tinytools subproject reference has been updated to a new commit, which includes a dirty state indicating local modifications. This change aligns the vendor dependency with the latest upstream changes. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Updated the tinytools subproject to the latest commit, incorporating upstream fixes and improvements. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 6 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Changes requested Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Previously reported and still active
Resolved this pass
Before merge
How this fits togetherflowchart LR
n0["answer_tool_search<br/>changed"]:::changed
n1["tool_call_schema<br/>changed"]:::changed
n2["bridge_schemas_are_byte_stable_across_builds<br/>changed<br/>3 findings"]:::flagged
n3["DeferredCatalog<br/>changed"]:::changed
n4["catalog"]:::impacted
n5["bridge_schemas"]:::impacted
n6["tool_search_schema"]:::impacted
n7["...ol_search_clamps_limit_and_handles_misses"]:::impacted
n8["...search_does_not_panic_on_a_zero_max_limit"]:::impacted
n9["...rch_schema_normalizes_inconsistent_limits"]:::impacted
n0 -->|uses| n3
n2 -->|calls| n4
n2 -->|tests| n4
n2 -->|calls| n5
n2 -->|tests| n5
n4 -->|uses| n3
n5 -->|calls| n1
n5 -->|uses| n3
n5 -->|calls| n6
n6 -->|uses| n3
n7 -->|calls| n0
n7 -->|tests| n0
n7 -->|calls| n4
n7 -->|tests| n4
n8 -->|calls| n0
n8 -->|tests| n0
n8 -->|calls| n4
n8 -->|tests| n4
n9 -->|calls| n4
n9 -->|tests| n4
n9 -->|calls| n5
n9 -->|tests| n5
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae48b2c34f
ℹ️ 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".
| ) | ||
| .await; |
There was a problem hiding this comment.
When an optional hosted ranker performs network I/O and stalls, this await is outside with_call_budget; the only deadline check happened before admission began. Consequently, a run with a configured wall-clock limit can remain stuck in tool_search indefinitely instead of returning TinyAgentsError::Timeout, so the ranking future should be raced against the run's remaining budget like model, tool, authorization, and screening calls.
Useful? React with 👍 / 👎.
| Self { | ||
| schema, | ||
| family, | ||
| searchable, |
There was a problem hiding this comment.
When a tool is discoverable primarily by its family/server name, the default and fallback BM25 paths cannot find it: family is stored separately here but never appended to searchable, and the catalogue's Bm25Index is built only from searchable. Host rankers receive the separate family field, but searches without a host ranker—or searches falling back after a ranker failure—silently lose the newly documented family matching.
Useful? React with 👍 / 👎.
| Ok(hits) if !hits.is_empty() => RankedSearch { | ||
| top_confidence: hits.first().and_then(|hit| hit.confidence), | ||
| names: hits.into_iter().map(|hit| hit.key).collect(), | ||
| ranker: ranker.kind(), | ||
| fallback: None, |
There was a problem hiding this comment.
When a hosted ranker returns only stale or invented keys, this branch treats the response as successful because the raw hit list is nonempty; answer_tool_search subsequently drops every key through catalog.get and reports zero matches rather than using BM25. Since the code explicitly accepts that rankers may name unknown tools, validate hits against the catalogue before deciding that the hosted result is nonempty, otherwise deferred tools can become unreachable despite a valid BM25 match.
Useful? React with 👍 / 👎.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0415 · 824,691 in / 28,848 out · 101,174 cached (12%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 1,156 embedded
critique: $0.0205 · 382,749 in / 15,449 out · 23,403 cached (6%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0188 · 371,504 in / 10,558 out · 19,232 cached (5%) · gpt-5.6-luna
tests: $0.0006 · 30,473 in / 154 out · 26,507 cached (87%) · deepseek/deepseek-v4-flash
description: $0.0004 · 21,262 in / 96 out · 17,295 cached (81%) · deepseek/deepseek-v4-flash
| /// Ranks as `policy` says — the host ranker when one is active, BM25 | ||
| /// otherwise or on failure — and returns the full schema of every hit so | ||
| /// the model can call it. | ||
| pub async fn answer_tool_search( |
There was a problem hiding this comment.
Update callers for the asynchronous SearchAnswer API
This changes answer_tool_search from a synchronous function returning (ToolResult, usize) to an async function returning SearchAnswer. The existing caller in agent_loop/tools.rs is not part of this diff, so it still calls and destructures the old API; it cannot compile because the result is now a future with different fields. Update that caller to await the function and use answer.result, answer.matched, and answer.ranking, or preserve the old API through a synchronous wrapper.
Additional security observation
Update callers for the asynchronous search function
[RULE] build-break
answer_tool_search was previously synchronous, and this diff changes it to return a future. The complete diff contains no corresponding caller updates, so existing calls from the agent-loop tools path will fail to compile until they await this function and handle the new SearchAnswer return value.
[RULE] compile-break ·
| query: String, | ||
| /// Number of deferred tools returned. | ||
| matched: usize, | ||
| /// Which ranker's answer was served: `"bm25"`, or the host ranker's |
There was a problem hiding this comment.
Update every ToolSearch constructor for the new fields
These fields are required when constructing the AgentEvent::ToolSearch struct variant in Rust. #[serde(default)] only affects deserialization; it does not supply values to existing AgentEvent::ToolSearch { query, matched } expressions, so the event emission paths that construct this variant will fail to compile until they provide ranker, top_confidence, fallback, shadow_matched, and latency_ms (or the variant is given a compatible construction API).
[RULE] breaking-enum-constructor ·
| } | ||
|
|
||
| #[test] | ||
| fn a_ranker_hit_naming_an_unknown_tool_is_dropped_from_the_answer() { |
There was a problem hiding this comment.
Exercise answer_tool_search when testing unknown ranker hits
This test never installs a ranker or calls answer_tool_search; it only verifies that the fixture catalog does not contain invented. An implementation that includes unknown ranker keys in the returned answer would still pass, so the stated filtering guarantee is untested. Invoke answer_tool_search with a ranker returning invented and assert that the result contains no such match.
[RULE] insufficient-test-coverage ·
| let shadow_names = (policy.rank_mode == DiscoveryRankMode::Compare) | ||
| .then(|| self.search_names(query, limit)); | ||
| match hosted { | ||
| Ok(hits) if !hits.is_empty() => RankedSearch { |
There was a problem hiding this comment.
Enforce the requested limit on ranker results
DeferredCatalog::rank promises to return at most limit hits, but this branch collects every item returned by the host ranker. A ranker that ignores or exceeds the requested limit can therefore cause tool_search to return more tools than ToolDiscoveryPolicy::max_limit allows, defeating the policy's prompt-size and exposure bound. Truncate the accepted hits to limit before building names (and use the same bounded set when deriving confidence).
[RULE] bounded-output ·
| match hosted { | ||
| Ok(hits) if !hits.is_empty() => RankedSearch { | ||
| top_confidence: hits.first().and_then(|hit| hit.confidence), | ||
| names: hits.into_iter().map(|hit| hit.key).collect(), |
There was a problem hiding this comment.
Cap ranker results to the requested limit
The policy's limit is intended to bound how many deferred tools a model can retrieve, but this trusts the host ToolRanker to honor it. A ranker can return more hits than requested, causing the bridge to emit an oversized or duplicated discovery response and bypassing the configured prompt/output bound. Truncate and, ideally, validate ranker results in this layer before constructing RankedSearch.
[RULE] bounded-output ·
tinysweeper
Bot
added
the
priority: p0
…enDecide Resolves the ToolSearched conflict by keeping both sides: the query is recorded only under RunPolicy::capture.tool_io (main) and the event carries the ranker, confidence, fallback, shadow ranking and latency (this branch). Points vendor/tinytools at tinyhumansai/tinytools#19 (main + the family strategy) and bumps every tinytools pin to 0.4.1. Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5c2411b78
ℹ️ 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".
| .map(|r| r.ranker.to_string()) | ||
| .unwrap_or_default(), | ||
| top_confidence: ranking.as_ref().and_then(|r| r.top_confidence), | ||
| fallback: ranking.as_ref().and_then(|r| r.fallback.clone()), |
There was a problem hiding this comment.
When capture.tool_io is false (the default), a hosted ranker can still leak the search query or upstream response data through its RankError reason: describe_failure copies that arbitrary string into ranking.fallback, and this line emits it without applying the capture gate used for query. Rankers that include request content or response bodies in backend errors therefore expose potentially sensitive tenant data to event sinks despite payload capture being disabled; redact the failure detail or gate it on capture.tool_io.
Useful? React with 👍 / 👎.
| "thiserror", | ||
| "tinyinference-core", | ||
| "tinytools-agent", | ||
| "tinytools-agent 0.3.0", |
There was a problem hiding this comment.
After bumping the vendored dependency to 0.4.1, tinyinference-llm still requires 0.3.0, so the workspace patch no longer satisfies it: the updated lockfile now resolves tinytools-agent 0.3.0 and tinytools 0.3.0 from the Git URL in addition to the vendored 0.4.1 copies. Consequently a fresh offline build must fetch the old Git revision and the graph contains two protocol implementations, contradicting the one-copy assumption documented in the root Cargo.toml; update the tinyinference dependency/submodule in lockstep or otherwise make its requirement accept the vendored version.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0420 · 777,890 in / 32,825 out · 106,974 cached (14%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash, deepseek-v4-flash · 1,214 embedded
critique: $0.0212 · 385,511 in / 14,612 out · 51,134 cached (13%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0182 · 315,998 in / 10,573 out · 12,505 cached (4%) · gpt-5.6-luna
tests: $0.0007 · 32,469 in / 206 out · 26,523 cached (82%) · deepseek/deepseek-v4-flash
description: $0.0006 · 23,132 in / 4,578 out · 0 cached (0%) · deepseek-v4-flash
| /// Which ranker's answer was served: `"bm25"`, or the host ranker's | ||
| /// [`tinytools::ToolRanker::kind`]. Empty when the query was rejected | ||
| /// before ranking. | ||
| #[serde(default)] |
There was a problem hiding this comment.
Update every ToolSearch event constructor
Adding fields to a struct-like enum variant changes the required fields for every AgentEvent::ToolSearch { ... } literal. Existing constructors that only provide query and matched will fail to compile; #[serde(default)] only affects serialization and deserialization, not Rust construction. Update all constructors to populate these fields (or add a constructor that supplies defaults).
[RULE] compile-break ·
| .all(|c| c.family.as_deref() == Some("fam")) | ||
| ); | ||
| match &self.answer { | ||
| Ok(keys) => Ok(keys |
There was a problem hiding this comment.
Exercise rankers that exceed the requested limit
The scripted ranker truncates its own response with .take(limit), so the tests cannot detect whether answer_tool_search caps a host ranker's overlong result. A ranker is allowed to return more hits than requested; add a fixture that ignores limit and assert that the served answer and matched count remain within the requested bound.
[RULE] insufficient-test-coverage ·
| and top-level property names. Returns up to `limit` matches (default 5, max | ||
| 20) as `{name, description, parameters}` with the **full** schema, or a | ||
| "no match" note. Its description embeds a manifest of every deferred tool: | ||
| - `tool_search { query, limit }` — ranks the catalogue's name, split |
There was a problem hiding this comment.
Document only fields that BM25 actually indexes
The built-in BM25 path does not index Tool::family: DeferredTool::from_schema builds its searchable text from the name, split name, description, and property names, while family is stored separately on the rank candidate. As a result, a default BM25 search for a family-only term cannot match as this documentation promises. Either include the family in the indexed searchable text or remove it from the documented BM25 fields.
[RULE] documentation-contract ·
| "no match" note. Its description embeds a manifest of every deferred tool: | ||
| - `tool_search { query, limit }` — ranks the catalogue's name, split | ||
| identifier, description, top-level property names and `Tool::family`. | ||
| Returns up to `limit` matches (default 5, max 20) as `{name, description, |
There was a problem hiding this comment.
Enforce the requested limit on ranker results
The host-ranker path passes limit to the ranker but then converts every returned hit into RankedSearch::names without truncating the result. A ranker that returns more than the requested limit therefore causes tool_search to expose more than limit matches, contradicting this documented contract. Enforce the bound after receiving ranker hits as well as when invoking the ranker.
[RULE] bounded-output ·
| assert_eq!(ranker.calls(), 0); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Exercise answer_tool_search with unknown ranker hits
This test only proves that the fixture catalog does not contain invented; it never installs a ranker or invokes answer_tool_search. An implementation that accidentally includes unknown ranker keys in the returned answer would still pass. Use a scripted ranker returning invented, call answer_tool_search, and assert that the result contains no unknown match.
Additional critique observation
Exercise answer_tool_search with an unknown ranker hit
[RULE] insufficient-test-coverage
This only proves that the fixture catalog lacks invented; it never installs a ranker or calls answer_tool_search. An implementation that emits unknown ranker keys in the model-facing result would still pass. Make this an async test with a ranker returning invented, call answer_tool_search, and assert that the answer contains no unknown-tool match.
[RULE] insufficient-test-coverage ·
1 participant
Summary
Makes
tool::discover— the intrinsictool_search/tool_callbridge overToolExposure::Deferredregistrations — rank with a pluggabletinytools::ToolRanker, so a host can plug in a decision model (Jev viatinytools-jev) or an embedding index without touching the loop.ToolDiscoveryPolicygainsranker: Option<Arc<dyn ToolRanker>>andrank_mode: DiscoveryRankMode { Ranker, Bm25, Compare }.Compareserves the host ranker and carries the BM25 ranking alongside so the two can be judged on live traffic without changing what the model sees.DeferredCatalog::rank(async) serves the host ranker and falls back to BM25 on error or an empty answer — a search never fails, since a failed search would leave every deferred tool unreachable for the turn. The reason lands inRankedSearch::fallback. Every hit is resolved throughcatalog.get, so a key a ranker invented never reaches the model.answer_tool_searchis async and returns aSearchAnswer;AgentEvent::ToolSearchedgainsranker,top_confidence,fallback,shadow_matched,latency_ms(allserde(default), so persisted journals still decode).Tool::family(ToolRegistry::deferred_schemas_with_families) as ranking text.Bm25Index/tokenizemoved totinytools::rankand are re-exported from their old path.Depends on tinyhumansai/tinytools#17 (the
vendor/tinytoolsgitlink here points at that branch). Consumed by the openhuman PR that opts the orchestrator into discovery.Test plan
cargo test --workspace— 2,655 passed (new: scripted-ranker served with confidence, failure → BM25 with reason, empty answer → BM25, compare mode shadow,Bm25mode ignores the ranker, policyPartialEq/Debugby ranker kind)tests/tool_deferral.rsstill asserts thetoolsarray is byte-identical across a runcargo clippy --workspace --all-targets -- -D warningscleandocs/modules/harness/tool-discovery.md+ module README updatedCo-authored-by: Medulla medulla@tinyhumans.ai
Summary by CodeRabbit
New Features
Documentation