Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,7 @@ fn provider_tool_names_stay_at_model_protocol_boundaries() {
// Composition-local protocol surfaces that reconstruct provider-shaped
// output or local-dev synthetic provider tools.
"crates/ironclaw_reborn_composition/src/openai_compat_serve.rs",
"crates/ironclaw_reborn_composition/src/runtime/local_dev/external_tool_capability.rs",
"crates/ironclaw_reborn_composition/src/runtime/local_dev/synthetic_capability.rs",
"crates/ironclaw_reborn_composition/src/trace_capture.rs",
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use std::collections::HashMap;
use std::sync::{Arc, Mutex as StdMutex};

use async_trait::async_trait;
use ironclaw_host_api::{CapabilityId, InvocationId, RuntimeKind};
use ironclaw_host_api::{CapabilityId, InvocationId, ProviderToolName, RuntimeKind};
use ironclaw_loop_support::{
CapabilityResultWrite, LoopCapabilityInputResolver, LoopCapabilityResultWriter,
};
Expand Down Expand Up @@ -64,11 +64,11 @@ pub(super) fn wrap_local_dev_external_tools(
struct ResolvedSurface {
version: CapabilitySurfaceVersion,
specs_by_capability_id: HashMap<CapabilityId, ToolSpec>,
capability_ids_by_tool_name: HashMap<String, CapabilityId>,
capability_ids_by_tool_name: HashMap<ProviderToolName, CapabilityId>,
}

struct ToolSpec {
tool_name: String,
tool_name: ProviderToolName,
description: String,
parameters_schema: serde_json::Value,
}
Expand All @@ -82,7 +82,7 @@ impl ToolSpec {
capability_id: capability_id.clone(),
provider: None,
runtime: RuntimeKind::System,
safe_name: self.tool_name.clone(),
safe_name: self.tool_name.as_str().to_string(),
safe_description: self.description.clone(),
// External tools are client-side; the host never runs them in
// parallel, and they always park, so mark them exclusive.
Expand Down Expand Up @@ -132,6 +132,17 @@ fn external_tool_capability_id(tool_name: &str) -> Result<CapabilityId, AgentLoo
})
}

fn provider_tool_name_for_external_tool(
tool_name: &str,
) -> Result<ProviderToolName, AgentLoopHostError> {
ProviderToolName::new(tool_name).map_err(|_| {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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
  1. Capabilities in an error state should not be advertised on any surface and should be handled consistently across all capability kinds.

AgentLoopHostError::new(
AgentLoopHostErrorKind::InvalidInvocation,
"external tool name cannot be represented as a provider tool name",
)
})
}

Comment on lines +135 to +145

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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

impl ExternalToolCapabilityPort {
fn surface_version(&self) -> Result<CapabilitySurfaceVersion, AgentLoopHostError> {
self.surface
Expand Down Expand Up @@ -161,7 +172,7 @@ impl ExternalToolCapabilityPort {
.unwrap_or(false)
}

fn capability_id_for_tool_name(&self, tool_name: &str) -> Option<CapabilityId> {
fn capability_id_for_tool_name(&self, tool_name: &ProviderToolName) -> Option<CapabilityId> {
self.surface.lock().ok().and_then(|surface| {
surface
.as_ref()
Expand Down Expand Up @@ -352,6 +363,7 @@ impl LoopCapabilityPort for ExternalToolCapabilityPort {
"external tool name shadows a host capability",
));
}
let tool_name = provider_tool_name_for_external_tool(spec.name())?;
let capability_id = external_tool_capability_id(spec.name())?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
  1. Capabilities in an error state should not be advertised on any surface and should be handled consistently across all capability kinds.

if surface
.descriptors
Expand All @@ -364,9 +376,9 @@ impl LoopCapabilityPort for ExternalToolCapabilityPort {
"external tool conflicts with another capability id",
));
}
capability_ids_by_tool_name.insert(spec.name().to_string(), capability_id.clone());
capability_ids_by_tool_name.insert(tool_name.clone(), capability_id.clone());
let tool_spec = ToolSpec {
tool_name: spec.name().to_string(),
tool_name,
description: spec.description().to_string(),
parameters_schema: spec.parameters_schema().clone(),
};
Expand Down Expand Up @@ -444,3 +456,186 @@ fn catalog_error(_error: ironclaw_turns::ExternalToolCatalogError) -> AgentLoopH
"external tool catalog is unavailable",
)
}

#[cfg(test)]
mod tests {
use super::*;

use ironclaw_host_api::{TenantId, ThreadId};
use ironclaw_loop_support::CapabilityWriteResult;
use ironclaw_turns::{
ExternalToolSpec, InMemoryExternalToolCatalog, RunProfileResolutionRequest,
RunProfileResolver, TurnId, TurnScope,
run_profile::{CapabilityInputRef, InMemoryRunProfileResolver},
};

struct EmptyInnerPort;

#[async_trait]
impl LoopCapabilityPort for EmptyInnerPort {
async fn visible_capabilities(
&self,
_request: VisibleCapabilityRequest,
) -> Result<VisibleCapabilitySurface, AgentLoopHostError> {
Ok(VisibleCapabilitySurface {
version: CapabilitySurfaceVersion::new("test.surface.v1").expect("surface version"),
descriptors: Vec::new(),
})
}

async fn invoke_capability(
&self,
_request: CapabilityInvocation,
) -> Result<CapabilityOutcome, AgentLoopHostError> {
Err(AgentLoopHostError::new(
AgentLoopHostErrorKind::InvalidInvocation,
"test inner port does not execute capabilities",
))
}

async fn invoke_capability_batch(
&self,
_request: CapabilityBatchInvocation,
) -> Result<CapabilityBatchOutcome, AgentLoopHostError> {
Err(AgentLoopHostError::new(
AgentLoopHostErrorKind::InvalidInvocation,
"test inner port does not execute capability batches",
))
}
}

struct TestInputResolver;

#[async_trait]
impl LoopCapabilityInputResolver for TestInputResolver {
async fn resolve_capability_input(
&self,
_run_context: &LoopRunContext,
_input_ref: &CapabilityInputRef,
) -> Result<serde_json::Value, AgentLoopHostError> {
Err(AgentLoopHostError::new(
AgentLoopHostErrorKind::InvalidInvocation,
"test input resolver does not resolve inputs",
))
}
}

struct TestResultWriter;

#[async_trait]
impl LoopCapabilityResultWriter for TestResultWriter {
async fn write_capability_result(
&self,
_write: CapabilityResultWrite<'_>,
) -> Result<CapabilityWriteResult, AgentLoopHostError> {
Err(AgentLoopHostError::new(
AgentLoopHostErrorKind::InvalidInvocation,
"test result writer does not write results",
))
}
}

async fn run_context() -> LoopRunContext {
let resolved = InMemoryRunProfileResolver::default()
.resolve_run_profile(RunProfileResolutionRequest::interactive_default())
.await
.expect("profile resolves");
LoopRunContext::new(
TurnScope::new(
TenantId::new("tenant-external-tools").expect("tenant id"),
None,
None,
ThreadId::new("thread-external-tools").expect("thread id"),
),
TurnId::new(),
TurnRunId::new(),
resolved,
)
}

fn external_tool_spec(name: &str) -> ExternalToolSpec {
ExternalToolSpec::new(
name,
"client-side external tool",
serde_json::json!({"type": "object"}),
)
.expect("external tool spec")
}

async fn wrapped_port_with_specs(
specs: Vec<ExternalToolSpec>,
) -> (Arc<dyn LoopCapabilityPort>, LoopRunContext) {
let run_context = run_context().await;
let catalog = Arc::new(InMemoryExternalToolCatalog::new());
catalog
.register(run_context.run_id, specs)
.await
.expect("register external tools");
let catalog: Arc<dyn ExternalToolCatalog> = catalog;
(
wrap_local_dev_external_tools(
Arc::new(EmptyInnerPort),
run_context.clone(),
Arc::new(TestInputResolver),
Arc::new(TestResultWriter),
catalog,
),
run_context,
)
}

#[tokio::test]
async fn external_tool_surface_maps_provider_name_to_capability_id() {
let (port, _run_context) =
wrapped_port_with_specs(vec![external_tool_spec("client_tool")]).await;

let surface = port
.visible_capabilities(VisibleCapabilityRequest)
.await
.expect("visible capabilities");
assert_eq!(surface.descriptors.len(), 1);
assert_eq!(
surface.descriptors[0].capability_id.as_str(),
"external_tool.client_tool"
);
assert_eq!(surface.descriptors[0].safe_name, "client_tool");

let definitions = port.tool_definitions().expect("tool definitions");
assert_eq!(definitions.len(), 1);
assert_eq!(definitions[0].name.as_str(), "client_tool");

let ids = port
.provider_tool_call_capability_ids(&ProviderToolCall {
provider_id: "test-provider".to_string(),
provider_model_id: "test-model".to_string(),
turn_id: Some("turn-1".to_string()),
id: "call-1".to_string(),
name: ProviderToolName::new("client_tool").expect("provider tool name"),
arguments: serde_json::json!({}),
response_reasoning: None,
reasoning: None,
signature: None,
})
.expect("capability ids");
assert_eq!(
ids.provider_capability_id.as_str(),
"external_tool.client_tool"
);
}

#[tokio::test]
async fn external_tool_surface_rejects_names_that_are_not_provider_safe() {
let (port, _run_context) =
wrapped_port_with_specs(vec![external_tool_spec("client.tool")]).await;

let error = port
.visible_capabilities(VisibleCapabilityRequest)
.await
.expect_err("invalid external tool name should fail closed");
assert_eq!(error.kind, AgentLoopHostErrorKind::InvalidInvocation);
assert_eq!(
error.safe_summary,
"external tool name cannot be represented as a provider tool name"
);
}
}
Loading