feat(loop-host): add schema-aware deferred tool search - #7273
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe change replaces legacy catalog keyword scoring with a bounded, authorization-scoped search index. It indexes verified tool metadata and schema vocabulary, caches authorized results, records privacy-preserving metrics, and adds corpus and integration coverage. ChangesAuthorized tool search
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant LoopCapabilityPort
participant AuthorizedToolSearchIndex
participant tool_describe
LoopCapabilityPort->>AuthorizedToolSearchIndex: search filtered authorized definitions
AuthorizedToolSearchIndex-->>LoopCapabilityPort: return ranked tool names and query class
LoopCapabilityPort->>LoopCapabilityPort: record rank and privacy-preserving metrics
LoopCapabilityPort->>tool_describe: describe the selected tool
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
🚅 Deployed to the ironclaw-pr-7273 environment in ironclaw-ci-preview
|
github-actions
Bot
added
size: XL
🔎 Review · PR #7273
GitHub request failed IronLoop could not complete a required GitHub request. Automatic · PR opened · attempt 1 of 3 · failed after 2m 15s Failure details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/loop/ironclaw_loop_host/src/tool_search.rs`:
- Around line 527-535: Update the test around AuthorizedToolSearchIndex::new so
it constructs an unfiltered index containing first, second, and denied, while
the authorized index contains only first and second. Assert the unfiltered
search for “assignee” produces the expected denied-stuffed ordering or count
change, then assert the authorized result remains ["allowed__first",
"allowed__second"] and excludes denied, proving the denied document is actually
isolated.
In `@tests/integration/tool_disclosure.rs`:
- Around line 467-509: Add a plain-English coverage row for the test function
tool_search_discovers_authorized_tools_by_parameter_only_vocabulary in
tests/integration/CLAUDE.md, following the existing row format and same-commit
documentation rules. Do not add a §4 binary row because the test belongs to the
existing integration test binary.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9e09dac6-4d64-4f55-a689-8d5f80923792
📒 Files selected for processing (6)
crates/loop/ironclaw_loop_host/src/lib.rscrates/loop/ironclaw_loop_host/src/tool_disclosure.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_port.rscrates/loop/ironclaw_loop_host/src/tool_search.rscrates/loop/ironclaw_loop_host/tests/fixtures/tool_search_relevance.jsontests/integration/tool_disclosure.rs
Railway preview QA — PASS
Given / When / Then evidence
Exact regression resultThe previously unavailable bridge is now active and directly exercised. A parameter-key-only query successfully retrieved the intended deferred capability through the production Deterministic coverage remains green on the same head: the committed corpus reports candidate recall@1/5/10 and MRR of Cleanup, skips, and remaining risk
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/loop/ironclaw_loop_host/tests/fixtures/tool_search_relevance.json (1)
331-383: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winThe corpus exercises the trust policy in one direction only.
google_calendar__create_eventis the only tool with"verified": true, and its"Repeated event rule"description at line 364 is the only nested schema description in the corpus. Every other tool deserializes asUntrustedand carries no nested description.The corpus can therefore confirm that verified prose is indexed. It cannot detect the inverse regression, where untrusted nested prose leaks into retrieval metadata. The unit test at
crates/loop/ironclaw_loop_host/src/tool_search.rs:429-465covers that direction, so this is a corpus-reinforcement gap, not an uncovered invariant.Consider adding one untrusted tool with a distinctive nested schema description, then adding a
no_matchintent for a term that appears only in that description. The existingno_match_accuracy == 1.0gate would then fail on a trust-policy regression.🤖 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 `@crates/loop/ironclaw_loop_host/tests/fixtures/tool_search_relevance.json` around lines 331 - 383, Add an untrusted tool fixture with a distinctive nested schema description, then add a corresponding no_match intent querying a term used only in that description. Ensure the tool remains deserialized as Untrusted and the term is absent from trusted metadata, allowing the existing no_match_accuracy == 1.0 assertion to detect untrusted nested-prose leakage.
🤖 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 `@crates/loop/ironclaw_loop_host/src/tool_search.rs`:
- Around line 855-868: Update the recall closure around recall_at_1/recall_at_5
so each query’s denominator is capped at the cutoff, using min(at,
intent.relevance.len()) instead of the full judged-set size; preserve the
existing hit-count filtering and aggregation, and retune the affected recall and
baseline-comparison thresholds to reflect the new normalization.
---
Outside diff comments:
In `@crates/loop/ironclaw_loop_host/tests/fixtures/tool_search_relevance.json`:
- Around line 331-383: Add an untrusted tool fixture with a distinctive nested
schema description, then add a corresponding no_match intent querying a term
used only in that description. Ensure the tool remains deserialized as Untrusted
and the term is absent from trusted metadata, allowing the existing
no_match_accuracy == 1.0 assertion to detect untrusted nested-prose leakage.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2add17f0-5c76-447c-8f76-5db8b5d812d9
📒 Files selected for processing (2)
crates/loop/ironclaw_loop_host/src/tool_search.rscrates/loop/ironclaw_loop_host/tests/fixtures/tool_search_relevance.json
Railway preview QA — PASS
Acceptance matrix
Status derivation
Regression resultThe exact-head preview directly exercised bridged Skipped cases and remaining risk
Cleanup
|
Railway preview QA — PASS
Acceptance matrix
Status derivation
Regression resultThe exact-head preview directly exercised bridged Skipped cases and remaining risk
Cleanup
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add schema-aware deferred tool search: replace name/description substring matching with a bounded, host-owned weighted lexical index over capability IDs, aliases, parameter keys, and trusted nested schema descriptions, with deterministic metadata invalidation, authorization-fitted retrieval, and a graded 50-tool/72-query relevance corpus.
Shape: normal primary mode; no modifiers. Selected because the PR targets main directly and is a single feature layer (not a stack). XL size (2,686 additions / 2,827 changed lines) drove complete packetization, not a mode change.
Coverage: complete. Diff source: local-git (exact base→head commit pair verified against PR snapshot; matches_supplied_files=true). File buckets: 4 production / 2 tests / 1 docs / 0 generated / 0 CI / 0 config. 4 diff packets, 0 oversized files, 0 failed reviewers. Limitations: none.
Stats: 14 findings (14 raw, 11 after overlap dedup) across 4 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: 0. Body-only: 0.
Verified in-worktree before posting: git status clean at head, tool_search.rs 989 lines matching the diff; reviewers' probe_tmp scratch reports were transient worktree state (created/removed by the review harness), not part of the PR, and were excluded.
Bugs
- Low PROVIDER_WEIGHT never takes effect: provider terms enter at NAME_WEIGHT via capability_id field (
crates/loop/ironclaw_loop_host/src/tool_search.rs:180-183, confidence 60) — anchor: crates/loop/ironclaw_loop_host/src/tool_search.rs:183
IndexedDocument::new adds full capability_id (e.g. 'github.list_issues') with NAME_WEIGHT=8.0 first, then dot-split provider prefix ('github') with PROVIDER_WEIGHT=4.0. add_field's entry().and_modify(max) keeps 8.0, so the provider field's 4.0 weight is always subsumed and PROVIDER_WEIGHT (and its tuning intent) is dead. Provider-name queries rank as strongly as exact name tokens; deterministic gates pass, but the declared weight scheme is not what runs.
Fix: Add provider field before capability_id, or index capability_id minus its provider segment at NAME_WEIGHT so the provider prefix can keep PROVIDER_WEIGHT.
Performance/Concurrency
- Medium Full-corpus deep schema fingerprint rehashed on every turn_state call under mutex (
crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs:574-586, confidence 62) (also flagged by: security/Medium) — anchor: crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs:582
turn_state() runs on every bridge capability call (tool_search, tool_describe, tool_call, promote_target, capability_info note, dispatch). Each call refetches definitions, filters, and recomputes definitions_fingerprint, which now deep-hashes every tool's full parameters JSON (hash_json recurses whole schema, unbounded size) plus sorts by capability_id (O(n log n)). Old fingerprint hashed only count + names. Rebuild is gated by the fingerprint, but the hash itself is recomputed unconditionally per call while holding the turn_state Mutex. For large MCP/extension tool surfaces (hundreds of tools x KB-sized schemas) this adds ms-scale per-message latency to the agent-loop hot path and lengthens mutex hold time.
Fix: Cache per-call fingerprint: memoize deep hash per capability_id once (schemas immutable within a turn), or two-tier check - cheap names/count fingerprint first, deep hash only when cheap tier changes.
Tests
-
Medium Equal-score deterministic tie-break by capability_id has no direct test (
crates/loop/ironclaw_loop_host/src/tool_search.rs:147-157, confidence 70) (also flagged by: performance/Low) — anchor: crates/loop/ironclaw_loop_host/src/tool_search.rs:156
Explicit constraint is deterministic ranking and tie-breaking. Sort uses score desc then capability_id asc (line 156). No test builds two documents with identical BM25 scores where capability_id order differs from name order, so the tie-break rule itself is unverified.
Fix: Add test: two tools with identical name/description/parameter vocabulary whose capability_id ordering differs from name ordering. -
Medium MAX_QUERY_TERMS (32) silent truncation of long queries untested (
crates/loop/ironclaw_loop_host/src/tool_search.rs:108-112, confidence 70) (also flagged by: bugs/Low) — anchor: crates/loop/ironclaw_loop_host/src/tool_search.rs:110
search() truncates tokenized queries with .take(MAX_QUERY_TERMS) (line 110). Bounded-input constraint is explicit; a >32-term query (reachable under the 1024-byte port cap) silently drops tail terms, but no test exercises the truncation boundary or its effect on ranking.
Fix: Add test covering a 33+-term query where only the first 32 terms determine results. -
Medium MAX_FIELD_BYTES / MAX_DOCUMENT_BYTES / MAX_UNIQUE_TERMS byte-limit paths untested (
crates/loop/ironclaw_loop_host/src/tool_search.rs:217-246, confidence 65) — anchor: crates/loop/ironclaw_loop_host/src/tool_search.rs:217
add_field truncates at 256-byte fields, 8192-byte documents, and 512 unique terms. The adversarial test only exercises depth and field-count caps; no test feeds a >256-byte description or parameter string or a corpus exceeding document/unique-term budgets, so the truncation branches (scan closure, admitted_bytes accounting) are unverified.
Fix: Add test covering >256-byte description field and >MAX_DOCUMENT_BYTES set of fields with no panic and bounded term_weights. -
Low Empty-corpus branch (average_length=1.0, NoMatch) untested (
crates/loop/ironclaw_loop_host/src/tool_search.rs:88-90, confidence 55) — anchor: crates/loop/ironclaw_loop_host/src/tool_search.rs:88
new() special-cases documents.is_empty() to average_length 1.0 and search on an empty index returns NoMatch; every existing test builds 1-3 documents or the 50-tool corpus. Low risk because the port rejects an unavailable catalog, but the branch is dead-code-adjacent with no guard.
Fix: Add test: AuthorizedToolSearchIndex::new(empty) returning NoMatch for any query. -
Low CamelCase token-boundary splitting in tokenize untested (
crates/loop/ironclaw_loop_host/src/tool_search.rs:313-335, confidence 55) — anchor: crates/loop/ironclaw_loop_host/src/tool_search.rs:313
tokenize inserts a space before an uppercase char following a lowercase/digit (camelCase splitting). No fixture or test uses camelCase identifiers (corpus uses snake_case names/params), so this branch - which affects indexing of mixed-case capability descriptions - has zero direct coverage. All-caps words and boundary state also unverified.
Fix: Add test covering createEvent and HTTPUrl tokenizing to create/event and http/url. -
Low Fingerprint stability across shuffled multi-definition arrays untested (
crates/loop/ironclaw_loop_host/src/tool_search.rs:339-370, confidence 55) — anchor: crates/loop/ironclaw_loop_host/src/tool_search.rs:339
definitions_fingerprint sorts by capability_id before hashing, but the only test uses single-definition slices, which cannot detect an ordering bug in multi-definition input. Rebuild correctness depends on fingerprint stability for the same effective corpus in any order.
Fix: Add test: two identical multi-definition arrays in different orders yielding the same u64. -
Low Same-turn rebuild preserving search_ranks across mid-turn fingerprint change untested (
crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs:592-631, confidence 55) — anchor: crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs:610
turn_state rebuild now filters to authorized definitions, widens the fingerprint to schema metadata, and preserves search_ranks across same-turn rebuilds. No unit or integration test exercises a mid-turn fingerprint change (e.g. schema-only mutation or extension activation) to verify ranks/disclosed_names survive and the index rebuilds.
Fix: Add test covering a fingerprint change between two turn_state calls in one turn.
Local Patterns
- Low Stale test name references removed tool_search_rank function (
crates/loop/ironclaw_loop_host/src/tool_disclosure.rs:1889-1895, confidence 65) — anchor: crates/loop/ironclaw_loop_host/src/tool_disclosure.rs:1890
tool_search_rank removed from tool_disclosure.rs, replaced by AuthorizedToolSearchIndex::search in tool_search.rs. Test name tool_search_rank_scores_deterministically is now the only grep hit for the deleted symbol. Readers searching ranking logic land on a test named after a nonexistent function; name drift after rename hurts navigation.
Fix: Rename test to describe current API, e.g. tool_search_index_scores_deterministically.
Maintainability
- Low Four near-identical bounded schema-traversal loops in collect_schema (
crates/loop/ironclaw_loop_host/src/tool_search.rs:272-310, confidence 65) — anchor: crates/loop/ironclaw_loop_host/src/tool_search.rs:272
The items (272), anyOf/oneOf/allOf (285), additionalProperties (295), and $defs/definitions (300) branches each repeat the same shape: grab children, .take(MAX_SCHEMA_NODES), guard self.schema_nodes >= MAX_SCHEMA_NODES, recurse with depth.saturating_add(1). The node-cap bookkeeping is copy-pasted five times (properties branch included). Any future change to traversal bounds or cap semantics must be applied in five places or the branches silently diverge.
Fix: Extract one helper owning the shared node-cap guard and recursion; the four keyword branches collapse into three-line calls.
All findings are Low/Medium — no blocking defects. The strongest item is the per-call full-schema fingerprint re-hash (performance + security concur), which contradicts the PR's own 'bounded traversal and inputs' constraint on the hot path.
| let definitions = self.inner.tool_definitions()?; | ||
| let fingerprint = definitions_fingerprint(&definitions); | ||
| // Fit and cache retrieval only over the effective authorized corpus. | ||
| // Denied schemas therefore cannot affect IDF, ordering, counts, cache |
There was a problem hiding this comment.
Medium — Full-corpus deep schema fingerprint rehashed on every turn_state call under mutex.
turn_state() runs on every bridge capability call (tool_search, tool_describe, tool_call, promote_target, capability_info note, dispatch). Each call refetches definitions, filters, and recomputes definitions_fingerprint, which now deep-hashes every tool's full parameters JSON (hash_json recurses whole schema, unbounded size) plus sorts by capability_id (O(n log n)). Old fingerprint hashed only count + names. Rebuild is gated by the fingerprint, but the hash itself is recomputed unconditionally per call while holding the turn_state Mutex. For large MCP/extension tool surfaces (hundreds of tools x KB-sized schemas) this adds ms-scale per-message latency to the agent-loop hot path and lengthens mutex hold time.
Fix: Cache per-call fingerprint: memoize deep hash per capability_id once (schemas immutable within a turn), or two-tier check - cheap names/count fingerprint first, deep hash only when cheap tier changes.
Also flagged by: security/Medium
| let capability_id = definition.capability_id.as_str(); | ||
| builder.add_field(capability_id, NAME_WEIGHT); | ||
| builder.add_field(definition.name.as_str(), NAME_WEIGHT); | ||
| if let Some(provider) = capability_id.split('.').next() { |
There was a problem hiding this comment.
Low — PROVIDER_WEIGHT never takes effect: provider terms enter at NAME_WEIGHT via capability_id field.
IndexedDocument::new adds full capability_id (e.g. 'github.list_issues') with NAME_WEIGHT=8.0 first, then dot-split provider prefix ('github') with PROVIDER_WEIGHT=4.0. add_field's entry().and_modify(max) keeps 8.0, so the provider field's 4.0 weight is always subsumed and PROVIDER_WEIGHT (and its tuning intent) is dead. Provider-name queries rank as strongly as exact name tokens; deterministic gates pass, but the declared weight scheme is not what runs.
Fix: Add provider field before capability_id, or index capability_id minus its provider segment at NAME_WEIGHT so the provider prefix can keep PROVIDER_WEIGHT.
| right | ||
| .2 | ||
| .total_cmp(&left.2) | ||
| .then_with(|| left.1.cmp(&right.1)) |
There was a problem hiding this comment.
Medium — Equal-score deterministic tie-break by capability_id has no direct test.
Explicit constraint is deterministic ranking and tie-breaking. Sort uses score desc then capability_id asc (line 156). No test builds two documents with identical BM25 scores where capability_id order differs from name order, so the tie-break rule itself is unverified.
Fix: Add test: two tools with identical name/description/parameter vocabulary whose capability_id ordering differs from name ordering.
Also flagged by: performance/Low
| let normalized_query = query.trim().to_lowercase(); | ||
| let query_terms: Vec<String> = tokenize(&normalized_query) | ||
| .into_iter() | ||
| .take(MAX_QUERY_TERMS) |
There was a problem hiding this comment.
Medium — MAX_QUERY_TERMS (32) silent truncation of long queries untested.
search() truncates tokenized queries with .take(MAX_QUERY_TERMS) (line 110). Bounded-input constraint is explicit; a >32-term query (reachable under the 1024-byte port cap) silently drops tail terms, but no test exercises the truncation boundary or its effect on ranking.
Fix: Add test covering a 33+-term query where only the first 32 terms determine results.
Also flagged by: bugs/Low
| } | ||
|
|
||
| impl SearchDocumentBuilder { | ||
| fn add_field(&mut self, value: &str, weight: f64) { |
There was a problem hiding this comment.
Medium — MAX_FIELD_BYTES / MAX_DOCUMENT_BYTES / MAX_UNIQUE_TERMS byte-limit paths untested.
add_field truncates at 256-byte fields, 8192-byte documents, and 512 unique terms. The adversarial test only exercises depth and field-count caps; no test feeds a >256-byte description or parameter string or a corpus exceeding document/unique-term budgets, so the truncation branches (scan closure, admitted_bytes accounting) are unverified.
Fix: Add test covering >256-byte description field and >MAX_DOCUMENT_BYTES set of fields with no panic and bounded term_weights.
| } | ||
| } | ||
|
|
||
| fn tokenize(value: &str) -> BTreeSet<String> { |
There was a problem hiding this comment.
Low — CamelCase token-boundary splitting in tokenize untested.
tokenize inserts a space before an uppercase char following a lowercase/digit (camelCase splitting). No fixture or test uses camelCase identifiers (corpus uses snake_case names/params), so this branch - which affects indexing of mixed-case capability descriptions - has zero direct coverage. All-caps words and boundary state also unverified.
Fix: Add test covering createEvent and HTTPUrl tokenizing to create/event and http/url.
|
|
||
| /// Stable for a fixed ranker version and effective authorized metadata. Object | ||
| /// keys are sorted recursively so semantically identical schemas share a key. | ||
| pub(crate) fn definitions_fingerprint(definitions: &[ProviderToolDefinition]) -> u64 { |
There was a problem hiding this comment.
Low — Fingerprint stability across shuffled multi-definition arrays untested.
definitions_fingerprint sorts by capability_id before hashing, but the only test uses single-definition slices, which cannot detect an ordering bug in multi-definition input. Rebuild correctness depends on fingerprint stability for the same effective corpus in any order.
Fix: Add test: two identical multi-definition arrays in different orders yielding the same u64.
| .unwrap_or(true); | ||
| if rebuild { | ||
| let catalog = CapabilityCatalog::new(&definitions, &[]); | ||
| let index_started_at = std::time::Instant::now(); |
There was a problem hiding this comment.
Low — Same-turn rebuild preserving search_ranks across mid-turn fingerprint change untested.
turn_state rebuild now filters to authorized definitions, widens the fingerprint to schema metadata, and preserves search_ranks across same-turn rebuilds. No unit or integration test exercises a mid-turn fingerprint change (e.g. schema-only mutation or extension activation) to verify ranks/disclosed_names survive and the index rebuilds.
Fix: Add test covering a fingerprint change between two turn_state calls in one turn.
| @@ -1971,7 +1888,7 @@ mod tests { | |||
|
|
|||
| #[test] | |||
| fn tool_search_rank_scores_deterministically() { | |||
There was a problem hiding this comment.
Low — Stale test name references removed tool_search_rank function.
tool_search_rank removed from tool_disclosure.rs, replaced by AuthorizedToolSearchIndex::search in tool_search.rs. Test name tool_search_rank_scores_deterministically is now the only grep hit for the deleted symbol. Readers searching ranking logic land on a test named after a nonexistent function; name drift after rename hurts navigation.
Fix: Rename test to describe current API, e.g. tool_search_index_scores_deterministically.
| self.collect_schema(schema, depth.saturating_add(1), trusted_descriptions); | ||
| } | ||
| } | ||
| if let Some(items) = object.get("items") { |
There was a problem hiding this comment.
Low — Four near-identical bounded schema-traversal loops in collect_schema.
The items (272), anyOf/oneOf/allOf (285), additionalProperties (295), and $defs/definitions (300) branches each repeat the same shape: grab children, .take(MAX_SCHEMA_NODES), guard self.schema_nodes >= MAX_SCHEMA_NODES, recurse with depth.saturating_add(1). The node-cap bookkeeping is copy-pasted five times (properties branch included). Any future change to traversal bounds or cap semantics must be applied in five places or the branches silently diverge.
Fix: Extract one helper owning the shared node-cap guard and recursion; the four keyword branches collapse into three-line calls.
|
Addressed all comments from review
Validation: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs`:
- Around line 1494-1551: Add #[cfg(any(test, feature = "test-support"))]
directly to the MutableDefinitionsPort test-helper implementation items
containing the lock().expect() calls, including its LoopCapabilityPort
implementation and any associated helper methods or type if needed. Ensure
scripts/check_no_panics.py recognizes these items as test-only without relying
on parent-module gating.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b91e93bf-9631-48cc-949c-da16f85441a5
📒 Files selected for processing (3)
crates/loop/ironclaw_loop_host/src/tool_disclosure.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_port.rscrates/loop/ironclaw_loop_host/src/tool_search.rs
|
Addressed the latest CodeRabbit comment in e9b6fe5.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs`:
- Around line 602-607: The tool disclosure filtering around tool_definitions
must apply the complete CapabilitySurfacePolicy, not only permits_capability_id.
Before computing definitions_fingerprint and constructing the disclosure catalog
or search index, filter definitions using allowed_runtimes, allowed_effects,
include_requires_approval, and max_capabilities consistently with
effective_entries and permits, reusing the existing policy semantics.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 26dc2034-33bd-417f-a0e1-1dbecbdc389a
📒 Files selected for processing (5)
crates/loop/ironclaw_loop_host/src/lib.rscrates/loop/ironclaw_loop_host/src/tool_disclosure.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_port.rstests/CLAUDE.mdtests/integration/tool_disclosure.rs
|
@ironloopai review |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Manual command by think-in-universe · attempt 1 of 3 · completed in 6m 41s IronLoop completed the review and posted it to GitHub. 🔗 Result |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 IronLoop review
Reviewed the complete merge-base-to-head comparison. Found one authorization-surface defect in the new schema-aware index.
Findings: 🟠 Medium 1
🟠 Medium · Fit the index to the complete capability surface policy
Inline on crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs:605. See the inline comment for details.
Validation
- ✅ Complete diff inspection — Inspected all seven changed files and traced the affected disclosure, policy-filter, runner-composition, cache, schema-trust, and integration-test paths.
- ✅ Patch integrity — `git diff --check refs/ironloop/merge-base refs/ironloop/head` completed successfully.
- ⚪ Targeted loop-host tests — Not run. The targeted Cargo test began downloading and compiling the locked workspace dependencies but did not reach test execution within the review window; it was interrupted during compilation.
Review details
- Run:
6b7aac39-0665-43a1-b4af-3d8440f133cd - Workflow: Review
- Attempts: 1
| let fingerprint = definitions_fingerprint(&definitions); | ||
| let authorized_definitions: Vec<_> = definitions | ||
| .into_iter() | ||
| .filter(|definition| self.policy.permits_capability_id(&definition.capability_id)) |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Fit the index to the complete capability surface policy
`authorized_definitions` applies only `capability_ids`, although `CapabilitySurfacePolicy` can also exclude tools by runtime, declared effects, approval requirement, and `max_capabilities`. The inner policy decorator likewise narrows `tool_definitions()` only by ID; its complete constraints apply later to visible descriptors. Consequently, a profile that permits an ID but excludes its runtime/effects/approval status—or truncates it via the count limit—still adds that tool’s names, parameters, and trusted nested schema descriptions to this index. A model can discover and receive metadata for a tool outside its effective visible surface, and excluded documents also alter IDF/ranking. Build the corpus from IDs surviving the complete effective surface policy, preserving its registry-order limit, before fingerprinting or indexing it.
* feat(loop-host): add schema-aware deferred tool search * test(loop-host): strengthen tool search isolation coverage * test(loop-host): expand tool search relevance benchmark * docs(loop-host): clarify retrieval recall semantics * fix(loop-host): address serrrfirat review — harden tool search bounds (nearai#7273) * fix(loop-host): address coderabbit review — gate test fixture (nearai#7273) * fix(loop-host): address coderabbit review — honor full surface policy (nearai#7273)
This branch was successfully deployed
3 participants
Summary
Retrieval Benchmark Findings
The committed benchmark now contains 50 representative deferred-tool schemas and 72 hand-judged queries across exact-name, canonical-ID, alias, parameter, nested-schema, provider, ambiguous, hard-negative, and no-match classes. Relevance is graded from 1 (acceptable) to 3 (primary match).
The largest gain comes from vocabulary that the legacy ranker cannot see: canonical capability IDs, provider aliases, parameter names, and bounded nested schema fields. Exact-name, canonical-ID, parameter, and provider classes each reached Recall@5 and nDCG@10 of 1.0. Hard-negative queries reached 0.938 Recall@5 and 0.959 nDCG@10; nested queries reached 1.0 Recall@5 and 0.877 nDCG@10.
Alias and ambiguous queries remain the weakest classes: aliases reached 0.819 Recall@5 / 0.865 nDCG@10, while ambiguous queries reached 0.889 / 0.845. The diagnostic report makes the five worst queries visible on every
--nocapturerun. Examples includelook up mail, where a lexical ranker does not infer that mail means Gmail, andsearch documents, where many tools share generic search vocabulary. These are useful limits of the current deterministic lexical design, not hidden passing cases; semantic synonym retrieval is intentionally out of scope for this PR.Recall uses the conventional full judged-set denominator. Consequently, Recall@1 is intentionally below 1.0 for queries with multiple relevant tools even when the first result is ideal. nDCG@10 is the primary graded-ordering gate, while Recall@5/10 protects retrieval breadth. The candidate must also improve nDCG@10 over the baseline by at least 0.15, every non-empty query class has its own gate, no-match accuracy must remain 1.0, and fixture validation prevents unknown tool labels, invalid grades, or corpus shrinkage below 50 tools / 60 queries.
The test prints index/query timings for diagnostics only. Machine-dependent latency thresholds and 500+ tool profiling remain follow-up work; no absolute timing gate was added to CI.
Change Type
Linked Issue
Closes #7177
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo build(covered by the crate and integration test builds below)cargo test -p ironclaw_loop_host;cargo test -p ironclaw_integration_tests --test reborn_integration_tool_disclosurecargo test --features integrationif database-backed or integration behavior changed (the current focused Reborn integration package above is the owning integration target; no database behavior changed)review-prorpr-shepherd --fixwas run before requesting review (not run; CodeRabbit review completed and both actionable comments were addressed)Test Strategy
User behavior: A model can discover a deferred tool using conceptual, provider, canonical capability, alias, or argument/schema vocabulary without loading full schemas into its prompt.
Risk areas:
Tests added or updated:
committer; a runtime-excluded full policy proves host-runtime tools and disclosure bridges are absent from the model request; existing narrowed-ID regressions also pass.tool_searchwith parameter-only vocabulary, verified the sole successful activity and rendered result, then passed refresh/read-back and console-health checks. New-head preview evidence is pending.cargo test -p ironclaw_loop_host(626 unit tests plus crate integration/doc targets) and all 29 Reborn tool-disclosure integration scenarios pass.What the tests prove: Across 50 tools and 72 graded queries, the candidate reaches recall@1/5/10 = 0.786/0.948/0.966, MRR = 0.949, nDCG@10 = 0.944, and no-match accuracy = 1.0 (baseline: 0.521/0.685/0.695, MRR 0.684, nDCG@10 0.682, no-match 1.0). Every non-empty query class clears its recall@5 and nDCG@10 gate; denied schemas are excluded before fitting; traversal and inputs are bounded; existing disclosure, describe, invoke, narrowing, and prompt-shape paths remain green.
Commands run:
cargo fmt --all -- --checkcargo check -p ironclaw_loop_host --testscargo clippy --all --benches --tests --examples --all-features -- -D warningsbash scripts/pre-commit-safety.shcargo test -p ironclaw_loop_hostcargo test -p ironclaw_integration_tests --test reborn_integration_tool_disclosurecargo test -p ironclaw_loop_host committed_corpus_quality_gate_and_benchmark_report -- --nocaptureSecurity Impact
Tool-search authorization is strengthened: definitions are admitted only from the complete policy-qualified visible surface before catalog/index construction, document-frequency fitting, fingerprinting, or ranking. Pre-surface access fails closed instead of rebuilding from incomplete definition metadata. Untrusted nested schema descriptions are excluded; schema traversal, query size, field count, node count, document bytes, and unique terms are bounded. Telemetry records only aggregate counts/classes/latencies/fingerprints/ranks and never raw query text, schemas, tool identity, or arguments. No network, secret, filesystem, sandbox, or execution-policy behavior changes.
Reborn Trust-Boundary Checklist
CapabilitySurfacePolicyvisible surface.rgand compiler coverage found no match sites to update.serde(default)fields fail closed or have migration tests. N/A: no persisted or security-bearing serde fields changed.Transient,Permanent,Misconfigured,PolicyDeniedor equivalent). Overlong/empty queries remain recoverable invalid-input outcomes; no host error class changed.AuthorizedToolSearchIndexis host-owned, crate-private, and explicitly authorization-fitted.Database Impact
None.
Blast Radius
Deferred discovery ranking and its per-turn catalog cache in
ironclaw_loop_host. Existing direct-tool, describe-first, promotion, narrowed allow-set, and dispatch behavior is unchanged and covered by the full crate and focused integration suites. Main regression risks are ranking drift, overbroad schema ingestion, and stale metadata; the committed corpus, trust/bounds tests, and full-metadata fingerprint cover those paths.Rollback Plan
Revert this commit to restore the prior name/description ranker. No schema, persistence, configuration, dependency, or wire-format migration is involved, so rollback is immediate and data-free.
Review Follow-Through
Prior-head Railway evidence is attached; new-head CI preview evidence is pending. Reviewer judgment is welcome on the initial field weights and corpus representativeness; both are deterministic and isolated in the host index.
Review track: B (feature)