diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bd60d2f0d..0d1bacee5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -334,10 +334,31 @@ jobs: packages/coding-agent/binaries/omp-* generate_release_notes: true + + release_github_verify: + if: ${{ startsWith(github.ref, 'refs/tags/v') && !cancelled() && + needs['release-github'].result == 'success' }} + needs: [release-github] + runs-on: macos-14 + permissions: + contents: read + steps: + - name: Download published macOS arm64 binary + run: | + curl -fsSL -o omp-darwin-arm64 "https://github.com/${{ github.repository }}/releases/download/${{ github.ref_name }}/omp-darwin-arm64" + chmod +x omp-darwin-arm64 + - name: Verify published macOS arm64 binary + run: | + codesign -dv ./omp-darwin-arm64 + runtime_dir="$(mktemp -d)" + HOME="$runtime_dir/home" XDG_DATA_HOME="$runtime_dir/xdg" ./omp-darwin-arm64 --version + release-npm: if: ${{ startsWith(github.ref, 'refs/tags/v') && !cancelled() && - needs.release_binary.result == 'success' && !inputs.skip_npm }} - needs: [release_binary, rust-hash] + needs.release_binary.result == 'success' && + needs.release_github_verify.result == 'success' && + !inputs.skip_npm }} + needs: [release_binary, release_github_verify, rust-hash] runs-on: ubuntu-22.04 steps: - uses: actions/checkout@v4 diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 6526c6481..600f65898 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed binary `omp update` rollbacks so a downloaded replacement that fails post-install version verification no longer remains installed over the previous working binary. ([#1240](https://github.com/can1357/oh-my-pi/issues/1240)) + ## [15.1.8] - 2026-05-20 ### Fixed diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index 9f874f133..60f82e980 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -20,6 +20,22 @@ interface ReleaseInfo { version: string; } +/** Result from running the installed binary and parsing its reported version. */ +export interface InstalledVersionVerification { + ok: boolean; + actual?: string; + path?: string; +} + +/** Paths and verifier used while replacing a downloaded binary update. */ +export interface BinaryReplacementOptions { + targetPath: string; + tempPath: string; + backupPath: string; + expectedVersion: string; + verifyInstalledVersion: (expectedVersion: string) => Promise; +} + /** * Parse update subcommand arguments. * Returns undefined if not an update command. @@ -197,9 +213,7 @@ function resolveOmpPath(): string | 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 }> { +async function verifyInstalledVersion(expectedVersion: string): Promise { const ompPath = resolveOmpPath(); if (!ompPath) return { ok: false }; try { @@ -215,29 +229,69 @@ async function verifyInstalledVersion( } } +function printVerifiedVersion(expectedVersion: string): void { + console.log(chalk.green(`\n${theme.status.success} Updated to ${expectedVersion}`)); +} + +function formatVerificationFailure(result: InstalledVersionVerification, expectedVersion: string): string { + if (result.actual) { + return `${APP_NAME} at ${result.path} still reports ${result.actual} (expected ${expectedVersion})`; + } + return `could not verify updated version${result.path ? ` at ${result.path}` : ""}`; +} + /** * 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}`)); + printVerifiedVersion(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(`\nWarning: ${formatVerificationFailure(result, expectedVersion)}`)); console.log(chalk.yellow(`You may need to reinstall: curl -fsSL https://omp.sh/install | sh`)); } +async function unlinkIfExists(filePath: string): Promise { + try { + await fs.promises.unlink(filePath); + } catch (err) { + if (!isEnoent(err)) throw err; + } +} + +/** + * Atomically replace the installed binary and roll back if version verification fails. + */ +export async function replaceBinaryForUpdate(options: BinaryReplacementOptions): Promise { + let backupReady = false; + try { + await unlinkIfExists(options.backupPath); + await fs.promises.rename(options.targetPath, options.backupPath); + backupReady = true; + await fs.promises.rename(options.tempPath, options.targetPath); + + const verification = await options.verifyInstalledVersion(options.expectedVersion); + if (!verification.ok) { + throw new Error( + `${formatVerificationFailure(verification, options.expectedVersion)}; restored previous ${APP_NAME} binary`, + ); + } + + backupReady = false; + await unlinkIfExists(options.backupPath); + return verification; + } catch (err) { + if (backupReady) { + await unlinkIfExists(options.targetPath); + await fs.promises.rename(options.backupPath, options.targetPath); + } + await unlinkIfExists(options.tempPath); + throw err; + } +} + /** * Update via bun package manager. */ @@ -271,27 +325,15 @@ async function updateViaBinaryAt(targetPath: string, expectedVersion: string): P await pipeline(response.body, fileStream); console.log(chalk.dim("Installing update...")); - try { - try { - await fs.promises.unlink(backupPath); - } catch (err) { - if (!isEnoent(err)) throw err; - } - await fs.promises.rename(targetPath, backupPath); - await fs.promises.rename(tempPath, targetPath); - await fs.promises.unlink(backupPath); - - await printVerification(expectedVersion); - console.log(chalk.dim(`Restart ${APP_NAME} to use the new version`)); - } catch (err) { - if (fs.existsSync(backupPath) && !fs.existsSync(targetPath)) { - await fs.promises.rename(backupPath, targetPath); - } - if (fs.existsSync(tempPath)) { - await fs.promises.unlink(tempPath); - } - throw err; - } + await replaceBinaryForUpdate({ + targetPath, + tempPath, + backupPath, + expectedVersion, + verifyInstalledVersion, + }); + printVerifiedVersion(expectedVersion); + console.log(chalk.dim(`Restart ${APP_NAME} to use the new version`)); } /** diff --git a/packages/coding-agent/test/update-cli.test.ts b/packages/coding-agent/test/update-cli.test.ts index c99837827..90965536f 100644 --- a/packages/coding-agent/test/update-cli.test.ts +++ b/packages/coding-agent/test/update-cli.test.ts @@ -1,6 +1,20 @@ -import { describe, expect, it } from "bun:test"; -import { resolveUpdateMethodForTest } from "../src/cli/update-cli"; +import { afterEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { replaceBinaryForUpdate, resolveUpdateMethodForTest } from "../src/cli/update-cli"; +const tempDirs: string[] = []; + +async function makeTempDir(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-update-test-")); + tempDirs.push(dir); + return dir; +} + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map(dir => fs.rm(dir, { recursive: true, force: true }))); +}); 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"); @@ -20,3 +34,49 @@ describe("update-cli install target detection", () => { expect(method).toBe("binary"); }); }); + +describe("update-cli binary replacement", () => { + it("restores the previous binary when the replacement fails verification", async () => { + const dir = await makeTempDir(); + const targetPath = path.join(dir, "omp"); + const tempPath = `${targetPath}.new`; + const backupPath = `${targetPath}.bak`; + await Bun.write(targetPath, "old binary"); + await Bun.write(tempPath, "broken binary"); + + await expect( + replaceBinaryForUpdate({ + targetPath, + tempPath, + backupPath, + expectedVersion: "15.1.8", + verifyInstalledVersion: async () => ({ ok: false, path: targetPath }), + }), + ).rejects.toThrow("restored previous omp binary"); + + expect(await Bun.file(targetPath).text()).toBe("old binary"); + expect(await Bun.file(tempPath).exists()).toBe(false); + expect(await Bun.file(backupPath).exists()).toBe(false); + }); + + it("keeps the replacement only after it reports the expected version", async () => { + const dir = await makeTempDir(); + const targetPath = path.join(dir, "omp"); + const tempPath = `${targetPath}.new`; + const backupPath = `${targetPath}.bak`; + await Bun.write(targetPath, "old binary"); + await Bun.write(tempPath, "new binary"); + + await replaceBinaryForUpdate({ + targetPath, + tempPath, + backupPath, + expectedVersion: "15.1.8", + verifyInstalledVersion: async () => ({ ok: true, actual: "15.1.8", path: targetPath }), + }); + + expect(await Bun.file(targetPath).text()).toBe("new binary"); + expect(await Bun.file(tempPath).exists()).toBe(false); + expect(await Bun.file(backupPath).exists()).toBe(false); + }); +});