From a63da8a3a381e931d650fbe3d4344bb340e20dcf Mon Sep 17 00:00:00 2001 From: roboomp Date: Fri, 19 Jun 2026 18:33:33 +0000 Subject: [PATCH] fix(coding-agent): made plugin install transaction atomic on validation failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #3063 fix introduced a second mutating step — `bun update ` — that rewrites bun.lock before extension validation runs. Three failure paths could still leave the rejected commit pinned in the lockfile or active tree: - Extension validation throwing after `bun update` had refreshed bun.lock — rollback restored package.json and node_modules/ but never touched bun.lock. - Feature validation (`omp plugin install pkg[ghost]`) throwing outside the rollback block entirely. - Runtime-config save failing after a successful install with no rollback path. Snapshot bun.lock alongside package.json before `bun install` runs and route every post-install step (resolution, update, package.json read, feature validation, extension validation, runtime-config save) through one outer catch that restores all three (package.json + bun.lock + node_modules/ from snapshot). `#rollbackFailedInstall` now tolerates an unresolved `actualName` for failures that throw before the dep key is known. Three regression tests in plugin-install-validation.test.ts pin the new contract: bun.lock restoration after a git reinstall fails validation, bun.lock removal when it didn't exist pre-install, and rollback on an unknown feature request. Addresses review feedback on #3069. --- packages/coding-agent/CHANGELOG.md | 2 +- .../src/extensibility/plugins/manager.ts | 70 +++++-- .../test/plugin-install-validation.test.ts | 191 ++++++++++++++++++ 3 files changed, 245 insertions(+), 18 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 2bfbe6fa2..a0cbd1db2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixed -- Fixed `omp plugin install github:owner/repo` silently keeping the user on a stale commit when re-run on an already-installed GitHub plugin. `bun install ` respects the existing `bun.lock` pin when the spec is unchanged and never re-resolves the remote ref, so the manager now follows a git re-install with `bun update ` to refresh the lockfile pin against the upstream. First-time installs are unaffected. ([#3063](https://github.com/can1357/oh-my-pi/issues/3063)) +- Fixed `omp plugin install github:owner/repo` silently keeping the user on a stale commit when re-run on an already-installed GitHub plugin. `bun install ` respects the existing `bun.lock` pin when the spec is unchanged and never re-resolves the remote ref, so the manager now follows a git re-install with `bun update ` to refresh the lockfile pin against the upstream. The install transaction also snapshots `bun.lock` up front and routes feature validation, extension validation, and runtime-config save through one rollback path so a failed install can never leave the rejected commit pinned in the active tree or lockfile. First-time installs are unaffected. ([#3063](https://github.com/can1357/oh-my-pi/issues/3063)) ## [16.1.3] - 2026-06-19 diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index 7b7264658..1fca10813 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -248,11 +248,29 @@ export class PluginManager { } async #rollbackFailedInstall( - actualName: string, + actualName: string | undefined, packageJsonBefore: string, + bunLockBefore: string | null, snapshot: PluginPackageSnapshot | null, ): Promise { await Bun.write(getPluginsPackageJson(), packageJsonBefore); + + // Restore (or remove) bun's lockfile. Without this, a `bun install` + + // `bun update` pair that successfully rewrote `bun.lock` would leave the + // rejected commit pinned even when validation rolls everything else back. + const bunLockPath = path.join(getPluginsDir(), "bun.lock"); + if (bunLockBefore === null) { + await fs.promises.rm(bunLockPath, { force: true }); + } else { + await Bun.write(bunLockPath, bunLockBefore); + } + + // `actualName` may be undefined when the install failed before the dep + // key was resolved — package.json + bun.lock restoration above is the + // complete rollback in that case. + if (!actualName) { + return; + } const packagePath = path.join(getPluginsNodeModules(), actualName); await fs.promises.rm(packagePath, { recursive: true, force: true }); if (!snapshot) { @@ -343,6 +361,19 @@ export class PluginManager { } const pkgJsonPath = getPluginsPackageJson(); const packageJsonBefore = await Bun.file(pkgJsonPath).text(); + // Snapshot bun's lockfile so the rollback path can restore the pin. Every + // step below — `bun install`, `bun update`, feature/extension validation, + // runtime-config save — must either complete entirely or leave the + // lockfile pointing at its pre-install state. Absent before install means + // "remove on rollback". + const bunLockPath = path.join(getPluginsDir(), "bun.lock"); + let bunLockBefore: string | null; + try { + bunLockBefore = await Bun.file(bunLockPath).text(); + } catch (err) { + if (!isEnoent(err)) throw err; + bunLockBefore = null; + } const depsBefore = await this.#readDeps(pkgJsonPath); const packageInstallSpec = gitSource ? gitInstallSpec(spec.packageName, gitSource) : spec.packageName; const existingActualName = gitSource @@ -350,6 +381,10 @@ export class PluginManager { : extractPackageName(spec.packageName); const packageSnapshot = await this.#snapshotInstalledPackage(existingActualName); + // `actualName` is hoisted so the rollback handler can clean up the right + // node_modules entry even if a step between `bun install` and the final + // validation throws. + let actualName: string | undefined; try { // Step 1: write the spec into plugins/package.json + node_modules. const installProc = Bun.spawn(["bun", "install", packageInstallSpec], { @@ -366,7 +401,6 @@ export class PluginManager { } // Resolve actual package name. npm specs encode the name (strip version); // git specs do not, so diff plugins/package.json deps to find the new entry. - let actualName: string; if (gitSource) { const depsAfter = await this.#readDeps(pkgJsonPath); let resolved: string | undefined; @@ -401,7 +435,7 @@ export class PluginManager { // `bun update ` re-resolves the ref against the remote and // rewrites the pin; SHA-pinned refs stay put because the commit can't // move. First-time installs skip this — the initial `bun install` already - // fetched HEAD. + // fetched HEAD. Rollback is handled by the outer catch. if (gitSource && existingActualName) { const updateProc = Bun.spawn(["bun", "update", actualName], { cwd: getPluginsDir(), @@ -413,12 +447,11 @@ export class PluginManager { const updateExit = await updateProc.exited; if (updateExit !== 0) { const stderr = await new Response(updateProc.stderr).text(); - await this.#rollbackFailedInstall(actualName, packageJsonBefore, packageSnapshot); throw new Error(`bun update ${actualName} failed: ${stderr}`); } } - const pkgPath = path.join(getPluginsNodeModules(), actualName, "package.json"); + const pkgPath = path.join(getPluginsNodeModules(), actualName, "package.json"); let pkg: { name: string; version: string; omp?: PluginManifest; pi?: PluginManifest }; try { pkg = await Bun.file(pkgPath).json(); @@ -465,18 +498,7 @@ export class PluginManager { enabled: true, }; - try { - await this.#validateInstalledExtensions(installedPlugin); - } catch (err) { - try { - await this.#rollbackFailedInstall(actualName, packageJsonBefore, packageSnapshot); - } catch (rollbackErr) { - const message = err instanceof Error ? err.message : String(err); - const rollbackMessage = rollbackErr instanceof Error ? rollbackErr.message : String(rollbackErr); - throw new Error(`${message}\nRollback failed: ${rollbackMessage}`); - } - throw err; - } + await this.#validateInstalledExtensions(installedPlugin); // Update runtime config const config = await this.#ensureConfigLoaded(); @@ -488,6 +510,20 @@ export class PluginManager { await this.#saveRuntimeConfig(); return installedPlugin; + } catch (err) { + try { + await this.#rollbackFailedInstall( + actualName ?? existingActualName, + packageJsonBefore, + bunLockBefore, + packageSnapshot, + ); + } catch (rollbackErr) { + const message = err instanceof Error ? err.message : String(err); + const rollbackMessage = rollbackErr instanceof Error ? rollbackErr.message : String(rollbackErr); + throw new Error(`${message}\nRollback failed: ${rollbackMessage}`); + } + throw err; } finally { await this.#cleanupSnapshot(packageSnapshot); } diff --git a/packages/coding-agent/test/plugin-install-validation.test.ts b/packages/coding-agent/test/plugin-install-validation.test.ts index f48afcfa3..39ce2f601 100644 --- a/packages/coding-agent/test/plugin-install-validation.test.ts +++ b/packages/coding-agent/test/plugin-install-validation.test.ts @@ -290,4 +290,195 @@ describe("PluginManager.install load validation", () => { expect(await Bun.file(path.join(pluginsNodeModules, "partial-plugin", "package.json")).exists()).toBe(false); expect(await Bun.file(path.join(tmpRoot, "omp-plugins.lock.json")).exists()).toBe(false); }); + + test("restores bun.lock when a git reinstall fails validation (#3069 follow-up)", async () => { + // Pre-existing valid v1 install plus a populated bun.lock pinning the + // original commit. The mock simulates `bun install` rewriting the lock + // to a new pin, then `bun update` rewriting it to a NEWER pin (the case + // the reviewer flagged: bun update mutates bun.lock before validation). + // Extension validation then fails and the rollback must restore the + // ORIGINAL pin — not the install-time pin, not the update-time pin. + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "git-plugin": "github:org/plugin#v1" } }, + null, + 2, + ), + ); + const bunLockPath = path.join(pluginsDir, "bun.lock"); + const ORIGINAL_LOCK = '# bun.lock\n"git-plugin": "github:org/plugin#sha-v1"\n'; + await Bun.write(bunLockPath, ORIGINAL_LOCK); + await Bun.write( + path.join(tmpRoot, "omp-plugins.lock.json"), + JSON.stringify( + { plugins: { "git-plugin": { version: "1.0.0", enabledFeatures: null, enabled: true } }, settings: {} }, + null, + 2, + ), + ); + await writePluginPackage(pluginsNodeModules, "git-plugin", { + version: "1.0.0", + source: 'export default function(pi) { pi.registerCommand("git-old-ok", { handler: async () => {} }); }\n', + }); + + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + if (cmd[1] === "install") { + const prepare = (async () => { + // bun install rewrites the lockfile to a stale-but-different pin + // and stages the broken v2 tree. + await Bun.write(bunLockPath, '# bun.lock\n"git-plugin": "github:org/plugin#sha-install"\n'); + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "git-plugin": "github:org/plugin" } }, + null, + 2, + ), + ); + await writePluginPackage(pluginsNodeModules, "git-plugin", { + version: "2.0.0", + peerDependencies: { "missing-peer": "^1.0.0" }, + source: + 'import { missing } from "missing-peer";\nexport default function(pi) { pi.registerCommand(String(missing), { handler: async () => {} }); }\n', + }); + })(); + return { + pid: 1, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + } + expect(cmd).toEqual(["bun", "update", "git-plugin"]); + const prepare = (async () => { + // bun update re-resolves the ref and rewrites the lockfile pin + // AGAIN. Without bun.lock snapshotting this pin would survive + // the validation failure below. + await Bun.write(bunLockPath, '# bun.lock\n"git-plugin": "github:org/plugin#sha-update"\n'); + })(); + return { + pid: 2, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + }) as typeof Bun.spawn); + + await expect(new PluginManager(tmpRoot).install("github:org/plugin")).rejects.toThrow(/missing-peer/); + + expect(await Bun.file(bunLockPath).text()).toBe(ORIGINAL_LOCK); + const pluginsPackage = await Bun.file(pluginsPkgJson).json(); + expect(pluginsPackage.dependencies).toEqual({ "git-plugin": "github:org/plugin#v1" }); + const restoredPackage = await Bun.file(path.join(pluginsNodeModules, "git-plugin", "package.json")).json(); + expect(restoredPackage.version).toBe("1.0.0"); + }); + + test("removes bun.lock on rollback when it did not exist before install", async () => { + // First-time install of a broken plugin: bun install creates bun.lock, + // extension validation fails, rollback must remove the newly-created + // lockfile so the next install starts clean. + const bunLockPath = path.join(pluginsDir, "bun.lock"); + expect(await Bun.file(bunLockPath).exists()).toBe(false); + + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + expect(cmd).toEqual(["bun", "install", "broken-plugin"]); + const prepare = (async () => { + await Bun.write(bunLockPath, '# bun.lock\n"broken-plugin": "1.0.0"\n'); + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "broken-plugin": "1.0.0" } }, + null, + 2, + ), + ); + await writePluginPackage(pluginsNodeModules, "broken-plugin", { + version: "1.0.0", + peerDependencies: { "missing-peer": "^1.0.0" }, + source: + 'import { missing } from "missing-peer";\nexport default function(pi) { pi.registerCommand(String(missing), { handler: async () => {} }); }\n', + }); + })(); + return { + pid: 1, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + }) as typeof Bun.spawn); + + await expect(new PluginManager(tmpRoot).install("broken-plugin")).rejects.toThrow(/missing-peer/); + + expect(await Bun.file(bunLockPath).exists()).toBe(false); + }); + + test("rolls back when an unknown feature is requested after a git reinstall", async () => { + // Feature validation lives between bun install/update and extension + // validation. Pre-#3069-followup it threw outside the rollback block, + // so an unknown feature would leave the rejected commit + lockfile pin + // in place. Now it must roll back everything. + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "git-plugin": "github:org/plugin" } }, + null, + 2, + ), + ); + const bunLockPath = path.join(pluginsDir, "bun.lock"); + const ORIGINAL_LOCK = '# bun.lock\n"git-plugin": "github:org/plugin#sha-v1"\n'; + await Bun.write(bunLockPath, ORIGINAL_LOCK); + await writePluginPackage(pluginsNodeModules, "git-plugin", { + version: "1.0.0", + source: 'export default function(pi) { pi.registerCommand("git-old-ok", { handler: async () => {} }); }\n', + }); + // The seeded manifest declares one feature `keep`; user will request + // the unknown feature `ghost` instead. + await Bun.write( + path.join(pluginsNodeModules, "git-plugin", "package.json"), + JSON.stringify( + { + name: "git-plugin", + version: "1.0.0", + omp: { + extensions: ["./dist/extension.ts"], + features: { keep: { description: "keep me" } }, + }, + }, + null, + 2, + ), + ); + + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + if (cmd[1] === "install") { + const prepare = (async () => { + await Bun.write(bunLockPath, '# bun.lock\n"git-plugin": "github:org/plugin#sha-install"\n'); + })(); + return { + pid: 1, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + } + expect(cmd).toEqual(["bun", "update", "git-plugin"]); + const prepare = (async () => { + await Bun.write(bunLockPath, '# bun.lock\n"git-plugin": "github:org/plugin#sha-update"\n'); + })(); + return { + pid: 2, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + }) as typeof Bun.spawn); + + await expect(new PluginManager(tmpRoot).install("github:org/plugin[ghost]")).rejects.toThrow(/Unknown feature/); + + expect(await Bun.file(bunLockPath).text()).toBe(ORIGINAL_LOCK); + const pluginsPackage = await Bun.file(pluginsPkgJson).json(); + expect(pluginsPackage.dependencies).toEqual({ "git-plugin": "github:org/plugin" }); + }); });