Repository navigation
feat(chat): show pictures sent to pi, and a picture-only prompt as a turn - #1945
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
| Layer / File(s) | Summary |
|---|---|
Shared inline image validation server/codex-images.ts, server/claude-transcript.ts, server/transcript-records.ts |
Shared helpers estimate base64 size and validate supported raster image data. Claude transcript handling delegates image-type checks to these helpers. |
Pi image references and retrieval server/transcript-records.ts, server/conversation.ts, server/transcript-turns.ts, server/pi.test.ts, server/omp-incremental.test.ts, server/pi-large-history.test.ts |
Pi parsing adds prompt-image references and includes valid prompt images in user turns, including image-only turns. Conversation lookup resolves prompt and tool-image refs against active-branch records. Turn-start logic recognizes image-only Pi and command-transcript prompts. Tests cover validation, image retrieval, branch changes, and context clears. |
Browser QA and changelog scripts/pi-image-browser-qa.ts, scripts/ci-browser.sh, scripts/ci-browser.test.ts, changelog.d/pi-chat-images.md |
Browser QA checks prompt-image and tool-image loading, including an image-only prompt. The browser CI lane runs the QA script. The changelog records the image-display and image-handling changes. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Feature
Possibly related PRs
- devswha/ovis#1935: Adds Claude Code image-display handling and shared validation and retrieval patterns used here.
Suggested reviewers: diegolhambi, noobear
Merge Risk: 
Minimal · up to 5293b
-
No outstanding issue identified here prevents merging after normal checks.
Pre-merge checks: 
5
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 10 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
…turn Part of #992, for pi. - A picture in a pi prompt, alone or beside words, is named in the turn's image parts as pi-prompt:<entry id>:<place among its pictures>, and the image endpoint reads it back by that ref. A prompt that is only pictures is drawn as a turn and opens one for paging. - One check, inlineImageType, decides what is a picture for Claude and pi alike, for the page and the endpoint: PNG/JPEG/GIF/WebP by declared type and leading bytes, at most 8 MiB decoded. pi tool pictures now pass it too. - pi pictures are read on the active branch after the last clear: a picture from before a clear is no longer served. - scripts/pi-image-browser-qa.ts runs in the CI browser lane and covers a picture-only prompt. The image endpoint reads a pi picture as the parser draws it: a tool's picture only from the first result that answers a call the page shows (not an orphan result, a second answer, or a later block for the same call; a call made again after an answer with no picture still names its own), a prompt's only from a prompt the page shows, and a clear or an id however JSON writes it.
|
Merge pass: #1937 (f6db8ee) and #1930 (f1fecef) just merged to main, and this PR now conflicts with main in Please rebase onto main and resolve those two files. After that, the merge pass will re-check only the conflict resolution and queue the PR again. |
5293b7c to
284ab4e
Compare
|
Rebased onto main (f6db8ee, #1937) as 284ab4e. The only conflicts were the parser option in On 284ab4e: |
|
Merge pass review-work report. Overall: PASSED. This PR adds a third ref shape to
Probes on the pi path:
Claude after the move:
Checks:
Follow-up: no HTTP contract test covers the Rebase onto f1fecef (284ab4e): #1937 added a
CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it. |
… a Codex tool returns (#1998) Part of #992, the omp and Codex part. It follows #1935 (Claude Code) and #1945 (pi), with the same picture check and the same endpoint rules. ## Change In an omp pane, Chat dropped every picture: a screenshot sent with a prompt, a prompt that was only a picture, and the picture a `read` returned. omp moves a picture's bytes out of the transcript into its blob store and writes `blob:sha256:<digest>` in their place, so there was nothing to decode. In a Codex pane, a picture a tool returned (`view_image`, an image generation, a screenshot a command read) was dropped too: only prompt pictures were read. What it does now: - **omp prompt pictures** are named as pi's are, `pi-prompt:<entry id>:<place>`, and a prompt that is only a picture is a turn of its own and opens one for paging (`turnStarts`). - **omp tool pictures** are named `pi:<call id>:<place>` on the tool's row. omp's call ids hold a `|` (`call_…|fc_…`), which the ref shape now takes. - **omp's blob store:** omp 18.8.7 keeps a session store at `<data>/sessions` and its blobs at `<data>/blobs` (`getBlobsDir`, the same `data` base), so `ompBlobStore` names the store beside the transcript's real session store (the default agent directory, a profile's, `PI_CODING_AGENT_DIR`'s or the XDG one), by layout alone, whether omp has made it yet or not. A blob is read only by omp's own 64-hex digest and only when its bytes hash to that digest. The image endpoint takes the store's identity from the objects that authorized the pane's transcript (`ompAuthorizedTranscript`): the ownership check returns the store's real root and the dev:ino of the transcript file it opened, and the agent data directory is then opened once, without following a link and held to the path named, and `sessions`, the path down to that same transcript file (each component without following a link) and `blobs` are all reached from that one held descriptor. It reads a blob only from that very directory: the store is opened as a directory without following a link, its descriptor must have that identity (`fstat` dev:ino) and be the directory the path names (`/proc/self/fd`), and the blob is opened through that descriptor (`O_NOFOLLOW`). So no directory or link swapped in afterwards, at the store or above it, can stand in for it. A store that is missing, a link, or not a directory answers nothing. **Platform limit:** Node has no descriptor-relative open on macOS or Windows (checked on macOS), and a blob opened by name there follows whatever a directory above it is swapped to for that moment, so there no omp blob is read: blob-backed omp pictures are not shown on those PCs, while omp's inline pictures and every Codex picture are (follow-up #2004). A session in a `--session-dir` of its own names no data directory: its blob pictures stay unnamed, and inline ones still show. - **Codex tool pictures:** the `input_image` data URLs in a `custom_tool_call_output` or `function_call_output` are named `codex-tool:<call id>:<place>` on the tool's row, for any tool. The `ImageView` and image-generation events that repeat them are not read, so each picture shows once. They are served `no-store` like Codex's other refs, so they load only when their row is opened (as #1952's work line requires). - **One check, both sides:** a blob or a data URL is a picture only as PNG, JPEG, GIF or WebP by declared type and leading bytes, at most 8 MiB, and the parser and the endpoint ask the same question (`inlineImageType`, `ompBlobPicture`), so the page never names a picture the endpoint refuses. - **Same history as the page:** the endpoint reads an omp picture after the last clear (also one JSON writes with escapes), a tool's picture only from the first result answering a call the page draws, a prompt's only from a prompt it draws; a Codex tool's picture only from the first output answering a call the page draws, in the rollouts the page reads (`codexHistorySegments`, without the bytes a backtrack left). Ids are matched however JSON writes them. Like Claude's, it searches a window at a time and reads a record's shape a window at a time (`recordShape`, which Claude's reader now shares), holding only the served picture whole; a picture only counted on the way to the served one is checked by a streaming scan that answers exactly as the whole-string check. The omp and Codex readers run in steps a 1 MiB window apart, and every 32 records, and give the event loop its turn between them; the scan for clears runs in steps too, on its own copy of what it keeps. So a lookup never holds the bridge for more than a window's work (Claude's reader and page reads run the same steps straight through, unchanged). **HTTP contract:** `/api/pane/conversation/image` accepts one more ref shape (`codex-tool:`) and answers omp's `pi:`/`pi-prompt:` refs. No new endpoint, no new wire field (`tool.images` and image parts already exist). **Remote bundle:** the server files are in the remote PC bundle. `REMOTE_BUNDLE_VERSION` is not raised here; the release does that (0.4.18 raised it to 37 and its Release run has finished). **Design:** no new UI: the prompt thumbnails, the work line, the row gallery and the viewer that Claude and pi pictures already use. **Not in this PR:** omo and gjc (no recorded sessions with pictures here; they name none, as before), omp's `xd://generate_image` pictures that live only in `details`, and local Markdown images in answers. ## Validation - **Recorded fixtures** from real CLIs on this PC, picture bytes kept as recorded: - `server/fixtures/omp-pictures-18.8.7-session.jsonl` and its blob `omp-pictures-18.8.7-blob.webp`: omp 18.8.7 (`omp -p --model openai-codex/gpt-6-luna:medium`) with `@shot.png` and a question, a `read` of shot.png (call id `call_…|fc_…`), and `@shot.png` alone. omp turned the PNG into a WebP and moved it to its blob store. - `server/fixtures/codex-pictures-0.163.0-rollout.jsonl`: codex-cli 0.163.0, `codex exec -i shot.png` then `codex exec resume` calling `view_image` through the exec tool. Only the records the chat reads are kept (no account ids, instructions, environment or encrypted reasoning). - **Unit tests** (`server/omp-images.test.ts`, new; `server/codex-images.test.ts`), each shown failing with its fix undone (21 mutations, every one fails its test): - the recorded sessions name their pictures by ref only (no base64, no blob ref in the page) and the endpoint serves their exact bytes; - refused, at the parser and the endpoint alike: an svg, non-raster bytes under a raster type, a picture past 8 MiB (exactly 8 MiB passes), a remote URL, a blob ref that is no digest omp writes, a missing blob, bytes that do not hash to their name, a blob that is a link, malformed refs; - a blob that goes or changes while the bridge runs keeps its place: its ref is a 404 and the next picture keeps its own ref; - a tool's picture only from the result or output the page draws: an orphan, a second answer, a call made again after an answer with no picture, a `function_call` output as well as a custom tool's; - a picture from before a clear (also an escaped `context_clear`) is out of the chat and out of reach; - call ids and entry ids written with JSON escapes (omp's `|` among them) are named and served as the parser reads them; - a picture-only omp prompt is its own turn and opens a page there (`turnStarts`); a growing session names the same refs as a cold read; - memory: a call and a result each padded with 20 MiB, two 3 MiB blobs, and a 2 MiB escaped inline picture before the served one: no allocation past a 1 MiB window except the served picture; - the streaming checks answer exactly as `inlineImageType` and the data URL check do, for every two-piece and many three-piece cuts, including a cut inside the padding at exactly 8 MiB. - `server/pi.test.ts`: the omp case of the picture-only `turnStarts` test now expects a turn (omp draws its prompt pictures now). - **HTTP contract** (`server/omp.contract.test.ts`, `server/codex.contract.test.ts`, on an isolated herdr via `bun run check run`): the recorded omp session's three refs answer `image/webp` with the blob's exact bytes and `private, max-age=86400, immutable`; the Codex `codex-tool:` ref answers `image/png` with the viewed PNG's bytes and `no-store`, and 404 once the rollout no longer holds it; unnamed and malformed refs are 404. 8/8 pass. - **Real transcripts on this PC** (shapes only, nothing printed): 36 omp sessions with 652 blob pictures name 650 refs (1 prompt, 649 tool) and the endpoint serves all 650; 7 Codex rollouts name 23 tool-picture refs and the endpoint serves all 23. No page carries base64 or a blob ref. - **Browser** (`scripts/omp-codex-image-browser-qa.ts`, real Chrome, added to the CI browser lane), served by the endpoint's own readers from the recorded files: omp's three prompt pictures decode from the blob store (200x200 WebP), the picture-only prompt is its own turn with no empty bubble, the `read` picture shows on its folded work line, is fetched by its `|` call id and opens the viewer; a Codex tool's picture stays off the folded line with no element requesting it, and decodes (32x32) once its row is opened, beside the prompt's own attachment; at 390 px nothing spills past the screen. Desktop and phone screenshots checked. - **Merge pass gate** (Astra), two rounds, every blocker fixed and reviewed again (see the gate follow-up comments): - On ae860b0: C1, a symlinked blobs directory served another store's picture; C5, the readers blocked the event loop (225 ms on a warm 64 MiB lookup). - On 5c1bc56: B1, an agent directory swapped for a link after the ownership check still moved the read (the store was found again from `realpath`); B2, the macOS/Windows by-name open is racy on an intermediate path; C5, 30,000 short records still held the loop 211 ms, and the scan for clears was synchronous. - On 64c8527: C1a, a directory swapped between the ownership check and the anchor lent another store (the anchor walked the path again); then, raised by Codex, the store's sessions reparented under another agent directory. Fixed by the one-held-descriptor anchor above; tests swap the agent directory, move another real sessions in, and reparent, through a hook right after the data directory's open. On a947a19, round 4: moves before the data directory's open adopt another data directory. Decided trust boundary (a): write access to the user's own omp data directory; the directory at that path when the anchor opens it is trusted, and the transcript and blob store always come from that one held descriptor (tests before and after the open, 1ef6bf8). - Fixes, as described above: the descriptor-relative open held to the identity taken at authorization; no blob read off Linux; yields every 32 records and per clear-scan window. Tests: through a test seam that acts at each open, a link or another real store swapped in for the store, or for the agent directory above it, at the store's or the blob's open; the agent directory swapped after authorization; the non-Linux branch forced; the longest stretch between loop turns bounded in buffers taken (no clocks) over 30,000 records and 40 MiB, cold. Each defence is shown failing its test when removed. - Measured: 30,000 short records over 30.5 MB, longest event-loop stall 3.9 ms warm (omp), 33.5 ms cold (one 16 MiB window of the clear scan), Codex 2.1 ms; a warm 64 MiB lookup 2.1 ms. - On the way, the code reviewer found the store named null before omp made it, so a live page missed pictures a cold read names (fixed: named by layout), and Codex found a link, then another real directory, swapped in at the open, and the anchor taken only after the async resolve (all fixed as above). Both approve the final commits. One gap left as a known one: the scan for clears working on its own copy has no test of its own (without it, a page read in the middle of an image read redoes the scan). - **Reviews:** - `omo-native-code-reviewer`: APPROVE on 4a9a574 (then 7005452d, before the rebase), with one MEDIUM (a blob deleted while the bridge runs could move the next picture into its ref) and four LOWs (refusals re-read every poll, the blob cache surviving a reset, weak spots in the browser QA, earlier inline pictures read whole to count them). - Codex (Sol 6.1): REQUEST CHANGES on 4a9a574 (then 7005452d, before the rebase): the same MEDIUM, a MEDIUM that a picture only counted on the way to the served one was read whole (a blob hashed whole to name it, a data URL rebuilt to count it), and a LOW that the browser QA could pass on an undecoded picture. - All fixed in ae860b0 (fcc0187a before the rebase) with tests: the place model, a per-file blob cache that keeps refusals, streaming checks, the QA's decode and error checks. Both reviewed the delta again: APPROVE, every finding FIXED, no new finding from Codex. The code reviewer's one new LOW is left as is: the served inline picture is read from the file twice (once checked, once held), up to about 11 MB of extra reading and no extra memory; holding it during the check would mean holding a picture before it passes, which the memory rule forbids for any picture not served. - `bun run check fast` on 1ef6bf8: workflow syntax, generated types and themes, typecheck and build pass, and 7265 of 7266 unit tests pass. The one failure is the known slot-admission flake in `scripts/check.test.ts` (`slots > gives its place up when it stops waiting`: ticket ports taken on a busy host), in a file this PR does not touch, which passes 66/0 alone. The run before it had only an untouched load-timing failure (`scripts/install-node-fallback.test.ts`, hung-host kill, 10/0 alone). Counted as passed per the lane's rule for known slot-admission flakes; GitHub CI runs the full suite. --------- Co-authored-by: mp <mp@local>
## Change Part of #992 (pictures in Chat). Chat showed transcript pictures for Claude, Codex, omp, pi and OpenCode, but not for GitHub Copilot CLI, though Copilot keeps them in its own session file. Copilot CLI 1.0.105 interns each picture once, as a root `session.binary_asset` record (base64, `assetId: "sha256:<digest of the bytes>"`). A prompt's `attachments[].assetId` and a tool's `result.binaryResultsForLlm[].assetId` name it by that id (checked against the SDK's `session-events.d.ts` and a real recording). - **Parser** (`server/copilot.ts`): a prompt's attached pictures become image parts and a tool's become its `images`, each named `copilot:<digest>`. Only refs reach the page. A picture-only prompt opens a turn, and `transcript-turns.ts` agrees for paging. - **Endpoint** (`GET /api/pane/conversation/image`, no new route): reads only the pane's own pinned Copilot session file (`openCopilotFile`). It serves a root record only (no sub-agent), a raster within `MAX_IMAGE_BYTES` (`inlineImageType` + magic bytes), and only bytes whose sha256 equals the ref. It reads in 1 MiB windows and skips any line longer than the largest picture record without holding it. Served immutable because the ref is the content digest; `workImages` keeps `copilot:` refs like Claude's and pi's. Design: same image part, ref route and viewer as the pi/Claude pictures (#1935, #1945); no new UI. ## Validation - Real fixture: `server/fixtures/copilot-pictures-1.0.105-events.jsonl`, recorded on this PC with `copilot -p ... --attachment red.png` plus the `view` tool reading `blue.png` (redacted; reasoning blobs and usage records dropped). - Failing first: the new `server/copilot.test.ts` case failed on the parser (`[]` instead of the image part), then passed. - `bun run check fast`: all steps ok, unit **9828 pass, 0 fail, 41 skip** (on 946a6e6). - `bun scripts/copilot-image-browser-qa.ts` (added to the CI browser lane): desktop + phone, both pictures decoded at 48x48. - `server/copilot.contract.test.ts` (new end-to-end case: pane-scoped 404, cache header) could not run locally. Its `beforeAll` fails with "isolated Copilot stand-in not detected" on unchanged `origin/main` too (herdr 0.9.5 here); CI runs it. ## Reviews - omo-native-code-reviewer: **PASS / APPROVE**. Escape from the pinned file, sub-agent records, SVG, symlink and fd swap were all probed and refused. MEDIUM: whole-line memory in the lookup, fixed in 946a6e6. - Codex Terra 6.1 fast xhigh, round 1: two MEDIUMs. (1) Unbounded line memory: fixed, RSS for a 64 MiB junk line went from 173 MB to 33 MB, and a test covers it. (2) A 16 MiB page window splits a turn holding big tool pictures: this reproduces identically on `origin/main`, so it is filed as #2478. Delta round: MEDIUM, a final record without a newline was dropped. Fixed, with a test.
## Change Part of #992 (pictures in Chat), after #2479 (Copilot). Grok CLI 1.0.67 logs a session's pictures inline in its own `updates.jsonl`: - a prompt's `user_message_chunk` with `content: { type: "image", data, mimeType }`, and - a tool update's `content: [{ type: "content", content: { type: "image", ... } }]` (here `read_file` reading a PNG). What changes: - **Parser** (`server/grok.ts`): prompt pictures become image parts and tool pictures become the tool's `images`, each named `grok-image:<sha256 of the bytes>`. Hidden prompt chunks are skipped, ACP's replace-the-whole-`content` rule is honoured, and only refs reach the page. - **Endpoint** (`GET /api/pane/conversation/image`, no new route): reads only the pane's own Grok update log (`resolveGrokStore`), over the history `grokProjection` keeps active. A prompt that was rewound away serves nothing, including one rewound while the lookup reads. - It checks that the file it opened is still the one the path names. - It reads 1 MiB windows with the event loop running between them. - It serves a raster within `MAX_IMAGE_BYTES` whose sha256 matches the ref, so the response is immutable. - `workImages` keeps `grok-image:` refs, like Claude's, pi's and Copilot's. Design: same image part, ref route and viewer as the other agents' pictures (#1935, #1945, #2479); no new UI. ## Validation - Real fixture: `server/fixtures/grok/pictures-1.0.67.jsonl`, recorded on this PC: `grok --prompt-json` with a red PNG, then `read_file ./blue.png`, through the owner's CLIProxy (`gpt-5.6-luna`, isolated `GROK_HOME`). Paths are redacted. - Failing first: the new `server/grok.test.ts` case failed on the parser (`[]` instead of the image part), then passed. Added cases cover a rewind before and during a lookup, a file swapped into the path during a lookup (mutation-checked: fails without the identity pin), an escaped `"im\u0061ge"`, and a missing digest. - `bun run check fast` on 3a9858c: all steps ok, unit **9828 pass, 0 fail, 41 skip**. - `bun scripts/grok-image-browser-qa.ts` (added to the CI browser lane): desktop + phone, both pictures decode at 48x48. - Lookup cost: a 6.6 MB text-only log with a missing digest takes 50 ms, and a 10 ms timer keeps firing. The first version took 4 s or more on 2.4-4.8 MB logs. ## Reviews - Codex Terra 6.1 fast xhigh: - Round 1: HIGH (quadratic read with no yield), MEDIUM (a prompt rewound during the lookup was still served), LOW (escaped type). All fixed in 3a9858c. - Delta round: MEDIUM (a file swapped into the path during the lookup). Fixed, with a test. MEDIUM (the final revalidation may catch up up to 32 MiB of new records synchronously): this is the same `SCAN_BYTES` bound every Grok page poll already pays, so it is not new here. - omo-native-code-reviewer: REQUEST_CHANGES on the same HIGH, now fixed. L2 (open, then check the path still names it) is fixed too. - L1 (a picture a later update replaced is still served from the active history) is left as is: it is the pane's own history, so nothing leaks. - L4: the parser hashes each picture on every parse; not measured.
Part of #992, the pi part. It follows #1935 (Claude Code), and uses the same check and the same endpoint rules.
Change
In a pi pane, Chat dropped the pictures sent to pi: a screenshot pasted into a prompt was in the session file and never shown, and a prompt that was only a picture opened no turn, so it vanished. Tool pictures were shown, but checked only by declared type.
What it does now:
pi-prompt:<entry id>:<place among its pictures>, andGET /api/pane/conversation/imagereads it back by that ref. Only refs reach the page, never base64; the bytes are fetched when the prompt is shown.turnStarts), as a prompt with words does.inlineImageType(moved next to the Codex image code) decides what is a picture for Claude and pi alike, for the parser and the endpoint: PNG, JPEG, GIF or WebP by declared type and leading bytes, at most 8 MiB decoded. pi tool pictures now pass it too, so a page never names a picture the endpoint refuses./treebranch after the last clear, and as the parser draws it: a tool's picture only from the first result that answers a call the page shows (not an orphan result, a second answer, or a later block for the same call), a prompt's only from a prompt the page shows. A clear or an id is matched however JSON writes it. Records are read one at a time (piBranchRecords, per-record cap), never the whole transcript.scripts/pi-image-browser-qa.tsnow runs in the CI browser lane and covers a picture-only prompt.HTTP contract:
/api/pane/conversation/imageaccepts one more ref shape (pi-prompt:). No new endpoint, no new wire field.Design: no new UI: the prompt thumbnails and the viewer pasted pictures already use (#951).
Validation
server/pi.test.ts), each failing on the code before its fix:/treeor a clear left behind is out of the chat and out of reach;context_clearand an escaped prompt id, read as the parser reads them.server/pi-large-history.test.ts: its tool-picture fixture now includes the call its result answers (it relied on the endpoint serving an orphan result's picture, which the page never names); fake image bytes inserver/omp-incremental.test.tsare real raster headers now.scripts/pi-image-browser-qa.ts, real Chrome): the picture-only prompt is its own turn with its thumbnail decoded from the served bytes and no empty bubble; the tool row fetches nothing while folded and decodes both pictures once opened; a result whose pictures vanish leaves no stranded/api/pane/commandsas its siblings do.bun run check fast: passes on this head (workflow syntax, generated types and themes, typecheck, build, 6797 unit tests).