Merge branch 'cw/review-tab-performance' into cw-merge-1772826318787

2026-03-06 20:45:19 +01:00
parent afdc1c7e00 1120473198
commit ba6ebe2594
40 changed files with 3594 additions and 320 deletions
--- a/apps/server/execution/orchestrator.test.ts
+++ b/apps/server/execution/orchestrator.test.ts
@@ -46,6 +46,9 @@ function createMocks() {
    ensureBranch: vi.fn(),
    mergeBranch: vi.fn().mockResolvedValue({ success: true, message: 'merged', previousRef: 'abc000' }),
    diffBranches: vi.fn().mockResolvedValue(''),
+    diffBranchesStat: vi.fn().mockResolvedValue([]),
+    diffFileSingle: vi.fn().mockResolvedValue(''),
+    getHeadCommitHash: vi.fn().mockResolvedValue('deadbeef00000000000000000000000000000000'),
    deleteBranch: vi.fn(),
    branchExists: vi.fn().mockResolvedValue(true),
    remoteBranchExists: vi.fn().mockResolvedValue(false),
--- a/apps/server/execution/orchestrator.ts
+++ b/apps/server/execution/orchestrator.ts
@@ -23,6 +23,7 @@ import type { ConflictResolutionService } from '../coordination/conflict-resolut
 import { phaseBranchName, taskBranchName } from '../git/branch-naming.js';
 import { ensureProjectClone } from '../git/project-clones.js';
 import { createModuleLogger } from '../logger/index.js';
+import { phaseMetaCache, fileDiffCache } from '../review/diff-cache.js';

 const log = createModuleLogger('execution-orchestrator');

@@ -253,6 +254,10 @@ export class ExecutionOrchestrator {
      log.info({ taskId, taskBranch, phaseBranch, project: project.name }, 'task branch merged into phase branch');
    }

+    // Invalidate diff cache — phase branch HEAD has advanced after merges
+    phaseMetaCache.invalidateByPrefix(`${phaseId}:`);
+    fileDiffCache.invalidateByPrefix(`${phaseId}:`);
+
    // Emit task:merged event
    const mergedEvent: TaskMergedEvent = {
      type: 'task:merged',
--- a/apps/server/git/branch-manager.ts
+++ b/apps/server/git/branch-manager.ts
@@ -6,7 +6,7 @@
 * a worktree to be checked out.
 */

-import type { MergeResult, MergeabilityResult, BranchCommit } from './types.js';
+import type { MergeResult, MergeabilityResult, BranchCommit, FileStatEntry } from './types.js';

 export interface BranchManager {
  /**
@@ -29,6 +29,27 @@ export interface BranchManager {
   */
  diffBranches(repoPath: string, baseBranch: string, headBranch: string): Promise<string>;

+  /**
+   * Get per-file metadata for changes between two branches.
+   * Uses three-dot diff (baseBranch...headBranch) — same divergence model as diffBranches.
+   * Binary files are included with status 'binary' and additions/deletions both 0.
+   * Does NOT return hunk content.
+   */
+  diffBranchesStat(repoPath: string, baseBranch: string, headBranch: string): Promise<FileStatEntry[]>;
+
+  /**
+   * Get the raw unified diff for a single file between two branches.
+   * Uses three-dot diff (baseBranch...headBranch).
+   * Returns empty string for binary files (caller must detect binary separately).
+   * filePath must be URL-decoded before being passed here.
+   */
+  diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise<string>;
+
+  /**
+   * Returns the current HEAD commit hash (40-char SHA) for the given branch in the repo.
+   */
+  getHeadCommitHash(repoPath: string, branch: string): Promise<string>;
+
  /**
   * Delete a branch. No-op if the branch doesn't exist.
   */
--- a/apps/server/git/simple-git-branch-manager.test.ts
+++ b/apps/server/git/simple-git-branch-manager.test.ts
@@ -65,6 +65,69 @@ async function createTestRepoWithRemote(): Promise<{
  };
 }

+/**
+ * Create a repo pair for testing diff operations.
+ * Sets up bare + clone with a 'feature' branch that has known changes vs 'main'.
+ */
+async function createTestRepoForDiff(): Promise<{
+  clonePath: string;
+  cleanup: () => Promise<void>;
+}> {
+  const tmpBase = await mkdtemp(path.join(tmpdir(), 'cw-diff-test-'));
+  const barePath = path.join(tmpBase, 'bare.git');
+  const workPath = path.join(tmpBase, 'work');
+  const clonePath = path.join(tmpBase, 'clone');
+
+  // Create bare repo
+  await simpleGit().init([barePath, '--bare']);
+
+  // Set up main branch in work dir
+  await simpleGit().clone(barePath, workPath);
+  const workGit = simpleGit(workPath);
+  await workGit.addConfig('user.email', 'test@example.com');
+  await workGit.addConfig('user.name', 'Test User');
+  await writeFile(path.join(workPath, 'README.md'), '# README\n');
+  await writeFile(path.join(workPath, 'to-delete.txt'), 'delete me\n');
+  await workGit.add(['README.md', 'to-delete.txt']);
+  await workGit.commit('Initial commit');
+  await workGit.push('origin', 'main');
+
+  // Clone and create feature branch with changes
+  await simpleGit().clone(barePath, clonePath);
+  const cloneGit = simpleGit(clonePath);
+  await cloneGit.addConfig('user.email', 'test@example.com');
+  await cloneGit.addConfig('user.name', 'Test User');
+  await cloneGit.checkoutLocalBranch('feature');
+
+  // Add new text file
+  await writeFile(path.join(clonePath, 'added.txt'), 'new content\n');
+  await cloneGit.add('added.txt');
+
+  // Modify existing file
+  await writeFile(path.join(clonePath, 'README.md'), '# README\n\nModified content\n');
+  await cloneGit.add('README.md');
+
+  // Delete a file
+  await cloneGit.rm(['to-delete.txt']);
+
+  // Add binary file
+  await writeFile(path.join(clonePath, 'image.bin'), Buffer.alloc(16));
+  await cloneGit.add('image.bin');
+
+  // Add file with space in name
+  await writeFile(path.join(clonePath, 'has space.txt'), 'content\n');
+  await cloneGit.add('has space.txt');
+
+  await cloneGit.commit('Feature branch changes');
+
+  return {
+    clonePath,
+    cleanup: async () => {
+      await rm(tmpBase, { recursive: true, force: true });
+    },
+  };
+}
+
 describe('SimpleGitBranchManager', () => {
  let clonePath: string;
  let cleanup: () => Promise<void>;
@@ -108,3 +171,80 @@ describe('SimpleGitBranchManager', () => {
    });
  });
 });
+
+describe('SimpleGitBranchManager - diffBranchesStat and diffFileSingle', () => {
+  let clonePath: string;
+  let cleanup: () => Promise<void>;
+  let branchManager: SimpleGitBranchManager;
+
+  beforeEach(async () => {
+    const setup = await createTestRepoForDiff();
+    clonePath = setup.clonePath;
+    cleanup = setup.cleanup;
+    branchManager = new SimpleGitBranchManager();
+  });
+
+  afterEach(async () => {
+    await cleanup();
+  });
+
+  describe('diffBranchesStat', () => {
+    it('returns correct entries for added, modified, and deleted text files', async () => {
+      const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature');
+      const added = entries.find(e => e.path === 'added.txt');
+      const modified = entries.find(e => e.path === 'README.md');
+      const deleted = entries.find(e => e.path === 'to-delete.txt');
+
+      expect(added?.status).toBe('added');
+      expect(added?.additions).toBeGreaterThan(0);
+      expect(modified?.status).toBe('modified');
+      expect(deleted?.status).toBe('deleted');
+      expect(deleted?.deletions).toBeGreaterThan(0);
+    });
+
+    it('marks binary files as status=binary with additions=0, deletions=0', async () => {
+      const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature');
+      const binary = entries.find(e => e.path === 'image.bin');
+      expect(binary?.status).toBe('binary');
+      expect(binary?.additions).toBe(0);
+      expect(binary?.deletions).toBe(0);
+    });
+
+    it('returns empty array when there are no changes', async () => {
+      const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'main');
+      expect(entries).toEqual([]);
+    });
+
+    it('handles files with spaces in their names', async () => {
+      const entries = await branchManager.diffBranchesStat(clonePath, 'main', 'feature');
+      const spaced = entries.find(e => e.path === 'has space.txt');
+      expect(spaced).toBeDefined();
+      expect(spaced?.status).toBe('added');
+    });
+  });
+
+  describe('diffFileSingle', () => {
+    it('returns unified diff containing addition hunks for an added file', async () => {
+      const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'added.txt');
+      expect(diff).toContain('+');
+      expect(diff).toContain('added.txt');
+    });
+
+    it('returns unified diff with removal hunks for a deleted file', async () => {
+      const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'to-delete.txt');
+      expect(diff).toContain('-');
+      expect(diff).toContain('to-delete.txt');
+    });
+
+    it('returns string for a binary file', async () => {
+      const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'image.bin');
+      // git diff returns empty or a "Binary files differ" line — no hunk content
+      expect(typeof diff).toBe('string');
+    });
+
+    it('handles file paths with spaces', async () => {
+      const diff = await branchManager.diffFileSingle(clonePath, 'main', 'feature', 'has space.txt');
+      expect(diff).toContain('has space.txt');
+    });
+  });
+});
--- a/apps/server/git/simple-git-branch-manager.ts
+++ b/apps/server/git/simple-git-branch-manager.ts
@@ -11,11 +11,31 @@ import { mkdtempSync, rmSync } from 'node:fs';
 import { tmpdir } from 'node:os';
 import { simpleGit } from 'simple-git';
 import type { BranchManager } from './branch-manager.js';
-import type { MergeResult, MergeabilityResult, BranchCommit } from './types.js';
+import type { MergeResult, MergeabilityResult, BranchCommit, FileStatEntry } from './types.js';
 import { createModuleLogger } from '../logger/index.js';

 const log = createModuleLogger('branch-manager');

+/**
+ * Normalize a numstat path to the new path for rename entries.
+ * Handles patterns like:
+ *   - "{old.txt => new.txt}" → "new.txt"
+ *   - "dir/{old.txt => new.txt}" → "dir/new.txt"
+ *   - "old_dir/file.txt => new_dir/file.txt" → "new_dir/file.txt"
+ */
+function normalizeNumstatPath(pathStr: string): string {
+  const braceMatch = pathStr.match(/^(.*)\{(.*) => (.*)\}(.*)$/);
+  if (braceMatch) {
+    const [, prefix, , newPart, suffix] = braceMatch;
+    return `${prefix}${newPart}${suffix}`;
+  }
+  const arrowMatch = pathStr.match(/^.* => (.+)$/);
+  if (arrowMatch) {
+    return arrowMatch[1];
+  }
+  return pathStr;
+}
+
 export class SimpleGitBranchManager implements BranchManager {
  async ensureBranch(repoPath: string, branch: string, baseBranch: string): Promise<void> {
    const git = simpleGit(repoPath);
@@ -97,6 +117,97 @@ export class SimpleGitBranchManager implements BranchManager {
    return diff;
  }

+  async diffBranchesStat(repoPath: string, baseBranch: string, headBranch: string): Promise<FileStatEntry[]> {
+    const git = simpleGit(repoPath);
+    const range = `${baseBranch}...${headBranch}`;
+
+    const [nameStatusRaw, numStatRaw] = await Promise.all([
+      git.raw(['diff', '--name-status', range]),
+      git.raw(['diff', '--numstat', range]),
+    ]);
+
+    if (!nameStatusRaw.trim()) return [];
+
+    // Parse numstat: "<additions>\t<deletions>\t<path>"
+    // Binary files: "-\t-\t<path>"
+    const numStatMap = new Map<string, { additions: number; deletions: number; binary: boolean }>();
+    for (const line of numStatRaw.split('\n')) {
+      if (!line.trim()) continue;
+      const tabIdx1 = line.indexOf('\t');
+      const tabIdx2 = line.indexOf('\t', tabIdx1 + 1);
+      if (tabIdx1 === -1 || tabIdx2 === -1) continue;
+      const addStr = line.slice(0, tabIdx1);
+      const delStr = line.slice(tabIdx1 + 1, tabIdx2);
+      const pathStr = line.slice(tabIdx2 + 1);
+      const binary = addStr === '-' && delStr === '-';
+      // Normalize rename paths like "{old => new}" or "dir/{old => new}/file" to new path
+      const newPath = normalizeNumstatPath(pathStr);
+      numStatMap.set(newPath, {
+        additions: binary ? 0 : parseInt(addStr, 10),
+        deletions: binary ? 0 : parseInt(delStr, 10),
+        binary,
+      });
+    }
+
+    // Parse name-status: "<status>\t<path>" or "<Rxx>\t<oldPath>\t<newPath>"
+    const entries: FileStatEntry[] = [];
+    for (const line of nameStatusRaw.split('\n')) {
+      if (!line.trim()) continue;
+      const parts = line.split('\t');
+      if (parts.length < 2) continue;
+
+      const statusCode = parts[0];
+      let status: FileStatEntry['status'];
+      let filePath: string;
+      let oldPath: string | undefined;
+
+      if (statusCode.startsWith('R')) {
+        status = 'renamed';
+        oldPath = parts[1];
+        filePath = parts[2];
+      } else if (statusCode === 'A') {
+        status = 'added';
+        filePath = parts[1];
+      } else if (statusCode === 'M') {
+        status = 'modified';
+        filePath = parts[1];
+      } else if (statusCode === 'D') {
+        status = 'deleted';
+        filePath = parts[1];
+      } else {
+        status = 'modified';
+        filePath = parts[1];
+      }
+
+      const numStat = numStatMap.get(filePath);
+      if (numStat?.binary) {
+        status = 'binary';
+      }
+
+      const entry: FileStatEntry = {
+        path: filePath,
+        status,
+        additions: numStat?.additions ?? 0,
+        deletions: numStat?.deletions ?? 0,
+      };
+      if (oldPath !== undefined) entry.oldPath = oldPath;
+      entries.push(entry);
+    }
+
+    return entries;
+  }
+
+  async diffFileSingle(repoPath: string, baseBranch: string, headBranch: string, filePath: string): Promise<string> {
+    const git = simpleGit(repoPath);
+    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);
--- a/apps/server/git/types.ts
+++ b/apps/server/git/types.ts
@@ -100,6 +100,29 @@ export interface BranchCommit {
  deletions: number;
 }

+// =============================================================================
+// File Stat Entry (per-file diff metadata)
+// =============================================================================
+
+/**
+ * Metadata for a single file changed between two branches.
+ * No hunk content — only path, status, and line-count statistics.
+ */
+export interface FileStatEntry {
+  /** New path (or old path for deletions) */
+  path: string;
+  /** Only set for renames — the path before the rename */
+  oldPath?: string;
+  /** Nature of the change */
+  status: 'added' | 'modified' | 'deleted' | 'renamed' | 'binary';
+  /** Lines added (0 for binary files) */
+  additions: number;
+  /** Lines deleted (0 for binary files) */
+  deletions: number;
+  /** Which project clone this file belongs to (populated by callers in multi-project scenarios) */
+  projectId?: string;
+}
+
 // =============================================================================
 // WorktreeManager Port Interface
 // =============================================================================
--- a/apps/server/review/diff-cache.test.ts
+++ b/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();
+  });
+});
--- a/apps/server/review/diff-cache.ts
+++ b/apps/server/review/diff-cache.ts
@@ -0,0 +1,70 @@
+/**
+ * DiffCache — in-memory TTL cache for git diff results.
+ *
+ * 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';
+
+interface CacheEntry<T> {
+  value: T;
+  expiresAt: number;
+}
+
+export class DiffCache<T> {
+  private store = new Map<string, CacheEntry<T>>();
+  private ttlMs: number;
+
+  constructor(ttlMs: number) {
+    this.ttlMs = ttlMs;
+  }
+
+  get(key: string): T | undefined {
+    const entry = this.store.get(key);
+    if (!entry) return undefined;
+    if (Date.now() > entry.expiresAt) {
+      this.store.delete(key);
+      return undefined;
+    }
+    return entry.value;
+  }
+
+  set(key: string, value: T): void {
+    this.store.set(key, { value, expiresAt: Date.now() + this.ttlMs });
+  }
+
+  invalidateByPrefix(prefix: string): void {
+    for (const key of this.store.keys()) {
+      if (key.startsWith(prefix)) this.store.delete(key);
+    }
+  }
+}
+
+// ---------------------------------------------------------------------------
+// Response shapes (mirror the return types of getPhaseReviewDiff / getFileDiff)
+// ---------------------------------------------------------------------------
+
+export interface PhaseMetaResponse {
+  phaseName: string;
+  sourceBranch: string;
+  targetBranch: string;
+  files: FileStatEntry[];
+  totalAdditions: number;
+  totalDeletions: number;
+}
+
+export interface FileDiffResponse {
+  binary: boolean;
+  rawDiff: string;
+}
+
+// ---------------------------------------------------------------------------
+// Singleton instances — TTL is read once at module load time
+// ---------------------------------------------------------------------------
+
+const TTL = parseInt(process.env.REVIEW_DIFF_CACHE_TTL_MS ?? '300000', 10);
+
+export const phaseMetaCache = new DiffCache<PhaseMetaResponse>(TTL);
+export const fileDiffCache = new DiffCache<FileDiffResponse>(TTL);
--- a/apps/server/test/integration/phase-review-diff.test.ts
+++ b/apps/server/test/integration/phase-review-diff.test.ts
@@ -0,0 +1,256 @@
+/**
+ * Integration tests for getPhaseReviewDiff and getFileDiff tRPC procedures.
+ *
+ * Uses real git repos on disk (no cassettes) + an in-memory SQLite database.
+ * No network calls — purely local git operations.
+ */
+
+import { describe, it, expect, beforeAll, afterAll } from 'vitest';
+import { mkdtemp, rm, writeFile, mkdir } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import path from 'node:path';
+import { simpleGit } from 'simple-git';
+import { router, publicProcedure, createCallerFactory } from '../../trpc/trpc.js';
+import { phaseProcedures } from '../../trpc/routers/phase.js';
+import { createTestDatabase } from '../../db/repositories/drizzle/test-helpers.js';
+import {
+  DrizzleInitiativeRepository,
+  DrizzlePhaseRepository,
+  DrizzleProjectRepository,
+} from '../../db/repositories/drizzle/index.js';
+import { SimpleGitBranchManager } from '../../git/simple-git-branch-manager.js';
+import { getProjectCloneDir } from '../../git/project-clones.js';
+import type { TRPCContext } from '../../trpc/context.js';
+
+// ============================================================================
+// Test router & caller factory
+// ============================================================================
+
+const testRouter = router({
+  ...phaseProcedures(publicProcedure),
+});
+
+const createCaller = createCallerFactory(testRouter);
+
+// ============================================================================
+// Shared test state (set up once for the whole suite)
+// ============================================================================
+
+let workspaceRoot: string;
+let cleanup: () => Promise<void>;
+let phaseId: string;
+let pendingPhaseId: string;
+
+/**
+ * Build the test git repo with the required branches and files.
+ *
+ * Repo layout on the phase branch vs main:
+ *   file1.txt through file5.txt — added (10–20 lines each)
+ *   photo.bin                  — binary file (Buffer.alloc(32))
+ *   gone.txt                   — deleted (existed on main)
+ *   has space.txt              — added (contains text)
+ */
+async function setupGitRepo(clonePath: string): Promise<void> {
+  await mkdir(clonePath, { recursive: true });
+
+  const git = simpleGit(clonePath);
+  await git.init();
+  await git.addConfig('user.email', 'test@example.com');
+  await git.addConfig('user.name', 'Test');
+
+  // Commit gone.txt on main so it can be deleted on the phase branch
+  await writeFile(path.join(clonePath, 'gone.txt'), Array.from({ length: 5 }, (_, i) => `line ${i + 1}`).join('\n') + '\n');
+  await git.add('gone.txt');
+  await git.commit('Initial commit on main');
+
+  // Create phase branch from main
+  // phaseBranchName('main', 'test-phase') => 'main-phase-test-phase'
+  await git.checkoutLocalBranch('main-phase-test-phase');
+
+  // Add 5 text files
+  for (let i = 1; i <= 5; i++) {
+    const lines = Array.from({ length: 15 }, (_, j) => `Line ${j + 1} in file${i}`).join('\n') + '\n';
+    await writeFile(path.join(clonePath, `file${i}.txt`), lines);
+  }
+  await git.add(['file1.txt', 'file2.txt', 'file3.txt', 'file4.txt', 'file5.txt']);
+
+  // Add binary file
+  await writeFile(path.join(clonePath, 'photo.bin'), Buffer.alloc(32));
+  await git.add('photo.bin');
+
+  // Delete gone.txt
+  await git.rm(['gone.txt']);
+
+  // Add file with space in name
+  await writeFile(path.join(clonePath, 'has space.txt'), 'content with spaces\n');
+  await git.add('has space.txt');
+
+  await git.commit('Phase branch changes');
+}
+
+beforeAll(async () => {
+  // Create workspace root temp dir
+  workspaceRoot = await mkdtemp(path.join(tmpdir(), 'cw-phase-diff-test-'));
+  cleanup = async () => {
+    await rm(workspaceRoot, { recursive: true, force: true });
+  };
+
+  // Set up in-memory database
+  const db = createTestDatabase();
+  const initiativeRepo = new DrizzleInitiativeRepository(db);
+  const phaseRepo = new DrizzlePhaseRepository(db);
+  const projectRepo = new DrizzleProjectRepository(db);
+
+  // Create initiative with branch='main'
+  const initiative = await initiativeRepo.create({
+    name: 'Test Initiative',
+    branch: 'main',
+  });
+
+  // Create project — we'll set up the git repo at the expected clone path
+  const project = await projectRepo.create({
+    name: 'test-repo',
+    url: 'file:///dev/null', // won't be cloned — we create the repo directly
+    defaultBranch: 'main',
+  });
+
+  // Link project to initiative
+  await projectRepo.addProjectToInitiative(initiative.id, project.id);
+
+  // Set up git repo at the expected clone path
+  const relPath = getProjectCloneDir(project.name, project.id);
+  const clonePath = path.join(workspaceRoot, relPath);
+  await setupGitRepo(clonePath);
+
+  // Create reviewable phase (pending_review)
+  const phase = await phaseRepo.create({
+    initiativeId: initiative.id,
+    name: 'test-phase',
+    status: 'pending_review',
+  });
+  phaseId = phase.id;
+
+  // Create a non-reviewable phase (pending) for error test
+  const pendingPhase = await phaseRepo.create({
+    initiativeId: initiative.id,
+    name: 'pending-phase',
+    status: 'pending',
+  });
+  pendingPhaseId = pendingPhase.id;
+
+  // Store db and repos so the caller can use them
+  // (stored in module-level vars to be accessed in test helper)
+  Object.assign(sharedCtx, {
+    initiativeRepository: initiativeRepo,
+    phaseRepository: phaseRepo,
+    projectRepository: projectRepo,
+    branchManager: new SimpleGitBranchManager(),
+    workspaceRoot,
+  });
+});
+
+afterAll(async () => {
+  await cleanup?.();
+});
+
+// ============================================================================
+// Shared context (filled in beforeAll)
+// ============================================================================
+
+const sharedCtx: Partial<TRPCContext> = {
+  eventBus: { emit: () => {}, on: () => {}, off: () => {}, once: () => {} } as any,
+  serverStartedAt: null,
+  processCount: 0,
+};
+
+function getCaller() {
+  return createCaller(sharedCtx as TRPCContext);
+}
+
+// ============================================================================
+// Tests: getPhaseReviewDiff
+// ============================================================================
+
+describe('getPhaseReviewDiff', () => {
+  it('returns files array with correct metadata and no rawDiff field', async () => {
+    const start = Date.now();
+    const result = await getCaller().getPhaseReviewDiff({ phaseId });
+    const elapsed = Date.now() - start;
+
+    expect(result).not.toHaveProperty('rawDiff');
+    expect(result.files).toBeInstanceOf(Array);
+    // 5 text + 1 binary + 1 deleted + 1 spaced = 8 files
+    expect(result.files.length).toBeGreaterThanOrEqual(7);
+    expect(elapsed).toBeLessThan(3000);
+  });
+
+  it('includes binary file with status=binary, additions=0, deletions=0', async () => {
+    const result = await getCaller().getPhaseReviewDiff({ phaseId });
+    const bin = result.files.find((f) => f.path === 'photo.bin');
+    expect(bin?.status).toBe('binary');
+    expect(bin?.additions).toBe(0);
+    expect(bin?.deletions).toBe(0);
+  });
+
+  it('includes deleted file with status=deleted and nonzero deletions', async () => {
+    const result = await getCaller().getPhaseReviewDiff({ phaseId });
+    const del = result.files.find((f) => f.path === 'gone.txt');
+    expect(del?.status).toBe('deleted');
+    expect(del?.deletions).toBeGreaterThan(0);
+  });
+
+  it('computes totalAdditions and totalDeletions as sums over files', async () => {
+    const result = await getCaller().getPhaseReviewDiff({ phaseId });
+    const sumAdd = result.files.reduce((s, f) => s + f.additions, 0);
+    const sumDel = result.files.reduce((s, f) => s + f.deletions, 0);
+    expect(result.totalAdditions).toBe(sumAdd);
+    expect(result.totalDeletions).toBe(sumDel);
+  });
+
+  it('throws NOT_FOUND for unknown phaseId', async () => {
+    const err = await getCaller().getPhaseReviewDiff({ phaseId: 'nonexistent' }).catch((e) => e);
+    expect(err.code).toBe('NOT_FOUND');
+  });
+
+  it('throws BAD_REQUEST for phase not in reviewable status', async () => {
+    const err = await getCaller().getPhaseReviewDiff({ phaseId: pendingPhaseId }).catch((e) => e);
+    expect(err.code).toBe('BAD_REQUEST');
+  });
+});
+
+// ============================================================================
+// Tests: getFileDiff
+// ============================================================================
+
+describe('getFileDiff', () => {
+  it('returns rawDiff with unified diff for a normal file, under 1 second', async () => {
+    const start = Date.now();
+    const result = await getCaller().getFileDiff({ phaseId, filePath: 'file1.txt' });
+    const elapsed = Date.now() - start;
+
+    expect(result.binary).toBe(false);
+    expect(result.rawDiff).toContain('+');
+    expect(elapsed).toBeLessThan(1000);
+  });
+
+  it('returns binary=true and rawDiff="" for binary file', async () => {
+    const result = await getCaller().getFileDiff({ phaseId, filePath: 'photo.bin' });
+    expect(result.binary).toBe(true);
+    expect(result.rawDiff).toBe('');
+  });
+
+  it('returns removal hunks for a deleted file', async () => {
+    const result = await getCaller().getFileDiff({ phaseId, filePath: 'gone.txt' });
+    expect(result.binary).toBe(false);
+    expect(result.rawDiff).toContain('-');
+  });
+
+  it('handles URL-encoded file path with space', async () => {
+    const result = await getCaller().getFileDiff({
+      phaseId,
+      filePath: encodeURIComponent('has space.txt'),
+    });
+    expect(result.binary).toBe(false);
+    expect(result.rawDiff).toContain('has space.txt');
+  });
+});
--- a/apps/server/trpc/routers/phase.test.ts
+++ b/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');
+  });
+});
--- a/apps/server/trpc/routers/phase.ts
+++ b/apps/server/trpc/routers/phase.ts
@@ -4,11 +4,14 @@

 import { TRPCError } from '@trpc/server';
 import { z } from 'zod';
+import { simpleGit } from 'simple-git';
 import type { Phase } from '../../db/schema.js';
 import type { ProcedureBuilder } from '../trpc.js';
 import { requirePhaseRepository, requireTaskRepository, requireBranchManager, requireInitiativeRepository, requireProjectRepository, requireExecutionOrchestrator, requireReviewCommentRepository, requireChangeSetRepository } from './_helpers.js';
 import { phaseBranchName } from '../../git/branch-naming.js';
 import { ensureProjectClone } from '../../git/project-clones.js';
+import type { FileStatEntry } from '../../git/types.js';
+import { phaseMetaCache, fileDiffCache } from '../../review/diff-cache.js';

 export function phaseProcedures(publicProcedure: ProcedureBuilder) {
  return {
@@ -230,26 +233,124 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) {

        const initBranch = initiative.branch;
        const phBranch = phaseBranchName(initBranch, phase.name);
-        // For completed phases, use stored merge base; for pending_review, use initiative branch
        const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch;

        const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId);
-        let rawDiff = '';
+
+        if (projects.length === 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}`;
+
+        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 diff = await branchManager.diffBranches(clonePath, diffBase, phBranch);
-          if (diff) {
-            rawDiff += diff + '\n';
+          const entries = await branchManager.diffBranchesStat(clonePath, diffBase, phBranch);
+          for (const entry of entries) {
+            const tagged: FileStatEntry = { ...entry, projectId: project.id };
+            if (projects.length > 1) {
+              tagged.path = `${project.name}/${entry.path}`;
+              if (entry.oldPath) {
+                tagged.oldPath = `${project.name}/${entry.oldPath}`;
+              }
+            }
+            files.push(tagged);
          }
        }

-        return {
+        const totalAdditions = files.reduce((sum, f) => sum + f.additions, 0);
+        const totalDeletions = files.reduce((sum, f) => sum + f.deletions, 0);
+
+        const result = {
          phaseName: phase.name,
          sourceBranch: phBranch,
          targetBranch: initBranch,
-          rawDiff,
+          files,
+          totalAdditions,
+          totalDeletions,
        };
+        phaseMetaCache.set(cacheKey, result);
+        return result;
+      }),
+
+    getFileDiff: publicProcedure
+      .input(z.object({
+        phaseId: z.string().min(1),
+        filePath: z.string().min(1),
+        projectId: z.string().optional(),
+      }))
+      .query(async ({ ctx, input }) => {
+        const phaseRepo = requirePhaseRepository(ctx);
+        const initiativeRepo = requireInitiativeRepository(ctx);
+        const projectRepo = requireProjectRepository(ctx);
+        const branchManager = requireBranchManager(ctx);
+
+        const phase = await phaseRepo.findById(input.phaseId);
+        if (!phase) {
+          throw new TRPCError({ code: 'NOT_FOUND', message: `Phase '${input.phaseId}' not found` });
+        }
+        if (phase.status !== 'pending_review' && phase.status !== 'completed') {
+          throw new TRPCError({ code: 'BAD_REQUEST', message: `Phase is not reviewable (status: ${phase.status})` });
+        }
+
+        const initiative = await initiativeRepo.findById(phase.initiativeId);
+        if (!initiative?.branch) {
+          throw new TRPCError({ code: 'BAD_REQUEST', message: 'Initiative has no branch configured' });
+        }
+
+        const initBranch = initiative.branch;
+        const phBranch = phaseBranchName(initBranch, phase.name);
+        const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch;
+
+        const decodedPath = decodeURIComponent(input.filePath);
+
+        const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId);
+
+        const firstClone = await ensureProjectClone(projects[0], ctx.workspaceRoot!);
+        const headHash = await branchManager.getHeadCommitHash(firstClone, phBranch);
+        const cacheKey = `${input.phaseId}:${headHash}:${input.filePath}`;
+        const cached = fileDiffCache.get(cacheKey);
+        if (cached) return cached;
+
+        let clonePath: string;
+        if (input.projectId) {
+          const project = projects.find((p) => p.id === input.projectId);
+          if (!project) {
+            throw new TRPCError({ code: 'NOT_FOUND', message: `Project '${input.projectId}' not found for this phase` });
+          }
+          clonePath = await ensureProjectClone(project, ctx.workspaceRoot!);
+        } else {
+          clonePath = firstClone;
+        }
+
+        const git = simpleGit(clonePath);
+        // Binary files appear as "-\t-\t<path>" in --numstat output
+        const numstatOut = await git.raw(['diff', '--numstat', `${diffBase}...${phBranch}`, '--', decodedPath]);
+        if (numstatOut.trim() && numstatOut.startsWith('-\t-\t')) {
+          const binaryResult = { binary: true, rawDiff: '' };
+          fileDiffCache.set(cacheKey, binaryResult);
+          return binaryResult;
+        }
+
+        const rawDiff = await branchManager.diffFileSingle(clonePath, diffBase, phBranch, decodedPath);
+        const result = { binary: false, rawDiff };
+        fileDiffCache.set(cacheKey, result);
+        return result;
      }),

    approvePhaseReview: publicProcedure