Repository navigation
fix: report failed actions as MCP tool errors - #280
Conversation
There was a problem hiding this comment.
This is an auto review done by revuto.
Tying isError to ok in structured_tool_result fixes the protocol problem described in the PR. It also changes what clients see for every refusal that comes back as ok:false. Two of the repo's own MCP regression scripts (scripts/semantic_click_test.py and scripts/repro_wtype_electron.cjs) still assume refusals return isError:false. They will now fail on their expected-refusal cases. Update them in this PR.
| })?; | ||
| let is_error = value.get("ok").and_then(serde_json::Value::as_bool) == Some(false); | ||
| let mut result = CallToolResult::structured(value); | ||
| result.is_error = Some(is_error); |
There was a problem hiding this comment.
This is an auto review done by revuto.
With this change, every ok:false result now comes back as isError: true. That includes refusals that are part of the safety contract, like password-field rejection and a disabled pointer fallback, not just real execution failures. Two of the repo's own MCP regression scripts still expect the old isError: false and will now fail:
scripts/semantic_click_test.py: the sharedrpc()helper runsassert not result.get('isError'), result(line 34) on every response. Lines 118–123 then make clicks that are supposed to be refused (click {x:0,y:0}and the entryelement_indexclick with pointer backends disabled) and checknot ...['ok']. Those calls now returnisError: true, so the assert inrpc()fails before theokcheck runs.scripts/repro_wtype_electron.cjs:67: themcp-passwordmode passes only when!response.isError && payload.ok === false && payload.message.includes('password fields').type_textsends itsErr(...)fromdispatch_type_textthroughfinish_action_outputand then this builder. The password refusal now setsisError: true, so that mode exits 1.
The Rust unit tests and the CI list in the PR description don't run these scripts, which is why this wasn't caught. Please update both scripts in this PR: allow isError when the expected result is ok:false, and require isError === true there. A CHANGELOG entry would also help, since hosts that handle isError differently from ok will see a change in behavior.
4a2f750 to
bfeb1a0
Compare
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
|
Thanks for the fix, @OnlyHa1bara — LGTM! |
|
Released in 0.7.23. Thanks @OnlyHa1bara for the MCP error-result fix. The final release passes the MCP safety/schema checks and keeps successful actions successful when only post-action observation fails. |
Why
Failed desktop actions returned
ok: falsein their structured output, but the shared result builder reportedisError: false. MCP clients could therefore treat refusals and execution failures as successful tool calls.What Changed
Set the MCP error flag when the shared result builder receives an explicit top-level
ok: false. Extend existing assertions for failed actions and observation-only results. Successful actions remain successful when only post-action capture fails.Update the existing GTK click regression and Electron input reproduction to expect
isError: truefor intentional refusals, while preserving the success, payload, refusal-reason, and UI outcome checks.Validation