Merge pull request #602 from Extra-Chill/fix/stale-clean-remote-worktree-cleanup

Fix stale clean remote-backed worktree cleanup

Author: chubes4

inc/Workspace/WorkspaceActiveNoSignalCleanup.php

@@ -989,7 +989,7 @@ private function validate_current_cleanup_worktree( string $repo, string $path,
 		}
 
 		$current_branch = (string) $this->resolve_worktree_branch_from_head_file($real_path);
-		if ( null !== $expected_branch && $expected_branch !== $current_branch ) {
+		if ( null !== $expected_branch && ! $this->cleanup_branch_identity_matches($expected_branch, $current_branch) ) {
 			return new \WP_Error('branch_identity_mismatch', 'worktree branch identity changed before apply');
 		}
 		if ( null === $expected_branch && '' === $current_branch ) {
@@ -1027,12 +1027,31 @@ private function validate_current_cleanup_worktree( string $repo, string $path,
 		return array(
 			'real_path'    => $real_path,
 			'primary_path' => $primary_path,
-			'branch'       => null !== $expected_branch ? $expected_branch : $current_branch,
+			'branch'       => '' !== $current_branch ? $current_branch : (string) $expected_branch,
 			'dirty'        => (int) $dirty,
 			'unpushed'     => (int) $unpushed,
 		);
 	}
 
+	/**
+	 * Compare a report branch identity against the live worktree branch.
+	 *
+	 * Active/no-signal rows may originate from stale metadata or handle slugs where
+	 * `fix/foo` is represented as `fix-foo`. Treat that as the same branch while
+	 * still rejecting detached/missing heads and unrelated branch changes.
+	 *
+	 * @param  string $expected_branch Branch carried by the report row.
+	 * @param  string $current_branch  Branch resolved from the live worktree HEAD.
+	 * @return bool Whether the identities are compatible.
+	 */
+	private function cleanup_branch_identity_matches( string $expected_branch, string $current_branch ): bool {
+		if ( '' === $expected_branch || '' === $current_branch ) {
+			return false;
+		}
+
+		return $expected_branch === $current_branch || $expected_branch === $this->slugify_branch($current_branch);
+	}
+
 	/**
 	 * Build a skip row for finalized active/no-signal apply.
 	 *

inc/Workspace/WorkspaceCoreUtilities.php

@@ -519,7 +519,8 @@ private function is_git_timeout_error( mixed $result ): bool {
 			return false;
 		}
 
-		return 'git_command_timeout' === $result->get_error_code();
+		$code = method_exists($result, 'get_error_code') ? $result->get_error_code() : ( $result->code ?? '' );
+		return 'git_command_timeout' === $code;
 	}
 
 	/**

inc/Workspace/WorkspaceWorktreeCleanupEngine.php

@@ -25,14 +25,17 @@ trait WorkspaceWorktreeCleanupEngine {
 	 *   - It is not the primary checkout
 	 *   - Its branch is not main/master/trunk/develop/HEAD
 	 *   - It has no uncommitted changes (unless $force)
-	 *   - At least one merge signal is present:
+	 *   - At least one cleanup signal is present:
 	 *       a) `git for-each-ref` reports upstream status "gone" (branch
 	 *          was deleted on the remote — typical after GitHub
 	 *          "auto-delete head branches" fires on PR merge), OR
 	 *       b) GitHub API reports a closed+merged PR whose head
-	 *          branch matches this worktree's branch.
+	 *          branch matches this worktree's branch, OR
+	 *       c) the clean local worktree has no unpushed commits and is backed
+	 *          by an existing remote branch, so removing the local checkout does
+	 *          not delete recoverable Git state.
 	 *
-	 * Signal (a) is local and fast; signal (b) requires a PAT + network
+	 * Signals (a) and (c) are local and fast; signal (b) requires a PAT + network
 	 * but catches the case where the remote branch still exists (e.g.
 	 * manual merge without branch deletion).
 	 *
@@ -848,7 +851,7 @@ private function classify_worktree_git_probe_failure( string $handle, string $re
 	 * @return string
 	 */
 	private function get_wp_error_code( \WP_Error $error ): string {
-		return (string) $error->get_error_code();
+		return method_exists($error, 'get_error_code') ? (string) $error->get_error_code() : (string) ( $error->code ?? '' );
 	}
 
 	/**
@@ -858,7 +861,7 @@ private function get_wp_error_code( \WP_Error $error ): string {
 	 * @return mixed
 	 */
 	private function get_wp_error_data( \WP_Error $error ): mixed {
-		return $error->get_error_data();
+		return method_exists($error, 'get_error_data') ? $error->get_error_data() : ( $error->data ?? null );
 	}
 
 	/**
@@ -2500,7 +2503,12 @@ private function classify_dirty_obsolete_on_default_branch(
 	 * Signal priority:
 	 *   1. `upstream-gone` — local branch's upstream tracking ref is gone.
 	 *      Typical after GitHub auto-deletes the head branch on PR merge.
-	 *   2. `pr-merged` — GitHub API reports a closed+merged PR for this
+	 *   2. `local-merged` — branch has no commits outside remote default.
+	 *   3. `remote-tracking-clean` — branch still exists on origin, while the
+	 *      local worktree is clean and has no unpushed commits. Age gates are
+	 *      enforced by the caller before removal when retention cleanup passes
+	 *      `older_than`.
+	 *   4. `pr-merged` — GitHub API reports a closed+merged PR for this
 	 *      branch. Requires $skip_github = false and a configured PAT.
 	 *
 	 * @param  string $primary_path Path to the primary git checkout.
@@ -2537,6 +2545,11 @@ private function detect_merge_signal( string $primary_path, string $repo, string
 			return $local_merged;
 		}
 
+		$remote_tracking_clean = $this->detect_remote_tracking_clean_signal($primary_path, $branch);
+		if ( null !== $remote_tracking_clean ) {
+			return $remote_tracking_clean;
+		}
+
 		if ( $skip_github ) {
 			return null;
 		}
@@ -2624,6 +2637,33 @@ private function detect_local_merged_signal( string $primary_path, string $branc
 		);
 	}
 
+	/**
+	 * Detect clean local-only checkouts whose work is preserved by a remote branch.
+	 *
+	 * This is intentionally less strict than a merge signal: the remote branch may
+	 * still be open or unmerged, but the local checkout is only a disposable copy
+	 * when it is clean, has no unpushed commits, and `origin/<branch>` exists.
+	 * Removing the local worktree and local branch does not delete the remote
+	 * branch or close any PR, so the work can be picked up from Git later.
+	 *
+	 * @param  string $primary_path Path to the primary git checkout.
+	 * @param  string $branch       Branch name.
+	 * @return array{signal: string, reason: string, remote_ref: string}|null
+	 */
+	private function detect_remote_tracking_clean_signal( string $primary_path, string $branch ): ?array {
+		$remote_ref = 'refs/remotes/origin/' . $branch;
+		$result     = $this->run_git($primary_path, sprintf('rev-parse --verify --quiet %s', escapeshellarg($remote_ref)), self::CLEANUP_GIT_PROBE_TIMEOUT);
+		if ( is_wp_error($result) ) {
+			return null;
+		}
+
+		return array(
+			'signal'     => 'remote-tracking-clean',
+			'reason'     => 'clean local worktree has no unpushed commits and the branch exists on origin; removing the local checkout preserves remote Git state',
+			'remote_ref' => $remote_ref,
+		);
+	}
+
 	/**
 	 * Extract owner/repo slug from a primary checkout's origin remote.
 	 *

tests/smoke-worktree-cleanup.php

@@ -450,42 +450,35 @@ public function worktree_list( ?string $repo = null, ?string $state = null, arra
     $mixed_dirty_row = array_values($mixed_dirty_skips)[0] ?? array();
     $assert('dirty_worktree', $mixed_dirty_row['reason_code'] ?? '', 'mixed source/artifact dirt stays protected as generic dirty worktree');
 
-    // unmerged-feature should be skipped (no merge signal)
-    $unmerged = array_filter($plan['skipped'] ?? array(), fn( $s ) => ( $s['handle'] ?? '' ) === 'demo@unmerged-feature');
-    $assert(1, count($unmerged), 'unmerged worktree skipped with exactly one entry');
-    $unmerged_row = array_values($unmerged)[0] ?? array();
-    $assert('no_merge_signal', $unmerged_row['reason_code'] ?? '', 'unmerged skip exposes stable reason code');
+    $assert_contains($plan['candidates'] ?? array(), 'demo@unmerged-feature', 'clean remote-backed unmerged worktree flagged as local-only cleanup candidate');
+    $unmerged_candidate = array_values(array_filter($plan['candidates'] ?? array(), fn( $s ) => ( $s['handle'] ?? '' ) === 'demo@unmerged-feature'))[0] ?? array();
+    $assert('remote-tracking-clean', $unmerged_candidate['reason_code'] ?? '', 'unmerged remote-backed candidate exposes stable reason code');
+    $assert('remote-tracking-clean', $unmerged_candidate['signal'] ?? '', 'unmerged remote-backed candidate records cleanup signal');
     $detached = array_values(array_filter($plan['skipped'] ?? array(), fn( $s ) => ( $s['handle'] ?? '' ) === 'demo@feature-detached-stored'))[0] ?? array();
     $assert('detached_worktree', $detached['reason_code'] ?? '', 'detached worktree with stored branch metadata is classified explicitly');
     $assert('feature/detached-stored', $detached['branch'] ?? '', 'detached worktree recovers branch from stored metadata');
     $assert(array( 'branch' ), $detached['hydrated_fields'] ?? array(), 'detached worktree reports hydrated branch field');
     $protected = array_values(array_filter($plan['skipped'] ?? array(), fn( $s ) => ( $s['handle'] ?? '' ) === 'demo@develop'))[0] ?? array();
     $assert('protected_base_branch_worktree', $protected['reason_code'] ?? '', 'protected branch worktree gets actionable protected-base diagnostic');
-    $assert('remote_branch_still_exists', $unmerged_row['merge_signal_evidence']['classification'] ?? '', 'unmerged skip distinguishes existing remote branch');
-    $assert('still_exists', $unmerged_row['merge_signal_evidence']['remote_branch'] ?? '', 'unmerged skip records remote branch evidence');
-    $assert(true, str_contains($unmerged_row['active_review_command'] ?? '', 'active-no-signal-report'), 'unmerged skip includes active/no-signal review command');
-    $assert(true, str_contains($unmerged_row['active_review_commands']['finalized_apply_dry_run'] ?? '', 'active-no-signal-finalized-apply --dry-run'), 'unmerged skip includes finalized PR dry-run apply command');
+    $assert(true, str_contains($unmerged_candidate['reason'] ?? '', 'origin'), 'unmerged remote-backed candidate explains remote preservation');
 
     $external_rows = array_values(array_filter($plan['skipped'] ?? array(), fn( $s ) => ( $s['reason_code'] ?? '' ) === 'external_worktree'));
     $external_row  = $external_rows[0] ?? array();
     $assert('external_worktree', $external_row['reason_code'] ?? '', 'external worktree exposes stable reason code');
     $assert(true, str_contains($external_row['hint'] ?? '', 'outside the DMC workspace'), 'external worktree includes remediation hint');
 
-    $assert(4, (int) ( $plan['summary']['would_remove'] ?? 0 ), 'summary counts cleanup candidates');
+    $assert(6, (int) ( $plan['summary']['would_remove'] ?? 0 ), 'summary counts cleanup candidates');
     $assert(2, (int) ( $plan['summary']['skipped_by_reason']['dirty_worktree'] ?? 0 ), 'summary counts dirty skips by reason');
     $assert(1, (int) ( $plan['summary']['skipped_by_reason']['artifact_only_dirty_worktree'] ?? 0 ), 'summary counts artifact-only dirty skips separately');
     $assert(1, (int) ( $plan['summary']['cleanup_buckets']['artifact_only_dirty_worktree'] ?? 0 ), 'cleanup buckets expose artifact-only dirty count');
     $assert(true, in_array('artifact_only_dirty_worktree', array_column($plan['summary']['skipped_next_commands'] ?? array(), 'reason_code'), true), 'summary exposes artifact-only dirty next command');
-    $assert(true, isset($plan['summary']['skipped_by_reason']['no_merge_signal']), 'summary includes no_merge_signal bucket');
+    $assert(false, isset($plan['summary']['skipped_by_reason']['no_merge_signal']), 'summary has no no_merge_signal bucket when clean remote-backed worktrees are removable');
 	$assert(4096, (int) ( $plan['summary']['total_size_bytes'] ?? -1 ), 'summary counts artifact-only dirty worktree size bytes');
 	$assert(4096, (int) ( $plan['summary']['artifact_size_bytes'] ?? -1 ), 'summary counts artifact-only dirty artifact size bytes');
 	$top_by_size = (array) ( $plan['summary']['top_by_size'] ?? array() );
 	$assert('demo@artifact-only-dirty', $top_by_size[0]['handle'] ?? '', 'summary top-by-size includes artifact-only dirty worktree');
 	$next_commands = (array) ( $plan['summary']['skipped_next_commands'] ?? array() );
-	$assert(true, in_array('no_merge_signal', array_column($next_commands, 'reason_code'), true), 'summary includes no_merge_signal next command');
-	$no_merge_commands = array_values(array_filter($next_commands, fn( $row ) => ( $row['reason_code'] ?? '' ) === 'no_merge_signal'))[0] ?? array();
-	$assert(true, str_contains($no_merge_commands['command'] ?? '', 'active-no-signal-report'), 'no_merge_signal next command routes to active/no-signal evidence report');
-	$assert(true, str_contains($no_merge_commands['commands']['equivalent_clean_apply_dry_run'] ?? '', 'active-no-signal-equivalent-clean-apply --dry-run'), 'no_merge_signal next commands include equivalent-clean dry-run apply');
+	$assert(false, in_array('no_merge_signal', array_column($next_commands, 'reason_code'), true), 'summary omits no_merge_signal next command when no rows need active/no-signal review');
 	$assert(true, array_key_exists('total_size_bytes', $plan['summary'] ?? array()), 'summary exposes total worktree size bytes field');
 	$assert(true, array_key_exists('artifact_size_bytes', $plan['summary'] ?? array()), 'summary exposes artifact size bytes field');
 	$assert(true, array_key_exists('top_by_size', $plan['summary'] ?? array()), 'summary exposes top worktrees by size field');
@@ -670,10 +663,11 @@ public function worktree_list( ?string $repo = null, ?string $state = null, arra
         )
     );
     $assert(true, ! is_wp_error($age_plan) && ( $age_plan['success'] ?? false ), 'age-filtered dry_run returns success');
-    $assert(1, (int) ( $age_plan['summary']['would_remove'] ?? 0 ), 'older_than keeps only old cleanup candidate');
+    $assert(2, (int) ( $age_plan['summary']['would_remove'] ?? 0 ), 'older_than keeps old merged and remote-backed cleanup candidates');
     $assert(1, (int) ( $age_plan['summary']['age_filter']['excluded'] ?? 0 ), 'age filter summary counts newer candidate exclusion');
-    $assert(2, (int) ( $age_plan['summary']['age_filter']['unknown_age'] ?? 0 ), 'age filter summary counts unknown-age candidate exclusions');
+    $assert(3, (int) ( $age_plan['summary']['age_filter']['unknown_age'] ?? 0 ), 'age filter summary counts unknown-age candidate exclusions');
     $assert_contains($age_plan['candidates'] ?? array(), 'demo@merged-autodelete', 'older_than keeps old merged worktree');
+    $assert_contains($age_plan['candidates'] ?? array(), 'demo@unmerged-feature', 'older_than keeps old clean remote-backed worktree');
     $recent_age_rows = array_values(array_filter($age_plan['skipped'] ?? array(), fn( $s ) => ( $s['handle'] ?? '' ) === 'demo@merged-recent'));
     $assert('age_filter', $recent_age_rows[0]['reason_code'] ?? '', 'newer merged worktree is skipped by age_filter');
     $assert('excluded', $recent_age_rows[0]['age_filter']['decision'] ?? '', 'age-filter skip row exposes excluded decision');
@@ -820,8 +814,8 @@ public function worktree_list( ?string $repo = null, ?string $state = null, arra
     // Primary survives.
     $assert(true, is_dir($primary . '/.git'), 'primary .git survives cleanup');
 
-    // Protected branches: unmerged/dirty worktrees survive.
-    $assert(true, is_dir($tmp . '/demo@unmerged-feature'), 'unmerged worktree survives cleanup');
+    // Dirty/external worktrees survive; clean remote-backed worktrees are local-only cleanup candidates.
+    $assert(false, is_dir($tmp . '/demo@unmerged-feature'), 'clean remote-backed unmerged worktree is removed locally');
     $assert(true, is_dir($tmp . '/demo@dirty-branch'), 'dirty worktree survives cleanup');
     $assert(true, is_dir($tmp . '/demo@merged-stale-plan'), 'dirty stale-plan worktree survives cleanup');
     $assert(true, is_dir($tmp . '-external/demo-external'), 'external worktree survives cleanup');