Skip to content

fix(cli): treat MCP tool isError as a failed mcp call - #128

Open
Arshgill01 wants to merge 2 commits into
CALLE-AI:mainfrom
Arshgill01:fix/mcp-call-tool-error
Open

fix(cli): treat MCP tool isError as a failed mcp call#128
Arshgill01 wants to merge 2 commits into
CALLE-AI:mainfrom
Arshgill01:fix/mcp-call-tool-error

Conversation

@Arshgill01

Copy link
Copy Markdown

Summary

  • Generic calle mcp call treated any HTTP 200 MCP tool result as success, including isError: true, so it printed ok: true and exited 0.
  • Dedicated calle call * already throws CallStageError in that case. mcp call now uses the same check and safe remote-error fields.
  • Docs now say a JSON-RPC 200 is not tool success, and a successful status lookup is not a successful telephone call.

Fixes #127.

Test plan

  • pnpm --filter @call-e/cli test:unit — 61 pass, including mcp call treats tool isError as a failed command
  • Existing mcp call success tests still exit 0
  • pnpm --filter @call-e/cli check
  • pnpm --filter @call-e/cli pack:dry-run
  • Reviewer: fixture with HTTP 200 + result.isError: true should now match calle call status (ok: false, exit 1) and must not print extra structured secrets

Made with Cursor

Generic `calle mcp call` reported ok:true and exited 0 when the tool
result had isError:true. Use the same CallStageError path as dedicated
call commands so shell automation cannot take the success branch.

Closes CALLE-AI#127

Co-authored-by: Cursor <cursoragent@cursor.com>

@Ray-56 Ray-56 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes on the exact current head (159cbe7).

  • [P1] Remote MCP text is copied into trusted stdout/stderr without a safe boundary. The new branch uses safeRemoteCallError(...).message as CallStageError.message, but this head's safeRemoteString only trims and slices. I reproduced an isError: true result whose message contained an OSC hyperlink, newline, and credential-shaped value; all three were emitted verbatim in error.message and stderr. Keep the summary fixed and CLI-authored. Put only strictly allowlisted, control-stripped, credential-redacted, bounded detail in a nested untrusted field, and add hostile stdout/stderr regressions. This exact head cannot rely on unmerged #106.

  • [P1] Generic failures default to unsafe real-world retry guidance. Every missing flag becomes call_started: false and retry_safe: true. For mcp call run_call, a failed/ambiguous response may follow carrier acceptance; for an unknown side-effecting tool, the CLI cannot know. Use conservative tool-aware defaults: run_call and unknown tools must be call_started: "unknown" / retry_safe: false unless trusted structured metadata proves otherwise. Add missing-flag run_call and unknown-side-effect tests.

  • [P2] An arbitrary tool name becomes trusted stage/code/message text. ${toolName}_error, the stage, and the fallback summary accept an unbounded user/remote-discovered name. Use a fixed CLI-owned code such as mcp_tool_error; validate/bound any inert tool identity and never interpolate it into trusted terminal output.

The CLI check, unit suite (61 pass, 1 platform skip), and package dry-run pass, but the new test currently asserts the unsafe remote message and does not cover side-effect ambiguity or hostile output. GitHub CI is action_required with zero jobs; this modifies real-call execution/error behavior, so the low-risk exception does not apply.

Release decision: Patch release recommended for @call-e/cli; the included patch changeset is appropriate after the blockers are fixed.

Do not copy remote isError text into trusted stdout/stderr. Default run_call
and unknown tools to unknown/not-retryable, and use a fixed mcp_tool_error
code instead of interpolating the tool name.

Co-authored-by: Cursor <cursoragent@cursor.com>
@Arshgill01

Copy link
Copy Markdown
Author

Addressed on f0c9cf5 against the 159cbe7 review.

  • P1 remote text: trusted error.message / stderr is now the fixed CLI-authored sentence The MCP tool returned an error. Remote isError text is only nested as error.untrusted.detail after C0/C1/OSC/ANSI stripping, credential-shaped redaction, and a 200-char bound. Added a hostile OSC + Bearer + sk- regression.
  • P1 retry defaults: plan_call / get_call_run stay call_started: false / retry_safe: true. run_call and unknown tools now default call_started: "unknown" / retry_safe: false unless trusted structured metadata says otherwise. Added missing-flag run_call and unknown-tool tests.
  • P2 tool name: trusted code is always mcp_tool_error. Known tools may appear as the stage; an unknown valid name is inert untrusted.tool with stage mcp_call. Invalid names are omitted.

Local CLI suite: 64 pass, 1 platform skip. GitHub CI is still action_required on this fork until a maintainer enables the workflows.

@Arshgill01

Copy link
Copy Markdown
Author

@Ray-56 the requested changes are addressed (see the summary above); ready for re-review whenever you have a moment. Happy to split or trim anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI: generic mcp call exits 0 for isError:true tool results

2 participants