From 145b06bec9d8c22be96dea8f405d50246b8e7999 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 11:06:56 -0400 Subject: [PATCH] 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. --- .../Core.Scripting.Abstractions/findings.md | 208 ++++++++++++++++++ .../AlarmPredicateContext.cs | 12 +- .../ScriptContext.cs | 15 +- .../VirtualTagContext.cs | 2 +- 4 files changed, 227 insertions(+), 10 deletions(-) create mode 100644 code-reviews/Core.Scripting.Abstractions/findings.md diff --git a/code-reviews/Core.Scripting.Abstractions/findings.md b/code-reviews/Core.Scripting.Abstractions/findings.md new file mode 100644 index 00000000..70107594 --- /dev/null +++ b/code-reviews/Core.Scripting.Abstractions/findings.md @@ -0,0 +1,208 @@ +# Code Review — Core.Scripting.Abstractions + + + +| 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 + + + +### 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 `` 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 `` 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): + +``` +/// The minimum change magnitude to detect (must be >= 0). +/// Negative values cause the function to always return true; NaN always returns false. +``` + +**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 `` 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 `` 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`) ``: "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`.)_ diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs index 6232d0a5..f57cff3b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/AlarmPredicateContext.cs @@ -11,9 +11,11 @@ namespace ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms; /// order or drive cascade behavior. /// /// -/// Per Phase 7 plan Shape A decision, alarm scripts are one-script-per-alarm -/// returning bool. 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 +/// docs/v2/implementation/phase-7-scripting-and-alarming.md, decision row 4), +/// alarm scripts are one-script-per-alarm returning bool. They read any tag +/// they want but should not write anything (the owning alarm's state is tracked by the +/// engine, not the script). /// 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()); } /// Rejects virtual tag writes for pure predicate semantics. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs index 8b6993dd..c0d0720b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/ScriptContext.cs @@ -6,9 +6,10 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Scripting; /// /// The API user scripts see as the global ctx. Abstract — concrete subclasses /// (e.g. VirtualTagScriptContext, AlarmScriptContext) 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 docs/v2/plan.md): scripts can read any tag, write only to virtual +/// tags, and have no other .NET reach — no HttpClient, no File, no Process, no +/// reflection. /// /// /// @@ -39,6 +40,11 @@ public abstract class ScriptContext /// publish by DependencyExtractor. This is intentional: the static /// dependency set is required for the change-driven scheduler to subscribe to the /// right upstream tags at load time. + /// + /// 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. + /// /// /// The literal tag path to read. public abstract DataValueSnapshot GetTag(string path); @@ -52,7 +58,8 @@ public abstract class ScriptContext /// /// Path rules identical to — literal only, dependency /// extractor tracks the write targets so the engine knows what downstream - /// subscribers to notify. + /// subscribers to notify. The {{equip}} token is supported in the same way + /// as for . /// /// The literal tag path to write to. /// The value to write to the virtual tag. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/VirtualTagContext.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/VirtualTagContext.cs index fafff3fb..fbfae77c 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/VirtualTagContext.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting.Abstractions/VirtualTagContext.cs @@ -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 ctx. Mutable state across runs is a future decision (e.g. a -/// dedicated ctx.Memory dictionary); not in scope for Phase 7. +/// dedicated ctx.Memory dictionary); not currently in scope. /// /// /// The clock is injectable so tests can pin time