fix(cli): register marketplace plugin installs#3245
Conversation
Added runtime symlink and lockfile bookkeeping for marketplace installs and removals so installed plugins are visible to the plugin loader. Updated manager coverage for install, uninstall, enablement, and loader discovery.\n\nFixes #3244
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc9be7c9de
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await this.#removeRuntimePlugin(scope, previousPackageName); | ||
| } | ||
| } | ||
| await this.#registerRuntimePlugin(scope, packageName, cachePath, version, wasDisabled ? false : undefined); |
There was a problem hiding this comment.
When installPlugin(..., { scope: "project" }) reaches this call, #registerRuntimePlugin writes the symlink and omp-plugins.lock.json under the project registry directory, but the runtime loader still enumerates only getPluginsNodeModules(home) / getPluginsLockfile(home) in loader.ts#getEnabledPlugins and uses project data only for overrides. In the documented omp plugin install --scope project name@marketplace flow, the new runtime bookkeeping is therefore written to a tree that slash commands/extensions never read, so project-scoped marketplace packages remain invisible.
Useful? React with 👍 / 👎.
| async #resolvePluginPackageName(installPath: string, fallbackName: string): Promise<string> { | ||
| try { | ||
| const pkg: { name?: unknown } = await Bun.file(path.join(installPath, "package.json")).json(); | ||
| return typeof pkg.name === "string" && pkg.name.length > 0 ? pkg.name : fallbackName; |
There was a problem hiding this comment.
If a marketplace plugin's copied package.json contains a malformed name such as ../foo, this returns it unchecked and #registerRuntimePlugin later uses it in path.join(node_modules, packageName) before rm and symlink. Marketplace installs copy arbitrary plugin directories rather than going through npm package-name validation, so bad metadata can delete or replace paths outside node_modules during install/uninstall; validate the manifest name and/or verify the resolved link stays inside the plugin root before using it as a path component.
Useful? React with 👍 / 👎.
Made getEnabledPlugins walk both the user plugins root and the active project plugins root, with project entries shadowing same-named user entries. This makes `omp plugin install --scope project name@marketplace` discoverable to slash commands and the extension loader, matching the existing user-scope path. Added regression coverage that exercises the project-scope install through the runtime loader.\n\nFollow-up to #3244 review.
|
Addressed the project-scope feedback in 1d500ce: |
Rejected malformed marketplace package.json names before runtime link creation and guarded node_modules link paths lexically.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 661b3e1672
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (projectRegistryPath) { | ||
| const projectRoot = path.dirname(projectRegistryPath); | ||
| if (projectRoot !== userRoot) { | ||
| projectPlugins = await collectPluginsAtRoot(projectRoot, projectOverrides); |
There was a problem hiding this comment.
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:listInstalledPluginRoots still maps every returned plugin to { level: "user" }. For a marketplace package installed with --scope project that 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 👍 / 👎.
| await fs.rm(linkPath, { recursive: true, force: true }); | ||
| await fs.symlink(cachePath, linkPath, process.platform === "win32" ? "junction" : "dir"); |
There was a problem hiding this comment.
When two installed marketplace entries in the same scope resolve to the same package.json name (for example a forked plugin from another marketplace), this unconditional remove/symlink overwrites the first plugin's runtime link while both plugin IDs remain in installed_plugins.json; later uninstalling either ID can also delete the shared lock entry and leave the other installed-but-invisible. Since buildPluginId permits distinct name@marketplace entries but the runtime tree is keyed only by package name, detect this collision or reference-count it before replacing the link.
Useful? React with 👍 / 👎.
Carried user/project root scope through enabled plugin enumeration so project marketplace installs keep project-level discovery metadata.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc74d09a8a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Project entries shadow user entries with the same package name. | ||
| const merged = new Map<string, ScopedInstalledPlugin>(); | ||
| 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.
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 package.json#name), listInstalledPlugins marks the user entry as shadowed by the project entry, but this merge only replaces entries with the same package name. In that project, getEnabledPlugins() will return both the old user package and the project package, so both sets of commands/extensions can load despite the project install being intended to take precedence for that plugin ID.
Useful? React with 👍 / 👎.
1 participant
Repro
A temp marketplace fixture installed with
MarketplaceManager.installPlugin("hello-plugin", "test-marketplace")cached the plugin and wroteinstalled_plugins.json, but the runtime plugin root had nonode_modules/hello-plugin, noomp-plugins.lock.json, andgetAllPluginExtensionPaths(process.cwd())returned[].Cause
packages/coding-agent/src/extensibility/plugins/marketplace/manager.tsonly maintained the Claude-compatible marketplace registry/cache. The runtime loader inpackages/coding-agent/src/extensibility/plugins/loader.tsdiscovers plugins from the plugin rootnode_modulestree plusomp-plugins.lock.json#plugins, so marketplace installs were invisible even after successful CLI installation.Fix
omp-plugins.lock.jsonwith the previous enabled state preserved.getEnabledPlugins()and that uninstall/reinstall/enablement keep runtime state consistent.Verification
bun test packages/coding-agent/test/marketplace/manager.test.tspassed with 39 tests;bun checkpassed; the repro script now reportsnodeModulesLinkExists: true,lockfileExists: true, and a non-emptyextensionPathsentry throughnode_modules/hello-plugin/extensions/index.ts. Fixes #3244