Parse and validate tool inputs at the Core boundary - #165
Parse and validate tool inputs at the Core boundary#165cunninghamcard-bit wants to merge 20 commits into
Conversation
17420c6 to
c4d46a7
Compare
c61092d to
cdb2497
Compare
Providers deliver the model's raw tool-argument text; Core parses it as exact JSON and validates it against the function tool's JSON Schema, following the AI SDK parseToolCall contract. Invalid calls are returned as tool calls marked invalid with a typed error (NoSuchTool / InvalidToolInput / ToolCallRepair) instead of silently passing raw strings through. The optional repair_tool_call callback gets one attempt and its replacement is revalidated from scratch. Provider-executed dynamic calls bypass the tool-set lookup, as in the AI SDK. The C ABI reports the new variants under the existing Tool error code; Node types carry them fully.
The golden variant-set test only asserted the wire tags of the variants it already knew, so an added variant sailed past a test named "exactly thirteen". Extend the list to the three tool-contract variants (the dead legacy catch-all Tool is gone), snapshot their exact JSON payloads, and add a compile-time exhaustive match, so any future addition fails the build of this test instead of relying on the runtime assertion.
cd589d5 to
716d40a
Compare
CI's clippy is newer than the workspace MSRV allows us to follow: as_chunks::<N>() is stable only since Rust 1.88 while the MSRV is 1.85, so trace/hash.rs and openai/embedding.rs keep chunks_exact behind unknown_lints; ws.rs's tungstenite error is consumed by its single caller, so boxing it buys nothing. Same attributes and wording as arcships#163/arcships#164, so whichever lands second is a no-op in these files.
716d40a to
deddae6
Compare
… end Port of the follow-up round built in the stacked worktree, rebased onto the master-based branch: - generate: assemble response messages in stream order (text/reasoning segment flushing, provider_executed on replayed tool calls), matching toResponseMessages; response_tool_call_input replaces the narrower response_message_input (JS 'typeof x === "object"' also admits arrays). - openai outputs: buffer raw tool arguments across repair so invalid streams replay verbatim. - anthropic (direct + vertex): collapse bash/text-editor code-execution variants into the caller's single code_execution tool; the wire name survives in the normalized input 'type'. Google/xAI/HuggingFace conversions aligned the same way. - bindings: thought_signature + provider_executed + arbitrary-JSON provider_metadata on ToolCall across Go/Java/Kotlin/Swift/Flutter/ Node/Python; wire-format fixtures extended. Deliberately not ported (coupled to the retry PR's machinery, restored when that branch rebases onto this one): timeout/abort racing of the repair callback and its six tests, recoverable stream-frame handling, anthropic_stream_error.
Mirror the AI SDK's file split: to-response-messages logic moves out of
generate.rs into response_messages.rs as a ResponseMessageBuilder that owns
all assembly state (parts, text/reasoning buffers, provider options, the
reasoning aggregate). Both the streaming consume loop and the non-streaming
content loop shrink to per-part method calls; the twenty-odd inline flush
sequences collapse into the builder's flush discipline.
Also aggregate the remaining provider duplication: the streamed tool-input
finalizer (empty input -> "{}" + code-execution wire-name re-wrap) that was
inlined in both the Anthropic and Vertex-Anthropic stream loops becomes
anthropic::stream::finalize_streamed_tool_input. raw_tool_input moves next
to the rest of the parse contract in tool.rs.
tool.rs mixed the tool type definitions with the parse contract. Match the AI SDK's split: parse_tool_call.rs now owns RawToolCall, the repair callback and its context, parse_tool_call, JSON parsing with the prototype-pollution guard, schema validation, and raw_tool_input; tool.rs keeps only the types (Tool/FunctionTool/ProviderTool/ToolCall/ToolResult/ToolChoice). No compatibility re-exports: importers move to aimux_core::parse_tool_call.
GenerateContent::ToolCall.input was a serde_json::Value that only ever carried the provider's raw argument text wrapped in Value::String — the contract lived in a doc comment, so nothing stopped a provider from parsing the arguments itself and handing Core a structured value. Typing the field as String makes 'providers never parse tool input' a compile error to violate, and drops the unwrap dance on the Core side. The wire format is unchanged: Value::String(text) and String serialize identically, so no binding or fixture moves. StreamPart::ToolCall.input stays a Value: unlike GenerateContent it is dual-use — providers emit raw text, Core replaces it in-flight with the parsed value, and the user-facing stream sees the parsed one. Separating those two directions means splitting the stream-part type the way the AI SDK splits LanguageModelV2StreamPart from TextStreamPart, which is its own PR.
…rse-repair # Conflicts: # aimux-provider-utils/src/ws.rs
- anthropic/stream.rs: clippy 1.98's manual_unwrap_or fires on the wire-name match. Its suggested `unwrap_or` is not equivalent (an unrecognized Some(name) must collapse to code_execution, not pass through), so express the same rule with filter + unwrap_or. - kotlin Types.kt: the merge added providerMetadata to ToolCall twice, which kotlinx.serialization rejects as a duplicate serial name.
eric8810
left a comment
eric8810
left a comment
There was a problem hiding this comment.
The Core-owned parsing direction is necessary and broadly matches the intended upstream behavior, but this version still has two correctness regressions and one understated compatibility break that should be resolved before merge. In particular, invalid primitive JSON arguments are not preserved exactly in OpenAI-compatible output, and Cohere streaming still terminates on malformed arguments instead of delegating parsing to Core. I ran the focused Core/provider suites (63 tests passed) and added a temporary regression test that reproduces the primitive-string corruption (\"hello\" is emitted as hello). Please also update the PR description: the wire enum has 15 variants, and the C ABI assigns three distinct new error codes (15/16/17).
| match input { | ||
| // Core uses a string carrier to retain malformed input on invalid | ||
| // calls; emitting it verbatim avoids adding a second JSON layer. | ||
| Value::String(raw) if invalid == Some(true) => raw.clone(), |
There was a problem hiding this comment.
[P1] Preserve the exact raw arguments instead of overloading Value::String. A syntactically valid primitive such as \"hello\" becomes Value::String(\"hello\") when its schema expects an object. This branch then emits hello, which is no longer valid JSON; Value::Null is likewise rewritten to {} below. The temporary regression test reproduces this. Please keep the provider's raw argument text as a separate field/representation so valid JSON strings, malformed text, and null remain distinguishable.
There was a problem hiding this comment.
Fixed in d079814 / 1bc4866 / b76361b: invalid calls render the raw text carried by InvalidToolInput.tool_input (also unwrapped through ToolCallRepair.original_error), valid calls render compact JSON of the parsed value, null stays null, blank text renders as {}. One table-driven end-to-end test covers the three error shapes. Known gap, documented in code: NoSuchTool with a bare JSON-string argument still renders unquoted, since that variant carries no raw text (same as the AI SDK).
| provider_metadata: None, | ||
| }); | ||
| let text = if trimmed.is_empty() { "{}" } else { trimmed }; | ||
| match serde_json::from_str::<Value>(text) { |
There was a problem hiding this comment.
[P1] Let Core parse malformed Cohere streaming arguments. This provider-side parse turns malformed tool input into a terminal stream error, so the call never reaches the new Core boundary as a retained invalid: true tool call. It also differs from the non-streaming Cohere path. Please emit the original accumulated text in StreamPart::ToolCall and let Core own parsing, validation, and repair as this PR promises.
There was a problem hiding this comment.
Fixed in 61e0076: tool-call-end forwards the accumulated text verbatim (blank → {}) and no longer ends the stream on a parse failure; Core owns parsing, validation and repair on both paths. malformed_streamed_tool_call_arguments_do_not_error_the_stream asserts the stream completes with an invalid: true call. No other provider parses provider-side.
| /// The model's raw argument text, exactly as the provider delivered | ||
| /// it (possibly malformed). Providers never parse it — `generate_text` | ||
| /// owns parsing, schema validation, and repair. | ||
| input: String, |
There was a problem hiding this comment.
[P2] Account for the public raw-result shape change. This changes GenerateResult.raw.content for valid calls too: callers that previously received a JSON object now receive JSON-encoded text, and persisted object-shaped results no longer deserialize into this type. The PR currently describes the break as affecting callers that relied on invalid provider arguments, which understates the scope. Please either provide a compatibility path or explicitly document the full migration and cover it in cross-binding serialization tests.
There was a problem hiding this comment.
Agreed the break was broader than stated. Fixed in 177aeed / 2ec1f5e: GenerateContent::ToolCall.input deserializes from both the legacy JSON value and the new string (legacy values re-serialize to compact text) and always serializes as a string; one test loads both shapes. docs/api/gaps.md §9 now documents the wire shape before and after, and the PR description is corrected.
parsed_tool_call_arguments() used Value::String plus the invalid flag as a
carrier for the provider's raw argument text, but a legitimately parsed JSON
string (e.g. "hello") and malformed text wrapped as a string fallback both
serialize to the identical Value::String, so a valid quoted string got
re-emitted unquoted (invalid JSON), and Value::Null was unconditionally
rewritten to "{}" even for valid calls.
AiMuxError::InvalidToolInput already carries tool_input: the byte-for-byte
provider text, set from RawToolCall.input on both the JSON-parse-failure and
schema-validation-failure paths. Use it directly for invalid calls instead of
re-deriving anything from the parsed Value, and drop the Null special case so
a valid call's null round-trips as null instead of {}. Threads the resolved
AiMuxError into the four call sites (streaming StreamPart::ToolCall and the
non-streaming generate_text_as_openai path) that build OpenAI-compatible
tool_calls[].function.arguments.
…self
Cohere's tool-call-end handler parsed the accumulated arguments and treated
a parse failure as a terminal stream Error, unlike every other streaming
provider (which forwards the raw text unparsed) and unlike Cohere's own
non-streaming path. A malformed streamed call never reached Core as a
retained invalid: true tool call — it aborted the whole stream instead.
Forward the trimmed accumulated text verbatim (empty still defaults to "{}"
to match the TS provider), unparsed, and let Core's parse_tool_call own
JSON parsing, schema validation, and repair, exactly as it already does for
every other provider. Audited every other StreamPart::ToolCall construction
site in aimux-providers (open_responses, xai, google, bedrock, anthropic,
vertex, huggingface, mistral, openai) for the same anti-pattern; none of
them parse provider-side.
should_stream_tool_call_deltas no longer expects compact-reserialized
arguments (interior whitespace from the deltas now survives, since nothing
parses and re-serializes it), and a new test drives a malformed streamed
call through both do_stream() directly (no Error, raw text forwarded) and
stream_text() (Core keeps it as an invalid: true call with a typed
InvalidToolInput error instead of erroring the stream).
…input GenerateContent::ToolCall.input changed from serde_json::Value to String (providers now hand Core raw text; Core owns parsing). The PR description called this a break only for invalid arguments, but it understated the blast radius: a GenerateResult persisted or replayed from before the refactor — back when tool input was already-parsed and object/array-shaped on this field — no longer deserializes at all, valid calls included. Add a deserialize_with that accepts both shapes: a JSON string loads unchanged (the current, and only, wire shape this version writes), and any other JSON value re-serializes to its compact JSON text. Serialization is untouched (always a plain string). Documents the wire shape and the compatibility behavior in docs/api/gaps.md §9. Regression tests cover both directions (a legacy object-shaped payload and the current string-shaped payload both load to the same value) and every legacy value shape (array/number/bool/null), plus that serialization never regresses to the legacy shape.
…r calls The first tool-input-parse-repair fix on this branch only recovered the provider's verbatim raw text from AiMuxError::InvalidToolInput, missing two other invalid-call paths that hit the same Value::String ambiguity: - ToolCallRepair (a failed repair *callback*, not a re-validation failure) wraps the pre-repair error as `original_error` without being unwrapped, so parsed_tool_call_arguments fell through to re-deriving text from the ambiguous Value. - NoSuchTool carries no tool_input field at all — like the AI SDK, it fires off the tool name alone, before the arguments are looked at — so invalid_tool_call's best-effort `serde_json::from_str` still ran and a valid quoted string such as "hello" lost its quotes on the way back out. parse_tool_call::invalid_tool_call now skips the best-effort parse entirely for NoSuchTool (recursing through ToolCallRepair.original_error), storing the unparsed raw text in `input: Value::String` instead — the same contract InvalidToolInput's fallback already had for malformed text, just applied before any parse attempt. openai_output::parsed_tool_call_arguments gained a raw_tool_call_text helper that reads InvalidToolInput.tool_input, unwraps one level of ToolCallRepair, or reads the now-guaranteed-unparsed Value::String for NoSuchTool. AiMuxError's own shape and wire format are untouched — this only changes how ToolCall.input and the rendered OpenAI arguments text are derived from it.
The previous commit stopped invalid_tool_call from parsing the raw text for
NoSuchTool, to keep a bare JSON string (`"hello"`) from losing its quotes on
the OpenAI-compat arguments wire. That traded a rare cosmetic glitch for a
common data loss: response_messages only carries a structured input into the
next turn's transcript (matching to-response-messages.ts, which replaces a
non-object invalid input with `{}`), so a model calling a misspelled tool
with perfectly good `{"city":"Tokyo"}` arguments replayed as `{}` — the
arguments were dropped.
Restore the AI SDK's unconditional best-effort parse (parse-tool-call.ts
catch-all: parsed value when the text is valid JSON, verbatim text
otherwise) for every invalid-call error, NoSuchTool included, and drop
input_was_never_parsed. raw_tool_call_text keeps its NoSuchTool arm: a
string left on `input` is either malformed text kept verbatim or a genuine
JSON string, indistinguishable without a raw-text field on the error, and
only the malformed case occurs in practice — now documented as such.
Blank argument text parses as `{}` and can still fail schema validation
against required properties. On that path parsed_tool_call_arguments
returned the recovered raw text verbatim — the empty string — which is not
valid JSON in an OpenAI `tool_calls[].function.arguments` field.
to_chat_completion already normalized empty text to `{}` for the unparsed
non-streaming path; apply the same rule to the parsed renderer, and make
to_chat_completion's check whitespace-tolerant so the two agree.
gaps.md §9 and the deserializer's own doc comment both claimed the wire
format was unchanged for results written by this version, because providers
had "only ever put the raw text in a Value::String". Neither is true: on
master the providers parsed their own arguments into that field (e.g.
openai/model.rs ran serde_json::from_str, google/model.rs stored the
response object directly), so `input` really did go from
`{"city":"Paris"}` to `"{\"city\":\"Paris\"}"` — as this PR's own
contract-tests/fixtures/wire-format.json change shows.
State the before/after wire shape, that deserialization accepts both, and
that the parsed value still reaches callers on
GenerateTextResult.tool_calls[].input; drop the rest.
Six parsed_tool_call_arguments unit tests and three near-identical
end-to-end stream tests covered one contract with different data: the raw
argument text of an invalid call must reach an OpenAI-compatible client
verbatim. Fold them into one table-driven end-to-end test over the three
distinct error shapes (schema-rejected JSON string, NoSuchTool, failed
repair callback), which also pins the error/`input` pairing parse_tool_call
produces — something the hand-built unit tests could not.
Keeps the two tests pinning separate contracts (a valid `null` input is not
rewritten to `{}`; blank text is). Behaviour unchanged: 286 deleted,
66 added.
Three tests covered the compatibility deserializer: one loading both wire shapes, one repeating that over every non-string JSON value, and one asserting serialization stays a string. The second only varied the data; the third is two lines inside the first. Keep one test that loads both shapes and checks the round-trip.
The golden error snapshots pinned NoSuchTool-with-available-tools, InvalidToolInput, and ToolCallRepair twice each, the second copy differing only in its data — the wire shape is what the snapshot pins. Keep one case per shape (NoSuchTool stays twice: `skip_serializing_if` makes its payload vary with `available_tools`). The provider-utils test asserting three error_variant log reasons only restated three string constants from a match a compile error already guards.
|
@eric8810 All three findings are addressed (replies on each thread point to the commits) and the description is corrected: 15 wire variants, three distinct C codes 15/16/17, and the full |
|
Review follow-up: all three findings from my Sept 3 review are addressed — raw tool-call arguments are now preserved verbatim (including the 09-07 follow-up for calls that remain invalid), Cohere streaming no longer parses tool-call input locally and delegates to Core, and the description correctly states 15 wire variants with C ABI codes 15/16/17. Thank you. The remaining blocker is mechanical: #164 was squash-merged to master as d5b8323, and this branch now conflicts. Please rebase onto current master and re-push so CI re-runs. As anticipated in the PR description, the overlap should be mostly textual; the three clippy- |
2 participants
Summary
Move tool-call input parsing and validation to the Core boundary, following the AI SDK
parseToolCall/repairToolCallcontract.generate_textandstream_textparse exact JSON and validate it against the selected function tool's JSON Schema.invalid: trueand a typed error instead of being dropped or silently passing an unparsed string through.repair_tool_callcallback gets one attempt; its replacement is parsed and validated again from scratch.Behavior
NoSuchTool, including the available tool names when a tool set was supplied.InvalidToolInput, preserving the original argument text and parsed value when available.ToolCallRepair, preserving both the original validation error and the repair failure.do_streamcontinues to expose raw serialized arguments; the user-facingstream_textresult exposes parsed input after the complete tool call is assembled.This is a breaking behavior change in two places:
invalid: truecall with a typed error.GenerateContent::ToolCall.inputinGenerateResult.raw.contentchanges from the provider's parsed JSON value to the raw argument text (a JSON string) for valid calls as well. Deserialization accepts both the legacy value shape and the new string shape, so persisted results still load; serialization always emits the string. Documented indocs/api/gaps.md§9.Public contract and bindings
AiMuxErrorto 15 wire variants. The C ABI assigns three distinct new codes:AIMUX_E_NO_SUCH_TOOL = 15,AIMUX_E_INVALID_TOOL_INPUT = 16,AIMUX_E_TOOL_CALL_REPAIR = 17, mirrored in every binding and its docs.provider_metadata,invalid, anderrorthrough the top-level and streaming tool-call surfaces in Node, Python, Go, Java, Kotlin, Swift, and Flutter.repair_tool_callis serde-skipped and cannot cross the JSON/FFI boundary, so only Rust callers can install the callback. Other bindings still receive the invalid call and typed failure as data and can implement an application-level repair loop. This limitation is recorded indocs/api/gaps.md.Scope
This PR is based directly on
masterand is independent of #164. It contains no operation-retry, timeout-pipeline, or recoverable-stream changes.Verification
cargo fmt --all -- --checkaimux-coreandaimux-providerstest suites