feat(py/strands): add Neo4jMemoryStore for Strands' MemoryStore protocol - #187
Merged
Conversation
Neo4jMemoryStore implementing Strands' long-term-memory MemoryStore protocol in both SDKs. Both Strands reviewers asked for it unprompted in harness-sdk#3871, where they also asked us to keep the session manager and the memory store apart as constructs. Verified against strands-agents 1.52.0 and @strands-agents/sdk 1.13.0: MemoryManager now ships its own per-model-call injection, and first-party vended stores exist to copy the idiom from. Notable outcomes: - One store class per SDK defining both write sinks, so extraction is server-side by default while add stays live for the manager's tools. - search() is an LTM fan-out over entities/preferences/facts, auto-gated per backend; add() writes a message into a deterministic sink conversation with extraction, with metadata["kind"] routing typed writes. - The store stays free of Strands internals: both coexistence guards live in the session manager, which is the side that gets an agent. - Store is positioned as the preferred memory construct; the session manager keeps transcript duties and gains no memory framing.
374 -> 337 lines. Cut narrative framing and restated context; kept every file:line citation, version, default and decision (verified: 21 citations and version constraints before, 21 after). Dropped the "Key design decisions (record)" section, which restated the Architecture, Scoping, Write durability and TS parity sections. Its four unique claims moved to where they belong: get_context() exclusion to Non-Goals, the add_memory name collision to the get_tools row, one-store- per-scope to Scoping, type-only import to TS parity. Also removed a leftover "(Resolves A6.)" reference to the internal handover document.
MemoryStore landed in 1.44.0; the previous >=0.1.0 floor resolved to 1.23.0, which has no strands.memory module at all.
Sibling to _retrieve_context that returns structured rows instead of a formatted context block, so a MemoryStore can map them to MemoryEntry. Per-kind error isolation; preference/fact searches skipped on NAMS.
Bolt sets metadata["similarity"] on entities, preferences, and facts alike (search_entities/search_preferences/search_facts in long_term.py all set it), but _preference_row and _fact_row never read it — only _entity_row did. A consumer sorting or filtering on metadata["score"] would drop every preference and fact hit on bolt, indistinguishable from an unscored NAMS hit, skewing what a MemoryManager injects. Extracted the score-lookup into a shared _row() builder used by all three row mappers so the behavior can't drift apart again.
The metadata table credited only search_entities with setting metadata["similarity"]. Bolt sets it on preferences and facts too (long_term.py:1126, :2065), which is why _preference_row and _fact_row were dropping it — fixed in 1b41697.
Config, protocol attributes, construction paths (borrowed vs owned client), for_nams, and an idempotent initialize. add/add_messages are deliberately undefined: a stub would flip _has_method's write-sink detection and change how extraction resolves before tasks 7/8 give those methods real behavior.
…path for_nams now reuses resolve_nams_connection/build_nams_settings instead of reimplementing them, so a missing MEMORY_API_KEY raises immediately with the same message, endpoint/api_key/transport_mode can be passed explicitly, and validate_on_connect defaults to False (Strands drives short synchronous bursts; skip the extra round-trip on connect). Add tests for the settings-owned construction path and the rewritten context-manager test, which previously monkey-patched _owns_client onto a client-constructed store and so could not catch a broken else branch. Trim initialize()'s docstring: it does not resolve a write sink yet.
AddMessagesContext carries no session identity, so the store owns its write scope. The sink name is deterministic and matched via conversation metadata, so restarts reuse one sink instead of accumulating orphans on NAMS.
Bolt keys conversations by session_id and create_conversation is idempotent under it, so listing up to 1000 conversations before creating by deterministic name was a wasted round-trip on every store's first write. NAMS still lists and matches on _STORE_KEY metadata, since it mints its own conversation ids.
…solve_sink Bolt's CREATE_CONVERSATION query has no metadata property and ShortTermMemory.create_conversation swallows metadata in **kwargs without forwarding it, so the eager create added in the previous fix could never tag a conversation as a memory-store sink. It was also redundant: add_message and add_messages_batch both auto-create the sink via _ensure_conversation on first write. Bolt now resolves to the deterministic name directly, with no backend call at all, matching Neo4jSessionManager._aresolve_conversation. FakeShortTerm.create_conversation now drops metadata in bolt mode too, so bolt-mode tests can't assert a tag a real bolt conversation would never carry. Strengthened the NAMS reuse test to assert the cached key is the minted uuid (not the deterministic name), and added a symmetric scan-count assertion for NAMS matching the existing zero-scan assertion for bolt.
The scoping section said the sink is tagged "where the backend accepts conversation metadata at creation", implying bolt does so conditionally. It cannot: CREATE_CONVERSATION (graph/queries.py) has no metadata property and create_conversation drops the kwarg (memory/short_term.py:521-524). Replaced with the per-backend split the code actually implements: bolt resolves to the deterministic name with no backend call, relying on the first write's _ensure_conversation to create the conversation; NAMS matches _STORE_KEY metadata and caches the server-minted id.
Long-term fan-out mapped to MemoryEntry, with kind/id/type/score metadata. Limit precedence: per-call option, then store default, then Strands' 3. Entities only on NAMS.
Four of the search() tests were passing for the wrong reasons: - test_kind_flags_are_honoured only populated the enabled kind, so a dropped include_* kwarg in the implementation would sail through. Now populates all three kinds and asserts search_calls == 1. - test_search_does_not_mint_a_sink used the bolt fake, where _resolve_sink() never calls the backend regardless of whether search() calls it. Switched to nams_mode=True, where an accidental _resolve_sink() call is observable via list_conversations_calls. - Limit precedence test didn't cover max_search_results=0, which an `if not limit` rewrite would silently break. - No test exercised search()'s own initialize() call without a prior explicit initialize() -- added one asserting connect_calls == 1. Implementation unchanged; all fixes are to test bodies only.
Default sink is a message written with extraction, the one path every backend supports. metadata['kind'] routes preference/fact/entity writes, falling back to the sink where NAMS does not expose the endpoint.
- FakeLongTerm.add_entity now mirrors NAMS's bare-Entity return (vs. bolt's (Entity, DeduplicationResult) tuple), exercising the isinstance narrowing in _add_typed for the case it exists for. - Add tests: entity kind on NAMS, entity name/type defaults, no dual write on entity routing. - _add_typed's entity branch is now an explicit `if kind == "entity"` with a ValueError on an unmatched kind, instead of a silent fall-through. - The unsupported-kind fallback warning is now logged at most once per store per kind, not once per call.
Server-side extraction sink: the filtered batch goes straight to bulk_add_messages, chunked at 100. At-least-once retries are deduped in-process on (run_id, sequence_number), since sequence numbers reset each run and message writes have no durable idempotency key. Widened ShortTermProtocol.bulk_add_messages to accept **kwargs, matching what both the bolt and NAMS implementations already accept and forward — the narrower Protocol signature was a pre-existing gap that add_messages's extract_entities/user_identifier call exposed.
Neo4jMemoryStore.add_messages passes user_identifier to bulk_add_messages, but ShortTermMemory.add_messages_batch had no such parameter and no **kwargs catch-all, so bolt raised TypeError on every call. Under multi_tenant=True the bulk path also silently wrote unscoped, unlinked conversations with no enforcement error. Give add_messages_batch a user_identifier parameter that enforces multi_tenant (mirroring add_message) and links the conversation to its :User node. Widen ShortTermProtocol.bulk_add_messages to declare the same explicit keyword-only params instead of **kwargs. Tighten FakeShortTerm.bulk_add_messages to the same explicit signature so it can no longer absorb a keyword the real backend would reject, and add a regression test that binds the store's forwarded kwargs against the real add_messages_batch signature. Add integration coverage for the bulk-path tenant link and guardrail.
Replace the **store_config: Unpack[Neo4jMemoryStoreConfig] pattern with a plain @DataClass: config.user_id is now a checked attribute instead of a string-keyed config.get("user_id") read. Validation (non-empty name; not both client and settings) moves into __post_init__ so it fails at the line the caller wrote; the "neither given" case is checked in Neo4jMemoryStore.__init__ instead, since a config en route to for_nams legitimately omits both until for_nams completes it via dataclasses.replace (which never mutates the caller's config). Also: read is_nams/is_connected directly instead of through getattr fallbacks now that both are real MemoryClient properties; return a local variable from _resolve_sink instead of the Optional attribute to clear the type-checker warning; unpack the fact-kind metadata directly instead of building a tuple just to destructure it; construct real Neo4jConfig/SecretStr instances in the settings-related store tests instead of a raw dict; cast the fakes passed into LongTermProtocol-typed _retrieve_entries parameters; and reorder/tidy the CHANGELOG Changed section.
get_entity_graph (get_related_entities on bolt, expand_graph on NAMS) and get_user_preferences (bolt only). Bound to the store's own client, and deliberately excluding search/add so nothing collides with the manager's search_memory / add_memory.
Replace cast(Any, client.long_term) with cast("NamsLongTermMemory", ...) /
cast("LongTermMemory", ...), scoped per branch, so the backend-specific
expand_graph and get_related_entities(depth=...) calls stay type-checked
instead of escaping mypy entirely.
get_user_preferences was calling search_preferences, which silently returns [] with no embedder and has no :User filter at all -- under multi_tenant it could leak another tenant's preferences. Switch to get_preferences_for, which is user-scoped and needs no embedder, and gate the tool on bolt + store.user_id being set (NAMS has no preferences endpoint; without a user_id there is nothing safe to scope to). Unscoped preference recall still reaches the model through the manager's own search_memory. Also tightens the get_tools test suite: drops a name-collision test fully subsumed by the exact-set assertion, asserts nodes/edges shapes precisely instead of truthiness, and adds depth-clamp and _MAX_EDGES-cap cases.
Pairing both is supported and recommended. Two overlaps are not: double extraction raises (the store can only extract by re-writing turns the session manager already persisted, and NAMS extracts every write regardless of our flag), double injection warns once. Guards live here because MemoryStore.initialize() gets no agent, keeping the store free of coupling to strands internals.
test_ignores_a_memory_manager_holding_only_foreign_stores previously used extraction=False (the TestMemoryStore default), so it passed regardless of whether _our_stores filtered by isinstance or duck-typed on the extraction attribute. Turn extraction on for the foreign store so the test actually proves the isinstance(store, Neo4jMemoryStore) filter, not just that a non-extracting neighbor is harmless.
…truct Guide leads with Neo4jMemoryStore; the memory-tools and Neo4jRetrievalConfig sections point at it for recall, plus the session-manager pairing shapes and a runnable no-API-key example (examples/strands-memory-store/).
…ontainer Neo4jMemoryStore's example uses a 384-dim embedder; the docker-compose container's named volume survives neo4j-stop/start, so a prior example run leaves indexes sized wrong for the integration suite's 1536-dim MockEmbedder. Add a fixture that detects this and skips with the actual cause and fix (make neo4j-clean) instead of a buried generic message, and a README note. Also: correct the pairing-table's unconditional "raises" claim (bolt + extract_entities=False does not raise), and add the missing doc-import-resolution test for the memory-store guide section.
Agent.__init__ initializes the store through strands._async.run_async
(asyncio.run in a throwaway thread) and every Agent.__call__ uses a
different loop. The neo4j async driver and the NAMS transport bind to the
loop that opened them, and initialize() returned early on _initialized,
so the guide's Quick Start raised "Task ... attached to a different loop"
from inside the driver on the first real call.
initialize() now records the running loop and, on a change:
- owned client (settings=): close and reconnect on the current loop.
One reconnect per synchronous Agent(...) invocation.
- borrowed client (client=): raise, naming the problem and both
remedies. Closing or reconnecting someone else's client is not ours
to do.
aclose() resets the initialization state so re-entering the async context
manager reconnects, and tolerates a client whose loop has already gone.
Also drops the write-only _conversation_id attribute (_sink_key carries
the value) and trims the config docstring's argument against the
rejected TypedDict design, which belongs in the spec.
.github/workflows/ci-python.yml runs `ruff format --check src tests` as a blocking step. `make lint` is only `ruff check` and does not cover it, so these three drifted. No behaviour change: `ruff format` output only.
… docs Findings from the whole-branch review, other than the loop rebinding and the formatting gate. get_entity_graph reported every bolt edge as an invented `relationship_type` attribute that the real Relationship model does not have (it carries `.type`), and inverted the direction the library reports. Now reads `.type` and orients edges by the relationship's own source_id/target_id. The fake returned an invented relationship shape, so the unit test asserted "WORKS_AT" — unreachable in production: the bolt path cannot report a relationship's own type at all, because execute_read's result.data() flattens a relationship to (start, type, end) and drops its properties. The fake now returns real Relationship objects shaped the way the bolt path really shapes them, the unit test asserts that, and a new integration test pins it against a real Neo4j so a library-side fix surfaces as a failing assertion. The library defect itself is left for a separate change. get_tools' names are now prefixed with the store's name. ToolRegistry skips its duplicate-name check for @tool functions (supports_hot_reload is always true there) and silently overwrites, so the store's get_entity_graph / get_user_preferences were replacing context_graph_tools' identically-named tools — which take different arguments — in the guide's own combined snippet. Prefixing by store name also lets several stores (the documented personal / team / org shape) coexist. search() applied its limit per kind, so max_search_results=5 returned up to 15 rows, entity-first; Strands caps per store and then slices the concatenation, so five entity hits starved preferences and facts entirely. The budget is now handed out round-robin across the enabled kinds, capped at the requested total. add(kind="preference") passes user_identifier, so the write gets its (:User)-[:HAS_PREFERENCE] edge — which is exactly what the store's own get_user_preferences reads — and does not raise under multi_tenant=True. The unit-test CI job installs the `strands` extra. Without it every strands test file's `pytest.importorskip("strands")` skipped, hiding 136 tests including the tripwire for a strands rename of MemoryManager._stores. Verified the extra resolves and the tests run on both 3.10 and 3.13. Docs and spec: user_id scopes writes, not reads; the three spec-mandated store limitations are now in the guide (separate sink, in-process retry dedupe, conversation_id pointed at the chat history); the row-3 pairing exception said "neither side extracts" where the store owns extraction; the spec's "one private attribute" limitation now names both (_stores and _injection_config) and its Testing table no longer promises a NAMS integration suite that was not shipped. Plus: _row() is keyword-only, the get_preferences_for docstring's stated reason for its cast was wrong (it is on the protocol, with a different calling convention), two short_term docstrings named MemorySettings.multi_tenant instead of MemorySettings.memory.multi_tenant, and the CHANGELOG's Fixed entry now states the observable break.
…resolution, rename _loop - _store_tools.py: get_preferences_for is on LongTermProtocol with a keyword-only user_identifier; call it that way and drop the now-unneeded cast (and the stale docstring claiming otherwise). Annotate the two @tool-built objects explicitly as AgentTool via a typed local, which is IDE-robust against decorator-overload inference gaps without a cast/ignore. - memory_store.py: _resolve_sink now has a single cache-and-return exit; the NAMS list-match-else-create branch moves into _resolve_nams_sink(). Behaviour unchanged (bolt: no backend call; NAMS: list/match/create). - memory_store.py: rename _loop to _connected_loop with a comment noting it is only ever compared, never awaited on; update the two integration test reads of the private attribute to match.
The hand-rolled NAMS fake was more permissive than the backend it stood
in for and produced three review-caught bugs (invented Relationship
attribute, bolt metadata that CREATE_CONVERSATION has no property for,
(Entity, None) where NAMS returns a bare Entity). Remove the class of
defect on the NAMS side: nams_mode=True now instantiates the real
NamsShortTermMemory / NamsLongTermMemory over StubTransport, a stub of
the single HttpTransport.request method (subclassed, so the constructor
and signature are the real ones). Bolt keeps its duck-typed fakes -- the
real bolt classes need a live driver -- and the docstring says why the
asymmetry is deliberate.
Assertions are re-pointed from vanished fake recorders to the recorded
requests, which is what NAMS would actually receive:
- add_entity: {"name": ..., "type": "organization"} -- the POLE+O type is
mapped into NAMS' lowercase vocabulary, not passed through as the old
fake's added_entities claimed.
- list_conversations: query params {"userId": ..., "limit": 1000}.
- add_message: {"content", "role"} only -- metadata, extract_entities and
user_identifier are dropped at the boundary.
- create_conversation: {"metadata": {...}} only; the Strands session id
survives solely inside metadata.
- The cross-instance sink reuse test now feeds the recorded create body
back as the listing, pinning "the metadata written is the metadata
matched on" instead of trusting a fake server to echo it.
- The preference/fact skip tests assert no request and no swallowed
failure, since NotSupportedError is caught by the gather.
Bolt fakes lose their NAMS branches, the NAMS-only expand_graph, and the
NotSupportedError impersonation they no longer need.
…cast _entity_graph's NAMS branch cast client.long_term to NamsLongTermMemory and called expand_graph on faith. Inside that branch httpx is installed by definition, so the concrete class can simply be imported and checked -- a mismatched backend now raises a TypeError naming what it got instead of an AttributeError from the call. Nested import, matching how for_nams imports build_nams_settings; the now-redundant TYPE_CHECKING entry is gone. Only testable now that nams_mode=True instantiates the real NamsLongTermMemory: against the old duck-typed fake, isinstance would have failed. The bolt branch keeps its cast -- LongTermProtocol declares get_related_entities without the depth parameter, so narrowing to the concrete class is genuinely required there.
… test _normalize_conversation's docstring said NAMS omits `metadata` from both GET and create responses. A live probe showed metadata round-trips through create and list_conversations, which is what Neo4jMemoryStore._resolve_nams_sink relies on. Docstring corrected, no behavior change. Adds the key-gated NAMS integration test promised in the design spec's Testing table but never shipped: two Neo4jMemoryStore instances with the same name/user_id resolve to the same sink, backed by exactly one metadata-tagged conversation. Reuses conftest.py's credential resolution and skip gate.
…s path Every existing test of Neo4jMemoryStore drives the store directly, on one event loop -- the blind spot that let a Critical through review. Strands' synchronous entry point runs Agent.__init__ on one throwaway loop and each Agent.__call__ on another, so a settings=-constructed store connected its client on the construction loop and then raised "attached to a different loop" on the first call. initialize() now rebinds an owned client; nothing exercised that end-to-end. tests/e2e/test_strands_agent_nams_e2e.py constructs a real Agent over hosted NAMS and a local tool-calling Ollama model, calls it twice synchronously (construction-loop -> call-loop, then call-loop -> call-loop), and asserts the memory landed in the sink conversation, comes back out of store.search(), that the store's graph tool is registered under its namespaced name while get_user_preferences is absent on NAMS, and that MemoryManager's <memory> block reached the model (read off a RecordingModel subclass overriding the public Model.stream -- no privates). A sync test on purpose: asyncio_mode="auto" would otherwise run the body inside a loop, and the fresh-loop-per-call entry point is the thing under test. Extraction is awaited via wait_for_extraction/get_extraction_status, never a sleep, and every live call is bounded by asyncio.wait_for. Gated so it never breaks anyone's CI: skips unless MEMORY_API_KEY is in the process environment and the Ollama endpoint answers a 5s probe. New `e2e` marker, deliberately not `integration` (the root conftest ties that marker to Neo4j reachability). Teardown deletes every conversation the run created and asserts they are gone; NAMS_E2E_KEEP=1 leaves them for inspection in the web UI.
…ver noise The SDK cannot clean up what these two tests create. clear_session deletes a conversation but not the entities NAMS extracted from it, and NamsLongTermMemory declares _SPEC_DELETE_ENTITY while exposing no delete_entity() -- so entities, and the pending fuzzy-merge candidates the resolver files against them, accumulate permanently in whatever workspace the credentials point at. A live run is a one-way write. Our own runs already left the owner a review backlog to clear by hand. So both tests now refuse to run unless NAMS_E2E_WORKSPACE_ID names a workspace that is not MEMORY_WORKSPACE_ID (nor its NAMS_SANDBOX_WORKSPACE_ID alias): unset and equal each skip with a reason that states why the variable is separate. The gate is new tests/nams_live.py, called at module scope before every other gate -- nams_credentials is session-scoped and would otherwise report a missing key first, hiding whether the gate is wired at all. Both docstrings now record that the opik pytest plugin loads .env into os.environ regardless of the shell, so skips must be verified with `-p no:opik`; two agents lost an hour to that. The workspace is wired in explicitly rather than inherited: the integration test overrides conftest's nams_config fixture, and build_nams_settings gains a keyword-only workspace_id forwarded to NamsConfig -- the only missing link, since without it MemorySettings falls back to MEMORY_WORKSPACE_ID and the gate would be theatre. Default None keeps every existing caller unchanged. Second fix: the e2e fixture's entity name was one stem plus a hex run suffix (Zorbium89980F486C, Zorbium868C72705A, ...). Those are ~85% similar to each other, so every run handed the resolver a fresh merge candidate against every previous run. TOKEN is now one word drawn from a 128-word pool of pronounceable nonsense, built by greedy rejection sampling that admitted a candidate only at <= 55 against every incumbent under all four of rapidfuzz's ratio, WRatio, partial_ratio and token_sort_ratio. Measured worst case over all 8128 pairs: 54.5, with no word a substring of another. Distinct runs cannot reach the threshold; identical draws (1 in 128) are exact matches that merge and queue nothing. One word, not two concatenated: a shared 8-char half measured 81.5 under partial_ratio, and a three-syllable scheme measured 90.9 -- only whole-word distinctness holds substring-flavoured scorers down. RUN_ID still namespaces the store name, user id and sink metadata, so runs stay traceable; the run banner prints the codename since it is no longer derivable from RUN_ID. Third: the aws-strands guide claimed user_id scopes writes via ":User edges", which is bolt-only. NAMS stores tenancy as a plain userId property on the conversation -- no node, no edge -- and filters the conversation listing on it; _link_user_to_conversation exists only in memory/short_term.py. Guide and design spec now state one clause per backend.
The Strands agent e2e test required a local Ollama serving a tool-calling model and a dedicated throwaway NAMS workspace -- runnable on one machine, never in CI. Removes it with tests/nams_live.py, the e2e pytest marker, the test-strands-agent-e2e target, and the build_nams_settings workspace_id kwarg that had no other caller. tests/integration/nams/test_strands_memory_store.py stays, gated by the existing NAMS conftest: it creates conversations only, so clear_session teardown is complete and it needs no separate workspace.
Andy2003
requested review from
johnymontana
and
a balanced review from Copilot
August 20, 2026 15:58
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Python support for Strands’ MemoryStore protocol, enabling Neo4j-backed cross-session recall alongside existing transcript persistence.
Changes:
- Implements store search, ingestion, graph tools, lifecycle handling, and coexistence guards.
- Adds tenant-scoped bulk-message support and raises the Strands dependency floor.
- Expands integration, unit, documentation, and example coverage.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci-python.yml |
Installs Strands during unit CI. |
CHANGELOG.md |
Documents the feature and fixes. |
docs/modules/ROOT/pages/how-to/integrations/aws-strands.adoc |
Adds MemoryStore guidance. |
docs/superpowers/specs/2026-08-19-strands-memory-store-design.md |
Records the design. |
examples/strands-memory-store/README.md |
Documents the example. |
examples/strands-memory-store/main.py |
Demonstrates the store API. |
pyproject.toml |
Raises the Strands minimum version. |
uv.lock |
Updates the resolved Strands package. |
src/neo4j_agent_memory/core/protocols.py |
Expands bulk-message protocol parameters. |
src/neo4j_agent_memory/integrations/strands/__init__.py |
Exports the new store. |
src/neo4j_agent_memory/integrations/strands/_retrieval.py |
Adds MemoryEntry retrieval fan-out. |
src/neo4j_agent_memory/integrations/strands/_store_tools.py |
Adds namespaced graph tools. |
src/neo4j_agent_memory/integrations/strands/config.py |
Improves NAMS configuration docs. |
src/neo4j_agent_memory/integrations/strands/memory_store.py |
Implements Neo4jMemoryStore. |
src/neo4j_agent_memory/integrations/strands/session_manager.py |
Adds coexistence guards. |
src/neo4j_agent_memory/memory/short_term.py |
Adds tenant-scoped batch writes. |
src/neo4j_agent_memory/nams/short_term.py |
Clarifies NAMS metadata normalization. |
tests/docs/test_code_snippets.py |
Verifies documented imports. |
tests/examples/test_strands_memory_store_example.py |
Smoke-tests the example. |
tests/integration/nams/test_strands_memory_store.py |
Tests live NAMS sink reuse. |
tests/integration/test_multi_tenant_scoping.py |
Tests scoped bulk writes. |
tests/integration/test_strands_memory_store_integration.py |
Tests Bolt behavior. |
tests/unit/integrations/strands_fakes.py |
Expands Bolt and NAMS test doubles. |
tests/unit/integrations/test_strands_coexistence.py |
Tests coexistence guards. |
tests/unit/integrations/test_strands_memory_protocol.py |
Verifies protocol availability. |
tests/unit/integrations/test_strands_memory_store.py |
Covers store behavior. |
tests/unit/integrations/test_strands_retrieval_entries.py |
Covers retrieval mapping. |
tests/unit/integrations/test_strands_session_manager.py |
Updates NAMS session tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- search() and session-manager injection scoped preference recall to the store's user_id. search_preferences applies no :User filter, so a scoped construct could surface another tenant's preferences; get_preferences_for is the only user-scoped primitive, so a scoped lookup lists the user's active preferences instead of searching all of them. - initialize()'s loop guard keys on the recorded loop, not _initialized: aclose() leaves a borrowed client connected, so an aclose/re-enter cycle on a new loop fell through to the already-connected path and produced the driver's opaque error instead of the named one. - Store tools route through store.initialize() instead of a captured client: with injection disabled a tool call can be the store's first operation on a new loop. - add_messages banks retry tokens per chunk, not after the whole batch, so a batch failing on chunk 2 is not re-written from chunk 1 on retry. - _tool_prefix is 1:1. Sanitisation alone maps team/graph and team graph to the same prefix, and ToolRegistry overwrites duplicate @tool names silently; names that need rewriting now carry a digest of the original. - Quick Start configures extraction. extraction defaults off and MemoryManager disables add_memory by default, so the documented agent("Remember ...") persisted nothing; extraction=True alone is every fifth turn, hence the explicit InvocationTrigger. - The doc field-drift guard resolves a class by its import, not by name alone: strands.memory.ExtractionConfig and our config.settings one share a name and no fields.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/neo4j_agent_memory/integrations/strands/session_manager.py:235
- An empty
ExtractionConfig()is a valid enabled Strands configuration (it resolves to the default trigger/extractor), but it is falsey. This filter therefore misses that store and allows the session manager and store to extract the same turns twice. Test forNone/Falseexplicitly rather than truthiness.
extracting = [store for store in stores if store.extraction]
src/neo4j_agent_memory/integrations/strands/_retrieval.py:231
- If every enabled backend query fails (for example, Neo4j is unavailable), each exception is converted to an empty bucket and
search()returns[]. That contradictsNeo4jMemoryStore.search()'s stated total-failure behavior and preventsMemoryManagerfrom recording a failed store; preserve per-kind isolation only when at least one query succeeds, and raise when all active queries fail.
for (kind, _, to_row), result in zip(active, results):
if isinstance(result, BaseException):
logger.warning("Long-term %s search failed: %s", kind, result)
per_kind.append([])
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements Strands' long-term-memory
MemoryStoreprotocol, so aMemoryClientcan backMemoryManager(stores=[...])for cross-sessionrecall. Complements
Neo4jSessionManager(per-session transcript) ratherthan replacing it; the two are documented as mutually exclusive for
extraction, and a coexistence guard raises instead of double-extracting.
Requires
strands-agents>=1.44.0.Behaviour worth reviewing
Agent.__init__and eachAgent.__call__on different loops(
strands._async.run_asyncisasyncio.runin a worker thread). A storebuilt from
settings=owns its client and reconnects it when the loopchanges; a client passed as
client=is never closed or reconnected, anda loop change raises a named error instead of an opaque driver
RuntimeError.strands-memory-store/{user_id or "_"}/{name}with no backend call; NAMSlists conversations and matches a metadata tag, creating one only on miss.
{store_name}_get_entity_graph/{store_name}_get_user_preferences.ToolRegistrysilently overwrites aduplicate name for
@toolfunctions, so unprefixed names would replacecontext_graph_tools' identically-named tools — which take differentarguments — with no warning.
max_search_resultscaps total rows persearch(), shared round-robinacross entities/preferences/facts so no kind is crowded out.
get_user_preferencesships only on bolt with a configureduser_id,via user-scoped
get_preferences_for.search_preferencesapplies no userfilter and returns
[]without an embedder — both a silent-empty and across-tenant-leak risk.
Incidental fix
bulk_add_messagesforwardeduser_identifiertoadd_messages_batch,which had no such parameter — every bolt
add_messagescall raisedTypeError.add_messages_batchnow accepts it and enforces multi-tenantscoping;
LongTermProtocol/ShortTermProtocolgain explicit parameters inplace of
**kwargs.Testing
--extra strands:these tests open with
pytest.importorskip("strands"), so the job wasreporting green on code it never ran.
tests/integration/test_strands_memory_store_integration.py(bolt),
tests/integration/nams/test_strands_memory_store.py(NAMS sinkmetadata round-trip).
examples/strands-memory-store/with a smoke test.Out of scope
TypeScript
Neo4jMemoryStoreand its guide ship separately.