fix(metrics) + perf(health-report): correct separator LOO, scope block-impact to changed files

lib/codeqa/block_impact_analyzer.ex

@@ -70,6 +70,12 @@ defmodule CodeQA.BlockImpactAnalyzer do
   def analyze(pipeline_result, files_map, opts \\ []) do
     nodes_top = Keyword.get(opts, :nodes_top, 3)
     workers = Keyword.get(opts, :workers, System.schedulers_online())
+    # When set, per-node leave-one-out is computed only for these paths (the PR's
+    # changed files). The codebase aggregate and baseline cosines below are still
+    # built from every file, so the codebase scope and overall grade are
+    # unaffected — only the per-file node computation, whose output is read for
+    # changed files alone, is skipped for the rest. `nil` means all files.
+    node_paths = Keyword.get(opts, :node_paths)
 
     t0 = now()
 
@@ -109,12 +115,13 @@ defmodule CodeQA.BlockImpactAnalyzer do
     )
 
     file_results = pipeline_result["files"]
+    node_path_set = node_paths && MapSet.new(node_paths)
 
     updated_files =
       file_results
       |> Task.async_stream(
         fn {path, file_data} ->
-          content = Map.get(files_map, path, "")
+          content = node_content(path, files_map, node_path_set)
           baseline_file_metrics = Map.get(file_data, "metrics", %{})
 
           {nodes, file_measurements} =
@@ -151,6 +158,16 @@ defmodule CodeQA.BlockImpactAnalyzer do
     Map.put(pipeline_result, "files", updated_files)
   end
 
+  # Returns a file's content for node computation. With no scope (`nil`), every
+  # file is in scope. With a scope set, files outside it get "" — which makes
+  # compute_nodes_timed short-circuit to no nodes, skipping the expensive
+  # per-node leave-one-out while leaving the codebase aggregate untouched.
+  defp node_content(path, files_map, nil), do: Map.get(files_map, path, "")
+
+  defp node_content(path, files_map, set) do
+    if MapSet.member?(set, path), do: Map.get(files_map, path, ""), else: ""
+  end
+
   defp compute_nodes_timed(
          _path,
          "",

lib/codeqa/cli/health_report.ex

@@ -77,9 +77,17 @@ defmodule CodeQA.CLI.HealthReport do
 
     telemetry_pid = if opts[:telemetry], do: attach_telemetry()
 
+    # In a PR context the report only shows blocks from changed files, so per-node
+    # leave-one-out is computed only for those. The codebase aggregate and
+    # baseline cosines are still built from every file. `nil` (no base ref) means
+    # all files get nodes, preserving standalone-run behavior.
+    {changed_files, diff_line_ranges} = collect_diff(path, base_ref, head_ref)
+    node_paths = if changed_files == [], do: nil, else: Enum.map(changed_files, & &1.path)
+
     analyze_opts =
       Options.build_analyze_opts(opts) ++
-        Config.near_duplicate_blocks_opts() ++ [compute_nodes: true]
+        Config.near_duplicate_blocks_opts() ++
+        [compute_nodes: true, node_paths: node_paths]
 
     start_time = System.monotonic_time(:millisecond)
     results = Analyzer.analyze_codebase(files, analyze_opts)
@@ -101,8 +109,7 @@ defmodule CodeQA.CLI.HealthReport do
         "total_bytes" => total_bytes
       })
 
-    {base_results, changed_files, diff_line_ranges} =
-      collect_base_snapshot(path, base_ref, head_ref, analyze_opts)
+    base_results = collect_base_snapshot(path, base_ref, analyze_opts)
 
     detail = parse_detail(opts[:detail])
     format = parse_format(opts[:format])
@@ -134,18 +141,25 @@ defmodule CodeQA.CLI.HealthReport do
     output
   end
 
-  defp collect_base_snapshot(_path, nil, _head_ref, _analyze_opts), do: {nil, [], %{}}
+  # Resolves the changed files and per-file diff line ranges once, up front, so
+  # the head analysis can scope its per-node work to the changed files. Returns
+  # `{[], %{}}` outside a PR context (no base ref) — meaning "no scoping".
+  defp collect_diff(_path, nil, _head_ref), do: {[], %{}}
 
-  defp collect_base_snapshot(path, base_ref, head_ref, analyze_opts) do
-    IO.puts(:stderr, "Collecting base snapshot at #{base_ref}...")
-    base_files = Git.collect_files_at_ref(path, base_ref)
+  defp collect_diff(path, base_ref, head_ref) do
     changed = Git.changed_files(path, base_ref, head_ref)
     diff_ranges = collect_diff_ranges(path, base_ref, head_ref)
+    {changed, diff_ranges}
+  end
 
-    IO.puts(:stderr, "Analyzing base snapshot (#{map_size(base_files)} files)...")
-    base_res = Analyzer.analyze_codebase(base_files, analyze_opts)
+  defp collect_base_snapshot(_path, nil, _analyze_opts), do: nil
 
-    {base_res, changed, diff_ranges}
+  defp collect_base_snapshot(path, base_ref, analyze_opts) do
+    IO.puts(:stderr, "Collecting base snapshot at #{base_ref}...")
+    base_files = Git.collect_files_at_ref(path, base_ref)
+
+    IO.puts(:stderr, "Analyzing base snapshot (#{map_size(base_files)} files)...")
+    Analyzer.analyze_codebase(base_files, analyze_opts)
   end
 
   defp collect_diff_ranges(path, base_ref, head_ref) do

lib/codeqa/engine/analyzer.ex

@@ -136,7 +136,7 @@ defmodule CodeQA.Engine.Analyzer do
     if Keyword.get(opts, :compute_nodes, false) do
       nodes_opts =
         [baseline_codebase_agg: aggregate] ++
-          Keyword.take(opts, [:nodes_top, :workers, :behavior_config_pid])
+          Keyword.take(opts, [:nodes_top, :workers, :behavior_config_pid, :node_paths])
 
       pipeline_result = %{
         "files" => file_results,

lib/codeqa/metrics/file/separator_counts.ex

@@ -25,14 +25,13 @@ defmodule CodeQA.Metrics.File.SeparatorCounts do
       "dot_count" => count(content, ".")
     }
 
-  @impl true
-  def analyze_loo(baseline, block_content),
-    do: %{
-      "underscore_count" => baseline["underscore_count"] - count(block_content, "_"),
-      "hyphen_count" => baseline["hyphen_count"] - count(block_content, "-"),
-      "slash_count" => baseline["slash_count"] - count(block_content, "/"),
-      "dot_count" => baseline["dot_count"] - count(block_content, ".")
-    }
+  # No `analyze_loo/2`: the subtractive path is incorrect here. The block-impact
+  # analyzer subtracts a block whose content is rebuilt from normalized
+  # structural tokens (which drop inter-token whitespace), while the baseline is
+  # computed from the original file. Counting separators in that whitespace-
+  # collapsed block string diverges from re-analyzing the reconstructed file
+  # (empirically 0/50 matches). Falling back to a full re-analyze keeps the LOO
+  # delta consistent with every other metric.
 
   defp count(content, char),
     do:

test/codeqa/block_impact_analyzer_test.exs

@@ -85,6 +85,42 @@ defmodule CodeQA.BlockImpactAnalyzerTest do
       |> Enum.each(&assert length(&1["refactoring_potentials"]) <= 1)
     end
 
+    test "node_paths scopes per-node work without changing scoped files' nodes" do
+      files = %{
+        "lib/changed.ex" => @fixture_content,
+        "lib/untouched.ex" => @fixture_content
+      }
+
+      pipeline_result = Analyzer.analyze_codebase(files)
+
+      full = BlockImpactAnalyzer.analyze(pipeline_result, files)
+      scoped = BlockImpactAnalyzer.analyze(pipeline_result, files, node_paths: ["lib/changed.ex"])
+
+      # The scoped file's nodes are bit-identical to the unscoped run: scoping
+      # only decides WHICH files compute nodes, never HOW.
+      assert scoped["files"]["lib/changed.ex"]["nodes"] ==
+               full["files"]["lib/changed.ex"]["nodes"]
+
+      # The out-of-scope file gets no nodes (its expensive LOO is skipped)...
+      assert scoped["files"]["lib/untouched.ex"]["nodes"] == []
+      # ...but it still carries its metrics, so the codebase aggregate is intact.
+      assert scoped["files"]["lib/untouched.ex"]["metrics"] ==
+               full["files"]["lib/untouched.ex"]["metrics"]
+
+      # The codebase scope (aggregate) is identical with and without scoping.
+      assert scoped["codebase"] == full["codebase"]
+    end
+
+    test "node_paths: nil computes nodes for all files (standalone behavior)" do
+      files = %{"lib/a.ex" => @fixture_content, "lib/b.ex" => @fixture_content}
+      pipeline_result = Analyzer.analyze_codebase(files)
+
+      result = BlockImpactAnalyzer.analyze(pipeline_result, files, node_paths: nil)
+
+      assert result["files"]["lib/a.ex"]["nodes"] != []
+      assert result["files"]["lib/b.ex"]["nodes"] != []
+    end
+
     test "node['type'] reflects classified block kind, not the always-:code default" do
       content = """
       defmodule Foo do

test/codeqa/metrics/file/subtractive_loo_test.exs

@@ -0,0 +1,95 @@
+defmodule CodeQA.Metrics.File.SubtractiveLooTest do
+  @moduledoc """
+  Regression guard for the leave-one-out path in block-impact analysis.
+
+  Any file metric that implements `analyze_loo/2` claims it can derive the
+  file-minus-block value subtractively. This asserts that claim holds: for every
+  top-level and nested block in real sample files, a metric's LOO value (via
+  `Analyzer.analyze_file_for_loo_partial/4`) must equal a full re-analyze of the
+  reconstructed file.
+
+  This caught a real bug: `separator_counts` had an `analyze_loo/2` that
+  subtracted counts from an original-file baseline using a block string rebuilt
+  from normalized structural tokens (inter-token whitespace collapsed) — wrong on
+  15/15 blocks in `mega_service.ex`. A metric that cannot satisfy this invariant
+  must NOT implement `analyze_loo/2`; the full re-analyze fallback is correct.
+
+  The harness fixtures are chosen because `reconstruct_without` collapses
+  whitespace there (the normalized join differs from the source), which is the
+  exact condition that exposes a bad subtractive metric. `guards the invariant`
+  asserts the fixtures actually exercise that condition, so this test can never
+  silently become a no-op the way an over-clean fixture would.
+  """
+  use ExUnit.Case, async: true
+
+  alias CodeQA.AST.Lexing.TokenNormalizer
+  alias CodeQA.AST.Parsing.Parser
+  alias CodeQA.BlockImpact.FileImpact
+  alias CodeQA.Engine.Analyzer
+  alias CodeQA.Engine.Pipeline
+  alias CodeQA.Languages.Unknown
+
+  # Real sample files with deep nesting, so reconstruct_without yields many
+  # non-trivial strict-subset reconstructions that collapse whitespace.
+  @fixtures [
+    "priv/combined_metrics/samples/file_structure/line_count_under_300/bad/mega_service.ex",
+    "priv/combined_metrics/samples/file_structure/single_responsibility/bad/user_handler.ex"
+  ]
+
+  defp subset_blocks(content) do
+    root = TokenNormalizer.normalize_structural(content)
+
+    root
+    |> Parser.detect_blocks(Unknown)
+    |> Enum.flat_map(fn node -> [node | node.children] end)
+    |> Enum.filter(&(length(&1.tokens) >= 10 and length(&1.tokens) < length(root) - 5))
+  end
+
+  test "fixtures actually collapse whitespace on reconstruction (guard is not vacuous)" do
+    for path <- @fixtures do
+      content = File.read!(path)
+
+      normalized =
+        content |> TokenNormalizer.normalize_structural() |> Enum.map_join("", & &1.content)
+
+      assert normalized != content,
+             "#{path} reconstructs byte-identically — it would not expose a bad subtractive metric"
+
+      assert subset_blocks(content) != [], "#{path} yields no strict-subset blocks"
+    end
+  end
+
+  test "every analyze_loo metric matches a full re-analyze on file-minus-block" do
+    loo_metrics =
+      Analyzer.build_registry().file_metrics
+      |> Enum.filter(fn mod ->
+        Code.ensure_loaded?(mod) and function_exported?(mod, :analyze_loo, 2)
+      end)
+
+    for path <- @fixtures do
+      content = File.read!(path)
+      root = TokenNormalizer.normalize_structural(content)
+      base_ctx = Pipeline.build_file_context(content, skip_structural: true)
+      baseline_metrics = Map.new(loo_metrics, fn mod -> {mod.name(), mod.analyze(base_ctx)} end)
+
+      for node <- subset_blocks(content), mod <- loo_metrics do
+        block_content = node.tokens |> Enum.map_join("", & &1.content)
+        reconstructed = FileImpact.reconstruct_without(root, node)
+
+        loo =
+          Analyzer.analyze_file_for_loo_partial(
+            path,
+            reconstructed,
+            baseline_metrics,
+            block_content
+          )
+
+        truth = mod.analyze(Pipeline.build_file_context(reconstructed, skip_structural: true))
+
+        assert loo[mod.name()] == truth,
+               "#{mod.name()}.analyze_loo/2 diverges from a full re-analyze on a real block in " <>
+                 "#{path} — remove analyze_loo/2 (use the re-analyze fallback) unless it matches exactly"
+      end
+    end
+  end
+end