fix(media-understanding): allow aws-sdk auth mode in image tool [AI-assisted]#72066
fix(media-understanding): allow aws-sdk auth mode in image tool [AI-assisted]#72066luyao618 wants to merge 1 commit into
Conversation
Greptile SummaryThis PR fixes a real bug where
Confidence Score: 3/5Not safe to merge — the runner.entries.ts fix is incomplete and the aws-sdk path for audio/video still fails at runtime. One P1 defect: the runner.entries.ts change doesn't actually fix the aws-sdk path because dedupeApiKeys drops the empty string, leaving an empty keys array that causes executeWithApiKeyRotation to throw. The image.ts fix is correct, but the scope of the stated fix exceeds what was actually repaired. src/media-understanding/runner.entries.ts — the resolveProviderExecutionAuth function and its interaction with executeWithApiKeyRotation when apiKeys is empty Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/media-understanding/runner.entries.ts
Line: 404-408
Comment:
**`executeWithApiKeyRotation` still throws for `aws-sdk` mode**
`requireApiKeyAllowAwsSdk` returns `""` when `auth.mode === "aws-sdk"` and no static key is present. `dedupeApiKeys` in `collectProviderApiKeysForExecution` silently drops empty strings (`if (!apiKey || seen.has(apiKey)) continue`), so if no additional keys are configured for `amazon-bedrock`, `apiKeys` comes out as `[]`. `executeWithApiKeyRotation` then immediately throws `No API keys configured for provider "amazon-bedrock"` before the execute callback is ever invoked — the same failure mode that was fixed for `image.ts`, just one level deeper.
The `image.ts` fix works because it bypasses `executeWithApiKeyRotation` entirely (it sets the key in `authStorage` and calls `complete()` directly). The runner path needs a parallel fix — either by treating an empty primary key as a valid sentinel for `aws-sdk` mode in `collectProviderApiKeysForExecution` / `dedupeApiKeys`, or by short-circuiting the key-rotation layer when `auth.mode === "aws-sdk"`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/model-auth.test.ts
Line: 89
Comment:
**No test cases added for `requireApiKeyAllowAwsSdk`**
`requireApiKeyAllowAwsSdk` is imported and destructured in `beforeAll`, but no test cases covering its behaviour appear in the diff. The two interesting branches — `aws-sdk` mode with a missing key (should return `""`) and a non-`aws-sdk` mode with a missing key (should throw, delegating to `requireApiKey`) — are untested at the unit level. The new `image.test.ts` case exercises the mock but doesn't reach the real implementation.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(media-understanding): allow aws-sdk ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return { | ||
| apiKeys: collectProviderApiKeysForExecution({ | ||
| provider: params.providerId, | ||
| primaryApiKey: requireApiKey(auth, params.providerId), | ||
| primaryApiKey: requireApiKeyAllowAwsSdk(auth, params.providerId), | ||
| }), |
There was a problem hiding this comment.
executeWithApiKeyRotation still throws for aws-sdk mode
requireApiKeyAllowAwsSdk returns "" when auth.mode === "aws-sdk" and no static key is present. dedupeApiKeys in collectProviderApiKeysForExecution silently drops empty strings (if (!apiKey || seen.has(apiKey)) continue), so if no additional keys are configured for amazon-bedrock, apiKeys comes out as []. executeWithApiKeyRotation then immediately throws No API keys configured for provider "amazon-bedrock" before the execute callback is ever invoked — the same failure mode that was fixed for image.ts, just one level deeper.
The image.ts fix works because it bypasses executeWithApiKeyRotation entirely (it sets the key in authStorage and calls complete() directly). The runner path needs a parallel fix — either by treating an empty primary key as a valid sentinel for aws-sdk mode in collectProviderApiKeysForExecution / dedupeApiKeys, or by short-circuiting the key-rotation layer when auth.mode === "aws-sdk".
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/media-understanding/runner.entries.ts
Line: 404-408
Comment:
**`executeWithApiKeyRotation` still throws for `aws-sdk` mode**
`requireApiKeyAllowAwsSdk` returns `""` when `auth.mode === "aws-sdk"` and no static key is present. `dedupeApiKeys` in `collectProviderApiKeysForExecution` silently drops empty strings (`if (!apiKey || seen.has(apiKey)) continue`), so if no additional keys are configured for `amazon-bedrock`, `apiKeys` comes out as `[]`. `executeWithApiKeyRotation` then immediately throws `No API keys configured for provider "amazon-bedrock"` before the execute callback is ever invoked — the same failure mode that was fixed for `image.ts`, just one level deeper.
The `image.ts` fix works because it bypasses `executeWithApiKeyRotation` entirely (it sets the key in `authStorage` and calls `complete()` directly). The runner path needs a parallel fix — either by treating an empty primary key as a valid sentinel for `aws-sdk` mode in `collectProviderApiKeysForExecution` / `dedupeApiKeys`, or by short-circuiting the key-rotation layer when `auth.mode === "aws-sdk"`.
How can I resolve this? If you propose a fix, please make it concise.| @@ -88,6 +88,7 @@ let applyAuthHeaderOverride: typeof import("./model-auth.js").applyAuthHeaderOve | |||
| let applyLocalNoAuthHeaderOverride: typeof import("./model-auth.js").applyLocalNoAuthHeaderOverride; | |||
| let hasUsableCustomProviderApiKey: typeof import("./model-auth.js").hasUsableCustomProviderApiKey; | |||
There was a problem hiding this comment.
requireApiKeyAllowAwsSdk
requireApiKeyAllowAwsSdk is imported and destructured in beforeAll, but no test cases covering its behaviour appear in the diff. The two interesting branches — aws-sdk mode with a missing key (should return "") and a non-aws-sdk mode with a missing key (should throw, delegating to requireApiKey) — are untested at the unit level. The new image.test.ts case exercises the mock but doesn't reach the real implementation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-auth.test.ts
Line: 89
Comment:
**No test cases added for `requireApiKeyAllowAwsSdk`**
`requireApiKeyAllowAwsSdk` is imported and destructured in `beforeAll`, but no test cases covering its behaviour appear in the diff. The two interesting branches — `aws-sdk` mode with a missing key (should return `""`) and a non-`aws-sdk` mode with a missing key (should throw, delegating to `requireApiKey`) — are untested at the unit level. The new `image.test.ts` case exercises the mock but doesn't reach the real implementation.
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: 3fe2cca346
ℹ️ 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".
|
|
||
| export function requireApiKeyAllowAwsSdk(auth: ResolvedProviderAuth, provider: string): string { | ||
| if (auth.mode === "aws-sdk") { | ||
| return normalizeSecretInput(auth.apiKey) ?? ""; |
There was a problem hiding this comment.
Returning "" for aws-sdk leaves callers with an empty credential even when they immediately register it in auth storage (for example src/media-understanding/image.ts:205-206). The existing aws-sdk handling in src/agents/pi-embedded-runner/run/auth-controller.ts uses a non-empty sentinel specifically so pi's auth check treats the provider as configured; writing an empty value bypasses that safeguard and can still fail Bedrock image calls that rely on IAM/instance-role credentials. Use a non-empty sentinel (or avoid setting runtime auth for this mode) instead of returning an empty string.
Useful? React with 👍 / 👎.
The image tool unconditionally called requireApiKey which throws when no static API key is present. For amazon-bedrock using IAM role or instance profile credentials (auth.mode === 'aws-sdk'), the AWS SDK resolves credentials at call time and no static key exists. Add requireApiKeyAllowAwsSdk helper that returns an empty string for aws-sdk mode instead of throwing, and use it in resolveImageRuntime, resolveMinimaxVlmFallbackRuntime, and resolveProviderExecutionAuth. AI-assisted (Claude Code via Hermes orchestration). Fixes openclaw#72031
3fe2cca to
bd00adc
Compare
|
Closing this as duplicate or superseded after Codex automated review. PR #72066 should close as superseded by the broader open PR #72092, which tracks the same linked bug #72031 and carries the media-understanding aws-sdk work through image plus audio/video paths. Current main still has the bug, but #72066's own review discussion identifies concrete correctness gaps in its implementation, while #72092 is the better canonical path. Best possible solution: Close #72066 as superseded and continue review on #72092 as the canonical fix for #72031. The implementation that should land needs to preserve the aws-sdk no-static-key contract across image, MiniMax fallback, and audio/video media runner paths, including the empty-key rotation/runtime-auth semantics and regression tests. What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex Review notes: model gpt-5.5, reasoning high; reviewed against 7e13f3f51401. |
1 participant
Summary
imagetool fails for all Bedrock models when using AWS SDK credential resolution (IAM role, instance profile,AWS_PROFILE).requireApiKeythrowsNo API key resolved for provider "amazon-bedrock" (auth mode: aws-sdk)even though chat/completion with the same model works fine.requireApiKeyAllowAwsSdkhelper inmodel-auth-runtime-shared.tsthat returns an empty string foraws-sdkmode instead of throwing. ReplacedrequireApiKeycalls inimage.ts(2 sites) andrunner.entries.ts(1 site) with the new helper.requireApiKeyitself is unchanged — all non-aws-sdk auth modes still throw on missing keys. The chat/completion path's existingallowMissingApiKeyModesmechanism is untouched.Change Type (select all)
Scope (select all touched areas)
src/media-understanding/)src/agents/model-auth*)Linked Issue/PR
imagetool fails for Bedrock withauth mode: aws-sdk—requireApiKeythrows even when AWS SDK creds are available #72031Root Cause
resolveImageRuntimeinimage.tsunconditionally calledrequireApiKey, which throws whenauth.apiKeyis empty regardless ofauth.mode. Foramazon-bedrockwithaws-sdkmode, no static API key exists — the AWS SDK resolves credentials at call time. The chat path already handled this viaallowMissingApiKeyModes: ["aws-sdk"], but the image path did not.Regression Test Plan
src/media-understanding/image.test.tsdescribeImageWithModelwithauth.mode === "aws-sdk"and no API key must succeed without throwing.User-visible / Behavior Changes
Image analysis with Bedrock models using AWS SDK credentials now works (previously always failed).
Diagram (if applicable)
N/A
Security Impact (required)
requireApiKeyAllowAwsSdkonly relaxes the API key requirement foraws-sdkmode, which by definition uses the AWS SDK credential chain instead of static keys. All other auth modes still require an explicit API key. The empty string passed asapiKeyis not used by the Bedrock SDK path — it resolves credentials independently.