fix(render): the PDF manifest's complete flag must mean something too - #152
Merged
Conversation
#141 made `complete` derived in both URL backends. The PDF backend writes the same key into the same filename and still hardcodes it, so the claim #139 makes about the manifest contract still holds on this path: a consumer cannot tell a whole document from a fragment of one. render_pdf takes a `pages` selection. When it is given, only those pages are written — and the manifest still said `complete: true`, with `total_pages` reporting the size of the subset, so nothing in the directory distinguished "this three-page document" from "three pages of some longer document". - `complete` is now `pages is None`: only a whole-document render may claim it. An explicit range counts as a selection even when it happens to cover every page, because the backend has not checked it against the document's length. - `requested_pages` records the selection, so a consumer can see what was asked for instead of being told out of band — the same move as recording `tile_height` in the URL manifests. No in-repo caller passes `pages` today; it is reachable through the public render_pdf API, which is exported from pixelrag_render. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
CI runs `uv sync --extra dev`, and pdf2image ships in the optional `pdf` extra, so the three new tests raised ImportError on the runner rather than exercising anything. They passed locally only because this machine happens to have pdf2image and poppler installed. Guard with importorskip plus a pdftoppm check, the way the suite already treats the serve and index extras. With the extra present the tests run; with it absent they skip instead of failing. Trade-off worth naming: on CI as configured they now skip, so they do not gate the backend. Making them gate means syncing `--extra pdf` and putting poppler on the runner — a CI change beyond this fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#141 made
completederived in both URL backends. The PDF backend writes the same key into the same filename and still hardcodes it, so the claim #139 makes about the manifest contract still holds on this path: a consumer cannot tell a whole document from a fragment of one.render_pdftakes apagesselection. When it is given, only those pages are written — and the manifest still saidcomplete: true, withtotal_pagesreporting the size of the subset, so nothing in the directory distinguished "this three-page document" from "three pages of some longer document".completeis nowpages is None: only a whole-document render may claim it. An explicit range counts as a selection even when it happens to cover every page, because the backend has not checked it against the document's length.requested_pagesrecords the selection, so a consumer can see what was asked for instead of being told out of band — the same move as recordingtile_heightin the URL manifests.Scope
No in-repo caller passes
pagestoday; it is reachable through the publicrender_pdfAPI, which is exported frompixelrag_renderand documents the parameter. So this is a public-API contract fix, not a live in-repo failure.Tests
Three new tests in
tests/test_pdf_manifest.py, written against the old backend first and red on all three. The 9 existing render tests, including #141's manifest tests, still pass.