feat(memory): add AgentCoreMemoryStore Strands integration - #588
feat(memory): add AgentCoreMemoryStore Strands integration#588strandly-the-agent wants to merge 6 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.
| @@ -124,8 +124,8 @@ jobs: | |||
| 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. |
Revert the AgentCoreMemorySessionManager relocation: session_manager, config, bedrock_converter and converters are byte-identical to main again, so existing import paths, patch targets, logger names and static API surface are unchanged. The new port stays contained under integrations/strands/memorystore. The only change to pre-existing files is a two-test adaptation to the public HookRegistry API required by the strands-agents>=1.46 floor.
|
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
|
max_search_results above AgentCore's topK limit of 100 reached the wire verbatim and failed as a raw ValidationException, while the same value with min_score set succeeded because the clamp lived inside the score-filter branch. Clamp on every path and warn once so the reduced cap is not silent. Deliberate divergence from the TypeScript source, requested in review.
4 participants
What changed in this revision
session_manager.py,config.py,bedrock_converter.py,converters/*and the package root are byte-identical tomain. The earlier relocation intostrands/memorysessionmanager/is reverted: it changedloggernames (...strands.session_manager→...memorysessionmanager.session_manager), dropped module globals from the static API surface, and relied on asys.modulesalias. The converters therefore live understrands/converters/again, as requested.store.py).MAX_TOPKis now applied on every retrieval path, not only themin_scorebranch, and a one-timelogger.warningreports the reduced cap so it is not silent. This is a deliberate, maintainer-requested divergence from the TypeScript source, which clamps only the over-fetch path.bedrock_agentcore.memory.integrations.strands.memorystoreand is purely additive.main(was conflicting):pyproject.toml/uv.lockkeep main's version and evals pins plus this PR'sstrands-agents>=1.46.0floor.TestAsyncModecases on the publicHookRegistry.get_callbacks_forAPI — required by the 1.46 floor (they fail onmaintoo under 1.46).Source: TypeScript PR #187 at
c7cb2bca47258108771e87966901d651aee3cef8· Target:8c06a2c· Closes #586Bug-bash matrix:
topKon the wire (was → now)Reproduced offline against a mock data-plane client at
8c06a2c(src/.../memorystore/store.py:210-231):topKbeforetopKnowmax_search_results=100, nomin_scoremax_search_results=101, nomin_scoreValidationExceptionmax_search_results=200, nomin_scoreValidationExceptionsearch(options={"max_search_results": 200})ValidationExceptionmax_search_results=200,min_score=0.1A cap above the limit now logs once per store:
AgentCoreMemoryStore <name>: max_search_results=200 exceeds AgentCore's topK limit of 100; searches return at most 100 records per request.Tests:
test_store.py:395covers 100/101/200 with and without a score floor,:421covers the per-callsearch_memoryoption,:428/:441cover warn-once and no-warning-at-100. The previoustest_result_cap_above_100_is_allowed_without_score_floor, which assertedtopK == 101, is gone.Still deferred to the fast follow you called on the
store.pythread: pagination (maxResults/nextToken), so a request for more than 100 records returns the first 100.Import-compatibility evidence (the failing Breaking Change check)
griffe==1.7.3, the same call the workflow makes, baseorigin/mainvs this branch:Public member sets of every pre-existing module are unchanged (only addition is the new
memorystoresubmodule):Runtime probe (import each module from a
mainworktree and from this branch; comparedir(),__all__,logger.name, and the owning module of every public symbol): identical, including the three logger names.At the earlier head the same check reported 7 breakages: 3 ×
logger: Public object was removed(from the relocation) and 4 caused by the branch being behindmain.The red Detect Breaking Changes run is the workflow's
actions/github-scriptstep getting403 Resource not accessible by integration—GITHUB_TOKENis read-only for fork PRs, so the informational comment cannot be posted. The griffe step itself exits 0; nothing in this PR can fix that permission (it needspull_request_targetor a PAT in the workflow).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_format.py:19format.test.ts:22,25,28test_format.py:24,:54format.test.ts:31test_format.py:40format.test.ts:37,40,43,46test_format.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:107store.test.ts:112test_store.py:114store.test.ts:128,144,155test_store.py:134store.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:247factory.test.ts:208,212,216,222,226test_factory.py:219,:225,:231tests_integ/memory.test.ts(live)tests_integ/memory/integrations/test_memory_store.py:67,:114,:162,:178,:213,:255— not executed (needs live AWS)Deliberate divergences from the source (both requested in review): the
topKclamp on every path (test_store.py:395,:421,:428,:441) and Python-nativestr.strip()/json.dumpsmetadata handling instead of ECMAScript emulation (test_format.py:40,test_sender.py:381,:406).Other Python-only tests cover Python-runtime concerns rather than new product behavior: 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:441–:514),TypedDictruntime metadata (test_types.py:18), package exports (test_package_exports.py), static typing fixtures (test_static_typing.py), and parity guards proving no Python-only request preflight was added (test_sender.py:332,:337,:345).Validation at
8c06a2cThe 2 failures and the
deepevalcollection error are pre-existing onmain— reproduced in amainworktree with the same interpreter/venv:evaluation/custom_code_based_evaluators/test_models.py::TestEvaluatorOutput::test_label_required_without_error_code,evaluation/integrations/strands_agents_evals/test_end_to_end.py::...::test_evaluation_with_empty_trajectory, andthird_party/deepevalneeding the optionaldeepevalpackage.mypyonsession_manager.pyreports the same pre-existing errors asmain.Live AWS was not run. The safety-gate failure is the repository's intended gate for any PR touching
.github/workflows/**(this PR adds the new live test to the memory matrix and installspytest-asyncio); a maintainer must trigger integration tests manually.Open items / decisions
store.pythread; thetopK-cap half of that thread is fixed here.CreateEventrequest preflight (payload/text/metadata service bounds) also stays as the source behaves — same fast-follow bucket. Say the word if you want it in this PR.AgentCoreMemorySessionManagercontainment understrands/memorysessionmanager: not in this PR — it is what broke the import surface. Ready to open it as a standalone PR.memorystore/README.md(correctedflush()semantics, standalone first example); no existing doc was touched.