-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(cli): register marketplace plugin installs #3245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
dc9be7c
1d500ce
77dd83d
661b3e1
bcfb5b5
dc74d09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,9 @@ | |
| */ | ||
| import * as fs from "node:fs"; | ||
| import * as path from "node:path"; | ||
| import { getPluginsLockfile, getPluginsNodeModules, getPluginsPackageJson, isEnoent } from "@oh-my-pi/pi-utils"; | ||
| import { getPluginsDir, getPluginsLockfile, isEnoent } from "@oh-my-pi/pi-utils"; | ||
| import { getConfigDirPaths } from "../../config"; | ||
| import { resolveActiveProjectRegistryPath } from "../../discovery/helpers"; | ||
| import { installLegacyPiSpecifierShim } from "./legacy-pi-compat"; | ||
| import { normalizePluginRuntimeConfig } from "./runtime-config"; | ||
| import type { InstalledPlugin, PluginManifest, PluginRuntimeConfig, ProjectPluginOverrides } from "./types"; | ||
|
|
@@ -51,38 +52,37 @@ async function loadProjectOverrides(cwd: string): Promise<ProjectPluginOverrides | |
| return {}; | ||
| } | ||
| /** | ||
| * Get list of enabled plugins with their resolved configurations. | ||
| * | ||
| * Respects both global runtime config and project overrides. Iterates the | ||
| * union of `<plugins>/package.json#dependencies` (`bun install`-installed | ||
| * packages) and `<plugins>/omp-plugins.lock.json#plugins` (so locally | ||
| * `plugin link`-symlinked extensions, which never get a dependency entry, | ||
| * are still discovered). The optional `home` parameter pins the plugins | ||
| * root for callers that need to enumerate plugins relative to a non-default | ||
| * home (tests with a tempdir, discovery loaders threaded with | ||
| * `LoadContext.home`). | ||
| * Per-root enumeration of plugins from `<root>/node_modules`, | ||
| * `<root>/package.json#dependencies`, and `<root>/omp-plugins.lock.json#plugins`. | ||
| * Honors `projectOverrides.disabled` and `projectOverrides.features`. Returns an | ||
| * empty array when the root has no `node_modules` yet. | ||
| */ | ||
| export async function getEnabledPlugins(cwd: string, opts: { home?: string } = {}): Promise<InstalledPlugin[]> { | ||
| const { home } = opts; | ||
|
|
||
| const nodeModulesPath = getPluginsNodeModules(home); | ||
| if (!fs.existsSync(nodeModulesPath)) { | ||
| return []; | ||
| } | ||
| async function collectPluginsAtRoot( | ||
| root: string, | ||
| projectOverrides: ProjectPluginOverrides, | ||
| ): Promise<InstalledPlugin[]> { | ||
| const nodeModulesPath = path.join(root, "node_modules"); | ||
| if (!fs.existsSync(nodeModulesPath)) return []; | ||
|
|
||
| let depsKeys: string[] = []; | ||
| const pkgJsonPath = getPluginsPackageJson(home); | ||
| const pkgJsonPath = path.join(root, "package.json"); | ||
| try { | ||
| const pkg: { dependencies?: Record<string, string> } = await Bun.file(pkgJsonPath).json(); | ||
| depsKeys = Object.keys(pkg.dependencies ?? {}); | ||
| } catch (err) { | ||
| // Linked-only setups may have no `<plugins>/package.json` yet — that's | ||
| // Linked-only setups may have no `<root>/package.json` yet — that's | ||
| // fine, the lockfile still records the link. | ||
| if (!isEnoent(err)) throw err; | ||
| } | ||
|
|
||
| const runtimeConfig = await loadRuntimeConfig(home); | ||
| const projectOverrides = await loadProjectOverrides(cwd); | ||
| const lockPath = path.join(root, "omp-plugins.lock.json"); | ||
| let runtimeConfig: PluginRuntimeConfig; | ||
| try { | ||
| runtimeConfig = normalizePluginRuntimeConfig(await Bun.file(lockPath).json()); | ||
| } catch (err) { | ||
| if (!isEnoent(err)) throw err; | ||
| runtimeConfig = normalizePluginRuntimeConfig({}); | ||
| } | ||
|
|
||
| // Union: dependencies (npm/marketplace installs) ∪ runtime-config plugins | ||
| // (links + already-recorded installs). Set preserves first-seen order, | ||
|
|
@@ -139,6 +139,47 @@ export async function getEnabledPlugins(cwd: string, opts: { home?: string } = { | |
| return plugins; | ||
| } | ||
|
|
||
| /** | ||
| * Get list of enabled plugins with their resolved configurations. | ||
| * | ||
| * Enumerates two plugin roots in order: the user root | ||
| * (`getPluginsDir(home)`) and, when a project anchor (`.omp/` or `.git/`) | ||
| * exists at or above `cwd`, the project root | ||
| * (`<projectAnchor>/.omp/plugins`). Each root contributes the union of its | ||
| * `package.json#dependencies` and `omp-plugins.lock.json#plugins`. Project | ||
| * entries shadow user entries with the same package name, matching the | ||
| * shadow semantics of `MarketplaceManager.listInstalledPlugins`. | ||
| * | ||
| * The optional `home` parameter pins the user plugins root for callers that | ||
| * need to enumerate plugins relative to a non-default home (tests with a | ||
| * tempdir, discovery loaders threaded with `LoadContext.home`). | ||
| */ | ||
| export async function getEnabledPlugins(cwd: string, opts: { home?: string } = {}): Promise<InstalledPlugin[]> { | ||
| const { home } = opts; | ||
| const projectOverrides = await loadProjectOverrides(cwd); | ||
|
|
||
| const userRoot = getPluginsDir(home); | ||
| const userPlugins = await collectPluginsAtRoot(userRoot, projectOverrides); | ||
|
|
||
| let projectPlugins: InstalledPlugin[] = []; | ||
| const projectRegistryPath = await resolveActiveProjectRegistryPath(cwd); | ||
| if (projectRegistryPath) { | ||
| const projectRoot = path.dirname(projectRegistryPath); | ||
| if (projectRoot !== userRoot) { | ||
| projectPlugins = await collectPluginsAtRoot(projectRoot, projectOverrides); | ||
| } | ||
| } | ||
|
|
||
| if (projectPlugins.length === 0) return userPlugins; | ||
| if (userPlugins.length === 0) return projectPlugins; | ||
|
|
||
| // Project entries shadow user entries with the same package name. | ||
| const merged = new Map<string, InstalledPlugin>(); | ||
| for (const plugin of userPlugins) merged.set(plugin.name, plugin); | ||
| for (const plugin of projectPlugins) merged.set(plugin.name, plugin); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the same marketplace plugin ID is installed in both user and project scopes but the two versions resolve to different runtime package names (for example, an upgrade adds or renames Useful? React with 👍 / 👎. |
||
| return Array.from(merged.values()); | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // Path Resolution | ||
| // ============================================================================= | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ import * as os from "node:os"; | |
| import * as path from "node:path"; | ||
|
|
||
| import { isEnoent, logger, pathIsWithin } from "@oh-my-pi/pi-utils"; | ||
| import { normalizePluginRuntimeConfig } from "../runtime-config"; | ||
| import type { PluginRuntimeConfig } from "../types"; | ||
|
|
||
| import { cachePlugin } from "./cache"; | ||
| import { classifySource, fetchMarketplace, parseMarketplaceCatalog, promoteCloneToCache } from "./fetcher"; | ||
|
|
@@ -38,6 +40,16 @@ import type { | |
| } from "./types"; | ||
| import { buildPluginId, parsePluginId } from "./types"; | ||
|
|
||
| const RUNTIME_PACKAGE_NAME_RE = /^(?:@[a-z0-9][a-z0-9._~-]*\/)?[a-z0-9][a-z0-9._~-]*$/; | ||
| const MAX_RUNTIME_PACKAGE_NAME_LENGTH = 214; | ||
|
|
||
| function assertRuntimePackageName(name: string): string { | ||
| if (name.length > MAX_RUNTIME_PACKAGE_NAME_LENGTH || !RUNTIME_PACKAGE_NAME_RE.test(name)) { | ||
| throw new Error(`Invalid marketplace plugin package name: ${JSON.stringify(name)}`); | ||
| } | ||
| return name; | ||
| } | ||
|
|
||
| // ── Options ────────────────────────────────────────────────────────────────── | ||
|
|
||
| export interface MarketplaceManagerOptions { | ||
|
|
@@ -298,6 +310,9 @@ export class MarketplaceManager { | |
| } | ||
| } | ||
|
|
||
| const packageName = await this.#resolvePluginPackageName(cachePath, name); | ||
| const previousPackageNames = await this.#resolveInstalledPackageNames(existing ?? [], name); | ||
|
|
||
| // Only now clean up old entries — new cache succeeded, so it is safe to remove old ones. | ||
| if (existing && existing.length > 0) { | ||
| // Remove from scope-appropriate registry first, then cross-check refs before disk deletion. | ||
|
|
@@ -337,6 +352,13 @@ export class MarketplaceManager { | |
| const newInstReg = addInstalledPlugin(freshInstReg, pluginId, installedEntry); | ||
| await writeInstalledPluginsRegistry(registryPath, newInstReg); | ||
|
|
||
| for (const previousPackageName of previousPackageNames) { | ||
| if (previousPackageName !== packageName) { | ||
| await this.#removeRuntimePlugin(scope, previousPackageName); | ||
| } | ||
| } | ||
| await this.#registerRuntimePlugin(scope, packageName, cachePath, version, wasDisabled ? false : undefined); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
|
|
||
| this.#clearCache(); | ||
|
|
||
| logger.debug("Plugin installed", { pluginId, version, cachePath }); | ||
|
|
@@ -434,6 +456,7 @@ export class MarketplaceManager { | |
| const targetEntries = targetScope === "project" ? projectEntries! : userEntries!; | ||
| const targetReg = targetScope === "project" ? projectReg : userReg; | ||
| const registryPath = this.#registryPath(targetScope); | ||
| const packageNames = await this.#resolveInstalledPackageNames(targetEntries, parsed.name); | ||
|
|
||
| const updatedReg = removeInstalledPlugin(targetReg, pluginId); | ||
| await writeInstalledPluginsRegistry(registryPath, updatedReg); | ||
|
|
@@ -453,6 +476,10 @@ export class MarketplaceManager { | |
| } | ||
| } | ||
|
|
||
| for (const packageName of packageNames) { | ||
| await this.#removeRuntimePlugin(targetScope, packageName); | ||
| } | ||
|
|
||
| this.#clearCache(); | ||
|
|
||
| logger.debug("Plugin uninstalled", { pluginId, scope: targetScope }); | ||
|
|
@@ -539,6 +566,12 @@ export class MarketplaceManager { | |
| }; | ||
| await writeInstalledPluginsRegistry(registryPath, updated); | ||
|
|
||
| const fallbackName = parsePluginId(pluginId)?.name ?? pluginId; | ||
| const packageNames = await this.#resolveInstalledPackageNames(entries, fallbackName); | ||
| for (const packageName of packageNames) { | ||
| await this.#setRuntimePluginEnabled(targetScope, packageName, enabled); | ||
| } | ||
|
|
||
| this.#clearCache(); | ||
|
|
||
| logger.debug("Plugin enabled state changed", { pluginId, enabled, scope: targetScope }); | ||
|
|
@@ -701,6 +734,107 @@ export class MarketplaceManager { | |
|
|
||
| // ── Private helpers ─────────────────────────────────────────────────────── | ||
|
|
||
| #runtimeRoot(scope: "user" | "project"): string { | ||
| return path.dirname(this.#registryPath(scope)); | ||
| } | ||
|
|
||
| #nodeModulesPath(scope: "user" | "project"): string { | ||
| return path.join(this.#runtimeRoot(scope), "node_modules"); | ||
| } | ||
|
|
||
| #runtimeLockPath(scope: "user" | "project"): string { | ||
| return path.join(this.#runtimeRoot(scope), "omp-plugins.lock.json"); | ||
| } | ||
|
|
||
| async #loadRuntimeConfig(scope: "user" | "project"): Promise<PluginRuntimeConfig> { | ||
| try { | ||
| return normalizePluginRuntimeConfig(await Bun.file(this.#runtimeLockPath(scope)).json()); | ||
| } catch (err) { | ||
| if (isEnoent(err)) return normalizePluginRuntimeConfig({}); | ||
| logger.warn("Failed to load marketplace plugin runtime config", { | ||
| path: this.#runtimeLockPath(scope), | ||
| error: String(err), | ||
| }); | ||
| return normalizePluginRuntimeConfig({}); | ||
| } | ||
| } | ||
|
|
||
| async #writeRuntimeConfig(scope: "user" | "project", config: PluginRuntimeConfig): Promise<void> { | ||
| await Bun.write(this.#runtimeLockPath(scope), JSON.stringify(config, null, 2)); | ||
| } | ||
|
|
||
| async #resolvePluginPackageName(installPath: string, fallbackName: string): Promise<string> { | ||
| try { | ||
| const pkg: { name?: unknown } = await Bun.file(path.join(installPath, "package.json")).json(); | ||
| const name = typeof pkg.name === "string" && pkg.name.length > 0 ? pkg.name : fallbackName; | ||
| return assertRuntimePackageName(name); | ||
| } catch (err) { | ||
| if (isEnoent(err)) return assertRuntimePackageName(fallbackName); | ||
| throw err; | ||
| } | ||
| } | ||
|
|
||
| #runtimePackagePath(scope: "user" | "project", packageName: string): string { | ||
| const nodeModules = path.resolve(this.#nodeModulesPath(scope)); | ||
| const linkPath = path.resolve(nodeModules, assertRuntimePackageName(packageName)); | ||
| const relative = path.relative(nodeModules, linkPath); | ||
| if (relative === "" || relative.startsWith("..") || path.isAbsolute(relative)) { | ||
| throw new Error(`Marketplace plugin package path escapes node_modules: ${JSON.stringify(packageName)}`); | ||
| } | ||
| return linkPath; | ||
| } | ||
|
|
||
| async #resolveInstalledPackageNames( | ||
| entries: readonly InstalledPluginEntry[], | ||
| fallbackName: string, | ||
| ): Promise<Set<string>> { | ||
| const packageNames = new Set<string>(); | ||
| for (const entry of entries) { | ||
| packageNames.add(await this.#resolvePluginPackageName(entry.installPath, fallbackName)); | ||
| } | ||
| return packageNames; | ||
| } | ||
|
|
||
| async #registerRuntimePlugin( | ||
| scope: "user" | "project", | ||
| packageName: string, | ||
| cachePath: string, | ||
| version: string, | ||
| enabled: boolean | undefined, | ||
| ): Promise<void> { | ||
| const linkPath = this.#runtimePackagePath(scope, packageName); | ||
| await fs.mkdir(path.dirname(linkPath), { recursive: true }); | ||
| await fs.rm(linkPath, { recursive: true, force: true }); | ||
| await fs.symlink(cachePath, linkPath, process.platform === "win32" ? "junction" : "dir"); | ||
|
Comment on lines
+807
to
+808
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When two installed marketplace entries in the same scope resolve to the same Useful? React with 👍 / 👎. |
||
|
|
||
| const config = await this.#loadRuntimeConfig(scope); | ||
| const previous = config.plugins[packageName]; | ||
| config.plugins[packageName] = { | ||
| version, | ||
| enabledFeatures: previous?.enabledFeatures ?? null, | ||
| enabled: enabled ?? previous?.enabled ?? true, | ||
| }; | ||
| await this.#writeRuntimeConfig(scope, config); | ||
| } | ||
|
|
||
| async #removeRuntimePlugin(scope: "user" | "project", packageName: string): Promise<void> { | ||
| await fs.rm(this.#runtimePackagePath(scope, packageName), { recursive: true, force: true }); | ||
|
|
||
| const config = await this.#loadRuntimeConfig(scope); | ||
| delete config.plugins[packageName]; | ||
| delete config.settings[packageName]; | ||
| await this.#writeRuntimeConfig(scope, config); | ||
| } | ||
|
|
||
| async #setRuntimePluginEnabled(scope: "user" | "project", packageName: string, enabled: boolean): Promise<void> { | ||
| const config = await this.#loadRuntimeConfig(scope); | ||
| const previous = config.plugins[packageName]; | ||
| if (!previous) return; | ||
|
|
||
| config.plugins[packageName] = { ...previous, enabled }; | ||
| await this.#writeRuntimeConfig(scope, config); | ||
| } | ||
|
|
||
| #registryPath(scope: "user" | "project"): string { | ||
| if (scope === "project") { | ||
| if (!this.#opts.projectInstalledRegistryPath) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this new branch mixes project-root installs into the same
InstalledPlugin[]shape as user installs, the scope is lost;packages/coding-agent/src/discovery/omp-extension-roots.ts:listInstalledPluginRootsstill maps every returned plugin to{ level: "user" }. For a marketplace package installed with--scope projectthat contributes OMP commands/tools/skills, those contributions are now surfaced as user-level metadata, so level-based diagnostics/source controls treat project config as user config. Carry the root scope through before merging or return scoped roots separately.Useful? React with 👍 / 👎.