WS-ART-001-03B3B2: add bounded PDF guide extraction - #231
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe change adds bounded PDF extraction with pinned ChangesPDF extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ExtractionService
participant IsolatedWorker
participant PDFAdapter
ExtractionService->>IsolatedWorker: start PDF extraction
IsolatedWorker->>IsolatedWorker: apply resource limits
IsolatedWorker->>PDFAdapter: lazy-load adapter
IsolatedWorker->>IsolatedWorker: install seccomp
IsolatedWorker->>PDFAdapter: parse PDF payload
PDFAdapter-->>IsolatedWorker: canonical JSON or bounded failure
IsolatedWorker-->>ExtractionService: persist extraction result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/spec_artifact_storage_service.md (1)
1292-1305: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the internal object-inspection bound.
This section enumerates concrete numeric bounds for the PDF adapter (500 pages, etc.), but omits the
_MAXIMUM_INSPECTED_OBJECTS(100,000) bound and itspdf_object_limitfailure code implemented inguide_pdf.py. Add it here for consistency with the rest of this exhaustively bounded specification.As per coding guidelines,
docs/**/*.mdshould "Prefer evidence-backed documentation over vague product claims"; documenting this concrete, already-enforced bound keeps the spec fully evidence-backed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/spec_artifact_storage_service.md` around lines 1292 - 1305, Update the PDF extraction specification near the existing page and failure-bound descriptions to document the enforced _MAXIMUM_INSPECTED_OBJECTS limit of 100,000 objects and identify pdf_object_limit as the corresponding bounded internal failure outcome, preserving the existing claims about other limits and sanitized agent-visible results.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/modules/artifacts/guide_pdf.py`:
- Around line 112-127: Update the exception handling in extract_pdf to include
the concrete pypdf parsing failures KeyError and IndexError in the existing
malformed-PDF except clause, so bounded_extract receives
PdfExtractionFailure("malformed", "invalid_pdf") rather than classifying these
cases as parser failures.
---
Nitpick comments:
In `@docs/spec_artifact_storage_service.md`:
- Around line 1292-1305: Update the PDF extraction specification near the
existing page and failure-bound descriptions to document the enforced
_MAXIMUM_INSPECTED_OBJECTS limit of 100,000 objects and identify
pdf_object_limit as the corresponding bounded internal failure outcome,
preserving the existing claims about other limits and sanitized agent-visible
results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e73e743e-ef15-43d4-8e26-3731cf104b6a
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B2-pdf-extractor.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B2-pr-trust-bundle.mdbackend/app/modules/artifacts/guide_extraction.pybackend/app/modules/artifacts/guide_extraction_service.pybackend/app/modules/artifacts/guide_extraction_worker.pybackend/app/modules/artifacts/guide_pdf.pybackend/pyproject.tomlbackend/scripts/run_test_lanes.pybackend/tests/test_artifact_architecture.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_extraction.pybackend/tests/test_guide_pdf.pydocs/spec_artifact_storage_service.md
2 participants
WS-ART-001-03B3B2 PR Trust Bundle
Chunk
WS-ART-001-03B3B2— PDF Extractor (L1).Goal and human-approved intent
Install only the dependency approved in merged 03B3B1 and add bounded PDF text
extraction inside the existing hidden, isolated guide-extraction framework.
Guide uploads remain independent from contributor ZIP submissions. This chunk
does not activate AUTH, invoke sufficiency, or begin OOXML/image work.
What changed and why
pypdf==6.14.2wheel to runtime dependencies andthe lockfile.
attached, embedded, external, and active PDFs; enforces 500 pages; and emits
deterministic compact JSON with explicit page boundaries.
installs seccomp before reading or parsing untrusted bytes.
unsupportedattempts cannot replay.Exact-lineage retry custody resets once to the new policy and preserves the
old immutable evidence.
shared_foundationslane andupdated the artifact specification and initiative status.
Design chosen
The existing
GuideExtractionRunnerand child protocol remain authoritative.The parent sees only the server-derived
pdfclassification and bounded result.There is no second parser runtime, direct provider access, raw agent input, or
new authorization path. Canonical output is
{"pages":[...]}.Rejected alternatives were parsing during upload, loading
pypdffor everyformat, retaining PDF as policy-v1 unsupported, accepting partial extraction,
or allowing arbitrary PDF actions.
Scope control and product behavior
All changed files are in the corrected chunk contract. The only contract
additions are the existing persistence service, its policy replay test, and the
canonical lane inventory required to make the original acceptance criteria
executable. No provider, AUTH, Celery, submission, review, compensation,
reputation, OCR, OOXML, image, route, or agent-assembly behavior changes.
PDF failures remain internal bounded extraction outcomes and never become guide
insufficiency decisions. Only successful current-policy canonical content can
later enter sufficiency.
Acceptance criteria proof
JavaScript, named/launch/submit/import/external actions, URI links, widgets,
and rich media are rejected with bounded codes.
limits, trusted import, seccomp, then untrusted parsing.
and protocol validation reuse and exercise the existing framework proofs.
replayed after PDF policy v2 and that v2 success becomes the replay target.
Tests and checks
uv run python scripts/check_guide_extractor_dependencies.py— pass.uv lock --check— pass; wheel URL/hash matches the approved manifest.uv run ruff check app tests scripts— pass.order/action cases; targeted final repair suite — 31 passed.
test_guide_pdf.pyis inshared_foundations.git diff --check— pass.GitHub Backend gate; no local full-suite run was used.
Test delta and CI integrity
No test or assertion was removed, skipped, or weakened. New adversarial cases
cover actual action annotations rather than only synthetic root keys. No
workflow, lane count, coverage threshold, or failure behavior changed. Hosted
installation remains protected by the exact direct wheel URL/hash and the
dependency checker; the lockfile records the same bytes.
Reviewer results
import-style proof hardening item, also repaired.
final re-review pass.
re-review pass.
External review
CodeRabbit and hosted GitHub gates have not run yet. Their findings remain
external merge evidence and must be checked after the draft PR opens.
Remaining risks and follow-up
pypdfis an untrusted-format parser, so OS termination remains an essentialcontrol. The trusted dependency import intentionally precedes descriptor-only
seccomp because Python import needs filesystem reads; resource limits are
already installed and no untrusted bytes are read until after seccomp.
03B3B3A (OOXML security) and 03B3B4 (image metadata) remain separate chunks.
03B4 sufficiency continuation cannot start until all required format chunks
merge.
Human review focus and merge ownership
Review the exact wheel/hash, active-content deny semantics, 500-page boundary,
trusted-import/seccomp ordering, PDF policy-v2 replay reset, and deterministic
page JSON. A human owns merge approval; the agent will not merge this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation