pr_work_lease.py

@@ -20,7 +20,11 @@ _FIELD_RE = re.compile(
     re.IGNORECASE | re.MULTILINE,
 )
 
-_TERMINAL_REVIEWER_PHASES = frozenset({"done", "released", "blocked"})
+# Must mirror reviewer_pr_lease._TERMINAL_PHASES: both modules read the same
+# append-only lease markers, so a phase that is terminal in one and active in
+# the other yields two conflicting truths for the same comment (#742 review
+# 460). "abandoned" is the owner-session merger finalization outcome.
+_TERMINAL_REVIEWER_PHASES = frozenset({"done", "released", "blocked", "abandoned"})
 _ACTIVE_REVIEWER_PHASES = frozenset({
     "claimed",
     "validating",

tests/test_merger_lease_finalization.py

@@ -24,6 +24,7 @@ sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.par
 
 import gitea_mcp_server as mcp_server  # noqa: E402
 import merger_lease_adoption as mla  # noqa: E402
+import pr_work_lease as pwl  # noqa: E402
 import reviewer_pr_lease as leases  # noqa: E402
 import task_capability_map as tcm  # noqa: E402
 
@@ -467,6 +468,115 @@ class TestFinalizationAssessment(unittest.TestCase):
         self.assertEqual(result["lease_comment_id"], 12354)
 
 
+class TestCrossModuleTerminalPhaseAgreement(unittest.TestCase):
+    """#742 review 460 — reviewer_pr_lease and pr_work_lease must agree.
+
+    Both modules parse the same append-only lease markers. A phase that is
+    terminal in one and active in the other produces two conflicting truths for
+    the same comment, so an abandoned finalization would still read as an active
+    lease to the conflict-fix acquire and PR-sync inventory paths.
+    """
+
+    TERMINAL_PHASES = ("released", "blocked", "done", "abandoned")
+    NONTERMINAL_PHASES = ("claimed", "validating")
+
+    def _both_active(self, phase: str) -> tuple[bool, bool]:
+        comments = [_lease_comment(phase=phase)]
+        return (
+            leases.find_active_reviewer_lease(comments, pr_number=PR) is not None,
+            pwl.find_active_reviewer_lease(comments, pr_number=PR) is not None,
+        )
+
+    def test_abandoned_marker_is_not_active_in_pr_work_lease(self):
+        _, pwl_active = self._both_active("abandoned")
+        self.assertFalse(pwl_active)
+
+    def test_terminal_phases_agree_across_modules(self):
+        for phase in self.TERMINAL_PHASES:
+            with self.subTest(phase=phase):
+                rpl_active, pwl_active = self._both_active(phase)
+                self.assertFalse(rpl_active)
+                self.assertFalse(pwl_active)
+                self.assertEqual(rpl_active, pwl_active)
+
+    def test_nonterminal_phases_remain_active_in_both_modules(self):
+        for phase in self.NONTERMINAL_PHASES:
+            with self.subTest(phase=phase):
+                rpl_active, pwl_active = self._both_active(phase)
+                self.assertTrue(rpl_active)
+                self.assertTrue(pwl_active)
+
+    def test_terminal_phase_sets_are_mirrored(self):
+        self.assertEqual(
+            set(leases._TERMINAL_PHASES), set(pwl._TERMINAL_REVIEWER_PHASES)
+        )
+
+    def test_default_released_finalization_behavior_unchanged(self):
+        # The default outcome is still 'released', and a released marker ends
+        # the lease for both modules exactly as before this change.
+        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,
+            )
+            self.assertTrue(result["finalize_allowed"], result["reasons"])
+            self.assertEqual(result["outcome"], mla.OUTCOME_RELEASED)
+            self.assertIn("phase: released", result["finalization_body"])
+            self.assertEqual(
+                result["finalization_reason"],
+                mla.DEFAULT_MERGER_FINALIZATION_REASON,
+            )
+        finally:
+            leases.clear_session_lease()
+
+    def test_finalization_appends_and_never_rewrites_prior_markers(self):
+        claimed = _lease_comment()
+        original_body = claimed["body"]
+        ledger = [claimed]
+        # A terminal marker is appended alongside the original claim; the
+        # earlier marker is left byte-for-byte intact and is never removed.
+        terminal = _lease_comment(phase="abandoned", comment_id=12361)
+        ledger.append(terminal)
+        self.assertEqual(len(ledger), 2)
+        self.assertEqual(ledger[0]["body"], original_body)
+        self.assertEqual(ledger[0]["id"], 12354)
+        # 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"):
+            with self.subTest(phase=phase):
+                self.assertEqual(pwl_active(phase), baseline)
+
+
 class TestReleaseMergerLeaseTool(unittest.TestCase):
     """AC7/AC9/AC10/AC11 — the native tool contract end to end."""