Skip to content

fix(tools): honor config auth for media model selection#85356

Closed
hxy91819 wants to merge 5 commits into
openclaw:mainfrom
hxy91819:fix/media-tool-config-auth
Closed

fix(tools): honor config auth for media model selection#85356
hxy91819 wants to merge 5 commits into
openclaw:mainfrom
hxy91819:fix/media-tool-config-auth

Conversation

@hxy91819
Copy link
Copy Markdown
Member

Summary

  • Use a shared config-aware auth preflight for media tool model selection so custom providers with models.providers.*.apiKey can be auto-selected.
  • Thread workspaceDir through generation tool model/list availability paths so discovery and execution use the same auth context.
  • Keep aws-sdk auth out of API-key-only tool preflight and preserve video reference-audio capability semantics.

Verification

  • node scripts/run-vitest.mjs src/agents/tools/model-config.helpers.test.ts src/agents/tools/media-tool-shared.test.ts src/agents/tools/image-generate-tool.test.ts src/agents/tools/music-generate-tool.test.ts src/agents/tools/image-tool.test.ts src/agents/tools/pdf-tool.model-config.test.ts (262 passed)
  • ReadLints on changed tool files
  • Droid narrow review of accepted fixes: PASS

Real behavior proof

Behavior addressed: Custom provider image/media tool auto-selection now recognizes config-backed API keys while avoiding aws-sdk-only providers that cannot satisfy current tool API-key execution.
Real environment tested: Local Mason worktree on Linux with focused Vitest coverage and review checks.
Exact steps or command run after this patch: node scripts/run-vitest.mjs src/agents/tools/model-config.helpers.test.ts src/agents/tools/media-tool-shared.test.ts src/agents/tools/image-generate-tool.test.ts src/agents/tools/music-generate-tool.test.ts src/agents/tools/image-tool.test.ts src/agents/tools/pdf-tool.model-config.test.ts
Evidence after fix: Focused suite reported 12 passed (12) test files and 262 passed (262) tests.
Observed result after fix: Config-backed custom provider auth is accepted for image/PDF/media model selection, aws-sdk-only tool preflight is rejected, and generation list/model availability receives workspace auth context.
What was not tested: Full repository check, live provider calls, and existing video reference-audio test failures unrelated to this diff.

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: M maintainer Maintainer-authored PR labels May 22, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 22, 2026

Codex review: found issues before merge.

Latest ClawSweeper review: 2026-05-22 15:35 UTC / May 22, 2026, 11:35 AM ET.

Workflow note: Future ClawSweeper reviews update this same comment in place.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Summary
The PR adds a config-aware media/PDF/image tool auth preflight, threads workspace auth context through list/model resolution paths, and adds focused model-selection tests.

Reproducibility: yes. Current main source shows config-backed provider API keys are missed by env/profile-only tool auth checks, and the PR’s remaining aws-sdk/API-key mismatch is visible by comparing the new helper with runtime auth precedence.

PR rating
Overall: 🦐 gold shrimp
Proof: 🌊 off-meta tidepool
Patch quality: 🦐 gold shrimp
Summary: Useful focused fix with regression coverage, but merge confidence is limited by the remaining auth-provider correctness blocker.

Rank-up moves:

  • Resolve the aws-sdk/API-key preflight mismatch and update the focused Bedrock/config auth tests.
  • Run the focused suite again and add a changed-surface gate or equivalent maintainer validation before landing.
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Real behavior proof
Not applicable: The contributor proof gate is waived for this member-authored, maintainer-labeled PR; the body records focused Vitest output rather than live provider proof.

Risk before merge

  • Merging as-is can list or auto-select an aws-sdk-mode provider based on config/profile API-key evidence even though execution still resolves aws-sdk and then requires a string API key.
  • The PR changes provider availability for media/PDF/image tool model selection, so existing provider auth precedence and upgrade behavior need maintainer confirmation before merge.
  • The supplied proof is focused unit coverage only; no live provider call or broader changed-surface gate is recorded in the provided context.

Maintainer options:

  1. Align preflight with current runtime (recommended)
    Reject aws-sdk-mode providers before accepting config/profile API-key shortcuts unless the runtime resolver is also changed to return a usable API key for those cases.
  2. Define API-key override precedence
    If maintainers want config/profile API keys to override aws-sdk for these tools, update resolveApiKeyForProvider, docs, and tests so the new precedence is intentional.
  3. Defer aws-sdk media behavior
    Pause the Bedrock/aws-sdk portion and land only the custom-provider config auth path if broader aws-sdk tool execution needs separate design.

Next step before merge
Human maintainer review is needed because the remaining blocker is the intended aws-sdk/API-key precedence and upgrade behavior for media/PDF tools.

Security
Cleared: The diff changes auth-selection code and tests without adding dependencies, workflow changes, lockfile changes, or new credential exposure.

Review findings

  • [P2] Reject aws-sdk before accepting config keys — src/agents/tools/model-config.helpers.ts:156-164
Review details

Best possible solution:

Land after the tool preflight and runtime auth resolver agree on aws-sdk versus API-key precedence, with focused coverage for config API keys, profiles, implicit Bedrock/aws-sdk, and explicit auth: "api-key" override behavior.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main source shows config-backed provider API keys are missed by env/profile-only tool auth checks, and the PR’s remaining aws-sdk/API-key mismatch is visible by comparing the new helper with runtime auth precedence.

Is this the best way to solve the issue?

No, not yet. The shared helper is the right shape, but it must mirror the execution auth contract or deliberately change that contract with tests and docs.

Label justifications:

  • P2: This is a normal-priority auth/provider selection fix with limited but real media and PDF tool impact.
  • merge-risk: 🚨 compatibility: The PR changes which configured providers become eligible for automatic tool model selection.
  • merge-risk: 🚨 auth-provider: The changed helper decides whether provider credentials qualify for tool execution and currently diverges from runtime aws-sdk handling.
  • rating: 🦐 gold shrimp: Current PR rating is 🦐 gold shrimp because proof is 🌊 off-meta tidepool, patch quality is 🦐 gold shrimp, and Useful focused fix with regression coverage, but merge confidence is limited by the remaining auth-provider correctness blocker.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The contributor proof gate is waived for this member-authored, maintainer-labeled PR; the body records focused Vitest output rather than live provider proof.

Full review comments:

  • [P2] Reject aws-sdk before accepting config keys — src/agents/tools/model-config.helpers.ts:156-164
    This accepts config-backed API keys before checking for aws-sdk mode, but resolveApiKeyForProvider returns aws-sdk for auth: "aws-sdk" and implicit Bedrock before config/profile keys, and resolveModelRuntimeApiKey then requires a string API key. That can make a provider listable/selectable while execution still fails; either move the aws-sdk rejection ahead of these shortcuts or change runtime precedence with matching tests.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.84

Acceptance criteria:

  • node scripts/run-vitest.mjs src/agents/tools/model-config.helpers.test.ts src/agents/tools/media-tool-shared.test.ts src/agents/tools/image-generate-tool.test.ts src/agents/tools/music-generate-tool.test.ts src/agents/tools/image-tool.test.ts src/agents/tools/pdf-tool.model-config.test.ts

What I checked:

  • current-main auth gap: Current main builds implicit tool model candidates through hasAuthForProvider, which checks env/profile auth but does not receive cfg or workspaceDir, so models.providers.*.apiKey is not considered for these candidates. (src/agents/tools/model-config.helpers.ts:111, 99d7c7077e23)
  • PR helper ordering: The PR accepts hasUsableCustomProviderApiKey before rejecting resolveModelAuthMode(...) === "aws-sdk", which can make an aws-sdk-configured provider pass API-key-only tool preflight. (src/agents/tools/model-config.helpers.ts:156, 26083be72952)
  • runtime precedence: Runtime auth returns aws-sdk immediately for auth: "aws-sdk" and for implicit Amazon Bedrock before reaching env/config/profile API-key resolution. (src/agents/model-auth.ts:642, 99d7c7077e23)
  • API-key-only execution path: Media/PDF tool runtime auth calls getApiKeyForModel and then requireApiKey, so an aws-sdk result is not executable in this path. (src/agents/tools/media-tool-shared.ts:601, 99d7c7077e23)
  • history provenance: Blame attributes the current tool auth helper and media selection code to commit 8432918. (src/agents/tools/model-config.helpers.ts:40, 84329182a7c4)
  • protected review context: The provided GitHub context shows the PR is open, member-authored, and carries the protected maintainer label, so it should not be auto-closed by cleanup review.

Likely related people:

  • vincentkoc: Current-main blame and log point the central tool auth/model-selection helpers and media shared code to commit 8432918. (role: introduced current behavior; confidence: medium; commits: 84329182a7c4; files: src/agents/tools/model-config.helpers.ts, src/agents/tools/media-tool-shared.ts, src/agents/model-auth.ts)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 99d7c7077e23.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. labels May 22, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 22, 2026

ClawSweeper PR egg

🔥 Warming up: real-behavior proof passed; findings, security review, or rank-up moves are still in progress.

Hatch command

Comment @clawsweeper hatch when this PR is hatchable.

Hatchability rules:

  • Merged PRs are hatchable.
  • Open PRs are hatchable when they are status: 👀 ready for maintainer look, status: 🚀 automerge armed, or labeled clawsweeper:automerge.
  • Closed unmerged PRs are hatchable only when one of those hatchable labels is still present in the durable record.
What is this egg doing here?
  • Eggs appear after the PR passes real-behavior proof. It is here for vibes, not verdicts: it does not change labels, ratings, merge decisions, or automation.
  • The shell reacts to review momentum: open follow-up work warms it up, re-review makes it wobble, and a clean final review lets it hatch.
  • Hatchability usually comes from sufficient real-behavior proof, no blocking P0/P1/P2 findings, no security attention needed, and clean correctness. A merged PR is already final, so merge makes the egg hatchable independently.
  • The hatch is seeded from this repository and PR number, so the same PR keeps the same creature; the reviewed head SHA can only change safe visual details.
  • Rarity is just collectible sparkle: 🥚 common, 🌱 uncommon, 💎 rare, ✨ glimmer, and 🌈 legendary.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels May 22, 2026
@hxy91819
Copy link
Copy Markdown
Member Author

/clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 22, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels May 22, 2026
@hxy91819
Copy link
Copy Markdown
Member Author

Closing in favor of a smaller follow-up PR.

Why close #85356

The original fix direction was correct (config-aware auth preflight for media tools), but the landed scope grew into a broader media-tool auth refactor:

  • pulled in hasRuntimeAvailableProviderAuth
  • added aws-sdk rejection and multiple Bedrock boundary commits
  • expanded risk/complexity beyond the reported bug (models.providers.*.apiKey not seen during image model auto-selection)

Replacement approach (Plan B)

  • unify on hasProviderAuthForTool() with only:
    • existing hasAuthForProvider() (env + auth profile)
    • hasUsableCustomProviderApiKey(cfg, provider)
  • keep shared helper usage across image / PDF / generation tools
  • thread workspaceDir for list/model availability alignment
  • do not touch aws-sdk / Bedrock semantics

A narrower replacement PR will be opened from fix/media-tool-config-auth-plan-b.

@hxy91819
Copy link
Copy Markdown
Member Author

Superseded by narrower Plan B PR (config apiKey preflight only; no aws-sdk/Bedrock expansion).

@hxy91819 hxy91819 closed this May 23, 2026
@hxy91819
Copy link
Copy Markdown
Member Author

Replacement PR: #85570

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling maintainer Maintainer-authored PR merge-risk: 🚨 auth-provider 🚨 May break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. size: L status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant