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 | 0 |

## 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:

```csharp
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 | Resolved |
| 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:

```csharp
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**

Resolved 2026-05-17 (commit `pending`): `RenameCompositionAsync` now recurses
into `derived.Compositions` via a new `CollectCascadeRenamesAsync` helper
(mirroring `CascadeDeleteDerivedAsync`), re-deriving each cascaded inner derived
template's name from its renamed parent and slot instance name, and runs the
same-name collision pre-check across every name in the cascade before any row
mutates. Regression tests:
`RenameComposition_CascadesRenameToNestedDerivedTemplates`,
`RenameComposition_NestedCascadeNameCollision_Fails`.

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

| | |
|--|--|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `src/ScadaLink.TemplateEngine/Flattening/FlatteningService.cs:750` |

**Description**

`ResolveComposedScriptsRecursive` assigns each composed script a `ScriptScope`:

```csharp
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**

Resolved 2026-05-17 (commit `pending`): threaded a `parentPath` parameter
through `ResolveComposedScriptsRecursive` — the top-level caller passes `""`
(a depth-1 composition's parent is the root template) and each nested call
passes the enclosing module's `prefix` — and the `ScriptScope` now sets
`ParentPath` to that value instead of a hard-coded `""`, so a depth-2 script's
`SelfPath = "Outer.Inner"` pairs with `ParentPath = "Outer"` and `Parent.X`
resolves against the real parent module. Regression test:
`Flatten_NestedComposedScript_ScopeCarriesCorrectParentPath`.