From 0127376e06984b7e58eab8626ab774df863ca49e Mon Sep 17 00:00:00 2001 From: Cipher208 <269750686+Cipher208@users.noreply.github.com> Date: Wed, 7 Oct 2026 17:00:16 +0200 Subject: [PATCH 1/2] test(hygiene): stop the suite writing to the operator's real data dir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several modules resolve their data directory ONCE, at import time, from MCP_MEMORY_DATA_DIR: `shared/saga/impl/storage.py` (SAGA_DIR), `shared/read_only.py` (which builds the module-level `read_only_replica` singleton on import) and `features/auth/__init__.py`. Importing any of them while the real value was in place was enough to write to the live directory — no test body had to do anything wrong, and `hermetic_global_db` could not help because by the time a fixture runs those constants are already resolved. Measured on the operator's `~/.mcp-ariel-memory` before this change: | path | what the suite left behind | |---|---| | `replica/memory.db` | 14 MB, rewritten on every run by the lifespan | | `sagas/` | 15875 state files, 63 MB | | `/data/auth/keys.enc` | 704 KB, +~860 B per run | `tests/conftest.py` now publishes a session temp dir as MCP_MEMORY_DATA_DIR before pytest imports the first test module, and the session fixture removes it and restores the operator's value on teardown. After the change a full run leaves all three paths byte-identical: same mtime, same size, same file count. Two tests were relying on the variable being unset and had to stop: - `test_backup_saga_success` patched `pathlib.Path.home` to fake a data dir, which only worked while MCP_MEMORY_DATA_DIR was unset and the fallback was reachable. It now names its own dir through the variable, which is what the saga actually reads. - `test_backup_saga_compensation` passed vacuously: with no source database the first step reported `skipped_no_source`, so `backup_path` was absent and the assertion that the backup was removed never ran. It now requires the backup to have existed, and it finds one. `test_auth_backup.py` also constructed `APIKeyAuth()` / `BearerAuth()` bare, which fall back to the CWD-relative `data/auth/*.enc` — a fallback no environment variable can redirect, hence the separate fix there. Each test now passes its own `tmp_path`, and `test_api_key_list` asserts `== 2` instead of `>= 2`: it was being satisfied by the accumulated history rather than by itself. Full suite: 2087 passed. Not addressed here, and reported instead: the same scratch dir holds 1.6 GB of `tmp*` directories left by tests that call `tempfile.mkdtemp()` without cleaning up, and `data/auth/keys.enc` from earlier runs is still on disk (gitignored, nothing was ever committed). --- tests/conftest.py | 30 +++++++++++- tests/shared/test_saga_impl.py | 88 ++++++++++++++++++---------------- tests/test_auth_backup.py | 36 +++++++++----- 3 files changed, 98 insertions(+), 56 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 3da3e3da..6258ba88 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -18,6 +18,21 @@ TMP_POLICY_DIR = _install_tmp_policy() +# Several modules resolve their data directory ONCE, at import time, from this +# variable: `shared/saga/impl/storage.py` (`SAGA_DIR`), `shared/read_only.py` +# (which builds the module-level `read_only_replica` singleton on import) and +# `features/auth/__init__.py` (`DEFAULT_KEYS_FILE`, `DEFAULT_TOKEN_FILE`). +# Importing any of them while the real value is in place is enough to write to +# the operator's live memory directory — no test body has to do anything wrong. +# Measured on 2026-10-07 before this line existed: `~/.mcp-ariel-memory/replica` +# held a 14 MB database and `sagas/` 15875 state files (63 MB). +# +# This has to run here, before pytest imports the first test module, because by +# the time a fixture executes the constants above are already resolved. +_OPERATOR_DATA_DIR = os.environ.get("MCP_MEMORY_DATA_DIR") +SESSION_DATA_DIR = tempfile.mkdtemp(prefix="ariel-test-data-") +os.environ["MCP_MEMORY_DATA_DIR"] = SESSION_DATA_DIR + import pytest @@ -54,10 +69,16 @@ def hermetic_global_db(): cm (adaptive_threshold, DreamBuffer, ConsolidationEngine...) would otherwise read/write the real ~/.mcp-ariel-memory data dir. Mutating the singleton in place keeps those references valid. + + The dir is the one already published as MCP_MEMORY_DATA_DIR in this file + (see the top): the module-level constants that resolve from it are created + on import, so this fixture cannot be what points them somewhere safe — it + can only agree with what was decided there. Its own teardown below is what + removes the directory and puts the operator's value back. """ from shared.connection import connection_manager - session_dir = tempfile.mkdtemp(prefix="ariel-test-global-") + session_dir = SESSION_DATA_DIR original_dir = connection_manager.base_dir connection_manager.base_dir = Path(session_dir) connection_manager._conns.clear() # drop any already-open real-dir handles @@ -80,6 +101,13 @@ def hermetic_global_db(): pass connection_manager.base_dir = original_dir connection_manager._conns.clear() + # Put the operator's value back: this process is not the only consumer of + # the variable, and leaving a deleted path published would be worse than + # never having set it. + if _OPERATOR_DATA_DIR is None: + os.environ.pop("MCP_MEMORY_DATA_DIR", None) + else: + os.environ["MCP_MEMORY_DATA_DIR"] = _OPERATOR_DATA_DIR # This fixture is the only thing that ever learns the path, so it is the # only thing that can remove it — same rule the eval harness had to learn # the hard way (one leaked directory per run, 972 of them / 1.6G in 48h). diff --git a/tests/shared/test_saga_impl.py b/tests/shared/test_saga_impl.py index 2c1fcba7..c7f7987c 100644 --- a/tests/shared/test_saga_impl.py +++ b/tests/shared/test_saga_impl.py @@ -12,7 +12,7 @@ def _pin_master_key(monkeypatch): _secrets._master_cache.clear() -from unittest.mock import patch, AsyncMock +from unittest.mock import AsyncMock from pathlib import Path import tempfile @@ -41,53 +41,57 @@ def engine(store): @pytest.mark.asyncio -async def test_backup_saga_success(engine, temp_dir): - # Setup mock environment - with patch("pathlib.Path.home", return_value=temp_dir): - base = temp_dir / ".mcp-ariel-memory" - base.mkdir(parents=True) - db_file = base / DB_NAME - db_file.write_text("dummy database content") +async def test_backup_saga_success(engine, temp_dir, monkeypatch): + # The saga resolves its base dir from MCP_MEMORY_DATA_DIR, which the suite + # now points at its own session dir (see tests/conftest.py), so patching + # `pathlib.Path.home` no longer decides anything: the variable wins. The + # test has to say where it wants its database instead of relying on the + # variable being unset. + base = temp_dir / ".mcp-ariel-memory" + base.mkdir(parents=True) + monkeypatch.setenv("MCP_MEMORY_DATA_DIR", str(base)) + (base / DB_NAME).write_text("dummy database content") + + steps = create_backup_saga() + state = SagaState(saga_id="backup_test", name="backup", context={}) - steps = create_backup_saga() - state = SagaState(saga_id="backup_test", name="backup", context={}) - - # Execute - result = await engine.execute(state, steps) + # Execute + result = await engine.execute(state, steps) - # Verify - assert "backup_path" in result - backup_path = Path(result["backup_path"]) - assert backup_path.exists() - assert (backup_path / DB_NAME).exists() - assert state.status == SagaStatus.COMPLETED + # Verify + assert "backup_path" in result + backup_path = Path(result["backup_path"]) + assert backup_path.exists() + assert (backup_path / DB_NAME).exists() + assert state.status == SagaStatus.COMPLETED @pytest.mark.asyncio -async def test_backup_saga_compensation(engine, temp_dir): +async def test_backup_saga_compensation(engine, temp_dir, monkeypatch): # Setup mock environment where verification fails - with patch("pathlib.Path.home", return_value=temp_dir): - base = temp_dir / ".mcp-ariel-memory" - base.mkdir(parents=True) - db_file = base / DB_NAME - db_file.write_text("dummy database content") - - steps = create_backup_saga() - # Force failure in second step - steps[1].action = AsyncMock(side_effect=ValueError("verify failed")) - - state = SagaState(saga_id="backup_fail", name="backup", context={}) - - # Execute - with pytest.raises(ValueError, match="verify failed"): - await engine.execute(state, steps) - - # Verify compensation (backup dir removed) - assert state.status == SagaStatus.COMPENSATED - # We need to find the backup dir from context - backup_path_str = state.context.get("backup_path") - if backup_path_str: - assert not Path(backup_path_str).exists() + base = temp_dir / ".mcp-ariel-memory" + base.mkdir(parents=True) + monkeypatch.setenv("MCP_MEMORY_DATA_DIR", str(base)) + (base / DB_NAME).write_text("dummy database content") + + steps = create_backup_saga() + # Force failure in second step + steps[1].action = AsyncMock(side_effect=ValueError("verify failed")) + + state = SagaState(saga_id="backup_fail", name="backup", context={}) + + # Execute + with pytest.raises(ValueError, match="verify failed"): + await engine.execute(state, steps) + + # Verify compensation (backup dir removed) + assert state.status == SagaStatus.COMPENSATED + # The backup has to have been made for its removal to mean anything; before + # this test named its own data dir the source was missing, the first step + # reported `skipped_no_source`, and this assertion never ran. + backup_path_str = state.context.get("backup_path") + assert backup_path_str, "the backup step must have produced a directory to compensate" + assert not Path(backup_path_str).exists() @pytest.mark.asyncio diff --git a/tests/test_auth_backup.py b/tests/test_auth_backup.py index 9b96f8aa..831af630 100644 --- a/tests/test_auth_backup.py +++ b/tests/test_auth_backup.py @@ -1,25 +1,35 @@ """ Tests for auth — unique tests only. + +Each test names its own store path. Constructed bare, `APIKeyAuth()` and +`BearerAuth()` fall back to the CWD-relative `data/auth/*.enc`, so these tests +used to append to `/data/auth/keys.enc` on every run — 704 KB of +accumulated `alice` keys by 2026-10-07, and growing by ~860 B per run. It is +gitignored (`*.enc`), so nothing was ever committed, but `test_api_key_list` +asserting `len(keys) >= 2` was satisfied by that history rather than by the +test. The suite-wide fix for the modules that resolve their directory at import +time is in `tests/conftest.py`; this file is fixed here because the fallback it +was hitting is relative, and no environment variable can redirect that. """ import pytest @pytest.mark.asyncio -async def test_api_key_create(): +async def test_api_key_create(tmp_path): from features.auth import APIKeyAuth - auth = APIKeyAuth() + auth = APIKeyAuth(keys_file=tmp_path / "keys.enc") key = auth.create_key("alice", "test key") assert key.startswith("ak_") assert len(key) > 20 @pytest.mark.asyncio -async def test_api_key_verify(): +async def test_api_key_verify(tmp_path): from features.auth import APIKeyAuth - auth = APIKeyAuth() + auth = APIKeyAuth(keys_file=tmp_path / "keys.enc") key = auth.create_key("alice", "test key") info = auth.verify(key) assert info is not None @@ -28,10 +38,10 @@ async def test_api_key_verify(): @pytest.mark.asyncio -async def test_api_key_revoke(): +async def test_api_key_revoke(tmp_path): from features.auth import APIKeyAuth - auth = APIKeyAuth() + auth = APIKeyAuth(keys_file=tmp_path / "keys.enc") key = auth.create_key("alice", "test key") assert auth.verify(key) is not None revoked = auth.revoke(key) @@ -40,21 +50,21 @@ async def test_api_key_revoke(): @pytest.mark.asyncio -async def test_api_key_list(): +async def test_api_key_list(tmp_path): from features.auth import APIKeyAuth - auth = APIKeyAuth() + auth = APIKeyAuth(keys_file=tmp_path / "keys.enc") auth.create_key("alice", "key1") auth.create_key("alice", "key2") keys = auth.list_keys() - assert len(keys) >= 2 + assert len(keys) == 2 @pytest.mark.asyncio -async def test_bearer_auth(): +async def test_bearer_auth(tmp_path): from features.auth import BearerAuth - ba = BearerAuth() + ba = BearerAuth(token_file=tmp_path / "token.enc") token = ba.get_token() assert token.startswith("mt_") assert ba.verify("Bearer " + token) is True @@ -63,10 +73,10 @@ async def test_bearer_auth(): @pytest.mark.asyncio -async def test_bearer_rotate(): +async def test_bearer_rotate(tmp_path): from features.auth import BearerAuth - ba = BearerAuth() + ba = BearerAuth(token_file=tmp_path / "token.enc") old_token = ba.get_token() new_token = ba.rotate() assert old_token != new_token From b0c57648dd280f2f5d8ce5fde1fa37065121caee Mon Sep 17 00:00:00 2001 From: Cipher208 <269750686+Cipher208@users.noreply.github.com> Date: Wed, 7 Oct 2026 17:18:23 +0200 Subject: [PATCH 2/2] chore: re-trigger CodeQL after a failed SARIF upload