From e38291f8f6a4d251bf5c4d9941950d25ccb9b0d5 Mon Sep 17 00:00:00 2001 From: Shay Date: Wed, 22 Jul 2026 20:32:06 -0700 Subject: [PATCH] =?UTF-8?q?fix(tests):=20session-scoped=20engine-state=20i?= =?UTF-8?q?solation=20baseline=20=E2=80=94=20close=20module-scope=20escape?= =?UTF-8?q?=20to=20the=20live=20life-store?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- conftest.py | 27 ++++++++++++ .../default-engine-state-test-hygiene.md | 42 ++++++++++++++++++- tests/test_conftest_engine_state_isolation.py | 41 +++++++++++++++++- 3 files changed, 107 insertions(+), 3 deletions(-) diff --git a/conftest.py b/conftest.py index 5b4077c5..839febed 100644 --- a/conftest.py +++ b/conftest.py @@ -29,6 +29,33 @@ import engine_state _USES_DEFAULT_ENGINE_STATE_MARKER = "uses_default_engine_state" +@pytest.fixture(autouse=True, scope="session") +def _isolate_engine_state_session_baseline(tmp_path_factory): + """Session-scoped floor under ``_isolate_engine_state_default``. + + The per-test fixture below is function-scoped, and pytest sets up + higher-scoped fixtures FIRST — so a module-/session-scoped fixture that + constructs a bare ``ChatRuntime()`` (e.g. ``tests/test_achat.py``'s + module-scoped ``runtime``) bound the REAL repo ``engine_state/`` before + the per-test patch existed: it loaded the live life-store and committed + real generations into it (observed 2026-07-22: smoke advanced the live + store's ``turn_count`` 14989→14990 and re-stamped its revision). + + This baseline is set up before fixtures of any narrower scope, so no + pytest-constructed default store can ever resolve to the repo dir. It is + deliberately unconditional: ``uses_default_engine_state`` opts out of the + per-test layer only — those tests verify default-dir RESOLUTION + semantics, which this baseline preserves; they too must never touch the + live life-store. + """ + isolated = tmp_path_factory.mktemp("engine_state_session") + mp = pytest.MonkeyPatch() + mp.setattr(engine_state, "_DEFAULT_DIR", isolated) + mp.setenv("CORE_ENGINE_STATE_DIR", str(isolated)) + yield + mp.undo() + + @pytest.fixture(autouse=True) def _isolate_engine_state_default(request, tmp_path_factory, monkeypatch): """Isolate the default engine-state checkpoint dir per test. diff --git a/docs/issues/default-engine-state-test-hygiene.md b/docs/issues/default-engine-state-test-hygiene.md index 0e0a7ca2..546a646c 100644 --- a/docs/issues/default-engine-state-test-hygiene.md +++ b/docs/issues/default-engine-state-test-hygiene.md @@ -1,7 +1,9 @@ # Issue — tests sharing the default `engine_state/` dir (reproducibility hazard) -Status: open (hygiene; interim rule below, recommended fix deferred to a validated PR) -Date: 2026-06-15 +Status: fix landed in two layers — per-test autouse fixture (recommended fix below, +landed post-2026-06-15) + session-scoped baseline (2026-07-22, module-scope escape); +nightly full lane is the final suite-wide confirmation. See 2026-07-22 addendum. +Date: 2026-06-15 (addendum 2026-07-22) Relates: ADR-0146 (engine-state persistence), ADR-0219 (generation-dir checkpoint), ADR-0220 (identity/provenance reconcile — surfaced the symptom) @@ -113,3 +115,39 @@ The brief is safe and immediately useful (documents the hazard + the rule). The fixture, though small in code, changes default behaviour suite-wide and so must be validated against the full suite — a deliberate cost that belongs in its own PR rather than riding on identity-doctrine or hygiene-doc work. + +## 2026-07-22 addendum — module-scope escape found and closed (session baseline) + +The recommended fixture above landed in the root `conftest.py` +(`_isolate_engine_state_default`) and closed the per-test layer. The weekly +audit (2026-07-22) then caught a **residual escape**: pytest sets up +higher-scoped fixtures before function-scoped ones, so a module-/session-scoped +fixture constructing a bare `ChatRuntime()` binds `engine_state._DEFAULT_DIR` +*before* the per-test patch exists. + +**Observed evidence**: `tests/test_achat.py`'s module-scoped `runtime` fixture +loaded the live repo `engine_state/` during smoke runs (emitting the ADR-0157 +revision-mismatch warning against the real checkpoint) and committed real +generations — the live store's `turn_count` advanced 14989→14990 and +`written_at_revision` was re-stamped to `f94dbd404575` by the suite itself. +Content pollution was nil in that instance (recognizers/candidates byte-identical +across the two test-written generations), but turn-count/lineage provenance was +not test-free. + +**Fix**: `_isolate_engine_state_session_baseline` — a session-scoped autouse +fixture (set up before fixtures of any narrower scope) that points +`engine_state._DEFAULT_DIR` + `CORE_ENGINE_STATE_DIR` at a session temp dir. +The per-test fixture continues to provide per-test freshness on top of it; the +`uses_default_engine_state` marker still opts out of the per-test layer only +(default-dir *resolution* semantics are preserved; the live store stays +unreachable at every scope). + +**Regression pin**: `tests/test_conftest_engine_state_isolation.py::` +`test_module_scoped_construction_cannot_bind_the_live_store` (req #4) — +constructs a default store from a module-scoped fixture and fails if it +resolves to the repo dir. + +**Residual (ledgered, weekly audit T-list)**: the live store's historical +`turn_count` (14990) includes an unknown number of pre-fix test turns, and the +465 persisted discovery candidates predate the fix — a provenance review of +live-store contents is a separate, opt-in remediation ruling. diff --git a/tests/test_conftest_engine_state_isolation.py b/tests/test_conftest_engine_state_isolation.py index 5f675e83..f2d9934c 100644 --- a/tests/test_conftest_engine_state_isolation.py +++ b/tests/test_conftest_engine_state_isolation.py @@ -7,7 +7,9 @@ decoration (CLAUDE.md "Schema-Defined Proof Obligations"): - drop the ``monkeypatch.setattr`` → ``test_default_dir_is_redirected_*`` fails, - drop the ``monkeypatch.setenv`` → ``test_env_var_matches_*`` fails, -- drop the marker opt-out check → ``test_opt_out_marker_*`` fails. +- drop the marker opt-out check → ``test_opt_out_marker_*`` fails, +- drop the session-scoped baseline (``_isolate_engine_state_session_baseline``) + → ``test_module_scoped_construction_cannot_bind_the_live_store`` fails. """ from __future__ import annotations @@ -52,3 +54,40 @@ def test_opt_out_marker_skips_isolation() -> None: assert "engine_state_default" not in str(engine_state._DEFAULT_DIR), ( "uses_default_engine_state marker did not opt out of the isolation fixture" ) + + +# --------------------------------------------------------------------------- +# Module-scope escape regression (weekly audit 2026-07-22) +# +# pytest sets up higher-scoped fixtures BEFORE function-scoped ones, so a +# module-/session-scoped fixture constructing a bare ``ChatRuntime()`` (e.g. +# tests/test_achat.py's module-scoped ``runtime``) binds its store before the +# per-test autouse patch exists. Before the session baseline landed, that +# bound the REAL repo engine_state/ — the live life-store — and smoke runs +# loaded it and committed generations into it (observed 2026-07-22: +# turn_count 14989→14990 written by the suite). +# --------------------------------------------------------------------------- + +_REPO_LIFE_STORE = Path(engine_state.__file__).resolve().parents[1] / "engine_state" + + +@pytest.fixture(scope="module") +def _module_scoped_default_store(): + """Constructed at module setup, before any function-scoped patch.""" + from engine_state import EngineStateStore + + return EngineStateStore() + + +def test_module_scoped_construction_cannot_bind_the_live_store( + _module_scoped_default_store, +) -> None: + """(req #4) fixtures of ANY scope must resolve inside an isolated dir.""" + bound = Path(_module_scoped_default_store.path).resolve() + assert bound != _REPO_LIFE_STORE.resolve(), ( + "module-scoped default store bound the live repo engine_state/ — the " + "session-scoped baseline fixture is missing or ordered too late" + ) + assert "engine_state_session" in str(bound) or "engine_state_default" in str(bound), ( + f"module-scoped store resolved outside every isolation layer: {bound}" + )