review(Core.Scripting.Abstractions): refresh stale Phase7 labels + document {{equip}}

First review at 7286d320. Five Low doc fixes (BadNodeIdUnknown comment parity, three stale Phase7 labels -> design-doc cites, {{equip}} token doc on GetTag/SetVirtualTag). Deadband NaN/negative-tolerance (004) + a stale docs path (007) left Open.
2026-06-19 11:06:56 -04:00
parent 38c48a009c
commit 145b06bec9
4 changed files with 227 additions and 10 deletions
@@ -0,0 +1,208 @@
+# Code Review — Core.Scripting.Abstractions
+
+<!-- Template for a per-module findings file. Copy to code-reviews/<Module>/findings.md.
+     See ../../REVIEW-PROCESS.md for the full process. The base README.md is generated
+     from these files by regen-readme.py — do not edit README.md by hand. -->
+
+| Field | Value |
+|---|---|
+| Module | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions` |
+| Reviewer | Claude Code |
+| Review date | 2026-06-19 |
+| Commit reviewed | `7286d320` |
+| Status | Reviewed |
+| Open findings | 2 |
+
+## Checklist coverage
+
+A comprehensive review completes every category, recording "No issues found" where
+a category produced nothing rather than leaving it blank.
+
+| # | Category | Result |
+|---|---|---|
+| 1 | Correctness & logic bugs | 1 finding (Core.Scripting.Abstractions-004) |
+| 2 | OtOpcUa conventions | No issues found |
+| 3 | Concurrency & thread safety | No issues found |
+| 4 | Error handling & resilience | No issues found |
+| 5 | Security | No issues found |
+| 6 | Performance & resource management | No issues found |
+| 7 | Design-document adherence | 1 finding (Core.Scripting.Abstractions-007) |
+| 8 | Code organization & conventions | No issues found |
+| 9 | Testing coverage | No issues found |
+| 10 | Documentation & comments | 5 findings (001–003, 005–006) |
+
+## Findings
+
+<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
+     never reused. Findings are never deleted — close them by changing Status and
+     completing Resolution. -->
+
+### Core.Scripting.Abstractions-001
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Documentation & comments |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs:43,46` |
+| Status | Resolved |
+
+**Description:** `AlarmPredicateContext.GetTag` uses the magic constant `0x80340000u` twice with no
+inline comment. The sibling `VirtualTagContext.GetTag` annotates both sites with
+`/* BadNodeIdUnknown */`, making the intent immediately clear. The inconsistency makes the alarm
+context harder to audit for correctness.
+
+**Recommendation:** Add `/* BadNodeIdUnknown */` inline comments matching the pattern in
+`VirtualTagContext.cs:55,58`.
+
+**Resolution:** Applied inline — added `/* BadNodeIdUnknown */` comments to both `0x80340000u`
+literals in `AlarmPredicateContext.GetTag`. Verified by build (no test project for this module).
+2026-06-19.
+
+---
+
+### Core.Scripting.Abstractions-002
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Documentation & comments |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs:14` |
+| Status | Resolved |
+
+**Description:** The `<remarks>` block opens with "Per Phase 7 plan Shape A decision...". The
+project-wide rename (master commit `40e8a23e`) retired the "Phase 7" prefix for the address-space
+pipeline; comments referencing it by that name are stale and confuse new readers. The authoritative
+plan document is `docs/v2/implementation/phase-7-scripting-and-alarming.md` (decision row 4, Shape
+A), so the relevant fix is to cite the design doc rather than the internal plan-phase label.
+
+**Recommendation:** Replace "Per Phase 7 plan Shape A decision" with "Per the scripting design
+(see `docs/v2/implementation/phase-7-scripting-and-alarming.md`, decision row 4)".
+
+**Resolution:** Applied inline — updated the stale "Phase 7 plan Shape A decision" reference to
+cite the design doc directly. Verified by build (no test project for this module). 2026-06-19.
+
+---
+
+### Core.Scripting.Abstractions-003
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Documentation & comments |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs:9,10` |
+| Status | Resolved |
+
+**Description:** The `ScriptContext` summary comment reads "Phase 7 plan decision #6: scripts can
+read any tag, write only to virtual tags...". The "Phase 7 plan" label is stale after the global
+rename. Decision #6 in `docs/v2/plan.md` remains the correct reference, but the wording should
+match the current naming convention.
+
+**Recommendation:** Replace "Phase 7 plan decision #6" with "plan decision #6 (see
+`docs/v2/plan.md`)".
+
+**Resolution:** Applied inline — updated the stale "Phase 7 plan decision #6" reference to use the
+current naming convention. Verified by build (no test project for this module). 2026-06-19.
+
+---
+
+### Core.Scripting.Abstractions-004
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Correctness & logic bugs |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs:84` |
+| Status | Open |
+
+**Description:** `ScriptContext.Deadband` has no guard or documentation for invalid `tolerance`
+values. A negative `tolerance` makes `Math.Abs(current - previous) > tolerance` trivially `true`
+for all finite inputs (any absolute value exceeds a negative number), so `Deadband(x, y, -1.0)`
+always reports a change regardless of the actual difference — silently inverting the intended
+semantics. A NaN `tolerance` makes the comparison always `false` (no change ever detected). Neither
+case is caught by the compiler and there is no runtime warning, so script authors have no feedback
+when they supply an invalid threshold.
+
+**Recommendation:** Document the behavior clearly in the `<param name="tolerance">` XML doc. If a
+design decision allows it, add an `ArgumentOutOfRangeException` guard for `tolerance < 0`, but this
+changes the public API contract for authored scripts and needs a plan-level decision. Minimum fix
+(doc-only, zero-risk):
+
+```
+/// <param name="tolerance">The minimum change magnitude to detect (must be &gt;= 0).
+///     Negative values cause the function to always return true; NaN always returns false.</param>
+```
+
+**Resolution:** _(Open — the doc-only fix is zero-risk but the tolerance param is part of the
+locked script API surface; the decision on whether to also add a guard belongs at the plan level.
+Deferred until that call is made. No test project exists for this module.)_
+
+---
+
+### Core.Scripting.Abstractions-005
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Documentation & comments |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/VirtualTagContext.cs:21` |
+| Status | Resolved |
+
+**Description:** The `<remarks>` second paragraph ends with "Mutable state across runs is a future
+decision (e.g. a dedicated `ctx.Memory` dictionary); not in scope for Phase 7." The "Phase 7"
+label is stale (see findings 002 and 003). The forward-looking note itself is still valid.
+
+**Recommendation:** Replace "not in scope for Phase 7" with "not currently in scope".
+
+**Resolution:** Applied inline — removed the stale Phase 7 label. Verified by build (no test
+project for this module). 2026-06-19.
+
+---
+
+### Core.Scripting.Abstractions-006
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Documentation & comments |
+| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs:35-36,55-58` |
+| Status | Resolved |
+
+**Description:** The `<summary>` for `ScriptContext.GetTag` describes the path syntax as
+"forward-slash delimited, matching the Equipment-namespace browse tree" but makes no mention of the
+`{{equip}}` equipment-relative token introduced in master commit `27c34a55`. Operators reading the
+XML doc for IntelliSense will not know the token exists. The `SetVirtualTag` remarks similarly omit
+it.
+
+**Recommendation:** Add a note to the `GetTag` (and `SetVirtualTag`) `<remarks>`: "The reserved
+`{{equip}}` token may appear in the path literal; it is substituted with the owning equipment's
+tag-base prefix at deployment time. See `docs/ScriptEditor.md` — Equipment-relative tag paths."
+
+**Resolution:** Applied inline — added `{{equip}}` token mention to `GetTag` and `SetVirtualTag`
+remarks in `ScriptContext.cs`. Verified by build (no test project for this module). 2026-06-19.
+
+---
+
+### Core.Scripting.Abstractions-007
+
+| Field | Value |
+|---|---|
+| Severity | Low |
+| Category | Design-document adherence |
+| Location | `docs/ScriptedAlarms.md:298` |
+| Status | Open |
+
+**Description:** `docs/ScriptedAlarms.md` line 298 lists the file path as
+`src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/AlarmPredicateContext.cs`. The file was moved to
+`src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs` as part of the
+A0 memory-reduction plan (`docs/plans/2026-06-07-virtualtag-script-memory.md`, step 1 `git mv`
+list). The stale path sends readers to a non-existent location. The namespace was intentionally
+preserved (`Core.ScriptedAlarms`) so no `using` changes were needed — the file location is the
+only thing that changed.
+
+**Recommendation:** Update `docs/ScriptedAlarms.md` line 298:
+```
+- `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs` — script-side `ScriptContext` (read-only, write rejected)
+```
+
+**Resolution:** _(Open — out of scope for this module's src-only fix pass. Should be fixed in a
+docs-cleanup commit targeting `docs/ScriptedAlarms.md`.)_
@@ -11,9 +11,11 @@ namespace ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms;
 ///     order or drive cascade behavior.
 /// </summary>
 /// <remarks>
-///     Per Phase 7 plan Shape A decision, alarm scripts are one-script-per-alarm
-///     returning <c>bool</c>. They read any tag they want but should not write
-///     anything (the owning alarm's state is tracked by the engine, not the script).
+///     Per the scripting design (see
+///     <c>docs/v2/implementation/phase-7-scripting-and-alarming.md</c>, decision row 4),
+///     alarm scripts are one-script-per-alarm returning <c>bool</c>. They read any tag
+///     they want but should not write anything (the owning alarm's state is tracked by the
+///     engine, not the script).
 /// </remarks>
 public sealed class AlarmPredicateContext : ScriptContext
 {
@@ -40,10 +42,10 @@ public sealed class AlarmPredicateContext : ScriptContext
    public override DataValueSnapshot GetTag(string path)
    {
        if (string.IsNullOrWhiteSpace(path))
-            return new DataValueSnapshot(null, 0x80340000u, null, _clock());
+            return new DataValueSnapshot(null, 0x80340000u /* BadNodeIdUnknown */, null, _clock());
        return _readCache.TryGetValue(path, out var v)
            ? v
-            : new DataValueSnapshot(null, 0x80340000u, null, _clock());
+            : new DataValueSnapshot(null, 0x80340000u /* BadNodeIdUnknown */, null, _clock());
    }

    /// <summary>Rejects virtual tag writes for pure predicate semantics.</summary>
@@ -6,9 +6,10 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting;
 /// <summary>
 ///     The API user scripts see as the global <c>ctx</c>. Abstract — concrete subclasses
 ///     (e.g. <c>VirtualTagScriptContext</c>, <c>AlarmScriptContext</c>) plug in the
-///     actual tag-backend + logger + virtual-tag writer for each evaluation. Phase 7 plan
-///     decision #6: scripts can read any tag, write only to virtual tags, and have no
-///     other .NET reach — no HttpClient, no File, no Process, no reflection.
+///     actual tag-backend + logger + virtual-tag writer for each evaluation. Plan decision
+///     #6 (see <c>docs/v2/plan.md</c>): scripts can read any tag, write only to virtual
+///     tags, and have no other .NET reach — no HttpClient, no File, no Process, no
+///     reflection.
 /// </summary>
 /// <remarks>
 ///     <para>
@@ -39,6 +40,11 @@ public abstract class ScriptContext
    ///     publish by <c>DependencyExtractor</c>. This is intentional: the static
    ///     dependency set is required for the change-driven scheduler to subscribe to the
    ///     right upstream tags at load time.
+    ///     <para>
+    ///         The reserved <c>{{equip}}</c> token may appear in the path literal; it is
+    ///         substituted with the owning equipment's tag-base prefix at deployment time.
+    ///         See <c>docs/ScriptEditor.md</c> — Equipment-relative tag paths.
+    ///     </para>
    /// </remarks>
    /// <param name="path">The literal tag path to read.</param>
    public abstract DataValueSnapshot GetTag(string path);
@@ -52,7 +58,8 @@ public abstract class ScriptContext
    /// <remarks>
    ///     Path rules identical to <see cref="GetTag"/> — literal only, dependency
    ///     extractor tracks the write targets so the engine knows what downstream
-    ///     subscribers to notify.
+    ///     subscribers to notify. The <c>{{equip}}</c> token is supported in the same way
+    ///     as for <see cref="GetTag"/>.
    /// </remarks>
    /// <param name="path">The literal tag path to write to.</param>
    /// <param name="value">The value to write to the virtual tag.</param>
@@ -18,7 +18,7 @@ namespace ZB.MOM.WW.OtOpcUa.Core.VirtualTags;
 ///         constructs a fresh context for every run — cheap because the constructor
 ///         just captures references — so scripts can't cache mutable state across runs
 ///         via <c>ctx</c>. Mutable state across runs is a future decision (e.g. a
-///         dedicated <c>ctx.Memory</c> dictionary); not in scope for Phase 7.
+///         dedicated <c>ctx.Memory</c> dictionary); not currently in scope.
 ///     </para>
 ///     <para>
 ///         The <see cref="Now"/> clock is injectable so tests can pin time