Joseph Doherty
32 KiB
Architecture Review 05 — Design-Time Pipeline: Templates, Deployment, Transport, Script Analysis
Reviewer domain: src/ZB.MOM.WW.ScadaBridge.TemplateEngine, src/ZB.MOM.WW.ScadaBridge.DeploymentManager, src/ZB.MOM.WW.ScadaBridge.Transport, src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis, their design docs (docs/requirements/Component-TemplateEngine.md, Component-Transport.md, Component-ScriptAnalysis.md, Component-DeploymentManager.md) and test projects. All findings cite code actually read.
Scope
- Template Engine: flattening (
Flattening/FlatteningService.cs), collision detection (CollisionDetector.cs), acyclicity (CycleDetector.cs), revision hashing (Flattening/RevisionHashService.cs), lock rules (LockEnforcer.cs), diffs (Flattening/DiffService.cs), validation (Validation/ValidationService.cs,Validation/SemanticValidator.cs,Validation/ScriptCompiler.cs), authoring services (TemplateService.cs,TemplateInheritanceResolver.cs). - Deployment Manager:
DeploymentService.cs(deploy/lifecycle pipeline, idempotency reconciliation),OperationLockManager.cs,FlatteningPipeline.cs,StateTransitionValidator.cs,StaleInstanceProbe.cs,ArtifactDeploymentService.cs. - Transport:
Import/BundleImporter.cs(3,832 lines — load/preview/apply),Export/BundleExporter.cs,Export/DependencyResolver.cs(skimmed),Serialization/EntitySerializer.cs/EntityDtos.cs,Encryption/BundleSecretEncryptor.cs+BundleManifestAad.cs,Import/LineDiffer.cs,Import/ArtifactDiff.cs,Import/BundleSessionStore.cs,TransportOptions.cs. - Script Analysis:
ScriptTrustPolicy.cs,ScriptTrustValidator.cs,RoslynScriptCompiler.cs,ScriptCompileSurface.cs. - Cross-component seam checked: Inbound API compiled-handler cache (
src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs) as a consumer of Transport-importedApiMethodrows;ManagementActornative-alarm-source override handlers.
Maturity Verdict
This slice is unusually well-engineered in its failure-path plumbing — the deploy pipeline's query-before-redeploy reconciliation, cancellation-token-safe failure writes, ref-counted operation locks, AES-GCM+AAD bundle crypto with zip-bomb caps and passphrase lockout, and the fused two-pass script trust validator are all production-grade and well beyond typical internal-tool quality. However, the Transport import apply path has serious correctness holes that undercut the whole promotion story: imported templates silently lose their inheritance edge, their per-script cadence/timeout fields, and their native alarm sources — three distinct silent-data-loss paths, none covered by the otherwise substantial integration test suite. The flattener has one real algorithmic bug (repeated composed templates lose grandchild members), the revision hash omits native alarm sources (breaking staleness detection for that member class), and the central node's validation path leaks Roslyn script assemblies on every Deployments-page comparison. The core design is sound; the defects are concentrated in the newest surface area (M8 Transport, native alarms) where spec, DTOs, and apply code drifted apart.
Findings — Stability & Correctness
[Critical] Bundle import drops template inheritance edges — derived templates land as root templates
TemplateDto.BaseTemplateName (Serialization/EntityDtos.cs:115) is populated at export (Serialization/EntitySerializer.cs:55) and even diffed in preview (Import/ArtifactDiff.cs:65), but the apply path never consumes it. BundleImporter.BuildTemplate (Import/BundleImporter.cs:1524-1562) never sets ParentTemplateId; the Overwrite branch (BundleImporter.cs:1466-1484) updates only Description/FolderId and the three child collections; the two second-pass rewires cover alarm→script FKs (ResolveAlarmScriptLinksAsync, line 1907) and compositions (ResolveCompositionEdgesAsync, line 1993) — there is no inheritance-edge pass (grep -n 'Parent' BundleImporter.cs returns nothing). The only code that wires ParentTemplateId from BaseTemplateName, EntitySerializer.FromBundleContent (EntitySerializer.cs:357-360), is invoked only from tests (tests/.../EntitySerializerTests.cs).
Failure scenario: export Pump + derived Pump.WaterPump from dev (the manifest explicitly records dependsOn: ["Template:Pump"] — Component-Transport.md:79-80); import into prod. WaterPump is created with ParentTemplateId = null. FlatteningPipeline.BuildTemplateChainAsync (DeploymentManager/FlatteningPipeline.cs:176-192) produces a one-element chain; every inherited attribute/alarm/script vanishes from the flattened config. Because validation validates what's there, an instance with no bindings on the missing members can deploy "successfully" with a silently amputated configuration. Neither tests/ZB.MOM.WW.ScadaBridge.Transport.Tests nor Transport.IntegrationTests contains any inheritance round-trip test (grep for BaseTemplateName|ParentTemplateId hits only serializer unit tests).
[High] Imported template scripts lose MinTimeBetweenRuns and ExecutionTimeoutSeconds
TemplateScriptDto carries both fields (EntityDtos.cs:148-151), but the importer drops them on both paths: BuildTemplate (Add/Rename — BundleImporter.cs:1550-1560) and SyncTemplateScriptsAsync (Overwrite — the changed comparison and field copy at BundleImporter.cs:~1836-1851 cover Code/TriggerType/TriggerConfiguration/Params/Return/IsLocked only). Both fields participate in the revision hash (RevisionHashService.cs:87-90), so the drift is invisible until redeploy, at which point a WhileTrue script's re-fire cadence or a per-script timeout silently reverts to default on the target environment. Same-class omissions: LockedInDerived is not carried in the DTO at all (no hits in EntityDtos.cs/EntitySerializer.cs), so a base template's derived-lock policy does not survive promotion.
[High] Template NativeAlarmSources are not transported, but instance-level overrides of them are
There is no TemplateNativeAlarmSourceDto; BuildTemplate copies only attributes/alarms/scripts, and none of the SyncTemplate* overwrite helpers touch native alarm sources — yet InstanceNativeAlarmSourceOverrideDto does travel and is applied (EntityDtos.cs:292, BundleImporter.cs:3325-3338). Consequences: (a) a template imported as Add has no native alarm mirrors at all; (b) an Overwrite leaves the target's existing sources untouched and the divergence is invisible because ArtifactDiff.CompareTemplate (Import/ArtifactDiff.cs:54-105) doesn't diff them either; (c) the carried instance overrides dangle and are silently ignored at flatten (FlatteningService.ApplyInstanceNativeAlarmSourceOverrides, FlatteningService.cs:781 — missing canonical name → continue). This contradicts Component-TemplateEngine.md:59-64 which makes native alarm sources first-class template members.
[High] Flattening silently drops grandchild members when the same template is composed twice in one subtree
The recursive composed-member resolvers share one visited set down the recursion (FlatteningService.cs:285-297 for attributes; identical pattern for alarms :649-660, scripts :913-924, native sources :761-772). The direct members of a repeated module still resolve (added before the visited check), but the descent into its nested compositions is skipped on the second occurrence.
Failure scenario: template X composes Y twice (slots y1, y2); Y composes Z. Flattening produces y1.*, y1.z.*, y2.* — but not y2.z.*. No validation error fires (the members simply don't exist in the flattened config), so the instance deploys with the second slot's nested sensor/alarm/script members missing. CollisionDetector.CollectComposedMembers (CollisionDetector.cs:126-143) has the same shared-visited early-return, and there it skips even the repeated module's direct members — so collision detection is also blind under the second slot. The visited set is the right tool for cycle-guarding but the wrong tool for slot deduplication: composition is by-slot, not by-template. Fix is to key the guard on the path (prefix) or track only the current recursion stack.
[High] Revision hash and diff omit NativeAlarmSources — no staleness signal for native-alarm changes
RevisionHashService.HashableConfiguration (RevisionHashService.cs:116-152) hashes Attributes, Alarms, Scripts, Connections — not FlattenedConfiguration.NativeAlarmSources (populated at FlatteningService.cs:135). Editing a template's native alarm source (connection, source reference, condition filter) or an instance override therefore does not change the revision hash: the Deployments page never flags the instance stale, DeploymentService.GetDeploymentComparisonAsync reports IsStale = false, and sites keep mirroring the old alarm source indefinitely until some unrelated change forces a redeploy. This contradicts the spec's own claim that overrides "participate in the revision hash — changes … re-deploy as expected" (Component-TemplateEngine.md:215), which is implemented for connection-binding overrides but not for the native-alarm member class. DiffService should be audited for the same omission.
[High] Bundle import leaves stale compiled Inbound API handlers serving old code
ApplyApiMethodsAsync (BundleImporter.cs:2625) writes ApiMethod rows through IInboundApiRepository — a DB-level write. The Inbound API's singleton InboundScriptExecutor caches compiled delegates by method name with no content-based invalidation (InboundAPI/InboundScriptExecutor.cs:311-331: _scriptHandlers.TryGetValue(method.Name, …) → serve cached delegate) and caches known-bad methods the same way (:39, :58-62). An import that Overwrites an ApiMethod script keeps the old delegate serving (HTTP 200 with stale behavior); an import that fixes a previously-broken method keeps returning "Script compilation failed" from _knownBadMethods until restart. This is the exact failure mode already documented in project memory for DB-direct edits — Transport import is a DB-direct edit at scale, and nothing in ApplyAsync signals the executor. Cross-cluster deployments make this worse: the cache lives per central node.
[High] Locked native alarm sources can be overridden at instance level — lock not enforced anywhere
Spec: "Lock applies to the entire source" (Component-TemplateEngine.md:114). Reality: ApplyInstanceNativeAlarmSourceOverrides (FlatteningService.cs:775-792) applies the override with no lock check — contrast the attribute path (:309 if (existing.IsLocked) continue) and alarm path (:337). ResolvedNativeAlarmSource doesn't even carry IsLocked (the inherit-time lockedNames set at :676 is local and discarded). The management command path has no check either: ManagementActor.HandleSetInstanceNativeAlarmSourceOverride (ManagementService/ManagementActor.cs:882-909) upserts unconditionally after site-scope authorization. A site-scoped operator can repoint a locked native alarm binding to an arbitrary connection/source reference — precisely what locking exists to prevent on a read-only alarm mirror.
[Medium] Post-success audit failure flips a committed Success deployment record to Failed
DeployInstanceAsync commits the terminal Success status (DeploymentService.cs:304-306), runs post-success side effects (correctly swallowed, :970-1005), then calls _auditService.LogAsync inside the same try (:330-332). If that audit write throws (transient DB fault, serialization), the catch block (:343-397) unconditionally sets record.Status = Failed and persists it — central now reports Failed while the site runs the new config, the exact divergence the surrounding comments (:298-303) were written to prevent. It also violates the repo convention "audit-write failure NEVER aborts the user-facing action". The audit call should be outside the try or individually guarded like TryLogLifecycleTimeoutAsync (:1034-1066) already is for lifecycle timeouts.
[Medium] Deleting a NotDeployed instance still requires a live site round-trip
StateTransitionValidator.CanDelete explicitly allows delete-from-NotDeployed as "central-side record cleanup (no live site config to tear down)" (StateTransitionValidator.cs:40-49), but DeploymentService.DeleteInstanceAsync (DeploymentService.cs:546-585) unconditionally sends DeleteInstanceCommand to the site and fails on timeout. A Transport-imported instance (always lands NotDeployed) targeted at a site that is unreachable or not yet commissioned is undeletable through the normal path. The two components disagree about the same design sentence.
[Medium] Import blocker heuristic can hard-block a valid import with no operator override
The Pass-1 name scan (RunSemanticValidationAsync, BundleImporter.cs:3603-3692; shared with DetectBlockersAsync:751-818) flags any PascalCase(-shaped token not in the enumerative KnownNonReferenceNames denylist (:853-874) as a missing SharedScript/ExternalSystem and throws SemanticValidationException at apply — rolling back the whole import. Any script that defines a local PascalCase method (decimal ComputeRate(...) then ComputeRate(x)), uses a stdlib type not on the list (Regex.Match, StringBuilder, Json), or embeds SQL keywords outside the nine listed, is a false positive with no "proceed anyway" escape hatch — the operator must edit scripts or re-cut the bundle. The denylist will drift forever. Conversely Regex/StringBuilder being absent from the list means the heuristic fires on innocent code today. Consider downgrading Pass-1 findings on template scripts to warnings (the deploy gate re-validates authoritatively) and keeping hard-block only for ApiMethod scripts, or adding an explicit ignore resolution.
[Medium] OperationLockManager is single-node in-memory; concurrent ops via the standby node aren't excluded
Documented as acceptable ("lost on central failover", OperationLockManager.cs:12-13) and the implementation itself is careful (ref-counting, reservation-before-wait, exception-filter cleanup — :46-129). Residual risk: nothing structurally prevents both central nodes from executing mutating instance ops concurrently if traffic bypasses Traefik's active-node routing (direct ports 9001/9002 are documented in CLAUDE.md). Optimistic concurrency on DeploymentRecord is the backstop, but instance-state writes (DisableInstanceAsync:456-458) are last-write-wins. Low probability, but worth an explicit invariant (reject management ops on the non-active node).
Findings — Performance
[High] Every validation run compiles every script with Roslyn — CPU heavy and leaks loaded assemblies on the central node
ValidationService.ValidateScriptCompilation (ValidationService.cs:256-273) calls ScriptCompiler.TryCompile per script; that runs ScriptTrustValidator.FindViolations (a fresh CSharpCompilation.CreateScriptCompilation over the entire TPA reference set, ScriptTrustValidator.cs:114-124) plus RoslynScriptCompiler.Compile → CSharpScript.Create(...).Compile() (RoslynScriptCompiler.cs:70-71). Script.Compile() on success emits and loads the script assembly via Roslyn's InteractiveAssemblyLoader, which is not collectible — each compiled script permanently occupies loader/metadata memory in the central host.
This sits on hot paths: FlatteningPipeline.FlattenAndValidateAsync (FlatteningPipeline.cs:161-167) is invoked (a) per deploy, (b) per instance by DeploymentService.GetDeploymentComparisonAsync (DeploymentService.cs:652) — i.e. at query time whenever the Deployments page computes staleness, and (c) per deployed instance by StaleInstanceProbe during every template-overwriting bundle import (BundleImporter.ComputeStaleInstanceIdsAsync:1242-1327). A 50-instance environment with 10 scripts each = 500 full trust-compilations + 500 script-assembly loads per Deployments-page staleness sweep, unbounded over process lifetime. There is no compile-result cache. Fix: cache TryCompile verdicts keyed by SHA-256 of code (the verdict is a pure function of code + policy), and/or skip ValidateScriptCompilation on read-only comparison paths (staleness needs the hash, not a compile).
[Medium] LineDiffer worst case is O((N+M)²) memory with no input cap
The Myers implementation snapshots the full V frontier per edit distance (LineDiffer.cs:157-204: trace.Add((int[])v.Clone()) with v of length 2(N+M)+1, up to N+M rounds). Two ~10,000-line scripts with no common lines ⇒ ~20k rounds × 160 KB ≈ 3+ GB transient allocation during PreviewAsync. maxLines (400) caps only the output (:66-92); nothing caps the inputs, and the bundle size cap (100 MB) comfortably admits such scripts. A crafted (or merely bloated) bundle can OOM the active central node from the import preview. Cap input line counts (fall back to the <N lines> summary beyond a threshold) or use the linear-space Myers variant.
[Medium] Any template with a folder, parent, or composition can never diff Identical
ArtifactDiff.CompareTemplate compares FolderNameOf/BaseTemplateNameOf/CompositionTargetNameOf placeholder strings ("<id:5>") against real incoming names (ArtifactDiff.cs:64-65, 100, 674-693), so FolderName/BaseTemplateName/Compositions.* always register as changed for such templates. Effect: the wizard's "Identical → auto-skipped" contract (Component-Transport.md:222) never applies to structured templates; every re-import of an unchanged bundle shows spurious Modified rows and, if bulk-Overwrite is chosen, generates needless audit rows and stale-instance probing. The preview loop already hydrates templates (BundleImporter.cs:379-388); passing the folder/template name maps it also builds (:365-366) into CompareTemplate would fix all three placeholders cheaply.
[Low] LoadAsync is O(bundle) even when only the manifest is needed, and each load double-opens the zip
Acknowledged in the design doc itself (Component-Transport.md:90 — deferred ReadManifestAsync). Combined with sessions holding the full decrypted content (BundleSession.DecryptedContent) for up to 30 minutes and no cap on concurrent session count (BundleSessionStore.cs:38), N concurrent 100 MB uploads pin N×~200 MB. The T-007 zeroing on apply/failure (BundleImporter.cs:1105, 1194) is good; a session-count cap would round it out.
[Low] Import apply is one long EF transaction with per-entity audit rows and four intermediate flushes
ApplyAsync (BundleImporter.cs:960-1098) holds a relational transaction across full semantic validation (Roslyn-free — fine), all entity staging, per-row IAuditService.LogAsync calls, and stale-instance probing (which itself re-flattens per instance, see the Roslyn finding). For a large bundle this is minutes of open-transaction lock footprint on the shared config DB while the Blazor circuit waits. The CLI's raised 5-minute timeout acknowledges the symptom.
Findings — Security / Trust Model
[Medium] Deny-list gaps: Environment.Exit/FailFast (site-node kill), Environment.GetEnvironmentVariable (secret read)
ScriptTrustPolicy.ForbiddenScopes (ScriptTrustPolicy.cs:36-45) blocks IO/Process/Threading/Reflection/Net/Interop/Win32, but the System namespace root types remain fully available: a template script containing Environment.Exit(0) compiles cleanly against the surface and terminates the site-runtime process on first trigger — a one-line denial of service of an entire site node from the Design role. Environment.FailFast, Environment.GetEnvironmentVariable (exfiltrate SCADABRIDGE_API_KEY-class secrets via Notify/ExternalSystem.Call), and GC.Collect loops are similarly unblocked. These are cheap adds to ForbiddenScopes (as fully-qualified type-or-member entries) or to ForbiddenIdentifiers.
[Medium] System.Data allowance is an intentional but unbounded network hole
Site scripts may use raw ADO.NET (Database.Connection returns DbConnection; System.Data.Common is necessarily bindable — ScriptCompileSurface.cs:119). System.Data is absent from ForbiddenScopes, so if the site-runtime execution reference set includes any concrete provider (e.g. Microsoft.Data.SqlClient, which the Database helper itself needs), a script can new SqlConnection("Server=attacker;...") — an arbitrary-host network channel that the System.Net deny was meant to close. The semantic pass will not flag it (SqlClient is not under a forbidden scope). Worth either denying provider namespaces (Microsoft.Data, System.Data.SqlClient, System.Data.Odbc, …) while keeping System.Data.Common types reachable only via the Database helper, or documenting the accepted risk explicitly in Component-ScriptAnalysis.md.
[Medium] Reflection-gateway member list misses GetTypes, Invoke, EntryPoint, DeclaredMembers
ReflectionGatewayMembers (ScriptTrustPolicy.cs:66-88) blocks GetMethod/InvokeMember/etc. but not Invoke (on MethodInfo/Delegate), GetTypes, EntryPoint, DeclaredMethods. Pass 1 usually compensates — any expression whose resolved member's containing type is under System.Reflection is flagged semantically (ScriptTrustValidator.cs:180-212) — but in the documented TPA-degraded fallback mode (ScriptTrustPolicy.cs:207-245) resolution weakens and Pass 2 becomes the primary defence for chained accesses; the syntactic list should be closed under the obvious invocation members since it exists precisely for that mode. Cheap hardening.
[Medium] Transport import bypasses the script trust gate entirely
ApplyAsync writes template scripts, shared scripts, and API-method scripts to the DB without ever invoking ScriptTrustValidator (RunSemanticValidationAsync at BundleImporter.cs:3603 is name-resolution + SemanticValidator only). Compensating controls exist downstream — the deploy gate re-validates template scripts (ScriptCompiler.TryCompile), the Inbound API re-checks trust at lazy compile — but the layering claim in Component-ScriptAnalysis.md ("all call sites delegate…") now has a fifth write path with no gate, and the Inbound API's check happens at serve time on the production node rather than at import review time. Running FindViolations over imported script bodies during Pass 2 (they're already in memory) would restore the design-time boundary and give the operator the finding in the wizard instead of a runtime 500.
Positive observations (crypto & envelope)
BundleSecretEncryptor is textbook: AES-256-GCM with 16-byte salt / 12-byte nonce / 16-byte tag, PBKDF2-SHA256 600k iterations, fresh salt+nonce per encrypt (BundleSecretEncryptor.cs:14-58). The T-005 AAD binding of non-derivative manifest fields (BundleManifestAad.cs:45-55, verified in LoadAsync at BundleImporter.cs:252-255) makes the source-environment confirmation gate tamper-evident — a genuinely thoughtful touch. T-006 zip-bomb caps validate central-directory headers before any decompression (BundleImporter.cs:296-338); T-003 lockout is keyed by content hash so a second tab/CLI can't sidestep it (BundleSessionStore.cs:40-47, 122-167); T-007 zeroes decrypted plaintext on both success and failure. Key management is passphrase-only with no persistence — appropriate for the file-carry threat model, with the residual (documented) weakness that unencrypted export is allowed and only audit-flagged.
Findings — Conventions & Spec Drift
[Medium] Instance alarm overrides implement less than the spec grants
Component-TemplateEngine.md:113 says instance-overridable alarm fields include Description and On-Trigger Script reference; ApplyInstanceAlarmOverrides (FlatteningService.cs:328-355) applies only TriggerConfigurationOverride and PriorityLevelOverride. Either the doc or the entity/flattener should move.
[Medium] ArtifactDiff child-equality predicates miss fields that the import sync does write
AttributesEqual omits ElementDataType and LockedInDerived (ArtifactDiff.cs:646-651); AlarmsEqual omits the on-trigger script reference (:653-658); ScriptsEqual omits MinTimeBetweenRuns/ExecutionTimeoutSeconds (:660-666). A bundle differing only in those fields classifies Identical → auto-skipped → silently not imported (for the fields the sync would have written) — the mirror image of the false-Modified problem above. Diff predicates and sync-helper changed predicates should be generated from one field list per entity so they cannot drift (they already have, in both directions).
[Low] Bundle format spec advertises a scripts/ directory that is never produced, while MaxBundleEntryCount defaults to 4
Component-Transport.md:45-47 documents optional scripts/template-{id}-{name}.cs entries; no code writes or reads them, and TransportOptions.MaxBundleEntryCount = 4 (TransportOptions.cs:22) would reject a bundle that actually used the feature for more than two scripts. Remove from the spec or note it as unimplemented.
[Low] ScriptCompiler.TryCompile surfaces only the first violation/error
Validation/ScriptCompiler.cs:42,47 return violations[0] / errors[0]; an operator fixing a script with five issues gets five deploy round-trips. Both underlying APIs return full lists — join them.
[Low] RevisionHashService determinism contract is fragile but guarded
Hash stability depends on alphabetical property declaration order in the Hashable* records (RevisionHashService.cs:12-19, 113-115); the guard test (HashableRecords_PropertiesDeclaredAlphabetically) exists, which is the right mitigation. Note for maintainers: adding NativeAlarmSources (see the High finding) will change all hashes — plan it as a deliberate migration (one-time global staleness) rather than a patch.
Positive observations (conventions)
DeploymentService is exemplary on the repo's own rules: CancellationToken.None for failure/audit writes after cancellation (DeploymentService.cs:355-372), timeout-audit entries for all lifecycle verbs, reconciliation preserving operator-intended Disabled state (:895-907), terminal-status-before-side-effects ordering (:298-306), and the notify-and-fetch PendingDeployment TTL rationale (:283-290) is documented where the decision lives. CycleDetector.BuildLookup duplicate-Id tolerance and the Id-0-is-legitimate handling (CycleDetector.cs:21-27, 56-59) show real defect-driven hardening. ScriptTrustPolicy's DefaultReferences vs AnalysisReferences separation with the loud TPA-degradation flag (ScriptTrustPolicy.cs:207-245) is a well-reasoned two-layer defence.
Underdeveloped Areas
- Transport site/instance wave (M8) shipped ahead of its template-fidelity basics. Instances, connections, name-mapping, FK rewiring, and stale-probing are all implemented and tested (
SiteInstanceImportTests.cs,CompositionImportTests.cs, rollback-failure tests), but the older template payload silently loses inheritance, cadence/timeout, derived-lock flags, and native alarm sources (findings above). The DTO layer (EntitySerializer.FromBundleContent) actually implements more fidelity than the apply path uses — the importer re-implements DTO→entity mapping by hand (BuildTemplate) and drifted. - No import test exercises a derived template.
Transport.IntegrationTestscovers round-trip, conflict resolution, composition edges, semantic validation failure — but grep shows zero inheritance coverage. A singleexport(base+derived) → import → flattentest would have caught the Critical finding. - No flattening test for repeated composition.
TemplateEngine.Tests/Flatteningexists but the shared-visitedsemantics (same template in two slots of one subtree) is untested; same forCollisionDetector. - Deferred items acknowledged in docs but load-bearing: manifest-only
ReadManifestAsync(perf), Configuration-Audit-Log "Bundle Import" filter UI (Transport-012), rename call-site rewriting ("call sites are not rewritten in v1" —BundleImporter.cs:1519-1522, meaning a Rename resolution on a shared script produces a bundle whose importing templates still call the old name, satisfied only because the name-set registers both — the target DB ends up with scripts calling a name that may not exist there; only Pass-1's dual registration hides it at import time). ArtifactDiffversion fields are permanently null (ArtifactDiff.cs:33-37) — the manifest's per-artifactversionnumbers are display-only; no optimistic concurrency exists between preview and apply, so a target edited between Preview and Apply is silently overwritten (session TTL is 30 min — a real window).- Compile-surface parity relies on three hand-maintained mirrors (
ScriptCompileSurface,TriggerCompileSurface, Central-UISandboxScriptHost), guarded by reflection tests for two of them and representative-script tests for the third (Component-ScriptAnalysis.md:141-145). Adequate but fragile; a source-generator would eliminate the class of drift. SemanticValidatorcall-target extraction is string-based and self-admittedly cannot resolve dynamic child names (SemanticValidator.cs:43-56— leaf-name fallback accepts any composed script with a matching leaf, so a wrong-childChildren[x].CallScript("Y")passes design-time validation). Documented tradeoff, but worth a warning-level finding rather than silence.
Prioritized Recommendations
- [Critical] Wire template inheritance in the import apply path. Add a
ResolveInheritanceEdgesAsyncpass mirroringResolveCompositionEdgesAsync(resolveBaseTemplateNameagainst Local + target by name, honouring Rename), run it after the template flush, and runCycleDetector.DetectInheritanceCycle/DetectCompositionCycleover the merged graph before commit (the importer currently performs no acyclicity check — a crafted bundle can persist a composition cycle that the authoring path would reject). Add a derived-template round-trip integration test. - [High] Close the import data-loss set in one sweep: carry
MinTimeBetweenRuns/ExecutionTimeoutSecondsthroughBuildTemplate+SyncTemplateScriptsAsync; addLockedInDerivedto the attribute/alarm/script DTOs and syncs; addTemplateNativeAlarmSourceDto(or block export of templates that have native alarm sources until supported). Drive diff predicates and sync predicates from a single per-entity field list. - [High] Fix the repeated-composition
visitedbug in the fourResolveComposed*Recursivemethods andCollisionDetector.CollectComposedMembers— guard on recursion path, not global template Id. - [High] Add
NativeAlarmSourcestoRevisionHashService(andDiffService) as a planned hash-migration, restoring staleness detection for native-alarm edits. - [High] Cache script-compile verdicts by code hash and stop loading script assemblies on validation-only paths; skip
ValidateScriptCompilationinGetDeploymentComparisonAsync/StaleInstanceProbe(staleness needs only the hash). This removes both the CPU spike and the non-collectible assembly leak from the Deployments page and bundle imports. - [High] Invalidate Inbound API handler caches on import — publish an ApiMethod-changed notification from
ApplyAsync(or key_scriptHandlersby(name, scriptHash)), covering the management-path gotcha already in project memory at the same time. - [High] Enforce locks on native alarm source overrides at both
ManagementActor.HandleSetInstanceNativeAlarmSourceOverrideandFlatteningService.ApplyInstanceNativeAlarmSourceOverrides(carryIsLockedonResolvedNativeAlarmSource). - [Medium] Harden the deny-list: add
System.Environmentmembers (Exit,FailFast,GetEnvironmentVariable(s),SetEnvironmentVariable), close the reflection-gateway list (Invoke,GetTypes,EntryPoint), and decide/document theSystem.Dataprovider posture. - [Medium] Deployment audit-write isolation: move the post-success
LogAsyncinDeployInstanceAsyncout of the try (or guard it) so an audit fault can never flip a committed Success record to Failed; skip the site round-trip when deletingNotDeployedinstances. - [Medium] Bound
LineDifferinputs; fixArtifactDiffplaceholder comparisons (pass folder/template name maps intoCompareTemplate) so Identical classification works for structured templates; consider an "ignore blocker" resolution for the Pass-1 heuristic on template scripts.