From a821b1d0e9c52e74050ea438e59c56262947c5e2 Mon Sep 17 00:00:00 2001 From: Jan Schlosser Date: Thu, 13 Aug 2026 15:33:15 +0200 Subject: [PATCH] SARIF ruleIndex and artifactLocation.index values are local references into each input run. MergeCommand previously compared raw Result objects before RunMergingVisitor remapped those references, so semantically identical findings from separate logs could receive different indices and survive deduplication. This is observable when merging CodeQL SARIF generated from two different databases. Perform optional deduplication after RunMergingVisitor has canonicalized referenced rules, artifacts, logical locations, and invocations. Keep the behavior opt-in for other RunMergingVisitor consumers, and add a regression test covering equivalent results with different local rule and artifact indices. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Sarif.Multitool.Library/MergeCommand.cs | 13 +-- src/Sarif/Visitors/RunMergingVisitor.cs | 8 +- .../MergeCommandTests.cs | 100 ++++++++++++++++++ 3 files changed, 108 insertions(+), 13 deletions(-) diff --git a/src/Sarif.Multitool.Library/MergeCommand.cs b/src/Sarif.Multitool.Library/MergeCommand.cs index 65a7abd277..f3c5110c92 100644 --- a/src/Sarif.Multitool.Library/MergeCommand.cs +++ b/src/Sarif.Multitool.Library/MergeCommand.cs @@ -33,14 +33,12 @@ public class MergeCommand : CommandBase private readonly List _toolKeyOrder; private readonly Dictionary _toolKeyToMergedRun; private readonly Dictionary _toolKeyToVisitor; - private readonly Dictionary> _toolKeyToResults; public MergeCommand(IFileSystem fileSystem = null) : base(fileSystem) { _toolKeyOrder = new List(); _toolKeyToMergedRun = new Dictionary(); _toolKeyToVisitor = new Dictionary(); - _toolKeyToResults = new Dictionary>(); } public int Run(MergeOptions mergeOptions) @@ -163,7 +161,7 @@ private async Task MergeSarifLogsAsync() if (!_toolKeyToVisitor.TryGetValue(toolKey, out RunMergingVisitor visitor)) { visitor = _toolKeyToVisitor[toolKey] = new RunMergingVisitor(); - _toolKeyToResults[toolKey] = new HashSet(Result.ValueComparer); + visitor.DeduplicateResults = true; _toolKeyOrder.Add(toolKey); // The first run of a given tool + version supplies the merged run's @@ -178,17 +176,8 @@ private async Task MergeSarifLogsAsync() continue; } - HashSet seenResults = _toolKeyToResults[toolKey]; foreach (Result result in run.Results) { - // Drop results that are value-identical to one already merged for this - // tool. A sharded scan can re-report the same finding in more than one - // input log; the merged run should carry each finding exactly once. - if (!seenResults.Add(result)) - { - continue; - } - visitor.CurrentRun = run; visitor.VisitResult(result.DeepClone()); } diff --git a/src/Sarif/Visitors/RunMergingVisitor.cs b/src/Sarif/Visitors/RunMergingVisitor.cs index 6edc98e859..0d92de980a 100644 --- a/src/Sarif/Visitors/RunMergingVisitor.cs +++ b/src/Sarif/Visitors/RunMergingVisitor.cs @@ -31,6 +31,7 @@ public class RunMergingVisitor : SarifRewritingVisitor private List LogicalLocations { get; } private List Rules { get; } private List Invocations { get; } + private HashSet SeenResults { get; } private Dictionary RuleIdToIndex { get; } private Dictionary, int> LogicalLocationToIndex { get; } @@ -38,6 +39,7 @@ public class RunMergingVisitor : SarifRewritingVisitor private Dictionary InvocationBaseIndexByRun { get; } public Run CurrentRun { get; set; } + public bool DeduplicateResults { get; set; } public RunMergingVisitor() { @@ -46,6 +48,7 @@ public RunMergingVisitor() LogicalLocations = new List(); Rules = new List(); Invocations = new List(); + SeenResults = new HashSet(Result.ValueComparer); RuleIdToIndex = new Dictionary(); LogicalLocationToIndex = new Dictionary, int>(); @@ -117,7 +120,10 @@ public override Result VisitResult(Result node) RemapInvocationIndex(node); Result result = base.VisitResult(node); - Results.Add(result); + if (!DeduplicateResults || SeenResults.Add(result)) + { + Results.Add(result); + } return result; } diff --git a/src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandTests.cs b/src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandTests.cs index d1238e0cf3..b4beae59c1 100644 --- a/src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandTests.cs +++ b/src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandTests.cs @@ -3,10 +3,12 @@ using System; using System.IO; +using System.Linq; using FluentAssertions; using Moq; +using Newtonsoft.Json.Linq; using Xunit; using Xunit.Abstractions; @@ -46,6 +48,55 @@ public void MergeCommand_WhenThereAreDuplicatedResults_ProducesNonDuplicatedResu RunTest("DuplicatedResults.sarif"); } + [Fact] + public void MergeCommand_WhenDuplicateResultsUseDifferentLocalIndices_ProducesOneResult() + { + string inputFolderPath = Directory.GetCurrentDirectory(); + string outputFileName = Guid.NewGuid().ToString() + SarifConstants.SarifFileExtension; + string outputFilePath = Path.Combine(TestOutputDirectory, outputFileName); + var mockFileSystem = new Mock(); + + mockFileSystem.Setup(x => x.FileExists(outputFilePath)).Returns(false); + mockFileSystem.Setup(x => x.DirectoryExists(inputFolderPath)).Returns(true); + mockFileSystem + .Setup(x => x.DirectoryEnumerateFiles(inputFolderPath, "*.sarif", SearchOption.TopDirectoryOnly)) + .Returns(new[] { "first_run.sarif", "second_run.sarif" }); + mockFileSystem + .Setup(x => x.FileReadAllText("first_run.sarif")) + .Returns(CreateSarifWithLocalIndices( + new[] { "rule-a", "rule-b" }, + new[] { "source/a.cpp", "source/b.cpp" }, + ruleIndex: 0, + artifactIndex: 0)); + mockFileSystem + .Setup(x => x.FileReadAllText("second_run.sarif")) + .Returns(CreateSarifWithLocalIndices( + new[] { "rule-b", "rule-a" }, + new[] { "source/b.cpp", "source/a.cpp" }, + ruleIndex: 1, + artifactIndex: 1)); + mockFileSystem + .Setup(x => x.DirectoryCreateDirectory(TestOutputDirectory)) + .Returns((string path) => Directory.CreateDirectory(path)); + mockFileSystem + .Setup(x => x.FileCreate(outputFilePath)) + .Returns((string path) => File.Create(path)); + + var options = new MergeOptions + { + OutputDirectoryPath = TestOutputDirectory, + TargetFileSpecifiers = new[] { "*.sarif" }, + OutputFileName = outputFileName, + OutputFileOptions = new[] { FilePersistenceOptions.ForceOverwrite, FilePersistenceOptions.PrettyPrint }, + }; + + int returnCode = new MergeCommand(mockFileSystem.Object).Run(options); + returnCode.Should().Be(0); + + JToken results = JObject.Parse(File.ReadAllText(outputFilePath))["runs"][0]["results"]; + results.Count().Should().Be(1); + } + [Fact] public void MergeCommand_WhenPassNoFolderOnlyFile_ProducesCorrectResults() { @@ -115,5 +166,54 @@ private void PrepareFileSystemMock(string inputResourceName, string inputFolderP return; } } + + private static string CreateSarifWithLocalIndices( + string[] ruleIds, + string[] artifactUris, + int ruleIndex, + int artifactIndex) + { + return JObject.FromObject(new + { + version = "2.1.0", + runs = new[] + { + new + { + tool = new + { + driver = new + { + name = "CodeQL", + version = "1.0", + rules = ruleIds.Select(id => new { id }).ToArray(), + }, + }, + artifacts = artifactUris + .Select(uri => new { location = new { uri } }) + .ToArray(), + results = new[] + { + new + { + ruleIndex, + message = new { text = "same finding" }, + locations = new[] + { + new + { + physicalLocation = new + { + artifactLocation = new { index = artifactIndex }, + region = new { startLine = 10, startColumn = 1 }, + }, + }, + }, + }, + }, + }, + }, + }).ToString(); + } } }