From 5d0856eaa6348e0f5f6bb7d6e469dfb3557e03a8 Mon Sep 17 00:00:00 2001 From: Sergi Torres Albert Date: Tue, 28 Jul 2026 19:48:48 +0200 Subject: [PATCH] fix(back): wire DATABASE_URL through to the RAG retrieval path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every generation ran without RAG passages: the system prompt went out with "(no example passages provided)" and the passport carried rag_sources: [], while the request still returned 200 (issue #87 / WO-06). Two independent breaks were masking each other: * The backend never read `.env`, despite README/CONTRIBUTING/DEPLOYMENT all instructing developers to create one. app.config now loads the repo-root `.env` on import, which is enough for the whole process because every entry point reaches configuration through that module. * `routes/generate.py` never forwarded `database_url` to `orchestrate()`, so `autoria_ai.db` fell back to `os.environ["DATABASE_URL"]` and the resulting KeyError was swallowed by generator.py's `except Exception` — turning a configuration error into a per-request warning. `.env` is loaded with `override=False`: Railway and Vercel inject the real secrets into the process environment, and a stale `.env` shipped in an image must never outrank them. DATABASE_URL is normalised to the `postgresql+asyncpg://` form in app.config.to_asyncpg_dsn. `.env.example` documents the plain `postgresql://` scheme (what Supabase hands out, what the psycopg2 seeding scripts consume) but SQLAlchemy's create_async_engine rejects a driver-less URL, so the backend translates at the boundary — the same rule, in the same direction, as scripts/seed_corpus.py:to_asyncpg_url. `ai_pipeline/autoria_ai/db.py` is untouched; its contract was already correct. A missing DATABASE_URL is now reported once, explicitly, at import, instead of surfacing as a generic "RAG retrieval failed" warning on every generation. Co-Authored-By: Claude Opus 5 --- backend/app/config.py | 126 ++++++++++++++++++++++++++++++++- backend/app/routes/generate.py | 10 ++- backend/pyproject.toml | 4 ++ backend/requirements.txt | 3 + backend/tests/test_config.py | 114 +++++++++++++++++++++++++++++ backend/tests/test_generate.py | 72 +++++++++++++++++++ 6 files changed, 327 insertions(+), 2 deletions(-) diff --git a/backend/app/config.py b/backend/app/config.py index 9f4a60e..e1bfc2a 100644 --- a/backend/app/config.py +++ b/backend/app/config.py @@ -4,6 +4,13 @@ ``os.getenv`` directly. On Railway/Vercel these values are injected by the platform; locally they come from ``.env`` (see ``.env.example``). +Importing this module loads the repo-root ``.env`` into ``os.environ`` with +``override=False`` — see ``_load_dotenv_once()``. Every backend entry point +reaches configuration through here (``app.main``, ``app.db``, +``app.services.watsonx_client``, ``app.routes.diagnostics``), so that single +import is enough to give ``uvicorn app.main:app`` the same environment the +platform dashboards inject in production. + Secrets are NEVER logged or serialized. ``env_report()`` and ``missing_required()`` expose only booleans / names — never the values — so they are safe to surface over the deploy-verification endpoint. @@ -11,8 +18,12 @@ from __future__ import annotations +import logging import os from dataclasses import dataclass +from pathlib import Path + +logger = logging.getLogger(__name__) # Secrets that MUST be injected on BOTH platforms for the app to be healthy. # Keep in sync with .env.example and docs/DEPLOYMENT.md. @@ -28,11 +39,97 @@ _DEFAULT_CORS_ORIGINS = "http://localhost:3000" +# backend/app/config.py -> parents[2] == repo root (where `.env` lives per +# README.md, CONTRIBUTING.md and docs/DEPLOYMENT.md §"Local parity"). +_REPO_ROOT: Path = Path(__file__).resolve().parents[2] +_DOTENV_PATH: Path = _REPO_ROOT / ".env" + def _split_origins(raw: str) -> tuple[str, ...]: return tuple(origin.strip() for origin in raw.split(",") if origin.strip()) +# --------------------------------------------------------------------------- +# .env loading +# --------------------------------------------------------------------------- + + +def _load_dotenv_once() -> bool: + """Load the repo-root ``.env`` into ``os.environ``; return True if read. + + ``override=False`` is load-bearing, not a stylistic default: on Railway and + Vercel the real secrets are injected into the process environment by the + platform, and a stale ``.env`` that happened to ship in the image must + never win over them. Only names that are *absent* from the environment are + filled in, so the platform always has the last word and this call is a + no-op in production (where no ``.env`` exists at all). + + ``python-dotenv`` is a declared runtime dependency (``pyproject.toml`` / + ``requirements.txt``), but its absence is degraded rather than fatal: a + deploy that injects every variable through the dashboard does not need it, + and ``/health`` must keep answering either way. + """ + if not _DOTENV_PATH.is_file(): + return False + try: + from dotenv import load_dotenv + except ImportError: # pragma: no cover - dependency is declared + logger.warning( + "python-dotenv is not installed; %s will NOT be read. " + "Configuration must come from the process environment.", + _DOTENV_PATH, + ) + return False + load_dotenv(_DOTENV_PATH, override=False) + logger.info("Loaded local environment overlay from %s (override=False).", _DOTENV_PATH) + return True + + +_DOTENV_LOADED: bool = _load_dotenv_once() + + +# --------------------------------------------------------------------------- +# DATABASE_URL normalisation +# --------------------------------------------------------------------------- + + +def to_asyncpg_dsn(database_url: str) -> str: + """Return *database_url* in the ``postgresql+asyncpg://`` form. + + ``.env.example`` — the canonical list of variable names and shapes — spells + ``DATABASE_URL`` with the plain ``postgresql://`` scheme, because that is + the DSN Supabase's dashboard hands out and the one psycopg2 consumers + (``scripts/seed_corpus.py``, ``scripts/precompute_umap.py``) expect. The + RAG path is the opposite: ``autoria_ai.db._make_engine`` feeds the value to + SQLAlchemy's ``create_async_engine``, which rejects a driver-less + ``postgresql://`` URL outright. + + Rather than change the documented ``.env`` shape (which would break the + seeding scripts) or ``autoria_ai.db``'s contract (explicitly out of scope + for WO-06), the backend translates at the boundary — the same rule, in the + same direction, as ``scripts/seed_corpus.py:to_asyncpg_url``. Both plain + schemes are accepted; ``postgres://`` is the short form some hosts emit. + + A DSN that already names a driver (``postgresql+asyncpg``, + ``postgresql+psycopg``, ...) is returned untouched, as is any unrecognised + scheme — normalising is a convenience, not a validator, and swallowing an + unknown DSN here would replace one silent failure with another. + """ + if database_url.startswith("postgresql://"): + return "postgresql+asyncpg://" + database_url[len("postgresql://") :] + if database_url.startswith("postgres://"): + return "postgresql+asyncpg://" + database_url[len("postgres://") :] + return database_url + + +def _resolve_database_url() -> str | None: + """Read ``DATABASE_URL`` and return it as an async-engine-ready DSN.""" + raw = os.getenv("DATABASE_URL") + if not raw: + return None + return to_asyncpg_dsn(raw) + + @dataclass(frozen=True) class Settings: """Snapshot of process configuration, read once at import time.""" @@ -42,6 +139,9 @@ class Settings: watsonx_project_id: str | None supabase_url: str | None supabase_key: str | None + #: Async DSN for the pgvector RAG lookup — always in the + #: ``postgresql+asyncpg://`` form (see ``to_asyncpg_dsn``), never the plain + #: ``postgresql://`` scheme written in ``.env``. ``None`` when unset. database_url: str | None cors_origins: tuple[str, ...] # Passport — paths/ids only; never log the values of *_PATH vars. @@ -59,7 +159,7 @@ def load_settings() -> Settings: watsonx_project_id=os.getenv("WATSONX_PROJECT_ID"), supabase_url=os.getenv("SUPABASE_URL"), supabase_key=os.getenv("SUPABASE_KEY"), - database_url=os.getenv("DATABASE_URL"), + database_url=_resolve_database_url(), cors_origins=_split_origins(os.getenv("AUTORIA_CORS_ORIGINS", _DEFAULT_CORS_ORIGINS)), passport_private_key_path=os.getenv("PASSPORT_PRIVATE_KEY_PATH"), passport_public_key_path=os.getenv("PASSPORT_PUBLIC_KEY_PATH"), @@ -71,6 +171,30 @@ def load_settings() -> Settings: settings = load_settings() +def _log_database_url_status() -> None: + """State once, at import, whether RAG retrieval can work at all. + + Without this the only symptom of an unset ``DATABASE_URL`` is a generic + ``RAG retrieval failed`` warning emitted by + ``autoria_ai.generator.orchestrate`` on *every* generation, while the + request still returns 200 with ``rag_sources: []`` — a configuration error + disguised as a per-request hiccup (issue #87). + """ + if settings.database_url: + return + logger.error( + "DATABASE_URL is not set: RAG retrieval is disabled for this process. " + "Every /api/generate will fall back to an unconditioned prompt " + "('(no example passages provided)') and issue a passport with " + "rag_sources: []. Set DATABASE_URL in %s (local) or in the Railway " + "service Variables (deploy). See .env.example.", + _DOTENV_PATH, + ) + + +_log_database_url_status() + + def env_report() -> dict[str, bool]: """Presence map for the required secrets — booleans only, never values. diff --git a/backend/app/routes/generate.py b/backend/app/routes/generate.py index a85ca49..f29c922 100644 --- a/backend/app/routes/generate.py +++ b/backend/app/routes/generate.py @@ -24,6 +24,7 @@ from fastapi import APIRouter, HTTPException +from app.config import settings from app.db import get_client from app.schemas import GenerateRequest, GenerateResponse @@ -161,7 +162,6 @@ async def generate_text(body: GenerateRequest) -> GenerateResponse: try: # Import here so the name is available for the except clause below; # the module is already cached by _load_orchestrator's sys.path setup. - from app.config import settings from app.services.watsonx_client import WatsonxError result: dict[str, Any] = await orchestrate( @@ -170,6 +170,14 @@ async def generate_text(body: GenerateRequest) -> GenerateResponse: author_id=body.author_id, author_uuid=author_uuid, model_id=_MODEL_ID, + # Without this the orchestrator calls retrieve_top_k(database_url= + # None), autoria_ai.db falls back to os.environ["DATABASE_URL"], + # and the resulting KeyError is swallowed by generator.py's + # `except Exception` — so every generation silently ran with zero + # RAG passages and shipped a passport with rag_sources: [] (#87). + # settings.database_url is already the +asyncpg form that + # create_async_engine requires (see app.config.to_asyncpg_dsn). + database_url=settings.database_url, verifier_url=settings.passport_verifier_url, ) except WatsonxError as exc: diff --git a/backend/pyproject.toml b/backend/pyproject.toml index f06d2ac..a4ef208 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -17,6 +17,10 @@ dependencies = [ "ibm-watsonx-ai>=1.0", "python-jose[cryptography]>=3.3", "jsonschema[format]>=4.18", + # Reads the repo-root `.env` in app/config.py so `uvicorn app.main:app` + # sees the same variables Railway/Vercel inject (loaded with + # override=False — the platform environment always wins). + "python-dotenv>=1.0", ] [project.optional-dependencies] diff --git a/backend/requirements.txt b/backend/requirements.txt index fc7a9f2..3322629 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -10,3 +10,6 @@ python-multipart>=0.0.9 ibm-watsonx-ai>=1.0 python-jose[cryptography]>=3.3 jsonschema[format]>=4.18 +# Local `.env` overlay read by app/config.py (override=False — platform-injected +# variables always win). requirements-deploy.txt pulls this file in via `-r`. +python-dotenv>=1.0 diff --git a/backend/tests/test_config.py b/backend/tests/test_config.py index f833e59..ff70a3f 100644 --- a/backend/tests/test_config.py +++ b/backend/tests/test_config.py @@ -1,10 +1,14 @@ """Tests for app.config env-var wiring.""" +from pathlib import Path + +import app.config as config from app.config import ( REQUIRED_ENV_VARS, env_report, load_settings, missing_required, + to_asyncpg_dsn, ) @@ -43,3 +47,113 @@ def test_cors_origins_defaults_to_localhost(monkeypatch): monkeypatch.delenv("AUTORIA_CORS_ORIGINS", raising=False) settings = load_settings() assert settings.cors_origins == ("http://localhost:3000",) + + +# --------------------------------------------------------------------------- +# DATABASE_URL — asyncpg DSN normalisation (issue #87 / WO-06) +# +# `.env.example` documents the plain `postgresql://` scheme (what Supabase +# hands out and what the psycopg2 seeding scripts consume), but the RAG path +# ends in SQLAlchemy's create_async_engine, which rejects a driver-less URL. +# config.py translates at the boundary, exactly like +# scripts/seed_corpus.py:to_asyncpg_url. +# --------------------------------------------------------------------------- + +_PLAIN_DSN = "postgresql://postgres.abc:pw@aws-0-eu-central-1.pooler.supabase.com:5432/postgres" +_ASYNC_DSN = ( + "postgresql+asyncpg://postgres.abc:pw@aws-0-eu-central-1.pooler.supabase.com:5432/postgres" +) + + +def test_to_asyncpg_dsn_rewrites_plain_postgresql_scheme(): + assert to_asyncpg_dsn(_PLAIN_DSN) == _ASYNC_DSN + + +def test_to_asyncpg_dsn_rewrites_short_postgres_scheme(): + assert to_asyncpg_dsn("postgres://u:p@h:5432/db") == "postgresql+asyncpg://u:p@h:5432/db" + + +def test_to_asyncpg_dsn_is_idempotent(): + assert to_asyncpg_dsn(_ASYNC_DSN) == _ASYNC_DSN + + +def test_to_asyncpg_dsn_preserves_credentials_and_query_string(): + raw = "postgresql://u:p%40ss@h:5432/db?sslmode=require" + assert to_asyncpg_dsn(raw) == "postgresql+asyncpg://u:p%40ss@h:5432/db?sslmode=require" + + +def test_to_asyncpg_dsn_leaves_other_drivers_untouched(): + """Normalisation is a convenience, not a validator — don't mangle inputs.""" + raw = "postgresql+psycopg://u:p@h:5432/db" + assert to_asyncpg_dsn(raw) == raw + + +def test_settings_database_url_is_async_form(monkeypatch): + """load_settings() exposes the DSN create_async_engine can actually open.""" + monkeypatch.setenv("DATABASE_URL", _PLAIN_DSN) + assert load_settings().database_url == _ASYNC_DSN + + +def test_settings_database_url_none_when_unset(monkeypatch): + monkeypatch.delenv("DATABASE_URL", raising=False) + assert load_settings().database_url is None + + +def test_settings_database_url_none_when_empty(monkeypatch): + """An empty value is a missing value — never hand '' to create_async_engine.""" + monkeypatch.setenv("DATABASE_URL", "") + assert load_settings().database_url is None + + +# --------------------------------------------------------------------------- +# .env overlay — the platform environment must always win +# --------------------------------------------------------------------------- + + +def test_dotenv_path_is_repo_root(): + """The overlay is the repo-root `.env` documented in DEPLOYMENT.md.""" + assert Path(config._REPO_ROOT, ".env") == config._DOTENV_PATH + assert (config._REPO_ROOT / ".env.example").is_file() + + +def test_dotenv_does_not_override_platform_variables(monkeypatch, tmp_path): + """A stale `.env` must never clobber a Railway/Vercel-injected variable. + + The regression this guards: loading with override=True would let a `.env` + baked into the deploy image replace the dashboard's value. + """ + dotenv_file = tmp_path / ".env" + dotenv_file.write_text('AUTORIA_WO06_PROBE="from-dotenv"\n', encoding="utf-8") + monkeypatch.setattr(config, "_DOTENV_PATH", dotenv_file) + + # Simulates the platform injecting the variable into the process env. + monkeypatch.setenv("AUTORIA_WO06_PROBE", "from-platform") + assert config._load_dotenv_once() is True + + import os + + assert os.environ["AUTORIA_WO06_PROBE"] == "from-platform" + + +def test_dotenv_fills_in_variables_absent_from_the_environment(monkeypatch, tmp_path): + """With nothing injected, the `.env` value is what local dev gets.""" + import os + + dotenv_file = tmp_path / ".env" + dotenv_file.write_text('AUTORIA_WO06_PROBE="from-dotenv"\n', encoding="utf-8") + monkeypatch.setattr(config, "_DOTENV_PATH", dotenv_file) + os.environ.pop("AUTORIA_WO06_PROBE", None) + + try: + assert config._load_dotenv_once() is True + assert os.environ["AUTORIA_WO06_PROBE"] == "from-dotenv" + finally: + # load_dotenv writes straight to os.environ, outside monkeypatch's + # undo log — clean up so the name cannot leak into another test. + os.environ.pop("AUTORIA_WO06_PROBE", None) + + +def test_dotenv_load_is_a_noop_when_file_absent(monkeypatch, tmp_path): + """No `.env` (the normal production case) is not an error.""" + monkeypatch.setattr(config, "_DOTENV_PATH", tmp_path / "does-not-exist.env") + assert config._load_dotenv_once() is False diff --git a/backend/tests/test_generate.py b/backend/tests/test_generate.py index c460602..e27ec8b 100644 --- a/backend/tests/test_generate.py +++ b/backend/tests/test_generate.py @@ -23,6 +23,7 @@ from __future__ import annotations +import dataclasses import sys import types import uuid @@ -733,3 +734,74 @@ def test_passport_db_failure_response_still_contains_passport(): assert "passport" in body assert body["passport"]["jws_token"] + + +# =========================================================================== +# 9. DATABASE_URL is propagated to the orchestrator (issue #87 / WO-06) +# +# The orchestrator hands this kwarg straight to +# autoria_ai.db.retrieve_top_k(database_url=...). When the route omitted it, +# autoria_ai.db fell back to os.environ["DATABASE_URL"], the KeyError was +# swallowed by generator.py's `except Exception`, and every generation +# returned 200 with an unconditioned prompt and rag_sources: []. +# =========================================================================== + +_TEST_DSN = "postgresql+asyncpg://user:pw@db.example.com:5432/postgres" + + +def _orchestrate_kwargs_with_settings(db_url: str | None) -> dict[str, Any]: + """Run one happy-path request with ``settings.database_url = db_url``. + + Returns the kwargs the route passed to the orchestrator. + """ + orchestrate = _make_orchestrate_mock() + sb = _make_sb_mock() + patched_settings = dataclasses.replace(_gen_route.settings, database_url=db_url) + + with ( + patch("app.routes.generate.get_client", return_value=sb), + patch("app.routes.generate.settings", patched_settings), + ): + _gen_route._ORCHESTRATE_FN = orchestrate + _gen_route._IMPORT_ERROR = None + TestClient(app, raise_server_exceptions=False).post( + "/api/generate", + json={"author_id": "dickens", "prompt": "A foggy night."}, + ) + + _, kwargs = orchestrate.call_args + return kwargs + + +def test_orchestrate_receives_database_url_kwarg(): + """The route must pass ``database_url`` — its absence is the #87 bug.""" + kwargs = _orchestrate_kwargs_with_settings(_TEST_DSN) + assert "database_url" in kwargs + + +def test_orchestrate_receives_configured_dsn(): + """The DSN forwarded is exactly the one resolved by app.config.""" + kwargs = _orchestrate_kwargs_with_settings(_TEST_DSN) + assert kwargs["database_url"] == _TEST_DSN + + +def test_orchestrate_receives_none_when_database_url_unset(): + """An unset DATABASE_URL forwards None — RAG degrades, request still 200. + + The explicit one-time startup log (app.config) is what surfaces the + misconfiguration; the request path keeps its documented degradation. + """ + kwargs = _orchestrate_kwargs_with_settings(None) + assert kwargs["database_url"] is None + + +def test_route_reads_dsn_from_settings_not_os_environ(monkeypatch): + """The DSN comes from app.config.settings, not a direct os.getenv call. + + Guards the WO-06 contract: config.py owns DATABASE_URL (including the + +asyncpg normalisation), so a raw env var that config never saw must not + reach the orchestrator. + """ + monkeypatch.setenv("DATABASE_URL", "postgresql://raw:env@bypass.example.com:5432/postgres") + kwargs = _orchestrate_kwargs_with_settings(_TEST_DSN) + assert kwargs["database_url"] == _TEST_DSN