Prevent foreign open-PR terminals on the durable review-decision lock from blocking fresh formal reviews. Scope correction authorization to the named prior review's PR/head, add read-only diagnosis with exact next_action and durable handoff payloads, require thread-visible correction audits, and cover the PR #688 → #692 recovery sequence in regression tests.
428 lines
15 KiB
Python
428 lines
15 KiB
Python
"""Review-decision-lock recovery / cross-PR isolation (#693).
|
|
|
|
Reproduces the PR #688 → PR #692 formal-review sequence:
|
|
|
|
* durable lock holds REQUEST_CHANGES on open PR #688 @ head A (review_id 425)
|
|
* reviewer leases PR #692 @ head B (6d86db…)
|
|
* mark_final for #692 must succeed without correction unlock
|
|
* cleanup of open #688 terminal remains forbidden (#594)
|
|
* authorize_review_correction for #688 cannot unlock #692
|
|
* eventual #692 APPROVE is unique and head-bound (review_id 426)
|
|
* mark_final is idempotent for same PR/action/head
|
|
* diagnosis returns one exact next_action + durable handoff payload
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import os
|
|
import unittest
|
|
from unittest.mock import patch
|
|
|
|
import mcp_server
|
|
import review_workflow_load
|
|
import stale_review_decision_lock as srdl
|
|
|
|
# PR #688 / #692 incident fixtures (sanitized reconstruction from issue #693)
|
|
HEAD_688 = "c7a444eb4b41cf916fdbd20a4999ffd78af496d0"
|
|
HEAD_692 = "6d86db793bbdfaed16bd54d93171f1dd66f234ca"
|
|
PR_A = 688
|
|
PR_B = 692
|
|
REVIEW_ID_A = 425
|
|
REVIEW_ID_B = 426
|
|
|
|
RC_688 = {
|
|
"pr_number": PR_A,
|
|
"action": "request_changes",
|
|
"review_id": REVIEW_ID_A,
|
|
"review_state": "request_changes",
|
|
"head_sha": HEAD_688,
|
|
}
|
|
APPROVE_692 = {
|
|
"pr_number": PR_B,
|
|
"action": "approve",
|
|
"review_id": REVIEW_ID_B,
|
|
"review_state": "approve",
|
|
"head_sha": HEAD_692,
|
|
}
|
|
|
|
|
|
def _lock(mutations=None, **kwargs):
|
|
from datetime import datetime, timezone
|
|
|
|
now = datetime.now(timezone.utc).isoformat()
|
|
base = {
|
|
"task": "review_pr",
|
|
"remote": "prgs",
|
|
"org": "Scaled-Tech-Consulting",
|
|
"repo": "Gitea-Tools",
|
|
"session_pid": os.getpid(),
|
|
"session_profile": "prgs-reviewer",
|
|
"session_profile_lock": "prgs-reviewer",
|
|
"profile_identity": "prgs-reviewer",
|
|
"final_review_decision_ready": False,
|
|
"ready_pr_number": None,
|
|
"ready_action": None,
|
|
"ready_expected_head_sha": None,
|
|
"ready_remote": None,
|
|
"ready_org": None,
|
|
"ready_repo": None,
|
|
"live_mutations": list(mutations or []),
|
|
"correction_authorized": False,
|
|
"correction_reason": None,
|
|
"correction_pr_number": None,
|
|
"correction_head_sha": None,
|
|
"updated_at": now,
|
|
"recorded_at": now,
|
|
"writer_pid": os.getpid(),
|
|
}
|
|
base.update(kwargs)
|
|
# Ensure TTL fields stay fresh unless the caller is testing expiry.
|
|
if "recorded_at" not in kwargs:
|
|
base["recorded_at"] = now
|
|
if "updated_at" not in kwargs:
|
|
base["updated_at"] = now
|
|
return base
|
|
|
|
|
|
REVIEWER_ENV = {
|
|
"GITEA_PROFILE_NAME": "prgs-reviewer",
|
|
"GITEA_ALLOWED_OPERATIONS": (
|
|
"gitea.read,gitea.pr.review,gitea.pr.approve,"
|
|
"gitea.pr.request_changes,gitea.pr.comment"
|
|
),
|
|
"GITEA_SESSION_PROFILE_LOCK": "prgs-reviewer",
|
|
}
|
|
|
|
REVIEWER_PROFILE = {
|
|
"profile_name": "prgs-reviewer",
|
|
"allowed_operations": [
|
|
"gitea.read",
|
|
"gitea.pr.review",
|
|
"gitea.pr.approve",
|
|
"gitea.pr.request_changes",
|
|
"gitea.pr.comment",
|
|
],
|
|
"forbidden_operations": ["gitea.pr.merge"],
|
|
}
|
|
|
|
|
|
def _seed(mutations=None, **kwargs):
|
|
review_workflow_load.record_review_workflow_load(mcp_server.PROJECT_ROOT)
|
|
mcp_server._save_review_decision_lock(_lock(mutations, **kwargs))
|
|
mcp_server.gitea_load_review_workflow()
|
|
|
|
|
|
def _no_lease():
|
|
return {"block": False, "reasons": [], "mutation_allowed": True}
|
|
|
|
|
|
def _open_pr(number, head):
|
|
return {
|
|
"number": number,
|
|
"state": "open",
|
|
"merged": False,
|
|
"merged_at": None,
|
|
"head": {"sha": head},
|
|
}
|
|
|
|
|
|
class TestCrossPrIsolationPure(unittest.TestCase):
|
|
def test_foreign_terminal_does_not_block_mark_boundary(self):
|
|
lock = _lock([RC_688])
|
|
self.assertFalse(
|
|
srdl.prior_live_mutations_block_boundary(
|
|
lock, pr_number=PR_B, expected_head_sha=HEAD_692
|
|
)
|
|
)
|
|
self.assertTrue(
|
|
srdl.prior_live_mutations_block_boundary(
|
|
lock, pr_number=PR_A, expected_head_sha=HEAD_688
|
|
)
|
|
)
|
|
|
|
def test_terminal_boundary_allows_fresh_decision_on_other_pr(self):
|
|
lock = _lock([RC_688])
|
|
self.assertTrue(
|
|
srdl.terminal_boundary_allows_fresh_decision(
|
|
lock,
|
|
pr_number=PR_B,
|
|
expected_head_sha=HEAD_692,
|
|
operation="mark_ready",
|
|
)
|
|
)
|
|
self.assertFalse(
|
|
srdl.terminal_boundary_allows_fresh_decision(
|
|
lock,
|
|
pr_number=PR_A,
|
|
expected_head_sha=HEAD_688,
|
|
operation="mark_ready",
|
|
)
|
|
)
|
|
|
|
def test_unscoped_correction_does_not_apply(self):
|
|
lock = _lock([RC_688], correction_authorized=True, correction_reason="x")
|
|
# Missing correction_pr_number → fail closed (#693)
|
|
self.assertFalse(
|
|
srdl.correction_applies(lock, pr_number=PR_A, expected_head_sha=HEAD_688)
|
|
)
|
|
|
|
def test_scoped_correction_only_for_named_pr(self):
|
|
lock = _lock(
|
|
[RC_688],
|
|
correction_authorized=True,
|
|
correction_reason="mistake",
|
|
correction_pr_number=PR_A,
|
|
correction_head_sha=HEAD_688,
|
|
)
|
|
self.assertTrue(
|
|
srdl.correction_applies(lock, pr_number=PR_A, expected_head_sha=HEAD_688)
|
|
)
|
|
self.assertFalse(
|
|
srdl.correction_applies(lock, pr_number=PR_B, expected_head_sha=HEAD_692)
|
|
)
|
|
|
|
|
|
class TestClassification(unittest.TestCase):
|
|
def test_foreign_pr_terminal_classification(self):
|
|
lock = _lock([RC_688])
|
|
c = srdl.classify_review_decision_lock(
|
|
lock,
|
|
target_pr_number=PR_B,
|
|
target_head_sha=HEAD_692,
|
|
pr_live=_open_pr(PR_A, HEAD_688),
|
|
active_profile_identity="prgs-reviewer",
|
|
)
|
|
self.assertEqual(c["classification"], srdl.CLASS_FOREIGN_PR_TERMINAL)
|
|
self.assertTrue(c["mark_final_allowed"])
|
|
self.assertFalse(c["correction_eligible"])
|
|
self.assertIn("do not call gitea_authorize_review_correction", c["next_action"])
|
|
|
|
def test_same_head_terminal_classification(self):
|
|
lock = _lock([RC_688])
|
|
c = srdl.classify_review_decision_lock(
|
|
lock,
|
|
target_pr_number=PR_A,
|
|
target_head_sha=HEAD_688,
|
|
pr_live=_open_pr(PR_A, HEAD_688),
|
|
)
|
|
self.assertEqual(c["classification"], srdl.CLASS_TERMINAL_ACTIVE_SAME_HEAD)
|
|
self.assertFalse(c["mark_final_allowed"])
|
|
self.assertTrue(c["correction_eligible"])
|
|
|
|
def test_precise_failure_includes_durable_handoff(self):
|
|
lock = _lock([RC_688])
|
|
c = srdl.classify_review_decision_lock(
|
|
lock, target_pr_number=PR_A, target_head_sha=HEAD_688
|
|
)
|
|
fail = srdl.build_precise_mark_final_failure(c)
|
|
self.assertIn("exact_next_action", fail)
|
|
self.assertIn("durable_issue_handoff", fail)
|
|
self.assertTrue(fail["durable_issue_handoff"]["required"])
|
|
|
|
|
|
class TestPr692Sequence(unittest.TestCase):
|
|
def setUp(self):
|
|
self._env = patch.dict(os.environ, REVIEWER_ENV, clear=False)
|
|
self._env.start()
|
|
self._prof = patch.object(
|
|
mcp_server, "get_profile", return_value=REVIEWER_PROFILE
|
|
)
|
|
self._prof.start()
|
|
|
|
def tearDown(self):
|
|
self._prof.stop()
|
|
self._env.stop()
|
|
mcp_server._save_review_decision_lock(None)
|
|
review_workflow_load.clear_review_workflow_load()
|
|
|
|
def test_mark_final_692_succeeds_with_open_688_terminal(self):
|
|
_seed([RC_688], writer_pid=25883, session_pid=60615)
|
|
with patch("mcp_server._list_pr_lease_comments", return_value=[]), patch(
|
|
"mcp_server._pr_work_lease_reviewer_block", return_value=_no_lease()
|
|
), patch.object(
|
|
mcp_server,
|
|
"gitea_check_pr_eligibility",
|
|
return_value={"head_sha": HEAD_692, "eligible": True},
|
|
):
|
|
result = mcp_server.gitea_mark_final_review_decision(
|
|
pr_number=PR_B,
|
|
action="approve",
|
|
expected_head_sha=HEAD_692,
|
|
remote="prgs",
|
|
org="Scaled-Tech-Consulting",
|
|
repo="Gitea-Tools",
|
|
)
|
|
self.assertTrue(result["marked_ready"], result.get("reasons"))
|
|
lock = mcp_server._load_review_decision_lock()
|
|
self.assertEqual(lock["ready_pr_number"], PR_B)
|
|
self.assertEqual(
|
|
srdl.normalize_head_sha(lock["ready_expected_head_sha"]), HEAD_692
|
|
)
|
|
# Foreign terminal history preserved (non-destructive isolation).
|
|
self.assertEqual(lock["live_mutations"][0]["pr_number"], PR_A)
|
|
|
|
def test_mark_final_idempotent_same_binding(self):
|
|
_seed(
|
|
[RC_688],
|
|
final_review_decision_ready=True,
|
|
ready_pr_number=PR_B,
|
|
ready_action="approve",
|
|
ready_expected_head_sha=HEAD_692,
|
|
ready_remote="prgs",
|
|
ready_org="Scaled-Tech-Consulting",
|
|
ready_repo="Gitea-Tools",
|
|
)
|
|
with patch("mcp_server._list_pr_lease_comments", return_value=[]), patch(
|
|
"mcp_server._pr_work_lease_reviewer_block", return_value=_no_lease()
|
|
), patch.object(
|
|
mcp_server,
|
|
"gitea_check_pr_eligibility",
|
|
return_value={"head_sha": HEAD_692, "eligible": True},
|
|
):
|
|
result = mcp_server.gitea_mark_final_review_decision(
|
|
pr_number=PR_B,
|
|
action="approve",
|
|
expected_head_sha=HEAD_692,
|
|
remote="prgs",
|
|
org="Scaled-Tech-Consulting",
|
|
repo="Gitea-Tools",
|
|
)
|
|
self.assertTrue(result["marked_ready"])
|
|
self.assertTrue(result.get("idempotent"))
|
|
|
|
def test_cleanup_still_forbidden_while_688_open(self):
|
|
_seed([RC_688])
|
|
assessment = srdl.assess_stale_review_decision_lock(
|
|
mcp_server._load_review_decision_lock(),
|
|
pr_live=_open_pr(PR_A, HEAD_688),
|
|
active_profile_identity="prgs-reviewer",
|
|
)
|
|
self.assertFalse(assessment["cleanup_allowed"])
|
|
self.assertFalse(assessment["is_moot"])
|
|
|
|
def test_correction_cannot_unlock_different_pr(self):
|
|
_seed([RC_688])
|
|
r = mcp_server.gitea_authorize_review_correction(
|
|
prior_review_id=REVIEW_ID_A,
|
|
prior_review_state="request_changes",
|
|
reason="try unlock 692",
|
|
operator_authorized=True,
|
|
target_pr_number=PR_B,
|
|
remote="prgs",
|
|
org="Scaled-Tech-Consulting",
|
|
repo="Gitea-Tools",
|
|
post_audit_comment=False,
|
|
)
|
|
self.assertFalse(r["authorized"])
|
|
self.assertTrue(any("different PR" in x for x in r["reasons"]))
|
|
|
|
def test_scoped_correction_for_same_pr_posts_audit(self):
|
|
_seed([RC_688])
|
|
with patch(
|
|
"mcp_server._authenticated_username", return_value="sysadmin"
|
|
), patch(
|
|
"mcp_server.get_auth_header", return_value="Basic dGVzdDp0ZXN0"
|
|
), patch(
|
|
"mcp_server._auth", return_value="Basic dGVzdDp0ZXN0"
|
|
), patch("mcp_server.api_request", return_value={"id": 99901}):
|
|
r = mcp_server.gitea_authorize_review_correction(
|
|
prior_review_id=REVIEW_ID_A,
|
|
prior_review_state="request_changes",
|
|
reason="mistaken RC wording",
|
|
operator_authorized=True,
|
|
target_pr_number=PR_A,
|
|
expected_head_sha=HEAD_688,
|
|
remote="prgs",
|
|
org="Scaled-Tech-Consulting",
|
|
repo="Gitea-Tools",
|
|
post_audit_comment=True,
|
|
)
|
|
self.assertTrue(r["authorized"], r.get("reasons"))
|
|
self.assertEqual(r.get("correction_pr_number"), PR_A)
|
|
self.assertEqual(
|
|
r.get("audit_comment_id"),
|
|
99901,
|
|
r.get("audit_comment_skipped") or r.get("audit_comment_error") or r,
|
|
)
|
|
|
|
def test_diagnose_foreign_terminal_next_action(self):
|
|
_seed([RC_688])
|
|
with patch(
|
|
"mcp_server._authenticated_username", return_value="sysadmin"
|
|
), patch(
|
|
"mcp_server.get_auth_header", return_value="Basic dGVzdDp0ZXN0"
|
|
), patch(
|
|
"mcp_server._auth", return_value="Basic dGVzdDp0ZXN0"
|
|
), patch("mcp_server.api_request", return_value=_open_pr(PR_A, HEAD_688)):
|
|
d = mcp_server.gitea_diagnose_review_decision_lock(
|
|
target_pr_number=PR_B,
|
|
expected_head_sha=HEAD_692,
|
|
remote="prgs",
|
|
org="Scaled-Tech-Consulting",
|
|
repo="Gitea-Tools",
|
|
)
|
|
self.assertTrue(d["success"], d.get("reasons"))
|
|
self.assertEqual(d["classification"], srdl.CLASS_FOREIGN_PR_TERMINAL)
|
|
self.assertTrue(d["mark_final_allowed"])
|
|
self.assertIn("mark_final", d["exact_next_action"])
|
|
self.assertTrue(d["correction_as_generic_unlock_forbidden"])
|
|
|
|
def test_approval_ledger_unique_and_bound_after_sequence(self):
|
|
"""After mark_final + record mutation, ledger binds unique approve to 692 head."""
|
|
_seed([RC_688])
|
|
with patch("mcp_server._list_pr_lease_comments", return_value=[]), patch(
|
|
"mcp_server._pr_work_lease_reviewer_block", return_value=_no_lease()
|
|
), patch.object(
|
|
mcp_server,
|
|
"gitea_check_pr_eligibility",
|
|
return_value={"head_sha": HEAD_692, "eligible": True},
|
|
):
|
|
marked = mcp_server.gitea_mark_final_review_decision(
|
|
pr_number=PR_B,
|
|
action="approve",
|
|
expected_head_sha=HEAD_692,
|
|
remote="prgs",
|
|
org="Scaled-Tech-Consulting",
|
|
repo="Gitea-Tools",
|
|
)
|
|
self.assertTrue(marked["marked_ready"])
|
|
mcp_server.record_live_review_mutation(PR_B, "approve", REVIEW_ID_B)
|
|
lock = mcp_server._load_review_decision_lock()
|
|
terminals = [
|
|
m
|
|
for m in lock["live_mutations"]
|
|
if m.get("action") in srdl.TERMINAL_REVIEW_ACTIONS
|
|
and m.get("pr_number") == PR_B
|
|
]
|
|
self.assertEqual(len(terminals), 1)
|
|
self.assertEqual(terminals[0]["review_id"], REVIEW_ID_B)
|
|
self.assertEqual(
|
|
srdl.normalize_head_sha(terminals[0].get("head_sha")), HEAD_692
|
|
)
|
|
# Same-head duplicate still blocked.
|
|
self.assertTrue(
|
|
srdl.prior_live_mutations_block_boundary(
|
|
lock, pr_number=PR_B, expected_head_sha=HEAD_692
|
|
)
|
|
)
|
|
|
|
|
|
class TestSessionRestartWriterPid(unittest.TestCase):
|
|
def tearDown(self):
|
|
mcp_server._save_review_decision_lock(None)
|
|
review_workflow_load.clear_review_workflow_load()
|
|
|
|
def test_writer_pid_change_does_not_inherit_foreign_block(self):
|
|
# session_pid 60615 origin, writer later 25883 — still foreign isolation.
|
|
_seed([RC_688], session_pid=60615, writer_pid=25883)
|
|
self.assertEqual(
|
|
mcp_server.terminal_review_hard_stop_reasons(
|
|
PR_B, "mark_ready", expected_head_sha=HEAD_692
|
|
),
|
|
[],
|
|
)
|
|
|
|
|
|
if __name__ == "__main__":
|
|
unittest.main()
|