Skip to content

feat(chat): show pictures sent to pi, and a picture-only prompt as a turn - #1945

Merged
devswha merged 1 commit into
mainfrom
feat/chat-pi-images
Oct 25, 2026
Merged

devswha merged 1 commit into
mainfrom
feat/chat-pi-images

Conversation

@devswha

@devswha devswha commented Oct 25, 2026

Copy link
Copy Markdown
Owner

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:

  • Prompt pictures: 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 GET /api/pane/conversation/image reads it back by that ref. Only refs reach the page, never base64; the bytes are fetched when the prompt is shown.
  • Picture-only prompts are drawn as a turn and open one for paging (turnStarts), as a prompt with words does.
  • One check: 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.
  • Same history as the page: the endpoint reads a pi picture on the active /tree branch 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.ts now runs in the CI browser lane and covers a picture-only prompt.

HTTP contract: /api/pane/conversation/image accepts 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

  • Unit tests (server/pi.test.ts), each failing on the code before its fix:
    • prompt pictures named by entry and place, decoded on request; an svg or non-raster bytes take no place; refs that name nothing are 404;
    • a picture-only prompt opens a page's turn;
    • a picture a /tree or a clear left behind is out of the chat and out of reach;
    • a tool's picture only from the result the page draws: a call answered twice, an answer with no picture before a duplicate, two result blocks in one entry, a result answering no call, a call made again after an answer with no picture;
    • an escaped context_clear and 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 in server/omp-incremental.test.ts are real raster headers now.
  • Browser (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 . Its stub server now answers /api/pane/commands as its siblings do.
  • Reviews: Codex (Sol 6.1) reviewed the change three times: three MEDIUMs (escaped clear, a picture from a result the page ignores, escaped prompt id), then one (a call made again after an answer with no picture). All fixed with tests. The last review approved with no findings. One known limit it noted: if a pi session reused a call id and both results carried a picture, the page would name the same ref twice and the endpoint serves the first; pi writes unique call ids.
  • bun run check fast: passes on this head (workflow syntax, generated types and themes, typecheck, build, 6797 unit tests).

@coderabbitai

coderabbitai Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between d6fd1d8 and 5293b7c.

Files selected for processing: 12
  • changelog.d/pi-chat-images.md
  • scripts/ci-browser.sh
  • scripts/ci-browser.test.ts
  • scripts/pi-image-browser-qa.ts
  • server/claude-transcript.ts
  • server/codex-images.ts
  • server/conversation.ts
  • server/omp-incremental.test.ts
  • server/pi-large-history.test.ts
  • server/pi.test.ts
  • server/transcript-records.ts
  • server/transcript-turns.ts
Files with no reviewable changes: 1
  • scripts/ci-browser.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

📝 Summary

Summary by CodeRabbit

  • New Features
    • Pi chat now displays images included in prompts, including messages containing only an image. Image-only prompts appear as their own conversation turns.
  • Bug Fixes
    • Improved handling of unsupported or oversized images.
    • Prevented images from before a conversation clear from appearing in the current conversation.

The change adds validated Pi prompt-image references and image-only turns, resolves prompt and tool-result images from active transcript context, and updates browser QA coverage. Claude transcript handling also uses shared inline-image validation.

Changes: Pi image handling

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: Normal

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: Passed 5
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: displaying pictures sent to pi and treating picture-only prompts as turns.
Description check Passed The description includes the required Change and Validation sections. It clearly explains the behavior, HTTP contract impact, design, tests, browser QA, and reported check results.
Docstring Coverage Passed 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…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
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.

❤️ Share

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.
@devswha

devswha commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Merge pass: #1937 (f6db8ee) and #1930 (f1fecef) just merged to main, and this PR now conflicts with main in server/conversation.ts and server/transcript-records.ts. The review approval of 5293b7c still stands.

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.

@devswha
devswha force-pushed the feat/chat-pi-images branch from 5293b7c to 284ab4e Compare October 25, 2026 17:40
@devswha

devswha commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto main (f6db8ee, #1937) as 284ab4e. The only conflicts were the parser option in server/transcript-records.ts and its one call in server/conversation.ts: #1937 added callIds beside toolImages, which this PR renamed to images. Both are kept ({ images, callIds, taskTitles }), no toolImages is left, and nothing else in the commit changed. #1937 only adds a call id to gjc tool parts, so the pending-call rules the image endpoint mirrors are the same.

On 284ab4e: bun run check fast passes (typecheck, build, unit tests), scripts/pi-image-browser-qa.ts passes in Chrome, and the touched suites pass (pi, pi-large-history, omp-incremental, conversation, claude-incremental, ci-browser, cmd, chatRows: 290/0). #1930 (f1fecef) touches none of this PR's files and merges cleanly on top.

@devswha

devswha commented Oct 25, 2026

Copy link
Copy Markdown
Owner Author

Merge pass review-work report. Overall: PASSED. This PR adds a third ref shape to /api/pane/conversation/image: pi-prompt:<entry>:<place>. That endpoint reads files, so the change went through the Astra gate (omo-native-gate-reviewer, gpt-6-astra) with the same bar and probe set as #1935.

Lane Verdict
Astra gate APPROVE, with no blocking issues

Probes on the pi path:

  • An orphan or hidden pi entry returns 404.
  • Malformed refs, out-of-range places, lookalike ids in prose and escaped ids don't authorise the wrong entry.
  • /clear and abandoned branches are excluded.
  • 33 MiB pi records fail closed before any whole-record read. The largest allocation or read was 4 MiB.

Claude after the move:

Checks:

Follow-up: no HTTP contract test covers the pi-prompt ref shape. It would be worth adding one next to the Claude one.

Rebase onto f1fecef (284ab4e): #1937 added a callIds option to the OMP transcript parser, which conflicted with this PR in server/conversation.ts and server/transcript-records.ts. A range-diff shows the resolution only combines #1937's callIds with this PR's rename of toolImages to images: the option type, createOmpTranscriptParser, ompTranscriptParser, the preview clone, parseOmpTranscript and the gjc/pi call site. Nothing else changed.

  • pi, omp-incremental, pi-large-history and ci-browser tests pass 58/58.
  • typecheck is clean, git merge-tree is clean, and there are 0 unresolved threads.

CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it.

@devswha
devswha merged commit fae9ae0 into main Oct 25, 2026
19 checks passed
@devswha
devswha deleted the feat/chat-pi-images branch October 25, 2026 19:43
devswha added a commit that referenced this pull request Oct 26, 2026
… 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>
devswha added a commit that referenced this pull request Oct 31, 2026
## 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.
devswha added a commit that referenced this pull request Oct 31, 2026
## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant