Add tool registry policy diagnostics#2336
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 diagnostics RPC to the tool registry: defines ToolPolicyDiagnostics, implements diagnostics() with helpers to count tools and discover write-capable and policy surfaces, and wires a diagnostics controller/handler with tests validating schema and JSON output. ChangesTool Registry Diagnostics Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/tool_registry/ops.rs (1)
276-311: ⚡ Quick winDeduplicate policy surface IDs to avoid future drift.
policy_surface_ids()andis_policy_surface()each carry the same hard-coded list. A single source of truth avoids mismatches when surfaces are added/removed.Proposed refactor
+const POLICY_SURFACES: &[&str] = &[ + "security.policy_info", + "approval.list_pending", + "approval.list_recent_decisions", + "approval.decide", + "tool_registry.list", + "tool_registry.get", + "tool_registry.diagnostics", +]; + fn policy_surface_ids() -> Vec<String> { - let mut ids = [ - "security.policy_info", - "approval.list_pending", - "approval.list_recent_decisions", - "approval.decide", - "tool_registry.list", - "tool_registry.get", - "tool_registry.diagnostics", - ] + let mut ids = POLICY_SURFACES .into_iter() - .map(String::from) + .map(|id| (*id).to_string()) .collect::<BTreeSet<_>>(); @@ fn is_policy_surface(tool_id: &str) -> bool { - matches!( - tool_id, - "security.policy_info" - | "approval.list_pending" - | "approval.list_recent_decisions" - | "approval.decide" - | "tool_registry.list" - | "tool_registry.get" - | "tool_registry.diagnostics" - ) + POLICY_SURFACES.contains(&tool_id) }🤖 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 276 - 311, The hard-coded policy surface list is duplicated between policy_surface_ids() and is_policy_surface(), so centralize the list into a single source of truth (e.g., a static array or BTreeSet constant like POLICY_SURFACES) and update both functions to use it: have policy_surface_ids() build its BTreeSet/Vec from POLICY_SURFACES and have is_policy_surface(tool_id: &str) check membership against POLICY_SURFACES (or the BTreeSet) instead of repeating the string literals; keep the existing return types for policy_surface_ids() and the signature of is_policy_surface().
🤖 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/schemas.rs`:
- Around line 107-115: The diagnostics RPC currently logs only on entry inside
handle_diagnostics; after calling
crate::openhuman::tool_registry::ops::diagnostics() and before returning the
JSON via to_json(...), add a debug-level exit log (e.g., log::debug!) that
indicates the diagnostics RPC completed (optionally include params.len() or a
brief response summary) so the handler has both entry and exit traces; place the
log immediately after the response is constructed and before the
ControllerFuture is returned from handle_diagnostics.
---
Nitpick comments:
In `@src/openhuman/tool_registry/ops.rs`:
- Around line 276-311: The hard-coded policy surface list is duplicated between
policy_surface_ids() and is_policy_surface(), so centralize the list into a
single source of truth (e.g., a static array or BTreeSet constant like
POLICY_SURFACES) and update both functions to use it: have policy_surface_ids()
build its BTreeSet/Vec from POLICY_SURFACES and have is_policy_surface(tool_id:
&str) check membership against POLICY_SURFACES (or the BTreeSet) instead of
repeating the string literals; keep the existing return types for
policy_surface_ids() and the signature of is_policy_surface().
🪄 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: 9cce37fe-5ad1-4ecd-ae8e-17886595aa5c
📒 Files selected for processing (4)
src/openhuman/tool_registry/mod.rssrc/openhuman/tool_registry/ops.rssrc/openhuman/tool_registry/schemas.rssrc/openhuman/tool_registry/types.rs
|
@vaddisrinivas CI is failing on changes in this PR — please fix before review. |
1 similar comment
|
@vaddisrinivas CI is failing on changes in this PR — please fix before review. |
vaddisrinivas
changed the title
|
Thanks for the heads-up. CI is green now on the latest run; re-review welcome. |
… pr-2336-fix-comments # Conflicts: # src/openhuman/tool_registry/ops.rs
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Review — tool_registry policy diagnostics
Well-structured PR. Clean module layout, good test coverage, and the diagnostics RPC fits the existing tool_registry pattern nicely. Both CodeRabbit findings (exit log, deduplication) are already addressed in the latest commits.
Two issues below — one logic bug and one heuristic gap.
| Area | Files | Verdict |
|---|---|---|
| Rust core (ops) | ops.rs |
1 major |
| Rust core (ops) | ops.rs |
1 minor |
| Rust core (schemas) | schemas.rs |
Clean |
| Rust core (types) | types.rs |
Clean |
| Rust core (mod) | mod.rs |
Clean |
|
@graycyrus Thanks again for the review. I addressed both requested diagnostics fixes in 8d25d1a (dynamic policy namespaces and |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Re-review — all prior changes addressed ✓
Both findings from my previous review are resolved in 8d25d1a8:
| File | Finding | Status |
|---|---|---|
ops.rs — policy_surface_ids() |
Dynamic extension was a no-op (filtered against same const that seeded the set) | Fixed — is_policy_surface() now matches security.* / approval.* namespaces dynamically, so newly registered policy schemas are picked up |
ops.rs — looks_write_capable() |
Missing {marker}.suffix pattern (e.g. create.user) |
Fixed — added {marker}. check, test updated to cover create.user |
No new issues found. Clean, well-structured addition — nice work @vaddisrinivas.
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Looks good, nice work!
3 participants
Refs #2136
Summary
tool_registry.diagnosticsas a generic, redacted diagnostics RPC for tool inventory and policy visibility review.Checklist
codex/oh-2136-policy-diagnosticsa99bd5d9820899a22a618f80d4fb9e0f35057fc9src/openhuman/tool_registry/mod.rs,src/openhuman/tool_registry/ops.rs,src/openhuman/tool_registry/schemas.rs,src/openhuman/tool_registry/types.rscargo fmt --manifest-path Cargo.toml --all --check;cargo test --manifest-path Cargo.toml tool_registrytool_registrySummary by CodeRabbit