fix(codex): avoid image tool loops on vision turns and respect image-model providers#65061
fix(codex): avoid image tool loops on vision turns and respect image-model providers#65061zhulijin1991 wants to merge 2 commits into
Conversation
openclaw-barnacle
Bot
added
agents
Greptile SummaryThis PR fixes two related bugs: it hides the dynamic Confidence Score: 5/5This PR is safe to merge; both fixes are correct and targeted, with no P0 or P1 findings. Both the vision-tool filtering and the provider-inheritance fix are logically sound. The only note is that resolveImageFallbackDefaultProvider also affects bare strings in the configured imageModel.fallbacks array (not just modelOverride), but this is almost certainly the right behaviour and there is no evidence of a real regression path. All remaining observations are P2. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/model-fallback.ts
Line: 374-391
Comment:
**Side-effect on bare fallback IDs**
`resolveImageFallbackDefaultProvider` is passed as `defaultProvider` to the entire `resolveImageFallbackCandidates` call, so it affects not only the `modelOverride` path but also any bare (provider-less) strings in `agents.defaults.imageModel.fallbacks`. Before this change, those bare fallbacks resolved under `DEFAULT_PROVIDER`; after the change they resolve under the primary image model's provider.
For a config like `primary: "openai-codex/gpt-5.4", fallbacks: ["gpt-4o"]`, `"gpt-4o"` will now resolve to `openai-codex/gpt-4o` instead of the previous `openai/gpt-4o`. This is likely the intended behaviour (consistent provider across the image model group), but the PR description only mentions the `modelOverride` path and the existing test exercises fully-qualified fallback strings, so the bare-fallback case is untested. A test or a note in the description would confirm the intent.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(image): respect configured provider ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function resolveImageFallbackDefaultProvider(cfg: OpenClawConfig | undefined): string { | ||
| const configuredPrimary = resolveAgentModelPrimaryValue(cfg?.agents?.defaults?.imageModel); | ||
| if (configuredPrimary?.trim()) { | ||
| const aliasIndex = buildModelAliasIndex({ | ||
| cfg: cfg ?? {}, | ||
| defaultProvider: DEFAULT_PROVIDER, | ||
| }); | ||
| const resolved = resolveModelRefFromString({ | ||
| raw: configuredPrimary, | ||
| defaultProvider: DEFAULT_PROVIDER, | ||
| aliasIndex, | ||
| }); | ||
| if (resolved?.ref.provider) { | ||
| return resolved.ref.provider; | ||
| } | ||
| } | ||
| return DEFAULT_PROVIDER; | ||
| } |
There was a problem hiding this comment.
resolveImageFallbackDefaultProvider is passed as defaultProvider to the entire resolveImageFallbackCandidates call, so it affects not only the modelOverride path but also any bare (provider-less) strings in agents.defaults.imageModel.fallbacks. Before this change, those bare fallbacks resolved under DEFAULT_PROVIDER; after the change they resolve under the primary image model's provider.
For a config like primary: "openai-codex/gpt-5.4", fallbacks: ["gpt-4o"], "gpt-4o" will now resolve to openai-codex/gpt-4o instead of the previous openai/gpt-4o. This is likely the intended behaviour (consistent provider across the image model group), but the PR description only mentions the modelOverride path and the existing test exercises fully-qualified fallback strings, so the bare-fallback case is untested. A test or a note in the description would confirm the intent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-fallback.ts
Line: 374-391
Comment:
**Side-effect on bare fallback IDs**
`resolveImageFallbackDefaultProvider` is passed as `defaultProvider` to the entire `resolveImageFallbackCandidates` call, so it affects not only the `modelOverride` path but also any bare (provider-less) strings in `agents.defaults.imageModel.fallbacks`. Before this change, those bare fallbacks resolved under `DEFAULT_PROVIDER`; after the change they resolve under the primary image model's provider.
For a config like `primary: "openai-codex/gpt-5.4", fallbacks: ["gpt-4o"]`, `"gpt-4o"` will now resolve to `openai-codex/gpt-4o` instead of the previous `openai/gpt-4o`. This is likely the intended behaviour (consistent provider across the image model group), but the PR description only mentions the `modelOverride` path and the existing test exercises fully-qualified fallback strings, so the bare-fallback case is untested. A test or a note in the description would confirm the intent.
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: e6882a29ba
ℹ️ 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 visionFilteredTools = filterToolsForVisionInputs(allTools, { | ||
| modelHasVision, | ||
| hasInboundImages: (params.images?.length ?? 0) > 0, | ||
| }); |
There was a problem hiding this comment.
Filtering the tool list by params.images makes the dynamic tool catalog vary per turn, but thread resume logic treats any catalog fingerprint change as incompatible and starts a new thread (extensions/codex/src/app-server/thread-lifecycle.ts). In sessions that alternate between image and non-image prompts, this will repeatedly invalidate thread binding and lose prior conversation context, which is a behavioral regression introduced by this change.
Useful? React with 👍 / 👎.
|
Thanks @zhulijin1991. I rebased and landed this as replacement PR #69551 because this fork is not maintainer-editable and the original branch conflicted with current |
|
Replacement landed via #69551. Main commit: Thanks again @zhulijin1991. |
2 participants
Summary
imagetool on Codex-native turns that already include inbound images and use a vision-capable modelagents.defaults.imageModel.primaryprovider when resolving bare image-model override idsProblem
Codex-native image turns can stall when inbound images are already present in the turn input but the dynamic
imagetool is still exposed. Separately, bare overrides such asgpt-5.4-minican resolve under the wrong provider when the configured image model lives under a non-default provider prefix.Closes #65050
Testing
pnpm exec node --no-maglev node_modules/vitest/vitest.mjs run --config test/vitest/vitest.extensions.config.ts extensions/codex/src/app-server/run-attempt.vision-tools.test.tspnpm exec node --no-maglev node_modules/vitest/vitest.mjs run --config test/vitest/vitest.agents.config.ts src/agents/model-fallback.image-provider.test.tspnpm exec node --no-maglev node_modules/vitest/vitest.mjs run --config test/vitest/vitest.extensions.config.ts extensions/codex/src/app-server/run-attempt.test.tspnpm exec node --no-maglev node_modules/vitest/vitest.mjs run --config test/vitest/vitest.agents.config.ts src/agents/model-fallback.test.ts