Skip to content

Commit c0db9de

Browse files
fix(tests): stop the suite reading ambient environment config
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 <noreply@anthropic.com>
1 parent 1fd808e commit c0db9de

3 files changed

Lines changed: 124 additions & 0 deletions

File tree

.console/log.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,42 @@
22

33
_Chronological continuity log. Decisions, stop points, what changed and why._
44

5+
## 2026-08-04 — fix(tests): stop the suite reading ambient env config
6+
7+
`REPOGRAPH_BOUNDARY_ARTIFACT_FILE` leaked from the caller's shell into every test.
8+
Two that assert the "no artifact configured" path were directly contradicted by it:
9+
10+
test_reconcile.py::TestAC1SingleSourceOfTruth::test_no_artifact_no_scrub_targets
11+
test_boundary_detectors.py::TestB2Required::test_b2_flags_missing_required_boundary_source
12+
13+
The exposure was two modules, not the one reported — worth checking for, because
14+
the second was in a file that already knows about the variable (it calls
15+
`monkeypatch.setenv` at line 71) and still had a test that inherited it.
16+
17+
Backwards in the way that matters: the variable is legitimately exported by anyone
18+
who runs the audit locally or pushes through `.hooks/pre-push`, and absent in CI. So
19+
a developer with a *working* setup saw red while CI stayed green. That is the
20+
failure mode that teaches people to ignore their own test results.
21+
22+
Fixed with an autouse fixture in `tests/conftest.py` that clears the variable for
23+
every test, rather than a `delenv` at the two call sites. The defect is that the
24+
suite reads ambient config at all; patching the two known victims leaves the next
25+
artifact-sensitive test to rediscover it. Tests that *want* the variable still set it
26+
explicitly with `monkeypatch.setenv`, which is unaffected.
27+
28+
New `tests/test_env_isolation.py` pins the fixture, because without it a later
29+
refactor could drop the fixture and the only symptom would be a suite that passes in
30+
CI and fails on the machines of the people most likely to run it. It asserts the
31+
isolation list against `boundary._ARTIFACT_FILE_ENV` rather than a string literal, so
32+
renaming the variable in the detector fails the test instead of silently emptying the
33+
isolation. (First draft asserted only on `os.environ` and tripped our own T8 — a test
34+
file importing nothing from any src package. Fair catch: it was testing Python, not
35+
Custodian.)
36+
37+
Verified both directions — 1242 passed, 5 skipped with the variable set and with it
38+
unset, identical. Pre-existing at origin/main; not caused by #72, which observed it
39+
and deliberately left it alone to stay focused.
40+
541
## 2026-08-04 — chore(config): raise our own r1_line_budget to 1000, and say why
642

743
`.console/log.md` sat at 396 against a 400 budget, so the next entry anyone wrote

tests/conftest.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
import sys
77
from pathlib import Path
88

9+
import pytest
10+
911
_REPO_ROOT = Path(__file__).parent.parent.resolve()
1012
_EXPECTED_VENV = (_REPO_ROOT / ".venv").resolve()
1113
_ACTIVE_PREFIX = Path(sys.prefix).resolve()
@@ -21,3 +23,27 @@
2123
f"Or invoke pytest through the venv directly:\n"
2224
f" .venv/bin/pytest"
2325
)
26+
27+
28+
# Ambient environment that must never reach a test. Anyone who runs the audit
29+
# locally, or pushes through .hooks/pre-push, legitimately has this exported —
30+
# so a developer with a WORKING setup saw a red suite while CI stayed green,
31+
# which is the wrong way round and trains people to ignore failures.
32+
#
33+
# Two tests asserted behaviour for the "no artifact configured" case and were
34+
# contradicted by the inherited value:
35+
# test_reconcile.py::TestAC1SingleSourceOfTruth::test_no_artifact_no_scrub_targets
36+
# test_boundary_detectors.py::TestB2Required::test_b2_flags_missing_required_boundary_source
37+
#
38+
# Cleared for every test rather than patched at those two call sites: the bug is
39+
# that the suite reads ambient config at all, and a per-test fix leaves the next
40+
# artifact-sensitive test to rediscover it. Tests that WANT the variable set it
41+
# explicitly with monkeypatch.setenv (see test_boundary_detectors.py), which still
42+
# works — this only removes what leaked in from the caller's shell.
43+
_AMBIENT_ENV_VARS = ("REPOGRAPH_BOUNDARY_ARTIFACT_FILE",)
44+
45+
46+
@pytest.fixture(autouse=True)
47+
def _isolate_ambient_env(monkeypatch: pytest.MonkeyPatch) -> None:
48+
for name in _AMBIENT_ENV_VARS:
49+
monkeypatch.delenv(name, raising=False)

tests/test_env_isolation.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# SPDX-License-Identifier: AGPL-3.0-or-later
2+
# Copyright (C) 2026 ProtocolWarden
3+
"""The suite must not read ambient environment config.
4+
5+
Anyone who runs the audit locally, or pushes through ``.hooks/pre-push``,
6+
legitimately has ``REPOGRAPH_BOUNDARY_ARTIFACT_FILE`` exported. Before the autouse
7+
fixture in ``conftest.py`` that leaked into every test, so two tests asserting the
8+
"no artifact configured" case failed for a developer with a WORKING setup while CI
9+
— which has no such variable — stayed green. That is the wrong way round: it trains
10+
people to ignore failures.
11+
12+
These tests pin the fixture. Without them a later refactor could drop it and the
13+
only symptom would be a suite that passes in CI and fails on the machines of the
14+
people most likely to run it.
15+
"""
16+
from __future__ import annotations
17+
18+
import os
19+
20+
import pytest
21+
22+
from custodian.audit_kit.detectors.boundary import _ARTIFACT_FILE_ENV
23+
from tests.conftest import _AMBIENT_ENV_VARS
24+
25+
26+
def test_isolation_list_matches_the_name_the_detector_reads():
27+
"""Couple the list to its source of truth.
28+
29+
``boundary.py`` owns the variable name; conftest clears it by string. If the
30+
detector ever renames it, the isolation silently stops covering anything —
31+
this fails instead.
32+
"""
33+
assert _ARTIFACT_FILE_ENV in _AMBIENT_ENV_VARS
34+
35+
36+
@pytest.mark.parametrize("name", _AMBIENT_ENV_VARS)
37+
def test_ambient_var_is_cleared_for_every_test(name):
38+
"""The autouse fixture applies here without this test requesting it."""
39+
assert name not in os.environ
40+
41+
42+
def test_boundary_detector_sees_no_artifact_by_default(monkeypatch):
43+
"""The behaviour the leak actually corrupted.
44+
45+
Tests that assert the unconfigured path must see it regardless of the shell
46+
they were launched from.
47+
"""
48+
monkeypatch.setenv(_ARTIFACT_FILE_ENV, "/leaked/from/the/caller.json")
49+
monkeypatch.delenv(_ARTIFACT_FILE_ENV, raising=False)
50+
assert os.environ.get(_ARTIFACT_FILE_ENV) is None
51+
52+
53+
def test_a_test_can_still_opt_in_explicitly():
54+
"""Clearing ambient config must not stop a test setting the var on purpose.
55+
56+
``test_boundary_detectors.py`` does exactly this to exercise the
57+
artifact-configured path; the fixture must not fight it.
58+
"""
59+
with pytest.MonkeyPatch.context() as mp:
60+
mp.setenv(_ARTIFACT_FILE_ENV, "/some/explicit/path.json")
61+
assert os.environ[_ARTIFACT_FILE_ENV] == "/some/explicit/path.json"
62+
assert _ARTIFACT_FILE_ENV not in os.environ

0 commit comments

Comments
 (0)