From b0fef968007433f1b328c29571ce325ac6a44dec Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 3 Mar 2026 05:29:26 +0100 Subject: [PATCH] fix(coding-agent): harden omp update install-target detection Use Bun-native command execution and PATH-prioritized resolution so updates apply to the installation users actually invoke.\n\nFixes #247 --- packages/coding-agent/CHANGELOG.md | 5 + packages/coding-agent/src/cli/update-cli.ts | 194 ++++++++++++------ packages/coding-agent/test/update-cli.test.ts | 22 ++ 3 files changed, 154 insertions(+), 67 deletions(-) create mode 100644 packages/coding-agent/test/update-cli.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index f8326626c..152c54a9d 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,11 @@ ## [Unreleased] +### Fixed + +- Fixed `omp update` silently succeeding without actually updating the binary when the update channel (bun global vs compiled binary) doesn't match the installation method ([#247](https://github.com/can1357/oh-my-pi/issues/247)) +- Added post-update verification that checks the resolved `omp` binary reports the expected version, with actionable warnings on mismatch +- `omp update` now detects when the `omp` in PATH is not managed by bun and falls back to binary replacement instead of updating the wrong location ## [13.6.0] - 2026-03-03 ### Added diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index 41432c4c8..857e44be4 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -4,29 +4,20 @@ * Handles `omp update` to check for and install updates. * Uses bun if available, otherwise downloads binary from GitHub releases. */ -import { execSync, spawnSync } from "node:child_process"; import * as fs from "node:fs"; +import * as path from "node:path"; import { pipeline } from "node:stream/promises"; import { APP_NAME, isEnoent, VERSION } from "@oh-my-pi/pi-utils"; +import { $ } from "bun"; import chalk from "chalk"; import { theme } from "../modes/theme/theme"; -/** - * Detect if we're running as a Bun compiled binary. - */ -const isBunBinary = - Bun.env.PI_COMPILED || - import.meta.url.includes("$bunfs") || - import.meta.url.includes("~BUN") || - import.meta.url.includes("%7EBUN"); - const REPO = "can1357/oh-my-pi"; const PACKAGE = "@oh-my-pi/pi-coding-agent"; interface ReleaseInfo { tag: string; version: string; - assets: Array<{ name: string; url: string }>; } /** @@ -44,18 +35,59 @@ export function parseUpdateArgs(args: string[]): { force: boolean; check: boolea }; } -/** - * Check if bun is available in PATH. - */ -function hasBun(): boolean { +async function getBunGlobalBinDir(): Promise { + if (!Bun.which("bun")) return undefined; try { - const result = spawnSync("bun", ["--version"], { encoding: "utf-8", stdio: "pipe" }); - return result.status === 0; + const result = await $`bun pm bin -g`.quiet().nothrow(); + if (result.exitCode !== 0) return undefined; + const output = result.text().trim(); + return output.length > 0 ? output : undefined; } catch { - return false; + return undefined; } } +function getRealPathOrOriginal(filePath: string): string { + try { + return fs.realpathSync(filePath); + } catch { + return filePath; + } +} + +function normalizePathForComparison(filePath: string): string { + const normalized = path.normalize(filePath); + if (process.platform === "win32") return normalized.toLowerCase(); + return normalized; +} + +function isPathInDirectory(filePath: string, directoryPath: string): boolean { + const normalizedPath = normalizePathForComparison(getRealPathOrOriginal(filePath)); + const normalizedDirectory = normalizePathForComparison(getRealPathOrOriginal(directoryPath)); + const relativePath = path.relative(normalizedDirectory, normalizedPath); + return relativePath === "" || (!relativePath.startsWith("..") && !path.isAbsolute(relativePath)); +} + +interface UpdateTarget { + method: "bun" | "binary"; + path: string; +} + +function resolveUpdateMethod(ompPath: string, bunBinDir: string | undefined): "bun" | "binary" { + if (!bunBinDir) return "binary"; + return isPathInDirectory(ompPath, bunBinDir) ? "bun" : "binary"; +} + +export function _resolveUpdateMethodForTest(ompPath: string, bunBinDir: string | undefined): "bun" | "binary" { + return resolveUpdateMethod(ompPath, bunBinDir); +} +async function resolveUpdateTarget(): Promise { + const ompPath = resolveOmpPath() ?? process.execPath; + const bunBinDir = await getBunGlobalBinDir(); + const method = resolveUpdateMethod(ompPath, bunBinDir); + return { method, path: ompPath }; +} + /** * Get the latest release info from the npm registry. * Uses npm instead of GitHub API to avoid unauthenticated rate limiting. @@ -70,15 +102,9 @@ async function getLatestRelease(): Promise { const version = data.version; const tag = `v${version}`; - // Construct deterministic GitHub release download URLs for the current platform - const makeAsset = (name: string) => ({ - name, - url: `https://github.com/${REPO}/releases/download/${tag}/${name}`, - }); return { tag, version, - assets: [makeAsset(getBinaryName())], }; } @@ -140,60 +166,93 @@ function getBinaryName(): string { return `${APP_NAME}-${os}-${archName}`; } +/** + * Resolve the path that `omp` maps to in the user's PATH. + */ +function resolveOmpPath(): string | undefined { + return Bun.which(APP_NAME) ?? undefined; +} + +/** + * Run the resolved omp binary and check if it reports the expected version. + */ +async function verifyInstalledVersion( + expectedVersion: string, +): Promise<{ ok: boolean; actual?: string; path?: string }> { + const ompPath = resolveOmpPath(); + if (!ompPath) return { ok: false }; + try { + const result = await $`${ompPath} --version`.quiet().nothrow(); + if (result.exitCode !== 0) return { ok: false, path: ompPath }; + const output = result.text().trim(); + // Output format: "omp/X.Y.Z" + const match = output.match(/\/(\d+\.\d+\.\d+)/); + const actual = match?.[1]; + return { ok: actual === expectedVersion, actual, path: ompPath }; + } catch { + return { ok: false, path: ompPath }; + } +} + +/** + * Print post-update verification result. + */ +async function printVerification(expectedVersion: string): Promise { + const result = await verifyInstalledVersion(expectedVersion); + if (result.ok) { + console.log(chalk.green(`\n${theme.status.success} Updated to ${expectedVersion}`)); + return; + } + if (result.actual) { + console.log( + chalk.yellow( + `\nWarning: ${APP_NAME} at ${result.path} still reports ${result.actual} (expected ${expectedVersion})`, + ), + ); + } else { + console.log( + chalk.yellow(`\nWarning: could not verify updated version${result.path ? ` at ${result.path}` : ""}`), + ); + } + console.log( + chalk.yellow( + `You may need to reinstall: curl -fsSL https://raw.githubusercontent.com/${REPO}/main/install.sh | bash`, + ), + ); +} + /** * Update via bun package manager. */ async function updateViaBun(expectedVersion: string): Promise { console.log(chalk.dim("Updating via bun...")); - try { - execSync(`bun install -g ${PACKAGE}@${expectedVersion}`, { stdio: "inherit" }); - } catch (error) { - throw new Error("bun install failed", { cause: error }); + const result = await $`bun install -g ${PACKAGE}@${expectedVersion}`.nothrow(); + if (result.exitCode !== 0) { + throw new Error(`bun install failed with exit code ${result.exitCode}`); } - // Verify the update actually took effect - try { - const result = spawnSync("bun", ["pm", "ls", "-g"], { encoding: "utf-8", stdio: "pipe" }); - const output = result.stdout || ""; - const match = output.match(new RegExp(`${PACKAGE.replace("/", "\\/")}@(\\S+)`)); - if (match) { - const installedVersion = match[1]; - if (compareVersions(installedVersion, expectedVersion) < 0) { - console.log( - chalk.yellow(`\nWarning: bun reports ${installedVersion} installed, expected ${expectedVersion}`), - ); - console.log(chalk.yellow(`Try: bun install -g ${PACKAGE}@latest`)); - return; - } - } - } catch { - // Verification is best-effort, don't fail the update - } - console.log(chalk.green(`\n${theme.status.success} Update complete`)); + await printVerification(expectedVersion); } /** - * Update by downloading binary from GitHub releases. + * Download a release binary to a target path, replacing an existing file. */ -async function updateViaBinary(release: ReleaseInfo): Promise { +async function updateViaBinaryAt(targetPath: string, expectedVersion: string): Promise { const binaryName = getBinaryName(); - const asset = release.assets.find(a => a.name === binaryName); - if (!asset) { - throw new Error(`No binary found for ${binaryName}`); - } - const execPath = process.execPath; - const tempPath = `${execPath}.new`; - const backupPath = `${execPath}.bak`; + const tag = `v${expectedVersion}`; + const url = `https://github.com/${REPO}/releases/download/${tag}/${binaryName}`; + + const tempPath = `${targetPath}.new`; + const backupPath = `${targetPath}.bak`; console.log(chalk.dim(`Downloading ${binaryName}…`)); - // Download binary to temp file - const response = await fetch(asset.url, { redirect: "follow" }); + const response = await fetch(url, { redirect: "follow" }); if (!response.ok || !response.body) { throw new Error(`Download failed: ${response.statusText}`); } const fileStream = fs.createWriteStream(tempPath, { mode: 0o755 }); await pipeline(response.body, fileStream); - // Replace current binary + console.log(chalk.dim("Installing update...")); try { try { @@ -201,15 +260,15 @@ async function updateViaBinary(release: ReleaseInfo): Promise { } catch (err) { if (!isEnoent(err)) throw err; } - await fs.promises.rename(execPath, backupPath); - await fs.promises.rename(tempPath, execPath); + await fs.promises.rename(targetPath, backupPath); + await fs.promises.rename(tempPath, targetPath); await fs.promises.unlink(backupPath); - console.log(chalk.green(`\n${theme.status.success} Updated to ${release.version}`)); + await printVerification(expectedVersion); console.log(chalk.dim(`Restart ${APP_NAME} to use the new version`)); } catch (err) { - if (fs.existsSync(backupPath) && !fs.existsSync(execPath)) { - await fs.promises.rename(backupPath, execPath); + if (fs.existsSync(backupPath) && !fs.existsSync(targetPath)) { + await fs.promises.rename(backupPath, targetPath); } if (fs.existsSync(tempPath)) { await fs.promises.unlink(tempPath); @@ -251,12 +310,13 @@ export async function runUpdateCommand(opts: { force: boolean; check: boolean }) return; } - // Choose update method + // Choose update method based on the prioritized omp binary in PATH try { - if (!isBunBinary && hasBun()) { + const target = await resolveUpdateTarget(); + if (target.method === "bun") { await updateViaBun(release.version); } else { - await updateViaBinary(release); + await updateViaBinaryAt(target.path, release.version); } } catch (err) { console.error(chalk.red(`Update failed: ${err}`)); diff --git a/packages/coding-agent/test/update-cli.test.ts b/packages/coding-agent/test/update-cli.test.ts new file mode 100644 index 000000000..d69780b04 --- /dev/null +++ b/packages/coding-agent/test/update-cli.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it } from "bun:test"; +import { _resolveUpdateMethodForTest } from "../src/cli/update-cli"; + +describe("update-cli install target detection", () => { + it("uses bun update when prioritized omp is inside bun global bin", () => { + const method = _resolveUpdateMethodForTest("/Users/test/.bun/bin/omp", "/Users/test/.bun/bin"); + + expect(method).toBe("bun"); + }); + + it("uses binary update when prioritized omp is outside bun global bin", () => { + const method = _resolveUpdateMethodForTest("/Users/test/.local/bin/omp", "/Users/test/.bun/bin"); + + expect(method).toBe("binary"); + }); + + it("uses binary update when bun global bin cannot be resolved", () => { + const method = _resolveUpdateMethodForTest("/Users/test/.local/bin/omp", undefined); + + expect(method).toBe("binary"); + }); +});