Merge pull request 'feat: add conflict-fix leases and stale-head protection (Closes #399)' (#416) from feat/issue-399-conflict-fix-leases into master

This commit was merged in pull request #416.
This commit is contained in:
2026-07-08 00:34:20 -05:00
14 changed files with 1246 additions and 99 deletions
+163 -51
View File
@@ -49,6 +49,7 @@ import mcp_server
import issue_lock_store
FAKE_AUTH = "Basic dGVzdDp0ZXN0"
FULL_HEAD_SHA = "a" * 40
_NO_BLOCKER_FEEDBACK = {
"success": True,
@@ -62,6 +63,7 @@ def _mark_request_changes_ready(pr_number=8, **kwargs):
suppression feedback fetch stubbed to 'no existing blocker'."""
with patch("mcp_server.gitea_get_pr_review_feedback",
return_value=dict(_NO_BLOCKER_FEEDBACK)):
kwargs.setdefault("expected_head_sha", "abc123")
return gitea_mark_final_review_decision(
pr_number, "request_changes", **kwargs)
@@ -82,6 +84,7 @@ def _visible_approval_reviews(reviewer="reviewer-bot", sha="abc123"):
_DEFAULT_LEASE_SESSION = "mcp-test-reviewer-lease"
_NO_PR_WORK_LEASE_BLOCK = {"block": False, "reasons": [], "mutation_allowed": True}
def _reviewer_lease_comment(
@@ -139,6 +142,45 @@ def _install_owned_reviewer_lease(
)
def _seed_ready_review_decision(
pr_number,
action,
*,
sha=FULL_HEAD_SHA,
remote="prgs",
org=None,
repo=None,
):
"""Mark the review-decision lock ready without mark_final API calls (#399)."""
import mcp_server as _m
resolved_org, resolved_repo = org, repo
if remote in _m.REMOTES:
_, resolved_org, resolved_repo = _m._resolve(remote, None, org, repo)
profile_name = (_m.get_profile().get("profile_name") or "").strip()
session_lock = (
(os.environ.get(_m.SESSION_PROFILE_LOCK_ENV) or "").strip()
or profile_name
)
_m._save_review_decision_lock({
"task": "review_pr",
"remote": remote,
"session_pid": os.getpid(),
"session_profile": profile_name,
"session_profile_lock": session_lock,
"final_review_decision_ready": True,
"ready_pr_number": pr_number,
"ready_action": action,
"ready_expected_head_sha": sha,
"ready_remote": remote,
"ready_org": resolved_org,
"ready_repo": resolved_repo,
"live_mutations": [],
"correction_authorized": False,
"correction_reason": None,
})
# Issue-write tools are profile-gated (#69).
ISSUE_WRITE_ENV = {
"GITEA_ALLOWED_OPERATIONS": (
@@ -638,6 +680,17 @@ class TestMergePR(unittest.TestCase):
self.addCleanup(self._auth_identity_patch.stop)
self.addCleanup(self._lease_patch.stop)
self.addCleanup(reviewer_pr_lease.clear_session_lease)
self._pr_lease_comments_patch = patch(
"mcp_server._list_pr_lease_comments", return_value=[]
)
self._pr_lease_comments_patch.start()
self.addCleanup(self._pr_lease_comments_patch.stop)
self._pr_work_lease_patch = patch(
"mcp_server._pr_work_lease_reviewer_block",
return_value=dict(_NO_PR_WORK_LEASE_BLOCK),
)
self._pr_work_lease_patch.start()
self.addCleanup(self._pr_work_lease_patch.stop)
def _pr(self, author, state="open", sha="abc123", mergeable=True):
return {
@@ -711,7 +764,8 @@ class TestMergePR(unittest.TestCase):
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(
pr_number=8, confirmation=self._confirm(8),
expected_changed_files=["b.py", "a.py"], remote="prgs")
expected_changed_files=["b.py", "a.py"],
expected_head_sha="abc123", remote="prgs")
self.assertTrue(r["performed"])
# -- read-back / cleanup surfacing (#98) -----------------------------------
@@ -729,8 +783,10 @@ class TestMergePR(unittest.TestCase):
env = {"GITEA_PROFILE_NAME": "gitea-merger",
"GITEA_ALLOWED_OPERATIONS": "read,merge"}
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(pr_number=8, confirmation=self._confirm(8),
remote="prgs")
r = gitea_merge_pr(
pr_number=8, confirmation=self._confirm(8),
expected_head_sha="abc123", remote="prgs",
)
# The merge itself is still reported performed/successful.
self.assertTrue(r["performed"])
self.assertEqual(r["merge_result"], "PR #8 merged via 'merge'.")
@@ -758,8 +814,10 @@ class TestMergePR(unittest.TestCase):
env = {"GITEA_PROFILE_NAME": "gitea-merger",
"GITEA_ALLOWED_OPERATIONS": "read,merge"}
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(pr_number=8, confirmation=self._confirm(8),
remote="prgs")
r = gitea_merge_pr(
pr_number=8, confirmation=self._confirm(8),
expected_head_sha="abc123", remote="prgs",
)
self.assertTrue(r["performed"])
self.assertEqual(r["merge_commit"], "c9")
self.assertTrue(r["cleanup_status"].startswith("skipped (cleanup error:"))
@@ -773,7 +831,7 @@ class TestMergePR(unittest.TestCase):
env = {"GITEA_PROFILE_NAME": "gitea-merger",
"GITEA_ALLOWED_OPERATIONS": "read,merge"}
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(pr_number=8, confirmation="", remote="prgs")
r = gitea_merge_pr(pr_number=8, confirmation="", expected_head_sha="abc123", remote="prgs")
self.assertFalse(r["performed"])
self.assertTrue(any("explicit confirmation required" in x for x in r["reasons"]))
mock_api.assert_not_called()
@@ -784,7 +842,7 @@ class TestMergePR(unittest.TestCase):
env = {"GITEA_PROFILE_NAME": "gitea-merger",
"GITEA_ALLOWED_OPERATIONS": "read,merge"}
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(pr_number=8, confirmation="MERGE PR 9", remote="prgs")
r = gitea_merge_pr(pr_number=8, confirmation="MERGE PR 9", expected_head_sha="abc123", remote="prgs")
self.assertFalse(r["performed"])
mock_api.assert_not_called()
@@ -918,7 +976,8 @@ class TestMergePR(unittest.TestCase):
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(
pr_number=8, confirmation=self._confirm(8),
expected_changed_files=["a.py", "b.py"], remote="prgs")
expected_changed_files=["a.py", "b.py"],
expected_head_sha="abc123", remote="prgs")
self.assertFalse(r["performed"])
self.assertIn(
"PR changed files do not match expected_changed_files (fail closed)",
@@ -931,7 +990,7 @@ class TestMergePR(unittest.TestCase):
def test_invalid_merge_method_rejected(self, mock_api):
with patch.dict(os.environ, {}, clear=True):
r = gitea_merge_pr(
pr_number=8, confirmation="MERGE PR 8", do="octopus", remote="prgs")
pr_number=8, confirmation="MERGE PR 8", do="octopus", expected_head_sha="abc123", remote="prgs")
self.assertFalse(r["performed"])
self.assertTrue(any("unknown merge method" in x for x in r["reasons"]))
mock_api.assert_not_called()
@@ -949,7 +1008,9 @@ class TestMergePR(unittest.TestCase):
"GITEA_TOKEN": "super-secret-token"}
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(
pr_number=8, confirmation=self._confirm(8), remote="prgs")
pr_number=8, confirmation=self._confirm(8),
expected_head_sha="abc123", remote="prgs",
)
blob = repr(r).lower()
for secret in ("super-secret-token", "authorization", "basic ", FAKE_AUTH.lower()):
self.assertNotIn(secret, blob)
@@ -966,7 +1027,9 @@ class TestMergePR(unittest.TestCase):
"GITEA_ALLOWED_OPERATIONS": "read,merge"}
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(
pr_number=8, confirmation=self._confirm(8), remote="prgs")
pr_number=8, confirmation=self._confirm(8),
expected_head_sha="abc123", remote="prgs",
)
self.assertFalse(r["performed"])
blob = repr(r)
self.assertIn("[REDACTED]", blob)
@@ -1007,7 +1070,9 @@ class TestMergePR(unittest.TestCase):
"GITEA_ALLOWED_OPERATIONS": "read,merge"}
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(
pr_number=8, confirmation=self._confirm(8), remote="prgs")
pr_number=8, confirmation=self._confirm(8),
expected_head_sha="abc123", remote="prgs",
)
self.assertFalse(r["performed"])
self.assertFalse(r.get("approval_visible"))
self.assertTrue(any("no visible APPROVED review" in x for x in r["reasons"]))
@@ -1028,7 +1093,9 @@ class TestMergePR(unittest.TestCase):
"GITEA_ALLOWED_OPERATIONS": "read,merge"}
with patch.dict(os.environ, env, clear=True):
r = gitea_merge_pr(
pr_number=8, confirmation=self._confirm(8), remote="prgs")
pr_number=8, confirmation=self._confirm(8),
expected_head_sha="abc123", remote="prgs",
)
self.assertFalse(r["performed"])
self.assertTrue(r.get("has_blocking_change_requests"))
self.assertTrue(any("REQUEST_CHANGES" in x for x in r["reasons"]))
@@ -1129,16 +1196,18 @@ class TestReviewPR(unittest.TestCase):
"forbidden_operations": [],
"base_url": None,
}
mock_get_all.return_value = [{"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": "abc1234"}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "jcwalker3"}}]
head_sha = FULL_HEAD_SHA
mock_get_all.return_value = [{"number": 1, "title": "PR 1", "state": "open", "head": {"ref": "branch1", "sha": head_sha}, "base": {"ref": "master"}, "mergeable": True, "user": {"login": "jcwalker3"}}]
# mock_api responses: 1) /user (inventory), 2) /user (eligibility), 3) /pulls/1 (eligibility)
mock_api.side_effect = [
{"login": "jcwalker3"}, # /api/v1/user (inventory)
{"login": "jcwalker3"}, # /api/v1/user (submit eligibility)
{"user": {"login": "jcwalker3"}, "state": "open", "head": {"sha": "abc1234"}, "mergeable": True}, # /pulls/1
{"user": {"login": "jcwalker3"}, "state": "open", "head": {"sha": head_sha}, "mergeable": True}, # /pulls/1
]
from mcp_server import init_review_decision_lock
init_review_decision_lock("prgs", "review_pr")
gitea_mark_final_review_decision(1, "approve", remote="prgs")
with patch("mcp_server._list_pr_lease_comments", return_value=[]):
_seed_ready_review_decision(1, "approve", sha=head_sha, remote="prgs")
result = gitea_review_pr(
pr_number=1,
event="APPROVE",
@@ -1794,7 +1863,7 @@ class TestReviewDecisionValidationGate(unittest.TestCase):
"""Block incidental live review mutations during validation."""
PR = 203
SHA = "abc123"
SHA = FULL_HEAD_SHA
def _pr(self, author, sha=SHA):
return {
@@ -1819,6 +1888,18 @@ class TestReviewDecisionValidationGate(unittest.TestCase):
self.addCleanup(self._auth_identity_patch.stop)
self.addCleanup(self._lease_patch.stop)
self.addCleanup(reviewer_pr_lease.clear_session_lease)
self._pr_lease_comments_patch = patch(
"mcp_server._list_pr_lease_comments", return_value=[]
)
self._pr_lease_comments_patch.start()
self._pr_work_lease_patch = patch(
"mcp_server._pr_work_lease_reviewer_block",
return_value=dict(_NO_PR_WORK_LEASE_BLOCK),
)
self._pr_work_lease_patch.start()
self.addCleanup(self._pr_lease_comments_patch.stop)
self.addCleanup(self._pr_work_lease_patch.stop)
def _env(self):
return patch.dict(os.environ, {
@@ -1916,15 +1997,28 @@ class TestSubmitPrReview(unittest.TestCase):
import reviewer_pr_lease
init_review_decision_lock("prgs", "review_pr")
gitea_mark_final_review_decision(8, "approve", remote="prgs")
self._lease_patch = _install_owned_reviewer_lease(8)
self._lease_patch.start()
self._auth_identity_patch = patch(
"mcp_server._authenticated_username", return_value="reviewer-bot"
)
self._auth_identity_patch.start()
self._pr_lease_comments_patch = patch(
"mcp_server._list_pr_lease_comments", return_value=[]
)
self._pr_lease_comments_patch.start()
self._pr_work_lease_patch = patch(
"mcp_server._pr_work_lease_reviewer_block",
return_value=dict(_NO_PR_WORK_LEASE_BLOCK),
)
self._pr_work_lease_patch.start()
gitea_mark_final_review_decision(
8, "approve", remote="prgs", expected_head_sha="abc123",
)
self.addCleanup(self._auth_identity_patch.stop)
self.addCleanup(self._lease_patch.stop)
self.addCleanup(self._pr_lease_comments_patch.stop)
self.addCleanup(self._pr_work_lease_patch.stop)
self.addCleanup(reviewer_pr_lease.clear_session_lease)
def _pr(self, author, state="open", sha="abc123", mergeable=True):
@@ -2085,10 +2179,11 @@ class TestSubmitPrReview(unittest.TestCase):
@patch("mcp_server.api_request")
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
def test_comment_succeeds_when_review_eligible(self, _auth, mock_api):
gitea_mark_final_review_decision(8, "comment", remote="prgs")
mock_api.side_effect = [
{"login": "reviewer-bot"}, self._pr("author-bot"),
{"login": "reviewer-bot"}, self._pr("author-bot"), {"id": 3},
]
gitea_mark_final_review_decision(8, "comment", remote="prgs", expected_head_sha="abc123")
env = {"GITEA_PROFILE_NAME": "gitea-reviewer",
"GITEA_ALLOWED_OPERATIONS": "read,review"}
with patch.dict(os.environ, env, clear=True):
@@ -2104,12 +2199,15 @@ class TestSubmitPrReview(unittest.TestCase):
with patch("mcp_server.get_auth_header", return_value=FAKE_AUTH), \
patch("mcp_server.api_request") as mock_api:
mock_api.side_effect = [
{"login": "jcwalker3"}, self._pr("jcwalker3"),
{"login": "jcwalker3"}, self._pr("jcwalker3"), {"id": 4},
]
env = {"GITEA_PROFILE_NAME": "gitea-reviewer",
"GITEA_ALLOWED_OPERATIONS": "read,review"}
with patch.dict(os.environ, env, clear=True):
gitea_mark_final_review_decision(8, "comment", remote="prgs")
gitea_mark_final_review_decision(
8, "comment", remote="prgs", expected_head_sha="abc123",
)
r = gitea_submit_pr_review(
pr_number=8, action="comment", body="note", remote="prgs",
final_review_decision_ready=True,
@@ -2152,14 +2250,14 @@ class TestSubmitPrReview(unittest.TestCase):
with patch("mcp_server.get_auth_header", return_value=FAKE_AUTH), \
patch("mcp_server.api_request") as mock_api:
mock_api.side_effect = [
{"login": "reviewer-bot"}, self._pr("author-bot", sha="abc123"),
{"login": "reviewer-bot"}, self._pr("author-bot", sha="deadbeef"),
]
env = {"GITEA_PROFILE_NAME": "gitea-reviewer",
"GITEA_ALLOWED_OPERATIONS": "read,review,approve"}
with patch.dict(os.environ, env, clear=True):
r = gitea_submit_pr_review(
pr_number=8, action="approve",
expected_head_sha="deadbeef", remote="prgs",
expected_head_sha="abc123", remote="prgs",
final_review_decision_ready=True,
)
self.assertFalse(r["performed"])
@@ -2210,7 +2308,7 @@ class TestSubmitPrReview(unittest.TestCase):
"GITEA_ALLOWED_OPERATIONS": "read,review,approve",
"GITEA_TOKEN": "super-secret-token"}
with patch.dict(os.environ, env, clear=True):
gitea_mark_final_review_decision(5, "approve", remote="prgs")
gitea_mark_final_review_decision(5, "approve", remote="prgs", expected_head_sha="abc123")
r = gitea_submit_pr_review(
pr_number=5, action="approve", remote="prgs",
final_review_decision_ready=True,
@@ -2230,7 +2328,6 @@ class TestSubmitPrReview(unittest.TestCase):
env = {"GITEA_PROFILE_NAME": "gitea-reviewer",
"GITEA_ALLOWED_OPERATIONS": "read,review,approve"}
with patch.dict(os.environ, env, clear=True):
gitea_mark_final_review_decision(8, "approve", remote="prgs")
r = gitea_submit_pr_review(
pr_number=8, action="approve", remote="prgs",
final_review_decision_ready=True,
@@ -2320,7 +2417,7 @@ class TestSubmitPrReview(unittest.TestCase):
def test_mark_final_decision_rejects_remote_mismatch(self):
init_review_decision_lock("prgs", "review_pr")
r = gitea_mark_final_review_decision(8, "approve", remote="dadeschools")
r = gitea_mark_final_review_decision(8, "approve", remote="dadeschools", expected_head_sha="abc123")
self.assertFalse(r["marked_ready"])
self.assertTrue(any("does not match locked remote" in x for x in r["reasons"]))
@@ -2332,28 +2429,30 @@ class TestSubmitPrReview(unittest.TestCase):
{"id": 42, "state": "APPROVED"},
[_formal_review("reviewer-bot", "APPROVED", review_id=42)],
{"login": "reviewer-bot"}, self._pr("author-bot"),
{"login": "reviewer-bot"}, self._pr("author-bot"),
{"id": 43, "state": "REQUEST_CHANGES"},
[_formal_review("reviewer-bot", "REQUEST_CHANGES", review_id=43)],
]
env = {"GITEA_PROFILE_NAME": "gitea-reviewer",
"GITEA_ALLOWED_OPERATIONS": "read,review,approve,request_changes"}
gitea_mark_final_review_decision(8, "approve", remote="prgs")
with patch.dict(os.environ, env, clear=True):
first = gitea_submit_pr_review(
pr_number=8, action="approve", remote="prgs",
final_review_decision_ready=True,
)
auth = gitea_authorize_review_correction(
prior_review_id=42,
prior_review_state="approve",
reason="operator approved correcting mistaken approve",
operator_authorized=True,
)
_mark_request_changes_ready(remote="prgs")
second = gitea_submit_pr_review(
pr_number=8, action="request_changes", remote="prgs",
final_review_decision_ready=True,
)
with patch("mcp_server.gitea_get_pr_review_feedback",
return_value=dict(_NO_BLOCKER_FEEDBACK)):
with patch.dict(os.environ, env, clear=True):
first = gitea_submit_pr_review(
pr_number=8, action="approve", remote="prgs",
final_review_decision_ready=True,
)
auth = gitea_authorize_review_correction(
prior_review_id=42,
prior_review_state="approve",
reason="operator approved correcting mistaken approve",
operator_authorized=True,
)
_mark_request_changes_ready(remote="prgs")
second = gitea_submit_pr_review(
pr_number=8, action="request_changes", remote="prgs",
final_review_decision_ready=True,
)
self.assertTrue(first["performed"])
self.assertTrue(auth["authorized"])
self.assertTrue(second["performed"])
@@ -2365,7 +2464,7 @@ class TestSubmitPrReview(unittest.TestCase):
"GITEA_ALLOWED_OPERATIONS": "read,review,approve"}
with patch.dict(os.environ, env, clear=True):
# Mark decision for specific remote, org, repo
gitea_mark_final_review_decision(8, "approve", remote="prgs", org="MyOrg", repo="MyRepo")
gitea_mark_final_review_decision(8, "approve", remote="prgs", expected_head_sha="abc123", org="MyOrg", repo="MyRepo")
# Mismatched remote
r = gitea_submit_pr_review(
@@ -2407,6 +2506,11 @@ class TestTrackerHygieneCleanup(unittest.TestCase):
self.mock_audit = patch("gitea_audit.write_event").start()
# gitea.pr.close: closing a PR via gitea_edit_pr is capability-gated (#216).
patch("mcp_server.get_profile", return_value={"profile_name": "test", "allowed_operations": ["read", "merge", "edit", "close", "gitea.pr.close", "gitea.issue.close"], "audit_label": "test", "forbidden_operations": []}).start()
patch("mcp_server._list_pr_lease_comments", return_value=[]).start()
patch(
"mcp_server._pr_work_lease_reviewer_block",
return_value=dict(_NO_PR_WORK_LEASE_BLOCK),
).start()
def tearDown(self):
patch.stopall()
@@ -2453,7 +2557,8 @@ class TestTrackerHygieneCleanup(unittest.TestCase):
def test_merge_pr_with_closes_removes_label(self):
import reviewer_pr_lease
lease_patch = _install_owned_reviewer_lease(1, head_sha="sha123")
head_sha = FULL_HEAD_SHA
lease_patch = _install_owned_reviewer_lease(1, head_sha=head_sha)
lease_patch.start()
self.addCleanup(lease_patch.stop)
self.addCleanup(reviewer_pr_lease.clear_session_lease)
@@ -2462,12 +2567,12 @@ class TestTrackerHygieneCleanup(unittest.TestCase):
if method == "GET" and "/user" in url:
return {"login": "merger"}
if method == "GET" and url.endswith("/reviews"):
return [_formal_review("reviewer", "APPROVED", sha="sha123")]
return [_formal_review("reviewer", "APPROVED", sha=head_sha)]
if method == "GET" and "pulls/1" in url and "/files" not in url:
return {
"user": {"login": "author"},
"state": "open",
"head": {"sha": "sha123", "ref": "feat/my-branch"},
"head": {"sha": head_sha, "ref": "feat/my-branch"},
"base": {"ref": "main"},
"mergeable": True,
"merged_commit_sha": "merge123",
@@ -2487,14 +2592,18 @@ class TestTrackerHygieneCleanup(unittest.TestCase):
return {}
self.mock_api.side_effect = api_side_effect
res = gitea_merge_pr(pr_number=1, confirmation="MERGE PR 1", do="merge")
res = gitea_merge_pr(
pr_number=1, confirmation="MERGE PR 1", do="merge",
expected_head_sha=head_sha,
)
self.assertTrue(res["performed"])
self.assertEqual(res["cleanup_status"].get(123), "released")
def test_merge_pr_with_branch_name_removes_label(self):
import reviewer_pr_lease
lease_patch = _install_owned_reviewer_lease(1, head_sha="sha123")
head_sha = FULL_HEAD_SHA
lease_patch = _install_owned_reviewer_lease(1, head_sha=head_sha)
lease_patch.start()
self.addCleanup(lease_patch.stop)
self.addCleanup(reviewer_pr_lease.clear_session_lease)
@@ -2503,12 +2612,12 @@ class TestTrackerHygieneCleanup(unittest.TestCase):
if method == "GET" and "/user" in url:
return {"login": "merger"}
if method == "GET" and url.endswith("/reviews"):
return [_formal_review("reviewer", "APPROVED", sha="sha123")]
return [_formal_review("reviewer", "APPROVED", sha=head_sha)]
if method == "GET" and "pulls/1" in url and "/files" not in url:
return {
"user": {"login": "author"},
"state": "open",
"head": {"sha": "sha123", "ref": "fix/issue-123-slug"},
"head": {"sha": head_sha, "ref": "fix/issue-123-slug"},
"base": {"ref": "main"},
"mergeable": True,
"merged_commit_sha": "merge123",
@@ -2528,7 +2637,10 @@ class TestTrackerHygieneCleanup(unittest.TestCase):
return {}
self.mock_api.side_effect = api_side_effect
res = gitea_merge_pr(pr_number=1, confirmation="MERGE PR 1", do="merge")
res = gitea_merge_pr(
pr_number=1, confirmation="MERGE PR 1", do="merge",
expected_head_sha=head_sha,
)
self.assertTrue(res["performed"])
self.assertEqual(res["cleanup_status"].get(123), "released")