feat: add support for TS memory#1636
Conversation
Package TarballHow to installgh release download pr-1636-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.21.0.tgz |
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for shipping this — the Strands TS memory wiring matches the Python pattern nicely. A few items need to be resolved before merge:
1. Asset snapshots are stale (will fail CI)
src/assets/__tests__/__snapshots__/assets.snapshot.test.ts.snap was not regenerated. Specifically:
- The
asset-file-listingsnapshot (around line 845) does not include the newtypescript/http/strands/capabilities/memory/memory.tsentry. - The snapshot for
typescript/http/strands/base/main.ts(line 7376) still shows the pre-PR contents (no memory branching, no agent cache). - The snapshot for
typescript/http/strands/base/package.json(line 7567) still pins@strands-agents/sdk: 1.0.0-rc.4andbedrock-agentcore: ^0.2.4.
Please run npm run test:update-snapshots and commit the regenerated file — the PR checklist box for snapshot regeneration is also unchecked.
2. package.json dependency versions
bedrock-agentcore: ^0.3.0 does not exist on npm yet (latest published is 0.2.4), and @strands-agents/sdk: ^1.5.0 is a caret range rather than a pin. You already called both of these out in the PR description; flagging here so it's not missed at merge time. Once the SDK ships, both should be pinned exactly (see inline comment).
3. Schema mapper guard removal creates a TS + VercelAI footgun
See inline comment on schema-mapper.ts — the CLI flag flow no longer rejects --language TypeScript --framework VercelAI --memory longAndShortTerm, and the schema mapper now lets it through.
4. getActorId can return undefined despite its string return type
See inline comment on memory.ts.
Points 1 and 2 will break CI / generated projects; 3 is a silent-cost footgun; 4 is a latent runtime bug for OAuth / CUSTOM_JWT callers. Happy to re-review once these are addressed.
| "@strands-agents/sdk": "1.0.0-rc.4", | ||
| "bedrock-agentcore": "^0.2.4", | ||
| "@strands-agents/sdk": "^1.5.0", | ||
| "bedrock-agentcore": "^0.3.0", |
There was a problem hiding this comment.
bedrock-agentcore@^0.3.0 is not on npm yet (latest published is 0.2.4), and the import bedrock-agentcore/experimental/memory/strands used in memory.ts ships in that unreleased version. Until 0.3.x is published, every newly generated TS Strands project (memory or not) will fail npm install. Per the PR description this PR shouldn't merge until the SDK is released — when it is, please also pin exact versions (drop the carets on both bedrock-agentcore and @strands-agents/sdk) per the note in the PR description.
There was a problem hiding this comment.
This will be updated in PR once new SDK version is released.
| isMcp || config.language === 'TypeScript' | ||
| ? [] | ||
| : mapMemoryOptionToMemoryProviders(config.memory, config.projectName), | ||
| memoryProviders: isMcp ? [] : mapMemoryOptionToMemoryProviders(config.memory, config.projectName), |
There was a problem hiding this comment.
Removing the config.language === 'TypeScript' guard here (and on line 279) is correct for TS+Strands, but it also lets TS+VercelAI propagate hasMemory=true and a populated memoryProviders. The TUI compensates by resetting memory to 'none' whenever the user picks a non-Strands SDK (useGenerateWizard.ts setSdk), but the CLI-flag flows do not — see src/cli/commands/add/validate.ts and src/cli/commands/create/validate.ts. Running agentcore add agent --language TypeScript --framework VercelAI --memory longAndShortTerm ... (or the equivalent agentcore create) would now provision a Memory resource and inject a MEMORY_*_ID env var that the VercelAI template never reads — silently costing the user money.
Options to fix:
- Tighten the guard here to e.g.
(isMcp || (config.language === 'TypeScript' && config.sdk !== 'Strands'))so only the Strands TS template gets memory. - Or, add a validation rule in
validateAddAgentOptions/validateCreateOptionsrejectinglanguage=TypeScript && framework!=Strands && memory!='none'.
Either is fine — just please don't leave the CLI-flag path unguarded.
| context?.headers?.[CUSTOM_ACTOR_ID_HEADER] ?? | ||
| payload?.userId ?? | ||
| context?.sessionId | ||
| ); |
There was a problem hiding this comment.
Return type is string but this can resolve to undefined when there's no custom-actor header, no payload.userId, and context.sessionId is also missing — exactly the OAuth / CUSTOM_JWT scenario the Python session.py template explicitly handles with a synthesized session id:
# AgentCoreMemoryConfig rejects None; OAuth/CUSTOM_JWT callers can reach us
# without a runtime session header, so synthesize one when absent.
session_id = session_id or uuid.uuid4().hexIf actorId is undefined when createAgentCoreMemoryStores is called, the cache key becomes "undefined:default-session" and the SDK call will either throw or write events under a literal 'undefined' actor.
Options:
- Synthesize a fallback (e.g.
crypto.randomUUID()) when all three sources are missing, matching the Python template. - Add a final
?? 'default-actor'literal. - Change the return type to
string | undefinedand handle that at the call site inmain.ts.
Minor: also worth aligning the fallback chain with Python — Python uses payload.user_id (snake_case) and context.user_id; this template uses payload.userId (camelCase) and context.sessionId. If that divergence is intentional, a comment explaining why would help.
e986e75 to
cbcedb2
Compare
|
Claude Security Review: no high-confidence findings. (run) |
cbcedb2 to
ab5be2c
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Coverage Report
|
Description
Wires up memory for typescript strands agent template. When a user picks a memory option during
agentcore createfor a TS Strands agent, the generated project now ships ready-to-run with the new Strands memory integration from the (SDK PR #187). Same devEx as we support for python strands path.Changes:
agentcore createnow shows the memory step for typescript strandshasMemory/memoryProviderssame as pythoncapabilities/memory/memory.ts— wires upMemoryManagerwith strands integrationscreateAgentCoreMemoryStoresTested e2e: Built locally against the SDK PR's branch, deployed a TS strands + memory project, and invoked with a memory to remember. Upon next invocation, memory was recalled as expected.
NOTE: This change is dependent on:
await import('@aws-sdk/client-s3')was unable to resolve given the previous bundler. This CDK fix is required for TS strands memory, otherwiseagentcore deployfails on all TS strands projects.Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.