feat(web): customisable AI code review agent configs#1143
Open
fatmcgav wants to merge 2 commits intosourcebot-dev:mainfrom
Open
feat(web): customisable AI code review agent configs#1143fatmcgav wants to merge 2 commits intosourcebot-dev:mainfrom
fatmcgav wants to merge 2 commits intosourcebot-dev:mainfrom
Conversation
Adds a per-org/connection/repo agent config system for the code review agent. Database: - New `AgentConfig` model with `AgentType`, `AgentScope`, and `PromptMode` enums - Junction tables `AgentConfigToRepo` and `AgentConfigToConnection` for scope targeting - Migration: `20260421203635_add_agent_config` Review agent: - Priority resolver: REPO > CONNECTION > ORG > system defaults - `resolveRules()`: APPEND or REPLACE custom prompt against built-in rules - `parseAgentConfigSettings()`: validates per-config JSON settings - `webhookUtils`: `isAutoReviewEnabled` and `getReviewCommand` respect per-config overrides - `invokeDiffReviewLlm`: accepts per-config model override - `fetchContextFile`: fetches repo-level instruction files (e.g. AGENTS.md) once per PR and injects as context for every diff hunk; supports comma/space-separated list of paths; missing files silently ignored - Webhook route wires resolved config through to all processing functions API: - `GET/POST /api/agents` — list and create agent configs - `GET/PATCH/DELETE /api/agents/[agentId]` — manage individual configs - Full OpenAPI registration with request/response schemas UI: - Agents list page shows all configs with scope/type/enabled badges - Create and edit forms with scope picker, repo/connection checkbox lists with filter inputs, custom prompt with APPEND/REPLACE mode, and settings overrides (model dropdown, review command, auto-review, context files) Tests: - 60 unit tests across resolveAgentConfig, app, webhookUtils, and the agents API route Docs: - Updated review-agent.mdx with agent configs, custom prompt, and context files sections - Env var table updated to note per-config override support Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
WalkthroughAdds a full agent-configuration system for the CODE_REVIEW agent: DB schema, Prisma models, REST CRUD endpoints, OpenAPI schemas, admin UI pages and form, resolution logic, webhook integration, review-agent runtime changes (prompt/model/context overrides), and tests/docs. Changes
Sequence DiagramsequenceDiagram
participant Webhook as GitHub/GitLab Webhook
participant Route as Webhook Route Handler
participant ConfigResolver as resolveAgentConfig()
participant Database as Prisma/Database
participant ReviewAgent as Review Agent Processor
participant LLM as LLM Service
Webhook->>Route: POST webhook (PR/MR event)
Route->>ConfigResolver: resolveAgentConfig(repoId, orgId, CODE_REVIEW)
ConfigResolver->>Database: Query REPO-scoped config
alt REPO config found
Database-->>ConfigResolver: AgentConfig
ConfigResolver-->>Route: AgentConfig
else Check CONNECTION scope
ConfigResolver->>Database: Query CONNECTION-scoped config via connection.repos
Database-->>ConfigResolver: AgentConfig | null
ConfigResolver-->>Route: AgentConfig | null
else Check ORG scope
ConfigResolver->>Database: Query ORG-scoped config
Database-->>ConfigResolver: AgentConfig | null
ConfigResolver-->>Route: AgentConfig | null
end
Route->>ReviewAgent: processPullRequest(config)
ReviewAgent->>ReviewAgent: resolveRules(config)
ReviewAgent->>ReviewAgent: parseAgentConfigSettings(config.settings)
ReviewAgent->>ReviewAgent: fetchContextFiles(settings.contextFiles)
ReviewAgent->>LLM: invokeDiffReviewLlm(prompt, modelOverride)
LLM-->>ReviewAgent: Review results
ReviewAgent-->>Webhook: Post review/comments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
|
Couple of screenshots showing the new UI functionality: |
![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: 20
🧹 Nitpick comments (6)
packages/db/prisma/migrations/20260421203635_add_agent_config/migration.sql (1)
29-42: Missing reverse-FK indexes on join tables.Same as noted on
schema.prisma:AgentConfigToRepo(repoId)andAgentConfigToConnection(connectionId)are only indexed as the 2nd column of the composite PK, so cascade deletes fromRepo/Connection(andresolveAgentConfiglookups by repo/connection) will seq-scan. Add indexes via the Prisma model, which will regenerate the migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/prisma/migrations/20260421203635_add_agent_config/migration.sql` around lines 29 - 42, The join tables AgentConfigToRepo and AgentConfigToConnection lack single-column indexes on repoId and connectionId, causing seq-scans for deletes/lookups; update the Prisma models for AgentConfigToRepo and AgentConfigToConnection to add explicit @@index([...]) or `@unique/`@index on the repoId and connectionId fields (respectively) so Prisma regenerates a migration that creates indexes on repoId and connectionId in the SQL, then run prisma migrate to emit the updated migration SQL.packages/db/prisma/schema.prisma (1)
641-660: Consider adding indexes onrepoId/connectionIdfor join tables.
AgentConfigToRepoandAgentConfigToConnectiononly have composite PKs(agentConfigId, repoId/connectionId), which PostgreSQL indexes on the leading column only. Cascade deletes triggered fromRepo/Connectionand reverse lookups (e.g., fromresolveAgentConfiggiven a repoId) will have to seq-scan the join table. Adding@@index([repoId])and@@index([connectionId])respectively mirrors whatRepoToConnectiondoes (@@index([repoId, connectionId])) and keeps cascade deletes fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/prisma/schema.prisma` around lines 641 - 660, The join models AgentConfigToRepo and AgentConfigToConnection currently only have composite primary keys (agentConfigId, repoId) and (agentConfigId, connectionId) which index the leading column only; add explicit secondary indexes by adding @@index([repoId]) to the AgentConfigToRepo model and @@index([connectionId]) to the AgentConfigToConnection model so lookups and cascade deletes from Repo/Connection are efficient (mirror the existing pattern used by RepoToConnection with @@index([repoId, connectionId])).packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts (1)
10-23: Module-levelcachedOrgis shared across PRs and never invalidated.Not new to this PR, but now exercised on every diff hunk via
fetchContextFile. If the Org row changes (e.g., rename, metadata updates), the process keeps the stale copy until restart. Consider either fetching per call (the lookup is cheap by PK) or adding a short TTL. Happy to defer if intentional.🤖 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 10 - 23, The module-level cachedOrg used by getOrg (backed by __unsafePrisma.org.findUnique and SINGLE_TENANT_ORG_ID) is never invalidated and can serve stale org data; update getOrg to avoid a long-lived module cache: either remove cachedOrg and always call __unsafePrisma.org.findUnique by PK inside getOrg, or implement a short TTL-based cache (store timestamp alongside cachedOrg and refresh when expired) so renames/metadata updates are observed without requiring a process restart. Ensure error handling (throwing "Organization not found") remains when lookup returns null.docs/docs/features/agents/review-agent.mdx (2)
169-171: Use second person for override descriptions.The repeated “Can be overridden...” fragments are passive. Use direct second-person wording.
Proposed wording cleanup
-| `REVIEW_AGENT_AUTO_REVIEW_ENABLED` | `false` | Automatically review new and updated PRs/MRs. Can be overridden per agent config. | -| `REVIEW_AGENT_REVIEW_COMMAND` | `review` | Comment command that triggers a manual review (without the `/`). Can be overridden per agent config. | -| `REVIEW_AGENT_MODEL` | first configured model | `displayName` of the language model to use for reviews. Can be overridden per agent config. | +| `REVIEW_AGENT_AUTO_REVIEW_ENABLED` | `false` | Automatically review new and updated PRs/MRs. You can override this per agent config. | +| `REVIEW_AGENT_REVIEW_COMMAND` | `review` | Comment command that triggers a manual review, without the `/`. You can override this per agent config. | +| `REVIEW_AGENT_MODEL` | first configured model | `displayName` of the language model to use for reviews. You can override this per agent config. |As per coding guidelines,
docs/**/*.mdxfiles should use second person present tense.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/features/agents/review-agent.mdx` around lines 169 - 171, The docs use passive phrasing for override notes; update the three rows for REVIEW_AGENT_AUTO_REVIEW_ENABLED, REVIEW_AGENT_REVIEW_COMMAND, and REVIEW_AGENT_MODEL so the override note uses second-person present tense (e.g., change "Can be overridden per agent config." to "You can override this in the agent config." or similar) to match the docs guideline for docs/**/*.mdx.
142-163: Replace em dashes and hyphenate the separator phrase.The new section uses em dashes and “comma or space separated”. Please use commas/colons and
comma- or space-separatedinstead.Proposed wording cleanup
-Agent configs let you customise how the review agent behaves per repository, connection, or your whole org. You can override the model, review command, auto-review behaviour, custom prompt, and context files — all without changing environment variables. +Agent configs let you customise how the review agent behaves per repository, connection, or your whole org. You can override the model, review command, auto-review behaviour, custom prompt, and context files without changing environment variables. ... -- **Repo** — applies to specific repositories (highest priority) -- **Connection** — applies to all repos in a specific connection -- **Org** — applies to all repositories in your org (lowest priority, catch-all) +- **Repo**: Applies to specific repositories. This has the highest priority. +- **Connection**: Applies to all repos in a specific connection. +- **Org**: Applies to all repositories in your org. This has the lowest priority. ... -- **Append** (default) — your instructions are added after the built-in review rules. -- **Replace** — your instructions entirely replace the built-in rules. Use this when you want full control over what the agent looks for. +- **Append** (default): Your instructions are added after the built-in review rules. +- **Replace**: Your instructions replace the built-in rules. Use this when you want full control over what the agent looks for. ... -You can configure one or more repository files to be fetched at review time and injected as additional context for the model. This is useful for encoding project-specific conventions that the model should be aware of when reviewing diffs — for example, preferred error handling patterns, style rules, or areas of the codebase that need extra scrutiny. +You can configure one or more repository files to be fetched at review time and injected as additional context for the model. This is useful for project-specific conventions, such as preferred error handling patterns, style rules, or areas of the codebase that need extra scrutiny. ... -Set **Context files** in the agent config form to a comma or space separated list of paths relative to the repository root (e.g. `AGENTS.md .sourcebot/review.md`). Files that do not exist in the repository are silently ignored. The files are fetched once per PR from the head commit and included in the context for every diff hunk. +Set **Context files** in the agent config form to a comma- or space-separated list of paths relative to the repository root (e.g. `AGENTS.md .sourcebot/review.md`). Files that do not exist in the repository are silently ignored. The files are fetched once per PR from the head commit and included in the context for every diff hunk.As per coding guidelines,
docs/**/*.mdxfiles must not use em dashes and should keep sentences short.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/features/agents/review-agent.mdx` around lines 142 - 163, In the "Agent configs" intro and the "Context files" paragraph (look for the headings/phrases "Agent configs let you customise...", "custom prompt", and the sentence beginning "Set **Context files**..."), replace em dashes with commas or a colon and change "comma or space separated" to the hyphenated phrase "comma- or space-separated"; ensure the sentence that currently reads "...context files — all without changing environment variables." becomes something like "...context files, all without changing environment variables." and the "Set **Context files**..." line reads with "comma- or space-separated list" instead of the em dash and unhyphenated phrase.packages/web/src/features/agents/review-agent/app.test.ts (1)
51-63: CovercontextFilesas a recognized setting.The parser tests should include the new context-file override so schema regressions are caught alongside
model,reviewCommand, andautoReviewEnabled.🧪 Proposed test update
const result = parseAgentConfigSettings({ autoReviewEnabled: false, reviewCommand: 'check', model: 'claude-sonnet-4-6', + contextFiles: 'AGENTS.md .sourcebot/review.md', }); expect(result).toEqual({ autoReviewEnabled: false, reviewCommand: 'check', model: 'claude-sonnet-4-6', + contextFiles: 'AGENTS.md .sourcebot/review.md', });🤖 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.test.ts` around lines 51 - 63, The test for parseAgentConfigSettings is missing the new contextFiles setting; update the test 'parses all recognised fields' to include a sample contextFiles value in the input and expected output so the parser is validated for the context-file override (reference parseAgentConfigSettings and the contextFiles property).
🤖 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/`(app)/agents/configs/[agentId]/page.tsx:
- Line 10: The page currently allows any authenticated user to edit agent
configs; update the authenticatedPage call to require org owners by passing the
options object { minRole: OrgRole.OWNER, redirectTo: '/settings' } to
authenticatedPage (i.e., change authenticatedPage(async (...) => ...) to
authenticatedPage({ minRole: OrgRole.OWNER, redirectTo: '/settings' }, async
(...) => ...)); also ensure OrgRole is imported where used (add import for
OrgRole if missing) so non-owners are redirected to /settings.
In `@packages/web/src/app/`(app)/agents/configs/agentConfigForm.tsx:
- Around line 89-94: The form is submitting settings.contextFiles but the
validation schema agentConfigSettingsSchema doesn't include contextFiles, so
those values are dropped; update the agentConfigSettingsSchema to add a
contextFiles property with the correct type/shape (e.g., array of file
identifiers or objects matching what the form sends) and default/optional rules,
and ensure any POST/PATCH validators or types that consume
agentConfigSettingsSchema (and any related request DTOs or parse/validate calls)
are updated to accept and persist settings.contextFiles (look for usages of
agentConfigSettingsSchema and the settings object in the agent save/update
handlers).
In `@packages/web/src/app/`(app)/agents/configs/new/page.tsx:
- Line 5: Update UI and server to require OrgRole.OWNER for agent config
creation/editing: change the page export that currently calls
authenticatedPage(...) to pass the options { minRole: OrgRole.OWNER, redirectTo:
'/settings' } (look for authenticatedPage in the new page and the edit page
under the agents config routes). In the API handler that uses withAuth (the POST
handler wrapped by apiHandler), replace or wrap the authorization check with
withMinimumOrgRole(OrgRole.OWNER, async ({ org, prisma }) => { ...existing POST
logic... }) so only owners can create configs; ensure you keep existing request
handling inside that callback. Also apply the same authenticatedPage gating to
the edit page (agents/configs/[agentId]/page) so UI and API enforce the same
minimum role.
In `@packages/web/src/app/`(app)/agents/page.tsx:
- Line 37: The page is exposing agent configs to all authenticated org members;
update the authenticatedPage call to require owner role by passing options like
{ minRole: OrgRole.OWNER, redirectTo: '/settings' } so only org owners can
access it; modify the existing authenticatedPage(...) invocation (the exported
default function) to include these options and ensure OrgRole is imported where
used.
In `@packages/web/src/app/api/`(server)/agents/[agentId]/route.ts:
- Around line 117-140: Detect when scope changes by first fetching the existing
agent config (e.g., via prisma.agentConfig.findUnique) and, inside the update
for prisma.agentConfig.update, ensure stale junction rows are cleared: always
include repos.deleteMany when new scope is CONNECTION or ORG (or when switching
away from REPO) unless repoIds are explicitly provided, and always include
connections.deleteMany when new scope is REPO or ORG (or when switching away
from CONNECTION) unless connectionIds are explicitly provided; reference the
existing scope, the new scope variable (scope), and the update block that
currently uses repos/create and connections/create to conditionally add
deleteMany entries so stale relations are removed even if the caller did not
supply repoIds/connectionIds.
- Around line 117-140: Before calling prisma.agentConfig.update, validate that
every id in repoIds and connectionIds belongs to the current org by querying the
corresponding tables and comparing counts/returned ids; if any provided id is
not owned by org.id return a 400 error. Specifically, when handling repoIds and
connectionIds in the update block (the code that constructs the nested repos and
connections create/deleteMany objects passed to prisma.agentConfig.update),
first run lookups like fetching repos with where: { id: { in: repoIds }, orgId }
and connections with where: { id: { in: connectionIds }, orgId }, ensure the
result length equals the supplied array length (or compute the set difference to
identify invalid ids) and abort with an appropriate error if mismatches exist;
only then include the repos: { deleteMany: {}, create: [...] } and connections:
{ deleteMany: {}, create: [...] } sections.
- Line 88: The PATCH and DELETE handlers currently call withAuth (e.g., the call
to withAuth at the start of the mutation handlers) which only rejects
unauthenticated/guest users; replace those calls with withMinimumOrgRole to
enforce owner/admin permissions. Specifically, swap uses of withAuth with
withMinimumOrgRole (requiring "owner" or "admin" per policy) in the agent
mutation paths (the invocation around withAuth in the mutation at the position
referenced and the similar call around line 162) so that the functions handling
PATCH and DELETE require the minimum org role instead of just authentication.
- Around line 89-116: Before performing the update, check whether the incoming
name (e.g., the request's name field) is being changed to a value that already
exists for another agent config in the same org and return a 409 Conflict
instead of letting the DB unique-constraint error surface; specifically, in the
PATCH handler in route.ts (after loading existing via
prisma.agentConfig.findFirst and before the try-update block), query
prisma.agentConfig.findFirst where name equals the requested name and orgId
equals org.id and id is not agentId, and if found return a 409 response
(StatusCodes.CONFLICT) with an appropriate errorCode and message indicating the
name collision.
- Around line 15-19: The PATCH validation schema agentConfigSettingsSchema
currently only allows autoReviewEnabled, reviewCommand, and model, so
settings.contextFiles from the form is dropped; update agentConfigSettingsSchema
to include a contextFiles field (e.g., contextFiles:
z.array(z.string()).optional()) so incoming settings.contextFiles are validated
and preserved by the PATCH handler that consumes agentConfigSettingsSchema.
In `@packages/web/src/app/api/`(server)/agents/route.test.ts:
- Around line 91-93: The "name collision" describe block fails because prior
tests leave Prisma mocks called; inside the describe that contains the
name-collision tests add a local beforeEach(() => vi.clearAllMocks()) so
agentConfig.create (and other mocks) have cleared call history before each test;
target the describe block that asserts agentConfig.create is not called and add
this beforeEach to ensure the assertion is reliable.
In `@packages/web/src/app/api/`(server)/agents/route.ts:
- Line 78: The route currently uses withAuth to allow any authenticated member
to create org-level review-agent configs; change the authorization wrapper to
withMinimumOrgRole and require the appropriate elevated role (e.g., "owner" or
"admin") so only org owners/admins can perform this mutation. Locate the call to
withAuth in the agents route handler (the async callback that returns result)
and replace it with withMinimumOrgRole, passing the required role constant/enum
and the same handler; ensure the handler still receives the org and prisma
parameters and that error handling remains unchanged.
- Around line 92-126: Before calling prisma.agentConfig.create, verify that all
repoIds (when scope === AgentScope.REPO) and connectionIds (when scope ===
AgentScope.CONNECTION) belong to the current org (org.id): query the repos table
for repoIds with where: { id: { in: repoIds }, orgId: org.id } and the
connections table similarly, compare the returned count to
repoIds.length/connectionIds.length and return StatusCodes.BAD_REQUEST +
ErrorCode.INVALID_REQUEST_BODY if any IDs are missing/invalid; only then pass
the nested repos: { create: ... } or connections: { create: ... } to
prisma.agentConfig.create so you avoid cross-org associations or raw Prisma
errors.
- Around line 16-20: The agentConfigSettingsSchema currently lacks the
contextFiles field so submitted contextFiles from the UI are dropped; update
agentConfigSettingsSchema to include a contextFiles property (optional) that
matches the shape used by the UI/review agent (e.g., an optional array of
strings or objects as expected by the review flow) so validation preserves and
returns contextFiles to the review agent; modify the schema defined in
agentConfigSettingsSchema to add this contextFiles entry to align with the
review agent's expected input.
In `@packages/web/src/app/api/`(server)/webhook/route.ts:
- Around line 147-160: The code currently resolves agent configs using
SINGLE_TENANT_ORG_ID which forces lookups into the single-tenant org; update the
logic so that after finding the repo record(s) you call resolveAgentConfig with
the matched repo.orgId instead of SINGLE_TENANT_ORG_ID (use the repo variable
returned by __unsafePrisma.repo.findFirst or change to findMany and iterate if
you need to handle multiple matching repo records), and apply the same change to
resolveGitLabAgentConfig so configs are resolved against the actual orgId(s) of
the repo(s).
In `@packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts`:
- Around line 56-93: The docstring promises content is capped at
CONTEXT_FILE_MAX_BYTES but fetchContextFile currently returns the full
parsed.data.source; update fetchContextFile (the function named fetchContextFile
in this diff) to enforce the cap: measure the UTF-8 byte length of
parsed.data.source (use Buffer.byteLength), and if it exceeds
CONTEXT_FILE_MAX_BYTES, truncate the string to fit that many bytes without
breaking multi-byte UTF-8 characters (iterate/truncate safely or use a helper to
slice by bytes), replace content with the truncated version, and add a debug/log
message indicating truncation occurred; ensure the returned context uses the
possibly-truncated content.
In `@packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts`:
- Around line 16-18: The Promise.all call that builds repoInstructionsContexts
lets a single fetchContextFile throw aborting the whole operation; change to
handle failures per-file by mapping contextFilePaths to per-path try/catch (or
use Promise.allSettled) and call fetchContextFile(pr_payload, p) inside each
handler, skipping or returning null on error, then filter with the existing type
guard to produce repoInstructionsContexts; keep symbols
repoInstructionsContexts, fetchContextFile, contextFilePaths and pr_payload to
locate and update the logic.
In `@packages/web/src/features/agents/review-agent/resolveAgentConfig.ts`:
- Around line 21-70: The findFirst queries (prisma.agentConfig.findFirst) that
populate repoConfig, connectionConfig, and orgConfig are non-deterministic when
multiple enabled configs match; update each query to include an orderBy: {
updatedAt: 'desc' } so the most recently updated config is consistently selected
(or alternatively enforce uniqueness on the combination of
orgId,type,enabled,scope if you want to prevent duplicates); locate the three
queries that assign repoConfig, connectionConfig, and orgConfig and add the
orderBy clause to each.
In `@packages/web/src/features/agents/review-agent/webhookUtils.test.ts`:
- Around line 57-60: The tests currently treat an undefined environment flag as
enabling auto-review; update them to reflect the documented default of disabled
by asserting false. For the tests that call isAutoReviewEnabled with a null/no
config (e.g., the test titled "returns true when no config and env flag is not
set" and the similar cases around the 87-102 block), set
mocks.env.REVIEW_AGENT_AUTO_REVIEW_ENABLED = undefined (or leave it unset) and
change the expectation to expect(isAutoReviewEnabled(null)).toBe(false); apply
the same change to the other affected test cases to ensure the default is
disabled.
In `@packages/web/src/features/agents/review-agent/webhookUtils.ts`:
- Line 17: The current check returns true when
env.REVIEW_AGENT_AUTO_REVIEW_ENABLED is undefined, enabling auto-review by
default; change the condition so auto-review is enabled only when
REVIEW_AGENT_AUTO_REVIEW_ENABLED is explicitly set to "true" (e.g., replace the
loose inequality check of env.REVIEW_AGENT_AUTO_REVIEW_ENABLED with a strict
comparison to "true" or equivalent explicit boolean parsing) so the expression
using env.REVIEW_AGENT_AUTO_REVIEW_ENABLED only returns true when the flag
equals "true".
In `@packages/web/src/openapi/publicApiSchemas.ts`:
- Around line 59-63: The publicAgentConfigSettingsSchema is missing the
contextFiles field used by the review agent; update the z.object for
publicAgentConfigSettingsSchema to include a new optional contextFiles property
typed as an array of strings (e.g., z.array(z.string()).optional()) and add a
descriptive .describe(...) (e.g., "List of file paths or glob patterns to
include as context for reviews; overrides the REVIEW_AGENT_CONTEXT_FILES env
var") so the OpenAPI output and clients can discover and validate this setting.
---
Nitpick comments:
In `@docs/docs/features/agents/review-agent.mdx`:
- Around line 169-171: The docs use passive phrasing for override notes; update
the three rows for REVIEW_AGENT_AUTO_REVIEW_ENABLED,
REVIEW_AGENT_REVIEW_COMMAND, and REVIEW_AGENT_MODEL so the override note uses
second-person present tense (e.g., change "Can be overridden per agent config."
to "You can override this in the agent config." or similar) to match the docs
guideline for docs/**/*.mdx.
- Around line 142-163: In the "Agent configs" intro and the "Context files"
paragraph (look for the headings/phrases "Agent configs let you customise...",
"custom prompt", and the sentence beginning "Set **Context files**..."), replace
em dashes with commas or a colon and change "comma or space separated" to the
hyphenated phrase "comma- or space-separated"; ensure the sentence that
currently reads "...context files — all without changing environment variables."
becomes something like "...context files, all without changing environment
variables." and the "Set **Context files**..." line reads with "comma- or
space-separated list" instead of the em dash and unhyphenated phrase.
In `@packages/db/prisma/migrations/20260421203635_add_agent_config/migration.sql`:
- Around line 29-42: The join tables AgentConfigToRepo and
AgentConfigToConnection lack single-column indexes on repoId and connectionId,
causing seq-scans for deletes/lookups; update the Prisma models for
AgentConfigToRepo and AgentConfigToConnection to add explicit @@index([...]) or
`@unique/`@index on the repoId and connectionId fields (respectively) so Prisma
regenerates a migration that creates indexes on repoId and connectionId in the
SQL, then run prisma migrate to emit the updated migration SQL.
In `@packages/db/prisma/schema.prisma`:
- Around line 641-660: The join models AgentConfigToRepo and
AgentConfigToConnection currently only have composite primary keys
(agentConfigId, repoId) and (agentConfigId, connectionId) which index the
leading column only; add explicit secondary indexes by adding @@index([repoId])
to the AgentConfigToRepo model and @@index([connectionId]) to the
AgentConfigToConnection model so lookups and cascade deletes from
Repo/Connection are efficient (mirror the existing pattern used by
RepoToConnection with @@index([repoId, connectionId])).
In `@packages/web/src/features/agents/review-agent/app.test.ts`:
- Around line 51-63: The test for parseAgentConfigSettings is missing the new
contextFiles setting; update the test 'parses all recognised fields' to include
a sample contextFiles value in the input and expected output so the parser is
validated for the context-file override (reference parseAgentConfigSettings and
the contextFiles property).
In `@packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts`:
- Around line 10-23: The module-level cachedOrg used by getOrg (backed by
__unsafePrisma.org.findUnique and SINGLE_TENANT_ORG_ID) is never invalidated and
can serve stale org data; update getOrg to avoid a long-lived module cache:
either remove cachedOrg and always call __unsafePrisma.org.findUnique by PK
inside getOrg, or implement a short TTL-based cache (store timestamp alongside
cachedOrg and refresh when expired) so renames/metadata updates are observed
without requiring a process restart. Ensure error handling (throwing
"Organization not found") remains when lookup returns null.
🪄 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: 3f53e2ce-1434-46ad-90f6-ed02e8835240
📒 Files selected for processing (25)
docs/api-reference/sourcebot-public.openapi.jsondocs/docs/features/agents/review-agent.mdxpackages/db/prisma/migrations/20260421203635_add_agent_config/migration.sqlpackages/db/prisma/schema.prismapackages/web/src/app/(app)/agents/configs/[agentId]/page.tsxpackages/web/src/app/(app)/agents/configs/agentConfigForm.tsxpackages/web/src/app/(app)/agents/configs/new/page.tsxpackages/web/src/app/(app)/agents/page.tsxpackages/web/src/app/api/(server)/agents/[agentId]/route.tspackages/web/src/app/api/(server)/agents/route.test.tspackages/web/src/app/api/(server)/agents/route.tspackages/web/src/app/api/(server)/webhook/route.tspackages/web/src/features/agents/review-agent/app.test.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/generatePrReview.tspackages/web/src/features/agents/review-agent/nodes/invokeDiffReviewLlm.tspackages/web/src/features/agents/review-agent/resolveAgentConfig.test.tspackages/web/src/features/agents/review-agent/resolveAgentConfig.tspackages/web/src/features/agents/review-agent/types.tspackages/web/src/features/agents/review-agent/webhookUtils.test.tspackages/web/src/features/agents/review-agent/webhookUtils.tspackages/web/src/lib/errorCodes.tspackages/web/src/openapi/publicApiDocument.tspackages/web/src/openapi/publicApiSchemas.ts
| params: Promise<{ agentId: string }>; | ||
| }; | ||
|
|
||
| export default authenticatedPage(async ({ prisma, org }, { params }: Props) => { |
Contributor
There was a problem hiding this comment.
Gate config editing to org owners.
This page exposes the edit form and config details with plain authentication. Add the owner-role option to authenticatedPage so non-owners cannot access agent configuration management.
Proposed fix
+import { OrgRole } from "@sourcebot/db";
...
-export default authenticatedPage(async ({ prisma, org }, { params }: Props) => {
+export default authenticatedPage(async ({ prisma, org }, { params }: Props) => {
const { agentId } = await params;
...
-});
+}, { minRole: OrgRole.OWNER, redirectTo: "/settings" });Based on learnings: Use authenticatedPage with { minRole: OrgRole.OWNER, redirectTo: '/settings' } option to gate pages by role in the (app) route group.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export default authenticatedPage(async ({ prisma, org }, { params }: Props) => { | |
| import { OrgRole } from "@sourcebot/db"; | |
| // ... other imports ... | |
| export default authenticatedPage(async ({ prisma, org }, { params }: Props) => { | |
| const { agentId } = await params; | |
| // ... rest of the function body ... | |
| }, { minRole: OrgRole.OWNER, redirectTo: "/settings" }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/app/`(app)/agents/configs/[agentId]/page.tsx at line 10, The
page currently allows any authenticated user to edit agent configs; update the
authenticatedPage call to require org owners by passing the options object {
minRole: OrgRole.OWNER, redirectTo: '/settings' } to authenticatedPage (i.e.,
change authenticatedPage(async (...) => ...) to authenticatedPage({ minRole:
OrgRole.OWNER, redirectTo: '/settings' }, async (...) => ...)); also ensure
OrgRole is imported where used (add import for OrgRole if missing) so non-owners
are redirected to /settings.
Comment on lines
+89
to
+94
| settings: { | ||
| ...(autoReviewEnabled !== undefined && { autoReviewEnabled }), | ||
| ...(reviewCommand && { reviewCommand }), | ||
| ...(model && { model }), | ||
| ...(contextFiles && { contextFiles }), | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether contextFiles is accepted by the agents API schemas.
rg -n -C4 'contextFiles|agentConfigSettingsSchema|createAgentConfigBodySchema|updateAgentConfigBodySchema' \
--glob 'packages/web/src/app/api/**/*.ts' \
--glob 'packages/web/src/app/(app)/agents/**/*.tsx'Repository: sourcebot-dev/sourcebot
Length of output: 9864
Add contextFiles to the agent config settings schema before shipping this field.
The form submits settings.contextFiles (line 93), but the agents POST/PATCH schemas define agentConfigSettingsSchema with only autoReviewEnabled, reviewCommand, and model. Users can enter context files but the setting will be rejected or lost on save.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/app/`(app)/agents/configs/agentConfigForm.tsx around lines
89 - 94, The form is submitting settings.contextFiles but the validation schema
agentConfigSettingsSchema doesn't include contextFiles, so those values are
dropped; update the agentConfigSettingsSchema to add a contextFiles property
with the correct type/shape (e.g., array of file identifiers or objects matching
what the form sends) and default/optional rules, and ensure any POST/PATCH
validators or types that consume agentConfigSettingsSchema (and any related
request DTOs or parse/validate calls) are updated to accept and persist
settings.contextFiles (look for usages of agentConfigSettingsSchema and the
settings object in the agent save/update handlers).
| import { NavigationMenu } from "@/app/(app)/components/navigationMenu"; | ||
| import { AgentConfigForm } from "../agentConfigForm"; | ||
|
|
||
| export default authenticatedPage(async ({ prisma, org }) => { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'withMinimumOrgRole|OrgRole\.' packages/web/src/app/api/\(server\)/agents
fd -t f 'page.tsx' packages/web/src/app/\(app\)/agents | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'Repository: sourcebot-dev/sourcebot
Length of output: 14689
🏁 Script executed:
cat packages/web/src/app/api/\(server\)/agents/route.tsRepository: sourcebot-dev/sourcebot
Length of output: 5751
Gate this page and the API with a minimum org role.
Agent config creation is a privileged operation that can change prompts, models, and auto-review behavior org-wide. Using authenticatedPage with { minRole: OrgRole.OWNER, redirectTo: '/settings' } will gate the page, but the API endpoint also needs role enforcement. The POST /api/agents handler currently uses only withAuth without withMinimumOrgRole, allowing any authenticated org member to create configs. Both UI and API must enforce the same role requirement.
Suggested diff for page.tsx
-export default authenticatedPage(async ({ prisma, org }) => {
+export default authenticatedPage(async ({ prisma, org }) => {
...
-});
+}, { minRole: OrgRole.OWNER, redirectTo: '/settings' });Also update packages/web/src/app/api/(server)/agents/route.ts to enforce the same role:
export const POST = apiHandler(async (request: NextRequest) => {
...
const result = await withMinimumOrgRole(OrgRole.OWNER, async ({ org, prisma }) => {
// existing logic
});
});Apply the same gating to the edit page at packages/web/src/app/(app)/agents/configs/[agentId]/page.tsx for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/app/`(app)/agents/configs/new/page.tsx at line 5, Update UI
and server to require OrgRole.OWNER for agent config creation/editing: change
the page export that currently calls authenticatedPage(...) to pass the options
{ minRole: OrgRole.OWNER, redirectTo: '/settings' } (look for authenticatedPage
in the new page and the edit page under the agents config routes). In the API
handler that uses withAuth (the POST handler wrapped by apiHandler), replace or
wrap the authorization check with withMinimumOrgRole(OrgRole.OWNER, async ({
org, prisma }) => { ...existing POST logic... }) so only owners can create
configs; ensure you keep existing request handling inside that callback. Also
apply the same authenticatedPage gating to the edit page
(agents/configs/[agentId]/page) so UI and API enforce the same minimum role.
| CODE_REVIEW: "Code Review", | ||
| }; | ||
|
|
||
| export default authenticatedPage(async ({ prisma, org }) => { |
Contributor
There was a problem hiding this comment.
Restrict the agent configuration list to org owners.
This page now exposes config names, descriptions, scopes, repo/connection associations, and edit links to any authenticated org member. Gate it with the owner role.
Proposed fix
+import { OrgRole } from "@sourcebot/db";
...
-export default authenticatedPage(async ({ prisma, org }) => {
+export default authenticatedPage(async ({ prisma, org }) => {
const agentConfigs = await prisma.agentConfig.findMany({
...
-});
+}, { minRole: OrgRole.OWNER, redirectTo: "/settings" });Based on learnings: Use authenticatedPage with { minRole: OrgRole.OWNER, redirectTo: '/settings' } option to gate pages by role in the (app) route group.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export default authenticatedPage(async ({ prisma, org }) => { | |
| import { OrgRole } from "@sourcebot/db"; | |
| export default authenticatedPage(async ({ prisma, org }) => { | |
| const agentConfigs = await prisma.agentConfig.findMany({ | |
| // ... rest of the function body | |
| }); | |
| // ... return statement | |
| }, { minRole: OrgRole.OWNER, redirectTo: "/settings" }); |
🤖 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 at line 37, The page is exposing
agent configs to all authenticated org members; update the authenticatedPage
call to require owner role by passing options like { minRole: OrgRole.OWNER,
redirectTo: '/settings' } so only org owners can access it; modify the existing
authenticatedPage(...) invocation (the exported default function) to include
these options and ensure OrgRole is imported where used.
Comment on lines
+57
to
+60
| test('returns true when no config and env flag is not set', () => { | ||
| mocks.env.REVIEW_AGENT_AUTO_REVIEW_ENABLED = undefined; | ||
|
|
||
| expect(isAutoReviewEnabled(null)).toBe(true); |
Contributor
There was a problem hiding this comment.
Update the unset-env expectations to default to disabled.
These tests currently lock in undefined env as auto-review enabled. That conflicts with the documented default and would keep the unsafe fallback from being fixed.
Proposed test update
- test('returns true when no config and env flag is not set', () => {
+ test('returns false when no config and env flag is not set', () => {
mocks.env.REVIEW_AGENT_AUTO_REVIEW_ENABLED = undefined;
- expect(isAutoReviewEnabled(null)).toBe(true);
+ expect(isAutoReviewEnabled(null)).toBe(false);
});
...
- test('per-config false overrides unset env (which would default to true)', () => {
+ test('per-config false is preserved when env is unset', () => {
mocks.env.REVIEW_AGENT_AUTO_REVIEW_ENABLED = undefined;
expect(isAutoReviewEnabled(makeConfig({ autoReviewEnabled: false }))).toBe(false);
});
...
- test('returns true when config is provided but has no settings at all', () => {
+ test('returns false when config is provided but has no settings at all', () => {
mocks.env.REVIEW_AGENT_AUTO_REVIEW_ENABLED = undefined;
- expect(isAutoReviewEnabled(makeConfig({}))).toBe(true);
+ expect(isAutoReviewEnabled(makeConfig({}))).toBe(false);
});Also applies to: 87-102
🤖 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/webhookUtils.test.ts` around
lines 57 - 60, The tests currently treat an undefined environment flag as
enabling auto-review; update them to reflect the documented default of disabled
by asserting false. For the tests that call isAutoReviewEnabled with a null/no
config (e.g., the test titled "returns true when no config and env flag is not
set" and the similar cases around the 87-102 block), set
mocks.env.REVIEW_AGENT_AUTO_REVIEW_ENABLED = undefined (or leave it unset) and
change the expectation to expect(isAutoReviewEnabled(null)).toBe(false); apply
the same change to the other affected test cases to ensure the default is
disabled.
| return settings.autoReviewEnabled; | ||
| } | ||
| } | ||
| return env.REVIEW_AGENT_AUTO_REVIEW_ENABLED !== "false"; |
Contributor
There was a problem hiding this comment.
Keep auto-review disabled when the env flag is unset.
Line 17 enables auto-review for undefined, which contradicts the documented default of disabled unless REVIEW_AGENT_AUTO_REVIEW_ENABLED is set to true. This can unexpectedly trigger reviews for every PR/MR when the env value is absent.
Proposed fix
- return env.REVIEW_AGENT_AUTO_REVIEW_ENABLED !== "false";
+ return env.REVIEW_AGENT_AUTO_REVIEW_ENABLED === "true";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return env.REVIEW_AGENT_AUTO_REVIEW_ENABLED !== "false"; | |
| return env.REVIEW_AGENT_AUTO_REVIEW_ENABLED === "true"; |
🤖 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/webhookUtils.ts` at line 17,
The current check returns true when env.REVIEW_AGENT_AUTO_REVIEW_ENABLED is
undefined, enabling auto-review by default; change the condition so auto-review
is enabled only when REVIEW_AGENT_AUTO_REVIEW_ENABLED is explicitly set to
"true" (e.g., replace the loose inequality check of
env.REVIEW_AGENT_AUTO_REVIEW_ENABLED with a strict comparison to "true" or
equivalent explicit boolean parsing) so the expression using
env.REVIEW_AGENT_AUTO_REVIEW_ENABLED only returns true when the flag equals
"true".
Comment on lines
+59
to
+63
| const publicAgentConfigSettingsSchema = z.object({ | ||
| autoReviewEnabled: z.boolean().optional().describe('Whether the agent automatically reviews new PRs/MRs. Overrides the REVIEW_AGENT_AUTO_REVIEW_ENABLED env var.'), | ||
| reviewCommand: z.string().optional().describe('Comment command that triggers a manual review (without the leading /). Overrides the REVIEW_AGENT_REVIEW_COMMAND env var.'), | ||
| model: z.string().optional().describe('Display name of the language model to use for this config. Overrides the REVIEW_AGENT_MODEL env var.'), | ||
| }).openapi('PublicAgentConfigSettings'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that all agent-config settings schemas expose contextFiles consistently.
rg -n -C3 'agentConfigSettingsSchema|publicAgentConfigSettingsSchema|contextFiles' \
--glob 'packages/web/src/**/*.ts' \
--glob 'packages/web/src/**/*.tsx'Repository: sourcebot-dev/sourcebot
Length of output: 13388
Add contextFiles to the public agent settings schema.
contextFiles is actively used by the review agent and sent by the form, but the public schema omits this field. External API clients and generated OpenAPI documentation will be unable to discover or validate this setting.
🔧 Proposed fix
const publicAgentConfigSettingsSchema = z.object({
autoReviewEnabled: z.boolean().optional().describe('Whether the agent automatically reviews new PRs/MRs. Overrides the REVIEW_AGENT_AUTO_REVIEW_ENABLED env var.'),
reviewCommand: z.string().optional().describe('Comment command that triggers a manual review (without the leading /). Overrides the REVIEW_AGENT_REVIEW_COMMAND env var.'),
model: z.string().optional().describe('Display name of the language model to use for this config. Overrides the REVIEW_AGENT_MODEL env var.'),
+ contextFiles: z.string().optional().describe('Comma- or space-separated repository paths whose contents are injected as review guidance.'),
}).openapi('PublicAgentConfigSettings');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const publicAgentConfigSettingsSchema = z.object({ | |
| autoReviewEnabled: z.boolean().optional().describe('Whether the agent automatically reviews new PRs/MRs. Overrides the REVIEW_AGENT_AUTO_REVIEW_ENABLED env var.'), | |
| reviewCommand: z.string().optional().describe('Comment command that triggers a manual review (without the leading /). Overrides the REVIEW_AGENT_REVIEW_COMMAND env var.'), | |
| model: z.string().optional().describe('Display name of the language model to use for this config. Overrides the REVIEW_AGENT_MODEL env var.'), | |
| }).openapi('PublicAgentConfigSettings'); | |
| const publicAgentConfigSettingsSchema = z.object({ | |
| autoReviewEnabled: z.boolean().optional().describe('Whether the agent automatically reviews new PRs/MRs. Overrides the REVIEW_AGENT_AUTO_REVIEW_ENABLED env var.'), | |
| reviewCommand: z.string().optional().describe('Comment command that triggers a manual review (without the leading /). Overrides the REVIEW_AGENT_REVIEW_COMMAND env var.'), | |
| model: z.string().optional().describe('Display name of the language model to use for this config. Overrides the REVIEW_AGENT_MODEL env var.'), | |
| contextFiles: z.string().optional().describe('Comma- or space-separated repository paths whose contents are injected as review guidance.'), | |
| }).openapi('PublicAgentConfigSettings'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/openapi/publicApiSchemas.ts` around lines 59 - 63, The
publicAgentConfigSettingsSchema is missing the contextFiles field used by the
review agent; update the z.object for publicAgentConfigSettingsSchema to include
a new optional contextFiles property typed as an array of strings (e.g.,
z.array(z.string()).optional()) and add a descriptive .describe(...) (e.g.,
"List of file paths or glob patterns to include as context for reviews;
overrides the REVIEW_AGENT_CONTEXT_FILES env var") so the OpenAPI output and
clients can discover and validate this setting.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/db/prisma/schema.prisma`:
- Around line 640-660: Add a tenant-scoped orgId column to both
AgentConfigToRepo and AgentConfigToConnection and make the PK include orgId
(e.g. @@id([orgId, agentConfigId, repoId]) and @@id([orgId, agentConfigId,
connectionId])) so the DB enforces tenant isolation; also add reverse lookup
indexes @@index([repoId, agentConfigId]) and @@index([connectionId,
agentConfigId]) respectively to speed queries. Ensure the new orgId field type
matches your org identifier (e.g. String) and populate/migrate it from
AgentConfig/Repo/Connection so the FK/PK invariants hold.
In `@packages/web/src/app/api/`(server)/agents/[agentId]/route.test.ts:
- Around line 127-166: The test suite's local beforeEach in the "name collision"
describe block doesn't clear prior mock call history causing the negative
assertion on prisma.agentConfig.findUnique (in the test that expects it not to
have been called) to be flaky; update that describe's beforeEach to call
vi.clearAllMocks() (or vi.resetAllMocks()) before setting
prisma.agentConfig.findFirst.mockResolvedValue(...) so that
prisma.agentConfig.findUnique calls from earlier tests are cleared and the
expect(prisma.agentConfig.findUnique).not.toHaveBeenCalled() assertion is valid.
In
`@packages/web/src/features/agents/review-agent/nodes/fetchFileContent.test.ts`:
- Around line 11-20: The test currently replaces fileSourceResponseSchema with
an overly permissive mock; instead restore the real fileSourceResponseSchema and
only mock getFileSourceForRepo (keep mockGetFileSourceForRepo). In
fetchFileContent.test.ts, remove the fake schema from the vi.mock and
import/retain the actual fileSourceResponseSchema used in production, then
update all success fixtures returned by mockGetFileSourceForRepo to include the
required fields (source, language, path, repo, repoCodeHostType, webUrl) so they
conform to the real schema’s validation.
🪄 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: ca46226b-3494-4ba8-9186-02b4e35ce290
📒 Files selected for processing (14)
packages/db/prisma/schema.prismapackages/web/src/app/(app)/agents/configs/[agentId]/page.tsxpackages/web/src/app/(app)/agents/configs/new/page.tsxpackages/web/src/app/api/(server)/agents/[agentId]/route.test.tspackages/web/src/app/api/(server)/agents/[agentId]/route.tspackages/web/src/app/api/(server)/agents/route.tspackages/web/src/app/api/(server)/webhook/route.tspackages/web/src/features/agents/review-agent/app.tspackages/web/src/features/agents/review-agent/nodes/fetchFileContent.test.tspackages/web/src/features/agents/review-agent/nodes/fetchFileContent.tspackages/web/src/features/agents/review-agent/nodes/generatePrReview.test.tspackages/web/src/features/agents/review-agent/nodes/generatePrReview.tspackages/web/src/features/agents/review-agent/resolveAgentConfig.tspackages/web/src/openapi/publicApiSchemas.ts
✅ Files skipped from review due to trivial changes (1)
- packages/web/src/app/(app)/agents/configs/new/page.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/web/src/features/agents/review-agent/nodes/generatePrReview.ts
- packages/web/src/features/agents/review-agent/nodes/fetchFileContent.ts
- packages/web/src/features/agents/review-agent/resolveAgentConfig.ts
- packages/web/src/openapi/publicApiSchemas.ts
- packages/web/src/app/(app)/agents/configs/[agentId]/page.tsx
- packages/web/src/app/api/(server)/agents/[agentId]/route.ts
- packages/web/src/app/api/(server)/agents/route.ts
- packages/web/src/features/agents/review-agent/app.ts
- packages/web/src/app/api/(server)/webhook/route.ts
Comment on lines
+640
to
+660
| /// Maps an AgentConfig to specific repositories (used when scope = REPO). | ||
| model AgentConfigToRepo { | ||
| agentConfig AgentConfig @relation(fields: [agentConfigId], references: [id], onDelete: Cascade) | ||
| agentConfigId String | ||
|
|
||
| repo Repo @relation(fields: [repoId], references: [id], onDelete: Cascade) | ||
| repoId Int | ||
|
|
||
| @@id([agentConfigId, repoId]) | ||
| } | ||
|
|
||
| /// Maps an AgentConfig to specific connections (used when scope = CONNECTION). | ||
| model AgentConfigToConnection { | ||
| agentConfig AgentConfig @relation(fields: [agentConfigId], references: [id], onDelete: Cascade) | ||
| agentConfigId String | ||
|
|
||
| connection Connection @relation(fields: [connectionId], references: [id], onDelete: Cascade) | ||
| connectionId Int | ||
|
|
||
| @@id([agentConfigId, connectionId]) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether the migration adds reverse indexes or tenant-safe constraints for AgentConfig mappings.
# Expected: indexes on repoId/connectionId and/or constraints preventing cross-org mappings.
rg -n -C3 \
'AgentConfigToRepo|AgentConfigToConnection|repoId|connectionId|CREATE INDEX|CREATE UNIQUE INDEX|FOREIGN KEY|REFERENCES' \
--glob '!**/node_modules/**' \
--glob '!**/dist/**' \
--glob '!**/build/**'Repository: sourcebot-dev/sourcebot
Length of output: 50380
🏁 Script executed:
#!/bin/bash
# Find migration files related to AgentConfig mappings
find packages/db/prisma/migrations -type f -name "*.sql" 2>/dev/null | head -20Repository: sourcebot-dev/sourcebot
Length of output: 1716
🏁 Script executed:
#!/bin/bash
# Check the current schema.prisma for AgentConfigToRepo and AgentConfigToConnection definitions
sed -n '640,680p' packages/db/prisma/schema.prismaRepository: sourcebot-dev/sourcebot
Length of output: 836
🏁 Script executed:
#!/bin/bash
# Search for any indexes or constraints on these models in migrations
find packages/db/prisma/migrations -type f -name "*.sql" -exec grep -l "AgentConfigToRepo\|AgentConfigToConnection\|agentConfigId.*repoId\|agentConfigId.*connectionId" {} \; 2>/dev/nullRepository: sourcebot-dev/sourcebot
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Examine the migration that added AgentConfig models
cat packages/db/prisma/migrations/20260421203635_add_agent_config/migration.sqlRepository: sourcebot-dev/sourcebot
Length of output: 2432
Add tenant constraints and reverse lookup indexes to AgentConfig mapping tables.
The migration does not include the recommended protections. The junction tables (AgentConfigToRepo and AgentConfigToConnection) lack orgId columns, so the database cannot enforce that a mapped repo or connection belongs to the same organization as the AgentConfig. While the API validates this at the application layer, the invariant should be protected at the schema layer to prevent data corruption from regressions or bypass.
Additionally, the composite primary keys are agentConfigId-first, but typical query patterns filter by repoId or connectionId first. Without reverse indexes, these lookups will perform full table scans or inefficient index usage.
Add orgId column with tenant-safe constraints and create reverse indexes such as @@index([repoId, agentConfigId]) and @@index([connectionId, agentConfigId]) to both tables.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/prisma/schema.prisma` around lines 640 - 660, Add a tenant-scoped
orgId column to both AgentConfigToRepo and AgentConfigToConnection and make the
PK include orgId (e.g. @@id([orgId, agentConfigId, repoId]) and @@id([orgId,
agentConfigId, connectionId])) so the DB enforces tenant isolation; also add
reverse lookup indexes @@index([repoId, agentConfigId]) and
@@index([connectionId, agentConfigId]) respectively to speed queries. Ensure the
new orgId field type matches your org identifier (e.g. String) and
populate/migrate it from AgentConfig/Repo/Connection so the FK/PK invariants
hold.
Comment on lines
+127
to
+166
| describe('name collision', () => { | ||
| beforeEach(() => { | ||
| prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig() as any); | ||
| }); | ||
|
|
||
| test('returns 409 when renaming to a name used by another config', async () => { | ||
| prisma.agentConfig.findUnique.mockResolvedValue(makeDbConfig({ id: 'cfg-other', name: 'taken-name' }) as any); | ||
|
|
||
| const res = await PATCH(makePatchRequest(AGENT_ID, { name: 'taken-name' }), params); | ||
|
|
||
| expect(res.status).toBe(StatusCodes.CONFLICT); | ||
| const body = await res.json(); | ||
| expect(body.message).toContain('taken-name'); | ||
| }); | ||
|
|
||
| test('does not call update when a name collision is detected', async () => { | ||
| prisma.agentConfig.findUnique.mockResolvedValue(makeDbConfig({ id: 'cfg-other' }) as any); | ||
|
|
||
| await PATCH(makePatchRequest(AGENT_ID, { name: 'taken-name' }), params); | ||
|
|
||
| expect(prisma.agentConfig.update).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('returns 200 when renaming to the same name the config already has', async () => { | ||
| // No collision — the config has name 'my-config' and we're "renaming" to the same value. | ||
| prisma.agentConfig.findUnique.mockResolvedValue(null); | ||
| prisma.agentConfig.update.mockResolvedValue(makeDbConfig() as any); | ||
|
|
||
| const res = await PATCH(makePatchRequest(AGENT_ID, { name: 'my-config' }), params); | ||
|
|
||
| expect(res.status).toBe(StatusCodes.OK); | ||
| }); | ||
|
|
||
| test('does not query for collision when name is not in the patch body', async () => { | ||
| prisma.agentConfig.update.mockResolvedValue(makeDbConfig() as any); | ||
|
|
||
| await PATCH(makePatchRequest(AGENT_ID, { enabled: false }), params); | ||
|
|
||
| expect(prisma.agentConfig.findUnique).not.toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect Vitest/test setup for automatic mock clearing.
# Expected: If no clearMocks/mockReset/restoreMocks or setup-level vi.clearAllMocks exists,
# this test file should clear mocks locally before negative toHaveBeenCalled assertions.
rg -n -C3 'clearMocks|mockReset|restoreMocks|vi\.clearAllMocks|vi\.resetAllMocks' \
--glob '!node_modules/**' \
--glob '!dist/**' \
--glob '!build/**'Repository: sourcebot-dev/sourcebot
Length of output: 9247
🏁 Script executed:
# Check the structure and setup of the route.test.ts file
head -130 packages/web/src/app/api/\(server\)/agents/\[agentId\]/route.test.ts | tail -40Repository: sourcebot-dev/sourcebot
Length of output: 1571
🏁 Script executed:
# Also check for any beforeEach or afterEach at the top level of the describe block
sed -n '1,170p' packages/web/src/app/api/\(server\)/agents/\[agentId\]/route.test.ts | grep -n "beforeEach\|afterEach\|describe\|PATCH"Repository: sourcebot-dev/sourcebot
Length of output: 983
Clear mock call history before the negative assertion on line 165.
The local beforeEach at line 128 does not clear mock call history. Earlier tests in the same describe block call prisma.agentConfig.findUnique, so the assertion expect(prisma.agentConfig.findUnique).not.toHaveBeenCalled() on line 165 will fail. Add vi.clearAllMocks() to the local beforeEach.
Proposed fix
describe('name collision', () => {
beforeEach(() => {
+ vi.clearAllMocks();
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig() as any);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/app/api/`(server)/agents/[agentId]/route.test.ts around
lines 127 - 166, The test suite's local beforeEach in the "name collision"
describe block doesn't clear prior mock call history causing the negative
assertion on prisma.agentConfig.findUnique (in the test that expects it not to
have been called) to be flaky; update that describe's beforeEach to call
vi.clearAllMocks() (or vi.resetAllMocks()) before setting
prisma.agentConfig.findFirst.mockResolvedValue(...) so that
prisma.agentConfig.findUnique calls from earlier tests are cleared and the
expect(prisma.agentConfig.findUnique).not.toHaveBeenCalled() assertion is valid.
Comment on lines
+11
to
+20
| vi.mock('@/features/git', () => ({ | ||
| getFileSourceForRepo: (...args: unknown[]) => mockGetFileSourceForRepo(...args), | ||
| fileSourceResponseSchema: { | ||
| safeParse: (v: unknown) => { | ||
| if (v && typeof v === 'object' && 'source' in v) { | ||
| return { success: true, data: v }; | ||
| } | ||
| return { success: false, error: new Error('parse failure') }; | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
Use the real fileSourceResponseSchema in this mock.
The mocked schema accepts any object with source, but the real schema also requires fields like language, path, repo, repoCodeHostType, and webUrl. As written, the success tests use responses production validation would reject, so schema-regression coverage is weakened. Prefer preserving the real schema and mocking only getFileSourceForRepo, then update success fixtures to be valid file-source responses.
Suggested direction
-vi.mock('@/features/git', () => ({
- getFileSourceForRepo: (...args: unknown[]) => mockGetFileSourceForRepo(...args),
- fileSourceResponseSchema: {
- safeParse: (v: unknown) => {
- if (v && typeof v === 'object' && 'source' in v) {
- return { success: true, data: v };
- }
- return { success: false, error: new Error('parse failure') };
- },
- },
-}));
+vi.mock('@/features/git', async (importOriginal) => {
+ const actual = await importOriginal<typeof import('@/features/git')>();
+ return {
+ ...actual,
+ getFileSourceForRepo: (...args: unknown[]) => mockGetFileSourceForRepo(...args),
+ };
+});🤖 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.test.ts`
around lines 11 - 20, The test currently replaces fileSourceResponseSchema with
an overly permissive mock; instead restore the real fileSourceResponseSchema and
only mock getFileSourceForRepo (keep mockGetFileSourceForRepo). In
fetchFileContent.test.ts, remove the fake schema from the vi.mock and
import/retain the actual fileSourceResponseSchema used in production, then
update all success fixtures returned by mockGetFileSourceForRepo to include the
required fields (source, language, path, repo, repoCodeHostType, webUrl) so they
conform to the real schema’s validation.
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
1 participant
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 a per-org/connection/repo agent config system for the code review agent.
Database:
AgentConfigmodel withAgentType,AgentScope, andPromptModeenumsAgentConfigToRepoandAgentConfigToConnectionfor scope targeting20260421203635_add_agent_configReview agent:
resolveRules(): APPEND or REPLACE custom prompt against built-in rulesparseAgentConfigSettings(): validates per-config JSON settingswebhookUtils:isAutoReviewEnabledandgetReviewCommandrespect per-config overridesinvokeDiffReviewLlm: accepts per-config model overridefetchContextFile: fetches repo-level instruction files (e.g. AGENTS.md) once per PR and injects as context for every diff hunk; supports comma/space-separated list of paths; missing files silently ignoredAPI:
GET/POST /api/agents— list and create agent configsGET/PATCH/DELETE /api/agents/[agentId]— manage individual configsUI:
Tests:
Docs:
Builds on changes from #1104
Summary by CodeRabbit
New Features
Documentation