From 5fcdae02b5790c4124e1a166c5f7258a1a5b4a74 Mon Sep 17 00:00:00 2001 From: Alex Verkhovsky Date: Sun, 4 Jan 2026 05:33:21 -0800 Subject: [PATCH] refactor(code-review): defer finding IDs until consolidation --- .../steps/step-03-context-aware-review.md | 27 ++++----- .../steps/step-04-adversarial-review.md | 19 +++--- .../steps/step-05-consolidate-findings.md | 58 +++++++------------ 3 files changed, 43 insertions(+), 61 deletions(-) diff --git a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-03-context-aware-review.md b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-03-context-aware-review.md index a36589e1..2d7a3c7e 100644 --- a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-03-context-aware-review.md +++ b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-03-context-aware-review.md @@ -43,15 +43,14 @@ 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 | +| 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}`: +For each discrepancy, add to `{context_aware_findings}` (no IDs yet - assigned after merge): ``` { - id: "CAF-{n}", source: "git-discrepancy", severity: "...", description: "...", @@ -66,15 +65,14 @@ 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 +4. If PARTIAL or MISSING → add High severity finding Add to `{context_aware_findings}`: ``` { - id: "CAF-{n}", source: "ac-validation", - severity: "HIGH", + severity: "High", description: "AC {id} not fully implemented: {details}", evidence: "Expected: {ac}, Found: {what_was_found}" } @@ -86,16 +84,15 @@ 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 +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: ``` { - id: "CAF-{n}", source: "task-audit", - severity: "CRITICAL", + severity: "Critical", description: "Task marked complete but not implemented: {task}", evidence: "Searched: {files}, Found: no evidence of {expected}" } @@ -137,10 +134,10 @@ Present context-aware findings: **Phase 1: Context-Aware Review Complete** **Findings:** {count} -- CRITICAL: {count} -- HIGH: {count} -- MEDIUM: {count} -- LOW: {count} +- Critical: {count} +- High: {count} +- Medium: {count} +- Low: {count} Proceeding to Phase 2: Adversarial Review... ``` diff --git a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-04-adversarial-review.md b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-04-adversarial-review.md index 1a2485ec..f77b2221 100644 --- a/src/modules/bmm/workflows/4-implementation/code-review/steps/step-04-adversarial-review.md +++ b/src/modules/bmm/workflows/4-implementation/code-review/steps/step-04-adversarial-review.md @@ -93,12 +93,11 @@ Capture findings from adversarial review. Evaluate severity (Critical, High, Medium, Low) and validity (Real, Noise, Undecided). -Create `{asymmetric_findings}` list: +Add each finding to `{asymmetric_findings}` (no IDs yet - assigned after merge): ``` { - id: "AAF-{n}", - source: "adversarial-review", + source: "adversarial", severity: "...", validity: "...", description: "...", @@ -115,15 +114,15 @@ Present adversarial findings: **Reviewer Context:** Pure diff review (no story knowledge) **Findings:** {count} -- CRITICAL: {count} -- HIGH: {count} -- MEDIUM: {count} -- LOW: {count} +- Critical: {count} +- High: {count} +- Medium: {count} +- Low: {count} **Validity Assessment:** -- Real issues: {count} -- Noise/false positives: {count} -- Needs judgment: {count} +- Real: {count} +- Noise: {count} +- Undecided: {count} Proceeding to findings consolidation... ``` 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 index 0ae3f418..69953215 100644 --- 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 @@ -57,53 +57,39 @@ Keep the MORE DETAILED version: - If adversarial finding has better technical detail → keep that - When in doubt, keep context-aware (has more context) -Mark duplicates as merged: - -``` -{ - id: "CF-{n}", - merged_from: ["CAF-3", "AAF-2"], - kept_version: "CAF-3", - reason: "Context-aware version includes AC reference" -} -``` +Note which findings were merged (for transparency in the summary). ### 3. Normalize Severity -Apply consistent severity scale: - -| Severity | Icon | Criteria | -| --- | --- | --- | -| CRITICAL | RED | Security vuln, data loss, tasks marked done but not, broken core functionality | -| HIGH | ORANGE | Missing AC implementation, logic errors, missing critical error handling | -| MEDIUM | YELLOW | Performance issues, incomplete features, documentation gaps | -| LOW | GREEN | Code style, minor improvements, suggestions | +Apply consistent severity scale (Critical, High, Medium, Low). ### 4. Filter Noise -Review adversarial findings marked as 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 +- 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 Critical or High severity - Any context-aware findings (they have story context) -### 5. Create Consolidated Table +### 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 | |----|----------|--------|-------------|----------| -| CF-1 | CRITICAL | task-audit | Task 3 marked [x] but not implemented | src/auth.ts | -| CF-2 | HIGH | ac-validation | AC2 partially implemented | src/api/*.ts | -| CF-3 | HIGH | adversarial | Missing error handling in API calls | src/api/client.ts:45 | -| CF-4 | MEDIUM | git-discrepancy | File changed but not in story | src/utils.ts | -| CF-5 | LOW | adversarial | Magic number should be constant | src/config.ts:12 | +| 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 @@ -115,10 +101,10 @@ Build `{consolidated_findings}`: **Summary:** - Total findings: {count} -- CRITICAL: {count} -- HIGH: {count} -- MEDIUM: {count} -- LOW: {count} +- Critical: {count} +- High: {count} +- Medium: {count} +- Low: {count} **Deduplication:** {merged_count} duplicate findings merged @@ -126,16 +112,16 @@ Build `{consolidated_findings}`: ## Findings by Severity -### CRITICAL (Must Fix) +### Critical (Must Fix) {list critical findings with full details} -### HIGH (Should Fix) +### High (Should Fix) {list high findings with full details} -### MEDIUM (Consider Fixing) +### Medium (Consider Fixing) {list medium findings} -### LOW (Nice to Fix) +### Low (Nice to Fix) {list low findings} ---