fix(tools): normalize model reference before registry lookup#66165
fix(tools): normalize model reference before registry lookup#66165yqli2420 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4686f6f48
ℹ️ 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".
| return requireApiKey({ | ||
| provider: params.provider, | ||
| modelId: params.modelId, | ||
| agentDir: params.agentDir, | ||
| }); |
There was a problem hiding this comment.
Resolve provider auth before calling
requireApiKey
resolveModelRuntimeApiKey now calls requireApiKey with { provider, modelId, agentDir }, but requireApiKey expects a resolved auth object containing apiKey (from getApiKeyForModel) and a provider string. In the PDF tool path (runPdfPrompt), this means API key resolution is skipped and the call will always throw (No API key resolved...) even when credentials are configured, so PDF analysis fails for all providers.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR correctly adds
Confidence Score: 1/5Not safe to merge — the refactored Two P0 defects: (1)
|
| resolveModelFromRegistry(params); | ||
| return requireApiKey({ | ||
| provider: params.provider, | ||
| modelId: params.modelId, | ||
| agentDir: params.agentDir, | ||
| }); |
There was a problem hiding this comment.
requireApiKey
The requireApiKey helper expects (resolvedProviderAuth, providerName). Here it is invoked with a single object { provider, modelId, agentDir } that does not match ResolvedProviderAuth — the source and mode fields are missing. This will throw at runtime because the resolved value cannot be extracted. The second positional argument is also omitted.
On top of this, getApiKeyForModel (which resolves credentials through profiles/env/config) and setRuntimeApiKey from the old implementation have been dropped with no replacement, so no real credential is ever resolved by this path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/tools/media-tool-shared.ts
Line: 418-423
Comment:
**Incorrect call to `requireApiKey`**
The `requireApiKey` helper expects `(resolvedProviderAuth, providerName)`. Here it is invoked with a single object `{ provider, modelId, agentDir }` that does not match `ResolvedProviderAuth` — the `source` and `mode` fields are missing. This will throw at runtime because the resolved value cannot be extracted. The second positional argument is also omitted.
On top of this, `getApiKeyForModel` (which resolves credentials through profiles/env/config) and `setRuntimeApiKey` from the old implementation have been dropped with no replacement, so no real credential is ever resolved by this path.
How can I resolve this? If you propose a fix, please make it concise.|
Thanks for chasing this. The core normalization fix is valid, but the branch also refactored the media-tool auth path in a way that breaks the PDF tool. I carried the safe version forward as #66422 on top of current main, with the lookup fix isolated, coverage added, and changelog wired, so I’m closing this PR in favor of the maintainer carry. |
Summary
Describe the problem and fix in 2–5 bullets:
imageandpdftools failed with "Unknown model" errors for configured Ollama models because they performed registry lookups using raw, non-normalized model references.resolveModelFromRegistryto normalize provider/model IDs usingnormalizeModelRefbefore lookup. RefactoredresolveModelRuntimeApiKeyto accept normalized references and simplify API key retrieval.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
The
imagetool now successfully processes requests using configured Ollama models (e.g.,ollama/qwen3.5:397b-cloud) instead of returning an "Unknown model" error.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
agents.defaults.imageModel.primaryset toollama/qwen3.5:397b-cloudSteps
agents.defaults.imageModel.primaryto an Ollama model with image support (e.g.,ollama/qwen3.5:397b-cloud).openclaw models status.imagetool with a prompt and image URL.Expected
The tool analyzes the image using the configured model and returns a description.
Actual
The tool returns a successful response with the image analysis (previously returned
{"status": "error", "tool": "image", "error": "Unknown model: ..."}).Evidence
Human Verification (required)
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
src/agents/tools/media-tool-shared.ts.src/agents/tools/media-tool-shared.ts.normalizeModelRefbehaves unexpectedly, valid models might fail to resolve.Risks and Mitiggedations
The change relies on the
normalizeModelRefutility. If this utility handles edge cases differently than the previous raw lookup, it could theoretically affect resolution. However, since it is a shared utility imported frommodel-selection.js, it aligns behavior with the rest of the system and reduces inconsistency.