Skip to content

fix(desktop): show Hindsight local_embedded mode + LLM config in settings - #58924

Open
yingliang-zhang wants to merge 2 commits into
NousResearch:mainfrom
yingliang-zhang:fix/hindsight-local-embedded-desktop-config
Open

fix(desktop): show Hindsight local_embedded mode + LLM config in settings#58924
yingliang-zhang wants to merge 2 commits into
NousResearch:mainfrom
yingliang-zhang:fix/hindsight-local-embedded-desktop-config

Conversation

@yingliang-zhang

Copy link
Copy Markdown
Contributor

Summary

  • Add local_embedded to the Hindsight mode select options in the desktop memory-provider schema.
  • Add llm_api_key (secret, HINDSIGHT_LLM_API_KEY), llm_provider, llm_base_url, and llm_model fields so local_embedded configs are fully visible and editable from the desktop settings panel.
  • Rename api_key label to "Cloud API key" for disambiguation.
  • Update test_memory_providers.py and test_web_server.py to match.

Why

A valid Hindsight config can use mode: local_embedded, but the desktop config surface only allowed cloud and local_external. The web endpoint therefore treated local_embedded as an unsupported select value and fell back to the default cloud, making the settings panel misleading and risking an accidental save back to cloud mode.

Additionally, the schema only declared HINDSIGHT_API_KEY (cloud) but not HINDSIGHT_LLM_API_KEY (local embedded), so the LLM API key status was invisible. The llm_provider, llm_base_url, and llm_model fields were also missing, making local_embedded configs unviewable and uneditable from the desktop settings panel.

Relationship to other PRs

Supersedes #57126 (which adds local_embedded mode only). This PR also adds the LLM config fields needed for local_embedded to be fully configurable from the desktop UI.

Related: #7718 (hindsight-all dependency gap for local_embedded), #48675 (broader schema-driven memory-provider refactor).

Test Plan

  • python -m pytest tests/hermes_cli/test_memory_providers.py tests/hermes_cli/test_web_server.py -v — 346 passed, 0 failed
  • Verified that GET /api/memory/providers/hindsight/config correctly returns mode=local_embedded, llm_api_key.is_set=true, and all LLM field values for a real local_embedded config

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have labels Jul 5, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #57126 (adds local_embedded mode only) — this PR is the superset (also adds llm_api_key/llm_provider/llm_base_url/llm_model so local_embedded is fully configurable from the desktop UI), and #48675 (broader schema-driven memory-provider refactor). Not a duplicate — competing subset/superset PRs for the same surface; a maintainer picks.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Good bug fix for Hindsight local_embedded mode display. Well-scoped change to show the correct mode indicator and LLM config in settings. Single concern.


Reviewed by Hermes Agent

@yingliang-zhang

Copy link
Copy Markdown
Contributor Author

Thanks for the review @tonydwb. Rebased onto latest main and resolved test conflicts in test_web_server.py:

  1. The exact-dict-equality assertion on config.json was replaced with individual key checks — upstream now writes 22+ default fields (auto_recall, bank_id_template, etc.) so the snapshot assertion was a change-detector that would break on any new field.

  2. The test_put_memory_provider_config_accepts_local_embedded_mode test (which sent mode: "spaceship" expecting 200 + coercion to local_embedded) was reverted to the upstream test_put_memory_provider_config_rejects_unsupported_select_value (expects 400) — upstream now validates and rejects unknown select values.

  3. The schema-level changes (local_embedded in mode options, llm_api_key/llm_provider/llm_model fields in memory_providers.py) are preserved as-is.

All 357 test_web_server.py tests pass.

@yingliang-zhang
yingliang-zhang force-pushed the fix/hindsight-local-embedded-desktop-config branch 2 times, most recently from 862f26e to 06034a4 Compare July 11, 2026 05:21

@teknium1 teknium1 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.

Thanks for addressing the Desktop declared-schema gap; the premise is confirmed on current main.

Problems

  • hermes_cli/memory_providers.py:124 makes llm_provider free text. The declared write path only validates selects (hermes_cli/web_server.py:5257-5265), but Hindsight defines a finite provider list (plugins/memory/hindsight/__init__.py:980) and forwards the stored value into HindsightEmbedded (plugins/memory/hindsight/__init__.py:1031-1040). Please expose this as a select using that supported list.
  • The changed test at tests/hermes_cli/test_web_server.py:475 covers the raw plugin endpoint. Desktop uses ?surface=declared (apps/desktop/src/hermes.ts:522,528). Current main's declared-surface test at tests/hermes_cli/test_web_server.py:494-502 still asserts the old five-field schema, so it must be updated along with a declared-surface save/secret-status round trip.

Automated hermes-sweeper review.

Comment thread hermes_cli/memory_providers.py Outdated
Comment thread tests/hermes_cli/test_web_server.py Outdated
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@yingliang-zhang
yingliang-zhang force-pushed the fix/hindsight-local-embedded-desktop-config branch from 06034a4 to 27211a8 Compare July 16, 2026 05:39
@teknium1 teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 2026
@yingliang-zhang
yingliang-zhang force-pushed the fix/hindsight-local-embedded-desktop-config branch from 27211a8 to 98de302 Compare July 20, 2026 18:01
…ings

The desktop memory-provider schema excluded local_embedded from the mode
select, causing the UI to coerce it to cloud (default) and display
"API unset" for users with a valid local_embedded config. Saving from
the UI would silently overwrite the mode to cloud, breaking the setup.

Additionally, the schema only declared HINDSIGHT_API_KEY (cloud) but not
HINDSIGHT_LLM_API_KEY (local embedded), so the LLM API key status was
invisible. The llm_provider, llm_base_url, and llm_model fields were
also missing, making local_embedded configs unviewable and uneditable
from the desktop settings panel.

Changes:
- Add local_embedded to Hindsight mode select options
- Add llm_api_key secret field (env_key=HINDSIGHT_LLM_API_KEY)
- Add llm_provider, llm_base_url, llm_model text fields
- Rename api_key label to "Cloud API key" for disambiguation
- Update test_memory_providers.py: expand field set, add llm_api_key test
- Update test_web_server.py: accept local_embedded mode, assert new fields

Supersedes NousResearch#57126 (which adds local_embedded mode only). This PR also
adds the LLM config fields needed for local_embedded to be fully
configurable from the desktop UI.

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

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

4 participants