Skip to content

Conversation

@sebsto
Copy link
Collaborator

@sebsto sebsto commented Jul 9, 2025

@sebsto sebsto requested a review from Copilot July 9, 2025 11:26
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

Adds support for Bedrock API Keys by extending authentication, configuration, and client setup, plus corresponding tests

  • Introduces apiKey case in BedrockAuthentication with redacted descriptions and resolver logic
  • Refactors client creation via a shared prepareConfig to inject API key headers and unset environment variables
  • Adds tests to verify that API key authentication sets headers, yields no AWS credential resolver, and redacts the key in descriptions

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
Tests/AuthenticationTests.swift Tests for apiKey authentication, header injection, resolver behavior, and key redaction
Sources/Protocols/BedrockConfigProtocol.swift Defines BedrockConfigProtocol for shared client configuration
Sources/BedrockService.swift Refactors client setup into prepareConfig; adds API key header handling and unsetenv
Sources/BedrockAuthentication.swift Adds apiKey auth case, updates description and resolver switch, adjusts logging level
Comments suppressed due to low confidence (3)

Sources/BedrockService.swift:116

  • [nitpick] The return type for createBedrockClient changed from the BedrockClientProtocol to the concrete BedrockClient. If you rely on abstraction for mocking or future changes, consider returning the protocol to preserve flexibility.
    internal static func createBedrockClient(

Tests/AuthenticationTests.swift:84

  • [nitpick] This assertion doesn't include a custom failure message, which could make diagnosing test failures harder. Adding an explicit message will clarify the expected region in case of a mismatch.
        #expect(config.region == Region.useast1.rawValue)  // default region

Sources/BedrockAuthentication.swift:82

  • [nitpick] Logging static AWS credential usage at info may hide a security risk. Retaining a warning level would better highlight non-recommended production usage.
            logger.info("Using static AWS credentials. This is not recommended for production.")

}

/// Generic function to create client configuration and avoid duplication code.
internal static func prepareConfig<C: BedrockConfigProtocol>(
Copy link

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

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

[nitpick] The prepareConfig function bundles initialization, credential resolver setup, API-key header injection, and environment cleanup. Consider splitting these responsibilities into focused helper methods or adding detailed comments to clarify each step and improve readability and testability.

Copilot uses AI. Check for mistakes.

//We uncheck AWS_BEARER_TOKEN_BEDROCK to avoid conflict with future AWS SDK version
//see https://docs.aws.amazon.com/bedrock/latest/userguide/getting-started-api-keys.html
unsetenv("AWS_BEARER_TOKEN_BEDROCK")
Copy link

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

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

[nitpick] Unsetting the environment variable globally may have unintended side effects for other concurrent code. Consider scoping this change or documenting its impact on other AWS SDK behavior.

Suggested change
unsetenv("AWS_BEARER_TOKEN_BEDROCK")
// Temporarily remove AWS_BEARER_TOKEN_BEDROCK from the environment for this configuration
var environment = ProcessInfo.processInfo.environment
environment["AWS_BEARER_TOKEN_BEDROCK"] = nil
// Pass the modified environment to the relevant configuration or process if needed

Copilot uses AI. Check for mistakes.
@sebsto sebsto merged commit 5bfbb96 into main Jul 9, 2025
16 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.

1 participant