feat(pdf): convert the readable pages when others need ocr - #153
feat(pdf): convert the readable pages when others need ocr#153larue-greenboard wants to merge 1 commit into
Conversation
A PDF with one image-only page returns nothing at all, so the pages that do carry text are lost along with it. `Ocr::Skip` converts those pages instead of failing the document and names the ones it left out on the result, leaving the default untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
4 issues found across 28 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/convert-documents-to-markdown/SKILL.md">
<violation number="1" location="skills/convert-documents-to-markdown/SKILL.md:25">
P3: Rule 5 adds that `--ocr skip` converts the readable pages and leaves the rest out, but does not say what exit code the process returns in that mode. `skip_pages_needing_ocr` returns success (exit 0) whenever even one page yields text, and only errors with exit 3 when nothing is readable. That makes rule 3's blanket contract "3 pages of a PDF need OCR" mode-dependent: a partially scanned PDF run with `--ocr skip` exits 0 while pages still need OCR. Clarify in this sentence that skip mode exits 0 when some pages convert (left-out pages are signalled only via the stderr message), so a caller that treats a non-zero exit as the detection signal is not misled.</violation>
</file>
<file name="python/anydoc/__init__.py">
<violation number="1" location="python/anydoc/__init__.py:65">
P3: When a caller stores the mode in the public `Ocr` type, both functions fail type checking because only literal overloads are exposed. Add fallback overloads accepting `Ocr` and returning `str | Conversion`.</violation>
</file>
<file name="wasm/test.mjs">
<violation number="1" location="wasm/test.mjs:95">
P3: A wasm conversion that drops readable pages 1 or 4 still passes this test because it checks only page 3. Assert all three readable pages so this binding test covers the per-page conversion guarantee.</violation>
<violation number="2" location="wasm/test.mjs:98">
P3: If the wasm error reports the wrong OCR pages or page count, this test still passes because it checks only `error.code`. Assert `[error.pages, error.pageCount]` equals `[[1, 2], 2]` to verify the documented error shape.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| 3. Exit codes: 0 success, 1 the document could not be converted, 2 usage error, 3 pages of a PDF need OCR. Failures print one `anydoc: <message>` line to stderr. The CLI never prompts. | ||
| 4. For a large document, write to a file with `-o` and read the parts you need instead of streaming everything into context. | ||
| 5. Scanned and image-only pages need OCR, which anydoc does not do, so the document exits 3. Rerun with `--ocr hosted` to send it to [Firecrawl Parse](https://firecrawl.dev/parse). No signup needed. Pass `--api-key` or set `FIRECRAWL_API_KEY` for higher limits. | ||
| 5. Scanned and image-only pages need OCR, which anydoc does not do, so the document exits 3. Rerun with `--ocr hosted` to send it to [Firecrawl Parse](https://firecrawl.dev/parse). No signup needed. Pass `--api-key` or set `FIRECRAWL_API_KEY` for higher limits. Where that is not an option, `--ocr skip` converts the pages that do carry text and names the pages it left out on stderr; stdout stays Markdown alone. |
There was a problem hiding this comment.
P3: Rule 5 adds that --ocr skip converts the readable pages and leaves the rest out, but does not say what exit code the process returns in that mode. skip_pages_needing_ocr returns success (exit 0) whenever even one page yields text, and only errors with exit 3 when nothing is readable. That makes rule 3's blanket contract "3 pages of a PDF need OCR" mode-dependent: a partially scanned PDF run with --ocr skip exits 0 while pages still need OCR. Clarify in this sentence that skip mode exits 0 when some pages convert (left-out pages are signalled only via the stderr message), so a caller that treats a non-zero exit as the detection signal is not misled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At skills/convert-documents-to-markdown/SKILL.md, line 25:
<comment>Rule 5 adds that `--ocr skip` converts the readable pages and leaves the rest out, but does not say what exit code the process returns in that mode. `skip_pages_needing_ocr` returns success (exit 0) whenever even one page yields text, and only errors with exit 3 when nothing is readable. That makes rule 3's blanket contract "3 pages of a PDF need OCR" mode-dependent: a partially scanned PDF run with `--ocr skip` exits 0 while pages still need OCR. Clarify in this sentence that skip mode exits 0 when some pages convert (left-out pages are signalled only via the stderr message), so a caller that treats a non-zero exit as the detection signal is not misled.</comment>
<file context>
@@ -22,5 +22,5 @@ Rules:
3. Exit codes: 0 success, 1 the document could not be converted, 2 usage error, 3 pages of a PDF need OCR. Failures print one `anydoc: <message>` line to stderr. The CLI never prompts.
4. For a large document, write to a file with `-o` and read the parts you need instead of streaming everything into context.
-5. Scanned and image-only pages need OCR, which anydoc does not do, so the document exits 3. Rerun with `--ocr hosted` to send it to [Firecrawl Parse](https://firecrawl.dev/parse). No signup needed. Pass `--api-key` or set `FIRECRAWL_API_KEY` for higher limits.
+5. Scanned and image-only pages need OCR, which anydoc does not do, so the document exits 3. Rerun with `--ocr hosted` to send it to [Firecrawl Parse](https://firecrawl.dev/parse). No signup needed. Pass `--api-key` or set `FIRECRAWL_API_KEY` for higher limits. Where that is not an option, `--ocr skip` converts the pages that do carry text and names the pages it left out on stderr; stdout stays Markdown alone.
6. Inside a Node, Python, or Rust codebase, prefer the library over shelling out: `@firecrawl/anydoc` on npm, `firecrawl-anydoc` on PyPI, `anydoc` on crates.io. Each exposes the same `to_markdown` / `toMarkdown` API.
</file context>
| 5. Scanned and image-only pages need OCR, which anydoc does not do, so the document exits 3. Rerun with `--ocr hosted` to send it to [Firecrawl Parse](https://firecrawl.dev/parse). No signup needed. Pass `--api-key` or set `FIRECRAWL_API_KEY` for higher limits. Where that is not an option, `--ocr skip` converts the pages that do carry text and names the pages it left out on stderr; stdout stays Markdown alone. | |
| 5. Scanned and image-only pages need OCR, which anydoc does not do, so the document exits 3. Rerun with `--ocr hosted` to send it to [Firecrawl Parse](https://firecrawl.dev/parse). No signup needed. Pass `--api-key` or set `FIRECRAWL_API_KEY` for higher limits. Where that is not an option, `--ocr skip` converts the pages that do carry text, exits 0 (or 3 if no page yields text), and names the pages it left out on stderr; stdout stays Markdown alone. |
|
|
||
|
|
||
| @overload | ||
| def to_markdown( |
There was a problem hiding this comment.
P3: When a caller stores the mode in the public Ocr type, both functions fail type checking because only literal overloads are exposed. Add fallback overloads accepting Ocr and returning str | Conversion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/anydoc/__init__.py, line 65:
<comment>When a caller stores the mode in the public `Ocr` type, both functions fail type checking because only literal overloads are exposed. Add fallback overloads accepting `Ocr` and returning `str | Conversion`.</comment>
<file context>
@@ -45,31 +48,58 @@
+@overload
+def to_markdown(
+ path: "str | os.PathLike[str]",
+ *,
</file context>
| test("ocr: 'skip' converts the pages that carry text and names the rest", () => { | ||
| const { markdown, pagesNeedingOcr, pageCount } = toMarkdownBytesWith(PARTLY_SCANNED, 'pdf', 'skip') | ||
| assert.deepEqual([pagesNeedingOcr, pageCount], [[2, 5], 5]) | ||
| assert.match(markdown, /Readable page three/) |
There was a problem hiding this comment.
P3: A wasm conversion that drops readable pages 1 or 4 still passes this test because it checks only page 3. Assert all three readable pages so this binding test covers the per-page conversion guarantee.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At wasm/test.mjs, line 95:
<comment>A wasm conversion that drops readable pages 1 or 4 still passes this test because it checks only page 3. Assert all three readable pages so this binding test covers the per-page conversion guarantee.</comment>
<file context>
@@ -86,6 +89,23 @@ test('conversion errors throw a coded Error', () => {
+test("ocr: 'skip' converts the pages that carry text and names the rest", () => {
+ const { markdown, pagesNeedingOcr, pageCount } = toMarkdownBytesWith(PARTLY_SCANNED, 'pdf', 'skip')
+ assert.deepEqual([pagesNeedingOcr, pageCount], [[2, 5], 5])
+ assert.match(markdown, /Readable page three/)
+ // The pages left out are reported, never written into the Markdown.
+ assert.doesNotMatch(markdown, /OCR|not converted/i)
</file context>
| assert.match(markdown, /Readable page three/) | |
| for (const page of ['one', 'three', 'four']) { | |
| assert.match(markdown, new RegExp(`Readable page ${page}`)) | |
| } |
| assert.match(markdown, /Readable page three/) | ||
| // The pages left out are reported, never written into the Markdown. | ||
| assert.doesNotMatch(markdown, /OCR|not converted/i) | ||
| assert.throws(() => toMarkdownBytesWith(SCANNED, 'pdf', 'skip'), (error) => { |
There was a problem hiding this comment.
P3: If the wasm error reports the wrong OCR pages or page count, this test still passes because it checks only error.code. Assert [error.pages, error.pageCount] equals [[1, 2], 2] to verify the documented error shape.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At wasm/test.mjs, line 98:
<comment>If the wasm error reports the wrong OCR pages or page count, this test still passes because it checks only `error.code`. Assert `[error.pages, error.pageCount]` equals `[[1, 2], 2]` to verify the documented error shape.</comment>
<file context>
@@ -86,6 +89,23 @@ test('conversion errors throw a coded Error', () => {
+ assert.match(markdown, /Readable page three/)
+ // The pages left out are reported, never written into the Markdown.
+ assert.doesNotMatch(markdown, /OCR|not converted/i)
+ assert.throws(() => toMarkdownBytesWith(SCANNED, 'pdf', 'skip'), (error) => {
+ assert.equal(error.code, 'needsOcr')
+ return true
</file context>
A PDF with one image-only page returns nothing at all, so the pages that do carry text are lost along with it. One image on page 3 of an 80-page contract costs the other 79 pages.
Ocr::Skipconverts the pages that carry text instead of failing the document, and names the ones it left out:Options::default().ocr(Ocr::Skip)returningConversion { markdown, pages_needing_ocr, page_count }in Rust,{ ocr: 'skip' }in Node,ocr="skip"in Python,toMarkdownBytesWith(bytes, format, 'skip')in wasm,--ocr skipon the CLI. A document where no page yielded text still fails withNeedsOcr— there is nothing to hand back, and the error is what tells a caller the document needs OCR rather than better parsing, so--ocr hostedand exit code 3 keep working as the signal for a real scan.The default is untouched, byte for byte:
to_markdown/to_markdown_bytesdelegate to the new*_withforms withOptions::default(), and all 66 existing snapshots are unchanged.Optionsis#[non_exhaustive]with a consumingocr()setter, so #102 and #146 have somewhere to land later. No dependency bump: pdf-inspector 1.14.2 already returns per-page Markdown with a per-pageneeds_ocr.Nothing is written into the Markdown
#144's third suggestion was an in-text marker where a page was dropped. This reports the pages on the result instead and leaves the output as content alone, because that Markdown is shown to readers and fed to models — a notice injected into it is corruption downstream, and unlike a marker, a page list can be branched on. The objection behind #140 was that the loss was silent, and it is not silent if the caller is handed the page numbers and had to opt in to get them. A test pins the Markdown as free of any such notice, so this cannot drift.
On the CLI the same reasoning puts the report on stderr, which already carries every diagnostic, so stdout stays Markdown alone:
Why per-page extraction, and where it costs
Whole-document extraction is all-or-nothing, not merely lossy:
process_pdf_memclassifies a mixed document asImageBasedand returnsmarkdown: Nonefor the whole thing, even where individual pages carry perfectly good text. So the readable pages have to come fromextract_pages_markdown_mem, and the per-pageneeds_ocrverdicts decide both what is converted and what is reported — the two cannot disagree.That pass only runs once detection and the existing confirmation pass agree a page is genuinely unreadable, so a document that converts today pays nothing new. Measured over 30 runs, an 80-page file:
ocr: 'skip'The second pass doubles the cost on documents that need it, against returning nothing at all. Worth a look if that trade is not the one you want.
Ocr::Skipis also decided before conversion rather than after a failure, unlike thehostedrecovery in #141: no throw, no second file read, no double parse.Tests
handmade-partly-scanned.pdfis a new 5-page fixture,TEXT, IMAGE, TEXT, TEXT, IMAGEwith per-page text, generated by the existinghandmade_pdf()helper. The existing one-text-one-image fixture cannot show that pages after an image page survive, which is the reported failure. Its default-path snapshot is the unchangedERROR: pages 2, 5 of 5 need OCR.Five assertions in
tests/snapshots.rscover the recovered pages, the absence of notices in the output, the fully scanned document still erroring,text.pdfbeing byte-identical in both modes, andOptions::default()matching the plain functions. Plus the Node, Python and wasm equivalents, and CLI coverage of both the stderr report and exit 3.cargo fmt --checkclean ·cargo clippy --workspace --all-targets --all-features -- -D warningsexit 0 ·cargo clippy -p anydoc-wasm --target wasm32-unknown-unknownexit 0 · 301 Rust tests, 24 Node, 13 Python, 9 wasm pass ·cargo docclean with-D warnings· Node overloads typecheck under--strict.Unrelated, noticed on the way
A PDF that detection does not flag but which yields no text returns
Unsupported, notNeedsOcr(the finalmatchinsrc/formats/pdf.rs). Since the hosted fallback keys oncode === 'needsOcr', such a document never reaches Parse even with--ocr hosted. Left alone here; happy to file it separately.Fixes #144
Summary by cubic
Fixes #144: a PDF with scanned pages previously returned nothing; now
ocr: 'skip'converts the readable pages and reports the ones it left out. The report is data on the result and stderr for the CLI, never a notice in the Markdown, and the default path is byte-for-byte unchanged.New Features
Ocr::SkiptoOptions, exposed asocr: 'skip'in Node and Python,--ocr skipon the CLI, andtoMarkdownBytesWith(bytes, format, 'skip')in wasm.Conversioncarryingmarkdown,pages_needing_ocr, andpage_count.NeedsOcr, so--ocr hostedand exit code 3 still signal a real scan.Written for commit 76c444f. Summary will update on new commits.