Skip to content

Memory: make memory_update work without an embedding provider and keep category links - #485

Open
niklassemmler wants to merge 1 commit into
ClickHouse:mainfrom
niklassemmler:niklassemmler/memory-update-without-embeddings
Open

niklassemmler wants to merge 1 commit into
ClickHouse:mainfrom
niklassemmler:niklassemmler/memory-update-without-embeddings

Conversation

@niklassemmler

Copy link
Copy Markdown

Problem to be solved:

memory_update fails on every content update when no OpenAI key is configured. The tool only reports Failed to update memory <id>. The log shows the cause:

update_item failed for <id>: Error code: 404 - {'type': 'error', 'error': {'type': 'not_found_error', 'message': 'Not found'}, ...}

memU's update_memory_item step embeds new content unconditionally. Without an OpenAI key, Nerve configures no embedding profile, and memU's settings alias it to default (memu/app/settings.py). The OpenAI-SDK client then POSTs /embeddings to the Anthropic API, which answers 404. On Bedrock the call goes to the placeholder base URL instead. Nerve already works around this for memorize by replacing categorize_items, but it doesn't for updates. Deletes don't embed, which is why they kept working. Edits from the web UI (PATCH /api/memory/memu/items/{id}) go through the same path.

A second bug sits behind the first. The same step reads categories=None as "no categories" and unlinks the item from every category it had. In _patch_update_memory_item, cats_to_remove is the old set minus map(None) == []. The agent tool passes None whenever no categories are given. So once the 404 is gone, every content-only update would silently drop the item's category links and tell each category summary that the content was discarded. This also hits instances that do have an OpenAI key today.

Changes:

  • MemUBridge._update_item_step replaces memU's update_memory_item step in the patch_update pipeline. It keeps the same step contract (requires, produces, capabilities, config), so memU's persist and response steps run unchanged. The new step:
    • embeds only when an embedding provider is configured, and still resolves the client through the step's embedding profile;
    • keeps the existing links when categories is None, while an explicit list (including []) relinks exactly as before;
    • skips the re-embed and the category-summary rewrite when the summary is unchanged. The web UI resends the full text on every save, so toggling one category used to cost one LLM call per linked category.
  • The step is installed right after MemoryService is constructed, before the bridge marks memU as available. If installing fails, memU is reported as unavailable instead of half-initialized.
  • Corrected the comment on the existing categorize_items swap. A missing embedding profile doesn't raise KeyError; it falls back to the chat profile.

Testing:

  • The new TestUpdateItemStep runs memU's real patch_update pipeline against a real SQLite store: its step validation, LLM-client resolution, persist step and response step. Only the per-profile base clients are fakes. It covers:
    • a content update without a provider;
    • a type-only update;
    • an explicit relink;
    • embedding through the embedding profile when a provider is configured;
    • an unchanged summary;
    • a missing item;
    • a canary asserting that memU's own step still embeds unconditionally.
  • I broke the code on purpose five ways, and each was caught by at least one test: removing the install call, pointing the step at the default profile, always embedding, dropping the None handling, and dropping the unchanged-content check.
  • Full suite: 3937 passed.
  • _memu_repo() now delegates to a new _memu_db() helper that returns the whole shared database, because the new tests also need the category repos.

Known gaps, left for follow-ups:

  • _map_category_names_to_ids silently drops names it doesn't know, so memory_update(categories="typo") still unlinks everything and reports success. Rejecting unmapped names seems right, but it changes behavior, so it belongs in a separate PR.
  • Without a provider, editing an item that still has a vector from an earlier keyed setup leaves the old vector in place, because embedding=None means "unchanged" in memU's repo. This is harmless until a key is added back.
  • The tool's error message doesn't include the cause. memorize already surfaces last_error, and memory_update could do the same.

🤖 Generated with Claude Code

…p category links

memU's update_memory_item step embeds new content unconditionally. With no
OpenAI key, Nerve configures no "embedding" profile, memU aliases it to the
"default" chat profile, and the OpenAI-SDK client then POSTs /embeddings to
the Anthropic API, which answers 404. Every content update through the
memory_update tool and the web UI failed with "Failed to update memory".

The same step reads categories=None as "no categories" and unlinks the item
from all of them. With the 404 gone, a content-only update would therefore
silently drop every category link the item has.

Nerve now replaces that step in memU's patch_update pipeline with its own.
The new step embeds only when an embedding provider is configured, keeps the
links when no categories are passed, and skips the re-embed and the
category-summary rewrite when the content is unchanged. The last part matters
because the web UI resends the full text on every save.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment on lines +3282 to +3286
if content == old_content:
# The web UI resends the full text with every edit, so an
# unchanged summary must not trigger a re-embed or a category
# summary rewrite.
content = None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[P2] Preserve unfinished category-summary work on retry

The item repository commits the new text before persist_index calls the LLM to patch category summaries. If that call times out, update_item() reports failure even though the item text is already saved. On an identical retry, this equality check sets content = None, so the later if content: block produces no category updates and the operation reports success with the old category summary still persisted.

Reproduced through full bridge initialization and the public update_item() method with a real SQLite store and one injected category-LLM timeout: the first call returns False with new item text / old category summary; the retry returns True, makes zero summary calls, and leaves that mismatch intact. This occurs with and without embeddings. The original memU step makes another summary call on retry. The partial commit predates this PR; suppressing recovery on retry is introduced here.

Please keep category-summary recovery independent of the embedding no-op optimization: only skip summary work when its completion is known, or defer that optimization. A failure-then-identical-retry regression test would cover this.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants