diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 6fb55cd..ad1a589 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -13659,36 +13659,18 @@ def gitea_route_task_session( ) -_restart_triggered = False - - -def _trigger_mcp_auto_restart(): - global _restart_triggered - if _restart_triggered or _preflight_in_test_mode(): - return - _restart_triggered = True - - config_path = os.environ.get( - "MCP_CONFIG_PATH", - os.path.expanduser("~/.gemini/config/mcp_config.json") - ) - - try: - if os.path.exists(config_path): - os.utime(config_path, None) - except Exception: - pass - - import threading - import time - def delayed_exit(): - time.sleep(1.0) - os._exit(0) - threading.Thread(target=delayed_exit, daemon=True).start() +# #685: resolver stale-runtime detection is report-only. Config touch / os._exit +# self-recovery was removed from the read-only path (was _trigger_mcp_auto_restart). +# Recovery is owned exclusively by the IDE/client reconnect path. def _check_mcp_runtimes_diagnostics(task: str, matching_profiles: list[str]) -> list[str]: - """Check running runtimes and return errors if they are missing or stale.""" + """Read-only: report missing or stale MCP runtimes (no config or process mutation). + + #685: Never touches MCP client config, never spawns recovery threads, never + calls ``os._exit``. Stale detection remains fail-closed via returned reasons + only; the IDE/client owns reconnect/reload. + """ import subprocess import re from datetime import datetime @@ -13768,11 +13750,13 @@ def _check_mcp_runtimes_diagnostics(task: str, matching_profiles: list[str]) -> } if self_stale: - _trigger_mcp_auto_restart() + # #685: report-only — no config utime, no thread, no os._exit. reasons.append( - "stale-runtime: The active Gitea MCP server process is stale (running code from before changes were merged). " - "Auto-restart has been triggered: touched mcp_config.json to reload the daemon. " - "The current process will cleanly exit shortly." + "stale-runtime: The active Gitea MCP server process is stale " + "(running code from before changes were merged). " + "Reconnect the IDE/client-managed MCP namespace for this profile " + "so it reloads current master. The resolver does not touch " + "mcp_config.json, spawn recovery threads, or terminate this process." ) if matching_profiles: @@ -13804,10 +13788,15 @@ def gitea_resolve_task_capability( remote: str = "dadeschools", host: str | None = None, ) -> dict: - """Read-only: Resolve which capability, profile, and namespace is required for a Gitea task. + """Read-only / side-effect free: resolve capability, profile, and namespace for a task. - Helps the client or LLM determine the correct namespace or profile before acting, - and returns exact next action instructions if the current session is not authorized. + Does **not** mutate MCP client configuration, spawn recovery threads, kill + processes, or trigger daemon reloads (#685). Stale-runtime detection remains + fail-closed: when the serving process is stale the result includes + ``blocker_kind=runtime_reconnect_required``, ``restart_required=true``, + ``stop_required=true``, and ``mutation_performed=false`` with a precise + ``exact_safe_next_action`` pointing at IDE/client reconnect. Recovery is + owned by the client reconnect path — never by this resolver. Args: task: The task/action to check (e.g. review_pr, create_issue). @@ -13992,31 +13981,32 @@ def gitea_resolve_task_capability( configured = len(matching_profiles) > 0 available_in_session = allowed_in_current_session + runtime_stale_blocker = False if "PYTEST_CURRENT_TEST" not in os.environ or "GITEA_FORCE_MCP_RUNTIME_CHECK" in os.environ: runtime_reasons = _check_mcp_runtimes_diagnostics(task, matching_profiles) if runtime_reasons: restart_required = True + runtime_stale_blocker = True reason_msg = "; ".join(runtime_reasons) - next_safe_action = ( - "stale-runtime: Gitea MCP runtime conflict or missing process detected. " - "Please fully restart the Gitea MCP server and retry." - ) if not allowed_in_current_session: if configured and switching: restart_required = True available_in_session = False - reason_msg = ( - f"{required_role.capitalize()} profile exists but MCP server " - "was added after session startup and is not attached." - ) + if not reason_msg: + reason_msg = ( + f"{required_role.capitalize()} profile exists but MCP server " + "was added after session startup and is not attached." + ) elif not configured: - reason_msg = ( - f"No profile configured with permission '{required_permission}'." - ) + if not reason_msg: + reason_msg = ( + f"No profile configured with permission '{required_permission}'." + ) elif role_mismatch_reason: - reason_msg = role_mismatch_reason + if not reason_msg: + reason_msg = role_mismatch_reason different_namespace_required = False next_safe_action = "None; ready for operations." @@ -14042,6 +14032,16 @@ def gitea_resolve_task_capability( "or use the corresponding MCP namespace." ) + # #685: stale-runtime typed remediation wins for exact_next_action when the + # serving process/profile inventory is stale — even if permission is OK. + if runtime_stale_blocker: + next_safe_action = ( + "blocker_kind=runtime_reconnect_required: reconnect/restart the " + "IDE-managed Gitea MCP server for this profile so it reloads current " + "master. Do not edit mcp_config.json by hand; the resolver does not " + "touch config, spawn recovery threads, or terminate the process." + ) + # Task/role alignment guards (#167): the requested task, not the # available credential, decides what the session may do. A review/merge # task under a non-reviewer profile must stop — not silently degrade @@ -14103,12 +14103,16 @@ def gitea_resolve_task_capability( "configured": configured, "restart_required": restart_required, "stop_required": stop_required or restart_required, + # #685: resolver is always side-effect free; never claims mutations. + "mutation_performed": False, "task_role_guidance": task_role_guidance, "matching_configured_profile": matching_profiles, "runtime_switching_supported": switching, "different_mcp_namespace_required": different_namespace_required, "exact_safe_next_action": next_safe_action, } + if runtime_stale_blocker: + result["blocker_kind"] = "runtime_reconnect_required" if reason_msg: result["reason"] = reason_msg if task in ("review_pr", "merge_pr"): diff --git a/tests/test_issue_685_side_effect_free_resolver.py b/tests/test_issue_685_side_effect_free_resolver.py new file mode 100644 index 0000000..1d63323 --- /dev/null +++ b/tests/test_issue_685_side_effect_free_resolver.py @@ -0,0 +1,326 @@ +"""#685: gitea_resolve_task_capability must be side-effect free. + +Stale-runtime detection remains fail-closed, but the resolver must never: + * touch mcp_config.json (or any MCP client config) + * spawn recovery threads + * call os._exit / terminate the serving process + * claim that an auto-restart was triggered + +Recovery is owned by the IDE/client reconnect path only. +""" + +from __future__ import annotations + +import os +import tempfile +import threading +import unittest +from datetime import datetime +from pathlib import Path +from unittest.mock import MagicMock, patch + +import sys + +ROOT = str(Path(__file__).resolve().parent.parent) +if ROOT not in sys.path: + sys.path.insert(0, ROOT) + +import gitea_mcp_server as mcp_server + + +ROLE_PROFILES = ( + ("create_issue", "prgs-author", "author"), + ("review_pr", "prgs-reviewer", "reviewer"), + ("merge_pr", "prgs-merger", "merger"), + ("reconciliation_cleanup", "prgs-reconciler", "reconciler"), +) + + +def _stale_self_ps_mocks(profile: str = "prgs-author"): + """Build subprocess mocks: self PID is stale vs code mtime.""" + mock_getpid = MagicMock(return_value=12345) + mock_exists = MagicMock(return_value=True) + code_time = datetime(2026, 7, 8, 14, 0, 0) + mock_getmtime = MagicMock(return_value=code_time.timestamp()) + + ps_output = ( + " PID LSTART COMMAND\n" + "12345 Wed Jul 8 13:00:00 2026 /path/to/python mcp_server.py\n" + ) + mock_run_ps = MagicMock() + mock_run_ps.stdout = ps_output + + mock_run_env = MagicMock() + mock_run_env.stdout = f"GITEA_MCP_PROFILE={profile}" + + mock_run_git = MagicMock() + mock_run_git.stdout = "SAME" + + def side_effect(args, **kwargs): + if args[0] == "ps" and "eww" in args: + return mock_run_env + if args[0] == "ps": + return mock_run_ps + if args[0] == "git": + return mock_run_git + raise ValueError(f"Unexpected subprocess args: {args}") + + mock_run = MagicMock(side_effect=side_effect) + return mock_getpid, mock_exists, mock_getmtime, mock_run + + +class TestIssue685DiagnosticsNoSideEffects(unittest.TestCase): + def setUp(self): + mcp_server._process_boot_head_sha = None + + def tearDown(self): + mcp_server._process_boot_head_sha = None + + @patch.dict(os.environ, {"GITEA_FORCE_MCP_RUNTIME_CHECK": "1"}, clear=False) + @patch("subprocess.run") + @patch("os.path.getmtime") + @patch("os.path.exists") + @patch("os.getpid") + @patch("os.utime") + @patch("threading.Thread") + @patch("os._exit") + def test_stale_self_does_not_touch_config_or_exit( + self, + mock_exit, + mock_thread, + mock_utime, + mock_getpid, + mock_exists, + mock_getmtime, + mock_run, + ): + mock_getpid.return_value = 12345 + mock_exists.return_value = True + mock_getmtime.return_value = datetime(2026, 7, 8, 14, 0, 0).timestamp() + mock_run.side_effect = _stale_self_ps_mocks("prgs-author")[3].side_effect + + before_threads = threading.active_count() + reasons = mcp_server._check_mcp_runtimes_diagnostics( + "create_issue", ["prgs-author"] + ) + after_threads = threading.active_count() + + self.assertTrue( + any("stale-runtime" in r and "active Gitea MCP server process is stale" in r + for r in reasons), + reasons, + ) + # Must not claim auto-restart / config touch + blob = " ".join(reasons) + self.assertNotIn("Auto-restart has been triggered", blob) + self.assertNotIn("touched mcp_config", blob) + self.assertNotIn("will cleanly exit", blob) + + mock_utime.assert_not_called() + mock_thread.assert_not_called() + mock_exit.assert_not_called() + self.assertEqual(before_threads, after_threads) + + @patch.dict(os.environ, {"GITEA_FORCE_MCP_RUNTIME_CHECK": "1"}, clear=False) + @patch("subprocess.run") + @patch("os.path.getmtime") + @patch("os.path.exists") + @patch("os.getpid") + @patch("os.utime") + def test_repeated_stale_calls_do_not_trigger_restart_loop( + self, mock_utime, mock_getpid, mock_exists, mock_getmtime, mock_run + ): + mock_getpid.return_value = 12345 + mock_exists.return_value = True + mock_getmtime.return_value = datetime(2026, 7, 8, 14, 0, 0).timestamp() + mock_run.side_effect = _stale_self_ps_mocks("prgs-author")[3].side_effect + + for _ in range(5): + reasons = mcp_server._check_mcp_runtimes_diagnostics( + "create_issue", ["prgs-author"] + ) + self.assertTrue(any("stale-runtime" in r for r in reasons)) + + mock_utime.assert_not_called() + + def test_trigger_mcp_auto_restart_removed(self): + """#685 AC: auto-restart helper is removed (unreachable from read-only).""" + self.assertFalse(hasattr(mcp_server, "_trigger_mcp_auto_restart")) + self.assertFalse(hasattr(mcp_server, "_restart_triggered")) + + +class TestIssue685ResolverTypedBlocker(unittest.TestCase): + def setUp(self): + mcp_server._process_boot_head_sha = None + + def tearDown(self): + mcp_server._process_boot_head_sha = None + if hasattr(mcp_server, "capability_stop_terminal"): + mcp_server.capability_stop_terminal.clear() + + def _resolve_with_stale_runtime(self, task: str, profile_name: str, role: str): + allowed = [ + "gitea.read", + "gitea.issue.create", + "gitea.issue.comment", + "gitea.issue.close", + "gitea.branch.create", + "gitea.branch.push", + "gitea.branch.delete", + "gitea.pr.create", + "gitea.pr.comment", + "gitea.pr.review", + "gitea.pr.approve", + "gitea.pr.request_changes", + "gitea.pr.merge", + "gitea.pr.close", + "gitea.repo.commit", + ] + profile = { + "profile_name": profile_name, + "role": role, + "allowed_operations": allowed, + "forbidden_operations": [], + } + config = { + "profiles": { + profile_name: { + "role": role, + "allowed_operations": allowed, + "forbidden_operations": [], + } + } + } + + mock_getpid, mock_exists, mock_getmtime, mock_run = _stale_self_ps_mocks( + profile_name + ) + + with patch.dict( + os.environ, + { + "GITEA_FORCE_MCP_RUNTIME_CHECK": "1", + "GITEA_MCP_PROFILE": profile_name, + }, + clear=False, + ), patch.object(mcp_server, "get_profile", return_value=profile), patch.object( + mcp_server.gitea_config, "load_config", return_value=config + ), patch.object( + mcp_server, "_authenticated_username", return_value="test-user" + ), patch.object( + mcp_server, "_ensure_matching_profile", return_value=None + ), patch.object( + mcp_server, "record_preflight_check", return_value=None + ), patch.object( + mcp_server, "record_mutation_authority", return_value=None + ), patch.object( + mcp_server, "init_review_decision_lock", return_value=None + ), patch( + "subprocess.run", mock_run + ), patch( + "os.path.getmtime", mock_getmtime + ), patch( + "os.path.exists", mock_exists + ), patch( + "os.getpid", mock_getpid + ), patch( + "os.utime" + ) as mock_utime, patch( + "threading.Thread" + ) as mock_thread, patch( + "os._exit" + ) as mock_exit: + result = mcp_server.gitea_resolve_task_capability(task=task, remote="prgs") + return result, mock_utime, mock_thread, mock_exit + + def test_stale_returns_typed_blocker_fields(self): + result, mock_utime, mock_thread, mock_exit = self._resolve_with_stale_runtime( + "create_issue", "prgs-author", "author" + ) + self.assertTrue(result.get("restart_required"), result) + self.assertTrue(result.get("stop_required"), result) + self.assertEqual(result.get("blocker_kind"), "runtime_reconnect_required") + self.assertIs(result.get("mutation_performed"), False) + action = result.get("exact_safe_next_action") or "" + self.assertIn("reconnect", action.lower()) + self.assertNotIn("None; ready for operations", action) + reason = result.get("reason") or "" + self.assertIn("stale-runtime", reason) + self.assertNotIn("Auto-restart has been triggered", reason) + mock_utime.assert_not_called() + mock_thread.assert_not_called() + mock_exit.assert_not_called() + + def test_all_four_role_profiles_get_same_side_effect_free_contract(self): + for task, profile, role in ROLE_PROFILES: + with self.subTest(task=task, profile=profile): + # Skip tasks that may be unknown on this branch + try: + import task_capability_map as tcm + + tcm.required_permission(task) + except Exception: + self.skipTest(f"task {task} not in capability map") + + result, mock_utime, mock_thread, mock_exit = ( + self._resolve_with_stale_runtime(task, profile, role) + ) + self.assertTrue( + result.get("restart_required") or result.get("stop_required"), + result, + ) + self.assertEqual( + result.get("blocker_kind"), "runtime_reconnect_required", result + ) + self.assertIs(result.get("mutation_performed"), False, result) + mock_utime.assert_not_called() + mock_thread.assert_not_called() + mock_exit.assert_not_called() + + def test_config_mtime_and_contents_unchanged(self): + with tempfile.TemporaryDirectory() as tmp: + cfg = os.path.join(tmp, "mcp_config.json") + original = '{"servers": {"gitea-author": {}}}' + with open(cfg, "w", encoding="utf-8") as fh: + fh.write(original) + mtime_before = os.path.getmtime(cfg) + + result, mock_utime, mock_thread, mock_exit = self._resolve_with_stale_runtime( + "create_issue", "prgs-author", "author" + ) + # Force-path also must not use real utime when diagnostics runs + with open(cfg, encoding="utf-8") as fh: + after = fh.read() + self.assertEqual(after, original) + self.assertEqual(os.path.getmtime(cfg), mtime_before) + mock_utime.assert_not_called() + self.assertTrue(result.get("restart_required"), result) + + +class TestIssue685MutationGatesStillFailClosed(unittest.TestCase): + def test_parity_stale_still_reports_restart_required(self): + """Mutation-facing parity gate remains fail-closed when heads differ.""" + import master_parity_gate as mpg + + out = mpg.assess_master_parity( + {"startup_head": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + ) + self.assertFalse(out.get("in_parity")) + self.assertTrue(out.get("restart_required") or out.get("stale")) + + +class TestIssue685DocstringReadOnlyContract(unittest.TestCase): + def test_resolve_docstring_declares_side_effect_free(self): + doc = mcp_server.gitea_resolve_task_capability.__doc__ or "" + lower = doc.lower() + self.assertTrue( + "side-effect" in lower or "read-only" in lower or "does not mutate" in lower, + doc, + ) + self.assertNotIn("auto-restart", lower) + self.assertNotIn("os._exit", lower) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_mcp_stale_runtime.py b/tests/test_mcp_stale_runtime.py index ddd6287..7a92fc3 100644 --- a/tests/test_mcp_stale_runtime.py +++ b/tests/test_mcp_stale_runtime.py @@ -111,21 +111,13 @@ class TestMcpStaleRuntime(unittest.TestCase): reasons = gitea_mcp_server._check_mcp_runtimes_diagnostics("create_issue", ["prgs-author"]) self.assertTrue(any("stale-runtime: The active Gitea MCP server process is stale" in r for r in reasons)) - @patch("threading.Thread") - @patch("os.utime") - @patch("os.path.exists") - @patch.dict("os.environ", {"MCP_CONFIG_PATH": "/tmp/mcp_config.json"}) - def test_auto_restart_trigger_touches_and_spawns(self, mock_exists, mock_utime, mock_thread): - mock_exists.return_value = True - gitea_mcp_server._restart_triggered = False - - # Ensure we are not skipped in test mode for testing purposes - with patch("gitea_mcp_server._preflight_in_test_mode", return_value=False): - gitea_mcp_server._trigger_mcp_auto_restart() - - mock_utime.assert_called_once_with("/tmp/mcp_config.json", None) - mock_thread.assert_called_once() - self.assertTrue(gitea_mcp_server._restart_triggered) + def test_auto_restart_helper_removed_from_read_only_path(self): + """#685: config-touch / os._exit self-recovery is no longer on the server.""" + self.assertFalse( + hasattr(gitea_mcp_server, "_trigger_mcp_auto_restart"), + "_trigger_mcp_auto_restart must not remain (side-effect-free resolver)", + ) + self.assertFalse(hasattr(gitea_mcp_server, "_restart_triggered")) if __name__ == "__main__":