Skip to content

fix(model): keep the finish reason and accept empty tool arguments in response conversion - #136

Merged
M3gA-Mind merged 1 commit into
tinyhumansai:mainfrom
M3gA-Mind:fix/response-conversion-args-finish
Oct 23, 2026
Merged

M3gA-Mind merged 1 commit into
tinyhumansai:mainfrom
M3gA-Mind:fix/response-conversion-args-finish

Conversation

@M3gA-Mind

Copy link
Copy Markdown
Collaborator

Summary

What was wrong. response_to_model_response had two gaps.

  • Zero-argument tool calls. Native tool-call arguments were parsed with a raw serde_json::from_str. Several providers send a zero-argument call as arguments: "", which is not valid JSON, so the call came back marked invalid.
  • Finish reason. The finish reason was always inferred: tool_calls when there were calls, otherwise stop. ChatResponse had no way to carry the provider's reason, so a reply cut off by the output limit was reported as a clean stop, and a caller could not tell to continue or retry.

Fix.

  • Empty or whitespace-only arguments are treated as {} before parsing. Other malformed arguments are still kept as invalid calls, unchanged.
  • ChatResponse gains finish_reason: Option<String>. When it is set, the conversion keeps it (passed through canonical_finish_reason, so max_tokens/max_output_tokens become length). When it is absent, the old stop/tool_calls inference is used.

Related issue

None.

API or behavior changes

  • New public field ChatResponse::finish_reason (Default sets it to None). This is breaking for downstream code that builds ChatResponse with an exhaustive struct literal; such code must add finish_reason or use ..Default::default(). Noted in the changelog. No version bump in this PR.
  • A native tool call with empty or blank arguments now converts to a valid call with {}.
  • A reported finish reason is kept instead of being replaced with stop/tool_calls.

Validation

  • N/A: cargo fmt --all -- --check. I ran rustfmt (edition 2024) on the touched files only; CI runs the full check.
  • N/A: cargo clippy --all-targets --all-features -- -D warnings. Not run locally; CI runs it.
  • N/A: cargo build --all-targets --all-features. Not run locally; CI runs it.
  • N/A: cargo test --all-features. I ran the targeted tests below; CI runs the full suite.

Tests

New response_conversion_tests.rs with two tests.

  • empty_or_blank_arguments_mean_an_empty_object
    • Before the fix: FAILED, "" marked invalid
    • After the fix: PASSED.
  • reported_finish_reason_is_kept. It also checks that a response with no reported reason still infers tool_calls.
    • Before the fix (field added, conversion unchanged): FAILED, left: Some("stop"), right: Some("length")
    • After the fix: PASSED.

Also run after the fix: cargo test -p tinyinference-llm --lib response (61 passed) and --lib usage::meta (15 passed), which cover the existing conversion tests and the updated ChatResponse literal.

Documentation

Changelog entry under Unreleased. The new field has rustdoc.

Checklist

  • The change is focused on one logical change
  • No new #[allow(...)], #[ignore], or relaxed lints
  • No secrets, tokens, or .env contents in the diff or the description

@tinysweeper

tinysweeper Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Reviewed head: d1e131a364248aad87f3ea69fa1a16ab6889a4f6

Verdict: Ready for maintainer review · Priority: none
Coverage: Reviewed 5 of 5 files.
Pull request purpose (title): fix(model): keep the…

Diagnostics and supporting details

Full summary

Pull request purpose (title): fix(model): keep the finish reason and accept empty tool arguments in response conversion. Tiny Sweeper completed its review; deterministic results follow.

Review snapshot

Change surface Files Review signal Count
Production 2 Active findings 0
Tests 2 Noted findings 0
Documentation 1 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: Test coverage is assessed from changed tests and lane evidence; execution is not claimed without trusted check data.

What changed

`ChatResponse` gains a public `finish_reason: Option<String>` field with rustdoc, defaulting to `None`, plus a new `#[path = "response_conversion_tests.rs"]` test module in the conversion file. Conversion now normalizes empty/blank `arguments` to `{}` before parsing, and replaces the fixed `stop`/`tool_calls` assignment with a match that prefers the reported reason via `canonical_finish_reason` and falls back to the existing inference. A changelog entry under Unreleased documents the behavior and the breaking-field requirement. The existing `ChatResponse` literal in `response_tests.rs` is updated with `finish_reason: None`.

Features

  • Modified — Empty or blank native tool arguments parse as an empty object: A zero-argument tool call delivered as `arguments: ""` or whitespace-only no longer becomes an invalid call; it converts to a valid call with `{}`. Other malformed arguments still produce invalid calls with the original argument text and parse error preserved. (crates/tinyinference-llm/src/model/response_conversion.rs pub fn response_to_model_response()
  • Added — Provider finish reason carried on ChatResponse and preserved in conversion: `ChatResponse::finish_reason` lets the adapter record the provider's reason; when set it passes through `canonical_finish_reason` (e.g. `max_tokens` → `length`), so a length-truncated reply is no longer reported as a clean `stop` and callers can decide to continue or retry. When absent, the prior `stop`/`tool_calls` inference applies. Adding the field is breaking for exhaustive struct literals. (crates/tinyinference-llm/src/providers/response.rs pub struct ChatResponse<U = crate::usage::Usage, T = crate::tool::ToolCall> {, crates/tinyinference-llm/src/providers/response.rs impl<U, T> Default for ChatResponse<U, T> {, crates/tinyinference-llm/src/model/response_conversion.rs pub fn response_to_model_response(, CHANGELOG.md)

Tests

  • existing_test_update — The `ChatResponse` literal in the existing clone-preservation test gains `finish_reason: None` to satisfy the new field.: Keeps the existing test compiling against the new struct shape; the change is a literal initialization only. Not evidence of execution. (crates/tinyinference-llm/src/providers/response_tests.rs fn raw_call_identity_arguments_signature_and_exact_billing_survive_a_clone() {)

Findings

No active actionable findings.

Before merge

None.

Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: Reviewed 5 files; 0 findings. _The code index is behind this pull request (indexed at `50e6d1ce19bd`), so retrieved context may be out of date._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The security lane reviewed 4 code files with 0 findings and explicitly excluded `CHANGELOG.md` as prose data.
  • Lane summary: Reviewed 4 files; 0 findings. 1 file was not security-reviewed: CHANGELOG.md (prose or tabular data). _The code index is behind this pull request (indexed at `50e6d1ce19bd`), so retrieved context may be out of date._

tests

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The two behavioural changes each have a dedicated test that would fail on regression: `empty_or_blank_arguments_mean_an_empty_object` pins the blank/empty `arguments` handling in the new branch, and `reported_finish_reason_is_kept` pins both the pass-through through `canonical_finish_reason` (`max_tokens` → `length`) and the unreported fallback. One gap remains: the untested `None if tool_calls.is_empty()` arm of the new finish-reason match, though the tool-call path of the fallback is covered and the removed `stop` inference is a pre-existing, unchanged behaviour. No defects otherwise. _The code index is behind this pull request (indexed at `50e6d1ce19bd`), so retrieved context may be out of date._

commits

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The commits lane reported no sensitive content in committed files.
  • Lane summary: No sensitive content in committed files. _The code index is behind this pull request (indexed at `50e6d1ce19bd`), so retrieved context may be out of date._

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Lane summary: The change does what the description says: `response_to_model_response` now maps empty or whitespace-only native tool arguments to `{}` before parsing, and keeps the provider's finish reason (canonicalized) via the new `ChatResponse::finish_reason` field, falling back to the old `stop`/`tool_calls` inference. Remaining `ChatResponse` literals in the crate use `..Default::default()` or are updated, the new test file follows the repository's `*_tests.rs` convention, and the changelog entry covers the breaking-field note. The description is accurate and looks sound to merge. _The code index is behind this pull request (indexed at `50e6d1ce19bd`), so retrieved context may be out of date._
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash
  • Spend: $0.000659
  • Tokens: 236498 input · 4813 output · 48128 cached · 259 embedding
  • Updated: 2026-10-22T23:01:17Z
Head State Pass summary
d1e131a36424 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-10-22T23:01:17Z)

tinysweeper 0.1.2

@coderabbitai

coderabbitai Bot commented Oct 22, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6fe24a31-62b9-4f67-b41d-c73b36a33a6e
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

tinysweeper found nothing blocking. Approving.

Diagnostics

Total spend: $0.0007

@M3gA-Mind
M3gA-Mind merged commit 55c5d0c into tinyhumansai:main Oct 23, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant