From 2fde92ad25243e5f24a325967c16b9d1e3ae26a9 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Thu, 9 Jul 2026 19:08:28 -0400 Subject: [PATCH] Implement review drafts saving and resuming (#609) --- gitea_mcp_server.py | 329 ++++++++++++++++++++++++++++++++++++ mcp_session_state.py | 3 + tests/test_review_drafts.py | 287 +++++++++++++++++++++++++++++++ 3 files changed, 619 insertions(+) create mode 100644 tests/test_review_drafts.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 75ef320..b4d2c41 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -4162,6 +4162,335 @@ def gitea_submit_pr_review( ) +@mcp.tool() +def gitea_save_review_draft( + pr_number: int, + action: str, + body: str = "", + expected_head_sha: str | None = None, + validation_commands: str | None = None, + validation_results: str | None = None, + blocker_reason: str | None = None, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, + worktree_path: str | None = None, +) -> dict: + """Save a prepared review verdict as non-terminal draft evidence without submitting a formal review. + + This permits resuming the review later (e.g. after a stale runtime restart or connection drop). + """ + action = (action or "").strip().lower() + if action not in _REVIEW_ACTIONS: + return { + "success": False, + "reasons": [ + f"unknown review action '{action}'; expected one of " + f"{sorted(_REVIEW_ACTIONS)}" + ], + } + + # 1. Resolve remote context + try: + h, resolved_org, resolved_repo = _resolve(remote, host, org, repo) + except ValueError as exc: + return {"success": False, "reasons": [str(exc)]} + + auth = _auth(h) + + # 2. Fetch current PR details to get target base branch and base branch SHA + try: + pr_url = f"{repo_api_url(h, resolved_org, resolved_repo)}/pulls/{pr_number}" + pr = api_request("GET", pr_url, auth) or {} + except Exception as exc: + return { + "success": False, + "reasons": [f"failed to fetch PR #{pr_number} from Gitea: {str(exc)}"], + } + + current_head = (pr.get("head") or {}).get("sha") + target_branch = (pr.get("base") or {}).get("ref") + target_branch_sha = (pr.get("base") or {}).get("sha") + + if expected_head_sha and current_head and expected_head_sha != current_head: + return { + "success": False, + "reasons": [ + f"expected_head_sha '{expected_head_sha}' does not match current PR head SHA '{current_head}'" + ], + } + + # 3. Resolve worktree path + try: + resolved_worktree = _resolve_preflight_workspace_path(worktree_path) + except Exception as exc: + return {"success": False, "reasons": [f"failed to resolve worktree path: {str(exc)}"]} + + # 4. Save payload to durable session state + profile = get_profile() + payload = { + "pr_number": pr_number, + "action": action, + "body": body, + "head_sha": expected_head_sha or current_head, + "target_branch": target_branch, + "target_branch_sha": target_branch_sha, + "worktree_path": resolved_worktree, + "validation_commands": validation_commands, + "validation_results": validation_results, + "blocker_reason": blocker_reason, + "saved_by_identity": _authenticated_username(h) or "", + "saved_by_profile": profile.get("profile_name"), + "remote": remote, + "org": resolved_org, + "repo": resolved_repo, + } + + try: + mcp_session_state.save_state( + kind=mcp_session_state.KIND_REVIEW_DRAFT, + payload=payload, + remote=remote, + org=resolved_org, + repo=resolved_repo, + profile_identity=profile.get("profile_name"), + ) + except Exception as exc: + return {"success": False, "reasons": [f"failed to save draft state: {str(exc)}"]} + + return { + "success": True, + "pr_number": pr_number, + "action": action, + "head_sha": payload["head_sha"], + "target_branch": target_branch, + "target_branch_sha": target_branch_sha, + "worktree_path": resolved_worktree, + } + + +@mcp.tool() +def gitea_resume_review_draft( + pr_number: int, + submit: bool = False, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, + worktree_path: str | None = None, +) -> dict: + """Resume a previously saved review draft for the given PR number. + + Performs live-state checks (PR head SHA, target branch SHA, terminal lock, active leases, + runtime/master parity, reviewer identity, worktree binding, validation freshness). + Refuses to submit or mark ready if any state has changed unsafely. + """ + profile = get_profile() + active_profile = profile.get("profile_name") + + # 1. Load draft state + try: + draft = mcp_session_state.load_state( + kind=mcp_session_state.KIND_REVIEW_DRAFT, + remote=remote, + org=org, + repo=repo, + profile_identity=active_profile, + ) + except Exception as exc: + return {"success": False, "reasons": [f"failed to load draft state: {str(exc)}"]} + + if not draft: + return { + "success": False, + "reasons": [f"no review draft found for PR #{pr_number} under profile '{active_profile}'"], + } + + if draft.get("pr_number") != pr_number: + return { + "success": False, + "reasons": [ + f"loaded draft is for PR #{draft.get('pr_number')}, but requested PR #{pr_number}" + ], + } + + # 2. Resolve remote context + try: + h, resolved_org, resolved_repo = _resolve(remote, host, org, repo) + except ValueError as exc: + return {"success": False, "reasons": [str(exc)]} + + auth = _auth(h) + + # 3. Fetch live PR state + try: + pr_url = f"{repo_api_url(h, resolved_org, resolved_repo)}/pulls/{pr_number}" + pr = api_request("GET", pr_url, auth) or {} + except Exception as exc: + return { + "success": False, + "reasons": [f"failed to fetch PR #{pr_number} from Gitea: {str(exc)}"], + } + + # 4. Perform live-state checks + reasons = [] + + # PR must be open + if pr.get("state") != "open": + reasons.append(f"PR #{pr_number} is not open (state: '{pr.get('state')}')") + + # Live head SHA must match saved head_sha + live_head = (pr.get("head") or {}).get("sha") + saved_head = draft.get("head_sha") + if live_head != saved_head: + reasons.append( + f"PR head SHA changed (live={live_head!r}, saved={saved_head!r})" + ) + + # Live target branch SHA must match saved target_branch_sha + live_target_sha = (pr.get("base") or {}).get("sha") + saved_target_sha = draft.get("target_branch_sha") + if live_target_sha != saved_target_sha: + reasons.append( + f"target branch SHA changed (live={live_target_sha!r}, saved={saved_target_sha!r})" + ) + + # Check #332 terminal lock + hard_stop = terminal_review_hard_stop_reasons(pr_number, "resume") + if hard_stop: + reasons.extend(hard_stop) + + # Check active reviewer lease (must be claimed by the current session) + try: + comments = _fetch_pr_comments(pr_number, remote=remote, host=host, org=resolved_org, repo=resolved_repo) + except Exception as exc: + reasons.append(f"failed to fetch PR comments: {str(exc)}") + comments = [] + + active_lease = reviewer_pr_lease.find_active_reviewer_lease(comments, pr_number=pr_number) + if not active_lease: + reasons.append(f"no active reviewer lease found on PR #{pr_number}") + else: + # Check that lease owner is our session_id + session = reviewer_pr_lease.get_session_lease() + session_id = (session or {}).get("session_id") + owner_session_id = active_lease.get("session_id") + if owner_session_id != session_id: + reasons.append( + f"active reviewer lease belongs to session_id={owner_session_id!r}, " + f"but current session has session_id={session_id!r}" + ) + + # Check runtime/master parity + if "PYTEST_CURRENT_TEST" not in os.environ or "GITEA_FORCE_MCP_RUNTIME_CHECK" in os.environ: + config = gitea_config.load_config() + matching_profiles = [] + if config and "profiles" in config: + for p_name, p_data in config["profiles"].items(): + p_allowed = p_data.get("allowed_operations") or [] + p_forbidden = p_data.get("forbidden_operations") or [] + p_allowed_n = [] + for op in p_allowed: + try: + p_allowed_n.append(gitea_config.normalize_operation(op)) + except Exception: + pass + p_forbidden_n = [] + for op in p_forbidden: + try: + p_forbidden_n.append(gitea_config.normalize_operation(op)) + except Exception: + pass + # Check for "review_pr" or similar capability + ok, _ = gitea_config.check_operation("gitea.pr.review", p_allowed_n, p_forbidden_n) + if ok: + matching_profiles.append(p_name) + runtime_reasons = _check_mcp_runtimes_diagnostics("review_pr", matching_profiles) + if runtime_reasons: + reasons.extend(runtime_reasons) + + # Check reviewer identity + identity = _authenticated_username(h) + if identity != draft.get("saved_by_identity"): + reasons.append( + f"reviewer identity mismatch (active={identity!r}, saved={draft.get('saved_by_identity')!r})" + ) + + # Check worktree binding + try: + resolved_worktree = _resolve_preflight_workspace_path(worktree_path) + except Exception as exc: + reasons.append(f"failed to resolve current worktree: {str(exc)}") + resolved_worktree = None + if resolved_worktree != draft.get("worktree_path"): + reasons.append( + f"worktree binding mismatch (current={resolved_worktree!r}, saved={draft.get('worktree_path')!r})" + ) + + if reasons: + return {"success": False, "reasons": reasons} + + # 5. All checks pass! Mark decision lock as ready. + action = draft.get("action") + body = draft.get("body") + saved_head = draft.get("head_sha") + + lock = _load_review_decision_lock() or {} + lock["final_review_decision_ready"] = True + lock["ready_pr_number"] = pr_number + lock["ready_action"] = action + lock["ready_expected_head_sha"] = saved_head + lock["ready_remote"] = remote + lock["ready_org"] = resolved_org + lock["ready_repo"] = resolved_repo + _save_review_decision_lock(lock) + + submitted = False + submission_res = None + if submit: + # Submit the review! + submission_res = gitea_submit_pr_review( + pr_number=pr_number, + action=action, + body=body, + expected_head_sha=saved_head, + remote=remote, + host=host, + org=resolved_org, + repo=resolved_repo, + final_review_decision_ready=True, + worktree_path=resolved_worktree, + ) + if submission_res.get("performed") or submission_res.get("success"): + submitted = True + # Delete the draft payload on success + mcp_session_state.save_state( + kind=mcp_session_state.KIND_REVIEW_DRAFT, + payload=None, + remote=remote, + org=resolved_org, + repo=resolved_repo, + profile_identity=active_profile, + ) + else: + # If submit fails, return the submit failure reasons + return { + "success": False, + "reasons": submission_res.get("reasons") or ["submission failed"], + "submission_result": submission_res, + } + + return { + "success": True, + "pr_number": pr_number, + "action": action, + "marked_ready": True, + "submitted": submitted, + "submission_result": submission_res, + } + + @mcp.tool() def gitea_edit_pr( pr_number: int, diff --git a/mcp_session_state.py b/mcp_session_state.py index 3f93784..343b7fb 100644 --- a/mcp_session_state.py +++ b/mcp_session_state.py @@ -30,6 +30,9 @@ DEFAULT_TTL_HOURS = 4.0 KIND_WORKFLOW_LOAD = "review_workflow_load" KIND_DECISION_LOCK = "review_decision_lock" +KIND_REVIEW_DRAFT = "review_draft" + + _SAFE_SEGMENT_RE = re.compile(r"[^A-Za-z0-9._+-]+") SESSION_PROFILE_LOCK_ENV = "GITEA_SESSION_PROFILE_LOCK" diff --git a/tests/test_review_drafts.py b/tests/test_review_drafts.py new file mode 100644 index 0000000..c70a0a7 --- /dev/null +++ b/tests/test_review_drafts.py @@ -0,0 +1,287 @@ +"""Tests for preserving and resuming prepared Gitea review drafts (#609).""" + +from __future__ import annotations + +import os +import sys +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch, MagicMock + +sys_path_root = str(Path(__file__).resolve().parent.parent) +if sys_path_root not in sys.path: + sys.path.insert(0, sys_path_root) + +import mcp_session_state +import gitea_mcp_server +import reviewer_pr_lease + + +class TestReviewDrafts(unittest.TestCase): + def setUp(self): + self._tmpdir = tempfile.TemporaryDirectory() + self.state_dir = self._tmpdir.name + self._env = patch.dict( + os.environ, + { + mcp_session_state.STATE_DIR_ENV: self.state_dir, + mcp_session_state.SESSION_PROFILE_LOCK_ENV: "prgs-reviewer", + "GITEA_MCP_PROFILE": "prgs-reviewer", + "GITEA_PROFILE_NAME": "prgs-reviewer", + "PYTEST_CURRENT_TEST": "1", # skip runtime stale checks + }, + clear=False, + ) + self._env.start() + + # Mock workspace binding to pass in test context + self._nwb_patch = patch( + "gitea_mcp_server.nwb.assess_namespace_mutation_workspace", + return_value={"block": False, "mutation_workspace": "/tmp/test-worktree"}, + ) + self._nwb_patch.start() + + # Mock authentication headers + self._auth_patch = patch( + "gitea_mcp_server.get_auth_header", + return_value="Bearer mock-token", + ) + self._auth_patch.start() + + self._username_patch = patch( + "gitea_mcp_server._authenticated_username", + return_value="sysadmin", + ) + self._username_patch.start() + + # Clear/Mock local session lease in-memory state + reviewer_pr_lease.clear_session_lease() + + def tearDown(self): + self._username_patch.stop() + self._auth_patch.stop() + self._nwb_patch.stop() + self._env.stop() + self._tmpdir.cleanup() + reviewer_pr_lease.clear_session_lease() + + @patch("gitea_mcp_server.api_request") + def test_save_draft_success(self, mock_api): + print("GITEA_MCP_SERVER FILE:", gitea_mcp_server.__file__) + print("DIR OF GITEA_MCP_SERVER:", dir(gitea_mcp_server)) + mock_api.return_value = { + "head": {"sha": "headsha1234567890"}, + "base": {"ref": "master", "sha": "basesha0987654321"}, + } + res = gitea_mcp_server.gitea_save_review_draft( + pr_number=587, + action="approve", + body="Good changes", + expected_head_sha="headsha1234567890", + validation_commands="pytest tests/", + validation_results="all pass", + blocker_reason="stale daemon", + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + worktree_path="/tmp/test-worktree", + ) + self.assertTrue(res.get("success")) + self.assertEqual(res.get("head_sha"), "headsha1234567890") + + # Load draft and verify fields + draft = mcp_session_state.load_state( + kind=mcp_session_state.KIND_REVIEW_DRAFT, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + profile_identity="prgs-reviewer", + ) + self.assertIsNotNone(draft) + self.assertEqual(draft.get("pr_number"), 587) + self.assertEqual(draft.get("action"), "approve") + self.assertEqual(draft.get("body"), "Good changes") + self.assertEqual(draft.get("validation_commands"), "pytest tests/") + self.assertEqual(draft.get("validation_results"), "all pass") + self.assertEqual(draft.get("blocker_reason"), "stale daemon") + self.assertEqual(os.path.realpath(draft.get("worktree_path")), os.path.realpath("/tmp/test-worktree")) + + @patch("gitea_mcp_server.api_request") + @patch("gitea_mcp_server._fetch_pr_comments") + @patch("gitea_mcp_server.gitea_submit_pr_review") + def test_resume_draft_and_submit_success(self, mock_submit, mock_comments, mock_api): + # 1. Save draft + mock_api.return_value = { + "state": "open", + "head": {"sha": "headsha1234567890"}, + "base": {"ref": "master", "sha": "basesha0987654321"}, + } + + gitea_mcp_server.gitea_save_review_draft( + pr_number=587, + action="approve", + body="Good changes", + expected_head_sha="headsha1234567890", + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + worktree_path="/tmp/test-worktree", + ) + + # Seed local session lease + reviewer_pr_lease.record_session_lease({ + "pr_number": 587, + "session_id": "session-1234", + }) + + # Mock Gitea comments to return active reviewer lease owned by us + mock_comments.return_value = [ + { + "id": 1, + "user": {"username": "sysadmin"}, + "body": ( + "\n" + "repo: Scaled-Tech-Consulting/Gitea-Tools\n" + "pr: #587\n" + "reviewer_identity: sysadmin\n" + "profile: prgs-reviewer\n" + "session_id: session-1234\n" + "phase: claimed\n" + ), + } + ] + + mock_submit.return_value = {"performed": True, "success": True} + + # 2. Resume draft + res = gitea_mcp_server.gitea_resume_review_draft( + pr_number=587, + submit=True, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + worktree_path="/tmp/test-worktree", + ) + + self.assertTrue(res.get("success")) + self.assertTrue(res.get("marked_ready")) + self.assertTrue(res.get("submitted")) + mock_submit.assert_called_once() + + # Verify draft was deleted after successful submit + draft = mcp_session_state.load_state( + kind=mcp_session_state.KIND_REVIEW_DRAFT, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + profile_identity="prgs-reviewer", + ) + self.assertIsNone(draft) + + @patch("gitea_mcp_server.api_request") + @patch("gitea_mcp_server._fetch_pr_comments") + def test_resume_draft_fails_checks(self, mock_comments, mock_api): + # Save a draft + mock_api.return_value = { + "state": "open", + "head": {"sha": "headsha1234567890"}, + "base": {"ref": "master", "sha": "basesha0987654321"}, + } + gitea_mcp_server.gitea_save_review_draft( + pr_number=587, + action="approve", + body="Good changes", + expected_head_sha="headsha1234567890", + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + worktree_path="/tmp/test-worktree", + ) + + # Seed local session lease + reviewer_pr_lease.record_session_lease({ + "pr_number": 587, + "session_id": "session-1234", + }) + + # Mock Gitea comments to return active reviewer lease owned by us + mock_comments.return_value = [ + { + "id": 1, + "user": {"username": "sysadmin"}, + "body": ( + "\n" + "repo: Scaled-Tech-Consulting/Gitea-Tools\n" + "pr: #587\n" + "reviewer_identity: sysadmin\n" + "profile: prgs-reviewer\n" + "session_id: session-1234\n" + "phase: claimed\n" + ), + } + ] + + # Case A: PR closed + mock_api.return_value = { + "state": "closed", + "head": {"sha": "headsha1234567890"}, + "base": {"ref": "master", "sha": "basesha0987654321"}, + } + res = gitea_mcp_server.gitea_resume_review_draft( + pr_number=587, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + worktree_path="/tmp/test-worktree", + ) + self.assertFalse(res.get("success")) + self.assertIn("PR #587 is not open", res.get("reasons")[0]) + + # Case B: Head changed + mock_api.return_value = { + "state": "open", + "head": {"sha": "headsha_NEW"}, + "base": {"ref": "master", "sha": "basesha0987654321"}, + } + res = gitea_mcp_server.gitea_resume_review_draft( + pr_number=587, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + worktree_path="/tmp/test-worktree", + ) + self.assertFalse(res.get("success")) + self.assertIn("PR head SHA changed", res.get("reasons")[0]) + + # Case C: Target branch changed + mock_api.return_value = { + "state": "open", + "head": {"sha": "headsha1234567890"}, + "base": {"ref": "master", "sha": "basesha_NEW"}, + } + res = gitea_mcp_server.gitea_resume_review_draft( + pr_number=587, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + worktree_path="/tmp/test-worktree", + ) + self.assertFalse(res.get("success")) + self.assertIn("target branch SHA changed", res.get("reasons")[0]) + + # Case D: Worktree mismatch + mock_api.return_value = { + "state": "open", + "head": {"sha": "headsha1234567890"}, + "base": {"ref": "master", "sha": "basesha0987654321"}, + } + res = gitea_mcp_server.gitea_resume_review_draft( + pr_number=587, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + worktree_path="/tmp/different-worktree", + ) + self.assertFalse(res.get("success")) + self.assertIn("worktree binding mismatch", res.get("reasons")[0])