diff --git a/apps/desktop/src/app/settings/config-settings.test.tsx b/apps/desktop/src/app/settings/config-settings.test.tsx new file mode 100644 index 000000000000..675d5f1ee7f1 --- /dev/null +++ b/apps/desktop/src/app/settings/config-settings.test.tsx @@ -0,0 +1,97 @@ +import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import { cleanup, render, screen, waitFor } from '@testing-library/react' +import { atom } from 'nanostores' +import { createRef } from 'react' +import { MemoryRouter } from 'react-router' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const getHermesConfigRecord = vi.fn() +const getHermesConfigSchema = vi.fn() +const saveHermesConfig = vi.fn() +const getElevenLabsVoices = vi.fn() + +vi.mock('@/hermes', () => ({ + getHermesConfigRecord: () => getHermesConfigRecord(), + getHermesConfigSchema: () => getHermesConfigSchema(), + saveHermesConfig: (config: unknown, profile?: string) => saveHermesConfig(config, profile), + getElevenLabsVoices: () => getElevenLabsVoices(), + setApiRequestProfile: () => {} +})) + +vi.mock('../hooks/use-on-profile-switch', () => ({ + useOnProfileSwitch: () => {} +})) + +// The real stores pull in the gateway/profile stack, which needs a live +// backend connection. This page only reads the "applies to" scope override +// and the repo-discovery signature, neither of which this test touches. +vi.mock('@/store/settings-scope', () => ({ + $settingsScopeOverride: atom(null) +})) + +vi.mock('@/store/projects', () => ({ + repoDiscoveryPolicyFromConfig: () => ({ enabled: true, roots: [], exclude_paths: [] }), + repoDiscoveryPolicySignature: (policy: unknown) => JSON.stringify(policy), + scanAndRecordRepos: vi.fn().mockResolvedValue(undefined) +})) + +beforeEach(() => { + getElevenLabsVoices.mockResolvedValue({ available: false }) + getHermesConfigSchema.mockResolvedValue({ fields: {} }) + saveHermesConfig.mockResolvedValue({ ok: true }) +}) + +afterEach(() => { + cleanup() + vi.clearAllMocks() +}) + +async function renderConfigSettings() { + const { ConfigSettings } = await import('./config-settings') + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + const importInputRef = createRef() + + render( + + + + + + ) + + return { importInputRef } +} + +describe('ConfigSettings autosave', () => { + it('sends a later revert instead of diffing it away against the stale page-load baseline', async () => { + getHermesConfigRecord.mockResolvedValue({ checkpoints: { enabled: false }, other: 'untouched' }) + + vi.useFakeTimers({ shouldAdvanceTime: true }) + + try { + await renderConfigSettings() + + const toggle = await screen.findByRole('switch') + + // Edit: flip checkpoints.enabled on, let the debounced autosave fire. + toggle.click() + await vi.advanceTimersByTimeAsync(700) + + await waitFor(() => expect(saveHermesConfig).toHaveBeenCalledTimes(1)) + expect(saveHermesConfig.mock.calls[0][0]).toEqual({ checkpoints: { enabled: true } }) + + // Revert: flip it back to its original value and let autosave fire again. + toggle.click() + await vi.advanceTimersByTimeAsync(700) + + await waitFor(() => expect(saveHermesConfig).toHaveBeenCalledTimes(2)) + // Must still explicitly send the reverted value — diffing against the + // never-advanced page-load baseline would produce an empty patch here + // (the field is back to its original value) and leave disk stuck at + // `enabled: true` from the first save. + expect(saveHermesConfig.mock.calls[1][0]).toEqual({ checkpoints: { enabled: false } }) + } finally { + vi.useRealTimers() + } + }) +}) diff --git a/apps/desktop/src/app/settings/config-settings.tsx b/apps/desktop/src/app/settings/config-settings.tsx index 77d501b7316a..bf6e21fa0816 100644 --- a/apps/desktop/src/app/settings/config-settings.tsx +++ b/apps/desktop/src/app/settings/config-settings.tsx @@ -33,6 +33,7 @@ import { PanelEmpty } from '../overlays/panel' import { ConfigField } from './config-field' import { clearsEnabledToolsets, + diffConfig, enumOptionsFor, getNested, isExternalMemoryProvider, @@ -121,11 +122,21 @@ function ConfigSettingsInner({ // Seed the local draft once, the first time the shared record lands. // Background refetches thereafter must not clobber in-progress edits. const configSeeded = useRef(false) + // Snapshot of the record as it was when the draft was seeded. Autosave + // diffs the draft against this (not against disk) so a field the user + // never touched — possibly changed out-of-band by `hermes config set` + // while this page sat open — is never resent with its stale value. + const configBaselineRef = useRef(null) + // Serializes autosave requests so an older save that's still in flight can't + // resolve after a newer one and re-advance the baseline / cache with stale + // data — each save's diff+request only starts once the previous one lands. + const saveQueueRef = useRef>(Promise.resolve()) // eslint-disable-next-line no-restricted-syntax -- legitimate non-atom ref write (see eslint rule comment) useEffect(() => { if (loadedConfig && !configSeeded.current) { configSeeded.current = true + configBaselineRef.current = loadedConfig savedDiscoverySignatureRef.current = repoDiscoveryPolicySignature(repoDiscoveryPolicyFromConfig(loadedConfig)) setConfig(loadedConfig) } @@ -137,10 +148,12 @@ function ConfigSettingsInner({ // the pending debounced autosave is cancelled by its effect cleanup. useOnProfileSwitch(() => { configSeeded.current = false + configBaselineRef.current = null savedDiscoverySignatureRef.current = undefined setConfig(null) saveVersionRef.current = 0 setSaveVersion(0) + saveQueueRef.current = Promise.resolve() }) useEffect(() => { @@ -173,25 +186,37 @@ function ConfigSettingsInner({ } const v = saveVersion + const snapshot = config const t = window.setTimeout(() => { - void (async () => { + // Chained onto the queue (not fired directly) so an older save that's + // still awaiting its response can't land after this one and undo its + // baseline advance — each save's diff is computed once its predecessor + // has fully resolved. + saveQueueRef.current = saveQueueRef.current.then(async () => { try { - const result = await saveHermesConfig(config, scopeProfile ?? undefined) + const patch = diffConfig(configBaselineRef.current ?? {}, snapshot) + const result = await saveHermesConfig(patch, scopeProfile ?? undefined) if (!result.ok) { throw new Error(c.autosaveFailed) } + // The saved snapshot becomes the new baseline, so the next autosave + // diffs against what's actually on disk instead of the page-load + // (or last-baseline) copy — otherwise reverting a field to its + // pre-save value diffs to nothing and the revert never reaches disk. + configBaselineRef.current = snapshot + // Mirror the saved record into the shared cache so MCP/model surfaces // reflect the edit without their own refetch. - writeConfigCache(config) + writeConfigCache(snapshot) if (saveVersionRef.current === v) { // The repo-discovery scan reads the ACTIVE profile's workspace // policy; skip it when this page is editing another profile. if (scopeProfile == null) { - const discoverySignature = repoDiscoveryPolicySignature(repoDiscoveryPolicyFromConfig(config)) + const discoverySignature = repoDiscoveryPolicySignature(repoDiscoveryPolicyFromConfig(snapshot)) if (savedDiscoverySignatureRef.current !== discoverySignature) { savedDiscoverySignatureRef.current = discoverySignature @@ -206,7 +231,7 @@ function ConfigSettingsInner({ notifyError(err, c.autosaveFailed) } } - })() + }) }, 550) return () => window.clearTimeout(t) diff --git a/apps/desktop/src/app/settings/helpers.test.ts b/apps/desktop/src/app/settings/helpers.test.ts index 8e7a300038fb..90c00e50e6a7 100644 --- a/apps/desktop/src/app/settings/helpers.test.ts +++ b/apps/desktop/src/app/settings/helpers.test.ts @@ -6,6 +6,7 @@ import { FIELD_DESCRIPTIONS, FIELD_LABELS, SECTIONS } from './constants' import { defineFieldCopy, fieldCopyForSchemaKey, schemaKeyToFieldCopyKey } from './field-copy' import { clearsEnabledToolsets, + diffConfig, enumOptionsFor, getNested, isExternalMemoryProvider, @@ -404,4 +405,44 @@ describe('settings helpers', () => { expect(clearsEnabledToolsets(prev, next)).toBe(false) }) }) + + describe('diffConfig', () => { + it('omits a top-level key the draft never touched', () => { + // The autosave baseline is a snapshot taken when Settings opened. A key + // an agent set via `hermes config set` while the page sat open must not + // come back in the patch just because it's still present in the draft. + const baseline: HermesConfigRecord = { fallback_providers: ['nara1'], timezone: 'UTC' } + const draft: HermesConfigRecord = { fallback_providers: ['nara1'], timezone: 'America/New_York' } + + expect(diffConfig(baseline, draft)).toEqual({ timezone: 'America/New_York' }) + }) + + it('includes a nested key only when it actually changed, leaving siblings out', () => { + const baseline: HermesConfigRecord = { display: { personality: 'default', show_reasoning: true } } + const draft: HermesConfigRecord = { display: { personality: 'default', show_reasoning: false } } + + expect(diffConfig(baseline, draft)).toEqual({ display: { show_reasoning: false } }) + }) + + it('sends a new key that was absent from the baseline', () => { + const baseline: HermesConfigRecord = {} + const draft: HermesConfigRecord = { timezone: 'UTC' } + + expect(diffConfig(baseline, draft)).toEqual({ timezone: 'UTC' }) + }) + + it('returns an empty object when the draft matches the baseline exactly', () => { + const baseline: HermesConfigRecord = { toolsets: ['memory'], display: { personality: 'default' } } + const draft: HermesConfigRecord = { toolsets: ['memory'], display: { personality: 'default' } } + + expect(diffConfig(baseline, draft)).toEqual({}) + }) + + it('treats an array as a whole value, not diffed element by element', () => { + const baseline: HermesConfigRecord = { toolsets: ['memory', 'terminal'] } + const draft: HermesConfigRecord = { toolsets: ['memory'] } + + expect(diffConfig(baseline, draft)).toEqual({ toolsets: ['memory'] }) + }) + }) }) diff --git a/apps/desktop/src/app/settings/helpers.ts b/apps/desktop/src/app/settings/helpers.ts index 8da0c92f0585..b51162f8549a 100644 --- a/apps/desktop/src/app/settings/helpers.ts +++ b/apps/desktop/src/app/settings/helpers.ts @@ -97,6 +97,43 @@ export function getNested(obj: HermesConfigRecord, path: string): unknown { return cur } +const isPlainObject = (value: unknown): value is Record => + typeof value === 'object' && value !== null && !Array.isArray(value) + +/** + * Structural diff between two config snapshots: an object holding only the + * branches of `next` that changed relative to `base`. Plain-object values are + * compared key by key so editing one field doesn't drag its untouched + * siblings back into the result; arrays and scalars are compared as whole + * values. + * + * The autosave path sends this instead of the full draft so a field the user + * never touched — one an agent may have changed via `hermes config set` + * while Settings was open with a stale snapshot — is never resent with its + * now-stale value. `PUT /api/config` deep-merges onto disk, so an omitted + * key keeps whatever is currently there. + */ +export function diffConfig(base: HermesConfigRecord, next: HermesConfigRecord): HermesConfigRecord { + const patch: HermesConfigRecord = {} + + for (const key of Object.keys(next)) { + const baseValue = base[key] + const nextValue = next[key] + + if (isPlainObject(baseValue) && isPlainObject(nextValue)) { + const nested = diffConfig(baseValue, nextValue) + + if (Object.keys(nested).length > 0) { + patch[key] = nested + } + } else if (JSON.stringify(baseValue) !== JSON.stringify(nextValue)) { + patch[key] = nextValue + } + } + + return patch +} + /** * True when an edit clears the entire "Enabled Toolsets" list — i.e. the * previous config had a non-empty toolsets array and the next one is an diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 439d8cd66c3a..45f29e6262f3 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -7524,15 +7524,22 @@ def _denormalize_config_from_web(config: Dict[str, Any]) -> Dict[str, Any]: string; the rest is preserved transparently. Also handles ``model_context_length`` — writes it back into the model dict - as ``context_length``. A value of 0 or absent means "auto-detect" (omitted - from the dict so get_model_context_length() uses its normal resolution). + as ``context_length``. A value of 0 means "auto-detect" (omitted from the + dict so get_model_context_length() uses its normal resolution). ``config`` + may be a partial update (e.g. the Settings autosave diff) that omits + ``model_context_length`` entirely when the user didn't touch it — that + must leave the on-disk override untouched, not get treated the same as an + explicit 0 and cleared. """ config = dict(config) # Remove any _model_meta that might have leaked in (shouldn't happen # with the stripped GET response, but be defensive) config.pop("_model_meta", None) - # Extract and remove model_context_length before processing model + # Extract and remove model_context_length before processing model, but + # remember whether it was actually present: a partial update omitting the + # key means "unchanged", which is different from an explicit 0. + ctx_sent = "model_context_length" in config ctx_override = config.pop("model_context_length", 0) if not isinstance(ctx_override, int): try: @@ -7541,50 +7548,59 @@ def _denormalize_config_from_web(config: Dict[str, Any]) -> Dict[str, Any]: ctx_override = 0 model_val = config.get("model") - if isinstance(model_val, str) and model_val: + if (isinstance(model_val, str) and model_val) or ctx_sent: # Read the current disk config to recover model subkeys try: disk_config = load_config() disk_model = disk_config.get("model") if isinstance(disk_model, dict): - prev_default = str(disk_model.get("default") or "").strip() - prev_provider = str(disk_model.get("provider") or "").strip() - # When the model name actually changed, re-detect which - # provider serves it. The Config-page Model field is a flat - # string with no provider info, so without this a user who - # picks an OpenRouter model while their default provider is - # ollama-local keeps the stale provider and 404s. Only fires - # on a real model change so saving unrelated config fields - # never overwrites an explicit provider. - if model_val != prev_default and prev_provider: - new_provider, resolved_model = _infer_provider_on_model_change( - model_val, prev_provider - ) - if new_provider and new_provider.strip().lower() != prev_provider.lower(): - # Route through the canonical assignment chokepoints so - # the model is normalized for the new provider and stale - # base_url/api_mode/api_key are cleared on the switch - # (and preserved on a same-provider re-pick). - norm_provider, norm_model = _normalize_main_model_assignment( - new_provider, resolved_model - ) - disk_model = _apply_main_model_assignment( - disk_model, norm_provider, norm_model + if isinstance(model_val, str) and model_val: + prev_default = str(disk_model.get("default") or "").strip() + prev_provider = str(disk_model.get("provider") or "").strip() + # When the model name actually changed, re-detect which + # provider serves it. The Config-page Model field is a flat + # string with no provider info, so without this a user who + # picks an OpenRouter model while their default provider is + # ollama-local keeps the stale provider and 404s. Only fires + # on a real model change so saving unrelated config fields + # never overwrites an explicit provider. + if model_val != prev_default and prev_provider: + new_provider, resolved_model = _infer_provider_on_model_change( + model_val, prev_provider ) - model_val = norm_model - # Preserve all subkeys, update default with the new value - disk_model["default"] = model_val - # Write context_length into the model dict (0 = remove/auto) - if ctx_override > 0: - disk_model["context_length"] = ctx_override - else: - disk_model.pop("context_length", None) + if new_provider and new_provider.strip().lower() != prev_provider.lower(): + # Route through the canonical assignment chokepoints so + # the model is normalized for the new provider and stale + # base_url/api_mode/api_key are cleared on the switch + # (and preserved on a same-provider re-pick). + norm_provider, norm_model = _normalize_main_model_assignment( + new_provider, resolved_model + ) + disk_model = _apply_main_model_assignment( + disk_model, norm_provider, norm_model + ) + model_val = norm_model + # Preserve all subkeys, update default with the new value + disk_model["default"] = model_val + # Write context_length into the model dict (0 = remove/auto), + # but only when the payload actually carried the key. + if ctx_sent: + if ctx_override > 0: + disk_model["context_length"] = ctx_override + else: + disk_model.pop("context_length", None) config["model"] = disk_model - # Model was previously a bare string — upgrade to dict if - # user is setting a context_length override - elif ctx_override > 0: + # Model was previously a bare string (or absent) — upgrade to a + # dict if the user is setting a context_length override. + elif ctx_sent and ctx_override > 0: + if isinstance(model_val, str) and model_val: + default = model_val + elif isinstance(disk_model, str) and disk_model: + default = disk_model + else: + default = "" config["model"] = { - "default": model_val, + "default": default, "context_length": ctx_override, } except Exception: diff --git a/tests/hermes_cli/test_web_server.py b/tests/hermes_cli/test_web_server.py index e8487de5fb69..cfa4f06f9712 100644 --- a/tests/hermes_cli/test_web_server.py +++ b/tests/hermes_cli/test_web_server.py @@ -2891,6 +2891,43 @@ def test_denormalize_writes_context_length_into_model_dict(self): assert result["model"]["context_length"] == 100000 assert "model_context_length" not in result # virtual field removed + def test_denormalize_context_length_alone_is_applied(self): + """The Settings autosave now sends a diff, not the full draft: editing + only the Context Window control must not omit ``model`` and thereby + drop the context_length edit on the floor (#89597 review).""" + from hermes_cli.web_server import _denormalize_config_from_web + from hermes_cli.config import save_config + + save_config({ + "model": {"default": "anthropic/claude-sonnet-4", "provider": "anthropic", + "context_length": 100000} + }) + + result = _denormalize_config_from_web({"model_context_length": 200000}) + assert isinstance(result["model"], dict) + assert result["model"]["context_length"] == 200000 + assert result["model"]["default"] == "anthropic/claude-sonnet-4" + + def test_denormalize_model_alone_preserves_context_length(self): + """The mirror case: editing only the Model field must not silently + wipe an existing context_length override just because the diff omits + the unrelated model_context_length key (#89597 review). + + No ``provider`` on disk here on purpose: that keeps this test isolated + to the diff-omission bug rather than the separate, pre-existing (and + intentional, see ``_apply_main_model_assignment``) behavior where a + real provider switch drops the context_length override.""" + from hermes_cli.web_server import _denormalize_config_from_web + from hermes_cli.config import save_config + + save_config({ + "model": {"default": "anthropic/claude-sonnet-4", "context_length": 150000} + }) + + result = _denormalize_config_from_web({"model": "anthropic/claude-opus-4.6"}) + assert result["model"]["context_length"] == 150000 + assert result["model"]["default"] == "anthropic/claude-opus-4.6" + class TestDenormalizeProviderSwitch: """The flat Config-page Model field carries no provider info. When the