diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 569c64b..31ee717 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -7154,6 +7154,62 @@ def gitea_assess_reviewer_pr_lease( } +@mcp.tool() +def gitea_diagnose_reviewer_pr_lease_handoff( + pr_number: int, + proposed_worktree: str | None = None, + instructed_session_id: str | None = None, + instructed_comment_id: int | None = None, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Read-only: diagnose open-PR reviewer lease handoff and emit next action (#599). + + Classifies no-lease / own / foreign / instructed-lease-missing-with-replacement + and worktree binding mismatch. Returns a canonical ``next_action`` without + mutating Gitea state. Never steals foreign leases. + """ + read_block = _profile_operation_gate("gitea.read") + if read_block: + return { + "success": False, + "reasons": read_block, + "permission_report": _permission_block_report("gitea.read"), + } + comments = _fetch_pr_comments( + pr_number, remote=remote, host=host, org=org, repo=repo) + h, _o, _r = _resolve(remote, host, org, repo) + try: + username = _authenticated_username(h) + except Exception: + username = None + session_lease = reviewer_pr_lease.get_session_lease() or {} + env_wt = ( + (os.environ.get("GITEA_REVIEWER_WORKTREE") or "").strip() + or (os.environ.get("GITEA_ACTIVE_WORKTREE") or "").strip() + or None + ) + # Prefer in-session lease id when present; otherwise diagnose as a fresh + # caller without inventing ownership of an on-thread lease. + session_id = (session_lease.get("session_id") or "").strip() or None + diagnosis = reviewer_pr_lease.diagnose_reviewer_pr_lease_handoff( + comments, + pr_number=pr_number, + current_session_id=session_id, + current_reviewer_identity=username, + proposed_worktree=proposed_worktree, + env_bound_worktree=env_wt, + instructed_session_id=instructed_session_id, + instructed_comment_id=instructed_comment_id, + ) + diagnosis["success"] = True + diagnosis["remote"] = remote + diagnosis["authenticated_user"] = username + return diagnosis + + @mcp.tool() def gitea_cleanup_post_merge_moot_lease( pr_number: int, diff --git a/reviewer_pr_lease.py b/reviewer_pr_lease.py index 2310843..58f774d 100644 --- a/reviewer_pr_lease.py +++ b/reviewer_pr_lease.py @@ -522,4 +522,241 @@ def assess_lease_inventory( "active_review_leases": active, "stale_review_leases": stale, "reclaimable_review_leases": reclaimable, - } \ No newline at end of file + } + +# Canonical next-action vocabulary for reviewer lease handoff (#599). +NEXT_ACTION_ACQUIRE = "acquire" +NEXT_ACTION_WAIT = "wait" +NEXT_ACTION_RESUME_EXACT_OWNER_SESSION = "resume_exact_owner_session" +NEXT_ACTION_RELEASE_EXPIRED_LEASE = "release_expired_lease" +NEXT_ACTION_OPERATOR_AUTHORIZED_CLEANUP = "operator_authorized_cleanup" +NEXT_ACTION_REPAIR_WORKTREE_BINDING = "repair_worktree_binding" + +_HANDOFF_CLASSIFICATIONS = frozenset({ + "no_lease", + "own_active", + "own_expired", + "foreign_active", + "foreign_reclaimable", + "foreign_expired", + "instructed_lease_missing_with_replacement", + "worktree_binding_mismatch", +}) + + +def _norm_path(value: str | None) -> str: + text = (value or "").strip() + if not text: + return "" + return os.path.normpath(text.rstrip("/")) + + +def diagnose_reviewer_pr_lease_handoff( + comments: list[dict], + *, + pr_number: int, + current_session_id: str | None, + current_reviewer_identity: str | None, + proposed_worktree: str | None = None, + env_bound_worktree: str | None = None, + instructed_session_id: str | None = None, + instructed_comment_id: int | None = None, + now: datetime | None = None, +) -> dict[str, Any]: + """Classify open-PR reviewer lease handoff and emit a canonical next action (#599). + + Read-only diagnosis. Never steals, releases, or adopts a foreign lease. + Fail-closed acquisition rules for active foreign leases remain intact. + + Returns a structured diagnosis with: + - classification + - next_action (one of wait / resume_exact_owner_session / + release_expired_lease / operator_authorized_cleanup / + repair_worktree_binding / acquire) + - active_lease identity fields when present + - worktree_binding match result + - instructed-lease mismatch flags + """ + now = now or datetime.now(timezone.utc) + session_id = (current_session_id or "").strip() + identity = (current_reviewer_identity or "").strip() + instructed_sid = (instructed_session_id or "").strip() + reasons: list[str] = [] + + active = find_active_reviewer_lease(comments, pr_number=pr_number, now=now) + session_lease = get_session_lease() + + # Worktree binding: env-bound vs proposed vs active lease worktree. + env_wt = _norm_path(env_bound_worktree) + prop_wt = _norm_path(proposed_worktree) + lease_wt = _norm_path((active or {}).get("worktree") if active else None) + binding_mismatch = False + binding_details: dict[str, Any] = { + "env_bound_worktree": env_bound_worktree or None, + "proposed_worktree": proposed_worktree or None, + "lease_worktree": (active or {}).get("worktree") if active else None, + "match": True, + } + paths = [p for p in (env_wt, prop_wt, lease_wt) if p] + if len(paths) >= 2 and len(set(paths)) > 1: + binding_mismatch = True + binding_details["match"] = False + reasons.append( + "worktree binding mismatch: env/proposed/lease worktree paths disagree" + ) + + # Instructed lease gone while a different lease is active (PR #592-style). + instructed_missing_with_replacement = False + if instructed_sid or instructed_comment_id is not None: + if not active: + reasons.append( + "instructed lease is gone and no active replacement lease remains" + ) + else: + owner = (active.get("session_id") or "").strip() + cid = active.get("comment_id") + sid_mismatch = bool(instructed_sid and owner and owner != instructed_sid) + cid_mismatch = ( + instructed_comment_id is not None + and cid is not None + and int(cid) != int(instructed_comment_id) + ) + if sid_mismatch or cid_mismatch: + instructed_missing_with_replacement = True + reasons.append( + "instructed lease is gone; a different active lease replaced it " + f"(active session_id={owner}, comment_id={cid})" + ) + + # Classification + next_action. + classification = "no_lease" + next_action = NEXT_ACTION_ACQUIRE + + if active: + owner = (active.get("session_id") or "").strip() + freshness = active.get("freshness") or classify_lease_freshness( + active, now=now + ) + owner_identity = (active.get("reviewer_identity") or "").strip() + is_own = bool(session_id and owner and owner == session_id) + # Same identity alone is NOT ownership for resume; session_id must match. + same_identity = bool( + identity and owner_identity and identity == owner_identity + ) + + if is_own and freshness in {"active", "stale_warning"}: + classification = "own_active" + next_action = NEXT_ACTION_RESUME_EXACT_OWNER_SESSION + elif is_own and freshness in {"reclaimable", "expired"}: + classification = "own_expired" + next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE + reasons.append( + f"own lease freshness is '{freshness}'; release via " + "gitea_release_reviewer_pr_lease then re-acquire" + ) + elif not is_own and freshness in {"active", "stale_warning"}: + classification = "foreign_active" + next_action = NEXT_ACTION_WAIT + reasons.append( + f"foreign active reviewer lease (session_id={owner}, " + f"phase={active.get('phase')}, freshness={freshness}); " + "do not submit; do not steal" + ) + if same_identity: + reasons.append( + "lease identity matches current reviewer but session_id differs; " + "resume only from the exact owner session_id or wait" + ) + elif not is_own and freshness == "reclaimable": + classification = "foreign_reclaimable" + next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE + reasons.append( + f"foreign reclaimable lease (session_id={owner}); clear only via " + "sanctioned gitea_release_reviewer_pr_lease when reclaimable" + ) + elif not is_own and freshness == "expired": + classification = "foreign_expired" + next_action = NEXT_ACTION_RELEASE_EXPIRED_LEASE + reasons.append( + f"foreign expired lease (session_id={owner}); use sanctioned release" + ) + else: + classification = "foreign_active" + next_action = NEXT_ACTION_WAIT + reasons.append( + f"unclassified active lease state (session_id={owner}, " + f"freshness={freshness}); wait fail-closed" + ) + + if instructed_missing_with_replacement and classification.startswith( + "foreign" + ): + classification = "instructed_lease_missing_with_replacement" + # Foreign active still means wait; reclaimable still release. + if next_action == NEXT_ACTION_WAIT: + reasons.append( + "replacement foreign lease is active — wait; " + "operator_authorized_cleanup only with explicit operator authority" + ) + else: + classification = "no_lease" + next_action = NEXT_ACTION_ACQUIRE + reasons.append("no active reviewer lease; acquire via gitea_acquire_reviewer_pr_lease") + + # Binding mismatch is a first-class blocker before submit, but does not + # erase foreign-lease wait/release guidance. Override next_action only when + # the session would otherwise be free to acquire or resume (submit path). + if binding_mismatch: + if next_action in { + NEXT_ACTION_ACQUIRE, + NEXT_ACTION_RESUME_EXACT_OWNER_SESSION, + }: + classification = "worktree_binding_mismatch" + next_action = NEXT_ACTION_REPAIR_WORKTREE_BINDING + else: + reasons.append( + "also repair worktree binding before submit " + f"(next_action remains {next_action})" + ) + + lease_summary = None + if active: + lease_summary = { + "comment_id": active.get("comment_id"), + "session_id": active.get("session_id"), + "phase": active.get("phase"), + "candidate_head": active.get("candidate_head"), + "expires_at": active.get("expires_at"), + "last_activity": active.get("last_activity"), + "freshness": active.get("freshness") + or classify_lease_freshness(active, now=now), + "reviewer_identity": active.get("reviewer_identity"), + "profile": active.get("profile"), + "worktree": active.get("worktree"), + "blocker": active.get("blocker"), + } + + return { + "pr_number": pr_number, + "classification": classification, + "next_action": next_action, + "active_lease": lease_summary, + "session_lease": session_lease, + "worktree_binding": binding_details, + "instructed_session_id": instructed_session_id, + "instructed_comment_id": instructed_comment_id, + "instructed_lease_missing_with_replacement": instructed_missing_with_replacement, + "mutation_allowed": ( + next_action == NEXT_ACTION_RESUME_EXACT_OWNER_SESSION + and not binding_mismatch + and bool(session_lease) + ), + "reasons": reasons, + "forbidden": [ + "manual lock deletion", + "raw API bypass", + "mtime manipulation", + "direct _SESSION_LEASE seeding", + "silent foreign lease steal", + ], + } diff --git a/tests/test_reviewer_pr_lease.py b/tests/test_reviewer_pr_lease.py index 875d562..61efab1 100644 --- a/tests/test_reviewer_pr_lease.py +++ b/tests/test_reviewer_pr_lease.py @@ -237,5 +237,113 @@ class TestReviewerLeaseMcpGate(unittest.TestCase): self.assertTrue(any("lease" in r.lower() for r in reasons)) + +class TestReviewerLeaseHandoffDiagnose(unittest.TestCase): + """#599: canonical next-action diagnosis for open-PR lease handoff.""" + + def setUp(self): + leases.clear_session_lease() + + def test_no_lease_next_action_acquire(self): + result = leases.diagnose_reviewer_pr_lease_handoff( + [], + pr_number=592, + current_session_id="me-1", + current_reviewer_identity="sysadmin", + proposed_worktree="branches/review-pr-592", + ) + self.assertEqual(result["classification"], "no_lease") + self.assertEqual(result["next_action"], leases.NEXT_ACTION_ACQUIRE) + self.assertIsNone(result["active_lease"]) + + def test_foreign_active_wait_pr592_style(self): + # Instructed lease gone; newer foreign claim is active. + old = _lease_comment(592, "23139-da018dab43ba", phase="released") + old["id"] = 8640 + active = _lease_comment(592, "51515-2bfb70f0685f", phase="claimed") + active["id"] = 8647 + result = leases.diagnose_reviewer_pr_lease_handoff( + [old, active], + pr_number=592, + current_session_id="fresh-session", + current_reviewer_identity="sysadmin", + proposed_worktree="branches/review-pr-592-issue-590", + instructed_session_id="23139-da018dab43ba", + instructed_comment_id=8640, + ) + self.assertEqual( + result["classification"], + "instructed_lease_missing_with_replacement", + ) + self.assertEqual(result["next_action"], leases.NEXT_ACTION_WAIT) + self.assertTrue(result["instructed_lease_missing_with_replacement"]) + self.assertEqual(result["active_lease"]["session_id"], "51515-2bfb70f0685f") + self.assertEqual(result["active_lease"]["comment_id"], 8647) + self.assertFalse(result["mutation_allowed"]) + + def test_foreign_reclaimable_release_expired(self): + reclaim = _lease_comment( + 592, "foreign-old", phase="claimed", minutes_ago=65 + ) + reclaim["id"] = 99 + result = leases.diagnose_reviewer_pr_lease_handoff( + [reclaim], + pr_number=592, + current_session_id="me-1", + current_reviewer_identity="sysadmin", + proposed_worktree="branches/review-pr-592", + ) + self.assertEqual(result["classification"], "foreign_reclaimable") + self.assertEqual( + result["next_action"], leases.NEXT_ACTION_RELEASE_EXPIRED_LEASE + ) + + def test_own_active_resume(self): + head = "a" * 40 + claim = _lease_comment(592, "me-1", phase="claimed", candidate_head=head) + claim["id"] = 10 + leases.record_session_lease({ + "pr_number": 592, + "session_id": "me-1", + "candidate_head": head, + "comment_id": 10, + }, lease_provenance=mla.build_lease_provenance( + source=mla.SOURCE_ACQUIRE, + comment_id=10, + )) + result = leases.diagnose_reviewer_pr_lease_handoff( + [claim], + pr_number=592, + current_session_id="me-1", + current_reviewer_identity="rev1", + proposed_worktree="branches/review-pr382", + ) + self.assertEqual(result["classification"], "own_active") + self.assertEqual( + result["next_action"], leases.NEXT_ACTION_RESUME_EXACT_OWNER_SESSION + ) + self.assertTrue(result["mutation_allowed"]) + + def test_worktree_binding_mismatch(self): + claim = _lease_comment(592, "me-1", phase="claimed") + claim["id"] = 11 + # _lease_comment uses branches/review-pr382 + result = leases.diagnose_reviewer_pr_lease_handoff( + [claim], + pr_number=592, + current_session_id="me-1", + current_reviewer_identity="rev1", + proposed_worktree="branches/review-pr-592-issue-590", + env_bound_worktree="branches/review-pr-538", + ) + self.assertEqual(result["classification"], "worktree_binding_mismatch") + self.assertEqual( + result["next_action"], leases.NEXT_ACTION_REPAIR_WORKTREE_BINDING + ) + self.assertFalse(result["worktree_binding"]["match"]) + self.assertFalse(result["mutation_allowed"]) + + + if __name__ == "__main__": unittest.main() \ No newline at end of file