From eff308ba56817a8c97c131a64a067a178e68bbce Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Thu, 23 Apr 2026 14:51:57 +0000 Subject: [PATCH] fix: skip invalid agent definitions --- src/api/routers/agentDefinitions.ts | 12 ++-- .../agentDefinitionsRepository.ts | 42 ++++++++++++-- .../unit/api/routers/agentDefinitions.test.ts | 39 +++++++++---- .../agentDefinitionsRepository.test.ts | 57 +++++++++++++++++++ 4 files changed, 128 insertions(+), 22 deletions(-) diff --git a/src/api/routers/agentDefinitions.ts b/src/api/routers/agentDefinitions.ts index b4c646b4..d60d3efb 100644 --- a/src/api/routers/agentDefinitions.ts +++ b/src/api/routers/agentDefinitions.ts @@ -17,7 +17,7 @@ import { import { getRawTemplate, validateTemplate } from '../../agents/prompts/index.js'; import { deleteAgentDefinition, - getAgentDefinition, + getAgentDefinitionMetadata, listAgentDefinitions, upsertAgentDefinition, } from '../../db/repositories/agentDefinitionsRepository.js'; @@ -145,9 +145,8 @@ export const agentDefinitionsRouter = router({ }), ) .mutation(async ({ input }) => { - // Validate agentType doesn't already exist in DB - // getAgentDefinition returns null for not-found, throws for DB errors - const existing = await getAgentDefinition(input.agentType); + // Validate agentType doesn't already exist in DB without parsing the stored body. + const existing = await getAgentDefinitionMetadata(input.agentType); if (existing !== null) { throw new TRPCError({ code: 'CONFLICT', @@ -192,9 +191,8 @@ export const agentDefinitionsRouter = router({ delete: superAdminProcedure .input(z.object({ agentType: z.string().min(1) })) .mutation(async ({ input }) => { - // Verify the definition exists in DB - // getAgentDefinition returns null for not-found, throws for DB errors - const dbRow = await getAgentDefinition(input.agentType); + // Verify the definition exists in DB without parsing the stored body. + const dbRow = await getAgentDefinitionMetadata(input.agentType); if (dbRow === null) { throw new TRPCError({ code: 'NOT_FOUND', diff --git a/src/db/repositories/agentDefinitionsRepository.ts b/src/db/repositories/agentDefinitionsRepository.ts index 5a101908..e442995d 100644 --- a/src/db/repositories/agentDefinitionsRepository.ts +++ b/src/db/repositories/agentDefinitionsRepository.ts @@ -1,6 +1,7 @@ import { eq } from 'drizzle-orm'; import type { AgentDefinition } from '../../agents/definitions/schema.js'; import { AgentDefinitionSchema } from '../../agents/definitions/schema.js'; +import { logger } from '../../utils/logging.js'; import { getDb } from '../client.js'; import { agentDefinitions } from '../schema/index.js'; @@ -23,11 +24,42 @@ export async function listAgentDefinitions(): Promise< > { const db = getDb(); const rows = await db.select().from(agentDefinitions); - return rows.map((row) => ({ - agentType: row.agentType, - definition: AgentDefinitionSchema.parse(row.definition), - isBuiltin: row.isBuiltin ?? false, - })); + const validRows: Array<{ agentType: string; definition: AgentDefinition; isBuiltin: boolean }> = + []; + + for (const row of rows) { + const result = AgentDefinitionSchema.safeParse(row.definition); + if (!result.success) { + logger.warn('Skipping invalid agent definition from DB', { + agentType: row.agentType, + issues: result.error.issues.map((issue) => ({ + path: issue.path.length > 0 ? issue.path.join('.') : '', + message: issue.message, + })), + }); + continue; + } + + validRows.push({ + agentType: row.agentType, + definition: result.data, + isBuiltin: row.isBuiltin ?? false, + }); + } + + return validRows; +} + +export async function getAgentDefinitionMetadata( + agentType: string, +): Promise<{ agentType: string; isBuiltin: boolean } | null> { + const db = getDb(); + const [row] = await db + .select({ agentType: agentDefinitions.agentType, isBuiltin: agentDefinitions.isBuiltin }) + .from(agentDefinitions) + .where(eq(agentDefinitions.agentType, agentType)); + if (!row) return null; + return { agentType: row.agentType, isBuiltin: row.isBuiltin ?? false }; } export async function upsertAgentDefinition( diff --git a/tests/unit/api/routers/agentDefinitions.test.ts b/tests/unit/api/routers/agentDefinitions.test.ts index b04a2655..be4aa5eb 100644 --- a/tests/unit/api/routers/agentDefinitions.test.ts +++ b/tests/unit/api/routers/agentDefinitions.test.ts @@ -15,7 +15,7 @@ const { mockResolveAgentDefinition, mockResolveKnownAgentTypes, mockListAgentDefinitions, - mockGetAgentDefinition, + mockGetAgentDefinitionMetadata, mockUpsertAgentDefinition, mockDeleteAgentDefinition, mockGetRawTemplate, @@ -29,7 +29,7 @@ const { mockResolveAgentDefinition: vi.fn<(agentType: string) => Promise>(), mockResolveKnownAgentTypes: vi.fn<() => Promise>(), mockListAgentDefinitions: vi.fn(), - mockGetAgentDefinition: vi.fn(), + mockGetAgentDefinitionMetadata: vi.fn(), mockUpsertAgentDefinition: vi.fn(), mockDeleteAgentDefinition: vi.fn(), mockGetRawTemplate: vi.fn<(agentType: string) => string>(), @@ -48,7 +48,7 @@ vi.mock('../../../../src/agents/definitions/loader.js', () => ({ vi.mock('../../../../src/db/repositories/agentDefinitionsRepository.js', () => ({ listAgentDefinitions: mockListAgentDefinitions, - getAgentDefinition: mockGetAgentDefinition, + getAgentDefinitionMetadata: mockGetAgentDefinitionMetadata, upsertAgentDefinition: mockUpsertAgentDefinition, deleteAgentDefinition: mockDeleteAgentDefinition, })); @@ -246,7 +246,7 @@ describe('agentDefinitionsRouter', () => { // ===================================================================== describe('create', () => { it('creates a new definition (superadmin)', async () => { - mockGetAgentDefinition.mockResolvedValue(null); + mockGetAgentDefinitionMetadata.mockResolvedValue(null); mockUpsertAgentDefinition.mockResolvedValue(undefined); const def = createMockDefinition(); @@ -259,7 +259,7 @@ describe('agentDefinitionsRouter', () => { }); it('marks YAML-backed type as builtin on create', async () => { - mockGetAgentDefinition.mockResolvedValue(null); + mockGetAgentDefinitionMetadata.mockResolvedValue(null); mockUpsertAgentDefinition.mockResolvedValue(undefined); const def = createMockDefinition(); @@ -270,7 +270,7 @@ describe('agentDefinitionsRouter', () => { }); it('throws CONFLICT when agent type already exists', async () => { - mockGetAgentDefinition.mockResolvedValue(createMockDefinition()); + mockGetAgentDefinitionMetadata.mockResolvedValue({ agentType: 'existing', isBuiltin: false }); const def = createMockDefinition(); const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); @@ -348,8 +348,10 @@ describe('agentDefinitionsRouter', () => { // ===================================================================== describe('delete', () => { it('deletes a non-builtin definition (superadmin)', async () => { - mockGetAgentDefinition.mockResolvedValue(createMockDefinition()); - mockGetBuiltinAgentTypes.mockReturnValue(['implementation', 'review']); // custom-agent is NOT in this list + mockGetAgentDefinitionMetadata.mockResolvedValue({ + agentType: 'custom-agent', + isBuiltin: false, + }); mockDeleteAgentDefinition.mockResolvedValue(undefined); const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); @@ -360,8 +362,22 @@ describe('agentDefinitionsRouter', () => { expect(mockInvalidateDefinitionCache).toHaveBeenCalled(); }); + it('deletes an invalid DB-only definition without parsing it', async () => { + mockGetAgentDefinitionMetadata.mockResolvedValue({ + agentType: 'email-joke', + isBuiltin: true, + }); + mockDeleteAgentDefinition.mockResolvedValue(undefined); + + const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); + const result = await caller.delete({ agentType: 'email-joke' }); + + expect(result).toEqual({ agentType: 'email-joke' }); + expect(mockDeleteAgentDefinition).toHaveBeenCalledWith('email-joke'); + }); + it('throws NOT_FOUND when definition not in DB', async () => { - mockGetAgentDefinition.mockResolvedValue(null); + mockGetAgentDefinitionMetadata.mockResolvedValue(null); const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); await expect(caller.delete({ agentType: 'missing' })).rejects.toMatchObject({ @@ -370,7 +386,10 @@ describe('agentDefinitionsRouter', () => { }); it('throws FORBIDDEN when trying to delete a builtin (YAML-backed) type', async () => { - mockGetAgentDefinition.mockResolvedValue(createMockDefinition()); + mockGetAgentDefinitionMetadata.mockResolvedValue({ + agentType: 'implementation', + isBuiltin: true, + }); const caller = createCaller({ user: mockSuperAdmin, effectiveOrgId: mockSuperAdmin.orgId }); await expect(caller.delete({ agentType: 'implementation' })).rejects.toMatchObject({ diff --git a/tests/unit/db/repositories/agentDefinitionsRepository.test.ts b/tests/unit/db/repositories/agentDefinitionsRepository.test.ts index 4fa175c4..ccfacb55 100644 --- a/tests/unit/db/repositories/agentDefinitionsRepository.test.ts +++ b/tests/unit/db/repositories/agentDefinitionsRepository.test.ts @@ -4,10 +4,19 @@ import { mockDbClientModule } from '../../../helpers/sharedMocks.js'; vi.mock('../../../../src/db/client.js', () => mockDbClientModule); +const { mockLoggerWarn } = vi.hoisted(() => ({ + mockLoggerWarn: vi.fn(), +})); + +vi.mock('../../../../src/utils/logging.js', () => ({ + logger: { warn: mockLoggerWarn }, +})); + import type { AgentDefinition } from '../../../../src/agents/definitions/schema.js'; import { deleteAgentDefinition, getAgentDefinition, + getAgentDefinitionMetadata, listAgentDefinitions, upsertAgentDefinition, } from '../../../../src/db/repositories/agentDefinitionsRepository.js'; @@ -43,6 +52,7 @@ describe('agentDefinitionsRepository', () => { beforeEach(() => { mockDb = createMockDbWithGetDb({ withUpsert: true }); + mockLoggerWarn.mockClear(); }); describe('getAgentDefinition', () => { @@ -103,6 +113,53 @@ describe('agentDefinitionsRepository', () => { const result = await listAgentDefinitions(); expect(result[0].isBuiltin).toBe(false); }); + + it('skips invalid definitions and keeps valid rows', async () => { + mockDb.chain.from.mockResolvedValueOnce([ + { agentType: 'implementation', definition: mockDefinition, isBuiltin: true }, + { agentType: 'email-joke', definition: { invalid: 'data' }, isBuiltin: true }, + { agentType: 'review', definition: mockDefinition, isBuiltin: false }, + ]); + + const result = await listAgentDefinitions(); + + expect(result.map((row) => row.agentType)).toEqual(['implementation', 'review']); + expect(mockLoggerWarn).toHaveBeenCalledWith( + 'Skipping invalid agent definition from DB', + expect.objectContaining({ + agentType: 'email-joke', + issues: expect.any(Array), + }), + ); + }); + }); + + describe('getAgentDefinitionMetadata', () => { + it('returns metadata without parsing the definition body', async () => { + mockDb.chain.where.mockResolvedValueOnce([ + { agentType: 'email-joke', isBuiltin: true, definition: { invalid: 'data' } }, + ]); + + const result = await getAgentDefinitionMetadata('email-joke'); + + expect(result).toEqual({ agentType: 'email-joke', isBuiltin: true }); + }); + + it('returns null when not found', async () => { + mockDb.chain.where.mockResolvedValueOnce([]); + + const result = await getAgentDefinitionMetadata('missing'); + + expect(result).toBeNull(); + }); + + it('defaults isBuiltin to false when null', async () => { + mockDb.chain.where.mockResolvedValueOnce([{ agentType: 'custom', isBuiltin: null }]); + + const result = await getAgentDefinitionMetadata('custom'); + + expect(result).toEqual({ agentType: 'custom', isBuiltin: false }); + }); }); describe('upsertAgentDefinition', () => {