Scaled-Tech-Consulting/Gitea-Tools
2
0
Fork 0
Code Issues 79 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

fix: head-scope durable #332 review-decision locks (#620)

Browse Source 
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
This commit is contained in:
sysadmin
2026-07-10 01:04:37 -04:00
parent 08c3488d7c
commit 9e1d5c5649
3 changed files with 683 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
stale_review_decision_lock.py
+194 -6
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
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)"