diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 29248f3..2b6a6d7 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -195,6 +195,12 @@ RECONCILER_WORKTREE_ENV = "GITEA_RECONCILER_WORKTREE" import namespace_workspace_binding as nwb # noqa: E402 import mcp_namespace_health # noqa: E402 +import stale_binding_recovery # noqa: E402 + +# Worktree env bindings inherited from the parent environment at daemon boot +# (#702). A value that still equals its boot snapshot was never established by +# a sanctioned tool in this daemon's lifetime. +_BOOT_ENV_WORKTREE_BINDINGS = stale_binding_recovery.snapshot_boot_bindings() def _preflight_in_test_mode() -> bool: @@ -260,6 +266,71 @@ def _actual_profile_role() -> str: ) +def _assess_stale_active_binding(auto_recover: bool = False) -> dict: + """Classify the GITEA_ACTIVE_WORKTREE binding; recover when provably stale (#702). + + Runs during ``gitea_resolve_task_capability`` / runtime context. Clearing + is the sanctioned in-daemon mechanism and happens only for the two + provably-stale classes (missing path, superseded by a sanctioned session + lease); an uncorroborated boot-inherited binding is surfaced with the + managed-reconnect next action, never cleared silently. + """ + active_value = (os.environ.get(ACTIVE_WORKTREE_ENV) or "").strip() or None + try: + role = _actual_profile_role() + except Exception: + role = "author" + role_env_key = nwb.ROLE_WORKTREE_ENVS.get(role, AUTHOR_WORKTREE_ENV) + boot_value = ( + (_BOOT_ENV_WORKTREE_BINDINGS.get("active_worktree") or "").strip() or None + ) + classification = stale_binding_recovery.classify_active_worktree_binding( + active_value=active_value, + role_env_value=(os.environ.get(role_env_key) or "").strip() or None, + session_lease_worktree=_reviewer_session_worktree(), + boot_inherited=bool(active_value and boot_value == active_value), + path_exists=os.path.isdir(active_value) if active_value else None, + role_kind=role, + ) + plan = stale_binding_recovery.plan_recovery(classification) + applied: dict = {"performed": False} + recovery_enabled = ( + not _preflight_in_test_mode() + or os.environ.get("GITEA_FORCE_STALE_BINDING_RECOVERY") == "1" + ) + if auto_recover and plan.get("clear_allowed") and recovery_enabled: + applied = stale_binding_recovery.apply_recovery(plan) + if applied.get("performed"): + try: + import mcp_session_state as mss + + mss.save_state( + kind=mss.KIND_STALE_BINDING_RECOVERY, + payload={ + "cleared_env": applied.get("cleared_env"), + "cleared_value": applied.get("cleared_value"), + "classification": applied.get("classification"), + "reasons": applied.get("reasons"), + }, + ) + except Exception: + pass + report = { + "classification": classification.get("classification"), + "active_worktree": classification.get("active_worktree"), + "boot_inherited": classification.get("boot_inherited", False), + "clear_eligible": bool(classification.get("clear_eligible")), + "recovery_action": plan.get("action"), + "recovery_performed": bool(applied.get("performed")), + "reasons": classification.get("reasons") or [], + } + if applied.get("performed"): + report["cleared_value"] = applied.get("cleared_value") + if plan.get("exact_next_action"): + report["exact_next_action"] = plan.get("exact_next_action") + return report + + def _resolve_preflight_workspace_path(worktree_path: str | None = None) -> str: """Resolve the namespace-scoped workspace root inspected by pre-flight guards.""" role = _effective_workspace_role() @@ -8052,6 +8123,13 @@ def gitea_diagnose_reviewer_pr_lease_handoff( except Exception: worktree_clean = None + # #702: derive owner-process evidence from the durable session-lease + # shadow left behind by a crashed daemon. Only a provably dead recorded + # owner pid yields False; PID liveness alone never proves ownership. + orphan_evidence = reviewer_pr_lease.assess_crashed_session_lease_orphan( + reviewer_pr_lease.load_session_lease_shadow(), lease=active + ) + diagnosis = reviewer_pr_lease.diagnose_reviewer_pr_lease_handoff( comments, pr_number=pr_number, @@ -8065,8 +8143,9 @@ def gitea_diagnose_reviewer_pr_lease_handoff( formal_reviews=formal_reviews, worktree_exists=worktree_exists, worktree_clean=worktree_clean, - owner_process_alive=None, # PID never proves ownership; leave unset + owner_process_alive=orphan_evidence.get("owner_process_alive"), ) + diagnosis["crashed_session_lease_evidence"] = orphan_evidence diagnosis["success"] = True diagnosis["remote"] = remote diagnosis["authenticated_user"] = username @@ -8296,6 +8375,12 @@ def gitea_cleanup_obsolete_reviewer_comment_lease( worktree_clean = None session = reviewer_pr_lease.get_session_lease() or {} + # #702: when the caller supplies no owner evidence, consult the durable + # session-lease shadow; an explicitly passed value always wins. + if owner_process_alive is None: + owner_process_alive = reviewer_pr_lease.assess_crashed_session_lease_orphan( + reviewer_pr_lease.load_session_lease_shadow(), lease=lease + ).get("owner_process_alive") assessment = reviewer_pr_lease.assess_obsolete_reviewer_comment_lease_cleanup( comments, pr_number=pr_number, @@ -10145,6 +10230,17 @@ def gitea_get_runtime_context( PROJECT_ROOT), } + # #702: read-only visibility into the inherited GITEA_ACTIVE_WORKTREE + # binding; recovery itself runs during capability resolution. + try: + stale_binding = _assess_stale_active_binding(auto_recover=False) + if stale_binding.get("classification") != ( + stale_binding_recovery.CLASSIFICATION_UNBOUND + ): + result["stale_binding_recovery"] = stale_binding + except Exception: + pass + parity = _current_master_parity() result["master_parity"] = { "in_parity": parity["in_parity"], @@ -11790,6 +11886,14 @@ def gitea_resolve_task_capability( record_mutation_authority(profile["profile_name"], username, remote if remote in REMOTES else None, task) + # #702: validate the inherited GITEA_ACTIVE_WORKTREE binding during + # capability resolution; provably-stale bindings are cleared by the + # sanctioned in-daemon mechanism with a durable audit record. + try: + stale_binding = _assess_stale_active_binding(auto_recover=True) + except Exception: + stale_binding = None + result = { "requested_task": task, "required_operation_permission": required_permission, @@ -11810,6 +11914,10 @@ def gitea_resolve_task_capability( "different_mcp_namespace_required": different_namespace_required, "exact_safe_next_action": next_safe_action, } + if stale_binding is not None and stale_binding.get("classification") != ( + stale_binding_recovery.CLASSIFICATION_UNBOUND + ): + result["stale_binding_recovery"] = stale_binding if reason_msg: result["reason"] = reason_msg if task in ("review_pr", "merge_pr"): @@ -12685,4 +12793,11 @@ if __name__ == "__main__": # processes (e.g. review_pr.py) can detect and refuse profile # side-channel overrides (#199). _export_session_profile_lock() + # #702: an auto-reconnected daemon inherits GITEA_ACTIVE_WORKTREE from its + # parent environment. Validate it at boot and clear it only when provably + # stale (missing path); anything less certain is surfaced, not cleared. + try: + _assess_stale_active_binding(auto_recover=True) + except Exception: + pass mcp.run(transport="stdio") diff --git a/mcp_session_state.py b/mcp_session_state.py index d7192ea..1698189 100644 --- a/mcp_session_state.py +++ b/mcp_session_state.py @@ -36,6 +36,15 @@ KIND_REVIEW_DRAFT = "review_draft" # other session proofs; a contaminated session fails closed on gated mutations # until a reconciler audits and clears it. KIND_STABLE_BRANCH_CONTAMINATION = "stable_branch_contamination" +# Durable shadow of the in-memory reviewer session lease (#702). Written on +# every sanctioned record/heartbeat and removed on sanctioned clear, so a +# daemon that dies without teardown leaves provable orphan evidence (owner +# pid + session id) for the guarded lease-cleanup path. Never a substitute +# for the in-session lease: mutation gates ignore it. +KIND_REVIEWER_SESSION_LEASE = "reviewer_session_lease" +# Durable audit record written whenever the daemon's sanctioned stale-binding +# recovery clears an inherited GITEA_ACTIVE_WORKTREE binding (#702). +KIND_STALE_BINDING_RECOVERY = "stale_binding_recovery" diff --git a/namespace_workspace_binding.py b/namespace_workspace_binding.py index 4831907..3f44f3d 100644 --- a/namespace_workspace_binding.py +++ b/namespace_workspace_binding.py @@ -56,23 +56,46 @@ def resolve_namespace_workspace( env: dict[str, str] | os._Environ | None = None, session_lease_worktree: str | None = None, profile_name: str | None = None, + demotions: list[str] | None = None, + verify_paths: bool = False, ) -> tuple[str, str]: - """Return ``(resolved_path, binding_source)`` for *role_kind*.""" + """Return ``(resolved_path, binding_source)`` for *role_kind*. + + With *verify_paths*, env-sourced candidates whose path no longer exists + are demoted (#702): a binding to a deleted worktree can never name a + valid task workspace, so resolution falls through to the next candidate. + Explicit arguments are never demoted — a caller-declared path must fail + loudly downstream rather than silently rebind. Demotion notes are + appended to *demotions* when provided. Runtime-context and mutation + guards resolve through :func:`resolve_namespace_mutation_context`, which + always verifies. + """ env_map = env if env is not None else os.environ role = normalize_role_kind(role_kind, profile_name=profile_name) role_env_key = ROLE_WORKTREE_ENVS[role] - for candidate, source in ( - (worktree_path, "worktree_path argument"), - (worktree, "worktree argument"), - (_env_value(env_map, ACTIVE_WORKTREE_ENV), f"{ACTIVE_WORKTREE_ENV} environment variable"), - (_env_value(env_map, role_env_key), f"{role_env_key} environment variable"), + for candidate, source, env_sourced in ( + (worktree_path, "worktree_path argument", False), + (worktree, "worktree argument", False), + (_env_value(env_map, ACTIVE_WORKTREE_ENV), + f"{ACTIVE_WORKTREE_ENV} environment variable", True), + (_env_value(env_map, role_env_key), + f"{role_env_key} environment variable", True), (session_lease_worktree if role in {"reviewer", "merger"} else None, - "reviewer PR lease worktree"), + "reviewer PR lease worktree", False), ): text = (candidate or "").strip() - if text: - return os.path.realpath(os.path.abspath(text)), source + if not text: + continue + real = os.path.realpath(os.path.abspath(text)) + if verify_paths and env_sourced and not os.path.isdir(real): + if demotions is not None: + demotions.append( + f"{source} '{real}' demoted: path no longer exists " + "(stale binding, #702)" + ) + continue + return real, source return os.path.realpath(process_project_root), "MCP server process root (default)" @@ -88,6 +111,7 @@ def resolve_namespace_mutation_context( profile_name: str | None = None, ) -> dict: """Shared workspace resolution for runtime_context and mutation guards.""" + demotions: list[str] = [] workspace, binding_source = resolve_namespace_workspace( role_kind=role_kind, worktree_path=worktree_path, @@ -96,6 +120,8 @@ def resolve_namespace_mutation_context( env=env, session_lease_worktree=session_lease_worktree, profile_name=profile_name, + demotions=demotions, + verify_paths=True, ) process_root = os.path.realpath(process_project_root) role = normalize_role_kind(role_kind, profile_name=profile_name) @@ -111,7 +137,7 @@ def resolve_namespace_mutation_context( "workspace_path": workspace, "workspace_binding_source": binding_source, "workspace_role_kind": role, - "ignored_bindings": pollution.get("ignored_bindings") or [], + "ignored_bindings": demotions + (pollution.get("ignored_bindings") or []), "process_project_root": process_root, "canonical_repo_root": canonical_root, "roots_aligned": canonical_root == process_root, diff --git a/reviewer_pr_lease.py b/reviewer_pr_lease.py index bdbc1c4..5cdf004 100644 --- a/reviewer_pr_lease.py +++ b/reviewer_pr_lease.py @@ -407,18 +407,150 @@ def record_session_lease( if lease_provenance: stored["lease_provenance"] = dict(lease_provenance) _SESSION_LEASE = stored + _persist_session_lease_shadow(stored) return dict(_SESSION_LEASE) def clear_session_lease() -> None: global _SESSION_LEASE _SESSION_LEASE = None + _persist_session_lease_shadow(None) def get_session_lease() -> dict[str, Any] | None: return dict(_SESSION_LEASE) if _SESSION_LEASE else None +def _persist_session_lease_shadow(lease: dict[str, Any] | None) -> None: + """Best-effort durable shadow of the in-session lease (#702). + + A daemon that dies without teardown leaves this record behind, giving a + later process provable orphan evidence (owner pid + session id) for the + guarded cleanup path. Observability only — mutation gates never read it, + and failures here must never block lease operations. + """ + try: + import mcp_session_state as mss + + if lease is None: + mss.clear_state(kind=mss.KIND_REVIEWER_SESSION_LEASE) + return + mss.save_state( + kind=mss.KIND_REVIEWER_SESSION_LEASE, + payload={ + "pr_number": lease.get("pr_number"), + "session_id": lease.get("session_id"), + "comment_id": lease.get("comment_id"), + "candidate_head": lease.get("candidate_head"), + "worktree": lease.get("worktree"), + "phase": lease.get("phase"), + "reviewer_identity": lease.get("reviewer_identity"), + "profile": lease.get("profile"), + "lease_repo": lease.get("repo"), + "owner_pid": os.getpid(), + }, + ) + except Exception: + pass + + +def load_session_lease_shadow() -> dict[str, Any] | None: + """Load the durable session-lease shadow left by this profile identity.""" + try: + import mcp_session_state as mss + + return mss.load_state(kind=mss.KIND_REVIEWER_SESSION_LEASE) + except Exception: + return None + + +def _pid_alive(pid: int) -> bool: + """Probe process existence; fail closed toward 'alive' when unsure.""" + try: + os.kill(int(pid), 0) + return True + except ProcessLookupError: + return False + except PermissionError: + return True + except Exception: + return True + + +def assess_crashed_session_lease_orphan( + shadow: dict[str, Any] | None, + *, + lease: dict[str, Any] | None, + current_pid: int | None = None, + pid_alive: bool | None = None, +) -> dict[str, Any]: + """Derive owner-process evidence for a durable lease from the shadow (#702). + + Returns ``owner_process_alive`` tri-state. Only a provably dead recorded + owner pid yields ``False`` (a dead pid cannot still be running the owning + daemon); an alive pid is *not* ownership proof and stays ``None`` unless + it is this very process. Session ids must match exactly — the shadow of a + different lease proves nothing. + """ + reasons: list[str] = [] + if not shadow or not lease: + return { + "orphan_evidence": False, + "owner_process_alive": None, + "reasons": ["no durable session-lease shadow evidence available"], + } + shadow_sid = (shadow.get("session_id") or "").strip() + lease_sid = (lease.get("session_id") or "").strip() + if not shadow_sid or not lease_sid or shadow_sid != lease_sid: + return { + "orphan_evidence": False, + "owner_process_alive": None, + "reasons": [ + "session-lease shadow session_id does not match the durable " + f"lease (shadow={shadow_sid or None}, lease={lease_sid or None})" + ], + } + owner_pid = shadow.get("owner_pid") or shadow.get("writer_pid") + try: + owner_pid = int(owner_pid) + except (TypeError, ValueError): + return { + "orphan_evidence": False, + "owner_process_alive": None, + "reasons": ["session-lease shadow has no usable owner pid (fail closed)"], + } + this_pid = current_pid if current_pid is not None else os.getpid() + if owner_pid == this_pid: + return { + "orphan_evidence": False, + "owner_process_alive": True, + "owner_pid": owner_pid, + "reasons": ["shadow owner pid is the current process"], + } + alive = pid_alive if pid_alive is not None else _pid_alive(owner_pid) + if alive: + reasons.append( + f"shadow owner pid {owner_pid} observed alive; PID liveness is not " + "ownership proof — owner evidence stays unknown (fail closed)" + ) + return { + "orphan_evidence": False, + "owner_process_alive": None, + "owner_pid": owner_pid, + "reasons": reasons, + } + reasons.append( + f"shadow owner pid {owner_pid} is dead: the daemon that recorded the " + "matching session lease exited without teardown (crash orphan, #702)" + ) + return { + "orphan_evidence": True, + "owner_process_alive": False, + "owner_pid": owner_pid, + "reasons": reasons, + } + + def assess_mutation_lease_gate( *, pr_number: int, @@ -557,6 +689,7 @@ _HANDOFF_CLASSIFICATIONS = frozenset({ "foreign_completed_superseded_head", "foreign_expired_superseded_head", "orphaned_owner_missing", + "orphaned_expired_superseded_head", "ambiguous_conflicting_evidence", "instructed_lease_missing_with_replacement", "worktree_binding_mismatch", @@ -850,6 +983,29 @@ def assess_obsolete_reviewer_comment_lease_cleanup( classification = "foreign_expired_superseded_head" elif head_superseded and has_terminal and not past_expiry: classification = "foreign_completed_superseded_head" + elif head_superseded and not has_terminal and past_expiry: + # Crash-orphan shape (#702): the lease outlived its head with no + # formal terminal review and has now passed canonical expiry, so + # it no longer gates fresh acquisition. Guarded cleanup of the + # ledger marker still demands provable owner-process-exit + # evidence and a safe worktree — never inferred. + classification = "orphaned_expired_superseded_head" + if owner_process_alive is True: + fail_closed_reasons.append( + "lease past expiry on superseded head but owner process " + "still alive; cleanup denied" + ) + elif owner_process_alive is None: + fail_closed_reasons.append( + "expired superseded-head lease with no terminal review: " + "cleanup requires provable owner-process-exit evidence " + "(owner_process_alive=false); the expired lease no longer " + "gates fresh acquisition" + ) + elif worktree_clean is False: + fail_closed_reasons.append( + "owner process absent but worktree dirty; cleanup denied" + ) elif head_superseded and not has_terminal: classification = "ambiguous_conflicting_evidence" fail_closed_reasons.append( @@ -891,6 +1047,7 @@ def assess_obsolete_reviewer_comment_lease_cleanup( "foreign_expired_superseded_head", "foreign_expired_current_head", "orphaned_owner_missing", + "orphaned_expired_superseded_head", }: fail_closed_reasons.append( "controller/recovery capability required " @@ -909,6 +1066,7 @@ def assess_obsolete_reviewer_comment_lease_cleanup( "foreign_completed_superseded_head", "foreign_expired_superseded_head", "orphaned_owner_missing", + "orphaned_expired_superseded_head", "foreign_expired_current_head", } # foreign_expired_current_head needs orphan/clean worktree + authority. @@ -1181,6 +1339,30 @@ def diagnose_reviewer_pr_lease_handoff( "foreign lease pinned to superseded head with completed formal " "review; use guarded obsolete-lease cleanup (do not wait indefinitely)" ) + elif not is_own and head_superseded and not terminal and past_expiry: + # Crash-orphan shape after canonical expiry (#702): the expired + # marker no longer gates acquisition, so a fresh reviewer may + # acquire at the current head. Guarded ledger cleanup becomes + # available only with provable owner-exit evidence. + classification = "orphaned_expired_superseded_head" + if owner_process_alive is False and ( + worktree_clean is True or worktree_exists is False + ): + next_action = NEXT_ACTION_CLEANUP_OBSOLETE_LEASE + reasons.append( + "expired superseded-head lease with no terminal review and " + "provable owner-process exit (crash orphan, #702); guarded " + "obsolete-lease cleanup eligible" + ) + else: + next_action = NEXT_ACTION_ACQUIRE + reasons.append( + "lease head superseded and lease past canonical expiry with " + "no formal terminal review (crash orphan, #702); expired " + "marker no longer gates acquisition — acquire a fresh lease " + "at the current head; guarded cleanup needs " + "owner_process_alive=false and a clean/absent worktree" + ) elif not is_own and head_superseded and not terminal: classification = "ambiguous_conflicting_evidence" next_action = NEXT_ACTION_WAIT diff --git a/stale_binding_recovery.py b/stale_binding_recovery.py new file mode 100644 index 0000000..37b73d4 --- /dev/null +++ b/stale_binding_recovery.py @@ -0,0 +1,257 @@ +"""Sanctioned recovery for stale env-bound task workspace bindings (#702). + +When the MCP daemon dies from an unhandled exception it never runs teardown, +and the auto-reconnected daemon inherits ``GITEA_ACTIVE_WORKTREE`` from its +parent environment. That inherited value can permanently bind the fresh +session to a prior task's worktree (the PR #701 / ``review-pr-654`` incident). + +This module classifies the active env binding against live session evidence +and produces a fail-closed recovery plan. Only two situations permit the +daemon to clear the binding on its own: + +- the bound path no longer exists on disk (``provably_stale_missing_path``) +- the binding was inherited at daemon boot and a *sanctioned* in-session + reviewer/merger lease later bound a different worktree + (``superseded_by_session_lease``) + +Everything else is surfaced (``unverified_inherited``) and left for the +managed reconnect path — never cleared silently, never rebound to a guessed +worktree. +""" + +from __future__ import annotations + +import os +from typing import Any + +ACTIVE_WORKTREE_ENV = "GITEA_ACTIVE_WORKTREE" + +CLASSIFICATION_UNBOUND = "unbound" +CLASSIFICATION_CORROBORATED = "corroborated" +CLASSIFICATION_UNVERIFIED_INHERITED = "unverified_inherited" +CLASSIFICATION_PROVABLY_STALE_MISSING_PATH = "provably_stale_missing_path" +CLASSIFICATION_SUPERSEDED_BY_SESSION_LEASE = "superseded_by_session_lease" + +CLASSIFICATIONS = frozenset({ + CLASSIFICATION_UNBOUND, + CLASSIFICATION_CORROBORATED, + CLASSIFICATION_UNVERIFIED_INHERITED, + CLASSIFICATION_PROVABLY_STALE_MISSING_PATH, + CLASSIFICATION_SUPERSEDED_BY_SESSION_LEASE, +}) + +# Roles whose task workspace is owned by a lease lifecycle, so an inherited +# generic binding without a corroborating lease is suspect. +LEASE_BOUND_ROLES = frozenset({"reviewer", "merger"}) + +RECOVERY_ACTION_NONE = "none" +RECOVERY_ACTION_CLEAR_ENV = "clear_env" + + +def _norm(path: str | None) -> str: + text = (path or "").strip() + if not text: + return "" + return os.path.realpath(os.path.abspath(text)) + + +def snapshot_boot_bindings( + env: dict[str, str] | os._Environ | None = None, +) -> dict[str, Any]: + """Capture worktree env bindings present when the daemon booted. + + A value present in this snapshot was inherited from the parent + environment, not established by any sanctioned tool in this daemon's + lifetime. + """ + env_map = env if env is not None else os.environ + return { + "active_worktree": (env_map.get(ACTIVE_WORKTREE_ENV) or "").strip() or None, + } + + +def classify_active_worktree_binding( + *, + active_value: str | None, + role_env_value: str | None = None, + session_lease_worktree: str | None = None, + boot_inherited: bool, + path_exists: bool | None, + role_kind: str | None = None, +) -> dict[str, Any]: + """Classify the GITEA_ACTIVE_WORKTREE binding against live evidence. + + Fail closed: unknown evidence never yields a clear-eligible class. + """ + reasons: list[str] = [] + active = _norm(active_value) + if not active: + return { + "classification": CLASSIFICATION_UNBOUND, + "clear_eligible": False, + "active_worktree": None, + "reasons": ["no GITEA_ACTIVE_WORKTREE binding present"], + } + + role = (role_kind or "").strip().lower() + role_env = _norm(role_env_value) + lease_wt = _norm(session_lease_worktree) + + result: dict[str, Any] = { + "active_worktree": active, + "boot_inherited": bool(boot_inherited), + "role_kind": role or None, + "session_lease_worktree": lease_wt or None, + "role_env_worktree": role_env or None, + } + + if path_exists is False: + reasons.append( + f"bound worktree '{active}' no longer exists on disk; the binding " + "cannot name a valid task workspace" + ) + result.update({ + "classification": CLASSIFICATION_PROVABLY_STALE_MISSING_PATH, + "clear_eligible": True, + "reasons": reasons, + }) + return result + + if lease_wt and lease_wt == active: + reasons.append("binding matches the sanctioned in-session lease worktree") + result.update({ + "classification": CLASSIFICATION_CORROBORATED, + "clear_eligible": False, + "reasons": reasons, + }) + return result + + if lease_wt and lease_wt != active and boot_inherited: + reasons.append( + "boot-inherited binding disagrees with the sanctioned in-session " + f"lease worktree '{lease_wt}'; the lease is live authority" + ) + result.update({ + "classification": CLASSIFICATION_SUPERSEDED_BY_SESSION_LEASE, + "clear_eligible": True, + "reasons": reasons, + }) + return result + + if lease_wt and lease_wt != active and not boot_inherited: + # Set after boot by tooling; disagreement is surfaced, never auto-cleared. + reasons.append( + "post-boot binding disagrees with the in-session lease worktree; " + "surfacing only (fail closed, no automatic clear)" + ) + result.update({ + "classification": CLASSIFICATION_UNVERIFIED_INHERITED, + "clear_eligible": False, + "reasons": reasons, + }) + return result + + if role_env and role_env == active: + reasons.append("binding matches the role-specific worktree env binding") + result.update({ + "classification": CLASSIFICATION_CORROBORATED, + "clear_eligible": False, + "reasons": reasons, + }) + return result + + if boot_inherited and role in LEASE_BOUND_ROLES and not lease_wt: + reasons.append( + "binding was inherited at daemon boot, no sanctioned in-session " + f"lease corroborates it, and the {role} role binds workspaces " + "through the lease lifecycle; treat as unverified until a fresh " + "lease or managed reconnect re-establishes the workspace" + ) + result.update({ + "classification": CLASSIFICATION_UNVERIFIED_INHERITED, + "clear_eligible": False, + "reasons": reasons, + }) + return result + + reasons.append("no contradicting evidence; binding treated as corroborated") + result.update({ + "classification": CLASSIFICATION_CORROBORATED, + "clear_eligible": False, + "reasons": reasons, + }) + return result + + +def plan_recovery(classification_result: dict[str, Any]) -> dict[str, Any]: + """Turn a classification into an explicit, fail-closed recovery plan.""" + classification = classification_result.get("classification") + clear_eligible = bool(classification_result.get("clear_eligible")) + reasons = list(classification_result.get("reasons") or []) + + if classification not in CLASSIFICATIONS: + return { + "action": RECOVERY_ACTION_NONE, + "clear_allowed": False, + "classification": classification, + "reasons": reasons + ["unknown classification (fail closed)"], + } + + if clear_eligible and classification in { + CLASSIFICATION_PROVABLY_STALE_MISSING_PATH, + CLASSIFICATION_SUPERSEDED_BY_SESSION_LEASE, + }: + return { + "action": RECOVERY_ACTION_CLEAR_ENV, + "clear_allowed": True, + "classification": classification, + "active_worktree": classification_result.get("active_worktree"), + "reasons": reasons, + } + + exact_next_action = None + if classification == CLASSIFICATION_UNVERIFIED_INHERITED: + exact_next_action = ( + "managed recovery required: reconnect the MCP namespace through " + "the managed client configuration (or start a fresh session) so " + "the daemon boots without the inherited GITEA_ACTIVE_WORKTREE, or " + "corroborate the binding by acquiring a lease for that worktree; " + "do not clear or rewrite the environment manually" + ) + return { + "action": RECOVERY_ACTION_NONE, + "clear_allowed": False, + "classification": classification, + "active_worktree": classification_result.get("active_worktree"), + "reasons": reasons, + **({"exact_next_action": exact_next_action} if exact_next_action else {}), + } + + +def apply_recovery( + plan: dict[str, Any], + env: dict[str, str] | os._Environ | None = None, +) -> dict[str, Any]: + """Apply a clear-eligible plan by removing the stale env binding. + + This is the sanctioned in-daemon mechanism (#702 AC2); callers must + persist the returned record for audit. Refuses anything but an explicit + ``clear_env`` plan. + """ + env_map = env if env is not None else os.environ + if not plan.get("clear_allowed") or plan.get("action") != RECOVERY_ACTION_CLEAR_ENV: + return { + "performed": False, + "action": RECOVERY_ACTION_NONE, + "reasons": ["recovery plan does not authorize clearing (fail closed)"], + } + cleared_value = (env_map.get(ACTIVE_WORKTREE_ENV) or "").strip() or None + env_map.pop(ACTIVE_WORKTREE_ENV, None) + return { + "performed": True, + "action": RECOVERY_ACTION_CLEAR_ENV, + "cleared_env": ACTIVE_WORKTREE_ENV, + "cleared_value": cleared_value, + "classification": plan.get("classification"), + "reasons": list(plan.get("reasons") or []), + } diff --git a/tests/test_issue_691_obsolete_reviewer_lease_cleanup.py b/tests/test_issue_691_obsolete_reviewer_lease_cleanup.py index 8d4fed4..0bfd40a 100644 --- a/tests/test_issue_691_obsolete_reviewer_lease_cleanup.py +++ b/tests/test_issue_691_obsolete_reviewer_lease_cleanup.py @@ -561,12 +561,29 @@ class TestIssue691SupersededHeadCleanup(unittest.TestCase): self.assertTrue(any("owns the lease" in r for r in result["reasons"])) def test_superseded_without_terminal_review_denied(self): + # Pre-expiry the missing terminal review is ambiguous (fail closed); + # post-expiry with unknown owner evidence it is a crash-orphan (#702) + # that still fails closed until owner-process exit is proven. now = datetime(2026, 7, 13, 5, 0, 0, tzinfo=timezone.utc) expires = datetime(2026, 7, 13, 4, 23, 0, tzinfo=timezone.utc) claim = _lease_comment(expires_at=expires, last_activity=expires - timedelta(hours=1)) - result = _assess([claim], formal_reviews=[], now=now) + result = _assess( + [claim], formal_reviews=[], now=now, owner_process_alive=None + ) self.assertFalse(result["cleanup_allowed"]) - self.assertEqual(result["classification"], "ambiguous_conflicting_evidence") + self.assertEqual( + result["classification"], "orphaned_expired_superseded_head" + ) + + fresh_expires = now + timedelta(minutes=60) + fresh = _lease_comment( + expires_at=fresh_expires, last_activity=now - timedelta(minutes=5) + ) + pre_expiry = _assess([fresh], formal_reviews=[], now=now) + self.assertFalse(pre_expiry["cleanup_allowed"]) + self.assertEqual( + pre_expiry["classification"], "ambiguous_conflicting_evidence" + ) class TestIssue691DiagnoseClassifications(unittest.TestCase): diff --git a/tests/test_issue_702_stale_binding_lease_recovery.py b/tests/test_issue_702_stale_binding_lease_recovery.py new file mode 100644 index 0000000..8c3306d --- /dev/null +++ b/tests/test_issue_702_stale_binding_lease_recovery.py @@ -0,0 +1,517 @@ +"""Regression tests for #702: stale runtime binding + orphaned durable lease. + +Reproduces the PR #701 incident (lease 65664-4f248d213d60, comments +11129/11131): the MCP daemon crashed without teardown, destroying the +in-memory session lease while the durable comment lease survived, and the +auto-reconnected daemon inherited a stale ``GITEA_ACTIVE_WORKTREE`` binding +(``branches/review-pr-654``) from its parent environment. + +Covers the five mandated scenarios: + +1. lost in-session leases — durable shadow survives a crash and yields + provable owner-process evidence +2. old-head leases — expired superseded-head lease with no terminal + review becomes recoverable only with orphan evidence + controller authority +3. runtime/worktree mismatch — env bindings to deleted worktrees are demoted, + never silently bound +4. managed reconnect — boot-inherited uncorroborated bindings are + surfaced for managed recovery; provably stale ones are clear-eligible +5. safe expiry — pre-expiry stays fail-closed wait (no steal); + canonical expiry unlocks acquisition and guarded cleanup +""" + +from __future__ import annotations + +import os +import sys +import unittest +from datetime import datetime, timedelta, timezone +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import namespace_workspace_binding as nwb # noqa: E402 +import reviewer_pr_lease as leases # noqa: E402 +import stale_binding_recovery as sbr # noqa: E402 + +# PR #701 incident fixtures. +OLD_HEAD = "b4d0cb22e452a07c8e816745c704ddb0f43edf0b" +NEW_HEAD = "6b675f5c834b41f9d74e8a54294ff44dddf28ae4" +CRASHED_SESSION = "65664-4f248d213d60" +LEASE_COMMENT = 11131 +REPO = "Scaled-Tech-Consulting/Gitea-Tools" +PR = 701 +ISSUE = 699 +LEASE_WORKTREE = "branches/review-fix-issue-699-structured-auth-mcp-errors" + + +def _now() -> datetime: + return datetime.now(timezone.utc) + + +def _lease_comment( + *, + phase: str = "validation-start", + candidate_head: str = OLD_HEAD, + session_id: str = CRASHED_SESSION, + comment_id: int = LEASE_COMMENT, + last_activity: datetime | None = None, + expires_at: datetime | None = None, + worktree: str = LEASE_WORKTREE, +) -> dict: + stamp = last_activity or _now() + body = leases.format_lease_body( + repo=REPO, + pr_number=PR, + issue_number=ISSUE, + reviewer_identity="sysadmin", + profile="prgs-reviewer", + session_id=session_id, + worktree=worktree, + phase=phase, + candidate_head=candidate_head, + target_branch="master", + target_branch_sha="2" * 40, + last_activity=stamp, + expires_at=expires_at, + ) + return { + "id": comment_id, + "body": body, + "user": {"login": "sysadmin"}, + "author": "sysadmin", + "created_at": stamp.isoformat(), + "updated_at": stamp.isoformat(), + } + + +def _expired_superseded_comments() -> list[dict]: + stale = _now() - timedelta(hours=3) + return [_lease_comment( + last_activity=stale, + expires_at=stale + timedelta(minutes=120), + )] + + +def _fresh_superseded_comments() -> list[dict]: + recent = _now() - timedelta(minutes=10) + return [_lease_comment( + last_activity=recent, + expires_at=recent + timedelta(minutes=120), + )] + + +class TestLostInSessionLeaseShadow(unittest.TestCase): + """Scenario 1: the durable shadow survives a daemon crash (#702 AC3).""" + + def setUp(self): + leases._SESSION_LEASE = None + + def tearDown(self): + leases._SESSION_LEASE = None + + def _record(self) -> dict: + return leases.record_session_lease( + { + "pr_number": PR, + "issue_number": ISSUE, + "session_id": CRASHED_SESSION, + "reviewer_identity": "sysadmin", + "profile": "prgs-reviewer", + "worktree": LEASE_WORKTREE, + "phase": "claimed", + "candidate_head": OLD_HEAD, + "repo": REPO, + "comment_id": LEASE_COMMENT, + }, + lease_provenance={"source": "acquire", "comment_id": LEASE_COMMENT}, + ) + + def test_shadow_written_on_record_and_survives_simulated_crash(self): + self._record() + # Simulated unhandled crash: the in-memory dict dies with the process + # but the sanctioned clear path never runs. + leases._SESSION_LEASE = None + shadow = leases.load_session_lease_shadow() + self.assertIsNotNone(shadow) + self.assertEqual(shadow.get("session_id"), CRASHED_SESSION) + self.assertEqual(shadow.get("pr_number"), PR) + self.assertEqual(int(shadow.get("owner_pid")), os.getpid()) + + def test_sanctioned_clear_removes_shadow(self): + self._record() + leases.clear_session_lease() + self.assertIsNone(leases.load_session_lease_shadow()) + + def test_orphan_assessment_dead_owner_pid_proves_exit(self): + self._record() + leases._SESSION_LEASE = None + shadow = leases.load_session_lease_shadow() + lease = {"session_id": CRASHED_SESSION} + verdict = leases.assess_crashed_session_lease_orphan( + shadow, lease=lease, current_pid=os.getpid() + 1, pid_alive=False + ) + self.assertTrue(verdict["orphan_evidence"]) + self.assertIs(verdict["owner_process_alive"], False) + + def test_orphan_assessment_alive_pid_is_not_ownership_proof(self): + self._record() + shadow = leases.load_session_lease_shadow() + verdict = leases.assess_crashed_session_lease_orphan( + shadow, + lease={"session_id": CRASHED_SESSION}, + current_pid=os.getpid() + 1, + pid_alive=True, + ) + self.assertFalse(verdict["orphan_evidence"]) + self.assertIsNone(verdict["owner_process_alive"]) + + def test_orphan_assessment_session_mismatch_proves_nothing(self): + self._record() + shadow = leases.load_session_lease_shadow() + verdict = leases.assess_crashed_session_lease_orphan( + shadow, lease={"session_id": "other-999"}, pid_alive=False + ) + self.assertFalse(verdict["orphan_evidence"]) + self.assertIsNone(verdict["owner_process_alive"]) + + def test_orphan_assessment_without_shadow_is_unknown(self): + verdict = leases.assess_crashed_session_lease_orphan( + None, lease={"session_id": CRASHED_SESSION} + ) + self.assertFalse(verdict["orphan_evidence"]) + self.assertIsNone(verdict["owner_process_alive"]) + + +class TestOldHeadLeaseCleanup(unittest.TestCase): + """Scenario 2: expired superseded-head lease with no terminal review.""" + + def _assess(self, comments, **kwargs): + defaults = dict( + pr_number=PR, + current_head_sha=NEW_HEAD, + formal_reviews=[], + repo=REPO, + expected_repo=REPO, + requesting_session_id="fresh-controller", + controller_recovery_authorized=True, + worktree_exists=True, + worktree_clean=True, + owner_process_alive=False, + ) + defaults.update(kwargs) + return leases.assess_obsolete_reviewer_comment_lease_cleanup( + comments, **defaults + ) + + def test_expired_orphan_with_full_evidence_is_cleanup_eligible(self): + result = self._assess(_expired_superseded_comments()) + self.assertEqual(result["classification"], "orphaned_expired_superseded_head") + self.assertTrue(result["cleanup_allowed"]) + self.assertIn("phase: released", result["release_body"] or "") + self.assertEqual( + result["exact_next_action"], "cleanup_obsolete_reviewer_comment_lease" + ) + + def test_expired_orphan_without_owner_evidence_fails_closed(self): + result = self._assess( + _expired_superseded_comments(), owner_process_alive=None + ) + self.assertFalse(result["cleanup_allowed"]) + self.assertIn( + "provable owner-process-exit evidence", + " ".join(result["fail_closed_reasons"]), + ) + + def test_expired_orphan_without_controller_authority_fails_closed(self): + result = self._assess( + _expired_superseded_comments(), controller_recovery_authorized=False + ) + self.assertFalse(result["cleanup_allowed"]) + + def test_expired_orphan_with_dirty_worktree_fails_closed(self): + result = self._assess( + _expired_superseded_comments(), + worktree_clean=False, + worktree_has_unpreserved_work=True, + ) + self.assertFalse(result["cleanup_allowed"]) + + def test_unexpired_superseded_no_terminal_stays_ambiguous_wait(self): + # Regression guard: pre-expiry there is still no steal path. + result = self._assess(_fresh_superseded_comments()) + self.assertEqual(result["classification"], "ambiguous_conflicting_evidence") + self.assertFalse(result["cleanup_allowed"]) + self.assertEqual(result["exact_next_action"], "wait") + + +class TestRuntimeWorktreeMismatch(unittest.TestCase): + """Scenario 3: env bindings to deleted worktrees are demoted (#702 AC2).""" + + def test_missing_active_env_binding_is_demoted(self): + demotions: list[str] = [] + missing = "/nonexistent/branches/review-pr-654" + path, source = nwb.resolve_namespace_workspace( + role_kind="reviewer", + process_project_root=os.getcwd(), + env={"GITEA_ACTIVE_WORKTREE": missing}, + demotions=demotions, + verify_paths=True, + ) + self.assertEqual(source, "MCP server process root (default)") + self.assertEqual(len(demotions), 1) + self.assertIn("no longer exists", demotions[0]) + + def test_missing_active_falls_through_to_existing_role_env(self): + existing = os.getcwd() + path, source = nwb.resolve_namespace_workspace( + role_kind="reviewer", + process_project_root="/tmp", + env={ + "GITEA_ACTIVE_WORKTREE": "/nonexistent/branches/review-pr-654", + "GITEA_REVIEWER_WORKTREE": existing, + }, + verify_paths=True, + ) + self.assertEqual(path, os.path.realpath(existing)) + self.assertEqual(source, "GITEA_REVIEWER_WORKTREE environment variable") + + def test_existing_active_env_binding_still_wins(self): + existing = os.getcwd() + path, source = nwb.resolve_namespace_workspace( + role_kind="reviewer", + process_project_root="/tmp", + env={"GITEA_ACTIVE_WORKTREE": existing}, + verify_paths=True, + ) + self.assertEqual(path, os.path.realpath(existing)) + self.assertEqual(source, "GITEA_ACTIVE_WORKTREE environment variable") + + def test_explicit_worktree_path_argument_is_never_demoted(self): + missing = "/nonexistent/branches/explicit" + path, source = nwb.resolve_namespace_workspace( + role_kind="reviewer", + worktree_path=missing, + process_project_root="/tmp", + env={}, + verify_paths=True, + ) + self.assertEqual(source, "worktree_path argument") + + def test_mutation_context_reports_demotion_in_ignored_bindings(self): + ctx = nwb.resolve_namespace_mutation_context( + role_kind="reviewer", + worktree_path=None, + process_project_root=os.getcwd(), + env={"GITEA_ACTIVE_WORKTREE": "/nonexistent/branches/review-pr-654"}, + ) + joined = " ".join(ctx["ignored_bindings"]) + self.assertIn("stale binding, #702", joined) + + +class TestManagedReconnectRecovery(unittest.TestCase): + """Scenario 4: boot-inherited bindings and the sanctioned recovery plan.""" + + def test_uncorroborated_inherited_reviewer_binding_is_unverified(self): + classification = sbr.classify_active_worktree_binding( + active_value=os.getcwd(), + role_env_value=None, + session_lease_worktree=None, + boot_inherited=True, + path_exists=True, + role_kind="reviewer", + ) + self.assertEqual( + classification["classification"], sbr.CLASSIFICATION_UNVERIFIED_INHERITED + ) + self.assertFalse(classification["clear_eligible"]) + plan = sbr.plan_recovery(classification) + self.assertFalse(plan["clear_allowed"]) + self.assertIn("managed", plan["exact_next_action"]) + self.assertIn("do not clear or rewrite", plan["exact_next_action"]) + + def test_missing_path_binding_is_provably_stale_and_clear_eligible(self): + classification = sbr.classify_active_worktree_binding( + active_value="/nonexistent/branches/review-pr-654", + boot_inherited=True, + path_exists=False, + role_kind="reviewer", + ) + self.assertEqual( + classification["classification"], + sbr.CLASSIFICATION_PROVABLY_STALE_MISSING_PATH, + ) + plan = sbr.plan_recovery(classification) + self.assertTrue(plan["clear_allowed"]) + + def test_inherited_binding_superseded_by_session_lease_is_clear_eligible(self): + classification = sbr.classify_active_worktree_binding( + active_value="/tmp", + session_lease_worktree=os.getcwd(), + boot_inherited=True, + path_exists=True, + role_kind="reviewer", + ) + self.assertEqual( + classification["classification"], + sbr.CLASSIFICATION_SUPERSEDED_BY_SESSION_LEASE, + ) + self.assertTrue(sbr.plan_recovery(classification)["clear_allowed"]) + + def test_post_boot_binding_conflict_is_surfaced_not_cleared(self): + classification = sbr.classify_active_worktree_binding( + active_value="/tmp", + session_lease_worktree=os.getcwd(), + boot_inherited=False, + path_exists=True, + role_kind="reviewer", + ) + self.assertEqual( + classification["classification"], sbr.CLASSIFICATION_UNVERIFIED_INHERITED + ) + self.assertFalse(sbr.plan_recovery(classification)["clear_allowed"]) + + def test_corroborated_bindings_are_untouched(self): + for kwargs in ( + dict(role_env_value=os.getcwd()), + dict(session_lease_worktree=os.getcwd()), + ): + classification = sbr.classify_active_worktree_binding( + active_value=os.getcwd(), + boot_inherited=True, + path_exists=True, + role_kind="reviewer", + **kwargs, + ) + self.assertEqual( + classification["classification"], sbr.CLASSIFICATION_CORROBORATED + ) + self.assertFalse(sbr.plan_recovery(classification)["clear_allowed"]) + + def test_author_inherited_binding_stays_corroborated(self): + classification = sbr.classify_active_worktree_binding( + active_value=os.getcwd(), + boot_inherited=True, + path_exists=True, + role_kind="author", + ) + self.assertEqual( + classification["classification"], sbr.CLASSIFICATION_CORROBORATED + ) + + def test_apply_recovery_clears_env_only_for_authorized_plan(self): + env = {"GITEA_ACTIVE_WORKTREE": "/nonexistent/branches/review-pr-654"} + plan = sbr.plan_recovery( + sbr.classify_active_worktree_binding( + active_value=env["GITEA_ACTIVE_WORKTREE"], + boot_inherited=True, + path_exists=False, + role_kind="reviewer", + ) + ) + applied = sbr.apply_recovery(plan, env=env) + self.assertTrue(applied["performed"]) + self.assertNotIn("GITEA_ACTIVE_WORKTREE", env) + self.assertIn("review-pr-654", applied["cleared_value"]) + + def test_apply_recovery_refuses_unauthorized_plan(self): + env = {"GITEA_ACTIVE_WORKTREE": os.getcwd()} + plan = sbr.plan_recovery( + sbr.classify_active_worktree_binding( + active_value=env["GITEA_ACTIVE_WORKTREE"], + boot_inherited=True, + path_exists=True, + role_kind="reviewer", + ) + ) + applied = sbr.apply_recovery(plan, env=env) + self.assertFalse(applied["performed"]) + self.assertIn("GITEA_ACTIVE_WORKTREE", env) + + +class TestSafeExpiryDiagnosis(unittest.TestCase): + """Scenario 5: canonical expiry flips wait into acquire/guarded cleanup.""" + + def _diagnose(self, comments, **kwargs): + defaults = dict( + pr_number=PR, + current_session_id=None, + current_reviewer_identity="fresh-reviewer", + current_head_sha=NEW_HEAD, + formal_reviews=[], + ) + defaults.update(kwargs) + return leases.diagnose_reviewer_pr_lease_handoff(comments, **defaults) + + def setUp(self): + leases._SESSION_LEASE = None + + def test_pre_expiry_superseded_no_terminal_stays_wait(self): + diagnosis = self._diagnose(_fresh_superseded_comments()) + self.assertEqual( + diagnosis["classification"], "ambiguous_conflicting_evidence" + ) + self.assertEqual(diagnosis["next_action"], "wait") + + def test_post_expiry_without_orphan_evidence_unlocks_acquire(self): + diagnosis = self._diagnose(_expired_superseded_comments()) + self.assertEqual( + diagnosis["classification"], "orphaned_expired_superseded_head" + ) + self.assertEqual(diagnosis["next_action"], "acquire") + + def test_post_expiry_with_orphan_evidence_offers_guarded_cleanup(self): + diagnosis = self._diagnose( + _expired_superseded_comments(), + owner_process_alive=False, + worktree_clean=True, + worktree_exists=True, + ) + self.assertEqual( + diagnosis["classification"], "orphaned_expired_superseded_head" + ) + self.assertEqual( + diagnosis["next_action"], "cleanup_obsolete_reviewer_comment_lease" + ) + + def test_expired_lease_no_longer_blocks_fresh_acquisition(self): + assessment = leases.assess_acquire_lease( + _expired_superseded_comments(), + pr_number=PR, + reviewer_identity="fresh-reviewer", + profile="prgs-reviewer", + session_id="99999-fresh", + repo=REPO, + issue_number=ISSUE, + worktree="branches/review-pr-701-fresh", + candidate_head=NEW_HEAD, + target_branch="master", + target_branch_sha="2" * 40, + ) + self.assertTrue(assessment.get("acquire_allowed")) + + def test_unparseable_expiry_fails_closed_in_cleanup(self): + comment = _lease_comment( + last_activity=_now() - timedelta(hours=3), expires_at=None + ) + comment["body"] = comment["body"].replace( + "expires_at: ", "expires_at: not-a-timestamp" + ) + result = leases.assess_obsolete_reviewer_comment_lease_cleanup( + [comment], + pr_number=PR, + current_head_sha=NEW_HEAD, + formal_reviews=[], + repo=REPO, + expected_repo=REPO, + requesting_session_id="fresh-controller", + controller_recovery_authorized=True, + worktree_exists=True, + worktree_clean=True, + owner_process_alive=False, + ) + self.assertFalse(result["cleanup_allowed"]) + + +if __name__ == "__main__": + unittest.main()