Report a finding as unverified when its declaring assembly is outside the closure

tools/VersioningRunner/src/VersioningRunner.Tests/ClosureReclassificationTests.cs

@@ -0,0 +1,196 @@
+using VersioningRunner.Commands;
+using VersioningRunner.Models;
+using VersioningRunner.Tests.Fixtures;
+using Xunit;
+
+namespace VersioningRunner.Tests
+{
+    // The reclassification rule is easy to get subtly wrong and had no coverage at first:
+    // no test calls RunCommand.Execute, so the static state v1 relied on was never populated
+    // and the rule could not fire in any test. The three tests v1 broke therefore passed
+    // again under its gate incidentally, not because the gate was verified. These arrange
+    // the closure explicitly so the rule is actually exercised.
+    public class ClosureReclassificationTests
+    {
+        private const string ModelQaEvent =
+            "Method TryGetValueFromSource from { \"_t\" : \"System.Type\", \"Name\" : \"BH.Revit.Engine.Core.Compute, Revit_ModelQA_Engine_2022, Version=9.0.0.0, Culture=neutral, PublicKeyToken=null\", \"_bhomVersion\" : \"9.2\" } failed to deserialise.";
+
+        private const string Config2024Event =
+            "Method ProjectParameter from { \"_t\" : \"System.Type\", \"Name\" : \"BH.Revit.Engine.Core.Create, Revit_Core_Engine_2024, Version=9.0.0.0, Culture=neutral, PublicKeyToken=null\", \"_bhomVersion\" : \"9.2\" } failed to deserialise.";
+
+        private const string SubjectAsmEvent =
+            "Method Gone from { \"_t\" : \"System.Type\", \"Name\" : \"BH.Revit.Engine.Core.Compute, Revit_Core_Engine_2022, Version=9.0.0.0, Culture=neutral, PublicKeyToken=null\", \"_bhomVersion\" : \"9.2\" } failed to deserialise.";
+
+        private static FakeTestResult Tree(string description, params string[] events)
+        {
+            var leaf = new FakeTestInfo
+            {
+                Status = "Error",
+                Description = description,
+                Message = "Error: Returned null from json.",
+                Information = events.Select(m => (object)new FakeEventMessage { Message = m }).ToList()
+            };
+            return new FakeTestResult
+            {
+                Status = "Error",
+                Information = [new FakeTestResult { Status = "Error", Information = [leaf] }]
+            };
+        }
+
+        private static ClosureContext Closure(string[] loaded, string[] subject)
+        {
+            var l = new HashSet<string>(loaded, StringComparer.Ordinal);
+            return new ClosureContext(
+                l,
+                new HashSet<string>(l.Select(RunCommand.StripConfigSuffix), StringComparer.Ordinal),
+                new HashSet<string>(subject.Select(RunCommand.StripConfigSuffix), StringComparer.Ordinal));
+        }
+
+        // A non-empty candidate list is the runner's record that some OTHER loaded assembly
+        // answered for the type.
+        private static Func<string, string, string?, (string?, ClassificationPath, IReadOnlyList<string>)> Answered(
+            params string[] answering)
+            => (_, _, _) => (null, ClassificationPath.SignatureResolved, answering);
+
+        private static readonly Func<string, string, string?, (string?, ClassificationPath, IReadOnlyList<string>)> NothingAnswered =
+            (_, _, _) => (null, ClassificationPath.DeclaringTypeNotLoaded, Array.Empty<string>());
+
+        private static (VersioningResult Result, FailureDiagnostic Diag) Run(
+            FakeTestResult tree,
+            Func<string, string, string?, (string?, ClassificationPath, IReadOnlyList<string>)> probe,
+            ClosureContext? closure)
+        {
+            var diagnostics = new List<FailureDiagnostic>();
+            var result = RunCommand.ExtractFilteredResult(
+                tree, _ => true, new List<RunCommand.UnverifiedFailure>(),
+                (t, m, a) => probe(t, m, a), diagnostics, closure);
+            return (result, Assert.Single(diagnostics));
+        }
+
+        [Fact]
+        public void ForeignAssemblyAnsweredByAnother_IsUnverified()
+        {
+            var (result, d) = Run(
+                Tree("BH.Revit.Engine.Core.Compute.TryGetValueFromSource", ModelQaEvent),
+                Answered("Revit_Core_Engine_2022"),
+                Closure(loaded: ["Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022", "Revit_oM"]));
+
+            Assert.Equal(0, result.FailureCount);
+            Assert.False(d.CountedAsReal);
+            Assert.Equal(ClassificationPath.ForeignDeclaringAssembly, d.Path);
+        }
+
+        [Fact]
+        public void ConfigurationVariantNotBuilt_IsUnverified()
+        {
+            var (result, d) = Run(
+                Tree("BH.Revit.Engine.Core.Create.ProjectParameter", Config2024Event),
+                Answered("Revit_Core_Engine_2022"),
+                Closure(loaded: ["Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022"]));
+
+            Assert.Equal(0, result.FailureCount);
+            Assert.False(d.CountedAsReal);
+            Assert.Equal(ClassificationPath.ConfigurationNotBuilt, d.Path);
+        }
+
+        // The v1 defect, guarded. With no answering assembly the path is
+        // DeclaringTypeNotLoaded, which is how a genuinely removed type presents. v1 had no
+        // candidates precondition and relabelled this as foreign, turning a real removal
+        // into a silent pass.
+        [Fact]
+        public void AbsentAssemblyAndNothingAnswered_StaysReal()
+        {
+            var (result, d) = Run(
+                Tree("BH.Revit.Engine.Core.Compute.Gone", SubjectAsmEvent),
+                NothingAnswered,
+                Closure(loaded: ["Revit_oM"], subject: ["Revit_oM"]));
+
+            Assert.Equal(1, result.FailureCount);
+            Assert.True(d.CountedAsReal);
+            Assert.Equal(ClassificationPath.DeclaringTypeNotLoaded, d.Path);
+        }
+
+        [Fact]
+        public void DeclaringAssemblyPresent_IsUntouched()
+        {
+            var (result, d) = Run(
+                Tree("BH.Revit.Engine.Core.Create.ProjectParameter", Config2024Event),
+                Answered("Revit_Core_Engine_2024"),
+                Closure(loaded: ["Revit_Core_Engine_2024", "Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022"]));
+
+            Assert.Equal(1, result.FailureCount);
+            Assert.True(d.CountedAsReal);
+            Assert.Equal(ClassificationPath.SignatureResolved, d.Path);
+        }
+
+        [Fact]
+        public void NoClosureSupplied_IsUntouched()
+        {
+            var (result, d) = Run(
+                Tree("BH.Revit.Engine.Core.Compute.TryGetValueFromSource", ModelQaEvent),
+                Answered("Revit_Core_Engine_2022"),
+                closure: null);
+
+            Assert.Equal(1, result.FailureCount);
+            Assert.True(d.CountedAsReal);
+        }
+
+        // The whole family is gone, not just one configuration of it. Nothing distinguishes
+        // that from a deliberate removal, so it must not be excused.
+        [Fact]
+        public void SubjectFamilyWithNoLoadedVariant_StaysReal()
+        {
+            var (result, d) = Run(
+                Tree("BH.Revit.Engine.Core.Create.ProjectParameter", Config2024Event),
+                Answered("Revit_oM"),
+                Closure(loaded: ["Revit_oM"], subject: ["Revit_Core_Engine_2022", "Revit_oM"]));
+
+            Assert.Equal(1, result.FailureCount);
+            Assert.True(d.CountedAsReal);
+            Assert.Equal(ClassificationPath.SignatureResolved, d.Path);
+        }
+
+        [Fact]
+        public void AlreadyUnverified_IsNotRelabelled()
+        {
+            var (result, d) = Run(
+                Tree("BH.Revit.Engine.Core.Compute.TryGetValueFromSource", ModelQaEvent),
+                (_, _, _) => ("RevitAPI", ClassificationPath.SignatureBlockerOutsideBHoM, new[] { "Revit_Core_Engine_2022" }),
+                Closure(loaded: ["Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022"]));
+
+            Assert.Equal(0, result.FailureCount);
+            Assert.False(d.CountedAsReal);
+            Assert.Equal(ClassificationPath.SignatureBlockerOutsideBHoM, d.Path);
+            Assert.Equal("RevitAPI", d.Cause);
+        }
+
+        [Theory]
+        [InlineData("Revit_Core_Engine_2024", "Revit_Core_Engine")]
+        [InlineData("Revit_Core_Engine", "Revit_Core_Engine")]
+        [InlineData("Structure_oM", "Structure_oM")]
+        // Documents a known limitation: the heuristic cannot tell a Revit release year from
+        // any other four-digit 20xx suffix. No such assembly exists in the fleet today
+        // (measured: 295 of 640 match, all prefixed Revit), but nothing enforces that.
+        [InlineData("Eurocode_2004", "Eurocode")]
+        [InlineData("Foo_1999", "Foo_1999")]
+        public void StripConfigSuffix_CollapsesOnlyA20xxTail(string input, string expected)
+            => Assert.Equal(expected, RunCommand.StripConfigSuffix(input));
+
+        // Documents current behaviour and a residual risk: the declaring assembly is taken
+        // from the FIRST parsable Method event, so a finding carrying events for several
+        // assemblies is decided by the first. A present assembly later in the list does not
+        // stop the finding being excused.
+        [Fact]
+        public void MultipleMethodEvents_TheFirstNamedAssemblyDecides()
+        {
+            var (result, d) = Run(
+                Tree("BH.Revit.Engine.Core.Compute.TryGetValueFromSource", ModelQaEvent, SubjectAsmEvent),
+                Answered("Revit_Core_Engine_2022"),
+                Closure(loaded: ["Revit_Core_Engine_2022"], subject: ["Revit_Core_Engine_2022"]));
+
+            Assert.Equal("Revit_ModelQA_Engine_2022", d.DeclaringAssembly);
+            Assert.Equal(0, result.FailureCount);
+            Assert.Equal(ClassificationPath.ForeignDeclaringAssembly, d.Path);
+        }
+    }
+}

tools/VersioningRunner/src/VersioningRunner/Commands/RunCommand.cs

@@ -58,6 +58,29 @@ public static int Execute(
         // comes back as CustomObject or null, so measured on a real PR that filter
         // attributed 1056 of 1056 failures to a repo that owned none of them.
         // Attribute to the exact namespaces the subject repo's own assemblies declare.
+        //
+        // What this run built, which the classifier needs to read a missing declaring
+        // assembly correctly. Null when no subject build dir was supplied: with whole-closure
+        // attribution there is no basis for calling any assembly foreign, so the
+        // reclassification is disabled and the fail-safe default stands.
+        ClosureContext? closure = null;
+        if (subjectBuildDir is not null && Directory.Exists(subjectBuildDir))
+        {
+            var loadedNames = new HashSet<string>(
+                loaded.Select(a => { try { return a.GetName().Name; } catch { return null; } })
+                      .Where(n => !string.IsNullOrEmpty(n))!,
+                StringComparer.Ordinal);
+            var subjectBases = new HashSet<string>(
+                Directory.GetFiles(subjectBuildDir, "*.dll", SearchOption.AllDirectories)
+                         .Select(f => StripConfigSuffix(Path.GetFileNameWithoutExtension(f))),
+                StringComparer.Ordinal);
+            if (subjectBases.Count > 0)
+                closure = new ClosureContext(
+                    loadedNames,
+                    new HashSet<string>(loadedNames.Select(StripConfigSuffix), StringComparer.Ordinal),
+                    subjectBases);
+        }
+
         var subjectNamespaces = BuildSubjectNamespaces(loaded, subjectBuildDir);
         Func<string, bool> isAttributable;
         if (subjectNamespaces is null)
@@ -122,7 +145,7 @@ public static int Execute(
                 return 1;
             }
 
-            var partial = ExtractFilteredResult(rawResult, isAttributable, unresolvableSkips, probeSignature, diagnostics);
+            var partial = ExtractFilteredResult(rawResult, isAttributable, unresolvableSkips, probeSignature, diagnostics, closure);
             allFailures.AddRange(partial.Failures);
         }
 
@@ -208,7 +231,11 @@ public static int Execute(
         if (configuration is not null)
             Console.WriteLine($"Configuration: {configuration}");
 
-        int ambiguous = diagnostics.Count(d => d.DeclaringTypeCandidates is { Count: > 1 });
+        // Counted over real findings only, because the per-finding note below is printed from
+        // result.Failures. Counting every diagnostic made the total disagree with the detail as
+        // soon as a finding could be reclassified to unverified: the warning claimed N ambiguous
+        // findings while fewer than N were listed.
+        int ambiguous = diagnostics.Count(d => d.CountedAsReal && d.DeclaringTypeCandidates is { Count: > 1 });
         if (ambiguous > 0)
             Console.Error.WriteLine(
                 $"::warning title=Versioning::{ambiguous} finding(s) have a declaring type present in more than one loaded assembly, " +
@@ -369,7 +396,8 @@ public static VersioningResult ExtractFilteredResult(object? rawResult, List<Ass
     public static VersioningResult ExtractFilteredResult(
         object? rawResult, Func<string, bool> isAttributable, List<UnverifiedFailure>? unresolvableSkips = null,
         Func<string, string, string?, (string? Cause, ClassificationPath Path, IReadOnlyList<string> Candidates)>? probeSignature = null,
-        List<FailureDiagnostic>? diagnostics = null)
+        List<FailureDiagnostic>? diagnostics = null,
+        ClosureContext? closure = null)
     {
         if (rawResult is null)
             return new VersioningResult
@@ -380,7 +408,7 @@ public static VersioningResult ExtractFilteredResult(
             };
 
         var failures = new List<FailureInfo>();
-        CollectLeafFailures(rawResult, isAttributable, failures, unresolvableSkips, probeSignature, diagnostics, depth: 0);
+        CollectLeafFailures(rawResult, isAttributable, failures, unresolvableSkips, probeSignature, diagnostics, closure, depth: 0);
 
         var status = failures.Count > 0 ? VersioningStatus.Error : VersioningStatus.Pass;
         return new VersioningResult
@@ -396,7 +424,7 @@ private static void CollectLeafFailures(
         object node, Func<string, bool> isAttributable, List<FailureInfo> failures,
         List<UnverifiedFailure>? unresolvableSkips,
         Func<string, string, string?, (string? Cause, ClassificationPath Path, IReadOnlyList<string> Candidates)>? probeSignature,
-        List<FailureDiagnostic>? diagnostics, int depth)
+        List<FailureDiagnostic>? diagnostics, ClosureContext? closure, int depth)
     {
         // BHoM's TestResult tree has at most 3 levels under the root (outer → per-version
         // summary → individual type result). Depth 5 gives headroom for unexpected nesting
@@ -469,6 +497,39 @@ private static void CollectLeafFailures(
                     path = ClassificationPath.ProbeNotSupplied;
             }
 
+            // Reclassify a finding whose recorded declaring assembly is not in
+            // this closure.
+            //
+            // candidates.Count > 0 is load-bearing and is the difference from v1. It means
+            // some OTHER loaded assembly answered for the type, so the probe verdict above
+            // describes code we were never asked about. When nothing answered, the path is
+            // DeclaringTypeNotLoaded, which is the signal that the type is genuinely gone;
+            // reclassifying that would convert a real removal into a silent pass.
+            if (cause is null && closure is not null && declaringAssembly is not null
+                && candidates.Count > 0
+                && !closure.LoadedNames.Contains(declaringAssembly))
+            {
+                string baseName = StripConfigSuffix(declaringAssembly);
+                if (!closure.SubjectBaseNames.Contains(baseName))
+                {
+                    // Nothing this repository builds under any configuration, and something
+                    // else answered for the type, so the entry is another repository's.
+                    cause = $"{declaringAssembly} (declaring assembly is not part of this repository)";
+                    path = ClassificationPath.ForeignDeclaringAssembly;
+                }
+                else if (closure.LoadedBaseNames.Contains(baseName))
+                {
+                    // Ours, and a sibling configuration of the same family is loaded, so the
+                    // family exists and only this configuration was not compiled.
+                    cause = $"{declaringAssembly} (build configuration not compiled in this run)";
+                    path = ClassificationPath.ConfigurationNotBuilt;
+                }
+                // Otherwise the family is ours but no configuration of it is loaded at all.
+                // "Not compiled" and "removed outright" are indistinguishable there, so the
+                // finding is left real. Ordering matters: folding this into the condition
+                // above makes the branch unreachable, which is how v1 lost it.
+            }
+
             if (cause is not null)
                 unresolvableSkips?.Add(new UnverifiedFailure(label, cause));
             else
@@ -488,7 +549,7 @@ private static void CollectLeafFailures(
             {
                 string childStatus = child.GetType().GetProperty("Status")?.GetValue(child)?.ToString() ?? "Pass";
                 if (childStatus is "Error" or "Warning")
-                    CollectLeafFailures(child, isAttributable, failures, unresolvableSkips, probeSignature, diagnostics, depth + 1);
+                    CollectLeafFailures(child, isAttributable, failures, unresolvableSkips, probeSignature, diagnostics, closure, depth + 1);
             }
         }
     }
@@ -734,6 +795,13 @@ public static (string? DeclaringType, string? MethodName) ParseMethodEvent(strin
     // reached through two helper layers that take no context parameter, and threading one
     // through both for two constant values would be a wider change than the values justify.
     private static string? s_configuration;
+    // Collapses a build-configuration suffix to the family name. Anchored and
+    // restricted to 20xx because that is the only config-variant convention in the fleet
+    // today: measured 295 of 640 assemblies match, all Revit, all with a sibling variant.
+    // It is a naming heuristic over an unenforced convention, not a declared relationship.
+    internal static string StripConfigSuffix(string assemblyName)
+        => Regex.Replace(assemblyName, @"_20\d{2}$", string.Empty);
+
     private static HashSet<string>? s_versionConditional;
     private static int s_subjectAssemblyCount;
     private static int s_subjectTypeCount;

tools/VersioningRunner/src/VersioningRunner/Models/VersioningResult.cs

@@ -38,8 +38,26 @@ public enum ClassificationPath
     SignatureBlockerInsideBHoM,
     // No probe was supplied by the caller, which happens only in unit tests.
     ProbeNotSupplied,
+    // The dataset record names a declaring assembly that is not in this
+    // closure, and the type was resolved from a different assembly instead. The entry
+    // describes another repository's code, so no verdict on it is available here.
+    ForeignDeclaringAssembly,
+    // As above, but the absent assembly is a build-configuration variant of
+    // one the subject did build (Revit_Core_Engine_2024 against a Release/2022 build), so
+    // the code exists in the repo and simply was not compiled in this run.
+    ConfigurationNotBuilt,
 }
 
+// What this run actually built, needed to tell "the recorded declaring
+// assembly is missing because it is someone else's" from "because we did not compile that
+// configuration" from "because it was genuinely removed". Passed explicitly rather than
+// held in static state: the runner is a single-shot process but the tests are not, and
+// static state cannot be arranged per-case.
+public sealed record ClosureContext(
+    IReadOnlySet<string> LoadedNames,
+    IReadOnlySet<string> LoadedBaseNames,
+    IReadOnlySet<string> SubjectBaseNames);
+
 // Whether the failing method's signature is version-conditional in the subject's source.
 //
 // Three states, deliberately. An empty grep result is not the same as "not