zombor/pergamum
1
0
Fork 0
Code Issues Pull requests 3 Projects Releases Packages 2 Wiki Activity Actions

fix(llm-scan): feed PDF text layer + embedded doc-info + more front pages to the extractor (bookshelf-lyhus) #1476

Open
zombor wants to merge 2 commits from bd-bookshelf-lyhus into main
pull from: bd-bookshelf-lyhus
merge into: zombor:main
Conversation 3 Commits 2 Files changed 13 +964 -87
zombor commented 2026-09-08 01:28:13 +00:00
Owner
Copy link

Root cause

The LLM scan of PDFs was vision-only and front-page-starved (bookshelf-lyhus):

  • renderImages returned empty bodyText for PDFs — only EPUB got its spine
    text extracted, so PDFs sent no text layer to the LLM.
  • renderPDFImages sampled only pages [1,2,last-1,last] — credits/title
    pages further into the front matter (e.g. page 3) were never rendered.
  • The PDF's embedded doc-info (/Author, /CreationDate, /Title) was never
    read into the scan.

On a real game-book PDF this meant author (page-3 credits + /Author) and
published date (/CreationDate) were both missed even though they were
trivially present in the file.

Fixes

internal/files (activity-internal — no workflow command-sequence change,
no gowf.Version gate needed):

  • ExtractPDFTextFromReader/-WithDeps: extracts pdfium GetPageText
    output for the first maxPages pages, capped at maxBytes total. Shares
    runWithPDFInstance/openPDFDocFromReader with the existing page-image
    render path (refactored out of RenderPDFPageImagesFromReaderWithDeps).
  • PDFMetadata gains PublishedDate, parsed from the PDF's /CreationDate
    info-dict entry via pdfcpu's date parser.

internal/wfengine/llm_scan_workflow.go:

  • renderPDFImages now renders 4 front + 1 back pages instead of 2+2, so
    front-matter credits pages are in the vision input.
  • bodyText for PDFs is now populated: a short "PDF embedded metadata:
    Title=... Author=... Published=..." hint (from readFile + doc-info) is
    prefixed to the PDF's text-layer excerpt (new ExtractPDFText dep,
    5 pages / 8 KiB cap). Both are hints fed to the extractor prompt, not
    applied directly as fetched candidates — the user still reviews every
    fetched candidate before it's persisted.
  • Both new inputs are strictly non-fatal: a nil ExtractPDFText dep, a
    failed text extraction, or a failed/absent doc-info read all degrade to
    the pre-existing vision-only behavior rather than failing the scan.

Test plan

  • Hand-crafted single-page PDF fixtures (real Helvetica content stream +
    /Author + /CreationDate, byte-accurate xref) prove text + doc-info now
    reach ExtractPDFTextFromReader and the activity's bodyText — see
    internal/files/pdf_text_extract_test.go and the new PDF-path Contexts
    in internal/wfengine/llm_scan_workflow_test.go.
  • EPUB and comic scan paths are unchanged and still pass their existing tests.
  • make test, make lint, and make coverage are green locally (100%
    internal/ coverage maintained).

Closes bead bookshelf-lyhus on merge.

## Root cause The LLM scan of PDFs was vision-only and front-page-starved (bookshelf-lyhus): - `renderImages` returned empty `bodyText` for PDFs — only EPUB got its spine text extracted, so PDFs sent no text layer to the LLM. - `renderPDFImages` sampled only pages [1,2,last-1,last] — credits/title pages further into the front matter (e.g. page 3) were never rendered. - The PDF's embedded doc-info (`/Author`, `/CreationDate`, `/Title`) was never read into the scan. On a real game-book PDF this meant author (page-3 credits + `/Author`) and published date (`/CreationDate`) were both missed even though they were trivially present in the file. ## Fixes `internal/files` (activity-internal — no workflow command-sequence change, no `gowf.Version` gate needed): - `ExtractPDFTextFromReader`/`-WithDeps`: extracts pdfium `GetPageText` output for the first `maxPages` pages, capped at `maxBytes` total. Shares `runWithPDFInstance`/`openPDFDocFromReader` with the existing page-image render path (refactored out of `RenderPDFPageImagesFromReaderWithDeps`). - `PDFMetadata` gains `PublishedDate`, parsed from the PDF's `/CreationDate` info-dict entry via pdfcpu's date parser. `internal/wfengine/llm_scan_workflow.go`: - `renderPDFImages` now renders 4 front + 1 back pages instead of 2+2, so front-matter credits pages are in the vision input. - `bodyText` for PDFs is now populated: a short "PDF embedded metadata: Title=... Author=... Published=..." hint (from `readFile` + doc-info) is prefixed to the PDF's text-layer excerpt (new `ExtractPDFText` dep, 5 pages / 8 KiB cap). Both are hints fed to the extractor prompt, not applied directly as fetched candidates — the user still reviews every fetched candidate before it's persisted. - Both new inputs are strictly non-fatal: a nil `ExtractPDFText` dep, a failed text extraction, or a failed/absent doc-info read all degrade to the pre-existing vision-only behavior rather than failing the scan. ## Test plan - Hand-crafted single-page PDF fixtures (real Helvetica content stream + `/Author` + `/CreationDate`, byte-accurate xref) prove text + doc-info now reach `ExtractPDFTextFromReader` and the activity's `bodyText` — see `internal/files/pdf_text_extract_test.go` and the new PDF-path `Context`s in `internal/wfengine/llm_scan_workflow_test.go`. - EPUB and comic scan paths are unchanged and still pass their existing tests. - `make test`, `make lint`, and `make coverage` are green locally (100% `internal/` coverage maintained). Closes bead bookshelf-lyhus on merge.
fix(llm-scan): feed PDF text layer + embedded doc-info + more front pages to the extractor (bookshelf-lyhus)
All checks were successful
/ JS Unit Tests (pull_request) Successful in 1m15s
Details
/ E2E API (pull_request) Successful in 1m25s
Details
/ Test Race (pull_request) Successful in 1m47s
Details
/ Integration (pull_request) Successful in 2m6s
Details
/ Lint (pull_request) Successful in 2m7s
Details
/ Coverage (pull_request) Successful in 2m29s
Details
/ E2E Browser (pull_request) Successful in 5m27s
Details
1f9ba7c070
 
The PDF LLM-scan path was vision-only and front-page-starved: renderImages
returned empty bodyText for PDFs (only EPUB got its spine text extracted),
renderPDFImages sampled only pages [1,2,last-1,last] (missing credits/title
pages further in), and the PDF's embedded doc-info (/Author, /CreationDate,
/Title) was never read into the scan. On a real game-book PDF this meant
author (on page 3 credits + in /Author) and published date (in
/CreationDate) were both missed even though they were trivially present in
the file.

Fixes, in internal/files (activity-internal, no workflow command-sequence
change — no gowf.Version gate needed):
- files.ExtractPDFTextFromReader/-WithDeps: extracts pdfium GetPageText
  output for the first maxPages pages, capped at maxBytes total, sharing
  runWithPDFInstance/openPDFDocFromReader with the existing page-image
  render path (refactored out of RenderPDFPageImagesFromReaderWithDeps).
- files.PDFMetadata gains PublishedDate, parsed from the PDF's /CreationDate
  info-dict entry via pdfcpu's date parser.

In internal/wfengine/llm_scan_workflow.go:
- LLMScanActivities.renderPDFImages now renders 4 front + 1 back pages
  (pdfScanFrontPages/pdfScanBackPages) instead of 2+2, so front-matter
  credits pages are in the vision input.
- bodyText for PDFs is now populated: a short "PDF embedded metadata:
  Title=... Author=... Published=..." hint (from readFile + doc-info) is
  prefixed to the PDF's text-layer excerpt (from the new ExtractPDFText dep,
  pdfScanTextPages=5 / pdfScanTextBytes=8KiB). Both are hints fed to the
  extractor prompt, not applied directly as fetched candidates — the user
  still reviews every fetched candidate before it's persisted.
- Both new inputs are strictly non-fatal: a nil ExtractPDFText dep, a failed
  text extraction, or a failed/absent doc-info read all degrade to the
  pre-existing vision-only behavior rather than failing the scan.

Verification: a hand-crafted single-page PDF fixture with a real Helvetica
content stream + /Author + /CreationDate proves author/date/text now reach
ExtractPDFTextFromReader and the activity's bodyText (see
pdf_text_extract_test.go and the new PDF-path Contexts in
llm_scan_workflow_test.go). EPUB and comic scan paths are unchanged and
still pass their existing tests. `make test`, `make lint`, and `make
coverage` are green (100% internal/ coverage maintained).

Closes bead bookshelf-lyhus on merge.
zombor commented 2026-09-08 01:38:06 +00:00
Author
Owner
Copy link

Code Review — PR #1476 (bd-bookshelf-lyhus)

Phase 1: Spec compliance — all four fix items from the bead are present: (1) files.ExtractPDFTextFromReader (bounded 5 pages / 8 KiB) wired as bodyText, (2) embedded doc-info hint (/Title,/Author,/CreationDate→PublishedDate) prepended as a hint line, not applied as a direct candidate, (3) front/back page split changed from 2+2 to 4+1 (pdfScanFrontPages/pdfScanBackPages, internal/wfengine/llm_scan_workflow.go:326-334), matching the union cap maxRenderPages=5 in internal/files/pdf_page_images.go:24. EPUB/comic paths are untouched (no diff lines touch renderComicImages/renderEPUBContent).

Phase 2 findings:

[MAJOR] internal/wfengine/llm_scan_workflow.go:396-402 (pdfDocInfoHint) + internal/app/build_llm_deps.go:203 (ReadFile: os.ReadFile) — unbounded full-file read for doc-info hint
pdfDocInfoHint calls a.readFile(fullPath), wired in production to plain os.ReadFile, with no size cap. This loads the entire PDF into memory just to pull three info-dict strings. The codebase already has the right pattern for exactly this operation: internal/library/scan/meta.go:220 bounds the same ExtractPDFMetadataWithSize call with io.ReadAll(io.LimitReader(f, files.MaxPDFBytes)). This PR doesn't reuse that bound. PDFs (unlike EPUBs, which already have this same unbounded-read wart pre-existing at line ~358) commonly run into the hundreds of MB to GB range (scanned/OCR books), and this scan path is invoked per-book both interactively and via the bounded-fan-out bulk sweep — several concurrent multi-hundred-MB os.ReadFile calls is a real memory-pressure risk that the project's own MaxPDFBytes convention exists to prevent. Fix: bound the read with io.LimitReader(..., files.MaxPDFBytes) (mirroring meta.go) before calling ExtractPDFMetadataWithSize, or expose a size-capped variant of readFile for this call site.

[MINOR] internal/wfengine/llm_scan_workflow.go:373-393 (buildPDFBodyText) — hint failure paths are silent beyond a Warn log
Both the text-layer extraction failure and the doc-info read/parse failure are correctly non-fatal (matches the "no fatal degradation" requirement) and are logged via a.logger.Warn with book_id + error — good, no defect. Optional follow-up: a metric/counter for "PDF scans that degraded to vision-only" would help track how often the new paths actually help; not required for this PR.

Verified clean:

  • Workflow replay safety: LLMScanActivities gained a new extractPDFText struct field / constructor param, but no new ExecuteActivity/sub-workflow/ContinueAsNew was added to the workflow's emitted command sequence — buildPDFBodyText/pdfDocInfoHint are called inside the existing ScanBook activity body, which is activity-internal and replay-exempt per project-conventions.md. replay_golden_test.go's only changes thread an extra nil constructor arg through existing golden-replay stub calls (lines 1649, 1964, 2026) — no new recorded command. No gowf.Version gate needed; correctly omitted.
  • Bounds: extractPDFTextOnInst (internal/files/pdf_page_images.go:186-216) enforces both maxPages (loop bound i < n) AND maxBytes (loop condition sb.Len() < maxBytes plus truncation of the final chunk) during extraction, not after buffering the whole doc — genuinely bounded, tested with real multi-page/oversized fixtures in pdf_text_extract_test.go.
  • Hint semantics: pdfDocInfoHint's output is joined into bodyText, which flows through to extractMetadata (the LLM call) as context — never directly assigned to a result field. Consistent with the "bookdrop fetched metadata needs review" pattern — the candidate-review step still gates before persistence.
  • CreationDate parsing: uses pdfcputypes.DateTime(info.CreationDate, true); malformed dates return ok=false → empty PublishedDate, tested (pdf_metadata_test.go "unparseable /CreationDate" case) and non-fatal (never fails ExtractPDFMetadataWithSize).
  • internal/files has no workflow-engine import — confirmed via grep, architecture boundary intact.
  • Tests are black-box, real, non-vacuous: pdf_text_extract_test.go hand-builds real PDFs with actual Tj content streams so GetPageText has genuine text to extract; llm_scan_workflow_test.go exercises the nil-dep, extraction-error, and doc-info-read-error degradation paths and asserts the scan still succeeds with empty bodyText — real regression tests, not tautologies.
  • Fan-out concurrency: unaffected by this PR — the per-book bounded fan-out (scanFileFanOutOptions) was pre-existing and untouched.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

### Code Review — PR #1476 (bd-bookshelf-lyhus) **Phase 1: Spec compliance** — all four fix items from the bead are present: (1) `files.ExtractPDFTextFromReader` (bounded 5 pages / 8 KiB) wired as `bodyText`, (2) embedded doc-info hint (`/Title`,`/Author`,`/CreationDate`→`PublishedDate`) prepended as a hint line, not applied as a direct candidate, (3) front/back page split changed from 2+2 to 4+1 (`pdfScanFrontPages`/`pdfScanBackPages`, internal/wfengine/llm_scan_workflow.go:326-334), matching the union cap `maxRenderPages=5` in internal/files/pdf_page_images.go:24. EPUB/comic paths are untouched (no diff lines touch `renderComicImages`/`renderEPUBContent`). **Phase 2 findings:** [MAJOR] internal/wfengine/llm_scan_workflow.go:396-402 (`pdfDocInfoHint`) + internal/app/build_llm_deps.go:203 (`ReadFile: os.ReadFile`) — unbounded full-file read for doc-info hint `pdfDocInfoHint` calls `a.readFile(fullPath)`, wired in production to plain `os.ReadFile`, with no size cap. This loads the *entire* PDF into memory just to pull three info-dict strings. The codebase already has the right pattern for exactly this operation: `internal/library/scan/meta.go:220` bounds the same `ExtractPDFMetadataWithSize` call with `io.ReadAll(io.LimitReader(f, files.MaxPDFBytes))`. This PR doesn't reuse that bound. PDFs (unlike EPUBs, which already have this same unbounded-read wart pre-existing at line ~358) commonly run into the hundreds of MB to GB range (scanned/OCR books), and this scan path is invoked per-book both interactively and via the bounded-fan-out bulk sweep — several concurrent multi-hundred-MB `os.ReadFile` calls is a real memory-pressure risk that the project's own `MaxPDFBytes` convention exists to prevent. Fix: bound the read with `io.LimitReader(..., files.MaxPDFBytes)` (mirroring meta.go) before calling `ExtractPDFMetadataWithSize`, or expose a size-capped variant of `readFile` for this call site. [MINOR] internal/wfengine/llm_scan_workflow.go:373-393 (`buildPDFBodyText`) — hint failure paths are silent beyond a Warn log Both the text-layer extraction failure and the doc-info read/parse failure are correctly non-fatal (matches the "no fatal degradation" requirement) and are logged via `a.logger.Warn` with book_id + error — good, no defect. Optional follow-up: a metric/counter for "PDF scans that degraded to vision-only" would help track how often the new paths actually help; not required for this PR. **Verified clean:** - **Workflow replay safety**: `LLMScanActivities` gained a new `extractPDFText` struct field / constructor param, but no new `ExecuteActivity`/sub-workflow/`ContinueAsNew` was added to the workflow's emitted command sequence — `buildPDFBodyText`/`pdfDocInfoHint` are called *inside* the existing `ScanBook` activity body, which is activity-internal and replay-exempt per project-conventions.md. `replay_golden_test.go`'s only changes thread an extra `nil` constructor arg through existing golden-replay stub calls (lines 1649, 1964, 2026) — no new recorded command. No `gowf.Version` gate needed; correctly omitted. - **Bounds**: `extractPDFTextOnInst` (internal/files/pdf_page_images.go:186-216) enforces both `maxPages` (loop bound `i < n`) AND `maxBytes` (loop condition `sb.Len() < maxBytes` plus truncation of the final chunk) *during* extraction, not after buffering the whole doc — genuinely bounded, tested with real multi-page/oversized fixtures in pdf_text_extract_test.go. - **Hint semantics**: `pdfDocInfoHint`'s output is joined into `bodyText`, which flows through to `extractMetadata` (the LLM call) as context — never directly assigned to a result field. Consistent with the "bookdrop fetched metadata needs review" pattern — the candidate-review step still gates before persistence. - **CreationDate parsing**: uses `pdfcputypes.DateTime(info.CreationDate, true)`; malformed dates return `ok=false` → empty `PublishedDate`, tested (pdf_metadata_test.go "unparseable /CreationDate" case) and non-fatal (never fails `ExtractPDFMetadataWithSize`). - **internal/files has no workflow-engine import** — confirmed via grep, architecture boundary intact. - **Tests are black-box, real, non-vacuous**: pdf_text_extract_test.go hand-builds real PDFs with actual `Tj` content streams so `GetPageText` has genuine text to extract; llm_scan_workflow_test.go exercises the nil-dep, extraction-error, and doc-info-read-error degradation paths and asserts the scan still succeeds with empty bodyText — real regression tests, not tautologies. - **Fan-out concurrency**: unaffected by this PR — the per-book bounded fan-out (`scanFileFanOutOptions`) was pre-existing and untouched. REVIEW VERDICT: 0 blocker, 1 major, 1 minor
zombor commented 2026-09-08 01:38:11 +00:00
Author
Owner
Copy link

Code Review — PR #1476 (bd-bookshelf-lyhus)

Phase 1: Spec compliance — all four fix items from the bead are present: (1) files.ExtractPDFTextFromReader (bounded 5 pages / 8 KiB) wired as bodyText, (2) embedded doc-info hint (/Title,/Author,/CreationDate→PublishedDate) prepended as a hint line, not applied as a direct candidate, (3) front/back page split changed from 2+2 to 4+1 (pdfScanFrontPages/pdfScanBackPages, internal/wfengine/llm_scan_workflow.go:326-334), matching the union cap maxRenderPages=5 in internal/files/pdf_page_images.go:24. EPUB/comic paths are untouched (no diff lines touch renderComicImages/renderEPUBContent).

Phase 2 findings:

[MAJOR] internal/wfengine/llm_scan_workflow.go:396-402 (pdfDocInfoHint) + internal/app/build_llm_deps.go:203 (ReadFile: os.ReadFile) — unbounded full-file read for doc-info hint
pdfDocInfoHint calls a.readFile(fullPath), wired in production to plain os.ReadFile, with no size cap. This loads the entire PDF into memory just to pull three info-dict strings. The codebase already has the right pattern for exactly this operation: internal/library/scan/meta.go:220 bounds the same ExtractPDFMetadataWithSize call with io.ReadAll(io.LimitReader(f, files.MaxPDFBytes)). This PR doesn't reuse that bound. PDFs (unlike EPUBs, which already have this same unbounded-read wart pre-existing at line ~358) commonly run into the hundreds of MB to GB range (scanned/OCR books), and this scan path is invoked per-book both interactively and via the bounded-fan-out bulk sweep — several concurrent multi-hundred-MB os.ReadFile calls is a real memory-pressure risk that the project's own MaxPDFBytes convention exists to prevent. Fix: bound the read with io.LimitReader(..., files.MaxPDFBytes) (mirroring meta.go) before calling ExtractPDFMetadataWithSize, or expose a size-capped variant of readFile for this call site.

[MINOR] internal/wfengine/llm_scan_workflow.go:373-393 (buildPDFBodyText) — hint failure paths are silent beyond a Warn log
Both the text-layer extraction failure and the doc-info read/parse failure are correctly non-fatal (matches the "no fatal degradation" requirement) and are logged via a.logger.Warn with book_id + error — good, no defect. Optional follow-up: a metric/counter for "PDF scans that degraded to vision-only" would help track how often the new paths actually help; not required for this PR.

Verified clean:

  • Workflow replay safety: LLMScanActivities gained a new extractPDFText struct field / constructor param, but no new ExecuteActivity/sub-workflow/ContinueAsNew was added to the workflow's emitted command sequence — buildPDFBodyText/pdfDocInfoHint are called inside the existing ScanBook activity body, which is activity-internal and replay-exempt per project-conventions.md. replay_golden_test.go's only changes thread an extra nil constructor arg through existing golden-replay stub calls (lines 1649, 1964, 2026) — no new recorded command. No gowf.Version gate needed; correctly omitted.
  • Bounds: extractPDFTextOnInst (internal/files/pdf_page_images.go:186-216) enforces both maxPages (loop bound i < n) AND maxBytes (loop condition sb.Len() < maxBytes plus truncation of the final chunk) during extraction, not after buffering the whole doc — genuinely bounded, tested with real multi-page/oversized fixtures in pdf_text_extract_test.go.
  • Hint semantics: pdfDocInfoHint's output is joined into bodyText, which flows through to extractMetadata (the LLM call) as context — never directly assigned to a result field. Consistent with the "bookdrop fetched metadata needs review" pattern — the candidate-review step still gates before persistence.
  • CreationDate parsing: uses pdfcputypes.DateTime(info.CreationDate, true); malformed dates return ok=false → empty PublishedDate, tested (pdf_metadata_test.go "unparseable /CreationDate" case) and non-fatal (never fails ExtractPDFMetadataWithSize).
  • internal/files has no workflow-engine import — confirmed via grep, architecture boundary intact.
  • Tests are black-box, real, non-vacuous: pdf_text_extract_test.go hand-builds real PDFs with actual Tj content streams so GetPageText has genuine text to extract; llm_scan_workflow_test.go exercises the nil-dep, extraction-error, and doc-info-read-error degradation paths and asserts the scan still succeeds with empty bodyText — real regression tests, not tautologies.
  • Fan-out concurrency: unaffected by this PR — the per-book bounded fan-out (scanFileFanOutOptions) was pre-existing and untouched.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

### Code Review — PR #1476 (bd-bookshelf-lyhus) **Phase 1: Spec compliance** — all four fix items from the bead are present: (1) `files.ExtractPDFTextFromReader` (bounded 5 pages / 8 KiB) wired as `bodyText`, (2) embedded doc-info hint (`/Title`,`/Author`,`/CreationDate`→`PublishedDate`) prepended as a hint line, not applied as a direct candidate, (3) front/back page split changed from 2+2 to 4+1 (`pdfScanFrontPages`/`pdfScanBackPages`, internal/wfengine/llm_scan_workflow.go:326-334), matching the union cap `maxRenderPages=5` in internal/files/pdf_page_images.go:24. EPUB/comic paths are untouched (no diff lines touch `renderComicImages`/`renderEPUBContent`). **Phase 2 findings:** [MAJOR] internal/wfengine/llm_scan_workflow.go:396-402 (`pdfDocInfoHint`) + internal/app/build_llm_deps.go:203 (`ReadFile: os.ReadFile`) — unbounded full-file read for doc-info hint `pdfDocInfoHint` calls `a.readFile(fullPath)`, wired in production to plain `os.ReadFile`, with no size cap. This loads the *entire* PDF into memory just to pull three info-dict strings. The codebase already has the right pattern for exactly this operation: `internal/library/scan/meta.go:220` bounds the same `ExtractPDFMetadataWithSize` call with `io.ReadAll(io.LimitReader(f, files.MaxPDFBytes))`. This PR doesn't reuse that bound. PDFs (unlike EPUBs, which already have this same unbounded-read wart pre-existing at line ~358) commonly run into the hundreds of MB to GB range (scanned/OCR books), and this scan path is invoked per-book both interactively and via the bounded-fan-out bulk sweep — several concurrent multi-hundred-MB `os.ReadFile` calls is a real memory-pressure risk that the project's own `MaxPDFBytes` convention exists to prevent. Fix: bound the read with `io.LimitReader(..., files.MaxPDFBytes)` (mirroring meta.go) before calling `ExtractPDFMetadataWithSize`, or expose a size-capped variant of `readFile` for this call site. [MINOR] internal/wfengine/llm_scan_workflow.go:373-393 (`buildPDFBodyText`) — hint failure paths are silent beyond a Warn log Both the text-layer extraction failure and the doc-info read/parse failure are correctly non-fatal (matches the "no fatal degradation" requirement) and are logged via `a.logger.Warn` with book_id + error — good, no defect. Optional follow-up: a metric/counter for "PDF scans that degraded to vision-only" would help track how often the new paths actually help; not required for this PR. **Verified clean:** - **Workflow replay safety**: `LLMScanActivities` gained a new `extractPDFText` struct field / constructor param, but no new `ExecuteActivity`/sub-workflow/`ContinueAsNew` was added to the workflow's emitted command sequence — `buildPDFBodyText`/`pdfDocInfoHint` are called *inside* the existing `ScanBook` activity body, which is activity-internal and replay-exempt per project-conventions.md. `replay_golden_test.go`'s only changes thread an extra `nil` constructor arg through existing golden-replay stub calls (lines 1649, 1964, 2026) — no new recorded command. No `gowf.Version` gate needed; correctly omitted. - **Bounds**: `extractPDFTextOnInst` (internal/files/pdf_page_images.go:186-216) enforces both `maxPages` (loop bound `i < n`) AND `maxBytes` (loop condition `sb.Len() < maxBytes` plus truncation of the final chunk) *during* extraction, not after buffering the whole doc — genuinely bounded, tested with real multi-page/oversized fixtures in pdf_text_extract_test.go. - **Hint semantics**: `pdfDocInfoHint`'s output is joined into `bodyText`, which flows through to `extractMetadata` (the LLM call) as context — never directly assigned to a result field. Consistent with the "bookdrop fetched metadata needs review" pattern — the candidate-review step still gates before persistence. - **CreationDate parsing**: uses `pdfcputypes.DateTime(info.CreationDate, true)`; malformed dates return `ok=false` → empty `PublishedDate`, tested (pdf_metadata_test.go "unparseable /CreationDate" case) and non-fatal (never fails `ExtractPDFMetadataWithSize`). - **internal/files has no workflow-engine import** — confirmed via grep, architecture boundary intact. - **Tests are black-box, real, non-vacuous**: pdf_text_extract_test.go hand-builds real PDFs with actual `Tj` content streams so `GetPageText` has genuine text to extract; llm_scan_workflow_test.go exercises the nil-dep, extraction-error, and doc-info-read-error degradation paths and asserts the scan still succeeds with empty bodyText — real regression tests, not tautologies. - **Fan-out concurrency**: unaffected by this PR — the per-book bounded fan-out (`scanFileFanOutOptions`) was pre-existing and untouched. REVIEW VERDICT: 0 blocker, 1 major, 1 minor
zombor commented 2026-09-08 02:50:58 +00:00
Author
Owner
Copy link

Security review (delayed — git.zombor.net returned HTTP 530 during the review session, so this could not be posted until now)

[BLOCKER] internal/files/pdf_page_images.go:190-224 (extractPDFTextOnInst) — per-page text decompression-bomb OOM. deps.GetPageText materializes the entire page's text into a Go string before the maxBytes truncation runs; the 8 KiB cap is only checked between pages. A single crafted page (Flate/LZW content stream, repeated Tj/TJ) can decompress into millions/billions of characters → a multi-GB string for one call → OOM of the shared worker on any ingested PDF. Fix: bound the read during the call — FPDFText_LoadPage + FPDFText_CountChars + ranged FPDFText_GetText{StartIndex:0, Count:min(charCount, remaining)} instead of the all-at-once GetPageText; or a hard per-page char ceiling with early-exit.

[MINOR] internal/wfengine/llm_scan_workflow.go:399-425 (pdfDocInfoHint) — full-file os.ReadFile for the doc-info hint (also flagged [MAJOR] by code review). Bound with files.MaxPDFBytes via io.LimitReader, mirroring internal/library/scan/meta.go:221.

Doc-info parsing, page-image rendering (maxRenderPages=5, fixed output size), no injection sink, no secret/PII logging, non-fatal failure paths, no arch-boundary violation, no workflow command-sequence change — all clean.

REVIEW VERDICT: 1 blocker, 0 major, 1 minor

**Security review (delayed — git.zombor.net returned HTTP 530 during the review session, so this could not be posted until now)** **[BLOCKER]** `internal/files/pdf_page_images.go:190-224` (`extractPDFTextOnInst`) — per-page text decompression-bomb OOM. `deps.GetPageText` materializes the *entire* page's text into a Go string **before** the `maxBytes` truncation runs; the 8 KiB cap is only checked *between* pages. A single crafted page (Flate/LZW content stream, repeated `Tj`/`TJ`) can decompress into millions/billions of characters → a multi-GB string for one call → OOM of the shared worker on any ingested PDF. Fix: bound the read *during* the call — `FPDFText_LoadPage` + `FPDFText_CountChars` + ranged `FPDFText_GetText{StartIndex:0, Count:min(charCount, remaining)}` instead of the all-at-once `GetPageText`; or a hard per-page char ceiling with early-exit. **[MINOR]** `internal/wfengine/llm_scan_workflow.go:399-425` (`pdfDocInfoHint`) — full-file `os.ReadFile` for the doc-info hint (also flagged [MAJOR] by code review). Bound with `files.MaxPDFBytes` via `io.LimitReader`, mirroring `internal/library/scan/meta.go:221`. Doc-info parsing, page-image rendering (maxRenderPages=5, fixed output size), no injection sink, no secret/PII logging, non-fatal failure paths, no arch-boundary violation, no workflow command-sequence change — all clean. **REVIEW VERDICT: 1 blocker, 0 major, 1 minor**
zombor force-pushed bd-bookshelf-lyhus from 1f9ba7c070
All checks were successful
/ JS Unit Tests (pull_request) Successful in 1m15s
Details
/ E2E API (pull_request) Successful in 1m25s
Details
/ Test Race (pull_request) Successful in 1m47s
Details
/ Integration (pull_request) Successful in 2m6s
Details
/ Lint (pull_request) Successful in 2m7s
Details
/ Coverage (pull_request) Successful in 2m29s
Details
/ E2E Browser (pull_request) Successful in 5m27s
Details
to 63d7b3e715
All checks were successful
/ E2E API (pull_request) Successful in 1m5s
Details
/ Test Race (pull_request) Successful in 1m40s
Details
/ JS Unit Tests (pull_request) Successful in 44s
Details
/ Lint (pull_request) Successful in 2m7s
Details
/ Integration (pull_request) Successful in 2m16s
Details
/ Coverage (pull_request) Successful in 2m23s
Details
/ E2E Browser (pull_request) Successful in 4m44s
Details
2026-09-08 03:07:55 +00:00 Compare
All checks were successful
/ E2E API (pull_request) Successful in 1m5s
Required
Details
/ Test Race (pull_request) Successful in 1m40s
Required
Details
/ JS Unit Tests (pull_request) Successful in 44s
Details
/ Lint (pull_request) Successful in 2m7s
Required
Details
/ Integration (pull_request) Successful in 2m16s
Required
Details
/ Coverage (pull_request) Successful in 2m23s
Required
Details
/ E2E Browser (pull_request) Successful in 4m44s
Required
Details
This pull request has changes conflicting with the target branch.
  • internal/app/build_llm_deps.go
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin bd-bookshelf-lyhus:bd-bookshelf-lyhus
git switch bd-bookshelf-lyhus
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
zombor/pergamum!1476
No description provided.