Repository navigation
fix(session): keep images returned by tools in Claude Code and Codex sessions - #1254
Conversation
|
No findings in the scoped PR diff. The PR description documents sufficient testing, including representative real-CLI migrations in both directions with PNG-preservation results. This satisfies the end-to-end testing requirement. Review was read-only; I did not run, build, or install anything from the PR. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
The PR keeps images that a tool returns when reading Claude Code (tool_result with an image part) and Codex (function_call_output with input_image items). Both readers move them into a separate user message after the round's tool results, so fidelity counts them and image-capable targets write them back. The earlier bot review reported no findings; nothing else is open.
Local checks, on the merge of cad23673 into origin/main (0da2be76, clean merge):
npx tsc --noEmit: pass (also on the head alone).npm run lint: exit 0.npx vitest run claude-tool-result-images session-cmd migrate fidelity-sweep agent-session-transcript session-search cursor-user-images scrub-session session-sync tclaude-tcodex save-session: 14 files, 235 tests passed.- Red/green: the PR's two test files on
origin/maingive 4 failed (all 4 new cases); on the merge they pass. npm run build, thennpm run test:e2e -- src/__tests__/e2e/session-migrate-opencode.test.ts: 1 passed.- Real CLI,
teamai session migrateunder a throwaway HOME with a 1x1 PNG returned by the tool:This matches the PR description. The written Claude Code session hasclaude-code -> codex main: 100.0%, PNG in rollout: 0 merge: 100.0%, PNG in rollout: 1 codex -> claude-code main: 92.5%, PNG in session: 0 merge: 94.0%, PNG in session: 1tool_result, then a user record with the image, then the assistant reply. The Codex rollout hasfunction_call_output, then a userinput_imagemessage.
I also checked the readers of the new image-only user message: fidelity (migrate.ts), the Claude Code, Codex and CodeBuddy IDE writers, scrub, the IR serializer, and the transcript/search index. None of them breaks. The extra user turn on targets that split turns at user messages is the trade-off described in the PR notes.
Findings
- [P3 nit]
src/session-flow/adapters/codex.ts:533flushes the held images on anyresponse_itemthat is not a tool output, including types the reader ignores (for exampleweb_search_call). If such an item falls between two outputs of one round, the next output is appended to the image message (codex.ts:584). A local probe (call c1, call c2, output c1 with an image,web_search_call, output c2) givesuser[tool_result],user[image, tool_result]. That is the mixed shape the PR avoids for codebuddy/workbuddy. Codex is not known to write a rollout like this, so this is optional. Flushing only on the types that start a new message (message,function_call,custom_tool_call,reasoning) would close it.
Not covered
- Cursor, OpenCode, CodeBuddy, WorkBuddy and CodeBuddy IDE as targets: their clients are not installed here (
claude-code -> codebuddystops with "not installed" on main and on the PR). The unit tests cover them. - I did not resume a migrated session in the real Claude Code or Codex.
- I did not run the full vitest suite or the full e2e suite.
Verdict: Approve.
|
This merged before the nit fix landed, so it is in #1260: held images are flushed only on |
Summary
Images returned by a tool are dropped when a session is read, and fidelity doesn't count them as lost:
imageblock; the reader only keeps thetextparts.view_imageand MCP tools writefunction_call_output.outputas content items; since fix(session): keep the text of a Codex tool output given as a list #1202 the reader keepsinput_textand dropsinput_image.Both readers now put those images in a user message of their own right after the round's tool results. Targets with image support write them, the others show a placeholder like for any other image.
They get their own message because codebuddy, workbuddy and the CodeBuddy IDE history write a user message's text before its
function_call_resultlines, and Cursor starts a new turn for user content. With the image in the tool-result message, those targets put a user line between a tool call and its result. Claude Code writes each parallel tool_result as a separate record, so the reader holds the images until the last result of the round.Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest run: 11258 passed; the same 5 tests fail on main without this change (cursor-bubble-range, cursor-user-images x2, submodule-checkout x2)claude-tool-result-images.test.ts(read, a parallel round with two images, write back to claude-code) and a Codex case insession-cmd.test.ts; all fail on mainteamai session migratewithHOMEin a scratch dir and a 1x1 PNG returned by the tool:Related Issues
None.
Notes for Reviewers
The IR tool_result holds only text, so the image moves out of it. A claude-code to claude-code round trip puts the image in a user record after the tool_result instead of inside it. Targets without images (Cursor, OpenCode, CodeBuddy) show that message as a user turn with the placeholder, after the tool round.