dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(transport): blocker heuristic no longer hard-blocks valid imports — local declarations excluded, denylist extended, template-script findings downgraded to warnings

Browse Source 
#05-T19. Import name-resolution findings are now split by origin: template-script
(and attribute-default-expression) references become non-blocking advisory
warnings (ConflictKind.Warning + ImportResult.Warnings) since the design-time
deploy gate re-validates authoritatively; ApiMethod-script references stay hard
errors (SemanticValidationException / ConflictKind.Blocker) as they have no
downstream gate. Names declared locally in a body (Roslyn-parsed local functions
/ methods) are excluded so a script's own helper isn't flagged; KnownNonReferenceNames
extended with common BCL/LINQ/SQL surface. DetectBlockersAsync and
RunSemanticValidationAsync Pass 1 apply the identical split so preview and apply
agree. Rollback/hard-fail tests retargeted to ApiMethods (the still-hard-failing
vehicle). CentralUI preview renders the new Warning badge/advisory.

Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
This commit is contained in:
Joseph Doherty
2026-07-10 02:12:26 -04:00
parent 8e92198078
commit c60347c5e7
10 changed files with 426 additions and 121 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs
+138 -30
View File
@@ -1,6 +1,9 @@
using System.IO.Compression;
using System.Security.Cryptography;
using System.Text.Json;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
@@ -779,10 +782,22 @@ public sealed class BundleImporter : IBundleImporter
// Name is a valid identifier, if Name appears in NEITHER set, surface
// it as a Blocker. This catches the documented use-case
// (HelperFn() / ErpSystem.Call()) without combinatorial blowup.
var referencedFromBundle = new HashSet<string>(StringComparer.Ordinal);
// #05-T19 severity split: track candidate references by ORIGIN. Template-
// script (and attribute-default-expression) references are advisory
// warnings — the design-time deploy gate re-validates them; ApiMethod
// references are hard blockers (no downstream gate). Local-function /
// method declarations in a body are excluded so a script's own helper
// isn't mistaken for a missing SharedScript.
var referencedFromTemplates = new HashSet<string>(StringComparer.Ordinal);
var referencedFromApiMethods = new HashSet<string>(StringComparer.Ordinal);
var locallyDeclared = new HashSet<string>(StringComparer.Ordinal);
foreach (var t in content.Templates)
{
foreach (var s in t.Scripts) CollectCallIdentifiers(s.Code, referencedFromBundle);
foreach (var s in t.Scripts)
{
CollectCallIdentifiers(s.Code, referencedFromTemplates);
CollectLocalDeclarations(s.Code, locallyDeclared);
}
foreach (var a in t.Attributes)
{
// Attribute.Value carries the design-time default expression, which
@@ -791,37 +806,44 @@ public sealed class BundleImporter : IBundleImporter
// it's never script source and must NOT be scanned, or the dot
// delimiter trips the heuristic into flagging the address segments
// as missing SharedScript/ExternalSystem references.
CollectCallIdentifiers(a.Value, referencedFromBundle);
CollectCallIdentifiers(a.Value, referencedFromTemplates);
CollectLocalDeclarations(a.Value, locallyDeclared);
}
}
foreach (var m in content.ApiMethods)
{
CollectCallIdentifiers(m.Script, referencedFromBundle);
CollectCallIdentifiers(m.Script, referencedFromApiMethods);
CollectLocalDeclarations(m.Script, locallyDeclared);
}
// For each candidate, only report it as a blocker if it looks like a
// 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))
// For each candidate, only report it if it looks like a resource
// reference (PascalCase, length > 1), isn't a well-known language /
// runtime / SQL token, isn't the body's own local declaration, AND
// isn't present anywhere we can satisfy it. A candidate that appears in
// an ApiMethod script is a hard Blocker; a template-only candidate is a
// non-blocking Warning.
var allCandidates = new HashSet<string>(referencedFromTemplates, StringComparer.Ordinal);
allCandidates.UnionWith(referencedFromApiMethods);
foreach (var candidate in allCandidates.OrderBy(n => n, StringComparer.Ordinal))
{
if (!LooksLikeResourceName(candidate)) continue;
if (KnownNonReferenceNames.Contains(candidate)) continue;
if (locallyDeclared.Contains(candidate)) continue;
var isShared = sharedScriptNames.Contains(candidate);
var isExternal = externalSystemNames.Contains(candidate);
if (isShared || isExternal) continue;
var fromApiMethod = referencedFromApiMethods.Contains(candidate);
blockers.Add(new ImportPreviewItem(
EntityType: "Reference",
Name: candidate,
ExistingVersion: null,
IncomingVersion: null,
Kind: ConflictKind.Blocker,
Kind: fromApiMethod ? ConflictKind.Blocker : ConflictKind.Warning,
FieldDiffJson: null,
BlockerReason: $"References SharedScript or ExternalSystem '{candidate}' not present in bundle or target."));
BlockerReason: fromApiMethod
? $"References SharedScript or ExternalSystem '{candidate}' not present in bundle or target."
: $"Script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target — advisory; the deploy-time gate re-validates."));
}
return blockers;
@@ -850,6 +872,40 @@ public sealed class BundleImporter : IBundleImporter
}
}
/// <summary>
/// Collects the names of every local function / method DECLARED inside a
/// script body, so the reference heuristic doesn't mistake a script's own
/// helper (<c>decimal ComputeRate(...) => ...; return ComputeRate(x);</c>)
/// for a missing SharedScript / ExternalSystem. Uses a Roslyn script-kind
/// parse (partial trees are fine — a body with syntax errors still yields
/// the declarations Roslyn could recover). Purely additive noise-suppression:
/// on any parse failure the sink is simply left unchanged.
/// </summary>
private static void CollectLocalDeclarations(string? body, HashSet<string> sink)
{
if (string.IsNullOrEmpty(body)) return;
try
{
var root = CSharpSyntaxTree
.ParseText(body, new CSharpParseOptions(kind: SourceCodeKind.Script))
.GetRoot();
foreach (var fn in root.DescendantNodes().OfType<LocalFunctionStatementSyntax>())
{
sink.Add(fn.Identifier.ValueText);
}
foreach (var m in root.DescendantNodes().OfType<MethodDeclarationSyntax>())
{
sink.Add(m.Identifier.ValueText);
}
}
catch
{
// Best-effort: a parse failure just means no declarations are
// excluded for this body — the finding (at worst) surfaces as an
// advisory warning, never a hard error for template scripts.
}
}
/// <summary>
/// Names that look like PascalCase references but are never user-defined
/// SharedScripts or ExternalSystems. Filters the false-positive noise the
@@ -877,7 +933,20 @@ public sealed class BundleImporter : IBundleImporter
// SQL keywords commonly seen inside string literals
"COUNT", "FROM", "GROUP", "INSERT", "JOIN", "ORDER", "SELECT",
"UPDATE", "WHERE",
"UPDATE", "WHERE", "HAVING", "VALUES", "DELETE", "DISTINCT", "LIMIT",
// #05-T19 — extended stdlib / BCL surface commonly reached from scripts.
// The list will still drift as scripts use more of the BCL — that is
// precisely why template-script findings are downgraded to warnings
// (the deploy-time gate re-validates authoritatively).
"Regex", "Match", "Matches", "IsMatch", "Replace", "Split",
"StringBuilder", "Append", "AppendLine", "Parse", "TryParse",
"Format", "Join", "Abs", "Round", "Min", "Max", "Floor", "Ceiling",
"Pow", "Sqrt", "Json", "Serialize", "Deserialize", "Where", "Select",
"First", "FirstOrDefault", "Any", "All", "Count", "Sum", "Average",
"OrderBy", "OrderByDescending", "GroupBy", "Distinct", "Add", "Remove",
"Clear", "Contains", "ContainsKey", "TryGetValue", "StartsWith",
"EndsWith", "Substring", "Trim", "ToUpper", "ToLower",
};
private static bool LooksLikeResourceName(string name)
@@ -982,7 +1051,8 @@ public sealed class BundleImporter : IBundleImporter
// Skip-resolved DTOs are excluded from the in-bundle name set so
// a Skip on a dependency surfaces as a missing-reference error
// rather than silently passing.
var validationErrors = await RunSemanticValidationAsync(content, resolutionMap, ct).ConfigureAwait(false);
var (validationErrors, validationWarnings) =
await RunSemanticValidationAsync(content, resolutionMap, ct).ConfigureAwait(false);
// Validate every site / connection / template reference the
// site-instance payload depends on BEFORE any row is staged. Running
// this in the validation phase (not as an apply-pass guard) preserves
@@ -999,6 +1069,17 @@ public sealed class BundleImporter : IBundleImporter
throw new SemanticValidationException(validationErrors);
}
// Advisory template-script reference findings never block the import;
// log them and surface them on the ImportResult so the operator sees
// the same advisory the preview showed. The deploy-time gate is the
// authoritative re-validation.
if (validationWarnings.Count > 0)
{
_logger?.LogWarning(
"Bundle import {BundleImportId}: {Count} advisory template-script reference warning(s): {Warnings}",
bundleImportId, validationWarnings.Count, string.Join("; ", validationWarnings));
}
// ---- Site/instance-scoped apply: sites + connections FIRST ----
// Sites are the FK target for both data connections (SiteId) and
// instances (SiteId); data connections are the FK target for instance
@@ -1152,7 +1233,8 @@ public sealed class BundleImporter : IBundleImporter
Renamed: summary.Renamed,
StaleInstanceIds: staleInstanceIds,
AuditEventCorrelation: bundleImportId.ToString(),
ApiKeysIgnored: apiKeysIgnored);
ApiKeysIgnored: apiKeysIgnored,
Warnings: validationWarnings);
}
catch (Exception ex)
{
@@ -4086,12 +4168,13 @@ public sealed class BundleImporter : IBundleImporter
/// operator isn't trying to import) would block the import.
/// </para>
/// </summary>
private async Task<IReadOnlyList<string>> RunSemanticValidationAsync(
private async Task<(IReadOnlyList<string> Errors, IReadOnlyList<string> Warnings)> RunSemanticValidationAsync(
BundleContentDto content,
Dictionary<(string, string), ImportResolution> resolutionMap,
CancellationToken ct)
{
var errors = new List<string>();
var warnings = new List<string>();
// ---- Pass 1: minimal name-resolution scan ----
@@ -4135,17 +4218,25 @@ public sealed class BundleImporter : IBundleImporter
}
// Collect every identifier-shaped call target from the bundle's
// templates + api methods. We only check the bundle's bodies here
// (matching PreviewAsync's blocker scan); pre-existing target rows are
// assumed already validated when they were originally written.
var referenced = new HashSet<string>(StringComparer.Ordinal);
// templates + api methods, keyed by ORIGIN (#05-T19 severity split).
// We only check the bundle's bodies here (matching PreviewAsync's blocker
// scan); pre-existing target rows are assumed already validated when they
// were originally written. Local-function / method declarations in a body
// are collected too so a script's own helper isn't flagged as missing.
var referencedFromTemplates = new HashSet<string>(StringComparer.Ordinal);
var referencedFromApiMethods = new HashSet<string>(StringComparer.Ordinal);
var locallyDeclared = new HashSet<string>(StringComparer.Ordinal);
foreach (var t in content.Templates)
{
// Skip-resolved templates aren't being written, so their script
// references don't need to resolve.
var resolution = ResolveOrDefault(resolutionMap, "Template", t.Name);
if (resolution.Action == ResolutionAction.Skip) continue;
foreach (var s in t.Scripts) CollectCallIdentifiers(s.Code, referenced);
foreach (var s in t.Scripts)
{
CollectCallIdentifiers(s.Code, referencedFromTemplates);
CollectLocalDeclarations(s.Code, locallyDeclared);
}
foreach (var a in t.Attributes)
{
// Value can hold script-callable design-time expressions;
@@ -4153,29 +4244,46 @@ public sealed class BundleImporter : IBundleImporter
// "ns=3;s=Tank.Level") and must NOT be scanned, or the dot
// delimiter will flag tag-path segments as missing references.
// Symmetric with DetectBlockersAsync.
CollectCallIdentifiers(a.Value, referenced);
CollectCallIdentifiers(a.Value, referencedFromTemplates);
CollectLocalDeclarations(a.Value, locallyDeclared);
}
}
foreach (var m in content.ApiMethods)
{
var resolution = ResolveOrDefault(resolutionMap, "ApiMethod", m.Name);
if (resolution.Action == ResolutionAction.Skip) continue;
CollectCallIdentifiers(m.Script, referenced);
CollectCallIdentifiers(m.Script, referencedFromApiMethods);
CollectLocalDeclarations(m.Script, locallyDeclared);
}
foreach (var candidate in referenced.OrderBy(n => n, StringComparer.Ordinal))
// ApiMethod references are hard errors (no downstream deploy gate);
// template-only references are advisory warnings (the design-time deploy
// gate re-validates with a real compile).
var allCandidates = new HashSet<string>(referencedFromTemplates, StringComparer.Ordinal);
allCandidates.UnionWith(referencedFromApiMethods);
foreach (var candidate in allCandidates.OrderBy(n => n, StringComparer.Ordinal))
{
if (!LooksLikeResourceName(candidate)) continue;
if (KnownNonReferenceNames.Contains(candidate)) continue;
if (locallyDeclared.Contains(candidate)) continue;
if (sharedScriptNames.Contains(candidate) || externalSystemNames.Contains(candidate)) continue;
errors.Add(
$"Script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target.");
if (referencedFromApiMethods.Contains(candidate))
{
errors.Add(
$"Script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target.");
}
else
{
warnings.Add(
$"Template script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target — advisory; re-validated at deploy time.");
}
}
// Fail fast — running the full validator over templates that already
// failed name resolution would produce duplicate / lower-quality errors
// (the missing identifier shows up there as "callee not found" too).
if (errors.Count > 0) return errors;
// Warnings ride along regardless — they never gate the import.
if (errors.Count > 0) return (errors, warnings);
// ---- Pass 2: full SemanticValidator over imported templates ----
@@ -4230,7 +4338,7 @@ public sealed class BundleImporter : IBundleImporter
}
}
return errors;
return (errors, warnings);
}
/// <summary>