Repository navigation
fix(harness): recover common tool-call argument mistakes and stop repeating cut-off calls - #365
Merged
Merged
Conversation
Vendors the tinytools package so the build no longer depends on fetching it at compile time. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
The loop now dispatches tool calls returned by the model and feeds their results back into the conversation, so multi-step tool use works end to end. Tool handling lives in a dedicated module to keep the loop itself focused on orchestration. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Moved the tool call wrapping logic out of the agent loop into a dedicated module so it can be reused and tested independently. No behaviour change. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
A call whose name is a wrapper alias such as tool_call or function_call is now unwrapped before the discovery bridge, rules, hooks and gates run, so those stages see the real tool name and arguments. The rewrite is reported as an argument repair rather than an unknown tool call, since the call does run. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a bridge module that connects the discovery tool to the harness, and add tests covering its behaviour. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
The todo tests were extracted from the parent module into their own mod_tests file to keep the implementation and its test suite separate. No test behaviour was changed. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add tests for the todo tool covering empty input, duplicate entries, and status transitions so regressions in validation and state handling are caught early. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add tests for the agent loop's handling of truncated tool calls, verifying that partial or malformed tool call payloads are detected and surfaced correctly rather than being silently dropped. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Move the tool call execution and result collection out of the main loop body into a dedicated helper so the loop reads as a sequence of clear steps. Behaviour is unchanged. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
…t limit Track which tools were cut off by the output token limit on the previous truncated turn so a repeat of the same oversized call is answered with a stronger corrective instead of another token-cap boost, and end the run when the same tool is cut off again after that warning. The output cap is fixed, so re-sending the same large call only burns the retry budget. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
The spent() test fixture now initializes the truncated_tool_names and truncated_repeat_names sets, which the TurnRecovery struct gained. Without them the fixture no longer compiles against the current struct shape. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Moved the response recovery logic out of the agent loop into a dedicated module so the loop body stays focused on orchestration. No behaviour change. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
The invalid-args tool error now returns a compact TypeScript-style signature of the expected arguments rather than the full JSON Schema, so the docs and the `InvalidArgsPolicy` comments were updated to describe what actually goes back into the transcript. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
…y recovery Expands the tool-call recovery documentation to cover wrapped pseudo-tool calls that are retargeted to the real tool before admission, and per-property object recovery plus null-dropping for optional properties. Also notes on RunPolicy that a tool cut off on two consecutive truncated turns gets a stronger corrective and one more cut-off call stops the run. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adjust the OTLP trace exporter so spans are serialized and exported correctly, and add tests covering the export path to guard against regressions. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted two attribute push calls in the OTLP trace exporter and its tests to satisfy rustfmt line width limits. No behaviour change. Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
…e_export/otlp.rs,crates/tinyage Auto-committed-on: macbook Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Tiny Sweeper reviewTiny Sweeper reviewed this change across 6 lane(s) and found 12 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
Previously reported and still active
Resolved this pass
Before merge
How this fits togetherflowchart LR
n0["parse_items<br/>changed"]:::changed
n1["dispatch"]:::impacted
n1 -->|calls| 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 configuration
📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to This change broadens tool-call argument recovery, stops repeated cut-off calls, and exports warning-level spans. No concrete merge-blocking defect was established. The tinytools submodule update could not be inspected, so confirm it separately. Security Architecture Review
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 75.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 65 functions across 19 files. (2 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the primary changes: recovering common tool-call argument mistakes and stopping repeated cut-off calls. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
Full details: Docstring Coverage
-
-
Explanation
Docstring coverage is 75.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 65 functions across 19 files. (2 skipped: 2 unsupported.)
-
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the tool-call trail,
And finds a wrapper’s hidden tale.
The todo fields now share the load,
Warnings take their marked-out code.
I thump my paws: the tests prevail!
Comment @coderabbitai help to get the list of available commands.
![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 critical.
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.0597 · 806,828 in / 56,956 out · 91,104 cached (11%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0364 · 458,437 in / 39,448 out · 51,384 cached (11%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0227 · 281,491 in / 13,557 out · 38,120 cached (14%) · gpt-5.6-luna
tests: $0.0002 · 21,956 in / 1,032 out · 64 cached (0%) · glm-5.3-flash
description: $0.0002 · 21,846 in / 172 out · 1,408 cached (6%) · glm-5.3-flash
tinysweeper
Bot
added
the
priority: p0
13 tasks done
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
A payload shaped like `{name, skill, arguments}` was being rewritten into a
use_skill call whenever the skill key was present, even when `name` referred to
a real callable tool that legitimately takes `skill` as its own argument. The
wrapper now only rewrites when `name` is not a callable tool, and tests cover
both this case and a blank query falling through to a populated alias.
Auto-committed-on: macbook
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
Requesting changes: 3 lane(s) blocking, worst finding is critical.
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.0154 · 232,962 in / 21,950 out · 28,081 cached (12%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0071 · 82,406 in / 7,238 out · 15,099 cached (18%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0076 · 81,024 in / 7,125 out · 12,790 cached (16%) · gpt-5.6-luna
tests: $0.0002 · 22,897 in / 2,594 out · 64 cached (0%) · glm-5.3-flash
description: $0.0002 · 22,950 in / 2,305 out · 64 cached (0%) · glm-5.3-flash
| .and_then(Value::as_str) | ||
| .map(str::trim) | ||
| .is_some_and(|name| !is_wrapper_name(name) && is_callable(name)); | ||
| if !wrapped_name && !named_real_tool { |
There was a problem hiding this comment.
Require the complete skill payload before selecting use_skill
Any wrapper payload containing only {"skill":"email"} (or a payload with skill but no usable tool/args) is rewritten to use_skill. The documented special shape is {skill, tool, args}, so incomplete or malformed wrapper payloads should remain untouched and receive the normal corrective response rather than being admitted as a use_skill call. Require the complete skill payload, including the fields needed by the pack schema, before returning this rewrite.
Additional security observation
Require a complete skill payload before selecting use_skill
[RULE] incomplete-input-validation
Any object containing skill is retargeted to use_skill unless another callable name is present, including incomplete payloads such as { "skill": "email" }. This bypasses the intended {skill, tool, args} shape and turns malformed pseudo-calls into real use_skill invocations, producing misleading downstream validation or execution behavior. Require the fields needed by the use_skill contract before returning this rewrite; otherwise leave the wrapper untouched.
[RULE] incomplete-payload-validation ·
| async fn answer_tool_search_reads_query_aliases() { | ||
| let policy = ToolDiscoveryPolicy::default(); | ||
| for key in ["q", "text", "description", "name", "skill", "search"] { | ||
| let answer = answer_tool_search(&catalog(), &policy, &json!({ key: "read a pdf" })).await; |
There was a problem hiding this comment.
Construct the alias argument with the loop key
json!({ key: ... }) treats key as a literal object field name, not as the value of the loop variable. Every iteration therefore passes { "key": "read a pdf" }, causing answer_tool_search to reject the query and the first assertion to fail. Build a Value::Object with key.to_string() so the test actually covers each alias.
[RULE] test-correctness ·
| /// when it is a JSON string; otherwise every key but the name key, which is | ||
| /// the "flattened" spelling `{"tool": "x", "query": "..."}`. | ||
| fn wrapped_arguments(payload: &Map<String, Value>, name_key: &str) -> Value { | ||
| for key in ARGUMENT_KEYS { |
There was a problem hiding this comment.
Test every argument alias and its precedence
The new wrapper accepts the shared ARGUMENT_KEYS aliases, but the added tests cover only arguments and args; they do not verify fallback through the other accepted aliases or behavior when multiple aliases are present. Since alias selection determines the arguments passed to the real tool, add focused tests for every alias and for the documented precedence order.
[RULE] missing-alias-coverage ·
| "[agent_loop] same tool cut off by the output limit twice; answering with the stop-repeating corrective" | ||
| ); | ||
| } | ||
| turn_recovery |
There was a problem hiding this comment.
Track repeated truncations by invocation, not tool name
The repeat detection still keys on the tool name: truncated_tool_names and truncated_repeat_names are BTreeSet<String> of names, and the matching in tools.rs answers a cut-off call with the stop-retrying corrective — or ends the run — whenever any call to that same tool name is cut off again. A legitimate, differently-shaped invocation of the same tool (e.g. a small save_workflow call after a huge one was cut) inherits the previous call's strike and can terminate the run on its first truncation. Track the strike per call shape or per invocation (for example a hash of the tool name plus argument length, or a counter of consecutive truncated turns that re-sent the same call), so a new, smaller call is judged on its own.
[RULE] wrong-key-tracking ·
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes for the tool-call failures in OpenHuman production Langfuse traces (Oct 3–10), each with a regression test. Moves the
vendor/tinytoolsgitlink to tinyhumansai/tinytools#59.tool_callfailed 100/100 times. DeepSeek wraps a real call in an envelope ({"name":"use_skill","arguments":…},{"use_skill":{…}},{skill,tool,args}).agent_loop/tool_call_wrapper.rs. When the name is a pseudo name (tool_call,call_tool,function_call, …) and no real tool has it, the call is unwrapped and sent to the real tool. String arguments go throughrecover_object. This runs before admission, so approvals and policy see the real tool. The event emitted isInvalidToolArgs { recovery: "unwrapped:<tool>" }. A payload that names no tool stays an unknown-tool error.todoitems senttitle/item/text/taskinstead ofcontent.content(contentwins). The item schema no longer requires them. A blank item is still refused.tool_searchwas called withdescription/name/q/skill/{}instead ofquery.query. Empty arguments get a short error with an example call.expected arguments: <type_signature>hint. Theinvalid arguments for toolprefix is unchanged, because hosts classify on it.LimitExceeded.ERROR(27% of shell calls showed as errors, about 70% of them ordinary non-zero exits).observation.levelspan attribute:WARNINGexports as WARNING for any span status. Hosts use it for command exits, cancelled turns and force-closed spans.Tests
cargo test --workspace: no failures (harness 2423, graph 476, plus the other crates)-D warningsandcargo fmt --checkare cleantool_call_wrapper_tests.rs(12),tools_args_recovery_tests.rs(3 wrapper cases),content_aliases_are_accepted,schema_admits_content_aliases_and_a_missing_status,answer_tool_search_reads_query_aliases,answer_tool_search_with_no_query_shows_an_example_call,use_skill_args_sent_as_relaxed_json_string_runs_the_tool,use_skill_with_null_tool_and_args_runs_the_listing_half,validation_corrective_is_a_short_hint_not_the_whole_schema,truncated_tool_call_tests.rs(4),a_span_can_set_its_level_to_warningDocs:
docs/modules/harness/tool.mdand theRunPolicydoc comments are updated.Note: this branch is based on the commit OpenHuman currently pins (
b60a650), andmainis 8 commits ahead.Summary by CodeRabbit
New Features
Improvements