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/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/conftest.py b/mcp/servers/witan-code/conftest.py new file mode 100644 index 00000000..d82fc196 --- /dev/null +++ b/mcp/servers/witan-code/conftest.py @@ -0,0 +1,26 @@ +"""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. + +★ 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 +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[3])) + +# 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-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: diff --git a/mcp/servers/witan/conftest.py b/mcp/servers/witan/conftest.py new file mode 100644 index 00000000..d82fc196 --- /dev/null +++ b/mcp/servers/witan/conftest.py @@ -0,0 +1,26 @@ +"""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. + +★ 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 +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[3])) + +# 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 new file mode 100644 index 00000000..3953e494 --- /dev/null +++ b/packages/agent-config-kit/conftest.py @@ -0,0 +1,26 @@ +"""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. + +★ 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 +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +# 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 new file mode 100644 index 00000000..3953e494 --- /dev/null +++ b/packages/agent-kit/conftest.py @@ -0,0 +1,26 @@ +"""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. + +★ 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 +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +# 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 new file mode 100644 index 00000000..3953e494 --- /dev/null +++ b/packages/witan-core/conftest.py @@ -0,0 +1,26 @@ +"""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. + +★ 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 +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +# 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 new file mode 100644 index 00000000..1d51c520 --- /dev/null +++ b/packages/witan-core/tests/test_hermetic_guard.py @@ -0,0 +1,165 @@ +"""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_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_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) + + target.write_text('{"a": 1, "leaked": 2}') + + assert hermetic._marker(target) != before + + +def test_marker_distinguishes_a_directory(): + assert hermetic._marker(hermetic.FAKE_HOME) == "dir" + + +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/__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..ffcdec2a --- /dev/null +++ b/testsupport/hermetic.py @@ -0,0 +1,419 @@ +"""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_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", + "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT", + "OTEL_EXPORTER_OTLP_METRICS_ENDPOINT", + "SENTRY_DSN", +) + + +# ★ 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) + + 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 _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 not _falsey(override) + return not _falsey(os.environ.get("CI", "")) + + +# 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 _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: + st = path.stat() + except OSError: + 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] = {} + + 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, 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 = [ + "", + "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.", + "", + ] + lines.extend(f" created {name}" for name in added) + lines.extend(f" modified {name}" for name in changed) + 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))