fix(installer): simplify install summary#1915
Merged
alexeyv merged 3 commits intobmad-code-org:mainfrom Mar 12, 2026
Merged
Conversation
🤖 Augment PR SummarySummary: This PR simplifies the installer’s final summary output and fixes skill-format install reporting to avoid double-counting when multiple artifacts map to the same skill directory. Changes:
Technical Notes: Unique skill counting is based on directory names written during setup, so collisions (e.g., workflow-skill + verbatim skill with same canonicalId) report once while preserving existing per-artifact counters. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis PR introduces per-skill write tracking and skill-directory count reporting to the IDE installer system. It adds a new test suite validating skill collision scenarios, enhances install summary rendering with dynamic step labeling, implements a skillWriteTracker in the config-driven installer to count skill files created, and modifies the result reporting to prioritize skillDirectories counts when available. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/cli/installers/lib/ide/_config-driven.js (1)
132-167:⚠️ Potential issue | 🟠 MajorKeep
skillWriteTrackerlocal to a single install run.
IdeManagerreuses one handler instance per platform, but this Set is stored on the handler and then mutated later bywriteSkillFile()andinstallVerbatimSkills(). Two overlappingsetup()calls on the same IDE will share and clear the same tracker, soskillDirectoriesand summary output can bleed across installs. Pass a localSetthrough the call chain instead of hanging per-run state onthis; at minimum, clear it in afinally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/_config-driven.js` around lines 132 - 167, The current this.skillWriteTracker is shared across handler instances and causes bleed between overlapping installs; instead create a local Set (e.g., const skillWriteTracker = config.skill_format ? new Set() : null) inside the install/setup call and pass that Set into installVerbatimSkills(...) and writeSkillFile(...) (update those method signatures/call sites to accept the tracker), use skillWriteTracker.size for results.skillDirectories, and remove reliance on this.skillWriteTracker; if you cannot change all call sites immediately, ensure the field is cleared in a finally block after await this.printSummary(...) to avoid cross-run leakage.
🧹 Nitpick comments (5)
test/test-installation-components.js (3)
1848-1852: These assertions are pinning intermediate bookkeeping, not the contract.The behavior this PR changes is the unique directory count (
detail/skillDirectories). Exactresults.agents,results.workflows, andresults.skillsvalues come from_config-driven.jsinternals and will break on harmless generator refactors. I'd keep the assertions on unique directories and on-disk paths, and drop the incidental per-bucket counts unless you intend them to be stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-installation-components.js` around lines 1848 - 1852, The tests are asserting internal bookkeeping values (result.handlerResult.results.agents, .workflows, .skills) which are not part of the public contract and may change with generator refactors; remove or replace those three assertions and keep only the assertions that check the unique directory count (result.detail and result.handlerResult.results.skillDirectories) and any assertions that validate on-disk paths/installed skill directories; specifically, delete or comment out the lines asserting results.agents, results.workflows, and results.skills and ensure the test still verifies unique skill directory behavior via result.detail and skillDirectories and any filesystem path checks.
90-117: Avoid hand-writing manifest CSV schemas in this fixture.These inline headers duplicate production manifest formats by hand. The next schema change in
agent-manifest.csv,workflow-manifest.csv, orskill-manifest.csvwill make this regression drift away from the real installer inputs. ReuseManifestGeneratoror a shared fixture helper so the test stays aligned with production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-installation-components.js` around lines 90 - 117, The test is hand-writing CSV manifest headers and rows for agent-manifest.csv, workflow-manifest.csv, and skill-manifest.csv which will drift from production schemas; replace the manual fs.writeFile calls in test/test-installation-components.js with the shared ManifestGenerator (or the existing fixture helper) to produce these manifests (use the generator to emit agent, workflow, and skill manifests with the same sample entries currently present), updating references to the generated file paths instead of the hard-coded CSV strings so the test remains in sync with production schema changes.
1853-1857: Assert which collidingbmad-helpcontent is supposed to survive.This fixture deliberately creates a workflow/native-skill name collision, but the test only checks that the directory exists. Add a content assertion on
.agent/skills/bmad-help/SKILL.mdso the suite locks down the intended overwrite precedence instead of just the count.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test-installation-components.js` around lines 1853 - 1857, The test currently only asserts the existence of .agent/skills/bmad-help/SKILL.md; update it to also assert the actual file content to lock down which source wins the collision by reading the file at '.agent/skills/bmad-help/SKILL.md' and comparing its contents against the expected SKILL.md from the fixture that should take precedence (i.e., the workflow or the native-skill fixture); implement this by using fs.readFile or fs.readFileSync and an assert.strictEqual (or assert.include) to compare to the expected fixture content so the test fails if the wrong variant survived.tools/cli/installers/lib/core/installer.js (1)
1389-1391: Use IDE display names in this final note, not platform keys.Successful IDE rows are still labeled with internal IDs like
github-copilotandrovo-dev. Since this summary is user-facing, it should render each handler'sdisplayNameinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/core/installer.js` around lines 1389 - 1391, The final note is using internal platform keys (e.g., r.step / selectedIdes entries) as labels; change it to use each IDE handler's user-facing displayName instead: when you set stepLabel (currently in the branch that checks selectedIdes.has(String(r.step).toLowerCase())), look up the corresponding handler by the internal id (r.step) and assign handler.displayName to stepLabel (fall back to r.step only if displayName is missing). Update any logic that compares against selectedIdes or lowercased ids to still find the handler (use the same id normalization) but render displayName in the summary.tools/cli/installers/lib/ide/manager.js (1)
165-173: Extract the install-result formatter into one shared helper.This precedence rule now exists here and again in
tools/cli/installers/lib/ide/_config-driven.js:715-723. The next tweak to skill reporting will drift one of them. Centralize the formatter and reuse it for bothdetailand the console summary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/manager.js` around lines 165 - 173, Extract the repeated precedence logic into a single helper function (e.g., formatInstallResult(result) or formatInstallResultSummary) that accepts the install result object (r) and returns the parts/summary string; replace the duplicated block in manager.js (the if (r.skillDirectories > 0) ... else ...) and the matching block in _config-driven.js with calls to this new helper, and export/import the helper so both the detail renderer and the console summary use the same implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/test-installation-components.js`:
- Around line 1827-1863: Add an automated test that exercises the
no-IDE/"--tools none" install-summary branch so
Installer.renderInstallSummary()'s _bmad-only path is covered: invoke
IdeManager.setup(...) with the same fixture used elsewhere but pass the options
that simulate no IDE/tools (e.g. tools: 'none' or the equivalent flag used by
Installer) so the Installer.renderInstallSummary() code path is executed, then
assert the returned summary/detail string contains the expected _bmad-only
wording and any handlerResult fields the PR changed; place this alongside the
existing Skill Count test and reference IdeManager.setup and
Installer.renderInstallSummary in the test to ensure the specific branch is hit.
- Around line 84-86: The helper createSkillCollisionFixture leaks the outer temp
directory because mkdtemp() creates fixtureRoot but only the child fixtureDir
(_bmad) is removed; update createSkillCollisionFixture to either create the BMAD
fixture directly in the temporary directory returned by mkdtemp (so fixtureRoot
=== fixtureDir) or return fixtureRoot to the caller so the caller can clean it
up; ensure references to fixtureRoot, fixtureDir and the mkdtemp call are
adjusted so the created temp root is the one removed in the test teardown.
In `@tools/cli/installers/lib/core/installer.js`:
- Around line 1409-1410: The summary currently hardcodes '_bmad' and always
includes an IDE-only footer even when (context.ides || []).length === 0; update
the branch that pushes the "No IDE selected" message to use BMAD_FOLDER_NAME
instead of the literal '_bmad' and make the footer that tells the user to run
`bmad-help` conditional on there being at least one IDE selected (i.e., the same
check using context.ides). Locate and modify the lines around the "No IDE
selected" push and the footer emission code so the footer is emitted only when
context.ides has items and the folder name is referenced via BMAD_FOLDER_NAME.
---
Outside diff comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 132-167: The current this.skillWriteTracker is shared across
handler instances and causes bleed between overlapping installs; instead create
a local Set (e.g., const skillWriteTracker = config.skill_format ? new Set() :
null) inside the install/setup call and pass that Set into
installVerbatimSkills(...) and writeSkillFile(...) (update those method
signatures/call sites to accept the tracker), use skillWriteTracker.size for
results.skillDirectories, and remove reliance on this.skillWriteTracker; if you
cannot change all call sites immediately, ensure the field is cleared in a
finally block after await this.printSummary(...) to avoid cross-run leakage.
---
Nitpick comments:
In `@test/test-installation-components.js`:
- Around line 1848-1852: The tests are asserting internal bookkeeping values
(result.handlerResult.results.agents, .workflows, .skills) which are not part of
the public contract and may change with generator refactors; remove or replace
those three assertions and keep only the assertions that check the unique
directory count (result.detail and
result.handlerResult.results.skillDirectories) and any assertions that validate
on-disk paths/installed skill directories; specifically, delete or comment out
the lines asserting results.agents, results.workflows, and results.skills and
ensure the test still verifies unique skill directory behavior via result.detail
and skillDirectories and any filesystem path checks.
- Around line 90-117: The test is hand-writing CSV manifest headers and rows for
agent-manifest.csv, workflow-manifest.csv, and skill-manifest.csv which will
drift from production schemas; replace the manual fs.writeFile calls in
test/test-installation-components.js with the shared ManifestGenerator (or the
existing fixture helper) to produce these manifests (use the generator to emit
agent, workflow, and skill manifests with the same sample entries currently
present), updating references to the generated file paths instead of the
hard-coded CSV strings so the test remains in sync with production schema
changes.
- Around line 1853-1857: The test currently only asserts the existence of
.agent/skills/bmad-help/SKILL.md; update it to also assert the actual file
content to lock down which source wins the collision by reading the file at
'.agent/skills/bmad-help/SKILL.md' and comparing its contents against the
expected SKILL.md from the fixture that should take precedence (i.e., the
workflow or the native-skill fixture); implement this by using fs.readFile or
fs.readFileSync and an assert.strictEqual (or assert.include) to compare to the
expected fixture content so the test fails if the wrong variant survived.
In `@tools/cli/installers/lib/core/installer.js`:
- Around line 1389-1391: The final note is using internal platform keys (e.g.,
r.step / selectedIdes entries) as labels; change it to use each IDE handler's
user-facing displayName instead: when you set stepLabel (currently in the branch
that checks selectedIdes.has(String(r.step).toLowerCase())), look up the
corresponding handler by the internal id (r.step) and assign handler.displayName
to stepLabel (fall back to r.step only if displayName is missing). Update any
logic that compares against selectedIdes or lowercased ids to still find the
handler (use the same id normalization) but render displayName in the summary.
In `@tools/cli/installers/lib/ide/manager.js`:
- Around line 165-173: Extract the repeated precedence logic into a single
helper function (e.g., formatInstallResult(result) or
formatInstallResultSummary) that accepts the install result object (r) and
returns the parts/summary string; replace the duplicated block in manager.js
(the if (r.skillDirectories > 0) ... else ...) and the matching block in
_config-driven.js with calls to this new helper, and export/import the helper so
both the detail renderer and the console summary use the same implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d94bfda8-2492-4299-a42a-1199320477ed
📒 Files selected for processing (4)
test/test-installation-components.jstools/cli/installers/lib/core/installer.jstools/cli/installers/lib/ide/_config-driven.jstools/cli/installers/lib/ide/manager.js
Collaborator
|
@alexeyv if you do this one, if not already, please remove the count of tools and other things so its just total skills and total agents |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Return fixture root from createSkillCollisionFixture so cleanup removes the parent temp directory, not just the _bmad child - Only show bmad-help next-step line when IDEs are configured
Collaborator
Author
Triage Summary — Review FindingsInline threads (all resolved above):
Review body findings:
Auto-dismissed (LOW severity, noise):
|
alexeyv
added a commit
to alexeyv/BMAD-METHOD
that referenced
this pull request
Mar 13, 2026
* fix(installer): simplify install summary * style: fix prettier formatting in test file Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(installer): clean up temp dir leak and conditional IDE footer - Return fixture root from createSkillCollisionFixture so cleanup removes the parent temp directory, not just the _bmad child - Only show bmad-help next-step line when IDEs are configured --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_bmad-only installsskillsandagentsVerification
node test/test-installation-components.js(passed in/Users/alex/src/bmad/mainbefore branching)--tools antigravity--tools none2 skills, 2 agents