fix(media): allow Bedrock SDK auth for image and PDF tools#72092
Conversation
Greptile SummaryThis PR extends the
Confidence Score: 3/5Not safe to merge as-is — the audio/video runner path will still fail for Bedrock aws-sdk users The image fix works correctly but the fix in runner.entries.ts is functionally broken: the empty-string apiKey is filtered by dedupeApiKeys inside collectProviderApiKeysForExecution, making executeWithApiKeyRotation throw before invoking the provider for all Bedrock aws-sdk deployments without rotation keys. src/media-understanding/runner.entries.ts — resolveProviderExecutionAuth and its interaction with collectProviderApiKeysForExecution/executeWithApiKeyRotation Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/media-understanding/runner.entries.ts
Line: 404-412
Comment:
**aws-sdk empty key filtered out before `executeWithApiKeyRotation`**
When `auth.mode === "aws-sdk"` and the key is empty, `primaryApiKey` is set to `""`. That value is then fed into `collectProviderApiKeysForExecution`, which internally calls `dedupeApiKeys`. `dedupeApiKeys` skips any entry where `value.trim()` is falsy (`if (!apiKey || seen.has(apiKey)) { continue; }`), so the `""` is silently dropped.
If the Bedrock deployment has no rotation keys configured (the common case for role/SSO/profile auth), `apiKeys` comes back as `[]`. `executeWithApiKeyRotation` then immediately throws `"No API keys configured for provider 'amazon-bedrock'."` at line 44, before even reaching the `execute` callback — so the audio/video path still fails for Bedrock `aws-sdk` users despite this fix.
The image path in `resolveImageRuntime` works because it passes the empty string directly to `complete()` and never goes through `executeWithApiKeyRotation`.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(media): allow aws-sdk auth mode for ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const primaryApiKey = | ||
| !auth.apiKey?.trim() && auth.mode === "aws-sdk" ? "" : requireApiKey(auth, params.providerId); | ||
| return { | ||
| apiKeys: collectProviderApiKeysForExecution({ | ||
| provider: params.providerId, | ||
| primaryApiKey: requireApiKey(auth, params.providerId), | ||
| primaryApiKey, | ||
| }), | ||
| providerConfig: params.cfg.models?.providers?.[params.providerId], | ||
| }; |
There was a problem hiding this comment.
executeWithApiKeyRotation
When auth.mode === "aws-sdk" and the key is empty, primaryApiKey is set to "". That value is then fed into collectProviderApiKeysForExecution, which internally calls dedupeApiKeys. dedupeApiKeys skips any entry where value.trim() is falsy (if (!apiKey || seen.has(apiKey)) { continue; }), so the "" is silently dropped.
If the Bedrock deployment has no rotation keys configured (the common case for role/SSO/profile auth), apiKeys comes back as []. executeWithApiKeyRotation then immediately throws "No API keys configured for provider 'amazon-bedrock'." at line 44, before even reaching the execute callback — so the audio/video path still fails for Bedrock aws-sdk users despite this fix.
The image path in resolveImageRuntime works because it passes the empty string directly to complete() and never goes through executeWithApiKeyRotation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/media-understanding/runner.entries.ts
Line: 404-412
Comment:
**aws-sdk empty key filtered out before `executeWithApiKeyRotation`**
When `auth.mode === "aws-sdk"` and the key is empty, `primaryApiKey` is set to `""`. That value is then fed into `collectProviderApiKeysForExecution`, which internally calls `dedupeApiKeys`. `dedupeApiKeys` skips any entry where `value.trim()` is falsy (`if (!apiKey || seen.has(apiKey)) { continue; }`), so the `""` is silently dropped.
If the Bedrock deployment has no rotation keys configured (the common case for role/SSO/profile auth), `apiKeys` comes back as `[]`. `executeWithApiKeyRotation` then immediately throws `"No API keys configured for provider 'amazon-bedrock'."` at line 44, before even reaching the `execute` callback — so the audio/video path still fails for Bedrock `aws-sdk` users despite this fix.
The image path in `resolveImageRuntime` works because it passes the empty string directly to `complete()` and never goes through `executeWithApiKeyRotation`.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03ba12215c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @@ -401,10 +401,12 @@ async function resolveProviderExecutionAuth(params: { | |||
| preferredProfile: params.entry.preferredProfile, | |||
| agentDir: params.agentDir, | |||
| }); | |||
| const primaryApiKey = | |||
| !auth.apiKey?.trim() && auth.mode === "aws-sdk" ? "" : requireApiKey(auth, params.providerId); | |||
There was a problem hiding this comment.
resolveProviderExecutionAuth now permits auth.mode === "aws-sdk" by setting primaryApiKey to "", but this value is immediately passed into collectProviderApiKeysForExecution and then executeWithApiKeyRotation, which drops empty keys and throws when the resulting list is empty. In the exact role/profile/default-chain setup this commit targets (no static key), audio/video provider entries still fail with No API keys configured for provider ... instead of running with SDK-resolved credentials.
Useful? React with 👍 / 👎.
openclaw-barnacle
Bot
added
agents
2e5f135 to
1ca8735
Compare
|
Codex review: stale review; fresh review needed. Summary Next step Review history (1 earlier review cycle)
|
1ca8735 to
e79b802
Compare
|
Greptile P1 / Codex P1 — empty-string apiKey getting filtered by Just rebased onto current main; the four pre-existing CI failures ( |
e79b802 to
5d3defe
Compare
|
Rebased again onto current main (head now |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
ClawSweeper PR egg 🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress. Hatch commandComment Hatchability rules:
What is this egg doing here?
|
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
5d3defe to
ecbd8a0
Compare
|
Rebased onto current
Local re-run: |
clawsweeper
Bot
added
rating: 🧂 unranked krab
ecbd8a0 to
0f1c28f
Compare
|
Rebased again onto No conflicts on rebase; my two commits replay cleanly onto the new tip. |
clawsweeper
Bot
added
rating: 🦐 gold shrimp
|
Status note, since this is now the only open fix for #72031. The three sibling attempts (#72066, #77479, #97525) all closed without merge; this one was kept open and re-rated after the 06-21 rebase onto the keyless I know |
f4d073e to
d3f9a22
Compare
|
Land-ready at final prepared head Work completed:
Proof:
Before/after: issue #72031 and the reporter's live proof show AWS profile-backed Bedrock image execution failing at OpenClaw's static-key gate before this change, then successfully producing an image through the AWS SDK credential chain after it. Independent live retry reached AWS STS through both locally configured profiles, but both tokens were invalid ( |
Thank youuu |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b62189de1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| }); | ||
| const nativeTurnCompleted = await waitForActiveNativeTurnCompletion(activeNativeTurnIds); | ||
| const nativeTurnCompleted = await waitForActiveNativeTurnCompletion(); |
There was a problem hiding this comment.
When a resumed thread reports activeTurnIds, this now discards those ids and waits on the route-level completion flag, which is set by any terminal notification for the thread while the route is unbound. In a resumed compact/review turn, a stale or unrelated turn/completed for the same thread can satisfy this wait, so turn/start can run while the active native turn is still in progress—the wedge this guard is meant to avoid. Use the reported active turn ids (or the exact watchNativeTurnCompletion path) for this wait.
Useful? React with 👍 / 👎.
Media understanding tools failed for amazon-bedrock deployments using auth.mode "aws-sdk" (BYOK via role/SSO/profile creds). Each path called requireApiKey, which throws on the empty-key sentinel before the AWS SDK credential chain can resolve creds at call time. The image path (image.ts) resolves auth via getApiKeyForModel; the audio/video paths route through resolveProviderExecutionAuth. Both now mirror the chat path's allowMissingApiKeyModes allowance: when the resolved key is empty and the mode is aws-sdk, execute keyless and let the SDK resolve credentials. The image path skips setRuntimeApiKey so no empty-string secret is persisted; audio/video return the kind:"none" execution auth so the runner bypasses key rotation. Closes openclaw#72031
|
Merged via squash.
|
5 participants
What Problem This Solves
Amazon Bedrock chat already accepts
auth.mode: "aws-sdk"without a literal API key, allowing the AWS SDK credential chain to resolve profile, role, SSO, environment, or instance credentials at request time. The image and PDF tools instead calledrequireApiKeyfirst, so both failed before the Bedrock runtime could use those credentials.Closes #72031.
Why This Change Was Made
aws-sdkand the selected runtime isbedrock-converse-stream.aws-sdkruntime auth as available during automatic image/PDF model selection, including default-chain setups without AWS environment markers.mediaUnderstandingProvidersaudio/video contract, so those branches were unreachable speculative surface.The Bedrock stream constructs
BedrockRuntimeClientwithout injected credentials in SDK-chain mode; AWS SDK 3.1078.0 supplies its default credential provider in that case.User Impact
Bedrock users can use the image and PDF tools with the same AWS profile, role, SSO, environment, or instance credentials that already work for normal chat. No static Bedrock bearer token or stored API-key surrogate is required.
Evidence
tbx_01kwxgk2yze3acn2bnnwn8kr0f. The initial 59-test execution run is linked in Actions.corepack pnpm check:changed— passed on the same Testbox lease, including core/core-test typechecks, changed-file lint, changelog attribution, dependency guards, import cycles, and policy checks.oxfmt --checkpassed for all four changed TypeScript files on Testbox.InvalidClientTokenId; no independent paid Bedrock request was possible from this checkout.AI-assisted