fix: resolve worker tool call errors (browser_evaluate, browser_close, compaction)#475
Open
TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Open
fix: resolve worker tool call errors (browser_evaluate, browser_close, compaction)#475TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
TheDarkSkyXD wants to merge 1 commit intospacedriveapp:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughModifies agent compaction logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…, compaction)
Three fixes for tool call errors observed during worker execution:
1. Enable browser_evaluate by default (config/types.rs)
Workers using browser_evaluate were always hitting "JavaScript evaluation
is disabled in browser config". Changed the default from false to true so
workers can extract page content without requiring explicit config changes.
2. Handle browser_close gracefully on dead connections (tools/browser.rs)
When the browser's WebSocket connection is already gone (e.g. process
killed, connection timeout), browser.close() fails with "oneshot canceled".
This is not a real error — the browser is effectively closed. Now logs a
warning and returns success instead of propagating the error to the worker.
3. Fix history compaction orphaning tool_result blocks (agent/worker.rs,
agent/compactor.rs)
When compaction removes messages by draining from the front of history,
the drain boundary could fall between an assistant message (with tool_use
blocks) and the following user message (with tool_result blocks). This
left orphaned tool_results referencing removed tool_use_ids, causing the
Anthropic API to reject requests with:
"unexpected tool_use_id found in tool_result blocks"
Fixed in all three compaction paths (worker compact_history, channel
emergency_truncate, channel run_compaction) by advancing the removal
boundary past any User messages containing ToolResult blocks.
All fixes are platform-independent (Windows, macOS, Linux/Docker).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TheDarkSkyXD
force-pushed
the
fix/worker-tool-call-errors
branch
from
f167a47 to
d519e35
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/config/types.rs (1)
840-846:⚠️ Potential issue | 🟠 MajorKeep arbitrary page evaluation opt-in.
src/tools/browser.rsonly gatesbrowser_evaluateon this flag, so flipping the default enables arbitrarypage.evaluate(...)for every agent that already has browser tools turned on. That materially broadens the security envelope—from DOM/storage reads to browser-originated requests that bypass the normalvalidate_url()path—without an explicit migration step. Please keep this disabled by default or gate the rollout behind an opt-in config change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/types.rs` around lines 840 - 846, The BrowserConfig Default currently enables evaluate_enabled which turns on arbitrary page.evaluate for agents; change the Default impl for BrowserConfig to set evaluate_enabled: false so page evaluation remains opt-in (update the Default for BrowserConfig where enabled/headless/executable_path are set). Also review the gating in src/tools/browser.rs (browser_evaluate) to ensure it continues to rely on this flag for rollout control.interface/src/components/ModelSelect.tsx (1)
17-37:⚠️ Potential issue | 🟡 MinorKeep the provider metadata table in sync with the backend.
src/api/models.rscan already emit providers likemoonshotandzai-coding-plan, and this PR starts surfacing more provider IDs again. Because those keys are missing from bothPROVIDER_LABELSandproviderOrder, the dropdown falls back to raw IDs and pushes them to the bottom. Add entries here for the full backend provider set.Also applies to: 45-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ModelSelect.tsx` around lines 17 - 37, PROVIDER_LABELS and providerOrder are missing several provider IDs emitted by the backend (e.g., moonshot, zai-coding-plan and others), causing raw IDs to appear and sort incorrectly; update the PROVIDER_LABELS constant and the providerOrder array in ModelSelect.tsx to include all provider keys returned by src/api/models.rs (add entries like "moonshot" and "zai-coding-plan" with user-friendly labels) so the dropdown shows proper names and ordering—modify the PROVIDER_LABELS object and the providerOrder array to mirror the backend provider set exactly.
🧹 Nitpick comments (1)
src/agent/compactor.rs (1)
226-247: Correct implementation; consider extracting shared helper.The logic is identical to
emergency_truncateabove andworker.rs::compact_history. All three correctly prevent orphanedtool_resultblocks.The pattern is now duplicated in three places. If this logic needs adjustment later (e.g., also skipping
ToolCallmessages without results), you'd need to update all three.♻️ Optional: Extract shared helper
// In a shared module (e.g., src/agent/compaction_utils.rs or at the top of compactor.rs) /// Advance `remove_count` past any User messages containing ToolResult blocks /// to avoid orphaning tool_results whose matching tool_use was removed. pub fn advance_past_tool_results(history: &[Message], mut remove_count: usize) -> usize { let total = history.len(); 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; } } remove_count }Then in each call site:
let mut remove_count = /* initial calculation */; remove_count = advance_past_tool_results(&history, remove_count);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/compactor.rs` around lines 226 - 247, The duplicate loop that advances remove_count past User messages containing ToolResult blocks in compactor.rs should be extracted into a shared helper to avoid repetition with emergency_truncate and worker.rs::compact_history; create a function (e.g., advance_past_tool_results(history: &[Message], remove_count: usize) -> usize) that encapsulates the matches! check for Message::User { content } and UserContent::ToolResult(_) and call that helper from compactor.rs (and replace the duplicated code in emergency_truncate and worker.rs::compact_history) so future changes (like also skipping ToolCall messages) are applied in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ModelSelect.tsx`:
- Around line 209-214: The ArrowUp handler mutates highlightIndex even when the
popup is closed, allowing Enter to select an unseen item; mirror the ArrowDown
behavior by checking the popup open state (e.g., open or isOpen) and, if the
popup is closed, open it instead of just changing highlightIndex. Update the
ArrowUp key branch in the onKeyDown handler to call setOpen(true) (or the
component's open toggler) when the list is closed; otherwise keep the existing
setHighlightIndex logic that wraps via flatList and highlightIndex. Ensure the
same open-check semantics used by the ArrowDown handler are applied to ArrowUp
to prevent hidden selections.
In `@src/agent/worker.rs`:
- Around line 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".
In `@src/api/models.rs`:
- Around line 290-301: The code only treats "ANTHROPIC_API_KEY" and the OAuth
JSON file as evidence Anthropic is configured; update the detection to also
consider "ANTHROPIC_AUTH_TOKEN". Modify the check that calls
has_key("anthropic_key", "ANTHROPIC_API_KEY") (and the surrounding branch that
pushes into providers) so it returns true if either has_key("anthropic_key",
"ANTHROPIC_API_KEY") OR has_key("anthropic_key", "ANTHROPIC_AUTH_TOKEN"); keep
the existing OAuth JSON fallback that checks
crate::auth::credentials_path(instance_dir).exists() so instances using the
bearer-token path are included in providers. Ensure you reference the same
providers vector and config_path logic when making this change.
In `@src/tools/browser.rs`:
- Around line 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.
---
Outside diff comments:
In `@interface/src/components/ModelSelect.tsx`:
- Around line 17-37: PROVIDER_LABELS and providerOrder are missing several
provider IDs emitted by the backend (e.g., moonshot, zai-coding-plan and
others), causing raw IDs to appear and sort incorrectly; update the
PROVIDER_LABELS constant and the providerOrder array in ModelSelect.tsx to
include all provider keys returned by src/api/models.rs (add entries like
"moonshot" and "zai-coding-plan" with user-friendly labels) so the dropdown
shows proper names and ordering—modify the PROVIDER_LABELS object and the
providerOrder array to mirror the backend provider set exactly.
In `@src/config/types.rs`:
- Around line 840-846: The BrowserConfig Default currently enables
evaluate_enabled which turns on arbitrary page.evaluate for agents; change the
Default impl for BrowserConfig to set evaluate_enabled: false so page evaluation
remains opt-in (update the Default for BrowserConfig where
enabled/headless/executable_path are set). Also review the gating in
src/tools/browser.rs (browser_evaluate) to ensure it continues to rely on this
flag for rollout control.
---
Nitpick comments:
In `@src/agent/compactor.rs`:
- Around line 226-247: The duplicate loop that advances remove_count past User
messages containing ToolResult blocks in compactor.rs should be extracted into a
shared helper to avoid repetition with emergency_truncate and
worker.rs::compact_history; create a function (e.g.,
advance_past_tool_results(history: &[Message], remove_count: usize) -> usize)
that encapsulates the matches! check for Message::User { content } and
UserContent::ToolResult(_) and call that helper from compactor.rs (and replace
the duplicated code in emergency_truncate and worker.rs::compact_history) so
future changes (like also skipping ToolCall messages) are applied in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfcfc43f-91ff-498b-a30d-bc9abc2edc35
⛔ Files ignored due to path filters (50)
desktop/src-tauri/gen/schemas/windows-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/icons/64x64.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square107x107Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square142x142Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square150x150Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square284x284Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square30x30Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square310x310Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square44x44Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square71x71Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/Square89x89Logo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/StoreLogo.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-anydpi-v26/ic_launcher.xmlis excluded by!**/*.xmldesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-hdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-mdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher_foreground.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/mipmap-xxxhdpi/ic_launcher_round.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/android/values/ic_launcher_background.xmlis excluded by!**/*.xmldesktop/src-tauri/icons/icon.icois excluded by!**/*.ico,!**/*.icodesktop/src-tauri/icons/icon.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-20x20@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-29x29@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@2x-1.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-40x40@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-512@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-60x60@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-60x60@3x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-76x76@1x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-76x76@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/src-tauri/icons/ios/AppIcon-83.5x83.5@2x.pngis excluded by!**/*.png,!**/*.pngdesktop/tauri.windows.conf.jsonis excluded by!**/*.json
📒 Files selected for processing (7)
interface/src/components/ModelSelect.tsxscripts/bundle-sidecar.ps1src/agent/compactor.rssrc/agent/worker.rssrc/api/models.rssrc/config/types.rssrc/tools/browser.rs
Comment on lines
+209
to
+214
| if (e.key === "ArrowUp") { | ||
| e.preventDefault(); | ||
| setHighlightIndex((prev) => | ||
| prev > 0 ? prev - 1 : flatList.length - 1, | ||
| ); | ||
| return; |
Contributor
There was a problem hiding this comment.
Mirror ArrowDown when the popup is closed.
When the field is closed, ArrowUp only mutates highlightIndex. A following Enter can then select the last model even though the list never opened, so keyboard users can change the value blindly.
Suggested fix
if (e.key === "ArrowUp") {
e.preventDefault();
+ if (!open) {
+ setOpen(true);
+ setFilter(value);
+ setHighlightIndex(flatList.length - 1);
+ return;
+ }
setHighlightIndex((prev) =>
prev > 0 ? prev - 1 : flatList.length - 1,
);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (e.key === "ArrowUp") { | |
| e.preventDefault(); | |
| setHighlightIndex((prev) => | |
| prev > 0 ? prev - 1 : flatList.length - 1, | |
| ); | |
| return; | |
| if (e.key === "ArrowUp") { | |
| e.preventDefault(); | |
| if (!open) { | |
| setOpen(true); | |
| setFilter(value); | |
| setHighlightIndex(flatList.length - 1); | |
| return; | |
| } | |
| setHighlightIndex((prev) => | |
| prev > 0 ? prev - 1 : flatList.length - 1, | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/components/ModelSelect.tsx` around lines 209 - 214, The ArrowUp
handler mutates highlightIndex even when the popup is closed, allowing Enter to
select an unseen item; mirror the ArrowDown behavior by checking the popup open
state (e.g., open or isOpen) and, if the popup is closed, open it instead of
just changing highlightIndex. Update the ArrowUp key branch in the onKeyDown
handler to call setOpen(true) (or the component's open toggler) when the list is
closed; otherwise keep the existing setHighlightIndex logic that wraps via
flatList and highlightIndex. Ensure the same open-check semantics used by the
ArrowDown handler are applied to ArrowUp to prevent hidden selections.
Comment on lines
+749
to
+770
| 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; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 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.rsRepository: 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 -40Repository: 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".
src/api/models.rs
Outdated
Comment on lines
+290
to
+301
| if has_key("anthropic_key", "ANTHROPIC_API_KEY") { | ||
| providers.push("anthropic"); | ||
| } | ||
| // Anthropic OAuth stores credentials as a separate JSON file | ||
| if !providers.contains(&"anthropic") { | ||
| if config_path | ||
| .parent() | ||
| .is_some_and(|instance_dir| crate::auth::credentials_path(instance_dir).exists()) | ||
| { | ||
| providers.push("anthropic"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Also treat ANTHROPIC_AUTH_TOKEN as configured here.
The OAuth file fallback is good, but this block still only considers ANTHROPIC_API_KEY plus the JSON credential file. anthropic_key can also come from ANTHROPIC_AUTH_TOKEN in src/config/types.rs, so instances using the bearer-token path will still look unconfigured to /api/models and the picker hides Anthropic models even though auth is valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/models.rs` around lines 290 - 301, The code only treats
"ANTHROPIC_API_KEY" and the OAuth JSON file as evidence Anthropic is configured;
update the detection to also consider "ANTHROPIC_AUTH_TOKEN". Modify the check
that calls has_key("anthropic_key", "ANTHROPIC_API_KEY") (and the surrounding
branch that pushes into providers) so it returns true if either
has_key("anthropic_key", "ANTHROPIC_API_KEY") OR has_key("anthropic_key",
"ANTHROPIC_AUTH_TOKEN"); keep the existing OAuth JSON fallback that checks
crate::auth::credentials_path(instance_dir).exists() so instances using the
bearer-token path are included in providers. Ensure you reference the same
providers vector and config_path logic when making this change.
Comment on lines
+2057
to
+2067
| 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" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
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.
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 three tool call errors that occur during worker execution, causing workers to fail or produce degraded results:
browser_evaluatedisabled by default — Workers hittingbrowser_evaluatealways got "JavaScript evaluation is disabled". Changed the default totrueso workers can extract page content without config changes.browser_closefailing on dead connections — When the browser WebSocket is already gone (e.g. "oneshot canceled"),browser.close()was returning an error to the worker. Now logs a warning and returns success since the browser is effectively closed.tool_resultblocks — When compaction drains messages from history, the boundary could split atool_use/tool_resultpair, leaving orphanedtool_resultblocks that reference removedtool_useIDs. The Anthropic API rejects these with:"unexpected tool_use_id found in tool_result blocks". Fixed in all three compaction paths (workercompact_history, channelemergency_truncate, channelrun_compaction) by advancing the removal boundary past User messages containingToolResultblocks.Files Changed
src/config/types.rsevaluate_enableddefaultfalse→truesrc/tools/browser.rsbrowser_closereturns success on dead connectionssrc/agent/worker.rssrc/agent/compactor.rsPlatform
All fixes are platform-independent — affects Windows, macOS, and Linux/Docker equally.
Test plan
browser_evaluate— should succeed without config changesbrowser_close— should return success with a warning log400 Bad Requesterrors from orphanedtool_resultblocks🤖 Generated with Claude Code