🥞 refactor: Duplicate Agent Versions as Informational Instead of Errors (#8881)

* Fix error when updating an agent with no changes

* Add tests

* Revert translation file changes
This commit is contained in:
Sebastien Bruel 2025-08-07 15:12:05 +09:00 committed by GitHub
parent 1092392ed8
commit 0b071c06f6
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 474 additions and 120 deletions

View file

@ -316,17 +316,10 @@ const updateAgent = async (searchParameter, updateData, options = {}) => {
if (shouldCreateVersion) {
const duplicateVersion = isDuplicateVersion(updateData, versionData, versions, actionsHash);
if (duplicateVersion && !forceVersion) {
const error = new Error(
'Duplicate version: This would create a version identical to an existing one',
);
error.statusCode = 409;
error.details = {
duplicateVersion,
versionIndex: versions.findIndex(
(v) => JSON.stringify(duplicateVersion) === JSON.stringify(v),
),
};
throw error;
// No changes detected, return the current agent without creating a new version
const agentObj = currentAgent.toObject();
agentObj.version = versions.length;
return agentObj;
}
}

View file

@ -879,45 +879,31 @@ describe('models/Agent', () => {
expect(emptyParamsAgent.model_parameters).toEqual({});
});
test('should detect duplicate versions and reject updates', async () => {
const originalConsoleError = console.error;
console.error = jest.fn();
test('should not create new version for duplicate updates', async () => {
const authorId = new mongoose.Types.ObjectId();
const testCases = generateVersionTestCases();
try {
const authorId = new mongoose.Types.ObjectId();
const testCases = generateVersionTestCases();
for (const testCase of testCases) {
const testAgentId = `agent_${uuidv4()}`;
for (const testCase of testCases) {
const testAgentId = `agent_${uuidv4()}`;
await createAgent({
id: testAgentId,
provider: 'test',
model: 'test-model',
author: authorId,
...testCase.initial,
});
await createAgent({
id: testAgentId,
provider: 'test',
model: 'test-model',
author: authorId,
...testCase.initial,
});
const updatedAgent = await updateAgent({ id: testAgentId }, testCase.update);
expect(updatedAgent.versions).toHaveLength(2); // No new version created
await updateAgent({ id: testAgentId }, testCase.update);
// Update with duplicate data should succeed but not create a new version
const duplicateUpdate = await updateAgent({ id: testAgentId }, testCase.duplicate);
let error;
try {
await updateAgent({ id: testAgentId }, testCase.duplicate);
} catch (e) {
error = e;
}
expect(duplicateUpdate.versions).toHaveLength(2); // No new version created
expect(error).toBeDefined();
expect(error.message).toContain('Duplicate version');
expect(error.statusCode).toBe(409);
expect(error.details).toBeDefined();
expect(error.details.duplicateVersion).toBeDefined();
const agent = await getAgent({ id: testAgentId });
expect(agent.versions).toHaveLength(2);
}
} finally {
console.error = originalConsoleError;
const agent = await getAgent({ id: testAgentId });
expect(agent.versions).toHaveLength(2);
}
});
@ -1093,20 +1079,13 @@ describe('models/Agent', () => {
expect(secondUpdate.versions).toHaveLength(3);
// Update without forceVersion and no changes should not create a version
let error;
try {
await updateAgent(
{ id: agentId },
{ tools: ['listEvents_action_test.com', 'createEvent_action_test.com'] },
{ updatingUserId: authorId.toString(), forceVersion: false },
);
} catch (e) {
error = e;
}
const duplicateUpdate = await updateAgent(
{ id: agentId },
{ tools: ['listEvents_action_test.com', 'createEvent_action_test.com'] },
{ updatingUserId: authorId.toString(), forceVersion: false },
);
expect(error).toBeDefined();
expect(error.message).toContain('Duplicate version');
expect(error.statusCode).toBe(409);
expect(duplicateUpdate.versions).toHaveLength(3); // No new version created
});
test('should handle isDuplicateVersion with arrays containing null/undefined values', async () => {
@ -2400,11 +2379,18 @@ describe('models/Agent', () => {
agent_ids: ['agent1', 'agent2'],
});
await updateAgent({ id: agentId }, { agent_ids: ['agent1', 'agent2', 'agent3'] });
const updatedAgent = await updateAgent(
{ id: agentId },
{ agent_ids: ['agent1', 'agent2', 'agent3'] },
);
expect(updatedAgent.versions).toHaveLength(2);
await expect(
updateAgent({ id: agentId }, { agent_ids: ['agent1', 'agent2', 'agent3'] }),
).rejects.toThrow('Duplicate version');
// Update with same agent_ids should succeed but not create a new version
const duplicateUpdate = await updateAgent(
{ id: agentId },
{ agent_ids: ['agent1', 'agent2', 'agent3'] },
);
expect(duplicateUpdate.versions).toHaveLength(2); // No new version created
});
test('should handle agent_ids field alongside other fields', async () => {
@ -2543,9 +2529,10 @@ describe('models/Agent', () => {
expect(updated.versions).toHaveLength(2);
expect(updated.agent_ids).toEqual([]);
await expect(updateAgent({ id: agentId }, { agent_ids: [] })).rejects.toThrow(
'Duplicate version',
);
// Update with same empty agent_ids should succeed but not create a new version
const duplicateUpdate = await updateAgent({ id: agentId }, { agent_ids: [] });
expect(duplicateUpdate.versions).toHaveLength(2); // No new version created
expect(duplicateUpdate.agent_ids).toEqual([]);
});
test('should handle agent without agent_ids field', async () => {

View file

@ -194,6 +194,9 @@ const updateAgentHandler = async (req, res) => {
});
}
// Add version count to the response
updatedAgent.version = updatedAgent.versions ? updatedAgent.versions.length : 0;
if (updatedAgent.author) {
updatedAgent.author = updatedAgent.author.toString();
}

View file

@ -498,6 +498,28 @@ describe('Agent Controllers - Mass Assignment Protection', () => {
expect(mockRes.json).toHaveBeenCalledWith({ error: 'Agent not found' });
});
test('should include version field in update response', async () => {
mockReq.user.id = existingAgentAuthorId.toString();
mockReq.params.id = existingAgentId;
mockReq.body = {
name: 'Updated with Version Check',
};
await updateAgentHandler(mockReq, mockRes);
expect(mockRes.json).toHaveBeenCalled();
const updatedAgent = mockRes.json.mock.calls[0][0];
// Verify version field is included and is a number
expect(updatedAgent).toHaveProperty('version');
expect(typeof updatedAgent.version).toBe('number');
expect(updatedAgent.version).toBeGreaterThanOrEqual(1);
// Verify in database
const agentInDb = await Agent.findOne({ id: existingAgentId });
expect(updatedAgent.version).toBe(agentInDb.versions.length);
});
test('should handle validation errors properly', async () => {
mockReq.user.id = existingAgentAuthorId.toString();
mockReq.params.id = existingAgentId;

View file

@ -0,0 +1,380 @@
/**
* @jest-environment jsdom
*/
import * as React from 'react';
import { describe, it, expect, beforeEach, jest } from '@jest/globals';
import { render, waitFor, fireEvent } from '@testing-library/react';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import type { Agent } from 'librechat-data-provider';
// Mock toast context - define this after all mocks
let mockShowToast: jest.Mock;
// Mock notification severity enum before other imports
jest.mock('~/common/types', () => ({
NotificationSeverity: {
SUCCESS: 'success',
ERROR: 'error',
INFO: 'info',
WARNING: 'warning',
},
}));
// Mock store to prevent import errors
jest.mock('~/store/toast', () => ({
default: () => ({
showToast: jest.fn(),
}),
}));
jest.mock('~/store', () => {});
// Mock the data service to control network responses
jest.mock('librechat-data-provider', () => {
const actualModule = jest.requireActual('librechat-data-provider') as any;
return {
...actualModule,
dataService: {
updateAgent: jest.fn(),
},
Tools: {
execute_code: 'execute_code',
file_search: 'file_search',
web_search: 'web_search',
},
Constants: {
EPHEMERAL_AGENT_ID: 'ephemeral',
},
SystemRoles: {
ADMIN: 'ADMIN',
},
EModelEndpoint: {
agents: 'agents',
chatGPTBrowser: 'chatGPTBrowser',
gptPlugins: 'gptPlugins',
},
isAssistantsEndpoint: jest.fn(() => false),
};
});
jest.mock('@librechat/client', () => ({
Button: ({ children, onClick, ...props }: any) => (
<button onClick={onClick} {...props}>
{children}
</button>
),
useToastContext: () => ({
get showToast() {
return mockShowToast || jest.fn();
},
}),
}));
// Mock other dependencies
jest.mock('librechat-data-provider/react-query', () => ({
useGetModelsQuery: () => ({ data: {} }),
}));
jest.mock('~/utils', () => ({
createProviderOption: jest.fn((provider: string) => ({ value: provider, label: provider })),
getDefaultAgentFormValues: jest.fn(() => ({
id: '',
name: '',
description: '',
model: '',
provider: '',
})),
}));
jest.mock('~/hooks', () => ({
useSelectAgent: () => ({ onSelect: jest.fn() }),
useLocalize: () => (key: string) => key,
useAuthContext: () => ({ user: { id: 'user-123', role: 'USER' } }),
}));
jest.mock('~/Providers/AgentPanelContext', () => ({
useAgentPanelContext: () => ({
activePanel: 'builder',
agentsConfig: { allowedProviders: [] },
setActivePanel: jest.fn(),
endpointsConfig: {},
setCurrentAgentId: jest.fn(),
agent_id: 'agent-123',
}),
}));
jest.mock('~/common', () => ({
Panel: {
model: 'model',
builder: 'builder',
advanced: 'advanced',
},
}));
// Mock child components to simplify testing
jest.mock('./AgentPanelSkeleton', () => ({
__esModule: true,
default: () => <div>{`Loading...`}</div>,
}));
jest.mock('./Advanced/AdvancedPanel', () => ({
__esModule: true,
default: () => <div>{`Advanced Panel`}</div>,
}));
jest.mock('./AgentConfig', () => ({
__esModule: true,
default: () => <div>{`Agent Config`}</div>,
}));
jest.mock('./AgentSelect', () => ({
__esModule: true,
default: () => <div>{`Agent Select`}</div>,
}));
jest.mock('./ModelPanel', () => ({
__esModule: true,
default: () => <div>{`Model Panel`}</div>,
}));
// Mock AgentFooter to provide a save button
jest.mock('./AgentFooter', () => ({
__esModule: true,
default: () => (
<button type="submit" data-testid="save-agent-button">
{`Save Agent`}
</button>
),
}));
// Mock react-hook-form to capture form submission
let mockFormSubmitHandler: (() => void) | null = null;
jest.mock('react-hook-form', () => {
const actual = jest.requireActual('react-hook-form') as any;
return {
...actual,
useForm: () => {
const methods = actual.useForm({
defaultValues: {
id: 'agent-123',
name: 'Test Agent',
description: 'Test description',
model: 'gpt-4',
provider: 'openai',
tools: [],
execute_code: false,
file_search: false,
web_search: false,
},
});
return {
...methods,
handleSubmit: (onSubmit: any) => (e?: any) => {
e?.preventDefault?.();
mockFormSubmitHandler = () => onSubmit(methods.getValues());
return mockFormSubmitHandler;
},
};
},
FormProvider: ({ children }: any) => children,
useWatch: () => 'agent-123',
};
});
// Import after mocks
import { dataService } from 'librechat-data-provider';
import { useGetAgentByIdQuery } from '~/data-provider';
import AgentPanel from './AgentPanel';
// Mock useGetAgentByIdQuery
jest.mock('~/data-provider', () => {
const actual = jest.requireActual('~/data-provider') as any;
return {
...actual,
useGetAgentByIdQuery: jest.fn(),
useUpdateAgentMutation: actual.useUpdateAgentMutation,
};
});
// Test wrapper with QueryClient
const createWrapper = () => {
const queryClient = new QueryClient({
defaultOptions: {
queries: { retry: false },
mutations: { retry: false },
},
});
return ({ children }: { children: React.ReactNode }) => (
<QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
);
};
// Test helpers
const setupMocks = () => {
const mockUseGetAgentByIdQuery = useGetAgentByIdQuery as jest.MockedFunction<
typeof useGetAgentByIdQuery
>;
const mockUpdateAgent = dataService.updateAgent as jest.MockedFunction<
typeof dataService.updateAgent
>;
return { mockUseGetAgentByIdQuery, mockUpdateAgent };
};
const mockAgentQuery = (
mockUseGetAgentByIdQuery: jest.MockedFunction<typeof useGetAgentByIdQuery>,
agent: Partial<Agent>,
) => {
mockUseGetAgentByIdQuery.mockReturnValue({
data: {
id: 'agent-123',
author: 'user-123',
isCollaborative: false,
...agent,
} as Agent,
isInitialLoading: false,
} as any);
};
const createMockAgent = (overrides: Partial<Agent> = {}): Agent =>
({
id: 'agent-123',
provider: 'openai',
model: 'gpt-4',
...overrides,
}) as Agent;
const renderAndSubmitForm = async () => {
const Wrapper = createWrapper();
const { container, rerender } = render(<AgentPanel />, { wrapper: Wrapper });
const form = container.querySelector('form');
expect(form).toBeTruthy();
fireEvent.submit(form!);
if (mockFormSubmitHandler) {
mockFormSubmitHandler();
}
return { container, rerender, form };
};
describe('AgentPanel - Update Agent Toast Messages', () => {
beforeEach(() => {
jest.clearAllMocks();
mockShowToast = jest.fn();
mockFormSubmitHandler = null;
});
describe('AgentPanel', () => {
it('should show "no changes" toast when version does not change', async () => {
const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks();
// Mock the agent query with version 2
mockAgentQuery(mockUseGetAgentByIdQuery, {
name: 'Test Agent',
version: 2,
});
// Mock network response - same version
mockUpdateAgent.mockResolvedValue(createMockAgent({ name: 'Test Agent', version: 2 }));
await renderAndSubmitForm();
// Wait for the toast to be shown
await waitFor(() => {
expect(mockShowToast).toHaveBeenCalledWith({
message: 'com_ui_no_changes',
status: 'info',
});
});
});
it('should show "update success" toast when version changes', async () => {
const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks();
// Mock the agent query with version 2
mockAgentQuery(mockUseGetAgentByIdQuery, {
name: 'Test Agent',
version: 2,
});
// Mock network response - different version
mockUpdateAgent.mockResolvedValue(createMockAgent({ name: 'Test Agent', version: 3 }));
await renderAndSubmitForm();
await waitFor(() => {
expect(mockShowToast).toHaveBeenCalledWith({
message: 'com_assistants_update_success Test Agent',
});
});
});
it('should show "update success" with default name when agent has no name', async () => {
const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks();
// Mock the agent query without name
mockAgentQuery(mockUseGetAgentByIdQuery, {
version: 1,
});
// Mock network response - no name
mockUpdateAgent.mockResolvedValue(createMockAgent({ version: 2 }));
await renderAndSubmitForm();
await waitFor(() => {
expect(mockShowToast).toHaveBeenCalledWith({
message: 'com_assistants_update_success com_ui_agent',
});
});
});
it('should show "update success" when agent query has no version (undefined)', async () => {
const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks();
// Mock the agent query with no version data
mockAgentQuery(mockUseGetAgentByIdQuery, {
name: 'Test Agent',
// No version property
});
mockUpdateAgent.mockResolvedValue(createMockAgent({ name: 'Test Agent', version: 1 }));
await renderAndSubmitForm();
await waitFor(() => {
expect(mockShowToast).toHaveBeenCalledWith({
message: 'com_assistants_update_success Test Agent',
});
});
});
it('should show error toast on update failure', async () => {
const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks();
// Mock the agent query
mockAgentQuery(mockUseGetAgentByIdQuery, {
name: 'Test Agent',
version: 1,
});
// Mock network error
mockUpdateAgent.mockRejectedValue(new Error('Update failed'));
await renderAndSubmitForm();
await waitFor(() => {
expect(mockShowToast).toHaveBeenCalledWith({
message: 'com_agents_update_error com_ui_error: Update failed',
status: 'error',
});
});
});
});
});

View file

@ -1,5 +1,5 @@
import { Plus } from 'lucide-react';
import React, { useMemo, useCallback } from 'react';
import React, { useMemo, useCallback, useRef } from 'react';
import { Button, useToastContext } from '@librechat/client';
import { useWatch, useForm, FormProvider } from 'react-hook-form';
import { useGetModelsQuery } from 'librechat-data-provider/react-query';
@ -54,6 +54,7 @@ export default function AgentPanel() {
const { control, handleSubmit, reset } = methods;
const agent_id = useWatch({ control, name: 'id' });
const previousVersionRef = useRef<number | undefined>();
const allowedProviders = useMemo(
() => new Set(agentsConfig?.allowedProviders),
@ -77,50 +78,29 @@ export default function AgentPanel() {
/* Mutations */
const update = useUpdateAgentMutation({
onMutate: () => {
// Store the current version before mutation
previousVersionRef.current = agentQuery.data?.version;
},
onSuccess: (data) => {
showToast({
message: `${localize('com_assistants_update_success')} ${
data.name ?? localize('com_ui_agent')
}`,
});
// Check if agent version is the same (no changes were made)
if (previousVersionRef.current !== undefined && data.version === previousVersionRef.current) {
showToast({
message: localize('com_ui_no_changes'),
status: 'info',
});
} else {
showToast({
message: `${localize('com_assistants_update_success')} ${
data.name ?? localize('com_ui_agent')
}`,
});
}
// Clear the ref after use
previousVersionRef.current = undefined;
},
onError: (err) => {
const error = err as Error & {
statusCode?: number;
details?: { duplicateVersion?: any; versionIndex?: number };
response?: { status?: number; data?: any };
};
const isDuplicateVersionError =
(error.statusCode === 409 && error.details?.duplicateVersion) ||
(error.response?.status === 409 && error.response?.data?.details?.duplicateVersion);
if (isDuplicateVersionError) {
let versionIndex: number | undefined = undefined;
if (error.details?.versionIndex !== undefined) {
versionIndex = error.details.versionIndex;
} else if (error.response?.data?.details?.versionIndex !== undefined) {
versionIndex = error.response.data.details.versionIndex;
}
if (versionIndex === undefined || versionIndex < 0) {
showToast({
message: localize('com_agents_update_error'),
status: 'error',
duration: 5000,
});
} else {
showToast({
message: localize('com_ui_agent_version_duplicate', { versionIndex: versionIndex + 1 }),
status: 'error',
duration: 10000,
});
}
return;
}
const error = err as Error;
showToast({
message: `${localize('com_agents_update_error')}${
error.message ? ` ${localize('com_ui_error')}: ${error.message}` : ''

View file

@ -43,11 +43,7 @@ export const useCreateAgentMutation = (
*/
export const useUpdateAgentMutation = (
options?: t.UpdateAgentMutationOptions,
): UseMutationResult<
t.Agent,
t.DuplicateVersionError,
{ agent_id: string; data: t.AgentUpdateParams }
> => {
): UseMutationResult<t.Agent, Error, { agent_id: string; data: t.AgentUpdateParams }> => {
const queryClient = useQueryClient();
return useMutation(
({ agent_id, data }: { agent_id: string; data: t.AgentUpdateParams }) => {
@ -59,8 +55,7 @@ export const useUpdateAgentMutation = (
{
onMutate: (variables) => options?.onMutate?.(variables),
onError: (error, variables, context) => {
const typedError = error as t.DuplicateVersionError;
return options?.onError?.(typedError, variables, context);
return options?.onError?.(error, variables, context);
},
onSuccess: (updatedAgent, variables, context) => {
const listRes = queryClient.getQueryData<t.AgentListResponse>([

View file

@ -554,7 +554,6 @@
"com_ui_agent_var": "{{0}} agent",
"com_ui_agent_version": "Version",
"com_ui_agent_version_active": "Active Version",
"com_ui_agent_version_duplicate": "Duplicate version detected. This would create a version identical to Version {{versionIndex}}.",
"com_ui_agent_version_empty": "No versions available",
"com_ui_agent_version_error": "Error fetching versions",
"com_ui_agent_version_history": "Version History",

View file

@ -136,12 +136,7 @@ export type DuplicateVersionError = Error & {
};
};
export type UpdateAgentMutationOptions = MutationOptions<
Agent,
UpdateAgentVariables,
unknown,
DuplicateVersionError
>;
export type UpdateAgentMutationOptions = MutationOptions<Agent, UpdateAgentVariables>;
export type DuplicateAgentBody = {
agent_id: string;