chore: resolve merge conflicts for DiffCache test task

Resolves add/add conflict in diff-cache.ts (kept typed PhaseMetaResponse/
FileDiffResponse interfaces from HEAD over unknown-typed singletons from test
branch) and content conflict in phase.ts (kept both phaseMetaCache and
fileDiffCache imports; removed auto-merged duplicate firstClone/headHash/
cacheKey/cached declarations and unreachable empty-projects guard).

Also cleans auto-merged duplicate getHeadCommitHash in orchestrator.test.ts
and simple-git-branch-manager.ts.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

apps/server/git/simple-git-branch-manager.ts

@@ -202,6 +202,12 @@ export class SimpleGitBranchManager implements BranchManager {
     return git.diff([`${baseBranch}...${headBranch}`, '--', filePath]);
   }
 
+  async getHeadCommitHash(repoPath: string, branch: string): Promise<string> {
+    const git = simpleGit(repoPath);
+    const result = await git.raw(['rev-parse', branch]);
+    return result.trim();
+  }
+
   async deleteBranch(repoPath: string, branch: string): Promise<void> {
     const git = simpleGit(repoPath);
     const exists = await this.branchExists(repoPath, branch);
@@ -267,12 +273,6 @@ export class SimpleGitBranchManager implements BranchManager {
     return result.trim();
   }
 
-  async getHeadCommitHash(repoPath: string, branch: string): Promise<string> {
-    const git = simpleGit(repoPath);
-    const result = await git.raw(['rev-parse', branch]);
-    return result.trim();
-  }
-
   async pushBranch(repoPath: string, branch: string, remote = 'origin'): Promise<void> {
     const git = simpleGit(repoPath);
     try {

apps/server/review/diff-cache.test.ts

@@ -0,0 +1,76 @@
+/**
+ * Unit tests for DiffCache class.
+ *
+ * Tests TTL expiry, prefix invalidation, and env-var TTL configuration.
+ */
+
+import { describe, it, expect, vi, afterEach } from 'vitest';
+import { DiffCache } from './diff-cache.js';
+
+afterEach(() => {
+  vi.restoreAllMocks();
+});
+
+describe('DiffCache', () => {
+  it('returns undefined for a key that was never set', () => {
+    const cache = new DiffCache<string>(5000);
+    expect(cache.get('nonexistent')).toBeUndefined();
+  });
+
+  it('returns value when entry has not expired', () => {
+    vi.spyOn(Date, 'now').mockReturnValue(1000);
+    const cache = new DiffCache<string>(5000);
+    cache.set('key', 'value');
+    vi.spyOn(Date, 'now').mockReturnValue(5999); // 4999ms elapsed, TTL=5000
+    expect(cache.get('key')).toBe('value');
+  });
+
+  it('returns undefined and deletes the entry when TTL has elapsed', () => {
+    vi.spyOn(Date, 'now').mockReturnValue(1000);
+    const cache = new DiffCache<string>(5000);
+    cache.set('key', 'value');
+    vi.spyOn(Date, 'now').mockReturnValue(6001); // 5001ms elapsed, TTL=5000
+    expect(cache.get('key')).toBeUndefined();
+    // Verify the key is no longer stored (second get also returns undefined)
+    vi.spyOn(Date, 'now').mockReturnValue(6001);
+    expect(cache.get('key')).toBeUndefined();
+  });
+
+  it('overwrites an existing entry and resets its TTL', () => {
+    vi.spyOn(Date, 'now').mockReturnValue(1000);
+    const cache = new DiffCache<string>(5000);
+    cache.set('key', 'first');
+    vi.spyOn(Date, 'now').mockReturnValue(4000); // overwrite before expiry
+    cache.set('key', 'second');
+    vi.spyOn(Date, 'now').mockReturnValue(8999); // 4999ms after overwrite, TTL=5000
+    expect(cache.get('key')).toBe('second');
+    vi.spyOn(Date, 'now').mockReturnValue(9001); // 5001ms after overwrite
+    expect(cache.get('key')).toBeUndefined();
+  });
+
+  it('invalidateByPrefix removes all matching keys and preserves others', () => {
+    const cache = new DiffCache<string>(60_000);
+    cache.set('phase-1:abc', 'a');
+    cache.set('phase-1:abc:file.ts', 'b');
+    cache.set('phase-2:xyz', 'c');
+    cache.invalidateByPrefix('phase-1:');
+    expect(cache.get('phase-1:abc')).toBeUndefined();
+    expect(cache.get('phase-1:abc:file.ts')).toBeUndefined();
+    expect(cache.get('phase-2:xyz')).toBe('c');
+  });
+
+  it('singleton instances use REVIEW_DIFF_CACHE_TTL_MS env var for TTL', async () => {
+    vi.resetModules();
+    vi.stubEnv('REVIEW_DIFF_CACHE_TTL_MS', '1000');
+    const { phaseMetaCache } = await import('./diff-cache.js');
+
+    vi.spyOn(Date, 'now').mockReturnValue(0);
+    phaseMetaCache.set('key', {} as any);
+    vi.spyOn(Date, 'now').mockReturnValue(999);
+    expect(phaseMetaCache.get('key')).toBeDefined();
+    vi.spyOn(Date, 'now').mockReturnValue(1001);
+    expect(phaseMetaCache.get('key')).toBeUndefined();
+
+    vi.unstubAllEnvs();
+  });
+});

apps/server/review/diff-cache.ts

@@ -1,6 +1,9 @@
 /**
- * DiffCache — in-memory cache with TTL and prefix-based invalidation.
+ * DiffCache — in-memory TTL cache for git diff results.
- * Used to avoid re-running expensive git diff subprocesses on repeated requests.
+ *
+ * Keyed by `phaseId:headHash` (or `phaseId:headHash:filePath` for per-file diffs).
+ * TTL defaults to 5 minutes, configurable via REVIEW_DIFF_CACHE_TTL_MS env var.
+ * Prefix-based invalidation clears all entries for a phase when a new commit lands.
  */
 
 import type { FileStatEntry } from '../git/types.js';

apps/server/trpc/routers/phase.test.ts

@@ -0,0 +1,249 @@
+/**
+ * Integration tests for getPhaseReviewDiff caching behaviour.
+ *
+ * Verifies that git diff is only invoked once per HEAD hash and that
+ * cache invalidation after a task merge triggers a re-run.
+ */
+
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import { router, publicProcedure, createCallerFactory } from '../trpc.js';
+import { phaseProcedures } from './phase.js';
+import type { TRPCContext } from '../context.js';
+import type { BranchManager } from '../../git/branch-manager.js';
+import { createTestDatabase } from '../../db/repositories/drizzle/test-helpers.js';
+import {
+  DrizzleInitiativeRepository,
+  DrizzlePhaseRepository,
+  DrizzleProjectRepository,
+} from '../../db/repositories/drizzle/index.js';
+import { phaseMetaCache, fileDiffCache } from '../../review/diff-cache.js';
+
+// =============================================================================
+// Mock ensureProjectClone — prevents actual git cloning
+// =============================================================================
+
+vi.mock('../../git/project-clones.js', () => ({
+  ensureProjectClone: vi.fn().mockResolvedValue('/fake/clone/path'),
+  getProjectCloneDir: vi.fn().mockReturnValue('repos/fake-project-id'),
+}));
+
+// =============================================================================
+// Test router
+// =============================================================================
+
+const testRouter = router({
+  ...phaseProcedures(publicProcedure),
+});
+
+const createCaller = createCallerFactory(testRouter);
+
+// =============================================================================
+// MockBranchManager
+// =============================================================================
+
+function makeMockBranchManager(): BranchManager {
+  return {
+    ensureBranch: vi.fn().mockResolvedValue(undefined),
+    mergeBranch: vi.fn().mockResolvedValue({ success: true, conflictFiles: [] }),
+    diffBranches: vi.fn().mockResolvedValue('diff --git a/file.ts'),
+    diffBranchesStat: vi.fn().mockResolvedValue([]),
+    diffFileSingle: vi.fn().mockResolvedValue('diff --git a/file.ts'),
+    deleteBranch: vi.fn().mockResolvedValue(undefined),
+    branchExists: vi.fn().mockResolvedValue(true),
+    remoteBranchExists: vi.fn().mockResolvedValue(true),
+    listCommits: vi.fn().mockResolvedValue([]),
+    diffCommit: vi.fn().mockResolvedValue(''),
+    getMergeBase: vi.fn().mockResolvedValue('mergebase123'),
+    pushBranch: vi.fn().mockResolvedValue(undefined),
+    checkMergeability: vi.fn().mockResolvedValue({ canMerge: true, conflicts: [] }),
+    fetchRemote: vi.fn().mockResolvedValue(undefined),
+    fastForwardBranch: vi.fn().mockResolvedValue(undefined),
+    updateRef: vi.fn().mockResolvedValue(undefined),
+    getHeadCommitHash: vi.fn().mockResolvedValue('abc123def456'),
+  };
+}
+
+// =============================================================================
+// Helpers
+// =============================================================================
+
+function createMockEventBus(): TRPCContext['eventBus'] {
+  return {
+    emit: vi.fn(),
+    on: vi.fn(),
+    off: vi.fn(),
+    once: vi.fn(),
+  };
+}
+
+interface SeedResult {
+  phaseId: string;
+  initiativeId: string;
+  projectId: string;
+}
+
+async function seedDatabase(): Promise<{
+  repos: {
+    initiativeRepo: DrizzleInitiativeRepository;
+    phaseRepo: DrizzlePhaseRepository;
+    projectRepo: DrizzleProjectRepository;
+  };
+  data: SeedResult;
+}> {
+  const db = createTestDatabase();
+  const initiativeRepo = new DrizzleInitiativeRepository(db);
+  const phaseRepo = new DrizzlePhaseRepository(db);
+  const projectRepo = new DrizzleProjectRepository(db);
+
+  const initiative = await initiativeRepo.create({
+    name: 'Test Initiative',
+    status: 'active',
+    branch: 'main',
+  });
+
+  const phase = await phaseRepo.create({
+    initiativeId: initiative.id,
+    name: 'Test Phase',
+    status: 'pending_review',
+  });
+
+  const project = await projectRepo.create({
+    name: 'Test Project',
+    url: 'https://github.com/test/repo',
+  });
+
+  await projectRepo.addProjectToInitiative(initiative.id, project.id);
+
+  return {
+    repos: { initiativeRepo, phaseRepo, projectRepo },
+    data: { phaseId: phase.id, initiativeId: initiative.id, projectId: project.id },
+  };
+}
+
+async function seedDatabaseNoProjects(): Promise<{
+  repos: {
+    initiativeRepo: DrizzleInitiativeRepository;
+    phaseRepo: DrizzlePhaseRepository;
+    projectRepo: DrizzleProjectRepository;
+  };
+  data: { phaseId: string };
+}> {
+  const db = createTestDatabase();
+  const initiativeRepo = new DrizzleInitiativeRepository(db);
+  const phaseRepo = new DrizzlePhaseRepository(db);
+  const projectRepo = new DrizzleProjectRepository(db);
+
+  const initiative = await initiativeRepo.create({
+    name: 'Test Initiative No Projects',
+    status: 'active',
+    branch: 'main',
+  });
+
+  const phase = await phaseRepo.create({
+    initiativeId: initiative.id,
+    name: 'Empty Phase',
+    status: 'pending_review',
+  });
+
+  return {
+    repos: { initiativeRepo, phaseRepo, projectRepo },
+    data: { phaseId: phase.id },
+  };
+}
+
+function makeCaller(
+  branchManager: BranchManager,
+  repos: {
+    initiativeRepo: DrizzleInitiativeRepository;
+    phaseRepo: DrizzlePhaseRepository;
+    projectRepo: DrizzleProjectRepository;
+  },
+) {
+  const ctx: TRPCContext = {
+    eventBus: createMockEventBus(),
+    serverStartedAt: null,
+    processCount: 0,
+    branchManager,
+    initiativeRepository: repos.initiativeRepo,
+    phaseRepository: repos.phaseRepo,
+    projectRepository: repos.projectRepo,
+    workspaceRoot: '/fake/workspace',
+  };
+  return createCaller(ctx);
+}
+
+// =============================================================================
+// Tests
+// =============================================================================
+
+beforeEach(() => {
+  // Clear caches between tests to ensure isolation
+  phaseMetaCache.invalidateByPrefix('');
+  fileDiffCache.invalidateByPrefix('');
+});
+
+describe('getPhaseReviewDiff caching', () => {
+  it('second call for same phase/HEAD returns cached result without calling git again', async () => {
+    const { repos, data } = await seedDatabase();
+    const branchManager = makeMockBranchManager();
+    const diffBranchesSpy = vi.spyOn(branchManager, 'diffBranchesStat');
+    const caller = makeCaller(branchManager, repos);
+
+    await caller.getPhaseReviewDiff({ phaseId: data.phaseId });
+    await caller.getPhaseReviewDiff({ phaseId: data.phaseId });
+
+    expect(diffBranchesSpy).toHaveBeenCalledTimes(1);
+  });
+
+  it('after cache invalidation, next call re-runs git diff', async () => {
+    const { repos, data } = await seedDatabase();
+    const branchManager = makeMockBranchManager();
+    const diffBranchesSpy = vi.spyOn(branchManager, 'diffBranchesStat');
+    const caller = makeCaller(branchManager, repos);
+
+    await caller.getPhaseReviewDiff({ phaseId: data.phaseId });
+    expect(diffBranchesSpy).toHaveBeenCalledTimes(1);
+
+    // Simulate a task merge → cache invalidated
+    phaseMetaCache.invalidateByPrefix(`${data.phaseId}:`);
+
+    await caller.getPhaseReviewDiff({ phaseId: data.phaseId });
+    expect(diffBranchesSpy).toHaveBeenCalledTimes(2);
+  });
+
+  it('different HEAD hashes for same phase are treated as distinct cache entries', async () => {
+    const { repos, data } = await seedDatabase();
+    const branchManager = makeMockBranchManager();
+    const diffBranchesSpy = vi.spyOn(branchManager, 'diffBranchesStat');
+    const caller = makeCaller(branchManager, repos);
+
+    // First call with headHash = 'abc123'
+    vi.spyOn(branchManager, 'getHeadCommitHash').mockResolvedValueOnce('abc123');
+    await caller.getPhaseReviewDiff({ phaseId: data.phaseId });
+
+    // Second call with headHash = 'def456' (simulates a new commit)
+    vi.spyOn(branchManager, 'getHeadCommitHash').mockResolvedValueOnce('def456');
+    await caller.getPhaseReviewDiff({ phaseId: data.phaseId });
+
+    expect(diffBranchesSpy).toHaveBeenCalledTimes(2);
+  });
+
+  it('throws NOT_FOUND for nonexistent phaseId', async () => {
+    const { repos } = await seedDatabase();
+    const caller = makeCaller(makeMockBranchManager(), repos);
+
+    await expect(caller.getPhaseReviewDiff({ phaseId: 'nonexistent' }))
+      .rejects.toMatchObject({ code: 'NOT_FOUND' });
+  });
+
+  it('phase with no projects returns empty result without calling git', async () => {
+    const { repos, data } = await seedDatabaseNoProjects();
+    const branchManager = makeMockBranchManager();
+    const diffBranchesSpy = vi.spyOn(branchManager, 'diffBranchesStat');
+    const caller = makeCaller(branchManager, repos);
+
+    const result = await caller.getPhaseReviewDiff({ phaseId: data.phaseId });
+    expect(diffBranchesSpy).not.toHaveBeenCalled();
+    expect(result).toHaveProperty('phaseName');
+  });
+});

apps/server/trpc/routers/phase.ts

@@ -236,18 +236,28 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) {
         const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch;
 
         const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId);
-        const files: FileStatEntry[] = [];
 
         if (projects.length === 0) {
-          return { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, files: [], totalAdditions: 0, totalDeletions: 0 };
+          return {
+            phaseName: phase.name,
+            sourceBranch: phBranch,
+            targetBranch: initBranch,
+            files: [],
+            totalAdditions: 0,
+            totalDeletions: 0,
+          };
         }
 
         const firstClone = await ensureProjectClone(projects[0], ctx.workspaceRoot!);
         const headHash = await branchManager.getHeadCommitHash(firstClone, phBranch);
         const cacheKey = `${input.phaseId}:${headHash}`;
-        const cached = phaseMetaCache.get(cacheKey);
+
+        type PhaseReviewDiffResult = { phaseName: string; sourceBranch: string; targetBranch: string; files: FileStatEntry[]; totalAdditions: number; totalDeletions: number };
+        const cached = phaseMetaCache.get(cacheKey) as PhaseReviewDiffResult | undefined;
         if (cached) return cached;
 
+        const files: FileStatEntry[] = [];
+
         for (const project of projects) {
           const clonePath = await ensureProjectClone(project, ctx.workspaceRoot!);
           const entries = await branchManager.diffBranchesStat(clonePath, diffBase, phBranch);

docs/server-api.md

@@ -118,7 +118,7 @@ Each procedure uses `require*Repository(ctx)` helpers that throw `TRPCError(INTE
 | listInitiativePhaseDependencies | query | All dependency edges |
 | getPhaseDependencies | query | What this phase depends on |
 | getPhaseDependents | query | What depends on this phase |
-| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content |
+| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content. Results are cached in-memory by `phaseId:headHash` (TTL: `REVIEW_DIFF_CACHE_TTL_MS`, default 5 min). Cache is invalidated when a task merges into the phase branch. |
 | getFileDiff | query | Per-file unified diff on demand: `{phaseId, filePath, projectId?}` → `{binary: boolean, rawDiff: string}`; `filePath` must be URL-encoded; binary files return `{binary: true, rawDiff: ''}` |
 | getPhaseReviewCommits | query | List commits between initiative and phase branch |
 | getCommitDiff | query | Diff for a single commit (by hash) in a phase |