From 9b96085d8d5fd6b7f54a3cac8573a261aad40972 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Thu, 9 Jul 2026 10:09:38 -0400 Subject: [PATCH] fix: newest lease marker wins so release ends prior claims find_active_reviewer_lease walked past terminal markers and re-armed older claimed leases after gitea_release_reviewer_pr_lease. Treat the newest marker as authoritative (append-only ledger). Closes #577 --- reviewer_pr_lease.py | 32 +++++++++++++++++---------- tests/test_reviewer_pr_lease.py | 39 +++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 11 deletions(-) diff --git a/reviewer_pr_lease.py b/reviewer_pr_lease.py index f55390b..2310843 100644 --- a/reviewer_pr_lease.py +++ b/reviewer_pr_lease.py @@ -190,18 +190,28 @@ def find_active_reviewer_lease( pr_number: int, now: datetime | None = None, ) -> dict[str, Any] | None: - """Newest non-terminal, unexpired lease for *pr_number*.""" + """Return the newest unexpired non-terminal lease for *pr_number*. + + Append-only lease markers form a ledger: the **newest** marker is + authoritative. A later terminal phase (``released`` / ``done`` / + ``blocked``) ends the lease even when older ``claimed`` markers remain + on the thread (#577). Skipping only terminal markers and walking older + claims incorrectly re-arms a lease after a successful release. + """ now = now or datetime.now(timezone.utc) - for lease in reversed(_lease_entries(comments, pr_number=pr_number)): - phase = (lease.get("phase") or "").strip().lower() - if phase in _TERMINAL_PHASES: - continue - if _lease_expired(lease, now=now): - continue - if phase in _ACTIVE_PHASES or phase: - lease = dict(lease) - lease["freshness"] = classify_lease_freshness(lease, now=now) - return lease + entries = list(reversed(_lease_entries(comments, pr_number=pr_number))) + if not entries: + return None + newest = entries[0] + phase = (newest.get("phase") or "").strip().lower() + if phase in _TERMINAL_PHASES: + return None + if _lease_expired(newest, now=now): + return None + if phase in _ACTIVE_PHASES or phase: + lease = dict(newest) + lease["freshness"] = classify_lease_freshness(lease, now=now) + return lease return None diff --git a/tests/test_reviewer_pr_lease.py b/tests/test_reviewer_pr_lease.py index 97e5a2e..875d562 100644 --- a/tests/test_reviewer_pr_lease.py +++ b/tests/test_reviewer_pr_lease.py @@ -98,6 +98,45 @@ class TestReviewerLeaseFreshness(unittest.TestCase): ) +class TestReviewerLeaseNewestWins(unittest.TestCase): + """#577: terminal release must supersede older claimed markers.""" + + def test_released_after_claimed_has_no_active_lease(self): + claim = _lease_comment(516, "session-a", phase="claimed") + claim["id"] = 1 + release = _lease_comment(516, "session-a", phase="released") + release["id"] = 2 + active = leases.find_active_reviewer_lease( + [claim, release], pr_number=516 + ) + self.assertIsNone(active) + + def test_new_claim_after_release_is_active(self): + claim = _lease_comment(516, "session-a", phase="claimed") + claim["id"] = 1 + release = _lease_comment(516, "session-a", phase="released") + release["id"] = 2 + reclaim = _lease_comment(516, "session-b", phase="claimed") + reclaim["id"] = 3 + active = leases.find_active_reviewer_lease( + [claim, release, reclaim], pr_number=516 + ) + self.assertIsNotNone(active) + self.assertEqual(active.get("session_id"), "session-b") + self.assertEqual(active.get("phase"), "claimed") + self.assertEqual(active.get("comment_id"), 3) + + def test_done_after_claimed_has_no_active_lease(self): + claim = _lease_comment(516, "session-a", phase="claimed") + claim["id"] = 10 + done = _lease_comment(516, "session-a", phase="done") + done["id"] = 11 + active = leases.find_active_reviewer_lease( + [claim, done], pr_number=516 + ) + self.assertIsNone(active) + + class TestReviewerLeaseMutationGate(unittest.TestCase): def setUp(self): leases.clear_session_lease()