Skip to content
This repository was archived by the owner on Jun 3, 2026. It is now read-only.

fix: warn on legacy Bedrock cache options #1055

Open
asim48-ctrl wants to merge 1 commit into
base: main
Choose a base branch
from
+25 −0
Open

fix: warn on legacy Bedrock cache options #1055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions strands-ts/src/models/__tests__/bedrock.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,18 @@ describe('BedrockModel', () => {
temperature: 0.7,
})
})

it('warns when unsupported legacy cache options are provided', () => {
new BedrockModel({
cachePrompt: 'default',
cacheTools: 'default',
} as any)

expect(warnOnce).toHaveBeenCalledWith(
expect.anything(),
'unsupported_fields=<cachePrompt,cacheTools> | cachePrompt and cacheTools are not supported by BedrockModel, use cacheConfig instead'
)
})
})

describe('format_message', async () => {
Expand Down
13 changes: 13 additions & 0 deletions strands-ts/src/models/bedrock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ const BEDROCK_CONTEXT_WINDOW_OVERFLOW_MESSAGES = [
*/
const SKIP_COUNT_TOKENS_MODELS = new Set<string>()

type LegacyBedrockCacheOptions = {
cachePrompt?: unknown
cacheTools?: unknown
}

/**
* Mapping of Bedrock stop reasons to SDK stop reasons.
*/
Expand Down Expand Up @@ -388,6 +393,14 @@ export class BedrockModel extends Model<BedrockModelConfig> {
constructor(options?: BedrockModelOptions) {
super()

const legacyCacheOptions = options as (BedrockModelOptions & LegacyBedrockCacheOptions) | undefined
if (legacyCacheOptions?.cachePrompt !== undefined || legacyCacheOptions?.cacheTools !== undefined) {
warnOnce(
logger,
'unsupported_fields=<cachePrompt,cacheTools> | cachePrompt and cacheTools are not supported by BedrockModel, use cacheConfig instead'

@notowen333 notowen333 May 22, 2026

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. 

  1. (Functional gap) updateConfig is not covered. The same legacy fields can
  land via updateConfig({ cachePrompt: 'default' } as any) (bedrock.ts:513) —
  that path silently drops them just like the constructor used to. If the goal
  is "users shouldn't believe caching is enabled when it isn't," updateConfig
  should warn too. Minor refactor: extract a warnIfLegacyCacheOptions(options)
  helper and call it in both places.

  2. (Test placement) The new test lives in the wrong describe block. Per the
  diff context (@@ -448,6 +448,18 @@), the it('warns when unsupported legacy
  cache options...') is inserted at the tail of describe('getConfig')
  (bedrock.test.ts:437), not describe('constructor') (line 183), where the
  analogous it('warns when modelId is not explicitly set') test already lives
  (line 190). Move it next to the other constructor-warning tests for
  discoverability.

  3. (Message accuracy) Warning text claims both fields when only one may be
  present. If a caller passes only cachePrompt, the warning still says
  unsupported_fields=<cachePrompt,cacheTools>. Either:
  - List only the keys actually present (more accurate, costs warnOnce dedup
  since the key set varies), or
  - Keep the static message but drop the redundancy (the structured prefix and
  prose say the same thing twice).

)
}

const { region, clientConfig, apiKey, ...modelConfig } = options ?? {}

// Initialize model config with default model ID if not provided
Expand Down
Loading