diff --git a/docs/known-issues/2026-06-24-template-inheritance-ui-and-cli-followups.md b/docs/known-issues/2026-06-24-template-inheritance-ui-and-cli-followups.md index ed0eff2e..b07f7198 100644 --- a/docs/known-issues/2026-06-24-template-inheritance-ui-and-cli-followups.md +++ b/docs/known-issues/2026-06-24-template-inheritance-ui-and-cli-followups.md @@ -1,8 +1,10 @@ # Follow-up tracker — template-inheritance UI gaps + CLI/validation footguns (2026-06-24 session) -**Status:** OPEN (tracker) · **Found:** 2026-06-24 · **Context:** live ops session on `wonder-app-vd03` (CvdReactor / Z28061 / Z28061Sim) — renaming the template, adding the LeakTest module, and adding MoveInType to the MESReceiver children. +**Status:** PARTIALLY RESOLVED · **Found:** 2026-06-24 · **Context:** live ops session on `wonder-app-vd03` (CvdReactor / Z28061 / Z28061Sim) — renaming the template, adding the LeakTest module, and adding MoveInType to the MESReceiver children. **Components:** Central UI (#9), Template Engine (#1), CLI (#19), Configuration Database (#17) +**Resolved:** #3 (collision detector) and #7 (sandbox compile surface) fixed on branch `fix/followups-3-7` (2026-06-24). Open: #1, #2, #4, #5, #6, #8. + Issues are listed worst-first. Severities are author estimates. None caused data loss; the runtime/flattened config and deployed instances are correct. --- @@ -32,7 +34,10 @@ Issues are listed worst-first. Severities are author estimates. None caused data --- ## 3. Collision detector blocks adding attributes/compositions to ANY derived template -**Severity:** Medium-High · **Components:** Template Engine (#1) +**Severity:** Medium-High · **Components:** Template Engine (#1) · **✅ RESOLVED 2026-06-24 (branch `fix/followups-3-7`)** + +**Fix:** `CollectDirectMembers` now takes a `skipInherited` flag and skips `IsInherited` placeholder rows for the direct-template and inherited-parent walks (where the inheritance walk already re-adds those members under the parent's origin), while keeping them for the composed-module walk (the sole representation of a derived module's inherited members). Covered by `CollisionDetectorTests` (`DerivedTemplateWithInheritedPlaceholders_NoFalseCollision`, `MultiLevelInheritedPlaceholders_NoFalseCollision`, `DerivedTemplate_GenuineCollisionStillDetected_DespiteInheritedPlaceholder`) and the end-to-end `TemplateServiceTests.AddAttribute_ToDerivedTemplateWithInheritedPlaceholders_Succeeds`. Documented in `Component-TemplateEngine.md` → Naming Collision Detection. + **Symptom:** `template attribute add --template-id 5 --name MoveInType ...` fails with **13** "Naming collision" errors — the new attribute *plus all 12 pre-existing inherited rows*. Same class of failure when adding a composition to a derived template (hit earlier when trying to add `LeakTest` to `LeftReactorSide`). @@ -74,7 +79,10 @@ Issues are listed worst-first. Severities are author estimates. None caused data --- ## 7. Central UI script editor false-flags batch/wait helpers (sandbox compile surface out of sync) -**Severity:** Medium · **Components:** Central UI (#9), Script Analysis (#25) +**Severity:** Medium · **Components:** Central UI (#9), Script Analysis (#25) · **✅ RESOLVED 2026-06-24 (branch `fix/followups-3-7`)** + +**Fix:** `SandboxAttributeAccessor` now mirrors the runtime `AttributeAccessor` — added `WriteBatchAndWaitAsync`, both `WaitAsync` overloads, and both `WaitForAsync` overloads with matching signatures. They throw a clearly-labelled `ScriptSandboxException` if exercised in Test Run (the central sandbox has no device-batch/event-waiter transport), but they now resolve at compile time so the editor stops false-flagging valid scripts. A reflection parity test isn't feasible across the CentralUI→SiteRuntime boundary (Central UI does not reference Site Runtime by design), so the guard is representative-script "diagnose clean" tests in `ScriptAnalysisServiceTests` (`InstanceScript_BatchAndWaitHelpers_DiagnoseClean`, `ChildInstanceScript_WriteBatchAndWait_DiagnoseClean`), consistent with how the inbound `Database`/`WaitForAttribute` and `Notify` surfaces are guarded. Documented in `Component-ScriptAnalysis.md` → Parity guard. + **Symptom:** In the template script editor (`/design/templates/{id}` → Scripts → Edit → Code), a script that calls `Attributes.WriteBatchAndWaitAsync(...)` (or on a child, `Children["X"].Attributes.WriteBatchAndWaitAsync(...)`) shows a red compile error: `'SandboxAttributeAccessor' does not contain a definition for 'WriteBatchAndWaitAsync' ... (CS1061)`. Confirmed on `CvdReactor.MesMoveIn`; the same false error hits the base `MESReceiver.MoveIn`/`MoveOut`, which also use the helper. @@ -87,5 +95,11 @@ Issues are listed worst-first. Severities are author estimates. None caused data --- -### Minor note (not tracked separately) -The deploy-time unbound-binding validation returns one giant semicolon-joined error string (one clause per attribute). For 50–194 unbound attrs it's a wall of text; a structured/summarized error (count + grouped-by-module) would be friendlier. +## 8. Deploy-time unbound-binding validation returns one giant semicolon-joined error string +**Severity:** Low · **Components:** Template Engine (#1), Deployment Manager (#2) + +**Symptom:** Deploying an instance whose data-sourced attributes aren't all bound fails with a single error that concatenates one clause per attribute: `Pre-deployment validation failed: Attribute 'LeftReactorSide.LeakTest.DeltaVac' has a data source reference but no connection binding; Attribute 'LeftReactorSide.LeakTest.ResultType' has …; …`. For 50–194 unbound attrs (e.g. Z28062's unbound LeakTest members) it's a wall of text that's hard to scan in a CLI/UI toast. + +**Root cause:** `ValidateConnectionBindingCompleteness` emits one clause per unbound attribute and joins them into a flat string; there is no grouping or count. + +**Suggested fix:** return a structured/summarized error — leading count (`52 attributes are unbound`) + grouped-by-module breakdown (or a capped list with "…and N more") — instead of the flat semicolon-joined dump. Keep the full list available in a detail/expandable view or the deploy log. diff --git a/docs/requirements/Component-ScriptAnalysis.md b/docs/requirements/Component-ScriptAnalysis.md index d31dd888..2b25704e 100644 --- a/docs/requirements/Component-ScriptAnalysis.md +++ b/docs/requirements/Component-ScriptAnalysis.md @@ -142,6 +142,8 @@ Mirrors `TriggerExpressionGlobals` in the same way. Used by `ValidationService.C A reflection-based parity test in `SiteRuntime.Tests` compares the public member names on `ScriptCompileSurface` against `ScriptGlobals` (and `TriggerCompileSurface` against `TriggerExpressionGlobals`). Any drift between the stub and the real globals causes this test to fail, ensuring the stubs cannot silently fall out of sync. +There is a **third** hand-maintained mirror of the runtime globals: the Central UI Test-Run host `SandboxScriptHost` (see REQ-SA-5 / Interactions). Because Central UI deliberately does not reference Site Runtime, it cannot share the reflection-based parity test above; instead it is guarded by representative-script "diagnose clean" tests in `CentralUI.Tests` (one per non-trivial surface — e.g. the `Attributes.WriteBatchAndWaitAsync` / `WaitAsync` / `WaitForAsync` batch-write-and-wait helpers, the inbound `Database`/`WaitForAttribute` helpers, and the `Notify` outbox shape). A member that drifts out of `SandboxScriptHost` does **not** fail the deploy gate (which compiles against `ScriptCompileSurface`) — it surfaces only as an in-editor `CS1061` false error against otherwise-valid scripts, so these compile-clean tests are the safety net for that surface. + --- ### REQ-SA-5: Consumer Delegation diff --git a/docs/requirements/Component-TemplateEngine.md b/docs/requirements/Component-TemplateEngine.md index 636f46c7..33b07de1 100644 --- a/docs/requirements/Component-TemplateEngine.md +++ b/docs/requirements/Component-TemplateEngine.md @@ -124,6 +124,8 @@ When a template composes two or more feature modules, the system must check for If any composed module introduces a name that already exists (from another composed module or from the composing template itself), this is a **design-time error**. The template cannot be saved until the conflict is resolved. Collision detection is performed recursively for nested module compositions. +A derived template stores `IsInherited` placeholder rows that mirror the members it inherits from its parent chain. These placeholders are **excluded** from collision detection on the template's own member set and on the inherited-parent walk — the inheritance walk already re-adds those members under the parent's origin, so counting the placeholder copies as well would report a spurious self-collision against the very member they mirror. (Placeholders are only counted when a composed module is itself a derived template, because the composed-module walk does not climb that module's parent chain and the placeholders are then the sole representation of its inherited members.) Without this exclusion there is no supported way to add an attribute, alarm, script, or composition to a derived template — every add is blocked by false collisions on the pre-existing inherited rows. + ## Flattening Process When an instance is deployed, the Template Engine resolves the full configuration: diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs index a2367b83..39b774e5 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs @@ -313,6 +313,59 @@ public class SandboxAttributeAccessor _ctx.SetAttribute(Resolve(key), value?.ToString() ?? string.Empty); return Task.CompletedTask; } + + // Batch-write/wait helpers. These mirror the runtime AttributeAccessor + // (SiteRuntime/Scripts/ScopeAccessors.cs) and the deploy-gate + // ScriptCompileSurface member-for-member so instance scripts using them COMPILE + // in the editor and pass Test Run analysis (follow-up #7) — previously the + // sandbox omitted them and the editor false-flagged valid scripts with CS1061. + // Execution needs the site's DCL batch path + event-driven attribute waiter, + // for which the central Test Run sandbox has no transport, so each throws a + // clearly-labelled ScriptSandboxException; the same code validates/deploys/runs + // unchanged at a site. + + /// + /// Sandbox stand-in for AttributeAccessor.WriteBatchAndWaitAsync: present + /// for editor/compile parity, throws when run + /// in Test Run (no device batch-write transport here). + /// + public Task WriteBatchAndWaitAsync( + IReadOnlyDictionary values, string flagKey, object? flagValue, + string responseKey, object? responseValue, TimeSpan timeout) + => throw NotInSandbox(nameof(WriteBatchAndWaitAsync)); + + /// + /// Sandbox stand-in for AttributeAccessor.WaitAsync (value-equality form); + /// see . + /// + public Task WaitAsync(string key, object? targetValue, TimeSpan timeout, bool requireGoodQuality = false) + => throw NotInSandbox(nameof(WaitAsync)); + + /// + /// Sandbox stand-in for AttributeAccessor.WaitAsync (predicate form); + /// see . + /// + public Task WaitAsync(string key, Func predicate, TimeSpan timeout, bool requireGoodQuality = false) + => throw NotInSandbox(nameof(WaitAsync)); + + /// + /// Sandbox stand-in for AttributeAccessor.WaitForAsync (value-equality form); + /// see . + /// + public Task WaitForAsync(string key, object? targetValue, TimeSpan timeout, bool requireGoodQuality = false) + => throw NotInSandbox(nameof(WaitForAsync)); + + /// + /// Sandbox stand-in for AttributeAccessor.WaitForAsync (predicate form); + /// see . + /// + public Task WaitForAsync(string key, Func predicate, TimeSpan timeout, bool requireGoodQuality = false) + => throw NotInSandbox(nameof(WaitForAsync)); + + private static ScriptSandboxException NotInSandbox(string member) => + new($"{member}(...) drives live device tags and the site's event-driven " + + "attribute waiter, which aren't available in the central Test Run sandbox — " + + "deploy to a site to exercise batch-write/wait handshakes."); } /// diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs index bf496ef5..aca705a4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs @@ -33,8 +33,9 @@ public static class CollisionDetector var lookup = CycleDetector.BuildLookup(allTemplates); var allMembers = new List(); - // Collect direct (top-level) members - CollectDirectMembers(template, prefix: null, originPrefix: template.Name, allMembers); + // Collect direct (top-level) members. Inherited placeholder rows are skipped: + // the inheritance walk below re-adds them under the parent's origin. + CollectDirectMembers(template, prefix: null, originPrefix: template.Name, allMembers, skipInherited: true); // Collect members from composed modules recursively foreach (var composition in template.Compositions) @@ -75,26 +76,41 @@ public static class CollisionDetector return collisions; } + /// + /// When true, IsInherited placeholder rows are skipped. Those rows + /// are materialized copies of members the template inherits from its parent + /// chain, and they are ALSO re-added — under the parent's name — by + /// . Counting both yielded two distinct + /// origins for the same canonical name and a spurious collision, which blocked + /// every attempt to add an attribute/composition to a derived template + /// (follow-up #3). Pass false only for the composed-module walk, where + /// placeholder rows are the sole representation of a derived module's inherited + /// members (that walk does not climb the composed template's parent chain). + /// private static void CollectDirectMembers( Template template, string? prefix, string originPrefix, - List members) + List members, + bool skipInherited) { foreach (var attr in template.Attributes) { + if (skipInherited && attr.IsInherited) continue; var canonicalName = prefix == null ? attr.Name : $"{prefix}.{attr.Name}"; members.Add(new ResolvedMember(canonicalName, "Attribute", originPrefix)); } foreach (var alarm in template.Alarms) { + if (skipInherited && alarm.IsInherited) continue; var canonicalName = prefix == null ? alarm.Name : $"{prefix}.{alarm.Name}"; members.Add(new ResolvedMember(canonicalName, "Alarm", originPrefix)); } foreach (var script in template.Scripts) { + if (skipInherited && script.IsInherited) continue; var canonicalName = prefix == null ? script.Name : $"{prefix}.{script.Name}"; members.Add(new ResolvedMember(canonicalName, "Script", originPrefix)); } @@ -110,8 +126,11 @@ public static class CollisionDetector if (!visited.Add(template.Id)) return; - // Add direct members of this composed template with the prefix - CollectDirectMembers(template, prefix, $"module '{prefix}'", members); + // Add direct members of this composed template with the prefix. Inherited + // placeholder rows are KEPT here: the composed-module walk does not climb the + // composed template's parent chain, so the placeholders are the only + // representation of a derived module's inherited members. + CollectDirectMembers(template, prefix, $"module '{prefix}'", members, skipInherited: false); // Recurse into nested compositions foreach (var composition in template.Compositions) @@ -139,8 +158,10 @@ public static class CollisionDetector if (!visited.Add(parent.Id)) return; - // Inherited direct members (no prefix) - CollectDirectMembers(parent, prefix: null, $"parent '{parent.Name}'", members); + // Inherited direct members (no prefix). Skip the parent's OWN inherited + // placeholders too: the next recursion adds the grandparent's real rows, so + // counting the parent's copies would re-introduce the collision one level up. + CollectDirectMembers(parent, prefix: null, $"parent '{parent.Name}'", members, skipInherited: true); // Inherited composed modules foreach (var composition in parent.Compositions) diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs index f8a93e04..041bf6ab 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/ScriptAnalysis/ScriptAnalysisServiceTests.cs @@ -677,4 +677,46 @@ public class ScriptAnalysisServiceTests Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("CS")); Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("SCADA")); } + + // ── Instance-script batch/wait surface (follow-up #7) ───────────────── + + [Fact] + public void InstanceScript_BatchAndWaitHelpers_DiagnoseClean() + { + // Follow-up #7: SandboxAttributeAccessor was missing WriteBatchAndWaitAsync / + // WaitAsync / WaitForAsync, so the editor false-flagged valid instance scripts + // with CS1061 even though `template validate` and the deploy gate accept them. + // The sandbox surface must mirror the runtime AttributeAccessor member-for-member. + var code = + "var data = new System.Collections.Generic.Dictionary { { \"Cmd\", 1 } };\n" + + "var done = await Attributes.WriteBatchAndWaitAsync(data, \"Flag\", true, \"Done\", true, System.TimeSpan.FromSeconds(5));\n" + + "var hit = await Attributes.WaitAsync(\"Done\", true, System.TimeSpan.FromSeconds(5));\n" + + "var pred = await Attributes.WaitAsync(\"Done\", v => v != null, System.TimeSpan.FromSeconds(5));\n" + + "var res = await Attributes.WaitForAsync(\"Done\", true, System.TimeSpan.FromSeconds(5));\n" + + "return done && hit && pred && res.Matched;"; + var resp = _svc.Diagnose(new DiagnoseRequest(code)); + + Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("CS")); + Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("SCADA")); + } + + [Fact] + public void ChildInstanceScript_WriteBatchAndWait_DiagnoseClean() + { + // The same helper on a child composition + // (Children["X"].Attributes.WriteBatchAndWaitAsync) — the exact shape used by + // the base MESReceiver MoveIn/MoveOut scripts — must also resolve cleanly + // through the sandbox composition accessor. + var code = + "var data = new System.Collections.Generic.Dictionary { { \"MoveInType\", \"Run\" } };\n" + + "var ok = await Children[\"RightMESReceiver\"].Attributes.WriteBatchAndWaitAsync(" + + "data, \"MoveInFlag\", true, \"MoveInCompleteFlag\", true, System.TimeSpan.FromSeconds(25));\n" + + "return ok;"; + var resp = _svc.Diagnose(new DiagnoseRequest( + Code: code, + Children: new[] { Comp("RightMESReceiver", attrs: new[] { Attr("MoveInType") }) })); + + Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("CS")); + Assert.DoesNotContain(resp.Markers, m => m.Code.StartsWith("SCADA")); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs index ed851872..ccfc5727 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs @@ -111,4 +111,81 @@ public class CollisionDetectorTests Assert.NotEmpty(collisions); Assert.Contains(collisions, c => c.Contains("modA.Temp")); } + + // ======================================================================== + // Follow-up #3: inherited placeholder rows must not self-collide with the + // parent rows the inheritance walk re-adds. Before the fix, every derived + // template reported a collision for each IsInherited row (child origin vs + // "parent 'X'" origin) — so there was no supported path to add an attribute + // or composition to a derived template. + // ======================================================================== + + [Fact] + public void DetectCollisions_DerivedTemplateWithInheritedPlaceholders_NoFalseCollision() + { + // Base carries the real attributes. + var parent = new Template("ReactorSide") { Id = 7 }; + parent.Attributes.Add(new TemplateAttribute("DeltaVac") { Id = 70, TemplateId = 7, DataType = DataType.Float }); + parent.Attributes.Add(new TemplateAttribute("ResultType") { Id = 71, TemplateId = 7, DataType = DataType.String }); + + // Derived child stores IsInherited placeholder copies of the base attrs + // PLUS a newly-added direct attribute (the member an author is trying to add). + var child = new Template("LeftReactorSide") { Id = 8, ParentTemplateId = 7 }; + child.Attributes.Add(new TemplateAttribute("DeltaVac") { Id = 80, TemplateId = 8, DataType = DataType.Float, IsInherited = true }); + child.Attributes.Add(new TemplateAttribute("ResultType") { Id = 81, TemplateId = 8, DataType = DataType.String, IsInherited = true }); + child.Attributes.Add(new TemplateAttribute("SideName") { Id = 82, TemplateId = 8, DataType = DataType.String }); + + var all = new List