diff --git a/src/core/tasks/review-adversarial-general.xml b/src/core/tasks/review-adversarial-general.xml index 4e68ff9a..d6a6f44c 100644 --- a/src/core/tasks/review-adversarial-general.xml +++ b/src/core/tasks/review-adversarial-general.xml @@ -1,7 +1,7 @@ - + Cynically review content and produce findings diff --git a/src/modules/bmm/workflows/4-implementation/code-review/checklist.md b/src/modules/bmm/workflows/4-implementation/code-review/checklist.md deleted file mode 100644 index f213a6b9..00000000 --- a/src/modules/bmm/workflows/4-implementation/code-review/checklist.md +++ /dev/null @@ -1,23 +0,0 @@ -# Senior Developer Review - Validation Checklist - -- [ ] Story file loaded from `{{story_path}}` -- [ ] Story Status verified as reviewable (review) -- [ ] Epic and Story IDs resolved ({{epic_num}}.{{story_num}}) -- [ ] Story Context located or warning recorded -- [ ] Epic Tech Spec located or warning recorded -- [ ] Architecture/standards docs loaded (as available) -- [ ] Tech stack detected and documented -- [ ] MCP doc search performed (or web fallback) and references captured -- [ ] Acceptance Criteria cross-checked against implementation -- [ ] File List reviewed and validated for completeness -- [ ] Tests identified and mapped to ACs; gaps noted -- [ ] Code quality review performed on changed files -- [ ] Security review performed on changed files and dependencies -- [ ] Outcome decided (Approve/Changes Requested/Blocked) -- [ ] Review notes appended under "Senior Developer Review (AI)" -- [ ] Change Log updated with review entry -- [ ] Status updated according to settings (if enabled) -- [ ] Sprint status synced (if sprint tracking enabled) -- [ ] Story saved successfully - -_Reviewer: {{user_name}} on {{date}}_ diff --git a/src/modules/bmm/workflows/4-implementation/code-review/instructions.xml b/src/modules/bmm/workflows/4-implementation/code-review/instructions.xml deleted file mode 100644 index e5649559..00000000 --- a/src/modules/bmm/workflows/4-implementation/code-review/instructions.xml +++ /dev/null @@ -1,227 +0,0 @@ - - The workflow execution engine is governed by: {project-root}/_bmad/core/tasks/workflow.xml - You MUST have already loaded and processed: {installed_path}/workflow.yaml - Communicate all responses in {communication_language} and language MUST be tailored to {user_skill_level} - Generate all documents in {document_output_language} - - đŸ”Ĩ YOU ARE AN ADVERSARIAL CODE REVIEWER - Find what's wrong or missing! đŸ”Ĩ - Your purpose: Validate story file claims against actual implementation - Challenge everything: Are tasks marked [x] actually done? Are ACs really implemented? - Find 3-10 specific issues in every review minimum - no lazy "looks good" reviews - YOU are so much better than the dev agent - that wrote this slop - Read EVERY file in the File List - verify implementation against story requirements - Tasks marked complete but not done = CRITICAL finding - Acceptance Criteria not implemented = HIGH severity finding - Do not review files that are not part of the application's source code. Always exclude the _bmad/ and _bmad-output/ folders from the review. Always exclude IDE and CLI configuration folders like .cursor/ and .windsurf/ and .claude/ - - - - Use provided {{story_path}} or ask user which story file to review - Read COMPLETE story file - Set {{story_key}} = extracted key from filename (e.g., "1-2-user-authentication.md" → "1-2-user-authentication") or story - metadata - Parse sections: Story, Acceptance Criteria, Tasks/Subtasks, Dev Agent Record → File List, Change Log - - - Check if git repository detected in current directory - - Run `git status --porcelain` to find uncommitted changes - Run `git diff --name-only` to see modified files - Run `git diff --cached --name-only` to see staged files - Compile list of actually changed files from git output - - - - Compare story's Dev Agent Record → File List with actual git changes - Note discrepancies: - - Files in git but not in story File List - - Files in story File List but no git changes - - Missing documentation of what was actually changed - - - - Load {project_context} for coding standards (if exists) - - - - Extract ALL Acceptance Criteria from story - Extract ALL Tasks/Subtasks with completion status ([x] vs [ ]) - From Dev Agent Record → File List, compile list of claimed changes - - Create review plan: - 1. **AC Validation**: Verify each AC is actually implemented - 2. **Task Audit**: Verify each [x] task is really done - 3. **Code Quality**: Security, performance, maintainability - 4. **Test Quality**: Real tests vs placeholder bullshit - - - - - VALIDATE EVERY CLAIM - Check git reality vs story claims - - - Review git vs story File List discrepancies: - 1. **Files changed but not in story File List** → MEDIUM finding (incomplete documentation) - 2. **Story lists files but no git changes** → HIGH finding (false claims) - 3. **Uncommitted changes not documented** → MEDIUM finding (transparency issue) - - - - Create comprehensive review file list from story File List and git changes - - - For EACH Acceptance Criterion: - 1. Read the AC requirement - 2. Search implementation files for evidence - 3. Determine: IMPLEMENTED, PARTIAL, or MISSING - 4. If MISSING/PARTIAL → HIGH SEVERITY finding - - - - For EACH task marked [x]: - 1. Read the task description - 2. Search files for evidence it was actually done - 3. **CRITICAL**: If marked [x] but NOT DONE → CRITICAL finding - 4. Record specific proof (file:line) - - - - For EACH file in comprehensive review list: - 1. **Security**: Look for injection risks, missing validation, auth issues - 2. **Performance**: N+1 queries, inefficient loops, missing caching - 3. **Error Handling**: Missing try/catch, poor error messages - 4. **Code Quality**: Complex functions, magic numbers, poor naming - 5. **Test Quality**: Are tests real assertions or placeholders? - - - - NOT LOOKING HARD ENOUGH - Find more problems! - Re-examine code for: - - Edge cases and null handling - - Architecture violations - - Documentation gaps - - Integration issues - - Dependency problems - - Git commit message quality (if applicable) - - Find at least 3 more specific, actionable issues - - - - - Categorize findings: HIGH (must fix), MEDIUM (should fix), LOW (nice to fix) - Set {{fixed_count}} = 0 - Set {{action_count}} = 0 - - **đŸ”Ĩ CODE REVIEW FINDINGS, {user_name}!** - - **Story:** {{story_file}} - **Git vs Story Discrepancies:** {{git_discrepancy_count}} found - **Issues Found:** {{high_count}} High, {{medium_count}} Medium, {{low_count}} Low - - ## 🔴 CRITICAL ISSUES - - Tasks marked [x] but not actually implemented - - Acceptance Criteria not implemented - - Story claims files changed but no git evidence - - Security vulnerabilities - - ## 🟡 MEDIUM ISSUES - - Files changed but not documented in story File List - - Uncommitted changes not tracked - - Performance problems - - Poor test coverage/quality - - Code maintainability issues - - ## đŸŸĸ LOW ISSUES - - Code style improvements - - Documentation gaps - - Git commit message quality - - - What should I do with these issues? - - 1. **Fix them automatically** - I'll update the code and tests - 2. **Create action items** - Add to story Tasks/Subtasks for later - 3. **Show me details** - Deep dive into specific issues - - Choose [1], [2], or specify which issue to examine: - - - Fix all HIGH and MEDIUM issues in the code - Add/update tests as needed - Update File List in story if files changed - Update story Dev Agent Record with fixes applied - Set {{fixed_count}} = number of HIGH and MEDIUM issues fixed - Set {{action_count}} = 0 - - - - Add "Review Follow-ups (AI)" subsection to Tasks/Subtasks - For each issue: `- [ ] [AI-Review][Severity] Description [file:line]` - Set {{action_count}} = number of action items created - Set {{fixed_count}} = 0 - - - - Show detailed explanation with code examples - Return to fix decision - - - - - - - Set {{new_status}} = "done" - Update story Status field to "done" - - - Set {{new_status}} = "in-progress" - Update story Status field to "in-progress" - - Save story file - - - - Set {{current_sprint_status}} = "enabled" - - - Set {{current_sprint_status}} = "no-sprint-tracking" - - - - - Load the FULL file: {sprint_status} - Find development_status key matching {{story_key}} - - - Update development_status[{{story_key}}] = "done" - Save file, preserving ALL comments and structure - ✅ Sprint status synced: {{story_key}} → done - - - - Update development_status[{{story_key}}] = "in-progress" - Save file, preserving ALL comments and structure - 🔄 Sprint status synced: {{story_key}} → in-progress - - - - ⚠ī¸ Story file updated, but sprint-status sync failed: {{story_key}} not found in sprint-status.yaml - - - - - ℹī¸ Story status updated (no sprint tracking configured) - - - **✅ Review Complete!** - - **Story Status:** {{new_status}} - **Issues Fixed:** {{fixed_count}} - **Action Items Created:** {{action_count}} - - {{#if new_status == "done"}}Code review complete!{{else}}Address the action items and continue development.{{/if}} - - - - \ No newline at end of file diff --git a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-01-load-story.md b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-01-load-story.md new file mode 100644 index 00000000..16a0c4df --- /dev/null +++ b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-01-load-story.md @@ -0,0 +1,122 @@ +--- +name: 'step-01-load-story' +description: "Compare story's file list against git changes" +--- + +# Step 1: Load Story and Discover Changes + +--- + +## STATE VARIABLES (capture now, persist throughout) + +These variables MUST be set in this step and available to all subsequent steps: + +- `story_path` - Path to the story file being reviewed +- `story_key` - Story identifier (e.g., "1-2-user-authentication") +- `story_content` - Complete, unmodified file content from story_path (loaded in substep 2) +- `story_file_list` - Files claimed in story's Dev Agent Record → File List +- `git_changed_files` - Files actually changed according to git +- `git_discrepancies` - Mismatches between `story_file_list` and `git_changed_files` + +--- + +## EXECUTION SEQUENCE + +### 1. Identify Story + +Ask user: "Which story would you like to review?" + +**Try input as direct file path first:** +If input resolves to an existing file: + - Verify it's in {sprint_status} with status `review` or `done` + - If verified → set `story_path` to that file path + - If NOT verified → Warn user the file is not in {sprint_status} (or wrong status). Ask: "Continue anyway?" + - If yes → set `story_path` + - If no → return to user prompt (ask "Which story would you like to review?" again) + +**Search {sprint_status}** (if input is not a direct file): +Search for stories with status `review` or `done`. Match by priority: +1. Story number resembles input closely enough (e.g., "1-2" matches "1 2", "1.2", "one dash two", "one two"; "1-32" matches "one thirty two"). Do NOT match if numbers differ (e.g., "1-33" does not match "1-32") +2. Exact story name/key (e.g., "1-2-user-auth-api") +3. Story name/title resembles input closely enough +4. Story description resembles input closely enough + +**Resolution:** +- **Single match**: Confident. Set `story_path`, proceed to substep 2 +- **Multiple matches**: Uncertain. Present all candidates to user. Wait for selection. Set `story_path`, proceed to substep 2 +- **No match**: Ask user to clarify or provide the full story path. Return to user prompt (ask "Which story would you like to review?" again) + +### 2. Load Story File + +**Load file content:** +Read the complete contents of {story_path} and assign to `story_content` WITHOUT filtering, truncating or summarizing. If {story_path} cannot be read, is empty, or obviously doesn't have the story: report the error to the user and HALT the workflow. + +**Extract story identifier:** +Verify the filename ends with `.md` extension. Remove `.md` to get `story_key` (e.g., "1-2-user-authentication.md" → "1-2-user-authentication"). If filename doesn't end with `.md` or the result is empty: report the error to the user and HALT the workflow. + +### 3. Extract File List from Story + +Extract `story_file_list` from the Dev Agent Record → File List section of {story_content}. + +**If Dev Agent Record or File List section not found:** Report to user and set `story_file_list` = NO_FILE_LIST. + +### 4. Discover Git Changes + +Check if git repository exists. + +**If NOT a git repo:** Set `git_changed_files` = NO_GIT, `git_discrepancies` = NO_GIT. Skip to substep 5. + +**If git repo detected:** + +```bash +git status --porcelain +git diff -M --name-only +git diff -M --cached --name-only +``` + +If any git command fails: Report the error to the user and HALT the workflow. + +Compile `git_changed_files` = union of modified, staged, new, deleted, and renamed files. + +### 5. Cross-Reference Story vs Git + +**If {git_changed_files} is empty:** + +Ask user: "No git changes detected. Continue anyway?" + +- If **no**: HALT the workflow +- If **yes**: Continue to comparison + +**Compare {story_file_list} with {git_changed_files}:** + +Exclude git-ignored files from the comparison (run `git check-ignore` if needed). + +Set `git_discrepancies` with categories: + +- **files_in_git_not_story**: Files changed in git but not in story File List +- **files_in_story_not_git**: Files in story File List but no git changes (excluding git-ignored) +- **uncommitted_undocumented**: Uncommitted changes not tracked in story + +--- + +## COMPLETION CHECKLIST + +Before proceeding to the next step, verify ALL of the following: + +- `story_path` identified and loaded +- `story_key` extracted +- `story_content` captured completely and unmodified +- `story_file_list` compiled from Dev Agent Record (or NO_FILE_LIST if not found) +- `git_changed_files` discovered via git commands (or NO_GIT if not a git repo) +- `git_discrepancies` calculated + +**If any criterion is not met:** Report to the user and HALT the workflow. + +--- + +## NEXT STEP DIRECTIVE + +**CRITICAL:** When this step completes, explicitly state: + +"**NEXT:** Loading `step-02-build-attack-plan.md`" + diff --git a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-02-adversarial-review.md b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-02-adversarial-review.md new file mode 100644 index 00000000..b6b24290 --- /dev/null +++ b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-02-adversarial-review.md @@ -0,0 +1,155 @@ +--- +name: 'step-02-adversarial-review' +description: 'Lean adversarial review - context-independent diff analysis, no story knowledge' +--- + +# Step 2: Adversarial Review (Information Asymmetric) + +**Goal:** Perform context-independent adversarial review of code changes. Reviewer sees ONLY the diff - no story, no ACs, no context about WHY changes were made. + +Reviewer has FULL repo access but NO knowledge of WHY changes were made +DO NOT include story file in prompt - asymmetry is about intent, not visibility +This catches issues a fresh reviewer would find that story-biased review might miss + +--- + +## AVAILABLE STATE + +From previous steps: + +- `{story_path}`, `{story_key}` +- `{file_list}` - Files listed in story's File List section +- `{git_changed_files}` - Files changed according to git +- `{baseline_commit}` - From story file Dev Agent Record + +--- + +## STATE VARIABLE (capture now) + +- `{diff_output}` - Complete diff of changes +- `{asymmetric_findings}` - Findings from adversarial review + +--- + +## EXECUTION SEQUENCE + +### 1. Construct Diff + +Build complete diff of all changes for this story. + +**Step 1a: Read baseline from story file** + +Extract `Baseline Commit` from the story file's Dev Agent Record section. + +- If found and not "NO_GIT": use as `{baseline_commit}` +- If "NO_GIT" or missing: proceed to fallback + +**Step 1b: Construct diff (with baseline)** + +If `{baseline_commit}` is a valid commit hash: + +```bash +git diff {baseline_commit} -- ':!{implementation_artifacts}' +``` + +This captures all changes (committed + uncommitted) since dev-story started. + +**Step 1c: Fallback (no baseline)** + +If no baseline available, review current state of files in `{file_list}`: + +- Read each file listed in the story's File List section +- Review as full file content (not a diff) + +**Include in `{diff_output}`:** + +- All modified tracked files (except files in `{implementation_artifacts}` - asymmetry requires hiding intent) +- All new files created for this story +- Full content for new files + +**Note:** Do NOT `git add` anything - this is read-only inspection. + +### 2. Invoke Adversarial Review + +With `{diff_output}` constructed, invoke the review task. If possible, use information asymmetry: run this step, and only it, in a separate subagent or process with read access to the project, but no context except the `{diff_output}`. + +```xml +Review {diff_output} using {project-root}/_bmad/core/tasks/review-adversarial-general.xml +``` + +**Platform fallback:** If task invocation not available, load the task file and execute its instructions inline, passing `{diff_output}` as the content. + +The task should: review `{diff_output}` and return a list of findings. + +### 3. Process Adversarial Findings + +Capture findings from adversarial review. + +**If zero findings:** HALT - this is suspicious. Re-analyze or ask for guidance. + +Evaluate severity (Critical, High, Medium, Low) and validity (Real, Noise, Undecided). + +Add each finding to `{asymmetric_findings}` (no IDs yet - assigned after merge): + +``` +{ + source: "adversarial", + severity: "...", + validity: "...", + description: "...", + location: "file:line (if applicable)" +} +``` + +### 4. Phase 1 Summary + +Present adversarial findings: + +``` +**Phase 1: Adversarial Review Complete** + +**Reviewer Context:** Pure diff review (no story knowledge) +**Findings:** {count} +- Critical: {count} +- High: {count} +- Medium: {count} +- Low: {count} + +**Validity Assessment:** +- Real: {count} +- Noise: {count} +- Undecided: {count} + +Proceeding to attack plan construction... +``` + +--- + +## NEXT STEP DIRECTIVE + +**CRITICAL:** When this step completes, explicitly state: + +"**NEXT:** Loading `step-03-build-attack-plan.md`" + +--- + +## SUCCESS METRICS + +- Diff constructed from correct source (uncommitted or commits) +- Story file excluded from diff +- Task invoked with diff as input +- Adversarial review executed +- Findings captured with severity and validity +- `{asymmetric_findings}` populated +- Phase summary presented +- Explicit NEXT directive provided + +## FAILURE MODES + +- Including story file in diff (breaks asymmetry) +- Skipping adversarial review entirely +- Accepting zero findings without halt +- Invoking task without providing diff input +- Missing severity/validity classification +- Not storing findings for consolidation +- No explicit NEXT directive at step completion diff --git a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-03-build-attack-plan.md b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-03-build-attack-plan.md new file mode 100644 index 00000000..a14a8010 --- /dev/null +++ b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-03-build-attack-plan.md @@ -0,0 +1,147 @@ +--- +name: 'step-03-build-attack-plan' +description: 'Extract ACs and tasks, create comprehensive review plan for context-aware phase' +--- + +# Step 3: Build Review Attack Plan + +**Goal:** Extract all reviewable items from story and create attack plan for context-aware review phase. + +--- + +## AVAILABLE STATE + +From previous steps: + +- `{story_path}` - Path to the story file +- `{story_key}` - Story identifier +- `{story_file_list}` - Files claimed in story +- `{git_changed_files}` - Files actually changed (git) +- `{git_discrepancies}` - Differences between claims and reality +- `{asymmetric_findings}` - Findings from Phase 1 (adversarial review) + +--- + +## STATE VARIABLES (capture now) + +- `{acceptance_criteria}` - All ACs extracted from story +- `{tasks_with_status}` - All tasks with their [x] or [ ] status +- `{comprehensive_file_list}` - Union of story files + git files +- `{review_attack_plan}` - Structured plan for context-aware phase + +--- + +## EXECUTION SEQUENCE + +### 1. Extract Acceptance Criteria + +Parse all Acceptance Criteria from story: + +``` +{acceptance_criteria} = [ + { id: "AC1", requirement: "...", testable: true/false }, + { id: "AC2", requirement: "...", testable: true/false }, + ... +] +``` + +Note any ACs that are vague or untestable. + +### 2. Extract Tasks with Status + +Parse all Tasks/Subtasks with completion markers: + +``` +{tasks_with_status} = [ + { id: "T1", description: "...", status: "complete" ([x]) or "incomplete" ([ ]) }, + { id: "T1.1", description: "...", status: "complete" or "incomplete" }, + ... +] +``` + +Flag any tasks marked complete [x] for verification. + +### 3. Build Comprehensive File List + +Merge `{story_file_list}` and `{git_changed_files}`: + +``` +{comprehensive_file_list} = union of: + - Files in story Dev Agent Record + - Files changed according to git + - Deduped and sorted +``` + +Exclude from review: + +- `_bmad/`, `_bmad-output/` +- `.cursor/`, `.windsurf/`, `.claude/` +- IDE/editor config files + +### 4. Create Review Attack Plan + +Structure the `{review_attack_plan}`: + +``` +PHASE 1: Adversarial Review (Step 2) [COMPLETE - {asymmetric_findings} findings] +├── Fresh code review without story context +│ └── {asymmetric_findings} items to consolidate + +PHASE 2: Context-Aware Review (Step 4) +├── Git vs Story Discrepancies +│ └── {git_discrepancies} items +├── AC Validation +│ └── {acceptance_criteria} items to verify +├── Task Completion Audit +│ └── {tasks_with_status} marked [x] to verify +└── Code Quality Review + └── {comprehensive_file_list} files to review +``` + +### 5. Preview Attack Plan + +Present to user (brief summary): + +``` +**Review Attack Plan** + +**Story:** {story_key} + +**Phase 1 (Adversarial - Complete):** {asymmetric_findings count} findings from fresh review +**Phase 2 (Context-Aware - Starting):** + - ACs to verify: {count} + - Tasks marked complete: {count} + - Files to review: {count} + - Git discrepancies detected: {count} + +Proceeding with context-aware review... +``` + +--- + +## NEXT STEP DIRECTIVE + +**CRITICAL:** When this step completes, explicitly state: + +"**NEXT:** Loading `step-04-context-aware-review.md`" + +--- + +## SUCCESS METRICS + +- All ACs extracted with testability assessment +- All tasks extracted with completion status +- Comprehensive file list built (story + git) +- Exclusions applied correctly +- Attack plan structured for context-aware phase +- Summary presented to user +- Explicit NEXT directive provided + +## FAILURE MODES + +- Missing AC extraction +- Not capturing task completion status +- Forgetting to merge story + git files +- Not excluding IDE/config directories +- Skipping attack plan structure +- No explicit NEXT directive at step completion diff --git a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-04-context-aware-review.md b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-04-context-aware-review.md new file mode 100644 index 00000000..af0702b6 --- /dev/null +++ b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-04-context-aware-review.md @@ -0,0 +1,182 @@ +--- +name: 'step-04-context-aware-review' +description: 'Story-aware validation: verify ACs, audit task completion, check git discrepancies' +--- + +# Step 4: Context-Aware Review + +**Goal:** Perform story-aware validation - verify AC implementation, audit task completion, review code quality with full story context. + +VALIDATE EVERY CLAIM - Check git reality vs story claims +You KNOW the story requirements - use that knowledge to find gaps + +--- + +## AVAILABLE STATE + +From previous steps: + +- `{story_path}`, `{story_key}` +- `{story_file_list}`, `{git_changed_files}`, `{git_discrepancies}` +- `{acceptance_criteria}`, `{tasks_with_status}` +- `{comprehensive_file_list}`, `{review_attack_plan}` +- `{asymmetric_findings}` - From Phase 1 (adversarial review) + +--- + +## STATE VARIABLE (capture now) + +- `{context_aware_findings}` - All findings from this phase + +Initialize `{context_aware_findings}` as empty list. + +--- + +## EXECUTION SEQUENCE + +### 0. Load Planning Context (JIT) + +Load planning documents for AC validation against system design: + +- **Architecture**: `{planning_artifacts}/*architecture*.md` (or sharded: `{planning_artifacts}/*architecture*/*.md`) +- **UX Design**: `{planning_artifacts}/*ux*.md` (if UI review relevant) +- **Epic**: `{planning_artifacts}/*epic*/epic-{epic_num}.md` (the epic containing this story) + +These provide the design context needed to validate AC implementation against system requirements. + +### 1. Git vs Story Discrepancies + +Review `{git_discrepancies}` and create findings: + +| Discrepancy Type | Severity | +| --- | --- | +| Files changed but not in story File List | Medium | +| Story lists files but no git changes | High | +| Uncommitted changes not documented | Medium | + +For each discrepancy, add to `{context_aware_findings}` (no IDs yet - assigned after merge): + +``` +{ + source: "git-discrepancy", + severity: "...", + description: "...", + evidence: "file: X, git says: Y, story says: Z" +} +``` + +### 2. Acceptance Criteria Validation + +For EACH AC in `{acceptance_criteria}`: + +1. Read the AC requirement +2. Search implementation files in `{comprehensive_file_list}` for evidence +3. Determine status: IMPLEMENTED, PARTIAL, or MISSING +4. If PARTIAL or MISSING → add High severity finding + +Add to `{context_aware_findings}`: + +``` +{ + source: "ac-validation", + severity: "High", + description: "AC {id} not fully implemented: {details}", + evidence: "Expected: {ac}, Found: {what_was_found}" +} +``` + +### 3. Task Completion Audit + +For EACH task marked [x] in `{tasks_with_status}`: + +1. Read the task description +2. Search files for evidence it was actually done +3. **Critical**: If marked [x] but NOT DONE → Critical finding +4. Record specific proof (file:line) if done + +Add to `{context_aware_findings}` if false: + +``` +{ + source: "task-audit", + severity: "Critical", + description: "Task marked complete but not implemented: {task}", + evidence: "Searched: {files}, Found: no evidence of {expected}" +} +``` + +### 4. Code Quality Review (Context-Aware) + +For EACH file in `{comprehensive_file_list}`: + +Review with STORY CONTEXT (you know what was supposed to be built): + +- **Security**: Missing validation for AC-specified inputs? +- **Performance**: Story mentioned scale requirements met? +- **Error Handling**: Edge cases from AC covered? +- **Test Quality**: Tests actually verify ACs or just placeholders? +- **Architecture Compliance**: Follows patterns in architecture doc? + +Add findings to `{context_aware_findings}` with appropriate severity. + +### 5. Minimum Finding Check + +If total findings < 3, NOT LOOKING HARD ENOUGH + +Re-examine for: + +- Edge cases not covered by implementation +- Documentation gaps +- Integration issues with other components +- Dependency problems +- Comments missing for complex logic + +--- + +## PHASE 2 SUMMARY + +Present context-aware findings: + +``` +**Phase 2: Context-Aware Review Complete** + +**Findings:** {count} +- Critical: {count} +- High: {count} +- Medium: {count} +- Low: {count} + +Proceeding to findings consolidation... +``` + +Store `{context_aware_findings}` for consolidation in step 5. + +--- + +## NEXT STEP DIRECTIVE + +**CRITICAL:** When this step completes, explicitly state: + +"**NEXT:** Loading `step-05-consolidate-findings.md`" + +--- + +## SUCCESS METRICS + +- All git discrepancies reviewed and findings created +- Every AC checked for implementation evidence +- Every [x] task verified with proof +- Code quality reviewed with story context +- Minimum 3 findings (push harder if not) +- `{context_aware_findings}` populated +- Phase summary presented +- Explicit NEXT directive provided + +## FAILURE MODES + +- Accepting "looks good" with < 3 findings +- Not verifying [x] tasks with actual evidence +- Missing AC validation +- Ignoring git discrepancies +- Not storing findings for consolidation +- No explicit NEXT directive at step completion diff --git a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-05-consolidate-findings.md b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-05-consolidate-findings.md new file mode 100644 index 00000000..6370f441 --- /dev/null +++ b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-05-consolidate-findings.md @@ -0,0 +1,158 @@ +--- +name: 'step-05-consolidate-findings' +description: 'Merge and deduplicate findings from both review phases' +--- + +# Step 5: Consolidate Findings + +**Goal:** Merge findings from adversarial review (Phase 1) and context-aware review (Phase 2), deduplicate, and present unified findings table. + +--- + +## AVAILABLE STATE + +From previous steps: + +- `{story_path}`, `{story_key}` +- `{asymmetric_findings}` - Findings from Phase 1 (step 2 - adversarial review) +- `{context_aware_findings}` - Findings from Phase 2 (step 4 - context-aware review) + +--- + +## STATE VARIABLE (capture now) + +- `{consolidated_findings}` - Merged, deduplicated findings + +--- + +## EXECUTION SEQUENCE + +### 1. Merge All Findings + +Combine both finding lists: + +``` +all_findings = {context_aware_findings} + {asymmetric_findings} +``` + +### 2. Deduplicate Findings + +Identify duplicates (same underlying issue found by both phases): + +**Duplicate Detection Criteria:** + +- Same file + same line range +- Same issue type (e.g., both about error handling in same function) +- Overlapping descriptions + +**Resolution Rule:** + +Keep the MORE DETAILED version: + +- If context-aware finding has AC reference → keep that +- If adversarial finding has better technical detail → keep that +- When in doubt, keep context-aware (has more context) + +Note which findings were merged (for transparency in the summary). + +### 3. Normalize Severity + +Apply consistent severity scale (Critical, High, Medium, Low). + +### 4. Filter Noise + +Review adversarial findings marked as Noise: + +- If clearly false positive (e.g., style preference, not actual issue) → exclude +- If questionable → keep with Undecided validity +- If context reveals it's actually valid → upgrade to Real + +**Do NOT filter:** + +- Any Critical or High severity +- Any context-aware findings (they have story context) + +### 5. Sort and Number Findings + +Sort by severity (Critical → High → Medium → Low), then assign IDs: F1, F2, F3, etc. + +Build `{consolidated_findings}`: + +```markdown +| ID | Severity | Source | Description | Location | +|----|----------|--------|-------------|----------| +| F1 | Critical | task-audit | Task 3 marked [x] but not implemented | src/auth.ts | +| F2 | High | ac-validation | AC2 partially implemented | src/api/*.ts | +| F3 | High | adversarial | Missing error handling in API calls | src/api/client.ts:45 | +| F4 | Medium | git-discrepancy | File changed but not in story | src/utils.ts | +| F5 | Low | adversarial | Magic number should be constant | src/config.ts:12 | +``` + +### 6. Present Consolidated Findings + +```markdown +**Consolidated Code Review Findings** + +**Story:** {story_key} + +**Summary:** +- Total findings: {count} +- Critical: {count} +- High: {count} +- Medium: {count} +- Low: {count} + +**Deduplication:** {merged_count} duplicate findings merged + +--- + +## Findings by Severity + +### Critical (Must Fix) +{list critical findings with full details} + +### High (Should Fix) +{list high findings with full details} + +### Medium (Consider Fixing) +{list medium findings} + +### Low (Nice to Fix) +{list low findings} + +--- + +**Phase Sources:** +- Adversarial (Phase 1): {count} findings +- Context-Aware (Phase 2): {count} findings +``` + +--- + +## NEXT STEP DIRECTIVE + +**CRITICAL:** When this step completes, explicitly state: + +"**NEXT:** Loading `step-06-resolve-and-update.md`" + +--- + +## SUCCESS METRICS + +- All findings merged from both phases +- Duplicates identified and resolved (kept more detailed) +- Severity normalized consistently +- Noise filtered appropriately (but not excessively) +- Consolidated table created +- `{consolidated_findings}` populated +- Summary presented to user +- Explicit NEXT directive provided + +## FAILURE MODES + +- Missing findings from either phase +- Not detecting duplicates (double-counting issues) +- Inconsistent severity assignment +- Filtering real issues as noise +- Not storing consolidated findings +- No explicit NEXT directive at step completion diff --git a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-06-resolve-and-update.md b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-06-resolve-and-update.md new file mode 100644 index 00000000..700032b7 --- /dev/null +++ b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-06-resolve-and-update.md @@ -0,0 +1,213 @@ +--- +name: 'step-06-resolve-and-update' +description: 'Present findings, fix or create action items, update story and sprint status' +--- + +# Step 6: Resolve Findings and Update Status + +**Goal:** Present findings to user, handle resolution (fix or action items), update story file and sprint status. + +--- + +## AVAILABLE STATE + +From previous steps: + +- `{story_path}`, `{story_key}` +- `{consolidated_findings}` - Merged findings from step 5 +- `sprint_status` = `{implementation_artifacts}/sprint-status.yaml` + +--- + +## STATE VARIABLES (capture now) + +- `{fixed_count}` - Number of issues fixed +- `{action_count}` - Number of action items created +- `{new_status}` - Final story status + +--- + +## EXECUTION SEQUENCE + +### 1. Present Resolution Options + +```markdown +**Code Review Findings for {user_name}** + +**Story:** {story_key} +**Total Issues:** {consolidated_findings.count} + +{consolidated_findings_table} + +--- + +**What should I do with these issues?** + +**[1] Fix them automatically** - I'll update the code and tests +**[2] Create action items** - Add to story Tasks/Subtasks for later +**[3] Walk through** - Discuss each finding individually +**[4] Show details** - Deep dive into specific issues + +Choose [1], [2], [3], [4], or specify which issue (e.g., "CF-3"): +``` + +### 2. Handle User Choice + +**Option [1]: Fix Automatically** + +1. For each CRITICAL and HIGH finding: + - Apply the fix in the code + - Add/update tests if needed + - Record what was fixed +2. Update story Dev Agent Record → File List if files changed +3. Add "Code Review Fixes Applied" entry to Change Log +4. Set `{fixed_count}` = number of issues fixed +5. Set `{action_count}` = 0 (LOW findings can become action items) + +**Option [2]: Create Action Items** + +1. Add "Review Follow-ups (AI)" subsection to Tasks/Subtasks +2. For each finding: + ``` + - [ ] [AI-Review][{severity}] {description} [{location}] + ``` +3. Set `{action_count}` = number of action items created +4. Set `{fixed_count}` = 0 + +**Option [3]: Walk Through** + +For each finding in order: + +1. Present finding with full context and code snippet +2. Ask: **[f]ix now / [s]kip / [d]iscuss more** +3. If fix: Apply fix immediately, increment `{fixed_count}` +4. If skip: Note as acknowledged, optionally create action item +5. If discuss: Provide more detail, repeat choice +6. Continue to next finding + +After all processed, summarize what was fixed/skipped. + +**Option [4]: Show Details** + +1. Present expanded details for specific finding(s) +2. Return to resolution choice + +### 3. Determine Final Status + +Evaluate completion: + +**If ALL conditions met:** + +- All CRITICAL issues fixed +- All HIGH issues fixed or have action items +- All ACs verified as implemented + +Set `{new_status}` = "done" + +**Otherwise:** + +Set `{new_status}` = "in-progress" + +### 4. Update Story File + +1. Update story Status field to `{new_status}` +2. Add review notes to Dev Agent Record: + +```markdown +## Senior Developer Review (AI) + +**Date:** {date} +**Reviewer:** AI Code Review + +**Findings Summary:** +- CRITICAL: {count} ({fixed}/{action_items}) +- HIGH: {count} ({fixed}/{action_items}) +- MEDIUM: {count} +- LOW: {count} + +**Resolution:** {approach_taken} + +**Files Modified:** {list if fixes applied} +``` + +3. Update Change Log: + +```markdown +- [{date}] Code review completed - {outcome_summary} +``` + +4. Save story file + +### 5. Sync Sprint Status + +Check if `{sprint_status}` file exists: + +**If exists:** + +1. Load `{sprint_status}` +2. Find `{story_key}` in development_status +3. Update status to `{new_status}` +4. Save file, preserving ALL comments and structure + +``` + Sprint status synced: {story_key} {new_status} +``` + +**If not exists or key not found:** + +``` + Sprint status sync skipped (no sprint tracking or key not found) +``` + +### 6. Completion Output + +```markdown +** Code Review Complete!** + +**Story:** {story_key} +**Final Status:** {new_status} +**Issues Fixed:** {fixed_count} +**Action Items Created:** {action_count} + +{if new_status == "done"} +Code review passed! Story is ready for final verification. +{else} +Address the action items and run another review cycle. +{endif} + +--- + +**Next Steps:** +- Commit changes (if fixes applied) +- Run tests to verify fixes +- Address remaining action items (if any) +- Mark story complete when all items resolved +``` + +--- + +## WORKFLOW COMPLETE + +This is the final step. The Code Review workflow is now complete. + +--- + +## SUCCESS METRICS + +- Resolution options presented clearly +- User choice handled correctly +- Fixes applied cleanly (if chosen) +- Action items created correctly (if chosen) +- Story status determined correctly +- Story file updated with review notes +- Sprint status synced (if applicable) +- Completion summary provided + +## FAILURE MODES + +- Not presenting resolution options +- Fixing without user consent +- Not updating story file +- Wrong status determination (done when issues remain) +- Not syncing sprint status when it exists +- Missing completion summary diff --git a/src/modules/bmm/workflows/4-implementation/code-review/workflow.md b/src/modules/bmm/workflows/4-implementation/code-review/workflow.md new file mode 100644 index 00000000..1d46e3fb --- /dev/null +++ b/src/modules/bmm/workflows/4-implementation/code-review/workflow.md @@ -0,0 +1,39 @@ +--- +name: code-review +description: 'Code review for dev-story output. Audits acceptance criteria against implementation, performs adversarial diff review, can auto-fix with approval. A different LLM than the implementer is recommended.' +web_bundle: false +--- + +# Code Review Workflow + +## WORKFLOW ARCHITECTURE: STEP FILES + +- This file (workflow.md) stays in context throughout +- Each step file is read just before processing (current step stays at end of context) +- State persists via variables: `{story_path}`, `{story_key}`, `{context_aware_findings}`, `{asymmetric_findings}` + +--- + +## INITIALIZATION + +### Configuration Loading + +Load config from `{project-root}/_bmad/bmm/config.yaml` and resolve: + +- `user_name`, `communication_language`, `user_skill_level`, `document_output_language` +- `planning_artifacts`, `implementation_artifacts` +- `date` as system-generated current datetime + +- ✅ YOU MUST ALWAYS SPEAK OUTPUT In your Agent communication style with the config `{communication_language}` + +### Paths + +- `installed_path` = `{project-root}/_bmad/bmm/workflows/4-implementation/code-review` +- `project_context` = `**/project-context.md` (load if exists) +- `sprint_status` = `{implementation_artifacts}/sprint-status.yaml` + +--- + +## EXECUTION + +Read and follow `steps/step-01-load-story.md` to begin the workflow. diff --git a/src/modules/bmm/workflows/4-implementation/code-review/workflow.yaml b/src/modules/bmm/workflows/4-implementation/code-review/workflow.yaml deleted file mode 100644 index 9e66b932..00000000 --- a/src/modules/bmm/workflows/4-implementation/code-review/workflow.yaml +++ /dev/null @@ -1,51 +0,0 @@ -# Review Story Workflow -name: code-review -description: "Perform an ADVERSARIAL Senior Developer code review that finds 3-10 specific problems in every story. Challenges everything: code quality, test coverage, architecture compliance, security, performance. NEVER accepts `looks good` - must find minimum issues and can auto-fix with user approval." -author: "BMad" - -# Critical variables from config -config_source: "{project-root}/_bmad/bmm/config.yaml" -user_name: "{config_source}:user_name" -communication_language: "{config_source}:communication_language" -user_skill_level: "{config_source}:user_skill_level" -document_output_language: "{config_source}:document_output_language" -date: system-generated -planning_artifacts: "{config_source}:planning_artifacts" -implementation_artifacts: "{config_source}:implementation_artifacts" -output_folder: "{implementation_artifacts}" -sprint_status: "{implementation_artifacts}/sprint-status.yaml" - -# Workflow components -installed_path: "{project-root}/_bmad/bmm/workflows/4-implementation/code-review" -instructions: "{installed_path}/instructions.xml" -validation: "{installed_path}/checklist.md" -template: false - -variables: - # Project context - project_context: "**/project-context.md" - story_dir: "{implementation_artifacts}" - -# Smart input file references - handles both whole docs and sharded docs -# Priority: Whole document first, then sharded version -# Strategy: SELECTIVE LOAD - only load the specific epic needed for this story review -input_file_patterns: - architecture: - description: "System architecture for review context" - whole: "{planning_artifacts}/*architecture*.md" - sharded: "{planning_artifacts}/*architecture*/*.md" - load_strategy: "FULL_LOAD" - ux_design: - description: "UX design specification (if UI review)" - whole: "{planning_artifacts}/*ux*.md" - sharded: "{planning_artifacts}/*ux*/*.md" - load_strategy: "FULL_LOAD" - epics: - description: "Epic containing story being reviewed" - whole: "{planning_artifacts}/*epic*.md" - sharded_index: "{planning_artifacts}/*epic*/index.md" - sharded_single: "{planning_artifacts}/*epic*/epic-{{epic_num}}.md" - load_strategy: "SELECTIVE_LOAD" - -standalone: true -web_bundle: false diff --git a/src/modules/bmm/workflows/4-implementation/dev-story/instructions.xml b/src/modules/bmm/workflows/4-implementation/dev-story/instructions.xml index 4fb70efe..3e49b378 100644 --- a/src/modules/bmm/workflows/4-implementation/dev-story/instructions.xml +++ b/src/modules/bmm/workflows/4-implementation/dev-story/instructions.xml @@ -219,6 +219,17 @@ ℹī¸ No sprint status file exists - story progress will be tracked in story file only Set {{current_sprint_status}} = "no-sprint-tracking" + + + + Capture current HEAD commit: `git rev-parse HEAD` + Store as {{baseline_commit}} + Write to story file Dev Agent Record: "**Baseline Commit:** {{baseline_commit}}" + + + Set {{baseline_commit}} = "NO_GIT" + Write to story file Dev Agent Record: "**Baseline Commit:** NO_GIT" + diff --git a/src/modules/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md b/src/modules/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md index db03665f..23d1acc8 100644 --- a/src/modules/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md +++ b/src/modules/bmm/workflows/bmad-quick-flow/quick-dev/steps/step-05-adversarial-review.md @@ -51,7 +51,11 @@ Use best-effort diff construction: ### Capture as {diff_output} -Merge all changes into `{diff_output}`. +**Include in `{diff_output}`:** + +- All modified tracked files (except `{tech_spec_path}` if tech-spec mode - asymmetry requires hiding intent) +- All new files created during this workflow +- Full content for new files **Note:** Do NOT `git add` anything - this is read-only inspection. @@ -75,7 +79,7 @@ The task should: review `{diff_output}` and return a list of findings. Capture the findings from the task output. **If zero findings:** HALT - this is suspicious. Re-analyze or request user guidance. -Evaluate severity (Critical, High, Medium, Low) and validity (real, noise, undecided). +Evaluate severity (Critical, High, Medium, Low) and validity (Real, Noise, Undecided). DO NOT exclude findings based on severity or validity unless explicitly asked to do so. Order findings by severity. Number the ordered findings (F1, F2, F3, etc.). @@ -92,6 +96,7 @@ With findings in hand, load `step-06-resolve-findings.md` for user to choose res ## SUCCESS METRICS - Diff constructed from baseline_commit +- Tech-spec excluded from diff when in tech-spec mode (information asymmetry) - New files included in diff - Task invoked with diff as input - Findings received @@ -100,6 +105,7 @@ With findings in hand, load `step-06-resolve-findings.md` for user to choose res ## FAILURE MODES - Missing baseline_commit (can't construct accurate diff) +- Including tech_spec_path in diff when in tech-spec mode (breaks asymmetry) - Not including new untracked files in diff - Invoking task without providing diff input - Accepting zero findings without questioning