diff --git a/tests/conftest.py b/tests/conftest.py index f7c9e8c..5e0863d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ """Shared pytest fixtures for the Gitea-Tools test suite.""" +import os import sys import pytest @@ -16,13 +17,46 @@ def _reset_mutation_authority(monkeypatch): the next test. It must never replace verify_mutation_authority with a no-op: individual tests that need a specific authority state set it up explicitly. + + Session-state isolation (#559 / #590): many tests use + ``patch.dict(os.environ, ..., clear=True)``, which drops + ``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). import tempfile _state_tmp = tempfile.TemporaryDirectory(prefix="gitea-session-state-") - monkeypatch.setenv("GITEA_MCP_SESSION_STATE_DIR", _state_tmp.name) + state_dir = _state_tmp.name + monkeypatch.setenv("GITEA_MCP_SESSION_STATE_DIR", state_dir) + try: + import mcp_session_state + 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 + ) + + def _isolated_default_state_dir( + _fallback: str = state_dir, + _env_key: str = mcp_session_state.STATE_DIR_ENV, + ) -> str: + raw = (os.environ.get(_env_key) or "").strip() + return raw or _fallback + + monkeypatch.setattr( + mcp_session_state, + "default_state_dir", + _isolated_default_state_dir, + ) try: import mcp_server except Exception: diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index d52263b..7b13541 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -774,6 +774,9 @@ class TestMergePR(unittest.TestCase): def setUp(self): import reviewer_pr_lease + # Start each merge test with a clean review-mutation ledger (#590). + # Host durable state can otherwise leak in when tests clear os.environ. + mcp_server._save_review_decision_lock(None) mcp_server.gitea_load_review_workflow() self._lease_patch = _install_owned_reviewer_lease(8) self._lease_patch.start() @@ -1003,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()