fix(memory): preserve user-configured hindsight embedded keys#71107
Open
xrwang8 wants to merge 1 commit into
Open
fix(memory): preserve user-configured hindsight embedded keys#71107xrwang8 wants to merge 1 commit into
xrwang8 wants to merge 1 commit into
Conversation
…NousResearch#70606) **Problem:** When Hermes rotates the hindsight embedded daemon's profile-scoped .env file (~/.local/state/hindsight/profiles/*/env) on every startup or config change, any user-configured Hindsight settings — embedding provider, ONNX parameters, reranker, port, etc. — are silently clobbered. Persisting changes across daemon restarts requires tedious manual re-patching after every `hermes update` (NousResearch#70606 RC1). Additionally, the config-change detector `_start_daemon` uses a naive dict equality check that treats *all* keys in the env file as Hermes-managed, so user-added keys trigger spurious daemon restarts even when Hermes-managed keys haven't changed (NousResearch#70606 RC3). **Solution:** RC1: Replace the unconditional write with a read-merge-write pattern: 1. Parse the existing env file via _load_simple_env. 2. Overlay only the keys _build_embedded_profile_env emits (the Hermes-managed subset). 3. Write back the merged dict. Existing keys maintain their position; new Hermes keys append at the end. Comments and blank lines are dropped per _load_simple_env semantics (acceptable tradeoff). RC3: Extract a new _embedded_config_changed helper that compares only the keys _build_embedded_profile_env emits, ignoring user-added keys. Wire it into the _start_daemon closure to replace the naive dict equality. Both functions derive the canonical key set from _build_embedded_profile_env output, avoiding duplication and keeping RC1 (merge) and RC3 (comparison) synchronized. **Caveats:** - If the user manually edits a Hermes-managed key to a different value, that value will be overwritten on next startup, and _embedded_config_changed will report a change, triggering a restart. - If a Hermes key is removed from the config (e.g., llm_base_url removed), the old key lingers in the env file but is excluded from the config-changed check. The daemon won't see it, but it won't be cleaned up either. This is an edge case with minimal impact (only two optional keys: base_url and idle_timeout). **Testing:** - tests/plugins/memory/test_hindsight_provider_env_merge.py: 7 new tests covering fresh install, user key preservation, Hermes key rotation, and RC3 change detection. - All 116 existing hindsight tests pass. Signed-off-by: xrwang8 <xrwang8@gmail.com>
alt-glitch
added
type/bug
|
One transition still leaves Hermes-managed state stale. Suggested next action: define the full Hermes-owned key set, remove owned keys that are absent from the newly built values before merging, and add a present→absent regression test (while continuing to preserve non-Hermes/user keys). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #70606 - Hindsight local_embedded overwrites user env config on every daemon start
Changes
Testing
Related
Fixes #70606