Skip to content

Add the verified local PyMuPDF PDF provider - #204

Open
salmonumbrella wants to merge 1 commit into
kenn-io:mainfrom
salmonumbrella:feat/pymupdf-rendition-provider
Open

Add the verified local PyMuPDF PDF provider#204
salmonumbrella wants to merge 1 commit into
kenn-io:mainfrom
salmonumbrella:feat/pymupdf-rendition-provider

Conversation

@salmonumbrella

@salmonumbrella salmonumbrella commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

What changed

Docbank now has a verified local PyMuPDF PDF-text provider without giving an ambient Python interpreter or filesystem surface to the rendition path. It sends the exact authorized PDF bytes over stdin to one operator-pinned executable and accepts only the fixed versioned stdout contract.

Docbank proves the PDF page count locally before publishing complete page evidence. Missing pages, partial output, runtime drift, oversized output, cancellation, expired authority, and executable replacement fail closed. Child stderr never crosses the public error boundary.

Why

Local PDF extraction should avoid network disclosure without becoming a privileged path that can reopen arbitrary files, discover runtimes, or publish page claims Docbank cannot verify.

Usage

For library use, construct the provider with pymupdf.New(profile). The profile pins the exact bridge executable, its SHA-256, immutable runtime identity, timeout, and byte/page limits; Docbank does not install Python or pass a source pathname.

This PR does not add daemon, API, or CLI selection. That application wiring is intentionally deferred to S1–S3.

Part of #176 (R12). Stacks on #203.

@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (a12cb91)

High-severity XML parsing flaws can crash the daemon or exhaust memory; two medium-severity reliability issues can cause endless retries or misclassified timeouts.

High

  • internal/processing/source_metadata.go:335 — XML elements store strings.Builder by value in a growing slice. Appending a child after character data initializes its parent can copy a non-zero builder, causing the next write to panic. Common formatted XMP or OOXML can therefore crash the daemon. Store builders behind pointers or use copy-safe storage, and test nested, formatted XML.

  • internal/processing/source_metadata.go:337 — Every character-data token is copied into every open ancestor without nesting or cumulative-memory limits. Deeply nested input can amplify a bounded document into excessive memory use and terminate the daemon. Limit nesting and total buffered text—ideally collecting only recognized metadata elements—and warn when limits are exceeded.

Medium

  • internal/processing/source_metadata.go:174metadataCollector.strings limits entry count but not individual entries to the 64 KiB schema maximum. Oversized extracted values can later fail MarshalSourceMetadataV1, preventing metadata publication and causing indefinite retries. Validate entries against MaxSourceMetadataValueBytes and omit oversized values with a warning.

  • document/bridge/client.go:124, document/bridge/client.go:176 — Expiration of the bridge client’s total-timeout context returns raw context.DeadlineExceeded, which the provider wrapper treats as unclassified. Routine timeouts are consequently marked operator_required instead of retryable. Preserve the caller context and classify local deadlines by phase: ambiguous during submission, retryable during polling or artifact retrieval, while retaining errors.Is compatibility.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 22m25s

@salmonumbrella
salmonumbrella force-pushed the feat/pymupdf-rendition-provider branch 3 times, most recently from 01fd7fa to 5ea8001 Compare August 25, 2026 05:20
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (5ea8001)

Review verdict: Four medium-severity issues could cause incorrect terminal failures, endless metadata retries, or bridge activity beyond authorization expiry.

Medium

  • Internal provider timeouts are misclassified as caller cancellation
    Locations: document/datalab/client.go:768, document/docling/client.go:503, document/marker/client.go:653
    Each client classifies expiration of its internally configured operation timeout as Canceled. The rendition worker treats that classification as terminal, so routine provider timeouts permanently fail jobs rather than permitting retry or ambiguous-outcome recovery. Distinguish parent-context cancellation from client-imposed deadlines; reserve Canceled for actual caller cancellation and classify internal timeouts as transient or ambiguous as appropriate.

  • Bridge context errors bypass provider-error classification
    Location: document/bridge/client.go:174
    Submission and polling return raw context.Canceled or context.DeadlineExceeded errors. The rendition worker does not recognize these as provider errors and moves affected jobs to operator_required, including during routine daemon shutdowns and bridge timeouts. Wrap context failures in classified RenditionProviderError values while distinguishing caller cancellation from bridge-imposed timeouts and preserving errors.Is behavior where needed.

  • Bridge operations can continue after authorization expires
    Location: document/bridge/client.go:124
    Render checks authorization.ExpiresAt only before starting, then bounds execution solely by client.totalTimeout. Uploads, polling, and artifact retrieval can therefore continue after authorization expiry, while receipt validation trusts provider-supplied timestamps rather than actual completion time. Bound the operation by the earliest caller deadline, total timeout, and authorization expiry; recheck expiry before every outbound request and use a separate bounded context for any necessary cleanup.

  • Metadata list entries bypass the codec’s value-size limit
    Location: internal/processing/source_metadata.go:174
    metadataCollector.strings validates UTF-8 and item count but does not enforce document.MaxSourceMetadataValueBytes for each entry. Oversized values can pass extraction and fail during marshaling, causing source-metadata backfill to retry indefinitely. Omit oversized entries with a warning, consistent with scalar metadata handling.


Reviewers: 2 done | Synthesis: codex, 13s | Total: 17m46s

@salmonumbrella salmonumbrella changed the title feat: add PyMuPDF rendition provider Add the verified local PyMuPDF PDF provider Aug 25, 2026
@salmonumbrella
salmonumbrella force-pushed the feat/pymupdf-rendition-provider branch from 5ea8001 to 9f15fa8 Compare August 25, 2026 10:38
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (9f15fa8)

High-severity reliability issue found, plus three medium-severity retry and metadata-validation risks.

High

  • internal/processing/source_metadata.go:323extractXMLText stores strings.Builder values in a growing slice. Appending nested elements after character data may copy a builder, causing subsequent writes to panic. Ordinary indented nested XML could terminate the daemon during metadata backfill.
    • Fix: Store builder pointers or safely copyable buffers, bound XML depth and accumulated text, and test nested XML with whitespace and mixed character data.

Medium

  • document/docling/client.go:362 — Retryable responses to the asynchronous submission POST are treated as ordinary transient errors. A 408 or 5xx may occur after Docling creates the job, so worker retries can trigger duplicate conversions and charges without a task ID for reconciliation.

    • Fix: Classify these responses as RenditionErrorAmbiguousSubmission unless an idempotency key or durable task handle makes resubmission safe.
  • document/mistral/rendition.go:413 — Transport failures after the Mistral POST begins are retried internally and then classified as transient, prompting another full worker retry. Without an idempotency key, a lost response can cause repeated billed OCR work.

    • Fix: Surface post-send uncertainty as RenditionErrorAmbiguousSubmission without automatic resubmission, or use provider-supported idempotency.
  • internal/processing/source_metadata.go:174 — List-valued metadata does not enforce the canonical codec’s per-item byte limit. Oversized values pass collection but fail serialization, preventing publication and causing indefinite retries.

    • Fix: Validate codec limits while collecting each item, omit invalid values with a bounded warning, and test that oversized list values still yield a publishable record.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 30m27s

@salmonumbrella
salmonumbrella force-pushed the feat/pymupdf-rendition-provider branch from 9f15fa8 to e915c10 Compare August 25, 2026 11:12
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (e915c10)

Code changes need revision: three Medium-severity reliability issues could cause unsafe retries, duplicate submissions, or publication with stale resume handles.

Medium

  • Provider deadlines are misclassified as caller cancellation
    Locations: document/bridge/client.go:174, document/datalab/client.go:768, document/docling/client.go:503, document/marker/client.go:653
    Bridge returns an unclassified ctx.Err(), while the other adapters classify internal timeouts as canceled. When the worker context remains active, these can become operator_required or terminal failures instead of safe retry/ambiguous outcomes. Preserve the caller context, distinguish caller cancellation, authorization expiry, and adapter timeout, then return a classified transient or ambiguous-submission error depending on whether egress occurred.

  • Docling polling exhaustion can duplicate remote submissions
    Location: document/docling/client.go:217
    Exhausting the polling limit after receiving a task ID returns retryable capacity. Because Docling has neither durable resume nor an idempotency key, retrying may resubmit the document while the original task is still running. Return RenditionErrorAmbiguousSubmission, or checkpoint the task ID through the resumable-provider contract before polling.

  • Resume checkpoint failures are discarded
    Location: document/execution.go:274
    ResumeRendition does not retain errors encountered while checkpointing a replacement handle. A provider can ignore that failure and return success or a retryable error, allowing publication or retry with a stale handle. Record the first checkpoint error and return it after RenderResumable, mirroring RenderRenditionWithResume.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 18m27s

@salmonumbrella
salmonumbrella force-pushed the feat/pymupdf-rendition-provider branch from e915c10 to b75f91f Compare August 25, 2026 11:39
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (b75f91f)

The changed provider and processing paths preserve authentication, destination pinning, bounded input/output handling, and consent checks without a concrete exploitable boundary bypass.

No issues found.


Reviewers: 2 total (1 done, 1 skipped) | Synthesis: codex | Total: 15m39s

@salmonumbrella
salmonumbrella force-pushed the feat/pymupdf-rendition-provider branch 5 times, most recently from bdfb28e to 9a43ae1 Compare August 25, 2026 12:51
@salmonumbrella
salmonumbrella force-pushed the feat/pymupdf-rendition-provider branch from 9a43ae1 to 4452692 Compare August 25, 2026 13:10
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (4452692)

Verdict: Changes requested — one high-severity security issue and four medium-severity reliability/resource issues.

High

  • Validated PyMuPDF executable can be replaced before execution
    Location: document/pymupdf/provider.go:201
    New validates the configured executable once with Lstat, but Render later executes the pathname without confirming it still refers to the same file. An account able to modify the executable or a parent directory could replace it and execute code as the Docbank daemon owner. RuntimeIdentity does not mitigate this because the replacement can return the expected value.
    Fix: Execute a pinned copy from an owner-private managed directory, or pin the executable to a stable file identity and prevent pathname replacement. Validate ownership and permissions/ACLs across the parent chain, derive runtime identity from verified contents, and include it in the policy fingerprint.

Medium

  • Metadata collection does not enforce canonical codec limits
    Location: internal/processing/source_metadata.go:174
    Oversized list items, labels, or aggregate metadata can pass extraction but fail MarshalSourceMetadataV1, leaving documents permanently queued for backfill.
    Fix: Enforce all codec limits during collection, including aggregate encoded size, and omit invalid metadata with a bounded warning.

  • XML extraction permits depth-amplified CPU and memory consumption
    Location: internal/processing/source_metadata.go:321
    extractXMLText copies each character-data token into every open ancestor builder, causing depth-times-text amplification for deeply nested XML.
    Fix: Capture text only for relevant elements and enforce explicit nesting-depth and aggregate-text budgets.

  • Stale retry entries can cause continuous SQLite scanning
    Location: cmd/docbank/backfill_retry.go:38
    If a retry target disappears, its expired entry remains indefinitely. waitDelay then continually returns zero, causing both daemon backfill loops to scan SQLite without sleeping.
    Fix: Reconcile retries against targets seen during a complete scan, or discard expired entries that no longer correspond to missing work while retaining a positive fallback delay.

  • Docling polling exhaustion can duplicate remote work and charges
    Location: document/docling/client.go:217
    Polling exhaustion after successful submission is treated as a retryable capacity failure. Because the task ID is not persisted, retrying submits another conversion while the original may still be running.
    Fix: Return an ambiguous-submission error, consistent with the result-fetch path, or persist the task ID and support resumable rendering.


Reviewers: 2 done | Synthesis: codex, 17s | Total: 24m54s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant