fix(agents): honor models.mode='replace' by skipping implicit provider discovery#68978
fix(agents): honor models.mode='replace' by skipping implicit provider discovery#68978hclsys wants to merge 1 commit into
Conversation
Greptile SummaryAdds a guard in Confidence Score: 5/5Safe to merge; the fix is correct and well-tested with only minor P2 style nits in the test file. The production code change is a two-line early-return that directly implements the documented contract. The existing mergeProviders semantics are preserved for the default and merge paths. All remaining findings are P2 style suggestions in the test file. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/models-config.mode-replace.test.ts
Line: 4
Comment:
**Indirect import path for `ProviderConfig`**
The test imports `ProviderConfig` from `"./models-config.providers.secrets.js"`, which re-exports it from `models-config.providers.secret-helpers.js`. The production code (`models-config.plan.ts`) sources it from `"./models-config.providers.js"`. If these two re-export chains ever diverge, the test fixture types could silently drift from the actual interface under test. Prefer the same import path used in the production file.
```suggestion
import type { ProviderConfig } from "./models-config.providers.js";
```
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/models-config.mode-replace.test.ts
Line: 89
Comment:
**Inconsistent key-ordering assertion**
The other two test cases use `.toSorted()` to make key-order comparisons stable, but this assertion relies on implicit insertion order. For a single-entry object it's harmless today, but it's inconsistent with the pattern established by the other cases in this `describe` block.
```suggestion
expect(Object.keys(providers).toSorted()).toEqual(["bailian"]);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(agents): honor models.mode='replace'..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { describe, expect, it, vi } from "vitest"; | ||
| import type { OpenClawConfig } from "../config/config.js"; | ||
| import { resolveProvidersForModelsJsonWithDeps } from "./models-config.plan.js"; | ||
| import type { ProviderConfig } from "./models-config.providers.secrets.js"; |
There was a problem hiding this comment.
ProviderConfig
The test imports ProviderConfig from "./models-config.providers.secrets.js", which re-exports it from models-config.providers.secret-helpers.js. The production code (models-config.plan.ts) sources it from "./models-config.providers.js". If these two re-export chains ever diverge, the test fixture types could silently drift from the actual interface under test. Prefer the same import path used in the production file.
| import type { ProviderConfig } from "./models-config.providers.secrets.js"; | |
| import type { ProviderConfig } from "./models-config.providers.js"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/models-config.mode-replace.test.ts
Line: 4
Comment:
**Indirect import path for `ProviderConfig`**
The test imports `ProviderConfig` from `"./models-config.providers.secrets.js"`, which re-exports it from `models-config.providers.secret-helpers.js`. The production code (`models-config.plan.ts`) sources it from `"./models-config.providers.js"`. If these two re-export chains ever diverge, the test fixture types could silently drift from the actual interface under test. Prefer the same import path used in the production file.
```suggestion
import type { ProviderConfig } from "./models-config.providers.js";
```
How can I resolve this? If you propose a fix, please make it concise.| // The fix: implicit fetch is skipped entirely, and the result contains | ||
| // ONLY the explicit providers from config. | ||
| expect(resolveImplicit).not.toHaveBeenCalled(); | ||
| expect(Object.keys(providers)).toEqual(["bailian"]); |
There was a problem hiding this comment.
The other two test cases use .toSorted() to make key-order comparisons stable, but this assertion relies on implicit insertion order. For a single-entry object it's harmless today, but it's inconsistent with the pattern established by the other cases in this describe block.
| expect(Object.keys(providers)).toEqual(["bailian"]); | |
| expect(Object.keys(providers).toSorted()).toEqual(["bailian"]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/models-config.mode-replace.test.ts
Line: 89
Comment:
**Inconsistent key-ordering assertion**
The other two test cases use `.toSorted()` to make key-order comparisons stable, but this assertion relies on implicit insertion order. For a single-entry object it's harmless today, but it's inconsistent with the pattern established by the other cases in this `describe` block.
```suggestion
expect(Object.keys(providers).toSorted()).toEqual(["bailian"]);
```
How can I resolve this? If you propose a fix, please make it concise.
…r discovery resolveProvidersForModelsJsonWithDeps() always fetched implicit providers from bundled plugin catalog hooks, then merged them with the explicit providers from config. This meant `models.mode: "replace"` — documented as excluding implicit providers — still emitted implicit entries (e.g. `codex` from the bundled codex plugin) into the generated models.json. When mode is 'replace', skip the implicit fetch entirely and return only the explicit providers. 'merge' and default unset behavior are unchanged. Fixes openclaw#68965
This comment was marked as low quality.
This comment was marked as low quality.
1 participant
Problem
`resolveProvidersForModelsJsonWithDeps` in `src/agents/models-config.plan.ts` always invoked `resolveImplicitProviders` and merged the result with the explicit providers from config. That meant `models.mode: "replace"` — documented as 'only use explicitly configured providers' — still pulled in implicit providers contributed by bundled plugin `catalog` hooks.
Reporter @Tony-ooo (#68965) traced this to the `codex` bundled plugin, which defines a `catalog` hook and ends up in `models.json` even when `mode: "replace"` is set. Other bundled plugins like `anthropic` aren't affected because they don't define a `catalog` hook, which is why the symptom looks codex-specific.
Fix
In `resolveProvidersForModelsJsonWithDeps`, check `cfg.models?.mode` before the implicit fetch. When it's `"replace"`, skip `resolveImplicitProviders` entirely and return a `mergeProviders` result with `implicit: {}`. Default (undefined) and `"merge"` paths are unchanged.
This is a single edit at the one source of truth. Downstream `mergeProviders` keeps the same explicit-wins semantics.
Testing
Added `src/agents/models-config.mode-replace.test.ts` with three cases:
`tsc --noEmit` and `oxlint` clean on the touched files.
Fixes #68965