From fe259e6d3812e7d187445a9fb22cde888fff9785 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Thu, 23 Jul 2026 21:19:32 -0400 Subject: [PATCH] fix(worktree-audit): make cleanup audit merged-PR aware for issue worktrees (Closes #858) gitea_audit_worktree_cleanup had no PR linkage. Issue worktrees therefore reported pr_number=null and classified as active_issue_work with removable=false permanently, even once their PR was merged and the head was already contained in master. Observed on live master 9301739910df: 42 issue_work worktrees, 0 removable, 0 with pr_number populated, while gitea_reconcile_merged_cleanups reported the same worktree safe to remove. Two independent gaps caused it: * build_worktree_metadata was never given a pr_number, and only open PRs were fetched, so no owning-PR evidence existed at all. * clean_stale_removable was unreachable for issue_work: it required ttl_expired, derived from a last_used_at that nothing populates, and is_ttl_expired fail-safes to False when the timestamp is unknown. This adds deterministic merged-PR linkage and gates removal on the complete cleanup policy: * build_pr_index / resolve_owning_pr link a worktree branch to exactly one owning PR. Competing PRs on one branch, a still-open owner, a head-branch mismatch, or missing PR state all fail closed while still reporting the resolved pr_number. * assess_merged_pr_worktree_cleanup requires all of: conclusive merged ownership, branch agreement, containment of the head in authoritative master, no open/competing PR, no active lease, no issue lock, no live session, a clean tree, and a non-protected checkout. Unknown state blocks. * Containment reuses merged_cleanup_reconcile.is_head_ancestor_of_ref so the audit and the PR-scoped reconciler agree on what "already landed" means. Lease evidence is now supplied. audit_branches_directory already accepted leased_branches but the MCP tool never passed it, so has_active_lease was false for every worktree in a live run. That was inert only while issue worktrees could never become removable; it is wired to authoritative control-plane leases here, scoped so a lease on issue N protects that issue's work worktree and not a baseline or review tree merely named after it. Issue work no longer becomes removable on TTL age alone, since age is not proof that a branch landed and would otherwise reclaim a worktree holding unmerged commits. conflict_fix keeps its existing TTL behaviour, and review, baseline, merge-simulation, detached, dirty, open-PR, and protected classifications are unchanged. The assessor still performs no deletion and gains no cleanup mutation. This is assessor-side only and does not implement the PR-scoped executor or the expired-lease reclaim policy tracked separately by #855. Co-Authored-By: Claude Opus 4.8 (1M context) --- gitea_mcp_server.py | 82 ++- tests/test_issue_858_audit_merged_pr_aware.py | 551 ++++++++++++++++++ tests/test_worktree_cleanup_audit.py | 26 +- worktree_cleanup_audit.py | 275 ++++++++- 4 files changed, 923 insertions(+), 11 deletions(-) create mode 100644 tests/test_issue_858_audit_merged_pr_aware.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index d059c25..04edea9 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -11634,6 +11634,7 @@ def gitea_audit_worktree_cleanup( org: str | None = None, repo: str | None = None, ttl_hours: float = worktree_cleanup_audit.DEFAULT_TTL_HOURS, + merged_pr_limit: int = 200, ) -> dict: """Read-only: classify every session-owned worktree under ``branches/`` (#401). @@ -11644,17 +11645,26 @@ def gitea_audit_worktree_cleanup( the active issue-lock branch is read from the local lock file and treated as active work. Deletes nothing and mutates no Gitea state. - Fails closed if the live open-PR list cannot be fetched: without it, - removability cannot be proven, so no candidates are returned. + Merged PRs are fetched as well, so an issue worktree can be linked to the + PR that owns its branch (#858). Such a worktree only becomes removable + when that owning PR is unambiguous and merged, the worktree head is + already contained in authoritative master, and nothing else protects it — + no open or competing PR, lease, issue lock, live session, dirty file, or + protected/control checkout. Anything unproven keeps it classified as + active issue work. + + Fails closed if the live open-PR list, the merged-PR list, or the + control-plane lease state cannot be read: without them removability + cannot be proven, so no candidates are returned. Args: remote: Known instance — 'dadeschools' or 'prgs'. host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. - ttl_hours: Age (hours) after which a clean issue/conflict-fix - worktree becomes stale-removable (default from - GITEA_WORKTREE_TTL_HOURS). + ttl_hours: Age (hours) after which a clean conflict-fix worktree + becomes stale-removable (default from GITEA_WORKTREE_TTL_HOURS). + merged_pr_limit: Max closed PRs scanned for merged-PR ownership. Returns: dict with per-worktree classifications, counts, removable @@ -11690,22 +11700,84 @@ def gitea_audit_worktree_cleanup( if (pr.get("head") or {}).get("ref") } + # #858: merged PRs are the ownership evidence that lets a landed issue + # worktree stop being reported as active work. Without them the audit can + # never agree with the PR-scoped reconciler, so treat a fetch failure the + # same way an open-PR fetch failure is treated: fail closed. + try: + closed_prs = api_get_all( + f"{repo_api_url(h, o, r)}/pulls?state=closed", auth, limit=merged_pr_limit + ) + except Exception as exc: + return { + "success": False, + "performed": False, + "open_pr_state_verified": True, + "merged_pr_state_verified": False, + "reasons": [ + "could not fetch merged PRs; worktree ownership unverified " + f"(fail closed): {_redact(str(exc))}" + ], + } + merged_prs = [pr for pr in closed_prs if (pr.get("merged") or pr.get("merged_at"))] + pr_index = worktree_cleanup_audit.build_pr_index(list(open_prs) + merged_prs) + + # #858: the auditor already accepted lease evidence but nothing ever + # supplied it, so every worktree looked unleased. Removability is now + # reachable for issue worktrees, so authoritative control-plane leases + # must be readable or the audit fails closed. + db, lease_errs = _control_plane_db_or_error() + if db is None: + return { + "success": False, + "performed": False, + "open_pr_state_verified": True, + "merged_pr_state_verified": True, + "lease_state_verified": False, + "reasons": [ + "could not read control-plane leases; worktree protection " + "unverified (fail closed)", + *lease_errs, + ], + } + lease_result = lease_lifecycle.list_active_leases( + db, remote=remote, org=o, repo=r, include_non_active=False, limit=500 + ) + leased_issue_numbers: set[int] = set() + live_session_paths: set[str] = set() + for lease in lease_result.get("leases") or []: + if lease.get("work_kind") == "issue" and lease.get("work_number") is not None: + try: + leased_issue_numbers.add(int(lease["work_number"])) + except (TypeError, ValueError): + pass + if lease.get("worktree_path"): + live_session_paths.add(str(lease["worktree_path"])) + active_issue_branches: set[str] = set() lock = merged_cleanup_reconcile.read_issue_lock(ISSUE_LOCK_FILE) if lock and lock.get("branch_name"): active_issue_branches.add(str(lock["branch_name"]).strip()) + master_ref = f"{remote}/master" if remote in REMOTES else "origin/master" report = worktree_cleanup_audit.audit_branches_directory( _canonical_local_git_root(), open_pr_branches=open_pr_branches, active_issue_branches=active_issue_branches, now=datetime.now(timezone.utc), ttl_hours=ttl_hours, + pr_index=pr_index, + leased_issue_numbers=leased_issue_numbers, + live_session_paths=live_session_paths, + master_ref=master_ref, ) return { "success": True, "performed": False, "open_pr_state_verified": True, + "merged_pr_state_verified": True, + "lease_state_verified": True, + "master_ref": master_ref, "task_mode": "work-issue", **report, } diff --git a/tests/test_issue_858_audit_merged_pr_aware.py b/tests/test_issue_858_audit_merged_pr_aware.py new file mode 100644 index 0000000..3232318 --- /dev/null +++ b/tests/test_issue_858_audit_merged_pr_aware.py @@ -0,0 +1,551 @@ +"""Merged-PR awareness for the worktree cleanup audit (#858). + +Before #858 an ``issue_work`` worktree could never leave ``active_issue_work``: +the audit had no PR linkage at all (``pr_number`` was structurally ``None``) +and its only route to ``clean_stale_removable`` was a TTL derived from a +``last_used_at`` that nothing ever populated. A merged, clean, unprotected +worktree was therefore reported as active work forever, disagreeing with the +PR-scoped reconciler. + +These tests use fabricated temporary repositories and synthetic PR records +only. Nothing here removes a worktree or deletes a branch. +""" + +import os +import subprocess +import sys +import tempfile +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import merged_cleanup_reconcile as mcr # noqa: E402 +import worktree_cleanup_audit as wca # noqa: E402 + + +MERGED_BRANCH = "feat/issue-777-timeline" +MERGED_PATH = "/repo/branches/issue-777-timeline" +HEAD_SHA = "a" * 40 + + +def _pr(number, branch, *, merged=True, sha=HEAD_SHA, state=None): + """Synthetic Gitea PR payload.""" + return { + "number": number, + "head": {"ref": branch, "sha": sha}, + "merged_at": "2026-07-24T01:00:00Z" if merged else None, + "state": state or ("closed" if merged else "open"), + } + + +def _porcelain(*entries): + out = [] + for path, branch, sha in entries: + out.append(f"worktree {path}") + out.append(f"HEAD {sha}") + if branch is None: + out.append("detached") + else: + out.append(f"branch refs/heads/{branch}") + out.append("") + return "\n".join(out) + + +class _AuditHarness(unittest.TestCase): + """Runs audit_branches_directory over a fabricated worktree listing.""" + + PORCELAIN = _porcelain( + ("/repo", "master", "f" * 40), + (MERGED_PATH, MERGED_BRANCH, HEAD_SHA), + ) + + def run_audit(self, *, dirty_paths=(), contained=True, **kwargs): + def fake_dirty(path): + if path in dirty_paths: + return {"exists": True, "dirty": True, "dirty_files": [" M x.py"]} + return {"exists": True, "dirty": False, "dirty_files": []} + + with patch.object( + wca, "list_worktrees", + return_value=wca.parse_worktree_porcelain(self.PORCELAIN), + ), patch.object( + wca, "read_worktree_dirty", side_effect=fake_dirty + ), patch.object( + wca, "git_worktree_list", return_value="(mocked)" + ), patch.object( + wca, "is_head_ancestor_of_ref", return_value=contained + ): + report = wca.audit_branches_directory("/repo", **kwargs) + return {wt["path"]: wt for wt in report["worktrees"]}, report + + def merged_audit(self, **kwargs): + kwargs.setdefault("pr_index", wca.build_pr_index([_pr(849, MERGED_BRANCH)])) + kwargs.setdefault("master_ref", "prgs/master") + return self.run_audit(**kwargs) + + +class TestMergedWorktreeBecomesRemovable(_AuditHarness): + def test_clean_merged_issue_worktree_is_linked_and_removable(self): + by_path, report = self.merged_audit() + entry = by_path[MERGED_PATH] + + self.assertEqual(entry["classification"], wca.CLASS_CLEAN_STALE_REMOVABLE) + self.assertTrue(entry["removable"]) + self.assertEqual(entry["merged_pr_linkage"]["status"], wca.LINKAGE_MERGED) + self.assertEqual(entry["merged_pr_cleanup"]["block_reasons"], []) + self.assertIn(MERGED_PATH, [c["path"] for c in report["removable_candidates"]]) + + def test_pr_number_populated_from_authoritative_linkage(self): + by_path, _ = self.merged_audit() + self.assertEqual(by_path[MERGED_PATH]["pr_number"], 849) + + def test_regression_without_pr_evidence_stays_active_issue_work(self): + """The pre-#858 behaviour, still correct when no PR state is supplied.""" + by_path, _ = self.run_audit() + entry = by_path[MERGED_PATH] + self.assertEqual(entry["classification"], wca.CLASS_ACTIVE_ISSUE_WORK) + self.assertFalse(entry["removable"]) + self.assertIsNone(entry["pr_number"]) + + +class TestProtectiveSignalsSurvive(_AuditHarness): + def test_open_pr_worktree_is_not_removable(self): + index = wca.build_pr_index([_pr(900, MERGED_BRANCH, merged=False)]) + by_path, _ = self.run_audit( + pr_index=index, + master_ref="prgs/master", + open_pr_branches={MERGED_BRANCH}, + ) + entry = by_path[MERGED_PATH] + self.assertEqual(entry["classification"], wca.CLASS_ACTIVE_OPEN_PR) + self.assertFalse(entry["removable"]) + # linkage still reports the owning PR, it just is not merge proof + self.assertEqual(entry["merged_pr_linkage"]["status"], wca.LINKAGE_OPEN) + self.assertEqual(entry["pr_number"], 900) + + def test_dirty_tracked_worktree_is_not_removable(self): + by_path, _ = self.merged_audit(dirty_paths=(MERGED_PATH,)) + entry = by_path[MERGED_PATH] + self.assertEqual(entry["classification"], wca.CLASS_DIRTY_LOCAL) + self.assertFalse(entry["removable"]) + self.assertIn( + "worktree has uncommitted changes", + entry["merged_pr_cleanup"]["block_reasons"], + ) + + def test_untracked_only_worktree_is_not_removable(self): + """``git status --porcelain`` reports untracked files as dirty too.""" + def untracked(path): + if path == MERGED_PATH: + return {"exists": True, "dirty": True, "dirty_files": ["?? scratch.txt"]} + return {"exists": True, "dirty": False, "dirty_files": []} + + with patch.object( + wca, "list_worktrees", + return_value=wca.parse_worktree_porcelain(self.PORCELAIN), + ), patch.object( + wca, "read_worktree_dirty", side_effect=untracked + ), patch.object( + wca, "git_worktree_list", return_value="(mocked)" + ), patch.object( + wca, "is_head_ancestor_of_ref", return_value=True + ): + report = wca.audit_branches_directory( + "/repo", + pr_index=wca.build_pr_index([_pr(849, MERGED_BRANCH)]), + master_ref="prgs/master", + ) + entry = {wt["path"]: wt for wt in report["worktrees"]}[MERGED_PATH] + self.assertEqual(entry["classification"], wca.CLASS_DIRTY_LOCAL) + self.assertFalse(entry["removable"]) + + def test_active_lease_by_issue_number_is_protective(self): + by_path, _ = self.merged_audit(leased_issue_numbers={777}) + entry = by_path[MERGED_PATH] + self.assertTrue(entry["has_active_lease"]) + self.assertEqual(entry["classification"], wca.CLASS_ACTIVE_ISSUE_WORK) + self.assertFalse(entry["removable"]) + + def test_active_lease_by_branch_is_protective(self): + by_path, _ = self.merged_audit(leased_branches={MERGED_BRANCH}) + entry = by_path[MERGED_PATH] + self.assertTrue(entry["has_active_lease"]) + self.assertFalse(entry["removable"]) + + def test_active_issue_lock_is_protective(self): + by_path, _ = self.merged_audit(active_issue_branches={MERGED_BRANCH}) + entry = by_path[MERGED_PATH] + self.assertTrue(entry["has_active_issue_lock"]) + self.assertEqual(entry["classification"], wca.CLASS_ACTIVE_ISSUE_WORK) + self.assertFalse(entry["removable"]) + + def test_live_session_worktree_is_protective(self): + by_path, _ = self.merged_audit(live_session_paths={MERGED_PATH}) + entry = by_path[MERGED_PATH] + self.assertTrue(entry["has_live_session"]) + self.assertEqual(entry["classification"], wca.CLASS_ACTIVE_ISSUE_WORK) + self.assertFalse(entry["removable"]) + + def test_head_not_contained_in_master_is_not_removable(self): + by_path, _ = self.merged_audit(contained=False) + entry = by_path[MERGED_PATH] + self.assertEqual(entry["classification"], wca.CLASS_ACTIVE_ISSUE_WORK) + self.assertFalse(entry["removable"]) + self.assertIn( + "worktree head is not contained in authoritative master " + "(unmerged commits remain)", + entry["merged_pr_cleanup"]["block_reasons"], + ) + + def test_unknown_containment_fails_closed(self): + by_path, _ = self.merged_audit(contained=None) + entry = by_path[MERGED_PATH] + self.assertFalse(entry["removable"]) + self.assertIn( + "containment of the worktree head in master is unknown", + entry["merged_pr_cleanup"]["block_reasons"], + ) + + def test_missing_master_ref_fails_closed(self): + by_path, _ = self.run_audit( + pr_index=wca.build_pr_index([_pr(849, MERGED_BRANCH)]) + ) + self.assertFalse(by_path[MERGED_PATH]["removable"]) + + def test_unmerged_owning_pr_is_not_removable(self): + index = wca.build_pr_index([_pr(901, MERGED_BRANCH, merged=False)]) + by_path, _ = self.run_audit(pr_index=index, master_ref="prgs/master") + entry = by_path[MERGED_PATH] + self.assertFalse(entry["removable"]) + self.assertIn( + "owning PR #901 is not merged", + entry["merged_pr_cleanup"]["block_reasons"], + ) + + def test_control_checkout_is_never_removable(self): + by_path, _ = self.merged_audit() + control = by_path["/repo"] + self.assertTrue(control["is_protected"]) + self.assertEqual(control["classification"], wca.CLASS_UNSAFE_UNKNOWN) + self.assertFalse(control["removable"]) + + def test_control_checkout_not_removable_even_if_linked_and_merged(self): + """A merged PR on the control checkout must not unlock removal.""" + porcelain = _porcelain(("/repo", MERGED_BRANCH, HEAD_SHA)) + with patch.object( + wca, "list_worktrees", return_value=wca.parse_worktree_porcelain(porcelain) + ), patch.object( + wca, "read_worktree_dirty", + return_value={"exists": True, "dirty": False, "dirty_files": []}, + ), patch.object( + wca, "git_worktree_list", return_value="(mocked)" + ), patch.object( + wca, "is_head_ancestor_of_ref", return_value=True + ): + report = wca.audit_branches_directory( + "/repo", + pr_index=wca.build_pr_index([_pr(849, MERGED_BRANCH)]), + master_ref="prgs/master", + ) + entry = report["worktrees"][0] + self.assertEqual(entry["classification"], wca.CLASS_UNSAFE_UNKNOWN) + self.assertFalse(entry["removable"]) + + +class TestAmbiguousLinkageFailsClosed(_AuditHarness): + def test_competing_prs_on_one_branch_fail_closed(self): + index = wca.build_pr_index( + [_pr(849, MERGED_BRANCH), _pr(860, MERGED_BRANCH)] + ) + by_path, _ = self.run_audit(pr_index=index, master_ref="prgs/master") + entry = by_path[MERGED_PATH] + self.assertEqual(entry["merged_pr_linkage"]["status"], wca.LINKAGE_AMBIGUOUS) + self.assertIsNone(entry["pr_number"]) + self.assertEqual(entry["classification"], wca.CLASS_ACTIVE_ISSUE_WORK) + self.assertFalse(entry["removable"]) + + def test_merged_plus_open_pr_on_one_branch_fails_closed(self): + index = wca.build_pr_index( + [_pr(849, MERGED_BRANCH), _pr(861, MERGED_BRANCH, merged=False)] + ) + by_path, _ = self.run_audit(pr_index=index, master_ref="prgs/master") + entry = by_path[MERGED_PATH] + self.assertEqual(entry["merged_pr_linkage"]["status"], wca.LINKAGE_AMBIGUOUS) + self.assertFalse(entry["removable"]) + + def test_no_owning_pr_fails_closed(self): + by_path, _ = self.run_audit( + pr_index=wca.build_pr_index([_pr(849, "feat/other-branch")]), + master_ref="prgs/master", + ) + entry = by_path[MERGED_PATH] + self.assertEqual(entry["merged_pr_linkage"]["status"], wca.LINKAGE_NONE) + self.assertFalse(entry["removable"]) + + def test_malformed_pr_records_are_dropped_not_guessed(self): + index = wca.build_pr_index( + [ + {"number": None, "head": {"ref": MERGED_BRANCH}}, + {"number": 5, "head": {}}, + {"number": "not-an-int", "head": {"ref": MERGED_BRANCH}}, + ] + ) + self.assertEqual(index, {}) + self.assertEqual( + wca.resolve_owning_pr(branch=MERGED_BRANCH, pr_index=index)["status"], + wca.LINKAGE_NONE, + ) + + def test_detached_worktree_has_no_branch_linkage(self): + self.assertEqual( + wca.resolve_owning_pr(branch=None, pr_index={})["status"], + wca.LINKAGE_UNKNOWN, + ) + + +class TestUnrelatedClassificationsUnchanged(unittest.TestCase): + """Non-issue_work worktrees keep their pre-#858 classifications.""" + + PORCELAIN = _porcelain( + ("/repo", "master", "f" * 40), + ("/repo/branches/review-pr42", "review-pr42", "2" * 40), + ("/repo/branches/baseline-master-x", "baseline-master-x", "3" * 40), + ("/repo/branches/conflict-fix-pr50", "conflict-fix-pr50", "4" * 40), + ("/repo/branches/review-pr99", None, "5" * 40), + ) + + def _audit(self, **kwargs): + with patch.object( + wca, "list_worktrees", + return_value=wca.parse_worktree_porcelain(self.PORCELAIN), + ), patch.object( + wca, "read_worktree_dirty", + return_value={"exists": True, "dirty": False, "dirty_files": []}, + ), patch.object( + wca, "git_worktree_list", return_value="(mocked)" + ), patch.object( + wca, "is_head_ancestor_of_ref", return_value=True + ): + report = wca.audit_branches_directory("/repo", **kwargs) + return {wt["path"]: wt for wt in report["worktrees"]} + + def test_classifications_identical_with_and_without_pr_evidence(self): + without = self._audit() + with_evidence = self._audit( + pr_index=wca.build_pr_index([_pr(849, MERGED_BRANCH)]), + master_ref="prgs/master", + ) + self.assertEqual( + {p: e["classification"] for p, e in without.items()}, + {p: e["classification"] for p, e in with_evidence.items()}, + ) + + def test_lease_on_issue_does_not_capture_similarly_named_scratch_trees(self): + """A lease on issue 777 protects issue work, not baseline/review trees.""" + porcelain = _porcelain( + ("/repo/branches/baseline-master-issue-777", "baseline-issue-777", "7" * 40), + ("/repo/branches/issue-777-timeline", MERGED_BRANCH, HEAD_SHA), + ) + with patch.object( + wca, "list_worktrees", return_value=wca.parse_worktree_porcelain(porcelain) + ), patch.object( + wca, "read_worktree_dirty", + return_value={"exists": True, "dirty": False, "dirty_files": []}, + ), patch.object( + wca, "git_worktree_list", return_value="(mocked)" + ), patch.object( + wca, "is_head_ancestor_of_ref", return_value=True + ): + report = wca.audit_branches_directory( + "/repo", + pr_index=wca.build_pr_index([_pr(849, MERGED_BRANCH)]), + master_ref="prgs/master", + leased_issue_numbers={777}, + ) + by_path = {wt["path"]: wt for wt in report["worktrees"]} + + baseline = by_path["/repo/branches/baseline-master-issue-777"] + self.assertFalse(baseline["has_active_lease"]) + self.assertEqual(baseline["classification"], wca.CLASS_CLEAN_STALE_REMOVABLE) + + issue_work = by_path["/repo/branches/issue-777-timeline"] + self.assertTrue(issue_work["has_active_lease"]) + self.assertFalse(issue_work["removable"]) + + def test_review_and_baseline_still_removable(self): + by_path = self._audit( + pr_index=wca.build_pr_index([]), master_ref="prgs/master" + ) + self.assertEqual( + by_path["/repo/branches/review-pr42"]["classification"], + wca.CLASS_CLEAN_STALE_REMOVABLE, + ) + self.assertEqual( + by_path["/repo/branches/baseline-master-x"]["classification"], + wca.CLASS_CLEAN_STALE_REMOVABLE, + ) + self.assertEqual( + by_path["/repo/branches/review-pr99"]["classification"], + wca.CLASS_DETACHED_REVIEW_LEFTOVER, + ) + + def test_conflict_fix_ttl_behaviour_unchanged(self): + """conflict_fix still needs only TTL expiry; #858 did not touch it.""" + self.assertEqual( + wca.classify_worktree( + workflow_type=wca.WORKFLOW_CONFLICT_FIX, + is_dirty=False, + ttl_expired=True, + ), + wca.CLASS_CLEAN_STALE_REMOVABLE, + ) + self.assertEqual( + wca.classify_worktree( + workflow_type=wca.WORKFLOW_CONFLICT_FIX, + is_dirty=False, + ttl_expired=False, + ), + wca.CLASS_ACTIVE_ISSUE_WORK, + ) + + def test_issue_work_ttl_alone_no_longer_grants_removal(self): + """Age is not landing proof: TTL alone must not reclaim issue work.""" + self.assertEqual( + wca.classify_worktree( + workflow_type=wca.WORKFLOW_ISSUE_WORK, + is_dirty=False, + ttl_expired=True, + ), + wca.CLASS_ACTIVE_ISSUE_WORK, + ) + + +class TestAssessorPerformsNoDeletion(_AuditHarness): + def test_audit_never_removes_a_worktree(self): + with patch.object(wca, "remove_worktree") as removal: + self.merged_audit() + removal.assert_not_called() + + def test_audit_shells_out_to_no_destructive_git_command(self): + seen = [] + real_run = subprocess.run + + def recording_run(cmd, *args, **kwargs): + seen.append(cmd) + return real_run(["true"], *args, **kwargs) + + with patch.object(subprocess, "run", side_effect=recording_run): + wca.audit_branches_directory("/nonexistent-repo-for-audit") + + joined = [" ".join(c) if isinstance(c, list) else str(c) for c in seen] + for cmd in joined: + self.assertNotIn("worktree remove", cmd) + self.assertNotIn("branch -D", cmd) + self.assertNotIn("push", cmd) + + +class TestAgreementWithPrScopedReconciler(unittest.TestCase): + """The audit and merged_cleanup_reconcile must agree on identical input. + + Uses a real throwaway git repository so containment is computed by git + rather than asserted. Nothing outside the temporary directory is touched. + """ + + def _git(self, *args): + subprocess.run( + ["git", "-C", self.root, *args], + check=True, + capture_output=True, + text=True, + ) + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = os.path.realpath(self._tmp.name) + self._git("init", "-b", "master", ".") + self._git("config", "user.email", "test@example.invalid") + self._git("config", "user.name", "Test") + with open(os.path.join(self.root, "seed.txt"), "w") as fh: + fh.write("seed\n") + self._git("add", "seed.txt") + self._git("commit", "-m", "seed") + + self.branch = "feat/issue-777-timeline" + self._git("checkout", "-b", self.branch) + with open(os.path.join(self.root, "feature.txt"), "w") as fh: + fh.write("feature\n") + self._git("add", "feature.txt") + self._git("commit", "-m", "feature") + self.head_sha = subprocess.run( + ["git", "-C", self.root, "rev-parse", "HEAD"], + capture_output=True, text=True, check=True, + ).stdout.strip() + self._git("checkout", "master") + self._git("merge", "--no-ff", "-m", "merge feature", self.branch) + + self.worktree = os.path.join(self.root, "branches", "issue-777-timeline") + self._git("worktree", "add", self.worktree, self.branch) + + def tearDown(self): + self._tmp.cleanup() + + def _pr_index(self): + return wca.build_pr_index( + [ + { + "number": 849, + "head": {"ref": self.branch, "sha": self.head_sha}, + "merged_at": "2026-07-24T01:00:00Z", + } + ] + ) + + def _audit_entry(self): + report = wca.audit_branches_directory( + self.root, pr_index=self._pr_index(), master_ref="master" + ) + return next(wt for wt in report["worktrees"] if wt["path"] == self.worktree) + + def _reconciler_entry(self): + return mcr.assess_local_worktree_cleanup( + pr_number=849, + head_branch=self.branch, + merged=True, + worktree_state=mcr.resolve_cleanup_worktree_state( + project_root=self.root, + head_branch=self.branch, + issue_number=777, + pr_head_sha=self.head_sha, + target_ref="master", + ), + active_lock=False, + ) + + def test_both_assessors_agree_the_worktree_is_safe(self): + audit_entry = self._audit_entry() + reconciler = self._reconciler_entry() + + self.assertTrue(reconciler["safe_to_remove_worktree"], reconciler) + self.assertTrue(audit_entry["removable"], audit_entry) + self.assertEqual(audit_entry["pr_number"], reconciler["pr_number"]) + self.assertEqual(audit_entry["merged_pr_cleanup"]["block_reasons"], []) + self.assertEqual(reconciler["block_reasons"], []) + + def test_both_assessors_agree_a_dirty_worktree_is_unsafe(self): + with open(os.path.join(self.worktree, "feature.txt"), "a") as fh: + fh.write("local edit\n") + + audit_entry = self._audit_entry() + reconciler = self._reconciler_entry() + + self.assertFalse(audit_entry["removable"]) + self.assertFalse(reconciler["safe_to_remove_worktree"]) + + def test_worktree_still_present_after_audit(self): + self._audit_entry() + self.assertTrue(os.path.isdir(self.worktree)) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_worktree_cleanup_audit.py b/tests/test_worktree_cleanup_audit.py index f55b652..f3fc510 100644 --- a/tests/test_worktree_cleanup_audit.py +++ b/tests/test_worktree_cleanup_audit.py @@ -134,13 +134,35 @@ class TestClassification(unittest.TestCase): self.assertEqual(cls, wca.CLASS_ACTIVE_OPEN_PR) self.assertFalse(wca.is_removable(cls)) - def test_stale_clean_issue_worktree_removable(self): - # Scenario 5: clean issue worktree, TTL expired, no lock -> removable. + def test_stale_clean_issue_worktree_needs_merged_pr_proof(self): + # Scenario 5 (#858): age is not proof that the branch landed, so a + # TTL-expired issue worktree stays active work. Only authoritative + # merged-PR evidence makes it removable, which is what keeps a + # worktree holding unmerged commits from being reclaimed by age. cls = wca.classify_worktree( workflow_type=wca.WORKFLOW_ISSUE_WORK, is_dirty=False, ttl_expired=True, ) + self.assertEqual(cls, wca.CLASS_ACTIVE_ISSUE_WORK) + self.assertFalse(wca.is_removable(cls)) + + cls = wca.classify_worktree( + workflow_type=wca.WORKFLOW_ISSUE_WORK, + is_dirty=False, + ttl_expired=True, + merged_pr_cleanup={"proven": True}, + ) + self.assertEqual(cls, wca.CLASS_CLEAN_STALE_REMOVABLE) + self.assertTrue(wca.is_removable(cls)) + + def test_stale_clean_conflict_fix_worktree_removable(self): + # conflict_fix keeps the original TTL rule; #858 changed issue work only. + cls = wca.classify_worktree( + workflow_type=wca.WORKFLOW_CONFLICT_FIX, + is_dirty=False, + ttl_expired=True, + ) self.assertEqual(cls, wca.CLASS_CLEAN_STALE_REMOVABLE) self.assertTrue(wca.is_removable(cls)) diff --git a/worktree_cleanup_audit.py b/worktree_cleanup_audit.py index 261164a..9eb4b35 100644 --- a/worktree_cleanup_audit.py +++ b/worktree_cleanup_audit.py @@ -34,7 +34,11 @@ import subprocess from datetime import datetime, timezone from typing import Any -from merged_cleanup_reconcile import branch_worktree_folder, read_local_worktree_state +from merged_cleanup_reconcile import ( + branch_worktree_folder, + is_head_ancestor_of_ref, + read_local_worktree_state, +) from reviewer_worktree import parse_dirty_tracked_files, REVIEW_WORKTREE_RE PROTECTED_BRANCHES = frozenset({"master", "main", "dev"}) @@ -67,6 +71,14 @@ REMOVABLE_CLASSES = frozenset( {CLASS_CLEAN_STALE_REMOVABLE, CLASS_DETACHED_REVIEW_LEFTOVER} ) +# Merged-PR linkage outcomes for issue worktrees (#858). Only ``LINKAGE_MERGED`` +# is ownership proof; every other outcome leaves the worktree protected. +LINKAGE_MERGED = "merged_pr" +LINKAGE_OPEN = "open_pr" +LINKAGE_NONE = "no_owning_pr" +LINKAGE_AMBIGUOUS = "ambiguous" +LINKAGE_UNKNOWN = "unknown" + _ISSUE_REF_RE = re.compile(r"issue-(\d+)", re.IGNORECASE) _ISSUE_BRANCH_PREFIXES = ("feat/", "fix/", "docs/", "chore/") @@ -169,6 +181,186 @@ def is_ttl_expired( return (now_dt - last).total_seconds() > ttl_hours * 3600.0 +def build_pr_index(prs: list[dict[str, Any]] | None) -> dict[str, list[dict[str, Any]]]: + """Index PR records by head branch for deterministic worktree linkage (#858). + + Accepts Gitea PR payloads (``head`` as a dict) and pre-flattened records + (``head_branch``/``head_sha``). Records without a usable head branch or + number are dropped rather than guessed at, so a branch is only ever linked + to a PR the caller actually proved. + """ + index: dict[str, list[dict[str, Any]]] = {} + for pr in prs or []: + head = pr.get("head") + if isinstance(head, dict): + head_branch = head.get("ref") + head_sha = head.get("sha") + else: + head_branch = pr.get("head_branch") or (head if isinstance(head, str) else None) + head_sha = pr.get("head_sha") + number = pr.get("number") + if not head_branch or number is None: + continue + try: + pr_number = int(number) + except (TypeError, ValueError): + continue + index.setdefault(str(head_branch).strip(), []).append( + { + "pr_number": pr_number, + "head_branch": str(head_branch).strip(), + "head_sha": head_sha, + "merged": bool(pr.get("merged") or pr.get("merged_at")), + "state": pr.get("state"), + } + ) + return index + + +def resolve_owning_pr( + *, + branch: str | None, + pr_index: dict[str, list[dict[str, Any]]] | None, +) -> dict[str, Any]: + """Resolve the single PR that owns ``branch``, failing closed when unclear. + + Ownership is only ``LINKAGE_MERGED`` when exactly one PR claims the branch + and that PR is merged. Several distinct PRs on one branch is a competing + claim (``LINKAGE_AMBIGUOUS``), and a still-open owner is reported as + ``LINKAGE_OPEN`` — both keep the worktree protected while still exposing + the PR number the audit resolved. + """ + if pr_index is None: + return { + "status": LINKAGE_UNKNOWN, + "pr_number": None, + "candidate_pr_numbers": [], + "reasons": ["live PR state was not supplied; ownership unproven"], + } + branch_name = (branch or "").strip() + if not branch_name: + return { + "status": LINKAGE_UNKNOWN, + "pr_number": None, + "candidate_pr_numbers": [], + "reasons": ["worktree has no attached branch; ownership unproven"], + } + + candidates = list(pr_index.get(branch_name) or []) + numbers = sorted({c["pr_number"] for c in candidates}) + if not candidates: + return { + "status": LINKAGE_NONE, + "pr_number": None, + "candidate_pr_numbers": [], + "reasons": [f"no PR claims branch '{branch_name}'"], + } + if len(numbers) > 1: + return { + "status": LINKAGE_AMBIGUOUS, + "pr_number": None, + "candidate_pr_numbers": numbers, + "reasons": [ + f"branch '{branch_name}' is claimed by competing PRs {numbers}; " + "ownership is ambiguous" + ], + } + + owner = candidates[0] + pr_number = owner["pr_number"] + if owner.get("head_branch") != branch_name: + return { + "status": LINKAGE_UNKNOWN, + "pr_number": pr_number, + "candidate_pr_numbers": numbers, + "reasons": [ + f"PR #{pr_number} head branch '{owner.get('head_branch')}' does not " + f"match worktree branch '{branch_name}'" + ], + } + if not owner.get("merged"): + return { + "status": LINKAGE_OPEN, + "pr_number": pr_number, + "candidate_pr_numbers": numbers, + "pr_head_sha": owner.get("head_sha"), + "reasons": [f"owning PR #{pr_number} is not merged"], + } + return { + "status": LINKAGE_MERGED, + "pr_number": pr_number, + "candidate_pr_numbers": numbers, + "pr_head_sha": owner.get("head_sha"), + "reasons": [], + } + + +def assess_merged_pr_worktree_cleanup( + *, + linkage: dict[str, Any] | None, + head_sha: str | None, + head_in_master: bool | None, + is_dirty: bool, + has_open_pr: bool, + has_active_lease: bool, + has_active_issue_lock: bool, + is_protected: bool, + has_live_session: bool = False, +) -> dict[str, Any]: + """Decide whether a merged issue worktree satisfies the full cleanup policy. + + Every condition must be independently proven: conclusive merged-PR + ownership, agreement between the worktree branch and the PR head branch, + containment of the worktree head in authoritative master (which is what + proves no unmerged commits remain), absence of any open/competing PR, + lease, issue lock, or live session, a clean tree, and a worktree that is + not the protected control checkout. Anything unknown blocks. + """ + link = linkage or { + "status": LINKAGE_UNKNOWN, + "pr_number": None, + "reasons": ["no linkage assessment supplied"], + } + status = link.get("status") + reasons: list[str] = [] + + if status != LINKAGE_MERGED: + reasons.extend( + link.get("reasons") or ["owning PR could not be conclusively identified"] + ) + if is_protected: + reasons.append("worktree is protected or the stable control checkout") + if is_dirty: + reasons.append("worktree has uncommitted changes") + if has_open_pr: + reasons.append("worktree branch has an open PR") + if has_active_lease: + reasons.append("worktree has an active lease") + if has_active_issue_lock: + reasons.append("an active issue lock references this branch") + if has_live_session: + reasons.append("a live process or session is using this worktree") + if not head_sha: + reasons.append("worktree head sha is unknown") + if head_in_master is None: + reasons.append("containment of the worktree head in master is unknown") + elif not head_in_master: + reasons.append( + "worktree head is not contained in authoritative master " + "(unmerged commits remain)" + ) + + proven = not reasons + return { + "linkage_status": status, + "pr_number": link.get("pr_number"), + "pr_head_sha": link.get("pr_head_sha"), + "head_in_master": head_in_master, + "proven": proven, + "block_reasons": reasons, + } + + def classify_worktree( *, workflow_type: str, @@ -181,6 +373,8 @@ def classify_worktree( ttl_expired: bool = False, is_protected: bool = False, metadata_known: bool = True, + merged_pr_cleanup: dict[str, Any] | None = None, + has_live_session: bool = False, ) -> str: """Classify a worktree, safety-first: any preservation signal wins. @@ -199,6 +393,8 @@ def classify_worktree( return CLASS_ACTIVE_ISSUE_WORK # never auto-deleted (criterion 8) if has_active_issue_lock: return CLASS_ACTIVE_ISSUE_WORK + if has_live_session: + return CLASS_ACTIVE_ISSUE_WORK # a live session still owns this tree if not metadata_known or workflow_type == WORKFLOW_UNKNOWN: return CLASS_UNSAFE_UNKNOWN # never auto-deleted without proof @@ -207,7 +403,15 @@ def classify_worktree( if is_detached or branch_gone: return CLASS_DETACHED_REVIEW_LEFTOVER return CLASS_CLEAN_STALE_REMOVABLE - # issue_work / conflict_fix: only removable once the TTL has expired. + if workflow_type == WORKFLOW_ISSUE_WORK: + # #858: an issue worktree becomes removable only on authoritative + # merged-PR evidence satisfying the whole cleanup policy. Age alone + # never proves the branch landed, so TTL cannot qualify one by itself + # — otherwise a worktree holding unmerged commits would be reclaimed. + if (merged_pr_cleanup or {}).get("proven"): + return CLASS_CLEAN_STALE_REMOVABLE + return CLASS_ACTIVE_ISSUE_WORK + # conflict_fix: only removable once the TTL has expired. if ttl_expired: return CLASS_CLEAN_STALE_REMOVABLE return CLASS_ACTIVE_ISSUE_WORK @@ -400,6 +604,20 @@ def remove_worktree(project_root: str, path: str) -> dict[str, Any]: } +def head_contained_in_ref( + project_root: str, head_sha: str | None, ref: str | None +) -> bool | None: + """Return True when ``head_sha`` is already contained in ``ref``. + + Shares :mod:`merged_cleanup_reconcile`'s ancestry check so the audit and + the PR-scoped reconciler agree on what "already landed" means (#858). + Returns None when containment cannot be determined, which fails closed. + """ + if not head_sha or not ref: + return None + return is_head_ancestor_of_ref(project_root, head_sha, ref) + + def _is_under_branches(project_root: str, path: str) -> bool: branches_root = os.path.join(os.path.abspath(project_root), "branches") return os.path.abspath(path or "").startswith(branches_root + os.sep) @@ -413,16 +631,30 @@ def audit_branches_directory( active_issue_branches: set[str] | None = None, now: datetime | str | None = None, ttl_hours: float = DEFAULT_TTL_HOURS, + pr_index: dict[str, list[dict[str, Any]]] | None = None, + leased_issue_numbers: set[int] | None = None, + live_session_paths: set[str] | None = None, + master_ref: str | None = None, ) -> dict[str, Any]: """Classify every session-owned worktree under ``branches/``. Read-only: shells out to git for discovery and dirty state, then applies the pure classifier. Returns per-worktree classifications, counts, the list of removable candidates, and the ``git worktree list`` proof. + + ``pr_index`` (see :func:`build_pr_index`) supplies the authoritative PR + ownership used to link issue worktrees to their merged PR (#858). + ``master_ref`` is the ref a worktree head must be contained in before it + can be considered landed. Both are optional and their absence only ever + fails closed: without them no issue worktree becomes removable. """ open_pr_branches = open_pr_branches or set() leased_branches = leased_branches or set() active_issue_branches = active_issue_branches or set() + leased_issue_numbers = leased_issue_numbers or set() + live_session_paths = { + os.path.abspath(p) for p in (live_session_paths or set()) if p + } worktrees: list[dict[str, Any]] = [] for entry in list_worktrees(project_root): @@ -433,12 +665,42 @@ def audit_branches_directory( ) dirty_state = read_worktree_dirty(path) is_dirty = bool(dirty_state.get("dirty")) + head_sha = entry.get("head") + linkage = resolve_owning_pr(branch=branch, pr_index=pr_index) metadata = build_worktree_metadata( - path=path, branch=branch, head_sha=entry.get("head") + path=path, + branch=branch, + head_sha=head_sha, + pr_number=linkage.get("pr_number"), ) has_open_pr = bool(branch) and branch in open_pr_branches - has_active_lease = bool(branch) and branch in leased_branches + # A lease on issue N protects that issue's own work worktree. It must + # not incidentally protect a baseline/review scratch tree that merely + # carries the same issue marker in its name, which would change the + # classification of worktrees this policy does not own. + has_active_lease = (bool(branch) and branch in leased_branches) or ( + metadata["workflow_type"] == WORKFLOW_ISSUE_WORK + and metadata.get("issue_number") is not None + and metadata["issue_number"] in leased_issue_numbers + ) has_active_lock = bool(branch) and branch in active_issue_branches + has_live_session = bool(path) and os.path.abspath(path) in live_session_paths + head_in_master = ( + head_contained_in_ref(project_root, head_sha, master_ref) + if master_ref + else None + ) + merged_pr_cleanup = assess_merged_pr_worktree_cleanup( + linkage=linkage, + head_sha=head_sha, + head_in_master=head_in_master, + is_dirty=is_dirty, + has_open_pr=has_open_pr, + has_active_lease=has_active_lease, + has_active_issue_lock=has_active_lock, + is_protected=is_protected, + has_live_session=has_live_session, + ) ttl_expired = is_ttl_expired( last_used_at=metadata.get("last_used_at"), now=now, ttl_hours=ttl_hours ) @@ -452,6 +714,8 @@ def audit_branches_directory( branch_gone=branch is None and not entry.get("detached"), ttl_expired=ttl_expired, is_protected=is_protected, + merged_pr_cleanup=merged_pr_cleanup, + has_live_session=has_live_session, ) metadata["cleanup_eligibility"] = classification worktrees.append( @@ -463,7 +727,10 @@ def audit_branches_directory( "has_open_pr": has_open_pr, "has_active_lease": has_active_lease, "has_active_issue_lock": has_active_lock, + "has_live_session": has_live_session, "is_protected": is_protected, + "merged_pr_linkage": linkage, + "merged_pr_cleanup": merged_pr_cleanup, "classification": classification, "removable": is_removable(classification), }