WS-ART-001-03B3B4: bounded image metadata extractors - #239
Conversation
|
Warning Review limit reached
Next review available in: 34 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 (5)
📝 WalkthroughWalkthroughAdds isolated, bounded structural metadata extraction for PNG, JPEG, and WebP images. The change adds Pillow dependency and platform validation, canonical v6 metadata, worker integration, stable failures, and extensive format, resource, isolation, and architecture tests. ChangesImage metadata extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GuideExtractionWorker
participant GuideImages
participant Pillow
GuideExtractionWorker->>GuideImages: Submit image bytes and detected format
GuideImages->>GuideImages: Validate structural headers and limits
GuideImages->>Pillow: Decode within bounded pixel limits
Pillow-->>GuideImages: Return decoded image facts
GuideImages-->>GuideExtractionWorker: Return canonical metadata or stable failure
Possibly related PRs
Suggested reviewers: 🚥 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/tests/test_guide_images.py (1)
175-183: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
monkeypatch.setattrfor the module constant.The test mutates
image_module.MAXIMUM_PIXELSand restores it infinally. The local namemonkeypatch_limitholds a saved value, not a fixture, which is confusing.monkeypatch.setattrperforms the same override and guarantees restoration through pytest teardown.♻️ Proposed refactor
-def test_exact_limit_constants_and_boundary_transitions() -> None: +def test_exact_limit_constants_and_boundary_transitions( + monkeypatch: pytest.MonkeyPatch, +) -> None: @@ - monkeypatch_limit = image_module.MAXIMUM_PIXELS - try: - image_module.MAXIMUM_PIXELS = 5 - image_module._enforce_limits(ImageStructuralFacts("png", 5, 1, 1, "rgb", 8, False)) - with pytest.raises(ImageExtractionFailure) as one_over: - image_module._enforce_limits(ImageStructuralFacts("png", 6, 1, 1, "rgb", 8, False)) - assert one_over.value.code == "image_pixel_limit" - finally: - image_module.MAXIMUM_PIXELS = monkeypatch_limit + monkeypatch.setattr(image_module, "MAXIMUM_PIXELS", 5) + image_module._enforce_limits(ImageStructuralFacts("png", 5, 1, 1, "rgb", 8, False)) + with pytest.raises(ImageExtractionFailure) as one_over: + image_module._enforce_limits(ImageStructuralFacts("png", 6, 1, 1, "rgb", 8, False)) + assert one_over.value.code == "image_pixel_limit"🤖 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 `@backend/tests/test_guide_images.py` around lines 175 - 183, Update the test around _enforce_limits to use the pytest monkeypatch fixture’s setattr method for temporarily overriding image_module.MAXIMUM_PIXELS. Remove the manual monkeypatch_limit save and try/finally restoration, while preserving both the boundary-success assertion and the image_pixel_limit failure assertion.backend/tests/test_artifact_architecture.py (1)
641-673: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the Pillow confinement assertion into its own test.
The function asserts two independent invariants:
guide_imagesimporter confinement andPILimporter confinement. It also walksAPP_ROOT.rglob("*.py")twice. The neighboringtest_docx_adapter_is_confined_to_the_isolated_workerandtest_xlsx_adapter_is_confined_to_the_isolated_workereach assert one invariant. A separate test keeps a failure report specific to the invariant that broke.♻️ Proposed refactor
assert adapter_importers == {"modules/artifacts/guide_extraction_worker.py"} + +def test_pillow_is_confined_to_the_worker_and_the_image_adapter() -> None: pillow_importers: set[str] = set() for path in APP_ROOT.rglob("*.py"):🤖 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 `@backend/tests/test_artifact_architecture.py` around lines 641 - 673, Split test_image_adapter_is_confined_to_the_isolated_worker into separate tests: keep the guide_images importer assertion in the existing test and move the PIL importer scan/assertion into a new dedicated test. Preserve both expected importer sets while ensuring each test reports only its respective confinement failure.backend/app/modules/artifacts/guide_images.py (1)
57-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate
_failwithNoReturn.
_failalways raises. TheNonereturn type hides that fact from type checkers. WithNoReturn, static analysis can prove thatparseris notNoneafter line 316 and thatwidth/heightare bound in_webp.♻️ Proposed refactor
+from typing import NoReturn + -def _fail(status: str, code: str) -> None: +def _fail(status: str, code: str) -> NoReturn: raise ImageExtractionFailure(status, code)🤖 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 `@backend/app/modules/artifacts/guide_images.py` around lines 57 - 58, Update the `_fail` helper’s return annotation from `None` to `NoReturn`, importing `NoReturn` from the appropriate typing module if needed. Preserve its existing behavior of always raising `ImageExtractionFailure` so static analysis can recognize subsequent control flow as unreachable after `_fail`.
🤖 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
@.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B4-pr-trust-bundle.md:
- Line 63: Update the suite description in the review document to hyphenate the
compound modifier, changing “Image/dependency focused suite” to
“Image/dependency-focused suite” while preserving the surrounding coverage
statement.
In `@backend/app/modules/artifacts/guide_images.py`:
- Around line 320-335: Update _expected_modes() so the grayscale_alpha PNG
color-model mapping accepts both the existing "LA" mode and Pillow’s "RGBA" mode
for 16-bit grayscale+alpha images, while leaving other expected-mode mappings
unchanged.
---
Nitpick comments:
In `@backend/app/modules/artifacts/guide_images.py`:
- Around line 57-58: Update the `_fail` helper’s return annotation from `None`
to `NoReturn`, importing `NoReturn` from the appropriate typing module if
needed. Preserve its existing behavior of always raising
`ImageExtractionFailure` so static analysis can recognize subsequent control
flow as unreachable after `_fail`.
In `@backend/tests/test_artifact_architecture.py`:
- Around line 641-673: Split
test_image_adapter_is_confined_to_the_isolated_worker into separate tests: keep
the guide_images importer assertion in the existing test and move the PIL
importer scan/assertion into a new dedicated test. Preserve both expected
importer sets while ensuring each test reports only its respective confinement
failure.
In `@backend/tests/test_guide_images.py`:
- Around line 175-183: Update the test around _enforce_limits to use the pytest
monkeypatch fixture’s setattr method for temporarily overriding
image_module.MAXIMUM_PIXELS. Remove the manual monkeypatch_limit save and
try/finally restoration, while preserving both the boundary-success assertion
and the image_pixel_limit failure assertion.
🪄 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: 0f10b49d-09cc-4583-9a8e-310d7113335d
⛔ Files ignored due to path filters (1)
backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B4-image-metadata-extractors.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B4-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B4-pr-trust-bundle.mdbackend/app/modules/artifacts/guide_extraction.pybackend/app/modules/artifacts/guide_extraction_worker.pybackend/app/modules/artifacts/guide_images.pybackend/pyproject.tomlbackend/scripts/check_guide_extractor_dependencies.pybackend/scripts/run_test_lanes.pybackend/tests/test_artifact_architecture.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_extraction.pybackend/tests/test_guide_extractor_dependencies.pybackend/tests/test_guide_images.pydocs/operations_backend_testing.mddocs/spec_artifact_storage_service.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
WS-ART-001-03B3B4 PR Trust Bundle
Chunk
WS-ART-001-03B3B4— Image Metadata Extractors (L1).Goal and human-approved intent
Install only the pre-approved Pillow artifact and add bounded PNG, JPEG, and
WebP structural metadata extraction after exact guide-source classification.
Original verified bytes remain authoritative. This hidden chunk does not add
OCR, raw pixels, AUTH activation, guide sufficiency, submission ZIP handling,
or a second parser/storage path.
What changed and why
structure before Pillow decoder entry and emits only canonical metadata.
guide-extraction-v6through the existingextraction worker and immutable persistence/replay path.
Pillow wheels and made unsupported Python/platform combinations fail closed.
semantic-lane, and documentation proof for all three formats.
Design chosen and alternatives rejected
The existing worker loads the approved Pillow plugins before descriptor-only
seccomp, then the image adapter re-parses PNG chunks/CRCs, JPEG SOF structure,
or WebP RIFF structure before
Image.open. Pillow must agree on format,dimensions, mode, and frame count. Rejected alternatives were request-path
parsing, trusting MIME/classifier facts, direct provider access, OCR, pixel
loading, metadata propagation, unapproved wheels, and generic download or AUTH
capabilities.
Scope control and product behavior
Only hidden guide-source extraction changes. Guide source items may be PNG,
JPEG, or WebP; contributor submissions remain one outer ZIP. Image output is
typed structural metadata and cannot satisfy required textual guide semantics.
No route, AUTH action, Celery continuation, agent invocation, submission,
review, contribution, payment, or reputation behavior changes.
Acceptance criteria proof
enums; omission facts remain exactly non-truncated/non-omitted.
tRNS, APNG sequences/count/bounds;JPEG validates SOF precision/components; WebP validates RIFF, VP8/VP8L/VP8X,
alpha, animation headers, frame bounds, and counts before decoder entry.
including exact one-over and smallest feasible production pixel cases.
diagnostics, and sentinels never reach canonical output or error protocol.
runtime declarations, and rejects unsupported Python, OS, architecture, or
libc facts.
Tests and checks run
uv lock --check, Ruff, stale-contract scan, Markdown links,and
git diff --check— pass.203 passed; image branch coverage 90.76%.
passed.
hosted Backend/Agent Gates; no local full-suite run was used.
Test delta and CI integrity
No test, assertion, lane, workflow, or coverage threshold was removed, skipped,
or weakened. All three formats have direct stable-code tests, real isolated
worker tests, and DB persistence/replay parametrization. The image module joins
the existing semantic lane. Hosted Artifact 90 percent and repository 78
percent coverage gates remain unchanged. The large lockfile deletion is
mechanical pruning of Python 3.13+ wheel records after narrowing the supported
backend range to the approved 3.11/3.12 native-wheel matrix.
Reviewer results
Plan, architecture, security, product/ops, QA, senior engineering, docs,
reuse/dedup, CI integrity, and test-delta reviews pass. Valid findings for PNG transparency,
APNG declaration validation, runtime/platform enforcement, missing runtime
dependency detection, pixel-boundary proof, cross-format isolated/persistence
proof, exact documentation, and lockfile scope were repaired or verified and
re-reviewed.
External review
CodeRabbit and hosted GitHub checks have not yet reviewed the branch. They
supplement but do not replace internal review, and valid findings must be
repaired before human merge approval.
Remaining risks and follow-up work
Classifier and child image limits are intentionally independently enforced, so
tests pin their agreement to prevent drift. ART-03B4 must consume v6 image JSON
as typed metadata rather than textual sufficiency input. AUTH activation and
legacy cutover remain later separate chunks.
Human review focus and merge ownership
Review pre-decoder hostile-container validation, metadata minimization, native
wheel/runtime enforcement, v6 replay identity, and the absence of OCR, raw
pixels, AUTH activation, or sufficiency invocation. A human owns merge
approval; the agent will not merge this PR.
Summary by CodeRabbit
New Features
Documentation
Tests