fix(transport): close code-review gaps in the import trust gate + severity split (T19/T20)

Addresses the T20 security code-review findings:
- Gate template script + alarm Expression-trigger bodies too (they compile and
  execute at the site), not just script bodies — EnumerateTrustGatedScripts now
  extracts and vets the {"expression":"..."} C# via the same trust validator.
- Per-origin local-declaration exclusion: a helper declared in a template script
  no longer suppresses a genuinely-missing reference of the same name in an
  ApiMethod (a global set silently defeated the ApiMethod hard-blocker).
- Fail-closed: a ScriptTrustValidator throw on one pathological body is caught
  and surfaced as a hard blocker FOR THAT script (apply) / Blocker row (preview)
  instead of aborting the whole import.
- Corrected the misleading attribute-Value comment (typed literal, not a compiled
  expression) and documented the Pass-0 fail-fast rationale.

Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj

src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs

@@ -791,57 +791,66 @@ public sealed class BundleImporter : IBundleImporter
         // it as a Blocker. This catches the documented use-case
         // (HelperFn() / ErpSystem.Call()) without combinatorial blowup.
         // #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.
+        // script references are advisory warnings — the design-time deploy gate
+        // re-validates them; ApiMethod references are hard blockers (no downstream
+        // gate). Local-function / method declarations are collected PER ORIGIN so a
+        // helper declared in a template script cannot suppress a genuinely-missing
+        // ApiMethod reference of the same name (and vice-versa) — a global set would
+        // silently defeat the ApiMethod hard-blocker.
         var referencedFromTemplates = new HashSet<string>(StringComparer.Ordinal);
         var referencedFromApiMethods = new HashSet<string>(StringComparer.Ordinal);
-        var locallyDeclared = new HashSet<string>(StringComparer.Ordinal);
+        var locallyDeclaredInTemplates = new HashSet<string>(StringComparer.Ordinal);
+        var locallyDeclaredInApiMethods = new HashSet<string>(StringComparer.Ordinal);
         foreach (var t in content.Templates)
         {
             foreach (var s in t.Scripts)
             {
                 CollectCallIdentifiers(s.Code, referencedFromTemplates);
-                CollectLocalDeclarations(s.Code, locallyDeclared);
+                CollectLocalDeclarations(s.Code, locallyDeclaredInTemplates);
             }
             foreach (var a in t.Attributes)
             {
-                // Attribute.Value carries the design-time default expression, which
-                // can be script-callable. DataSourceReference is an OPC UA node
-                // address path (e.g. "ns=3;s=Tank.Level") owned by the device --
-                // 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.
+                // Attribute.Value is a typed-literal default (int/double/string/list,
+                // decoded by AttributeValueCodec — never compiled), so it is not a
+                // trust surface; we still scan it for identifier-shaped tokens, but
+                // any finding is only ever an advisory template warning.
+                // DataSourceReference is an OPC UA node address path (e.g.
+                // "ns=3;s=Tank.Level") owned by the device 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, referencedFromTemplates);
-                CollectLocalDeclarations(a.Value, locallyDeclared);
+                CollectLocalDeclarations(a.Value, locallyDeclaredInTemplates);
             }
         }
         foreach (var m in content.ApiMethods)
         {
             CollectCallIdentifiers(m.Script, referencedFromApiMethods);
-            CollectLocalDeclarations(m.Script, locallyDeclared);
+            CollectLocalDeclarations(m.Script, locallyDeclaredInApiMethods);
         }
 
         // 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.
+        // runtime / SQL token, AND isn't present anywhere we can satisfy it. A
+        // candidate genuinely unresolved in an ApiMethod (referenced there and NOT
+        // locally declared there) is a hard Blocker; a candidate genuinely
+        // unresolved only in a template 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);
+            var apiUnresolved = referencedFromApiMethods.Contains(candidate)
+                && !locallyDeclaredInApiMethods.Contains(candidate);
+            var templateUnresolved = referencedFromTemplates.Contains(candidate)
+                && !locallyDeclaredInTemplates.Contains(candidate);
+            if (!apiUnresolved && !templateUnresolved) continue; // satisfied by a local decl in its own origin
+
+            var fromApiMethod = apiUnresolved;
             blockers.Add(new ImportPreviewItem(
                 EntityType: "Reference",
                 Name: candidate,
@@ -861,7 +870,29 @@ public sealed class BundleImporter : IBundleImporter
         // as distinct rows.
         foreach (var (kind, entityName, scriptLabel, code) in EnumerateTrustGatedScripts(content, resolutionMap: null))
         {
-            foreach (var violation in ScriptTrustValidator.FindViolations(code))
+            IReadOnlyList<string> violations;
+            try
+            {
+                violations = ScriptTrustValidator.FindViolations(code);
+            }
+            catch (Exception ex)
+            {
+                // Fail closed (mirrors the apply-time gate): a validator throw on
+                // one body surfaces as a Blocker for that script, not a preview crash.
+                _logger?.LogWarning(ex,
+                    "Script trust analysis threw for {Kind} '{EntityName}' script '{ScriptLabel}' during preview; surfacing as a Blocker.",
+                    kind, entityName, scriptLabel);
+                blockers.Add(new ImportPreviewItem(
+                    EntityType: kind,
+                    Name: $"{entityName}.{scriptLabel}",
+                    ExistingVersion: null,
+                    IncomingVersion: null,
+                    Kind: ConflictKind.Blocker,
+                    FieldDiffJson: null,
+                    BlockerReason: "Script trust analysis failed to complete — rejected."));
+                continue;
+            }
+            foreach (var violation in violations)
             {
                 blockers.Add(new ImportPreviewItem(
                     EntityType: kind,
@@ -935,11 +966,15 @@ public sealed class BundleImporter : IBundleImporter
     }
 
     /// <summary>
-    /// #05-T20 — enumerates every executable script body the trust gate must
-    /// vet: non-Skip template scripts, shared scripts, and ApiMethod scripts.
-    /// A null <paramref name="resolutionMap"/> (preview time, before resolutions
-    /// are chosen) gates everything. Yields <c>(kind, entityName, scriptLabel,
-    /// code)</c> so callers can format either an error string or a preview row.
+    /// #05-T20 — enumerates every executable C# surface a bundle carries that the
+    /// trust gate must vet: non-Skip template scripts + their Expression-trigger
+    /// bodies, template alarm Expression-trigger bodies, shared scripts, and
+    /// ApiMethod scripts. Expression triggers compile and execute at the site
+    /// (`TriggerExpressionGlobals`), so they are a genuine trust surface, not just
+    /// script bodies. A null <paramref name="resolutionMap"/> (preview time, before
+    /// resolutions are chosen) gates everything. Yields <c>(kind, entityName,
+    /// scriptLabel, code)</c> so callers can format either an error string or a
+    /// preview row.
     /// </summary>
     private static IEnumerable<(string Kind, string EntityName, string ScriptLabel, string Code)> EnumerateTrustGatedScripts(
         BundleContentDto content,
@@ -954,6 +989,19 @@ public sealed class BundleImporter : IBundleImporter
                 {
                     yield return ("Template", t.Name, s.Name, s.Code);
                 }
+                var scriptExpr = ExtractTriggerExpression(s.TriggerType, s.TriggerConfiguration);
+                if (!string.IsNullOrEmpty(scriptExpr))
+                {
+                    yield return ("Template", t.Name, $"{s.Name} (trigger expression)", scriptExpr);
+                }
+            }
+            foreach (var a in t.Alarms)
+            {
+                var alarmExpr = ExtractTriggerExpression(a.TriggerType.ToString(), a.TriggerConfiguration);
+                if (!string.IsNullOrEmpty(alarmExpr))
+                {
+                    yield return ("Template", t.Name, $"{a.Name} (alarm trigger expression)", alarmExpr);
+                }
             }
         }
         foreach (var s in content.SharedScripts)
@@ -974,6 +1022,38 @@ public sealed class BundleImporter : IBundleImporter
         }
     }
 
+    /// <summary>
+    /// Extracts the C# boolean expression from an <c>{"expression":"…"}</c> trigger
+    /// configuration when the trigger type is <c>Expression</c> — the same shape
+    /// the site's <c>TriggerExpressionGlobals</c> compiles. Returns null for any
+    /// non-Expression trigger, empty config, or malformed JSON (nothing to gate).
+    /// </summary>
+    private static string? ExtractTriggerExpression(string? triggerType, string? triggerConfigJson)
+    {
+        if (triggerType is null
+            || !triggerType.Equals("Expression", StringComparison.OrdinalIgnoreCase)
+            || string.IsNullOrWhiteSpace(triggerConfigJson))
+        {
+            return null;
+        }
+        try
+        {
+            using var doc = JsonDocument.Parse(triggerConfigJson);
+            if (doc.RootElement.ValueKind == JsonValueKind.Object
+                && doc.RootElement.TryGetProperty("expression", out var expr)
+                && expr.ValueKind == JsonValueKind.String)
+            {
+                return expr.GetString();
+            }
+        }
+        catch (JsonException)
+        {
+            // Malformed trigger config isn't executable as an expression — nothing
+            // to gate here (a separate validation surfaces the malformed config).
+        }
+        return null;
+    }
+
     private static bool IsSkipResolution(
         Dictionary<(string, string), ImportResolution>? resolutionMap, string entityType, string name)
         => resolutionMap != null
@@ -4250,19 +4330,39 @@ public sealed class BundleImporter : IBundleImporter
         var warnings = new List<string>();
 
         // ---- Pass 0: script trust gate ----
-        // Bundle import is the fifth script-trust call site. Every executable
-        // script body (template, shared, ApiMethod) is run through the shared
-        // ScriptTrustValidator BEFORE name resolution — a forbidden-API verdict
-        // is authoritative (not the false-positive-prone name heuristic), so it
-        // is a HARD error for all kinds, and it must not be masked by a Pass-1
-        // name-resolution error. Task 15's verdict cache keeps repeat cost nil.
+        // Bundle import is the fifth script-trust call site. Every executable C#
+        // surface a bundle carries (template / shared / ApiMethod script bodies
+        // AND template script + alarm Expression-trigger bodies) is run through
+        // the shared ScriptTrustValidator BEFORE name resolution — a forbidden-API
+        // verdict is authoritative (not the false-positive-prone name heuristic),
+        // so it is a HARD error for all kinds, and it must not be masked by a
+        // Pass-1 name-resolution error. Task 15's verdict cache keeps repeat cost nil.
         foreach (var (kind, entityName, scriptLabel, code) in EnumerateTrustGatedScripts(content, resolutionMap))
         {
-            foreach (var violation in ScriptTrustValidator.FindViolations(code))
+            IReadOnlyList<string> violations;
+            try
+            {
+                violations = ScriptTrustValidator.FindViolations(code);
+            }
+            catch (Exception ex)
+            {
+                // Fail closed: a validator throw on one pathological body must not
+                // abort the whole import — treat it as a hard blocker FOR THAT
+                // script so the offender is named and the rest still surface.
+                _logger?.LogWarning(ex,
+                    "Script trust analysis threw for {Kind} '{EntityName}' script '{ScriptLabel}'; treating as a hard blocker.",
+                    kind, entityName, scriptLabel);
+                errors.Add($"{kind} '{entityName}' script '{scriptLabel}': trust analysis failed to complete — rejected.");
+                continue;
+            }
+            foreach (var violation in violations)
             {
                 errors.Add($"{kind} '{entityName}' script '{scriptLabel}': {violation}");
             }
         }
+        // Fail fast on trust violations: the operator sees trust errors OR name-
+        // resolution errors, not both at once. `warnings` is always empty here
+        // (Pass 0 emits only errors), so nothing surfacable is dropped.
         if (errors.Count > 0) return (errors, warnings);
 
         // ---- Pass 1: minimal name-resolution scan ----
@@ -4314,7 +4414,8 @@ public sealed class BundleImporter : IBundleImporter
         // 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);
+        var locallyDeclaredInTemplates = new HashSet<string>(StringComparer.Ordinal);
+        var locallyDeclaredInApiMethods = new HashSet<string>(StringComparer.Ordinal);
         foreach (var t in content.Templates)
         {
             // Skip-resolved templates aren't being written, so their script
@@ -4324,17 +4425,18 @@ public sealed class BundleImporter : IBundleImporter
             foreach (var s in t.Scripts)
             {
                 CollectCallIdentifiers(s.Code, referencedFromTemplates);
-                CollectLocalDeclarations(s.Code, locallyDeclared);
+                CollectLocalDeclarations(s.Code, locallyDeclaredInTemplates);
             }
             foreach (var a in t.Attributes)
             {
-                // Value can hold script-callable design-time expressions;
+                // Attribute.Value is a typed-literal default (never compiled), so it
+                // is not a trust surface; scanned only for the advisory name heuristic.
                 // DataSourceReference is an OPC UA address-space path (e.g.
                 // "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, referencedFromTemplates);
-                CollectLocalDeclarations(a.Value, locallyDeclared);
+                CollectLocalDeclarations(a.Value, locallyDeclaredInTemplates);
             }
         }
         foreach (var m in content.ApiMethods)
@@ -4342,26 +4444,32 @@ public sealed class BundleImporter : IBundleImporter
             var resolution = ResolveOrDefault(resolutionMap, "ApiMethod", m.Name);
             if (resolution.Action == ResolutionAction.Skip) continue;
             CollectCallIdentifiers(m.Script, referencedFromApiMethods);
-            CollectLocalDeclarations(m.Script, locallyDeclared);
+            CollectLocalDeclarations(m.Script, locallyDeclaredInApiMethods);
         }
 
-        // 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).
+        // ApiMethod references genuinely unresolved (referenced there and NOT locally
+        // declared there) are hard errors (no downstream deploy gate); template-only
+        // unresolved references are advisory warnings (the deploy gate re-validates
+        // with a real compile). Local-declaration exclusion is per-origin so a
+        // template helper cannot mask a missing ApiMethod reference.
         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;
-            if (referencedFromApiMethods.Contains(candidate))
+
+            var apiUnresolved = referencedFromApiMethods.Contains(candidate)
+                && !locallyDeclaredInApiMethods.Contains(candidate);
+            var templateUnresolved = referencedFromTemplates.Contains(candidate)
+                && !locallyDeclaredInTemplates.Contains(candidate);
+            if (apiUnresolved)
             {
                 errors.Add(
                     $"Script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target.");
             }
-            else
+            else if (templateUnresolved)
             {
                 warnings.Add(
                     $"Template script references SharedScript or ExternalSystem '{candidate}' not present in bundle or target — advisory; re-validated at deploy time.");

tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs

@@ -367,6 +367,86 @@ public sealed class SemanticValidatorImportTests : IDisposable
         Assert.Contains(ex.Errors, err => err.Contains("Process", StringComparison.Ordinal));
     }
 
+    [Fact]
+    public async Task Apply_AlarmExpressionTriggerWithForbiddenApi_HardBlocks()
+    {
+        // #05-T20 (code-review fix): an alarm's Expression trigger compiles and
+        // executes at the site, so a forbidden API in the trigger expression must
+        // be caught at import — not just in template/shared/ApiMethod script bodies.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var t = new Template("AlarmedTank");
+            t.Alarms.Add(new TemplateAlarm("Bad")
+            {
+                TriggerType = AlarmTriggerType.Expression,
+                TriggerConfiguration = "{\"expression\":\"System.Diagnostics.Process.GetCurrentProcess() != null\"}",
+                PriorityLevel = 1,
+            });
+            ctx.Templates.Add(t);
+            await ctx.SaveChangesAsync();
+        }
+
+        var sessionId = await ExportWipeAndLoadAsync();
+
+        SemanticValidationException ex = default!;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            ex = await Assert.ThrowsAsync<SemanticValidationException>(() =>
+                importer.ApplyAsync(sessionId,
+                    new List<ImportResolution>
+                    {
+                        new("Template", "AlarmedTank", ResolutionAction.Add, null),
+                    },
+                    user: "bob"));
+        }
+
+        Assert.NotEmpty(ex.Errors);
+        Assert.Contains(ex.Errors, err => err.Contains("Process", StringComparison.Ordinal));
+    }
+
+    [Fact]
+    public async Task Apply_ApiMethodReference_NotSuppressedByTemplateLocalFunction()
+    {
+        // #05-T19 (code-review fix): a local function declared in a TEMPLATE script
+        // must not suppress a genuinely-missing reference of the same name in an
+        // ApiMethod — the ApiMethod finding stays a HARD error. A pre-fix global
+        // locallyDeclared set silently defeated this.
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var ctx = scope.ServiceProvider.GetRequiredService<ScadaBridgeDbContext>();
+            var t = new Template("Helper");
+            t.Scripts.Add(new Commons.Entities.Templates.TemplateScript(
+                "init",
+                "decimal SharedHelper(decimal x) => x * 2; return SharedHelper(1m);"));
+            ctx.Templates.Add(t);
+            ctx.ApiMethods.Add(new Commons.Entities.InboundApi.ApiMethod(
+                "Ingest",
+                "var x = SharedHelper(); return x;"));
+            await ctx.SaveChangesAsync();
+        }
+
+        var sessionId = await ExportWipeAndLoadAsync();
+
+        SemanticValidationException ex = default!;
+        await using (var scope = _provider.CreateAsyncScope())
+        {
+            var importer = scope.ServiceProvider.GetRequiredService<IBundleImporter>();
+            ex = await Assert.ThrowsAsync<SemanticValidationException>(() =>
+                importer.ApplyAsync(sessionId,
+                    new List<ImportResolution>
+                    {
+                        new("Template", "Helper", ResolutionAction.Add, null),
+                        new("ApiMethod", "Ingest", ResolutionAction.Add, null),
+                    },
+                    user: "bob"));
+        }
+
+        Assert.NotEmpty(ex.Errors);
+        Assert.Contains(ex.Errors, err => err.Contains("SharedHelper", StringComparison.Ordinal));
+    }
+
     [Fact]
     public async Task SemanticValidator_catches_alarm_trigger_type_mismatch_at_import()
     {