From 674024649a307283c63890bc62b30a6ddc5dfe47 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Mon, 24 Aug 2026 19:47:02 -0400 Subject: [PATCH 1/3] fix(witan-code): stop four tests asserting the machine's logged-out state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `identity.actor_id()` reads the real ~/.config/witan/config.toml and the real OIDC token cache, and nothing in the suite stopped it. So on any machine that had run `witan login` the branch views got namespaced under the resolved actor and `check_writable` took its logged-in branch, while four tests asserted the un-namespaced names and the logged-out refusal prose. They were green in CI the whole time: a GitHub runner has no witan identity, so CI only ever exercised the half those tests describe. Four standing local failures is the number at which a contributor stops reading the summary line. #276, #277 and #278 each spent a stash-and-rerun establishing they were pre-existing. `_fresh_identity` now points WITAN_CONFIG and WITAN_TOKEN_CACHE at tmp_path and clears the env vars that short-circuit ahead of them, making logged-out deterministic rather than incidental. `logged_in_actor` opts back in through WITAN_ACTOR — the path a non-interactive writer actually takes — and covers the half a CI runner cannot reach: a logged-in writer's views carry their owner, on the local store too. `check_writable` loses its `actor` default, which is what made the guard's behaviour depend on the caller's environment in the first place. Every call site already resolves an actor, because it needs one to name the view it is about to write. The fallback also made `ingest` wrong: a request arriving with no actor was judged against the serving process's identity instead of being refused for having none. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RH7kCwSp8TbKQLZ7kbVnY1 --- mcp/servers/witan-code/CHANGELOG.md | 33 +++++++++++++ mcp/servers/witan-code/tests/conftest.py | 46 ++++++++++++++++++- mcp/servers/witan-code/tests/test_branches.py | 45 ++++++++++++++++++ mcp/servers/witan-code/tests/test_graph.py | 17 +++++++ mcp/servers/witan-code/witan_code/graph.py | 14 ++++-- 5 files changed, 148 insertions(+), 7 deletions(-) diff --git a/mcp/servers/witan-code/CHANGELOG.md b/mcp/servers/witan-code/CHANGELOG.md index d2f9300d..deb01691 100644 --- a/mcp/servers/witan-code/CHANGELOG.md +++ b/mcp/servers/witan-code/CHANGELOG.md @@ -6,6 +6,39 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/) (pre-1.0: a MINOR bump may include breaking changes). +## [Unreleased] + +### Fixed + +- **The test suite no longer asserts an environment.** Four tests + (`test_branches.py`'s three branch-view assertions and + `test_graph.py::test_a_shared_branch_view_needs_an_identity_to_own_it`) + failed on any machine whose `witan login` had resolved an actor, and passed + in CI, where a GitHub runner has none. They asserted un-namespaced view + names and the logged-out refusal prose — the shape identity resolution + happens to take when there is no identity — because nothing stopped + `identity.actor_id()` from reading the real `~/.config/witan/config.toml` + and OIDC token cache. + + The `_fresh_identity` fixture now points `WITAN_CONFIG` and + `WITAN_TOKEN_CACHE` at `tmp_path` and clears the four env vars that + short-circuit ahead of them, so logged-out is deterministic everywhere. A + new `logged_in_actor` fixture opts back in through `WITAN_ACTOR`, and + `test_a_logged_in_writer_owns_the_views_it_indexes` uses it to cover the + half of `witan_code.views` a CI runner cannot reach: on a logged-in machine + the views carry their owner, on the local store too. + +### Changed + +- **`graph.check_writable` requires `actor`; `None` now means "no identity", + full stop.** It used to fall back to `identity.actor_id()`, which gave the + parameter two meanings depending on the caller. Every call site + (`indexer`, `bridge`, `ingest`) already resolves an actor — it needs one to + *name* the view it is about to write — so nothing relied on the fallback, + and for `ingest` it was wrong: a request arriving with no actor was judged + against the serving process's identity rather than being refused for having + none. + ## [0.15.0] - 2026-08-21 ### Changed diff --git a/mcp/servers/witan-code/tests/conftest.py b/mcp/servers/witan-code/tests/conftest.py index afe01f25..8f64bf4d 100644 --- a/mcp/servers/witan-code/tests/conftest.py +++ b/mcp/servers/witan-code/tests/conftest.py @@ -41,21 +41,63 @@ def main(): """ +# Every input `identity._resolve` reads. Isolating the file paths is not +# enough on its own: each of these env vars short-circuits ahead of them. +_IDENTITY_ENV = ( + "WITAN_ACTOR", + "WITAN_REMOTE_URL", + "WITAN_OIDC_ISSUER", + "WITAN_TARGET", +) + + @pytest.fixture(autouse=True) -def _fresh_identity(): - """Forget the process-lifetime actor between tests. +def _fresh_identity(tmp_path, monkeypatch): + """Pin the identity these tests run under: nobody, unless one is asked for. + Two jobs. The first is to forget the process-lifetime actor between tests — ``identity.actor_id`` memoizes deliberately (a witan-code process writes as exactly one identity), which without this would let the first test that resolves one decide the branch-view names for every test after it. + + The second is to stop that resolution reaching the machine. It reads the + real ``~/.config/witan/config.toml`` and the real OIDC token cache, so on a + developer's box — logged in, unlike a CI runner — ``actor_id()`` returns an + ``act-…`` and the branch views get namespaced under it. Four tests asserted + un-namespaced names and failed for everyone who had run ``witan login``, + green in CI the whole time. Pointing both at ``tmp_path`` makes logged-out + the deterministic default everywhere; ``logged_in_actor`` opts back in. """ from witan_code import identity + monkeypatch.setenv("WITAN_CONFIG", str(tmp_path / "witan-config.toml")) + monkeypatch.setenv("WITAN_TOKEN_CACHE", str(tmp_path / "witan-tokens.json")) + for var in _IDENTITY_ENV: + monkeypatch.delenv(var, raising=False) + identity.reset_cache() yield identity.reset_cache() +@pytest.fixture +def logged_in_actor(monkeypatch): + """Run the test as a specific actor — the half of the guard CI never sees. + + Set through ``WITAN_ACTOR`` rather than by stubbing ``actor_id``, so the + resolution path a non-interactive writer (the CI indexer, a maintenance + job) actually takes is the one under test. + """ + from witan_code import identity + + def _login(actor: str = "act-alice") -> str: + monkeypatch.setenv(identity.ACTOR_ENV_VAR, actor) + identity.reset_cache() + return actor + + return _login + + @pytest.fixture(autouse=True) def _fresh_git_context(): """Forget the memoized git context between tests. diff --git a/mcp/servers/witan-code/tests/test_branches.py b/mcp/servers/witan-code/tests/test_branches.py index 56ce9ec5..2ef0ccf6 100644 --- a/mcp/servers/witan-code/tests/test_branches.py +++ b/mcp/servers/witan-code/tests/test_branches.py @@ -155,6 +155,51 @@ def test_feature_branch_indexes_to_own_branch(tmp_path, monkeypatch): assert not on_main, "main view must not see in-flight branch symbols" +@requires_stack +def test_a_logged_in_writer_owns_the_views_it_indexes( + tmp_path, monkeypatch, logged_in_actor +): + """The other half of `witan_code.views`, which CI never reaches. + + A GitHub runner has no witan identity, so every branch-view assertion above + is of the un-namespaced name. On a logged-in machine — every real user — + the views carry their owner, on the local store too (there is no second + naming rule for local stores). Asserting it here is what keeps the two + halves from drifting. + """ + from witan_code import config as cfg_module + from witan_code import indexer + from witan_code.graph import OmnigraphClient + from witan_code.views import owner + + actor = logged_in_actor("act-alice") + monkeypatch.setenv("WITAN_REPO", REPO) + monkeypatch.setenv("WITAN_CODE_DIR", str(tmp_path / "code")) + cfg = cfg_module.load() + + base = _git_repo(tmp_path / "r") + (base / "svc.py").write_text(SAMPLE) + indexer.index_path(base, config=cfg) + + _git(base, "checkout", "-q", "-b", "feature/new-api") + (base / "extra.py").write_text("def branch_only_symbol():\n return 2\n") + indexer.index_path(base, config=cfg) + + view = f"{actor}/feature_new-api" + store = str(cfg_module.store_path(REPO, cfg.code_dir)) + main_client = OmnigraphClient(store, cfg.queries_dir) + assert view in main_client.list_branches() + assert "feature_new-api" not in main_client.list_branches(), ( + "the un-namespaced name is the collision this scheme replaced" + ) + assert owner(view) == actor + + branch_client = OmnigraphClient(store, cfg.queries_dir, branch=view) + assert branch_client.read( + "code_read.gq", "find_by_name", {"name": "branch_only_symbol"} + ) + + @requires_stack def test_branch_index_writes_to_bridge_overlay_not_main(tmp_path, monkeypatch): """A non-default branch's bridge writes land on its repo-qualified diff --git a/mcp/servers/witan-code/tests/test_graph.py b/mcp/servers/witan-code/tests/test_graph.py index 03f88e31..59fdcd61 100644 --- a/mcp/servers/witan-code/tests/test_graph.py +++ b/mcp/servers/witan-code/tests/test_graph.py @@ -118,6 +118,23 @@ def test_a_shared_branch_view_needs_an_identity_to_own_it(): _check(remote=True, branch="feature-x", actor=None) +def test_no_actor_means_logged_out_even_when_the_machine_has_one(logged_in_actor): + """`actor=None` is "nobody owns this write", never "go ask the process". + + The guard used to fall back to `identity.actor_id()` here, which made the + refusal depend on whether whoever ran it had done a `witan login` — the + test above asserted the logged-out prose and CI, with no identity, was the + only place it held. It also meant a request arriving with no actor + (:mod:`witan_code.ingest`) was judged against the *server's* identity. + """ + logged_in_actor("act-the-machine") + + with pytest.raises(SharedGraphWriteRefused) as excinfo: + _check(remote=True, branch="feature-x", actor=None) + assert "witan login" in str(excinfo.value) + assert "act-the-machine" not in str(excinfo.value) + + def test_local_branch_views_need_no_actor(): """A local store has one user, who owns every view in it — unchanged names, no migration, no login required to index offline.""" diff --git a/mcp/servers/witan-code/witan_code/graph.py b/mcp/servers/witan-code/witan_code/graph.py index f577e4bc..fdf7aa9d 100644 --- a/mcp/servers/witan-code/witan_code/graph.py +++ b/mcp/servers/witan-code/witan_code/graph.py @@ -72,13 +72,18 @@ def check_writable( branch: str | None, cfg: cfg_module.Config, slug: str, - actor: str | None = None, + actor: str | None, ) -> None: """Raise :class:`SharedGraphWriteRefused` unless :func:`owns_view` allows it. - ``actor`` is the identity this process writes as; it defaults to the - resolved one, so a caller that does not construct view names itself does - not have to thread it through. + ``actor`` is the identity the write is being made as, and ``None`` means + exactly one thing: there is no identity to own the view. Required, with no + fallback to :func:`~witan_code.identity.actor_id` — every caller already + resolves an actor, because it needs one to *name* the view it is about to + write, and a caller serving somebody else's write resolves theirs, not this + process's (:mod:`witan_code.ingest`). A default would make ``None`` mean + "logged out" or "ask the machine" depending on who was calling, and the + second reading is the one that cannot be right here. ``is_remote`` is "is this graph shared", which for a client is a property of its store (``client.is_remote``) and for the MCP tier serving somebody @@ -86,7 +91,6 @@ def check_writable( exists (:mod:`witan_code.ingest`). Taking the bit rather than the client is what lets both ask the same question. """ - actor = actor if actor is not None else identity_module.actor_id() if owns_view(is_remote=is_remote, branch=branch, cfg=cfg, actor=actor): return if branch is None: From 67e460e845660e5ff45ff044087cead00c9b8a61 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Tue, 25 Aug 2026 10:04:57 -0400 Subject: [PATCH 2/3] test: stop every suite reading and writing the machine it runs on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The class behind #282 and #285, swept across all five packages instead of fixed a sixth time when it next trips someone. A hostile-environment run — HOME seeded to look like a working developer's machine, a narrow terminal, a witan identity present — found considerably more than the two known cases: 19 of witan's 49 test files created a real graph in ~/.local/share/witan witan-code's test_ingest.py created a real code store in the same tree 8 agent-config-kit tests assert an unwrapped Rich message, so they fail on a narrow terminal; 2 of them already fail at an ordinary width The witan and witan-code leaks were silent. Every test passed. The evidence they had been running for a long time is sitting in the real store directory: https_github.com_test_cg.omni, the fixture repo from test_branches.py. testsupport/hermetic.py redirects HOME, the XDG dirs, the witan state files and both graph stores into a throwaway directory, clears the ambient WITAN_* selectors, and pins the terminal width. Each package gets a rootdir conftest.py that loads it. ★ It runs at IMPORT, not in a fixture, and that is the load-bearing detail. Importing witan.server IS a write — _ensure_graph creates the store at module scope, deliberately, because the CLI's local-dispatch guard depends on it. That import happens during collection, so an autouse fixture body runs long after the store exists. Only something imported by a rootdir conftest is early enough. PATH deliberately keeps its entry for the real ~/.local/bin: that is where CI installs the omnigraph binary and where _find_binary looks when PATH misses. A binary is a tool, not state, and relocating it would break every test that needs a graph to prevent a leak that cannot happen. Redirection fixes today's leaks and not tomorrow's, so pytest_sessionfinish reports anything that reaches the real home anyway — a warning locally, where another agent session may legitimately be writing, and a failure on CI. Strictness is inferred from CI rather than set in each workflow: ten env blocks would be ten chances to miss the tenth, and it covers the publish workflows and whatever gets added next for free. Verified by inducing an actual leak: detected in all four strictness paths, exit 1 where it should fail and 0 where it should warn. 2394 tests pass across the five suites, and the fake home comes back empty. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RH7kCwSp8TbKQLZ7kbVnY1 --- AGENTS.md | 1 + mcp/servers/witan-code/conftest.py | 19 ++ mcp/servers/witan/conftest.py | 19 ++ packages/agent-config-kit/conftest.py | 19 ++ packages/agent-kit/conftest.py | 19 ++ packages/witan-core/conftest.py | 19 ++ .../witan-core/tests/test_hermetic_guard.py | 89 ++++++ testsupport/__init__.py | 6 + testsupport/hermetic.py | 272 ++++++++++++++++++ 9 files changed, 463 insertions(+) create mode 100644 mcp/servers/witan-code/conftest.py create mode 100644 mcp/servers/witan/conftest.py create mode 100644 packages/agent-config-kit/conftest.py create mode 100644 packages/agent-kit/conftest.py create mode 100644 packages/witan-core/conftest.py create mode 100644 packages/witan-core/tests/test_hermetic_guard.py create mode 100644 testsupport/__init__.py create mode 100644 testsupport/hermetic.py diff --git a/AGENTS.md b/AGENTS.md index 4f4953c5..8f7928cb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -94,6 +94,7 @@ See [`skills/workflow/creating-skills/SKILL.md`](./skills/workflow/creating-skil - MD013 (line length) and MD033 (inline HTML) are disabled in markdownlint; long lines in code blocks are fine. - `witan` MCP servers use `uv` exclusively — never `pip` directly. - This repo is one `uv` workspace with a single shared `.venv` at the root (`packages/agent-config-kit`, `packages/agent-kit`, `packages/witan-core`, `mcp/servers/witan`, `mcp/servers/witan-code`) — running `uv sync --package X` then testing package Y against the same env risks cross-contamination (Y sees X's deps, or a stale build of a sibling you just edited). Use the `just test-*` recipes (`justfile`, repo root): each runs `uv run --isolated --package --group test pytest ` in its own throwaway venv, so results can't leak between packages. `just test-all` runs all five concurrently via just's native `[parallel]` recipe attribute. +- **Tests never touch the machine they run on.** Every package's rootdir `conftest.py` loads `testsupport/hermetic.py` (repo root), which redirects `HOME`, the XDG dirs, the witan state files and both graph stores into a throwaway directory, clears the ambient `WITAN_*` selectors, and pins the terminal width — **at import time, not in a fixture**, because importing `witan.server` creates a graph and that happens during collection, before any fixture body runs. A suite that needs one of those values sets it itself; a suite that does not must not inherit yours. The one deliberate exception is `PATH`, which keeps its entry for the real `~/.local/bin` so the omnigraph binary stays findable — a binary is a tool, not state. A `pytest_sessionfinish` hook reports anything that reached the real home anyway: a warning locally (your machine may legitimately be writing there from another session), a failure on CI, inferred from `CI` rather than set per-workflow. Override with `AGENT_KIT_STRICT_HERMETICITY=1|0`. **A new package needs its own rootdir `conftest.py`** — nothing else will supply one, and without it the suite runs against your real home. - Skills are distributed as ZIPs on GitHub releases (tagged `v*`) — the publish workflow handles this automatically. - Each publishable package (`agent-config-kit`, `mcp/servers/witan`, `mcp/servers/witan-code`, `packages/agent-kit`, `packages/witan-core`) carries a `[tool.bumpversion]` config — bump a release with **`just bump patch|minor|major`** (repo root), then commit and push to `main`; each package's `publish-*.yml` workflow tests, builds, publishes to PyPI, and tags the release automatically whenever its `pyproject.toml` version line changes. `just bump` writes the CHANGELOG entry's version into `pyproject.toml` only if that entry already exists, so the changelog and the version cannot drift apart; it wraps the pinned `uvx bump-my-version@1.4.1` (calling that directly still works, but skips the changelog gate and the post-check). Commit `pyproject.toml`, `CHANGELOG.md` **and `uv.lock`** together — the lockfile records workspace member versions and moves with the bump. `just check-versions` asserts version/bumpversion-config/CHANGELOG agree and runs in CI on every PR touching a `pyproject.toml` or `CHANGELOG.md`. **If a change adds a `witan_core` symbol AND a caller of it in `witan`/`witan-code`, raise that server's `witan-core>=X` floor in the same change** — the workspace resolves `witan-core` by path, so the new symbol imports fine everywhere except an external `pip install`, which then fails at import rather than at use. `just check-core-floor` is what catches that: it installs each server's wheel into a clean venv with `witan-core` pinned to exactly its declared floor and imports every module, and it runs in CI on every PR touching either server or `witan-core`. `packages/agent-kit`'s `dependencies` on `agent-config-kit[cli]`/`witan-council`/`witan-code` are open-ended floors (no upper bound), so a new release of any of the three is picked up by a fresh install without `ol-agent-kit` itself needing a release. diff --git a/mcp/servers/witan-code/conftest.py b/mcp/servers/witan-code/conftest.py new file mode 100644 index 00000000..30e4e017 --- /dev/null +++ b/mcp/servers/witan-code/conftest.py @@ -0,0 +1,19 @@ +"""Load the workspace's shared test-environment guard. + +This file exists to be imported EARLY. ``testsupport.hermetic`` redirects every +ambient input — HOME, the XDG dirs, the witan state files and graph stores, the +terminal width — at import time rather than in a fixture, because the leak it +exists to stop can happen while pytest is still collecting (importing +``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest +hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. + +``pytest_plugins`` is only honoured in a rootdir conftest, which is the other +reason this is here and not in ``tests/conftest.py``. +""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[3])) + +pytest_plugins = ["testsupport.hermetic"] diff --git a/mcp/servers/witan/conftest.py b/mcp/servers/witan/conftest.py new file mode 100644 index 00000000..30e4e017 --- /dev/null +++ b/mcp/servers/witan/conftest.py @@ -0,0 +1,19 @@ +"""Load the workspace's shared test-environment guard. + +This file exists to be imported EARLY. ``testsupport.hermetic`` redirects every +ambient input — HOME, the XDG dirs, the witan state files and graph stores, the +terminal width — at import time rather than in a fixture, because the leak it +exists to stop can happen while pytest is still collecting (importing +``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest +hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. + +``pytest_plugins`` is only honoured in a rootdir conftest, which is the other +reason this is here and not in ``tests/conftest.py``. +""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[3])) + +pytest_plugins = ["testsupport.hermetic"] diff --git a/packages/agent-config-kit/conftest.py b/packages/agent-config-kit/conftest.py new file mode 100644 index 00000000..7768b42d --- /dev/null +++ b/packages/agent-config-kit/conftest.py @@ -0,0 +1,19 @@ +"""Load the workspace's shared test-environment guard. + +This file exists to be imported EARLY. ``testsupport.hermetic`` redirects every +ambient input — HOME, the XDG dirs, the witan state files and graph stores, the +terminal width — at import time rather than in a fixture, because the leak it +exists to stop can happen while pytest is still collecting (importing +``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest +hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. + +``pytest_plugins`` is only honoured in a rootdir conftest, which is the other +reason this is here and not in ``tests/conftest.py``. +""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +pytest_plugins = ["testsupport.hermetic"] diff --git a/packages/agent-kit/conftest.py b/packages/agent-kit/conftest.py new file mode 100644 index 00000000..7768b42d --- /dev/null +++ b/packages/agent-kit/conftest.py @@ -0,0 +1,19 @@ +"""Load the workspace's shared test-environment guard. + +This file exists to be imported EARLY. ``testsupport.hermetic`` redirects every +ambient input — HOME, the XDG dirs, the witan state files and graph stores, the +terminal width — at import time rather than in a fixture, because the leak it +exists to stop can happen while pytest is still collecting (importing +``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest +hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. + +``pytest_plugins`` is only honoured in a rootdir conftest, which is the other +reason this is here and not in ``tests/conftest.py``. +""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +pytest_plugins = ["testsupport.hermetic"] diff --git a/packages/witan-core/conftest.py b/packages/witan-core/conftest.py new file mode 100644 index 00000000..7768b42d --- /dev/null +++ b/packages/witan-core/conftest.py @@ -0,0 +1,19 @@ +"""Load the workspace's shared test-environment guard. + +This file exists to be imported EARLY. ``testsupport.hermetic`` redirects every +ambient input — HOME, the XDG dirs, the witan state files and graph stores, the +terminal width — at import time rather than in a fixture, because the leak it +exists to stop can happen while pytest is still collecting (importing +``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest +hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. + +``pytest_plugins`` is only honoured in a rootdir conftest, which is the other +reason this is here and not in ``tests/conftest.py``. +""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +pytest_plugins = ["testsupport.hermetic"] diff --git a/packages/witan-core/tests/test_hermetic_guard.py b/packages/witan-core/tests/test_hermetic_guard.py new file mode 100644 index 00000000..f519bd05 --- /dev/null +++ b/packages/witan-core/tests/test_hermetic_guard.py @@ -0,0 +1,89 @@ +"""Tests for the workspace's shared test-environment guard. + +``testsupport.hermetic`` is loaded by every package's rootdir ``conftest.py``, +so a guard that silently stopped guarding would take all five suites with it — +and by construction the symptom is nothing happening. Hence a test. + +It lives in witan-core's suite rather than a suite of its own because +``testsupport`` is a repo-root module, not a package: it has no ``testpaths`` of +its own for pytest to find, and adding one would mean a sixth workspace package +with a version, a CHANGELOG and a release workflow for thirty lines of test +support. witan-core is the package everything else already depends on, which +makes it the least arbitrary of the five homes available. + +The leak path itself is deliberately NOT exercised here: proving the detector +fires means writing to the real home, which is the one thing this whole +mechanism exists to prevent. That end of it was verified once, by hand, with a +throwaway test that wrote a sentinel into ``~/.local/share/witan`` — the guard +reported it and exited 1 under the strict flag, 0 with a warning without it. +What is asserted here is the logic that decision rests on. +""" + +import os +from pathlib import Path + +import pytest + +from testsupport import hermetic + + +def test_the_redirection_actually_moved_the_home(): + """The premise every other suite is relying on.""" + assert Path.home() == hermetic.FAKE_HOME + assert Path.home() != hermetic.REAL_HOME + assert os.environ["HOME"] == str(hermetic.FAKE_HOME) + + +def test_the_state_files_point_inside_the_fake_home(): + for var in ( + "WITAN_CONFIG", + "WITAN_TOKEN_CACHE", + "WITAN_MERGE_WATERMARKS", + "WITAN_MEMORY_URI", + "WITAN_CODE_DIR", + ): + assert str(hermetic.FAKE_HOME) in os.environ[var], var + + +def test_the_ambient_selectors_are_cleared(): + """A value here is a decision the test did not make — agent-kit#285.""" + for var in hermetic._CLEARED: + assert var not in os.environ, var + + +def test_the_real_local_bin_stays_on_path(): + """The one deliberate hole: omnigraph is a tool, not state. + + CI installs it to the real ``~/.local/bin``, so moving HOME without this + would break every test that needs a graph. + """ + real_bin = hermetic.REAL_HOME / ".local" / "bin" + if not real_bin.is_dir(): + pytest.skip("no ~/.local/bin on this machine to keep reachable") + assert str(real_bin) in os.environ["PATH"] + + +def test_entries_is_empty_for_a_directory_that_is_not_there(): + """The watched directories are optional — a fresh machine has none.""" + assert hermetic._entries(hermetic.FAKE_HOME / "nope" / "still-nope") == set() + + +def test_entries_reports_top_level_names_only(tmp_path): + (tmp_path / "a.omni").mkdir() + (tmp_path / "a.omni" / "buried").write_text("x") + (tmp_path / "b.json").write_text("x") + + assert hermetic._entries(tmp_path) == {"a.omni", "b.json"} + + +def test_a_new_entry_is_what_counts_as_a_leak(tmp_path): + """Growth, not difference: a suite that DELETES something from the real + home is a different (and louder) problem, and pre-existing entries must not + register — a developer's machine is full of them.""" + (tmp_path / "pre-existing").write_text("x") + before = hermetic._entries(tmp_path) + + assert hermetic._entries(tmp_path) - before == set() + + (tmp_path / "leaked").write_text("x") + assert hermetic._entries(tmp_path) - before == {"leaked"} diff --git a/testsupport/__init__.py b/testsupport/__init__.py new file mode 100644 index 00000000..69c72460 --- /dev/null +++ b/testsupport/__init__.py @@ -0,0 +1,6 @@ +"""Test-support shared across the workspace's packages. + +Not a distributed package and deliberately not one: it is imported by each +package's rootdir ``conftest.py`` off the repo root, so it needs no release, +no version, and no place in ``just check-versions``. See ``hermetic``. +""" diff --git a/testsupport/hermetic.py b/testsupport/hermetic.py new file mode 100644 index 00000000..0641d9fe --- /dev/null +++ b/testsupport/hermetic.py @@ -0,0 +1,272 @@ +"""Point every suite's ambient state at a throwaway directory, before import. + +── THE DEFECT CLASS ── + +A test that reads or writes the machine it runs on instead of state it set up +itself. It passes in CI and only in CI, because a GitHub runner is the empty +case of every ambient input there is: no ``witan login``, no agent configs, no +``~/.gitconfig``, and a terminal wide enough that nothing wraps. A test that +asserts any of those *absences* is asserting the runner, not a behaviour. + +It has cost real time in three packages already, each found by a person rather +than by a check: + + agent-kit#282 witan's merge tests wrote to the developer's real + ``~/.config/witan/merge-watermarks.json``, one unbounded + entry per run. Found by opening the file by hand. + agent-kit#285 four witan-code tests asserted the logged-out branch of the + branch-view write guard, so they failed for anyone who had + run ``witan login``. Three unrelated PRs each paid a + stash-and-rerun to prove they were pre-existing. + (this change) 19 of witan's 49 test files created a real graph in + ``~/.local/share/witan``, and ``witan-code``'s + ``test_ingest.py`` created a real code store in + ``~/.local/share/witan/code``. Silently: every test passed. + +── WHY THIS RUNS AT IMPORT, NOT IN A FIXTURE ── + +★ A per-test fixture is structurally too late for the largest case. Importing +``witan.server`` IS a write — ``_ensure_graph`` creates the local store at +module scope, deliberately, because the CLI's local-dispatch guard depends on +it (see that function's docstring). That import happens during COLLECTION, so +by the time any autouse fixture body runs, the store already exists. Hence the +redirection below happens when this module is imported, which a rootdir +``conftest.py`` does before pytest imports a single test module. + +That makes the fake home session-scoped rather than per-test. Deliberate: the +goal is "never touch the real machine", not "isolate tests from each other", +and per-test isolation is what ``tmp_path`` is already for. + +── WHAT IS *NOT* REDIRECTED ── + +``PATH`` keeps its entry for the real ``~/.local/bin``, because that is where +CI installs the omnigraph binary and where ``OmnigraphClient._find_binary`` +looks when ``PATH`` misses. A binary is a tool, not state: relocating it would +break every test that needs a graph, in service of a leak that cannot happen — +nothing writes there. +""" + +from __future__ import annotations + +import atexit +import os +import shutil +import tempfile +from pathlib import Path + +__all__ = ["FAKE_HOME", "REAL_HOME", "STRICT_ENV_VAR"] + +# Captured BEFORE the redirection below, so the PATH entry we re-add points at +# the real one. Order in this module is load-bearing; do not reorder. +REAL_HOME = Path.home() + +FAKE_HOME = Path(tempfile.mkdtemp(prefix="agent-kit-tests-home-")) +atexit.register(shutil.rmtree, FAKE_HOME, True) + +# Every ambient input that would otherwise decide a test's answer. Each is +# either redirected into FAKE_HOME (a location) or cleared (a value), never +# left to the machine. +# +# Cleared rather than redirected: these select or override, so any value at all +# is a decision the test did not make. A suite that wants one sets it itself. +_CLEARED = ( + # Identity and routing — agent-kit#285. WITAN_TARGET and WITAN_REMOTE_URL + # decide local-vs-deployed, WITAN_ACTOR decides who owns a branch view. + "WITAN_ACTOR", + "WITAN_TARGET", + "WITAN_REMOTE_URL", + "WITAN_OIDC_ISSUER", + "WITAN_OIDC_AUDIENCE", + "WITAN_OIDC_CLIENT_ID", + "WITAN_ACTOR_TOKENS_FILE", + "WITAN_MEMORY_TOKEN", + "WITAN_MEMORY_GRAPH", + "WITAN_CODE_TOKEN", + "WITAN_CODE_SERVER", + "WITAN_CODE_TRANSPORT", + "WITAN_CODE_INDEX_ROLE", + "WITAN_REPO", + "WITAN_AGENT", + "WITAN_MODEL", + "WITAN_AUTHOR", + "WITAN_CONTEXT_TTL", + "WITAN_REQUIRE_OMNIGRAPH", + "WITAN_TEST_OMNIGRAPH_SERVER", + "WITAN_TEST_OMNIGRAPH_GRAPH", + "AC_KIT_CONFIG", + # Observability: an exporter endpoint set on a developer's box would have + # the suite emit spans at a real collector. + "OTEL_EXPORTER_OTLP_ENDPOINT", + "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", + "OTEL_EXPORTER_OTLP_METRICS_ENDPOINT", + "SENTRY_DSN", +) + + +def _redirect() -> None: + home = FAKE_HOME + (home / ".local" / "bin").mkdir(parents=True, exist_ok=True) + + for var in _CLEARED: + os.environ.pop(var, None) + + # HOME itself, for the `Path.home()` reads no env var covers — + # agent-config-kit's registry reaches for ~/.claude.json, ~/.claude/, + # ~/.pi/ and ~/.config/opencode directly. + os.environ["HOME"] = str(home) + os.environ["USERPROFILE"] = str(home) # Windows' equivalent + os.environ["XDG_CONFIG_HOME"] = str(home / ".config") + os.environ["XDG_CACHE_HOME"] = str(home / ".cache") + os.environ["XDG_DATA_HOME"] = str(home / ".local" / "share") + + # The state files whose defaults are module-level `Path.home()` constants + # (witan_core.config_file, witan_core.remote.oidc). Moving HOME is not + # enough for those: the constant is evaluated when the module is imported, + # which for a test that imports it from another conftest can precede this. + # Both consult their env var first, so setting it wins whatever the + # constant froze. + os.environ["WITAN_CONFIG"] = str(home / ".config" / "witan" / "config.toml") + os.environ["WITAN_TOKEN_CACHE"] = str(home / ".config" / "witan" / "tokens.json") + os.environ["WITAN_MERGE_WATERMARKS"] = str( + home / ".config" / "witan" / "merge-watermarks.json" + ) + # The graph stores. WITAN_MEMORY_URI is what `witan.server` bootstraps at + # import; without it that import creates a real graph under the (now fake) + # home, which is harmless but slow, and names the store `graph.omni` where + # a suite may expect otherwise. + os.environ["WITAN_MEMORY_URI"] = str(home / "graph.omni") + os.environ["WITAN_CODE_DIR"] = str(home / "code") + + # Deterministic rendering, and WIDE. Rich wraps to the terminal, so a + # message that fits on one line for the author wraps mid-phrase for a + # reviewer with a narrower window, and an `in` assertion against it fails on + # the inserted newline. That is how two agent-config-kit tests came to + # assert a terminal width. + # + # Generous rather than merely fixed, because the wrap point is + # data-dependent as well: these messages interpolate a `tmp_path`, whose + # length varies per run, so a narrow width makes the suite depend on how + # deep pytest's temp directory happened to be. 200 clears every message the + # suite asserts on today with room to spare, while staying narrow enough + # that pytest's own output is still readable in a CI log — which a width of + # 1000 is not. + # + # ★ A width is headroom, not a guarantee. A test asserting a substring of a + # long interpolated string is fragile at ANY finite width; the fix for that + # test is to normalise whitespace before the check, not to widen this. + # + # ★ This pins what the suite asserts (message CONTENT) and says nothing + # about whether the CLI renders READABLY in a narrow terminal. That is a + # question about the product, filed separately; do not read a green suite as + # having answered it. + os.environ["COLUMNS"] = "200" + os.environ["LINES"] = "40" + + # git needs an identity to commit, and the fake home has no ~/.gitconfig. + # Supplying one keeps the many tests that build throwaway repos working + # without each having to pass `-c user.email=...`. + os.environ.setdefault("GIT_AUTHOR_NAME", "agent-kit tests") + os.environ.setdefault("GIT_AUTHOR_EMAIL", "tests@example.invalid") + os.environ.setdefault("GIT_COMMITTER_NAME", "agent-kit tests") + os.environ.setdefault("GIT_COMMITTER_EMAIL", "tests@example.invalid") + + # ★ The one thing deliberately NOT isolated — see the module docstring. + real_bin = REAL_HOME / ".local" / "bin" + if real_bin.is_dir(): + os.environ["PATH"] = f"{real_bin}{os.pathsep}{os.environ.get('PATH', '')}" + + +_redirect() + + +# ── The guard ──────────────────────────────────────────────────────────────── +# +# Redirecting the environment fixes today's leaks; it does not stop tomorrow's. +# A new package without the rootdir conftest, or anything that resolves a real +# path itself rather than through the env, walks straight past everything above +# — and the failure is silent, which is the whole reason this class survived +# three separate discoveries. +# +# So: watch the real home's state directories for the length of the session and +# say something if they grow. Two listdirs, no measurable cost, and it runs +# inside the jobs that already exist rather than adding one that re-runs every +# suite. + +STRICT_ENV_VAR = "AGENT_KIT_STRICT_HERMETICITY" +"""Override the strictness this would otherwise infer. ``1`` on, ``0`` off. + +Strict means a detected leak fails the run instead of warning. The default is +inferred from ``CI`` rather than set per-workflow: every runner exports it, so +strictness lands in all four test workflows AND the five publish ones AND +whatever gets added next, with no list to keep in sync. Ten `env:` blocks would +have been ten chances to forget the tenth. + +Warn-only off CI, because a developer's machine can *legitimately* be writing to +these directories while the suite runs — another agent session indexing a repo, +a ``witan`` command in the next terminal — and a false failure people learn to +re-run past is worth less than no check at all. A runner has no concurrent +writer and no reason for these directories to gain anything, so there it is +strict. +""" + + +def _strict() -> bool: + override = os.environ.get(STRICT_ENV_VAR) + if override is not None: + return override == "1" + return bool(os.environ.get("CI")) + + +_WATCHED = ( + REAL_HOME / ".local" / "share" / "witan", + REAL_HOME / ".config" / "witan", + REAL_HOME / ".claude", + REAL_HOME / ".pi", +) + + +def _entries(directory: Path) -> set[str]: + """Top-level names under ``directory``, or empty if it does not exist.""" + try: + return {p.name for p in directory.iterdir()} + except OSError: + return set() + + +_BEFORE = {d: _entries(d) for d in _WATCHED} + + +def pytest_sessionfinish(session, exitstatus): # noqa: ARG001 — pytest hook + """Report anything the suite added to the real home.""" + leaked = { + directory: sorted(_entries(directory) - before) + for directory, before in _BEFORE.items() + if _entries(directory) - before + } + if not leaked: + return + + lines = [ + "", + "TEST ENVIRONMENT LEAK — the suite wrote to the real home directory.", + "", + " Something resolved a real path instead of the redirected one. See", + " testsupport/hermetic.py; the usual cause is a module-level default", + " captured before the redirection, or a package missing its rootdir", + " conftest.py.", + "", + ] + for directory, names in leaked.items(): + lines.append(f" {directory}") + lines.extend(f" + {name}" for name in names) + lines.append("") + + if _strict(): + lines.append(" Failing the run.") + print("\n".join(lines)) + session.exitstatus = 1 + return + lines.append( + f" Warning only, off CI. Set {STRICT_ENV_VAR}=1 to make this fail here too." + ) + print("\n".join(lines)) From 8b668991668e81e58d2af7fc9ed5100cb5cc1dad Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Tue, 25 Aug 2026 10:58:57 -0400 Subject: [PATCH 3/3] test: fix six review findings in the hermeticity guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From /code-review high on #287. All six verified against the code before acting; the first two were real regressions this PR introduced. WITAN_REQUIRE_OMNIGRAPH is no longer cleared. witan-core-tests.yml sets it to "1" on the test step so a missing omnigraph binary is a hard failure rather than a skip, and test_binary_contract.py reads it at MODULE scope — after this plugin ran. Popping it reverted the entire binary-contract suite to skipping green. That suite exists to stop itself being retired quietly, and this change retired it quietly, which is the same defect the PR is about. Proved by probe: with the var exported, the suite saw '' before, '1' after. WITAN_TEST_OMNIGRAPH_SERVER/_GRAPH likewise. They are the documented opt-in for the live-server tests, whose skipif is evaluated at import, so clearing them made those tests unreachable — you could not opt in at all. Both cases are now an explicit _EXEMPT tuple with the reasoning, asserted disjoint from _CLEARED, because the distinction is subtle: clear what the suite must not INHERIT, keep what a human or a workflow set to make the suite STRICTER. Added the selectors the list missed: WITAN_OUTPUT_FORMAT (a cyclopts env_var on both CLIs), the seven WITAN_SCAN_* vars (an ambient ENABLED=false would run the suite with the write-path scanner off), WITAN_OMNIGRAPH_HTTP, OMNIGRAPH_BEARER_TOKEN, WITAN_OPTIMIZE_INTERVAL, CLAUDE_SESSION_ID. The leak detector was blind to the leak it was written for. It compared only immediate children, and the motivating defect lands at ~/.local/share/witan/code/.omni — inside a `code` directory that already exists on any machine that has run the indexer. It fired on CI, where the tree is absent, and nowhere else. My own verification missed this because the sentinel I wrote was a top-level child: a leak of a shape the real one does not have. Now a bounded-depth snapshot (3 levels) keyed on absolute path, with size+mtime markers so the #282 shape — an APPEND to an existing merge-watermarks.json — is visible at all, which no name-based check can see. Strictness no longer reads CI=false or CI=0 as true, matching the parsing test_binary_contract.py already uses. Dropped pytest_plugins for a direct import. That setting is only honoured in whichever conftest is top-level for the chosen rootdir, so running pytest from the repo root turned all five into non-top-level and aborted collection outright. Importing the module has the same effect with no such rule. Bare root-level pytest still fails on 46 pre-existing collection errors — one package's venv cannot satisfy five — identically to main at deedb68, so this restores parity rather than fixing something that worked. Deepening the watch surfaced ambient churn: four of five suites warned, every line of it the machine rather than the suite. Measured with a 95s idle probe and a parallel test-all. Modifications of those paths are now filtered; creations never are, so on CI — where none of them exist — a real write still reports. 2399 tests pass, no warnings. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RH7kCwSp8TbKQLZ7kbVnY1 --- mcp/servers/witan-code/conftest.py | 15 +- mcp/servers/witan/conftest.py | 15 +- packages/agent-config-kit/conftest.py | 15 +- packages/agent-kit/conftest.py | 15 +- packages/witan-core/conftest.py | 15 +- .../witan-core/tests/test_hermetic_guard.py | 110 ++++++++-- testsupport/hermetic.py | 197 +++++++++++++++--- 7 files changed, 320 insertions(+), 62 deletions(-) diff --git a/mcp/servers/witan-code/conftest.py b/mcp/servers/witan-code/conftest.py index 30e4e017..d82fc196 100644 --- a/mcp/servers/witan-code/conftest.py +++ b/mcp/servers/witan-code/conftest.py @@ -5,10 +5,15 @@ terminal width — at import time rather than in a fixture, because the leak it exists to stop can happen while pytest is still collecting (importing ``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest -hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. +hook that runs for every invocation. -``pytest_plugins`` is only honoured in a rootdir conftest, which is the other -reason this is here and not in ``tests/conftest.py``. +★ IMPORTED, not named in ``pytest_plugins``. That setting is only honoured in +whichever conftest is TOP-LEVEL for the rootdir pytest picked, and the rootdir +depends on the arguments: run ``pytest`` from the repo root and all five of +these become non-top-level, aborting collection outright with "Defining +'pytest_plugins' in a non-top-level conftest is no longer supported". A plain +import carries no such rule, does the same redirection (it happens at module +import), and re-exporting the hook below makes this conftest its own plugin. """ import sys @@ -16,4 +21,6 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[3])) -pytest_plugins = ["testsupport.hermetic"] +# The import is the point: `testsupport.hermetic` redirects the environment at +# module scope. The hook re-export is what lets this conftest report a leak. +from testsupport.hermetic import pytest_sessionfinish # noqa: E402,F401 diff --git a/mcp/servers/witan/conftest.py b/mcp/servers/witan/conftest.py index 30e4e017..d82fc196 100644 --- a/mcp/servers/witan/conftest.py +++ b/mcp/servers/witan/conftest.py @@ -5,10 +5,15 @@ terminal width — at import time rather than in a fixture, because the leak it exists to stop can happen while pytest is still collecting (importing ``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest -hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. +hook that runs for every invocation. -``pytest_plugins`` is only honoured in a rootdir conftest, which is the other -reason this is here and not in ``tests/conftest.py``. +★ IMPORTED, not named in ``pytest_plugins``. That setting is only honoured in +whichever conftest is TOP-LEVEL for the rootdir pytest picked, and the rootdir +depends on the arguments: run ``pytest`` from the repo root and all five of +these become non-top-level, aborting collection outright with "Defining +'pytest_plugins' in a non-top-level conftest is no longer supported". A plain +import carries no such rule, does the same redirection (it happens at module +import), and re-exporting the hook below makes this conftest its own plugin. """ import sys @@ -16,4 +21,6 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[3])) -pytest_plugins = ["testsupport.hermetic"] +# The import is the point: `testsupport.hermetic` redirects the environment at +# module scope. The hook re-export is what lets this conftest report a leak. +from testsupport.hermetic import pytest_sessionfinish # noqa: E402,F401 diff --git a/packages/agent-config-kit/conftest.py b/packages/agent-config-kit/conftest.py index 7768b42d..3953e494 100644 --- a/packages/agent-config-kit/conftest.py +++ b/packages/agent-config-kit/conftest.py @@ -5,10 +5,15 @@ terminal width — at import time rather than in a fixture, because the leak it exists to stop can happen while pytest is still collecting (importing ``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest -hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. +hook that runs for every invocation. -``pytest_plugins`` is only honoured in a rootdir conftest, which is the other -reason this is here and not in ``tests/conftest.py``. +★ IMPORTED, not named in ``pytest_plugins``. That setting is only honoured in +whichever conftest is TOP-LEVEL for the rootdir pytest picked, and the rootdir +depends on the arguments: run ``pytest`` from the repo root and all five of +these become non-top-level, aborting collection outright with "Defining +'pytest_plugins' in a non-top-level conftest is no longer supported". A plain +import carries no such rule, does the same redirection (it happens at module +import), and re-exporting the hook below makes this conftest its own plugin. """ import sys @@ -16,4 +21,6 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[2])) -pytest_plugins = ["testsupport.hermetic"] +# The import is the point: `testsupport.hermetic` redirects the environment at +# module scope. The hook re-export is what lets this conftest report a leak. +from testsupport.hermetic import pytest_sessionfinish # noqa: E402,F401 diff --git a/packages/agent-kit/conftest.py b/packages/agent-kit/conftest.py index 7768b42d..3953e494 100644 --- a/packages/agent-kit/conftest.py +++ b/packages/agent-kit/conftest.py @@ -5,10 +5,15 @@ terminal width — at import time rather than in a fixture, because the leak it exists to stop can happen while pytest is still collecting (importing ``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest -hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. +hook that runs for every invocation. -``pytest_plugins`` is only honoured in a rootdir conftest, which is the other -reason this is here and not in ``tests/conftest.py``. +★ IMPORTED, not named in ``pytest_plugins``. That setting is only honoured in +whichever conftest is TOP-LEVEL for the rootdir pytest picked, and the rootdir +depends on the arguments: run ``pytest`` from the repo root and all five of +these become non-top-level, aborting collection outright with "Defining +'pytest_plugins' in a non-top-level conftest is no longer supported". A plain +import carries no such rule, does the same redirection (it happens at module +import), and re-exporting the hook below makes this conftest its own plugin. """ import sys @@ -16,4 +21,6 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[2])) -pytest_plugins = ["testsupport.hermetic"] +# The import is the point: `testsupport.hermetic` redirects the environment at +# module scope. The hook re-export is what lets this conftest report a leak. +from testsupport.hermetic import pytest_sessionfinish # noqa: E402,F401 diff --git a/packages/witan-core/conftest.py b/packages/witan-core/conftest.py index 7768b42d..3953e494 100644 --- a/packages/witan-core/conftest.py +++ b/packages/witan-core/conftest.py @@ -5,10 +5,15 @@ terminal width — at import time rather than in a fixture, because the leak it exists to stop can happen while pytest is still collecting (importing ``witan.server`` creates a graph). A rootdir ``conftest.py`` is the earliest -hook that runs for every invocation, `just test-*` or a bare ``pytest`` alike. +hook that runs for every invocation. -``pytest_plugins`` is only honoured in a rootdir conftest, which is the other -reason this is here and not in ``tests/conftest.py``. +★ IMPORTED, not named in ``pytest_plugins``. That setting is only honoured in +whichever conftest is TOP-LEVEL for the rootdir pytest picked, and the rootdir +depends on the arguments: run ``pytest`` from the repo root and all five of +these become non-top-level, aborting collection outright with "Defining +'pytest_plugins' in a non-top-level conftest is no longer supported". A plain +import carries no such rule, does the same redirection (it happens at module +import), and re-exporting the hook below makes this conftest its own plugin. """ import sys @@ -16,4 +21,6 @@ sys.path.insert(0, str(Path(__file__).resolve().parents[2])) -pytest_plugins = ["testsupport.hermetic"] +# The import is the point: `testsupport.hermetic` redirects the environment at +# module scope. The hook re-export is what lets this conftest report a leak. +from testsupport.hermetic import pytest_sessionfinish # noqa: E402,F401 diff --git a/packages/witan-core/tests/test_hermetic_guard.py b/packages/witan-core/tests/test_hermetic_guard.py index f519bd05..1d51c520 100644 --- a/packages/witan-core/tests/test_hermetic_guard.py +++ b/packages/witan-core/tests/test_hermetic_guard.py @@ -63,27 +63,103 @@ def test_the_real_local_bin_stays_on_path(): assert str(real_bin) in os.environ["PATH"] -def test_entries_is_empty_for_a_directory_that_is_not_there(): - """The watched directories are optional — a fresh machine has none.""" - assert hermetic._entries(hermetic.FAKE_HOME / "nope" / "still-nope") == set() +def test_marker_reports_a_missing_path_rather_than_raising(): + """The watched paths are optional — a fresh machine has none of them.""" + assert hermetic._marker(hermetic.FAKE_HOME / "nope" / "still-nope") == "" -def test_entries_reports_top_level_names_only(tmp_path): - (tmp_path / "a.omni").mkdir() - (tmp_path / "a.omni" / "buried").write_text("x") - (tmp_path / "b.json").write_text("x") +def test_marker_changes_when_a_file_changes(tmp_path): + """The agent-kit#282 leak APPENDED to a file that already existed. A + name-based check cannot see that however deep it walks, so the marker + carries size and mtime.""" + target = tmp_path / "merge-watermarks.json" + target.write_text('{"a": 1}') + before = hermetic._marker(target) - assert hermetic._entries(tmp_path) == {"a.omni", "b.json"} + target.write_text('{"a": 1, "leaked": 2}') + assert hermetic._marker(target) != before -def test_a_new_entry_is_what_counts_as_a_leak(tmp_path): - """Growth, not difference: a suite that DELETES something from the real - home is a different (and louder) problem, and pre-existing entries must not - register — a developer's machine is full of them.""" - (tmp_path / "pre-existing").write_text("x") - before = hermetic._entries(tmp_path) - assert hermetic._entries(tmp_path) - before == set() +def test_marker_distinguishes_a_directory(): + assert hermetic._marker(hermetic.FAKE_HOME) == "dir" - (tmp_path / "leaked").write_text("x") - assert hermetic._entries(tmp_path) - before == {"leaked"} + +def test_the_watch_reaches_deep_enough_for_a_nested_store(): + """The regression the shallow check had. + + The leak this whole change exists to stop lands at + ``~/.local/share/witan/code/.omni`` — below a ``code`` directory that + already exists on any machine that has run the indexer. Comparing immediate + children saw no new name and reported nothing, on exactly the machines + where the leak was real. + """ + watched = dict(hermetic._WATCHED_TREES) + store_root = hermetic.REAL_HOME / ".local" / "share" / "witan" + + assert watched[store_root] >= 3, ( + "depth must reach witan/code/.omni/, or the guard is " + "blind to the defect it was written for" + ) + + +def test_the_watched_files_include_the_282_watermark(): + names = {path.name for path in hermetic._WATCHED_FILES} + assert "merge-watermarks.json" in names + + +def test_an_exempt_selector_is_never_also_cleared(): + """Clearing WITAN_REQUIRE_OMNIGRAPH silently retired the binary-contract + suite, whose entire purpose is to not be retired silently. The module + asserts this at import; this states it as a test so the reason is findable. + """ + assert not set(hermetic._CLEARED) & set(hermetic._EXEMPT) + + +def test_the_ci_switch_treats_an_explicit_negative_as_off(monkeypatch): + """`CI=false` and `CI=0` are both set by real tooling.""" + monkeypatch.delenv(hermetic.STRICT_ENV_VAR, raising=False) + for value, expected in ( + ("true", True), + ("1", True), + ("false", False), + ("0", False), + ("", False), + ): + monkeypatch.setenv("CI", value) + assert hermetic._strict() is expected, value + + +def test_the_override_beats_the_inferred_default(monkeypatch): + monkeypatch.setenv("CI", "true") + monkeypatch.setenv(hermetic.STRICT_ENV_VAR, "0") + assert hermetic._strict() is False + + monkeypatch.delenv("CI", raising=False) + monkeypatch.setenv(hermetic.STRICT_ENV_VAR, "1") + assert hermetic._strict() is True + + +def test_ambient_churn_is_filtered_but_only_for_modifications(): + """A working machine rewrites these on its own schedule. + + Measured rather than guessed: a 95-second idle probe with no tests running + reported a store's `.repo` sidecar, and a parallel `just test-all` added + the token cache and Claude Code's own config. Four of five suites warned, + every line of it ambient — and a check that cries wolf gets re-run past. + """ + assert hermetic._is_ambient("/home/x/.claude.json") + assert hermetic._is_ambient("/home/x/.config/witan/tokens.json") + assert hermetic._is_ambient("/home/x/.local/share/witan/code/foo.omni.repo") + assert hermetic._is_ambient("/home/x/.local/share/witan/graph.omni.lock") + + +def test_the_282_watermark_is_not_treated_as_ambient(): + """The append-to-an-existing-file leak has to stay visible: it only + changes when someone actually runs `witan migrate merge`.""" + assert not hermetic._is_ambient("/home/x/.config/witan/merge-watermarks.json") + + +def test_a_store_directory_itself_is_not_ambient(): + """The sidecar churns; the store landing there is the leak.""" + assert not hermetic._is_ambient("/home/x/.local/share/witan/code/foo.omni") diff --git a/testsupport/hermetic.py b/testsupport/hermetic.py index 0641d9fe..ffcdec2a 100644 --- a/testsupport/hermetic.py +++ b/testsupport/hermetic.py @@ -90,10 +90,27 @@ "WITAN_MODEL", "WITAN_AUTHOR", "WITAN_CONTEXT_TTL", - "WITAN_REQUIRE_OMNIGRAPH", - "WITAN_TEST_OMNIGRAPH_SERVER", - "WITAN_TEST_OMNIGRAPH_GRAPH", + "WITAN_OPTIMIZE_INTERVAL", "AC_KIT_CONFIG", + # Rendering and transport selectors. WITAN_OUTPUT_FORMAT is a cyclopts + # `env_var` on both CLIs, so an ambient `json` turns every table command's + # output into something no assertion here expects. + "WITAN_OUTPUT_FORMAT", + "WITAN_OMNIGRAPH_HTTP", + "OMNIGRAPH_BEARER_TOKEN", + # The write-path scanner. `WITAN_SCAN_ENABLED=false` is a documented + # opt-out, and inheriting it would run the whole suite with the scanner + # off — green, and testing something other than what ships. + "WITAN_SCAN_ENABLED", + "WITAN_SCAN_SECRET_ACTION", + "WITAN_SCAN_PII_ACTION", + "WITAN_SCAN_ENABLED_DETECTORS", + "WITAN_SCAN_DISABLED_DETECTORS", + "WITAN_SCAN_PLUGINS", + "WITAN_SCAN_ALLOWLIST", + # Identity of the calling agent session, which several code paths record + # as provenance. + "CLAUDE_SESSION_ID", # Observability: an exporter endpoint set on a developer's box would have # the suite emit spans at a real collector. "OTEL_EXPORTER_OTLP_ENDPOINT", @@ -103,6 +120,39 @@ ) +# ★ DELIBERATELY NOT CLEARED, and re-adding any of these breaks a check +# silently — which is precisely the class of defect this module exists to end. +# +# WITAN_REQUIRE_OMNIGRAPH .github/workflows/witan-core-tests.yml sets +# this to "1" on the test step so that a missing +# omnigraph binary is a HARD FAILURE instead of a +# skip. test_binary_contract.py reads it at +# MODULE SCOPE, which runs after this plugin, so +# popping it silently reverted the entire binary +# contract suite to skipping green. That suite's +# whole purpose is to stop itself being retired +# quietly; clearing this retired it quietly. +# +# WITAN_TEST_OMNIGRAPH_SERVER The documented opt-in for the live-server +# WITAN_TEST_OMNIGRAPH_GRAPH tests (`WITAN_TEST_OMNIGRAPH_SERVER= +# pytest -k live_server`). Their `skipif` is +# evaluated at import, after this runs, so +# clearing them made those tests permanently +# unreachable — you could not opt in at all. +# +# The distinction is whether the variable selects BEHAVIOUR THE SUITE SHOULD NOT +# INHERIT (clear it) or ENABLES A TEST MODE THE CALLER ASKED FOR (keep it). An +# ambient value that changes what the code under test does is contamination; an +# ambient value that a human or a workflow set to make the suite stricter is an +# instruction. +_EXEMPT = ( + "WITAN_REQUIRE_OMNIGRAPH", + "WITAN_TEST_OMNIGRAPH_SERVER", + "WITAN_TEST_OMNIGRAPH_GRAPH", +) +assert not set(_CLEARED) & set(_EXEMPT), "an exempt selector must not also be cleared" + + def _redirect() -> None: home = FAKE_HOME (home / ".local" / "bin").mkdir(parents=True, exist_ok=True) @@ -210,40 +260,138 @@ def _redirect() -> None: """ +def _falsey(value: str) -> bool: + """Treat an explicitly-negative string as unset. + + ``CI=false`` and ``CI=0`` are both set by real tooling, and a bare + truthiness check reads them as "yes, strict". Same parsing as + ``test_binary_contract.py`` uses for ``WITAN_REQUIRE_OMNIGRAPH``. + """ + return value.strip().lower() in ("", "0", "false", "no", "off") + + def _strict() -> bool: override = os.environ.get(STRICT_ENV_VAR) if override is not None: - return override == "1" - return bool(os.environ.get("CI")) + return not _falsey(override) + return not _falsey(os.environ.get("CI", "")) -_WATCHED = ( - REAL_HOME / ".local" / "share" / "witan", - REAL_HOME / ".config" / "witan", - REAL_HOME / ".claude", - REAL_HOME / ".pi", +# What to watch, and how deep. Depth matters more than it looks: the leak this +# whole change exists to stop lands at ``~/.local/share/witan/code/.omni`` +# — INSIDE a ``code`` directory that already exists on any machine that has run +# the indexer. Comparing only immediate children of ``~/.local/share/witan`` +# therefore saw no new name and reported nothing, on exactly the machines where +# the leak was real. It only ever fired on CI, where the whole tree is absent so +# ``code`` itself counts as new. +# +# Depth 3 covers store creation under ``code/`` and writes landing in an +# existing store's ``nodes``/``edges``/``__manifest`` directories. +# +# ★ A depth is a bound, not a proof. A write buried deeper than this goes +# unreported, and the check says so rather than implying it is exhaustive. +_WATCHED_TREES = ( + (REAL_HOME / ".local" / "share" / "witan", 3), + (REAL_HOME / ".config" / "witan", 2), +) + +# Watched as FILES, by size and mtime, because the agent-kit#282 leak was an +# APPEND to a file that already existed — a new-name check cannot see that, no +# matter how deep it walks. +_WATCHED_FILES = ( + REAL_HOME / ".config" / "witan" / "merge-watermarks.json", + REAL_HOME / ".claude.json", + REAL_HOME / ".claude" / "settings.json", + REAL_HOME / ".pi" / "agent" / "mcp.json", +) + + +# Paths a working machine rewrites on its own schedule. Measured, not guessed: +# a 95-second idle probe (no tests running at all) and a parallel `just +# test-all` between them reported exactly these — the indexer touching a +# store's `.repo` sidecar, the OIDC client refreshing its token, Claude Code +# persisting its own config. +# +# Only their MODIFICATION is ignored, never their creation. On CI none of these +# exist, so a test that writes one still shows up as a `created` and is caught; +# locally they pre-exist, so the churn is a modification and is filtered. That +# keeps the agent-kit#282 shape — an append to an existing +# `merge-watermarks.json`, which is NOT on this list — visible where it happens. +# +# ★ Four of five suites warned before this existed, every line of it ambient. +# A check that cries wolf gets re-run past, which this file's own docstring says +# is worth less than no check; the filter is what makes the warning mean +# something off CI. +_AMBIENT_CHURN = ( + ".claude.json", # Claude Code's own state, rewritten constantly + "tokens.json", # refreshed by any witan client, incl. another session + "tokens.json.lock", +) +_AMBIENT_SUFFIXES = ( + ".omni.repo", # per-store sidecar, touched on any index + ".lock", + ".schema_mtime", ) -def _entries(directory: Path) -> set[str]: - """Top-level names under ``directory``, or empty if it does not exist.""" +def _is_ambient(path_str: str) -> bool: + """Whether a MODIFICATION of this path is the machine rather than the suite.""" + name = path_str.rsplit("/", 1)[-1] + return name in _AMBIENT_CHURN or path_str.endswith(_AMBIENT_SUFFIXES) + + +def _marker(path: Path) -> str: + """A value that changes when ``path`` does. ``dir`` for directories. + + Size and mtime rather than a hash: these trees hold hundreds of megabytes + of Lance data, and the check runs twice per session. + """ try: - return {p.name for p in directory.iterdir()} + st = path.stat() except OSError: - return set() + return "" + if path.is_dir(): + return "dir" + return f"{st.st_size}:{st.st_mtime_ns}" + +def _snapshot() -> dict[str, str]: + """Relative path -> marker, for everything currently watched.""" + seen: dict[str, str] = {} -_BEFORE = {d: _entries(d) for d in _WATCHED} + def walk(directory: Path, root: Path, depth: int) -> None: + if depth < 0: + return + try: + children = sorted(directory.iterdir()) + except OSError: + return + for child in children: + # Absolute paths as keys, so a path reached by both the tree walk + # and the explicit file list is one entry rather than two. + seen[str(child)] = _marker(child) + if child.is_dir(): + walk(child, root, depth - 1) + + for root, depth in _WATCHED_TREES: + walk(root, root, depth - 1) + for path in _WATCHED_FILES: + if path.exists(): + seen[str(path)] = _marker(path) + return seen + + +_BEFORE = _snapshot() def pytest_sessionfinish(session, exitstatus): # noqa: ARG001 — pytest hook - """Report anything the suite added to the real home.""" - leaked = { - directory: sorted(_entries(directory) - before) - for directory, before in _BEFORE.items() - if _entries(directory) - before - } - if not leaked: + """Report anything the suite added to, or changed in, the real home.""" + now = _snapshot() + added = sorted(k for k in now if k not in _BEFORE) + changed = sorted( + k for k in now if k in _BEFORE and now[k] != _BEFORE[k] and not _is_ambient(k) + ) + if not added and not changed: return lines = [ @@ -256,9 +404,8 @@ def pytest_sessionfinish(session, exitstatus): # noqa: ARG001 — pytest hook " conftest.py.", "", ] - for directory, names in leaked.items(): - lines.append(f" {directory}") - lines.extend(f" + {name}" for name in names) + lines.extend(f" created {name}" for name in added) + lines.extend(f" modified {name}" for name in changed) lines.append("") if _strict():