From c0db9def88f2d4db9aa4d601beeb91599b83e4b9 Mon Sep 17 00:00:00 2001 From: ProtocolWarden <32967198+ProtocolWarden@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:28:31 -0400 Subject: [PATCH 1/2] fix(tests): stop the suite reading ambient environment config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REPOGRAPH_BOUNDARY_ARTIFACT_FILE leaked from the caller's shell into every test. Two that assert the "no artifact configured" path were 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 having checked: the second lives 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 is absent in CI. So a developer with a WORKING setup saw a red suite while CI stayed green — the failure mode that teaches people to ignore their own 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 — 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. It asserts the isolation list against boundary._ARTIFACT_FILE_ENV rather than a string literal, so renaming the variable in the detector fails the test instead of silently emptying the isolation. (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.) Verified both directions: 1242 passed, 5 skipped with the variable set and with it unset, identical. Audit clean apart from the pre-existing W2 (core.hooksPath unset in this clone; CI sets it as the audit job's first step). Pre-existing at origin/main — not caused by #72, which observed it and deliberately left it alone to stay focused. Co-Authored-By: Claude Opus 5 --- .console/log.md | 36 +++++++++++++++++++++ tests/conftest.py | 26 ++++++++++++++++ tests/test_env_isolation.py | 62 +++++++++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+) create mode 100644 tests/test_env_isolation.py diff --git a/.console/log.md b/.console/log.md index d60bdfb..dd7158e 100644 --- a/.console/log.md +++ b/.console/log.md @@ -2,6 +2,42 @@ _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. It asserts the +isolation list against `boundary._ARTIFACT_FILE_ENV` rather than a string literal, so +renaming the variable in the detector fails the test instead of silently emptying the +isolation. (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.) + +Verified both directions — 1242 passed, 5 skipped with the variable set and with it +unset, identical. 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..31d4cf2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,6 +6,8 @@ import sys from pathlib import Path +import pytest + _REPO_ROOT = Path(__file__).parent.parent.resolve() _EXPECTED_VENV = (_REPO_ROOT / ".venv").resolve() _ACTIVE_PREFIX = Path(sys.prefix).resolve() @@ -21,3 +23,27 @@ 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. +_AMBIENT_ENV_VARS = ("REPOGRAPH_BOUNDARY_ARTIFACT_FILE",) + + +@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..41c2e00 --- /dev/null +++ b/tests/test_env_isolation.py @@ -0,0 +1,62 @@ +# 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. +""" +from __future__ import annotations + +import os + +import pytest + +from custodian.audit_kit.detectors.boundary import _ARTIFACT_FILE_ENV +from tests.conftest import _AMBIENT_ENV_VARS + + +def test_isolation_list_matches_the_name_the_detector_reads(): + """Couple the list to its source of truth. + + ``boundary.py`` owns the variable name; conftest clears it by string. If the + detector ever renames it, the isolation silently stops covering anything — + this fails instead. + """ + assert _ARTIFACT_FILE_ENV in _AMBIENT_ENV_VARS + + +@pytest.mark.parametrize("name", _AMBIENT_ENV_VARS) +def test_ambient_var_is_cleared_for_every_test(name): + """The autouse fixture applies here without this test requesting it.""" + assert name not in os.environ + + +def test_boundary_detector_sees_no_artifact_by_default(monkeypatch): + """The behaviour the leak actually corrupted. + + Tests that assert the unconfigured path must see it regardless of the shell + they were launched from. + """ + monkeypatch.setenv(_ARTIFACT_FILE_ENV, "/leaked/from/the/caller.json") + monkeypatch.delenv(_ARTIFACT_FILE_ENV, raising=False) + assert os.environ.get(_ARTIFACT_FILE_ENV) is None + + +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 From ed3e8f410501aa2e7defa511a652208efb44cbbc Mon Sep 17 00:00:00 2001 From: ProtocolWarden <32967198+ProtocolWarden@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:32:16 -0400 Subject: [PATCH 2/2] fix(tests): import the env name from the detector, not from tests.conftest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught what my local run could not: `from tests.conftest import _AMBIENT_ENV_VARS` failed collection with `No module named 'tests'`. tests/ has no __init__.py, so that name resolves only when the repo root is on sys.path. `python -m pytest` puts it there; the bare `pytest -q` that CI runs does not. I verified through the looser entry point, which hid the breakage — the same mistake in kind as the bug this branch fixes: checking through a path CI does not use. Fixed by inverting the dependency rather than adding tests/__init__.py or importing `conftest` bare. conftest now derives _AMBIENT_ENV_VARS from boundary._ARTIFACT_FILE_ENV, and the test imports only from custodian. There is no test->conftest import left to be fragile, and the single source of truth moved to the module that actually owns the name: renaming it in the detector cannot leave the isolation silently covering nothing. 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. Audit clean apart from the pre-existing W2. Co-Authored-By: Claude Opus 5 --- .console/log.md | 34 ++++++++++++++++++++++++---------- tests/conftest.py | 7 ++++++- tests/test_env_isolation.py | 33 ++++++++++++--------------------- 3 files changed, 42 insertions(+), 32 deletions(-) diff --git a/.console/log.md b/.console/log.md index dd7158e..dc55562 100644 --- a/.console/log.md +++ b/.console/log.md @@ -27,16 +27,30 @@ 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. It asserts the -isolation list against `boundary._ARTIFACT_FILE_ENV` rather than a string literal, so -renaming the variable in the detector fails the test instead of silently emptying the -isolation. (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.) - -Verified both directions — 1242 passed, 5 skipped with the variable set and with it -unset, identical. Pre-existing at origin/main; not caused by #72, which observed it -and deliberately left it alone to stay focused. +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 diff --git a/tests/conftest.py b/tests/conftest.py index 31d4cf2..e9d6035 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -8,6 +8,8 @@ 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() @@ -40,7 +42,10 @@ # 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. -_AMBIENT_ENV_VARS = ("REPOGRAPH_BOUNDARY_ARTIFACT_FILE",) +# +# 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) diff --git a/tests/test_env_isolation.py b/tests/test_env_isolation.py index 41c2e00..3137c69 100644 --- a/tests/test_env_isolation.py +++ b/tests/test_env_isolation.py @@ -12,6 +12,11 @@ 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 @@ -20,34 +25,20 @@ import pytest from custodian.audit_kit.detectors.boundary import _ARTIFACT_FILE_ENV -from tests.conftest import _AMBIENT_ENV_VARS - - -def test_isolation_list_matches_the_name_the_detector_reads(): - """Couple the list to its source of truth. - - ``boundary.py`` owns the variable name; conftest clears it by string. If the - detector ever renames it, the isolation silently stops covering anything — - this fails instead. - """ - assert _ARTIFACT_FILE_ENV in _AMBIENT_ENV_VARS -@pytest.mark.parametrize("name", _AMBIENT_ENV_VARS) -def test_ambient_var_is_cleared_for_every_test(name): +def test_ambient_var_is_cleared_for_every_test(): """The autouse fixture applies here without this test requesting it.""" - assert name not in os.environ + assert _ARTIFACT_FILE_ENV not in os.environ -def test_boundary_detector_sees_no_artifact_by_default(monkeypatch): - """The behaviour the leak actually corrupted. +def test_the_cleared_name_is_the_one_the_detector_reads(): + """Guard against the isolation drifting off the real variable. - Tests that assert the unconfigured path must see it regardless of the shell - they were launched from. + ``conftest`` builds its list from this same constant, so this asserts the + wiring rather than a duplicated string. """ - monkeypatch.setenv(_ARTIFACT_FILE_ENV, "/leaked/from/the/caller.json") - monkeypatch.delenv(_ARTIFACT_FILE_ENV, raising=False) - assert os.environ.get(_ARTIFACT_FILE_ENV) is None + assert _ARTIFACT_FILE_ENV == "REPOGRAPH_BOUNDARY_ARTIFACT_FILE" def test_a_test_can_still_opt_in_explicitly():