Skip to content

Fix stale clean remote-backed worktree cleanup #602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chubes4 merged 1 commit into from
Jun 7, 2026
Merged

Fix stale clean remote-backed worktree cleanup #602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions inc/Workspace/WorkspaceActiveNoSignalCleanup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) {
Expand Down Expand Up @@ -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.
*
Expand Down
3 changes: 2 additions & 1 deletion inc/Workspace/WorkspaceCoreUtilities.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down
52 changes: 46 additions & 6 deletions inc/Workspace/WorkspaceWorktreeCleanupEngine.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
*
Expand Down Expand Up @@ -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 ?? '' );
}

/**
Expand All @@ -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 );
}

/**
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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.
*
Expand Down
32 changes: 13 additions & 19 deletions tests/smoke-worktree-cleanup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand Down
Loading