Skip to content

Add the hosted Reducto rendition provider - #206

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

Add the hosted Reducto rendition provider#206
salmonumbrella wants to merge 1 commit into
kenn-io:mainfrom
salmonumbrella:feat/reducto-rendition-provider

Conversation

@salmonumbrella

@salmonumbrella salmonumbrella commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

What changed

Docbank now has a hosted Reducto adapter for authorized PDFs and slide decks. Complete output keeps page or slide evidence; URL results, returned PDFs or images, partial output, unsupported spreadsheet provenance, malformed responses, and changed runtime identity fail closed.

Accepted jobs get a durable secret-free resume handle before polling. A restart resumes the known job with cumulative usage history instead of paying for an ambiguous resubmission. The final receipt keeps the original authorized start and records when completion was actually observed.

Why

Reducto’s async API needs the same exact-byte, destination, evidence, cost, and lifecycle boundaries as every other hosted provider. In particular, an uncertain response must never turn into a duplicate paid parse.

Usage

For library use, construct the client with reducto.NewProvider(profile, secrets, transport). The profile pins the hosted identity, PDF/PPTX support, and finite limits; the injected transport owns the approved destination policy.

Durable resume is consumed by the R10 worker contract. Daemon, API, and CLI registration is intentionally deferred to S1–S3.

Part of #176 (R14). Stacks on #205.

@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (39d6041)

High-severity upload-integrity flaw and two medium-severity retry-classification issues require fixes before merge.

High

  • document/bridge/client.go:261 — Upload bytes cross the provider boundary before their SHA-256 is verified, with only the length checked afterward. Corrupted or mutable AuthorizedUpload content can therefore be transmitted under a manifest claiming a different identity; oversized streams also transmit one extra byte before rejection.
    • Fix: Snapshot metadata, spool and verify the complete length and SHA-256 locally before creating the HTTP request, and test that same-length mismatches and oversized streams never reach the transport.

Medium

  • document/provider.go:323, document/reducto/client.go:398 — A nil resume checkpoint becomes a successful no-op, making Reducto believe its job handle was durably persisted. Later polling failures or exhaustion are classified as retryable transient errors even though the caller cannot resume, potentially submitting and billing a duplicate job. Direct Render calls have the same unsafe classification at the polling limit.

    • Fix: Preserve a nil checkpoint or require a real callback. Classify every post-submission failure as ambiguous_submission unless the handle was actually persisted.
  • document/docling/client.go:217, document/datalab/client.go:761, document/marker/client.go:646 — Internally imposed deadlines are classified as caller cancellation, and Docling reports a still-pending job at the polling limit as retryable capacity exhaustion. Because these providers cannot resume the known remote operation, the worker may either fail terminally or submit a duplicate while the original is still running.

    • Fix: Distinguish caller cancellation from internal deadlines and track whether submission may have occurred. After submission, classify internal timeouts and polling exhaustion as ambiguous_submission.

Reviewers: 2 done | Synthesis: codex, 14s | Total: 27m10s

@salmonumbrella
salmonumbrella force-pushed the feat/reducto-rendition-provider branch from 39d6041 to b598d4c Compare August 25, 2026 06:48
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (b598d4c)

Medium-severity issues remain in metadata extraction and Reducto resume validation.

Medium

  • internal/processing/source_metadata.go:321 — XML character data is copied into every open ancestor’s buffer without nesting or aggregate-size limits. Deeply nested metadata can amplify bounded input into excessive memory use and crash the daemon. Limit nesting and accumulated text, ideally collecting only recognized elements, and stop extraction with a warning when limits are exceeded.

  • document/reducto/client.go:805 — Reducto resume handles are not bound to the source digest or authorization that created them. Reusing a valid handle with a different same-sized source can attach the original job’s rendition to the replacement source. Add and validate a versioned, stable source/authorization identity in the resume payload, with tests rejecting cross-source reuse.

  • internal/processing/source_metadata.go:174 — String-list metadata validates UTF-8 and item count but not per-value or total encoded size. Oversized headers or PDF metadata can fail during canonical marshaling, causing permanent backfill retries without publishing metadata or a warning. Enforce per-value and aggregate encoded-size limits while collecting lists, dropping excessive data with an extraction warning.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 24m44s

@salmonumbrella salmonumbrella changed the title Add hosted Reducto rendition provider Add the hosted Reducto rendition provider Aug 25, 2026
@salmonumbrella
salmonumbrella force-pushed the feat/reducto-rendition-provider branch from b598d4c to dfba112 Compare August 25, 2026 10:38
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (dfba112)

Review verdict: Three medium-severity correctness issues should be addressed before merging.

Medium

  • internal/processing/source_metadata.go:174 — List metadata validates UTF-8 and item count, but not each item’s byte limit. An oversized entry creates a record rejected by canonical marshaling, leaving the target pending for repeated retries instead of omitting the field with a warning. Enforce MaxSourceMetadataValueBytes for every list entry and add a backfill test with oversized embedded list metadata.

  • internal/processing/source_metadata.go:798 — Eight-digit timestamps are accepted without checking whether they represent valid dates. Values such as 20241399 become invalid normalized timestamps that later fail marshaling and prevent metadata publication. Validate compact dates with time.Parse, omit invalid values with a warning, and test malformed embedded dates.

  • internal/store/source_metadata.go:201 — Historical content-version reads attach the node’s latest unsuperseded provenance rather than provenance associated with the requested version. After multiple ingests, an older version can report the newer version’s source path, modification time, and ingestion time. Persist and query a provenance-to-content-version association, or omit provenance facts when they cannot be tied to the requested version; test two successive ingests followed by an older-version lookup.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 14m21s

@salmonumbrella
salmonumbrella force-pushed the feat/reducto-rendition-provider branch from dfba112 to 798a29f Compare August 25, 2026 11:12
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (798a29f)

Verdict: Changes require fixes for one high-severity security issue and four medium-severity reliability/contract issues.

High

Windows upload spools may inherit permissive ACLs

Location: document/upload/spool_windows.go:29

Authorize accepts any absolute spool directory, but Windows ignores the Unix-style 0700 and 0600 modes. The spool directory and source file inherit the selected parent’s DACL, potentially allowing less-privileged local users to enumerate temporary uploads and read document contents.

Create the directory with a protected, current-user-only DACL using the repository’s winsecurity.MkdirPrivatePinnedAt pattern, retaining the pin for its lifetime. Alternatively, reject parents that fail restricted-DACL validation before writing source bytes.

Medium

Non-resumable operations may continue after authorization expires

Locations: document/bridge/client.go:124; document/plaintext/provider.go:92

Authorization expiry is checked only when an operation begins. Processing and artifact retrieval can therefore continue beyond ExpiresAt, even though late results are rejected.

Bound the operation context by the earlier of the configured timeout and ExpiresAt, returning a classified expiry error when that deadline is reached.

Bridge context failures are returned without provider classification

Location: document/bridge/client.go:176

Raw ctx.Err() values are returned on context failure. RenderRendition treats these as unclassified provider errors, so an internal bridge timeout can incorrectly place a worker job into operator_required.

Convert every context exit into the appropriate top-level RenditionProviderError, preserving the context error as its cause.

OpenAPI artifact media type conflicts with client validation

Location: document/bridge/openapi.yaml:74

The contract specifies artifact downloads as application/octet-stream, while fetchArtifact requires the response Content-Type to match the artifact’s declared media type. Contract-compliant bridges may consequently be rejected for JSON, image, and other artifacts.

Align the OpenAPI response with the dynamically declared media type, or permit application/octet-stream in the client while retaining checksum and manifest-media-type validation.

Resume path can lose checkpoint callback failures

Location: document/execution.go:275

ResumeRendition can accept a successful result when a provider ignores an error returned by the checkpoint callback, even though durable checkpointing or fencing failed.

Record and return the first checkpoint error after RenderResumable, matching RenderRenditionWithResume, and add coverage for a provider that ignores the callback error.


Reviewers: 2 done | Synthesis: codex, 15s | Total: 20m59s

@salmonumbrella
salmonumbrella force-pushed the feat/reducto-rendition-provider branch from 798a29f to eeaef50 Compare August 25, 2026 11:40
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (eeaef50)

The changed provider, credential, consent, upload, and API paths preserve their intended trust boundaries, with no plausible exploit path identified.

No issues found.


Reviewers: 2 total (1 done, 1 skipped) | Synthesis: codex | Total: 14m35s

@salmonumbrella
salmonumbrella force-pushed the feat/reducto-rendition-provider branch 6 times, most recently from 7326582 to 80d25ed Compare August 25, 2026 12:51
@salmonumbrella
salmonumbrella force-pushed the feat/reducto-rendition-provider branch from 80d25ed to 6409936 Compare August 25, 2026 13:10
@roborev-ci

roborev-ci Bot commented Aug 25, 2026

Copy link
Copy Markdown

roborev: Combined Review (6409936)

Code changes require fixes for three medium-severity reliability and resource-bound issues; no security regressions were identified.

Medium

  • cmd/docbank/daemon.go:153 — The rendition runtime registry is created empty and checked immediately, with no production registration path. Ready() is always false, so the daemon never starts the rendition worker and queued or restored jobs remain unprocessed. Construct and register configured provider runtimes before checking Ready(), and test daemon startup with a configured provider and queued job.

  • internal/processing/source_metadata.go:174 — List entries and source-field labels are appended without enforcing codec size limits. An oversized iCalendar organizer value or parameterized field name can make MarshalSourceMetadataV1 reject the entire record, causing indefinite retries. Validate labels and individual list values before appending them, and emit sanitized, bounded warnings for omitted metadata.

  • internal/processing/source_metadata.go:337 — XML character data is copied into every open ancestor without nesting or aggregate-text limits. A small, deeply nested OOXML or XMP document can trigger gigabytes of allocations and terminate automatic metadata backfill. Bound XML depth and total captured text—preferably collecting only supported fields—and return a metadata warning when limits are exceeded.


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

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