fix(installer): address CodeRabbit review findings

- ui.js: skip stable-channel upgrade classification when the user has
  already declared intent via --pin/--next=/--channel or the review
  gate. Prevents the decline / major-refused / fetch-error branches
  from silently overwriting an explicit pin with prev.version.
- external-manager.js: short-circuit cloneExternalModule when the
  requested plan matches an existing in-process resolution and the
  cache is valid. Avoids redundant resolveChannel() + git fetch on
  every same-plan lookup in a single install.
- installer.js: fall back to CommunityModuleManager.getResolution()
  when no external resolution exists, so community module result
  rows carry newChannel/newSha instead of null under --next/--pin.
- installer.js: don't label a module as "no change" when its version
  string is 'main'/'HEAD' — the SHA may have moved and preVersions
  doesn't track the prior SHA. Show "(refreshed)" instead.
- official-modules.js: match versionInfo.version to the manifest's
  cloneRef || (hasGitClone ? 'main' : version) expression so summary
  lines report the cloned ref for git-backed custom installs.
- install-bmad.md: clarify that sha is only written for git-backed
  modules and that rerunning the same --modules on another machine
  does not reproduce stable-channel installs — convert recorded tags
  into explicit --pin flags for cross-machine reproducibility.
This commit is contained in:
Brian Madison 2026-04-23 22:57:11 -05:00
parent b292cb9bfe
commit b768e0b369
5 changed files with 83 additions and 42 deletions

View File

@ -56,7 +56,7 @@ Two independent axes control what ends up on disk.
Every external module — bmb, cis, gds, tea, and any community module — installs on one of three channels: Every external module — bmb, cis, gds, tea, and any community module — installs on one of three channels:
| Channel | What gets installed | Who picks this | | Channel | What gets installed | Who picks this |
| --- | --- | --- | | ------------------ | ---------------------------------------------------------------------------- | --------------------------------------- |
| `stable` (default) | Highest released semver tag. Prereleases like `v2.0.0-alpha.1` are excluded. | Most users | | `stable` (default) | Highest released semver tag. Prereleases like `v2.0.0-alpha.1` are excluded. | Most users |
| `next` | Main branch HEAD at install time | Contributors, early adopters | | `next` | Main branch HEAD at install time | Contributors, early adopters |
| `pinned` | A specific tag you name | Enterprise installs, CI reproducibility | | `pinned` | A specific tag you name | Enterprise installs, CI reproducibility |
@ -68,7 +68,7 @@ Channels are per-module. You can run bmb on `next` while leaving cis on `stable`
The `bmad-method` npm package itself has two dist-tags: The `bmad-method` npm package itself has two dist-tags:
| Command | What you get | | Command | What you get |
| --- | --- | | ------------------------------------- | ----------------------------------------------------------------- |
| `npx bmad-method install` (`@latest`) | Latest stable installer release | | `npx bmad-method install` (`@latest`) | Latest stable installer release |
| `npx bmad-method@next install` | Latest prerelease installer, auto-published on every push to main | | `npx bmad-method@next install` | Latest prerelease installer, auto-published on every push to main |
@ -89,7 +89,7 @@ They're stapled to the installer binary you ran:
Running `npx bmad-method install` in a directory that already contains `_bmad/` gives you a menu: Running `npx bmad-method install` in a directory that already contains `_bmad/` gives you a menu:
| Choice | What it does | | Choice | What it does |
| --- | --- | | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------- |
| **Quick Update** | Re-runs the install with your existing settings. Refreshes files, applies patches and minor stable upgrades, refuses major upgrades. Fast, non-interactive. | | **Quick Update** | Re-runs the install with your existing settings. Refreshes files, applies patches and minor stable upgrades, refuses major upgrades. Fast, non-interactive. |
| **Modify Install** | Full interactive flow. Add or remove modules, reconfigure settings, optionally review and switch channels for existing modules. | | **Modify Install** | Full interactive flow. Add or remove modules, reconfigure settings, optionally review and switch channels for existing modules. |
@ -98,7 +98,7 @@ Running `npx bmad-method install` in a directory that already contains `_bmad/`
When Modify detects a newer stable tag for a module you've installed on `stable`, it classifies the diff and prompts accordingly: When Modify detects a newer stable tag for a module you've installed on `stable`, it classifies the diff and prompts accordingly:
| Upgrade type | Example | Default | | Upgrade type | Example | Default |
| --- | --- | --- | | ------------ | --------------- | ------- |
| Patch | v1.7.0 → v1.7.1 | Y | | Patch | v1.7.0 → v1.7.1 | Y |
| Minor | v1.7.0 → v1.8.0 | Y | | Minor | v1.7.0 → v1.8.0 | Y |
| Major | v1.7.0 → v2.0.0 | **N** | | Major | v1.7.0 → v2.0.0 | **N** |
@ -118,7 +118,7 @@ Under `--yes`, patch and minor upgrades apply automatically. Majors stay frozen
### Flag reference ### Flag reference
| Flag | Purpose | | Flag | Purpose |
| --- | --- | | ------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------- |
| `--yes`, `-y` | Skip all prompts; accept flag values + defaults | | `--yes`, `-y` | Skip all prompts; accept flag values + defaults |
| `--directory <path>` | Install into this directory (default: current working dir) | | `--directory <path>` | Install into this directory (default: current working dir) |
| `--modules <a,b,c>` | Exact module set. Core is auto-added. Not a delta — list everything you want kept. | | `--modules <a,b,c>` | Exact module set. Core is auto-added. Not a delta — list everything you want kept. |
@ -198,7 +198,14 @@ modules:
repoUrl: https://github.com/bmad-code-org/bmad-builder repoUrl: https://github.com/bmad-code-org/bmad-builder
``` ```
The `sha` field is always populated. For reproducible installs, pass the same `--modules` + `--pin` / `--next=` combination on a fresh machine and you'll land on the same commits. The `sha` field is written for git-backed modules (external, community, and URL-based custom). Bundled modules (core, bmm) and local-path custom modules don't have one — their code travels with the installer binary or your filesystem, not a cloneable ref.
For cross-machine reproducibility, don't rely on rerunning the same `--modules` command. Stable-channel installs resolve to the highest released tag **at install time**, so a later rerun lands on whatever has been released since. Convert the recorded tags from `manifest.yaml` into explicit `--pin` flags on the target machine, e.g.:
```bash
npx bmad-method install --yes --modules bmb,cis \
--pin bmb=v1.7.0 --pin cis=v0.4.2 --tools none
```
## Troubleshooting ## Troubleshooting

View File

@ -614,20 +614,26 @@ class Installer {
const displayName = moduleInfo?.name || moduleName; const displayName = moduleInfo?.name || moduleName;
const externalResolution = officialModules.externalModuleManager.getResolution(moduleName); const externalResolution = officialModules.externalModuleManager.getResolution(moduleName);
let communityResolution = null;
if (!externalResolution) {
const { CommunityModuleManager } = require('../modules/community-manager');
communityResolution = new CommunityModuleManager().getResolution(moduleName);
}
const resolution = externalResolution || communityResolution;
const cachedResolution = CustomModuleManager._resolutionCache.get(moduleName); const cachedResolution = CustomModuleManager._resolutionCache.get(moduleName);
const versionInfo = await resolveModuleVersion(moduleName, { const versionInfo = await resolveModuleVersion(moduleName, {
moduleSourcePath: sourcePath, moduleSourcePath: sourcePath,
fallbackVersion: externalResolution?.version || cachedResolution?.version, fallbackVersion: resolution?.version || cachedResolution?.version,
marketplacePluginNames: cachedResolution?.pluginName ? [cachedResolution.pluginName] : [], marketplacePluginNames: cachedResolution?.pluginName ? [cachedResolution.pluginName] : [],
}); });
// Prefer the git tag recorded by the external resolution (e.g. "v1.7.0") over // Prefer the git tag recorded by the resolution (e.g. "v1.7.0") over
// the on-disk package.json (which may be ahead of the released tag). // the on-disk package.json (which may be ahead of the released tag).
const version = externalResolution?.version || versionInfo.version || ''; const version = resolution?.version || versionInfo.version || '';
addResult(displayName, 'ok', '', { addResult(displayName, 'ok', '', {
moduleCode: moduleName, moduleCode: moduleName,
newVersion: version, newVersion: version,
newChannel: externalResolution?.channel || null, newChannel: resolution?.channel || null,
newSha: externalResolution?.sha || null, newSha: resolution?.sha || null,
}); });
} }
} }
@ -1114,8 +1120,15 @@ class Installer {
return v; return v;
}; };
const newV = fmt(r.newVersion, r.newSha); const newV = fmt(r.newVersion, r.newSha);
if (oldVersion && oldVersion === r.newVersion) { // 'main'/'HEAD' strings only identify the channel, not the commit, so
// we can't assert "no change" without comparing SHAs — and preVersions
// doesn't carry the old SHA. Render these as a refresh instead of a
// false-negative "no change".
const isMainLike = oldVersion === 'main' || oldVersion === 'HEAD';
if (oldVersion && oldVersion === r.newVersion && !isMainLike) {
detail = ` (${newV}, no change)`; detail = ` (${newV}, no change)`;
} else if (oldVersion && isMainLike) {
detail = ` (${newV}, refreshed)`;
} else if (oldVersion) { } else if (oldVersion) {
detail = ` (${fmt(oldVersion, r.newSha)}${newV})`; detail = ` (${fmt(oldVersion, r.newSha)}${newV})`;
} else { } else {

View File

@ -249,6 +249,17 @@ class ExternalModuleManager {
registryDefault: moduleInfo.defaultChannel, registryDefault: moduleInfo.defaultChannel,
}); });
// Same-plan short-circuit: a single install calls cloneExternalModule
// several times (config collection, directory setup, help-catalog rebuild)
// with the same channelOptions. The first call resolves + clones; later
// calls with an identical plan and a valid cache should return immediately
// instead of re-running resolveChannel() and `git fetch` (slow; can fail
// on flaky networks even though the tagCache dedupes the GitHub API hit).
if (existingResolution && haveUsableCache && existingResolution.channel === planEntry.channel) {
const samePin = planEntry.channel !== 'pinned' || existingResolution.version === planEntry.pin;
if (samePin) return moduleCacheDir;
}
let resolved; let resolved;
try { try {
resolved = await resolveChannel({ resolved = await resolveChannel({

View File

@ -386,7 +386,10 @@ class OfficialModules {
success: true, success: true,
module: resolved.code, module: resolved.code,
path: targetPath, path: targetPath,
versionInfo: { version: resolved.cloneRef || resolved.version || '' }, // 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 || '') },
}; };
} }

View File

@ -1899,6 +1899,13 @@ class UI {
// Stable channel: check for a newer released tag. // Stable channel: check for a newer released tag.
if (!parsed) continue; if (!parsed) continue;
// Respect explicit CLI intent (--pin / --next=CODE / --all-*) and any
// choice the user already made in the earlier review gate. Without this
// guard the upgrade classifier below would unconditionally call
// `channelOptions.pins.set(code, prev.version)` on decline/major-refuse/
// fetch-error, silently clobbering the user's override.
const alreadyDecided = channelOptions.global || channelOptions.nextSet.has(code) || channelOptions.pins.has(code);
if (alreadyDecided) continue;
let tags; let tags;
try { try {
tags = await fetchStableTags(parsed.owner, parsed.repo); tags = await fetchStableTags(parsed.owner, parsed.repo);