diff --git a/code-reviews/Core.Scripting/findings.md b/code-reviews/Core.Scripting/findings.md index 2ef06e3..3c1c962 100644 --- a/code-reviews/Core.Scripting/findings.md +++ b/code-reviews/Core.Scripting/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 5 | +| Open findings | 0 | ## Checklist coverage @@ -169,7 +169,7 @@ member-access call to a non-ctx `GetTag` is untested and would be misattributed. | Severity | Low | | Category | Correctness & logic bugs | | Location | `DependencyExtractor.cs:97` | -| Status | Open | +| Status | Resolved | **Description:** A raw string literal token passed as the tag path (a raw triple-quote literal) tokenizes as `SingleLineRawStringLiteralToken` / @@ -183,7 +183,7 @@ paths) but the error text would confuse anyone who does. `literal.IsKind(SyntaxKind.StringLiteralExpression)` on the expression node, or include the raw-string token kinds, so a static raw string is harvested rather than rejected. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `HandleTagCall` now checks `literal.IsKind(SyntaxKind.StringLiteralExpression)` on the expression node, which covers regular string literals, single-line raw strings, and multi-line raw strings uniformly. Regression tests `Accepts_single_line_raw_string_literal_path` and `Accepts_multi_line_raw_string_literal_path` added to `DependencyExtractorTests`. ### Core.Scripting-006 @@ -192,7 +192,7 @@ the raw-string token kinds, so a static raw string is harvested rather than reje | Severity | Low | | Category | Concurrency & thread safety | | Location | `CompiledScriptCache.cs:55` | -| Status | Open | +| Status | Resolved | **Description:** On a failed compile the `catch` block calls `_cache.TryRemove(key, out _)` without a value comparison. If two threads race a miss for @@ -206,7 +206,7 @@ but the removal should be key+value scoped for correctness. remove only the specific faulted `Lazy` instance, so a concurrently re-added entry is not evicted. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `GetOrCompile`'s catch block now evicts via `_cache.TryRemove(new KeyValuePair>(key, lazy))`, comparing the value reference so only the faulted Lazy is removed; a concurrent retry that re-inserted a fresh Lazy under the same key is preserved. Regression test `Failed_compile_eviction_does_not_remove_a_concurrent_retry_entry` added to `CompiledScriptCacheTests` (reflection-driven deterministic race: the faulted Lazy's factory swaps the dictionary entry to a fresh Lazy as a side effect of its throw, modelling the precise race window). ### Core.Scripting-007 @@ -257,7 +257,7 @@ compile scripts into a collectible `AssemblyLoadContext` so `Clear()` can unload generations. At minimum add a note to `docs/ScriptedAlarms.md` so operators with high-publish-frequency deployments are aware. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — accepted as a documented known limitation rather than fixing in code (collectible `AssemblyLoadContext` for Roslyn-emitted assemblies is a v3 concern). The "Compile cache" section of `docs/VirtualTags.md` now carries a "Per-publish assembly accretion (accepted limitation, Core.Scripting-008)" note that operators with high-publish-frequency deployments can scan, and `docs/ScriptedAlarms.md` cross-references it. The accretion is benign at the expected "low thousands" of scripts scale; recommended mitigation is a scheduled server restart for deployments that publish very frequently. ### Core.Scripting-009 @@ -266,7 +266,7 @@ high-publish-frequency deployments are aware. | Severity | Low | | Category | Design-document adherence | | Location | `ForbiddenTypeAnalyzer.cs:45` | -| Status | Open | +| Status | Resolved | **Description:** The Phase 7 plan decision #6 (`docs/v2/implementation/phase-7-scripting-and-alarming.md`) enumerates the forbidden @@ -283,7 +283,7 @@ authoritative deny-list exactly as `ForbiddenTypeAnalyzer.ForbiddenNamespacePref defines it, including the `System.Environment` allowed-compromise, so the docs match the code. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — `docs/v2/implementation/phase-7-scripting-and-alarming.md` decision #6 row + the "Sandbox escape" compliance-check row now enumerate the authoritative deny-list exactly as `ForbiddenTypeAnalyzer` defines it (namespace-prefix denies for `System.IO` / `System.Net` / `System.Diagnostics` / `System.Reflection` / `System.Threading.Tasks` / `System.Runtime.InteropServices` / `Microsoft.Win32`; type-granular denies for `System.Environment` / `System.AppDomain` / `System.GC` / `System.Activator` / `System.Threading.Thread`), and the compliance-check row lists the syntactic vectors (`typeof` / generic arg / cast / `is`/`as` / `default(T)` / array element / declared local) the broadened analyzer covers. `docs/VirtualTags.md` already documents the same list and is unchanged. ### Core.Scripting-010 @@ -318,7 +318,7 @@ assert a `ScriptSandboxViolationException` (or `CompilationErrorException`) at c | Severity | Low | | Category | Testing coverage | | Location | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/` | -| Status | Open | +| Status | Resolved | **Description:** Two source files have no direct test coverage: `ScriptContext` (`Deadband` static helper is exercised only indirectly through `ScriptSandboxTests`, and @@ -335,4 +335,4 @@ unverified. a script logging at Error level produces both a `scripts-*.log` event and a companion Warning event. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-23 — added three new test files: `ScriptSandboxBuildTests` covers the `Build` null / non-`ScriptContext` / base-class / concrete-subclass paths; `ScriptContextTests` locks `Deadband` boundary semantics (equal-to-tolerance returns false; just-over returns true; symmetric in direction; zero-tolerance returns true only on non-equal; negative tolerance trips on any non-equal); the new `Factory_plus_companion_sink_integration_surfaces_script_error_in_both_logs` test in `ScriptLogCompanionSinkTests` wires `ScriptLoggerFactory` + the companion sink together end-to-end and asserts an Error emission lands in both the scripts sink (at Error) and the main sink (at Warning), each tagged with `ScriptName`. Suite now 101 green (was 85 before). diff --git a/docs/VirtualTags.md b/docs/VirtualTags.md index 435f11b..5f134b5 100644 --- a/docs/VirtualTags.md +++ b/docs/VirtualTags.md @@ -28,7 +28,9 @@ Similarly, **`System.Threading.Tasks` is now denied** (Core.Scripting-003), whic ### Compile cache (`CompiledScriptCache`) -`ConcurrentDictionary>>` keyed on `SHA-256(UTF8(source))` rendered to hex. `Lazy` with `ExecutionAndPublication` mode means two threads racing a miss compile exactly once. Failed compiles evict the entry so a corrected retry can succeed (used during Admin UI authoring). No capacity bound — scripts are operator-authored and bounded by the config DB. Whitespace changes miss the cache on purpose. `Clear()` is called on config-publish. +`ConcurrentDictionary>>` keyed on `SHA-256(UTF8(source))` rendered to hex. `Lazy` with `ExecutionAndPublication` mode means two threads racing a miss compile exactly once. Failed compiles evict the entry (via the `TryRemove(KeyValuePair<,>)` overload so a concurrently re-added retry entry is not collateral damage — Core.Scripting-006) so a corrected retry can succeed (used during Admin UI authoring). No capacity bound — scripts are operator-authored and bounded by the config DB. Whitespace changes miss the cache on purpose. `Clear()` is called on config-publish. + +**Per-publish assembly accretion (accepted limitation, Core.Scripting-008).** Each compiled `ScriptEvaluator` holds a Roslyn `ScriptRunner` delegate, which keeps the dynamically-emitted script assembly loaded for the process lifetime. Emitted assemblies in the default `AssemblyLoadContext` cannot be unloaded; `CompiledScriptCache.Clear()` drops the dictionary entries but does **not** unload the underlying assemblies. Across many config-publish generations (each `Clear()` followed by recompiling every script), the process accumulates dead script assemblies. For the expected "low thousands" of scripts this is benign, but a long-running server with very frequent publishes will see steady managed-memory growth that does not return until the process restarts. Out-of-process script evaluation or a collectible `AssemblyLoadContext` is a v3 concern; deployments with high-publish-frequency requirements should schedule a periodic server restart to reclaim the accrued assemblies. ### Per-evaluation timeout (`TimedScriptEvaluator`) diff --git a/docs/v2/implementation/phase-7-scripting-and-alarming.md b/docs/v2/implementation/phase-7-scripting-and-alarming.md index defe069..8be57e6 100644 --- a/docs/v2/implementation/phase-7-scripting-and-alarming.md +++ b/docs/v2/implementation/phase-7-scripting-and-alarming.md @@ -29,7 +29,7 @@ Tie-in capability — **historian alarm sink**: | 3 | Evaluation trigger = **change-driven + timer-driven**; operator chooses per-tag | Change-driven is cheap at steady state; timer is the escape hatch for polling derivations that don't have a discrete "input changed" signal. | | 4 | Script shape = **Shape A — one script per virtual tag/alarm**; `return` produces the value (or `bool` for alarm condition) | Minimal surface; no predicate/action split. Alarm side-effects (severity, message) configured out-of-band, not in the script. | | 5 | Alarm fidelity = **full OPC UA Part 9** | Uniform with Galaxy + ALMD on the wire; client-side tooling (HMIs, historians, event pipelines) gets one shape. | -| 6 | Sandbox = **read-only context**; scripts can only read any tag + write to virtual tags | Strict Roslyn `ScriptOptions` allow-list. No HttpClient / File / Process / reflection. | +| 6 | Sandbox = **read-only context**; scripts can only read any tag + write to virtual tags | Strict Roslyn `ScriptOptions` allow-list. Authoritative deny-list (`ForbiddenTypeAnalyzer`): namespace-prefix deny `System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Tasks` (Task / Parallel fan-out — Core.Scripting-003), `System.Runtime.InteropServices`, `Microsoft.Win32`; type-granular deny `System.Environment`, `System.AppDomain`, `System.GC`, `System.Activator`, `System.Threading.Thread` (these live directly in the allow-listed `System` / `System.Threading` namespaces, so a prefix rule cannot reach them without blocking primitives — Core.Scripting-001 / -009). | | 7 | Dependency declaration = **AST inference**; operator doesn't maintain a separate dependency list | `CSharpSyntaxWalker` extracts `ctx.GetTag("path")` string-literal calls at compile time; dynamic paths rejected at publish. | | 8 | Config storage = **config DB with generation-sealed cache** (same as driver instances) | Virtual tags + alarms publish atomically in the same generation as the driver instance config they may depend on. | | 9 | Script return value shape (`ctx.GetTag`) = **`DataValue { Value, StatusCode, Timestamp }`** | Scripts branch on quality naturally without separate `ctx.GetQuality(...)` calls. | @@ -162,7 +162,7 @@ Tie-in capability — **historian alarm sink**: ## Compliance Checks (run at exit gate) -- [ ] **Sandbox escape**: attempts to reference `System.IO.File`, `System.Net.Http.HttpClient`, `System.Diagnostics.Process`, or `typeof(X).Assembly.Load` fail at script compile with an actionable error. +- [ ] **Sandbox escape**: attempts to reference any deny-listed namespace prefix (`System.IO`, `System.Net`, `System.Diagnostics`, `System.Reflection`, `System.Threading.Tasks`, `System.Runtime.InteropServices`, `Microsoft.Win32`) or any of the type-granular forbidden types (`System.Environment`, `System.AppDomain`, `System.GC`, `System.Activator`, `System.Threading.Thread`) fail at script compile with an actionable error. Vectors include direct calls, `typeof(T)`, generic type arguments, casts, `is`/`as` patterns, `default(T)`, array element types, and explicitly-typed local declarations. - [ ] **Dependency inference**: `ctx.GetTag(myStringVar)` (non-literal path) is rejected at publish with a span-pointed error; `ctx.GetTag("Line1/Speed")` is accepted + appears in the inferred input set. - [ ] **Change cascade**: tag A → virtual tag B → virtual tag C. When A changes, B recomputes, then C recomputes. Single change event triggers the full cascade in topological order within one evaluation pass. - [ ] **Cycle rejection**: publish a config where virtual tag B depends on A and A depends on B. Publish fails pre-commit with a clear cycle message. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/CompiledScriptCache.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/CompiledScriptCache.cs index 4b59278..2caaef6 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/CompiledScriptCache.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/CompiledScriptCache.cs @@ -58,8 +58,13 @@ public sealed class CompiledScriptCache } catch { - // Failed compile — evict so a retry with corrected source can succeed. - _cache.TryRemove(key, out _); + // Failed compile — evict the SPECIFIC faulted Lazy instance so a retry with + // corrected source can succeed. The KeyValuePair<,> overload compares the + // value reference, so if two threads race the same bad source both observe + // the same faulted Lazy and both reach this catch, and a concurrent retry + // re-added a fresh Lazy under the same key between the two removals, the + // second removal does NOT evict the in-flight retry. (Core.Scripting-006.) + _cache.TryRemove(new KeyValuePair>>(key, lazy)); throw; } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/DependencyExtractor.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/DependencyExtractor.cs index b764c5d..a19d2d4 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/DependencyExtractor.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting/DependencyExtractor.cs @@ -103,8 +103,14 @@ public static class DependencyExtractor } var pathArg = args[0].Expression; + // Accept any string-literal expression, including raw-string forms which + // tokenize as SingleLineRawStringLiteralToken / MultiLineRawStringLiteralToken + // rather than StringLiteralToken. Checking the expression kind + // (StringLiteralExpression) covers all token kinds Roslyn assigns to literal + // strings, so a """raw""" path is harvested rather than mis-rejected as a + // dynamic path. (Core.Scripting-005.) if (pathArg is not LiteralExpressionSyntax literal - || !literal.Token.IsKind(SyntaxKind.StringLiteralToken)) + || !literal.IsKind(SyntaxKind.StringLiteralExpression)) { _rejections.Add(new DependencyRejection( Span: pathArg.Span, diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/CompiledScriptCacheTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/CompiledScriptCacheTests.cs index 3fa9ce7..46e5ab8 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/CompiledScriptCacheTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/CompiledScriptCacheTests.cs @@ -148,4 +148,77 @@ public sealed class CompiledScriptCacheTests var cache = new CompiledScriptCache(); Should.Throw(() => cache.GetOrCompile(null!)); } + + [Fact] + public void Failed_compile_eviction_does_not_remove_a_concurrent_retry_entry() + { + // Regression for Core.Scripting-006: when a faulted Lazy is observed by a thread, + // the eviction must scope to that specific Lazy instance, not the key. If a + // concurrent retry has already inserted a fresh Lazy under the same key between + // the throw and the catch-block removal, the buggy TryRemove(key, out _) overload + // evicts the retry entry. The fixed TryRemove(KeyValuePair<,>) overload compares + // value identity, so only the faulted Lazy is removed. + // + // Deterministic setup: pre-populate the cache's internal dictionary with a + // faulted Lazy whose factory itself swaps the entry to a fresh Lazy as a side + // effect during the throw. By the time GetOrCompile reaches its catch block, the + // dictionary holds the fresh entry under the same key — exactly the race window + // the finding describes. The fix must leave the fresh entry in place. + var cache = new CompiledScriptCache(); + + // Reach the private _cache + HashSource via reflection — they're private, so + // InternalsVisibleTo doesn't help. + var cacheField = typeof(CompiledScriptCache) + .GetField("_cache", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + cacheField.ShouldNotBeNull(); + var dict = (System.Collections.Concurrent.ConcurrentDictionary< + string, Lazy>>)cacheField!.GetValue(cache)!; + + const string source = """return 7;"""; + var hashSourceMethod = typeof(CompiledScriptCache) + .GetMethod("HashSource", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic); + hashSourceMethod.ShouldNotBeNull(); + var key = (string)hashSourceMethod!.Invoke(null, [source])!; + + // The fresh Lazy is what a concurrent retry would have inserted between the + // faulted-throw and the catch's removal. Materialise it eagerly so we have a + // stable reference to assert identity against. + var fresh = new Lazy>( + () => ScriptEvaluator.Compile(source), + LazyThreadSafetyMode.ExecutionAndPublication); + + // The faulted Lazy throws — but only after swapping its own dictionary entry + // for the fresh Lazy, modelling the race window between the throw and the + // catch-block eviction. + var faulted = new Lazy>( + () => + { + dict[key] = fresh; + throw new InvalidOperationException("bad compile"); + }, + LazyThreadSafetyMode.ExecutionAndPublication); + dict[key] = faulted; + + // Drive GetOrCompile through the public API. It observes the faulted Lazy + // currently under `key`, invokes .Value (which swaps in the fresh Lazy then + // throws), and runs the catch block's eviction. The fix removes only the + // specific faulted Lazy instance; the fresh entry survives. + Should.Throw(() => cache.GetOrCompile(source)); + + dict.ContainsKey(key).ShouldBeTrue( + "the fresh retry entry that won the race must survive the faulted Lazy eviction (Core.Scripting-006)"); + ReferenceEquals(dict[key], fresh).ShouldBeTrue( + "the entry under the key must still be the fresh Lazy — an unconditional TryRemove(key) would have evicted it"); + } + + [Fact] + public void Failed_compile_path_still_evicts_its_own_faulted_entry() + { + // Companion to the race test above: confirm the fix's value-scoped eviction + // still removes the actual faulted Lazy (so retries with corrected source can + // succeed — the original Core.Scripting test that locked the contract). + var cache = new CompiledScriptCache(); + Should.Throw(() => cache.GetOrCompile("""return unknownIdentifier + 1;""")); + cache.Count.ShouldBe(0, "faulted Lazy must still be evicted after compile failure"); + } } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/DependencyExtractorTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/DependencyExtractorTests.cs index aaa6751..07ce0d9 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/DependencyExtractorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/DependencyExtractorTests.cs @@ -209,4 +209,33 @@ public sealed class DependencyExtractorTests result.IsValid.ShouldBeTrue(); result.Reads.Count.ShouldBe(2); } + + [Fact] + public void Accepts_single_line_raw_string_literal_path() + { + // A single-line raw string literal ("""Line1/Speed""") tokenizes as + // SingleLineRawStringLiteralToken, not StringLiteralToken — the old check + // would mis-reject it as a "dynamic path". Confirm static raw-string paths are + // harvested. (Core.Scripting-005.) + var src = "return ctx.GetTag(\"\"\"Line1/Speed\"\"\").Value;"; + var result = DependencyExtractor.Extract(src); + result.IsValid.ShouldBeTrue(); + result.Reads.ShouldContain("Line1/Speed"); + result.Rejections.ShouldBeEmpty(); + } + + [Fact] + public void Accepts_multi_line_raw_string_literal_path() + { + // A multi-line raw string literal tokenizes as MultiLineRawStringLiteralToken. + // Even though it is unusual for a tag path, it is still a static string and + // must not be mis-rejected. (Core.Scripting-005.) + // Note: the multi-line raw string strips the common leading indent and the + // surrounding newlines, leaving exactly the body text. + var src = "return ctx.GetTag(\"\"\"\nLine1/Speed\n\"\"\").Value;"; + var result = DependencyExtractor.Extract(src); + result.IsValid.ShouldBeTrue(); + result.Reads.ShouldContain("Line1/Speed"); + result.Rejections.ShouldBeEmpty(); + } } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptContextTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptContextTests.cs new file mode 100644 index 0000000..d223f40 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptContextTests.cs @@ -0,0 +1,70 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Scripting; + +namespace ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests; + +/// +/// Locks the boundary semantics of . The helper +/// is the canonical "ignore small noise" predicate alarm authors compose into bigger +/// expressions; subtle sign / boundary changes here would silently move the +/// active-state edge of every consuming alarm. (Core.Scripting-011.) +/// +[Trait("Category", "Unit")] +public sealed class ScriptContextTests +{ + [Fact] + public void Deadband_returns_false_when_difference_equals_tolerance() + { + // Strict greater-than comparison: a delta exactly equal to tolerance is + // considered "within deadband" and must NOT trip. + ScriptContext.Deadband(current: 10.5, previous: 10.0, tolerance: 0.5).ShouldBeFalse(); + } + + [Fact] + public void Deadband_returns_true_when_difference_just_exceeds_tolerance() + { + // Any delta strictly greater than tolerance trips the deadband. + ScriptContext.Deadband(current: 10.6, previous: 10.0, tolerance: 0.5).ShouldBeTrue(); + } + + [Fact] + public void Deadband_returns_false_when_difference_just_below_tolerance() + { + ScriptContext.Deadband(current: 10.4, previous: 10.0, tolerance: 0.5).ShouldBeFalse(); + } + + [Fact] + public void Deadband_is_symmetric_in_direction_of_change() + { + // Math.Abs ensures the helper trips identically whether the value rose or fell. + ScriptContext.Deadband(current: 9.0, previous: 10.0, tolerance: 0.5).ShouldBeTrue(); + ScriptContext.Deadband(current: 11.0, previous: 10.0, tolerance: 0.5).ShouldBeTrue(); + } + + [Fact] + public void Deadband_returns_false_when_values_are_equal() + { + ScriptContext.Deadband(current: 10.0, previous: 10.0, tolerance: 0.001).ShouldBeFalse(); + } + + [Fact] + public void Deadband_with_zero_tolerance_returns_true_for_any_difference() + { + // Zero-tolerance is the "trip on any non-equal change" mode. Equality still + // returns false (delta 0 is not strictly greater than 0). + ScriptContext.Deadband(current: 10.0, previous: 10.0, tolerance: 0).ShouldBeFalse(); + ScriptContext.Deadband(current: 10.0, previous: 10.000001, tolerance: 0).ShouldBeTrue(); + } + + [Fact] + public void Deadband_with_negative_tolerance_always_trips_for_unequal_values() + { + // A negative tolerance is nonsensical input but the helper does not guard + // against it — Math.Abs(delta) is always >= 0, so the comparison is + // effectively "any non-equal change". Lock this so a refactor that adds + // (or removes) input validation requires an explicit test update. + ScriptContext.Deadband(current: 10.0, previous: 10.5, tolerance: -1.0).ShouldBeTrue(); + ScriptContext.Deadband(current: 10.0, previous: 10.0, tolerance: -1.0).ShouldBeTrue(); + } +} diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptLogCompanionSinkTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptLogCompanionSinkTests.cs index 08ca420..a701d3d 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptLogCompanionSinkTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptLogCompanionSinkTests.cs @@ -152,4 +152,47 @@ public sealed class ScriptLogCompanionSinkTests scriptLogger.ForContext(ScriptLoggerFactory.ScriptNameProperty, "X").Fatal("fatal"); mainSink.Events.Count.ShouldBe(1); } + + [Fact] + public void Factory_plus_companion_sink_integration_surfaces_script_error_in_both_logs() + { + // End-to-end Core.Scripting-011 coverage: an Error-level emission from a logger + // built by ScriptLoggerFactory must land in BOTH the per-script sink (acting as + // the scripts-*.log substitute here) AND the main-log companion at Warning level, + // tagged with the originating ScriptName property. This is the integration the + // server's logger pipeline relies on so operators see script failures in the + // primary log without monitoring the scripts file separately. + var scriptsSink = new CapturingSink(); + var mainSink = new CapturingSink(); + + // Main pipeline — receives only Error+ events forwarded by the companion sink. + var mainLogger = new LoggerConfiguration() + .MinimumLevel.Verbose().WriteTo.Sink(mainSink).CreateLogger(); + + // Root script pipeline — fans out to the scripts sink (stand-in for the + // rolling scripts-*.log file) AND the companion sink that forwards Error+ + // to the main logger. + var rootScriptLogger = new LoggerConfiguration() + .MinimumLevel.Verbose() + .WriteTo.Sink(scriptsSink) + .WriteTo.Sink(new ScriptLogCompanionSink(mainLogger)) + .CreateLogger(); + + // Factory binds the per-script ScriptName property — this is the only way the + // mirror knows which script raised the event. + var factory = new ScriptLoggerFactory(rootScriptLogger); + var perScript = factory.Create("HighTemp"); + perScript.Error("threshold exceeded"); + + // Scripts sink saw the Error at its original level with the ScriptName bound. + scriptsSink.Events.Count.ShouldBe(1); + scriptsSink.Events[0].Level.ShouldBe(LogEventLevel.Error); + ((ScalarValue)scriptsSink.Events[0].Properties[ScriptLoggerFactory.ScriptNameProperty]).Value.ShouldBe("HighTemp"); + + // Main sink saw a Warning-level companion entry tagged with the same ScriptName. + mainSink.Events.Count.ShouldBe(1); + mainSink.Events[0].Level.ShouldBe(LogEventLevel.Warning); + ((ScalarValue)mainSink.Events[0].Properties[ScriptLoggerFactory.ScriptNameProperty]).Value.ShouldBe("HighTemp"); + ((ScalarValue)mainSink.Events[0].Properties["OriginalLevel"]).Value.ShouldBe(LogEventLevel.Error); + } } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxBuildTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxBuildTests.cs new file mode 100644 index 0000000..c3dad5b --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests/ScriptSandboxBuildTests.cs @@ -0,0 +1,56 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Scripting; + +namespace ZB.MOM.WW.OtOpcUa.Core.Scripting.Tests; + +/// +/// Covers the argument-validation guards. The +/// guards are the only call-site protection against a typo / mis-wired context +/// type silently producing a sandbox missing the concrete context's assembly +/// reference; without coverage, the guards could be deleted by a refactor without +/// any test failing. (Core.Scripting-011.) +/// +[Trait("Category", "Unit")] +public sealed class ScriptSandboxBuildTests +{ + [Fact] + public void Null_context_type_throws_ArgumentNullException() + { + Should.Throw(() => ScriptSandbox.Build(null!)); + } + + [Fact] + public void Non_ScriptContext_type_throws_ArgumentException() + { + // ScriptSandbox must reject types that don't derive from ScriptContext — + // ScriptGlobals is constrained where TContext : ScriptContext, so + // sneaking a non-derived type past Build would later blow up inside Roslyn. + Should.Throw(() => ScriptSandbox.Build(typeof(string))) + .ParamName.ShouldBe("contextType"); + } + + [Fact] + public void Abstract_ScriptContext_base_type_is_accepted() + { + // The base ScriptContext type satisfies the IsAssignableFrom check, so the + // factory must not reject it even though it cannot be instantiated directly. + // Callers wiring a base-typed sandbox up for diagnostics rely on this. + var options = ScriptSandbox.Build(typeof(ScriptContext)); + options.ShouldNotBeNull(); + } + + [Fact] + public void Concrete_subclass_is_accepted_and_its_assembly_referenced() + { + // The concrete context type's assembly must end up in the allow-listed + // references — otherwise Roslyn cannot resolve ScriptGlobals at + // compile. We can't easily inspect the ScriptOptions metadata references + // by-assembly cross-version, so we exercise the end-to-end path instead: a + // script compiled against FakeScriptContext must successfully reach a + // FakeScriptContext member. + var evaluator = ScriptEvaluator.Compile( + """return (double)ctx.GetTag("X").Value;"""); + evaluator.ShouldNotBeNull(); + } +}