fix(core): respect worktree.baseBranch in cleanup service#1424
fix(core): respect worktree.baseBranch in cleanup service#1424truffle-dev wants to merge 1 commit intocoleam00:devfrom
Conversation
The cleanup service called getDefaultBranch directly at three sites without first consulting `worktree.baseBranch` from `.archon/config.yaml`. On repos where the default branch is `master` and `origin/HEAD` is unset, git auto-detection throws and produces repeated `env_cleanup_error` logs on every startup — even though the error message tells users to set `worktree.baseBranch`, which the cleanup service silently ignored. Add a small `resolveBaseBranch` helper that reads `loadRepoConfig` first and only falls back to `getDefaultBranch` when the config has no value (or only whitespace). Sweep all three call sites: - runScheduledCleanup (line 327) - getWorktreeStatusBreakdown (line 481) - cleanupMergedWorktrees (line 609) The same pattern already exists in `packages/git/src/repo.ts:syncWorkspace`, where the caller passes a `baseBranch` parameter and falls back to auto-detection only when omitted. Five regression tests cover: config-respect at all three sites, fallback when config is empty, and the whitespace-only edge case. Closes coleam00#1419
📝 WalkthroughWalkthroughThe PR adds config-aware base branch resolution to the cleanup service. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/services/cleanup-service.ts (1)
27-42: The code is functionally correct; consider adding a comment explaining why the PR 1423 decision was reversed.The switch to
loadRepoConfigis safe and properly mitigated by the test mock setup incleanup-service.test.ts(lines 99–100), which short-circuits the@archon/providersdependency chain before importing the cleanup service. TheRepoConfiginterface correctly includesworktree?.baseBranch?: string, and no test failures are evident.However, PR 1423 deliberately avoided
loadRepoConfigfor this exact reason. Since that decision has been reversed, a brief comment inresolveBaseBranch(e.g., "We now useloadRepoConfigbecause test mocks handle the@archon/providerschain") would clarify intent for future maintainers and prevent accidental reversion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/services/cleanup-service.ts` around lines 27 - 42, Add a brief explanatory comment above resolveBaseBranch saying we intentionally use loadRepoConfig (reversing the prior PR 1423 decision) because test mocks short-circuit the `@archon/providers` dependency chain and RepoConfig includes worktree.baseBranch; mention that this prevents noisy git auto-detection and that getDefaultBranch remains the fallback. Reference the function name resolveBaseBranch, the loader loadRepoConfig, and the fallback getDefaultBranch in the comment so future maintainers understand the reasoning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/services/cleanup-service.ts`:
- Around line 27-42: Add a brief explanatory comment above resolveBaseBranch
saying we intentionally use loadRepoConfig (reversing the prior PR 1423
decision) because test mocks short-circuit the `@archon/providers` dependency
chain and RepoConfig includes worktree.baseBranch; mention that this prevents
noisy git auto-detection and that getDefaultBranch remains the fallback.
Reference the function name resolveBaseBranch, the loader loadRepoConfig, and
the fallback getDefaultBranch in the comment so future maintainers understand
the reasoning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4816ad59-cd1a-4260-bcb7-fb6f5bf2ba11
📒 Files selected for processing (2)
packages/core/src/services/cleanup-service.test.tspackages/core/src/services/cleanup-service.ts
|
Closing this. I missed that @kagura-agent opened #1423 at 09:08Z with the same approach (resolveMainBranch helper, 3 call sites swept) two hours before I opened this one. Sorry for the duplicate — should have re-checked open PRs before pushing rather than relying on a stale scout note. If you end up wanting any of the extras here once #1423 lands, the differences are:
Happy to drop those as a follow-up PR after #1423 merges if useful, or open a comment on #1423 with the whitespace nit. |
Closes #1419.
Bug
On startup, the cleanup service throws repeated
env_cleanup_errorfor every tracked environment whose repo has a non-maindefault branch and noorigin/HEAD(e.g. repos usingmaster). The error message itself tells users to setworktree.baseBranchin.archon/config.yaml— but the cleanup service never reads that config, so the suggestion is a dead end.Root cause
packages/core/src/services/cleanup-service.tscallsgetDefaultBranch(repoPath)directly at three call sites without first consultingloadRepoConfig:runScheduledCleanup— runs on every startup tick, source of the reported log spamgetWorktreeStatusBreakdown— same auto-detect path used by status displayscleanupMergedWorktrees— same path used when cleaning merged worktreesThe same pattern is already implemented correctly in
packages/git/src/repo.ts:syncWorkspace, where the caller passes abaseBranchparameter and falls back to auto-detection only when omitted.Fix
Add a small
resolveBaseBranchhelper that readsloadRepoConfigfirst and only falls back togetDefaultBranchwhen the config has no value (or only whitespace):All three call sites swap
getDefaultBranch(repoPath)→resolveBaseBranch(repoPath). The reporter only flagged the first site; sweeping all three keeps the behavior consistent across the cleanup surface.Tests
Five new tests in
packages/core/src/services/cleanup-service.test.ts:runScheduledCleanupusesworktree.baseBranchfrom repo config and skips git auto-detectionrunScheduledCleanupfalls back togetDefaultBranchwhen repo config has nobaseBranchrunScheduledCleanuptreats whitespace-onlybaseBranchas unset and falls backgetWorktreeStatusBreakdownusesworktree.baseBranchfor merge detectioncleanupMergedWorktreesusesworktree.baseBranchwhen comparing merge stateExisting 42 tests in the file continue to pass.
Verification
bun run typecheck— clean across all 10 packagesbunx prettier --checkon both files — cleanbunx eslintoncleanup-service.ts— clean (0 errors, 0 warnings)upstream/devper CONTRIBUTING.mdSummary by CodeRabbit
New Features
Tests