From aa1afb8a780b1c6df9211fb1a31488fc92a79192 Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:26:07 -0400 Subject: [PATCH 1/2] test: keep .env out of the suite; report the real interpreter in notebooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite was documented as hermetic in three places, but core/config.py reads .env for every field and builds its settings singleton at import, so a local .env silently changed what the suite asserted. Following the README's own advice to set NOTEBOOK_VERIFICATION_ENABLED=false turned the suite red, which is how this was found. conftest.py now sets DATA_CONCIERGE_ENV_FILE="" before anything imports config, and config.py honours it (empty means no env file). Three tests asserted shipped defaults or a security guard's default posture by reading the live singleton, which also reflects the process environment; they now assert Settings.model_fields[...] or pin the value with monkeypatch, so an exported variable cannot reach them either. Separately, generated notebooks hardcoded language_info.version = "3.11" in two places — impossible since requires-python became >=3.12. The notebook is the deliverable and its provenance has to be true, so both now report platform.python_version(). Verified: 802 passed with a hostile .env (both flags flipped) AND the same two variables exported in the shell; 802 passed clean; ruff clean. Generated notebook metadata confirmed to carry the running interpreter. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 4 +++- .serena/memories/suggested_commands.md | 2 +- CONTRIBUTING.md | 8 ++++++-- .../agents/notebook_generator.py | 8 ++++++-- src/data_concierge/core/config.py | 8 +++++++- tests/conftest.py | 9 +++++++++ tests/unit/test_mcp_guards.py | 20 ++++++++++++++++--- tests/unit/test_notebook_verifier.py | 12 ++++++++--- 8 files changed, 58 insertions(+), 13 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f1543aa..f7dd490 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,7 +46,9 @@ jobs: # No env vars and no secrets: the suite is hermetic. Every outbound call # is mocked, storage is redirected at a temp dir by tests/conftest.py, # and the notebook-verification tests get their Jupyter kernel from the - # venv's own kernelspec (ipykernel is a dependency). + # venv's own kernelspec (ipykernel is a dependency). conftest.py also + # sets DATA_CONCIERGE_ENV_FILE="" so a local .env — which core/config.py + # reads for every field — cannot change what the suite asserts. # # Do not add `working-directory` here — mcp/registry.py resolves its # config relative to the process cwd, and four tests read it. diff --git a/.serena/memories/suggested_commands.md b/.serena/memories/suggested_commands.md index 89ea120..a0f7083 100644 --- a/.serena/memories/suggested_commands.md +++ b/.serena/memories/suggested_commands.md @@ -20,7 +20,7 @@ Two traps: - **Always run from the repo root.** `mcp/registry.py` sets `MCP_CONFIG_DIR = Path.cwd() / "configs"` at module scope, and several tests read it. This is why `.github/workflows/ci.yml` carries a comment forbidding `working-directory`. - **`addopts` in `pyproject.toml` includes `--cov=src/data_concierge --cov-report=term-missing`**, so a bare `pytest` is slower than CI and prints a coverage table. Pass `--no-cov` to match CI. -The suite is hermetic: no API key, no network, no `.env`. `asyncio_mode = "auto"`, so `async def` tests need no marker. Notebook-verification tests really execute notebooks (the venv's own ipykernel), so a full run is slower than a typical unit suite. +The suite is hermetic: no API key, no network, no `.env`. It actively ignores `.env` — `core/config.py` reads that file for every setting, so `tests/conftest.py` sets `DATA_CONCIERGE_ENV_FILE=""` before anything imports the `settings` singleton. Assert shipped defaults via `Settings.model_fields["x"].default`, never the live `settings`, which still reflects the process environment (a shell `FOO=bar pytest` bypasses the conftest guard). `asyncio_mode = "auto"`, so `async def` tests need no marker. Notebook-verification tests really execute notebooks (the venv's own ipykernel), so a full run is slower than a typical unit suite. ## Lint / format / types ```bash diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8f78fcb..fb5047d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -13,9 +13,13 @@ pytest The full local setup — configuration, sign-in, Docker — is in the [README](README.md). -**The test suite is hermetic.** ~640 tests, under a minute, and it needs no API key, no +**The test suite is hermetic.** ~800 tests, under a minute, and it needs no API key, no network and no `.env`: every outbound call is mocked, and a fixture points storage at a -temporary directory so a run never writes into your checkout. If a change you make can only be +temporary directory so a run never writes into your checkout. It also *ignores* your `.env` — +`core/config.py` reads that file for every setting, so `tests/conftest.py` sets +`DATA_CONCIERGE_ENV_FILE=""` to keep your local configuration out of the run. Assert shipped +defaults against `Settings.model_fields[...]`, not the live `settings` singleton, which still +reflects the process environment. If a change you make can only be tested with a live key or a running service, the test is at the wrong boundary — mock the boundary, or make it a manual script. diff --git a/src/data_concierge/agents/notebook_generator.py b/src/data_concierge/agents/notebook_generator.py index 5e069d8..d16142d 100644 --- a/src/data_concierge/agents/notebook_generator.py +++ b/src/data_concierge/agents/notebook_generator.py @@ -11,6 +11,7 @@ """ import json +import platform from datetime import datetime from typing import Any @@ -300,7 +301,10 @@ async def process(self, state: GraphState) -> GraphState: }, "language_info": { "name": "python", - "version": "3.11", + # The interpreter that actually ran this, not a hardcoded one: + # the notebook is the deliverable and its provenance has to be + # true. A pinned "3.11" outlived support for 3.11 entirely. + "version": platform.python_version(), "codemirror_mode": {"name": "ipython", "version": 3}, "file_extension": ".py", "mimetype": "text/x-python", @@ -1287,7 +1291,7 @@ def generate_notebook_from_trace( }, "language_info": { "name": "python", - "version": "3.11", + "version": platform.python_version(), }, "data_concierge": { "version": "0.1.0", diff --git a/src/data_concierge/core/config.py b/src/data_concierge/core/config.py index a54412a..c6aa6b2 100644 --- a/src/data_concierge/core/config.py +++ b/src/data_concierge/core/config.py @@ -1,5 +1,6 @@ """Application configuration using Pydantic Settings.""" +import os from functools import lru_cache from typing import Literal @@ -11,7 +12,12 @@ class Settings(BaseSettings): """Application settings loaded from environment variables.""" model_config = SettingsConfigDict( - env_file=".env", + # `.env` feeds every field here, so it reaches a test run too: the suite + # is otherwise only as hermetic as whatever happens to be in the working + # copy's .env, and a developer following the README's own advice to + # disable notebook verification turned the suite red. tests/conftest.py + # sets this to "" so a run never depends on it. Empty means no file. + env_file=os.environ.get("DATA_CONCIERGE_ENV_FILE", ".env") or None, env_file_encoding="utf-8", case_sensitive=False, extra="ignore", diff --git a/tests/conftest.py b/tests/conftest.py index f839e53..33ba100 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,8 +14,17 @@ original. """ +import os + import pytest +# Set before anything imports `data_concierge.core.config`, which builds its +# `settings` singleton at import time. `.env` feeds every field, so without this +# the suite inherits whatever a developer happens to have configured locally — +# which is how a documented `NOTEBOOK_VERIFICATION_ENABLED=false` turned the +# suite red. Empty means "no env file"; export it to a path to opt back in. +os.environ.setdefault("DATA_CONCIERGE_ENV_FILE", "") + @pytest.fixture(autouse=True, scope="session") def _isolate_storage_from_the_repo(tmp_path_factory): diff --git a/tests/unit/test_mcp_guards.py b/tests/unit/test_mcp_guards.py index 3e99e2b..5956819 100644 --- a/tests/unit/test_mcp_guards.py +++ b/tests/unit/test_mcp_guards.py @@ -68,9 +68,15 @@ def test_metadata_refused_even_with_private_allowed(self) -> None: validate_server_url("http://169.254.169.254/", allow_private=True) def test_ships_locked_down(self) -> None: - from data_concierge.core.config import settings + """Asserts the declared default, not the live singleton. - assert settings.mcp_allow_private_urls is False + `MCP_ALLOW_PRIVATE_URLS` exists precisely so local development can point + at localhost MCP servers, so the one developer most likely to run this + suite is the one most likely to have it set. + """ + from data_concierge.core.config import Settings + + assert Settings.model_fields["mcp_allow_private_urls"].default is False class TestSseEndpointPinning: @@ -133,13 +139,21 @@ def test_model_validator_rejects_metadata_url(self) -> None: url="http://169.254.169.254/latest/meta-data/", ) - def test_model_validator_rejects_on_update_construction(self) -> None: + def test_model_validator_rejects_on_update_construction( + self, monkeypatch: "pytest.MonkeyPatch" + ) -> None: """The update path rebuilds the model, so it is covered too.""" import pytest from pydantic import ValidationError + from data_concierge.core import config from data_concierge.mcp.models import MCPServerConfig, MCPTransportType + # The validator consults the live setting, and MCP_ALLOW_PRIVATE_URLS + # exists for local development — pin the default posture so an exported + # variable cannot quietly turn this guard test into a failure. + monkeypatch.setattr(config.settings, "mcp_allow_private_urls", False) + good = MCPServerConfig( id="x", name="x", transport=MCPTransportType.STREAMABLE_HTTP, url="https://example.com/mcp", diff --git a/tests/unit/test_notebook_verifier.py b/tests/unit/test_notebook_verifier.py index addc453..045e493 100644 --- a/tests/unit/test_notebook_verifier.py +++ b/tests/unit/test_notebook_verifier.py @@ -206,10 +206,16 @@ def _score(): ) def test_ships_enabled(self) -> None: - """Verification is ON (#131) — the egress guard makes it safe.""" - from data_concierge.core.config import settings + """Verification is ON (#131) — the egress guard makes it safe. - assert settings.notebook_verification_enabled is True + Reads the declared default rather than the live `settings` singleton. + conftest already keeps `.env` out of a test run, but the singleton also + reflects the process environment, so asserting the class default is what + makes this independent of the machine it runs on. + """ + from data_concierge.core.config import Settings + + assert Settings.model_fields["notebook_verification_enabled"].default is True def test_disabled_omits_the_factor_entirely(self, monkeypatch: pytest.MonkeyPatch) -> None: """When off, no perpetual 'still running' for a check that never runs. From dd50c723255768b274d1d96ac954b265f20d317e Mon Sep 17 00:00:00 2001 From: Joel Natividad <1980690+jqnatividad@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:34:18 -0400 Subject: [PATCH 2/2] docs: drop stale .env advice from the notebook-verification sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both README sections still described the bug the previous commit fixed. They claimed core/config.py hardcodes env_file=".env" and that a .env entry fails test_ships_enabled — neither is true now: env_file is conditional on DATA_CONCIERGE_ENV_FILE, which tests/conftest.py sets to "", and test_ships_enabled asserts the class default rather than the live singleton. Confirmed by appending the flag to .env and running the file: 33 passed. Setting it in .env is now a fine way to keep a local default, so both sections say so. The behaviour-flags section at :264 carried the same advice plus a cross-reference to the reason being removed, so it is updated too. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index d0d599a..01efdbf 100644 --- a/README.md +++ b/README.md @@ -260,9 +260,8 @@ All settings come from `.env` (see [`.env.example`](.env.example)). **Only Notebook verification executes generated code. It's contained: a subprocess with a hard timeout, a minimal environment allowlist that withholds every credential, shell-escape cells skipped, and an egress guard blocking loopback, private, link-local and cloud-metadata -addresses from inside the kernel. To turn it off, pass `NOTEBOOK_VERIFICATION_ENABLED=false` -in the environment for that run rather than adding it to `.env` — see *Notebook verification -never completes* under Troubleshooting for why. +addresses from inside the kernel. Set `NOTEBOOK_VERIFICATION_ENABLED=false` to turn it off — +in `.env` for a persistent local default, or on the command line for a single run. @@ -444,12 +443,9 @@ depending on the notebook. To skip it, pass the flag for that run: NOTEBOOK_VERIFICATION_ENABLED=false ./run_web.sh ``` -Pass it in the environment, not in `.env`. `core/config.py` sets `env_file=".env"`, and the -test suite reads the same settings singleton — so a `.env` entry also disables verification -under `pytest` and fails -`tests/unit/test_notebook_verifier.py::TestFeatureFlag::test_ships_enabled`, the test that -pins the shipped default to on. A shell variable applies only to the command you prefix, -which is what you want here. +Or set it in `.env` to make it your local default. Either is safe: `tests/conftest.py` sets +`DATA_CONCIERGE_ENV_FILE=""` before anything imports the settings singleton, so the test suite +ignores your `.env` and a local override cannot turn it red.