test(witan): stop the merge tests writing to the developer's real state - #282
Merged
Conversation
`witan migrate merge` records a per-pair watermark, and the default path is a real user file beside the token cache. Any test that drives the merge CLI without overriding `WITAN_MERGE_WATERMARKS` therefore writes to the developer's own `~/.config/witan/merge-watermarks.json`. It was doing exactly that. The file had ten entries keyed by `/tmp/pytest-of-*/pytest-NNN/.../personal.omni` from one afternoon's runs, accumulating one per run — each pytest tmp path is unique, so nothing ever replaced anything and the file grew without bound. Nothing in the suite could catch it: every test asserting on watermark contents sets the variable itself and passes, and the leak is a property of the tests that DON'T. It surfaced only because the file was opened by hand before a real merge. Fixed with an autouse fixture next to `no_real_remote`, which exists for the same class of bug. Autouse is the point: opting in is what was already missing. Verified by measurement rather than inspection — snapshot the real file, run the merge tests, compare checksums: 11 pairs / sha 405bb6bf before and after, unchanged. On the unfixed conftest that same run appends an entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BZBrdCenMsbribvmY6zVpe
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents Witan tests from modifying developers’ real merge-watermark state.
Changes:
- Adds an autouse fixture redirecting merge watermarks to a temporary file.
- Preserves explicit per-test watermark overrides.
💡 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
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
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?
No ticket. Found by hand while running a real merge against QA to verify #281 —
the watermark file was opened to check its state and turned out to be full of
test output.
Description (What does it do?)
witan migrate merge(#281) records a per-pair watermark, and the default pathis a real user file next to the token cache
(
~/.config/witan/merge-watermarks.json). Any test that drives the merge CLIwithout overriding
WITAN_MERGE_WATERMARKSwrites to the developer's own state.It was doing exactly that. Ten entries from a single afternoon:
It accumulates rather than overwrites: each pytest tmp path is unique, so the
pair key is unique, so nothing ever replaces anything and the file grows one
entry per run forever.
Nothing in the suite could have caught this. Every test that asserts on
watermark contents sets
WITAN_MERGE_WATERMARKSitself and passes; the leak isa property of the tests that don't. That is why the fix is an autouse fixture
rather than another override — opting in is precisely what was already missing.
Placed next to
no_real_remote, which exists for the same class of bug (testsreaching the developer's real config and, at one point, production).
How can this be tested?
Measured, not inspected — snapshot the real file, run the merge tests, compare:
On the unfixed conftest the same run appends an entry and the checksum moves.
Additional Context
The ten leaked entries have been cleaned out of the affected machine's file, and
only the one legitimate pair (a real merge into QA) kept. Nothing else read that
file, so there was no downstream effect beyond unbounded growth — but a merge
watermark is load-bearing for #281's divergence reporting, and a file the test
suite scribbles in is a bad place for it.
Worth noting for the next writer of user-facing state: the same trap is open for
anything else defaulting to a real path under
$HOME.no_real_remote,tmp_state_dirandno_background_optimizeare three prior instances of it inthis one conftest.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BZBrdCenMsbribvmY6zVpe