feat(tool-search): complete fair discovery and benchmark arms - #7410
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughTool disclosure now supports namespace, compact, signature, and bridged modes. Search results can include bounded complete schemas. Profile-specific capability pins flow through runtime policy. Gateway tests cover authorized deferred discovery and invocation. A live benchmark measures discovery behavior across catalog sizes. ChangesTool disclosure
Estimated code review effort: 5 (Critical) | ~120 minutes 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-7410 environment in ironclaw-ci-preview
|
github-actions
Bot
added
risk: low
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Automatic trigger · attempt 1 of 3 · completed in 11m 28s 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
Found 1 medium-severity correctness issue.
Findings: 🟠 Medium 1
🟠 Medium · Reserve space for the tool-result envelope
Inline on crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs:37. See the inline comment for details.
Validation
- ✅ Bounded-signature unit tests — All 3 focused bounded-signature tests passed.
- ⚪ Model-visible response path — Not run. Static tracing was sufficient to establish the preview-cap interaction; no separate execution was needed.
Review details
- Run:
24e31143-583c-4b6f-b382-8e3b6a400362 - Workflow: Review
- Attempts: 1
|
|
||
| /// Maximum canonical JSON bytes devoted to complete input signatures in one | ||
| /// `tool_search` response. Compact result metadata is always returned. | ||
| const MAX_SEARCH_SIGNATURE_BYTES_TOTAL: usize = 24 * 1024; |
There was a problem hiding this comment.
🔍 IronLoop review · Inline finding
🟠 Medium · Reserve space for the tool-result envelope
The 24 KiB cap counts only `parameters`, while the serialized search result also contains the query, result array, names, descriptions, required fields, and JSON syntax. The production result path exposes only the first 24 KiB inline to the model, so a response whose included schemas reach this cap is necessarily truncated. Those entries still report `schema_complete: true`, even though the model receives an incomplete JSON preview and must page it through a result read. Budget the full serialized response (or reserve envelope space) before marking schemas complete.
There was a problem hiding this comment.
Addressed in 4688d2e369: tool_search now budgets the complete serialized JSON response before marking a schema complete, so complete signatures fit the model-visible inline result. Verification: 676 loop-host tests and strict Clippy passed.
ffbe9e0 to
8805277
Compare
github-actions
Bot
added
scope: docs
serrrfirat
changed the title
github-actions
Bot
added
size: XL
serrrfirat
changed the title
![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_disclosure_port.rs`:
- Around line 1114-1132: Update the search-result handling around search_ranks,
disclosed_names, and ranked_results so names are inserted into disclosed_names
only when the corresponding result has schema_complete: true; leave compact and
bounded incomplete results undisclosed so tool_describe can recover their
schemas. Add a caller-path regression covering an oversized-schema search
followed by an invalid target call, asserting should_describe_first returns the
schema rather than dispatching the target.
In `@crates/loop/ironclaw_turn_runner/src/runtime.rs`:
- Around line 147-176: The tool disclosure profile-pin configuration currently
suppresses parse and capability-ID errors; make it fallible and reject invalid
values during runtime construction. Update tool_disclosure_profile_pins_from_env
and its caller to propagate errors with contextual mapping via ?, while
retaining an unset REBORN_TOOL_DISCLOSURE_PROFILE_PINS_ENV as an empty default.
Preserve underlying serde and CapabilityId errors, and add a caller-path test
covering malformed JSON and invalid capability IDs.
🪄 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: e1663171-f9bc-4b96-9dff-aaae382dd5d5
📒 Files selected for processing (15)
crates/app/ironclaw_composition/src/runtime.rscrates/loop/ironclaw_loop_host/prompts/tool_disclosure_protocol.mdcrates/loop/ironclaw_loop_host/src/system_prompt_assets.rscrates/loop/ironclaw_loop_host/src/tool_disclosure.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_mode.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_scale_baseline.jsoncrates/loop/ironclaw_turn_runner/src/runtime.rsdocs/internal/tool-discovery-evaluation.mddocs/internal/tool-discovery-subjective-decisions.mdtests/integration/support/builder.rstests/integration/support/group.rstests/integration/support/group_options.rstests/integration/tool_disclosure.rs
Railway QA — exact head
|
| Case | Result | Observable evidence |
|---|---|---|
| Gateway authentication and app shell | PASS | Connected to the preview and opened a new conversation. |
| Passive namespace awareness | PASS | Without calling a tool, the model read builtin (22) and ironclaw (2) from the visible tool_search description: 24 deferred tools across 2 authorized namespaces. |
| Complete-signature direct invocation | PASS | tool_search returned builtin__trace_commons__status with schema_complete=true; the recorded activity contained exactly two successful tools, tool_search then status, with no tool_describe. The UI reported 14 seconds. |
| Read-only result | PASS | status returned runtime enrollment/auth-mode state; no write operation was performed. |
| No-match failure path | PASS | Searching lunar_unicorn_tax_filing_v99 returned {"results":[]}. Recorded activity contained exactly one tool_search; no substitute tool, describe, call, or write followed. |
| Browser console | PASS | No error-level console entries during the QA session. |
This validates the deployed default bridged arm and the eliminated describe round trip on a real model run. It is not the full quantitative benchmark: Railway deployed one configured arm and a 24-tool catalog. The five-arm cold/warm 100/500/1,000 matrix remains required before closing #7405; token and latency fields will be populated from those provider runs rather than estimated locally.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/tests/llm_gateway.rs`:
- Around line 737-803: Add regression tests covering the remaining
permits_policy_checked_call branches: a wrapped tool_call whose
provider-supplied arguments["name"] targets the requested canonical capability,
and an unrelated substitute capability that must be suppressed with
UNAVAILABLE_CAPABILITY_REPLY. Place these alongside the existing gateway
deferred-capability tests, asserting the authorized wrapper reaches capability
execution and the unrelated call is not emitted.
In `@scripts/tool_discovery_benchmark/README.md`:
- Around line 9-13: Update the README quickstart command block to state that a
live model credential is required and add an example export for NEARAI_API_KEY
before invoking run_benchmark.py; mention that LIVE_OPENAI_COMPATIBLE_API_KEY is
also accepted.
In `@scripts/tool_discovery_benchmark/run_benchmark.py`:
- Around line 148-173: The generated description namespace in the
tool-construction loop does not match the tool’s eventual bucket namespace.
Update the description generation around the tool append and bucket assignment
so its namespace is derived from the same bucket index used by
canonical_capability_id, or remove the namespace token from the description;
preserve consistent namespace-aware benchmark data.
- Around line 589-598: Update the summary construction around the git HEAD
lookup to use a checked subprocess API instead of os.popen, ensuring the command
failure raises or otherwise aborts rather than recording an empty head. Keep the
resolved commit string in summary["head"], and avoid blocking synchronous
process execution within the async benchmark flow by using the appropriate
asynchronous subprocess mechanism.
🪄 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: 1ca0015f-d995-4f10-9afb-f100a7bb0c6f
📒 Files selected for processing (6)
crates/loop/ironclaw_loop_host/src/model_gateway.rscrates/loop/ironclaw_loop_host/tests/llm_gateway.rsdocs/internal/tool-discovery-subjective-decisions.mdscripts/tool_discovery_benchmark/README.mdscripts/tool_discovery_benchmark/run_benchmark.pyscripts/tool_discovery_benchmark/test_run_benchmark.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/CLAUDE.md (2)
190-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the complete-search invocation contract.
The row lists the exposed tools but omits the changed result semantics. Complete
tool_searchresults can calltool_callwithouttool_describe. Incomplete or ambiguous results retain thetool_describefallback. Add both user-visible paths to the coverage sentence. Remove any matching §7 gap row if this row closes it.Based on learnings, coverage rows must describe user-visible behavior, and a closed §7 gap must be removed when its coverage row is added.
🤖 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 `@tests/CLAUDE.md` at line 190, Update the tool_disclosure.rs coverage row in tests/CLAUDE.md to document both complete-search results calling tool_call directly and incomplete or ambiguous results falling back to tool_describe. Remove the corresponding §7 gap row if it tracks this behavior, leaving only unresolved gaps.Source: Learnings
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDisambiguate the two Python coverage totals.
If Section 2 counts only active or registered Reborn coverage, label that scope at Line 67. Otherwise, reconcile it with Line 313. The file currently presents
102files and869test functions alongside103files and1,141tests. Section 6 explicitly calls the latter exhaustive, but Section 2 calls its value a total.Also applies to: 313-313
🤖 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 `@tests/CLAUDE.md` at line 67, Clarify the scope of the Python coverage figures in Section 2 near the `102` files and `869` test functions entry: label them as active or registered Reborn coverage if that is what they represent; otherwise reconcile them with the exhaustive `103` files and `1,141` tests totals in Section 6. Ensure the wording no longer presents both different figures as unqualified totals.scripts/ci/reborn_pr_test_plan.py (2)
764-777: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not let nextest widening bypass unknown-path validation. The early return can accept a mixed diff that contains an unclassified path and still produce a full Reborn plan.
scripts/ci/reborn_pr_test_plan.py#L764-L777: defer_full_plan(...)until all paths are classified, while retaining fail-closed errors for truly unclassified paths.scripts/ci/test_reborn_pr_test_plan.py#L920-L937: add a mixed-diff regression with.config/nextest.tomland an unclassifiedscripts/**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 `@scripts/ci/reborn_pr_test_plan.py` around lines 764 - 777, The nextest configuration branch in scripts/ci/reborn_pr_test_plan.py:764-777 must not return before validating every changed path; defer _full_plan(...) until classification completes, preserving fail-closed errors for unclassified paths. Add the mixed-diff regression in scripts/ci/test_reborn_pr_test_plan.py:920-937 covering .config/nextest.toml together with an unclassified scripts/** path.
343-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for
scripts/e2e-skill-self-creation.sh.
PR_STATIC_CONTROL_PATHScontains this path, butRebornPrTestPlanTests.test_decided_repo_root_paths_are_owned_by_other_workflowsdoes not assert that a change to it producesmode: noneandcrate_buckets: []. Add a regression in that table to match the new static-control entry.🤖 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/ci/reborn_pr_test_plan.py` around lines 343 - 354, Add a regression case to the table used by RebornPrTestPlanTests.test_decided_repo_root_paths_are_owned_by_other_workflows for scripts/e2e-skill-self-creation.sh, asserting that the planned result is mode: none with crate_buckets: []. Match the existing cases for paths in PR_STATIC_CONTROL_PATHS.scripts/ci/test_reborn_pr_test_plan.py (1)
920-937: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover mixed diffs in this regression.
This test exercises
.config/nextest.tomlalone. It cannot detect the early return skipping a second unclassified path. Add a case with.config/nextest.tomlandscripts/some-undecided-helper.sh, then assert the fail-closed error.🤖 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/ci/test_reborn_pr_test_plan.py` around lines 920 - 937, Extend test_nextest_config_widens_to_exhaustive_plan with a mixed changed-file set containing .config/nextest.toml and an undecided helper path such as scripts/some-undecided-helper.sh. Assert that plan raises the fail-closed unclassified pull-request path error instead of returning the exhaustive plan, covering processing of subsequent paths after the known configuration change.
🤖 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.
Outside diff comments:
In `@scripts/ci/reborn_pr_test_plan.py`:
- Around line 764-777: The nextest configuration branch in
scripts/ci/reborn_pr_test_plan.py:764-777 must not return before validating
every changed path; defer _full_plan(...) until classification completes,
preserving fail-closed errors for unclassified paths. Add the mixed-diff
regression in scripts/ci/test_reborn_pr_test_plan.py:920-937 covering
.config/nextest.toml together with an unclassified scripts/** path.
- Around line 343-354: Add a regression case to the table used by
RebornPrTestPlanTests.test_decided_repo_root_paths_are_owned_by_other_workflows
for scripts/e2e-skill-self-creation.sh, asserting that the planned result is
mode: none with crate_buckets: []. Match the existing cases for paths in
PR_STATIC_CONTROL_PATHS.
In `@scripts/ci/test_reborn_pr_test_plan.py`:
- Around line 920-937: Extend test_nextest_config_widens_to_exhaustive_plan with
a mixed changed-file set containing .config/nextest.toml and an undecided helper
path such as scripts/some-undecided-helper.sh. Assert that plan raises the
fail-closed unclassified pull-request path error instead of returning the
exhaustive plan, covering processing of subsequent paths after the known
configuration change.
In `@tests/CLAUDE.md`:
- Line 190: Update the tool_disclosure.rs coverage row in tests/CLAUDE.md to
document both complete-search results calling tool_call directly and incomplete
or ambiguous results falling back to tool_describe. Remove the corresponding §7
gap row if it tracks this behavior, leaving only unresolved gaps.
- Line 67: Clarify the scope of the Python coverage figures in Section 2 near
the `102` files and `869` test functions entry: label them as active or
registered Reborn coverage if that is what they represent; otherwise reconcile
them with the exhaustive `103` files and `1,141` tests totals in Section 6.
Ensure the wording no longer presents both different figures as unqualified
totals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f933eddb-fd70-4d71-91fb-b6464b9bf22f
📒 Files selected for processing (5)
crates/app/ironclaw_composition/src/runtime.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_port.rsscripts/ci/reborn_pr_test_plan.pyscripts/ci/test_reborn_pr_test_plan.pytests/CLAUDE.md
💤 Files with no reviewable changes (1)
- crates/loop/ironclaw_loop_host/src/tool_disclosure_port.rs
Railway preview QA — PASS
Status derivationRequired cases passed: 3. Failed: 0. Blocked or not executed: 0. Therefore the exact-head result is PASS. Regression result and remaining scopeThe preview confirms the selected Cleanup: all three QA conversations were deleted and browser tabs were finalized. |
There was a problem hiding this comment.
Actionable comments posted: 2
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/src/tool_disclosure.rs (1)
2028-2028: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale test rationale.
The test name now specifies namespace summaries and representative tools. The adjacent comment still requires every discoverable tool name in the index. Namespace mode intentionally uses fair representative sampling within a byte cap. Update the comment to describe the current contract.
The PR objective requires fair namespace previews rather than exhaustive catalog listing.
🤖 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/src/tool_disclosure.rs` at line 2028, Update the comment adjacent to tool_search_description_summarizes_namespace_and_representative_tool to describe the current namespace-preview contract: summarize namespaces using fair representative tool sampling within the byte cap, rather than requiring every discoverable tool name in the index.
🤖 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.rs`:
- Around line 314-360: Replace the fixed namespace string returns in
discovery_namespace and builtin_discovery_namespace with a DiscoveryNamespace
enum covering first-party groups, plus a separate dynamic variant for
extension-derived namespaces. Update catalog-summary rendering to convert
DiscoveryNamespace values into model-facing labels at the boundary, while
preserving existing namespace classification and extension fallback behavior.
In `@scripts/tool_discovery_benchmark/test_run_benchmark.py`:
- Around line 43-48: Update
test_upload_task_is_self_contained_and_does_not_require_a_workspace_fixture to
assert that the upload prompt contains the concrete MIME value "text/csv", in
addition to the existing mime_type assertion.
---
Outside diff comments:
In `@crates/loop/ironclaw_loop_host/src/tool_disclosure.rs`:
- Line 2028: Update the comment adjacent to
tool_search_description_summarizes_namespace_and_representative_tool to describe
the current namespace-preview contract: summarize namespaces using fair
representative tool sampling within the byte cap, rather than requiring every
discoverable tool name in the index.
🪄 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: 8d9b8462-2090-452a-a19f-a955e35527c9
📒 Files selected for processing (4)
crates/loop/ironclaw_loop_host/src/tool_disclosure.rsdocs/internal/tool-discovery-subjective-decisions.mdscripts/tool_discovery_benchmark/run_benchmark.pyscripts/tool_discovery_benchmark/test_run_benchmark.py
Railway preview QA — PASS
Status derivationRequired cases passed: 3. Failed: 0. Blocked or not executed: 0. Therefore the exact-head result is PASS. Supplemental live-model benchmarkA focused 100-tool Cleanup: all three QA conversations were deleted and browser tabs were finalized. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/support/group.rs (1)
965-970: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the disclosure-mode error text.
Line 965 accepts every enabled disclosure mode. The error still says that only Bridged mode is valid. A Compact, Signatures, or Namespaces caller receives incorrect remediation.
State that the policy override requires any enabled disclosure mode. Rename the bridged-only helper separately if its public name is now inaccurate.
🤖 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 `@tests/integration/support/group.rs` around lines 965 - 970, Update the error text in the narrowed bridged policy validation to state that the override requires any enabled tool disclosure mode, and provide remediation that applies to Compact, Signatures, and Namespaces as well as Bridged. Separately review the public name of with_narrowed_capability_surface_policy_for_bridged_test() and rename it if the bridged-only wording is no longer accurate.scripts/tool_discovery_benchmark/run_benchmark.py (1)
146-188: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject empty benchmark packages before start-up.
generate_catalog(50)leavesworkflow-adminwith zero tools;install_catalogrejects that run after startup withnot installed.get(package_id, {}).get("tools"). Add one tool for every catalog before returning, with a precise generator failure message.🤖 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/tool_discovery_benchmark/run_benchmark.py` around lines 146 - 188, The generate_catalog function must ensure every namespace bucket contains at least one tool before returning, including when the requested tool count is smaller than the number of namespaces. Add a validation or generation step before the return that raises a precise ValueError identifying the empty namespace, or otherwise supplies the required tool according to the existing catalog rules; preserve the existing install_catalog contract.
🤖 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/prompts/tool_search_empty_catalog.md`:
- Line 1: Replace the content of the empty-catalog prompt with a concise
response stating that no deferred or additional tools are available. Remove
instructions to search or inspect tools, preserving the no-match behavior used
when discoverable_namespaces() returns empty.
In `@scripts/tool_discovery_benchmark/run_benchmark.py`:
- Around line 645-648: Update the run metadata construction in run_task_group so
thermal_class reflects the cache state for the current invocation rather than
using repetition == 0. Track whether the first repetition executed in that
invocation has completed, label that first executed repetition "cold", and label
subsequent repetitions "warm", including resumed runs starting at repetition 1.
- Around line 714-724: Define a shared OBSERVATION_SCHEMA_VERSION constant with
value 2, use it in run_task_group instead of the literal schema version, and
update load_observations to reject rows whose schema_version is not the current
constant before deduplication. Ensure stale schema_version 1 rows cannot enter
observations or suppress re-execution.
---
Outside diff comments:
In `@scripts/tool_discovery_benchmark/run_benchmark.py`:
- Around line 146-188: The generate_catalog function must ensure every namespace
bucket contains at least one tool before returning, including when the requested
tool count is smaller than the number of namespaces. Add a validation or
generation step before the return that raises a precise ValueError identifying
the empty namespace, or otherwise supplies the required tool according to the
existing catalog rules; preserve the existing install_catalog contract.
In `@tests/integration/support/group.rs`:
- Around line 965-970: Update the error text in the narrowed bridged policy
validation to state that the override requires any enabled tool disclosure mode,
and provide remediation that applies to Compact, Signatures, and Namespaces as
well as Bridged. Separately review the public name of
with_narrowed_capability_surface_policy_for_bridged_test() and rename it if the
bridged-only wording is no longer accurate.
🪄 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: b564109a-1542-48e7-83fc-344054af53b5
📒 Files selected for processing (15)
.env.examplecrates/app/ironclaw_composition/src/runtime.rscrates/loop/ironclaw_loop_host/prompts/tool_search_empty_catalog.mdcrates/loop/ironclaw_loop_host/prompts/tool_search_namespace_header.mdcrates/loop/ironclaw_loop_host/src/tool_disclosure.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_mode.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_port.rscrates/loop/ironclaw_loop_host/tests/llm_gateway.rscrates/loop/ironclaw_turn_runner/src/runtime.rsdocs/internal/tool-discovery-subjective-decisions.mdscripts/tool_discovery_benchmark/README.mdscripts/tool_discovery_benchmark/run_benchmark.pyscripts/tool_discovery_benchmark/test_run_benchmark.pytests/integration/support/group.rstests/integration/tool_disclosure.rs
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)
docs/internal/tool-discovery-evaluation.md (1)
54-77: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse one canonical arm identifier.
Line 64 requires each observation to record the selected
REBORN_TOOL_DISCLOSUREvalue. The table definessignaturesandnamespaces, but the schema example at Line 97 usesbounded_complete_signatures. The benchmark tests use short values such asnamespacesandbridgedinscripts/tool_discovery_benchmark/test_run_benchmark.pyLines 118-164. Different identifiers can split aggregates or break report consumers.Change Line 97 to
"arm": "signatures", or add a separate display-label field.🤖 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/internal/tool-discovery-evaluation.md` around lines 54 - 77, Update the benchmark schema example’s arm identifier to use the canonical REBORN_TOOL_DISCLOSURE value "signatures" instead of "bounded_complete_signatures", keeping it consistent with the arm table and benchmark tests.
🤖 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 `@docs/internal/tool-discovery-subjective-decisions.md`:
- Around line 375-384: The benchmark evidence must identify the exact evaluation
configuration and artifact. Update the evidence paragraph around the corrected
v2 runner to include the model route, temperature, catalog generator and seed,
thermal class, repetition metadata, and a link or unique identifier for the
result report, using the requirements in tool-discovery-evaluation.md.
---
Outside diff comments:
In `@docs/internal/tool-discovery-evaluation.md`:
- Around line 54-77: Update the benchmark schema example’s arm identifier to use
the canonical REBORN_TOOL_DISCLOSURE value "signatures" instead of
"bounded_complete_signatures", keeping it consistent with the arm table and
benchmark tests.
🪄 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: 9402176d-d6bf-4d69-9edb-4a1959393ce7
📒 Files selected for processing (2)
docs/internal/tool-discovery-evaluation.mddocs/internal/tool-discovery-subjective-decisions.md
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tool_discovery_benchmark/run_benchmark.py (1)
538-542: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve unavailable provider usage as
None.Lines 538-542 convert a missing trace into zero token and cache values.
_metric_deltacan then persist0as provider usage even when no provider reported usage. Keep call counts separate, but returnNonefor unavailable provider-token and cache metrics.The benchmark contract requires unavailable cache measurements to remain
null, not synthetic zeroes.🤖 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/tool_discovery_benchmark/run_benchmark.py` around lines 538 - 542, Update the missing-trace return path to keep call counts at zero while returning None for all provider-token and cache metrics, including input_tokens, output_tokens, cache_read_tokens, uncached_input_tokens, and cost_usd. Preserve the existing empty list result and ensure unavailable cache measurements remain null rather than synthetic zeroes.
🤖 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.rs`:
- Around line 318-365: Update DiscoveryNamespace equality to compare the values
returned by as_str() rather than using structural derived equality, and remove
the conflicting derived PartialEq/Eq implementations. Ensure the resulting
PartialEq and Eq behavior matches Ord/PartialOrd so first-party variants and
Extension values with identical labels compare equal.
In `@crates/loop/ironclaw_turn_runner/src/runtime.rs`:
- Around line 1157-1174: Update the child test invocation in
profile_pin_environment_rejects_invalid_configuration_at_runtime_startup to pass
--exact and the full module::tests::test_name filter, matching the sibling
tool_disclosure_mode_non_unicode_env_fails_closed pattern. Preserve the existing
invalid-environment arguments and success assertion.
In `@scripts/tool_discovery_benchmark/run_benchmark.py`:
- Around line 731-735: Update the result-building logic around result.success
and scored["completed"] so task_incomplete is assigned only when the live run
succeeds but task scoring fails. For unsuccessful runs, preserve a stable
failure category from the live result or caught runner error, and ensure that
category flows into failure_categories and aggregate results; add regression
coverage for both unsuccessful-run and successful-but-incorrect-task paths.
---
Outside diff comments:
In `@scripts/tool_discovery_benchmark/run_benchmark.py`:
- Around line 538-542: Update the missing-trace return path to keep call counts
at zero while returning None for all provider-token and cache metrics, including
input_tokens, output_tokens, cache_read_tokens, uncached_input_tokens, and
cost_usd. Preserve the existing empty list result and ensure unavailable cache
measurements remain null rather than synthetic zeroes.
🪄 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: 5c01009c-df26-44fc-a9b2-cf5fea2a0300
📒 Files selected for processing (16)
.env.examplecrates/app/ironclaw_composition/src/runtime.rscrates/loop/ironclaw_loop_host/prompts/tool_search_empty_catalog.mdcrates/loop/ironclaw_loop_host/src/tool_disclosure.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_mode.rscrates/loop/ironclaw_loop_host/src/tool_disclosure_port.rscrates/loop/ironclaw_turn_runner/src/runtime.rsdocs/internal/tool-discovery-evaluation.mddocs/internal/tool-discovery-subjective-decisions.mdscripts/ci/reborn_pr_test_plan.pyscripts/ci/test_reborn_pr_test_plan.pyscripts/tool_discovery_benchmark/README.mdscripts/tool_discovery_benchmark/run_benchmark.pyscripts/tool_discovery_benchmark/test_run_benchmark.pytests/CLAUDE.mdtests/integration/support/group.rs
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| enum DiscoveryNamespace { | ||
| Agents, | ||
| Coding, | ||
| Data, | ||
| Extensions, | ||
| Memory, | ||
| Messaging, | ||
| Observability, | ||
| Scheduling, | ||
| Settings, | ||
| Skills, | ||
| System, | ||
| Web, | ||
| Extension(String), | ||
| } | ||
|
|
||
| impl DiscoveryNamespace { | ||
| fn as_str(&self) -> &str { | ||
| match self { | ||
| Self::Agents => "agents", | ||
| Self::Coding => "coding", | ||
| Self::Data => "data", | ||
| Self::Extensions => "extensions", | ||
| Self::Memory => "memory", | ||
| Self::Messaging => "messaging", | ||
| Self::Observability => "observability", | ||
| Self::Scheduling => "scheduling", | ||
| Self::Settings => "settings", | ||
| Self::Skills => "skills", | ||
| Self::System => "system", | ||
| Self::Web => "web", | ||
| Self::Extension(extension_id) => extension_id, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl PartialOrd for DiscoveryNamespace { | ||
| fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | ||
| Some(self.cmp(other)) | ||
| } | ||
| } | ||
|
|
||
| impl Ord for DiscoveryNamespace { | ||
| fn cmp(&self, other: &Self) -> std::cmp::Ordering { | ||
| self.as_str().cmp(other.as_str()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make PartialEq agree with the label-based Ord.
Ord/PartialOrd compare self.as_str(), but PartialEq/Eq are derived structurally. DiscoveryNamespace::Memory and DiscoveryNamespace::Extension("memory".to_string()) then compare Ordering::Equal while == returns false. The same holds for an extension id equal to any first-party label (data, system, web, skills, …).
discoverable_namespaces uses this type as a BTreeMap key, and BTreeMap resolves keys through Ord only. Two distinct variants therefore collapse into one entry, and lookups become order-dependent. std::cmp::Ord requires consistency with PartialEq.
Derive the equality from the same label so both relations agree.
🔒️ Proposed fix
-#[derive(Debug, Clone, PartialEq, Eq)]
+#[derive(Debug, Clone)]
enum DiscoveryNamespace {
@@
+impl PartialEq for DiscoveryNamespace {
+ fn eq(&self, other: &Self) -> bool {
+ self.as_str() == other.as_str()
+ }
+}
+
+impl Eq for DiscoveryNamespace {}
+
impl PartialOrd for DiscoveryNamespace {📝 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.
| #[derive(Debug, Clone, PartialEq, Eq)] | |
| enum DiscoveryNamespace { | |
| Agents, | |
| Coding, | |
| Data, | |
| Extensions, | |
| Memory, | |
| Messaging, | |
| Observability, | |
| Scheduling, | |
| Settings, | |
| Skills, | |
| System, | |
| Web, | |
| Extension(String), | |
| } | |
| impl DiscoveryNamespace { | |
| fn as_str(&self) -> &str { | |
| match self { | |
| Self::Agents => "agents", | |
| Self::Coding => "coding", | |
| Self::Data => "data", | |
| Self::Extensions => "extensions", | |
| Self::Memory => "memory", | |
| Self::Messaging => "messaging", | |
| Self::Observability => "observability", | |
| Self::Scheduling => "scheduling", | |
| Self::Settings => "settings", | |
| Self::Skills => "skills", | |
| Self::System => "system", | |
| Self::Web => "web", | |
| Self::Extension(extension_id) => extension_id, | |
| } | |
| } | |
| } | |
| impl PartialOrd for DiscoveryNamespace { | |
| fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | |
| Some(self.cmp(other)) | |
| } | |
| } | |
| impl Ord for DiscoveryNamespace { | |
| fn cmp(&self, other: &Self) -> std::cmp::Ordering { | |
| self.as_str().cmp(other.as_str()) | |
| } | |
| } | |
| #[derive(Debug, Clone)] | |
| enum DiscoveryNamespace { | |
| Agents, | |
| Coding, | |
| Data, | |
| Extensions, | |
| Memory, | |
| Messaging, | |
| Observability, | |
| Scheduling, | |
| Settings, | |
| Skills, | |
| System, | |
| Web, | |
| Extension(String), | |
| } | |
| impl DiscoveryNamespace { | |
| fn as_str(&self) -> &str { | |
| match self { | |
| Self::Agents => "agents", | |
| Self::Coding => "coding", | |
| Self::Data => "data", | |
| Self::Extensions => "extensions", | |
| Self::Memory => "memory", | |
| Self::Messaging => "messaging", | |
| Self::Observability => "observability", | |
| Self::Scheduling => "scheduling", | |
| Self::Settings => "settings", | |
| Self::Skills => "skills", | |
| Self::System => "system", | |
| Self::Web => "web", | |
| Self::Extension(extension_id) => extension_id, | |
| } | |
| } | |
| } | |
| impl PartialEq for DiscoveryNamespace { | |
| fn eq(&self, other: &Self) -> bool { | |
| self.as_str() == other.as_str() | |
| } | |
| } | |
| impl Eq for DiscoveryNamespace {} | |
| impl PartialOrd for DiscoveryNamespace { | |
| fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | |
| Some(self.cmp(other)) | |
| } | |
| } | |
| impl Ord for DiscoveryNamespace { | |
| fn cmp(&self, other: &Self) -> std::cmp::Ordering { | |
| self.as_str().cmp(other.as_str()) | |
| } | |
| } |
🤖 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/src/tool_disclosure.rs` around lines 318 -
365, Update DiscoveryNamespace equality to compare the values returned by
as_str() rather than using structural derived equality, and remove the
conflicting derived PartialEq/Eq implementations. Ensure the resulting PartialEq
and Eq behavior matches Ord/PartialOrd so first-party variants and Extension
values with identical labels compare equal.
| for invalid in ["{", r#"{"interactive_tools":["invalid"]}"#] { | ||
| let output = std::process::Command::new( | ||
| std::env::current_exe().expect("current test executable"), | ||
| ) | ||
| .args([ | ||
| "profile_pin_environment_rejects_invalid_configuration_at_runtime_startup", | ||
| "--nocapture", | ||
| ]) | ||
| .env(CHILD_MARKER, "1") | ||
| .env(REBORN_TOOL_DISCLOSURE_PROFILE_PINS_ENV, invalid) | ||
| .output() | ||
| .expect("child test process executes"); | ||
| assert!( | ||
| output.status.success(), | ||
| "runtime startup must reject invalid pin config: {}", | ||
| String::from_utf8_lossy(&output.stderr) | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Pin the child test filter with --exact and the full module path.
The child is invoked with a bare name filter. If the test is renamed or moved, the filter matches zero tests, the child harness exits 0, and the parent assertion passes without verifying anything. The sibling tool_disclosure_mode_non_unicode_env_fails_closed test avoids this by passing --exact with the full module::tests::name path.
💚 Proposed fix
.args([
- "profile_pin_environment_rejects_invalid_configuration_at_runtime_startup",
+ "--exact",
+ "runtime::tests::profile_pin_environment_rejects_invalid_configuration_at_runtime_startup",
+ "--test-threads=1",
"--nocapture",
])📝 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.
| for invalid in ["{", r#"{"interactive_tools":["invalid"]}"#] { | |
| let output = std::process::Command::new( | |
| std::env::current_exe().expect("current test executable"), | |
| ) | |
| .args([ | |
| "profile_pin_environment_rejects_invalid_configuration_at_runtime_startup", | |
| "--nocapture", | |
| ]) | |
| .env(CHILD_MARKER, "1") | |
| .env(REBORN_TOOL_DISCLOSURE_PROFILE_PINS_ENV, invalid) | |
| .output() | |
| .expect("child test process executes"); | |
| assert!( | |
| output.status.success(), | |
| "runtime startup must reject invalid pin config: {}", | |
| String::from_utf8_lossy(&output.stderr) | |
| ); | |
| } | |
| for invalid in ["{", r#"{"interactive_tools":["invalid"]}"#] { | |
| let output = std::process::Command::new( | |
| std::env::current_exe().expect("current test executable"), | |
| ) | |
| .args([ | |
| "--exact", | |
| "runtime::tests::profile_pin_environment_rejects_invalid_configuration_at_runtime_startup", | |
| "--test-threads=1", | |
| "--nocapture", | |
| ]) | |
| .env(CHILD_MARKER, "1") | |
| .env(REBORN_TOOL_DISCLOSURE_PROFILE_PINS_ENV, invalid) | |
| .output() | |
| .expect("child test process executes"); | |
| assert!( | |
| output.status.success(), | |
| "runtime startup must reject invalid pin config: {}", | |
| String::from_utf8_lossy(&output.stderr) | |
| ); | |
| } |
🤖 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_turn_runner/src/runtime.rs` around lines 1157 - 1174,
Update the child test invocation in
profile_pin_environment_rejects_invalid_configuration_at_runtime_startup to pass
--exact and the full module::tests::test_name filter, matching the sibling
tool_disclosure_mode_non_unicode_env_fails_closed pattern. Preserve the existing
invalid-environment arguments and success assertion.
| "ui_probe_success": result.success, | ||
| "installed_namespaces": len(packages), | ||
| "failure": None | ||
| if result.success and scored["completed"] | ||
| else "task_incomplete", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not classify every failed run as task_incomplete.
Lines 731-735 assign task_incomplete for both an unsuccessful live run and a completed live run with an incorrect task result. This removes provider and transport failure information from failure_categories and makes aggregate results unable to distinguish model-task failures from runner failures.
Reserve task_incomplete for a successful run that fails task scoring. Record a stable failure category from the live result or caught runner error for unsuccessful runs. Add regression cases for both paths.
🤖 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/tool_discovery_benchmark/run_benchmark.py` around lines 731 - 735,
Update the result-building logic around result.success and scored["completed"]
so task_incomplete is assigned only when the live run succeeds but task scoring
fails. For unsuccessful runs, preserve a stable failure category from the live
result or caught runner error, and ensure that category flows into
failure_categories and aggregate results; add regression coverage for both
unsuccessful-run and successful-but-incorrect-task paths.
…#7410) * test(tool-search): add large-catalog baseline * feat(tool-search): return and use bounded signatures * feat(tool-search): add fair discovery benchmark arms * test(tool-discovery): add live benchmark harness * feat(tool-discovery): default to namespace summaries * ci(tool-discovery): classify benchmark harness * feat(tool-discovery): use semantic namespaces * fix(tool-search): harden discovery benchmark and mode wiring * docs(tool-search): record corrected benchmark verdict * fix(tool-search): address review feedback
This branch was successfully deployed
2 participants
Summary
tool_search, removing the mandatorytool_describeround trip when the schema fits.bridgedmode, while production defaults to the safer unpinnednamespacesmode.docs/internal/tool-discovery-subjective-decisions.md.What this achieves
Previously, a deferred tool generally required
search → describe → invoke. A search hit now carries the canonical parameter schema plusschema_complete; the normal path issearch → invoke.tool_describeremains available when a schema exceeds the 8 KiB per-result or 24 KiB aggregate response budget, or when explicit inspection is useful.The passive preview reports authorized namespace counts, then spends its remaining fixed budget on representative tool names in fair namespace rounds. First-party ownership buckets such as
builtinandironclaware regrouped deterministically by intent (coding,memory,scheduling,skills,observability, and so on). External tools retain their stable extension identity, such asgithub,gmail, orgoogle-calendar.Catalog construction starts from the effective policy-filtered surface. Search results, complete signatures, namespace counts, pins, direct calls, and wrapped calls cannot widen authority; every invocation still crosses validation, authorization, approval, hooks, safety, runtime dispatch, and evidence handling.
This is provider-neutral. It complements rather than replaces Anthropic-native
defer_loading/tool_referencebehavior and preserves the provider's stable advertised-array contract.Multi-agent review findings and fixes
The eight-lens review completed with full diff coverage. It found that the first benchmark version could support a retrieval claim but not an end-to-end merge decision:
bridgedbefore a follow-up override.The fixes now:
tool_calltargets, for forbidden calls;CapabilitySurfaceProfileIdkeys and require the disclosure mode at construction;Static model-facing disclosure prose also moved into prompt assets, and runtime logging now reports the actual selected mode instead of calling every enabled arm “bridged.”
Corrected 100-tool benchmark verdict
The authoritative v2 comparison (artifact identity
tool-discovery-v2-1a674e7-nearai-deepseek-v4-flash-seed7405-100tools-56obs) ran the shippingironclaw servebinary at runtime commit1a674e7724, NearAIdeepseek-ai/DeepSeek-V4-Flash, temperature0.0, catalog generatortool-search-scale-v2, 20 semantic hosted MCP integrations, deterministic seed 7405, seven task classes, and one cold plus three warm repetitions per task/arm: 56 observations total.namespacesbridgedThe overall latency difference is directional, not a clean causal estimate: different task/arm groups independently hit the provider's roughly 187-second tail. The per-scenario results explain the tradeoff:
namespacesand 0/4 withbridgedunder argument-and-order scoring.Verdict: keep
namespacesas the production default.bridgedis a useful latency optimization for deployments that validate their pin set against representative end-to-end workflows, but its lower completion rate fails the default-selection gate.The earlier 268-observation exploratory run remains useful for discovering scale and provider-tail behavior, but its anonymous package allocation and name-only scoring are superseded by the v2 result above. NearAI usage fields were inconsistent/zero, so this PR does not invent token estimates from JSON bytes. Deterministic context bounds remain: passive preview ≤4,096 bytes, complete schema ≤8 KiB, the complete search-result JSON envelope ≤24 KiB, and the default adds no standing pinned definitions.
Configuration
REBORN_TOOL_DISCLOSUREoffcompactsignaturesnamespacesbridgedUnknown values fail closed to
off. Invalid pin JSON, profile IDs, or capability IDs reject production startup with the parse cause retained; an unset pin variable remains the empty default. Pins never widen authority.Change Type
Validation
cargo fmt --all -- --checkcargo clippy -p ironclaw_loop_host -p ironclaw_turn_runner -p ironclaw_composition --all-targets --all-features -- -D warningscargo test -p ironclaw_loop_host— package suite passed, including 89 gateway testscargo test -p ironclaw_integration_tests --test reborn_integration_tool_disclosure— 34 passedcargo test -p ironclaw_composition --lib— 528 passedcargo test -p ironclaw_architecture_tests— passedpython3 -m pytest scripts/tool_discovery_benchmark/test_run_benchmark.py -q— 8 passedpython3 scripts/ci/docs_publication_boundary.pyironclaw_turn_runnerlibrary suite — 242/243 passed; unrelated pre-existingtrace_capture::tests::capture_skips_when_policy_missing_or_disabledfails because a queue directory exists for a non-enrolled scope. This PR does not modifytrace_capture.rs.Test Strategy
User behavior: complete search results can invoke directly; incomplete results recover through describe-first; invalid pin configuration now blocks startup with an actionable error.
Risk areas:
Tiers:
What the tests prove: response-envelope bounds match model-visible bytes, incomplete signatures cannot bypass recovery, pin errors fail at startup, benchmark resume/report data stays interpretable, and CI selection remains fail-closed for mixed paths.
Commands run: see the checked Validation list above; the review-fix gate additionally ran the complete loop-host suite, composition suite, disclosure integration binary, strict clippy, architecture tests, and 85 Python tests with 157 subtests.
Security Impact
Authorized canonical schemas may appear in bounded search results, and authorized namespace counts appear in the safe bridge description. Denied tools cannot affect counts, preview allocation, search results, signatures, pins, or callable targets. Forbidden attempts are now measured from model traces instead of reported as a constant.
Reborn Trust-Boundary Checklist
RebornRuntimeError.serde(default)field was added.Database Impact
None.
Blast Radius
Touches provider-neutral tool disclosure, planned-runtime configuration startup, the live benchmark/report schema, integration-test harness selection, and internal coverage documentation. Primary regressions would be an incomplete schema treated as callable, a valid deployment rejected by pin parsing, or stale benchmark rows contaminating a resumed report. No persistence schema, authorization grant, approval path, or provider-specific wire protocol changes.
Compatibility and rollback
No migration is required.
namespacesremains the default; deployments with validated pins can selectbridged. SetREBORN_TOOL_DISCLOSURE=offfor immediate rollback. No persistent-data cleanup is needed.Linked Issue
Closes #7405.
Review Follow-Through
Audited 7 conversation comments, 8 submitted review bodies, and all 13 inline threads with complete pagination. Valid findings were addressed in
4688d2e369; duplicate, already-addressed, and non-actionable items were documented in per-thread replies without resolving threads. The 1,000-tool tier remains an explicit user-approved exception, and exact-current-head Railway QA remains the only open validation item.Review track
B — provider-neutral feature with model-behavior, authorization, context-budget, and benchmark-validity implications.