From e6e9318515d01446f4052ece34e1737e737f4d7a Mon Sep 17 00:00:00 2001 From: Brian Madison Date: Sun, 26 Apr 2026 22:15:30 -0500 Subject: [PATCH] fix(installer): harden community marketplace.json resolution path Address review feedback on the community marketplace.json install path: - Wrap PluginResolver.resolve() in try/catch so a malformed plugin entry falls through to the legacy install path with a warn instead of crashing cloneModule. - Stop mutating the resolver's return object; shallow-clone before stamping community provenance so install state cannot leak back into resolver-owned objects. - Warn when _selectPluginForModule lands on the single-plugin fallback with a name that doesn't match the registry code or module_definition hint, so a misconfigured marketplace.json can't silently install the wrong plugin. - Add CommunityModuleManager.resolveFromCache() and call it from OfficialModules.install() when the in-process plugin cache is empty, so callers that reach install() without pre-cloning still get the marketplace-aware path. Reuses an existing channel resolution when present, otherwise synthesizes a stable-channel stub from the registry entry plus the cached repo's HEAD. - Align installFromResolution()'s returned versionInfo.version with manifestEntry.version precedence (communityVersion || cloneRef || ...) so downstream summaries match what was written to the manifest. Tests: lint, format:check, lint:md, test:install (290), test:channels (83), test:refs (7) all green. --- tools/installer/modules/community-manager.js | 147 +++++++++++++++---- tools/installer/modules/official-modules.js | 22 ++- 2 files changed, 138 insertions(+), 31 deletions(-) diff --git a/tools/installer/modules/community-manager.js b/tools/installer/modules/community-manager.js index 62f0e364b..192e8f701 100644 --- a/tools/installer/modules/community-manager.js +++ b/tools/installer/modules/community-manager.js @@ -449,20 +449,39 @@ class CommunityModuleManager { const plugins = Array.isArray(marketplaceData?.plugins) ? marketplaceData.plugins : []; if (plugins.length === 0) return; - const plugin = this._selectPluginForModule(plugins, moduleInfo); - if (!plugin) { - await prompts.log - .warn?.( - `Community module '${moduleInfo.code}' ships marketplace.json but no plugin entry matches the registry code. ` + - `Falling back to legacy install path.`, - ) - .catch(() => {}); + const selection = this._selectPluginForModule(plugins, moduleInfo); + if (!selection) { + await this._safeWarn( + `Community module '${moduleInfo.code}' ships marketplace.json but no plugin entry matches the registry code. ` + + `Falling back to legacy install path.`, + ); return; } + if (selection.source === 'single-fallback') { + // Single-entry marketplace.json whose plugin name doesn't match the registry + // code or the module_definition hint. Most likely correct, but worth surfacing + // in case marketplace.json is misconfigured and we'd install the wrong plugin. + await this._safeWarn( + `Community module '${moduleInfo.code}' picked the only plugin in marketplace.json ('${selection.plugin?.name}') ` + + `because no name or module_definition match was found. Verify marketplace.json if the install looks wrong.`, + ); + } + const { PluginResolver } = require('./plugin-resolver'); const resolver = new PluginResolver(); - const resolved = await resolver.resolve(repoPath, plugin); + let resolved; + try { + resolved = await resolver.resolve(repoPath, selection.plugin); + } catch (error) { + // PluginResolver threw (malformed plugin entry, missing files, etc.). + // Honor the silent-fallthrough contract — warn and let the legacy + // findModuleSource path handle the install. + await this._safeWarn( + `PluginResolver failed for community module '${moduleInfo.code}': ${error.message}. ` + `Falling back to legacy install path.`, + ); + return; + } if (!resolved || resolved.length === 0) return; // The registry registers a single code per module. If the resolver returns @@ -472,19 +491,94 @@ class CommunityModuleManager { const matched = resolved.find((mod) => mod.code === moduleInfo.code) || (resolved.length === 1 ? resolved[0] : null); if (!matched) return; - // Stamp registry/clone provenance so installFromResolution and downstream - // manifest writers see the same channel/sha/tag as the legacy path. - matched.code = moduleInfo.code; - matched.repoUrl = moduleInfo.url; - matched.cloneRef = resolution.channel === 'pinned' ? resolution.version : resolution.approvedTag || null; - matched.cloneSha = resolution.sha; - matched.communitySource = true; - matched.communityChannel = resolution.channel; - matched.communityVersion = resolution.version; - matched.registryApprovedTag = resolution.approvedTag; - matched.registryApprovedSha = resolution.approvedSha; + // Shallow-clone before stamping provenance — the resolver may cache or reuse + // its return objects, and we don't want install-specific fields leaking back. + const stamped = { + ...matched, + code: moduleInfo.code, + repoUrl: moduleInfo.url, + cloneRef: resolution.channel === 'pinned' ? resolution.version : resolution.approvedTag || null, + cloneSha: resolution.sha, + communitySource: true, + communityChannel: resolution.channel, + communityVersion: resolution.version, + registryApprovedTag: resolution.approvedTag, + registryApprovedSha: resolution.approvedSha, + }; - CommunityModuleManager._pluginResolutions.set(moduleInfo.code, matched); + CommunityModuleManager._pluginResolutions.set(moduleInfo.code, stamped); + } + + /** + * Lazy fallback: resolve marketplace.json straight from the on-disk cache + * when `_pluginResolutions` is empty (e.g. callers that reach `install()` + * without `cloneModule` having populated the cache earlier in this process). + * + * Reuses an existing channel resolution if present; otherwise synthesizes a + * minimal stable-channel stub from the registry entry + the cached repo's + * current HEAD. Returns the cached plugin resolution if one is produced, + * otherwise null (caller falls back to the legacy path). + * + * @param {string} moduleCode + * @returns {Promise} + */ + async resolveFromCache(moduleCode) { + const existing = this.getPluginResolution(moduleCode); + if (existing) return existing; + + const cacheRepoDir = path.join(this.getCacheDir(), moduleCode); + const marketplacePath = path.join(cacheRepoDir, '.claude-plugin', 'marketplace.json'); + if (!(await fs.pathExists(marketplacePath))) return null; + + let moduleInfo; + try { + moduleInfo = await this.getModuleByCode(moduleCode); + } catch { + return null; + } + if (!moduleInfo) return null; + + let channelResolution = this.getResolution(moduleCode); + if (!channelResolution) { + let sha = ''; + try { + sha = execSync('git rev-parse HEAD', { cwd: cacheRepoDir, stdio: 'pipe' }).toString().trim(); + } catch { + // Not a git repo or unreadable — give up and let the legacy path run. + return null; + } + channelResolution = { + channel: 'stable', + version: moduleInfo.approvedTag || sha.slice(0, 7), + sha, + registryApprovedTag: moduleInfo.approvedTag || null, + registryApprovedSha: moduleInfo.approvedSha || null, + }; + } + + await this._tryResolveMarketplacePlugin(cacheRepoDir, moduleInfo, { + channel: channelResolution.channel, + version: channelResolution.version, + sha: channelResolution.sha, + approvedTag: channelResolution.registryApprovedTag, + approvedSha: channelResolution.registryApprovedSha, + }); + + return this.getPluginResolution(moduleCode); + } + + /** + * Best-effort warning emitter. `prompts.log.warn` may be undefined in some + * harnesses and may return a rejected promise — swallow both cases so a + * fallthrough warning can never crash the install. + */ + async _safeWarn(message) { + try { + const result = prompts.log?.warn?.(message); + if (result && typeof result.then === 'function') await result; + } catch { + /* ignore */ + } } /** @@ -492,12 +586,15 @@ class CommunityModuleManager { * Precedence: * 1. Exact match on `plugin.name === moduleInfo.code` * 2. Trailing directory of `module_definition` matches `plugin.name` - * 3. Single plugin in marketplace.json — use it + * 3. Single plugin in marketplace.json — accepted with a warning so a + * mismatched-but-uniquely-named plugin doesn't install silently. * Otherwise null (caller falls back to legacy path). + * + * @returns {{plugin: Object, source: 'name'|'hint'|'single-fallback'}|null} */ _selectPluginForModule(plugins, moduleInfo) { const byCode = plugins.find((p) => p && p.name === moduleInfo.code); - if (byCode) return byCode; + if (byCode) return { plugin: byCode, source: 'name' }; if (moduleInfo.moduleDefinition) { // module_definition like "src/skills/suno-setup/assets/module.yaml" → @@ -507,11 +604,11 @@ class CommunityModuleManager { if (setupIdx !== -1) { const hint = segments[setupIdx]; const byHint = plugins.find((p) => p && p.name === hint); - if (byHint) return byHint; + if (byHint) return { plugin: byHint, source: 'hint' }; } } - if (plugins.length === 1) return plugins[0]; + if (plugins.length === 1) return { plugin: plugins[0], source: 'single-fallback' }; return null; } diff --git a/tools/installer/modules/official-modules.js b/tools/installer/modules/official-modules.js index 89c21c1cd..4bd1e56b3 100644 --- a/tools/installer/modules/official-modules.js +++ b/tools/installer/modules/official-modules.js @@ -270,9 +270,16 @@ class OfficialModules { } // Community modules whose cloned repo ships marketplace.json get the same - // skill-level install treatment as custom-source installs. + // skill-level install treatment as custom-source installs. If the in-process + // cache wasn't populated (e.g. caller skipped the pre-clone phase), fall + // back to resolving directly from `~/.bmad/cache/community-modules//` + // so we don't silently regress to the legacy half-install path. const { CommunityModuleManager } = require('./community-manager'); - const communityResolved = new CommunityModuleManager().getPluginResolution(moduleName); + const communityMgr = new CommunityModuleManager(); + let communityResolved = communityMgr.getPluginResolution(moduleName); + if (!communityResolved) { + communityResolved = await communityMgr.resolveFromCache(moduleName); + } if (communityResolved) { return this.installFromResolution(communityResolved, bmadDir, fileTrackingCallback, options); } @@ -400,10 +407,13 @@ class OfficialModules { success: true, module: resolved.code, path: targetPath, - // Match the manifestEntry.version expression above so downstream summary - // lines show the cloned ref (tag or 'main') instead of the on-disk - // package.json version for git-backed custom installs. - versionInfo: { version: resolved.cloneRef || (hasGitClone ? 'main' : resolved.version || '') }, + // Mirror the manifestEntry.version precedence above so downstream summary + // lines show the same string we just wrote to disk (community installs + // use the registry-approved tag via `communityVersion`; custom git-backed + // installs show the cloned ref or 'main'). + versionInfo: { + version: resolved.communityVersion || resolved.cloneRef || (hasGitClone ? 'main' : resolved.version || ''), + }, }; }