Skip to content

Commit 41f978f

Browse files
committed
fix(web): restore ServiceError boundary in getFileSourceForRepo
## Problem In v4.16.14 (PR #1104, GitLab MR Review Agent), the core file-fetch logic was extracted from inside `sew()` into a standalone `getFileSourceForRepo` function so it could be called by privileged server-side callers (e.g. the review agent webhook handler) without going through the auth middleware. The extraction introduced a missing error boundary. The catch block inside `getFileSourceForRepo` handled two known git error patterns and **re-threw everything else**: ```ts // getFileSourceApi.ts — before this fix } catch (error: unknown) { const errorMessage = error instanceof Error ? error.message : String(error); if (errorMessage.includes('does not exist') || ...) return fileNotFound(...); if (errorMessage.includes('unknown revision') || ...) return unexpectedError(...); throw error; // ← propagates uncaught; sew() no longer wraps this code path } ``` Because `getFileSourceForRepo` is no longer inside `sew()`, any re-thrown exception escapes as a fatal error through the Next.js server task runner, producing the `z.onFatalException` / `z.attemptTask` stack trace seen in production on v4.16.14. A second contributing factor: the review agent was also changed in v4.16.14 to pass `ref: pr_payload.head_sha` where `ref` was previously `undefined`. If the bare clone hasn't fetched that commit yet, `git show` throws with `"unknown revision"` — which fell into the same unhandled re-throw path. Rolling back to v4.16.13 restored the original `sew()`-wrapped code path, which is why the rollback fixed the issue. ## Fix Two targeted changes in `getFileSourceApi.ts`: 1. **`throw error` → `return unexpectedError(errorMessage)`** — ensures `getFileSourceForRepo` always returns `FileSourceResponse | ServiceError` and never rejects its promise. This is the root-cause fix. 2. **`unexpectedError(...)` → `invalidGitRef(gitRef)`** for the `"unknown revision"` / `"bad revision"` / `"invalid object name"` branch — uses the semantically correct error type (already imported), which also gives callers a more actionable error when `head_sha` hasn't been fetched. ## Tests Added `getFileSourceApi.test.ts` with 19 tests covering: - **Repository validation** — `NOT_FOUND` when repo is absent from the DB; correct `findFirst` query shape. - **Input validation** — path traversal and null-byte paths → `FILE_NOT_FOUND`; refs starting with `-` → `INVALID_GIT_REF` (flag-injection guard). - **Git error handling** (the regression suite): - `"does not exist"` / `"fatal: path"` → `FILE_NOT_FOUND` - `"unknown revision"` (unfetched `head_sha`) → `INVALID_GIT_REF` - `"bad revision"` / `"invalid object name"` → `INVALID_GIT_REF` - **Unrecognised error → `UNEXPECTED_ERROR`, not a throw** — explicit regression test; before the fix, `.resolves` would fail because the promise rejected. - **Successful response** — source content, language, ref fallback chain (`ref` → `defaultBranch` → `HEAD`), correct `cwd` path. - **Language detection** — prefers `.gitattributes`; falls back to filename-based detection when the file is absent. ## Files changed | File | Change | |------|--------| | `packages/web/src/features/git/getFileSourceApi.ts` | `throw error` → `return unexpectedError(errorMessage)`; invalid-ref branch now returns `invalidGitRef(gitRef)` | | `packages/web/src/features/git/getFileSourceApi.test.ts` | New — 19 tests |
1 parent d111397 commit 41f978f

2 files changed

Lines changed: 331 additions & 2 deletions

File tree

Lines changed: 329 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,329 @@
1+
import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest';
2+
3+
// Mocks must be declared before imports
4+
vi.mock('simple-git');
5+
vi.mock('@sourcebot/shared', () => ({
6+
getRepoPath: (repo: { id: number }) => ({
7+
path: `/mock/repos/${repo.id}`,
8+
}),
9+
env: {
10+
AUTH_URL: 'https://sourcebot.example.com',
11+
},
12+
createLogger: () => ({
13+
debug: vi.fn(),
14+
info: vi.fn(),
15+
warn: vi.fn(),
16+
error: vi.fn(),
17+
}),
18+
}));
19+
vi.mock('@/lib/utils', () => ({
20+
getCodeHostBrowseFileAtBranchUrl: vi.fn().mockReturnValue('https://github.com/owner/repo/blob/main/src/index.ts'),
21+
isServiceError: (obj: unknown): boolean => {
22+
return obj !== null && typeof obj === 'object' && 'errorCode' in (obj as object);
23+
},
24+
}));
25+
vi.mock('@/app/(app)/browse/hooks/utils', () => ({
26+
getBrowsePath: vi.fn().mockReturnValue('/browse/github.com/owner/repo/blob/main/src/index.ts'),
27+
}));
28+
vi.mock('@/lib/gitattributes', () => ({
29+
parseGitAttributes: vi.fn().mockReturnValue({}),
30+
resolveLanguageFromGitAttributes: vi.fn().mockReturnValue(undefined),
31+
}));
32+
vi.mock('@/lib/languageDetection', () => ({
33+
detectLanguageFromFilename: vi.fn().mockReturnValue('TypeScript'),
34+
}));
35+
// Required for module load; not exercised by getFileSourceForRepo directly
36+
vi.mock('next/headers', () => ({
37+
headers: vi.fn().mockResolvedValue(new Headers()),
38+
}));
39+
vi.mock('@/middleware/sew', () => ({
40+
sew: async <T>(fn: () => Promise<T> | T): Promise<T> => fn(),
41+
}));
42+
vi.mock('@/middleware/withAuth', () => ({
43+
withOptionalAuth: vi.fn(),
44+
}));
45+
vi.mock('@/ee/features/audit/factory', () => ({
46+
getAuditService: () => ({
47+
createAudit: vi.fn(),
48+
}),
49+
}));
50+
51+
import { simpleGit } from 'simple-git';
52+
import { getFileSourceForRepo } from './getFileSourceApi';
53+
54+
const MOCK_ORG = { id: 1, name: 'test-org' } as Parameters<typeof getFileSourceForRepo>[1]['org'];
55+
56+
const MOCK_REPO = {
57+
id: 123,
58+
name: 'github.com/owner/repo',
59+
orgId: 1,
60+
defaultBranch: 'main',
61+
webUrl: 'https://github.com/owner/repo',
62+
external_codeHostType: 'GITHUB',
63+
displayName: null,
64+
};
65+
66+
describe('getFileSourceForRepo', () => {
67+
const mockGitRaw = vi.fn();
68+
const mockCwd = vi.fn();
69+
const mockSimpleGit = simpleGit as unknown as Mock;
70+
const mockFindFirst = vi.fn();
71+
72+
const mockPrisma = {
73+
repo: { findFirst: mockFindFirst },
74+
} as Parameters<typeof getFileSourceForRepo>[1]['prisma'];
75+
76+
beforeEach(() => {
77+
vi.clearAllMocks();
78+
79+
mockCwd.mockReturnValue({ raw: mockGitRaw });
80+
mockSimpleGit.mockReturnValue({ cwd: mockCwd });
81+
mockFindFirst.mockResolvedValue(MOCK_REPO);
82+
83+
// Default: file show succeeds; .gitattributes not present
84+
mockGitRaw.mockImplementation(async (args: string[]) => {
85+
if (args[1]?.endsWith('.gitattributes')) {
86+
throw new Error('does not exist in HEAD');
87+
}
88+
return 'console.log("hello");';
89+
});
90+
});
91+
92+
describe('repository validation', () => {
93+
it('returns NOT_FOUND when repo is absent from the database', async () => {
94+
mockFindFirst.mockResolvedValue(null);
95+
96+
const result = await getFileSourceForRepo(
97+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
98+
{ org: MOCK_ORG, prisma: mockPrisma },
99+
);
100+
101+
expect(result).toMatchObject({ errorCode: 'NOT_FOUND' });
102+
});
103+
104+
it('queries the database by repo name and orgId', async () => {
105+
await getFileSourceForRepo(
106+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
107+
{ org: MOCK_ORG, prisma: mockPrisma },
108+
);
109+
110+
expect(mockFindFirst).toHaveBeenCalledWith({
111+
where: { name: 'github.com/owner/repo', orgId: 1 },
112+
});
113+
});
114+
});
115+
116+
describe('input validation', () => {
117+
it('returns FILE_NOT_FOUND for path traversal attempts', async () => {
118+
const result = await getFileSourceForRepo(
119+
{ path: 'src/../../etc/passwd', repo: 'github.com/owner/repo' },
120+
{ org: MOCK_ORG, prisma: mockPrisma },
121+
);
122+
123+
expect(result).toMatchObject({ errorCode: 'FILE_NOT_FOUND' });
124+
});
125+
126+
it('returns FILE_NOT_FOUND for null-byte paths', async () => {
127+
const result = await getFileSourceForRepo(
128+
{ path: 'src/\0evil', repo: 'github.com/owner/repo' },
129+
{ org: MOCK_ORG, prisma: mockPrisma },
130+
);
131+
132+
expect(result).toMatchObject({ errorCode: 'FILE_NOT_FOUND' });
133+
});
134+
135+
it('returns INVALID_GIT_REF for refs starting with "-" (flag injection guard)', async () => {
136+
const result = await getFileSourceForRepo(
137+
{ path: 'src/index.ts', repo: 'github.com/owner/repo', ref: '--upload-pack=evil' },
138+
{ org: MOCK_ORG, prisma: mockPrisma },
139+
);
140+
141+
expect(result).toMatchObject({ errorCode: 'INVALID_GIT_REF' });
142+
});
143+
});
144+
145+
describe('git error handling', () => {
146+
it('returns FILE_NOT_FOUND when git reports the file does not exist', async () => {
147+
mockGitRaw.mockRejectedValueOnce(
148+
new Error("fatal: path 'src/missing.ts' does not exist in 'main'"),
149+
);
150+
151+
const result = await getFileSourceForRepo(
152+
{ path: 'src/missing.ts', repo: 'github.com/owner/repo' },
153+
{ org: MOCK_ORG, prisma: mockPrisma },
154+
);
155+
156+
expect(result).toMatchObject({ errorCode: 'FILE_NOT_FOUND' });
157+
});
158+
159+
it('returns FILE_NOT_FOUND for "fatal: path" errors', async () => {
160+
mockGitRaw.mockRejectedValueOnce(new Error('fatal: path not found'));
161+
162+
const result = await getFileSourceForRepo(
163+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
164+
{ org: MOCK_ORG, prisma: mockPrisma },
165+
);
166+
167+
expect(result).toMatchObject({ errorCode: 'FILE_NOT_FOUND' });
168+
});
169+
170+
it('returns INVALID_GIT_REF when head_sha has not been fetched on the local clone ("unknown revision")', async () => {
171+
// This is the scenario from the v4.16.14 regression: the review agent passes
172+
// pr_payload.head_sha as ref, but the bare clone hasn't fetched it yet.
173+
mockGitRaw.mockRejectedValueOnce(
174+
new Error("fatal: ambiguous argument 'deadbeef': unknown revision or path not in the working tree"),
175+
);
176+
177+
const result = await getFileSourceForRepo(
178+
{ path: 'src/index.ts', repo: 'github.com/owner/repo', ref: 'deadbeef' },
179+
{ org: MOCK_ORG, prisma: mockPrisma },
180+
);
181+
182+
expect(result).toMatchObject({ errorCode: 'INVALID_GIT_REF' });
183+
});
184+
185+
it('returns INVALID_GIT_REF for "bad revision" errors', async () => {
186+
mockGitRaw.mockRejectedValueOnce(new Error('fatal: bad revision'));
187+
188+
const result = await getFileSourceForRepo(
189+
{ path: 'src/index.ts', repo: 'github.com/owner/repo', ref: 'nonexistent' },
190+
{ org: MOCK_ORG, prisma: mockPrisma },
191+
);
192+
193+
expect(result).toMatchObject({ errorCode: 'INVALID_GIT_REF' });
194+
});
195+
196+
it('returns INVALID_GIT_REF for "invalid object name" errors', async () => {
197+
mockGitRaw.mockRejectedValueOnce(new Error('fatal: invalid object name HEAD'));
198+
199+
const result = await getFileSourceForRepo(
200+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
201+
{ org: MOCK_ORG, prisma: mockPrisma },
202+
);
203+
204+
expect(result).toMatchObject({ errorCode: 'INVALID_GIT_REF' });
205+
});
206+
207+
it('returns UNEXPECTED_ERROR — not throw — for unrecognised git errors (regression: v4.16.14 fatal exception)', async () => {
208+
// Before the fix, getFileSourceForRepo re-threw unknown errors.
209+
// Outside sew(), this caused a fatal Next.js task-runner exception.
210+
// After the fix, all errors are returned as ServiceError.
211+
mockGitRaw.mockRejectedValueOnce(new Error('I/O error: device busy'));
212+
213+
const result = await getFileSourceForRepo(
214+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
215+
{ org: MOCK_ORG, prisma: mockPrisma },
216+
);
217+
218+
expect(result).toMatchObject({ errorCode: 'UNEXPECTED_ERROR' });
219+
});
220+
221+
it('never rejects its returned promise for unrecognised git errors', async () => {
222+
mockGitRaw.mockRejectedValueOnce(new Error('transient I/O error'));
223+
224+
await expect(
225+
getFileSourceForRepo(
226+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
227+
{ org: MOCK_ORG, prisma: mockPrisma },
228+
),
229+
).resolves.toMatchObject({ errorCode: 'UNEXPECTED_ERROR' });
230+
});
231+
});
232+
233+
describe('successful response', () => {
234+
it('returns the file source with language detected from filename', async () => {
235+
const result = await getFileSourceForRepo(
236+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
237+
{ org: MOCK_ORG, prisma: mockPrisma },
238+
);
239+
240+
expect(result).toMatchObject({
241+
source: 'console.log("hello");',
242+
language: 'TypeScript',
243+
path: 'src/index.ts',
244+
repo: 'github.com/owner/repo',
245+
});
246+
});
247+
248+
it('uses the provided ref for the git show command', async () => {
249+
await getFileSourceForRepo(
250+
{ path: 'src/index.ts', repo: 'github.com/owner/repo', ref: 'abc123sha' },
251+
{ org: MOCK_ORG, prisma: mockPrisma },
252+
);
253+
254+
expect(mockGitRaw).toHaveBeenCalledWith(['show', 'abc123sha:src/index.ts']);
255+
});
256+
257+
it('falls back to defaultBranch when ref is omitted', async () => {
258+
await getFileSourceForRepo(
259+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
260+
{ org: MOCK_ORG, prisma: mockPrisma },
261+
);
262+
263+
expect(mockGitRaw).toHaveBeenCalledWith(['show', 'main:src/index.ts']);
264+
});
265+
266+
it('falls back to HEAD when both ref and defaultBranch are absent', async () => {
267+
mockFindFirst.mockResolvedValue({ ...MOCK_REPO, defaultBranch: null });
268+
269+
await getFileSourceForRepo(
270+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
271+
{ org: MOCK_ORG, prisma: mockPrisma },
272+
);
273+
274+
expect(mockGitRaw).toHaveBeenCalledWith(['show', 'HEAD:src/index.ts']);
275+
});
276+
277+
it('uses the repo path from getRepoPath for the git working directory', async () => {
278+
await getFileSourceForRepo(
279+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
280+
{ org: MOCK_ORG, prisma: mockPrisma },
281+
);
282+
283+
// getRepoPath mock returns `/mock/repos/${repo.id}`
284+
expect(mockCwd).toHaveBeenCalledWith('/mock/repos/123');
285+
});
286+
});
287+
288+
describe('language detection', () => {
289+
it('uses language from .gitattributes when present', async () => {
290+
const { resolveLanguageFromGitAttributes, parseGitAttributes } = await import('@/lib/gitattributes');
291+
const mockResolve = resolveLanguageFromGitAttributes as unknown as Mock;
292+
const mockParse = parseGitAttributes as unknown as Mock;
293+
294+
mockParse.mockReturnValue({ '*.ts': { linguist_language: 'TypeScript' } });
295+
mockResolve.mockReturnValue('TypeScript');
296+
297+
// Override default: .gitattributes call succeeds
298+
mockGitRaw.mockImplementation(async (args: string[]) => {
299+
if (args[1]?.endsWith('.gitattributes')) {
300+
return 'linguist-language=TypeScript';
301+
}
302+
return 'file content';
303+
});
304+
305+
const result = await getFileSourceForRepo(
306+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
307+
{ org: MOCK_ORG, prisma: mockPrisma },
308+
);
309+
310+
expect(result).toMatchObject({ language: 'TypeScript' });
311+
expect(mockResolve).toHaveBeenCalled();
312+
});
313+
314+
it('falls back to filename-based detection when .gitattributes is absent', async () => {
315+
const { detectLanguageFromFilename } = await import('@/lib/languageDetection');
316+
const mockDetect = detectLanguageFromFilename as unknown as Mock;
317+
mockDetect.mockReturnValue('TypeScript');
318+
319+
// Default beforeEach setup: .gitattributes throws → falls back to filename detection
320+
const result = await getFileSourceForRepo(
321+
{ path: 'src/index.ts', repo: 'github.com/owner/repo' },
322+
{ org: MOCK_ORG, prisma: mockPrisma },
323+
);
324+
325+
expect(result).toMatchObject({ language: 'TypeScript' });
326+
expect(mockDetect).toHaveBeenCalledWith('src/index.ts');
327+
});
328+
});
329+
});

packages/web/src/features/git/getFileSourceApi.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,9 +56,9 @@ export const getFileSourceForRepo = async (
5656
return fileNotFound(filePath, repoName);
5757
}
5858
if (errorMessage.includes('unknown revision') || errorMessage.includes('bad revision') || errorMessage.includes('invalid object name')) {
59-
return unexpectedError(`Invalid git reference: ${gitRef}`);
59+
return invalidGitRef(gitRef);
6060
}
61-
throw error;
61+
return unexpectedError(errorMessage);
6262
}
6363

6464
let gitattributesContent: string | undefined;

0 commit comments

Comments
 (0)