Preserve MCP tool error outcomes - #2138
Conversation
ApprovabilityVerdict: Needs human review This PR introduces behavioral changes to tool error handling with a new field on a core type. An unresolved review comment from the file's primary author questions the fundamental design approach, which should be resolved before merging. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
47ea576 to
1433270
Compare
| content: MessageContent | ||
| name: str | None = None | ||
| """Needed by templates such as Harmony when bridge tails omit the issuing call.""" | ||
| is_error: bool = False |
There was a problem hiding this comment.
why do we need this departure from standard api? can we not just put the error in the content and call it a day?
df1ae39 to
05ea449
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 05ea449. Configure here.
05ea449 to
254c065
Compare
254c065 to
e056a0a
Compare
e056a0a to
6b5814a
Compare
| # `reasoning` (vLLM / Together / OpenRouter), `reasoning_content` (DeepSeek / Qwen / SGLang / | ||
| # Fireworks / Kimi), `reasoning_details` (OpenRouter / MiniMax). | ||
| REASONING_FIELDS = ("reasoning", "reasoning_content", "reasoning_details") | ||
| TOOL_ERROR_KEY = "_vf_is_error" |
There was a problem hiding this comment.
do we need this only to prefix with TOOL_ERROR_PREFIX? can we not just let the error type bubble up as is. i think its going to be clear enough that the tool call failed. this is also what we do in e.g. rlm harness. e.g. my terminal is also not prefixing every error with Command failed:\n
6b5814a to
daab68f
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |

Overview
Preserves MCP tool-declared outcome semantics across harness execution, typed messages, traces, replay, and provider conversion.
This PR is stacked on #2134 and adds structured tool-error transport on top of its shared null/bash lifecycle and replay-safety foundation.
Details
MCP
CallToolResultcontains both model-visible content and anisErroroutcome. The shared MCP client now returns both values, and the harness program carries the outcome intoToolMessage.is_error.The typed outcome participates in graph identity and trace serialization, so success and failure results with identical content remain distinguishable after persistence and resume.
For chat-compatible providers, the localhost harness handoff uses a private marker that is removed before the provider request while adding a readable failure label to the model-visible content. Anthropic-native
tool_result.is_errormaps directly into the same typed representation.Note
Medium Risk
message_hashnow differs for error vs success tool messages, which can change graph dedup for existing traces; provider-facing tool content also gains a failure prefix on chat wire output.Overview
Adds
ToolMessage.is_errorso tool failures are typed end-to-end instead of living only in message text.MCP harness:
call_mcpnow returns MCPisError; tool messages can carry a private_vf_is_errormarker, and chat requests useextra_body.messagesso that marker survives the localhost interception path.Wire / providers: Chat serialization prefixes model-visible content with
Tool execution failed:when not using internal mode; harness handoff writes initial messages withinclude_internal=True. Anthropictool_result.is_errormaps into the same field.apply_overridesre-normalizes messages that still carry the internal marker before the upstream request.Traces:
message_hashfor tool nodes includes error vs success, so identical content from a failed vs successful tool call stays distinct after persistence and resume.Reviewed by Cursor Bugbot for commit daab68f. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Preserve MCP tool error outcomes across harness, transcript, and retry logic
is_errortoToolMessagein types.py and propagates it through Anthropic and chat dialect parsing/serialization, including a_vf_is_errorinternal key and a human-visibleTool execution failed:prefix on wire output.MCPTransportErrorandMCPDeliveryUnknownErrorin errors.py; the harness raises these instead of a genericHarnessErrorwhen the program emits aVF_MCP_ERROR=JSON marker.--bashflag; both harnesses now write initial messages withinclude_internal=Trueto preserve error markers.mcp_sessionin the minimal program classifies MCP failures as transient/non-transient and replay-safe/unsafe, andwith_retrynarrows retries to replay-safe transport failures only.MCPDeliveryUnknownErrorare blocked from retry in retries.py unless explicitly opted in via retry config.message_hashin graph.py now differs between error and success tool outcomes, which may invalidate cached hashes for existing tool messages.📊 Macroscope summarized daab68f. 13 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.