fix(transport): drop blocker false positives for stdlib + member access

The DetectBlockersAsync heuristic was catching every PascalCase
"Identifier(" or "Identifier." token in script bodies and treating it
as a candidate SharedScript or ExternalSystem reference. On a normal
template catalog this surfaced 30+ blocker rows for .NET stdlib
(DateTimeOffset, Convert, ToString, Dispose, UtcNow...), ScadaLink
runtime API roots (Notify, Database, ExternalSystem, Scripts...), and
SQL keywords inside string literals (COUNT), blocking the import.

Two surgical fixes:

1. Skip identifiers preceded by `.` so `obj.Method()` no longer flags
   `Method` as a top-level reference.
2. Maintain a `KnownNonReferenceNames` denylist for the small set of
   well-known stdlib / runtime / SQL tokens that can never be
   user-defined SharedScripts or ExternalSystems.

The documented use case -- a top-level free-standing call to a missing
SharedScript or ExternalSystem (e.g. `MissingHelper()` at the start of
an expression, or `ErpSystem.Call(...)` where ErpSystem is the
external-system identifier) -- still produces a blocker row, pinned
by the existing test plus a new noise-filter regression test.
This commit is contained in:
Joseph Doherty
2026-05-24 07:46:24 -04:00
parent 6299743a35
commit 6bdada7549
2 changed files with 90 additions and 6 deletions

View File

@@ -384,13 +384,16 @@ public sealed class BundleImporter : IBundleImporter
}
// For each candidate, only report it as a blocker if it looks like a
// resource reference (PascalCase, length > 1) AND it's not present
// anywhere we can satisfy it. We deliberately do not look at language
// keywords or stdlib helpers — the test surface only ever uses
// well-named identifiers.
// resource reference (PascalCase, length > 1), isn't a well-known
// language / runtime / SQL token, AND isn't present anywhere we can
// satisfy it. The denylist is the noise filter that keeps the
// heuristic usable on real script bodies — without it, every member
// access (`obj.ToString()`) and stdlib type (`DateTimeOffset`) gets
// flagged.
foreach (var candidate in referencedFromBundle.OrderBy(n => n, StringComparer.Ordinal))
{
if (!LooksLikeResourceName(candidate)) continue;
if (KnownNonReferenceNames.Contains(candidate)) continue;
var isShared = sharedScriptNames.Contains(candidate);
var isExternal = externalSystemNames.Contains(candidate);
if (isShared || isExternal) continue;
@@ -413,11 +416,13 @@ public sealed class BundleImporter : IBundleImporter
if (string.IsNullOrEmpty(body)) return;
// Find every "Identifier(" or "Identifier." occurrence. The boundary
// before the identifier must NOT be an identifier char so we don't
// match the trailing portion of a longer token.
// match the trailing portion of a longer token, AND must not be a
// dot — otherwise `obj.Method()` would flag `Method` as a top-level
// reference. Member-access trailing identifiers are skipped.
for (var i = 0; i < body.Length; i++)
{
if (!IsIdentifierStart(body[i])) continue;
if (i > 0 && IsIdentifierChar(body[i - 1])) continue;
if (i > 0 && (IsIdentifierChar(body[i - 1]) || body[i - 1] == '.')) continue;
var start = i;
while (i < body.Length && IsIdentifierChar(body[i])) i++;
if (i >= body.Length) break;
@@ -429,6 +434,36 @@ public sealed class BundleImporter : IBundleImporter
}
}
/// <summary>
/// Names that look like PascalCase references but are never user-defined
/// SharedScripts or ExternalSystems. Filters the false-positive noise the
/// identifier scan produces against real script bodies: .NET stdlib types
/// and helpers, ScadaLink runtime API roots, and common SQL keywords that
/// appear inside string literals. Match is case-sensitive (Ordinal).
/// </summary>
private static readonly HashSet<string> KnownNonReferenceNames = new(StringComparer.Ordinal)
{
// .NET / C# stdlib
"Boolean", "Byte", "Char", "Console", "Convert", "DateTime",
"DateTimeOffset", "Decimal", "Dispose", "Double", "Enumerable",
"Exception", "Guid", "Int16", "Int32", "Int64", "List", "Math",
"Now", "Object", "Single", "String", "Task", "TimeSpan", "ToBoolean",
"ToDateTime", "ToDecimal", "ToDouble", "ToInt16", "ToInt32", "ToInt64",
"ToList", "ToSingle", "ToString", "UtcNow",
// ScadaLink script runtime API roots and well-known members
"Attribute", "Attributes", "Call", "CallScript", "CallShared",
"Connection", "CreateCommand", "Database", "ExecuteAsync",
"ExecuteNonQueryAsync", "ExecuteReaderAsync", "ExecuteScalarAsync",
"ExternalSystem", "GetAsync", "GetAttribute", "Instance", "Notify",
"Request", "Response", "Route", "Scheduler", "Scripts", "Send",
"SetAsync", "SetAttribute",
// SQL keywords commonly seen inside string literals
"COUNT", "FROM", "GROUP", "INSERT", "JOIN", "ORDER", "SELECT",
"UPDATE", "WHERE",
};
private static bool LooksLikeResourceName(string name)
{
if (name.Length < 2) return false;

View File

@@ -250,4 +250,53 @@ public sealed class BundleImporterPreviewTests : IDisposable
Assert.DoesNotContain(preview.Items, i =>
i.Kind == ConflictKind.Blocker && i.Name == "HelperFn");
}
[Fact]
public async Task PreviewAsync_does_not_flag_stdlib_or_runtime_member_accesses_as_blockers()
{
// Arrange: a template script that uses a representative mix of stdlib
// calls, runtime-API roots, and member-access patterns. None of these
// are user-defined SharedScripts or ExternalSystems and the previous
// heuristic was flagging every one of them.
await using (var scope = _provider.CreateAsyncScope())
{
var ctx = scope.ServiceProvider.GetRequiredService<ScadaLinkDbContext>();
var t = new Template("Pump") { Description = "noise-check" };
t.Scripts.Add(new TemplateScript("init", """
var now = DateTimeOffset.UtcNow;
var s = Convert.ToString(123);
await Notify.Send("alerts", "msg");
var x = await Database.ExecuteScalarAsync("SELECT COUNT(*) FROM t");
var y = await ExternalSystem.Call("erp", "ping");
obj.Dispose();
"""));
ctx.Templates.Add(t);
await ctx.SaveChangesAsync();
}
var bundleStream = await ExportTemplatesAsync();
var bytes = await StreamToBytes(bundleStream);
// Act
ImportPreview preview;
await using (var scope = _provider.CreateAsyncScope())
{
var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
var session = await importer.LoadAsync(new MemoryStream(bytes), passphrase: null);
preview = await importer.PreviewAsync(session.SessionId);
}
// Assert: none of the well-known names produce blocker rows.
string[] noiseNames =
{
"DateTimeOffset", "UtcNow", "Convert", "ToString", "Notify", "Send",
"Database", "ExecuteScalarAsync", "COUNT", "ExternalSystem", "Call",
"Dispose",
};
foreach (var name in noiseNames)
{
Assert.DoesNotContain(preview.Items, i =>
i.Kind == ConflictKind.Blocker && i.Name == name);
}
}
}