Skip to content

fix(ci-compliance): survive a file the engine throws on #28

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sakanni merged 3 commits into from
Oct 16, 2026
Merged

fix(ci-compliance): survive a file the engine throws on #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b4b171
fix(ci-compliance): survive a file the engine throws on
sakanni Oct 15, 2026
bc84116
test(ci-compliance): pin the exit code to the failure count alone
sakanni Oct 15, 2026
a166294
style(ci-compliance): drop em dashes from the comments touched by the…
sakanni Oct 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
228 changes: 135 additions & 93 deletions tools/ComplianceRunner/src/ComplianceRunner/ComplianceRunner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,108 +54,127 @@ static int Main(string[] args)
continue;
}

TestResult resultForThisFile;

if (checkType == "project")
// One file must not take the run down with it. A rule that throws used to escape
// Main, which killed the process before OutputEmitter ran: the check went 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 were 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.
try
{
if (file.EndsWith(".csproj", StringComparison.OrdinalIgnoreCase))
resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.CheckProjectFile(file, orgUrl);
else
resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.CheckAssemblyInfo(file, orgUrl);

// Remap absolute location paths back to the relative file path so
// annotations point to the correct diff line — mirrors BHoMBot ProjectCompliance.cs.
// Note: .OfType<Error>() silently drops any ITestInformation that is not exactly
// BH.oM.Test.CodeCompliance.Error. Currently safe — the project engine only emits
// Error-typed findings — but revisit if new finding sub-types are introduced.
if (resultForThisFile?.Information != null)
TestResult resultForThisFile;

if (checkType == "project")
{
resultForThisFile.Information = resultForThisFile.Information
.OfType<BH.oM.Test.CodeCompliance.Error>()
.Select(e => (ITestInformation)new BH.oM.Test.CodeCompliance.Error
{
Status = e.Status,
Message = e.Message,
Name = e.Name,
BHoM_Guid = e.BHoM_Guid,
UTCTime = e.UTCTime,
DocumentationLink = e.DocumentationLink,
Location = new BH.oM.Test.CodeCompliance.Location
if (file.EndsWith(".csproj", StringComparison.OrdinalIgnoreCase))
resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.CheckProjectFile(file, orgUrl);
else
resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.CheckAssemblyInfo(file, orgUrl);

// Remap absolute location paths back to the relative file path so
// annotations point to the correct diff line, mirroring BHoMBot ProjectCompliance.cs.
// Note: .OfType<Error>() silently drops any ITestInformation that is not exactly
// BH.oM.Test.CodeCompliance.Error. Currently safe, because the project engine only emits
// Error-typed findings, but revisit if new finding sub-types are introduced.
if (resultForThisFile?.Information != null)
{
resultForThisFile.Information = resultForThisFile.Information
.OfType<BH.oM.Test.CodeCompliance.Error>()
.Select(e => (ITestInformation)new BH.oM.Test.CodeCompliance.Error
{
FilePath = file,
Line = e.Location?.Line
}
})
.ToList();
Status = e.Status,
Message = e.Message,
Name = e.Name,
BHoM_Guid = e.BHoM_Guid,
UTCTime = e.UTCTime,
DocumentationLink = e.DocumentationLink,
Location = new BH.oM.Test.CodeCompliance.Location
{
FilePath = file,
Line = e.Location?.Line
}
})
.ToList();
}
}
}
else
{
// Test_Toolkit's [Path(@"..._Engine\\..")] rules match a literal backslash;
// flip git's forward-slash paths or every rule silently no-ops.
resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.RunChecks(file.Replace('/', '\\'), checkType);

// Group findings one-per-line and remap the path to the diff-relative file,
// mirroring BHoMBot's GroupInformation -> GroupErrors pipeline (code/copyright/
// documentation only; project has its own remap above). GroupErrors concatenates
// each finding's FullMessage() into the grouped Error.Message, so downstream these
// annotations are built from that message directly (see the preformatted branch in
// AnnotationConvert) rather than via IFullMessage, which would append the
// documentation suffix a second time.
if (resultForThisFile?.Information != null)
else
{
var errors = resultForThisFile.Information
.OfType<BH.oM.Test.CodeCompliance.Error>()
.ToList();
resultForThisFile.Information = BH.Engine.Test.CodeCompliance.Modify
.GroupErrors(errors, file)
.Select(e => (ITestInformation)e)
.ToList();
// Test_Toolkit's [Path(@"..._Engine\\..")] rules match a literal backslash;
// flip git's forward-slash paths or every rule silently no-ops.
resultForThisFile = BH.Engine.Test.CodeCompliance.Compute.RunChecks(file.Replace('/', '\\'), checkType);

// Group findings one-per-line and remap the path to the diff-relative file,
// mirroring BHoMBot's GroupInformation -> GroupErrors pipeline (code/copyright/
// documentation only; project has its own remap above). GroupErrors concatenates
// each finding's FullMessage() into the grouped Error.Message, so downstream these
// annotations are built from that message directly (see the preformatted branch in
// AnnotationConvert) rather than via IFullMessage, which would append the
// documentation suffix a second time.
if (resultForThisFile?.Information != null)
{
var errors = resultForThisFile.Information
.OfType<BH.oM.Test.CodeCompliance.Error>()
.ToList();
resultForThisFile.Information = BH.Engine.Test.CodeCompliance.Modify
.GroupErrors(errors, file)
.Select(e => (ITestInformation)e)
.ToList();
}
}
}

if (resultForThisFile == null)
{
accounting.CountNoResult();
Console.WriteLine($" [SKIP] No result returned for: {file}");
continue;
}
if (resultForThisFile == null)
{
accounting.CountNoResult();
Console.WriteLine($" [SKIP] No result returned for: {file}");
continue;
}

if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}");

accounting.CountExamined();
mergedResult = mergedResult.Merge(resultForThisFile);

// Code/copyright/documentation findings have been through GroupErrors, whose
// Error.Message is already the concatenated FullMessage(); take it verbatim.
//
// Project findings use IFullMessage, which appends the " - For more information
// see <url>" documentation suffix. This is a deliberate, documented divergence from
// BHoMBot: BHoMBot's ProjectCompliance emitted the bare Error.Message with no suffix,
// even though it carried the DocumentationLink. Every other compliance check surfaces
// that link (via GroupErrors -> FullMessage), and the project engine attaches a
// DocumentationLink to every finding specifically so it can be shown, so BHoMBot's
// omission was an inconsistency. Keeping the suffix makes all five checks uniform and
// points authors at the remediation guidance. Nothing downstream parses this text
// (pass/fail is status-driven; SARIF carries the link separately).
bool preformatted = checkType != "project";
var information = resultForThisFile.Information ?? Enumerable.Empty<ITestInformation>();
var perFileAnnotations = information.Select(i => i.ToAnnotationEquivalent(preformatted)).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)
if (verbose) Console.WriteLine($" Result Status: {resultForThisFile.Status}");

accounting.CountExamined();
mergedResult = mergedResult.Merge(resultForThisFile);

// Code/copyright/documentation findings have been through GroupErrors, whose
// Error.Message is already the concatenated FullMessage(); take it verbatim.
//
// Project findings use IFullMessage, which appends the " - For more information
// see <url>" documentation suffix. This is a deliberate, documented divergence from
// BHoMBot: BHoMBot's ProjectCompliance emitted the bare Error.Message with no suffix,
// even though it carried the DocumentationLink. Every other compliance check surfaces
// that link (via GroupErrors -> FullMessage), and the project engine attaches a
// DocumentationLink to every finding specifically so it can be shown, so BHoMBot's
// omission was an inconsistency. Keeping the suffix makes all five checks uniform and
// points authors at the remediation guidance. Nothing downstream parses this text
// (pass/fail is status-driven; SARIF carries the link separately).
bool preformatted = checkType != "project";
var information = resultForThisFile.Information ?? Enumerable.Empty<ITestInformation>();
var perFileAnnotations = information.Select(i => i.ToAnnotationEquivalent(preformatted)).ToList();
var infoList = information.ToList();

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]);
}
allAnnotations.Add(a);
}
allAnnotations.Add(a);
}
catch (Exception ex)
{
accounting.CountFailed();
allAnnotations.Add(FailureAnnotation(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());
}
}

Expand All @@ -164,7 +183,30 @@ 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 code, 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;
}

/// <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,
};
}
25 changes: 20 additions & 5 deletions tools/ComplianceRunner/src/Shared/Compliance.Shared/FileAccounting.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@
/// Counts what a compliance runner did with the files it was handed, and renders that as a
/// coverage denominator.
///
/// Both runners walk the same four-exit loop: a file is dropped by the filter, dropped because
/// it is not on disk, dropped because the engine returned no result, or examined. Without a
/// count of those, a run that examined every file and a run that examined none produce the same
/// output, so a green check carries no evidence that anything was checked.
/// Both runners walk the same five-exit loop: a file is dropped by the filter, dropped because
/// it is not on disk, dropped because the engine returned no result, examined, or failed because
/// the engine threw on it. Without a count of those, a run that examined every file and a run
/// that examined none produce the same output, so a green check carries no evidence that
/// anything was checked.
///
/// Deliberately free of BHoM types and of any dependency on either runner, so it can be
/// compiled directly into the hermetic test project and its arithmetic and wording tested
Expand Down Expand Up @@ -37,19 +38,31 @@ public sealed class FileAccounting
/// <summary>Files the runner actually examined and merged into its verdict.</summary>
public int Examined { get; private set; }

/// <summary>
/// Files the engine threw on. Counted apart from the other drops because it is the only one
/// that means the check could not answer, rather than that it had nothing to answer about.
/// </summary>
public int Failed { get; private set; }

public FileAccounting(int handedIn) => HandedIn = handedIn;

public void CountNotRelevant() => NotRelevant++;
public void CountNotOnDisk() => NotOnDisk++;
public void CountNoResult() => NoResult++;
public void CountExamined() => Examined++;
public void CountFailed() => Failed++;

/// <summary>
/// True when files were supplied and none of them was examined. The calling workflow exits
/// before invoking a runner when it has no files at all, so this is the anomalous case
/// rather than the empty one: something selected these files and the runner used none.
///
/// A run with failures is excluded deliberately. This flag exists to catch a check that
/// reports success having inspected nothing, and its warning says exactly that. A run that
/// failed on every file is already red and already carries an annotation per file, so
/// raising it here would contradict the exit code.
/// </summary>
public bool ExaminedNothing => HandedIn > 0 && Examined == 0;
public bool ExaminedNothing => HandedIn > 0 && Examined == 0 && Failed == 0;

/// <summary>
/// The coverage line. Leads with what was examined rather than with what was handed in:
Expand All @@ -75,6 +88,7 @@ private string DropSummary()
if (NotRelevant > 0) parts.Add($"{NotRelevant} not relevant to this check");
if (NotOnDisk > 0) parts.Add($"{NotOnDisk} not found on disk");
if (NoResult > 0) parts.Add($"{NoResult} returned no result");
if (Failed > 0) parts.Add($"{Failed} could not be analysed");
return parts.Count == 0 ? "none dropped" : string.Join(", ", parts);
}

Expand All @@ -86,5 +100,6 @@ private string DropSummary()
["notRelevant"] = NotRelevant,
["notOnDisk"] = NotOnDisk,
["noResult"] = NoResult,
["failed"] = Failed,
};
}
Loading
Loading