fix(orchestrator): bound run-completion image replay

[JusticeWay]

Summary

• keep foreign and unidentified run-completion notices as text, but do not attach their pixels to an agent turn
• cap matched/legacy automatic completion previews at 8 images per event while all outputs remain visible in the panel
• tell the agent when previews were withheld or bounded
• preserve explicit user attachments and the existing run-completion journal semantics

Fixes the immediate replay amplifier documented in #1516.

Why this boundary

On 0.51.29, a fresh Codex session with zero compactions reached 211,885,110 bytes after two turns. Two foreign completions at startup produced one 105,703,660-byte record containing 28 inline images. The interrupted turn returned two events to the journal; replay produced another 105,701,700-byte record with the same 28 images. The Codex app-server reached about 3.02 GB private memory and stopped producing a reply.

The existing correlation preamble already says a foreign/unidentified completion is UNDETERMINED and must not be treated as the agent's awaited run. Attaching its pixels contradicts that boundary and makes safe journal replay expensive. For a matched automatic completion, the panel already displays every output, so a bounded representative set gives the agent visual context without making one seven-way comparison an unbounded multimodal turn.

This does not alter explicit user image attachments, remove panel-visible outputs, suppress completion notices, or change journal acknowledgement/replay behavior.

Verification

• npx vitest run src/__tests__/orchestrator/run-completion-continuation.test.ts — 80/80
• npm run lint — passed
• npm run build — passed
• full suite: 9215/9223 on the first run; the 8 failures were 7 missing better-sqlite3 native bindings from npm ci --ignore-scripts plus one 6-second ui-bridge timing flake under full parallel load
• after npm rebuild better-sqlite3, the affected suites passed independently: 7/7 and 207/207
• installed-distribution harness: foreign 0/28, matched 8/28
• live recovery: resumed gpt-5.6-sol turn completed; the matched follow-up carried one image, with Codex at about 233 MB private memory instead of 3.02 GB

[artokun]

Merged main (0.51.50) into the branch and pushed three follow-up commits. The bound itself is unchanged — foreign/unidentified still get zero pixels, matched/legacy still cap at 8.

What changed

1. The count in the sentence is now the count on the turn. The bound sliced ev.images, but the attachment is built only from entries that have a filename — a filter that predates this PR and exists because the payload arrives over the wire, where filename: string is a type rather than a guarantee (the same reason names falls back to (unnamed)). An event carrying an unnamed entry would have told the agent "the first 8 image(s) are attached below" and handed it seven. Now the bound slices the attachable list, so the number claimed is the number delivered. This is the issue's own "must not claim that previews were attached when a safety budget prevented delivery", applied to the bound's own message.

2. A withheld output now names a reader that can produce it. Both new sentences point at get_image, so the bound costs a tool call rather than leaving the agent to treat a render it can still fetch as gone. The wording deliberately stops short of "nothing was lost": previews live in ComfyUI's temp folder and a restart clears it, so the honest claim is that a fetch is the way to look, not that it is guaranteed to succeed.

3. The bound is now tested on the replay, which is the half that compounded in the report. The first delivery was never what stalled the session, and a first-delivery test cannot tell the two apart. The matched test grew a replay leg and there is a new foreign-replay test.

4. A reachability test pins the route a real panel frame travels — agent_event kind:"executed" → flushRunCompletions → manager.injectEvent → PanelAgent.injectEvent — because makeHarness only mirrors index.ts, and a mirror cannot show that the orchestrator still uses the route it mirrors.

The first version of that reachability test was passing without measuring anything: these files are CRLF in a Windows checkout, so its \n }\n end-marker matched nothing, indexOf returned -1, and slice(start, -1) handed toContain most of index.ts. It would have been green on CI and red only on Windows. Caught by the found-marker guard inside the same test; second commit normalises line endings and keeps the guard.

Verified by mutation, not by reading: widening the slice kills the matched test; disarming the untrusted gate kills three tests including the new replay one. Local: lint, build, check:vocabulary, check:unknown-collapse, vocab:export --check, asset-counts --check, i18n:check, both docs checks all pass. The affected suites pass per-file; a full-suite run on this (heavily loaded) box produced 21 files of 30s hook/test timeouts that all pass on re-run with a longer timeout.

Two things for the reviewer, neither introduced here:

• A foreign completion with run_correlation_prior (restart fences the endpoint, not the instance + cannot identify local process without start times #925) is a run this session did queue whose ticket aged out. It now gets zero pixels like any other UNDETERMINED origin. That is disclosed in the notice and recoverable via get_image, but it is a real behaviour change for a legitimate render, and worth an explicit yes/no rather than inheriting it from the foreign label.
• This bounds the count, not the bytes. On the numbers in the issue (105,703,660 bytes / 28 images ≈ 3.8 MB each) eight previews is still roughly 30 MB on a single turn. The cumulative per-conversation byte budget and the oversized-resume guard in Codex run-completion previews can replay and compound into stalled sessions #1516's "Expected behavior" are untouched, so the issue should stay open after this lands.

Refs #1516

[artokun]

Review was right on all three. The first one was right in the way that mattered: the bound was not bounding what reached the backend. Numbers first, since that was the ask.

1. Per-event was not a cap — measured

channel() drains the WHOLE queue into one turn and flatMaps the attachments (batch.flatMap((it) => it.images ?? [])), so a per-event cap of 8 delivers 8N. Measured on the real PanelAgentManager and the real journal — four matched completions landing while the agent was busy on a turn:

	images on the next turn
per-event cap of 8 (as the PR stood)	32
per-turn budget (now)	8
per-turn budget, after an interrupt that requeues the batch	8

32 is exactly #1516's compounding shape, so the PR as it stood did not fix the reported failure for matched runs — it fixed the foreign half and left the sweep case. The budget is now per turn, enforced at two points doing different jobs:

• injectEvent spends what is LEFT (queued preview images subtracted), so each notice states a count true of the turn it will land in. Synchronous from the count to queue.push, so nothing can drain in between.
• channel() caps the assembled batch, keyed on completionOnly so a user's own attachment never spends the preview budget — Codex run-completion previews can replay and compound into stalled sessions #1516 asks for that to stay a separately reviewed policy.

Each mechanism holds alone (disabling either still measured 8). That makes the drain's shortfall branch unreachable while the other stands, so it logs instead of telling the agent something no reachable state can produce. I'd rather ship a guard I can't reach than a paragraph I can't test.

2. The remedy was not callable

Correct, and worse than the review states in one spot. get_image action:"get" takes a filename and defaults type to "output" — but a PreviewImage output lives in temp, the event's name list carries filenames only, and a custom note (the panel's video-storyboard summary) replaces that list outright. So for the video case the withheld outputs were named nowhere at all, and for the ordinary preview case get_image would have looked in output/ and missed them.

Withheld outputs now carry type and subfolder whenever those differ from the default — preview_b.png (type:"temp", subfolder:"clips") — and are emitted independently of the note. An UNDETERMINED run with no usable filename now says it cannot be fetched by name and points at get_history, rather than offering a colon with nothing after it.

3. Zero images, zero notice

Fixed here as asked. Worth saying plainly: I called this pre-existing and unreachable in practice, and "unreachable in practice" was a judgement I had taken no measurement for. It promises pixels the turn does not carry either way, and it is in the exact sentence this PR rewrites.

Verification

Every one of the four is mutation-checked, not just asserted: removing both budget halves measures 32 again; stripping the coordinates drops the type:"temp" assertions; disarming the no-filename branch restores the false "attached below". Six #1516 tests now, covering the shared turn, the requeue, the coordinates, both no-filename paths, and the production route.

Local: full suite green, lint, build, check:vocabulary, check:unknown-collapse, vocab:export --check, asset-counts --check, i18n:check, both docs checks.

Still true from before, still for you

• This bounds count, not bytes. Eight previews is still ~30 MB on the issue's own numbers. Codex run-completion previews can replay and compound into stalled sessions #1516 should stay open.
• foreign + run_correlation_prior (restart fences the endpoint, not the instance + cannot identify local process without start times #925) — a run this session did queue, whose ticket aged out — gets zero pixels. Disclosed and now genuinely fetchable, but still a behaviour change worth an explicit yes/no.

Refs #1516

[artokun]

It reproduces. Codex is right, my "unreachable" was wrong, and logging was the wrong call.

I built the exact state and measured it before touching anything.

The state, and what it produced

queuedPreviewImageCount() reads queue; an in-flight batch is not on it. So:

1. completion A is taken in flight with its 8 previews;
2. completion B arrives behind it, reads the budget as untouched (queue is empty) and commits its own 8 — its notice says "The first 8 image(s) are attached below";
3. an interrupt requeues A beside B, and one drain sees 16.

	measured (before this commit)
images on the resulting turn	8
…of which came from A	8
…of which came from B	0
"attached below" claims in that turn	2
B's filenames anywhere in the turn text	absent

B carried a custom note, which replaces the generated name list — so its outputs were named nowhere at all and get_image was impossible. Exactly the chain Codex described, no link broken.

The ceiling was holding the whole time. What was broken is the half of #1516 that actually costs the user something: the agent is told the render is attached, is shown nothing, and has no way to fetch it.

Why my earlier measurement missed it

My requeue test interrupted first and injected the second batch after — so the requeued items were already back on queue when the new completion computed its budget, and it correctly saw 8 spent. Codex's ordering queues the second batch while the first is still in flight, which is the only window where the count is blind. Same two operations, opposite order, different answer. That's the whole difference, and my version of the test was the one that couldn't see it.

The fix

The drain now corrects the claim rather than enforcing the number silently. It is the only place that can: the notice was composed before its images were known to be losing a turn, and with a note those filenames exist nowhere else in the turn. The appended note states how many were not attached, says plainly which sentence to believe when two disagree, and names every trimmed output in coordinates get_image action:"get" can use:

[panel note: 8 automatic preview image(s) were NOT attached to this turn. Several run completions were merged into one turn and they SHARE a single 8-image budget, so a notice above may say its images are "attached below" when they are not — where they disagree, THIS note is the accurate one. They are outputs on the ComfyUI side that this turn does not carry — fetch any of them with get_image action:"get" — B_0.png (type:"temp"), …]

Counting in-flight items at inject time is deliberately not the fix. A turn that ends normally never comes back, so charging its images against the next budget would starve every sequential render of its preview — the common case broken to tidy the rare one.

Measured after: trimmed outputs are named with type:"temp" and fetchable. Mutation-checked — disarming the disclosure restores the silent false claim. I also dropped "Nothing was deleted" from the wording, for the same reason "nothing was lost" went earlier: a temp output does not survive a ComfyUI restart and I won't promise the fetch succeeds.

On the retraction

That is twice on this PR that "unreachable" turned out to mean "I did not construct it." Both times the word was carrying weight no measurement supported, and both times it was load-bearing for a decision — first to leave a pre-existing branch alone, then to log instead of tell. The pattern is mine, not Codex's, and the review caught it twice.

Full suite green, all gates green, CI to follow on the pushed head.

Refs #1516