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 @@ -741,7 +741,7 @@ mod tests {
#[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("ClientTool")]).await;
wrapped_port_with_specs(vec![external_tool_spec("client_tool")]).await;

let surface = port
.visible_capabilities(VisibleCapabilityRequest)
Expand All @@ -750,21 +750,21 @@ mod tests {
assert_eq!(surface.descriptors.len(), 1);
assert_eq!(
surface.descriptors[0].capability_id.as_str(),
"external_tool.clienttool"
"external_tool.client_tool"
);
assert_eq!(surface.descriptors[0].safe_name, "clienttool");
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(), "clienttool");
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("clienttool").expect("provider tool name"),
name: ProviderToolName::new("client_tool").expect("provider tool name"),
arguments: serde_json::json!({}),
response_reasoning: None,
reasoning: None,
Expand All @@ -773,7 +773,7 @@ mod tests {
.expect("capability ids");
assert_eq!(
ids.provider_capability_id.as_str(),
"external_tool.clienttool"
"external_tool.client_tool"
);
}

Expand Down
75 changes: 74 additions & 1 deletion crates/ironclaw_reborn_openai_compat/src/responses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ pub struct OpenAiResponsesCreateRequest {
pub tools: Option<Vec<serde_json::Value>>,
#[serde(default, skip_serializing_if = "Option::is_none")]
pub tool_choice: Option<serde_json::Value>,
/// IronClaw extension: structured context injected into the submitted turn.
#[serde(default, alias = "context", skip_serializing_if = "Option::is_none")]
pub x_context: Option<serde_json::Value>,
}

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
Expand All @@ -30,7 +33,7 @@ pub enum OpenAiResponsesInput {
Items(Vec<OpenAiResponsesInputItem>),
}

#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
#[derive(Debug, Clone, PartialEq, Serialize)]
#[serde(tag = "type", rename_all = "snake_case")]
pub enum OpenAiResponsesInputItem {
Message {
Expand All @@ -48,6 +51,76 @@ pub enum OpenAiResponsesInputItem {
},
}

impl<'de> Deserialize<'de> for OpenAiResponsesInputItem {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
let value = serde_json::Value::deserialize(deserializer)?;
let object = value
.as_object()
.ok_or_else(|| de::Error::custom("responses input item must be an object"))?;
match object.get("type").and_then(serde_json::Value::as_str) {
Some("message") => {
#[derive(Deserialize)]
struct MessageWire {
role: OpenAiResponsesMessageRole,
content: serde_json::Value,
}
let wire = MessageWire::deserialize(value).map_err(de::Error::custom)?;
Ok(Self::Message {
role: wire.role,
content: wire.content,
})
}
None if object.contains_key("role") => {
Comment on lines +63 to +76

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject non-string type values instead of treating them as legacy messages.

object.get("type").and_then(serde_json::Value::as_str) makes "type"-missing and "type"-present-but-not-a-string indistinguishable. A malformed payload like {"type": 1, "role": "user", "content": "hi"} will therefore hit the legacy role branch and be accepted as OpenAiResponsesInputItem::Message instead of failing at the request boundary. Match on object.get("type") directly so only an actually absent type can use the legacy fallback, and add a DTO regression test for the non-string case. As per path instructions, Fail loud at request boundaries, and as per coding guidelines, Use enums with #[serde(rename_all = "snake_case")] or explicit #[serde(rename = "...")] for fixed small sets instead of string comparisons.

🤖 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_openai_compat/src/responses.rs` around lines 63 - 76,
The deserialization in responses.rs is treating a present non-string "type" the
same as an absent one, which lets malformed payloads fall back to the legacy
role-based Message path. Update the matching logic in the
OpenAiResponsesInputItem deserializer to inspect object.get("type") directly so
only a truly missing type can use the legacy branch, and reject any non-string
type value before reaching the fallback. Keep the existing MessageWire /
OpenAiResponsesInputItem::Message handling for valid "message" values, and add a
regression test covering a payload with "type": 1 to ensure it fails at the
boundary.

Sources: Coding guidelines, Path instructions

#[derive(Deserialize)]
struct MessageWire {
role: OpenAiResponsesMessageRole,
content: serde_json::Value,
}
let wire = MessageWire::deserialize(value).map_err(de::Error::custom)?;
Ok(Self::Message {
role: wire.role,
content: wire.content,
})
}
Some("function_call") => {
#[derive(Deserialize)]
struct FunctionCallWire {
call_id: String,
name: String,
arguments: String,
}
let wire = FunctionCallWire::deserialize(value).map_err(de::Error::custom)?;
Ok(Self::FunctionCall {
call_id: wire.call_id,
name: wire.name,
arguments: wire.arguments,
})
}
Some("function_call_output") => {
#[derive(Deserialize)]
struct FunctionCallOutputWire {
call_id: String,
output: serde_json::Value,
}
let wire = FunctionCallOutputWire::deserialize(value).map_err(de::Error::custom)?;
Ok(Self::FunctionCallOutput {
call_id: wire.call_id,
output: wire.output,
})
}
Some(other) => Err(de::Error::custom(format!(
"unsupported responses input item type: {other}"
))),
None => Err(de::Error::custom(
"responses input item is missing type or role",
)),
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "lowercase")]
pub enum OpenAiResponsesMessageRole {
Expand Down
85 changes: 82 additions & 3 deletions crates/ironclaw_reborn_openai_compat/src/responses_workflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ use ironclaw_product_adapters::{
const DEFAULT_RESPONSES_WAIT_TIMEOUT: Duration = Duration::from_secs(30);
const DEFAULT_BIND_INTERNAL_REFS_TIMEOUT: Duration = Duration::from_secs(2);
const MAX_RESPONSES_BODY_BYTES: usize = 4 * 1024 * 1024;
const MAX_RESPONSES_CONTEXT_BYTES: usize = 10 * 1024;
const MAX_RESPONSES_INPUT_ITEMS: usize = 1_000;
const OPENAI_COMPAT_CONVERSATION_PREFIX: &str = "response";

Expand Down Expand Up @@ -887,9 +888,36 @@ fn validate_responses_supported_fields(
"tool_choice".to_string(),
)));
}
if let Some(context) = &request.x_context
&& serialized_json_len(context) > MAX_RESPONSES_CONTEXT_BYTES
{
return Err(OpenAiCompatHttpError::invalid_request(Some(
"x_context".to_string(),
)));
}
Comment thread
ilblackdragon marked this conversation as resolved.
Ok(())
}

fn serialized_json_len(value: &serde_json::Value) -> usize {
struct CountingWriter(usize);

impl std::io::Write for CountingWriter {
fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> {
self.0 = self.0.saturating_add(buf.len());
Ok(buf.len())
}

fn flush(&mut self) -> std::io::Result<()> {
Ok(())
}
}

let mut writer = CountingWriter(0);
serde_json::to_writer(&mut writer, value)
.map(|_| writer.0)
.unwrap_or(usize::MAX)
}

fn accepted_ack_from_ack(
mut ack: ProductInboundAck,
) -> Result<ProductInboundAck, OpenAiCompatHttpError> {
Expand Down Expand Up @@ -1025,6 +1053,11 @@ fn responses_input_to_product_text(
) -> Result<String, OpenAiCompatHttpError> {
let input = match &request.input {
OpenAiResponsesInput::Text(text) => {
if text.trim().is_empty() {
return Err(OpenAiCompatHttpError::invalid_request(Some(
"input".to_string(),
)));
}
vec![serde_json::json!({
"type": "message",
"role": "user",
Expand All @@ -1040,16 +1073,62 @@ fn responses_input_to_product_text(
items.iter().map(response_input_item_to_value).collect()
}
};
serde_json::to_string(&serde_json::json!({
let mut payload = serde_json::json!({
"format": "openai_compat.responses_input.v1",
"instructions": request
.instructions
.as_ref()
.filter(|value| !value.is_empty())
.map(|value| sanitize_product_text_fragment(value)),
"input": input,
}))
.map_err(|_| OpenAiCompatHttpError::internal())
});
if let Some(context) = &request.x_context {
payload["context"] = serde_json::Value::String(responses_context_to_product_text(context));
}
serde_json::to_string(&payload).map_err(|_| OpenAiCompatHttpError::internal())
}

fn responses_context_to_product_text(context: &serde_json::Value) -> String {
use std::fmt::Write as _;

let Some(object) = context.as_object() else {
let val_str = context_value_to_product_text(context);
return format!("[Context: {val_str}]");
};

let mut result = String::new();
for (index, (key, value)) in object.iter().enumerate() {
if index > 0 {
result.push('\n');
}
let key = sanitize_product_text_fragment(key);
match value.as_object() {
Some(inner) => {
let mut fields = String::new();
for (field_index, (field, value)) in inner.iter().enumerate() {
if field_index > 0 {
fields.push_str(", ");
}
let field = sanitize_product_text_fragment(field);
let value = context_value_to_product_text(value);
let _ = write!(&mut fields, "{field}: {value}");
}
let _ = write!(&mut result, "[Context: {key} - {fields}]");
}
None => {
let value = context_value_to_product_text(value);
let _ = write!(&mut result, "[Context: {key}: {value}]");
}
}
}
result
}

fn context_value_to_product_text(value: &serde_json::Value) -> String {
match value {
serde_json::Value::String(text) => sanitize_product_text_fragment(text),
other => sanitize_product_text_fragment(&other.to_string()),
}
}
Comment thread
ilblackdragon marked this conversation as resolved.

fn response_input_item_to_value(item: &OpenAiResponsesInputItem) -> serde_json::Value {
Expand Down
14 changes: 14 additions & 0 deletions crates/ironclaw_reborn_openai_compat/tests/dto_contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,20 @@ fn responses_items_are_tagged_and_tolerate_future_request_fields() {
));
}

#[test]
fn responses_explicit_message_item_missing_role_reports_missing_role() {
let error = serde_json::from_value::<OpenAiResponsesInputItem>(json!({
"type": "message",
"content": "hello"
}))
.expect_err("explicit message items without a role must reject as malformed messages");

assert!(
error.to_string().contains("missing field `role`"),
"unexpected error: {error}"
);
}

#[test]
fn request_dtos_reject_missing_required_fields() {
serde_json::from_value::<OpenAiChatCompletionRequest>(json!({
Expand Down
Loading
Loading