From 9e1d5c56498bce0388075459a66f2248c0991ab5 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Fri, 10 Jul 2026 01:04:37 -0400 Subject: [PATCH] fix: head-scope durable #332 review-decision locks (#620) Prior REQUEST_CHANGES on head A no longer blocks formal re-review on head B of the same open PR. Locks store reviewed head SHA, hard-stop and mark_ready/submit gates allow a fresh decision boundary when the live head differs, and same-head duplicates remain fail-closed. Open-PR lock cleanup stays forbidden (#594); assessment reports locked vs current head and stale_by_head / fresh_review_on_current_head_allowed. Closes #620 --- gitea_mcp_server.py | 132 ++++-- stale_review_decision_lock.py | 200 ++++++++- .../test_head_scoped_review_decision_lock.py | 397 ++++++++++++++++++ 3 files changed, 683 insertions(+), 46 deletions(-) create mode 100644 tests/test_head_scoped_review_decision_lock.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index a0ebcde..636eaea 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -3095,22 +3095,26 @@ def check_review_decision_gate( ) prior = list(lock.get("live_mutations") or []) - if prior and not lock.get("correction_authorized"): - reasons.append( - "live review mutation already recorded in this run; only one live " - "review mutation is allowed unless " - "gitea_authorize_review_correction was invoked (fail closed)" - ) - elif ( - action in _TERMINAL_REVIEW_ACTIONS - and any(m.get("action") in _TERMINAL_REVIEW_ACTIONS for m in prior) - and not lock.get("correction_authorized") + ready_head = lock.get("ready_expected_head_sha") + if stale_review_decision_lock.prior_live_mutations_block_boundary( + lock, pr_number=pr_number, expected_head_sha=ready_head ): - reasons.append( - "terminal review decision already submitted on this PR in this " - "run; blocked unless an operator-approved correction was " - "authorized (fail closed)" - ) + if prior and not lock.get("correction_authorized"): + reasons.append( + "live review mutation already recorded for this PR head in this " + "run; only one live review mutation is allowed per head unless " + "gitea_authorize_review_correction was invoked (fail closed, #620)" + ) + elif ( + action in _TERMINAL_REVIEW_ACTIONS + and any(m.get("action") in _TERMINAL_REVIEW_ACTIONS for m in prior) + and not lock.get("correction_authorized") + ): + reasons.append( + "terminal review decision already submitted for this PR head in " + "this run; blocked unless an operator-approved correction was " + "authorized (fail closed, #620)" + ) return reasons @@ -3118,12 +3122,18 @@ def check_review_decision_gate( def record_live_review_mutation(pr_number: int, action: str, review_id: int | None = None): lock = _load_review_decision_lock() or {} mutations = list(lock.get("live_mutations") or []) - mutations.append({ + head_sha = stale_review_decision_lock.normalize_head_sha( + lock.get("ready_expected_head_sha") + ) + entry = { "pr_number": pr_number, "action": action, "review_id": review_id, "review_state": action, - }) + } + if head_sha: + entry["head_sha"] = head_sha + mutations.append(entry) lock["live_mutations"] = mutations if lock.get("correction_authorized"): lock["correction_authorized"] = False @@ -3131,17 +3141,28 @@ def record_live_review_mutation(pr_number: int, action: str, review_id: int | No _save_review_decision_lock(lock) -def terminal_review_hard_stop_reasons(pr_number: int, operation: str) -> list[str]: - """Session hard-stop after a terminal live review mutation (#332). +def terminal_review_hard_stop_reasons( + pr_number: int, + operation: str, + expected_head_sha: str | None = None, +) -> list[str]: + """Session hard-stop after a terminal live review mutation (#332 / #620). - After a terminal verdict is consumed in this run, the only permitted - continuation is the merge sequence for the same PR that was approved. - A REQUEST_CHANGES (or an approval of a different PR) blocks every - further review/mark-ready/merge mutation. An operator-approved - correction (#211) re-opens the review path only — never a cross-PR - merge. + After a terminal verdict is consumed for a given PR **head**, the only + permitted continuation is the merge sequence for that same approved PR + (merge does not require a new head). A REQUEST_CHANGES (or an approval of + a different PR, or the same head) blocks further review/mark-ready/merge + mutations for that boundary. - *operation* is one of 'merge', 'mark_ready', or 'review'. + #620: when *expected_head_sha* differs from the last terminal's reviewed + head on the **same open PR**, mark_ready / review / resume may proceed so + a fresh formal decision can be recorded for the new head. Historical + mutations are preserved. + + An operator-approved correction (#211) re-opens the review path only — + never a cross-PR merge. + + *operation* is one of 'merge', 'mark_ready', 'review', or 'resume'. Returns [] when the operation may proceed. """ lock = _load_review_decision_lock() @@ -3160,8 +3181,19 @@ def terminal_review_hard_stop_reasons(pr_number: int, operation: str) -> list[st and last.get("pr_number") == pr_number ): return [] - if operation in ("mark_ready", "review") and lock.get("correction_authorized"): + if operation in ("mark_ready", "review", "resume") and lock.get( + "correction_authorized" + ): return [] + # #620: same PR, different reviewed head → fresh decision boundary. + if stale_review_decision_lock.terminal_boundary_allows_fresh_decision( + lock, + pr_number=pr_number, + expected_head_sha=expected_head_sha, + operation=operation, + ): + return [] + locked_head = stale_review_decision_lock.mutation_head_sha(last, lock) if last.get("action") == "approve": guidance = ( f"only the merge sequence for approved PR " @@ -3169,15 +3201,22 @@ def terminal_review_hard_stop_reasons(pr_number: int, operation: str) -> list[st ) else: guidance = "the session must stop and produce a final report" + head_note = ( + f" at head {locked_head[:12]}…" + if locked_head + else "" + ) recovery = ( "if that PR is already merged/closed, use " "gitea_cleanup_stale_review_decision_lock (apply=true) after live-state " - "proof (#594); never delete session-state files by hand" + "proof (#594); if the PR is still open but the head moved, mark/submit " + "with the new expected_head_sha (#620); never delete session-state " + "files by hand" ) return [ "terminal review mutation already consumed in this run " - f"({last.get('action')} on PR #{last.get('pr_number')}); {guidance} " - f"(fail closed, #332); {recovery}" + f"({last.get('action')} on PR #{last.get('pr_number')}{head_note}); " + f"{guidance} (fail closed, #332); {recovery}" ] @@ -3800,18 +3839,11 @@ def gitea_mark_final_review_decision( "reasons": workflow_blockers + ( review_workflow_load.recovery_handoff_without_replay()), } - hard_stop = terminal_review_hard_stop_reasons(pr_number, "mark_ready") + hard_stop = terminal_review_hard_stop_reasons( + pr_number, "mark_ready", expected_head_sha=expected_head_sha + ) if hard_stop: return {"marked_ready": False, "reasons": hard_stop} - if lock.get("live_mutations") and not lock.get("correction_authorized"): - return { - "marked_ready": False, - "reasons": [ - "cannot mark final decision after a live review mutation was " - "already recorded in this run unless an operator-approved " - "correction was authorized" - ], - } if action not in _REVIEW_ACTIONS: return { "marked_ready": False, @@ -3828,6 +3860,17 @@ def gitea_mark_final_review_decision( "decision (fail closed, #399)" ], } + if stale_review_decision_lock.prior_live_mutations_block_boundary( + lock, pr_number=pr_number, expected_head_sha=expected_head_sha + ): + return { + "marked_ready": False, + "reasons": [ + "cannot mark final decision after a live review mutation was " + "already recorded for this PR head unless an operator-approved " + "correction was authorized (fail closed, #620)" + ], + } elig = gitea_check_pr_eligibility( pr_number=pr_number, action="review", @@ -3880,6 +3923,8 @@ def gitea_mark_final_review_decision( "#332)" ], } + # Preserve historical terminal head boundaries before advancing ready_* (#620). + stale_review_decision_lock.backfill_terminal_heads_from_ready(lock) lock["final_review_decision_ready"] = True lock["ready_pr_number"] = pr_number lock["ready_action"] = action @@ -3897,6 +3942,7 @@ def gitea_mark_final_review_decision( "org": org, "repo": repo, "final_review_decision_ready": True, + "head_scoped": True, "reasons": [], } @@ -4063,6 +4109,12 @@ def gitea_cleanup_stale_review_decision_lock( "cleanup_allowed": assessment.get("cleanup_allowed"), "last_terminal_pr": assessment.get("last_terminal_pr"), "last_terminal_action": assessment.get("last_terminal_action"), + "locked_head_sha": assessment.get("locked_head_sha"), + "current_pr_head_sha": assessment.get("current_pr_head_sha"), + "stale_by_head": assessment.get("stale_by_head"), + "fresh_review_on_current_head_allowed": assessment.get( + "fresh_review_on_current_head_allowed" + ), "pr_state": assessment.get("pr_state"), "pr_merged": assessment.get("pr_merged"), "pr_merged_or_closed": assessment.get("pr_merged_or_closed"), diff --git a/stale_review_decision_lock.py b/stale_review_decision_lock.py index 28b6fc9..5f47486 100644 --- a/stale_review_decision_lock.py +++ b/stale_review_decision_lock.py @@ -1,4 +1,4 @@ -"""Stale / moot #332 review-decision lock detection and cleanup policy (#594). +"""Stale / moot #332 review-decision lock detection and cleanup policy (#594/#620). #332 correctly hard-stops a reviewer session after a terminal live review mutation. After #559 those locks are durable on disk, so they can outlive the @@ -6,6 +6,12 @@ work they protect: when the referenced PR is already merged or closed, no same-PR merge sequence remains, yet the durable ledger still blocks unrelated new terminal reviews. +#620 scopes the terminal boundary by **reviewed head SHA**: a prior +REQUEST_CHANGES (or APPROVE) on head A must not block a fresh formal decision +on head B of the **same open PR**. Same-head duplicates remain fail-closed. +Open-PR lock *cleanup* remains forbidden unless the PR is truly merged/closed +(#594); new-head re-review is allowed without deleting the durable lock. + This module is pure policy (no Gitea I/O). The MCP tool ``gitea_cleanup_stale_review_decision_lock`` fetches live PR state, calls these helpers, and only then clears durable state when cleanup is allowed. @@ -23,6 +29,45 @@ from typing import Any TERMINAL_REVIEW_ACTIONS = frozenset({"approve", "request_changes"}) +def normalize_head_sha(value: Any) -> str | None: + """Normalize a git SHA for equality comparison; None if empty/invalid.""" + if value is None: + return None + text = str(value).strip().lower() + if not text: + return None + return text + + +def heads_equal(a: Any, b: Any) -> bool: + na = normalize_head_sha(a) + nb = normalize_head_sha(b) + if not na or not nb: + return False + return na == nb + + +def mutation_head_sha(mutation: dict | None, lock: dict | None = None) -> str | None: + """Head SHA that bounds a live mutation (#620). + + Prefers fields recorded on the mutation itself. For *legacy* mutations + that predate head-scoping, falls back to the lock's + ``ready_expected_head_sha`` only when that ready binding is for the same + PR number (the frozen durable PR #619-style ledger). + """ + if not isinstance(mutation, dict): + return None + for key in ("head_sha", "expected_head_sha", "reviewed_head_sha"): + head = normalize_head_sha(mutation.get(key)) + if head: + return head + if not isinstance(lock, dict): + return None + if lock.get("ready_pr_number") != mutation.get("pr_number"): + return None + return normalize_head_sha(lock.get("ready_expected_head_sha")) + + def last_terminal_mutation(lock: dict | None) -> dict | None: """Return the last terminal live mutation on *lock*, or None.""" if not lock: @@ -35,18 +80,125 @@ def last_terminal_mutation(lock: dict | None) -> dict | None: return terminals[-1] if terminals else None +def prior_live_mutations_block_boundary( + lock: dict | None, + *, + pr_number: int, + expected_head_sha: str | None, +) -> bool: + """True when prior live mutations block a new decision for PR+head (#620). + + Historical terminals on a **different head of the same PR** do not block. + Any prior mutation on another PR, the same head, or without a comparable + head SHA fails closed (blocks). + + Head resolution uses ``mutation_head_sha(m, lock)`` so pre-#620 ledgers + that only stored ``ready_expected_head_sha`` still compare correctly + (PR #619-style). + """ + if not lock: + return False + if lock.get("correction_authorized"): + return False + prior = list(lock.get("live_mutations") or []) + if not prior: + return False + target = normalize_head_sha(expected_head_sha) + for m in prior: + if not isinstance(m, dict): + return True + m_pr = m.get("pr_number") + m_head = mutation_head_sha(m, lock) + if ( + m_pr == pr_number + and target + and m_head + and not heads_equal(m_head, target) + ): + # Same open PR, different reviewed head — historical boundary only. + continue + return True + return False + + +def terminal_boundary_allows_fresh_decision( + lock: dict | None, + *, + pr_number: int, + expected_head_sha: str | None, + operation: str, +) -> bool: + """Whether #332 hard-stop should yield for a new PR+head boundary (#620). + + Only for ``mark_ready`` / ``review`` / ``resume`` on the **same PR** when + the requested head differs from the last terminal's head. Merge is never + reopened by head change (approved head merge path is separate). + """ + if operation not in ("mark_ready", "review", "resume"): + return False + if not lock: + return False + if lock.get("correction_authorized"): + return True + last = last_terminal_mutation(lock) + if last is None: + return True + if last.get("pr_number") != pr_number: + return False + locked_head = mutation_head_sha(last, lock) + target = normalize_head_sha(expected_head_sha) + if not locked_head or not target: + return False + return not heads_equal(locked_head, target) + + +def backfill_terminal_heads_from_ready(lock: dict) -> dict: + """Stamp head_sha onto legacy terminal mutations before overwriting ready_*. + + Called when marking a fresh decision on a new head so historical rows keep + their original boundary after ``ready_expected_head_sha`` advances (#620). + """ + if not isinstance(lock, dict): + return lock + ready_pr = lock.get("ready_pr_number") + ready_head = normalize_head_sha(lock.get("ready_expected_head_sha")) + if ready_pr is None or not ready_head: + return lock + mutations = list(lock.get("live_mutations") or []) + changed = False + for m in mutations: + if not isinstance(m, dict): + continue + if m.get("action") not in TERMINAL_REVIEW_ACTIONS: + continue + if m.get("pr_number") != ready_pr: + continue + if mutation_head_sha(m): + continue + m["head_sha"] = ready_head + changed = True + if changed: + lock["live_mutations"] = mutations + return lock + + def classify_pr_live_state(pr_live: dict | None) -> dict[str, Any]: """Normalize a Gitea PR payload into merge/closed flags.""" pr = pr_live or {} merged = bool(pr.get("merged") or pr.get("merged_at")) state = (pr.get("state") or "").strip().lower() or None closed = state == "closed" + head = pr.get("head") if isinstance(pr.get("head"), dict) else {} + head_sha = normalize_head_sha( + pr.get("head_sha") or pr.get("head_commit_sha") or head.get("sha") + ) return { "pr_state": state, "pr_merged": merged, "pr_merged_or_closed": merged or closed, "merge_commit_sha": pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), + "current_pr_head_sha": head_sha, } @@ -56,6 +208,7 @@ def lock_summary(lock: dict | None) -> dict[str, Any] | None: return None last = last_terminal_mutation(lock) mutations = list(lock.get("live_mutations") or []) + last_head = mutation_head_sha(last, lock) if last else None return { "task": lock.get("task"), "remote": lock.get("remote"), @@ -67,16 +220,21 @@ def lock_summary(lock: dict | None) -> dict[str, Any] | None: "session_profile": lock.get("session_profile"), "ready_pr_number": lock.get("ready_pr_number"), "ready_action": lock.get("ready_action"), + "ready_expected_head_sha": normalize_head_sha( + lock.get("ready_expected_head_sha") + ), "live_mutations_count": len(mutations), "last_terminal": ( { "pr_number": last.get("pr_number"), "action": last.get("action"), "review_id": last.get("review_id"), + "head_sha": last_head, } if last else None ), + "locked_head_sha": last_head, "correction_authorized": bool(lock.get("correction_authorized")), "updated_at": lock.get("updated_at") or lock.get("recorded_at"), } @@ -88,13 +246,16 @@ def assess_stale_review_decision_lock( pr_live: dict | None = None, pr_lookup_error: str | None = None, active_profile_identity: str | None = None, + current_pr_head_sha: str | None = None, ) -> dict[str, Any]: - """Assess whether a durable review-decision lock is stale/moot (#594). + """Assess whether a durable review-decision lock is stale/moot (#594/#620). *pr_live* must be the live Gitea payload for the **last terminal mutation's PR**. When no lock / no terminal mutation exists, cleanup is not applicable. When the PR is still open (or lookup fails), cleanup is - forbidden and #332 hard-stop remains in force. + forbidden (#594). Open PR + different live head reports + ``stale_by_head`` / ``fresh_review_on_current_head_allowed`` (#620) + without enabling cleanup. """ summary = lock_summary(lock) result: dict[str, Any] = { @@ -102,6 +263,10 @@ def assess_stale_review_decision_lock( "lock_summary": summary, "last_terminal_pr": None, "last_terminal_action": None, + "locked_head_sha": None, + "current_pr_head_sha": normalize_head_sha(current_pr_head_sha), + "stale_by_head": False, + "fresh_review_on_current_head_allowed": False, "is_moot": False, "cleanup_allowed": False, "profile_match": True, @@ -141,8 +306,10 @@ def assess_stale_review_decision_lock( pr_number = last.get("pr_number") action = last.get("action") + locked_head = mutation_head_sha(last, lock) result["last_terminal_pr"] = pr_number result["last_terminal_action"] = action + result["locked_head_sha"] = locked_head if pr_number is None: result["reasons"].append( @@ -165,25 +332,46 @@ def assess_stale_review_decision_lock( return result live = classify_pr_live_state(pr_live) + current_head = normalize_head_sha( + current_pr_head_sha or live.get("current_pr_head_sha") + ) result.update( { "pr_state": live["pr_state"], "pr_merged": live["pr_merged"], "pr_merged_or_closed": live["pr_merged_or_closed"], "merge_commit_sha": live["merge_commit_sha"], + "current_pr_head_sha": current_head, } ) if not live["pr_merged_or_closed"]: - result["reasons"].append( - f"terminal review mutation on open PR #{pr_number} is still active " - f"({action}); #332 hard-stop remains — cleanup forbidden (#594)" + stale_by_head = bool( + locked_head and current_head and not heads_equal(locked_head, current_head) ) + result["stale_by_head"] = stale_by_head + # Fresh formal decision is allowed on the new head without cleanup (#620). + result["fresh_review_on_current_head_allowed"] = stale_by_head + if stale_by_head: + result["reasons"].append( + f"terminal {action} on PR #{pr_number} is bound to head " + f"{locked_head[:12]}…; live head is {current_head[:12]}… — " + "fresh formal review on current head is allowed without " + "cleanup (fail closed for cleanup, #620); #332 same-head " + "duplicate protection remains" + ) + else: + result["reasons"].append( + f"terminal review mutation on open PR #{pr_number} is still active " + f"({action}); #332 hard-stop remains — cleanup forbidden (#594)" + ) return result # Merged or closed: lock is moot. Same-PR merge continuation is impossible. result["is_moot"] = True result["cleanup_allowed"] = True + result["stale_by_head"] = False + result["fresh_review_on_current_head_allowed"] = False result["reasons"].append( f"terminal mutation ({action} on PR #{pr_number}) is moot: PR is " f"{'merged' if live['pr_merged'] else 'closed'}; canonical cleanup allowed (#594)" diff --git a/tests/test_head_scoped_review_decision_lock.py b/tests/test_head_scoped_review_decision_lock.py new file mode 100644 index 0000000..c9244c7 --- /dev/null +++ b/tests/test_head_scoped_review_decision_lock.py @@ -0,0 +1,397 @@ +"""Head-scoped #332 review-decision locks (#620). + +Proves: + * REQUEST_CHANGES on head A does not block mark_ready/submit on head B + * APPROVE on head B is allowed after RC on head A (same open PR) + * same-head duplicate terminal remains blocked + * open-PR cleanup remains forbidden; assessment reports stale_by_head + * historical mutations keep their head_sha (preserved ledger) + * PR #619-style: old RC at 036b78e…, current head 2429d9d…, approve allowed +""" + +from __future__ import annotations + +import os +import unittest +from unittest.mock import patch + +import mcp_server +import stale_review_decision_lock as srdl + +HEAD_A = "036b78e31ea5d036452b3a52e0e086b01e0c763f" +HEAD_B = "2429d9d2e826e519eb8eb4ade45987c00838aefb" +HEAD_C = "c" * 40 + + +def _lock(mutations=None, correction=False, ready_pr=None, ready_head=None, ready_action=None): + return { + "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": ready_pr, + "ready_action": ready_action, + "ready_expected_head_sha": ready_head, + "ready_remote": "prgs" if ready_pr else None, + "ready_org": "Scaled-Tech-Consulting" if ready_pr else None, + "ready_repo": "Gitea-Tools" if ready_pr else None, + "live_mutations": list(mutations or []), + "correction_authorized": correction, + "correction_reason": None, + } + + +RC_619_LEGACY = { + "pr_number": 619, + "action": "request_changes", + "review_id": 410, + "review_state": "request_changes", + # no head_sha — pre-#620 durable ledger shape +} +RC_619_HEAD_A = { + "pr_number": 619, + "action": "request_changes", + "review_id": 410, + "review_state": "request_changes", + "head_sha": HEAD_A, +} +APPROVE_619_HEAD_B = { + "pr_number": 619, + "action": "approve", + "review_id": 411, + "review_state": "approve", + "head_sha": HEAD_B, +} + + +def _seed(mutations=None, **kwargs): + import review_workflow_load + + 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 _open_pr(pr_number=619, head=HEAD_B): + return { + "number": pr_number, + "state": "open", + "merged": False, + "merged_at": None, + "head": {"sha": head}, + } + + +def _no_lease(): + return {"block": False, "reasons": [], "mutation_allowed": True} + + +def _feedback(blocking=False, stale=False, success=True): + return { + "success": success, + "has_blocking_change_requests": blocking, + "review_feedback_stale": stale, + "current_head_sha": HEAD_B, + } + + +class TestPureHeadScopeHelpers(unittest.TestCase): + def test_mutation_head_prefers_recorded_sha(self): + m = {"pr_number": 1, "head_sha": HEAD_A} + self.assertEqual(srdl.mutation_head_sha(m), HEAD_A) + + def test_legacy_mutation_uses_ready_expected_for_same_pr(self): + lock = _lock( + [RC_619_LEGACY], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + self.assertEqual(srdl.mutation_head_sha(RC_619_LEGACY, lock), HEAD_A) + + def test_legacy_mutation_ignores_ready_for_other_pr(self): + lock = _lock([RC_619_LEGACY], ready_pr=1, ready_head=HEAD_A) + self.assertIsNone(srdl.mutation_head_sha(RC_619_LEGACY, lock)) + + def test_prior_blocks_same_head_not_other_head(self): + lock = _lock([RC_619_HEAD_A]) + self.assertTrue( + srdl.prior_live_mutations_block_boundary( + lock, pr_number=619, expected_head_sha=HEAD_A + ) + ) + self.assertFalse( + srdl.prior_live_mutations_block_boundary( + lock, pr_number=619, expected_head_sha=HEAD_B + ) + ) + + def test_backfill_stamps_legacy_terminals(self): + lock = _lock( + [dict(RC_619_LEGACY)], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + srdl.backfill_terminal_heads_from_ready(lock) + self.assertEqual(lock["live_mutations"][0]["head_sha"], HEAD_A) + + +class TestHardStopHeadScope(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + mcp_server.review_workflow_load.clear_review_workflow_load() + + def test_rc_head_a_allows_mark_ready_head_b(self): + _seed( + [RC_619_HEAD_A], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons( + 619, "mark_ready", expected_head_sha=HEAD_B + ), + [], + ) + + def test_rc_head_a_blocks_mark_ready_same_head(self): + _seed( + [RC_619_HEAD_A], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + reasons = mcp_server.terminal_review_hard_stop_reasons( + 619, "mark_ready", expected_head_sha=HEAD_A + ) + self.assertTrue(reasons) + self.assertIn("#332", reasons[0]) + + def test_rc_head_a_blocks_other_pr(self): + _seed([RC_619_HEAD_A], ready_pr=619, ready_head=HEAD_A) + reasons = mcp_server.terminal_review_hard_stop_reasons( + 700, "mark_ready", expected_head_sha=HEAD_B + ) + self.assertTrue(reasons) + + def test_legacy_619_ledger_allows_new_head(self): + """PR #619-style durable lock without mutation head_sha.""" + _seed( + [RC_619_LEGACY], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons( + 619, "mark_ready", expected_head_sha=HEAD_B + ), + [], + ) + + +class TestMarkFinalAndRecord(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + mcp_server.review_workflow_load.clear_review_workflow_load() + + def _mark(self, pr, action, head, feedback=None): + 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_get_pr_review_feedback", + return_value=feedback or _feedback(blocking=False, stale=True), + ), patch.object( + mcp_server, + "gitea_check_pr_eligibility", + return_value={"eligible": True, "head_sha": head}, + ): + return mcp_server.gitea_mark_final_review_decision( + pr_number=pr, + action=action, + expected_head_sha=head, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + + def test_rc_then_rc_on_new_head(self): + _seed( + [RC_619_HEAD_A], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + # Prior RC at head A is unresolved but head moved → feedback stale. + res = self._mark( + 619, + "request_changes", + HEAD_B, + feedback=_feedback(blocking=True, stale=True), + ) + self.assertTrue(res.get("marked_ready"), res) + lock = mcp_server._load_review_decision_lock() + # Historical head A preserved on mutation after backfill. + heads = { + m.get("head_sha") + for m in lock["live_mutations"] + if m.get("action") == "request_changes" + } + self.assertIn(HEAD_A, heads) + self.assertEqual(lock["ready_expected_head_sha"], HEAD_B) + + def test_rc_then_approve_on_new_head(self): + _seed( + [RC_619_HEAD_A], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + res = self._mark(619, "approve", HEAD_B) + self.assertTrue(res.get("marked_ready"), res) + self.assertTrue(res.get("head_scoped")) + + def test_same_head_rc_still_blocked_by_hard_stop(self): + _seed( + [RC_619_HEAD_A], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + res = self._mark(619, "approve", HEAD_A) + self.assertFalse(res.get("marked_ready")) + self.assertTrue(any("#332" in r for r in res.get("reasons") or [])) + + def test_pr619_style_legacy_lock_approve_on_current_head(self): + _seed( + [RC_619_LEGACY], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + res = self._mark(619, "approve", HEAD_B) + self.assertTrue(res.get("marked_ready"), res) + lock = mcp_server._load_review_decision_lock() + # Backfill should have stamped HEAD_A onto the legacy mutation. + self.assertEqual(lock["live_mutations"][0].get("head_sha"), HEAD_A) + self.assertEqual(lock["ready_expected_head_sha"], HEAD_B) + + def test_record_live_mutation_stores_head(self): + _seed(ready_pr=619, ready_head=HEAD_B, ready_action="approve") + lock = mcp_server._load_review_decision_lock() + lock["final_review_decision_ready"] = True + lock["ready_pr_number"] = 619 + lock["ready_expected_head_sha"] = HEAD_B + mcp_server._save_review_decision_lock(lock) + mcp_server.record_live_review_mutation(619, "approve", review_id=99) + stored = mcp_server._load_review_decision_lock()["live_mutations"][-1] + self.assertEqual(stored["head_sha"], HEAD_B) + self.assertEqual(stored["pr_number"], 619) + + def test_submit_gate_allows_new_head_after_prior_terminal(self): + """check_review_decision_gate must not block different-head submit.""" + mutations = [RC_619_HEAD_A] + _seed( + mutations, + ready_pr=619, + ready_head=HEAD_B, + ready_action="approve", + ) + lock = mcp_server._load_review_decision_lock() + lock["final_review_decision_ready"] = True + lock["ready_pr_number"] = 619 + lock["ready_action"] = "approve" + lock["ready_expected_head_sha"] = HEAD_B + lock["ready_remote"] = "prgs" + lock["ready_org"] = "Scaled-Tech-Consulting" + lock["ready_repo"] = "Gitea-Tools" + mcp_server._save_review_decision_lock(lock) + reasons = mcp_server.check_review_decision_gate( + 619, + "approve", + final_review_decision_ready=True, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertEqual(reasons, [], reasons) + + def test_submit_gate_blocks_same_head_duplicate(self): + mutations = [RC_619_HEAD_A] + _seed( + mutations, + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + lock = mcp_server._load_review_decision_lock() + lock["final_review_decision_ready"] = True + lock["ready_pr_number"] = 619 + lock["ready_action"] = "request_changes" + lock["ready_expected_head_sha"] = HEAD_A + lock["ready_remote"] = "prgs" + lock["ready_org"] = "Scaled-Tech-Consulting" + lock["ready_repo"] = "Gitea-Tools" + mcp_server._save_review_decision_lock(lock) + reasons = mcp_server.check_review_decision_gate( + 619, + "request_changes", + final_review_decision_ready=True, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(reasons) + + +class TestAssessmentOpenPrHead(unittest.TestCase): + def test_open_pr_stale_by_head_no_cleanup(self): + lock = _lock( + [RC_619_HEAD_A], + ready_pr=619, + ready_head=HEAD_A, + ready_action="request_changes", + ) + a = srdl.assess_stale_review_decision_lock( + lock, pr_live=_open_pr(619, HEAD_B) + ) + self.assertTrue(a["has_lock"]) + self.assertFalse(a["is_moot"]) + self.assertFalse(a["cleanup_allowed"]) + self.assertTrue(a["stale_by_head"]) + self.assertTrue(a["fresh_review_on_current_head_allowed"]) + self.assertEqual(a["locked_head_sha"], HEAD_A) + self.assertEqual(a["current_pr_head_sha"], HEAD_B) + + def test_open_pr_same_head_no_fresh(self): + lock = _lock([RC_619_HEAD_A], ready_pr=619, ready_head=HEAD_A) + a = srdl.assess_stale_review_decision_lock( + lock, pr_live=_open_pr(619, HEAD_A) + ) + self.assertFalse(a["stale_by_head"]) + self.assertFalse(a["fresh_review_on_current_head_allowed"]) + self.assertFalse(a["cleanup_allowed"]) + + def test_historical_mutation_preserved_in_summary(self): + lock = _lock( + [RC_619_HEAD_A, APPROVE_619_HEAD_B], + ready_pr=619, + ready_head=HEAD_B, + ready_action="approve", + ) + summary = srdl.lock_summary(lock) + self.assertEqual(summary["live_mutations_count"], 2) + self.assertEqual(summary["last_terminal"]["head_sha"], HEAD_B) + self.assertEqual(summary["locked_head_sha"], HEAD_B) + + +if __name__ == "__main__": + unittest.main()