scadalink-design/code-reviews/TemplateEngine/findings.md

Code Review — TemplateEngine

Field	Value
Module	src/ScadaLink.TemplateEngine
Design doc	docs/requirements/Component-TemplateEngine.md
Status	Reviewed
Last reviewed	2026-05-17
Reviewer	claude-agent
Commit reviewed	39d737e
Open findings	2

Summary

The Template Engine is a pure central-side modeling library: stateless services over ITemplateEngineRepository plus four static helper classes (collision, cycle, lock, resolver). It has no Akka actors and no direct concurrency, so the Akka and thread-safety categories produce nothing of substance. The code is generally well-structured and the cascade-based composition model (derived templates owned by composition slots) is consistently applied. However the review surfaced several real correctness gaps. The most serious are in flattening: composed alarms and scripts nested below the first level are silently dropped, derived templates omit base alarms entirely (breaking per-slot alarm override), and the alarm-on-trigger-script resolution step is an empty placeholder so that whole validation rule is dead. Validation has two security-relevant weaknesses — the forbidden-API scan is a naive substring match and the brace-balance "compile" check mispredicts on verbatim / interpolated / raw string literals. Several documented behaviours (collision check on create, optimistic concurrency on instance state) are claimed but not implemented. Themes: validation that is weaker than the design promises, and asymmetric handling of attributes vs. alarms vs. scripts throughout the resolve/flatten/derive paths.

Re-review 2026-05-17 (commit 39d737e)

Re-reviewed the whole module against all ten checklist categories at commit 39d737e. All fourteen prior findings remain closed — the batch-4 fixes (bc88a36/804697f and predecessors) hold up: the recursive composition walk, the per-slot alarm override mechanism, the code-region-aware delimiter scanner, and the single-source deletion-constraint logic are all correctly in place. Two new Medium findings surfaced, both in the composition-cascade path and both affecting nested (depth ≥ 2) compositions specifically — the same blind spot that produced TemplateEngine-001. TemplateEngine-015: RenameCompositionAsync renames only the directly slot-owned derived template, leaving cascaded inner derived templates with a stale dotted-path name. TemplateEngine-016: FlatteningService hard-codes ScriptScope.ParentPath to the empty string for every composed script regardless of nesting depth, so a script two or more levels deep cannot resolve Parent.X references to its real parent module. Both are limited-impact (nested compositions are the less common case and there is design-time visibility) but represent genuine drift from the recursive-nesting design promise.

Checklist coverage

#	Category	Examined	Notes
1	Correctness & logic bugs	✓	Prior bugs (001–005, 013) all resolved and verified. Re-review 2026-05-17 found two new nested-composition defects: rename does not cascade (TemplateEngine-015), composed-script ParentPath always empty (TemplateEngine-016).
2	Akka.NET conventions	✓	No actors in this module (AddTemplateEngineActors is an empty placeholder). Nothing to assess.
3	Concurrency & thread safety	✓	Services are stateless, scoped per request; static helpers hold no mutable state. Design says template editing is last-write-wins; that is honoured. See TemplateEngine-010 re: a doc claim of optimistic concurrency that is not implemented.
4	Error handling & resilience	✓	Result<T> used consistently; repository nulls guarded. FlatteningService wraps in try/catch. No store-and-forward or failover surface in this module.
5	Security	✓	No auth checks in-module (delegated to callers per design). Script trust-model enforcement is weak — see TemplateEngine-006 and TemplateEngine-007.
6	Performance & resource management	✓	GetAllTemplatesAsync reloaded on most member edits; one genuine N+1 in TemplateDeletionService (TemplateEngine-009). No IDisposable leaks (JsonDocument/streams disposed).
7	Design-document adherence	✓	Drift found: recursive composition not fully implemented in flattening; DataType enum naming differs from doc; optimistic-concurrency claim.
8	Code organization & conventions	✓	POCO entities in Commons, repo interfaces in Commons, Options pattern N/A (no options here). Duplicate deletion logic (TemplateEngine-014).
9	Testing coverage	✓	Tests exist for every file, but the dead/placeholder paths (TemplateEngine-004, 005) and deep nesting (TemplateEngine-001) are not exercised.
10	Documentation & comments	✓	Mostly accurate; a misleading converter comment (TemplateEngine-011) and a stale enum/doc mismatch (TemplateEngine-012).

Findings

TemplateEngine-001 — Deeply nested composed members are dropped during flattening

Severity	High
Category	Correctness & logic bugs
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

The design doc states composition supports "recursive nesting of feature modules" and that nested paths extend as [Outer].[Inner].[Member]. ResolveComposedAttributes only descends one level of nesting: it resolves the directly-composed module, then its immediate child compositions, and stops. A module composed three or more levels deep contributes no attributes to the flattened configuration. ResolveComposedAlarms and ResolveComposedScripts are worse — they handle only the first (direct) level and do not descend at all, so any alarm or script in a nested composed module is dropped entirely. CollisionDetector and TemplateResolver recurse fully, so collision detection and the authoring UI will show members that the deployed configuration silently lacks.

Recommendation

Replace the hand-unrolled one/two-level loops with a single recursive walk (carrying the accumulated path prefix) for attributes, alarms, and scripts, matching the recursion already in TemplateResolver.AddComposedMembers and CollisionDetector.CollectComposedMembers.

Resolution

Resolved 2026-05-16 (commit <pending>): 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

Severity	High
Category	Correctness & logic bugs
Status	Resolved
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:799

Description

BuildDerivedTemplate copies the base template's Attributes and Scripts into the new derived template as IsInherited = true placeholder rows so they can be overridden per composition slot, but there is no loop for Alarms. The derived template therefore has zero alarm rows. The TemplateAlarm entity also has no IsInherited or LockedInDerived fields (unlike TemplateAttribute / TemplateScript), so even if a copy loop were added there is no mechanism to mark a copied alarm as inherited or to override one. The design's Override Granularity section explicitly requires composed alarm fields (Priority, Trigger thresholds, Description, On-Trigger Script) to be overridable. As written, a composed module's alarms cannot be tuned for the slot they are used in.

Recommendation

Add an alarm copy loop to BuildDerivedTemplate and add IsInherited / LockedInDerived fields to TemplateAlarm, mirroring TemplateAttribute. Update UpdateAlarmAsync to honour them as UpdateAttributeAsync / UpdateScriptAsync already do.

Resolution

Resolved 2026-05-16 (commit <pending>): implemented the per-slot alarm override mechanism as a coordinated Commons + ConfigurationDatabase + TemplateEngine change, mirroring the existing attribute/script override design. Added IsInherited / LockedInDerived to the TemplateAlarm POCO (ScadaLink.Commons) and an EF migration AddDerivedAlarmFields adding two bit NOT NULL DEFAULT 0 columns to TemplateAlarms. BuildDerivedTemplate now copies base alarms as IsInherited = true placeholder rows. FlatteningService.ResolveInheritedAlarms skips IsInherited placeholder rows so they no longer shadow the live base alarm, and ValidateLockedInDerived now rejects a derived override of a LockedInDerived base alarm. UpdateAlarmAsync honours the base LockedInDerived lock and persists IsInherited / LockedInDerived, exactly as UpdateAttributeAsync / UpdateScriptAsync do. Regression tests: Flatten_InheritedAlarmOnDerived_BaseValueWins, Flatten_OverriddenAlarmOnDerived_DerivedValueWins, Flatten_LockedInDerivedAlarmOverride_Fails, AddComposition_CopiesAlarmsAsInherited, UpdateAlarm_LockedInDerivedBase_RejectsOnDerived, UpdateAlarm_DerivedOverride_PersistsIsInheritedFalse.

TemplateEngine-003 — UpdateAttributeAsync lets a non-locked attribute change its fixed DataType / DataSourceReference

Severity	High
Category	Correctness & logic bugs
Status	Resolved
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:285

Description

LockEnforcer.ValidateAttributeOverride correctly rejects a change to DataType or DataSourceReference (both "fixed by the defining level" per the design). But the caller only honours that error when the attribute is already locked:

var granularityError = LockEnforcer.ValidateAttributeOverride(existing, proposed);
if (granularityError != null && existing.IsLocked)
    return Result<TemplateAttribute>.Failure(granularityError);

Lines 293-294 then unconditionally apply existing.DataType = proposed.DataType and existing.DataSourceReference = proposed.DataSourceReference. For the common case of an unlocked attribute, the fixed-field guard is dead and both fields are silently mutable, violating the override-granularity rule. (The lock-error branch of the same helper is also redundant — a locked attribute already returns earlier inside the helper.)

Recommendation

Remove the && existing.IsLocked condition so the granularity error is always returned, and stop assigning DataType / DataSourceReference from proposed in the apply block.

Resolution

Resolved 2026-05-16 (commit <pending>): 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)

Severity	High
Category	Correctness & logic bugs
Status	Resolved
Location	src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:695

Description

ResolveAlarmScriptReferences is invoked as Step 7 of Flatten but its body is empty — only a comment describing what it should do. Consequently every ResolvedAlarm.OnTriggerScriptCanonicalName stays null. This has two downstream effects: (1) SemanticValidator's "on-trigger script must exist" check (SemanticValidator.cs:209) can never fire, so the design-mandated validation of alarm on-trigger script references is silently absent; (2) RevisionHashService and DiffService both hash/compare OnTriggerScriptCanonicalName, so a change to which script an alarm triggers never affects the revision hash and is invisible to the diff — a real staleness-detection gap.

Recommendation

Implement the resolution: map each alarm's OnTriggerScriptId (set on TemplateAlarm) to the canonical name of the corresponding resolved script, accounting for composition prefixes. If the design intends scripts to be referenced by name within scope, document and implement that consistently.

Resolution

Resolved 2026-05-16 (commit <pending>): 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

Severity	High
Category	Correctness & logic bugs
Status	Resolved
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:56

Description

CreateTemplateAsync contains a block guarded by if (parentTemplateId.HasValue) that loads GetAllTemplatesAsync and then does nothing but hold a comment — it never runs a collision check. A child template created with a parent inherits the parent's members; if the child is later given members (via AddAttributeAsync etc.) those calls do run CollisionDetector, but the create path itself performs no naming-collision validation and UpdateTemplateAsync only validates collisions on a name change. The design states naming collisions are design-time errors that must block a save. The dead block is also confusing and allocates an unused full-table read.

Recommendation

Either run a real collision check on the to-be-created template (including its inherited members) or delete the dead block and its unused query. If create-time collisions are genuinely impossible because a fresh template has no members, document that explicitly instead of leaving a no-op.

Resolution

Resolved 2026-05-16 (commit <pending>): 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)

Severity	Medium
Category	Security
Status	Resolved
Location	src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs:21, src/ScadaLink.TemplateEngine/Validation/ValidationService.cs:318

Description

ScriptCompiler.ForbiddenPatterns is checked with code.Contains(pattern). This is both under- and over-inclusive against the script trust model:

• Bypass: using System.IO; followed by File.ReadAllText(...) contains no System.IO. token; using static System.IO.File;, namespace aliases, and global::System.IO.File all evade the literal patterns.
• False positive: a string literal, comment, or attribute name containing the text System.IO. is flagged as a forbidden API even though it is inert.

The same patterns are reused for trigger-expression validation (CheckExpressionSyntax), inheriting the same weakness. The file comment acknowledges this is interim until Roslyn is wired in, but the trust model is security-relevant and the gap should be tracked.

Recommendation

Defer real enforcement to the Roslyn-based compiler (semantic symbol analysis of referenced types/namespaces) rather than text matching. Until then, document the limitation prominently and treat the substring scan as advisory, not authoritative.

Resolution

Resolved 2026-05-16 (commit pending): fixed the false-positive half and documented the (deferred) bypass half. Added CSharpDelimiterScanner.ContainsInCode, a code-region-aware substring search that blanks out string/char-literal/comment spans before matching, so the inert text System.IO. inside a string or comment is no longer flagged. ScriptCompiler.TryCompile and ValidationService.CheckExpressionSyntax now use it. The bypass half (namespace aliases, using static, global::) genuinely requires Roslyn semantic symbol analysis, which the TemplateEngine project deliberately does not reference — that authoritative check is deferred to the real script compiler / Site Runtime sandbox. The limitation is now documented prominently as a SECURITY LIMITATION note in the ScriptCompiler class summary and the ForbiddenPatterns doc, and the scan is explicitly labelled advisory. Regression tests: TryCompile_ForbiddenApiTextInsideStringLiteral_NotFlagged, TryCompile_ForbiddenApiTextInsideComment_NotFlagged, TryCompile_ForbiddenApiInRealCode_StillFlagged.

TemplateEngine-007 — Brace-balance "compilation" misjudges verbatim / interpolated / raw strings

Severity	Medium
Category	Correctness & logic bugs
Status	Resolved
Location	src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs:54, src/ScadaLink.TemplateEngine/SharedScriptService.cs:124

Description

ScriptCompiler.TryCompile tracks string state with a single inString flag toggled on " and an escaped-quote check of code[i-1] != '\\'. It does not understand verbatim strings (@"..." where "" is the escape and \ is literal), interpolated strings ($"{...}" whose braces are code, not text), raw string literals ("""..."""), or char literals. A script with a verbatim string containing a brace, an interpolated string, or a '}' char literal will be wrongly rejected as having mismatched braces — blocking a valid script from deployment. SharedScriptService.ValidateSyntax is even cruder: it counts braces/brackets/parens with no string or comment awareness at all, so any string literal containing one of those characters produces a false syntax error.

Recommendation

Once the Roslyn compiler is available, parse with CSharpSyntaxTree.ParseText and inspect diagnostics instead of hand-rolling a tokenizer. If an interim check must remain, at minimum handle verbatim/interpolated/char literals or scope the check down to something that cannot false-positive.

Resolution

Resolved 2026-05-16 (commit pending): replaced both hand-rolled string trackers with CSharpDelimiterScanner, a single string-/comment-aware scanner that correctly skips regular strings (with \ escapes), verbatim strings (@"...", "" escape), interpolated strings ($"..." / $@"...", interpolation holes {...} treated as code, {{/}} as escaped braces), C# 11 raw string literals ("""..."""), char literals, and line/block comments while tracking {}/[]/() depth. ScriptCompiler .TryCompile and SharedScriptService.ValidateSyntax now delegate to it, so a valid script containing a delimiter inside a literal/comment is no longer falsely rejected; genuine mismatches are still caught. Regression tests in ScriptCompilerTests (TryCompile_VerbatimStringWithBrace_*, _VerbatimStringWithEscapedQuote_*, _InterpolatedStringWithBraces_*, _RawStringLiteralWithBraces_*, _CharLiteralWithBrace_*, _GenuineMismatchedBraces_StillDetected) and SharedScriptServiceTests.ValidateSyntax_DelimiterInsideStringOrComment_ReturnsNull.

TemplateEngine-008 — SetAlarmOverrideAsync accepts overrides for unknown / composed alarms with no validation

Severity	Medium
Category	Error handling & resilience
Status	Resolved
Location	src/ScadaLink.TemplateEngine/Services/InstanceService.cs:178

Description

SetAlarmOverrideAsync looks up the alarm by name among the template's direct alarms only. When the lookup returns null — which is the case for every composed (path-qualified) alarm as well as for a genuinely non-existent name — the method skips the lock check and proceeds to persist the override. This means: (1) an override can be created for an alarm that does not exist (a silent dead record), and (2) a composed alarm that is IsLocked at the template level can be overridden, bypassing the lock rule. SetAttributeOverrideAsync by contrast rejects unknown attribute names. The inline comment acknowledges the gap but the behaviour is inconsistent and risky.

Recommendation

Resolve the full effective alarm set (via the resolver / flattening) so composed alarms are found, reject overrides whose canonical name is not in that set, and apply the lock check to composed alarms too.

Resolution

Resolved 2026-05-16 (commit pending): SetAlarmOverrideAsync now resolves the instance template's full effective alarm set via TemplateResolver.ResolveAllMembers (loaded from GetAllTemplatesAsync) instead of looking up only the template's direct alarms. An override whose canonical name is absent from that set is rejected with a "does not exist" failure (mirroring SetAttributeOverrideAsync); the IsLocked check now also applies to composed (path-qualified) and inherited alarms, closing the lock-bypass. Regression tests: SetAlarmOverride_NonExistentAlarm_ReturnsFailure, SetAlarmOverride_ComposedLockedAlarm_ReturnsFailure, SetAlarmOverride_ComposedUnlockedAlarm_ReturnsSuccess, SetAlarmOverride_DirectLockedAlarm_ReturnsFailure.

TemplateEngine-009 — N+1 query in TemplateDeletionService.CanDeleteTemplateAsync

Severity	Medium
Category	Performance & resource management
Status	Resolved
Location	src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs:75

Description

Check 3 ("other templates compose it directly") loads all templates and then issues a separate GetCompositionsByTemplateIdAsync call inside a loop over every template — one round-trip per template in the database. The composition information needed is already reachable via t.Compositions on the templates returned by GetAllTemplatesAsync (which TemplateService.DeleteTemplateAsync uses for the equivalent check at line 162). The loop scales linearly with the template count on every delete-precheck and every actual delete.

Recommendation

Use the Compositions navigation already loaded by GetAllTemplatesAsync, or add a single repository call that returns all compositions, rather than querying per template.

Resolution

Resolved 2026-05-16 (commit pending): CanDeleteTemplateAsync Check 3 now reads the Compositions navigation already loaded by GetAllTemplatesAsync (a single SelectMany) instead of issuing one GetCompositionsByTemplateIdAsync round-trip per template — the same source TemplateService.DeleteTemplateAsync uses for the equivalent check. The per-delete cost no longer scales with template count. Regression test: CanDeleteTemplate_DoesNotIssuePerTemplateCompositionQuery (verifies GetCompositionsByTemplateIdAsync is never called); the existing CanDeleteTemplate_ComposedByOthers_ReturnsFailure and CanDeleteTemplate_MultipleConstraints_AllErrorsReported tests were updated to seed the Compositions navigation, matching how EF's GetAllTemplatesAsync loads it.

TemplateEngine-010 — InstanceService documents optimistic concurrency that is not implemented

Severity	Low — re-triaged from Medium: this is a stale XML comment, not a behavioural defect. The code matches the design (last-write-wins); only the doc string was wrong.
Category	Documentation & comments
Status	Resolved
Location	src/ScadaLink.TemplateEngine/Services/InstanceService.cs:9

Description

The class summary states instances support "Enabled/disabled state with optimistic concurrency". EnableAsync, DisableAsync, AssignToAreaAsync and the override/binding mutators all perform a plain read-modify-write with no version token, RowVersion, or concurrency check. Two concurrent enable/disable requests last-writer-wins with no detection. Either the doc is stale (the design's optimistic-concurrency decision applies to deployment status records, not instance state) or a concurrency token was intended and is missing.

Recommendation

If last-write-wins is acceptable for instance state, correct the XML doc. If optimistic concurrency is required, add a concurrency token to Instance and surface a conflict result.

Re-triage

Verified against the design: docs/requirements/Component-TemplateEngine.md states "Concurrent editing uses last-write-wins — no pessimistic locking or conflict detection." The system's optimistic-concurrency decision (per CLAUDE.md) applies to deployment status records, not instance state. The code is therefore correct — a plain read-modify-write is the intended behaviour — and the only defect is the stale "with optimistic concurrency" phrase in the class XML summary. Re-triaged from Medium (Error handling) to Low (Documentation): doc-only fix, no behaviour change.

Resolution

Resolved 2026-05-16 (commit pending): corrected the InstanceService class XML summary — replaced "Enabled/disabled state with optimistic concurrency" with an explicit statement that instance-state edits are last-write-wins (no version token / conflict detection), citing the design decision and noting that optimistic concurrency in the system applies to deployment status records, not instance state. No code or behaviour change; no regression test (documentation-only).

TemplateEngine-011 — SortedPropertiesConverterFactory is dead code with a misleading comment

Severity	Low
Category	Documentation & comments
Status	Resolved
Location	src/ScadaLink.TemplateEngine/Flattening/RevisionHashService.cs:136

Description

SortedPropertiesConverterFactory.CanConvert always returns false and CreateConverter always returns null, so the factory registered in CanonicalJsonOptions does nothing. The class comment claims it "ensures properties are serialized in alphabetical order for deterministic output", and the options comment says "Ensure consistent ordering" — both are false. Determinism actually relies entirely on the Hashable* records being hand-declared with alphabetically-ordered properties (plus camelCase). That works today but is fragile: a future contributor adding a property out of alphabetical order silently changes every revision hash, and the dead converter gives false confidence that ordering is enforced programmatically.

Recommendation

Either implement the converter to genuinely sort properties, or delete it and replace the comments with an explicit note that determinism depends on the manual property ordering of the Hashable* records (ideally enforced by a test).

Resolution

Resolved 2026-05-16 (commit pending commit): deleted the dead SortedPropertiesConverterFactory (and removed it from CanonicalJsonOptions), and replaced the misleading "sorted keys / consistent ordering" comments with an explicit DETERMINISM CONTRACT note on the RevisionHashService class summary — System.Text.Json serializes properties in CLR declaration order and does not sort, so stable hashes rely on the private Hashable* records declaring their properties alphabetically (collections are explicitly sorted by CanonicalName). That manual ordering is now guarded by a regression test: RevisionHashServiceTests.HashableRecords_PropertiesDeclaredAlphabetically (reflects over the nested Hashable* types and asserts ordinal-alphabetical property declaration order), so adding a property out of order now fails the build's test gate instead of silently changing every revision hash.

TemplateEngine-012 — DataType enum naming diverges from the design doc

Severity	Low
Category	Design-document adherence
Status	Deferred
Location	src/ScadaLink.TemplateEngine/Validation/SemanticValidator.cs:18

Description

The design doc (Attribute section) lists data types as "Boolean, Integer, Float, String". The actual DataType enum is Boolean, Int32, Float, Double, DateTime, Binary. SemanticValidator.NumericDataTypes correctly hard-codes the real names (Int32, Float, Double), so the code is internally consistent, but the design doc is stale — it omits Double, DateTime, Binary and calls the integer type "Integer". This makes the doc an unreliable reference for which trigger-operand types are numeric.

Recommendation

Update docs/requirements/Component-TemplateEngine.md to list the actual enum members, or rename the enum to match the doc if "Integer" is the intended canonical name.

Re-triage

Verified against the source: the DataType enum is declared in src/ScadaLink.Commons/Types/Enums/DataType.cs (Boolean, Int32, Float, Double, String, DateTime, Binary) — not in the TemplateEngine module — and is consumed across modules (TemplateAttribute entity, management command contracts). The only in-module file the finding cites, SemanticValidator.cs:18, is confirmed correct: NumericDataTypes already hard-codes the real enum names. Both remediation options in the recommendation therefore land outside this module's resolution boundary (src/ScadaLink.TemplateEngine/**): renaming the enum touches ScadaLink.Commons (and every consumer of DataType), and the alternative — updating the design doc — touches docs/requirements/Component-TemplateEngine.md. There is no in-module code defect to fix. Re-triaged from Open to Deferred: the fix is a one-line design-doc correction (list the actual seven enum members instead of "Boolean, Integer, Float, String") that must be made by an agent owning the docs / Commons scope.

Resolution

Deferred 2026-05-16 (no commit): no in-module fix possible — see Re-triage. The TemplateEngine code is correct as-is. FLAGGED for the docs owner: correct the Attribute data-type list in docs/requirements/Component-TemplateEngine.md to match ScadaLink.Commons DataType (Boolean, Int32, Float, Double, String, DateTime, Binary). Renaming the enum is not recommended (cross-module churn for no behavioural gain); the doc is the authoritative thing to fix.

TemplateEngine-013 — ToDictionary(t => t.Id) throws on duplicate IDs; cycle detectors overload Id 0 as a sentinel

Severity	Low
Category	Correctness & logic bugs
Status	Resolved
Location	src/ScadaLink.TemplateEngine/CycleDetector.cs:30, src/ScadaLink.TemplateEngine/CycleDetector.cs:38

Description

Across the static helpers, allTemplates.ToDictionary(t => t.Id) is used freely; if the caller ever passes a list containing two templates with the same Id (e.g. a not-yet-saved template assigned Id == 0, or duplicated input) the call throws an unhandled ArgumentException rather than returning a Result failure. Separately, CycleDetector uses 0 as the "no parent" sentinel (currentId != 0, ParentTemplateId ?? 0) and DetectInheritanceCycle / DetectCrossGraphCycle ignore a proposed parent/composed id of 0. EF identity keys start at 1 so this is currently benign, but the overload is fragile — an in-memory or test template with Id == 0 would be treated as "no template" and cycle checks would be silently skipped.

Recommendation

Guard the dictionary builds (or use a grouping/ToLookup) and validate input, and use int?/-1 rather than 0 as the no-parent sentinel so a real id of 0 is never special.

Resolution

Resolved 2026-05-16 (commit pending commit): added CycleDetector.BuildLookup, a duplicate-tolerant Id-keyed lookup (Dictionary + TryAdd, first occurrence wins) that replaces every allTemplates.ToDictionary(t => t.Id) in the static helpers — CycleDetector (all three methods), TemplateResolver.ResolveAllMembers, and CollisionDetector.DetectCollisions — so a list containing two templates with the same Id (e.g. not-yet-saved templates carrying Id 0) no longer throws an unhandled ArgumentException. Separately, the 0-as-"no-parent" sentinel was removed: DetectInheritanceCycle now walks the parent chain via the int? ParentTemplateId (HasValue gates continuation), and DetectCrossGraphCycle gates the proposed parent/composed edges on HasValue rather than != 0, so a template with a real Id of 0 is treated like any other node and cycles through it are detected. Regression tests: CycleDetectorTests.DetectInheritanceCycle_DuplicateIdsInList_DoesNotThrow, DetectCompositionCycle_DuplicateIdsInList_DoesNotThrow, DetectCrossGraphCycle_DuplicateIdsInList_DoesNotThrow, DetectInheritanceCycle_RealIdZero_StillDetectsCycle, DetectInheritanceCycle_ParentChainThroughIdZero_DetectsCycle.

TemplateEngine-014 — Template-deletion constraint logic is duplicated and divergent

Severity	Low
Category	Code organization & conventions
Status	Resolved
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:109, src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs:27

Description

TemplateService.DeleteTemplateAsync and TemplateDeletionService.CanDeleteTemplateAsync both implement the "can this template be deleted" rules (instances, child templates, derived templates, composing templates). The two implementations have already drifted: TemplateService reads composing templates from the in-memory t.Compositions navigation while TemplateDeletionService issues per-template GetCompositionsByTemplateIdAsync calls (see TemplateEngine-009), they format error messages differently, and TemplateService returns on the first failing category while TemplateDeletionService accumulates all of them. A future rule change must be made in two places or behaviour will diverge further.

Recommendation

Make TemplateService.DeleteTemplateAsync delegate to TemplateDeletionService (or vice versa) so the constraint logic lives in exactly one place.

Resolution

Resolved 2026-05-16 (commit pending commit): TemplateService.DeleteTemplateAsync no longer re-implements the deletion-constraint rules — it now delegates the constraint check and the delete to TemplateDeletionService.DeleteTemplateAsync (the surviving single implementation, which already accumulates every blocking reason rather than returning on the first failing category). TemplateService retains only its audit-logging side effect: after a successful delete it writes the Delete audit entry and calls SaveChangesAsync (the deletion service is unaware of auditing and persists the delete itself, so the audit entry needs its own save). The constraint logic now lives in exactly one place, so a future rule change cannot drift between two implementations. Behavioural change: DeleteTemplateAsync now reports all blocking reasons and uses TemplateDeletionService's phrasing — the affected TemplateServiceTests delete tests were updated to the unified messages, and a regression test DeleteTemplate_MultipleConstraints_ReportsAllNotJustFirst verifies all three constraint categories are surfaced together.

TemplateEngine-015 — RenameCompositionAsync does not cascade-rename nested derived templates

Severity	Medium
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/TemplateService.cs:680

Description

AddCompositionAsync builds a cascade of derived templates whose names follow a dotted path: composing $Sensor (which itself composes $Probe as Probe1) into $Pump as TempSensor produces $Pump.TempSensor and the nested $Pump.TempSensor.Probe1 (see CreateCascadedCompositionAsync and the AddComposition_CascadesChildCompositions test). RenameCompositionAsync, however, renames only the directly slot-owned derived template:

var derived = await _repository.GetTemplateByIdAsync(composition.ComposedTemplateId, ...);
if (derived != null && derived.IsDerived && derived.OwnerCompositionId == compositionId)
{
    var newDerivedName = $"{owner.Name}.{newInstanceName}";
    ...
    derived.Name = newDerivedName;
    await _repository.UpdateTemplateAsync(derived, ...);
}

There is no recursion into derived.Compositions. After renaming the TempSensor slot to MainSensor, the parent derived becomes $Pump.MainSensor but the cascaded child stays $Pump.TempSensor.Probe1 — its name no longer reflects the slot path it lives under, breaking the dotted-path naming invariant the cascade otherwise maintains. DeleteCompositionAsync correctly recurses (CascadeDeleteDerivedAsync), so rename is the asymmetric outlier. The RenameComposition_RenamesSlotAndDerivedTemplate test only exercises a single-level derived, so the gap is untested. The stale name also breaks the AddComposition_DerivedNameCollision_Fails / cascade-name pre-check on any subsequent compose that walks the now-inconsistent name tree.

Recommendation

Recurse over derived.Compositions (mirroring CascadeDeleteDerivedAsync), re-deriving each cascaded child's name from the renamed parent ($"{parentDerivedName}.{childComposition.InstanceName}"), and run the existing same-name collision pre-check across every name the cascade will produce — not just the top-level one. Add a regression test covering a two-level cascade rename.

Resolution

Unresolved.

TemplateEngine-016 — Composed-script ScriptScope.ParentPath is always empty, breaking Parent.X resolution for nested modules

Severity	Medium
Category	Correctness & logic bugs
Status	Open
Location	src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:750

Description

ResolveComposedScriptsRecursive assigns each composed script a ScriptScope:

Scope = new Commons.Types.Scripts.ScriptScope(SelfPath: prefix, ParentPath: "")

prefix is the accumulated path-qualified module path (Outer at depth 1, Outer.Inner at depth 2, etc.), so SelfPath is correct. ParentPath, however, is hard-coded to the empty string at every depth. Per ScriptScope's own XML doc, ParentPath is "computed at flattening time and seeded into the script's globals … so Attributes["X"] / Parent.X can prepend the right path-prefix." For a script directly composed at depth 1 the parent is the root and "" is correct, but for a script in a nested module (Outer.Inner.Foo) the parent module is Outer — yet ParentPath is still "". A nested composed script that references Parent.X will therefore resolve the reference against the root flat namespace instead of its actual parent module, reading the wrong attribute (or failing to find one). This is the same depth-≥2 nesting blind spot as TemplateEngine-001; the recursive walk was added there but the Scope construction was not updated to carry the parent path. ResolveComposedScripts for direct (root-template) scripts leaves Scope at the default ScriptScope.Root, which is correct.

Recommendation

Thread the parent module path through ResolveComposedScriptsRecursive (the caller already knows it — it is the prefix of the enclosing recursion frame, or "" for a depth-1 composition) and set ParentPath to that value, so SelfPath = "Outer.Inner" pairs with ParentPath = "Outer". Add a flattening test asserting the Scope of a two-level composed script.

Resolution

Unresolved.