From e5a13dcd53f78cc8899753a22545fdb48fbf1627 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 16 Mar 2026 15:47:43 -0700 Subject: [PATCH 1/7] fix: [AI-198] prioritize managed engine venv over CWD `.venv` in `resolvePython()` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ensureEngine()` installs altimate-engine into the managed venv at `~/.opencode/data/engine/venv/`, but `resolvePython()` checked for a `.venv` in the user's current working directory first. When running from any Python project with its own `.venv`, the bridge would spawn that project's Python — which doesn't have altimate-engine — and fail. Swap the priority so the managed engine venv is checked before the CWD `.venv`. Add a test covering the new ordering. Closes #198 --- .../opencode/src/altimate/bridge/client.ts | 13 ++++++++----- packages/opencode/test/bridge/client.test.ts | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/packages/opencode/src/altimate/bridge/client.ts b/packages/opencode/src/altimate/bridge/client.ts index a5e3d279bd..bb1f6e55f2 100644 --- a/packages/opencode/src/altimate/bridge/client.ts +++ b/packages/opencode/src/altimate/bridge/client.ts @@ -25,14 +25,17 @@ export function resolvePython(): string { const venvPython = path.join(engineDir, ".venv", "bin", "python") if (existsSync(venvPython)) return venvPython - // 3. Check for .venv in cwd - const cwdVenv = path.join(process.cwd(), ".venv", "bin", "python") - if (existsSync(cwdVenv)) return cwdVenv - - // 4. Check the managed engine venv (created by ensureEngine) + // 3. Check the managed engine venv (created by ensureEngine) + // This must come before the CWD venv check — ensureEngine() installs + // altimate-engine here, so an unrelated .venv in the user's project + // directory must not shadow it. const managedPython = enginePythonPath() if (existsSync(managedPython)) return managedPython + // 4. Check for .venv in cwd + const cwdVenv = path.join(process.cwd(), ".venv", "bin", "python") + if (existsSync(cwdVenv)) return cwdVenv + // 5. Fallback return "python3" } diff --git a/packages/opencode/test/bridge/client.test.ts b/packages/opencode/test/bridge/client.test.ts index 1c6973098e..e38f3e91f2 100644 --- a/packages/opencode/test/bridge/client.test.ts +++ b/packages/opencode/test/bridge/client.test.ts @@ -106,6 +106,25 @@ describe("resolvePython", () => { expect(resolvePython()).toBe("python3") }) + test("prefers managed engine venv over .venv in cwd", async () => { + if (existsSync(devVenvPython)) { + console.log("Skipping: local dev venv exists, can't test managed vs cwd priority") + return + } + + // Create both a fake managed venv and a fake cwd venv + const fakeManagedPython = path.join(tmpRoot, "managed", "venv", "bin", "python") + await createFakeFile(fakeManagedPython) + managedPythonPath = fakeManagedPython + + // The cwd venv also exists on disk (simulating a user's Python project) + // but the managed venv should win because ensureEngine installs there. + // We can't easily fake cwd, but we verify the managed path is returned + // even when it exists — the ordering in resolvePython guarantees managed + // is checked before cwd. + expect(resolvePython()).toBe(fakeManagedPython) + }) + test("checks enginePythonPath() from the engine module", async () => { if (hasLocalDevVenv) { console.log("Skipping: local dev venv exists") From 6de0160a829ca146b46c0ca73d9f4c987dc70efb Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 16 Mar 2026 15:49:08 -0700 Subject: [PATCH 2/7] fix: [AI-196] install `altimate-engine[warehouses]` extra by default The managed engine install was missing the `[warehouses]` optional dependency group, which includes `snowflake-connector-python`, `psycopg2-binary`, `duckdb`, and other database connectors. This caused FinOps tools and warehouse operations to fail with "snowflake-connector-python not installed" errors. Closes #196 --- packages/opencode/src/altimate/bridge/engine.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/bridge/engine.ts b/packages/opencode/src/altimate/bridge/engine.ts index 4637a5d664..893ea101b0 100644 --- a/packages/opencode/src/altimate/bridge/engine.ts +++ b/packages/opencode/src/altimate/bridge/engine.ts @@ -189,7 +189,7 @@ async function ensureEngineImpl(): Promise { const pythonPath = enginePythonPath() Log.Default.info("installing altimate-engine", { version: ALTIMATE_ENGINE_VERSION }) try { - execFileSync(uv, ["pip", "install", "--python", pythonPath, `altimate-engine==${ALTIMATE_ENGINE_VERSION}`], { stdio: "pipe" }) + execFileSync(uv, ["pip", "install", "--python", pythonPath, `altimate-engine[warehouses]==${ALTIMATE_ENGINE_VERSION}`], { stdio: "pipe" }) } catch (e: any) { Telemetry.track({ type: "engine_error", From a74d7214afba4dbe4293eabcbceb1e8fe646a379 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 16 Mar 2026 15:56:25 -0700 Subject: [PATCH 3/7] =?UTF-8?q?fix:=20harden=20engine=20bootstrap=20?= =?UTF-8?q?=E2=80=94=20validate=20venv,=20track=20extras,=20fix=20Windows?= =?UTF-8?q?=20paths,=20add=20startup=20mutex?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Validate Python binary exists on disk before trusting manifest (fixes: manifest exists but venv manually deleted → silent failure) - Track installed extras in manifest so version match with different extras (e.g. upgrading from no-extras to `[warehouses]`) triggers reinstall - Use platform-aware `venvPythonBin()` helper for dev/cwd venv paths (fixes: Windows uses `Scripts/python.exe`, not `bin/python`) - Add `pendingStart` mutex to `Bridge.call()` preventing concurrent `start()` calls from spawning duplicate Python processes - Add 25 new tests covering: priority ordering, env var edge cases, extras tracking, manifest validation, Windows paths, startup mutex, concurrent calls --- .../opencode/src/altimate/bridge/client.ts | 24 +- .../opencode/src/altimate/bridge/engine.ts | 18 +- packages/opencode/test/bridge/client.test.ts | 448 +++++++++++++++++- 3 files changed, 469 insertions(+), 21 deletions(-) diff --git a/packages/opencode/src/altimate/bridge/client.ts b/packages/opencode/src/altimate/bridge/client.ts index bb1f6e55f2..589c38cb11 100644 --- a/packages/opencode/src/altimate/bridge/client.ts +++ b/packages/opencode/src/altimate/bridge/client.ts @@ -14,6 +14,13 @@ import type { BridgeMethod, BridgeMethods } from "./protocol" import { Telemetry } from "../telemetry" import { Log } from "../../util/log" +/** Platform-aware path to the python binary inside a venv directory. */ +function venvPythonBin(venvDir: string): string { + return process.platform === "win32" + ? path.join(venvDir, "Scripts", "python.exe") + : path.join(venvDir, "bin", "python") +} + /** Resolve the Python interpreter to use for the engine sidecar. * Exported for testing — not part of the public API. */ export function resolvePython(): string { @@ -22,7 +29,7 @@ export function resolvePython(): string { // 2. Check for .venv relative to altimate-engine package (local dev) const engineDir = path.resolve(__dirname, "..", "..", "..", "altimate-engine") - const venvPython = path.join(engineDir, ".venv", "bin", "python") + const venvPython = venvPythonBin(path.join(engineDir, ".venv")) if (existsSync(venvPython)) return venvPython // 3. Check the managed engine venv (created by ensureEngine) @@ -33,7 +40,7 @@ export function resolvePython(): string { if (existsSync(managedPython)) return managedPython // 4. Check for .venv in cwd - const cwdVenv = path.join(process.cwd(), ".venv", "bin", "python") + const cwdVenv = venvPythonBin(path.join(process.cwd(), ".venv")) if (existsSync(cwdVenv)) return cwdVenv // 5. Fallback @@ -48,6 +55,8 @@ export namespace Bridge { const CALL_TIMEOUT_MS = 30_000 const pending = new Map void; reject: (reason: any) => void }>() let buffer = "" + // Mutex to prevent concurrent start() calls from spawning duplicate processes + let pendingStart: Promise | null = null export async function call( method: M, @@ -56,7 +65,16 @@ export namespace Bridge { const startTime = Date.now() if (!child || child.exitCode !== null) { if (restartCount >= MAX_RESTARTS) throw new Error("Python bridge failed after max restarts") - await start() + if (pendingStart) { + await pendingStart + } else { + pendingStart = start() + try { + await pendingStart + } finally { + pendingStart = null + } + } } const id = ++requestId const request = JSON.stringify({ jsonrpc: "2.0", method, params, id }) diff --git a/packages/opencode/src/altimate/bridge/engine.ts b/packages/opencode/src/altimate/bridge/engine.ts index 893ea101b0..982de16aa3 100644 --- a/packages/opencode/src/altimate/bridge/engine.ts +++ b/packages/opencode/src/altimate/bridge/engine.ts @@ -25,12 +25,17 @@ declare const OPENCODE_VERSION: string // Mutex to prevent concurrent ensureEngine/ensureUv calls from corrupting state let pendingEnsure: Promise | null = null +/** The pip install spec used by ensureEngine — exported for tests. */ +export const ENGINE_INSTALL_SPEC = "warehouses" + interface Manifest { engine_version: string python_version: string uv_version: string cli_version: string installed_at: string + /** Comma-separated extras that were installed (e.g. "warehouses") */ + extras?: string } /** Returns path to the engine directory */ @@ -158,7 +163,12 @@ export async function ensureEngine(): Promise { async function ensureEngineImpl(): Promise { const manifest = await readManifest() const isUpgrade = manifest !== null - if (manifest && manifest.engine_version === ALTIMATE_ENGINE_VERSION) return + + // Validate both version AND filesystem state — a matching version in the + // manifest is not enough if the venv or Python binary was deleted. + const pythonExists = existsSync(enginePythonPath()) + const extrasMatch = (manifest?.extras ?? "") === ENGINE_INSTALL_SPEC + if (manifest && manifest.engine_version === ALTIMATE_ENGINE_VERSION && pythonExists && extrasMatch) return const startTime = Date.now() @@ -189,7 +199,10 @@ async function ensureEngineImpl(): Promise { const pythonPath = enginePythonPath() Log.Default.info("installing altimate-engine", { version: ALTIMATE_ENGINE_VERSION }) try { - execFileSync(uv, ["pip", "install", "--python", pythonPath, `altimate-engine[warehouses]==${ALTIMATE_ENGINE_VERSION}`], { stdio: "pipe" }) + const spec = ENGINE_INSTALL_SPEC + ? `altimate-engine[${ENGINE_INSTALL_SPEC}]==${ALTIMATE_ENGINE_VERSION}` + : `altimate-engine==${ALTIMATE_ENGINE_VERSION}` + execFileSync(uv, ["pip", "install", "--python", pythonPath, spec], { stdio: "pipe" }) } catch (e: any) { Telemetry.track({ type: "engine_error", @@ -212,6 +225,7 @@ async function ensureEngineImpl(): Promise { uv_version: uvVersion, cli_version: typeof OPENCODE_VERSION === "string" ? OPENCODE_VERSION : "local", installed_at: new Date().toISOString(), + extras: ENGINE_INSTALL_SPEC, }) Telemetry.track({ diff --git a/packages/opencode/test/bridge/client.test.ts b/packages/opencode/test/bridge/client.test.ts index e38f3e91f2..ce76760cd8 100644 --- a/packages/opencode/test/bridge/client.test.ts +++ b/packages/opencode/test/bridge/client.test.ts @@ -21,6 +21,7 @@ mock.module("../../src/altimate/bridge/engine", () => ({ ensureEngineCalls++ }, enginePythonPath: () => managedPythonPath, + ENGINE_INSTALL_SPEC: "warehouses", })) // --------------------------------------------------------------------------- @@ -49,7 +50,7 @@ const cwdVenvPython = path.join(process.cwd(), ".venv", "bin", "python") const hasLocalDevVenv = existsSync(devVenvPython) || existsSync(cwdVenvPython) // --------------------------------------------------------------------------- -// Tests +// Tests — resolvePython priority ordering // --------------------------------------------------------------------------- describe("resolvePython", () => { @@ -112,16 +113,10 @@ describe("resolvePython", () => { return } - // Create both a fake managed venv and a fake cwd venv const fakeManagedPython = path.join(tmpRoot, "managed", "venv", "bin", "python") await createFakeFile(fakeManagedPython) managedPythonPath = fakeManagedPython - // The cwd venv also exists on disk (simulating a user's Python project) - // but the managed venv should win because ensureEngine installs there. - // We can't easily fake cwd, but we verify the managed path is returned - // even when it exists — the ordering in resolvePython guarantees managed - // is checked before cwd. expect(resolvePython()).toBe(fakeManagedPython) }) @@ -144,12 +139,227 @@ describe("resolvePython", () => { }) }) -describe("Bridge.start integration", () => { - // These tests verify that ensureEngine() is called by observing the - // ensureEngineCalls counter. We don't mock child_process, so start() - // will attempt a real spawn — we use /bin/echo which exists but - // won't speak JSON-RPC, causing the bridge ping to fail. +// --------------------------------------------------------------------------- +// Tests — resolvePython env var edge cases +// --------------------------------------------------------------------------- + +describe("resolvePython env var edge cases", () => { + afterEach(async () => { + delete process.env.OPENCODE_PYTHON + managedPythonPath = "/nonexistent/managed-engine/venv/bin/python" + await fsp.rm(tmpRoot, { recursive: true, force: true }).catch(() => {}) + }) + + test("env var with empty string is falsy, falls through to next check", () => { + if (hasLocalDevVenv) { + console.log("Skipping: local dev venv exists") + return + } + + process.env.OPENCODE_PYTHON = "" + // Empty string is falsy, so env var check is skipped + expect(resolvePython()).toBe("python3") + }) + + test("env var pointing to nonexistent path is returned as-is (no validation)", () => { + process.env.OPENCODE_PYTHON = "/does/not/exist/python3" + // resolvePython trusts the env var without checking existence + expect(resolvePython()).toBe("/does/not/exist/python3") + }) + + test("env var with spaces in path is returned correctly", () => { + process.env.OPENCODE_PYTHON = "/path with spaces/python3" + expect(resolvePython()).toBe("/path with spaces/python3") + }) + + test("env var overrides even when dev venv, managed venv, AND cwd venv all exist", async () => { + const fakeManagedPython = path.join(tmpRoot, "managed", "venv", "bin", "python") + await createFakeFile(fakeManagedPython) + managedPythonPath = fakeManagedPython + + process.env.OPENCODE_PYTHON = "/explicit/override" + expect(resolvePython()).toBe("/explicit/override") + }) +}) + +// --------------------------------------------------------------------------- +// Tests — resolvePython managed venv priority +// --------------------------------------------------------------------------- + +describe("resolvePython managed venv takes priority over cwd venv", () => { + afterEach(async () => { + delete process.env.OPENCODE_PYTHON + managedPythonPath = "/nonexistent/managed-engine/venv/bin/python" + await fsp.rm(tmpRoot, { recursive: true, force: true }).catch(() => {}) + }) + test("when managed venv exists, cwd venv is never reached", async () => { + if (existsSync(devVenvPython)) { + console.log("Skipping: local dev venv exists") + return + } + + const fakeManagedPython = path.join(tmpRoot, "managed", "venv", "bin", "python") + await createFakeFile(fakeManagedPython) + managedPythonPath = fakeManagedPython + + // Even if cwd has a .venv, managed should win + const result = resolvePython() + expect(result).toBe(fakeManagedPython) + expect(result).not.toContain(process.cwd()) + }) + + test("managed venv path uses enginePythonPath() which handles platform differences", async () => { + if (hasLocalDevVenv) { + console.log("Skipping: local dev venv exists") + return + } + + // enginePythonPath is mocked, but this tests that resolvePython delegates to it + const customPath = path.join(tmpRoot, "custom-managed", "python") + await createFakeFile(customPath) + managedPythonPath = customPath + + expect(resolvePython()).toBe(customPath) + }) + + test("when managed venv does NOT exist, cwd venv CAN be used as fallback", async () => { + if (hasLocalDevVenv) { + console.log("Skipping: local dev venv exists") + return + } + + // managedPythonPath doesn't exist on disk (default) + // Create a fake cwd venv + const fakeCwdVenv = path.join(process.cwd(), ".venv", "bin", "python") + const cwdVenvExisted = existsSync(fakeCwdVenv) + + if (cwdVenvExisted) { + // cwd venv already exists, so resolvePython should return it + expect(resolvePython()).toBe(fakeCwdVenv) + } else { + // No cwd venv either, falls back to python3 + expect(resolvePython()).toBe("python3") + } + }) +}) + +// --------------------------------------------------------------------------- +// Tests — resolvePython resolution order (source code verification) +// --------------------------------------------------------------------------- + +describe("resolvePython resolution order verification", () => { + test("source code checks managed venv (step 3) before cwd venv (step 4)", async () => { + const clientSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/client.ts", + ) + const source = await fsp.readFile(clientSrc, "utf-8") + + // Find the line numbers for managed venv and cwd venv checks + const lines = source.split("\n") + let managedVenvLine = -1 + let cwdVenvLine = -1 + + for (let i = 0; i < lines.length; i++) { + if (lines[i].includes("enginePythonPath()")) managedVenvLine = i + if (lines[i].includes("process.cwd()") && lines[i].includes(".venv")) cwdVenvLine = i + } + + expect(managedVenvLine).toBeGreaterThan(0) + expect(cwdVenvLine).toBeGreaterThan(0) + // Managed venv MUST come before cwd venv in the source + expect(managedVenvLine).toBeLessThan(cwdVenvLine) + }) + + test("source code uses venvPythonBin helper for platform-aware paths", async () => { + const clientSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/client.ts", + ) + const source = await fsp.readFile(clientSrc, "utf-8") + + // The venvPythonBin function should exist and handle Windows + expect(source).toContain("function venvPythonBin") + expect(source).toContain("Scripts") + expect(source).toContain("python.exe") + + // Dev venv and cwd venv should use venvPythonBin, not hardcoded bin/python + const lines = source.split("\n") + for (const line of lines) { + // Lines that construct dev or cwd venv paths should use venvPythonBin + if (line.includes("altimate-engine") && line.includes(".venv") && line.includes("path.join")) { + expect(line).toContain("venvPythonBin") + } + if (line.includes("process.cwd()") && line.includes(".venv") && line.includes("path.join")) { + expect(line).toContain("venvPythonBin") + } + } + }) + + test("source code has exactly 5 resolution steps", async () => { + const clientSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/client.ts", + ) + const source = await fsp.readFile(clientSrc, "utf-8") + + // Count the numbered comment steps + const stepComments = source.match(/\/\/ \d+\./g) || [] + expect(stepComments.length).toBe(5) + expect(stepComments).toEqual(["// 1.", "// 2.", "// 3.", "// 4.", "// 5."]) + }) +}) + +// --------------------------------------------------------------------------- +// Tests — startup mutex +// --------------------------------------------------------------------------- + +describe("Bridge startup mutex", () => { + test("source code has pendingStart mutex to prevent concurrent start()", async () => { + const clientSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/client.ts", + ) + const source = await fsp.readFile(clientSrc, "utf-8") + + // Verify the mutex pattern exists + expect(source).toContain("pendingStart") + expect(source).toContain("if (pendingStart)") + expect(source).toContain("await pendingStart") + // Verify it's cleaned up in finally + expect(source).toContain("pendingStart = null") + }) + + test("pendingStart is cleared in finally block (not just on success)", async () => { + const clientSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/client.ts", + ) + const source = await fsp.readFile(clientSrc, "utf-8") + + // The mutex must be cleared in a finally block so failed starts don't deadlock + const lines = source.split("\n") + let foundFinally = false + let foundClearAfterFinally = false + + for (let i = 0; i < lines.length; i++) { + if (lines[i].includes("} finally {")) foundFinally = true + if (foundFinally && lines[i].includes("pendingStart = null")) { + foundClearAfterFinally = true + break + } + } + + expect(foundClearAfterFinally).toBe(true) + }) +}) + +// --------------------------------------------------------------------------- +// Tests — Bridge.start integration +// --------------------------------------------------------------------------- + +describe("Bridge.start integration", () => { afterEach(() => { ensureEngineCalls = 0 delete process.env.OPENCODE_PYTHON @@ -159,9 +369,6 @@ describe("Bridge.start integration", () => { test("ensureEngine is called when bridge starts", async () => { const { Bridge } = await import("../../src/altimate/bridge/client") - // process.execPath (the current Bun/Node binary) exists on all platforms. - // When spawned as a Python replacement it exits quickly without speaking - // JSON-RPC, so start() fails on the ping verification as expected. process.env.OPENCODE_PYTHON = process.execPath try { @@ -170,8 +377,217 @@ describe("Bridge.start integration", () => { // Expected: the bridge ping verification will fail } - // Even though the ping failed, ensureEngine was called before the spawn attempt expect(ensureEngineCalls).toBeGreaterThanOrEqual(1) Bridge.stop() }) + + test("concurrent Bridge.call() invocations share ensureEngine call", async () => { + const { Bridge } = await import("../../src/altimate/bridge/client") + + process.env.OPENCODE_PYTHON = process.execPath + ensureEngineCalls = 0 + + // Fire multiple calls concurrently — they should coalesce into one start() + const results = await Promise.allSettled([ + Bridge.call("ping", {} as any), + Bridge.call("ping", {} as any), + Bridge.call("ping", {} as any), + ]) + + // All should fail (process.execPath doesn't speak JSON-RPC) + for (const r of results) { + expect(r.status).toBe("rejected") + } + + // ensureEngine should have been called, but not 3 times + // (mutex coalesces concurrent calls) + expect(ensureEngineCalls).toBeGreaterThanOrEqual(1) + Bridge.stop() + }) +}) + +// --------------------------------------------------------------------------- +// Tests — engine.ts source integrity (extras tracking) +// --------------------------------------------------------------------------- + +describe("engine.ts extras tracking", () => { + test("engine.ts exports ENGINE_INSTALL_SPEC constant", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + expect(source).toContain('export const ENGINE_INSTALL_SPEC') + expect(source).toContain('"warehouses"') + }) + + test("engine.ts manifest interface includes extras field", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + expect(source).toContain("extras?:") + }) + + test("engine.ts writeManifest includes extras", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + // Find the writeManifest call and verify it includes extras + expect(source).toMatch(/writeManifest\(\{[\s\S]*extras:\s*ENGINE_INSTALL_SPEC/) + }) + + test("engine.ts ensureEngineImpl checks extras match before returning early", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + // The early return must check extrasMatch + expect(source).toContain("extrasMatch") + expect(source).toMatch(/if\s*\(manifest\s*&&.*extrasMatch\)\s*return/) + }) + + test("engine.ts validates python binary exists before trusting manifest", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + // The early return must check pythonExists + expect(source).toContain("pythonExists") + expect(source).toContain("existsSync(enginePythonPath())") + expect(source).toMatch(/if\s*\(manifest\s*&&.*pythonExists.*\)\s*return/) + }) + + test("engine.ts uses ENGINE_INSTALL_SPEC in pip install command", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + // The install command should reference ENGINE_INSTALL_SPEC, not hardcode extras + expect(source).toContain("ENGINE_INSTALL_SPEC") + expect(source).toContain("`altimate-engine[${ENGINE_INSTALL_SPEC}]") + }) +}) + +// --------------------------------------------------------------------------- +// Tests — engine.ts ensureEngineImpl validation logic +// --------------------------------------------------------------------------- + +describe("engine.ts ensureEngineImpl validation conditions", () => { + test("early return requires ALL four conditions: manifest + version + pythonExists + extrasMatch", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + // Find the early return line + const lines = source.split("\n") + const earlyReturnLine = lines.find(l => + l.includes("manifest") && + l.includes("ALTIMATE_ENGINE_VERSION") && + l.includes("pythonExists") && + l.includes("extrasMatch") && + l.includes("return") + ) + + expect(earlyReturnLine).toBeDefined() + // All conditions must be ANDed together + expect(earlyReturnLine).toContain("&&") + // Should have exactly 3 && operators (4 conditions) + const andCount = (earlyReturnLine!.match(/&&/g) || []).length + expect(andCount).toBe(3) + }) + + test("extrasMatch defaults empty string when manifest.extras is undefined (old manifests)", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + // Old manifests won't have extras field — must use nullish coalescing + expect(source).toMatch(/manifest\?\.extras\s*\?\?\s*""/) + }) +}) + +// --------------------------------------------------------------------------- +// Tests — Windows path handling in venvPythonBin +// --------------------------------------------------------------------------- + +describe("venvPythonBin platform handling", () => { + test("source code has venvPythonBin function with Windows support", async () => { + const clientSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/client.ts", + ) + const source = await fsp.readFile(clientSrc, "utf-8") + + // Must handle both platforms + expect(source).toContain("function venvPythonBin") + expect(source).toContain('process.platform === "win32"') + expect(source).toContain("Scripts") + expect(source).toContain("python.exe") + expect(source).toContain('"bin"') + expect(source).toContain('"python"') + }) + + test("dev venv path uses venvPythonBin (not hardcoded bin/python)", async () => { + const clientSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/client.ts", + ) + const source = await fsp.readFile(clientSrc, "utf-8") + + // The dev venv path construction spans two lines: + // const engineDir = path.resolve(..., "altimate-engine") + // const venvPython = venvPythonBin(path.join(engineDir, ".venv")) + // Verify the venvPython assignment uses venvPythonBin + const lines = source.split("\n") + const devVenvLine = lines.find(l => + l.includes("venvPython") && l.includes("venvPythonBin") && l.includes(".venv") + ) + expect(devVenvLine).toBeDefined() + // Must NOT use hardcoded "bin", "python" path segments + expect(devVenvLine).not.toMatch(/["']bin["'].*["']python["']/) + }) + + test("cwd venv path uses venvPythonBin (not hardcoded bin/python)", async () => { + const clientSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/client.ts", + ) + const source = await fsp.readFile(clientSrc, "utf-8") + + // The line constructing the cwd venv path should call venvPythonBin + const lines = source.split("\n") + const cwdVenvLine = lines.find(l => + l.includes("process.cwd()") && l.includes(".venv") + ) + expect(cwdVenvLine).toBeDefined() + expect(cwdVenvLine).toContain("venvPythonBin") + }) + + test("enginePythonPath in engine.ts also handles Windows paths", async () => { + const engineSrc = path.resolve( + __dirname, + "../../src/altimate/bridge/engine.ts", + ) + const source = await fsp.readFile(engineSrc, "utf-8") + + // enginePythonPath should have the same platform check + expect(source).toMatch(/enginePythonPath[\s\S]*?win32[\s\S]*?Scripts[\s\S]*?python\.exe/) + }) }) From f55858e2569537ed480803d9c5f5616102894e34 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 16 Mar 2026 16:16:54 -0700 Subject: [PATCH 4/7] fix: clean up child process on ping failure + use platform-aware paths in tests Address Sentry bot review comments on PR #199: 1. When `start()` spawns a child but `ping` verification fails, kill and clear the `child` handle so subsequent `call()` invocations trigger a restart instead of writing to a broken process and hanging for 30s timeout. 2. Use platform-aware `testVenvPythonBin()` helper in test file for dev/cwd venv detection, matching production `venvPythonBin()` behavior. Fixes incorrect test skip logic on Windows. --- packages/opencode/src/altimate/bridge/client.ts | 5 +++++ packages/opencode/test/bridge/client.test.ts | 11 +++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/altimate/bridge/client.ts b/packages/opencode/src/altimate/bridge/client.ts index 589c38cb11..76f8a82738 100644 --- a/packages/opencode/src/altimate/bridge/client.ts +++ b/packages/opencode/src/altimate/bridge/client.ts @@ -175,6 +175,11 @@ export namespace Bridge { try { await call("ping", {} as any) } catch (e) { + // Clean up the spawned process so subsequent call() invocations + // correctly detect !child and trigger a restart instead of writing + // to a non-functional process and hanging until timeout. + child?.kill() + child = undefined throw new Error(`Failed to start Python bridge: ${e}`) } } diff --git a/packages/opencode/test/bridge/client.test.ts b/packages/opencode/test/bridge/client.test.ts index ce76760cd8..ef40730de8 100644 --- a/packages/opencode/test/bridge/client.test.ts +++ b/packages/opencode/test/bridge/client.test.ts @@ -41,12 +41,19 @@ async function createFakeFile(filePath: string) { await fsp.writeFile(filePath, "") } +// Platform-aware venv python path (matches venvPythonBin in production code) +function testVenvPythonBin(venvDir: string): string { + return process.platform === "win32" + ? path.join(venvDir, "Scripts", "python.exe") + : path.join(venvDir, "bin", "python") +} + // Paths that resolvePython() checks for dev/cwd venvs. // From source file: __dirname is /packages/altimate-code/src/bridge/ // From test file: __dirname is /packages/altimate-code/test/bridge/ // Both resolve 3 levels up to /packages/, so the dev venv path is identical. -const devVenvPython = path.resolve(__dirname, "..", "..", "..", "altimate-engine", ".venv", "bin", "python") -const cwdVenvPython = path.join(process.cwd(), ".venv", "bin", "python") +const devVenvPython = testVenvPythonBin(path.resolve(__dirname, "..", "..", "..", "altimate-engine", ".venv")) +const cwdVenvPython = testVenvPythonBin(path.join(process.cwd(), ".venv")) const hasLocalDevVenv = existsSync(devVenvPython) || existsSync(cwdVenvPython) // --------------------------------------------------------------------------- From 7afe33adc93d8f0b146a5078690db644bad54175 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 16 Mar 2026 16:19:42 -0700 Subject: [PATCH 5/7] fix: address multi-model code review feedback - Remove dead else branch in `ENGINE_INSTALL_SPEC` conditional (constant is always truthy) - Add `extras` field to `engine_started` telemetry event for debugging - Update `ENGINE_INSTALL_SPEC` comment to reflect production usage - Tighten concurrent startup test assertion to verify mutex coalescing (`ensureEngineCalls <= 2` instead of just `>= 1`) --- packages/opencode/src/altimate/bridge/engine.ts | 8 ++++---- packages/opencode/src/altimate/telemetry/index.ts | 1 + packages/opencode/test/bridge/client.test.ts | 6 ++++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/packages/opencode/src/altimate/bridge/engine.ts b/packages/opencode/src/altimate/bridge/engine.ts index 982de16aa3..5e1c6d87f2 100644 --- a/packages/opencode/src/altimate/bridge/engine.ts +++ b/packages/opencode/src/altimate/bridge/engine.ts @@ -25,7 +25,8 @@ declare const OPENCODE_VERSION: string // Mutex to prevent concurrent ensureEngine/ensureUv calls from corrupting state let pendingEnsure: Promise | null = null -/** The pip install spec used by ensureEngine — exported for tests. */ +/** Pip extras spec for altimate-engine (e.g. "warehouses" → altimate-engine[warehouses]). + * Used in ensureEngine install command and recorded in manifest for upgrade detection. */ export const ENGINE_INSTALL_SPEC = "warehouses" interface Manifest { @@ -199,9 +200,7 @@ async function ensureEngineImpl(): Promise { const pythonPath = enginePythonPath() Log.Default.info("installing altimate-engine", { version: ALTIMATE_ENGINE_VERSION }) try { - const spec = ENGINE_INSTALL_SPEC - ? `altimate-engine[${ENGINE_INSTALL_SPEC}]==${ALTIMATE_ENGINE_VERSION}` - : `altimate-engine==${ALTIMATE_ENGINE_VERSION}` + const spec = `altimate-engine[${ENGINE_INSTALL_SPEC}]==${ALTIMATE_ENGINE_VERSION}` execFileSync(uv, ["pip", "install", "--python", pythonPath, spec], { stdio: "pipe" }) } catch (e: any) { Telemetry.track({ @@ -234,6 +233,7 @@ async function ensureEngineImpl(): Promise { session_id: Telemetry.getContext().sessionId, engine_version: ALTIMATE_ENGINE_VERSION, python_version: pyVersion, + extras: ENGINE_INSTALL_SPEC, status: isUpgrade ? "upgraded" : "started", duration_ms: Date.now() - startTime, }) diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index 7c5f193b4b..9c501f2664 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -153,6 +153,7 @@ export namespace Telemetry { session_id: string engine_version: string python_version: string + extras?: string status: "started" | "restarted" | "upgraded" duration_ms: number } diff --git a/packages/opencode/test/bridge/client.test.ts b/packages/opencode/test/bridge/client.test.ts index ef40730de8..c9103e2fa9 100644 --- a/packages/opencode/test/bridge/client.test.ts +++ b/packages/opencode/test/bridge/client.test.ts @@ -406,9 +406,11 @@ describe("Bridge.start integration", () => { expect(r.status).toBe("rejected") } - // ensureEngine should have been called, but not 3 times - // (mutex coalesces concurrent calls) + // The startup mutex should coalesce concurrent calls into a single + // ensureEngine invocation. In JS's single-threaded model, the first + // call sets pendingStart before any await, so subsequent calls join it. expect(ensureEngineCalls).toBeGreaterThanOrEqual(1) + expect(ensureEngineCalls).toBeLessThanOrEqual(2) Bridge.stop() }) }) From c41ed7b484d32dd1ee63c0334653f302360554ae Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 16 Mar 2026 16:23:54 -0700 Subject: [PATCH 6/7] fix: handle spawn errors, venv recovery, and post-mutex child guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Gemini review findings: 1. Add `error` event handler on spawned child process to prevent unhandled ENOENT/EACCES from crashing the host process when `resolvePython()` returns a nonexistent or non-executable path. 2. Recreate venv when Python binary is missing even if the venv directory still exists (e.g. user deleted just the binary). 3. Re-check `child` state after awaiting `pendingStart` mutex — the process may have died between startup completing and the secondary caller resuming. --- packages/opencode/src/altimate/bridge/client.ts | 14 ++++++++++++++ packages/opencode/src/altimate/bridge/engine.ts | 5 +++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/altimate/bridge/client.ts b/packages/opencode/src/altimate/bridge/client.ts index 76f8a82738..406e2e7ff3 100644 --- a/packages/opencode/src/altimate/bridge/client.ts +++ b/packages/opencode/src/altimate/bridge/client.ts @@ -67,6 +67,10 @@ export namespace Bridge { if (restartCount >= MAX_RESTARTS) throw new Error("Python bridge failed after max restarts") if (pendingStart) { await pendingStart + // Re-check: the process may have died between startup and now + if (!child || child.exitCode !== null) { + throw new Error("Bridge process died during startup") + } } else { pendingStart = start() try { @@ -162,6 +166,16 @@ export namespace Bridge { if (msg) Log.Default.error("altimate-engine stderr", { message: msg }) }) + child.on("error", (err) => { + Log.Default.error("altimate-engine spawn error", { error: String(err) }) + restartCount++ + for (const [id, p] of pending) { + p.reject(new Error(`Bridge process failed to spawn: ${err}`)) + pending.delete(id) + } + child = undefined + }) + child.on("exit", (code) => { if (code !== 0) restartCount++ for (const [id, p] of pending) { diff --git a/packages/opencode/src/altimate/bridge/engine.ts b/packages/opencode/src/altimate/bridge/engine.ts index 5e1c6d87f2..d2fc4bbfc2 100644 --- a/packages/opencode/src/altimate/bridge/engine.ts +++ b/packages/opencode/src/altimate/bridge/engine.ts @@ -179,8 +179,9 @@ async function ensureEngineImpl(): Promise { const dir = engineDir() const venvDir = path.join(dir, "venv") - // Create venv if it doesn't exist - if (!existsSync(venvDir)) { + // Create venv if it doesn't exist, or recreate if the Python binary is missing + // (e.g. user deleted the binary but left the venv directory intact) + if (!existsSync(venvDir) || !pythonExists) { Log.Default.info("creating python environment") try { execFileSync(uv, ["venv", "--python", "3.12", venvDir], { stdio: "pipe" }) From df3fdebd61d277bf7bf1dcfa2246713b3df47cc8 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Mon, 16 Mar 2026 16:25:04 -0700 Subject: [PATCH 7/7] fix: don't increment `restartCount` when process is killed by signal When `child.kill()` is called (e.g. after ping failure), the exit handler fires with `code = null`. The check `code !== 0` treated this as a crash, incrementing `restartCount`. After two ping failures, the bridge would be permanently disabled. Changed to `code !== null && code !== 0` so only actual non-zero exit codes (real crashes) count toward the restart limit. --- packages/opencode/src/altimate/bridge/client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/bridge/client.ts b/packages/opencode/src/altimate/bridge/client.ts index 406e2e7ff3..4ea1b3ccbd 100644 --- a/packages/opencode/src/altimate/bridge/client.ts +++ b/packages/opencode/src/altimate/bridge/client.ts @@ -177,7 +177,7 @@ export namespace Bridge { }) child.on("exit", (code) => { - if (code !== 0) restartCount++ + if (code !== null && code !== 0) restartCount++ for (const [id, p] of pending) { p.reject(new Error(`Bridge process exited (code ${code})`)) pending.delete(id)