fix(plugins): enumerate linked plugins for sub-discovery
PluginManager.link symlinks the package into <plugins>/node_modules and records it in omp-plugins.lock.json, but never writes to <plugins>/package.json#dependencies. getEnabledPlugins iterated only the dependency map, so the documented `omp install ./local-extension` workflow (delegated to plugin link) succeeded but its sibling skills/, hooks/, tools/, etc. stayed invisible after install. Iterate the union of package.json#dependencies and omp-plugins.lock.json#plugins so symlinked-only packages surface alongside npm/marketplace installs. Lockfile entries whose node_modules tree has since been deleted (stale link) are skipped silently. Linked-only setups with no <plugins>/package.json at all now work too. Per-PR review feedback: https://github.com/can1357/oh-my-pi/pull/1498
This commit is contained in:
@@ -52,48 +52,63 @@ async function loadProjectOverrides(cwd: string): Promise<ProjectPluginOverrides
|
||||
/**
|
||||
* Get list of enabled plugins with their resolved configurations.
|
||||
*
|
||||
* Respects both global runtime config and project overrides. 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`).
|
||||
* 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`).
|
||||
*/
|
||||
export async function getEnabledPlugins(cwd: string, opts: { home?: string } = {}): Promise<InstalledPlugin[]> {
|
||||
const { home } = opts;
|
||||
const pkgJsonPath = getPluginsPackageJson(home);
|
||||
let pkg: { dependencies?: Record<string, string> };
|
||||
try {
|
||||
pkg = await Bun.file(pkgJsonPath).json();
|
||||
} catch (err) {
|
||||
if (isEnoent(err)) return [];
|
||||
throw err;
|
||||
}
|
||||
|
||||
const nodeModulesPath = getPluginsNodeModules(home);
|
||||
if (!fs.existsSync(nodeModulesPath)) {
|
||||
return [];
|
||||
}
|
||||
|
||||
const deps = pkg.dependencies || {};
|
||||
let depsKeys: string[] = [];
|
||||
const pkgJsonPath = getPluginsPackageJson(home);
|
||||
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
|
||||
// fine, the lockfile still records the link.
|
||||
if (!isEnoent(err)) throw err;
|
||||
}
|
||||
|
||||
const runtimeConfig = await loadRuntimeConfig(home);
|
||||
const projectOverrides = await loadProjectOverrides(cwd);
|
||||
|
||||
// Union: dependencies (npm/marketplace installs) ∪ runtime-config plugins
|
||||
// (links + already-recorded installs). Set preserves first-seen order,
|
||||
// putting deps before link-only entries for deterministic output.
|
||||
const names = new Set<string>(depsKeys);
|
||||
for (const name of Object.keys(runtimeConfig.plugins ?? {})) {
|
||||
names.add(name);
|
||||
}
|
||||
|
||||
const plugins: InstalledPlugin[] = [];
|
||||
for (const [name] of Object.entries(deps)) {
|
||||
for (const name of names) {
|
||||
const pluginPkgPath = path.join(nodeModulesPath, name, "package.json");
|
||||
let pluginPkg: { version: string; omp?: PluginManifest; pi?: PluginManifest };
|
||||
try {
|
||||
pluginPkg = await Bun.file(pluginPkgPath).json();
|
||||
} catch (err) {
|
||||
// Lockfile entry without a corresponding node_modules tree means the
|
||||
// link was deleted out from under us; skip silently.
|
||||
if (isEnoent(err)) continue;
|
||||
throw err;
|
||||
}
|
||||
|
||||
const manifest: PluginManifest | undefined = pluginPkg.omp || pluginPkg.pi;
|
||||
|
||||
if (!manifest) {
|
||||
// Not an omp plugin, skip
|
||||
continue;
|
||||
}
|
||||
|
||||
manifest.version = pluginPkg.version;
|
||||
|
||||
const runtimeState = runtimeConfig.plugins[name];
|
||||
|
||||
@@ -215,3 +215,31 @@ test("disabled installed plugins do not contribute sub-discovery", async () => {
|
||||
const skills = await loadFromPlugin<{ name: string; path: string }>(skillCapability.id, ctx());
|
||||
expect(skills.find(s => s.path.includes("my-disabled-ext"))).toBeUndefined();
|
||||
});
|
||||
|
||||
test("linked plugins (only in lockfile, not in package.json#dependencies) are surfaced", async () => {
|
||||
// `omp plugin link ./local-ext` creates a symlink under
|
||||
// `<plugins>/node_modules/<pkg>` plus a lockfile entry, but it never
|
||||
// touches `<plugins>/package.json#dependencies`. The discovery path must
|
||||
// still find the package — otherwise the documented `omp install
|
||||
// ./local-extension` workflow leaves the sibling skills/hooks/tools
|
||||
// invisible (see PR #1498 review).
|
||||
const pluginsDir = path.join(home, ".omp", "plugins");
|
||||
const nodeModules = path.join(pluginsDir, "node_modules");
|
||||
fs.mkdirSync(nodeModules, { recursive: true });
|
||||
const linkTarget = path.join(nodeModules, "my-linked-ext");
|
||||
fs.symlinkSync(ext, linkTarget);
|
||||
// Intentionally NO `<plugins>/package.json` — matches a fresh `plugin link`
|
||||
// against a setup that has never run `plugin install`.
|
||||
writeFile(
|
||||
path.join(pluginsDir, "omp-plugins.lock.json"),
|
||||
JSON.stringify({
|
||||
plugins: { "my-linked-ext": { version: "1.0.0", enabled: true, enabledFeatures: null } },
|
||||
settings: {},
|
||||
}),
|
||||
);
|
||||
|
||||
const skills = await loadFromPlugin<{ name: string; path: string }>(skillCapability.id, ctx());
|
||||
const tools = await loadFromPlugin<{ name: string; path: string }>(toolCapability.id, ctx());
|
||||
expect(skills.find(s => s.name === "my-skill" && s.path.includes("my-linked-ext"))).toBeDefined();
|
||||
expect(tools.find(t => t.name === "wcount" && t.path.includes("my-linked-ext"))).toBeDefined();
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user