From 11ffe0f9ddd473146d6b442fc32296bc885abbc7 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Thu, 9 Jul 2026 14:16:26 -0400 Subject: [PATCH] fix: isolate review-mutation ledger from host session-state (#590) Tests that call patch.dict(os.environ, clear=True) dropped GITEA_MCP_SESSION_STATE_DIR and re-adopted the operator host cache under ~/.cache/gitea-tools/session-state. A residual terminal request_changes mutation then tripped the #332 hard-stop before test_unknown_profile_blocks could assert the empty-profile gate. - Pin mcp_session_state.default_state_dir / DEFAULT_STATE_DIR to a per-test temp dir in conftest (still honors an explicit env override) - Clear the decision lock at the start of each TestMergePR test - Add regression coverage for env-clear isolation Closes #590 --- tests/conftest.py | 11 +++-- tests/test_mcp_server.py | 58 +++++++++++++++++++++++++ tests/test_session_state_isolation.py | 62 +++++++++++++++++++++++++++ 3 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 tests/test_session_state_isolation.py diff --git a/tests/conftest.py b/tests/conftest.py index 1873223..e14d42a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,9 +20,11 @@ def _reset_mutation_authority(monkeypatch): Session-state isolation (#559 / #590 / #594): many tests use ``patch.dict(os.environ, ..., clear=True)``, which drops - ``GITEA_MCP_SESSION_STATE_DIR``. Pin ``default_state_dir`` / - ``DEFAULT_STATE_DIR`` to a per-test temp dir so durable load/save never - touches host state even after env clears. + ``GITEA_MCP_SESSION_STATE_DIR``. Relying only on the env var therefore + re-exposes the operator host cache + (``~/.cache/gitea-tools/session-state``) and its review-mutation ledger. + Pin ``default_state_dir`` / ``DEFAULT_STATE_DIR`` to a per-test temp dir + so durable load/save never touches host state even after env clears. """ monkeypatch.delenv("GITEA_SESSION_PROFILE_LOCK", raising=False) # Isolate durable session-state files so tests never share host cache (#559). @@ -36,6 +38,9 @@ def _reset_mutation_authority(monkeypatch): except Exception: mcp_session_state = None if mcp_session_state is not None: + # Survive patch.dict(..., clear=True) that drops the env var (#590). + # Still honor an explicit GITEA_MCP_SESSION_STATE_DIR when tests set + # their own temp dir (see tests/test_mcp_session_state.py). monkeypatch.setattr( mcp_session_state, "DEFAULT_STATE_DIR", state_dir, raising=False ) diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index a793e15..66d4477 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -1006,6 +1006,64 @@ class TestMergePR(unittest.TestCase): r["reasons"]) self._assert_no_merge_call(mock_api) + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_env_clear_does_not_adopt_host_review_mutation_ledger( + self, _auth, mock_api + ): + """#590: patch.dict(clear=True) must not load host terminal mutations. + + Simulates a dirty host session-state file for profile gitea-default + (the profile active when env is fully cleared). Isolation must keep + the #332 hard-stop ledger clean so the empty-profile gate fires. + """ + import tempfile + + import mcp_session_state + + poison_dir = tempfile.mkdtemp(prefix="gitea-host-poison-") + mcp_session_state.save_state( + kind=mcp_session_state.KIND_DECISION_LOCK, + profile_identity="gitea-default", + state_dir=poison_dir, + payload={ + "task": "review_pr", + "remote": "prgs", + "final_review_decision_ready": True, + "ready_pr_number": 8, + "ready_action": "request_changes", + "live_mutations": [ + { + "pr_number": 8, + "action": "request_changes", + "review_id": 99, + "review_state": "request_changes", + } + ], + }, + ) + # If isolation only used the env var, clear=True would fall back to + # DEFAULT_STATE_DIR and adopt this poison ledger. + mcp_server._REVIEW_DECISION_LOCK = None + mock_api.side_effect = [{"login": "merger-bot"}, self._pr("author-bot")] + with patch.object(mcp_session_state, "DEFAULT_STATE_DIR", poison_dir): + with patch.dict(os.environ, {}, clear=True): + r = gitea_merge_pr( + pr_number=8, + confirmation=self._confirm(8), + remote="prgs", + ) + self.assertFalse(r["performed"]) + self.assertIn( + "profile has no configured allowed operations (fail closed)", + r["reasons"], + ) + self.assertFalse( + any("terminal review mutation already consumed" in x for x in r["reasons"]), + msg=f"host ledger leaked into reasons: {r['reasons']}", + ) + self._assert_no_merge_call(mock_api) + @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_profile_without_merge_permission_blocks(self, _auth, mock_api): diff --git a/tests/test_session_state_isolation.py b/tests/test_session_state_isolation.py new file mode 100644 index 0000000..663502b --- /dev/null +++ b/tests/test_session_state_isolation.py @@ -0,0 +1,62 @@ +"""Regression: durable session-state isolation survives env clears (#590).""" + +from __future__ import annotations + +import os +from pathlib import Path +from unittest.mock import patch + +import mcp_session_state + + +def test_default_state_dir_survives_env_clear(): + """conftest pins default_state_dir so clear=True cannot hit host cache.""" + isolated = os.environ.get("GITEA_MCP_SESSION_STATE_DIR") + assert isolated, "conftest must set GITEA_MCP_SESSION_STATE_DIR" + host_cache = Path.home() / ".cache" / "gitea-tools" / "session-state" + with patch.dict(os.environ, {}, clear=True): + assert os.environ.get("GITEA_MCP_SESSION_STATE_DIR") is None + resolved = mcp_session_state.default_state_dir() + assert resolved == isolated + assert Path(resolved).resolve() != host_cache.resolve() + + +def test_decision_lock_save_load_stays_in_isolated_dir(): + """Review-mutation ledger files land only under the per-test state dir.""" + isolated = os.environ.get("GITEA_MCP_SESSION_STATE_DIR") + assert isolated + with patch.dict(os.environ, {}, clear=True): + saved = mcp_session_state.save_state( + kind=mcp_session_state.KIND_DECISION_LOCK, + profile_identity="gitea-default", + payload={ + "live_mutations": [ + {"pr_number": 1, "action": "request_changes"} + ], + }, + ) + assert saved is not None + path = mcp_session_state.state_file_path( + kind=mcp_session_state.KIND_DECISION_LOCK, + profile_identity="gitea-default", + ) + assert path.startswith(isolated + os.sep) or path.startswith( + isolated + "/" + ) + loaded = mcp_session_state.load_state( + kind=mcp_session_state.KIND_DECISION_LOCK, + profile_identity="gitea-default", + ) + assert loaded is not None + assert loaded.get("live_mutations") + host_path = ( + Path.home() + / ".cache" + / "gitea-tools" + / "session-state" + / "review_decision_lock-gitea-default.json" + ) + # Host file may exist from operator use; this write must not refresh it. + # Prove our isolated file is distinct and present. + assert Path(path).is_file() + assert Path(path).resolve() != host_path.resolve()