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
2026-07-09 14:23:06 -04:00
parent 6913ac98f1
commit 5fae0ae4e7
3 changed files with 158 additions and 1 deletions
@@ -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:
@@ -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):
@@ -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()