feat(memory): add AgentCoreMemoryStore Strands integration - #588
feat(memory): add AgentCoreMemoryStore Strands integration#588strandly-the-agent wants to merge 2 commits into
Conversation
|
Meant to set this as draft, doing some work to validate parity, ignore for now. |
| @@ -0,0 +1,145 @@ | |||
| """Factory helpers for multi-namespace AgentCore memory topologies.""" | |||
| @@ -0,0 +1,62 @@ | |||
| """Internal Strands-message formatting helpers.""" | |||
|
|
||
| ## Native Strands long-term memory stores | ||
|
|
||
| `AgentCoreMemoryStore` plugs AgentCore long-term memory directly into Strands' `MemoryManager`. |
There was a problem hiding this comment.
This is meant to replace AgentCoreMemorySessionManager implementation long term. Lets remove mentions of AgentCoreMemorySessionManager since it long term will be on deprecation path
| MEMORY_KINESIS_ARN: ${{ secrets.MEMORY_KINESIS_ARN }} | ||
| MEMORY_ROLE_ARN: ${{ secrets.MEMORY_ROLE_ARN }} | ||
| MEMORY_PREPOPULATED_ID: ${{ secrets.MEMORY_PREPOPULATED_ID }} | ||
| MEMORY_STORE_TEST_ID: ${{ secrets.MEMORY_STORE_TEST_ID }} |
There was a problem hiding this comment.
Lets verify there is no existing MEMORY_STORE_TEST_ID we can re-use from the legacy AgentCoreMemorySessionManager.
| uv run pytest tests_integ/memory/test_memory_client.py -xvs | ||
| ``` | ||
|
|
||
| ### Strands memory-store integration |
There was a problem hiding this comment.
Same comment from above,
Let's verify there is no existing MEMORY_STORE_TEST_ID we can re-use from the legacy AgentCoreMemorySessionManager.
If this is available.
|
|
||
| Integration tests run via `integration-testing.yml` on PRs to main. The workflow runs runtime, memory (stream delivery only), and evaluation tests in parallel. | ||
| Integration tests run via `integration-testing.yml` on PRs to main. The workflow runs runtime, | ||
| control-plane/client/session-manager/native memory-store memory tests, evaluation, services, policy, |
There was a problem hiding this comment.
Please do not put paths like this in any comments or markdown documentation. Reference the class or primitive implementation as needed
| `MEMORY_STORE_TEST_ID` or fall back to `MEMORY_PREPOPULATED_ID`. | ||
|
|
||
| Stream delivery tests fail in CI unless `MEMORY_KINESIS_ARN` and `MEMORY_ROLE_ARN` secrets are configured on the repo. Other memory integration tests are not yet run in CI due to provisioning times and flaky LLM-dependent assertions — CI support is planned once test stability is addressed. | ||
| Changes to `.github/workflows/` trigger the workflow security gate and require maintainer security |
There was a problem hiding this comment.
Valid to add this for agents to read, but not relevant to this port. We can add this in a separate PR.
| @@ -0,0 +1,211 @@ | |||
| """Public types and namespace helpers for the Strands AgentCore memory store.""" | |||
There was a problem hiding this comment.
Can we have all these new memorystore related changes under /strands/memorystore so they are more contained
There was a problem hiding this comment.
Move AgentCoreMemorySessionManager related changes seperate /strands/ a /strands/memorysessionmanager. Do not mention this is legacy/deprecated yet, will have this in a follow up.
| @@ -1,5 +1,145 @@ | |||
| """Strands integration for Bedrock AgentCore Memory.""" | |||
| """Strands integrations for Bedrock AgentCore Memory.""" | |||
There was a problem hiding this comment.
This runtime check is overkill
we have raised the minor version requirement to "strands-agents>=1.46.0". So client will just upgrade their strands version on the next agentcore release.
| path: tests_integ/runtime | ||
| timeout: 10 | ||
| extra-deps: "" | ||
| ignore: "" |
There was a problem hiding this comment.
General high level.
We missed a lot of the existing inline code comments from the typescript package during the port.
Make sure we are bringing over any comments that are relevant to the python version
| return f"{self._memory_id}-{self._actor_id}-{self._run_id}-{first}-{last}" | ||
|
|
||
|
|
||
| def _ecmascript_number_string(value: int | float) -> str: |
There was a problem hiding this comment.
We are hyper-optimizing with these ecmascript helper functions. Please just rely use json.dumps(metadata, sort_keys=True)
| return "{" + ",".join(entries) + "}" | ||
|
|
||
|
|
||
| def _metadata_scalar_string(key: str, value: object) -> str: |
There was a problem hiding this comment.
this is _metadata_scalar_string is overkill aswell lets maintain parity with a logic like
if value is None or (isinstance(value, float) and not math.isfinite(value)):
raise ValueError(...)
string_value = value if isinstance(value, str) else json.dumps(value)
|
|
||
| from bedrock_agentcore._utils.user_agent import build_user_agent_suffix | ||
|
|
||
| from ._format import _ecmascript_trim |
There was a problem hiding this comment.
in general for this whole port. this is a python package. lets not try to reach super optimized parity with ecmascript.
|
@strandly-the-agent review this |
There was a problem hiding this comment.
Superseded — reconciled at 8f2fac4. This review was written against 8a9543ee; the verdict below no longer reflects the current state.
- The two documentation findings (
flush()semantics, non-runnable first example) are fixed in the containedmemorystore/README.md; their original threads pointed at the old root README, which is now byte-identical tomain, so I deleted those two stale comments rather than leave them dangling. - The two service-contract findings (retrieval pagination/
maxResults, fullCreateEventrequest preflight) are intentionally not implemented: they are behavior the TypeScript source does not have, andarielnabavianconfirmed on the pagination thread that this matches TypeScript and belongs in a fast follow. Those two threads stay open as that record — no further nagging from me. - Nothing here is a blocker on the current head. Verification evidence for
8f2fac4is in the PR description.
| top_k = min(math.ceil(want * self._over_fetch_factor), MAX_TOPK) | ||
| kwargs: dict[str, Any] = { | ||
| "memoryId": self._memory_id, | ||
| "searchCriteria": {"searchQuery": query, "topK": top_k}, |
There was a problem hiding this comment.
🔴 Retrieval silently truncates the requested count. max_search_results=50 sets topK=50, but this omits top-level maxResults (service default: 20), and the response path below ignores nextToken. Two cold local runs returned 20 entries after one call. Values above 100 also emit an invalid topK because AgentCore caps it at 100.
Please set maxResults, follow pagination until the requested count/no token, and reject or otherwise define behavior for requests above the service cap.
There was a problem hiding this comment.
This is the same implementation in typescript. I suggest we fix this as a fast follow
| Raises: | ||
| ValueError: If ``max_turns_per_event`` is not a positive integer. | ||
| """ | ||
| if type(max_turns_per_event) is not int or max_turns_per_event < 1: |
There was a problem hiding this comment.
🔴 Preflight every CreateEvent group before dispatching any of them. Cold repros passed a 101-item payload, 100,001-character text, 16 metadata entries, a 129-character key, and a 257-character value into the client; botocore did not reject them locally. AgentCore limits these to 100 payload items, 100,000 text characters, 15 metadata entries, 1–128-character patterned keys, and 0–256-character patterned values.
Because _send_all dispatches sibling groups concurrently, an invalid group can fail only after another group has committed. Please fully materialize and validate every group before gather(), cap max_turns_per_event at 100, and update the tests to cover exact boundaries plus zero client calls when a later group is invalid. The README’s “any positive integer” claim must change with that cap.
|
@strandly-the-agent read all the comments on this pr. Read the comments thoroughly and makes sure we are filling the requirements of the given task, (port typescript AgentCoreMemoryStore to python AgentCoreMemoryStore). Make sure not to add any logic changes outside of whats required for 1-1 parity. Make sure the code comments are all address, make a revision to the PR once you are done. |
|
Revision pushed at
Review dispositionThe two earlier Strandly implementation blockers requested behavior beyond the TypeScript source. Per the explicit 1:1-parity direction, this revision intentionally does not implement them. The two documentation findings are fixed. Full traceability and validation evidence are in the updated PR description. |
|
Final parity follow-up pushed at The independent correctness pass found one Python-only overflow edge in over-fetch calculation: a huge finite factor overflowed to Current focused validation: 386 passed; Ruff, format, lock, and diff checks pass. The PR description now points to the final head and updated traceability evidence. Live AWS remains unrun. |
|
For sanity, ran a manual audit as well full_suite_interval_results.xlsx |
|
@strandly-the-agent make a revision. Make sure we are not messing up any imports for existing clients. specifically these 1s I also recommended moving our converters back under strands folder rather than memorysessionmanager folders since these are more reusable. Also please investigate this bug we found in a bug bash:
store.py:208-213 computes top_k: top_k = want Verified against the live service: config topK sent result Fix: hoist the clamp out of the if — top_k = min(top_k, MAX_TOPK) — or reject max_search_results > MAX_TOPK at construction and in search. Revise the CR. |
|
Revision pushed at 1. Imports for existing clients. The griffe step in Detect Breaking Changes exits 0 — the job died on its 2. Converters back under 3. Bug-bash defect:
This is a deliberate divergence from the TypeScript source (which clamps only the over-fetch path) — flagged as such in the PR description so it does not read as a parity slip. I went with hoisting the clamp rather than rejecting Evidence & open items
|
| path: tests_integ/memory/test_controlplane.py tests_integ/memory/test_memory_client.py tests_integ/memory/integrations/test_session_manager.py | ||
| timeout: 15 | ||
| path: tests_integ/memory/test_controlplane.py tests_integ/memory/test_memory_client.py tests_integ/memory/integrations/strands/memorysessionmanager/test_session_manager.py tests_integ/memory/integrations/strands/memorystore/test_memory_store.py | ||
| timeout: 30 |
There was a problem hiding this comment.
Why are we increasing this 30 minutes now?
| AgentCoreRole = Literal["USER", "ASSISTANT"] | ||
|
|
||
|
|
||
| def map_role(message: Message) -> AgentCoreRole: |
There was a problem hiding this comment.
It seems like these functions can be part of the AgentCoreSender class as private function _map_roles(). Let's shift these functions into AgentCoreSender if its only used by that class.
| RESERVED_METADATA_PREFIX = "_" | ||
|
|
||
|
|
||
| class AgentCoreDataPlaneClient(Protocol): |
There was a problem hiding this comment.
Cant we use the MemoryClient we already vend in this repo?
| ) | ||
|
|
||
|
|
||
| def assert_writable_topology(stores: Sequence[MemoryStore], expect_extraction: bool = False) -> None: |
|
@strandly-the-agent address these comments and revise the pr. |
Port the TypeScript AgentCoreMemoryStore to Python as a contained package under memory/integrations/strands/memorystore: exact/subtree recall, role-preserving batched writes, deterministic idempotency tokens, one-writer extraction topology, and typed public configuration. Existing Strands integration modules are untouched, so client import paths, patch targets and logger names are unchanged (griffe reports no breaking changes). Review follow-ups included: - topK is clamped on every retrieval path (a plausible max_search_results above the service limit previously failed as a raw ValidationException), with a warn-once log. - The sender-exclusive message helpers are private methods of AgentCoreEventSender. - The SDK's own MemoryClient is accepted wherever a data-plane client is, by unwrapping the client it already vends. - The internal topology guard is named _assert_writable_topology. - The integration-testing workflow is left untouched; wiring the live suite into CI is deferred to a maintainer-owned change.
8c06a2c to
ddd0ba5
Compare
Drop the memory-group path/timeout and pytest-asyncio hunks; wiring the new live suite into CI is a maintainer-owned change (review thread r3660049235).
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
GitHub Actions
Waiting
|
@strandly-the-agent address these comments from |
5 participants
Review responses in this revision
AgentCoreEventSender(r3707303752)_format.pyis gone and they are_map_role/_extract_text/_is_user_or_assistant_with_texton the class.MemoryClientwe already vend (r3708398179)client=MemoryClient(...)is accepted by the store, factory and sender, and its vendedgmdp_clientis used.MemoryClient's owncreate_eventis a(text, role)convenience API with noclientToken, so the raw data-plane surface is still what the store calls._assert_writable_topology(r3708398590)__all__; it is an internal topology guard..github/workflows/integration-testing.ymlis byte-identical tomain. Wiring the live suite into CI is a maintainer-owned change; details on the thread.Source: TypeScript PR #187 at
c7cb2bca47258108771e87966901d651aee3cef8· Target:7bc3595· Closes #586Scope: what this PR changes
No existing module, test, doc, or workflow is modified.
griffe(the Breaking Change Check's own call) reports 0 breaking changes againstmain, and a runtime probe of the pre-existing Strands modules (dir(),__all__,logger.name, owning module per public symbol) is identical to amainworktree — import paths, patch targets and logger names are untouched.Behavior traceability matrix (TypeScript test → Python test)
Source paths are relative to
src/memory/integrations/strands/__tests__/; target paths totests/bedrock_agentcore/memory/integrations/strands/memorystore/.file:line)file:line)format.test.ts:13,16mapRoleuser/assistant →USER/ASSISTANTtest_message_formatting.py:19format.test.ts:22,25,28test_message_formatting.py:24,:54format.test.ts:31test_message_formatting.py:40format.test.ts:37,40,43,46test_message_formatting.py:68sender.test.ts:57createEvent, role-tagged, orderedtest_sender.py:66sender.test.ts:70maxTurnsPerEventtest_sender.py:80sender.test.ts:79,238test_sender.py:93sender.test.ts:86,136clientTokenwithout complete sequence numberstest_sender.py:114sender.test.ts:92,105test_sender.py:126sender.test.ts:111,122test_sender.py:142,:150sender.test.ts:142,153test_sender.py:167sender.test.ts:166,198stringValue; omit empty bagtest_sender.py:188,:421sender.test.ts:179createEventtest_sender.py:212,:230,:238sender.test.ts:188test_sender.py:247,:358sender.test.ts:205,213,222test_sender.py:259,:281sender.test.ts:244maxTurnsPerEventtest_sender.py:291sender.test.ts:248,255extractionModepassthrough only when configuredtest_sender.py:303store.test.ts:67,401{actorId}/{sessionId}, query exactnamespacetest_store.py:58store.test.ts:75namespacePathtest_store.py:67store.test.ts:83MemoryRecordSummary→MemoryEntrymappingtest_store.py:81store.test.ts:105,137topK == wantwithout a score floortest_store.py:107,:395store.test.ts:112test_store.py:114store.test.ts:128,144,155test_store.py:134,:405store.test.ts:172test_store.py:155store.test.ts:179,195,201[]on empty, errors propagatetest_store.py:162,:170,:175store.test.ts:211,225,237test_store.py:183store.test.ts:251addMessagestest_store.py:206store.test.ts:259,268,277,309test_store.py:212,:225store.test.ts:285,299test_store.py:233,:253store.test.ts:292writabledefaults to falsetest_store.py:268store.test.ts:317,341test_store.py:280,:302store.test.ts:369,373,380test_store.py:318,:339store.test.ts:395$-sequences in{actorId}inserted verbatimtest_store.py:345,:382factory.test.ts:29,34,41test_factory.py:42factory.test.ts:47,54test_factory.py:53factory.test.ts:61,76writableflag / opt-out selectiontest_factory.py:62,:79factory.test.ts:91,105test_factory.py:96,:110factory.test.ts:118,126,131extraction: truepassthroughtest_factory.py:124,:134factory.test.ts:140,152test_factory.py:140factory.test.ts:162,166,180test_factory.py:166,:173,:191factory.test.ts:188test_factory.py:249factory.test.ts:208,212,216,222,226test_factory.py:219,:225,:233tests_integ/memory.test.ts(live)tests_integ/memory/integrations/test_memory_store.py:66,:113,:161,:177,:212,:254— not executed (needs live AWS)Deliberate divergences from the source, both requested in review: the
topKclamp on every retrieval path (test_store.py:405,:421,:428,:439) and Python-nativestr.strip()/json.dumpsmetadata handling instead of ECMAScript emulation (test_message_formatting.py:40,test_sender.py:381,:406).Python-only tests cover Python-runtime concerns rather than new product behavior:
MemoryClientacceptance (test_store.py:458,test_factory.py:262), blocking boto3 call kept off the event loop (test_sender.py:314), cancellation settlement (test_sender.py:430,:452), region/session resolution (test_store.py:475–:548),TypedDictruntime metadata (test_types.py:18), package exports (test_package_exports.py), and static typing fixtures (test_static_typing.py, incl. aMemoryClientcase).Validation at
7bc3595Live AWS was not run (the suite needs
MEMORY_PREPOPULATED_IDplus Bedrock model access). Two independent review passes were run over the review-response commit; both findings (widened client types not applied to the factory/TypedDicts, two stale docstrings) are fixed here.Open items
maxResults/nextToken) stays a fast follow, perarielnabavianon thestore.pythread; a request above 100 records returns the first 100.CreateEventrequest preflight (payload/text/metadata service bounds) also matches the source and is in the same fast-follow bucket.tests_integ/memory/integrations/test_memory_store.py,pytest-asyncio, and a timeout that accommodates eventual-consistency polling.