mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
fix(agent): drop bisected tool_call/tool_result pairs before sending to provider (#2840)
## Summary
* Filter unpaired `AssistantToolCalls` and orphan `ToolResults` out of the wire payload in `NativeToolDispatcher::to_provider_messages` so a bisected tool cycle can no longer reach the provider.
* Strip trailing `assistant` messages that carry `tool_calls` without paired tool responses from a resumed transcript in `bound_cached_transcript_messages` (symmetric to the existing leading-orphan strip).
* Add `assistant_message_has_tool_calls` helper that peeks into the JSON-encoded `assistant` ChatMessage content to detect the tool_calls field at the `ChatMessage` boundary.
* Add 5 regression tests for `NativeToolDispatcher::to_provider_messages`: paired cycle, trailing unpaired, mid-history unpaired, orphan ToolResults, and multiple paired cycles back-to-back.
## Problem
Sentry issue **TAURI-RUST-7** — `OpenHuman API error (400 Bad Request): {"success":false,"error":"400 An assistant message with 'tool_calls' must be followed by tool messages responding to each 'tool_call_id'. (insufficient tool messages following tool_calls message)"}` — 589 events / 14d on the `tauri-rust` project.
The OpenAI chat-completions contract requires every `assistant{tool_calls}` message to be immediately followed by `tool` messages — one for every `tool_call_id`. Any other ordering produces `400 An assistant message with 'tool_calls' must be followed by tool messages`.
Two reachable code paths in our harness produce a bisected pair:
1. **Mid-turn abort / resume**. `ConversationMessage::AssistantToolCalls` is pushed into `Agent::history` before the tool runner finishes. If the turn aborts (user cancel, error, process kill, max-iter cap) before the matching `ConversationMessage::ToolResults` is appended, history ends on a tool_calls with no follow-up. The next turn submits the whole history and the backend rejects it.
2. **Cached transcript bound**. `bound_cached_transcript_messages` (`turn.rs:1660`) keeps the tail of a resumed transcript. The existing leading-orphan strip handles a window that opens on a stray `tool` message, but it never stripped a *trailing* assistant tool_calls. A resume from a mid-cycle snapshot ships an unpaired tool_calls and the backend rejects.
`trim_history` already had a leading-orphan guard ([`[turn.rs:1637](https://chatgpt.com/c/src/openhuman/agent/harness/session/turn.rs:1637)`](src/openhuman/agent/harness/session/turn.rs:1637)). The symmetric trailing / unpaired case was uncovered.
## Solution
### `src/openhuman/agent/dispatcher.rs` — Native dispatcher only
`to_provider_messages` is the boundary that serialises `ConversationMessage` → `ChatMessage` (wire format). The Native dispatcher is the only impl affected — XML and PFormat dispatchers render tool results into a `user` role with inline `<tool_result>` tags, so the tool_calls/tool ordering rule doesn't apply to them and their impls are untouched.
Two-pass pairing in a single loop:
```rust
let mut paired_indices: Vec<usize> = Vec::with_capacity(history.len());
for (i, msg) in history.iter().enumerate() {
match msg {
AssistantToolCalls { .. } => {
// Keep only if next is ToolResults.
if matches!(history.get(i + 1), Some(ToolResults(_))) {
paired_indices.push(i);
} else { log::debug!("dropping unpaired AssistantToolCalls at index {i}"); }
}
ToolResults(_) => {
// Keep only if the previous *emitted* index is a kept AssistantToolCalls.
let preceded_by_kept = i > 0
&& matches!(history.get(i - 1), Some(AssistantToolCalls { .. }))
&& paired_indices.last() == Some(&(i - 1));
if preceded_by_kept { paired_indices.push(i); }
else { log::debug!("dropping orphan ToolResults at index {i}"); }
}
Chat(_) => paired_indices.push(i),
}
}
```
The key invariant is `paired_indices.last() == Some(&(i - 1))` — that captures emitted-not-raw lookbehind. A bisected assistant tool_calls is dropped, which means the now-orphan tool results that physically followed it are also dropped, even though `history[i-1]` is still `AssistantToolCalls`. Symmetric drop in a single pass.
Second pass: `flat_map` over `paired_indices` runs the existing serialisation logic verbatim (assistant tool_calls → JSON-encoded ChatMessage; tool results → one tool ChatMessage per result). No serialisation change — only the input set is filtered.
### `src/openhuman/agent/harness/session/turn.rs` — cached transcript bound
This layer operates on `Vec<ChatMessage>`, not `ConversationMessage`. Can't pattern-match on enum variants because the dispatcher's serialisation packs both content and tool_calls into a single JSON-encoded string in the assistant `ChatMessage.content` (see `dispatcher.rs:484-490`). To detect tool_calls at this boundary the helper peeks inside the JSON:
```rust
fn assistant_message_has_tool_calls(msg: &ChatMessage) -> bool {
if msg.role != "assistant" { return false; }
let Ok(value) = serde_json::from_str::<serde_json::Value>(&msg.content) else { return false; };
value.get("tool_calls").and_then(|tc| tc.as_array())
.map(|arr| !arr.is_empty()).unwrap_or(false)
}
```
Non-assistant role → false. Non-JSON content (a plain text reply) → false. Missing field or empty array → false. Message kept in all those cases.
Then a `pop_while` after the existing leading-orphan strip:
```rust
while bounded.last().map(assistant_message_has_tool_calls).unwrap_or(false) {
bounded.pop();
dropped_tail += 1;
}
```
Symmetric to the existing leading-orphan strip — both ends of the bounded window now end on a clean turn boundary.
## Design choices
- Filter at the wire boundary, not at write time. Both write sites (turn loop, transcript restore) push into history without coordinating with each other. Centralising the guard at the serialisation boundary catches every code path that ends up shipping to a provider, present and future.
- Native dispatcher only. XML / PFormat dispatchers render tool results into user role with inline tags and aren't subject to the OpenAI tool ordering rule. Leaving their `to_provider_messages` untouched avoids unrelated behaviour change.
- What we drop the backend would have rejected anyway. Every dropped message would have triggered the same 400. Dropping client-side turns a hard failure into a recoverable turn — the rest of the well-formed history still flies.
- `log::debug!` on drop. Visibility into how often the guard fires without flooding warn-level logs.
-JSON-content inspection helper isolated and tested by the integration tests above. A `serde_json::from_str` parse failure means the content isn't the dispatcher's JSON envelope, so the message is a plain text assistant reply and must be kept — false return is correct.
## Submission Checklist
- Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
- Diff coverage ≥ 80% — pending local pnpm test:rust run
- Coverage matrix updated — N/A: bug-fix behaviour-only change, no new feature row
- All affected feature IDs from the matrix are listed in the PR description under ## Related
- No new external network dependencies introduced
- Manual smoke checklist updated — N/A: no release-cut surface touched
- Linked issue closed via Closes #NNN — N/A: Sentry-tracked issue, no GitHub issue yet
## Impact
- Runtime: desktop (Rust core). No mobile / web / CLI surface change.
- Performance: one extra `Vec<usize>` allocation sized to `history.len()` in the dispatcher and one O(n) pass over the bounded transcript tail. Happy-path cost is two extra `matches!` checks per message. No measurable overhead on the chat hot path.
- Security: none — no new network surface, no new inputs trusted, no auth path touched.
- Migration / compatibility: none. Trait signatures, RPC schemas, and dispatcher output for fully-paired histories are unchanged. Previously-failing turns that hit the backend's 400 now succeed with a `log::debug!` line per dropped orphan.
## Related
- Closes: [TAURI-RUST-7](https://sentry.tinyhumans.ai/organizations/tinyhumans/issues/47/?project=4&referrer=issue-list&statsPeriod=14d)
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **Bug Fixes**
* Sanitized provider-bound message history to keep only well-formed assistant opener + matching tool-results (exact ID-set match, order-insensitive); orphaned or mismatched tool messages are dropped.
* Hardened transcript trimming to strip partial/native tool-call envelopes at window edges so turns end on clean boundaries and log the count of stripped envelopes.
* **Tests**
* Expanded regression suite covering pairing semantics, orphan/drop cases, ordering tolerance, and transcript-bounding behavior.
<!-- review_stack_entry_start -->
[](https://app.coderabbit.ai/change-stack/tinyhumansai/openhuman/pull/2840?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack)
<!-- review_stack_entry_end -->
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: M3gA-Mind <megamind@mahadao.com>
This commit is contained in:
YellowSnnowmann
co-authored by
M3gA-Mind
M3gA-Mind
parent
7fbcbe87c1
commit
ad6d600469
@@ -477,9 +477,78 @@ impl ToolDispatcher for NativeToolDispatcher {
|
||||
}
|
||||
|
||||
fn to_provider_messages(&self, history: &[ConversationMessage]) -> Vec<ChatMessage> {
|
||||
history
|
||||
.iter()
|
||||
.flat_map(|msg| match msg {
|
||||
// TAURI-RUST-7: providers (incl. the OpenHuman backend) reject any
|
||||
// assistant `tool_calls` message that isn't immediately followed by
|
||||
// `tool` messages responding to every `tool_call_id`, with
|
||||
// `400 An assistant message with 'tool_calls' must be followed by
|
||||
// tool messages responding to each 'tool_call_id'`. Cached transcript
|
||||
// restores, mid-turn aborts, and history compaction can all produce a
|
||||
// bisected tool cycle (assistant tool_calls preserved, ToolResults
|
||||
// dropped or never persisted). Filter those out here, just before
|
||||
// serialising to provider wire format, so a bisected pair never
|
||||
// reaches the wire. Symmetric drop: a `ToolResults` whose preceding
|
||||
// `AssistantToolCalls` was dropped is also stripped to keep the
|
||||
// sequence well-formed.
|
||||
// CodeRabbit follow-up: backend's 400 says "insufficient tool messages
|
||||
// following tool_calls", which fires on either (a) zero following tool
|
||||
// messages, or (b) a tool message set whose `tool_call_id`s don't
|
||||
// cover every `tool_calls[].id` on the opener. Adjacency alone is
|
||||
// not sufficient — require the full set of opener ids to equal the
|
||||
// set of follower `tool_call_id`s.
|
||||
let mut paired_indices: Vec<usize> = Vec::with_capacity(history.len());
|
||||
for (i, msg) in history.iter().enumerate() {
|
||||
match msg {
|
||||
ConversationMessage::AssistantToolCalls { tool_calls, .. } => {
|
||||
let Some(ConversationMessage::ToolResults(results)) = history.get(i + 1) else {
|
||||
log::debug!(
|
||||
"[agent][dispatcher] dropping unpaired AssistantToolCalls at index \
|
||||
{i} of {} (no immediately following ToolResults — would trip \
|
||||
provider 400 'tool_calls must be followed by tool messages')",
|
||||
history.len()
|
||||
);
|
||||
continue;
|
||||
};
|
||||
let opener_ids: std::collections::BTreeSet<&str> =
|
||||
tool_calls.iter().map(|tc| tc.id.as_str()).collect();
|
||||
let result_ids: std::collections::BTreeSet<&str> =
|
||||
results.iter().map(|r| r.tool_call_id.as_str()).collect();
|
||||
if !opener_ids.is_empty() && opener_ids == result_ids {
|
||||
paired_indices.push(i);
|
||||
} else {
|
||||
log::debug!(
|
||||
"[agent][dispatcher] dropping AssistantToolCalls at index {i}: \
|
||||
tool_call_id set mismatch between opener ({:?}) and ToolResults \
|
||||
({:?})",
|
||||
opener_ids,
|
||||
result_ids
|
||||
);
|
||||
}
|
||||
}
|
||||
ConversationMessage::ToolResults(_) => {
|
||||
let preceded_by_kept_tool_calls = i > 0
|
||||
&& matches!(
|
||||
history.get(i - 1),
|
||||
Some(ConversationMessage::AssistantToolCalls { .. })
|
||||
)
|
||||
&& paired_indices.last() == Some(&(i - 1));
|
||||
if preceded_by_kept_tool_calls {
|
||||
paired_indices.push(i);
|
||||
} else {
|
||||
log::debug!(
|
||||
"[agent][dispatcher] dropping orphan ToolResults at index {i} \
|
||||
(no preceding AssistantToolCalls in the emitted sequence)"
|
||||
);
|
||||
}
|
||||
}
|
||||
ConversationMessage::Chat(_) => {
|
||||
paired_indices.push(i);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
paired_indices
|
||||
.into_iter()
|
||||
.flat_map(|i| match &history[i] {
|
||||
ConversationMessage::Chat(chat) => vec![chat.clone()],
|
||||
ConversationMessage::AssistantToolCalls { text, tool_calls } => {
|
||||
let payload = serde_json::json!({
|
||||
|
||||
@@ -252,3 +252,244 @@ fn native_format_results_keeps_tool_call_id() {
|
||||
_ => panic!("expected ToolResults variant"),
|
||||
}
|
||||
}
|
||||
|
||||
// ── TAURI-RUST-7 regression: tool_calls / ToolResults pairing ──────────
|
||||
//
|
||||
// Providers reject any assistant `tool_calls` message that isn't immediately
|
||||
// followed by `tool` messages responding to every `tool_call_id`. Cached
|
||||
// transcript restores and mid-turn aborts can produce bisected pairs. The
|
||||
// fix in `to_provider_messages` drops unpaired AssistantToolCalls and orphan
|
||||
// ToolResults so the wire payload is always well-formed.
|
||||
|
||||
fn assistant_tool_calls(id: &str) -> ConversationMessage {
|
||||
ConversationMessage::AssistantToolCalls {
|
||||
text: Some("calling tool".into()),
|
||||
tool_calls: vec![crate::openhuman::inference::provider::ToolCall {
|
||||
id: id.into(),
|
||||
name: "shell".into(),
|
||||
arguments: "{}".into(),
|
||||
}],
|
||||
}
|
||||
}
|
||||
|
||||
fn tool_results(id: &str) -> ConversationMessage {
|
||||
use crate::openhuman::inference::provider::ToolResultMessage;
|
||||
ConversationMessage::ToolResults(vec![ToolResultMessage {
|
||||
tool_call_id: id.into(),
|
||||
content: "ok".into(),
|
||||
}])
|
||||
}
|
||||
|
||||
fn user_chat(text: &str) -> ConversationMessage {
|
||||
ConversationMessage::Chat(crate::openhuman::inference::provider::ChatMessage::user(
|
||||
text,
|
||||
))
|
||||
}
|
||||
|
||||
fn assistant_chat(text: &str) -> ConversationMessage {
|
||||
ConversationMessage::Chat(crate::openhuman::inference::provider::ChatMessage::assistant(text))
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_keeps_paired_tool_cycle() {
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("hi"),
|
||||
assistant_tool_calls("tc-1"),
|
||||
tool_results("tc-1"),
|
||||
assistant_chat("done"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(roles, vec!["user", "assistant", "tool", "assistant"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_drops_trailing_unpaired_tool_calls() {
|
||||
// The assistant emitted tool_calls but the run was aborted before the
|
||||
// ToolResults were persisted. The trailing tool_calls must not reach
|
||||
// the wire.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("hi"),
|
||||
assistant_chat("ok"),
|
||||
user_chat("again"),
|
||||
assistant_tool_calls("tc-2"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(
|
||||
roles,
|
||||
vec!["user", "assistant", "user"],
|
||||
"trailing unpaired AssistantToolCalls must be stripped"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_drops_mid_history_unpaired_tool_calls() {
|
||||
// History with a bisected pair in the middle: tool_calls followed
|
||||
// directly by a Chat (not ToolResults). Drop the tool_calls; keep
|
||||
// everything else.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("hi"),
|
||||
assistant_tool_calls("tc-3"), // bisected — no following ToolResults
|
||||
user_chat("nevermind"),
|
||||
assistant_chat("ok"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(roles, vec!["user", "user", "assistant"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_drops_orphan_tool_results() {
|
||||
// Symmetric drop: ToolResults whose preceding AssistantToolCalls was
|
||||
// never emitted (either never persisted or already dropped above)
|
||||
// must not appear in the wire payload.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("hi"),
|
||||
tool_results("tc-4"), // orphan — no preceding AssistantToolCalls
|
||||
assistant_chat("ok"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(roles, vec!["user", "assistant"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_handles_multiple_tool_cycles() {
|
||||
// Two paired cycles in a row — both must survive.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("a"),
|
||||
assistant_tool_calls("tc-5"),
|
||||
tool_results("tc-5"),
|
||||
assistant_tool_calls("tc-6"),
|
||||
tool_results("tc-6"),
|
||||
assistant_chat("final"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(
|
||||
roles,
|
||||
vec![
|
||||
"user",
|
||||
"assistant",
|
||||
"tool",
|
||||
"assistant",
|
||||
"tool",
|
||||
"assistant"
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
// ── tool_call_id set-pairing (CodeRabbit follow-up) ─────────────────────
|
||||
|
||||
fn assistant_tool_calls_multi(ids: &[&str]) -> ConversationMessage {
|
||||
ConversationMessage::AssistantToolCalls {
|
||||
text: Some("calling tools".into()),
|
||||
tool_calls: ids
|
||||
.iter()
|
||||
.map(|id| crate::openhuman::inference::provider::ToolCall {
|
||||
id: (*id).into(),
|
||||
name: "shell".into(),
|
||||
arguments: "{}".into(),
|
||||
})
|
||||
.collect(),
|
||||
}
|
||||
}
|
||||
|
||||
fn tool_results_multi(ids: &[&str]) -> ConversationMessage {
|
||||
use crate::openhuman::inference::provider::ToolResultMessage;
|
||||
ConversationMessage::ToolResults(
|
||||
ids.iter()
|
||||
.map(|id| ToolResultMessage {
|
||||
tool_call_id: (*id).into(),
|
||||
content: "ok".into(),
|
||||
})
|
||||
.collect(),
|
||||
)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_drops_pair_when_tool_call_ids_mismatch() {
|
||||
// Opener requests `tc-1`, but the only ToolResults entry answers `tc-x`.
|
||||
// Backend would 400 with "insufficient tool messages following tool_calls"
|
||||
// — drop both.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("hi"),
|
||||
assistant_tool_calls_multi(&["tc-1"]),
|
||||
tool_results_multi(&["tc-x"]),
|
||||
assistant_chat("done"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(
|
||||
roles,
|
||||
vec!["user", "assistant"],
|
||||
"id-set mismatch must drop the bisected pair entirely, kept: {roles:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_drops_pair_when_results_are_partial() {
|
||||
// Opener requests two tool_call_ids, results answer only one. Backend
|
||||
// rejects with "insufficient tool messages". Strict set equality drops
|
||||
// the pair.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("hi"),
|
||||
assistant_tool_calls_multi(&["tc-1", "tc-2"]),
|
||||
tool_results_multi(&["tc-1"]),
|
||||
assistant_chat("done"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(
|
||||
roles,
|
||||
vec!["user", "assistant"],
|
||||
"partial tool-result coverage must drop the pair, kept: {roles:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_keeps_pair_with_full_id_coverage() {
|
||||
// Strict set equality: opener has {tc-1, tc-2}, results cover both,
|
||||
// even if listed in a different order. Both messages must be emitted.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("hi"),
|
||||
assistant_tool_calls_multi(&["tc-1", "tc-2"]),
|
||||
tool_results_multi(&["tc-2", "tc-1"]),
|
||||
assistant_chat("done"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(
|
||||
roles,
|
||||
vec!["user", "assistant", "tool", "tool", "assistant"]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn to_provider_messages_drops_pair_with_extra_unsolicited_results() {
|
||||
// Opener requests `tc-1`; results answer `tc-1` *and* an unsolicited
|
||||
// `tc-extra`. The id sets differ, so the pair is dropped.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![
|
||||
user_chat("hi"),
|
||||
assistant_tool_calls_multi(&["tc-1"]),
|
||||
tool_results_multi(&["tc-1", "tc-extra"]),
|
||||
assistant_chat("done"),
|
||||
];
|
||||
let out = dispatcher.to_provider_messages(&history);
|
||||
let roles: Vec<&str> = out.iter().map(|m| m.role.as_str()).collect();
|
||||
assert_eq!(
|
||||
roles,
|
||||
vec!["user", "assistant"],
|
||||
"extra unsolicited tool_call_ids must invalidate the pair, kept: {roles:?}"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -51,6 +51,51 @@ use anyhow::Result;
|
||||
use std::hash::{Hash, Hasher};
|
||||
use std::sync::Arc;
|
||||
|
||||
/// True when `msg` is an `assistant` ChatMessage whose JSON-encoded content
|
||||
/// carries a non-empty `tool_calls` array.
|
||||
///
|
||||
/// `to_provider_messages` (in `agent/dispatcher.rs`) serialises an
|
||||
/// `AssistantToolCalls` ConversationMessage as a single `assistant` ChatMessage
|
||||
/// with a JSON body of the form `{"content": "...", "tool_calls": [...]}`. To
|
||||
/// detect those at the `ChatMessage` boundary (where `bound_cached_transcript_messages`
|
||||
/// operates) we have to peek inside the JSON. See TAURI-RUST-7 for the
|
||||
/// failure mode this guards against.
|
||||
fn assistant_message_has_tool_calls(msg: &ChatMessage) -> bool {
|
||||
if msg.role != "assistant" {
|
||||
return false;
|
||||
}
|
||||
let Ok(value) = serde_json::from_str::<serde_json::Value>(&msg.content) else {
|
||||
return false;
|
||||
};
|
||||
// CodeRabbit follow-up: only treat this as the native tool_calls envelope
|
||||
// when the full expected shape is present:
|
||||
// - top-level JSON object
|
||||
// - `content` key present (the envelope `dispatcher.rs` emits — see
|
||||
// `to_provider_messages`)
|
||||
// - non-empty `tool_calls` array whose every element carries an `id`
|
||||
// string, a `name` string, and an `arguments` field
|
||||
// This stops a legitimate assistant text reply that happens to contain
|
||||
// the literal string `tool_calls` from being misclassified and dropped at
|
||||
// the bound-cached-transcript boundary.
|
||||
let Some(obj) = value.as_object() else {
|
||||
return false;
|
||||
};
|
||||
if !obj.contains_key("content") {
|
||||
return false;
|
||||
}
|
||||
let Some(tool_calls) = obj.get("tool_calls").and_then(|tc| tc.as_array()) else {
|
||||
return false;
|
||||
};
|
||||
if tool_calls.is_empty() {
|
||||
return false;
|
||||
}
|
||||
tool_calls.iter().all(|tc| {
|
||||
tc.get("id").and_then(|v| v.as_str()).is_some()
|
||||
&& tc.get("name").and_then(|v| v.as_str()).is_some()
|
||||
&& tc.get("arguments").is_some()
|
||||
})
|
||||
}
|
||||
|
||||
/// Instruction appended (as a synthetic user turn) to the provider
|
||||
/// messages when a turn hits the tool-call iteration cap. Asks the model
|
||||
/// to wrap up with a resumable checkpoint instead of letting the turn die.
|
||||
@@ -1725,6 +1770,30 @@ impl Agent {
|
||||
bounded.push(messages[0].clone());
|
||||
}
|
||||
bounded.extend(tail.iter().cloned());
|
||||
|
||||
// TAURI-RUST-7: symmetric guard to the leading-orphan strip above. A
|
||||
// resumed transcript that ends on an `assistant` message containing
|
||||
// `tool_calls` (because the cached transcript was captured mid-cycle,
|
||||
// before the tool responses were persisted) is rejected by the
|
||||
// provider with `400 An assistant message with 'tool_calls' must be
|
||||
// followed by tool messages`. Pop any such trailing assistant
|
||||
// tool_calls so the bounded transcript ends on a clean turn boundary.
|
||||
let mut dropped_tail = 0usize;
|
||||
while bounded
|
||||
.last()
|
||||
.map(assistant_message_has_tool_calls)
|
||||
.unwrap_or(false)
|
||||
{
|
||||
bounded.pop();
|
||||
dropped_tail += 1;
|
||||
}
|
||||
if dropped_tail > 0 {
|
||||
log::debug!(
|
||||
"[agent] bound_cached_transcript_messages stripped {dropped_tail} trailing \
|
||||
assistant tool_calls message(s) without paired tool responses"
|
||||
);
|
||||
}
|
||||
|
||||
bounded
|
||||
}
|
||||
|
||||
|
||||
@@ -1324,3 +1324,190 @@ async fn fetch_learned_context_loads_general_prefs_when_learning_enabled() {
|
||||
learned.user_profile
|
||||
);
|
||||
}
|
||||
|
||||
// ── assistant_message_has_tool_calls — TAURI-RUST-7 envelope check ─────
|
||||
|
||||
#[test]
|
||||
fn assistant_message_has_tool_calls_detects_native_envelope() {
|
||||
let body = serde_json::json!({
|
||||
"content": "calling tool",
|
||||
"tool_calls": [{
|
||||
"id": "tc-1",
|
||||
"name": "shell",
|
||||
"arguments": "{}"
|
||||
}]
|
||||
})
|
||||
.to_string();
|
||||
let msg = ChatMessage::assistant(body);
|
||||
assert!(super::assistant_message_has_tool_calls(&msg));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn assistant_message_has_tool_calls_rejects_non_assistant_role() {
|
||||
let body = serde_json::json!({
|
||||
"content": "x",
|
||||
"tool_calls": [{ "id": "tc-1", "name": "shell", "arguments": "{}" }]
|
||||
})
|
||||
.to_string();
|
||||
let msg = ChatMessage::user(body);
|
||||
assert!(!super::assistant_message_has_tool_calls(&msg));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn assistant_message_has_tool_calls_rejects_plain_text_reply() {
|
||||
// Most common positive case for the previous over-broad check: a plain
|
||||
// assistant reply whose text happens to mention `tool_calls`.
|
||||
let msg = ChatMessage::assistant("I considered using tool_calls but chose not to.");
|
||||
assert!(!super::assistant_message_has_tool_calls(&msg));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn assistant_message_has_tool_calls_rejects_envelope_without_content_field() {
|
||||
// A bare `{"tool_calls": [...]}` JSON in the content (no `content` field)
|
||||
// is not the envelope `dispatcher.rs` emits.
|
||||
let body = serde_json::json!({
|
||||
"tool_calls": [{ "id": "tc-1", "name": "shell", "arguments": "{}" }]
|
||||
})
|
||||
.to_string();
|
||||
let msg = ChatMessage::assistant(body);
|
||||
assert!(!super::assistant_message_has_tool_calls(&msg));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn assistant_message_has_tool_calls_rejects_empty_tool_calls_array() {
|
||||
let body = serde_json::json!({
|
||||
"content": "no tools",
|
||||
"tool_calls": []
|
||||
})
|
||||
.to_string();
|
||||
let msg = ChatMessage::assistant(body);
|
||||
assert!(!super::assistant_message_has_tool_calls(&msg));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn assistant_message_has_tool_calls_rejects_malformed_tool_call_items() {
|
||||
// tool_call object missing `id` — not the native envelope shape.
|
||||
let body_no_id = serde_json::json!({
|
||||
"content": "x",
|
||||
"tool_calls": [{ "name": "shell", "arguments": "{}" }]
|
||||
})
|
||||
.to_string();
|
||||
assert!(!super::assistant_message_has_tool_calls(
|
||||
&ChatMessage::assistant(body_no_id)
|
||||
));
|
||||
|
||||
// tool_call object missing `arguments` — also rejected.
|
||||
let body_no_args = serde_json::json!({
|
||||
"content": "x",
|
||||
"tool_calls": [{ "id": "tc-1", "name": "shell" }]
|
||||
})
|
||||
.to_string();
|
||||
assert!(!super::assistant_message_has_tool_calls(
|
||||
&ChatMessage::assistant(body_no_args)
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn assistant_message_has_tool_calls_rejects_non_object_root() {
|
||||
// Content is a JSON array, not an object.
|
||||
let msg = ChatMessage::assistant(r#"["just", "an", "array"]"#.to_string());
|
||||
assert!(!super::assistant_message_has_tool_calls(&msg));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn assistant_message_has_tool_calls_rejects_non_json_content() {
|
||||
// Plain prose that doesn't parse as JSON at all — early-returns false via
|
||||
// the `let Ok(value) = serde_json::from_str(...)` arm. Keeps the message
|
||||
// when the trailing-strip uses this helper.
|
||||
let msg = ChatMessage::assistant("Just a normal text reply, no JSON here.");
|
||||
assert!(!super::assistant_message_has_tool_calls(&msg));
|
||||
}
|
||||
|
||||
// ── bound_cached_transcript_messages — TAURI-RUST-7 trailing-strip ─────
|
||||
//
|
||||
// `bound_cached_transcript_messages` operates on a `Vec<ChatMessage>` (the
|
||||
// dispatcher-serialised wire format), so its detection runs through
|
||||
// `assistant_message_has_tool_calls`. Verify the symmetric trailing-strip
|
||||
// pops unpaired tool_calls envelopes while leaving plain assistant replies
|
||||
// untouched.
|
||||
|
||||
fn tool_calls_envelope(id: &str) -> String {
|
||||
serde_json::json!({
|
||||
"content": "calling tool",
|
||||
"tool_calls": [{
|
||||
"id": id,
|
||||
"name": "shell",
|
||||
"arguments": "{}"
|
||||
}]
|
||||
})
|
||||
.to_string()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bound_cached_transcript_messages_pops_trailing_tool_calls_envelope() {
|
||||
let agent = make_agent(None); // max_history_messages = 3
|
||||
// Need > max so the bound runs (early-returns when len <= max).
|
||||
let messages = vec![
|
||||
ChatMessage::system("sys"),
|
||||
ChatMessage::user("u1"),
|
||||
ChatMessage::assistant("a1"),
|
||||
ChatMessage::user("u2"),
|
||||
ChatMessage::assistant(tool_calls_envelope("tc-trailing")),
|
||||
];
|
||||
|
||||
// With `max_history_messages = 3` and the leading `system` message,
|
||||
// `bound_cached_transcript_messages` keeps the last 2 non-system entries
|
||||
// — i.e. `[system, u2, trailing-envelope]`. After the envelope pop the
|
||||
// tail is `user("u2")`, not the dropped assistant message.
|
||||
let bounded = agent.bound_cached_transcript_messages(messages);
|
||||
assert!(
|
||||
bounded
|
||||
.last()
|
||||
.is_some_and(|m| m.role == "user" && m.content == "u2"),
|
||||
"trailing tool_calls envelope must be popped; expected user tail 'u2' — got tail role={:?} content={:?}",
|
||||
bounded.last().map(|m| m.role.as_str()),
|
||||
bounded.last().map(|m| m.content.as_str())
|
||||
);
|
||||
assert!(
|
||||
!bounded.iter().any(super::assistant_message_has_tool_calls),
|
||||
"no tool_calls envelope should survive the strip"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bound_cached_transcript_messages_leaves_plain_assistant_tail_intact() {
|
||||
let agent = make_agent(None); // max_history_messages = 3
|
||||
let messages = vec![
|
||||
ChatMessage::system("sys"),
|
||||
ChatMessage::user("u1"),
|
||||
ChatMessage::assistant("a1"),
|
||||
ChatMessage::user("u2"),
|
||||
ChatMessage::assistant("plain text reply, no tool_calls"),
|
||||
];
|
||||
|
||||
let bounded = agent.bound_cached_transcript_messages(messages);
|
||||
let tail = bounded.last().expect("bounded transcript is non-empty");
|
||||
assert_eq!(tail.role, "assistant");
|
||||
assert_eq!(tail.content, "plain text reply, no tool_calls");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bound_cached_transcript_messages_strips_multiple_trailing_envelopes() {
|
||||
// Defence-in-depth: if the cached transcript ends on multiple consecutive
|
||||
// unpaired tool_calls envelopes (e.g. two abortive turns), pop them all.
|
||||
let agent = make_agent(None);
|
||||
let messages = vec![
|
||||
ChatMessage::system("sys"),
|
||||
ChatMessage::user("u1"),
|
||||
ChatMessage::assistant("a1"),
|
||||
ChatMessage::assistant(tool_calls_envelope("tc-1")),
|
||||
ChatMessage::assistant(tool_calls_envelope("tc-2")),
|
||||
];
|
||||
|
||||
let bounded = agent.bound_cached_transcript_messages(messages);
|
||||
let any_envelope = bounded.iter().any(super::assistant_message_has_tool_calls);
|
||||
assert!(
|
||||
!any_envelope,
|
||||
"all trailing tool_calls envelopes must be stripped"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1317,22 +1317,67 @@ fn xml_dispatcher_converts_history_to_provider_messages() {
|
||||
|
||||
#[test]
|
||||
fn native_dispatcher_converts_tool_results_to_tool_messages() {
|
||||
// TAURI-RUST-7: `to_provider_messages` now drops orphan `ToolResults`
|
||||
// that aren't preceded by an emitted `AssistantToolCalls`, since the
|
||||
// OpenAI chat-completions contract rejects a `tool` message that does
|
||||
// not follow a `tool_calls` opener. Feed a properly paired cycle so
|
||||
// this test continues to verify the serialisation shape of ToolResults
|
||||
// (one `tool` ChatMessage per result) rather than the orphan path.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![ConversationMessage::ToolResults(vec![
|
||||
ToolResultMessage {
|
||||
tool_call_id: "tc1".into(),
|
||||
content: "output1".into(),
|
||||
let history = vec![
|
||||
ConversationMessage::AssistantToolCalls {
|
||||
text: None,
|
||||
tool_calls: vec![
|
||||
ToolCall {
|
||||
id: "tc1".into(),
|
||||
name: "shell".into(),
|
||||
arguments: "{}".into(),
|
||||
},
|
||||
ToolCall {
|
||||
id: "tc2".into(),
|
||||
name: "shell".into(),
|
||||
arguments: "{}".into(),
|
||||
},
|
||||
],
|
||||
},
|
||||
ToolResultMessage {
|
||||
tool_call_id: "tc2".into(),
|
||||
content: "output2".into(),
|
||||
},
|
||||
])];
|
||||
ConversationMessage::ToolResults(vec![
|
||||
ToolResultMessage {
|
||||
tool_call_id: "tc1".into(),
|
||||
content: "output1".into(),
|
||||
},
|
||||
ToolResultMessage {
|
||||
tool_call_id: "tc2".into(),
|
||||
content: "output2".into(),
|
||||
},
|
||||
]),
|
||||
];
|
||||
|
||||
let messages = dispatcher.to_provider_messages(&history);
|
||||
assert_eq!(messages.len(), 2);
|
||||
assert_eq!(messages[0].role, "tool");
|
||||
// assistant tool_calls opener + one `tool` message per tool_call_id.
|
||||
assert_eq!(messages.len(), 3);
|
||||
assert_eq!(messages[0].role, "assistant");
|
||||
assert_eq!(messages[1].role, "tool");
|
||||
assert_eq!(messages[2].role, "tool");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn native_dispatcher_drops_standalone_orphan_tool_results() {
|
||||
// TAURI-RUST-7 regression: a `ToolResults` without a preceding
|
||||
// `AssistantToolCalls` is an invalid wire shape (the provider rejects
|
||||
// a `tool` message that doesn't follow a `tool_calls` opener). The
|
||||
// dispatcher must drop it before serialising.
|
||||
let dispatcher = NativeToolDispatcher;
|
||||
let history = vec![ConversationMessage::ToolResults(vec![ToolResultMessage {
|
||||
tool_call_id: "tc-orphan".into(),
|
||||
content: "stranded".into(),
|
||||
}])];
|
||||
|
||||
let messages = dispatcher.to_provider_messages(&history);
|
||||
assert!(
|
||||
messages.is_empty(),
|
||||
"standalone ToolResults must be dropped, got {} message(s)",
|
||||
messages.len()
|
||||
);
|
||||
}
|
||||
|
||||
// ═══════════════════════════════════════════════════════════════════════════
|
||||
|
||||
Reference in New Issue
Block a user