-
Notifications
You must be signed in to change notification settings - Fork 37
Fix entire clean logging leak and branch deletion with packed refs #201
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.
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
Conversation
…d refs Two bugs fixed: 1. Structured logs from `entire clean` leaked to stderr because `logging.Init()` was never called. Added initialization in `runClean()` so logs go to `.entire/logs/entire.log`. 2. `--force` reported branches as deleted but they persisted because go-git v5's `RemoveReference()` doesn't work with packed refs or worktrees. Replaced all non-test `RemoveReference` calls with `git branch -D` via CLI, consistent with the existing `HardResetWithProtection()` pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: da8d550b66f6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses two operational issues in the Entire CLI cleanup path: (1) structured logs from entire clean appearing on stderr due to missing logging initialization, and (2) shadow branch deletions not persisting when refs are packed or in worktree contexts by switching branch deletion from go-git to git branch -D.
Changes:
- Initialize structured logging in
runClean()so cleanup logs go to.entire/logs/entire.loginstead of stderr. - Replace go-git
RemoveReference()branch deletion withgit branch -Dvia a sharedDeleteBranchCLIhelper and update call sites. - Adjust cleanup tests to verify branch deletion via git CLI (avoids go-git ref cache/staleness issues).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/clean.go | Initializes logging for entire clean so structured cleanup logs are routed to the log file. |
| cmd/entire/cli/sessions_fix.go | Switches stuck-session shadow branch cleanup to use CLI-based branch deletion. |
| cmd/entire/cli/strategy/common.go | Adds DeleteBranchCLI() helper using git branch -D. |
| cmd/entire/cli/strategy/cleanup.go | Uses DeleteBranchCLI() for deleting orphaned shadow branches. |
| cmd/entire/cli/strategy/clean_test.go | Verifies branch deletion using git CLI for accuracy vs go-git ref visibility. |
| cmd/entire/cli/strategy/manual_commit_git.go | Updates shadow-branch deletion helper to use CLI deletion and treat “not found” as idempotent. |
| cmd/entire/cli/strategy/manual_commit_condensation.go | Updates shadow-branch cleanup to delete via CLI and treat “not found” as non-fatal. |
| cmd/entire/cli/strategy/manual_commit_migration.go | Deletes old shadow branches via CLI during migration (packed refs/worktrees workaround). |
| cmd/entire/cli/strategy/manual_commit_reset.go | Deletes shadow branches via CLI during reset. |
| cmd/entire/cli/checkpoint/temporary.go | Switches checkpoint store shadow-branch deletion to git branch -D for packed refs/worktrees reliability. |
- Add idempotent "not found" handling in discardSession so deleting an already-gone shadow branch doesn't error - Set logging.SetLogLevelGetter before logging.Init in clean command - Remove stale docstring about repo opening in DeleteShadowBranches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 7be20d70c73b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
- Add ErrBranchNotFound sentinel error and git show-ref pre-check in DeleteBranchCLI so callers use errors.Is instead of string parsing - Add branchExistsCLI helper for CLI-based branch existence checks - Replace repo.Reference() with branchExistsCLI in ResetSession to avoid stale go-git cache after CLI-based deletion - Add -- separator before branch name in git branch -D calls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 861c10d60125
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
Only map exit code 1 (ref not found) to ErrBranchNotFound. Propagate exit code 128+ (fatal git errors like corrupt repo or permissions) as real errors so callers don't silently swallow them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4cff5845d5a2
Summary
entire cleanleaked structured logs (e.g.INFO deleted orphaned shadow branch...) to stderr instead of.entire/logs/entire.logbecauselogging.Init()was never called. Added initialization inrunClean().entire clean --forcereported shadow branches as deleted but they persisted. Root cause: go-git v5'sRemoveReference()doesn't work with packed refs (.git/packed-refs) or worktree contexts. Replaced all 7 non-testRemoveReferencecall sites withgit branch -Dvia CLI, consistent with the existingHardResetWithProtection()workaround pattern.Test plan
mise run fmt— passesmise run lint— passes (0 issues)mise run test:ci— all unit + integration tests passentire clean -f, verify branches are actually gone withgit branch -l 'entire/*'🤖 Generated with Claude Code
Note
Medium Risk
Switching branch deletion to
gitsubprocesses changes behavior and error handling across multiple session/cleanup paths and depends on a working git executable/workdir; mistakes could leave stale shadow branches or delete the wrong ref.Overview
Fixes
entire cleanto initialize structured logging so cleanup audit logs are written to.entire/logs/instead of leaking to stderr.Replaces shadow-branch deletion across cleanup/session flows from go-git
RemoveReferenceto a newstrategy.DeleteBranchCLI(backed bygit show-ref+git branch -D) to reliably delete branches when refs are packed or in worktree contexts; updates related code paths (checkpoint.GitStore.DeleteShadowBranch,doctordiscard, manual-commit reset/migration/condensation) and adjusts tests to validate deletion via git CLI rather than go-git ref reads.Written by Cursor Bugbot for commit 343d64a. This will update automatically on new commits. Configure here.