[jozer-rami]

Summary

Supersedes #1099 (closed without merging on 2026-11-10). This branch is feat/document-read-tool (head 4b649f433) with dev merged in at d02fcb9f1, plus the follow-up work from zach88's round-2 review, from a deep review of e1fe20095, and from the two reviews of ee95833cb (Evenfire Reviewer round 1 and an independent review). The design, scope and approved deviations of clerum__document_read are unchanged from #1099. See that PR's description, corrected by the sections below.

Part of #656 and #1093. Neither closes yet:

• bug(grok-subscription): read PDF/XLSX attachments through a Host-side document reader tool #1093, visual mode on Grok: Grok gets visual_not_supported. resolveVisualDeliveryLimits('grok-subscription') is null (pinned in deliveryLimits.test.ts), so the criterion "visual mode returns a page image that a Grok model reads" is not met. Text mode works on Grok, Codex, Claude and every other provider.
• feat(agent-tools): Host document reader tool (PDF text, selected-page images, XLSX) #656 and bug(grok-subscription): read PDF/XLSX attachments through a Host-side document reader tool #1093, live smoke: the T2 / make minikube-run-document-upload lane has not run on this head.
• feat(agent-tools): Host document reader tool (PDF text, selected-page images, XLSX) #656, production image: the image smoke now loads the reader (below). Image size and startup time on the production image are still unmeasured.

Deep review fixes (e1fe20095 → this head)

Each finding below has a test that failed on e1fe20095 (or, for test-only gaps, fails under the mutant the review found surviving).

Finding	Commit	Change
B1 CI red: workerRunner.test.ts sampled during the sh → node exec	d35702d8a	The test waits for node's second thread. During each execve, /proc shows resident pages with VmData 0, and the kernel renames the process to node before node is mapped, so the name alone is not enough. In arm64 containers (node:24.22.1 slim and alpine), one sample per spawn read VmData 0 1/300 and 3/300 times right after spawn, and 0/300 after the wait.
M1 x64 RLIMIT_DATA reserve measured through Rosetta	d35702d8a	x64 V changes from 1963 to 1534 MiB, the native median of 175 jobs on the glibc CI runner (range 1526–1602), about 430 MiB less. Replaying all 216 x64 CI reader jobs against the new limit, none exceeds it. The tightest is the job the sampler already SIGKILLed, with 73 MiB to spare, and the tightest successful job has 162 MiB. A Linux-only test re-measures the idle child on every run and fails outside the 128 MiB drift, so CI x64 now confirms the figure. arm64 measured 1281–1292 MiB against 1282. x64 musl is unmeasured.
M2 XLSX row probe stalls the Host event loop	94c314423, 14eedd3a2	Row probes are measured by group and halved only when a group does not fit, and only rows the page can reach are checked: a call cannot emit more rows than fit a page as []. 5,000 one-cell rows with the real preview and token counter: 1 sheet 474 → 75 ms; 300 sheet names 4,988 → 80 ms; a 10,000-character name 17,859 → 201 ms (5,015 → 21 measures). A sheet list crafted to nearly fill the page took 10,016 measures with grouping alone, and takes 137 with the reach bound. There is still no byte-length shortcut (see R1-H1 below).
M3 prefixed SpreadsheetML (x:row) read as document_empty	0b9f5f7ce	Elements are matched by local name, and the sheet's relationship id is resolved by namespace.
M4 two redaction guarantees untested	b2713409c	One test covers a configured value split by a PDF continuation cut, and one covers redaction running before the cell cap. Each fails under the mutant that survived 223/223.
M5 a row without r naming the next row failed the workbook	0b9f5f7ce	The cell reference is compared with the row before the provisional number.
M6 sparse gaps used up the read budget	0b9f5f7ce, 426285b83, 1752f041d	A run of empty rows is returned whole or not at all, so a gap costs at most one call. Pinned in the parser, and also end to end: a 200k-row gap read through the real child and the Tool, following the cursor. The gap logic sits outside the per-row function: inlined, it raised the 150k-row streaming witness's child RSS by 50–155 MB in 6 of 9 full-suite runs (bisected; 0 of 10 without it).
Visual image_bytes paused on Codex for a page too large for an empty request	f41efbf68	The read pauses only when earlier images shrank the bytes; such a page fails as image_bytes instead of looping on reattach. Only Codex had the loop: on Claude the call cap (20,971,520 bytes) is already below row 7 at zero usage (21,000,000), so this changes nothing there. The commit message and the comment at index.ts:741 still say "Codex and Claude".
Admission refusals	d35702d8a, 9c52cc3e0	An unreadable cgroup now returns host_memory_unavailable, as MEMORY_MODEL.md says, instead of a worker defect. Each refusal logs Document reader refused to start with its condition. Since 9c52cc3e0 the model-facing message no longer mentions a retry (see below).
Reader never loaded in an image job	9ff8622c9, f34977657	image-runtime-smoke.cjs extracts and renders a one-page PDF through the shipped modules, and since f34977657 also runs the dist child (see below).
R2-L5 (zach88): missing requestImages spent a read	ce5a2c257	The check runs before ledger.beginRead().
Surviving mutants M6b, M19, M24, M28b; R1-H2 cursor chain; zach88 R1-M9, R2-L4, R1-L5	3b27b51ae	Each test fails under its mutant. Fake runners now validate outcomes with the worker's isJobOutcome, which caught two invalid hand-built outcomes. boundCursor uses the production encodeCursor. zipLimits.test.ts adds 45 tests: 34 of 38 single-guard deletions fail, and the other 4 are implied by earlier checks.

Review fixes for ee95833cb → this head

Fixes for the two reviews posted on ee95833cb: the Evenfire Reviewer's round 1 (R1-) and the independent review (M1–M4). Each change has a test that fails without it. For M3 and M4 that is the mutant the review found surviving; for M2 it is the reviewer's workbook shape.

Finding	Commit	Change
R1-H1: the head no longer merged with dev	a992742dc, 2e1f47022	dev merged twice (see Merge with dev).
R1-M1: host_memory_unavailable said "Retrying later may help"	9c52cc3e0	Three of its conditions never clear on retry. The message no longer promises a retry and points at the Host log; round 2's N2 (7b5bffb4e) reworded "cannot start" for the passing host_headroom case. A test pins the text.
M1: visual pages read before an approval suspension never reached the model	057e45cc8	The resume rebuilt the next request from text-only tool messages. It now appends the batch with the loop's own appendToolResults, seeded with the turn's attachments from before the batch, so page images from completed_results and from the approved call reach the request. The forwarded image count now describes what the request carries. This also applies to other tools: their same-batch images, e.g. screenshots, now reach the resumed request too. Tests count the image parts of the rebuilt request: the M19 batch (20) and a real approved visual read (1).
M2: synchronous measure loops blocked the Host event loop for seconds	19ec7babf	XLSX row probes stop once an exact measure shows that no result emitting a later row can fit. fit aims its search at the allowance instead of bisecting. Measures per call: the reviewer's workbook (1,300 31-character sheet names, 430 rows of 900 characters) 874 → 21; the nearly-full sheet list 137 → 83; a 400,000-character PDF page 21 → 6. Tests assert a worst-case bound in emitted and window rows. A differential pins 18 PDF and XLSX read chains (20 since 9d78c9d7e) to the previous implementation's results under a byte-count measure; they include sheet boundaries, parser-skipped and too-wide rows, and a partly spent turn budget.
M3: no gate ran the shipped child chain	f34977657	The image smoke runs a pdf_text job through the dist runner in every Host image CI builds (alpine, slim, full and desktop, on amd64 and arm64). That covers the image's /bin/sh, ulimit -d, exec of the dist entry and env: {}. It runs without --memory, so it proves the spawn chain and IPC, not headroom admission. The rendered page must also carry ink.
M4: security wiring and slot release unpinned	581c9279a	Tests cover three things. document_read masks a ConfigStore secret (NR2) and charges the same ledger as attachment_read (NR1). The reader slot is released on a throwing pre-spawn premise, on an abort between admission and spawn, and on a synchronous spawn throw; each test then runs a real job that must start. The RSS interval is cleared when the job settles.

The independent review's Low findings are not fixed here. One of them comes from 0b9f5f7ce, which matches by local name: a foreign-namespace <foo:row> or <foo:c> element is now read as data.

Review fixes for 80d71c642 → this head

Fixes for the independent review's round 2 on 80d71c642 (N1–N6, P1, P2).

Finding	Commit	Change
Merge state: conflicting again	c583c141a	dev at 0ca497d37 merged (see Merge with dev).
N1: the aimed fit search assumes cost grows with n	c90f2fb77	Code unchanged: a result never exceeds the allowance. Under the production token measure, which can drop by one token as a result grows, it can hold a few characters more or fewer than a bisection. The differential now says its "results unchanged" claim holds under its byte-count measure.
N2: "cannot start" is too absolute for host_headroom	7b5bffb4e	The message now reads "This Host could not start the document reader for this read: it had too little free memory, or it lacks a configuration the reader requires…". It promises neither a retry (R1-M1) nor permanence; the test rejects "retry" and "cannot".
N3: a repeated image can be sent twice after a cold resume	not fixed	See Known limits.
N4: three resume mutations survived	cd4cb5aa6	Tests for a repeated frame with and without source identity (dropped, or named by reference only), and for rebuilt tool messages carrying no spillover ref, which pins the rebuild's existing behaviour (the loop's own append keeps the ref). Each of the three mutants now fails its test.
N5: the smoke comment overstated the ink check	c90f2fb77	The comment now says ink shows the page was drawn, not which font face drew it.
N6: a test cited a commit that does not exist	c90f2fb77	It cites f34977657, the commit before the M2 change.
P1 (on dev before this branch): a second suspension in one turn re-sent answered tool results	a7af8232d	The resume appends, and counts images for, only the batch results that the snapshot's last tool-call message still lacks. Its test drives two suspensions in one turn, frozen the way toolUseLoopToolBatch freezes them. Before the fix, the second resume sent call-capture and call-approve-1 again after the new tool-call message and counted the frame twice; a provider's response to that request was not run. It is its own commit, so it can move to a separate PR.
P2: rows that are all too wide still stalled the event loop	9d78c9d7e, 6fff76dc3	Blanked rows feed the early stop. After 4 blanked rows in a row, groups are split without a probe until a row fits again. This only skips group probes, so results are unchanged under a measure that grows with the result (the byte-count differential); they were not compared under the production token measure, whose counts vary per run with the fixture's random ids. A reconstruction of the reviewer's shape (300 sheet names, 600 rows each too wide; the reviewer measured 703 per call on their own workbook) fell from 1,340 to 516 measures with the turn ledger and from 1,338 to 709 with a fresh one, over 2 calls. With the production BPE measure on this machine, it went from about 3 s to about 1.3 s per call. The shape is added to the differential: all 20 chains return the same results as f34977657, and a bound on measures fails at 80d71c642 (1,340 > 750).

Review fixes for 6fff76dc3 → this head

Fixes for the independent review's round 3 on 6fff76dc3.

Finding	Commit	Change
A-1 (on dev before this branch): P1 still occurred with drivers whose tool-call ids restart on every response	367dd959e	The Gemini driver numbers calls from call_0 on every response, so the earlier batch's call_0 result was appended again as an answer to the new call_0, and its frame was counted twice. Results carried from an earlier batch are now marked answeredInSnapshot when they are carried; the resume skips them, and the GFS projection keeps the mark through the SQLite row. The id filter stays for approvals saved before the mark. Tests: a two-suspension turn with restarting ids (red before the fix: Captured re-sent after the new tool-call message), and the mark surviving projection and a cold load (red without the projection change). Each of the two taskExecutor sites, removed alone, turns the first red.
A-2, A-3 (text)	367dd959e	The rebuild comment now says the resolver keeps the spillover ref and the map drops it; the helper says the earlier batch is carried only when the turn has attachments.

Not changed: B-1–B-5 (coverage and wording of the P2 commit), B-6–B-8 (info).

Final review fixes (367dd959e → this head)

A final review of the whole diff, by slice: resume path, Tool, reader child and memory, parsers, and boundary. Each finding was checked against the code.

Finding	Commit	Change
A default OSS or minikube Host refused every read	07f7112de	HCC's default Host memory limit was 512Mi. Admission needs the Host's working set plus 433 MiB of reserves plus at least 128 MiB, and idle Hosts on the dev cluster use 135–165 MiB (kubectl top). So every read on a default install returned host_memory_unavailable. The default is now 2Gi, the value hosted Hosts run with; the request stays 128Mi. A test pins the default. This PR now changes host-context-controller: each Host Deployment rolls once when HCC is upgraded. T2 has not run, so this is not yet proven on a minikube Host.
Four paths survived mutation	45212ddbe	Each new test fails under its mutant. The four paths: an approved result whose Gemini id recurs in the next batch; the id check for approvals saved before the mark; later sheets probed after the page ends in an earlier one (26 against 352 measures, same result); and a PDF text cut that splits a surrogate pair.

Not changed: two minor findings, listed under Known limits (the oversized cursor and the guardrail tool name).

Round-2 review findings

The four blocking findings were already fixed in code on #1099's head, by the commits after e7fa6846f. This branch adds the regression tests and design notes the review asked for, with one commit per finding:

Finding	Commit	What it adds
R1-H2 visual request byte limit	d8ac778de	VISUAL_BUDGET.md decision table (8 limit sources, the smallest one wins, and what happens when each runs out), plus a test that follows the cursor through a long scan and pushes the result through the real assertVisualRequestFits with the Anthropic profile. Removing the request-byte term makes it fail (58,926,114 > 32,000,000).
R1-H4 cgroup OOM	56d59ca9f	MEMORY_MODEL.md: the protection layers, why RLIMIT_DATA and not RLIMIT_AS, an evaluation of a delegated sub-cgroup (not implemented, because a non-root container can't create one), the remaining risk, and the cluster settings that limit it.
R2-M1 uncached formula reads as heap	1016349f5	xlsxUncachedFormula.test.ts: a 150k-row sheet with one formula that has no saved result, plus its continuation. Fails on e7fa6846f, passes on head.
R2-M2 / R1-M6 terminal row_width	844ee2a5c	xlsxRowWidth.test.ts (5 tests in that commit; 3b27b51ae adds 2): a 13×32,767-character row as row 2 and as row 1, a 12-cell control, the A1/A2/XFD2/A3 stray-cell case at 128k and 200k, and a cursor chain. 4 of 5 fail on e7fa6846f.
X1 (new) wide-row measurement	8c8079051	Fixes SheetWindow.add, which serialized whole rows to measure them. A 51 KB workbook with one 32,767-character shared string repeated across 16,384 columns serialized to about 537 MB and failed the whole sheet as heap. Rows are now measured with an early exit (serializedRowLength). Includes a regression test.
Merge follow-up	e1fe20095	dev's attachment-save test listed a file as reader=binary. The strict document-reader fixture rejects that value, so the test now uses reader=text.

R1-H1 (the Host's fit step): the 231–301 ms figure covered fit only, not the XLSX row probes. Both are reduced by 19ec7babf (independent review M2, above). There is still no byte-length shortcut: previewForLlm can make content longer (tag escaping) or shorter (redaction).

Closed in writing:

• R1-M3, earlier document_read results are not collapsed by C17. This is token cost, not correctness. A stub needs a strict parser for each result shape (PDF text, visual, XLSX), so it is left for a follow-up issue.
• R1-M4, a single reader slot with no queue depth or wait timeout. One job per Host is deliberate (feat(mcp-host): read PDF and XLSX attachments with clerum__document_read #1099 deviation 12). A job is bounded by its 30 s time limit, and the slot is released on every path. One caller can still delay another by up to 30 s per queued job.
• R1-M7, a partial prefix of a real XLSX classified as binary_unsupported. No production caller passes partial prefixes.
• R1-M10, fake pdfjs document in pdfText.test.ts. The real-pdfjs tests cover the same branches.

Merge with dev

dev (d02fcb9f1) is merged in 2d15076b5. 8 files conflicted:

• Version lines in the mcp-host and tests/e2e/fixtures package files.
• The T1 scripts: this branch takes dev's versions. dev already runs only the public password-recovery suites, so feat(mcp-host): read PDF and XLSX attachments with clerum__document_read #1099's T1 commit 33f037f62 is no longer needed.
• provider.mjs: uses dev's toolOutputPattern and keeps both output patterns.
• README.md: keeps both sections.

dev at b6db9c297 (the Next.js 16.4.6 upgrade and the CI dependency gates) is merged in ee95833cb. There were no conflicts and no mcp-host files changed. The PR's diff against dev stayed at 77 files then; it was 82 at 367dd959e and is 89 at this head.

dev at 4fd7b2878 (184 commits) is merged in a992742dc. Only mcp-host/package.json and package-lock.json conflicted.

• package.json: dev's openai ^4.87.0, with the reader's pdfjs-dist 6.4.299, saxes, exceljs 4.4.0 and @napi-rs/canvas ^1.0.11.
• package-lock.json: regenerated with npm install --package-lock-only, starting from dev's lockfile. Regeneration resolved canvas to 1.1.1. I put back the 12 @napi-rs/canvas entries at the 1.0.11 the reader was tested with, then re-ran npm, which left them. Every entry in the merged lockfile now matches one parent, and npm ci installs it.
• Version: 0.1.633, the repository's max(dev, branch) + 1, rather than dev's 0.1.632.

dev at 05419da54 (#1434, the lone-surrogate repair) is merged in 2e1f47022. Only the version line conflicted: dev 0.1.635, branch 0.1.639, merged as 0.1.640.

dev at 0ca497d37 (59 commits) is merged in c583c141a. Only the version lines conflicted: dev 0.1.659, branch 0.1.641, merged as 0.1.660. dev changed no mcp-host dependency.

dev at 1b1b1131e (54 commits) is merged in 8686fb77a. Five files conflicted:

• Version lines in the mcp-host and host-context-controller package files. mcp-host: dev 0.1.661, branch 0.1.669, merged as 0.1.670. host-context-controller: dev 0.1.445, branch 0.1.427, merged as 0.1.446. dev changed no dependency in either package, so the branch's dependencies stand.
• host-context-controller/README.md: dev re-padded the Host env table. The merge keeps dev's table, with the memory limit row at 2Gi.

dev's config.ts change (request deadlines for the NetworkPolicy pass) merged without a conflict and does not touch the Host resources.

Every merge commit's tree equals the automatic merge, apart from those conflict resolutions. The PR's diff against dev is the same 89 files, with the same line counts, before and after 8686fb77a.

Release note (replaces #1099's)

There is no required deploy order between Desktop and the Host.

• New Desktop with an old Host: the composer shows PDF and XLSX chips as readable, but the old Host doesn't read them.
• Old Desktop with a new Host: works. The new Host reads the file, and the old composer's chip says "No reader available".
• GFS file references to PDF and XLSX carry reader: none. clerum__document_read reads only files attached to the message.
• HCC default Host memory limit, 512Mi → 2Gi. Each Host Deployment rolls once when HCC is upgraded. An explicit CONTEXT_MAPPER_HOST_RESOURCES_LIMIT_MEMORY is kept.
• Guardrail and approval rules match tools by name. A rule that denies or asks for clerum__attachment_read does not cover clerum__document_read; add a rule for it, or use a pattern such as clerum__*_read.

Operator prerequisites (details in MEMORY_MODEL.md). When one is missing, every read returns host_memory_unavailable, and the Host logs Document reader refused to start with the reason:

• Linux with a unified cgroup v2 memory controller.
• /sys/module/kernel/parameters/ignore_rlimit_data reads N.
• An x64 or arm64 node.
• Host memory above its working set plus about 433 MiB (margin, IPC and drift reserves) plus the reader: at least 128 MiB, and 768 MiB for the full default reader. Idle Hosts on the dev cluster use 135–165 MiB, so the old 512Mi default refused every read. HCC now defaults to 2Gi (07f7112de). An operator who sets CONTEXT_MAPPER_HOST_RESOURCES_LIMIT_MEMORY below about 730 MiB gets host_memory_unavailable on every read. Not yet measured on a T2 Host.
• Recommended: kubelet singleProcessOOMKill: true (Kubernetes 1.32+), so a combined overrun kills only the reader.

Corrections to #1099's description

• Out-of-memory claim. feat(mcp-host): read PDF and XLSX attachments with clerum__document_read #1099 said running out of memory returns heap "without touching the Host process". Corrected: the child's private memory is capped by RLIMIT_DATA, so an oversized document fails an allocation inside the child and returns document_limits_exceeded / heap. The parent also samples memory every 25 ms, kills the child early, and marks it as the first OOM victim. This protects the Host from the document, but not from the Host's own growth during a job. If the Host and the child together reach the container limit, the kernel OOM killer runs, and with memory.oom.group=1 that restarts the container. Kubelet singleProcessOOMKill: true (Kubernetes 1.32+) makes the child the only victim.
• Wide XLSX rows. A row too wide for one call comes back as [], is listed in skippedRows, and the read continues. A row of about 13 maximal cells is skipped rather than shown cut, on purpose: redaction has to see whole values.
• No text layer. A PDF with no extractable text, including a blank one, returns no_text_layer. The message says the selected pages have no text layer and points to mode: "visual"; on a model without image input it says a vision-capable model can read visible content.

Known limits (not fixed here)

• Grok visual mode (see Summary).
• CDATA sections in XLSX parts are not read (as on e1fe20095). No spreadsheet writer seen emits them, and a saxes cdata handler measurably raised the streaming child's RSS.
• Page PNGs are also collected as response attachments. They are carried in the pending approval and dropped at delivery as non-JPEG, with one warning each. The pending approval is also written to its SQLite row, so the PNGs are persisted with it: projectGfsApproval strips only GFS images. Since 057e45cc8 the resume uses those persisted pages. In the default loop the model's image parts depend on the same collection, so separating them is its own change.
• The 150k-row XLSX streaming witness is flaky under load. The independent review saw 9 failures in 31 runs on macOS under heavy load, against 0 in 20 at e1fe20095. It bisected the cause to 0b9f5f7ce, without isolating a root cause. My earlier 3 passes in 3 runs do not refute that.
• Duplicate sheet names, which only crafted files have (Excel forbids them), resolve the continuation to the first sheet of that name.
• fileReference.cjs accepts reader: pdf/xlsx on GFS references. Only the Desktop producer emits references, and it never does.
• No request-level check for page-only image requests. assertVisualRequestFits checks only requests that also carry GFS images. A request whose only images are document_read pages relies on the 4 MB text reserve (VISUAL_BUDGET.md).
• A repeated frame can reach the model twice after a cold resume (round 2, N3). The resume's dedup seed, the approval's attachments, is not written to SQLite, while its completed_results are. After a restart, a frame that repeats an image from an earlier iteration of the turn is sent again, or in full instead of by reference under source identity. Its image count is still charged. A fix needs attachment metadata persisted with the approval row.
• Rows that are all too wide still cost one measure each (round 2, P2). Every row a call emits as [] needs its own exact probe, each the size of a page. Beside a long sheet list that is about 1.3 s per call on this machine with the production measure; the independent review measured a median of 2,219 ms per call on its own workbook (round 3). Going lower would change what a read returns.
• An oversized cursor after the read count is spent fails the turn (final review). The read-exhausted notice repeats the model's cursor and attachmentId unchecked. One longer than a page (10% of the context window) makes the notice debit throw, and the turn ends with an error instead of the notice. clerum__attachment_read repeats only the resolved id and a validated offset.
• Guardrail rules naming clerum__attachment_read do not cover this tool (final review). See the release note.

Verification (this head)

Gate	Result
CI, every check	pending at 8686fb77a; 63/63 passed at 367dd959e, 6fff76dc3 and 80d71c642
host-context-controller full suite at 8686fb77a (macOS arm64)	130 files, 3,114 passed; tsc --noEmit clean
Changed mcp-host test files at 8686fb77a (taskExecutor, pdfText, fit differential)	3 files, 116 passed; tsc --noEmit clean. Each new test failed under its mutant at 45212ddbe; the two resume mutants had passed all 540 tests in the 26 resume test files before.
Local serial run at 367dd959e: agent, core/conversation, visualInput, orchestration, ironclaw, session approval (macOS arm64, --fileParallelism=false)	131 files, 1,699 passed, 1 todo
Local serial run at 9d78c9d7e (6fff76dc3 changes comments only): documentRead, agent, orchestration and nativeToolRegistry.documentRead (macOS arm64, --fileParallelism=false)	115 files, 1,592 passed, 3 skipped (Linux-only)
Resume suites after P1 (taskExecutor, attachment approval budget and integration, Responses continuity, approved-tools lifecycle, cold-start loader, GFS approval suites, session approval, suspension projection)	11 files, 249 passed
CI mcp-host (ubuntu-24.04 x64) at 2e1f47022	624 files, 9,040 passed, 1 skipped, 1 todo. The x64 reserve witness measured 1538 MiB against the calibrated 1534 MiB (drift allowance 128 MiB), as at ee95833cb.
CI Host image smokes at 2e1f47022	alpine, slim, full and desktop, on amd64 and arm64: PASS, with childPdfText (the dist child through /bin/sh -c 'ulimit -d … && exec') and pdfRenderInk. On alpine amd64 (x64 musl, its first run) the child finished ok: data limit 2,488,320 KiB; VmData 1,708 MiB with up to 132 MiB resident, so about 1,576 MiB non-resident, within 1534 + 128. That was one small job.
mcp-host tsc --noEmit, eslint, prettier (changed files)	clean
mcp-host full suite locally (macOS arm64, load average about 14)	9,027 passed. 10 timed out at their 5 s or 20 s limits under load; the 7 files involved pass when run one at a time. The documentRead folder run one file at a time: 21 files, 319 passed, 3 skipped (Linux-only).
make minikube-t2-public-boundary	PUBLIC_BOUNDARY_PASS at 8686fb77a (CI's shell job rejected 2e1f47022 for a fixture value; fixed in 80d71c642).
Release validators at 8686fb77a (host-context-controller 0.1.446, mcp-host 0.1.670)	validate-release-version-bumps --previous origin/dev and the desktop manifest check pass.
Linux-only tests (node:24.22.1 slim and alpine, arm64), at d35702d8a	workerRunner.test.ts 66/66 on each; B1 sampler test 20/20 on each; reserve witness 1289–1292 MiB (slim), 1281 MiB (alpine).

Not run on this head:

• T2 and make minikube-run-document-upload. This lane is now also the check that proves the 2Gi default lets a minikube Host read.
• V1.
• A warm-cache run of the working-set headroom (the 512 MiB OSS default).
• Confirming the cluster's memory.oom.group / singleProcessOOMKill setting.

🤖 Generated with Claude Code

[Mircala]

Evenfire Reviewer — Round 1

Reviewed head: ee95833cbe439d2aba8eb54448307f15e147bb27

Decision: Blocked

The reader design holds up: attachment binding, worker isolation, cursor binding, and the visual-budget pause split are coherent. Merge is blocked because this head does not merge with current dev, and the model-facing memory-refusal text still tells the model to retry a configuration failure.

Must fix

• R1-H1 — High — mcp-host/package.json:1 — Rebase onto current dev (e611ebfcb, 151 commits ahead of the recorded base). The trial merge conflicts in mcp-host/package.json and package-lock.json. Keep pdfjs-dist 6.4.299, saxes, exceljs pinned at 4.4.0, and @napi-rs/canvas ^1.0.11. Take dev's version 0.1.632 and openai ^4.87.0. Regenerate the lockfile; do not hand-merge it.
• R1-M1 — Medium — mcp-host/src/core/tools/documentRead/index.ts:101 — host_memory_unavailable still says "Retrying later may help". rlimit_data_ignored, uncalibrated_architecture, and cgroup_unavailable do not clear on retry. Say this Host cannot start the reader and point at the operator log.

Verified

• clerum__document_read is registered only for an admitted reader=pdf or reader=xlsx attachment on the current message, and clerum__attachment_read does not decode those bytes.
• The worker drops the environment, validates IPC outcomes, and applies RLIMIT_DATA through a fixed ulimit script with the limit as a separate argv entry. The CodeQL shell findings are not an injection path.
• PDF resource loading is confined to bundled basenames. saxes does not install DTD entities. Worksheet targets must match xl/worksheets/sheetN.xml.
• Visual resume counts images from the frozen snapshot and earlier batch results. GFS references are forced to text or none.

Validation gaps

• T2 / make minikube-run-document-upload has not run on this head (the PR says so).
• Previous-head regression proof was not re-run in this environment.
• No test asserts the host_memory_unavailable message text (R1-M1).

PR description

Update required. After the rebase, name the new dev SHA and the combined dependency set. docs/testing/desktop-headful-journeys.md:39 still says the upload lane is waiting on the final head of closed PR #1099 (R1-L1, follow-up).

Pre-existing

Pre-existing: none identified during the scoped review.

Next action

Rebase onto current dev, keep both dependency sets, and delete the retry promise from the host_memory_unavailable message.

Full review archive: pr-reviews/pr-1424/r1-26-11-12.md

— Review performed by Evenfire Reviewer

[alfredolopez80]

Independent review — ee95833cb (blind round 1 + code review)

Scope: head ee95833cbe439d2aba8eb54448307f15e147bb27 against merge-base b6db9c297 (dev). 77 files, +15962/-219.
Behaviour-preserved checks were also run against two earlier reviewed heads: 429de1d10 and e1fe20095.

Method: eight independent review lanes:

• child process and memory admission;
• XLSX parser;
• PDF and visual budget;
• integration (tool loop, gfs-interaction-policy, Desktop);
• security;
• contract and PR-body claims;
• test quality;
• behaviour-preserved checks.

Lanes worked only from the diff, the PR body and the repository code and tests. A line-by-line code review ran alongside them. Findings are backed by runs, mutations or transplanted tests unless marked read-only. Everything ran on macOS arm64. Linux-only paths were read, not run.

Verdict

No Critical or High findings. There are two Medium functional defects (M1, M2) and two Medium test-coverage gaps (M3, M4).

Suggestion: fix M1 and M2 and close M3 and M4 before the T2 lane, then run T2 once on the resulting head. The PR body says T2 and make minikube-run-document-upload have not run on this head.

---

M1 — Medium — Visual pages read before an approval suspension never reach the model after resume

Where: mcp-host/src/agent/taskExecutor.ts:881-893

After approval, the next request is rebuilt from context_snapshot plus tool messages that carry only content. The image parts of completed_results and of the resumed call are dropped. The tool result still says mode:"visual" with an images[] list, so the model is told it has page images it never receives.

requestImageUsage(approval.context_snapshot, resolvedCompletedResults) at :825 also counts images that the rebuilt request no longer carries. In the M19 scenario it counts 20 images while 0 are sent. A resumed visual read can therefore pause with "request already carries as many images" while every slot is free, and the M19 test pins that overcount.

Reachable in two ways:

• Any guardrail rule set. On a Host with a rule set that does not match this tool, unmatchedDefault(hasRules) returns ask (core/guardrails/decision.ts), even though requiresApproval() is false.
• Approval later in the same batch. On any Host, a visual read followed by an approval-gated call in the same batch lands in completed_results.

It does not occur on a Host with no guardrail config whose batches need no approval.

Run: a resumed real visual read rendered 1 page, and the next request carried 0 image parts.

Attribution: the text-only rebuild predates this PR (5d663a496). This PR adds the exposure (2b031b3e0, cefcab7d6) and the tests that pin it (12b1a2b93, 3b27b51ae).

M2 — Medium — Synchronous measurement loops block the Host event loop for seconds per call

Where: mcp-host/src/core/tools/documentRead/index.ts:891-1028 (XLSX row probes) and :1121-1175 (fit)

XLSX cost. Take an Excel-legal workbook: about 1,300 sheets with 31-character names, and about 430 rows of 900 characters. Each call makes about 800 full measures (preview plus token count over the whole sheet list) and emits one row.

Load-independent figure. The same input takes 821 measures at HEAD and 442 at e1fe20095. The M2 rework (94c314423, 14eedd3a2) raised the cost on this input; it bounds only the small-row case.

Re-run on the exact HEAD (lightly loaded machine, 200k window): 799 measures and 5.0 s for one execute, 1 row out. Reading the 430 rows takes about 430 calls, each blocking the event loop for about 5 s.

Under heavy load (not re-run): 11.1 s at 200k through the real child, with one unbroken 10.9 s block, and about 55 s at a 1M window. No deployed model is known to report a 1M window.

PDF text. fit runs 21-22 synchronous measures over up to about 400k characters on every call, cursor continuations included. Under heavy load that took 0.4-0.6 s at 128k-200k windows and about 2 s at 1M. A 2.7 KB PDF triggers it.

On the PR body. The "231-301 ms" R1-H1 figure covers only fit, excludes the row probes, and was not reproduced under comparable conditions.

Contributing factor. margin (:1023) is measured with the full wrapped measure, which counts the wrapper overhead twice. Fewer groups pass, so the code halves and re-measures more often.

M3 — Medium (coverage) — The shipped child chain runs in no gate

No test or smoke runs dist/.../worker.js as a child process, and none runs the Linux /bin/sh -c 'ulimit -d … && exec' path:

• the Linux tests start the child through the TypeScript bootstrap;
• image-runtime-smoke.cjs calls the parsers in-process.

The mutant WR11 survives 207 tests. A dist probe gives EXIT=0 at head and "Document reader worker failed" (EXIT=4) under the mutant.

The production combination is unverified until T2: alpine/busybox sh, ulimit -d, exec of the dist entry and env: {}.

M4 — Medium (coverage) — Security-relevant wiring and slot release are unpinned

Registry wiring (nativeToolRegistry.ts:420,432-438). Neither property below is tested, and each mutant passes 315/315 across 17 registry test files:

• document_read receives the redactor that carries the turn's ConfigStore secrets (mutant NR2);
• document_read shares the turn ledger with attachment_read (mutant NR1).

Slot release (workerRunner.ts:379,383,625, plus clearInterval). These paths are correct at HEAD, but their mutants survive:

• release on an abort between acquire and spawn;
• release on a synchronous spawn throw (e.g. ENOMEM);
• release in the pre-spawn catch.

A regression on any of these paths would leave the single reader slot held forever, so every later read hangs.

---

Low

1. One bad date cell fails the whole workbook (xlsxParser.ts:530-547). A single date-formatted number outside the JS Date range throws UnreadableCellError, and the whole workbook fails as document_parse_failed. For example, excelToDate(123456789012) is invalid, so a 12-digit ID column with a date format makes every sheet unreadable.
2. Host memory pressure labelled as heap (workerRunner.ts:521-529). When the Host's own working set nears memory.max, the child is killed and the model is told document_limits_exceeded/heap. host_memory_unavailable is the accurate label, and workerMemoryQueue.test.ts:259 pins the inaccurate one.
3. Pause decided on call totals (index.ts:708-741). The image_bytes pause is decided on call totals, not on the bound that failed. Run with the real child on the Codex profile: a page over the per-image cap pauses once (with an earlier 1 KB image), then fails after the reattach. One round trip is wasted.
4. Whole page shipped every call (pdfText.ts:125-131). Every text call sends the whole page over IPC. A 100 KB PDF with one 36M-character page always returns output_bytes and is never readable; a 26M-character page costs 11 s and 26 MB per call.
5. Page PNGs persisted to SQLite. They are written to the pending_approvals row, because projectGfsApproval strips only GFS images. The body says "in-memory".
6. Duplicate sheet names loop (index.ts:857). With duplicate sheet names (crafted files only), the continuation repeats one page forever: 8 calls made no progress.
7. Tool rejects its own cursor (selection.ts:34). The cursor allows 12 row digits, but the parser accepts larger rows. Row 1e13 gets a cursor that the next call rejects as invalid_range. Crafted files only; new since 0b9f5f7ce.
8. Foreign-namespace rows read as data (xlsxParser.ts:263-266). localName ignores the namespace, so foreign-namespace <foo:row>/<foo:c> elements are read as data: [[1],[],[3]] at e1fe20095 becomes [[1],[99],[3]] at HEAD. This is wider than M3 and undocumented; introduced by 0b9f5f7ce.
9. Redacted sheet names cannot be selected (index.ts:854-857, read-only). Sheet names are redacted in the output, but sheet needs the exact name.
10. skippedRows.first cut to 20 (index.ts:1049, read-only). count stays complete, while the tool description says skipped rows are listed.
11. Unannounced @napi-rs/canvas major bump (mcp-host/package.json). The ^0.1.65 → ^1.0.11 bump also affects workflow charts, fonts and embedded images, and visualInput/imageValidation. The body does not mention it; CI's full suite passed.
12. Flaky tests. The 150k-row RSS witness fails 9/31 at HEAD vs 0/20 at e1fe20095; bisected to 0b9f5f7ce, root cause not isolated, macOS under load. Two real-child tests time out at the 5 s default under load.
13. Other test gaps (the code is correct): the 1904 date system, the per-page PNG byte share in pdfRender.ts:126-130, the turn-memory branch of the pause decision, and the Linux-only workerRunner tests not pinned in mcp-host.json.
14. Cleanups:
    • The duplicate requestImages guard at index.ts:357 runs before argument validation (unreachable in production).
    • isEncryptedOoxml scans the full file twice for CFB files declared as xlsx/docx.

Text and contract (Low)

• "image_bytes always paused on Codex and Claude" is false for Claude. With default limits, row 7 is 21,000,000 > callImageBytes 20,971,520, so f41efbf68 changes nothing on Claude. The sentence appears in:

  • the PR body;
  • the commit f41efbf68;
  • the comment at index.ts:736-739;
  • the test comment at documentReadVisualBudget.test.ts:663;
  • VISUAL_BUDGET.md row 7.

  The Anthropic half of the new test also passes under the old comparison; only Codex had the loop.

• Stale header in index.ts:18-22. It says a secret cut by maxExtractedChars is masked only within each call. The code masks it on both sides, as documentReadTool.test.ts:898 shows.

• Stale or inaccurate text elsewhere:

  • types.ts:401: says "worker defect" for an unreadable cgroup.
  • types.ts:123: the meaning given for row_width.
  • The invalid_range message says mode is PDF-only, but XLSX accepts mode:"text".
  • visual_not_supported and the textless-page messages blame the model's vision when only the provider profile is missing.
  • image-runtime-smoke.cjs:76 describes an ink check that does not exist.
  • residentMemory.ts calls VmData "non-resident".
  • desktop-headful-journeys.md points at the closed feat(mcp-host): read PDF and XLSX attachments with clerum__document_read #1099.
  • MEMORY_MODEL.md says the pinned image is 24.16.0; it is 24.22.1.
  • VISUAL_BUDGET.md treats row 8 as Host-dependent; it is a constant 96 MiB.
  • The body says xlsxRowWidth.test.ts has 5 tests; it has 7 at HEAD.

• SIGTRAP is classified as heap since e5bbfc5c4; the body does not name the change.

Not assessed

• index.ts:351,360: ledger.beginRead() runs before runJob, so a worker defect, an abort or a host_memory_unavailable refusal spends one of the turn's reads. Is that intended?

---

PR body claims

Verified:

• the 77-file set, unchanged by the dev merge, and the clean ee95833cb merge;
• the 8 conflicts in 2d15076b5;
• HEAD's tests on e1fe20095 fail on every non-test-only finding (17);
• "4 of 5 fail on e7fa684", the uncached-formula test, and 58,926,114 > 32,000,000;
• zipLimits has 45 tests;
• the CI figures at ee95833cb;
• the 433/79 MiB arithmetic;
• the release-note compatibility statements;
• .d.ts matches .cjs, and the regenerated vectors are identical;
• admission refusals return host_memory_unavailable and log their condition;
• pdfjs has no eval path, XXE and entity expansion are blocked, and the child runs with env: {}.

False: the Claude half of the f41efbf68 row.

Partial:

• "the image smoke loads the reader": in-process only (M3);
• "in-memory pending approval": the PNGs are also persisted (Low 5);
• "R1-H1 231-301 ms": covers fit only (M2);
• "fake runners validate outcomes with isJobOutcome": 4 hand-built outcomes skip it, although all 4 are valid;
• the xlsxRowWidth test count.

Behaviour preserved across the delta (429de1d10..HEAD and e1fe20095..HEAD)

Assertions. Every removed or rewritten assertion maps to a named commit or finding.

Old tests on new code:

• e1fe20095 tests on HEAD: 5/305 fail, all explained by the body.
• 429de1d10 tests on HEAD: every failure is a shape change or a named behaviour change.
• Merges: no test was lost, and no package version is below the higher parent.

Old and new code on the same inputs:

• Parser (26 workbooks × 6 modes): only the named M3/M5/M6 rows change, plus Low 8.
• Row-probe grouping: 0/321 rows differ.
• Visual pause: only the Codex empty-request rows change.
• Admission: only the named rows change.

Verified clean (highlights)

Child process and outcomes:

• The slot is released on every path (exercised with the compiled runner).
• env: {} is applied, and an abort SIGKILLs the child and frees the slot.
• Malformed, empty and forged child outcomes are rejected.

XLSX values: 1900/1904 dates and the leap bug, rich text and phonetic runs, _x escapes, formulas, prefixed namespaces, and rows without r.

Continuation chains: 1,547 parser calls and 675 Tool calls, with 0 rows lost or duplicated.

Parsing safety:

• Zip guards count the real inflated bytes.
• Redaction runs before the cell cap, on cells, sheet names and PDF text.
• A cursor cannot select another attachment.
• Forged </tool_output> / <system> in document text is escaped.

Other suites: gfs-interaction-policy passes 140/140 (head tests on merge-base code fail 18 assertions, as expected), and the e2e provider fixture passes 54/54.

Pre-existing (not attributed to this PR): 2 Low.

• desktop-headful-journeys.md counts 14 specs, but the merge-base already has 15.
• The C17 pressure collapse skips document_read pages; the body already closes this as R1-M3.

Limitations

• macOS arm64 only. The Linux-only paths were read, not run, and CI evidence was used for them: cgroup v2, ulimit -d, /proc and oom_score_adj.
• Some timings were taken on a heavily loaded machine. The M2 XLSX figure was re-run on a lightly loaded one.
• Not run: the Desktop ComposerPanel tests, the Playwright spec, mcp-host tsc, and T2.

[alfredolopez80]

Independent review, round 2: 80d71c642 (fix verification + quick blind review of the delta)

Scope

• Head: 80d71c6422cb43bd85a7c48944d7ac428968296c.
• Delta: compared against the last reviewed head ee95833cb.
• Commits: six own commits (9c52cc3e0, 057e45cc8, 581c9279a, f34977657, 19ec7babf, 80d71c642) and two dev merges (a992742dc, 2e1f47022).
• Production files: taskExecutor.ts, documentRead/index.ts, package.json (+141/-54), plus the lock.

Method

• Lanes: three independent review lanes:
  • the approval resume;
  • the fit and row-probe rewrite;
  • contract, tests and merges.
• Behaviour preserved: the old and new code ran on the same inputs.
• Fixes: each claimed fix was checked by reverting it or by its mutant.
• Platform: macOS arm64; the Linux evidence comes from this PR's CI logs.

CI at 80d71c642: 63/63 checks pass.

Merge state: conflicting again. dev is now 0ca497d37, 61 commits past this branch's merge-base (05419da54). A trial merge conflicts in mcp-host/package.json and mcp-host/package-lock.json.

Status of earlier findings

Finding	Status	Evidence
M1: visual pages lost after an approval resume	Fixed (057e45cc8)	With the production hunk reverted, the M19 and M1 tests fail; at HEAD they pass (96/96). An old-vs-new table of the resume (23 rows × legacy/identity modes) shows only one change: other tools' images now reach the resumed request, which the body names as a decision. Adapters without image support withhold them with a notice, as the normal loop does.
M2: Host event-loop blocking	Fixed for the reported shapes; one case remains (19ec7babf)	Reviewer workbook: 874 → 21 measures, about 5.4 s → 0.27 s with a production-shaped measure (BasicSafety preview + cl100k). 400k-char PDF page: 21 → 6 to 9 measures. Remaining: a long sheet list where every row is too wide still costs about 4.2 s per call (703 measures), the same at ee95833cb and HEAD. Blanked rows never feed the new stop.
M3: the shipped child chain ran in no gate	Fixed (f34977657)	The smoke fails loudly on each of: a missing worker, a silent worker, an invalid outcome and a blank page. All 8 Host image CI jobs show the dist child ok with a non-null dataLimitKiB, so the Linux ulimit -d chain ran.
M4: security wiring and slot release unpinned	Fixed (581c9279a)	Each of the 6 mutants fails on its intended assertion, and every test has a liveness witness.
R1-H1 (Evenfire Reviewer): merge with dev	Fixed at the time, stale again	Both merge resolutions are clean: only the version lines differ from the automatic merge, no test was lost, and the 12 canvas lock entries are at 1.0.11. dev has moved since (see above).
R1-M1 (Evenfire Reviewer): retry promise	Fixed (9c52cc3e0)	Reverting the string turns the text test red. See N2 below.
Earlier Low findings	Mostly not fixed, as the body says	Fixed since then: the "ink check" text, the xlsxRowWidth count and the "in-memory" wording. @napi-rs/canvas is now named in the body. The rest are still present.

New findings in the delta (none blocking)

• N1 (Low): the aimed fit search can stop short (documentRead/index.ts:1197). The search assumes cost grows with n, but the production BPE measure is not monotonic: cost(1022)=485, cost(1023)=484.
  • In 11 of 6,565 production-measure calls (PDF pages with secrets and tags), HEAD emits 2-3 fewer characters than ee95833cb, which matches the brute-force maximum.
  • In 1 case HEAD emits a fitting emoji where ee95833cb returned the exhausted notice.
  • Nothing is lost, and the allowance is never exceeded.
  • The differential's "neither may change what a read returns" holds only under the bytes/4 measure.
• N2 (Low): the new message is too absolute for host_headroom. 9c52cc3e0's "This Host cannot start the reader" also covers this condition, which is computed per job from the live working set (documentMemory.ts:97-99,158-169). The condition is transient, so a later retry can succeed.
• N3 (Low): a repeated image can be sent twice after a cold resume (taskExecutor.ts). The dedup seed approval.attachments is not persisted to SQLite, while the completed_results images are. After a cold restore, a frame that repeats an earlier-turn image is sent again: 2 parts instead of 1 on a hot resume, and as a full part instead of sourceIdentityOnly in identity mode. The cold loader was not run end to end.
• N4 (Low, coverage): three resume mutations survive. Each passes all six resume suites (145/145), and a probe shows each changes behaviour:
  • the dedup seed replaced by [];
  • spillover_ref kept;
  • the identity flag forced to false.
• N5 (Low, text): the smoke comment overstates the ink check (mcp-host/test/image-runtime-smoke.cjs:103-104). The comment says the glyphs come from the shipped font data. With both standard_fonts paths in the dist pdfLoad.js broken, a fallback face still draws ink and the smoke still passes.
• N6 (Low, text): a test cites a commit that does not exist (documentReadFitDifferential.test.ts:9). The cited 5af3a16ce is not in the repository or on GitHub. The claim it supports does hold: the differential passes against the index.ts from before 19ec7babf.

Pre-existing (not caused by this delta)

• P1 (Medium): a second suspension leaves orphan tool messages (taskExecutor.ts:910-925). When a second suspension in the same turn carries attachments, the first batch's tool results are re-appended after the new assistant message as orphan tool messages. This is the same at ee95833cb and HEAD, and the code predates this PR.
  • The second approval's shape was built from reading the code.
  • Provider rejection was not executed.
• P2 (Medium): the M2 remainder above. About 4.2 s per call when every row is too wide, unchanged by the delta.

Verdict

• The four Medium findings and R1-M1 are fixed. M2 has a measured remainder (P2) at the same level as before.
• No new blocking issue was found in the delta.
• Before merge, two things remain:
  • dev must be merged again (conflicts in the mcp-host package files);
  • T2 / make minikube-run-document-upload has still not run on this head.
• For the author:
  • should host_headroom keep a retry hint (N2)?
  • should P1 and P2 get follow-up issues?

[jozer-rami]

Thanks for round 2. Each finding against the new head (6fff76dc3):

Finding	Status	Commit
Merge with dev	dev at 0ca497d37 merged; only the version lines conflicted (0.1.660).	c583c141a
N1 aimed fit vs non-monotonic BPE	Kept in code: the allowance always holds. The differential now says its "unchanged" claim holds under its byte-count measure, and that the production measure can settle a few characters either way.	c90f2fb77
N2 host_headroom wording	Now: "This Host could not start the document reader for this read: it had too little free memory, or it lacks a configuration the reader requires." No retry promise (R1-M1), no claim of permanence. The test also rejects "cannot".	7b5bffb4e
N3 cold-resume duplicate frame	Not fixed: the seed would need attachment metadata persisted with the approval row. Listed under Known limits.	—
N4 three surviving resume mutants	Tests for a repeated frame with and without source identity, and for rebuilt messages carrying no spillover ref (they become the next snapshot, and the cold loader expires an approval whose snapshot names a missing blob). Each mutant now fails. The spillover test pins the rebuild's existing behaviour; the loop's own append keeps the ref.	cd4cb5aa6
N5 smoke ink comment	Comment corrected: ink shows the page was drawn, not which face drew it.	c90f2fb77
N6 nonexistent commit cited	Now cites f34977657.	c90f2fb77
P1 orphan tool messages after a second suspension	Fixed in its own commit (it predates this branch, so it can move to a separate PR if preferred). The rebuild and the forwarded image count take only results the snapshot's last tool-call message still lacks. Red before the fix: call-capture and call-approve-1 re-sent after the new tool-call message, frame counted twice. A provider's response to that request was not run.	a7af8232d
P2 every row too wide	Blanked rows feed the stop; after 4 blanked rows in a row, groups are split without a probe until a row fits. A reconstruction of your shape (300 names, 600 too-wide rows; you measured 703 per call on your workbook): 1,340 → 516 measures (turn), 1,338 → 709 (fresh); production BPE about 3 s → 1.3 s per call here. Each [] row still needs one exact probe, so the remaining cost is the floor for unchanged results. The shape is in the differential: 20/20 chains equal f34977657 under its byte-count measure; the production measure was not compared (fixture ids change its counts per run).	9d78c9d7e, 6fff76dc3

T2 has still not run on this head.

[alfredolopez80]

Independent review, round 3: 6fff76dc3 (fix verification + quick review of the delta)

Scope

• Head: 6fff76dc3f1a3404a1ab7d399c618261511a83dd.
• Delta: compared against the last reviewed head 80d71c642.
  • Six own commits: 7b5bffb4e, c90f2fb77, cd4cb5aa6, a7af8232d, 9d78c9d7e, 6fff76dc3.
  • Production lines, without version bumps: taskExecutor.ts +34/-4 and documentRead/index.ts +34/-12. Tests: +289/-15.
• Dev merge c583c141a: git show --remerge-diff shows that the resolution touches only the mcp-host version lines (0.1.660), and no mcp-host dependency changed.
• Method: two lanes, the approval resume and documentRead.
  • The old and new code ran on the same inputs.
  • Each claimed fix was checked by reverting it or with its mutant.
  • Platform: macOS arm64.
• CI at 6fff76dc3: 63/63 checks pass.

Status of round-2 findings

Finding	Status	Evidence
P1: second suspension	Fixed when tool-call ids are distinct; see A-1 (a7af8232d)	At 80d71c642, every second-suspension shape sends the first batch's results as orphan tool messages and counts the frame twice. The shapes checked: a mixed batch, cold (SQLite) resumes, connect_required and stopTurn. At HEAD every tool message answers an open call, and the frame is counted once. Reverting either site of the fix turns the P1 test red (352/353).
N4: three resume mutants	Fixed (cd4cb5aa6)	seed-empty fails 2 tests, keep-ref fails 1 and identity-false fails 1. Each failing test has a liveness witness.
N3: cold-resume duplicate frame	Not fixed, as stated	Reproduced through a real on-disk SQLite store and the cold loader. In legacy mode a hot resume sends 1 image and a cold resume sends 2. In identity mode the cold resume sends the repeat in full. HEAD behaves the same as 80d71c642.
N2: host_headroom wording	Fixed (7b5bffb4e)	The new text is true for all four refusal causes. Restoring the old string turns the test red.
N1: aimed fit vs non-monotonic BPE	Unchanged in code; the text now matches (c90f2fb77)	A production-measure sweep finds 5 allowances where the result stops 2–3 characters short, at both heads. It never exceeds the allowance.
N5: smoke ink comment	Fixed (c90f2fb77)	With both standard_fonts paths in the dist pdfLoad.js broken, the page still renders ink, which matches the new comment. Checked on macOS only.
N6: nonexistent commit cited	Fixed (c90f2fb77)	f34977657 exists and reproduces all 20 pinned digests. All 42 SHAs cited in the PR body exist.
P2: every row too wide	Halved, not removed (9d78c9d7e)	Round 2's shape: 1,300 sheet names, 430 rows of 900 characters, production measure (BasicSafety preview + cl100k), 8 runs per side in separate processes. 80d71c642: 703 measures, median 4,360 ms per call. HEAD: 374 measures, median 2,219 ms. The shapes fixed in round 2 did not regress: the reviewer workbook takes 21 measures and the 400k-char PDF page 6–10. Reverting the hunk turns the <= 750 test red.

Behaviour preserved:

• documentRead: 80d71c642 and HEAD return identical results on 652 read chains, under both the byte-count and the production measure. The new dense path ran 1,220 times.
• Resume: the only changed verdicts are the P1 rows, plus one row that no producer can create.
• Tests: the earlier tests pass on HEAD. For documentRead, the only failure is the expected N2 text.

New findings

A-1 (Medium, pre-existing): P1 still occurs with drivers whose tool-call ids restart on every response (taskExecutor.ts:327-340, used at :845-856 and :921).

• The id clash:
  • The Gemini driver numbers synthetic ids from call_0 on every response (googleGenerative.ts:160-170). Its comment says they are unique only within one assistant turn.
  • unansweredBatchResults identifies the earlier batch by tool-call id. On a second suspension, the earlier batch's call_0 result therefore counts as an answer to the new call_0.
• Reproduction: the real driver with a stubbed client. The resumed request carries functionResponse some_tool "OLD screenshot result" before "NEW approved result". 80d71c642 is worse: it also leaves call_1 as an orphan.
• Root cause: the accumulation at :941-953 (on dev before this branch) keeps the earlier batch in completed_results, while the snapshot already holds that batch's messages. A tool-call id cannot identify the batch for this driver. Marking which results belong to the current batch would close it.
• Not run: a real Gemini endpoint.

Low (coverage):

• B-1 (9d78c9d7e, index.ts:1013): no test pins the half "blanked rows feed the early stop". With the decided(...) call removed, every test passes.
  • It saves measures only on the turn chain (707 → 516). It saves none on the fresh chain or on the P2 shape (374 either way).
  • The likely reason, inferred from the code and not isolated: the floor smallest (:1043-1062) leaves out the skippedRows field that every emitted result carries.

Low (text):

• A-2 (taskExecutor.ts:908-911): the comment says the resolution drops the spillover ref. The resolution keeps it (:778-783); the explicit spillover_ref: undefined map at :921-924 drops it.
• A-3 (taskExecutor.ts:320-324, a7af8232d): the comment says completed_results keeps the earlier batch. It is kept only when the turn has attachments (:941).
• B-2 (index.ts:994-998): "blanked rows in a row … until a row fits again" does not match the counter. Only a fitting single-row probe resets it (:1015); rows accepted inside a group do not.
• B-3 (index.ts:999-1003): "the byte-count differential pins this" overstates what the differential covers. The pinned scenarios never reach a fitting row in dense mode. The reason given for not comparing the production measure also does not hold: the fixture ids do not prevent the comparison, which ran here on 652 chains.
• B-4 (fitScenarios.ts:256): for the new scenario the page is about 0.56 of a row past the list, not "about 1.5 rows".
• B-5 (documentReadFitDifferential.test.ts:47-48): the "1,340 measures" figure comes from this fixture; round 2 measured 703 on its own workbook.

Info:

• B-6: the fallback face that draws ink comes from the canvas library, not from pdf.js.
• B-7 (documentReadFitDifferential.test.ts:13-14): one more character can cost up to 2 tokens less, not only 1.
• B-8: a few mixed wide-row patterns take up to 4.4% more measures. The net change is about −8%.

Verdict

• The delta contains no blocking issue. N2, N4, N5 and N6 are fixed, and P1 is fixed for providers whose tool-call ids are unique across responses.
• Before merge:
  • T2 / make minikube-run-document-upload has still not run on this head.
  • P2 still costs about 2.2 s of event-loop time per call when every row is too wide.
• For the author:
  • Fix A-1 here, or in the separate PR you offered for P1?
  • Is the P2 remainder accepted, or does it get a follow-up issue?

[jozer-rami]

Thanks for round 3. A-1 is fixed here, in 367dd959e:

• Fix: results carried from an earlier batch are marked answeredInSnapshot when the resume carries them (the accumulation you pointed at). The next resume skips marked results instead of matching tool-call ids, and projectResult keeps the mark, so it survives the GFS projection and the SQLite row. The id filter remains only for approvals saved before the mark existed.
• Tests:
  • A two-suspension turn with Gemini-style ids (call_0, call_1, then call_0). Before the fix, the earlier batch's Captured result was sent again after the new tool-call message, and the frame was counted twice.
  • The mark surviving suspendForApproval, the projection of a GFS-image result, and reconstructPendingApproval. This test fails without the projectResult change.
  • Removing either taskExecutor site alone turns the first test red.
• Not run: the real Gemini driver or endpoint; the test reproduces the driver's documented id scheme with the loop mocked.
• A-2 and A-3 are corrected in the same commit, since they describe the lines it changes.
• B-1 to B-8 are not changed in this push.

On your questions: A-1 is fixed in this PR, next to P1. The P2 remainder (your 2,219 ms median per call) is listed under Known limits; whether it is accepted or gets a follow-up issue is still open. T2 has still not run on this head.