diff --git a/lib/codeqa/block_impact_analyzer.ex b/lib/codeqa/block_impact_analyzer.ex index 18c0dee..8d99dd7 100644 --- a/lib/codeqa/block_impact_analyzer.ex +++ b/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, "", diff --git a/lib/codeqa/cli/health_report.ex b/lib/codeqa/cli/health_report.ex index 626198f..ced0d88 100644 --- a/lib/codeqa/cli/health_report.ex +++ b/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 diff --git a/lib/codeqa/engine/analyzer.ex b/lib/codeqa/engine/analyzer.ex index da35826..3dbdd4d 100644 --- a/lib/codeqa/engine/analyzer.ex +++ b/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, diff --git a/lib/codeqa/metrics/file/separator_counts.ex b/lib/codeqa/metrics/file/separator_counts.ex index 381ac45..0633d67 100644 --- a/lib/codeqa/metrics/file/separator_counts.ex +++ b/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: diff --git a/test/codeqa/block_impact_analyzer_test.exs b/test/codeqa/block_impact_analyzer_test.exs index dd70a01..0a3bd79 100644 --- a/test/codeqa/block_impact_analyzer_test.exs +++ b/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 diff --git a/test/codeqa/metrics/file/subtractive_loo_test.exs b/test/codeqa/metrics/file/subtractive_loo_test.exs new file mode 100644 index 0000000..7642356 --- /dev/null +++ b/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