From 14468cdec51219d34ac47e9cd0aee0a7c3b2abb9 Mon Sep 17 00:00:00 2001 From: Sean Ford Date: Thu, 23 Jul 2026 10:25:28 -0400 Subject: [PATCH] fix(bifrost): send v-prefixed transport version, not bare semver (#8194) * fix(bifrost): send v-prefixed transport version, not bare semver bifrost.ts resolveSpawnArgs() set BIFROST_TRANSPORT_VERSION straight from getInstalledVersionSync(), which reads the raw "version" field out of node_modules/@maximhq/bifrost/package.json - always bare semver per npm convention (e.g. "1.6.3"). @maximhq/bifrost's own bin.js validates that env var against /^v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/ or the literal "latest" and rejects anything else with "Invalid transport version format", exiting immediately. Every embedded Bifrost instance failed to start as a result. Adds formatTransportVersion() to normalize at the call site that owns the env var, so bifrost.ts and the upstream @maximhq/bifrost package both stay exactly as designed - no changes needed to bifrost itself. Strengthens the existing resolveSpawnArgs test to assert the actual v-prefix format (it previously only checked for a non-empty string, the same gap #6877 called out for cliproxy's pre-existing test), and adds a dedicated regression test file covering the pure formatTransportVersion() helper plus a real-filesystem resolveSpawnArgs() integration check. Found and fixed while self-hosting OmniRoute and diagnosing why bifrost kept crash-looping on startup. * fix(bifrost): resolve install dir lazily so version read honors runtime DATA_DIR Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Co-authored-by: seanford --- src/lib/services/installers/bifrost.ts | 33 +++++- .../bifrost-transport-version-format.test.ts | 112 ++++++++++++++++++ .../unit/services/installers/bifrost.test.ts | 10 +- 3 files changed, 151 insertions(+), 4 deletions(-) create mode 100644 tests/unit/services/installers/bifrost-transport-version-format.test.ts diff --git a/src/lib/services/installers/bifrost.ts b/src/lib/services/installers/bifrost.ts index a097f6cdc5..9c3003e484 100644 --- a/src/lib/services/installers/bifrost.ts +++ b/src/lib/services/installers/bifrost.ts @@ -25,12 +25,21 @@ export interface SpawnArgs { let latestVersionCache: { value: string; expiresAt: number } | null = null; const VERSION_CACHE_TTL_MS = 3_600_000; +// Resolve the install dir lazily from the *current* DATA_DIR so a runtime +// DATA_DIR override (operator env change, or a test's tmp-dir isolation) is +// honored — the module-level BIFROST_INSTALL_DIR const is frozen at import. +function getBifrostInstallDir(): string { + return process.env.DATA_DIR + ? path.join(process.env.DATA_DIR, "services", "bifrost") + : BIFROST_INSTALL_DIR; +} + function getInstalledPkgPath(): string { - return path.join(BIFROST_INSTALL_DIR, "node_modules", "@maximhq", "bifrost", "package.json"); + return path.join(getBifrostInstallDir(), "node_modules", "@maximhq", "bifrost", "package.json"); } function getBinPath(): string { - return path.join(BIFROST_INSTALL_DIR, "node_modules", "@maximhq", "bifrost", "bin.js"); + return path.join(getBifrostInstallDir(), "node_modules", "@maximhq", "bifrost", "bin.js"); } function getInstalledVersionSync(): string | null { @@ -118,10 +127,28 @@ export async function update(): Promise { return install("latest"); } +/** + * Bifrost's own bin.js validates BIFROST_TRANSPORT_VERSION against + * /^v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/ or the literal "latest" - it does + * NOT accept a bare semver string like "1.6.3". getInstalledVersionSync() + * reads the raw "version" field straight out of package.json, which is + * always bare semver (npm/package.json convention never includes the "v" + * prefix), so passing it through unmodified made every embedded Bifrost + * instance fail on startup with "Invalid transport version format". + * + * Normalize here, at the call site that owns the env var, so both + * bifrost.ts and the upstream @maximhq/bifrost package can stay exactly as + * they are otherwise designed to be used. + */ +export function formatTransportVersion(version: string | null): string { + if (!version || version === "latest") return version ?? "latest"; + return version.startsWith("v") ? version : `v${version}`; +} + export function resolveSpawnArgs(port: number): SpawnArgs { const binPath = getBinPath(); // Pin transport version to the installed npm version for reproducibility (spec §2b) - const transportVersion = getInstalledVersionSync() ?? "latest"; + const transportVersion = formatTransportVersion(getInstalledVersionSync()); return { command: process.execPath, diff --git a/tests/unit/services/installers/bifrost-transport-version-format.test.ts b/tests/unit/services/installers/bifrost-transport-version-format.test.ts new file mode 100644 index 0000000000..480ad934bd --- /dev/null +++ b/tests/unit/services/installers/bifrost-transport-version-format.test.ts @@ -0,0 +1,112 @@ +/** + * Regression test - bifrost resolveSpawnArgs() must pass a v-prefixed (or + * "latest") BIFROST_TRANSPORT_VERSION, never the bare semver string that + * getInstalledVersion() reads out of node_modules/@maximhq/bifrost/package.json. + * + * Bifrost's own bin.js validates the env var against + * /^v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/ or the literal "latest" and exits + * immediately with "Invalid transport version format" on anything else. + * package.json's "version" field is always bare semver per npm convention + * (e.g. "1.6.3"), so every embedded Bifrost instance failed to start until + * this fix - unlike cliproxy's #6877 regression, this was silent at the + * resolveSpawnArgs() call site itself; the failure only surfaced once the + * spawned process actually ran. + * + * Covers both the pure formatTransportVersion() helper directly, and the + * real resolveSpawnArgs() integration path against a filesystem-backed + * fake npm, matching the pattern used by + * tests/unit/services/installers/cliproxy-resolve-spawn-args-6877.test.ts. + */ + +import { describe, it, before, after } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +describe("formatTransportVersion (pure)", () => { + it("prepends v to bare semver", async () => { + const { formatTransportVersion } = await import( + "../../../../src/lib/services/installers/bifrost.ts" + ); + assert.equal(formatTransportVersion("1.6.3"), "v1.6.3"); + assert.equal(formatTransportVersion("2.0.0-beta.1"), "v2.0.0-beta.1"); + }); + + it("leaves an already-v-prefixed version untouched", async () => { + const { formatTransportVersion } = await import( + "../../../../src/lib/services/installers/bifrost.ts" + ); + assert.equal(formatTransportVersion("v1.6.3"), "v1.6.3"); + }); + + it('passes through "latest" untouched', async () => { + const { formatTransportVersion } = await import( + "../../../../src/lib/services/installers/bifrost.ts" + ); + assert.equal(formatTransportVersion("latest"), "latest"); + }); + + it('defaults null to "latest"', async () => { + const { formatTransportVersion } = await import( + "../../../../src/lib/services/installers/bifrost.ts" + ); + assert.equal(formatTransportVersion(null), "latest"); + }); +}); + +describe("resolveSpawnArgs BIFROST_TRANSPORT_VERSION (real filesystem)", () => { + const ORIGINAL_DATA_DIR = process.env.DATA_DIR; + const ORIGINAL_PATH = process.env.PATH ?? ""; + let dataDir: string; + let fakeBinDir: string; + + before(() => { + dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-bifrost-vfix-")); + fakeBinDir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-bifrost-vfix-bin-")); + process.env.DATA_DIR = dataDir; + process.env.PATH = `${fakeBinDir}:${ORIGINAL_PATH}`; + + // Same package.json shape a real `npm install @maximhq/bifrost@1.6.3` would + // leave behind - bare semver "version" field, no "v" prefix. + const pkgDir = path.join(dataDir, "services", "bifrost", "node_modules", "@maximhq", "bifrost"); + fs.mkdirSync(pkgDir, { recursive: true }); + fs.writeFileSync( + path.join(pkgDir, "package.json"), + JSON.stringify({ name: "@maximhq/bifrost", version: "1.6.3" }), + "utf8" + ); + fs.writeFileSync(path.join(pkgDir, "bin.js"), "", "utf8"); + }); + + after(() => { + if (ORIGINAL_DATA_DIR === undefined) { + delete process.env.DATA_DIR; + } else { + process.env.DATA_DIR = ORIGINAL_DATA_DIR; + } + process.env.PATH = ORIGINAL_PATH; + fs.rmSync(dataDir, { recursive: true, force: true }); + fs.rmSync(fakeBinDir, { recursive: true, force: true }); + }); + + it("BIFROST_TRANSPORT_VERSION is v-prefixed, matching what bin.js requires", async () => { + const { resolveSpawnArgs } = await import( + "../../../../src/lib/services/installers/bifrost.ts" + ); + + const args = resolveSpawnArgs(8080); + + assert.equal( + args.env.BIFROST_TRANSPORT_VERSION, + "v1.6.3", + "must be v-prefixed - a bare '1.6.3' is rejected by bifrost's bin.js with " + + "'Invalid transport version format'" + ); + assert.match( + args.env.BIFROST_TRANSPORT_VERSION as string, + /^(latest|v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?)$/, + "must satisfy bifrost bin.js's own validateTransportVersion() regex" + ); + }); +}); diff --git a/tests/unit/services/installers/bifrost.test.ts b/tests/unit/services/installers/bifrost.test.ts index 8e20574a68..f59541a436 100644 --- a/tests/unit/services/installers/bifrost.test.ts +++ b/tests/unit/services/installers/bifrost.test.ts @@ -127,12 +127,20 @@ test("resolveSpawnArgs shape: command is node, bin.js path, Go single-dash flags assert.ok(logLevelIdx !== -1, "must have -log-level flag"); assert.equal(args.args[logLevelIdx + 1], "warn"); - // BIFROST_TRANSPORT_VERSION must be set in env + // BIFROST_TRANSPORT_VERSION must be set in env AND match the format + // bifrost's own bin.js requires: /^v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/ or "latest". + // (regression check for the "Invalid transport version format" startup crash - + // the fake npm helper above reports version "1.6.3", bare semver with no "v") assert.ok( typeof args.env.BIFROST_TRANSPORT_VERSION === "string" && args.env.BIFROST_TRANSPORT_VERSION.length > 0, "BIFROST_TRANSPORT_VERSION must be set in env" ); + assert.match( + args.env.BIFROST_TRANSPORT_VERSION, + /^(latest|v\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?)$/, + `BIFROST_TRANSPORT_VERSION must be "latest" or v-prefixed semver, got: ${args.env.BIFROST_TRANSPORT_VERSION}` + ); }); test("resolveSpawnArgs with different port passes correct -port value", () => {