Skip to content

fix(desktop): stop Settings autosave from clobbering out-of-band config edits - #89645

Open
chelsealong wants to merge 3 commits into
NousResearch:mainfrom
chelsealong:fix/89597-settings-autosave-stale-overwrite
Open

fix(desktop): stop Settings autosave from clobbering out-of-band config edits#89645
chelsealong wants to merge 3 commits into
NousResearch:mainfrom
chelsealong:fix/89597-settings-autosave-stale-overwrite

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

Fixes #89597.

Problem

The issue reports that on Windows, the desktop Settings UI has priority over
config.yaml: an edit an agent makes via hermes config set gets silently
reverted the next time Settings is touched, leaving no reliable
programmatic config path.

Two of the issue's three sub-problems are already resolved on main:

What's left, and what this PR fixes, is the actual clobbering mechanism —
and it isn't Windows-specific; the code path is shared across every
platform, so the issue title just reflects the reporter's environment.

Root cause

ConfigSettingsInner (apps/desktop/src/app/settings/config-settings.tsx)
seeds its local editable draft once from the config record when Settings
opens, and deliberately does not re-seed it from background refetches while
the page stays open (configSeeded ref — this is intentional, so in-progress
edits aren't clobbered by a stale refetch).

The debounced autosave, however, always sent the entire draft object on
every save:

const result = await saveHermesConfig(config, scopeProfile ?? undefined)

The backend (hermes_cli/web_server.py::update_config) deep-merges the
incoming body onto a fresh read of disk specifically so a field the frontend
doesn't know about (e.g. custom_providers) survives an untouched round
trip. But every schema-known field — including fallback_providers, the
exact field the issue names — is always present in the draft, so it's always
"explicitly sent," and the deep-merge degenerates into a full overwrite for
it.

Concretely: open Settings, have an agent run
hermes config set fallback_providers '[...]' in the meantime, then edit any
unrelated field in the still-open Settings page (e.g. toggle a switch on
another section). The debounced autosave fires 550ms later and writes the
seed-time value of fallback_providers back to disk, discarding the
agent's change — with no error, no conflict, nothing to signal it happened.

Fix

Snapshot the record at seed time (configBaselineRef) and, on autosave,
diff the current draft against that baseline (diffConfig, new pure helper
in helpers.ts). Only the branches that actually changed are sent. Plain
objects are diffed recursively (so a sibling under the same top-level key
that the user never touched stays out of the patch too); arrays and scalars
are compared as whole values. A field nobody edited locally is now never
resent, so the backend's existing deep-merge genuinely protects it instead
of being handed a full copy of its own stale value.

configBaselineRef resets alongside configSeeded on a profile switch, so
it always tracks "the record as of this page's most recent seed."

Follow-up: a hidden model / model_context_length coupling

Review caught a real regression in the initial version of this fix.
model and model_context_length are two virtual top-level fields that
_normalize_config_for_web synthesizes from a single on-disk model dict
(model.default and model.context_length). _denormalize_config_from_web
reverses that, but it only ever wrote model_context_length back into the
model dict inside the branch gated on model also being present in the
payload:

model_val = config.get("model")
if isinstance(model_val, str) and model_val:
    ...
    if ctx_override > 0:
        disk_model["context_length"] = ctx_override
    ...

Under the old full-draft autosave this was never a problem — model was
always present, so the branch always ran. Under diffConfig, editing the
Context Window control alone (a real field, one of only two in the Model
section) produces a patch containing only model_context_length; model
is absent, the branch is skipped, and the edit is silently discarded before
it ever reaches disk.

The mirror case regressed too, more subtly: editing model alone now
omits model_context_length from the diff. The old code treated a missing
key the same as an explicit 0 and popped any existing context_length
off the disk model — so saving an unrelated model change would silently
wipe a context-length override the user never touched.

Fixed by tracking whether model_context_length was actually present in
the payload (ctx_sent) and only mutating context_length when it was,
independent of whether model also changed in the same payload. Added two
regression tests in tests/hermes_cli/test_web_server.py
(TestModelContextLength) exercising the real (mocked-disk)
_denormalize_config_from_web path directly — one for each direction —
and confirmed both fail on the pre-fix code (KeyError) and pass after.

Test plan

Added describe('diffConfig', ...) to
apps/desktop/src/app/settings/helpers.test.ts (5 cases): omits an untouched
top-level key, includes a nested key only when it changed while leaving
siblings out, includes a brand-new key, returns {} for an unchanged draft,
and treats arrays as whole values.

Verified the tests fail without the fix:

$ git checkout HEAD~1 -- apps/desktop/src/app/settings/helpers.ts
$ npx vitest run src/app/settings/helpers.test.ts
 Test Files  1 failed (1)
      Tests  5 failed | 36 passed (41)
 TypeError: diffConfig is not a function
$ git checkout HEAD -- apps/desktop/src/app/settings/helpers.ts

Full suite green with the fix applied:

$ npx vitest run src/app/settings/
 Test Files  31 passed (31)
      Tests  297 passed (297)

$ npx tsc -p . --noEmit          # clean, no output
$ npx eslint src/app/settings/config-settings.tsx src/app/settings/helpers.ts src/app/settings/helpers.test.ts   # clean
$ npx prettier --check src/app/settings/config-settings.tsx src/app/settings/helpers.ts src/app/settings/helpers.test.ts
All matched files use Prettier code style!

Backend regression tests (scripts/run_tests.sh tests/hermes_cli/test_web_server.py):

166 passed, 1 skipped

Confirmed the two new tests fail on the pre-fix code:

$ git stash push -- hermes_cli/web_server.py   # keep only the new tests
$ scripts/run_tests.sh tests/hermes_cli/test_web_server.py -k \
    "test_denormalize_context_length_alone_is_applied or test_denormalize_model_alone_preserves_context_length"
2 failed  (KeyError: 'model', KeyError: 'context_length')
$ git stash pop                                # restore the fix

ruff check clean on both changed files; ty check hermes_cli/web_server.py
shows only pre-existing, unrelated diagnostics (Windows-only stubs and an
optional uvicorn import), none near the changed lines.

Manual testing on Windows itself wasn't possible from this environment; the
affected code path (config-settings.tsx, use-config-record.ts,
hermes.ts, web_server.py) has no OS branching, so the fix and its test
coverage apply identically on macOS/Linux/Windows.

Disclosure

This PR was prepared with AI assistance (Claude Code), with the root cause,
fix, and tests verified by running the actual test suite, typecheck, and
lint locally as shown above.

…ig edits

ConfigSettingsInner seeds its local draft once from the config record and
never re-seeds it while the page stays open, but every autosave PUT still
sent the entire draft. Since PUT /api/config deep-merges onto disk, that
degenerates into a full overwrite for every field the UI's schema knows
about: if `hermes config set` (or another profile/session) changes a
schema-known key like fallback_providers while Settings is open, the next
autosave — triggered by editing any unrelated field — writes the stale
seed-time value back over it.

Diff the draft against the seed-time baseline and send only the changed
branches, so an untouched key is never resent and the backend's deep-merge
actually protects it.
…iped

_denormalize_config_from_web only wrote model_context_length into the
on-disk model dict inside the branch gated on `model` also being present
in the payload. That was harmless when the frontend always sent the full
config, but the prior commit switched Settings autosave to send only the
diff (diffConfig), so editing the Context Window control alone omits
`model` from the payload and the context-length edit is silently thrown
away. The mirror case regressed too: editing `model` alone now omits
model_context_length from the diff, and the old code treated that missing
key the same as an explicit 0, wiping an existing context_length override
that the user never touched.

Track whether model_context_length was actually present in the payload
and only mutate context_length when it was, independent of whether
`model` also changed.
@alt-glitch alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/desktop Electron desktop app (apps/desktop/*) comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 19, 2026
@egilewski

Copy link
Copy Markdown
Contributor

suggesting changes

The configuration write-path security invariants remain intact, but one autosave correctness blocker remains: after a successful save, the autosave baseline is not advanced. A later edit that restores a field to its pre-save value is compared with the original page-load snapshot, so the revert can produce an empty patch and leave the saved value unchanged. Advance the baseline only after each accepted save, preserve request order for overlapping saves, and add regression coverage for edit, save, then revert.

Security evidence:

  • trust boundary: Desktop settings state and imported JSON are untrusted renderer inputs. They flow through the authenticated configuration write path to durable configuration storage. The change adds no route, authentication, profile-resolution, command-execution, or network sink.
  • source/sink/invariant: Autosave sends only changed scalar/object branches; arrays remain atomic values, and omitted keys are preserved by server deep merge. An omitted model_context_length remains unchanged, while an explicit zero/non-positive value clears it; provider switching remains gated on an actual model change. Existing endpoint credential cleanup remains on the canonical assignment path.
  • current-main reproduction: Before this change, context-only updates could be dropped and model-only updates could clear an existing context override. The new presence-sensitive handling distinguishes omission from explicit zero.
  • PR-head or patch-replay validation: Focused model-context and provider-switch tests pass on the reviewed change.
  • positive/negative cases: Coverage includes context-only updates, model-only updates preserving context, provider switching, context override alongside a switch, omitted context keys, explicit zero clearing, bare-string models, non-dict disk models, and non-string model inputs.
  • residual bypass search: The authenticated endpoint and related callers preserve profile routing and credential cleanup; no alternate write path or new secret, shell-execution, or cross-profile exposure was found.
  • reviewer validation: Focused tests and direct probes covered model preservation, context-only application, explicit-zero clearing, and the autosave diff invariants.

Review setup: I reviewed a run-owned local rebase or patch replay against current GitHub main because the submitted branch is stale or conflicted; this does not mean the submitted branch itself merges cleanly.

Not checked:

  • Broader config round-trip validation
  • Ruff validation
  • Desktop TypeScript/Vitest validation

Signed: GPT-5.6-luna-max in Codex

Without this, diffConfig kept comparing against the page-load snapshot
forever, so reverting a field to its original value produced an empty
patch and left the earlier (now-stale) save on disk. Saves are now
queued so an older in-flight request can't resolve after a newer one
and re-advance the baseline with stale data.
@chelsealong

Copy link
Copy Markdown
Contributor Author

Addressed: the autosave baseline now advances to the saved snapshot after each accepted save, so a later edit that reverts a field to its pre-save value is diffed against the actual on-disk state instead of the original page-load snapshot (previously that produced an empty patch and left the stale saved value on disk). Autosave requests are now queued so an older in-flight save can't resolve after a newer one and re-advance the baseline with stale data. Added a regression test (config-settings.test.tsx) covering edit → save → revert, verified it fails against the prior code and passes with the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard comp/desktop Electron desktop app (apps/desktop/*) P1 High — major feature broken, no workaround sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows: Settings UI overrides config.yaml; agent cannot edit configs programmatically

3 participants