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: