Skip to content

feat(sidecar): add provider manifest client - #6083

Closed
KooshaPari wants to merge 3 commits into
diegosouzapw:release/v3.8.44from
KooshaPari:fix/pr-6042-replay
Closed

KooshaPari wants to merge 3 commits into
diegosouzapw:release/v3.8.44from
KooshaPari:fix/pr-6042-replay

Conversation

@KooshaPari

Copy link
Copy Markdown
Contributor

Summary

  • add a reusable provider plugin manifest HTTP client for sidecars and backend adapters
  • keep the dashboard quick-start API Manager link stable in the same replay because it is part of the live PR head
  • replay the actual PR head onto release/v3.8.44 from the current koosha/sidecar-provider-manifest-client branch, not the stale local sidecar worktree

Why

Bifrost, CLIProxyAPI, and future native sidecars should consume provider metadata through a stable HTTP manifest contract.

Validation

  • node --import tsx --test /tmp/omniroute-pr-6042-replay/tests/unit/provider-plugin-manifest-client.test.ts
  • node --import tsx --test /tmp/omniroute-pr-6042-replay/tests/unit/ui/quick-start-api-keys-link-5695.test.ts
  • git -C /tmp/omniroute-pr-6042-replay diff --check upstream/release/v3.8.44...HEAD

@KooshaPari
KooshaPari requested a review from diegosouzapw as a code owner July 3, 2026 10:25

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new client, providerPluginManifestClient.ts, along with corresponding unit tests and documentation, to resolve and fetch the provider plugin manifest instead of hard-coding its route. The review feedback focuses on improving robustness and preventing potential runtime crashes. Specifically, it suggests checking if process is defined before accessing process.env to support non-Node environments, and adding defensive checks to ensure the fetched manifest and its providers array are valid before accessing their properties.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +20 to +38
export function resolveProviderPluginManifestUrl(
options: Pick<ProviderPluginManifestClientOptions, "baseUrl" | "manifestUrl"> = {},
): string {
const explicitUrl = options.manifestUrl?.trim();
if (explicitUrl) return explicitUrl;

const envUrl = process.env[PROVIDER_PLUGIN_MANIFEST_ENV]?.trim();
if (envUrl) return envUrl;

const baseUrl = options.baseUrl?.trim();
if (baseUrl) {
return `${trimTrailingSlash(baseUrl)}${PROVIDER_PLUGIN_MANIFEST_PATH}`;
}

const host = process.env.HOST || "127.0.0.1";
const port = process.env.PORT || process.env.DASHBOARD_PORT || process.env.API_PORT || "20128";
const protocol = process.env.OMNIROUTE_PUBLIC_PROTOCOL || "http";
return `${protocol}://${host}:${port}${PROVIDER_PLUGIN_MANIFEST_PATH}`;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In environments where process is not globally defined (such as browser environments or certain edge runtimes), directly accessing process.env will throw a ReferenceError. It is safer to check if process is defined before accessing its properties.

Suggested change
export function resolveProviderPluginManifestUrl(
options: Pick<ProviderPluginManifestClientOptions, "baseUrl" | "manifestUrl"> = {},
): string {
const explicitUrl = options.manifestUrl?.trim();
if (explicitUrl) return explicitUrl;
const envUrl = process.env[PROVIDER_PLUGIN_MANIFEST_ENV]?.trim();
if (envUrl) return envUrl;
const baseUrl = options.baseUrl?.trim();
if (baseUrl) {
return `${trimTrailingSlash(baseUrl)}${PROVIDER_PLUGIN_MANIFEST_PATH}`;
}
const host = process.env.HOST || "127.0.0.1";
const port = process.env.PORT || process.env.DASHBOARD_PORT || process.env.API_PORT || "20128";
const protocol = process.env.OMNIROUTE_PUBLIC_PROTOCOL || "http";
return `${protocol}://${host}:${port}${PROVIDER_PLUGIN_MANIFEST_PATH}`;
}
export function resolveProviderPluginManifestUrl(
options: Pick<ProviderPluginManifestClientOptions, "baseUrl" | "manifestUrl"> = {},
): string {
const explicitUrl = options.manifestUrl?.trim();
if (explicitUrl) return explicitUrl;
const env = typeof process !== "undefined" ? process.env : {};
const envUrl = env[PROVIDER_PLUGIN_MANIFEST_ENV]?.trim();
if (envUrl) return envUrl;
const baseUrl = options.baseUrl?.trim();
if (baseUrl) {
return `${trimTrailingSlash(baseUrl)}${PROVIDER_PLUGIN_MANIFEST_PATH}`;
}
const host = env.HOST || "127.0.0.1";
const port = env.PORT || env.DASHBOARD_PORT || env.API_PORT || "20128";
const protocol = env.OMNIROUTE_PUBLIC_PROTOCOL || "http";
return `${protocol}://${host}:${port}${PROVIDER_PLUGIN_MANIFEST_PATH}`;
}

Comment on lines +54 to +55
const manifest = (await response.json()) as ProviderPluginManifest;
if (manifest.schemaVersion !== 1 || !Array.isArray(manifest.providers)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the response body is empty, null, or not an object, accessing manifest.schemaVersion will throw a TypeError. Adding a null/object check ensures defensive programming and robust error handling.

Suggested change
const manifest = (await response.json()) as ProviderPluginManifest;
if (manifest.schemaVersion !== 1 || !Array.isArray(manifest.providers)) {
const manifest = (await response.json()) as ProviderPluginManifest;
if (!manifest || manifest.schemaVersion !== 1 || !Array.isArray(manifest.providers)) {

Comment on lines +62 to +66
export function getProviderPluginManifestEntryForModelFromManifest(
manifest: ProviderPluginManifest,
model: string | undefined,
): ProviderPluginManifestEntry | null {
if (!model) return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent runtime crashes if manifest is null/undefined or if manifest.providers is not an array, add defensive checks before accessing its properties.

Suggested change
export function getProviderPluginManifestEntryForModelFromManifest(
manifest: ProviderPluginManifest,
model: string | undefined,
): ProviderPluginManifestEntry | null {
if (!model) return null;
export function getProviderPluginManifestEntryForModelFromManifest(
manifest: ProviderPluginManifest,
model: string | undefined,
): ProviderPluginManifestEntry | null {
if (!model || !manifest || !Array.isArray(manifest.providers)) return null;

@diegosouzapw

Copy link
Copy Markdown
Owner

Thank you, @KooshaPari 🙏 — closing as deferred, not rejected. This PR is the sidecar provider-manifest client — infra with no production consumer yet; lands once a native adapter reads it. It's one piece of the native-router backend workstream tracked in #5670, where I've cataloged all of these as the staged backlog for the 3.9.0/4.0 window (see my comment there). Merging these subsystem pieces individually into the hot release/v3.8.x branch would commit the core to a major rewrite before the design is agreed — so the plan is: lock the migration plan (#6081) as the RFC on #5670, then re-open focused slices against the 3.9.0 branch. The engineering is solid; this is purely about sequencing a big subsystem behind an agreed design. 🚀

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.

2 participants