From af52e1de0fe6190baf6a3f77fb552e0842002a3c Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 11 Aug 2026 15:55:48 +0200 Subject: [PATCH] refactor(coding-agent): verified binary updates by explicit path - Extracted version verification logic into a reusable function accepting an explicit binary path. - Updated shim takeover to verify the newly placed executable path directly instead of re-resolving via PATH. --- packages/coding-agent/src/cli/update-cli.ts | 83 +++++++++--- packages/coding-agent/test/update-cli.test.ts | 127 +++++++++++++----- 2 files changed, 162 insertions(+), 48 deletions(-) diff --git a/packages/coding-agent/src/cli/update-cli.ts b/packages/coding-agent/src/cli/update-cli.ts index 23c2e4d70..80107b418 100644 --- a/packages/coding-agent/src/cli/update-cli.ts +++ b/packages/coding-agent/src/cli/update-cli.ts @@ -849,24 +849,31 @@ function resolveOmpPath(): string | undefined { } /** - * Run the resolved omp binary and check if it reports the expected version. + * Run a specific binary and check if it reports the expected version. */ -async function verifyInstalledVersion(expectedVersion: string): Promise { - const ompPath = resolveOmpPath(); - if (!ompPath) return { ok: false }; +async function verifyBinaryAtPath(binaryPath: string, expectedVersion: string): Promise { try { - const result = await $`${ompPath} --version`.quiet().nothrow(); - if (result.exitCode !== 0) return { ok: false, path: ompPath }; + const result = await $`${binaryPath} --version`.quiet().nothrow(); + if (result.exitCode !== 0) return { ok: false, path: binaryPath }; 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 }; + return { ok: actual === expectedVersion, actual, path: binaryPath }; } catch { - return { ok: false, path: ompPath }; + return { ok: false, path: binaryPath }; } } +/** + * Run the PATH-resolved omp binary and check if it reports the expected version. + */ +async function verifyInstalledVersion(expectedVersion: string): Promise { + const ompPath = resolveOmpPath(); + if (!ompPath) return { ok: false }; + return await verifyBinaryAtPath(ompPath, expectedVersion); +} + function printVerifiedVersion(expectedVersion: string): void { console.log(chalk.green(`\n${theme.status.success} Updated to ${expectedVersion}`)); } @@ -1169,6 +1176,21 @@ export async function updateViaBinaryAt( console.log(chalk.dim(`Restart ${APP_NAME} to use the new version`)); } +/** + * In-place forwarder bodies, by shim extension, for launchers that cannot be + * renamed aside during a script-shim takeover; each execs the sibling + * `omp.exe`. Rewriting matters for the shims that outrank `.exe` at command + * resolution: PowerShell prefers `.ps1` and Git Bash resolves the + * extensionless sh shim first, so leaving the old body behind would keep + * launching the replaced install. + */ +const SHIM_FORWARDERS: Record = { + "": `#!/bin/sh\nexec "$(dirname "$0")/${APP_NAME}.exe" "$@"\n`, + ".cmd": `@"%~dp0${APP_NAME}.exe" %*\r\n`, + ".bat": `@"%~dp0${APP_NAME}.exe" %*\r\n`, + ".ps1": `& "$PSScriptRoot\\${APP_NAME}.exe" @args\nexit $LASTEXITCODE\n`, +}; + /** * Take over a Windows script-launcher install for a binary-only release. * @@ -1180,7 +1202,8 @@ export async function updateViaBinaryAt( * once the shims are out of the way. A working launcher exists at every * step — the exe lands before any shim moves, a shim that refuses to move * (a running `.cmd` can be renamed but may be held open some other way) is - * skipped, and a failed version verification moves everything back. + * rewritten in place as a forwarder to the exe, and a failed version + * verification moves everything back. */ export async function updateViaShimTakeover( shimPath: string, @@ -1189,7 +1212,7 @@ export async function updateViaShimTakeover( binaryName?: string; fetchImpl?: Fetch; githubToken?: string; - verifyInstalledVersion?: typeof verifyInstalledVersion; + verifyBinary?: typeof verifyBinaryAtPath; } = {}, ): Promise { const binaryName = options.binaryName ?? getBinaryName(); @@ -1211,28 +1234,48 @@ export async function updateViaShimTakeover( await fs.promises.rename(tempPath, exePath); // Retire the shims so PATH resolution lands on the new exe. Renamed, not // deleted: restorable on verification failure, and Windows permits - // renaming a batch file that is still executing. + // renaming a batch file that is still executing. A shim that cannot be + // renamed (held open without delete sharing) is rewritten in place as a + // forwarder to the exe — write and rename take different Windows locks, + // so one can succeed where the other fails. const backupSuffix = `${Date.now()}.${process.pid}.bak`; const retired: Array<{ launcher: string; backup: string }> = []; + const forwarded: Array<{ launcher: string; original: string }> = []; + const stuck: string[] = []; for (const ext of ["", ".cmd", ".ps1", ".bat"]) { const launcher = path.join(launcherDir, `${APP_NAME}${ext}`); const backup = `${launcher}.${backupSuffix}`; try { await fs.promises.rename(launcher, backup); retired.push({ launcher, backup }); - } catch { - // Shim absent or immovable; .exe still outranks .cmd/.bat in PATHEXT. + } catch (err) { + if (isEnoent(err)) continue; + try { + const original = await Bun.file(launcher).text(); + await Bun.write(launcher, SHIM_FORWARDERS[ext]); + forwarded.push({ launcher, original }); + } catch { + stuck.push(launcher); + } } } - const verify = options.verifyInstalledVersion ?? verifyInstalledVersion; - const verification = await verify(expectedVersion); + // Verify the exe by its explicit path: $which cached the shim path when + // the update target was resolved, and the shim was just renamed away, so + // a PATH re-resolution here would test a file that no longer exists. + const verify = options.verifyBinary ?? verifyBinaryAtPath; + const verification = await verify(exePath, expectedVersion); if (!verification.ok) { for (const { launcher, backup } of retired) { try { await fs.promises.rename(backup, launcher); } catch {} } + for (const { launcher, original } of forwarded) { + try { + await Bun.write(launcher, original); + } catch {} + } await unlinkIfExists(exePath); throw new Error( `${formatVerificationFailure(verification, expectedVersion)}; restored previous ${APP_NAME} launcher`, @@ -1245,6 +1288,16 @@ export async function updateViaShimTakeover( for (const ext of [".exe", "", ".cmd", ".ps1", ".bat"]) { await sweepStaleBackups(path.join(launcherDir, `${APP_NAME}${ext}`)); } + for (const { launcher } of forwarded) { + console.log(chalk.dim(`Converted ${launcher} to a forwarder (it could not be removed).`)); + } + for (const launcher of stuck) { + console.log( + chalk.yellow( + `Could not retire ${launcher}; shells that prefer it may keep launching the old version until it is deleted manually.`, + ), + ); + } 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 255e4d809..76484d380 100644 --- a/packages/coding-agent/test/update-cli.test.ts +++ b/packages/coding-agent/test/update-cli.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, spyOn, vi } from "bun:test"; +import { afterEach, describe, expect, it, type Mock, spyOn, vi } from "bun:test"; import { createHash } from "node:crypto"; import * as nodeFs from "node:fs"; import * as fs from "node:fs/promises"; @@ -783,32 +783,33 @@ describe("update-cli script-shim takeover", () => { const version = "18.0.0"; const binaryName = "omp-windows-x64.exe"; const url = `https://github.com/can1357/oh-my-pi/releases/download/v${version}/${binaryName}`; - const content = "native release binary"; - const digest = `sha256:${createHash("sha256").update(content).digest("hex")}`; - const fetchImpl = async (input: string | URL | Request): Promise => { - const requestUrl = String(input); - if (requestUrl.startsWith("https://api.github.com/")) { - return new Response( - JSON.stringify({ - tag_name: `v${version}`, - draft: false, - prerelease: false, - assets: [ - { - name: binaryName, - state: "uploaded", - size: Buffer.byteLength(content), - digest, - browser_download_url: url, - }, - ], - }), - ); - } - if (requestUrl === url) return new Response(content); - throw new Error(`Unexpected request: ${requestUrl}`); - }; + function makeFetch(content: string): (input: string | URL | Request) => Promise { + const digest = `sha256:${createHash("sha256").update(content).digest("hex")}`; + return async (input: string | URL | Request): Promise => { + const requestUrl = String(input); + if (requestUrl.startsWith("https://api.github.com/")) { + return new Response( + JSON.stringify({ + tag_name: `v${version}`, + draft: false, + prerelease: false, + assets: [ + { + name: binaryName, + state: "uploaded", + size: Buffer.byteLength(content), + digest, + browser_download_url: url, + }, + ], + }), + ); + } + if (requestUrl === url) return new Response(content); + throw new Error(`Unexpected request: ${requestUrl}`); + }; + } const shims: Record = { omp: "#!/bin/sh\nnode omp.js\n", @@ -825,15 +826,18 @@ describe("update-cli script-shim takeover", () => { it("installs omp.exe beside the shims and retires them", async () => { const dir = await makeTempDir(); await writeShims(dir); + // Real executable, no injected verifier: the takeover must verify the + // exe by explicit path — $which cached the shim path before it was + // renamed away, so a PATH re-resolution would fail here. + const exe = `#!/bin/sh\necho omp/${version}\n`; await updateViaShimTakeover(path.join(dir, "omp.cmd"), version, { binaryName, - fetchImpl, + fetchImpl: makeFetch(exe), githubToken: "test-token", - verifyInstalledVersion: async () => ({ ok: true, actual: version, path: path.join(dir, "omp.exe") }), }); - expect(await Bun.file(path.join(dir, "omp.exe")).text()).toBe(content); + expect(await Bun.file(path.join(dir, "omp.exe")).text()).toBe(exe); for (const name in shims) { expect(await Bun.file(path.join(dir, name)).exists()).toBe(false); } @@ -841,18 +845,19 @@ describe("update-cli script-shim takeover", () => { expect(residue).toEqual([]); }); - it("restores the shims and removes the exe when verification fails", async () => { + it("restores the shims and removes the exe when the exe reports the wrong version", async () => { const dir = await makeTempDir(); await writeShims(dir); + // Executable runs but reports the previous version -> full rollback. + const exe = "#!/bin/sh\necho omp/17.2.12\n"; await expect( updateViaShimTakeover(path.join(dir, "omp.cmd"), version, { binaryName, - fetchImpl, + fetchImpl: makeFetch(exe), githubToken: "test-token", - verifyInstalledVersion: async () => ({ ok: false, actual: "17.2.12", path: path.join(dir, "omp.cmd") }), }), - ).rejects.toThrow("restored previous omp launcher"); + ).rejects.toThrow(/still reports 17\.2\.12 \(expected 18\.0\.0\); restored previous omp launcher/); expect(await Bun.file(path.join(dir, "omp.exe")).exists()).toBe(false); for (const name in shims) { @@ -861,4 +866,60 @@ describe("update-cli script-shim takeover", () => { const residue = (await fs.readdir(dir)).filter(name => name.endsWith(".bak") || name.endsWith(".new")); expect(residue).toEqual([]); }); + + function renameLockingPs1(): Mock { + const realRename = nodeFs.promises.rename; + return spyOn(nodeFs.promises, "rename").mockImplementation(async (from, to) => { + if (path.basename(String(from)) === "omp.ps1") { + throw Object.assign(new Error("EPERM: file is locked"), { code: "EPERM" }); + } + return await realRename(from, to); + }); + } + + it("rewrites an immovable precedence-winning shim as a forwarder to the exe", async () => { + const dir = await makeTempDir(); + await writeShims(dir); + const exe = `#!/bin/sh\necho omp/${version}\n`; + const renameSpy = renameLockingPs1(); + try { + await updateViaShimTakeover(path.join(dir, "omp.cmd"), version, { + binaryName, + fetchImpl: makeFetch(exe), + githubToken: "test-token", + }); + } finally { + renameSpy.mockRestore(); + } + + expect(await Bun.file(path.join(dir, "omp.exe")).text()).toBe(exe); + expect(await Bun.file(path.join(dir, "omp")).exists()).toBe(false); + expect(await Bun.file(path.join(dir, "omp.cmd")).exists()).toBe(false); + // PowerShell resolves .ps1 before .exe: the locked shim must now exec + // the new binary instead of keeping its old body. + expect(await Bun.file(path.join(dir, "omp.ps1")).text()).toContain('& "$PSScriptRoot\\omp.exe" @args'); + }); + + it("restores a forwarded shim's original body when verification fails", async () => { + const dir = await makeTempDir(); + await writeShims(dir); + const exe = "#!/bin/sh\necho omp/17.2.12\n"; + const renameSpy = renameLockingPs1(); + try { + await expect( + updateViaShimTakeover(path.join(dir, "omp.cmd"), version, { + binaryName, + fetchImpl: makeFetch(exe), + githubToken: "test-token", + }), + ).rejects.toThrow("restored previous omp launcher"); + } finally { + renameSpy.mockRestore(); + } + + expect(await Bun.file(path.join(dir, "omp.exe")).exists()).toBe(false); + for (const name in shims) { + expect(await Bun.file(path.join(dir, name)).text()).toBe(shims[name]); + } + }); });