diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aad57323..7d335d2f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- Fixed a missing error boundary in `getFileSourceForRepo` introduced in v4.16.14: the function was extracted outside `sew()` but still re-threw unrecognised git exceptions, causing fatal Next.js task-runner errors. All error paths now return a `ServiceError`. Also tightened the error message for unresolved git refs (e.g. an unfetched `head_sha`) to distinguish them from syntactically invalid refs. [#1145](https://github.com/sourcebot-dev/sourcebot/pull/1145) + ## [4.16.14] - 2026-04-21 ### Added diff --git a/packages/web/src/features/git/getFileSourceApi.test.ts b/packages/web/src/features/git/getFileSourceApi.test.ts new file mode 100644 index 000000000..d45eafd63 --- /dev/null +++ b/packages/web/src/features/git/getFileSourceApi.test.ts @@ -0,0 +1,366 @@ +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; + +// Hoist the mock function so it can be referenced in both the vi.mock factory +// and the test body. The SUT imports simpleGit as a default export; the factory +// maps both default and named exports to the same fn so both resolve identically. +const mockSimpleGit = vi.hoisted(() => vi.fn()); + +vi.mock('simple-git', () => ({ + default: mockSimpleGit, + simpleGit: mockSimpleGit, +})); +vi.mock('@sourcebot/shared', () => ({ + getRepoPath: (repo: { id: number }) => ({ + path: `/mock/repos/${repo.id}`, + }), + env: { + AUTH_URL: 'https://sourcebot.example.com', + }, + createLogger: () => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }), +})); +vi.mock('@/lib/utils', () => ({ + getCodeHostBrowseFileAtBranchUrl: vi.fn().mockReturnValue('https://github.com/owner/repo/blob/main/src/index.ts'), + isServiceError: (obj: unknown): boolean => { + return obj !== null && typeof obj === 'object' && 'errorCode' in (obj as object); + }, +})); +vi.mock('@/app/(app)/browse/hooks/utils', () => ({ + getBrowsePath: vi.fn().mockReturnValue('/browse/github.com/owner/repo/blob/main/src/index.ts'), +})); +vi.mock('@/lib/gitattributes', () => ({ + parseGitAttributes: vi.fn().mockReturnValue({}), + resolveLanguageFromGitAttributes: vi.fn().mockReturnValue(undefined), +})); +vi.mock('@/lib/languageDetection', () => ({ + detectLanguageFromFilename: vi.fn().mockReturnValue('TypeScript'), +})); +// Required for module load; not exercised by getFileSourceForRepo directly +vi.mock('next/headers', () => ({ + headers: vi.fn().mockResolvedValue(new Headers()), +})); +vi.mock('@/middleware/sew', () => ({ + sew: async (fn: () => Promise | T): Promise => { + try { + return await fn(); + } catch (error) { + return { + errorCode: 'UNEXPECTED_ERROR', + message: error instanceof Error ? error.message : String(error), + } as T; + } + }, +})); +vi.mock('@/middleware/withAuth', () => ({ + withOptionalAuth: vi.fn(), +})); +vi.mock('@/ee/features/audit/factory', () => ({ + getAuditService: () => ({ + createAudit: vi.fn(), + }), +})); + +import { getFileSourceForRepo } from './getFileSourceApi'; + +const MOCK_ORG = { id: 1, name: 'test-org' } as Parameters[1]['org']; + +const MOCK_REPO = { + id: 123, + name: 'github.com/owner/repo', + orgId: 1, + defaultBranch: 'main', + webUrl: 'https://github.com/owner/repo', + external_codeHostType: 'GITHUB', + displayName: null, +}; + +describe('getFileSourceForRepo', () => { + const mockGitRaw = vi.fn(); + const mockCwd = vi.fn(); + const mockFindFirst = vi.fn(); + + const mockPrisma = { + repo: { findFirst: mockFindFirst }, + } as Parameters[1]['prisma']; + + beforeEach(() => { + vi.clearAllMocks(); + + mockCwd.mockReturnValue({ raw: mockGitRaw }); + mockSimpleGit.mockReturnValue({ cwd: mockCwd }); + mockFindFirst.mockResolvedValue(MOCK_REPO); + + // Default: file show succeeds; .gitattributes not present + mockGitRaw.mockImplementation(async (args: string[]) => { + if (args[1]?.endsWith('.gitattributes')) { + throw new Error('does not exist in HEAD'); + } + return 'console.log("hello");'; + }); + }); + + describe('repository validation', () => { + it('returns NOT_FOUND when repo is absent from the database', async () => { + mockFindFirst.mockResolvedValue(null); + + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ errorCode: 'NOT_FOUND' }); + }); + + it('queries the database by repo name and orgId', async () => { + await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(mockFindFirst).toHaveBeenCalledWith({ + where: { name: 'github.com/owner/repo', orgId: 1 }, + }); + }); + + it('returns UNEXPECTED_ERROR when the database throws (caught by sew)', async () => { + mockFindFirst.mockRejectedValue(new Error('DB connection refused')); + + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ errorCode: 'UNEXPECTED_ERROR' }); + }); + }); + + describe('input validation', () => { + it('returns FILE_NOT_FOUND for path traversal attempts', async () => { + const result = await getFileSourceForRepo( + { path: 'src/../../etc/passwd', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ errorCode: 'FILE_NOT_FOUND' }); + }); + + it('returns FILE_NOT_FOUND for null-byte paths', async () => { + const result = await getFileSourceForRepo( + { path: 'src/\0evil', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ errorCode: 'FILE_NOT_FOUND' }); + }); + + it('returns INVALID_GIT_REF with a syntactic message for refs starting with "-" (flag injection guard)', async () => { + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo', ref: '--upload-pack=evil' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ + errorCode: 'INVALID_GIT_REF', + message: expect.stringContaining("cannot start with '-'"), + }); + }); + }); + + describe('git error handling', () => { + it('returns FILE_NOT_FOUND when git reports the file does not exist', async () => { + mockGitRaw.mockRejectedValueOnce( + new Error("fatal: path 'src/missing.ts' does not exist in 'main'"), + ); + + const result = await getFileSourceForRepo( + { path: 'src/missing.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ errorCode: 'FILE_NOT_FOUND' }); + }); + + it('returns FILE_NOT_FOUND for "fatal: path" errors', async () => { + mockGitRaw.mockRejectedValueOnce(new Error('fatal: path not found')); + + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ errorCode: 'FILE_NOT_FOUND' }); + }); + + it('returns INVALID_GIT_REF with an unresolved-ref message when head_sha has not been fetched ("unknown revision")', async () => { + // This is the scenario from the v4.16.14 regression: the review agent passes + // pr_payload.head_sha as ref, but the bare clone hasn't fetched it yet. + mockGitRaw.mockRejectedValueOnce( + new Error("fatal: ambiguous argument 'deadbeef': unknown revision or path not in the working tree"), + ); + + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo', ref: 'deadbeef' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ + errorCode: 'INVALID_GIT_REF', + message: expect.stringContaining('could not be resolved'), + }); + }); + + it('returns INVALID_GIT_REF with an unresolved-ref message for "bad revision" errors', async () => { + mockGitRaw.mockRejectedValueOnce(new Error('fatal: bad revision')); + + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo', ref: 'nonexistent' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ + errorCode: 'INVALID_GIT_REF', + message: expect.stringContaining('could not be resolved'), + }); + }); + + it('returns INVALID_GIT_REF with an unresolved-ref message for "invalid object name" errors', async () => { + mockGitRaw.mockRejectedValueOnce(new Error('fatal: invalid object name HEAD')); + + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ + errorCode: 'INVALID_GIT_REF', + message: expect.stringContaining('could not be resolved'), + }); + }); + + it('returns UNEXPECTED_ERROR — not throw — for unrecognised git errors (regression: v4.16.14 fatal exception)', async () => { + // Before the fix, getFileSourceForRepo re-threw unknown errors. + // Outside sew(), this caused a fatal Next.js task-runner exception. + // After the fix, all errors are returned as ServiceError. + mockGitRaw.mockRejectedValueOnce(new Error('I/O error: device busy')); + + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ errorCode: 'UNEXPECTED_ERROR' }); + }); + + it('never rejects its returned promise for unrecognised git errors', async () => { + mockGitRaw.mockRejectedValueOnce(new Error('transient I/O error')); + + await expect( + getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ), + ).resolves.toMatchObject({ errorCode: 'UNEXPECTED_ERROR' }); + }); + }); + + describe('successful response', () => { + it('returns the file source with language detected from filename', async () => { + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ + source: 'console.log("hello");', + language: 'TypeScript', + path: 'src/index.ts', + repo: 'github.com/owner/repo', + }); + }); + + it('uses the provided ref for the git show command', async () => { + await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo', ref: 'abc123sha' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(mockGitRaw).toHaveBeenCalledWith(['show', 'abc123sha:src/index.ts']); + }); + + it('falls back to defaultBranch when ref is omitted', async () => { + await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(mockGitRaw).toHaveBeenCalledWith(['show', 'main:src/index.ts']); + }); + + it('falls back to HEAD when both ref and defaultBranch are absent', async () => { + mockFindFirst.mockResolvedValue({ ...MOCK_REPO, defaultBranch: null }); + + await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(mockGitRaw).toHaveBeenCalledWith(['show', 'HEAD:src/index.ts']); + }); + + it('uses the repo path from getRepoPath for the git working directory', async () => { + await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + // getRepoPath mock returns `/mock/repos/${repo.id}` + expect(mockCwd).toHaveBeenCalledWith('/mock/repos/123'); + }); + }); + + describe('language detection', () => { + it('uses language from .gitattributes when present', async () => { + const { resolveLanguageFromGitAttributes, parseGitAttributes } = await import('@/lib/gitattributes'); + const mockResolve = resolveLanguageFromGitAttributes as unknown as Mock; + const mockParse = parseGitAttributes as unknown as Mock; + + mockParse.mockReturnValue({ '*.ts': { linguist_language: 'TypeScript' } }); + mockResolve.mockReturnValue('TypeScript'); + + // Override default: .gitattributes call succeeds + mockGitRaw.mockImplementation(async (args: string[]) => { + if (args[1]?.endsWith('.gitattributes')) { + return 'linguist-language=TypeScript'; + } + return 'file content'; + }); + + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ language: 'TypeScript' }); + expect(mockResolve).toHaveBeenCalled(); + }); + + it('falls back to filename-based detection when .gitattributes is absent', async () => { + const { detectLanguageFromFilename } = await import('@/lib/languageDetection'); + const mockDetect = detectLanguageFromFilename as unknown as Mock; + mockDetect.mockReturnValue('TypeScript'); + + // Default beforeEach setup: .gitattributes throws → falls back to filename detection + const result = await getFileSourceForRepo( + { path: 'src/index.ts', repo: 'github.com/owner/repo' }, + { org: MOCK_ORG, prisma: mockPrisma }, + ); + + expect(result).toMatchObject({ language: 'TypeScript' }); + expect(mockDetect).toHaveBeenCalledWith('src/index.ts'); + }); + }); +}); diff --git a/packages/web/src/features/git/getFileSourceApi.ts b/packages/web/src/features/git/getFileSourceApi.ts index 2ba2b57cb..df22896ab 100644 --- a/packages/web/src/features/git/getFileSourceApi.ts +++ b/packages/web/src/features/git/getFileSourceApi.ts @@ -3,7 +3,7 @@ import { getBrowsePath } from '@/app/(app)/browse/hooks/utils'; import { getAuditService } from '@/ee/features/audit/factory'; import { parseGitAttributes, resolveLanguageFromGitAttributes } from '@/lib/gitattributes'; import { detectLanguageFromFilename } from '@/lib/languageDetection'; -import { ServiceError, notFound, fileNotFound, invalidGitRef, unexpectedError } from '@/lib/serviceError'; +import { ServiceError, notFound, fileNotFound, invalidGitRef, unresolvedGitRef, unexpectedError } from '@/lib/serviceError'; import { getCodeHostBrowseFileAtBranchUrl } from '@/lib/utils'; import { withOptionalAuth } from '@/middleware/withAuth'; import { env, getRepoPath } from '@sourcebot/shared'; @@ -26,7 +26,7 @@ export type FileSourceResponse = z.infer; export const getFileSourceForRepo = async ( { path: filePath, repo: repoName, ref }: FileSourceRequest, { org, prisma }: { org: Org; prisma: PrismaClient }, -): Promise => { +): Promise => sew(async () => { const repo = await prisma.repo.findFirst({ where: { name: repoName, orgId: org.id }, }); @@ -56,9 +56,9 @@ export const getFileSourceForRepo = async ( return fileNotFound(filePath, repoName); } if (errorMessage.includes('unknown revision') || errorMessage.includes('bad revision') || errorMessage.includes('invalid object name')) { - return unexpectedError(`Invalid git reference: ${gitRef}`); + return unresolvedGitRef(gitRef); } - throw error; + return unexpectedError(errorMessage); } let gitattributesContent: string | undefined; @@ -98,7 +98,7 @@ export const getFileSourceForRepo = async ( webUrl, externalWebUrl, } satisfies FileSourceResponse; -}; +}); export const getFileSource = async ({ path: filePath, repo: repoName, ref }: FileSourceRequest, { source }: { source?: string } = {}): Promise => sew(() => withOptionalAuth(async ({ org, prisma, user }) => { if (user) { diff --git a/packages/web/src/lib/serviceError.ts b/packages/web/src/lib/serviceError.ts index f874cdb7b..a96969824 100644 --- a/packages/web/src/lib/serviceError.ts +++ b/packages/web/src/lib/serviceError.ts @@ -109,3 +109,11 @@ export const invalidGitRef = (ref: string): ServiceError => { }; } +export const unresolvedGitRef = (ref: string): ServiceError => { + return { + statusCode: StatusCodes.BAD_REQUEST, + errorCode: ErrorCode.INVALID_GIT_REF, + message: `Git reference "${ref}" could not be resolved.`, + }; +} +