fix: improve devx for instantiating multiple bedrock KBs#2722
Conversation
d3629fa to
74a2f8d
Compare
74a2f8d to
10c461f
Compare
|
Assessment: Comment Clean, well-motivated refactor: the nested Review Categories
Nice work keeping the diff focused on the constructor reshape while preserving every behavior assertion. |
notowen333
left a comment
There was a problem hiding this comment.
Maybe in the same PR we can add some nuance to where nested constructor configs is valid. Can also follow up with it.
@notowen333 do you mean adding a code example like in the description to a docstring? |
Description
Running one Bedrock Knowledge Base across multiple namespaces is a first-class use case (per-user, per-team, per-org memory partitions backed by the same KB). With the constructor shipped in #2630, doing so meant retyping the entire connection config for every namespace, with only
scopediffering:This PR restructures the constructor into a single nested shape that cleanly separates the reusable connection from the per-store identity. Build the connection once, then spin up one store per namespace:
This is a DX-only change — search/ingest/validation behavior is unchanged. It only restructures the constructor input.
Key design decisions (reviewer-relevant):
shared-config merge were both considered and rejected: the store should have no notion of "sharing," and a configured store is not the place to encode a caller-side reuse pattern. Reusingconfigacross stores is just the caller passing the same object.knowledgeBaseId,dataSourceType,dataSourceId,s3,scopeMetadataKey,runtimeClient,agentClient) live inside the newBedrockKnowledgeBaseConfig; per-store identity/behavior (name,scope,description,writable,maxSearchResults,filter) sit beside it. No field is settable in two places, so there is no override-precedence rule or excess-property edge case to reason about.nameis required and independent ofscope. An earlier iteration defaultednametoscope; this was dropped in favor of keeping them distinct —nameis the routing identity surfaced to the model,scopeis the data partition. Requiringnamelets the store config simplyextends MemoryStoreConfig(which already providesname,description,maxSearchResults,writable) instead of carrying anOmit<>workaround, and removes a runtime guard.scope,scopeMetadataKey, andfilterarereadonly— to target a different namespace, construct another store from the sameconfigrather than mutating one in place.runtimeClient/agentClientintoconfigand every store built from it reuses them; omit them and each store lazily builds its own default, exactly as before.Related Issues
Builds on #2630 (adds
BedrockKnowledgeBaseStore) and should land after it. Follow-up to #2544.Documentation PR
No docs change in this PR. Note: the pre-existing examples in
designs/0011-memory-manager.mduse the original flat constructor form (and reference anextractionfield that does not exist on the store); they predate this change and #2630, and are left for a separate docs reconciliation.Type of Change
Breaking change
This restructures the public constructor of
BedrockKnowledgeBaseStore. Because the store is introduced by #2630 and is not yet onmain, this is a pre-release API change with no released consumers to migrate.Public API Changes
The constructor now takes a nested
configplus per-store fields, and a newBedrockKnowledgeBaseConfigtype is exported:nameis now required.scope,scopeMetadataKey, andfilterare exposed asreadonlyinstance fields — to target a different namespace, construct another store from the sameconfigrather than mutating one in place.Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
TypeScript-only change, verified with the strands-ts gates:
npm run build,npm run test(unit, including a@ts-expect-errorassertion thatnameis required and aconfig-reuse suite covering client reuse, per-store defaults, andMemoryManagerregistration/dedup),npm run type-check(src + integ),npm run lint,npm run format:check— all green. All existing unit tests and the 15 integ-test constructions were migrated to the nested shape with no behavior assertions lost.hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.