WS-ART-001-03B3B3B: add bounded DOCX extraction - #234
Conversation
|
Warning Review limit reached
Next review available in: 28 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 (7)
📝 WalkthroughWalkthroughThis change adds bounded DOCX extraction through the isolated worker, canonical JSON output, omission metadata, policy ChangesDOCX extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant guide_extraction_worker
participant validate_ooxml
participant extract_docx
participant guide_extraction_service
guide_extraction_worker->>validate_ooxml: Validate DOCX payload
guide_extraction_worker->>extract_docx: Extract validated DOCX
extract_docx-->>guide_extraction_worker: Return canonical output and omission_facts
guide_extraction_worker-->>guide_extraction_service: Return extraction result with policy v3
guide_extraction_service->>guide_extraction_service: Persist and compare omission_facts
Possibly related PRs
🚥 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 (2)
backend/tests/test_guide_docx.py (1)
276-283: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete omission-fact shape after the cross-process round trip.
This test is the only proof that omission facts survive the isolated-child JSON protocol. It checks two keys. The nine-key shape is asserted only in-process at Line 122.
A partial assertion here would not detect a parent that drops or renames keys during result parsing. Assert the full mapping so the wire contract is proven end to end.
💚 Proposed test strengthening
assert (result.status, result.policy_version) == ("extracted", "guide-extraction-v3") assert result.canonical_output == ('{"blocks":[{"text":"visible","type":"paragraph"}]}') - assert result.omission_facts["tracked_deletions"] is True - assert result.omission_facts["omitted"] is True + assert result.omission_facts == { + "truncated": False, + "omitted": True, + "headers": False, + "footers": False, + "comments": False, + "tracked_deletions": True, + "embedded_objects": False, + "hidden_text": False, + "field_instructions": False, + } assert list(tmp_path.iterdir()) == []🤖 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_docx.py` around lines 276 - 283, Strengthen the cross-process assertion in GuideExtractionRunner().extract by comparing result.omission_facts against the complete nine-key omission-fact mapping already asserted in the in-process test around the existing omission-facts coverage. Preserve the current protocol and result assertions while ensuring the full mapping, including all keys and values, is validated after JSON round-trip parsing.backend/app/modules/artifacts/guide_docx.py (1)
295-298: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueResolve part names through the casefolded map to match the validator.
Line 296 builds
nameswithcasefold(). Lines 297 and 298 then read with the exact lowercase literals"word/document.xml"and"word/styles.xml".validate_ooxmlinbackend/app/modules/artifacts/guide_ooxml.pycompares required markers aftercasefold(), so it accepts a package that storesword/Document.xml. The adapter then raisesKeyErrorand reportsmalformed/docx_document_unavailable.The outcome stays bounded and fail-closed, so this is not a security or data-loss defect. It is an asymmetry between the validator contract and the adapter. Map each casefolded name to the stored name and read that name.
♻️ Proposed refactor
- with zipfile.ZipFile(BytesIO(payload)) as archive: - names = frozenset(info.filename.casefold() for info in archive.infolist()) - document = archive.read("word/document.xml") - styles = archive.read("word/styles.xml") if "word/styles.xml" in names else None + with zipfile.ZipFile(BytesIO(payload)) as archive: + stored = {info.filename.casefold(): info.filename for info in archive.infolist()} + names = frozenset(stored) + document = archive.read(stored["word/document.xml"]) + styles = ( + archive.read(stored["word/styles.xml"]) + if "word/styles.xml" in stored + else None + )🤖 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_docx.py` around lines 295 - 298, Update the archive handling around the names map in the DOCX adapter to resolve required parts case-insensitively: map each casefolded filename to its original stored filename, then use those mapped names for reading document.xml and the optional styles.xml. Preserve the existing missing-part behavior and continue passing the resolved content to the validator.
🤖 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_docx.py`:
- Around line 237-254: Update _table_text to obtain rows and cells through the
same content-control-aware traversal used by _contained_blocks, so
w:sdt/w:sdtContent wrappers are unwrapped for both row-level and cell-level
structures while preserving existing paragraph/table handling and omission
tracking. Add a focused test covering wrapped rows and wrapped cells, including
the expected canonical text and omitted state.
- Around line 222-234: Bound recursive traversal depth across _contained_blocks,
_table_text, and the walk helper in _paragraph_text by adding a shared depth
parameter and incrementing it on recursive calls. When the fixed maximum depth
is exceeded, raise or propagate the existing malformed-input handling so deeply
nested documents produce a deterministic malformed outcome rather than
RecursionError/parser_failure; preserve normal traversal behavior below the
limit.
---
Nitpick comments:
In `@backend/app/modules/artifacts/guide_docx.py`:
- Around line 295-298: Update the archive handling around the names map in the
DOCX adapter to resolve required parts case-insensitively: map each casefolded
filename to its original stored filename, then use those mapped names for
reading document.xml and the optional styles.xml. Preserve the existing
missing-part behavior and continue passing the resolved content to the
validator.
In `@backend/tests/test_guide_docx.py`:
- Around line 276-283: Strengthen the cross-process assertion in
GuideExtractionRunner().extract by comparing result.omission_facts against the
complete nine-key omission-fact mapping already asserted in the in-process test
around the existing omission-facts coverage. Preserve the current protocol and
result assertions while ensuring the full mapping, including all keys and
values, is validated after JSON round-trip parsing.
🪄 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: 2bbd7415-51c5-488d-bbec-67b403bbf1b2
📒 Files selected for processing (16)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B3B3B-docx-extractor.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3B-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3B-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B3B3B-pr-trust-bundle.mdbackend/app/modules/artifacts/guide_docx.pybackend/app/modules/artifacts/guide_extraction.pybackend/app/modules/artifacts/guide_extraction_service.pybackend/app/modules/artifacts/guide_extraction_worker.pybackend/scripts/run_test_lanes.pybackend/tests/fixtures/guide_extraction_probe_worker.pybackend/tests/test_artifact_architecture.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_docx.pybackend/tests/test_guide_extraction.pydocs/spec_artifact_storage_service.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
WS-ART-001-03B3B3B PR Trust Bundle
Chunk
WS-ART-001-03B3B3B— DOCX Extractor (L1).Goal and human-approved intent
Add deterministic, bounded DOCX text and table extraction after exact DOCX
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 other formats, or touch contributor ZIPs.
What changed and why
document order with fixed paragraph, row, cell, and nested-table semantics.
passive objects, hidden text, and field instructions.
guide-extraction-v3and bound omission facts toimmutable persistence and replay checks.
validator and DOCX adapter imports confined to the isolated child.
exact storage-service contract.
Design chosen and alternatives rejected
The isolated worker composes the shared OOXML validator with the DOCX adapter
before descriptor-only seccomp. The adapter receives an explicit validator,
then parses only validated
word/document.xmland bounded style facts. Theparent accepts only the exact result and omission schemas. Rejected alternatives
were upload-request parsing, direct provider access, a second OOXML validation
path, raw binary agent input, partial output, or generic document authority.
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. Unsupported and malformed content remains a bounded internal artifact
outcome, not a guide-insufficiency decision.
Acceptance criteria proof
structures have exact canonical output tests.
objects, fields, direct/inherited style-hidden text, and document-default
hidden text are omitted and recorded.
cleanup; architecture tests prove worker-only adapter/validator imports.
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. Invalid result-shape fixtures carry valid
omission facts so they still reach their intended validation branches. The new
DOCX module joins the existing
shared_foundationslane.Reviewer results
Architecture, CI integrity, test delta, and docs pass. Security, QA, senior
engineering, product/ops, and reuse/dedup pass with only documented low risks.
All blocking deletion traversal, hidden-style cascade, isolated-runner, shared-
loader reuse, protocol-test, and documentation findings were repaired and
re-reviewed.
External review
CodeRabbit and hosted GitHub checks are pending on the PR head. Any valid
findings will be repaired and recorded before merge readiness is reported.
Remaining risks and follow-up
DOCX has a large visibility model; this chunk intentionally supports a bounded
subset and treats
w:vanishpresence conservatively, including explicit falsevalues. Future formats should reuse the same isolated result protocol without
growing duplicated schemas. PPTX/XLSX, durable sufficiency continuation, and
AUTH activation remain separate later chunks.
Human review focus and merge ownership
Review exact canonical ordering/separators, omission semantics, injected shared
OOXML validation, v3 replay identity, isolation, 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