Skip to content

Commit c6878cf

Browse files
author
Gavin Williams
committed
Prevent Agent overlaps at the various config levels.
PATCH handler findFirst include — Added repos: { select: { repoId: true } } and connections: { select: { connectionId: true } } to the PATCH handler's initial findFirst, so existing.repos and existing.connections are available when computing effective IDs for the scope conflict check. resolveAgentConfig.ts — Removed orderBy: { updatedAt: 'desc' } from all three findFirst queries (REPO, CONNECTION, ORG). Scope-level uniqueness is now enforced at the API layer, making tiebreaker ordering unnecessary. Tests — Added scope conflict coverage to both route test files: - route.test.ts (POST): 5 new tests — ORG conflict, REPO overlap, CONNECTION overlap, disabled bypass, no create on conflict - [agentId]/route.test.ts (PATCH): 5 new tests — ORG conflict, REPO overlap, CONNECTION overlap, disabled bypass, no update on conflict - Fixed beforeEach mock chaining in PATCH name-collision and successful-update suites (was inadvertently passing makeDbConfig back for the conflict findFirst too) - Updated makeDbConfig overrides type to accept repos/connections relation arrays
1 parent 6871e3a commit c6878cf

7 files changed

Lines changed: 257 additions & 6 deletions

File tree

‎docs/docs/features/agents/review-agent.mdx‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,14 @@ Configs are managed on the **Agents** page in the Sourcebot UI. Each config has
147147
- **Connection** — applies to all repos in a specific connection
148148
- **Org** — applies to all repositories in your org (lowest priority, catch-all)
149149

150+
Sourcebot enforces uniqueness within each scope level to avoid ambiguity:
151+
152+
- Only one enabled org-scoped config can exist per agent type.
153+
- No two enabled repo-scoped configs can cover the same repository.
154+
- No two enabled connection-scoped configs can cover the same connection.
155+
156+
Sourcebot rejects a create or update request that would violate these rules. To apply a different config to a subset of repos covered by an existing config, disable the existing config first, then create the new narrower one.
157+
150158
When a PR or MR arrives, Sourcebot selects the most specific matching config. If no config exists, the agent falls back to global environment variable defaults.
151159

152160
## Custom prompt

‎packages/web/src/app/api/(server)/agents/[agentId]/route.test.ts‎

Lines changed: 83 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,12 @@ function makeDeleteRequest(agentId: string): NextRequest {
6666
return new NextRequest(makeUrl(agentId), { method: 'DELETE' });
6767
}
6868

69-
function makeDbConfig(overrides: Partial<AgentConfig> = {}): AgentConfig & { repos: []; connections: [] } {
69+
type DbConfigOverrides = Partial<AgentConfig> & {
70+
repos?: { repoId: number }[];
71+
connections?: { connectionId: number }[];
72+
};
73+
74+
function makeDbConfig(overrides: DbConfigOverrides = {}): AgentConfig & { repos: { repoId: number }[]; connections: { connectionId: number }[] } {
7075
return {
7176
id: 'cfg-abc',
7277
orgId: MOCK_ORG.id,
@@ -126,7 +131,10 @@ describe('PATCH /api/agents/[agentId]', () => {
126131

127132
describe('name collision', () => {
128133
beforeEach(() => {
129-
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig() as any);
134+
// First findFirst: fetch existing config. Subsequent calls (scope conflict): no conflict.
135+
prisma.agentConfig.findFirst
136+
.mockResolvedValueOnce(makeDbConfig() as any)
137+
.mockResolvedValue(null);
130138
});
131139

132140
test('returns 409 when renaming to a name used by another config', async () => {
@@ -177,7 +185,10 @@ describe('PATCH /api/agents/[agentId]', () => {
177185

178186
describe('successful update', () => {
179187
beforeEach(() => {
180-
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig() as any);
188+
// First findFirst: fetch existing config. Subsequent calls (scope conflict): no conflict.
189+
prisma.agentConfig.findFirst
190+
.mockResolvedValueOnce(makeDbConfig() as any)
191+
.mockResolvedValue(null);
181192
prisma.agentConfig.findUnique.mockResolvedValue(null);
182193
});
183194

@@ -204,6 +215,75 @@ describe('PATCH /api/agents/[agentId]', () => {
204215
);
205216
});
206217
});
218+
219+
describe('scope conflict', () => {
220+
const params = { params: Promise.resolve({ agentId: AGENT_ID }) };
221+
222+
test('returns 409 when patching an ORG config conflicts with another enabled org-wide config', async () => {
223+
prisma.agentConfig.findFirst
224+
.mockResolvedValueOnce(makeDbConfig() as any) // existing
225+
.mockResolvedValueOnce(makeDbConfig({ id: 'cfg-other', name: 'conflicting' }) as any); // scope conflict
226+
prisma.agentConfig.findUnique.mockResolvedValue(null);
227+
228+
const res = await PATCH(makePatchRequest(AGENT_ID, { name: 'renamed' }), params);
229+
230+
expect(res.status).toBe(StatusCodes.CONFLICT);
231+
const body = await res.json();
232+
expect(body.message).toContain('conflicting');
233+
});
234+
235+
test('does not call update when a scope conflict is detected', async () => {
236+
prisma.agentConfig.findFirst
237+
.mockResolvedValueOnce(makeDbConfig() as any)
238+
.mockResolvedValueOnce(makeDbConfig({ id: 'cfg-other', name: 'conflicting' }) as any);
239+
prisma.agentConfig.findUnique.mockResolvedValue(null);
240+
241+
await PATCH(makePatchRequest(AGENT_ID, { name: 'renamed' }), params);
242+
243+
expect(prisma.agentConfig.update).not.toHaveBeenCalled();
244+
});
245+
246+
test('returns 409 when a REPO config overlaps with another enabled REPO config', async () => {
247+
const existingRepoConfig = makeDbConfig({
248+
scope: AgentScope.REPO,
249+
repos: [{ repoId: 5 }] as any,
250+
});
251+
prisma.agentConfig.findFirst
252+
.mockResolvedValueOnce(existingRepoConfig as any)
253+
.mockResolvedValueOnce(makeDbConfig({ id: 'cfg-other', name: 'conflicting', scope: AgentScope.REPO }) as any);
254+
prisma.agentConfig.findUnique.mockResolvedValue(null);
255+
256+
const res = await PATCH(makePatchRequest(AGENT_ID, { prompt: 'updated' }), params);
257+
258+
expect(res.status).toBe(StatusCodes.CONFLICT);
259+
});
260+
261+
test('returns 409 when a CONNECTION config overlaps with another enabled CONNECTION config', async () => {
262+
const existingConnConfig = makeDbConfig({
263+
scope: AgentScope.CONNECTION,
264+
connections: [{ connectionId: 7 }] as any,
265+
});
266+
prisma.agentConfig.findFirst
267+
.mockResolvedValueOnce(existingConnConfig as any)
268+
.mockResolvedValueOnce(makeDbConfig({ id: 'cfg-other', name: 'conflicting', scope: AgentScope.CONNECTION }) as any);
269+
prisma.agentConfig.findUnique.mockResolvedValue(null);
270+
271+
const res = await PATCH(makePatchRequest(AGENT_ID, { prompt: 'updated' }), params);
272+
273+
expect(res.status).toBe(StatusCodes.CONFLICT);
274+
});
275+
276+
test('does not check scope conflict when patching enabled to false', async () => {
277+
prisma.agentConfig.findFirst.mockResolvedValueOnce(makeDbConfig() as any);
278+
prisma.agentConfig.findUnique.mockResolvedValue(null);
279+
prisma.agentConfig.update.mockResolvedValue(makeDbConfig({ enabled: false }) as any);
280+
281+
const res = await PATCH(makePatchRequest(AGENT_ID, { enabled: false }), params);
282+
283+
expect(res.status).toBe(StatusCodes.OK);
284+
expect(prisma.agentConfig.findFirst).toHaveBeenCalledTimes(1);
285+
});
286+
});
207287
});
208288

209289
// ── DELETE /api/agents/[agentId] ──────────────────────────────────────────────

‎packages/web/src/app/api/(server)/agents/[agentId]/route.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,10 @@ export const PATCH = apiHandler(async (request: NextRequest, { params }: RoutePa
8787
return withMinimumOrgRole(role, OrgRole.OWNER, async () => {
8888
const existing = await prisma.agentConfig.findFirst({
8989
where: { id: agentId, orgId: org.id },
90+
include: {
91+
repos: { select: { repoId: true } },
92+
connections: { select: { connectionId: true } },
93+
},
9094
});
9195

9296
if (!existing) {
@@ -154,6 +158,64 @@ export const PATCH = apiHandler(async (request: NextRequest, { params }: RoutePa
154158
}
155159
}
156160

161+
// Enforce scope-level uniqueness (only when the config is or will be enabled)
162+
const willBeEnabled = enabled !== undefined ? enabled : existing.enabled;
163+
if (willBeEnabled) {
164+
const effectiveType = type ?? existing.type;
165+
const notSelf = { id: { not: agentId } };
166+
167+
if (effectiveScope === AgentScope.ORG) {
168+
const conflict = await prisma.agentConfig.findFirst({
169+
where: { orgId: org.id, type: effectiveType, scope: AgentScope.ORG, enabled: true, ...notSelf },
170+
});
171+
if (conflict) {
172+
return {
173+
statusCode: StatusCodes.CONFLICT,
174+
errorCode: ErrorCode.AGENT_CONFIG_SCOPE_CONFLICT,
175+
message: `An org-wide config of this type already exists: '${conflict.name}'`,
176+
};
177+
}
178+
}
179+
180+
const effectiveRepoIds = repoIds ?? (effectiveScope === AgentScope.REPO
181+
? existing.repos?.map((r: { repoId: number }) => r.repoId) ?? []
182+
: []);
183+
if (effectiveScope === AgentScope.REPO && effectiveRepoIds.length > 0) {
184+
const conflict = await prisma.agentConfig.findFirst({
185+
where: {
186+
orgId: org.id, type: effectiveType, scope: AgentScope.REPO, enabled: true, ...notSelf,
187+
repos: { some: { repoId: { in: effectiveRepoIds } } },
188+
},
189+
});
190+
if (conflict) {
191+
return {
192+
statusCode: StatusCodes.CONFLICT,
193+
errorCode: ErrorCode.AGENT_CONFIG_SCOPE_CONFLICT,
194+
message: `One or more of the selected repos is already covered by config '${conflict.name}'`,
195+
};
196+
}
197+
}
198+
199+
const effectiveConnectionIds = connectionIds ?? (effectiveScope === AgentScope.CONNECTION
200+
? existing.connections?.map((c: { connectionId: number }) => c.connectionId) ?? []
201+
: []);
202+
if (effectiveScope === AgentScope.CONNECTION && effectiveConnectionIds.length > 0) {
203+
const conflict = await prisma.agentConfig.findFirst({
204+
where: {
205+
orgId: org.id, type: effectiveType, scope: AgentScope.CONNECTION, enabled: true, ...notSelf,
206+
connections: { some: { connectionId: { in: effectiveConnectionIds } } },
207+
},
208+
});
209+
if (conflict) {
210+
return {
211+
statusCode: StatusCodes.CONFLICT,
212+
errorCode: ErrorCode.AGENT_CONFIG_SCOPE_CONFLICT,
213+
message: `One or more of the selected connections is already covered by config '${conflict.name}'`,
214+
};
215+
}
216+
}
217+
}
218+
157219
try {
158220
// Rebuild junction table rows when scope or IDs are updated
159221
const updated = await prisma.agentConfig.update({

‎packages/web/src/app/api/(server)/agents/route.test.ts‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,61 @@ describe('POST /api/agents', () => {
194194
});
195195
});
196196

197+
describe('scope conflict', () => {
198+
beforeEach(() => {
199+
prisma.agentConfig.findUnique.mockResolvedValue(null);
200+
});
201+
202+
test('returns 409 when an enabled ORG-scoped config of the same type already exists', async () => {
203+
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig({ id: 'cfg-conflict', name: 'existing' }) as any);
204+
205+
const res = await POST(makePostRequest({ name: 'new-config', type: 'CODE_REVIEW', scope: 'ORG' }));
206+
207+
expect(res.status).toBe(StatusCodes.CONFLICT);
208+
const body = await res.json();
209+
expect(body.message).toContain('existing');
210+
});
211+
212+
test('returns 409 when a REPO-scoped config already covers one of the provided repos', async () => {
213+
prisma.repo.count.mockResolvedValue(1);
214+
prisma.agentConfig.findFirst.mockResolvedValue(
215+
makeDbConfig({ id: 'cfg-conflict', name: 'repo-config', scope: AgentScope.REPO }) as any,
216+
);
217+
218+
const res = await POST(makePostRequest({ name: 'new-config', type: 'CODE_REVIEW', scope: 'REPO', repoIds: [1] }));
219+
220+
expect(res.status).toBe(StatusCodes.CONFLICT);
221+
});
222+
223+
test('returns 409 when a CONNECTION-scoped config already covers one of the provided connections', async () => {
224+
prisma.connection.count.mockResolvedValue(1);
225+
prisma.agentConfig.findFirst.mockResolvedValue(
226+
makeDbConfig({ id: 'cfg-conflict', name: 'conn-config', scope: AgentScope.CONNECTION }) as any,
227+
);
228+
229+
const res = await POST(makePostRequest({ name: 'new-config', type: 'CODE_REVIEW', scope: 'CONNECTION', connectionIds: [2] }));
230+
231+
expect(res.status).toBe(StatusCodes.CONFLICT);
232+
});
233+
234+
test('does not check for scope conflict when enabled is false', async () => {
235+
prisma.agentConfig.create.mockResolvedValue(makeDbConfig({ enabled: false }) as any);
236+
237+
const res = await POST(makePostRequest({ name: 'new-config', type: 'CODE_REVIEW', scope: 'ORG', enabled: false }));
238+
239+
expect(res.status).toBe(StatusCodes.CREATED);
240+
expect(prisma.agentConfig.findFirst).not.toHaveBeenCalled();
241+
});
242+
243+
test('does not call create when a scope conflict is detected', async () => {
244+
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig({ id: 'cfg-conflict', name: 'existing' }) as any);
245+
246+
await POST(makePostRequest({ name: 'new-config', type: 'CODE_REVIEW', scope: 'ORG' }));
247+
248+
expect(prisma.agentConfig.create).not.toHaveBeenCalled();
249+
});
250+
});
251+
197252
describe('name collision', () => {
198253
test('returns 409 when a config with the same name already exists', async () => {
199254
prisma.agentConfig.findUnique.mockResolvedValue(makeDbConfig() as any);

‎packages/web/src/app/api/(server)/agents/route.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,54 @@ export const POST = apiHandler(async (request: NextRequest) => {
131131
}
132132
}
133133

134+
// Enforce scope-level uniqueness
135+
if (enabled !== false) {
136+
if (scope === AgentScope.ORG) {
137+
const conflict = await prisma.agentConfig.findFirst({
138+
where: { orgId: org.id, type, scope: AgentScope.ORG, enabled: true },
139+
});
140+
if (conflict) {
141+
return {
142+
statusCode: StatusCodes.CONFLICT,
143+
errorCode: ErrorCode.AGENT_CONFIG_SCOPE_CONFLICT,
144+
message: `An org-wide config of this type already exists: '${conflict.name}'`,
145+
};
146+
}
147+
}
148+
149+
if (scope === AgentScope.REPO && repoIds && repoIds.length > 0) {
150+
const conflict = await prisma.agentConfig.findFirst({
151+
where: {
152+
orgId: org.id, type, scope: AgentScope.REPO, enabled: true,
153+
repos: { some: { repoId: { in: repoIds } } },
154+
},
155+
});
156+
if (conflict) {
157+
return {
158+
statusCode: StatusCodes.CONFLICT,
159+
errorCode: ErrorCode.AGENT_CONFIG_SCOPE_CONFLICT,
160+
message: `One or more of the selected repos is already covered by config '${conflict.name}'`,
161+
};
162+
}
163+
}
164+
165+
if (scope === AgentScope.CONNECTION && connectionIds && connectionIds.length > 0) {
166+
const conflict = await prisma.agentConfig.findFirst({
167+
where: {
168+
orgId: org.id, type, scope: AgentScope.CONNECTION, enabled: true,
169+
connections: { some: { connectionId: { in: connectionIds } } },
170+
},
171+
});
172+
if (conflict) {
173+
return {
174+
statusCode: StatusCodes.CONFLICT,
175+
errorCode: ErrorCode.AGENT_CONFIG_SCOPE_CONFLICT,
176+
message: `One or more of the selected connections is already covered by config '${conflict.name}'`,
177+
};
178+
}
179+
}
180+
}
181+
134182
try {
135183
const config = await prisma.agentConfig.create({
136184
data: {

‎packages/web/src/features/agents/review-agent/resolveAgentConfig.ts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ export async function resolveAgentConfig(
2828
some: { repoId },
2929
},
3030
},
31-
orderBy: { updatedAt: 'desc' },
3231
});
3332

3433
if (repoConfig) {
@@ -53,7 +52,6 @@ export async function resolveAgentConfig(
5352
},
5453
},
5554
},
56-
orderBy: { updatedAt: 'desc' },
5755
});
5856

5957
if (connectionConfig) {
@@ -69,7 +67,6 @@ export async function resolveAgentConfig(
6967
enabled: true,
7068
scope: 'ORG',
7169
},
72-
orderBy: { updatedAt: 'desc' },
7370
});
7471

7572
if (orgConfig) {

‎packages/web/src/lib/errorCodes.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,4 +36,5 @@ export enum ErrorCode {
3636
API_KEY_USAGE_DISABLED = 'API_KEY_USAGE_DISABLED',
3737
AGENT_CONFIG_NOT_FOUND = 'AGENT_CONFIG_NOT_FOUND',
3838
AGENT_CONFIG_ALREADY_EXISTS = 'AGENT_CONFIG_ALREADY_EXISTS',
39+
AGENT_CONFIG_SCOPE_CONFLICT = 'AGENT_CONFIG_SCOPE_CONFLICT',
3940
}

0 commit comments

Comments
 (0)