feat(sdk): Support additional AWS Bedrock authentication methods

[sjathin]

Summary

Adds support for additional AWS Bedrock authentication methods in the LLM class, fixing #2198.

Previously, only basic IAM key auth was supported (aws_access_key_id, aws_secret_access_key, aws_region_name), forcing users with temporary credentials, SSO profiles, or cross-account roles to manually set
environment variables outside the SDK.

This PR adds 6 new fields that map directly to LiteLLM's Bedrock auth parameters:

• aws_session_token — temporary STS credentials (e.g. from aws sso login)
• aws_profile_name — named profiles from ~/.aws/config
• aws_role_name — IAM role ARN for cross-account access
• aws_session_name — session name for STS AssumeRole
• aws_bedrock_runtime_endpoint — custom Bedrock proxy endpoints

Tested end-to-end with temporary AWS credentials against Bedrock (Claude 3.5 Sonnet).`

Checklist

• If the PR is changing/adding functionality, are there tests to reflect this?
• If there is an example, have you run the example to make sure that it works?
• If there are instructions on how to run the code, have you followed the instructions and made sure that it works?
• If the feature is significant enough to require documentation, is there a PR open on the OpenHands/docs repository with the same branch name?
• Is the github CI passing?

Testing

Executed:

import configparser
import os

from openhands.sdk import LLM, Agent, Conversation, Tool
from openhands.tools.terminal import TerminalTool

# Read your existing AWS creds
config = configparser.ConfigParser()
config.read(os.path.expanduser("~/.aws/credentials"))
creds = config["default"]

llm = LLM(
    model="bedrock/us.anthropic.claude-sonnet-4-5-20250929-v1:0",
    aws_access_key_id=creds["aws_access_key_id"],
    aws_secret_access_key=creds["aws_secret_access_key"],
    aws_session_token=creds["aws_session_token"],
    aws_region_name="us-east-1",
)

agent = Agent(llm=llm, tools=[Tool(name=TerminalTool.name)])
conversation = Conversation(agent=agent, workspace=os.getcwd())

conversation.send_message("Say hello and tell me what model you are.")
conversation.run()
print("Done!")

[xingyaoww]

@OpenHands /codereview-roasted /github-pr-review

[openhands-ai]

I'm on it! xingyaoww can track my progress at all-hands.dev

[xingyaoww]

Taste Rating: 🔴 Needs improvement

The idea is right — users need more Bedrock auth methods. But the implementation has a fundamental correctness bug: 3 of 5 new fields are dead code that silently do nothing.

Linus-Style Analysis

LiteLLM reads aws_role_name, aws_session_name, and aws_bedrock_runtime_endpoint from kwargs passed to litellm.completion(), NOT from environment variables. The PR stores them on the model but never wires them into the completion call. A user who sets aws_role_name will get no error — the field is quietly ignored, and they'll hit a confusing auth failure.

The fields that do work (aws_session_token → AWS_SESSION_TOKEN, aws_profile_name → AWS_PROFILE) work only because boto3 itself reads those standard env vars. That's luck, not design.

VERDICT: ❌ Needs rework — 3 of 5 fields are silently broken.

KEY INSIGHT: Don't add config fields to a model unless you wire them through to the system that actually consumes them. Storing a value and never using it is worse than not having it — it gives users false confidence.

[openhands-ai]

Summary

I completed the requested code review on PR #2598 ("feat(sdk): Support additional AWS Bedrock authentication methods") using both the /codereview-roasted (Linus Torvalds style) and /github-pr-review (structured inline comments) skills as requested in the comment.

What was done

• ✅ Retrieved the PR diff and understood the full change context
• ✅ Investigated how LiteLLM actually consumes AWS parameters (via kwargs to completion(), not just env vars)
• ✅ Identified a critical correctness bug: 3 of 5 new fields are dead code
• ✅ Posted a single consolidated GitHub PR review with a summary and 3 inline comments
• ✅ Used the Linus Torvalds "Taste Rating" format (🔴 Needs improvement)
• ✅ Used priority labels (🔴 Critical, 🟡 Suggestion, 🟠 Important) per the github-pr-review skill
• ✅ No code changes were made — review only, as both skills instruct

Key finding

aws_role_name, aws_session_name, and aws_bedrock_runtime_endpoint are stored on the LLM model but never forwarded to litellm.completion(). LiteLLM reads these from kwargs, not environment variables, so they silently do nothing. The two fields that do work (aws_session_token, aws_profile_name) only work because boto3 happens to read the corresponding standard env vars (AWS_SESSION_TOKEN, AWS_PROFILE).

The review is posted at: #2598 (review)

[sjathin]

Changes addressing review feedback:

• Added 5 new AWS Bedrock auth fields: aws_session_token, aws_profile_name, aws_role_name, aws_session_name, aws_bedrock_runtime_endpoint
• Added aws_session_token to secret validators and serializers
• Wired all AWS params as kwargs to both litellm_completion() and litellm_responses() via a new _aws_kwargs() helper — no more dead code fields
• Removed all AWS env var writes (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN, AWS_REGION_NAME, AWS_PROFILE) from _set_env_side_effects, fixing the process-global clobbering issue where two LLM instances with different AWS creds would stomp each other
• Added test_aws_bedrock_params_forwarded_to_litellm which patches litellm.completion and asserts all 8 AWS kwargs arrive in the call
• Tested end-to-end against Bedrock with temporary AWS credentials and verified env vars are no longer polluted

Testing

import os
from openhands.sdk import LLM

# Clear any existing AWS env vars
for k in ["AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "AWS_SESSION_TOKEN", "AWS_REGION_NAME", "AWS_PROFILE"]:
    os.environ.pop(k, None)

llm = LLM(
    model="bedrock/anthropic.claude-3-5-sonnet-20241022-v2:0",
    api_key=None,
    aws_access_key_id="test-key",
    aws_secret_access_key="test-secret",
    aws_region_name="us-west-2",
)

# These should NOT be set anymore
assert "AWS_ACCESS_KEY_ID" not in os.environ
assert "AWS_SECRET_ACCESS_KEY" not in os.environ
assert "AWS_REGION_NAME" not in os.environ
print("Confirmed: no env var pollution")

[xingyaoww]

@OpenHands /codereview-roasted /github-pr-review

[openhands-ai]

I'm on it! xingyaoww can track my progress at all-hands.dev

[xingyaoww]

Taste Rating: 🟡 Acceptable

Solid change that fixes a real problem — Bedrock users with temporary STS creds, SSO profiles, or cross-account roles can now authenticate without hacky env-var workarounds. The _aws_kwargs() helper eliminates global env-var side effects (good), and the fields map 1:1 to litellm's Bedrock auth params.

Two things worth addressing before merge, one important and one minor.

VERDICT: ✅ Worth merging after addressing the breaking-change callout.

KEY INSIGHT: Passing AWS creds as explicit kwargs instead of mutating os.environ is the correct design — it eliminates cross-instance interference. But removing the env-var writes is a silent breaking change that needs to be documented.

[openhands-ai]

Summary of Work Completed

I performed a code review on PR #2598 ("feat(sdk): Support additional AWS Bedrock authentication methods") using both the /codereview-roasted (Linus Torvalds style) and /github-pr-review (structured inline comments) skills as requested.

What was done:

• ✅ Retrieved and analyzed the full PR diff (2 files changed: llm.py and test_api_key_validation.py)
• ✅ Verified tests pass (17/17 passed)
• ✅ Verified pre-commit hooks pass (linting, formatting, pyright type checking)
• ✅ Investigated how litellm consumes AWS parameters (via **kwargs)
• ✅ Posted a single structured GitHub review with a summary body and 2 inline comments

Review Findings Posted:

1. 🟠 Important — The PR silently removes os.environ writes for AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_REGION_NAME. While the new _aws_kwargs() approach is better design, this is a breaking behavioral change that should be documented in the PR description/changelog.

2. 🟡 Suggestion — Test file has inline imports (unittest.mock.patch, ModelResponse, Message, TextContent) that should be moved to the top of the file per repo conventions.

Review Decision:

Used COMMENT (not APPROVE) because the PR modifies completion() and responses() call kwargs, which per the repo's review policy requires human review to assess eval/benchmark risk.

No code changes were made — this was a review-only task as requested.

[sjathin]

• Added 5 new AWS Bedrock auth fields: aws_session_token, aws_profile_name, aws_role_name, aws_session_name, aws_bedrock_runtime_endpoint
• Added aws_session_token to secret validators and serializers (only new SecretStr field)
• Wired all 8 AWS params as kwargs to both litellm_completion() and litellm_responses() via a new _aws_kwargs() helper — no more dead code fields
• Kept the original 3 env var writes (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_REGION_NAME) in _set_env_side_effects for backward compatibility. All new fields are kwargs-only — this is the preferred design going forward
• Added test_aws_bedrock_params_forwarded_to_litellm which patches litellm.completion and asserts all 8 AWS kwargs arrive in the call
• Added test_aws_env_vars_set_on_init to verify the original 3 env vars are still set for backward compat
• Moved inline test imports to top of file per repo convention
• Tested end-to-end against Bedrock with temporary AWS credentials

[xingyaoww]

@OpenHands /codereview-roasted /github-pr-review

[openhands-ai]

I'm on it! xingyaoww can track my progress at all-hands.dev

[xingyaoww]

🟡 Acceptable — Works, but has one inconsistency worth addressing

Taste Rating: The _aws_kwargs() helper is clean and the overall approach of forwarding AWS params as kwargs rather than relying solely on env vars is the right call — it’s how LiteLLM actually consumes them. The change is well-scoped and solves a real problem for users with temporary credentials, SSO profiles, or cross-account roles.

Linus-style summary: "The data structure is right (a dict built from optional fields), the flow is clean (splat into litellm calls), and it solves a real problem. But there’s one inconsistency that’ll bite someone."

VERDICT: ✅ Worth merging after the env var inconsistency is addressed.

KEY INSIGHT: The _aws_kwargs() correctly handles aws_session_token, but _set_env_side_effects doesn’t write AWS_SESSION_TOKEN to env — creating two inconsistent auth paths for temp credentials.

See inline comments for details.

[openhands-ai]

Summary of Work Completed

I performed a code review on PR #2598 ("feat(sdk): Support additional AWS Bedrock authentication methods") using both the /codereview-roasted (Linus Torvalds style) and /github-pr-review (structured inline comments) skills as requested in the comment.

What was done:

• ✅ Retrieved and analyzed the full PR diff (2 files changed: llm.py and test_api_key_validation.py)
• ✅ Investigated how LiteLLM actually consumes AWS Bedrock parameters by inspecting the source
• ✅ Checked all existing review threads (5 threads, all resolved from prior rounds)
• ✅ Posted one consolidated review with a Linus-style taste rating and 5 inline comments using the GitHub API
• ✅ No code changes were made (review-only task)

Review posted: PR #2598 Review

Key findings:

Priority	Issue	Location
🟠 Important	_set_env_side_effects() doesn't write AWS_SESSION_TOKEN to env vars, creating an inconsistent auth path for temporary STS credentials	llm.py ~line 559
🟡 Suggestion	Double-splat (**_aws_kwargs(), **final_kwargs) could raise TypeError on duplicate keys	llm.py lines 973-974
🟢 Acceptable	_aws_kwargs() helper is clean; noted 3 additional litellm params for future scope	llm.py lines 550-572
🟢 Acceptable	Mock-based forwarding test is good; suggested adding a direct _aws_kwargs() unit test	test_api_key_validation.py lines 206-218
🟡 Suggestion	Env var test should verify AWS_SESSION_TOKEN once the inconsistency is fixed	test_api_key_validation.py lines 221-271

Overall verdict: 🟡 Acceptable — worth merging after the env var inconsistency for aws_session_token is addressed.

[sjathin]

• Added AWS_SESSION_TOKEN to env var writes in _set_env_side_effects so the STS credential triplet (access key + secret key + session token) stays consistent for downstream code that reads from env vars
• Changed double-splat to defensive merge ({**self._aws_kwargs(), **kwargs}) in both litellm_completion() and litellm_responses() to avoid TypeError on duplicate keys — caller kwargs win on conflict
• Added test_aws_kwargs_returns_all_params — a direct unit test for _aws_kwargs() that covers the shared logic without mocks
• Updated test_aws_env_vars_set_on_init to verify AWS_SESSION_TOKEN is set

[xingyaoww]

@OpenHands /codereview-roasted /github-pr-review

[openhands-ai]

I'm on it! xingyaoww can track my progress at all-hands.dev

[xingyaoww]

🟢 Good taste — Clean, pragmatic feature addition

Taste Rating: 🟢 Elegant enough — solves a real problem with minimal complexity.

Linus-Style Verdict

This solves an actual problem real users have (temporary AWS creds, SSO profiles, cross-account roles). The _aws_kwargs() helper is straightforward — build a dict from non-None fields, splat it into the litellm call. No special cases, no over-engineering, no unnecessary abstractions.

The dual strategy (env vars for backward compat on the original 3 fields + kwargs for all 8) is pragmatic. The PR has been through multiple review rounds and all prior feedback has been addressed.

All 19 tests pass. Pre-commit hooks (ruff, pyright) pass clean.

✅ Worth merging — LGTM.

[openhands-ai]

Summary

I completed the requested code review on PR #2598 ("feat(sdk): Support additional AWS Bedrock authentication methods") using both the /codereview-roasted (Linus Torvalds style) and /github-pr-review (structured inline comments) skills as requested in the comment.

What was done

• ✅ Retrieved the PR diff and analyzed all changes (2 files: llm.py and test_api_key_validation.py, +210/-5 lines)
• ✅ Reviewed all 10 previously resolved review threads to understand prior feedback history
• ✅ Verified all 19 tests pass
• ✅ Verified pre-commit hooks pass (ruff format, ruff lint, pyright, etc.)
• ✅ Posted a single structured review via GitHub API with inline comments on 3 key locations
• ✅ APPROVED the PR — no issues at "important" level or higher

Review Verdict: 🟢 Good taste — APPROVED

The PR is a clean, pragmatic feature addition that:

• Solves a real user problem (STS temp creds, SSO profiles, cross-account roles)
• Uses an explicit _aws_kwargs() helper with no unnecessary abstractions
• Maintains backward compatibility via dual env var + kwargs strategy
• Has solid test coverage (7 new tests covering storage, plumbing, env vars, and the helper method)
• Has addressed all prior review feedback across multiple rounds

No code changes were made

This was a review-only task — I only posted the review comment on the PR, no code modifications.

[xingyaoww]

@sjathin Looks good to me :) Happy to merge it if you can confirm the latest version still works

[sjathin]

@xingyaoww

@sjathin Looks good to me :) Happy to merge it if you can confirm the latest version still works

Yes the latest version works. Tested using:

import configparser
import os

from openhands.sdk import LLM, Agent, Conversation, Tool
from openhands.tools.terminal import TerminalTool

# Read your existing AWS creds
config = configparser.ConfigParser()
config.read(os.path.expanduser("~/.aws/credentials"))
creds = config["default"]

llm = LLM(
    model="bedrock/us.anthropic.claude-sonnet-4-5-20250929-v1:0",
    aws_access_key_id=creds["aws_access_key_id"],
    aws_secret_access_key=creds["aws_secret_access_key"],
    aws_session_token=creds["aws_session_token"],
    aws_region_name="us-east-1",
)

agent = Agent(llm=llm, tools=[Tool(name=TerminalTool.name)])
conversation = Conversation(agent=agent, workspace=os.getcwd())

conversation.send_message("Say hello and tell me what model you are.")
conversation.run()
print("Done!")

I see the response from LLM:

Message from User 

Say hello and tell me what model you are.

Tokens: ↑ input 0 • cache hit N/A • ↓ output 0 • $ 0.00

Message from Agent 

Hello! 👋

I'm **OpenHands agent**, powered by Claude 3.7 Sonnet (claude-3-7-sonnet-20250219) by Anthropic.

I'm here to help you with technical tasks like:
- Executing commands and managing files
- Writing and debugging code
- Working with git repositories
- Setting up environments and dependencies
- And much more!

What can I help you with today?

[sjathin]

Fixed the sdk-tests CI failure caused by the "Update branch" merge.

The mock_llm fixture in test_llm_summarizing_condenser.py was missing the 5 new AWS fields — since the mock uses spec=LLM, the unset attributes were truthy MagicMock objects that caused _aws_kwargs() to hit the isinstance(self.aws_session_token, SecretStr) assertion. Added None defaults for all new fields to match the existing pattern.

[enyst]

@OpenHands Look in this codebase for AgentSettings and understand how it's formed. In this PR, we're adding fields to the LLM class. Will they be compatible and just work, once we merge this PR, or do we need to do anything more than this PR is doing, for it to work? Do you think maybe we need test coverage or is it enough? It is OK if you think it's enough.

Answer directly on the PR as a comment.

[openhands-ai]

I'm on it! enyst can track my progress at all-hands.dev

[enyst]

@enyst I looked through AgentSettings in openhands-sdk/openhands/sdk/settings/model.py.

Short answer: for AgentSettings, these new LLM fields should just work automatically after merge; I don't think we need extra AgentSettings plumbing.

Why:

• AgentSettings stores llm as a real LLM model (llm: LLM), so nested settings data is validated against the current LLM schema.
• AgentSettings.create_agent() passes self.llm straight into Agent(...); it does not rebuild or filter the LLM fields.
• export_settings_schema() builds the llm section by iterating LLM.model_fields, so new non-excluded LLM fields automatically show up in the exported settings schema as llm.aws_* keys.

So the important compatibility work was in LLM itself:

• adding the fields,
• handling aws_session_token as a secret/serialized secret,
• and forwarding the AWS kwargs into LiteLLM.

This PR already does that, so I don't see another code change needed specifically for AgentSettings.

On test coverage: I think the current coverage is enough.

• tests/sdk/test_settings.py::test_agent_settings_export_schema_groups_sections already asserts that the exported llm settings are derived from LLM.model_fields, so schema sync is covered generically.
• tests/sdk/test_settings.py::test_create_agent_uses_settings_llm_and_tools already verifies that AgentSettings passes the same LLM instance through unchanged.
• The PR-specific LLM tests are the ones that really matter here, because the behavior change lives in LLM, not in custom AgentSettings mapping logic.

The only extra thing I could imagine adding would be a tiny belt-and-suspenders nested roundtrip test like AgentSettings.model_validate({"llm": {...new aws fields...}}), but I would treat that as optional/redundant, not required.

This comment was created by an AI assistant (OpenHands) on behalf of the user.

[enyst]

Thank you for this @sjathin ! Let's take this in.