-
Notifications
You must be signed in to change notification settings - Fork 0
Let explicit selectors route unverifiable ARNs to BedrockInvokeModel #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -235,6 +235,44 @@ def model_double(id, metadata: {}, provider: 'bedrock') | |
| end | ||
| end | ||
|
|
||
| context 'with an unverifiable ARN id (no provider_name metadata)' do # rubocop:disable RSpec/MultipleMemoizedHelpers | ||
| before { allow(RubyLLM.logger).to receive(:warn) } | ||
|
|
||
| it 'routes to BedrockInvokeModel when an Array selector lists the ARN' do | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| provider = build_bedrock(use_invoke_model: [arn_id]) | ||
| expect(provider.protocol_for(model_double(arn_id))).to be(RubyLLM::Protocols::BedrockInvokeModel) | ||
| end | ||
|
|
||
| it 'routes to BedrockInvokeModel when a Proc selector returns true' do | ||
| provider = build_bedrock(use_invoke_model: ->(_m) { true }) | ||
| expect(provider.protocol_for(model_double(arn_id))).to be(RubyLLM::Protocols::BedrockInvokeModel) | ||
| end | ||
|
|
||
| it 'routes to Converse when a Proc selector returns false' do | ||
| provider = build_bedrock(use_invoke_model: ->(_m) { false }) | ||
| expect(provider.protocol_for(model_double(arn_id))).to be(RubyLLM::Protocols::Converse) | ||
| end | ||
|
|
||
| it 'still requires positive verification under the blanket true selector' do | ||
| provider = build_bedrock(use_invoke_model: true) | ||
| expect(provider.protocol_for(model_double(arn_id))).to be(RubyLLM::Protocols::Converse) | ||
| expect(RubyLLM.logger).to have_received(:warn).with(/cannot verify.*Anthropic-backed/) | ||
| end | ||
| end | ||
|
|
||
| context 'with provably non-Anthropic ids under explicit selectors' do # rubocop:disable RSpec/MultipleMemoizedHelpers | ||
| it 'never routes a bare vendor id, even when an Array selector lists it' do | ||
| provider = build_bedrock(use_invoke_model: [nova_id]) | ||
| expect(provider.protocol_for(model_double(nova_id))).to be(RubyLLM::Protocols::Converse) | ||
| end | ||
|
|
||
| it 'never routes a cross-region vendor id, even when a Proc selector returns true' do | ||
| provider = build_bedrock(use_invoke_model: ->(_m) { true }) | ||
| expect(provider.protocol_for(model_double(us_nova_id))).to be(RubyLLM::Protocols::Converse) | ||
| expect(provider.protocol_for(model_double(llama_id))).to be(RubyLLM::Protocols::Converse) | ||
| end | ||
| end | ||
|
|
||
| describe 'anthropic_model? directly' do # rubocop:disable RSpec/MultipleMemoizedHelpers | ||
| let(:bedrock) { build_bedrock } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the optional geo-prefix segment
[a-z0-9-]+inNON_ANTHROPIC_PATTERNis unboundedly wide — it would treat any single-dot-prefixed string (e.g. a hypothetical"foo.amazon.x") as a cross-region non-Anthropic id. The existing cross-region geo codes are all short (us,eu,apac,global,jp,au,us-gov), so a tighter bound like[a-z]{2,6}(?:-[a-z]+)?would make the intent clearer and prevent surprise matches on longer prefixes. Low severity since real Bedrock model IDs don't look like that today.