diff --git a/pr_work_lease.py b/pr_work_lease.py index 92cccc1..f5a843d 100644 --- a/pr_work_lease.py +++ b/pr_work_lease.py @@ -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 diff --git a/tests/test_merger_lease_finalization.py b/tests/test_merger_lease_finalization.py index 3f83620..3cea475 100644 --- a/tests/test_merger_lease_finalization.py +++ b/tests/test_merger_lease_finalization.py @@ -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):