From 495c570ed46cf9d0c3f57642bebc2d298c1d4d58 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sun, 31 May 2026 04:47:39 +0200 Subject: [PATCH] fix(coding-agent): normalize hosted plugin git shorthands Addresses review feedback on #1527. --- .../src/extensibility/plugins/git-url.ts | 10 ++-- .../src/extensibility/plugins/manager.ts | 22 +++++++-- .../test/plugin-install-git.test.ts | 49 +++++++++++++++++++ 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/packages/coding-agent/src/extensibility/plugins/git-url.ts b/packages/coding-agent/src/extensibility/plugins/git-url.ts index 5dd18af1f..56d10bf63 100644 --- a/packages/coding-agent/src/extensibility/plugins/git-url.ts +++ b/packages/coding-agent/src/extensibility/plugins/git-url.ts @@ -26,9 +26,9 @@ const KNOWN_HOSTS: Record { user: st }; /** - * Namespaced shorthand prefixes recognised by bun's `bun install `, mapped - * to their canonical host. Mirrors npm's `github:`, `gitlab:`, `bitbucket:` etc. - * shorthand so callers can pass them through to bun verbatim. + * Namespaced shorthand prefixes accepted by `omp plugin install`, mapped to + * their canonical host. `PluginManager.install` normalizes non-GitHub prefixes + * before invoking bun because bun only treats `github:` as a hosted shorthand. */ const SHORTHAND_PREFIXES: Record = { github: "github.com", @@ -274,8 +274,8 @@ function tryNamespacedShorthand(trimmed: string): GitSource | null { * * Rules: * - Namespaced shorthand (`github:user/repo`, `gitlab:`, `bitbucket:`, - * `codeberg:`, `sourcehut:`/`srht:`) is accepted directly — these mirror - * bun/npm's built-in install shorthands and skip the rest of the pipeline. + * `codeberg:`, `sourcehut:`/`srht:`) is accepted directly; installers should + * normalize entries that bun does not understand natively. * - With `git:` prefix, accept generic shorthand forms. * - Without `git:` prefix, only accept explicit protocol URLs. * diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index 2271ca048..7a59e3235 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -10,7 +10,7 @@ import { isEnoent, logger, } from "@oh-my-pi/pi-utils"; -import { isGitSpec } from "./git-url"; +import { parseGitUrl, type GitSource } from "./git-url"; import { extractPackageName, parsePluginSpec } from "./parser"; import type { DoctorCheck, @@ -64,6 +64,16 @@ function validateGitSpec(spec: string): void { } } +function gitInstallSpec(original: string, source: GitSource): string { + if (/^github:/i.test(original) || !/^[a-z]+:[^/]/i.test(original)) { + return original; + } + if (!source.ref || source.repo.includes("#")) { + return source.repo; + } + return `${source.repo}#${source.ref}`; +} + // ============================================================================= // Plugin Manager // ============================================================================= @@ -187,7 +197,7 @@ export class PluginManager { */ async install(specString: string, options: InstallOptions = {}): Promise { const spec = parsePluginSpec(specString); - const gitSource = isGitSpec(spec.packageName); + const gitSource = parseGitUrl(spec.packageName); if (gitSource) { validateGitSpec(spec.packageName); } else { @@ -208,9 +218,10 @@ export class PluginManager { } const pkgJsonPath = getPluginsPackageJson(); const depsBefore = gitSource ? await this.#readDeps(pkgJsonPath) : {}; + const packageInstallSpec = gitSource ? gitInstallSpec(spec.packageName, gitSource) : spec.packageName; // Run npm install - const proc = Bun.spawn(["bun", "install", spec.packageName], { + const proc = Bun.spawn(["bun", "install", packageInstallSpec], { cwd: getPluginsDir(), stdin: "ignore", stdout: "pipe", @@ -237,9 +248,10 @@ export class PluginManager { } // Fallback: a force-reinstall of an already-present git plugin will not // add a new key, just rewrite the existing one to the new spec value. - // Match by the value containing the original spec instead. + // Match by the install value for force-reinstalls where no new key is + // added (non-GitHub shorthands are normalized before bun sees them). if (!resolved) { - const needle = spec.packageName.replace(/^git\+/, ""); + const needle = packageInstallSpec.replace(/^git\+/i, ""); for (const [key, value] of Object.entries(depsAfter)) { if (typeof value === "string" && value.includes(needle)) { resolved = key; diff --git a/packages/coding-agent/test/plugin-install-git.test.ts b/packages/coding-agent/test/plugin-install-git.test.ts index 7b78fe58e..f9a0c69ff 100644 --- a/packages/coding-agent/test/plugin-install-git.test.ts +++ b/packages/coding-agent/test/plugin-install-git.test.ts @@ -111,6 +111,55 @@ describe("PluginManager.install with git sources", () => { expect(result.path).toBe(path.join(pluginsNodeModules, "real-name")); }); + test("normalizes non-GitHub shorthand before invoking bun install", async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify({ name: "omp-plugins", private: true, dependencies: {} }, null, 2), + ); + + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + expect(cmd[0]).toBe("bun"); + expect(cmd[1]).toBe("install"); + expect(cmd[2]).toBe("https://gitlab.com/group/sub/project#v1.0.0"); + + const prepare = (async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { + name: "omp-plugins", + private: true, + dependencies: { + "gitlab-plugin": "git+https://gitlab.com/group/sub/project.git#v1.0.0", + }, + }, + null, + 2, + ), + ); + const installedDir = path.join(pluginsNodeModules, "gitlab-plugin"); + await fs.mkdir(installedDir, { recursive: true }); + await Bun.write( + path.join(installedDir, "package.json"), + JSON.stringify({ name: "gitlab-plugin", version: "1.0.0" }, null, 2), + ); + })(); + + return { + pid: 1, + stdout: emptyStream(), + stderr: emptyStream(), + exited: prepare.then(() => 0), + } as Subprocess; + }) as typeof Bun.spawn); + + const mgr = new PluginManager(tmpRoot); + const result = await mgr.install("gitlab:group/sub/project#v1.0.0"); + + expect(result.name).toBe("gitlab-plugin"); + expect(result.version).toBe("1.0.0"); + }); + 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/);