Surface Copilot tool progress instead of discarding it - #334131
Surface Copilot tool progress instead of discarding it#334131Ryan Ewen (RyanEwen) wants to merge 4 commits into
Conversation
The Codex mapper appended every mcpToolCall/progress message to the entry's output and re-sent the whole concatenation as the content of a ChatToolCallContentChanged action. That output field is what completion falls back to, so when an MCP tool legitimately returned nothing the progress narration was persisted as its result, an orphaned call's result became the narration, and each notification resent an ever-growing string. Progress now rides on _meta.progressMessage, a new IToolCallMeta key that readToolCallMeta validates. The mapper records the _meta it emitted at start on the tool call entry and spreads it into the progress action, because the reducer replaces the whole bag whenever an action carries one. Identical consecutive messages are coalesced. The mcpToolCall completion no longer falls back to entry.output: nothing writes output for that item type any more, and keeping the fallback would have left the one path by which narration could still leak into the result. On the workbench side, updateRunningToolSpecificData and toolCallStateToInvocation surface the message through ChatToolInvocation.acceptProgress while the call is Running, so a client attaching mid-turn sees it as well. finalizeToolInvocation clears the progress before didExecuteTool, which otherwise promotes any progress message to pastTenseMessage whenever toolResultMessage is absent. The adapter never sets toolResultMessage, so without the clear the last progress string replaced the provider's past-tense text and became the label for providers that omit one.
`applyToolCallProgress` gated the call to `acceptProgress` on the message being truthy, so an empty string was dropped. `progressMessage` is typed and validated as any string, and Codex can send an empty message to clear a status line it set earlier, so the invocation kept showing the stale text instead. Both the running-refresh path and the mid-turn seeding path go through this one helper, so both were affected; testing for undefined instead fixes them together. The mapper needed no change. It coalesces on `params.message === entry.progressMessage`, and the entry's `progressMessage` starts undefined rather than empty, so a first empty message is a real change and still emits; only a repeat of the same empty message is dropped.
`tool.execution_progress` was only trace-logged, so an MCP server's status messages never reached the client. Route them through the same `_meta.progressMessage` channel Codex uses, so both providers behave the same. The message stays out of the tool call's content, repeats are coalesced, and the action is only emitted while the tracked call is Running, which is the one status in which the reducer applies a content change. `tool.execution_partial_result` is untouched: that carries the tool's real output and keeps streaming to its terminal resource.
There was a problem hiding this comment.
🟡 Changes recommended
The authentication pause-and-resume path central to the progress gate lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Routes transient Copilot and Codex tool progress through shared metadata into the workbench progress line without altering tool output.
Changes:
- Adds typed
_meta.progressMessagehandling. - Emits and coalesces provider progress updates.
- Displays and clears transient invocation progress, with regression coverage.
File summaries
| File | Description |
|---|---|
agentToolCallMeta.ts |
Defines and validates progress metadata. |
codexMapAppServerEvents.ts |
Separates Codex progress from output. |
codexMapAppServerEvents.test.ts |
Tests Codex progress mapping. |
copilotAgentSession.ts |
Emits gated Copilot progress actions. |
copilotAgentSession.test.ts |
Tests Copilot progress behavior. |
stateToProgressAdapter.ts |
Applies progress to workbench invocations. |
stateToProgressAdapter.test.ts |
Tests progress display and finalization. |
agentMetaReaders.test.ts |
Tests progress metadata validation. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| private _emitToolCallProgress(toolCallId: string, progressMessage: string): void { | ||
| const tracked = this._activeToolCalls.get(toolCallId); | ||
| if (!tracked || !tracked.isRunning || tracked.progressMessage === progressMessage) { | ||
| return; | ||
| } | ||
| tracked.progressMessage = progressMessage; |
There was a problem hiding this comment.
Correct, and the gate's justification was reasoning in a commit message with nothing behind it, which is exactly what gets refactored away quietly. Added in 3f9a954.
The test drives the real transitions rather than poking the flag: an MCP authentication request moves the call to AuthRequired, progress is emitted and asserted absent, authentication is resolved so the call returns to Running, then the same message is emitted again and asserted present. Both transitions travel through the same emit path production uses.
I ran two perturbations rather than one, because "nothing was emitted" can pass for the wrong reason. Recording the message regardless of running state leaves the paused assertion unchanged and breaks only the post-resume one, which is the coalescing regression you described and is invisible to every other test in the file. Removing the running gate entirely breaks the paused assertion instead, which proves the call really is in AuthRequired and the pause is genuinely exercised. Each fails only this test.
copilotAgentSession is now 418 passing. The production file is untouched by this commit.
AI disclosure: this comment and the related code were written with the assistance of AI.
The running-state gate had no test for the pause it exists to handle. Emit progress while the call sits in AuthRequired, resolve authentication, then emit the same message again: it must be published the second time, which only holds because the paused message was never recorded as last sent. Without that, a message the reducer discarded would still count as sent and the valid repeat would be coalesced away, leaving the status line stuck.
Routes Copilot's tool progress to the tool's progress line instead of discarding it, matching what #334053 does for Codex.
Stacked on #334053. That PR owns the shared plumbing, the
progressMessagemeta key and the workbench adapter hook, so this branch is based on it rather than on main. The commit list below therefore includes its two commits. This is a draft until it lands; rebasing onto main afterwards leaves only the Copilot commit.The gap
Copilot receives
tool.execution_progressand only trace-logs it, so the status a tool reports while running is discarded. Codex had the opposite problem, merging progress into the tool's output, which #334053 fixes by routing it through_meta.progressMessage. This gives Copilot the same route.The payload is the same concept
Worth stating, since it decides the treatment. The SDK declares
ToolExecutionProgressDataasprogressMessage: string, documented as a human-readable status message from an MCP server, plus the tool call id. No percentage, no structured payload, no textless heartbeat. It is the same MCP progress notification Codex maps, so nothing had to be forced into a message field.The change
The session tracks whether each tool call is Running and what progress it last sent, then emits
ChatToolCallContentChangedcarrying the message in_metawith the content left untouched, routed through the parent tool call id for a subagent. Repeats are coalesced.The Running flag mirrors the reducer's own transitions and is maintained in the single funnel every emitted action passes through, so it cannot drift as emission sites are added. The gate matters: the reducer ignores a content-changed action unless the call is Running, so a message emitted while the call is paused awaiting authentication would be dropped by the reducer yet still recorded as last sent, and a later valid repeat would then be coalesced away and never shown. The reducer also replaces the whole meta bag, so the start-time meta is re-spread, following the existing convention in this file.
Progress is not partial output
Distinguished by what the payload is, not by tool name.
tool.execution_partial_resultcarries the tool's real stdout as cumulative snapshots: it is gated on shell tools, appended to a terminal resource and to the call's content, and reaches the result.tool.execution_progresscarries a transient status line that never touches content and lives only in meta. The partial-result path is untouched, and a test asserts a shell tool interleaving both keeps its terminal channel and content identical while the progress action rides alongside carrying only meta.The workbench needed no change
The adapter reads the meta key off any Running tool call and never inspects scheme, tool name or contributor, and Copilot's action flows through the identical reducer path. This commit touches no workbench file, which confirms #334053's adapter work is genuinely provider-agnostic rather than Codex-specific.
Tests
A progress event travels as meta without altering content, identical repeats are coalesced, progress after completion is dropped, and the partial-result terminal channel is unaffected. Three of the four fail without the change; the fourth is an absence assertion that passes either way by design.
copilotAgentSession417 passing,copilotAgent335 passing,agentMetaReaders32 passing. No new type errors.Part of #333174.
AI disclosure: this comment and the related code were written with the assistance of AI.