fix(coding-agent): made plugin install transaction atomic on validation failure

The #3063 fix introduced a second mutating step — `bun update <name>` — 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/<name> 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/<name> 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.
2026-06-19 18:33:33 +00:00
parent 0ada5fe168
commit a63da8a3a3
3 changed files with 245 additions and 18 deletions
@@ -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 <spec>` 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 <name>` 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 <spec>` 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 <name>` 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

@@ -248,11 +248,29 @@ export class PluginManager {
 	}

 	async #rollbackFailedInstall(
-		actualName: string,
+		actualName: string | undefined,
 		packageJsonBefore: string,
+		bunLockBefore: string | null,
 		snapshot: PluginPackageSnapshot | null,
 	): Promise<void> {
 		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 <name>` 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);
 		}
@@ -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" });
+	});
 });