From dd2566338a3f067c9beeefe04a93d1fa8a8cb7ba Mon Sep 17 00:00:00 2001 From: Arvin A <51036481+DeveloperTheExplorer@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:04:05 +0200 Subject: [PATCH] feat(core): Add REST endpoints for agent-eval ratings (no-changelog) (#35419) --- .../agent-eval-rating.service.test.ts | 224 ++++++++++-------- .../agent-eval-runner.service.test.ts | 3 + .../__tests__/agent-eval.service.test.ts | 5 +- .../__tests__/agent-evals.controller.test.ts | 97 ++++++-- .../agent-evals/agent-eval-rating.service.ts | 125 +++++----- .../agent-evals/agent-eval-record-mappers.ts | 16 +- .../agent-evals-required-modules.ts | 11 +- .../agent-evals/agent-evals.controller.ts | 59 ++++- 8 files changed, 355 insertions(+), 185 deletions(-) diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-rating.service.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-rating.service.test.ts index 78d60d3c9df..2452dca3d4a 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-rating.service.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-rating.service.test.ts @@ -1,8 +1,6 @@ -import { AGENT_EVALS_FLAG, type CreateAgentEvalRatingPayload } from '@n8n/api-types'; -import type { Logger } from '@n8n/backend-common'; +import type { CreateAgentEvalRatingPayload } from '@n8n/api-types'; +import type { Logger, ModuleRegistry } from '@n8n/backend-common'; import type { - AgentEvalDataset, - AgentEvalDatasetRepository, AgentEvalRating, AgentEvalRatingRepository, AgentEvalResult, @@ -15,126 +13,150 @@ import type { Mocked } from 'vitest'; import { mock } from 'vitest-mock-extended'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; -import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import type { AgentRepository } from '@/modules/agents/repositories/agent.repository'; -import { userHasScopes } from '@/permissions.ee/check-access'; -import type { PostHogClient } from '@/posthog'; import { AgentEvalRatingService } from '../agent-eval-rating.service'; -// Stub the statically imported specifiers to keep the agents module graph out. +// Stub the statically imported specifier to keep the agents module graph out. vi.mock('@/modules/agents/repositories/agent.repository', () => ({ AgentRepository: class AgentRepository {}, })); -vi.mock('@/permissions.ee/check-access', () => ({ - userHasScopes: vi.fn().mockResolvedValue(true), -})); const user = mock({ id: 'user-1' }); const PROJECT_ID = 'project-1'; +const AGENT_ID = 'agent-1'; + +// Fixed so the mapper's ISO strings are assertable. +const RATED_AT = new Date('2026-07-31T12:00:00.000Z'); + +const makeRating = (overrides: Partial = {}) => + mock({ + id: 'rating-1', + resultId: 'res-1', + vote: 'up', + comment: null, + correction: null, + ratedById: 'user-1', + createdAt: RATED_AT, + updatedAt: RATED_AT, + ...overrides, + }); describe('AgentEvalRatingService', () => { let service: AgentEvalRatingService; let logger: Mocked; + let moduleRegistry: Mocked; let ratingRepository: Mocked; let resultRepository: Mocked; let runRepository: Mocked; - let datasetRepository: Mocked; let agentRepository: Mocked; - let postHogClient: Mocked; const result = mock({ id: 'res-1', runId: 'run-1', status: 'success' }); const run = mock({ id: 'run-1', datasetId: 'ds-1' }); - const dataset = mock({ id: 'ds-1', agentId: 'agent-1' }); beforeEach(() => { - vi.mocked(userHasScopes).mockResolvedValue(true); - logger = mock(); + moduleRegistry = mock(); ratingRepository = mock(); resultRepository = mock(); runRepository = mock(); - datasetRepository = mock(); agentRepository = mock(); - postHogClient = mock(); - postHogClient.getFeatureFlags.mockResolvedValue({ [AGENT_EVALS_FLAG]: true }); + moduleRegistry.isActive.mockReturnValue(true); resultRepository.findById.mockResolvedValue(result); - runRepository.findById.mockResolvedValue(run); - datasetRepository.findById.mockResolvedValue(dataset); + runRepository.findByIdAndAgentId.mockResolvedValue(run); agentRepository.existsByIdAndProjectId.mockResolvedValue(true); - ratingRepository.createRating.mockImplementation(async (attrs) => - mock({ id: 'rating-1', ...attrs }), - ); + ratingRepository.findByResultId.mockResolvedValue([]); + ratingRepository.findLatestByRunId.mockResolvedValue([]); + ratingRepository.createRating.mockImplementation(async (attrs) => makeRating(attrs)); service = new AgentEvalRatingService( logger, + moduleRegistry, ratingRepository, resultRepository, runRepository, - datasetRepository, agentRepository, - postHogClient, ); }); - describe('access control', () => { - it('rejects when the agent-evals flag is disabled (as not-found, leaking no flag state)', async () => { - postHogClient.getFeatureFlags.mockResolvedValue({ [AGENT_EVALS_FLAG]: false }); - - await expect( - service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'up' }), - ).rejects.toThrowError(NotFoundError); - expect(ratingRepository.createRating).not.toHaveBeenCalled(); - }); - - it('rejects a caller lacking the rating scope, without touching the eval tables', async () => { - vi.mocked(userHasScopes).mockResolvedValue(false); - - await expect( - service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'up' }), - ).rejects.toThrowError(ForbiddenError); - // Authorization runs before any lookup, so result ids can't be probed. - expect(resultRepository.findById).not.toHaveBeenCalled(); - }); - - // A project viewer holds execute but not update. - it('requires agent:execute to rate and agent:read to read', async () => { - await service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'up' }); - expect(vi.mocked(userHasScopes)).toHaveBeenCalledWith(user, ['agent:execute'], false, { - projectId: PROJECT_ID, - }); - - vi.mocked(userHasScopes).mockClear(); - - await service.listRatingsForResult(user, PROJECT_ID, 'res-1'); - expect(vi.mocked(userHasScopes)).toHaveBeenCalledWith(user, ['agent:read'], false, { - projectId: PROJECT_ID, - }); - }); - + /** + * The project scope and the rollout flag are the controller's to enforce, so + * what is asserted here is ownership: that no id from another agent or project + * resolves, and that a foreign one reads as missing rather than forbidden. + */ + describe('ownership scoping', () => { it('rejects an unknown result', async () => { resultRepository.findById.mockResolvedValue(null); await expect( - service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'up' }), + service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'up' }), ).rejects.toThrowError(NotFoundError); + expect(ratingRepository.createRating).not.toHaveBeenCalled(); }); - it('rejects a result whose agent lives in another project (as not-found)', async () => { + it('rejects an agent outside the path project, without touching the eval tables', async () => { agentRepository.existsByIdAndProjectId.mockResolvedValue(false); await expect( - service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'up' }), + service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'up' }), ).rejects.toThrowError(NotFoundError); + // Ownership resolves before any lookup, so result ids can't be probed. + expect(resultRepository.findById).not.toHaveBeenCalled(); + }); + + // The result exists and its project checks out, but its run belongs to a + // sibling agent — `@ProjectScope` cannot catch this, only the agent filter. + it.each([ + [ + 'rateResult', + async (svc: AgentEvalRatingService) => + await svc.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'up' }), + ], + [ + 'listRatingsForResult', + async (svc: AgentEvalRatingService) => + await svc.listRatingsForResult(AGENT_ID, PROJECT_ID, 'res-1'), + ], + [ + 'listLatestRatingsForRun', + async (svc: AgentEvalRatingService) => + await svc.listLatestRatingsForRun(AGENT_ID, PROJECT_ID, 'run-1'), + ], + ])('%s rejects an id belonging to another agent (as not-found)', async (_name, call) => { + runRepository.findByIdAndAgentId.mockResolvedValue(null); + + await expect(call(service)).rejects.toThrowError(NotFoundError); expect(ratingRepository.createRating).not.toHaveBeenCalled(); + expect(ratingRepository.findByResultId).not.toHaveBeenCalled(); + expect(ratingRepository.findLatestByRunId).not.toHaveBeenCalled(); + }); + + it('resolves runs through the path agent rather than a bare id', async () => { + await service.listLatestRatingsForRun(AGENT_ID, PROJECT_ID, 'run-1'); + + expect(runRepository.findByIdAndAgentId).toHaveBeenCalledWith('run-1', AGENT_ID); + }); + + // With `agents` off there is no agent to address, so the surface reads as + // unknown rather than misused — and the message still names the module, so a + // TypeORM missing-metadata error never reaches the caller. + it('reports an inactive dependency module as not-found, naming the module', async () => { + moduleRegistry.isActive.mockReturnValue(false); + + const rate = async () => + await service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'up' }); + + await expect(rate()).rejects.toThrowError(NotFoundError); + await expect(rate()).rejects.toThrow('require these modules to be active'); + expect(agentRepository.existsByIdAndProjectId).not.toHaveBeenCalled(); }); }); describe('rateResult', () => { it('persists an upvote with no correction, attributed to the rater', async () => { - await service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'up' }); + await service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'up' }); expect(ratingRepository.createRating).toHaveBeenCalledWith({ resultId: 'res-1', @@ -146,7 +168,7 @@ describe('AgentEvalRatingService', () => { }); it('persists a downvote with a comment and the edited answer', async () => { - await service.rateResult(user, PROJECT_ID, 'res-1', { + await service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'down', comment: 'missed the refund policy', correction: { finalText: 'Refunds are processed within 14 days.' }, @@ -161,13 +183,31 @@ describe('AgentEvalRatingService', () => { }); }); + // Field-by-field, so a new entity column can't leak onto the wire. + it('returns the persisted rating as a wire record with ISO timestamps', async () => { + const correction = { finalText: 'the expected answer' }; + + await expect( + service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'down', correction }), + ).resolves.toEqual({ + id: 'rating-1', + resultId: 'res-1', + vote: 'down', + comment: null, + correction, + ratedById: 'user-1', + createdAt: RATED_AT.toISOString(), + updatedAt: RATED_AT.toISOString(), + }); + }); + it.each(['new', 'running'] as const)('refuses to rate a %s case', async (status) => { resultRepository.findById.mockResolvedValue( mock({ id: 'res-1', runId: 'run-1', status }), ); await expect( - service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'up' }), + service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'up' }), ).rejects.toThrowError(BadRequestError); expect(ratingRepository.createRating).not.toHaveBeenCalled(); }); @@ -179,7 +219,7 @@ describe('AgentEvalRatingService', () => { mock({ id: 'res-1', runId: 'run-1', status }), ); - await service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'down' }); + await service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'down' }); expect(ratingRepository.createRating).toHaveBeenCalledWith( expect.objectContaining({ vote: 'down' }), @@ -188,8 +228,8 @@ describe('AgentEvalRatingService', () => { ); it('appends on re-vote rather than overwriting the earlier rating', async () => { - await service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'down' }); - await service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'up' }); + await service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'down' }); + await service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'up' }); expect(ratingRepository.createRating).toHaveBeenCalledTimes(2); expect(ratingRepository.createRating).toHaveBeenLastCalledWith( @@ -199,7 +239,7 @@ describe('AgentEvalRatingService', () => { it('rejects an over-long comment', async () => { await expect( - service.rateResult(user, PROJECT_ID, 'res-1', { + service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'down', comment: 'x'.repeat(2_001), }), @@ -209,7 +249,7 @@ describe('AgentEvalRatingService', () => { it('rejects an over-long corrected answer', async () => { await expect( - service.rateResult(user, PROJECT_ID, 'res-1', { + service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'down', correction: { finalText: 'x'.repeat(20_001) }, }), @@ -226,12 +266,13 @@ describe('AgentEvalRatingService', () => { }; await expect( - service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'down', correction }), + service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'down', correction }), ).rejects.toThrowError(BadRequestError); }); - // The service is the enforcement point until the REST layer's DTO validation - // lands, and a correction nothing can read defeats the point of capturing it. + // The route's DTO now rejects these shapes first, but the length caps live + // nowhere else and a correction nothing can read defeats the point of + // capturing it — so the service keeps checking rather than trusting a caller. it.each([ { label: 'an absent finalText', correction: {} }, { label: 'the wrong key', correction: { output: 'the expected answer' } }, @@ -243,16 +284,16 @@ describe('AgentEvalRatingService', () => { // payload type now refuses them at compile time. const payload = { vote: 'down', correction } as CreateAgentEvalRatingPayload; - await expect(service.rateResult(user, PROJECT_ID, 'res-1', payload)).rejects.toThrowError( - BadRequestError, - ); + await expect( + service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', payload), + ).rejects.toThrowError(BadRequestError); expect(ratingRepository.createRating).not.toHaveBeenCalled(); }); it('keeps extra correction keys alongside the edited answer', async () => { const correction = { finalText: 'the expected answer', fields: { tone: 'formal' } }; - await service.rateResult(user, PROJECT_ID, 'res-1', { vote: 'down', correction }); + await service.rateResult(user, AGENT_ID, PROJECT_ID, 'res-1', { vote: 'down', correction }); expect(ratingRepository.createRating).toHaveBeenCalledWith( expect.objectContaining({ correction }), @@ -261,33 +302,24 @@ describe('AgentEvalRatingService', () => { }); describe('listRatingsForResult', () => { - it('returns the result history', async () => { - const ratings = [mock({ id: 'rating-2' })]; - ratingRepository.findByResultId.mockResolvedValue(ratings); + it('returns the result history as wire records', async () => { + ratingRepository.findByResultId.mockResolvedValue([makeRating({ id: 'rating-2' })]); - await expect(service.listRatingsForResult(user, PROJECT_ID, 'res-1')).resolves.toBe(ratings); + await expect(service.listRatingsForResult(AGENT_ID, PROJECT_ID, 'res-1')).resolves.toEqual([ + expect.objectContaining({ id: 'rating-2', resultId: 'res-1' }), + ]); expect(ratingRepository.findByResultId).toHaveBeenCalledWith('res-1'); }); }); describe('listLatestRatingsForRun', () => { it('returns the newest rating per result for the run', async () => { - const ratings = [mock({ id: 'rating-3' })]; - ratingRepository.findLatestByRunId.mockResolvedValue(ratings); + ratingRepository.findLatestByRunId.mockResolvedValue([makeRating({ id: 'rating-3' })]); - await expect(service.listLatestRatingsForRun(user, PROJECT_ID, 'run-1')).resolves.toBe( - ratings, + await expect(service.listLatestRatingsForRun(AGENT_ID, PROJECT_ID, 'run-1')).resolves.toEqual( + [expect.objectContaining({ id: 'rating-3' })], ); expect(ratingRepository.findLatestByRunId).toHaveBeenCalledWith('run-1'); }); - - it('rejects a run from another project (as not-found)', async () => { - agentRepository.existsByIdAndProjectId.mockResolvedValue(false); - - await expect(service.listLatestRatingsForRun(user, PROJECT_ID, 'run-1')).rejects.toThrowError( - NotFoundError, - ); - expect(ratingRepository.findLatestByRunId).not.toHaveBeenCalled(); - }); }); }); diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.test.ts index f289c0286b1..7ae1f3c3375 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.test.ts @@ -207,6 +207,9 @@ describe('AgentEvalRunnerService', () => { await expect(service.startRun('ds-1', 'proj-1', user)).rejects.toThrow( 'require these modules to be active: data-table', ); + // Not-found, so the whole agent-eval surface reads as absent when a module + // it depends on is off, rather than half-present. + await expect(service.startRun('ds-1', 'proj-1', user)).rejects.toThrow(NotFoundError); expect(runRepository.createRun).not.toHaveBeenCalled(); }); 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 eba86ddb061..4e44b88e285 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 @@ -155,11 +155,14 @@ describe('AgentEvalService', () => { // The agent lookup below reads a module entity, so the dependency check has // to land before it — otherwise TypeORM raises missing metadata first. + // Not-found, not bad-request: with `agents` off there is no agent to address, + // matching the 404 that module's own unregistered routes already give. it.each(callsRequiringAnAgent)( - '%s reports the inactive module instead of querying the agent', + '%s reports the inactive module as not-found instead of querying the agent', async (_, call) => { moduleRegistry.isActive.mockImplementation((name) => name !== 'agents'); + await expect(call()).rejects.toThrow(NotFoundError); await expect(call()).rejects.toThrow('Agent evals require these modules to be active'); expect(agentRepository.findByIdAndProjectId).not.toHaveBeenCalled(); }, 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 278bbb182a7..ec32e1680a7 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 @@ -6,11 +6,15 @@ import { mock, type MockProxy } from 'vitest-mock-extended'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; +import type { AgentEvalRatingService } from '../agent-eval-rating.service'; import type { AgentEvalService } from '../agent-eval.service'; import type { AgentEvalsFlagGate } from '../agent-evals-flag-gate'; import { AgentEvalsController } from '../agent-evals.controller'; vi.mock('../agent-eval.service', () => ({ AgentEvalService: class AgentEvalService {} })); +vi.mock('../agent-eval-rating.service', () => ({ + AgentEvalRatingService: class AgentEvalRatingService {}, +})); vi.mock('../agent-evals-flag-gate', () => ({ AgentEvalsFlagGate: class AgentEvalsFlagGate {} })); const PROJECT_ID = 'proj-1'; @@ -20,6 +24,7 @@ describe('AgentEvalsController', () => { const user = mock({ id: 'user-1' }); let service: MockProxy; + let ratingService: MockProxy; let flagGate: MockProxy; let controller: AgentEvalsController; @@ -33,12 +38,14 @@ describe('AgentEvalsController', () => { const agentReq = () => makeReq({ projectId: PROJECT_ID, agentId: AGENT_ID }); const datasetReq = () => makeReq({ projectId: PROJECT_ID, agentId: AGENT_ID, datasetId: 'ds-1' }); const runReq = () => makeReq({ projectId: PROJECT_ID, agentId: AGENT_ID, runId: 'run-1' }); + const resultReq = () => makeReq({ projectId: PROJECT_ID, agentId: AGENT_ID, resultId: 'res-1' }); beforeEach(() => { service = mock(); + ratingService = mock(); flagGate = mock(); flagGate.assertEnabled.mockResolvedValue(undefined); - controller = new AgentEvalsController(service, flagGate); + controller = new AgentEvalsController(service, ratingService, flagGate); }); /** @@ -65,24 +72,38 @@ describe('AgentEvalsController', () => { expect(route.accessScope?.scope.startsWith('agent:')).toBe(true); }); - it.each([ - // Reads stay on agent:read; anything that writes eval config — including - // generation, which spends the builder's model credits — needs - // agent:update; starting/cancelling a run is agent:execute. - ['listDatasets', 'agent:read'], - ['getDataset', 'agent:read'], - ['listRuns', 'agent:read'], - ['getRun', 'agent:read'], - ['getRunSummary', 'agent:read'], - ['createDataset', 'agent:update'], - ['updateDataset', 'agent:update'], - ['deleteDataset', 'agent:update'], - ['generateDraftCases', 'agent:update'], - ['startRun', 'agent:execute'], - ['cancelRun', 'agent:execute'], - ])('%s uses %s', (handlerName, scope) => { + // 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. + const expectedScopes = { + listDatasets: 'agent:read', + getDataset: 'agent:read', + listRuns: 'agent:read', + getRun: 'agent:read', + getRunSummary: 'agent:read', + listRatingsForResult: 'agent:read', + listLatestRatingsForRun: 'agent:read', + createDataset: 'agent:update', + updateDataset: 'agent:update', + deleteDataset: 'agent:update', + generateDraftCases: 'agent:update', + rateResult: 'agent:update', + startRun: 'agent:execute', + cancelRun: 'agent:execute', + } as const; + + it.each(Object.entries(expectedScopes))('%s uses %s', (handlerName, scope) => { expect(metadata.routes.get(handlerName)?.accessScope?.scope).toBe(scope); }); + + // The controller is the only enforcement point for the scopes, so a route + // added without a deliberate entry above is a scope nobody chose. + it('pins an expected scope for every registered handler', () => { + expect(routeCases.map(({ handlerName }) => handlerName).sort()).toEqual( + Object.keys(expectedScopes).sort(), + ); + }); }); // Flag-off must look like an unknown endpoint on every route, and must not @@ -120,6 +141,12 @@ describe('AgentEvalsController', () => { ['getRun', async () => await controller.getRun(runReq())], ['getRunSummary', async () => await controller.getRunSummary(runReq())], ['cancelRun', async () => await controller.cancelRun(runReq())], + [ + 'rateResult', + async () => await controller.rateResult(resultReq(), undefined, { vote: 'up' }), + ], + ['listRatingsForResult', async () => await controller.listRatingsForResult(resultReq())], + ['listLatestRatingsForRun', async () => await controller.listLatestRatingsForRun(runReq())], ]; it.each(calls)('%s 404s when the flag is off for the user', async (_name, call) => { @@ -212,5 +239,41 @@ describe('AgentEvalsController', () => { expect(service.cancelRun).toHaveBeenCalledWith(AGENT_ID, PROJECT_ID, 'run-1'); }); + + // The rating service resolves the result through the path agent, so the agent + // has to reach it — a result id alone would be unscoped. + it('rates a result scoped to the path agent, attributed to the caller', async () => { + const payload = { vote: 'down' as const, correction: { finalText: 'the right answer' } }; + + await controller.rateResult(resultReq(), undefined, payload); + + expect(ratingService.rateResult).toHaveBeenCalledWith( + user, + AGENT_ID, + PROJECT_ID, + 'res-1', + payload, + ); + }); + + it('reads a result rating history scoped to the path agent', async () => { + await controller.listRatingsForResult(resultReq()); + + expect(ratingService.listRatingsForResult).toHaveBeenCalledWith( + AGENT_ID, + PROJECT_ID, + 'res-1', + ); + }); + + it('reads a run rating summary scoped to the path agent', async () => { + await controller.listLatestRatingsForRun(runReq()); + + expect(ratingService.listLatestRatingsForRun).toHaveBeenCalledWith( + AGENT_ID, + PROJECT_ID, + 'run-1', + ); + }); }); }); diff --git a/packages/cli/src/modules/agent-evals/agent-eval-rating.service.ts b/packages/cli/src/modules/agent-evals/agent-eval-rating.service.ts index e7e8d35cb09..54e7bb82936 100644 --- a/packages/cli/src/modules/agent-evals/agent-eval-rating.service.ts +++ b/packages/cli/src/modules/agent-evals/agent-eval-rating.service.ts @@ -1,21 +1,19 @@ -import { AGENT_EVALS_FLAG, type CreateAgentEvalRatingPayload } from '@n8n/api-types'; -import { Logger } from '@n8n/backend-common'; -import type { AgentEvalRating, AgentEvalResult, User } from '@n8n/db'; +import type { AgentEvalRatingRecord, CreateAgentEvalRatingPayload } from '@n8n/api-types'; +import { Logger, ModuleRegistry } from '@n8n/backend-common'; +import type { AgentEvalResult, User } from '@n8n/db'; import { - AgentEvalDatasetRepository, AgentEvalRatingRepository, AgentEvalResultRepository, AgentEvalRunRepository, } from '@n8n/db'; import { Service } from '@n8n/di'; -import type { Scope } from '@n8n/permissions'; import { BadRequestError } from '@/errors/response-errors/bad-request.error'; -import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { AgentRepository } from '@/modules/agents/repositories/agent.repository'; -import { userHasScopes } from '@/permissions.ee/check-access'; -import { PostHogClient } from '@/posthog'; + +import { toRatingRecord } from './agent-eval-record-mappers'; +import { assertRequiredModulesActive } from './agent-evals-required-modules'; // The body arrives straight from a request, so bound it before it hits the column. const MAX_COMMENT_CHARS = 2_000; @@ -25,33 +23,38 @@ const MAX_CORRECTION_CHARS = 32_000; /** * A human's 👍/👎 on an eval result, with an optional comment and correction. * Corrections stay on the rating row (never the dataset); ratings are append-only. + * + * **Every method is agent-scoped, and none of them authorizes.** As in the + * sibling `AgentEvalService`, the project scope and the rollout flag are the + * controller's to enforce — reach this service any other way and it checks + * neither. What it does own is ownership: `@ProjectScope` proves the caller may + * act on `:projectId`, not that the addressed agent lives there nor that a + * result/run id belongs to it, so each entry point resolves `(agentId, + * projectId)` and then reads through agent-filtered queries. A result on a + * sibling agent 404s like a missing one. */ @Service() export class AgentEvalRatingService { constructor( private readonly logger: Logger, + private readonly moduleRegistry: ModuleRegistry, private readonly ratingRepository: AgentEvalRatingRepository, private readonly resultRepository: AgentEvalResultRepository, private readonly runRepository: AgentEvalRunRepository, - private readonly datasetRepository: AgentEvalDatasetRepository, private readonly agentRepository: AgentRepository, - private readonly postHogClient: PostHogClient, ) {} - /** Authorization is enforced here too, so no caller can skip it. */ async rateResult( user: User, + agentId: string, projectId: string, resultId: string, payload: CreateAgentEvalRatingPayload, - ): Promise { - await this.assertFeatureEnabled(user); - // Execute, not update: a project viewer holds execute and can run the eval, and - // the reviewer of a result is often not its builder. - await this.assertProjectScopes(user, projectId, ['agent:execute']); + ): Promise { + await this.assertAgentInProject(agentId, projectId); assertPayloadWithinBounds(payload); - const result = await this.resolveResultInProject(projectId, resultId); + const result = await this.resolveResult(agentId, resultId); // A pending case has no output, so the vote would judge nothing. Errored and // cancelled stay rateable — "it failed" is a judgment. if (result.status === 'new' || result.status === 'running') { @@ -72,85 +75,77 @@ export class AgentEvalRatingService { hasCorrection: rating.correction !== null, }); - return rating; + return toRatingRecord(rating); } /** The per-case history, newest first. */ async listRatingsForResult( - user: User, + agentId: string, projectId: string, resultId: string, - ): Promise { - await this.assertFeatureEnabled(user); - await this.assertProjectScopes(user, projectId, ['agent:read']); + ): Promise { + await this.assertAgentInProject(agentId, projectId); - const result = await this.resolveResultInProject(projectId, resultId); + const result = await this.resolveResult(agentId, resultId); + const ratings = await this.ratingRepository.findByResultId(result.id); - return await this.ratingRepository.findByResultId(result.id); + return ratings.map(toRatingRecord); } /** What a reopened run renders, and the corrections calibration reads. */ async listLatestRatingsForRun( - user: User, + agentId: string, projectId: string, runId: string, - ): Promise { - await this.assertFeatureEnabled(user); - await this.assertProjectScopes(user, projectId, ['agent:read']); + ): Promise { + await this.assertAgentInProject(agentId, projectId); - await this.resolveRunInProject(projectId, runId); + await this.resolveRun(agentId, runId); + const ratings = await this.ratingRepository.findLatestByRunId(runId); - return await this.ratingRepository.findLatestByRunId(runId); + return ratings.map(toRatingRecord); } // ---- internals ---- /** - * Not-found rather than forbidden, so a flag-off instance leaks no flag state - * (matching the case-generation gate). + * Stops a caller with access to one project from addressing an agent in another, + * which `@ProjectScope` alone cannot. Existence only: `findByIdAndProjectId` + * would load `activeVersion` on every rating call. + * + * Every public method starts here, which makes it the one place to assert the + * modules this one depends on — before the agent lookup that would otherwise + * fail as a TypeORM missing-metadata error. */ - private async assertFeatureEnabled(user: User): Promise { - const flags = await this.postHogClient.getFeatureFlags(user); - if (flags?.[AGENT_EVALS_FLAG] !== true) { - throw new NotFoundError('Not found'); - } + private async assertAgentInProject(agentId: string, projectId: string): Promise { + assertRequiredModulesActive(this.moduleRegistry); + const inProject = await this.agentRepository.existsByIdAndProjectId(agentId, projectId); + if (!inProject) throw new NotFoundError(`Agent ${agentId} not found.`); } - /** Runs before any lookup, so unauthorized callers can't probe for ids. */ - private async assertProjectScopes(user: User, projectId: string, scopes: Scope[]): Promise { - if (!(await userHasScopes(user, scopes, false, { projectId }))) { - throw new ForbiddenError('You do not have permission to review agent evals in this project.'); - } - } + /** + * A result is owned through its run, so this resolves the run agent-filtered + * rather than trusting a bare result id. A result on a sibling agent reads as + * missing, not forbidden, so its existence doesn't leak. + */ + private async resolveResult(agentId: string, resultId: string): Promise { + const notFound = () => new NotFoundError(`Agent eval result ${resultId} not found.`); - private async resolveResultInProject( - projectId: string, - resultId: string, - ): Promise { const result = await this.resultRepository.findById(resultId); - if (!result) throw new NotFoundError(`Agent eval result ${resultId} not found.`); + if (!result) throw notFound(); - await this.resolveRunInProject(projectId, result.runId); + // Reported against the result, not the run: the run id is an internal detail + // the caller never named, so surfacing it would leak more than it explains. + const run = await this.runRepository.findByIdAndAgentId(result.runId, agentId); + if (!run) throw notFound(); return result; } - /** - * Walks run → dataset → agent to confirm project ownership. A run elsewhere reads - * as missing, not forbidden, so its existence doesn't leak. - */ - private async resolveRunInProject(projectId: string, runId: string): Promise { - const notFound = () => new NotFoundError(`Agent eval run ${runId} not found.`); - - const run = await this.runRepository.findById(runId); - if (!run) throw notFound(); - - const dataset = await this.datasetRepository.findById(run.datasetId); - if (!dataset) throw notFound(); - - // Existence only: `findByIdAndProjectId` would load `activeVersion` per rating. - const inProject = await this.agentRepository.existsByIdAndProjectId(dataset.agentId, projectId); - if (!inProject) throw notFound(); + /** The run's own agent scoping — `findByIdAndAgentId` walks run → dataset → agent. */ + private async resolveRun(agentId: string, runId: string): Promise { + const run = await this.runRepository.findByIdAndAgentId(runId, agentId); + if (!run) throw new NotFoundError(`Agent eval run ${runId} not found.`); } } diff --git a/packages/cli/src/modules/agent-evals/agent-eval-record-mappers.ts b/packages/cli/src/modules/agent-evals/agent-eval-record-mappers.ts index 48db48a79a3..c7a0770014e 100644 --- a/packages/cli/src/modules/agent-evals/agent-eval-record-mappers.ts +++ b/packages/cli/src/modules/agent-evals/agent-eval-record-mappers.ts @@ -1,12 +1,13 @@ import type { AgentEvalDatasetRecord, + AgentEvalRatingRecord, AgentEvalResultRecord, AgentEvalRunRecord, DataTableDatasetRef, DatasetRef, GoogleSheetsDatasetRef, } from '@n8n/api-types'; -import type { AgentEvalDataset, AgentEvalResult, AgentEvalRun } from '@n8n/db'; +import type { AgentEvalDataset, AgentEvalRating, AgentEvalResult, AgentEvalRun } from '@n8n/db'; import { UnexpectedError } from 'n8n-workflow'; /** @@ -95,3 +96,16 @@ export function toResultRecord(result: AgentEvalResult): AgentEvalResultRecord { updatedAt: result.updatedAt.toISOString(), }; } + +export function toRatingRecord(rating: AgentEvalRating): AgentEvalRatingRecord { + return { + id: rating.id, + resultId: rating.resultId, + vote: rating.vote, + comment: rating.comment, + correction: rating.correction, + ratedById: rating.ratedById, + createdAt: rating.createdAt.toISOString(), + updatedAt: rating.updatedAt.toISOString(), + }; +} diff --git a/packages/cli/src/modules/agent-evals/agent-evals-required-modules.ts b/packages/cli/src/modules/agent-evals/agent-evals-required-modules.ts index c793b26f38d..c1541eec77d 100644 --- a/packages/cli/src/modules/agent-evals/agent-evals-required-modules.ts +++ b/packages/cli/src/modules/agent-evals/agent-evals-required-modules.ts @@ -1,6 +1,6 @@ import type { ModuleRegistry } from '@n8n/backend-common'; -import { BadRequestError } from '@/errors/response-errors/bad-request.error'; +import { NotFoundError } from '@/errors/response-errors/not-found.error'; const REQUIRED_MODULES = ['agents', 'data-table'] as const; @@ -8,11 +8,18 @@ const REQUIRED_MODULES = ['agents', 'data-table'] as const; * Agent evals read agents and Data Tables, whose entities only exist while those * modules are active. Callers must assert before their first repository touch, * or TypeORM raises a missing-metadata error instead of saying what is off. + * + * Not-found rather than bad-request: with `agents` off there is no agent to + * address, and that module's own controller isn't even registered — so its routes + * already 404. A nested agent surface answering 400 for the same instance state + * would be the odd one out. The caller sent nothing wrong; the resource is + * genuinely absent. The message still names the inactive module, so an operator + * reading the response body gets the same diagnostic as before. */ export function assertRequiredModulesActive(moduleRegistry: ModuleRegistry): void { const inactive = REQUIRED_MODULES.filter((name) => !moduleRegistry.isActive(name)); if (inactive.length > 0) { - throw new BadRequestError( + throw new NotFoundError( `Agent evals require these modules to be active: ${inactive.join(', ')}.`, ); } 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 dab14b1d5ec..4c2f04da5bf 100644 --- a/packages/cli/src/modules/agent-evals/agent-evals.controller.ts +++ b/packages/cli/src/modules/agent-evals/agent-evals.controller.ts @@ -1,9 +1,11 @@ import { + CreateAgentEvalRatingDto, CreateAgentEvalRunDto, GenerateDraftCasesOptionsDto, UpdateAgentEvalDatasetDto, createAgentEvalDatasetSchema, type AgentEvalDatasetRecord, + type AgentEvalRatingRecord, type AgentEvalRunDetail, type AgentEvalRunRecord, type AgentEvalRunSummary, @@ -14,26 +16,33 @@ import { Body, Delete, Get, Patch, Post, ProjectScope, RestController } from '@n import { BadRequestError } from '@/errors/response-errors/bad-request.error'; +import { AgentEvalRatingService } from './agent-eval-rating.service'; import { AgentEvalService } from './agent-eval.service'; import { AgentEvalsFlagGate } from './agent-evals-flag-gate'; type AgentParam = { projectId: string; agentId: string }; type DatasetParam = AgentParam & { datasetId: string }; type RunParam = AgentParam & { runId: string }; +type ResultParam = AgentParam & { resultId: string }; /** - * REST surface for agent evals: generation, datasets, runs and per-case results. - * Nested under the agent so `@ProjectScope` rejects before the handler runs. + * REST surface for agent evals: generation, datasets, runs, per-case results and + * the human ratings of them. Nested under the agent so `@ProjectScope` rejects + * before the handler runs. + * + * **This is the single enforcement point** for both the project scope and the + * `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 * eval-config writes — including generation, which spends the builder's model * credits and so must stay closed to viewers (they hold `agent:execute`). - * Ratings ship with the service that persists them. */ @RestController('/projects/:projectId/agents/v2') export class AgentEvalsController { constructor( private readonly service: AgentEvalService, + private readonly ratingService: AgentEvalRatingService, private readonly flagGate: AgentEvalsFlagGate, ) {} @@ -153,4 +162,48 @@ export class AgentEvalsController { const { agentId, projectId, runId } = req.params; return await this.service.cancelRun(agentId, projectId, runId); } + + // ---- ratings ---- + + /** + * `agent:update`, not `agent:execute`: the chat-user role holds execute and + * nothing else, and a rating — especially a correction, which seeds later judge + * calibration — is eval config a chat-only member has no business writing. + */ + @Post('/:agentId/evals/results/:resultId/ratings') + @ProjectScope('agent:update') + async rateResult( + req: AuthenticatedRequest, + _res: unknown, + @Body payload: CreateAgentEvalRatingDto, + ): Promise { + await this.flagGate.assertEnabled(req.user); + const { agentId, projectId, resultId } = req.params; + return await this.ratingService.rateResult(req.user, agentId, projectId, resultId, payload); + } + + /** Ratings are append-only, so this is the case's full history, newest first. */ + @Get('/:agentId/evals/results/:resultId/ratings') + @ProjectScope('agent:read') + async listRatingsForResult( + req: AuthenticatedRequest, + ): Promise { + await this.flagGate.assertEnabled(req.user); + const { agentId, projectId, resultId } = req.params; + return await this.ratingService.listRatingsForResult(agentId, projectId, resultId); + } + + /** + * Newest rating per rated case — what reopening a run renders. Not every rating + * in the run: superseded votes stay on record but are the per-case route's job. + */ + @Get('/:agentId/evals/runs/:runId/ratings') + @ProjectScope('agent:read') + async listLatestRatingsForRun( + req: AuthenticatedRequest, + ): Promise { + await this.flagGate.assertEnabled(req.user); + const { agentId, projectId, runId } = req.params; + return await this.ratingService.listLatestRatingsForRun(agentId, projectId, runId); + } }