From bddeb901229a84113c2ad3f25f4a12faee23e962 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 22 Sep 2026 01:14:32 +0900 Subject: [PATCH 1/5] ci(noema): review binary-only document PRs via a base/head object diff A binary DOCX/HWPX/PDF/image change arrives as 'Binary files ... differ' with no hunk, so Noema had no changed line to cite and every formal verdict failed; PDF/image bytes were also decoded into the prompt. Materialize base/head blobs, extract hashed document objects under fail-closed safety checks, emit document_diff_review.v1, and render the changes as synthetic hunks whose line numbers are object ordinals. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LeLbwEgYXzCzpaFMTHDcBS --- .github/actions/noema-review/two_phase.py | 1 + CHANGELOG.md | 8 + scripts/ci/document_blob_diff.py | 466 ++++++++++++++++++++++ scripts/ci/noema_review_gate.py | 71 ++++ tests/test_document_blob_diff.py | 403 +++++++++++++++++++ 5 files changed, 949 insertions(+) create mode 100644 scripts/ci/document_blob_diff.py create mode 100644 tests/test_document_blob_diff.py diff --git a/.github/actions/noema-review/two_phase.py b/.github/actions/noema-review/two_phase.py index c850da81b9..04301b27a0 100755 --- a/.github/actions/noema-review/two_phase.py +++ b/.github/actions/noema-review/two_phase.py @@ -164,6 +164,7 @@ def prepare_verdict(repo: str, number: int, expected_head: str, path: Path) -> i return 0 diff, truncated = gate.fetch_diff(repo, number) + diff, truncated = gate.augment_binary_document_diff(repo, pull_request, diff, truncated) changed_files = gate.fetch_changed_files(repo, number) changed_paths = tuple(file_path for file_path, _status in changed_files) review_context = gate.build_review_context(repo, number, pull_request, changed_files) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34281625cb..c9242480d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +### Noema reviews binary-only document PRs through a base/head object diff + +- A DOCX/HWPX/PDF/image change reaches Noema as `Binary files … differ` with no hunk. `changed_diff_locations()` returned an empty set, so `validate_substantive_verdict()` raised "requires parseable changed-line evidence" for every approve or request_changes, and a binary-only document PR could never get a formal verdict. That empty textual patch is an unsupported textual diff, not "no change". Separately, `fetch_file_content_at_ref()` decoded PDF and image bytes with `errors="replace"` straight into the prompt. +- New `scripts/ci/document_blob_diff.py` (stdlib only) materializes the base blob at the merge base and the head blob (contents API for the blob SHA, then the Git blobs API for the bytes) and extracts ordered objects. DOCX yields body paragraphs, tables and relationship-resolved figures; HWPX yields manifest/section paragraphs, nested tables and `binaryItemIDRef` figures; PDF and images are one opaque hashed `page` object. Every object carries a sha256. The objects are aligned into added, removed, modified and neighbouring unchanged objects, and a byte change with no object change becomes a package-level `style` object, never "no change". The result is emitted as `document_diff_review.v1` (contextual-orchestrator#1220 contract; all five fixture envelopes pass that PR's validator). +- Fail-closed safety: blob size, member count, total expansion, per-member compression ratio, traversal, encrypted members, macros (`vbaProject.bin`, `Scripts/`, `macroEnabled`), external image/OLE/template/frame relationships, external HWPX manifest hrefs, DTD/entity XML, unresolved figures, participant-material directories, and email, resident-registration-number, mobile-number and secret patterns in extracted text all reject the review instead of redacting. Original bytes and media never leave the runner; figures and pages carry only hashes. +- `noema_review_gate.augment_binary_document_diff()`, called from both the direct and the two-phase paths, replaces each binary document stanza with synthetic hunks whose LEFT/RIGHT line numbers are base/head object ordinals. The existing citation validator therefore accepts findings on document objects unchanged. Opaque binaries are no longer decoded into the changed-file context. +- Tests: `tests/test_document_blob_diff.py` (37). The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. + ### 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/scripts/ci/document_blob_diff.py b/scripts/ci/document_blob_diff.py new file mode 100644 index 0000000000..03f173be17 --- /dev/null +++ b/scripts/ci/document_blob_diff.py @@ -0,0 +1,466 @@ +"""Object-level base/head diff for binary review documents, fail closed. + +GitHub renders a changed DOCX/HWPX/PDF/image as ``Binary files … differ`` with +no hunk. An empty textual patch is not "no change"; it is an unsupported +textual diff. This module turns the base and head blobs into ordered document +objects (paragraphs, tables, figures, or one opaque blob), diffs them, and +emits the ``document_diff_review.v1`` envelope. Only bounded extracted text and +sha256 hashes leave the runner. Original bytes, embedded media, macros, +external relationships, and participant or secret patterns never do: they +raise :class:`DocumentSafetyError` instead. +""" + +from __future__ import annotations + +import difflib +import hashlib +import io +import re +import zipfile +from collections.abc import Callable, Sequence +from dataclasses import dataclass +from pathlib import PurePosixPath +from typing import Any +from xml.etree import ElementTree + +EXTRACTOR_VERSION = "document_blob_diff/1" +CONTRACT_VERSION = "document_diff_review.v1" +PACKAGE_SUFFIXES = frozenset({".docx", ".hwpx"}) +OPAQUE_SUFFIXES = frozenset( + {".pdf", ".png", ".jpg", ".jpeg", ".gif", ".webp", ".tif", ".tiff", ".bmp"} +) +REVIEW_DOCUMENT_SUFFIXES = PACKAGE_SUFFIXES | OPAQUE_SUFFIXES +MAX_BLOB_BYTES = 25 * 1024 * 1024 +MAX_MEMBERS = 1000 +MAX_UNCOMPRESSED_BYTES = 200 * 1024 * 1024 +MAX_MEMBER_RATIO = 200 +RATIO_FLOOR_BYTES = 1024 * 1024 +MAX_OBJECTS = 200 +MAX_OBJECT_TEXT = 8 * 1024 +MAX_ENVELOPE_TEXT = 256 * 1024 +# Relationship types that make a reader fetch or execute remote content. A +# plain hyperlink is metadata and stays allowed. +_REMOTE_CONTENT_REL = re.compile(r"/(image|oleObject|attachedTemplate|frame|subDocument|package)$") +_PARTICIPANT_DIRECTORY = re.compile( + r"(^|/)(participants|participant_data|raw_data|subjects|interviews|consent|consents|pii)(/|$)", + re.IGNORECASE, +) +_PARTICIPANT_PATTERNS = ( + re.compile(r"[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}"), + re.compile(r"(? bool: + """Return whether ``path`` is a binary document this module reviews.""" + return PurePosixPath(path).suffix.lower() in REVIEW_DOCUMENT_SUFFIXES + + +def _sha256(data: bytes) -> str: + """Return the lowercase hex sha256 of ``data``.""" + return hashlib.sha256(data).hexdigest() + + +def _local(tag: str) -> str: + """Return an XML tag without its namespace.""" + return tag.rsplit("}", 1)[-1] + + +def open_package(raw: bytes) -> zipfile.ZipFile: + """Open a DOCX/HWPX container after size, bomb, traversal and macro checks.""" + if len(raw) > MAX_BLOB_BYTES: + raise DocumentSafetyError(f"blob exceeds {MAX_BLOB_BYTES} bytes") + try: + package = zipfile.ZipFile(io.BytesIO(raw)) + except zipfile.BadZipFile as exc: + raise DocumentSafetyError("document is not a readable ZIP package") from exc + members = package.infolist() + if len(members) > MAX_MEMBERS: + raise DocumentSafetyError(f"package has more than {MAX_MEMBERS} members") + total = 0 + for member in members: + name = member.filename + if name.startswith("/") or ".." in PurePosixPath(name).parts or "\\" in name: + raise DocumentSafetyError(f"package member path is unsafe: {name!r}") + if member.flag_bits & 0x1: + raise DocumentSafetyError("package member is encrypted") + lowered = name.lower() + if lowered.endswith("vbaproject.bin") or lowered.startswith("scripts/"): + raise DocumentSafetyError(f"package contains macro content: {name!r}") + total += member.file_size + if ( + member.file_size > RATIO_FLOOR_BYTES + and member.file_size > MAX_MEMBER_RATIO * max(member.compress_size, 1) + ): + raise DocumentSafetyError(f"package member compression ratio is unsafe: {name!r}") + if total > MAX_UNCOMPRESSED_BYTES: + raise DocumentSafetyError(f"package expands beyond {MAX_UNCOMPRESSED_BYTES} bytes") + content_types = _read_optional(package, "[Content_Types].xml") + if content_types is not None and b"macroEnabled" in content_types: + raise DocumentSafetyError("package declares macro-enabled content") + return package + + +def _read_optional(package: zipfile.ZipFile, name: str) -> bytes | None: + """Read one member or return None when it is absent.""" + try: + return package.read(name) + except KeyError: + return None + + +def _parse(data: bytes, name: str) -> ElementTree.Element: + """Parse package XML, failing closed on malformed or DTD-bearing input.""" + if b" str: + """Join ``text_tag`` descendants, skipping any subtree rooted in ``skip``.""" + parts: list[str] = [] + + def walk(node: ElementTree.Element) -> None: + """Depth-first text collection that honours ``skip``.""" + if skip and id(node) in skip: + return + if _local(node.tag) == text_tag and node.text: + parts.append(node.text) + for child in node: + walk(child) + + walk(element) + return "".join(parts) + + +def _table_text(table: ElementTree.Element, row_tag: str, cell_tag: str, text_tag: str) -> str: + """Render a table as ``cell | cell`` rows separated by newlines.""" + rows = [] + for row in table.iter(): + if _local(row.tag) != row_tag: + continue + cells = [_text(cell, text_tag) for cell in row if _local(cell.tag) == cell_tag] + rows.append(" | ".join(cells)) + return "\n".join(rows) + + +def _figure(package: zipfile.ZipFile, target: str, locator: str) -> DocumentObject: + """Hash one embedded media member; a missing target fails closed.""" + data = _read_optional(package, target) + if data is None: + raise DocumentSafetyError(f"figure target is missing: {target!r}") + return DocumentObject("figure", locator, _sha256(data), None) + + +def _text_object(kind: str, locator: str, text: str) -> DocumentObject: + """Hash a paragraph/table by its extracted text.""" + return DocumentObject(kind, locator, _sha256(text.encode("utf-8")), text) + + +def extract_docx(raw: bytes) -> list[DocumentObject]: + """Return DOCX body paragraphs, tables and figures in document order.""" + package = open_package(raw) + document = _read_optional(package, "word/document.xml") + if document is None: + raise DocumentSafetyError("DOCX has no word/document.xml") + relationships: dict[str, str] = {} + rels = _read_optional(package, "word/_rels/document.xml.rels") + if rels is not None: + for rel in _parse(rels, "document.xml.rels"): + if rel.get("TargetMode") == "External" and _REMOTE_CONTENT_REL.search(rel.get("Type", "")): + raise DocumentSafetyError(f"DOCX has an external {rel.get('Type', '').rsplit('/', 1)[-1]} relationship") + target = rel.get("Target", "") + relationships[rel.get("Id", "")] = target[1:] if target.startswith("/") else "word/" + target + body = next((node for node in _parse(document, "document.xml") if _local(node.tag) == "body"), None) + if body is None: + raise DocumentSafetyError("DOCX document has no body") + objects: list[DocumentObject] = [] + paragraphs = tables = 0 + for node in body: + kind = _local(node.tag) + if kind == "tbl": + tables += 1 + objects.append(_text_object("table", f"tbl{tables}", _table_text(node, "tr", "tc", "t"))) + elif kind == "p": + paragraphs += 1 + text = _text(node, "t") + if text: + objects.append(_text_object("paragraph", f"p{paragraphs}", text)) + for blip in (d for d in node.iter() if _local(d.tag) == "blip"): + rel_id = next((v for k, v in blip.attrib.items() if _local(k) == "embed"), "") + if rel_id not in relationships: + raise DocumentSafetyError(f"DOCX figure relationship is unresolved: {rel_id!r}") + objects.append(_figure(package, relationships[rel_id], f"p{paragraphs}/fig:{relationships[rel_id]}")) + return objects + + +def extract_hwpx(raw: bytes) -> list[DocumentObject]: + """Return HWPX section paragraphs, tables and figures in document order.""" + package = open_package(raw) + manifest = _read_optional(package, "Contents/content.hpf") + if manifest is None: + raise DocumentSafetyError("HWPX has no Contents/content.hpf") + items: dict[str, str] = {} + for item in _parse(manifest, "content.hpf").iter(): + if _local(item.tag) == "item": + href = item.get("href", "") + if re.match(r"^[a-z][a-z0-9+.-]*:", href, re.IGNORECASE): + raise DocumentSafetyError(f"HWPX manifest references external content: {href!r}") + items[item.get("id", "")] = href + sections = sorted( + (n for n in package.namelist() if re.fullmatch(r"Contents/section\d+\.xml", n)), + key=lambda n: int(re.search(r"\d+", n).group()), + ) + if not sections: + raise DocumentSafetyError("HWPX has no Contents/section*.xml") + objects: list[DocumentObject] = [] + paragraphs = tables = 0 + for section in sections: + for node in _parse(package.read(section), section): + if _local(node.tag) != "p": + continue + paragraphs += 1 + nested = [d for d in node.iter() if _local(d.tag) == "tbl"] + text = _text(node, "t", skip={id(t) for t in nested}) + if text: + objects.append(_text_object("paragraph", f"p{paragraphs}", text)) + for table in nested: + tables += 1 + objects.append(_text_object("table", f"tbl{tables}", _table_text(table, "tr", "tc", "t"))) + for pic in (d for d in node.iter() if "binaryItemIDRef" in d.attrib): + ref = pic.get("binaryItemIDRef", "") + if ref not in items: + raise DocumentSafetyError(f"HWPX figure reference is unresolved: {ref!r}") + objects.append(_figure(package, items[ref], f"p{paragraphs}/fig:{items[ref]}")) + return objects + + +def extract_objects(path: str, raw: bytes) -> list[DocumentObject]: + """Extract ordered objects; PDF/images are one opaque hashed ``page`` object.""" + suffix = PurePosixPath(path).suffix.lower() + if suffix == ".docx": + return extract_docx(raw) + if suffix == ".hwpx": + return extract_hwpx(raw) + if suffix in OPAQUE_SUFFIXES: + if len(raw) > MAX_BLOB_BYTES: + raise DocumentSafetyError(f"blob exceeds {MAX_BLOB_BYTES} bytes") + return [DocumentObject("page", "blob", _sha256(raw), None)] + raise DocumentSafetyError(f"unsupported review document type: {suffix or path!r}") + + +def diff_objects( + base: Sequence[DocumentObject], head: Sequence[DocumentObject] +) -> list[tuple[str, int | None, DocumentObject | None, int | None, DocumentObject | None]]: + """Align base/head objects by (kind, hash) and return changes with context. + + Each entry is ``(change, base_ordinal, base_object, head_ordinal, + head_object)`` with 1-based ordinals; ``change`` is added, removed, + modified, or unchanged. Only the unchanged object on each side of a change + is kept, as body/table/caption consistency context. + """ + keys_a = [(o.kind, o.sha256) for o in base] + keys_b = [(o.kind, o.sha256) for o in head] + opcodes = difflib.SequenceMatcher(a=keys_a, b=keys_b, autojunk=False).get_opcodes() + changes = [] + for position, (tag, i1, i2, j1, j2) in enumerate(opcodes): + if tag == "equal": + keep = set() + if position > 0: + keep.add(0) + if position < len(opcodes) - 1: + keep.add(i2 - i1 - 1) + for offset in sorted(keep): + changes.append(("unchanged", i1 + offset + 1, base[i1 + offset], j1 + offset + 1, head[j1 + offset])) + continue + span = max(i2 - i1, j2 - j1) + for offset in range(span): + i, j = i1 + offset, j1 + offset + old = base[i] if i < i2 else None + new = head[j] if j < j2 else None + if old is not None and new is not None and old.kind == new.kind: + changes.append(("modified", i + 1, old, j + 1, new)) + continue + if old is not None: + changes.append(("removed", i + 1, old, None, None)) + if new is not None: + changes.append(("added", None, None, j + 1, new)) + return changes + + +def _check_text(text: str | None, locator: str, sensitive: Callable[[str], bool]) -> str | None: + """Bound one object's text and reject participant or secret patterns.""" + if text is None: + return None + if any(p.search(text) for p in _PARTICIPANT_PATTERNS) or sensitive(text): + raise DocumentSafetyError(f"participant or secret pattern in extracted text at {locator}") + return text[:MAX_OBJECT_TEXT] + + +@dataclass(frozen=True) +class DocumentReview: + """A CO-strict envelope plus the object ordinals used for citable hunks.""" + + envelope: dict[str, Any] + ordinals: tuple[tuple[int | None, int | None], ...] + + +def _hash(digest: str | None) -> str | None: + """Return the envelope's ``sha256:`` form of a digest.""" + return f"sha256:{digest}" if digest else None + + +def build_envelope( + repo: str, + path: str, + base_blob: str | None, + head_blob: str | None, + base_raw: bytes | None, + head_raw: bytes | None, + sensitive: Callable[[str], bool] = lambda _text: False, +) -> DocumentReview: + """Build a bounded ``document_diff_review.v1`` envelope for one path.""" + if _PARTICIPANT_DIRECTORY.search(path): + raise DocumentSafetyError(f"path is under a participant-material directory: {path}") + if base_blob is None and head_blob is None: + raise DocumentSafetyError("both base and head blobs are missing") + if base_blob == head_blob: + raise DocumentSafetyError("base and head blobs are identical") + base = extract_objects(path, base_raw) if base_raw is not None else [] + head = extract_objects(path, head_raw) if head_raw is not None else [] + objects: list[dict[str, Any]] = [] + ordinals: list[tuple[int | None, int | None]] = [] + total = 0 + for change, base_ord, old, head_ord, new in diff_objects(base, head): + subject = new or old + base_text = _check_text(old.text if old else None, subject.locator, sensitive) + head_text = _check_text(new.text if new else None, subject.locator, sensitive) + total += len(base_text or "") + len(head_text or "") + objects.append( + { + "page": None, + "object_kind": subject.kind, + "locator": subject.locator[:256], + "change": change, + "object_hash_base": _hash(old.sha256 if old else None), + "object_hash_head": _hash(new.sha256 if new else None), + "base_text": base_text, + "head_text": head_text, + } + ) + ordinals.append((base_ord, head_ord)) + if not any(o["change"] != "unchanged" for o in objects) and base_raw is not None and head_raw is not None: + # The bytes changed but no body object did (styles, settings, metadata): + # still a reviewable change, never "no change". + objects.append( + { + "page": None, + "object_kind": "style", + "locator": "package", + "change": "modified", + "object_hash_base": _hash(_sha256(base_raw)), + "object_hash_head": _hash(_sha256(head_raw)), + "base_text": None, + "head_text": None, + } + ) + ordinals.append((1, 1)) + if len(objects) > MAX_OBJECTS or total > MAX_ENVELOPE_TEXT: + raise DocumentSafetyError( + f"document diff exceeds review bounds ({len(objects)} objects, {total} text bytes)" + ) + envelope = { + "contract_version": CONTRACT_VERSION, + "repo": repo, + "path": path, + "base_blob": base_blob, + "head_blob": head_blob, + "extractor_version": EXTRACTOR_VERSION, + "participant_material": False, + "objects": objects, + } + return DocumentReview(envelope, tuple(ordinals)) + + +def _line(obj: dict[str, Any], side: str) -> str: + """Render one synthetic diff line for an object side.""" + digest = obj[f"object_hash_{side}"].removeprefix("sha256:")[:12] + text = obj[f"{side}_text"] + body = " ".join(text.split())[:500] if text is not None else "(no text extracted; hash only)" + return f"[{obj['object_kind']} {obj['locator']} sha256:{digest}] {body}" + + +def synthetic_hunks(review: DocumentReview, old_path: str | None = None) -> str: + """Render changed objects as unified-diff hunks citable as ``path:ordinal``. + + LEFT line numbers are base object ordinals and RIGHT line numbers are head + object ordinals, so existing changed-line validation accepts citations of + document objects without any new location type. Unchanged context objects + stay in the envelope only. + """ + envelope = review.envelope + path = envelope["path"] + old = old_path or path + lines = [ + f"diff --git a/{old} b/{path}", + f"--- a/{old}" if envelope["base_blob"] else "--- /dev/null", + f"+++ b/{path}" if envelope["head_blob"] else "+++ /dev/null", + ] + for obj, (left, new) in zip(envelope["objects"], review.ordinals): + if obj["change"] == "unchanged": + continue + lines.append(f"@@ -{left or 0},{1 if left else 0} +{new or 0},{1 if new else 0} @@ {obj['change']}") + if left: + lines.append("-" + _line(obj, "base")) + if new: + lines.append("+" + _line(obj, "head")) + return "\n".join(lines) + + +_BINARY_STANZA = re.compile(r"^Binary files (?:a/(?P.+)|/dev/null) and (?:b/(?P.+)|/dev/null) differ$") + + +def _stanza(block: str) -> tuple[str | None, str | None] | None: + """Return the ``(old, new)`` paths of a block's binary stanza, if any.""" + for line in block.splitlines(): + match = _BINARY_STANZA.match(line) + if match: + return match.group("old"), match.group("new") + return None + + +def binary_document_stanzas(diff: str) -> list[tuple[str | None, str | None]]: + """Return ``(old, new)`` review-document paths the diff reports only as binary.""" + stanzas = [] + for block in re.split(r"(?m)^(?=diff --git )", diff): + pair = _stanza(block) + if pair and is_review_document(pair[1] or pair[0]) and pair not in stanzas: + stanzas.append(pair) + return stanzas + + +def replace_binary_stanzas(diff: str, hunks: dict[tuple[str | None, str | None], str]) -> str: + """Replace each ``diff --git`` block of a reviewed binary pair with its hunks.""" + out = [] + for block in re.split(r"(?m)^(?=diff --git )", diff): + pair = _stanza(block) + out.append(hunks[pair] + "\n" if pair in hunks else block) + return "".join(out) diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index c8709304fc..e12abc6026 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -25,6 +25,7 @@ from typing import Any from scripts.ci.opencode_review_normalize_output import changed_file_is_material +from scripts.ci import document_blob_diff from scripts.ci.noema_review_document import DocumentReadError, extract_review_document @@ -580,6 +581,11 @@ def current_actor() -> str: def fetch_diff(repo: str, number: int) -> tuple[str, bool]: """Fetch the PR diff and truncate it to the bounded LLM prompt size.""" diff = run(["gh", "api", f"repos/{repo}/pulls/{number}", "-H", "Accept: application/vnd.github.v3.diff"]) + return bound_diff(diff) + + +def bound_diff(diff: str) -> tuple[str, bool]: + """Truncate a unified diff to ``MAX_DIFF_CHARS`` without splitting a changed line.""" truncated = len(diff) > MAX_DIFF_CHARS if truncated: marker = "[overlong changed line content omitted]" @@ -862,6 +868,9 @@ def fetch_file_content_at_ref(repo: str, path: str, ref: str) -> str: except (binascii.Error, ValueError) as exc: raise RuntimeError("GitHub content response contained malformed base64") from exc suffix = PurePosixPath(path).suffix.lower() + if suffix in document_blob_diff.OPAQUE_SUFFIXES: + # Never decode PDF/image bytes into the prompt; the object diff covers them. + return "[binary document: reviewed through the object-level diff; raw bytes are not sent]" if suffix in {".docx", ".hwp", ".hwpx"}: try: return extract_review_document(path, raw) @@ -870,6 +879,67 @@ def fetch_file_content_at_ref(repo: str, path: str, ref: str) -> str: return raw.decode("utf-8", errors="replace") +def fetch_file_blob_at_ref(repo: str, path: str, ref: str) -> tuple[str, bytes]: + """Fetch one file's blob SHA and raw bytes at an exact ref. + + The contents API names the blob; the Git blobs API returns its bytes for + files beyond the contents API's 1 MB inline limit. The bytes stay on the + runner: callers extract bounded objects from them and never forward them. + """ + encoded_path = urllib.parse.quote(path, safe="/") + encoded_ref = urllib.parse.quote(ref, safe="") + blob_sha = run( + ["gh", "api", f"repos/{repo}/contents/{encoded_path}?ref={encoded_ref}", "--jq", ".sha // empty"] + ).strip() + if not re.fullmatch(r"[0-9a-f]{40}", blob_sha): + raise RuntimeError(f"GitHub did not return a blob SHA for {path} at {ref}") + content = run(["gh", "api", f"repos/{repo}/git/blobs/{blob_sha}", "--jq", ".content // empty"]) + try: + raw = base64.b64decode("".join(content.split()), validate=True) + except (binascii.Error, ValueError) as exc: + raise RuntimeError("GitHub blob response contained malformed base64") from exc + return blob_sha, raw + + +def augment_binary_document_diff( + repo: str, pr: dict[str, Any], diff: str, truncated: bool +) -> tuple[str, bool]: + """Replace binary-only document stanzas with citable object-level hunks. + + A DOCX/HWPX/PDF/image change arrives as ``Binary files … differ`` with no + hunk, which left a formal verdict with no changed line to cite. The base + blob at the merge base and the head blob are materialized, diffed as + document objects, and rendered as synthetic hunks whose line numbers are + object ordinals. Any safety rejection fails the review closed; a changed + blob never becomes "no change". + """ + stanzas = document_blob_diff.binary_document_stanzas(diff) + if not stanzas: + return diff, truncated + head_sha = str(pr.get("headRefOid") or "") + merge_base = fetch_merge_base_sha(repo, str(pr.get("baseRefOid") or ""), head_sha) + hunks: dict[tuple[str | None, str | None], str] = {} + for old_path, new_path in stanzas: + base_blob, base_raw = fetch_file_blob_at_ref(repo, old_path, merge_base) if old_path else (None, None) + head_blob, head_raw = fetch_file_blob_at_ref(repo, new_path, head_sha) if new_path else (None, None) + path = new_path or old_path + try: + review = document_blob_diff.build_envelope( + repo, + path, + base_blob, + head_blob, + base_raw, + head_raw, + sensitive=lambda text: scrub_sensitive_data(text) != text, + ) + except document_blob_diff.DocumentSafetyError as exc: + raise RuntimeError(f"binary document review failed closed for {path}: {exc}") from exc + hunks[(old_path, new_path)] = document_blob_diff.synthetic_hunks(review, old_path) + bounded, more = bound_diff(document_blob_diff.replace_binary_stanzas(diff, hunks)) + return bounded, truncated or more + + def fetch_merge_base_sha(repo: str, base_sha: str, head_sha: str) -> str: """Return the immutable merge-base SHA for the current base/head pair.""" if not re.fullmatch(r"[0-9a-fA-F]{40}", base_sha): @@ -1927,6 +1997,7 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: print("Current head already has a Noema review; nothing to do.") return 0 diff, truncated = fetch_diff(repo, number) + diff, truncated = augment_binary_document_diff(repo, pr, diff, truncated) changed_files = fetch_changed_files(repo, number) changed_paths = tuple(path for path, _status in changed_files) review_context = build_review_context(repo, number, pr, changed_files) diff --git a/tests/test_document_blob_diff.py b/tests/test_document_blob_diff.py new file mode 100644 index 0000000000..2b078ea1e1 --- /dev/null +++ b/tests/test_document_blob_diff.py @@ -0,0 +1,403 @@ +"""Binary review documents diff as hashed objects, never as "no change".""" + +from __future__ import annotations + +import base64 +import io +import zipfile + +import pytest + +from scripts.ci import document_blob_diff as dbd +from scripts.ci import noema_review_gate as gate + +W = 'xmlns:w="http://schemas.openxmlformats.org/wordprocessingml/2006/main"' +A = 'xmlns:a="http://schemas.openxmlformats.org/drawingml/2006/main"' +R = 'xmlns:r="http://schemas.openxmlformats.org/officeDocument/2006/relationships"' +IMAGE_REL = "http://schemas.openxmlformats.org/officeDocument/2006/relationships/image" +LINK_REL = "http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink" + + +def _zip(members: dict[str, bytes | str], *, encrypt: str = "") -> bytes: + """Build an in-memory ZIP package; ``encrypt`` flags one member as encrypted.""" + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w", zipfile.ZIP_DEFLATED) as package: + for name, data in members.items(): + package.writestr(name, data) + raw = bytearray(buffer.getvalue()) + if encrypt: + # zipfile clears the encryption bit on write; set it in the central + # directory, which is what the reader's infolist() reports. + start = raw.index(b"PK\x01\x02") + raw[start + 8] |= 0x1 + return bytes(raw) + + +def _docx(paragraphs: list[str], *, table: list[list[str]] | None = None, image: bytes | None = None, + rels: str | None = None, extra: dict[str, bytes | str] | None = None, body: bool = True) -> bytes: + """Build a synthetic DOCX with paragraphs, an optional table and figure.""" + parts = [f"{text}" for text in paragraphs] + if table is not None: + rows = "".join( + "" + "".join(f"{c}" for c in row) + "" + for row in table + ) + parts.append(f"{rows}") + if image is not None: + parts.append('') + inner = f"{''.join(parts)}" if body else "" + members: dict[str, bytes | str] = { + "[Content_Types].xml": "", + "word/document.xml": f"{inner}", + } + if rels is None and image is not None: + rels = f'' + if rels is not None: + members["word/_rels/document.xml.rels"] = rels + if image is not None: + members["word/media/image1.png"] = image + members.update(extra or {}) + return _zip(members) + + +HP = 'xmlns:hp="http://www.hancom.co.kr/hwpml/2011/paragraph"' + + +def _hwpx(sections: list[str], *, manifest: str | None = None, extra: dict[str, bytes | str] | None = None) -> bytes: + """Build a synthetic HWPX with the given section bodies.""" + members: dict[str, bytes | str] = { + "Contents/content.hpf": manifest + if manifest is not None + else '' + '', + "BinData/image1.png": b"\x89PNG-one", + } + for index, body in enumerate(sections): + members[f"Contents/section{index}.xml"] = f"{body}" + members.update(extra or {}) + return _zip(members) + + +def _hp(text: str) -> str: + """One HWPX paragraph.""" + return f"{text}" + + +# ---------------------------------------------------------------- extraction + + +def test_docx_objects_are_ordered_hashed_and_resolve_figures() -> None: + """Paragraphs, tables and figures come back in body order with sha256.""" + raw = _docx(["Intro", "Method"], table=[["N", "1020"], ["M", "3.1"]], image=b"PNG1") + objects = dbd.extract_objects("paper.DOCX", raw) + assert [(o.kind, o.locator) for o in objects] == [ + ("paragraph", "p1"), + ("paragraph", "p2"), + ("table", "tbl1"), + ("figure", "p3/fig:word/media/image1.png"), + ] + assert objects[2].text == "N | 1020\nM | 3.1" + assert objects[3].text is None and len(objects[3].sha256) == 64 + + +def test_docx_package_absolute_relationship_target_and_hyperlinks_are_allowed() -> None: + """``/word/media`` targets resolve and external hyperlinks are plain metadata.""" + rels = ( + f'' + f'' + "" + ) + objects = dbd.extract_objects("a.docx", _docx([], image=b"PNG", rels=rels)) + assert objects[-1].locator == "p1/fig:word/media/image1.png" + + +def test_hwpx_objects_split_tables_from_paragraph_text_across_sections() -> None: + """Section order is numeric; a nested table is its own object.""" + table = "ab" + raw = _hwpx( + [ + _hp("first") + f"lead{table}", + '', + ] + + [""] * 9 + + [_hp("eleventh")] + ) + objects = dbd.extract_objects("x.hwpx", raw) + assert [(o.kind, o.locator, o.text) for o in objects] == [ + ("paragraph", "p1", "first"), + ("paragraph", "p2", "lead"), + ("table", "tbl1", "a | b"), + ("figure", "p3/fig:BinData/image1.png", None), + ("paragraph", "p4", "eleventh"), + ] + + +def test_opaque_documents_are_one_hashed_blob_object() -> None: + """PDF and images are never decoded; they are one hashed object.""" + (obj,) = dbd.extract_objects("scan.pdf", b"%PDF-1.7 binary") + assert (obj.kind, obj.locator, obj.text) == ("page", "blob", None) + assert dbd.is_review_document("fig.PNG") and not dbd.is_review_document("notes.md") + + +@pytest.mark.parametrize( + ("path", "raw", "message"), + ( + ("a.docx", b"not a zip", "not a readable ZIP"), + ("a.docx", _zip({"../evil": "x"}), "member path is unsafe"), + ("a.docx", _zip({"word\\evil": "x"}), "member path is unsafe"), + ("a.docx", _zip({"/abs": "x"}), "member path is unsafe"), + ("a.docx", _zip({"word/vbaProject.bin": "x"}), "macro content"), + ("a.hwpx", _zip({"Scripts/main.js": "x"}), "macro content"), + ("a.docx", _zip({"word/document.xml": "x"}, encrypt="word/document.xml"), "encrypted"), + ("a.docx", _zip({"big": b"\0" * (2 * 1024 * 1024)}), "compression ratio"), + ("a.docx", _zip({"[Content_Types].xml": "application/vnd.ms-word.document.macroEnabled"}), "macro-enabled"), + ("a.docx", _zip({"[Content_Types].xml": ""}), "no word/document.xml"), + ("a.docx", _docx(["x"], body=False), "no body"), + ("a.docx", _zip({"word/document.xml": ""}), "DTD or entity"), + ("a.docx", _zip({"word/document.xml": ""}), "not well-formed"), + ( + "a.docx", + _docx([], rels=f''), + "external image relationship", + ), + ("a.docx", _docx([], image=b"P", rels=""), "relationship is unresolved"), + ( + "a.docx", + _docx([], image=b"P", rels=f''), + "figure target is missing", + ), + ("a.hwpx", _zip({"x": "y"}), "no Contents/content.hpf"), + ("a.hwpx", _hwpx([], manifest=''), "external content"), + ("a.hwpx", _hwpx([]), "no Contents/section"), + ("a.hwpx", _hwpx(['']), "reference is unresolved"), + ("a.txt", b"x", "unsupported review document type"), + ("noext", b"x", "unsupported review document type"), + ), +) +def test_unsafe_or_malformed_documents_fail_closed(path: str, raw: bytes, message: str) -> None: + """Every unsafe or unreadable package raises instead of being skipped.""" + with pytest.raises(dbd.DocumentSafetyError, match=message): + dbd.extract_objects(path, raw) + + +def test_size_member_and_expansion_bounds_fail_closed(monkeypatch: pytest.MonkeyPatch) -> None: + """Blob size, member count and total expansion are bounded.""" + monkeypatch.setattr(dbd, "MAX_BLOB_BYTES", 10) + with pytest.raises(dbd.DocumentSafetyError, match="blob exceeds"): + dbd.extract_objects("a.docx", _docx(["x"])) + with pytest.raises(dbd.DocumentSafetyError, match="blob exceeds"): + dbd.extract_objects("a.pdf", b"%PDF-" + b"x" * 20) + monkeypatch.setattr(dbd, "MAX_BLOB_BYTES", 10**9) + monkeypatch.setattr(dbd, "MAX_MEMBERS", 1) + with pytest.raises(dbd.DocumentSafetyError, match="more than 1 members"): + dbd.extract_objects("a.docx", _docx(["x"])) + monkeypatch.setattr(dbd, "MAX_MEMBERS", 1000) + monkeypatch.setattr(dbd, "MAX_UNCOMPRESSED_BYTES", 10) + with pytest.raises(dbd.DocumentSafetyError, match="expands beyond"): + dbd.extract_objects("a.docx", _docx(["x"])) + + +# ---------------------------------------------------------------- diff + + +def _o(kind: str, key: str) -> dbd.DocumentObject: + """A tiny object whose hash is its key.""" + return dbd.DocumentObject(kind, key, key * 4, key) + + +def test_diff_objects_reports_changes_with_unchanged_neighbours() -> None: + """Alignment pairs same-kind replacements, splits kind changes, keeps context.""" + base = [_o("paragraph", "a"), _o("paragraph", "k"), _o("paragraph", "b"), _o("table", "t"), _o("paragraph", "z")] + head = [_o("paragraph", "a"), _o("paragraph", "k"), _o("paragraph", "B"), _o("figure", "f"), _o("paragraph", "y"), _o("paragraph", "n")] + changes = [(c, bo, ho) for c, bo, _o1, ho, _o2 in dbd.diff_objects(base, head)] + assert changes == [ + ("unchanged", 2, 2), + ("modified", 3, 3), + ("removed", 4, None), + ("added", None, 4), + ("modified", 5, 5), + ("added", None, 6), + ] + middle = [_o("paragraph", "x"), _o("paragraph", "m1"), _o("paragraph", "m2"), _o("paragraph", "m3"), _o("paragraph", "y")] + edited = [_o("paragraph", "X"), *middle[1:4], _o("paragraph", "Y")] + assert [(c, bo) for c, bo, *_ in dbd.diff_objects(middle, edited)] == [ + ("modified", 1), ("unchanged", 2), ("unchanged", 4), ("modified", 5), + ] + assert [c[0] for c in dbd.diff_objects(base, base[:2])] == ["unchanged", "removed", "removed", "removed"] + + +# ---------------------------------------------------------------- envelope + + +def test_envelope_matches_the_co_contract_and_hunks_are_citable() -> None: + """Envelope fields follow document_diff_review.v1; hunks cite object ordinals.""" + base = _docx(["Intro", "N = 957"], image=b"PNG-A") + head = _docx(["Intro", "N = 1,020"], image=b"PNG-B") + review = dbd.build_envelope("o/r", "paper.docx", "1" * 40, "2" * 40, base, head) + env = review.envelope + assert set(env) == { + "contract_version", "repo", "path", "base_blob", "head_blob", + "extractor_version", "participant_material", "objects", + } + assert env["contract_version"] == "document_diff_review.v1" and env["participant_material"] is False + assert [(o["object_kind"], o["change"], o["base_text"], o["head_text"]) for o in env["objects"]] == [ + ("paragraph", "unchanged", "Intro", "Intro"), + ("paragraph", "modified", "N = 957", "N = 1,020"), + ("figure", "modified", None, None), + ] + for obj in env["objects"]: + assert set(obj) == {"page", "object_kind", "locator", "change", "object_hash_base", "object_hash_head", "base_text", "head_text"} + assert obj["object_hash_base"].startswith("sha256:") and len(obj["object_hash_head"]) == 71 + assert env["objects"][0]["object_hash_base"] == env["objects"][0]["object_hash_head"] + hunks = dbd.synthetic_hunks(review) + assert gate.changed_diff_locations(hunks) == { + ("paper.docx", 2, "LEFT"), ("paper.docx", 2, "RIGHT"), + ("paper.docx", 3, "LEFT"), ("paper.docx", 3, "RIGHT"), + } + assert "(no text extracted; hash only)" in hunks + assert "PNG-A" not in hunks and "PNG-B" not in hunks and "Intro" not in hunks + + +def test_added_removed_and_style_only_changes_are_never_no_change() -> None: + """Added/removed documents and metadata-only edits still yield objects.""" + added = dbd.build_envelope("o/r", "n.pdf", None, "2" * 40, None, b"%PDF-new") + assert added.envelope["objects"][0]["object_kind"] == "page" + assert dbd.synthetic_hunks(added).splitlines()[1:3] == ["--- /dev/null", "+++ b/n.pdf"] + removed = dbd.build_envelope("o/r", "n.pdf", "1" * 40, None, b"%PDF-old", None) + hunks = dbd.synthetic_hunks(removed, "old/n.pdf") + assert hunks.splitlines()[:3] == ["diff --git a/old/n.pdf b/n.pdf", "--- a/old/n.pdf", "+++ /dev/null"] + style = dbd.build_envelope( + "o/r", "s.docx", "1" * 40, "2" * 40, _docx(["same"]), _docx(["same"], extra={"word/styles.xml": ""}) + ) + assert [(o["object_kind"], o["locator"], o["change"]) for o in style.envelope["objects"]] == [("style", "package", "modified")] + + +def test_envelope_rejects_identical_or_missing_blobs_participant_material_secrets_and_overflow( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Participant material or secrets are rejected, not redacted, and bounds hold.""" + with pytest.raises(dbd.DocumentSafetyError, match="identical"): + dbd.build_envelope("o/r", "a.pdf", "1" * 40, "1" * 40, b"x", b"y") + with pytest.raises(dbd.DocumentSafetyError, match="both base and head blobs are missing"): + dbd.build_envelope("o/r", "a.pdf", None, None, None, None) + with pytest.raises(dbd.DocumentSafetyError, match="participant-material directory"): + dbd.build_envelope("o/r", "study/Interviews/s1.docx", "1" * 40, "2" * 40, _docx(["a"]), _docx(["b"])) + for text in ("contact kim@example.org", "RRN 900101-1234567", "call 010-1234-5678"): + with pytest.raises(dbd.DocumentSafetyError, match="participant or secret"): + dbd.build_envelope("o/r", "a.docx", "1" * 40, "2" * 40, _docx(["ok"]), _docx([text])) + with pytest.raises(dbd.DocumentSafetyError, match="participant or secret"): + dbd.build_envelope("o/r", "a.docx", "1" * 40, "2" * 40, _docx(["ok"]), _docx(["token"]), sensitive=lambda t: "token" in t) + monkeypatch.setattr(dbd, "MAX_OBJECTS", 0) + with pytest.raises(dbd.DocumentSafetyError, match="exceeds review bounds"): + dbd.build_envelope("o/r", "a.docx", "1" * 40, "2" * 40, _docx(["a"]), _docx(["b"])) + + +# ---------------------------------------------------------------- stanzas + + +BINARY_ONLY_DIFF = ( + "diff --git a/paper.docx b/paper.docx\n" + "index 1111111..2222222 100644\n" + "Binary files a/paper.docx and b/paper.docx differ\n" + "diff --git a/logo.bin b/logo.bin\n" + "Binary files a/logo.bin and b/logo.bin differ\n" + "diff --git a/new.pdf b/new.pdf\n" + "new file mode 100644\n" + "Binary files /dev/null and b/new.pdf differ\n" +) + + +def test_binary_stanzas_are_found_and_replaced_per_document() -> None: + """Only review-document binaries are selected; others keep their stanza.""" + assert dbd.binary_document_stanzas(BINARY_ONLY_DIFF + BINARY_ONLY_DIFF) == [ + ("paper.docx", "paper.docx"), + (None, "new.pdf"), + ] + replaced = dbd.replace_binary_stanzas(BINARY_ONLY_DIFF, {("paper.docx", "paper.docx"): "HUNK"}) + assert replaced.startswith("HUNK\ndiff --git a/logo.bin") + assert "Binary files /dev/null and b/new.pdf differ" in replaced + + +# ---------------------------------------------------------------- gate + + +def test_binary_only_textual_diff_alone_has_no_citable_line() -> None: + """RED characterization: the raw binary-only diff cannot carry a formal verdict.""" + verdict = {"decision": "approve", "reviewed_lines": [], "findings": []} + with pytest.raises(RuntimeError, match="requires parseable changed-line evidence"): + gate.validate_substantive_verdict(verdict, BINARY_ONLY_DIFF) + + +def _fake_github(monkeypatch: pytest.MonkeyPatch, blobs: dict[tuple[str, str], bytes]) -> None: + """Serve contents/blobs/compare calls from ``blobs`` keyed by (path, ref).""" + shas = {key: f"{index:040x}" for index, key in enumerate(blobs, start=1)} + + def fake_run(args, *, stdin=None): + """Return canned GitHub API output for the materialization calls.""" + url = args[2] + if "/compare/" in url: + return "b" * 40 + if "/contents/" in url: + path, ref = url.split("/contents/", 1)[1].split("?ref=") + return shas[(path, ref)] + sha = url.rsplit("/", 1)[1] + key = next(k for k, v in shas.items() if v == sha) + return base64.b64encode(blobs[key]).decode() + + monkeypatch.setattr(gate, "run", fake_run) + + +def test_binary_only_docx_pr_yields_a_citable_request_changes_finding(monkeypatch: pytest.MonkeyPatch) -> None: + """GREEN: a binary-only PR produces object hunks that a real finding can cite.""" + head = "c" * 40 + _fake_github( + monkeypatch, + { + ("paper.docx", "b" * 40): _docx(["Intro", "N = 957"]), + ("paper.docx", head): _docx(["Intro", "N = 1,020"]), + ("new.pdf", head): b"%PDF-1.7", + }, + ) + pr = {"baseRefOid": "a" * 40, "headRefOid": head} + diff, truncated = gate.augment_binary_document_diff("o/r", pr, BINARY_ONLY_DIFF, False) + assert not truncated + assert "Binary files a/paper.docx" not in diff and "Binary files a/logo.bin" in diff + verdict = { + "decision": "request_changes", + "reviewed_lines": [{"path": "paper.docx", "line": 2, "side": "RIGHT"}], + "findings": [{"path": "paper.docx", "line": 2, "side": "RIGHT", "message": "Sample size contradicts Table 1."}], + } + locations = gate.changed_diff_locations(diff) + assert ("paper.docx", 2, "RIGHT") in locations and ("new.pdf", 1, "RIGHT") in locations + for finding in verdict["findings"]: + assert (finding["path"], finding["line"], finding["side"]) in locations + + +def test_augmentation_passes_through_text_diffs_and_fails_closed_on_unsafe_blobs(monkeypatch: pytest.MonkeyPatch) -> None: + """No stanza means no API call; a safety rejection fails the review closed.""" + monkeypatch.setattr(gate, "run", lambda *_a, **_k: pytest.fail("no API call expected")) + assert gate.augment_binary_document_diff("o/r", {}, "diff --git a/x b/x\n", True) == ("diff --git a/x b/x\n", True) + head = "c" * 40 + _fake_github( + monkeypatch, + {("paper.docx", "b" * 40): _docx(["ok"]), ("paper.docx", head): _zip({"word/vbaProject.bin": "x"}), ("new.pdf", head): b"%PDF"}, + ) + with pytest.raises(RuntimeError, match="binary document review failed closed for paper.docx: package contains macro"): + gate.augment_binary_document_diff("o/r", {"baseRefOid": "a" * 40, "headRefOid": head}, BINARY_ONLY_DIFF, False) + + +def test_blob_fetch_fails_closed_on_missing_sha_or_bad_base64(monkeypatch: pytest.MonkeyPatch) -> None: + """A missing blob SHA or malformed payload raises.""" + monkeypatch.setattr(gate, "run", lambda args, **_k: "" if "/contents/" in args[2] else "!") + with pytest.raises(RuntimeError, match="did not return a blob SHA"): + gate.fetch_file_blob_at_ref("o/r", "a b.pdf", "main") + monkeypatch.setattr(gate, "run", lambda args, **_k: "c" * 40 if "/contents/" in args[2] else "!!notbase64") + with pytest.raises(RuntimeError, match="malformed base64"): + gate.fetch_file_blob_at_ref("o/r", "a.pdf", "main") + + +def test_opaque_head_content_is_never_decoded_into_the_prompt(monkeypatch: pytest.MonkeyPatch) -> None: + """PDF/image bytes no longer reach the changed-file context as mojibake.""" + monkeypatch.setattr(gate, "run", lambda *_a, **_k: base64.b64encode(b"%PDF secret-bytes").decode()) + text = gate.fetch_file_content_at_ref("o/r", "scan.pdf", "h" * 40) + assert "secret-bytes" not in text and "object-level diff" in text From c6f4b49bd35ad74658ad47ac1e9ee577913bc439 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 22 Sep 2026 01:42:49 +0900 Subject: [PATCH 2/5] ci(noema): bound document text in UTF-8 bytes and scan every object Align the envelope with contextual-orchestrator#1220: cut object text on UTF-8 byte boundaries (8 KiB) and count the 256 KiB total in bytes, keeping changed objects before unchanged context. Scan every extracted base/head object for participant or secret patterns, and apply the same gate to the full extracted document body in the changed-file context. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LeLbwEgYXzCzpaFMTHDcBS --- CHANGELOG.md | 5 +- scripts/ci/document_blob_diff.py | 66 +++++++++++++++++------ scripts/ci/noema_review_gate.py | 11 +++- tests/test_document_blob_diff.py | 89 +++++++++++++++++++++++++++++++- 4 files changed, 151 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9242480d6..eaa8ca7901 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,10 @@ - New `scripts/ci/document_blob_diff.py` (stdlib only) materializes the base blob at the merge base and the head blob (contents API for the blob SHA, then the Git blobs API for the bytes) and extracts ordered objects. DOCX yields body paragraphs, tables and relationship-resolved figures; HWPX yields manifest/section paragraphs, nested tables and `binaryItemIDRef` figures; PDF and images are one opaque hashed `page` object. Every object carries a sha256. The objects are aligned into added, removed, modified and neighbouring unchanged objects, and a byte change with no object change becomes a package-level `style` object, never "no change". The result is emitted as `document_diff_review.v1` (contextual-orchestrator#1220 contract; all five fixture envelopes pass that PR's validator). - Fail-closed safety: blob size, member count, total expansion, per-member compression ratio, traversal, encrypted members, macros (`vbaProject.bin`, `Scripts/`, `macroEnabled`), external image/OLE/template/frame relationships, external HWPX manifest hrefs, DTD/entity XML, unresolved figures, participant-material directories, and email, resident-registration-number, mobile-number and secret patterns in extracted text all reject the review instead of redacting. Original bytes and media never leave the runner; figures and pages carry only hashes. - `noema_review_gate.augment_binary_document_diff()`, called from both the direct and the two-phase paths, replaces each binary document stanza with synthetic hunks whose LEFT/RIGHT line numbers are base/head object ordinals. The existing citation validator therefore accepts findings on document objects unchanged. Opaque binaries are no longer decoded into the changed-file context. -- Tests: `tests/test_document_blob_diff.py` (37). The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. +- Review fixes on the same PR (contextual-orchestrator lead findings, reproduced with that lead's fixtures): + - Object text is now cut on UTF-8 byte boundaries (8 KiB per text) and the envelope total is counted in bytes (256 KiB). This matches contextual-orchestrator#1220, which had rejected a 5,000-character Korean paragraph (15,000 B) with 400 and a >256 KiB Korean envelope with 413. Changed objects claim the byte budget first, and unchanged context is dropped once bytes or object slots run out. XML-legal control characters (CR) are blanked. + - The participant/secret scan now covers every extracted base and head object, not only the diffed ones. The changed-file context path applies the same scan to the whole `extract_review_document` body of a DOCX/HWP/HWPX and withholds it on a match, so a phone number in an unchanged paragraph far from the edit no longer reaches the prompt. +- Tests: `tests/test_document_blob_diff.py` (43; 6 byte-bound and full-body privacy regressions fail on bddeb901). The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. ### Noema transport capacity schedules a bounded continuation re-dispatch diff --git a/scripts/ci/document_blob_diff.py b/scripts/ci/document_blob_diff.py index 03f173be17..801ad2e44c 100644 --- a/scripts/ci/document_blob_diff.py +++ b/scripts/ci/document_blob_diff.py @@ -36,8 +36,10 @@ MAX_MEMBER_RATIO = 200 RATIO_FLOOR_BYTES = 1024 * 1024 MAX_OBJECTS = 200 -MAX_OBJECT_TEXT = 8 * 1024 -MAX_ENVELOPE_TEXT = 256 * 1024 +# UTF-8 byte bounds, matching contextual-orchestrator document_diff_review.v1. +MAX_OBJECT_TEXT_BYTES = 8 * 1024 +MAX_ENVELOPE_TEXT_BYTES = 256 * 1024 +_DISALLOWED_CONTROL = re.compile(r"[\x00-\x08\x0b-\x1f\x7f\ud800-\udfff]") # Relationship types that make a reader fetch or execute remote content. A # plain hyperlink is metadata and stays allowed. _REMOTE_CONTENT_REL = re.compile(r"/(image|oleObject|attachedTemplate|frame|subDocument|package)$") @@ -306,13 +308,23 @@ def diff_objects( return changes -def _check_text(text: str | None, locator: str, sensitive: Callable[[str], bool]) -> str | None: - """Bound one object's text and reject participant or secret patterns.""" +def assert_no_participant_material(text: str, where: str, sensitive: Callable[[str], bool]) -> None: + """Reject text carrying participant identifiers or secrets; never redact.""" + if any(p.search(text) for p in _PARTICIPANT_PATTERNS) or sensitive(text): + raise DocumentSafetyError(f"participant or secret pattern in extracted text at {where}") + + +def _bounded(text: str | None, budget: int) -> str | None: + """Return ``text`` cut on a UTF-8 boundary to ``budget`` bytes, controls blanked.""" if text is None: return None - if any(p.search(text) for p in _PARTICIPANT_PATTERNS) or sensitive(text): - raise DocumentSafetyError(f"participant or secret pattern in extracted text at {locator}") - return text[:MAX_OBJECT_TEXT] + clean = _DISALLOWED_CONTROL.sub(" ", text) + return clean.encode("utf-8")[:budget].decode("utf-8", "ignore") + + +def _size(text: str | None) -> int: + """Return the UTF-8 byte length of optional text.""" + return len(text.encode("utf-8")) if text else 0 @dataclass(frozen=True) @@ -346,14 +358,38 @@ def build_envelope( raise DocumentSafetyError("base and head blobs are identical") base = extract_objects(path, base_raw) if base_raw is not None else [] head = extract_objects(path, head_raw) if head_raw is not None else [] + # Scan every extracted object, not only the diffed ones: the same document + # text also reaches the changed-file context, so a participant identifier + # anywhere in either blob fails the review closed. + for obj in (*base, *head): + if obj.text is not None: + assert_no_participant_material(obj.text, obj.locator, sensitive) + entries = diff_objects(base, head) + changed = [e for e in entries if e[0] != "unchanged"] + context = [e for e in entries if e[0] == "unchanged"] + if len(changed) > MAX_OBJECTS: + raise DocumentSafetyError(f"document diff has more than {MAX_OBJECTS} changed objects") + # Changed objects claim the byte budget first; unchanged context is kept + # only while objects and bytes remain, so the envelope always fits. + budget = MAX_ENVELOPE_TEXT_BYTES + kept: dict[int, tuple[str | None, str | None]] = {} + for entry in changed + context[: MAX_OBJECTS - len(changed)]: + _change, _bo, old, _ho, new = entry + if entry[0] == "unchanged" and budget <= 0: + break + base_text = _bounded(old.text if old else None, min(MAX_OBJECT_TEXT_BYTES, max(budget, 0))) + budget -= _size(base_text) + head_text = _bounded(new.text if new else None, min(MAX_OBJECT_TEXT_BYTES, max(budget, 0))) + budget -= _size(head_text) + kept[id(entry)] = (base_text, head_text) objects: list[dict[str, Any]] = [] ordinals: list[tuple[int | None, int | None]] = [] - total = 0 - for change, base_ord, old, head_ord, new in diff_objects(base, head): + for entry in entries: + if id(entry) not in kept: + continue + change, base_ord, old, head_ord, new = entry subject = new or old - base_text = _check_text(old.text if old else None, subject.locator, sensitive) - head_text = _check_text(new.text if new else None, subject.locator, sensitive) - total += len(base_text or "") + len(head_text or "") + base_text, head_text = kept[id(entry)] objects.append( { "page": None, @@ -367,7 +403,7 @@ def build_envelope( } ) ordinals.append((base_ord, head_ord)) - if not any(o["change"] != "unchanged" for o in objects) and base_raw is not None and head_raw is not None: + if not changed and base_raw is not None and head_raw is not None: # The bytes changed but no body object did (styles, settings, metadata): # still a reviewable change, never "no change". objects.append( @@ -383,10 +419,6 @@ def build_envelope( } ) ordinals.append((1, 1)) - if len(objects) > MAX_OBJECTS or total > MAX_ENVELOPE_TEXT: - raise DocumentSafetyError( - f"document diff exceeds review bounds ({len(objects)} objects, {total} text bytes)" - ) envelope = { "contract_version": CONTRACT_VERSION, "repo": repo, diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index e12abc6026..1cabd53f6e 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -873,9 +873,18 @@ def fetch_file_content_at_ref(repo: str, path: str, ref: str) -> str: return "[binary document: reviewed through the object-level diff; raw bytes are not sent]" if suffix in {".docx", ".hwp", ".hwpx"}: try: - return extract_review_document(path, raw) + text = extract_review_document(path, raw) except DocumentReadError as exc: raise RuntimeError(f"document extraction failed: {exc}") from exc + # The whole extracted body would reach the prompt: apply the same + # participant/secret gate as the object diff and never send on a match. + try: + document_blob_diff.assert_no_participant_material( + text, path, lambda value: scrub_sensitive_data(value) != value + ) + except document_blob_diff.DocumentSafetyError as exc: + raise RuntimeError(f"document context withheld: {exc}") from exc + return text return raw.decode("utf-8", errors="replace") diff --git a/tests/test_document_blob_diff.py b/tests/test_document_blob_diff.py index 2b078ea1e1..99be6de637 100644 --- a/tests/test_document_blob_diff.py +++ b/tests/test_document_blob_diff.py @@ -288,7 +288,7 @@ def test_envelope_rejects_identical_or_missing_blobs_participant_material_secret with pytest.raises(dbd.DocumentSafetyError, match="participant or secret"): dbd.build_envelope("o/r", "a.docx", "1" * 40, "2" * 40, _docx(["ok"]), _docx(["token"]), sensitive=lambda t: "token" in t) monkeypatch.setattr(dbd, "MAX_OBJECTS", 0) - with pytest.raises(dbd.DocumentSafetyError, match="exceeds review bounds"): + with pytest.raises(dbd.DocumentSafetyError, match="more than 0 changed objects"): dbd.build_envelope("o/r", "a.docx", "1" * 40, "2" * 40, _docx(["a"]), _docx(["b"])) @@ -401,3 +401,90 @@ def test_opaque_head_content_is_never_decoded_into_the_prompt(monkeypatch: pytes monkeypatch.setattr(gate, "run", lambda *_a, **_k: base64.b64encode(b"%PDF secret-bytes").decode()) text = gate.fetch_file_content_at_ref("o/r", "scan.pdf", "h" * 40) assert "secret-bytes" not in text and "object-level diff" in text + + +# ---------------------------------------------------------------- CO byte bounds and full-body privacy + + +def _texts(envelope: dict) -> list[str]: + """All non-null base/head texts in an envelope.""" + return [t for o in envelope["objects"] for t in (o["base_text"], o["head_text"]) if t is not None] + + +def test_korean_object_text_is_cut_on_utf8_bytes_not_characters() -> None: + """A 5,000-character Korean paragraph (15,000 UTF-8 bytes) fits the 8 KiB byte bound. + + RED on bddeb901: text was cut at 8,192 characters, so the envelope kept all + 15,000 bytes and contextual-orchestrator#1220 answered 400 invalid_text. + """ + long_ko = "가" * 5000 + review = dbd.build_envelope("o/r", "k.docx", "1" * 40, "2" * 40, _docx(["짧음"]), _docx([long_ko])) + head_text = review.envelope["objects"][0]["head_text"] + assert len(head_text.encode("utf-8")) <= dbd.MAX_OBJECT_TEXT_BYTES == 8 * 1024 + assert head_text == "가" * (8 * 1024 // 3) + + +def test_envelope_total_stays_within_256_kib_utf8_bytes() -> None: + """Forty ~8.1 KB Korean paragraphs exceed 256 KiB in bytes, not in characters. + + RED on bddeb901: the total counted characters, so the envelope carried more + than 256 KiB and contextual-orchestrator#1220 answered 413 request_too_large. + """ + base = _docx([f"{i}" + "나" * 2700 for i in range(40)]) + head = _docx([f"{i}" + "다" * 2700 for i in range(40)]) + review = dbd.build_envelope("o/r", "k.docx", "1" * 40, "2" * 40, base, head) + total = sum(len(t.encode("utf-8")) for t in _texts(review.envelope)) + assert total <= dbd.MAX_ENVELOPE_TEXT_BYTES + assert len(review.envelope["objects"]) == 40 + assert all(o["change"] == "modified" for o in review.envelope["objects"]) + + +def test_unchanged_context_is_dropped_before_changed_objects_lose_budget(monkeypatch: pytest.MonkeyPatch) -> None: + """Changed objects claim the budget first; context goes when bytes or slots run out.""" + base = _docx(["ctx-a", "old", "ctx-b"]) + head = _docx(["ctx-a", "new", "ctx-b"]) + monkeypatch.setattr(dbd, "MAX_ENVELOPE_TEXT_BYTES", 6) + review = dbd.build_envelope("o/r", "c.docx", "1" * 40, "2" * 40, base, head) + assert [(o["change"], o["base_text"], o["head_text"]) for o in review.envelope["objects"]] == [ + ("modified", "old", "new"), + ] + monkeypatch.setattr(dbd, "MAX_ENVELOPE_TEXT_BYTES", 1024) + monkeypatch.setattr(dbd, "MAX_OBJECTS", 2) + review = dbd.build_envelope("o/r", "c.docx", "1" * 40, "2" * 40, base, head) + assert [o["change"] for o in review.envelope["objects"]] == ["unchanged", "modified"] + + +def test_control_characters_are_blanked_to_match_the_co_text_rule() -> None: + """contextual-orchestrator rejects control characters other than newline and tab. + + XML 1.0 already refuses most of them; a `` `` reference still yields a + carriage return, which is blanked instead of sent. + """ + review = dbd.build_envelope( + "o/r", "c.docx", "1" * 40, "2" * 40, _docx(["a"]), _docx(["x y\tz"]) + ) + assert review.envelope["objects"][0]["head_text"] == "x y\tz" + + +def test_participant_identifier_far_from_the_change_fails_the_review_closed() -> None: + """A phone number in an unchanged paragraph far from the edit is still rejected. + + RED on bddeb901: only diffed objects and their neighbours were scanned, so + the phone number was not in the envelope and passed, while the full body + still reached the changed-file context. + """ + body = ["title", "010-2345-6789 연락처", "filler 1", "filler 2", "filler 3", "result old"] + edited = body[:-1] + ["result new"] + with pytest.raises(dbd.DocumentSafetyError, match="participant or secret pattern in extracted text at p2"): + dbd.build_envelope("o/r", "far.docx", "1" * 40, "2" * 40, _docx(body), _docx(edited)) + + +def test_full_document_context_is_withheld_when_it_carries_participant_material(monkeypatch: pytest.MonkeyPatch) -> None: + """The changed-file context path applies the same gate to the whole extracted body.""" + raw = _docx(["intro", "call 010-2345-6789"]) + monkeypatch.setattr(gate, "run", lambda *_a, **_k: base64.b64encode(raw).decode()) + monkeypatch.setattr(gate, "extract_review_document", lambda _path, _raw: "intro\ncall 010-2345-6789") + with pytest.raises(RuntimeError, match="document context withheld: participant or secret pattern"): + gate.fetch_file_content_at_ref("o/r", "far.docx", "c" * 40) + monkeypatch.setattr(gate, "extract_review_document", lambda _path, _raw: "intro only") + assert gate.fetch_file_content_at_ref("o/r", "ok.docx", "c" * 40) == "intro only" From e6f0671f6158b48d853b483b00c198051f3ee821 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 22 Sep 2026 01:59:06 +0900 Subject: [PATCH 3/5] ci(noema): fail closed on cut changed text; captions, spine, author email Never cut or drop changed object text: if the 8 KiB object bound or the 256 KiB envelope budget would, fail closed before any provider call and trim only unchanged context (marked). Attach figure captions, order HWPX sections by the manifest spine, and allow only declared, allowlisted corresponding-author emails in front matter, masked as [CORRESPONDING_AUTHOR_EMAIL]. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LeLbwEgYXzCzpaFMTHDcBS --- .github/workflows/noema-review.yml | 4 + CHANGELOG.md | 7 +- scripts/ci/document_blob_diff.py | 139 ++++++++++++++++++++++---- scripts/ci/noema_review_gate.py | 15 ++- tests/test_document_blob_diff.py | 154 +++++++++++++++++++++++++---- 5 files changed, 282 insertions(+), 37 deletions(-) diff --git a/.github/workflows/noema-review.yml b/.github/workflows/noema-review.yml index 9be705a50c..663741cabf 100644 --- a/.github/workflows/noema-review.yml +++ b/.github/workflows/noema-review.yml @@ -764,6 +764,10 @@ jobs: NOEMA_REVIEW_ACTOR: ${{ steps.noema_github_app_token.outputs['app-slug'] && format('{0}[bot]', steps.noema_github_app_token.outputs['app-slug']) || '' }} NOEMA_REVIEW_INSTALLATION_ID: ${{ steps.noema_github_app_token.outputs['installation-id'] }} NOEMA_TRANSPORT_RETRY_ATTEMPT: ${{ github.event.client_payload.transport_retry_attempt || 0 }} + # Exact allowlist of declared corresponding-author emails that a + # manuscript's front matter may carry; they reach the model only as + # [CORRESPONDING_AUTHOR_EMAIL]. Empty means every email fails closed. + NOEMA_CORRESPONDING_AUTHOR_EMAILS: ${{ vars.NOEMA_CORRESPONDING_AUTHOR_EMAILS || '' }} run: | set -euo pipefail if [ -z "${PR_NUMBER:-}" ]; then diff --git a/CHANGELOG.md b/CHANGELOG.md index eaa8ca7901..aa05bc4035 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,12 @@ - Review fixes on the same PR (contextual-orchestrator lead findings, reproduced with that lead's fixtures): - Object text is now cut on UTF-8 byte boundaries (8 KiB per text) and the envelope total is counted in bytes (256 KiB). This matches contextual-orchestrator#1220, which had rejected a 5,000-character Korean paragraph (15,000 B) with 400 and a >256 KiB Korean envelope with 413. Changed objects claim the byte budget first, and unchanged context is dropped once bytes or object slots run out. XML-legal control characters (CR) are blanked. - The participant/secret scan now covers every extracted base and head object, not only the diffed ones. The changed-file context path applies the same scan to the whole `extract_review_document` body of a DOCX/HWP/HWPX and withholds it on a match, so a phone number in an unchanged paragraph far from the edit no longer reaches the prompt. -- Tests: `tests/test_document_blob_diff.py` (43; 6 byte-bound and full-body privacy regressions fail on bddeb901). The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. +- Second review round: + - Fail-open fix: c6f4b49b kept changed objects with `""` text once the 256 KiB budget ran out, so a late substantive change ("결론: 효과 없음") never reached the reviewer. Now if any changed object's text would be cut by the 8 KiB per-object bound or the envelope budget, the envelope fails closed and no provider is called. Only unchanged context is cut, ending with `…[truncated]`, or dropped. A regression test checks that no changed object ever carries empty or truncated text. + - Figures carry the text of an adjacent "Figure N"/"Fig."/"그림 N" caption, so contextual-orchestrator's rule finding (changed image, unchanged caption) fires. + - HWPX sections follow the manifest spine; a spine section missing from the package fails closed. + - Emails still fail closed by default, with one narrow exception for a declared corresponding author. An address is replaced by `[CORRESPONDING_AUTHOR_EMAIL]` before hashing, diffing, context or prompt only when it is on the workflow's exact `NOEMA_CORRESPONDING_AUTHOR_EMAILS` allowlist and sits in a front-matter author-contact paragraph (before the abstract or introduction, within the first 20 paragraphs, declaring "Corresponding author"/"Correspondence"/"교신저자"). Allowlist mismatches, the same address elsewhere, emails after the abstract, undeclared contacts, extra addresses, table cells and participant phone numbers are all still rejected, and no error message contains the address. +- Tests: `tests/test_document_blob_diff.py` (58). The budget fail-open, empty-changed-text invariant, caption, spine and corresponding-author cases fail on c6f4b49b. The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. ### Noema transport capacity schedules a bounded continuation re-dispatch diff --git a/scripts/ci/document_blob_diff.py b/scripts/ci/document_blob_diff.py index 801ad2e44c..a907cced52 100644 --- a/scripts/ci/document_blob_diff.py +++ b/scripts/ci/document_blob_diff.py @@ -221,16 +221,30 @@ def extract_hwpx(raw: bytes) -> list[DocumentObject]: if manifest is None: raise DocumentSafetyError("HWPX has no Contents/content.hpf") items: dict[str, str] = {} + spine: list[str] = [] for item in _parse(manifest, "content.hpf").iter(): if _local(item.tag) == "item": href = item.get("href", "") if re.match(r"^[a-z][a-z0-9+.-]*:", href, re.IGNORECASE): raise DocumentSafetyError(f"HWPX manifest references external content: {href!r}") items[item.get("id", "")] = href - sections = sorted( - (n for n in package.namelist() if re.fullmatch(r"Contents/section\d+\.xml", n)), - key=lambda n: int(re.search(r"\d+", n).group()), - ) + elif _local(item.tag) == "itemref": + spine.append(item.get("idref", "")) + if spine: + # Reading order is the manifest spine, as the HWPX reader uses it. + sections = [] + for idref in spine: + href = items.get(idref, "") + if not re.fullmatch(r"Contents/section\d+\.xml", href): + continue + if href not in package.namelist(): + raise DocumentSafetyError(f"HWPX spine section is missing: {href!r}") + sections.append(href) + else: + sections = sorted( + (n for n in package.namelist() if re.fullmatch(r"Contents/section\d+\.xml", n)), + key=lambda n: int(re.search(r"\d+", n).group()), + ) if not sections: raise DocumentSafetyError("HWPX has no Contents/section*.xml") objects: list[DocumentObject] = [] @@ -269,6 +283,70 @@ def extract_objects(path: str, raw: bytes) -> list[DocumentObject]: raise DocumentSafetyError(f"unsupported review document type: {suffix or path!r}") +CORRESPONDING_AUTHOR_MARKER = "[CORRESPONDING_AUTHOR_EMAIL]" +FRONT_MATTER_MAX_PARAGRAPHS = 20 +_EMAIL = _PARTICIPANT_PATTERNS[0] +_CONTACT_DECLARATION = re.compile(r"corresponding\s+author|correspondence|교신\s*저자", re.IGNORECASE) +_FRONT_MATTER_END = re.compile(r"^\s*(abstract|초록|요약|introduction|서론|1\.\s)", re.IGNORECASE) +_CAPTION = re.compile(r"^\s*(figure|fig\.|그림)\s*\d+", re.IGNORECASE) + + +def redact_corresponding_author(paragraphs: Sequence[str], allowlist: frozenset[str]) -> list[str]: + """Replace declared, allowlisted corresponding-author emails with a marker. + + An address is replaced only when it is on the workflow's exact allowlist + AND sits in a front-matter paragraph (before the abstract/introduction and + within the first ``FRONT_MATTER_MAX_PARAGRAPHS``) that declares author + contact. Every other email, including the same address anywhere else, + is left in place so the participant scan rejects the document. + """ + out = [] + front = True + for index, text in enumerate(paragraphs): + if front and (index >= FRONT_MATTER_MAX_PARAGRAPHS or _FRONT_MATTER_END.match(text)): + front = False + emails = _EMAIL.findall(text) + if ( + emails + and front + and _CONTACT_DECLARATION.search(text) + and all(email.lower() in allowlist for email in emails) + ): + text = _EMAIL.sub(CORRESPONDING_AUTHOR_MARKER, text) + out.append(text) + return out + + +def _apply_contact_policy(objects: Sequence[DocumentObject], allowlist: frozenset[str]) -> list[DocumentObject]: + """Apply :func:`redact_corresponding_author` to paragraph objects, rehashing them.""" + paragraphs = [o for o in objects if o.kind == "paragraph"] + redacted = iter(redact_corresponding_author([o.text or "" for o in paragraphs], allowlist)) + out = [] + for obj in objects: + if obj.kind == "paragraph": + text = next(redacted) + obj = obj if text == obj.text else _text_object("paragraph", obj.locator, text) + out.append(obj) + return out + + +def _attach_captions(objects: Sequence[DocumentObject]) -> list[DocumentObject]: + """Give each figure the text of an adjacent "Figure N"/"그림 N" caption paragraph. + + The figure keeps its media hash, so a changed image with an unchanged + caption is visible as a modified figure whose base and head text agree. + """ + out = list(objects) + for index, obj in enumerate(out): + if obj.kind != "figure": + continue + for near in (index + 1, index - 1): + if 0 <= near < len(out) and out[near].kind == "paragraph" and _CAPTION.match(out[near].text or ""): + out[index] = DocumentObject("figure", obj.locator, obj.sha256, out[near].text) + break + return out + + def diff_objects( base: Sequence[DocumentObject], head: Sequence[DocumentObject] ) -> list[tuple[str, int | None, DocumentObject | None, int | None, DocumentObject | None]]: @@ -314,12 +392,23 @@ def assert_no_participant_material(text: str, where: str, sensitive: Callable[[s raise DocumentSafetyError(f"participant or secret pattern in extracted text at {where}") -def _bounded(text: str | None, budget: int) -> str | None: - """Return ``text`` cut on a UTF-8 boundary to ``budget`` bytes, controls blanked.""" +TRUNCATION_MARKER = "…[truncated]" + + +def _bounded(text: str | None) -> str | None: + """Return ``text`` within ``MAX_OBJECT_TEXT_BYTES`` UTF-8 bytes. + + Controls other than newline and tab are blanked. A cut text ends with + ``TRUNCATION_MARKER`` so a reviewer can never mistake it for the whole text. + """ if text is None: return None clean = _DISALLOWED_CONTROL.sub(" ", text) - return clean.encode("utf-8")[:budget].decode("utf-8", "ignore") + encoded = clean.encode("utf-8") + if len(encoded) <= MAX_OBJECT_TEXT_BYTES: + return clean + room = MAX_OBJECT_TEXT_BYTES - len(TRUNCATION_MARKER.encode("utf-8")) + return encoded[:room].decode("utf-8", "ignore") + TRUNCATION_MARKER def _size(text: str | None) -> int: @@ -348,6 +437,7 @@ def build_envelope( base_raw: bytes | None, head_raw: bytes | None, sensitive: Callable[[str], bool] = lambda _text: False, + corresponding_author_emails: frozenset[str] = frozenset(), ) -> DocumentReview: """Build a bounded ``document_diff_review.v1`` envelope for one path.""" if _PARTICIPANT_DIRECTORY.search(path): @@ -358,6 +448,8 @@ def build_envelope( raise DocumentSafetyError("base and head blobs are identical") base = extract_objects(path, base_raw) if base_raw is not None else [] head = extract_objects(path, head_raw) if head_raw is not None else [] + base = _attach_captions(_apply_contact_policy(base, corresponding_author_emails)) + head = _attach_captions(_apply_contact_policy(head, corresponding_author_emails)) # Scan every extracted object, not only the diffed ones: the same document # text also reaches the changed-file context, so a participant identifier # anywhere in either blob fails the review closed. @@ -369,19 +461,32 @@ def build_envelope( context = [e for e in entries if e[0] == "unchanged"] if len(changed) > MAX_OBJECTS: raise DocumentSafetyError(f"document diff has more than {MAX_OBJECTS} changed objects") - # Changed objects claim the byte budget first; unchanged context is kept - # only while objects and bytes remain, so the envelope always fits. + # Every changed object is sent in full: if any changed text would be cut by + # the per-object bound or the envelope budget, the review fails closed and + # no provider is called. Only unchanged context is cut (marked) or dropped. budget = MAX_ENVELOPE_TEXT_BYTES kept: dict[int, tuple[str | None, str | None]] = {} - for entry in changed + context[: MAX_OBJECTS - len(changed)]: + for entry in changed: _change, _bo, old, _ho, new = entry - if entry[0] == "unchanged" and budget <= 0: - break - base_text = _bounded(old.text if old else None, min(MAX_OBJECT_TEXT_BYTES, max(budget, 0))) - budget -= _size(base_text) - head_text = _bounded(new.text if new else None, min(MAX_OBJECT_TEXT_BYTES, max(budget, 0))) - budget -= _size(head_text) - kept[id(entry)] = (base_text, head_text) + texts = (_bounded(old.text if old else None), _bounded(new.text if new else None)) + if any(_size(text) > MAX_OBJECT_TEXT_BYTES for text in (old and old.text, new and new.text)): + raise DocumentSafetyError( + f"changed object {(new or old).locator} exceeds the {MAX_OBJECT_TEXT_BYTES}-byte text bound" + ) + budget -= _size(texts[0]) + _size(texts[1]) + kept[id(entry)] = texts + if budget < 0: + raise DocumentSafetyError( + f"changed document objects exceed the {MAX_ENVELOPE_TEXT_BYTES}-byte review budget" + ) + for entry in context[: MAX_OBJECTS - len(changed)]: + _change, _bo, old, _ho, new = entry + texts = (_bounded(old.text if old else None), _bounded(new.text if new else None)) + cost = _size(texts[0]) + _size(texts[1]) + if cost > budget: + continue + budget -= cost + kept[id(entry)] = texts objects: list[dict[str, Any]] = [] ordinals: list[tuple[int | None, int | None]] = [] for entry in entries: diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 1cabd53f6e..bfcc3bb6dd 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -877,7 +877,13 @@ def fetch_file_content_at_ref(repo: str, path: str, ref: str) -> str: except DocumentReadError as exc: raise RuntimeError(f"document extraction failed: {exc}") from exc # The whole extracted body would reach the prompt: apply the same - # participant/secret gate as the object diff and never send on a match. + # corresponding-author redaction and participant/secret gate as the + # object diff, and never send on a match. + text = "\n".join( + document_blob_diff.redact_corresponding_author( + text.split("\n"), corresponding_author_allowlist() + ) + ) try: document_blob_diff.assert_no_participant_material( text, path, lambda value: scrub_sensitive_data(value) != value @@ -910,6 +916,12 @@ def fetch_file_blob_at_ref(repo: str, path: str, ref: str) -> tuple[str, bytes]: return blob_sha, raw +def corresponding_author_allowlist() -> frozenset[str]: + """Return the workflow's exact corresponding-author email allowlist, lowercased.""" + raw = os.environ.get("NOEMA_CORRESPONDING_AUTHOR_EMAILS", "") + return frozenset(item.strip().lower() for item in raw.split(",") if item.strip()) + + def augment_binary_document_diff( repo: str, pr: dict[str, Any], diff: str, truncated: bool ) -> tuple[str, bool]: @@ -941,6 +953,7 @@ def augment_binary_document_diff( base_raw, head_raw, sensitive=lambda text: scrub_sensitive_data(text) != text, + corresponding_author_emails=corresponding_author_allowlist(), ) except document_blob_diff.DocumentSafetyError as exc: raise RuntimeError(f"binary document review failed closed for {path}: {exc}") from exc diff --git a/tests/test_document_blob_diff.py b/tests/test_document_blob_diff.py index 99be6de637..08f50f84bd 100644 --- a/tests/test_document_blob_diff.py +++ b/tests/test_document_blob_diff.py @@ -411,32 +411,56 @@ def _texts(envelope: dict) -> list[str]: return [t for o in envelope["objects"] for t in (o["base_text"], o["head_text"]) if t is not None] -def test_korean_object_text_is_cut_on_utf8_bytes_not_characters() -> None: - """A 5,000-character Korean paragraph (15,000 UTF-8 bytes) fits the 8 KiB byte bound. +def test_changed_korean_text_over_the_byte_bound_fails_closed_and_context_is_marked() -> None: + """Changed text is never cut: a 5,000-character Korean change (15,000 B) fails closed. - RED on bddeb901: text was cut at 8,192 characters, so the envelope kept all - 15,000 bytes and contextual-orchestrator#1220 answered 400 invalid_text. + RED on bddeb901: text was cut at 8,192 characters (15,000 bytes kept, so + contextual-orchestrator#1220 answered 400). An unchanged neighbour of the + same size is cut on a UTF-8 boundary and ends with the truncation marker. """ long_ko = "가" * 5000 - review = dbd.build_envelope("o/r", "k.docx", "1" * 40, "2" * 40, _docx(["짧음"]), _docx([long_ko])) - head_text = review.envelope["objects"][0]["head_text"] - assert len(head_text.encode("utf-8")) <= dbd.MAX_OBJECT_TEXT_BYTES == 8 * 1024 - assert head_text == "가" * (8 * 1024 // 3) + with pytest.raises(dbd.DocumentSafetyError, match="changed object p1 exceeds the 8192-byte text bound"): + dbd.build_envelope("o/r", "k.docx", "1" * 40, "2" * 40, _docx(["짧음"]), _docx([long_ko])) + review = dbd.build_envelope("o/r", "k.docx", "1" * 40, "2" * 40, _docx([long_ko, "old"]), _docx([long_ko, "new"])) + context = review.envelope["objects"][0] + assert context["change"] == "unchanged" + assert context["head_text"].endswith(dbd.TRUNCATION_MARKER) + assert len(context["head_text"].encode("utf-8")) <= dbd.MAX_OBJECT_TEXT_BYTES == 8 * 1024 -def test_envelope_total_stays_within_256_kib_utf8_bytes() -> None: - """Forty ~8.1 KB Korean paragraphs exceed 256 KiB in bytes, not in characters. +def test_changed_objects_over_the_envelope_budget_fail_closed_without_a_provider_call( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """contextual-orchestrator lead fixture: 40 changed Korean paragraphs, the last substantive. - RED on bddeb901: the total counted characters, so the envelope carried more - than 256 KiB and contextual-orchestrator#1220 answered 413 request_too_large. + RED on c6f4b49b: the budget ran out and 24 changed objects were kept with + "" text, so "결론: 효과 없음" never reached the reviewer and a verdict could + approve an unseen change. Now the envelope fails closed and the gate raises + before any model request. """ - base = _docx([f"{i}" + "나" * 2700 for i in range(40)]) - head = _docx([f"{i}" + "다" * 2700 for i in range(40)]) + base = _docx(["다" * 2700 + f"{i:03d}" for i in range(39)] + ["결론: 효과 있음"]) + head = _docx(["라" * 2700 + f"{i:03d}" for i in range(39)] + ["결론: 효과 없음"]) + with pytest.raises(dbd.DocumentSafetyError, match="changed document objects exceed the 262144-byte review budget"): + dbd.build_envelope("o/r", "k.docx", "1" * 40, "2" * 40, base, head) + c = "c" * 40 + _fake_github(monkeypatch, {("paper.docx", "b" * 40): base, ("paper.docx", c): head, ("new.pdf", c): b"%PDF"}) + with pytest.raises(RuntimeError, match="binary document review failed closed for paper.docx: changed document objects exceed"): + gate.augment_binary_document_diff("o/r", {"baseRefOid": "a" * 40, "headRefOid": c}, BINARY_ONLY_DIFF, False) + + +@pytest.mark.parametrize("count", (1, 5, 30)) +def test_no_changed_object_ever_carries_empty_text(count: int) -> None: + """Invariant: every accepted changed object carries its full source text.""" + base = _docx([f"base {i} " + "가" * 900 for i in range(count)]) + head = _docx([f"head {i} " + "나" * 900 for i in range(count)]) review = dbd.build_envelope("o/r", "k.docx", "1" * 40, "2" * 40, base, head) - total = sum(len(t.encode("utf-8")) for t in _texts(review.envelope)) - assert total <= dbd.MAX_ENVELOPE_TEXT_BYTES - assert len(review.envelope["objects"]) == 40 - assert all(o["change"] == "modified" for o in review.envelope["objects"]) + changed = [o for o in review.envelope["objects"] if o["change"] != "unchanged"] + assert len(changed) == count + for obj in changed: + for side in ("base", "head"): + text = obj[f"{side}_text"] + assert text and not text.endswith(dbd.TRUNCATION_MARKER) + assert obj[f"object_hash_{side}"] == "sha256:" + dbd._sha256(text.encode("utf-8")) def test_unchanged_context_is_dropped_before_changed_objects_lose_budget(monkeypatch: pytest.MonkeyPatch) -> None: @@ -488,3 +512,97 @@ def test_full_document_context_is_withheld_when_it_carries_participant_material( gate.fetch_file_content_at_ref("o/r", "far.docx", "c" * 40) monkeypatch.setattr(gate, "extract_review_document", lambda _path, _raw: "intro only") assert gate.fetch_file_content_at_ref("o/r", "ok.docx", "c" * 40) == "intro only" + + +# ---------------------------------------------------------------- captions, spine, corresponding author + + +def test_figure_objects_carry_their_caption_so_a_silent_image_swap_is_visible() -> None: + """A changed image under an unchanged "Figure N" caption keeps equal caption text.""" + review = dbd.build_envelope( + "o/r", "f.docx", "1" * 40, "2" * 40, + _docx(["Figure 1. Flow of participants"], image=b"IMG-A"), + _docx(["Figure 1. Flow of participants"], image=b"IMG-B"), + ) + figure = next(o for o in review.envelope["objects"] if o["object_kind"] == "figure") + assert figure["change"] == "modified" + assert figure["base_text"] == figure["head_text"] == "Figure 1. Flow of participants" + objects = dbd._attach_captions( + [dbd.DocumentObject("figure", "f", "h", None), dbd.DocumentObject("paragraph", "p", "t", "그림 2 결과")] + ) + assert objects[0].text == "그림 2 결과" + lone = dbd._attach_captions([dbd.DocumentObject("paragraph", "p", "t", "not a caption"), dbd.DocumentObject("figure", "f", "h", None)]) + assert lone[1].text is None + + +def test_hwpx_sections_follow_the_manifest_spine() -> None: + """Reading order is the spine, not the file-name number; missing spine sections fail closed.""" + manifest = ( + '' + '' + '' + '' + ) + raw = _hwpx([_hp("zero"), _hp("one")], manifest=manifest) + assert [o.text for o in dbd.extract_objects("s.hwpx", raw)] == ["one", "zero"] + broken = manifest.replace('href="Contents/section1.xml"', 'href="Contents/section9.xml"') + with pytest.raises(dbd.DocumentSafetyError, match="spine section is missing"): + dbd.extract_objects("s.hwpx", _hwpx([_hp("zero"), _hp("one")], manifest=broken)) + + +AUTHOR = "lead.author@univ.example.ac.kr" +MANUSCRIPT = [ + "Late-life anxiety reanalysis", + f"Corresponding author: Kim, Dept. of Psychology ({AUTHOR})", + "Abstract", + "Results changed here.", +] + + +def test_declared_allowlisted_corresponding_author_email_is_masked_everywhere(monkeypatch: pytest.MonkeyPatch) -> None: + """A normal manuscript passes; the real address never reaches envelope, hunks or errors.""" + allow = frozenset({AUTHOR}) + edited = MANUSCRIPT[:-1] + ["Results changed there."] + review = dbd.build_envelope("o/r", "m.docx", "1" * 40, "2" * 40, _docx(MANUSCRIPT), _docx(edited), corresponding_author_emails=allow) + serialized = str(review.envelope) + dbd.synthetic_hunks(review) + assert AUTHOR not in serialized + title_change = MANUSCRIPT[:1] + [MANUSCRIPT[1] + " revised"] + MANUSCRIPT[2:] + review = dbd.build_envelope("o/r", "m.docx", "1" * 40, "2" * 40, _docx(MANUSCRIPT), _docx(title_change), corresponding_author_emails=allow) + masked = [o["head_text"] for o in review.envelope["objects"] if o["change"] == "modified"][0] + assert dbd.CORRESPONDING_AUTHOR_MARKER in masked and AUTHOR not in masked + # The same marker reaches the whole-body context path through the workflow allowlist. + monkeypatch.setenv("NOEMA_CORRESPONDING_AUTHOR_EMAILS", f" {AUTHOR.upper()} , other@x.org") + monkeypatch.setattr(gate, "run", lambda *_a, **_k: base64.b64encode(_docx(MANUSCRIPT)).decode()) + monkeypatch.setattr(gate, "extract_review_document", lambda _p, _r: "\n".join(MANUSCRIPT)) + context = gate.fetch_file_content_at_ref("o/r", "m.docx", "c" * 40) + assert AUTHOR not in context and dbd.CORRESPONDING_AUTHOR_MARKER in context + + +@pytest.mark.parametrize( + ("paragraphs", "allow"), + ( + (MANUSCRIPT, frozenset()), + (MANUSCRIPT, frozenset({"someone.else@univ.example.ac.kr"})), + (MANUSCRIPT + [f"Please write to {AUTHOR} for data."], frozenset({AUTHOR})), + (["Title", "Abstract", f"Corresponding author: {AUTHOR}"], frozenset({AUTHOR})), + (["Title", f"Contact the lab at {AUTHOR}"], frozenset({AUTHOR})), + (["Title", f"Corresponding author: {AUTHOR}; co-author b@univ.example.ac.kr"], frozenset({AUTHOR})), + ([f"p{i}" for i in range(20)] + [f"Corresponding author: {AUTHOR}"], frozenset({AUTHOR})), + (["Title", f"Corresponding author: {AUTHOR}, tel 010-2345-6789"], frozenset({AUTHOR})), + ), +) +def test_every_other_email_placement_still_fails_closed_without_leaking_it(paragraphs: list[str], allow: frozenset[str]) -> None: + """Allowlist mismatch, body reuse, post-abstract, undeclared, extra address, late position, phone.""" + with pytest.raises(dbd.DocumentSafetyError) as raised: + dbd.build_envelope("o/r", "m.docx", "1" * 40, "2" * 40, _docx(["x"]), _docx(paragraphs), corresponding_author_emails=allow) + assert AUTHOR not in str(raised.value) + + +def test_email_inside_a_table_is_never_excepted() -> None: + """Tables are not front-matter author-contact paragraphs.""" + with pytest.raises(dbd.DocumentSafetyError, match="participant or secret"): + dbd.build_envelope( + "o/r", "m.docx", "1" * 40, "2" * 40, _docx(["x"]), + _docx(["Title"], table=[["Corresponding author", AUTHOR]]), + corresponding_author_emails=frozenset({AUTHOR}), + ) From 8da729e419b2e5f55cf6035f3f02a1fdd621182a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 22 Sep 2026 02:16:02 +0900 Subject: [PATCH 4/5] ci(noema): refuse approval on hash-only document objects A PDF/image or uncaptioned figure object only proves bytes changed, so an approve citing it is rejected and the prompt asks for comment or request_changes. Also give added/removed packages without extractable objects a citable package object, and re-read a truncated diff before searching for binary stanzas. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LeLbwEgYXzCzpaFMTHDcBS --- .github/actions/noema-review/two_phase.py | 2 +- CHANGELOG.md | 7 ++- scripts/ci/document_blob_diff.py | 36 ++++++++++--- scripts/ci/noema_review_gate.py | 25 +++++++-- tests/test_document_blob_diff.py | 64 +++++++++++++++++++++-- 5 files changed, 116 insertions(+), 18 deletions(-) diff --git a/.github/actions/noema-review/two_phase.py b/.github/actions/noema-review/two_phase.py index 04301b27a0..897232bd72 100755 --- a/.github/actions/noema-review/two_phase.py +++ b/.github/actions/noema-review/two_phase.py @@ -164,7 +164,7 @@ def prepare_verdict(repo: str, number: int, expected_head: str, path: Path) -> i return 0 diff, truncated = gate.fetch_diff(repo, number) - diff, truncated = gate.augment_binary_document_diff(repo, pull_request, diff, truncated) + diff, truncated = gate.augment_binary_document_diff(repo, number, pull_request, diff, truncated) changed_files = gate.fetch_changed_files(repo, number) changed_paths = tuple(file_path for file_path, _status in changed_files) review_context = gate.build_review_context(repo, number, pull_request, changed_files) diff --git a/CHANGELOG.md b/CHANGELOG.md index aa05bc4035..2db2f874dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,12 @@ - Figures carry the text of an adjacent "Figure N"/"Fig."/"그림 N" caption, so contextual-orchestrator's rule finding (changed image, unchanged caption) fires. - HWPX sections follow the manifest spine; a spine section missing from the package fails closed. - Emails still fail closed by default, with one narrow exception for a declared corresponding author. An address is replaced by `[CORRESPONDING_AUTHOR_EMAIL]` before hashing, diffing, context or prompt only when it is on the workflow's exact `NOEMA_CORRESPONDING_AUTHOR_EMAILS` allowlist and sits in a front-matter author-contact paragraph (before the abstract or introduction, within the first 20 paragraphs, declaring "Corresponding author"/"Correspondence"/"교신저자"). Allowlist mismatches, the same address elsewhere, emails after the abstract, undeclared contacts, extra addresses, table cells and participant phone numbers are all still rejected, and no error message contains the address. -- Tests: `tests/test_document_blob_diff.py` (58). The budget fail-open, empty-changed-text invariant, caption, spine and corresponding-author cases fail on c6f4b49b. The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. +- Tests: `tests/test_document_blob_diff.py` (58). The budget fail-open, empty-changed-text invariant, caption, spine and corresponding-author cases fail on c6f4b49b. +- Third round (exact-head review of e6f0671f and CodeRabbit): + - A hash-only object (a PDF/image `page`, an uncaptioned figure, or a package-level change) proves bytes changed but not what changed. `validate_substantive_verdict` now refuses an `approve` whenever the diff carries such a synthetic line (`NoemaModelOutputError`), and the prompt tells the model to use comment or request_changes and say what it could not review. + - An added or removed package with no extractable body still gets a citable package object (`added`/`removed`). + - When `fetch_diff` already cut the diff, `augment_binary_document_diff` re-reads the full diff before searching for binary stanzas, so a document after the cut is no longer skipped. +- Tests: 61; the three third-round cases fail on e6f0671f. The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. ### Noema transport capacity schedules a bounded continuation re-dispatch diff --git a/scripts/ci/document_blob_diff.py b/scripts/ci/document_blob_diff.py index a907cced52..3b054f8277 100644 --- a/scripts/ci/document_blob_diff.py +++ b/scripts/ci/document_blob_diff.py @@ -508,22 +508,26 @@ def build_envelope( } ) ordinals.append((base_ord, head_ord)) - if not changed and base_raw is not None and head_raw is not None: - # The bytes changed but no body object did (styles, settings, metadata): - # still a reviewable change, never "no change". + if not changed: + # The bytes changed but no body object did (styles, settings, metadata, + # or a package with no extractable body): still a reviewable, citable + # change, never "no change". + change = "modified" if base_raw is not None and head_raw is not None else ( + "added" if base_raw is None else "removed" + ) objects.append( { "page": None, "object_kind": "style", "locator": "package", - "change": "modified", - "object_hash_base": _hash(_sha256(base_raw)), - "object_hash_head": _hash(_sha256(head_raw)), + "change": change, + "object_hash_base": _hash(_sha256(base_raw)) if base_raw is not None else None, + "object_hash_head": _hash(_sha256(head_raw)) if head_raw is not None else None, "base_text": None, "head_text": None, } ) - ordinals.append((1, 1)) + ordinals.append((1 if base_raw is not None else None, 1 if head_raw is not None else None)) envelope = { "contract_version": CONTRACT_VERSION, "repo": repo, @@ -537,11 +541,27 @@ def build_envelope( return DocumentReview(envelope, tuple(ordinals)) +UNOBSERVED_TEXT = "(no text extracted; hash only)" +_UNOBSERVED_LINE = re.compile( + r"^[+-]\[(?:page|figure|style) [^\]\s]+ sha256:[0-9a-f]{12}\] \(no text extracted; hash only\)$", + re.MULTILINE, +) + + +def has_unobserved_objects(diff: str) -> bool: + """Return whether a diff carries synthetic hash-only document object lines. + + Such an object proves that bytes changed but not what changed, so a + formal approval must not rest on it. + """ + return bool(_UNOBSERVED_LINE.search(diff)) + + def _line(obj: dict[str, Any], side: str) -> str: """Render one synthetic diff line for an object side.""" digest = obj[f"object_hash_{side}"].removeprefix("sha256:")[:12] text = obj[f"{side}_text"] - body = " ".join(text.split())[:500] if text is not None else "(no text extracted; hash only)" + body = " ".join(text.split())[:500] if text is not None else UNOBSERVED_TEXT return f"[{obj['object_kind']} {obj['locator']} sha256:{digest}] {body}" diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index bfcc3bb6dd..3e6dbe0931 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -710,6 +710,11 @@ def validate_substantive_verdict( decision = str(verdict.get("decision") or "").lower() if decision == "comment": return + if decision == "approve" and document_blob_diff.has_unobserved_objects(diff): + raise NoemaModelOutputError( + "Noema cannot formally approve binary document content it did not observe " + "(hash-only page, figure, or package objects); use comment or request_changes" + ) locations = changed_diff_locations(diff) if not locations: raise RuntimeError("Noema formal verdict requires parseable changed-line evidence") @@ -923,7 +928,7 @@ def corresponding_author_allowlist() -> frozenset[str]: def augment_binary_document_diff( - repo: str, pr: dict[str, Any], diff: str, truncated: bool + repo: str, number: int, pr: dict[str, Any], diff: str, truncated: bool ) -> tuple[str, bool]: """Replace binary-only document stanzas with citable object-level hunks. @@ -932,11 +937,15 @@ def augment_binary_document_diff( blob at the merge base and the head blob are materialized, diffed as document objects, and rendered as synthetic hunks whose line numbers are object ordinals. Any safety rejection fails the review closed; a changed - blob never becomes "no change". + blob never becomes "no change". A diff already cut to ``MAX_DIFF_CHARS`` + is re-read in full first, so a binary stanza after the cut is not lost. """ + if truncated: + diff = run(["gh", "api", f"repos/{repo}/pulls/{number}", "-H", "Accept: application/vnd.github.v3.diff"]) stanzas = document_blob_diff.binary_document_stanzas(diff) if not stanzas: - return diff, truncated + bounded, more = bound_diff(diff) + return bounded, truncated or more head_sha = str(pr.get("headRefOid") or "") merge_base = fetch_merge_base_sha(repo, str(pr.get("baseRefOid") or ""), head_sha) hunks: dict[tuple[str | None, str | None], str] = {} @@ -1754,6 +1763,14 @@ def call_llm( "content": "\n".join( [ "You are Noema, an independent pull request reviewer for ContextualWisdomLab.", + *( + [ + "Some changed binary document objects are marked \"(no text extracted; hash only)\": their content " + "was not observed, so you must not approve; use comment or request_changes and say what could not be reviewed." + ] + if document_blob_diff.has_unobserved_objects(diff) + else [] + ), "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.", @@ -2019,7 +2036,7 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: print("Current head already has a Noema review; nothing to do.") return 0 diff, truncated = fetch_diff(repo, number) - diff, truncated = augment_binary_document_diff(repo, pr, diff, truncated) + diff, truncated = augment_binary_document_diff(repo, number, pr, diff, truncated) changed_files = fetch_changed_files(repo, number) changed_paths = tuple(path for path, _status in changed_files) review_context = build_review_context(repo, number, pr, changed_files) diff --git a/tests/test_document_blob_diff.py b/tests/test_document_blob_diff.py index 08f50f84bd..2d9c4cce10 100644 --- a/tests/test_document_blob_diff.py +++ b/tests/test_document_blob_diff.py @@ -359,7 +359,7 @@ def test_binary_only_docx_pr_yields_a_citable_request_changes_finding(monkeypatc }, ) pr = {"baseRefOid": "a" * 40, "headRefOid": head} - diff, truncated = gate.augment_binary_document_diff("o/r", pr, BINARY_ONLY_DIFF, False) + diff, truncated = gate.augment_binary_document_diff("o/r", 7, pr, BINARY_ONLY_DIFF, False) assert not truncated assert "Binary files a/paper.docx" not in diff and "Binary files a/logo.bin" in diff verdict = { @@ -376,14 +376,14 @@ def test_binary_only_docx_pr_yields_a_citable_request_changes_finding(monkeypatc def test_augmentation_passes_through_text_diffs_and_fails_closed_on_unsafe_blobs(monkeypatch: pytest.MonkeyPatch) -> None: """No stanza means no API call; a safety rejection fails the review closed.""" monkeypatch.setattr(gate, "run", lambda *_a, **_k: pytest.fail("no API call expected")) - assert gate.augment_binary_document_diff("o/r", {}, "diff --git a/x b/x\n", True) == ("diff --git a/x b/x\n", True) + assert gate.augment_binary_document_diff("o/r", 7, {}, "diff --git a/x b/x\n", False) == ("diff --git a/x b/x\n", False) head = "c" * 40 _fake_github( monkeypatch, {("paper.docx", "b" * 40): _docx(["ok"]), ("paper.docx", head): _zip({"word/vbaProject.bin": "x"}), ("new.pdf", head): b"%PDF"}, ) with pytest.raises(RuntimeError, match="binary document review failed closed for paper.docx: package contains macro"): - gate.augment_binary_document_diff("o/r", {"baseRefOid": "a" * 40, "headRefOid": head}, BINARY_ONLY_DIFF, False) + gate.augment_binary_document_diff("o/r", 7, {"baseRefOid": "a" * 40, "headRefOid": head}, BINARY_ONLY_DIFF, False) def test_blob_fetch_fails_closed_on_missing_sha_or_bad_base64(monkeypatch: pytest.MonkeyPatch) -> None: @@ -445,7 +445,7 @@ def test_changed_objects_over_the_envelope_budget_fail_closed_without_a_provider c = "c" * 40 _fake_github(monkeypatch, {("paper.docx", "b" * 40): base, ("paper.docx", c): head, ("new.pdf", c): b"%PDF"}) with pytest.raises(RuntimeError, match="binary document review failed closed for paper.docx: changed document objects exceed"): - gate.augment_binary_document_diff("o/r", {"baseRefOid": "a" * 40, "headRefOid": c}, BINARY_ONLY_DIFF, False) + gate.augment_binary_document_diff("o/r", 7, {"baseRefOid": "a" * 40, "headRefOid": c}, BINARY_ONLY_DIFF, False) @pytest.mark.parametrize("count", (1, 5, 30)) @@ -606,3 +606,59 @@ def test_email_inside_a_table_is_never_excepted() -> None: _docx(["Title"], table=[["Corresponding author", AUTHOR]]), corresponding_author_emails=frozenset({AUTHOR}), ) + + +# ---------------------------------------------------------------- exact-head and CodeRabbit review fixes + + +def test_hash_only_pdf_change_cannot_be_formally_approved(monkeypatch: pytest.MonkeyPatch) -> None: + """RED on e6f0671f: a PDF-only PR yielded a citable hash-only line, so an approve passed. + + Base and head PDFs say opposite things, but the extractor only proves the + bytes changed. A model approval citing that line must be refused; + request_changes and comment stay available. + """ + c = "c" * 40 + pdf_diff = "diff --git a/r.pdf b/r.pdf\nBinary files a/r.pdf and b/r.pdf differ\n" + _fake_github(monkeypatch, {("r.pdf", "b" * 40): b"%PDF effect found", ("r.pdf", c): b"%PDF no effect"}) + diff, _ = gate.augment_binary_document_diff("o/r", 7, {"baseRefOid": "a" * 40, "headRefOid": c}, pdf_diff, False) + assert dbd.has_unobserved_objects(diff) + line = {"path": "r.pdf", "line": 1, "side": "RIGHT"} + approve = {"decision": "approve", "reviewed_lines": [line], "findings": []} + with pytest.raises(gate.NoemaModelOutputError, match="cannot formally approve binary document content it did not observe"): + gate.validate_substantive_verdict(approve, diff) + gate.validate_substantive_verdict({"decision": "comment"}, diff) + assert not dbd.has_unobserved_objects("+[paragraph p1 sha256:0123456789ab] text\n") + assert not dbd.has_unobserved_objects('+ body = "(no text extracted; hash only)"\n') + + +def test_added_or_removed_package_without_extractable_objects_is_still_citable() -> None: + """A DOCX with an empty body added or removed yields a package object, not an empty hunk.""" + added = dbd.build_envelope("o/r", "e.docx", None, "2" * 40, None, _docx([])) + assert [(o["object_kind"], o["change"], o["object_hash_base"]) for o in added.envelope["objects"]] == [("style", "added", None)] + assert ("e.docx", 1, "RIGHT") in gate.changed_diff_locations(dbd.synthetic_hunks(added)) + removed = dbd.build_envelope("o/r", "e.docx", "1" * 40, None, _docx([]), None) + assert [(o["change"], o["object_hash_head"]) for o in removed.envelope["objects"]] == [("removed", None)] + assert ("e.docx", 1, "LEFT") in gate.changed_diff_locations(dbd.synthetic_hunks(removed)) + + +def test_truncated_diff_is_reread_so_a_late_binary_stanza_is_not_lost(monkeypatch: pytest.MonkeyPatch) -> None: + """A binary stanza after the MAX_DIFF_CHARS cut is recovered from the full diff.""" + c = "c" * 40 + full = "diff --git a/big.txt b/big.txt\n+" + "x" * 50 + "\n" + BINARY_ONLY_DIFF + blobs = {("paper.docx", "b" * 40): _docx(["A"]), ("paper.docx", c): _docx(["B"]), ("new.pdf", c): b"%PDF"} + _fake_github(monkeypatch, blobs) + served = gate.run + + def run_with_full_diff(args, *, stdin=None): + """Serve the full diff for the re-read; delegate blob calls.""" + if any("v3.diff" in a for a in args): + return full + return served(args, stdin=stdin) + + monkeypatch.setattr(gate, "run", run_with_full_diff) + cut = full[:40] + diff, truncated = gate.augment_binary_document_diff("o/r", 7, {"baseRefOid": "a" * 40, "headRefOid": c}, cut, True) + assert truncated is True + assert ("paper.docx", 1, "RIGHT") in gate.changed_diff_locations(diff) + assert "Binary files a/paper.docx" not in diff From 3e77db6e9b2f4c31ca4e05c5c3e37196bd25aab6 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 22 Sep 2026 02:39:28 +0900 Subject: [PATCH 5/5] ci(noema): derive the unobserved-content approve guard from envelopes The approve guard scanned rendered diff text, so a captioned image swap (not rendered as hash-only) and a hash-only line past MAX_DIFF_CHARS both slipped through. Compute path#locator ids of changed figure, page and textless package objects from the envelopes, return them from augment as review metadata, list them in the prompt and refuse any approve while one is present. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LeLbwEgYXzCzpaFMTHDcBS --- .github/actions/noema-review/two_phase.py | 3 +- CHANGELOG.md | 4 +- scripts/ci/document_blob_diff.py | 24 ++-- scripts/ci/noema_review_gate.py | 45 ++++--- tests/test_document_blob_diff.py | 140 +++++++++++++++++++--- 5 files changed, 169 insertions(+), 47 deletions(-) diff --git a/.github/actions/noema-review/two_phase.py b/.github/actions/noema-review/two_phase.py index 897232bd72..0a6d499392 100755 --- a/.github/actions/noema-review/two_phase.py +++ b/.github/actions/noema-review/two_phase.py @@ -164,7 +164,7 @@ def prepare_verdict(repo: str, number: int, expected_head: str, path: Path) -> i return 0 diff, truncated = gate.fetch_diff(repo, number) - diff, truncated = gate.augment_binary_document_diff(repo, number, pull_request, diff, truncated) + diff, truncated, unobserved = gate.augment_binary_document_diff(repo, number, pull_request, diff, truncated) changed_files = gate.fetch_changed_files(repo, number) changed_paths = tuple(file_path for file_path, _status in changed_files) review_context = gate.build_review_context(repo, number, pull_request, changed_files) @@ -178,6 +178,7 @@ def prepare_verdict(repo: str, number: int, expected_head: str, path: Path) -> i expected, review_context, changed_paths, + unobserved, ) except gate.NoemaTransportError as exc: _emit_transport_capacity_outputs(exc, expected_head=expected) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2db2f874dd..f24d90a1ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,10 +14,10 @@ - Emails still fail closed by default, with one narrow exception for a declared corresponding author. An address is replaced by `[CORRESPONDING_AUTHOR_EMAIL]` before hashing, diffing, context or prompt only when it is on the workflow's exact `NOEMA_CORRESPONDING_AUTHOR_EMAILS` allowlist and sits in a front-matter author-contact paragraph (before the abstract or introduction, within the first 20 paragraphs, declaring "Corresponding author"/"Correspondence"/"교신저자"). Allowlist mismatches, the same address elsewhere, emails after the abstract, undeclared contacts, extra addresses, table cells and participant phone numbers are all still rejected, and no error message contains the address. - Tests: `tests/test_document_blob_diff.py` (58). The budget fail-open, empty-changed-text invariant, caption, spine and corresponding-author cases fail on c6f4b49b. - Third round (exact-head review of e6f0671f and CodeRabbit): - - A hash-only object (a PDF/image `page`, an uncaptioned figure, or a package-level change) proves bytes changed but not what changed. `validate_substantive_verdict` now refuses an `approve` whenever the diff carries such a synthetic line (`NoemaModelOutputError`), and the prompt tells the model to use comment or request_changes and say what it could not review. + - A changed figure (captioned or not), a PDF/image `page`, or a textless package object proves bytes changed but not what changed. `document_blob_diff.unobserved_changed_objects()` derives their `path#locator` ids from the envelope itself, and `augment_binary_document_diff` returns them as structured review metadata. `call_llm` lists them in the prompt and passes them to `validate_substantive_verdict`, which refuses any `approve` while one is present (`NoemaModelOutputError`); comment and request_changes stay available. The flag no longer comes from scanning rendered or truncated diff text, so neither a caption (8da729e4 bypass 1) nor a `MAX_DIFF_CHARS` cut (bypass 2) can hide an unobserved change. - An added or removed package with no extractable body still gets a citable package object (`added`/`removed`). - When `fetch_diff` already cut the diff, `augment_binary_document_diff` re-reads the full diff before searching for binary stanzas, so a document after the cut is no longer skipped. -- Tests: 61; the three third-round cases fail on e6f0671f. The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. +- Tests: 66. The captioned and uncaptioned image swap, diff-cut, PDF-only and prompt/leak cases fail on 8da729e4. A text-only DOCX edit is the negative control: no unobserved ids, and a valid approve passes. The binary-only request_changes integration test, the fail-closed augmentation test and the no-raw-decode test fail on the previous gate and pass now. The new module has 100% branch coverage. ### Noema transport capacity schedules a bounded continuation re-dispatch diff --git a/scripts/ci/document_blob_diff.py b/scripts/ci/document_blob_diff.py index 3b054f8277..7de18330f0 100644 --- a/scripts/ci/document_blob_diff.py +++ b/scripts/ci/document_blob_diff.py @@ -542,19 +542,21 @@ def build_envelope( UNOBSERVED_TEXT = "(no text extracted; hash only)" -_UNOBSERVED_LINE = re.compile( - r"^[+-]\[(?:page|figure|style) [^\]\s]+ sha256:[0-9a-f]{12}\] \(no text extracted; hash only\)$", - re.MULTILINE, -) - - -def has_unobserved_objects(diff: str) -> bool: - """Return whether a diff carries synthetic hash-only document object lines. +def unobserved_changed_objects(review: DocumentReview) -> list[str]: + """Return ``path#locator`` ids of changed objects whose content was not observed. - Such an object proves that bytes changed but not what changed, so a - formal approval must not rest on it. + A figure or page is never observed in v1 (no pixels or page render reach + the reviewer, caption or not), and a textless package object only proves + the bytes changed. Computed from the envelope, not from rendered or + truncated diff text, so neither a caption nor a diff cut can hide it. """ - return bool(_UNOBSERVED_LINE.search(diff)) + envelope = review.envelope + return [ + f"{envelope['path']}#{obj['locator']}" + for obj in envelope["objects"] + if obj["change"] != "unchanged" + and (obj["object_kind"] in {"figure", "page"} or (obj["base_text"] is None and obj["head_text"] is None)) + ] def _line(obj: dict[str, Any], side: str) -> str: diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 3e6dbe0931..64c0b371c4 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -704,16 +704,25 @@ def _nearby_changed_locations( def validate_substantive_verdict( - verdict: dict[str, Any], diff: str, changed_paths: Sequence[str] = () + verdict: dict[str, Any], + diff: str, + changed_paths: Sequence[str] = (), + unobserved_document_objects: Sequence[str] = (), ) -> None: - """Reject formal verdicts without exact changed-line/adversarial evidence.""" + """Reject formal verdicts without exact changed-line/adversarial evidence. + + ``unobserved_document_objects`` are ``path#locator`` ids of changed + document objects (figures, pages, textless packages) whose content no + reviewer saw; any of them makes a formal approval fail closed. + """ decision = str(verdict.get("decision") or "").lower() if decision == "comment": return - if decision == "approve" and document_blob_diff.has_unobserved_objects(diff): + if decision == "approve" and unobserved_document_objects: raise NoemaModelOutputError( - "Noema cannot formally approve binary document content it did not observe " - "(hash-only page, figure, or package objects); use comment or request_changes" + "Noema cannot formally approve changed document content it did not observe " + f"({len(unobserved_document_objects)} unobserved object(s): " + f"{', '.join(unobserved_document_objects[:5])}); use comment or request_changes" ) locations = changed_diff_locations(diff) if not locations: @@ -929,7 +938,7 @@ def corresponding_author_allowlist() -> frozenset[str]: def augment_binary_document_diff( repo: str, number: int, pr: dict[str, Any], diff: str, truncated: bool -) -> tuple[str, bool]: +) -> tuple[str, bool, tuple[str, ...]]: """Replace binary-only document stanzas with citable object-level hunks. A DOCX/HWPX/PDF/image change arrives as ``Binary files … differ`` with no @@ -939,16 +948,19 @@ def augment_binary_document_diff( object ordinals. Any safety rejection fails the review closed; a changed blob never becomes "no change". A diff already cut to ``MAX_DIFF_CHARS`` is re-read in full first, so a binary stanza after the cut is not lost. + Also returns the ids of changed objects whose content was not observed, + derived from the envelopes so a caption or a diff cut cannot hide them. """ if truncated: diff = run(["gh", "api", f"repos/{repo}/pulls/{number}", "-H", "Accept: application/vnd.github.v3.diff"]) stanzas = document_blob_diff.binary_document_stanzas(diff) if not stanzas: bounded, more = bound_diff(diff) - return bounded, truncated or more + return bounded, truncated or more, () head_sha = str(pr.get("headRefOid") or "") merge_base = fetch_merge_base_sha(repo, str(pr.get("baseRefOid") or ""), head_sha) hunks: dict[tuple[str | None, str | None], str] = {} + unobserved: list[str] = [] for old_path, new_path in stanzas: base_blob, base_raw = fetch_file_blob_at_ref(repo, old_path, merge_base) if old_path else (None, None) head_blob, head_raw = fetch_file_blob_at_ref(repo, new_path, head_sha) if new_path else (None, None) @@ -967,8 +979,9 @@ def augment_binary_document_diff( except document_blob_diff.DocumentSafetyError as exc: raise RuntimeError(f"binary document review failed closed for {path}: {exc}") from exc hunks[(old_path, new_path)] = document_blob_diff.synthetic_hunks(review, old_path) + unobserved.extend(document_blob_diff.unobserved_changed_objects(review)) bounded, more = bound_diff(document_blob_diff.replace_binary_stanzas(diff, hunks)) - return bounded, truncated or more + return bounded, truncated or more, tuple(unobserved) def fetch_merge_base_sha(repo: str, base_sha: str, head_sha: str) -> str: @@ -1732,6 +1745,7 @@ def call_llm( expected_head: str, review_context: str = "", changed_paths: Sequence[str] = (), + unobserved_document_objects: Sequence[str] = (), ) -> dict[str, Any]: """Issue exactly one structured-output request through contextual-orchestrator. @@ -1765,10 +1779,11 @@ def call_llm( "You are Noema, an independent pull request reviewer for ContextualWisdomLab.", *( [ - "Some changed binary document objects are marked \"(no text extracted; hash only)\": their content " - "was not observed, so you must not approve; use comment or request_changes and say what could not be reviewed." + "These changed document objects were not observed (no pixels, page render or body text " + f"reached you): {', '.join(unobserved_document_objects[:20])}. You must not approve; " + "use comment or request_changes and say what could not be reviewed." ] - if document_blob_diff.has_unobserved_objects(diff) + if unobserved_document_objects else [] ), "Review the PR diff plus the additional changed-file and review-thread context for correctness, security, maintainability, and behavioral regressions.", @@ -1858,7 +1873,7 @@ def call_llm( raise NoemaModelOutputError( "Noema LLM request_changes response did not contain a substantive finding" ) - validate_substantive_verdict(verdict, diff, changed_paths) + validate_substantive_verdict(verdict, diff, changed_paths, unobserved_document_objects) except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc: gateway_telemetry: dict[str, str | int] = {} http_status: int | None = None @@ -2036,11 +2051,13 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: print("Current head already has a Noema review; nothing to do.") return 0 diff, truncated = fetch_diff(repo, number) - diff, truncated = augment_binary_document_diff(repo, number, pr, diff, truncated) + diff, truncated, unobserved = augment_binary_document_diff(repo, number, pr, diff, truncated) changed_files = fetch_changed_files(repo, number) changed_paths = tuple(path for path, _status in changed_files) review_context = build_review_context(repo, number, pr, changed_files) - verdict = call_llm(repo, number, pr, diff, truncated, expected_head, review_context, changed_paths) + verdict = call_llm( + repo, number, pr, diff, truncated, expected_head, review_context, changed_paths, unobserved + ) current_pr = fetch_pr(repo, number) try: require_expected_head(current_pr, expected_head) diff --git a/tests/test_document_blob_diff.py b/tests/test_document_blob_diff.py index 2d9c4cce10..a501c0b364 100644 --- a/tests/test_document_blob_diff.py +++ b/tests/test_document_blob_diff.py @@ -359,7 +359,7 @@ def test_binary_only_docx_pr_yields_a_citable_request_changes_finding(monkeypatc }, ) pr = {"baseRefOid": "a" * 40, "headRefOid": head} - diff, truncated = gate.augment_binary_document_diff("o/r", 7, pr, BINARY_ONLY_DIFF, False) + diff, truncated, _unobserved = gate.augment_binary_document_diff("o/r", 7, pr, BINARY_ONLY_DIFF, False) assert not truncated assert "Binary files a/paper.docx" not in diff and "Binary files a/logo.bin" in diff verdict = { @@ -376,7 +376,7 @@ def test_binary_only_docx_pr_yields_a_citable_request_changes_finding(monkeypatc def test_augmentation_passes_through_text_diffs_and_fails_closed_on_unsafe_blobs(monkeypatch: pytest.MonkeyPatch) -> None: """No stanza means no API call; a safety rejection fails the review closed.""" monkeypatch.setattr(gate, "run", lambda *_a, **_k: pytest.fail("no API call expected")) - assert gate.augment_binary_document_diff("o/r", 7, {}, "diff --git a/x b/x\n", False) == ("diff --git a/x b/x\n", False) + assert gate.augment_binary_document_diff("o/r", 7, {}, "diff --git a/x b/x\n", False) == ("diff --git a/x b/x\n", False, ()) head = "c" * 40 _fake_github( monkeypatch, @@ -611,25 +611,127 @@ def test_email_inside_a_table_is_never_excepted() -> None: # ---------------------------------------------------------------- exact-head and CodeRabbit review fixes -def test_hash_only_pdf_change_cannot_be_formally_approved(monkeypatch: pytest.MonkeyPatch) -> None: - """RED on e6f0671f: a PDF-only PR yielded a citable hash-only line, so an approve passed. +def _approve(path: str, line: int) -> dict: + """A structurally valid approve verdict citing one changed line (as in the gate tests).""" + probe = { + "path": path, "line": line, "side": "RIGHT", "hypothesis": "h", "attack_or_counterexample": "a", + "evidence": "e", "outcome": "falsified", + } + return { + "decision": "approve", + "summary": "Reviewed.", + "findings": [], + "reviewed_lines": [{"path": path, "line": line, "side": "RIGHT", "analysis": "Checked."}], + "adversarial_validation": {"status": "passed", "residual_risk": "none", "probes": [probe, dict(probe, hypothesis="h2")]}, + } - Base and head PDFs say opposite things, but the extractor only proves the - bytes changed. A model approval citing that line must be refused; - request_changes and comment stay available. - """ + +def _augment(monkeypatch: pytest.MonkeyPatch, blobs: dict, diff: str, truncated: bool = False): + """Run augmentation against canned blobs for head ``c*40``.""" + _fake_github(monkeypatch, blobs) + return gate.augment_binary_document_diff("o/r", 7, {"baseRefOid": "a" * 40, "headRefOid": "c" * 40}, diff, truncated) + + +PAPER = "diff --git a/paper.docx b/paper.docx\nBinary files a/paper.docx and b/paper.docx differ\n" + + +def test_hash_only_pdf_change_cannot_be_formally_approved(monkeypatch: pytest.MonkeyPatch) -> None: + """A PDF-only PR whose base/head say opposite things: approve fails closed, comment stays open.""" c = "c" * 40 pdf_diff = "diff --git a/r.pdf b/r.pdf\nBinary files a/r.pdf and b/r.pdf differ\n" - _fake_github(monkeypatch, {("r.pdf", "b" * 40): b"%PDF effect found", ("r.pdf", c): b"%PDF no effect"}) - diff, _ = gate.augment_binary_document_diff("o/r", 7, {"baseRefOid": "a" * 40, "headRefOid": c}, pdf_diff, False) - assert dbd.has_unobserved_objects(diff) - line = {"path": "r.pdf", "line": 1, "side": "RIGHT"} - approve = {"decision": "approve", "reviewed_lines": [line], "findings": []} - with pytest.raises(gate.NoemaModelOutputError, match="cannot formally approve binary document content it did not observe"): - gate.validate_substantive_verdict(approve, diff) - gate.validate_substantive_verdict({"decision": "comment"}, diff) - assert not dbd.has_unobserved_objects("+[paragraph p1 sha256:0123456789ab] text\n") - assert not dbd.has_unobserved_objects('+ body = "(no text extracted; hash only)"\n') + diff, _, unobserved = _augment(monkeypatch, {("r.pdf", "b" * 40): b"%PDF effect found", ("r.pdf", c): b"%PDF no effect"}, pdf_diff) + assert unobserved == ("r.pdf#blob",) + with pytest.raises(gate.NoemaModelOutputError, match=r"did not observe \(1 unobserved object\(s\): r.pdf#blob\)"): + gate.validate_substantive_verdict(_approve("r.pdf", 1), diff, (), unobserved) + gate.validate_substantive_verdict({"decision": "comment"}, diff, (), unobserved) + + +@pytest.mark.parametrize("caption", (True, False)) +def test_captioned_or_uncaptioned_image_swap_is_unobserved(monkeypatch: pytest.MonkeyPatch, caption: bool) -> None: + """RED bypass 1 on 8da729e4: a captioned image swap rendered with text and passed the regex guard.""" + c = "c" * 40 + text = ["Figure 1. Recruitment flow"] if caption else ["Body"] + diff, _, unobserved = _augment( + monkeypatch, {("paper.docx", "b" * 40): _docx(text, image=b"IMG-A"), ("paper.docx", c): _docx(text, image=b"IMG-B")}, PAPER + ) + assert unobserved == ("paper.docx#p2/fig:word/media/image1.png",) + line = 2 + with pytest.raises(gate.NoemaModelOutputError, match="did not observe"): + gate.validate_substantive_verdict(_approve("paper.docx", line), diff, (), unobserved) + + +def test_unobserved_flag_survives_a_diff_cut(monkeypatch: pytest.MonkeyPatch) -> None: + """RED bypass 2 on 8da729e4: the hash-only line past MAX_DIFF_CHARS vanished from the guard's view.""" + c = "c" * 40 + filler = "diff --git a/big.txt b/big.txt\n--- a/big.txt\n+++ b/big.txt\n@@ -1,1 +1,1 @@\n-a\n+" + "x" * (gate.MAX_DIFF_CHARS + 10) + "\n" + diff, truncated, unobserved = _augment( + monkeypatch, {("r.pdf", "b" * 40): b"%PDF old", ("r.pdf", c): b"%PDF new"}, + filler + "diff --git a/r.pdf b/r.pdf\nBinary files a/r.pdf and b/r.pdf differ\n", + ) + assert truncated and "r.pdf" not in diff + assert unobserved == ("r.pdf#blob",) + with pytest.raises(gate.NoemaModelOutputError, match="r.pdf#blob"): + gate.validate_substantive_verdict(_approve("big.txt", 1), diff, (), unobserved) + + +def test_observable_text_document_change_is_a_negative_control(monkeypatch: pytest.MonkeyPatch) -> None: + """A text-only DOCX edit is fully observed: no unobserved ids, and a valid approve passes.""" + c = "c" * 40 + diff, _, unobserved = _augment(monkeypatch, {("paper.docx", "b" * 40): _docx(["Old claim"]), ("paper.docx", c): _docx(["New claim"])}, PAPER) + assert unobserved == () + gate.validate_substantive_verdict(_approve("paper.docx", 1), diff, (), unobserved) + + +def test_prompt_lists_unobserved_objects_and_a_refused_approve_leaks_no_secret(monkeypatch: pytest.MonkeyPatch) -> None: + """The model is told which objects it did not see; the refusal names ids only.""" + import json as _json + + seen: dict = {} + monkeypatch.setenv("NOEMA_LLM_API_URL", "https://llm.example.test/chat") + monkeypatch.setenv("NOEMA_LLM_API_KEY", "sk-live-secret-value") + + class _Response: + """Minimal HTTP response carrying one approve verdict.""" + + def __init__(self, payload: dict) -> None: + """Store the JSON payload.""" + self.body = _json.dumps(payload).encode() + self.headers = {} + self.status = 200 + + def read(self) -> bytes: + """Return the body.""" + return self.body + + def __enter__(self): + """Context-manager entry.""" + return self + + def __exit__(self, *_exc) -> None: + """Context-manager exit.""" + + class _Opener: + """Capture the request and return an approve verdict.""" + + def open(self, request, timeout=None): + """Record the prompt and answer approve.""" + seen["body"] = _json.loads(request.data.decode()) + verdict = _approve("r.pdf", 1) + return _Response({"choices": [{"message": {"content": _json.dumps(verdict)}}]}) + + monkeypatch.setattr(gate.urllib.request, "build_opener", lambda *_a: _Opener()) + diff = "diff --git a/r.pdf b/r.pdf\n--- a/r.pdf\n+++ b/r.pdf\n@@ -1,1 +1,1 @@ modified\n-[page blob sha256:000000000000] x\n+[page blob sha256:111111111111] y\n" + with pytest.raises(RuntimeError) as raised: + gate.call_llm("o/r", 7, {"headRefOid": "c" * 40}, diff, False, "c" * 40, "", ("r.pdf",), ("r.pdf#blob",)) + prompt = _json.dumps(seen["body"]) + assert "These changed document objects were not observed" in prompt and "r.pdf#blob" in prompt + assert isinstance(raised.value, gate.NoemaModelOutputError) + assert "sk-live-secret-value" not in str(raised.value) + # Control: the identical model answer is accepted when every changed object was observed, + # so the refusal above comes from the unobserved guard and nothing else. + verdict = gate.call_llm("o/r", 7, {"headRefOid": "c" * 40}, diff, False, "c" * 40, "", ("r.pdf",), ()) + assert verdict["decision"] == "approve" + assert "were not observed" not in _json.dumps(seen["body"]) def test_added_or_removed_package_without_extractable_objects_is_still_citable() -> None: @@ -658,7 +760,7 @@ def run_with_full_diff(args, *, stdin=None): monkeypatch.setattr(gate, "run", run_with_full_diff) cut = full[:40] - diff, truncated = gate.augment_binary_document_diff("o/r", 7, {"baseRefOid": "a" * 40, "headRefOid": c}, cut, True) + diff, truncated, _unobserved = gate.augment_binary_document_diff("o/r", 7, {"baseRefOid": "a" * 40, "headRefOid": c}, cut, True) assert truncated is True assert ("paper.docx", 1, "RIGHT") in gate.changed_diff_locations(diff) assert "Binary files a/paper.docx" not in diff