fix(cleanup): post-delete readback and active ownership gates (#687)
Require authoritative not-found readback after merged-PR branch DELETE, and block cleanup when active author/reviewer/merger/controller/reconciler session, lease, or worktree-binding ownership still uses the target branch.
This commit is contained in:
+345
-12
@@ -6128,10 +6128,53 @@ def gitea_cleanup_merged_pr_branch(
|
||||
"reasons": assessment["block_reasons"],
|
||||
}
|
||||
|
||||
# #687: active ownership protection (sessions/leases/worktree bindings).
|
||||
# Do not rely only on the caller worktree living under branches/.
|
||||
ownership_records = _collect_branch_ownership_records(
|
||||
remote=remote,
|
||||
org=o,
|
||||
repo=r,
|
||||
branch=head_branch,
|
||||
pr_number=pr_number,
|
||||
project_root=PROJECT_ROOT,
|
||||
)
|
||||
ownership = branch_cleanup_guard.assess_active_branch_ownership(
|
||||
remote=remote,
|
||||
org=o,
|
||||
repo=r,
|
||||
branch=head_branch,
|
||||
records=ownership_records,
|
||||
)
|
||||
if ownership.get("block"):
|
||||
return {
|
||||
"success": False,
|
||||
"performed": False,
|
||||
"pr_number": pr_number,
|
||||
"branch": head_branch,
|
||||
"assessment": assessment,
|
||||
"ownership": {
|
||||
"block": True,
|
||||
"blocking_categories": ownership.get("blocking_categories") or [],
|
||||
"reasons": ownership.get("reasons") or [],
|
||||
"blocker_kind": ownership.get("blocker_kind"),
|
||||
},
|
||||
"reasons": ownership.get("reasons") or [
|
||||
"active ownership protects the target branch"
|
||||
],
|
||||
"blocker_kind": "active_branch_ownership",
|
||||
}
|
||||
|
||||
import urllib.parse
|
||||
|
||||
encoded_branch = urllib.parse.quote(head_branch, safe="")
|
||||
url = f"{base}/branches/{encoded_branch}"
|
||||
request_metadata = {
|
||||
"branch": head_branch,
|
||||
"required_permission": "gitea.branch.delete",
|
||||
"cleanup_path": "gitea_cleanup_merged_pr_branch",
|
||||
"ownership_checked": True,
|
||||
"ownership_blocking_categories": [],
|
||||
}
|
||||
with _audited(
|
||||
"cleanup_merged_pr_branch",
|
||||
host=h,
|
||||
@@ -6140,36 +6183,326 @@ def gitea_cleanup_merged_pr_branch(
|
||||
repo=r,
|
||||
pr_number=pr_number,
|
||||
target_branch=head_branch,
|
||||
request_metadata={
|
||||
"branch": head_branch,
|
||||
"required_permission": "gitea.branch.delete",
|
||||
"cleanup_path": "gitea_cleanup_merged_pr_branch",
|
||||
},
|
||||
request_metadata=request_metadata,
|
||||
):
|
||||
api_request("DELETE", url, auth)
|
||||
|
||||
# #687: authoritative post-delete readback. DELETE success alone is not
|
||||
# enough — only an authoritative not-found proves deletion.
|
||||
readback = _probe_remote_branch(h, o, r, auth, head_branch)
|
||||
readback_assessment = branch_cleanup_guard.assess_post_delete_readback(readback)
|
||||
request_metadata["post_delete_readback"] = {
|
||||
"status": (readback_assessment.get("readback") or {}).get("status"),
|
||||
"verified_absent": bool(
|
||||
(readback_assessment.get("readback") or {}).get("verified_absent")
|
||||
),
|
||||
"error_class": (readback_assessment.get("readback") or {}).get(
|
||||
"error_class"
|
||||
),
|
||||
}
|
||||
# Emit a second audit row capturing readback evidence (no secrets).
|
||||
if gitea_audit.audit_enabled():
|
||||
_audit(
|
||||
"cleanup_merged_pr_branch_readback",
|
||||
host=h,
|
||||
remote=remote,
|
||||
org=o,
|
||||
repo=r,
|
||||
result=(
|
||||
gitea_audit.SUCCEEDED
|
||||
if readback_assessment.get("ok")
|
||||
else gitea_audit.FAILED
|
||||
),
|
||||
reason="; ".join(readback_assessment.get("reasons") or []) or None,
|
||||
request_metadata=request_metadata,
|
||||
pr_number=pr_number,
|
||||
target_branch=head_branch,
|
||||
)
|
||||
|
||||
if not readback_assessment.get("ok"):
|
||||
return {
|
||||
"success": False,
|
||||
"performed": True,
|
||||
"delete_acknowledged": True,
|
||||
"pr_number": pr_number,
|
||||
"branch": head_branch,
|
||||
"assessment": assessment,
|
||||
"ownership": {
|
||||
"block": False,
|
||||
"blocking_categories": [],
|
||||
"checked": True,
|
||||
},
|
||||
"readback": readback_assessment.get("readback"),
|
||||
"reasons": readback_assessment.get("reasons") or [
|
||||
"post-delete branch readback could not verify deletion"
|
||||
],
|
||||
"blocker_kind": readback_assessment.get("blocker_kind")
|
||||
or "post_delete_readback_failed",
|
||||
"message": (
|
||||
f"DELETE accepted for '{head_branch}' but post-delete readback "
|
||||
"did not verify absence"
|
||||
),
|
||||
}
|
||||
|
||||
return {
|
||||
"success": True,
|
||||
"performed": True,
|
||||
"pr_number": pr_number,
|
||||
"branch": head_branch,
|
||||
"message": f"Merged PR #{pr_number} source branch '{head_branch}' deleted.",
|
||||
"message": (
|
||||
f"Merged PR #{pr_number} source branch '{head_branch}' deleted "
|
||||
"and verified absent via post-delete readback."
|
||||
),
|
||||
"assessment": assessment,
|
||||
"ownership": {
|
||||
"block": False,
|
||||
"blocking_categories": [],
|
||||
"checked": True,
|
||||
},
|
||||
"readback": readback_assessment.get("readback"),
|
||||
}
|
||||
|
||||
|
||||
def _remote_branch_exists(h: str, o: str, r: str, auth: str, branch: str) -> bool:
|
||||
def _probe_remote_branch(
|
||||
h: str, o: str, r: str, auth: str, branch: str
|
||||
) -> dict:
|
||||
"""GET a remote branch and return a secret-free structured readback."""
|
||||
import urllib.parse
|
||||
|
||||
encoded = urllib.parse.quote(branch, safe="")
|
||||
url = f"{repo_api_url(h, o, r)}/branches/{encoded}"
|
||||
try:
|
||||
api_request("GET", url, auth)
|
||||
return True
|
||||
return branch_cleanup_guard.classify_branch_readback_http_status(200)
|
||||
except Exception as exc:
|
||||
message = str(exc).lower()
|
||||
if "404" in message or "not found" in message:
|
||||
return False
|
||||
raise
|
||||
return branch_cleanup_guard.classify_branch_readback_exception(exc)
|
||||
|
||||
|
||||
def _remote_branch_exists(h: str, o: str, r: str, auth: str, branch: str) -> bool:
|
||||
"""True when the remote branch exists; False on authoritative 404.
|
||||
|
||||
Authentication, authorization, transport, and unexpected failures are
|
||||
re-raised (or returned as structured failures by callers that use
|
||||
``_probe_remote_branch``) rather than treated as absence.
|
||||
"""
|
||||
probe = _probe_remote_branch(h, o, r, auth, branch)
|
||||
status = probe.get("status")
|
||||
if status == branch_cleanup_guard.READBACK_NOT_FOUND:
|
||||
return False
|
||||
if status == branch_cleanup_guard.READBACK_EXISTS:
|
||||
return True
|
||||
# Preserve structured failure modes for callers that need them.
|
||||
error_class = probe.get("error_class") or "unexpected"
|
||||
raise RuntimeError(
|
||||
f"remote branch probe failed ({error_class}/{status})"
|
||||
)
|
||||
|
||||
|
||||
def _collect_branch_ownership_records(
|
||||
*,
|
||||
remote: str,
|
||||
org: str,
|
||||
repo: str,
|
||||
branch: str,
|
||||
pr_number: int | None,
|
||||
project_root: str,
|
||||
) -> list[dict]:
|
||||
"""Gather canonical ownership records for *branch* (secret-free).
|
||||
|
||||
Sources:
|
||||
- author issue locks (task-session / lease files)
|
||||
- control-plane leases (author/reviewer/merger/controller/reconciler)
|
||||
- local worktree bindings checked out to the branch
|
||||
"""
|
||||
records: list[dict] = []
|
||||
target_branch = (branch or "").strip()
|
||||
if not target_branch:
|
||||
return records
|
||||
|
||||
# --- Author issue locks (session + lease) ---
|
||||
try:
|
||||
for path in issue_lock_store.iter_lock_files():
|
||||
lock = issue_lock_store.read_lock_file(path)
|
||||
if not isinstance(lock, dict):
|
||||
continue
|
||||
if (
|
||||
str(lock.get("remote") or "") != str(remote)
|
||||
or str(lock.get("org") or "") != str(org)
|
||||
or str(lock.get("repo") or "") != str(repo)
|
||||
):
|
||||
continue
|
||||
if str(lock.get("branch_name") or "").strip() != target_branch:
|
||||
continue
|
||||
freshness = issue_lock_store.assess_lock_freshness(lock)
|
||||
reclaim = issue_lock_store.assess_expired_lock_reclaim(lock)
|
||||
status = str(freshness.get("status") or "unknown")
|
||||
if freshness.get("live"):
|
||||
status = "active"
|
||||
records.append(
|
||||
{
|
||||
"category": branch_cleanup_guard.OWNERSHIP_CATEGORY_AUTHOR_LEASE,
|
||||
"status": status,
|
||||
"remote": remote,
|
||||
"org": org,
|
||||
"repo": repo,
|
||||
"branch": target_branch,
|
||||
"reclaim_allowed": bool(reclaim.get("reclaim_allowed")),
|
||||
"role": "author",
|
||||
}
|
||||
)
|
||||
# Session pointer binding (same record, distinct category when live)
|
||||
if freshness.get("live"):
|
||||
records.append(
|
||||
{
|
||||
"category": (
|
||||
branch_cleanup_guard.OWNERSHIP_CATEGORY_AUTHOR_SESSION
|
||||
),
|
||||
"status": "active",
|
||||
"remote": remote,
|
||||
"org": org,
|
||||
"repo": repo,
|
||||
"branch": target_branch,
|
||||
"reclaim_allowed": False,
|
||||
"role": "author",
|
||||
}
|
||||
)
|
||||
except Exception:
|
||||
# Fail closed: if lock inventory cannot be read, invent a sticky block.
|
||||
records.append(
|
||||
{
|
||||
"category": branch_cleanup_guard.OWNERSHIP_CATEGORY_AUTHOR_LEASE,
|
||||
"status": "unknown",
|
||||
"remote": remote,
|
||||
"org": org,
|
||||
"repo": repo,
|
||||
"branch": target_branch,
|
||||
"reclaim_allowed": False,
|
||||
"role": "author",
|
||||
}
|
||||
)
|
||||
|
||||
# --- Control-plane leases (role-tagged) ---
|
||||
try:
|
||||
db = control_plane_db.get_db() if hasattr(control_plane_db, "get_db") else None
|
||||
if db is None and hasattr(control_plane_db, "ControlPlaneDB"):
|
||||
# Best-effort default path used by runtime tools.
|
||||
try:
|
||||
db = control_plane_db.ControlPlaneDB()
|
||||
except TypeError:
|
||||
db = None
|
||||
if db is not None:
|
||||
listed = lease_lifecycle.list_active_leases(
|
||||
db,
|
||||
remote=remote,
|
||||
org=org,
|
||||
repo=repo,
|
||||
include_non_active=True,
|
||||
limit=200,
|
||||
)
|
||||
for lease in listed.get("leases") or []:
|
||||
work_kind = str(lease.get("work_kind") or "")
|
||||
work_number = lease.get("work_number")
|
||||
lease_branch = str(
|
||||
lease.get("branch")
|
||||
or lease.get("branch_name")
|
||||
or ""
|
||||
).strip()
|
||||
matches_branch = lease_branch == target_branch
|
||||
matches_pr = (
|
||||
work_kind == "pr"
|
||||
and pr_number is not None
|
||||
and int(work_number or 0) == int(pr_number)
|
||||
)
|
||||
if not (matches_branch or matches_pr):
|
||||
continue
|
||||
if matches_pr and not lease_branch:
|
||||
# PR-scoped lease without explicit branch still protects
|
||||
# the merged PR source branch being cleaned.
|
||||
lease_branch = target_branch
|
||||
if lease_branch != target_branch:
|
||||
continue
|
||||
fr = lease.get("freshness") or lease_lifecycle.classify_lease_freshness(
|
||||
lease
|
||||
)
|
||||
freshness_status = str(
|
||||
(fr.get("freshness") if isinstance(fr, dict) else None)
|
||||
or lease.get("status")
|
||||
or "unknown"
|
||||
)
|
||||
role = str(lease.get("role") or "unknown")
|
||||
category = branch_cleanup_guard.ownership_category_for_role(role)
|
||||
reclaim_allowed = freshness_status in {
|
||||
"released",
|
||||
"abandoned",
|
||||
"expired",
|
||||
} and freshness_status != "active"
|
||||
if freshness_status == "active":
|
||||
status = "active"
|
||||
reclaim_allowed = False
|
||||
elif freshness_status in {"released", "abandoned"}:
|
||||
status = freshness_status
|
||||
reclaim_allowed = True
|
||||
elif freshness_status == "expired" or (
|
||||
isinstance(fr, dict) and fr.get("expired_by_time")
|
||||
):
|
||||
status = "expired"
|
||||
# Sticky if session still alive with worktree — unknown here
|
||||
# defaults to reclaimable when status is expired by time.
|
||||
reclaim_allowed = True
|
||||
else:
|
||||
status = freshness_status
|
||||
reclaim_allowed = False
|
||||
records.append(
|
||||
{
|
||||
"category": category,
|
||||
"status": status,
|
||||
"remote": remote,
|
||||
"org": org,
|
||||
"repo": repo,
|
||||
"branch": target_branch,
|
||||
"reclaim_allowed": reclaim_allowed,
|
||||
"role": role,
|
||||
}
|
||||
)
|
||||
except Exception:
|
||||
# Soft: control-plane may be unavailable in unit tests / offline.
|
||||
pass
|
||||
|
||||
# --- Worktree bindings checked out to the target branch ---
|
||||
try:
|
||||
for entry in worktree_cleanup_audit.list_worktrees(project_root):
|
||||
wt_branch = str(entry.get("branch") or "").strip()
|
||||
if wt_branch != target_branch:
|
||||
continue
|
||||
records.append(
|
||||
{
|
||||
"category": (
|
||||
branch_cleanup_guard.OWNERSHIP_CATEGORY_WORKTREE_BINDING
|
||||
),
|
||||
"status": "active",
|
||||
"remote": remote,
|
||||
"org": org,
|
||||
"repo": repo,
|
||||
"branch": target_branch,
|
||||
"reclaim_allowed": False,
|
||||
"role": "worktree",
|
||||
}
|
||||
)
|
||||
except Exception:
|
||||
records.append(
|
||||
{
|
||||
"category": branch_cleanup_guard.OWNERSHIP_CATEGORY_WORKTREE_BINDING,
|
||||
"status": "unknown",
|
||||
"remote": remote,
|
||||
"org": org,
|
||||
"repo": repo,
|
||||
"branch": target_branch,
|
||||
"reclaim_allowed": False,
|
||||
"role": "worktree",
|
||||
}
|
||||
)
|
||||
|
||||
return records
|
||||
|
||||
|
||||
@mcp.tool()
|
||||
|
||||
Reference in New Issue
Block a user