refactor(installer): remove dead .agent.yaml/.xml fallback logic

[alexeyv]

Summary

• Remove dead .agent.yaml/.xml fallback blocks from getCanonicalId, getArtifactType, getInstallToBmad in skill-manifest.js
• Simplify scanInstalledModules() to use agents/ dir check + recursive SKILL.md scan, removing workflows//tasks//tools/ directory checks

Test plan

• npm ci && npm run quality passes (227 tests, all lint/format/refs/skills clean)
• grep confirms no .agent.yaml/.xml fallback references remain in skill-manifest.js
• 3-layer adversarial review (blind hunter, edge case hunter, acceptance auditor) — all clear

[augmentcode]

🤖 Augment PR Summary

Summary: Refactors installer module/skill discovery by removing legacy manifest fallbacks that are no longer used.

Changes:

• Simplified scanInstalledModules() to detect modules via agents/ presence and/or recursive SKILL.md discovery.
• Removed dead .agent.yaml/.xml filename fallback logic from getCanonicalId, getArtifactType, and getInstallToBmad.
• Updated docstrings to reflect that multi-entry manifests are keyed by the concrete source filename.

Technical Notes: Assumes installed modules are discoverable through agents/ directories or native SKILL.md entrypoints, and that manifest lookups only need exact filename keys.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

scanInstalledModules() now only treats a directory as a module if it has agents/ or contains a SKILL.md somewhere. If there are still supported/legacy modules that only ship workflows/, tasks/, or tools/ (no agents and no SKILL.md), they’ll now be skipped and won’t have manifests generated—worth confirming this behavior is intentionally unsupported (or covered by a test).

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactored module-detection logic to remove workflows, tasks, and tools directory checks and eliminate filename-based fallback logic for compiled artifacts. Modules are now detected solely via agents/ directory presence or recursive SKILL.md discovery.

Changes

Cohort / File(s)	Summary
Module Detection Criteria tools/cli/installers/lib/core/manifest-generator.js	Simplified scanInstalledModules to remove directory checks for workflows, tasks, and tools, always running _hasSkillMdRecursive() instead of conditionally. Classification now requires only hasAgents or hasSkills instead of multiple directory types.
Manifest Lookup Fallbacks tools/cli/installers/lib/ide/shared/skill-manifest.js	Removed alternate-key fallback logic in getCanonicalId, getArtifactType, and getInstallToBmad that previously derived base filenames and checked compiled artifact keys (.agent.yaml, .xml). Functions now return default values unless exact filename match exists in manifest.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat(tasks): convert review-adversarial-general XML task to native skill #1857: Modifies installer manifest discovery and skill detection logic in the same core files.
• refactor(skills): convert index-docs.xml to native skill directory #1878: Converts XML artifacts into native skill directories with SKILL.md, aligning with the removal of XML/compiled-filename fallback logic.
• refactor(installer): discover skills by SKILL.md instead of manifest YAML #2082: Directly refactors skill-discovery in manifest-generator.js to rely on SKILL.md and agents instead of legacy detection methods.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and concisely describes the main change: removing dead fallback logic for .agent.yaml/.xml files from the installer code.
Description check	✅ Passed	The description is directly related to the changeset, outlining the removal of fallback logic and simplification of module detection, with test verification details.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch remove-legacy-fallbacks

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tools/cli/installers/lib/core/manifest-generator.js (1)

707-713: Short-circuit the recursive SKILL scan when agents/ already exists.

On Line 710, _hasSkillMdRecursive(modulePath) runs even when hasAgents is already true, which adds avoidable filesystem traversal cost.

⚡ Suggested optimization

-        const hasAgents = await fs.pathExists(path.join(modulePath, 'agents'));
-        const hasSkills = await this._hasSkillMdRecursive(modulePath);
+        const hasAgents = await fs.pathExists(path.join(modulePath, 'agents'));
+        const hasSkills = hasAgents ? false : await this._hasSkillMdRecursive(modulePath);
 
         if (hasAgents || hasSkills) {
           modules.push(entry.name);
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tools/cli/installers/lib/core/manifest-generator.js` around lines 707 - 713,
The code always calls this._hasSkillMdRecursive(modulePath) even when hasAgents
is true; avoid the extra I/O by short-circuiting the skill scan: compute
hasSkills only when hasAgents is false (e.g., set hasSkills = hasAgents ? false
: await this._hasSkillMdRecursive(modulePath) or call _hasSkillMdRecursive only
inside the if (!hasAgents) branch) and then push entry.name into modules when
hasAgents || hasSkills; update references to modulePath, hasAgents, hasSkills,
and _hasSkillMdRecursive accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 707-713: The code always calls
this._hasSkillMdRecursive(modulePath) even when hasAgents is true; avoid the
extra I/O by short-circuiting the skill scan: compute hasSkills only when
hasAgents is false (e.g., set hasSkills = hasAgents ? false : await
this._hasSkillMdRecursive(modulePath) or call _hasSkillMdRecursive only inside
the if (!hasAgents) branch) and then push entry.name into modules when hasAgents
|| hasSkills; update references to modulePath, hasAgents, hasSkills, and
_hasSkillMdRecursive accordingly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: ac6796ce-7e37-4753-bade-43c4def0b116

📥 Commits

Reviewing files that changed from the base of the PR and between 93a1e1d and 2c05fe7.

📒 Files selected for processing (2)

• tools/cli/installers/lib/core/manifest-generator.js
• tools/cli/installers/lib/ide/shared/skill-manifest.js