Repository navigation
Conversation
`tool_info("mission_create")` returned "No tool named 'mission_create' is registered" even though `mission_create` was advertised in the system prompt and executed correctly. Root cause: two sources of truth — the v1 `ToolRegistry` (holds `Tool` trait impls) and the engine-v2 `CapabilityRegistry` (holds engine-native actions like missions). `EffectBridgeAdapter::available_actions` merges both when building the system prompt, but the discovery tools only consulted the v1 registry. So the LLM saw mission actions advertised, tried to introspect them, got contradicted, and stopped trusting its own action list.
- Taught `ToolRegistry` to carry an optional `Arc<CapabilityRegistry>` handle. `None` on v1, `Some` after the v2 bootstrap wires it.
- Made `tool_info` and `system_tools_list` search both sources. On v1 the capability registry is `None` so the fallback is a no-op; on v2 missions resolve through it.
- Factored the dual-consumer wiring (effect adapter + tool registry) into `wire_capability_registry` in `router.rs` — one call site, one test, locked in.
- Closed the external-tool shadowing risk: `ToolRegistry::register()` rejects any dynamic tool whose name (including hyphen/underscore aliases) matches a capability action. WASM and MCP tools named `mission_create` or `mission-create` can no longer shadow capability actions.
- Debug-assert at wire time catches the reverse: a built-in tool registered before v2 bootstrap that collides with a capability name.
There was a problem hiding this comment.
Code Review
This pull request centralizes and enforces the wiring of the engine v2 CapabilityRegistry to both the EffectBridgeAdapter and the ToolRegistry. It introduces a wire_capability_registry helper to ensure both consumers are synchronized and includes a debug-time check to prevent name collisions between built-in tools and capability actions. Additionally, discovery tools such as system_tools_list and tool_info have been updated to introspect and surface capability actions, while the ToolRegistry now prevents dynamic tool registrations from shadowing capability names (including hyphen/underscore aliases). I have no feedback to provide as there were no review comments.
There was a problem hiding this comment.
Pull request overview
Fixes a split-brain between v1 ToolRegistry and v2-only CapabilityRegistry so discovery tools (notably tool_info and system_tools_list) can introspect engine-native v2 capability actions (e.g. missions) that are already advertised and executable in engine v2.
Changes:
- Added
resolve_with_aliaseshelper to consistently resolve hyphen/underscore name variants for capability lookups. - Extended
ToolRegistryto optionally hold anArc<CapabilityRegistry>and updated discovery tools to consult both registries. - Centralized capability-registry wiring via
wire_capability_registryand added tests; added guardrails to prevent dynamic tools from shadowing capability actions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/tool.rs | Adds shared alias-resolution helper for consistent name lookup. |
| src/tools/registry.rs | Stores optional v2 CapabilityRegistry handle; rejects dynamic tools that would shadow capability actions; adds tests. |
| src/tools/mod.rs | Re-exports alias resolver for debug-only collision checks. |
| src/tools/builtin/tool_info.rs | Falls back to v2 capability actions when a tool isn’t found in v1 registry; adds tests. |
| src/tools/builtin/system.rs | Extends system_tools_list to include v2 capability actions. |
| src/bridge/router.rs | Adds wire_capability_registry helper + test; uses it during v2 init. |
| src/bridge/effect_adapter.rs | Restricts capability registry setter visibility and updates comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nto fix/unified-tool-info
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@henrypark133 until you implement the multi-stage solution for #2767 this PR creates a good fix to expose |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Copilot please stop asking to add a |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // is a no-op. Assumes v1 tool and v2 capability names don't overlap. | ||
| // `tool_opt` carries the Tool forward so the `Summary` arm can ask | ||
| // it for a curated `discovery_summary()`; `None` means the source | ||
| // was a capability action and only `fallback_summary` applies. |
There was a problem hiding this comment.
High Severity
This fixes discovery for capability actions, but discovery is still incomplete because EffectBridgeAdapter::available_actions() also advertises latent provider actions from AuthManager::latent_extension_actions(). Those still will not show up in tool_info / system_tools_list, so the original 'the prompt says this action exists, but discovery says it does not' mismatch remains reproducible for inactive MCP/WASM providers.
Can these built-ins enumerate the same shared action source as available_actions() instead of special-casing only capability-registry actions here?
|
I think we can close this one as the whole system will be redesigned #2826 |
tool_info("mission_create")returned "No tool named 'mission_create' is registered" even thoughmission_createwas advertised in the system prompt and executed correctly.Root cause: two sources of truth for tools: the V1
ToolRegistryand the V2-onlyCapabilityRegistry(holds engine-native actions like missions).EffectBridgeAdapter::available_actionsmerges both when building the system prompt, but the discovery tools only consulted the v1 registry. So the LLM saw mission actions advertised, tried to introspect them, got contradicted, and stopped trusting its own action list.ToolRegistryto carry an optionalArc<CapabilityRegistry>handle.Noneon v1,Someafter the v2 bootstrap wires it.tool_infoandsystem_tools_listsearch both sources. On v1 the capability registry isNoneso the fallback is a no-op; on v2 missions resolve through it.wire_capability_registryinrouter.rs— one call site, one test, locked in.ToolRegistry::register()rejects any dynamic tool whose name (including hyphen/underscore aliases) matches a capability action. WASM and MCP tools namedmission_createormission-createcan no longer shadow capability actions.Note for AI reviewers: Capabilities are v2 right now and do not overlap with v1 internal tools, if somebody breaks that invariant in a future commit they can go and fix it. Trying to impose a
v2check when the registry is created, or appended, always ends up in a new error, so please stop requesting it.Change Type
Linked Issue
closes #2793
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildreview-prorpr-shepherd --fixwas run before requesting reviewSecurity Impact
I added a guardrail so people cannot register tools that are called as capabilities
Database Impact
None
Blast Radius
Tool discovery
Review Follow-Through
Review track: B?