refactor(installer): remove legacy workflow, task, and agent IDE generators#2078
Merged
refactor(installer): remove legacy workflow, task, and agent IDE generators#2078
Conversation
…rators All platforms now use skill_format exclusively. The old WorkflowCommandGenerator, TaskToolCommandGenerator, and AgentCommandGenerator code paths in _config-driven.js were no-ops — collectSkills claims every directory before the legacy collectors run, making their manifests empty. Removed: - workflow-command-generator.js (deleted) - task-tool-command-generator.js (deleted) - writeAgentArtifacts, writeWorkflowArtifacts, writeTaskToolArtifacts - AgentCommandGenerator import from _config-driven.js - Legacy artifact_types/agents/workflows/tasks result fields Simplified installToTarget, installToMultipleTargets, printSummary, and IDE manager detail builder to skills-only. Updated test fixture to use SKILL.md format instead of old agent format.
🤖 Augment PR SummarySummary: This PR refactors IDE skill installation to drop legacy agent/workflow/task/tool generators and rely exclusively on the native Changes:
Technical Notes: The installer now treats 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe pull request transitions the IDE installer from an agent/workflow-based architecture to a skill-first approach. Legacy artifact generators for workflows, tasks, and tools are removed entirely. The installer now only processes fixtures containing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
test/test-installation-components.js (1)
67-82: Add one non-SKILL.mdfile to the shared fixture.With only
SKILL.mdhere, the install suites still pass ifinstallVerbatimSkills()regresses to copying just the launcher and drops the rest of the skill directory. A tinyworkflow.mdplus one representative assertion would keep coverage on the new verbatim-copy path.🤖 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 67 - 82, Add a non-SKILL.md file into the shared fixture so the verbatim-copy path is exercised: in test-installation-components.js, after creating SKILL.md and bmad-skill-manifest.yaml (using skillDir), write a small workflow.md (or workflow.yaml) file into skillDir with one or two lines of content and ensure the manifest/fixture reflects it if needed; also add one minimal assertion in the same test that the installed skill directory contains that workflow.md (so installVerbatimSkills is validated to copy full directory, not just the launcher).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 117-119: The current early-return when (!config.skill_format)
incorrectly reports success after setup()/cleanup() may have removed an install;
change that branch to fail loudly: instead of returning { success: true,
results: { skills: 0 } }, return or throw a failure indicating an invalid
installer config (e.g., set success: false and include an error message
referencing missing config.skill_format) so callers and process exit codes
reflect the error; update the check in _config-driven.js that references
config.skill_format and ensure any caller of setup()/cleanup() propagates
non-zero exit behavior.
---
Nitpick comments:
In `@test/test-installation-components.js`:
- Around line 67-82: Add a non-SKILL.md file into the shared fixture so the
verbatim-copy path is exercised: in test-installation-components.js, after
creating SKILL.md and bmad-skill-manifest.yaml (using skillDir), write a small
workflow.md (or workflow.yaml) file into skillDir with one or two lines of
content and ensure the manifest/fixture reflects it if needed; also add one
minimal assertion in the same test that the installed skill directory contains
that workflow.md (so installVerbatimSkills is validated to copy full directory,
not just the launcher).
🪄 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: 3b06f4d1-83dd-477e-9090-1ba507e16cd4
📒 Files selected for processing (6)
test/test-installation-components.jstools/cli/installers/lib/ide/_config-driven.jstools/cli/installers/lib/ide/manager.jstools/cli/installers/lib/ide/shared/agent-command-generator.jstools/cli/installers/lib/ide/shared/task-tool-command-generator.jstools/cli/installers/lib/ide/shared/workflow-command-generator.js
💤 Files with no reviewable changes (3)
- tools/cli/installers/lib/ide/shared/agent-command-generator.js
- tools/cli/installers/lib/ide/shared/workflow-command-generator.js
- tools/cli/installers/lib/ide/shared/task-tool-command-generator.js
- Fix temp dir leak in test fixture cleanup (use path.dirname) - Fail loudly when skill_format missing instead of silent success - Add workflow.md to test fixture for verbatim-copy coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
F4 (nitpick): Add non-SKILL.md fixture file for verbatim-copy coverage Fixed in 6c761bb. Added |
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
1 participant
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
WorkflowCommandGenerator,TaskToolCommandGenerator, andAgentCommandGeneratorfrom IDE skill installationskill_formatexclusively viainstallVerbatimSkills— the old generators were no-ops becausecollectSkillsclaims every directory before legacy collectors runworkflow-command-generator.jsandtask-tool-command-generator.js(715 lines removed)installToTarget,installToMultipleTargets,printSummary, and IDE manager detail builder to skills-onlyTest plan
.claude/skills/output (43 skills, content-identical to baseline)_bmad/directory identical between baseline and post-change (only timestamps differ)npm run qualitypasses (format, lint, markdown lint, docs build, tests, ref validation, skill validation)