fix(witan-code): stop four tests asserting the machine's logged-out state - #285
Merged
Conversation
…tate `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RH7kCwSp8TbKQLZ7kbVnY1
Contributor
There was a problem hiding this comment.
Pull request overview
Makes witan-code identity-dependent tests deterministic and removes environment-dependent write authorization.
Changes:
- Isolates tests from real Witan configuration and token caches.
- Requires callers to pass
actorexplicitly tocheck_writable. - Adds logged-in identity regression coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
witan_code/graph.py |
Removes implicit actor resolution. |
tests/conftest.py |
Isolates identity state and adds a login fixture. |
tests/test_graph.py |
Tests explicit logged-out authorization. |
tests/test_branches.py |
Tests actor-namespaced branch indexing. |
CHANGELOG.md |
Documents fixes and API change. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
blarghmatey
added a commit
that referenced
this pull request
Aug 25, 2026
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RH7kCwSp8TbKQLZ7kbVnY1
blarghmatey
added a commit
that referenced
this pull request
Aug 25, 2026
* fix(witan-code): stop four tests asserting the machine's logged-out state `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RH7kCwSp8TbKQLZ7kbVnY1 * test: stop every suite reading and writing the machine it runs on 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RH7kCwSp8TbKQLZ7kbVnY1 * test: fix six review findings in the hermeticity guard 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 '<absent>' 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/<slug>.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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RH7kCwSp8TbKQLZ7kbVnY1 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What are the relevant tickets?
tk-four-witan-code-tests-fail-on-any-machine-with-a-298214Same defect class as #282, different package and different mechanism: that one had tests writing to the developer's real state, this one has tests reading it.
Description (What does it do?)
Four witan-code tests fail on any machine that has run
witan login, and pass in CI:identity.actor_id()resolves from the real~/.config/witan/config.tomland the real OIDC token cache, and nothing in the suite stopped it. With an identity resolved, branch views get namespaced under it (act-alice/feature_new-api) andcheck_writabletakes its logged-in branch — while those four tests assert the un-namespaced names and the logged-out refusal prose. A GitHub runner has no witan identity, so CI only ever exercised the half they describe. They are not asserting a behaviour; they are asserting an environment, silently.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 these were pre-existing.
The fix is two parts.
Isolate the resolution inputs.
_fresh_identity(already autouse, already resetting the memo) now also pointsWITAN_CONFIGandWITAN_TOKEN_CACHEattmp_pathand clears the four env vars that short-circuit ahead of them. Logged-out becomes the deterministic default everywhere instead of an accident of who ran it. This is whattest_identity.py's ownconfigfixture already did for itself, which is why those tests never had the problem.Drop
check_writable'sactordefault. It fell back toidentity.actor_id(), which is what let the guard's behaviour depend on the caller's environment. Every call site (indexer.py:394,bridge.py:113,ingest.py:261) already resolves an actor, because it needs one to name the view it is about to write, so nothing relied on the fallback. Foringestit was actively wrong: a request arriving with no actor was judged against the serving process's identity rather than being refused for having none.actoris now required, andNonemeans one thing.Then two tests for the half CI cannot reach:
test_no_actor_means_logged_out_even_when_the_machine_has_one— the regression test for this exact bug, via a newlogged_in_actorfixture.test_a_logged_in_writer_owns_the_views_it_indexes— a logged-in writer's views carry their owner, on the local store too, and the un-namespaced name is not created.logged_in_actorsetsWITAN_ACTORrather than stubbingactor_id, so the resolution path a non-interactive writer (CI indexer, maintenance job) actually takes is the one under test.How can this be tested?
On a machine with a witan identity, at the base commit (3c72b93):
4 failed, 523 passed. On this branch:529 passed— the four fixed, plus the two new tests. On a logged-out machine both are green, which is the point.To see the old failure yourself, check out the base commit on a machine where
witan coderesolves an identity. That is the whole defect.just test-allalso passes, except two failures inpackages/agent-config-kit/tests/test_cli.py(test_manifest_init_exits_2_on_invalid_derived_name,..._over_64_chars). Unrelated to this change and pre-existing — this branch touches only witan-code, andjust test-agent-config-kitfails identically on the base commit. Same defect class again in a third package: Rich wraps the message at terminal width, so the asserted substring is split by a newline. Both pass underCOLUMNS=200. Filed separately rather than fixed here.Additional Context
check_writable's signature change is source-breaking for any caller that omittedactor. There are none in this workspace; the CHANGELOG entry says so under Changed.