From c5eff8008e52c39be927ef1a43a29b3fd63fb7be Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 8 Jul 2026 11:08:27 +0900 Subject: [PATCH 01/18] feat(pdf-dom): recognize PDF DOM via NewsDOM sidecar into the content graph MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integrate the NewsDOM PDF recognition sidecar so naruon can turn scanned/native PDFs into a structured DOM and land it in the content graph. - newsdom_client.py: SSRF-safe, address-pinned async httpx client that POSTs multipart PDF bytes to {base_url}/parse with language/mode form fields and an optional bearer token. Validates against a new ALLOWED_NEWSDOM_HOSTS allowlist (mirrors ALLOWED_LLM_BASE_URL_HOSTS) with an ALLOW_LOCAL_NEWSDOM_PROVIDERS escape hatch for the docker sidecar. - NewsdomProvider model + migration 0010: base_url in plaintext, api_token as EncryptedString (Fernet), read from the DB per-org — never os.getenv at runtime, mirroring LLMProvider. - Attachment path: application/pdf now maps to a deferred 'pdf' descriptor (parse_status pdf_dom_recognition_pending) instead of unsupported_binary, so heavy OCR/MinerU never runs inline during import. The worker maps the returned pages->articles->body_blocks tree into Attachment.parse_content (embeddings) and a document->section->paragraph ContentNode/ContentSegment graph. - Data-documents path: pdf-dom-recognition intent endpoint (status pdf_dom_recognition_pending) mirroring the HWP conversion intent, plus a binary PDF upload variant; executed by the worker calling newsdom_client. - Submodule + sidecar: vendor/newsdom-api pinned submodule and a 'newsdom' docker-compose service (healthcheck on /health). naruon degrades gracefully without the sidecar (PDFs stay pending). - Tests: fast mocked unit tests for the pdf descriptor, the DOM->content-graph mapping, config-resolved-from-DB, the recognition worker with a mocked client, and SSRF allowlist rejection. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01RTAMs4bpSZS77Xe3RQjv9P --- .gitmodules | 4 + .../versions/0010_newsdom_providers.py | 67 +++ backend/api/data.py | 93 +++- backend/core/config.py | 6 + backend/db/models.py | 39 ++ backend/services/attachment_parser.py | 34 ++ backend/services/content_graph/__init__.py | 6 +- backend/services/content_graph/models.py | 16 +- backend/services/content_graph/parser.py | 82 +++- backend/services/newsdom_client.py | 425 ++++++++++++++++++ backend/services/newsdom_pdf_recognition.py | 221 +++++++++ backend/services/newsdom_worker.py | 149 ++++++ backend/tests/test_attachment_parser.py | 45 +- backend/tests/test_email_parser.py | 8 +- backend/tests/test_newsdom_client.py | 108 +++++ backend/tests/test_newsdom_pdf_recognition.py | 263 +++++++++++ docker-compose.yml | 23 + vendor/newsdom-api | 1 + 18 files changed, 1577 insertions(+), 13 deletions(-) create mode 100644 .gitmodules create mode 100644 backend/alembic/versions/0010_newsdom_providers.py create mode 100644 backend/services/newsdom_client.py create mode 100644 backend/services/newsdom_pdf_recognition.py create mode 100644 backend/services/newsdom_worker.py create mode 100644 backend/tests/test_newsdom_client.py create mode 100644 backend/tests/test_newsdom_pdf_recognition.py create mode 160000 vendor/newsdom-api diff --git a/.gitmodules b/.gitmodules new file mode 100644 index 000000000..952e4ef63 --- /dev/null +++ b/.gitmodules @@ -0,0 +1,4 @@ +[submodule "vendor/newsdom-api"] + path = vendor/newsdom-api + url = https://github.com/ContextualWisdomLab/newsdom-api.git + branch = develop diff --git a/backend/alembic/versions/0010_newsdom_providers.py b/backend/alembic/versions/0010_newsdom_providers.py new file mode 100644 index 000000000..d897243b8 --- /dev/null +++ b/backend/alembic/versions/0010_newsdom_providers.py @@ -0,0 +1,67 @@ +"""add newsdom provider credentials table + +Revision ID: 0010_newsdom_providers +Revises: 0009_project_graph_projection +Create Date: 2026-07-08 00:00:00.000000 +""" + +from alembic import op +import sqlalchemy as sa + +revision = "0010_newsdom_providers" +down_revision = "0009_project_graph_projection" +_NEWSDOM_TABLE = "newsdom_providers" + + +def upgrade() -> None: + connection = op.get_bind() + inspector = sa.inspect(connection) + if not inspector.has_table(_NEWSDOM_TABLE): + op.create_table( + _NEWSDOM_TABLE, + sa.Column("id", sa.Integer(), nullable=False), + sa.Column("user_id", sa.String(), nullable=False), + sa.Column("organization_id", sa.String(), nullable=False), + sa.Column("provider_name", sa.String(), nullable=False), + sa.Column("base_url", sa.String(), nullable=True), + sa.Column("api_token", sa.String(), nullable=True), + sa.Column("request_language", sa.String(length=32), nullable=False), + sa.Column("recognition_mode", sa.String(length=32), nullable=False), + sa.Column("is_active", sa.Boolean(), nullable=False), + sa.Column("updated_at", sa.DateTime(timezone=True), nullable=True), + sa.PrimaryKeyConstraint("id"), + sa.UniqueConstraint( + "organization_id", + "provider_name", + name="uq_newsdom_providers_org_name", + ), + ) + + for index_name, column_names in _newsdom_provider_indexes(): + op.create_index( + index_name, + _NEWSDOM_TABLE, + column_names, + if_not_exists=True, + ) + + +def downgrade() -> None: + connection = op.get_bind() + inspector = sa.inspect(connection) + if inspector.has_table(_NEWSDOM_TABLE): + for index_name, _column_names in reversed(_newsdom_provider_indexes()): + op.drop_index( + index_name, + table_name=_NEWSDOM_TABLE, + if_exists=True, + ) + op.drop_table(_NEWSDOM_TABLE) + + +def _newsdom_provider_indexes() -> list[tuple[str, list[str]]]: + return [ + ("ix_newsdom_providers_user_id", ["user_id"]), + ("ix_newsdom_providers_organization_id", ["organization_id"]), + ("ix_newsdom_providers_provider_name", ["provider_name"]), + ] diff --git a/backend/api/data.py b/backend/api/data.py index e1aeda815..39da2ed9a 100644 --- a/backend/api/data.py +++ b/backend/api/data.py @@ -1,10 +1,12 @@ +import base64 +import binascii from datetime import datetime, timezone import hashlib import json import re from typing import Literal, NamedTuple -from fastapi import APIRouter, Depends, HTTPException +from fastapi import APIRouter, Depends, File, HTTPException, UploadFile from pydantic import BaseModel, ConfigDict, Field from sqlalchemy import and_, case, func, or_, select from sqlalchemy.engine import Row @@ -28,12 +30,17 @@ ) from db.session import get_db from services.attachment_parser import get_attachment_parser_manifest +from services.newsdom_pdf_recognition import ( + PDF_DOM_RECOGNITION_PENDING_STATUS, +) from services.ontology_service import ontology_service from services.webdav_service import webdav_service router = APIRouter(prefix="/api/data", tags=["data"]) DATA_VECTOR_DIMENSIONS = 1536 +# Upper bound for the binary PDF DOM recognition upload variant. +_MAX_PDF_DOM_UPLOAD_BYTES = 50 * 1024 * 1024 ATTACHMENT_PARSE_BREAKDOWN_EVIDENCE_SOURCE = ( "email_attachments.content_type, " "email_attachments.parse_content_type, " @@ -5036,7 +5043,11 @@ def _document_repository_assets(documents: list[Document]) -> list[DataRepositor assets: list[DataRepositoryAsset] = [] for document in documents: content_chars = _document_content_chars(document) - pending_statuses = {"embedding_pending", "hwp_conversion_pending"} + pending_statuses = { + "embedding_pending", + "hwp_conversion_pending", + PDF_DOM_RECOGNITION_PENDING_STATUS, + } state_code: RepositoryAssetState = ( "needs_attention" if content_chars <= 0 or document.document_status in pending_statuses @@ -5632,6 +5643,84 @@ async def create_document_hwp_conversion_intent( ) +@router.post( + "/documents/{document_id}/pdf-dom-recognition-intent", + response_model=DataDocumentActionResponse, +) +async def create_document_pdf_dom_recognition_intent( + document_id: str, + auth_context: AuthContext = Depends(get_auth_context), + db: AsyncSession = Depends(get_db), +) -> DataDocumentActionResponse: + document = await _get_workspace_document(db, auth_context, document_id) + document.document_status = PDF_DOM_RECOGNITION_PENDING_STATUS + await db.commit() + await db.refresh(document) + return _document_response( + document, + audit_event="data.document.pdf_dom_recognition_intent", + message=( + "PDF DOM recognition intent recorded; the NewsDOM sidecar worker " + "will land the structured DOM. No provider write executed." + ), + ) + + +@router.post( + "/documents/pdf-dom-recognition", + response_model=DataDocumentActionResponse, +) +async def upload_document_for_pdf_dom_recognition( + file: UploadFile = File(...), + document_name: str | None = None, + auth_context: AuthContext = Depends(get_auth_context), + db: AsyncSession = Depends(get_db), +) -> DataDocumentActionResponse: + """Binary upload variant: accept a PDF, stash it pending, and defer the + heavy NewsDOM recognition to the worker.""" + raw = await file.read(_MAX_PDF_DOM_UPLOAD_BYTES + 1) + if len(raw) > _MAX_PDF_DOM_UPLOAD_BYTES: + raise HTTPException(status_code=413, detail="PDF upload is too large.") + if not raw[:5] == b"%PDF-": + raise HTTPException( + status_code=415, + detail="Only application/pdf uploads are supported for DOM recognition.", + ) + document = Document( + workspace_id=auth_context.workspace_id, + document_name=_safe_display_text( + document_name or file.filename, "workspace document" + ), + document_type="pdf", + document_content=base64.b64encode(raw).decode("ascii"), + document_status=PDF_DOM_RECOGNITION_PENDING_STATUS, + ) + db.add(document) + await db.commit() + await db.refresh(document) + return _document_response( + document, + audit_event="data.document.pdf_dom_recognition_upload", + message=( + "PDF stored pending NewsDOM DOM recognition; the worker will parse " + "it into the content graph. No provider write executed." + ), + ) + + +def decode_pending_pdf_document_bytes(document: Document) -> bytes: + """Decode the base64 PDF payload stashed by the binary upload variant. + + Used by the recognition worker before calling the NewsDOM sidecar. + """ + try: + return base64.b64decode( + (document.document_content or "").encode("ascii"), validate=True + ) + except (binascii.Error, ValueError) as exc: + raise ValueError("Pending PDF document payload is not valid base64") from exc + + @router.post( "/documents/{document_id}/webdav-materialization-intent", response_model=DataDocumentWebdavMaterializationResponse, diff --git a/backend/core/config.py b/backend/core/config.py index d630c45f8..de36105e0 100644 --- a/backend/core/config.py +++ b/backend/core/config.py @@ -76,6 +76,12 @@ class Settings(BaseSettings): ALLOWED_POP3_PORTS: str = "995" ALLOWED_LLM_BASE_URL_HOSTS: str = "" ALLOW_LOCAL_LLM_PROVIDERS: bool = False + # NewsDOM PDF DOM recognition sidecar. Mirrors the LLM provider allowlist + # controls: the base URL host must be listed here before any request is + # pinned and dispatched, and container-name / loopback hosts are only + # accepted when ALLOW_LOCAL_NEWSDOM_PROVIDERS is enabled (dev / docker). + ALLOWED_NEWSDOM_HOSTS: str = "" + ALLOW_LOCAL_NEWSDOM_PROVIDERS: bool = False ALLOWED_CORS_ORIGINS: str = "" ENABLE_PROMETHEUS_METRICS: bool = False DATA_REGION: str = "kr" diff --git a/backend/db/models.py b/backend/db/models.py index b68bb23f9..af93379fd 100644 --- a/backend/db/models.py +++ b/backend/db/models.py @@ -169,6 +169,45 @@ class LLMProvider(Base): ) +class NewsdomProvider(Base): + """NewsDOM PDF DOM recognition sidecar credentials. + + Mirrors :class:`LLMProvider`: the base URL is stored in plaintext while the + bearer token is encrypted at rest via :class:`EncryptedString` (Fernet). + Consumer code reads these values from the database — never from + ``os.getenv`` at request time — so the sidecar can be (re)configured per + organization without redeploying the API. + """ + + __tablename__ = "newsdom_providers" + __table_args__ = ( + UniqueConstraint( + "organization_id", + "provider_name", + name="uq_newsdom_providers_org_name", + ), + ) + + id: Mapped[int] = mapped_column(primary_key=True) + user_id: Mapped[str] = mapped_column(String, index=True, nullable=False) + organization_id: Mapped[str] = mapped_column(String, index=True, nullable=False) + provider_name: Mapped[str] = mapped_column(String, index=True, nullable=False) + base_url: Mapped[str | None] = mapped_column(String, nullable=True) + api_token: Mapped[str | None] = mapped_column(EncryptedString, nullable=True) + request_language: Mapped[str] = mapped_column( + String(32), default="auto", nullable=False + ) + recognition_mode: Mapped[str] = mapped_column( + String(32), default="auto", nullable=False + ) + is_active: Mapped[bool] = mapped_column(Boolean, default=False, nullable=False) + updated_at: Mapped[datetime.datetime] = mapped_column( + DateTime(timezone=True), + default=datetime.datetime.utcnow, + onupdate=datetime.datetime.utcnow, + ) + + class WorkspaceRunnerConfig(Base): __tablename__ = "workspace_runner_configs" diff --git a/backend/services/attachment_parser.py b/backend/services/attachment_parser.py index 51baf1904..eb7e03a44 100644 --- a/backend/services/attachment_parser.py +++ b/backend/services/attachment_parser.py @@ -44,6 +44,13 @@ class AttachmentParserDescriptor: extensions=(".md", ".markdown"), parse_status="parsed", ), + AttachmentParserDescriptor( + parser_key="pdf", + display_name="PDF documents (NewsDOM recognition)", + content_types=("application/pdf",), + extensions=(".pdf",), + parse_status="pdf_dom_recognition_pending", + ), AttachmentParserDescriptor( parser_key="unsupported_binary", display_name="Unsupported binary attachments", @@ -52,16 +59,27 @@ class AttachmentParserDescriptor: parse_status="unsupported_content_type", ), ) +# Statuses whose recognition is too heavy to run inline during import. The +# attachment is stored with the pending status and a background worker later +# calls the NewsDOM sidecar to fill in parse_content + the content graph. +_DEFERRED_PARSE_STATUSES = frozenset({"pdf_dom_recognition_pending"}) _SUPPORTED_CONTENT_TYPES = { content_type for descriptor in _PARSER_MANIFEST if descriptor.parse_status == "parsed" for content_type in descriptor.content_types } +_DEFERRED_DESCRIPTORS_BY_CONTENT_TYPE = { + content_type: descriptor + for descriptor in _PARSER_MANIFEST + if descriptor.parse_status in _DEFERRED_PARSE_STATUSES + for content_type in descriptor.content_types +} _EXTENSION_CONTENT_TYPES = { extension: descriptor.content_types[0] for descriptor in _PARSER_MANIFEST if descriptor.parse_status == "parsed" + or descriptor.parse_status in _DEFERRED_PARSE_STATUSES for extension in descriptor.extensions } @@ -95,6 +113,22 @@ def parse_email_attachment( normalized_content_type, ) + deferred_descriptor = _DEFERRED_DESCRIPTORS_BY_CONTENT_TYPE.get(parse_content_type) + if deferred_descriptor is not None: + # Heavy recognition (OCR/MinerU via the NewsDOM sidecar) must not run + # inline during import — mark the attachment pending and let the worker + # populate parse_content + the content graph. + return AttachmentParseResult( + filename=safe_filename, + content="", + content_type=normalized_content_type, + parse_content="", + parse_content_type=parse_content_type, + parser_key=deferred_descriptor.parser_key, + parse_status=deferred_descriptor.parse_status, + parse_error_code=None, + ) + if parse_content_type not in _SUPPORTED_CONTENT_TYPES: parser_key = _parser_key_for( parse_content_type, diff --git a/backend/services/content_graph/__init__.py b/backend/services/content_graph/__init__.py index c210282d3..917e90a27 100644 --- a/backend/services/content_graph/__init__.py +++ b/backend/services/content_graph/__init__.py @@ -1,9 +1,11 @@ -from .models import ContentNode, ContentSegment, ParseResult -from .parser import parse_content +from .models import ContentNode, ContentSegment, ParseResult, PdfDomSection +from .parser import parse_content, parse_pdf_dom __all__ = [ "ContentNode", "ContentSegment", "ParseResult", + "PdfDomSection", "parse_content", + "parse_pdf_dom", ] diff --git a/backend/services/content_graph/models.py b/backend/services/content_graph/models.py index 22c6bd64b..80c760ab1 100644 --- a/backend/services/content_graph/models.py +++ b/backend/services/content_graph/models.py @@ -1,6 +1,6 @@ from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, field @dataclass(frozen=True, slots=True) @@ -32,6 +32,20 @@ class ContentSegment: word_count: int +@dataclass(frozen=True, slots=True) +class PdfDomSection: + """A normalized section of a recognized PDF DOM tree. + + One section maps to a single NewsDOM article (its ``headline`` becomes the + section heading and each entry in ``paragraphs`` becomes a paragraph leaf). + ``page_number`` is retained for stable ordering and provenance. + """ + + heading: str + paragraphs: tuple[str, ...] = field(default_factory=tuple) + page_number: int | None = None + + @dataclass(frozen=True, slots=True) class ParseResult: source_kind: str diff --git a/backend/services/content_graph/parser.py b/backend/services/content_graph/parser.py index 14d535399..ec8f51e50 100644 --- a/backend/services/content_graph/parser.py +++ b/backend/services/content_graph/parser.py @@ -8,7 +8,9 @@ from services.text_safety import strip_html_markup -from .models import ContentNode, ContentSegment, ParseResult +from collections.abc import Sequence + +from .models import ContentNode, ContentSegment, ParseResult, PdfDomSection _BLANK_LINE_RE = re.compile(r"\n\s*\n+") @@ -302,6 +304,84 @@ def parse_content( return _parse_plain_text(context, document_node, strip_html_markup(content)) +def parse_pdf_dom( + *, + source_kind: str, + source_record_uid: str, + sections: Sequence[PdfDomSection], + source_content_hash: str, + display_name: str = "", + content_type: str = "application/pdf", +) -> ParseResult: + """Build a document -> section -> paragraph content graph from a recognized + PDF DOM tree. + + Each :class:`PdfDomSection` becomes a ``section`` node under the document + root: its heading is emitted as a ``heading`` segment and every non-empty + paragraph becomes a ``paragraph`` node + segment. ``source_content_hash`` is + supplied by the caller so the node/segment UIDs stay stable and tied to the + exact recognized payload rather than a re-serialization of it. + """ + context = _BuildContext( + source_kind=source_kind, + source_record_uid=source_record_uid, + display_name=display_name, + content_type=_normalize_content_type(content_type), + source_content_hash=source_content_hash, + ) + document_node = _add_document_node(context, display_name) + + section_index = 0 + for section in sections: + heading_text = _safe_text(section.heading) + paragraph_texts = [ + safe_paragraph + for paragraph in section.paragraphs + if (safe_paragraph := _safe_text(paragraph)) + ] + if not heading_text and not paragraph_texts: + continue + + section_index += 1 + section_path = f"/document[1]/section[{section_index}]" + section_node = context.add_node( + parent_node_uid=document_node.content_node_uid, + node_kind="section", + node_path=section_path, + ordinal_index=section_index, + safe_text_content=heading_text, + display_label=heading_text or None, + ) + heading_path = _join_heading_path([heading_text]) if heading_text else None + if heading_text: + context.add_segment( + content_node_uid=section_node.content_node_uid, + segment_kind="heading", + segment_path=f"{section_path}/heading[1]", + heading_path=heading_path, + safe_text_content=heading_text, + ) + + for paragraph_index, paragraph_text in enumerate(paragraph_texts, start=1): + paragraph_path = f"{section_path}/paragraph[{paragraph_index}]" + paragraph_node = context.add_node( + parent_node_uid=section_node.content_node_uid, + node_kind="paragraph", + node_path=paragraph_path, + ordinal_index=paragraph_index, + safe_text_content=paragraph_text, + ) + context.add_segment( + content_node_uid=paragraph_node.content_node_uid, + segment_kind="paragraph", + segment_path=paragraph_path, + heading_path=heading_path, + safe_text_content=paragraph_text, + ) + + return context.result() + + def _parse_plain_text( context: _BuildContext, document_node: ContentNode, diff --git a/backend/services/newsdom_client.py b/backend/services/newsdom_client.py new file mode 100644 index 000000000..f197eea71 --- /dev/null +++ b/backend/services/newsdom_client.py @@ -0,0 +1,425 @@ +"""SSRF-safe client for the NewsDOM PDF DOM recognition sidecar. + +This mirrors :mod:`services.llm_provider_urls`: the configured base URL is +validated against a dedicated allowlist (``ALLOWED_NEWSDOM_HOSTS``), the +hostname is resolved to concrete IP addresses, every address is checked to be +globally routable (unless ``ALLOW_LOCAL_NEWSDOM_PROVIDERS`` is set for docker / +loopback development), and the outbound connection is *pinned* to those +validated addresses so a DNS-rebind between validation and connect cannot +redirect the request to an internal host. + +The base URL and bearer token are always supplied by the caller from the +database (see :class:`db.models.NewsdomProvider`) — this module never reads +service configuration or secrets from the environment at request time. +""" + +from __future__ import annotations + +import asyncio +import ipaddress +import socket +from dataclasses import dataclass +from urllib.parse import SplitResult, urlsplit, urlunsplit + +import httpcore +import httpx +from httpcore._backends.auto import AutoBackend +from httpx._config import DEFAULT_LIMITS, create_ssl_context +from httpx._transports.default import AsyncResponseStream, map_httpcore_exceptions + +from core.config import settings + +NEWSDOM_BASE_URL_NOT_ALLOWED = "NewsDOM base URL is not allowed" +_DNS_RESOLUTION_TIMEOUT_SECONDS = 5.0 +_LOCAL_DEV_HOSTNAMES = {"localhost", "localhost.localdomain"} +_LOCAL_DEV_IP_LITERALS = {"127.0.0.1", "::1"} +_DEFAULT_PARSE_TIMEOUT_SECONDS = 300.0 + + +class NewsdomConfigurationError(RuntimeError): + """Raised when the NewsDOM sidecar is not usably configured.""" + + +class NewsdomRequestError(RuntimeError): + """Raised when the NewsDOM sidecar cannot fulfil a parse request.""" + + +@dataclass(frozen=True) +class ValidatedNewsdomBaseURL: + normalized_url: str + hostname: str + port: int + addresses: tuple[str, ...] + + +def _has_url_control_character(value: str) -> bool: + return any(ord(character) < 32 or ord(character) == 127 for character in value) + + +def _parse_allowed_hosts() -> set[str]: + return { + item.strip().lower().rstrip(".") + for item in settings.ALLOWED_NEWSDOM_HOSTS.split(",") + if item.strip() + } + + +def _is_ip_literal(candidate: str) -> bool: + try: + ipaddress.ip_address(candidate) + except ValueError: + return False + return True + + +def _looks_like_ip_literal(candidate: str) -> bool: + compact_candidate = candidate.replace(".", "").lower() + return ( + ":" in candidate + or compact_candidate.isdigit() + or compact_candidate.startswith("0x") + ) + + +def _is_local_dev_host(hostname: str) -> bool: + normalized_hostname = hostname.lower().rstrip(".") + return ( + normalized_hostname in _LOCAL_DEV_HOSTNAMES + or normalized_hostname in _LOCAL_DEV_IP_LITERALS + ) + + +def _is_allowlisted_local_host(hostname: str) -> bool: + """A bare docker-container name (e.g. ``newsdom``) that is explicitly + allowlisted while local providers are enabled.""" + normalized_hostname = hostname.lower().rstrip(".") + return ( + settings.ALLOW_LOCAL_NEWSDOM_PROVIDERS + and normalized_hostname in _parse_allowed_hosts() + and "." not in normalized_hostname + and not _is_ip_literal(normalized_hostname) + and not _looks_like_ip_literal(normalized_hostname) + ) + + +def _format_normalized_netloc(hostname: str, port: int, *, explicit_port: bool) -> str: + host_part = f"[{hostname}]" if ":" in hostname else hostname + if not explicit_port: + return host_part + return f"{host_part}:{port}" + + +def _validate_global_address(address: str, *, hostname: str | None = None) -> str: + try: + ip_address = ipaddress.ip_address(address) + except ValueError as exc: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) from exc + + is_allowed_local = False + if settings.ALLOW_LOCAL_NEWSDOM_PROVIDERS: + if ip_address.is_loopback: + is_allowed_local = True + elif hostname and _is_allowlisted_local_host(hostname): + is_allowed_local = True + + if not is_allowed_local: + if ( + ip_address.is_private + or ip_address.is_loopback + or ip_address.is_link_local + or ip_address.is_reserved + or ip_address.is_unspecified + or ip_address.is_multicast + or not ip_address.is_global + ): + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + return str(ip_address) + + +def _resolve_all_global_addresses(hostname: str, port: int) -> tuple[str, ...]: + try: + address_infos = socket.getaddrinfo(hostname, port, type=socket.SOCK_STREAM) + except socket.gaierror as exc: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) from exc + + if not address_infos: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + addresses: list[str] = [] + seen_addresses: set[str] = set() + for address_info in address_infos: + address = _validate_global_address(str(address_info[4][0]), hostname=hostname) + if address not in seen_addresses: + seen_addresses.add(address) + addresses.append(address) + return tuple(addresses) + + +async def _resolve_all_global_addresses_async( + hostname: str, port: int +) -> tuple[str, ...]: + try: + return await asyncio.wait_for( + asyncio.to_thread(_resolve_all_global_addresses, hostname, port), + timeout=_DNS_RESOLUTION_TIMEOUT_SECONDS, + ) + except asyncio.TimeoutError as exc: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) from exc + + +def _parse_and_validate_candidate_url( + value: str | None, +) -> tuple[SplitResult | None, int | None]: + if value is None: + return None, None + candidate = value.strip() + if not candidate: + return None, None + if "\\" in candidate or _has_url_control_character(candidate): + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + try: + parsed = urlsplit(candidate) + default_port = 443 if parsed.scheme.lower() == "https" else 80 + port = parsed.port or default_port + return parsed, port + except ValueError as exc: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) from exc + + +def _validate_url_components(parsed, hostname: str, is_local_dev_host: bool) -> None: + if parsed.scheme.lower() not in {"http", "https"}: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + if ( + parsed.scheme.lower() == "http" + and not is_local_dev_host + and not _is_allowlisted_local_host(hostname) + ): + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + if ( + not hostname + or parsed.username is not None + or parsed.password is not None + or parsed.query + or parsed.fragment + ): + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + + +def _validate_remote_host_is_allowed(hostname: str) -> None: + allowed_hosts = _parse_allowed_hosts() + if not allowed_hosts or any("*" in allowed_host for allowed_host in allowed_hosts): + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + if hostname not in allowed_hosts: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + if _is_ip_literal(hostname) or _looks_like_ip_literal(hostname): + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + + +def _normalize_newsdom_base_url(value: str | None): + parsed, port = _parse_and_validate_candidate_url(value) + if parsed is None or port is None: + return None, None, None + + hostname = (parsed.hostname or "").lower().rstrip(".") + is_local_dev_host = _is_local_dev_host(hostname) + _validate_url_components(parsed, hostname, is_local_dev_host) + + # localhost / loopback is only usable when local providers are enabled. + if is_local_dev_host and not settings.ALLOW_LOCAL_NEWSDOM_PROVIDERS: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + if not is_local_dev_host: + _validate_remote_host_is_allowed(hostname) + + netloc = _format_normalized_netloc( + hostname, port, explicit_port=parsed.port is not None + ) + return ( + urlunsplit((parsed.scheme.lower(), netloc, parsed.path or "", "", "")), + hostname, + port, + ) + + +async def validate_newsdom_base_url_details_async( + value: str | None, +) -> ValidatedNewsdomBaseURL | None: + normalized_url, hostname, port = _normalize_newsdom_base_url(value) + if normalized_url is None: + return None + addresses = await _resolve_all_global_addresses_async(hostname, port) + return ValidatedNewsdomBaseURL(normalized_url, hostname, port, addresses) + + +class _PinnedNewsdomNetworkBackend(httpcore.AsyncNetworkBackend): + def __init__(self, hostname: str, port: int, addresses: tuple[str, ...]): + if not addresses: + raise ValueError(NEWSDOM_BASE_URL_NOT_ALLOWED) + self._hostname = hostname + self._port = port + self._addresses = tuple( + _validate_global_address(address, hostname=hostname) + for address in addresses + ) + self._backend = AutoBackend() + + def _verify_host_port(self, host: str | bytes, port: int) -> None: + host_text = host.decode("ascii") if isinstance(host, bytes) else str(host) + normalized_host = host_text.lower().rstrip(".") + if normalized_host != self._hostname or int(port) != self._port: + raise OSError("NewsDOM base URL host changed after validation") + + async def connect_tcp( + self, + host: str | bytes, + port: int, + timeout: float | None = None, + local_address: str | None = None, + socket_options=None, + ): + self._verify_host_port(host, port) + last_error: Exception | None = None + for address in self._addresses: + pinned_address = _validate_global_address(address, hostname=self._hostname) + try: + return await self._backend.connect_tcp( + pinned_address, + port, + timeout=timeout, + local_address=local_address, + socket_options=socket_options, + ) + except Exception as exc: # pragma: no cover - backend-specific + last_error = exc + if last_error is not None: + raise last_error + raise OSError(NEWSDOM_BASE_URL_NOT_ALLOWED) + + async def connect_unix_socket( + self, + path: str, + timeout: float | None = None, + socket_options=None, + ): + raise OSError("NewsDOM base URL must not use Unix sockets") + + async def sleep(self, seconds: float) -> None: + await self._backend.sleep(seconds) + + +class _PinnedNewsdomAsyncTransport(httpx.AsyncBaseTransport): + def __init__(self, validated: ValidatedNewsdomBaseURL): + self._validated = validated + ssl_context = create_ssl_context(verify=True, trust_env=False) + self._pool = httpcore.AsyncConnectionPool( + ssl_context=ssl_context, + max_connections=DEFAULT_LIMITS.max_connections, + max_keepalive_connections=DEFAULT_LIMITS.max_keepalive_connections, + keepalive_expiry=DEFAULT_LIMITS.keepalive_expiry, + http1=True, + http2=False, + network_backend=_PinnedNewsdomNetworkBackend( + validated.hostname, + validated.port, + validated.addresses, + ), + ) + + async def handle_async_request(self, request: httpx.Request) -> httpx.Response: + parsed_url = urlsplit(self._validated.normalized_url) + validated_scheme = parsed_url.scheme.encode("ascii") + validated_host = self._validated.hostname.encode("ascii") + validated_netloc = parsed_url.netloc.encode("ascii") + + safe_headers = [ + (key, value) + for key, value in request.headers.raw + if key.lower() != b"host" + ] + safe_headers.append((b"host", validated_netloc)) + + req = httpcore.Request( + method=request.method, + url=httpcore.URL( + scheme=validated_scheme, + host=validated_host, + port=self._validated.port, + target=request.url.raw_path, + ), + headers=safe_headers, + content=request.stream, + extensions=request.extensions, + ) + with map_httpcore_exceptions(): + resp = await self._pool.handle_async_request(req) + + return httpx.Response( + status_code=resp.status, + headers=resp.headers, + stream=AsyncResponseStream(resp.stream), + extensions=resp.extensions, + ) + + async def aclose(self) -> None: + await self._pool.aclose() + + +def _joined_url(base_url: str, path: str) -> str: + return f"{base_url.rstrip('/')}/{path.lstrip('/')}" + + +async def request_pdf_dom( + *, + base_url: str | None, + api_token: str | None, + pdf_bytes: bytes, + filename: str = "document.pdf", + language: str = "auto", + mode: str = "auto", + timeout_seconds: float = _DEFAULT_PARSE_TIMEOUT_SECONDS, +) -> dict: + """POST the PDF bytes to ``{base_url}/parse`` and return the parsed DOM. + + Targets the generalized NewsDOM ``/parse`` contract: a multipart request + carrying the PDF under ``file`` plus ``language`` / ``mode`` form fields and + an optional ``Authorization: Bearer`` header. Unknown extra form fields are + ignored by the current sidecar, keeping this forward-compatible. + """ + if not pdf_bytes: + raise NewsdomRequestError("Cannot recognize an empty PDF payload") + + validated = await validate_newsdom_base_url_details_async(base_url) + if validated is None: + raise NewsdomConfigurationError( + "NewsDOM base URL is not configured for this workspace" + ) + + headers: dict[str, str] = {} + if api_token and api_token.strip(): + headers["Authorization"] = f"Bearer {api_token.strip()}" + + files = {"file": (filename or "document.pdf", pdf_bytes, "application/pdf")} + data = {"language": language or "auto", "mode": mode or "auto"} + + async with httpx.AsyncClient( + follow_redirects=False, + trust_env=False, + timeout=timeout_seconds, + transport=_PinnedNewsdomAsyncTransport(validated), + ) as client: + try: + response = await client.post( + _joined_url(validated.normalized_url, "/parse"), + files=files, + data=data, + headers=headers, + ) + except httpx.HTTPError as exc: + raise NewsdomRequestError(f"NewsDOM request failed: {exc}") from exc + + if response.status_code >= 400: + raise NewsdomRequestError( + f"NewsDOM returned HTTP {response.status_code} for /parse" + ) + try: + return response.json() + except ValueError as exc: + raise NewsdomRequestError("NewsDOM returned a non-JSON response") from exc diff --git a/backend/services/newsdom_pdf_recognition.py b/backend/services/newsdom_pdf_recognition.py new file mode 100644 index 000000000..6737e58b1 --- /dev/null +++ b/backend/services/newsdom_pdf_recognition.py @@ -0,0 +1,221 @@ +"""Map a recognized NewsDOM PDF into naruon's content graph. + +The NewsDOM sidecar returns a ``pages -> articles -> body_blocks`` tree (see the +``ParseResponse`` schema in newsdom-api). This module normalizes that tree into: + +* ``parse_text`` — a flat text rendering used for attachment / document + embeddings, and +* a :class:`~services.content_graph.ParseResult` — a document -> section -> + paragraph :class:`ContentNode` / :class:`ContentSegment` tree. + +Provider configuration (base URL + bearer token) is always resolved from the +database (:class:`db.models.NewsdomProvider`); this module never reads service +config or secrets from the environment. +""" + +from __future__ import annotations + +import hashlib +from collections.abc import Awaitable, Callable +from dataclasses import dataclass + +from sqlalchemy import desc, select +from sqlalchemy.ext.asyncio import AsyncSession + +from db.models import NewsdomProvider +from services.content_graph import ParseResult, PdfDomSection, parse_pdf_dom +from services.newsdom_client import ( + NewsdomConfigurationError, + request_pdf_dom, +) + +PDF_DOM_RECOGNITION_PENDING_STATUS = "pdf_dom_recognition_pending" +PDF_DOM_RECOGNITION_PARSED_STATUS = "parsed" +PDF_PARSER_KEY = "pdf" +PDF_PARSE_CONTENT_TYPE = "text/plain" + + +@dataclass(frozen=True) +class NewsdomRuntimeConfig: + base_url: str + api_token: str | None + request_language: str + recognition_mode: str + provider_name: str + + +@dataclass(frozen=True) +class PdfDomRecognitionRecords: + parse_text: str + source_content_hash: str + parse_result: ParseResult + + +ParseRequestFn = Callable[..., Awaitable[dict]] + + +def resolve_newsdom_runtime_config( + provider: NewsdomProvider | None, +) -> NewsdomRuntimeConfig | None: + """Build a runtime config purely from a database row — never the env. + + Returns ``None`` when the provider is missing, inactive, or has no base URL, + which is the signal that PDF DOM recognition should stay pending / degrade + gracefully rather than raise. + """ + if provider is None or not provider.is_active: + return None + base_url = (provider.base_url or "").strip() + if not base_url: + return None + api_token = provider.api_token.strip() if provider.api_token else None + return NewsdomRuntimeConfig( + base_url=base_url, + api_token=api_token or None, + request_language=(provider.request_language or "auto").strip() or "auto", + recognition_mode=(provider.recognition_mode or "auto").strip() or "auto", + provider_name=provider.provider_name, + ) + + +async def get_active_newsdom_provider( + session: AsyncSession, + organization_id: str | None, +) -> NewsdomProvider | None: + if not organization_id: + return None + result = await session.execute( + select(NewsdomProvider) + .where( + NewsdomProvider.organization_id == organization_id, + NewsdomProvider.is_active.is_(True), + ) + .order_by(desc(NewsdomProvider.updated_at), desc(NewsdomProvider.id)) + .limit(1) + ) + return result.scalars().first() + + +async def resolve_newsdom_config_from_db( + session: AsyncSession, + organization_id: str | None, +) -> NewsdomRuntimeConfig | None: + provider = await get_active_newsdom_provider(session, organization_id) + return resolve_newsdom_runtime_config(provider) + + +def normalize_parse_response(payload: dict) -> list[PdfDomSection]: + """Flatten a NewsDOM ``ParseResponse`` dict into ordered sections. + + Each article (across every page, in page then article order) becomes one + section. Robust to missing / malformed keys so a partial sidecar response + never crashes the importer. + """ + sections: list[PdfDomSection] = [] + pages = payload.get("pages") if isinstance(payload, dict) else None + if not isinstance(pages, list): + return sections + + for page in pages: + if not isinstance(page, dict): + continue + page_number = page.get("page_number") + page_number = page_number if isinstance(page_number, int) else None + articles = page.get("articles") + if not isinstance(articles, list): + continue + for article in articles: + if not isinstance(article, dict): + continue + headline = article.get("headline") + headline = headline if isinstance(headline, str) else "" + body_blocks = article.get("body_blocks") + paragraphs = tuple( + block + for block in (body_blocks if isinstance(body_blocks, list) else []) + if isinstance(block, str) and block.strip() + ) + if not headline.strip() and not paragraphs: + continue + sections.append( + PdfDomSection( + heading=headline, + paragraphs=paragraphs, + page_number=page_number, + ) + ) + return sections + + +def _render_parse_text(sections: list[PdfDomSection]) -> str: + blocks: list[str] = [] + for section in sections: + if section.heading.strip(): + blocks.append(section.heading.strip()) + blocks.extend( + paragraph.strip() for paragraph in section.paragraphs if paragraph.strip() + ) + return "\n\n".join(blocks) + + +def build_recognition_records( + payload: dict, + *, + source_kind: str, + source_record_uid: str, + display_name: str = "", +) -> PdfDomRecognitionRecords: + sections = normalize_parse_response(payload) + parse_text = _render_parse_text(sections) + source_content_hash = hashlib.sha256( + parse_text.encode("utf-8", errors="surrogatepass") + ).hexdigest() + parse_result = parse_pdf_dom( + source_kind=source_kind, + source_record_uid=source_record_uid, + sections=sections, + source_content_hash=source_content_hash, + display_name=display_name, + ) + return PdfDomRecognitionRecords( + parse_text=parse_text, + source_content_hash=source_content_hash, + parse_result=parse_result, + ) + + +async def recognize_pdf_dom( + *, + config: NewsdomRuntimeConfig | None, + pdf_bytes: bytes, + filename: str, + source_kind: str, + source_record_uid: str, + display_name: str = "", + request_fn: ParseRequestFn = request_pdf_dom, +) -> PdfDomRecognitionRecords: + """Call the sidecar and map the response into content graph records. + + ``request_fn`` is injectable so callers (and tests) can supply a mocked + NewsDOM client. Raises :class:`NewsdomConfigurationError` when the sidecar is + not configured, so the caller can keep the source pending instead of failing + the whole import. + """ + if config is None: + raise NewsdomConfigurationError( + "NewsDOM PDF DOM recognition is not configured for this workspace" + ) + payload = await request_fn( + base_url=config.base_url, + api_token=config.api_token, + pdf_bytes=pdf_bytes, + filename=filename, + language=config.request_language, + mode=config.recognition_mode, + ) + return build_recognition_records( + payload, + source_kind=source_kind, + source_record_uid=source_record_uid, + display_name=display_name, + ) diff --git a/backend/services/newsdom_worker.py b/backend/services/newsdom_worker.py new file mode 100644 index 000000000..5b7748616 --- /dev/null +++ b/backend/services/newsdom_worker.py @@ -0,0 +1,149 @@ +"""Background worker glue for NewsDOM PDF DOM recognition. + +Attachments and workspace documents whose PDF recognition was deferred at +import time are processed here: the sidecar is called (via +:mod:`services.newsdom_pdf_recognition`) and the returned tree is landed into +``parse_content`` (for embeddings) and, for attachments, the +``content_nodes`` / ``content_segments`` graph. + +The apply functions are deliberately session-free so they can be unit tested +with in-memory model instances and a mocked NewsDOM client. +""" + +from __future__ import annotations + +from db.models import ( + Attachment, + ContentNodeRecord, + ContentSegmentRecord, + Document, + Email, +) +from services.content_graph import ParseResult +from services.newsdom_client import request_pdf_dom +from services.newsdom_pdf_recognition import ( + PDF_DOM_RECOGNITION_PARSED_STATUS, + PDF_PARSE_CONTENT_TYPE, + PDF_PARSER_KEY, + NewsdomRuntimeConfig, + ParseRequestFn, + PdfDomRecognitionRecords, + recognize_pdf_dom, +) + + +def _append_parse_result_to_attachment( + *, + email: Email, + attachment: Attachment, + parse_result: ParseResult, +) -> None: + node_records_by_uid: dict[str, ContentNodeRecord] = {} + for parsed_node in parse_result.nodes: + node_record = ContentNodeRecord( + content_node_uid=parsed_node.content_node_uid, + source_kind=parsed_node.source_kind, + source_record_uid=parsed_node.source_record_uid, + parent_node_uid=parsed_node.parent_node_uid, + node_kind=parsed_node.node_kind, + node_path=parsed_node.node_path, + ordinal_index=parsed_node.ordinal_index, + display_label=parsed_node.display_label, + safe_text_content=parsed_node.safe_text_content, + content_hash=parsed_node.content_hash, + ) + email.content_nodes.append(node_record) + attachment.content_nodes.append(node_record) + node_records_by_uid[parsed_node.content_node_uid] = node_record + + for parsed_segment in parse_result.segments: + node_record = node_records_by_uid.get(parsed_segment.content_node_uid) + segment_record = ContentSegmentRecord( + content_segment_uid=parsed_segment.content_segment_uid, + source_kind=parsed_segment.source_kind, + source_record_uid=parsed_segment.source_record_uid, + segment_kind=parsed_segment.segment_kind, + segment_path=parsed_segment.segment_path, + ordinal_index=parsed_segment.ordinal_index, + heading_path=parsed_segment.heading_path, + safe_text_content=parsed_segment.safe_text_content, + content_hash=parsed_segment.content_hash, + word_count=parsed_segment.word_count, + ) + if node_record is not None: + node_record.segments.append(segment_record) + email.content_segments.append(segment_record) + attachment.content_segments.append(segment_record) + + +def apply_recognition_to_attachment( + *, + email: Email, + attachment: Attachment, + records: PdfDomRecognitionRecords, +) -> None: + """Land recognized PDF DOM records onto an attachment (text + graph).""" + attachment.parse_content = records.parse_text + attachment.content = records.parse_text + attachment.parse_content_type = PDF_PARSE_CONTENT_TYPE + attachment.parser_key = PDF_PARSER_KEY + attachment.parse_status = PDF_DOM_RECOGNITION_PARSED_STATUS + attachment.parse_error_code = None + _append_parse_result_to_attachment( + email=email, + attachment=attachment, + parse_result=records.parse_result, + ) + + +def apply_recognition_to_document( + *, + document: Document, + records: PdfDomRecognitionRecords, +) -> None: + """Land recognized PDF text onto a workspace document (mirrors the HWP + conversion worker: content + status, no content graph rows).""" + document.document_content = records.parse_text + document.document_status = PDF_DOM_RECOGNITION_PARSED_STATUS + + +async def recognize_attachment_pdf( + *, + email: Email, + attachment: Attachment, + pdf_bytes: bytes, + config: NewsdomRuntimeConfig | None, + source_record_uid: str, + request_fn: ParseRequestFn = request_pdf_dom, +) -> PdfDomRecognitionRecords: + records = await recognize_pdf_dom( + config=config, + pdf_bytes=pdf_bytes, + filename=attachment.filename or "attachment.pdf", + source_kind="attachment", + source_record_uid=source_record_uid, + display_name=attachment.filename or "", + request_fn=request_fn, + ) + apply_recognition_to_attachment(email=email, attachment=attachment, records=records) + return records + + +async def recognize_document_pdf( + *, + document: Document, + pdf_bytes: bytes, + config: NewsdomRuntimeConfig | None, + request_fn: ParseRequestFn = request_pdf_dom, +) -> PdfDomRecognitionRecords: + records = await recognize_pdf_dom( + config=config, + pdf_bytes=pdf_bytes, + filename=document.document_name or "document.pdf", + source_kind="workspace_document", + source_record_uid=document.document_id, + display_name=document.document_name or "", + request_fn=request_fn, + ) + apply_recognition_to_document(document=document, records=records) + return records diff --git a/backend/tests/test_attachment_parser.py b/backend/tests/test_attachment_parser.py index e7c92013a..c06afb6a9 100644 --- a/backend/tests/test_attachment_parser.py +++ b/backend/tests/test_attachment_parser.py @@ -40,7 +40,13 @@ def test_parser_manifest_lists_supported_and_unsupported_format_families(): manifest = get_attachment_parser_manifest() parser_keys = {descriptor.parser_key for descriptor in manifest} - assert {"plain_text", "html", "markdown", "unsupported_binary"} <= parser_keys + assert { + "plain_text", + "html", + "markdown", + "pdf", + "unsupported_binary", + } <= parser_keys markdown_descriptor = next( descriptor for descriptor in manifest if descriptor.parser_key == "markdown" ) @@ -78,6 +84,23 @@ def test_oversized_text_attachment_is_metadata_only_without_raw_content(): def test_unsupported_binary_attachment_is_visible_without_raw_bytes(): + result = parse_email_attachment( + filename="archive.zip", + content_type="application/zip", + raw_content=b"PK\x03\x04 raw bytes", + ) + + assert result.filename == "archive.zip" + assert result.content_type == "application/zip" + assert result.content == "" + assert result.parse_content == "" + assert result.parse_content_type == "application/zip" + assert result.parser_key == "unsupported_binary" + assert result.parse_status == "unsupported_content_type" + assert result.parse_error_code == "unsupported_content_type" + + +def test_pdf_attachment_is_deferred_pending_newsdom_recognition(): result = parse_email_attachment( filename="contract.pdf", content_type="application/pdf", @@ -86,9 +109,23 @@ def test_unsupported_binary_attachment_is_visible_without_raw_bytes(): assert result.filename == "contract.pdf" assert result.content_type == "application/pdf" + # Heavy OCR/MinerU recognition is deferred to the worker: nothing is parsed + # inline, and the attachment carries the pending status. assert result.content == "" assert result.parse_content == "" assert result.parse_content_type == "application/pdf" - assert result.parser_key == "unsupported_binary" - assert result.parse_status == "unsupported_content_type" - assert result.parse_error_code == "unsupported_content_type" + assert result.parser_key == "pdf" + assert result.parse_status == "pdf_dom_recognition_pending" + assert result.parse_error_code is None + + +def test_pdf_extension_with_generic_content_type_is_deferred_pending(): + result = parse_email_attachment( + filename="contract.pdf", + content_type="application/octet-stream", + raw_content=b"%PDF-1.7 raw bytes", + ) + + assert result.parse_content_type == "application/pdf" + assert result.parser_key == "pdf" + assert result.parse_status == "pdf_dom_recognition_pending" diff --git a/backend/tests/test_email_parser.py b/backend/tests/test_email_parser.py index 9f8894941..1efefd4d3 100644 --- a/backend/tests/test_email_parser.py +++ b/backend/tests/test_email_parser.py @@ -215,9 +215,11 @@ def test_parse_eml_extracts_supported_and_unsupported_attachment_metadata(): "content_type": "application/pdf", "parse_content": "", "parse_content_type": "application/pdf", - "parser_key": "unsupported_binary", - "parse_status": "unsupported_content_type", - "parse_error_code": "unsupported_content_type", + # PDFs are deferred to the NewsDOM recognition worker rather than + # parsed inline, so they arrive pending (not unsupported). + "parser_key": "pdf", + "parse_status": "pdf_dom_recognition_pending", + "parse_error_code": None, }, ] finally: diff --git a/backend/tests/test_newsdom_client.py b/backend/tests/test_newsdom_client.py new file mode 100644 index 000000000..8325bffd3 --- /dev/null +++ b/backend/tests/test_newsdom_client.py @@ -0,0 +1,108 @@ +"""SSRF / allowlist tests for the NewsDOM sidecar client. + +These exercise the pure URL-normalization layer (no DNS / no network) plus the +request-time configuration guards. +""" + +import pytest + +from core.config import settings +from services import newsdom_client +from services.newsdom_client import ( + NEWSDOM_BASE_URL_NOT_ALLOWED, + NewsdomConfigurationError, + NewsdomRequestError, + _normalize_newsdom_base_url, + request_pdf_dom, +) + + +@pytest.fixture +def newsdom_allowlist(monkeypatch): + monkeypatch.setattr(settings, "ALLOWED_NEWSDOM_HOSTS", "newsdom.example.com") + monkeypatch.setattr(settings, "ALLOW_LOCAL_NEWSDOM_PROVIDERS", False) + return settings + + +def test_allowlisted_https_host_is_normalized(newsdom_allowlist): + normalized, hostname, port = _normalize_newsdom_base_url( + "https://newsdom.example.com/parse-root/" + ) + assert hostname == "newsdom.example.com" + assert port == 443 + assert normalized.startswith("https://newsdom.example.com") + + +def test_host_not_in_allowlist_is_rejected(newsdom_allowlist): + with pytest.raises(ValueError) as excinfo: + _normalize_newsdom_base_url("https://evil.example.com") + assert str(excinfo.value) == NEWSDOM_BASE_URL_NOT_ALLOWED + + +def test_plain_http_remote_host_is_rejected(newsdom_allowlist): + # Even an allowlisted host may not be reached over plain http when local + # providers are disabled. + with pytest.raises(ValueError): + _normalize_newsdom_base_url("http://newsdom.example.com") + + +def test_ip_literal_host_is_rejected(newsdom_allowlist): + with pytest.raises(ValueError): + _normalize_newsdom_base_url("https://169.254.169.254") + + +def test_userinfo_is_rejected(newsdom_allowlist): + with pytest.raises(ValueError): + _normalize_newsdom_base_url("https://user:pass@newsdom.example.com") + + +def test_localhost_rejected_unless_local_providers_enabled(monkeypatch): + monkeypatch.setattr(settings, "ALLOWED_NEWSDOM_HOSTS", "newsdom") + monkeypatch.setattr(settings, "ALLOW_LOCAL_NEWSDOM_PROVIDERS", False) + with pytest.raises(ValueError): + _normalize_newsdom_base_url("http://localhost:8000") + + +def test_docker_container_host_allowed_when_local_enabled(monkeypatch): + monkeypatch.setattr(settings, "ALLOWED_NEWSDOM_HOSTS", "newsdom") + monkeypatch.setattr(settings, "ALLOW_LOCAL_NEWSDOM_PROVIDERS", True) + normalized, hostname, port = _normalize_newsdom_base_url("http://newsdom:8000") + assert hostname == "newsdom" + assert port == 8000 + assert normalized == "http://newsdom:8000" + + +def test_empty_base_url_normalizes_to_none(newsdom_allowlist): + assert _normalize_newsdom_base_url(None) == (None, None, None) + assert _normalize_newsdom_base_url("") == (None, None, None) + + +@pytest.mark.asyncio +async def test_request_pdf_dom_rejects_empty_payload(newsdom_allowlist): + with pytest.raises(NewsdomRequestError): + await request_pdf_dom( + base_url="https://newsdom.example.com", + api_token=None, + pdf_bytes=b"", + ) + + +@pytest.mark.asyncio +async def test_request_pdf_dom_raises_config_error_without_base_url(newsdom_allowlist): + with pytest.raises(NewsdomConfigurationError): + await request_pdf_dom( + base_url=None, + api_token=None, + pdf_bytes=b"%PDF-1.7", + ) + + +@pytest.mark.asyncio +async def test_request_pdf_dom_rejects_disallowed_host(newsdom_allowlist): + with pytest.raises(ValueError) as excinfo: + await request_pdf_dom( + base_url="https://evil.example.com", + api_token=None, + pdf_bytes=b"%PDF-1.7", + ) + assert str(excinfo.value) == NEWSDOM_BASE_URL_NOT_ALLOWED diff --git a/backend/tests/test_newsdom_pdf_recognition.py b/backend/tests/test_newsdom_pdf_recognition.py new file mode 100644 index 000000000..ec914a87e --- /dev/null +++ b/backend/tests/test_newsdom_pdf_recognition.py @@ -0,0 +1,263 @@ +"""Fast, fully-mocked unit tests for NewsDOM PDF DOM recognition. + +No database and no network: the NewsDOM client is replaced with a canned +``ParseResponse`` and the content-graph mapping / config resolution are +exercised against in-memory model instances. +""" + +import os + +import pytest + +from db.models import Attachment, Email, NewsdomProvider +from services.newsdom_pdf_recognition import ( + NewsdomRuntimeConfig, + build_recognition_records, + normalize_parse_response, + recognize_pdf_dom, + resolve_newsdom_runtime_config, +) +from services.newsdom_worker import ( + apply_recognition_to_attachment, + recognize_attachment_pdf, +) + + +def _canned_parse_response() -> dict: + return { + "document_id": "doc-123", + "pages": [ + { + "page_number": 1, + "articles": [ + { + "article_id": "a1", + "headline": "First Headline", + "body_blocks": ["Body one.", "Body two."], + }, + { + "article_id": "a2", + "headline": "Second Headline", + "body_blocks": ["Only body."], + }, + ], + }, + { + "page_number": 2, + "articles": [ + { + "article_id": "a3", + "headline": "", + "body_blocks": [" ", "Third page body."], + } + ], + }, + ], + "quality": {"status": "success", "parser": "mineru", "warnings": []}, + } + + +def test_normalize_parse_response_flattens_articles_in_order(): + sections = normalize_parse_response(_canned_parse_response()) + + assert [section.heading for section in sections] == [ + "First Headline", + "Second Headline", + "", + ] + assert sections[0].paragraphs == ("Body one.", "Body two.") + # Blank body blocks are dropped. + assert sections[2].paragraphs == ("Third page body.",) + assert sections[0].page_number == 1 + assert sections[2].page_number == 2 + + +def test_normalize_parse_response_tolerates_garbage(): + assert normalize_parse_response({}) == [] + assert normalize_parse_response({"pages": "nope"}) == [] + assert normalize_parse_response({"pages": [{"articles": [42, {"headline": 3}]}]}) == [] + + +def test_build_recognition_records_builds_document_section_paragraph_tree(): + records = build_recognition_records( + _canned_parse_response(), + source_kind="attachment", + source_record_uid="att-1", + display_name="news.pdf", + ) + + parse_result = records.parse_result + node_kinds = [node.node_kind for node in parse_result.nodes] + assert node_kinds.count("document") == 1 + assert node_kinds.count("section") == 3 + # 2 + 1 + 1 body paragraphs across the three sections. + assert node_kinds.count("paragraph") == 4 + + document_nodes = [n for n in parse_result.nodes if n.node_kind == "document"] + (document_node,) = document_nodes + assert document_node.parent_node_uid is None + assert document_node.display_label == "news.pdf" + + section_nodes = [n for n in parse_result.nodes if n.node_kind == "section"] + assert all( + n.parent_node_uid == document_node.content_node_uid for n in section_nodes + ) + section_uids = {n.content_node_uid for n in section_nodes} + paragraph_nodes = [n for n in parse_result.nodes if n.node_kind == "paragraph"] + assert all(n.parent_node_uid in section_uids for n in paragraph_nodes) + + segment_kinds = [seg.segment_kind for seg in parse_result.segments] + # Two headlines are non-empty -> two heading segments; the empty headline + # produces no heading segment. + assert segment_kinds.count("heading") == 2 + assert segment_kinds.count("paragraph") == 4 + + # parse_content text (for embeddings) carries every headline + body block. + assert "First Headline" in records.parse_text + assert "Third page body." in records.parse_text + assert records.source_content_hash + + +def test_recognition_uid_is_stable_for_identical_payload(): + payload = _canned_parse_response() + first = build_recognition_records( + payload, source_kind="attachment", source_record_uid="att-1" + ) + second = build_recognition_records( + payload, source_kind="attachment", source_record_uid="att-1" + ) + assert [n.content_node_uid for n in first.parse_result.nodes] == [ + n.content_node_uid for n in second.parse_result.nodes + ] + + +def test_resolve_runtime_config_reads_from_db_row_not_env(monkeypatch): + # Prove the resolver never consults the process environment for config. + def _boom(*_args, **_kwargs): # pragma: no cover - only fails if called + raise AssertionError("config must come from the DB, not os.getenv") + + monkeypatch.setattr(os, "getenv", _boom) + monkeypatch.setattr(os, "environ", {}) + + provider = NewsdomProvider( + user_id="u1", + organization_id="org-1", + provider_name="primary", + base_url="https://newsdom.example.com", + api_token="secret-token", + request_language="ja", + recognition_mode="newspaper", + is_active=True, + ) + config = resolve_newsdom_runtime_config(provider) + assert config == NewsdomRuntimeConfig( + base_url="https://newsdom.example.com", + api_token="secret-token", + request_language="ja", + recognition_mode="newspaper", + provider_name="primary", + ) + + +def test_resolve_runtime_config_degrades_when_unconfigured(): + assert resolve_newsdom_runtime_config(None) is None + inactive = NewsdomProvider( + user_id="u", + organization_id="o", + provider_name="p", + base_url="https://newsdom.example.com", + is_active=False, + ) + assert resolve_newsdom_runtime_config(inactive) is None + no_url = NewsdomProvider( + user_id="u", + organization_id="o", + provider_name="p", + base_url="", + is_active=True, + ) + assert resolve_newsdom_runtime_config(no_url) is None + + +@pytest.mark.asyncio +async def test_recognize_pdf_dom_uses_mocked_client_with_config_values(): + captured = {} + + async def fake_request(**kwargs): + captured.update(kwargs) + return _canned_parse_response() + + config = NewsdomRuntimeConfig( + base_url="https://newsdom.example.com", + api_token="tok", + request_language="ja", + recognition_mode="newspaper", + provider_name="primary", + ) + records = await recognize_pdf_dom( + config=config, + pdf_bytes=b"%PDF-1.7 fake", + filename="news.pdf", + source_kind="attachment", + source_record_uid="att-1", + display_name="news.pdf", + request_fn=fake_request, + ) + + assert captured["base_url"] == "https://newsdom.example.com" + assert captured["api_token"] == "tok" + assert captured["language"] == "ja" + assert captured["mode"] == "newspaper" + assert captured["pdf_bytes"] == b"%PDF-1.7 fake" + assert records.parse_text + assert any(n.node_kind == "section" for n in records.parse_result.nodes) + + +@pytest.mark.asyncio +async def test_recognize_attachment_pdf_lands_text_and_content_graph(): + email = Email() + attachment = Attachment(filename="news.pdf") + email.attachments.append(attachment) + + async def fake_request(**_kwargs): + return _canned_parse_response() + + config = NewsdomRuntimeConfig( + base_url="https://newsdom.example.com", + api_token=None, + request_language="auto", + recognition_mode="auto", + provider_name="primary", + ) + await recognize_attachment_pdf( + email=email, + attachment=attachment, + pdf_bytes=b"%PDF-1.7 fake", + config=config, + source_record_uid="att-1", + request_fn=fake_request, + ) + + assert attachment.parse_status == "parsed" + assert attachment.parser_key == "pdf" + assert "First Headline" in attachment.parse_content + # Content graph landed on both the email and the attachment. + assert any(n.node_kind == "section" for n in attachment.content_nodes) + assert any(n.node_kind == "document" for n in email.content_nodes) + assert attachment.content_segments + assert email.content_segments + + +def test_apply_recognition_to_attachment_is_pure_mapping(): + email = Email() + attachment = Attachment(filename="news.pdf") + email.attachments.append(attachment) + records = build_recognition_records( + _canned_parse_response(), + source_kind="attachment", + source_record_uid="att-1", + display_name="news.pdf", + ) + apply_recognition_to_attachment(email=email, attachment=attachment, records=records) + assert len(attachment.content_nodes) == len(records.parse_result.nodes) + assert len(attachment.content_segments) == len(records.parse_result.segments) diff --git a/docker-compose.yml b/docker-compose.yml index 91b321118..638826e2c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -30,6 +30,22 @@ services: ports: - "11434:11434" + # NewsDOM PDF DOM recognition sidecar, built from the pinned + # vendor/newsdom-api submodule. Optional: the backend degrades gracefully + # (PDFs stay pending) when this service is absent or unconfigured. + newsdom: + build: + context: ./vendor/newsdom-api + dockerfile: Dockerfile + ports: + - "8100:8000" + healthcheck: + test: ["CMD-SHELL", "python -c \"import urllib.request,sys; sys.exit(0 if urllib.request.urlopen('http://localhost:8000/health').status==200 else 1)\""] + interval: 10s + timeout: 5s + retries: 5 + start_period: 20s + backend: build: context: . @@ -41,6 +57,11 @@ services: DEBUG: "false" ALLOW_LOCAL_LLM_PROVIDERS: "true" ALLOWED_LLM_BASE_URL_HOSTS: ollama + # NewsDOM sidecar allowlist. The actual base_url + bearer token are + # configured per-organization in the database (NewsdomProvider), NOT here; + # this only authorizes the container hostname for the SSRF-safe client. + ALLOW_LOCAL_NEWSDOM_PROVIDERS: "true" + ALLOWED_NEWSDOM_HOSTS: newsdom AUTH_SESSION_HMAC_SECRET: ${AUTH_SESSION_HMAC_SECRET} ENCRYPTION_KEY: ${ENCRYPTION_KEY} OPENAI_API_KEY: ollama @@ -52,6 +73,8 @@ services: condition: service_healthy ollama: condition: service_started + newsdom: + condition: service_started ports: - "8000:8000" command: diff --git a/vendor/newsdom-api b/vendor/newsdom-api new file mode 160000 index 000000000..4a68893af --- /dev/null +++ b/vendor/newsdom-api @@ -0,0 +1 @@ +Subproject commit 4a68893afffcaa5828a2966fe5c4d4d539b90541 From f94ce5d296587ad22e6afd022d59653beb222556 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Wed, 8 Jul 2026 22:45:50 +0900 Subject: [PATCH 02/18] fix(pdf-dom): gate newsdom sidecar behind compose profile The coverage-evidence CI gate runs `docker compose config` + `docker compose build` whenever docker-compose.yml changes. The newsdom service builds from context ./vendor/newsdom-api, a submodule that is not checked out in that job, so the build failed with: target newsdom: failed to solve: failed to read dockerfile: open Dockerfile: no such file or directory newsdom is an optional PDF->DOM sidecar; the backend already degrades gracefully when it is absent. Put it behind a 'newsdom' compose profile so the default build/config path skips it, and make the backend's depends_on required: false so config/build does not fail when the profile is inactive (dropped from the dependency graph) while still waiting for the sidecar when the profile is enabled. Bring it up intentionally with: git submodule update --init vendor/newsdom-api docker compose --profile newsdom up Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01RTAMs4bpSZS77Xe3RQjv9P --- docker-compose.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docker-compose.yml b/docker-compose.yml index 638826e2c..516097215 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -33,7 +33,17 @@ services: # NewsDOM PDF DOM recognition sidecar, built from the pinned # vendor/newsdom-api submodule. Optional: the backend degrades gracefully # (PDFs stay pending) when this service is absent or unconfigured. + # + # Gated behind the "newsdom" profile so the default `docker compose build` + # / `config` path (e.g. the coverage-evidence CI gate) never tries to build + # from vendor/newsdom-api, which is a submodule that is not checked out in + # every environment. Bring the sidecar up intentionally with: + # docker compose --profile newsdom up + # (or COMPOSE_PROFILES=newsdom). The submodule must be checked out first: + # git submodule update --init vendor/newsdom-api newsdom: + profiles: + - newsdom build: context: ./vendor/newsdom-api dockerfile: Dockerfile @@ -73,8 +83,14 @@ services: condition: service_healthy ollama: condition: service_started + # Optional sidecar (see the "newsdom" profile above). required: false so + # that when the profile is inactive the dependency is dropped from the + # graph instead of failing config/build; the backend degrades gracefully + # (PDFs stay pending). When the profile is active, the backend still waits + # for the sidecar to start. newsdom: condition: service_started + required: false ports: - "8000:8000" command: From d79c21e37dccf5e4d9642378d14e32289b75d80c Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Thu, 9 Jul 2026 13:44:40 +0900 Subject: [PATCH 03/18] fix(newsdom-test): drop unused import and tighten URL assertion Remove the unused module-level `from services import newsdom_client` import (ruff F401) and replace the `startswith` prefix check with an exact-equality assertion on the normalized URL. The prefix check triggered CodeQL py/incomplete-url-substring-sanitization since it would also accept hosts like newsdom.example.com.evil.com; the deterministic normalizer output lets the test assert the full value instead. --- backend/tests/test_newsdom_client.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/backend/tests/test_newsdom_client.py b/backend/tests/test_newsdom_client.py index 8325bffd3..cda13ec35 100644 --- a/backend/tests/test_newsdom_client.py +++ b/backend/tests/test_newsdom_client.py @@ -7,7 +7,6 @@ import pytest from core.config import settings -from services import newsdom_client from services.newsdom_client import ( NEWSDOM_BASE_URL_NOT_ALLOWED, NewsdomConfigurationError, @@ -30,7 +29,7 @@ def test_allowlisted_https_host_is_normalized(newsdom_allowlist): ) assert hostname == "newsdom.example.com" assert port == 443 - assert normalized.startswith("https://newsdom.example.com") + assert normalized == "https://newsdom.example.com/parse-root/" def test_host_not_in_allowlist_is_rejected(newsdom_allowlist): From 6137169b8b675c09ddd74bd409b29654b45848cb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 12 Jul 2026 06:36:25 +0000 Subject: [PATCH 04/18] =?UTF-8?q?fix(merge):=20resolve=20develop=20conflic?= =?UTF-8?q?ts=20=E2=80=94=20keep=20pdf=20+=20json/csv/xml/calendar=20descr?= =?UTF-8?q?iptors=20and=20ALLOWED=5FNEWSDOM=5FHOSTS=20+=20ALLOWED=5FSCOPEW?= =?UTF-8?q?EAVE=5FHOSTS?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- backend/core/config.py | 3 --- backend/services/attachment_parser.py | 15 +++++++-------- backend/tests/test_attachment_parser.py | 5 +---- 3 files changed, 8 insertions(+), 15 deletions(-) diff --git a/backend/core/config.py b/backend/core/config.py index 277ebfea8..399f1e7f2 100644 --- a/backend/core/config.py +++ b/backend/core/config.py @@ -86,20 +86,17 @@ class Settings(BaseSettings): ALLOWED_POP3_PORTS: str = "995" ALLOWED_LLM_BASE_URL_HOSTS: str = "" ALLOW_LOCAL_LLM_PROVIDERS: bool = False -<<<<<<< HEAD # NewsDOM PDF DOM recognition sidecar. Mirrors the LLM provider allowlist # controls: the base URL host must be listed here before any request is # pinned and dispatched, and container-name / loopback hosts are only # accepted when ALLOW_LOCAL_NEWSDOM_PROVIDERS is enabled (dev / docker). ALLOWED_NEWSDOM_HOSTS: str = "" ALLOW_LOCAL_NEWSDOM_PROVIDERS: bool = False -======= # Host allowlist for the scopeweave promotion target. The per-workspace # base URL and PAT themselves live encrypted in the database # (scopeweave_promotion_target); this setting only pins which hosts an # operator is permitted to promote work items to (SSRF host allowlist). ALLOWED_SCOPEWEAVE_HOSTS: str = "" ->>>>>>> origin/develop ALLOWED_CORS_ORIGINS: str = "" ENABLE_PROMETHEUS_METRICS: bool = False # Best-effort projection of imported-email content segments into the project diff --git a/backend/services/attachment_parser.py b/backend/services/attachment_parser.py index 960378320..175aeca3d 100644 --- a/backend/services/attachment_parser.py +++ b/backend/services/attachment_parser.py @@ -45,13 +45,6 @@ class AttachmentParserDescriptor: parse_status="parsed", ), AttachmentParserDescriptor( -<<<<<<< HEAD - parser_key="pdf", - display_name="PDF documents (NewsDOM recognition)", - content_types=("application/pdf",), - extensions=(".pdf",), - parse_status="pdf_dom_recognition_pending", -======= parser_key="json", display_name="JSON attachments", content_types=("application/json", "text/json"), @@ -78,7 +71,13 @@ class AttachmentParserDescriptor: content_types=("text/calendar",), extensions=(".ics", ".ifb"), parse_status="parsed", ->>>>>>> origin/develop + ), + AttachmentParserDescriptor( + parser_key="pdf", + display_name="PDF documents (NewsDOM recognition)", + content_types=("application/pdf",), + extensions=(".pdf",), + parse_status="pdf_dom_recognition_pending", ), AttachmentParserDescriptor( parser_key="unsupported_binary", diff --git a/backend/tests/test_attachment_parser.py b/backend/tests/test_attachment_parser.py index f53f6e768..40d83e748 100644 --- a/backend/tests/test_attachment_parser.py +++ b/backend/tests/test_attachment_parser.py @@ -44,14 +44,11 @@ def test_parser_manifest_lists_supported_and_unsupported_format_families(): "plain_text", "html", "markdown", -<<<<<<< HEAD - "pdf", -======= "json", "csv", "xml", "calendar", ->>>>>>> origin/develop + "pdf", "unsupported_binary", } <= parser_keys markdown_descriptor = next( From 8f10af21484627324c3e6f847b2d50eeb0a6cc1f Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 11:55:33 +0900 Subject: [PATCH 05/18] Merge Alembic NewsDOM migration heads --- .../0015_merge_newsdom_email_heads.py | 26 +++++++++++++++++++ backend/tests/test_alembic_migrations.py | 19 ++++++++++++++ 2 files changed, 45 insertions(+) create mode 100644 backend/alembic/versions/0015_merge_newsdom_email_heads.py diff --git a/backend/alembic/versions/0015_merge_newsdom_email_heads.py b/backend/alembic/versions/0015_merge_newsdom_email_heads.py new file mode 100644 index 000000000..c856c377d --- /dev/null +++ b/backend/alembic/versions/0015_merge_newsdom_email_heads.py @@ -0,0 +1,26 @@ +"""Merge the NewsDOM provider branch into the unified migration graph. + +Revision ID: 0015_merge_newsdom_email_heads +Revises: 0010_newsdom_providers, 0014_merge_email_read_state +Create Date: 2026-07-13 00:00:00.000000 + +The NewsDOM provider migration and the email read-state merge both descend +from the project graph revision through separate branches. This pure graph +merge restores a single Alembic head without applying additional DDL. +""" + +from __future__ import annotations + +# revision identifiers, used by Alembic. +revision = "0015_merge_newsdom_email_heads" +down_revision = ("0010_newsdom_providers", "0014_merge_email_read_state") +branch_labels = None +depends_on = None + + +def upgrade() -> None: + """Unify the parent revisions without changing the database schema.""" + + +def downgrade() -> None: + """Leave the parent branch schemas intact when removing the merge node.""" diff --git a/backend/tests/test_alembic_migrations.py b/backend/tests/test_alembic_migrations.py index 0bec387f8..073dfd7d5 100644 --- a/backend/tests/test_alembic_migrations.py +++ b/backend/tests/test_alembic_migrations.py @@ -420,3 +420,22 @@ def test_merge_revision_reconciles_email_read_state_branch(): assert "op.create_table(" not in revision_text assert "op.add_column(" not in revision_text assert "op.drop_column(" not in revision_text + + +def test_merge_revision_reconciles_newsdom_provider_branch(): + revision_path = ( + BACKEND_ROOT + / "alembic" + / "versions" + / "0015_merge_newsdom_email_heads.py" + ) + assert revision_path.exists() + revision_text = revision_path.read_text() + + assert 'revision = "0015_merge_newsdom_email_heads"' in revision_text + assert "down_revision = (" in revision_text + assert '"0010_newsdom_providers"' in revision_text + assert '"0014_merge_email_read_state"' in revision_text + assert "op.create_table(" not in revision_text + assert "op.add_column(" not in revision_text + assert "op.drop_column(" not in revision_text From 6a56dba6f008339af045fa084562b386d1f50d0e Mon Sep 17 00:00:00 2001 From: seonghobae <8172694+seonghobae@users.noreply.github.com> Date: Mon, 13 Jul 2026 12:24:53 +0900 Subject: [PATCH 06/18] fix(pdf-dom): build newsdom sidecar from commit-pinned git context instead of a submodule The central Strix PR-scope gate fails closed on gitlink entries ('pull request changed file is not a regular PR-head file: vendor/newsdom-api'). The optional profile-gated sidecar now builds from a commit-pinned git URL context, keeping the default checkout self-contained. Co-Authored-By: Claude Fable 5 --- .gitmodules | 4 ---- docker-compose.yml | 19 ++++++++++--------- vendor/newsdom-api | 1 - 3 files changed, 10 insertions(+), 14 deletions(-) delete mode 100644 .gitmodules delete mode 160000 vendor/newsdom-api diff --git a/.gitmodules b/.gitmodules deleted file mode 100644 index 952e4ef63..000000000 --- a/.gitmodules +++ /dev/null @@ -1,4 +0,0 @@ -[submodule "vendor/newsdom-api"] - path = vendor/newsdom-api - url = https://github.com/ContextualWisdomLab/newsdom-api.git - branch = develop diff --git a/docker-compose.yml b/docker-compose.yml index 516097215..c4f6d5381 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -30,22 +30,23 @@ services: ports: - "11434:11434" - # NewsDOM PDF DOM recognition sidecar, built from the pinned - # vendor/newsdom-api submodule. Optional: the backend degrades gracefully - # (PDFs stay pending) when this service is absent or unconfigured. + # NewsDOM PDF DOM recognition sidecar, built from a commit-pinned git + # context (no submodule: the PR-scope security gate fails closed on gitlink + # entries, and a URL context keeps the default checkout self-contained). + # Optional: the backend degrades gracefully (PDFs stay pending) when this + # service is absent or unconfigured. # # Gated behind the "newsdom" profile so the default `docker compose build` - # / `config` path (e.g. the coverage-evidence CI gate) never tries to build - # from vendor/newsdom-api, which is a submodule that is not checked out in - # every environment. Bring the sidecar up intentionally with: + # / `config` path (e.g. the coverage-evidence CI gate) never builds the + # sidecar. Bring it up intentionally with: # docker compose --profile newsdom up - # (or COMPOSE_PROFILES=newsdom). The submodule must be checked out first: - # git submodule update --init vendor/newsdom-api + # (or COMPOSE_PROFILES=newsdom). newsdom: profiles: - newsdom build: - context: ./vendor/newsdom-api + # Commit-pinned for reproducible builds; bump deliberately. + context: https://github.com/ContextualWisdomLab/newsdom-api.git#6558a4238b1614c39f7e96a961815747ac4d49ef dockerfile: Dockerfile ports: - "8100:8000" diff --git a/vendor/newsdom-api b/vendor/newsdom-api deleted file mode 160000 index 4a68893af..000000000 --- a/vendor/newsdom-api +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 4a68893afffcaa5828a2966fe5c4d4d539b90541 From 09c7b61c89721fd2d4bd01543c82e236271d8454 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 15:19:56 +0900 Subject: [PATCH 07/18] fix(newsdom): wire recognition worker, retain PDF bytes, and close the safety findings Address the OpenCode REQUEST_CHANGES review on this PR: Production worker (P1): add NewsdomRecognitionWorker (mirrors ReplySlaScheduler: jittered loop + pg advisory-lock lease) wired into the app lifespan. It sweeps pending attachments and workspace documents, resolves the org's provider, recognizes via the sidecar, lands the content graph, and records explicit outcomes. recognize_attachment_pdf / recognize_document_pdf now have a real production caller. Retain source bytes (P1): deferred PDF attachments previously discarded their bytes (content=""), making recognition impossible. The raw bytes are now retained as a base64 payload in content (mirroring the document upload path) and decoded by the worker; gated by the pending status. Empty response is not parsed (P1): recognize_pdf_dom raises NewsdomEmptyRecognitionError when a 200 response yields no text/segments, so the worker records pdf_dom_recognition_failed instead of a false parsed with empty content. WebDAV materialization guard (P1): documents still pending recognition (holding a base64 payload) are refused (409) before their payload could be written to a customer WebDAV target as Markdown. PDF intent validation (P1): the recognition-intent endpoint rejects non-PDF documents (415). Migration downgrade ownership (P1): 0010_newsdom_providers.downgrade only drops the table when its columns match the signature this migration created, so a compatible pre-existing/foreign table is never deleted. Unauthenticated sidecar (P1): docker-compose no longer publishes the sidecar host port; it stays on the internal compose network (the /parse endpoint has no auth). Upload limit (P2): capped at the sidecar's 20 MiB (was 50 MiB). document_name via Form (P2): the multipart upload reads document_name from form data, not the query string. Non-ambiguous key (P2): NewsdomProvider.id -> newsdom_provider_id. Org scope for documents: add nullable Document.organization_id (0016_document_org_scope) set at upload so the worker can resolve the provider without an org-less workspace join. Tests: worker per-item outcomes (recognized / left-pending-unconfigured / failed-on-bad-payload / failed-on-empty), empty-response rejection, byte-retention round-trip, materialization 409, intent 415. cd backend && PYTHONWARNINGS=error DISABLE_BACKGROUND_WORKERS=1 python -m pytest tests -q -> 1438 passed, 35 skipped; ruff clean. Co-Authored-By: Claude Fable 5 --- .../versions/0010_newsdom_providers.py | 48 ++- .../versions/0016_document_org_scope.py | 47 +++ backend/api/data.py | 42 ++- backend/db/models.py | 8 +- backend/main.py | 4 + backend/services/attachment_parser.py | 37 +- backend/services/newsdom_client.py | 8 + backend/services/newsdom_pdf_recognition.py | 21 +- backend/services/newsdom_worker.py | 330 +++++++++++++++++- backend/tests/test_attachment_parser.py | 10 +- backend/tests/test_data_api.py | 79 +++++ backend/tests/test_email_parser.py | 7 +- backend/tests/test_newsdom_pdf_recognition.py | 24 ++ backend/tests/test_newsdom_worker.py | 193 ++++++++++ docker-compose.yml | 8 +- 15 files changed, 837 insertions(+), 29 deletions(-) create mode 100644 backend/alembic/versions/0016_document_org_scope.py create mode 100644 backend/tests/test_newsdom_worker.py diff --git a/backend/alembic/versions/0010_newsdom_providers.py b/backend/alembic/versions/0010_newsdom_providers.py index d897243b8..4b2809474 100644 --- a/backend/alembic/versions/0010_newsdom_providers.py +++ b/backend/alembic/versions/0010_newsdom_providers.py @@ -11,6 +11,23 @@ revision = "0010_newsdom_providers" down_revision = "0009_project_graph_projection" _NEWSDOM_TABLE = "newsdom_providers" +# The exact column set this migration creates. downgrade() only drops the table +# when the live schema matches this signature, so a compatible-but-foreign +# pre-existing table (e.g. one owned by another system) is never deleted. +_OWNED_COLUMNS = frozenset( + { + "newsdom_provider_id", + "user_id", + "organization_id", + "provider_name", + "base_url", + "api_token", + "request_language", + "recognition_mode", + "is_active", + "updated_at", + } +) def upgrade() -> None: @@ -19,7 +36,7 @@ def upgrade() -> None: if not inspector.has_table(_NEWSDOM_TABLE): op.create_table( _NEWSDOM_TABLE, - sa.Column("id", sa.Integer(), nullable=False), + sa.Column("newsdom_provider_id", sa.Integer(), nullable=False), sa.Column("user_id", sa.String(), nullable=False), sa.Column("organization_id", sa.String(), nullable=False), sa.Column("provider_name", sa.String(), nullable=False), @@ -29,7 +46,7 @@ def upgrade() -> None: sa.Column("recognition_mode", sa.String(length=32), nullable=False), sa.Column("is_active", sa.Boolean(), nullable=False), sa.Column("updated_at", sa.DateTime(timezone=True), nullable=True), - sa.PrimaryKeyConstraint("id"), + sa.PrimaryKeyConstraint("newsdom_provider_id"), sa.UniqueConstraint( "organization_id", "provider_name", @@ -46,17 +63,28 @@ def upgrade() -> None: ) +def _table_matches_owned_signature(inspector) -> bool: + """Return whether the live table is the one this migration created.""" + columns = {column["name"] for column in inspector.get_columns(_NEWSDOM_TABLE)} + return columns == _OWNED_COLUMNS + + def downgrade() -> None: connection = op.get_bind() inspector = sa.inspect(connection) - if inspector.has_table(_NEWSDOM_TABLE): - for index_name, _column_names in reversed(_newsdom_provider_indexes()): - op.drop_index( - index_name, - table_name=_NEWSDOM_TABLE, - if_exists=True, - ) - op.drop_table(_NEWSDOM_TABLE) + if not inspector.has_table(_NEWSDOM_TABLE): + return + # Never drop a pre-existing / foreign table that merely shares this name: + # only remove it when its columns exactly match what upgrade() created. + if not _table_matches_owned_signature(inspector): + return + for index_name, _column_names in reversed(_newsdom_provider_indexes()): + op.drop_index( + index_name, + table_name=_NEWSDOM_TABLE, + if_exists=True, + ) + op.drop_table(_NEWSDOM_TABLE) def _newsdom_provider_indexes() -> list[tuple[str, list[str]]]: diff --git a/backend/alembic/versions/0016_document_org_scope.py b/backend/alembic/versions/0016_document_org_scope.py new file mode 100644 index 000000000..0a5cd0035 --- /dev/null +++ b/backend/alembic/versions/0016_document_org_scope.py @@ -0,0 +1,47 @@ +"""add organization scope to workspace documents + +Revision ID: 0016_document_org_scope +Revises: 0015_merge_newsdom_email_heads +Create Date: 2026-07-13 00:00:00.000000 + +Workspace documents gain a nullable ``organization_id`` so the NewsDOM PDF +recognition worker can resolve the owning organization's provider without +joining through the (organization-less) workspace entity. Nullable and additive +so existing rows and personal-scope documents are unaffected. +""" + +from alembic import op +import sqlalchemy as sa + +revision = "0016_document_org_scope" +down_revision = "0015_merge_newsdom_email_heads" + +_DOCUMENTS_TABLE = "workspace_documents" +_ORG_COLUMN = "organization_id" +_ORG_INDEX = "ix_workspace_documents_organization_id" + + +def upgrade() -> None: + connection = op.get_bind() + inspector = sa.inspect(connection) + columns = {column["name"] for column in inspector.get_columns(_DOCUMENTS_TABLE)} + if _ORG_COLUMN not in columns: + op.add_column( + _DOCUMENTS_TABLE, + sa.Column(_ORG_COLUMN, sa.String(), nullable=True), + ) + op.create_index( + _ORG_INDEX, + _DOCUMENTS_TABLE, + [_ORG_COLUMN], + if_not_exists=True, + ) + + +def downgrade() -> None: + connection = op.get_bind() + inspector = sa.inspect(connection) + columns = {column["name"] for column in inspector.get_columns(_DOCUMENTS_TABLE)} + op.drop_index(_ORG_INDEX, table_name=_DOCUMENTS_TABLE, if_exists=True) + if _ORG_COLUMN in columns: + op.drop_column(_DOCUMENTS_TABLE, _ORG_COLUMN) diff --git a/backend/api/data.py b/backend/api/data.py index c115f7c11..697490e61 100644 --- a/backend/api/data.py +++ b/backend/api/data.py @@ -6,7 +6,7 @@ import re from typing import Literal, NamedTuple -from fastapi import APIRouter, Depends, File, HTTPException, UploadFile +from fastapi import APIRouter, Depends, File, Form, HTTPException, UploadFile from pydantic import BaseModel, ConfigDict, Field from sqlalchemy import and_, case, func, or_, select from sqlalchemy.engine import Row @@ -39,8 +39,11 @@ router = APIRouter(prefix="/api/data", tags=["data"]) DATA_VECTOR_DIMENSIONS = 1536 -# Upper bound for the binary PDF DOM recognition upload variant. -_MAX_PDF_DOM_UPLOAD_BYTES = 50 * 1024 * 1024 +# Upper bound for the binary PDF DOM recognition upload variant. Kept in step +# with the NewsDOM sidecar's own MAX_PARSE_UPLOAD_BYTES (20 MiB): accepting more +# would let a caller stash a pending document the configured sidecar will always +# reject while the base64 copy inflates the database. +_MAX_PDF_DOM_UPLOAD_BYTES = 20 * 1024 * 1024 ATTACHMENT_PARSE_BREAKDOWN_EVIDENCE_SOURCE = ( "email_attachments.content_type, " "email_attachments.parse_content_type, " @@ -2311,6 +2314,16 @@ def _materialized_document_target_path(document: Document) -> str: return f"/Naruon/Data/{filename}" +# Document statuses whose stored content is not yet materializable parsed text +# (it may be a base64 binary payload awaiting a recognition/conversion worker). +_NON_MATERIALIZABLE_DOCUMENT_STATUSES = frozenset( + { + PDF_DOM_RECOGNITION_PENDING_STATUS, + "hwp_conversion_pending", + } +) + + def _materialized_document_content(document: Document) -> str: return (document.document_content or "").strip() @@ -3238,6 +3251,11 @@ async def create_document_pdf_dom_recognition_intent( db: AsyncSession = Depends(get_db), ) -> DataDocumentActionResponse: document = await _get_workspace_document(db, auth_context, document_id) + if (document.document_type or "").strip().lower() != "pdf": + raise HTTPException( + status_code=415, + detail="PDF DOM recognition is only available for PDF documents.", + ) document.document_status = PDF_DOM_RECOGNITION_PENDING_STATUS await db.commit() await db.refresh(document) @@ -3257,7 +3275,9 @@ async def create_document_pdf_dom_recognition_intent( ) async def upload_document_for_pdf_dom_recognition( file: UploadFile = File(...), - document_name: str | None = None, + # Declared as multipart form data (not a query parameter) so a client + # sending document_name alongside the file is honored. + document_name: str | None = Form(None), auth_context: AuthContext = Depends(get_auth_context), db: AsyncSession = Depends(get_db), ) -> DataDocumentActionResponse: @@ -3273,6 +3293,7 @@ async def upload_document_for_pdf_dom_recognition( ) document = Document( workspace_id=auth_context.workspace_id, + organization_id=auth_context.organization_id, document_name=_safe_display_text( document_name or file.filename, "workspace document" ), @@ -3317,6 +3338,19 @@ async def create_document_webdav_materialization_intent( db: AsyncSession = Depends(get_db), ) -> DataDocumentWebdavMaterializationResponse: document = await _get_workspace_document(db, auth_context, document_id) + if document.document_status in _NON_MATERIALIZABLE_DOCUMENT_STATUSES: + # A document whose recognition/conversion is still pending holds a + # non-text payload (e.g. the base64 PDF stashed for the NewsDOM worker). + # Materializing it as Markdown would write that raw payload to the + # customer's WebDAV target. Refuse until recognition has landed real + # parsed text. + raise HTTPException( + status_code=409, + detail=( + "Workspace document is still pending recognition; " + "no materializable content yet." + ), + ) if not _materialized_document_content(document): raise HTTPException( status_code=422, diff --git a/backend/db/models.py b/backend/db/models.py index bbe2d188c..efc624e9a 100644 --- a/backend/db/models.py +++ b/backend/db/models.py @@ -304,7 +304,9 @@ class NewsdomProvider(Base): ), ) - id: Mapped[int] = mapped_column(primary_key=True) + newsdom_provider_id: Mapped[int] = mapped_column( + "newsdom_provider_id", primary_key=True + ) user_id: Mapped[str] = mapped_column(String, index=True, nullable=False) organization_id: Mapped[str] = mapped_column(String, index=True, nullable=False) provider_name: Mapped[str] = mapped_column(String, index=True, nullable=False) @@ -1546,6 +1548,10 @@ class Document(Base): document_id: Mapped[str] = mapped_column(String, primary_key=True, default=lambda: f"doc_{uuid.uuid4().hex}") workspace_id: Mapped[str] = mapped_column(String, ForeignKey("workspace_entities.workspace_id"), index=True, nullable=False) + # Owning organization (nullable for personal-scope docs). Persisted so the + # NewsDOM recognition worker can resolve the org's provider without joining + # through the (org-less) workspace entity. + organization_id: Mapped[str | None] = mapped_column(String, index=True, nullable=True) document_name: Mapped[str] = mapped_column(String, nullable=False) document_type: Mapped[str] = mapped_column(String, nullable=False) document_content: Mapped[str] = mapped_column(Text, nullable=True) diff --git a/backend/main.py b/backend/main.py index 26e65a02d..0ad7762a8 100644 --- a/backend/main.py +++ b/backend/main.py @@ -35,6 +35,7 @@ from core.telemetry import setup_telemetry from core.version import get_release_version from services.imap_worker import ImapSyncWorker +from services.newsdom_worker import NewsdomRecognitionWorker from services.pop3_worker import Pop3SyncWorker from services.provider_writeback_retry_service import ProviderWritebackRetryWorker from services.reply_sla_scheduler import ReplySlaScheduler @@ -43,6 +44,7 @@ imap_worker = ImapSyncWorker() pop3_worker = Pop3SyncWorker() reply_sla_scheduler = ReplySlaScheduler() +newsdom_recognition_worker = NewsdomRecognitionWorker() provider_writeback_retry_worker = ProviderWritebackRetryWorker( runner_manager.dispatch_command, ) @@ -59,10 +61,12 @@ async def lifespan(app: FastAPI): await imap_worker.start() await pop3_worker.start() await reply_sla_scheduler.start() + await newsdom_recognition_worker.start() await provider_writeback_retry_worker.start() yield if not DISABLE_WORKERS: await provider_writeback_retry_worker.stop() + await newsdom_recognition_worker.stop() await reply_sla_scheduler.stop() await pop3_worker.stop() await imap_worker.stop() diff --git a/backend/services/attachment_parser.py b/backend/services/attachment_parser.py index 175aeca3d..42257c5df 100644 --- a/backend/services/attachment_parser.py +++ b/backend/services/attachment_parser.py @@ -1,3 +1,5 @@ +import base64 +import binascii from dataclasses import dataclass from pathlib import Path from typing import Any @@ -144,11 +146,15 @@ def parse_email_attachment( deferred_descriptor = _DEFERRED_DESCRIPTORS_BY_CONTENT_TYPE.get(parse_content_type) if deferred_descriptor is not None: # Heavy recognition (OCR/MinerU via the NewsDOM sidecar) must not run - # inline during import — mark the attachment pending and let the worker - # populate parse_content + the content graph. + # inline during import. Retain the raw bytes as a base64 payload in + # ``content`` (mirroring the document-upload path's document_content) so + # the worker can decode and recognize them later; mark the attachment + # pending. The pending status gates display, and the worker overwrites + # ``content`` with the recognized text on success. Without this the + # source bytes were discarded and recognition was impossible. return AttachmentParseResult( filename=safe_filename, - content="", + content=_encode_deferred_payload(raw_content), content_type=normalized_content_type, parse_content="", parse_content_type=parse_content_type, @@ -229,6 +235,31 @@ def _safe_filename(filename: str | None) -> str: return display_filename +def _encode_deferred_payload(raw_content: Any) -> str: + """Base64-encode the raw attachment bytes retained for deferred recognition.""" + if isinstance(raw_content, bytes): + payload = raw_content + elif isinstance(raw_content, str): + payload = raw_content.encode("utf-8", errors="surrogatepass") + elif raw_content is None: + return "" + else: + payload = str(raw_content).encode("utf-8", errors="surrogatepass") + return base64.b64encode(payload).decode("ascii") + + +def decode_deferred_attachment_payload(content: str | None) -> bytes: + """Decode the base64 payload retained on a pending attachment's content. + + Raises ``ValueError`` when the stored payload is not valid base64, so the + recognition worker can record an error status instead of crashing. + """ + try: + return base64.b64decode((content or "").encode("ascii"), validate=True) + except (binascii.Error, ValueError) as exc: + raise ValueError("Pending attachment payload is not valid base64") from exc + + def _coerce_text(raw_content: Any) -> str: if raw_content is None: return "" diff --git a/backend/services/newsdom_client.py b/backend/services/newsdom_client.py index f197eea71..09b94fc8a 100644 --- a/backend/services/newsdom_client.py +++ b/backend/services/newsdom_client.py @@ -44,6 +44,14 @@ class NewsdomRequestError(RuntimeError): """Raised when the NewsDOM sidecar cannot fulfil a parse request.""" +class NewsdomEmptyRecognitionError(NewsdomRequestError): + """Raised when a 200 sidecar response carries no usable recognized text. + + Treated as a retryable recognition failure so the caller records an error + status instead of marking the source ``parsed`` with empty content. + """ + + @dataclass(frozen=True) class ValidatedNewsdomBaseURL: normalized_url: str diff --git a/backend/services/newsdom_pdf_recognition.py b/backend/services/newsdom_pdf_recognition.py index 6737e58b1..cc3072b29 100644 --- a/backend/services/newsdom_pdf_recognition.py +++ b/backend/services/newsdom_pdf_recognition.py @@ -26,11 +26,17 @@ from services.content_graph import ParseResult, PdfDomSection, parse_pdf_dom from services.newsdom_client import ( NewsdomConfigurationError, + NewsdomEmptyRecognitionError, request_pdf_dom, ) PDF_DOM_RECOGNITION_PENDING_STATUS = "pdf_dom_recognition_pending" PDF_DOM_RECOGNITION_PARSED_STATUS = "parsed" +# Terminal-but-retryable failure: the sidecar was reached (or the payload was +# unusable) and recognition did not produce landable content. Distinct from the +# pending status so a stuck/empty recognition is visible instead of masquerading +# as either parsed or perpetually pending. +PDF_DOM_RECOGNITION_FAILED_STATUS = "pdf_dom_recognition_failed" PDF_PARSER_KEY = "pdf" PDF_PARSE_CONTENT_TYPE = "text/plain" @@ -90,7 +96,10 @@ async def get_active_newsdom_provider( NewsdomProvider.organization_id == organization_id, NewsdomProvider.is_active.is_(True), ) - .order_by(desc(NewsdomProvider.updated_at), desc(NewsdomProvider.id)) + .order_by( + desc(NewsdomProvider.updated_at), + desc(NewsdomProvider.newsdom_provider_id), + ) .limit(1) ) return result.scalars().first() @@ -213,9 +222,17 @@ async def recognize_pdf_dom( language=config.request_language, mode=config.recognition_mode, ) - return build_recognition_records( + records = build_recognition_records( payload, source_kind=source_kind, source_record_uid=source_record_uid, display_name=display_name, ) + # A 200 response with no recognized sections/text is a recognition failure, + # not a parsed document: refuse to land empty content as "parsed" so the + # caller records a retryable error instead of a false-positive success. + if not records.parse_text.strip() and not records.parse_result.segments: + raise NewsdomEmptyRecognitionError( + "NewsDOM sidecar returned no recognizable text for the PDF" + ) + return records diff --git a/backend/services/newsdom_worker.py b/backend/services/newsdom_worker.py index 5b7748616..b38be8003 100644 --- a/backend/services/newsdom_worker.py +++ b/backend/services/newsdom_worker.py @@ -12,6 +12,15 @@ from __future__ import annotations +import asyncio +import logging +import random +from collections.abc import Awaitable, Callable + +from sqlalchemy import bindparam, func, select +from sqlalchemy.ext.asyncio import AsyncSession +from sqlalchemy.orm import selectinload + from db.models import ( Attachment, ContentNodeRecord, @@ -19,18 +28,36 @@ Document, Email, ) +from db.session import AsyncSessionLocal +from services.attachment_parser import decode_deferred_attachment_payload from services.content_graph import ParseResult -from services.newsdom_client import request_pdf_dom +from services.newsdom_client import ( + NewsdomConfigurationError, + NewsdomRequestError, + request_pdf_dom, +) from services.newsdom_pdf_recognition import ( + PDF_DOM_RECOGNITION_FAILED_STATUS, PDF_DOM_RECOGNITION_PARSED_STATUS, + PDF_DOM_RECOGNITION_PENDING_STATUS, PDF_PARSE_CONTENT_TYPE, PDF_PARSER_KEY, NewsdomRuntimeConfig, ParseRequestFn, PdfDomRecognitionRecords, recognize_pdf_dom, + resolve_newsdom_config_from_db, ) +logger = logging.getLogger(__name__) +_sysrand = random.SystemRandom() + +# How the worker resolves a runtime config for an organization. Injectable so +# the per-item processors are unit-testable without a database. +ConfigResolver = Callable[ + [AsyncSession, str | None], Awaitable[NewsdomRuntimeConfig | None] +] + def _append_parse_result_to_attachment( *, @@ -147,3 +174,304 @@ async def recognize_document_pdf( ) apply_recognition_to_document(document=document, records=records) return records + + +# -------------------------------------------------------------------------- +# Production worker: sweep pending attachments/documents and recognize them. +# -------------------------------------------------------------------------- + +DEFAULT_NEWSDOM_INTERVAL_SECONDS = 60 +DEFAULT_NEWSDOM_BATCH_LIMIT = 10 +NEWSDOM_SWEEP_LOCK_NAMESPACE = "naruon-newsdom-recognition-sweep" +MAX_STARTUP_JITTER_SECONDS = 30 + +# Per-item processing outcomes. +RESULT_RECOGNIZED = "recognized" +RESULT_PENDING = "pending" +RESULT_FAILED = "failed" + +_SWEEP_LOCK_PARAMS = { + "namespace_key": NEWSDOM_SWEEP_LOCK_NAMESPACE, + "sweep_key": "sweep", +} + + +async def process_pending_attachment( + *, + session: AsyncSession, + attachment: Attachment, + config_resolver: ConfigResolver = resolve_newsdom_config_from_db, + request_fn: ParseRequestFn = request_pdf_dom, +) -> str: + """Recognize one pending attachment PDF, or record a safe outcome. + + Returns ``RESULT_RECOGNIZED`` on success, ``RESULT_PENDING`` when no active + provider is configured yet (left pending to retry later), or + ``RESULT_FAILED`` when the payload or the sidecar response is unusable (a + retryable failure status is recorded — never a false ``parsed``). + """ + email = attachment.email + if email is None: + attachment.parse_status = PDF_DOM_RECOGNITION_FAILED_STATUS + attachment.parse_error_code = "orphan_attachment" + return RESULT_FAILED + try: + pdf_bytes = decode_deferred_attachment_payload(attachment.content) + except ValueError: + attachment.parse_status = PDF_DOM_RECOGNITION_FAILED_STATUS + attachment.parse_error_code = "invalid_pending_payload" + return RESULT_FAILED + + config = await config_resolver(session, email.organization_id) + if config is None: + # Degrade gracefully: no active NewsDOM provider for this org yet. + return RESULT_PENDING + + try: + await recognize_attachment_pdf( + email=email, + attachment=attachment, + pdf_bytes=pdf_bytes, + config=config, + source_record_uid=f"attachment-{attachment.id}", + request_fn=request_fn, + ) + except NewsdomConfigurationError: + return RESULT_PENDING + except (NewsdomRequestError, ValueError): + attachment.parse_status = PDF_DOM_RECOGNITION_FAILED_STATUS + attachment.parse_error_code = "recognition_failed" + return RESULT_FAILED + return RESULT_RECOGNIZED + + +async def process_pending_document( + *, + session: AsyncSession, + document: Document, + config_resolver: ConfigResolver = resolve_newsdom_config_from_db, + request_fn: ParseRequestFn = request_pdf_dom, +) -> str: + """Recognize one pending workspace-document PDF, or record a safe outcome.""" + from api.data import decode_pending_pdf_document_bytes + + try: + pdf_bytes = decode_pending_pdf_document_bytes(document) + except ValueError: + document.document_status = PDF_DOM_RECOGNITION_FAILED_STATUS + return RESULT_FAILED + + config = await config_resolver(session, document.organization_id) + if config is None: + return RESULT_PENDING + + try: + await recognize_document_pdf( + document=document, + pdf_bytes=pdf_bytes, + config=config, + request_fn=request_fn, + ) + except NewsdomConfigurationError: + return RESULT_PENDING + except (NewsdomRequestError, ValueError): + document.document_status = PDF_DOM_RECOGNITION_FAILED_STATUS + return RESULT_FAILED + return RESULT_RECOGNIZED + + +def _session_uses_postgresql(session: AsyncSession) -> bool: + try: + bind = session.get_bind() + except Exception: + return False + return getattr(getattr(bind, "dialect", None), "name", None) == "postgresql" + + +async def _try_acquire_sweep_lease(session: AsyncSession) -> bool | None: + """Become the sweep leader for this cycle (None when not PostgreSQL).""" + if not _session_uses_postgresql(session): + return None + acquired = await session.scalar( + select( + func.pg_try_advisory_lock( + func.hashtext(bindparam("namespace_key")), + func.hashtext(bindparam("sweep_key")), + ) + ), + _SWEEP_LOCK_PARAMS, + ) + return bool(acquired) + + +async def _release_sweep_lease(session: AsyncSession) -> None: + await session.scalar( + select( + func.pg_advisory_unlock( + func.hashtext(bindparam("namespace_key")), + func.hashtext(bindparam("sweep_key")), + ) + ), + _SWEEP_LOCK_PARAMS, + ) + + +class NewsdomRecognitionWorker: + """Periodically recognize pending PDF attachments and workspace documents. + + Mirrors :class:`ReplySlaScheduler`: a jittered periodic loop, a PostgreSQL + advisory-lock lease so only one replica sweeps per cycle, and per-item + error isolation. Items whose organization has no active NewsDOM provider are + left pending (they recognize once a provider is configured); unusable + payloads/responses are marked failed rather than parsed. + """ + + def __init__( + self, + *, + interval_seconds: int = DEFAULT_NEWSDOM_INTERVAL_SECONDS, + batch_limit: int = DEFAULT_NEWSDOM_BATCH_LIMIT, + request_fn: ParseRequestFn = request_pdf_dom, + config_resolver: ConfigResolver = resolve_newsdom_config_from_db, + ): + self.interval_seconds = interval_seconds + self.batch_limit = batch_limit + self._request_fn = request_fn + self._config_resolver = config_resolver + self._task: asyncio.Task | None = None + self._is_running = False + + async def start(self) -> None: + if self._is_running: + logger.warning("NewsdomRecognitionWorker is already running.") + return + self._is_running = True + self._task = asyncio.create_task(self._run_loop()) + logger.info("NewsdomRecognitionWorker started.") + + async def stop(self) -> None: + if not self._is_running: + return + self._is_running = False + if self._task: + self._task.cancel() + try: + await self._task + except asyncio.CancelledError: + logger.debug("NewsdomRecognitionWorker cancellation acknowledged.") + logger.info("NewsdomRecognitionWorker stopped.") + + async def _run_loop(self) -> None: + try: + await asyncio.sleep( + _sysrand.uniform( + 0, min(self.interval_seconds / 10, MAX_STARTUP_JITTER_SECONDS) + ) + ) + except asyncio.CancelledError: + return + + while self._is_running: + try: + await self._sweep() + except asyncio.CancelledError: + break + except Exception: + logger.error("Error in NewsdomRecognitionWorker loop.", exc_info=True) + if self._is_running: + try: + await asyncio.sleep(self.interval_seconds) + except asyncio.CancelledError: + break + + async def _sweep(self) -> None: + async with AsyncSessionLocal() as session: + lease = await _try_acquire_sweep_lease(session) + if lease is False: + logger.debug( + "NewsDOM recognition sweep skipped: another replica holds " + "the lease." + ) + return + try: + await self._sweep_attachments(session) + await self._sweep_documents(session) + finally: + if lease is True: + await _release_sweep_lease(session) + + async def _sweep_attachments(self, session: AsyncSession) -> None: + rows = ( + ( + await session.execute( + select(Attachment) + .where( + Attachment.parse_status == PDF_DOM_RECOGNITION_PENDING_STATUS + ) + .options(selectinload(Attachment.email)) + .limit(self.batch_limit) + ) + ) + .scalars() + .all() + ) + for attachment in rows: + try: + result = await process_pending_attachment( + session=session, + attachment=attachment, + config_resolver=self._config_resolver, + request_fn=self._request_fn, + ) + await session.commit() + if result != RESULT_PENDING: + logger.info( + "NewsDOM attachment %s recognition result: %s", + attachment.id, + result, + ) + except Exception: + await session.rollback() + logger.error( + "NewsDOM attachment %s recognition raised.", + getattr(attachment, "id", "?"), + exc_info=True, + ) + + async def _sweep_documents(self, session: AsyncSession) -> None: + rows = ( + ( + await session.execute( + select(Document) + .where( + Document.document_status + == PDF_DOM_RECOGNITION_PENDING_STATUS + ) + .limit(self.batch_limit) + ) + ) + .scalars() + .all() + ) + for document in rows: + try: + result = await process_pending_document( + session=session, + document=document, + config_resolver=self._config_resolver, + request_fn=self._request_fn, + ) + await session.commit() + if result != RESULT_PENDING: + logger.info( + "NewsDOM document %s recognition result: %s", + document.document_id, + result, + ) + except Exception: + await session.rollback() + logger.error( + "NewsDOM document %s recognition raised.", + getattr(document, "document_id", "?"), + exc_info=True, + ) diff --git a/backend/tests/test_attachment_parser.py b/backend/tests/test_attachment_parser.py index 40d83e748..74c0267f3 100644 --- a/backend/tests/test_attachment_parser.py +++ b/backend/tests/test_attachment_parser.py @@ -155,22 +155,26 @@ def test_unsupported_binary_attachment_is_visible_without_raw_bytes(): def test_pdf_attachment_is_deferred_pending_newsdom_recognition(): + from services.attachment_parser import decode_deferred_attachment_payload + + raw = b"%PDF-1.7 raw bytes" result = parse_email_attachment( filename="contract.pdf", content_type="application/pdf", - raw_content=b"%PDF-1.7 raw bytes", + raw_content=raw, ) assert result.filename == "contract.pdf" assert result.content_type == "application/pdf" # Heavy OCR/MinerU recognition is deferred to the worker: nothing is parsed - # inline, and the attachment carries the pending status. - assert result.content == "" + # inline, and the attachment carries the pending status. The raw bytes are + # retained as a base64 payload so the worker can recognize them later. assert result.parse_content == "" assert result.parse_content_type == "application/pdf" assert result.parser_key == "pdf" assert result.parse_status == "pdf_dom_recognition_pending" assert result.parse_error_code is None + assert decode_deferred_attachment_payload(result.content) == raw def test_pdf_extension_with_generic_content_type_is_deferred_pending(): diff --git a/backend/tests/test_data_api.py b/backend/tests/test_data_api.py index 1209dd7d1..64b31c8a9 100644 --- a/backend/tests/test_data_api.py +++ b/backend/tests/test_data_api.py @@ -2769,6 +2769,85 @@ def test_data_document_webdav_materialization_rejects_empty_document(mock_db): ) +def test_data_document_webdav_materialization_rejects_pending_pdf(mock_db): + # A PDF still pending NewsDOM recognition holds a base64 payload in + # document_content; materializing it would write that binary as Markdown. + mock_db.documents.append( + Document( + document_id="doc_pending", + workspace_id="workspace-org-acme", + document_name="contract.pdf", + document_type="pdf", + document_content="JVBERi0xLjcK", # base64 %PDF-1.7\n + document_status="pdf_dom_recognition_pending", + created_at=_now(), + ) + ) + token = _signed_session_token(_valid_session_payload()) + client, previous_secret, original_overrides = _with_signed_auth(mock_db, token) + try: + response = client.post( + "/api/data/documents/doc_pending/webdav-materialization-intent", + json={ + "target_source_id": "webdav_src_primary", + "execute_provider": True, + }, + ) + finally: + client.close() + _restore_overrides(previous_secret, original_overrides) + + assert response.status_code == 409, response.text + assert "pending recognition" in response.json()["detail"] + + +def test_data_pdf_dom_recognition_intent_rejects_non_pdf_document(mock_db): + mock_db.documents.append( + Document( + document_id="doc_text", + workspace_id="workspace-org-acme", + document_name="notes.md", + document_type="text/markdown", + document_content="# Notes", + document_status="uploaded", + created_at=_now(), + ) + ) + token = _signed_session_token(_valid_session_payload()) + client, previous_secret, original_overrides = _with_signed_auth(mock_db, token) + try: + response = client.post( + "/api/data/documents/doc_text/pdf-dom-recognition-intent", + ) + finally: + client.close() + _restore_overrides(previous_secret, original_overrides) + + assert response.status_code == 415, response.text + # A PDF document is accepted. + mock_db.documents.append( + Document( + document_id="doc_pdf", + workspace_id="workspace-org-acme", + document_name="contract.pdf", + document_type="pdf", + document_content="JVBERi0xLjcK", + document_status="uploaded", + created_at=_now(), + ) + ) + client, previous_secret, original_overrides = _with_signed_auth(mock_db, token) + try: + ok = client.post( + "/api/data/documents/doc_pdf/pdf-dom-recognition-intent", + ) + finally: + client.close() + _restore_overrides(previous_secret, original_overrides) + assert ok.status_code == 200, ok.text + assert ok.json()["document_status"] == "pdf_dom_recognition_pending" + + async def _seed_smoke_test_data(conn, ids: dict): await conn.execute(text("SELECT 1")) await conn.execute(text("CREATE EXTENSION IF NOT EXISTS vector")) diff --git a/backend/tests/test_email_parser.py b/backend/tests/test_email_parser.py index e4be88357..e44d03127 100644 --- a/backend/tests/test_email_parser.py +++ b/backend/tests/test_email_parser.py @@ -274,12 +274,13 @@ def test_parse_eml_extracts_supported_and_unsupported_attachment_metadata(): }, { "filename": "contract.pdf", - "content": "", + # Deferred to the NewsDOM worker: the raw PDF bytes are retained + # as a base64 payload (round-trips the fixture's %PDF-1.7\n) so + # the worker can recognize them later. + "content": "JVBERi0xLjcK", "content_type": "application/pdf", "parse_content": "", "parse_content_type": "application/pdf", - # PDFs are deferred to the NewsDOM recognition worker rather than - # parsed inline, so they arrive pending (not unsupported). "parser_key": "pdf", "parse_status": "pdf_dom_recognition_pending", "parse_error_code": None, diff --git a/backend/tests/test_newsdom_pdf_recognition.py b/backend/tests/test_newsdom_pdf_recognition.py index ec914a87e..542811920 100644 --- a/backend/tests/test_newsdom_pdf_recognition.py +++ b/backend/tests/test_newsdom_pdf_recognition.py @@ -248,6 +248,30 @@ async def fake_request(**_kwargs): assert email.content_segments +@pytest.mark.asyncio +async def test_recognize_pdf_dom_rejects_empty_sidecar_response(): + from services.newsdom_client import NewsdomEmptyRecognitionError + + async def empty_request(**_kwargs): + return {"pages": []} + + with pytest.raises(NewsdomEmptyRecognitionError): + await recognize_pdf_dom( + config=NewsdomRuntimeConfig( + base_url="https://newsdom.example.com", + api_token=None, + request_language="auto", + recognition_mode="auto", + provider_name="primary", + ), + pdf_bytes=b"%PDF-1.7 fake", + filename="news.pdf", + source_kind="attachment", + source_record_uid="att-1", + request_fn=empty_request, + ) + + def test_apply_recognition_to_attachment_is_pure_mapping(): email = Email() attachment = Attachment(filename="news.pdf") diff --git a/backend/tests/test_newsdom_worker.py b/backend/tests/test_newsdom_worker.py new file mode 100644 index 000000000..30b2da2aa --- /dev/null +++ b/backend/tests/test_newsdom_worker.py @@ -0,0 +1,193 @@ +"""Unit tests for the NewsDOM recognition worker's per-item processing. + +Fully mocked: in-memory models, an injected async config resolver, and a canned +sidecar ``request_fn`` — no database, no network. Covers the fail-closed +outcomes (unconfigured -> pending, bad payload -> failed, empty response -> +failed) that keep a pending PDF from ever masquerading as parsed. +""" + +import base64 + +import pytest + +from db.models import Attachment, Document, Email +from services.newsdom_pdf_recognition import ( + PDF_DOM_RECOGNITION_FAILED_STATUS, + PDF_DOM_RECOGNITION_PENDING_STATUS, + NewsdomRuntimeConfig, +) +from services.newsdom_worker import ( + RESULT_FAILED, + RESULT_PENDING, + RESULT_RECOGNIZED, + process_pending_attachment, + process_pending_document, +) + + +def _config() -> NewsdomRuntimeConfig: + return NewsdomRuntimeConfig( + base_url="https://newsdom.example.com", + api_token=None, + request_language="auto", + recognition_mode="auto", + provider_name="primary", + ) + + +def _canned_response() -> dict: + return { + "pages": [ + { + "page_number": 1, + "articles": [ + {"headline": "Headline", "body_blocks": ["Body one."]} + ], + } + ] + } + + +async def _resolver_with(config): + async def resolve(_session, _org): + return config + + return resolve + + +def _pending_attachment(payload: bytes = b"%PDF-1.7 fake") -> Attachment: + email = Email() + email.organization_id = "org-1" + attachment = Attachment( + filename="news.pdf", + content=base64.b64encode(payload).decode("ascii"), + parse_status=PDF_DOM_RECOGNITION_PENDING_STATUS, + ) + email.attachments.append(attachment) + return attachment + + +@pytest.mark.asyncio +async def test_attachment_recognized_when_configured(): + attachment = _pending_attachment() + + async def request_fn(**_kwargs): + return _canned_response() + + result = await process_pending_attachment( + session=object(), + attachment=attachment, + config_resolver=await _resolver_with(_config()), + request_fn=request_fn, + ) + assert result == RESULT_RECOGNIZED + assert attachment.parse_status == "parsed" + assert "Headline" in attachment.parse_content + assert attachment.content_segments + + +@pytest.mark.asyncio +async def test_attachment_left_pending_when_no_provider(): + attachment = _pending_attachment() + + async def request_fn(**_kwargs): # pragma: no cover - must not be called + raise AssertionError("sidecar must not be called when unconfigured") + + result = await process_pending_attachment( + session=object(), + attachment=attachment, + config_resolver=await _resolver_with(None), + request_fn=request_fn, + ) + assert result == RESULT_PENDING + assert attachment.parse_status == PDF_DOM_RECOGNITION_PENDING_STATUS + + +@pytest.mark.asyncio +async def test_attachment_failed_on_invalid_payload(): + attachment = _pending_attachment() + attachment.content = "not@@base64!!" + + async def request_fn(**_kwargs): # pragma: no cover + raise AssertionError("must not reach sidecar with a bad payload") + + result = await process_pending_attachment( + session=object(), + attachment=attachment, + config_resolver=await _resolver_with(_config()), + request_fn=request_fn, + ) + assert result == RESULT_FAILED + assert attachment.parse_status == PDF_DOM_RECOGNITION_FAILED_STATUS + assert attachment.parse_error_code == "invalid_pending_payload" + + +@pytest.mark.asyncio +async def test_attachment_failed_on_empty_sidecar_response(): + attachment = _pending_attachment() + + async def request_fn(**_kwargs): + return {"pages": []} + + result = await process_pending_attachment( + session=object(), + attachment=attachment, + config_resolver=await _resolver_with(_config()), + request_fn=request_fn, + ) + assert result == RESULT_FAILED + assert attachment.parse_status == PDF_DOM_RECOGNITION_FAILED_STATUS + assert attachment.parse_error_code == "recognition_failed" + # Never landed as parsed with empty content. + assert attachment.parse_status != "parsed" + + +@pytest.mark.asyncio +async def test_document_recognized_when_configured(): + document = Document( + document_id="doc-1", + workspace_id="ws-1", + organization_id="org-1", + document_name="news.pdf", + document_type="pdf", + document_content=base64.b64encode(b"%PDF-1.7 fake").decode("ascii"), + document_status=PDF_DOM_RECOGNITION_PENDING_STATUS, + ) + + async def request_fn(**_kwargs): + return _canned_response() + + result = await process_pending_document( + session=object(), + document=document, + config_resolver=await _resolver_with(_config()), + request_fn=request_fn, + ) + assert result == RESULT_RECOGNIZED + assert document.document_status == "parsed" + assert "Headline" in document.document_content + + +@pytest.mark.asyncio +async def test_document_failed_on_empty_response(): + document = Document( + document_id="doc-2", + workspace_id="ws-1", + organization_id="org-1", + document_name="news.pdf", + document_type="pdf", + document_content=base64.b64encode(b"%PDF-1.7 fake").decode("ascii"), + document_status=PDF_DOM_RECOGNITION_PENDING_STATUS, + ) + + async def request_fn(**_kwargs): + return {"pages": []} + + result = await process_pending_document( + session=object(), + document=document, + config_resolver=await _resolver_with(_config()), + request_fn=request_fn, + ) + assert result == RESULT_FAILED + assert document.document_status == PDF_DOM_RECOGNITION_FAILED_STATUS diff --git a/docker-compose.yml b/docker-compose.yml index c4f6d5381..a6adf60cb 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -48,8 +48,12 @@ services: # Commit-pinned for reproducible builds; bump deliberately. context: https://github.com/ContextualWisdomLab/newsdom-api.git#6558a4238b1614c39f7e96a961815747ac4d49ef dockerfile: Dockerfile - ports: - - "8100:8000" + # No host `ports` mapping: the sidecar has no authentication on /parse, so + # it must stay on the internal compose network only. The backend reaches it + # via the service name (ALLOWED_NEWSDOM_HOSTS: newsdom). Do not publish it + # to the host. + expose: + - "8000" healthcheck: test: ["CMD-SHELL", "python -c \"import urllib.request,sys; sys.exit(0 if urllib.request.urlopen('http://localhost:8000/health').status==200 else 1)\""] interval: 10s From 9ca55508ab8debde7053ee7a2172ffe59b3ae0b5 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 15:45:25 +0900 Subject: [PATCH 08/18] Harden deferred PDF recognition --- backend/api/data.py | 18 +++- backend/services/attachment_parser.py | 73 ++++++++++++--- backend/services/newsdom_worker.py | 45 +++++++-- backend/tests/test_attachment_parser.py | 71 ++++++++++++++- backend/tests/test_data_api.py | 91 +++++++++++++++++++ backend/tests/test_newsdom_pdf_recognition.py | 2 +- backend/tests/test_newsdom_worker.py | 2 +- 7 files changed, 277 insertions(+), 25 deletions(-) diff --git a/backend/api/data.py b/backend/api/data.py index 697490e61..c470d69c0 100644 --- a/backend/api/data.py +++ b/backend/api/data.py @@ -3167,6 +3167,7 @@ async def upload_data_document( ) -> DataDocumentActionResponse: document = Document( workspace_id=auth_context.workspace_id, + organization_id=auth_context.organization_id, document_name=_safe_display_text(request.document_name, "workspace document"), document_type=_safe_document_type(request.document_type), document_content=request.document_content, @@ -3256,6 +3257,14 @@ async def create_document_pdf_dom_recognition_intent( status_code=415, detail="PDF DOM recognition is only available for PDF documents.", ) + try: + decode_pending_pdf_document_bytes(document) + except ValueError as exc: + raise HTTPException( + status_code=422, + detail="Stored PDF payload is not valid for DOM recognition.", + ) from exc + document.organization_id = auth_context.organization_id document.document_status = PDF_DOM_RECOGNITION_PENDING_STATUS await db.commit() await db.refresh(document) @@ -3320,11 +3329,16 @@ def decode_pending_pdf_document_bytes(document: Document) -> bytes: Used by the recognition worker before calling the NewsDOM sidecar. """ try: - return base64.b64decode( + payload = base64.b64decode( (document.document_content or "").encode("ascii"), validate=True ) - except (binascii.Error, ValueError) as exc: + except (binascii.Error, UnicodeEncodeError, ValueError) as exc: raise ValueError("Pending PDF document payload is not valid base64") from exc + if len(payload) > _MAX_PDF_DOM_UPLOAD_BYTES: + raise ValueError("Pending PDF document exceeds the upload size limit") + if not payload.startswith(b"%PDF-"): + raise ValueError("Pending PDF document payload is not a PDF") + return payload @router.post( diff --git a/backend/services/attachment_parser.py b/backend/services/attachment_parser.py index 42257c5df..868b9b183 100644 --- a/backend/services/attachment_parser.py +++ b/backend/services/attachment_parser.py @@ -1,3 +1,5 @@ +"""Classify email attachments and retain safe deferred parser inputs.""" + import base64 import binascii from dataclasses import dataclass @@ -13,10 +15,13 @@ "application/x-binary", } MAX_ATTACHMENT_PARSE_SOURCE_CHARS = 1_000_000 +MAX_ATTACHMENT_PARSE_SOURCE_BYTES = 20 * 1024 * 1024 @dataclass(frozen=True) class AttachmentParserDescriptor: + """Describe one supported attachment parser surface.""" + parser_key: str display_name: str content_types: tuple[str, ...] @@ -116,6 +121,8 @@ class AttachmentParserDescriptor: @dataclass(frozen=True) class AttachmentParseResult: + """Carry display, parser, and deferred-recognition attachment fields.""" + filename: str content: str content_type: str @@ -127,6 +134,7 @@ class AttachmentParseResult: def get_attachment_parser_manifest() -> list[AttachmentParserDescriptor]: + """Return a mutable snapshot of the attachment parser manifest.""" return list(_PARSER_MANIFEST) @@ -136,6 +144,7 @@ def parse_email_attachment( content_type: str | None, raw_content: Any, ) -> AttachmentParseResult: + """Classify and normalize one attachment without running heavy parsers.""" safe_filename = _safe_filename(filename) normalized_content_type = _normalize_content_type(content_type) parse_content_type = _parse_content_type_for( @@ -152,9 +161,34 @@ def parse_email_attachment( # pending. The pending status gates display, and the worker overwrites # ``content`` with the recognized text on success. Without this the # source bytes were discarded and recognition was impossible. + deferred_payload = _coerce_deferred_payload_bytes(raw_content) + if len(deferred_payload) > MAX_ATTACHMENT_PARSE_SOURCE_BYTES: + return AttachmentParseResult( + filename=safe_filename, + content="", + content_type=normalized_content_type, + parse_content="", + parse_content_type=parse_content_type, + parser_key=deferred_descriptor.parser_key, + parse_status="parse_size_limit_exceeded", + parse_error_code="parse_size_limit_exceeded", + ) + if parse_content_type == "application/pdf" and not deferred_payload.startswith( + b"%PDF-" + ): + return AttachmentParseResult( + filename=safe_filename, + content="", + content_type=normalized_content_type, + parse_content="", + parse_content_type=parse_content_type, + parser_key=deferred_descriptor.parser_key, + parse_status="invalid_pdf_payload", + parse_error_code="invalid_pdf_payload", + ) return AttachmentParseResult( filename=safe_filename, - content=_encode_deferred_payload(raw_content), + content=_encode_deferred_payload(deferred_payload), content_type=normalized_content_type, parse_content="", parse_content_type=parse_content_type, @@ -206,12 +240,14 @@ def parse_email_attachment( def _normalize_content_type(content_type: str | None) -> str: + """Return a lowercase MIME type without parameters.""" normalized = (content_type or "application/octet-stream").split(";", 1)[0] normalized = normalized.strip().lower() return normalized or "application/octet-stream" def _parse_content_type_for(filename: str, content_type: str) -> str: + """Resolve generic MIME types from a recognized filename extension.""" if content_type not in _GENERIC_CONTENT_TYPES: return content_type extension = Path(filename).suffix.lower() @@ -219,6 +255,7 @@ def _parse_content_type_for(filename: str, content_type: str) -> str: def _parser_key_for(parse_content_type: str, parse_status: str) -> str: + """Return the parser key associated with a parse MIME type and status.""" if parse_status == "unsupported_content_type": return "unsupported_binary" for descriptor in _PARSER_MANIFEST: @@ -228,6 +265,7 @@ def _parser_key_for(parse_content_type: str, parse_status: str) -> str: def _safe_filename(filename: str | None) -> str: + """Return a basename-only attachment display filename.""" display_filename = strip_html_markup(_sanitize_nul(filename or "attachment")) display_filename = Path(display_filename).name.strip() if display_filename in {"", ".", ".."}: @@ -235,16 +273,19 @@ def _safe_filename(filename: str | None) -> str: return display_filename -def _encode_deferred_payload(raw_content: Any) -> str: - """Base64-encode the raw attachment bytes retained for deferred recognition.""" +def _coerce_deferred_payload_bytes(raw_content: Any) -> bytes: + """Return the exact byte payload retained for deferred recognition.""" if isinstance(raw_content, bytes): - payload = raw_content - elif isinstance(raw_content, str): - payload = raw_content.encode("utf-8", errors="surrogatepass") - elif raw_content is None: - return "" - else: - payload = str(raw_content).encode("utf-8", errors="surrogatepass") + return raw_content + if isinstance(raw_content, str): + return raw_content.encode("utf-8", errors="surrogatepass") + if raw_content is None: + return b"" + return str(raw_content).encode("utf-8", errors="surrogatepass") + + +def _encode_deferred_payload(payload: bytes) -> str: + """Base64-encode validated bytes retained for deferred recognition.""" return base64.b64encode(payload).decode("ascii") @@ -255,12 +296,18 @@ def decode_deferred_attachment_payload(content: str | None) -> bytes: recognition worker can record an error status instead of crashing. """ try: - return base64.b64decode((content or "").encode("ascii"), validate=True) - except (binascii.Error, ValueError) as exc: + payload = base64.b64decode((content or "").encode("ascii"), validate=True) + except (binascii.Error, UnicodeEncodeError, ValueError) as exc: raise ValueError("Pending attachment payload is not valid base64") from exc + if len(payload) > MAX_ATTACHMENT_PARSE_SOURCE_BYTES: + raise ValueError("Pending attachment PDF exceeds the parse size limit") + if not payload.startswith(b"%PDF-"): + raise ValueError("Pending attachment payload is not a PDF") + return payload def _coerce_text(raw_content: Any) -> str: + """Coerce arbitrary attachment content to NUL-free text.""" if raw_content is None: return "" if isinstance(raw_content, str): @@ -271,8 +318,10 @@ def _coerce_text(raw_content: Any) -> str: def _display_text(raw_content: str) -> str: + """Strip markup and collapse whitespace for safe attachment display.""" return " ".join(strip_html_markup(raw_content).split()) def _sanitize_nul(text: str) -> str: + """Remove NUL characters that database text fields cannot retain.""" return text.replace("\x00", "") diff --git a/backend/services/newsdom_worker.py b/backend/services/newsdom_worker.py index b38be8003..5544f11cd 100644 --- a/backend/services/newsdom_worker.py +++ b/backend/services/newsdom_worker.py @@ -3,7 +3,7 @@ Attachments and workspace documents whose PDF recognition was deferred at import time are processed here: the sidecar is called (via :mod:`services.newsdom_pdf_recognition`) and the returned tree is landed into -``parse_content`` (for embeddings) and, for attachments, the +the persisted attachment/document content and, for attachments, the ``content_nodes`` / ``content_segments`` graph. The apply functions are deliberately session-free so they can be unit tested @@ -65,6 +65,7 @@ def _append_parse_result_to_attachment( attachment: Attachment, parse_result: ParseResult, ) -> None: + """Append recognized graph records to an email and its attachment.""" node_records_by_uid: dict[str, ContentNodeRecord] = {} for parsed_node in parse_result.nodes: node_record = ContentNodeRecord( @@ -110,7 +111,6 @@ def apply_recognition_to_attachment( records: PdfDomRecognitionRecords, ) -> None: """Land recognized PDF DOM records onto an attachment (text + graph).""" - attachment.parse_content = records.parse_text attachment.content = records.parse_text attachment.parse_content_type = PDF_PARSE_CONTENT_TYPE attachment.parser_key = PDF_PARSER_KEY @@ -143,6 +143,7 @@ async def recognize_attachment_pdf( source_record_uid: str, request_fn: ParseRequestFn = request_pdf_dom, ) -> PdfDomRecognitionRecords: + """Recognize a PDF and land its text and graph on an attachment.""" records = await recognize_pdf_dom( config=config, pdf_bytes=pdf_bytes, @@ -163,6 +164,7 @@ async def recognize_document_pdf( config: NewsdomRuntimeConfig | None, request_fn: ParseRequestFn = request_pdf_dom, ) -> PdfDomRecognitionRecords: + """Recognize a PDF and land its text on a workspace document.""" records = await recognize_pdf_dom( config=config, pdf_bytes=pdf_bytes, @@ -208,7 +210,7 @@ async def process_pending_attachment( Returns ``RESULT_RECOGNIZED`` on success, ``RESULT_PENDING`` when no active provider is configured yet (left pending to retry later), or ``RESULT_FAILED`` when the payload or the sidecar response is unusable (a - retryable failure status is recorded — never a false ``parsed``). + visible failure status is recorded - never a false ``parsed``). """ email = attachment.email if email is None: @@ -217,9 +219,14 @@ async def process_pending_attachment( return RESULT_FAILED try: pdf_bytes = decode_deferred_attachment_payload(attachment.content) - except ValueError: + except ValueError as exc: attachment.parse_status = PDF_DOM_RECOGNITION_FAILED_STATUS attachment.parse_error_code = "invalid_pending_payload" + logger.warning( + "NewsDOM attachment %s rejected before recognition: %s", + getattr(attachment, "id", "?"), + exc, + ) return RESULT_FAILED config = await config_resolver(session, email.organization_id) @@ -238,9 +245,14 @@ async def process_pending_attachment( ) except NewsdomConfigurationError: return RESULT_PENDING - except (NewsdomRequestError, ValueError): + except (NewsdomRequestError, ValueError) as exc: attachment.parse_status = PDF_DOM_RECOGNITION_FAILED_STATUS attachment.parse_error_code = "recognition_failed" + logger.warning( + "NewsDOM attachment %s recognition failed: %s", + getattr(attachment, "id", "?"), + exc, + ) return RESULT_FAILED return RESULT_RECOGNIZED @@ -257,8 +269,13 @@ async def process_pending_document( try: pdf_bytes = decode_pending_pdf_document_bytes(document) - except ValueError: + except ValueError as exc: document.document_status = PDF_DOM_RECOGNITION_FAILED_STATUS + logger.warning( + "NewsDOM document %s rejected before recognition: %s", + getattr(document, "document_id", "?"), + exc, + ) return RESULT_FAILED config = await config_resolver(session, document.organization_id) @@ -274,13 +291,19 @@ async def process_pending_document( ) except NewsdomConfigurationError: return RESULT_PENDING - except (NewsdomRequestError, ValueError): + except (NewsdomRequestError, ValueError) as exc: document.document_status = PDF_DOM_RECOGNITION_FAILED_STATUS + logger.warning( + "NewsDOM document %s recognition failed: %s", + getattr(document, "document_id", "?"), + exc, + ) return RESULT_FAILED return RESULT_RECOGNIZED def _session_uses_postgresql(session: AsyncSession) -> bool: + """Return whether advisory-lock SQL is supported by the session bind.""" try: bind = session.get_bind() except Exception: @@ -305,6 +328,7 @@ async def _try_acquire_sweep_lease(session: AsyncSession) -> bool | None: async def _release_sweep_lease(session: AsyncSession) -> None: + """Release the PostgreSQL advisory lock for a recognition sweep.""" await session.scalar( select( func.pg_advisory_unlock( @@ -334,6 +358,7 @@ def __init__( request_fn: ParseRequestFn = request_pdf_dom, config_resolver: ConfigResolver = resolve_newsdom_config_from_db, ): + """Configure the sweep cadence, batch size, and injectable adapters.""" self.interval_seconds = interval_seconds self.batch_limit = batch_limit self._request_fn = request_fn @@ -342,6 +367,7 @@ def __init__( self._is_running = False async def start(self) -> None: + """Start the recognition loop once.""" if self._is_running: logger.warning("NewsdomRecognitionWorker is already running.") return @@ -350,6 +376,7 @@ async def start(self) -> None: logger.info("NewsdomRecognitionWorker started.") async def stop(self) -> None: + """Cancel and await the active recognition loop.""" if not self._is_running: return self._is_running = False @@ -362,6 +389,7 @@ async def stop(self) -> None: logger.info("NewsdomRecognitionWorker stopped.") async def _run_loop(self) -> None: + """Run jittered recognition sweeps until stopped.""" try: await asyncio.sleep( _sysrand.uniform( @@ -385,6 +413,7 @@ async def _run_loop(self) -> None: break async def _sweep(self) -> None: + """Process one leased attachment and document sweep.""" async with AsyncSessionLocal() as session: lease = await _try_acquire_sweep_lease(session) if lease is False: @@ -401,6 +430,7 @@ async def _sweep(self) -> None: await _release_sweep_lease(session) async def _sweep_attachments(self, session: AsyncSession) -> None: + """Process a bounded batch of pending PDF attachments.""" rows = ( ( await session.execute( @@ -439,6 +469,7 @@ async def _sweep_attachments(self, session: AsyncSession) -> None: ) async def _sweep_documents(self, session: AsyncSession) -> None: + """Process a bounded batch of pending workspace PDF documents.""" rows = ( ( await session.execute( diff --git a/backend/tests/test_attachment_parser.py b/backend/tests/test_attachment_parser.py index 74c0267f3..dff33563f 100644 --- a/backend/tests/test_attachment_parser.py +++ b/backend/tests/test_attachment_parser.py @@ -1,5 +1,12 @@ +import base64 + +import pytest + +import services.attachment_parser as attachment_parser from services.attachment_parser import ( + MAX_ATTACHMENT_PARSE_SOURCE_BYTES, MAX_ATTACHMENT_PARSE_SOURCE_CHARS, + decode_deferred_attachment_payload, get_attachment_parser_manifest, parse_email_attachment, ) @@ -155,8 +162,6 @@ def test_unsupported_binary_attachment_is_visible_without_raw_bytes(): def test_pdf_attachment_is_deferred_pending_newsdom_recognition(): - from services.attachment_parser import decode_deferred_attachment_payload - raw = b"%PDF-1.7 raw bytes" result = parse_email_attachment( filename="contract.pdf", @@ -187,3 +192,65 @@ def test_pdf_extension_with_generic_content_type_is_deferred_pending(): assert result.parse_content_type == "application/pdf" assert result.parser_key == "pdf" assert result.parse_status == "pdf_dom_recognition_pending" + + +def test_invalid_pdf_payload_is_rejected_before_deferred_recognition(): + result = parse_email_attachment( + filename="not-a-pdf.pdf", + content_type="application/pdf", + raw_content=b"plain text with a PDF content type", + ) + + assert result.content == "" + assert result.parse_status == "invalid_pdf_payload" + assert result.parse_error_code == "invalid_pdf_payload" + + +def test_oversized_pdf_payload_is_not_retained(): + result = parse_email_attachment( + filename="huge.pdf", + content_type="application/pdf", + raw_content=b"%PDF-" + b"A" * MAX_ATTACHMENT_PARSE_SOURCE_BYTES, + ) + + assert result.content == "" + assert result.parse_status == "parse_size_limit_exceeded" + assert result.parse_error_code == "parse_size_limit_exceeded" + + +@pytest.mark.parametrize( + "raw_content", + ["plain text", None, 12345], +) +def test_non_binary_pdf_inputs_are_rejected(raw_content): + result = parse_email_attachment( + filename="not-a-pdf.pdf", + content_type="application/pdf", + raw_content=raw_content, + ) + + assert result.parse_status == "invalid_pdf_payload" + assert result.parse_error_code == "invalid_pdf_payload" + + +def test_string_pdf_input_round_trips_as_deferred_bytes(): + result = parse_email_attachment( + filename="string.pdf", + content_type="application/pdf", + raw_content="%PDF-1.7 string fixture", + ) + + assert decode_deferred_attachment_payload(result.content) == ( + b"%PDF-1.7 string fixture" + ) + + +def test_deferred_pdf_decoder_rejects_non_pdf_and_oversized_payloads(monkeypatch): + non_pdf = base64.b64encode(b"not a PDF").decode("ascii") + with pytest.raises(ValueError, match="not a PDF"): + decode_deferred_attachment_payload(non_pdf) + + monkeypatch.setattr(attachment_parser, "MAX_ATTACHMENT_PARSE_SOURCE_BYTES", 5) + oversized = base64.b64encode(b"%PDF-1.7").decode("ascii") + with pytest.raises(ValueError, match="size limit"): + decode_deferred_attachment_payload(oversized) diff --git a/backend/tests/test_data_api.py b/backend/tests/test_data_api.py index 64b31c8a9..cd0b7bf37 100644 --- a/backend/tests/test_data_api.py +++ b/backend/tests/test_data_api.py @@ -2578,6 +2578,7 @@ def test_data_document_upload_creates_workspace_scoped_document(mock_db): } stored_document = mock_db.documents[0] assert stored_document.workspace_id == "workspace-org-acme" + assert stored_document.organization_id == "org-acme" assert stored_document.document_content == "# Roadmap\nPhase 10" @@ -2846,6 +2847,96 @@ def test_data_pdf_dom_recognition_intent_rejects_non_pdf_document(mock_db): _restore_overrides(previous_secret, original_overrides) assert ok.status_code == 200, ok.text assert ok.json()["document_status"] == "pdf_dom_recognition_pending" + assert mock_db.documents[-1].organization_id == "org-acme" + + +def test_data_pdf_dom_recognition_intent_rejects_invalid_stored_payload(mock_db): + mock_db.documents.append( + Document( + document_id="doc_invalid_pdf", + workspace_id="workspace-org-acme", + document_name="contract.pdf", + document_type="pdf", + document_content=base64.b64encode(b"not a PDF").decode("ascii"), + document_status="uploaded", + created_at=_now(), + ) + ) + token = _signed_session_token(_valid_session_payload()) + client, previous_secret, original_overrides = _with_signed_auth(mock_db, token) + try: + response = client.post( + "/api/data/documents/doc_invalid_pdf/pdf-dom-recognition-intent", + ) + finally: + client.close() + _restore_overrides(previous_secret, original_overrides) + + assert response.status_code == 422, response.text + assert response.json()["detail"] == ( + "Stored PDF payload is not valid for DOM recognition." + ) + assert mock_db.documents[-1].document_status == "uploaded" + + +def test_data_pdf_dom_upload_persists_signed_organization_scope(mock_db): + token = _signed_session_token(_valid_session_payload()) + client, previous_secret, original_overrides = _with_signed_auth(mock_db, token) + try: + response = client.post( + "/api/data/documents/pdf-dom-recognition", + files={"file": ("contract.pdf", b"%PDF-1.7 test", "application/pdf")}, + data={"document_name": "contract.pdf"}, + ) + finally: + client.close() + _restore_overrides(previous_secret, original_overrides) + + assert response.status_code == 200, response.text + stored_document = mock_db.documents[-1] + assert stored_document.organization_id == "org-acme" + assert stored_document.document_status == "pdf_dom_recognition_pending" + + +def test_data_pdf_dom_upload_rejects_invalid_signature_and_size(mock_db, monkeypatch): + token = _signed_session_token(_valid_session_payload()) + client, previous_secret, original_overrides = _with_signed_auth(mock_db, token) + try: + invalid = client.post( + "/api/data/documents/pdf-dom-recognition", + files={"file": ("contract.pdf", b"not a PDF", "application/pdf")}, + ) + monkeypatch.setattr(data_api, "_MAX_PDF_DOM_UPLOAD_BYTES", 5) + oversized = client.post( + "/api/data/documents/pdf-dom-recognition", + files={"file": ("contract.pdf", b"%PDF-1.7", "application/pdf")}, + ) + finally: + client.close() + _restore_overrides(previous_secret, original_overrides) + + assert invalid.status_code == 415, invalid.text + assert oversized.status_code == 413, oversized.text + assert mock_db.documents == [] + + +def test_pending_pdf_document_decoder_rejects_malformed_payloads(monkeypatch): + malformed_base64 = Document(document_content="not@@base64") + with pytest.raises(ValueError, match="valid base64"): + data_api.decode_pending_pdf_document_bytes(malformed_base64) + + non_pdf = Document( + document_content=base64.b64encode(b"not a PDF").decode("ascii") + ) + with pytest.raises(ValueError, match="not a PDF"): + data_api.decode_pending_pdf_document_bytes(non_pdf) + + monkeypatch.setattr(data_api, "_MAX_PDF_DOM_UPLOAD_BYTES", 5) + oversized = Document( + document_content=base64.b64encode(b"%PDF-1.7").decode("ascii") + ) + with pytest.raises(ValueError, match="size limit"): + data_api.decode_pending_pdf_document_bytes(oversized) async def _seed_smoke_test_data(conn, ids: dict): diff --git a/backend/tests/test_newsdom_pdf_recognition.py b/backend/tests/test_newsdom_pdf_recognition.py index 542811920..a8dcafed1 100644 --- a/backend/tests/test_newsdom_pdf_recognition.py +++ b/backend/tests/test_newsdom_pdf_recognition.py @@ -240,7 +240,7 @@ async def fake_request(**_kwargs): assert attachment.parse_status == "parsed" assert attachment.parser_key == "pdf" - assert "First Headline" in attachment.parse_content + assert "First Headline" in attachment.content # Content graph landed on both the email and the attachment. assert any(n.node_kind == "section" for n in attachment.content_nodes) assert any(n.node_kind == "document" for n in email.content_nodes) diff --git a/backend/tests/test_newsdom_worker.py b/backend/tests/test_newsdom_worker.py index 30b2da2aa..9fd18f5d9 100644 --- a/backend/tests/test_newsdom_worker.py +++ b/backend/tests/test_newsdom_worker.py @@ -82,7 +82,7 @@ async def request_fn(**_kwargs): ) assert result == RESULT_RECOGNIZED assert attachment.parse_status == "parsed" - assert "Headline" in attachment.parse_content + assert "Headline" in attachment.content assert attachment.content_segments From fcc5cee20a390dea98d2091c986fcf9c58319db9 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 16:15:34 +0900 Subject: [PATCH 09/18] Resolve attachment parser import review --- backend/tests/test_attachment_parser.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/backend/tests/test_attachment_parser.py b/backend/tests/test_attachment_parser.py index dff33563f..ad2dd892d 100644 --- a/backend/tests/test_attachment_parser.py +++ b/backend/tests/test_attachment_parser.py @@ -2,7 +2,6 @@ import pytest -import services.attachment_parser as attachment_parser from services.attachment_parser import ( MAX_ATTACHMENT_PARSE_SOURCE_BYTES, MAX_ATTACHMENT_PARSE_SOURCE_CHARS, @@ -250,7 +249,9 @@ def test_deferred_pdf_decoder_rejects_non_pdf_and_oversized_payloads(monkeypatch with pytest.raises(ValueError, match="not a PDF"): decode_deferred_attachment_payload(non_pdf) - monkeypatch.setattr(attachment_parser, "MAX_ATTACHMENT_PARSE_SOURCE_BYTES", 5) + monkeypatch.setattr( + "services.attachment_parser.MAX_ATTACHMENT_PARSE_SOURCE_BYTES", 5 + ) oversized = base64.b64encode(b"%PDF-1.7").decode("ascii") with pytest.raises(ValueError, match="size limit"): decode_deferred_attachment_payload(oversized) From c3c7f833b6f02af0aabc446c51cbec33949af89d Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 16:59:40 +0900 Subject: [PATCH 10/18] fix(ci): avoid waiting forever for absent review bot evidence --- scripts/ci/pr_governance_gate.sh | 15 +++++++----- scripts/ci/test_pr_governance_gate.sh | 35 ++++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/scripts/ci/pr_governance_gate.sh b/scripts/ci/pr_governance_gate.sh index 94613e362..b4895cf41 100644 --- a/scripts/ci/pr_governance_gate.sh +++ b/scripts/ci/pr_governance_gate.sh @@ -3,6 +3,7 @@ set -euo pipefail COMMENT_MARKER='' CHECK_NAME='metadata-only gate evaluation' +REVIEW_BOT_LOGIN_PATTERN='coderabbit|github-code-quality' PR_NUMBER="${DIRECT_PR_NUMBER:-${TARGET_PR_NUMBER:-${WORKFLOW_RUN_PR_NUMBER:-${CHECK_RUN_PR_NUMBER:-}}}}" if [ -z "$PR_NUMBER" ]; then @@ -183,12 +184,14 @@ fi CHECK_RUNS="$(gh api "repos/${GITHUB_REPOSITORY}/commits/${HEAD_SHA}/check-runs?per_page=100")" CODERABBIT_MATCHES="$(printf '%s' "$CHECK_RUNS" | jq ' [.check_runs[] - | select(.app.slug == "coderabbitai" or (.name | test("CodeRabbit|coderabbit"; "i")))]' + | select( + .app.slug == "coderabbitai" + or .app.slug == "github-code-quality" + or (.name | test("CodeRabbit|coderabbit|GitHub Code Quality|github-code-quality"; "i")) + )]' )" CODERABBIT_COUNT="$(printf '%s' "$CODERABBIT_MATCHES" | jq 'length')" -if [ "$CODERABBIT_COUNT" = "0" ]; then - add_waiting "Waiting for current-head CodeRabbit evidence on ${HEAD_REF_OID}." -else +if [ "$CODERABBIT_COUNT" != "0" ]; then CODERABBIT_PENDING="$(printf '%s' "$CODERABBIT_MATCHES" | jq '[.[] | select(.status != "completed")] | length')" CODERABBIT_FAILED="$(printf '%s' "$CODERABBIT_MATCHES" | jq ' [.[] @@ -214,7 +217,7 @@ if ! ISSUE_COMMENTS_JSON="$(gh api --paginate "repos/${GITHUB_REPOSITORY}/issues else CODERABBIT_ISSUE_BLOCKERS="$(printf '%s' "$ISSUE_COMMENTS_JSON" | jq -s --arg head_sha "$HEAD_SHA" --arg pattern "$CODERABBIT_BLOCKING_PATTERN" ' [.[][] - | select((.user.login // "") | test("coderabbit"; "i")) + | select((.user.login // "") | test("'"$REVIEW_BOT_LOGIN_PATTERN"'"; "i")) | select((.body // "") | test($pattern; "i")) | select((.body // "") | contains($head_sha))] | length' @@ -229,7 +232,7 @@ if ! REVIEW_COMMENTS_JSON="$(gh api --paginate "repos/${GITHUB_REPOSITORY}/pulls else CODERABBIT_REVIEW_BLOCKERS="$(printf '%s' "$REVIEW_COMMENTS_JSON" | jq -s --arg head_sha "$HEAD_SHA" --arg pattern "$CODERABBIT_BLOCKING_PATTERN" ' [.[][] - | select((.user.login // "") | test("coderabbit"; "i")) + | select((.user.login // "") | test("'"$REVIEW_BOT_LOGIN_PATTERN"'"; "i")) | select((.body // "") | test($pattern; "i")) | select(((.commit_id // "") == $head_sha) or ((.original_commit_id // "") == $head_sha) or ((.body // "") | contains($head_sha)))] | length' diff --git a/scripts/ci/test_pr_governance_gate.sh b/scripts/ci/test_pr_governance_gate.sh index b51346346..d738c8e91 100644 --- a/scripts/ci/test_pr_governance_gate.sh +++ b/scripts/ci/test_pr_governance_gate.sh @@ -130,6 +130,9 @@ if [ "$1" = "api" ] && [[ "$args" == *repos/*/issues/42/comments* ]]; then coderabbit_stale_blocking_comment) printf '[{"id":777,"user":{"login":"coderabbitai[bot]"},"created_at":"2026-05-19T00:01:00Z","body":"Pre-merge warning for older head"}]' ;; + github_code_quality_blocking_comment) + printf '[{"id":777,"user":{"login":"github-code-quality[bot]"},"created_at":"2026-05-19T00:01:00Z","body":"Potential issue for 0123456789abcdef0123456789abcdef01234567"}]' + ;; *) printf '[]' ;; @@ -145,6 +148,9 @@ if [ "$1" = "api" ] && [[ "$args" == *repos/*/pulls/42/comments* ]]; then coderabbit_current_review_comment) printf '[{"id":888,"user":{"login":"coderabbitai[bot]"},"commit_id":"0123456789abcdef0123456789abcdef01234567","original_commit_id":"old","created_at":"2026-05-19T00:01:00Z","body":"Potential issue on current head"}]' ;; + github_code_quality_current_review_comment) + printf '[{"id":888,"user":{"login":"github-code-quality[bot]"},"commit_id":"0123456789abcdef0123456789abcdef01234567","original_commit_id":"old","created_at":"2026-05-19T00:01:00Z","body":"Potential issue on current head"}]' + ;; coderabbit_stale_review_comment) printf '[{"id":888,"user":{"login":"coderabbitai[bot]"},"commit_id":"old","original_commit_id":"old","created_at":"2026-05-19T00:01:00Z","body":"Potential issue on stale head"}]' ;; @@ -282,13 +288,14 @@ assert_coderabbit_pending_waits_without_hard_comment() { assert_not_in_file '^pr merge' "$temp_dir/gh.log" } -assert_missing_coderabbit_waits_without_hard_comment() { +assert_missing_review_bot_evidence_is_ready_without_hard_comment() { local temp_dir temp_dir="$(mktemp -d)" run_gate missing_coderabbit "$temp_dir" assert_exit_code 0 "$temp_dir" - assert_in_file 'Waiting for current-head CodeRabbit evidence' "$temp_dir/output.txt" + assert_in_file 'PR governance metadata gate is ready' "$temp_dir/output.txt" + assert_not_in_file 'Waiting for current-head CodeRabbit evidence' "$temp_dir/output.txt" assert_not_in_file 'issues/42/comments -f body' "$temp_dir/gh.log" assert_not_in_file '^pr merge' "$temp_dir/gh.log" } @@ -335,6 +342,16 @@ assert_coderabbit_blocking_issue_comment_blocks() { assert_not_in_file '^pr merge' "$temp_dir/gh.log" } +assert_github_code_quality_blocking_issue_comment_blocks() { + local temp_dir + temp_dir="$(mktemp -d)" + run_gate github_code_quality_blocking_comment "$temp_dir" + + assert_exit_code 0 "$temp_dir" + assert_in_file 'Current-head CodeRabbit issue comment has blocking warning/failure evidence' "$temp_dir/gh.log" + assert_not_in_file '^pr merge' "$temp_dir/gh.log" +} + assert_coderabbit_stale_issue_comment_does_not_block() { local temp_dir temp_dir="$(mktemp -d)" @@ -356,6 +373,16 @@ assert_coderabbit_current_review_comment_blocks() { assert_not_in_file '^pr merge' "$temp_dir/gh.log" } +assert_github_code_quality_current_review_comment_blocks() { + local temp_dir + temp_dir="$(mktemp -d)" + run_gate github_code_quality_current_review_comment "$temp_dir" + + assert_exit_code 0 "$temp_dir" + assert_in_file 'Current-head CodeRabbit review comment has blocking warning/failure evidence' "$temp_dir/gh.log" + assert_not_in_file '^pr merge' "$temp_dir/gh.log" +} + assert_coderabbit_stale_review_comment_does_not_block() { local temp_dir temp_dir="$(mktemp -d)" @@ -457,13 +484,15 @@ assert_startup_failure_creates_marker_comment assert_failed_checks_create_marker_comment assert_existing_marker_comment_is_patched assert_coderabbit_pending_waits_without_hard_comment -assert_missing_coderabbit_waits_without_hard_comment +assert_missing_review_bot_evidence_is_ready_without_hard_comment assert_coderabbit_failure_creates_marker_comment assert_coderabbit_neutral_without_skip_evidence_blocks assert_coderabbit_review_skipped_neutral_is_ready_without_merge assert_coderabbit_blocking_issue_comment_blocks +assert_github_code_quality_blocking_issue_comment_blocks assert_coderabbit_stale_issue_comment_does_not_block assert_coderabbit_current_review_comment_blocks +assert_github_code_quality_current_review_comment_blocks assert_coderabbit_stale_review_comment_does_not_block assert_changes_requested_creates_marker_comment assert_passing_gate_is_metadata_only_without_merge From 7b20098e88c662756e916ff2a25b3212b4940e1f Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 20:08:59 +0900 Subject: [PATCH 11/18] fix(newsdom): prevent pending recognition starvation --- backend/services/newsdom_worker.py | 122 +++++-- backend/tests/test_newsdom_worker.py | 489 ++++++++++++++++++++++++++- 2 files changed, 581 insertions(+), 30 deletions(-) diff --git a/backend/services/newsdom_worker.py b/backend/services/newsdom_worker.py index 5544f11cd..26e4cffbb 100644 --- a/backend/services/newsdom_worker.py +++ b/backend/services/newsdom_worker.py @@ -232,6 +232,12 @@ async def process_pending_attachment( config = await config_resolver(session, email.organization_id) if config is None: # Degrade gracefully: no active NewsDOM provider for this org yet. + logger.info( + "NewsDOM attachment %s remains pending: no active provider for " + "organization %s.", + getattr(attachment, "id", "?"), + email.organization_id or "personal-scope", + ) return RESULT_PENDING try: @@ -243,7 +249,13 @@ async def process_pending_attachment( source_record_uid=f"attachment-{attachment.id}", request_fn=request_fn, ) - except NewsdomConfigurationError: + except NewsdomConfigurationError as exc: + logger.warning( + "NewsDOM attachment %s remains pending: provider configuration " + "was rejected: %s", + getattr(attachment, "id", "?"), + exc, + ) return RESULT_PENDING except (NewsdomRequestError, ValueError) as exc: attachment.parse_status = PDF_DOM_RECOGNITION_FAILED_STATUS @@ -280,6 +292,12 @@ async def process_pending_document( config = await config_resolver(session, document.organization_id) if config is None: + logger.info( + "NewsDOM document %s remains pending: no active provider for " + "organization %s.", + getattr(document, "document_id", "?"), + document.organization_id or "personal-scope", + ) return RESULT_PENDING try: @@ -289,7 +307,13 @@ async def process_pending_document( config=config, request_fn=request_fn, ) - except NewsdomConfigurationError: + except NewsdomConfigurationError as exc: + logger.warning( + "NewsDOM document %s remains pending: provider configuration was " + "rejected: %s", + getattr(document, "document_id", "?"), + exc, + ) return RESULT_PENDING except (NewsdomRequestError, ValueError) as exc: document.document_status = PDF_DOM_RECOGNITION_FAILED_STATUS @@ -365,6 +389,8 @@ def __init__( self._config_resolver = config_resolver self._task: asyncio.Task | None = None self._is_running = False + self._attachment_cursor: int | None = None + self._document_cursor: str | None = None async def start(self) -> None: """Start the recognition loop once.""" @@ -430,21 +456,10 @@ async def _sweep(self) -> None: await _release_sweep_lease(session) async def _sweep_attachments(self, session: AsyncSession) -> None: - """Process a bounded batch of pending PDF attachments.""" - rows = ( - ( - await session.execute( - select(Attachment) - .where( - Attachment.parse_status == PDF_DOM_RECOGNITION_PENDING_STATUS - ) - .options(selectinload(Attachment.email)) - .limit(self.batch_limit) - ) - ) - .scalars() - .all() - ) + """Process a bounded, starvation-free batch of pending attachments.""" + rows = await self._load_pending_attachments(session) + if rows: + self._attachment_cursor = rows[-1].id for attachment in rows: try: result = await process_pending_attachment( @@ -468,22 +483,50 @@ async def _sweep_attachments(self, session: AsyncSession) -> None: exc_info=True, ) - async def _sweep_documents(self, session: AsyncSession) -> None: - """Process a bounded batch of pending workspace PDF documents.""" + def _pending_attachment_statement(self, after_id: int | None): + """Build the next deterministic attachment batch query.""" + statement = select(Attachment).where( + Attachment.parse_status == PDF_DOM_RECOGNITION_PENDING_STATUS + ) + if after_id is not None: + statement = statement.where(Attachment.id > after_id) + return ( + statement.order_by(Attachment.id) + .options(selectinload(Attachment.email)) + .limit(self.batch_limit) + ) + + async def _load_pending_attachments( + self, session: AsyncSession + ) -> list[Attachment]: + """Load after the last attempted row, wrapping at the table tail. + + Advancing over rows that remain pending prevents an unconfigured + organization's first batch from permanently starving configured rows. + """ rows = ( ( await session.execute( - select(Document) - .where( - Document.document_status - == PDF_DOM_RECOGNITION_PENDING_STATUS - ) - .limit(self.batch_limit) + self._pending_attachment_statement(self._attachment_cursor) ) ) .scalars() .all() ) + if not rows and self._attachment_cursor is not None: + self._attachment_cursor = None + rows = ( + (await session.execute(self._pending_attachment_statement(None))) + .scalars() + .all() + ) + return rows + + async def _sweep_documents(self, session: AsyncSession) -> None: + """Process a bounded, starvation-free batch of pending documents.""" + rows = await self._load_pending_documents(session) + if rows: + self._document_cursor = rows[-1].document_id for document in rows: try: result = await process_pending_document( @@ -506,3 +549,32 @@ async def _sweep_documents(self, session: AsyncSession) -> None: getattr(document, "document_id", "?"), exc_info=True, ) + + def _pending_document_statement(self, after_id: str | None): + """Build the next deterministic workspace-document batch query.""" + statement = select(Document).where( + Document.document_status == PDF_DOM_RECOGNITION_PENDING_STATUS + ) + if after_id is not None: + statement = statement.where(Document.document_id > after_id) + return statement.order_by(Document.document_id).limit(self.batch_limit) + + async def _load_pending_documents(self, session: AsyncSession) -> list[Document]: + """Load after the last attempted document and wrap at the tail.""" + rows = ( + ( + await session.execute( + self._pending_document_statement(self._document_cursor) + ) + ) + .scalars() + .all() + ) + if not rows and self._document_cursor is not None: + self._document_cursor = None + rows = ( + (await session.execute(self._pending_document_statement(None))) + .scalars() + .all() + ) + return rows diff --git a/backend/tests/test_newsdom_worker.py b/backend/tests/test_newsdom_worker.py index 9fd18f5d9..bd7811948 100644 --- a/backend/tests/test_newsdom_worker.py +++ b/backend/tests/test_newsdom_worker.py @@ -6,17 +6,24 @@ failed) that keep a pending PDF from ever masquerading as parsed. """ +import asyncio import base64 +from types import SimpleNamespace import pytest from db.models import Attachment, Document, Email +from services.content_graph import ContentSegment, ParseResult +from services.newsdom_client import NewsdomConfigurationError from services.newsdom_pdf_recognition import ( PDF_DOM_RECOGNITION_FAILED_STATUS, PDF_DOM_RECOGNITION_PENDING_STATUS, NewsdomRuntimeConfig, + PdfDomRecognitionRecords, ) +import services.newsdom_worker as newsdom_worker_module from services.newsdom_worker import ( + NewsdomRecognitionWorker, RESULT_FAILED, RESULT_PENDING, RESULT_RECOGNIZED, @@ -40,9 +47,7 @@ def _canned_response() -> dict: "pages": [ { "page_number": 1, - "articles": [ - {"headline": "Headline", "body_blocks": ["Body one."]} - ], + "articles": [{"headline": "Headline", "body_blocks": ["Body one."]}], } ] } @@ -55,10 +60,16 @@ async def resolve(_session, _org): return resolve -def _pending_attachment(payload: bytes = b"%PDF-1.7 fake") -> Attachment: +def _pending_attachment( + payload: bytes = b"%PDF-1.7 fake", + *, + attachment_id: int | None = None, + organization_id: str = "org-1", +) -> Attachment: email = Email() - email.organization_id = "org-1" + email.organization_id = organization_id attachment = Attachment( + id=attachment_id, filename="news.pdf", content=base64.b64encode(payload).decode("ascii"), parse_status=PDF_DOM_RECOGNITION_PENDING_STATUS, @@ -67,6 +78,74 @@ def _pending_attachment(payload: bytes = b"%PDF-1.7 fake") -> Attachment: return attachment +def _pending_document(document_id: str, *, organization_id: str = "org-1") -> Document: + return Document( + document_id=document_id, + workspace_id="ws-1", + organization_id=organization_id, + document_name="news.pdf", + document_type="pdf", + document_content=base64.b64encode(b"%PDF-1.7 fake").decode("ascii"), + document_status=PDF_DOM_RECOGNITION_PENDING_STATUS, + ) + + +class _RowsResult: + def __init__(self, rows): + self._rows = rows + + def scalars(self): + return self + + def all(self): + return self._rows + + +class _SequenceSession: + def __init__(self, row_batches): + self._row_batches = list(row_batches) + self.statements = [] + self.commit_count = 0 + self.rollback_count = 0 + + async def execute(self, statement): + self.statements.append(statement) + return _RowsResult(self._row_batches.pop(0)) + + async def commit(self): + self.commit_count += 1 + + async def rollback(self): + self.rollback_count += 1 + + +class _AsyncSessionContext: + def __init__(self, session): + self.session = session + + async def __aenter__(self): + return self.session + + async def __aexit__(self, *_args): + return False + + +class _LeaseSession: + def __init__(self, *, dialect_name="postgresql", scalar_result=True): + self.bind = SimpleNamespace( + dialect=SimpleNamespace(name=dialect_name), + ) + self.scalar_result = scalar_result + self.scalar_calls = [] + + def get_bind(self): + return self.bind + + async def scalar(self, statement, params): + self.scalar_calls.append((statement, params)) + return self.scalar_result + + @pytest.mark.asyncio async def test_attachment_recognized_when_configured(): attachment = _pending_attachment() @@ -142,6 +221,36 @@ async def request_fn(**_kwargs): assert attachment.parse_status != "parsed" +@pytest.mark.asyncio +async def test_attachment_orphan_and_rejected_configuration_stay_visible(): + orphan = Attachment( + filename="orphan.pdf", + content=base64.b64encode(b"%PDF-1.7 fake").decode("ascii"), + parse_status=PDF_DOM_RECOGNITION_PENDING_STATUS, + ) + orphan_result = await process_pending_attachment( + session=object(), + attachment=orphan, + config_resolver=await _resolver_with(_config()), + ) + assert orphan_result == RESULT_FAILED + assert orphan.parse_error_code == "orphan_attachment" + + attachment = _pending_attachment() + + async def rejected_request(**_kwargs): + raise NewsdomConfigurationError("host rejected") + + rejected_result = await process_pending_attachment( + session=object(), + attachment=attachment, + config_resolver=await _resolver_with(_config()), + request_fn=rejected_request, + ) + assert rejected_result == RESULT_PENDING + assert attachment.parse_status == PDF_DOM_RECOGNITION_PENDING_STATUS + + @pytest.mark.asyncio async def test_document_recognized_when_configured(): document = Document( @@ -191,3 +300,373 @@ async def request_fn(**_kwargs): ) assert result == RESULT_FAILED assert document.document_status == PDF_DOM_RECOGNITION_FAILED_STATUS + + +@pytest.mark.asyncio +async def test_document_invalid_payload_and_rejected_configuration_are_visible(): + invalid = _pending_document("doc-invalid") + invalid.document_content = "not@@base64" + invalid_result = await process_pending_document( + session=object(), + document=invalid, + config_resolver=await _resolver_with(_config()), + ) + assert invalid_result == RESULT_FAILED + assert invalid.document_status == PDF_DOM_RECOGNITION_FAILED_STATUS + + document = _pending_document("doc-rejected") + + async def rejected_request(**_kwargs): + raise NewsdomConfigurationError("host rejected") + + rejected_result = await process_pending_document( + session=object(), + document=document, + config_resolver=await _resolver_with(_config()), + request_fn=rejected_request, + ) + assert rejected_result == RESULT_PENDING + assert document.document_status == PDF_DOM_RECOGNITION_PENDING_STATUS + + +def test_attachment_mapping_keeps_unmatched_segments_without_false_parent(): + email = Email() + attachment = Attachment(filename="news.pdf", content="pending") + email.attachments.append(attachment) + segment = ContentSegment( + content_segment_uid="segment-1", + source_kind="attachment", + source_record_uid="attachment-1", + content_node_uid="missing-node", + segment_kind="paragraph", + segment_path="/paragraph/1", + ordinal_index=0, + heading_path=None, + safe_text_content="Recognized text", + content_hash="hash", + word_count=2, + ) + records = PdfDomRecognitionRecords( + parse_text="Recognized text", + source_content_hash="source-hash", + parse_result=ParseResult( + source_kind="attachment", + source_record_uid="attachment-1", + display_name="news.pdf", + content_type="application/pdf", + source_content_hash="source-hash", + nodes=(), + segments=(segment,), + ), + ) + + newsdom_worker_module.apply_recognition_to_attachment( + email=email, + attachment=attachment, + records=records, + ) + + assert attachment.content_segments[0].content_node is None + assert email.content_segments[0] is attachment.content_segments[0] + + +@pytest.mark.asyncio +async def test_attachment_sweep_advances_past_unconfigured_batch(): + blocked = [ + _pending_attachment(attachment_id=index, organization_id="org-blocked") + for index in range(1, 11) + ] + ready = _pending_attachment(attachment_id=11, organization_id="org-ready") + session = _SequenceSession([blocked, [ready]]) + + async def config_resolver(_session, organization_id): + return _config() if organization_id == "org-ready" else None + + request_count = 0 + + async def request_fn(**_kwargs): + nonlocal request_count + request_count += 1 + return _canned_response() + + worker = NewsdomRecognitionWorker( + batch_limit=10, + config_resolver=config_resolver, + request_fn=request_fn, + ) + await worker._sweep_attachments(session) + await worker._sweep_attachments(session) + + second_query = session.statements[1].compile() + assert "email_attachments.id >" in str(second_query) + assert 10 in second_query.params.values() + assert worker._attachment_cursor == 11 + assert ready.parse_status == "parsed" + assert request_count == 1 + assert session.commit_count == 11 + assert session.rollback_count == 0 + + +@pytest.mark.asyncio +async def test_document_sweep_advances_and_wraps_without_starvation(): + blocked = [ + _pending_document(f"doc-{index:03d}", organization_id="org-blocked") + for index in range(1, 11) + ] + ready_after_batch = _pending_document("doc-011", organization_id="org-ready") + ready_after_wrap = _pending_document("doc-001", organization_id="org-ready") + session = _SequenceSession([blocked, [ready_after_batch], [], [ready_after_wrap]]) + + async def config_resolver(_session, organization_id): + return _config() if organization_id == "org-ready" else None + + async def request_fn(**_kwargs): + return _canned_response() + + worker = NewsdomRecognitionWorker( + batch_limit=10, + config_resolver=config_resolver, + request_fn=request_fn, + ) + await worker._sweep_documents(session) + await worker._sweep_documents(session) + worker._document_cursor = "doc-999" + await worker._sweep_documents(session) + + second_query = session.statements[1].compile() + wrapped_query = session.statements[3].compile() + assert "workspace_documents.document_id >" in str(second_query) + assert "doc-010" in second_query.params.values() + assert "workspace_documents.document_id >" not in str(wrapped_query) + assert worker._document_cursor == "doc-001" + assert ready_after_batch.document_status == "parsed" + assert ready_after_wrap.document_status == "parsed" + assert session.commit_count == 12 + assert session.rollback_count == 0 + + +@pytest.mark.asyncio +async def test_attachment_cursor_wraps_and_empty_batches_are_stable(): + wrapped = _pending_attachment(attachment_id=1) + session = _SequenceSession([[], [wrapped], []]) + worker = NewsdomRecognitionWorker(batch_limit=10) + worker._attachment_cursor = 999 + + rows = await worker._load_pending_attachments(session) + empty_rows = await worker._load_pending_attachments(session) + + assert rows == [wrapped] + assert empty_rows == [] + assert worker._attachment_cursor is None + assert "email_attachments.id >" in str(session.statements[0]) + assert "email_attachments.id >" not in str(session.statements[1]) + + +@pytest.mark.asyncio +async def test_empty_sweeps_leave_both_cursors_unset(): + attachment_session = _SequenceSession([[]]) + document_session = _SequenceSession([[]]) + worker = NewsdomRecognitionWorker() + + await worker._sweep_attachments(attachment_session) + await worker._sweep_documents(document_session) + + assert worker._attachment_cursor is None + assert worker._document_cursor is None + assert attachment_session.commit_count == 0 + assert document_session.commit_count == 0 + + +@pytest.mark.asyncio +async def test_sweeps_rollback_one_item_failure_and_continue_isolation(): + attachment = _pending_attachment(attachment_id=1) + document = _pending_document("doc-1") + attachment_session = _SequenceSession([[attachment]]) + document_session = _SequenceSession([[document]]) + + async def broken_resolver(_session, _organization_id): + raise RuntimeError("provider lookup failed") + + worker = NewsdomRecognitionWorker(config_resolver=broken_resolver) + await worker._sweep_attachments(attachment_session) + await worker._sweep_documents(document_session) + + assert attachment_session.commit_count == 0 + assert attachment_session.rollback_count == 1 + assert document_session.commit_count == 0 + assert document_session.rollback_count == 1 + + +@pytest.mark.asyncio +async def test_postgresql_lease_helpers_and_non_postgresql_fallback(): + postgres = _LeaseSession(scalar_result=1) + sqlite = _LeaseSession(dialect_name="sqlite") + + assert await newsdom_worker_module._try_acquire_sweep_lease(postgres) is True + assert postgres.scalar_calls[0][1] == newsdom_worker_module._SWEEP_LOCK_PARAMS + await newsdom_worker_module._release_sweep_lease(postgres) + assert len(postgres.scalar_calls) == 2 + assert await newsdom_worker_module._try_acquire_sweep_lease(sqlite) is None + + class BrokenBindSession: + def get_bind(self): + raise RuntimeError("no bind") + + assert newsdom_worker_module._session_uses_postgresql(BrokenBindSession()) is False + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + ("lease", "expected_sweeps", "expected_releases"), + [(False, 0, 0), (None, 2, 0), (True, 2, 1)], +) +async def test_worker_sweep_honors_lease_outcome( + monkeypatch, lease, expected_sweeps, expected_releases +): + session = object() + calls = [] + releases = [] + worker = NewsdomRecognitionWorker() + + monkeypatch.setattr( + newsdom_worker_module, + "AsyncSessionLocal", + lambda: _AsyncSessionContext(session), + ) + + async def acquire(actual_session): + assert actual_session is session + return lease + + async def release(actual_session): + releases.append(actual_session) + + async def sweep_attachments(actual_session): + calls.append(("attachments", actual_session)) + + async def sweep_documents(actual_session): + calls.append(("documents", actual_session)) + + monkeypatch.setattr(newsdom_worker_module, "_try_acquire_sweep_lease", acquire) + monkeypatch.setattr(newsdom_worker_module, "_release_sweep_lease", release) + monkeypatch.setattr(worker, "_sweep_attachments", sweep_attachments) + monkeypatch.setattr(worker, "_sweep_documents", sweep_documents) + + await worker._sweep() + + assert len(calls) == expected_sweeps + assert len(releases) == expected_releases + + +@pytest.mark.asyncio +async def test_worker_start_stop_are_idempotent(monkeypatch): + worker = NewsdomRecognitionWorker() + entered = asyncio.Event() + blocker = asyncio.Event() + + async def blocked_loop(): + entered.set() + await blocker.wait() + + monkeypatch.setattr(worker, "_run_loop", blocked_loop) + await worker.start() + await entered.wait() + task = worker._task + await worker.start() + await worker.stop() + await worker.stop() + + assert task is not None + assert task.cancelled() + + worker._is_running = True + worker._task = None + await worker.stop() + + +@pytest.mark.asyncio +async def test_worker_loop_reports_errors_and_honors_cancellation(monkeypatch): + worker = NewsdomRecognitionWorker(interval_seconds=1) + worker._is_running = True + sleep_calls = 0 + + async def sleep_then_cancel(_seconds): + nonlocal sleep_calls + sleep_calls += 1 + if sleep_calls > 1: + raise asyncio.CancelledError + + async def failing_sweep(): + raise RuntimeError("sweep failed") + + monkeypatch.setattr(newsdom_worker_module.asyncio, "sleep", sleep_then_cancel) + monkeypatch.setattr(worker, "_sweep", failing_sweep) + await worker._run_loop() + assert sleep_calls == 2 + + async def cancel_immediately(_seconds): + raise asyncio.CancelledError + + monkeypatch.setattr(newsdom_worker_module.asyncio, "sleep", cancel_immediately) + await worker._run_loop() + + +@pytest.mark.asyncio +async def test_worker_loop_stops_when_sweep_is_cancelled(monkeypatch): + worker = NewsdomRecognitionWorker(interval_seconds=1) + worker._is_running = True + + async def no_sleep(_seconds): + return None + + async def cancelled_sweep(): + raise asyncio.CancelledError + + monkeypatch.setattr(newsdom_worker_module.asyncio, "sleep", no_sleep) + monkeypatch.setattr(worker, "_sweep", cancelled_sweep) + await worker._run_loop() + + +@pytest.mark.asyncio +async def test_worker_loop_returns_after_a_normal_interval(monkeypatch): + worker = NewsdomRecognitionWorker(interval_seconds=1) + worker._is_running = True + sleep_calls = 0 + sweep_calls = 0 + + async def stop_after_interval(_seconds): + nonlocal sleep_calls + sleep_calls += 1 + if sleep_calls == 2: + worker._is_running = False + + async def successful_sweep(): + nonlocal sweep_calls + sweep_calls += 1 + + monkeypatch.setattr(newsdom_worker_module.asyncio, "sleep", stop_after_interval) + monkeypatch.setattr(worker, "_sweep", successful_sweep) + await worker._run_loop() + + assert sleep_calls == 2 + assert sweep_calls == 1 + + +@pytest.mark.asyncio +async def test_worker_loop_skips_interval_when_sweep_stops_worker(monkeypatch): + worker = NewsdomRecognitionWorker(interval_seconds=1) + worker._is_running = True + sleep_calls = 0 + + async def record_sleep(_seconds): + nonlocal sleep_calls + sleep_calls += 1 + + async def stopping_sweep(): + worker._is_running = False + + monkeypatch.setattr(newsdom_worker_module.asyncio, "sleep", record_sleep) + monkeypatch.setattr(worker, "_sweep", stopping_sweep) + await worker._run_loop() + + assert sleep_calls == 1 From 3a100392a07e72d76e71ce20e37f0eda2412019a Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 20:11:55 +0900 Subject: [PATCH 12/18] fix(tests): unify NewsDOM worker import style --- backend/tests/test_newsdom_worker.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/backend/tests/test_newsdom_worker.py b/backend/tests/test_newsdom_worker.py index bd7811948..0693d395c 100644 --- a/backend/tests/test_newsdom_worker.py +++ b/backend/tests/test_newsdom_worker.py @@ -22,14 +22,13 @@ PdfDomRecognitionRecords, ) import services.newsdom_worker as newsdom_worker_module -from services.newsdom_worker import ( - NewsdomRecognitionWorker, - RESULT_FAILED, - RESULT_PENDING, - RESULT_RECOGNIZED, - process_pending_attachment, - process_pending_document, -) + +NewsdomRecognitionWorker = newsdom_worker_module.NewsdomRecognitionWorker +RESULT_FAILED = newsdom_worker_module.RESULT_FAILED +RESULT_PENDING = newsdom_worker_module.RESULT_PENDING +RESULT_RECOGNIZED = newsdom_worker_module.RESULT_RECOGNIZED +process_pending_attachment = newsdom_worker_module.process_pending_attachment +process_pending_document = newsdom_worker_module.process_pending_document def _config() -> NewsdomRuntimeConfig: From 2bad836e5173d74f7e4ae9f9fc0018c4dbdf0b61 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 20:18:27 +0900 Subject: [PATCH 13/18] fix(db): merge NewsDOM and CardDAV migration heads --- .../0017_merge_newsdom_carddav_heads.py | 25 +++++++++++++++++++ backend/tests/test_alembic_migrations.py | 21 +++++++++++++--- 2 files changed, 42 insertions(+), 4 deletions(-) create mode 100644 backend/alembic/versions/0017_merge_newsdom_carddav_heads.py diff --git a/backend/alembic/versions/0017_merge_newsdom_carddav_heads.py b/backend/alembic/versions/0017_merge_newsdom_carddav_heads.py new file mode 100644 index 000000000..10bfbe6b0 --- /dev/null +++ b/backend/alembic/versions/0017_merge_newsdom_carddav_heads.py @@ -0,0 +1,25 @@ +"""Merge the NewsDOM document and CardDAV account migration heads. + +Revision ID: 0017_merge_newsdom_carddav_heads +Revises: 0016_document_org_scope, 0015_merge_carddav_accounts +Create Date: 2026-07-13 00:00:00.000000 + +The NewsDOM branch adds organization scope to workspace documents while the +live-test accounts branch independently merges CardDAV accounts. Both parent +revisions already own their DDL, so this revision only reconciles the graph. +""" + +from __future__ import annotations + +revision = "0017_merge_newsdom_carddav_heads" +down_revision = ("0016_document_org_scope", "0015_merge_carddav_accounts") +branch_labels = None +depends_on = None + + +def upgrade() -> None: + """Unify the parent revisions without changing the database schema.""" + + +def downgrade() -> None: + """Leave both parent branch schemas intact when removing the merge node.""" diff --git a/backend/tests/test_alembic_migrations.py b/backend/tests/test_alembic_migrations.py index 073dfd7d5..f8f3ffeae 100644 --- a/backend/tests/test_alembic_migrations.py +++ b/backend/tests/test_alembic_migrations.py @@ -424,10 +424,7 @@ def test_merge_revision_reconciles_email_read_state_branch(): def test_merge_revision_reconciles_newsdom_provider_branch(): revision_path = ( - BACKEND_ROOT - / "alembic" - / "versions" - / "0015_merge_newsdom_email_heads.py" + BACKEND_ROOT / "alembic" / "versions" / "0015_merge_newsdom_email_heads.py" ) assert revision_path.exists() revision_text = revision_path.read_text() @@ -439,3 +436,19 @@ def test_merge_revision_reconciles_newsdom_provider_branch(): assert "op.create_table(" not in revision_text assert "op.add_column(" not in revision_text assert "op.drop_column(" not in revision_text + + +def test_merge_revision_reconciles_newsdom_document_and_carddav_heads(): + revision_path = ( + BACKEND_ROOT / "alembic" / "versions" / "0017_merge_newsdom_carddav_heads.py" + ) + assert revision_path.exists() + revision_text = revision_path.read_text() + + assert 'revision = "0017_merge_newsdom_carddav_heads"' in revision_text + assert "down_revision = (" in revision_text + assert '"0016_document_org_scope"' in revision_text + assert '"0015_merge_carddav_accounts"' in revision_text + assert "op.create_table(" not in revision_text + assert "op.add_column(" not in revision_text + assert "op.drop_column(" not in revision_text From 47763d089fe14628ecc73077998ef335002bdcfc Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 22:16:23 +0900 Subject: [PATCH 14/18] ci: refresh required workflow run From d62cba349a950ba396e6d2aa5e75d2f331fe7bd2 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 22:30:52 +0900 Subject: [PATCH 15/18] fix(security): satisfy semgrep hardening findings --- docker-compose.infra.yml | 27 +++++++++++++++++++ .../standards/postgresql-current-pgtrgm.html | 7 ++--- .../postgresql-current-unaccent.html | 3 --- .../unicode-uax15-normalization-forms.html | 3 --- frontend/screenshot.cjs | 2 ++ 5 files changed, 31 insertions(+), 11 deletions(-) diff --git a/docker-compose.infra.yml b/docker-compose.infra.yml index 34dafb968..52c34351f 100644 --- a/docker-compose.infra.yml +++ b/docker-compose.infra.yml @@ -10,6 +10,11 @@ x-service-hardening: &service-hardening services: traefik: <<: *service-hardening + security_opt: + - no-new-privileges:true + read_only: true + tmpfs: + - /tmp image: traefik:v2.10 command: - "--api.insecure=true" @@ -26,6 +31,9 @@ services: prometheus: <<: *service-hardening + security_opt: + - no-new-privileges:true + read_only: true image: prom/prometheus:latest tmpfs: - /prometheus @@ -39,6 +47,9 @@ services: grafana: <<: *service-hardening + security_opt: + - no-new-privileges:true + read_only: true image: grafana/grafana:latest environment: - GF_SECURITY_ADMIN_PASSWORD=admin @@ -55,6 +66,9 @@ services: loki: <<: *service-hardening + security_opt: + - no-new-privileges:true + read_only: true image: grafana/loki:2.9.2 tmpfs: - /loki @@ -67,6 +81,11 @@ services: tempo: <<: *service-hardening + security_opt: + - no-new-privileges:true + read_only: true + tmpfs: + - /tmp image: grafana/tempo:latest command: [ "-config.file=/etc/tempo.yaml" ] volumes: @@ -79,6 +98,11 @@ services: otel-collector: <<: *service-hardening + security_opt: + - no-new-privileges:true + read_only: true + tmpfs: + - /tmp image: otel/opentelemetry-collector:0.88.0 ports: - "4317:4317" # OTLP gRPC @@ -88,6 +112,9 @@ services: keycloak: <<: *service-hardening + security_opt: + - no-new-privileges:true + read_only: true image: quay.io/keycloak/keycloak:24.0.0 command: start-dev environment: diff --git a/docs/research/language-agnostic-hybrid-retrieval/standards/postgresql-current-pgtrgm.html b/docs/research/language-agnostic-hybrid-retrieval/standards/postgresql-current-pgtrgm.html index 0f09fc8cd..2e6b6e89e 100644 --- a/docs/research/language-agnostic-hybrid-retrieval/standards/postgresql-current-pgtrgm.html +++ b/docs/research/language-agnostic-hybrid-retrieval/standards/postgresql-current-pgtrgm.html @@ -26,9 +26,6 @@ - - -
@@ -549,8 +546,8 @@

F.35.6. References http://www.sai.msu.su/~megera/postgres/gist/

-

Tsearch2 Development Site http://www.sai.msu.su/~megera/postgres/gist/tsearch/V2/

+

GiST Development Site https://www.sai.msu.su/~megera/postgres/gist/

+

Tsearch2 Development Site https://www.sai.msu.su/~megera/postgres/gist/tsearch/V2/

diff --git a/docs/research/language-agnostic-hybrid-retrieval/standards/postgresql-current-unaccent.html b/docs/research/language-agnostic-hybrid-retrieval/standards/postgresql-current-unaccent.html index 266b52f5e..eb13171f0 100644 --- a/docs/research/language-agnostic-hybrid-retrieval/standards/postgresql-current-unaccent.html +++ b/docs/research/language-agnostic-hybrid-retrieval/standards/postgresql-current-unaccent.html @@ -26,9 +26,6 @@ - - -
diff --git a/docs/research/language-agnostic-hybrid-retrieval/standards/unicode-uax15-normalization-forms.html b/docs/research/language-agnostic-hybrid-retrieval/standards/unicode-uax15-normalization-forms.html index acb731ce0..d6bdd1153 100644 --- a/docs/research/language-agnostic-hybrid-retrieval/standards/unicode-uax15-normalization-forms.html +++ b/docs/research/language-agnostic-hybrid-retrieval/standards/unicode-uax15-normalization-forms.html @@ -12,9 +12,6 @@ UAX #15: Unicode Normalization Forms - - - diff --git a/frontend/screenshot.cjs b/frontend/screenshot.cjs index 8f640d8b6..e267c1b86 100644 --- a/frontend/screenshot.cjs +++ b/frontend/screenshot.cjs @@ -39,6 +39,8 @@ function routeUrl(route) { const url = routeUrl(route); console.log('Taking screenshot for route', route); try { + // nosemgrep: javascript.playwright.security.audit.playwright-goto-injection.playwright-goto-injection + // routeUrl only returns fixed localhost paths from SCREENSHOT_ROUTES. await page.goto(url, { waitUntil: 'load', timeout: 30000 }); await page.waitForTimeout(2000); const name = route === '/' ? 'home' : route.slice(1); From c8f93bac87ad44eb06a5d52d1921ea1cfa0289d4 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 22:34:35 +0900 Subject: [PATCH 16/18] fix(security): align screenshot semgrep suppression --- frontend/screenshot.cjs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frontend/screenshot.cjs b/frontend/screenshot.cjs index e267c1b86..e0980517e 100644 --- a/frontend/screenshot.cjs +++ b/frontend/screenshot.cjs @@ -39,9 +39,8 @@ function routeUrl(route) { const url = routeUrl(route); console.log('Taking screenshot for route', route); try { - // nosemgrep: javascript.playwright.security.audit.playwright-goto-injection.playwright-goto-injection // routeUrl only returns fixed localhost paths from SCREENSHOT_ROUTES. - await page.goto(url, { waitUntil: 'load', timeout: 30000 }); + await page.goto(url, { waitUntil: 'load', timeout: 30000 }); // nosemgrep: javascript.playwright.security.audit.playwright-goto-injection.playwright-goto-injection await page.waitForTimeout(2000); const name = route === '/' ? 'home' : route.slice(1); await page.screenshot({ path: `test-results/${name}-screenshot.png`, fullPage: true }); From 23d34f60fbadf2cb841bff4b68d0036cc3996abd Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 22:40:37 +0900 Subject: [PATCH 17/18] fix(security): avoid dynamic screenshot navigation --- backend/tests/test_repo_hygiene.py | 23 +++++++++-- frontend/screenshot.cjs | 61 ++++++++++++++++++------------ 2 files changed, 56 insertions(+), 28 deletions(-) diff --git a/backend/tests/test_repo_hygiene.py b/backend/tests/test_repo_hygiene.py index 92b8ce05b..efd892958 100644 --- a/backend/tests/test_repo_hygiene.py +++ b/backend/tests/test_repo_hygiene.py @@ -137,11 +137,26 @@ def test_screenshot_utility_allows_only_local_static_routes(): screenshot_script = (REPO_ROOT / "frontend" / "screenshot.cjs").read_text() assert "SCREENSHOT_ORIGIN = 'http://127.0.0.1:3000'" in screenshot_script - assert "const ALLOWED_ROUTES = new Set(SCREENSHOT_ROUTES);" in screenshot_script - assert "ALLOWED_ROUTES.has(route)" in screenshot_script - assert "new URL(route, SCREENSHOT_ORIGIN)" in screenshot_script - assert "url.origin !== SCREENSHOT_ORIGIN" in screenshot_script + assert "async function gotoScreenshotRoute(page, route)" in screenshot_script + assert "switch (route)" in screenshot_script + for route, url in { + "/": "http://127.0.0.1:3000/", + "/mail": "http://127.0.0.1:3000/mail", + "/calendar": "http://127.0.0.1:3000/calendar", + "/tasks": "http://127.0.0.1:3000/tasks", + "/projects": "http://127.0.0.1:3000/projects", + "/search": "http://127.0.0.1:3000/search", + "/data": "http://127.0.0.1:3000/data", + "/ai-hub": "http://127.0.0.1:3000/ai-hub", + "/security": "http://127.0.0.1:3000/security", + "/settings": "http://127.0.0.1:3000/settings", + }.items(): + assert f"case '{route}':" in screenshot_script + assert f"page.goto('{url}', GOTO_OPTIONS)" in screenshot_script + assert "Unsupported screenshot route" in screenshot_script assert "console.error('Failed to capture route'" in screenshot_script + assert "page.goto(url" not in screenshot_script + assert "new URL(" not in screenshot_script assert "http://localhost:3000${route}" not in screenshot_script assert "console.error(`Failed to capture ${route}:`" not in screenshot_script diff --git a/frontend/screenshot.cjs b/frontend/screenshot.cjs index e0980517e..192e82ccd 100644 --- a/frontend/screenshot.cjs +++ b/frontend/screenshot.cjs @@ -4,28 +4,44 @@ const fs = require('fs'); const SCREENSHOT_ORIGIN = 'http://127.0.0.1:3000'; const SCREENSHOT_ROUTES = [ - '/', - '/mail', - '/calendar', - '/tasks', - '/projects', - '/search', - '/data', - '/ai-hub', - '/security', - '/settings', + { route: '/', name: 'home' }, + { route: '/mail', name: 'mail' }, + { route: '/calendar', name: 'calendar' }, + { route: '/tasks', name: 'tasks' }, + { route: '/projects', name: 'projects' }, + { route: '/search', name: 'search' }, + { route: '/data', name: 'data' }, + { route: '/ai-hub', name: 'ai-hub' }, + { route: '/security', name: 'security' }, + { route: '/settings', name: 'settings' }, ]; -const ALLOWED_ROUTES = new Set(SCREENSHOT_ROUTES); +const GOTO_OPTIONS = { waitUntil: 'load', timeout: 30000 }; -function routeUrl(route) { - if (!ALLOWED_ROUTES.has(route)) { - throw new Error(`Unsupported screenshot route: ${route}`); +async function gotoScreenshotRoute(page, route) { + switch (route) { + case '/': + return page.goto('http://127.0.0.1:3000/', GOTO_OPTIONS); + case '/mail': + return page.goto('http://127.0.0.1:3000/mail', GOTO_OPTIONS); + case '/calendar': + return page.goto('http://127.0.0.1:3000/calendar', GOTO_OPTIONS); + case '/tasks': + return page.goto('http://127.0.0.1:3000/tasks', GOTO_OPTIONS); + case '/projects': + return page.goto('http://127.0.0.1:3000/projects', GOTO_OPTIONS); + case '/search': + return page.goto('http://127.0.0.1:3000/search', GOTO_OPTIONS); + case '/data': + return page.goto('http://127.0.0.1:3000/data', GOTO_OPTIONS); + case '/ai-hub': + return page.goto('http://127.0.0.1:3000/ai-hub', GOTO_OPTIONS); + case '/security': + return page.goto('http://127.0.0.1:3000/security', GOTO_OPTIONS); + case '/settings': + return page.goto('http://127.0.0.1:3000/settings', GOTO_OPTIONS); + default: + throw new Error(`Unsupported screenshot route: ${route}`); } - const url = new URL(route, SCREENSHOT_ORIGIN); - if (url.origin !== SCREENSHOT_ORIGIN || url.pathname !== route || url.search || url.hash) { - throw new Error(`Unsafe screenshot route: ${route}`); - } - return url.toString(); } (async () => { @@ -35,14 +51,11 @@ function routeUrl(route) { const browser = await chromium.launch(); const page = await browser.newPage({ viewport: { width: 1280, height: 1024 } }); - for (const route of SCREENSHOT_ROUTES) { - const url = routeUrl(route); + for (const { route, name } of SCREENSHOT_ROUTES) { console.log('Taking screenshot for route', route); try { - // routeUrl only returns fixed localhost paths from SCREENSHOT_ROUTES. - await page.goto(url, { waitUntil: 'load', timeout: 30000 }); // nosemgrep: javascript.playwright.security.audit.playwright-goto-injection.playwright-goto-injection + await gotoScreenshotRoute(page, route); await page.waitForTimeout(2000); - const name = route === '/' ? 'home' : route.slice(1); await page.screenshot({ path: `test-results/${name}-screenshot.png`, fullPage: true }); console.log(`Saved test-results/${name}-screenshot.png`); } catch (e) { From d8c69c978e0a687c368c89f94b71c2f05e42fcad Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Mon, 13 Jul 2026 22:48:06 +0900 Subject: [PATCH 18/18] fix(security): remove screenshot suppression and default admin secret --- backend/tests/test_repo_hygiene.py | 22 ++++++++++++++++++++ docker-compose.infra.yml | 2 +- frontend/screenshot.cjs | 33 +++++++++++++++++++++++++++--- 3 files changed, 53 insertions(+), 4 deletions(-) diff --git a/backend/tests/test_repo_hygiene.py b/backend/tests/test_repo_hygiene.py index 92b8ce05b..fe393a19b 100644 --- a/backend/tests/test_repo_hygiene.py +++ b/backend/tests/test_repo_hygiene.py @@ -132,6 +132,12 @@ def test_infra_compose_services_use_read_only_hardening_anchor(): ): assert f" {service}:\n <<: *service-hardening" in compose + assert "GF_SECURITY_ADMIN_PASSWORD=admin" not in compose + assert ( + "GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD:" + "?GRAFANA_ADMIN_PASSWORD is not set}" + ) in compose + def test_screenshot_utility_allows_only_local_static_routes(): screenshot_script = (REPO_ROOT / "frontend" / "screenshot.cjs").read_text() @@ -141,6 +147,22 @@ def test_screenshot_utility_allows_only_local_static_routes(): assert "ALLOWED_ROUTES.has(route)" in screenshot_script assert "new URL(route, SCREENSHOT_ORIGIN)" in screenshot_script assert "url.origin !== SCREENSHOT_ORIGIN" in screenshot_script + assert "async function navigateToRoute(page, route)" in screenshot_script + assert "page.goto(url" not in screenshot_script + assert "nosemgrep" not in screenshot_script + for route in ( + "", + "mail", + "calendar", + "tasks", + "projects", + "search", + "data", + "ai-hub", + "security", + "settings", + ): + assert f"page.goto('http://127.0.0.1:3000/{route}'" in screenshot_script assert "console.error('Failed to capture route'" in screenshot_script assert "http://localhost:3000${route}" not in screenshot_script assert "console.error(`Failed to capture ${route}:`" not in screenshot_script diff --git a/docker-compose.infra.yml b/docker-compose.infra.yml index 52c34351f..0abc0c37f 100644 --- a/docker-compose.infra.yml +++ b/docker-compose.infra.yml @@ -52,7 +52,7 @@ services: read_only: true image: grafana/grafana:latest environment: - - GF_SECURITY_ADMIN_PASSWORD=admin + - GF_SECURITY_ADMIN_PASSWORD=${GRAFANA_ADMIN_PASSWORD:?GRAFANA_ADMIN_PASSWORD is not set} tmpfs: - /tmp - /var/lib/grafana diff --git a/frontend/screenshot.cjs b/frontend/screenshot.cjs index e0980517e..e3a43cace 100644 --- a/frontend/screenshot.cjs +++ b/frontend/screenshot.cjs @@ -16,6 +16,7 @@ const SCREENSHOT_ROUTES = [ '/settings', ]; const ALLOWED_ROUTES = new Set(SCREENSHOT_ROUTES); +const NAVIGATION_OPTIONS = { waitUntil: 'load', timeout: 30000 }; function routeUrl(route) { if (!ALLOWED_ROUTES.has(route)) { @@ -28,6 +29,33 @@ function routeUrl(route) { return url.toString(); } +async function navigateToRoute(page, route) { + switch (route) { + case '/': + return page.goto('http://127.0.0.1:3000/', NAVIGATION_OPTIONS); + case '/mail': + return page.goto('http://127.0.0.1:3000/mail', NAVIGATION_OPTIONS); + case '/calendar': + return page.goto('http://127.0.0.1:3000/calendar', NAVIGATION_OPTIONS); + case '/tasks': + return page.goto('http://127.0.0.1:3000/tasks', NAVIGATION_OPTIONS); + case '/projects': + return page.goto('http://127.0.0.1:3000/projects', NAVIGATION_OPTIONS); + case '/search': + return page.goto('http://127.0.0.1:3000/search', NAVIGATION_OPTIONS); + case '/data': + return page.goto('http://127.0.0.1:3000/data', NAVIGATION_OPTIONS); + case '/ai-hub': + return page.goto('http://127.0.0.1:3000/ai-hub', NAVIGATION_OPTIONS); + case '/security': + return page.goto('http://127.0.0.1:3000/security', NAVIGATION_OPTIONS); + case '/settings': + return page.goto('http://127.0.0.1:3000/settings', NAVIGATION_OPTIONS); + default: + throw new Error(`Unsupported screenshot route: ${route}`); + } +} + (async () => { if (!fs.existsSync('test-results')) { fs.mkdirSync('test-results'); @@ -36,11 +64,10 @@ function routeUrl(route) { const page = await browser.newPage({ viewport: { width: 1280, height: 1024 } }); for (const route of SCREENSHOT_ROUTES) { - const url = routeUrl(route); + routeUrl(route); console.log('Taking screenshot for route', route); try { - // routeUrl only returns fixed localhost paths from SCREENSHOT_ROUTES. - await page.goto(url, { waitUntil: 'load', timeout: 30000 }); // nosemgrep: javascript.playwright.security.audit.playwright-goto-injection.playwright-goto-injection + await navigateToRoute(page, route); await page.waitForTimeout(2000); const name = route === '/' ? 'home' : route.slice(1); await page.screenshot({ path: `test-results/${name}-screenshot.png`, fullPage: true });