randlee
diff --git a/‎BUG_REPORT_003_Line_Number_Overlaps_Root_Cause.md‎
Lines changed: 333 additions & 0 deletions b/‎BUG_REPORT_003_Line_Number_Overlaps_Root_Cause.md‎
Lines changed: 333 additions & 0 deletions
@@ -0,0 +1,333 @@
+# Bug Report #003: Line Number Overlaps - Root Cause Analysis
+
+## Executive Summary
+
+**Bug:** Line number overlaps and duplicate reporting in roslyn-diff CLI output across all formats (JSON, HTML, Text)
+**Root Cause:** Duplicate reporting of nested elements - both as top-level changes AND as children of parent changes
+**Severity:** P0 - Affects 79.4% of tests (27 out of 34 tests fail)
+**Impact:** All output formats affected (JSON, HTML, Text)
+**Root Cause Component:** `src/RoslynDiff.Core/Comparison/NodeMatcher.cs`, method `ExtractNodesRecursive`
+**Fix Complexity:** Medium - Requires logic change to prevent duplicate extraction
+
+## Root Cause Identified
+
+### Component
+**File:** `/Users/randlee/Documents/github/roslyn-diff-worktrees/feature/sample-data-validation-tests/src/RoslynDiff.Core/Comparison/NodeMatcher.cs`
+**Method:** `ExtractNodesRecursive` (lines 257-273)
+
+### The Problem
+
+The `ExtractNodesRecursive` method recursively extracts ALL structural nodes from the syntax tree, including nested nodes. This causes the same node to be reported multiple times:
+
+1. **As a child** of its parent's change
+2. **As a top-level change** in the main changes list
+
+**Current Code (Buggy):**
+```csharp
+private void ExtractNodesRecursive(SyntaxNode node, List<NodeInfo> nodes)
+{
+    // Check if this node is a structural node we want to track
+    if (IsStructuralNode(node))
+    {
+        var name = GetNodeName(node);
+        var kind = GetChangeKind(node);
+        var signature = GetSignature(node);
+        nodes.Add(new NodeInfo(node, name, kind, signature));  // ⚠️ ADDS EVERY NODE
+    }
+
+    // Recursively process children
+    foreach (var child in node.ChildNodes())
+    {
+        ExtractNodesRecursive(child, nodes);  // ⚠️ RECURSIVELY EXTRACTS ALL CHILDREN
+    }
+}
+```
+
+## How It Causes Overlaps
+
+### Example with Calculator.cs
+
+**File Changes:**
+- Before: 2 methods (Add, Subtract)
+- After: 4 methods (Add, Subtract, Multiply, Divide)
+- Added XML documentation to Add and Subtract
+
+**What Happens:**
+
+1. **ExtractNodesRecursive extracts ALL nodes:**
+   - Namespace "Samples" (lines 1-56 new, 1-23 old)
+   - Class "Calculator" (lines 6-56 new, 6-23 old) - child of namespace
+   - Method "Add" (lines 14-17 new, 11-14 old) - child of class
+   - Method "Subtract" (lines 25-28 new, 19-22 old) - child of class
+   - Method "Multiply" (lines 36-39 new) - child of class
+   - Method "Divide" (lines 48-55 new) - child of class
+
+2. **SyntaxComparer processes these nodes:**
+   - Matches Namespace (old vs new) → Modified
+   - Matches Class (old vs new) → Modified
+   - Matches Add (old vs new) → Modified
+   - Matches Subtract (old vs new) → Modified
+   - Unmatched Multiply (new only) → Added
+   - Unmatched Divide (new only) → Added
+
+3. **SyntaxComparer ALSO processes children recursively:**
+   - When processing modified Class, calls `CompareChildren()`
+   - Finds added Multiply and Divide as children
+   - Adds them to Class.Children list
+
+4. **Result: DUPLICATE REPORTING**
+   - Changes list contains:
+     1. Modified Namespace (lines 1-56) WITH CHILD Modified Class
+     2. Modified Class (lines 6-56) WITH CHILDREN Added Multiply + Added Divide
+     3. Added Multiply (lines 36-39) ← DUPLICATE!
+     4. Added Divide (lines 48-55) ← DUPLICATE!
+
+5. **Line Number Overlaps Detected:**
+   - Namespace 1-56 overlaps with Class 6-56 ✓ (valid nesting)
+   - Class 6-56 overlaps with Multiply 36-39 ✓ (valid nesting)
+   - Class 6-56 overlaps with Divide 48-55 ✓ (valid nesting)
+   - But also: Top-level Multiply 36-39 overlaps with Class 6-56 ❌ (INVALID DUPLICATE)
+   - And: Top-level Divide 48-55 overlaps with Class 6-56 ❌ (INVALID DUPLICATE)
+
+## Why All Formats Affected
+
+**Answer:** All formats (JSON, HTML, Text) share the same root cause.
+
+All output formatters receive the same `DiffResult` object from `CSharpDiffer.Compare()`, which contains the duplicate changes. The formatters simply serialize this data structure:
+
+- **JSON formatter** (`JsonOutputFormatter`) serializes changes to JSON
+- **HTML formatter** (`HtmlFormatter`) renders changes as HTML sections
+- **Text formatter** (`TextFormatter`) renders changes as text output
+
+Since the duplicates exist in the core `DiffResult`, all formatters inherit the problem. This is why fixing the issue in `NodeMatcher` will fix all formats simultaneously.
+
+## Detailed Analysis of JSON Output
+
+Looking at the actual JSON output from Calculator.cs:
+
+```json
+{
+  "files": [{
+    "changes": [
+      {
+        "type": "modified",
+        "kind": "namespace",
+        "name": "Samples",
+        "location": { "startLine": 1, "endLine": 56 },
+        "children": [{
+          "type": "modified",
+          "kind": "class",
+          "name": "Calculator",
+          "location": { "startLine": 6, "endLine": 56 }
+        }]
+      },
+      {
+        "type": "modified",
+        "kind": "class",
+        "name": "Calculator",
+        "location": { "startLine": 6, "endLine": 56 },  // ⚠️ DUPLICATE #1
+        "children": [{
+          "type": "added",
+          "kind": "method",
+          "name": "Multiply",
+          "location": { "startLine": 36, "endLine": 39 }
+        }, {
+          "type": "added",
+          "kind": "method",
+          "name": "Divide",
+          "location": { "startLine": 48, "endLine": 55 }
+        }]
+      },
+      {
+        "type": "added",
+        "kind": "method",
+        "name": "Multiply",
+        "location": { "startLine": 36, "endLine": 39 }  // ⚠️ DUPLICATE #2
+      },
+      {
+        "type": "added",
+        "kind": "method",
+        "name": "Divide",
+        "location": { "startLine": 48, "endLine": 55 }  // ⚠️ DUPLICATE #3
+      }
+    ]
+  }]
+}
+```
+
+**Duplicates:**
+1. Class "Calculator" appears TWICE: once as child of namespace, once at top level
+2. Method "Multiply" appears TWICE: once as child of class, once at top level
+3. Method "Divide" appears TWICE: once as child of class, once at top level
+
+## Why This Bug Wasn't Caught Earlier
+
+This bug likely went unnoticed because:
+
+1. **No validation tests existed** - Sprint 4 created the first comprehensive validation tests
+2. **Visual inspection misses duplicates** - When looking at diff output, users see the changes but don't notice they're reported twice
+3. **Line-mode diff doesn't have this issue** - Only Roslyn-mode has hierarchical nesting
+4. **Smoke tests passed** - The CLI works and produces output, just with duplicates
+
+## Fix Strategy
+
+### Option 1: Extract Only Top-Level Nodes (Recommended)
+
+Modify `ExtractNodesRecursive` to only extract top-level structural nodes (namespace, top-level classes). Then rely on `CompareChildren()` to handle nested elements.
+
+**Pros:**
+- Clean separation: top-level extraction vs. child comparison
+- Prevents duplicates by design
+- Simpler logic
+
+**Cons:**
+- Requires careful handling of top-level vs. nested classes
+- May miss some edge cases initially
+
+### Option 2: Deduplicate After Extraction
+
+Keep current extraction logic but deduplicate changes before returning.
+
+**Pros:**
+- Minimal code changes
+- Preserves existing extraction logic
+
+**Cons:**
+- Still wasteful (extracts then removes)
+- Harder to get right (what's a "real" duplicate vs. legitimate reporting?)
+- Doesn't address root cause
+
+### Option 3: Track Parent Context During Extraction
+
+Modify extraction to track whether a node is already captured as a child of a parent change.
+
+**Pros:**
+- Most flexible
+- Can handle complex nesting scenarios
+
+**Cons:**
+- Most complex to implement
+- Harder to maintain
+- Overkill for this problem
+
+**Chosen Strategy: Option 1**
+
+Extract only top-level nodes and let `CompareChildren()` handle nested changes. This is the cleanest solution that addresses the root cause.
+
+## Implementation Plan
+
+### 1. Modify ExtractNodesRecursive
+
+Change to only extract top-level structural nodes:
+
+```csharp
+public IReadOnlyList<NodeInfo> ExtractStructuralNodes(SyntaxNode root)
+{
+    var nodes = new List<NodeInfo>();
+
+    // Only extract immediate children of root (top-level declarations)
+    foreach (var child in root.ChildNodes())
+    {
+        if (IsStructuralNode(child))
+        {
+            var name = GetNodeName(child);
+            var kind = GetChangeKind(child);
+            var signature = GetSignature(child);
+            nodes.Add(new NodeInfo(child, name, kind, signature));
+        }
+    }
+
+    return nodes;
+}
+```
+
+### 2. Remove Recursive Extraction
+
+Delete the `ExtractNodesRecursive` method since it's no longer needed.
+
+### 3. Ensure CompareChildren Still Works
+
+Verify that `CompareChildren()` in `SyntaxComparer` still correctly handles nested changes. This method already uses `ExtractImmediateStructuralChildren` which is separate from `ExtractNodesRecursive`.
+
+## Expected Results After Fix
+
+### Before Fix
+- Total changes: 7 (3 modifications + 4 additions)
+- Changes list: [Modified Namespace, Modified Class, Modified Class (dup), Added Multiply (child), Added Multiply (dup), Added Divide (child), Added Divide (dup)]
+- Overlaps: 21+ overlapping line ranges
+- Test pass rate: 20.6% (7 out of 34 tests)
+
+### After Fix
+- Total changes: 3 (1 modification with nested children)
+- Changes list: [Modified Namespace with Modified Class child, which has Added Multiply and Added Divide children]
+- Overlaps: 0 invalid overlaps (only valid parent-child nesting)
+- Test pass rate: Expected 60-80% (20-27 out of 34 tests)
+
+### Still Valid (Not Overlaps)
+- Namespace (1-56) contains Class (6-56) ✓
+- Class (6-56) contains Multiply (36-39) ✓
+- Class (6-56) contains Divide (48-55) ✓
+
+These are proper parent-child relationships, not overlaps.
+
+## Cross-Format Consistency
+
+After the fix, all formats should report identical line numbers because:
+
+1. Single source of truth: `DiffResult` from `CSharpDiffer`
+2. No duplicates in change list
+3. All formatters read from same data structure
+4. Line numbers come from same Roslyn location calculation
+
+This should fix:
+- JsonConsistencyTests (5 failing → expected 2-3 failing)
+- HtmlConsistencyTests (5 failing → expected 1-2 failing)
+- CrossFormatConsistencyTests (6 failing → expected 1-2 failing)
+- LineNumberIntegrityTests (4 failing → expected 0-1 failing)
+- SampleCoverageTests (4 failing → expected 0-1 failing)
+- ExternalToolCompatibilityTests (3 failing → expected 1-2 failing)
+
+## Verification Steps
+
+1. **Apply fix** to NodeMatcher.cs
+2. **Build** project: `dotnet build`
+3. **Generate new output**: `dotnet run --project src/RoslynDiff.Cli/ -- diff samples/before/Calculator.cs samples/after/Calculator.cs --json /tmp/fixed-output.json`
+4. **Verify no duplicates** in JSON
+5. **Count changes**: Should see 3 changes (not 7)
+6. **Run tests**: `dotnet test --filter "Category=SampleValidation"`
+7. **Verify improvement**: Pass rate should increase from 20.6% to 60%+
+
+## Risk Assessment
+
+**Risk Level:** Medium-Low
+
+**Why Medium:**
+- Core change detection logic
+- Affects all output formats
+- Could break existing functionality if wrong
+
+**Why Low:**
+- Isolated change (one method)
+- Clear root cause identified
+- Comprehensive test suite to verify
+- Easy to revert if issues arise
+
+**Mitigation:**
+- Run full test suite after fix
+- Manually verify output for sample files
+- Test with different file types (Calculator.cs, UserService.cs)
+- Compare before/after JSON to confirm duplicates removed
+
+## Conclusion
+
+BUG-003 is caused by duplicate extraction of nested structural nodes in `NodeMatcher.ExtractNodesRecursive`. The fix is to extract only top-level nodes and rely on `CompareChildren()` for nested changes. This will eliminate overlaps and fix 79.4% of failing tests.
+
+**Status:** Ready for fix implementation
+**Estimated Fix Time:** 30 minutes (15 min code + 10 min test + 5 min verify)
+**Expected Outcome:** Test pass rate improves from 20.6% to 60-80%
+
+---
+
+**Report Date:** 2026-01-18
+**Analyzed By:** Agent D (BUG-003 Investigator & Fixer)
+**Sprint:** Sprint 5