From 916ff3946ac30eb246fe382d6e00a977204f59f4 Mon Sep 17 00:00:00 2001 From: Arvin A <51036481+DeveloperTheExplorer@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:24:54 +0200 Subject: [PATCH] feat(core): Paginate agent-eval list endpoints and retype response DTOs (no-changelog) (#35425) --- .../__tests__/pagination.dto.test.ts | 16 ++++ .../src/dto/pagination/pagination.dto.ts | 9 +- packages/@n8n/api-types/src/index.ts | 4 + .../src/schemas/agent-evals.schema.ts | 49 +++++++--- .../agent-eval-result.repository.test.ts | 35 ++++++-- .../agent-eval-run.repository.test.ts | 46 ++++++++-- .../agent-eval-result.repository.ee.ts | 19 +++- .../agent-eval-run.repository.ee.ts | 18 +++- ...nt-eval-runner.service.integration.test.ts | 4 +- ...gent-eval-scoped-reads.integration.test.ts | 73 +++++++++++++-- .../__tests__/agent-eval.service.test.ts | 89 ++++++++++++++----- .../__tests__/agent-evals.controller.test.ts | 87 ++++++++++++++++-- .../modules/agent-evals/agent-eval.service.ts | 25 ++++-- .../agent-evals/agent-evals.controller.ts | 43 +++++++-- 14 files changed, 437 insertions(+), 80 deletions(-) diff --git a/packages/@n8n/api-types/src/dto/pagination/__tests__/pagination.dto.test.ts b/packages/@n8n/api-types/src/dto/pagination/__tests__/pagination.dto.test.ts index 18081ac854a..ef6398f0af9 100644 --- a/packages/@n8n/api-types/src/dto/pagination/__tests__/pagination.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/pagination/__tests__/pagination.dto.test.ts @@ -145,5 +145,21 @@ describe('PaginationDto', () => { expect(result.data).toBe(customMaxItems); } }); + + test('should fall back to 10 when no default is given', () => { + expect(createTakeValidator(MAX_ITEMS_PER_PAGE).parse(undefined)).toBe(10); + }); + + test('should apply a custom default when take is omitted', () => { + expect(createTakeValidator(MAX_ITEMS_PER_PAGE, false, 50).parse(undefined)).toBe(50); + }); + + // The default is a fallback, not a floor or ceiling. + test('should let an explicit take override a custom default', () => { + const validator = createTakeValidator(MAX_ITEMS_PER_PAGE, false, 50); + + expect(validator.parse('5')).toBe(5); + expect(validator.parse('9999')).toBe(MAX_ITEMS_PER_PAGE); + }); }); }); diff --git a/packages/@n8n/api-types/src/dto/pagination/pagination.dto.ts b/packages/@n8n/api-types/src/dto/pagination/pagination.dto.ts index eef87cd1ed0..5493e6c5b77 100644 --- a/packages/@n8n/api-types/src/dto/pagination/pagination.dto.ts +++ b/packages/@n8n/api-types/src/dto/pagination/pagination.dto.ts @@ -15,11 +15,16 @@ const skipValidator = z message: 'Param `skip` must be a non-negative integer', }); -export const createTakeValidator = (maxItems: number, allowInfinity: boolean = false) => +/** `defaultTake` applies when a request omits `take`. */ +export const createTakeValidator = ( + maxItems: number, + allowInfinity: boolean = false, + defaultTake: number = 10, +) => z .string() .optional() - .transform((val) => (val ? parseInt(val, 10) : 10)) + .transform((val) => (val ? parseInt(val, 10) : defaultTake)) .refine((val) => !isNaN(val) && Number.isInteger(val), { message: 'Param `take` must be a valid integer', }) diff --git a/packages/@n8n/api-types/src/index.ts b/packages/@n8n/api-types/src/index.ts index 5f6f42be3d6..bb2219529c0 100644 --- a/packages/@n8n/api-types/src/index.ts +++ b/packages/@n8n/api-types/src/index.ts @@ -585,6 +585,8 @@ export { UpdateAgentEvalDatasetDto, createAgentEvalRunSchema, CreateAgentEvalRunDto, + AGENT_EVAL_RESULTS_DEFAULT_TAKE, + AgentEvalRunDetailQueryDto, agentEvalCorrectionSchema, createAgentEvalRatingSchema, CreateAgentEvalRatingDto, @@ -602,10 +604,12 @@ export type { UpdateAgentEvalDatasetPayload, CreateAgentEvalRunPayload, CreateAgentEvalRatingPayload, + AgentEvalPage, AgentEvalDatasetRecord, AgentEvalRunRecord, AgentEvalResultRecord, AgentEvalRatingRecord, + AgentEvalRunList, AgentEvalRunDetail, AgentEvalRunSummary, AgentEvalDraftCase, diff --git a/packages/@n8n/api-types/src/schemas/agent-evals.schema.ts b/packages/@n8n/api-types/src/schemas/agent-evals.schema.ts index 432bf15b9ea..87717a97d2f 100644 --- a/packages/@n8n/api-types/src/schemas/agent-evals.schema.ts +++ b/packages/@n8n/api-types/src/schemas/agent-evals.schema.ts @@ -1,7 +1,12 @@ -import type { JsonValue } from 'n8n-workflow'; +import type { IDataObject, JsonObject, JsonValue } from 'n8n-workflow'; import { z } from 'zod'; import { datasetRefSchema, type DatasetRef } from '../dto/evaluations/evaluation-config.dto'; +import { + MAX_ITEMS_PER_PAGE, + createTakeValidator, + paginationSchema, +} from '../dto/pagination/pagination.dto'; import { Z } from '../zod-class'; // A JSON blob that flows from a request into persistence must infer to the @@ -107,6 +112,14 @@ export const createAgentEvalRunSchema = z.object(createAgentEvalRunShape); export type CreateAgentEvalRunPayload = z.infer; export class CreateAgentEvalRunDto extends Z.class(createAgentEvalRunShape) {} +// Wider default than the shared `PaginationDto`: opening a run reads its cases, +// of which there are many, so 10 would page immediately for no reason. +export const AGENT_EVAL_RESULTS_DEFAULT_TAKE = 50; +export class AgentEvalRunDetailQueryDto extends Z.class({ + ...paginationSchema, + take: createTakeValidator(MAX_ITEMS_PER_PAGE, false, AGENT_EVAL_RESULTS_DEFAULT_TAKE), +}) {} + // A correction carries the edited answer under `finalText`, mirroring the key the // runner writes into a result's `output` so calibration can diff the two directly. // Required when a correction is sent: a correction without it persists a rating @@ -134,8 +147,17 @@ export class CreateAgentEvalRatingDto extends Z.class(createAgentEvalRatingShape // own output through validation. Dates are serialized as ISO strings; internal // coordination columns (`runningInstanceId`, `cancelRequested`) are omitted // from the contract. +// +// JSON blobs mirror the type their entity column stores, so consumers needn't +// re-narrow `unknown` on every read. // --------------------------------------------------------------------------- +/** One page of a list route. `count` is the total matching rows, not the page length. */ +export type AgentEvalPage = { + count: number; + data: T[]; +}; + export type AgentEvalDatasetRecord = { id: string; name: string; @@ -154,9 +176,9 @@ export type AgentEvalRunRecord = { status: AgentEvalRunStatus; runAt: string | null; completedAt: string | null; - metrics: Record | null; + metrics: IDataObject | null; errorCode: string | null; - errorDetails: Record | null; + errorDetails: IDataObject | null; createdById: string | null; createdAt: string; updatedAt: string; @@ -168,14 +190,14 @@ export type AgentEvalResultRecord = { sourceRowId: string | null; runIndex: number | null; status: AgentEvalResultStatus; - input: Record | null; - output: Record | null; - toolCalls: Record | null; - metrics: Record | null; + input: JsonObject | null; + output: JsonObject | null; + toolCalls: JsonObject | null; + metrics: IDataObject | null; runAt: string | null; completedAt: string | null; errorCode: string | null; - errorDetails: Record | null; + errorDetails: IDataObject | null; createdAt: string; updatedAt: string; }; @@ -185,15 +207,20 @@ export type AgentEvalRatingRecord = { resultId: string; vote: AgentEvalVote; comment: string | null; - correction: Record | null; + correction: JsonObject | null; ratedById: string | null; createdAt: string; updatedAt: string; }; -// A run with its per-case results — the "open a run" view. +// Runs of a dataset, newest first. Paginated because nothing caps how many a +// dataset accumulates — every "Run all" adds one. +export type AgentEvalRunList = AgentEvalPage; + +// A run with a page of its per-case results — the "open a run" view. Paginated +// because each row carries its full input/output JSON. export type AgentEvalRunDetail = AgentEvalRunRecord & { - results: AgentEvalResultRecord[]; + results: AgentEvalPage; }; // The progress-polling shape: status plus tallies, no per-case rows, so polling diff --git a/packages/@n8n/db/src/repositories/__tests__/agent-eval-result.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/agent-eval-result.repository.test.ts index f23a2c1d859..bb347f8f51d 100644 --- a/packages/@n8n/db/src/repositories/__tests__/agent-eval-result.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/agent-eval-result.repository.test.ts @@ -115,14 +115,39 @@ describe('AgentEvalResultRepository', () => { }); }); - describe('findByRunId', () => { + describe('findAndCountByRunId', () => { it('scopes to runId ordered by runIndex ascending', async () => { - entityManager.find.mockResolvedValueOnce([]); + entityManager.findAndCount.mockResolvedValueOnce([[], 0]); - await repo.findByRunId('run-1'); + await repo.findAndCountByRunId('run-1'); - const callArgs = entityManager.find.mock.calls[0]; - expect(callArgs?.[1]).toEqual({ where: { runId: 'run-1' }, order: { runIndex: 'ASC' } }); + expect(entityManager.findAndCount.mock.calls[0]?.[1]).toMatchObject({ + where: { runId: 'run-1' }, + order: { runIndex: 'ASC' }, + }); + }); + + // Must reach the query: slicing in memory would load the full + // input/output/toolCalls JSON that pagination exists to avoid. + it('pushes the page window into the query rather than slicing in memory', async () => { + entityManager.findAndCount.mockResolvedValueOnce([[], 0]); + + await repo.findAndCountByRunId('run-1', { take: 25, skip: 50 }); + + expect(entityManager.findAndCount.mock.calls[0]?.[1]).toMatchObject({ + take: 25, + skip: 50, + }); + }); + + it('returns the unpaginated total alongside the page', async () => { + const page = [{ id: 'res-1' }] as AgentEvalResult[]; + entityManager.findAndCount.mockResolvedValueOnce([page, 500]); + + const [results, count] = await repo.findAndCountByRunId('run-1', { take: 1, skip: 0 }); + + expect(results).toEqual(page); + expect(count).toBe(500); }); }); diff --git a/packages/@n8n/db/src/repositories/__tests__/agent-eval-run.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/agent-eval-run.repository.test.ts index e204a473901..0a5f0b08e6e 100644 --- a/packages/@n8n/db/src/repositories/__tests__/agent-eval-run.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/agent-eval-run.repository.test.ts @@ -185,17 +185,53 @@ describe('AgentEvalRunRepository', () => { }); }); - describe('findByDatasetIdAndAgentId', () => { + describe('findAndCountByDatasetIdAndAgentId', () => { it('constrains the dataset’s runs by that agent, newest first', async () => { - entityManager.find.mockResolvedValueOnce([]); + entityManager.findAndCount.mockResolvedValueOnce([[], 0]); - await repo.findByDatasetIdAndAgentId('ds-1', 'agent-1'); + await repo.findAndCountByDatasetIdAndAgentId('ds-1', 'agent-1'); - expect(entityManager.find.mock.calls[0]?.[1]).toEqual({ + expect(entityManager.findAndCount.mock.calls[0]?.[1]).toMatchObject({ where: { datasetId: 'ds-1', dataset: { agentId: 'agent-1' } }, - order: { createdAt: 'DESC' }, }); }); + + // `createdAt` alone is not a total order, so without the tiebreak a run + // sharing a timestamp could show up on two pages, or on none. + it('breaks createdAt ties deterministically so paging cannot skip a run', async () => { + entityManager.findAndCount.mockResolvedValueOnce([[], 0]); + + await repo.findAndCountByDatasetIdAndAgentId('ds-1', 'agent-1'); + + expect(entityManager.findAndCount.mock.calls[0]?.[1]).toMatchObject({ + order: { createdAt: 'DESC', id: 'DESC' }, + }); + }); + + // The point of the method: one page from the database, not every run. + it('pushes the page window into the query', async () => { + entityManager.findAndCount.mockResolvedValueOnce([[], 0]); + + await repo.findAndCountByDatasetIdAndAgentId('ds-1', 'agent-1', { take: 20, skip: 40 }); + + expect(entityManager.findAndCount.mock.calls[0]?.[1]).toMatchObject({ + take: 20, + skip: 40, + }); + }); + + it('returns the unpaginated total alongside the page', async () => { + const page = [{ id: 'run-1' }, { id: 'run-2' }] as AgentEvalRun[]; + entityManager.findAndCount.mockResolvedValueOnce([page, 137]); + + const [runs, count] = await repo.findAndCountByDatasetIdAndAgentId('ds-1', 'agent-1', { + take: 2, + skip: 0, + }); + + expect(runs).toEqual(page); + expect(count).toBe(137); + }); }); describe('isCancellationRequested', () => { diff --git a/packages/@n8n/db/src/repositories/agent-eval-result.repository.ee.ts b/packages/@n8n/db/src/repositories/agent-eval-result.repository.ee.ts index c6fe41f7058..257d5b2b64f 100644 --- a/packages/@n8n/db/src/repositories/agent-eval-result.repository.ee.ts +++ b/packages/@n8n/db/src/repositories/agent-eval-result.repository.ee.ts @@ -34,7 +34,7 @@ export class AgentEvalResultRepository extends Repository { status: 'new', runId: c.runId, sourceRowId: c.sourceRowId ?? null, - // Fall back to the seed position so `findByRunId` (orders by + // Fall back to the seed position so `findAndCountByRunId` (orders by // runIndex ASC) returns a stable order on every database. A null // runIndex would sort first on SQLite but last on Postgres. runIndex: c.runIndex ?? index, @@ -103,8 +103,21 @@ export class AgentEvalResultRepository extends Repository { }); } - async findByRunId(runId: string): Promise { - return await this.find({ where: { runId }, order: { runIndex: 'ASC' } }); + /** + * Paged in SQL, not by slicing a full read: every row carries its `input`, + * `output` and `toolCalls` JSON. `runIndex` is distinct per run, so it is + * already a total order and needs no tiebreak. + */ + async findAndCountByRunId( + runId: string, + options: { skip?: number; take?: number } = {}, + ): Promise<[AgentEvalResult[], number]> { + return await this.findAndCount({ + where: { runId }, + order: { runIndex: 'ASC' }, + skip: options.skip, + take: options.take, + }); } async findById(id: string): Promise { diff --git a/packages/@n8n/db/src/repositories/agent-eval-run.repository.ee.ts b/packages/@n8n/db/src/repositories/agent-eval-run.repository.ee.ts index 586376582a6..5658e517a49 100644 --- a/packages/@n8n/db/src/repositories/agent-eval-run.repository.ee.ts +++ b/packages/@n8n/db/src/repositories/agent-eval-run.repository.ee.ts @@ -96,11 +96,21 @@ export class AgentEvalRunRepository extends Repository { return await this.findOne({ where: { id, dataset: { agentId } } }); } - /** Runs of a dataset, scoped to the agent that dataset must belong to. */ - async findByDatasetIdAndAgentId(datasetId: string, agentId: string): Promise { - return await this.find({ + /** + * One page of a dataset's runs, newest first, scoped to its agent. The `id` + * tiebreak is what keeps equal-`createdAt` rows from landing on two pages or + * none — ids are random nanoids, so it orders nothing, it just stays stable. + */ + async findAndCountByDatasetIdAndAgentId( + datasetId: string, + agentId: string, + options: { skip?: number; take?: number } = {}, + ): Promise<[AgentEvalRun[], number]> { + return await this.findAndCount({ where: { datasetId, dataset: { agentId } }, - order: { createdAt: 'DESC' }, + order: { createdAt: 'DESC', id: 'DESC' }, + skip: options.skip, + take: options.take, }); } diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.integration.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.integration.test.ts index 973ea835d22..73954f43dc0 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.integration.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.integration.test.ts @@ -158,7 +158,7 @@ describe('AgentEvalRunnerService (integration)', () => { const inputsSent = evalAgentExecutionService.executeWithLlmMock.mock.calls.map((c) => c[3]); expect(inputsSent).toEqual(expect.arrayContaining(['What is 2+2?', 'Capital of France?'])); - const results = await Container.get(AgentEvalResultRepository).findByRunId(runId); + const [results] = await Container.get(AgentEvalResultRepository).findAndCountByRunId(runId); expect(results).toHaveLength(2); for (const result of results) { expect(result.status).toBe('success'); @@ -217,7 +217,7 @@ describe('AgentEvalRunnerService (integration)', () => { expect(summary.status).toBe('completed'); expect(summary.counts).toMatchObject({ total: 1, success: 0, error: 1 }); - const [result] = await Container.get(AgentEvalResultRepository).findByRunId(runId); + const [[result]] = await Container.get(AgentEvalResultRepository).findAndCountByRunId(runId); expect(result.status).toBe('error'); expect(result.errorCode).toBe('execution_failed'); }); diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-scoped-reads.integration.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-scoped-reads.integration.test.ts index f1c6191cd8b..1c53c92c1f0 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-scoped-reads.integration.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-scoped-reads.integration.test.ts @@ -102,17 +102,78 @@ describe('agent-eval scoped repository reads (integration)', () => { await expect(runRepository.findByIdAndAgentId(run.id, foreign.id)).resolves.toBeNull(); }); + // TypeORM pages a relation-filtered query with a distinct sub-query, so only + // a real database proves it. Every run shares one `createdAt`, which is the + // case the `id` tiebreak exists for. + it('walks a dataset’s runs page by page without repeating or dropping one', async () => { + const { own, dataset } = await seedTwoAgents(); + // One run exists from the seed; five more leave a partial second page. + for (let i = 0; i < 5; i++) await runRepository.createRun({ datasetId: dataset.id }); + // `@CreateDateColumn` only auto-fills on insert, so pin them explicitly. + const sharedCreatedAt = new Date('2026-01-01T00:00:00.000Z'); + await runRepository.update({ datasetId: dataset.id }, { createdAt: sharedCreatedAt }); + + const [firstPage, total] = await runRepository.findAndCountByDatasetIdAndAgentId( + dataset.id, + own.id, + { take: 4, skip: 0 }, + ); + const [secondPage] = await runRepository.findAndCountByDatasetIdAndAgentId( + dataset.id, + own.id, + { take: 4, skip: 4 }, + ); + // The same rows read in one go, as the yardstick for the paged walk. + const [whole] = await runRepository.findAndCountByDatasetIdAndAgentId(dataset.id, own.id); + + // The count is the whole set, not the page. + expect(total).toBe(6); + expect(firstPage).toHaveLength(4); + expect(secondPage).toHaveLength(2); + + const walked = [...firstPage, ...secondPage].map((r) => r.id); + // No run repeated across pages, and none lost between them. + expect(new Set(walked).size).toBe(6); + // Paging agrees with reading everything at once — compared against the + // database's own ordering, since its collation is not JS's sort order. + expect(walked).toEqual(whole.map((r) => r.id)); + }); + + it('pages a run’s cases in seed order and counts them all', async () => { + const { dataset } = await seedTwoAgents(); + const run = await runRepository.createRun({ datasetId: dataset.id }); + await resultRepository.seedResults( + Array.from({ length: 5 }, (_, i) => ({ + runId: run.id, + sourceRowId: `row-${i}`, + runIndex: i, + input: { input: `q${i}` }, + })), + ); + + const [page, total] = await resultRepository.findAndCountByRunId(run.id, { + take: 2, + skip: 2, + }); + + // `seedTwoAgents` seeded a case under another run, so 5 rather than 6 + // is what shows the count is scoped to this one. + expect(total).toBe(5); + expect(page.map((r) => r.sourceRowId)).toEqual(['row-2', 'row-3']); + }); + it('lists a dataset’s runs only for that dataset’s agent', async () => { const { own, foreign, dataset, run } = await seedTwoAgents(); await expect( - runRepository.findByDatasetIdAndAgentId(dataset.id, own.id), - ).resolves.toMatchObject([{ id: run.id }]); + runRepository.findAndCountByDatasetIdAndAgentId(dataset.id, own.id), + ).resolves.toMatchObject([[{ id: run.id }], 1]); // Right dataset id, wrong agent: the pairing has to be checked, or a foreign - // caller reads another agent's run history. + // caller reads another agent's run history — and a bare count would leak + // how many runs that agent has. await expect( - runRepository.findByDatasetIdAndAgentId(dataset.id, foreign.id), - ).resolves.toEqual([]); + runRepository.findAndCountByDatasetIdAndAgentId(dataset.id, foreign.id), + ).resolves.toEqual([[], 0]); }); }); @@ -154,7 +215,7 @@ describe('agent-eval scoped repository reads (integration)', () => { await expect(datasetRepository.findById(dataset.id)).resolves.toBeNull(); await expect(runRepository.findById(run.id)).resolves.toBeNull(); - await expect(resultRepository.findByRunId(run.id)).resolves.toEqual([]); + await expect(resultRepository.findAndCountByRunId(run.id)).resolves.toEqual([[], 0]); expect(result).toBeDefined(); }); }); diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-eval.service.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-eval.service.test.ts index 4e44b88e285..36f2dc2b854 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-eval.service.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-eval.service.test.ts @@ -33,6 +33,8 @@ vi.mock('../agent-eval-case-generation.service', () => ({ const AGENT_ID = 'agent-1'; const PROJECT_ID = 'proj-1'; +// What `PaginationDto` hands the service once the route has defaulted it. +const PAGE = { take: 10, skip: 0 }; describe('AgentEvalService', () => { const user = mock({ id: 'user-1' }); @@ -93,6 +95,8 @@ describe('AgentEvalService', () => { agentRepository.findByIdAndProjectId.mockResolvedValue(mock({ id: AGENT_ID })); datasetRepository.findByIdAndAgentId.mockResolvedValue(makeDataset()); runRepository.findByIdAndAgentId.mockResolvedValue(makeRun()); + runRepository.findAndCountByDatasetIdAndAgentId.mockResolvedValue([[], 0]); + resultRepository.findAndCountByRunId.mockResolvedValue([[], 0]); service = new AgentEvalService( moduleRegistry, @@ -132,8 +136,8 @@ describe('AgentEvalService', () => { async () => await service.generateDraftCases(user, AGENT_ID, PROJECT_ID, {}), ], ['startRun', async () => await service.startRun(user, AGENT_ID, PROJECT_ID, 'ds-1', {})], - ['listRuns', async () => await service.listRuns(AGENT_ID, PROJECT_ID, 'ds-1')], - ['getRunDetail', async () => await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1')], + ['listRuns', async () => await service.listRuns(AGENT_ID, PROJECT_ID, 'ds-1', PAGE)], + ['getRunDetail', async () => await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1', PAGE)], ['getRunSummary', async () => await service.getRunSummary(AGENT_ID, PROJECT_ID, 'run-1')], ['cancelRun', async () => await service.cancelRun(AGENT_ID, PROJECT_ID, 'run-1')], ]; @@ -184,10 +188,10 @@ describe('AgentEvalService', () => { it('404s a run belonging to another agent, and reads no results for it', async () => { runRepository.findByIdAndAgentId.mockResolvedValue(null); - await expect(service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-other')).rejects.toThrow( + await expect(service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-other', PAGE)).rejects.toThrow( NotFoundError, ); - expect(resultRepository.findByRunId).not.toHaveBeenCalled(); + expect(resultRepository.findAndCountByRunId).not.toHaveBeenCalled(); }); it('refuses to start a run on another agent’s dataset, without touching the runner', async () => { @@ -329,9 +333,7 @@ describe('AgentEvalService', () => { describe('response mapping', () => { it('serializes dates as ISO strings and keeps run coordination columns off the wire', async () => { - resultRepository.findByRunId.mockResolvedValue([]); - - const detail = await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1'); + const detail = await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1', PAGE); expect(detail.runAt).toBe('2026-01-03T00:00:00.000Z'); expect(detail.createdAt).toBe('2026-01-03T00:00:00.000Z'); @@ -382,23 +384,26 @@ describe('AgentEvalService', () => { ); }); - it('includes every per-case result in a run detail', async () => { - resultRepository.findByRunId.mockResolvedValue([ - mock({ - id: 'res-1', - runId: 'run-1', - status: 'success', - runAt: new Date('2026-01-03T00:00:01.000Z'), - completedAt: new Date('2026-01-03T00:00:02.000Z'), - createdAt: new Date('2026-01-03T00:00:00.000Z'), - updatedAt: new Date('2026-01-03T00:00:02.000Z'), - }), + it('maps every per-case result on the requested page of a run detail', async () => { + resultRepository.findAndCountByRunId.mockResolvedValue([ + [ + mock({ + id: 'res-1', + runId: 'run-1', + status: 'success', + runAt: new Date('2026-01-03T00:00:01.000Z'), + completedAt: new Date('2026-01-03T00:00:02.000Z'), + createdAt: new Date('2026-01-03T00:00:00.000Z'), + updatedAt: new Date('2026-01-03T00:00:02.000Z'), + }), + ], + 1, ]); - const detail = await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1'); + const detail = await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1', PAGE); - expect(detail.results).toHaveLength(1); - expect(detail.results[0]).toMatchObject({ + expect(detail.results.data).toHaveLength(1); + expect(detail.results.data[0]).toMatchObject({ id: 'res-1', status: 'success', runAt: '2026-01-03T00:00:01.000Z', @@ -407,6 +412,48 @@ describe('AgentEvalService', () => { }); }); + // The window has to reach the repositories: slicing in memory would still + // grow without bound as a dataset ages. + describe('pagination', () => { + it('passes the run-list window to the repository and reports the total', async () => { + runRepository.findAndCountByDatasetIdAndAgentId.mockResolvedValue([[makeRun()], 342]); + + const page = await service.listRuns(AGENT_ID, PROJECT_ID, 'ds-1', { take: 25, skip: 50 }); + + expect(runRepository.findAndCountByDatasetIdAndAgentId).toHaveBeenCalledWith( + 'ds-1', + AGENT_ID, + { take: 25, skip: 50 }, + ); + expect(page).toMatchObject({ count: 342 }); + expect(page.data).toHaveLength(1); + }); + + it('passes the run-detail window to the repository and reports the case total', async () => { + resultRepository.findAndCountByRunId.mockResolvedValue([[], 500]); + + const detail = await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1', { + take: 25, + skip: 50, + }); + + expect(resultRepository.findAndCountByRunId).toHaveBeenCalledWith('run-1', { + take: 25, + skip: 50, + }); + expect(detail.results).toEqual({ count: 500, data: [] }); + }); + + // A client sizing its pager off `data.length` would misread the total. + it('reports a total larger than the page it returned', async () => { + runRepository.findAndCountByDatasetIdAndAgentId.mockResolvedValue([[makeRun()], 342]); + + const page = await service.listRuns(AGENT_ID, PROJECT_ID, 'ds-1', { take: 1, skip: 0 }); + + expect(page.count).toBeGreaterThan(page.data.length); + }); + }); + describe('generateDraftCases', () => { it('delegates with the project resolved from the URL', async () => { caseGenerationService.generateDraftCases.mockResolvedValue({ diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-evals.controller.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-evals.controller.test.ts index ec32e1680a7..a3623f47225 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-evals.controller.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-evals.controller.test.ts @@ -1,3 +1,9 @@ +import { + AGENT_EVAL_RESULTS_DEFAULT_TAKE, + AgentEvalRunDetailQueryDto, + MAX_ITEMS_PER_PAGE, + PaginationDto, +} from '@n8n/api-types'; import type { AuthenticatedRequest, User } from '@n8n/db'; import { ControllerRegistryMetadata } from '@n8n/decorators'; import { Container } from '@n8n/di'; @@ -19,6 +25,10 @@ vi.mock('../agent-evals-flag-gate', () => ({ AgentEvalsFlagGate: class AgentEval const PROJECT_ID = 'proj-1'; const AGENT_ID = 'agent-1'; +// What each route binds when the client sends no window. The two DTOs differ: +// run detail pages cases, so it defaults wider. +const PAGE = PaginationDto.parse({}); +const RUN_PAGE = AgentEvalRunDetailQueryDto.parse({}); describe('AgentEvalsController', () => { const user = mock({ id: 'user-1' }); @@ -72,10 +82,9 @@ describe('AgentEvalsController', () => { expect(route.accessScope?.scope.startsWith('agent:')).toBe(true); }); - // Reads stay on agent:read; anything that writes eval config — including - // generation, which spends the builder's model credits, and rating, whose - // corrections seed judge calibration — needs agent:update; - // starting/cancelling a run is agent:execute. + // Reads on agent:read; eval-config writes — generation, which spends the + // builder's model credits, rating, and cancellation, which acts on a run + // someone else started — on agent:update. Only starting is agent:execute. const expectedScopes = { listDatasets: 'agent:read', getDataset: 'agent:read', @@ -89,8 +98,8 @@ describe('AgentEvalsController', () => { deleteDataset: 'agent:update', generateDraftCases: 'agent:update', rateResult: 'agent:update', + cancelRun: 'agent:update', startRun: 'agent:execute', - cancelRun: 'agent:execute', } as const; it.each(Object.entries(expectedScopes))('%s uses %s', (handlerName, scope) => { @@ -104,6 +113,13 @@ describe('AgentEvalsController', () => { Object.keys(expectedScopes).sort(), ); }); + + // `PROJECT_CHAT_USER_SCOPES` is only `agent:execute` + `workflow:execute-chat`, + // so cancel on `agent:execute` would let a chat-only user stop any run. + it('keeps cancel off the scope a chat-only user holds', () => { + expect(metadata.routes.get('cancelRun')?.accessScope?.scope).not.toBe('agent:execute'); + expect(metadata.routes.get('startRun')?.accessScope?.scope).toBe('agent:execute'); + }); }); // Flag-off must look like an unknown endpoint on every route, and must not @@ -137,8 +153,8 @@ describe('AgentEvalsController', () => { async () => await controller.generateDraftCases(agentReq(), undefined, {}), ], ['startRun', async () => await controller.startRun(datasetReq(), undefined, {})], - ['listRuns', async () => await controller.listRuns(datasetReq())], - ['getRun', async () => await controller.getRun(runReq())], + ['listRuns', async () => await controller.listRuns(datasetReq(), undefined, PAGE)], + ['getRun', async () => await controller.getRun(runReq(), undefined, RUN_PAGE)], ['getRunSummary', async () => await controller.getRunSummary(runReq())], ['cancelRun', async () => await controller.cancelRun(runReq())], [ @@ -276,4 +292,61 @@ describe('AgentEvalsController', () => { ); }); }); + + // A controller that dropped `query` would silently serve the default page. + describe('pagination', () => { + it('forwards the run-list window to the service', async () => { + const query = PaginationDto.parse({ take: '25', skip: '50' }); + + await controller.listRuns(datasetReq(), undefined, query); + + expect(service.listRuns).toHaveBeenCalledWith(AGENT_ID, PROJECT_ID, 'ds-1', { + take: 25, + skip: 50, + }); + }); + + it('forwards the run-detail window to the service', async () => { + const query = AgentEvalRunDetailQueryDto.parse({ take: '25', skip: '50' }); + + await controller.getRun(runReq(), undefined, query); + + expect(service.getRunDetail).toHaveBeenCalledWith(AGENT_ID, PROJECT_ID, 'run-1', { + take: 25, + skip: 50, + }); + }); + + // The DTO always supplies a window, so neither route defaults to unbounded. + it('bounds both list reads even when the client sends no window', async () => { + await controller.listRuns(datasetReq(), undefined, PaginationDto.parse({})); + await controller.getRun(runReq(), undefined, AgentEvalRunDetailQueryDto.parse({})); + + for (const call of [service.listRuns, service.getRunDetail]) { + const page = call.mock.calls[0]?.at(-1) as { take: number; skip: number }; + expect(page.take).toBeGreaterThan(0); + expect(page.skip).toBe(0); + } + }); + + // Opening a run reads its cases, so this route defaults wider. + it('defaults the run-detail window wider than the shared list default', async () => { + await controller.getRun(runReq(), undefined, AgentEvalRunDetailQueryDto.parse({})); + + expect(service.getRunDetail).toHaveBeenCalledWith(AGENT_ID, PROJECT_ID, 'run-1', { + take: AGENT_EVAL_RESULTS_DEFAULT_TAKE, + skip: 0, + }); + expect(AGENT_EVAL_RESULTS_DEFAULT_TAKE).toBeGreaterThan(PaginationDto.parse({}).take); + }); + + // A bigger ask is still held to the shared cap. + it('clamps an oversized run-detail window to the shared maximum', async () => { + const query = AgentEvalRunDetailQueryDto.parse({ take: '9999' }); + + await controller.getRun(runReq(), undefined, query); + + expect(query.take).toBe(MAX_ITEMS_PER_PAGE); + }); + }); }); diff --git a/packages/cli/src/modules/agent-evals/agent-eval.service.ts b/packages/cli/src/modules/agent-evals/agent-eval.service.ts index 9ab87de88ab..42a9b93134d 100644 --- a/packages/cli/src/modules/agent-evals/agent-eval.service.ts +++ b/packages/cli/src/modules/agent-evals/agent-eval.service.ts @@ -1,6 +1,7 @@ import type { AgentEvalDatasetRecord, AgentEvalRunDetail, + AgentEvalRunList, AgentEvalRunRecord, AgentEvalRunSummary, CreateAgentEvalDatasetDto, @@ -30,6 +31,12 @@ import { assertRequiredModulesActive } from './agent-evals-required-modules'; /** Statuses a run can still be asked to stop from. */ const CANCELLABLE_STATUSES = new Set(['new', 'running']); +/** + * Required, not optional, so a caller can't forget to pass a window. Note a + * `take` of 0 is n8n's "no limit" idiom, so this bounds the default path only. + */ +type PageParams = { take: number; skip: number }; + /** * Dataset CRUD, run reads and cancellation behind the agent-eval REST routes. * @@ -158,23 +165,29 @@ export class AgentEvalService { agentId: string, projectId: string, datasetId: string, - ): Promise { + page: PageParams, + ): Promise { await this.assertAgentInProject(agentId, projectId); await this.resolveDataset(agentId, datasetId); - const runs = await this.runRepository.findByDatasetIdAndAgentId(datasetId, agentId); - return runs.map(toRunRecord); + const [runs, count] = await this.runRepository.findAndCountByDatasetIdAndAgentId( + datasetId, + agentId, + page, + ); + return { count, data: runs.map(toRunRecord) }; } - /** A run with every per-case result — the "open a run" view. */ + /** A run with a page of its per-case results — the "open a run" view. */ async getRunDetail( agentId: string, projectId: string, runId: string, + page: PageParams, ): Promise { await this.assertAgentInProject(agentId, projectId); const run = await this.resolveRun(agentId, runId); - const results = await this.resultRepository.findByRunId(runId); - return { ...toRunRecord(run), results: results.map(toResultRecord) }; + const [results, count] = await this.resultRepository.findAndCountByRunId(runId, page); + return { ...toRunRecord(run), results: { count, data: results.map(toResultRecord) } }; } // Per-case status counts for progress polling, ownership-resolved first so this diff --git a/packages/cli/src/modules/agent-evals/agent-evals.controller.ts b/packages/cli/src/modules/agent-evals/agent-evals.controller.ts index 4c2f04da5bf..dab8aed25d0 100644 --- a/packages/cli/src/modules/agent-evals/agent-evals.controller.ts +++ b/packages/cli/src/modules/agent-evals/agent-evals.controller.ts @@ -1,18 +1,30 @@ import { + AgentEvalRunDetailQueryDto, CreateAgentEvalRatingDto, CreateAgentEvalRunDto, GenerateDraftCasesOptionsDto, + PaginationDto, UpdateAgentEvalDatasetDto, createAgentEvalDatasetSchema, type AgentEvalDatasetRecord, type AgentEvalRatingRecord, type AgentEvalRunDetail, + type AgentEvalRunList, type AgentEvalRunRecord, type AgentEvalRunSummary, type GenerateDraftCasesResult, } from '@n8n/api-types'; import type { AuthenticatedRequest } from '@n8n/db'; -import { Body, Delete, Get, Patch, Post, ProjectScope, RestController } from '@n8n/decorators'; +import { + Body, + Delete, + Get, + Patch, + Post, + ProjectScope, + Query, + RestController, +} from '@n8n/decorators'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; @@ -34,9 +46,12 @@ type ResultParam = AgentParam & { resultId: string }; * `101_agent_evals` rollout — the services behind it check neither, so a route * added here without a scope decorator and a `flagGate` call is an open one. * - * `agent:read` for reads, `agent:execute` for running a run, `agent:update` for + * `agent:read` for reads, `agent:execute` for starting a run, `agent:update` for * eval-config writes — including generation, which spends the builder's model - * credits and so must stay closed to viewers (they hold `agent:execute`). + * credits, and cancellation, which acts on a run someone else started. Both must + * stay closed to viewers, who hold `agent:execute`. + * + * Paged reads answer with `{ count, data }`, where `count` is the total. */ @RestController('/projects/:projectId/agents/v2') export class AgentEvalsController { @@ -130,20 +145,30 @@ export class AgentEvalsController { return await this.service.startRun(req.user, agentId, projectId, datasetId, payload); } + /** Newest first. Unbounded over a dataset's life, so the window is required. */ @Get('/:agentId/evals/datasets/:datasetId/runs') @ProjectScope('agent:read') - async listRuns(req: AuthenticatedRequest): Promise { + async listRuns( + req: AuthenticatedRequest, + _res: unknown, + @Query query: PaginationDto, + ): Promise { await this.flagGate.assertEnabled(req.user); const { agentId, projectId, datasetId } = req.params; - return await this.service.listRuns(agentId, projectId, datasetId); + return await this.service.listRuns(agentId, projectId, datasetId, query); } + /** `take`/`skip` page the run's cases, not the run itself. */ @Get('/:agentId/evals/runs/:runId') @ProjectScope('agent:read') - async getRun(req: AuthenticatedRequest): Promise { + async getRun( + req: AuthenticatedRequest, + _res: unknown, + @Query query: AgentEvalRunDetailQueryDto, + ): Promise { await this.flagGate.assertEnabled(req.user); const { agentId, projectId, runId } = req.params; - return await this.service.getRunDetail(agentId, projectId, runId); + return await this.service.getRunDetail(agentId, projectId, runId, query); } /** Counts only — cheap enough for the UI to poll while a run is in flight. */ @@ -155,8 +180,10 @@ export class AgentEvalsController { return await this.service.getRunSummary(agentId, projectId, runId); } + // Cancelling stops work someone else started, so it's a write, not an + // execution — and `agent:execute` is all a chat-only user holds. @Post('/:agentId/evals/runs/:runId/cancel') - @ProjectScope('agent:execute') + @ProjectScope('agent:update') async cancelRun(req: AuthenticatedRequest): Promise { await this.flagGate.assertEnabled(req.user); const { agentId, projectId, runId } = req.params;