diff --git a/api/server/routes/__tests__/messages-feedback.spec.js b/api/server/routes/__tests__/messages-feedback.spec.js new file mode 100644 index 0000000000..2669ef67d0 --- /dev/null +++ b/api/server/routes/__tests__/messages-feedback.spec.js @@ -0,0 +1,154 @@ +const express = require('express'); +const request = require('supertest'); + +jest.mock('@librechat/agents', () => ({ + sleep: jest.fn(), +})); + +jest.mock('@librechat/api', () => ({ + unescapeLaTeX: jest.fn((value) => value), + countTokens: jest.fn().mockResolvedValue(10), + sendFeedbackScore: jest.fn().mockResolvedValue(undefined), + traceIdForMessage: jest.fn((messageId) => `trace-${messageId}`), +})); + +jest.mock('@librechat/data-schemas', () => ({ + ...jest.requireActual('@librechat/data-schemas'), + logger: { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }, +})); + +jest.mock('librechat-data-provider', () => ({ + ...jest.requireActual('librechat-data-provider'), +})); + +jest.mock('~/models', () => ({ + saveConvo: jest.fn(), + getMessage: jest.fn(), + saveMessage: jest.fn(), + getMessages: jest.fn(), + updateMessage: jest.fn(), + deleteMessages: jest.fn(), + getConvosQueried: jest.fn(), + searchMessages: jest.fn(), + getMessagesByCursor: jest.fn(), +})); + +jest.mock('~/server/services/Artifacts/update', () => ({ + findAllArtifacts: jest.fn(), + replaceArtifactContent: jest.fn(), +})); + +jest.mock('~/server/middleware', () => ({ + requireJwtAuth: (req, res, next) => next(), + validateMessageReq: (req, res, next) => next(), + configMiddleware: (req, res, next) => next(), + sendValidationResponse: jest.fn(), + prepareMessageRequestValidation: jest.fn(), +})); + +jest.mock('~/db/models', () => ({ + Message: { + findOne: jest.fn(), + find: jest.fn(), + meiliSearch: jest.fn(), + }, +})); + +describe('PUT /:conversationId/:messageId/feedback', () => { + let app; + const { sendFeedbackScore } = require('@librechat/api'); + const { updateMessage } = require('~/models'); + + beforeAll(() => { + const messagesRouter = require('../messages'); + + app = express(); + app.use(express.json()); + app.use((req, res, next) => { + req.user = { id: 'user-1' }; + next(); + }); + app.use('/api/messages', messagesRouter); + }); + + beforeEach(() => { + jest.clearAllMocks(); + updateMessage.mockImplementation((userId, { messageId, feedback }) => + Promise.resolve({ + messageId, + conversationId: 'conversation-1', + endpoint: 'openAI', + feedback, + }), + ); + }); + + it('persists validated feedback and removes unknown fields', async () => { + const feedback = { + rating: 'thumbsDown', + tag: 'inaccurate', + text: 'The answer is incorrect', + ignored: 'value', + }; + + const response = await request(app) + .put('/api/messages/conversation-1/message-1/feedback') + .send({ feedback }); + + expect(response.status).toBe(200); + expect(updateMessage).toHaveBeenCalledWith( + 'user-1', + { + messageId: 'message-1', + feedback: { + rating: 'thumbsDown', + tag: 'inaccurate', + text: 'The answer is incorrect', + }, + }, + { context: 'updateFeedback' }, + ); + expect(sendFeedbackScore).toHaveBeenCalledWith( + expect.objectContaining({ + feedback: { + rating: 'thumbsDown', + tag: 'inaccurate', + text: 'The answer is incorrect', + }, + }), + ); + }); + + it.each([ + ['an object tag', { rating: 'thumbsDown', tag: { key: 'inaccurate' } }], + ['a tag for the opposite rating', { rating: 'thumbsUp', tag: 'inaccurate' }], + ['oversized text', { rating: 'thumbsDown', tag: 'other', text: 'x'.repeat(1025) }], + ])('rejects %s before persistence or export', async (_name, feedback) => { + const response = await request(app) + .put('/api/messages/conversation-1/message-1/feedback') + .send({ feedback }); + + expect(response.status).toBe(400); + expect(response.body).toEqual({ error: 'Invalid feedback' }); + expect(updateMessage).not.toHaveBeenCalled(); + expect(sendFeedbackScore).not.toHaveBeenCalled(); + }); + + it('preserves clearing feedback', async () => { + const response = await request(app) + .put('/api/messages/conversation-1/message-1/feedback') + .send({}); + + expect(response.status).toBe(200); + expect(updateMessage).toHaveBeenCalledWith( + 'user-1', + { messageId: 'message-1', feedback: null }, + { context: 'updateFeedback' }, + ); + }); +}); diff --git a/api/server/routes/messages.js b/api/server/routes/messages.js index f541202989..bb1e64ce57 100644 --- a/api/server/routes/messages.js +++ b/api/server/routes/messages.js @@ -1,7 +1,7 @@ const express = require('express'); const { v4: uuidv4 } = require('uuid'); const { logger } = require('@librechat/data-schemas'); -const { ContentTypes, isAssistantsEndpoint } = require('librechat-data-provider'); +const { ContentTypes, feedbackSchema, isAssistantsEndpoint } = require('librechat-data-provider'); const { unescapeLaTeX, countTokens, @@ -431,12 +431,17 @@ router.put( try { const { conversationId, messageId } = req.params; const { feedback } = req.body; + const feedbackResult = feedback == null ? null : feedbackSchema.safeParse(feedback); + + if (feedbackResult && !feedbackResult.success) { + return res.status(400).json({ error: 'Invalid feedback' }); + } const updatedMessage = await db.updateMessage( req?.user?.id, { messageId, - feedback: feedback || null, + feedback: feedbackResult?.data ?? null, }, { context: 'updateFeedback' }, ); diff --git a/packages/data-provider/src/feedback.spec.ts b/packages/data-provider/src/feedback.spec.ts new file mode 100644 index 0000000000..b2abb6f22f --- /dev/null +++ b/packages/data-provider/src/feedback.spec.ts @@ -0,0 +1,13 @@ +import { FEEDBACK_TAGS, feedbackSchema } from './feedback'; + +describe('feedbackSchema', () => { + it.each(FEEDBACK_TAGS)('accepts $key with its $direction rating', ({ direction, key }) => { + expect(feedbackSchema.safeParse({ rating: direction, tag: key }).success).toBe(true); + }); + + it.each(FEEDBACK_TAGS)('rejects $key with the opposite rating', ({ direction, key }) => { + const oppositeRating = direction === 'thumbsUp' ? 'thumbsDown' : 'thumbsUp'; + + expect(feedbackSchema.safeParse({ rating: oppositeRating, tag: key }).success).toBe(false); + }); +}); diff --git a/packages/data-provider/src/feedback.ts b/packages/data-provider/src/feedback.ts index 206c55ac66..0bb574bec9 100644 --- a/packages/data-provider/src/feedback.ts +++ b/packages/data-provider/src/feedback.ts @@ -107,11 +107,22 @@ export function getTagsForRating(rating: TFeedbackRating): TFeedbackTag[] { export const feedbackTagKeySchema = z.enum(FEEDBACK_REASON_KEYS); export const feedbackRatingSchema = z.enum(FEEDBACK_RATINGS); -export const feedbackSchema = z.object({ - rating: feedbackRatingSchema, - tag: feedbackTagKeySchema, - text: z.string().max(1024).optional(), -}); +export const feedbackSchema = z + .object({ + rating: feedbackRatingSchema, + tag: feedbackTagKeySchema, + text: z.string().max(1024).optional(), + }) + .refine( + ({ rating, tag }) => + FEEDBACK_TAGS.some( + (feedbackTag) => feedbackTag.key === tag && feedbackTag.direction === rating, + ), + { + message: 'Feedback tag does not match rating', + path: ['tag'], + }, + ); export type TMinimalFeedback = z.infer;