Add recoverable InvalidArgsPolicy for schema-validation failures#42
Conversation
A schema-validation failure on a registered tool call previously
propagated TinyAgentsError::Validation out of the agent loop and
aborted the whole turn, with no recoverable analogue to
UnknownToolPolicy. A missing required field, wrong type, or bad enum
killed the run instead of letting the model self-correct.
Add InvalidArgsPolicy { Fail, ReturnToolError } on RunPolicy, mirroring
UnknownToolPolicy. Under ReturnToolError, admission emits an
AgentEvent::InvalidToolArgs and returns a recoverable tool-error result
carrying the validation detail and the expected schema, consuming one
tool-call budget slot. Default stays Fail (backward-compatible).
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a new ChangesInvalidArgsPolicy Recovery Flow
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/harness/agent_loop/tools.rs (1)
203-231: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSolid mirror of the unknown-tool recovery path; minor:
tool.schema()computed twice.The branching logic correctly consumes the budget slot already reserved at line 140, emits
InvalidToolArgswith the validation detail and schema, and avoids invoking the tool — matching the PR's requirements and theUnknownToolPolicypattern above.One small nit:
tool.schema()is called once forvalidate_call(line 203) and again for.parameters(line 216), rebuilding the schema object twice on the error path. Not a functional issue, but avoidable.♻️ Cache the schema lookup
- if let Err(err) = tool.schema().validate_call(call) { + let schema = tool.schema(); + if let Err(err) = schema.validate_call(call) { // The model called a registered tool with schema-invalid arguments. // Apply the run's `InvalidArgsPolicy` instead of unconditionally // aborting the turn (mirrors the unknown-tool recovery above). if matches!(self.policy.invalid_args, InvalidArgsPolicy::Fail) { return Err(err); } ... - let schema_repr = serde_json::to_string(&tool.schema().parameters) + let schema_repr = serde_json::to_string(&schema.parameters) .unwrap_or_else(|_| "<unserializable>".to_string());🤖 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 `@src/harness/agent_loop/tools.rs` around lines 203 - 231, The error path in tools.rs recomputes the tool schema twice, once for validate_call and again when formatting the expected schema for the recovery message. Cache the result of tool.schema() in the InvalidArgs handling block and reuse it for both validation and schema serialization so the error path avoids rebuilding the schema object. Keep the existing InvalidToolArgs event emission and ErrorMessage flow in the same branch.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@src/harness/agent_loop/tools.rs`:
- Around line 203-231: The error path in tools.rs recomputes the tool schema
twice, once for validate_call and again when formatting the expected schema for
the recovery message. Cache the result of tool.schema() in the InvalidArgs
handling block and reuse it for both validation and schema serialization so the
error path avoids rebuilding the schema object. Keep the existing
InvalidToolArgs event emission and ErrorMessage flow in the same branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24b930c6-6c48-4889-a6f4-2a981af810db
📒 Files selected for processing (5)
src/harness/agent_loop/mod.rssrc/harness/agent_loop/test.rssrc/harness/agent_loop/tools.rssrc/harness/events/types.rssrc/harness/runtime/types.rs
collapsible_if in StreamAccumulator::push_tool_chunk and a duplicated #[cfg(test)] on the stream test module both fail CI's cargo clippy --all-targets -- -D warnings under rust 1.96. Behavior-neutral.
|
CI guardian: the |
Add
InvalidArgsPolicy { Fail, ReturnToolError }onRunPolicy, mirroringUnknownToolPolicy, so a schema-validation failure on a registered tool call no longer hard-aborts the turn. Previouslyvalidate_call(call)?propagatedTinyAgentsError::Validationout of the agent loop; a missingrequiredfield, wrong type, or badenumkilled the run with no way for the model to self-correct.Under
ReturnToolError, admission emitsAgentEvent::InvalidToolArgsand returns a recoverable tool-error result carrying the validation detail + expected schema, consuming one tool-call budget slot (mirrors the existing unknown-tool recovery). Default staysFail(backward-compatible).Validation:
cargo fmtclean;cargo test --libgreen (996 passed, incl. a new recoverable-path test and the unchangedFailbaseline). Note:cargo clippy --lib --tests -D warningscurrently fails on two pre-existing, unrelated lints under Rust 1.93.0 in files this PR does not touch (collapsible_ifinsrc/harness/model/mod.rs, duplicated attribute insrc/harness/stream/mod.rs) — reproducible on pristineb1f0d82; this diff introduces zero clippy warnings.Closes tinyhumansai/openhuman#4639
Summary by CodeRabbit