fix(git): replace branch-per-iteration with commit-per-iteration (v0.8.1)

[dean0x]

Summary

v0.8.0 shipped with a broken branch-per-iteration git strategy. This patch replaces it with a single-branch commit-per-iteration design that's simpler, safer, and more efficient.

Design Changes

• One branch for entire loop: All iterations use the same feature branch (e.g., task-123-optimization)
• One commit per successful iteration: Each completed iteration creates a single atomic commit
• Full revert on failure: Failed or discarded iterations are fully reverted (clean working directory)
• No orphaned branches: Eliminates intermediate branches and cleanup complexity

Implementation Details

• New git utilities for branch/commit operations with built-in rollback
• Domain model renames for clarity (LoopIterationError → LoopIterationFailed)
• Migration 12: database schema updates for commit-per-iteration tracking
• Loop handler rewrite: simplified iteration logic, atomic transaction boundaries
• Comprehensive test coverage: unit, integration, and error scenarios

Testing

• All existing tests updated for new git strategy
• New tests for commit creation, revert, and error handling
• Integration tests verify multi-iteration flows with rollback
• Manual testing on macOS and Linux

Related Issues

Fixes: v0.8.0 git integration regression

[dean0x]

🔴 Blocking: Duplicated git commit logic in handleRetryResult vs recordAndContinue

The retry pass path in handleRetryResult duplicates the git commit/diff capture logic that also exists in recordAndContinue. Both paths:

1. Call commitAndCaptureDiff
2. Handle errors identically
3. Produce the same gitCommitSha and gitDiffSummary variables

The only reason handleRetryResult has its own copy is the atomic transaction for iteration pass + loop completion, but this creates a maintenance burden. If commit logic changes (e.g., adding verification), both places must be updated in lockstep.

Suggested fix: Extract the git pre-persist step into a shared helper, or refactor handleRetryResult to reuse recordAndContinue with a post-action callback for loop completion. At minimum, add a comment cross-referencing:

// NOTE: This git logic is duplicated in recordAndContinue() — keep both in sync

Flagged by: consistency, complexity, architecture reviewers (82-85% confidence)

[dean0x]

⚠️ Should-Fix: startNextIteration has 13 branches and cyclomatic complexity ~14

This method grew from ~30 lines to 96 lines with git integration. It now handles:

• atomic iteration increment
• git branch creation on first iteration
• git branch re-checkout on subsequent iterations
• pre-iteration SHA capture
• dispatch to single-task vs pipeline iteration

The result: 13 decision points with max nesting depth 4 levels.

Suggested fix: Extract the git setup block (lines 510-562) into a private helper method like setupGitForIteration(loop, iterationNumber): Promise<string | undefined>. This would reduce startNextIteration to ~50 lines and bring branching down to ~5.

private async setupGitForIteration(
  loop: Loop,
  iterationNumber: number,
): Promise<string | undefined> {
  if (iterationNumber === 1 && loop.gitBranch) {
    const branchResult = await createAndCheckoutBranch(
      loop.workingDirectory, loop.gitBranch, loop.gitBaseBranch,
    );
    if (!branchResult.ok) {
      this.logger.warn('Failed to create git branch for loop, continuing without git', { ... });
    }
  } else if (iterationNumber > 1 && loop.gitBranch) {
    const checkoutResult = await createAndCheckoutBranch(loop.workingDirectory, loop.gitBranch);
    if (!checkoutResult.ok) {
      this.logger.warn('Failed to re-checkout loop branch, continuing without git', { ... });
    }
  }

  const shaResult = await getCurrentCommitSha(loop.workingDirectory);
  if (shaResult.ok) return shaResult.value;
  this.logger.warn('Failed to capture pre-iteration commit SHA', { ... });
  return undefined;
}

Flagged by: complexity reviewer (90% confidence)

[dean0x]

⚠️ Should-Fix: recordAndContinue nesting depth reaches 6 levels

The git revert path (lines 1098-1112) has excessive nesting depth. The deepest path is:
method body → if preIterationCommitSha → try → else (discard path) → if resetTarget → if !resetResult.ok

Per complexity-patterns, nesting depth >4 is critical-warning territory.

Suggested fix: Extract the revert path into a separate helper method to flatten nesting:

// In recordAndContinue, replace the else-block with:
} else {
  await this.revertIterationChanges(loop, iteration);
}

private async revertIterationChanges(loop: Loop, iteration: LoopIteration): Promise<void> {
  const resetTarget = await this.getResetTargetSha(loop);
  if (!resetTarget) return;

  const resetResult = await resetToCommit(loop.workingDirectory, resetTarget);
  if (!resetResult.ok) {
    this.logger.warn('Failed to reset to commit after iteration failure', {
      loopId: loop.id,
      iterationNumber: iteration.iterationNumber,
      resetTarget,
      error: resetResult.error.message,
    });
  }
}

Flagged by: complexity reviewer (88% confidence)

[dean0x]

⚠️ Should-Fix: Weak assertion in subsequent-iteration branch checkout test

The test wraps its core assertion in if (calls.length > 0), which makes the test vacuously true if createAndCheckoutBranch is never called for iteration 2 (a regression).

The test name says "should NOT call createAndCheckoutBranch" but the actual behavior (per the comment) is that it SHOULD call it with different args (no baseBranch). The conditional assertion means if the call is missing, the test silently passes without catching the regression.

Suggested fix: Rename the test to accurately describe expected behavior and replace the conditional with definitive assertions:

it('should re-checkout loop branch without baseBranch for subsequent iterations', async () => {
  // ... setup ...
  vi.mocked(createAndCheckoutBranch).mockClear();
  const taskId1 = await getLatestTaskId(loop.id);
  await eventBus.emit('TaskCompleted', { taskId: taskId1!, exitCode: 0, duration: 100 });
  await flushEventLoop();

  const calls = vi.mocked(createAndCheckoutBranch).mock.calls;
  expect(calls).toHaveLength(1); // Exactly one re-checkout call
  expect(calls[0][2]).toBeUndefined(); // No fromRef for re-checkout
});

Flagged by: tests reviewer (90% confidence)

[dean0x]

⚠️ Should-Fix: Duplicated git state capture in loop-manager and schedule-handler

Both files contain nearly identical git state capture logic:

1. Call captureGitState
2. Conditionally extract gitBaseBranch
3. Call getCurrentCommitSha
4. Conditionally extract gitStartCommitSha
5. Spread-merge into loop update

This is a DRY violation. If the git capture strategy evolves (e.g., adding a new field, changing capture order), both call sites must be updated in lockstep.

Suggested fix: Extract a shared utility function:

// in src/utils/git-state.ts
export async function captureGitContextForLoop(
  workingDirectory: string,
  gitBranch?: string,
): Promise<{ gitBaseBranch?: string; gitStartCommitSha?: string }> {
  const gitStateResult = await captureGitState(workingDirectory);
  if (!gitStateResult.ok || !gitStateResult.value) return {};

  const gitBaseBranch = gitBranch ? gitStateResult.value.branch : undefined;
  const shaResult = await getCurrentCommitSha(workingDirectory);
  const gitStartCommitSha = shaResult.ok ? shaResult.value : undefined;

  return { gitBaseBranch, gitStartCommitSha };
}

Then both callers reduce to:

const gitCtx = await captureGitContextForLoop(dir, request.gitBranch);
const loopWithGit = (gitCtx.gitBaseBranch || gitCtx.gitStartCommitSha) 
  ? updateLoop(loop, gitCtx) 
  : loop;

Flagged by: architecture, complexity, performance reviewers (82-85% confidence)

[greptile-apps]

Confidence Score: 3/5

• One concrete data-loss risk needs resolution before merge: git reset+clean now runs for all loops in git repos, not just --git-branch loops.
• The implementation is clean and well-tested. The P1 concern is that git commit/reset operations now activate unconditionally for any loop in a git repo (via the always-set gitStartCommitSha), silently running git clean -fd on failed iterations even when the user didn't opt in with --git-branch. This is a real data-loss vector that doesn't match the --git-branch opt-in framing. Additionally, the missing release notes file will block the release workflow per CLAUDE.md requirements.
• src/services/loop-manager.ts and src/services/handlers/loop-handler.ts — the isGitLoop/gitStartCommitSha interaction that triggers destructive git ops without --git-branch.

Important Files Changed

Filename	Overview
src/services/handlers/loop-handler.ts	Core iteration engine rewritten for commit-per-iteration. New git operations (commitAllChanges, resetToCommit) now trigger for ALL loops in git repos via isGitLoop — not just those with --git-branch — due to gitStartCommitSha being unconditionally populated. This is the primary risk vector for this PR.
src/services/loop-manager.ts	Now captures gitStartCommitSha for every loop in a git repo, even without --git-branch. This is the source of the unintended opt-in behavior that makes git commit/reset run on all git-repo loops.
src/utils/git-state.ts	Three new utility functions added: getCurrentCommitSha, commitAllChanges (git add -A + commit), and resetToCommit (git reset --hard + git clean -fd). SHA validation in isValidCommitSha is sound. captureGitDiff uses validateGitRefName for SHA inputs — works but semantically inconsistent.
src/implementations/database.ts	Migration 12 adds git_start_commit_sha to loops and git_commit_sha / pre_iteration_commit_sha to loop_iterations. Old columns retained (SQLite DROP COLUMN limitation) — safe additive migration.
src/implementations/loop-repository.ts	Schema, Zod validators, prepared statements, and row-to-domain mappers all updated consistently for the three new git columns. No issues found.
src/services/handlers/schedule-handler.ts	Mirrors loop-manager's gitStartCommitSha capture for schedule-triggered loops. Same concern about unconditional capture, but secondary to the loop-manager path.
src/core/domain.ts	Domain types updated cleanly — gitStartCommitSha on Loop, gitCommitSha and preIterationCommitSha on LoopIteration. Old fields correctly annotated as dead. createLoop initializes gitStartCommitSha to undefined.
tests/unit/utils/git-state.test.ts	Comprehensive tests for all three new utilities, including happy paths, nothing-to-commit case, and injection-attempt rejections for resetToCommit. Well structured.
tests/unit/services/handlers/loop-handler.test.ts	New git integration tests cover branch creation, pre-iteration SHA capture, commit on pass/keep, reset on fail/discard, and agent-already-committed fallback. Tests explicitly verify unconditional gitStartCommitSha capture, confirming intentional design.

Sequence Diagram

sequenceDiagram
    participant U as User/Agent
    participant LM as LoopManager
    participant LH as LoopHandler
    participant Git as git-state utils
    participant DB as SQLiteLoopRepository

    U->>LM: createLoop(request)
    LM->>Git: captureGitState(workingDir)
    Git-->>LM: { branch, commitSha }
    LM->>Git: getCurrentCommitSha(workingDir)
    Git-->>LM: startCommitSha
    LM->>DB: emit LoopCreated (with gitStartCommitSha)

    loop Each iteration
        LH->>LH: isGitLoop = !!(gitBranch ∥ gitStartCommitSha)
        opt iterationNumber === 1 AND gitBranch set
            LH->>Git: createAndCheckoutBranch(dir, branch, baseBranch)
        end
        LH->>Git: getCurrentCommitSha(workingDir)
        Git-->>LH: preIterationCommitSha
        LH->>DB: recordIteration(preIterationCommitSha)

        U->>LH: TaskCompleted / TaskFailed

        alt eval passes (retry=pass, optimize=keep)
            LH->>Git: commitAllChanges(dir, message)
            Git-->>LH: gitCommitSha (or null if agent already committed)
            opt gitCommitSha is null
                LH->>Git: getCurrentCommitSha(workingDir)
                Git-->>LH: gitCommitSha (fallback)
            end
            LH->>Git: captureGitDiff(dir, preIterationSHA, gitCommitSha)
            Git-->>LH: diffSummary
            LH->>DB: updateIteration(gitCommitSha, gitDiffSummary, status=pass/keep)
        else eval fails (retry=fail, optimize=discard, crash)
            LH->>LH: getResetTargetSha(loop)
            note over LH: OPTIMIZE: bestIteration.gitCommitSha<br/>RETRY / fallback: loop.gitStartCommitSha
            LH->>Git: resetToCommit(dir, targetSha)
            note over Git: git reset --hard sha<br/>git clean -fd
            LH->>DB: updateIteration(status=fail/discard/crash)
        end
    end

Loading

_{Reviews (1): Last reviewed commit: "test: add git integration test coverage ..." | Re-trigger Greptile}

[greptile-apps]

Git operations now activate for all loops in git repos (not just --git-branch)

gitStartCommitSha is now captured unconditionally whenever the working directory is a git repository, regardless of whether --git-branch was specified:

// Always capture start commit SHA in git repos
const shaResult = await getCurrentCommitSha(validatedWorkingDirectory);
if (shaResult.ok) {
  gitStartCommitSha = shaResult.value;
}

Because isGitLoop = !!(updatedLoop.gitBranch || updatedLoop.gitStartCommitSha) in loop-handler.ts, and because gitStartCommitSha is now always populated in git repos, every loop running inside a git repository will now:

1. Run git add -A && git commit on every pass/keep iteration
2. Run git reset --hard <sha> && git clean -fd on every fail/discard/crash

The git clean -fd call in particular permanently destroys untracked (but non-ignored) files that the user created during the iteration — with no warning, and even when the user did not opt into git integration via --git-branch.

This conflicts with the CLI/docs framing of --git-branch as an opt-in flag. A user running beat loop create "refactor X" --exit-condition ./check.sh in a git repo with untracked WIP files will silently lose them on the first failed iteration.

If the intent is to always track git state in git repos, this should be explicitly documented and communicated as an opt-out (or the destructive revert path should be gated on updatedLoop.gitBranch explicitly). Consider guarding the commit/reset behavior with a check like:

const isCommitLoop = !!updatedLoop.gitBranch; // only when user opts in

and keeping gitStartCommitSha purely for informational SHA recording.

[greptile-apps]

Missing release notes file (CLAUDE.md release process)

CLAUDE.md specifies that creating a release notes file is REQUIRED before any release:

Create release notes (REQUIRED): Must match version in package.json

The package version is bumped to 0.8.1, but there is no corresponding docs/releases/RELEASE_NOTES_v0.8.1.md file in this PR (only RELEASE_NOTES_v0.8.0.md exists). The release workflow validates that this file exists before publishing to npm, so the release will fail without it.

[greptile-apps]

captureGitDiff uses branch-name validator for commit SHA inputs

captureGitDiff now accepts commit SHAs as fromRef/toRef (called from commitAndCaptureDiff with values like iteration.preIterationCommitSha), but it validates them with validateGitRefName, which is designed for branch names.

Hex SHAs happen to pass this validator (they contain no spaces, .., @{, etc.), so there is no functional bug today. However, isValidCommitSha is already defined in the same file and is the purpose-built validator for this input type. Using the wrong validator is a semantic inconsistency that could silently permit inputs that are valid branch names but not valid SHAs (e.g. my-feature), and it'll be a maintenance pitfall if SHA-specific checks are ever added.

Consider accepting a union type or using isValidCommitSha (if exported) for the ref validation path when the caller provides commit SHAs.

[dean0x]

Summary of Code Review Findings

PR #118: Commit-per-iteration git integration for Backbeat v0.8.1

Overall Assessment

10 reviewers analyzed this PR across architecture, complexity, consistency, database, dependencies, documentation, performance, regression, security, and test quality. The commit-per-iteration design is sound. 4 blocking issues (80%+ confidence) were found, all with straightforward fixes outlined in the inline comments above.

---

Inline Comments Created

Four critical-path comments were created on changed code:

1. Duplicated git commit logic in handleRetryResult vs recordAndContinue (line 766)
2. startNextIteration complexity — 13 branches, extract git setup helper (line 510)
3. recordAndContinue nesting — 6 levels deep, extract revert path (line 1100)
4. Weak test assertion — vacuously-true conditional check (line 1302)
5. Duplicated git state capture in loop-manager and schedule-handler (line 232)

---

Lower-Confidence Suggestions (60-79%)

These don't block merge but warrant discussion:

Performance & Design (60-75% confidence)

• Redundant git rev-parse calls: captureGitState() already gets HEAD SHA; getCurrentCommitSha() duplicates it. Reuse the existing SHA.
• Sequential git spawns: 4-6 spawns per operation is inherent to git model (must stage before commit before diff). Timeout protection in place (30s per op).

Test Coverage Gaps (60-85% confidence)

• Missing git commit failure test: No test for pre-commit hook rejection. Add test case.
• Missing 40-char SHA boundary test: Add test for exact upper limit alongside 7-char short SHA test.
• Task failure test assertion: Test name says "should NOT call createAndCheckoutBranch" but actually it SHOULD (without baseBranch). Clarify and add negative assertion.

Documentation Issues (80-95% confidence)

• CLI output regression for v0.8.0 loops: Existing loops have gitBranch set but gitCommitSha is NULL. Add fallback logic.
• v0.8.0 release notes: Subtitle (line 3) still says "per-iteration git branch isolation" but body says "commit-per-iteration". Update subtitle.
• Missing v0.8.1 release notes file: Create docs/releases/RELEASE_NOTES_v0.8.1.md (required by release workflow).
• Loop handler file header: Version pin "v0.7.0" is stale; remove or update to v0.8.1.

---

Pre-Existing Issues (Not Blocking)

1. Security: Add branch verification to resetToCommit() before destructive ops
2. Architecture: loop-handler.ts is 1,612 lines (3x threshold); split into modules post-merge
3. Database: Legacy gitBranch field still written (always NULL for v0.8.1+); add comment
4. Migrations: No rollback strategy (pre-existing pattern); safe but worth noting

---

Reviewer Scores

Reviewer	Score	Recommendation
Architecture	7/10	APPROVED_WITH_CONDITIONS
Complexity	6/10	APPROVED_WITH_CONDITIONS
Consistency	7/10	CHANGES_REQUESTED
Database	9/10	APPROVED
Dependencies	10/10	APPROVED
Documentation	7/10	CHANGES_REQUESTED
Performance	7/10	APPROVED_WITH_CONDITIONS
Regression	6/10	CHANGES_REQUESTED
Security	8/10	APPROVED_WITH_CONDITIONS
Tests	8/10	CHANGES_REQUESTED

---

To Merge

Address the 4 blocking inline comments (code changes required):

1. ✅ Extract duplicated git logic in handleRetryResult (reuse recordAndContinue helper or cross-reference)
2. ✅ Extract git setup from startNextIteration (reduce 13 branches to 5)
3. ✅ Extract revert path from recordAndContinue (reduce nesting from 6 to 4)
4. ✅ Fix test assertion in subsequent-iteration checkout test (replace conditional with definitive assertion)

Recommended before merge:

• Fix documentation issues (release notes, CLI fallbacks)
• Add missing test case for git commit failure
• Consider performance optimization (reuse rev-parse SHA)

---

Git Code Review Footer: This review was generated by Claude Code analyzing 10 reviewer reports across architecture, complexity, consistency, database, dependencies, documentation, performance, regression, security, and testing dimensions. Confidence: inline comments 80%+, summary 60%+.