From 336703fe48fbf8ead65e3f8014f48b271df798af Mon Sep 17 00:00:00 2001 From: Marco Beretta <81851188+berry-13@users.noreply.github.com> Date: Fri, 14 Aug 2026 18:20:51 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=94=20fix:=20Report=20Agent=20Saves=20?= =?UTF-8?q?That=20Reuse=20the=20Newest=20Version=20Entry=20(#14824)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: report agent saves that reuse the newest version entry An update whose result matches the newest version is written without recording a version entry. The Agent Builder derived its success message from the version count, so every such save reported "No changes were made" while the edit had in fact been persisted. Base the message on whether the submission carried an edit of its own instead, and keep the version count for the version history panel. Also stop suppressing the version entry when the update carries an atomic operator. isDuplicateVersion compares direct updates only, so it cannot speak for the operator half; suppressing there applied a change that no version entry recorded, leaving the document diverged from every entry in its own history. Closes #14809 * fix: count an avatar reset as a persisted edit An avatar upload uses its own endpoint, but a reset rides the update payload as avatar: null, so classifying every avatar-only submission as non-persisted was wrong for resets. Clearing an avatar the newest version never recorded reads as a duplicate to isDuplicateVersion, since it skips a field when both sides are falsy, so the reset landed with the version count unchanged and reported "No changes were made". * fix: skip the version entry when an atomic operator changes nothing An update carrying $push, $pull or $addToSet bypassed duplicate suppression on the operator's mere presence. Re-attaching a resource file an agent already holds makes $addToSet a no-op, so an agent with actions recorded a version entry for a write that never touched the document, and its version count climbed on retries. Resolve the operators against the current document instead. $push always appends and $pull matches arbitrary criteria, so both still count as mutating; $addToSet counts only when some value it adds is missing. Whatever cannot be compared cheaply counts as mutating, since over-reporting costs a redundant version while under-reporting would apply a change no version records. * fix: confirm the submitted edit survived before claiming a save changed anything Treating a dirty form as proof of a persisted edit reports success for a save that stored nothing. The server can normalize a submission straight back to the stored value: an MCP tool the user added is dropped when authorization rejects it, and a skill is pruned when it no longer exists. Neither moves the version count, so the toast claimed the agent was updated when it was untouched. Capture the agent as it stands before the write, since the mutation replaces that cache entry on success, and compare it against the one the server returns across the fields the submission carried. Keep the dirty check alongside it: an agent loaded through the basic projection carries fewer fields than the update endpoint returns, and pairing the two keeps an untouched save honest either way. * fix: compare a save against the expanded agent, not a basic projection The panel falls back to the basic agent query whenever the expanded one has not resolved, and that projection drops instructions, tools, edges, skills and the rest while reducing model_parameters to a single flag. Comparing a submission against it made every one of those fields read as changed, so a rejected MCP tool or a pruned skill still reported success. Compare against the expanded agent, the only projection carrying every field a submission sends. When it is unavailable the comparison reports true and leaves the dirty check to decide, since claiming nothing changed for a save that did is the worse of the two errors. Renamed to say what it now answers. * fix: drop the operator a suppressed update judged a no-op Suppression reads whether $addToSet would add anything from a document fetched before the write, and that reading cannot bind a concurrent one. A $pull landing in between leaves the operator re-adding the value while the version entry has already been suppressed, which is the one outcome this path exists to prevent: a change applied with nothing in the history recording it. Drop what was judged a no-op instead of racing it. Only $addToSet reaches here, and only once every value it adds was found stored, so removing it makes the suppression true by construction rather than true if nothing else writes first. * fix: leave a suppressed update carrying no operator at all Dropping only $addToSet left the invariant resting on which operators callers happen to send. A present but empty $push or $pull counts as no operator when deciding suppression, yet survived into the write, so the suppressed update was operator-free by convention rather than by construction. Drop all three. Reaching suppression already means none of them can change the document, so removing them states that outright and keeps the write consistent with the history it declines to record. --- .../SidePanel/Agents/AgentPanel.test.tsx | 144 +++++++++- .../SidePanel/Agents/AgentPanel.tsx | 74 +++++- .../__tests__/AgentPanel.helpers.spec.ts | 120 +++++++++ .../data-schemas/src/methods/agent.spec.ts | 250 ++++++++++++++++++ packages/data-schemas/src/methods/agent.ts | 115 ++++++-- 5 files changed, 665 insertions(+), 38 deletions(-) diff --git a/client/src/components/SidePanel/Agents/AgentPanel.test.tsx b/client/src/components/SidePanel/Agents/AgentPanel.test.tsx index e2899bf1e6..213d94a10f 100644 --- a/client/src/components/SidePanel/Agents/AgentPanel.test.tsx +++ b/client/src/components/SidePanel/Agents/AgentPanel.test.tsx @@ -2,9 +2,11 @@ * @jest-environment jsdom */ import * as React from 'react'; -import { render, waitFor, fireEvent } from '@testing-library/react'; +import { render, waitFor, fireEvent, act } from '@testing-library/react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import type { UseFormReturn } from 'react-hook-form'; import type { Agent } from 'librechat-data-provider'; +import type { AgentForm } from '~/common'; // Mock toast context - define this after all mocks let mockShowToast: jest.Mock; @@ -167,6 +169,7 @@ jest.mock('./AgentFooter', () => ({ // Mock react-hook-form to capture form submission let mockFormSubmitHandler: (() => void) | null = null; +let capturedFormMethods: UseFormReturn | null = null; jest.mock('react-hook-form', () => { const actual = jest.requireActual('react-hook-form') as any; @@ -187,6 +190,8 @@ jest.mock('react-hook-form', () => { }, }); + capturedFormMethods = methods; + return { ...methods, handleSubmit: (onSubmit: any) => (e?: any) => { @@ -203,7 +208,7 @@ jest.mock('react-hook-form', () => { // Import after mocks import { dataService } from 'librechat-data-provider'; -import { useGetAgentByIdQuery } from '~/data-provider'; +import { useGetAgentByIdQuery, useGetExpandedAgentByIdQuery } from '~/data-provider'; import AgentPanel from './AgentPanel'; // Mock useGetAgentByIdQuery @@ -212,10 +217,7 @@ jest.mock('~/data-provider', () => { return { ...actual, useGetAgentByIdQuery: jest.fn(), - useGetExpandedAgentByIdQuery: jest.fn(() => ({ - data: null, - isInitialLoading: false, - })), + useGetExpandedAgentByIdQuery: jest.fn(), useUpdateAgentMutation: actual.useUpdateAgentMutation, }; }); @@ -250,14 +252,22 @@ const mockAgentQuery = ( mockUseGetAgentByIdQuery: jest.MockedFunction, agent: Partial, ) => { - mockUseGetAgentByIdQuery.mockReturnValue({ - data: { - id: 'agent-123', - author: 'user-123', - ...agent, - } as Agent, - isInitialLoading: false, - } as any); + const data = { + id: 'agent-123', + author: 'user-123', + /** Matches `createMockAgent`, so a field the submission carries but never edits + * compares equal across the update rather than reading as a change. */ + provider: 'openai', + model: 'gpt-4', + ...agent, + } as Agent; + + mockUseGetAgentByIdQuery.mockReturnValue({ data, isInitialLoading: false } as any); + /** The panel resolves to the expanded query once it has data, and only that projection + * carries every field the submission compares against. */ + ( + useGetExpandedAgentByIdQuery as jest.MockedFunction + ).mockReturnValue({ data, isInitialLoading: false } as any); }; const createMockAgent = (overrides: Partial = {}): Agent => @@ -289,6 +299,7 @@ describe('AgentPanel - Update Agent Toast Messages', () => { jest.clearAllMocks(); mockShowToast = jest.fn(); mockFormSubmitHandler = null; + capturedFormMethods = null; }); describe('AgentPanel', () => { @@ -315,6 +326,111 @@ describe('AgentPanel - Update Agent Toast Messages', () => { }); }); + it('should show "update success" toast when an edited agent reuses the same version', async () => { + const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks(); + + mockAgentQuery(mockUseGetAgentByIdQuery, { + name: 'Test Agent', + version: 2, + }); + + /** An update whose result matches the newest version is written without recording a + * version entry, so the count comes back unchanged even though the edit was saved. */ + mockUpdateAgent.mockResolvedValue(createMockAgent({ name: 'Renamed Agent', version: 2 })); + + const Wrapper = createWrapper(); + const { container } = render(, { wrapper: Wrapper }); + + act(() => { + capturedFormMethods!.setValue('name', 'Renamed Agent', { shouldDirty: true }); + }); + + fireEvent.submit(container.querySelector('form')!); + mockFormSubmitHandler?.(); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith({ + message: 'com_assistants_update_success_name', + status: undefined, + }); + }); + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ message: 'com_ui_no_changes' }), + ); + }); + + it('should show "update success" toast when an avatar reset reuses the same version', async () => { + const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks(); + + mockAgentQuery(mockUseGetAgentByIdQuery, { + name: 'Test Agent', + version: 2, + avatar: { filepath: '/images/agent-123/avatar.png', source: 'local' }, + }); + + /** A reset rides the update payload as `avatar: null`, and clearing an avatar the + * newest version never recorded reads as a duplicate, so the count comes back + * unchanged even though the avatar was deleted. */ + mockUpdateAgent.mockResolvedValue( + createMockAgent({ name: 'Test Agent', version: 2, avatar: null }), + ); + + const Wrapper = createWrapper(); + const { container } = render(, { wrapper: Wrapper }); + + act(() => { + capturedFormMethods!.setValue('avatar_action', 'reset', { shouldDirty: true }); + }); + + fireEvent.submit(container.querySelector('form')!); + mockFormSubmitHandler?.(); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith({ + message: 'com_assistants_update_success_name', + status: undefined, + }); + }); + expect(mockShowToast).not.toHaveBeenCalledWith( + expect.objectContaining({ message: 'com_ui_no_changes' }), + ); + }); + + it('should show "no changes" toast when the server drops the submitted edit', async () => { + const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks(); + + mockAgentQuery(mockUseGetAgentByIdQuery, { + name: 'Test Agent', + version: 2, + tools: ['keep'], + }); + + /** An MCP tool the user added can be stripped by authorization before the write, so + * a dirty submission is no promise that anything was persisted. */ + mockUpdateAgent.mockResolvedValue( + createMockAgent({ name: 'Test Agent', version: 2, tools: ['keep'] }), + ); + + const Wrapper = createWrapper(); + const { container } = render(, { wrapper: Wrapper }); + + act(() => { + capturedFormMethods!.setValue('tools', ['keep', 'rejected_mcp_tool'], { + shouldDirty: true, + }); + }); + + fireEvent.submit(container.querySelector('form')!); + mockFormSubmitHandler?.(); + + 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(); diff --git a/client/src/components/SidePanel/Agents/AgentPanel.tsx b/client/src/components/SidePanel/Agents/AgentPanel.tsx index a796ee6c56..f36aa8ddad 100644 --- a/client/src/components/SidePanel/Agents/AgentPanel.tsx +++ b/client/src/components/SidePanel/Agents/AgentPanel.tsx @@ -1,5 +1,6 @@ import React, { useMemo, useCallback, useRef, useState } from 'react'; import { Plus } from 'lucide-react'; +import isEqual from 'lodash/isEqual'; import { Button, useToastContext } from '@librechat/client'; import { useWatch, useForm, FormProvider } from 'react-hook-form'; import { useGetModelsQuery } from 'librechat-data-provider/react-query'; @@ -12,8 +13,8 @@ import { PermissionBits, isAssistantsEndpoint, } from 'librechat-data-provider'; +import type { Agent, AgentUpdateParams } from 'librechat-data-provider'; import type { FieldNamesMarkedBoolean } from 'react-hook-form'; -import type { Agent } from 'librechat-data-provider'; import type { TranslationKeys } from '~/hooks/useLocalize'; import type { AgentForm, StringOption } from '~/common'; import { @@ -224,6 +225,52 @@ export const isAvatarUploadOnlyDirty = ( return result.sawDirty && result.onlyAvatarDirty; }; +/** + * Whether the submission carries an edit the agent update endpoint persists. Only an + * avatar upload travels through its own endpoint; a reset rides the update payload as + * `avatar: null` (see `composeAgentUpdatePayload`), so it is an edit like any other. + */ +export const hasPersistedDirtyFields = ( + dirtyFields?: FieldNamesMarkedBoolean, + avatarAction?: AgentForm['avatar_action'], +): boolean => { + if (avatarAction === 'reset') { + return true; + } + + if (!dirtyFields) { + return false; + } + + const result = evaluateDirtyFields(dirtyFields); + return result.sawDirty && !result.onlyAvatarDirty; +}; + +/** + * Whether the save may have left the stored agent different from the one it replaced, + * across the fields the submission carried. A dirty field is no promise that anything was + * written: the server can normalize a submission straight back to the stored value, by + * pruning a skill that no longer exists or by dropping an MCP tool authorization rejects. + * + * `previous` must be the expanded agent. A basic projection omits fields the submission + * still carries, and the update endpoint answers with their unchanged values, which would + * read as a change that never happened. Without it the comparison cannot be trusted and + * reports true, leaving the dirty check to decide: claiming nothing changed for a save + * that did is the worse error of the two. + */ +export const mayHavePersistedChange = ( + submitted?: AgentUpdateParams, + previous?: Agent, + updated?: Agent, +): boolean => { + if (!submitted || !previous || !updated) { + return true; + } + + const fields = Object.keys(submitted) as Array; + return fields.some((field) => !isEqual(previous[field], updated[field])); +}; + export default function AgentPanel() { const localize = useLocalize(); const { user } = useAuthContext(); @@ -315,6 +362,8 @@ export default function AgentPanel() { ); const agent_id = useWatch({ control, name: 'id' }); const previousVersionRef = useRef(); + const submittedDirtyRef = useRef(false); + const submittedRef = useRef<{ payload?: AgentUpdateParams; previous?: Agent }>({}); const allowedProviders = useMemo( () => new Set(agentsConfig?.allowedProviders), @@ -336,14 +385,27 @@ export default function AgentPanel() { /* Mutations */ const update = useUpdateAgentMutation({ - onMutate: () => { - // Store the current version before mutation + onMutate: (variables) => { + /** The agent as it stands before the write, taken from the expanded query so every + * submitted field is comparable. The mutation replaces this cache entry on success, + * so it has to be captured here to stay comparable afterwards. */ previousVersionRef.current = agentQuery.data?.version; + submittedDirtyRef.current = hasPersistedDirtyFields(dirtyFields, getValues('avatar_action')); + submittedRef.current = { payload: variables.data, previous: expandedAgentQuery.data }; }, onSuccess: async (data) => { const avatarActionState = getValues('avatar_action'); + /** An update whose result matches the newest version is written without recording a + * version entry, so an unchanged count no longer means the save was a no-op. Only + * a save that both carried no edit and left the agent as it found it can claim + * nothing changed. */ + const persistedEdit = + submittedDirtyRef.current && + mayHavePersistedChange(submittedRef.current.payload, submittedRef.current.previous, data); const noVersionChange = - previousVersionRef.current !== undefined && data.version === previousVersionRef.current; + !persistedEdit && + previousVersionRef.current !== undefined && + data.version === previousVersionRef.current; const toastMessage = getUpdateToastMessage( noVersionChange, avatarActionState, @@ -375,8 +437,10 @@ export default function AgentPanel() { setValue('avatar_preview', '', { shouldDirty: false }); } - // Clear the ref after use + // Clear the refs after use previousVersionRef.current = undefined; + submittedDirtyRef.current = false; + submittedRef.current = {}; }, onError: (err) => { const error = err as Error; diff --git a/client/src/components/SidePanel/Agents/__tests__/AgentPanel.helpers.spec.ts b/client/src/components/SidePanel/Agents/__tests__/AgentPanel.helpers.spec.ts index c0f701200e..599e2e216e 100644 --- a/client/src/components/SidePanel/Agents/__tests__/AgentPanel.helpers.spec.ts +++ b/client/src/components/SidePanel/Agents/__tests__/AgentPanel.helpers.spec.ts @@ -2,12 +2,15 @@ * @jest-environment jsdom */ import { Constants, type Agent } from 'librechat-data-provider'; +import type { AgentModelParameters } from 'librechat-data-provider'; import type { FieldNamesMarkedBoolean } from 'react-hook-form'; import type { AgentForm } from '~/common'; import { composeAgentUpdatePayload, persistAvatarChanges, isAvatarUploadOnlyDirty, + hasPersistedDirtyFields, + mayHavePersistedChange, } from '../AgentPanel'; const createForm = (): AgentForm => ({ @@ -166,3 +169,120 @@ describe('isAvatarUploadOnlyDirty', () => { expect(isAvatarUploadOnlyDirty(dirtyFields)).toBe(false); }); }); + +describe('hasPersistedDirtyFields', () => { + it('returns false for an untouched form', () => { + expect(hasPersistedDirtyFields(undefined)).toBe(false); + expect(hasPersistedDirtyFields({} as FieldNamesMarkedBoolean)).toBe(false); + }); + + it('returns false for an upload-only submission, which uses its own endpoint', () => { + const dirtyFields = { + avatar_action: true, + avatar_preview: true, + } as FieldNamesMarkedBoolean; + + expect(hasPersistedDirtyFields(dirtyFields, 'upload')).toBe(false); + }); + + it('returns true for a reset-only submission, which is sent as avatar: null', () => { + const dirtyFields = { + avatar_action: true, + avatar_preview: true, + } as FieldNamesMarkedBoolean; + + expect(hasPersistedDirtyFields(dirtyFields, 'reset')).toBe(true); + }); + + it('returns true for an edit the update endpoint persists', () => { + const dirtyFields = { + avatar_action: true, + tools: true, + } as unknown as FieldNamesMarkedBoolean; + + expect(hasPersistedDirtyFields(dirtyFields)).toBe(true); + }); + + it('ignores the agent field, which tracks selection rather than an edit', () => { + const dirtyFields = { + agent: { value: true }, + } as unknown as FieldNamesMarkedBoolean; + + expect(hasPersistedDirtyFields(dirtyFields)).toBe(false); + }); +}); + +describe('mayHavePersistedChange', () => { + const agent = (overrides: Partial = {}): Agent => + ({ + id: 'agent_123', + provider: 'openai', + model: 'gpt-4', + name: 'Agent', + tools: ['a'], + ...overrides, + }) as Agent; + + it('returns true when anything needed for the comparison is missing', () => { + /** Without the expanded agent the comparison cannot be trusted, and reporting no + * change for a save that did change is the worse error. */ + expect(mayHavePersistedChange(undefined, agent(), agent())).toBe(true); + expect(mayHavePersistedChange({ name: 'Renamed' }, undefined, agent())).toBe(true); + expect(mayHavePersistedChange({ name: 'Renamed' }, agent(), undefined)).toBe(true); + }); + + it('returns true when the stored agent came back changed', () => { + expect(mayHavePersistedChange({ name: 'Renamed' }, agent(), agent({ name: 'Renamed' }))).toBe( + true, + ); + }); + + it('returns true for a reset that cleared an avatar the agent was carrying', () => { + const previous = agent({ avatar: { filepath: '/images/a.png', source: 'local' } }); + + expect(mayHavePersistedChange({ avatar: null }, previous, agent({ avatar: null }))).toBe(true); + }); + + it('returns false when the server normalized the submission back to the stored value', () => { + /** An MCP tool rejected by authorization, or a skill pruned because it no longer + * exists, is dropped server-side and nothing is persisted. */ + const stored = agent({ tools: ['a'], skills: ['keep'] }); + + expect( + mayHavePersistedChange( + { tools: ['a', 'mcp_rejected'], skills: ['keep', 'deleted'] }, + stored, + agent({ tools: ['a'], skills: ['keep'] }), + ), + ).toBe(false); + }); + + it('compares nested values rather than object identity', () => { + const parameters = (temperature: number): AgentModelParameters => ({ + ...createForm().model_parameters, + temperature, + }); + const previous = agent({ model_parameters: parameters(1) }); + + expect( + mayHavePersistedChange( + { model_parameters: parameters(1) }, + previous, + agent({ model_parameters: parameters(1) }), + ), + ).toBe(false); + expect( + mayHavePersistedChange( + { model_parameters: parameters(2) }, + previous, + agent({ model_parameters: parameters(2) }), + ), + ).toBe(true); + }); + + it('ignores fields the submission did not carry', () => { + expect( + mayHavePersistedChange({ name: 'Agent' }, agent({ description: 'before' }), agent()), + ).toBe(false); + }); +}); diff --git a/packages/data-schemas/src/methods/agent.spec.ts b/packages/data-schemas/src/methods/agent.spec.ts index 47da924565..a440dfc882 100644 --- a/packages/data-schemas/src/methods/agent.spec.ts +++ b/packages/data-schemas/src/methods/agent.spec.ts @@ -16,6 +16,7 @@ import type { RootFilterQuery, QueryOptions, UpdateQuery, + Model, } from 'mongoose'; import type { IAgent, IAclEntry, IUser, IAccessRole } from '..'; import { createAgentMethods, type AgentMethods } from './agent'; @@ -2014,6 +2015,255 @@ describe('Agent Methods', () => { expect(reloaded!.versions).toHaveLength(2); }); + test('should persist a resource file attached to an agent carrying actions', async () => { + const agentId = `agent_${uuidv4()}`; + const fileIdsOf = (agent: IAgent | null) => + (agent?.tool_resources as Record | undefined)?.file_search + ?.file_ids; + + await createAgent({ + id: agentId, + provider: 'test', + model: 'test-model', + author: new mongoose.Types.ObjectId(), + actions: [`example.com${actionDelimiter}act_1`], + tools: [], + }); + + /** `isDuplicateVersion` only skips operator-only updates while `actionsHash` is + * falsy, so an agent with actions reaches the comparison on every file attach. */ + await updateAgent({ id: agentId }, { name: 'With actions' }); + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f1', + }); + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f1', + }); + + /** The re-attach snapshots the current state, so the document now equals the newest + * version and the next attach is judged a duplicate. */ + const settled = await getAgent({ id: agentId }); + const newestVersion = settled!.versions![settled!.versions!.length - 1] as VersionEntry; + expect(fileIdsOf(settled)).toEqual(['f1']); + expect(newestVersion.tool_resources).toEqual(settled!.tool_resources); + + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f2', + }); + + expect(fileIdsOf(await getAgent({ id: agentId }))).toEqual(['f1', 'f2']); + }); + + test('should not record a version when an atomic operator changes nothing', async () => { + const agentId = `agent_${uuidv4()}`; + + await createAgent({ + id: agentId, + provider: 'test', + model: 'test-model', + author: new mongoose.Types.ObjectId(), + actions: [`example.com${actionDelimiter}act_1`], + tools: [], + }); + + await updateAgent({ id: agentId }, { name: 'With actions' }); + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f1', + }); + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f1', + }); + + /** The document now equals its newest version, so the snapshot is a duplicate and + * only the operator can justify recording an entry. */ + const settled = await getAgent({ id: agentId }); + const versionCount = settled!.versions!.length; + + /** Re-attaching an id the agent already holds makes `$addToSet` a Mongo no-op. An + * entry here would record a change the document never took. */ + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f1', + }); + + const after = await getAgent({ id: agentId }); + expect(after!.versions).toHaveLength(versionCount); + expect( + (after?.tool_resources as Record | undefined)?.file_search + ?.file_ids, + ).toEqual(['f1']); + }); + + test('should send no mutating operator once it has suppressed the version entry', async () => { + const agentId = `agent_${uuidv4()}`; + + await createAgent({ + id: agentId, + provider: 'test', + model: 'test-model', + author: new mongoose.Types.ObjectId(), + actions: [`example.com${actionDelimiter}act_1`], + tools: [], + }); + + await updateAgent({ id: agentId }, { name: 'With actions' }); + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f1', + }); + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f1', + }); + + const settled = await getAgent({ id: agentId }); + const versionCount = settled!.versions!.length; + + /** The no-op reading comes from a document fetched before the write, so a `$pull` + * landing in between would leave a surviving `$addToSet` re-adding the value with + * no version entry recording it. Suppressing means the operator is gone, not that + * it is expected to stay harmless. */ + const Agent = mongoose.models.Agent as Model; + const spy = jest.spyOn(Agent, 'findOneAndUpdate'); + + await addAgentResourceFile({ + agent_id: agentId, + tool_resource: 'file_search', + file_id: 'f1', + }); + + const suppressedUpdate = spy.mock.calls[spy.mock.calls.length - 1][1] as UpdateQuery; + spy.mockRestore(); + + expect(suppressedUpdate.$addToSet).toBeUndefined(); + expect(suppressedUpdate.$push).toBeUndefined(); + expect(suppressedUpdate.$pull).toBeUndefined(); + expect((await getAgent({ id: agentId }))!.versions).toHaveLength(versionCount); + }); + + test('should record a version when a duplicate direct update carries an atomic operator', async () => { + const agentId = `agent_${uuidv4()}`; + + await createAgent({ + id: agentId, + provider: 'test', + model: 'test-model', + author: new mongoose.Types.ObjectId(), + name: 'Operator agent', + tools: [], + }); + await updateAgent({ id: agentId }, { name: 'Renamed' }); + + /** The direct half matches the newest version while the operator half really changes + * the document. Suppressing here would apply a change no version entry records, and + * the document would diverge from every entry in its own history. */ + const updated = await updateAgent( + { id: agentId }, + { name: 'Renamed', $push: { tools: 'appended_tool' } }, + ); + + expect(updated!.tools).toEqual(['appended_tool']); + expect(updated!.versions).toHaveLength(3); + + const reloaded = await getAgent({ id: agentId }); + expect(reloaded!.tools).toEqual(['appended_tool']); + expect(reloaded!.versions).toHaveLength(3); + }); + + test('should persist an update that repairs drift left by a skipVersioning write', async () => { + const agentId = `agent_${uuidv4()}`; + + await createAgent({ + id: agentId, + provider: 'test', + model: 'test-model', + author: new mongoose.Types.ObjectId(), + description: 'original', + }); + await updateAgent({ id: agentId }, { name: 'Versioned' }); + + /** `skipVersioning` writes snapshot nothing, so the document drifts from the newest + * version without any entry recording it. */ + await updateAgent({ id: agentId }, { description: 'drifted' }, { skipVersioning: true }); + expect((await getAgent({ id: agentId }))!.description).toBe('drifted'); + + const repaired = await updateAgent({ id: agentId }, { description: 'original' }); + + expect(repaired!.description).toBe('original'); + expect(repaired!.versions).toHaveLength(2); + expect((await getAgent({ id: agentId }))!.description).toBe('original'); + }); + + test('should clear an avatar the newest version never recorded without adding a version', async () => { + const agentId = `agent_${uuidv4()}`; + + await createAgent({ + id: agentId, + provider: 'test', + model: 'test-model', + author: new mongoose.Types.ObjectId(), + name: 'Avatar agent', + }); + await updateAgent({ id: agentId }, { name: 'Avatar agent' }); + + /** Avatar writes go through `skipVersioning`, so the newest version can carry no + * avatar at all while the document has one. */ + await updateAgent( + { id: agentId }, + { avatar: { filepath: '/images/a.png', source: 'local' } }, + { skipVersioning: true }, + ); + const withAvatar = await getAgent({ id: agentId }); + expect(withAvatar!.avatar).toBeTruthy(); + expect((withAvatar!.versions![1] as VersionEntry).avatar).toBeUndefined(); + + /** `isDuplicateVersion` skips a field when both sides are falsy, so clearing the + * avatar reads as a duplicate: the write lands and the count stays put. */ + const cleared = await updateAgent({ id: agentId }, { avatar: null }); + + expect(cleared!.avatar).toBeNull(); + expect(cleared!.versions).toHaveLength(2); + expect((await getAgent({ id: agentId }))!.avatar).toBeNull(); + }); + + test('should leave the document untouched when a duplicate update changes nothing', async () => { + const agentId = `agent_${uuidv4()}`; + + await createAgent({ + id: agentId, + provider: 'test', + model: 'test-model', + author: new mongoose.Types.ObjectId(), + name: 'Idempotent', + tools: ['a', 'b'], + }); + await updateAgent({ id: agentId }, { name: 'Idempotent' }); + + const before = await getAgent({ id: agentId }); + const duplicate = await updateAgent({ id: agentId }, { name: 'Idempotent' }); + + /** The suppressed path reports the unchanged version count as `version`. */ + expect((duplicate as IAgent & { version?: number }).version).toBe(before!.versions!.length); + + const after = await getAgent({ id: agentId }); + expect(after!.name).toBe(before!.name); + expect(after!.tools).toEqual(before!.tools); + expect(after!.versions).toHaveLength(before!.versions!.length); + }); + test('should track updatedBy when a different user updates an agent', async () => { const agentId = `agent_${uuidv4()}`; const originalAuthor = new mongoose.Types.ObjectId(); diff --git a/packages/data-schemas/src/methods/agent.ts b/packages/data-schemas/src/methods/agent.ts index 0ed31cfe81..22b0412e1b 100644 --- a/packages/data-schemas/src/methods/agent.ts +++ b/packages/data-schemas/src/methods/agent.ts @@ -187,6 +187,72 @@ function rebuildMCPServerNames(tools: string[] | undefined | null, priorNames: s return Array.from(retained); } +const hasOperatorKeys = (value: unknown): boolean => + typeof value === 'object' && value !== null && Object.keys(value as object).length > 0; + +/** Resolves a dotted operator path, such as `tool_resources.file_search.file_ids`. */ +function resolveDocumentPath(source: Record, path: string): unknown { + let current: unknown = source; + for (const segment of path.split('.')) { + if (typeof current !== 'object' || current === null) { + return undefined; + } + current = + current instanceof Map ? current.get(segment) : (current as Record)[segment]; + } + return current; +} + +/** The values an `$addToSet` specification would add, flattening the `$each` form. */ +function addToSetCandidates(spec: unknown): unknown[] { + if ( + typeof spec === 'object' && + spec !== null && + Array.isArray((spec as { $each?: unknown }).$each) + ) { + return (spec as { $each: unknown[] }).$each; + } + return [spec]; +} + +/** + * Whether an update's atomic operators can still change the stored document. `$push` + * always appends and `$pull` matches on arbitrary query criteria, so both count as + * mutating. `$addToSet` is a no-op once every value it adds is already stored, which is + * exactly what an idempotent retry looks like, so it is resolved against the document. + * Whatever cannot be compared cheaply counts as mutating: over-reporting only records a + * redundant version, while under-reporting would apply a change no version records. + */ +function operatorsMutateDocument( + currentObject: Record, + $push: unknown, + $pull: unknown, + $addToSet: unknown, +): boolean { + if (hasOperatorKeys($push) || hasOperatorKeys($pull)) { + return true; + } + + if (!hasOperatorKeys($addToSet)) { + return false; + } + + for (const [path, spec] of Object.entries($addToSet as Record)) { + const existing = resolveDocumentPath(currentObject, path); + const stored = Array.isArray(existing) ? existing : []; + for (const candidate of addToSetCandidates(spec)) { + if (typeof candidate === 'object' && candidate !== null) { + return true; + } + if (!stored.includes(candidate)) { + return true; + } + } + } + + return false; +} + /** * Check if a version already exists in the versions array, excluding timestamp and author fields. */ @@ -610,14 +676,8 @@ export function createAgentMethods( const currentAgent = await Agent.findOne(searchParameter); if (currentAgent) { - const { - __v, - _id, - id: __id, - versions, - author: _author, - ...versionData - } = currentAgent.toObject() as unknown as Record; + const currentObject = currentAgent.toObject() as unknown as Record; + const { __v, _id, id: __id, versions, author: _author, ...versionData } = currentObject; const { $push, $pull, $addToSet, ...directUpdates } = updateData; /** Self-heal: drop allowlist ids whose skill no longer exists in the @@ -696,15 +756,31 @@ export function createAgentMethods( versions as Record[], actionsHash, ); - if (duplicateVersion && !forceVersion) { - /** A snapshot identical to the newest version adds no history, but the update - * itself must still be applied. The document is not always equal to that - * version: `$push`/`$pull`/`$addToSet` updates snapshot the pre-update state, - * and `skipVersioning` writes snapshot nothing at all. Whenever the caller - * moves the document back onto the newest version's content — removing a tool - * `addAgentResourceFile` added, for instance — returning here dropped a real - * change and reported success. */ + /** A duplicate snapshot adds no history, but the write itself must still land: the + * document is regularly not equal to its newest version, because `$push`/`$pull`/ + * `$addToSet` snapshot the pre-update state and `skipVersioning` snapshots nothing. + * `isDuplicateVersion` compares direct updates only, so it cannot speak for an + * update that also carries an operator that lands a change; suppressing there + * would apply a change no version records. An operator that changes nothing, the + * shape of an idempotent retry, leaves the snapshot a genuine duplicate. */ + const mutatesOutsideSnapshot = operatorsMutateDocument( + currentObject, + $push, + $pull, + $addToSet, + ); + if (duplicateVersion && !forceVersion && !mutatesOutsideSnapshot) { suppressedVersionEntry = true; + /** Every operator that reaches here was judged unable to change the document, + * and for `$addToSet` that reading came from a document fetched before the + * write, so it cannot bind a concurrent one: a `$pull` landing in between would + * leave this update re-adding the value with no version entry to record it. + * Drop what was judged a no-op rather than race it, so the suppressed write + * carries no operator at all and is true by construction instead of true only + * while nothing else writes first. */ + delete updateData.$addToSet; + delete updateData.$push; + delete updateData.$pull; } } @@ -736,9 +812,10 @@ export function createAgentMethods( mongoOptions, ).lean()) as IAgent | null; - /** Callers that create a version read `version` back from their own count of - * `versions`; a suppressed entry leaves that count unchanged, so report it here to - * keep the "no new version" signal these callers already relied on. */ + /** `version` is a response-only field holding the count of `versions`. It is reported + * here so a suppressed entry keeps the shape callers saw before the write was fixed. + * It answers "was a version recorded", never "did the update apply". The two stopped + * being the same question once a suppressed update started landing. */ if (updatedAgent && suppressedVersionEntry) { (updatedAgent as IAgent & { version?: number }).version = updatedAgent.versions?.length ?? 0; }