Skip to content

Conversation

@akshaydeo
Copy link
Contributor

@akshaydeo akshaydeo commented Dec 15, 2025

Improve key selection for non-model-required operations

This PR enhances Bifrost's key selection logic to handle file operations and batch list operations more gracefully. When a model is not required for a request, the system now falls back to using the first available key for the provider instead of returning an error.

Changes

  • Modified requestWorker in bifrost.go to check if a model is required before returning an error during key selection
  • For operations that don't require a model (like file operations), the system now selects the first available key
  • Added support for extra parameters in batch and file operations for Bedrock provider
  • Added BatchExtraParams and FileExtraParams to test configuration to support provider-specific parameters
  • Updated Bedrock tests to use S3 bucket and role ARN from environment variables
  • Made IsModelRequired function private (renamed to isModelRequired) as it's only used internally
  • Fixed a potential nil pointer dereference in Bedrock's FileList method

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (Next.js)
  • Docs

How to test

# Run Bedrock tests with AWS credentials and S3 bucket configured
export AWS_S3_BUCKET="your-s3-bucket"
export AWS_BEDROCK_ROLE_ARN="your-role-arn"
go test ./core/providers/bedrock/...

# Run all tests
go test ./...

Breaking changes

  • Yes
  • No

Security considerations

The PR handles AWS credentials and S3 bucket information more robustly, ensuring proper configuration for Bedrock batch operations.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved nil-safety checks to prevent potential errors in provider operations.
    • Enhanced fallback behavior for non-required operations to continue with available resources instead of failing.
  • Chores

    • Extended provider configuration to support additional credentials (ARN) for enhanced authentication.
    • Updated test infrastructure to support extended operation parameters.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

This pull request introduces a conditional fallback mechanism for non-required model operations in the request handler and extends the test infrastructure to support extra parameters for batch and file operations. It includes a nil-pointer safety fix for Bedrock key configuration, renames a public function to unexported scope, and updates test configurations with environment-driven parameters.

Changes

Cohort / File(s) Summary
Request handling fallback
core/bifrost.go
Adds conditional error handling in requestWorker: when model selection fails for non-required operations, fetches available keys via GetKeysForProvider and uses the first key instead of returning an error. Required models still error out on failure.
Test configuration fields
core/internal/testutil/account.go
Adds two new fields to ComprehensiveTestConfig: BatchExtraParams and FileExtraParams (both map[string]interface{}) for carrying arbitrary parameters in batch and file operation tests.
Test parameter propagation
core/internal/testutil/batch.go
Propagates ExtraParams from test config to BifrostBatchCreateRequest, BifrostFileUploadRequest, and BifrostFileListRequest in batch, file, and integration test flows.
Bedrock provider updates
core/providers/bedrock/bedrock.go
Adds nil-check for key.BedrockKeyConfig before accessing Region field in FileList to prevent nil-pointer dereference.
Bedrock test environment integration
core/providers/bedrock/bedrock_test.go
Reads AWS_S3_BUCKET and AWS_BEDROCK_ROLE_ARN from environment, constructs BatchExtraParams and FileExtraParams conditionally, and wires them into ComprehensiveTestConfig for test execution.
Utility function scope change
core/utils.go
Renames IsModelRequired to isModelRequired (unexported) and updates call site in validateRequest to match.
Configuration updates
tests/integrations/config.json
Adds arn field to bedrock_key_config for environment-based credential management.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Extra attention recommended for:
    • Verification that the fallback logic in bifrost.go correctly determines model requiredness and handles edge cases where GetKeysForProvider returns empty or nil
    • Confirmation that ExtraParams propagation is consistent across all request types (BatchCreate, FileUpload, FileList) and doesn't inadvertently break existing test paths
    • Validation of the bedrock nil-check prevents all potential nil-dereference paths for BedrockKeyConfig
    • Ensuring environment variable handling in bedrock_test.go gracefully handles missing or malformed AWS credentials without breaking test execution

Poem

🐰 A fallback path for those unbound,
With extra params scattered 'round,
Safe nil-checks stand guard so true,
Config flows from env right through,
Scope refined, the refactor's done!

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 12-16-fixes_bedrock_tests

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b5df028 and 258f572.

📒 Files selected for processing (7)
  • core/bifrost.go (1 hunks)
  • core/internal/testutil/account.go (1 hunks)
  • core/internal/testutil/batch.go (10 hunks)
  • core/providers/bedrock/bedrock.go (1 hunks)
  • core/providers/bedrock/bedrock_test.go (2 hunks)
  • core/utils.go (2 hunks)
  • tests/integrations/config.json (1 hunks)

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

🧪 Test Suite Available

This PR can be tested by a repository admin.

Run tests for PR #1098

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@akshaydeo akshaydeo marked this pull request as ready for review December 15, 2025 18:38
@akshaydeo akshaydeo force-pushed the 12-16-fixes_bedrock_tests branch from 6e76f03 to 258f572 Compare December 15, 2025 18:41
@akshaydeo akshaydeo merged commit d959f9b into main Dec 15, 2025
6 of 7 checks passed
@akshaydeo akshaydeo deleted the 12-16-fixes_bedrock_tests branch December 15, 2025 18:41
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