fix(core): respect worktree.baseBranch in cleanup service default branch resolution

[kagura-agent]

Problem

The cleanup service called getDefaultBranch() directly in three places (merged-branch detection, status breakdown, scheduled cleanup) without consulting the configured worktree.baseBranch from .archon/config.yaml. This caused repeated env_cleanup_error log entries on every startup for repositories where the default branch is not main and origin/HEAD is not set (e.g. repos using master).

The error message itself advises users to set worktree.baseBranch — but the cleanup service never reads it, making the suggestion ineffective.

Fix

Add a resolveMainBranch() helper in cleanup-service.ts that:

1. Reads worktree.baseBranch from .archon/config.yaml (via Bun.YAML.parse)
2. Falls back to getDefaultBranch() auto-detection when no config exists or the field is unset

Replace all three getDefaultBranch() call sites in cleanup-service with resolveMainBranch().

Note: Other callers like executor.ts and workflow.ts already respect this config — this aligns the cleanup service with the existing pattern.

Changes

• packages/core/src/services/cleanup-service.ts: Add resolveMainBranch(), replace 3 call sites
• packages/core/src/services/cleanup-service.test.ts: Add 2 tests (config override + fallback)

Validation

• All 44 tests pass (bun test packages/core/src/services/cleanup-service.test.ts)
• Lint-staged passes (eslint + prettier)
• No new type-check errors introduced

Closes #1419

Side Effects / Blast Radius (required)

• Affected subsystems/workflows: cleanupMergedWorktrees, getWorktreeStatusBreakdown, runScheduledCleanup — all three getDefaultBranch() call sites in cleanup-service now go through resolveMainBranch()
• Potential unintended effects: If .archon/config.yaml has an invalid worktree.baseBranch value (e.g. a branch that doesn't exist), the cleanup service will use it and isBranchMerged will fail to find merged branches — but this is the same behavior as other callers (executor.ts, workflow.ts) that already read this config
• Guardrails/monitoring for early detection: The catch block in resolveMainBranch() treats config load failure as non-fatal and falls back to getDefaultBranch(). Existing env_cleanup_error log entries will disappear for repos with baseBranch configured (the desired outcome)

Rollback Plan (required)

• Fast rollback command/path: Revert this commit — the three call sites revert from resolveMainBranch() back to getDefaultBranch(), restoring the previous behavior
• Feature flags or config toggles: The existing worktree.baseBranch config key acts as an implicit toggle — if the field is unset or the config file is absent, behavior is identical to before this PR
• Observable failure symptoms: If rollback is needed, repos using master (or any non-main default branch) without origin/HEAD set will again produce env_cleanup_error log entries on every startup

Summary by CodeRabbit

• New Features

  • Cleanup and merge-detection now respect a repository-configured base branch for determining the main branch, while automatically falling back to the previous default-branch behavior when no valid configuration exists.

• Tests

  • Added coverage validating base-branch resolution and fallback behavior (including invalid or missing configs) to ensure merge checks and merged-worktree cleanup use the configured or default branch as appropriate.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds an async helper to resolve a repository’s main branch by reading .archon/config.yaml (worktree.baseBranch, normalized via toBranchName) and falling back to getDefaultBranch on error or absence. Updates cleanup-service call sites to use this resolver and adds tests for configured and fallback behaviors.

Changes

Cohort / File(s)	Summary
Main branch resolution & call-site updates packages/core/src/services/cleanup-service.ts	Adds resolveMainBranch which loads repo config and returns worktree.baseBranch (trimmed + toBranchName) if available; otherwise falls back to getDefaultBranch. Replaces direct getDefaultBranch calls in runScheduledCleanup, getWorktreeStatusBreakdown, and cleanupMergedWorktrees.
Tests: cleanup service packages/core/src/services/cleanup-service.test.ts	Adds tests asserting that when worktree.baseBranch is a valid string, getDefaultBranch is not called and merge checks use the configured branch; adds coverage for fallback cases (invalid YAML, non-string, null) ensuring getDefaultBranch is invoked once and used for merge checks.

Sequence Diagram(s)

sequenceDiagram
    participant Cleanup as CleanupService
    participant Config as ConfigLoader
    participant Git as GitUtils
    Cleanup->>Config: loadRepoConfig(repoPath)
    alt config.worktree.baseBranch exists
        Config-->>Cleanup: baseBranch
        Cleanup->>Git: toBranchName(trim(baseBranch))
        Git-->>Cleanup: normalizedBranch
        Cleanup->>Cleanup: use normalizedBranch for merge checks / cleanup
    else config missing or parse error
        Config-->>Cleanup: null / error
        Cleanup->>Git: getDefaultBranch(repoPath)
        Git-->>Cleanup: defaultBranch
        Cleanup->>Cleanup: use defaultBranch for merge checks / cleanup
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• fix(isolation): complete reports false success when worktree remains on disk (fixes #964) #1034 — Modifies the same cleanup-service implementation and related call sites; likely directly related to main-branch resolution changes.

Poem

🐰 I peeked in config with a twitch and a hop,

Found baseBranch hiding — I gave it a chop,
Trimmed and named it, then danced in the night,
Now cleanup knows left from right. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main fix: respecting worktree.baseBranch in the cleanup service's default branch resolution logic.
Description check	✅ Passed	The PR description comprehensively covers the problem, solution, validation evidence, side effects, and rollback plan, closely following the required template structure with all critical sections filled.
Linked Issues check	✅ Passed	The code changes fully address issue #1419: resolveMainBranch() reads worktree.baseBranch from config and falls back to getDefaultBranch(); three call sites are updated; tests verify both config override and fallback behavior.
Out of Scope Changes check	✅ Passed	All changes are tightly scoped to the cleanup service's base branch resolution: new resolveMainBranch() helper, three updated call sites, and corresponding test coverage with no extraneous modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

packages/core/src/services/cleanup-service.test.ts (1)

1463-1507: Strengthen the override test by also asserting getDefaultBranch is not called.

The PR objective for #1419 explicitly calls for "a unit test verifying getDefaultBranch is not called when the config provides baseBranch" — i.e., the whole point is skipping git auto-detection (which is the source of the startup error spam). Today the test only asserts the value passed to isBranchMerged. A loud regression — e.g., someone refactors resolveMainBranch to call getDefaultBranch first and override afterward — would still pass.

Also: mergeResult is captured but never asserted on; either drop the binding or assert against it.

♻️ Suggested additional assertions

         const mergeResult = await cleanupMergedWorktrees('codebase-1', tempDir);

         // isBranchMerged should have been called with 'master' (from config)
         const branchMergeCalls = mockIsBranchMerged.mock.calls.filter(
           (call: unknown[]) => call[0] === tempDir
         );
         expect(branchMergeCalls).toHaveLength(1);
         expect(branchMergeCalls[0][2]).toBe('master');
+        // Critical: git auto-detection must be bypassed when config provides baseBranch
+        expect(mockGetDefaultBranch).not.toHaveBeenCalledWith(tempDir);
+        expect(mergeResult.removed).toContain('feature-x');
       } finally {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/services/cleanup-service.test.ts` around lines 1463 - 1507,
The test for cleanupMergedWorktrees currently verifies isBranchMerged gets
'master' from the config but doesn't assert that git auto-detection was skipped;
update the test to additionally assert that the getDefaultBranch mock (or the
mocked function used by resolveMainBranch/git detection) was not called
(expect(mockGetDefaultBranch).not.toHaveBeenCalled()) to ensure auto-detection
is bypassed, and either remove the unused mergeResult binding or add an
assertion on mergeResult (e.g., expected shape/status) to avoid unused-variable
noise; reference the test's use of cleanupMergedWorktrees, mockIsBranchMerged,
and the mocked getDefaultBranch function when making these changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/src/services/cleanup-service.test.ts`:
- Around line 1509-1516: The test currently short-circuits because
mockListByCodebase returns an empty array; update the test so mockListByCodebase
resolves to a single environment entry (so the for-loop executes) and clear
mockGetDefaultBranch before calling cleanupMergedWorktrees to avoid prior-test
leakage (use mockGetDefaultBranch.mockClear() or resetMocks). Then assert
mockGetDefaultBranch was called exactly once with '/nonexistent/repo' and assert
that isBranchMerged (or the consumer that checks merge status) was invoked with
the fallback branch value ('main') to verify the resolver→consumer chain
(reference cleanupMergedWorktrees, mockListByCodebase, mockGetDefaultBranch, and
isBranchMerged).
- Around line 1462-1518: The new describe('resolveMainBranch config override',
...) is incorrectly nested inside the describe('cleanupStaleWorktrees', ...)
block but tests exercise cleanupMergedWorktrees; move that entire describe block
so it is a sibling (or inside) the describe('cleanupMergedWorktrees', ...) block
and not inside cleanupStaleWorktrees, and add a beforeEach inside that describe
to clear/reset the mocks it depends on (mockIsBranchMerged,
mockGetDefaultBranch, mockGetPrState, mockIsPatchEquivalent, mockListByCodebase)
using jest.clearAllMocks() or individually mockClear/mockReset before each test
so one-shot returns and call history cannot leak. Ensure tests still call
cleanupMergedWorktrees and keep assertions unchanged.

In `@packages/core/src/services/cleanup-service.ts`:
- Around line 41-54: The resolveMainBranch function currently re-implements YAML
loading and uses unsafe type assertions; change it to call
loadRepoConfig(repoPath) and, if loadRepoConfig returns a baseBranch string,
return toBranchName(result.baseBranch) (fall back to getDefaultBranch(repoPath)
otherwise). Replace the double-cast approach by checking typeof raw === 'string'
when validating baseBranch (use the already-normalized baseBranch from
loadRepoConfig), and in the catch block add a debug/process logger entry that
includes the caught error to aid diagnosing malformed or missing
.archon/config.yaml files.

---

Nitpick comments:
In `@packages/core/src/services/cleanup-service.test.ts`:
- Around line 1463-1507: The test for cleanupMergedWorktrees currently verifies
isBranchMerged gets 'master' from the config but doesn't assert that git
auto-detection was skipped; update the test to additionally assert that the
getDefaultBranch mock (or the mocked function used by resolveMainBranch/git
detection) was not called (expect(mockGetDefaultBranch).not.toHaveBeenCalled())
to ensure auto-detection is bypassed, and either remove the unused mergeResult
binding or add an assertion on mergeResult (e.g., expected shape/status) to
avoid unused-variable noise; reference the test's use of cleanupMergedWorktrees,
mockIsBranchMerged, and the mocked getDefaultBranch function when making these
changes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c923c077-9402-4b3a-a652-bced306872b2

📥 Commits

Reviewing files that changed from the base of the PR and between 9122673 and 4d05a4f.

📒 Files selected for processing (2)

• packages/core/src/services/cleanup-service.test.ts
• packages/core/src/services/cleanup-service.ts

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether loadRepoConfig already exists and exposes worktree.baseBranch
ast-grep --pattern 'export function loadRepoConfig($$$) { $$$ }'
ast-grep --pattern 'export async function loadRepoConfig($$$) { $$$ }'
rg -nP --type=ts -C3 '\bloadRepoConfig\b'
# Also confirm baseBranch normalization is centralized
rg -nP --type=ts -C2 '\bbaseBranch\b' -g '!**/*.test.ts' -g '!**/dist/**'

Repository: coleam00/Archon

Length of output: 38421

---

Refactor to use loadRepoConfig for consistent config handling.

The codebase already has loadRepoConfig() (in packages/core/src/config/config-loader.ts) which reads, parses, and normalizes worktree.baseBranch from .archon/config.yaml. Re-implementing YAML reading and extraction here creates a parallel codepath that can drift during schema evolution. Use loadRepoConfig(repoPath) and access result.baseBranch instead, which is already normalized with .trim() applied.

Additionally:

1. Replace the double type assertion with a typeof raw === 'string' check for defensive runtime validation
2. Enhance the bare catch block with a debug log to aid diagnosis of misconfigured .archon/config.yaml files

Suggested fix

 async function resolveMainBranch(repoPath: RepoPath): Promise<BranchName> {
-  try {
-    const content = await readFile(join(repoPath, '.archon', 'config.yaml'), 'utf-8');
-    const config = Bun.YAML.parse(content) as Record<string, unknown> | null;
-    const worktree = (config as Record<string, Record<string, unknown>> | null)?.worktree;
-    const configured = (worktree?.baseBranch as string | undefined)?.trim();
+  const config = await loadRepoConfig(repoPath);
+  const configured = config.baseBranch;
+  if (configured) {
+    try {
+      return toBranchName(configured);
+    } catch (err) {
+      getLog().debug({ err, repoPath }, 'repo_config_base_branch_parse_failed');
+    }
+  }
-    if (configured) {
-      return toBranchName(configured);
-    }
-  } catch (err) {
-    // Config load failure (missing file, parse error) is non-fatal — fall through to auto-detection
-    getLog().debug({ err, repoPath }, 'repo_config_load_failed');
-  }
   return getDefaultBranch(repoPath);
 }

🤖 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 41 - 54, The
resolveMainBranch function currently re-implements YAML loading and uses unsafe
type assertions; change it to call loadRepoConfig(repoPath) and, if
loadRepoConfig returns a baseBranch string, return
toBranchName(result.baseBranch) (fall back to getDefaultBranch(repoPath)
otherwise). Replace the double-cast approach by checking typeof raw === 'string'
when validating baseBranch (use the already-normalized baseBranch from
loadRepoConfig), and in the catch block add a debug/process logger entry that
includes the caught error to aid diagnosing malformed or missing
.archon/config.yaml files.

[kagura-agent]

Considered this, but loadRepoConfig() imports config-loader.ts which has a top-level import from '@archon/providers'. This triggers a module resolution failure in the cleanup-service test suite because those tests don't (and shouldn't need to) mock the entire providers registry.

The inline YAML read is intentionally self-contained to avoid pulling in the config-loader dependency chain. Happy to revisit if config-loader gets decoupled from @archon/providers in the future.

[coderabbitai]

@kagura-agent, that's a clear and valid constraint — pulling config-loader.ts into cleanup-service.ts would drag @archon/providers into the test dependency graph, which is a real cost. The self-contained inline read is the right call here.

The refactoring suggestion is withdrawn. Two smaller hardening points from the original comment still apply independently, if you'd like them:

1. typeof guard instead of double cast — replaces the as Record<string, Record<string, unknown>> | null re-cast with a runtime check:

   const worktree = config !== null && typeof config === 'object' && 'worktree' in config
     ? (config as Record<string, unknown>).worktree
     : undefined;
   const raw = worktree !== null && typeof worktree === 'object' && 'baseBranch' in (worktree as object)
     ? (worktree as Record<string, unknown>).baseBranch
     : undefined;
   const configured = typeof raw === 'string' ? raw.trim() : undefined;

2. Debug log in catch — helps diagnose genuinely malformed .archon/config.yaml files without being noisy for the common "file doesn't exist" case:

   } catch (err) {
     getLog().debug({ err, repoPath }, 'resolve_main_branch_config_load_failed');
   }

Neither is blocking. Happy to close out the comment as-is if you'd prefer to keep the current form.

---

✏️ Learnings added

Learnt from: kagura-agent
Repo: coleam00/Archon PR: 1423
File: packages/core/src/services/cleanup-service.ts:41-54
Timestamp: 2026-04-26T09:16:35.436Z
Learning: In `packages/core/src/services/cleanup-service.ts`, `resolveMainBranch` intentionally uses an inline `readFile` + `Bun.YAML.parse` instead of `loadRepoConfig()` from `packages/core/src/config/config-loader.ts`. The reason is that `config-loader.ts` has a top-level `import from 'archon/providers'`, which causes module resolution failures in the cleanup-service test suite (tests do not mock the full providers registry). The self-contained YAML read avoids the `archon/providers` dependency chain in tests.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 Learnings used

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to packages/git/src/**/*.{ts,tsx} : `archon/git` handles git operations - worktrees, branches, repos, exec wrappers; depends only on archon/paths; use `execFileAsync` for direct git calls (not `exec`)

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to packages/cli/src/**/*.{ts,tsx} : CLI isolation defaults: auto-creates worktree with generated branch name (isolation by default); `--branch` flag for explicit branch name; `--no-worktree` opts out of isolation (runs in live checkout)

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to packages/paths/src/**/*.{ts,tsx} : `archon/paths` provides path resolution utilities, Pino logger factory, web dist cache path (`getWebDistDir`), and CWD env stripper (`stripCwdEnv`, `strip-cwd-env-boot`); has no archon/* deps; only `pino` and `dotenv` allowed as external deps

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to **/*.{ts,tsx} : Use `archon/git` functions for git operations; use `execFileAsync` (not `exec`) when calling git directly

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to packages/workflows/src/defaults/**/*.{ts,tsx,yaml} : Bundled defaults are in `.archon/commands/defaults/` and `.archon/workflows/defaults/`; binary builds embed at compile time via `packages/workflows/src/defaults/bundled-defaults.generated.ts`; source builds load from filesystem; merged with repo-specific (repo overrides defaults by name)

---

Learnt from: truck0321
Repo: coleam00/Archon PR: 1104
File: .archon/scripts/forge-cli.ts:15-16
Timestamp: 2026-04-15T12:56:38.364Z
Learning: `.archon/scripts/forge-cli.ts` is an intentionally standalone Bun script with zero dependencies by design. It runs in workflow bash nodes via `bun` execution outside the monorepo package graph, so importing `archon/git` or any other monorepo package would require bundling and is explicitly not desired. Do not suggest importing monorepo packages into this script.

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to **/*.{ts,tsx} : Import workflow engine types/functions from direct subpaths, not main package export: `from 'archon/workflows/deps'`, `from 'archon/workflows/store'`, `from 'archon/workflows/executor'`, etc.

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to packages/workflows/src/**/*.{ts,tsx} : `archon/workflows` is the workflow engine - loader, router, executor, DAG, logger, bundled defaults; depends only on archon/git + archon/paths + archon/providers/types + hono/zod-openapi + zod; DB/AI/config injected via `WorkflowDeps`

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to **/*.{ts,tsx} : Use `import * as conversationDb from 'archon/core/db/conversations'` and `import * as git from 'archon/git'` for namespace imports from well-organized submodules

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to **/*.{ts,tsx} : Import workflow engine types/functions from direct subpaths (e.g., `from 'archon/workflows/deps'`, `from 'archon/workflows/store'`, `from 'archon/workflows/executor'`) rather than the main package export

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to packages/workflows/src/workflow-discovery.ts : Migration from pre-0.x `~/.archon/.archon/workflows/`: Archon emits a one-time WARN with exact `mv` command and does NOT load from old location. Move with: `mv ~/.archon/.archon/workflows ~/.archon/workflows && rmdir ~/.archon/.archon`

---

Learnt from: Wirasm
Repo: coleam00/Archon PR: 514
File: archon-ui-main/src/services/crawlProgressService.ts:35-39
Timestamp: 2025-08-28T13:07:24.810Z
Learning: The crawlProgressService.ts in the Archon codebase should be deprecated in favor of the existing useCrawlProgressPolling hook from usePolling.ts, which already includes ETag support, 304 handling, tab visibility detection, and proper React lifecycle integration. This consolidation reduces code duplication and improves performance.

---

Learnt from: CR
Repo: coleam00/Archon PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-20T18:47:37.611Z
Learning: Applies to packages/workflows/src/**/*.{ts,tsx} : Provider is inherited from `.archon/config.yaml` unless explicitly set; per-node `provider` and `model` overrides are supported; model validation ensures provider/model compatibility at load time

[Wirasm]

Hi @kagura-agent — thanks for opening this PR.

This repository uses a PR template at .github/pull_request_template.md with several required sections. A few of them appear to be empty or placeholder here:

• Side Effects / Blast Radius
• Rollback Plan

Could you fill those out (even briefly)? The template helps reviewers understand scope, risk, and rollback — it speeds up review significantly.

If a section genuinely doesn't apply, just write "N/A" in it rather than leaving it blank.

[kagura-agent]

Thanks @Wirasm — added the Side Effects / Blast Radius and Rollback Plan sections!

[Wirasm]

Review Summary

Verdict: minor-fixes-needed

Your fix is tight and well-scoped — resolveMainBranch is a clean addition, and the three call sites are updated consistently. The type safety and test structure are solid. A few items need attention before merge, but nothing blocking.

Suggested fixes

• cleanup-service.ts:34-52 — The catch block is completely silent. loadRepoConfig() in config-loader.ts handles the same scenario (missing file, parse error) and logs before falling through — your new function should do the same. At minimum add getLog().debug(...) for expected failures. Better yet, prefer Option A — delegate to loadRepoConfig() directly:

  async function resolveMainBranch(repoPath: RepoPath): Promise<BranchName> {
    const repoConfig = await loadRepoConfig(repoPath);
    if (repoConfig.worktree?.baseBranch?.trim()) {
      return toBranchName(repoConfig.worktree.baseBranch.trim());
    }
    return getDefaultBranch(repoPath);
  }

  This eliminates the duplicate YAML parsing and gets proper error handling for free.

• cleanup-service.ts:37-55 — Add test cases for the error fallback paths that aren't currently exercised:

  // Invalid YAML syntax in config
  test('falls back on YAML parse error', async () => {
    await writeFile(configPath, 'worktree:\n  baseBranch: [invalid');
    await cleanupMergedWorktrees(repoPath, mockLog, mockExecutor);
    expect(mockGetDefaultBranch).toHaveBeenCalledWith(repoPath);
  });
  
  // baseBranch is a non-string type
  test('falls back when baseBranch is not a string', async () => {
    await writeFile(configPath, 'worktree:\n  baseBranch: !!null null');
    await cleanupMergedWorktrees(repoPath, mockLog, mockExecutor);
    expect(mockGetDefaultBranch).toHaveBeenCalledWith(repoPath);
  });

• cleanup-service.ts:329 & :483 (runScheduledCleanup, getWorktreeStatusBreakdown) — These two call sites for resolveMainBranch aren't directly tested. They're covered transitively, but adding an assertion that mockGetDefaultBranch is NOT called (or IS called with correct args) in these contexts would catch silent regressions.

Minor / nice-to-have

• cleanup-service.ts:37-40 — The JSDoc on resolveMainBranch restates what the function name already says. Consider removing it — the function body makes the fallback order clear without it.

Compliments

• The catch block comment at line 52-53 is exactly right — it captures the why of silent fallback (// Config load failure is non-fatal) without restating what the code does. This is the standard for Archon comments.
• The test suite's approach of using a real temp directory with a real YAML file is the correct choice for config-loading tests, and the try/finally cleanup is properly handled.

---

Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality.

[kagura-agent]

Thanks for the review @Wirasm! Just a quick note — the docker-entrypoint.sh comment style suggestion doesn't apply to this PR, as it only touches TypeScript files (cleanup-service.ts and cleanup-service.test.ts) where // is the standard comment syntax. No other blocking or suggested changes, so this should be ready to go. Let me know if I missed anything!

[kagura-agent]

Thanks for the detailed review! Addressed all items:

• Added getLog().debug(...) logging to the resolveMainBranch catch block for visibility on config load failures
• Added tests for YAML parse error fallback and non-string baseBranch fallback (number type triggers TypeError on .trim(), null falls through)
• Added assertions verifying resolveMainBranch is called correctly in both runScheduledCleanup and getWorktreeStatusBreakdown call sites

Re: Option A (delegating to loadRepoConfig) — I considered this but config-loader.ts imports from @archon/providers at the top level, which causes module resolution failures in cleanup-service.test.ts. The inline Bun.YAML.parse approach avoids this coupling while still providing the same fallback behavior. Happy to discuss if you see a better path!

[coderabbitai]

🧹 Nitpick comments (1)

packages/core/src/services/cleanup-service.test.ts (1)

1692-1708: Make the “no config exists” fallback test deterministic.

Using '/nonexistent/repo' (Line 1697 / Line 1703) is environment-dependent. Prefer a fresh temp dir with no .archon/config.yaml to avoid host-specific false failures.

♻️ Proposed stabilization

   test('falls back to getDefaultBranch when no config exists', async () => {
-    mockListByCodebase.mockResolvedValueOnce([
-      {
-        id: 'env-fallback',
-        branch_name: 'feature-y',
-        working_path: '/nonexistent/repo/worktrees/feature-y',
-        status: 'active',
-      },
-    ]);
-    mockGetDefaultBranch.mockResolvedValueOnce('main');
-
-    await cleanupMergedWorktrees('codebase-1', '/nonexistent/repo');
-
-    expect(mockGetDefaultBranch).toHaveBeenCalledTimes(1);
-    expect(mockGetDefaultBranch).toHaveBeenCalledWith('/nonexistent/repo');
-    expect(mockIsBranchMerged.mock.calls[0][2]).toBe('main');
+    const { mkdtemp, rm } = await import('fs/promises');
+    const { join } = await import('path');
+    const { tmpdir } = await import('os');
+    const tempDir = await mkdtemp(join(tmpdir(), 'archon-cleanup-test-'));
+    try {
+      mockListByCodebase.mockResolvedValueOnce([
+        {
+          id: 'env-fallback',
+          branch_name: 'feature-y',
+          working_path: join(tempDir, 'worktrees', 'feature-y'),
+          status: 'active',
+        },
+      ]);
+      mockGetDefaultBranch.mockResolvedValueOnce('main');
+
+      await cleanupMergedWorktrees('codebase-1', tempDir);
+
+      expect(mockGetDefaultBranch).toHaveBeenCalledTimes(1);
+      expect(mockGetDefaultBranch).toHaveBeenCalledWith(tempDir);
+      expect(mockIsBranchMerged.mock.calls[0][2]).toBe('main');
+    } finally {
+      await rm(tempDir, { recursive: true, force: true });
+    }
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/services/cleanup-service.test.ts` around lines 1692 - 1708,
The test uses a host-dependent path '/nonexistent/repo'; make it deterministic
by creating an isolated temporary directory with no .archon/config.yaml and pass
that dir to cleanupMergedWorktrees instead. Replace the hardcoded
'/nonexistent/repo' in this test with a freshly-created temp dir (e.g., using
fs.mkdtemp / os.tmpdir) and ensure the temp dir is empty (or explicitly remove
any .archon/config.yaml) before calling cleanupMergedWorktrees('codebase-1',
tempDir), then clean up the temp dir after the test; keep existing assertions
for mockGetDefaultBranch and mockIsBranchMerged.

🤖 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.test.ts`:
- Around line 1692-1708: The test uses a host-dependent path
'/nonexistent/repo'; make it deterministic by creating an isolated temporary
directory with no .archon/config.yaml and pass that dir to
cleanupMergedWorktrees instead. Replace the hardcoded '/nonexistent/repo' in
this test with a freshly-created temp dir (e.g., using fs.mkdtemp / os.tmpdir)
and ensure the temp dir is empty (or explicitly remove any .archon/config.yaml)
before calling cleanupMergedWorktrees('codebase-1', tempDir), then clean up the
temp dir after the test; keep existing assertions for mockGetDefaultBranch and
mockIsBranchMerged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 183fe10d-fdbe-4f79-8287-4363eba2dad1

📥 Commits

Reviewing files that changed from the base of the PR and between c5e41ba and f456031.

📒 Files selected for processing (2)

• packages/core/src/services/cleanup-service.test.ts
• packages/core/src/services/cleanup-service.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/core/src/services/cleanup-service.ts