diff --git a/apps/server/git/simple-git-branch-manager.ts b/apps/server/git/simple-git-branch-manager.ts index 74a1e83..bfe969d 100644 --- a/apps/server/git/simple-git-branch-manager.ts +++ b/apps/server/git/simple-git-branch-manager.ts @@ -202,6 +202,12 @@ export class SimpleGitBranchManager implements BranchManager { return git.diff([`${baseBranch}...${headBranch}`, '--', filePath]); } + async getHeadCommitHash(repoPath: string, branch: string): Promise { + const git = simpleGit(repoPath); + const result = await git.raw(['rev-parse', branch]); + return result.trim(); + } + async deleteBranch(repoPath: string, branch: string): Promise { const git = simpleGit(repoPath); const exists = await this.branchExists(repoPath, branch); @@ -267,12 +273,6 @@ export class SimpleGitBranchManager implements BranchManager { return result.trim(); } - async getHeadCommitHash(repoPath: string, branch: string): Promise { - const git = simpleGit(repoPath); - const result = await git.raw(['rev-parse', branch]); - return result.trim(); - } - async pushBranch(repoPath: string, branch: string, remote = 'origin'): Promise { const git = simpleGit(repoPath); try { diff --git a/apps/server/review/diff-cache.test.ts b/apps/server/review/diff-cache.test.ts new file mode 100644 index 0000000..ddb1622 --- /dev/null +++ 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(5000); + expect(cache.get('nonexistent')).toBeUndefined(); + }); + + it('returns value when entry has not expired', () => { + vi.spyOn(Date, 'now').mockReturnValue(1000); + const cache = new DiffCache(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(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(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(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(); + }); +}); diff --git a/apps/server/review/diff-cache.ts b/apps/server/review/diff-cache.ts index 312734d..44cb1ec 100644 --- a/apps/server/review/diff-cache.ts +++ b/apps/server/review/diff-cache.ts @@ -1,6 +1,9 @@ /** - * DiffCache — in-memory cache with TTL and prefix-based invalidation. - * Used to avoid re-running expensive git diff subprocesses on repeated requests. + * 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'; diff --git a/apps/server/trpc/routers/phase.test.ts b/apps/server/trpc/routers/phase.test.ts new file mode 100644 index 0000000..ec021bd --- /dev/null +++ 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'); + }); +}); diff --git a/apps/server/trpc/routers/phase.ts b/apps/server/trpc/routers/phase.ts index 8e84beb..9381722 100644 --- a/apps/server/trpc/routers/phase.ts +++ b/apps/server/trpc/routers/phase.ts @@ -236,18 +236,28 @@ export function phaseProcedures(publicProcedure: ProcedureBuilder) { const diffBase = (phase.status === 'completed' && phase.mergeBase) ? phase.mergeBase : initBranch; const projects = await projectRepo.findProjectsByInitiativeId(phase.initiativeId); - const files: FileStatEntry[] = []; if (projects.length === 0) { - return { phaseName: phase.name, sourceBranch: phBranch, targetBranch: initBranch, files: [], totalAdditions: 0, totalDeletions: 0 }; + return { + phaseName: phase.name, + sourceBranch: phBranch, + targetBranch: initBranch, + files: [], + totalAdditions: 0, + totalDeletions: 0, + }; } const firstClone = await ensureProjectClone(projects[0], ctx.workspaceRoot!); const headHash = await branchManager.getHeadCommitHash(firstClone, phBranch); const cacheKey = `${input.phaseId}:${headHash}`; - const cached = phaseMetaCache.get(cacheKey); + + type PhaseReviewDiffResult = { phaseName: string; sourceBranch: string; targetBranch: string; files: FileStatEntry[]; totalAdditions: number; totalDeletions: number }; + const cached = phaseMetaCache.get(cacheKey) as PhaseReviewDiffResult | undefined; if (cached) return cached; + const files: FileStatEntry[] = []; + for (const project of projects) { const clonePath = await ensureProjectClone(project, ctx.workspaceRoot!); const entries = await branchManager.diffBranchesStat(clonePath, diffBase, phBranch); diff --git a/docs/server-api.md b/docs/server-api.md index eb5e5ef..887bccd 100644 --- a/docs/server-api.md +++ b/docs/server-api.md @@ -118,7 +118,7 @@ Each procedure uses `require*Repository(ctx)` helpers that throw `TRPCError(INTE | listInitiativePhaseDependencies | query | All dependency edges | | getPhaseDependencies | query | What this phase depends on | | getPhaseDependents | query | What depends on this phase | -| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content | +| getPhaseReviewDiff | query | File-level metadata for pending_review phase: `{phaseName, sourceBranch, targetBranch, files: FileStatEntry[], totalAdditions, totalDeletions}` — no hunk content. Results are cached in-memory by `phaseId:headHash` (TTL: `REVIEW_DIFF_CACHE_TTL_MS`, default 5 min). Cache is invalidated when a task merges into the phase branch. | | getFileDiff | query | Per-file unified diff on demand: `{phaseId, filePath, projectId?}` → `{binary: boolean, rawDiff: string}`; `filePath` must be URL-encoded; binary files return `{binary: true, rawDiff: ''}` | | getPhaseReviewCommits | query | List commits between initiative and phase branch | | getCommitDiff | query | Diff for a single commit (by hash) in a phase |