Skip to content

Optional AI review, and which models to use at what cost - #5

Merged
bbertucc merged 9 commits into
mainfrom
ai-review
Sep 24, 2026
Merged

bbertucc merged 9 commits into
mainfrom
ai-review

Conversation

@bbertucc

Copy link
Copy Markdown
Member

Adds iris-pdf review, an optional AI check of a tagged PDF, and docs/models.md, which covers which model to use for each AI task in this project and what each costs.

Review

veraPDF checks that the tags are valid PDF/UA. It can't tell whether they say what the page says. review sends a Claude model each page's image and what a screen reader gets from that page: the structure, text, Alt, link targets, field names, the document language, and the headings on earlier pages. The model reports these findings:

  • missing content
  • reading order
  • element types and heading levels
  • tables
  • alt text
  • link text
  • field names
  • language

The command prints the findings, writes them to --report as JSON with token usage and an estimated cost, and exits 0. It never changes the PDF.

  • Providers. It uses the Anthropic API (plain fetch) when ANTHROPIC_API_KEY is set, and otherwise Bedrock through the AWS CLI, which supplies credentials, region and retries. No new npm dependencies.
  • Model. The default is Opus 5.5, at about $0.03 a page.
  • Tool calls. tool_choice stays auto, because Opus 5.5 rejects a forced tool. A model that answers in text is asked once more, and if it still doesn't call the tool, the review fails with review_failed.
  • Privacy. It sends page images and text to the provider. The README says so.
  • Code moves. The structure reader moves from test/helpers.ts to src/pdf/read.ts, and helpers re-exports it.

Models (details in docs/models.md)

Measured on Bedrock us. profiles against 8 seeded defects, 3 clean pages, and a real 25-page scanned report:

Model Seeded found Clean-page findings 25 pages
Opus 5.5 8/8 0 $0.72
Sonnet 5 8/8 0 $0.47
Haiku 4.5 8/8 4 $0.14

Both Opus and Sonnet found real problems. For example, on the report's contents pages the tagger keeps unmatched OCR dot-leader text as paragraphs, and that is a follow-up for the tagger.

The PR reviewer's last 18 runs on Opus 5 averaged $1.44 each. The doc recommends Opus 5.5, which has a 20% lower list price. Switching needs a change to the shared IAM role, so this PR leaves the workflow as it is.

Tests

test/review.test.ts uses a stubbed send and checks:

  • the request shape: the image, the outline, no forced tool;
  • parsing, and how unknown kinds and severities are normalized;
  • the second ask when the model answers in text, and the failure after it;
  • refusal of untagged PDFs, and missing credentials;
  • per-page outlines and the headings from earlier pages;
  • the truncation marker;
  • cost arithmetic.

A CLI test checks exit 3 for a bad provider and for an untagged PDF. network.test.ts still shows that tagging is offline.

🤖 Generated with Claude Code

iris-pdf review sends each tagged page's image and its screen-reader outline
to a Claude model (Anthropic API, or Bedrock through the AWS CLI) and reports
what a blind reader would miss or get wrong. Report-only; no new dependencies.
The structure reader moves from the test helpers to src/pdf/read.ts.

docs/models.md records the models measured for review, the PR reviewer's
costs, and the recommended models.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All checks pass. Three blocking issues, all in code this PR promotes from test-only to
production and points at arbitrary user PDFs. review opens with readOnly: true, so
openPdf returns at src/pdf/document.ts:43 before every structural refusal: any file
with a /StructTreeRoot reaches the new walkers.

1. A cyclic structure tree crashes review with a stack overflow — src/pdf/read.ts:60-79

structTree's visit recurses into every non-MCR/OBJR child of /K with no depth or
visited guard:

      } else {
        const kid = visit(x);

Input that reaches it: StructTreeRoot /K → a Document element whose /K is a Sect
whose /K points back to Document. Built that PDF against this branch and called
structTree:

THREW: RangeError Maximum call stack size exceeded

RangeError is not an IrisPdfError, so src/cli.ts:119 rethrows it: the CLI dies with a
stack trace and exit 1 instead of iris-pdf: <code>: <message>. src/report.ts:60 states
"Never a crash, never a silent degradation", and #4 added exactly this guard for a cyclic
/AP; forEachField and inherited (src/pdf/document.ts:74,88) both cap at depth 32.
This walk needs the same cap, plus a seen-set so a diamond does not blow up exponentially.

2. An attacker-chosen /ToUnicode bfrange hangs or crashes review — src/pdf/read.ts:15-18

      for (let g = hex(a), u = hex(c); g <= hex(b); g++, u++) map.set(g, String.fromCodePoint(u));

Both bounds come straight from the file. beginbfrange <0000> <ffffffff> <0041> iterates
~4e9 times building a Map — unbounded time and memory (and hex(b) is re-parsed every
iteration). A short range with a high destination throws instead. Verified end-to-end
through structTree on a PDF with a /StructTreeRoot, a P with /K 0, and a page stream
<</MCID 0>> BDC /F9 12 Tf <0041> EMC whose /F9 has <0000> <0010> <10ffff>:

THREW: RangeError Invalid code point 1114112

Another non-IrisPdfError crash, driven entirely by the input file. Cap the range length and
the code point.

3. review has no page limit, so one command makes unbounded model calls — src/review/review.ts:84,90

tag refuses over MAX_PAGES = 25 (src/pdf/document.ts:52-55), but review skips that
check and then builds one request per page:

  const requests = Array.from({ length: doc.countPages() }, (_, i) => {

Input: any tagged PDF with 2,000 pages — an Acrobat-tagged file passes the not_tagged gate.
That is 2,000 Claude requests at the documented ~US$0.03 a page, about US$60, with no
ceiling, no confirmation and no --max-pages. Worse, requests is built before the first
await, so every page is rendered and base64-encoded up front and all images are held at
once (measured 30–53 KB per fixture page, much more for a dense full-colour scan), so a long
document exhausts memory before a single request goes out. Refuse over MAX_PAGES like the
rest of the tool, and render each page inside its worker.

Non-blocking notes

  • src/review/review.ts:99-104: if the reply that fails reported() contains a tool_use
    block (a mis-named tool, or input.findings missing after a max_tokens truncation),
    said carries that block into the new assistant turn with no tool_result after it, which
    the Messages API refuses. The recovery then fails as review_failed with a confusing API
    error. Strip tool_use blocks from said, or resend the original request.
  • stop_reason is never checked (src/review/review.ts:149): a reply truncated at
    max_tokens: 4096 whose partial findings array still parses is accepted as a complete
    page review, so findings are dropped silently. Reachable on a page with many findings.
  • src/review/outline.ts:16: the outline shows Scope but never /Headers or /ID, though
    src/tag.ts:293 writes them and the prompt asks the model to report "header cells missing or
    wrong". A TD whose /Headers names the wrong TH is invisible to the review.
  • A PDF tagged by another tool whose StructTreeRoot /K is an array (the common shape) gives
    an empty tree — verified: root type="" kids=0 outline="" — so every page is sent as
    "(no tagged content)" and the model reports the whole document as missing content. Nothing
    checks that the tree is this tool's; refuse or warn rather than bill for a bogus review.
  • Privacy: with tag --flatten the field values become tagged text, so they are in the outline
    sent to the provider and can come back quoted in a finding, which review prints and writes
    to --report. The Privacy section still says form values are "never printed, logged, or put
    in the report"; the PR amends it only for page images.

Accessibility impact: none on the tagged PDF — review writes nothing — but on an untrusted
tagged file the new reader crashes instead of reporting, and it is blind to /Headers, so
wrong table-header associations pass the review it claims to check.

The structure reader stops at cycles and refuses nesting past 64 levels;
/ToUnicode ranges over 256 codes and invalid code points are skipped.
review refuses over 25 pages and trees with no readable text, renders each
page in its worker, fails on max_tokens, and re-asks with text only.
The outline shows ID and Headers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three blocking issues from my review of 9108e71 are fixed in dd88f28: the structure walk now has a seen set and MAX_DEPTH (bad_structure), bfrange is capped at 256 codes with a code-point check, and review refuses over MAX_PAGES and renders each page inside its worker. All five non-blocking notes are addressed too (tool_use stripped from the retry, stop_reason: max_tokens rejected, /Headers and ID in the outline, a root /K array read, the Privacy section amended for --flatten). Checks all pass.

One new blocking issue, introduced by the fix commit.

review refuses this tool's own output when the document is image-only — src/review/review.ts:89

if (!readingOrder(root)) throw new IrisPdfError("no_readable_structure", "The structure tree has no text this tool can read. Only PDFs tagged by iris-pdf can be reviewed.", EXIT.badInput);

readingOrder is the concatenated text of the tree. A page whose HTML is an image with alt text produces Document > Figure with /Alt and no marked-content text (src/tag.ts:300-303 emits an empty Figure marked content), so a document made only of such pages — a tagged chart, poster, map or photo page — has no text at all and is refused.

Input that reaches it, against this branch: a one-page PDF that draws a rectangle, tagged with {"html": "<img alt=\"A bar chart of permit fees by zone\">"}:

warnings [] structure {"elements":2,"byType":{"Document":1,"Figure":1}}
root Document kids [["Figure","",[0]]] readingOrder=[]
THREW no_readable_structure 3 The structure tree has no text this tool can read. Only PDFs tagged by iris-pdf can be reviewed.

tag warns nothing; the file is fully tagged by this tool. pageOutline renders it correctly (Figure Alt="A bar chart …"), so the review would work — only this gate stops it, with exit 3 and a message telling the user their PDF was not tagged by iris-pdf. The documents lost are the ones whose only accessibility risk is alt text, one of the eight things review exists to check. Gate on something that shows the tree is ours and readable — any marked content, a Figure/OBJR element, or the ParentTree — not on non-empty text. The test at test/review.test.ts:137 ("no text this tool can read") passes an empty /K array, which the corrected gate should still refuse.

Non-blocking notes

  • src/review/outline.ts:27: a Link whose annotation has no /A at all prints href=(in this document), the same as a working GoTo link. tag tags such an annotation as a Link (src/tag.ts:228), so a link that goes nowhere is described to the model as an in-document link, and the prompt's "link text that does not say where the link goes" cannot be judged for it. Internal destinations are never shown at all.
  • src/review/review.ts:111 with src/cli.ts:75-80: one page's bad reply throws review_failed for the whole run, and the CLI writes --report only on success — a user who has already paid for 24 pages gets no findings and no usage or cost figure.
  • No timeout on either provider call (fetch, src/review/review.ts:179; execFile, :198): a hung provider hangs the command with no ceiling. The retry loop bounds 429/5xx but not a stalled connection.
  • Prompt injection: page text goes into the prompt verbatim, so a document can instruct the model to report nothing. Nothing acts on the findings, but a suppressed review looks the same as a clean one.

Accessibility impact: review writes nothing, so the tagged PDF is unchanged — but an image-only tagged document, where alt text is the whole accessibility question, cannot be reviewed at all.

The readable-structure gate now asks for content on a page, not text, so
a tagged figure-only document is reviewed. A page the model fails is
reported with its error and exit 1; the others and the usage are kept.
Calls time out after 3 minutes. The outline says where a link goes, or
that it goes nowhere, and no longer crashes on a Link without /A. The
prompt tells the model not to follow instructions in the document.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The image-only refusal from my review of dd88f28 is fixed: the gate is now content(root) on pages, with a test that reviews a figure-only document. The four notes there are addressed too (link actions distinguished, per-page failure kept with exit 1 and the report written, a timeout on both providers, an anti-injection line in the prompt and in the README limits). All checks pass.

One blocking issue: the /ToUnicode hardening from dd88f28 capped the wrong dimension.

A 2 KB crafted PDF crashes review with a stack overflow — src/pdf/read.ts:16

  const str = (h: string) => String.fromCodePoint(...(h.match(/.{4}/g) ?? []).map(hex).filter(valid));

if (hi - lo > 255) continue; bounds how many mappings a bfrange makes, but nothing bounds the length of a single destination string. \w+ in /<(\w+)>\s*<(\w+)>/ matches a destination of any length, and all of it is spread into String.fromCodePoint, which overflows the stack past ~100k arguments (measured: 50,000 fine, 200,000 throws).

Input that reaches it, built against this branch and saved with compression — 1,980 bytes: a page whose stream is <</MCID 0>> BDC /F1 12 Tf <0001> Tj EMC, a /StructTreeRoot whose P has /K 0 and /Pg that page, and /F1's /ToUnicode holding beginbfchar <0001> <0041×250000> endbfchar.

$ node src/cli.ts review --pdf /tmp/evil.pdf --provider bedrock
src/pdf/read.ts:16
  const str = (h) => String.fromCodePoint(...(h.match(/.{4}/g) ?? []).map(hex).filter(valid));
RangeError: Maximum call stack size exceeded
    at str (src/pdf/read.ts:16:37)
    at toUnicode (src/pdf/read.ts:18:81)
    at mcidText (src/pdf/read.ts:43:43)
    at textOn (src/pdf/read.ts:66:65)
exit=1

RangeError is not an IrisPdfError, so src/cli.ts:121 rethrows it: a stack trace and exit 1 instead of iris-pdf: <code>: <message>, against src/report.ts:60 ("Never a crash, never a silent degradation"). Cap the destination length, or build the string without a spread. test/review.test.ts:200 covers over-long ranges and invalid code points, not an over-long destination.

Non-blocking notes

  • src/review/outline.ts:24-38: the outline never shows an annotation's /Contents, though src/tag.ts:235,326 writes it and PDF/UA-1 requires it — it is what AT announces for the annotation. A link whose /Contents is tag's fallback "Link to another part of this document", or disagrees with the link text, is invisible to a review that lists link among the eight things it checks. Internal GoTo destinations are still shown only as (in this document).
  • src/review/review.ts:104: const req = build(i) sits outside the per-page try, so an error from rendering or outlining one page (not an IrisPdfError) aborts the whole run with a raw error and no report, after the earlier pages are paid for. I found no input that reaches it — mupdf normalises [0 0 0 0] and clamps a 1,000,000-unit MediaBox, so image() held up. Moving build(i) inside the try would make such a page review_failed like every other failure.
  • Unchanged since 9108e71: nothing checks the tree is this tool's. A PDF tagged by another tool passes the new content() gate (its MCIDs carry /Pg), but mcidText's two regexes only decode our overlay (<</MCID n>> BDC plus /F 12 Tf <hex>), so every element's text comes back empty and the user pays for a review that reports the whole document as missing content — while the refusal message claims "Only PDFs tagged by iris-pdf can be reviewed."
  • src/cli.ts:78: f.element and f.detail are printed raw. quote() escapes document text on the way into the prompt, but a finding that quotes the page back can carry ANSI escapes into the user's terminal.

Accessibility impact: none on the tagged PDF — review writes nothing — but on an untrusted tagged file the reader still crashes instead of reporting, and the review is blind to the annotation descriptions PDF/UA-1 requires, so a link that announces the wrong thing passes it.

A long bfchar destination was spread into String.fromCodePoint and could
overflow the stack.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f9667cf: the over-long /ToUnicode bfchar destination from my review of 431d2a9 is capped at 1024 hex chars, with a test. The first of that review's notes (annotation /Contents in the outline) is addressed too; the other three are unchanged. All checks pass.

One blocking issue. It is the third instance of the same class, and the reason is that each round caps one field instead of guarding the walk.

review still dies with a raw error and a wasm stack trace on a 973-byte crafted PDF — src/pdf/read.ts:14

  const cmap = font.get("ToUnicode").readStream().asString();

Nothing checks that /ToUnicode is a stream. Input that reaches it, built with mupdf against this branch and saved compressed — 973 bytes: one page whose stream is <</MCID 0>> BDC /F1 12 Tf <0041> Tj EMC with /F1 a /Type1 /Helvetica font and no /ToUnicode, plus a /StructTreeRoot whose P has /K 0 and /Pg that page.

$ node src/cli.ts review --pdf /tmp/nofont.pdf --provider bedrock
Error: object is not a stream
    at toUnicode (src/pdf/read.ts:14:38)
    at mcidText (src/pdf/read.ts:44:43)
    at textOn (src/pdf/read.ts:67:65)
exit=1

Four more shapes of malformed tree reach a raw error in the same walk. Verified by calling structTree directly on each:

Input Thrown
page has no /Font resource, stream still has /F1 12 Tf <…> (src/pdf/read.ts:35,44) TypeError: Cannot read properties of null (reading '_fromPDFObjectKeep')
element with /K 0 and no /Pg (src/pdf/read.ts:67,76) TypeError: Cannot read properties of null
/Contents a plain dictionary, or an array holding one (src/pdf/read.ts:38-39) Error: object is not a stream
MCR dict with no /MCID (src/pdf/read.ts:77) TypeError: Cannot read properties of null

None is an IrisPdfError, so src/cli.ts:121 rethrows: exit 1 with a minified mupdf stack trace instead of iris-pdf: <code>: <message>, against src/report.ts:60 ("Never a crash, never a silent degradation"). openPdf(…, { readOnly: true }) returns before every structural refusal, so any file with a /StructTreeRoot reaches this.

Per-field caps will keep missing cases; one guard ends the class. Catch anything that is not an IrisPdfError around structTree in src/review/review.ts:90 and report it as unreadable, and add isStream()/isNull() checks in toUnicode and mcidText. test/review.test.ts:200 covers over-long ranges, over-long destinations and invalid code points — nothing covers a font without /ToUnicode, a missing /Pg, or a /Contents that is not a stream.

Non-blocking notes

  • src/review/review.ts:104: const req = build(i) is still outside the per-page try, so a render or outline error on one page aborts the whole run with a raw error and no report, after the earlier pages are paid for. Same class as above; with structTree guarded this is the last unguarded mupdf call on the review path. Raised on 431d2a9 and unchanged.
  • Unchanged since 431d2a9: an internal GoTo destination is still shown only as href=(in this document), so the review cannot judge whether an in-document link goes anywhere useful.
  • Unchanged since 9108e71, raised three times, so leaving it to a human: nothing checks the tree is this tool's. A PDF tagged by another tool passes the content() gate but mcidText's two regexes decode none of its text, so every element comes back empty and the user pays for a review that reports the whole document as missing content — while the refusal message says "Only PDFs tagged by iris-pdf can be reviewed."
  • src/cli.ts:78: f.element and f.detail are printed raw, so document text the model quotes back can carry ANSI escapes into the terminal.

Accessibility impact: none on the tagged PDF — review writes nothing — but on an untrusted or third-party-tagged file the reader still crashes with a stack trace instead of reporting, so the accessibility check the PR adds cannot be run on those files at all.

Any error reading the tree is now `unreadable` (exit 3), and any error
building one page's request fails only that page. Fonts without
/ToUnicode, pages without /Font, /Contents that is not a stream and
elements without /Pg read as unknown text instead of throwing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 06a1cae: the unguarded mupdf calls from my review of f9667cf — structTree is wrapped as unreadable, and toUnicode/mcidText/textOn/on check isDictionary/isStream/isIndirect. build(i) is now inside the per-page try as well. Checks all pass.

One blocking issue, in the same /ToUnicode parser. It is the fourth instance of this class, and the new whole-walk guard cannot catch it.

A 962-byte PDF makes review spin forever — src/pdf/read.ts:24-26

const [lo, hi, u] = [hex(a), hex(b), hex(c)];
if (hi - lo > 255) continue;
for (let g = lo; g <= hi; g++) if (valid(u + g - lo)) map.set(g, String.fromCodePoint(u + g - lo));

hex is parseInt(h, 16) and /<(\w+)>/ matches a code of any length, so a code of ~256 or more hex digits parses to Infinity. With lo === hi === Infinity: hi - lo is NaN, NaN > 255 is false so the cap does not fire, Infinity <= Infinity is true, and g++ leaves g at Infinity. The loop never ends.

Input that reaches it, built with mupdf against this branch and saved compressed — 962 bytes: one page whose stream is <</MCID 0>> BDC /F1 12 Tf <0041> Tj EMC, /F1 a font whose /ToUnicode holds beginbfrange <F×300> <F×300> <0041> endbfrange, and a /StructTreeRoot whose P has /K 0 and /Pg that page.

$ timeout 45 node src/cli.ts review --pdf /tmp/evil.pdf --provider bedrock
exit=124     # still running, 100% CPU, no output

Verified separately that mcidText on that page never returns (killed at 20 s).

The guard added in this commit does not help: a hang throws nothing, so the try around structTree never runs, TIMEOUT_MS covers only provider calls, and there is no watchdog. The result is worse than the three crashes it replaced — no error line, no exit, no report, against src/report.ts:60 ("Never a crash, never a silent degradation").

Fix the parse rather than the loop: reject a code longer than 8 hex digits (PDF 9.7.6.2 allows at most a 4-byte code), or require Number.isInteger(lo) && Number.isInteger(hi) before looping.

test/review.test.ts:200 covers an over-long range by value (<0000> <FFFFFFFF>), an over-long destination and invalid code points — nothing covers a code string long enough to parse as Infinity.

Non-blocking notes

  • src/pdf/read.ts:15: font.get("ToUnicode").readStream().asString() is unbounded, and deflate compresses this kind of text ~1000:1, so a small PDF can produce a very large cmap string before any per-field cap applies. Same untrusted-input path; capping the cmap length (and the code length above) would end the class instead of one more instance.
  • src/review/outline.ts:29: an internal GoTo destination is still shown only as href=(in this document), so the review cannot judge whether an in-document link goes anywhere useful. Raised on 431d2a9 and f9667cf and pushed past; leaving it to a human.
  • src/cli.ts:78: f.element and f.detail are still printed raw, so document text the model quotes back can carry ANSI escapes into the user's terminal. Raised twice.

Accessibility impact: none on the tagged PDF — review writes nothing — but on an untrusted tagged file the reader now hangs silently instead of reporting, so the accessibility check this PR adds cannot be run on those files at all.

…indings

A code of hundreds of hex digits parsed as Infinity and the bfrange loop
never ended. Codes are now at most 4 bytes and a CMap at most 1 MB.
Findings quote the document, so the CLI prints them without control
characters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in aec3e21: the Infinity bfrange hang from my review of 06a1cae — codes and
destinations are now matched as bounded hex ({1,8} / {1,1024}), the CMap is capped at 1 MB,
and both have tests. The ANSI-escape note is addressed too (plain() in src/cli.ts:78,80,
with a test). All checks pass.

One blocking issue. It is the fifth instance of the same class, and it is in the other half of
the same reader: the page content stream, which has no cap at all.

A 8.9 KB crafted PDF makes review spin for two minutes per page — src/pdf/read.ts:41,45

    if (s.isStream()) streams.push(s.readStream().asString());
  }
  const all = streams.join("\n");
  for (const [, id, body] of all.matchAll(/<<\/MCID (\d+)>> BDC([\s\S]*?)EMC/g)) {

Two unbounded things meet. /ToUnicode is capped at 1 MB (read.ts:17) but the page's
/Contents is read and stringified with no limit, and the ([\s\S]*?)EMC scan is quadratic
when EMC never appears: for each of the N <</MCID n>> BDC starts the engine walks the rest
of the string looking for EMC, so the cost is N × length.

Input that reaches it, built with mupdf against this branch and saved with compress — one
page whose content stream is "<</MCID 0>> BDC ".repeat(n) with no EMC, and a
/StructTreeRoot whose P has /K 0 and /Pg that page:

/Contents after inflation PDF on disk mcidText
1 MB 2,773 bytes 7.4 s
4 MB 8,869 bytes 121 s
$ time timeout 300 node scratch/t3.ts     # mcidText on the 8,869-byte PDF's page
mcidText ms: 121294
real  2m1.412s

Quadratic, so 16 MB (~35 KB on disk) is ~32 minutes and 64 MB (~140 KB) is hours, per page, on
25 pages. Nothing interrupts it: the walk is synchronous, TIMEOUT_MS covers only provider
calls, and the try around structTree in src/review/review.ts:90 never runs because
nothing is thrown. No error line, no exit, no report — the same outcome as the 06a1cae hang,
against src/report.ts:60 ("Never a crash, never a silent degradation").

Cap all.length the way the CMap is capped, and match the body without a backtracking scan
(split on EMC, or (?:(?!EMC)[\s\S]) bounded). test/review.test.ts:199-231 covers CMap
size, range and destination limits and a /Contents that is not a stream — nothing covers the
size of a content stream that is one, or a BDC with no EMC.

Non-blocking notes

  • src/review/outline.ts:29: an internal GoTo destination is still shown only as
    href=(in this document), so the review cannot judge whether an in-document link goes
    anywhere useful. Raised on 431d2a9, f9667cf and 06a1cae and pushed past each time;
    leaving it to a human.
  • Unchanged since 9108e71, raised four times, also for a human: nothing checks the tree is
    this tool's. A PDF tagged by another tool passes the content() gate but mcidText's two
    regexes decode none of its text, so every element comes back empty and the user pays for a
    review that reports the whole document as missing content — while the refusal message says
    "Only PDFs tagged by iris-pdf can be reviewed."

Accessibility impact: none on the tagged PDF — review writes nothing — but on an untrusted
tagged file the reader still hangs silently instead of reporting, so the accessibility check
this PR adds cannot be run on those files at all.

…nnot decode

A BDC with no EMC made the lazy match scan the rest of the stream for
each start. Page content is now scanned once and capped at 32M characters.
review refuses a tree whose marked text all decodes empty (tagged by
another tool) instead of paying for a review of nothing. An internal
link shows its target page or named destination.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a91d25a: the quadratic BDC-with-no-EMC scan from my review of aec3e21. The 8,681-byte repro (4 MB content stream, no EMC) that took 121 s now reads in 44 ms, and 96 MB of BDC tokens (187 KB PDF) in 368 ms. Two long-standing notes are closed too: a tagged tree whose marked text this tool cannot decode is now refused as no_readable_structure, and an internal link shows its target page or named destination. All checks pass.

I ran review with a stubbed send over all 11 tagged fixtures (text-simple, text-embedded, text-two-column, structure, links, form-acroform, cjk, blank-page, mixed, scan-300dpi, scan-skewed): none is wrongly refused by the new gate and no page errors, so the decode requirement does not reject this tool's own output, including CJK.

No blocking issue.

Non-blocking notes

  • ColSpan and RowSpan never reach the model — src/review/outline.ts:27-34. props() reads only Scope and Headers out of /A, but the tagger writes spans (src/html/build.ts:200-201: if (cs > 1) a.ColSpan = cs;). Reachable with ordinary input — any HTML table with a merged cell. Tagging <table><tr><th id="a">Zone</th><th colspan="2">Fees</th></tr><tr><td headers="a">North</td><td>10</td><td>20</td></tr></table> and printing pageOutline:

    Table
      TBody
        TR
          TH ID=\"p1-th1\" Scope=Column \"Zone\"
          TH ID=\"p1-th2\" Scope=Column \"Fees\"
        TR
          TD Headers=\"p1-th1\" \"North\"
          TD Headers=\"p1-th2\" \"10\"
          TD Headers=\"p1-th2\" \"20\"
    

    The model sees a 2-cell header row over a 3-cell body row with nothing to explain it, against a page image that shows a spanned header: it will report a table finding that is not a defect, and it cannot see a wrong or missing span either. The PDF itself is correct; only the review's view of it is short.

  • The content-stream cap is applied after the allocation — `src/pdf/read.ts:41-45.

        if (s.isStream()) streams.push(s.readStream().asString());
      }
      const all = streams.join(\"\\n\").slice(0, MAX_CONTENT);

    MAX_CONTENT bounds the regex work, not the read: the whole inflated stream is stringified first. Measured on a crafted PDF, one page, /Contents of \"x\".repeat(n) after a tagged BDC…EMC, saved with compress: 98 KB on disk (100 MB inflated) → 447 MB RSS in structTree; 195 KB (200 MB) → mupdf's own compression bomb detected, which the structTree guard turns into unreadable. So it is bounded today by mupdf, not by this code. toUnicode already does it the other way round (buf.getLength() > 1 << 20 before asString()); the same check here would make the cap the thing that holds.

  • An OBJR with no /Obj is reported as a model failure — src/review/outline.ts:35-36, exit 1. o.get(\"Subtype\") on the null object throws a TypeError inside build(i), so every page comes back review_failed: page N: Cannot read properties of null (reading '_fromPDFObjectKeep') and the command exits 1, where the same class of malformed tree in the walk gives unreadable and exit 3. No money is spent (it throws before ask), and test/review.test.ts:245 asserts only /./ on that message, so the shape is unpinned.

Accessibility impact: none on the tagged PDF — review writes nothing — but on pages with merged table cells the review's screen-reader view drops the spans, so it will report table defects that are not there and miss ones that are.

Without ColSpan and RowSpan the model saw a short header row and could
report a table problem that is not there. An OBJR without /Obj is skipped.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8d34270: all three notes from my review of a91d25a — ColSpan/RowSpan now reach the model (src/review/outline.ts:19, with a test), the content-stream length is checked before the string is built (src/pdf/read.ts:45-46), and an OBJR with no /Obj is skipped instead of failing the page (src/review/outline.ts:25, the test now asserts error === undefined). All checks pass.

One blocking issue, in the half of that new cap that was left open.

A 31 KB PDF makes review use 1.7 GB — src/pdf/read.ts:44-48

  for (const s of contents.isArray() ? Array.from({ length: contents.length }, (_, i) => contents.get(i)) : [contents]) {
    const buf = s.isStream() ? s.readStream() : undefined;
    if (buf && buf.getLength() <= MAX_CONTENT) streams.push(buf.asString());
  }
  const all = streams.join("\n").slice(0, MAX_CONTENT);

MAX_CONTENT bounds one stream, not the page. A /Contents array may hold any number of entries, and they may all be the same indirect stream, so the bytes read scale with the array length while the file stays tiny. src/pdf/read.ts:8 says "bytes of page content read for text", a bound the code does not have.

Input that reaches it — one page whose /Contents is an array of N references to a single 30 MB stream ("x".repeat(30 << 20)), saved with compress, reopened, mcidText called on the page:

N PDF on disk result
8 31,305 bytes 941 MB RSS, 1.3 s
16 31,353 bytes 1,697 MB RSS, 2.5 s
32 31,449 bytes throws Invalid string length (V8's string cap) after 4.5 s
64 31,641 bytes throws realloc (40190937 bytes) failed after 7.2 s

So a 31 KB file drives ~1.7 GB of resident memory before anything stops it, and what stops it is V8's or mupdf's limit, not this code. On a machine with less memory than that the process is OOM-killed: no error line, no exit code, no report — the outcome src/report.ts:60 rules out ("Never a crash, never a silent degradation"). Above the limit the throw is caught by the guard at src/review/review.ts:90-93 and reported as unreadable, so either way review cannot be run on such a file.

The array branch is the ordinary path, not an exotic one: src/tag.ts:111 writes page.put("Contents", [...artifactStreams(doc, page), doc.addStream(overlay, {})]), so every page this tool tags has a /Contents array.

Fix: carry a running total and stop at MAX_CONTENT —
if (buf && total + buf.getLength() <= MAX_CONTENT) { total += buf.getLength(); streams.push(buf.asString()); } — which also makes slice(0, MAX_CONTENT) redundant.

Tests: test/review.test.ts:220 covers a /Contents that is not a stream and :239 a CMap over 1 MB; nothing covers the size of a content stream that is one. The behaviour this commit changed — an over-long stream is now skipped whole, where before its first 32 MB were read — has no test either, so neither the per-stream cap nor a page total is pinned.

Non-blocking notes

  • Unchanged since 9108e71, raised four times and left to a human: nothing checks the tree is this tool's beyond "its marked text decodes".

Accessibility impact: none on the tagged PDF — review writes nothing — but a crafted tagged file can still exhaust memory in the reader, so the accessibility check this PR adds cannot be run on those files.

A /Contents array repeating one large stream read it again for every
entry. Streams are now read once each, from the end where our overlay is,
until the page total reaches 32 MB.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5d8d6bd: the per-page content cap from my review of 8d34270. Streams are read once each (src/pdf/read.ts:46-47) and the running total stops at 32 MB, so the /Contents array length no longer drives the bytes read. Re-ran my repro — one page whose /Contents is N references to a single 30 MB stream, saved with compress, reopened, mcidText on the page:

N PDF on disk before now
16 ~31 KB 1,697 MB RSS 248 MB RSS, 157 ms
64 ~31 KB realloc … failed 248 MB RSS, 157 ms
200 ~31 KB — 248 MB RSS, 160 ms

Flat in N, and the <</MCID 7>> at the end of the array is still decoded, so reading from the end keeps our overlay. test/review.test.ts:256-268 pins both halves (dedup and the page total). All checks pass; no blocking issue.

Non-blocking notes

  • Unchanged since 9108e71, now raised six times and pushed past each time — leaving it to a human: nothing checks the tree is this tool's beyond "its marked text decodes" (src/review/review.ts:93-97). A PDF tagged by another tool whose text happens to decode through /ToUnicode hex shows as empty elements and is reviewed as if the content were missing, while the refusal message says "Only PDFs tagged by iris-pdf can be reviewed."

Accessibility impact: none on the tagged PDF — review writes nothing — and the reader's memory is now bounded by this code rather than by mupdf's or V8's limits, so the check can be run on untrusted tagged files.

@bbertucc
bbertucc merged commit 89792b6 into main Sep 24, 2026
2 checks passed
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