Add senior bug review for SARIF merge and parsing issues

CODEBASE_ANALYSIS.md

@@ -0,0 +1,94 @@
+# Senior Bug Review
+
+## Scope
+Focused review of executable helper code in this repository, prioritizing correctness-impacting bugs over style issues.
+
+## Bug 1 — SARIF merge drops valid findings (high impact)
+**File:** `plugins/static-analysis/skills/semgrep/scripts/merge_sarif.py`
+
+### Problem
+The fallback merge de-duplicates findings using only:
+- `ruleId`
+- `artifactLocation.uri`
+- `region.startLine`
+
+This key is too coarse. Multiple distinct findings on the same line (same rule, different sink/trace/message) are silently discarded.
+
+### Why this is a bug
+Security tooling must preserve findings unless equivalence is proven. Current logic can under-report vulnerabilities and mislead triage.
+
+### Evidence
+`dedup_key = (rule_id, uri, start_line)` then skip if already seen.
+
+### Fix direction
+Use stable fingerprints when present (`partialFingerprints`/`fingerprints`) and only fall back to a richer composite key including start/end columns and normalized message text.
+
+---
+
+## Bug 2 — Rule collisions across tools corrupt merged metadata (medium impact)
+**File:** `plugins/static-analysis/skills/semgrep/scripts/merge_sarif.py`
+
+### Problem
+Merged rules are keyed only by `rule.id`, regardless of tool source.
+
+### Why this is a bug
+Different tools can legitimately share the same rule id string with different semantics. Current behavior keeps the first seen rule and overwrites logical provenance.
+
+### Evidence
+`seen_rules` is a dict keyed by `rule_id` only.
+
+### Fix direction
+Namespace rules by tool identity (e.g., `tool_name:rule_id`) or preserve per-run separation instead of flattening all results into a single run.
+
+---
+
+## Bug 3 — Dedup fingerprint can merge unrelated findings in large repos (medium impact)
+**File:** `plugins/static-analysis/skills/sarif-parsing/resources/sarif_helpers.py`
+
+### Problem
+`compute_fingerprint()` uses only:
+- `ruleId`
+- **filename only** (not path)
+- `startLine`
+- message prefix (first 50 chars)
+
+### Why this is a bug
+Two different files with the same basename (common in monorepos) can collide and be treated as duplicates, causing false “fixed/unchanged/new” results.
+
+### Evidence
+The fingerprint intentionally truncates path to `Path(file_path).name`.
+
+### Fix direction
+Use normalized relative path (or URI) plus start/end column and full message hash. Keep optional path-stripping as an explicit mode, not default.
+
+---
+
+## Bug 4 — Path normalization mishandles file URIs with authorities (low/medium impact)
+**File:** `plugins/static-analysis/skills/sarif-parsing/resources/sarif_helpers.py`
+
+### Problem
+`normalize_path()` strips `file://` using string slicing, which can mis-handle URIs like `file://hostname/path` and certain platform-specific forms.
+
+### Why this is a bug
+Incorrect path normalization can break grouping/filtering and produce unstable fingerprints across environments.
+
+### Evidence
+Logic does `uri = uri[7:]` without URI parsing.
+
+### Fix direction
+Parse file URIs via `urllib.parse.urlparse` and handle netloc/path explicitly.
+
+---
+
+## Priority Order
+1. Fix Bug 1 first (finding loss).
+2. Fix Bug 3 next (diff accuracy).
+3. Fix Bug 2 (metadata correctness in multi-tool merges).
+4. Fix Bug 4 (portability/stability).
+
+## Recommended Validation
+- Add regression SARIF fixtures with intentionally colliding findings.
+- Assert merged result counts remain stable across:
+  - same rule + same line + different column/message
+  - same basename in different directories
+  - mixed-tool rule-id overlap