From 41ed5e84b55771b8241918ad95b5bbb38894e1ec Mon Sep 17 00:00:00 2001 From: Andreas Solleder Date: Sat, 6 Jun 2026 15:32:20 +0200 Subject: [PATCH 1/2] fix(metrics): remove incorrect separator_counts subtractive LOO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit separator_counts.analyze_loo/2 subtracted separator counts from an original-file baseline using a block string rebuilt from normalized structural tokens, which collapse inter-token whitespace. The baseline came from analyze(original) while the block came from the normalized token stream, so the subtraction mixed two different tokenizations. Empirically the LOO value diverged from a full re-analyze of the reconstructed file in 0/50 real blocks (pricing.ex) and 15/15 in mega_service.ex — every refactoring-potential delta for these counts has been wrong since the metric gained analyze_loo in #30. Drop analyze_loo/2 so the block-impact analyzer falls back to a full re-analyze, which is consistent with the other 22 file metrics. Add a regression guard (subtractive_loo_test) that runs every analyze_loo metric against a full re-analyze on real sample files with deep nesting, plus a non-vacuous check asserting the fixtures actually collapse whitespace on reconstruction. Verified the guard goes red on the reintroduced bug and green on the fix. --- lib/codeqa/metrics/file/separator_counts.ex | 15 ++- .../metrics/file/subtractive_loo_test.exs | 95 +++++++++++++++++++ 2 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 test/codeqa/metrics/file/subtractive_loo_test.exs 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/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 From afb3a84e8adce24de8bf95d673ec3cf2ee3ec93b Mon Sep 17 00:00:00 2001 From: Andreas Solleder Date: Sat, 6 Jun 2026 15:45:14 +0200 Subject: [PATCH 2/2] perf(health-report): scope per-node block impact to changed files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The block-impact analyzer ran a full per-node leave-one-out (each node: a whole-file re-analyze plus two cosine passes over ~1400 behaviors) for every file in the repo, then TopBlocks discarded the nodes of every file not in the diff. On a 647-file project this meant ~30min+ of work per run, almost all of it thrown away — and health-report does it twice (head and base snapshot). Scope per-node computation to the PR's changed files. The codebase aggregate, baseline cosines, similarity and near-duplicate metrics are still computed over every file, so the codebase scope, overall grade and each changed block's codebase-scope delta are unchanged. Only the per-file node output — read for changed files alone — is skipped for the rest. Wiring: resolve changed_files up front (before analysis) and pass them as :node_paths through analyze_codebase to BlockImpactAnalyzer, which feeds out-of-scope files "" so compute_nodes_timed short-circuits to no nodes. nil (no base ref / standalone run) keeps the all-files behavior. Measured on a 647-file project with 18 changed files: head analysis 135s vs an extrapolated ~30min unscoped. Tests assert the scoped run's nodes are bit-identical to the unscoped run for in-scope files, empty for the rest, and that the codebase aggregate is untouched. --- lib/codeqa/block_impact_analyzer.ex | 19 +++++++++++- lib/codeqa/cli/health_report.ex | 34 ++++++++++++++------ lib/codeqa/engine/analyzer.ex | 2 +- test/codeqa/block_impact_analyzer_test.exs | 36 ++++++++++++++++++++++ 4 files changed, 79 insertions(+), 12 deletions(-) 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/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