Skip to content

fix(hindsight): preserve existing key on blank local_embedded setup - #15309

Closed
poruru-code wants to merge 2 commits into
NousResearch:mainfrom
poruru-code:fix/hindsight-local-embedded-key
Closed

fix(hindsight): preserve existing key on blank local_embedded setup#15309
poruru-code wants to merge 2 commits into
NousResearch:mainfrom
poruru-code:fix/hindsight-local-embedded-key

Conversation

@poruru-code

@poruru-code poruru-code commented Apr 24, 2026

Copy link
Copy Markdown

Summary

This fixes a local_embedded setup bug in the Hindsight provider.

Re-running the setup wizard with a blank LLM API key input was wiping an existing key from the generated embedded profile env. This PR preserves the existing key instead of overwriting it with an empty value.

This is intended as a bugfix, not a new behavior change: the expected preserve-on-blank behavior was already covered by the existing post-setup test contract.

I split this out from the auto-prefetch tag redesign work so the fix can be reviewed independently.

The default preserve-on-blank expectation was already encoded in the existing post-setup test contract. This PR aligns the implementation with that contract, adds coverage for the non-default saved-profile fallback path, and hardens setup against malformed non-dict saved config.

Root Cause

In plugins/memory/hindsight/__init__.py, HindsightMemoryProvider.post_setup() wrote an empty HINDSIGHT_LLM_API_KEY into the setup env writes, then rewrote hermes_home/.env, and only after that materialized the embedded profile env.

That order destroyed the fallback source before the later profile-env materialization step could reuse it.

What Changed

  • Preserve the existing HINDSIGHT_LLM_API_KEY when the local_embedded setup prompt is left blank.
  • Prefer the existing key from hermes_home/.env.
  • If that is absent, fall back to the existing embedded profile env resolved from the saved profile config.
  • Ignore malformed non-dict hindsight/config.json content during setup recovery instead of crashing.
  • Make the local_embedded wizard prompt explicitly say that blank input keeps the existing key.
  • Keep the previous behavior for first-time setup when no key exists anywhere: write an explicit empty value rather than leaving the variable unset.

How To Test

Run:

uv run scripts/run_tests.sh tests/plugins/memory/test_hindsight_provider.py::TestPostSetup::test_local_embedded_setup_preserves_existing_key_when_input_left_blank tests/plugins/memory/test_hindsight_provider.py::TestPostSetup::test_local_embedded_setup_preserves_existing_key_from_nondefault_profile_env_when_input_left_blank tests/plugins/memory/test_hindsight_provider.py::TestPostSetup::test_local_embedded_setup_ignores_nondict_saved_config_when_input_left_blank

uv run scripts/run_tests.sh tests/plugins/memory/test_hindsight_provider.py

For a direct setup-path check, exercise HindsightMemoryProvider.post_setup() in local_embedded mode with a blank key input and an existing coder.env, and verify both the generated profile env and hermes_home/.env still contain the existing key.

Validation

Results:

  • targeted tests: 3 passed
  • focused hindsight provider tests: 77 passed
  • direct setup-path check: preserved existing-key in coder.env on Linux

Platforms Tested

  • Linux

Related

Copilot AI review requested due to automatic review settings April 24, 2026 19:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a local_embedded setup regression in the Hindsight memory provider where leaving the LLM API key prompt blank could overwrite an existing key, by resolving and reusing the prior key from .env or the saved embedded profile env.

Changes:

  • Preserve HINDSIGHT_LLM_API_KEY when local_embedded setup key input is blank by falling back to $HERMES_HOME/.env, then the resolved embedded profile env.
  • Add a regression test covering preserve-on-blank behavior when the existing key is only present in a non-default embedded profile env.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
plugins/memory/hindsight/__init__.py Adds fallback resolution for an existing LLM API key during local_embedded post-setup to avoid wiping keys on blank input.
tests/plugins/memory/test_hindsight_provider.py Adds a test ensuring blank key input preserves a key stored in a non-default embedded profile env.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plugins/memory/hindsight/__init__.py
Comment thread plugins/memory/hindsight/__init__.py Outdated
Comment thread tests/plugins/memory/test_hindsight_provider.py
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers labels Apr 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #15233 — same root cause: Hindsight post_setup() overwrites existing HINDSIGHT_LLM_API_KEY when local_embedded setup prompt is left blank. Both PRs preserve the prior key from env/profile. See also merged #14163 and #14011.

@poruru-code

poruru-code commented Apr 25, 2026

Copy link
Copy Markdown
Author

Closing this as a duplicate of #15233.
The main root cause and fix direction overlap, and I'd rather reduce parallel PRs here.
This branch also included a few extra hardening / test deltas, but those can be ported separately if they still turn out to be useful.

@alt-glitch

Copy link
Copy Markdown
Collaborator

Salvaged into #15503 — your key preservation logic, config hardening, and test cases were cherry-picked onto current main with your authorship preserved. Combined with the masked-key UX from #15233. Thank you @poruru-code!

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

Labels

comp/plugins Plugin system and bundled plugins P2 Medium — degraded but workaround exists tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants