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.
This commit is contained in:
Brian Madison 2026-04-26 22:15:30 -05:00
parent 867ef95654
commit e6e9318515
2 changed files with 138 additions and 31 deletions

View File

@ -449,20 +449,39 @@ class CommunityModuleManager {
const plugins = Array.isArray(marketplaceData?.plugins) ? marketplaceData.plugins : []; const plugins = Array.isArray(marketplaceData?.plugins) ? marketplaceData.plugins : [];
if (plugins.length === 0) return; if (plugins.length === 0) return;
const plugin = this._selectPluginForModule(plugins, moduleInfo); const selection = this._selectPluginForModule(plugins, moduleInfo);
if (!plugin) { if (!selection) {
await prompts.log await this._safeWarn(
.warn?.( `Community module '${moduleInfo.code}' ships marketplace.json but no plugin entry matches the registry code. ` +
`Community module '${moduleInfo.code}' ships marketplace.json but no plugin entry matches the registry code. ` + `Falling back to legacy install path.`,
`Falling back to legacy install path.`, );
)
.catch(() => {});
return; 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 { PluginResolver } = require('./plugin-resolver');
const resolver = new PluginResolver(); 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; if (!resolved || resolved.length === 0) return;
// The registry registers a single code per module. If the resolver returns // 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); const matched = resolved.find((mod) => mod.code === moduleInfo.code) || (resolved.length === 1 ? resolved[0] : null);
if (!matched) return; if (!matched) return;
// Stamp registry/clone provenance so installFromResolution and downstream // Shallow-clone before stamping provenance — the resolver may cache or reuse
// manifest writers see the same channel/sha/tag as the legacy path. // its return objects, and we don't want install-specific fields leaking back.
matched.code = moduleInfo.code; const stamped = {
matched.repoUrl = moduleInfo.url; ...matched,
matched.cloneRef = resolution.channel === 'pinned' ? resolution.version : resolution.approvedTag || null; code: moduleInfo.code,
matched.cloneSha = resolution.sha; repoUrl: moduleInfo.url,
matched.communitySource = true; cloneRef: resolution.channel === 'pinned' ? resolution.version : resolution.approvedTag || null,
matched.communityChannel = resolution.channel; cloneSha: resolution.sha,
matched.communityVersion = resolution.version; communitySource: true,
matched.registryApprovedTag = resolution.approvedTag; communityChannel: resolution.channel,
matched.registryApprovedSha = resolution.approvedSha; 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<Object|null>}
*/
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: * Precedence:
* 1. Exact match on `plugin.name === moduleInfo.code` * 1. Exact match on `plugin.name === moduleInfo.code`
* 2. Trailing directory of `module_definition` matches `plugin.name` * 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). * Otherwise null (caller falls back to legacy path).
*
* @returns {{plugin: Object, source: 'name'|'hint'|'single-fallback'}|null}
*/ */
_selectPluginForModule(plugins, moduleInfo) { _selectPluginForModule(plugins, moduleInfo) {
const byCode = plugins.find((p) => p && p.name === moduleInfo.code); const byCode = plugins.find((p) => p && p.name === moduleInfo.code);
if (byCode) return byCode; if (byCode) return { plugin: byCode, source: 'name' };
if (moduleInfo.moduleDefinition) { if (moduleInfo.moduleDefinition) {
// module_definition like "src/skills/suno-setup/assets/module.yaml" → // module_definition like "src/skills/suno-setup/assets/module.yaml" →
@ -507,11 +604,11 @@ class CommunityModuleManager {
if (setupIdx !== -1) { if (setupIdx !== -1) {
const hint = segments[setupIdx]; const hint = segments[setupIdx];
const byHint = plugins.find((p) => p && p.name === hint); 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; return null;
} }

View File

@ -270,9 +270,16 @@ class OfficialModules {
} }
// Community modules whose cloned repo ships marketplace.json get the same // 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/<name>/`
// so we don't silently regress to the legacy half-install path.
const { CommunityModuleManager } = require('./community-manager'); 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) { if (communityResolved) {
return this.installFromResolution(communityResolved, bmadDir, fileTrackingCallback, options); return this.installFromResolution(communityResolved, bmadDir, fileTrackingCallback, options);
} }
@ -400,10 +407,13 @@ class OfficialModules {
success: true, success: true,
module: resolved.code, module: resolved.code,
path: targetPath, path: targetPath,
// Match the manifestEntry.version expression above so downstream summary // Mirror the manifestEntry.version precedence above so downstream summary
// lines show the cloned ref (tag or 'main') instead of the on-disk // lines show the same string we just wrote to disk (community installs
// package.json version for git-backed custom installs. // use the registry-approved tag via `communityVersion`; custom git-backed
versionInfo: { version: resolved.cloneRef || (hasGitClone ? 'main' : resolved.version || '') }, // installs show the cloned ref or 'main').
versionInfo: {
version: resolved.communityVersion || resolved.cloneRef || (hasGitClone ? 'main' : resolved.version || ''),
},
}; };
} }