From c6bda061e72848792ff6e990f12f6f754845fee4 Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Sun, 18 Oct 2026 18:33:28 +0100 Subject: [PATCH 1/2] fix(ci-compliance): survive a dataset file the engine throws on IsValidDataset opens the file with an unguarded StreamReader before it parses anything, so an exception does not need a malformed dataset to reach it. There was no catch around the per-file work, so the exception escaped Main and killed the process before OutputEmitter ran: no annotation, no coverage line, no name for the file that caused it, and the findings already gathered for every other file in the run were lost. Same shape as the ComplianceRunner catch, with the failure annotation moved to Compliance.Shared so one reword cannot land in one runner and not the other. The run still fails on accounting.Failed, so this states the abort rather than making the check more permissive. --- .../src/ComplianceRunner/ComplianceRunner.cs | 23 +---- .../DatasetComplianceRunner.cs | 96 ++++++++++++------- .../Shared/Compliance.Shared/Annotation.cs | 24 +++++ .../DatasetComplianceRunnerE2ETests.cs | 94 ++++++++++++++++++ .../Unit/AnnotationTests.cs | 83 ++++++++++++++++ 5 files changed, 264 insertions(+), 56 deletions(-) create mode 100644 tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/AnnotationTests.cs diff --git a/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs b/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs index 3ec8840..915e571 100644 --- a/tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs +++ b/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; } - - /// - /// 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. - /// - 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, - }; } diff --git a/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs b/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs index 87a2206..97db7fe 100644 --- a/tools/ComplianceRunner/src/DatasetComplianceRunner/DatasetComplianceRunner.cs +++ b/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(); - var perFileAnnotations = information - .Select(i => i.ToAnnotationEquivalent()) - .ToList(); - var infoList = information.ToList(); + var information = resultForThisFile.Information ?? Enumerable.Empty(); + 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; } } diff --git a/tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs b/tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs index 40004eb..e1e5a04 100644 --- a/tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs +++ b/tools/ComplianceRunner/src/Shared/Compliance.Shared/Annotation.cs @@ -23,4 +23,28 @@ public class Annotation public string BHoMGuid { get; set; } = ""; /// UTC timestamp at which the engine produced this finding. public DateTime UTCTime { get; set; } + + /// + /// 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. + /// + 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, + }; } diff --git a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs b/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs index c09eb42..a445f2f 100644 --- a/tools/ComplianceRunner/tests/Compliance.Tests/Integration/DatasetComplianceRunnerE2ETests.cs +++ b/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 ─────────────────────────────────────────── + + /// + /// 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". + /// + 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 */ } + } + } } diff --git a/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/AnnotationTests.cs b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/AnnotationTests.cs new file mode 100644 index 0000000..723c525 --- /dev/null +++ b/tools/ComplianceRunner/tests/Compliance.Unit.Tests/Unit/AnnotationTests.cs @@ -0,0 +1,83 @@ +using NUnit.Framework; +using System; +using System.IO; + +namespace Compliance.Tests +{ + /// + /// The annotation both runners emit for a file the engine threw on, tested without a BHoM + /// install. + /// + /// It was a private method on ComplianceRunner until DatasetComplianceRunner needed the same + /// thing. Moving it here rather than writing it out twice is what these tests hold in place: + /// the wording is the whole deliverable, it is the only thing an author sees when a file + /// cannot be checked, and two copies of it would drift. + /// + [TestFixture] + public class AnnotationTests + { + [Test] + public void ItNamesTheFileAndSaysTheFileWasNotChecked() + { + var a = Annotation.ForUnanalysableFile( + "Datasets/Regional.json", new IOException("The process cannot access the file.")); + + Assert.Multiple(() => + { + Assert.That(a.FilePath, Is.EqualTo("Datasets/Regional.json"), + "without the file name the author cannot act on the red"); + Assert.That(a.Message, Does.Contain("could not be analysed and was not checked")); + Assert.That(a.Message, Does.Contain("Every other file in this run was checked normally."), + "says the run continued, so the author does not assume the rest went unchecked"); + }); + } + + [Test] + public void ItCarriesTheExceptionTypeAndMessage() + { + // The distinction the author needs is between their file being malformed and the + // environment being wrong. The type alone makes that call: an IOException is not a + // defect in the dataset. + var a = Annotation.ForUnanalysableFile( + "Datasets/Regional.json", new IOException("used by another process")); + + Assert.Multiple(() => + { + Assert.That(a.Message, Does.Contain("IOException")); + Assert.That(a.Message, Does.Contain("used by another process")); + }); + } + + [Test] + public void ItIsAFailureAndNotAFinding() + { + // Level drives the GitHub annotation level, so a warning here would let a run that + // could not read a file look like one that read it and had a minor comment. + // RuleName is the SARIF ruleId and is deliberately not a check name: no check ran. + var a = Annotation.ForUnanalysableFile("Datasets/Regional.json", new Exception("x")); + + Assert.Multiple(() => + { + Assert.That(a.Level, Is.EqualTo("failure")); + Assert.That(a.RuleName, Is.EqualTo("FileCouldNotBeAnalysed")); + }); + } + + [Test] + public void ItAnchorsToLineOne() + { + // GitHub drops the "#L" suffix when LineStart is 0 and renders the annotation + // against the file rather than the diff. There is no meaningful line for a file that + // was never parsed, and line 1 is what puts it in the pull request's file view. + var a = Annotation.ForUnanalysableFile("Datasets/Regional.json", new Exception("x")); + + Assert.Multiple(() => + { + Assert.That(a.LineStart, Is.EqualTo(1)); + Assert.That(a.LineEnd, Is.EqualTo(1)); + Assert.That(a.ColumnStart, Is.EqualTo(1)); + Assert.That(a.ColumnEnd, Is.EqualTo(1)); + }); + } + } +} From 7b13989e51637988ccb0eef3fc2615c23e9f48b6 Mon Sep 17 00:00:00 2001 From: Seun Akanni Date: Sun, 18 Oct 2026 18:33:29 +0100 Subject: [PATCH 2/2] fix(ci-compliance): say which files a skipped compliance check looked for A self-skip is correct behaviour: the check reads only the files a pull request changed, so no dataset files changed means nothing to examine. The result did not say so. Every step after the diff is skipped, the runner never runs, and its coverage line is the only thing that reports what was examined, so the job summary was empty and a green with nothing to check read the same as a green that checked everything. Adds a summary line naming the pathspec that was searched, because the one way a skip is wrong is the pathspec missing a file that did change. compute-changed-files gains a reason output so the line can tell an empty diff from a run that was never a pull request, which the count alone collapses. --- .github/actions/ci-compliance/action.yml | 31 ++++++++++++++++ .../actions/compute-changed-files/action.yml | 11 ++++++ .../tests/ci-compliance-action.Tests.ps1 | 37 +++++++++++++++++++ 3 files changed, 79 insertions(+) diff --git a/.github/actions/ci-compliance/action.yml b/.github/actions/ci-compliance/action.yml index c4e15d6..8a2f81c 100644 --- a/.github/actions/ci-compliance/action.yml +++ b/.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' diff --git a/.github/actions/compute-changed-files/action.yml b/.github/actions/compute-changed-files/action.yml index 5725d54..df12423 100644 --- a/.github/actions/compute-changed-files/action.yml +++ b/.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 diff --git a/.github/scripts/tests/ci-compliance-action.Tests.ps1 b/.github/scripts/tests/ci-compliance-action.Tests.ps1 index 760efb9..f1a421b 100644 --- a/.github/scripts/tests/ci-compliance-action.Tests.ps1 +++ b/.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" + } + } + } }