fix: newest reviewer lease marker wins so release ends prior claims (Closes #577) #579
+21
-11
@@ -190,18 +190,28 @@ def find_active_reviewer_lease(
|
|||||||
pr_number: int,
|
pr_number: int,
|
||||||
now: datetime | None = None,
|
now: datetime | None = None,
|
||||||
) -> dict[str, Any] | 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)
|
now = now or datetime.now(timezone.utc)
|
||||||
for lease in reversed(_lease_entries(comments, pr_number=pr_number)):
|
entries = list(reversed(_lease_entries(comments, pr_number=pr_number)))
|
||||||
phase = (lease.get("phase") or "").strip().lower()
|
if not entries:
|
||||||
if phase in _TERMINAL_PHASES:
|
return None
|
||||||
continue
|
newest = entries[0]
|
||||||
if _lease_expired(lease, now=now):
|
phase = (newest.get("phase") or "").strip().lower()
|
||||||
continue
|
if phase in _TERMINAL_PHASES:
|
||||||
if phase in _ACTIVE_PHASES or phase:
|
return None
|
||||||
lease = dict(lease)
|
if _lease_expired(newest, now=now):
|
||||||
lease["freshness"] = classify_lease_freshness(lease, now=now)
|
return None
|
||||||
return lease
|
if phase in _ACTIVE_PHASES or phase:
|
||||||
|
lease = dict(newest)
|
||||||
|
lease["freshness"] = classify_lease_freshness(lease, now=now)
|
||||||
|
return lease
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -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):
|
class TestReviewerLeaseMutationGate(unittest.TestCase):
|
||||||
def setUp(self):
|
def setUp(self):
|
||||||
leases.clear_session_lease()
|
leases.clear_session_lease()
|
||||||
|
|||||||
Reference in New Issue
Block a user