fix(ci-compliance): survive a dataset file the engine throws on, and say so on a skip

.github/actions/ci-compliance/action.yml

@@ -52,6 +52,37 @@ runs:
         patterns: ${{ inputs.patterns }}
         output_file: changed_files.txt
 
+    # A skipped job writes nothing to the summary, so a green with no files to check and a
+    # green that checked everything read the same there. The runner's coverage line solves
+    # this only once the runner runs, and on a skip it never does. Says which pathspec was
+    # searched, because the one way a skip is wrong is the pathspec missing a file that did
+    # change, and that is not diagnosable from "skipped" alone.
+    - name: Report the skip
+      if: steps.changed.outputs.count == '0'
+      shell: bash
+      env:
+        CHECK_TYPE: ${{ inputs.check_type }}
+        PATTERNS:   ${{ inputs.patterns }}
+        REASON:     ${{ steps.changed.outputs.reason }}
+      run: |
+        set -euo pipefail
+
+        if [ "$REASON" = "not-a-pull-request" ]; then
+          detail="This run was not triggered by a pull request, so there was no diff to examine."
+        else
+          detail="No files matching \`${PATTERNS}\` changed in this pull request, so no file was examined."
+        fi
+
+        echo "::notice title=${CHECK_TYPE} compliance::Skipped. ${detail}"
+
+        if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then
+          {
+            echo "### ${CHECK_TYPE} compliance skipped"
+            echo ""
+            echo "${detail}"
+          } >> "$GITHUB_STEP_SUMMARY"
+        fi
+
     - name: Mint dependency token
       id: dep-token
       if: steps.changed.outputs.count != '0'

.github/actions/compute-changed-files/action.yml

@@ -31,6 +31,14 @@ outputs:
       '0' signals callers to skip; any other value signals proceed. May
       exceed the output_file line count, which excludes deletions.
     value: ${{ steps.compute.outputs.count }}
+  reason:
+    description: >
+      Why the count came out as it did: 'files-changed', 'no-matching-files'
+      or 'not-a-pull-request'. A caller reporting its own skip needs this to
+      say which one happened; the count alone collapses the last two, and a
+      summary line claiming no files changed when the run was never a pull
+      request would be wrong.
+    value: ${{ steps.compute.outputs.reason }}
 
 runs:
   using: composite
@@ -58,6 +66,7 @@ runs:
         # failure mode for this action, because every consumer treats count=0
         # as "nothing to do".
         if [ -z "$BASE_SHA" ]; then
+          echo "reason=not-a-pull-request" >> "$GITHUB_OUTPUT"
           if [ "$SKIP_IF_NOT_PR" = "true" ]; then
             echo "count=0" >> "$GITHUB_OUTPUT"
             echo "::notice::Not triggered from a pull request. Check skipped."
@@ -127,7 +136,9 @@ runs:
         echo "count=$count" >> "$GITHUB_OUTPUT"
 
         if [ "$count" -gt 0 ]; then
+          echo "reason=files-changed" >> "$GITHUB_OUTPUT"
           echo "::notice::$count relevant file(s) changed."
         else
+          echo "reason=no-matching-files" >> "$GITHUB_OUTPUT"
           echo "::notice::No relevant files changed. Check skipped."
         fi

.github/scripts/tests/ci-compliance-action.Tests.ps1

@@ -17,6 +17,7 @@ BeforeAll {
     $script:actionPath = Join-Path $repoRoot '.github/actions/ci-compliance/action.yml'
     $script:lines = Get-Content $actionPath
     $script:text  = $lines -join "`n"
+    $script:changedFiles = Get-Content (Join-Path $repoRoot '.github/actions/compute-changed-files/action.yml') -Raw
 }
 
 Describe 'ci-compliance action.yml' {
@@ -40,4 +41,40 @@ Describe 'ci-compliance action.yml' {
             $text | Should -Match "match '\^Coverage: '"
         }
     }
+
+    Context 'a self-skip says so' {
+
+        # The coverage line only exists once the runner runs. On a skip it never does, so
+        # without this the summary is empty and a green that had nothing to check reads the
+        # same as a green that checked everything.
+        It 'writes to the summary when the check is skipped' {
+            $text | Should -Match 'compliance skipped'
+        }
+
+        It 'names the pathspec it searched' {
+            $text | Should -Match 'PATTERNS' -Because 'the one way a skip is wrong is the pathspec missing a file that did change, and that is not diagnosable from "skipped" alone'
+        }
+
+        # Two different states both produce count=0. Saying "no files changed" when the run was
+        # never a pull request would be a wrong statement, not just a vague one.
+        It 'distinguishes an empty diff from a run that was never a pull request' {
+            $text | Should -Match 'not-a-pull-request'
+        }
+
+        It 'takes that reason from the action that knows it' {
+            $text | Should -Match 'steps\.changed\.outputs\.reason' -Because 'compute-changed-files is the only place the two cases are told apart'
+        }
+
+        # The other end of that contract. A consumer reading an output no producer sets gets an
+        # empty string and falls through to the wrong branch, silently.
+        It 'is an output compute-changed-files actually declares' {
+            $changedFiles | Should -Match '(?m)^\s{2}reason:'
+        }
+
+        It 'sets a reason on every path out of compute-changed-files' {
+            foreach ($r in 'not-a-pull-request', 'no-matching-files', 'files-changed') {
+                $changedFiles | Should -Match "reason=$r"
+            }
+        }
+    }
 }

tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs

@@ -169,7 +169,7 @@ static int Main(string[] args)
             catch (Exception ex)
             {
                 accounting.CountFailed();
-                allAnnotations.Add(FailureAnnotation(file, ex));
+                allAnnotations.Add(Annotation.ForUnanalysableFile(file, ex));
 
                 // Stack trace to stderr, not into the annotation. GitHub truncates a long
                 // annotation message, and the one thing the author needs is the file name.
@@ -188,25 +188,4 @@ static int Main(string[] args)
         // green check having examined less than it was asked to.
         return mergedResult.Status == TestStatus.Error || accounting.Failed > 0 ? 1 : 0;
     }
-
-    /// <summary>
-    /// The annotation for a file the engine threw on. Carries the exception type and message so
-    /// the author can tell a malformed file from an environment problem, and says plainly that
-    /// this file was not checked, because a failure annotation that looks like a finding would
-    /// send them looking for a defect in their own code.
-    /// </summary>
-    static Annotation FailureAnnotation(string file, Exception ex) => new Annotation
-    {
-        FilePath    = file,
-        LineStart   = 1,
-        LineEnd     = 1,
-        ColumnStart = 1,
-        ColumnEnd   = 1,
-        Level       = "failure",
-        RuleName    = "FileCouldNotBeAnalysed",
-        Message     = $"This file could not be analysed and was not checked. "
-                    + $"{ex.GetType().Name}: {ex.Message} "
-                    + $"Every other file in this run was checked normally.",
-        UTCTime     = DateTime.UtcNow,
-    };
 }

tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs

@@ -53,46 +53,71 @@ static int Main(string[] args)
                 continue;
             }
 
-            var resultForThisFile = file.IsValidDataset();
-
-            if (resultForThisFile == null)
+            // One file must not take the run down with it, for the same reason it must not in
+            // ComplianceRunner: an escaping exception kills the process before OutputEmitter
+            // runs, so the check goes red with no annotation, no coverage line, and no name for
+            // the file that caused it, and the findings already gathered for every other file
+            // are lost. The run still fails below on accounting.Failed, so this converts a
+            // silent abort into a stated one rather than making the check more permissive.
+            //
+            // IsValidDataset opens the file with an unguarded StreamReader before it parses
+            // anything, so the throw does not need a malformed dataset to reach it. Reproduced
+            // by holding the file open with no sharing, which is what an antivirus scan or a
+            // concurrent step does.
+            try
             {
-                accounting.CountNoResult();
-                Console.WriteLine($"  [SKIP] No result returned for: {file}");
-                continue;
-            }
+                var resultForThisFile = file.IsValidDataset();
+
+                if (resultForThisFile == null)
+                {
+                    accounting.CountNoResult();
+                    Console.WriteLine($"  [SKIP] No result returned for: {file}");
+                    continue;
+                }
 
-            if (verbose) Console.WriteLine($"  Result Status: {resultForThisFile.Status}");
+                if (verbose) Console.WriteLine($"  Result Status: {resultForThisFile.Status}");
 
-            accounting.CountExamined();
-            mergedResult = mergedResult.Merge(resultForThisFile);
+                accounting.CountExamined();
+                mergedResult = mergedResult.Merge(resultForThisFile);
 
-            var information        = resultForThisFile.Information ?? Enumerable.Empty<ITestInformation>();
-            var perFileAnnotations = information
-                .Select(i => i.ToAnnotationEquivalent())
-                .ToList();
-            var infoList = information.ToList();
+                var information        = resultForThisFile.Information ?? Enumerable.Empty<ITestInformation>();
+                var perFileAnnotations = information
+                    .Select(i => i.ToAnnotationEquivalent())
+                    .ToList();
+                var infoList = information.ToList();
 
-            for (int i = 0; i < perFileAnnotations.Count; i++)
-            {
-                var a           = perFileAnnotations[i];
-                var displayPath = string.IsNullOrEmpty(a.FilePath) ? file : a.FilePath;
-                if (verbose)
+                for (int i = 0; i < perFileAnnotations.Count; i++)
                 {
-                    Console.WriteLine($"  - [{a.Level}] {displayPath}:{a.LineStart}:{a.ColumnStart}" +
-                                      $"-{a.LineEnd}:{a.ColumnEnd} [{a.RuleName}]");
-                    Console.WriteLine($"    {a.Message}");
-                    if (i < infoList.Count)
-                        AnnotationConvert.LogDetailedFinding(infoList[i]);
+                    var a           = perFileAnnotations[i];
+                    var displayPath = string.IsNullOrEmpty(a.FilePath) ? file : a.FilePath;
+                    if (verbose)
+                    {
+                        Console.WriteLine($"  - [{a.Level}] {displayPath}:{a.LineStart}:{a.ColumnStart}" +
+                                          $"-{a.LineEnd}:{a.ColumnEnd} [{a.RuleName}]");
+                        Console.WriteLine($"    {a.Message}");
+                        if (i < infoList.Count)
+                            AnnotationConvert.LogDetailedFinding(infoList[i]);
+                    }
+                    // Dataset checks don't populate Location.FilePath, so anchor to the JSON file
+                    // so GitHub can produce an inline PR annotation instead of falling back to
+                    // "unknown".
+                    if (string.IsNullOrEmpty(a.FilePath))
+                        a.FilePath = file;
+                    if (a.LineStart <= 0)
+                        a.LineStart = 1;
+
+                    allAnnotations.Add(a);
                 }
-                // Dataset checks don't populate Location.FilePath — anchor to the JSON file so
-                // GitHub can produce an inline PR annotation instead of falling back to "unknown".
-                if (string.IsNullOrEmpty(a.FilePath))
-                    a.FilePath = file;
-                if (a.LineStart <= 0)
-                    a.LineStart = 1;
-
-                allAnnotations.Add(a);
+            }
+            catch (Exception ex)
+            {
+                accounting.CountFailed();
+                allAnnotations.Add(Annotation.ForUnanalysableFile(file, ex));
+
+                // Stack trace to stderr, not into the annotation. GitHub truncates a long
+                // annotation message, and the one thing the author needs is the file name.
+                Console.Error.WriteLine($"  [FAIL] {file}: {ex.GetType().Name}: {ex.Message}");
+                Console.Error.WriteLine(ex.ToString());
             }
         }
 
@@ -102,7 +127,10 @@ static int Main(string[] args)
             accounting);
 
         // Exit code mirrors BHoMBot: failure only on Error; Warning and Pass are both success.
-        return mergedResult.Status == TestStatus.Error ? 1 : 0;
+        // A file the engine threw on also fails the run. It is not a finding about the dataset,
+        // so it does not reach mergedResult, and without this a crash would exit 0 and report a
+        // green check having examined less than it was asked to.
+        return mergedResult.Status == TestStatus.Error || accounting.Failed > 0 ? 1 : 0;
     }
 
 }

tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs

@@ -23,4 +23,28 @@ public class Annotation
     public string   BHoMGuid         { get; set; } = "";
     /// <summary>UTC timestamp at which the engine produced this finding.</summary>
     public DateTime UTCTime          { get; set; }
+
+    /// <summary>
+    /// The annotation for a file the engine threw on. Carries the exception type and message so
+    /// the author can tell a malformed file from an environment problem, and says plainly that
+    /// this file was not checked, because a failure annotation that looks like a finding would
+    /// send them looking for a defect in their own code.
+    ///
+    /// Shared by both runners rather than written out in each, so a reword cannot land in one
+    /// and not the other. It names no check and no engine, which is what lets it be shared.
+    /// </summary>
+    public static Annotation ForUnanalysableFile(string file, Exception ex) => new Annotation
+    {
+        FilePath    = file,
+        LineStart   = 1,
+        LineEnd     = 1,
+        ColumnStart = 1,
+        ColumnEnd   = 1,
+        Level       = "failure",
+        RuleName    = "FileCouldNotBeAnalysed",
+        Message     = $"This file could not be analysed and was not checked. "
+                    + $"{ex.GetType().Name}: {ex.Message} "
+                    + $"Every other file in this run was checked normally.",
+        UTCTime     = DateTime.UtcNow,
+    };
 }

tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs

@@ -152,4 +152,98 @@ public void GitHubOutput_ForPassingRun_ProducesNoFindingAnnotations()
             Assert.That(stdout, Does.Contain("::warning title=Compliance coverage::"));
         });
     }
+
+    // ── A file the engine throws on ───────────────────────────────────────────
+
+    /// <summary>
+    /// Writes a dataset file and a sibling under a real Datasets directory, so both pass the
+    /// filter and reach the engine, and returns the pair with the directory to clean up.
+    ///
+    /// The sibling carries a genuine finding. Without it a test cannot tell "the run continued"
+    /// from "the run died quietly".
+    /// </summary>
+    private static (string Target, string Sibling, string Dir) WriteDatasetPair()
+    {
+        string dir = Path.Combine(Path.GetTempPath(), "bhom-ci-" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(Path.Combine(dir, "Datasets"));
+
+        string target = Path.Combine(dir, "Datasets", "Target.json");
+        File.WriteAllText(target, "{ \"_t\" : \"BH.oM.Data.Library.Dataset\" }");
+
+        string sibling = Path.Combine(dir, "Datasets", "Sibling.json");
+        File.WriteAllText(sibling, "not a dataset");
+
+        return (target, sibling, dir);
+    }
+
+    [Test]
+    [Category("RequiresBHoM")]
+    [Platform("Win", Reason = "FileShare.None only blocks another process on Windows; on Unix .NET's sharing mode is advisory, so the file would open and nothing would throw.")]
+    [Description("A dataset file the engine throws on fails the run, names itself, and does not stop the other files being reported.")]
+    public void UnanalysableFile_FailsTheRunAndIsNamed_WithoutLosingOtherFindings()
+    {
+        var (target, sibling, dir) = WriteDatasetPair();
+        try
+        {
+            // Held open with no sharing, which is what an antivirus scan or a concurrent step
+            // does. IsValidDataset opens the file with an unguarded StreamReader before it
+            // parses anything, so this reaches the throw without needing a malformed dataset,
+            // and the file stays a valid one so the test is not really about its content.
+            using (File.Open(target, FileMode.Open, FileAccess.Read, FileShare.None))
+            {
+                var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner",
+                    "--output", "github", target, sibling);
+
+                Assert.Multiple(() =>
+                {
+                    Assert.That(exitCode, Is.EqualTo(1), "a file the engine could not analyse must fail the run");
+                    Assert.That(stdout, Does.Contain("Target.json"),
+                        "the failing file must be named, or the author cannot act on the red");
+                    Assert.That(stdout, Does.Contain("could not be analysed"));
+                    Assert.That(stdout, Does.Contain("1 of 2 file(s) examined"),
+                        "the coverage line must survive, which it cannot if the process died");
+                    Assert.That(stdout, Does.Contain("did not deserialise"),
+                        "the other file's finding must still be reported; losing them was the defect");
+                });
+            }
+        }
+        finally
+        {
+            try { Directory.Delete(dir, recursive: true); } catch { /* temp dir, best effort */ }
+        }
+    }
+
+    [Test]
+    [Category("RequiresBHoM")]
+    [Platform("Win", Reason = "FileShare.None only blocks another process on Windows; on Unix .NET's sharing mode is advisory, so the file would open and nothing would throw.")]
+    [Description("A run whose only dataset could not be analysed still fails, on the failure alone.")]
+    public void UnanalysableFileAlone_StillFailsTheRun()
+    {
+        // Separate from the test above because that one cannot see this. Its sibling carries a
+        // real finding, so the run exits 1 on the merged Error whether or not failures count
+        // towards the exit code. With nothing else in the run the exit code here rests on the
+        // failure count and nothing else.
+        var (target, _, dir) = WriteDatasetPair();
+        try
+        {
+            using (File.Open(target, FileMode.Open, FileAccess.Read, FileShare.None))
+            {
+                var (exitCode, stdout) = RunnerFixture.Run("DatasetComplianceRunner",
+                    "--output", "github", target);
+
+                Assert.Multiple(() =>
+                {
+                    Assert.That(exitCode, Is.EqualTo(1),
+                        "a run that could analyse nothing must not report success");
+                    Assert.That(stdout, Does.Contain("0 of 1 file(s) examined"));
+                    Assert.That(stdout, Does.Not.Contain("reports success without having inspected anything"),
+                        "that warning is for a green that examined nothing, and this run is red");
+                });
+            }
+        }
+        finally
+        {
+            try { Directory.Delete(dir, recursive: true); } catch { /* temp dir, best effort */ }
+        }
+    }
 }