diff --git a/.github/workflows/agent-review-runtime-quality-ci.yml b/.github/workflows/agent-review-runtime-quality-ci.yml index a601e25522..2f8f2c9fcc 100644 --- a/.github/workflows/agent-review-runtime-quality-ci.yml +++ b/.github/workflows/agent-review-runtime-quality-ci.yml @@ -16,7 +16,10 @@ on: - "scripts/ci/noema-document-reader/package.json" - "scripts/ci/noema-document-reader/package-lock.json" - "tests/test_noema_document_review_context.py" + - "tests/test_noema_review_document_multimodal.py" + - "docs/doctoring/noema-document-multimodal-proofreading.md" - "docs/doctoring/noema-review-token-lifetime.md" + - "scripts/ci/noema_review_gate.py" - "docs/product-technical-gap-baseline.md" - ".github/workflows/opencode-review-dispatch.yml" - "scripts/ci/ensure_rust_llvm19.sh" @@ -198,6 +201,9 @@ jobs: scripts/ci/noema-document-reader/package.json|\ scripts/ci/noema-document-reader/package-lock.json|\ tests/test_noema_document_review_context.py|\ + tests/test_noema_review_document_multimodal.py|\ + docs/doctoring/noema-document-multimodal-proofreading.md|\ + scripts/ci/noema_review_gate.py|\ docs/doctoring/noema-review-token-lifetime.md) noema_suite=true ;; @@ -366,14 +372,16 @@ jobs: tests/test_noema_two_phase_handoff.py \ tests/test_noema_refreshed_app_identity.py \ tests/test_noema_token_lifetime_stale_run_contract.py \ - tests/test_noema_document_review_context.py + tests/test_noema_document_review_context.py \ + tests/test_noema_review_document_multimodal.py python -m compileall -q \ .github/actions/noema-review/two_phase.py \ tests/test_noema_reviewer_token_lifetime.py \ tests/test_noema_two_phase_handoff.py \ tests/test_noema_refreshed_app_identity.py \ tests/test_noema_token_lifetime_stale_run_contract.py \ - tests/test_noema_document_review_context.py + tests/test_noema_document_review_context.py \ + tests/test_noema_review_document_multimodal.py - name: Verify OpenCode Rust coverage toolchain contract if: steps.affected_suites.outputs.opencode == 'true' diff --git a/.github/workflows/trusted-uv-materializer-quality-ci.yml b/.github/workflows/trusted-uv-materializer-quality-ci.yml index db70ec324c..4b8e58092c 100644 --- a/.github/workflows/trusted-uv-materializer-quality-ci.yml +++ b/.github/workflows/trusted-uv-materializer-quality-ci.yml @@ -12,6 +12,7 @@ on: - "tests/test_uv*.py" - "tests/test_repository_branch_coverage_*.py" - "requirements-opencode-review-ci-hashes.txt" + - "requirements-noema-document-ci-hashes.txt" - "pyproject.toml" push: branches: [main] @@ -24,6 +25,7 @@ on: - "tests/test_uv*.py" - "tests/test_repository_branch_coverage_*.py" - "requirements-opencode-review-ci-hashes.txt" + - "requirements-noema-document-ci-hashes.txt" - "pyproject.toml" concurrency: @@ -104,10 +106,12 @@ jobs: with: python-version: "3.14" cache: pip - cache-dependency-path: requirements-opencode-review-ci-hashes.txt + cache-dependency-path: | + requirements-opencode-review-ci-hashes.txt + requirements-noema-document-ci-hashes.txt - name: Install hash-locked quality tooling - run: python -m pip install --disable-pip-version-check --require-hashes -r requirements-opencode-review-ci-hashes.txt + run: python -m pip install --disable-pip-version-check --require-hashes -r requirements-opencode-review-ci-hashes.txt -r requirements-noema-document-ci-hashes.txt - name: Run trusted uv tests with complete branch coverage run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 34281625cb..e93c5e20f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +### Noema DOCX and HWPX figures preserve document relationship order + +- `scripts/ci/noema_review_document.py` resolves embedded DOCX figures through `word/_rels/document.xml.rels` and HWPX figures through `Contents/content.hpf` plus each spine-ordered section's `binaryItemIDRef`. Both paths preserve semantic source order, ignore orphaned archive media, bind stable relationship/media locators, and fail closed on duplicate, unresolved, external, traversal, malformed, or unreadable relationships. The immutable contextual-orchestrator multimodal release/pin and protected merge remain Proposed dependencies of `.github#2281`; this entry does not claim release completion. + ### Noema transport capacity schedules a bounded continuation re-dispatch - After gateway failover, HTTP 429/5xx no longer end only as a permanent required-check failure with `caller attempts=1`. ADR-0031 classifies that class as `provider_capacity_unavailable`, keeps the single gateway request per job, surfaces `provider_attempt_count` from the orchestrator error envelope, and authorizes at most two same-head `repository_dispatch` retries after a capped `Retry-After` or deterministic 60–180 s jitter. Review is never skipped. Refs #2165. diff --git a/docs/doctoring/noema-document-multimodal-proofreading.md b/docs/doctoring/noema-document-multimodal-proofreading.md new file mode 100644 index 0000000000..6f650c8c70 --- /dev/null +++ b/docs/doctoring/noema-document-multimodal-proofreading.md @@ -0,0 +1,83 @@ +# Noema document multimodal envelope and proofreading checklist + +Issue: ContextualWisdomLab/.github#2280 + +## Problem + +Noema's document reader extracted bounded text from `.docx`, `.hwp`, and `.hwpx` +files, but embedded figures never reached the model. A text-only success path +could omit images silently. Document PRs also lacked an explicit proofreading +contract tied to the organization's existing skills. + +## Contract + +1. **Extraction** — `scripts/ci/noema_review_document.py` + - `extract_review_document_bundle()` returns `DocumentExtraction` with text, + declared media count, and OpenAI-style multimodal parts (`text` locator + + `image_url` data-URL per figure). + - `extract_review_document()` remains text-only and **fails closed** when + figures are present. + - `ensure_figures_attached()` rejects partial or missing figure coverage. + - Provisional leaf budget: at most eight figures, each at most 1.5 MiB. + - DOCX figures are resolved only from internal image relationships in + `word/_rels/document.xml.rels` and attached in the source order of + `a:blip` elements in `word/document.xml`. Orphan ZIP media is not source + evidence; missing, external, or out-of-bound relationship targets fail + closed. + - HWPX figures are resolved from the `Contents/content.hpf` manifest and + spine-ordered section XML `binaryItemIDRef` references. Locators preserve + section, paragraph/run/table-cell position, manifest ID, and exact + `BinData` path. Orphans are ignored; duplicate IDs/entries, unresolved or + external references, traversal, malformed XML, and unreadable targets + fail closed. + +2. **Review gate** — `scripts/ci/noema_review_gate.py` + - `fetch_file_review_bundle()` fetches office documents as text + parts. + - `ReviewContext` carries bounded text and flattened multimodal parts from + changed/removed files. + - `call_llm()` emits a multimodal user message when parts exist; otherwise + the legacy string envelope is preserved. + - `document_proofreading_prompt_lines()` encodes the reused skills: + `~/.claude/skills/humanize-korean/SKILL.md` (KO/EN style and terminology + consistency without rewriting substance) and + `~/.agents/skills/source-check/SKILL.md` (citation/page verification; + no arbitrary number or citation edits). + +3. **Fixtures** — synthetic archives only in `tests/test_noema_document_review_context.py`. + Research originals and participant materials are never used. + +## Ownership and delivery state + +- `.github#2281` DOCX implementation evidence is functional commit + `4513708f47ee44b51d431272f91af753dda8a882` (tree + `b3deea106a94799f324cee385f9246db6b443548`). HWPX RED + `21eae9d5e9ce4ee43ee692776a1062c13c9f1a98` precedes GREEN + `768860076068384552c9dfdb02bec1d8996962be` (tree + `3b8b28e6b815ec7b635458c456f0deb921a49ec8`). The branch remains Draft and + Proposed until fresh exact-head Checks and an independent approval exist. +- Multimodal route discovery and fail-closed capability selection belong to + `ContextualWisdomLab/contextual-orchestrator#1203`, current head + `37435b5e82e9fe53abc67b032c67df83425c0250` (tree + `e7bf07b644af5b3ed7b1117b0baeab14767f4ad8`). It ordinarily preserves source + repair `738ab3689d110685ca07f09b7c51031f11d3f07f`, the prior exact evidence, and + RED `7f69bacb0d35f00e6902df8e440efeafbe08dbe3` before its latest judge-failover + GREEN. Its four exact-head workflows are queued and no independent approval + exists. The leaf must consume a protected immutable release/pin; an open + owner PR is not released API authority. + +## Verification + +```bash +PYTHONWARNINGS=error python3 -m pytest -q \ + tests/test_noema_review_document_multimodal.py \ + tests/test_noema_document_review_context.py \ + tests/test_noema_removed_file_context.py +coverage run -m pytest tests && coverage report --show-missing +interrogate +``` + +Multimodal e2e tests assert `image_url` data-URLs in the captured LLM payload +and fail-closed behavior when figures are omitted or media types are unsupported. +The current focused evidence is `79 passed, 2 skipped`; the owned document +reader is `381/381` statements and `134/134` branches (100%). Hosted exact-head +evidence remains required after the branch is published. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index c617e3ad73..5aa316c290 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -7,6 +7,17 @@ 이 문서는 제품·기술·운영 Gap을 현재 문서와 현재 GitHub 상태에 묶어 두는 기준선이다. 새 작업은 먼저 이 문서의 Gap ID를 PR 설명과 테스트 증거에 연결하고, PR의 정확한 exact HEAD·Checks·리뷰를 다시 수집한 뒤 구현한다. 표의 상태는 작성 시점의 관측값이므로, 병합 판단에는 재사용하지 않는다. 이 인벤토리는 스냅샷이며 merge authorization이 아니다. +### 2026-09-20 Noema document multimodal delta + +| Gap ID | 상태 | exact evidence | causal owner / next gate | +|---|---|---|---| +| CONTROL-NOEMA-DOCX-SOURCE-ORDER-01 | **Proposed; DOCX source-order repair implemented on `.github#2281`, not merged** | Functional commit `4513708f47ee44b51d431272f91af753dda8a882`, tree `b3deea106a94799f324cee385f9246db6b443548`; relationship order, orphan exclusion, unresolved relationship and path-escape fixtures; focused evidence is included in the combined `79 passed / 2 skipped` warnings-as-errors lane below. | `.github` owns document extraction. Fresh exact-head hosted Checks and independent approval remain required before ordinary merge. | +| CONTROL-NOEMA-HWPX-SOURCE-ORDER-04 | **Proposed; HWPX source-order repair implemented on `.github#2281`, not merged** | RED `21eae9d5e9ce4ee43ee692776a1062c13c9f1a98` exposed filename-order inference, orphan admission, and missing manifest validation. GREEN `768860076068384552c9dfdb02bec1d8996962be`, tree `3b8b28e6b815ec7b635458c456f0deb921a49ec8`, binds `content.hpf` manifest IDs to spine/section `binaryItemIDRef` order and stable paragraph/run/table-cell locators; duplicate reuse preserves distinct positions. Duplicate IDs/entries, external/traversal/unresolved relationships, malformed XML, missing sections, unreadable/empty media, and unsupported media fail closed. Focused warnings-as-errors: `79 passed / 2 skipped`; owned reader: `381/381` statements and `134/134` branches (100%); compileall and diff check pass. | `.github` owns document extraction. Exact-head hosted protection and independent review remain open; release completion is not claimed. | +| CONTROL-NOEMA-MULTIMODAL-OWNER-02 | **Canonical owner repaired but open/unreleased** | `ContextualWisdomLab/contextual-orchestrator#1203` historical exact `5fa1c8b8f19353d712d6578c4af4c75e96f6988b`, tree `027ee33be749e702290e24ae663879d523974455`, preserves the earlier owner lineage. Judge-failover RED `7f69bacb0d35f00e6902df8e440efeafbe08dbe3` proved a selected free image judge could escape to an ineligible text-only or paid sibling; GREEN `37435b5e82e9fe53abc67b032c67df83425c0250` persists the exact outer free/image-qualified ID set into adapter failover. Later source GREEN `b7440092d1cda47008271ed658fe372f536dd58f` repairs role-ineligible worker preflight, restores complete source blob `1dd97e36fe1579c434413317a5366f9f27d6e766` after the reviewed truncation incident, contains no truncation marker, passes `py_compile`, and completes the three affected warnings-as-errors suites at `91 passed`. Owner-lineage RED `a952041f26ed258c8668fffff3f28e1fe5121355` and GREEN `acb31d0e8929438bf6f46ec12422ad4cc78b91f2` preserve those functional milestones in the canonical owner baseline; formatting-only `5fa1c8b8f19353d712d6578c4af4c75e96f6988b` preserves that earlier evidence. Durable-modality exact `e4846673c110ceaace5cfe300081f02521b4429b` adds case-normalized `input:*` admission after restart while retaining explicit-modality precedence. Current owner exact `ad45a74f0acdddcd023646661413e76a120b88e3` additionally carries mode-alias RED `10f96453a6050ab47575e4975aa068cb4f899e23` and GREEN `c3e4e94cf6566f6a0187c502d279dccc6989d4a1`, so explicit falsey aliases fail closed instead of becoming omitted-mode `auto`; it remains open, Ready/Proposed, unreleased, with fresh hosted acceptance and independent approval pending. | contextual-orchestrator owns modality-aware discovery/routing. Merge under protection, make an immutable release, then advance the `.github` consumer pin and run contract/E2E evidence. No mutable branch/source copy, paid fallback, or Python leaf workaround is authorized. | +| CONTROL-NOEMA-DOC-QUALITY-DEPS-05 | **Proposed repair on `.github#2281`; hosted exact-head acceptance pending** | [Trusted uv run `35459690722`](https://github.com/ContextualWisdomLab/.github/actions/runs/35459690722), job `105941203532`, passed 108 focused tests at 100% coverage and then failed complete-suite collection in 11 document tests because `defusedxml` was absent. RED `4614971896dac7f52871eec4501f32338cca0375` requires the Noema document hash lock in path, cache, and install authority; GREEN `5521b8128c90d374eb05533aa00da31cc619eaa7` installs both immutable locks before the full suite. | `.github` owns the central quality workflow and its complete dependency closure. Fresh exact-head hosted execution and independent review remain required; focused success does not supersede the failed full-suite oracle. | +| CONTROL-STRIX-FIXTURE-BINDER-06 | **Proposed fixture repair on `.github#2281`; hosted exact-head acceptance pending** | [Runtime Quality run `35459690876`](https://github.com/ContextualWisdomLab/.github/actions/runs/35459690876), job `105941203883`, showed the production gate correctly failing closed because synthetic repositories copied `strix_quick_gate.sh` and `strix_model_utils.sh` but omitted required `strix_evidence_binding.py`. GREEN `346e1160174926c59995e748f4d9ecb0652c1f41` materializes the production binder in all 25 model-utils fixture roots; the 24 direct gate roots are covered without weakening production admission. | `.github` owns the Strix gate fixture contract. Fresh exact-head Runtime Quality must prove every scenario; the repair does not bypass or relax the production binder requirement. | +| CONTROL-GAP-BASELINE-PRESERVATION-03 | **Proposed repair on `.github#2281`** | Predecessor `dc47aa82faf4a838b96b63a84a459efea7e91c84` changed this baseline by +11/-1,897 and dropped 28 of 59 protected level-two sections. RED `9ba78fe89f2d2dd19477650c5a26d42df5e4bd24` requires representative protected security, runtime, compliance, APA, and credential-lifetime authorities. The selected repair restores protected `main` and keeps this four-row delta additive. | `.github` owns the central baseline. Exact-head hosted checks and independent review must confirm the restored tree before Ready/merge; future baseline updates must preserve or explicitly supersede protected authority rather than replace the file from a stale branch snapshot. | + ### 2026-09-13 current-head incident delta | Gap ID | 상태 | exact-head evidence | causal owner / next gate | diff --git a/scripts/ci/noema_review_document.py b/scripts/ci/noema_review_document.py index 17d3ca603c..d72eadc628 100644 --- a/scripts/ci/noema_review_document.py +++ b/scripts/ci/noema_review_document.py @@ -1,19 +1,25 @@ -"""Extract bounded review text from office documents without model access. - -DOCX is a ZIP/XML container whose text can be read with the Python standard -library. HWP and HWPX stay delegated to the reviewed hwp-mcp/rhwp reader; this -module only supplies a temporary local file and validates the subprocess -contract. +"""Extract bounded review text and figures from office documents. + +DOCX is a ZIP/XML container whose text and embedded media can be read with the +Python standard library. HWP and HWPX stay delegated to the reviewed +hwp-mcp/rhwp reader for text; when those archives also contain embedded media +this module fails closed unless the media can be attached as multimodal parts +(see ContextualWisdomLab/.github#2280). Research originals and participant +materials are never used as fixtures — synthetic archives only. """ from __future__ import annotations +import base64 import io import os +import posixpath import subprocess import tempfile import zipfile +from dataclasses import dataclass, field from pathlib import PurePosixPath +from typing import Any from defusedxml import ElementTree as ET from defusedxml.common import DefusedXmlException @@ -23,38 +29,132 @@ MAX_DOCUMENT_ZIP_ENTRIES = 2048 MAX_DOCUMENT_ZIP_UNCOMPRESSED_BYTES = 64 * 1024 * 1024 MAX_DOCUMENT_TEXT_BYTES = 256 * 1024 +MAX_DOCUMENT_IMAGES = 8 +MAX_DOCUMENT_IMAGE_BYTES = int(1.5 * 1024 * 1024) HWP_READER_ENV = "NOEMA_HWP_MCP_SOURCE" HWP_READER_TIMEOUT_SECONDS = 45 W_NS = "http://schemas.openxmlformats.org/wordprocessingml/2006/main" +R_NS = "http://schemas.openxmlformats.org/officeDocument/2006/relationships" +A_NS = "http://schemas.openxmlformats.org/drawingml/2006/main" M_NS = "http://schemas.openxmlformats.org/officeDocument/2006/math" +PKG_REL_NS = "http://schemas.openxmlformats.org/package/2006/relationships" W = f"{{{W_NS}}}" +R = f"{{{R_NS}}}" +A = f"{{{A_NS}}}" M = f"{{{M_NS}}}" +_IMAGE_SUFFIX_MIME = { + ".png": "image/png", + ".jpg": "image/jpeg", + ".jpeg": "image/jpeg", + ".gif": "image/gif", + ".bmp": "image/bmp", + ".tif": "image/tiff", + ".tiff": "image/tiff", + ".webp": "image/webp", +} + class DocumentReadError(RuntimeError): - """A document could not be converted to bounded review text.""" + """A document could not be converted to bounded review evidence.""" + + +@dataclass(frozen=True) +class DocumentImage: + """One embedded figure extracted for multimodal review attachment.""" + + path: str + media_path: str + mime_type: str + data: bytes + locator: str + + def to_multimodal_parts(self) -> list[dict[str, Any]]: + """Return OpenAI-style text locator + image_url content parts.""" + encoded = base64.standard_b64encode(self.data).decode("ascii") + return [ + { + "type": "text", + "text": ( + f"[document figure] path={self.path} media={self.media_path} " + f"locator={self.locator} mime={self.mime_type}" + ), + }, + { + "type": "image_url", + "image_url": {"url": f"data:{self.mime_type};base64,{encoded}"}, + }, + ] + + +@dataclass +class DocumentExtraction: + """Bounded text plus figures for one review document.""" + + path: str + text: str + images: list[DocumentImage] = field(default_factory=list) + media_declared: int = 0 + + def multimodal_parts(self) -> list[dict[str, Any]]: + """Flatten figure attachments for the reviewer request envelope.""" + parts: list[dict[str, Any]] = [] + for image in self.images: + parts.extend(image.to_multimodal_parts()) + return parts + + def ensure_figures_attached(self) -> None: + """Fail closed when the archive declared media that was not attached.""" + if self.media_declared and not self.images: + raise DocumentReadError( + f"{self.path}: document declares {self.media_declared} embedded " + "media entr(y/ies) but no image parts were attached; refusing " + "text-only success (ContextualWisdomLab/.github#2280)" + ) + if self.media_declared and len(self.images) < self.media_declared: + raise DocumentReadError( + f"{self.path}: attached {len(self.images)} image part(s) but " + f"archive declared {self.media_declared}; refusing partial " + "figure coverage as success" + ) def extract_review_document(path: str, raw: bytes) -> str: """Return text for one supported document path or fail closed. - The input bytes are obtained from the exact GitHub content ref by the - caller. HWP/HWPX bytes are never decoded as UTF-8 and never sent to an - external service; the configured reader runs as a local subprocess only. + When the document contains embedded figures, extraction fails closed unless + those figures are also represented as multimodal parts via + :func:`extract_review_document_bundle` — this text-only helper therefore + rejects figure-bearing archives so omission cannot look like success. """ + bundle = extract_review_document_bundle(path, raw) + if bundle.media_declared or bundle.images: + raise DocumentReadError( + f"{path}: embedded figures require multimodal attachment; use " + "extract_review_document_bundle / reviewer multimodal envelope " + "(ContextualWisdomLab/.github#2280)" + ) + return bundle.text + + +def extract_review_document_bundle(path: str, raw: bytes) -> DocumentExtraction: + """Return text and figure parts for one supported document path.""" if len(raw) > MAX_DOCUMENT_BYTES: raise DocumentReadError("document exceeds the bounded 8 MiB review input") suffix = PurePosixPath(path).suffix.lower() if suffix == ".docx": - return _extract_docx(raw) - if suffix in {".hwp", ".hwpx"}: - return _extract_hwp_with_reviewed_reader(path, raw) - raise DocumentReadError(f"unsupported review document format: {suffix or ''}") + extraction = _extract_docx_bundle(path, raw) + elif suffix in {".hwp", ".hwpx"}: + extraction = _extract_hwp_bundle(path, raw) + else: + raise DocumentReadError(f"unsupported review document format: {suffix or ''}") + extraction.ensure_figures_attached() + return extraction -def _extract_docx(raw: bytes) -> str: - """Extract paragraphs, tables, and Office Math text from one DOCX.""" +def _extract_docx_bundle(path: str, raw: bytes) -> DocumentExtraction: + """Extract paragraphs, tables, Office Math, and embedded DOCX media.""" try: with zipfile.ZipFile(io.BytesIO(raw)) as archive: infos = archive.infolist() @@ -73,21 +173,24 @@ def _extract_docx(raw: bytes) -> str: raise DocumentReadError( "DOCX archive has no word/document.xml" ) from exc + try: + root = ET.fromstring(document_xml) + except (ET.ParseError, DefusedXmlException) as exc: + raise DocumentReadError("DOCX document.xml is malformed") from exc + + body = root.find(f"{W}body") + if body is None: + raise DocumentReadError("DOCX document.xml has no document body") + media_names, locators = _docx_image_references(archive, body) + images = _docx_images_from_archive( + path, archive, media_names, locators=locators + ) except DocumentReadError: raise except (zipfile.BadZipFile, OSError, ValueError) as exc: raise DocumentReadError("DOCX archive is malformed") from exc - try: - root = ET.fromstring(document_xml) - except (ET.ParseError, DefusedXmlException) as exc: - raise DocumentReadError("DOCX document.xml is malformed") from exc - - body = root.find(f"{W}body") - if body is None: - raise DocumentReadError("DOCX document.xml has no document body") - - sections: list[str] = [] + sections: list[str] = [f"[document text] path={path}"] table_number = 0 for child in body: if child.tag == f"{W}p": @@ -101,9 +204,156 @@ def _extract_docx(raw: bytes) -> str: sections.append(table) text = "\n\n".join(sections).strip() - if not text: - raise DocumentReadError("DOCX contains no readable text") - return _bounded_text(text) + if not text or text == f"[document text] path={path}": + if not images: + raise DocumentReadError("DOCX contains no readable text") + text = f"[document text] path={path}\n[body empty; figures attached separately]" + return DocumentExtraction( + path=path, + text=_bounded_text(text), + images=images, + media_declared=len(media_names), + ) + + +def _docx_image_references( + archive: zipfile.ZipFile, + body: ET.Element, +) -> tuple[list[str], list[str]]: + """Resolve main-document image relationships in source order.""" + blips = list(body.iter(f"{A}blip")) + if not blips: + return [], [] + try: + relationships_xml = archive.read("word/_rels/document.xml.rels") + except KeyError as exc: + raise DocumentReadError( + "DOCX body image has an unresolved relationship" + ) from exc + try: + relationships_root = ET.fromstring(relationships_xml) + except (ET.ParseError, DefusedXmlException) as exc: + raise DocumentReadError("DOCX document relationships are malformed") from exc + + relationship_targets: dict[str, str] = {} + relationship_ids: set[str] = set() + for relationship in relationships_root.findall(f"{{{PKG_REL_NS}}}Relationship"): + relationship_id = relationship.attrib.get("Id") + target = relationship.attrib.get("Target") + relationship_type = relationship.attrib.get("Type", "") + if relationship_id and relationship_id in relationship_ids: + raise DocumentReadError( + f"DOCX has duplicate relationship ID {relationship_id}" + ) + if relationship_id: + relationship_ids.add(relationship_id) + if ( + not relationship_id + or not target + or not relationship_type.endswith("/image") + or relationship.attrib.get("TargetMode", "").casefold() == "external" + ): + continue + target_path = PurePosixPath(target) + if ( + target_path.is_absolute() + or ".." in target_path.parts + or "\\" in target + or "?" in target + or "#" in target + ): + raise DocumentReadError( + f"DOCX image relationship {relationship_id} targets " + "outside word/media" + ) + media_path = posixpath.normpath(posixpath.join("word", target)) + if not media_path.startswith("word/media/"): + raise DocumentReadError( + f"DOCX image relationship {relationship_id} targets " + "outside word/media" + ) + relationship_targets[relationship_id] = media_path + + media_names: list[str] = [] + locators: list[str] = [] + for index, blip in enumerate(blips, start=1): + relationship_id = blip.attrib.get(f"{R}embed") + media_path = relationship_targets.get(relationship_id or "") + if media_path is None: + raise DocumentReadError( + f"DOCX body image has unresolved relationship " + f"{relationship_id or ''}" + ) + media_names.append(media_path) + locators.append( + f"document-body-blip-{index}:{relationship_id}->{media_path}" + ) + return media_names, locators + + +def _docx_images_from_archive( + path: str, + archive: zipfile.ZipFile, + media_names: list[str], + *, + locators: list[str] | None = None, +) -> list[DocumentImage]: + """Load bounded DOCX media entries as multimodal figure parts.""" + return _images_from_archive( + "DOCX", path, archive, media_names, locators=locators + ) + + +def _images_from_archive( + format_name: str, + path: str, + archive: zipfile.ZipFile, + media_names: list[str], + *, + locators: list[str] | None = None, +) -> list[DocumentImage]: + """Load bounded image entries for a relationship-resolved ZIP document.""" + if len(media_names) > MAX_DOCUMENT_IMAGES: + raise DocumentReadError( + f"{format_name} declares {len(media_names)} media entries; " + f"limit is {MAX_DOCUMENT_IMAGES}" + ) + images: list[DocumentImage] = [] + if locators is not None and len(locators) != len(media_names): + raise DocumentReadError( + f"{format_name} image locator count does not match media" + ) + for index, media_path in enumerate(media_names, start=1): + suffix = PurePosixPath(media_path).suffix.lower() + mime = _IMAGE_SUFFIX_MIME.get(suffix) + if mime is None: + raise DocumentReadError( + f"{format_name} media {media_path} has unsupported image type " + f"{suffix or ''}" + ) + try: + data = archive.read(media_path) + except KeyError as exc: + raise DocumentReadError( + f"{format_name} media {media_path} is declared but unreadable" + ) from exc + if not data: + raise DocumentReadError(f"{format_name} media {media_path} is empty") + if len(data) > MAX_DOCUMENT_IMAGE_BYTES: + raise DocumentReadError( + f"{format_name} media {media_path} exceeds the bounded " + f"{MAX_DOCUMENT_IMAGE_BYTES} byte image size" + ) + images.append( + DocumentImage( + path=path, + media_path=media_path, + mime_type=mime, + data=data, + locator=(locators[index - 1] if locators else f"figure-{index}"), + ) + ) + return images def _paragraph_text(paragraph: ET.Element) -> str: @@ -142,6 +392,253 @@ def _table_markdown(table: ET.Element, table_number: int) -> str: return "\n".join(lines) +def _xml_local_name(tag: str) -> str: + """Return an XML element's local name without trusting its prefix.""" + return tag.rsplit("}", 1)[-1] + + +def _safe_hwpx_section_path(href: str) -> str: + """Resolve one manifest section href inside the HWPX Contents directory.""" + target = PurePosixPath(href) + if ( + not href + or target.is_absolute() + or ".." in target.parts + or "\\" in href + or "?" in href + or "#" in href + ): + raise DocumentReadError("HWPX section relationship targets outside Contents") + section_path = href if href.startswith("Contents/") else f"Contents/{href}" + return section_path + + +def _safe_hwpx_media_path( + relationship_id: str, + href: str, + media_type: str, + is_embedded: str, +) -> str: + """Validate one section image relationship against the HWPX manifest.""" + if is_embedded == "0": + raise DocumentReadError( + f"HWPX image {relationship_id} uses an external relationship" + ) + target = PurePosixPath(href) + if ( + not href + or target.is_absolute() + or ".." in target.parts + or "\\" in href + or "?" in href + or "#" in href + or not href.startswith("BinData/") + ): + raise DocumentReadError( + f"HWPX image relationship {relationship_id} targets outside BinData" + ) + suffix = target.suffix.lower() + if not media_type.casefold().startswith("image/") or suffix not in _IMAGE_SUFFIX_MIME: + raise DocumentReadError( + f"HWPX image relationship {relationship_id} has unsupported image media" + ) + return href + + +def _hwpx_picture_references( + section_root: ET.Element, + *, + section_number: int, + manifest_items: dict[str, tuple[str, str, str]], +) -> tuple[list[str], list[str]]: + """Resolve picture references in semantic XML order with stable locators.""" + media_names: list[str] = [] + locators: list[str] = [] + + def walk(element: ET.Element, path: str) -> None: + sibling_counts: dict[str, int] = {} + for child in element: + local_name = _xml_local_name(child.tag) + sibling_counts[local_name] = sibling_counts.get(local_name, 0) + 1 + child_path = f"{path}/{local_name}-{sibling_counts[local_name]}" + if local_name == "pic": + image_elements = [ + descendant + for descendant in child.iter() + if _xml_local_name(descendant.tag) == "img" + and "binaryItemIDRef" in descendant.attrib + ] + if len(image_elements) != 1: + raise DocumentReadError( + "HWPX picture has missing or ambiguous image relationship" + ) + relationship_id = image_elements[0].attrib["binaryItemIDRef"].strip() + item = manifest_items.get(relationship_id) + if not relationship_id or item is None: + raise DocumentReadError( + f"HWPX picture has unresolved relationship " + f"{relationship_id or ''}" + ) + href, media_type, is_embedded = item + media_path = _safe_hwpx_media_path( + relationship_id, href, media_type, is_embedded + ) + media_names.append(media_path) + locators.append( + f"section-{section_number}:{child_path}:" + f"{relationship_id}->{media_path}" + ) + walk(child, child_path) + + walk(section_root, f"section-{section_number}") + return media_names, locators + + +def _hwpx_media_references(raw: bytes) -> tuple[list[str], list[str]]: + """Resolve manifest-bound HWPX images in spine and section source order.""" + try: + with zipfile.ZipFile(io.BytesIO(raw)) as archive: + infos = archive.infolist() + if len(infos) > MAX_DOCUMENT_ZIP_ENTRIES: + raise DocumentReadError("HWPX archive has too many entries") + if ( + sum(info.file_size for info in infos) + > MAX_DOCUMENT_ZIP_UNCOMPRESSED_BYTES + ): + raise DocumentReadError( + "HWPX archive exceeds the bounded unpacked size" + ) + entry_names = [info.filename for info in infos if not info.is_dir()] + if len(entry_names) != len(set(entry_names)): + raise DocumentReadError("HWPX archive has duplicate entry names") + image_entries = { + name + for name in entry_names + if PurePosixPath(name).suffix.lower() in _IMAGE_SUFFIX_MIME + } + if not image_entries: + return [], [] + try: + content_xml = archive.read("Contents/content.hpf") + except KeyError as exc: + raise DocumentReadError( + "HWPX image archive has no Contents/content.hpf" + ) from exc + try: + content_root = ET.fromstring(content_xml) + except (ET.ParseError, DefusedXmlException) as exc: + raise DocumentReadError("HWPX content.hpf is malformed") from exc + + manifest_items: dict[str, tuple[str, str, str]] = {} + manifest_order: list[str] = [] + for element in content_root.iter(): + if _xml_local_name(element.tag) != "item": + continue + item_id = element.attrib.get("id", "").strip() + href = element.attrib.get("href", "").strip() + if not item_id or not href: + continue + if item_id in manifest_items: + raise DocumentReadError( + f"HWPX has duplicate manifest ID {item_id}" + ) + manifest_items[item_id] = ( + href, + element.attrib.get("media-type", "").strip(), + element.attrib.get("isEmbeded", "1").strip(), + ) + manifest_order.append(item_id) + + spine_ids = [ + element.attrib.get("idref", "").strip() + for element in content_root.iter() + if _xml_local_name(element.tag) == "itemref" + and element.attrib.get("idref", "").strip() + ] + section_ids = [ + item_id + for item_id in (spine_ids or manifest_order) + if item_id in manifest_items + and manifest_items[item_id][1] == "application/xml" + and "section" in manifest_items[item_id][0].casefold() + ] + if not section_ids: + raise DocumentReadError( + "HWPX image archive has no manifest-bound section relationship" + ) + + media_names: list[str] = [] + locators: list[str] = [] + for section_number, section_id in enumerate(section_ids, start=1): + section_path = _safe_hwpx_section_path( + manifest_items[section_id][0] + ) + try: + section_xml = archive.read(section_path) + except KeyError as exc: + raise DocumentReadError( + f"HWPX section relationship {section_id} is unreadable" + ) from exc + try: + section_root = ET.fromstring(section_xml) + except (ET.ParseError, DefusedXmlException) as exc: + raise DocumentReadError( + f"HWPX section relationship {section_id} is malformed" + ) from exc + section_media, section_locators = _hwpx_picture_references( + section_root, + section_number=section_number, + manifest_items=manifest_items, + ) + media_names.extend(section_media) + locators.extend(section_locators) + + if not media_names and image_entries: + raise DocumentReadError( + "HWPX archive media is not referenced by any section picture" + ) + return media_names, locators + except DocumentReadError: + raise + except (zipfile.BadZipFile, OSError, ValueError): + # Classic .hwp is not a ZIP; absence of ZIP media is not evidence of + # figures, so the text reader path remains authoritative. + return [], [] + + +def _hwpx_media_names(raw: bytes) -> list[str]: + """Return manifest-bound HWPX image paths in section source order.""" + return _hwpx_media_references(raw)[0] + + +def _extract_hwp_bundle(path: str, raw: bytes) -> DocumentExtraction: + """Extract HWP/HWPX text and fail closed on unattached archive media.""" + suffix = PurePosixPath(path).suffix.lower() + media_names, locators = ( + _hwpx_media_references(raw) if suffix == ".hwpx" else ([], []) + ) + text = _extract_hwp_with_reviewed_reader(path, raw) + images: list[DocumentImage] = [] + if media_names: + try: + with zipfile.ZipFile(io.BytesIO(raw)) as archive: + images = _images_from_archive( + "HWPX", path, archive, media_names, locators=locators + ) + except DocumentReadError: + raise + except (zipfile.BadZipFile, OSError, ValueError) as exc: + raise DocumentReadError( + f"{path}: HWPX declares image media but the archive is unreadable" + ) from exc + return DocumentExtraction( + path=path, + text=_bounded_text(f"[document text] path={path}\n{text}"), + images=images, + media_declared=len(media_names), + ) + + def _extract_hwp_with_reviewed_reader(path: str, raw: bytes) -> str: """Delegate HWP/HWPX parsing to the reviewed hwp-mcp/rhwp source tree.""" source = os.environ.get(HWP_READER_ENV, "").strip() @@ -190,7 +687,7 @@ def _extract_hwp_with_reviewed_reader(path: str, raw: bytes) -> str: ) from exc if not text: raise DocumentReadError("reviewed hwp-mcp/rhwp reader returned empty text") - return _bounded_text(text) + return text def _bounded_text(text: str) -> str: @@ -209,14 +706,24 @@ def _main() -> int: parser = argparse.ArgumentParser() parser.add_argument("path") + parser.add_argument( + "--allow-figures", + action="store_true", + help="Print text even when figures are present (does not emit image bytes).", + ) args = parser.parse_args() try: with open(args.path, "rb") as handle: - text = extract_review_document(args.path, handle.read()) + raw = handle.read() + if args.allow_figures: + bundle = extract_review_document_bundle(args.path, raw) + print(bundle.text) + print(f"[figures attached: {len(bundle.images)}]", file=os.sys.stderr) + else: + print(extract_review_document(args.path, raw)) except (OSError, DocumentReadError) as exc: print(str(exc), file=os.sys.stderr) return 1 - print(text) return 0 diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index c8709304fc..8641ca8b44 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -21,11 +21,20 @@ import urllib.parse import urllib.request from collections.abc import Sequence +from dataclasses import dataclass, field from pathlib import PurePosixPath from typing import Any from scripts.ci.opencode_review_normalize_output import changed_file_is_material -from scripts.ci.noema_review_document import DocumentReadError, extract_review_document +from scripts.ci.noema_review_document import ( + DocumentReadError, + extract_review_document, + extract_review_document_bundle, +) + +NOEMA_SKILL_HUMANIZE_KOREAN = "~/.claude/skills/humanize-korean/SKILL.md" +NOEMA_SKILL_SOURCE_CHECK = "~/.agents/skills/source-check/SKILL.md" +NOEMA_OFFICE_DOCUMENT_SUFFIXES = frozenset({".docx", ".hwp", ".hwpx"}) PRIMARY_REVIEW_AUTHORS = { @@ -72,6 +81,33 @@ DIFF_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@") SAFE_MODEL_IDENTIFIER_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._:/@+-]{0,199}$") + +@dataclass +class ReviewContext: + """Bounded non-diff review context plus optional multimodal document parts.""" + + text: str + multimodal_parts: list[dict[str, Any]] = field(default_factory=list) + + def __str__(self) -> str: + """Return the text envelope for backward-compatible string checks.""" + return self.text + + def __contains__(self, item: str) -> bool: + """Support ``in`` checks against the text envelope.""" + return item in self.text + + def __eq__(self, other: object) -> bool: + """Compare text-only callers against an empty or plain-text context.""" + if isinstance(other, ReviewContext): + return ( + self.text == other.text + and self.multimodal_parts == other.multimodal_parts + ) + if isinstance(other, str): + return self.text == other and not self.multimodal_parts + return NotImplemented + ORCHESTRATOR_LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1"}) ORCHESTRATOR_BASE_ENV = "CONTEXTUAL_ORCHESTRATOR_BASE_URL" # OpenAI Chat Completions structured-output envelope for the verdict shape @@ -841,8 +877,29 @@ def fetch_changed_files(repo: str, number: int) -> list[tuple[str, str]]: return files -def fetch_file_content_at_ref(repo: str, path: str, ref: str) -> str: - """Fetch one repository file at an exact Git ref through GitHub.""" +def document_proofreading_prompt_lines() -> list[str]: + """Return proofreading checklist lines reused from organization skills.""" + return [ + "Document proofreading checklist (reuse humanize-korean and source-check skills; " + "ContextualWisdomLab/.github#2280):", + f"- humanize-korean skill: {NOEMA_SKILL_HUMANIZE_KOREAN}", + f"- source-check skill: {NOEMA_SKILL_SOURCE_CHECK}", + "- For changed .docx/.hwp/.hwpx files, check Korean/English sentence rhythm, " + "terminology, and table/figure caption consistency across body text and any " + "attached document figures.", + "- Style and phrasing fixes must preserve content anchors; do not delete or " + "rewrite substantive claims, numbers, or citations without evidence.", + "- Citations and page references require source verification per source-check; " + "flag unsupported or invented citations instead of rewriting them arbitrarily.", + "- Findings must cite exact changed-side path, line, and side with rationale " + "tied to observed document text or figure evidence.", + "- Do not request arbitrary number, statistic, or citation edits without " + "independent source-verification evidence.", + ] + + +def _fetch_repository_file_bytes(repo: str, path: str, ref: str) -> bytes: + """Return decoded repository file bytes at one exact Git ref.""" encoded_path = urllib.parse.quote(path, safe="/") encoded_ref = urllib.parse.quote(ref, safe="") content = run( @@ -856,18 +913,55 @@ def fetch_file_content_at_ref(repo: str, path: str, ref: str) -> str: ) compact = "".join(content.split()) if not compact: - return "" + return b"" try: - raw = base64.b64decode(compact, validate=True) + return base64.b64decode(compact, validate=True) except (binascii.Error, ValueError) as exc: raise RuntimeError("GitHub content response contained malformed base64") from exc + + +def fetch_file_review_bundle( + repo: str, path: str, ref: str +) -> tuple[str, list[dict[str, Any]]]: + """Fetch bounded text and optional multimodal figure parts for one path.""" + raw = _fetch_repository_file_bytes(repo, path, ref) + if not raw: + return "", [] suffix = PurePosixPath(path).suffix.lower() - if suffix in {".docx", ".hwp", ".hwpx"}: + if suffix in NOEMA_OFFICE_DOCUMENT_SUFFIXES: try: - return extract_review_document(path, raw) + bundle = extract_review_document_bundle(path, raw) except DocumentReadError as exc: raise RuntimeError(f"document extraction failed: {exc}") from exc - return raw.decode("utf-8", errors="replace") + return bundle.text, bundle.multimodal_parts() + return raw.decode("utf-8", errors="replace"), [] + + +def fetch_file_content_at_ref(repo: str, path: str, ref: str) -> str: + """Fetch one repository file at an exact Git ref through GitHub.""" + text, multimodal_parts = fetch_file_review_bundle(repo, path, ref) + if multimodal_parts: + raise RuntimeError( + f"{path}: embedded figures require multimodal attachment; use " + "fetch_file_review_bundle / ReviewContext (ContextualWisdomLab/.github#2280)" + ) + return text + + +def _coerce_review_context(review_context: str | ReviewContext) -> ReviewContext: + """Normalize legacy string review context into a ReviewContext envelope.""" + if isinstance(review_context, ReviewContext): + return review_context + return ReviewContext(text=review_context) + + +def _user_message_content( + prompt_text: str, multimodal_parts: Sequence[dict[str, Any]] +) -> str | list[dict[str, Any]]: + """Build OpenAI-style user content with optional multimodal figure parts.""" + if not multimodal_parts: + return prompt_text + return [{"type": "text", "text": prompt_text}, *list(multimodal_parts)] def fetch_merge_base_sha(repo: str, base_sha: str, head_sha: str) -> str: @@ -895,7 +989,7 @@ def removed_file_context_section( path: str, merge_base_sha: str, merge_base_error: str = "", -) -> str: +) -> tuple[str, list[dict[str, Any]]]: """Build review context for a file deleted relative to the merge base. A deleted path does not exist at the PR head. Its relevant pre-deletion @@ -907,29 +1001,36 @@ def removed_file_context_section( if merge_base_error: return ( f"### {path}\n[File removed in this PR.] " - f"Merge-base lookup unavailable: {merge_base_error}" + f"Merge-base lookup unavailable: {merge_base_error}", + [], ) if not merge_base_sha: return ( f"### {path}\n[File removed in this PR — no head-side content applicable; " - "merge-base SHA unavailable for pre-deletion content.]" + "merge-base SHA unavailable for pre-deletion content.]", + [], ) try: - content = fetch_file_content_at_ref(repo, path, merge_base_sha) + content, multimodal_parts = fetch_file_review_bundle( + repo, path, merge_base_sha + ) except RuntimeError as exc: reason = scrub_sensitive_data(str(exc)) or "unknown error" return ( f"### {path}\n[File removed in this PR.] " - f"Unavailable from merge-base content API: {reason}" + f"Unavailable from merge-base content API: {reason}", + [], ) if not content: return ( f"### {path}\n[File removed in this PR — no UTF-8 text content " - "available from merge-base content API.]" + "available from merge-base content API.]", + [], ) return ( f"### {path}\n[File removed in this PR. Pre-deletion content at merge base " - f"`{merge_base_sha}`:]\n{truncate_text(content, MAX_FILE_CONTEXT_CHARS)}" + f"`{merge_base_sha}`:]\n{truncate_text(content, MAX_FILE_CONTEXT_CHARS)}", + list(multimodal_parts), ) @@ -939,13 +1040,13 @@ def changed_file_context( head_sha: str, base_sha: str = "", changed_files: Sequence[tuple[str, str]] | None = None, -) -> str: +) -> tuple[str, list[dict[str, Any]]]: """Build bounded changed-file context from one status-preserving snapshot.""" if not head_sha: - return "Changed file context unavailable: missing PR head SHA." + return "Changed file context unavailable: missing PR head SHA.", [] files = list(changed_files) if changed_files is not None else fetch_changed_files(repo, number) if not files: - return "Changed file context unavailable: PR reported no changed files." + return "Changed file context unavailable: PR reported no changed files.", [] merge_base_sha = "" merge_base_error = "" @@ -956,16 +1057,17 @@ def changed_file_context( merge_base_error = scrub_sensitive_data(str(exc)) or "unknown error" sections: list[str] = [] + multimodal_parts: list[dict[str, Any]] = [] for path, status in files[:MAX_CONTEXT_FILES]: if status == "removed": - sections.append( - removed_file_context_section( - repo, path, merge_base_sha, merge_base_error - ) + section, parts = removed_file_context_section( + repo, path, merge_base_sha, merge_base_error ) + sections.append(section) + multimodal_parts.extend(parts) continue try: - content = fetch_file_content_at_ref(repo, path, head_sha) + content, parts = fetch_file_review_bundle(repo, path, head_sha) except RuntimeError as exc: reason = scrub_sensitive_data(str(exc)) or "unknown error" sections.append(f"### {path}\nUnavailable from head content API: {reason}") @@ -974,9 +1076,10 @@ def changed_file_context( sections.append(f"### {path}\nNo UTF-8 text content available from head content API.") continue sections.append(f"### {path}\n{truncate_text(content, MAX_FILE_CONTEXT_CHARS)}") + multimodal_parts.extend(parts) if len(files) > MAX_CONTEXT_FILES: sections.append(f"[{len(files) - MAX_CONTEXT_FILES} changed files omitted from context budget]") - return "\n\n".join(sections) + return "\n\n".join(sections), multimodal_parts def review_thread_context(pr: dict[str, Any]) -> str: @@ -1006,22 +1109,25 @@ def build_review_context( number: int, pr: dict[str, Any], changed_files: Sequence[tuple[str, str]] | None = None, -) -> str: +) -> ReviewContext: """Build bounded non-diff context from review threads and changed files.""" sections: list[str] = [] threads = review_thread_context(pr) if threads: sections.append("## Prior review threads\n" + threads) - files = changed_file_context( + file_text, multimodal_parts = changed_file_context( repo, number, str(pr.get("headRefOid") or ""), str(pr.get("baseRefOid") or ""), changed_files, ) - if files: - sections.append("## Changed file context\n" + files) - return truncate_text("\n\n".join(sections), MAX_REVIEW_CONTEXT_CHARS) + if file_text: + sections.append("## Changed file context\n" + file_text) + return ReviewContext( + text=truncate_text("\n\n".join(sections), MAX_REVIEW_CONTEXT_CHARS), + multimodal_parts=multimodal_parts, + ) class NoRedirectHandler(urllib.request.HTTPRedirectHandler): @@ -1629,7 +1735,7 @@ def call_llm( diff: str, truncated: bool, expected_head: str, - review_context: str = "", + review_context: str | ReviewContext = "", changed_paths: Sequence[str] = (), ) -> dict[str, Any]: """Issue exactly one structured-output request through contextual-orchestrator. @@ -1657,29 +1763,32 @@ def call_llm( "path": "path", "line": 0, "side": "RIGHT" } allowed_locations_json = _bounded_allowed_locations_json(allowed_locations) + context = _coerce_review_context(review_context) + prompt_text = "\n".join( + [ + "You are Noema, an independent pull request reviewer for ContextualWisdomLab.", + "Review the PR diff plus the additional changed-file and review-thread context for correctness, security, maintainability, and behavioral regressions.", + "Return only JSON with the declared response_format schema.", + "Every formal verdict must cite exact changed-side lines. APPROVE requires falsifying concrete regression hypotheses; source or test changes require at least two distinct probes and other changes require at least one. REQUEST_CHANGES requires a confirmed probe at a finding location.", + "Use only path, line, and side tuples listed in the bounded allowed-locations JSON below. If it is truncated, omit a formal verdict for any location not listed instead of guessing.", + f"Allowed changed-side locations: {allowed_locations_json}", + f"Location shape example: {json.dumps(location_example, separators=(',', ':'))}", + "Use request_changes only for blocking, concrete issues. A generic no-issues statement is not review evidence.", + *document_proofreading_prompt_lines(), + f"Repository: {repo}", + f"PR: #{number}", + f"Title: {pr.get('title') or ''}", + f"Head SHA: {pr.get('headRefOid') or ''}", + f"Diff truncated: {truncated}", + "Additional context:", + context.text or "No additional context was available.", + "Diff:", + diff, + ] + ) prompt = { "role": "user", - "content": "\n".join( - [ - "You are Noema, an independent pull request reviewer for ContextualWisdomLab.", - "Review the PR diff plus the additional changed-file and review-thread context for correctness, security, maintainability, and behavioral regressions.", - "Return only JSON with the declared response_format schema.", - "Every formal verdict must cite exact changed-side lines. APPROVE requires falsifying concrete regression hypotheses; source or test changes require at least two distinct probes and other changes require at least one. REQUEST_CHANGES requires a confirmed probe at a finding location.", - "Use only path, line, and side tuples listed in the bounded allowed-locations JSON below. If it is truncated, omit a formal verdict for any location not listed instead of guessing.", - f"Allowed changed-side locations: {allowed_locations_json}", - f"Location shape example: {json.dumps(location_example, separators=(',', ':'))}", - "Use request_changes only for blocking, concrete issues. A generic no-issues statement is not review evidence.", - f"Repository: {repo}", - f"PR: #{number}", - f"Title: {pr.get('title') or ''}", - f"Head SHA: {pr.get('headRefOid') or ''}", - f"Diff truncated: {truncated}", - "Additional context:", - review_context or "No additional context was available.", - "Diff:", - diff, - ] - ), + "content": _user_message_content(prompt_text, context.multimodal_parts), } payload = { "model": model, diff --git a/scripts/ci/test_strix_quick_gate.sh b/scripts/ci/test_strix_quick_gate.sh index 150b9102b3..acd6dbd3e5 100755 --- a/scripts/ci/test_strix_quick_gate.sh +++ b/scripts/ci/test_strix_quick_gate.sh @@ -3296,6 +3296,7 @@ run_gate_case() { local gate_under_test="$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$GATE_SCRIPT" "$gate_under_test" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$gate_under_test" local fake_strix="$bin_dir/strix" local path_hijack_log="$tmp_dir/path-hijack.log" @@ -7026,6 +7027,7 @@ run_pull_request_target_head_scope_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -7174,6 +7176,7 @@ run_pull_request_target_plaintext_runner_token_fails_closed_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -7296,6 +7299,7 @@ run_pull_request_target_bounded_head_context_scope_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -7401,6 +7405,7 @@ run_pull_request_target_changed_context_scope_uses_pr_head_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -7580,6 +7585,7 @@ run_pull_request_target_changed_backend_context_scope_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -7839,6 +7845,7 @@ run_pull_request_target_frontend_email_context_scope_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -8029,6 +8036,7 @@ run_pull_request_target_shallow_head_merge_base_fallback_case() { cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -8144,6 +8152,7 @@ run_pull_request_target_aborts_on_pr_head_blob_failure_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local real_git @@ -8268,6 +8277,7 @@ run_pull_request_target_rejects_invalid_sha_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -8361,6 +8371,7 @@ run_pull_request_target_irregular_head_entry_fails_closed_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -8444,6 +8455,7 @@ run_pull_request_target_gitlink_is_explicitly_skipped_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -8526,6 +8538,7 @@ run_full_head_scope_skips_gitlink_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -8640,6 +8653,7 @@ run_pull_request_target_rejects_unsafe_changed_path_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" @@ -8732,6 +8746,7 @@ run_timeout_cleanup_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" local child_pid_file="$tmp_dir/child.pid" @@ -8814,6 +8829,7 @@ run_vertex_model_ignores_untrusted_llm_api_base_file_case() { mkdir -p "$repo_root_dir/scripts/ci" "$allowed_input_dir" "$outside_dir" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cat >"$fake_strix" <<'EOF' @@ -8866,6 +8882,7 @@ run_total_timeout_case() { mkdir -p "$bin_dir" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" local output_log="$tmp_dir/output.log" @@ -9193,6 +9210,7 @@ run_llm_api_base_file_outside_input_root_fails_closed_case() { mkdir -p "$repo_root_dir/scripts/ci" "$allowed_input_dir" "$outside_dir" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cat >"$fake_strix" <<'EOF' @@ -9248,6 +9266,7 @@ run_pr_scoped_llm_api_base_file_config_failure_exits_2_case() { mkdir -p "$repo_root_dir/scripts/ci" "$repo_root_dir/src" "$allowed_input_dir" "$outside_dir" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" printf '%s\n' 'print("one")' >"$repo_root_dir/src/one.py" printf '%s\n' 'print("two")' >"$repo_root_dir/src/two.py" @@ -9309,6 +9328,7 @@ run_required_input_file_outside_input_root_fails_closed_case() { mkdir -p "$repo_root_dir/scripts/ci" "$allowed_input_dir" "$outside_dir" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cat >"$fake_strix" <<'EOF' @@ -9379,6 +9399,7 @@ run_input_file_root_override_takes_precedence_over_runner_temp_case() { mkdir -p "$repo_root_dir/scripts/ci" "$explicit_input_root" "$inherited_runner_temp" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cat >"$fake_strix" <<'EOF' @@ -9433,6 +9454,7 @@ run_stale_report_case() { mkdir -p "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" mkdir -p "$stale_report_dir" @@ -9488,6 +9510,7 @@ run_symlink_report_case() { mkdir -p "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" mkdir -p "$external_report_dir" "$repo_root_dir/strix_runs" @@ -9544,6 +9567,7 @@ run_unsafe_target_path_case() { mkdir -p "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cat >"$fake_strix" <<'EOF' @@ -9592,6 +9616,7 @@ run_absolute_outside_target_path_case() { mkdir -p "$bin_dir" "$repo_root_dir/src" "$repo_root_dir/scripts/ci" cp "$GATE_SCRIPT" "$repo_root_dir/scripts/ci/strix_quick_gate.sh" cp "$REPO_ROOT/scripts/ci/strix_model_utils.sh" "$repo_root_dir/scripts/ci/strix_model_utils.sh" + cp "$REPO_ROOT/scripts/ci/strix_evidence_binding.py" "$repo_root_dir/scripts/ci/strix_evidence_binding.py" chmod +x "$repo_root_dir/scripts/ci/strix_quick_gate.sh" local fake_strix="$bin_dir/strix" local call_log="$tmp_dir/calls.log" diff --git a/tests/test_noema_document_review_context.py b/tests/test_noema_document_review_context.py index e6ec2e6d70..842293a041 100644 --- a/tests/test_noema_document_review_context.py +++ b/tests/test_noema_document_review_context.py @@ -14,23 +14,57 @@ from scripts.ci import noema_review_document as document from scripts.ci import noema_review_gate as noema +_MINIMAL_PNG = base64.b64decode( + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==" +) + + +def _prompt_text(content: object) -> str: + """Return the text envelope from a string or multimodal user message.""" + if isinstance(content, str): + return content + if isinstance(content, list): + return "\n".join( + part["text"] for part in content if isinstance(part, dict) and part.get("type") == "text" + ) + raise AssertionError(f"unexpected message content shape: {type(content)!r}") + -def _docx_bytes(*, malformed: bool = False) -> bytes: +def _docx_bytes(*, malformed: bool = False, with_image: bool = False) -> bytes: """Build a synthetic DOCX containing body, table, and Office Math text.""" if malformed: return b"not a zip archive" xml = """ + xmlns:m="http://schemas.openxmlformats.org/officeDocument/2006/math" + xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main" + xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships"> DOCX-REVIEW-MARKERx+y table-cell-a table-cell-b + {drawing} -""" +""".format( + drawing=( + '' + "" + if with_image + else "" + ) + ) output = io.BytesIO() with zipfile.ZipFile(output, "w", zipfile.ZIP_DEFLATED) as archive: archive.writestr("word/document.xml", xml) + if with_image: + archive.writestr( + "word/_rels/document.xml.rels", + '' + '', + ) + archive.writestr("word/media/figure1.png", _MINIMAL_PNG) return output.getvalue() @@ -90,10 +124,13 @@ def test_hosted_reader_bundle_is_pinned_and_local(): assert "Install exact Noema document dependencies" in quality_workflow for path in ( "scripts/ci/noema_review_document.py", + "scripts/ci/noema_review_gate.py", "scripts/ci/noema_hwp_mcp_reader.mjs", "scripts/ci/noema-document-reader/package.json", "scripts/ci/noema-document-reader/package-lock.json", "tests/test_noema_document_review_context.py", + "tests/test_noema_review_document_multimodal.py", + "docs/doctoring/noema-document-multimodal-proofreading.md", ): assert path in quality_workflow assert "tests/test_noema_document_review_context.py" in quality_workflow @@ -151,9 +188,11 @@ def open(self, request): context, ("docs/review.docx",), ) - prompt = captured["messages"][1]["content"] + prompt = _prompt_text(captured["messages"][1]["content"]) assert "DOCX-REVIEW-MARKER" in prompt assert "table-cell-a" in prompt + assert noema.NOEMA_SKILL_HUMANIZE_KOREAN in prompt + assert noema.NOEMA_SKILL_SOURCE_CHECK in prompt def test_malformed_docx_is_explicit_in_review_context(monkeypatch): @@ -161,13 +200,14 @@ def test_malformed_docx_is_explicit_in_review_context(monkeypatch): encoded = base64.b64encode(_docx_bytes(malformed=True)).decode("ascii") monkeypatch.setattr(noema, "run", lambda _args, stdin=None: encoded) - context = noema.changed_file_context( + context, parts = noema.changed_file_context( "owner/repo", 7, "head", changed_files=[("docs/broken.docx", "modified")] ) assert "### docs/broken.docx" in context assert "document extraction failed: DOCX archive is malformed" in context assert "not a zip archive" not in context + assert parts == [] def test_forbidden_docx_entities_are_explicitly_rejected(): @@ -183,9 +223,8 @@ def test_hwp_reader_contract_is_local_and_fail_closed(monkeypatch): ["node"], 0, stdout=b"HWP-REVIEW-MARKER\n", stderr=b"" ) monkeypatch.setattr(document.subprocess, "run", lambda *args, **kwargs: completed) - assert ( - document.extract_review_document("docs/review.hwpx", b"binary") - == "HWP-REVIEW-MARKER" + assert document.extract_review_document("docs/review.hwpx", b"binary").endswith( + "HWP-REVIEW-MARKER" ) failed = document.subprocess.CompletedProcess( @@ -271,7 +310,134 @@ def open(self, request): context, (f"docs/{fixture_name}",), ) - prompt = captured["messages"][1]["content"] + prompt = _prompt_text(captured["messages"][1]["content"]) assert expected_text in prompt if fixture_name == "simple.hwp": assert "| 이름 | 회사 |" in prompt + + +def test_docx_figures_reach_multimodal_llm_request(monkeypatch): + """Synthetic DOCX figures must appear as image_url data-URLs in the model request.""" + raw = _docx_bytes(with_image=True) + encoded = base64.b64encode(raw).decode("ascii") + monkeypatch.setattr(noema, "run", lambda _args, stdin=None: encoded) + + context = noema.build_review_context( + "owner/repo", 7, _pr(), [("docs/review.docx", "modified")] + ) + assert context.multimodal_parts + assert any(part.get("type") == "image_url" for part in context.multimodal_parts) + + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "test-key") + monkeypatch.setattr(noema, "validate_substantive_verdict", lambda *_args: None) + captured: dict[str, object] = {} + + class Response: + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def read(self): + verdict = {"decision": "comment", "summary": "checked", "findings": []} + return json.dumps( + {"choices": [{"message": {"content": json.dumps(verdict)}}]} + ).encode() + + class Opener: + def open(self, request): + captured.update(json.loads(request.data.decode())) + return Response() + + monkeypatch.setattr(noema.urllib.request, "build_opener", lambda *_args: Opener()) + noema.call_llm( + "owner/repo", + 7, + _pr(), + "diff --git a/docs/review.docx b/docs/review.docx\n+binary\n", + False, + "head", + context, + ("docs/review.docx",), + ) + user_content = captured["messages"][1]["content"] + assert isinstance(user_content, list) + image_parts = [part for part in user_content if part.get("type") == "image_url"] + assert len(image_parts) == 1 + url = image_parts[0]["image_url"]["url"] + assert url.startswith("data:image/png;base64,") + + +def test_docx_with_figures_rejects_text_only_extraction(): + """Text-only extraction must fail closed when figures are present.""" + raw = _docx_bytes(with_image=True) + with pytest.raises(document.DocumentReadError, match="embedded figures require multimodal"): + document.extract_review_document("docs/review.docx", raw) + + +def test_docx_unsupported_media_type_fails_closed(): + """Unsupported embedded media types must not look like text-only success.""" + output = io.BytesIO() + with zipfile.ZipFile(output, "w", zipfile.ZIP_DEFLATED) as archive: + archive.writestr( + "word/document.xml", + """ + text + """, + ) + archive.writestr( + "word/_rels/document.xml.rels", + '' + '', + ) + archive.writestr("word/media/figure1.svg", b"") + with pytest.raises(document.DocumentReadError, match="unsupported image type"): + document.extract_review_document_bundle("docs/review.docx", output.getvalue()) + + +def test_docx_bundle_attaches_all_declared_media(): + """Every declared media entry must become a multimodal part.""" + bundle = document.extract_review_document_bundle( + "docs/review.docx", _docx_bytes(with_image=True) + ) + assert bundle.media_declared == 1 + assert len(bundle.images) == 1 + assert bundle.multimodal_parts()[1]["type"] == "image_url" + + +def test_review_context_equality_and_fetch_file_content_fail_closed(monkeypatch): + """ReviewContext compares like legacy strings; text-only fetch rejects figures.""" + empty = noema.ReviewContext(text="") + assert str(empty) == "" + assert empty == "" + assert empty.__eq__(7) is NotImplemented + assert empty != noema.ReviewContext(text="", multimodal_parts=[{"type": "text", "text": "x"}]) + + monkeypatch.setattr( + noema, + "fetch_file_review_bundle", + lambda repo, path, ref: ("text", [{"type": "image_url", "image_url": {"url": "data:"}}]), + ) + with pytest.raises(RuntimeError, match="embedded figures require multimodal"): + noema.fetch_file_content_at_ref("owner/repo", "docs/x.docx", "head") + + monkeypatch.setattr( + noema, + "fetch_file_review_bundle", + lambda repo, path, ref: ("plain-text", []), + ) + assert noema.fetch_file_content_at_ref("owner/repo", "README.md", "head") == "plain-text" + + +def test_fetch_repository_file_bytes_rejects_malformed_base64(monkeypatch): + """Malformed GitHub content base64 fails closed before decoding.""" + monkeypatch.setattr(noema, "run", lambda args, stdin=None: "%%%not-base64%%%") + with pytest.raises(RuntimeError, match="malformed base64"): + noema.fetch_file_review_bundle("owner/repo", "docs/x.docx", "head") diff --git a/tests/test_noema_removed_file_context.py b/tests/test_noema_removed_file_context.py index 500d406f73..7156a11b04 100644 --- a/tests/test_noema_removed_file_context.py +++ b/tests/test_noema_removed_file_context.py @@ -51,7 +51,7 @@ def fake_run(args, stdin=None): monkeypatch.setattr(noema, "run", fake_run) - context = noema.changed_file_context("owner/repo", 1486, head_sha, base_sha) + context, _parts = noema.changed_file_context("owner/repo", 1486, head_sha, base_sha) assert f"Pre-deletion content at merge base `{merge_base_sha}`" in context assert "def doomed" in context @@ -108,18 +108,24 @@ def test_fetch_merge_base_sha_rejects_malformed_compare_response(monkeypatch): def test_removed_file_context_section_without_merge_base_or_error(): """No merge-base SHA and no recorded error must still be explicit, not silent.""" - context = noema.removed_file_context_section("owner/repo", "gone.py", "", "") + context, parts = noema.removed_file_context_section("owner/repo", "gone.py", "", "") assert "merge-base SHA unavailable for pre-deletion content" in context + assert parts == [] def test_removed_file_context_section_empty_merge_base_content(monkeypatch): """An empty (non-UTF-8-decodable) merge-base blob must be reported, not silently dropped.""" - monkeypatch.setattr(noema, "fetch_file_content_at_ref", lambda repo, path, ref: "") + monkeypatch.setattr( + noema, "fetch_file_review_bundle", lambda repo, path, ref: ("", []) + ) - context = noema.removed_file_context_section("owner/repo", "gone.py", "c" * 40, "") + context, parts = noema.removed_file_context_section( + "owner/repo", "gone.py", "c" * 40, "" + ) assert "no UTF-8 text content available from merge-base content API" in context + assert parts == [] def test_removed_file_context_fails_closed_without_base_sha(monkeypatch): @@ -131,11 +137,11 @@ def test_removed_file_context_fails_closed_without_base_sha(monkeypatch): ) monkeypatch.setattr( noema, - "fetch_file_content_at_ref", + "fetch_file_review_bundle", lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("unexpected fetch")), ) - context = noema.changed_file_context("owner/repo", 7, "a" * 40, "") + context, _parts = noema.changed_file_context("owner/repo", 7, "a" * 40, "") assert "PR base SHA was unavailable or malformed" in context assert "Merge-base lookup unavailable" in context @@ -159,9 +165,9 @@ def test_removed_file_merge_base_content_failure_is_distinct_from_head_failure(m def fail_fetch(repo, path, ref): raise RuntimeError("HTTP 502: token ***") - monkeypatch.setattr(noema, "fetch_file_content_at_ref", fail_fetch) + monkeypatch.setattr(noema, "fetch_file_review_bundle", fail_fetch) - context = noema.changed_file_context("owner/repo", 7, head_sha, base_sha) + context, _parts = noema.changed_file_context("owner/repo", 7, head_sha, base_sha) assert "Unavailable from merge-base content API" in context assert "Unavailable from head content API" not in context @@ -174,7 +180,7 @@ def test_build_review_context_passes_live_base_ref(monkeypatch): def fake_context(repo, number, head_sha, base_sha="", changed_files=None): observed.append((repo, number, head_sha, base_sha, changed_files)) - return "files" + return "files", [] monkeypatch.setattr(noema, "changed_file_context", fake_context) @@ -185,4 +191,4 @@ def fake_context(repo, number, head_sha, base_sha="", changed_files=None): ) assert observed == [("owner/repo", 7, "head-sha", "base-sha", None)] - assert "## Changed file context\nfiles" in result + assert "## Changed file context\nfiles" in result.text diff --git a/tests/test_noema_review_document_multimodal.py b/tests/test_noema_review_document_multimodal.py new file mode 100644 index 0000000000..5f42372d3a --- /dev/null +++ b/tests/test_noema_review_document_multimodal.py @@ -0,0 +1,930 @@ +"""Unit tests for Noema document multimodal extraction edge cases.""" + +from __future__ import annotations + +import io +import os +import subprocess +import warnings +import zipfile +from pathlib import Path + +import pytest + +from scripts.ci import noema_review_document as document + + +def _minimal_docx_xml(body: str) -> str: + return f""" + + {body} +""" + + +def _docx_blip(relationship_id: str) -> str: + """Return one minimal document-order image reference.""" + return f'' + + +def _write_docx( + *, + body: str = "hello", + media: dict[str, bytes] | None = None, + include_document_xml: bool = True, + extra_entries: int = 0, + relationships: dict[str, str] | None = None, + include_relationships: bool = True, + relationships_xml: str | None = None, +) -> bytes: + media = media or {} + if relationships is None: + relationships = { + f"rId{index}": name.removeprefix("word/") + for index, name in enumerate(media, start=1) + } + if media and "r:embed=" not in body: + body += "".join(_docx_blip(relationship_id) for relationship_id in relationships) + output = io.BytesIO() + with zipfile.ZipFile(output, "w", zipfile.ZIP_DEFLATED) as archive: + if include_document_xml: + archive.writestr("word/document.xml", _minimal_docx_xml(body)) + if relationships_xml is None and include_relationships and relationships: + rows = "".join( + ''.format( + relationship_id, target + ) + for relationship_id, target in relationships.items() + ) + relationships_xml = ( + '' + f"{rows}" + ) + if relationships_xml is not None: + archive.writestr("word/_rels/document.xml.rels", relationships_xml) + for name, data in media.items(): + archive.writestr(name, data) + for index in range(extra_entries): + archive.writestr(f"padding/{index}.txt", b"x") + return output.getvalue() + + +def _write_hwpx( + *, + sections: dict[str, tuple[str, str]], + spine: list[str], + media: dict[str, tuple[str, bytes]], + extra_media: dict[str, bytes] | None = None, + manifest_rows: str | None = None, +) -> bytes: + """Build a synthetic HWPX package with manifest-bound image references.""" + if manifest_rows is None: + manifest_rows = "".join( + ''.format( + item_id, href + ) + for item_id, (href, _data) in media.items() + ) + manifest_rows += "".join( + ''.format( + section_id, section_path + ) + for section_id, (section_path, _xml) in sections.items() + ) + content_hpf = ( + '' + f"{manifest_rows}" + "" + + "".join(f'' for section_id in spine) + + "" + ) + output = io.BytesIO() + with zipfile.ZipFile(output, "w", zipfile.ZIP_DEFLATED) as archive: + archive.writestr("Contents/content.hpf", content_hpf) + for section_path, xml in sections.values(): + archive.writestr(section_path, xml) + for href, data in media.values(): + archive.writestr(href, data) + for href, data in (extra_media or {}).items(): + archive.writestr(href, data) + return output.getvalue() + + +def _hwpx_section(*binary_refs: str) -> str: + """Return section XML whose nested pictures preserve the supplied order.""" + pictures = "".join( + '' + "".format(binary_ref) + for binary_ref in binary_refs + ) + return ( + '' + f"{pictures}" + ) + + +def _hwpx_table_section(first_ref: str, table_ref: str) -> str: + """Return a section with figures in a text run and a nested table cell.""" + return ( + '' + f'' + "" + "" + f'' + "" + "" + ) + + +def _write_zip(entries: list[tuple[str, str | bytes]]) -> bytes: + """Build a synthetic ZIP while preserving entry order and duplicates.""" + output = io.BytesIO() + with zipfile.ZipFile(output, "w", zipfile.ZIP_DEFLATED) as archive: + for name, data in entries: + with warnings.catch_warnings(): + warnings.filterwarnings( + "ignore", message="Duplicate name:.*", category=UserWarning + ) + archive.writestr(name, data) + return output.getvalue() +def test_document_extraction_rejects_missing_attached_figures(): + """Declared media without attached images must fail closed.""" + extraction = document.DocumentExtraction( + path="docs/x.docx", text="t", images=[], media_declared=1 + ) + with pytest.raises(document.DocumentReadError, match="no image parts were attached"): + extraction.ensure_figures_attached() + + +def test_document_extraction_rejects_partial_figure_coverage(): + """Partial figure attachment must fail closed.""" + image = document.DocumentImage( + path="docs/x.docx", + media_path="word/media/a.png", + mime_type="image/png", + data=b"png", + locator="figure-1", + ) + extraction = document.DocumentExtraction( + path="docs/x.docx", text="t", images=[image], media_declared=2 + ) + with pytest.raises(document.DocumentReadError, match="partial"): + extraction.ensure_figures_attached() + + +def test_document_image_multimodal_parts_shape(): + """Each figure emits a locator text part and a data-URL image part.""" + image = document.DocumentImage( + path="docs/x.docx", + media_path="word/media/a.png", + mime_type="image/png", + data=b"png", + locator="figure-1", + ) + parts = image.to_multimodal_parts() + assert parts[0]["type"] == "text" + assert parts[1]["type"] == "image_url" + assert parts[1]["image_url"]["url"].startswith("data:image/png;base64,") + + +def test_extract_review_document_rejects_oversized_input(): + """Oversized archives are rejected before parsing.""" + raw = b"x" * (document.MAX_DOCUMENT_BYTES + 1) + with pytest.raises(document.DocumentReadError, match="8 MiB"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_extract_review_document_rejects_unsupported_suffix(): + """Unknown suffixes fail closed.""" + with pytest.raises(document.DocumentReadError, match="unsupported review document format"): + document.extract_review_document_bundle("docs/x.pdf", b"data") + + +def test_docx_rejects_too_many_zip_entries(): + """DOCX entry-count bounds are enforced.""" + raw = _write_docx(extra_entries=document.MAX_DOCUMENT_ZIP_ENTRIES) + with pytest.raises(document.DocumentReadError, match="too many entries"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_missing_document_xml(): + """DOCX without word/document.xml fails closed.""" + raw = _write_docx(include_document_xml=False) + with pytest.raises(document.DocumentReadError, match="no word/document.xml"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_missing_body(): + """DOCX without a body element fails closed.""" + xml = ( + '' + '' + "orphan" + ) + output = io.BytesIO() + with zipfile.ZipFile(output, "w") as archive: + archive.writestr("word/document.xml", xml) + with pytest.raises(document.DocumentReadError, match="no document body"): + document.extract_review_document_bundle("docs/x.docx", output.getvalue()) + + +def test_docx_image_only_body_is_allowed(): + """Figure-only DOCX archives still produce bounded text.""" + png = b"\x89PNG\r\n\x1a\n" + raw = _write_docx(body="", media={"word/media/a.png": png}) + bundle = document.extract_review_document_bundle("docs/x.docx", raw) + assert "figures attached separately" in bundle.text + assert len(bundle.images) == 1 + + +def test_docx_uses_relationship_order_and_ignores_orphan_media(): + """Only body-referenced figures are attached, in document source order.""" + png = b"\x89PNG\r\n\x1a\n" + raw = _write_docx( + body=_docx_blip("rIdSecond") + _docx_blip("rIdFirst"), + media={ + "word/media/a.png": png, + "word/media/z.png": png, + "word/media/orphan.png": png, + }, + relationships={ + "rIdFirst": "media/a.png", + "rIdSecond": "media/z.png", + }, + ) + + bundle = document.extract_review_document_bundle("docs/x.docx", raw) + + assert [image.media_path for image in bundle.images] == [ + "word/media/z.png", + "word/media/a.png", + ] + assert [image.locator for image in bundle.images] == [ + "document-body-blip-1:rIdSecond->word/media/z.png", + "document-body-blip-2:rIdFirst->word/media/a.png", + ] + assert bundle.media_declared == 2 + + +def test_docx_rejects_unresolved_body_image_relationship(): + """A body figure with no internal relationship must fail closed.""" + raw = _write_docx( + body=_docx_blip("rIdMissing"), + media={"word/media/a.png": b"png"}, + include_relationships=False, + ) + + with pytest.raises(document.DocumentReadError, match="unresolved relationship"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_image_relationship_outside_media_directory(): + """A relationship cannot escape the bounded DOCX media directory.""" + raw = _write_docx( + body=_docx_blip("rIdEscape"), + media={"outside.png": b"png"}, + relationships={"rIdEscape": "../outside.png"}, + ) + + with pytest.raises(document.DocumentReadError, match="outside word/media"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_relationship_traversal_that_reenters_media_directory(): + """Normalizing back into word/media must not erase a traversal attempt.""" + raw = _write_docx( + body=_docx_blip("rIdEscape"), + media={"word/media/a.png": b"png"}, + relationships={"rIdEscape": "media/../../word/media/a.png"}, + ) + + with pytest.raises(document.DocumentReadError, match="outside word/media"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_duplicate_relationship_ids(): + """Duplicate relationship IDs are ambiguous and must fail closed.""" + relationships_xml = ( + '' + '' + '' + "" + ) + raw = _write_docx( + body=_docx_blip("rId1"), + media={"word/media/a.png": b"a", "word/media/b.png": b"b"}, + relationships_xml=relationships_xml, + ) + + with pytest.raises(document.DocumentReadError, match="duplicate relationship ID"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_external_target_mode_is_case_insensitive(): + """Lowercase external image relationships must not authorize archive media.""" + relationships_xml = ( + '' + '' + "" + ) + raw = _write_docx( + body=_docx_blip("rId1"), + media={"word/media/a.png": b"a"}, + relationships_xml=relationships_xml, + ) + + with pytest.raises(document.DocumentReadError, match="unresolved relationship"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_malformed_relationship_xml(): + """Malformed relationship XML fails closed before image resolution.""" + raw = _write_docx( + body=_docx_blip("rId1"), + relationships_xml="' + '' + "" + ) + raw = _write_docx( + body=_docx_blip("rId1"), + media={"word/media/a.png": b"a"}, + relationships_xml=relationships_xml, + ) + + with pytest.raises(document.DocumentReadError, match="unresolved relationship"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_sibling_media_directory_prefix(): + """A word/media2 target must not satisfy the word/media boundary.""" + raw = _write_docx( + body=_docx_blip("rId1"), + media={"word/media2/a.png": b"a"}, + relationships={"rId1": "media2/a.png"}, + ) + + with pytest.raises(document.DocumentReadError, match="outside word/media"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_locator_media_cardinality_mismatch(): + """Internal locator and media cardinality must stay one-to-one.""" + output = io.BytesIO() + with zipfile.ZipFile(output, "w") as archive: + archive.writestr("word/media/a.png", b"a") + with zipfile.ZipFile(io.BytesIO(output.getvalue())) as archive: + with pytest.raises(document.DocumentReadError, match="locator count"): + document._docx_images_from_archive( + "docs/x.docx", + archive, + ["word/media/a.png"], + locators=[], + ) + + +def test_docx_rejects_too_many_media_entries(): + """Media count limits are enforced.""" + media = {f"word/media/{index}.png": b"x" for index in range(document.MAX_DOCUMENT_IMAGES + 1)} + raw = _write_docx(media=media) + with pytest.raises(document.DocumentReadError, match="limit is"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_empty_media_bytes(): + """Empty media entries fail closed.""" + raw = _write_docx(media={"word/media/a.png": b""}) + with pytest.raises(document.DocumentReadError, match="is empty"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_oversized_media_bytes(): + """Oversized images fail closed.""" + raw = _write_docx( + media={"word/media/a.png": b"x" * (document.MAX_DOCUMENT_IMAGE_BYTES + 1)} + ) + with pytest.raises(document.DocumentReadError, match="bounded"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_paragraph_text_preserves_tabs_and_breaks(): + """Tabs and line breaks inside paragraphs are preserved.""" + xml = _minimal_docx_xml( + "ab" + ) + output = io.BytesIO() + with zipfile.ZipFile(output, "w") as archive: + archive.writestr("word/document.xml", xml) + bundle = document.extract_review_document_bundle("docs/x.docx", output.getvalue()) + assert "a\t\nb" in bundle.text + + +def test_docx_skips_empty_paragraphs_and_tables(): + """Empty paragraphs and tables are omitted from the text envelope.""" + body = "kept" + raw = _write_docx(body=body) + bundle = document.extract_review_document_bundle("docs/x.docx", raw) + assert "kept" in bundle.text + assert "### Table" not in bundle.text + + +def test_docx_ignores_non_paragraph_body_children(): + """Section properties and other body children are skipped safely.""" + body = ( + "" + "kept" + ) + raw = _write_docx(body=body) + bundle = document.extract_review_document_bundle("docs/x.docx", raw) + assert "kept" in bundle.text + + +def test_docx_table_only_empty_rows_fail_without_figures(): + """A table-only DOCX with no renderable rows still fails without figures.""" + raw = _write_docx(body="") + with pytest.raises(document.DocumentReadError, match="no readable text"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_table_markdown_escapes_pipes_and_skips_empty_tables(): + """Tables render as markdown and empty tables are omitted.""" + body = ( + "a|b" + "" + ) + raw = _write_docx(body=body) + bundle = document.extract_review_document_bundle("docs/x.docx", raw) + assert "a\\|b" in bundle.text + assert "### Table 1" in bundle.text + + +def test_hwpx_media_discovery_and_attachment(monkeypatch): + """Manifest-bound HWPX media is attached alongside reviewed reader text.""" + png = b"\x89PNG\r\n\x1a\n" + raw = _write_hwpx( + sections={ + "section0": ("Contents/section0.xml", _hwpx_section("image1")) + }, + spine=["section0"], + media={"image1": ("BinData/image1.png", png)}, + ) + monkeypatch.setenv(document.HWP_READER_ENV, "/trusted/hwp-mcp-source") + completed = subprocess.CompletedProcess( + ["node"], 0, stdout=b"HWPX-TEXT\n", stderr=b"" + ) + monkeypatch.setattr(document.subprocess, "run", lambda *args, **kwargs: completed) + bundle = document.extract_review_document_bundle("docs/x.hwpx", raw) + assert "HWPX-TEXT" in bundle.text + assert len(bundle.images) == 1 + + +def test_hwpx_uses_manifest_and_section_order_with_reused_image(monkeypatch): + """HWPX figures follow spine/section order, not ZIP or filename order.""" + png_a = b"\x89PNG\r\n\x1a\nA" + png_b = b"\x89PNG\r\n\x1a\nB" + raw = _write_hwpx( + sections={ + "section0": ("Contents/section0.xml", _hwpx_section("imageA")), + "section1": ( + "Contents/section1.xml", + _hwpx_section("imageB", "imageA"), + ), + }, + spine=["section1", "section0"], + media={ + "imageA": ("BinData/a.png", png_a), + "imageB": ("BinData/b.png", png_b), + }, + extra_media={"BinData/orphan.png": b"orphan"}, + ) + monkeypatch.setenv(document.HWP_READER_ENV, "/trusted/hwp-mcp-source") + monkeypatch.setattr( + document.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess( + ["node"], 0, stdout=b"HWPX-TEXT\n", stderr=b"" + ), + ) + + bundle = document.extract_review_document_bundle("docs/x.hwpx", raw) + + assert [image.media_path for image in bundle.images] == [ + "BinData/b.png", + "BinData/a.png", + "BinData/a.png", + ] + assert [image.data for image in bundle.images] == [png_b, png_a, png_a] + assert bundle.media_declared == 3 + assert "section-1" in bundle.images[0].locator + assert "imageB->BinData/b.png" in bundle.images[0].locator + assert "section-2" in bundle.images[2].locator + assert "imageA->BinData/a.png" in bundle.images[2].locator + assert all(image.data != b"orphan" for image in bundle.images) + + +def test_hwpx_locator_preserves_text_run_and_table_cell_positions(monkeypatch): + """Stable locators distinguish paragraph and table-cell picture positions.""" + raw = _write_hwpx( + sections={ + "section0": ( + "Contents/section0.xml", + _hwpx_table_section("imageB", "imageA"), + ) + }, + spine=["section0"], + media={ + "imageA": ("BinData/a.png", b"a"), + "imageB": ("BinData/b.png", b"b"), + }, + ) + monkeypatch.setenv(document.HWP_READER_ENV, "/trusted/hwp-mcp-source") + monkeypatch.setattr( + document.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess( + ["node"], 0, stdout=b"HWPX-TEXT\n", stderr=b"" + ), + ) + + bundle = document.extract_review_document_bundle("docs/x.hwpx", raw) + + assert [image.media_path for image in bundle.images] == [ + "BinData/b.png", + "BinData/a.png", + ] + assert "/p-1/run-1/pic-1:" in bundle.images[0].locator + assert "/tbl-1/tr-1/tc-1/subList-1/p-1/run-1/pic-1:" in bundle.images[1].locator + + +def test_hwpx_relationship_helpers_reject_invalid_shapes(): + """Direct relationship helpers reject unsafe paths and missing picture refs.""" + with pytest.raises(document.DocumentReadError, match="outside Contents"): + document._safe_hwpx_section_path("../section0.xml") + with pytest.raises(document.DocumentReadError, match="unsupported image media"): + document._safe_hwpx_media_path( + "image1", "BinData/image1.bin", "application/octet-stream", "1" + ) + picture_without_image = document.ET.fromstring("") + with pytest.raises(document.DocumentReadError, match="missing or ambiguous"): + document._hwpx_picture_references( + picture_without_image, + section_number=1, + manifest_items={}, + ) + + +@pytest.mark.parametrize( + ("entries", "message"), + [ + ( + [("BinData/a.png", b"a"), ("BinData/a.png", b"b")], + "duplicate entry names", + ), + ([("BinData/a.png", b"a")], "no Contents/content.hpf"), + ( + [("BinData/a.png", b"a"), ("Contents/content.hpf", "', + ), + ], + "no manifest-bound section relationship", + ), + ( + [ + ("BinData/a.png", b"a"), + ( + "Contents/content.hpf", + '', + ), + ], + "section relationship section0 is unreadable", + ), + ( + [ + ("BinData/a.png", b"a"), + ( + "Contents/content.hpf", + '', + ), + ("Contents/section0.xml", "', + ), + ("Contents/section0.xml", "
"), + ], + "not referenced by any section picture", + ), + ], +) +def test_hwpx_malformed_package_boundaries_fail_closed(entries, message): + """Malformed package and section boundaries never fall back to filename order.""" + with pytest.raises(document.DocumentReadError, match=message): + document._hwpx_media_references(_write_zip(entries)) + + +def test_hwpx_without_image_entries_has_no_multimodal_references(): + """A text-only archive does not invent image relationships.""" + raw = _write_zip([("Contents/section0.xml", "
")]) + assert document._hwpx_media_references(raw) == ([], []) + + +@pytest.mark.parametrize( + ("manifest_rows", "section", "message"), + [ + ( + '' + '' + '', + _hwpx_section("image1"), + "duplicate manifest ID", + ), + ( + '' + '', + _hwpx_section("image1"), + "outside BinData", + ), + ( + '' + '', + _hwpx_section("image1"), + "external relationship", + ), + ( + '' + '', + _hwpx_section("missing"), + "unresolved relationship", + ), + ], +) +def test_hwpx_rejects_ambiguous_or_unsafe_image_relationships( + monkeypatch, manifest_rows, section, message +): + """HWPX image admission fails closed on ambiguous or unsafe mappings.""" + raw = _write_hwpx( + sections={"section0": ("Contents/section0.xml", section)}, + spine=["section0"], + media={"image1": ("BinData/a.png", b"png")}, + manifest_rows=manifest_rows, + ) + monkeypatch.setenv(document.HWP_READER_ENV, "/trusted/hwp-mcp-source") + monkeypatch.setattr( + document.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess( + ["node"], 0, stdout=b"HWPX-TEXT\n", stderr=b"" + ), + ) + + with pytest.raises(document.DocumentReadError, match=message): + document.extract_review_document_bundle("docs/x.hwpx", raw) + + +def test_hwpx_non_zip_input_has_no_media_names(): + """Classic HWP bytes are not treated as ZIP media containers.""" + assert document._hwpx_media_names(b"not-a-zip") == [] + + +def test_hwpx_unreadable_archive_with_media_fails_closed(monkeypatch): + """Broken HWPX media archives fail closed.""" + monkeypatch.setenv(document.HWP_READER_ENV, "/trusted/hwp-mcp-source") + completed = subprocess.CompletedProcess( + ["node"], 0, stdout=b"HWPX-TEXT\n", stderr=b"" + ) + monkeypatch.setattr(document.subprocess, "run", lambda *args, **kwargs: completed) + + def broken_zip(*args, **kwargs): + raise zipfile.BadZipFile("broken") + + monkeypatch.setattr(document.zipfile, "ZipFile", broken_zip) + monkeypatch.setattr( + document, + "_hwpx_media_references", + lambda raw: (["BinData/image1.png"], ["section-1/p-1:image1"]), + ) + with pytest.raises(document.DocumentReadError, match="unreadable"): + document.extract_review_document_bundle("docs/x.hwpx", b"zip") + + +def test_hwp_reader_missing_configuration(monkeypatch): + """Missing reader configuration fails closed.""" + monkeypatch.delenv(document.HWP_READER_ENV, raising=False) + with pytest.raises(document.DocumentReadError, match="not configured"): + document.extract_review_document_bundle("docs/x.hwp", b"binary") + + +def test_hwp_reader_additional_failure_paths(monkeypatch): + """Additional reviewed-reader subprocess failures are bounded.""" + monkeypatch.setenv(document.HWP_READER_ENV, "/trusted/hwp-mcp-source") + + def start_failure(*args, **kwargs): + raise OSError("node missing") + + monkeypatch.setattr(document.subprocess, "run", start_failure) + with pytest.raises(document.DocumentReadError, match="could not start"): + document.extract_review_document("docs/x.hwp", b"binary") + + monkeypatch.setattr( + document.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess( + ["node"], 0, stdout=b"x" * (document.MAX_DOCUMENT_TEXT_BYTES + 1), stderr=b"" + ), + ) + with pytest.raises(document.DocumentReadError, match="exceeded the bounded output"): + document.extract_review_document("docs/x.hwp", b"binary") + + monkeypatch.setattr( + document.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess( + ["node"], 0, stdout=b"\xff\xfe", stderr=b"" + ), + ) + with pytest.raises(document.DocumentReadError, match="non-UTF-8"): + document.extract_review_document("docs/x.hwp", b"binary") + + monkeypatch.setattr( + document.subprocess, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess( + ["node"], 0, stdout=b" \n", stderr=b"" + ), + ) + with pytest.raises(document.DocumentReadError, match="empty text"): + document.extract_review_document("docs/x.hwp", b"binary") + + +def test_bounded_text_truncates_large_output(): + """Reader output is clipped to the bounded text budget.""" + clipped = document._bounded_text("x" * (document.MAX_DOCUMENT_TEXT_BYTES + 50)) + assert "truncated" in clipped + + +def test_docx_rejects_unpacked_size_limit(monkeypatch): + """DOCX unpacked-size bounds are enforced from ZipInfo metadata.""" + monkeypatch.setattr(document, "MAX_DOCUMENT_ZIP_UNCOMPRESSED_BYTES", 16) + raw = _write_docx(body="this body exceeds the test limit") + with pytest.raises(document.DocumentReadError, match="bounded unpacked size"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_rejects_textless_body_without_figures(): + """Empty DOCX bodies without figures fail closed.""" + raw = _write_docx(body="") + with pytest.raises(document.DocumentReadError, match="no readable text"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_docx_media_read_keyerror_is_fail_closed(monkeypatch): + """Unreadable declared media entries fail closed.""" + raw = _write_docx(media={"word/media/a.png": b"png"}) + + class BrokenZip(zipfile.ZipFile): + def read(self, name, pwd=None): + if name == "word/media/a.png": + raise KeyError(name) + return super().read(name, pwd) + + monkeypatch.setattr(document.zipfile, "ZipFile", BrokenZip) + with pytest.raises(document.DocumentReadError, match="declared but unreadable"): + document.extract_review_document_bundle("docs/x.docx", raw) + + +def test_hwpx_rejects_too_many_zip_entries(): + """HWPX entry-count bounds are enforced.""" + output = io.BytesIO() + with zipfile.ZipFile(output, "w") as archive: + for index in range(document.MAX_DOCUMENT_ZIP_ENTRIES + 1): + archive.writestr(f"entry/{index}.txt", b"x") + with pytest.raises(document.DocumentReadError, match="too many entries"): + document._hwpx_media_names(output.getvalue()) + + +def test_hwpx_rejects_unpacked_size_limit(monkeypatch): + """HWPX unpacked-size bounds are enforced.""" + monkeypatch.setattr(document, "MAX_DOCUMENT_ZIP_UNCOMPRESSED_BYTES", 16) + output = io.BytesIO() + with zipfile.ZipFile(output, "w") as archive: + archive.writestr("Contents/section0.xml", b"
too-large-for-limit
") + with pytest.raises(document.DocumentReadError, match="bounded unpacked size"): + document._hwpx_media_names(output.getvalue()) + + +def test_hwpx_skips_directory_entries(): + """Directory entries are ignored during HWPX media discovery.""" + raw = _write_hwpx( + sections={ + "section0": ("Contents/section0.xml", _hwpx_section("image1")) + }, + spine=["section0"], + media={"image1": ("BinData/image1.png", b"png")}, + extra_media={"BinData/": b""}, + ) + assert document._hwpx_media_names(raw) == ["BinData/image1.png"] + + +def test_hwpx_empty_media_bytes_fail_closed(monkeypatch): + """Empty HWPX media entries fail closed during bundle extraction.""" + raw = _write_hwpx( + sections={ + "section0": ("Contents/section0.xml", _hwpx_section("image1")) + }, + spine=["section0"], + media={"image1": ("BinData/image1.png", b"")}, + ) + monkeypatch.setenv(document.HWP_READER_ENV, "/trusted/hwp-mcp-source") + completed = subprocess.CompletedProcess( + ["node"], 0, stdout=b"HWPX-TEXT\n", stderr=b"" + ) + monkeypatch.setattr(document.subprocess, "run", lambda *args, **kwargs: completed) + with pytest.raises(document.DocumentReadError, match="is empty"): + document.extract_review_document_bundle("docs/x.hwpx", raw) + + +def test_module_main_entrypoint(tmp_path: Path, monkeypatch): + """Running the module as __main__ exits through the CLI wrapper.""" + import runpy + import sys + + docx = tmp_path / "sample.docx" + docx.write_bytes(_write_docx()) + monkeypatch.setattr( + document, + "extract_review_document", + lambda path, raw: "MAIN-TEXT", + ) + monkeypatch.setattr(sys, "argv", ["noema_review_document.py", str(docx)]) + with pytest.warns(RuntimeWarning, match="found in sys.modules"): + with pytest.raises(SystemExit) as exc: + runpy.run_module("scripts.ci.noema_review_document", run_name="__main__") + assert exc.value.code == 0 + + +def test_main_cli_smoke(tmp_path: Path, monkeypatch, capsys): + """The module CLI prints extracted text for local smoke tests.""" + import sys + + docx = tmp_path / "sample.docx" + docx.write_bytes(_write_docx()) + monkeypatch.setattr( + document, + "extract_review_document", + lambda path, raw: "CLI-TEXT", + ) + monkeypatch.setattr( + document, + "extract_review_document_bundle", + lambda path, raw: document.DocumentExtraction(path=path, text="CLI-BUNDLE", images=[]), + ) + monkeypatch.setattr(sys, "argv", ["noema_review_document.py", str(docx)]) + assert document._main() == 0 + assert "CLI-TEXT" in capsys.readouterr().out + + monkeypatch.setattr( + sys, + "argv", + ["noema_review_document.py", str(docx), "--allow-figures"], + ) + assert document._main() == 0 + + missing = tmp_path / "missing.docx" + monkeypatch.setattr(sys, "argv", ["noema_review_document.py", str(missing)]) + assert document._main() == 1 diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 5053645802..2ebe5e7d8a 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1304,11 +1304,13 @@ def test_current_actor_rejects_unbound_action_identity(monkeypatch, actor, insta def test_review_context_builders_include_threads_and_files(monkeypatch, tmp_path): assert noema.truncate_text("abc", 10) == "abc" assert "truncated 2 characters" in noema.truncate_text("abcdef", 4) - assert "missing PR head SHA" in noema.changed_file_context("owner/repo", 7, "") + missing_head, _parts = noema.changed_file_context("owner/repo", 7, "") + assert "missing PR head SHA" in missing_head original_fetch_changed_files = noema.fetch_changed_files monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: []) - assert "no changed files" in noema.changed_file_context("owner/repo", 7, "head") + no_files, _parts = noema.changed_file_context("owner/repo", 7, "head") + assert "no changed files" in no_files monkeypatch.setattr(noema, "fetch_changed_files", original_fetch_changed_files) encoded = base64.b64encode(b"print('hello')\n").decode("ascii") @@ -1370,9 +1372,11 @@ def test_review_context_reports_omitted_files(monkeypatch, tmp_path): "fetch_changed_files", lambda repo, number: [(path, "modified") for path in paths], ) - monkeypatch.setattr(noema, "fetch_file_content_at_ref", lambda repo, path, ref: "x") + monkeypatch.setattr( + noema, "fetch_file_review_bundle", lambda repo, path, ref: ("x", []) + ) - context = noema.changed_file_context("owner/repo", 7, "head") + context, _parts = noema.changed_file_context("owner/repo", 7, "head") assert "1 changed files omitted from context budget" in context diff --git a/tests/test_product_technical_gap_baseline.py b/tests/test_product_technical_gap_baseline.py index d44ffdb8e6..07a9559d49 100644 --- a/tests/test_product_technical_gap_baseline.py +++ b/tests/test_product_technical_gap_baseline.py @@ -98,3 +98,37 @@ def test_master_context_points_at_live_baseline_without_freezing_shas() -> None: assert "ContextualWisdomLab/naruon#975" in source assert "Done" in source assert "merge authorization" in source + +def test_baseline_preserves_protected_main_authority_sections() -> None: + """Partial-file replacements must not erase protected Gap evidence.""" + + source = BASELINE.read_text(encoding="utf-8") + for marker in ( + "## 2026-08-30 sidecar-preflight outage: consolidated evidence and why it is not one deterministic bug", + "## 2026-08-30 ZDR/NIM-routing architecture review (owner-directed)", + "## 2026-09-01 OpenCode contextual-orchestrator runtime ceiling", + "## 6. Compliance and data boundary", + "## 7. APA 7th references", + "## Noema reviewer credential-lifetime delta — 2026-09-01", + ): + assert marker in source, marker + +def test_noema_multimodal_owner_row_preserves_verified_repair_lineage() -> None: + """Current owner evidence must retain every verified multimodal repair.""" + source = BASELINE.read_text(encoding="utf-8") + owner_row = next( + line + for line in source.splitlines() + if line.startswith("| CONTROL-NOEMA-MULTIMODAL-OWNER-02 ") + ) + required_evidence = ( + "7f69bacb0d35f00e6902df8e440efeafbe08dbe3", + "37435b5e82e9fe53abc67b032c67df83425c0250", + "b7440092d1cda47008271ed658fe372f536dd58f", + "5fa1c8b8f19353d712d6578c4af4c75e96f6988b", + "10f96453a6050ab47575e4975aa068cb4f899e23", + "c3e4e94cf6566f6a0187c502d279dccc6989d4a1", + "ad45a74f0acdddcd023646661413e76a120b88e3", + ) + assert all(evidence in owner_row for evidence in required_evidence) + diff --git a/tests/test_repository_branch_coverage_javascript_and_noema.py b/tests/test_repository_branch_coverage_javascript_and_noema.py index caeca87236..c72a7ba5ab 100644 --- a/tests/test_repository_branch_coverage_javascript_and_noema.py +++ b/tests/test_repository_branch_coverage_javascript_and_noema.py @@ -176,7 +176,7 @@ def test_noema_review_context_includes_locations_bodies_and_all_sections( assert "src/runtime.py:7" in rendered assert "reviewer: Fix this" in rendered - monkeypatch.setattr(noema, "changed_file_context", lambda *_args: "files") + monkeypatch.setattr(noema, "changed_file_context", lambda *_args: ("files", [])) context = noema.build_review_context("owner/repo", 1, pr) assert "Prior review threads" in context assert "Changed file context" in context diff --git a/tests/test_repository_branch_coverage_reporting_edges.py b/tests/test_repository_branch_coverage_reporting_edges.py index f5dbf1dae0..c84f4130fb 100644 --- a/tests/test_repository_branch_coverage_reporting_edges.py +++ b/tests/test_repository_branch_coverage_reporting_edges.py @@ -130,7 +130,7 @@ def test_noema_small_diff_and_empty_context_branches( assert rendered_context == "- Thread open at src/runtime.py:\n - reviewer: note" monkeypatch.setattr(noema, "review_thread_context", lambda _pr: "") - monkeypatch.setattr(noema, "changed_file_context", lambda *_args: "") + monkeypatch.setattr(noema, "changed_file_context", lambda *_args: ("", [])) assert noema.build_review_context("owner/repo", 1, pr) == "" diff --git a/tests/test_trusted_uv_materializer_quality_workflow_contract.py b/tests/test_trusted_uv_materializer_quality_workflow_contract.py index 50a5ddb5fe..64e0773a49 100644 --- a/tests/test_trusted_uv_materializer_quality_workflow_contract.py +++ b/tests/test_trusted_uv_materializer_quality_workflow_contract.py @@ -26,6 +26,7 @@ def test_quality_workflow_runs_for_every_materializer_surface() -> None: '"tests/test_uv*.py"', '"tests/test_repository_branch_coverage_*.py"', '"requirements-opencode-review-ci-hashes.txt"', + '"requirements-noema-document-ci-hashes.txt"', '"pyproject.toml"', ) for required_path in required_paths: @@ -70,7 +71,13 @@ def test_full_quality_gate_proves_tests_coverage_docstrings_and_compilation() -> assert 'python-version: "3.14"' in workflow assert ( "python -m pip install --disable-pip-version-check --require-hashes " - "-r requirements-opencode-review-ci-hashes.txt" + "-r requirements-opencode-review-ci-hashes.txt " + "-r requirements-noema-document-ci-hashes.txt" + ) in workflow + assert ( + "cache-dependency-path: |\n" + " requirements-opencode-review-ci-hashes.txt\n" + " requirements-noema-document-ci-hashes.txt" ) in workflow assert "branch = True" in workflow assert "scripts/ci/materialize_base_python_requirements.py" in workflow