From 80d4a4c9786ba9b1d5812ba56d98346bea330a0e Mon Sep 17 00:00:00 2001 From: murat Date: Wed, 22 Apr 2026 09:18:26 -0500 Subject: [PATCH] fix: addressed review comments --- test/test-installation-components.js | 78 +++++++++++++++++++++ tools/installer/core/manifest.js | 2 +- tools/installer/modules/version-resolver.js | 45 ++++++++++-- 3 files changed, 120 insertions(+), 5 deletions(-) diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 32d7c15eb..24cf782e5 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -2403,6 +2403,9 @@ async function runTests() { { const { resolveModuleVersion } = require('../tools/installer/modules/version-resolver'); const tempRepo39 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-version-module-yaml-')); + const tempCacheDir39 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-version-module-yaml-cache-')); + const priorCacheEnv39 = process.env.BMAD_EXTERNAL_MODULES_CACHE; + process.env.BMAD_EXTERNAL_MODULES_CACHE = tempCacheDir39; try { const moduleDir = path.join(tempRepo39, 'src'); @@ -2419,7 +2422,13 @@ async function runTests() { assert(versionInfo.version === '2.4.0', 'resolver falls back to module.yaml when package.json is missing'); assert(versionInfo.source === 'module.yaml', 'resolver reports module.yaml when it provides the selected version'); } finally { + if (priorCacheEnv39 === undefined) { + delete process.env.BMAD_EXTERNAL_MODULES_CACHE; + } else { + process.env.BMAD_EXTERNAL_MODULES_CACHE = priorCacheEnv39; + } await fs.remove(tempRepo39).catch(() => {}); + await fs.remove(tempCacheDir39).catch(() => {}); } } @@ -2427,6 +2436,9 @@ async function runTests() { { const { resolveModuleVersion } = require('../tools/installer/modules/version-resolver'); const tempRepo39 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-version-marketplace-')); + const tempCacheDir39 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-version-marketplace-cache-')); + const priorCacheEnv39 = process.env.BMAD_EXTERNAL_MODULES_CACHE; + process.env.BMAD_EXTERNAL_MODULES_CACHE = tempCacheDir39; try { const moduleDir = path.join(tempRepo39, 'src'); @@ -2454,7 +2466,48 @@ async function runTests() { ); assert(versionInfo.source === 'marketplace.json', 'resolver reports marketplace.json when it is the only usable metadata source'); } finally { + if (priorCacheEnv39 === undefined) { + delete process.env.BMAD_EXTERNAL_MODULES_CACHE; + } else { + process.env.BMAD_EXTERNAL_MODULES_CACHE = priorCacheEnv39; + } await fs.remove(tempRepo39).catch(() => {}); + await fs.remove(tempCacheDir39).catch(() => {}); + } + } + + // --- package.json lookup must not escape the module repo boundary --- + { + const { resolveModuleVersion } = require('../tools/installer/modules/version-resolver'); + const tempHost39 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-version-boundary-host-')); + const tempCacheDir39 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-version-boundary-cache-')); + const priorCacheEnv39 = process.env.BMAD_EXTERNAL_MODULES_CACHE; + process.env.BMAD_EXTERNAL_MODULES_CACHE = tempCacheDir39; + + try { + const moduleRoot = path.join(tempHost39, 'nested-module'); + const moduleDir = path.join(moduleRoot, 'src'); + await fs.ensureDir(path.join(moduleRoot, '.claude-plugin')); + await fs.ensureDir(moduleDir); + + await fs.writeFile(path.join(tempHost39, 'package.json'), JSON.stringify({ name: 'host-project', version: '9.9.9' }, null, 2) + '\n'); + await fs.writeFile(path.join(moduleDir, 'module.yaml'), ['code: sample-mod', 'module_version: 2.4.0', ''].join('\n')); + await fs.writeFile( + path.join(moduleRoot, '.claude-plugin', 'marketplace.json'), + JSON.stringify({ plugins: [{ name: 'sample-mod', version: '1.7.2' }] }, null, 2) + '\n', + ); + + const versionInfo = await resolveModuleVersion('sample-mod', { moduleSourcePath: moduleDir }); + assert(versionInfo.version === '2.4.0', 'resolver does not read a host project package.json outside the module repo boundary'); + assert(versionInfo.source === 'module.yaml', 'resolver stops at the module repo boundary before climbing into host project metadata'); + } finally { + if (priorCacheEnv39 === undefined) { + delete process.env.BMAD_EXTERNAL_MODULES_CACHE; + } else { + process.env.BMAD_EXTERNAL_MODULES_CACHE = priorCacheEnv39; + } + await fs.remove(tempHost39).catch(() => {}); + await fs.remove(tempCacheDir39).catch(() => {}); } } @@ -2544,6 +2597,31 @@ async function runTests() { } } + // --- Update checks ignore non-semver version strings instead of flagging false positives --- + { + const { Manifest } = require('../tools/installer/core/manifest'); + const manifest39 = new Manifest(); + const originalGetAllModuleVersions39 = manifest39.getAllModuleVersions.bind(manifest39); + const originalFetchNpmVersion39 = manifest39.fetchNpmVersion.bind(manifest39); + + manifest39.getAllModuleVersions = async () => [ + { + name: 'tea', + version: 'workspace-build', + npmPackage: 'bmad-method-test-architecture-enterprise', + }, + ]; + manifest39.fetchNpmVersion = async () => 'latest-build'; + + try { + const updates = await manifest39.checkForUpdates('/unused'); + assert(updates.length === 0, 'update check ignores non-semver version strings instead of reporting misleading updates'); + } finally { + manifest39.getAllModuleVersions = originalGetAllModuleVersions39; + manifest39.fetchNpmVersion = originalFetchNpmVersion39; + } + } + console.log(''); // ============================================================ diff --git a/tools/installer/core/manifest.js b/tools/installer/core/manifest.js index ab673fb19..f20c2397f 100644 --- a/tools/installer/core/manifest.js +++ b/tools/installer/core/manifest.js @@ -396,7 +396,7 @@ class Manifest { const installedVersion = semver.valid(module.version) || semver.valid(semver.coerce(module.version || '')); const availableVersion = semver.valid(latestVersion) || semver.valid(semver.coerce(latestVersion)); - if (!installedVersion || !availableVersion || semver.gt(availableVersion, installedVersion)) { + if (installedVersion && availableVersion && semver.gt(availableVersion, installedVersion)) { updates.push({ name: module.name, installedVersion: module.version, diff --git a/tools/installer/modules/version-resolver.js b/tools/installer/modules/version-resolver.js index fe31af337..7ba42ee30 100644 --- a/tools/installer/modules/version-resolver.js +++ b/tools/installer/modules/version-resolver.js @@ -73,7 +73,7 @@ async function findPackageJsonPath(moduleName, moduleSourcePath) { const roots = await buildSearchRoots(moduleName, moduleSourcePath); for (const root of roots) { - const packageJsonPath = await findNearestUpwardFile(root, 'package.json'); + const packageJsonPath = await findNearestUpwardFile(root.searchDir, 'package.json', { boundaryDir: root.boundaryDir }); if (packageJsonPath) { return packageJsonPath; } @@ -97,7 +97,9 @@ async function findMarketplaceVersion(moduleName, moduleSourcePath, marketplaceP const roots = await buildSearchRoots(moduleName, moduleSourcePath); for (const root of roots) { - const marketplacePath = await findNearestUpwardFile(root, path.join('.claude-plugin', 'marketplace.json')); + const marketplacePath = await findNearestUpwardFile(root.searchDir, path.join('.claude-plugin', 'marketplace.json'), { + boundaryDir: root.boundaryDir, + }); if (!marketplacePath) { continue; } @@ -131,7 +133,10 @@ async function buildSearchRoots(moduleName, moduleSourcePath) { } seen.add(normalized); - roots.push(normalized); + roots.push({ + searchDir: normalized, + boundaryDir: await findSearchBoundary(normalized), + }); }; await addRoot(moduleSourcePath); @@ -145,12 +150,14 @@ async function buildSearchRoots(moduleName, moduleSourcePath) { return roots; } -async function findNearestUpwardFile(startDir, relativeFilePath, maxDepth = DEFAULT_PARENT_DEPTH) { +async function findNearestUpwardFile(startDir, relativeFilePath, options = {}) { const normalizedStartDir = await normalizeExistingDirectory(startDir); if (!normalizedStartDir) { return null; } + const maxDepth = options.maxDepth ?? DEFAULT_PARENT_DEPTH; + const normalizedBoundaryDir = await normalizeDirectoryPath(options.boundaryDir); let currentDir = normalizedStartDir; for (let depth = 0; depth <= maxDepth; depth++) { const candidate = path.join(currentDir, relativeFilePath); @@ -158,6 +165,10 @@ async function findNearestUpwardFile(startDir, relativeFilePath, maxDepth = DEFA return candidate; } + if (normalizedBoundaryDir && currentDir === normalizedBoundaryDir) { + break; + } + const parentDir = path.dirname(currentDir); if (parentDir === currentDir) { break; @@ -168,6 +179,32 @@ async function findNearestUpwardFile(startDir, relativeFilePath, maxDepth = DEFA return null; } +async function findSearchBoundary(startDir) { + const normalizedStartDir = await normalizeExistingDirectory(startDir); + if (!normalizedStartDir) { + return null; + } + + let currentDir = normalizedStartDir; + for (let depth = 0; depth <= DEFAULT_PARENT_DEPTH; depth++) { + if ( + (await fs.pathExists(path.join(currentDir, 'package.json'))) || + (await fs.pathExists(path.join(currentDir, '.claude-plugin', 'marketplace.json'))) || + (await fs.pathExists(path.join(currentDir, '.git'))) + ) { + return currentDir; + } + + const parentDir = path.dirname(currentDir); + if (parentDir === currentDir) { + break; + } + currentDir = parentDir; + } + + return normalizedStartDir; +} + async function normalizeDirectoryPath(candidate) { if (!candidate) { return null;