Merge pull request 'fix: head-scope durable #332 review-decision locks (Closes #620)' (#621) from fix/issue-620-head-scoped-review-locks into master

Reviewed-on: #621
This commit was merged in pull request #621.
This commit is contained in:
2026-07-10 01:12:57 -05:00
3 changed files with 683 additions and 46 deletions
+83 -31
View File
@@ -3095,11 +3095,15 @@ def check_review_decision_gate(
) )
prior = list(lock.get("live_mutations") or []) prior = list(lock.get("live_mutations") or [])
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
):
if prior and not lock.get("correction_authorized"): if prior and not lock.get("correction_authorized"):
reasons.append( reasons.append(
"live review mutation already recorded in this run; only one live " "live review mutation already recorded for this PR head in this "
"review mutation is allowed unless " "run; only one live review mutation is allowed per head unless "
"gitea_authorize_review_correction was invoked (fail closed)" "gitea_authorize_review_correction was invoked (fail closed, #620)"
) )
elif ( elif (
action in _TERMINAL_REVIEW_ACTIONS action in _TERMINAL_REVIEW_ACTIONS
@@ -3107,9 +3111,9 @@ def check_review_decision_gate(
and not lock.get("correction_authorized") and not lock.get("correction_authorized")
): ):
reasons.append( reasons.append(
"terminal review decision already submitted on this PR in this " "terminal review decision already submitted for this PR head in "
"run; blocked unless an operator-approved correction was " "this run; blocked unless an operator-approved correction was "
"authorized (fail closed)" "authorized (fail closed, #620)"
) )
return reasons 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): def record_live_review_mutation(pr_number: int, action: str, review_id: int | None = None):
lock = _load_review_decision_lock() or {} lock = _load_review_decision_lock() or {}
mutations = list(lock.get("live_mutations") 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, "pr_number": pr_number,
"action": action, "action": action,
"review_id": review_id, "review_id": review_id,
"review_state": action, "review_state": action,
}) }
if head_sha:
entry["head_sha"] = head_sha
mutations.append(entry)
lock["live_mutations"] = mutations lock["live_mutations"] = mutations
if lock.get("correction_authorized"): if lock.get("correction_authorized"):
lock["correction_authorized"] = False 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) _save_review_decision_lock(lock)
def terminal_review_hard_stop_reasons(pr_number: int, operation: str) -> list[str]: def terminal_review_hard_stop_reasons(
"""Session hard-stop after a terminal live review mutation (#332). 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 After a terminal verdict is consumed for a given PR **head**, the only
continuation is the merge sequence for the same PR that was approved. permitted continuation is the merge sequence for that same approved PR
A REQUEST_CHANGES (or an approval of a different PR) blocks every (merge does not require a new head). A REQUEST_CHANGES (or an approval of
further review/mark-ready/merge mutation. An operator-approved a different PR, or the same head) blocks further review/mark-ready/merge
correction (#211) re-opens the review path only — never a cross-PR mutations for that boundary.
merge.
*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. Returns [] when the operation may proceed.
""" """
lock = _load_review_decision_lock() 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 and last.get("pr_number") == pr_number
): ):
return [] 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 [] 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": if last.get("action") == "approve":
guidance = ( guidance = (
f"only the merge sequence for approved PR " 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: else:
guidance = "the session must stop and produce a final report" guidance = "the session must stop and produce a final report"
head_note = (
f" at head {locked_head[:12]}"
if locked_head
else ""
)
recovery = ( recovery = (
"if that PR is already merged/closed, use " "if that PR is already merged/closed, use "
"gitea_cleanup_stale_review_decision_lock (apply=true) after live-state " "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 [ return [
"terminal review mutation already consumed in this run " "terminal review mutation already consumed in this run "
f"({last.get('action')} on PR #{last.get('pr_number')}); {guidance} " f"({last.get('action')} on PR #{last.get('pr_number')}{head_note}); "
f"(fail closed, #332); {recovery}" f"{guidance} (fail closed, #332); {recovery}"
] ]
@@ -3800,18 +3839,11 @@ def gitea_mark_final_review_decision(
"reasons": workflow_blockers + ( "reasons": workflow_blockers + (
review_workflow_load.recovery_handoff_without_replay()), 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: if hard_stop:
return {"marked_ready": False, "reasons": 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: if action not in _REVIEW_ACTIONS:
return { return {
"marked_ready": False, "marked_ready": False,
@@ -3828,6 +3860,17 @@ def gitea_mark_final_review_decision(
"decision (fail closed, #399)" "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( elig = gitea_check_pr_eligibility(
pr_number=pr_number, pr_number=pr_number,
action="review", action="review",
@@ -3880,6 +3923,8 @@ def gitea_mark_final_review_decision(
"#332)" "#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["final_review_decision_ready"] = True
lock["ready_pr_number"] = pr_number lock["ready_pr_number"] = pr_number
lock["ready_action"] = action lock["ready_action"] = action
@@ -3897,6 +3942,7 @@ def gitea_mark_final_review_decision(
"org": org, "org": org,
"repo": repo, "repo": repo,
"final_review_decision_ready": True, "final_review_decision_ready": True,
"head_scoped": True,
"reasons": [], "reasons": [],
} }
@@ -4063,6 +4109,12 @@ def gitea_cleanup_stale_review_decision_lock(
"cleanup_allowed": assessment.get("cleanup_allowed"), "cleanup_allowed": assessment.get("cleanup_allowed"),
"last_terminal_pr": assessment.get("last_terminal_pr"), "last_terminal_pr": assessment.get("last_terminal_pr"),
"last_terminal_action": assessment.get("last_terminal_action"), "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_state": assessment.get("pr_state"),
"pr_merged": assessment.get("pr_merged"), "pr_merged": assessment.get("pr_merged"),
"pr_merged_or_closed": assessment.get("pr_merged_or_closed"), "pr_merged_or_closed": assessment.get("pr_merged_or_closed"),
+191 -3
View File
@@ -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 #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 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 same-PR merge sequence remains, yet the durable ledger still blocks unrelated
new terminal reviews. 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 This module is pure policy (no Gitea I/O). The MCP tool
``gitea_cleanup_stale_review_decision_lock`` fetches live PR state, calls these ``gitea_cleanup_stale_review_decision_lock`` fetches live PR state, calls these
helpers, and only then clears durable state when cleanup is allowed. 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"}) 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: def last_terminal_mutation(lock: dict | None) -> dict | None:
"""Return the last terminal live mutation on *lock*, or None.""" """Return the last terminal live mutation on *lock*, or None."""
if not lock: if not lock:
@@ -35,18 +80,125 @@ def last_terminal_mutation(lock: dict | None) -> dict | None:
return terminals[-1] if terminals else 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]: def classify_pr_live_state(pr_live: dict | None) -> dict[str, Any]:
"""Normalize a Gitea PR payload into merge/closed flags.""" """Normalize a Gitea PR payload into merge/closed flags."""
pr = pr_live or {} pr = pr_live or {}
merged = bool(pr.get("merged") or pr.get("merged_at")) merged = bool(pr.get("merged") or pr.get("merged_at"))
state = (pr.get("state") or "").strip().lower() or None state = (pr.get("state") or "").strip().lower() or None
closed = state == "closed" 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 { return {
"pr_state": state, "pr_state": state,
"pr_merged": merged, "pr_merged": merged,
"pr_merged_or_closed": merged or closed, "pr_merged_or_closed": merged or closed,
"merge_commit_sha": pr.get("merge_commit_sha") "merge_commit_sha": pr.get("merge_commit_sha")
or pr.get("merged_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 return None
last = last_terminal_mutation(lock) last = last_terminal_mutation(lock)
mutations = list(lock.get("live_mutations") or []) mutations = list(lock.get("live_mutations") or [])
last_head = mutation_head_sha(last, lock) if last else None
return { return {
"task": lock.get("task"), "task": lock.get("task"),
"remote": lock.get("remote"), "remote": lock.get("remote"),
@@ -67,16 +220,21 @@ def lock_summary(lock: dict | None) -> dict[str, Any] | None:
"session_profile": lock.get("session_profile"), "session_profile": lock.get("session_profile"),
"ready_pr_number": lock.get("ready_pr_number"), "ready_pr_number": lock.get("ready_pr_number"),
"ready_action": lock.get("ready_action"), "ready_action": lock.get("ready_action"),
"ready_expected_head_sha": normalize_head_sha(
lock.get("ready_expected_head_sha")
),
"live_mutations_count": len(mutations), "live_mutations_count": len(mutations),
"last_terminal": ( "last_terminal": (
{ {
"pr_number": last.get("pr_number"), "pr_number": last.get("pr_number"),
"action": last.get("action"), "action": last.get("action"),
"review_id": last.get("review_id"), "review_id": last.get("review_id"),
"head_sha": last_head,
} }
if last if last
else None else None
), ),
"locked_head_sha": last_head,
"correction_authorized": bool(lock.get("correction_authorized")), "correction_authorized": bool(lock.get("correction_authorized")),
"updated_at": lock.get("updated_at") or lock.get("recorded_at"), "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_live: dict | None = None,
pr_lookup_error: str | None = None, pr_lookup_error: str | None = None,
active_profile_identity: str | None = None, active_profile_identity: str | None = None,
current_pr_head_sha: str | None = None,
) -> dict[str, Any]: ) -> 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 *pr_live* must be the live Gitea payload for the **last terminal
mutation's PR**. When no lock / no terminal mutation exists, cleanup is 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 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) summary = lock_summary(lock)
result: dict[str, Any] = { result: dict[str, Any] = {
@@ -102,6 +263,10 @@ def assess_stale_review_decision_lock(
"lock_summary": summary, "lock_summary": summary,
"last_terminal_pr": None, "last_terminal_pr": None,
"last_terminal_action": 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, "is_moot": False,
"cleanup_allowed": False, "cleanup_allowed": False,
"profile_match": True, "profile_match": True,
@@ -141,8 +306,10 @@ def assess_stale_review_decision_lock(
pr_number = last.get("pr_number") pr_number = last.get("pr_number")
action = last.get("action") action = last.get("action")
locked_head = mutation_head_sha(last, lock)
result["last_terminal_pr"] = pr_number result["last_terminal_pr"] = pr_number
result["last_terminal_action"] = action result["last_terminal_action"] = action
result["locked_head_sha"] = locked_head
if pr_number is None: if pr_number is None:
result["reasons"].append( result["reasons"].append(
@@ -165,16 +332,35 @@ def assess_stale_review_decision_lock(
return result return result
live = classify_pr_live_state(pr_live) 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( result.update(
{ {
"pr_state": live["pr_state"], "pr_state": live["pr_state"],
"pr_merged": live["pr_merged"], "pr_merged": live["pr_merged"],
"pr_merged_or_closed": live["pr_merged_or_closed"], "pr_merged_or_closed": live["pr_merged_or_closed"],
"merge_commit_sha": live["merge_commit_sha"], "merge_commit_sha": live["merge_commit_sha"],
"current_pr_head_sha": current_head,
} }
) )
if not live["pr_merged_or_closed"]: if not live["pr_merged_or_closed"]:
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( result["reasons"].append(
f"terminal review mutation on open PR #{pr_number} is still active " f"terminal review mutation on open PR #{pr_number} is still active "
f"({action}); #332 hard-stop remains — cleanup forbidden (#594)" f"({action}); #332 hard-stop remains — cleanup forbidden (#594)"
@@ -184,6 +370,8 @@ def assess_stale_review_decision_lock(
# Merged or closed: lock is moot. Same-PR merge continuation is impossible. # Merged or closed: lock is moot. Same-PR merge continuation is impossible.
result["is_moot"] = True result["is_moot"] = True
result["cleanup_allowed"] = True result["cleanup_allowed"] = True
result["stale_by_head"] = False
result["fresh_review_on_current_head_allowed"] = False
result["reasons"].append( result["reasons"].append(
f"terminal mutation ({action} on PR #{pr_number}) is moot: PR is " f"terminal mutation ({action} on PR #{pr_number}) is moot: PR is "
f"{'merged' if live['pr_merged'] else 'closed'}; canonical cleanup allowed (#594)" f"{'merged' if live['pr_merged'] else 'closed'}; canonical cleanup allowed (#594)"
@@ -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()