fix(workflow): cross-profile decision-lock cleanup and irrecoverable provenance (#709)
Prevent merger-local empty decision locks from standing in for reviewer terminal cleanup, refuse silent re-init overwrite of unresolved terminal evidence, record post-merge recovery-required state when audit fails, and add a truthful irrecoverable-provenance path that never claims applied=true. Closes #709
This commit is contained in:
@@ -0,0 +1,523 @@
|
||||
"""#709: cross-profile decision-lock cleanup, overwrite protection, recovery.
|
||||
|
||||
Covers AC1–AC8 regression scenarios without fabricating historical PR #696
|
||||
provenance or special-casing live PR numbers in production code.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import tempfile
|
||||
import unittest
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import mcp_session_state as ss
|
||||
import stale_review_decision_lock as srdl
|
||||
|
||||
|
||||
def _lock(
|
||||
mutations=None,
|
||||
*,
|
||||
profile="prgs-reviewer",
|
||||
remote="prgs",
|
||||
head=None,
|
||||
):
|
||||
muts = []
|
||||
for m in mutations or []:
|
||||
row = dict(m)
|
||||
if head and "head_sha" not in row:
|
||||
row["head_sha"] = head
|
||||
muts.append(row)
|
||||
return {
|
||||
"task": "review_pr",
|
||||
"remote": remote,
|
||||
"org": "Scaled-Tech-Consulting",
|
||||
"repo": "Gitea-Tools",
|
||||
"session_pid": os.getpid(),
|
||||
"session_profile": profile,
|
||||
"session_profile_lock": profile,
|
||||
"profile_identity": profile,
|
||||
"final_review_decision_ready": False,
|
||||
"ready_pr_number": None,
|
||||
"ready_action": None,
|
||||
"ready_expected_head_sha": None,
|
||||
"live_mutations": muts,
|
||||
"correction_authorized": False,
|
||||
"correction_reason": None,
|
||||
"kind": ss.KIND_DECISION_LOCK,
|
||||
}
|
||||
|
||||
|
||||
APPROVE = {"pr_number": 100, "action": "approve", "review_id": 9}
|
||||
APPROVE_OTHER = {"pr_number": 200, "action": "approve", "review_id": 10}
|
||||
HEAD_A = "a" * 40
|
||||
HEAD_B = "b" * 40
|
||||
|
||||
|
||||
class TestAC2InitOverwrite(unittest.TestCase):
|
||||
def test_empty_lock_allows_reinit(self):
|
||||
a = srdl.assess_init_overwrite(_lock([]), force=True)
|
||||
self.assertTrue(a["overwrite_allowed"])
|
||||
|
||||
def test_terminal_lock_blocks_force_reinit(self):
|
||||
a = srdl.assess_init_overwrite(_lock([APPROVE]), force=True)
|
||||
self.assertFalse(a["overwrite_allowed"])
|
||||
self.assertTrue(a["has_unresolved_terminal"])
|
||||
self.assertEqual(a["last_terminal_pr"], 100)
|
||||
|
||||
def test_none_lock_allows_init(self):
|
||||
a = srdl.assess_init_overwrite(None)
|
||||
self.assertTrue(a["overwrite_allowed"])
|
||||
|
||||
|
||||
class TestAC1TargetApproval(unittest.TestCase):
|
||||
def test_targets_matching_approve(self):
|
||||
self.assertTrue(
|
||||
srdl.lock_targets_merged_pr_approval(
|
||||
_lock([APPROVE], head=HEAD_A),
|
||||
pr_number=100,
|
||||
expected_head_sha=HEAD_A,
|
||||
)
|
||||
)
|
||||
|
||||
def test_rejects_other_pr(self):
|
||||
self.assertFalse(
|
||||
srdl.lock_targets_merged_pr_approval(
|
||||
_lock([APPROVE_OTHER]), pr_number=100
|
||||
)
|
||||
)
|
||||
|
||||
def test_rejects_head_mismatch(self):
|
||||
self.assertFalse(
|
||||
srdl.lock_targets_merged_pr_approval(
|
||||
_lock([APPROVE], head=HEAD_A),
|
||||
pr_number=100,
|
||||
expected_head_sha=HEAD_B,
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
class TestAC5IrrecoverableRecord(unittest.TestCase):
|
||||
def test_never_sets_applied_true(self):
|
||||
rec = srdl.build_irrecoverable_provenance_record(
|
||||
pr_number=42,
|
||||
head_sha=HEAD_A,
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
actor_username="sysadmin",
|
||||
profile_name="prgs-reconciler",
|
||||
reason="evidence destroyed",
|
||||
incident_ref="#700 comment 1",
|
||||
operator_authorized=True,
|
||||
)
|
||||
self.assertFalse(rec["applied"])
|
||||
self.assertFalse(rec["historical_cleanup_proven"])
|
||||
self.assertEqual(rec["status"], "provenance_irrecoverable")
|
||||
self.assertTrue(rec["merger_may_accept"])
|
||||
body = srdl.format_irrecoverable_audit_comment(rec)
|
||||
self.assertIn("applied: `False`", body)
|
||||
self.assertIn("must remain false", body)
|
||||
|
||||
def test_unauthorized_not_merger_acceptable(self):
|
||||
rec = srdl.build_irrecoverable_provenance_record(
|
||||
pr_number=42,
|
||||
head_sha=HEAD_A,
|
||||
remote="prgs",
|
||||
org=None,
|
||||
repo=None,
|
||||
actor_username="x",
|
||||
profile_name="y",
|
||||
reason="r",
|
||||
incident_ref=None,
|
||||
operator_authorized=False,
|
||||
)
|
||||
self.assertFalse(rec["merger_may_accept"])
|
||||
|
||||
|
||||
class TestAC3PostMergeRecoveryRecord(unittest.TestCase):
|
||||
def test_recovery_record_is_not_applied_cleanup(self):
|
||||
rec = srdl.build_post_merge_recovery_record(
|
||||
pr_number=10,
|
||||
head_sha=HEAD_A,
|
||||
merge_commit_sha="m" * 40,
|
||||
target_profile_identity="prgs-reviewer",
|
||||
failed_step="audit_comment_publish",
|
||||
error="timeout",
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
actor_username="sysadmin",
|
||||
profile_name="prgs-merger",
|
||||
)
|
||||
self.assertEqual(rec["status"], "recovery_required")
|
||||
self.assertFalse(rec["applied"])
|
||||
self.assertTrue(rec["recovery_critical"])
|
||||
|
||||
|
||||
class TestSessionStateCrossProfile(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.state_dir = self._tmp.name
|
||||
os.chmod(self.state_dir, 0o700)
|
||||
|
||||
def tearDown(self):
|
||||
self._tmp.cleanup()
|
||||
|
||||
def test_list_and_load_foreign_profile_lock(self):
|
||||
ss.save_state(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
payload=_lock([APPROVE], profile="prgs-reviewer"),
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
profile_identity="prgs-reviewer",
|
||||
state_dir=self.state_dir,
|
||||
)
|
||||
# Merger-local empty lock
|
||||
ss.save_state(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
payload=_lock([], profile="prgs-merger"),
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
profile_identity="prgs-merger",
|
||||
state_dir=self.state_dir,
|
||||
)
|
||||
ids = ss.list_decision_lock_profile_identities(state_dir=self.state_dir)
|
||||
self.assertIn("prgs-reviewer", ids)
|
||||
self.assertIn("prgs-merger", ids)
|
||||
|
||||
foreign = ss.load_state_for_profile(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
profile_identity="prgs-reviewer",
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
state_dir=self.state_dir,
|
||||
skip_identity_match=True,
|
||||
)
|
||||
self.assertIsNotNone(foreign)
|
||||
self.assertTrue(
|
||||
srdl.lock_targets_merged_pr_approval(foreign, pr_number=100)
|
||||
)
|
||||
empty = ss.load_state_for_profile(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
profile_identity="prgs-merger",
|
||||
state_dir=self.state_dir,
|
||||
skip_identity_match=True,
|
||||
)
|
||||
self.assertIsNotNone(empty)
|
||||
self.assertFalse(
|
||||
srdl.lock_targets_merged_pr_approval(empty, pr_number=100)
|
||||
)
|
||||
|
||||
def test_clear_reviewer_not_merger_empty(self):
|
||||
ss.save_state(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
payload=_lock([APPROVE], profile="prgs-reviewer"),
|
||||
profile_identity="prgs-reviewer",
|
||||
state_dir=self.state_dir,
|
||||
)
|
||||
ss.save_state(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
payload=_lock([], profile="prgs-merger"),
|
||||
profile_identity="prgs-merger",
|
||||
state_dir=self.state_dir,
|
||||
)
|
||||
ss.clear_state(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
profile_identity="prgs-reviewer",
|
||||
state_dir=self.state_dir,
|
||||
)
|
||||
self.assertIsNone(
|
||||
ss.load_state_for_profile(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
profile_identity="prgs-reviewer",
|
||||
state_dir=self.state_dir,
|
||||
skip_identity_match=True,
|
||||
)
|
||||
)
|
||||
# Merger empty lock remains
|
||||
self.assertIsNotNone(
|
||||
ss.load_state_for_profile(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
profile_identity="prgs-merger",
|
||||
state_dir=self.state_dir,
|
||||
skip_identity_match=True,
|
||||
)
|
||||
)
|
||||
|
||||
def test_recovery_critical_kinds_ttl_exempt(self):
|
||||
rec = srdl.build_irrecoverable_provenance_record(
|
||||
pr_number=1,
|
||||
head_sha=HEAD_A,
|
||||
remote="prgs",
|
||||
org=None,
|
||||
repo=None,
|
||||
actor_username="a",
|
||||
profile_name="p",
|
||||
reason="gone",
|
||||
incident_ref=None,
|
||||
operator_authorized=True,
|
||||
)
|
||||
rec["kind"] = ss.KIND_IRRECOVERABLE_DECISION_PROVENANCE
|
||||
# Force old recorded_at
|
||||
rec["recorded_at"] = "2000-01-01T00:00:00Z"
|
||||
rec["updated_at"] = rec["recorded_at"]
|
||||
rec["profile_identity"] = "prgs-reconciler"
|
||||
rec["session_profile_lock"] = "prgs-reconciler"
|
||||
reasons = ss.identity_match_reasons(
|
||||
rec, profile_identity="prgs-reconciler"
|
||||
)
|
||||
self.assertFalse(
|
||||
any("expired" in r for r in reasons),
|
||||
msg=reasons,
|
||||
)
|
||||
|
||||
|
||||
class TestInitReviewDecisionLockIntegration(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.env = patch.dict(
|
||||
os.environ,
|
||||
{
|
||||
"GITEA_MCP_SESSION_STATE_DIR": self._tmp.name,
|
||||
"GITEA_SESSION_PROFILE_LOCK": "prgs-reviewer",
|
||||
"GITEA_PROFILE_NAME": "prgs-reviewer",
|
||||
},
|
||||
clear=False,
|
||||
)
|
||||
self.env.start()
|
||||
import mcp_server
|
||||
|
||||
self.mcp = mcp_server
|
||||
self.mcp._REVIEW_DECISION_LOCK = None
|
||||
|
||||
def tearDown(self):
|
||||
self.mcp._REVIEW_DECISION_LOCK = None
|
||||
self.env.stop()
|
||||
self._tmp.cleanup()
|
||||
|
||||
def test_init_does_not_wipe_terminal_ledger(self):
|
||||
self.mcp._save_review_decision_lock(_lock([APPROVE], profile="prgs-reviewer"))
|
||||
# force=True would previously wipe
|
||||
self.mcp.init_review_decision_lock("prgs", "review_pr", force=True)
|
||||
loaded = self.mcp._load_review_decision_lock()
|
||||
self.assertIsNotNone(loaded)
|
||||
last = srdl.last_terminal_mutation(loaded)
|
||||
self.assertIsNotNone(last)
|
||||
self.assertEqual(last.get("pr_number"), 100)
|
||||
|
||||
def test_init_creates_empty_when_no_terminal(self):
|
||||
self.mcp._save_review_decision_lock(None)
|
||||
self.mcp.init_review_decision_lock("prgs", "review_pr", force=True)
|
||||
loaded = self.mcp._load_review_decision_lock()
|
||||
self.assertIsNotNone(loaded)
|
||||
self.assertEqual(loaded.get("live_mutations"), [])
|
||||
|
||||
|
||||
class TestIrrecoverableTool(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.env = patch.dict(
|
||||
os.environ,
|
||||
{
|
||||
"GITEA_MCP_SESSION_STATE_DIR": self._tmp.name,
|
||||
"GITEA_SESSION_PROFILE_LOCK": "prgs-reconciler",
|
||||
"GITEA_PROFILE_NAME": "prgs-reconciler",
|
||||
"GITEA_ALLOWED_OPERATIONS": "gitea.read,gitea.issue.comment,gitea.pr.comment",
|
||||
},
|
||||
clear=False,
|
||||
)
|
||||
self.env.start()
|
||||
import mcp_server
|
||||
|
||||
self.mcp = mcp_server
|
||||
|
||||
def tearDown(self):
|
||||
self.env.stop()
|
||||
self._tmp.cleanup()
|
||||
|
||||
def test_requires_confirmation_and_operator(self):
|
||||
with patch.object(
|
||||
self.mcp,
|
||||
"get_profile",
|
||||
return_value={
|
||||
"profile_name": "prgs-reconciler",
|
||||
"allowed_operations": [
|
||||
"gitea.read",
|
||||
"gitea.issue.comment",
|
||||
"gitea.pr.comment",
|
||||
],
|
||||
"forbidden_operations": [],
|
||||
},
|
||||
):
|
||||
r = self.mcp.gitea_record_irrecoverable_decision_lock_provenance(
|
||||
pr_number=50,
|
||||
reason="lost",
|
||||
confirmation="",
|
||||
operator_authorized=False,
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
post_audit_comment=False,
|
||||
)
|
||||
self.assertFalse(r["success"])
|
||||
self.assertFalse(r["applied"])
|
||||
|
||||
def test_records_without_applied_true(self):
|
||||
with patch.object(
|
||||
self.mcp,
|
||||
"get_profile",
|
||||
return_value={
|
||||
"profile_name": "prgs-reconciler",
|
||||
"allowed_operations": [
|
||||
"gitea.read",
|
||||
"gitea.issue.comment",
|
||||
"gitea.pr.comment",
|
||||
],
|
||||
"forbidden_operations": [],
|
||||
},
|
||||
), patch.object(
|
||||
self.mcp, "_authenticated_username", return_value="sysadmin"
|
||||
), patch.object(
|
||||
self.mcp, "_profile_operation_gate", return_value=None
|
||||
), patch.object(
|
||||
self.mcp, "_resolve", return_value=("h", "Scaled-Tech-Consulting", "Gitea-Tools")
|
||||
):
|
||||
r = self.mcp.gitea_record_irrecoverable_decision_lock_provenance(
|
||||
pr_number=50,
|
||||
reason="terminal evidence overwritten",
|
||||
confirmation="IRRECOVERABLE DECISION PROVENANCE PR 50",
|
||||
operator_authorized=True,
|
||||
expected_head_sha=HEAD_A,
|
||||
incident_ref="issue-700-comment-11489",
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
post_audit_comment=False,
|
||||
)
|
||||
self.assertTrue(r["success"])
|
||||
self.assertFalse(r["applied"])
|
||||
self.assertFalse(r["historical_cleanup_proven"])
|
||||
self.assertEqual(r["record"]["status"], "provenance_irrecoverable")
|
||||
|
||||
# Idempotent replay
|
||||
with patch.object(
|
||||
self.mcp,
|
||||
"get_profile",
|
||||
return_value={
|
||||
"profile_name": "prgs-reconciler",
|
||||
"allowed_operations": [
|
||||
"gitea.read",
|
||||
"gitea.issue.comment",
|
||||
"gitea.pr.comment",
|
||||
],
|
||||
"forbidden_operations": [],
|
||||
},
|
||||
), patch.object(
|
||||
self.mcp, "_authenticated_username", return_value="sysadmin"
|
||||
), patch.object(
|
||||
self.mcp, "_profile_operation_gate", return_value=None
|
||||
), patch.object(
|
||||
self.mcp, "_resolve", return_value=("h", "Scaled-Tech-Consulting", "Gitea-Tools")
|
||||
):
|
||||
r2 = self.mcp.gitea_record_irrecoverable_decision_lock_provenance(
|
||||
pr_number=50,
|
||||
reason="terminal evidence overwritten",
|
||||
confirmation="IRRECOVERABLE DECISION PROVENANCE PR 50",
|
||||
operator_authorized=True,
|
||||
expected_head_sha=HEAD_A,
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
post_audit_comment=False,
|
||||
)
|
||||
self.assertTrue(r2["success"])
|
||||
self.assertFalse(r2["performed"]) # idempotent hit
|
||||
|
||||
def test_wrong_confirmation_cannot_unblock_other_pr(self):
|
||||
with patch.object(
|
||||
self.mcp,
|
||||
"get_profile",
|
||||
return_value={
|
||||
"profile_name": "prgs-reconciler",
|
||||
"allowed_operations": ["gitea.read", "gitea.issue.comment"],
|
||||
"forbidden_operations": [],
|
||||
},
|
||||
), patch.object(
|
||||
self.mcp, "_authenticated_username", return_value="sysadmin"
|
||||
), patch.object(
|
||||
self.mcp, "_profile_operation_gate", return_value=None
|
||||
), patch.object(
|
||||
self.mcp, "_resolve", return_value=("h", "o", "r")
|
||||
):
|
||||
r = self.mcp.gitea_record_irrecoverable_decision_lock_provenance(
|
||||
pr_number=50,
|
||||
reason="x",
|
||||
confirmation="IRRECOVERABLE DECISION PROVENANCE PR 51",
|
||||
operator_authorized=True,
|
||||
remote="prgs",
|
||||
post_audit_comment=False,
|
||||
)
|
||||
self.assertFalse(r["success"])
|
||||
|
||||
|
||||
class TestClearProfileHelper(unittest.TestCase):
|
||||
def setUp(self):
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.env = patch.dict(
|
||||
os.environ,
|
||||
{
|
||||
"GITEA_MCP_SESSION_STATE_DIR": self._tmp.name,
|
||||
"GITEA_SESSION_PROFILE_LOCK": "prgs-merger",
|
||||
},
|
||||
clear=False,
|
||||
)
|
||||
self.env.start()
|
||||
import mcp_server
|
||||
|
||||
self.mcp = mcp_server
|
||||
self.mcp._REVIEW_DECISION_LOCK = None
|
||||
|
||||
def tearDown(self):
|
||||
self.mcp._REVIEW_DECISION_LOCK = None
|
||||
self.env.stop()
|
||||
self._tmp.cleanup()
|
||||
|
||||
def test_clear_only_matching_reviewer_approve(self):
|
||||
ss.save_state(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
payload=_lock([APPROVE], profile="prgs-reviewer", head=HEAD_A),
|
||||
profile_identity="prgs-reviewer",
|
||||
state_dir=self._tmp.name,
|
||||
)
|
||||
ss.save_state(
|
||||
kind=ss.KIND_DECISION_LOCK,
|
||||
payload=_lock([], profile="prgs-merger"),
|
||||
profile_identity="prgs-merger",
|
||||
state_dir=self._tmp.name,
|
||||
)
|
||||
out = self.mcp._clear_decision_lock_for_profile(
|
||||
profile_identity="prgs-reviewer",
|
||||
pr_number=100,
|
||||
expected_head_sha=HEAD_A,
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
)
|
||||
self.assertTrue(out["cleared"])
|
||||
skip = self.mcp._clear_decision_lock_for_profile(
|
||||
profile_identity="prgs-merger",
|
||||
pr_number=100,
|
||||
expected_head_sha=HEAD_A,
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
)
|
||||
self.assertFalse(skip["cleared"])
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user