fix: isolate review-mutation ledger from host session-state (Closes #590) #592
+11
-3
@@ -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",
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user