Repository navigation
Add agent tool protocol crate - #5
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 9 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Changes requested Review snapshot
Completeness: Complete What changedThe review could not produce a supported behavioral summary; inspect the cited changed surface and lane details below. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. Findings
Before merge
How this fits togetherflowchart LR
n0["response"]:::impacted
n1["weather_schema"]:::impacted
n2["...ture_matches_what_the_parser_reconstructs"]:::impacted
n3["...ed_registry_delegates_all_text_operations"]:::impacted
n4["...ect_values_fallback_and_protocol_metadata"]:::impacted
n2 -->|calls| n0
n2 -->|tests| n0
n2 -->|calls| n1
n2 -->|tests| n1
n3 -->|calls| n0
n3 -->|tests| n0
n3 -->|calls| n1
n3 -->|tests| n1
n4 -->|calls| n0
n4 -->|tests| n0
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
✨ Finishing Touches📝 Generate docstrings
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f08b1c3edf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db93caeeb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !combined.iter().any(|call| { | ||
| call.name == canonical.name | ||
| && call.arguments == canonical.arguments | ||
| && call.id == canonical.id | ||
| }) { | ||
| combined.push(canonical); |
There was a problem hiding this comment.
Fresh evidence after ea579cc: this merge still treats invocations as an unordered set. When an attribute-form <invoke> precedes a P-Format tag, the canonical call is appended after the positional call, reversing source order; when both calls have identical names and arguments, the .any(...) check drops the canonical occurrence entirely. This can execute side-effecting calls out of order or only once, so merge calls by source occurrence rather than value equality.
Useful? React with 👍 / 👎.
| let result_ids: std::collections::BTreeSet<&str> = results | ||
| .iter() | ||
| .map(|result| result.tool_call_id.as_str()) | ||
| .collect(); | ||
| if !opener_ids.is_empty() && opener_ids == result_ids { |
There was a problem hiding this comment.
Converting both sides to BTreeSet discards duplicate IDs, so one opener with ID call_1 and two persisted results for call_1 passes this equality check. Native replay then emits two tool messages for one call, leaving the second result orphaned and causing the provider rejection this repair function is intended to prevent; compare counts as well as unique IDs.
Useful? React with 👍 / 👎.
| id: None, | ||
| }); | ||
| if let Some(r) = raw { | ||
| cleaned_text = cleaned_text.replace(r, ""); |
There was a problem hiding this comment.
When the same GLM spelling also appears inside narrative text, replace removes every occurrence rather than only the line that was parsed as a call. For example, Use shell/command>ls carefully followed by a standalone shell/command>ls call returns narrative Use carefully; track the matched line span and remove only that occurrence.
Useful? React with 👍 / 👎.
| /// One flat chat message, as a dialect emits it toward a provider. | ||
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| pub struct DialectMessage { |
There was a problem hiding this comment.
The new serde-derived protocol types have no unit test asserting their exact serialized field names, optional-field behavior, and role spellings. These values are the provider/host wire contract, so an otherwise harmless rename or serde-attribute change can become a runtime decode failure without a regression test; add an exact representation test covering the new payload types.
AGENTS.md reference: AGENTS.md:L170-L172
Useful? React with 👍 / 👎.
| //! Agent-facing tool-call protocols. | ||
| //! | ||
| //! `tinytools` owns the vocabulary a callable tool exposes. This crate owns the | ||
| //! model-facing protocol around those declarations: parsing calls, rendering | ||
| //! catalogues and result blocks, and replaying tool cycles. |
There was a problem hiding this comment.
The new crate overview explains the design but contains no runnable rustdoc example demonstrating its primary API. Add a short example so the public entry points are exercised as documentation and cannot drift from the implementation.
AGENTS.md reference: AGENTS.md:L197-L201
Useful? React with 👍 / 👎.
| //! End-to-end tool calling for a host that drives its own model loop. | ||
| //! | ||
| //! [`super`] answers *"what did the model ask for"*. This module answers | ||
| //! everything around it: how the model is **told** to ask, how results are | ||
| //! **rendered** back, and how a transcript is **replayed** onto the provider |
There was a problem hiding this comment.
The new dialect module spans seven implementation files and owns parsing, rendering, transcript repair, and provider replay, but it has no module-level README.md. Add one under src/dialect/ documenting its design, public surface, and operational constraints.
AGENTS.md reference: AGENTS.md:L202-L203
Useful? React with 👍 / 👎.
| @@ -0,0 +1,824 @@ | |||
| #![allow(clippy::expect_used, clippy::panic, clippy::unwrap_used)] | |||
There was a problem hiding this comment.
This new test.rs begins with a lint attribute and never supplies the required concise //! module description. Add the module-level documentation before the attribute, matching the repository's existing feature test modules.
AGENTS.md reference: AGENTS.md:L195-L196
Useful? React with 👍 / 👎.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.1343 · 2,538,198 in / 80,224 out · 237,138 cached (9%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash, deepseek-v4-flash · 1,137 embedded
critique: $0.0715 · 1,323,630 in / 45,301 out · 123,275 cached (9%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0518 · 994,422 in / 18,856 out · 36,295 cached (4%) · gpt-5.6-luna
tests: $0.0077 · 78,453 in / 4,858 out · 3,584 cached (5%) · deepseek/deepseek-v4-flash
description: $0.0016 · 69,699 in / 8,887 out · 2,048 cached (3%) · deepseek-v4-flash
| // The literal-tag pass cannot visit provider-specific structures such as | ||
| // Claude's attribute-form `<invoke name="…">`. Preserve canonical calls | ||
| // it did not reconstruct, so mixed responses never lose an invocation. | ||
| for canonical in json_calls { |
There was a problem hiding this comment.
Preserve duplicate calls and source order when merging parses
This treats calls as a set even though a response can legitimately request the same tool with identical arguments more than once. For example, with a non-empty registry, a P-Format call followed by an equivalent native <invoke> call produces one entry because the native call matches the already-combined P-Format call. It also appends every unmatched canonical call after all literal-tag calls, so an attribute-form <invoke> that appears before a later <tool_call> is executed after it. Merge the two parses by source position and preserve multiplicity rather than suppressing any call merely because an equal call already exists.
[RULE] preserve-call-multiplicity ·
| // non-string parameter fail schema validation — a typed `max_results` | ||
| // arriving as `""` means the tool never runs, and the error names a field | ||
| // the model deliberately left blank, which it cannot satisfy. | ||
| if raw.trim().is_empty() { |
There was a problem hiding this comment.
Preserve whitespace-only string arguments
This treats any value containing only whitespace as omitted, even when the schema declares a string parameter. For example, shell[0| ] produces no command key instead of preserving the three spaces, so valid string input is silently lost. Only an actually empty token should be omitted; trimming is appropriate for index parsing and numeric/boolean coercion, but not for deciding whether a string argument was sent.
| if raw.trim().is_empty() { | |
| if raw.is_empty() { |
[RULE] preserve-string-values ·
| // (e.g. extra whitespace, JSON, prose) means this isn't a valid | ||
| // p-format call and we leave it for the JSON fallback. | ||
| let open = trimmed.find('[')?; | ||
| if !trimmed.ends_with(']') { |
There was a problem hiding this comment.
Reject an escaped bracket mistaken for the closing delimiter
Checking only the final byte does not establish that the final ] is an unescaped delimiter. For example, shell[0|x\] has an escaped ] as its last character and no closing bracket, but this code strips that character, passes 0|x\ to split_pipes, and accepts the call with a value ending in \ instead of falling back as malformed input. Scan the body while tracking escapes and require the closing bracket to be unescaped; a trailing escaped ] must be rejected.
[RULE] malformed-escape ·
| .iter() | ||
| .flat_map(|entry| match entry { | ||
| TranscriptEntry::Chat(chat) => vec![chat.clone()], | ||
| TranscriptEntry::AssistantToolCalls { |
There was a problem hiding this comment.
Preserve assistant tool calls when text is absent
TranscriptEntry::AssistantToolCalls allows text to be None while still carrying non-empty tool_calls, but this arm ignores tool_calls and emits an empty assistant message. For a valid entry such as AssistantToolCalls { text: None, tool_calls: vec![...], ... }, replay loses the assistant's request and then emits the following tool results without any visible call for the model to answer. Reconstruct the text-dialect call representation from tool_calls, or reject/avoid persisting entries without the raw model text before replay.
[RULE] data-loss ·
| // constant, so this hands back fixed-size arrays and the two destructurings | ||
| // below need no bounds check. `chunks_exact_to_as_chunks` (clippy, Rust | ||
| // 1.98) flags the older form. | ||
| for &[open, close] in tags.as_chunks::<2>().0 { |
There was a problem hiding this comment.
Pair only matched tool-call tag boundaries
The normalization pass pairs every recognized tag strictly by global position. If the response contains an unmatched tool-call marker before a valid pair—for example, narrative text containing <|tool_call> followed by <tool_call>{"name":"echo","arguments":{}}</tool_call>—the unmatched marker is treated as the opening tag for the next tag, while the real closing tag becomes the trailing unpaired marker. The valid call is therefore rewritten into malformed markup and can be left as text instead of being dispatched. Match each opening marker with its compatible closing marker, or leave unmatched leading markers untouched rather than pairing them positionally.
[RULE] positional-tag-pairing ·
| return None; | ||
| } | ||
|
|
||
| let inner = &trimmed[open + 1..trimmed.len() - 1]; |
There was a problem hiding this comment.
Reject unescaped closing brackets inside values
Only the final ] is checked, so an additional unescaped closing bracket inside the body is accepted as ordinary argument data even though the format defines \] as the required escape. For example, shell[0|x]junk] is parsed with the command x]junk instead of being rejected and falling back to the other parser, and malformed output such as shell[0|echo ] can silently lose the intended bracket from the command. Validate unescaped ] while scanning the argument body and reject the call unless the bracket is escaped.
[RULE] malformed-input-validation ·
| results | ||
| .iter() | ||
| .map(|result| ToolResultEntry { | ||
| tool_call_id: result.tool_call_id.clone().unwrap_or_else(|| { |
There was a problem hiding this comment.
Preserve a replayable id for fallback tool calls
A native response can fall back to XML parsing at parse_response, and those ParsedToolCalls have id: None. If the resulting outcome is formatted here, it is assigned the literal unknown id. On replay, pair_tool_cycles requires the result id set to exactly match the assistant opener's call id set and explicitly drops mismatches, so a fallback call whose opener received any synthesized id will lose its entire tool cycle from the provider messages. Generate and retain the same fallback id when constructing the assistant call, or keep the fallback call out of native structured replay instead of emitting an unmatchable result id.
[RULE] preserve-call-correlation ·
| ); | ||
| continue; | ||
| }; | ||
| let opener_ids: std::collections::BTreeSet<&str> = |
There was a problem hiding this comment.
Reject duplicate tool-call or result identifiers
Using BTreeSet discards multiplicity, so a malformed cycle such as two assistant calls with id "call-1" followed by one result with that id passes the equality check and is emitted. The reverse also passes: one opener and two results with the same id. Providers generally require a one-to-one result for each tool call and may reject these histories, which defeats this function's purpose of producing safe-to-serialize transcripts. Compare identifier counts (or explicitly reject duplicates) before keeping the cycle.
[RULE] validate-one-to-one-pairing ·
| /// See [`Self::trusted_verbatim`]. Text dialects give such an outcome a | ||
| /// message of its own rather than folding it into the batch. | ||
| #[must_use] | ||
| pub fn verbatim(mut self) -> Self { |
There was a problem hiding this comment.
Restrict the verbatim output escape hatch
Any caller that can construct a ToolOutcome can mark arbitrary tool-controlled output as trusted. The text dialect then emits that output as a standalone message without neutralize_protocol_tags or the normal result envelope, so a compromised or untrusted tool can inject protocol markers and influence how subsequent transcript content is interpreted. Keep this flag host-controlled (for example, by removing this public builder and setting it only in a narrowly scoped conversion API), or always apply protocol escaping and framing to tool output.
[RULE] untrusted-output-bypass ·
1 participant
Summary
Adds
tinytools-agent, a provider-neutral crate for model-facing tool protocols. It owns tolerant tool-call parsing, P-Format, XML/P-Format/native dialects, catalogue and result rendering, and transcript replay while leaving execution policy in the host.Related issue
None.
API or behavior changes
tinytools-agentcrate.tinytools::ToolSpecwithout depending on TinyAgents or an inference/provider runtime.tracingfeature for recovery and replay diagnostics.This is additive and does not change the base
tinytoolsAPI.Validation
Commands actually run, with their outcome:
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-targets --all-featurescargo test --all-featuresRUSTDOCFLAGS=-Dwarnings cargo doc --all-features --no-depsTests
Migrated and retained 86 protocol tests covering dialect rendering and replay, tolerant JSON/XML/GLM parsing, malformed calls, result neutralization, P-Format coercion, sparse slots, and unknown-tool rejection. The full TinyTools workspace passes 145 unit tests plus doctests.
Documentation
Added
crates/tinytools-agent/README.md,docs/specs/agent-tool-protocols.md, and updated the workspace README.Checklist
#[allow(...)],#[ignore], or relaxed lints — test modules allow panic/unwrap under TinyTools test conventions, and the migrated top-level parser retains a scoped line-count allowance..envcontents in the diff or the descriptionSummary by CodeRabbit
New Features
tinytools-agentfor parsing model tool calls across JSON, XML, P-Format, native, and other supported response styles.Documentation