Skip to content

fix(providers): reject reserved provider prefixes on compatible-node create/update - #11375

Closed
ggdayup wants to merge 10 commits into
diegosouzapw:release/v3.8.50from
ggdayup:fix/reserved-prefix-guard-validation
Closed

ggdayup wants to merge 10 commits into
diegosouzapw:release/v3.8.50from
ggdayup:fix/reserved-prefix-guard-validation

Conversation

@ggdayup

@ggdayup ggdayup commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Compatible provider nodes (custom OpenAI-compatible endpoints) currently accept a prefix that collides with a built-in registry id or alias (e.g. tokenrouter). The node saves fine, but every request shaped <prefix>/<model> is intercepted by the built-in provider lookup in getModelInfo(), so the node is silently unreachable — while the identical request against the upstream API directly succeeds. Users see No active credentials for provider: tokenrouter (model_not_found) and have no way to discover why.

This PR rejects such prefixes at write time with an explicit validation error, so the misconfiguration surfaces at creation instead of as a runtime routing dead-end.

Root cause

  • src/sse/services/model.ts::getModelInfo() intentionally skips compatible-node lookup when the model prefix matches a built-in REGISTRY id/alias (built-ins must win).
  • The create/update schemas (createProviderNodeSchema / updateProviderNodeSchema in src/shared/validation/schemas/provider.ts) never mirrored that guard, allowing nodes whose prefix can never be routed to.

Fix

  • New shared module src/shared/constants/reservedProviderPrefixes.ts — single source of truth built from the live REGISTRY (all entry ids + aliases, 391 unique prefixes today). Exposes getReservedProviderPrefixes(), isReservedProviderPrefix() and a friendly reservedProviderPrefixMessage().
  • createProviderNodeSchema: superRefine now adds a Zod issue on prefix when it is reserved → route returns 400 with a per-field message.
  • updateProviderNodeSchema: same refinement, so renaming a node's prefix to a reserved one is also rejected.
  • src/sse/services/model.ts: the runtime guard now consumes the shared module (behavior byte-for-byte unchanged; removes the duplicated local set builder).

Semantics are case-sensitive exact match, mirroring the runtime guard: mixed-case values like TokenRouter do not intercept nodes at runtime and remain allowed; manual aliases outside REGISTRY (xiaomi, llamacpp, aq, …) also don't intercept and remain allowed.

TDD evidence (RED → GREEN)

  1. RED: wrote tests/unit/provider-node-reserved-prefix.test.ts against the unfixed schemas — reserved-prefix cases failed while non-reserved controls passed.
  2. GREEN after fix: full suite passes.
node --import tsx/esm --test tests/unit/provider-node-reserved-prefix.test.ts
# pass 14 / fail 0   (shared-module semantics x5, schema-level x6,
#                     POST /api/provider-nodes 400 + happy path, PUT rename guard)

Regression battery after merging latest main (65e8115):

tests/unit/{provider-node-reserved-prefix,model-resolver,chat-helpers,
            provider-node-icon-url,custom-headers-provider-nodes,
            local-aliases-precedence}.test.ts
# pass 111 / fail 0

npm run typecheck:core    # clean (exit 0)
npx eslint <4 changed files> --suppressions-location config/quality/eslint-suppressions.json  # exit 0

Runtime behavior unchanged (verified e2e on a live instance): a legacy node already saved with prefix tokenrouter still routes to the built-in provider via getModelInfo("tokenrouter/..."), and node-by-internal-id requests still reach the custom node — this PR only gates new writes.

Notes for reviewers

  • The count assertion pins 391 (measured at merge time). It exists to prove the set is a full REGISTRY walk rather than a hand-maintained list; it will move as providers are added — that's intentional.
  • ⚠️ base-red inherited: main CI is failing on main head commits independent of this change (OpenSSF Scorecard / opencode-plugin CI / Build App / Publish to Docker Hub failures present before this branch), and issue 🔴 Release branch not green: release/v3.8.50 #9985 documents release/v3.8.50 redness. None of the failing checks touch the files in this PR.
  • Merge commit e014dff resolves the conflict with 65e8115 by keeping both the upstream icon-url rewrite and this PR's shared-module import/guard.

diegosouzapw and others added 10 commits August 8, 2026 00:08
…ouzapw#189, diegosouzapw#190)

Bumps: nanoid ^3.3.17 (was transitive, now overridden), dompurify ^3.4.13
(with monaco-editor scoped override). Closes Dependabot diegosouzapw#189, diegosouzapw#190.

Remaining diegosouzapw#182-diegosouzapw#188 (js-yaml + mermaid) already closed by diegosouzapw#9651 merge —
awaiting Dependabot re-scan.

npm audit → 0 vulnerabilities.
…egosouzapw#190

Closes Dependabot diegosouzapw#189 (dompurify 3.4.13) and diegosouzapw#190 (nanoid 3.3.17). npm audit → 0.
_tasks is a SEPARATE nested git repo (gitignored). The pattern _tasks/ (trailing
slash) ignores only a directory, not a SYMLINK named _tasks. A self-referential
_tasks symlink can slip in via git add -A and, once pulled, checkout materializes
it over the real _tasks repo (destroying plans/specs/hands-off). Anchored /_tasks
ignores the symlink too, preventing re-capture.
…pw#10026)

Mirror the request-time exclusion rule (provider_specific_data.excludedModels)
in the unified catalog builder: a model is hidden when its provider has
connections but none of them is eligible for it. Applied across the
PROVIDER_MODELS, synced, custom, alias-backed, and managed-fallback loops
so ghost models no longer appear as available.

Co-authored-by: ritheshcn25 <ritheshcn25@users.noreply.github.com>
…osouzapw#10055)

* fix(models): memoize getModelsDevPricing for /v1/models catalog

resolveCatalogPricing called getModelsDevPricing once per model while
building GET /v1/models. Each call re-scanned models_dev_pricing and
JSON.parsed every row (~10k SQL scans + multi-GB parse work), pegging
the event loop so even /healthz timed out (diegosouzapw#9685, diegosouzapw#10052).

Memoize the parsed map until saveModelsDevPricing / clearModelsDevPricing
and add a unit test for invalidation.

Signed-off-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>

* fix(db): invalidate modelsDevPricing cache on DB reset (diegosouzapw#10055)

Copilot review fixes:
1. Register invalidateModelsDevPricingCache() with DB state reset system
   so resetDbInstance() clears the process-local memo, preventing stale
   pricing data from surviving across DB reset/restore operations.
2. Add test assertion verifying DB reset bypasses the memo (Copilot diegosouzapw#10055).

The process-local memo at modelsDevSync.ts:204 caches getModelsDevPricing()
results until saveModelsDevPricing()/clearModelsDevPricing() to avoid
re-scanning all pricing rows on every /v1/models request. Without this hook,
backup restore and test DB resets would serve stale cached data from the
previous connection.

Tests: npm run test:unit:serial -- tests/unit/modelsDevSync-extended.test.ts

---------

Signed-off-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
⭐5 — Fornecedores locais compartilhados (ollama-local, LM Studio, vLLM) declaram passthroughModels:true no registry, mas hasPerModelQuota() não consultava o registry compartilhado — fallha de modelo faltante virava cooldown de conexão inteira. Broadens a classificação de model-lockout. TDD + 78/291 testes + typecheck + lint verdes. Fecha diegosouzapw#11071.
Landed with the design call resolved per the owner's pick — **option 1**: the synced store is now endpoint-agnostic (persistDiscoveredModels and managedModelImport no longer drop non-chat models at write time), and chat selectability moved to read time (auto-pool expansion in autoStrategy applies filterChatSelectableModels; the models-route projection already had its chatOnly filter). Your discovery test now passes end-to-end (3/3): /api/show capabilities persist per connection and image/embedding requests route through the advertising host.

Reconciliation notes: conflicted areas merged onto the current tip (adobe discovery import, requestedModel preflight signature, resolvedProvider fast-path coexists with the synced-route override — explicit resolution wins); carried base-red drains (diegosouzapw#10055 memoization, diegosouzapw#11071 test variants) dropped as already-landed; the managed-model-import exclusion test was propagated to the new contract (image/video models persist; the read filter still hides them from chat pickers — pinned by a new assertion). Full battery: 205/206 focused (the one red is a confirmed periodic-timer timing flake on the loaded devbox — 20/20 isolated), autoCombo vitest 30/30, combo suites 46/46, gates + typecheck clean.

Thank you @yourspraveen — the capability probe + routing design was right; it just needed the store contract opened up. Fixes diegosouzapw#11087.
…create/update

A compatible node created with prefix "tokenrouter" was silently
unreachable: the runtime model resolver (src/sse/services/model.ts)
skips compatible-node lookup for built-in registry ids/aliases, so
"tokenrouter/qwen/..." routed to the built-in tokenrouter provider and
failed with "No active credentials for provider: tokenrouter" even
though the node itself worked when addressed by its internal id.

Reject reserved prefixes at the write path instead:

- new shared module src/shared/constants/reservedProviderPrefixes.ts
  (REGISTRY ids + aliases, case-sensitive, built lazily) — single
  source of truth consumed by both the runtime guard and the
  validation schemas so they can never drift apart
- createProviderNodeSchema / updateProviderNodeSchema now reject
  reserved prefixes with a clear message naming the colliding prefix
- src/sse/services/model.ts consumes the shared module; runtime
  behavior is byte-for-byte unchanged (verified e2e)

Set semantics mirror the old inline guard exactly: manual alias ids
outside REGISTRY (xiaomi/llamacpp/aq) do not intercept nodes at
runtime and stay allowed; mixed-case input (TokenRouter) does not
collide with the exact-match runtime lookup either.
…-guard-validation

# Conflicts:
#	src/sse/services/model.ts
…stream merge

Upstream 65e8115 added new providers to the registry; the reserved set
is a full REGISTRY walk, so the pinned count moves 329 -> 391. The
tracked-artifacts pre-commit gate fails on this branch because the same
upstream commit force-tracked two docs/superpowers/ files that its own
.gitignore excludes — an inherited upstream issue unrelated to this fix,
so hooks are skipped for this fixture-only commit with operator approval.
@ggdayup
ggdayup requested a review from diegosouzapw as a code owner August 24, 2026 10:29
@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.8.50 August 24, 2026 14:36
@diegosouzapw

Copy link
Copy Markdown
Owner

Landed via consolidated batch validation (direct push — your branch's history had diverged heavily from release/v3.8.50, carrying ~8 already-shipped commits alongside your own; cherry-picked just your 2 commits onto the current tip). One adjustment during landing: your import of getReservedProviderPrefixes conflicted with a sibling batch PR (#11362-family) that had added other new imports to the same block in src/sse/services/model.ts — combined both cleanly (kept all imports). Also bumped your own fixture pin from 391 to 395, since another PR in this same batch (#11333, volcengine) added 2 new providers that grow the REGISTRY-derived reserved-prefix count. Own regression suite (14/14) passes. Fixes the tokenrouter-shadowing bug you diagnosed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants