fix(desktop): config autosave sends only session-edited keys — stale drafts stop reverting newer changes - #90537
fix(desktop): config autosave sends only session-edited keys — stale drafts stop reverting newer changes#90537teknium1 wants to merge 1 commit into
Conversation
…drafts stop reverting newer changes (#89184 residual) The config page seeds its draft once and autosaved the WHOLE record; the backend deep-merge only protects ABSENT keys, and the flattened model string was always present — so a model switched elsewhere while the page sat open got reverted by the next unrelated toggle (backend re-detection included). Autosave now PUTs a sparse record of the exact keys edited this session (model_context_length rides with model); JSON import keeps whole-record semantics.
૮ >ﻌ< ა ci reviewran on 2c76554 — fix(desktop): config autosave sends only session-edited keys
|
alt-glitch
added
type/bug
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 2c765549825961bfce0c1bbdbf703c2688a047da (base snapshot 9c12d2d6c33176ecc8392d83f07b9e0beb2638d2; current main is bbb7b607b4c125a5f0cedf1ce29adc6ae4a59b40). CI, Docker, and Nix are green on this head; the OSV bot warning is repository dependency state, not caused by this 3-file diff.
The core direction is right — sparse writes are the correct boundary for #89184/#89597 — but I see two correctness blockers on the current implementation.
1. The dirty-key authority is session-long, so the original stale-clobber bug comes back after the first successful save
editedKeysRef is monotonic for the lifetime of ConfigSettingsInner: keys are added in applyConfig, but never retired after a successful autosave. fullSaveRef is likewise one-way: once a JSON import sets it, every later autosave is a whole-record PUT forever.
That means the payload is not actually “what this save owns”; it is “everything this page has ever owned.” A concrete same-profile witness:
- Open Settings; edit
model; autosave succeeds. (editedKeysRef = {model}.) - Change the model out-of-band through composer
/model/ Model page / CLI. - Toggle an unrelated field such as
display.show_reasoningin the still-open Config page. - The second autosave sends both
display.show_reasoningand the stalemodel(plus its context length), becausemodelis still in the Set. The newer model is reasserted away — the exact class this PR is meant to close.
There is a cross-profile version too. Current ConfigSettings remounts on explicit Settings-scope changes, but an app-wide active-profile switch with scopeProfile === null uses useOnProfileSwitch() inside the same component instance. That callback resets configSeeded, draft, and saveVersion; this PR does not reset editedKeysRef or fullSaveRef. So keys/whole-record intent from profile A can survive into profile B and be included in B’s next save. That violates the profile-boundary work already established by #88755 / #74829.
Required shape: dirty ownership must be per pending save epoch, not per page session. Snapshot the keys (or diff) for a given save; after that exact save succeeds, retire only the ownership it consumed without dropping edits that arrived while it was in flight. Reset all autosave ownership state on app-wide profile switch. A whole-record import should be a one-save intent, not a permanent mode. Please add component-level witnesses for same-key external mutation after a prior local save, app-wide profile switch, and import-then-later-unrelated-edit.
2. model_context_length is still not a legal independent sparse update on current main
buildSparseRecord() expands model -> model_context_length, but not the reverse. A user editing only the Context Window field therefore sends {model_context_length: ...}.
Current hermes_cli/web_server.py::_denormalize_config_from_web() immediately pops model_context_length, then only reconstructs/writes it inside the if isinstance(model_val, str) and model_val: branch. With no model in the sparse payload, the context-length edit is silently discarded.
This is not theoretical: #89645 found and regression-tested this exact partial-update coupling. Its current backend patch distinguishes “context key absent” from “explicit 0” (ctx_sent) and can apply a context-only update against the on-disk model. That is the safer fix than merely bundling model in both directions, because forcing a stale flattened model to accompany every context edit would recreate the cross-surface stale-model overwrite this PR is trying to eliminate.
Please preserve that backend partial-update contract (or an equivalent one) and add an end-to-end/context-only regression, not just a helper test.
Topology / interlocks
- #89645 is a competing implementation of the same #89597 root cause, not unrelated work. Do not mark it superseded until this branch preserves the pieces it already discovered: advancing save state after a successful write, serialization/version safety for overlapping autosaves, and the backend
model_context_lengthpartial-update semantics/tests. Contributor credit there should survive whichever implementation lands. - #90512 is complementary, not duplicate. This PR controls payload ownership (don’t send stale fields). #90512 controls backend RMW serialization (don’t lose concurrent writes even when two valid sparse writers race). Sparse autosave alone does not close the concurrent lost-update half of the settings-pipeline class.
- #88755 / #74829 are boundary invariants this change must compose with: explicit scope changes remount the inner settings page, while app-wide profile changes are handled by
useOnProfileSwitch; new autosave refs must obey both.
The “other side of the shape” here is that sparse is not sufficient unless ownership is also time-bounded. A stale full snapshot is dangerous, but so is a stale sparse key set. The durable invariant should be: each config write may mutate only the fields intentionally changed since the last acknowledged write for that exact profile, and an acknowledged write retires that authority.
Once the two blockers above are fixed and the new component-level witnesses are green, this should be re-reviewed on the new exact head.
3 participants
Summary
The Settings config page's autosave now PUTs only the keys the user edited this session — a model (or any setting) changed elsewhere while the page sits open is no longer reverted by the page's stale seeded draft. This was the remaining live half of the #89184/#89597 class (settings-pipeline audit Bug 2, HIGH).
Mechanism: the page seeds its local draft ONCE (deliberately — background refetches must not clobber in-progress edits) and autosaved the ENTIRE record on any field change. The backend deep-merge only protects keys ABSENT from the payload, and the flattened
modelstring is always present — so: open Settings → switch model via composer/modelor Model page → toggle any unrelated checkbox → the stale draft'smodelre-asserts, and_denormalize_config_from_webdutifully re-detects the provider and reverts the user's newer choice.Changes
apps/desktop/src/app/settings/helpers.ts:buildSparseRecord(config, keys)— copies only the named schema paths at their nested positions (deep-cloned); bundlesmodel_context_lengthwithmodel(the backend denormalizer treats them as one field).apps/desktop/src/app/settings/config-settings.tsx:updateConfig/applyConfigtrack the edited schema key per change (editedKeysRef); the autosave payload is the sparse record. A JSON import keeps whole-record semantics (fullSaveRef) — sending everything is the point there.helpers.test.ts: 4 new cases (only-edited-paths, model+context bundling, deep-copy isolation, empty set).Validation
/api/model/set→ sonnet-5 → toggle unrelated switch on the seeded page → autosave fires → model stays sonnet-5 (previously reverted); the toggled key (display.show_reasoning) persistedAlso hardens the same class for the future: any key edited on ANOTHER surface (aux models, MoA, approvals) can no longer be clobbered by this page's autosave, because it's simply never sent.
Infographic