feat(memory): add AgentCore Memory Strands integration (experimental)#187
feat(memory): add AgentCore Memory Strands integration (experimental)#187aidandaly24 wants to merge 29 commits into
Conversation
aidandaly24
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import { Agent, MemoryManager, MessageAddedEvent } from '@strands-agents/sdk' | ||
| import { createAgentCoreMemoryStores, AgentCoreBatchTrigger } from 'bedrock-agentcore/experimental/memory/strands' | ||
|
|
||
| const stores = createAgentCoreMemoryStores({ |
There was a problem hiding this comment.
Could we create a thin convenience wrapper in the case a user wants to set up a single store?
const store = createAgentCoreMemoryStore({ // singular, returns one store
memoryId, actorId, sessionId,
namespace: '/users/{actorId}/facts',
})
reads much more intuitive if I have just 1 store, which I assume is the majority of the use cases here.
There was a problem hiding this comment.
Added createAgentCoreMemoryStore (singular) returning one store for the single-namespace case — 519d6c9. Same options, just namespace instead of namespaces.
| console.warn(`[agentcore-memory] search failed for store "${this.name}":`, err) | ||
| return [] |
There was a problem hiding this comment.
The memory manager ensure the agent loop doesn't break here, so you can throw. Otherwise failures are swallowed on the MM level
There was a problem hiding this comment.
Good catch — confirmed MemoryManager.search wraps each store in Promise.allSettled, so I removed the try/catch and let search() throw. Errors are now isolated per-store and surfaced by the manager instead of swallowed — 519d6c9.
| this.onDropped?.({ reason, text: extractText(message), ...(cause !== undefined && { cause }) }) | ||
| } catch (err) { | ||
| // Never let a customer callback break the write path. | ||
| console.warn('[agentcore-memory] onDropped callback threw:', err) |
There was a problem hiding this comment.
Can you use the SDK logger here instead?
There was a problem hiding this comment.
Done — routed through a module Logger (with configureMemoryLogging to inject) instead of console, mirroring the Strands logger convention — 519d6c9. (@strands-agents/sdk exports configureLogging/Logger but not the logger instance, so an external package owns a console-backed default of the same shape.)
| * store's per-namespace identity. The factory builds the shared config once and one of these per | ||
| * namespace. | ||
| */ | ||
| export interface AgentCoreMemoryStoreConfig { |
There was a problem hiding this comment.
Can this extend MemoryStoreConfig?
There was a problem hiding this comment.
Done — AgentCoreMemoryStoreConfig extends MemoryStoreConfig now (matching BedrockKnowledgeBaseStoreConfig), inheriting name/description/maxSearchResults/extraction — 519d6c9.
| { namespace: '/users/{actorId}/facts' }, | ||
| { namespace: '/users/{actorId}/preferences' }, | ||
| ], | ||
| trigger: new AgentCoreBatchTrigger({ messageCount: 4, maxDelayMs: 3000, messageAddedEvent: MessageAddedEvent }), |
There was a problem hiding this comment.
Can we to converge the config surface here? Currently there is some tension in that the trigger is flat (trigger for what?), it's mandatory (no defaults), and there are 3 things that can disagree: writable, the flat mandatory trigger, and writeNamespace.
For AC, writable ⟺ extraction-enabled ⟺ “writes via createEvent”. So can we collapse these into one switch, reusing the upstream boolean | config setup?
extraction?: boolean | { cadence?: ExtractionTrigger; namespace?: string }
// omitted/false -> recall-only
// true -> writable, default cadence
//
// { cadence } -> custom cadence
// { namespace } -> which namespace writes
// today
{ writeNamespace: '...', trigger: new AgentCoreBatchTrigger({ messageAddedEvent: MessageAddedEvent }) }
// proposed shorthand convenience
{ extraction: true }This would give it one mental model and make recall-only expressible
WDYT?
There was a problem hiding this comment.
Agreed — converged onto a single extraction?: boolean | { cadence?, namespace? } switch, removing the separate trigger/writeNamespace — 519d6c9. Omitting it now gives recall-only (which wasn't expressible before); true uses the default cadence; the object form sets custom cadence / write namespace. One pre-publish wrinkle: true needs a messageAddedEvent to build the default trigger (since the real MessageAddedEvent isn't importable until the flip), so it's a top-level factory input for now.
| 1. **Metadata-filtered recall (indexed keys) is not exposed to the agent.** AgentCore supports | ||
| `metadataFilters` on retrieval (gated on indexed keys declared at resource creation), but the | ||
| generic `MemoryStore.search(query, { maxSearchResults })` interface has no slot to carry a filter, | ||
| so the `search_memory` tool can't express one. Pending an upstream `SearchOptions` addition. |
There was a problem hiding this comment.
nit: per our discussion, this needs to be set as per-instance defaults on the store
There was a problem hiding this comment.
Fixed — reworded to 'per-instance store defaults' (bake the filter into store config like minScore), and dropped the stale 'pending SearchOptions' line since that addition was dropped upstream — 519d6c9.
Addresses opieter-aws review on #187: - Converge config: replace flat `trigger` + `writeNamespace` with a single `extraction?: boolean | { cadence?, namespace? }` switch (mirrors the upstream boolean|config shorthand). Omitting it now yields a recall-only topology. - Add `createAgentCoreMemoryStore` (singular) convenience for the one-namespace case. - `search()` no longer swallows errors; lets them throw so MemoryManager's Promise.allSettled isolates per-store failures (and surfaces them). - `AgentCoreMemoryStoreConfig` now extends `MemoryStoreConfig` (like the KB store). - Route logging through a module Logger (configureMemoryLogging) instead of console. - Fix CodeQL polynomial-regex flag in the namespace slugifier. - Docs: indexed-key filtering is per-instance store defaults (not a pending SearchOptions change, which upstream dropped); update examples to the new API.
| const extractionObj: AgentCoreExtractionConfig = typeof input.extraction === 'object' ? input.extraction : {} | ||
| const extraction: ExtractionConfig | undefined = writeEnabled | ||
| ? { trigger: extractionObj.cadence ?? defaultTrigger(input.messageAddedEvent) } | ||
| : undefined |
There was a problem hiding this comment.
For extraction: true, can we pass the boolean straight through to the store’s extraction field instead of eagerly building a trigger here? The MemoryManager defaults the trigger to the SDK’s own IntervalTrigger using its internal event import, so the default path needs no messageAddedEvent. Then you can keep AgentCoreBatchTrigger only for the explicit { cadence } case . Trade-off: the default becomes turn-based rather than byte/time (which seems fine for “just give me defaults", since that then syncs with Strands SDK). WDYT?
There was a problem hiding this comment.
Done in b4994b9 — extraction: true now passes the boolean straight through to the store's extraction field, so the MemoryManager resolves its own default trigger (IntervalTrigger) via its internal event import. We only build an ExtractionConfig for the explicit { cadence }/{ filter } case, and AgentCoreBatchTrigger is reserved for that. Agreed the turn-based default is the right "just give me defaults" behavior since it syncs with the rest of Strands.
| const retried = await Promise.allSettled(failed.map((m) => this.sendOne(m))) | ||
| retried.forEach((r, i) => { | ||
| if (r.status === 'rejected') { | ||
| this.reportDropped(failed[i]!.message, 'retry-failed', (r as PromiseRejectedResult).reason) |
There was a problem hiding this comment.
Failures are swallowed here. Because addMessages never throws, the ExtractionCoordinator never rolls back or re-fires. Consider throwing to make use of the coordinator’s retry
There was a problem hiding this comment.
Fixed in b4994b9. addMessages now throws (an AggregateError) on any createEvent failure, so the coordinator rolls back its high-water mark and re-fires the batch. I also removed the sender's own one-retry/onDropped/timeout-drop layer entirely — confirmed against coordinator.ts that it owns retry + backoff + repeated-failure logging, so that layer was duplicating it (and was the source of the #13 phantom-drop bug).
| */ | ||
| private clientTokenFor(item: SeqMessage): string | undefined { | ||
| if (item.seq === undefined) return undefined | ||
| return `${this.memoryId}-${this.actorId}-${this.sessionId}-${item.seq}` |
There was a problem hiding this comment.
I confirmed that sessionId does not refresh upon session restore. Since the memory manager currently does not support session history, the seq counter is set back zero on a restored session. That makes this identifier not unique to the message. What's the clientToken dedup window?
I would like to introduce unique message IDs in Strands which would solve this problem (we'd replace the seqNumber with a uuid), but we do not have this available today.
There was a problem hiding this comment.
As an interim mitigation, you could generate a run-unique ID at sender construction time and replace the sessionId with that
There was a problem hiding this comment.
Good catch, thank you for confirming the restore behavior. Fixed in b4994b9 by anchoring the clientToken on a run-unique id minted at sender construction (crypto.randomUUID() by default, overridable via runId) instead of sessionId — so a restored session that replays seq 0.. gets a different run segment and never collides. This is the interim mitigation you suggested in the next comment. Re: the dedup window — AgentCore's clientToken idempotency is bounded, not indefinite, which is fine here since we only need it to absorb a coordinator re-fire within a run. I'd happily switch to stable per-message UUIDs once Strands exposes them; the token derivation is a one-line change.
There was a problem hiding this comment.
Done exactly this in b4994b9 — a run-unique id is generated at AgentCoreEventSender construction and replaces sessionId in the token. Looking forward to swapping it for real per-message UUIDs when they land.
There was a problem hiding this comment.
The code has been released, so you can install the latest version
There was a problem hiding this comment.
Thanks — flipped in b4994b9. Deleted the vendored mirror and now import the interface directly from @strands-agents/sdk (bumped to >=1.5.0 peer / ^1.6.0 dev), and internalized the real MessageAddedEvent so AgentCoreBatchTrigger no longer needs the injected event.
| if (record.memoryRecordId !== undefined) metadata.id = record.memoryRecordId | ||
| if (record.score !== undefined) metadata.score = record.score | ||
| if (record.namespaces !== undefined) metadata.namespaces = record.namespaces |
There was a problem hiding this comment.
Consider using underscored keys for these reserved metadata fields to avoid potential collision with user-defined metadata
There was a problem hiding this comment.
Done in b4994b9 — the store-provided keys are now _id, _score, _namespaces (and _createdAt, per your other comment), underscore-prefixed so they can't collide with user-defined metadata.
| context.fire() // fire-and-forget; coordinator only processes messages past the high-water mark | ||
| } | ||
|
|
||
| context.agent.addHook(this.messageAddedEvent, (event: unknown) => { |
There was a problem hiding this comment.
Consider counting only isUserOrAssistantWithText messages here to match the Strands perTurn count. That would make it match with the memory trigger for other stores.
There was a problem hiding this comment.
Done in b4994b9 — the batch trigger now counts only messages where isUserOrAssistantWithText is true (skipping tool-only/empty turns), matching the coordinator's per-turn write accounting. Added a test for it.
| export type ReadMode = 'per-namespace' | 'subtree' | ||
|
|
||
| /** Per-message metadata attached to each `createEvent`. Values use the AgentCore metadata shape. */ | ||
| export type MetadataProvider = (message: MessageData) => Record<string, { stringValue: string }> |
There was a problem hiding this comment.
Is it an idea to make this more lenient (Record<string, JSONValue>) and map to string internally?
There was a problem hiding this comment.
Done in b4994b9 — MetadataProvider now returns Record<string, JSONValue> and the sender maps each value to AgentCore's { stringValue } shape internally (stringifying non-strings via JSON.stringify).
| // Resolve the single `extraction` switch into: is writing enabled, with what cadence, on which namespace. | ||
| const writeEnabled = input.extraction !== undefined && input.extraction !== false | ||
| const extractionObj: AgentCoreExtractionConfig = typeof input.extraction === 'object' ? input.extraction : {} | ||
| const extraction: ExtractionConfig | undefined = writeEnabled |
There was a problem hiding this comment.
Is the filter passed through correctly here?
There was a problem hiding this comment.
You were right — the filter wasn't being threaded. Fixed in b4994b9: AgentCoreExtractionConfig now takes an optional filter, and the factory forwards it into the store's ExtractionConfig.filter. Added a test asserting it reaches the writable store.
| .map((r) => this.toEntry(r)) | ||
| } | ||
|
|
||
| private toEntry(record: MemoryRecordSummary): MemoryEntry { |
There was a problem hiding this comment.
Consider setting createdAt timestamp on the entries
There was a problem hiding this comment.
Done in b4994b9 — entries now carry _createdAt (ISO string from MemoryRecordSummary.createdAt) alongside the other underscored metadata keys.
| if (storeConfig.description !== undefined) this.description = storeConfig.description | ||
| if (storeConfig.maxSearchResults !== undefined) this.maxSearchResults = storeConfig.maxSearchResults | ||
| this.writable = storeConfig.writable | ||
| if (storeConfig.writable && storeConfig.extraction !== undefined) this.extraction = storeConfig.extraction |
There was a problem hiding this comment.
Consider logging a warning here, so extraction is not dropped silently if writable is false
There was a problem hiding this comment.
Done in b4994b9 — the store constructor now logs a warning when an extraction config is present but writable is false, so it isn't silently dropped. Added tests for both the warn and the no-warn (recall-only) cases.
| return joined.length > 0 ? joined : undefined | ||
| } | ||
|
|
||
| function ensureUniqueNames(stores: AgentCoreMemoryStore[]): void { |
There was a problem hiding this comment.
You can defer this check to the memory manager: https://github.com/strands-agents/harness-sdk/blob/main/strands-ts/src/memory/memory-manager.ts#L110
There was a problem hiding this comment.
Done in b4994b9 — removed the factory's ensureUniqueNames check and deferred to the MemoryManager constructor, which already throws on duplicate store names. Dropped the now-redundant factory test.
|
I think we should have this under |
| context.agent.addHook(this.messageAddedEvent, (event: unknown) => { | ||
| n++ | ||
| if (maxBytes !== undefined && hasMessage(event)) { | ||
| bytes += extractText(event.message).length |
There was a problem hiding this comment.
.length here doesn't return bytes, you might want something like this Buffer.byteLength(text, 'utf8')
There was a problem hiding this comment.
Fixed in b4994b9 — switched to Buffer.byteLength(text, 'utf8') so the maxBytes cap reflects the actual UTF-8 byte count AgentCore receives, not the JS string length.
…l peer @strands-agents/sdk declares an optional peer on @opentelemetry/* 2.x while this package pins OpenTelemetry 1.30.x. The mismatch is on an optional peer (no runtime impact), but npm's strict resolution rejects it on a clean install — which broke CI's `npm run clean && npm install`. A committed .npmrc applies legacy-peer-deps to every install path (local, `npm ci`, CI) with no manual flag. Regenerated the lockfile under the flag; `files` is `["dist"]`, so .npmrc is not published to consumers.
Add tests_integ/memory.test.ts covering the AgentCore Memory <-> Strands store
against the real data plane:
- store-level: create a throwaway memory (semantic strategy) -> addMessages via
createEvent -> clientToken idempotency on re-send -> recall via
retrieveMemoryRecords (polled, since extraction is async) -> subtree read ->
delete in afterAll
- E2E: a real MemoryManager + Agent writes a turn, flushes, and recalls the
extracted fact through search_memory
Both suites create and tear down their own resources (verified no orphans).
Also tighten the unit-test message fixtures to the real ToolUseBlockData /
ToolResultBlockData shapes now that the types come from @strands-agents/sdk
(the old vendored mirror allowed bare `{ toolUse: {} }`), and document how to
run the suites in docs/MEMORY.md.
…pace contract
Relocation (addresses jona62's PR comment): move src/memory/strands ->
src/memory/integrations/strands to match the Python SDK and the sibling
browser/code-interpreter Strands integrations (both live under integrations/).
The public export subpath stays `experimental/memory/strands` (consistent with
the siblings); only the dist target moves.
Namespace contract hardening — the most common silent-failure surface, verified
against the live service (records are stored under the fully-resolved concrete
path; retrieve matches your query as a prefix and rejects "{"/"}"):
- resolveNamespace now has assertResolvedNamespace: if any placeholder survives
substitution (only {actorId}/{sessionId} are resolved client-side), the store
throws at construction with a clear message instead of letting the service
reject the brace string later. {memoryStrategyId}-convention templates fail
loudly, not silently.
- validate non-empty memoryId/actorId/sessionId/namespace, minScore in [0,1],
and maxSearchResults >= 1 — all previously silent misconfigurations.
- correct the stale doc claim that the service resolves placeholders on read.
Tests:
- 19 negative-space unit tests (unresolved placeholder, empty identity, invalid
minScore/maxSearchResults, subtree commonParent over placeholder templates).
- live integ: strengthen the recall test to assert records land under exactly
the {actorId}-resolved namespace the store queried (the contract), and add a
session-scoped drift test proving a {sessionId} namespace is per-session
(session B sees none of session A's records). Full integ suite 7/7 green, no
orphaned resources.
Docs: add a "namespace contract" section to README + docs/MEMORY.md explaining
provision-vs-query matching, the {actorId}/{sessionId}-only rule, and per-session
scoping — the practical guidance for an AgentCore CLI user.
…ed metadata prefix
Two low-risk refinements from the PR decision audit (both reviewers endorsed):
- AgentCoreNamespaceConfig/StoreConfig gain an optional `overFetchFactor`
(default 4, validated >= 1), so the minScore over-fetch multiplier is tunable
per store instead of hardcoded. AgentCore's retrieve API has no server-side
relevance threshold, so the floor stays client-side; this just controls how
wide we fetch before trimming.
- Export `RESERVED_METADATA_PREFIX` ('_') and derive the result-metadata keys
(_id/_score/_namespaces/_createdAt) from it, so the reserved-key contract is a
single named constant consumers can reference rather than magic underscores.
Both documented in the module README. 6 new unit tests (overFetchTopK honored,
ignored without minScore, validation, prefix coupling). Unit suite 489 green.
The store previously sent one CreateEvent per message, so a session's write API-call volume grew linearly with messages and the extraction trigger only changed *when* calls happened, not how many. CreateEvent.payload is an array (verified the service accepts >=100 turns per event, and a multi-turn event extracts the same records as N single-turn events), so: - sendBatch now packs a flush's user/assistant turns into a single CreateEvent (matching the Python reference), chunked at maxTurnsPerEvent (configurable, default 50). Turns are split into a new event only where per-message metadata changes, since event metadata is per-event. - This makes the extraction trigger a real cost lever: API calls = flushes x ceil(turns / maxTurnsPerEvent). - clientToken is now one token per event spanning its [firstSeq,lastSeq], still anchored on the run id; isolated in tokenForSeqs so it can move to a stable per-message id when the framework exposes one. - On any event failure, throw AggregateError so the coordinator re-fires (one duplicate event on re-fire, collapsed by consolidation — not N). Tests: rewrote sender unit tests for the batched model (single-event packing, ceil chunking, per-event seq-range token, metadata grouping, failure); added a live integ test asserting a 4-turn write is ONE CreateEvent and still extracts. Unit suite 494 green; batching integ green; no orphaned resources. Docs updated to describe batching as the cost-control mechanism.
…/cadence/flush) No logic change — clarifies how to reduce createEvent volume and corrects the durability story: - factory.ts: the `cadence` JSDoc now states that a trigger only *dispatches* a write (fire-and-forget, never awaited); IntervalTrigger loses the tail at true end-of-conversation and AgentCoreBatchTrigger's maxDelayMs only narrows that window — MemoryManager.flush() is the durability mechanism, not the timer. - README + docs/MEMORY.md: add a "Reducing write API calls" section explaining the three independent levers — payload batching (always on, the main cost reduction), cadence (the trigger, batches across turns only if you reuse the manager and don't flush every turn), and flush() (durability, not cost). Shows the durable-default vs cost-tuned-advanced setups and that the app (not the SDK) owns per-session MemoryManager reuse. extraction:true stays the Strands default and the `cadence` field name is kept (both opieter's calls, approved).
No logic change. The default extraction:true is Strands' IntervalTrigger (every 5 turns, not every invocation; an empty fire is a no-op). Strands triggers are turn-gated, so AgentCoreBatchTrigger's only unique capability is non-turn-gated cadence (wall-clock maxDelayMs / byte maxBytes). Make the JSDoc + README + docs/MEMORY.md say plainly: it is optional, not the cost lever (payload batching is), largely moot under the per-turn-flush pattern, and earns its keep mainly on a long-lived server that flushes less often.
….crypto AgentCoreEventSender derived the default runId via globalThis.crypto.randomUUID(). globalThis.crypto only exists as a Node global in Node 19+ (gated in 18), so in environments without it the sender threw "Cannot read properties of undefined (reading 'randomUUID')" at construction — crashing every writable store. Import randomUUID from 'crypto' (the module), matching runtime/client.ts and app.ts, so it works regardless of Node version/runtime.
Two bugs surfaced by the bug bash of the Strands integration:
- search() could pass a non-integer topK to RetrieveMemoryRecords when a
fractional overFetchFactor was configured (e.g. 1.5 * want=5 -> 7.5).
The service requires an integer; ceil the product so over-fetch always
requests a whole page.
- createEvent metadata values are restricted server-side to
[a-zA-Z0-9 ._:/=+@-]; a value outside it (punctuation, or a stringified
array/object containing []{}",) failed with an opaque ValidationException.
Validate client-side BEFORE the allSettled fan-out and throw a clear error
naming the offending key, mirroring the namespace placeholder check. Doing
it up-front means the error is unwrapped (not re-wrapped as AggregateError)
and no createEvent is attempted when metadata is invalid.
Tests + MetadataProvider JSDoc updated accordingly.
… assertWritableTopology
Address jariy17's review: make AgentCoreMemoryStore a self-contained primitive
and shrink the factory to the one job only it can do (enforce write topology).
- Store reads a { namespace } | { namespacePath } discriminated union instead
of a namespace string + readMode flag — mirrors the RetrieveMemoryRecords API
1:1, so the field name is the read mode (no flag to desync).
- Store takes flat identity ({ memoryId, actorId, sessionId, ... }) OR the
shared config bundle, so 'new AgentCoreMemoryStore({...})' stands alone
without the factory. It self-names from the namespace (slugifyNamespace moved
in) and defaults writable to false (recall-safe; writes are an explicit opt-in).
- Writer selection moves to a per-namespace 'writable: true' flag (replacing
extraction.namespace string-matching); the writer defaults to the first
namespace when extraction is enabled and none is flagged.
- Add + export assertWritableTopology: at most one writable store (createEvent
is namespace-free, so two writers duplicate events and nothing upstream
dedupes them), exactly one when extraction runs. Exported so hand-built
multi-store setups can guard themselves.
- Keep commonParent + parentNamespace (explicit parent already works; inference
is the zero-config fallback) and the singular createAgentCoreMemoryStore as a
thin shim over the now-flat constructor (opieter's named single-store path).
Tests + README + docs/MEMORY.md migrated; integ adds a standalone-constructor
case. Full unit suite green (495).
…eam in Strands) Per team decision: the trigger did nothing the built-in Strands triggers can't, and any genuinely new cadence (wall-clock / byte-size) belongs upstream in the Strands repo rather than an AgentCore-specific package. It is unreleased, so this removes a default offering, not a customer-facing capability — the 'cadence' field stays (typed as the generic ExtractionTrigger) and accepts any Strands trigger (IntervalTrigger / InvocationTrigger / a future generic batch trigger). Add it back here only if a concrete customer need arises and upstream hasn't. Resolves jariy17's split-into-three comment: with the compound trigger gone, there is nothing to split.
Per TJ's review: the store constructor took identity either flat OR as a nested `config` bundle, with a resolveIdentity normalizer. The bundle was a hedge from when AgentCoreMemoryConfig looked load-bearing for the factory's shared-client reuse — but it isn't: the factory still builds the client once and now just spreads that identity flat into each store instead of nesting it. So the dual shape bought nothing and cost a second construction path for users to learn. - Store config is now flat-only: identity (memoryId/actorId/sessionId required, client/region/credentials/metadataProvider/maxTurnsPerEvent optional) at the top level, intersected with the read-target union + store fields. - Drop the `config?` bundle field, the Partial<AgentCoreMemoryConfig> dual shape, and the resolveIdentity helper. - Factory's buildStoreConfig spreads the shared identity flat (...config) rather than nesting it; the build-client-once optimization is unchanged. One obvious way to construct a store; no behavior or capability change. Net -34 lines. Full unit suite green (495).
…or is the single-store path
The singular factory was a pure createAgentCoreMemoryStores({namespaces:[ns]})[0]
alias — it adds no capability the now-flat constructor doesn't already provide.
Cross-SDK precedent and design guidance favor removing it:
- Creating one store maps exactly to constructing one instance, so per the .NET
design guidelines it should be the constructor, not a factory alias (factories
are for operations that don't map to 'construct one instance').
- The framework's own vended sibling, BedrockKnowledgeBaseStore (@strands-agents/sdk
v1.6.0), ships a constructor-only single-store path with no factory — the directly
comparable SDK does not offer a singular factory.
- No widely-used TS SDK (AWS SDK v3, viem, React) ships a singular+plural pair of
the same construction factory.
Single store -> new AgentCoreMemoryStore({ memoryId, actorId, sessionId, namespace, ... })
Multiple -> createAgentCoreMemoryStores({ ...namespaces }) (earns its slot: shared
client + assertWritableTopology single-writer enforcement)
Removes the function + CreateAgentCoreMemoryStoreInput + the two re-exports + the
singular-only unit tests; migrates the integ call sites to the constructor; docs now
lead with the constructor as the single-store path. assertWritableTopology stays
exported. Pre-GA, so dropping the experimental symbol now avoids a later breaking
change. No behavior change to the kept paths; unit suite green (492).
Reconcile the lockfile against the current main dependency tree (rebased onto 67fbfb5). Net removal of stale OpenTelemetry/protobufjs entries; no functional dependency change for the memory module.
aidandaly24
force-pushed
the
feat/agentcore-memory-store
branch
from
b9b65b8 to
9806ee6
Compare
aidandaly24
had a problem deploying
to
auto-approve
GitHub Actions
Failure
A code review found search() validated the constructor's maxSearchResults but not the per-call options.maxSearchResults. A value of 0 (reachable from the search_memory tool, whose schema is z.number().optional()) silently returned [] via topK=0 + slice(0,0); negative/non-integer values reached RetrieveMemoryRecords, which requires an integer. Mirror the constructor guard on the effective value. Also adds two regression tests the review flagged as uncovered: - topK over-fetch is capped at MAX_TOPK (100) — previously no test exercised the Math.min cap arm (max asserted topK was 30). - groupIntoEvents groups only CONSECUTIVE same-metadata runs (A,A,B,C,B -> 4 events) — guards against a future global-grouping refactor silently merging. Strands unit suite 100/100.
aidandaly24
had a problem deploying
to
auto-approve
GitHub Actions
Failure
…riter, integ)
A 28-scenario SDK bug bash (833 cases) surfaced one regression and several
silent-failure edge cases. Fixes:
- HIGH: integ tests built writer stores with extraction:true but no writable:true
after the singular->constructor migration (the removed factory derived writable
from extraction; the constructor does not), so CI test:integ failed. Add
writable:true to the four writer stores; make the recall-only namespace-contract
test actually recall-only.
- MED: toAgentCoreMetadata silently sent undefined (as {stringValue: undefined})
and NaN/Infinity/null (as the literal "null") past the charset guard, because
JSON.stringify is typed string but returns undefined/"null" at runtime. Reject
nullish/non-finite values up front with a clear keyed error.
- resolveNamespace used the identity value as a String.replace replacement string,
so $-sequences in an actorId silently corrupted the namespace; use replacer
functions (also single-pass, preventing cross-substitution).
- assertResolvedNamespace only caught matched {token} pairs; a lone/unmatched brace
survived into the query. Reject any stray brace, matching its documented contract.
- assertNonEmpty now rejects non-strings (null/number) with the clean named error
instead of an opaque TypeError; factory namespaces array hardened (Array.isArray,
null entries).
- Explicit writable:false on the first namespace is now honored (default writer
skips opted-out namespaces; throws if extraction is on but all opt out).
- extraction:false no longer emits a spurious 'extraction will not run' warning.
- Empty/whitespace store name falls back to the slug instead of a degenerate ''.
- maxTurnsPerEvent validated in the factory too (recall-only topology); extraction
object guarded against arrays; empty {} metadata bag omitted; AggregateError
message folds in the first underlying reason.
- extractText drops empty/blank text blocks before joining (mirrors upstream).
11 new unit tests; strands suite 111/111, full suite 607/607.
aidandaly24
had a problem deploying
to
auto-approve
GitHub Actions
Failure
…polling The memory integ tests poll live AgentCore for async server-side extraction (several minutes per record); the memory suite alone runs ~10 min, so the full integ suite tipped over the 10-minute step timeout — the tests PASSED (130/137, 7 skipped) but the step was killed after reporting success. Raise the budget to 25m for the full suite + extraction-latency variance. No other change; the run: step is unchanged (npm run test:integ), no untrusted input.
aidandaly24
had a problem deploying
to
auto-approve
GitHub Actions
Failure
…entNamespace Following up on the review thread: commonParent (longest-common-prefix inference of the subtree parent) was kept earlier as a zero-config fallback, but it earns almost nothing — subtree mode reads ONE parent path, so passing a list of child namespaces just to reverse-engineer their parent is clunkier than naming the parent directly (our own integ test already passes parentNamespace explicitly). It also mis-infers on a single namespace. Per the reviewer's original ask, delete it and require an explicit parentNamespace in subtree mode, throwing a clear error if absent. No public capability lost (parentNamespace already existed and was the documented path); net -28 lines. Tests + README updated. Strands suite 110/110.
aidandaly24
had a problem deploying
to
auto-approve
GitHub Actions
Failure
Remove comments that narrate what the code already says or editorialize (e.g. extractText's 'defensive flatten of whatever text remains', one-line restatements of assertNonEmpty and the placeholder regexes, the store/buildStoreConfig method- mapping bullets). Kept all load-bearing comments — the why-this-choice, external- constraint, and gotcha notes (namespace resolution contract, no-retry-here rationale, idempotency token, charset guard, exactOptionalPropertyTypes spreads). Comment-only change; no code touched. Net -29 comment lines; 110/110 unit tests.
aidandaly24
had a problem deploying
to
auto-approve
GitHub Actions
Failure
The earlier trim removed narration but left the bigger issue: the same 'why' explained 3-4x. The namespace-free / single-writer rationale lived in the writable-field doc, assertWritableTopology's doc, AND createAgentCoreMemoryStores' doc. Keep it once (in assertWritableTopology, the enforcer) and have the rest point there; collapse the cadence-vs-durability essay and the per-namespace/subtree narration. Comment-only; 100 -> 55 comment lines; 110/110 unit tests.
aidandaly24
had a problem deploying
to
auto-approve
GitHub Actions
Failure
The README + docs/MEMORY.md Requirements listed >= 3.1020 (the version where namespacePath first landed in the types), but package.json pins ^3.1065.0. Match the actual dependency. Title import path (experimental/memory/strands) and the >=1.5.0 Strands peer were already correct.
5 participants
Description
Adds an experimental AgentCore Memory integration for the Strands TypeScript SDK:
AgentCoreMemoryStore, aMemoryStorethat plugs into Strands'MemoryManager. Exposed via the package exportbedrock-agentcore/experimental/memory/strands.search()maps toretrieveMemoryRecords. The read target is a discriminated union:{ namespace }(exact prefix) or{ namespacePath }(subtree), mirroring the API directly. Optional client-sideminScorefloor with over-fetch, and reserved underscored result-metadata keys (_id/_score/_namespaces/_createdAt).addMessages()packs a turn's role-tagged messages into a singlecreateEvent(one API call carrying many turns, chunked atmaxTurnsPerEvent); AgentCore extracts and consolidates server-side (no client-side LLM pass). Idempotent via a run-anchoredclientTokenover the message sequence range.new AgentCoreMemoryStore({ memoryId, actorId, sessionId, namespace, writable?, extraction? })(self-names from the namespace;writabledefaults tofalse; creates its own client if none is passed). For multiple namespaces,createAgentCoreMemoryStores(...)builds one shared client and returns stores ready to spread intoMemoryManagerConfig.stores.createEventis namespace-free, so at most one store per(actorId, sessionId)may be writable. The writer is the namespace flaggedwritable: true(else the first when extraction is enabled);assertWritableTopologyenforces the at-most-one rule and is exported so hand-built multi-store setups can self-guard.extractionis the single write switch (boolean | { cadence?, filter? }): omit for recall-only,truefor the MemoryManager's default cadence, or pass any StrandsExtractionTrigger(single or array) for custom cadence. Write-cost reduction comes from payload batching (always on); cadence andMemoryManager.flush()control when/how durably writes land.Bumps
@aws-sdk/client-bedrock-agentcoreto^3.1065.0for the typednamespacePathfield, and depends on@strands-agents/sdk>=1.5.0for theMemoryManager/MemoryStore/ extraction interface.Related Issues
Implements the Strands memory extraction interface from
strands-agents/harness-sdk#2671 (now published in@strands-agents/sdk).Review history (this PR)
The implementation went through multiple review rounds and was hardened before this description was rewritten:
retrieveMemoryRecordsvia the{ namespace } | { namespacePath }union (dropped thereadModeflag), self-names, and stands alone with flat config; the singularcreateAgentCoreMemoryStorewas removed in favor of the constructor;assertWritableTopologyextracted + exported; the AgentCore-specific batch trigger was removed (generic cadence belongs upstream — thecadencefield accepts any Strands trigger).maxSearchResultsvalidation gap (a0from thesearch_memorytool silently returned[]); metadata values ofundefined/NaN/Infinity/nullslipping past the charset guard;$-sequence namespace corruption inresolveNamespace; an unmatched-brace gap in namespace validation; an opaqueTypeErroron non-string identity fields; a silently-overridden explicitwritable: false; and a spuriousextraction: falsewarning. All fixed with tests; core read/write/recall logic held across the bash.Documentation
Included in this PR:
docs/MEMORY.md(concept + deployment guide) andsrc/memory/integrations/strands/README.md(API reference). MainREADME.mdFeatures entry lists Memory as experimental →docs/MEMORY.md.Type of Change
New feature (experimental)
Testing
src/memory/integrations/strands/__tests__/(store search/topK/minScore + read-target mapping; sender batching/chunking/clientToken/metadata-charset/grouping/AggregateError; factory writer-selection +assertWritableTopology+ validation; namespace resolution +$-safety + brace/placeholder rejection; format helpers). Full unit suite green.tests_integ/memory.test.tsagainst real AgentCore Memory (deploy account): write → server-side extraction → recall round-trip, idempotency, multi-turn batching = onecreateEvent, the namespace contract (records land under the queried{actorId}path),{sessionId}per-session isolation, the standalone directly-constructed store, and a fullMemoryManager+AgentE2E. All passing in CI (the memory suite polls live extraction, which is slow — the integ step timeout was raised to 25m to accommodate it).tsc --noEmit, ESLint, and Prettier clean on the module.Checklist
@strands-agents/sdkmemory interface is published and imported directly)By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.