Skip to content
Open
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
50 changes: 50 additions & 0 deletions .console/log.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 31 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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)
53 changes: 53 additions & 0 deletions tests/test_env_isolation.py
Original file line number Diff line number Diff line change
@@ -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
Loading