Skip to content

fix: make branch cleanup reliable on failure and in clean - #55

Merged
vitali87 merged 4 commits into
mainfrom
fix/reliable-branch-cleanup
Aug 31, 2026
Merged

fix: make branch cleanup reliable on failure and in clean#55
vitali87 merged 4 commits into
mainfrom
fix/reliable-branch-cleanup

Conversation

@vitali87

Copy link
Copy Markdown
Owner

Summary

Three related cleanup gaps found by review:

  • Failed groups left branches behind. add_worktree creates the branch before the commit step, but the failure path in _create_branches_and_commits only deleted branches of successful groups. Reproduced: after pr-2 failed, git branch --list 'pr-split/*' still showed pr-split/ns/pr-2. Now the failed groups' branches are deleted as well (silently, since add_worktree itself may have been the failure).
  • delete_branch(remote=True) skipped the remote. If git branch -D failed (branch checked out to inspect CI, or already removed by merge --delete-branch), the git push origin --delete never ran. Now the remote deletion is always attempted and the local error re-raised afterwards.
  • clean discarded the plan on partial failure. plan.json was unlinked unconditionally, so an orphaned remote branch or PR record was lost. Now the plan is kept when any PR/branch could not be cleaned, clean prints why and exits 1 so it can be re-run.

Test plan

  • New tests: failed group's branch deleted; local failure still deletes remote; local failure without remote raises immediately; partial cleanup keeps the plan
  • uv run ruff check / ruff format --check clean
  • uv run pytest -q — 447 passed

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 39 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5653a6fa-2ad4-468c-8346-37082c9eb3a0

📥 Commits

Reviewing files that changed from the base of the PR and between 408cffb and 6d5a5f0.

📒 Files selected for processing (6)
  • pr_split/cli.py
  • pr_split/git_ops/branches.py
  • pr_split/logs.py
  • tests/test_cli_helpers.py
  • tests/test_cli_new_features.py
  • tests/test_git_branches.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Aug 30, 2026

Copy link
Copy Markdown

Greptile Summary

This change expands cleanup for failed branch creation and retains the saved plan when cleanup is incomplete. Reproduction found that failed branch setup can delete a branch that existed before the command started, and that cleanup retries can remain incomplete after remote branches were already deleted because completion is neither persisted nor treated idempotently.

Confidence Score: 3/5

Not safe to merge until failed branch cleanup preserves pre-existing branches and cleanup retries can recognize resources already removed.

Two independent reproduced failures affect destructive branch handling and cleanup completion. Both were exercised with isolated runtime harnesses and captured outputs.

Files Needing Attention: pr_split/cli.py needs changes around failed-group branch cleanup and retry accounting; pr_split/git_ops/branches.py should be considered when making remote deletion idempotent.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex authored an isolated Git failure-path harness to reproduce the P1 finding and captured before- and after-state logs to verify the test scenario.
  • T-Rex produced proofs for multiple P1 findings and referenced them in review comments.
  • T-Rex performed general contract validation, confirming that the before-capture SHAs matched, the after-capture state included the expected deletion, and that the harness and command-output captures were uploaded.
  • T-Rex ran the requested verification, but its local artifact references were not uploaded.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (3)

  1. pr_split/cli.py, line 939-944 (link)

    P1 Remote branch cleanup retries never converge

    Successful branch deletions are not persisted, so a later clean retry attempts to delete every original remote branch again. Git reports an already-deleted remote branch as an error, which leaves deleted_branches below the original record count even though the branch is gone; clean then retains the plan and exits unsuccessfully indefinitely. Treat an absent remote branch as already cleaned up, or persist completed cleanup records so retries operate only on unfinished resources.

    T-Rex Ran code and verified through T-Rex

  2. General comment

    P1 Failure cleanup deletes a branch that existed before the split operation

    • Bug
      • When a target branch already exists, add_worktree saves its SHA, deletes it temporarily, and restores it if git worktree add -b fails. _create_branches_and_commits then treats the failed group name as newly created and calls delete_branch on it. The restored pre-existing branch is therefore force-deleted.
    • Cause
      • pr_split/cli.py:326 synthesizes every failed group's branch name without recording whether that branch existed before the operation; pr_split/cli.py:327-329 deletes every synthesized failed name. This loses the provenance established by pr_split/git_ops/branches.py:111-120.
    • Fix
      • Have add_worktree (or its caller) return/retain whether it replaced an existing branch, and only clean failed branch names that were newly created by this invocation. Alternatively, pre-record existing target branch names/SHAs before scheduling groups and exclude those names from failure cleanup.

    T-Rex Ran code and verified through T-Rex

  3. General comment

    P1 Cleanup retries never complete after a remote branch was deleted during an earlier partial attempt

    • Bug
      • After any partial cleanup, _cleanup_git_state retries every original branch record. A previously deleted remote makes git push origin --delete <branch> fail. The error is caught but not treated as completed or persisted, so deleted_branches remains below the original record count even when every remote resource is already gone. clean() then exits 1 and keeps the plan, causing indefinite retry failure.
    • Cause
      • pr_split/cli.py:928-951 uses only per-invocation success counters and does not persist completed records. pr_split/git_ops/branches.py:73-87 propagates the expected missing-remote error from the delete command.
    • Fix
      • Make branch cleanup idempotent: treat a missing remote branch as successful, or query remote existence before deletion. Also persist successful PR/branch record removal (or rewrite git_state) after each success so retries operate only on unfinished resources. Keep PR behavior unchanged because gh pr close is already idempotent for closed PRs.

    T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "fix: make branch cleanup reliable on fai..." | Re-trigger Greptile

Comment thread pr_split/cli.py Outdated
- A group that failed after add_worktree left its branch behind because
  only successful groups were cleaned up; delete failed groups' branches too
- delete_branch(remote=True) skipped the remote deletion when the local
  branch could not be deleted (checked out, or already removed); always
  attempt the remote deletion and re-raise the local error afterwards
- clean removed plan.json even when some PRs or branches could not be
  cleaned up, losing the records needed to retry; keep the plan and exit
  non-zero when cleanup is incomplete
Move the failed-group branch deletion into the worker, after add_worktree
has succeeded. If add_worktree itself fails it restores any pre-existing
branch of that name, and that branch must not be removed.
@vitali87
vitali87 force-pushed the fix/reliable-branch-cleanup branch from 247c028 to 1f73aa3 Compare August 30, 2026 19:24
@vitali87
vitali87 merged commit d6c8930 into main Aug 31, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant