Skip to content

fix(core): publish MCP tool progress notifications - #59102

Open
mark-kojo wants to merge 2 commits into
anomalyco:devfrom
mark-kojo:fix/mcp-tool-progress
Open

mark-kojo wants to merge 2 commits into
anomalyco:devfrom
mark-kojo:fix/mcp-tool-progress

Conversation

@mark-kojo

@mark-kojo mark-kojo commented Oct 25, 2026 •

Copy link
Copy Markdown

Issue for this PR

Refs #49929 (the core half: the progress is published; drawing it on the TUI's tool card is not in this PR)

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

McpClient.callTool asks servers for progress (onprogress, so the SDK sends a progressToken) but the handler is () => {}, so every notifications/progress a server sends is dropped.

This passes an optional onProgress through Mcp.callTool to the client, and the MCP tool wrapper publishes each notification as the running call's progress with context.progress({ progress, total, message }), the same path the shell and subagent tools use. Clients then get it as session.tool.progress metadata. The SDK calls onprogress outside any fiber, so it runs with the tool's context.

Inside Code Mode (the default for MCP tools) a nested tool shares the execute call's context, so its progress used to replace that call's { toolCalls }. Each nested call now gets its own progress, which records the update on its toolCalls row (a new optional progress field) and drops it once that call has settled. The execute call's metadata is always its call list. This also changes the existing test in tool-registry.test.ts that expected a nested { stage: "old" } to replace the parent's progress.

We run an MCP server with calls that take a few minutes (reading a multi-page design), and our client had nothing to show until the call ended.

How did you verify your code works?

  • New test in test/mcp.test.ts: a server that sends two progress notifications during a tool call; the client's onProgress gets both, in order, with total and message.
  • New test in test/tool-registry.test.ts: two nested calls inside execute; every progress the parent publishes is a call list, the running call's row carries its progress, and a progress sent after the call settled publishes nothing.
  • bun test for test/mcp.test.ts (75), test/tool-registry.test.ts (42), test/tool-execute.test.ts, test/session-runner.test.ts, test/codemode and packages/codemode, all passing; bun run typecheck in packages/core.

Screenshots / recordings

Not a UI change.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Added by the opencode bot (AI-generated).

Related: #49929

Public API: adds an optional progress field to Code Mode toolCalls rows; MCP tools publish progress via session.tool.progress (backwards compatible)

callTool already asks servers for progress but drops each notification. Forward them to the running tool's progress so clients can show what a long MCP call is doing.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Direct MCP tools now forward progress, but MCP tools are Code Mode tools by default, and there the progress overwrites the execute call's own progress, so this needs a change before it's ready.

How I checked: the new client test passes. A temporary test through the tool registry (not committed) shows a progress notification from an MCP tool called inside execute replacing the execute call's { toolCalls } progress with { progress, total, message } until the next update. Core and the client both replace progress metadata instead of merging it, and the TUI draws the execute card from metadata.toolCalls. So during a long MCP call (the case this PR is for), the card loses its call list. Details are inline. bun test test/mcp.test.ts gives 74 pass and 1 fail; the failing test (terminates MCP descendants…) fails on the base too. typecheck:tsc passes.

Public API: no schema changes. MCP tools now emit the existing session.tool.progress event with progress/total/message metadata. The onProgress option and the McpClient.Progress type are internal to core.

Related: #49929

name: tool.name,
args: (input ?? {}) as Record<string, unknown>,
sessionID: context.sessionID,
onProgress: (progress) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When this tool runs inside Code Mode (the default for MCP tools), context belongs to the outer execute call, so context.progress({ progress, total, message }) replaces that call's { toolCalls } progress (publish-llm-event sets tool.progress = update, and client/src/solid/data.ts replaces state.metadata). I checked with a mocked onProgress call: the execute call's progress updates went {toolCalls:[running]} → {progress:1,total:2,message} → {toolCalls:[completed]}, so the execute card's call list disappears while a slow MCP call runs. The 'late notifications are ignored' guarantee also doesn't hold here: the outer execute call is still running, so a late notification from a nested call that has finished still gets published. Nested calls probably need their progress recorded per call (e.g. on the ExecuteCall entry through Code Mode's progress hooks) instead of overwriting the parent's metadata.

A tool called inside execute shares the execute call's context, so its progress replaced the call list. Record it on the call's own toolCalls row instead, and drop progress for a call that has settled.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new commit fixes the Code Mode problem from my earlier comment: a nested call's progress now goes on its own toolCalls row instead of replacing the execute call's progress, and progress sent after a call has finished is dropped. I re-ran my earlier temporary test. Inside execute (with and without arguments), every progress update was a call list, and the running row had progress: { progress, total, message }. A direct MCP tool still gets the bare { progress, total, message }. The related tests in packages/core pass, apart from terminates MCP descendants…, which also fails on the base. typecheck:tsc passes.

Public API: rows in the execute tool's toolCalls (both its progress and its final output) can now have an optional progress field. It's an added optional field, so existing clients keep working. MCP tools also now publish { progress, total, message } through the existing session.tool.progress event.

No further concerns.

This branch has not been deployed

No deployments
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.

1 participant