Skip to content

fix: resolve worker tool call errors (browser_evaluate, browser_close, compaction) #475

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TheDarkSkyXD wants to merge 1 commit into
base: main
Choose a base branch
from
+69 −10
Open

fix: resolve worker tool call errors (browser_evaluate, browser_close, compaction) #475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions src/agent/compactor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,24 @@ impl Compactor {
return Ok(());
}

let remove_count = total / 2;
let mut remove_count = total / 2;

// Advance past User messages containing ToolResult blocks so we never
// orphan tool_results whose matching tool_use was in a removed assistant
// message (the Anthropic API rejects orphaned tool_result blocks).
while remove_count < total.saturating_sub(2) {
let has_tool_result = matches!(
history.get(remove_count),
Some(Message::User { content })
if content.iter().any(|item|
matches!(item, UserContent::ToolResult(_)))
);
if has_tool_result {
remove_count += 1;
} else {
break;
}
}

let removed: Vec<Message> = history.drain(..remove_count).collect();
drop(removed);
Expand Down Expand Up @@ -206,12 +223,29 @@ async fn run_compaction(
let (removed_messages, remove_count) = {
let mut hist = history.write().await;
let total = hist.len();
let remove_count = ((total as f32 * fraction) as usize)
let mut remove_count = ((total as f32 * fraction) as usize)
.max(1)
.min(total.saturating_sub(2));
if remove_count == 0 {
return Ok(0);
}

// Advance past User messages containing ToolResult blocks so we never
// orphan tool_results whose matching tool_use was in a removed message.
while remove_count < total.saturating_sub(2) {
let has_tool_result = matches!(
hist.get(remove_count),
Some(Message::User { content })
if content.iter().any(|item|
matches!(item, UserContent::ToolResult(_)))
);
if has_tool_result {
remove_count += 1;
} else {
break;
}
}

let removed: Vec<Message> = hist.drain(..remove_count).collect();
(removed, remove_count)
};
Expand Down
22 changes: 21 additions & 1 deletion src/agent/worker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,9 +746,29 @@ impl Worker {
let estimated = estimate_history_tokens(history);
let usage = estimated as f32 / context_window as f32;

let remove_count = ((total as f32 * fraction) as usize)
let mut remove_count = ((total as f32 * fraction) as usize)
.max(1)
.min(total.saturating_sub(2));

// Advance the boundary past User messages that contain ToolResult blocks.
// If we stop right after an Assistant message with ToolCalls, the next
// User message holds the corresponding ToolResults. Leaving those orphaned
// causes the Anthropic API to reject the request with:
// "unexpected tool_use_id found in tool_result blocks"
while remove_count < total.saturating_sub(2) {
let has_tool_result = matches!(
history.get(remove_count),
Some(rig::message::Message::User { content })
if content.iter().any(|item|
matches!(item, rig::message::UserContent::ToolResult(_)))
);
if has_tool_result {
remove_count += 1;
} else {
break;
}
}
Comment on lines +749 to +770
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check if branch.rs compaction logic handles tool_result boundaries

# Search for drain operations in branch.rs and surrounding context
rg -n -B10 -A5 'drain\(' src/agent/branch.rs

# Check if ToolResult handling exists in branch.rs
rg -n 'ToolResult' src/agent/branch.rs

Repository: spacedriveapp/spacebot

Length of output: 733

🏁 Script executed:

# Check if branch.rs builds agents with tool_server_handle
rg -n 'tool_server_handle|ToolCall|tools' src/agent/branch.rs | head -20

# Check the branch agent construction pattern
rg -n -A15 'AgentBuilder::new\|\.build\(' src/agent/branch.rs | head -40

Repository: spacedriveapp/spacebot

Length of output: 179

Apply ToolResult safeguard to branch compaction as well.

The worker.rs fix (lines 749-770) correctly prevents orphaning ToolResult blocks when compaction removes preceding ToolCall messages. However, branch.rs has identical compaction logic at line 296 without this safeguard. Since branch.rs enables tools via .tool_server_handle(self.tool_server.clone()) at line 116, branches can also accumulate tool interactions and will hit the same Anthropic API rejection: "unexpected tool_use_id found in tool_result blocks".

Apply the same while-loop boundary advancement from worker.rs (lines 756–770) to branch.rs's compact_history() function to prevent runtime failures when branches compact their history.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/agent/worker.rs` around lines 749 - 770, The branch.rs compact_history()
implementation needs the same ToolResult-safeguard as worker.rs: when computing
remove_count (based on total and fraction) advance the boundary forward while
the next message in history is a User message containing any
UserContent::ToolResult to avoid orphaning ToolResult blocks; copy the while
loop logic from worker.rs that checks history.get(remove_count) for
Some(rig::message::Message::User { content }) and content.iter().any(|item|
matches!(item, rig::message::UserContent::ToolResult(_))) and increments
remove_count until the condition fails (respecting total.saturating_sub(2)),
keeping the same remove_count/total/rust matches so branches using
.tool_server_handle(self.tool_server.clone()) won't produce "unexpected
tool_use_id found in tool_result blocks".


let removed: Vec<rig::message::Message> = history.drain(..remove_count).collect();
compacted_history.extend(removed.iter().cloned());

Expand Down
2 changes: 1 addition & 1 deletion src/config/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,7 @@ impl Default for BrowserConfig {
Self {
enabled: true,
headless: true,
evaluate_enabled: false,
evaluate_enabled: true,
executable_path: None,
screenshot_dir: None,
persist_session: false,
Expand Down
17 changes: 11 additions & 6 deletions src/tools/browser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2108,12 +2108,17 @@ impl Tool for BrowserCloseTool {
task.abort();
}

if let Some(mut browser) = browser
&& let Err(error) = browser.close().await
{
let message = format!("failed to close browser: {error}");
tracing::warn!(policy = "close_browser", %message);
return Err(BrowserError::new(message));
if let Some(mut browser) = browser {
if let Err(error) = browser.close().await {
// The browser connection may already be dead (e.g. "oneshot canceled"
// from a dropped WebSocket). That's fine — the browser is effectively
// closed. Log it but don't fail the tool call.
tracing::warn!(
policy = "close_browser",
%error,
"browser.close() failed, treating as already closed"
);
}
Comment on lines +2057 to +2067
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Only suppress the known “browser already gone” close errors.

This now turns every browser.close().await failure into success, which can hide real shutdown failures and leave Chrome running after local state has already been dropped. Because the handler is torn down immediately before this block, some errors here can also come from our own teardown ordering rather than from the browser already being dead. Please match only the disconnected/closed-transport cases here and keep returning a structured tool error for everything else. As per coding guidelines "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them."

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/tools/browser.rs` around lines 2057 - 2067, The current handler
indiscriminately suppresses all errors from browser.close().await; change it to
only treat known "already gone" transport errors as non-fatal by matching the
error kind/message (e.g., "oneshot canceled", disconnected/closed
WebSocket/transport) coming from browser.close().await and keep the existing
tracing::warn for those cases (policy = "close_browser"). For any other error
from browser.close().await (i.e., not matching the known disconnected/transport
patterns), do not swallow it—convert or propagate it as a structured tool error
(return Err(...) or propagate via the function's error type) so shutdown
failures are surfaced instead of silently ignored; locate this logic around the
browser variable and the browser.close() call and adjust the match/if-let
accordingly.

}

if !persistent_profile && let Some(dir) = user_data_dir {
Expand Down