From 674024649a307283c63890bc62b30a6ddc5dfe47 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Mon, 24 Aug 2026 19:47:02 -0400 Subject: [PATCH] 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: