diff --git a/tests/conftest.py b/tests/conftest.py index 1873223..7a9d84f 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 ) @@ -69,6 +74,9 @@ def _reset_mutation_authority(monkeypatch): monkeypatch.setattr(mcp_server, "_preflight_capability_baseline_porcelain", None) monkeypatch.setattr(mcp_server, "_preflight_resolved_task", None) monkeypatch.setattr(mcp_server, "_preflight_reviewer_violation_files", []) + # Unit tests must not depend on live host MCP process health. Tests that + # use patch.dict(..., clear=True) drop PYTEST_CURRENT_TEST, which would + # otherwise re-enable the stale-runtime probe against operator daemons. monkeypatch.setattr( mcp_server, "_check_mcp_runtimes_diagnostics", diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index a793e15..8e74d0e 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -776,6 +776,7 @@ class TestMergePR(unittest.TestCase): # Clean ledger each merge test so residual/host durable #332 state # cannot short-circuit eligibility gates (#590 / #594). + # 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) @@ -1006,6 +1007,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()