Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion .serena/memories/suggested_commands.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 6 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
14 changes: 5 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

</details>

Expand Down Expand Up @@ -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.

</details>

Expand Down
8 changes: 6 additions & 2 deletions src/data_concierge/agents/notebook_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"""

import json
import platform
from datetime import datetime
from typing import Any

Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
8 changes: 7 additions & 1 deletion src/data_concierge/core/config.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Application configuration using Pydantic Settings."""

import os
from functools import lru_cache
from typing import Literal

Expand All @@ -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",
Expand Down
9 changes: 9 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
20 changes: 17 additions & 3 deletions tests/unit/test_mcp_guards.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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",
Expand Down
12 changes: 9 additions & 3 deletions tests/unit/test_notebook_verifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading