feat(tasks): convert review-adversarial-general XML task to native skill#1857
Merged
alexeyv merged 4 commits intobmad-code-org:mainfrom Mar 8, 2026
Merged
feat(tasks): convert review-adversarial-general XML task to native skill#1857alexeyv merged 4 commits intobmad-code-org:mainfrom
alexeyv merged 4 commits intobmad-code-org:mainfrom
Conversation
🤖 Augment PR SummarySummary: This PR migrates the core adversarial review task from an XML Changes:
Technical notes: Skill discovery for task-subdirectories relies on reading 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
src/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThe PR transitions the adversarial review functionality from an XML task-based approach to a skill-based architecture. This involves updating multiple workflows to invoke the new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 (5)
src/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md (5)
10-10:⚠️ Potential issue | 🟡 MinorStale "task" terminology in Goal.
Line 10 says "invoke adversarial review task" — should be "skill" for consistency.
📝 Proposed fix
-**Goal:** Construct diff of all changes, invoke adversarial review task, present findings. +**Goal:** Construct diff of all changes, invoke adversarial review skill, present findings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md` at line 10, Update the Goal line that currently reads "invoke adversarial review task" to use the consistent terminology "invoke adversarial review skill"; specifically find the string "invoke adversarial review task" in the document and replace "task" with "skill" so the Goal becomes "Construct diff of all changes, invoke adversarial review skill, present findings."
96-96:⚠️ Potential issue | 🟡 MinorStale "task" in failure modes.
Line 96 says "Invoking task without providing diff input" — should be "skill."
📝 Proposed fix
-- Invoking task without providing diff input +- Invoking skill without providing diff input🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md` at line 96, Update the stale wording in the document: replace the phrase "Invoking task without providing diff input" with "Invoking skill without providing diff input" in quick-dev/step-05-adversarial-review.md (look for the sentence currently using "task") so the failure mode correctly references "skill" instead of "task".
68-68:⚠️ Potential issue | 🟡 MinorStale "task output" reference.
Line 68 says "Capture the findings from the task output" but the invocation is now a skill.
📝 Proposed fix
-Capture the findings from the task output. +Capture the findings from the skill output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md` at line 68, Update the stale phrase "Capture the findings from the task output" in step-05-adversarial-review.md to reference the new invocation type (e.g., "Capture the findings from the skill output"); search for and replace any other occurrences of "task output" within this step to "skill output" (or equivalent consistent wording) so the documentation matches the current implementation and invocation model.
3-3:⚠️ Potential issue | 🟡 MinorStale "task" terminology in frontmatter description.
Line 3 description says "invoke adversarial review task" but the PR converts this to a skill. Should be "invoke adversarial review skill" for consistency.
📝 Proposed fix
-description: 'Construct diff and invoke adversarial review task' +description: 'Construct diff and invoke adversarial review skill'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md` at line 3, Update the YAML frontmatter "description" value in the quick-dev step for the adversarial review (the frontmatter key named description in step-05-adversarial-review.md) to replace the stale word "task" with "skill" so it reads "Construct diff and invoke adversarial review skill" (maintain punctuation and casing).
88-88:⚠️ Potential issue | 🟡 MinorStale "Task invoked" in success metrics.
Line 88 says "Task invoked with diff as input" — should reference "Skill."
📝 Proposed fix
-- Task invoked with diff as input +- Skill invoked with diff as input🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md` at line 88, Replace the stale success-metric string "Task invoked with diff as input" in step-05-adversarial-review.md with a referent to "Skill" (e.g., "Skill invoked with diff as input") so the metric correctly references the Skill; locate the exact string "Task invoked with diff as input" and update it to the new phrasing wherever it appears in the document.
🧹 Nitpick comments (3)
tools/cli/installers/lib/core/manifest-generator.js (1)
459-485: Silent failure when type:skill directory lacks workflow.md or required frontmatter.If a directory has
type: skillin its manifest butworkflow.mdis missing (line 462 check fails) or lacksname/description(line 467 check fails), the code silently continues without any warning. This could cause confusion when a skill doesn't appear in the generated manifest.Consider adding debug logging consistent with the workflow branch (e.g., lines 217-220, 231-234).
🔍 Proposed debug logging
if (subArtifactType === 'skill') { // This subdirectory is a type:skill — process its workflow.md const workflowPath = path.join(fullPath, 'workflow.md'); if (await fs.pathExists(workflowPath)) { // ... existing code ... + } else { + if (process.env.BMAD_DEBUG_MANIFEST === 'true') { + console.log(`[DEBUG] Skipped skill (no workflow.md): ${fullPath}`); + } } }🤖 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 459 - 485, When encountering a subArtifactType === 'skill' that doesn't add a skill entry, add diagnostic logging so missing workflow.md or missing frontmatter fields are visible: in the block handling subArtifactType === 'skill' (use symbols workflowPath, frontmatterMatch, frontmatter, canonicalId, this.skills, this.getInstallToBmad, this.cleanForCSV to find the code), log a warning when fs.pathExists(workflowPath) is false and another warning when frontmatter exists but frontmatter.name or frontmatter.description are missing; mirror the logging style used in the workflow branch so messages clearly state the moduleName and entry.name/canonicalId and indicate whether the file or required fields are missing.src/core/tasks/bmad-review-adversarial-general/workflow.md (2)
23-23: Content type identification has no downstream effect.Step 1 instructs to "Identify content type (diff, branch, uncommitted changes, document, etc.)" but the subsequent steps never reference or use this classification. Either remove the instruction or explain how content type should influence the review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/tasks/bmad-review-adversarial-general/workflow.md` at line 23, The instruction "Identify content type (diff, branch, uncommitted changes, document, etc.)" in Step 1 of workflow.md has no downstream effect; either remove that line or augment it to state how the identified content type affects later steps (e.g., by mapping content types to different review actions or checklists). Update the Step 1 text so it either deletes the content-type identification or replaces it with guidance such as "Identify content type and use X checklist for diffs, Y checklist for branches, Z checklist for uncommitted changes" and reference the affected steps or checklists so the classification is actually used.
27-27: Hardcoded "at least ten issues" may be inappropriate for small artifacts.Requiring "at least ten issues" for every review—regardless of artifact size—could force padding with noise for trivial changes (e.g., a 5-line config change). Consider softening to "identify as many issues as exist, targeting at least ten for substantive artifacts."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/tasks/bmad-review-adversarial-general/workflow.md` at line 27, Replace the hardcoded sentence "Review with extreme skepticism — assume problems exist. Find at least ten issues to fix or improve in the provided content." with a softened, size-aware instruction: keep the "extreme skepticism" guidance but change the requirement to something like "Identify as many issues as exist; for substantive artifacts aim for at least ten issues" so small artifacts aren't forced to pad reviews; update the line in workflow.md (the quoted instruction string) and any related examples or tests that assert exactly ten issues to accept a flexible target or document thresholds for trivial vs substantive artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bmm/workflows/bmad-quick-flow/quick-spec/steps/step-04-review.md`:
- Line 159: Update the stale phrase "Capture the findings from the task output"
to reference the new skill invocation—replace it with something like "Capture
the findings from the skill invocation" or "Capture the findings from the skill
result" in step-04-review.md (search for that exact sentence and update it).
Ensure any surrounding instructions that mention "task output" are also adjusted
to consistently refer to the skill invocation/result so consumers of the step
are not confused.
In `@src/core/tasks/bmad-review-adversarial-general/workflow.md`:
- Around line 22-23: The workflow currently says "If content to review is empty,
ask for clarification and abort" which conflicts with subagent usage that
provides no conversation context (see the "Review subagents get NO conversation
context" note in step-04-review.md); update the logic in workflow.md so that
when invoked as a subagent and the content is empty it does not attempt to ask
for clarification but instead fails immediately with a clear, machine-readable
error message (e.g., "empty_content_for_subagent") and aborts; locate the rule
text "If content to review is empty, ask for clarification and abort" and
replace or augment it to branch on subagent invocation, returning a clear error
message when no conversation context is present.
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 463-467: The YAML frontmatter parse in manifest-generator.js uses
yaml.parse(frontmatterMatch[1]) without error handling, so wrap that parse in a
try-catch around the call to yaml.parse (referencing frontmatterMatch,
frontmatter and workflowPath) and on parse failure log a warning (consistent
with the existing workflow handling style) and skip processing that workflow
rather than letting the exception bubble up; ensure the catch includes the error
details in the warning message.
---
Outside diff comments:
In
`@src/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md`:
- Line 10: Update the Goal line that currently reads "invoke adversarial review
task" to use the consistent terminology "invoke adversarial review skill";
specifically find the string "invoke adversarial review task" in the document
and replace "task" with "skill" so the Goal becomes "Construct diff of all
changes, invoke adversarial review skill, present findings."
- Line 96: Update the stale wording in the document: replace the phrase
"Invoking task without providing diff input" with "Invoking skill without
providing diff input" in quick-dev/step-05-adversarial-review.md (look for the
sentence currently using "task") so the failure mode correctly references
"skill" instead of "task".
- Line 68: Update the stale phrase "Capture the findings from the task output"
in step-05-adversarial-review.md to reference the new invocation type (e.g.,
"Capture the findings from the skill output"); search for and replace any other
occurrences of "task output" within this step to "skill output" (or equivalent
consistent wording) so the documentation matches the current implementation and
invocation model.
- Line 3: Update the YAML frontmatter "description" value in the quick-dev step
for the adversarial review (the frontmatter key named description in
step-05-adversarial-review.md) to replace the stale word "task" with "skill" so
it reads "Construct diff and invoke adversarial review skill" (maintain
punctuation and casing).
- Line 88: Replace the stale success-metric string "Task invoked with diff as
input" in step-05-adversarial-review.md with a referent to "Skill" (e.g., "Skill
invoked with diff as input") so the metric correctly references the Skill;
locate the exact string "Task invoked with diff as input" and update it to the
new phrasing wherever it appears in the document.
---
Nitpick comments:
In `@src/core/tasks/bmad-review-adversarial-general/workflow.md`:
- Line 23: The instruction "Identify content type (diff, branch, uncommitted
changes, document, etc.)" in Step 1 of workflow.md has no downstream effect;
either remove that line or augment it to state how the identified content type
affects later steps (e.g., by mapping content types to different review actions
or checklists). Update the Step 1 text so it either deletes the content-type
identification or replaces it with guidance such as "Identify content type and
use X checklist for diffs, Y checklist for branches, Z checklist for uncommitted
changes" and reference the affected steps or checklists so the classification is
actually used.
- Line 27: Replace the hardcoded sentence "Review with extreme skepticism —
assume problems exist. Find at least ten issues to fix or improve in the
provided content." with a softened, size-aware instruction: keep the "extreme
skepticism" guidance but change the requirement to something like "Identify as
many issues as exist; for substantive artifacts aim for at least ten issues" so
small artifacts aren't forced to pad reviews; update the line in workflow.md
(the quoted instruction string) and any related examples or tests that assert
exactly ten issues to accept a flexible target or document thresholds for
trivial vs substantive artifacts.
In `@tools/cli/installers/lib/core/manifest-generator.js`:
- Around line 459-485: When encountering a subArtifactType === 'skill' that
doesn't add a skill entry, add diagnostic logging so missing workflow.md or
missing frontmatter fields are visible: in the block handling subArtifactType
=== 'skill' (use symbols workflowPath, frontmatterMatch, frontmatter,
canonicalId, this.skills, this.getInstallToBmad, this.cleanForCSV to find the
code), log a warning when fs.pathExists(workflowPath) is false and another
warning when frontmatter exists but frontmatter.name or frontmatter.description
are missing; mirror the logging style used in the workflow branch so messages
clearly state the moduleName and entry.name/canonicalId and indicate whether the
file or required fields are missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b3a42270-75cb-4a8e-922e-2af5be004eb5
⛔ Files ignored due to path filters (1)
src/core/module-help.csvis excluded by!**/*.csv
📒 Files selected for processing (9)
src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.mdsrc/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/workflow.mdsrc/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.mdsrc/bmm/workflows/bmad-quick-flow/quick-spec/steps/step-04-review.mdsrc/core/tasks/bmad-review-adversarial-general/bmad-skill-manifest.yamlsrc/core/tasks/bmad-review-adversarial-general/workflow.mdsrc/core/tasks/bmad-skill-manifest.yamlsrc/core/tasks/review-adversarial-general.xmltools/cli/installers/lib/core/manifest-generator.js
💤 Files with no reviewable changes (3)
- src/core/tasks/bmad-skill-manifest.yaml
- src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/workflow.md
- src/core/tasks/review-adversarial-general.xml
src/bmm/workflows/bmad-quick-flow/quick-spec/steps/step-04-review.md
Outdated
Show resolved
Hide resolved
alexeyv
added a commit
to alexeyv/BMAD-METHOD
that referenced
this pull request
Mar 8, 2026
Address review findings from PR bmad-code-org#1857: replace remaining "task" references with "skill" in workflow steps and test documentation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ve skill Convert the simplest core task (review-adversarial-general.xml) from type:task XML format to type:skill markdown format. This establishes the pattern for converting remaining XML tasks to self-contained skills. - Convert XML task to workflow.md with frontmatter, role, execution steps - Add type:skill manifest for verbatim directory copying - Extend manifest-generator getTasksFromDir to recurse into subdirectories and detect type:skill entries (mirrors existing workflow skill detection) - Update cross-references in quick-dev-new-preview, quick-dev, quick-spec - Update module-help.csv to use skill: prefix
Consumers of review-adversarial-general now invoke by skill name instead of loading via _bmad/ file path. Removes the indirection variable from frontmatter and inlines the skill name directly.
Teach collectWorkflows to also scan the tasks/ subdirectory for type:skill entries. Skills can live anywhere in the source tree — the workflow scanner just needs to look in more places.
Address review findings from PR bmad-code-org#1857: replace remaining "task" references with "skill" in workflow steps and test documentation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alexeyv
force-pushed
the
convert-task-to-skill
branch
from
28b38b6 to
1cbaeae
Compare
This was referenced Mar 9, 2026
3 tasks
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
What changed
Test plan