feat: add bedrock as a new data source#730
Open
AlexHladin wants to merge 9 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds AMAZON_BEDROCK_RUNTIME AppSync data source: types and validation, IAM role generation (bedrock:InvokeModel/Converse), esbuild bundling externals, unit and e2e tests, an example app with schema/resolvers, and user documentation. ChangesAmazon Bedrock Runtime Data Source
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/__tests__/validation/datasources.test.ts (1)
312-390: ⚡ Quick winExtend Bedrock validation tests to cover other optional config keys.
This block validates
modelswell, but it does not assert schema behavior forregion,serviceRoleArn, andiamRoleStatements. Adding those cases will better lock the Bedrock contract and prevent silent schema regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/validation/datasources.test.ts` around lines 312 - 390, Add tests for AMAZON_BEDROCK_RUNTIME to assert optional keys region, serviceRoleArn, and iamRoleStatements are validated correctly: extend the Valid assertions to include configs with region (string), serviceRoleArn (string), and iamRoleStatements (array/object as expected by the schema) and add corresponding Invalid assertions where each of those keys uses an incorrect type (e.g., region: 123, serviceRoleArn: {}, iamRoleStatements: 'not-an-array') so validateConfig(...) returns true for valid cases and throws (use toThrowErrorMatchingSnapshot) for invalid cases; place these new cases alongside the existing assertions array and reuse the same test iteration pattern that calls validateConfig with {...basicConfig, ...config.config}.src/__tests__/dataSources.test.ts (1)
225-226: ⚡ Quick winAdd explicit IAM policy assertions for Bedrock default-role semantics.
Line 225 currently relies on snapshot-only verification. Add direct checks that the generated default policy contains both
bedrock:InvokeModelandbedrock:Converseand that scoped model/profile resources are present; this gives stronger regression protection than snapshot diffs alone.Suggested test hardening
it('should generate default role with scoped models', () => { const api = new Api(given.appSyncConfig(), plugin); const dataSource = new DataSource(api, { type: 'AMAZON_BEDROCK_RUNTIME', name: 'bedrock', description: 'My Bedrock data source', config: { models: [ 'amazon.titan-text-lite-v1', 'arn:aws:bedrock:us-east-1:123456789012:inference-profile/us.anthropic.claude-3-5-haiku-20241022-v1:0', ], }, }); expect(dataSource.compile()).toMatchSnapshot(); + const role = JSON.stringify(dataSource.compileDataSourceIamRole()); + expect(role).toContain('bedrock:InvokeModel'); + expect(role).toContain('bedrock:Converse'); + expect(role).toContain('foundation-model/amazon.titan-text-lite-v1'); + expect(role).toContain('inference-profile/us.anthropic.claude-3-5-haiku-20241022-v1:0'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/dataSources.test.ts` around lines 225 - 226, Update the test that calls dataSource.compile() to add explicit assertions that the generated default IAM policy includes the Bedrock actions and scoped resources: call const compiled = dataSource.compile() (or reuse the existing compile result), locate the default-role policy document in compiled (e.g. the synthesized IAM Policy/PolicyDocument for the default role name used by the datasource), and add expects that the PolicyDocument.Statement array contains an entry whose Action includes "bedrock:InvokeModel" and "bedrock:Converse" (use expect.arrayContaining or toEqual(expect.arrayContaining([...]))), and that the Statement.Resource includes resource ARNs or patterns scoped to model/profile identifiers (use string matching or expect.stringMatching(/model|profile|arn:.*bedrock.*model/)). Keep these assertions alongside the existing snapshot assertion after expect(dataSource.compile()).toMatchSnapshot().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/dataSources.md`:
- Around line 157-164: Add a short prerequisite note in the doc/dataSources.md
near the `models` and `region` bullet points (the section describing model IDs
and ARN expansion) stating that the AWS account must have access to the selected
Bedrock foundation model in the chosen region before using the plugin or copying
the quick start; reference the Bedrock access requirement and link or point to
the AWS Nova invocation docs so readers know where to request model access and
avoid first-run failures.
In `@e2e/datasource-bedrock.e2e.test.ts`:
- Around line 26-39: Narrow the IAM assertion so it targets the Bedrock
datasource role specifically (instead of any role) by enhancing the predicate in
the bedrockRole lookup: use findResourcesByType and then filter roles where the
role clearly belongs to the datasource (for example check
resource.Properties.RoleName or resource.Metadata['aws:cdk:path'] or another
datasource-specific identifier present on the role) and then assert that that
role's Policies contain both 'bedrock:InvokeModel' and 'bedrock:Converse';
update the expect to verify that this specific bedrock datasource role is
defined and that its PolicyDocument Statement Actions include both permissions.
In `@examples/datasource-bedrock/resolvers/summarize.js`:
- Around line 1-14: The request() currently sends Titan-style inputText; update
the invokeModel body for modelId 'amazon.nova-micro-v1:0' to use the Nova
messages payload: include schemaVersion: "messages-v1" and a messages array
(e.g. a single message with role "user" and content containing the text to
summarize), and optionally add an inferenceConfig if needed; in response(),
return the generated text from ctx.result.results.outputText instead of
returning ctx.result.results. Make these changes in the request and response
functions to match the Bedrock Nova messages schema and extract outputText.
In `@src/resources/DataSource.ts`:
- Around line 430-469: The Bedrock IAM resource construction currently always
turns non-ARN strings into foundation-model ARNs and uses config.region, which
breaks inference-profile selectors and can target the wrong region; update the
AMAZON_BEDROCK_RUNTIME branch and getBedrockModelResource to (1) force the
region used for the resource ARN to the AppSync/API region (e.g., use { Ref:
'AWS::Region' } instead of this.config.config?.region) so resources are created
in the same region as the API, and (2) preserve inference-profile selectors by
detecting model strings that are inference-profile IDs or start with
'inference-profile/' and build the ARN using 'inference-profile/<id>' (instead
of always 'foundation-model/<id>'); keep existing behavior of returning the
input when model is already an ARN (getBedrockModelResource) and adjust the
models -> resources mapping in the AMAZON_BEDROCK_RUNTIME case to pass the
enforced region into getBedrockModelResource.
---
Nitpick comments:
In `@src/__tests__/dataSources.test.ts`:
- Around line 225-226: Update the test that calls dataSource.compile() to add
explicit assertions that the generated default IAM policy includes the Bedrock
actions and scoped resources: call const compiled = dataSource.compile() (or
reuse the existing compile result), locate the default-role policy document in
compiled (e.g. the synthesized IAM Policy/PolicyDocument for the default role
name used by the datasource), and add expects that the PolicyDocument.Statement
array contains an entry whose Action includes "bedrock:InvokeModel" and
"bedrock:Converse" (use expect.arrayContaining or
toEqual(expect.arrayContaining([...]))), and that the Statement.Resource
includes resource ARNs or patterns scoped to model/profile identifiers (use
string matching or expect.stringMatching(/model|profile|arn:.*bedrock.*model/)).
Keep these assertions alongside the existing snapshot assertion after
expect(dataSource.compile()).toMatchSnapshot().
In `@src/__tests__/validation/datasources.test.ts`:
- Around line 312-390: Add tests for AMAZON_BEDROCK_RUNTIME to assert optional
keys region, serviceRoleArn, and iamRoleStatements are validated correctly:
extend the Valid assertions to include configs with region (string),
serviceRoleArn (string), and iamRoleStatements (array/object as expected by the
schema) and add corresponding Invalid assertions where each of those keys uses
an incorrect type (e.g., region: 123, serviceRoleArn: {}, iamRoleStatements:
'not-an-array') so validateConfig(...) returns true for valid cases and throws
(use toThrowErrorMatchingSnapshot) for invalid cases; place these new cases
alongside the existing assertions array and reuse the same test iteration
pattern that calls validateConfig with {...basicConfig, ...config.config}.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7282a3ad-338b-4a57-b36d-18d3a6980e02
⛔ Files ignored due to path filters (2)
src/__tests__/__snapshots__/dataSources.test.ts.snapis excluded by!**/*.snapsrc/__tests__/validation/__snapshots__/datasources.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
doc/dataSources.mde2e/datasource-bedrock.e2e.test.tse2e/helpers/assertions.tsexamples/README.mdexamples/datasource-bedrock/resolvers/summarize.jsexamples/datasource-bedrock/schema.graphqlexamples/datasource-bedrock/serverless.ymlsrc/__tests__/dataSources.test.tssrc/__tests__/validation/datasources.test.tssrc/index.tssrc/resources/DataSource.tssrc/resources/JsResolver.tssrc/types/cloudFormation.tssrc/types/common.tssrc/types/plugin.tssrc/validation.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
examples/datasource-bedrock/resolvers/summarize.js (2)
6-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
schemaVersion: "messages-v1"to the Nova InvokeModel body.The
messagespayload is now correct, but Amazon Nova's InvokeModel schema requires the top-levelschemaVersionfield. Without it, the request body is incomplete foramazon.nova-micro-v1:0.🐛 Proposed fix
body: { + schemaVersion: 'messages-v1', messages: [ { role: 'user', content: [{ text: ctx.args.text }], }, ], },Amazon Nova InvokeModel request body required fields schemaVersion messages-v1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/datasource-bedrock/resolvers/summarize.js` around lines 6 - 13, In the summarize resolver, the InvokeModel request body is missing the required top-level schemaVersion; update the body object (the one that currently contains messages: [{ role: 'user', content: [{ text: ctx.args.text }] }]) to include schemaVersion: "messages-v1" alongside messages so the request to the amazon.nova-micro-v1:0 model conforms to the InvokeModel schema.
17-19:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
ctx.result.resultsis undefined for Nova; extract fromoutput.message.content.The
resultskey is specific to legacy Titan models. Nova's InvokeModel response is shaped as{ output: { message: { content: [{ text }] } }, ... }with no top-levelresults, so this handler returnsundefined.🐛 Proposed fix
export function response(ctx) { - return ctx.result.results; + return ctx.result.output.message.content[0].text; }Amazon Nova InvokeModel response shape output message content text appsync ctx.result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/datasource-bedrock/resolvers/summarize.js` around lines 17 - 19, The response function currently returns ctx.result.results which is undefined for Amazon Nova; instead read the Nova shape at ctx.result.output.message.content (an array of content objects) and extract the text fields (e.g., map content items to their .text or join them) to produce the same list/string the rest of the app expects; update the response(ctx) implementation to handle both legacy ctx.result.results and the Nova path (ctx.result.output.message.content) so it works for either model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@examples/datasource-bedrock/resolvers/summarize.js`:
- Around line 6-13: In the summarize resolver, the InvokeModel request body is
missing the required top-level schemaVersion; update the body object (the one
that currently contains messages: [{ role: 'user', content: [{ text:
ctx.args.text }] }]) to include schemaVersion: "messages-v1" alongside messages
so the request to the amazon.nova-micro-v1:0 model conforms to the InvokeModel
schema.
- Around line 17-19: The response function currently returns ctx.result.results
which is undefined for Amazon Nova; instead read the Nova shape at
ctx.result.output.message.content (an array of content objects) and extract the
text fields (e.g., map content items to their .text or join them) to produce the
same list/string the rest of the app expects; update the response(ctx)
implementation to handle both legacy ctx.result.results and the Nova path
(ctx.result.output.message.content) so it works for either model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6643a0b-808e-4a15-b132-a7fd30882ecc
📒 Files selected for processing (1)
examples/datasource-bedrock/resolvers/summarize.js
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
examples/datasource-bedrock/resolvers/summarize.js (1)
6-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
schemaVersion: 'messages-v1'in the Nova invoke payload.For Nova v1 invoke payloads, missing
schemaVersioncan cause request validation failures.Suggested fix
body: { + schemaVersion: 'messages-v1', messages: [ { role: 'user', content: [{ text: ctx.args.text }], }, ], },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/datasource-bedrock/resolvers/summarize.js` around lines 6 - 13, The Nova v1 invoke payload is missing schemaVersion which can break validation; update the request body construction (the object containing messages: [{ role: 'user', content: [{ text: ctx.args.text }] }]) to include schemaVersion: 'messages-v1' at the top level of the body object so the payload becomes { schemaVersion: 'messages-v1', messages: [...] } before sending in the summarize resolver.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/datasource-bedrock/resolvers/summarize.js`:
- Line 5: The resolver uses invokeModel.modelId 'eu.amazon.nova-micro-v1:0'
which doesn't match the Bedrock allowlist in dataSources.bedrock.config.models;
update one side so they align: either add 'eu.amazon-nova-micro-v1:0' (or the
plugin-expected inference-profile ID format) to the serverless.yml
dataSources.bedrock.config.models allowlist, or change the resolver's
invokeModel.modelId to 'amazon.nova-micro-v1:0' so it matches the existing
allowlist; modify the modelId in
examples/datasource-bedrock/resolvers/summarize.js or the models array in
serverless.yml accordingly.
In `@src/resources/DataSource.ts`:
- Around line 466-469: The regex inferenceProfilePrefix in
getBedrockModelResources() only matches /^(us-gov|us|eu|apac)\./ so models
prefixed with "global." are missed; update the pattern to include "global"
(e.g., add "global" to the alternation) so that when you call
inferenceProfilePrefix.exec(model) it will match "global." and let the
subsequent logic that computes baseModelId = model.slice(match[0].length) and
constructs IAM ARNs include the inference-profile/global.* paths.
---
Duplicate comments:
In `@examples/datasource-bedrock/resolvers/summarize.js`:
- Around line 6-13: The Nova v1 invoke payload is missing schemaVersion which
can break validation; update the request body construction (the object
containing messages: [{ role: 'user', content: [{ text: ctx.args.text }] }]) to
include schemaVersion: 'messages-v1' at the top level of the body object so the
payload becomes { schemaVersion: 'messages-v1', messages: [...] } before sending
in the summarize resolver.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5363e93-2f24-494c-b839-c856d37b3121
⛔ Files ignored due to path filters (1)
src/__tests__/__snapshots__/dataSources.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
doc/dataSources.mdexamples/datasource-bedrock/resolvers/summarize.jssrc/__tests__/dataSources.test.tssrc/resources/DataSource.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- doc/dataSources.md
- src/tests/dataSources.test.ts
Collaborator
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/datasource-bedrock.e2e.test.ts (1)
47-47: ⚡ Quick winAvoid order-dependent policy assertion on Line 47.
policies?.[0]makes the test fragile if policy order changes. Assert by predicate instead of fixed index.Suggested fix
- expect(policies?.[0]?.PolicyName).toBe('AppSync-Datasource-bedrock'); + expect( + policies?.some( + (policy) => policy.PolicyName === 'AppSync-Datasource-bedrock', + ), + ).toBe(true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/datasource-bedrock.e2e.test.ts` at line 47, The assertion on line 47 uses a fixed index `policies?.[0]` to access the policy, which makes the test order-dependent and fragile. Replace this index-based access with a predicate-based assertion that searches the entire policies array for a policy with the name 'AppSync-Datasource-bedrock' using a method like find() instead of relying on a fixed position. This ensures the test passes regardless of the order in which policies are returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/datasource-bedrock.e2e.test.ts`:
- Line 43: The non-null assertion operator (!) on bedrockRole at line 43 should
be removed and replaced with an explicit guard check. Instead of using
bedrockRole!, add an if statement to verify that bedrockRole is defined before
accessing bedrockRole.resource.Properties?.Policies. Move the const policies
declaration and any dependent code inside this guard block to ensure safe
narrowing without using the unsafe non-null assertion operator.
---
Nitpick comments:
In `@e2e/datasource-bedrock.e2e.test.ts`:
- Line 47: The assertion on line 47 uses a fixed index `policies?.[0]` to access
the policy, which makes the test order-dependent and fragile. Replace this
index-based access with a predicate-based assertion that searches the entire
policies array for a policy with the name 'AppSync-Datasource-bedrock' using a
method like find() instead of relying on a fixed position. This ensures the test
passes regardless of the order in which policies are returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29c40601-98cc-4dad-bbcb-b120b8811476
📒 Files selected for processing (5)
doc/dataSources.mde2e/datasource-bedrock.e2e.test.tsexamples/datasource-bedrock/resolvers/summarize.jsexamples/datasource-bedrock/serverless.ymlsrc/resources/DataSource.ts
✅ Files skipped from review due to trivial changes (1)
- doc/dataSources.md
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/datasource-bedrock/resolvers/summarize.js
- examples/datasource-bedrock/serverless.yml
- src/resources/DataSource.ts
Collaborator
Author
|
@sid88in please review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
This PR adds AWS Bedrock as a data source support.
Issue(s)
Resolves #727
Summary by CodeRabbit
AMAZON_BEDROCK_RUNTIME) AppSync data source support, including model selection, region support, and IAM role options for synchronous modelinvokeModel/converse.datasource-bedrockexample with asummarize(text: String!)query and updated request/response resolvers.