feat: Add MemoryStore protocol for cross-session agent memory#231
feat: Add MemoryStore protocol for cross-session agent memory#231rdwj wants to merge 16 commits intokagenti:mainfrom
Conversation
Introduce a MemoryStore abstraction that complements ContextStore. ContextStore handles per-conversation message replay; MemoryStore handles durable, governed knowledge that persists across sessions. The protocol consists of: - MemoryStore (ABC): factory that creates per-context instances - MemoryStoreInstance (Protocol): search, write, read, update, delete - MemoryResult (Pydantic model): returned from search and read The interface is backend-agnostic — implementations can target any persistent memory service. Assisted-By: Claude Code (Opus 4.6) Signed-off-by: rdwj <wjackson@redhat.com>
…ation 40 tests covering: - MemoryResult model (field defaults, explicit overrides) - MemoryHubMemoryStore (construction, from_env, client caching) - MemoryHubMemoryStoreInstance (search, write, read, update, delete) - _MemoryProxy (lazy init, method delegation, curation gating) - create_memory_dependency (sync provider, proxy resolution) The memoryhub SDK is mocked at the module level to avoid requiring the optional dependency in the test environment. Assisted-By: Claude Code (Opus 4.6) Signed-off-by: rdwj <wjackson@redhat.com>
Brief doc covering the MemoryStore protocol, MemoryHub implementation, DI integration via create_memory_dependency(), and a minimal agent example. Assisted-By: Claude Code (Opus 4.6) Signed-off-by: rdwj <wjackson@redhat.com>
- MemoryHubMemoryStoreInstance now subclasses MemoryStoreInstance, matching the ADK convention (MemoryContextStoreInstance, etc.) - Add close() to MemoryHubMemoryStore for client session cleanup - Add full type annotations to _MemoryProxy methods and DI provider - Fix ruff lint errors: unused variables (F841), import sort (I001) - Add tests for close() (cleanup and no-op paths) - Move doc to docs/development/sdk/memory.mdx and register in docs.json Assisted-by: Claude Code (Opus 4.6) Signed-off-by: rdwj <wjackson@redhat.com>
fc092c8 to
766aa67
Compare
JanPokorny
left a comment
There was a problem hiding this comment.
Hi, thank you for the PR. I added some feedback to help align with the current ADK patterns. Ping back if something does not make sense to you.
The Depends machinery resolved the dependency callable synchronously during __call__, which forced async dependency providers to bridge via proxy objects. Move resolution into the lifespan body and await any resulting awaitable so async providers are first-class. Refs kagenti#229. Assisted-by: Claude Code (Opus 4.7)
`create` more clearly distinguishes "make a new memory" from `update`, matching the read/create/update/delete vocabulary the rest of the protocol uses. The MemoryHub backend still calls the underlying SDK's `client.write()`; only the protocol surface changes. Assisted-by: Claude Code (Opus 4.7)
…mantics The protocol intentionally leaves scope/weight/tags/project_id with backend-defined meaning. Spell that out in the docstrings so reviewers don't read MemoryHub's vocabulary as the protocol contract. Each entry includes the MemoryHub mapping as a worked example. Assisted-by: Claude Code (Opus 4.7)
Mirrors services/llm.py: Demand/Fulfillment/Params/Metadata/Spec/ Server/Client classes plus an env-var fallback used as the server-side default. Lets clients hand the agent its MemoryHub connection via A2A metadata instead of forcing the agent to read process env directly. No store wiring yet — that lands in the next commit. Assisted-by: Claude Code (Opus 4.7)
…uto-loader Drops MemoryHubMemoryStore, _MemoryProxy and create_memory_dependency. MemoryHubExtensionServer now owns the MemoryHubClient lifecycle via its A2A lifespan() and exposes per-context MemoryHubMemoryStoreInstance via .store(context_id). Async resolution is now handled by the awaitable Depends fix in commit 1, so the proxy workaround is no longer needed. Closes the dependency on _MemoryProxy that kagenti#229 introduced. Assisted-by: Claude Code (Opus 4.7)
Every consumer in the repo already imports from the concrete module (context_store, memory_store, platform_context_store, etc.). The package-level re-exports added unhelpful indirection and a second place to maintain the symbol list. Assisted-by: Claude Code (Opus 4.7)
Minimal agent that searches MemoryHub for the user's query and stores one fact from the input. Mirrors llm-proxy-service/llm-access in structure. The matching E2E test fulfills against a real cluster using repo secrets MEMORYHUB_E2E_URL plus either MEMORYHUB_E2E_API_KEY or the OAuth trio (AUTH_URL/CLIENT_ID/CLIENT_SECRET); skips cleanly when secrets aren't set so contributor forks aren't broken. Maintainers: please add the secrets above to enable the test in CI. Assisted-by: Claude Code (Opus 4.7)
Drops the from_env / Depends / proxy story entirely. The new flow shows the protocol, the server-side MemoryHubExtensionServer.store() accessor, and the client-side MemoryHubExtensionClient fulfillment pattern. Switches install instructions to ``uv add`` and embeds the new memoryhub-recall example via embedme. Closes the remaining doc-review comments on PR kagenti#231. Assisted-by: Claude Code (Opus 4.7)
- Replace en-dash with hyphen in MemoryStore docstrings (RUF001/RUF002). - Drop unused MagicMock import (F401). - Rename test variable ClientCls -> client_cls (N806). Caught running ``uv run ruff check`` locally before pushing the rework. Assisted-by: Claude Code (Opus 4.7)
|
Thanks for the careful review. I've pushed a rework on
A trailing |
| if result.memory is None: | ||
| # Curation gated the create — return empty string to signal no-op | ||
| logger.warning("MemoryHub curation gated create: %s", result.curation.reason) | ||
| return "" |
There was a problem hiding this comment.
I don't like using empty string as an error state, too easy to forget to handle it. Throw an exception or return None. Also, what does "curation gated the create" mean exactly?
There was a problem hiding this comment.
Thanks — agreed, returning an empty string was too easy to miss. Fixed in 481a68f with a follow-up clarifying comment in a853d7a.
The implementation now raises MemoryRejectionError (in kagenti_adk.server.store.exceptions), which subclasses RuntimeError and carries the backend's reason on .reason. That matches the existing ToolCallRejectionError / ApprovalRejectionError pattern, and the exception's own docstring spells out what the various rejection sources are so callers don't have to read MemoryHub's internals.
To answer "what does curation gated the create mean exactly": MemoryHub runs every write through a pre-storage pipeline that can refuse to record the memory — typically because it's a duplicate of an existing memory, contradicts an existing memory, or trips a policy/curator rule, such as PII or secrets filtering. The reason string on the exception is the backend's explanation. The old logger.warning is gone; the exception itself is the signal, matching how the other *RejectionErrors in the SDK are raised without a sibling log line.
I also added a short inline comment at the SDK boundary (a853d7a) noting that memoryhub.WriteResult.memory is None is the rejection signal — the contract is invisible at the call site otherwise.
Docs and the rejection test were updated accordingly. Unit suite stays at 123 passing, ruff clean.
Replace the empty-string sentinel return on rejected writes with a typed MemoryRejectionError. Empty-string-as-error was easy for callers to miss; the exception carries the backend's reason and matches the existing ToolCallRejectionError / ApprovalRejectionError pattern in the SDK. Addresses review feedback on PR kagenti#231. Assisted-by: Claude Code (Opus 4.7)
Document that memoryhub.WriteResult.memory is None when the SDK's curation pipeline rejects a write. The contract is invisible at this call site without the comment, and the boundary between two systems warrants a pointer. Assisted-by: Claude Code (Opus 4.7)
|
Pausing this PR — found that the upstream |
…208) * planning: Add design for SDK rework against compacted tool surface The memoryhub SDK still calls per-action MCP tools but the deployed primary server only exposes register_session + memory after #198/#202. Issue #202 called for keeping per-action tools as deprecation aliases; that step was skipped. The kagenti-adk integration in kagenti/adk#231 hit this when run against the live server. Decision recorded: take the SDK forward to the new surface rather than back-porting server-side aliases. Public method signatures stay stable; only the wire format changes. Bump SDK 0.6.0 → 0.7.0. Tracks #210. Assisted-by: Claude Code (Opus 4.7) Signed-off-by: rdwj <wjackson@redhat.com> * sdk: Rework client against compacted memory(action=...) tool surface Every public MemoryHubClient method now dispatches through the unified `memory` tool (#198/#202) instead of the legacy per-action tool names. Public Python API is unchanged — only the wire format changes. This restores end-to-end behavior against the primary memory-hub-mcp deployment, which exposes only register_session + memory after the consolidation. The cutover replaces the deprecation-alias path that was scoped in #202 but never shipped. Signature stability is the load-bearing property here: the kagenti-adk MemoryStore wrapper (kagenti/adk#231) calls search, write, read, update, delete, etc. directly — those signatures are preserved, so consumers only need to bump the dependency pin. Smoke-tested against the live primary server: search, get_session, and list_projects all succeed. Note: server-side max_results behavior on search appears to over-return appendix entries; that's a separate server bug, not SDK. Tracks #210. Assisted-by: Claude Code (Opus 4.7) Signed-off-by: rdwj <wjackson@redhat.com> * sdk: Bump to 0.7.0 with BREAKING wire-format note The 0.6.0 → 0.7.0 cutover swaps every MCP tool call from the legacy per-action names to memory(action=..., options={...}). Public Python API is unchanged but the wire format is incompatible with servers that only expose the per-action surface, and 0.6.0 is incompatible with the primary memory-hub-mcp deployment. Tracks #210. Assisted-by: Claude Code (Opus 4.7) Signed-off-by: rdwj <wjackson@redhat.com> * sdk: Add kagenti-adk contract test (#208) Pins the SDK surface that kagenti-adk's MemoryHubMemoryStoreInstance depends on: constructor (api_key + OAuth modes), search/write/read/ update/delete signatures, the WriteResult.curation.reason path that the wrapper raises MemoryRejectionError on, and NotFoundError on missing-id reads/deletes. Uses the SDK's existing mocked transport — no live server needed. A failure here is the cue to coordinate with kagenti-adk maintainers before shipping the SDK release. Closes #208. Assisted-by: Claude Code (Opus 4.7) Signed-off-by: rdwj <wjackson@redhat.com> --------- Signed-off-by: rdwj <wjackson@redhat.com>
Required to talk to the deployed primary memory-hub-mcp server, which exposes only the unified memory(action=...) MCP tool after the upstream consolidation in redhat-ai-americas/memory-hub#198, kagenti#202. SDK 0.6.x calls the legacy per-action tool names and fails end-to-end against that deployment. Public Python API of memoryhub is unchanged — the existing MemoryHubMemoryStoreInstance wrapper still compiles and all 24 adk-py memory store unit tests pass against 0.7.0 with no source changes required. Tracks redhat-ai-americas/memory-hub#210. Signed-off-by: rdwj <wjackson@redhat.com>
|
Unpausing — fixed the underlying issue and pushed Background. The Upstream fix. redhat-ai-americas/memory-hub#211 — reworks New SDK contract test. redhat-ai-americas/memory-hub#208 / Verification done locally. All 24 One snag for your CI / re-test. This repo's
I left the lock alone so you can choose. Happy to push the lock update as a follow-up commit if you'd prefer option 2. |
Summary
Adds a
MemoryStoreprotocol and a MemoryHub-backed implementation for governed, cross-session agent memory. This is the ADK component of the integration proposal in kagenti/kagenti#1334.MemoryStorecomplementsContextStore: whereContextStorereplays the current conversation,MemoryStoreprovides durable knowledge that persists across sessions — preferences, decisions, and project context that agents should remember regardless of which conversation is active.What's included:
MemoryStoreprotocol andMemoryStoreInstanceProtocol inkagenti_adk.server.store— follows the same Protocol + ABC +create(context_id)factory pattern asContextStoreMemoryHubMemoryStoreimplementation wrapping thememoryhubPython SDK (optional dependency viapip install kagenti-adk[memoryhub])create_memory_dependency()helper for the ADKDependsDI pattern — includes a lazy proxy workaround for Depends does not await async dependency callables #229docs/development/sdk/memory.mdxValidated in PoC (2026-04-23): Memory write, semantic search recall, and pod restart survival confirmed end-to-end on a fresh RHPDS cluster with Kagenti + MemoryHub + test agent. Details in kagenti/kagenti#1334.
Linked Issues
Relates to kagenti/kagenti#1334 (integration proposal)
Workaround for #229 (
Dependsdoesn't await async callables)Documentation
Documentation is included:
docs/development/sdk/memory.mdx, registered indocs.jsonunder the "Using the SDK" group (development version)./cc @Ladas