diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index caafcff95..2bfbe6fa2 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### 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)) + ## [16.1.3] - 2026-06-19 ### Changed diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index 7e7e41803..7b7264658 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -351,19 +351,18 @@ export class PluginManager { const packageSnapshot = await this.#snapshotInstalledPackage(existingActualName); try { - // Run npm install - const proc = Bun.spawn(["bun", "install", packageInstallSpec], { + // Step 1: write the spec into plugins/package.json + node_modules. + const installProc = Bun.spawn(["bun", "install", packageInstallSpec], { cwd: getPluginsDir(), stdin: "ignore", stdout: "pipe", stderr: "pipe", windowsHide: true, }); - - const exitCode = await proc.exited; - if (exitCode !== 0) { - const stderr = await new Response(proc.stderr).text(); - throw new Error(`npm install failed: ${stderr}`); + const installExit = await installProc.exited; + if (installExit !== 0) { + const stderr = await new Response(installProc.stderr).text(); + throw new Error(`bun install failed: ${stderr}`); } // 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. @@ -393,6 +392,31 @@ export class PluginManager { } else { actualName = extractPackageName(spec.packageName); } + + // Step 2: refresh the git lockfile pin when re-installing an existing + // git plugin. `bun install ` is a no-op when the spec matches the + // lockfile entry — it never re-resolves the remote ref — so re-running + // `omp plugin install github:owner/repo` would silently keep the user on + // the original resolved commit even after upstream moved (#3063). + // `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. + if (gitSource && existingActualName) { + const updateProc = Bun.spawn(["bun", "update", actualName], { + cwd: getPluginsDir(), + stdin: "ignore", + stdout: "pipe", + stderr: "pipe", + windowsHide: true, + }); + 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"); let pkg: { name: string; version: string; omp?: PluginManifest; pi?: PluginManifest }; diff --git a/packages/coding-agent/test/plugin-install-git.test.ts b/packages/coding-agent/test/plugin-install-git.test.ts index f9a0c69ff..9420202c5 100644 --- a/packages/coding-agent/test/plugin-install-git.test.ts +++ b/packages/coding-agent/test/plugin-install-git.test.ts @@ -160,6 +160,117 @@ describe("PluginManager.install with git sources", () => { expect(result.version).toBe("1.0.0"); }); + test("re-installing a github plugin runs `bun update` to refresh the stale lockfile pin (#3063)", async () => { + // Seed plugins/package.json + node_modules with a previously-installed + // github plugin. `findGitPackageName` matches the new install spec to + // this existing dep (by repository identity), which is the signal the + // manager uses to trigger the lockfile-refresh follow-up. + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { + name: "omp-plugins", + private: true, + dependencies: { "stale-plugin": "github:foo/bar" }, + }, + null, + 2, + ), + ); + const seedDir = path.join(pluginsNodeModules, "stale-plugin"); + await fs.mkdir(seedDir, { recursive: true }); + await Bun.write( + path.join(seedDir, "package.json"), + JSON.stringify({ name: "stale-plugin", version: "0.1.0" }, null, 2), + ); + + const spawnedCommands: string[][] = []; + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + spawnedCommands.push([...cmd]); + if (cmd[1] === "install") { + // `bun install ` is a no-op on the lockfile pin — + // the manager must NOT rely on this call to refresh the commit. + // Leave package.json and node_modules untouched so the test + // fails loudly if the manager skips the follow-up `bun update`. + return { + pid: 1, + stdout: emptyStream(), + stderr: emptyStream(), + exited: Promise.resolve(0), + } as Subprocess; + } + // The follow-up call: simulate bun resolving the upstream HEAD to a + // newer commit and bumping the on-disk version. The manager should + // read the new version from package.json after this step returns. + expect(cmd).toEqual(["bun", "update", "stale-plugin"]); + const prepare = (async () => { + await Bun.write( + path.join(seedDir, "package.json"), + JSON.stringify({ name: "stale-plugin", version: "0.1.6" }, null, 2), + ); + })(); + return { + pid: 2, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + }) as typeof Bun.spawn); + + const mgr = new PluginManager(tmpRoot); + const result = await mgr.install("github:foo/bar"); + + expect(result.version).toBe("0.1.6"); + expect(spawnedCommands).toEqual([ + ["bun", "install", "github:foo/bar"], + ["bun", "update", "stale-plugin"], + ]); + }); + + test("first-time github install does NOT run `bun update` (no existing pin to refresh)", async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify({ name: "omp-plugins", private: true, dependencies: {} }, null, 2), + ); + + const spawnedCommands: string[][] = []; + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + spawnedCommands.push([...cmd]); + expect(cmd[1]).toBe("install"); + const prepare = (async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { + name: "omp-plugins", + private: true, + dependencies: { "fresh-plugin": "github:foo/bar" }, + }, + null, + 2, + ), + ); + const installedDir = path.join(pluginsNodeModules, "fresh-plugin"); + await fs.mkdir(installedDir, { recursive: true }); + await Bun.write( + path.join(installedDir, "package.json"), + JSON.stringify({ name: "fresh-plugin", version: "0.1.6" }, null, 2), + ); + })(); + return { + pid: 1, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + }) as typeof Bun.spawn); + + const mgr = new PluginManager(tmpRoot); + await mgr.install("github:foo/bar"); + + expect(spawnedCommands).toEqual([["bun", "install", "github:foo/bar"]]); + }); + test("rejects git specs containing shell metacharacters", async () => { const mgr = new PluginManager(tmpRoot); await expect(mgr.install("github:foo/bar; rm -rf /")).rejects.toThrow(/Invalid characters in plugin source/); diff --git a/packages/coding-agent/test/plugin-install-validation.test.ts b/packages/coding-agent/test/plugin-install-validation.test.ts index b81f8bd2a..f48afcfa3 100644 --- a/packages/coding-agent/test/plugin-install-validation.test.ts +++ b/packages/coding-agent/test/plugin-install-validation.test.ts @@ -188,30 +188,42 @@ describe("PluginManager.install load validation", () => { }); vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { - expect(cmd).toEqual(["bun", "install", "github:org/plugin#v2"]); + if (cmd[1] === "install") { + expect(cmd).toEqual(["bun", "install", "github:org/plugin#v2"]); - const prepare = (async () => { - await Bun.write( - pluginsPkgJson, - JSON.stringify( - { name: "omp-plugins", private: true, dependencies: { "git-plugin": "github:org/plugin#v2" } }, - 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', - }); - })(); + const prepare = (async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { name: "omp-plugins", private: true, dependencies: { "git-plugin": "github:org/plugin#v2" } }, + 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; + } + // The manager follows a git re-install with `bun update ` to refresh + // the lockfile pin (#3063). The mock treats it as a no-op exit-0 — the + // on-disk state already reflects the v2 install above. + expect(cmd).toEqual(["bun", "update", "git-plugin"]); return { - pid: 1, + pid: 2, stdout: emptyStream(), stderr: emptyStream(), - exited: prepare.then(() => 0), + exited: Promise.resolve(0), } as Subprocess; }) as typeof Bun.spawn);