Repository navigation
feat(services): promote Bifrost to embedded/supervised service (#5670) - #5817
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request integrates Bifrost as a new supervised embedded service, introducing lifecycle API endpoints, UI components, database migrations, and comprehensive tests. The review feedback highlights several critical issues: potential TypeErrors in routingBackend.ts and the update route due to unsafe chaining on sup?.getStatus(), a missing stateChange event listener on the dynamically initialized supervisor that prevents model synchronization, hardcoded Portuguese error messages in the start and restart routes, a type mismatch returning a number instead of a boolean for autoStart, and a violation of Repository Hard Rule #8 by defining a Zod schema locally.
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.
| if (!resolvedBaseUrl) { | ||
| const sup = getSupervisor("bifrost"); | ||
| if (sup?.getStatus().state === "running") { | ||
| resolvedBaseUrl = `http://127.0.0.1:${sup.getStatus().port}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
If getSupervisor("bifrost") returns undefined (e.g., if the service has not been initialized or started yet), sup?.getStatus() will evaluate to undefined. Attempting to access .state on undefined will throw a TypeError: Cannot read properties of undefined (reading 'state').
To prevent this, store the status in a variable and use optional chaining to safely check the state and retrieve the port.
| if (!resolvedBaseUrl) { | |
| const sup = getSupervisor("bifrost"); | |
| if (sup?.getStatus().state === "running") { | |
| resolvedBaseUrl = `http://127.0.0.1:${sup.getStatus().port}`; | |
| } | |
| } | |
| if (!resolvedBaseUrl) { | |
| const sup = getSupervisor("bifrost"); | |
| const status = sup?.getStatus(); | |
| if (status?.state === "running") { | |
| resolvedBaseUrl = `http://127.0.0.1:${status.port}`; | |
| } | |
| } |
| const sup = getSupervisor("bifrost"); | ||
| const wasRunning = sup?.getStatus().state === "running"; |
There was a problem hiding this comment.
If getSupervisor("bifrost") returns undefined, sup?.getStatus() will evaluate to undefined. Accessing .state on undefined will throw a TypeError. Use optional chaining to safely access the state.
| const sup = getSupervisor("bifrost"); | |
| const wasRunning = sup?.getStatus().state === "running"; | |
| const sup = getSupervisor("bifrost"); | |
| const wasRunning = sup?.getStatus()?.state === "running"; |
| export async function getOrInitSupervisor(): Promise<ServiceSupervisor> { | ||
| const existing = getSupervisor(TOOL); | ||
| if (existing) return existing; | ||
|
|
||
| const sup = new ServiceSupervisor({ | ||
| tool: TOOL, | ||
| port: PORT, | ||
| spawnArgs: () => resolveSpawnArgs(PORT), | ||
| healthUrl: () => `http://127.0.0.1:${PORT}/v1/models`, | ||
| healthIntervalMs: 5_000, | ||
| stopTimeoutMs: 15_000, | ||
| logsBufferBytes: 5_242_880, | ||
| }); | ||
|
|
||
| registerSupervisor(sup); | ||
| return sup; | ||
| } |
There was a problem hiding this comment.
When the supervisor is initialized on demand via getOrInitSupervisor(), it is registered in the registry but lacks the "stateChange" event listener that is attached during bootstrapEmbeddedServices().
Because Bifrost starts as not_installed by default, it is skipped during the initial bootstrap. When a user later installs and starts the service, the supervisor is created here on demand. Without the "stateChange" listener, transitioning to "running" will never trigger model synchronization (scheduleServiceModelSync), and stopping or encountering an error will never stop synchronization or mark models as unavailable.
We should dynamically import the required helpers and attach the "stateChange" listener here as well.
export async function getOrInitSupervisor(): Promise<ServiceSupervisor> {
const existing = getSupervisor(TOOL);
if (existing) return existing;
const sup = new ServiceSupervisor({
tool: TOOL,
port: PORT,
spawnArgs: () => resolveSpawnArgs(PORT),
healthUrl: () => `http://127.0.0.1:${PORT}/v1/models`,
healthIntervalMs: 5_000,
stopTimeoutMs: 15_000,
logsBufferBytes: 5_242_880,
});
const { scheduleServiceModelSync, stopServiceModelSync } = await import("@/lib/services/modelSync");
const { markAllUnavailable } = await import("@/lib/db/serviceModels");
const baseUrl = `http://127.0.0.1:${PORT}`;
sup.on("stateChange", (status) => {
if (status.state === "running") {
scheduleServiceModelSync(TOOL, baseUrl, "");
} else if (status.state === "stopped" || status.state === "error") {
stopServiceModelSync(TOOL);
markAllUnavailable(TOOL);
}
});
registerSupervisor(sup);
return sup;
}| if (!row || row.status === "not_installed") { | ||
| return createErrorResponse({ status: 409, message: "Bifrost não está instalado." }); | ||
| } |
There was a problem hiding this comment.
The error message "Bifrost não está instalado." is hardcoded in Portuguese, which is inconsistent with the rest of the API routes and the OpenAPI specification (Bifrost is not installed). It should be in English.
| if (!row || row.status === "not_installed") { | |
| return createErrorResponse({ status: 409, message: "Bifrost não está instalado." }); | |
| } | |
| if (!row || row.status === "not_installed") { | |
| return createErrorResponse({ status: 409, message: "Bifrost is not installed." }); | |
| } |
| if (!row || row.status === "not_installed") { | ||
| return createErrorResponse({ status: 409, message: "Bifrost não está instalado." }); | ||
| } |
There was a problem hiding this comment.
The error message "Bifrost não está instalado." is hardcoded in Portuguese, which is inconsistent with the rest of the API routes and the OpenAPI specification (Bifrost is not installed). It should be in English.
| if (!row || row.status === "not_installed") { | |
| return createErrorResponse({ status: 409, message: "Bifrost não está instalado." }); | |
| } | |
| if (!row || row.status === "not_installed") { | |
| return createErrorResponse({ status: 409, message: "Bifrost is not installed." }); | |
| } |
| installedVersion: installedVersion ?? row?.installedVersion ?? null, | ||
| latestVersion, | ||
| updateAvailable: !!installedVersion && !!latestVersion && installedVersion !== latestVersion, | ||
| autoStart: row?.autoStart ?? false, |
There was a problem hiding this comment.
Since SQLite stores boolean values as integers (0 or 1), row?.autoStart will return a number. Returning a number directly violates the OpenAPI schema which expects autoStart to be a boolean. Coerce it to a boolean explicitly.
| autoStart: row?.autoStart ?? false, | |
| autoStart: !!row?.autoStart, |
| import { createErrorResponse } from "@/lib/api/errorResponse"; | ||
| import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; | ||
|
|
||
| const BodySchema = z.object({ enabled: z.boolean() }); |
There was a problem hiding this comment.
Defining the Zod schema BodySchema locally violates Hard Rule #8 of the Repository Style Guide, which states: 'Always validate inputs with Zod schemas from src/shared/validation/schemas.ts'.
Please move this schema to src/shared/validation/schemas.ts and import it here.
References
- Always validate inputs with Zod schemas from
src/shared/validation/schemas.ts. (link)
8d08f1a to
b83d351
Compare
Re-stacked on the provider-manifest stack (#5869 → #5870)This PR was rebased on top of #5870 ( Why: both this PR and #5870 edit
Together: starting the supervised Bifrost service auto-enables the Merge order: #5869 → #5870 → this PR. Until #5869/#5870 land on Validation on the combined tree: Merge gate unchanged: the Go-binary install/start/health path still needs the |
CI Coverage Report
Coverage artifact was not available for this run. |
Promotes Bifrost (@maximhq/bifrost — Go AI-gateway) from an env-only relay sidecar to a first-class embedded/supervised service, matching the existing cliproxy/9router model. Implements item #2 of #5670; the broader RouterBackend contract (items #1, #3-#5) stays out of scope. - Installer (npm-style, ninerouter model): install/update/getInstalledVersion/ getLatestVersion (1h cache)/resolveSpawnArgs (Go single-dash flags, pinned BIFROST_TRANSPORT_VERSION), needsApiKey=false - Bootstrap SERVICES entry (healthPath /v1/models) + spawn-args factory branch - Migration 113 seeds the version_manager row (not_installed, port 8080, auto_update=1, provider_expose=1) - 7 lifecycle API routes under /api/services/bifrost/ (verbatim from cliproxy, errors sanitized) — loopback-only via existing LOCAL_ONLY_API_PREFIXES - Shared [name]/logs branch for bifrost - Dashboard tab + registration in the services page shell - Relay auto-wiring: getBifrostRoutingConfig defaults BIFROST_BASE_URL to the supervised port when the instance is running; explicit env still wins; the env-only relay path (/v1/relay/.../bifrost) stays unchanged (compat layer) - Docs (EMBEDDED-SERVICES, openapi) + unit tests (installer/route-guard/routing, 19 tests) + RUN_SERVICES_INT-gated integration lifecycle Note: the actual Go-binary install/start/health path requires a documented VPS live-test before merge (Hard Rule #18 / spec section 7); the gated integration harness is the vehicle for that run.
b83d351 to
0ceab1c
Compare
…er-inference#5670) (cheaper-inference#5817) Promotes Bifrost (@maximhq/bifrost — Go AI-gateway) from an env-only relay sidecar to a first-class embedded/supervised service, matching the existing cliproxy/9router model. Implements item cheaper-inference#2 of cheaper-inference#5670; the broader RouterBackend contract (items cheaper-inference#1, cheaper-inference#3-cheaper-inference#5) stays out of scope. - Installer (npm-style, ninerouter model): install/update/getInstalledVersion/ getLatestVersion (1h cache)/resolveSpawnArgs (Go single-dash flags, pinned BIFROST_TRANSPORT_VERSION), needsApiKey=false - Bootstrap SERVICES entry (healthPath /v1/models) + spawn-args factory branch - Migration 113 seeds the version_manager row (not_installed, port 8080, auto_update=1, provider_expose=1) - 7 lifecycle API routes under /api/services/bifrost/ (verbatim from cliproxy, errors sanitized) — loopback-only via existing LOCAL_ONLY_API_PREFIXES - Shared [name]/logs branch for bifrost - Dashboard tab + registration in the services page shell - Relay auto-wiring: getBifrostRoutingConfig defaults BIFROST_BASE_URL to the supervised port when the instance is running; explicit env still wins; the env-only relay path (/v1/relay/.../bifrost) stays unchanged (compat layer) - Docs (EMBEDDED-SERVICES, openapi) + unit tests (installer/route-guard/routing, 19 tests) + RUN_SERVICES_INT-gated integration lifecycle Note: the actual Go-binary install/start/health path requires a documented VPS live-test before merge (Hard Rule cheaper-inference#18 / spec section 7); the gated integration harness is the vehicle for that run.
What
Promotes Bifrost (
@maximhq/bifrost— the Go AI-gateway) from an env-only relay sidecar to a first-class embedded/supervised service, using the exact same model already shipped forcliproxyand9router: installer → supervisor → lifecycle APIs → dashboard tab → health/auto-start/logs/version row → local-only route guard.This implements item #2 of #5670. The broader
RouterBackendcontract (items #1, #3–#5) and the native hot-path migration stay out of scope. The existing env-configured relay path (/v1/relay/.../bifrost,OMNIROUTE_RELAY_BACKEND=bifrost) is kept unchanged as a compatibility layer.Implementation (8 steps, mapped to the design spec)
src/lib/services/installers/bifrost.ts— npm-style (mirrorsninerouter.ts, since@maximhq/bifrostis a thin wrapper that lazily downloads the Go binary):install/update/getInstalledVersion/getLatestVersion(1h cache) +resolveSpawnArgs(port)(Go single-dash flags-port/-host 127.0.0.1/-app-dir/-log-level warn, pinnedBIFROST_TRANSPORT_VERSION).needsApiKey=false.src/lib/services/bootstrap.ts—SERVICES[]entry (healthPath: /v1/models, 5s health interval, 15s stop timeout, 5 MiB log ring) +buildSpawnArgsFactorybranch.113_bifrost_service.sql— seeds theversion_managerrow (not_installed, port 8080,auto_update=1,provider_expose=1).src/app/api/services/bifrost/—_lib.ts+ 7 lifecycle routes (install/start/stop/restart/update/status/auto-start), verbatim fromcliproxy, all errors viacreateErrorResponse/sanitizeErrorMessage.[name]/logs/route.ts—bifrostbranch./api/services/is alreadyLOCAL_ONLY); a unit test assertsisLocalOnlyPath("/api/services/bifrost/*") === true.BifrostServiceTab.tsx+ registration in the servicespage.tsxshell (Tab type +TABS[]+ render block).EMBEDDED-SERVICES.md(§1 table + §4.3 endpoint table) +openapi.yaml(7 paths); unit tests + gated integration lifecycle.Relay auto-wiring: when
BIFROST_BASE_URLis unset and the supervised instance isrunning,getBifrostRoutingConfig()defaults the relay base URL tohttp://127.0.0.1:{port}. ExplicitBIFROST_BASE_URLalways wins.Tests
full-lifecycle.int.test.ts, gated byRUN_SERVICES_INT=1(does not run or download the Go binary in the default suite).typecheck:core,lint(0 errors),check:cycles,test:unit+test:vitest(no feature-attributable failures).The actual Go-binary install → start → health path can only be validated live (it downloads a per-platform binary from
downloads.getmaxim.aion first start). Per Hard Rule #18, this must not be merged on "worked locally" alone — run the gated integration harness on the VPS and record the result before merge:Open questions to close during that run: Windows binary download; headless boot with an empty
-app-dir.Out of scope
RouterBackendunification, CLIProxyAPI ecosystem parity, and the hot-path migration (#5670 items #1, #3–#5).Part of #5670.