refactor: move tool-result metadata into a structured note side channel#1437
Conversation
…and web UIs Tool results carry <system> blocks as side-channel notes for the model (ReadMediaFile summaries, Read status, MCP image captions, error/empty sentinels). Keep them in history for the model, but strip them at every core-to-UI boundary so they no longer render as plain text. vis is intentionally left untouched to preserve the model's-eye view for debugging.
…<system> tags Unwrap the tool error/empty sentinels (<system>ERROR: ...</system>, <system>Tool output is empty.</system>) instead of deleting them: keep the human-readable text and drop only the tags. Otherwise a failed or empty tool result rendered as a blank output, indistinguishable from a rendering bug. The model still reads the wrapped form in history.
Tool-produced model-facing metadata (ReadMediaFile summaries, Read status lines, MCP image-compression captions) was baked into tool output as <system> text, so every UI had to strip it back out and three copies of the model-view normalization had silently drifted apart. - ExecutableToolResult gains `note`: content rendered to the model but never to UIs; records and history now store the raw output plus the structured isError/note fields - the model view is rendered exactly once at the LLM projection boundary by renderToolResultForModel; the transcript and vis hand-copies are deleted (vis now calls the same function for its model view, fixing their drifted empty-output checks) - ReadMediaFile / Read / MCP captions write `note`; tool outputs stay pure data, and text-only results keep a single text part (note joined with a newline) so provider tool content stays a plain string - all UI-side <system> stripping is removed; failed tools show their own error text with the structured isError flag - wire protocol 1.4 -> 1.5 migrates existing records' tool-produced <system> blocks into `note` on resume
🦋 Changeset detectedLatest commit: ca5f65f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cb44dba41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- "The attached image was downsampled" replaces directional wording that depended on provider serialization order (inline media vs flatten-and- re-attach) - drop the 1.4 -> 1.5 wire migration: legacy records replay verbatim, so the model view of old sessions stays byte-identical to what the model originally saw and UIs show the legacy <system> text as-is; this also removes the risk of the migration misclassifying user data that quotes tool metadata, and the additive note field needs no version bump - pass optional result fields as undefined instead of conditional spreads (repo convention)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49eeb40238
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…system-tag guard - normalizeToolResult now keeps a note only when it is a non-empty string: tools and finalize hooks are arbitrary JS, and a malformed note (null, number, object) would previously persist into the record and crash every subsequent LLM projection of the session. Everything downstream now trusts note to be string | undefined. - the TUI tool body suppression matches the full <system-reminder> tag instead of any <system prefix: reminder piggy-backing stays hidden, while real output that merely starts with a literal <system> tag (file contents, MCP text) stays visible, covered through the real ToolCallComponent path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8173a1ccb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…em from text
compressImageContentParts now returns { parts, captions } — captions come
back from the compressor as structured data and are never inserted into
the parts, so the MCP pipeline no longer pattern-matches text to move
them into the note side channel. Tool output that merely quotes a
caption (a doc, a log, a test fixture) stays verbatim in the output.
Also corrects the stale claim that prompt ingestion uses this helper
(it compresses per image while constructing the part).
…sedContentParts The package-root comment still described compressImageContentParts as the input-stage helper every ingestion site calls; prompt ingestion compresses per image with compressBase64ForModel / compressImageForModel, and the MCP pipeline is the walker's only caller. Also export the walker's CompressedContentParts return type so public-API consumers can name it.
…rness verdicts from tool output The error/empty status text is model-only after the note refactor (UIs render the raw output and style failures via the structured isError flag), so the earlier plain-text wording served no remaining audience. Wrapping the statuses in <system> gives every piece of system-generated text inside a tool result the same marker: - failed calls get '<system>ERROR: Tool execution failed.</system>' unconditionally — the ERROR:-prefix guard is removed, so the harness verdict can no longer be confused with tool output that happens to start with error-like text - empty outputs render as '<system>Tool output is empty.</system>'; the plain placeholder the loop layer bakes into records is still recognized and upgraded at projection time
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ec78e71cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca5f65fbf0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| output: input.renderedLines.join('\n'), | ||
| note: `<system>${this.finishMessage(input)}</system>`, |
There was a problem hiding this comment.
When Read returns zero rendered lines (empty file, EOF range, or empty tail), this now emits output: '' with the status in note, but the loop normalizer still rewrites empty string outputs to the literal Tool output is empty. before recording/sending events. That means users see a generic placeholder and the model receives both the placeholder and the note, even though the note already makes the projected tool message non-empty; handle non-empty note as sufficient content or avoid returning an empty raw output here.
Useful? React with 👍 / 👎.
1 participant
Summary
Tool-produced model-facing metadata (ReadMediaFile summaries, Read status lines, MCP image-compression captions) was baked into tool output as
<system>text. That in-band scheme leaked raw markup into every UI, forced three separately-maintained stripping/normalization copies (live context, transcript, vis) that had silently drifted apart, and made it impossible to distinguish tool metadata from user data that merely contains the literal tag.This PR separates facts from views:
ExecutableToolResultgainsnote: content routed to the model but never to UIs. Formatting is the producing tool's choice; the channel itself adds nothing.outputplus structuredisError/note.renderToolResultForModel: plain-textERROR:prefix for failures, empty-output placeholder, note appended verbatim. The transcript copy is deleted and vis's model view now calls the same function, fixing their drifted empty-output checks.ReadMediaFile/Read/ MCP captions writenote; tool outputs stay pure data. Text-only results keep a single text part (note joined with a newline) so provider tool content stays a plain string across OpenAI-compatible backends.<system>stripping is removed (stripSystemTagsand call sites deleted). Failed tools show their own error text with the structuredisErrorflag. User data containing a literal<system>tag is no longer eaten from display.Legacy records
Records written before this change keep their inline
<system>text and replay verbatim — no wire migration, no version bump (thenotefield is additive in both directions). The model view of old sessions stays byte-identical to what the model originally saw; UIs show the legacy text as-is. This deliberately avoids a migration that would have to classify arbitrary historical text as tool metadata vs. user data quoting it.Behavior changes
isError, without the model-facing scaffold line. Sessions recorded before this change keep showing the old inline text.Testing
test/cli/update/preflight.test.tsrollout gating are pre-existing on a pristine tree and unrelated)