Skip to content

Commit f5d5633

Browse files
author
Gavin Williams
committed
Address a number of the CodeRabbit findings.
1 parent 525d02e commit f5d5633

14 files changed

Lines changed: 632 additions & 27 deletions

File tree

‎packages/db/prisma/schema.prisma‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -627,7 +627,7 @@ model AgentConfig {
627627
connections AgentConfigToConnection[]
628628
629629
/// Extensible per-agent settings stored as JSON.
630-
/// Shape: { autoReviewEnabled?: boolean, reviewCommand?: string, model?: string }
630+
/// Shape: { autoReviewEnabled?: boolean, reviewCommand?: string, model?: string, contextFiles?: string }
631631
settings Json @default("{}")
632632
633633
createdAt DateTime @default(now())

‎packages/web/src/app/(app)/agents/configs/[agentId]/page.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { authenticatedPage } from "@/middleware/authenticatedPage";
22
import { NavigationMenu } from "@/app/(app)/components/navigationMenu";
33
import { AgentConfigForm } from "../agentConfigForm";
44
import { notFound } from "next/navigation";
5+
import { OrgRole } from "@sourcebot/db";
56

67
type Props = {
78
params: Promise<{ agentId: string }>;
@@ -59,4 +60,4 @@ export default authenticatedPage(async ({ prisma, org }, { params }: Props) => {
5960
</div>
6061
</div>
6162
);
62-
});
63+
}, { minRole: OrgRole.OWNER, redirectTo: '/agents' });

‎packages/web/src/app/(app)/agents/configs/new/page.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { authenticatedPage } from "@/middleware/authenticatedPage";
22
import { NavigationMenu } from "@/app/(app)/components/navigationMenu";
33
import { AgentConfigForm } from "../agentConfigForm";
4+
import { OrgRole } from "@sourcebot/db";
45

56
export default authenticatedPage(async ({ prisma, org }) => {
67
const connections = await prisma.connection.findMany({
@@ -24,4 +25,4 @@ export default authenticatedPage(async ({ prisma, org }) => {
2425
</div>
2526
</div>
2627
);
27-
});
28+
}, { minRole: OrgRole.OWNER, redirectTo: '/agents' });

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

Lines changed: 232 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,232 @@
1+
import { expect, test, vi, describe, beforeEach } from 'vitest';
2+
import { NextRequest } from 'next/server';
3+
import { MOCK_ORG, MOCK_USER_WITH_ACCOUNTS, prisma } from '@/__mocks__/prisma';
4+
import { AgentConfig, AgentScope, AgentType, OrgRole, PromptMode } from '@sourcebot/db';
5+
import { StatusCodes } from 'http-status-codes';
6+
7+
vi.mock('@/prisma', async () => {
8+
const actual = await vi.importActual<typeof import('@/__mocks__/prisma')>('@/__mocks__/prisma');
9+
return { ...actual };
10+
});
11+
12+
vi.mock('server-only', () => ({ default: vi.fn() }));
13+
14+
vi.mock('@sourcebot/shared', () => ({
15+
createLogger: () => ({
16+
debug: vi.fn(),
17+
info: vi.fn(),
18+
warn: vi.fn(),
19+
error: vi.fn(),
20+
}),
21+
env: {},
22+
}));
23+
24+
vi.mock('@/lib/posthog', () => ({ captureEvent: vi.fn() }));
25+
26+
vi.mock('@/middleware/withAuth', () => ({
27+
withAuth: vi.fn(async (fn: Function) =>
28+
fn({ org: MOCK_ORG, user: MOCK_USER_WITH_ACCOUNTS, role: OrgRole.OWNER, prisma })
29+
),
30+
}));
31+
32+
// app.ts imports heavy node deps — provide a real Zod schema so updateAgentConfigBodySchema.safeParse works.
33+
vi.mock('@/features/agents/review-agent/app', async () => {
34+
const { z } = await import('zod');
35+
return {
36+
agentConfigSettingsSchema: z.object({
37+
autoReviewEnabled: z.boolean().optional(),
38+
reviewCommand: z.string().optional(),
39+
model: z.string().optional(),
40+
contextFiles: z.string().optional(),
41+
}),
42+
};
43+
});
44+
45+
import { GET, PATCH, DELETE } from './route';
46+
47+
// ── helpers ───────────────────────────────────────────────────────────────────
48+
49+
function makeUrl(agentId: string): string {
50+
return `http://localhost/api/agents/${agentId}`;
51+
}
52+
53+
function makePatchRequest(agentId: string, body: unknown): NextRequest {
54+
return new NextRequest(makeUrl(agentId), {
55+
method: 'PATCH',
56+
body: JSON.stringify(body),
57+
headers: { 'Content-Type': 'application/json' },
58+
});
59+
}
60+
61+
function makeGetRequest(agentId: string): NextRequest {
62+
return new NextRequest(makeUrl(agentId), { method: 'GET' });
63+
}
64+
65+
function makeDeleteRequest(agentId: string): NextRequest {
66+
return new NextRequest(makeUrl(agentId), { method: 'DELETE' });
67+
}
68+
69+
function makeDbConfig(overrides: Partial<AgentConfig> = {}): AgentConfig & { repos: []; connections: [] } {
70+
return {
71+
id: 'cfg-abc',
72+
orgId: MOCK_ORG.id,
73+
name: 'my-config',
74+
description: null,
75+
type: AgentType.CODE_REVIEW,
76+
enabled: true,
77+
prompt: null,
78+
promptMode: PromptMode.APPEND,
79+
scope: AgentScope.ORG,
80+
settings: {},
81+
createdAt: new Date(),
82+
updatedAt: new Date(),
83+
repos: [],
84+
connections: [],
85+
...overrides,
86+
};
87+
}
88+
89+
// ── GET /api/agents/[agentId] ─────────────────────────────────────────────────
90+
91+
describe('GET /api/agents/[agentId]', () => {
92+
test('returns 404 when config does not exist', async () => {
93+
prisma.agentConfig.findFirst.mockResolvedValue(null);
94+
95+
const res = await GET(makeGetRequest('cfg-missing'), { params: Promise.resolve({ agentId: 'cfg-missing' }) });
96+
97+
expect(res.status).toBe(StatusCodes.NOT_FOUND);
98+
});
99+
100+
test('returns 200 with the config when found', async () => {
101+
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig() as any);
102+
103+
const res = await GET(makeGetRequest('cfg-abc'), { params: Promise.resolve({ agentId: 'cfg-abc' }) });
104+
105+
expect(res.status).toBe(StatusCodes.OK);
106+
const body = await res.json();
107+
expect(body.id).toBe('cfg-abc');
108+
});
109+
});
110+
111+
// ── PATCH /api/agents/[agentId] ───────────────────────────────────────────────
112+
113+
describe('PATCH /api/agents/[agentId]', () => {
114+
const AGENT_ID = 'cfg-abc';
115+
const params = { params: Promise.resolve({ agentId: AGENT_ID }) };
116+
117+
describe('not found', () => {
118+
test('returns 404 when the config does not exist', async () => {
119+
prisma.agentConfig.findFirst.mockResolvedValue(null);
120+
121+
const res = await PATCH(makePatchRequest(AGENT_ID, { name: 'new-name' }), params);
122+
123+
expect(res.status).toBe(StatusCodes.NOT_FOUND);
124+
});
125+
});
126+
127+
describe('name collision', () => {
128+
beforeEach(() => {
129+
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig() as any);
130+
});
131+
132+
test('returns 409 when renaming to a name used by another config', async () => {
133+
prisma.agentConfig.findUnique.mockResolvedValue(makeDbConfig({ id: 'cfg-other', name: 'taken-name' }) as any);
134+
135+
const res = await PATCH(makePatchRequest(AGENT_ID, { name: 'taken-name' }), params);
136+
137+
expect(res.status).toBe(StatusCodes.CONFLICT);
138+
const body = await res.json();
139+
expect(body.message).toContain('taken-name');
140+
});
141+
142+
test('does not call update when a name collision is detected', async () => {
143+
prisma.agentConfig.findUnique.mockResolvedValue(makeDbConfig({ id: 'cfg-other' }) as any);
144+
145+
await PATCH(makePatchRequest(AGENT_ID, { name: 'taken-name' }), params);
146+
147+
expect(prisma.agentConfig.update).not.toHaveBeenCalled();
148+
});
149+
150+
test('returns 200 when renaming to the same name the config already has', async () => {
151+
// No collision — the config has name 'my-config' and we're "renaming" to the same value.
152+
prisma.agentConfig.findUnique.mockResolvedValue(null);
153+
prisma.agentConfig.update.mockResolvedValue(makeDbConfig() as any);
154+
155+
const res = await PATCH(makePatchRequest(AGENT_ID, { name: 'my-config' }), params);
156+
157+
expect(res.status).toBe(StatusCodes.OK);
158+
});
159+
160+
test('does not query for collision when name is not in the patch body', async () => {
161+
prisma.agentConfig.update.mockResolvedValue(makeDbConfig() as any);
162+
163+
await PATCH(makePatchRequest(AGENT_ID, { enabled: false }), params);
164+
165+
expect(prisma.agentConfig.findUnique).not.toHaveBeenCalled();
166+
});
167+
168+
test('returns 200 when renaming to a free name', async () => {
169+
prisma.agentConfig.findUnique.mockResolvedValue(null);
170+
prisma.agentConfig.update.mockResolvedValue(makeDbConfig({ name: 'free-name' }) as any);
171+
172+
const res = await PATCH(makePatchRequest(AGENT_ID, { name: 'free-name' }), params);
173+
174+
expect(res.status).toBe(StatusCodes.OK);
175+
});
176+
});
177+
178+
describe('successful update', () => {
179+
beforeEach(() => {
180+
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig() as any);
181+
prisma.agentConfig.findUnique.mockResolvedValue(null);
182+
});
183+
184+
test('returns 200 on a valid patch', async () => {
185+
prisma.agentConfig.update.mockResolvedValue(makeDbConfig({ enabled: false }) as any);
186+
187+
const res = await PATCH(makePatchRequest(AGENT_ID, { enabled: false }), params);
188+
189+
expect(res.status).toBe(StatusCodes.OK);
190+
});
191+
192+
test('calls prisma.agentConfig.update with the patched fields', async () => {
193+
prisma.agentConfig.update.mockResolvedValue(makeDbConfig() as any);
194+
195+
await PATCH(makePatchRequest(AGENT_ID, { enabled: false, prompt: 'Be strict.' }), params);
196+
197+
expect(prisma.agentConfig.update).toHaveBeenCalledWith(
198+
expect.objectContaining({
199+
data: expect.objectContaining({
200+
enabled: false,
201+
prompt: 'Be strict.',
202+
}),
203+
}),
204+
);
205+
});
206+
});
207+
});
208+
209+
// ── DELETE /api/agents/[agentId] ──────────────────────────────────────────────
210+
211+
describe('DELETE /api/agents/[agentId]', () => {
212+
const AGENT_ID = 'cfg-abc';
213+
const params = { params: Promise.resolve({ agentId: AGENT_ID }) };
214+
215+
test('returns 404 when the config does not exist', async () => {
216+
prisma.agentConfig.findFirst.mockResolvedValue(null);
217+
218+
const res = await DELETE(makeDeleteRequest(AGENT_ID), params);
219+
220+
expect(res.status).toBe(StatusCodes.NOT_FOUND);
221+
});
222+
223+
test('returns 200 and calls delete when the config exists', async () => {
224+
prisma.agentConfig.findFirst.mockResolvedValue(makeDbConfig() as any);
225+
prisma.agentConfig.delete.mockResolvedValue(makeDbConfig() as any);
226+
227+
const res = await DELETE(makeDeleteRequest(AGENT_ID), params);
228+
229+
expect(res.status).toBe(StatusCodes.OK);
230+
expect(prisma.agentConfig.delete).toHaveBeenCalledWith({ where: { id: AGENT_ID } });
231+
});
232+
});

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

Lines changed: 52 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,19 @@
33
import { apiHandler } from "@/lib/apiHandler";
44
import { requestBodySchemaValidationError, serviceErrorResponse, notFound } from "@/lib/serviceError";
55
import { isServiceError } from "@/lib/utils";
6+
import { ErrorCode } from "@/lib/errorCodes";
67
import { withAuth } from "@/middleware/withAuth";
8+
import { withMinimumOrgRole } from "@/middleware/withMinimumOrgRole";
9+
import { OrgRole } from "@sourcebot/db";
710
import { NextRequest } from "next/server";
811
import { z } from "zod";
912
import { StatusCodes } from "http-status-codes";
1013
import { AgentScope, AgentType, PromptMode } from "@sourcebot/db";
1114
import { createLogger } from "@sourcebot/shared";
15+
import { agentConfigSettingsSchema } from "@/features/agents/review-agent/app";
1216

1317
const logger = createLogger('agents-api');
1418

15-
const agentConfigSettingsSchema = z.object({
16-
autoReviewEnabled: z.boolean().optional(),
17-
reviewCommand: z.string().optional(),
18-
model: z.string().optional(),
19-
});
20-
2119
const updateAgentConfigBodySchema = z.object({
2220
name: z.string().min(1).max(255).optional(),
2321
description: z.string().nullable().optional(),
@@ -85,7 +83,8 @@ export const PATCH = apiHandler(async (request: NextRequest, { params }: RoutePa
8583

8684
const { name, description, type, enabled, prompt, promptMode, scope, repoIds, connectionIds, settings } = parsed.data;
8785

88-
const result = await withAuth(async ({ org, prisma }) => {
86+
const result = await withAuth(async ({ org, role, prisma }) => {
87+
return withMinimumOrgRole(role, OrgRole.OWNER, async () => {
8988
const existing = await prisma.agentConfig.findFirst({
9089
where: { id: agentId, orgId: org.id },
9190
});
@@ -94,6 +93,21 @@ export const PATCH = apiHandler(async (request: NextRequest, { params }: RoutePa
9493
return notFound(`Agent config '${agentId}' not found`);
9594
}
9695

96+
// Check for name collision with a different config in the same org
97+
if (name !== undefined && name !== existing.name) {
98+
const collision = await prisma.agentConfig.findUnique({
99+
where: { orgId_name: { orgId: org.id, name } },
100+
});
101+
102+
if (collision) {
103+
return {
104+
statusCode: StatusCodes.CONFLICT,
105+
errorCode: ErrorCode.AGENT_CONFIG_ALREADY_EXISTS,
106+
message: `An agent config named '${name}' already exists`,
107+
};
108+
}
109+
}
110+
97111
const effectiveScope = scope ?? existing.scope;
98112

99113
// When scope changes to REPO/CONNECTION, IDs must be supplied
@@ -113,6 +127,33 @@ export const PATCH = apiHandler(async (request: NextRequest, { params }: RoutePa
113127
};
114128
}
115129

130+
// Verify all provided IDs belong to this org
131+
if (repoIds && repoIds.length > 0) {
132+
const count = await prisma.repo.count({
133+
where: { id: { in: repoIds }, orgId: org.id },
134+
});
135+
if (count !== repoIds.length) {
136+
return {
137+
statusCode: StatusCodes.BAD_REQUEST,
138+
errorCode: ErrorCode.INVALID_REQUEST_BODY,
139+
message: "One or more repoIds are invalid or do not belong to this org",
140+
};
141+
}
142+
}
143+
144+
if (connectionIds && connectionIds.length > 0) {
145+
const count = await prisma.connection.count({
146+
where: { id: { in: connectionIds }, orgId: org.id },
147+
});
148+
if (count !== connectionIds.length) {
149+
return {
150+
statusCode: StatusCodes.BAD_REQUEST,
151+
errorCode: ErrorCode.INVALID_REQUEST_BODY,
152+
message: "One or more connectionIds are invalid or do not belong to this org",
153+
};
154+
}
155+
}
156+
116157
try {
117158
// Rebuild junction table rows when scope or IDs are updated
118159
const updated = await prisma.agentConfig.update({
@@ -147,6 +188,7 @@ export const PATCH = apiHandler(async (request: NextRequest, { params }: RoutePa
147188
logger.error('Error updating agent config', { error, agentId, orgId: org.id });
148189
throw error;
149190
}
191+
});
150192
});
151193

152194
if (isServiceError(result)) {
@@ -159,7 +201,8 @@ export const PATCH = apiHandler(async (request: NextRequest, { params }: RoutePa
159201
export const DELETE = apiHandler(async (_request: NextRequest, { params }: RouteParams) => {
160202
const { agentId } = await params;
161203

162-
const result = await withAuth(async ({ org, prisma }) => {
204+
const result = await withAuth(async ({ org, role, prisma }) => {
205+
return withMinimumOrgRole(role, OrgRole.OWNER, async () => {
163206
const existing = await prisma.agentConfig.findFirst({
164207
where: { id: agentId, orgId: org.id },
165208
});
@@ -175,6 +218,7 @@ export const DELETE = apiHandler(async (_request: NextRequest, { params }: Route
175218
logger.error('Error deleting agent config', { error, agentId, orgId: org.id });
176219
throw error;
177220
}
221+
});
178222
});
179223

180224
if (isServiceError(result)) {

0 commit comments

Comments
 (0)