From 9894cdd06f066e00a96fbb9e1124a6fb7d867bc9 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:33:47 +0100 Subject: [PATCH 1/3] feat: add diffBranchesStat and diffFileSingle to BranchManager Adds FileStatEntry type and two new primitives to the BranchManager port and SimpleGitBranchManager adapter, enabling split diff procedures in the tRPC layer without returning raw multi-megabyte diffs. - FileStatEntry captures path, status, additions/deletions, oldPath (renames), and optional projectId for multi-project routing - diffBranchesStat uses --name-status + --numstat, detects binary files (shown as - / - in numstat), handles spaces in filenames - diffFileSingle returns raw unified diff for a single file path --- apps/server/execution/orchestrator.test.ts | 2 + apps/server/git/branch-manager.ts | 18 ++- .../git/simple-git-branch-manager.test.ts | 140 ++++++++++++++++++ apps/server/git/simple-git-branch-manager.ts | 107 ++++++++++++- apps/server/git/types.ts | 23 +++ docs/git-process-logging.md | 2 + 6 files changed, 290 insertions(+), 2 deletions(-) diff --git a/apps/server/execution/orchestrator.test.ts b/apps/server/execution/orchestrator.test.ts index 6cf293d..ebf6ea2 100644 --- a/apps/server/execution/orchestrator.test.ts +++ b/apps/server/execution/orchestrator.test.ts @@ -46,6 +46,8 @@ 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(''), deleteBranch: vi.fn(), branchExists: vi.fn().mockResolvedValue(true), remoteBranchExists: vi.fn().mockResolvedValue(false), diff --git a/apps/server/git/branch-manager.ts b/apps/server/git/branch-manager.ts index 9ba6d85..dc7d174 100644 --- 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,22 @@ export interface BranchManager { */ diffBranches(repoPath: string, baseBranch: string, headBranch: string): Promise; + /** + * 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; + + /** + * 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; + /** * Delete a branch. No-op if the branch doesn't exist. */ diff --git a/apps/server/git/simple-git-branch-manager.test.ts b/apps/server/git/simple-git-branch-manager.test.ts index 18cb50a..f59c65b 100644 --- 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; +}> { + 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; @@ -108,3 +171,80 @@ describe('SimpleGitBranchManager', () => { }); }); }); + +describe('SimpleGitBranchManager - diffBranchesStat and diffFileSingle', () => { + let clonePath: string; + let cleanup: () => Promise; + 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'); + }); + }); +}); diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index 47b690e..7e96848 100644 --- 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 { const git = simpleGit(repoPath); @@ -97,6 +117,91 @@ export class SimpleGitBranchManager implements BranchManager { return diff; } + async diffBranchesStat(repoPath: string, baseBranch: string, headBranch: string): Promise { + 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: "\t\t" + // Binary files: "-\t-\t" + const numStatMap = new Map(); + 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: "\t" or "\t\t" + 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 { + const git = simpleGit(repoPath); + return git.diff([`${baseBranch}...${headBranch}`, '--', filePath]); + } + async deleteBranch(repoPath: string, branch: string): Promise { const git = simpleGit(repoPath); const exists = await this.branchExists(repoPath, branch); diff --git a/apps/server/git/types.ts b/apps/server/git/types.ts index 51a35b7..00cb829 100644 --- 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 // ============================================================================= diff --git a/docs/git-process-logging.md b/docs/git-process-logging.md index 2e5d8c4..16968af 100644 --- a/docs/git-process-logging.md +++ b/docs/git-process-logging.md @@ -40,6 +40,8 @@ Worktrees stored in `.cw-worktrees/` subdirectory of the repo. Each agent gets a | `ensureBranch(repoPath, branch, baseBranch)` | Create branch from base if it doesn't exist (idempotent) | | `mergeBranch(repoPath, source, target)` | Merge via ephemeral worktree, returns conflict info | | `diffBranches(repoPath, base, head)` | Three-dot diff between branches | +| `diffBranchesStat(repoPath, base, head)` | Per-file metadata (path, status, additions, deletions) — no hunk content. Binary files included with `status: 'binary'` and counts of 0. Returns `FileStatEntry[]`. | +| `diffFileSingle(repoPath, base, head, filePath)` | Raw unified diff for a single file (three-dot diff). `filePath` must be URL-decoded. Returns empty string for binary files. | | `deleteBranch(repoPath, branch)` | Delete local branch (no-op if missing) | | `branchExists(repoPath, branch)` | Check local branches | | `remoteBranchExists(repoPath, branch)` | Check remote tracking branches (`origin/`) | From 05eb1607493bf3b3825ee67ac72154d401c1f0cb Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:33:47 +0100 Subject: [PATCH 2/3] feat: add diffBranchesStat and diffFileSingle to BranchManager Adds FileStatEntry type and two new primitives to the BranchManager port and SimpleGitBranchManager adapter, enabling split diff procedures in the tRPC layer without returning raw multi-megabyte diffs. - FileStatEntry captures path, status, additions/deletions, oldPath (renames), and optional projectId for multi-project routing - diffBranchesStat uses --name-status + --numstat, detects binary files (shown as - / - in numstat), handles spaces in filenames - diffFileSingle returns raw unified diff for a single file path --- apps/server/execution/orchestrator.test.ts | 2 + apps/server/git/branch-manager.ts | 18 ++- .../git/simple-git-branch-manager.test.ts | 140 ++++++++++++++++++ apps/server/git/simple-git-branch-manager.ts | 107 ++++++++++++- apps/server/git/types.ts | 23 +++ docs/git-process-logging.md | 2 + 6 files changed, 290 insertions(+), 2 deletions(-) diff --git a/apps/server/execution/orchestrator.test.ts b/apps/server/execution/orchestrator.test.ts index 6cf293d..ebf6ea2 100644 --- a/apps/server/execution/orchestrator.test.ts +++ b/apps/server/execution/orchestrator.test.ts @@ -46,6 +46,8 @@ 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(''), deleteBranch: vi.fn(), branchExists: vi.fn().mockResolvedValue(true), remoteBranchExists: vi.fn().mockResolvedValue(false), diff --git a/apps/server/git/branch-manager.ts b/apps/server/git/branch-manager.ts index 9ba6d85..dc7d174 100644 --- 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,22 @@ export interface BranchManager { */ diffBranches(repoPath: string, baseBranch: string, headBranch: string): Promise; + /** + * 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; + + /** + * 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; + /** * Delete a branch. No-op if the branch doesn't exist. */ diff --git a/apps/server/git/simple-git-branch-manager.test.ts b/apps/server/git/simple-git-branch-manager.test.ts index 18cb50a..f59c65b 100644 --- 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; +}> { + 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; @@ -108,3 +171,80 @@ describe('SimpleGitBranchManager', () => { }); }); }); + +describe('SimpleGitBranchManager - diffBranchesStat and diffFileSingle', () => { + let clonePath: string; + let cleanup: () => Promise; + 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'); + }); + }); +}); diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index 47b690e..7e96848 100644 --- 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 { const git = simpleGit(repoPath); @@ -97,6 +117,91 @@ export class SimpleGitBranchManager implements BranchManager { return diff; } + async diffBranchesStat(repoPath: string, baseBranch: string, headBranch: string): Promise { + 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: "\t\t" + // Binary files: "-\t-\t" + const numStatMap = new Map(); + 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: "\t" or "\t\t" + 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 { + const git = simpleGit(repoPath); + return git.diff([`${baseBranch}...${headBranch}`, '--', filePath]); + } + async deleteBranch(repoPath: string, branch: string): Promise { const git = simpleGit(repoPath); const exists = await this.branchExists(repoPath, branch); diff --git a/apps/server/git/types.ts b/apps/server/git/types.ts index 51a35b7..00cb829 100644 --- 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 // ============================================================================= diff --git a/docs/git-process-logging.md b/docs/git-process-logging.md index 2e5d8c4..16968af 100644 --- a/docs/git-process-logging.md +++ b/docs/git-process-logging.md @@ -40,6 +40,8 @@ Worktrees stored in `.cw-worktrees/` subdirectory of the repo. Each agent gets a | `ensureBranch(repoPath, branch, baseBranch)` | Create branch from base if it doesn't exist (idempotent) | | `mergeBranch(repoPath, source, target)` | Merge via ephemeral worktree, returns conflict info | | `diffBranches(repoPath, base, head)` | Three-dot diff between branches | +| `diffBranchesStat(repoPath, base, head)` | Per-file metadata (path, status, additions, deletions) — no hunk content. Binary files included with `status: 'binary'` and counts of 0. Returns `FileStatEntry[]`. | +| `diffFileSingle(repoPath, base, head, filePath)` | Raw unified diff for a single file (three-dot diff). `filePath` must be URL-decoded. Returns empty string for binary files. | | `deleteBranch(repoPath, branch)` | Delete local branch (no-op if missing) | | `branchExists(repoPath, branch)` | Check local branches | | `remoteBranchExists(repoPath, branch)` | Check remote tracking branches (`origin/`) | From 4890721a92d5a87f0eb986cdacd8cf17be3e0829 Mon Sep 17 00:00:00 2001 From: Lukas May Date: Fri, 6 Mar 2026 19:45:57 +0100 Subject: [PATCH 3/3] feat: split getPhaseReviewDiff into metadata + add getFileDiff procedure Rewrites getPhaseReviewDiff to return file-level metadata (path, status, additions, deletions) instead of a raw diff string, eliminating 10MB+ payloads for large repos. Adds getFileDiff for on-demand per-file hunk content with binary detection via numstat. Multi-project initiatives prefix file paths with the project name to avoid collisions. Adds integration tests that use real local git repos + in-memory SQLite to verify both procedures end-to-end (binary files, deleted files, spaces in paths, error cases). Co-Authored-By: Claude Sonnet 4.6 --- .../integration/phase-review-diff.test.ts | 256 ++++++++++++++++++ apps/server/trpc/routers/phase.ts | 79 +++++- docs/server-api.md | 3 +- 3 files changed, 331 insertions(+), 7 deletions(-) create mode 100644 apps/server/test/integration/phase-review-diff.test.ts diff --git a/apps/server/test/integration/phase-review-diff.test.ts b/apps/server/test/integration/phase-review-diff.test.ts new file mode 100644 index 0000000..036e12d --- /dev/null +++ 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; +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 { + 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 = { + 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'); + }); +}); diff --git a/apps/server/trpc/routers/phase.ts b/apps/server/trpc/routers/phase.ts index be59ef2..0d9c999 100644 --- a/apps/server/trpc/routers/phase.ts +++ b/apps/server/trpc/routers/phase.ts @@ -4,11 +4,13 @@ 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'; export function phaseProcedures(publicProcedure: ProcedureBuilder) { return { @@ -230,28 +232,93 @@ 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 = ''; + 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); } } + const totalAdditions = files.reduce((sum, f) => sum + f.additions, 0); + const totalDeletions = files.reduce((sum, f) => sum + f.deletions, 0); + return { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, - rawDiff, + files, + totalAdditions, + totalDeletions, }; }), + 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); + 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 = await ensureProjectClone(projects[0], ctx.workspaceRoot!); + } + + const git = simpleGit(clonePath); + // Binary files appear as "-\t-\t" in --numstat output + const numstatOut = await git.raw(['diff', '--numstat', `${diffBase}...${phBranch}`, '--', decodedPath]); + if (numstatOut.trim() && numstatOut.startsWith('-\t-\t')) { + return { binary: true, rawDiff: '' }; + } + + const rawDiff = await branchManager.diffFileSingle(clonePath, diffBase, phBranch, decodedPath); + return { binary: false, rawDiff }; + }), + approvePhaseReview: publicProcedure .input(z.object({ phaseId: z.string().min(1) })) .mutation(async ({ ctx, input }) => { diff --git a/docs/server-api.md b/docs/server-api.md index b064576..eb5e5ef 100644 --- a/docs/server-api.md +++ b/docs/server-api.md @@ -118,7 +118,8 @@ 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 | Full branch diff for pending_review phase | +| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content | +| 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 | | approvePhaseReview | mutation | Approve and merge phase branch |