feat(web): GitLab MR Review Agent#1104
Open
fatmcgav wants to merge 9 commits intosourcebot-dev:mainfrom
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds GitLab Merge Request support to the review agent: env schema and client, webhook routing/validation for GitLab events, MR parsing and posting, UI split for GitHub/GitLab agents, AI SDK model selection, repo-scoped file source API, tests, dependency and docs/changelog updates. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Webhook as Webhook Handler
participant GitLab as GitLab API
participant Parser as MR Parser
participant LLM as LLM Service
participant Pusher as MR Pusher
User->>Webhook: POST GitLab webhook (MR or note)
Webhook->>Webhook: validate x-gitlab-token vs GITLAB_REVIEW_AGENT_WEBHOOK_SECRET
Webhook->>GitLab: MergeRequests.show(projectId, mrIid)
GitLab-->>Webhook: MR details
Webhook->>Parser: gitlabMrParser(client, payload)
Parser->>GitLab: MergeRequests.allDiffs(projectId, mrIid)
GitLab-->>Parser: file diffs
Parser-->>Webhook: parsed PR payload (with file_diffs, diff_refs)
Webhook->>LLM: generatePrReviews(prPayload)
LLM-->>Webhook: review items per file
Webhook->>Pusher: gitlabPushMrReviews(client, projectId, prPayload, reviews)
Pusher->>GitLab: MergeRequestDiscussions.create (inline)
alt inline succeeds
GitLab-->>Pusher: discussion created
else inline fails
Pusher->>GitLab: MergeRequestNotes.create (fallback note)
GitLab-->>Pusher: note created
end
Pusher-->>Webhook: done
Webhook-->>User: { status: "ok" }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Contributor
Author
|
I've tested this locally against a Gitlab repo using AWS Bedrock, and got a successful review 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: 6
🧹 Nitpick comments (7)
packages/web/src/features/git/getFileSourceApi.ts (1)
10-10: Preferimport typefor purely type-level imports.
OrgandPrismaClientare only used in type positions here.Suggested cleanup
-import { Org, PrismaClient } from '@sourcebot/db'; +import type { Org, PrismaClient } from '@sourcebot/db';🤖 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` at line 10, The import currently brings in Org and PrismaClient as runtime imports even though they are only used as types; change the import in getFileSourceApi.ts to a type-only import by replacing "import { Org, PrismaClient } from '@sourcebot/db';" with "import type { Org, PrismaClient } from '@sourcebot/db';" and ensure no runtime references to Org or PrismaClient remain so the type-only import is valid.packages/web/src/app/(app)/agents/page.tsx (1)
30-42: Single-agent layout branches are now dead code.Since
agentsis statically two entries, theagents.length === 1conditionals are no longer reachable and can be simplified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/`(app)/agents/page.tsx around lines 30 - 42, The conditional branches checking agents.length === 1 are dead code; simplify the layout by removing the single-agent branches and using the multi-agent classes directly for the container and the agent card. Update the outer container JSX (where agents.map is rendered) to always use "grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-10" and update the agent card JSX (the div with key={agent.id} and className logic) to always use the multi-agent class "relative flex flex-col items-center border border-border rounded-2xl p-8 bg-card shadow-xl" so the unnecessary ternary checks around agents.length are eliminated.packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts (1)
13-18: Avoid re-querying the org on every file fetch call.
fetchFileContentis called repeatedly per diff, so this org lookup is repeated unnecessarily. Consider resolvingorgonce upstream and passing it down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts` around lines 13 - 18, The org lookup inside fetchFileContent (currently using __unsafePrisma.org.findUnique with SINGLE_TENANT_ORG_ID and throwing if org is missing) causes repeated DB queries; refactor fetchFileContent to accept an org (or orgId/needed org fields) as an argument and remove the internal __unsafePrisma.org.findUnique call so callers resolve org once upstream and pass it in, or alternatively add a lightweight in-memory cache keyed by SINGLE_TENANT_ORG_ID and reuse the cached org instead of re-querying each call.packages/web/src/features/agents/review-agent/app.ts (1)
62-80: Consider extracting duplicate log setup logic.The log path initialization (directory creation, timestamp formatting, path construction) is duplicated between
processGitHubPullRequest(lines 29-47) andprocessGitLabMergeRequest(lines 62-80). This could be extracted to a shared helper.♻️ Example helper extraction
function getReviewAgentLogPath(identifier: string): string | undefined { if (!env.REVIEW_AGENT_LOGGING_ENABLED) { return undefined; } const reviewAgentLogDir = path.join(env.DATA_CACHE_DIR, "review-agent"); if (!fs.existsSync(reviewAgentLogDir)) { fs.mkdirSync(reviewAgentLogDir, { recursive: true }); } const timestamp = new Date().toLocaleString('en-US', { year: 'numeric', month: '2-digit', day: '2-digit', hour: '2-digit', minute: '2-digit', second: '2-digit', hour12: false }).replace(/(\d+)\/(\d+)\/(\d+), (\d+):(\d+):(\d+)/, '$3_$1_$2_$4_$5_$6'); const logPath = path.join(reviewAgentLogDir, `review-agent-${identifier}-${timestamp}.log`); logger.info(`Review agent logging to ${logPath}`); return logPath; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/app.ts` around lines 62 - 80, Extract the duplicated log setup in processGitHubPullRequest and processGitLabMergeRequest into a single helper (e.g., getReviewAgentLogPath(identifier)) that checks env.REVIEW_AGENT_LOGGING_ENABLED, ensures the review-agent directory exists (fs.existsSync + fs.mkdirSync with recursive), builds the timestamp using the same toLocaleString/replace logic, composes the filename with the provided identifier (replace uses of mrPayload.object_attributes.iid or pullRequest number), calls logger.info with the final path, and returns the path or undefined; then replace the duplicate blocks in both functions with a call to this helper.packages/web/src/app/api/(server)/webhook/route.ts (2)
246-252: Same error handling concern for note event processing.Apply similar try-catch wrapping here to prevent webhook retry storms on processing errors.
♻️ Suggested error handling
+ try { await processGitLabMergeRequest( gitlabClient, body.project.id, mrPayload, env.GITLAB_REVIEW_AGENT_HOST, ); + } catch (error) { + logger.error('Error processing GitLab note event:', error); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/api/`(server)/webhook/route.ts around lines 246 - 252, Wrap the call to processGitLabMergeRequest in a try-catch to mirror the note-event handling: invoke await processGitLabMergeRequest(gitlabClient, body.project.id, mrPayload, env.GITLAB_REVIEW_AGENT_HOST) inside try, and on catch log the error with context (including body.project.id and event type) and return a successful webhook response so GitLab doesn't retry; ensure you reference processGitLabMergeRequest, gitlabClient, mrPayload and env.GITLAB_REVIEW_AGENT_HOST when adding the logging/context.
220-226: Consider wrappingprocessGitLabMergeRequestin try-catch to prevent retry storms.If
processGitLabMergeRequestthrows, the webhook returns a 500 error, which may cause GitLab to retry the webhook delivery repeatedly. This could lead to duplicate review attempts or excessive load.♻️ Suggested error handling
if (isGitLabMergeRequestEvent(gitlabEvent, body)) { if (env.REVIEW_AGENT_AUTO_REVIEW_ENABLED === "false") { logger.info('Review agent auto review (REVIEW_AGENT_AUTO_REVIEW_ENABLED) is disabled, skipping'); return Response.json({ status: 'ok' }); } + try { await processGitLabMergeRequest( gitlabClient, body.project.id, body, env.GITLAB_REVIEW_AGENT_HOST, ); + } catch (error) { + logger.error('Error processing GitLab merge request:', error); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/api/`(server)/webhook/route.ts around lines 220 - 226, Wrap the call to processGitLabMergeRequest(gitlabClient, body.project.id, body, env.GITLAB_REVIEW_AGENT_HOST) in a try-catch inside the webhook route handler; on error catch the exception, log the full error (including contextual values like body.project.id and env.GITLAB_REVIEW_AGENT_HOST) using the existing logger (or console.error if none), and return a non-retriable HTTP response (e.g., 200/202 with an error payload) instead of letting the exception bubble up to produce a 500. Ensure the catch only suppresses delivery retries while still surfacing enough detail in logs for debugging.packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts (1)
35-41: Consider swapping system and prompt content for better provider compatibility.The detailed review prompt is placed in
systemwhile the user turn only contains "Review the code changes." This inverted usage may cause suboptimal results with some providers that expect the system message to contain role/behavior instructions and the user message to contain the actual task content.♻️ Suggested refactor
const result = await generateText({ model, - system: prompt, - prompt: "Review the code changes.", + system: "You are a code review assistant. Respond only with valid JSON matching the expected schema.", + prompt: prompt, providerOptions, temperature, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts` around lines 35 - 41, The generateText call in invokeDiffReviewLlm.ts currently passes the detailed review instructions in the system parameter and a generic "Review the code changes." as the prompt, which can confuse some providers; update the call to use a concise role/behavior string for system (e.g., a short reviewer persona) and pass the detailed review text stored in the prompt variable as the prompt/user message instead. Modify the generateText invocation (function generateText, params model, system, prompt, providerOptions, temperature) so system contains role/behavior guidance and prompt is the detailed review instructions currently in the prompt variable, keeping providerOptions and temperature unchanged.
🤖 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/shared/src/env.server.ts`:
- Around line 198-202: The GITLAB_REVIEW_AGENT_HOST env entry currently accepts
any string which can include a protocol and break the later `https://${...}` URL
assembly; update the env schema for GITLAB_REVIEW_AGENT_HOST in
packages/shared/src/env.server.ts (the env schema definition) to
validate/normalize it as a hostname: replace the current
z.string().default('gitlab.com') with a schema that strips any leading
protocol/slashes and validates the remainder as a hostname (e.g., use
z.string().default('gitlab.com').transform(s => s.replace(/^https?:\/\//,
'').replace(/\/+$/, '')).refine(s => /^[a-z0-9.-]+$/i.test(s), { message:
'invalid hostname' })), so downstream code that constructs
`https://${GITLAB_REVIEW_AGENT_HOST}` always receives a bare hostname.
In `@packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts`:
- Around line 21-27: fileSourceRequest currently omits the Git ref so
getFileSourceForRepo may read the default branch; add a ref property to
fileSourceRequest set to the PR/MR head SHA (the head SHA variable available in
this scope) before calling getFileSourceForRepo so the fetch uses the exact
revision under review; update the object passed to getFileSourceForRepo
(fileSourceRequest) to include ref and ensure you pass the same org and
__unsafePrisma options.
In
`@packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.test.ts`:
- Around line 133-139: The async test "does not throw when all review comments
fail" incorrectly uses resolves.not.toThrow(); update the assertion to await
that the promise resolves to undefined by replacing the matcher with
resolves.toBeUndefined() when calling githubPushPrReviews(octokit, MOCK_PAYLOAD,
SINGLE_REVIEW) so the test correctly asserts the resolved value; locate the test
block containing githubPushPrReviews and change the expectation accordingly.
In `@packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.ts`:
- Around line 99-101: The code in gitlabMrParser.ts currently force-casts
mr.diff_refs to a typed object when building the MR payload
(head_sha/diff_refs), which can throw if mr.diff_refs is null; update the logic
in the function that constructs the object (where mr.diff_refs is accessed) to
first validate mr.diff_refs !== null && mr.diff_refs !== undefined before the
type assertion, and if missing, set diff_refs to undefined or an empty safe
fallback and ensure head_sha is derived from mr.sha or guarded similarly so
downstream gitlabPushMrReviews can gracefully fall back to general notes; this
avoids runtime errors by failing over rather than force-casting a potentially
null value.
In `@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts`:
- Around line 29-38: The position object sent to GitLab is using camelCase and
wrong types; update the object in gitlabPushMrReviews.ts (the position block
that references base_sha, head_sha, start_sha, fileDiffReview.filename and
review.line_end) to use GitLab's snake_case keys: position_type, base_sha,
head_sha, start_sha, new_path and include old_path (set to
fileDiffReview.filename), and ensure new_line is a number (use review.line_end
as a number, not String). Preserve the surrounding structure but replace
camelCase keys (baseSha, headSha, startSha, newPath, newLine) with the correct
snake_case ones so `@gitbeaker/rest` forwards a valid GitLab API payload.
In `@packages/web/src/features/agents/review-agent/types.ts`:
- Around line 59-74: The GitLab webhook interfaces assume diff_refs is always
present, which is unsafe; update
GitLabMergeRequestPayload.object_attributes.diff_refs and
GitLabNotePayload.merge_request.diff_refs to be optional or nullable (e.g., mark
as diff_refs?: { base_sha: string; head_sha: string; start_sha: string } | null)
so downstream code must handle missing/null diff_refs; adjust any related type
imports/exports if necessary and run type checks to ensure no remaining non-null
assumptions.
---
Nitpick comments:
In `@packages/web/src/app/`(app)/agents/page.tsx:
- Around line 30-42: The conditional branches checking agents.length === 1 are
dead code; simplify the layout by removing the single-agent branches and using
the multi-agent classes directly for the container and the agent card. Update
the outer container JSX (where agents.map is rendered) to always use "grid
grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-10" and update the agent card JSX
(the div with key={agent.id} and className logic) to always use the multi-agent
class "relative flex flex-col items-center border border-border rounded-2xl p-8
bg-card shadow-xl" so the unnecessary ternary checks around agents.length are
eliminated.
In `@packages/web/src/app/api/`(server)/webhook/route.ts:
- Around line 246-252: Wrap the call to processGitLabMergeRequest in a try-catch
to mirror the note-event handling: invoke await
processGitLabMergeRequest(gitlabClient, body.project.id, mrPayload,
env.GITLAB_REVIEW_AGENT_HOST) inside try, and on catch log the error with
context (including body.project.id and event type) and return a successful
webhook response so GitLab doesn't retry; ensure you reference
processGitLabMergeRequest, gitlabClient, mrPayload and
env.GITLAB_REVIEW_AGENT_HOST when adding the logging/context.
- Around line 220-226: Wrap the call to processGitLabMergeRequest(gitlabClient,
body.project.id, body, env.GITLAB_REVIEW_AGENT_HOST) in a try-catch inside the
webhook route handler; on error catch the exception, log the full error
(including contextual values like body.project.id and
env.GITLAB_REVIEW_AGENT_HOST) using the existing logger (or console.error if
none), and return a non-retriable HTTP response (e.g., 200/202 with an error
payload) instead of letting the exception bubble up to produce a 500. Ensure the
catch only suppresses delivery retries while still surfacing enough detail in
logs for debugging.
In `@packages/web/src/features/agents/review-agent/app.ts`:
- Around line 62-80: Extract the duplicated log setup in
processGitHubPullRequest and processGitLabMergeRequest into a single helper
(e.g., getReviewAgentLogPath(identifier)) that checks
env.REVIEW_AGENT_LOGGING_ENABLED, ensures the review-agent directory exists
(fs.existsSync + fs.mkdirSync with recursive), builds the timestamp using the
same toLocaleString/replace logic, composes the filename with the provided
identifier (replace uses of mrPayload.object_attributes.iid or pullRequest
number), calls logger.info with the final path, and returns the path or
undefined; then replace the duplicate blocks in both functions with a call to
this helper.
In `@packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts`:
- Around line 13-18: The org lookup inside fetchFileContent (currently using
__unsafePrisma.org.findUnique with SINGLE_TENANT_ORG_ID and throwing if org is
missing) causes repeated DB queries; refactor fetchFileContent to accept an org
(or orgId/needed org fields) as an argument and remove the internal
__unsafePrisma.org.findUnique call so callers resolve org once upstream and pass
it in, or alternatively add a lightweight in-memory cache keyed by
SINGLE_TENANT_ORG_ID and reuse the cached org instead of re-querying each call.
In `@packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts`:
- Around line 35-41: The generateText call in invokeDiffReviewLlm.ts currently
passes the detailed review instructions in the system parameter and a generic
"Review the code changes." as the prompt, which can confuse some providers;
update the call to use a concise role/behavior string for system (e.g., a short
reviewer persona) and pass the detailed review text stored in the prompt
variable as the prompt/user message instead. Modify the generateText invocation
(function generateText, params model, system, prompt, providerOptions,
temperature) so system contains role/behavior guidance and prompt is the
detailed review instructions currently in the prompt variable, keeping
providerOptions and temperature unchanged.
In `@packages/web/src/features/git/getFileSourceApi.ts`:
- Line 10: The import currently brings in Org and PrismaClient as runtime
imports even though they are only used as types; change the import in
getFileSourceApi.ts to a type-only import by replacing "import { Org,
PrismaClient } from '@sourcebot/db';" with "import type { Org, PrismaClient }
from '@sourcebot/db';" and ensure no runtime references to Org or PrismaClient
remain so the type-only import is valid.
🪄 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: a2462b8d-0146-426e-8d4c-f6797bd49a90
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
.gitignorepackages/shared/src/env.server.tspackages/web/package.jsonpackages/web/src/app/(app)/agents/page.tsxpackages/web/src/app/(app)/components/navigationMenu/index.tsxpackages/web/src/app/(app)/components/navigationMenu/navigationItems.tsxpackages/web/src/app/api/(server)/webhook/route.tspackages/web/src/features/agents/review-agent/app.tspackages/web/src/features/agents/review-agent/nodes/fetchFileContent.tspackages/web/src/features/agents/review-agent/nodes/githubPrParser.test.tspackages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabMrParser.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabMrParser.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.tspackages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.tspackages/web/src/features/agents/review-agent/types.tspackages/web/src/features/git/getFileSourceApi.ts
Comment on lines
+29
to
+38
| { | ||
| position: { | ||
| positionType: "text", | ||
| baseSha: base_sha, | ||
| headSha: head_sha, | ||
| startSha: start_sha, | ||
| newPath: fileDiffReview.filename, | ||
| newLine: String(review.line_end), | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@gitbeaker/rest MergeRequestDiscussions create position parameter format
💡 Result:
For @gitbeaker/rest MergeRequestDiscussions.create, the position you pass should match GitLab’s “Create a new thread in the merge request diff” payload: i.e., an object that contains the same snake_case keys GitLab documents under position[...] (GitBeaker forwards these as request fields). [1]
Single-line diff comment (position_type: "text")
Provide the SHA triplet from the latest MR version + file paths + either new_line or old_line (or both for unchanged lines):
position: {
position_type: 'text',
base_sha: '<base_commit_sha>',
start_sha: '<start_commit_sha>',
head_sha: '<head_commit_sha>',
new_path: 'path/to/file.ext',
old_path: 'path/to/file.ext',
// added line:
new_line: 18,
// removed line (use instead of new_line):
// old_line: 18,
// unchanged line (include both):
// new_line: 18,
// old_line: 18,
}Notes from GitLab:
position[old_path]andposition[new_path]are required for text positions.- Use only
new_linefor added lines, onlyold_linefor removed lines, and both for unchanged lines. [1]
Multi-line diff comment (position[line_range])
Add position[line_range][start] and position[line_range][end] with line_code + type:
position: {
position_type: 'text',
base_sha: '<base_commit_sha>',
start_sha: '<start_commit_sha>',
head_sha: '<head_commit_sha>',
new_path: 'path/to/file.ext',
old_path: 'path/to/file.ext',
line_range: {
start: { line_code: '<sha1(path)>_<old>_<new>', type: 'new', new_line: 10 },
end: { line_code: '<sha1(path)>_<old>_<new>', type: 'new', new_line: 12 },
},
}GitLab’s documented line_code format is: <SHA>_<old>_<new> where <SHA> is the SHA1 of the filename (path). [1] Also, line_code is required when using position.line_range. [2]
Where to get base_sha/start_sha/head_sha: call the MR “versions” endpoint and use the latest item’s base_commit_sha, start_commit_sha, and head_commit_sha. [1]
Sources: GitLab Discussions API docs (merge request diff discussions + line_range + line_code format) [1], StackOverflow clarification on line_code usage [2].
Fix position object property casing and format for GitLab API compatibility.
The position object uses incorrect camelCase properties when @gitbeaker/rest forwards these to GitLab's API as snake_case. Additionally, new_line must be a number, not a string, and old_path is required alongside new_path:
Correct format
{
position: {
position_type: "text",
base_sha: base_sha,
head_sha: head_sha,
start_sha: start_sha,
new_path: fileDiffReview.filename,
old_path: fileDiffReview.filename, // required
new_line: review.line_end, // number, not string
},
},Current properties (baseSha, headSha, startSha, newPath, newLine) will not be recognized by GitLab's API, causing the request to fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts`
around lines 29 - 38, The position object sent to GitLab is using camelCase and
wrong types; update the object in gitlabPushMrReviews.ts (the position block
that references base_sha, head_sha, start_sha, fileDiffReview.filename and
review.line_end) to use GitLab's snake_case keys: position_type, base_sha,
head_sha, start_sha, new_path and include old_path (set to
fileDiffReview.filename), and ensure new_line is a number (use review.line_end
as a number, not String). Preserve the surrounding structure but replace
camelCase keys (baseSha, headSha, startSha, newPath, newLine) with the correct
snake_case ones so `@gitbeaker/rest` forwards a valid GitLab API payload.
Contributor
Author
There was a problem hiding this comment.
Clade thinks this one is incorrect:
The Gitbeaker API expects camelCase — positionType, baseSha, headSha, startSha, newPath, newLine — which is exactly what the current code uses. The finding is incorrect.
However, there are two genuine issues the type definition reveals:
- newLine should be string (it already is — String(review.line_end) is correct per the type)
- oldPath is missing — the type includes it as optional, and adding it is good practice for renamed files
The type confirms the current code is correct. No changes are needed.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the gitbeaker MergeRequestDiscussions type definition to confirm camelCase vs snake_case
fd -i "MergeRequestDiscussions" --type f
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the gitbeaker package types for position object
find . -path "*/node_modules/@gitbeaker/core*" -name "*.d.ts" | head -20
Length of output: 0
@fatmcgav, you're correct — I apologize for the incorrect review comment. @gitbeaker is a comprehensive and typed Gitlab SDK, and its TypeScript API exposes camelCase property names (positionType, baseSha, headSha, startSha, newPath, newLine) for the position object — the SDK handles the translation to GitLab's snake_case REST API internally. The current code is correct.
I'll disregard this comment.
✏️ Learnings added
Learnt from: fatmcgav
Repo: sourcebot-dev/sourcebot PR: 1104
File: packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts:29-38
Timestamp: 2026-04-14T08:35:05.785Z
Learning: In gitbeaker/rest (TypeScript GitLab SDK), `MergeRequestDiscussions.create()` expects camelCase property names for the `position` object: `positionType`, `baseSha`, `headSha`, `startSha`, `newPath`, `newLine`. The SDK internally translates these to GitLab's snake_case REST API fields. Do not flag these as incorrect casing. The `newLine` field also expects a string (e.g., `String(review.line_end)`) per the gitbeaker TypeScript types.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
fatmcgav
force-pushed
the
feat-gitlab-mr-agent
branch
from
ec2dc25 to
456fa94
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/web/src/features/agents/review-agent/types.ts (1)
69-73:⚠️ Potential issue | 🟡 MinorMake
diff_refsnullable/optional in both GitLab webhook interfaces.
packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.tsalready documents that merge request hooks can omit or nulldiff_refson some actions, but these interfaces still require it. That keeps downstream code falsely narrowed and masks the null-handling path the GitLab flow already depends on.Suggested type fix
export interface GitLabMergeRequestPayload { object_kind: string; object_attributes: { iid: number; title: string; description: string | null; action: string; last_commit: { id: string; }; - diff_refs: { + diff_refs?: { base_sha: string; head_sha: string; start_sha: string; - }; + } | null; }; project: { id: number; name: string; path_with_namespace: string; @@ export interface GitLabNotePayload { object_kind: string; object_attributes: { note: string; noteable_type: string; }; merge_request: { iid: number; title: string; description: string | null; last_commit: { id: string; }; - diff_refs: { + diff_refs?: { base_sha: string; head_sha: string; start_sha: string; - }; + } | null; }; project: { id: number; name: string; path_with_namespace: string;Also applies to: 97-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/types.ts` around lines 69 - 73, The diff_refs property in the GitLab webhook interfaces is currently required but can be omitted/null for some MR actions; update both occurrences of diff_refs in this file to be nullable/optional (e.g., change from diff_refs: { base_sha: string; head_sha: string; start_sha: string; } to diff_refs?: { base_sha: string; head_sha: string; start_sha: string; } | null) so downstream code can handle the null/undefined path; ensure you update both interface occurrences referenced in the comment (the MR webhook interface and the other GitLab webhook interface at the second occurrence) and run type checks to confirm no remaining narrow assumptions.
🤖 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/app/api/`(server)/webhook/route.ts:
- Around line 99-117: The GitLab webhook branch uses body typed as unknown and
relies on isGitLabMergeRequestEvent / isGitLabNoteEvent guards but still
dereferences body.project and body.merge_request (e.g., body.project.id,
body.merge_request.iid/title/description/last_commit/diff_refs) without
validating their shapes; add Zod schemas (e.g., GitLabProjectSchema,
GitLabMergeRequestSchema, GitLabNoteSchema or a combined GitLabWebhookSchema)
and in the route handler parse await request.json(), call schema.safeParse(); if
parse fails return requestBodySchemaValidationError, otherwise use the parsed
value for downstream logic instead of assuming fields exist. Ensure you
reference and replace unchecked accesses in the branches that used
isGitLabMergeRequestEvent / isGitLabNoteEvent to use the validated parsed
object.
In `@packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.ts`:
- Around line 33-34: The repo lookup key is derived incorrectly from
mrPayload.project.name; change the repo variable (repoName) to use the last
segment of mrPayload.project.path_with_namespace instead (e.g., const repoName =
mrPayload.project.path_with_namespace.split('/').pop() or equivalent) so that
the repo used when calling fetchFileContent() matches the stored repo.name;
update any use of namespace/repoName pair (namespace and repoName) accordingly.
---
Duplicate comments:
In `@packages/web/src/features/agents/review-agent/types.ts`:
- Around line 69-73: The diff_refs property in the GitLab webhook interfaces is
currently required but can be omitted/null for some MR actions; update both
occurrences of diff_refs in this file to be nullable/optional (e.g., change from
diff_refs: { base_sha: string; head_sha: string; start_sha: string; } to
diff_refs?: { base_sha: string; head_sha: string; start_sha: string; } | null)
so downstream code can handle the null/undefined path; ensure you update both
interface occurrences referenced in the comment (the MR webhook interface and
the other GitLab webhook interface at the second occurrence) and run type checks
to confirm no remaining narrow assumptions.
🪄 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: 2460ae46-0310-47d2-adc3-95f59f241b30
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.gitignoreCHANGELOG.mdpackages/shared/src/env.server.tspackages/web/package.jsonpackages/web/src/app/(app)/agents/page.tsxpackages/web/src/app/(app)/components/navigationMenu/index.tsxpackages/web/src/app/(app)/components/navigationMenu/navigationItems.tsxpackages/web/src/app/api/(server)/webhook/route.tspackages/web/src/features/agents/review-agent/app.tspackages/web/src/features/agents/review-agent/nodes/fetchFileContent.tspackages/web/src/features/agents/review-agent/nodes/githubPrParser.test.tspackages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabMrParser.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabMrParser.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.tspackages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.tspackages/web/src/features/agents/review-agent/types.tspackages/web/src/features/git/getFileSourceApi.ts
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- packages/web/package.json
- CHANGELOG.md
- packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/shared/src/env.server.ts
- packages/web/src/app/(app)/components/navigationMenu/index.tsx
- packages/web/src/app/(app)/agents/page.tsx
- packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts
- packages/web/src/features/agents/review-agent/nodes/githubPrParser.test.ts
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts
- packages/web/src/features/git/getFileSourceApi.ts
- packages/web/src/app/(app)/components/navigationMenu/navigationItems.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/web/src/app/api/(server)/webhook/route.ts (1)
249-260: Consider adding explicit type annotation to synthetic MR payload.The inline
mrPayloadobject is structurally correct, but adding an explicit type annotation would provide compile-time verification that it matchesGitLabMergeRequestPayloadand improve code clarity.♻️ Add type annotation
+ import { GitLabMergeRequestPayload } from "@/features/agents/review-agent/types"; // ... in the handler: - const mrPayload = { + const mrPayload: GitLabMergeRequestPayload = { object_kind: "merge_request", object_attributes: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/app/api/`(server)/webhook/route.ts around lines 249 - 260, The inline mrPayload object lacks an explicit type annotation; annotate it as GitLabMergeRequestPayload to get compile-time checking and clarity. Update the declaration of mrPayload (the object built from parsed.data.merge_request and parsed.data.project) to include the type GitLabMergeRequestPayload, ensure the fields you set (object_kind, object_attributes, project) conform to that type, and adjust any property names or casts if TypeScript reports mismatches in the function handling the webhook in route.ts.packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.ts (1)
37-49: Minor style note: type assertions before validation.Lines 38-39 cast to
stringbefore the null check on line 41. While functionally safe (falsy check catchesundefined), a cleaner pattern would be to access the properties without casting and let the null check handle type narrowing.♻️ Optional cleaner pattern
const sourcebotFileDiffs: (sourcebot_file_diff | null)[] = fileDiffs.map((fileDiff) => { - const fromPath = fileDiff.old_path as string; - const toPath = fileDiff.new_path as string; + const fromPath = fileDiff.old_path; + const toPath = fileDiff.new_path; if (!fromPath || !toPath) { logger.debug(`Skipping file due to missing old_path (${fromPath}) or new_path (${toPath})`); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.ts` around lines 37 - 49, The current code assigns fromPath and toPath using type assertions before checking for null/undefined; change the pattern in the fileDiffs mapping (the sourcebotFileDiffs creation) to read fileDiff.old_path and fileDiff.new_path without "as string" first, perform the null/undefined/falsy check (if (!fromPath || !toPath) ...) to let TypeScript narrow the types, and only cast or treat them as strings after the check (or simply rely on the narrowed type) so the null check is meaningful and avoids premature assertions in the sourcebotFileDiffs / fileDiffs mapping logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/web/src/app/api/`(server)/webhook/route.ts:
- Around line 249-260: The inline mrPayload object lacks an explicit type
annotation; annotate it as GitLabMergeRequestPayload to get compile-time
checking and clarity. Update the declaration of mrPayload (the object built from
parsed.data.merge_request and parsed.data.project) to include the type
GitLabMergeRequestPayload, ensure the fields you set (object_kind,
object_attributes, project) conform to that type, and adjust any property names
or casts if TypeScript reports mismatches in the function handling the webhook
in route.ts.
In `@packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.ts`:
- Around line 37-49: The current code assigns fromPath and toPath using type
assertions before checking for null/undefined; change the pattern in the
fileDiffs mapping (the sourcebotFileDiffs creation) to read fileDiff.old_path
and fileDiff.new_path without "as string" first, perform the
null/undefined/falsy check (if (!fromPath || !toPath) ...) to let TypeScript
narrow the types, and only cast or treat them as strings after the check (or
simply rely on the narrowed type) so the null check is meaningful and avoids
premature assertions in the sourcebotFileDiffs / fileDiffs mapping logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1fa14f8-a173-41ae-ac05-734028458bd7
📒 Files selected for processing (3)
packages/web/src/app/api/(server)/webhook/route.tspackages/web/src/features/agents/review-agent/nodes/gitlabMrParser.tspackages/web/src/features/agents/review-agent/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/features/agents/review-agent/types.ts
Contributor
Author
|
@msukkari @brendan-kellam A review on this one would be greatly appreciated, as we're keen to give it a try on a few repos internally :) |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/features/agents/review-agent/types.ts (1)
73-101: Tighten the webhook discriminators.
object_kindandnoteable_typeare plainz.string(), so unrelated GitLab payloads can still satisfy these schemas if the rest of the shape matches. Using literals here would reject unsupported hooks earlier and make downstream narrowing safer.🔎 Suggested schema tightening
export const gitLabMergeRequestPayloadSchema = z.object({ - object_kind: z.string(), + object_kind: z.literal("merge_request"), object_attributes: z.object({ iid: z.number(), title: z.string(), description: z.string().nullable(), action: z.string(), last_commit: z.object({ id: z.string() }), diff_refs: gitLabDiffRefsSchema, }), project: gitLabProjectSchema, }); export const gitLabNotePayloadSchema = z.object({ - object_kind: z.string(), + object_kind: z.literal("note"), object_attributes: z.object({ note: z.string(), - noteable_type: z.string(), + noteable_type: z.literal("MergeRequest"), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/types.ts` around lines 73 - 101, The schemas gitLabMergeRequestPayloadSchema and gitLabNotePayloadSchema use broad z.string() for discriminators allowing unrelated webhooks to pass; update the object_kind fields to z.literal('merge_request') in both gitLabMergeRequestPayloadSchema and gitLabNotePayloadSchema, and change noteable_type in gitLabNotePayloadSchema to the appropriate literal (e.g., z.literal('MergeRequest') or the exact GitLab value) so the schemas reject non-merge-request hooks and make downstream type narrowing safe.
🤖 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/agents/review-agent/nodes/gitlabMrParser.ts`:
- Line 100: The head_sha assignment currently uses mr.sha ?? "" which can
discard a valid webhook commit; change the fallback in the head_sha field to
prefer the webhook value by using mr.sha ??
mrPayload.object_attributes.last_commit.id ?? "" (update the head_sha
construction inside gitlabMrParser.ts where head_sha is set) so the MR flow
retains the webhook-provided commit id when MergeRequests.show() lacks sha.
---
Nitpick comments:
In `@packages/web/src/features/agents/review-agent/types.ts`:
- Around line 73-101: The schemas gitLabMergeRequestPayloadSchema and
gitLabNotePayloadSchema use broad z.string() for discriminators allowing
unrelated webhooks to pass; update the object_kind fields to
z.literal('merge_request') in both gitLabMergeRequestPayloadSchema and
gitLabNotePayloadSchema, and change noteable_type in gitLabNotePayloadSchema to
the appropriate literal (e.g., z.literal('MergeRequest') or the exact GitLab
value) so the schemas reject non-merge-request hooks and make downstream type
narrowing safe.
🪄 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: 2f200f46-48a5-40e2-8f26-35d18cb99089
📒 Files selected for processing (4)
packages/shared/src/env.server.tspackages/web/src/app/api/(server)/webhook/route.tspackages/web/src/features/agents/review-agent/nodes/gitlabMrParser.tspackages/web/src/features/agents/review-agent/types.ts
✅ Files skipped from review due to trivial changes (1)
- packages/shared/src/env.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/web/src/app/api/(server)/webhook/route.ts
| repo: repoName, | ||
| file_diffs: filteredSourcebotFileDiffs, | ||
| number: mrIid, | ||
| head_sha: mr.sha ?? "", |
Contributor
There was a problem hiding this comment.
Prefer the webhook SHA over "" as the fallback.
If MergeRequests.show() comes back without sha, head_sha: "" drops a valid commit id the webhook already supplied and downstream file fetches will miss the head revision. Falling back to mrPayload.object_attributes.last_commit.id keeps the MR review flow usable on that edge case.
🛠️ Suggested fallback
- head_sha: mr.sha ?? "",
+ head_sha: mr.sha ?? mrPayload.object_attributes.last_commit.id,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.ts` at
line 100, The head_sha assignment currently uses mr.sha ?? "" which can discard
a valid webhook commit; change the fallback in the head_sha field to prefer the
webhook value by using mr.sha ?? mrPayload.object_attributes.last_commit.id ??
"" (update the head_sha construction inside gitlabMrParser.ts where head_sha is
set) so the MR flow retains the webhook-provided commit id when
MergeRequests.show() lacks sha.
|
Prepared a fix for the two open CodeRabbit findings:
Patch is available on my fork branch: If helpful, maintainers can cherry-pick directly: |
added 9 commits
April 15, 2026 13:36
Adds support for the AI Review Agent to review GitLab Merge Requests, mirroring the existing GitHub PR review functionality. Also fixes several bugs discovered during implementation and improves the shared review pipeline.
---
## New files
### `packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.ts`
Parses a GitLab MR webhook payload into the shared `sourcebot_pr_payload` format. Calls `MergeRequests.show()` and `MergeRequests.allDiffs()` in parallel — the API response is used for `title`, `description`, `sha`, and `diff_refs` (which can be absent in webhook payloads for `update` action events), while per-file diffs are parsed using the existing `parse-diff` library.
### `packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts`
Posts review comments back to GitLab using `MergeRequestDiscussions.create()` with a position object carrying `base_sha`, `head_sha`, and `start_sha`. Falls back to `MergeRequestNotes.create()` (a general MR note) if the inline comment is rejected by the API (e.g. the line is not within the diff), ensuring reviews are always surfaced even when precise positioning fails.
### Test files (4 new files, 34 tests total)
- `githubPrParser.test.ts` — diff parsing and metadata mapping for the GitHub parser
- `githubPushPrReviews.test.ts` — single-line vs multi-line comment parameters, error resilience
- `gitlabMrParser.test.ts` — API call arguments, metadata mapping, diff parsing, edge cases (empty diffs, nested groups, null description, API failures)
- `gitlabPushMrReviews.test.ts` — inline comment posting, fallback behaviour, missing `diff_refs` guard, multi-file iteration
---
## Modified files
### `packages/web/src/features/agents/review-agent/types.ts`
- Added `sourcebot_diff_refs` schema/type (`base_sha`, `head_sha`, `start_sha`) and an optional `diff_refs` field on `sourcebot_pr_payload`
- Added `GitLabMergeRequestPayload` and `GitLabNotePayload` interfaces for webhook event typing
### `packages/web/src/features/agents/review-agent/app.ts`
- Added `processGitLabMergeRequest()` function mirroring `processGitHubPullRequest()`: sets up logging, runs the GitLab parser, generates reviews via the shared LLM pipeline, and pushes results
- Removed stale `OPENAI_API_KEY` guards (model availability is now enforced inside `invokeDiffReviewLlm`)
### `packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts`
**Replaces the hardcoded OpenAI client** with the Vercel AI SDK's `generateText` and the shared `getAISDKLanguageModelAndOptions` / `getConfiguredLanguageModels` utilities from `chat/utils.server.ts`. The review agent now uses whichever language model is configured in `config.json`, supporting all providers (Anthropic, Bedrock, Azure, etc.).
- `REVIEW_AGENT_MODEL` env var (matched against `displayName`) selects a specific model when multiple are configured; falls back to `models[0]` with a warning if the name is not found
- Prompt is passed via the `system` parameter with a `"Review the code changes."` user turn, satisfying providers (e.g. Bedrock/Anthropic) that require conversations to begin with a user message
### `packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts`
**Fixes "Not authenticated" error** when the review agent calls `getFileSource`. The original implementation used `withOptionalAuth`, which reads a session cookie — absent in webhook handlers. Now calls `getFileSourceForRepo` directly with `__unsafePrisma` and the single-tenant org, bypassing the session-based auth layer. The webhook handler has already authenticated the request via its own mechanism (GitHub App signature / GitLab token).
### `packages/web/src/features/git/getFileSourceApi.ts`
- Extracted the core repo-lookup + git + language-detection logic into a new exported `getFileSourceForRepo({ path, repo, ref }, { org, prisma })` function
- `getFileSource` now handles auth and audit logging then delegates to `getFileSourceForRepo` — all existing callers are unchanged
### `packages/web/src/app/api/(server)/webhook/route.ts`
- Added GitLab webhook handling alongside the existing GitHub branch
- Verifies `x-gitlab-token` against `GITLAB_REVIEW_AGENT_WEBHOOK_SECRET`
- Handles `Merge Request Hook` events (auto-review on `open`, `update`, `reopen`) and `Note Hook` events (manual `/review` command on MR comments)
- Initialises a `Gitlab` client at module load if `GITLAB_REVIEW_AGENT_TOKEN` is set
### `packages/web/src/app/(app)/agents/page.tsx`
- Split the single "Review Agent" card into two separate cards: **GitHub Review Agent** and **GitLab Review Agent**, each showing its own configuration status
- Removed `OPENAI_API_KEY` from the GitHub card's required env vars (no longer applicable)
### `packages/web/src/app/(app)/components/navigationMenu/navigationItems.tsx` & `index.tsx`
- Added an **Agents** nav item (with `BotIcon`) between Repositories and Settings
- Visible when the user is authenticated **and** at least one agent is configured (GitHub App triple or GitLab token pair), computed in the server component and passed down as `isAgentsVisible`
### `packages/shared/src/env.server.ts`
Added four new environment variables:
| Variable | Purpose |
|---|---|
| `GITLAB_REVIEW_AGENT_WEBHOOK_SECRET` | Verifies the `x-gitlab-token` header on incoming webhooks |
| `GITLAB_REVIEW_AGENT_TOKEN` | Personal or project access token used for GitLab API calls |
| `GITLAB_REVIEW_AGENT_HOST` | GitLab hostname (defaults to `gitlab.com`; set for self-hosted instances) |
| `REVIEW_AGENT_MODEL` | `displayName` of the configured language model to use for reviews; falls back to the first model if unset or not matched |
### `packages/web/package.json`
Added `@gitbeaker/rest` dependency (already used in `packages/backend`).
---
## Bug fixes
| Bug | Fix |
|---|---|
| `"Not authenticated"` when fetching file content from the review agent | `fetchFileContent` now calls `getFileSourceForRepo` directly instead of `getFileSource` (which gates on session auth) |
| `"diff_refs is missing"` when posting GitLab MR reviews | `gitlabMrParser` now fetches the full MR via `MergeRequests.show()` instead of relying on the webhook payload, which omits `diff_refs` on `update` events |
| Bedrock/Anthropic rejection: `"A conversation must start with a user message"` | `invokeDiffReviewLlm` now passes the prompt via `system` + a `prompt` user turn instead of a `system`-role entry inside `messages` |
| Review agent silently used `models[0]` with no way to specify a different model | New `REVIEW_AGENT_MODEL` env var selects by `displayName` |
fatmcgav
force-pushed
the
feat-gitlab-mr-agent
branch
from
967380d to
a824d7a
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/shared/src/env.server.ts (1)
201-201: Allowhost:portinGITLAB_REVIEW_AGENT_HOSTvalidation.Current validation rejects
:so values likegitlab.example.com:8443fail, even though downstream URL assembly supports them. Consider widening validation to accept optional ports.Suggested adjustment
- GITLAB_REVIEW_AGENT_HOST: z.string().default('gitlab.com').transform(s => s.replace(/^https?:\/\//, '').replace(/\/+$/, '')).refine(s => /^[a-z0-9.-]+$/i.test(s), { message: 'invalid hostname' }), + GITLAB_REVIEW_AGENT_HOST: z.string() + .default('gitlab.com') + .transform(s => s.trim().replace(/^https?:\/\//i, '').replace(/\/+$/, '')) + .refine( + s => /^(?:[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?\.)*[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?(?::\d{1,5})?$/i.test(s), + { message: 'invalid host[:port]' } + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/env.server.ts` at line 201, The GITLAB_REVIEW_AGENT_HOST zod schema currently rejects host:port because the refine regex disallows ':'; update the validation on GITLAB_REVIEW_AGENT_HOST (the .transform(...) and .refine(...) chain) to allow an optional port by changing the regex to accept an optional colon followed by digits (e.g. adjust the refine pattern from /^[a-z0-9.-]+$/i to something like /^[a-z0-9.-]+(?::\d+)?$/i), keep the existing transform that strips http(s):// and trailing slashes, and keep or update the refine error message to indicate "invalid hostname or port" if you prefer.packages/web/src/features/agents/review-agent/app.ts (1)
60-72: Remove the extra project-id source of truth.
mrPayload.project.idis already in scope andgitlabMrParser()uses that value. Keeping a separateprojectIdparameter makes it possible to parse one project and post comments to another if a caller ever passes mismatched values.♻️ Suggested simplification
export async function processGitLabMergeRequest( gitlabClient: InstanceType<typeof Gitlab>, - projectId: number, mrPayload: GitLabMergeRequestPayload, hostDomain: string, ) { @@ - await gitlabPushMrReviews(gitlabClient, projectId, prPayload, fileDiffReviews); + await gitlabPushMrReviews(gitlabClient, mrPayload.project.id, prPayload, fileDiffReviews); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/app.ts` around lines 60 - 72, The function processGitLabMergeRequest currently accepts a separate projectId parameter which duplicates mrPayload.project.id and risks mismatched usage; remove the projectId parameter from processGitLabMergeRequest's signature and update the body to derive the project id from mrPayload.project.id (e.g., use mrPayload.project.id when calling gitlabPushMrReviews), then update all call sites to stop passing projectId so the single source of truth is mrPayload; also ensure gitlabMrParser and any other usages still receive the correct values.packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.test.ts (1)
57-251: Add regression coverage for the new fallback branches ingitlabMrParser().The suite never exercises
mr.sha ?? last_commit.idormr.diff_refs == null, so the exact GitLab edge cases this PR hardened can regress unnoticed.🧪 Suggested tests
+ test('falls back to webhook last_commit.id when show() omits sha', async () => { + const client = makeMockGitlabClient([], { sha: undefined }); + const result = await gitlabMrParser(client, MOCK_MR_PAYLOAD, 'gitlab.com'); + expect(result.head_sha).toBe('abc123def456'); + }); + + test('omits diff_refs when show() returns null diff_refs', async () => { + const client = makeMockGitlabClient([], { diff_refs: null }); + const result = await gitlabMrParser(client, MOCK_MR_PAYLOAD, 'gitlab.com'); + expect(result.diff_refs).toBeUndefined(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.test.ts` around lines 57 - 251, The tests need to cover the new fallback branches in gitlabMrParser: add unit cases that (1) pass an MR payload where mr.sha is null and last_commit.id is present and assert the resulting head_sha equals last_commit.id, and (2) pass an MR payload where mr.diff_refs is null (or missing) and assert diff_refs on the result are derived via the parser's fallback logic; locate the test cases in the gitlabMrParser.test.ts suite and use the existing makeMockGitlabClient / MOCK_MR_PAYLOAD patterns, invoking gitlabMrParser(client, payload, host) and asserting on result.head_sha and result.diff_refs to ensure the fallbacks (mr.sha ?? last_commit.id and handling of mr.diff_refs == null) are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/shared/src/env.server.ts`:
- Line 201: The GITLAB_REVIEW_AGENT_HOST zod schema currently rejects host:port
because the refine regex disallows ':'; update the validation on
GITLAB_REVIEW_AGENT_HOST (the .transform(...) and .refine(...) chain) to allow
an optional port by changing the regex to accept an optional colon followed by
digits (e.g. adjust the refine pattern from /^[a-z0-9.-]+$/i to something like
/^[a-z0-9.-]+(?::\d+)?$/i), keep the existing transform that strips http(s)://
and trailing slashes, and keep or update the refine error message to indicate
"invalid hostname or port" if you prefer.
In `@packages/web/src/features/agents/review-agent/app.ts`:
- Around line 60-72: The function processGitLabMergeRequest currently accepts a
separate projectId parameter which duplicates mrPayload.project.id and risks
mismatched usage; remove the projectId parameter from
processGitLabMergeRequest's signature and update the body to derive the project
id from mrPayload.project.id (e.g., use mrPayload.project.id when calling
gitlabPushMrReviews), then update all call sites to stop passing projectId so
the single source of truth is mrPayload; also ensure gitlabMrParser and any
other usages still receive the correct values.
In `@packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.test.ts`:
- Around line 57-251: The tests need to cover the new fallback branches in
gitlabMrParser: add unit cases that (1) pass an MR payload where mr.sha is null
and last_commit.id is present and assert the resulting head_sha equals
last_commit.id, and (2) pass an MR payload where mr.diff_refs is null (or
missing) and assert diff_refs on the result are derived via the parser's
fallback logic; locate the test cases in the gitlabMrParser.test.ts suite and
use the existing makeMockGitlabClient / MOCK_MR_PAYLOAD patterns, invoking
gitlabMrParser(client, payload, host) and asserting on result.head_sha and
result.diff_refs to ensure the fallbacks (mr.sha ?? last_commit.id and handling
of mr.diff_refs == null) are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c140fde0-e600-49bc-ad81-f06bab25ab74
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (20)
.gitignoreCHANGELOG.mddocs/docs/features/agents/review-agent.mdxpackages/shared/src/env.server.tspackages/web/package.jsonpackages/web/src/app/(app)/agents/page.tsxpackages/web/src/app/(app)/components/navigationMenu/index.tsxpackages/web/src/app/(app)/components/navigationMenu/navigationItems.tsxpackages/web/src/app/api/(server)/webhook/route.tspackages/web/src/features/agents/review-agent/app.tspackages/web/src/features/agents/review-agent/nodes/fetchFileContent.tspackages/web/src/features/agents/review-agent/nodes/githubPrParser.test.tspackages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabMrParser.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabMrParser.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.tspackages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.tspackages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.tspackages/web/src/features/agents/review-agent/types.tspackages/web/src/features/git/getFileSourceApi.ts
✅ Files skipped from review due to trivial changes (5)
- packages/web/package.json
- .gitignore
- CHANGELOG.md
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.test.ts
- packages/web/src/features/agents/review-agent/nodes/githubPushPrReviews.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/web/src/app/(app)/components/navigationMenu/index.tsx
- packages/web/src/app/(app)/components/navigationMenu/navigationItems.tsx
- packages/web/src/app/(app)/agents/page.tsx
- packages/web/src/features/agents/review-agent/nodes/githubPrParser.test.ts
- packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.ts
- docs/docs/features/agents/review-agent.mdx
- packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.ts
- packages/web/src/app/api/(server)/webhook/route.ts
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.
Adds support for the AI Review Agent to review GitLab Merge Requests, mirroring the existing GitHub PR review functionality. Also fixes several bugs discovered during implementation and improves the shared review pipeline.
New files
packages/web/src/features/agents/review-agent/nodes/gitlabMrParser.tsParses a GitLab MR webhook payload into the shared
sourcebot_pr_payloadformat. CallsMergeRequests.show()andMergeRequests.allDiffs()in parallel — the API response is used fortitle,description,sha, anddiff_refs(which can be absent in webhook payloads forupdateaction events), while per-file diffs are parsed using the existingparse-difflibrary.packages/web/src/features/agents/review-agent/nodes/gitlabPushMrReviews.tsPosts review comments back to GitLab using
MergeRequestDiscussions.create()with a position object carryingbase_sha,head_sha, andstart_sha. Falls back toMergeRequestNotes.create()(a general MR note) if the inline comment is rejected by the API (e.g. the line is not within the diff), ensuring reviews are always surfaced even when precise positioning fails.Test files (4 new files, 34 tests total)
githubPrParser.test.ts— diff parsing and metadata mapping for the GitHub parsergithubPushPrReviews.test.ts— single-line vs multi-line comment parameters, error resiliencegitlabMrParser.test.ts— API call arguments, metadata mapping, diff parsing, edge cases (empty diffs, nested groups, null description, API failures)gitlabPushMrReviews.test.ts— inline comment posting, fallback behaviour, missingdiff_refsguard, multi-file iterationModified files
packages/web/src/features/agents/review-agent/types.tssourcebot_diff_refsschema/type (base_sha,head_sha,start_sha) and an optionaldiff_refsfield onsourcebot_pr_payloadGitLabMergeRequestPayloadandGitLabNotePayloadinterfaces for webhook event typingpackages/web/src/features/agents/review-agent/app.tsprocessGitLabMergeRequest()function mirroringprocessGitHubPullRequest(): sets up logging, runs the GitLab parser, generates reviews via the shared LLM pipeline, and pushes resultsOPENAI_API_KEYguards (model availability is now enforced insideinvokeDiffReviewLlm)packages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.tsReplaces the hardcoded OpenAI client with the Vercel AI SDK's
generateTextand the sharedgetAISDKLanguageModelAndOptions/getConfiguredLanguageModelsutilities fromchat/utils.server.ts. The review agent now uses whichever language model is configured inconfig.json, supporting all providers (Anthropic, Bedrock, Azure, etc.).REVIEW_AGENT_MODELenv var (matched againstdisplayName) selects a specific model when multiple are configured; falls back tomodels[0]with a warning if the name is not foundsystemparameter with a"Review the code changes."user turn, satisfying providers (e.g. Bedrock/Anthropic) that require conversations to begin with a user messagepackages/web/src/features/agents/review-agent/nodes/fetchFileContent.tsFixes "Not authenticated" error when the review agent calls
getFileSource. The original implementation usedwithOptionalAuth, which reads a session cookie — absent in webhook handlers. Now callsgetFileSourceForRepodirectly with__unsafePrismaand the single-tenant org, bypassing the session-based auth layer. The webhook handler has already authenticated the request via its own mechanism (GitHub App signature / GitLab token).packages/web/src/features/git/getFileSourceApi.tsgetFileSourceForRepo({ path, repo, ref }, { org, prisma })functiongetFileSourcenow handles auth and audit logging then delegates togetFileSourceForRepo— all existing callers are unchangedpackages/web/src/app/api/(server)/webhook/route.tsx-gitlab-tokenagainstGITLAB_REVIEW_AGENT_WEBHOOK_SECRETMerge Request Hookevents (auto-review onopen,update,reopen) andNote Hookevents (manual/reviewcommand on MR comments)Gitlabclient at module load ifGITLAB_REVIEW_AGENT_TOKENis setpackages/web/src/app/(app)/agents/page.tsxOPENAI_API_KEYfrom the GitHub card's required env vars (no longer applicable)packages/web/src/app/(app)/components/navigationMenu/navigationItems.tsx&index.tsxBotIcon) between Repositories and SettingsisAgentsVisiblepackages/shared/src/env.server.tsAdded four new environment variables:
GITLAB_REVIEW_AGENT_WEBHOOK_SECRETx-gitlab-tokenheader on incoming webhooksGITLAB_REVIEW_AGENT_TOKENGITLAB_REVIEW_AGENT_HOSTgitlab.com; set for self-hosted instances)REVIEW_AGENT_MODELdisplayNameof the configured language model to use for reviews; falls back to the first model if unset or not matchedpackages/web/package.jsonAdded
@gitbeaker/restdependency (already used inpackages/backend).Bug fixes
"Not authenticated"when fetching file content from the review agentfetchFileContentnow callsgetFileSourceForRepodirectly instead ofgetFileSource(which gates on session auth)"diff_refs is missing"when posting GitLab MR reviewsgitlabMrParsernow fetches the full MR viaMergeRequests.show()instead of relying on the webhook payload, which omitsdiff_refsonupdateevents"A conversation must start with a user message"invokeDiffReviewLlmnow passes the prompt viasystem+ apromptuser turn instead of asystem-role entry insidemessagesmodels[0]with no way to specify a different modelREVIEW_AGENT_MODELenv var selects bydisplayNameSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores