perf(template-engine): read-only staleness/comparison paths skip Expression-trigger compiles — deploy gate remains the authoritative check (plan R2-05 T4)

2026-07-13 09:48:40 -04:00
parent d233ecbe8f
commit 31fa5eafa4
2 changed files with 76 additions and 15 deletions
@@ -97,11 +97,16 @@ public class ValidationService
    /// </param>
    /// <param name="validateScriptCompilation">
    /// <c>true</c> (default) runs the authoritative Roslyn <see cref="ValidateScriptCompilation"/>
-    /// stage — the single most expensive check (a non-collectible assembly load per script).
-    /// <c>false</c> skips ONLY that stage; every other structural/semantic check still runs.
-    /// Read-only staleness/comparison callers (DeploymentManager's FlatteningPipeline on its
-    /// comparison/probe paths) pass <c>false</c> because they need only the flattened config and
-    /// revision hash, not a compile. The deploy gate keeps the default (<c>true</c>).
+    /// stage — the single most expensive check (a non-collectible assembly load per script) —
+    /// AND the Expression-trigger syntax/compile check
+    /// (<see cref="CheckExpressionSyntax"/>, a forbidden-API verdict plus a real compile of every
+    /// Expression-triggered script/alarm body against <see cref="TriggerCompileSurface"/>).
+    /// <c>false</c> skips ONLY those two compile stages; every other structural/semantic check
+    /// still runs — including, for Expression triggers, the blank-expression check and the
+    /// cheap attribute-reference scan. Read-only staleness/comparison callers (DeploymentManager's
+    /// FlatteningPipeline on its comparison/probe paths) pass <c>false</c> because they need only
+    /// the flattened config and revision hash, not a compile. The deploy gate keeps the default
+    /// (<c>true</c>) — it is the authoritative check for both stages.
    /// </param>
    /// <returns>A merged <see cref="ValidationResult"/> aggregating all pipeline stage outcomes.</returns>
    public ValidationResult Validate(
@@ -128,7 +133,7 @@ public class ValidationService
            validateScriptCompilation ? ValidateScriptCompilation(configuration) : ValidationResult.Success(),
            ValidateAlarmTriggerReferences(configuration),
            ValidateScriptTriggerReferences(configuration),
-            ValidateExpressionTriggers(configuration),
+            ValidateExpressionTriggers(configuration, validateScriptCompilation),
            ValidateConnectionBindingCompleteness(configuration, enforceConnectionBindings, siteConnectionNames),
            ValidateSchemaReferences(configuration, sharedScripts, resolveSchemaRef),
            _semanticValidator.Validate(configuration, sharedScripts, alarmCapableConnectionNames)
@@ -366,8 +371,16 @@ public class ValidationService
    /// </list>
    /// </summary>
    /// <param name="configuration">The flattened configuration to validate.</param>
+    /// <param name="validateExpressionSyntax">
+    /// <c>true</c> (default) runs the authoritative <see cref="CheckExpressionSyntax"/>
+    /// forbidden-API + compile stage for each Expression trigger. <c>false</c> skips ONLY
+    /// that stage — the blank-expression check and the attribute-reference scan still run —
+    /// so read-only staleness/comparison callers do not pay the per-expression compile.
+    /// </param>
    /// <returns>A <see cref="ValidationResult"/> with errors and warnings from all expression trigger checks.</returns>
-    public static ValidationResult ValidateExpressionTriggers(FlattenedConfiguration configuration)
+    public static ValidationResult ValidateExpressionTriggers(
+        FlattenedConfiguration configuration,
+        bool validateExpressionSyntax = true)
    {
        var errors = new List<ValidationEntry>();
        var warnings = new List<ValidationEntry>();
@@ -382,7 +395,7 @@ public class ValidationService
            CheckExpressionTrigger(
                ValidationCategory.ScriptTriggerReference, "script",
                script.CanonicalName, script.TriggerConfiguration,
-                attributeNames, errors, warnings);
+                attributeNames, errors, warnings, validateExpressionSyntax);
        }

        foreach (var alarm in configuration.Alarms)
@@ -393,7 +406,7 @@ public class ValidationService
            CheckExpressionTrigger(
                ValidationCategory.AlarmTriggerReference, "alarm",
                alarm.CanonicalName, alarm.TriggerConfiguration,
-                attributeNames, errors, warnings);
+                attributeNames, errors, warnings, validateExpressionSyntax);
        }

        return new ValidationResult { Errors = errors, Warnings = warnings };
@@ -424,7 +437,8 @@ public class ValidationService
        string? triggerConfigJson,
        HashSet<string> attributeNames,
        List<ValidationEntry> errors,
-        List<ValidationEntry> warnings)
+        List<ValidationEntry> warnings,
+        bool validateExpressionSyntax = true)
    {
        var expression = ExtractExpressionFromTriggerConfig(triggerConfigJson);
        var strict = IsStrictAnalysis(triggerConfigJson);
@@ -443,6 +457,13 @@ public class ValidationService
            return;
        }

+        // The syntax/compile stage is the expensive one (a forbidden-API verdict + a
+        // real Roslyn compile of the expression). Read-only staleness/comparison
+        // callers pass validateExpressionSyntax: false to skip ONLY this stage; the
+        // blank-expression check above and the attribute-reference scan below still run.
+        // The deploy gate keeps the default (true) — the authoritative check.
+        if (validateExpressionSyntax)
+        {
            var syntaxError = CheckExpressionSyntax(expression);
            if (syntaxError != null)
            {
@@ -450,6 +471,7 @@ public class ValidationService
                    $"The {entityLabel} '{entityName}' expression trigger failed validation: {syntaxError}",
                    entityName));
            }
+        }

        foreach (var attrName in ExtractAttributeReferences(expression))
        {
@@ -462,4 +462,43 @@ public class ValidationServiceTests
        Assert.DoesNotContain(strictResult.Errors, e => e.Category == ValidationCategory.AlarmTriggerReference);
        Assert.DoesNotContain(strictResult.Warnings, w => w.Category == ValidationCategory.AlarmTriggerReference);
    }
+
+    [Fact]
+    public void Validate_SkipCompilation_SkipsExpressionTriggerSyntaxCheck()
+    {
+        var config = ConfigWithExpressionTriggerScript("this is not C# ((");
+        var gated = new ValidationService().Validate(config, validateScriptCompilation: false);
+        Assert.DoesNotContain(gated.Errors, e => e.Message.Contains("failed validation"));  // FAILS today
+        var full = new ValidationService().Validate(config);
+        Assert.Contains(full.Errors, e => e.Message.Contains("failed validation"));         // deploy gate unchanged
+    }
+
+    [Fact]
+    public void Validate_SkipCompilation_StillChecksExpressionAttributeReferences()
+    {
+        // expression "Attributes[\"Ghost\"] != null" referencing a missing attribute:
+        // the reference scan is cheap string work and MUST survive the gate.
+        var config = ConfigWithExpressionTriggerScript("Attributes[\"Ghost\"] != null");
+        var gated = new ValidationService().Validate(config, validateScriptCompilation: false);
+        Assert.Contains(gated.Errors, e => e.Message.Contains("Ghost"));
+    }
+
+    private static FlattenedConfiguration ConfigWithExpressionTriggerScript(string expression)
+    {
+        var json = System.Text.Json.JsonSerializer.Serialize(new { expression });
+        return new FlattenedConfiguration
+        {
+            InstanceUniqueName = "Instance1",
+            Attributes = [new ResolvedAttribute { CanonicalName = "Temp", Value = "25", DataType = "Double" }],
+            Scripts =
+            [
+                new ResolvedScript
+                {
+                    CanonicalName = "OnExpr",
+                    TriggerType = "Expression",
+                    TriggerConfiguration = json
+                }
+            ]
+        };
+    }
 }