From 638878e3a424ba2e0a369bf98bcaeed740773652 Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Mon, 3 Aug 2026 15:38:13 +0530 Subject: [PATCH 1/6] fix(review): resolve regressions from #147-#240 batch, harden auth + prompt-injection guards (round 9) The previous batch commit (1a7c9de) introduced several unnoticed regressions alongside its 82 issue fixes, found via ruff/pytest plus a targeted 6-agent review of that same diff: - router/graph.py: guard disabled the documented mid-confidence fallback for the default threshold (0.7 == HIGH_CONFIDENCE) - router/schema.py: extra="ignore" weakened the OWASP-LLM01 strict-JSON guard back to extra="forbid" - router/prompts.py: query sanitization targeted <<< / >>> instead of the actual << / >> fence tokens, making it a no-op against fence-forgery - backend/app/health.py: /ready leaked the dependency breakdown to any request carrying an unregistered X-API-Key (presence check, not validation) - backend/app/api/schemas.py: response-side Document.domain over-tightened to the write-side pattern, 500ing on any existing non-conforming value - db/qdrant_bootstrap.py: vector-size verification ran even on freshly created collections instead of only pre-existing ones - db/models.py: uq_documents_tenant_hash index predicate drifted from migration 004's tombstoned_at fix (issue #206) - ingestion/pipeline.py + backend/app/adapters/ingestor.py: non-GLOBAL residency_region silently dropped to the DB default on the fallback-only ingest path (DPDP FR-028) - backend/app/productization/roi_calculator.py: docstring stale vs. the actual positive-only validation contract Also: normalized 15 Python files flagged by the project's ASCII-only standard (em-dash/section-sign/arrow -> ASCII, stripped a stray UTF-8 BOM), and cleared all pre-existing ruff findings (silent except:pass, unsorted imports, import-not-at-top, line length). All 331 backend tests + 64 frontend tests pass; ruff and eslint clean. Co-Authored-By: Claude Sonnet 5 --- backend/app/adapters/__init__.py | 5 ++- backend/app/adapters/generation.py | 11 ++++++ backend/app/adapters/ingestor.py | 3 +- backend/app/api/answer.py | 2 +- backend/app/api/dependencies.py | 5 ++- backend/app/api/interfaces.py | 2 +- backend/app/api/observability.py | 2 +- backend/app/api/routing.py | 2 +- backend/app/api/schemas.py | 4 +-- backend/app/errors.py | 4 +-- backend/app/health.py | 19 +++++++---- backend/app/main.py | 6 ++-- backend/app/productization/__init__.py | 4 +-- backend/app/productization/metrics.py | 2 +- backend/app/productization/roi_calculator.py | 9 ++--- backend/app/security/auth.py | 4 +-- backend/app/settings.py | 2 +- db/models.py | 8 +++-- db/qdrant_bootstrap.py | 3 +- db/tests/test_qdrant_bootstrap.py | 30 +++++++++++++++++ db/tests/test_schema.py | 9 +++-- ingestion/chunker.py | 2 +- ingestion/parser.py | 2 +- ingestion/pipeline.py | 24 ++++++-------- ingestion/tests/conftest.py | 4 +++ ingestion/toc_extractor.py | 2 +- router/graph.py | 3 -- router/prompts.py | 2 +- router/schema.py | 2 +- tests/test_health.py | 35 ++++++++++++++++++-- 30 files changed, 149 insertions(+), 63 deletions(-) diff --git a/backend/app/adapters/__init__.py b/backend/app/adapters/__init__.py index 29f54cc..2a5d7c0 100644 --- a/backend/app/adapters/__init__.py +++ b/backend/app/adapters/__init__.py @@ -44,7 +44,10 @@ def __getattr__(name: str) -> object: AttributeError: When ``name`` is not a known adapter. """ _lazy_map = { - "SqlAlchemyDocumentStore": ("backend.app.adapters.document_store", "SqlAlchemyDocumentStore"), + "SqlAlchemyDocumentStore": ( + "backend.app.adapters.document_store", + "SqlAlchemyDocumentStore", + ), "ClaudeGenerationLLM": ("backend.app.adapters.generation", "ClaudeGenerationLLM"), "PipelineIngestor": ("backend.app.adapters.ingestor", "PipelineIngestor"), "RouterModuleAdapter": ("backend.app.adapters.router", "RouterModuleAdapter"), diff --git a/backend/app/adapters/generation.py b/backend/app/adapters/generation.py index 64f3cc2..effb245 100644 --- a/backend/app/adapters/generation.py +++ b/backend/app/adapters/generation.py @@ -18,12 +18,15 @@ from __future__ import annotations import asyncio +import logging import os from collections.abc import AsyncIterator from backend.app.api.interfaces import DependencyUnavailable, RoutedSection from backend.app.settings import get_settings +_logger = logging.getLogger(__name__) + DEFAULT_GENERATION_MODEL = os.environ.get("GENERATION_MODEL", "claude-opus-4-8") """Answer-synthesis model id (overridable via the GENERATION_MODEL env var).""" @@ -49,6 +52,10 @@ def _default_max_tokens() -> int: try: return get_settings().generation_max_tokens except Exception: + _logger.warning( + "Failed to read generation_max_tokens from settings; using fallback", + exc_info=True, + ) return 16000 @@ -61,6 +68,10 @@ def _default_thinking_budget() -> int: try: return get_settings().generation_thinking_budget_tokens except Exception: + _logger.warning( + "Failed to read generation_thinking_budget_tokens from settings; using fallback", + exc_info=True, + ) return 5000 diff --git a/backend/app/adapters/ingestor.py b/backend/app/adapters/ingestor.py index 1d2f28f..3e2e192 100644 --- a/backend/app/adapters/ingestor.py +++ b/backend/app/adapters/ingestor.py @@ -1,4 +1,4 @@ -"""Ingestor adapter binding ``ingestion.ingest_document`` to the backend Protocol. +"""Ingestor adapter binding ``ingestion.ingest_document`` to the backend Protocol. The backend :class:`Ingestor` Protocol is ``ingest_document(tenant_id, content, filename, title, domain, no_retention, residency_region, ocr) -> IngestOutcome``. @@ -247,6 +247,7 @@ async def ingest_document( toc = list(result.get("toc") or []) fallback_only = bool(result.get("fallback_only", False)) extracted_title = result.get("title") or title + return IngestOutcome( doc_id=doc_id_str, title=extracted_title, diff --git a/backend/app/api/answer.py b/backend/app/api/answer.py index bdfb760..2da4664 100644 --- a/backend/app/api/answer.py +++ b/backend/app/api/answer.py @@ -218,7 +218,7 @@ async def answer_query( raise document_not_found() if document.fallback_only: - # TODO: product owner to confirm — Option B (whole-document RAG) may replace this + # TODO: product owner to confirm - Option B (whole-document RAG) may replace this raise validation_error( detail=( "This document does not support section-level routing." diff --git a/backend/app/api/dependencies.py b/backend/app/api/dependencies.py index bb0a55a..2256694 100644 --- a/backend/app/api/dependencies.py +++ b/backend/app/api/dependencies.py @@ -22,6 +22,7 @@ from __future__ import annotations +import logging import threading from backend.app.api.interfaces import ( @@ -33,6 +34,8 @@ from backend.app.errors import service_unavailable from backend.app.settings import get_settings +logger = logging.getLogger(__name__) + _singleton_lock = threading.RLock() """Single re-entrant process-wide lock for all singleton builders (issue #222). @@ -207,4 +210,4 @@ async def _dispose_engines() -> None: if engine is not None: await engine.dispose() except Exception: - pass + logger.warning("Error disposing document store engine on shutdown", exc_info=True) diff --git a/backend/app/api/interfaces.py b/backend/app/api/interfaces.py index 5ec22da..3a8a7f2 100644 --- a/backend/app/api/interfaces.py +++ b/backend/app/api/interfaces.py @@ -18,7 +18,7 @@ from __future__ import annotations -from collections.abc import AsyncGenerator, AsyncIterator +from collections.abc import AsyncIterator from dataclasses import dataclass, field from typing import Protocol, runtime_checkable diff --git a/backend/app/api/observability.py b/backend/app/api/observability.py index aa785f2..21f5516 100644 --- a/backend/app/api/observability.py +++ b/backend/app/api/observability.py @@ -1,4 +1,4 @@ -"""Observability surface: the ``/metrics`` scrape endpoint (PRD §21, NFR-009). +"""Observability surface: the ``/metrics`` scrape endpoint (PRD Sec. 21, NFR-009). Exposes the four product KPIs (token reduction %, answer accuracy, routing latency, fallback rate) in Prometheus text exposition format. LangSmith diff --git a/backend/app/api/routing.py b/backend/app/api/routing.py index b2a41c1..8036276 100644 --- a/backend/app/api/routing.py +++ b/backend/app/api/routing.py @@ -87,7 +87,7 @@ async def route_query( """ document_ids = list(dict.fromkeys(_target_document_ids(body))) - # Fix #262: rerank=True is not supported — reject with 422 instead of + # Fix #262: rerank=True is not supported - reject with 422 instead of # silently downgrading and returning HTTP 200. if body.rerank: raise validation_error( diff --git a/backend/app/api/schemas.py b/backend/app/api/schemas.py index eed6337..2f503ab 100644 --- a/backend/app/api/schemas.py +++ b/backend/app/api/schemas.py @@ -180,7 +180,7 @@ class Citation(_Strict): page_end: Annotated[int, Field(ge=1)] @model_validator(mode="after") - def _validate_page_range(self) -> "Citation": + def _validate_page_range(self) -> Citation: """Validate page_start <= page_end. Returns: @@ -267,7 +267,7 @@ class Document(_Strict): doc_id: DocumentId title: str | None = None total_pages: Annotated[int, Field(ge=0)] - domain: Annotated[str, Field(max_length=100, pattern=r"^[a-zA-Z0-9._-]+$")] | None = None + domain: str | None = None residency_region: ResidencyRegion fallback_only: bool created_at: str | None = None diff --git a/backend/app/errors.py b/backend/app/errors.py index 778c538..a33af04 100644 --- a/backend/app/errors.py +++ b/backend/app/errors.py @@ -105,11 +105,11 @@ def unauthorized( """Build a 401 UNAUTHORIZED problem. The ``WWW-Authenticate: Bearer`` header is included on every 401 response - per RFC 6750 §3 so that HTTP clients and API gateways can identify the + per RFC 6750 Sec. 3 so that HTTP clients and API gateways can identify the accepted credential scheme. This is intentional even when only API-key auth is configured: the header is informational and harmless, and omitting it would require threading settings through every error-raise site (issue - #183 — accepted as RFC-compliant behaviour). + #183 - accepted as RFC-compliant behaviour). Args: detail: Occurrence-specific explanation. diff --git a/backend/app/health.py b/backend/app/health.py index 27d270f..6ceea63 100644 --- a/backend/app/health.py +++ b/backend/app/health.py @@ -15,6 +15,8 @@ from fastapi import APIRouter, Request, Response, status from pydantic import BaseModel +from backend.app.errors import ProblemException +from backend.app.security.auth import resolve_principal from backend.app.settings import Settings, get_settings router = APIRouter(tags=["Operations"]) @@ -61,9 +63,10 @@ async def _check_postgres(database_url: str, timeout_seconds: float) -> bool: Returns: True if the authenticated query succeeded, otherwise False. """ - import asyncpg import re + import asyncpg + try: # asyncpg rejects SQLAlchemy dialect prefixes like postgresql+asyncpg:// clean_url = re.sub(r"^postgresql\+\w+://", "postgresql://", database_url) @@ -165,10 +168,13 @@ async def get_readiness(request: Request, response: Response) -> ReadinessStatus avoid disclosing infrastructure details (postgres/qdrant host reachability) to unauthenticated probers. K8s liveness and readiness probes do not send auth headers, so they still receive the correct status code; only the - ``dependencies`` breakdown is hidden from them. + ``dependencies`` breakdown is hidden from them. The credential is fully + verified via :func:`resolve_principal` (signature/expiry/key-hash checks) - + a caller cannot see the breakdown by merely sending an arbitrary header + value, since that would defeat the purpose of hiding it. Args: - request: The incoming request (used to detect auth headers). + request: The incoming request (used to verify the caller's credential). response: The response object whose status code is set to 503 on a degraded result. @@ -181,9 +187,8 @@ async def get_readiness(request: Request, response: Response) -> ReadinessStatus readiness = await evaluate_readiness(settings) if readiness.status != "ready": response.status_code = status.HTTP_503_SERVICE_UNAVAILABLE - has_auth = bool( - request.headers.get("X-API-Key") or request.headers.get("Authorization") - ) - if not has_auth: + try: + resolve_principal(request, settings) + except ProblemException: return ReadinessStatus(status=readiness.status, dependencies={}) return readiness diff --git a/backend/app/main.py b/backend/app/main.py index 0bc036c..b71fb55 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -2,7 +2,7 @@ Builds the real ASGI application: the liveness/readiness probe router (NFR-009, ADR-5), the /v1 routing, answer, and document/compliance routers -(openapi.yaml), the ``/metrics`` observability surface (PRD §21), and the +(openapi.yaml), the ``/metrics`` observability surface (PRD Sec. 21), and the RFC-7807 exception handlers. LangSmith tracing is configured from the environment at construction time. The factory is side-effect free with respect to external dependencies so it can be imported by the test suite and run @@ -16,8 +16,8 @@ from __future__ import annotations import logging +from collections.abc import AsyncIterator from contextlib import asynccontextmanager -from typing import AsyncIterator from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddleware @@ -38,7 +38,7 @@ async def _lifespan(_app: FastAPI) -> AsyncIterator[None]: """Manage application startup and shutdown lifecycle. - On startup no eager initialisation is performed — heavyweight singletons + On startup no eager initialisation is performed - heavyweight singletons (SQLAlchemy engine, Qdrant client, etc.) are created lazily on first request so the process starts quickly and without requiring credentials in the test environment. diff --git a/backend/app/productization/__init__.py b/backend/app/productization/__init__.py index 06023df..33c2b0d 100644 --- a/backend/app/productization/__init__.py +++ b/backend/app/productization/__init__.py @@ -5,10 +5,10 @@ behavior: - ``roi_calculator``: projects monthly LLM-spend savings from the documented - 40-70% token-reduction range (PRD §15.1, §21) for sales/GTM modeling. + 40-70% token-reduction range (PRD Sec. 15.1, Sec. 21) for sales/GTM modeling. - ``metrics``: an in-process registry for the four product KPIs (token reduction %, answer accuracy, routing latency, fallback rate) exposed at - ``/metrics`` for scrape-based observability (PRD §21, NFR-009). + ``/metrics`` for scrape-based observability (PRD Sec. 21, NFR-009). """ from __future__ import annotations diff --git a/backend/app/productization/metrics.py b/backend/app/productization/metrics.py index 630107b..febe2bc 100644 --- a/backend/app/productization/metrics.py +++ b/backend/app/productization/metrics.py @@ -1,4 +1,4 @@ -"""In-process product-KPI metrics registry (PRD §21, NFR-009). +"""In-process product-KPI metrics registry (PRD Sec. 21, NFR-009). Tracks the four product KPIs the PRD measures the system against: diff --git a/backend/app/productization/roi_calculator.py b/backend/app/productization/roi_calculator.py index b044462..bc82137 100644 --- a/backend/app/productization/roi_calculator.py +++ b/backend/app/productization/roi_calculator.py @@ -4,7 +4,7 @@ documented token-reduction range when they place this refinement layer in front of their existing RAG stack. The savings band is anchored to the PRD-verified range of 40-70% average token reduction vs. full-document RAG -(PRD §15.1 / §21), with a documented conservative/expected/optimistic split. +(PRD Sec. 15.1 / Sec. 21), with a documented conservative/expected/optimistic split. This module is a pure-function GTM/sales utility. It performs no I/O, calls no provider, and does not touch routing, ingestion, or persistence. The dollar @@ -124,7 +124,8 @@ def calculate_roi( Args: current_monthly_spend_usd: Current monthly LLM spend in USD. Must be a - finite, non-negative number. + finite, positive number; a zero or negative spend has no + addressable savings to project. context_spend_fraction: Fraction (0.0-1.0] of the spend attributable to retrieved context that this layer can reduce. Defaults to 0.7, a common context-heavy RAG profile. @@ -134,8 +135,8 @@ def calculate_roi( projections plus the expected annual saving. Raises: - ValueError: If the spend is negative/non-finite or the context - fraction is outside the (0.0, 1.0] range. + ValueError: If the spend is not finite or not positive, or the + context fraction is outside the (0.0, 1.0] range. """ if not math.isfinite(current_monthly_spend_usd) or current_monthly_spend_usd <= 0: raise ValueError( diff --git a/backend/app/security/auth.py b/backend/app/security/auth.py index 31e0dd5..6f87757 100644 --- a/backend/app/security/auth.py +++ b/backend/app/security/auth.py @@ -23,14 +23,14 @@ import threading from dataclasses import dataclass -_RATE_LIMIT_KEY_HASH_LENGTH = 16 - import jwt from fastapi import Request from backend.app.errors import unauthorized from backend.app.settings import Settings, get_settings +_RATE_LIMIT_KEY_HASH_LENGTH = 16 + class PrincipalKind(enum.StrEnum): """The credential class a principal authenticated with.""" diff --git a/backend/app/settings.py b/backend/app/settings.py index 386c20a..e028a7c 100644 --- a/backend/app/settings.py +++ b/backend/app/settings.py @@ -64,7 +64,7 @@ class Settings(BaseSettings): default="HS256", alias="JWT_ALGORITHM", description=( - "JWT signing algorithm — restricted to HMAC-SHA variants" + "JWT signing algorithm - restricted to HMAC-SHA variants" " to prevent alg:none bypass." ), ) diff --git a/db/models.py b/db/models.py index b915105..ea920cc 100644 --- a/db/models.py +++ b/db/models.py @@ -118,14 +118,16 @@ class Document(Base): __table_args__ = ( CheckConstraint("total_pages >= 0", name="documents_total_pages_nonneg"), - # Canonical DDL: migrations/002_indexes_constraints.sql (#104). - # Keep the partial predicate (content_hash IS NOT NULL) in sync with that migration. + # Canonical DDL: migrations/004_fix_tenant_hash_partial_index.sql (#206). + # Keep the partial predicate in sync with that migration: it excludes + # rows with no hash AND tombstoned rows, so a soft-deleted document no + # longer blocks re-upload of the same content under the same tenant. Index( "uq_documents_tenant_hash", "tenant_id", "content_hash", unique=True, - postgresql_where=content_hash.isnot(None), + postgresql_where=content_hash.isnot(None) & tombstoned_at.is_(None), ), Index("idx_documents_tenant_id", "tenant_id"), Index("idx_documents_domain", "domain"), diff --git a/db/qdrant_bootstrap.py b/db/qdrant_bootstrap.py index e5f3e92..31c926b 100644 --- a/db/qdrant_bootstrap.py +++ b/db/qdrant_bootstrap.py @@ -179,7 +179,8 @@ def bootstrap_collection( collection_name=collection_name, vectors_config=vectors_config(), ) - _verify_vector_size(client, collection_name) + else: + _verify_vector_size(client, collection_name) indexed = ensure_payload_indexes(client, collection_name=collection_name) return BootstrapResult(created=not exists, indexed_fields=indexed) diff --git a/db/tests/test_qdrant_bootstrap.py b/db/tests/test_qdrant_bootstrap.py index 74a081a..acd264c 100644 --- a/db/tests/test_qdrant_bootstrap.py +++ b/db/tests/test_qdrant_bootstrap.py @@ -8,6 +8,8 @@ from __future__ import annotations +from types import SimpleNamespace + import pytest from qdrant_client import models as qm @@ -22,6 +24,30 @@ ) +def _fake_collection_info(size: int) -> SimpleNamespace: + """Build a minimal stand-in exposing only the attribute path bootstrap reads. + + Real ``qm.CollectionInfo`` construction requires many unrelated required + fields that drift across qdrant-client versions; the production code only + ever reads ``.config.params.vectors.size``. + """ + return SimpleNamespace( + config=SimpleNamespace( + params=SimpleNamespace( + vectors=SimpleNamespace(size=size, distance=qm.Distance.COSINE) + ) + ) + ) + + +def test_existing_collection_with_mismatched_vector_size_raises() -> None: + """Bootstrap raises when an existing collection's vector size differs.""" + client = FakeQdrantClient(existing=True) + client.get_collection = lambda collection_name: _fake_collection_info(768) # type: ignore[method-assign] + with pytest.raises(RuntimeError, match="vector size"): + bootstrap_collection(client) # type: ignore[arg-type] + + class FakeQdrantClient: """Minimal in-memory stand-in for QdrantClient used in bootstrap tests. @@ -62,6 +88,10 @@ def create_payload_index( """Record a payload-index call for ``field_name``.""" self.payload_indexes.append(field_name) + def get_collection(self, collection_name: str) -> SimpleNamespace: + """Return collection info reporting a vector size matching VECTOR_SIZE.""" + return _fake_collection_info(VECTOR_SIZE) + def test_vector_size_matches_embedding_model() -> None: """Vector size is 1536 and distance is Cosine (text-embedding-3-small).""" diff --git a/db/tests/test_schema.py b/db/tests/test_schema.py index a9d55bc..09076fc 100644 --- a/db/tests/test_schema.py +++ b/db/tests/test_schema.py @@ -38,7 +38,7 @@ _FORWARD_SQL_004 = _MIGRATIONS_DIR / "004_fix_tenant_hash_partial_index.sql" _DOWN_SQL_004 = _MIGRATIONS_DIR / "004_fix_tenant_hash_partial_index.down.sql" -# Ordered list of all forward migrations — used by the live integration test +# Ordered list of all forward migrations - used by the live integration test # to apply them in dependency order. _ALL_FORWARD_MIGRATIONS: list[pathlib.Path] = [ _FORWARD_SQL, @@ -268,7 +268,10 @@ def test_migration_002_declares_tenant_hash_unique_index() -> None: r"uq_documents_tenant_hash[\s\S]*content_hash\s+IS\s+NOT\s+NULL", ddl, re.IGNORECASE, - ), "uq_documents_tenant_hash in migration 002 must be a partial index on content_hash IS NOT NULL" + ), ( + "uq_documents_tenant_hash in migration 002 must be a partial index " + "on content_hash IS NOT NULL" + ) def test_migration_003_declares_erasure_outbox_fk() -> None: @@ -337,7 +340,7 @@ def test_forward_then_rollback_restores_baseline() -> None: from sqlalchemy import create_engine, inspect, text engine = create_engine(os.environ["DATABASE_URL"]) - # Apply all migrations in order (001 → 002 → 003 → 004). + # Apply all migrations in order (001 -> 002 -> 003 -> 004). for migration_path in _ALL_FORWARD_MIGRATIONS: migration_sql = _read(migration_path) with engine.begin() as conn: diff --git a/ingestion/chunker.py b/ingestion/chunker.py index 420bf85..4960ee4 100644 --- a/ingestion/chunker.py +++ b/ingestion/chunker.py @@ -99,7 +99,7 @@ def _section_page_text(doc: ParsedDocument, entry: TocEntry) -> list[tuple[int, continue if not page.text.strip(): logger.warning( - "Page %d in section '%s' has no extractable text (image-only?) — skipping", + "Page %d in section '%s' has no extractable text (image-only?) - skipping", page.number, getattr(entry, "title", "unknown"), ) diff --git a/ingestion/parser.py b/ingestion/parser.py index b8b8532..24211b4 100644 --- a/ingestion/parser.py +++ b/ingestion/parser.py @@ -19,7 +19,7 @@ logger = logging.getLogger(__name__) -MAX_PDF_BYTES: int = 100 * 1024 * 1024 # 100 MB — fix #246 +MAX_PDF_BYTES: int = 100 * 1024 * 1024 # 100 MB - fix #246 class ParseError(ValueError): diff --git a/ingestion/pipeline.py b/ingestion/pipeline.py index 801340d..edf24f3 100644 --- a/ingestion/pipeline.py +++ b/ingestion/pipeline.py @@ -1,4 +1,4 @@ -"""Ingestion pipeline orchestration (STORY-003/008/009/011). +"""Ingestion pipeline orchestration (STORY-003/008/009/011). Implements the data-engineer-owned ``ingest_document`` entry point that backend calls: parse -> TOC -> section-aware chunk -> embed -> upsert, writing section @@ -127,6 +127,7 @@ def upsert_document( content_hash_value: str | None, ingest_status: str, fallback_only: bool, + residency_region: str = "GLOBAL", ) -> None: """Create or update the document row (tenant-scoped). @@ -139,6 +140,7 @@ def upsert_document( content_hash_value: Content hash (None in no-retention mode). ingest_status: One of ``db.models.INGEST_STATUS_VALUES``. fallback_only: True when no structure was detected (Scenario C). + residency_region: DPDP data-residency region (FR-028); defaults to GLOBAL. """ ... @@ -188,11 +190,12 @@ def upsert_document_and_replace_sections( ... def update_residency_region(self, doc_id: str, tenant_id: str, residency_region: str) -> None: - """Update the residency_region for a document after ingestion (DPDP FR-028). + """Manually correct the residency_region for an already-ingested document (DPDP FR-028). - Called after the pipeline completes to stamp the data-residency region that - the synchronous pipeline cannot persist because ``IngestInput`` carries no - residency field. + ``ingest_document`` now persists ``residency_region`` on every write path via + ``upsert_document`` and ``upsert_document_and_replace_sections``, so this method + is no longer part of the ingest write path; it remains as an administrative + correction hook for out-of-band residency reassignment. Args: doc_id: Document primary key. @@ -544,17 +547,9 @@ def ingest_document( content_hash_value=hash_value, ingest_status="indexed", fallback_only=False, + residency_region=doc.residency_region, ) else: - # Fix #180: for no_retention documents, residency_region is not passed - # to upsert_document because the Protocol does not carry it yet. - # The ingestor adapter (PipelineIngestor) calls update_residency_region - # after the pipeline completes to stamp the region on the document row, - # but only when no_retention=False (DPDP FR-028). For true no_retention - # documents, content is not persisted so residency enforcement is - # inherently limited; the TODO below tracks the gap. - # TODO: extend upsert_document Protocol to accept residency_region so - # ephemeral documents still record their intended region for audit. section_store.upsert_document( doc_id=doc_id, tenant_id=doc.tenant_id, @@ -564,6 +559,7 @@ def ingest_document( content_hash_value=None if doc.no_retention else hash_value, ingest_status=ingest_status, fallback_only=fallback_only_flag, + residency_region=doc.residency_region, ) return IngestResult( diff --git a/ingestion/tests/conftest.py b/ingestion/tests/conftest.py index 8535577..cebcf93 100644 --- a/ingestion/tests/conftest.py +++ b/ingestion/tests/conftest.py @@ -138,6 +138,7 @@ def upsert_document( content_hash_value: str | None, ingest_status: str, fallback_only: bool, + residency_region: str = "GLOBAL", ) -> None: """Create or update a document row and index its content hash. @@ -150,6 +151,7 @@ def upsert_document( content_hash_value: Content hash (None in no-retention). ingest_status: Ingest status enum value. fallback_only: Scenario C flag. + residency_region: DPDP data-residency region; defaults to GLOBAL. """ self.documents[doc_id] = { "doc_id": doc_id, @@ -160,6 +162,7 @@ def upsert_document( "content_hash": content_hash_value, "ingest_status": ingest_status, "fallback_only": fallback_only, + "residency_region": residency_region, } if content_hash_value is not None: self.hash_index[(tenant_id, content_hash_value)] = doc_id @@ -222,6 +225,7 @@ def upsert_document_and_replace_sections( content_hash_value=content_hash_value, ingest_status=ingest_status, fallback_only=fallback_only, + residency_region=residency_region, ) return self.replace_sections(tenant_id, doc_id, rows) diff --git a/ingestion/toc_extractor.py b/ingestion/toc_extractor.py index bdb96c8..7f43db0 100644 --- a/ingestion/toc_extractor.py +++ b/ingestion/toc_extractor.py @@ -309,7 +309,7 @@ def extract_toc( refined = llm_refiner.refine(heuristic) if not refined and heuristic: logger.warning( - "LLM refiner returned empty result for %d heuristic entries — " + "LLM refiner returned empty result for %d heuristic entries - " "falling back to Scenario C. doc_id=%s", len(heuristic), getattr(doc, "doc_id", "unknown"), diff --git a/router/graph.py b/router/graph.py index 9a62f4e..e653390 100644 --- a/router/graph.py +++ b/router/graph.py @@ -245,9 +245,6 @@ def _apply_threshold( high = [item for item in eligible if item.confidence >= confidence_threshold] if high: return high[:max_sections] - # Only fall back to mid-band for sub-HIGH_CONFIDENCE thresholds - if confidence_threshold >= HIGH_CONFIDENCE: - return [] # No section met the strict threshold -> fallback mid = [item for item in eligible if item.confidence < confidence_threshold] return mid[:max_sections] diff --git a/router/prompts.py b/router/prompts.py index 9a939e0..3eec665 100644 --- a/router/prompts.py +++ b/router/prompts.py @@ -146,7 +146,7 @@ def build_router_messages( A tuple ``(system_prompt, messages)`` where ``messages`` is the few-shot-primed user turn list for the Messages API. """ - safe_query = query.replace("<<<", "< < <").replace(">>>", "> > >") + safe_query = query.replace("<<", "< <").replace(">>", "> >") allowed_repr = "[" + ", ".join(f'"{sid}"' for sid in allowed_section_ids) + "]" user_turn = ( f"ALLOWED_SECTION_IDS: {allowed_repr}\n\n" diff --git a/router/schema.py b/router/schema.py index 920fb4f..a6e387f 100644 --- a/router/schema.py +++ b/router/schema.py @@ -41,7 +41,7 @@ class RankedSection(BaseModel): confidence: Router confidence in [0.0, 1.0] that the section is relevant. """ - model_config = ConfigDict(extra="ignore") + model_config = ConfigDict(extra="forbid") section_id: str confidence: float = Field(ge=0.0, le=1.0) diff --git a/tests/test_health.py b/tests/test_health.py index d05e628..e196441 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -12,7 +12,9 @@ from backend.app import health as health_module from backend.app.main import create_app +from backend.app.security.auth import get_api_key_store from backend.app.settings import Settings +from tests.conftest import TENANT_A @pytest.fixture @@ -38,9 +40,11 @@ def test_ready_returns_503_when_dependency_down( ) -> None: """Readiness must return 503 when a configured dependency is down. - Sends an API-key header so the dependency breakdown is included in the - response (authenticated callers receive the full ``dependencies`` map per - the auth-gating logic in ``get_readiness``). + Sends a registered, valid API-key header so the dependency breakdown is + included in the response (authenticated callers receive the full + ``dependencies`` map per the auth-gating logic in ``get_readiness``, which + verifies the credential via :func:`resolve_principal` rather than merely + checking for header presence). """ async def fake_evaluate(settings: Settings) -> health_module.ReadinessStatus: @@ -50,6 +54,7 @@ async def fake_evaluate(settings: Settings) -> health_module.ReadinessStatus: ) monkeypatch.setattr(health_module, "evaluate_readiness", fake_evaluate) + get_api_key_store().register("probe-key", TENANT_A, "key_probe") response = client.get("/ready", headers={"X-API-Key": "probe-key"}) assert response.status_code == 503 body = response.json() @@ -57,6 +62,30 @@ async def fake_evaluate(settings: Settings) -> health_module.ReadinessStatus: assert body["dependencies"]["postgres"] == "down" +def test_ready_hides_dependencies_for_unregistered_api_key( + client: TestClient, monkeypatch: pytest.MonkeyPatch +) -> None: + """An unregistered/bogus API key must not unlock the dependency breakdown. + + The credential is fully verified (hash lookup), not merely checked for + presence, so an arbitrary header value is treated the same as no header + at all. + """ + + async def fake_evaluate(settings: Settings) -> health_module.ReadinessStatus: + return health_module.ReadinessStatus( + status="degraded", + dependencies={"postgres": "down", "qdrant": "up"}, + ) + + monkeypatch.setattr(health_module, "evaluate_readiness", fake_evaluate) + response = client.get("/ready", headers={"X-API-Key": "never-registered"}) + assert response.status_code == 503 + body = response.json() + assert body["status"] == "degraded" + assert body["dependencies"] == {} + + def test_ready_returns_200_when_deps_up( client: TestClient, monkeypatch: pytest.MonkeyPatch ) -> None: From 5e2c9c74c29d373d4a52ccd36a5f2182d641c402 Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Mon, 3 Aug 2026 16:21:07 +0530 Subject: [PATCH 2/6] fix(review): resolve mypy errors, pip-audit CVE, and MissingGreenlet test bug Discovered by actually inspecting CI results for #298 rather than trusting local ruff+pytest alone -- mypy, bandit, and pip-audit were never run locally in this session, and CI turned out to already be red on main from the batch commit onward (over a month), for reasons unrelated to the round-9 fixes. - backend/app/api/interfaces.py: GenerationLLM.stream_answer's Protocol stub body was `...` with no `yield`, so mypy inferred calling it returns Coroutine[Any, Any, AsyncIterator[str]] instead of AsyncIterator[str] directly -- breaking `async for` at every call site through the Protocol type (backend/app/api/answer.py) and forcing two now-dead `type: ignore` workarounds in dependencies.py's singleton. Fixed the stub to be a real (unreachable) async generator so mypy treats it correctly; removed the dead ignores. - db/qdrant_bootstrap.py: `_verify_vector_size` accessed `.size` on a `VectorParams | dict[str, VectorParams] | None` union without narrowing. Added an isinstance guard that raises a clear RuntimeError for named-vector collections instead of leaving an unchecked AttributeError risk. - router/tests/test_router.py: removed a stale `type: ignore[arg-type]` that mypy now reports as unused. - pyproject.toml: pinned setuptools>=83.0.0 (build-system and dev extras) -- pip-audit flagged the installed 79.0.1 for PYSEC-2026-3447. This vulnerability was previously masked because bandit failed first in the same CI job (on the S110 issue this round's earlier commit already fixed), and pip-audit never got to run. - db/tests/test_schema.py: `test_forward_then_rollback_restores_baseline` used sync `create_engine` against `DATABASE_URL`, which CI sets to a `postgresql+asyncpg://` DSN (asyncpg is the project's only Postgres driver) -- raising `sqlalchemy.exc.MissingGreenlet`. Converted to `create_async_engine` + `conn.run_sync(inspect(...))`; added the matching `anyio_backend` fixture to db/tests/conftest.py. This test is skipped locally (no DATABASE_URL) so it was never caught outside CI. ruff, mypy, and pytest (330 passed, 1 skipped -- the live-DB test, correctly skipped without DATABASE_URL) are all clean locally after this commit. Co-Authored-By: Claude Sonnet 5 --- backend/app/api/dependencies.py | 4 +- backend/app/api/interfaces.py | 3 +- db/qdrant_bootstrap.py | 9 ++++- db/tests/conftest.py | 8 ++++ db/tests/test_qdrant_bootstrap.py | 8 ++-- db/tests/test_schema.py | 61 +++++++++++++++++++++---------- pyproject.toml | 6 ++- router/tests/test_router.py | 2 +- 8 files changed, 73 insertions(+), 28 deletions(-) diff --git a/backend/app/api/dependencies.py b/backend/app/api/dependencies.py index 2256694..8f59af4 100644 --- a/backend/app/api/dependencies.py +++ b/backend/app/api/dependencies.py @@ -149,10 +149,10 @@ def _generation_llm_singleton() -> GenerationLLM: from backend.app.settings import get_settings settings = get_settings() - _generation_llm_cache = ClaudeGenerationLLM( # type: ignore[assignment] + _generation_llm_cache = ClaudeGenerationLLM( thinking_budget_tokens=settings.generation_thinking_budget_tokens ) - return _generation_llm_cache # type: ignore[return-value] + return _generation_llm_cache def get_document_store() -> DocumentStore: diff --git a/backend/app/api/interfaces.py b/backend/app/api/interfaces.py index 3a8a7f2..a6945c8 100644 --- a/backend/app/api/interfaces.py +++ b/backend/app/api/interfaces.py @@ -214,7 +214,8 @@ async def stream_answer( sections: list[RoutedSection], ) -> AsyncIterator[str]: """Yield answer token fragments for the routed sections.""" - ... + raise NotImplementedError + yield # pragma: no cover - unreachable, marks this an async generator class DependencyUnavailable(Exception): # noqa: N818 - maps to SERVICE_UNAVAILABLE, not an *Error diff --git a/db/qdrant_bootstrap.py b/db/qdrant_bootstrap.py index 31c926b..651df80 100644 --- a/db/qdrant_bootstrap.py +++ b/db/qdrant_bootstrap.py @@ -143,7 +143,14 @@ def _verify_vector_size(client: QdrantClient, collection_name: str) -> None: intervention (delete the collection and restart). """ collection_info = client.get_collection(collection_name) - actual_size = collection_info.config.params.vectors.size + vectors = collection_info.config.params.vectors + if not isinstance(vectors, qm.VectorParams): + raise RuntimeError( + f"Qdrant collection '{collection_name}' does not use a single unnamed " + "vector config (found named-vectors or no vectors); this collection was " + "not created by bootstrap_collection. Delete it and restart." + ) + actual_size = vectors.size if actual_size != VECTOR_SIZE: raise RuntimeError( f"Qdrant collection '{collection_name}' has vector size {actual_size} " diff --git a/db/tests/conftest.py b/db/tests/conftest.py index d63ff1e..28dd9f7 100644 --- a/db/tests/conftest.py +++ b/db/tests/conftest.py @@ -5,6 +5,14 @@ import pathlib import sys +import pytest + _PROJECT_ROOT = pathlib.Path(__file__).resolve().parents[2] if str(_PROJECT_ROOT) not in sys.path: sys.path.insert(0, str(_PROJECT_ROOT)) + + +@pytest.fixture +def anyio_backend() -> str: + """Run anyio-based async tests on asyncio only (no trio dependency).""" + return "asyncio" diff --git a/db/tests/test_qdrant_bootstrap.py b/db/tests/test_qdrant_bootstrap.py index acd264c..c627ab6 100644 --- a/db/tests/test_qdrant_bootstrap.py +++ b/db/tests/test_qdrant_bootstrap.py @@ -28,13 +28,15 @@ def _fake_collection_info(size: int) -> SimpleNamespace: """Build a minimal stand-in exposing only the attribute path bootstrap reads. Real ``qm.CollectionInfo`` construction requires many unrelated required - fields that drift across qdrant-client versions; the production code only - ever reads ``.config.params.vectors.size``. + fields that drift across qdrant-client versions, so everything except + ``vectors`` is a plain stand-in; ``vectors`` itself must be a real + ``qm.VectorParams`` since ``_verify_vector_size`` does an ``isinstance`` + check against it (to reject named-vector collections it cannot support). """ return SimpleNamespace( config=SimpleNamespace( params=SimpleNamespace( - vectors=SimpleNamespace(size=size, distance=qm.Distance.COSINE) + vectors=qm.VectorParams(size=size, distance=qm.Distance.COSINE) ) ) ) diff --git a/db/tests/test_schema.py b/db/tests/test_schema.py index 09076fc..3489726 100644 --- a/db/tests/test_schema.py +++ b/db/tests/test_schema.py @@ -330,28 +330,51 @@ def test_migration_004_erasure_outbox_fk_is_cascade_in_model() -> None: not os.environ.get("DATABASE_URL"), reason="DATABASE_URL not set; live forward+rollback migration test skipped.", ) -def test_forward_then_rollback_restores_baseline() -> None: +@pytest.mark.anyio +async def test_forward_then_rollback_restores_baseline() -> None: """Apply all forward migrations in order, then rollback 001; expect clean state. Applies migrations 001 through 004 in dependency order against a live Postgres instance, then runs the 001 rollback. All migrations are tested, not just 001. Skipped without ``DATABASE_URL`` so the suite stays offline-green. + + ``DATABASE_URL`` carries the ``postgresql+asyncpg://`` async DSN (per + ``asyncpg`` being the project's only Postgres driver dependency), so this + uses ``create_async_engine`` rather than the sync ``create_engine`` - + mixing a sync engine with an async-only driver raises + ``sqlalchemy.exc.MissingGreenlet``. """ - from sqlalchemy import create_engine, inspect, text - - engine = create_engine(os.environ["DATABASE_URL"]) - # Apply all migrations in order (001 -> 002 -> 003 -> 004). - for migration_path in _ALL_FORWARD_MIGRATIONS: - migration_sql = _read(migration_path) - with engine.begin() as conn: - conn.execute(text(migration_sql)) - inspector = inspect(engine) - assert inspector.has_table("documents") - assert inspector.has_table("sections") - # Roll back 001 (which cascades to drop all dependent objects). - down = _read(_DOWN_SQL) - with engine.begin() as conn: - conn.execute(text(down)) - inspector = inspect(engine) - assert not inspector.has_table("sections") - assert not inspector.has_table("documents") + from sqlalchemy import inspect, text + from sqlalchemy.ext.asyncio import create_async_engine + + engine = create_async_engine(os.environ["DATABASE_URL"]) + try: + # Apply all migrations in order (001 -> 002 -> 003 -> 004). + for migration_path in _ALL_FORWARD_MIGRATIONS: + migration_sql = _read(migration_path) + async with engine.begin() as conn: + await conn.execute(text(migration_sql)) + async with engine.connect() as conn: + has_documents = await conn.run_sync( + lambda sync_conn: inspect(sync_conn).has_table("documents") + ) + has_sections = await conn.run_sync( + lambda sync_conn: inspect(sync_conn).has_table("sections") + ) + assert has_documents + assert has_sections + # Roll back 001 (which cascades to drop all dependent objects). + down = _read(_DOWN_SQL) + async with engine.begin() as conn: + await conn.execute(text(down)) + async with engine.connect() as conn: + sections_remain = await conn.run_sync( + lambda sync_conn: inspect(sync_conn).has_table("sections") + ) + documents_remain = await conn.run_sync( + lambda sync_conn: inspect(sync_conn).has_table("documents") + ) + assert not sections_remain + assert not documents_remain + finally: + await engine.dispose() diff --git a/pyproject.toml b/pyproject.toml index 980047c..1b532a3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -39,6 +39,9 @@ dependencies = [ [project.optional-dependencies] dev = [ + # >=83.0.0 remediates PYSEC-2026-3447; pinned explicitly since CI's base + # image ships an older setuptools that pip does not otherwise upgrade. + "setuptools>=83.0.0", "ruff>=0.4,<1.0", "mypy>=1.10,<2.0", "pytest>=9.0.3,<10.0", @@ -50,7 +53,8 @@ dev = [ ] [build-system] -requires = ["setuptools>=68", "wheel"] +# >=83.0.0 remediates PYSEC-2026-3447. +requires = ["setuptools>=83.0.0", "wheel"] build-backend = "setuptools.build_meta" [tool.setuptools] diff --git a/router/tests/test_router.py b/router/tests/test_router.py index fdc603d..c377fa6 100644 --- a/router/tests/test_router.py +++ b/router/tests/test_router.py @@ -339,7 +339,7 @@ async def test_llm_error_during_route_triggers_fallback(golden_toc: list[dict]) "doc_abc123", golden_toc, tenant_id="tenant_test", - llm=error_llm, # type: ignore[arg-type] + llm=error_llm, confidence_threshold=0.7, max_sections=3, ) From c0b4cdb6335f468708be5ec03a840559f4e99752 Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Mon, 3 Aug 2026 16:26:38 +0530 Subject: [PATCH 3/6] fix(security): realign detect-secrets baseline line number after auth.py edit The auth.py E402 fix earlier in this round moved a module-level constant below the imports, shifting PrincipalKind.API_KEY (the acknowledged Secret-Keyword false positive) from line 36 to line 38. detect-secrets' baseline compares findings by exact line number, so the shift made CI treat the same already-acknowledged false positive as a brand-new secret and fail the security job. Updated the recorded line_number to match; left every other baseline entry untouched (no new scan performed, to avoid the Windows-vs-Linux path-separator mismatch a native rescan would introduce). Co-Authored-By: Claude Sonnet 5 --- .secrets.baseline | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.secrets.baseline b/.secrets.baseline index 357466a..97326e5 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -157,7 +157,7 @@ "filename": "backend/app/security/auth.py", "hashed_secret": "665b1e3851eefefa3fb878654292f16597d25155", "is_verified": false, - "line_number": 36 + "line_number": 38 } ], "deploy/k8s/ingress.yaml": [ From 7516eb0e3fdab2e5c6c03083c75f0dc5b6618a4d Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Mon, 3 Aug 2026 16:27:36 +0530 Subject: [PATCH 4/6] fix(ci): add missing Postgres service container to the test job DATABASE_URL was set to a postgresql+asyncpg://localhost:5432 DSN with no `services:` block provisioning that server, so test_forward_then_rollback_restores_baseline (which runs for real rather than skipping, since CI does set DATABASE_URL) always failed on connection refused -- previously masked by the sync/async engine bug fixed earlier in this pass, which failed before ever reaching the network. Added a postgres:16 service container with a pg_isready health check, matching the test/test/test credentials already baked into DATABASE_URL. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/ci.yml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97a3967..fc4f769 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,6 +45,23 @@ jobs: test: timeout-minutes: 30 runs-on: ubuntu-latest + # db/tests/test_schema.py::test_forward_then_rollback_restores_baseline runs + # a live migration round-trip against DATABASE_URL instead of skipping, so + # this job needs an actual Postgres server for that DSN to resolve. + services: + postgres: + image: postgres:16 + env: + POSTGRES_USER: test + POSTGRES_PASSWORD: test + POSTGRES_DB: test + ports: + - 5432:5432 + options: >- + --health-cmd="pg_isready -U test" + --health-interval=5s + --health-timeout=5s + --health-retries=10 env: DATABASE_URL: "postgresql+asyncpg://test:test@localhost:5432/test" QDRANT_URL: "http://localhost:6333" From c93db0ebcfaa0c9096a9b27efd4ed647a615bad6 Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Mon, 3 Aug 2026 16:31:46 +0530 Subject: [PATCH 5/6] fix(test): execute migration scripts via raw asyncpg connection test_forward_then_rollback_restores_baseline reached a live Postgres for the first time after the MissingGreenlet fix, surfacing a second real bug: SQLAlchemy's asyncpg dialect always executes through prepared statements, and Postgres's extended query protocol rejects multiple SQL commands in a single prepared statement -- every migration file here is multi-statement DDL. conn.execute(text(sql)) failed with "PostgresSyntaxError: cannot insert multiple commands into a prepared statement" for every migration, then cascaded into unrelated test failures (event-loop-closed noise) later in the same CI run. Added _exec_script(), which runs the script through conn.get_raw_connection().driver_connection.execute(sql) -- the raw asyncpg connection's simple query protocol, which has no such restriction. Co-Authored-By: Claude Sonnet 5 --- db/tests/test_schema.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/db/tests/test_schema.py b/db/tests/test_schema.py index 3489726..bf16808 100644 --- a/db/tests/test_schema.py +++ b/db/tests/test_schema.py @@ -342,10 +342,19 @@ async def test_forward_then_rollback_restores_baseline() -> None: ``asyncpg`` being the project's only Postgres driver dependency), so this uses ``create_async_engine`` rather than the sync ``create_engine`` - mixing a sync engine with an async-only driver raises - ``sqlalchemy.exc.MissingGreenlet``. + ``sqlalchemy.exc.MissingGreenlet``. Each migration script is applied via + the raw asyncpg connection rather than ``conn.execute(text(...))``: + SQLAlchemy's asyncpg dialect always prepares statements, and Postgres's + extended query protocol rejects multiple commands in one prepared + statement, which every multi-statement migration file here is. """ - from sqlalchemy import inspect, text - from sqlalchemy.ext.asyncio import create_async_engine + from sqlalchemy import inspect + from sqlalchemy.ext.asyncio import AsyncConnection, create_async_engine + + async def _exec_script(conn: AsyncConnection, sql: str) -> None: + """Run a (possibly multi-statement) SQL script via the raw driver connection.""" + raw_connection = await conn.get_raw_connection() + await raw_connection.driver_connection.execute(sql) engine = create_async_engine(os.environ["DATABASE_URL"]) try: @@ -353,7 +362,7 @@ async def test_forward_then_rollback_restores_baseline() -> None: for migration_path in _ALL_FORWARD_MIGRATIONS: migration_sql = _read(migration_path) async with engine.begin() as conn: - await conn.execute(text(migration_sql)) + await _exec_script(conn, migration_sql) async with engine.connect() as conn: has_documents = await conn.run_sync( lambda sync_conn: inspect(sync_conn).has_table("documents") @@ -366,7 +375,7 @@ async def test_forward_then_rollback_restores_baseline() -> None: # Roll back 001 (which cascades to drop all dependent objects). down = _read(_DOWN_SQL) async with engine.begin() as conn: - await conn.execute(text(down)) + await _exec_script(conn, down) async with engine.connect() as conn: sections_remain = await conn.run_sync( lambda sync_conn: inspect(sync_conn).has_table("sections") From c2f23df0368f5a4e5ff90a0bfded59bad5cbdebb Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Mon, 3 Aug 2026 16:37:30 +0530 Subject: [PATCH 6/6] fix(test): correct Settings() alias usage and mypy union-attr in migration test Adding a real Postgres service to CI (previous commit) surfaced a real bug: test_unconfigured_dependencies_report_degraded constructed Settings(database_url=None, qdrant_url=None, ...) using the Python attribute names, but Settings has no populate_by_name=True and its fields are aliased (alias="DATABASE_URL" etc.) with extra="ignore" -- so those two kwargs were silently dropped as unrecognized extras, and the settings fell through to the real DATABASE_URL env var instead of the intended None override. This was invisible before because CI had no Postgres server either way, so the env-var fallback also produced "down" by coincidence. Every other Settings(...) construction in the test suite already uses the alias spelling (DATABASE_URL=, JWT_SECRET=, ...); fixed this one call site to match. Also fixed a new mypy union-attr error from the prior commit's _exec_script helper: AsyncConnection.get_raw_connection()'s driver_connection is typed Any | None. Co-Authored-By: Claude Sonnet 5 --- db/tests/test_schema.py | 4 +++- tests/test_health_internals.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/db/tests/test_schema.py b/db/tests/test_schema.py index bf16808..f578604 100644 --- a/db/tests/test_schema.py +++ b/db/tests/test_schema.py @@ -354,7 +354,9 @@ async def test_forward_then_rollback_restores_baseline() -> None: async def _exec_script(conn: AsyncConnection, sql: str) -> None: """Run a (possibly multi-statement) SQL script via the raw driver connection.""" raw_connection = await conn.get_raw_connection() - await raw_connection.driver_connection.execute(sql) + driver_connection = raw_connection.driver_connection + assert driver_connection is not None + await driver_connection.execute(sql) engine = create_async_engine(os.environ["DATABASE_URL"]) try: diff --git a/tests/test_health_internals.py b/tests/test_health_internals.py index 772fb36..d103a39 100644 --- a/tests/test_health_internals.py +++ b/tests/test_health_internals.py @@ -150,7 +150,7 @@ class TestEvaluateReadiness: async def test_unconfigured_dependencies_report_degraded(self) -> None: """Unconfigured DATABASE_URL/QDRANT_URL surface as degraded + down.""" - settings = Settings(database_url=None, qdrant_url=None, JWT_ISSUER="test-issuer") + settings = Settings(DATABASE_URL=None, QDRANT_URL=None, JWT_ISSUER="test-issuer") readiness = await health.evaluate_readiness(settings) assert readiness.status == "degraded" assert readiness.dependencies == {"postgres": "down", "qdrant": "down"}