fix: convert every page of a PDF, not the first eighty - #234
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📝 WalkthroughWalkthroughThe worker now delegates document conversion to Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to Some documents can be marked converted despite containing no extractable text, while updated documents can retain stale final-window results. Correct both conversion and reread paths before merging. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cf0251767
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (textLayer(read.markdown).characters > 0) { | ||
| text = read.markdown; | ||
| pages = pageMetadata(read); |
There was a problem hiding this comment.
When a cached image-only PDF is encountered, unsure is true but this condition is false, so the retained Markdown is never rewritten with page metadata. Every later convert message therefore sees customMetadata.pages missing and runs convertPdf over as many as 2,000 pages again; fresh scans enter the same state after their initial toMarkdown conversion. Persist an inspection marker even when retaining the AI-produced Markdown so scans receive the intended one-time migration rather than repeatedly consuming Worker CPU.
AGENTS.md reference: AGENTS.md:L52-L54
Useful? React with 👍 / 👎.
| // Pages the converter did not write down, which no window can have read. | ||
| const unconverted = pagesUnconverted(object.customMetadata); |
There was a problem hiding this comment.
unconverted into skipped readings
For a PDF longer than 2,000 pages that the gate routes through leftUnread, this positive count is discarded because that branch writes a skipped reading and returns before the later unconverted spread. pull-specs.ts passes that record through readInFull, which sees no partial marker and adds the source to reread, allowing staleFigures to remove old claims even though the omitted pages were never examined. Include unconverted in skipped readings and avoid the existing-skipped early return when that marker needs updating.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: bc25565d-b8d8-4f6a-b181-81be102dad7a
📒 Files selected for processing (11)
apps/worker/src/consumer.tsapps/worker/src/convert.tsapps/worker/src/extract.tsapps/worker/src/render.tsapps/worker/test/convert.test.tsapps/worker/test/extract.test.tsapps/worker/test/layout.test.tsapps/worker/test/world.tssrc/specs.tstest/specs.test.tstools/gate/pull-specs.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de2ee79bf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // for. A document the gate left unread is finished whatever the cap. | ||
| const written = await env.ARCHIVE.get(reading); | ||
| const before = written ? await written.json<KeptReading>() : undefined; | ||
| if (before && (before.skipped !== undefined || before.windows >= cap)) return; |
There was a problem hiding this comment.
When the old 80-page conversion produced exactly cap windows with no unread marker, the new full conversion can contain additional windows, but this guard returns before loading and chunking the replacement Markdown. The retained reading is therefore still treated as complete by pull-specs.ts, allowing staleFigures to remove claims even though none of the newly converted pages were examined. Only short-circuit on before.windows >= cap after confirming the current text has no additional windows, or update the reading with the new partial marker.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: ffcdd150-b571-47e7-961a-050b668c1eb0
📒 Files selected for processing (2)
apps/worker/src/convert.tsapps/worker/test/convert.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
| if (text === undefined && unsure) text = await (await env.ARCHIVE.get(markdown))?.text(); | ||
| if (text === undefined) { | ||
| const blob = new Blob([bytes], { type: message.contentType }); | ||
| const name = new URL(message.url).pathname.split("/").pop() || message.sha256; | ||
| const result = await env.AI.toMarkdown({ name, blob }); | ||
| const one = Array.isArray(result) ? result[0] : result; | ||
| if (!one || one.format === "error" || typeof one.data !== "string") { | ||
| // A scanned manual with no text layer is an answer about the maker's catalogue, not a | ||
| // failure to retry. It is recorded and the message is done. | ||
| await env.ARCHIVE.put( | ||
| partKey.converted(message.manufacturer, message.run, message.sha256), | ||
| JSON.stringify({ ...message, error: one?.error ?? "the converter returned no text" }), | ||
| { httpMetadata: { contentType: "application/json" } }, | ||
| ); | ||
| return; | ||
| } | ||
| text = one.data; | ||
| } | ||
| const written = text ?? ""; | ||
| await env.ARCHIVE.put(markdown, written, { | ||
| httpMetadata: { contentType: "text/markdown" }, | ||
| ...(pages ? { customMetadata: pages } : {}), | ||
| }); | ||
| size = new TextEncoder().encode(written).length; | ||
| } | ||
| await env.ARCHIVE.put( | ||
| partKey.converted(message.manufacturer, message.run, message.sha256), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,145p' apps/worker/src/convert.ts
rg -n 'function textLayer|const textLayer|textLayer\(|toMarkdown|converted|CONVERTED|MARKDOWN' apps/worker/src apps/worker/test/convert.test.ts
sed -n '1,180p' apps/worker/test/convert.test.tsRepository: origin89hq/data
Length of output: 25033
🏁 Script executed:
sed -n '400,515p' apps/worker/src/reading.ts
sed -n '65,155p' apps/worker/src/extract.ts
sed -n '1,120p' apps/worker/src/vision.tsRepository: origin89hq/data
Length of output: 15821
Validate reused and fresh Markdown before marking conversion successful.
toMarkdown can return only a title, metadata, and page headings. textLayer(...).characters is zero for that output. The current code accepts it, stores it with PDF page metadata, records a successful conversion, and queues extraction. Later conversions then skip toMarkdown.
The archive-reuse path also assigns stored Markdown without validation. Validate reused text before assigning text, and validate fresh output before storing it.
Proposed fix
- if (text === undefined && unsure) text = await (await env.ARCHIVE.get(markdown))?.text();
+ if (text === undefined && unsure) {
+ const reused = await (await env.ARCHIVE.get(markdown))?.text();
+ if (reused !== undefined && textLayer(reused).characters > 0) text = reused;
+ }
if (text === undefined) {
const blob = new Blob([bytes], { type: message.contentType });
const name = new URL(message.url).pathname.split("/").pop() || message.sha256;
const result = await env.AI.toMarkdown({ name, blob });
const one = Array.isArray(result) ? result[0] : result;
- if (!one || one.format === "error" || typeof one.data !== "string") {
+ if (
+ !one ||
+ one.format === "error" ||
+ typeof one.data !== "string" ||
+ textLayer(one.data).characters === 0
+ ) {Add regression tests for empty and page-heading-only output, including invalid archived output.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b7e54a2f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (textLayer(read.markdown).characters > 0) { | ||
| text = read.markdown; |
There was a problem hiding this comment.
When an older scan has no PDF text in its first 80 pages but contains even one text character on a later page, this condition replaces the cached toMarkdown OCR with the mostly empty PDFium output instead of taking the unsure retention path. readDocument can then reuse window objects from the old OCR by ordinal—or return immediately if the replacement is shorter—so the persisted reading can contain claims from text that is no longer in the archived Markdown. Preserve the cached OCR for scan-like output, or invalidate and rebuild all cached windows whenever the conversion prefix changes.
AGENTS.md reference: AGENTS.md:L49-L51
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26212ba897
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (textLayer(read.markdown).characters > 0) { | ||
| text = read.markdown; |
There was a problem hiding this comment.
For a first-time PDF whose first 80 pages are image-only but which has even one extractable character later, this branch now stores the mostly empty PDFium output instead of calling toMarkdown; previously the 80-page conversion contained zero characters and took the OCR path. seeDocument then either skips vision when the later text satisfies its density check or transcribes only the first 80 pages, while the text reading has converted === pages and is treated as complete, so scanned specifications after page 80 can be omitted and their prior claims removed as stale. Use the scan-density criterion rather than any single character, or otherwise ensure all image-only pages are represented before marking this conversion complete.
AGENTS.md reference: AGENTS.md:L55-L57
Useful? React with 👍 / 👎.
Change
The page-reading converter (#216) wrote down only the first 80 pages of a PDF and said nothing. The limit was the page reader's: a budget for drawing pages and paying a model to read each picture, borrowed as a default.
toMarkdown, which it replaced, wrote every page. In wave 2, 69 documents were cut at page 80, 8,029 pages in all, and a manual keeps its specifications at the back: Victron's 140-page off-grid booklet has its battery tables past page 80. Their readings look whole, so a figures pull would take back every figure those pages gave before.The limit was not only borrowed. Each page was converted by opening the whole PDF again for its characters, its pictures, its rules and its outline, so a 513-page manual was parsed over two thousand times: 67 seconds converting every page, over what a Worker may spend on a message.
convertPdfopens the document once and reads each page once for both its text and its outline. The per-page helpers keep their signatures and behaviour for their other callers.MOST_TEXT_PAGES(2,000), a bound against a catalogue rather than a budget. The conversion records the pages the document has and the pages written, as R2 metadata on the Markdown. A reading of a document converted only in part says how many pages it never saw (unconverted), and the pull counts it as partial, as it does a failed window or a window past the cap (fix: stop cutting manuals off at sixty windows #231).convert.ts, out of the handler that imports the WebAssembly, so it runs in a test. A PDF conversion that does not record its pages, which is every conversion made so far, is made once more. Every PDF's conversion records them, a scan's included: a scan keeps the texttoMarkdownwrote, is not sent to it again, and is written back with its pages counted, so it too is looked at once rather than at every convert. The new conversion replaces the one kept only where it adds to it: windows already read are taken up again by their place in the text, so a text changed before its end would have them stand for words it no longer holds. A scan with a typed page past page 80, converted bytoMarkdownthen and readable from its characters now, keeps whattoMarkdownwrote. Anything that is not a PDF is left alone.characters), so text added inside the last window, which adds no window, is seen too. A reading made before it recorded that was read from a conversion stopped at page 80, so one whose document is now converted past page 80 is read again at its end. And a reading at the cap is checked against its text unless it says it stopped there: one written at exactly the cap before readings said so looked whole.The converter name stays
layout-v1. Changing it would re-make every conversion under a new key, and the pull's retailer check withholds any reading whose text it cannot find at the current key, so retailer documents would have been withheld until each maker was converted again.Validation
just checkpasses.Timing on wave 2's longest PDFs, downloaded from the makers and checked against their archive hashes:
Converting every page now costs what converting eighty did.
The output is byte-identical to what production stored under
layout-v1for 36 wave-2 PDFs spread over all twelve makers (whole documents), and for pages 1 to 80 of the four above. The windows already read of those pages still stand, which is what lets the new pages be read on alone.Tests: a document converted past page 80 with its outline reaching the last page; a document cut at an explicit limit says how much it wrote; a document is opened once however many pages it has; the convert step writes page metadata, the outline, the new size in the converted record and the extract message; a conversion without page metadata is made again once and then left alone without opening the PDF; one with metadata is kept; a scan converted before is not sent to
toMarkdownagain and gets its page count, a new one goes totoMarkdownonce and gets it too, and neither is opened again after; a conversion the new one does not only extend is kept, with no outline written for the text it did not keep, and is not looked at again; a non-PDF is kept; the reader recordsunconvertedand reads a grown text on from its last window; text grown inside the last window is read again there; a whole reading of the text it has is left alone before its windows are listed; a reading written at the cap withoutunreadis checked and marked partial; a pre-existing reading of a document now past page 80 is read again at its end, one within 80 pages is not;readInFullis false forunconverted.Each rule broken in turn fails a test (20 of 20). Added last: a conversion replaced whatever the new text is, and an outline written for a text not kept. Before those, the recorded length ignored or not written, the cap returning before the text is checked, and the page-80 rule for earlier readings dropped. Two guards whose breaking failed nothing were removed as unreachable. The fourteen before: the limit back at 80, the document reopened per page, the outline dropped, the converted count not reported, no re-conversion, non-PDFs re-converted, a scan sent to
toMarkdownagain, a scan left without its page count, the metadata not written, the stale size, the last window not read again, the last window read again after a cap,unconvertednot recorded, and the pull ignoring it.Not run: the re-conversion itself, which needs this deployed.
Rollout
After deploy,
/convertfor the twelve wave-2 makers converts each of their PDFs once more, at no model cost, and reads on the 67 whose text grew. Estimated from each document's old conversion past page 80, scaled by how much longer the new converter made its first 80 pages, that is about 3,200 windows, around $16. 19 of them will still be longer than 150 windows; #232 now covers those. No figures pull for these makers should be merged before it has run.