diff --git a/pr_work_lease.py b/pr_work_lease.py index 2997462..92cccc1 100644 --- a/pr_work_lease.py +++ b/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", diff --git a/tests/test_merger_lease_finalization.py b/tests/test_merger_lease_finalization.py index 7785e08..3f83620 100644 --- a/tests/test_merger_lease_finalization.py +++ b/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."""