Skip to content

feat(memory): support managed Bedrock knowledge bases in retrieve and ACL#2909

Merged
notowen333 merged 12 commits into
strands-agents:mainfrom
notowen333:worktree-bedrock-kb-managed
Jun 24, 2026
Merged

feat(memory): support managed Bedrock knowledge bases in retrieve and ACL#2909
notowen333 merged 12 commits into
strands-agents:mainfrom
notowen333:worktree-bedrock-kb-managed

Conversation

@notowen333

@notowen333 notowen333 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Description

Amazon Bedrock recently launched managed knowledge bases, where Bedrock owns the ingestion, indexing, and storage infrastructure. They are queried differently from the self-managed (vector) knowledge bases this store was built for: Retrieve takes a managedSearchConfiguration rather than a vectorSearchConfiguration. Until now BedrockKnowledgeBaseStore always sent vectorSearchConfiguration, so search() against a managed knowledge base was malformed.

This change makes the store work with all knowledge base types. On agent startup (or first search() for standalone usage), it resolves the knowledge base type via GetKnowledgeBase, caches the result, and wraps the retrieval config under the matching key (managedSearchConfiguration for MANAGED, vectorSearchConfiguration for everything else). Detection is eager: MemoryManager.initAgent now calls initialize() on each store, so a missing bedrock:GetKnowledgeBase permission fails fast at construction rather than silently mis-routing searches at runtime. A new knowledge_base_type / knowledgeBaseType config option lets callers skip the GetKnowledgeBase call entirely when the type is known ahead of time.

Additionally, managed knowledge bases commonly have ACL awareness enabled on their data sources. An ACL-enabled data source rejects ingestion without a per-document access control list. This change adds an optional access_control_list / accessControlList param so users of ACL-enabled data sources can write memories. If the param is absent and Bedrock rejects with an ACL error, the store rewrites the exception to point at the new field.

Both features are mirrored across the Python and TypeScript SDKs.

Public API additions:

  • access_control_list / accessControlList on BedrockKnowledgeBaseStoreConfig (optional, affects writes only)
  • BedrockKnowledgeBaseAccessControlEntry type
  • knowledge_base_type / knowledgeBaseType on BedrockKnowledgeBaseConfig (optional, skips GetKnowledgeBase)
  • initialize() method on the MemoryStore protocol/interface (optional, called by MemoryManager)
  • MemoryManager.init_agent / initAgent is now async (calls initialize() on stores)

No breaking changes to the end-user API. The init_agent/initAgent async change is internal to the plugin lifecycle (Plugin.initAgent); agent construction already awaits plugin initialization. Stores without initialize() are unaffected. The GetKnowledgeBase auto-detection is internal, and both new config params are optional (stores without them work exactly as before).

Related Issues

Type of Change

New feature

Testing

Unit tests: Both SDKs — managed type detection (routing, memoization, all KB types), eager initialization via MemoryManager, ACL on CUSTOM (inline metadata, ACL-only guard, error rewrite), ACL on S3 (sidecar capitalization, ACL-only sidecar, combined scope+ACL).

Live validation: Ran against a real managed knowledge base (WFQFKU3TIR, example-managed-kb, us-east-1) with ACL enabled:

  • No-ACL write → store rewrites the error to name access_control_list
  • ACL-only (no scope) → store raises before calling Bedrock ✅
  • ACL + scope → successful ingest, doc id returned ✅
  • Search → _kb_type detected as MANAGED, managedSearchConfiguration sent, no error ✅

No CDK/CI integ coverage — CloudFormation does not yet expose managedKnowledgeBaseConfiguration, so the test-infra stack can't provision a managed KB. Follow-up when CFN support lands.

  • I ran hatch run prepare

Checklist

  • I have read the CONTRIBUTING document
  • I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
  • My change is focused and reasonably small; I have split unrelated work into separate PRs
  • 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

@github-actions github-actions Bot added size/m area-model Related to models or model providers area-tool Tool behavior/api enhancement New feature or request python Pull requests that update python code labels Jun 22, 2026
@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.42857% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...nded_memory_stores/bedrock_knowledge_base/store.py 95.34% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Comment thread strands-py/src/strands/vended_memory_stores/bedrock_knowledge_base/store.py Outdated
Comment thread strands-py/src/strands/vended_memory_stores/bedrock_knowledge_base/store.py Outdated
Comment thread strands-py/src/strands/vended_memory_stores/bedrock_knowledge_base/store.py Outdated
@github-actions github-actions Bot added size/l and removed size/m labels Jun 23, 2026
@notowen333 notowen333 changed the title feat(memory): support managed Bedrock knowledge bases in retrieve feat(memory): support managed Bedrock knowledge bases in retrieve and ACL Jun 23, 2026
Comment thread site/src/content/docs/user-guide/concepts/memory/bedrock-knowledge-base.mdx Outdated
Comment thread site/src/content/docs/user-guide/concepts/memory/bedrock-knowledge-base.mdx Outdated
@notowen333 notowen333 marked this pull request as ready for review June 23, 2026 21:03
Comment thread site/src/content/docs/user-guide/concepts/memory/bedrock-knowledge-base.mdx Outdated
Comment thread site/src/content/docs/user-guide/concepts/memory/bedrock-knowledge-base.mdx Outdated
@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Documentation Preview Ready

Your documentation preview has been successfully deployed!

Changed pages:

Updated at: 2026-06-24T22:18:15.800Z

opieter-aws
opieter-aws previously approved these changes Jun 24, 2026
Add an optional access_control_list / accessControlList param to
BedrockKnowledgeBaseStoreConfig, stamped on every write. Required by
ACL-enabled data sources on managed knowledge bases. For CUSTOM data
sources the entries are passed through verbatim (lowercase fields). For
S3 data sources they are translated to the sidecar's capitalized field
names (Name/Type/Access).

When a write fails because the data source requires an ACL and none is
configured, the store rewrites the ValidationException into a
ValueError/Error pointing at the new param. An inline ACL on the CUSTOM
path requires at least one metadata attribute (scope or caller metadata);
the store raises early with guidance rather than letting the API reject
it with a cryptic error.

Validated end-to-end against a live managed knowledge base with ACL
enabled: no-ACL error rewrite, ACL-only guard, and ACL+scope successful
ingest all confirmed.
The managed knowledge base types (KnowledgeBaseRetrievalConfiguration
with managedSearchConfiguration, DocumentAccessControlEntry) were added
in SDK build 3.1075.0. Restore the typed imports that were removed when
the older SDK couldn't resolve them.
Previously a failed GetKnowledgeBase call was not cached, causing a
repeated failing control-plane call on every search(). Cache the VECTOR
fallback so subsequent searches skip the redundant network round-trip.

Also tighten the ACL test assertion to check the full metadata shape.
Replace the lazy fallback-to-VECTOR detection with an eager initialize()
method on MemoryStore. MemoryManager.initAgent calls it at agent startup so
a missing GetKnowledgeBase permission fails fast instead of silently
mis-routing searches.

- Add optional initialize() to the MemoryStore protocol/interface
- MemoryManager.init_agent / initAgent is now async; calls initialize()
- BedrockKnowledgeBaseStore.initialize() is idempotent and also called
  lazily on first search() for standalone usage
- Add knowledge_base_type / knowledgeBaseType config — skips GetKnowledgeBase
- Support all KB types (MANAGED, VECTOR, KENDRA, SQL)
- Remove _logging.py (warn_once no longer needed)
- Tighten test assertions to full metadata shape
- Update docs with new config param and IAM note
opieter-aws
opieter-aws previously approved these changes Jun 24, 2026
Comment thread strands-py/src/strands/vended_memory_stores/bedrock_knowledge_base/store.py Outdated
Comment thread strands-py/src/strands/vended_memory_stores/bedrock_knowledge_base/store.py Outdated
Comment thread strands-ts/src/vended-memory-stores/bedrock-knowledge-base/store.ts Outdated
Comment thread strands-py/src/strands/vended_memory_stores/bedrock_knowledge_base/types.py Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment

The eager-initialize() refactor is a solid response to the previous round's "silent mis-routing" concern — failing fast on a GetKnowledgeBase error at agent construction is the right call, and the Python tests now assert full object shapes. A few things didn't fully catch up to the refactor, and there's one cross-SDK behavioral gap worth closing before merge.

Review themes
  • Docs/description drift from the new behavior: The class docstrings (store.py L75-77, store.ts L177-179) still say detection "falls back to the vector configuration," and the PR description still states detection "fails open... preserving behavior for existing stores" and "No breaking changes." After this refactor initialize() raises, and MemoryManager calls it eagerly — so a store that lacks bedrock:GetKnowledgeBase (or runs an older SDK) now fails at agent construction where it previously worked. That may well be the intended trade-off, but the docs/description should describe it accurately and call out the new IAM requirement as a potential breaking change for existing deployments.
  • Cross-SDK parity on a malformed detection response: TS coerces a missing type to undefined (silently routing as vector and defeating idempotency), while Python raises KeyError. Worth unifying on the fail-fast contract.
  • Diagnostics: initialize() is the only remote call that doesn't log store context before propagating; an eager-startup failure surfaces as a bare client error.
  • Testing: Python ACL tests now assert the full metadata shape; the TS counterparts still assert individual fields (carried over from last round's thread).
  • API review: Adds a public type and config fields. The PR description includes the API materials — just confirm whether the api/needs-review label/process applies.

Nicely scoped overall, and the ACL serialization across CUSTOM (inline) and S3 (capitalized sidecar) is clean and well-documented.

- Update stale class docstrings (Python + TS) to reflect eager fail-fast
  behavior instead of old silent-fallback language
- Add error logging in initialize() before re-raising, matching the pattern
  used by search() and add()
- Use non-null assertions in TS initialize() so a missing type throws
  instead of silently being undefined (parity with Python's KeyError)
- Type knowledge_base_type as Literal union in Python for parity with TS
Comment thread strands-ts/src/vended-memory-stores/bedrock-knowledge-base/store.ts
@github-actions

Copy link
Copy Markdown
Contributor

Issue (PR description): The description still states detection "fails open... preserving behavior for existing stores" and "No breaking changes." After the eager-initialize() refactor that's no longer accurate: initialize() now raises, and MemoryManager calls it at agent construction — so an existing deployment whose credentials lack bedrock:GetKnowledgeBase (and that doesn't set knowledge_base_type/knowledgeBaseType) will now fail at construction where it previously worked. The class docstrings and the .mdx IAM section already describe this correctly; only the PR description lags.

Suggestion: Update the description to describe the fail-fast behavior and call out the new bedrock:GetKnowledgeBase IAM requirement as a potential breaking change for existing stores (with the knowledge_base_type escape hatch). This is the intended trade-off — it just needs to be stated so reviewers and downstream users aren't surprised.

@github-actions

Copy link
Copy Markdown
Contributor

Assessment: Comment

This round closes most of the prior feedback — the class docstrings now describe the fail-fast detection accurately, the Python ACL tests assert full metadata shapes, and the removed warnOnce usage moots the earlier logging-parity thread. The async init_agent/initAgent change is safe: both plugin registries already await it (Python via call_init_methodrun_async, TS via await plugin.initAgent). Tests pass locally (70 KB-store + 102 memory-manager in Python; 70 KB-store in TS). A few items remain before merge, none blocking on their own.

Review themes
  • Missing coverage for the headline behavior: nothing asserts MemoryManager actually calls initialize() on stores — the eager fail-fast guarantee this PR is built around. A dropped await would pass the suite.
  • Cross-SDK parity on a malformed detection response: TS coerces a missing type to undefined (silent vector routing + defeated memoization), while Python raises. Worth unifying on the fail-fast contract, with a test on both sides.
  • Testing style: the remaining TS ACL assertions check individual fields where the Python equivalents (and TESTING.md) use full-object toStrictEqual.
  • Description drift: the "No breaking changes / fails open" wording predates the eager-init refactor; the new bedrock:GetKnowledgeBase requirement is a potential breaking change for existing stores and should be called out (docstrings/.mdx already do).
  • API review: adds a public type, two config params, and initialize() on the MemoryStore protocol — a moderate change per API_BAR_RAISING.md. The PR description includes the required API materials; just confirm whether the api/needs-review label/process applies.

The ACL serialization across CUSTOM (inline, lowercase) and S3 (capitalized sidecar) is clean and well-documented, and the live-validation notes against a real managed KB are a nice touch.

@notowen333 notowen333 merged commit 73a2a46 into strands-agents:main Jun 24, 2026
43 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-model Related to models or model providers area-tool Tool behavior/api enhancement New feature or request python Pull requests that update python code size/l

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants