diff --git a/code-reviews/TemplateEngine/findings.md b/code-reviews/TemplateEngine/findings.md index 212674a..57db1b8 100644 --- a/code-reviews/TemplateEngine/findings.md +++ b/code-reviews/TemplateEngine/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 14 | +| Open findings | 10 | ## Summary @@ -52,7 +52,7 @@ of attributes vs. alarms vs. scripts throughout the resolve/flatten/derive paths |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:211`, `src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:535`, `src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:609` | **Description** @@ -77,7 +77,13 @@ the recursion already in `TemplateResolver.AddComposedMembers` and **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``): replaced the hand-unrolled +one/two-level composition loops in `ResolveComposedAttributes`, +`ResolveComposedAlarms`, and `ResolveComposedScripts` with single recursive +walks (`*Recursive` helpers) carrying the accumulated path prefix and a +`visited` set, so composed members at arbitrary nesting depth are resolved. +Regression tests: `Flatten_ThreeLevelComposition_AttributesAlarmsScriptsAllResolved`, +`Flatten_NestedComposedAlarm_TriggerAttributePrefixed`. ### TemplateEngine-002 — Derived templates omit all base alarms; composed alarms cannot be overridden per slot @@ -110,7 +116,22 @@ already do. **Resolution** -_Unresolved._ +_Unresolved (re-triaged 2026-05-16)._ Partially mis-stated and out of the +current fix scope. Correction to the description: composed/inherited alarms +are **not** dropped from the flattened deployment output — `FlatteningService` +resolves alarms from the entire inheritance chain (`ResolveInheritedAlarms` +walks `templateChain`, which includes the base of a derived template), so an +instance of a derived template still receives the base template's alarms. The +real, valid gap is narrower: there is no per-slot **alarm override** +mechanism. The fix genuinely requires adding `IsInherited` / `LockedInDerived` +fields to the `TemplateAlarm` entity, which lives in `ScadaLink.Commons` +(a different module). Adding an alarm copy loop to `BuildDerivedTemplate` +without those fields would be actively harmful: copied alarm rows on the +derived template would shadow the live base alarm with stale data during +flattening (`ResolveInheritedAlarms` has no `IsInherited` skip for alarms, +unlike attributes/scripts). Resolving this safely is a cross-module change +(`Commons` + `TemplateEngine`) and must be scheduled as a coordinated edit; +left **Open** pending that. ### TemplateEngine-003 — `UpdateAttributeAsync` lets a non-locked attribute change its fixed DataType / DataSourceReference @@ -118,7 +139,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/TemplateService.cs:285` | **Description** @@ -147,7 +168,12 @@ apply block. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``): removed the `&& existing.IsLocked` +guard in `UpdateAttributeAsync` so the fixed-field granularity error is always +honoured, and removed the unconditional `existing.DataType` / +`existing.DataSourceReference` assignments from the apply block. Regression +tests: `UpdateAttribute_UnlockedAttribute_DataTypeChangeRejected`, +`UpdateAttribute_UnlockedAttribute_DataSourceReferenceChangeRejected`. ### TemplateEngine-004 — Alarm on-trigger script references are never resolved (empty placeholder) @@ -155,7 +181,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:695` | **Description** @@ -179,7 +205,15 @@ and implement that consistently. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``): implemented `ResolveAlarmScriptReferences`. +Alarm resolution now records each resolved alarm's `OnTriggerScriptId` keyed by +canonical name, and script resolution records each resolved `TemplateScript.Id` +keyed by its canonical name (both honour composition path prefixes). Step 7 +joins the two maps to set `ResolvedAlarm.OnTriggerScriptCanonicalName`, so the +revision hash, diff, and `SemanticValidator` on-trigger-script-exists check now +all see the reference. Regression tests: +`Flatten_AlarmOnTriggerScript_ResolvedToCanonicalName`, +`Flatten_ComposedAlarmOnTriggerScript_ResolvedWithPrefix`. ### TemplateEngine-005 — Collision validation is skipped when creating a child template @@ -187,7 +221,7 @@ _Unresolved._ |--|--| | Severity | High | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.TemplateEngine/TemplateService.cs:56` | **Description** @@ -210,7 +244,15 @@ that explicitly instead of leaving a no-op. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit ``): deleted the dead `if +(parentTemplateId.HasValue)` block and its unused `GetAllTemplatesAsync` +read in `CreateTemplateAsync`. A create-time collision check on a child is a +guaranteed no-op — a freshly created template has no members of its own, the +parent's members were already collision-validated on every member-mutating +call, and a new child cannot be an ancestor of its parent. Replaced the no-op +with an explanatory comment documenting that collision detection is enforced +on `AddAttribute`/`AddAlarm`/`AddScript`/`AddComposition` and on rename. +Regression test: `CreateTemplate_WithParent_DoesNotRunDeadCollisionQuery`. ### TemplateEngine-006 — Forbidden-API enforcement is a naive substring scan (bypassable and false-positive prone) diff --git a/src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs b/src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs index 3e96ed5..261a627 100644 --- a/src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs +++ b/src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs @@ -78,17 +78,23 @@ public class FlatteningService // Step 4: Apply connection bindings ApplyConnectionBindings(instance.ConnectionBindings, attributes, dataConnections); - // Step 5: Resolve alarms from inheritance chain - var alarms = ResolveInheritedAlarms(templateChain); - ResolveComposedAlarms(templateChain, compositionMap, composedTemplateChains, alarms); + // Step 5: Resolve alarms from inheritance chain. + // alarmScriptIds maps a resolved alarm's canonical name to the + // TemplateScript.Id of its on-trigger script (if any). + var alarmScriptIds = new Dictionary(StringComparer.Ordinal); + var alarms = ResolveInheritedAlarms(templateChain, prefix: null, alarmScriptIds); + ResolveComposedAlarms(templateChain, compositionMap, composedTemplateChains, alarms, alarmScriptIds); ApplyInstanceAlarmOverrides(instance.AlarmOverrides, alarms); - // Step 6: Resolve scripts from inheritance chain - var scripts = ResolveInheritedScripts(templateChain); - ResolveComposedScripts(templateChain, compositionMap, composedTemplateChains, scripts); + // Step 6: Resolve scripts from inheritance chain. + // scriptCanonicalById maps a TemplateScript.Id to its resolved + // canonical name, used to wire up alarm on-trigger script refs. + var scriptCanonicalById = new Dictionary(); + var scripts = ResolveInheritedScripts(templateChain, prefix: null, scriptCanonicalById); + ResolveComposedScripts(templateChain, compositionMap, composedTemplateChains, scripts, scriptCanonicalById); // Step 7: Resolve alarm on-trigger script references to canonical names - ResolveAlarmScriptReferences(alarms, scripts); + ResolveAlarmScriptReferences(alarms, alarmScriptIds, scriptCanonicalById); // Step 8: Collect connection configurations for deployment packaging var connections = new Dictionary(); @@ -221,57 +227,56 @@ public class FlatteningService continue; foreach (var composition in compositions) + ResolveComposedAttributesRecursive( + composition, composition.InstanceName, + compositionMap, composedTemplateChains, attributes, new HashSet()); + } + } + + /// + /// Recursively resolves the attributes of a composed module and every + /// module nested inside it (to arbitrary depth), path-qualifying each + /// canonical name with the accumulated . + /// + private static void ResolveComposedAttributesRecursive( + TemplateComposition composition, + string prefix, + IReadOnlyDictionary> compositionMap, + IReadOnlyDictionary> composedTemplateChains, + Dictionary attributes, + HashSet visited) + { + if (!composedTemplateChains.TryGetValue(composition.ComposedTemplateId, out var composedChain)) + return; + + var composedAttrs = ResolveInheritedAttributes(composedChain); + foreach (var (name, attr) in composedAttrs) + { + var canonicalName = $"{prefix}.{name}"; + // Don't overwrite if already defined (most-derived wins) + if (!attributes.ContainsKey(canonicalName)) { - if (!composedTemplateChains.TryGetValue(composition.ComposedTemplateId, out var composedChain)) - continue; - - var prefix = composition.InstanceName; - var composedAttrs = ResolveInheritedAttributes(composedChain); - - foreach (var (name, attr) in composedAttrs) + attributes[canonicalName] = attr with { - var canonicalName = $"{prefix}.{name}"; - // Don't overwrite if already defined (most-derived wins) - if (!attributes.ContainsKey(canonicalName)) - { - attributes[canonicalName] = attr with - { - CanonicalName = canonicalName, - Source = "Composed" - }; - } - } - - // Recurse into nested compositions - foreach (var composedTemplate in composedChain) - { - if (!compositionMap.TryGetValue(composedTemplate.Id, out var nestedCompositions)) - continue; - - foreach (var nested in nestedCompositions) - { - if (!composedTemplateChains.TryGetValue(nested.ComposedTemplateId, out var nestedChain)) - continue; - - var nestedPrefix = $"{prefix}.{nested.InstanceName}"; - var nestedAttrs = ResolveInheritedAttributes(nestedChain); - - foreach (var (name, attr) in nestedAttrs) - { - var canonicalName = $"{nestedPrefix}.{name}"; - if (!attributes.ContainsKey(canonicalName)) - { - attributes[canonicalName] = attr with - { - CanonicalName = canonicalName, - Source = "Composed" - }; - } - } - } - } + CanonicalName = canonicalName, + Source = "Composed" + }; } } + + // Descend into nested compositions of every template in the chain. + foreach (var composedTemplate in composedChain) + { + if (!visited.Add(composedTemplate.Id)) + continue; + if (!compositionMap.TryGetValue(composedTemplate.Id, out var nestedCompositions)) + continue; + + foreach (var nested in nestedCompositions) + ResolveComposedAttributesRecursive( + nested, $"{prefix}.{nested.InstanceName}", + compositionMap, composedTemplateChains, attributes, visited); + } } private static void ApplyInstanceOverrides( @@ -356,10 +361,22 @@ public class FlatteningService } } + /// + /// Resolves alarms from an inheritance chain. When + /// is non-null the alarm names are returned bare (caller path-qualifies); + /// the keys of the returned dictionary are always bare alarm names. + /// is populated with the on-trigger + /// script id of each resolved alarm keyed by the canonical name the alarm + /// will ultimately carry (bare name when is null, + /// otherwise prefix.name). + /// private static Dictionary ResolveInheritedAlarms( - IReadOnlyList