Skip to content

feat: add support for TS memory#1636

Merged
aidandaly24 merged 4 commits into
mainfrom
feat/ts-memory-support
Jun 25, 2026
Merged

feat: add support for TS memory#1636
aidandaly24 merged 4 commits into
mainfrom
feat/ts-memory-support

Conversation

@nborges-aws

Copy link
Copy Markdown
Contributor

Description

Wires up memory for typescript strands agent template. When a user picks a memory option during agentcore create for 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:

  • TUI: agentcore create now shows the memory step for typescript strands
  • Schema mapper: TS Strands now flows through hasMemory / memoryProviders same as python
  • TS Strands template:
    • New capabilities/memory/memory.ts — wires up MemoryManager with strands integrations createAgentCoreMemoryStores
  • Tests: existing TS-skips-memory test flipped to assert the new behavior (previous asserted memory not included)

Tested 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:

  1. SDK PR #187. The template should be pinned to latest SDK version once this PR is merged & released. This PR should be updated with new pinned version before merge.
  2. CDK PR #292. Strands SDK's dynamic await import('@aws-sdk/client-s3') was unable to resolve given the previous bundler. This CDK fix is required for TS strands memory, otherwise agentcore deploy fails on all TS strands projects.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Other (please describe):

Testing

How have you tested the change?

  • I ran npm run test:unit and npm run test:integ
  • I ran npm run typecheck
  • I ran npm run lint
  • If I modified src/assets/, I ran npm run test:update-snapshots and committed the updated snapshots

Checklist

  • I have read the CONTRIBUTING document
  • I have added any necessary tests that prove my fix is effective or my feature works
  • I have updated the documentation accordingly
  • I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.

@nborges-aws nborges-aws requested a review from a team June 24, 2026 19:35
@github-actions github-actions Bot added the size/m PR size: M label Jun 24, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 24, 2026
@github-actions github-actions Bot added the agentcore-harness-reviewing AgentCore Harness review in progress label Jun 24, 2026
@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Package Tarball

aws-agentcore-0.21.0.tgz

How to install

gh 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

@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 24, 2026

@agentcore-cli-automation agentcore-cli-automation left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-listing snapshot (around line 845) does not include the new typescript/http/strands/capabilities/memory/memory.ts entry.
  • 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.4 and bedrock-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",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / validateCreateOptions rejecting language=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
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().hex

If 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 | undefined and handle that at the call site in main.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.

@github-actions github-actions Bot removed the agentcore-harness-reviewing AgentCore Harness review in progress label Jun 24, 2026
@nborges-aws nborges-aws force-pushed the feat/ts-memory-support branch from e986e75 to cbcedb2 Compare June 25, 2026 13:25
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Jun 25, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 25, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 25, 2026
@nborges-aws nborges-aws force-pushed the feat/ts-memory-support branch from cbcedb2 to ab5be2c Compare June 25, 2026 13:35
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Jun 25, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 25, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 25, 2026
@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 37.42% 13774 / 36805
🔵 Statements 36.69% 14649 / 39916
🔵 Functions 31.99% 2356 / 7363
🔵 Branches 31.46% 9183 / 29183
Generated in workflow #3846 for commit 3da448b by the Vitest Coverage Report Action

@aidandaly24 aidandaly24 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH-2 — multi-actor isolation can never work from the default scaffold

getActorId() reads x-amzn-bedrock-agentcore-runtime-custom-actor-id as its first source, and the comment says it's "declared in requestHeaderAllowlist" — but the create-with-memory flow never adds that header to the runtime's requestHeaderAllowlist (schema-mapper.ts:140 emits it only when the user supplies one, and there's no --request-header-allowlist flag on create). The managed runtime strips non-allowlisted headers, so the header is always absent and getActorId silently falls through to sessionId — distinct callers in one session collapse to a single actor (a within-session cross-user memory exposure risk).

Proven both ways on real deploys: with no allowlist, records landed under /users/<sessionId>/facts (the predicted collapse); after manually adding requestHeaderAllowlist: ['x-amzn-bedrock-agentcore-runtime-custom-actor-id'], list-actors returned two distinct actors and Bob correctly did not recall Alice's fact. So the mechanism works — only the scaffold's wiring is missing.

Suggested fixes (both small, both want a rendered-output regression test)

  1. HIGH-2: when a multi-actor-capable memory is wired, auto-append the actor-id header to the owning runtime's requestHeaderAllowlist (the schema + CDK already support the field — zero new CDK work). Optionally expose --request-header-allowlist on create. At minimum, fix the comment + add a README note so the promise matches reality.

Note: the existing assets.snapshot.test.ts snapshots the un-rendered handlebars template, which is why HIGH-1 shipped green. A test that renders the shortTerm variant and asserts a non-empty namespaces[] (or no factory call) would catch this class of bug.

Full bug-bash report (23 scenarios, ~203 cases, coverage matrix, positive findings) available on request. Happy to pair on the fixes.

Comment on lines +9 to +19
export function getActorId(payload: any, context: any): string {
// Multi-actor agents: caller passes user via the custom header (declared in
// requestHeaderAllowlist) or as a `userId` field in the invocation payload.
// Single-actor agents: actorId == sessionId is fine.
// RandomUUID fallback when all sources are absent
return (
context?.headers?.[CUSTOM_ACTOR_ID_HEADER] ??
payload?.userId ??
context?.sessionId ??
randomUUID()
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH — multi-actor isolation can't work from the default scaffold (in-scope)

This reads x-amzn-bedrock-agentcore-runtime-custom-actor-id as the first actor source, and the comment above says the header is "declared in requestHeaderAllowlist" — but the create-with-memory flow never declares it. schema-mapper.ts:140 emits requestHeaderAllowlist only ...(config.requestHeaderAllowlist?.length && {...}), i.e. only when the user explicitly supplies one, and agentcore create has no --request-header-allowlist flag (it exists only on add agent). So the default agentcore.json runtime has no allowlist, the synth template emits no RequestHeaderConfiguration, and the managed runtime strips the custom header → this lookup is always undefined and getActorId silently falls through to payload.userIdcontext.sessionId. Distinct callers in one session collapse to a single actor (within-session cross-user memory exposure).

Proven both ways on real deploys: no allowlist → records landed under /users/<sessionId>/facts (the collapse); after manually adding requestHeaderAllowlist: ['x-amzn-bedrock-agentcore-runtime-custom-actor-id'], list-actors returned two distinct actors and Bob did not recall Alice's fact. The mechanism is fine — only the wiring is missing.

Fix: when a multi-actor-capable memory (e.g. longAndShortTerm) is wired, auto-append the actor-id header to the owning runtime's requestHeaderAllowlist (the RequestHeaderAllowlistSchema + CDK RequestHeaderConfiguration mapping already support it — no CDK change). Optionally register --request-header-allowlist on create. At minimum, fix this comment and add a README "per-actor memory isolation" note so the scaffold doesn't promise behavior it doesn't deliver.

(Minor, same function: the ?? chain lets an empty-string header/userId through and the declared : string return can be violated since payload is any/users//facts. Coercing/trimming each candidate and using || after normalization would harden it.)

@nborges-aws nborges-aws force-pushed the feat/ts-memory-support branch from ab5be2c to 3da448b Compare June 25, 2026 19:26
@github-actions github-actions Bot added size/m PR size: M and removed size/m PR size: M labels Jun 25, 2026
@agentcore-devx-automation agentcore-devx-automation Bot added the claude-security-reviewing Claude Code /security-review in progress label Jun 25, 2026
@agentcore-devx-automation

Copy link
Copy Markdown
Contributor

Claude Security Review: no high-confidence findings. (run)

@agentcore-devx-automation agentcore-devx-automation Bot removed the claude-security-reviewing Claude Code /security-review in progress label Jun 25, 2026

@aidandaly24 aidandaly24 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving and merging then waiting for CI to pass

@aidandaly24 aidandaly24 merged commit 39a47fa into main Jun 25, 2026
75 of 77 checks passed
@aidandaly24 aidandaly24 deleted the feat/ts-memory-support branch June 25, 2026 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants