Greptile Summary

This PR refactors the built-in model suppression check in loadModelCatalog to call listManifestModelCatalogSuppressions once (via createManifestBuiltInModelSuppressionResolver) instead of once per catalog entry, eliminating 800+ redundant filesystem scans. The architecture of the factory/closure pattern is sound, but the new hot path silently drops normalizeProviderId alias resolution, which will cause suppressions to miss any catalog entry whose provider is stored as an alias (e.g., "bedrock", "modelstudio", "bytedance").

Confidence Score: 3/5

Not safe to merge as-is — the new fast path silently drops provider alias normalization, meaning suppression rules will fail for any aliased provider ID.

One P1 logic defect: the closure returned by createShouldSuppressBuiltInModel uses normalizeLowercaseStringOrEmpty instead of normalizeProviderId, losing alias→canonical mappings (bedrock→amazon-bedrock, modelstudio→qwen, etc.). Any catalog entry for an aliased provider will escape suppression silently. Score is capped at 4 by a single P1 and pulled to 3 because the defect is on the main cataloging path.

src/agents/model-suppression.ts (missing normalizeProviderId in new factory) and src/plugins/manifest-model-suppression.ts (resolver closure also uses plain lowercase normalization)

This is a comment left during a code review.
Path: src/agents/model-suppression.ts
Line: 85-87

Comment:
**Provider alias normalization dropped in new hot path**

The returned closure calls `normalizeLowercaseStringOrEmpty(input.provider)` inside `createManifestBuiltInModelSuppressionResolver`, but the original `shouldSuppressBuiltInModel` path went through `resolveBuiltInModelSuppressionFromManifest` → `normalizeProviderId(params.provider ?? "")`. `normalizeProviderId` maps several aliases to canonical names (e.g., `"bedrock"` → `"amazon-bedrock"`, `"modelstudio"` → `"qwen"`, `"bytedance"` → `"volcengine"`, `"z.ai"` → `"zai"`). Without this step, `buildModelCatalogMergeKey` will build the wrong key for any alias provider, and suppression entries stored under the canonical name will silently fail to match — leaving those models unsuppressed in the catalog.

```ts
  return (input) => {
    return resolver({ ...input, provider: normalizeProviderId(input.provider ?? "") })?.suppress ?? false;
  };
```
Or apply `normalizeProviderId` inside `createManifestBuiltInModelSuppressionResolver`.

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/plugins/manifest-model-suppression.ts
Line: 152-170

Comment:
**`resolveManifestBuiltInModelSuppression` now creates a fresh resolver on every call**

Before this PR, `resolveManifestBuiltInModelSuppression` called `listManifestModelCatalogSuppressions` directly (one filesystem scan per call). After the refactor it instantiates a `createManifestBuiltInModelSuppressionResolver` — which also calls `listManifestModelCatalogSuppressions` — and then immediately uses it once and discards it. The cost per call is identical to before; nothing is cached here. Any caller outside the new hot path (e.g., `buildSuppressedBuiltInModelError`, `shouldSuppressBuiltInModel`) still pays the full scan cost on each invocation. This is expected for single-item use, but worth documenting in a comment to prevent future confusion about which function actually caches.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "perf(catalog): cache manifest built-in m..." | Re-trigger Greptile}

Codex review: needs maintainer review before merge.

Keep this PR open. Current main still calls the single-item manifest suppression resolver for every model catalog entry, while this PR is an active source/test-only implementation candidate for building one reusable resolver per catalog load. The latest PR head also appears to address the earlier alias-normalization review concern, so the right next step is normal maintainer review and targeted validation, not cleanup closure.

Maintainer follow-up before merge:

Keep this PR open for normal maintainer review. The mergeable shape is the one this PR is aiming for: compute manifest suppressions once per loadModelCatalog call, preserve existing provider alias and conditional suppression behavior, keep single-item suppression behavior explicit, include focused cache and alias regression tests, then run targeted agent/plugin tests plus build or changed-gate validation before merge.

Best possible solution:

Keep this PR open for normal maintainer review. The mergeable shape is the one this PR is aiming for: compute manifest suppressions once per loadModelCatalog call, preserve existing provider alias and conditional suppression behavior, keep single-item suppression behavior explicit, include focused cache and alias regression tests, then run targeted agent/plugin tests plus build or changed-gate validation before merge.

Acceptance criteria:

• pnpm test src/plugins/manifest-model-suppression.test.ts src/agents/model-suppression.test.ts src/agents/model-catalog.test.ts src/agents/pi-embedded-runner/model.test.ts
• pnpm build
• pnpm check:changed

What I checked:

• Current main still suppresses inside the catalog loop: loadModelCatalog imports shouldSuppressBuiltInModel and calls it inside the registry-entry loop with config: cfg; there is no reusable per-load suppression resolver on current main. (src/agents/model-catalog.ts:152, 1c17fd5edf79)
• Manifest suppression lookup still scans per lookup on current main: resolveManifestBuiltInModelSuppression calls listManifestModelCatalogSuppressions during each single lookup; the current test explicitly expects two registry loads for two lookups. (src/plugins/manifest-model-suppression.ts:120, 1c17fd5edf79)
• Current main lacks the new cached-resolver API: Source search found shouldSuppressBuiltInModel and resolveManifestBuiltInModelSuppression, but no buildShouldSuppressBuiltInModel or buildManifestBuiltInModelSuppressionResolver symbols on current main; model-suppression.runtime.ts only exports the single-item wrapper. (src/agents/model-suppression.runtime.ts:1, 1c17fd5edf79)
• Provider alias and suppression contract: Current suppression normalizes provider aliases through normalizeProviderId; plugin manifest docs state modelCatalog.suppressions are honored for owned providers or declared aliases, so the PR must preserve this behavior while optimizing the hot path. (src/agents/provider-id.ts:3, 1c17fd5edf79)
• Latest PR head and diff surface: refs/pull/74236/head resolves to a2247a5aec6bb7bf92c189239da7f962576f9736. The public diff adds the reusable resolver API, normalizes provider/model ids in buildShouldSuppressBuiltInModel, adds cache/alias tests, and touches only TypeScript source/tests under src/agents and src/plugins; no workflows, dependency manifests, lockfiles, package scripts, release scripts, or secrets handling are changed. (a2247a5aec6b)
• Review discussion considered: The provided PR discussion includes Greptile/Copilot comments flagging a provider alias-normalization regression in the first head. The latest PR diff contains normalizeProviderId in the returned factory closure and focused tests for bedrock/aws-bedrock, so those comments are useful review context rather than a reason to close the PR. (src/agents/model-suppression.ts:69, a2247a5aec6b)

Likely related people:

• steipete: Recent commit history shows work on the same model catalog, manifest suppression, and plugin cache boundaries, including moving suppressions to manifests and sharing catalog capability lookup. (role: recent maintainer and refactor owner; confidence: high; commits: 947aae5a9920, 8c8dfa768a0d, 7a5b419843ec; files: src/agents/model-catalog.ts, src/agents/model-suppression.ts, src/plugins/manifest-model-suppression.ts)
• shakkernerd: History shows shakkernerd added the manifest model catalog planner, planned aliases/suppressions, and resolved model suppressions from manifests, which are the semantics this PR optimizes. (role: introduced manifest suppression and planner behavior; confidence: high; commits: dabdc779be8b, b2685e72c129, d014b36347ef; files: src/model-catalog/manifest-planner.ts, src/agents/model-suppression.ts, src/plugins/manifest-model-suppression.ts)
• vincentkoc: History shows adjacent Qwen conditional suppression and shared type seam work in the same area, and the PR timeline assigns Vincent for review; the latest PR head also includes a commit by Vincent preserving cached suppression semantics. (role: adjacent maintainer and reviewer; confidence: medium; commits: 058b57867e8c, 74e7b8d47b18, a2247a5aec6; files: src/plugins/manifest-model-suppression.ts, src/agents/model-catalog.ts, src/agents/model-suppression.ts)

Remaining risk / open question:

• No tests or build were run during this read-only sweep; the PR still needs targeted validation on the PR branch.
• The hot-path change must preserve provider alias normalization plus conditional baseUrl/provider API suppression semantics while reducing manifest registry loads.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 1c17fd5edf79.