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

fix(mcp): chain-scoped newest-wins reviewer lease reads in pr_work_lease (#742)

Browse Source 
Second author remediation on PR #743. Fixes the full-ledger defect surfaced by
the first remediation.

Root cause: pr_work_lease.find_active_reviewer_lease iterated the reviewer
markers newest-first and used `continue` on a terminal marker. On a realistic
append-only ledger (claimed -> validating -> terminal) it therefore stepped
over the terminal marker and returned the older claim of the very chain that
marker had just ended. reviewer_pr_lease got strict newest-wins under #577;
pr_work_lease never did, so the two modules disagreed for released, blocked,
done, and abandoned alike, and merger owner finalization did not leave "no
active lease" for pr_work_lease consumers (conflict-fix acquire, PR sync
inventory).

Fix: a claim is skipped when a later marker terminates its own chain. Chain
identity is (repo, pr_number, candidate_head, session_id, reviewer_identity,
profile) via the new _reviewer_chain_key; _chain_terminated_after scans only
markers appended after the candidate. Consequences:

- a valid terminal marker ends its matching earlier claim (no resurrection);
- a foreign-session, wrong-repo, wrong-PR, wrong-head, wrong-identity, or
  wrong-profile terminal marker cannot cancel another session's active lease,
  which strict newest-wins alone would have allowed;
- a malformed terminal marker has no provable chain key, so it cancels nothing
  and cannot hide a valid active claim;
- expiry, freshness, phase sets, ownership and integrity checks, both parsers,
  and find_active_conflict_fix_lease are untouched;
- history stays append-only; no marker is deleted or rewritten.

Reviewer lease markers carry no token field, so token-fingerprint validation
remains where it already lives (session provenance, merger finalization) and is
not weakened here.

Tests: new TestFullLedgerNewestWins in tests/test_merger_lease_finalization.py
covers claimed->released/blocked/done/abandoned, claimed->validating->terminal,
foreign-session and mismatched repo/PR/head/identity/profile terminals, the
malformed terminal marker, a newer active chain surviving an older terminated
chain, newest-valid-chain selection across multiple histories, all-chains-
terminated, single-marker parity between the two modules across eight phases,
expired-claim/freshness non-regression, and the real ledger written by
gitea_release_merger_pr_lease reading as terminal in both modules with the
prior marker preserved. TestCrossModuleTerminalPhaseAgreement's scope-limited
placeholder test is replaced by a real both-modules full-ledger assertion.

Verified 10 of the new tests fail at the prior head 7ae5f3a and pass here.

Validation: focused TestFullLedgerNewestWins 14 passed / 21 subtests;
tests/test_merger_lease_finalization.py 59 passed / 42 subtests; targeted
pr_work_lease + reviewer/merger lease + adoption + provenance + acquire/release
+ anti-stomp + capability-map + decision-lock + report-validator suites 309
passed / 41 subtests; full suite 3326 passed, 6 skipped, 2 failed. Both
failures (test_issue_702_review_findings_f1_f6 F1 recovery, reconciler
supersession close) reproduce identically on clean baseline master a8d2087 and
are pre-existing #737 org/repo-forwarding drift. git diff --check clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
sysadmin
2026-07-18 10:12:49 -04:00
co-authored by Claude Opus 4.8
parent 7ae5f3a541
commit fde95b9266
2 changed files with 272 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pr_work_lease.py
+49 -2
View File
@@ -157,25 +157,72 @@ def _lease_phase_active(lease: dict, *, active_phases: frozenset[str]) -> bool:
))
def _reviewer_chain_key(lease: dict) -> tuple | None:
"""Identity of the lease chain a reviewer marker belongs to (#742).
A chain is one session's claim → heartbeat → terminal sequence, keyed by
repository, PR, candidate head, session id, identity, and profile. Returns
None when any component is missing: an incomplete or malformed marker has
no provable chain, so it can neither be cancelled by nor cancel anything.
"""
raw = lease.get("raw_fields") or {}
repo = (raw.get("repo") or "").strip().lower()
session_id = (lease.get("session_id") or "").strip()
identity = (lease.get("reviewer_identity") or "").strip()
profile = (lease.get("profile") or "").strip()
head = lease.get("candidate_head")
pr_number = lease.get("pr_number")
if not (repo and session_id and identity and profile and head and pr_number):
return None
return (repo, pr_number, head, session_id, identity, profile)
def _chain_terminated_after(entries: list[dict], index: int) -> bool:
"""True when a later marker terminates the chain of ``entries[index]``.
Append-only newest-wins (#577 semantics, chain-scoped for #742): a terminal
marker ends only its *own* claim, so a foreign, forged, or malformed
terminal marker cannot cancel another session's valid active lease.
"""
key = _reviewer_chain_key(entries[index])
if key is None:
return False
for later in entries[index + 1:]:
if (later.get("phase") or "").strip().lower() not in _TERMINAL_REVIEWER_PHASES:
continue
if _reviewer_chain_key(later) == key:
return True
return False
def find_active_reviewer_lease(
comments: list[dict],
*,
pr_number: int,
now: datetime | None = None,
) -> dict[str, Any] | None:
"""Return the newest unexpired reviewer lease for *pr_number*, if any."""
"""Return the newest unexpired, non-terminated reviewer lease for *pr_number*.
Walking backward past a terminal marker used to resurrect the older claim of
the very chain that marker ended, so a released/abandoned finalization still
read as an active lease here while ``reviewer_pr_lease`` reported it ended
(#742). A claim is now skipped when a later marker terminates its own chain.
"""
now = now or datetime.now(timezone.utc)
candidates = [
entry for entry in _comment_entries(comments, pr_number=pr_number)
if entry.get("lease_kind") == "reviewer"
]
for lease in reversed(candidates):
for index in range(len(candidates) - 1, -1, -1):
lease = candidates[index]
if _lease_expired(lease, now=now):
continue
phase = (lease.get("phase") or "").strip().lower()
if phase in _TERMINAL_REVIEWER_PHASES:
continue
if phase in _ACTIVE_REVIEWER_PHASES or phase:
if _chain_terminated_after(candidates, index):
continue
return lease
return None
tests/test_merger_lease_finalization.py
+223 -22
View File
@@ -17,7 +17,7 @@ Covers the two confirmed defects behind the PR #740 failure:
import os
import sys
import unittest
from datetime import datetime, timezone
from datetime import datetime, timedelta, timezone
from unittest.mock import patch
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
@@ -553,28 +553,229 @@ class TestCrossModuleTerminalPhaseAgreement(unittest.TestCase):
# Newest-wins (#577): the appended terminal marker ends the lease.
self.assertIsNone(leases.find_active_reviewer_lease(ledger, pr_number=PR))
def test_abandoned_matches_preexisting_terminal_phases_on_full_ledger(self):
"""'abandoned' must behave exactly like the other terminal phases.
On a claim-then-terminal ledger, pr_work_lease.find_active_reviewer_lease
skips the terminal marker and walks back to the older claim, so it still
reports an active lease. That newest-wins gap is pre-existing on clean
baseline a8d2087 for 'released', 'blocked', and 'done' alike (the #577
fix landed in reviewer_pr_lease only) and is out of scope for #742; this
test pins that 'abandoned' introduces no behavior of its own, so the two
modules can be reconciled for all four phases in one separate change.
"""
def pwl_active(phase):
ledger = [
_lease_comment(),
_lease_comment(phase=phase, comment_id=12361),
]
return pwl.find_active_reviewer_lease(ledger, pr_number=PR) is not None
baseline = pwl_active("released")
for phase in ("blocked", "done", "abandoned"):
def test_full_ledger_terminal_phases_end_the_lease_in_both_modules(self):
for phase in self.TERMINAL_PHASES:
with self.subTest(phase=phase):
self.assertEqual(pwl_active(phase), baseline)
ledger = [
_lease_comment(),
_lease_comment(phase=phase, comment_id=12361),
]
for module in (leases, pwl):
self.assertIsNone(
module.find_active_reviewer_lease(ledger, pr_number=PR),
f"{module.__name__} still active after {phase}",
)
class TestFullLedgerNewestWins(unittest.TestCase):
"""#742 second remediation — chain-scoped newest-wins in pr_work_lease.
On a realistic append-only ledger (claim -> heartbeat -> terminal),
pr_work_lease.find_active_reviewer_lease skipped the terminal marker and
walked backward to the older claim of that same chain, resurrecting it. A
terminal marker must end its own chain, while a foreign, forged, or
malformed terminal marker must never cancel someone else's active lease.
"""
OTHER_SESSION = "other-session"
def _claim(self, **overrides):
return _lease_comment(**overrides)
def _terminal(self, phase="released", comment_id=12361, **overrides):
return _lease_comment(phase=phase, comment_id=comment_id, **overrides)
def _pwl_active(self, ledger):
return pwl.find_active_reviewer_lease(ledger, pr_number=PR)
# --- the chain must end -------------------------------------------------
def test_claim_then_each_terminal_phase_leaves_no_active_lease(self):
for phase in ("released", "blocked", "done", "abandoned"):
with self.subTest(phase=phase):
self.assertIsNone(
self._pwl_active([self._claim(), self._terminal(phase=phase)])
)
def test_claim_heartbeat_terminal_leaves_no_active_lease(self):
for phase in ("released", "blocked", "done", "abandoned"):
with self.subTest(phase=phase):
ledger = [
self._claim(),
self._lease_validating(),
self._terminal(phase=phase, comment_id=12362),
]
self.assertIsNone(self._pwl_active(ledger))
self.assertIsNone(
leases.find_active_reviewer_lease(ledger, pr_number=PR)
)
def _lease_validating(self):
return _lease_comment(phase="validating", comment_id=12355)
# --- foreign / forged terminal markers must not cancel ------------------
def test_foreign_session_terminal_does_not_cancel_active_claim(self):
ledger = [
self._claim(),
self._terminal(session_id=self.OTHER_SESSION),
]
active = self._pwl_active(ledger)
self.assertIsNotNone(active)
self.assertEqual(active["session_id"], "merger-session")
def test_mismatched_chain_fields_do_not_cancel_active_claim(self):
cases = {
"identity": {"identity": "someone-else"},
"profile": {"profile": "prgs-reviewer"},
"candidate_head": {"candidate_head": OTHER_HEAD},
}
for label, override in cases.items():
with self.subTest(field=label):
ledger = [self._claim(), self._terminal(**override)]
self.assertIsNotNone(
self._pwl_active(ledger),
f"terminal with wrong {label} must not cancel the claim",
)
def test_terminal_for_another_pr_does_not_cancel_active_claim(self):
ledger = [
self._claim(),
self._terminal(pr_number=741, comment_id=12363),
]
self.assertIsNotNone(self._pwl_active(ledger))
def test_terminal_for_another_repository_does_not_cancel_active_claim(self):
foreign = self._terminal()
foreign["body"] = foreign["body"].replace(
REPO, "Scaled-Tech-Consulting/Other-Repo"
)
self.assertIsNotNone(self._pwl_active([self._claim(), foreign]))
def test_malformed_terminal_marker_does_not_hide_active_claim(self):
malformed = self._terminal()
# Strip the session id line: no provable chain, so it cancels nothing.
malformed["body"] = "\n".join(
line
for line in malformed["body"].splitlines()
if not line.startswith("session_id:")
)
active = self._pwl_active([self._claim(), malformed])
self.assertIsNotNone(active)
self.assertEqual(active["session_id"], "merger-session")
# --- multi-chain ledgers ------------------------------------------------
def test_newer_active_chain_survives_older_terminated_chain(self):
ledger = [
self._claim(),
self._terminal(comment_id=12361),
_lease_comment(session_id=self.OTHER_SESSION, comment_id=12370),
]
active = self._pwl_active(ledger)
self.assertIsNotNone(active)
self.assertEqual(active["session_id"], self.OTHER_SESSION)
def test_newest_valid_chain_selected_across_multiple_histories(self):
ledger = [
_lease_comment(session_id="chain-1", comment_id=12300),
_lease_comment(session_id="chain-1", phase="released", comment_id=12301),
_lease_comment(session_id="chain-2", comment_id=12310),
_lease_comment(session_id="chain-2", phase="abandoned", comment_id=12311),
_lease_comment(session_id="chain-3", comment_id=12320),
]
active = self._pwl_active(ledger)
self.assertIsNotNone(active)
self.assertEqual(active["session_id"], "chain-3")
def test_all_chains_terminated_leaves_no_active_lease(self):
ledger = [
_lease_comment(session_id="chain-1", comment_id=12300),
_lease_comment(session_id="chain-1", phase="released", comment_id=12301),
_lease_comment(session_id="chain-2", comment_id=12310),
_lease_comment(session_id="chain-2", phase="abandoned", comment_id=12311),
]
self.assertIsNone(self._pwl_active(ledger))
# --- no regression in existing behavior ---------------------------------
def test_single_marker_behavior_matches_reviewer_pr_lease(self):
for phase in (
"claimed", "validating", "approved", "merging",
"released", "blocked", "done", "abandoned",
):
with self.subTest(phase=phase):
ledger = [_lease_comment(phase=phase)]
self.assertEqual(
pwl.find_active_reviewer_lease(ledger, pr_number=PR) is not None,
leases.find_active_reviewer_lease(ledger, pr_number=PR)
is not None,
)
def test_expired_claim_stays_inactive_and_freshness_unchanged(self):
past = datetime.now(timezone.utc) - timedelta(hours=4)
expired = {
"id": 12399,
"user": {"login": "merger-user"},
"body": leases.format_lease_body(
repo=REPO,
pr_number=PR,
issue_number=742,
reviewer_identity="merger-user",
profile="prgs-merger",
session_id="expired-session",
worktree="branches/merger-pr740",
phase="claimed",
candidate_head=HEAD,
target_branch="master",
target_branch_sha="e" * 40,
last_activity=past,
expires_at=past,
),
}
self.assertIsNone(self._pwl_active([expired]))
self.assertIsNone(
leases.find_active_reviewer_lease([expired], pr_number=PR)
)
def test_active_claim_without_any_terminal_marker_remains_active(self):
self.assertIsNotNone(self._pwl_active([self._claim()]))
def test_production_readers_agree_after_release_tool_finalization(self):
"""The ledger the release tool actually writes must read as terminal."""
leases.clear_session_lease()
try:
session = _record_acquired()
result = mla.assess_merger_lease_finalization(
[_lease_comment()],
pr_number=PR,
session=session,
actor_identity="merger-user",
actor_profile="prgs-merger",
actor_session_id="merger-session",
repo=REPO,
worktree="branches/merger-pr740",
candidate_head=HEAD,
live_head_sha=HEAD,
outcome=mla.OUTCOME_ABANDONED,
)
self.assertTrue(result["finalize_allowed"], result["reasons"])
posted = {
"id": 12370,
"user": {"login": "merger-user"},
"body": result["finalization_body"],
}
ledger = [_lease_comment(), posted]
for module in (leases, pwl):
with self.subTest(module=module.__name__):
self.assertIsNone(
module.find_active_reviewer_lease(ledger, pr_number=PR)
)
# Append-only: the claim marker is still present and unmodified.
self.assertEqual(len(ledger), 2)
self.assertEqual(ledger[0]["id"], 12354)
finally:
leases.clear_session_lease()
class TestReleaseMergerLeaseTool(unittest.TestCase):