perf: speed up provider-filtered models list#70632
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟠 Path traversal allows arbitrary module execution via providerDiscoveryEntry import
Description
This creates a path traversal / arbitrary local module import primitive:
An attacker who can place/modify a plugin manifest (notably a workspace plugin when Vulnerable code: providerDiscoverySource: params.manifest.providerDiscoveryEntry
? resolvePluginSourcePath(
path.resolve(params.candidate.rootDir, params.manifest.providerDiscoveryEntry),
)
: undefined,RecommendationEnforce that
Example fix: import { isPathInside, safeRealpathSync } from "./path-safety.js";
function resolveDiscoveryEntry(candidateRoot: string, entry: string): string {
if (path.isAbsolute(entry)) {
throw new Error("providerDiscoveryEntry must be relative");
}
const resolved = path.resolve(candidateRoot, entry);
const realRoot = safeRealpathSync(candidateRoot);
const realResolved = safeRealpathSync(resolved);
if (!isPathInside(realRoot, realResolved)) {
throw new Error("providerDiscoveryEntry must stay within plugin root");
}
return resolvePluginSourcePath(realResolved);
}Then use Analyzed PR: #70632 at commit Last updated on: 2026-04-24T04:48:16Z |
Greptile SummaryThis PR adds a static-catalog fast path for Confidence Score: 5/5Safe to merge; all findings are P2 style/behaviour notes that don't block correctness. No P0 or P1 defects found. The fast-path gating logic in hasProviderStaticCatalogForFilter, the discoveryEntriesOnly flag, and the registry-skip condition are all consistent. The two P2 comments (removed apiKey guard in shouldListConfiguredProviderModel and a dead seenKeys reassignment in the fallback branch) are minor and do not affect the primary use-cases targeted by this PR. src/commands/models/list.rows.ts — the apiKey guard removal is a silent behaviour change worth a second look before merging if strict listing semantics for partially-configured providers matter. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/commands/models/list.rows.ts
Line: 111
Comment:
**`apiKey` guard silently dropped for configured-provider rows**
Previously `shouldListConfiguredProviderModel` required `providerConfig.apiKey !== undefined` in addition to an `api` setting, so a provider entry that lacked `apiKey` was hidden from the list. That guard is now gone. Any configured provider block that declares an `api` (or per-model `api`) but has no `apiKey` — e.g. a partially-filled stub — will now appear as a visible row in `models list`, which may surprise users who never intended to surface it.
If the intent is to allow non-key auth mechanisms (e.g. header-based or env-var-based auth without an explicit `apiKey` field), consider tightening the guard to also accept other credential forms rather than removing the requirement entirely.
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/commands/models/list.row-sources.ts
Line: 47-53
Comment:
**Dead `seenKeys` reassignment in fast-path fallback**
When `catalogRows === 0` on the static fast path, `seenKeys` is reassigned to the return value of `appendDiscoveredRows`, but then the function immediately `return`s and `seenKeys` is never read again. The reassignment is dead code. Since `params.modelRegistry` is also `undefined` on this path (registry loading was skipped), `appendDiscoveredRows` always receives an empty array and adds nothing, so the fallback is effectively a no-op.
```suggestion
if (catalogRows === 0) {
appendDiscoveredRows({
rows: params.rows,
models: params.modelRegistry?.getAll() ?? [],
context: params.context,
});
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "fix: bound unscoped provider discovery f..." | Re-trigger Greptile |
![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: 2da7e94046
ℹ️ 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".
openclaw-barnacle
Bot
added
docs
openclaw-barnacle
Bot
removed
extensions: byteplus
extensions: volcengine
extensions: tencent
labels
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b1dacedab
ℹ️ 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".
shakkernerd
force-pushed
the
fix/models-list-auth-cost
branch
from
2b1dace to
daa2a05
Compare
shakkernerd
changed the title
openclaw-barnacle
Bot
added
extensions: byteplus
extensions: moonshot
extensions: volcengine
extensions: tencent
size: L
and removed
docs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daa2a05f8e
ℹ️ 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".
| }); | ||
|
|
||
| if (params.modelRegistry) { | ||
| if (params.modelRegistry && !params.context.filter.provider) { |
There was a problem hiding this comment.
Gating appendCatalogSupplementRows on !params.context.filter.provider removes model-catalog supplementation from all models list --all --provider <id> flows. That path is what pulls in fallback/synthetic entries via loadModelCatalog + resolveModelWithRegistry (in src/commands/models/list.rows.ts), so providers whose filtered results depend on catalog supplementation (for example when registry.getAll() is empty and static catalogs do not cover the provider) now silently lose rows and can print incomplete or empty filtered output.
Useful? React with 👍 / 👎.
shakkernerd
force-pushed
the
fix/models-list-auth-cost
branch
from
daa2a05 to
a0a4176
Compare
openclaw-barnacle
Bot
added
docs
|
Landed via temp rebase onto current main.
Thanks @shakkernerd! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0a41769f5
ℹ️ 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".
| const scopedPluginIds = pluginIds.filter((pluginId) => bundledPluginIdSet.has(pluginId)); | ||
| if (scopedPluginIds.length === 0) { |
There was a problem hiding this comment.
When a provider id is owned by both a workspace plugin and a bundled plugin, resolveProviderCatalogPluginIdsForFilter can return both ids, but this logic keeps only bundled ids and still enables the static fast path. That makes models list --all --provider <id> skip registry loading and silently ignore the workspace plugin’s runtime catalog/availability behavior, showing bundled static rows instead. The fast-path gate should only pass when all owning plugin ids are bundled (or when ownership is unambiguous).
Useful? React with 👍 / 👎.
1 participant
This PR makes
models list --all --provider <id>avoid the broad model registry path when the filtered provider has a safe lightweight static catalog.Before this change, provider-filtered listing still paid much of the same cost as broad
models list --all: registry loading, auth/model discovery, and provider catalog work could run even when the command only needed static rows for one provider. That made simple provider checks unnecessarily slow.What changed:
models list --all --provider <id>.Provider behavior:
moonshot,deepseek,byteplus,volcengine,tencent-tokenhub,codex.kilocode,chutes,vercel-ai-gateway,arcee,openrouter.Measured with direct built CLI (
node openclaw.mjs ... --json):Before, representative provider-filtered commands were still on the broad slow path:
moonshot: ~20s class in debug timing before provider-static fast path work.openrouter: ~20s+ class before earlier scoped registry work.After this PR:
moonshot: 1.13s, 5 rowsdeepseek: 1.08s, 2 rowsbyteplus: 1.09s, 3 rowsvolcengine: 1.08s, 5 rowstencent-tokenhub: 1.07s, 1 rowcodex: 1.09s, 3 rowsPreserved registry/live path timings:
kilocode: 10.08s, 1 rowchutes: 8.68s, 47 rowsvercel-ai-gateway: 13.08s, 156 rowsarcee: 5.16s, 0 rows in my local envopenrouter: 14.16s, 254 rowsBroad baseline remains slow and is not solved by this PR:
models list --all: 30.03s, 937 rowsWhy some providers stay on the registry path:
kilocode,chutes, andvercel-ai-gatewayhave live/account-specific discovery or fallback catalogs where static rows are not complete enough.arceesupports OpenRouter-backed auth under the samearcee/*refs, so static listing would incorrectly mark rows unavailable.openrouterstill depends on broader registry/catalog loading.This is intentionally an incremental safe path. The follow-up architecture work should move provider display catalogs, auth availability metadata, and live/account discovery into separate declared lanes so all provider-list commands can become fast without lying about availability or missing account-specific models.
Verification:
pnpm test src/plugins/provider-discovery.runtime.test.ts src/commands/models/list.provider-catalog.test.ts src/commands/models/list.list-command.forward-compat.test.ts src/commands/models.list.e2e.test.tsmodels list --all --provider <id> --jsonon the providers listed above.