diff --git a/src/components/configuration/review/ChangeReview.test.tsx b/src/components/configuration/review/ChangeReview.test.tsx index c245ba7c..143167ff 100644 --- a/src/components/configuration/review/ChangeReview.test.tsx +++ b/src/components/configuration/review/ChangeReview.test.tsx @@ -56,6 +56,9 @@ function reviewFor( saving: false, sessionExpired: false, termsUrl: null, + conflict: false, + reloadLatest: vi.fn(), + mergeLatest: vi.fn().mockResolvedValue(undefined), saveIssues: [], title: 'Review', blocked: issues.some((issue) => issue.blocking), @@ -99,6 +102,47 @@ describe('ChangeReview', () => { expect(link).toHaveAttribute('target', '_blank'); }); + it('offers to reload or merge when the target was saved elsewhere meanwhile', () => { + const review = reviewFor([{ fieldPath: 'registration.socialLogins', value: ['openid'] }], { + conflict: true, + }); + render(); + const alert = screen.getByRole('alert'); + expect(alert).toHaveTextContent('Changed elsewhere'); + expect(alert).toHaveTextContent(/Base configuration was saved elsewhere/); + fireEvent.click(within(alert).getByRole('button', { name: 'Reload' })); + expect(review.reloadLatest).toHaveBeenCalledTimes(1); + fireEvent.click(within(alert).getByRole('button', { name: 'Merge and save' })); + expect(review.mergeLatest).toHaveBeenCalledTimes(1); + }); + + it('keeps the footer Save disabled while the conflict waits for Reload or Merge', () => { + const edit = [{ fieldPath: 'registration.socialLogins', value: ['openid'] }]; + const { unmount } = render(); + const footer = screen.getByRole('contentinfo'); + expect(within(footer).getByRole('button', { name: 'Save' })).toBeDisabled(); + unmount(); + render(); + expect(screen.getByRole('button', { name: 'Save' })).toBeEnabled(); + }); + + it('names the profile that was saved elsewhere', () => { + const scope = { + principalType: 'role', + principalId: 'USER', + name: 'USER', + } as t.ConfigScope; + render( + , + ); + expect(screen.getByRole('alert')).toHaveTextContent(/USER was saved elsewhere/); + }); + it('shows list changes as added and removed items', () => { render( +

+

+

+ {localize('com_config_conflict_body', { + target: review.scope + ? `${scopeTypeLabel(review.scope, localize)}: ${review.scope.name}` + : localize('com_scope_base_config'), + })} +

+
+
+ + ); +} + /** * The change review drawer (WS7): what Save writes to the open target, * grouped tab › section › entry, with BEFORE → AFTER, the value a reset * reverts to, masked secrets, warnings and problems. Save stays disabled - * while pre-flight problems exist. + * while pre-flight problems exist, and after a conflict, which the notice's + * Reload or Merge and save resolves. */ export function ChangeReview({ review }: t.ChangeReviewProps) { const localize = useLocalize(); - const { model, open, saving, blocked } = review; + const { model, open, saving, blocked, conflict } = review; const blocking = model.issues.filter((issue) => issue.blocking); const isEmpty = model.count === 0; const returnFocus = useReturnFocus(open, activeConfigTab); @@ -148,6 +192,7 @@ export function ChangeReview({ review }: t.ChangeReviewProps) {
+ {conflict && } {blocking.length > 0 && ( void review.confirm()} - disabled={saving || blocked} + disabled={saving || blocked || conflict} /> diff --git a/src/constants/contract.ts b/src/constants/contract.ts index 6016fe0c..efaf1514 100644 --- a/src/constants/contract.ts +++ b/src/constants/contract.ts @@ -52,6 +52,15 @@ export const BUNDLED_CONFIG_LIMITS: t.ConfigLimits = { overrideCacheTtlMs: 60_000, }; +/** `expectedRevision` that makes a write apply only when the principal has no document. */ +export const ABSENT_REVISION = 'absent'; + +/** The admin API's code for a write whose `expectedRevision` no longer matches (409). */ +export const VERSION_CONFLICT_CODE = 'CONFIG_VERSION_CONFLICT'; + +/** A save refused before anything is written: one section holds more changes than one batch takes (`maxChangeOps`). */ +export const TOO_MANY_CHANGES_CODE = 'TOO_MANY_CHANGES'; + /** A stdio MCP server's process fields: only librechat.yaml sets them (D10). */ export const PROCESS_MCP_FIELDS: readonly string[] = ['command', 'args', 'env', 'cwd', 'stderr']; diff --git a/src/hooks/config/layers.ts b/src/hooks/config/layers.ts index 4e77426b..9d4e769c 100644 --- a/src/hooks/config/layers.ts +++ b/src/hooks/config/layers.ts @@ -196,6 +196,7 @@ export function useConfigLayers({ overridePaths: isEditingScope ? scopeOverridePaths : dbOverridePaths, listOverrideCounts, valueLayers, + targetRevision: isEditingScope ? scopeData?.revision : baseConfigData?.dbRevision, }; } diff --git a/src/hooks/config/review.test.tsx b/src/hooks/config/review.test.tsx index 62286478..9e389471 100644 --- a/src/hooks/config/review.test.tsx +++ b/src/hooks/config/review.test.tsx @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { configSchema } from 'librechat-data-provider'; import { act, renderHook } from '@testing-library/react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; @@ -8,6 +8,7 @@ import { extractSchemaTree, getSchemaPathSet } from '@/server/config'; import { useChangeReview } from './review'; import { useConfigEdits } from './edits'; import { flattenObject } from '@/utils'; +import { applyConfigChangesFn } from '@/server'; import { ConfigContractContext } from '@/contexts/ContractContext'; import { createServerContract } from '@/test/fixtures'; import { BUNDLED_CONFIG_CONTRACT } from '@/constants'; @@ -37,6 +38,7 @@ function renderPage( dbOverrides: t.ConfigRecord = {}, yamlConfig: t.ConfigRecord = config, contract: t.ConfigContract = BUNDLED_CONFIG_CONTRACT, + initialRevision?: string | null, ) { const client = new QueryClient(); const wrapper = ({ children }: { children: ReactNode }) => ( @@ -45,7 +47,7 @@ function renderPage( ); return renderHook( - () => { + ({ targetRevision }: { targetRevision?: string | null }) => { const edits = useConfigEdits({ isEditingScope: false, flatBaseline: flattenObject(config), @@ -64,6 +66,7 @@ function renderPage( overridePaths: new Set(Object.keys(flattenObject(dbOverrides))), valueLayers: null, baseActiveConfigValues: config, + targetRevision, }; const save = useConfigSave({ schemaTree: tree, schemaPathSet, layers, edits }); const review = useChangeReview({ @@ -75,7 +78,7 @@ function renderPage( }); return { edits, save, review }; }, - { wrapper }, + { wrapper, initialProps: { targetRevision: initialRevision } }, ); } @@ -192,3 +195,206 @@ describe('the change review over the save pipeline (WS1 × WS7)', () => { expect(result.current.edits.renameEdits).toEqual({}); }); }); + +describe('saving against the loaded revision (changesBatch)', () => { + const batch = createServerContract({ changesBatch: true, expectedRevision: true }); + const applyMock = vi.mocked(applyConfigChangesFn); + const saved: t.ApplyConfigChangesResult = { + applied: 1, + failed: [], + warnings: [], + issues: [], + skipped: [], + reported: [], + }; + const conflicted: t.ApplyConfigChangesResult = { + ...saved, + applied: 0, + conflict: true, + failed: [{ path: '', code: 'CONFIG_VERSION_CONFLICT', message: 'changed' }], + }; + const sentRevisions = () => + applyMock.mock.calls.map( + ([call]) => (call as { data: { expectedRevision?: string } }).data.expectedRevision, + ); + + /** A server whose target is stored at `revision`: a save named by another one writes nothing. */ + const storedAt = (revision: string) => + applyMock.mockImplementation(async (call) => { + const { expectedRevision } = (call as { data: { expectedRevision?: string } }).data; + return expectedRevision === undefined || expectedRevision === revision ? saved : conflicted; + }); + + beforeEach(() => { + applyMock.mockReset(); + }); + + async function editAndSave(result: { + current: ReturnType['result']['current']; + }) { + act(() => result.current.edits.handleFieldChange('balance.startBalance', 5)); + act(() => result.current.save.openReview()); + await act(() => result.current.review.confirm()); + } + + it('sends the revision the page loaded the target at', async () => { + applyMock.mockResolvedValue(saved); + const { result } = renderPage(yaml, {}, yaml, batch, 'base:4'); + await editAndSave(result); + expect(sentRevisions()).toEqual(['base:4']); + expect(result.current.edits.editedValues).toEqual({}); + }); + + it('names a target without a document as absent', async () => { + applyMock.mockResolvedValue(saved); + const { result } = renderPage(yaml, {}, yaml, batch, null); + await editAndSave(result); + expect(sentRevisions()).toEqual(['absent']); + }); + + it('sends no revision to a server without change batches', async () => { + applyMock.mockResolvedValue(saved); + const { result } = renderPage(yaml, {}, yaml, BUNDLED_CONFIG_CONTRACT, 'base:4'); + await editAndSave(result); + expect(sentRevisions()).toEqual([undefined]); + }); + + it('keeps the edits after a conflict, and merges them over the latest version', async () => { + applyMock.mockResolvedValueOnce(conflicted).mockResolvedValueOnce(saved); + const { result } = renderPage(yaml, {}, yaml, batch, 'base:4'); + await editAndSave(result); + expect(result.current.review.conflict).toBe(true); + expect(result.current.review.open).toBe(true); + expect(result.current.review.saveIssues).toEqual([]); + expect(result.current.edits.editedValues).toEqual({ 'balance.startBalance': 5 }); + await act(() => result.current.review.mergeLatest()); + expect(sentRevisions()).toEqual(['base:4', undefined]); + expect(result.current.review.conflict).toBe(false); + expect(result.current.edits.editedValues).toEqual({}); + }); + + it("does not save from the footer over the other admin's version after a conflict", async () => { + applyMock.mockResolvedValueOnce(conflicted).mockResolvedValue(saved); + const { result, rerender } = renderPage(yaml, {}, yaml, batch, 'base:4'); + await editAndSave(result); + rerender({ targetRevision: 'base:5' }); + await act(() => result.current.review.confirm()); + expect(sentRevisions()).toEqual(['base:4']); + expect(result.current.review.conflict).toBe(true); + expect(result.current.review.open).toBe(true); + expect(result.current.edits.editedValues).toEqual({ 'balance.startBalance': 5 }); + await act(() => result.current.review.mergeLatest()); + expect(sentRevisions()).toEqual(['base:4', undefined]); + expect(result.current.edits.editedValues).toEqual({}); + }); + + it('keeps the revision the edits were made against when the target is re-read meanwhile', async () => { + storedAt('base:5'); + const { result, rerender } = renderPage(yaml, {}, yaml, batch, 'base:4'); + act(() => result.current.edits.handleFieldChange('balance.startBalance', 5)); + rerender({ targetRevision: 'base:5' }); + act(() => result.current.save.openReview()); + rerender({ targetRevision: 'base:5' }); + await act(() => result.current.review.confirm()); + expect(sentRevisions()).toEqual(['base:4']); + expect(result.current.review.conflict).toBe(true); + expect(result.current.edits.editedValues).toEqual({ 'balance.startBalance': 5 }); + }); + + it('conflicts again from a review opened again, until the edits are merged', async () => { + storedAt('base:5'); + const { result, rerender } = renderPage(yaml, {}, yaml, batch, 'base:4'); + await editAndSave(result); + rerender({ targetRevision: 'base:5' }); + act(() => result.current.review.closeReview()); + expect(result.current.review.conflict).toBe(false); + expect(result.current.edits.editedValues).toEqual({ 'balance.startBalance': 5 }); + act(() => result.current.review.openReview()); + await act(() => result.current.review.confirm()); + expect(sentRevisions()).toEqual(['base:4', 'base:4']); + expect(result.current.review.conflict).toBe(true); + await act(() => result.current.review.mergeLatest()); + expect(sentRevisions()).toEqual(['base:4', 'base:4', undefined]); + expect(result.current.edits.editedValues).toEqual({}); + }); + + it('follows a re-read while no edits are pending', async () => { + storedAt('base:5'); + const { result, rerender } = renderPage(yaml, {}, yaml, batch, 'base:4'); + rerender({ targetRevision: 'base:5' }); + await editAndSave(result); + expect(sentRevisions()).toEqual(['base:5']); + expect(result.current.review.conflict).toBe(false); + expect(result.current.edits.editedValues).toEqual({}); + }); + + it('follows the loaded revision again once the edits are saved', async () => { + storedAt('base:4'); + const { result, rerender } = renderPage(yaml, {}, yaml, batch, 'base:4'); + await editAndSave(result); + storedAt('base:5'); + rerender({ targetRevision: 'base:5' }); + act(() => result.current.edits.handleFieldChange('balance.startBalance', 6)); + act(() => result.current.save.openReview()); + await act(() => result.current.review.confirm()); + expect(sentRevisions()).toEqual(['base:4', 'base:5']); + expect(result.current.edits.editedValues).toEqual({}); + }); + + it('follows the loaded revision again once the edits are discarded', async () => { + storedAt('base:5'); + const { result, rerender } = renderPage(yaml, {}, yaml, batch, 'base:4'); + act(() => result.current.edits.handleFieldChange('balance.startBalance', 5)); + rerender({ targetRevision: 'base:5' }); + act(() => result.current.edits.handleDiscard()); + await editAndSave(result); + expect(sentRevisions()).toEqual(['base:5']); + expect(result.current.edits.editedValues).toEqual({}); + }); + + it('says earlier batches stay saved when a later batch of a split save meets a conflict', async () => { + applyMock.mockResolvedValueOnce({ + ...saved, + applied: 0, + partiallyWritten: true, + failed: [{ path: 'balance.startBalance', code: 'CONFIG_VERSION_CONFLICT', message: 'x' }], + }); + const { result } = renderPage(yaml, {}, yaml, batch, 'base:4'); + await editAndSave(result); + expect(result.current.review.conflict).toBe(false); + expect(result.current.edits.saveError).toMatch(/^Save failed partway: the changes written/); + expect(result.current.edits.editedValues).toEqual({ 'balance.startBalance': 5 }); + }); + + it('says which section holds more changes than one save takes, and keeps the edits', async () => { + applyMock.mockResolvedValueOnce({ + ...saved, + applied: 0, + failed: [ + { + path: 'balance', + code: 'TOO_MANY_CHANGES', + message: 'has 1200 changes', + params: { count: 1200, limit: 1000 }, + }, + ], + }); + const { result } = renderPage(yaml, {}, yaml, batch, 'base:4'); + await editAndSave(result); + expect(result.current.edits.saveError).toBe( + 'Save failed and nothing was changed — Balance has 1,200 changes, more than the 1,000 LibreChat takes in one save: save some of them first, then the rest', + ); + expect(result.current.edits.editedValues).toEqual({ 'balance.startBalance': 5 }); + }); + + it('drops the edits and closes the review on Reload after a conflict', async () => { + applyMock.mockResolvedValueOnce(conflicted); + const { result } = renderPage(yaml, {}, yaml, batch, 'base:4'); + await editAndSave(result); + act(() => result.current.review.reloadLatest()); + expect(result.current.review.conflict).toBe(false); + expect(result.current.review.open).toBe(false); + expect(result.current.edits.editedValues).toEqual({}); + expect(applyMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/hooks/config/review.ts b/src/hooks/config/review.ts index e336a481..7d417b22 100644 --- a/src/hooks/config/review.ts +++ b/src/hooks/config/review.ts @@ -291,6 +291,13 @@ export function useChangeReview({ ); }, [model.count, localize]); + const { reloadLatest: reloadTarget, cancelReview: closeAfterReload } = save; + const reloadLatest = useCallback(() => { + discardAll(); + closeAfterReload(); + void reloadTarget(); + }, [discardAll, closeAfterReload, reloadTarget]); + const blocked = issues.some((issue) => issue.blocking); return { @@ -299,6 +306,9 @@ export function useChangeReview({ saving: save.saving, sessionExpired: save.sessionExpired, termsUrl: save.termsUrl, + conflict: save.conflict, + reloadLatest, + mergeLatest: save.saveOverLatest, saveIssues, title, scope: editingScope, diff --git a/src/hooks/config/save.ts b/src/hooks/config/save.ts index 60747e07..926d37f0 100644 --- a/src/hooks/config/save.ts +++ b/src/hooks/config/save.ts @@ -10,8 +10,10 @@ import { notifySuccess, getValueAtPath, termsRefusalUrl, + usesChangeBatch, planConfigChanges, mergedBackChanges, + toRevisionPrecondition, } from '@/utils'; import { ADMIN_API_KEY_FIELD, @@ -41,6 +43,23 @@ function hasWrites(plan: t.WritePlan): boolean { ); } +/** + * The revision the pending edits were made against: the target's loaded + * revision when the first of them was made, held while any is pending. A + * re-read meanwhile (the review opening, a window-focus refetch) does not + * move it, so a save over a version someone else saved since is a conflict + * rather than a silent overwrite. Once the edits are saved or dropped it + * follows the loaded revision again. + */ +function useEditRevision( + loaded: string | null | undefined, + pending: boolean, +): string | null | undefined { + const [held, setHeld] = useState({ pending, revision: loaded }); + if (held.pending !== pending) setHeld({ pending, revision: loaded }); + return pending && held.pending ? held.revision : loaded; +} + /** The admin session ended (signed out elsewhere or expired): the save never reached LibreChat. */ const SESSION_ERROR_RE = /no admin session token|session (has )?expired|unauthori[sz]ed|\b401\b/i; @@ -82,6 +101,7 @@ export function useConfigSave({ overridePaths, valueLayers, baseActiveConfigValues, + targetRevision, } = layers; const { editedValues, @@ -100,13 +120,25 @@ export function useConfigSave({ const [serverIssues, setServerIssues] = useState([]); const [sessionExpired, setSessionExpired] = useState(false); const [termsUrl, setTermsUrl] = useState(null); + const [conflict, setConflict] = useState(false); useEffect(() => setServerIssues([]), [editedValues, visibilityEdits]); + const hasPendingEdits = + Object.keys(editedValues).length > 0 || Object.keys(visibilityEdits).length > 0; + const editRevision = useEditRevision(targetRevision, hasPendingEdits); + + /** The revision the edits are saved against (`changesBatch`): see `useEditRevision`. */ + const expectedRevision = useMemo( + () => (usesChangeBatch(features) ? toRevisionPrecondition(editRevision) : undefined), + [features, editRevision], + ); + const clearEdits = useCallback(() => { resetEdits(); setConfirmSaveOpen(false); setSaving(false); + setConflict(false); setSaveError(null); }, [resetEdits, setSaveError]); @@ -152,13 +184,16 @@ export function useConfigSave({ /** * Applies ops through the server's validated apply; queries are refetched - * whatever the outcome. A call that fails outright (an ended session) is - * reported at once: the refetch, which retries, runs in the background. + * whatever the outcome, so after a conflict the page shows what is stored + * now. A call that fails outright (an ended session) is reported at once: + * the refetch, which retries, runs in the background. */ const applyChanges = useCallback( - async (target: t.WriteTarget, ops: t.ConfigChange[]) => { + async (target: t.WriteTarget, ops: t.ConfigChange[], revision?: string) => { try { - const result = await applyConfigChangesFn({ data: { target: toTargetRef(target), ops } }); + const result = await applyConfigChangesFn({ + data: { target: toTargetRef(target), ops, expectedRevision: revision }, + }); await invalidateTarget(target); return result; } catch (err) { @@ -176,6 +211,7 @@ export function useConfigSave({ const { summaries } = describeIssues(blocking, collections, localize); const titleKey = (() => { if (result.issues.length > 0) return 'com_config_validation_failed'; + if (result.partiallyWritten) return 'com_config_apply_partial'; return result.compensated === false ? 'com_config_apply_rollback_incomplete' : 'com_config_apply_failed'; @@ -391,92 +427,135 @@ export function useConfigSave({ localize, ]); - const handleConfirmSave = useCallback(async () => { - if (saving || unsafeKeys.length > 0) return; - if (clientIssues.length > 0) { - setSaveError(`${localize('com_config_validation_failed')} — ${issueSummaries.join('; ')}`); - return; - } - if (!hasWrites(plan)) { - clearEdits(); - if (savePlan.skipped.length > 0) - announceWrite(savePlan.skipped, 0, localize('com_config_saved')); - else notifyInfo(localize('com_config_nothing_to_save')); - return; - } - const mcpError = findMcpError(); - if (mcpError) { - setSaveError(mcpError); - notifyError(mcpError); - return; - } - - setSaving(true); - setSaveError(null); - setSessionExpired(false); - setTermsUrl(null); - try { - const result = await applyChanges(editTarget, changeSet.ops); - const terms = termsUrlOf(result); - if (terms !== null) { - const message = localize('com_config_terms_not_accepted'); - setSaving(false); - setTermsUrl(terms); - setSaveError(message); - notifyError(message); + /** + * Saves the pending edits to the edit target, against `revision` (see + * `ApplyConfigChangesRequest.expectedRevision`). A target saved elsewhere + * meanwhile is a `conflict`: nothing was written, and the review offers to + * reload or to merge. + */ + const saveAgainst = useCallback( + async (revision: string | undefined) => { + if (saving || unsafeKeys.length > 0) return; + if (clientIssues.length > 0) { + setSaveError(`${localize('com_config_validation_failed')} — ${issueSummaries.join('; ')}`); return; } - if (result.issues.length > 0 || result.failed.length > 0) { - const message = describeFailure(result, changeSet.collections); - setServerIssues([...result.issues, ...result.failed]); + if (!hasWrites(plan)) { + clearEdits(); + if (savePlan.skipped.length > 0) + announceWrite(savePlan.skipped, 0, localize('com_config_saved')); + else notifyInfo(localize('com_config_nothing_to_save')); + return; + } + const mcpError = findMcpError(); + if (mcpError) { + setSaveError(mcpError); + notifyError(mcpError); + return; + } + + setSaving(true); + setSaveError(null); + setSessionExpired(false); + setTermsUrl(null); + setConflict(false); + try { + const result = await applyChanges(editTarget, changeSet.ops, revision); + if (result.conflict) { + setSaving(false); + setConflict(true); + notifyWarning( + localize('com_config_conflict_title'), + localize('com_config_conflict_toast'), + ); + return; + } + const terms = termsUrlOf(result); + if (terms !== null) { + const message = localize('com_config_terms_not_accepted'); + setSaving(false); + setTermsUrl(terms); + setSaveError(message); + notifyError(message); + return; + } + if (result.issues.length > 0 || result.failed.length > 0) { + const message = describeFailure(result, changeSet.collections); + setServerIssues([...result.issues, ...result.failed]); + setSaving(false); + setSaveError(message); + notifyError(message); + return; + } + clearEdits(); + announceWrite( + [...changeSet.payload.skipped, ...result.skipped, ...result.reported], + result.applied, + localize('com_config_saved'), + ); + } catch (err) { + const raw = err instanceof Error ? err.message : String(err); + const expired = isSessionError(raw); + const message = expired ? localize('com_config_session_expired') : raw; setSaving(false); + setSessionExpired(expired); setSaveError(message); notifyError(message); - return; } - clearEdits(); - announceWrite( - [...changeSet.payload.skipped, ...result.skipped, ...result.reported], - result.applied, - localize('com_config_saved'), - ); - } catch (err) { - const raw = err instanceof Error ? err.message : String(err); - const expired = isSessionError(raw); - const message = expired ? localize('com_config_session_expired') : raw; - setSaving(false); - setSessionExpired(expired); - setSaveError(message); - notifyError(message); - } - }, [ - saving, - unsafeKeys, - clientIssues, - issueSummaries, - plan, - savePlan, - changeSet, - clearEdits, - announceWrite, - findMcpError, - applyChanges, - describeFailure, - editTarget, - localize, - setSaveError, - ]); + }, + [ + saving, + unsafeKeys, + clientIssues, + issueSummaries, + plan, + savePlan, + changeSet, + clearEdits, + announceWrite, + findMcpError, + applyChanges, + describeFailure, + editTarget, + localize, + setSaveError, + ], + ); + + /** + * Save from the review, against the revision the edits were made against. + * While the conflict notice is shown only Reload or Merge resolves it: Save + * would send that revision again and meet the same conflict. Closing the + * review ends the notice; the review opened again compares the edits with + * what is stored now, and its Save conflicts again until the admin reloads + * or merges. + */ + const handleConfirmSave = useCallback(async () => { + if (conflict) return; + await saveAgainst(expectedRevision); + }, [conflict, saveAgainst, expectedRevision]); + + /** Merges after a conflict: the server plans the edits over the target as stored now. */ + const saveOverLatest = useCallback(() => saveAgainst(undefined), [saveAgainst]); + + const reloadLatest = useCallback(async () => { + setConflict(false); + setSaveError(null); + await invalidateTarget(editTarget); + }, [invalidateTarget, editTarget, setSaveError]); /** * Opening the review re-reads the target, so rows saved by someone else - * meanwhile are flagged as drifted (SV-8). Blocking problems are listed by - * the review from `issues`. + * meanwhile are flagged as drifted (SV-8); the revision Save sends stays the + * one the edits were made against. Blocking problems are listed by the + * review from `issues`. */ const openReview = useCallback(() => { void invalidateTarget(editTarget); setSaveError(null); setSessionExpired(false); setTermsUrl(null); + setConflict(false); setConfirmSaveOpen(true); }, [invalidateTarget, editTarget, setSaveError]); @@ -485,6 +564,7 @@ export function useConfigSave({ setSaveError(null); setSessionExpired(false); setTermsUrl(null); + setConflict(false); }, [setSaveError]); const closeReview = useCallback(() => setConfirmSaveOpen(false), []); @@ -502,6 +582,9 @@ export function useConfigSave({ fieldErrors, issueSummaries, handleConfirmSave, + conflict, + reloadLatest, + saveOverLatest, openReview, cancelReview, closeReview, diff --git a/src/locales/en/translation.json b/src/locales/en/translation.json index a75f456a..3a7a0c4f 100644 --- a/src/locales/en/translation.json +++ b/src/locales/en/translation.json @@ -592,6 +592,11 @@ "com_config_list_owned_scope": "This profile has its own copy of this list, replacing the one from Base: changes to the Base list won't reach these users until you revert it.", "com_config_list_revert": "Revert list to librechat.yaml", "com_config_list_revert_scope": "Revert list to Base", + "com_config_conflict_title": "Changed elsewhere", + "com_config_conflict_toast": "Nothing was saved: someone else saved this configuration while you were editing. Reload or merge in the review.", + "com_config_conflict_body": "{{target}} was saved elsewhere while you were editing, so nothing was saved. The review now compares your edits with what is stored, and marks each setting that changed after you edited it. Reload to drop your edits, or merge to save them over the latest version.", + "com_config_conflict_reload": "Reload", + "com_config_conflict_merge": "Merge and save", "com_config_session_expired": "Your session ended, so nothing was saved. Your unsaved changes are still here: sign in again, then save.", "com_config_session_sign_in": "Sign in again (opens a new tab)", "com_config_terms_not_accepted": "LibreChat refused the save because your account hasn't accepted its terms of service, which it now requires of everyone, admins included. Nothing was saved and your changes are still here: open LibreChat, accept the terms, then save again.", @@ -659,6 +664,7 @@ "com_config_api_error_principal_not_found": "The role, group or user this profile is for doesn't exist (it may have been deleted or renamed).", "com_config_api_error_process_mcp_server_override": "This MCP server runs a local process defined in librechat.yaml, so LibreChat applies no overrides to it.", "com_config_apply_failed": "Save failed and nothing was changed", + "com_config_apply_partial": "Save failed partway: the changes written before the failure stay saved; reload to see what is stored", "com_config_apply_not_effective_one": "Saved, but {{count}} change did not take effect", "com_config_apply_not_effective_other": "Saved, but {{count}} changes did not take effect", "com_config_apply_rollback_incomplete": "Save failed and could not be fully rolled back; reload to see what is stored", @@ -708,6 +714,7 @@ "com_config_issue_custom_format": "must be only host:port, without http:// or a path", "com_config_issue_custom_missing_port": "must include a port, for example localhost:11434", "com_config_issue_custom_public_ip": "is a public IP address: enter a private address, or a hostname that resolves to one", + "com_config_issue_too_many_changes": "has {{count}} changes, more than the {{limit}} LibreChat takes in one save: save some of them first, then the rest", "com_config_issue_invalid_value": "is not a valid value", "com_config_issue_wrong_type": "has the wrong type of value", "com_config_issue_invalid_option": "is not one of the allowed values", diff --git a/src/server/config/base.test.ts b/src/server/config/base.test.ts new file mode 100644 index 00000000..7309a4ec --- /dev/null +++ b/src/server/config/base.test.ts @@ -0,0 +1,68 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { PrincipalType } from 'librechat-data-provider'; + +const apiFetchMock = vi.fn(); + +vi.mock('../utils/api', () => ({ + apiFetch: (path: string, init?: RequestInit) => apiFetchMock(path, init), +})); + +vi.mock('@tanstack/react-start', () => ({ + createServerFn: () => ({ + inputValidator: () => ({ handler: (fn: (...args: never[]) => unknown) => fn }), + handler: (fn: (...args: never[]) => unknown) => fn, + }), +})); + +vi.mock('@tanstack/react-query', () => ({ queryOptions: (opts: unknown) => opts })); + +import { getResolvedConfigFn } from '../scopes/resolved'; +import { getBaseConfigFn } from './base'; + +function reply(status: number, body?: object): Response { + return new Response(body ? JSON.stringify(body) : null, { status }); +} + +const yaml = { balance: { startBalance: 1000 } }; + +/** Answers the base reads: the merged base, librechat.yaml alone, and the base document. */ +function serveBase(doc: Response) { + apiFetchMock.mockImplementation((path: string) => { + if (path.startsWith('/api/admin/config/base')) return reply(200, { config: yaml }); + return doc; + }); +} + +beforeEach(() => { + apiFetchMock.mockReset(); +}); + +describe('the revision of the stored document (expectedRevision)', () => { + it('reads the base document revision', async () => { + serveBase(reply(200, { config: { overrides: {}, tombstones: [], revision: '6adb:7' } })); + const data = await getBaseConfigFn(); + expect(data.dbRevision).toBe('6adb:7'); + }); + + it('reads no base document as a null revision', async () => { + serveBase(reply(404, { error: 'Config not found' })); + const data = await getBaseConfigFn(); + expect(data).toMatchObject({ dbOverrides: undefined, dbRevision: null }); + }); + + it('leaves the revision out where the server reports none', async () => { + serveBase(reply(200, { config: { overrides: {}, tombstones: [] } })); + const data = await getBaseConfigFn(); + expect(data).not.toHaveProperty('dbRevision'); + }); + + it("reads a profile document's revision", async () => { + apiFetchMock.mockResolvedValue( + reply(200, { config: { overrides: { balance: { startBalance: 5 } }, revision: 'eng:3' } }), + ); + const data = await getResolvedConfigFn({ + data: { principalType: PrincipalType.GROUP, principalId: 'eng' }, + }); + expect(data).toMatchObject({ revision: 'eng:3', changedPaths: ['balance.startBalance'] }); + }); +}); diff --git a/src/server/config/base.ts b/src/server/config/base.ts index 802d7f0e..8faa8ef8 100644 --- a/src/server/config/base.ts +++ b/src/server/config/base.ts @@ -42,14 +42,22 @@ function readSchemaDefaults(): t.FlatConfigMap { } } +/** + * The base admin override document. Its revision is `null` when there is + * none (a server that reports revisions answers 404 for no document too), + * and left out when the server reports none. + */ async function readOverrideDoc( response: Response, -): Promise> { +): Promise> { + if (response.status === 404) + return { dbOverrides: undefined, dbTombstones: [], dbRevision: null }; if (!response.ok) return { dbOverrides: undefined, dbTombstones: [] }; const { config } = (await response.json()) as t.AdminConfigDocResponse; return { dbOverrides: (config.overrides ?? {}) as t.ConfigRecord, dbTombstones: config.tombstones ?? [], + ...(typeof config.revision === 'string' && { dbRevision: config.revision }), }; } @@ -61,7 +69,7 @@ async function readOverrideDoc( * - `yamlPaths`: what the raw librechat.yaml itself sets. Values LibreChat * synthesizes at startup appear in both trees above but not here, so they * are not counted as configured (EV-2); - * - `dbOverrides`/`dbTombstones`: the base admin override document; + * - `dbOverrides`/`dbTombstones`/`dbRevision`: the base admin override document; * - `schemaDefaults`: flattened schema defaults. */ export const getBaseConfigFn = createServerFn({ method: 'GET' }).handler( diff --git a/src/server/config/save.test.ts b/src/server/config/save.test.ts index f8c13fae..a2d929d8 100644 --- a/src/server/config/save.test.ts +++ b/src/server/config/save.test.ts @@ -355,6 +355,26 @@ describe('applyConfigChanges — clear/reset and verification (SV-4, SV-5, SV-7) expect(api.current!.tombstones()).toEqual(['balance.refillAmount']); }); + it('leaves no {} behind when a union switches back to its librechat.yaml key', async () => { + api.current!.seed( + 'role/__base__', + { memory: { agent: { provider: 'openAI', model: 'gpt-4o-mini' } } }, + ['memory.agent.id'], + ); + const result = await applyConfigChanges(base, [ + { op: 'reset', path: 'memory.agent.provider' }, + { op: 'reset', path: 'memory.agent.model' }, + { op: 'untombstone', path: 'memory.agent.id' }, + ]); + expect(result).toMatchObject({ issues: [], failed: [], skipped: [] }); + expect(api.current!.writes().map((r) => r.path)).toEqual([ + expect.stringMatching(/fieldPath=memory\.agent\.id$/), + expect.stringMatching(/fieldPath=memory$/), + ]); + expect(api.current!.stored()).toEqual({}); + expect(api.current!.tombstones()).toEqual([]); + }); + it('reports a value LibreChat dropped as not stored', async () => { api.current!.stripSection('speech'); const result = await applyConfigChanges(base, [ @@ -410,6 +430,33 @@ describe('applyConfigChanges — profiles (SV-9)', () => { expect(api.current!.tombstones('group/eng')).toEqual(['mcpServers.cloudflare']); }); + it('keeps an edit inside a server shown again in the same save', async () => { + const url = 'https://eng-cloudflare.example.com/mcp'; + api.current!.seed('group/eng', {}, ['mcpServers.cloudflare']); + const result = await applyConfigChanges(scope, [ + { op: 'untombstone', path: 'mcpServers.cloudflare' }, + { op: 'set', path: 'mcpServers.cloudflare.url', value: url }, + ]); + expect(result).toMatchObject({ applied: 2, issues: [], failed: [], skipped: [] }); + expect(api.current!.writes().map((r) => r.method)).toEqual(['DELETE', 'PATCH']); + expect(api.current!.stored('group/eng')).toEqual({ mcpServers: { cloudflare: { url } } }); + expect(api.current!.tombstones('group/eng')).toEqual([]); + }); + + it('keeps a server hidden when a field inside it is edited in the same save', async () => { + api.current!.seed('group/eng', { + mcpServers: { cloudflare: { url: 'https://eng-cloudflare.example.com/mcp' } }, + }); + const result = await applyConfigChanges(scope, [ + { op: 'set', path: 'mcpServers.cloudflare.url', value: 'https://other.example.com/mcp' }, + { op: 'tombstone', path: 'mcpServers.cloudflare' }, + ]); + expect(result).toMatchObject({ applied: 1, issues: [], failed: [], skipped: [] }); + expect(api.current!.writes().map((r) => r.method)).toEqual(['POST']); + expect(api.current!.stored('group/eng')).toEqual({ mcpServers: {} }); + expect(api.current!.tombstones('group/eng')).toEqual(['mcpServers.cloudflare']); + }); + it('diffs a profile entry edit against the inherited base entry', async () => { const moonshot = customEntries[0]; api.current!.seed('group/eng', {}); @@ -537,12 +584,22 @@ describe('applyConfigChanges — write reports, structured issues and codes', () const result = await applyConfigChanges(base, [ { op: 'set', path: 'balance.startBalance', value: 5 }, { op: 'set', path: 'balance.refillAmount', value: 6 }, + { op: 'set', path: 'interface.modelSelect', value: false }, ]); - expect(result).toMatchObject({ issues: [], failed: [], applied: 1 }); + expect(result).toMatchObject({ issues: [], failed: [], applied: 2 }); expect(result.skipped).toEqual([{ fieldPath: 'balance.refillAmount', reason: 'yamlOnly' }]); expect(result.reported).toEqual([{ fieldPath: 'interface.prompts', reason: 'permission' }]); }); + it('does not report a part of a section the save did not change', async () => { + api.current!.reportMeta({ features: { writeReport: true } }); + api.current!.reportWrites(() => ({ applied: [], skipped: [skippedPart] })); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'balance.startBalance', value: 5 }, + ]); + expect(result).toMatchObject({ issues: [], failed: [], applied: 1, skipped: [], reported: [] }); + }); + it('leaves the report alone on a server without writeReport', async () => { api.current!.reportWrites(() => ({ skipped: [skippedWhole] })); const result = await applyConfigChanges(base, [ @@ -791,3 +848,471 @@ describe('applyConfigChanges — write reports, structured issues and codes', () ]); }); }); + +describe('applyConfigChanges — change batches (changesBatch, expectedRevision)', () => { + const batchMeta = (limits: Partial = {}): t.AdminConfigMetaBody => ({ + features: { + changesBatch: true, + expectedRevision: true, + prunesEmptyParents: true, + awaitedInvalidation: true, + }, + limits: { maxChangeOps: 1000, ...limits }, + }); + const posts = () => api.current!.writes().filter((request) => request.path.endsWith('/changes')); + + beforeEach(() => { + api.current!.reportMeta(batchMeta()); + }); + + it('writes every change as one batch named by the revision it was planned against', async () => { + api.current!.seed('role/__base__', { balance: { refillAmount: 7 } }); + const revision = api.current!.revision(); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'balance.startBalance', value: 5 }, + { op: 'reset', path: 'balance.refillAmount' }, + { op: 'tombstone', path: 'mcpServers.cloudflare' }, + ]); + expect(result).toMatchObject({ applied: 3, issues: [], failed: [], skipped: [] }); + expect(api.current!.writes()).toEqual([ + { + method: 'POST', + path: '/api/admin/config/role/__base__/changes', + body: { + ops: [ + { op: 'tombstone', fieldPath: 'mcpServers.cloudflare' }, + { op: 'set', fieldPath: 'balance.startBalance', value: 5 }, + { op: 'unset', fieldPath: 'balance.refillAmount' }, + ], + priority: 0, + expectedRevision: revision, + }, + }, + ]); + expect(api.current!.stored()).toEqual({ balance: { startBalance: 5 } }); + expect(api.current!.tombstones()).toEqual(['mcpServers.cloudflare']); + }); + + it('creates the base document only while it has none', async () => { + await applyConfigChanges(base, [{ op: 'set', path: 'balance.startBalance', value: 5 }]); + expect(posts()[0].body?.expectedRevision).toBe('absent'); + }); + + it('sends a reset as planned, without widening it: the server prunes emptied parents', async () => { + api.current!.seed('role/__base__', { balance: { startBalance: 5 } }); + await applyConfigChanges(base, [{ op: 'reset', path: 'balance.startBalance' }]); + expect(posts()[0].body?.ops).toEqual([{ op: 'unset', fieldPath: 'balance.startBalance' }]); + expect(api.current!.stored()).toEqual({}); + }); + + it('writes nothing and reports a conflict when the target changed since the edits were loaded', async () => { + api.current!.seed('role/__base__', { balance: { startBalance: 5 } }); + const loaded = api.current!.revision(); + api.current!.seed('role/__base__', { balance: { startBalance: 6 } }); + const result = await applyConfigChanges( + base, + [{ op: 'set', path: 'balance.refillAmount', value: 9 }], + loaded!, + ); + expect(result).toMatchObject({ conflict: true, applied: 0 }); + expect(result.failed.map((issue) => issue.code)).toEqual(['CONFIG_VERSION_CONFLICT']); + expect(api.current!.writes()).toEqual([]); + }); + + it('saves against the revision the edits were loaded at', async () => { + api.current!.seed('role/__base__', { balance: { startBalance: 5 } }); + const loaded = api.current!.revision(); + const result = await applyConfigChanges( + base, + [{ op: 'set', path: 'balance.refillAmount', value: 9 }], + loaded!, + ); + expect(result).toMatchObject({ applied: 1, failed: [] }); + expect(result.conflict).toBeUndefined(); + expect(posts()[0].body?.expectedRevision).toBe(loaded); + }); + + it('reports a conflict when another save lands between the read and the batch (409)', async () => { + api.current!.seed('role/__base__', { balance: { startBalance: 5 } }); + const loaded = api.current!.revision(); + let raced = false; + api.current!.setOverride((request) => { + if (!request.path.endsWith('/changes') || raced) return undefined; + raced = true; + api.current!.seed('role/__base__', { balance: { startBalance: 8 } }); + return undefined; + }); + const result = await applyConfigChanges( + base, + [{ op: 'set', path: 'balance.refillAmount', value: 9 }], + loaded!, + ); + expect(result).toMatchObject({ conflict: true, applied: 0 }); + expect(posts()).toHaveLength(1); + expect(api.current!.stored()).toEqual({ balance: { startBalance: 8 } }); + }); + + it('plans once more when a save without a loaded revision (an import) loses that race', async () => { + api.current!.seed('role/__base__', { balance: { startBalance: 5 } }); + let raced = false; + api.current!.setOverride((request) => { + if (!request.path.endsWith('/changes') || raced) return undefined; + raced = true; + api.current!.seed('role/__base__', { balance: { startBalance: 8 } }); + return undefined; + }); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'balance.refillAmount', value: 9 }, + ]); + expect(result).toMatchObject({ applied: 1, failed: [] }); + expect(posts()).toHaveLength(2); + expect(api.current!.stored()).toEqual({ balance: { startBalance: 8, refillAmount: 9 } }); + }); + + it('refuses a batch whole and sends nothing to roll it back', async () => { + api.current!.seed('role/__base__', { balance: { startBalance: 5 } }); + api.current!.setOverride((request) => + request.path.endsWith('/changes') + ? { + status: 400, + body: { + error: 'Config change cannot be applied to the stored config', + code: 'CONFIG_PATH_INVALID', + opIndex: 1, + }, + } + : undefined, + ); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'balance.startBalance', value: 6 }, + { op: 'set', path: 'speech.speechTab.advancedMode', value: true }, + ]); + expect(result.failed).toEqual([ + { + path: 'speech.speechTab.advancedMode', + code: 'WRITE_FAILED', + message: 'Config change cannot be applied to the stored config', + }, + ]); + expect(result.compensated).toBeUndefined(); + expect(api.current!.writes()).toHaveLength(1); + expect(api.current!.stored()).toEqual({ balance: { startBalance: 5 } }); + }); + + it('reports what the batch did not store, counting only whole changes as not applied', async () => { + api.current!.stripSection('speech'); + api.current!.reportWrites(() => ({ + skipped: [ + { + opIndex: 1, + segments: ['speech', 'speechTab', 'advancedMode'], + reason: 'base_only_section', + }, + { opIndex: 2, segments: ['interface', 'prompts'], reason: 'interface_permission' }, + ], + })); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'balance.startBalance', value: 6 }, + { op: 'set', path: 'speech.speechTab.advancedMode', value: true }, + { op: 'set', path: 'interface', value: { prompts: false, modelSelect: true } }, + ]); + expect(result.applied).toBe(2); + expect(result.skipped).toEqual([ + { fieldPath: 'speech.speechTab.advancedMode', reason: 'baseOnly' }, + ]); + expect(result.reported).toEqual([{ fieldPath: 'interface.prompts', reason: 'permission' }]); + }); + + it('reports a change the runtime merge does not use', async () => { + api.current!.reportWrites(() => ({ + ignored: [ + { + opIndex: 0, + segments: ['mcpServers', 'cloudflare', 'title'], + reason: 'process_mcp_server', + }, + ], + })); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'mcpServers.cloudflare.title', value: 'Cloudflare' }, + ]); + expect(result.applied).toBe(0); + expect(result.skipped).toEqual([ + { fieldPath: 'mcpServers.cloudflare.title', reason: 'processServer' }, + ]); + }); + + describe('a dropped credential or a partial report, for the changes a batch made', () => { + const dropped = { + path: 'endpoints.custom.Moonshot.apiKey', + segments: ['endpoints', 'custom', 'Moonshot', 'apiKey'], + reason: 'secret_rebinding', + }; + const noted = { fieldPath: 'endpoints.custom.Moonshot.apiKey', reason: 'secretRebinding' }; + const [moonshot, gpt] = customEntries; + const editMoonshot = upsert( + 'Moonshot', + 'Moonshot', + { ...moonshot, titleModel: 'kimi-k2' }, + moonshot, + ); + + it('is noted apart from the count when the batch changed its entry', async () => { + api.current!.reportWrites(() => ({ ignored: [dropped] })); + const result = await applyConfigChanges(base, [ + editMoonshot, + { op: 'set', path: 'balance.startBalance', value: 5 }, + ]); + expect(result).toMatchObject({ applied: 2, failed: [], skipped: [] }); + expect(result.reported).toEqual([noted]); + }); + + it('is not repeated by an unrelated save, or one of another entry the batch sends whole', async () => { + api.current!.reportWrites(() => ({ ignored: [dropped] })); + const unrelated = await applyConfigChanges(base, [ + { op: 'set', path: 'endpoints.openAI.titleModel', value: 'gpt-5-nano' }, + ]); + expect(unrelated).toMatchObject({ applied: 1, failed: [], skipped: [], reported: [] }); + const other = await applyConfigChanges(base, [ + upsert('GPT', 'GPT', { ...gpt, titleModel: 'gpt-4o-mini' }, gpt), + ]); + expect(posts().at(-1)?.body?.ops).toEqual([ + expect.objectContaining({ op: 'set', fieldPath: 'endpoints.custom' }), + ]); + expect(other).toMatchObject({ applied: 1, failed: [], skipped: [], reported: [] }); + expect((await applyConfigChanges(base, [editMoonshot])).reported).toEqual([noted]); + }); + + it('is noted once however many batches repeat it', async () => { + api.current!.reportMeta(batchMeta({ maxChangeOps: 1 })); + api.current!.reportWrites(() => ({ ignored: [dropped] })); + const result = await applyConfigChanges(base, [ + editMoonshot, + { op: 'set', path: 'balance.startBalance', value: 5 }, + ]); + expect(posts()).toHaveLength(2); + expect(result).toMatchObject({ applied: 2, failed: [], skipped: [] }); + expect(result.reported).toEqual([noted]); + }); + + it('keeps a part reported inside the entry the batch changed, not inside another', async () => { + const part = (name: string) => ({ + opIndex: 0, + segments: ['endpoints', 'custom', name, 'headers'], + reason: 'base_only_path', + }); + api.current!.reportWrites(() => ({ skipped: [part('GPT'), part('Moonshot')] })); + const result = await applyConfigChanges(base, [editMoonshot]); + expect(result).toMatchObject({ applied: 1, failed: [], skipped: [] }); + expect(result.reported).toEqual([ + { fieldPath: 'endpoints.custom.Moonshot.headers', reason: 'baseOnly' }, + ]); + }); + }); + + it("splits a plan above maxChangeOps into batches, each named by the previous one's revision", async () => { + api.current!.reportMeta(batchMeta({ maxChangeOps: 2 })); + await applyConfigChanges(base, [ + { op: 'set', path: 'balance.startBalance', value: 5 }, + { op: 'set', path: 'memory.tokenLimit', value: 6 }, + { op: 'set', path: 'interface.presets', value: false }, + ]); + expect(posts().map((request) => request.body?.ops?.length)).toEqual([2, 1]); + expect(posts().map((request) => request.body?.expectedRevision)).toEqual([ + 'absent', + 'role-__base__:1', + ]); + expect(api.current!.stored()).toEqual({ + balance: { startBalance: 5 }, + memory: { tokenLimit: 6 }, + interface: { presets: false }, + }); + }); + + describe('a split save, by sections', () => { + const switchAgent: t.ConfigChange[] = [ + { op: 'set', path: 'memory.agent.provider', value: 'openAI' }, + { op: 'set', path: 'memory.agent.model', value: 'gpt-4o-mini' }, + { op: 'tombstone', path: 'memory.agent.id' }, + ]; + + beforeEach(() => { + api.current = createFakeAdminApi({ ...yaml, memory: { agent: { id: 'agent_yaml' } } }); + api.current.judgeEndStates(); + }); + + it("keeps a section's changes in one batch, so each batch's end state is valid", async () => { + api.current!.reportMeta(batchMeta({ maxChangeOps: 3 })); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'balance.startBalance', value: 5 }, + ...switchAgent, + ]); + expect(result).toMatchObject({ applied: 4, failed: [], issues: [], skipped: [] }); + expect(posts().map((request) => request.body?.ops)).toEqual([ + [ + { op: 'tombstone', fieldPath: 'memory.agent.id' }, + { op: 'set', fieldPath: 'memory.agent.provider', value: 'openAI' }, + { op: 'set', fieldPath: 'memory.agent.model', value: 'gpt-4o-mini' }, + ], + [{ op: 'set', fieldPath: 'balance.startBalance', value: 5 }], + ]); + expect(api.current!.stored()).toEqual({ + balance: { startBalance: 5 }, + memory: { agent: { provider: 'openAI', model: 'gpt-4o-mini' } }, + }); + expect(api.current!.tombstones()).toEqual(['memory.agent.id']); + }); + + it('judges a batch that hides the agent ID alone, the end state a split must avoid', async () => { + const response = await api.current!.fetch('/api/admin/config/role/__base__/changes', { + method: 'POST', + body: JSON.stringify({ ops: [{ op: 'tombstone', fieldPath: 'memory.agent.id' }] }), + }); + expect(response.status).toBe(400); + expect(api.current!.stored()).toBeUndefined(); + }); + + it('refuses a section with more changes than one batch takes, writing nothing', async () => { + api.current!.reportMeta(batchMeta({ maxChangeOps: 2 })); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'balance.startBalance', value: 5 }, + ...switchAgent, + ]); + expect(result).toMatchObject({ applied: 0, issues: [] }); + expect(result.partiallyWritten).toBeUndefined(); + expect(result.failed).toEqual([ + { + path: 'memory', + code: 'TOO_MANY_CHANGES', + message: + 'has 3 changes, more than the 2 LibreChat takes in one save: save some of them first, then the rest', + params: { count: 3, limit: 2 }, + }, + ]); + expect(api.current!.writes()).toEqual([]); + }); + }); + + it('reports a split save that failed after an earlier batch was written', async () => { + api.current!.reportMeta(batchMeta({ maxChangeOps: 1 })); + api.current!.setOverride((request) => + request.path.endsWith('/changes') && request.body?.expectedRevision !== 'absent' + ? { status: 500, body: { error: 'Failed to apply config changes' } } + : undefined, + ); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'balance.startBalance', value: 5 }, + { op: 'set', path: 'memory.tokenLimit', value: 6 }, + ]); + expect(result).toMatchObject({ partiallyWritten: true, applied: 0 }); + expect(result.failed.map((issue) => issue.message)).toEqual(['Failed to apply config changes']); + expect(api.current!.stored()).toEqual({ balance: { startBalance: 5 } }); + }); + + it('reports a conflict on a later batch as a partial write, not as nothing saved', async () => { + api.current!.reportMeta(batchMeta({ maxChangeOps: 1 })); + const loaded = api.current!.revision(); + api.current!.setOverride((request) => + request.path.endsWith('/changes') && request.body?.expectedRevision !== 'absent' + ? { + status: 409, + body: { + error: 'Config was modified by another request', + code: 'CONFIG_VERSION_CONFLICT', + }, + } + : undefined, + ); + const result = await applyConfigChanges( + base, + [ + { op: 'set', path: 'balance.startBalance', value: 5 }, + { op: 'set', path: 'memory.tokenLimit', value: 6 }, + ], + loaded ?? 'absent', + ); + expect(result).toMatchObject({ partiallyWritten: true, applied: 0 }); + expect(result.conflict).toBeUndefined(); + expect(result.failed.map((issue) => issue.code)).toEqual(['CONFIG_VERSION_CONFLICT']); + expect(api.current!.stored()).toEqual({ balance: { startBalance: 5 } }); + }); + + it('keeps an edit inside a server shown again in the same batch', async () => { + const url = 'https://eng-cloudflare.example.com/mcp'; + const scope: t.WriteTargetRef = { + type: 'scope', + scope: { principalType: 'role' as t.ConfigScope['principalType'], principalId: 'USER' }, + }; + api.current!.seed('role/USER', {}, ['mcpServers.cloudflare']); + const result = await applyConfigChanges( + scope, + [ + { op: 'untombstone', path: 'mcpServers.cloudflare' }, + { op: 'set', path: 'mcpServers.cloudflare.url', value: url }, + ], + api.current!.revision('role/USER')!, + ); + expect(result).toMatchObject({ applied: 2, issues: [], failed: [], skipped: [] }); + expect(posts()).toHaveLength(1); + expect(posts()[0].body?.ops).toEqual([ + { op: 'unset', fieldPath: 'mcpServers.cloudflare' }, + { op: 'set', fieldPath: 'mcpServers.cloudflare.url', value: url }, + ]); + expect(api.current!.stored('role/USER')).toEqual({ mcpServers: { cloudflare: { url } } }); + expect(api.current!.tombstones('role/USER')).toEqual([]); + }); + + it('keeps a server hidden when a field inside it is edited in the same batch', async () => { + api.current!.seed('role/USER', { + mcpServers: { cloudflare: { url: 'https://user-cloudflare.example.com/mcp' } }, + }); + const result = await applyConfigChanges( + { + type: 'scope', + scope: { principalType: 'role' as t.ConfigScope['principalType'], principalId: 'USER' }, + }, + [ + { op: 'set', path: 'mcpServers.cloudflare.url', value: 'https://other.example.com/mcp' }, + { op: 'tombstone', path: 'mcpServers.cloudflare' }, + ], + api.current!.revision('role/USER')!, + ); + expect(result).toMatchObject({ applied: 1, issues: [], failed: [], skipped: [] }); + expect(posts()[0].body?.ops).toEqual([{ op: 'tombstone', fieldPath: 'mcpServers.cloudflare' }]); + expect(api.current!.stored('role/USER')).toEqual({}); + expect(api.current!.tombstones('role/USER')).toEqual(['mcpServers.cloudflare']); + }); + + it('keeps separate field writes on a server without changesBatch', async () => { + api.current!.reportMeta({ features: { expectedRevision: true, prunesEmptyParents: true } }); + api.current!.seed('role/__base__', { balance: { startBalance: 5 } }); + await applyConfigChanges(base, [ + { op: 'set', path: 'balance.refillAmount', value: 6 }, + { op: 'reset', path: 'balance.startBalance' }, + ]); + expect(api.current!.writes().map((request) => request.method)).toEqual(['PATCH', 'DELETE']); + expect(api.current!.writes()[1].path).toContain('fieldPath=balance.startBalance'); + }); + + it('writes a union switch as one batch, validated on its end state', async () => { + api.current = createFakeAdminApi({ ...yaml, memory: { agent: { id: 'agent_yaml' } } }); + api.current.reportMeta(batchMeta()); + const result = await applyConfigChanges(base, [ + { op: 'set', path: 'memory.agent.provider', value: 'openAI' }, + { op: 'set', path: 'memory.agent.model', value: 'gpt-4o-mini' }, + { op: 'tombstone', path: 'memory.agent.id' }, + ]); + expect(result).toMatchObject({ failed: [], issues: [] }); + expect(posts()).toHaveLength(1); + expect(api.current.tombstones()).toEqual(['memory.agent.id']); + }); + + it('blocks hiding a librechat.yaml value alone when the section is left invalid', async () => { + api.current = createFakeAdminApi({ ...yaml, memory: { agent: { id: 'agent_yaml' } } }); + api.current.reportMeta(batchMeta()); + const result = await applyConfigChanges(base, [{ op: 'tombstone', path: 'memory.agent.id' }]); + expect(result.issues.length).toBeGreaterThan(0); + expect(result.issues.every((issue) => issue.path.startsWith('memory.agent'))).toBe(true); + expect(api.current.writes()).toEqual([]); + }); +}); diff --git a/src/server/config/save.ts b/src/server/config/save.ts index c6c7122c..cc642096 100644 --- a/src/server/config/save.ts +++ b/src/server/config/save.ts @@ -4,6 +4,7 @@ import { PrincipalType } from 'librechat-data-provider'; import { SystemCapabilities } from '@librechat/data-schemas/capabilities'; import type * as t from '@/types'; import { + toBatchOps, verifyWrite, getEntryKey, reportedPath, @@ -11,19 +12,25 @@ import { isConfigRecord, getValueAtPath, readWriteReport, + readBatchReport, + usesChangeBatch, createPathIndex, describeApiError, planConfigChanges, describeIssueCode, + pathIndexOverlaps, getKeyedCollection, getWriteSkipReason, classifyReportedIssue, + toRevisionPrecondition, stripSecretPreviewValues, } from '@/utils'; import { getWriteRules, getConfigSection, MERGED_BACK_REASONS, + VERSION_CONFLICT_CODE, + TOO_MANY_CHANGES_CODE, TERMS_NOT_ACCEPTED_CODE, TERMS_NOT_ACCEPTED_ISSUE, } from '@/constants'; @@ -73,22 +80,24 @@ async function readJson(response: Response): Promise { } /** - * The target's stored overrides (redacted by the admin API) and tombstones; - * none when base has no config yet. A profile whose document is gone is - * refused (PR-3): the admin API would otherwise silently recreate it, active - * and at the default priority. + * The target's stored overrides (redacted by the admin API), tombstones and + * revision (`expectedRevision`); none, and a `null` revision, when base has + * no config yet. A profile whose document is gone is refused (PR-3): the + * admin API would otherwise silently recreate it, active and at the default + * priority. */ async function readStoredLayer( target: t.WriteTargetRef, -): Promise> { +): Promise> { const response = await apiFetch(targetConfigUrl(target)); if (response.status === 404 && target.type === 'scope') throw new Error(MISSING_SCOPE_MESSAGE); - if (response.status === 404) return { stored: {}, tombstones: [] }; + if (response.status === 404) return { stored: {}, tombstones: [], revision: null }; if (!response.ok) throw new Error(`Failed to read the stored config: ${response.status}`); const { config } = await readJson(response); return { stored: normalizeAppServiceKeys((config?.overrides ?? {}) as t.ConfigRecord), tombstones: config?.tombstones ?? [], + ...(typeof config?.revision === 'string' && { revision: config.revision }), }; } @@ -208,6 +217,17 @@ async function failureIssues( batch: t.SaveEntry[] = [], ): Promise { const body = await readJson(response); + return describeFailure(body, response.status, path, features, batch); +} + +/** A refused write as issues: the values it rejected, else the refusal itself, located at `path`. */ +function describeFailure( + body: t.PatchFieldsResponse & { section?: string }, + status: number, + path: string, + features: t.ConfigFeatures, + batch: t.SaveEntry[], +): t.ConfigIssue[] { if (Array.isArray(body.issues) && body.issues.length > 0) { return body.issues.map((issue) => attributeBackendIssue(issue, batch, features)); } @@ -225,8 +245,10 @@ async function failureIssues( describeApiError(body) ?? body.error ?? body.message ?? - `The admin API rejected the change (${response.status})`; - return [{ path, code: 'WRITE_FAILED', message }]; + `The admin API rejected the change (${status})`; + return [ + { path, code: body.code === VERSION_CONFLICT_CODE ? body.code : 'WRITE_FAILED', message }, + ]; } interface WriteOutcome { @@ -239,20 +261,19 @@ interface WriteOutcome { } /** - * Files what a write's reply reports (`readWriteReport`, for the locations - * the save changes): a location the write addressed as a whole is `dropped`, - * a part of it only `reported`. Before `writeReport`, a PATCH's dropped - * entries are read from the stored sections (`findDroppedEntries`). + * Files what a write's reply reports (`readWriteReport`, `readBatchReport`, + * for the locations the save changes): a location the write addressed as a + * whole is `dropped`, a part of it only `reported`. Before `writeReport`, a + * PATCH's dropped entries are read from the stored sections + * (`findDroppedEntries`). */ function fileWriteReport( - outcome: WriteOutcome, - body: t.PatchFieldsResponse, + outcome: Pick, + items: t.SkippedChange[], written: readonly string[], - features: t.ConfigFeatures, - changed: t.PathIndex, ): void { const whole = new Set(written); - for (const item of readWriteReport(body, features, changed)) { + for (const item of items) { (whole.has(item.fieldPath) ? outcome.dropped : outcome.reported).push(item); } } @@ -289,7 +310,7 @@ async function patchFields( })), ); } - fileWriteReport(outcome, body, paths, features, changed); + fileWriteReport(outcome, readWriteReport(body, features, changed), paths); } return true; } @@ -327,20 +348,24 @@ async function writeFieldOps( outcome.writes.push({ kind, path }); if (response.ok) { const body = await readJson(response); - fileWriteReport(outcome, body, [path], features, changed); + fileWriteReport(outcome, readWriteReport(body, features, changed), [path]); } } return true; } /** - * Writes a plan in a fixed order: tombstones, then every set (one PATCH per - * 100 entries), then resets and untombstones. Tombstones go first because - * LibreChat validates a PATCH against the layer beneath minus the target's - * stored tombstones: switching `memory.agent` from a librechat.yaml `id` to - * an inline provider/model only validates once `memory.agent.id` is hidden. - * Sets precede resets, so a rename writes the new entry before removing the - * old one. Stops at the first failure. + * Before `changesBatch`, writes a plan as separate field writes in a fixed + * order: untombstones, tombstones, then every set (one PATCH per + * `maxPatchEntries` entries), then resets. An untombstone removes everything + * at and under its path, so it goes first: after them, it would remove a set + * or tombstone inside it (an MCP server shown again with its URL edited). + * Tombstones precede sets because LibreChat validates a PATCH against the + * layer beneath minus the target's stored tombstones: switching + * `memory.agent` from a librechat.yaml `id` to an inline provider/model only + * validates once `memory.agent.id` is hidden. Sets precede resets, so a + * rename writes the new entry before removing the old one. Stops at the + * first failure. */ async function executePlan( target: t.WriteTargetRef, @@ -351,10 +376,10 @@ async function executePlan( const { features } = contract; const changed = createPathIndex(plan.changed); const steps = [ + () => writeFieldOps(target, 'untombstone', plan.untombstones, outcome, features, changed), () => writeFieldOps(target, 'tombstone', plan.tombstones, outcome, features, changed), () => patchFields(target, plan.sets, outcome, contract, changed), () => writeFieldOps(target, 'reset', plan.resets, outcome, features, changed), - () => writeFieldOps(target, 'untombstone', plan.untombstones, outcome, features, changed), ]; try { for (const step of steps) { @@ -449,27 +474,9 @@ function hasWrites(plan: t.WritePlan): boolean { ); } -/** - * The one write path for config changes (SV-1/SV-2/SV-4). Reads the target's - * stored overrides and the layer beneath it, and the server's contract - * (`readConfigContract`), plans the changes against them - * (`planConfigChanges`: keyed collection ops by identity, validation of - * fields, entries and whole sections, write rules), and writes nothing when - * any issue is found. Otherwise it writes tombstones, sets, then resets; - * if a write fails, the writes before it are rolled back to the captured - * overrides. Afterwards the stored (and, for base, effective) config is read - * back, and every change that did not take effect is reported in `skipped`. - */ -export async function applyConfigChanges( - target: t.WriteTargetRef, - ops: t.ConfigChange[], -): Promise { - const contract = await readConfigContract(); - const rules = getWriteRules(contract); - await authorizeWrite(target, ops, rules); - const snapshot = await readWriteSnapshot(target); - const plan = planConfigChanges(sanitizeOps(ops), snapshot, contract.features, rules); - const result: t.ApplyConfigChangesResult = { +/** What a plan reports before anything is written. */ +function planResult(plan: t.WritePlan): t.ApplyConfigChangesResult { + return { applied: 0, failed: [], warnings: plan.warnings, @@ -477,11 +484,49 @@ export async function applyConfigChanges( skipped: plan.skipped, reported: [], }; - if (plan.issues.length > 0 || !hasWrites(plan)) return result; +} + +/** + * A written plan's result: the input changes that took effect (every change + * not reported or found ineffective, merged-back keys aside), each that did + * not, with why (`skipped`), and apart from them the parts of written ones + * the server reported (`reported`, once each however many writes repeat them). + */ +function writtenResult( + plan: t.WritePlan, + reported: Pick, + verified: t.SkippedChange[], +): t.ApplyConfigChangesResult { + const notEffective = [...reported.dropped, ...verified]; + const lost = new Set( + notEffective + .filter((item) => !MERGED_BACK_REASONS.has(item.reason)) + .map((item) => item.fieldPath), + ); + return { + ...planResult(plan), + applied: Math.max(0, plan.effective - lost.size), + skipped: [...plan.skipped, ...notEffective], + reported: uniqueSkipped(reported.reported), + }; +} + +/** + * Before `changesBatch`: tombstones, sets, then resets as separate field + * writes; if a write fails, the writes before it are rolled back to the + * captured overrides. Afterwards the stored (and, for base, effective) + * config is read back for what did not take effect. + */ +async function applyFieldWrites( + target: t.WriteTargetRef, + plan: t.WritePlan, + snapshot: t.WriteSnapshot, + contract: t.ConfigContract, +): Promise { const outcome = await executePlan(target, plan, contract); if (outcome.failure) { const compensated = await compensate(target, outcome.writes, snapshot.stored); - return { ...result, failed: outcome.failure, compensated }; + return { ...planResult(plan), failed: outcome.failure, compensated }; } const reportedPaths = new Set( [...outcome.dropped, ...outcome.reported].map((item) => item.fieldPath), @@ -490,20 +535,268 @@ export async function applyConfigChanges( const verified = (await verifyTarget(target, plan, snapshot)).filter( (item) => !reportedPaths.has(item.fieldPath), ); - const notEffective = [...outcome.dropped, ...verified]; - const lost = new Set( - notEffective - .filter((item) => !MERGED_BACK_REASONS.has(item.reason)) - .map((item) => item.fieldPath), + return writtenResult(plan, outcome, verified); +} + +interface BatchOutcome extends Pick { + /** 409: the target is no longer at the revision the batch named. */ + conflict?: boolean; + /** A batch failed after earlier batches of the plan were written. */ + partial?: boolean; + /** The target's stored overrides after the last batch, as its reply returned them. */ + stored?: t.ConfigRecord; +} + +/** A refused batch as issues, located at the change it names (`opIndex`). */ +function batchFailure( + body: t.ChangesBatchResponse & { section?: string }, + status: number, + ops: t.ConfigBatchOp[], + features: t.ConfigFeatures, +): t.ConfigIssue[] { + const refused = body.opIndex === undefined ? undefined : ops[body.opIndex]; + const sets = ops.flatMap((op): t.SaveEntry[] => + op.op === 'set' ? [{ fieldPath: op.fieldPath, value: op.value }] : [], ); + return describeFailure(body, status, (refused ?? ops[0]).fieldPath, features, sets); +} + +/** How many changes of a batch fall in each config section, in the order the sections first appear. */ +function countSectionChanges(ops: t.ConfigBatchOp[]): Map { + const counts = new Map(); + for (const { fieldPath } of ops) { + const section = getConfigSection(fieldPath); + counts.set(section, (counts.get(section) ?? 0) + 1); + } + return counts; +} + +/** Sections packed in order into groups of at most `size` changes in all; a section is never split. */ +function packSections(counts: ReadonlyMap, size: number): string[][] { + const groups: string[][] = []; + let group: string[] = []; + let filled = 0; + for (const [section, count] of counts) { + if (group.length > 0 && filled + count > size) { + groups.push(group); + group = []; + filled = 0; + } + group.push(section); + filled += count; + } + if (group.length > 0) groups.push(group); + return groups; +} + +function tooManyChangesIssue(section: string, count: number, limit: number): t.ConfigIssue { + const params = { count, limit }; return { - ...result, - applied: Math.max(0, plan.effective - lost.size), - skipped: [...plan.skipped, ...notEffective], - reported: uniqueSkipped(outcome.reported), + path: section, + code: TOO_MANY_CHANGES_CODE, + message: describeIssueCode(TOO_MANY_CHANGES_CODE, params, 'has too many changes for one save'), + params, }; } +/** + * The batches a plan is sent in: one, or for a plan above `maxChangeOps`, one + * per group of whole config sections. LibreChat judges each batch on its own + * end state, section by section, so the changes of one section (a hide and + * the values that replace it, an entry and the settings that depend on it) + * always go together. Each batch is planned again from its sections' changes, + * against the layers as read (the batches before it write other sections + * only), and sent as that plan writes it. Nothing is written, and the refused + * save's result is returned instead, when one section alone holds more + * changes than a batch takes or a batch's own plan finds an issue. + */ +function splitPlan( + changes: t.ConfigChange[], + plan: t.WritePlan, + snapshot: t.WriteSnapshot, + contract: t.ConfigContract, + rules: t.ConfigWriteRules, +): t.ConfigBatchOp[][] | t.ApplyConfigChangesResult { + const ops = toBatchOps(plan); + const size = contract.limits.maxChangeOps; + if (size <= 0 || ops.length <= size) return [ops]; + const counts = countSectionChanges(ops); + const oversized = [...counts] + .filter(([, count]) => count > size) + .map(([section, count]) => tooManyChangesIssue(section, count, size)); + if (oversized.length > 0) return { ...planResult(plan), failed: oversized }; + const batches = packSections(counts, size).map((sections) => { + const included = new Set(sections); + const own = changes.filter((change) => included.has(getConfigSection(change.path))); + return planConfigChanges(own, snapshot, contract.features, rules); + }); + const issues = batches.flatMap((batch) => batch.issues); + if (issues.length > 0) return { ...planResult(plan), issues }; + return batches.map(toBatchOps); +} + +/** + * Writes a plan's batches (`splitPlan`) as `POST …/changes` requests + * (`changesBatch`), each named by the revision the one before it left. A + * batch applies whole or not at all, so nothing is rolled back by hand; a + * failure after earlier batches were written is `partial`. + */ +async function writeBatches( + target: t.WriteTargetRef, + batches: t.ConfigBatchOp[][], + plan: t.WritePlan, + contract: t.ConfigContract, + revision: string | undefined, +): Promise { + const changed = createPathIndex(plan.changed); + const priority = target.type === 'base' ? { priority: 0 } : {}; + const outcome: BatchOutcome = { dropped: [], reported: [] }; + let expected = revision; + let index = 0; + try { + for (; index < batches.length; index++) { + const batch = batches[index]; + const response = await apiFetch(`${targetConfigUrl(target)}/changes`, { + method: 'POST', + body: JSON.stringify({ + ops: batch, + ...priority, + ...(expected !== undefined && { expectedRevision: expected }), + }), + }); + const body = await readJson(response); + if (!response.ok) { + outcome.conflict = response.status === 409 && body.code === VERSION_CONFLICT_CODE; + outcome.failure = batchFailure(body, response.status, batch, contract.features); + outcome.partial = index > 0; + return outcome; + } + const written = batch.map(({ fieldPath }) => fieldPath); + fileWriteReport(outcome, readBatchReport(body, changed), written); + expected = toRevisionPrecondition(body.revision); + const stored = body.config?.overrides; + if (isConfigRecord(stored)) outcome.stored = normalizeAppServiceKeys(stored); + } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + outcome.failure = [{ path: '', code: 'WRITE_FAILED', message }]; + outcome.partial = index > 0; + } + return outcome; +} + +/** + * What a written batch did not make take effect beyond what its reply + * reported: the stored overrides the reply returned, checked against the + * plan, and for Base the effective config, read once where the server + * clears its caches before replying (`awaitedInvalidation`). A finding at, + * inside or around a location the reply reported is that same change. + */ +async function verifyBatch( + target: t.WriteTargetRef, + plan: t.WritePlan, + snapshot: t.WriteSnapshot, + outcome: BatchOutcome, + features: t.ConfigFeatures, +): Promise { + const reported = createPathIndex( + [...outcome.dropped, ...outcome.reported].map((item) => item.fieldPath), + ); + const readsEffective = target.type === 'base' && features.awaitedInvalidation; + const effective = readsEffective ? await readBaseConfig(false).catch(() => undefined) : undefined; + return verifyWrite(plan, snapshot, outcome.stored ?? plan.storedAfter, effective).filter( + (item) => !pathIndexOverlaps(reported, item.fieldPath), + ); +} + +const CONFLICT_ISSUE: t.ConfigIssue = { + path: '', + code: VERSION_CONFLICT_CODE, + message: + describeApiError({ code: VERSION_CONFLICT_CODE }) ?? 'The configuration changed elsewhere', +}; + +/** The result of a save that found the target changed since `expectedRevision`: nothing was written. */ +function conflictResult(plan?: t.WritePlan): t.ApplyConfigChangesResult { + const result = plan + ? planResult(plan) + : { applied: 0, warnings: [], issues: [], skipped: [], reported: [] }; + return { ...result, failed: [CONFLICT_ISSUE], conflict: true }; +} + +/** + * With `changesBatch`: the plan is written as one `POST …/changes` batch + * (`writeBatches`) named by the revision it was planned against, or above + * `maxChangeOps` as batches of whole sections (`splitPlan`). The edits were + * made against `expectedRevision`: when the target is stored at another + * revision now, or a write lands between the read and the batch (409), + * nothing is written and the result is a `conflict`. Without one (an import), + * a batch that loses that race is planned and sent once more. A 409 on a + * later batch of a split save is `partiallyWritten`, not a conflict: the + * batches before it stay saved. + */ +async function applyBatch( + target: t.WriteTargetRef, + ops: t.ConfigChange[], + contract: t.ConfigContract, + rules: t.ConfigWriteRules, + expectedRevision: string | undefined, +): Promise { + const changes = sanitizeOps(ops); + for (let attempt = 0; ; attempt++) { + const snapshot = await readWriteSnapshot(target); + const revision = toRevisionPrecondition(snapshot.revision); + const comparable = revision !== undefined && expectedRevision !== undefined; + if (comparable && revision !== expectedRevision) { + return conflictResult(); + } + const plan = planConfigChanges(changes, snapshot, contract.features, rules); + if (plan.issues.length > 0 || !hasWrites(plan)) return planResult(plan); + const batches = splitPlan(changes, plan, snapshot, contract, rules); + if (!Array.isArray(batches)) return batches; + const outcome = await writeBatches(target, batches, plan, contract, revision); + const retry = outcome.conflict && !outcome.partial && expectedRevision === undefined; + if (retry && attempt === 0) continue; + if (outcome.failure) { + return { + ...(outcome.conflict && !outcome.partial ? conflictResult(plan) : planResult(plan)), + failed: outcome.failure, + ...(outcome.partial && { partiallyWritten: true }), + }; + } + const verified = await verifyBatch(target, plan, snapshot, outcome, contract.features); + return writtenResult(plan, outcome, verified); + } +} + +/** + * The one write path for config changes (SV-1/SV-2/SV-4). Reads the target's + * stored overrides and the layer beneath it, and the server's contract + * (`readConfigContract`), plans the changes against them + * (`planConfigChanges`: keyed collection ops by identity, validation of + * fields, entries and whole sections, write rules), and writes nothing when + * any issue is found. A server with `changesBatch` and `expectedRevision` + * takes the plan as one atomic batch (`applyBatch`); an older one as + * separate field writes (`applyFieldWrites`). Every change that did not take + * effect is reported in `skipped`. + */ +export async function applyConfigChanges( + target: t.WriteTargetRef, + ops: t.ConfigChange[], + expectedRevision?: string, +): Promise { + const contract = await readConfigContract(); + const rules = getWriteRules(contract); + await authorizeWrite(target, ops, rules); + if (usesChangeBatch(contract.features)) { + return applyBatch(target, ops, contract, rules, expectedRevision); + } + const snapshot = await readWriteSnapshot(target); + const plan = planConfigChanges(sanitizeOps(ops), snapshot, contract.features, rules); + if (plan.issues.length > 0 || !hasWrites(plan)) return planResult(plan); + return applyFieldWrites(target, plan, snapshot, contract); +} + const writeTargetSchema = z.discriminatedUnion('type', [ z.object({ type: z.literal('base') }), z.object({ @@ -536,18 +829,25 @@ const configChangeSchema = z.discriminatedUnion('op', [ }), ]); -/** Applies a batch of config changes to base or a profile, all or nothing (see `applyConfigChanges`). */ +/** + * Applies a batch of config changes to base or a profile, all or nothing, + * unless the target changed since `expectedRevision` (see `applyConfigChanges`). + */ export const applyConfigChangesFn = createServerFn({ method: 'POST' }) .inputValidator( z.object({ target: writeTargetSchema, ops: z.array(configChangeSchema).min(1).max(2000), - expectedVersion: z.number().int().optional(), + expectedRevision: z.string().min(1).max(200).optional(), }), ) .handler( ({ data }): Promise => - applyConfigChanges(data.target as t.WriteTargetRef, data.ops as t.ConfigChange[]), + applyConfigChanges( + data.target as t.WriteTargetRef, + data.ops as t.ConfigChange[], + data.expectedRevision, + ), ); /** Deletes the entire base config DB override, reverting every value back to diff --git a/src/server/scopes/read.ts b/src/server/scopes/read.ts index d0d6b270..67f2d242 100644 --- a/src/server/scopes/read.ts +++ b/src/server/scopes/read.ts @@ -9,12 +9,17 @@ import type { AdminUserListItem, } from '@librechat/data-schemas'; import type * as t from '@/types'; +import { + ABSENT_REVISION, + MAX_SCOPE_PRIORITY, + VERSION_CONFLICT_CODE, + DEFAULT_SCOPE_PRIORITY, +} from '@/constants'; import { collectOverridePaths, scopeOverrideEntries, stripSecretPreviewValues } from '@/utils'; import { normalizeAppServiceKeys } from '../config/normalize'; import { apiErrorBodyMessage, apiErrorMessage, apiFetch } from '../utils/api'; import { BASE_CONFIG_PRINCIPAL_ID } from '../constants'; import { requireCapability, requireAnyCapability } from '../capabilities'; -import { MAX_SCOPE_PRIORITY, DEFAULT_SCOPE_PRIORITY } from '@/constants'; import { getSchemaPathSet } from '../config/schema'; import { readConfigContract } from '../config/meta'; import { deepGet } from './paths'; @@ -239,11 +244,7 @@ export const fieldProfileValuesOptions = (fieldPath: string) => queryFn: () => getFieldProfileValuesFn({ data: { fieldPath } }).then((r) => r.values), }); -/** `expectedRevision` that makes a PUT create the profile only when the principal has none. */ -const ABSENT_REVISION = 'absent'; - -/** The admin API's codes for a create-only PUT the principal already has a profile for, or has none. */ -const VERSION_CONFLICT_CODE = 'CONFIG_VERSION_CONFLICT'; +/** The admin API's code for a create-only PUT for a principal that does not exist. */ const PRINCIPAL_NOT_FOUND_CODE = 'PRINCIPAL_NOT_FOUND'; function missingRoleMessage(principalId: string): string { diff --git a/src/server/scopes/resolved.ts b/src/server/scopes/resolved.ts index 517f8a9a..ed88409b 100644 --- a/src/server/scopes/resolved.ts +++ b/src/server/scopes/resolved.ts @@ -48,6 +48,12 @@ export const getResolvedConfigFn = createServerFn({ method: 'GET' }) const resolvedConfig: t.FlatConfigMap = {}; for (const path of changedPaths) resolvedConfig[path] = getValueAtPath(overrides, path); - return { resolvedConfig, changedPaths, overrides, tombstones: config.tombstones ?? [] }; + return { + resolvedConfig, + changedPaths, + overrides, + tombstones: config.tombstones ?? [], + ...(typeof config.revision === 'string' && { revision: config.revision }), + }; }, ); diff --git a/src/test/admin.ts b/src/test/admin.ts index a3270b84..64e78679 100644 --- a/src/test/admin.ts +++ b/src/test/admin.ts @@ -3,20 +3,30 @@ import { isConfigRecord, getValueAtPath, setValueAtPath, + validateSection, mergeConfigLayers, removeValueAtPath, } from '@/utils'; +import { getConfigSection } from '@/constants'; /** One request the fake admin API received. */ export interface FakeAdminRequest { method: string; path: string; - body?: { entries?: t.SaveEntry[]; fieldPath?: string; priority?: number }; + body?: { + entries?: t.SaveEntry[]; + fieldPath?: string; + priority?: number; + ops?: t.ConfigBatchOp[]; + expectedRevision?: string; + }; } interface FakeDoc { overrides: t.ConfigRecord; tombstones: string[]; + /** Bumped by every write, as `configVersion` is. */ + version: number; } /** What a write's reply adds to the stored document (a write report, ignored credentials). */ @@ -29,6 +39,38 @@ export type FakeAdminOverride = ( const SECRET_KEYS = new Set(['apiKey']); +const BASE_KEY = 'role/__base__'; + +function issueKey(issue: t.ConfigIssue): string { + return `${issue.path}|${issue.code ?? ''}`; +} + +/** Whether `path` is `ancestor` or lies inside it. */ +function isAtOrUnder(path: string, ancestor: string): boolean { + return path === ancestor || path.startsWith(`${ancestor}.`); +} + +/** + * Whether a write at `path` clears `tombstone`, as LibreChat's writes do: one + * at the path or above it below the section, so a whole-section tombstone + * survives a nested write. + */ +function clearsTombstone(path: string, tombstone: string): boolean { + return tombstone === path || (tombstone.includes('.') && isAtOrUnder(path, tombstone)); +} + +/** Removes `path` and every object that leaves empty above it, stopping at arrays (`prunesEmptyParents`). */ +function removePruned(overrides: t.ConfigRecord, path: string): t.ConfigRecord { + let result = removeValueAtPath(overrides, path); + for (let dot = path.lastIndexOf('.'); dot > 0; dot = path.lastIndexOf('.', dot - 1)) { + const parent = path.slice(0, dot); + const value = getValueAtPath(result, parent); + if (!isConfigRecord(value) || Object.keys(value).length > 0) break; + result = removeValueAtPath(result, parent); + } + return result; +} + /** Secret values stored as plain references (`${ENV}`, `user_provided`). */ function isPassthrough(value: string): boolean { return value.startsWith('${') || value === 'user_provided'; @@ -62,10 +104,18 @@ export function createFakeAdminApi(yaml: t.ConfigRecord) { let override: FakeAdminOverride | undefined; let report: FakeWriteReport | undefined; let meta: t.AdminConfigMetaBody | undefined; + let judging = false; const docKey = (principalType: string, principalId: string) => `${principalType}/${decodeURIComponent(principalId)}`; + /** Revisions are reported only by a server whose contract says so (`expectedRevision`). */ + const reportsRevisions = () => meta?.features?.expectedRevision === true; + const revisionOf = (key: string, doc: FakeDoc | undefined) => + doc ? `${key.replace('/', '-')}:${doc.version}` : null; + const store = (key: string, next: Omit) => + docs.set(key, { ...next, version: (docs.get(key)?.version ?? 0) + 1 }); + const reply = (status: number, body: unknown) => ({ ok: status >= 200 && status < 300, status, json: async () => body }) as Response; @@ -99,7 +149,7 @@ export function createFakeAdminApi(yaml: t.ConfigRecord) { } function baseMerged(): t.ConfigRecord { - const base = docs.get(docKey('role', '__base__')); + const base = docs.get(BASE_KEY); return mergeConfigLayers(yaml, base?.overrides ?? {}) as t.ConfigRecord; } @@ -122,9 +172,10 @@ export function createFakeAdminApi(yaml: t.ConfigRecord) { const config = url.searchParams.get('baseOnly') === 'true' ? yaml : baseMerged(); return reply(200, { config: redact(config) }); } - const match = /^\/api\/admin\/config\/([^/]+)\/([^/]+)(\/fields(\/tombstone)?)?$/.exec( - url.pathname, - ); + const match = + /^\/api\/admin\/config\/([^/]+)\/([^/]+)(\/fields(\/tombstone)?|\/changes)?$/.exec( + url.pathname, + ); if (!match) return reply(404, { error: 'Not found' }); const [, principalType, principalId, fields, tombstone] = match; const key = docKey(principalType, principalId); @@ -133,13 +184,18 @@ export function createFakeAdminApi(yaml: t.ConfigRecord) { if (!fields) { if (!doc) return reply(404, { error: 'Config not found' }); return reply(200, { - config: { overrides: redact(doc.overrides), tombstones: doc.tombstones }, + config: { + overrides: redact(doc.overrides), + tombstones: doc.tombstones, + ...(reportsRevisions() && { revision: revisionOf(key, doc) }), + }, }); } + if (fields === '/changes') return applyBatch(request, key, doc); const current = doc ?? { overrides: {}, tombstones: [] }; if (tombstone) { const fieldPath = String(body?.fieldPath); - docs.set(key, { + store(key, { overrides: removeValueAtPath(current.overrides, fieldPath), tombstones: [...new Set([...current.tombstones, fieldPath])], }); @@ -148,7 +204,7 @@ export function createFakeAdminApi(yaml: t.ConfigRecord) { if (method === 'DELETE') { if (!doc) return reply(404, { error: 'Config not found' }); const fieldPath = String(url.searchParams.get('fieldPath')); - docs.set(key, { + store(key, { overrides: removeValueAtPath(doc.overrides, fieldPath), tombstones: doc.tombstones.filter((p) => p !== fieldPath && !p.startsWith(`${fieldPath}.`)), }); @@ -165,14 +221,116 @@ export function createFakeAdminApi(yaml: t.ConfigRecord) { const existing = getValueAtPath(current.overrides, entry.fieldPath); overrides = setValueAtPath(overrides, entry.fieldPath, encryptSecrets(entry.value, existing)); } - const paths = (body?.entries ?? []).map((e: t.SaveEntry) => e.fieldPath); - docs.set(key, { + const paths: string[] = (body?.entries ?? []).map((e: t.SaveEntry) => e.fieldPath); + store(key, { overrides, - tombstones: current.tombstones.filter((p) => !paths.includes(p)), + tombstones: current.tombstones.filter((p) => !paths.some((path) => clearsTombstone(path, p))), }); return written(request, { config: { overrides: redact(overrides) } }); } + /** A document's schema issues in `section`: its overrides over the layer beneath, without what it hides. */ + function sectionIssues( + key: string, + state: Omit, + section: string, + ): t.ConfigIssue[] { + const lower = key === BASE_KEY ? yaml : baseMerged(); + const floor = state.tombstones.reduce(removeValueAtPath, lower); + return validateSection( + section, + mergeConfigLayers(floor[section], state.overrides[section], section), + ); + } + + /** + * The issues a batch introduces (`judgeEndStates`): those of each section + * it writes that the document did not have before, by the panel's own + * section validation. LibreChat judges a batch on its end state too, a + * little more leniently (only sections the document stores values in). + */ + function endStateIssues( + key: string, + before: Omit, + after: Omit, + ops: t.ConfigBatchOp[], + ): t.ConfigIssue[] { + const sections = new Set(ops.map(({ fieldPath }) => getConfigSection(fieldPath))); + return [...sections].flatMap((section) => { + const had = new Set(sectionIssues(key, before, section).map(issueKey)); + return sectionIssues(key, after, section).filter((issue) => !had.has(issueKey(issue))); + }); + } + + /** One batch op applied to a document, as `POST …/changes` applies it. */ + function applyOp( + current: Omit, + op: t.ConfigBatchOp, + ): Omit { + const { fieldPath } = op; + if (op.op === 'set') { + const existing = getValueAtPath(current.overrides, fieldPath); + return { + overrides: setValueAtPath(current.overrides, fieldPath, encryptSecrets(op.value, existing)), + tombstones: current.tombstones.filter((p) => !clearsTombstone(fieldPath, p)), + }; + } + const overrides = removePruned(current.overrides, fieldPath); + const kept = current.tombstones.filter((p) => !isAtOrUnder(p, fieldPath)); + return { overrides, tombstones: op.op === 'tombstone' ? [...kept, fieldPath] : kept }; + } + + /** + * `POST …/changes`: every op applied in order, written at once and named + * by `expectedRevision` (409 when the document is at another revision). + * Ops in a stripped section are reported as skipped. + */ + function applyBatch(request: FakeAdminRequest, key: string, doc: FakeDoc | undefined): Response { + const { ops = [], expectedRevision } = request.body ?? {}; + const currentRevision = revisionOf(key, doc); + if (expectedRevision !== undefined && expectedRevision !== (currentRevision ?? 'absent')) { + return reply(409, { + error: 'Config was modified by another request', + code: 'CONFIG_VERSION_CONFLICT', + currentRevision, + }); + } + const current: Omit = doc ?? { overrides: {}, tombstones: [] }; + let next = current; + const applied: object[] = []; + const skipped: object[] = []; + ops.forEach((op, opIndex) => { + const ref = { opIndex, op: op.op, segments: op.fieldPath.split('.') }; + if (strippedSections.has(ref.segments[0])) { + skipped.push({ ...ref, reason: 'base_only_section' }); + return; + } + next = applyOp(next, op); + applied.push(ref); + }); + const issues = judging ? endStateIssues(key, current, next, ops) : []; + if (issues.length > 0) { + return reply(400, { + error: 'Invalid config override', + code: 'CONFIG_OVERRIDE_INVALID', + issues, + }); + } + store(key, next); + const stored = docs.get(key)!; + const revision = revisionOf(key, stored); + return written(request, { + config: { overrides: redact(stored.overrides), tombstones: stored.tombstones, revision }, + revision, + written: true, + dryRun: false, + applied, + skipped, + ignored: [], + unchanged: [], + }); + } + return { fetch, requests, @@ -180,7 +338,9 @@ export function createFakeAdminApi(yaml: t.ConfigRecord) { stored: (principal = 'role/__base__') => docs.get(principal)?.overrides, tombstones: (principal = 'role/__base__') => docs.get(principal)?.tombstones ?? [], seed: (principal: string, overrides: t.ConfigRecord, tombstones: string[] = []) => - docs.set(principal, { overrides, tombstones }), + store(principal, { overrides, tombstones }), + /** The revision a principal's document is at (`null`: none), as the server reports it. */ + revision: (principal = 'role/__base__') => revisionOf(principal, docs.get(principal)), /** Silently drops writes to a section, as LibreChat does for base-only sections. */ stripSection: (section: string) => strippedSections.add(section), setOverride: (next?: FakeAdminOverride) => { @@ -190,6 +350,10 @@ export function createFakeAdminApi(yaml: t.ConfigRecord) { reportWrites: (next?: FakeWriteReport) => { report = next; }, + /** Refuses (400) a batch that leaves a section it writes with a schema issue it did not have. */ + judgeEndStates: () => { + judging = true; + }, /** Serves `GET /api/admin/config/meta` (a server with the contract); unset, it answers 404 like an older server. */ reportMeta: (body?: t.AdminConfigMetaBody) => { meta = body; diff --git a/src/types/changes.ts b/src/types/changes.ts index 4d329cb3..f3de9280 100644 --- a/src/types/changes.ts +++ b/src/types/changes.ts @@ -80,8 +80,16 @@ export interface ReportedIssue extends ReportedLocation { /** A location a write did not store (`skipped`) or stores without effect (`ignored`), and why. */ export interface ReportedSkip extends ReportedLocation { reason: string; + /** The change of a `POST …/changes` batch it belongs to, by position in the sent ops. */ + opIndex?: number; } +/** One change of a `POST …/changes` batch as the panel sends it (`changesBatch`). */ +export type ConfigBatchOp = + | { op: 'set'; fieldPath: string; value: ConfigValue } + /** Removes the target's override at the path, the objects that leaves empty and the tombstones at or below it. */ + | { op: 'unset' | 'tombstone'; fieldPath: string }; + /** An admin API error reply: its text, and the stable `code` (and route details) newer servers add. */ export interface ApiErrorBody { error?: string; @@ -95,7 +103,12 @@ export interface ApiErrorBody { export interface ApplyConfigChangesRequest { target: WriteTarget; ops: ConfigChange[]; - expectedVersion?: number; + /** + * The target's revision the edits were made against (`changesBatch`): + * `absent` when it had no document. A save finds it changed elsewhere + * (`conflict`) instead of writing over it. + */ + expectedRevision?: string; } /** Warning attached to a change in the review (WS1 produces, WS7 renders). */ @@ -148,8 +161,15 @@ export interface ApplyConfigChangesResult { * without effect (a credential it drops): the change itself is written. */ reported: SkippedChange[]; - /** A failed write was rolled back to the overrides captured before it. */ + /** A failed write was rolled back to the overrides captured before it (servers without `changesBatch`). */ compensated?: boolean; + /** + * The target was saved elsewhere since the revision the edits were made + * against (`expectedRevision`), so nothing was written. + */ + conflict?: boolean; + /** A save split into batches of `maxChangeOps` failed after earlier batches were written. */ + partiallyWritten?: boolean; } /** How a change reads in the review (WS1). */ @@ -229,6 +249,11 @@ export interface WriteSnapshot { /** The layer beneath the target: librechat.yaml for base, the base config for a profile. */ lower: ConfigRecord; lowerKind: LowerLayerKind; + /** + * The target's stored revision (`expectedRevision`): `null` when it has no + * document, absent when the server does not report revisions. + */ + revision?: string | null; } /** The admin API writes a batch of changes resolves to, and what blocks or limits it (WS1). */ diff --git a/src/types/config-ui.ts b/src/types/config-ui.ts index 4a1954af..0810718a 100644 --- a/src/types/config-ui.ts +++ b/src/types/config-ui.ts @@ -556,6 +556,19 @@ export interface PatchFieldsResponse { ignored?: ReportedSkip[]; } +/** Body of a `POST …/changes` reply (`changesBatch`): the stored document's revision and what the batch did. */ +export interface ChangesBatchResponse extends PatchFieldsResponse { + config?: { overrides?: ConfigRecord; tombstones?: string[]; revision?: string } | null; + /** The stored document's revision after the batch; `null` when the target has none. */ + revision?: string | null; + /** Whether a document was written: a batch that changes nothing writes nothing. */ + written?: boolean; + /** A refused change of the batch (400), by position in the sent ops. */ + opIndex?: number; + /** `CONFIG_VERSION_CONFLICT` (409): the revision stored now. */ + currentRevision?: string | null; +} + /** Outcome of a save call: how many entries the backend applied and which were dropped. */ export interface ConfigWriteResult { success: boolean; diff --git a/src/types/hooks.ts b/src/types/hooks.ts index b214f7b4..881f9fe2 100644 --- a/src/types/hooks.ts +++ b/src/types/hooks.ts @@ -132,6 +132,11 @@ export interface ConfigLayers { listOverrideCounts: ReadonlyMap; /** Every layer `useFieldState` derives provenance and revert targets from. */ valueLayers: ValueLayers | null; + /** + * The edit target's stored revision as loaded (`expectedRevision`): `null` + * when it has no document, absent when the server reports none. + */ + targetRevision?: string | null; } /** Options for `useTabBadges` (WS4). */ @@ -253,6 +258,7 @@ export interface ConfigSaveOptions { | 'overridePaths' | 'valueLayers' | 'baseActiveConfigValues' + | 'targetRevision' >; edits: Pick< ConfigEditsState, @@ -287,6 +293,15 @@ export interface ConfigSaveState { /** One readable line per issue ("GPT › Provider must be "anthropic""). */ issueSummaries: string[]; handleConfirmSave: () => Promise; + /** + * The last save found the target saved elsewhere since the edits were + * loaded (`expectedRevision`), and wrote nothing. + */ + conflict: boolean; + /** Re-reads the target after a conflict; the edits stay, and the review marks what changed. */ + reloadLatest: () => Promise; + /** Saves the edits over the target as it is stored now: what others saved elsewhere stays. */ + saveOverLatest: () => Promise; openReview: () => void; cancelReview: () => void; closeReview: () => void; diff --git a/src/types/review.ts b/src/types/review.ts index d4dedd07..a120935a 100644 --- a/src/types/review.ts +++ b/src/types/review.ts @@ -201,6 +201,9 @@ export interface ChangeReviewOptions { | 'openReview' | 'cancelReview' | 'handleConfirmSave' + | 'conflict' + | 'reloadLatest' + | 'saveOverLatest' >; /** Switches the page to a tab (the review's "Go to field"). */ onTabChange: (tabId: string) => void; @@ -215,6 +218,15 @@ export interface ChangeReviewState { sessionExpired: boolean; /** The last save was refused until the admin accepts LibreChat's terms: where to accept them, else `null`. */ termsUrl: string | null; + /** + * The last save found the target saved elsewhere meanwhile, and wrote + * nothing: until Reload or Merge, or the review is closed, Save is refused. + */ + conflict: boolean; + /** After a conflict: drops the edits (with Undo) and shows the target as it is stored now. */ + reloadLatest: () => void; + /** After a conflict: saves the edits over the target as it is stored now. */ + mergeLatest: () => Promise; /** Error from the last failed save, as labelled issues. */ saveIssues: ReviewIssue[]; /** Title naming the target: "Save 2 changes to Role: USER?". */ diff --git a/src/types/values.ts b/src/types/values.ts index b1229cd5..29c18979 100644 --- a/src/types/values.ts +++ b/src/types/values.ts @@ -215,15 +215,23 @@ export interface BaseConfigData { dbOverrides?: ConfigRecord; /** Paths the base overrides remove from librechat.yaml. */ dbTombstones: string[]; + /** + * The base document's revision (`expectedRevision`): `null` when there is + * none, absent when the server does not report revisions. + */ + dbRevision?: string | null; schemaDefaults: FlatConfigMap; /** MCP servers defined in librechat.yaml. */ yamlMcpKeys?: string[]; yamlMcpServers?: Record; } -/** An admin config document as the API returns it, including its tombstones. */ +/** + * An admin config document as the API returns it, including its tombstones + * and (`expectedRevision`) its revision, `:`. + */ export interface AdminConfigDocResponse { - config: AdminConfig & Pick; + config: AdminConfig & Pick & { revision?: string }; } /** One profile's own config document (WS4). */ @@ -236,6 +244,11 @@ export interface ResolvedConfigData { overrides: ConfigRecord; /** Paths the profile hides from what it inherits. */ tombstones: string[]; + /** + * The profile document's revision (`expectedRevision`): `null` when there is + * none, absent when the server does not report revisions. + */ + revision?: string | null; } /** One setting of a read-only summary: its label ("Group › Setting"), display text and data path. */ diff --git a/src/utils/changes.test.ts b/src/utils/changes.test.ts index 95b2c79c..ea5c665a 100644 --- a/src/utils/changes.test.ts +++ b/src/utils/changes.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest'; import type * as t from '@/types'; import { + toBatchOps, verifyWrite, planConfigChanges, mergedBackChanges, @@ -542,6 +543,22 @@ describe('planConfigChanges', () => { expect(plan.resets).toEqual(['balance.startBalance']); }); + it('widens over a stored tombstone the same batch unhides', () => { + const plan = planConfigChanges( + [ + { op: 'reset', path: 'memory.agent.provider' }, + { op: 'reset', path: 'memory.agent.model' }, + { op: 'untombstone', path: 'memory.agent.id' }, + ], + snapshot({ memory: { agent: { provider: 'openAI', model: 'gpt-4o-mini' } } }, yaml, [ + 'memory.agent.id', + ]), + ); + expect(plan.resets).toEqual(['memory']); + expect(plan.untombstones).toEqual(['memory.agent.id']); + expect(plan.storedAfter).toEqual({}); + }); + it('never widens a reset past a tombstone the same batch writes', () => { const plan = planConfigChanges( [ @@ -574,6 +591,16 @@ describe('planConfigChanges', () => { expect(plan.resets).toEqual(['endpoints.anthropic.models']); }); + it('sends a reset as is to a server that prunes emptied parents itself', () => { + const plan = planConfigChanges( + [{ op: 'reset', path: 'endpoints.anthropic.models.default' }], + snapshot({ endpoints: { anthropic: { models: { default: ['claude-opus-5-5'] } } } }), + { ...BUNDLED_CONFIG_FEATURES, prunesEmptyParents: true }, + ); + expect(plan.resets).toEqual(['endpoints.anthropic.models.default']); + expect(plan.issues).toEqual([]); + }); + it('folds a reset with new descendant values into one set (rename onto the same key)', () => { const stored = { mcpServers: { srv: { type: 'sse', url: 'https://old', title: 'Old' } } }; const plan = planConfigChanges( @@ -666,6 +693,121 @@ describe('planConfigChanges', () => { }); }); +describe('planConfigChanges — change batches', () => { + const batch: t.ConfigFeatures = { + ...BUNDLED_CONFIG_FEATURES, + changesBatch: true, + expectedRevision: true, + }; + const memoryYaml: t.ConfigRecord = { ...yaml, memory: { agent: { id: 'agent_yaml' } } }; + + it('validates a hide alone on the end state, as the batch is judged', () => { + const ops: t.ConfigChange[] = [{ op: 'tombstone', path: 'memory.agent.id' }]; + const legacy = planConfigChanges(ops, snapshot({}, memoryYaml)); + const batched = planConfigChanges(ops, snapshot({}, memoryYaml), batch); + expect(legacy.issues).toEqual([]); + expect(batched.issues.length).toBeGreaterThan(0); + expect(batched.issues.every((issue) => issue.path.startsWith('memory.agent'))).toBe(true); + }); + + it('accepts a union switch whose hide and sets leave a valid section', () => { + const plan = planConfigChanges( + [ + { op: 'set', path: 'memory.agent.provider', value: 'openAI' }, + { op: 'set', path: 'memory.agent.model', value: 'gpt-4o-mini' }, + { op: 'tombstone', path: 'memory.agent.id' }, + ], + snapshot({}, memoryYaml), + batch, + ); + expect(plan.issues).toEqual([]); + }); +}); + +describe('toBatchOps', () => { + const features: t.ConfigFeatures = { + ...BUNDLED_CONFIG_FEATURES, + changesBatch: true, + expectedRevision: true, + prunesEmptyParents: true, + }; + + it('orders unhides, hides, sets, then resets, one unset per removed path', () => { + const plan = planConfigChanges( + [ + { op: 'set', path: 'balance.startBalance', value: 5 }, + { op: 'reset', path: 'balance.refillAmount' }, + { op: 'reset', path: 'balance.enabled' }, + { op: 'untombstone', path: 'balance.refillAmount' }, + { op: 'tombstone', path: 'mcpServers.github' }, + ], + snapshot({ balance: { refillAmount: 3, enabled: false } }, yaml, ['balance.refillAmount']), + features, + ); + expect(toBatchOps(plan)).toEqual([ + { op: 'unset', fieldPath: 'balance.refillAmount' }, + { op: 'tombstone', fieldPath: 'mcpServers.github' }, + { op: 'set', fieldPath: 'balance.startBalance', value: 5 }, + { op: 'unset', fieldPath: 'balance.enabled' }, + ]); + }); + + it('unhides an entry before the edits and hides inside it, which the unhide would remove', () => { + const url = 'https://example.org/mcp'; + const plan = planConfigChanges( + [ + { op: 'untombstone', path: 'mcpServers.github' }, + { op: 'set', path: 'mcpServers.github.url', value: url }, + { op: 'tombstone', path: 'mcpServers.github.headers' }, + ], + snapshot({}, yaml, ['mcpServers.github']), + features, + ); + expect(plan.issues).toEqual([]); + expect(toBatchOps(plan)).toEqual([ + { op: 'unset', fieldPath: 'mcpServers.github' }, + { op: 'tombstone', fieldPath: 'mcpServers.github.headers' }, + { op: 'set', fieldPath: 'mcpServers.github.url', value: url }, + ]); + }); + + it('leaves out an edit inside an entry the same batch hides, which would show it again', () => { + const plan = planConfigChanges( + [ + { op: 'set', path: 'mcpServers.github.url', value: 'https://example.org/mcp' }, + { op: 'set', path: 'balance.startBalance', value: 5 }, + { op: 'tombstone', path: 'mcpServers.github' }, + ], + snapshot({ mcpServers: { github: { url: 'https://stored.example.org/mcp' } } }), + features, + ); + expect(plan).toMatchObject({ issues: [], skipped: [], effective: 2 }); + expect(plan.sets).toEqual([{ fieldPath: 'balance.startBalance', value: 5 }]); + expect(toBatchOps(plan)).toEqual([ + { op: 'tombstone', fieldPath: 'mcpServers.github' }, + { op: 'set', fieldPath: 'balance.startBalance', value: 5 }, + ]); + }); +}); + +describe('toBatchOps — one change per path', () => { + it('leaves out the unset of a path the batch hides anyway', () => { + const ops = toBatchOps({ + sets: [], + resets: ['mcpServers.github'], + tombstones: ['mcpServers.github'], + untombstones: [], + skipped: [], + issues: [], + warnings: [], + storedAfter: {}, + effective: 2, + changed: ['mcpServers.github'], + }); + expect(ops).toEqual([{ op: 'tombstone', fieldPath: 'mcpServers.github' }]); + }); +}); + describe('verifyWrite', () => { it('flags values LibreChat did not store, did not apply, or merges back', () => { const plan = planConfigChanges( diff --git a/src/utils/changes.ts b/src/utils/changes.ts index 191826fa..fa29393e 100644 --- a/src/utils/changes.ts +++ b/src/utils/changes.ts @@ -41,6 +41,7 @@ import { findYamlManagedValues, getWriteSkipReason } from './writes'; import { isBlankText, findBlankText, findUnsafeKeys } from './keys'; import { resolveSubSchema, unwrapSchema } from './schema'; import { scopeGateSkipReason } from './scopes'; +import { usesChangeBatch } from './contract'; /** Whether a write at `path` sets a process-backed (stdio) MCP server field, which the admin API rejects. */ export function isProcessMcpWrite(path: string, value: t.ConfigValue): boolean { @@ -267,8 +268,14 @@ function expandIndexedArraySet(state: PlanState, path: string, value: t.ConfigVa return true; } +/** + * Sorts the ops into the plan. A hide removes what the target stores at and + * under its path, so a set there in the same batch is not written: the hide + * wins, whatever order the two are written in. + */ function collectOps(state: PlanState, ops: t.ConfigChange[]): void { const isProfile = state.snapshot.lowerKind === 'inherited'; + const sets: Array<[string, t.ConfigValue]> = []; for (const op of ops) { const isRemoval = op.op === 'reset' || op.op === 'untombstone' || op.op === 'removeEntry'; const reason = @@ -278,12 +285,16 @@ function collectOps(state: PlanState, ops: t.ConfigChange[]): void { state.skipped.push({ fieldPath: op.path, reason }); continue; } - if (op.op === 'set') collectSet(state, op.path, op.value); + if (op.op === 'set') sets.push([op.path, op.value]); else if (op.op === 'upsertEntry' || op.op === 'removeEntry') addEntryOp(state, op); else if (op.op === 'reset') state.resets.push(op.path); else if (op.op === 'tombstone') state.tombstones.push(op.path); else state.untombstones.push(op.path); } + for (const [path, value] of sets) { + if (state.tombstones.some((hidden) => isAtOrUnder(path, hidden))) continue; + collectSet(state, path, value); + } } function collectSet(state: PlanState, path: string, value: t.ConfigValue): void { @@ -484,10 +495,19 @@ function applyToStored(state: PlanState): t.ConfigRecord { return stored; } -/** Widens each reset to the highest parent it leaves empty, then returns the stored overrides afterwards. */ +/** + * Widens each reset to the highest parent it leaves empty, then returns the + * stored overrides afterwards. A server that prunes emptied parents itself + * (`prunesEmptyParents`) gets each reset as planned. A stored tombstone the + * plan unhides does not stop the widening: the unhide removes it first. + */ function pruneEmptyParents(state: PlanState): t.ConfigRecord { + if (state.features.prunesEmptyParents) return applyToStored(state); const removed = applyToStored(state); - const tombstones = [...state.snapshot.tombstones, ...state.tombstones]; + const kept = state.snapshot.tombstones.filter( + (path) => !state.untombstones.some((shown) => isAtOrUnder(path, shown)), + ); + const tombstones = [...kept, ...state.tombstones]; const widened = state.resets.map((path) => emptiedAncestor(removed, path, tombstones)); state.resets = [...new Set(widened)].filter((path) => !hasAncestorIn(path, widened)); return applyToStored(state); @@ -568,6 +588,53 @@ function isInCollection(path: string): boolean { return findKeyedCollectionEntry(path) !== undefined; } +/** The layer beneath without the values `tombstones` hide. */ +function hideAll(lower: t.ConfigRecord, tombstones: Iterable): t.ConfigRecord { + let result = lower; + for (const path of tombstones) result = removeValueAtPath(result, path); + return result; +} + +/** + * The target's tombstones once a batch is applied: a set clears those at its + * path and above it, a removal those at or below it, and new hides are added. + */ +function tombstonesAfter(state: PlanState, sets: readonly string[]): string[] { + const removals = [...state.resets, ...state.untombstones]; + const kept = state.snapshot.tombstones.filter( + (tombstone) => + !removals.some((path) => isAtOrUnder(tombstone, path)) && + !sets.some((path) => isAtOrUnder(path, tombstone)), + ); + return [...new Set([...kept, ...state.tombstones])]; +} + +/** + * The layer beneath the target before and after the write, as LibreChat + * validates against it. A change batch (`usesChangeBatch`) is judged on its + * end state: the layer without the tombstones the target holds before and + * after it, for every section it writes (a hide alone included). Separate + * field writes go tombstones first, and LibreChat validates a field PATCH + * against the layer without the target's tombstones, so a section the plan + * sets values in is validated without the values it hides; a hide alone is + * not validated by LibreChat, and is not here either. + */ +function validationFloors( + state: PlanState, + sets: readonly string[], +): { before: t.ConfigRecord; after: t.ConfigRecord } { + const { lower, tombstones } = state.snapshot; + if (usesChangeBatch(state.features)) { + return { + before: hideAll(lower, tombstones), + after: hideAll(lower, tombstonesAfter(state, sets)), + }; + } + const setSections = new Set(sets.map(getConfigSection)); + const hidden = state.tombstones.filter((path) => setSections.has(getConfigSection(path))); + return { before: lower, after: hideAll(lower, hidden) }; +} + /** * Whole-section validation of the merged result (lower layer + overrides * after the write), so refinements and required siblings are checked. An @@ -576,17 +643,17 @@ function isInCollection(path: string): boolean { * not block an unrelated write. Collection entries are validated per entry. */ function validateSections(state: PlanState, blocked: ReadonlySet): void { + const sets = [...state.sets.keys()].filter((path) => !blocked.has(path)); let storedAfter = state.snapshot.stored; - for (const [path, value] of state.sets) { - if (!blocked.has(path)) storedAfter = setValueAtPath(storedAfter, path, value); - } + for (const path of sets) storedAfter = setValueAtPath(storedAfter, path, state.sets.get(path)); for (const path of [...state.resets, ...state.tombstones]) { storedAfter = removeStored(storedAfter, path); } const written = [ - ...[...state.sets.keys()].filter((path) => !blocked.has(path)), + ...sets, ...state.resets, ...state.tombstones, + ...(usesChangeBatch(state.features) ? state.untombstones : []), ]; /** LibreChat reads librechat.yaml-only sections from the file alone: removing an override there can't break them. */ const sections = new Set( @@ -595,24 +662,11 @@ function validateSections(state: PlanState, blocked: ReadonlySet): void .map(getConfigSection), ); const reported = new Set(state.issues.map(issueKey)); - const { lower, stored } = state.snapshot; - /** - * LibreChat validates a field PATCH against the layer beneath without the - * target's tombstones (tombstones go first), so a section this batch sets - * values in is validated without the values it hides. A hide alone is not - * validated by LibreChat, and is not here either. - */ - const setSections = new Set( - [...state.sets.keys()].filter((path) => !blocked.has(path)).map(getConfigSection), - ); - const lowerAfter = state.tombstones.reduce( - (current, path) => - setSections.has(getConfigSection(path)) ? removeValueAtPath(current, path) : current, - lower, - ); + const { stored } = state.snapshot; + const floors = validationFloors(state, sets); for (const section of sections) { - const before = mergeConfigLayers(lower[section], stored[section], section); - const after = mergeConfigLayers(lowerAfter[section], storedAfter[section], section); + const before = mergeConfigLayers(floors.before[section], stored[section], section); + const after = mergeConfigLayers(floors.after[section], storedAfter[section], section); const existing = new Set(validateSection(section, before).map(issueKey)); for (const issue of validateSection(section, after)) { if (isInCollection(issue.path) || reported.has(issueKey(issue))) continue; @@ -707,6 +761,31 @@ export function planConfigChanges( }; } +function toUnsetOp(fieldPath: string): t.ConfigBatchOp { + return { op: 'unset', fieldPath }; +} + +/** + * A plan as the changes of one `POST …/changes` batch: unhides, hides, sets, + * then resets, one `unset` per path a hide or set does not already write (a + * batch refuses two changes at one path). An unhide removes everything at and + * under its path, so it goes first: after them, it would remove a set or hide + * inside it (an MCP server shown again with its URL edited). The server + * applies the changes in that order and validates the end state. + */ +export function toBatchOps(plan: t.WritePlan): t.ConfigBatchOp[] { + const written = new Set([...plan.tombstones, ...plan.sets.map(({ fieldPath }) => fieldPath)]); + const unhides = [...new Set(plan.untombstones)].filter((path) => !written.has(path)); + const removed = new Set([...written, ...unhides]); + const resets = [...new Set(plan.resets)].filter((path) => !removed.has(path)); + return [ + ...unhides.map(toUnsetOp), + ...plan.tombstones.map((fieldPath): t.ConfigBatchOp => ({ op: 'tombstone', fieldPath })), + ...plan.sets.map(({ fieldPath, value }): t.ConfigBatchOp => ({ op: 'set', fieldPath, value })), + ...resets.map(toUnsetOp), + ]; +} + /** Keys the admin API redacts on read (secrets and their preview companions). */ function isRedactedKey(key: string): boolean { return isSecretKey(key) || key.endsWith('Preview'); diff --git a/src/utils/contract.ts b/src/utils/contract.ts index 27a8d238..f68e3ab4 100644 --- a/src/utils/contract.ts +++ b/src/utils/contract.ts @@ -1,5 +1,6 @@ import type * as t from '@/types'; import { + ABSENT_REVISION, CONFIG_FEATURE_FLAGS, BUNDLED_CONFIG_RULES, BUNDLED_CONFIG_LIMITS, @@ -45,6 +46,25 @@ export function foldsEndpointReferences(contract: t.ConfigContract): boolean { return contract.rules.titleResolution === 'field'; } +/** + * Whether saves go to `POST …/changes` as one batch with a revision + * precondition: written all at once and validated as a whole, so nothing is + * rolled back by hand and another admin's save is never overwritten unseen. + */ +export function usesChangeBatch(features: t.ConfigFeatures): boolean { + return features.changesBatch && features.expectedRevision; +} + +/** + * The `expectedRevision` that names a stored revision: `absent` for a + * target without a document (`null`), none when the server reports no + * revisions (`undefined`). + */ +export function toRevisionPrecondition(revision: string | null | undefined): string | undefined { + if (revision === undefined) return undefined; + return revision ?? ABSENT_REVISION; +} + /** A copy without the keys whose value is `undefined`, so they don't hide the fallback's. */ function definedOnly(value: T | undefined): Partial { if (!value) return {}; diff --git a/src/utils/validation.ts b/src/utils/validation.ts index 59293720..1ec1bc6f 100644 --- a/src/utils/validation.ts +++ b/src/utils/validation.ts @@ -141,6 +141,8 @@ const ISSUE_MESSAGES: Readonly> = { custom_missing_port: 'must include a port, for example localhost:11434', custom_public_ip: 'is a public IP address: enter a private address, or a hostname that resolves to one', + TOO_MANY_CHANGES: + 'has {{count}} changes, more than the {{limit}} LibreChat takes in one save: save some of them first, then the rest', }; /** diff --git a/src/utils/writes.test.ts b/src/utils/writes.test.ts index fec7a451..26547529 100644 --- a/src/utils/writes.test.ts +++ b/src/utils/writes.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect } from 'vitest'; import type * as t from '@/types'; import { BUNDLED_CONFIG_FEATURES, getWriteRules } from '@/constants'; -import { reportedPath, readWriteReport, getWriteSkipReason } from './writes'; +import { reportedPath, readWriteReport, readBatchReport, getWriteSkipReason } from './writes'; import { createServerContract } from '@/test/fixtures'; import { entryLocation } from './collections'; import { createPathIndex } from './paths'; @@ -124,6 +124,85 @@ describe('readWriteReport', () => { }); }); +describe('readBatchReport', () => { + it('reads what a change batch skipped and what the merge ignores, whatever the flags', () => { + const body: t.PatchFieldsResponse = { + skipped: [ + { opIndex: 0, segments: ['turnstile'], reason: 'base_only_section' }, + { opIndex: 1, segments: ['interface', 'prompts'], reason: 'interface_permission' }, + ], + ignored: [ + { opIndex: 2, segments: ['mcpServers', 'everything'], reason: 'process_mcp_server' }, + { + path: 'webSearch.jinaApiKey', + segments: ['webSearch', 'jinaApiKey'], + reason: 'secret_rebinding', + }, + ], + }; + const changed = createPathIndex(['turnstile', 'interface', 'mcpServers.everything']); + expect(readBatchReport(body, changed)).toEqual([ + { fieldPath: 'turnstile', reason: 'baseOnly' }, + { fieldPath: 'interface.prompts', reason: 'permission' }, + { fieldPath: 'mcpServers.everything', reason: 'processServer' }, + ]); + }); + + describe('for the locations the save changed', () => { + const rebinding = (segments: string[]): t.ReportedSkip => ({ + path: segments.join('.'), + segments, + reason: 'secret_rebinding', + }); + const body: t.PatchFieldsResponse = { + skipped: [ + { opIndex: 0, segments: ['endpoints', 'custom'], reason: 'base_only_path' }, + { + opIndex: 1, + segments: ['endpoints', 'custom', 'GPT', 'headers'], + reason: 'base_only_path', + }, + { + opIndex: 1, + segments: ['endpoints', 'custom', 'Moonshot', 'headers'], + reason: 'base_only_path', + }, + ], + ignored: [ + { opIndex: 2, segments: ['mcpServers', 'everything'], reason: 'process_mcp_server' }, + rebinding(['endpoints', 'custom', 'Moonshot', 'apiKey']), + rebinding(['mcpServers', 'github', 'apiKey', 'key']), + rebinding(['ocr', 'apiKey']), + ], + }; + const shownFor = (changed: string[]) => + readBatchReport(body, createPathIndex(changed)).map(({ fieldPath }) => fieldPath); + + it('keeps a dropped credential only when the save changed its entry or the object holding it', () => { + expect(shownFor(['endpoints.custom.Moonshot'])).toEqual([ + 'endpoints.custom', + 'endpoints.custom.Moonshot.headers', + 'endpoints.custom.Moonshot.apiKey', + ]); + expect(shownFor(['mcpServers.github.url'])).toEqual(['mcpServers.github.apiKey.key']); + expect(shownFor(['ocr.baseURL'])).toEqual(['ocr.apiKey']); + }); + + it('does not repeat a dropped credential or a part of another entry for a save elsewhere', () => { + expect(shownFor(['balance.startBalance'])).toEqual([]); + expect(shownFor(['endpoints.custom.GPT'])).toEqual([ + 'endpoints.custom', + 'endpoints.custom.GPT.headers', + ]); + expect(shownFor(['mcpServers.cloudflare.url', 'speech.tts.openai.url'])).toEqual([]); + }); + + it('keeps a part reported under a server the save changed, whatever its reason', () => { + expect(shownFor(['mcpServers.everything.url'])).toEqual(['mcpServers.everything']); + }); + }); +}); + describe('reportedPath', () => { it('names a dotted record key by its record, and a location by its segments', () => { expect( diff --git a/src/utils/writes.ts b/src/utils/writes.ts index 95fa4192..57a1fde5 100644 --- a/src/utils/writes.ts +++ b/src/utils/writes.ts @@ -78,17 +78,18 @@ export function reportedPath(location: t.ReportedLocation): string { return (dotted === -1 ? segments : segments.slice(0, Math.max(dotted, 1))).join('.'); } +/** The reported items `keep` accepts that name a location, with the panel's reasons. */ function toSkippedChanges( items: t.ReportedSkip[] | undefined, fallback: t.SkipReason, + keep: (item: t.ReportedSkip) => boolean, ): t.SkippedChange[] { if (!Array.isArray(items)) return []; - return items - .map((item) => ({ - fieldPath: reportedPath(item), - reason: REPORTED_SKIP_REASONS[item.reason] ?? fallback, - })) - .filter((item) => item.fieldPath !== ''); + return items.flatMap((item): t.SkippedChange[] => { + const fieldPath = reportedPath(item); + if (fieldPath === '' || !keep(item)) return []; + return [{ fieldPath, reason: REPORTED_SKIP_REASONS[item.reason] ?? fallback }]; + }); } /** A reported location key by key: its `segments`, else its dotted `path`. */ @@ -114,26 +115,55 @@ function credentialOwner(segments: readonly string[]): string { /** * What a write's reply says did not take effect, with the panel's reasons: - * each location it did not store (`skipped`, from a server with - * `writeReport`), then each stored value the runtime merge leaves out - * (`ignored`: with `writeReport`, or a credential `secretRebinding` drops). - * A server without them reports neither, and nothing is read. `ignored` - * describes the whole stored document, so with the locations the save - * changes (`changed`, `WritePlan.changed`) only an entry whose owner the save - * changed is kept (`credentialOwner`: the custom endpoint, MCP server or - * object it is set in). A save elsewhere, even in the same section, does not - * repeat it, and one that no longer applies is not reported. + * each location it did not store (`skipped`), then each stored value the + * runtime merge leaves out (`ignored`). `ignored` describes the whole stored + * document, and a value sent as a whole collection can carry parts the save + * did not change, so with the locations the save changes (`changed`, + * `WritePlan.changed`) only an item whose owner the save changed is kept + * (`credentialOwner`: the custom endpoint, MCP server or object it is set + * in). A location a write addressed as a whole is one the save changed, so + * it is always kept. A save elsewhere, even in the same section, does not + * repeat an item, and one that no longer applies is not reported. + */ +function collectReport( + body: t.PatchFieldsResponse, + lists: { skipped: boolean; ignored: boolean }, + changed?: t.PathIndex, +): t.SkippedChange[] { + const owned = (item: t.ReportedSkip): boolean => + !changed || pathIndexOverlaps(changed, credentialOwner(segmentsOf(item))); + return [ + ...(lists.skipped ? toSkippedChanges(body.skipped, 'notStored', owned) : []), + ...(lists.ignored ? toSkippedChanges(body.ignored, 'notApplied', owned) : []), + ]; +} + +/** + * What a field write's reply says did not take effect (`collectReport`): + * `skipped` from a server with `writeReport`, `ignored` with `writeReport` + * or from one that drops credentials (`secretRebinding`). */ export function readWriteReport( body: t.PatchFieldsResponse, features: t.ConfigFeatures, changed?: t.PathIndex, ): t.SkippedChange[] { - const skipped = features.writeReport ? toSkippedChanges(body.skipped, 'notStored') : []; - const readsIgnored = features.writeReport || features.secretRebinding; - if (!readsIgnored || !Array.isArray(body.ignored)) return skipped; - const ignored = changed - ? body.ignored.filter((item) => pathIndexOverlaps(changed, credentialOwner(segmentsOf(item)))) - : body.ignored; - return [...skipped, ...toSkippedChanges(ignored, 'notApplied')]; + const lists = { + skipped: features.writeReport, + ignored: features.writeReport || features.secretRebinding, + }; + return collectReport(body, lists, changed); +} + +/** + * What a `POST …/changes` reply says did not take effect (`changesBatch`, + * `collectReport`): a batch reply always lists what it did not store + * (`skipped`) and what the runtime merge does not use (`ignored`, its + * dropped credentials for the whole stored document). + */ +export function readBatchReport( + body: t.PatchFieldsResponse, + changed: t.PathIndex, +): t.SkippedChange[] { + return collectReport(body, { skipped: true, ignored: true }, changed); }