WS-ART-001-03B3B3C: bounded PPTX guide extraction - #235
Conversation
|
Warning Review limit reached
Next review available in: 12 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 (4)
📝 WalkthroughWalkthroughAdds bounded PPTX slide and notes extraction with deterministic JSON output, omission facts, shared OOXML validation, isolated-worker dispatch, policy versioning, architecture constraints, focused tests, and updated artifact-storage specifications. ChangesPPTX extraction
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[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 (1)
backend/tests/test_guide_pptx.py (1)
205-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
_replace_memberinstead of hand-rebuilding the ZIP archive.In
test_relationship_conflicts_and_orphans_fail_closed, themixed,wrong_type, andrelationship_rootscenarios each manually read every member, mutate one entry, and rewrite a new ZIP archive. The file already defines_replace_memberfor this exact purpose (see lines 104-112), and the sibling testtest_complete_relationship_identity_and_ownership_matrix_fails_closed(lines 282-333) consistently uses it. Reuse_replace_memberhere to remove the duplicated boilerplate.♻️ Proposed refactor for the `mixed` scenario (apply the same pattern to `wrong_type` and `relationship_root`)
- mixed = _package( - [_slide()], - notes={1: _notes()}, - family=_TRANSITIONAL, - ) - with zipfile.ZipFile(BytesIO(mixed)) as source: - members = {info.filename: source.read(info) for info in source.infolist()} - members["ppt/slides/_rels/slide1.xml.rels"] = _relationships( - [("rIdNotes", f"{_STRICT}/notesSlide", "../notesSlides/notesSlide1.xml")] - ) - output = BytesIO() - with zipfile.ZipFile(output, "w", zipfile.ZIP_DEFLATED) as archive: - for name, body in members.items(): - archive.writestr(name, body) + mixed = _replace_member( + _package([_slide()], notes={1: _notes()}, family=_TRANSITIONAL), + "ppt/slides/_rels/slide1.xml.rels", + _relationships([("rIdNotes", f"{_STRICT}/notesSlide", "../notesSlides/notesSlide1.xml")]), + )🤖 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_pptx.py` around lines 205 - 267, Refactor test_relationship_conflicts_and_orphans_fail_closed to use the existing _replace_member helper for the mixed, wrong_type, and relationship_root archive mutations. Remove the repeated ZipFile member-reading and archive-rebuilding boilerplate while preserving each scenario’s modified relationship content and expected PptxExtractionFailure assertions.
🤖 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_pptx.py`:
- Around line 317-323: Move the hidden-slide visibility check from the
_ordered_parts loop to _canonical_slides, where the parsed slide root element is
available, and read its show attribute there. Remove the ineffective
slide_id.get("show") guard while preserving hidden_metadata["hidden_metadata"]
updates for values 0, false, off, or no.
- Around line 180-199: Update _paragraph_text to accept the current traversal
depth as an argument and start its nested walk from that value instead of
resetting to 0. Modify the _paragraphs.walk call site to pass its current depth,
preserving the existing MAXIMUM_NESTING_DEPTH check across the entire traversal.
---
Nitpick comments:
In `@backend/tests/test_guide_pptx.py`:
- Around line 205-267: Refactor
test_relationship_conflicts_and_orphans_fail_closed to use the existing
_replace_member helper for the mixed, wrong_type, and relationship_root archive
mutations. Remove the repeated ZipFile member-reading and archive-rebuilding
boilerplate while preserving each scenario’s modified relationship content and
expected PptxExtractionFailure assertions.
🪄 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: 1dc7c54d-e930-41ae-afc4-74e8169ffc41
📒 Files selected for processing (13)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B3C-pptx-extractor.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3C-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3C-pr-trust-bundle.mdbackend/app/modules/artifacts/guide_extraction.pybackend/app/modules/artifacts/guide_extraction_worker.pybackend/app/modules/artifacts/guide_pptx.pybackend/scripts/run_test_lanes.pybackend/tests/test_artifact_architecture.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_extraction.pybackend/tests/test_guide_pptx.pydocs/spec_artifact_storage_service.md
|
CodeRabbit review repair pushed in
Validation: 21 PPTX tests passed at 94.94% branch coverage; the broader affected suite passed 165 tests; Ruff, dependency approval, stale-contract, Markdown-link, and diff checks passed. Focused security, QA, and test-delta re-reviews found no remaining blocker. The hosted gates and CodeRabbit re-review are now running on this head. |
2 participants
WS-ART-001-03B3B3C PR Trust Bundle
Chunk
WS-ART-001-03B3B3C— PPTX Extractor (L1).Goal and human-approved intent
Add deterministic, bounded PPTX slide and notes extraction after exact PPTX
classification and the merged OOXML security boundary. Preserve original
verified guide bytes as authoritative. This hidden chunk must not activate
AUTH, invoke guide sufficiency, add XLSX/image behavior, or touch contributor
submission ZIPs.
What changed and why
extraction, deterministic compact JSON, and fixed omission evidence.
guide-extraction-v4and bound its complete resultschema to immutable persistence and replay checks.
import confined to the isolated child.
DB-backed replay cases, and the exact storage-service contract.
Design chosen and alternatives rejected
The adapter accepts only bytes that pass the shared OOXML boundary. It resolves
the exact presentation relationship graph, traverses only approved shape-tree
text bodies and tables, and returns canonical content plus a fixed omission
map. Rejected alternatives were request-path parsing, direct provider access,
generic ZIP handling, raw binary agent input, broad XML scanning, partial
output, or a second authorization/OOXML protocol.
Scope control and product behavior
Only hidden guide-source extraction changes. There is no route, authorization
activation, Celery continuation, sufficiency invocation, provider read, guide
binding, submission, checker, review, contribution, payment, or reputation
change. Artifact/parser failures remain bounded internal outcomes rather than
guide-insufficiency decisions.
Acceptance criteria proof
semantics, Strict/Transitional relationships, and empty structures have
deterministic canonical-output tests.
objects use an exact boolean omission schema bound to v4 evidence.
orphans, shared ownership, namespace mixing, and invalid parts fail closed.
fail without partial agent input.
architecture tests prove worker-only adapter imports.
obsolete PPTX policy evidence as a replay target.
Tests and checks
git diff --check— pass.and semantic-lane proof; no local full-suite run was used.
Test delta and CI integrity
No test, assertion, lane, workflow, dependency rule, or coverage threshold was
removed, skipped, or weakened. The new PPTX module joins the existing
shared_foundationslane. No dependency or workflow file changed.Reviewer results
Architecture, security, senior engineering, CI integrity, test delta, and docs
pass. QA, product/ops, and reuse/dedup pass with only documented low risks. All
blocking traversal, non-text leakage, relationship, visibility, depth, and
contract/test findings were repaired and re-reviewed.
External review
CodeRabbit and hosted GitHub checks have not yet run on the committed PR head.
They remain required before human merge approval.
Remaining risks and follow-up
DB-backed replay and repository-wide coverage evidence is hosted-only. If the
next XLSX adapter repeats the small OOXML loader or serializer patterns, assess
a bounded shared helper then rather than widening this chunk. XLSX, image,
durable sufficiency continuation, AUTH activation, and legacy cutover remain
separate later chunks.
Human review focus and merge ownership
Review exact relationship ownership/order, structure-aware text eligibility,
omission semantics, v4 replay identity, depth/output limits, and absence of AUTH
or sufficiency activation. A human owns merge approval; the agent will not
merge this PR.
Summary by CodeRabbit
New Features
Documentation
Tests