Skip to content

Commit 651700b

Browse files
fatmcgavGavin Williamsclaude
authored
fix(web): restore ServiceError boundary in getFileSourceForRepo (#1145)
* 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 | * fix(web): harden getFileSourceForRepo error boundary Three fixes to getFileSourceForRepo, which was extracted outside sew() in v4.16.14 and lost its error boundary: - Wrap the entire function body in a top-level try/catch so exceptions from prisma, getRepoPath, simpleGit().cwd(), language helpers, and URL builders are converted to unexpectedError rather than propagating as fatal Next.js task-runner exceptions. - Add unresolvedGitRef() to serviceError.ts (errorCode: INVALID_GIT_REF, distinct message) and use it for "unknown revision"/"bad revision"/ "invalid object name" git errors, replacing the syntactic invalidGitRef message that was misleading for unfetched head_sha refs. - Fix the simple-git vi.mock() factory in the test file to map both the default and named exports to the same hoisted mock fn, ensuring the SUT and the test body reference identical mocks. Add a test for the outer catch (DB throws) and tighten INVALID_GIT_REF assertions to distinguish syntactic from unresolved-ref errors by message content. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add CHANGELOG entry * refactor(web): use sew() instead of bare try/catch in getFileSourceForRepo The top-level try/catch added to getFileSourceForRepo silently swallowed unexpected errors with no Sentry capture or log entry. Replace it with sew(), which provides the same ServiceError conversion plus Sentry reporting and structured logging. The inner git-specific catch is preserved unchanged — sew() only handles anything that escapes it (DB errors, getRepoPath, URL builders, etc.). Update the sew mock in the test file to catch and convert exceptions, matching the real sew() behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Gavin Williams <gavin.williams@getchip.uk> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent d111397 commit 651700b

4 files changed

Lines changed: 382 additions & 5 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
- 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)
12+
1013
## [4.16.14] - 2026-04-21
1114

1215
### Added

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

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

0 commit comments

Comments
 (0)