refactor(agents): remove phase-4 agent personas#2014
Open
Conversation
Skills now serve as direct tools without persona wrappers. Delete dev, pm, qa, sm, and quick-flow-solo-dev agent definitions and clean up references in help CSV, party-mode, and manifests.
🤖 Augment PR SummarySummary: Refactors BMM agents by removing the phase-4 persona “wrapper” agents and relying on skills as direct tools. Changes:
Technical Notes: PR is primarily deletions/cleanup; test updates keep the agent compilation coverage aligned with the new agent set. 🤖 Was this summary useful? React with 👍 or 👎 |
📝 WalkthroughWalkthroughThis PR removes five agent configurations (Developer, Product Manager, QA Engineer, Quick Flow Solo Dev, and Scrum Master) from the codebase along with their manifest entries. It updates related workflow documentation to remove explicit executor references and modifies tests to use the Analyst agent instead of removed agents. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/test-installation-components.js (1)
151-184: Missing negative test coverage for removed agents.This PR removes five agent personas (dev, pm, qa, sm, quick-flow-solo-dev), but there's no test verifying that:
- Attempting to compile the removed agent YAMLs produces an appropriate error
- The removed agent paths no longer exist
- Installations don't reference the deleted agents
A defensive test asserting the removed agent files don't exist would catch accidental re-introduction:
🧪 Suggested negative test
// After Test Suite 1, add: console.log(`${colors.yellow}Test Suite 1b: Removed Agents Verification${colors.reset}\n`); const removedAgents = ['dev', 'pm', 'qa', 'sm', 'quick-flow-solo-dev']; for (const agent of removedAgents) { const agentPath = path.join(projectRoot, `src/bmm/agents/${agent}.agent.yaml`); const exists = await fs.pathExists(agentPath); assert(!exists, `Removed agent ${agent}.agent.yaml should not exist`); }🤖 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 151 - 184, Add a negative test after the existing "Agent Compilation" suite that verifies the five removed agent personas are gone: for each name in ['dev','pm','qa','sm','quick-flow-solo-dev'] assert fs.pathExists(path.join(projectRoot, `src/bmm/agents/${agent}.agent.yaml`)) is false; additionally attempt to call YamlXmlBuilder.buildAgent(...) for one of the removed agent paths and assert it throws (or returns an error) to ensure compilation fails for deleted agents, and scan installation references (e.g., any installer manifest loader used in tests) to assert they do not reference these agent names.
🤖 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 155-162: The test uses a shared temp filename in tempOutput which
can collide across suites; update the test around analystAgentPath and the
builder.buildAgent call to generate a unique temp filename per invocation (for
example append a suite identifier, process.pid, timestamp, or use a
tmp/mkdtemp-based temp file) and ensure the file is cleaned up after the test;
replace the hardcoded path using __dirname + 'temp-analyst-agent.md' with a
deterministic unique-name strategy so Suite 1 and Suite 16 (and parallel runs)
cannot race or read stale output.
- Around line 855-877: Test Suite 16 duplicates Test Suite 1 by compiling the
same analyst agent; either delete the entire Test Suite 16 block or update it to
compile a different agent to increase coverage. Locate the try block that
creates a new YamlXmlBuilder and calls builder.buildAgent with analystAgentPath
and tempOutput (temp-analyst-agent.md) and either remove that whole block (Test
Suite 16 console.log and nested try/catch) or change analystAgentPath to point
to a different agent (e.g., bmad-master) and update the assertion to check for
that agent's expected output so it no longer mirrors Test Suite 1.
---
Nitpick comments:
In `@test/test-installation-components.js`:
- Around line 151-184: Add a negative test after the existing "Agent
Compilation" suite that verifies the five removed agent personas are gone: for
each name in ['dev','pm','qa','sm','quick-flow-solo-dev'] assert
fs.pathExists(path.join(projectRoot, `src/bmm/agents/${agent}.agent.yaml`)) is
false; additionally attempt to call YamlXmlBuilder.buildAgent(...) for one of
the removed agent paths and assert it throws (or returns an error) to ensure
compilation fails for deleted agents, and scan installation references (e.g.,
any installer manifest loader used in tests) to assert they do not reference
these agent names.
🪄 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: e19734ff-4885-4fed-ab41-3f958f60205a
⛔ Files ignored due to path filters (2)
src/bmm/module-help.csvis excluded by!**/*.csvsrc/bmm/teams/default-party.csvis excluded by!**/*.csv
📒 Files selected for processing (10)
src/bmm/agents/bmad-skill-manifest.yamlsrc/bmm/agents/dev.agent.yamlsrc/bmm/agents/pm.agent.yamlsrc/bmm/agents/qa.agent.yamlsrc/bmm/agents/quick-flow-solo-dev.agent.yamlsrc/bmm/agents/sm.agent.yamlsrc/core/skills/bmad-help/workflow.mdsrc/core/skills/bmad-party-mode/steps/step-02-discussion-orchestration.mdsrc/core/skills/bmad-party-mode/workflow.mdtest/test-installation-components.js
💤 Files with no reviewable changes (6)
- src/bmm/agents/sm.agent.yaml
- src/bmm/agents/dev.agent.yaml
- src/bmm/agents/qa.agent.yaml
- src/bmm/agents/quick-flow-solo-dev.agent.yaml
- src/bmm/agents/pm.agent.yaml
- src/bmm/agents/bmad-skill-manifest.yaml
test/test-installation-components.js
Outdated
Comment on lines
155
to
162
| const analystAgentPath = path.join(projectRoot, 'src/bmm/agents/analyst.agent.yaml'); | ||
|
|
||
| // Create temp output path | ||
| const tempOutput = path.join(__dirname, 'temp-pm-agent.md'); | ||
| const tempOutput = path.join(__dirname, 'temp-analyst-agent.md'); | ||
|
|
||
| try { | ||
| const result = await builder.buildAgent(pmAgentPath, null, tempOutput, { includeMetadata: true }); | ||
| const result = await builder.buildAgent(analystAgentPath, null, tempOutput, { includeMetadata: true }); | ||
|
|
There was a problem hiding this comment.
Shared temp filename across test suites risks collision.
Both Test Suite 1 (line 158) and Test Suite 16 (line 862) use the same temp filename temp-analyst-agent.md in __dirname. If cleanup fails in Suite 1 or tests are ever parallelized, Suite 16 could read stale data or encounter race conditions.
Consider using unique filenames like temp-analyst-agent-suite1.md and temp-analyst-agent-suite16.md, or use a unique suffix per invocation.
💡 Suggested fix
- const tempOutput = path.join(__dirname, 'temp-analyst-agent.md');
+ const tempOutput = path.join(__dirname, 'temp-analyst-agent-suite1.md');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const analystAgentPath = path.join(projectRoot, 'src/bmm/agents/analyst.agent.yaml'); | |
| // Create temp output path | |
| const tempOutput = path.join(__dirname, 'temp-pm-agent.md'); | |
| const tempOutput = path.join(__dirname, 'temp-analyst-agent.md'); | |
| try { | |
| const result = await builder.buildAgent(pmAgentPath, null, tempOutput, { includeMetadata: true }); | |
| const result = await builder.buildAgent(analystAgentPath, null, tempOutput, { includeMetadata: true }); | |
| const analystAgentPath = path.join(projectRoot, 'src/bmm/agents/analyst.agent.yaml'); | |
| // Create temp output path | |
| const tempOutput = path.join(__dirname, 'temp-analyst-agent-suite1.md'); | |
| try { | |
| const result = await builder.buildAgent(analystAgentPath, null, tempOutput, { includeMetadata: true }); |
🤖 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 155 - 162, The test uses a
shared temp filename in tempOutput which can collide across suites; update the
test around analystAgentPath and the builder.buildAgent call to generate a
unique temp filename per invocation (for example append a suite identifier,
process.pid, timestamp, or use a tmp/mkdtemp-based temp file) and ensure the
file is cleaned up after the test; replace the hardcoded path using __dirname +
'temp-analyst-agent.md' with a deterministic unique-name strategy so Suite 1 and
Suite 16 (and parallel runs) cannot race or read stale output.
test/test-installation-components.js
Outdated
Comment on lines
855
to
877
| // Test 16: Analyst Agent Compilation | ||
| // ============================================================ | ||
| console.log(`${colors.yellow}Test Suite 16: QA Agent Compilation${colors.reset}\n`); | ||
| console.log(`${colors.yellow}Test Suite 16: Analyst Agent Compilation${colors.reset}\n`); | ||
|
|
||
| try { | ||
| const builder = new YamlXmlBuilder(); | ||
| const qaAgentPath = path.join(projectRoot, 'src/bmm/agents/qa.agent.yaml'); | ||
| const tempOutput = path.join(__dirname, 'temp-qa-agent.md'); | ||
| const analystAgentPath = path.join(projectRoot, 'src/bmm/agents/analyst.agent.yaml'); | ||
| const tempOutput = path.join(__dirname, 'temp-analyst-agent.md'); | ||
|
|
||
| try { | ||
| const result = await builder.buildAgent(qaAgentPath, null, tempOutput, { includeMetadata: true }); | ||
| const result = await builder.buildAgent(analystAgentPath, null, tempOutput, { includeMetadata: true }); | ||
| const compiled = await fs.readFile(tempOutput, 'utf8'); | ||
|
|
||
| assert(compiled.includes('QA Engineer'), 'QA agent compilation includes agent title'); | ||
|
|
||
| assert(compiled.includes('qa-generate-e2e-tests'), 'QA agent menu includes automate workflow'); | ||
| assert(compiled.includes('Business Analyst'), 'Analyst agent compilation includes agent title'); | ||
|
|
||
| // Cleanup | ||
| await fs.remove(tempOutput); | ||
| } catch (error) { | ||
| assert(false, 'QA agent compiles successfully', error.message); | ||
| assert(false, 'Analyst agent compiles successfully', error.message); | ||
| } | ||
| } catch (error) { | ||
| assert(false, 'QA compilation test setup', error.message); | ||
| assert(false, 'Analyst compilation test setup', error.message); | ||
| } |
There was a problem hiding this comment.
Test Suite 16 is now redundant with Test Suite 1.
After this change, both Test Suite 1 (lines 151-183) and Test Suite 16 (lines 855-877) test the exact same analyst agent:
- Same source path:
src/bmm/agents/analyst.agent.yaml - Same temp output file:
temp-analyst-agent.md - Same assertion:
includes('Business Analyst')
Suite 16's single assertion is a strict subset of Suite 1's assertions (which also checks <agent>, <persona>, <menu> tags). This provides no additional coverage.
Consider either:
- Removing Suite 16 entirely, or
- Testing a different remaining agent (e.g.,
bmad-masteror another bmm agent) to maintain agent compilation coverage breadth
🤖 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 855 - 877, Test Suite 16
duplicates Test Suite 1 by compiling the same analyst agent; either delete the
entire Test Suite 16 block or update it to compile a different agent to increase
coverage. Locate the try block that creates a new YamlXmlBuilder and calls
builder.buildAgent with analystAgentPath and tempOutput (temp-analyst-agent.md)
and either remove that whole block (Test Suite 16 console.log and nested
try/catch) or change analystAgentPath to point to a different agent (e.g.,
bmad-master) and update the assertion to check for that agent's expected output
so it no longer mirrors Test Suite 1.
5 tasks
The agent persona files are deleted but their party roster entries must stay so party-mode discussions keep the full cast.
alexeyv
force-pushed
the
delete-phase4-agents
branch
from
f84b41c to
43ca26e
Compare
PM owns phase 2-3 workflows (Create/Validate/Edit PRD, Create Epics). Restore compiled skill directory, skill manifest entry, and module-help agent column values.
# Conflicts: # src/bmm/module-help.csv # test/test-installation-components.js
Collaborator
Author
|
Parking this PR — agent removal is blocked until we resolve the customization story. Two open questions:
Will revisit once the customization approach is decided. |
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
Follow-up
Test plan