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
29 changes: 28 additions & 1 deletion crates/tinytools-agent/src/repair/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,14 @@ pub fn accepts_object(schema: &Value) -> bool {
/// / `object`, and a bare scalar → `[scalar]` for `array`. A value that does
/// not convert is left as it was, so the schema validator still reports it.
/// Recurses into nested objects and array items with their own schemas.
///
/// An object-typed property given a string that is not strict JSON goes
/// through the same lenient [`super::json::recover_object`] ladder as a whole
/// argument blob (trailing commas, bare keys, a fence, leaked template
/// markers). A `null` for an **optional** property whose declared `type` does
/// not admit null is dropped: the model meant "not given" (`"tool": null`),
/// and the key's absence is what the schema accepts. A required property is
/// never dropped, so the validator still names it.
#[must_use]
pub fn coerce_to_schema(arguments: Value, schema: &Value) -> Value {
let Value::Object(map) = arguments else {
Expand All @@ -122,9 +130,14 @@ pub fn coerce_to_schema(arguments: Value, schema: &Value) -> Value {
let Some(properties) = schema.get("properties").and_then(Value::as_object) else {
return Value::Object(map);
};
let required = schema.get("required").and_then(Value::as_array);
let is_required = |key: &str| required.is_some_and(|list| list.iter().any(|r| r == key));
let mut out = Map::with_capacity(map.len());
for (key, value) in map {
let coerced = match properties.get(&key) {
Some(property) if value.is_null() && !is_required(&key) && rejects_null(property) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

priority medium security likely

Preserve explicit nulls for schema validation

An explicit null is not equivalent to an omitted optional property: the schema deliberately rejects null for this property, and downstream tools may distinguish a missing argument from an explicit null or apply a default when the key is absent. Dropping attacker- or model-controlled nulls silently turns an invalid tool call into a different, valid call and bypasses the validator's error path. Preserve the value for validation, or introduce an explicit, tool-level policy for treating null as omission rather than applying this transformation generically.

[RULE] schema-validation-bypass ·

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

priority medium critique confident

Preserve explicit nulls for schema validation

This drops an explicitly supplied null for every optional property whose simple type rejects null. That changes the caller's input into a different argument object and contradicts the existing contract that values which do not convert are left in place so the schema validator can report them. For example, { "tool": null } with an optional { "tool": { "type": "string" } } becomes {}, allowing the call to proceed as if tool was omitted instead of reporting the invalid explicit null. Keep the value and let schema validation distinguish omission from an explicit null.

[RULE] preserve-input-semantics ·

continue;
}
Some(property) => coerce_value(value, property),
None => value,
};
Expand All @@ -133,6 +146,16 @@ pub fn coerce_to_schema(arguments: Value, schema: &Value) -> Value {
Value::Object(out)
}

/// Whether `schema` declares a `type` that does not include `null`. An
/// untyped property accepts null already, so it is never rewritten.
fn rejects_null(schema: &Value) -> bool {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

priority medium security confident

Honor nullable schemas expressed with composition keywords

This helper only recognizes nullability from the direct type field. A valid schema such as { "anyOf": [{"type":"string"}, {"type":"null"}] } or an equivalent oneOf schema has no direct type, so this helper currently leaves the value intact; however, schemas that combine a non-null direct type with a nullable branch can be incorrectly classified and have explicit nulls dropped. Determine whether the schema accepts null through anyOf, oneOf, enum, or const before removing the property, so normalization does not turn a valid nullable value into an omitted property.

[RULE] schema-nullability ·

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

priority medium critique confident

Honor nullable schemas expressed with composition keywords

This only detects nullability from the top-level type keyword. An optional property such as { "anyOf": [{ "type": "string" }, { "type": "null" }] } returns false here, so an explicit null is retained instead of being treated consistently with a nullable type array. Extend the nullability check to the supported composition keywords (anyOf/oneOf, and any other schema forms accepted by the validator) so a schema that admits null is not treated as rejecting it.

[RULE] schema-nullability ·

match schema.get("type") {
Some(Value::String(kind)) => kind != "null",
Some(Value::Array(kinds)) => !kinds.iter().any(|kind| kind == "null"),
_ => false,
}
}

fn schema_type(schema: &Value) -> Option<&str> {
match schema.get("type") {
Some(Value::String(s)) => Some(s.as_str()),
Expand Down Expand Up @@ -169,7 +192,11 @@ fn coerce_value(value: Value, schema: &Value) -> Value {
(Some("array"), scalar @ (Value::Number(_) | Value::Bool(_))) => Value::Array(vec![scalar]),
(Some("object"), Value::String(s)) => match serde_json::from_str::<Value>(s.trim()) {
Ok(object @ Value::Object(_)) => coerce_to_schema(object, schema),
_ => Value::String(s),
Ok(_) => Value::String(s),
Err(_) => match super::json::recover_object(super::json::strip_code_fence(&s)) {
Some(object) => coerce_to_schema(object, schema),
None => Value::String(s),
},
},
(Some("object"), object @ Value::Object(_)) => coerce_to_schema(object, schema),
(Some("string"), Value::Number(n)) => Value::String(n.to_string()),
Expand Down
68 changes: 68 additions & 0 deletions crates/tinytools-agent/src/repair/test/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,3 +231,71 @@ fn unconvertible_scalars_are_left_for_the_validator() {
json!({ "l": [3] })
);
}

/// `use_skill` captures: the nested `args` object arrives as a JSON string that
/// is not strictly valid (single-quoted keys stripped by the model, a trailing
/// comma, unquoted keys). Strict parsing fails; the lenient object recovery
/// the rest of the repair path uses must still apply.
#[test]
fn an_object_typed_string_that_is_relaxed_json_is_recovered() {
let schema = json!({
"type": "object",
"properties": {
"skill": { "type": "string" },
"args": { "type": "object", "additionalProperties": true }
},
"required": ["skill"]
});
for raw in [
"{\"query\": \"inbox\",}",
"{query: \"inbox\"}",
"```json\n{\"query\":\"inbox\"}\n```",
"{\"query\":\"inbox\"}<tool_call|>",
] {
assert_eq!(
coerce_to_schema(json!({ "skill": "email", "args": raw }), &schema),
json!({ "skill": "email", "args": { "query": "inbox" } }),
"{raw}"
);
}
}

/// `use_skill` captures: `"tool": null` / `"args": null` for optional
/// properties whose declared type does not admit null. The model meant "not
/// given"; dropping the key is what makes the call validate.
#[test]
fn null_for_an_optional_non_nullable_property_is_dropped() {
let schema = json!({
"type": "object",
"properties": {
"skill": { "type": "string" },
"tool": { "type": "string" },
"args": { "type": "object" },
"maybe": { "type": ["string", "null"] },
"untyped": {}
},
"required": ["skill"]
});
assert_eq!(
coerce_to_schema(
json!({ "skill": "email", "tool": null, "args": null, "maybe": null, "untyped": null }),
&schema
),
json!({ "skill": "email", "maybe": null, "untyped": null })
);
}

/// A required property set to null is left for the validator: dropping it
/// would only trade "must be string" for "is required".
#[test]
fn null_for_a_required_property_is_kept() {
let schema = json!({
"type": "object",
"properties": { "skill": { "type": "string" } },
"required": ["skill"]
});
assert_eq!(
coerce_to_schema(json!({ "skill": null }), &schema),
json!({ "skill": null })
);
}
Loading