Skip to content

Fix desktop memory provider built-in option - #49514

Closed
szafranski wants to merge 1 commit into
NousResearch:mainfrom
szafranski:fix/desktop-memory-provider-built-in
Closed

Fix desktop memory provider built-in option#49514
szafranski wants to merge 1 commit into
NousResearch:mainfrom
szafranski:fix/desktop-memory-provider-built-in

Conversation

@szafranski

@szafranski szafranski commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Treat blank memory.provider as the built-in memory option in Desktop Settings.
  • Remove the misleading builtin plugin option from the provider dropdown.
  • Normalize legacy built-in aliases before rendering provider setup controls.
  • Add regression coverage for the dropdown and legacy aliases.

Why

Built-in persistent memory is the default MEMORY.md / USER.md layer. The memory.provider setting selects an external provider plugin that runs alongside it.

Desktop Settings exposed builtin as if it were a provider plugin. Selecting it could also render plugin setup controls for a provider that does not exist. The backend already normalizes built-in, builtin, and none to the blank built-in value; this change applies the same contract in the desktop UI.

Closes #49513

Tests

  • npm --prefix apps/desktop run test:ui -- src/app/settings/helpers.test.ts22 passed
  • npm --prefix apps/desktop run typecheck → passed
  • ESLint on all changed desktop files with --quiet → passed
  • git diff --check upstream/main...HEAD → passed

@alt-glitch alt-glitch added type/bug Something isn't working comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/cli CLI entry point, hermes_cli/, setup wizard tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have labels Jun 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related: #49513 (the issue this PR Closes), #12990 (adjacent memory-provider UX: CLI hermes memory setup picker defaulting wrong — different code path).

Note: the genuine scope of this PR is the desktop Memory Provider dropdown fix (apps/desktop/src/app/settings/*, i18n strings) plus the backend memory.provider config schema (hermes_cli/web_server.py). The diff also carries unrelated files from an unclean base — gateway/platforms/telegram.py + tests/gateway/test_telegram_voice_duration.py (a Telegram voice-duration change) and a scripts/release.py salvage-credit entry. Those appear to be sibling-branch contamination (PR is currently CONFLICTING); rebasing onto a clean main should drop them.

@szafranski
szafranski force-pushed the fix/desktop-memory-provider-built-in branch from d847a6e to 2ff69df Compare June 20, 2026 08:11
@szafranski

Copy link
Copy Markdown
Contributor Author

Thanks, you were right. I rebased the branch onto a clean upstream/main and force-pushed with only the desktop memory-provider/settings schema fix.

The PR now contains one commit and no longer includes the Telegram voice-duration or release author-map files. While resolving the rebase I kept the newer external hindsight provider option from main and only removed the misleading builtin option.

Re-ran the focused checks:

  • npm --prefix apps/desktop run test:ui -- src/app/settings/helpers.test.ts
  • npm --prefix apps/desktop run typecheck
  • .venv/bin/python -m pytest tests/hermes_cli/test_web_server_config_schema.py tests/hermes_cli/test_doctor.py::TestDoctorMemoryProviderSection -q
  • git diff --check upstream/main..HEAD

@alt-glitch alt-glitch added comp/desktop Electron desktop app (apps/desktop/*) and removed comp/tui Terminal UI (ui-tui/ + tui_gateway/) labels Jun 26, 2026
@szafranski
szafranski force-pushed the fix/desktop-memory-provider-built-in branch from 2ff69df to 4850715 Compare July 1, 2026 08:52
@szafranski
szafranski force-pushed the fix/desktop-memory-provider-built-in branch from 4850715 to 5a86f5e Compare July 11, 2026 09:46
@szafranski
szafranski force-pushed the fix/desktop-memory-provider-built-in branch from 5a86f5e to de5d648 Compare July 13, 2026 11:28
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused desktop fix. The premise is verified on current main: apps/desktop/src/app/settings/constants.ts:241 still exposes builtin, while apps/desktop/src/app/settings/config-settings.tsx:462-479 treats every non-empty value as a plugin provider and renders plugin-only controls. Current backend normalization already maps built-in, builtin, and none to the empty external-provider value (hermes_cli/web_server.py:4417-4421).

The PR's desktop-side normalization, blank-value label, removal of the misleading option, and alias regression coverage align with that existing backend contract. No additional correctness or design-fit issue was found in the nine-file diff.

Automated hermes-sweeper review.

@alt-glitch alt-glitch removed the comp/cli CLI entry point, hermes_cli/, setup wizard label Jul 14, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 14, 2026
@szafranski

Copy link
Copy Markdown
Contributor Author

Superseded by merged PR #67206, which includes the built-in memory provider fix and closes #49513. Thanks.

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/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

Desktop Memory Provider dropdown treats built-in memory like a provider plugin

3 participants