diff --git a/.console/log.md b/.console/log.md index d60bdfb..dc55562 100644 --- a/.console/log.md +++ b/.console/log.md @@ -2,6 +2,56 @@ _Chronological continuity log. Decisions, stop points, what changed and why._ +## 2026-08-04 — fix(tests): stop the suite reading ambient env config + +`REPOGRAPH_BOUNDARY_ARTIFACT_FILE` leaked from the caller's shell into every test. +Two that assert the "no artifact configured" path were directly contradicted by it: + + test_reconcile.py::TestAC1SingleSourceOfTruth::test_no_artifact_no_scrub_targets + test_boundary_detectors.py::TestB2Required::test_b2_flags_missing_required_boundary_source + +The exposure was two modules, not the one reported — worth checking for, because +the second was in a file that already knows about the variable (it calls +`monkeypatch.setenv` at line 71) and still had a test that inherited it. + +Backwards in the way that matters: the variable is legitimately exported by anyone +who runs the audit locally or pushes through `.hooks/pre-push`, and absent in CI. So +a developer with a *working* setup saw red while CI stayed green. That is the +failure mode that teaches people to ignore their own test results. + +Fixed with an autouse fixture in `tests/conftest.py` that clears the variable for +every test, rather than a `delenv` at the two call sites. The defect is that the +suite reads ambient config at all; patching the two known victims leaves the next +artifact-sensitive test to rediscover it. Tests that *want* the variable still set it +explicitly with `monkeypatch.setenv`, which is unaffected. + +New `tests/test_env_isolation.py` pins the fixture, because without it a later +refactor could drop the fixture and the only symptom would be a suite that passes in +CI and fails on the machines of the people most likely to run it. `conftest` builds +its list from `boundary._ARTIFACT_FILE_ENV` rather than a literal, so renaming the +variable in the detector cannot leave the isolation silently covering nothing. + +Two self-inflicted detours worth recording, both caught by tooling rather than by +me: + +- The first draft asserted only on `os.environ` and tripped our own **T8** — a test + file importing nothing from any src package. Fair catch; it was testing Python, not + Custodian. +- The second imported the list via `from tests.conftest import ...` and passed + locally but **failed collection in CI**: `No module named 'tests'`. `tests/` has no + `__init__.py`, so that name resolves only when the repo root is on `sys.path` — + true under `python -m pytest`, which is what I verified with, and false under the + bare `pytest` CI runs. The lesson is the same one this entry is about: verifying + through a different entry point than CI uses hides exactly the class of bug being + fixed. Resolved by inverting the dependency — conftest derives the name from the + detector, and the test imports only from `custodian`, so no test→conftest import + exists to be fragile. + +Verified through the CI invocation this time — bare `pytest -q`, with the variable +set and unset, plus `python -m pytest`: 1241 passed, 5 skipped, identical across all +three. Pre-existing at origin/main; not caused by #72, which observed it and +deliberately left it alone to stay focused. + ## 2026-08-04 — chore(config): raise our own r1_line_budget to 1000, and say why `.console/log.md` sat at 396 against a 400 budget, so the next entry anyone wrote diff --git a/tests/conftest.py b/tests/conftest.py index d98b98d..e9d6035 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,6 +6,10 @@ import sys from pathlib import Path +import pytest + +from custodian.audit_kit.detectors.boundary import _ARTIFACT_FILE_ENV + _REPO_ROOT = Path(__file__).parent.parent.resolve() _EXPECTED_VENV = (_REPO_ROOT / ".venv").resolve() _ACTIVE_PREFIX = Path(sys.prefix).resolve() @@ -21,3 +25,30 @@ f"Or invoke pytest through the venv directly:\n" f" .venv/bin/pytest" ) + + +# Ambient environment that must never reach a test. Anyone who runs the audit +# locally, or pushes through .hooks/pre-push, legitimately has this exported — +# so a developer with a WORKING setup saw a red suite while CI stayed green, +# which is the wrong way round and trains people to ignore failures. +# +# Two tests asserted behaviour for the "no artifact configured" case and were +# contradicted by the inherited value: +# test_reconcile.py::TestAC1SingleSourceOfTruth::test_no_artifact_no_scrub_targets +# test_boundary_detectors.py::TestB2Required::test_b2_flags_missing_required_boundary_source +# +# Cleared for every test rather than patched at those two call sites: the bug is +# that the suite reads ambient config at all, and a per-test fix leaves the next +# artifact-sensitive test to rediscover it. Tests that WANT the variable set it +# explicitly with monkeypatch.setenv (see test_boundary_detectors.py), which still +# works — this only removes what leaked in from the caller's shell. +# +# Taken from the detector rather than spelled as a literal: boundary.py owns the +# name, so renaming it there cannot leave this isolation silently covering nothing. +_AMBIENT_ENV_VARS = (_ARTIFACT_FILE_ENV,) + + +@pytest.fixture(autouse=True) +def _isolate_ambient_env(monkeypatch: pytest.MonkeyPatch) -> None: + for name in _AMBIENT_ENV_VARS: + monkeypatch.delenv(name, raising=False) diff --git a/tests/test_env_isolation.py b/tests/test_env_isolation.py new file mode 100644 index 0000000..3137c69 --- /dev/null +++ b/tests/test_env_isolation.py @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 ProtocolWarden +"""The suite must not read ambient environment config. + +Anyone who runs the audit locally, or pushes through ``.hooks/pre-push``, +legitimately has ``REPOGRAPH_BOUNDARY_ARTIFACT_FILE`` exported. Before the autouse +fixture in ``conftest.py`` that leaked into every test, so two tests asserting the +"no artifact configured" case failed for a developer with a WORKING setup while CI +— which has no such variable — stayed green. That is the wrong way round: it trains +people to ignore failures. + +These tests pin the fixture. Without them a later refactor could drop it and the +only symptom would be a suite that passes in CI and fails on the machines of the +people most likely to run it. + +Imports the variable name from the detector that owns it, never from ``conftest``: +``tests/`` is not a package, so ``from tests.conftest import ...`` resolves only when +the repo root happens to be on ``sys.path`` — true under ``python -m pytest``, false +under the bare ``pytest`` CI runs. +""" +from __future__ import annotations + +import os + +import pytest + +from custodian.audit_kit.detectors.boundary import _ARTIFACT_FILE_ENV + + +def test_ambient_var_is_cleared_for_every_test(): + """The autouse fixture applies here without this test requesting it.""" + assert _ARTIFACT_FILE_ENV not in os.environ + + +def test_the_cleared_name_is_the_one_the_detector_reads(): + """Guard against the isolation drifting off the real variable. + + ``conftest`` builds its list from this same constant, so this asserts the + wiring rather than a duplicated string. + """ + assert _ARTIFACT_FILE_ENV == "REPOGRAPH_BOUNDARY_ARTIFACT_FILE" + + +def test_a_test_can_still_opt_in_explicitly(): + """Clearing ambient config must not stop a test setting the var on purpose. + + ``test_boundary_detectors.py`` does exactly this to exercise the + artifact-configured path; the fixture must not fight it. + """ + with pytest.MonkeyPatch.context() as mp: + mp.setenv(_ARTIFACT_FILE_ENV, "/some/explicit/path.json") + assert os.environ[_ARTIFACT_FILE_ENV] == "/some/explicit/path.json" + assert _ARTIFACT_FILE_ENV not in os.environ