fix(codex): restore semantic MCP cards from code-mode history - #702
Merged
xintaofei merged 15 commits intoSep 10, 2026
Merged
Conversation
A code-mode script whose MCP call was REFUSED still finishes: codeg-mcp answers `isError: true` and the wrapper prints `Script completed` (measured on a real rollout — a `delegate_to_agent` turned down for `depth_limit`). Hardcoding `status: "completed"` on the recovered card therefore contradicted the `is_error: true` result block written right beside it, and `ContentBlock::ToolUse::status` is documented as a claim a reader may act on. Read the outcome off the item instead. Adds the failed-item regression, plus the guard case that keeps every correlation honest: a script mixing an MCP call with a shell call publishes only one semantic item, so the items cannot be zipped onto the call sites and the script's own reading has to stand.
Two ways the recovered card could misreport a call it has the facts for. `infer_tool_call_output_is_error` reads an outcome out of the output TEXT because a script card carries no error field. An app-server `McpToolCall` does — `result.isError`, plus the item's own terminal `status` — so running the heuristic on top of it could only invent failures: a tool that wraps a command and prints `exit code: 1`, or answers with a line opening `Error:`, returned perfectly well. Explicit fields now decide; the heuristic stays as the fallback for a record that states neither. The semantic path also discards the wrapper's printed output, so a result carrying no text of its own — an image-only `content` array, or a transport failure that answered with `error` and no result — left a completed call rendering as an empty card where the script card used to show the run. Fall through to the error string and then to the serialized result, which is what `pi::content_to_text` already does with this same MCP shape. Projecting MCP image blocks onto `ToolResult::images` would be better still, and wants the raw-JSON extractor the typed ACP path has and the parsers do not.
…le once
Three refinements from a second review pass.
The serialize fallback took the whole `CallToolResult` — protocol fields,
`_meta` and any base64 blob — with no bound, and rendered `{"content":[],
"isError":false}` for a call that simply returned nothing. Serialize the
`content` alone, only when it carries blocks, capped like `pi` caps its own
fallback on the same shape. A call that answered with nothing still says
nothing.
A record can also state failure through `error` alone. That was reachable
only through the text heuristic, which a stated success now suppresses, so a
transport failure sitting beside `status: "completed"` would have rendered
green. Read it as the stated failure it is. The rule for when an `error`
field STATES an error rather than merely existing — `null`, `false` and a
blank string are how a record says "no error" — was written out inline twice
already; it is `is_stated_error` now, verbatim, and both sites use it.
Tests: the outcome precedence is pinned directly against the fields, so the
disagreements (`isError` against `status`, either against the output text)
are covered rather than implied by four agreeing rollouts; and the mixed
shell/MCP guard compares the results as well as the calls.
`is_error` read as the boolean outcome sitting three lines below it; the local holds the record's `error` VALUE, which is only one of the signals that decide that boolean.
`ContentBlock::ToolUse::status` claimed grok was the only parser that could honestly supply a status, and that codex deliberately never does. The semantic MCP path falsifies half of that: a card rebuilt from an `item_completed.McpToolCall` carries that record's own terminal outcome. The warning it was really making still stands and is kept — what must never be copied onto an inner call is the SCRIPT's `ScriptStatus`, which is script-level and would manufacture a permanent spinner. Say that instead of naming codex as a whole. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A script that threw keeps its own card even when its one MCP call did publish a semantic item, because the wrapper's `Script error:` text is the whole story of that turn and the semantic path discards it. Nothing held that gate down. The count gate cannot stand in for it: a script can throw after its last call already answered, leaving exactly as many items as call sites, so deleting the status check would have passed every other test in the file while silently swallowing the JS error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two fallbacks that serialize a result — the structured twin, and the `content` array for a call that answered in blocks with no text — called `serde_json::to_string` on the whole value first. For an image-only result that is the entire base64 blob: built in full, then scanned twice by `truncate_str`, to keep four thousand characters of it. Every `McpToolCall` record pays it at capture time, before anything knows whether a script will even accept the correlation. `serialize_preview` writes into a sink that refuses bytes past a budget, which stops the serializer instead of letting it run to the end of its input. UTF-8 spends at most 4 bytes per character, so 4x the character cap always covers the cap, and the partial character a byte cut leaves behind always falls outside the truncation — the output is identical for every input, which is what the equivalence test pins across a value that fits, one landing exactly on the cap, one far past it, and one whose characters are multi-byte. Measured: parsing all 3394 real rollouts on this machine costs the same before and after the whole stack (190.63s vs 190.65s, inside a 0.07% run spread) — but that corpus tops out at a 5.3 KB result, so it cannot exercise this at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When `status`, `result.isError` and `error` are all absent, the outcome is inferred from the result text rather than stated by the record — the precedence test calls that case "a record that states nothing". Say "yields" so the sentence covers every supported case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Budgeting `structuredContent` was wrong. This preview is read TWICE:
the card shows it, and `infer_output_text_is_error` re-parses a preview
opening with `{` and searches the result for a failed `status`. Cut the
JSON and that parse fails silently — so a record stating no `status`, no
`isError` and no `error`, whose structured answer says
`{…,"status":"failed"}`, settled GREEN. The previous commit introduced
that; this restores the whole serialization and pins it with a test that
fails the moment a cap goes back on.
The budget stays where it belongs: the last fallback, the `content`
array that can hold a base64 blob. That branch was already truncated
before it was made cheap, so nothing about what it shows changed — and
the comment now says which branch is truncated and why the others are
not, instead of claiming all of them are.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The block fallback is the one preview that is cut, and its outcome was still being read back out of the cut string. Same failure as the structured branch: `infer_output_text_is_error` re-parses a preview opening with `[`, a truncated document does not parse, and a result whose blocks reported a failure settled GREEN. Capping the base64 a card displays is right; letting that cap decide the call is not. So the fallback now hands back the blocks it cut, and the outcome is inferred from those. That is exactly what parsing an untruncated preview would have produced — the same `infer_output_value_is_error` over the same value — so the cap costs the card characters and never costs the call its verdict. Verified by removing the arm and watching the new test fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit read a cut result's outcome off the blocks instead of the cut string — right, but through a full walk, and `infer_output_value_is_error` follows `data`. In a tool-output envelope `data` is a nested result; on an MCP content block it is the PAYLOAD. So an outcome-less image result walked straight into the base64 the cap was added to avoid touching, and `infer_output_text_is_error` lowercased it into a second copy of itself — the bounded preview undone by the inference standing next to it. `blocks_report_failure` reads each block's OWN keys and refuses every descent. That is all a block can honestly report: a marker is a key on the block, a payload is bytes. Pinned by a test with a payload past the cap that reads like an error, which fails against the full walk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three claims around the semantic preview promised more than the code delivers. `serialize_preview` said "not one byte further", but `serde_json` walks a string looking for escapes before offering any of it to the writer, so one oversized string is still read through once. `blocks_report_failure` said the cap "avoids touching" the payload when what it avoids is COPYING it, and said "every own key" when it reads the seven outcome fields the walker recognizes. Its depth-4 stop is also not free in every shape — a recognized field can itself be megabytes, and a `trim` on one costs a pass. What is bounded is MEMORY, which is the part that can fail; a pass over bytes already resident is not a second copy of them, and stopping even that would mean replacing the serializer. Say that, rather than leaving a reader to discover it the way this review did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two sizing claims on `serialize_preview` were loose. "Without ever building the whole serialization" reads as if a small value were also streamed, when a value that fits is written out whole — into a buffer that just cannot grow. And the budget was written `4 * max_chars` when it is `4 * max_chars + 1`, which is not a rounding detail: the `+ 1` is what makes a full buffer decode to STRICTLY more than `max_chars` characters, and strictness is the entire equivalence proof. At `4 * max_chars` the property would still hold for JSON, because JSON always opens with an ASCII byte and so cannot land a full buffer on exactly `max_chars` — but that is a fact about the format, not about this function, and nothing here says so. The `+ 1` makes it arithmetic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`max_chars = 0` is the one case where a hypothetical `4 * max_chars` budget would be an EMPTY buffer, which decodes to exactly zero characters without ever reaching the opening ASCII byte the argument leans on. The parenthetical said "true" flatly. It is true for every positive `max_chars`, which is what it now says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Owner
|
codeg work task |
xintaofei
added a commit
that referenced
this pull request
Sep 11, 2026
pi gets a deep pass — reasoning, images, real diffs and rewound branches now come through correctly, and a new session no longer opens with a page of startup noise. Local screenshots render inline in replies, imported worktrees land under their repo, and six bundled agents move up. ## New - **Local images in a reply now render inline** instead of a blocked placeholder, and click to open in the image viewer. (#691, @Adam-Dalloul) - **pi is now a local MCP source** — servers in `~/.pi/agent/mcp.json` show up in MCP settings and can be edited there. (#653 requested by @galact-byte, #673 @dawNotPoi) ## Improved - **Updated bundled agents:** DeepSeek 0.9.0, Kimi Code 0.42.0, OpenCode 1.18.30, CodeBuddy 2.149.0, Grok 1.0.25, Qoder 1.1.49. - **DeepSeek's model panel follows 0.9.0** — the built-in list is now `deepseek-flash` and `deepseek-v4-pro`, image detail is replaced by a pixel budget, and a list written by an older codeg says so and is repaired by saving it again. - **pi's settings panel now warns that a custom config or session directory only applies to pi itself** — sessions written there won't appear in codeg's history. ## Fixed - **pi's reasoning is no longer dropped** — thinking blocks were read from the wrong field and vanished entirely. - **A rewound pi session now shows only the branch you're on**, not the abandoned ones still sitting in the same file. - **Opening a pi session no longer starts with pi's startup banner** as the assistant's first message. - **More of a pi transcript survives:** images, an `edit`'s real diff, cancelled commands marked failed, an error message on a failed turn, and context-compaction cards. - **A Codex code-mode MCP call keeps its identity when you reopen the session** — real tool name, arguments, result and error state, instead of a generic script card. (#702, @asteroida123) - **An imported worktree session now lands under its repo**, so it joins the repo's worktree group and gets its branch label. (#552 reported by @Luis-WowApps, #707 @Adam-Dalloul) - **An agent process is no longer left running** when a connection ends at the wrong moment. (#701, @asteroida123) Thanks to @Adam-Dalloul, @asteroida123 and @dawNotPoi for contributing to this release, and to @Luis-WowApps and @galact-byte for the reports. ----------------------------- # 发布版本 0.30.7 这一版对 pi 做了一轮深度适配——推理内容、图片、真实 diff、回退后的分支都能正确还原，新开会话也不会再先甩出一大段启动信息。 回复里的本地截图现在直接内联显示，导入的工作树会话归到所属仓库下面，另有六个内置智能体版本更新。 ## 新增 - **回复里的本地图片现在直接内联显示**，不再是一行占位文字，点击即可在图片查看器中打开。（#691，@Adam-Dalloul） - **pi 现在也是本地 MCP 来源之一**——`~/.pi/agent/mcp.json` 里的服务器会出现在 MCP 设置中，并可直接编辑。（#653 由 @galact-byte 提出，#673，@dawNotPoi） ## 改进 - **内置智能体版本更新：** DeepSeek 0.9.0、Kimi Code 0.42.0、OpenCode 1.18.30、CodeBuddy 2.149.0、Grok 1.0.25、Qoder 1.1.49。 - **DeepSeek 模型面板跟进 0.9.0**——内置列表改为 `deepseek-flash` 与 `deepseek-v4-pro`，「图片细节」换成像素预算，旧版本 codeg 写下的列表会明确提示，重新保存一次即可修复。 - **pi 设置面板会提示自定义配置目录/会话目录只对 pi 本身生效**，写到那里的会话不会出现在 codeg 的历史里。 ## 修复 - **pi 的推理内容不再丢失**——思考块此前读错字段，导致全部推理凭空消失。 - **回退过的 pi 会话只显示你当前所在的分支**，不再混进同一文件里被放弃的分支。 - **打开 pi 会话不会再以 pi 的启动横幅作为助手的第一条消息。** - **pi 会话记录还原得更完整：** 图片、`edit` 的真实 diff、被取消的命令标记为失败、失败轮次显示错误信息，以及上下文压缩卡片。 - **Codex code mode 里的 MCP 调用重开会话后仍保有身份**——工具名、参数、结果和错误状态都在，不再退化成一张通用脚本卡片。（#702，@asteroida123） - **导入的工作树会话现在归到所属仓库下面**，会并入仓库的工作树分组并拿到分支标签。（#552 由 @Luis-WowApps 反馈，#707，@Adam-Dalloul） - **连接在特定时机结束时不会再留下没退出的智能体进程。**（#701，@asteroida123） 感谢 @Adam-Dalloul、@asteroida123、@dawNotPoi 为本次发布做出的贡献，也感谢 @Luis-WowApps 和 @galact-byte 的反馈。
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
Codex code mode can execute multiple MCP calls behind one outer exec wrapper and print their results as one JSON chunk. The static script parser cannot always evaluate variable-based arguments, so history reload falls back to a generic script card and loses the real MCP call identity.
This uses the app-server semantic completion records as the authoritative source while retaining conservative fallbacks whenever correlation is ambiguous.
Extracted from #693 so the parser fix can land independently.
Verification