fix(reconcile): server-enforced, revalidated missing-worktree cleanup (#970 review 644 B1-B5)

Addresses the five blocking findings of review 644 on PR #972. B1 — live ownership and status revalidation. resolve_missing_worktree_binding now re-reads the authoritative lease, session, checkpoint, and issue-lock rows immediately before mutating and diffs them against the snapshot the audit recorded (binding path and identity, lease id/status/session/owner pid, checkpoint path/status, live-session evidence, trustworthy ownership evidence, issue-lock state). Any drift fails closed without mutation, and the binding is reclassified from the live values rather than the audit snapshot. A candidate carrying no audited snapshot is refused rather than trusted. B2 — server-enforced cleanup authorization. Apply mode no longer accepts a client-supplied operator_authorized boolean; it is rejected outright at the MCP tool and in the module (#709 F1 / review 434). Authorization is now the project's own reconciliation cleanup gate, required at both the task-capability boundary (new reconciler-only reconcile_missing_worktree_bindings capability, gitea.branch.delete, role-exclusive) and the production mutation boundary (an authorized audit_reconciliation_mode cleanup phase, re-checked at the point of mutation so a forged authorization mapping cannot stand in for the gate). Dry-run remains available to any gitea.read profile and stays non-mutating. Existing role, repository, parity, and provenance gates are unchanged. B3 — expected-path compare-and-swap. retire_session_checkpoint_worktree_path now requires expected_path and performs a guarded update keyed on the stored path, refusing without mutation when the stored path was moved, replaced, or concurrently changed, when the row is unknown, or when a selector matches more than one checkpoint. retire_lease_worktree_path gains the same treatment plus optional status/session/owner-pid compare-and-swap, and its UPDATE is keyed on the audited path. Both report an idempotent already_retired outcome instead of falsely reporting a retirement. B4 — live-session and issue-lock evidence. session_active is now derived from the control-plane sessions table instead of never being set, along two axes: genuine liveness (recorded active, PID not dead, heartbeat fresh — the rule reused from restart_coordinator) and weaker but still trustworthy recorded ownership. A non-terminal lease now protects its binding regardless of whether the recorded PID is alive, so dead-PID evidence alone can no longer retire a lease the control plane still holds. The previously unused issue_lock_store is now read: a live durable issue lock binding the path or branch blocks cleanup, and locks whose own paths are missing are reported for release through their own lifecycle rather than retired here. B5 — adversarial regression coverage. The suite now drives the registered MCP tools through mcp_server, the real ControlPlaneDB, and the real cleanup gate, covering lease status/ownership/session/path drift, expected-path mismatch, concurrent recreation, an unauthorized caller submitting operator_authorized, wrong profile and missing capability, live-session and trustworthy-owner evidence, conflicting issue locks, non-mutating dry-run, exact-binding-only retirement, preservation of unrelated worktrees and git metadata, idempotent re-execution, and worktrees-dimension resolution. All original #970 acceptance criteria are preserved, including the distinctions between deleted paths, moved paths, unavailable hosts or mounts, transient filesystem failures, live ownership, and concurrent recreation. Tests: focused #970 suite 47 passed. Adjacent suites (capability role invariants, audit reconciliation mode, control plane DB, lease lifecycle, reconciler cleanup integration, delete-branch capability, restart coordinator, bootstrap lock contract) 252 passed / 93 subtests. Full suite 28 failed / 6000 passed, an exact match of the pre-change baseline's 28 failing test ids at 3f584352 (28 failed / 5960 passed). Closes #970 Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
2026-07-29 06:31:52 -05:00
parent 3f584352df
commit c763161702
6 changed files with 1930 additions and 104 deletions
@@ -280,6 +280,22 @@ def _ts(dt: datetime | None = None) -> str:
    return value.astimezone(timezone.utc).replace(microsecond=0).isoformat().replace("+00:00", "Z")


+def _realpath_or_raw(value: str | None) -> str:
+    """Normalize a filesystem path for compare-and-swap equality (#970).
+
+    Symlinks and ``..`` segments must not make two spellings of the same path
+    look different, but an unresolvable path must still compare as itself
+    rather than collapsing to empty — an empty result means "no path given".
+    """
+    text = (value or "").strip()
+    if not text:
+        return ""
+    try:
+        return os.path.realpath(os.path.abspath(text))
+    except Exception:
+        return text
+
+
 def _parse_ts(value: str | None) -> datetime | None:
    if not value:
        return None
@@ -1918,16 +1934,36 @@ class ControlPlaneDB:
        lease_id: str,
        *,
        expected_path: str | None = None,
+        expected_status: str | None = None,
+        expected_session_id: str | None = None,
+        expected_owner_pid: int | None = None,
        reason: str = "missing_worktree_path_retired",
    ) -> dict[str, Any]:
        """Retire a missing worktree_path binding from a control-plane lease (#970).

        Clears worktree_path on the lease row, updates provenance_json with
-        durable retirement audit proof, and writes a worktree_binding_retired event.
-        Fail closed: if expected_path is provided and does not match current
-        worktree_path, the update is refused to prevent racing mutations.
+        durable retirement audit proof, and writes a worktree_binding_retired
+        event.
+
+        The update is a compare-and-swap (#970 review 644 B1/B3): the caller
+        states the exact path it audited and, when known, the lease status,
+        owning session, and owner pid it classified against. Every stated value
+        must still match the stored row, and the ``UPDATE`` itself is keyed on
+        the stored ``worktree_path``, so a concurrent writer that moved or
+        replaced the binding between audit and apply loses the race instead of
+        having its value silently overwritten. A mismatch raises and mutates
+        nothing.
+
+        ``expected_path`` is mandatory: a retirement that does not name the path
+        it intends to clear cannot be safe against concurrent recreation.
        """
        now_s = _ts()
+        expected_norm = _realpath_or_raw(expected_path)
+        if not expected_norm:
+            raise ControlPlaneError(
+                f"cannot retire lease {lease_id} worktree_path: expected_path is "
+                "required for compare-and-swap retirement (fail closed)"
+            )
        with self._tx() as conn:
            cols = self._lease_columns(conn)
            row = conn.execute(
@@ -1937,15 +1973,56 @@ class ControlPlaneDB:
            if not row:
                raise ControlPlaneError(f"unknown lease_id {lease_id}")

-            current_wt = (dict(row).get("worktree_path") or "").strip()
-            if expected_path and current_wt and os.path.realpath(current_wt) != os.path.realpath(expected_path):
+            record = dict(row)
+            current_wt = (record.get("worktree_path") or "").strip()
+
+            if not current_wt:
+                # Idempotent: the binding this caller audited is already gone.
+                return {
+                    "lease_id": lease_id,
+                    "retired": False,
+                    "already_retired": True,
+                    "prior_worktree_path": "",
+                    "expected_worktree_path": expected_path,
+                    "reason": reason,
+                    "compare_and_swap": {
+                        "matched": True,
+                        "outcome": "already_retired",
+                    },
+                }
+
+            if _realpath_or_raw(current_wt) != expected_norm:
                raise ControlPlaneError(
-                    f"cannot retire lease {lease_id} worktree_path: expected '{expected_path}' "
-                    f"does not match current '{current_wt}' (fail closed)"
+                    f"cannot retire lease {lease_id} worktree_path: expected "
+                    f"'{expected_path}' does not match current '{current_wt}' "
+                    "(fail closed)"
                )

+            for field, expected_value in (
+                ("status", expected_status),
+                ("session_id", expected_session_id),
+            ):
+                if expected_value is None:
+                    continue
+                current_value = record.get(field)
+                if str(current_value or "").strip() != str(expected_value).strip():
+                    raise ControlPlaneError(
+                        f"cannot retire lease {lease_id} worktree_path: lease "
+                        f"{field} changed since audit (expected "
+                        f"'{expected_value}', found '{current_value}'); fail closed"
+                    )
+
+            if expected_owner_pid is not None:
+                current_pid = record.get("owner_pid")
+                if current_pid is not None and int(current_pid) != int(expected_owner_pid):
+                    raise ControlPlaneError(
+                        f"cannot retire lease {lease_id} worktree_path: lease "
+                        f"owner_pid changed since audit (expected "
+                        f"{expected_owner_pid}, found {current_pid}); fail closed"
+                    )
+
            # Parse and update provenance_json
-            raw_prov = dict(row).get("provenance_json") or "{}"
+            raw_prov = record.get("provenance_json") or "{}"
            try:
                prov = json.loads(raw_prov) if isinstance(raw_prov, str) else dict(raw_prov)
            except Exception:
@@ -1953,21 +2030,31 @@ class ControlPlaneDB:
            if not isinstance(prov, dict):
                prov = {}

-            prior_path = current_wt or prov.get("worktree_path")
+            prior_path = current_wt
            prov.update({
                "worktree_path_retired": True,
                "retired_worktree_path": prior_path,
                "retired_at": now_s,
                "retirement_reason": reason,
+                "retired_from_status": record.get("status"),
+                "retired_from_session_id": record.get("session_id"),
                "worktree_path": "",
            })
            prov_json = json.dumps(prov)

            if "worktree_path" in cols:
-                conn.execute(
-                    "UPDATE leases SET worktree_path = '', provenance_json = ? WHERE lease_id = ?",
-                    (prov_json, lease_id),
+                # CAS: keyed on the exact stored path this caller audited.
+                cur = conn.execute(
+                    "UPDATE leases SET worktree_path = '', provenance_json = ? "
+                    "WHERE lease_id = ? AND worktree_path = ?",
+                    (prov_json, lease_id, record.get("worktree_path")),
                )
+                if cur.rowcount != 1:
+                    raise ControlPlaneError(
+                        f"cannot retire lease {lease_id} worktree_path: "
+                        "compare-and-swap matched no row (concurrent change); "
+                        "fail closed"
+                    )
            else:
                conn.execute(
                    "UPDATE leases SET provenance_json = ? WHERE lease_id = ?",
@@ -1980,7 +2067,7 @@ class ControlPlaneDB:
                VALUES (?, 'worktree_binding_retired', ?, ?)
                """,
                (
-                    row["work_item_id"],
+                    record["work_item_id"],
                    f"lease {lease_id} worktree_path '{prior_path}' retired: {reason}",
                    now_s,
                ),
@@ -1989,9 +2076,18 @@ class ControlPlaneDB:
            return {
                "lease_id": lease_id,
                "retired": True,
+                "already_retired": False,
                "prior_worktree_path": prior_path,
+                "expected_worktree_path": expected_path,
                "retired_at": now_s,
                "reason": reason,
+                "compare_and_swap": {
+                    "matched": True,
+                    "outcome": "retired",
+                    "expected_status": expected_status,
+                    "expected_session_id": expected_session_id,
+                    "expected_owner_pid": expected_owner_pid,
+                },
            }


@@ -3140,25 +3236,116 @@ class ControlPlaneDB:
        *,
        checkpoint_id: str | None = None,
        expected_path: str | None = None,
+        expected_status: str | None = None,
        reason: str = "missing_worktree_path_retired",
    ) -> dict[str, Any]:
-        """Retire a missing worktree_path from session_checkpoints (#970)."""
+        """Retire a missing worktree_path from session_checkpoints (#970).
+
+        Compare-and-swap, mirroring :meth:`retire_lease_worktree_path` (#970
+        review 644 B3). ``expected_path`` names the exact stored path the caller
+        audited; the guarded ``UPDATE`` is keyed on that stored value, so a
+        checkpoint whose path was moved, replaced, or concurrently rewritten
+        after the audit is refused without mutation rather than blindly cleared.
+
+        Exactly one checkpoint row is targeted: by ``checkpoint_id`` when given,
+        otherwise by ``session_id``, which must identify a single row.
+        """
        now_s = _ts()
+        expected_norm = _realpath_or_raw(expected_path)
+        if not expected_norm:
+            raise ControlPlaneError(
+                "cannot retire session checkpoint worktree_path: expected_path "
+                "is required for compare-and-swap retirement (fail closed)"
+            )
+        if not checkpoint_id and not (session_id or "").strip():
+            raise ControlPlaneError(
+                "cannot retire session checkpoint worktree_path: checkpoint_id "
+                "or session_id is required (fail closed)"
+            )
+
        with self._tx() as conn:
            if checkpoint_id:
-                conn.execute(
-                    "UPDATE session_checkpoints SET worktree_path = '', updated_at = ? WHERE checkpoint_id = ?",
-                    (now_s, checkpoint_id),
+                selector_sql = "SELECT * FROM session_checkpoints WHERE checkpoint_id = ?"
+                selector_params: tuple[Any, ...] = (checkpoint_id,)
+                selector_desc = f"checkpoint_id '{checkpoint_id}'"
+            else:
+                selector_sql = "SELECT * FROM session_checkpoints WHERE session_id = ?"
+                selector_params = (session_id,)
+                selector_desc = f"session_id '{session_id}'"
+
+            rows = [dict(r) for r in conn.execute(selector_sql, selector_params).fetchall()]
+            if not rows:
+                raise ControlPlaneError(
+                    f"cannot retire session checkpoint worktree_path: no "
+                    f"checkpoint matches {selector_desc} (fail closed)"
                )
-            elif session_id:
-                conn.execute(
-                    "UPDATE session_checkpoints SET worktree_path = '', updated_at = ? WHERE session_id = ?",
-                    (now_s, session_id),
+            if len(rows) > 1:
+                raise ControlPlaneError(
+                    f"cannot retire session checkpoint worktree_path: "
+                    f"{selector_desc} matches {len(rows)} checkpoints; supply an "
+                    "exact checkpoint_id (fail closed)"
                )
+
+            record = rows[0]
+            target_checkpoint_id = record.get("checkpoint_id")
+            current_wt = (record.get("worktree_path") or "").strip()
+
+            if not current_wt:
+                # Idempotent: the binding this caller audited is already gone.
+                return {
+                    "session_id": session_id,
+                    "checkpoint_id": target_checkpoint_id,
+                    "retired": False,
+                    "already_retired": True,
+                    "prior_worktree_path": "",
+                    "expected_worktree_path": expected_path,
+                    "reason": reason,
+                    "compare_and_swap": {
+                        "matched": True,
+                        "outcome": "already_retired",
+                    },
+                }
+
+            if _realpath_or_raw(current_wt) != expected_norm:
+                raise ControlPlaneError(
+                    f"cannot retire session checkpoint worktree_path for "
+                    f"{selector_desc}: expected '{expected_path}' does not match "
+                    f"current '{current_wt}' (fail closed)"
+                )
+
+            if expected_status is not None:
+                current_status = record.get("status")
+                if str(current_status or "").strip() != str(expected_status).strip():
+                    raise ControlPlaneError(
+                        f"cannot retire session checkpoint worktree_path for "
+                        f"{selector_desc}: status changed since audit (expected "
+                        f"'{expected_status}', found '{current_status}'); fail closed"
+                    )
+
+            cur = conn.execute(
+                "UPDATE session_checkpoints SET worktree_path = '', updated_at = ? "
+                "WHERE checkpoint_id = ? AND worktree_path = ?",
+                (now_s, target_checkpoint_id, record.get("worktree_path")),
+            )
+            if cur.rowcount != 1:
+                raise ControlPlaneError(
+                    f"cannot retire session checkpoint worktree_path for "
+                    f"{selector_desc}: compare-and-swap matched no row "
+                    "(concurrent change); fail closed"
+                )
+
            return {
                "session_id": session_id,
-                "checkpoint_id": checkpoint_id,
+                "checkpoint_id": target_checkpoint_id,
                "retired": True,
+                "already_retired": False,
+                "prior_worktree_path": current_wt,
+                "expected_worktree_path": expected_path,
                "retired_at": now_s,
                "reason": reason,
+                "compare_and_swap": {
+                    "matched": True,
+                    "outcome": "retired",
+                    "expected_status": expected_status,
+                },
            }
@@ -13597,6 +13597,7 @@ def gitea_audit_missing_worktree_bindings(
@mcp.tool()
 def gitea_reconcile_missing_worktree_bindings(
    dry_run: bool = True,
+    # Deprecated: retained so callers that still pass it get an explicit deny.
    operator_authorized: bool = False,
    remote: str = "dadeschools",
    host: str | None = None,
@@ -13606,12 +13607,22 @@ def gitea_reconcile_missing_worktree_bindings(
    """Audit and safely resolve (retire) missing worktree path bindings (#970).

    Identifies confirmed stale deleted worktree bindings, re-validates them
-    immediately before mutation to prevent recreation races, and retires only the
-    exact stale bindings while preserving unrelated worktrees and Git metadata.
+    against a fresh authoritative read immediately before mutation to prevent
+    recreation and ownership races, and retires only the exact stale bindings
+    while preserving unrelated worktrees and Git metadata.
+
+    Apply mode (``dry_run=False``) is authorized **server-side** (#970 review
+    644 B2): it requires the reconciler-only ``gitea.branch.delete``
+    capability, the resolved ``reconcile_missing_worktree_bindings`` task
+    capability, and an active cleanup phase minted through
+    ``gitea_authorize_reconciliation_cleanup_phase``. A caller-supplied
+    ``operator_authorized`` is never authorization evidence (#709 F1 /
+    review 434) and is rejected outright. Dry-run stays available to any
+    ``gitea.read`` profile and never mutates.

    Args:
        dry_run: If True (default), reports planned mutations without modifying state.
-        operator_authorized: Explicit operator authorization required for apply mode.
+        operator_authorized: Rejected. Authorization is a server-side artifact.
        remote: Known instance — 'dadeschools' or 'prgs'.
        host: Override the Gitea host.
        org: Override the owner/organization.
@@ -13620,6 +13631,22 @@ def gitea_reconcile_missing_worktree_bindings(
    Returns:
        dict with before/after audit state, resolutions, and dimension status.
    """
+    import missing_worktree_reconcile
+
+    # Explicitly reject the self-assertable Boolean before anything else, so it
+    # can never combine with a legitimate gate to authorize a mutation.
+    if operator_authorized:
+        return {
+            "success": False,
+            "performed": False,
+            "reason": "operator_authorized_rejected",
+            "reasons": [missing_worktree_reconcile.OPERATOR_AUTHORIZED_REJECTION],
+            "exact_next_action": (
+                "authorize cleanup via gitea_authorize_reconciliation_cleanup_phase "
+                "from a reconciler profile, then re-run with dry_run=False"
+            ),
+        }
+
    read_block = _profile_operation_gate("gitea.read")
    if read_block:
        return {
@@ -13629,14 +13656,39 @@ def gitea_reconcile_missing_worktree_bindings(
            "permission_report": _permission_block_report("gitea.read"),
        }

-    import missing_worktree_reconcile
    db, db_errs = _control_plane_db_or_error()
    root = _canonical_local_git_root()
    h, o, r = _resolve(remote, host, org, repo)

    profile = get_profile()
-    role = profile.get("role") or ""
-    workflow_authorized = role in ("reconciler", "author", "controller")
+    role = (profile.get("role") or profile.get("role_kind") or "").strip().lower()
+
+    cleanup_authorization = None
+    if not dry_run:
+        cleanup_task = missing_worktree_reconcile.CLEANUP_TASK
+        required_permission = task_capability_map.required_permission(cleanup_task)
+        capability_blockers = _profile_operation_gate(required_permission)
+        role_matches = role == task_capability_map.required_role(cleanup_task)
+        cleanup_authorization = missing_worktree_reconcile.assess_cleanup_authorization(
+            role=role,
+            capability_blockers=capability_blockers,
+            operator_authorized=False,
+            task_capability_resolved=(not capability_blockers) and role_matches,
+        )
+        if not cleanup_authorization.get("authorized"):
+            return {
+                "success": False,
+                "performed": False,
+                "reason": "cleanup_authorization_required",
+                "reasons": cleanup_authorization.get("reasons") or [],
+                "cleanup_authorization": cleanup_authorization,
+                "permission_report": _permission_block_report(required_permission),
+                "exact_next_action": (
+                    "resolve the reconcile_missing_worktree_bindings capability "
+                    "from a reconciler profile and authorize cleanup via "
+                    "gitea_authorize_reconciliation_cleanup_phase"
+                ),
+            }

    return missing_worktree_reconcile.reconcile_missing_worktree_bindings(
        db,
@@ -13646,8 +13698,8 @@ def gitea_reconcile_missing_worktree_bindings(
        repo=r,
        host=h,
        dry_run=dry_run,
-        operator_authorized=operator_authorized,
-        workflow_authorized=workflow_authorized,
+        operator_authorized=False,
+        cleanup_authorization=cleanup_authorization,
    )


@@ -386,6 +386,25 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = {
        "permission": "gitea.branch.delete",
        "role": "reconciler",
    },
+    # #970: auditing missing worktree bindings is read-only; retiring one is a
+    # control-plane cleanup mutation and carries the same reconciler-only
+    # authority as any other reconciliation cleanup (review 644 B2).
+    "audit_missing_worktree_bindings": {
+        "permission": "gitea.read",
+        "role": "reconciler",
+    },
+    "gitea_audit_missing_worktree_bindings": {
+        "permission": "gitea.read",
+        "role": "reconciler",
+    },
+    "reconcile_missing_worktree_bindings": {
+        "permission": "gitea.branch.delete",
+        "role": "reconciler",
+    },
+    "gitea_reconcile_missing_worktree_bindings": {
+        "permission": "gitea.branch.delete",
+        "role": "reconciler",
+    },
    "work_issue": {
        "permission": "gitea.pr.create",
        "role": "author",
@@ -653,6 +672,8 @@ ROLE_EXCLUSIVE_TASKS: frozenset[str] = frozenset(
        "delete_branch",
        "cleanup_merged_pr_branch",
        "reconciliation_cleanup",
+        "reconcile_missing_worktree_bindings",
+        "gitea_reconcile_missing_worktree_bindings",
        "work_issue",
        "work-issue",
    }
@@ -153,6 +153,12 @@ EXPECTED_ROLE_EXCLUSIVE_TASKS = frozenset(
        "delete_branch",
        "cleanup_merged_pr_branch",
        "reconciliation_cleanup",
+        # #970 review 644 B2: retiring a missing worktree binding is a
+        # control-plane cleanup mutation, so it carries the same reconciler-only
+        # authority as every other reconciliation cleanup. Permission alone must
+        # not authorize it.
+        "reconcile_missing_worktree_bindings",
+        "gitea_reconcile_missing_worktree_bindings",
        "work_issue",
        "work-issue",
    }