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 @coderabbitai help to get the list of available commands and usage tips.}

Comprehensive PR Review

PR: #1860 — Fix: stop hard-resetting source/ on every chat tick
Reviewed by: 4 specialized agents (code-review, error-handling, test-coverage, docs-impact)
Date: 2026-06-04

Summary

The core fix is solid. The mode: 'fast-forward' | 'reset' API is clean and unambiguous, the 5-state fast-forward logic is correct, and post-message-reminder.ts is well-isolated and non-fatal by construction. The main blockers are two missing test surfaces: the entirely new post-message-reminder.ts module has zero dedicated tests, and the two new exported git helpers (getCurrentBranch, countCommitsAhead) lack their own describe blocks despite every other function in branch.ts having them.

Verdict: REQUEST_CHANGES

🔴 HIGH Issues (Should fix before merge)

HIGH-1: post-message-reminder.ts — entirely new module with zero dedicated tests

📍 packages/core/src/orchestrator/post-message-reminder.ts

The orchestrator test mocks @archon/git so getCurrentBranch returns null, making the reminder exit early at line 40 — the only path exercised. None of these branches are tested:

• Path guard: skipping non-managed repos (cwdNormalized.startsWith(archonWorkspacesPath))
• !platform.sendStructuredEvent guard
• ahead > 0 && !dirty → emit unpushed-commit count
• ahead === 0 && dirty → emit uncommitted-changes message
• Both true → composite message
• SSE event type 'system' and content format
• catch block that logs at warn and swallows

The core invariant — "fires when ahead > 0 or dirty, silent otherwise" — is never asserted. If the Windows path-separator guard is wrong, the reminder either fires for all repos or never fires for managed repos, silently.

// packages/core/src/orchestrator/post-message-reminder.test.ts
import { mock, describe, test, expect, beforeEach } from 'bun:test';

const mockGetCurrentBranch = mock(() => Promise.resolve(null));
const mockCountCommitsAhead = mock(() => Promise.resolve(0));
const mockHasUncommittedChanges = mock(() => Promise.resolve(false));

mock.module('@archon/git', () => ({
  getCurrentBranch: mockGetCurrentBranch,
  countCommitsAhead: mockCountCommitsAhead,
  hasUncommittedChanges: mockHasUncommittedChanges,
  toRepoPath: (p: string) => p,
}));

mock.module('@archon/paths', () => ({
  getArchonWorkspacesPath: () => '/home/test/.archon/workspaces',
  createLogger: () => ({ warn: mock(() => undefined), error: mock(() => undefined) }),
}));

import { reportUnpushedWorkInSource } from './post-message-reminder';

// Tests: no sendStructuredEvent, non-managed path, detached HEAD (null branch),
// clean+in-sync (no emit), ahead > 0 (emit with count), dirty (emit), error swallowed

HIGH-2: getCurrentBranch + countCommitsAhead — exported functions with no unit tests

📍 packages/git/src/branch.ts:321-360, packages/git/src/git.test.ts

Every other function in branch.ts has its own describe block (hasUncommittedChanges, getDefaultBranch, commitAllChanges, isBranchMerged, isPatchEquivalent, isAncestorOf, getLastCommitDate). These two are the only exceptions despite being exported public API.

The fail-silent contracts (return null/0 on any error) are critical to the fast-forward guard and the reminder. If a future refactor removes the catch, syncWorkspaceFastForward would throw on detached-HEAD repos, breaking the non-destructive sync guarantee silently.

describe('getCurrentBranch', () => {
  let execSpy: Mock<typeof git.execFileAsync>;
  beforeEach(() => { execSpy = spyOn(git, 'execFileAsync'); });
  afterEach(() => { execSpy.mockRestore(); });

  test('returns branch name on clean symbolic-ref output', async () => {
    execSpy.mockResolvedValue({ stdout: 'main\n', stderr: '' });
    expect(await git.getCurrentBranch('/workspace/repo')).toBe('main');
  });
  test('returns null for empty stdout (detached HEAD)', async () => {
    execSpy.mockResolvedValue({ stdout: '', stderr: '' });
    expect(await git.getCurrentBranch('/workspace/repo')).toBeNull();
  });
  test('returns null on any git error (ENOENT, not a repo)', async () => {
    execSpy.mockRejectedValue(new Error('fatal: not a git repository'));
    expect(await git.getCurrentBranch('/workspace/repo')).toBeNull();
  });
});

describe('countCommitsAhead', () => {
  let execSpy: Mock<typeof git.execFileAsync>;
  beforeEach(() => { execSpy = spyOn(git, 'execFileAsync'); });
  afterEach(() => { execSpy.mockRestore(); });

  test('returns parsed integer', async () => {
    execSpy.mockResolvedValue({ stdout: '3\n', stderr: '' });
    expect(await git.countCommitsAhead('/workspace/repo', 'main')).toBe(3);
  });
  test('returns 0 for NaN output (malformed git output)', async () => {
    execSpy.mockResolvedValue({ stdout: 'not-a-number\n', stderr: '' });
    expect(await git.countCommitsAhead('/workspace/repo', 'main')).toBe(0);
  });
  test('returns 0 on any error', async () => {
    execSpy.mockRejectedValue(new Error("fatal: unknown revision 'origin/main..HEAD'"));
    expect(await git.countCommitsAhead('/workspace/repo', 'main')).toBe(0);
  });
});

🟡 MEDIUM Issues (Needs decision)

MEDIUM-1: Codebase fetched twice per chat tick — redundant DB round-trip

📍 packages/core/src/orchestrator/orchestrator-agent.ts:1140-1152

handleMessage fetches the codebase inside discoverAllWorkflows (line 604), then fetches it again at line 1140 to pass to reportUnpushedWorkInSource. Two DB round-trips per non-slash-command tick. On Postgres this is a real network round-trip on every message.

DiscoverResult already carries syncResult, syncError, and config computed inside discoverAllWorkflows. Adding codebase?: Codebase | null is consistent with this pattern and eliminates the second fetch cleanly.

Options: Fix now (LOW effort — small interface extension) | Create issue | Skip (negligible on SQLite)

MEDIUM-2: clone.ts backfill guard untested — wrong condition could silently overwrite configured branch

📍 packages/core/src/handlers/clone.ts:175-179

if (!existing.default_branch && detectedBranch) {
  updates.default_branch = detectedBranch;
}

This guard is critical. If the condition were wrong (e.g., || instead of &&), a re-register would silently overwrite a manually configured default_branch, causing every chat tick to sync to the wrong branch. clone.ts has no test file.

Options: Fix now (MED effort — new clone.test.ts) | Create issue (acceptable — condition is simple and reviewable manually)

🟢 LOW Issues

✅ What's Good

• mode: 'fast-forward' | 'reset' API is clean and unambiguous. Named modes make intent explicit at every call site; the safe ?? 'fast-forward' default is the right conservative choice.
• All 5 fast-forward states are covered. The regression guard test ("chat tick never passes mode: 'reset'") is exactly the right contract test for the original bug git reset --hard origin/<default_branch> on source/ every message — destroys any local state that has diverged from origin for managed clones #1516.
• Error paths for the sync refactor are sound. Fetch errors are non-fatal; syncError captures the message for the AI's context. Double-boundary pattern (reportUnpushedWorkInSource has its own try/catch + orchestrator outer catch) is correctly defensive.
• Migration sequencing is correct. 023_add_default_branch_to_codebases.sql uses IF NOT EXISTS, is present in 000_combined.sql, and numbering is sequential.
• worktree.ts correctly distinguishes managed vs. non-managed clones. mode: 'reset' for managed clones, mode: 'fast-forward' for local repos — logic is clear and tested.
• detectDefaultBranch error contract is clean. Returns null for all error cases; callers store NULL; NULL in DB falls back to runtime detection. Safe at every layer.

Suggested Follow-up Issues

Reviewed by Archon comprehensive-pr-review workflow
Full artifacts: /Users/rasmus/.archon/workspaces/coleam00/Archon/artifacts/runs/29af962b000d1be0108709498fcf5f30/review/

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-bench-1516-claude
Commit: c04f6f1a
Philosophy: Fix everything unless clearly a new concern

Fixes Applied (10 total)

Tests Added

• packages/core/src/orchestrator/post-message-reminder.test.ts (new, 9 tests)
• packages/git/src/git.test.ts: getCurrentBranch (5 tests) + countCommitsAhead (4 tests)
• packages/core/src/db/codebases.test.ts: 2 tests for updateCodebase with default_branch

Skipped (1)

Suggested Follow-up Issues

1. "Add tests for clone.ts default_branch detection and backfill logic" — The backfill guard !existing.default_branch && detectedBranch is critical; a wrong condition would silently overwrite user-configured branches on re-register

Validation

✅ Type check | ✅ Lint (0 warnings) | ✅ Format | ✅ Tests (all packages pass)

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-bench-1516-claude