fix(memory/hindsight): make HINDSIGHT_* env vars overlay config.json (#51166) - #51375
fix(memory/hindsight): make HINDSIGHT_* env vars overlay config.json (#51166)#51375mkslzk wants to merge 1 commit into
Conversation
…ousResearch#51166) `_load_config()` in the Hindsight memory plugin short-circuited with `return json.loads(...)` whenever a `~/.hermes/hindsight/config.json` existed, so the HINDSIGHT_BANK_ID (and HINDSIGHT_MODE, HINDSIGHT_API_KEY, HINDSIGHT_BUDGET, ...) env vars in a profile's .env were silently ignored. The user set `HINDSIGHT_BANK_ID=nihai-tcm` but the plugin loaded bank `hermes` (or whatever the JSON had) and routed every auto retain/recall to the wrong bank. After the fix, HINDSIGHT_* env vars are treated as a per-process overlay on top of the loaded config. JSON is still the source of truth when no env var is set; unsetting the env var is the operator-facing way to let the JSON value win. HINDSIGHT_BANK_ID is surfaced at both `config.bank_id` (the read site in HindsightMemoryProvider.__init__ at line 1287) and the legacy `config.banks.hermes.bankId` location so both lookup shapes work regardless of which path seeded the config. Covered by 5 regression tests in `tests/plugins/memory/test_hindsight_load_config.py`: - env var wins when config.json exists (the headline NousResearch#51166 case) - all HINDSIGHT_* overlay env vars (mode, apiKey, budget) work - JSON value still wins when env var is unset - env vars seed the config when no config.json exists - env-var bank_id is reachable through the read site's lookup chain All 128 existing Hindsight tests remain green.
alt-glitch
added
type/bug
Competing fix for #51166 alongside #51173 (@westkite1201). #51173 takes a narrow |
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Important fix for the Hindsight memory plugin: HINDSIGHT_* env vars now overlay matching keys on the loaded config, so a per-profile .env like HINDSIGHT_BANK_ID=nihai-tcm is honored regardless of whether a config.json exists on disk.
Looks Good
- The env-var overlay pattern is clean: load JSON first, then overlay env vars on top
HINDSIGHT_BANK_IDis surfaced at both top-levelbank_idand nestedbanks.hermes.bankIdfor backward compatibility- 153 lines of regression tests cover: env var wins over JSON, JSON wins when no env var, bank-id reaches the read site
- Well-documented with clear comments explaining the #51166 bug and the fix
Reviewed by Hermes Agent
|
When can we go live? |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real current-main configuration bug: _load_config() still returns existing JSON before environment fallback (plugins/memory/hindsight/__init__.py:360-363), and bank resolution defaults to hermes when JSON has no bank ID (plugins/memory/hindsight/__init__.py:1286-1299).
Problems
- The claimed general overlay omits documented
HINDSIGHT_API_URL(plugins/memory/hindsight/README.md:140); persistedapi_urlstill wins atplugins/memory/hindsight/__init__.py:1283. tests/plugins/memory/test_hindsight_load_config.py:72-73is tautological because line 70 already proves the secondorcondition.- The new tests do not initialize
HindsightMemoryProviderthrough the production resolution path atplugins/memory/hindsight/__init__.py:1243-1299.
Suggested changes
- Either narrow the patch to
HINDSIGHT_BANK_ID, or include and test every documented override claimed by the new precedence rule. - Add an initialized-provider regression using an existing profile config that lacks
bank_id, and replace the tautological assertion with direct representation checks.
Automated hermes-sweeper review.
| "HINDSIGHT_RETAIN_SOURCE": "retain_source", | ||
| "HINDSIGHT_RETAIN_USER_PREFIX": "retain_user_prefix", | ||
| "HINDSIGHT_RETAIN_ASSISTANT_PREFIX": "retain_assistant_prefix", | ||
| "HINDSIGHT_BUDGET": "budget", |
There was a problem hiding this comment.
This map claims to overlay matching HINDSIGHT_* values, but it omits documented HINDSIGHT_API_URL (README.md:140). With persisted api_url, current initialization still ignores that override at initialize() line 1283. Please either add and test this mapping or narrow the patch's stated scope.
| # *resolved* bank is the env-var one, not the JSON one. | ||
| assert cfg.get("bank_id") == "nihai-tcm" | ||
| # The JSON-stored bank must NOT silently win. | ||
| assert cfg.get("banks", {}).get("hermes", {}).get("bankId") != "nihai-tcm" or \ |
There was a problem hiding this comment.
This assertion is vacuous: line 70 has already established cfg.get("bank_id") == "nihai-tcm", making the second or branch true regardless of the nested value. Assert the intended nested value directly, or remove this check and test the initialized provider's resolved bank.
teknium1
added
sweeper:risk-compatibility
5 participants
Summary
_load_config()in the Hindsight memory plugin short-circuited withreturn json.loads(...)whenever a~/.hermes/hindsight/config.jsonexisted, so theHINDSIGHT_BANK_ID(andHINDSIGHT_MODE,HINDSIGHT_API_KEY,HINDSIGHT_BUDGET, ...) env vars in a profile's.envwere silently ignored.The user reported setting
HINDSIGHT_BANK_ID=nihai-tcmin the profile.envbut the plugin still loaded bankhermes(the value fromconfig.json) and routed every auto retain/recall to the wrong bank.Fix
HINDSIGHT_* env vars are now treated as a per-process overlay on top of the loaded config. JSON remains the source of truth when no env var is set; unsetting the env var is the operator-facing way to let the JSON value win.
HINDSIGHT_BANK_IDis surfaced at bothconfig.bank_id(the read site inHindsightMemoryProvider.__init__at line 1287) and the legacyconfig.banks.hermes.bankIdlocation so both lookup shapes work regardless of which path seeded the config.Tests
tests/plugins/memory/test_hindsight_load_config.py_load_config()callRelated