feat(tool-registry): add trusted capability providers#2551
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a config-backed capability-provider schema and registry with ID normalization/validation, exposes registry/listing/lookup helpers, integrates provider summaries into tool-policy diagnostics and the diagnostics RPC, updates re-exports and docs, and adds unit and integration tests. ChangesCapability Provider Registry Feature
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
coderabbitai
Bot
added
the
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/openhuman/tool_registry/ops.rs (1)
44-73: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd debug/trace diagnostics to the new
diagnostics_for_configflow.Line 44 introduces a new diagnostics path, but it currently has no entry/exit or summary logging. Please add structured debug logs with stable prefix + counts to keep this path diagnosable.
♻️ Proposed logging patch
pub fn diagnostics_for_config(config: &Config) -> RpcOutcome<ToolPolicyDiagnostics> { + log::debug!("[tool_registry] diagnostics_for_config start"); + let tools = registry_entries(); let total_tools = tools.len(); let enabled_tools = tools.iter().filter(|entry| entry.enabled).count(); @@ let diagnostics = ToolPolicyDiagnostics { total_tools, enabled_tools, mcp_stdio_tools, json_rpc_tools, possible_write_surfaces, policy_surfaces, capability_providers: capability_provider_diagnostics(config), }; + + log::debug!( + "[tool_registry] diagnostics_for_config completed total_tools={} enabled_tools={} mcp_stdio_tools={} json_rpc_tools={} providers_total={} providers_enabled={} providers_trusted={} providers_trusted_enabled={} provider_errors={}", + diagnostics.total_tools, + diagnostics.enabled_tools, + diagnostics.mcp_stdio_tools, + diagnostics.json_rpc_tools, + diagnostics.capability_providers.total_providers, + diagnostics.capability_providers.enabled_providers, + diagnostics.capability_providers.trusted_providers, + diagnostics.capability_providers.trusted_enabled_providers, + diagnostics.capability_providers.registry_errors.len(), + ); RpcOutcome::new(diagnostics, vec![]) }As per coding guidelines, "Debug logging must follow these rules: default to verbose diagnostics on new/changed flows... All changes lacking diagnosis logging are incomplete."
🤖 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 `@src/openhuman/tool_registry/ops.rs` around lines 44 - 73, Add structured debug/trace logging to the diagnostics_for_config flow: at function entry log a stable prefix (e.g. "tool_registry::diagnostics:entry") with total_tools and enabled_tools, then log intermediate counts (mcp_stdio_tools, json_rpc_tools) and the length of possible_write_surfaces and policy_surfaces after they are computed, and finally log an exit/summary message (e.g. "tool_registry::diagnostics:summary") that includes all counts and the size of capability_providers; use the project’s tracing/log macro (trace! or debug!) consistent with existing code, and place logs near the calls to registry_entries(), looks_write_capable(), policy_surface_ids(), and capability_provider_diagnostics(config) so the referenced symbols (diagnostics_for_config, ToolPolicyDiagnostics, registry_entries, looks_write_capable, policy_surface_ids, capability_provider_diagnostics) are easy to locate.
🤖 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 `@src/openhuman/tool_registry/providers.rs`:
- Around line 31-170: Add trace/debug/error logs across the new registry flow:
instrument CapabilityProviderRegistry::from_config to log entry (trace) with
count of config.capability_providers, log a debug when a provider is normalized
(use normalize_capability_provider_id return) and an error/debug when a
DuplicateId is detected before returning
CapabilityProviderRegistryError::DuplicateId, and log debug/error when
clean_string fails to fall back to id; also update
normalize_capability_provider_id to trace invalid/empty inputs and the final
normalized value before returning or returning InvalidId; finally, in
capability_provider_diagnostics and capability_provider_registry add debug/error
logging for the Err path to record the registry error (err.to_string()) so
fallback diagnostics include a logged message.
---
Outside diff comments:
In `@src/openhuman/tool_registry/ops.rs`:
- Around line 44-73: Add structured debug/trace logging to the
diagnostics_for_config flow: at function entry log a stable prefix (e.g.
"tool_registry::diagnostics:entry") with total_tools and enabled_tools, then log
intermediate counts (mcp_stdio_tools, json_rpc_tools) and the length of
possible_write_surfaces and policy_surfaces after they are computed, and finally
log an exit/summary message (e.g. "tool_registry::diagnostics:summary") that
includes all counts and the size of capability_providers; use the project’s
tracing/log macro (trace! or debug!) consistent with existing code, and place
logs near the calls to registry_entries(), looks_write_capable(),
policy_surface_ids(), and capability_provider_diagnostics(config) so the
referenced symbols (diagnostics_for_config, ToolPolicyDiagnostics,
registry_entries, looks_write_capable, policy_surface_ids,
capability_provider_diagnostics) are easy to locate.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab272e84-9f6c-4220-9848-8ca4e9a8a796
📒 Files selected for processing (11)
gitbooks/developing/mcp-server.mdgitbooks/developing/mcp-server.zh-CN.mdsrc/openhuman/config/README.mdsrc/openhuman/config/mod.rssrc/openhuman/config/schema/capability_providers.rssrc/openhuman/config/schema/mod.rssrc/openhuman/config/schema/types.rssrc/openhuman/tool_registry/mod.rssrc/openhuman/tool_registry/ops.rssrc/openhuman/tool_registry/providers.rssrc/openhuman/tool_registry/types.rs
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 (1)
src/openhuman/tool_registry/ops.rs (1)
43-99:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftWire the public diagnostics RPC to the active config snapshot.
diagnostics_for_confignow reports provider counts/errors fromconfig, butdiagnostics()still invokes it withConfig::default(). That meanstool_registry.diagnosticswill always hide realcapability_providersstate and never surface duplicate/invalid provider config at runtime.🤖 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 `@src/openhuman/tool_registry/ops.rs` around lines 43 - 99, diagnostics_for_config currently uses the provided Config to report provider counts/errors, but diagnostics() still calls diagnostics_for_config with Config::default(), hiding runtime provider state; fix by changing diagnostics() to pass the active runtime config snapshot instead of Config::default() — locate the diagnostics() function and replace the Config::default() argument with the active configuration snapshot provided by your runtime (e.g. the function or value that returns the current config snapshot used elsewhere in the app), so diagnostics_for_config and capability_provider_diagnostics receive real runtime data.
🧹 Nitpick comments (1)
src/openhuman/tool_registry/ops.rs (1)
43-99: 🏗️ Heavy liftSplit this module before it grows further.
This addition lands in a file that is already ~650 lines long. Please peel the diagnostics/provider-specific logic into a smaller sibling module so
ops.rsstays focused.As per coding guidelines: “File size should not exceed approximately 500 lines. When a module grows beyond this threshold, split it into smaller, more focused modules with clear responsibilities.”
🤖 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 `@src/openhuman/tool_registry/ops.rs` around lines 43 - 99, The diagnostics/provider logic in ops.rs is making the file too large; extract the diagnostics implementation into a new sibling module (e.g., tool_registry/diagnostics.rs) and move functions and types related to diagnostics and provider inspection—specifically diagnostics_for_config, ToolPolicyDiagnostics, capability_provider_diagnostics, and any helpers that call registry_entries or policy_surface_ids—into that module; update ops.rs to call the moved functions (keep the public signatures) and re-export types if needed so callers remain unchanged, and ensure imports/uses are adjusted (use crate::openhuman::tool_registry::diagnostics::{diagnostics_for_config, ToolPolicyDiagnostics} or similar) and run tests to confirm behavior is preserved.
🤖 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 `@src/openhuman/tool_registry/ops.rs`:
- Around line 43-99: diagnostics_for_config currently uses the provided Config
to report provider counts/errors, but diagnostics() still calls
diagnostics_for_config with Config::default(), hiding runtime provider state;
fix by changing diagnostics() to pass the active runtime config snapshot instead
of Config::default() — locate the diagnostics() function and replace the
Config::default() argument with the active configuration snapshot provided by
your runtime (e.g. the function or value that returns the current config
snapshot used elsewhere in the app), so diagnostics_for_config and
capability_provider_diagnostics receive real runtime data.
---
Nitpick comments:
In `@src/openhuman/tool_registry/ops.rs`:
- Around line 43-99: The diagnostics/provider logic in ops.rs is making the file
too large; extract the diagnostics implementation into a new sibling module
(e.g., tool_registry/diagnostics.rs) and move functions and types related to
diagnostics and provider inspection—specifically diagnostics_for_config,
ToolPolicyDiagnostics, capability_provider_diagnostics, and any helpers that
call registry_entries or policy_surface_ids—into that module; update ops.rs to
call the moved functions (keep the public signatures) and re-export types if
needed so callers remain unchanged, and ensure imports/uses are adjusted (use
crate::openhuman::tool_registry::diagnostics::{diagnostics_for_config,
ToolPolicyDiagnostics} or similar) and run tests to confirm behavior is
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6de0df1-0b5d-47b4-92cd-8a4fd89f9b53
📒 Files selected for processing (2)
src/openhuman/tool_registry/ops.rssrc/openhuman/tool_registry/providers.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/tool_registry/ops.rs (1)
490-706: 🏗️ Heavy liftMove the expanded test block out of
ops.rs.This file is now well past the repo’s ~500-line target, and most of the new growth here is test-only code. Please extract the diagnostics tests/helpers into a sibling
ops_test.rsand keepops.rsfocused on runtime logic.As per coding guidelines,
**/*.{ts,tsx,rs}files should not exceed approximately 500 lines, andsrc/**/*.rstest extractions should prefer a sibling*_test.rswired with#[cfg(test)] #[path = "..._test.rs"] mod tests;.🤖 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 `@src/openhuman/tool_registry/ops.rs` around lines 490 - 706, The ops.rs file has grown >500 lines due to many test helpers and diagnostics tests; extract them into a sibling ops_test.rs and wire it as a test module. Move the tests (including async test diagnostics_loads_active_capability_provider_config, diagnostics_for_config_reports_*, looks_write_capable, is_policy_surface, insert_registry_entry_skips_duplicate_tool_id, get_tool_*, controller_json_schema_marks_required_and_optional_fields), helper fn capability_provider, struct EnvRestore and its impl/Drop, and any test-only uses of diagnostics() and diagnostics_for_config() into ops_test.rs; then remove those items from ops.rs and add in ops.rs the test module declaration #[cfg(test)] #[path = "ops_test.rs"] mod tests; so runtime ops.rs stays under ~500 lines while tests remain compiled only in test builds.
🤖 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.
Nitpick comments:
In `@src/openhuman/tool_registry/ops.rs`:
- Around line 490-706: The ops.rs file has grown >500 lines due to many test
helpers and diagnostics tests; extract them into a sibling ops_test.rs and wire
it as a test module. Move the tests (including async test
diagnostics_loads_active_capability_provider_config,
diagnostics_for_config_reports_*, looks_write_capable, is_policy_surface,
insert_registry_entry_skips_duplicate_tool_id, get_tool_*,
controller_json_schema_marks_required_and_optional_fields), helper fn
capability_provider, struct EnvRestore and its impl/Drop, and any test-only uses
of diagnostics() and diagnostics_for_config() into ops_test.rs; then remove
those items from ops.rs and add in ops.rs the test module declaration
#[cfg(test)] #[path = "ops_test.rs"] mod tests; so runtime ops.rs stays under
~500 lines while tests remain compiled only in test builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54adaeed-a9d6-4aa4-a33f-52ad91581ff7
📒 Files selected for processing (2)
src/openhuman/tool_registry/ops.rssrc/openhuman/tool_registry/schemas.rs
|
@graycyrus @senamakel #2551 is ready for review. I addressed the CodeRabbit feedback, wired diagnostics to the active runtime config, extracted the expanded tests into |
|
Checked the latest CI. The only blocking check is The failure is in
All other checks are green, including Rust/TS/coverage and CodeRabbit. I also checked #2499 and it is failing on the exact same Composio RPC points, so this currently looks more like a shared mega-flow/Composio CI issue than a regression specific to this PR. The uploaded artifact only contains screenshots/XML/meta, not the core/app log, so the next useful step is to capture the underlying RPC error payload before blindly rerunning. |
|
Follow-up: opened #2609 to make this shared E2E failure diagnosable. It adds runner-temp app log upload paths and changes the mega-flow Composio RPC assertions to include the RPC method/status/error instead of only Once #2609 CI runs, we should be able to see the underlying core/RPC error payload and decide whether the Composio failure is a shared harness/session issue or a real backend/mock regression. |
|
Follow-up on the shared Linux Appium failure: #2609 now confirms and fixes the root cause. The failing #2609 defaults E2E sessions to |
2 participants
Summary
capability_providersconfig metadata for external capability sources with id, display name, source URI, source digest, trust state, and enabled state.tool_registrywith list/get/trusted-enabled helpers for policy and diagnostics callers.Problem
Solution
CapabilityProviderConfigandCapabilityProviderTrustStateunder the TOML config schema.CapabilityProviderRegistrythat normalizes provider ids, rejects invalid ids, rejects duplicates after normalization, and exposes list/get/trusted-enabled helpers.Submission Checklist
11.1.5 Global tool registryremains applicable.## RelatedCloses #NNNin the## RelatedsectionImpact
trust_state = "untrusted",enabled = false) and duplicate/invalid provider ids are rejected before policy callers consume metadata.capability_providersdeserializes to an empty list.Related
11.1.5 Global tool registryAI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/2541-trusted-capability-providersc7d7b078b137e1b7dc1e94fec193ce3d8d2381adValidation Run
pnpm --filter openhuman-app format:checkblocked locally; seeValidation Blocked.pnpm format:checkdelegates to this command.pnpm typechecknot run because there are no TypeScript/frontend changes and local app dependencies are not installed.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml tool_registry --libGGML_NATIVE=OFF cargo test --manifest-path Cargo.toml config_parses_capability_provider_entries --libcargo fmt --manifest-path Cargo.toml --checkGGML_NATIVE=OFF cargo check --manifest-path Cargo.tomlgit diff --checkValidation Blocked
command:pnpm format:checkerror:local app workspace has nonode_modules;prettier: command not found. The environment also reports Nodev22.14.0whileopenhuman-appwants>=24.0.0.impact:Prettier/app format check could not be completed locally; Rust fmt/check and focused Rust tests passed.Behavior Changes
Parity Contract
tools/calland JSON-RPC controller dispatch remain unchanged.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Documentation
New Features
Tests