Skip to content

Commit d33c3f7

Browse files
mednsvincentkoc
medns
and
vincentkoc
authored
perf(catalog): cache manifest built-in model suppression resolver (#74236)
* perf(catalog): cache manifest built-in model suppression resolver * fix(catalog): address PR review comments for manifest suppression resolver * fix(catalog): preserve cached suppression semantics --------- Co-authored-by: Vincent Koc <[email protected]>
1 parent b521974 commit d33c3f7

7 files changed

Lines changed: 205 additions & 34 deletions

‎src/agents/model-catalog.test.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,11 @@ vi.mock("./model-suppression.runtime.js", () => ({
1919
params.provider === "azure-openai-responses" ||
2020
params.provider === "openai-codex") &&
2121
params.id === "gpt-5.3-codex-spark",
22+
buildShouldSuppressBuiltInModel: () => (params: { provider?: string; id?: string }) =>
23+
(params.provider === "openai" ||
24+
params.provider === "azure-openai-responses" ||
25+
params.provider === "openai-codex") &&
26+
params.id === "gpt-5.3-codex-spark",
2227
}));
2328

2429
function mockCatalogImportFailThenRecover() {

‎src/agents/model-catalog.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ export async function loadModelCatalog(params?: {
149149
const piSdk = await importPiSdk();
150150
logStage("pi-sdk-imported");
151151
const agentDir = resolveOpenClawAgentDir();
152-
const { shouldSuppressBuiltInModel } = await loadModelSuppression();
152+
const { buildShouldSuppressBuiltInModel } = await loadModelSuppression();
153153
logStage("catalog-deps-ready");
154154
const authStorage = piSdk.discoverAuthStorage(
155155
agentDir,
@@ -164,6 +164,10 @@ export async function loadModelCatalog(params?: {
164164
logStage("registry-ready");
165165
const entries = Array.isArray(registry) ? registry : registry.getAll();
166166
logStage("registry-read", `entries=${entries.length}`);
167+
168+
const shouldSuppressBuiltInModel = buildShouldSuppressBuiltInModel({ config: cfg });
169+
logStage("suppress-resolver-ready");
170+
167171
for (const entry of entries) {
168172
const id = normalizeOptionalString(entry?.id) ?? "";
169173
if (!id) {
@@ -173,7 +177,7 @@ export async function loadModelCatalog(params?: {
173177
if (!provider) {
174178
continue;
175179
}
176-
if (shouldSuppressBuiltInModel({ provider, id, config: cfg })) {
180+
if (shouldSuppressBuiltInModel({ provider, id })) {
177181
continue;
178182
}
179183
const name = normalizeOptionalString(entry?.name ?? id) || id;

‎src/agents/model-suppression.runtime.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,21 @@
1-
import { shouldSuppressBuiltInModel as shouldSuppressBuiltInModelImpl } from "./model-suppression.js";
1+
import {
2+
buildShouldSuppressBuiltInModel as buildShouldSuppressBuiltInModelImpl,
3+
shouldSuppressBuiltInModel as shouldSuppressBuiltInModelImpl,
4+
} from "./model-suppression.js";
25

36
type ShouldSuppressBuiltInModel =
47
typeof import("./model-suppression.js").shouldSuppressBuiltInModel;
8+
type BuildShouldSuppressBuiltInModel =
9+
typeof import("./model-suppression.js").buildShouldSuppressBuiltInModel;
510

611
export function shouldSuppressBuiltInModel(
712
...args: Parameters<ShouldSuppressBuiltInModel>
813
): ReturnType<ShouldSuppressBuiltInModel> {
914
return shouldSuppressBuiltInModelImpl(...args);
1015
}
16+
17+
export function buildShouldSuppressBuiltInModel(
18+
...args: Parameters<BuildShouldSuppressBuiltInModel>
19+
): ReturnType<BuildShouldSuppressBuiltInModel> {
20+
return buildShouldSuppressBuiltInModelImpl(...args);
21+
}

‎src/agents/model-suppression.test.ts‎

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,23 @@
11
import { beforeEach, describe, expect, it, vi } from "vitest";
22

33
const mocks = vi.hoisted(() => ({
4+
buildManifestBuiltInModelSuppressionResolver: vi.fn(),
45
resolveManifestBuiltInModelSuppression: vi.fn(),
56
}));
67

78
vi.mock("../plugins/manifest-model-suppression.js", () => ({
9+
buildManifestBuiltInModelSuppressionResolver: mocks.buildManifestBuiltInModelSuppressionResolver,
810
resolveManifestBuiltInModelSuppression: mocks.resolveManifestBuiltInModelSuppression,
911
}));
1012

11-
import { shouldSuppressBuiltInModel } from "./model-suppression.js";
13+
import {
14+
buildShouldSuppressBuiltInModel,
15+
shouldSuppressBuiltInModel,
16+
} from "./model-suppression.js";
1217

1318
describe("model suppression", () => {
1419
beforeEach(() => {
20+
mocks.buildManifestBuiltInModelSuppressionResolver.mockReset();
1521
mocks.resolveManifestBuiltInModelSuppression.mockReset();
1622
});
1723

@@ -43,4 +49,48 @@ describe("model suppression", () => {
4349

4450
expect(mocks.resolveManifestBuiltInModelSuppression).toHaveBeenCalledOnce();
4551
});
52+
53+
describe("buildShouldSuppressBuiltInModel", () => {
54+
beforeEach(() => {
55+
mocks.buildManifestBuiltInModelSuppressionResolver.mockReset();
56+
});
57+
58+
it("creates a reusable manifest resolver with normalized provider and model ids", () => {
59+
const resolver = vi
60+
.fn()
61+
.mockReturnValueOnce({ suppress: true, errorMessage: "manifest suppression" })
62+
.mockReturnValueOnce(undefined);
63+
const config = {};
64+
mocks.buildManifestBuiltInModelSuppressionResolver.mockReturnValueOnce(resolver);
65+
66+
const shouldSuppress = buildShouldSuppressBuiltInModel({ config });
67+
68+
expect(shouldSuppress({ provider: "bedrock", id: "Claude-3" })).toBe(true);
69+
expect(shouldSuppress({ provider: "aws-bedrock", id: "claude-4" })).toBe(false);
70+
expect(mocks.buildManifestBuiltInModelSuppressionResolver).toHaveBeenCalledOnce();
71+
expect(mocks.buildManifestBuiltInModelSuppressionResolver).toHaveBeenCalledWith({
72+
config,
73+
env: process.env,
74+
});
75+
expect(resolver).toHaveBeenNthCalledWith(1, {
76+
provider: "amazon-bedrock",
77+
id: "claude-3",
78+
});
79+
expect(resolver).toHaveBeenNthCalledWith(2, {
80+
provider: "amazon-bedrock",
81+
id: "claude-4",
82+
});
83+
});
84+
85+
it("does not call the manifest resolver for empty provider or model ids", () => {
86+
const resolver = vi.fn();
87+
mocks.buildManifestBuiltInModelSuppressionResolver.mockReturnValueOnce(resolver);
88+
89+
const shouldSuppress = buildShouldSuppressBuiltInModel({});
90+
91+
expect(shouldSuppress({ provider: "openai", id: "" })).toBe(false);
92+
expect(shouldSuppress({ provider: "", id: "gpt-5.5" })).toBe(false);
93+
expect(resolver).not.toHaveBeenCalled();
94+
});
95+
});
4696
});

‎src/agents/model-suppression.ts‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import type { OpenClawConfig } from "../config/types.openclaw.js";
2-
import { resolveManifestBuiltInModelSuppression } from "../plugins/manifest-model-suppression.js";
2+
import {
3+
buildManifestBuiltInModelSuppressionResolver,
4+
resolveManifestBuiltInModelSuppression,
5+
} from "../plugins/manifest-model-suppression.js";
36
import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js";
47
import { normalizeProviderId } from "./provider-id.js";
58

@@ -66,3 +69,21 @@ export function buildSuppressedBuiltInModelError(params: {
6669
}): string | undefined {
6770
return resolveBuiltInModelSuppression(params)?.errorMessage;
6871
}
72+
73+
export function buildShouldSuppressBuiltInModel(params: {
74+
config?: OpenClawConfig;
75+
}): (input: { provider?: string | null; id?: string | null; baseUrl?: string | null }) => boolean {
76+
const resolver = buildManifestBuiltInModelSuppressionResolver({
77+
config: params.config,
78+
env: process.env,
79+
});
80+
81+
return (input) => {
82+
const provider = normalizeProviderId(input.provider ?? "");
83+
const id = normalizeLowercaseStringOrEmpty(input.id);
84+
if (!provider || !id) {
85+
return false;
86+
}
87+
return resolver({ ...input, provider, id })?.suppress ?? false;
88+
};
89+
}

‎src/plugins/manifest-model-suppression.test.ts‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ vi.mock("./plugin-registry.js", () => ({
99
}));
1010

1111
import {
12+
buildManifestBuiltInModelSuppressionResolver,
1213
clearManifestModelSuppressionCacheForTest,
1314
resolveManifestBuiltInModelSuppression,
1415
} from "./manifest-model-suppression.js";
@@ -46,6 +47,30 @@ describe("manifest model suppression", () => {
4647
});
4748
});
4849

50+
describe("buildManifestBuiltInModelSuppressionResolver", () => {
51+
it("reads planned manifest suppressions once per resolver creation", () => {
52+
const config = { plugins: { entries: { openai: { enabled: true } } } };
53+
54+
const resolver = buildManifestBuiltInModelSuppressionResolver({
55+
config,
56+
env: process.env,
57+
});
58+
59+
expect(mocks.loadPluginManifestRegistryForPluginRegistry).toHaveBeenCalledTimes(1);
60+
61+
resolver({
62+
provider: "azure-openai-responses",
63+
id: "gpt-5.3-codex-spark",
64+
});
65+
resolver({
66+
provider: "azure-openai-responses",
67+
id: "gpt-5.3-codex-spark",
68+
});
69+
70+
expect(mocks.loadPluginManifestRegistryForPluginRegistry).toHaveBeenCalledTimes(1);
71+
});
72+
});
73+
4974
it("resolves manifest suppressions for declared provider aliases", () => {
5075
expect(
5176
resolveManifestBuiltInModelSuppression({
@@ -89,6 +114,29 @@ describe("manifest model suppression", () => {
89114
expect(mocks.loadPluginManifestRegistryForPluginRegistry).toHaveBeenCalledTimes(2);
90115
});
91116

117+
it("reuses planned manifest suppressions inside a resolver instance", () => {
118+
const config = { plugins: { entries: { openai: { enabled: true } } } };
119+
120+
const resolver = buildManifestBuiltInModelSuppressionResolver({
121+
config,
122+
env: process.env,
123+
});
124+
125+
expect(
126+
resolver({
127+
provider: "azure-openai-responses",
128+
id: "gpt-5.3-codex-spark",
129+
})?.suppress,
130+
).toBe(true);
131+
expect(
132+
resolver({
133+
provider: "azure-openai-responses",
134+
id: "gpt-4.1",
135+
}),
136+
).toBeUndefined();
137+
expect(mocks.loadPluginManifestRegistryForPluginRegistry).toHaveBeenCalledTimes(1);
138+
});
139+
92140
it("matches conditional suppressions by base URL host", () => {
93141
mocks.loadPluginManifestRegistryForPluginRegistry.mockReturnValue({
94142
diagnostics: [],

‎src/plugins/manifest-model-suppression.ts‎

Lines changed: 61 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -103,43 +103,75 @@ export function clearManifestModelSuppressionCacheForTest(): void {
103103
// Manifest suppressions are read fresh. Keep the test hook as a no-op.
104104
}
105105

106-
export function resolveManifestBuiltInModelSuppression(params: {
107-
provider?: string | null;
108-
id?: string | null;
106+
export function buildManifestBuiltInModelSuppressionResolver(params: {
109107
config?: OpenClawConfig;
110108
workspaceDir?: string;
111109
env?: NodeJS.ProcessEnv;
112-
baseUrl?: string | null;
113110
}) {
114-
const provider = normalizeLowercaseStringOrEmpty(params.provider);
115-
const modelId = normalizeLowercaseStringOrEmpty(params.id);
116-
if (!provider || !modelId) {
117-
return undefined;
118-
}
119-
const mergeKey = buildModelCatalogMergeKey(provider, modelId);
120-
const suppression = listManifestModelCatalogSuppressions({
111+
const suppressions = listManifestModelCatalogSuppressions({
121112
config: params.config,
122113
workspaceDir: params.workspaceDir,
123114
env: params.env ?? process.env,
124-
}).find(
125-
(entry) =>
126-
entry.mergeKey === mergeKey &&
127-
manifestSuppressionMatchesConditions({
128-
suppression: entry,
115+
});
116+
117+
return (input: {
118+
provider?: string | null;
119+
id?: string | null;
120+
baseUrl?: string | null;
121+
}) => {
122+
const provider = normalizeLowercaseStringOrEmpty(input.provider);
123+
const modelId = normalizeLowercaseStringOrEmpty(input.id);
124+
if (!provider || !modelId) {
125+
return undefined;
126+
}
127+
const mergeKey = buildModelCatalogMergeKey(provider, modelId);
128+
const suppression = suppressions.find(
129+
(entry) =>
130+
entry.mergeKey === mergeKey &&
131+
manifestSuppressionMatchesConditions({
132+
suppression: entry,
133+
provider,
134+
baseUrl: input.baseUrl,
135+
config: params.config,
136+
}),
137+
);
138+
if (!suppression) {
139+
return undefined;
140+
}
141+
return {
142+
suppress: true,
143+
errorMessage: buildManifestSuppressionError({
129144
provider,
130-
baseUrl: params.baseUrl,
131-
config: params.config,
145+
modelId,
146+
reason: suppression.reason,
132147
}),
133-
);
134-
if (!suppression) {
135-
return undefined;
136-
}
137-
return {
138-
suppress: true,
139-
errorMessage: buildManifestSuppressionError({
140-
provider,
141-
modelId,
142-
reason: suppression.reason,
143-
}),
148+
};
144149
};
145150
}
151+
152+
/**
153+
* Resolves whether a built-in model should be suppressed based on manifest declarations.
154+
*
155+
* Note: This function instantiates a fresh resolver on every call, which incurs a full
156+
* filesystem scan of the manifest registry. For hot paths (like building the model catalog),
157+
* instantiate and reuse `buildManifestBuiltInModelSuppressionResolver` instead.
158+
*/
159+
export function resolveManifestBuiltInModelSuppression(params: {
160+
provider?: string | null;
161+
id?: string | null;
162+
config?: OpenClawConfig;
163+
workspaceDir?: string;
164+
env?: NodeJS.ProcessEnv;
165+
baseUrl?: string | null;
166+
}) {
167+
const resolver = buildManifestBuiltInModelSuppressionResolver({
168+
config: params.config,
169+
workspaceDir: params.workspaceDir,
170+
env: params.env,
171+
});
172+
return resolver({
173+
provider: params.provider,
174+
id: params.id,
175+
baseUrl: params.baseUrl,
176+
});
177+
}

0 commit comments

Comments
 (0)