fix(web): restore ServiceError boundary in getFileSourceForRepo#1145
Merged
brendan-kellam merged 4 commits intosourcebot-dev:mainfrom Apr 23, 2026
Merged
fix(web): restore ServiceError boundary in getFileSourceForRepo#1145brendan-kellam merged 4 commits intosourcebot-dev:mainfrom
getFileSourceForRepo#1145brendan-kellam merged 4 commits intosourcebot-dev:mainfrom
Conversation
## Problem In v4.16.14 (PR sourcebot-dev#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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Vitest test suite for getFileSourceForRepo and updates getFileSourceForRepo to run inside a sew(...) wrapper, map unresolved/bad git refs to a new unresolvedGitRef ServiceError, and ensure all error paths return ServiceError instead of rethrowing exceptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as getFileSourceForRepo
participant DB as PrismaRepo
participant FS as getRepoPath
participant Git as simple-git
participant Attr as .gitattributesResolver
participant Lang as FilenameLanguageResolver
Client->>API: Request(repoId, path, ref?)
API->>DB: findUnique(repoId)
DB-->>API: repo or null
alt repo null
API-->>Client: ServiceError NOT_FOUND
else
API->>FS: resolve repo path
FS-->>API: workingDir
API->>Git: git.raw(['show', `${ref|defaultBranch|HEAD}:${path}`], {cwd: workingDir})
Git-->>API: file content or error
alt file content
API->>Attr: parse .gitattributes (if present)
Attr-->>API: language override?
alt override present
API-->>Client: ServiceResponse { source, language from .gitattributes }
else
API->>Lang: infer from filename
Lang-->>API: language
API-->>Client: ServiceResponse { source, language }
end
else git error
alt message matches "unknown/bad/invalid"
API-->>Client: ServiceError INVALID_GIT_REF (unresolvedGitRef)
else message indicates missing file/path
API-->>Client: ServiceError FILE_NOT_FOUND
else
API-->>Client: ServiceError UNEXPECTED_ERROR
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/features/git/getFileSourceApi.ts (1)
26-61:⚠️ Potential issue | 🟠 MajorRestore the error boundary around the whole helper.
Line 61 only converts failures from the first
git.raw(['show', ...])call.prisma.repo.findFirst,getRepoPath,simpleGit().cwd, parsing/language helpers, and URL builders can still throw/reject before or after this block, which still escapes direct callers that only checkisServiceError()afterawait. Wrap the full helper body, while keeping the existing typed git error mapping inside it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/git/getFileSourceApi.ts` around lines 26 - 61, The helper getFileSourceForRepo currently only catches errors from the git.raw(['show', ...]) call; wrap the entire function body (everything after entering getFileSourceForRepo) in a top-level try/catch so any rejection from prisma.repo.findFirst, getRepoPath, simpleGit().cwd, parsing helpers, or URL builders is converted to a ServiceError via unexpectedError, and preserve the existing inner mapping for git errors (the current errorMessage checks that return fileNotFound or invalidGitRef) inside that catch rather than letting those escape; ensure the function still returns Promise<FileSourceResponse | ServiceError> and that caught unknown errors are normalized to a string and passed to unexpectedError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/features/git/getFileSourceApi.test.ts`:
- Line 4: The test is mocking 'simple-git' but creates different mock objects
for the default export and the named export used in the test; update the vi.mock
call to provide an explicit factory that maps both the default export and the
named export to the same mock function so the SUT (which imports default
simpleGit) receives the configured mock chain—create a single mock function and
return it as both default and simpleGit in the factory so tests that import
either { simpleGit } or default simpleGit use the identical mock.
In `@packages/web/src/features/git/getFileSourceApi.ts`:
- Around line 58-59: The getFileSourceApi branch currently returns
invalidGitRef(gitRef) for errors like "unknown revision"/"bad revision"/"invalid
object name", which produces a syntactic-validation-style message; change this
to produce an "unresolved-ref" message while preserving errorCode:
INVALID_GIT_REF by either (a) adding a new helper (e.g.,
unresolvedGitRef(gitRef)) that formats user text about an unavailable/unresolved
ref and use that in getFileSourceApi, or (b) extend invalidGitRef to accept a
flag/variant for unresolved vs syntactic errors and call it with the unresolved
variant; update the service error helper(s) in serviceError.ts (where
invalidGitRef is defined) accordingly and ensure callers still get errorCode:
INVALID_GIT_REF.
---
Outside diff comments:
In `@packages/web/src/features/git/getFileSourceApi.ts`:
- Around line 26-61: The helper getFileSourceForRepo currently only catches
errors from the git.raw(['show', ...]) call; wrap the entire function body
(everything after entering getFileSourceForRepo) in a top-level try/catch so any
rejection from prisma.repo.findFirst, getRepoPath, simpleGit().cwd, parsing
helpers, or URL builders is converted to a ServiceError via unexpectedError, and
preserve the existing inner mapping for git errors (the current errorMessage
checks that return fileNotFound or invalidGitRef) inside that catch rather than
letting those escape; ensure the function still returns
Promise<FileSourceResponse | ServiceError> and that caught unknown errors are
normalized to a string and passed to unexpectedError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a07528a5-c91d-4aee-82e5-a9184471b2e1
📒 Files selected for processing (2)
packages/web/src/features/git/getFileSourceApi.test.tspackages/web/src/features/git/getFileSourceApi.ts
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>
…rRepo 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>
brendan-kellam
approved these changes
Apr 23, 2026
Contributor
There was a problem hiding this comment.
LGTM thanks!
The review agent is a unique path where we don't actually have a auth context to lean on (and therefore, cannot use withAuth).
Some loose thoughts: Eventually, I would like to get to the point where there is some amount of formalization between the "API layer" where calls are authenticated and authorized for the caller, and the "service layer" where the business logic lives. The idea would then be that we could better reason about what code does / does not contain auth guarantees.
For the most part, this formalization is not necessary right now, but it's something to consider
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In v4.16.14 (PR #1104, GitLab MR Review Agent), the core file-fetch logic was
extracted from inside
sew()into a standalonegetFileSourceForRepofunctionso 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
getFileSourceForRepohandled two known git error patterns and re-threweverything else:
Because
getFileSourceForRepois no longer insidesew(), any re-thrownexception escapes as a fatal error through the Next.js server task runner,
producing the
z.onFatalException/z.attemptTaskstack trace seen inproduction on v4.16.14.
A second contributing factor: the review agent was also changed in v4.16.14 to
pass
ref: pr_payload.head_shawhererefwas previouslyundefined. If thebare clone hasn't fetched that commit yet,
git showthrows 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, whichis why the rollback fixed the issue.
Fix
Two targeted changes in
getFileSourceApi.ts:throw error→return unexpectedError(errorMessage)— ensuresgetFileSourceForRepoalways returnsFileSourceResponse | ServiceErrorand never rejects its promise. This is the root-cause fix.
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_shahasn't been fetched.Tests
Added
getFileSourceApi.test.tswith 19 tests covering:NOT_FOUNDwhen repo is absent from the DB;correct
findFirstquery shape.FILE_NOT_FOUND;refs starting with
-→INVALID_GIT_REF(flag-injection guard)."does not exist"/"fatal: path"→FILE_NOT_FOUND"unknown revision"(unfetchedhead_sha) →INVALID_GIT_REF"bad revision"/"invalid object name"→INVALID_GIT_REFUNEXPECTED_ERROR, not a throw — explicitregression test; before the fix,
.resolveswould fail because thepromise rejected.
(
ref→defaultBranch→HEAD), correctcwdpath..gitattributes; falls back tofilename-based detection when the file is absent.
Files changed
packages/web/src/features/git/getFileSourceApi.tsthrow error→return unexpectedError(errorMessage); invalid-ref branch now returnsinvalidGitRef(gitRef)packages/web/src/features/git/getFileSourceApi.test.tsFixes #1144
Summary by CodeRabbit
Bug Fixes
Tests