Keep Codex MCP tool progress out of the tool result - #334053
Giuseppe Cianci (Giuspepe) merged 2 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.
There was a problem hiding this comment.
🟡 Changes recommended
Empty progress messages currently leave stale progress visible.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Routes Codex MCP narration through transient progress metadata instead of tool output.
Changes:
- Adds typed
progressMessagemetadata and workbench handling. - Prevents narration from persisting as empty/orphaned results.
- Adds mapper, metadata, and adapter tests.
File summaries
| File | Description |
|---|---|
agentToolCallMeta.ts |
Defines and validates progress metadata. |
codexMapAppServerEvents.ts |
Separates progress from output. |
agentMetaReaders.test.ts |
Tests metadata validation. |
codexMapAppServerEvents.test.ts |
Tests Codex progress mapping. |
stateToProgressAdapter.ts |
Applies and clears invocation progress. |
stateToProgressAdapter.test.ts |
Tests progress display and finalization. |
Review details
- Files reviewed: 6/6 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.
| if (progressMessage) { | ||
| invocation.acceptProgress({ message: progressMessage }); | ||
| } |
There was a problem hiding this comment.
Correct. Fixed in 65c23c1: the check is now progressMessage !== undefined, so an empty string reaches acceptProgress and clears the display. Both call sites go through the one helper, and the Running guard above it is unchanged, so a non-Running call is still skipped.
The mapper side needs no change, and I checked rather than assumed: entry.progressMessage starts undefined rather than '', so '' === undefined is false and an initial empty message is treated as a real change and emits; only a repeated empty message is coalesced, which is the intended behaviour. The wire path preserves it too, since toToolCallMeta drops only undefined values and readToolCallMeta accepts any string.
Tests: an empty _meta.progressMessage clears the message already shown (both the refresh and the seeding path) and an empty mcpToolCall progress message clears the previous one. The adapter test fails without the fix, retaining the stale message.
Worth noting alongside this: didExecuteTool only promotes a truthy progress message to the past-tense label, so an empty message can never become one, and the clear added in finalizeToolInvocation still covers the non-empty case.
AI disclosure: this comment and the related code were written with the assistance of AI.
`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.
|
Thanks for your contribution! |
dd12d29
into
microsoft:main
Keeps a Codex MCP tool's progress narration out of its result, routing it to the client's progress line instead, so the transcript and the model see the tool's real output and not the running commentary.
Defect
mapMcpToolCallProgressappended each progress notification toentry.output, the same field real streamed output uses, and emitted the whole accumulation as tool content. Two consequences followed. When an MCP tool legitimately returned nothing,mapItemCompletedfell back toentry.output, so the concatenated narration became the persisted result content; an interrupted turn did the same for the orphaned completion. And each notification resent the ever-growing concatenation, one telemetry event each. The workbench renders that content as the tool's output embed and forwards it to MCP App webviews, so narration showed as output rather than progress. Claude does not merge progress and Copilot only trace-logs it; Codex was the outlier.Fix
Progress travels on
_metainstead of in the content, since the protocol has no progress action and_metais the sanctioned extension point.mapMcpToolCallProgressno longer touchesentry.output; it coalesces repeats and emitsChatToolCallContentChangedcarrying_meta.progressMessage(a new typed key onIToolCallMeta). The completion path uses only the real MCP output. The workbench readsprogressMessagefor a Running call and calls the tool invocation'sacceptProgress, both on every Running update and when a client attaches mid-turn.One related fix:
finalizeToolInvocationnow clears the progress before the tool is finalized. Without it,didExecuteToolpromotes the last progress string to the past-tense label for any tool whose result carries notoolResultMessage, which the adapter never sets, so this affected every provider, not just Codex. A test pins it.Tests
Mapper: progress is not persisted as the result when the MCP result is empty, is not carried into an orphaned completion, is dropped after completion, coalesces identical messages, and does not clobber start-time meta. Meta reader: a progress message round-trips and a non-string one is dropped. Adapter: progress reaches the executing invocation, a missing or non-string value leaves it unchanged, and transient progress is not promoted to the past-tense message.
The empty-result test fails without the fix. Mapper 59, meta readers 32, adapter 152, all passing. No new type errors, nothing under the synced protocol directory touched.
Part of #333174.
AI disclosure: this comment and the related code were written with the assistance of AI.
Public patches and patcher scripts
Source patch and installed-bundle workaround mapping. The public collection includes the maintained patchers, rollback instructions, regression scripts, and historical snapshots. Build restrictions and exact installer coverage are documented there.