fix(template-engine): resolve TemplateEngine-006..010 — code-region-aware API/brace scanning, composed-alarm override validation, N+1 fix, doc correction

code-reviews/TemplateEngine/findings.md

@@ -8,7 +8,7 @@
 | Last reviewed | 2026-05-16 |
 | Reviewer | claude-agent |
 | Commit reviewed | `9c60592` |
-| Open findings | 9 |
+| Open findings | 4 |
 
 ## Summary
 
@@ -263,7 +263,7 @@ Regression test: `CreateTemplate_WithParent_DoesNotRunDeadCollisionQuery`.
 |--|--|
 | Severity | Medium |
 | Category | Security |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs:21`, `src/ScadaLink.TemplateEngine/Validation/ValidationService.cs:318` |
 
 **Description**
@@ -289,7 +289,20 @@ limitation prominently and treat the substring scan as advisory, not authoritati
 
 **Resolution**
 
-_Unresolved._
+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
 
@@ -297,7 +310,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Correctness & logic bugs |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.TemplateEngine/Validation/ScriptCompiler.cs:54`, `src/ScadaLink.TemplateEngine/SharedScriptService.cs:124` |
 
 **Description**
@@ -321,7 +334,18 @@ to something that cannot false-positive.
 
 **Resolution**
 
-_Unresolved._
+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
 
@@ -329,7 +353,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Error handling & resilience |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.TemplateEngine/Services/InstanceService.cs:178` |
 
 **Description**
@@ -351,7 +375,16 @@ the lock check to composed alarms too.
 
 **Resolution**
 
-_Unresolved._
+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`
 
@@ -359,7 +392,7 @@ _Unresolved._
 |--|--|
 | Severity | Medium |
 | Category | Performance & resource management |
-| Status | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.TemplateEngine/Services/TemplateDeletionService.cs:75` |
 
 **Description**
@@ -380,15 +413,24 @@ template.
 
 **Resolution**
 
-_Unresolved._
+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 | Medium |
+| 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 | Open |
+| Status | Resolved |
 | Location | `src/ScadaLink.TemplateEngine/Services/InstanceService.cs:9` |
 
 **Description**
@@ -407,9 +449,24 @@ If last-write-wins is acceptable for instance state, correct the XML doc. If opt
 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**
 
-_Unresolved._
+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