Skip to content

Conversation

@marckraw
Copy link
Owner

No description provided.

@marckraw marckraw requested a review from Copilot November 25, 2025 10:25
@changeset-bot
Copy link

changeset-bot bot commented Nov 25, 2025

🦋 Changeset detected

Latest commit: 2bfa6a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@mrck-labs/grid-core Minor
@mrck-labs/grid-agents Patch
@mrck-labs/grid-workflows Major
express-test Patch
hono-test Patch
nextjs-test Patch
terminal-agent Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Nov 25, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
grid-docs Ready Ready Preview Comment Nov 25, 2025 2:14pm

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for Amazon Bedrock as an AI provider, enabling the use of Claude, Llama, Titan, and other models hosted on AWS Bedrock. The implementation integrates the @ai-sdk/amazon-bedrock package and extends the existing LLM service architecture to support provider-specific options like Bedrock guardrails.

Key Changes:

  • Added @ai-sdk/amazon-bedrock dependency (v2.2.8) and integrated it into the LLM service
  • Introduced ProviderOptionsMap type system to support provider-specific options across different AI providers
  • Updated LLM service methods to handle Bedrock provider and forward provider-specific options

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
pnpm-lock.yaml Added @ai-sdk/amazon-bedrock package (v2.2.12) and its dependencies including AWS crypto utilities and Smithy packages
packages/core/package.json Added @ai-sdk/amazon-bedrock dependency with version constraint ^2.2.8
packages/core/src/types/llm.types.ts Defined provider-specific option types (BedrockProviderOptions, AnthropicProviderOptions, OpenAIProviderOptions) and ProviderOptionsMap interface
packages/core/src/services/base.llm.service.ts Integrated Bedrock provider throughout all LLM methods and added providerOptions parameter forwarding
packages/core/src/factories/configurable-agent.factory.ts Contains unrelated MCP client lifecycle management changes and formatting updates
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +665 to +676
// Cleanup method to close MCP connections when agent is done
cleanup: async () => {
if (mcpClientService) {
try {
console.log(`[${config.id}] Closing MCP clients...`);
await mcpClientService.closeAll();
console.log(`[${config.id}] ✅ MCP clients closed`);
} catch (error) {
console.error(`[${config.id}] Error closing MCP clients:`, error);
}
}
},
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This change appears to be unrelated to the "Add amazonbedrock support" PR. The addition of a cleanup method for MCP clients should be in a separate PR. Mixing unrelated changes makes it harder to review and potentially revert changes if needed.

Suggested change
// Cleanup method to close MCP connections when agent is done
cleanup: async () => {
if (mcpClientService) {
try {
console.log(`[${config.id}] Closing MCP clients...`);
await mcpClientService.closeAll();
console.log(`[${config.id}] ✅ MCP clients closed`);
} catch (error) {
console.error(`[${config.id}] Error closing MCP clients:`, error);
}
}
},

Copilot uses AI. Check for mistakes.
temperature,
maxOutputTokens,
// Forward provider-specific options (bedrock guardrails, anthropic cache, etc.)
...(providerOptions ? { providerOptions } : {}),
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The providerOptions object should not be nested under a providerOptions key when passing to AI SDK functions. The AI SDK expects provider-specific options (like guardrailConfig for Bedrock) to be passed directly at the top level of the configuration object.

Instead of:

...(providerOptions ? { providerOptions } : {})

It should be:

...(providerOptions?.bedrock || {}),
...(providerOptions?.anthropic || {}),
...(providerOptions?.openai || {}),

Or filter to only spread options for the current provider:

...(providerOptions?.[sdkProvider] || {})
Suggested change
...(providerOptions ? { providerOptions } : {}),
...(providerOptions?.[sdkProvider] || {}),

Copilot uses AI. Check for mistakes.
maxOutputTokens,
tools,
// Forward provider-specific options (bedrock guardrails, anthropic cache, etc.)
...(providerOptions ? { providerOptions } : {}),
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The providerOptions object should not be nested under a providerOptions key when passing to AI SDK functions. The AI SDK expects provider-specific options to be passed directly at the top level. This should be updated to spread only the relevant provider's options based on the sdkProvider variable.

For example:

...(providerOptions?.[sdkProvider] || {})
Suggested change
...(providerOptions ? { providerOptions } : {}),
...(providerOptions?.[sdkProvider] || {}),

Copilot uses AI. Check for mistakes.
Comment on lines +515 to +517
// DON'T close MCP clients here - they need to stay open for subsequent calls
// MCP clients will be closed when the agent is destroyed or process exits
// Closing them here causes "closed client" errors on tool execution
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This change appears to be unrelated to the "Add amazonbedrock support" PR. The MCP client lifecycle management changes should be in a separate PR focused on fixing MCP client connection issues. Mixing unrelated changes makes it harder to review and potentially revert changes if needed.

Suggested change
// DON'T close MCP clients here - they need to stay open for subsequent calls
// MCP clients will be closed when the agent is destroyed or process exits
// Closing them here causes "closed client" errors on tool execution

Copilot uses AI. Check for mistakes.
Comment on lines +108 to +111
console.error(
`[${config.id}] ❌ Failed to initialize MCP clients:`,
error
);
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This formatting change appears to be unrelated to the "Add amazonbedrock support" PR. Pure formatting changes should ideally be in separate commits or PRs to avoid cluttering the main feature changes.

Suggested change
console.error(
`[${config.id}] ❌ Failed to initialize MCP clients:`,
error
);
console.error(`[${config.id}] ❌ Failed to initialize MCP clients:`, error);

Copilot uses AI. Check for mistakes.
temperature,
maxOutputTokens,
// Forward provider-specific options
...(providerOptions ? { providerOptions } : {}),
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The providerOptions object should not be nested under a providerOptions key when passing to AI SDK functions. The AI SDK expects provider-specific options to be passed directly at the top level. This method needs access to the provider/sdkProvider variable to determine which provider's options to spread.

For example:

...(providerOptions?.[provider === 'anthropic' ? 'anthropic' : provider === 'bedrock' ? 'bedrock' : provider === 'openrouter' ? 'openrouter' : 'openai'] || {})
Suggested change
...(providerOptions ? { providerOptions } : {}),
...(providerOptions?.[provider === 'anthropic' ? 'anthropic' : provider === 'bedrock' ? 'bedrock' : provider === 'openrouter' ? 'openrouter' : 'openai'] || {}),

Copilot uses AI. Check for mistakes.
maxOutputTokens,
tools: tools && tools.length > 0 ? (tools as any) : undefined,
// Forward provider-specific options
...(providerOptions ? { providerOptions } : {}),
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The providerOptions object should not be nested under a providerOptions key when passing to AI SDK functions. The AI SDK expects provider-specific options to be passed directly at the top level. This method needs access to the provider variable to determine which provider's options to spread.

For example:

...(providerOptions?.[provider === 'anthropic' ? 'anthropic' : provider === 'bedrock' ? 'bedrock' : provider === 'openrouter' ? 'openrouter' : 'openai'] || {})
Suggested change
...(providerOptions ? { providerOptions } : {}),
...(providerOptions?.[provider === 'anthropic' ? 'anthropic' : provider === 'bedrock' ? 'bedrock' : provider === 'openrouter' ? 'openrouter' : 'openai'] || {}),

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +74
cacheControl?: { type: "ephemeral" };
// Extended thinking/reasoning
thinking?: {
type: "enabled";
budgetTokens: number;
};
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AnthropicProviderOptions interface appears to have incorrect structure. Based on the AI SDK for Anthropic, cache control and thinking options are not passed as top-level properties.

Cache control is passed via experimental_providerMetadata.anthropic.cacheControl, and extended thinking is a model parameter. This interface should either be removed or restructured to match the actual AI SDK API for Anthropic provider options.

Suggested change
cacheControl?: { type: "ephemeral" };
// Extended thinking/reasoning
thinking?: {
type: "enabled";
budgetTokens: number;
};
experimental_providerMetadata?: {
anthropic?: {
cacheControl?: { type: "ephemeral" };
};
};
// Extended thinking is a model parameter, not a provider option.

Copilot uses AI. Check for mistakes.
Comment on lines +599 to +600
// DON'T close MCP clients on error either - they might be needed for retry or next call
// MCP clients will be closed when the agent is destroyed or process exits
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] This change appears to be unrelated to the "Add amazonbedrock support" PR. The MCP client lifecycle management changes should be in a separate PR. Mixing unrelated changes makes it harder to review and potentially revert changes if needed.

Suggested change
// DON'T close MCP clients on error either - they might be needed for retry or next call
// MCP clients will be closed when the agent is destroyed or process exits

Copilot uses AI. Check for mistakes.
@marckraw marckraw merged commit b3e0154 into develop Nov 25, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants