fix(reborn): align external tool provider names - #5303
Conversation
github-actions
Bot
added
size: L
📝 WalkthroughSummary by CodeRabbit
WalkthroughLocal-dev external tool capability handling now threads ChangesProvider tool name handling at the local-dev boundary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the external tool capability implementation to use the strongly-typed ProviderToolName instead of raw strings, ensuring better alignment with model protocol boundaries. It also adds comprehensive unit tests to verify the mapping and validation of external tool names. The reviewer identified two potential issues where invalid or oversized external tool names could cause the entire visible_capabilities call to fail closed (due to CapabilityId validation failures or length mismatches). It is recommended to handle these error states by skipping (not advertising) the problematic capabilities rather than failing the entire call.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| )); | ||
| } | ||
| let tool_name = provider_tool_name_for_external_tool(spec.name())?; | ||
| let capability_id = external_tool_capability_id(spec.name())?; |
There was a problem hiding this comment.
If an external tool name starts with _ or - (e.g., _my_tool), ProviderToolName::new will succeed because it allows these characters anywhere. However, external_tool_capability_id will sanitize it to _my_tool and attempt to construct CapabilityId::new("external_tool._my_tool"). This will fail because CapabilityId segments must start with a lowercase ASCII letter or digit. Currently, this failure causes the entire visible_capabilities call to fail closed.
To fix this and align with repository rules, capabilities in an error state should not be advertised on any surface and should be handled consistently across all capability kinds. Instead of failing the entire call, handle the error state by not advertising (skipping) this capability.
References
- Capabilities in an error state should not be advertised on any surface and should be handled consistently across all capability kinds.
| fn provider_tool_name_for_external_tool( | ||
| tool_name: &str, | ||
| ) -> Result<ProviderToolName, AgentLoopHostError> { | ||
| ProviderToolName::new(tool_name).map_err(|_| { |
There was a problem hiding this comment.
There is a mismatch between the maximum allowed name length in ExternalToolSpec::new (which is MAX_EXTERNAL_TOOL_NAME_BYTES = 128 in crates/ironclaw_turns/src/external_tool_catalog.rs) and ProviderToolName::MAX_BYTES (which is 64 in crates/ironclaw_host_api/src/ids.rs).
If a client registers an external tool with a name between 65 and 128 bytes, registration will succeed, but visible_capabilities will subsequently fail closed when trying to convert it to a ProviderToolName. To align with repository rules, capabilities in an error state should not be advertised on any surface and should be handled consistently across all capability kinds. Instead of failing the entire visible_capabilities call, ensure that such capabilities in an error state are not advertised.
References
- Capabilities in an error state should not be advertised on any surface and should be handled consistently across all capability kinds.
![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
🤖 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/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs`:
- Around line 135-145: The `provider_tool_name_for_external_tool` helper is
discarding the validation error from `ProviderToolName::new`, so preserve the
underlying `HostApiError` cause instead of using `map_err(|_| ...)`. Update the
mapping in `provider_tool_name_for_external_tool` to carry or log the original
error details before converting to `AgentLoopHostError`, keeping the existing
invalid-invocation summary but adding the specific validation reason for
diagnostics.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aec38548-ec9e-4fad-9a57-a157e7d2fb81
📒 Files selected for processing (2)
crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rscrates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs
| fn provider_tool_name_for_external_tool( | ||
| tool_name: &str, | ||
| ) -> Result<ProviderToolName, AgentLoopHostError> { | ||
| ProviderToolName::new(tool_name).map_err(|_| { | ||
| AgentLoopHostError::new( | ||
| AgentLoopHostErrorKind::InvalidInvocation, | ||
| "external tool name cannot be represented as a provider tool name", | ||
| ) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
.map_err(|_| …) drops the validation cause.
ProviderToolName::new returns a HostApiError saying why the name is invalid (bad char, length, etc.); the |_| closure discards it, leaving only a generic summary. Carry the cause for diagnostics.
Proposed fix — preserve the cause
- ProviderToolName::new(tool_name).map_err(|_| {
- AgentLoopHostError::new(
- AgentLoopHostErrorKind::InvalidInvocation,
- "external tool name cannot be represented as a provider tool name",
- )
- })
+ ProviderToolName::new(tool_name).map_err(|e| {
+ AgentLoopHostError::new(
+ AgentLoopHostErrorKind::InvalidInvocation,
+ format!("external tool name cannot be represented as a provider tool name: {e}"),
+ )
+ })Note: the sibling external_tool_capability_id (Line 127) has the same pattern but is pre-existing/untouched. If you keep summaries fixed for the user boundary, at minimum debug! the bound error before mapping.
As per coding guidelines: "Do not use .map_err(|_| OtherError) — a closure that ignores its error binding and substitutes a generic error drops the underlying cause."
📝 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.
| fn provider_tool_name_for_external_tool( | |
| tool_name: &str, | |
| ) -> Result<ProviderToolName, AgentLoopHostError> { | |
| ProviderToolName::new(tool_name).map_err(|_| { | |
| AgentLoopHostError::new( | |
| AgentLoopHostErrorKind::InvalidInvocation, | |
| "external tool name cannot be represented as a provider tool name", | |
| ) | |
| }) | |
| } | |
| fn provider_tool_name_for_external_tool( | |
| tool_name: &str, | |
| ) -> Result<ProviderToolName, AgentLoopHostError> { | |
| ProviderToolName::new(tool_name).map_err(|e| { | |
| AgentLoopHostError::new( | |
| AgentLoopHostErrorKind::InvalidInvocation, | |
| format!("external tool name cannot be represented as a provider tool name: {e}"), | |
| ) | |
| }) | |
| } |
🤖 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/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs`
around lines 135 - 145, The `provider_tool_name_for_external_tool` helper is
discarding the validation error from `ProviderToolName::new`, so preserve the
underlying `HostApiError` cause instead of using `map_err(|_| ...)`. Update the
mapping in `provider_tool_name_for_external_tool` to carry or log the original
error details before converting to `AgentLoopHostError`, keeping the existing
invalid-invocation summary but adding the specific validation reason for
diagnostics.
Source: Coding guidelines
|
🚅 Deployed to the ironclaw-pr-5303 environment in ironclaw-ci-preview
|
This branch was successfully deployed
1 participant
Summary
CI root cause
PR #5300 does not touch the failing file. The shared failure was a main-branch compile error in crates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs after ProviderToolDefinition and ProviderToolCall moved to ProviderToolName.
Tests