Joseph Doherty
5bbd7689fa
Re-ran all 8 domain reviews at HEAD8c888f13against theb910f5ebbaseline: every round-1 finding source-verified (168 fixed, 0 regressions, 0 false claims); 56 new findings (1 Critical / 4 High / 15 Medium / 36 Low), concentrated in post-baseline code (anti-entropy resync, KPI rollup backfill, live alarm stream) and seams the fixes exposed. Headliners: S&F resync predicate inversion can wipe the delivering node's buffer (02-N1 Critical); resync snapshot exceeds the Akka remoting frame size (02-N2); failover drill kills the one node keep-oldest can't survive (01-N1); unbounded rollup backfill per failover (04-R1); live production API key in untracked test.txt (08-NF1). Adds PLAN-R2-01..08 + .tasks.json manifests and the Round-2 board, P0 list, cross-plan mutexes, and wave order in 00-MASTER-TRACKER.
24 KiB
Architecture Review 05 — Design-Time Pipeline: Templates, Deployment, Transport, Script Analysis (Round 2)
Date: 2026-07-12 (round 2; round-1 baseline commit b910f5eb, re-review at HEAD 8c888f13)
Scope
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 the test projects. Cross-component seams re-checked: Inbound API compiled-handler cache (InboundScriptExecutor) as the consumer of the plan-05/plan-06 invalidation work; ManagementActor native-alarm-source override handlers.
Method
- Re-read the round-1 report (the "silent data loss in Transport import" report: 1 Critical, 7 High, 10 Medium, 5 Low, 7 underdeveloped areas) and
archreview/plans/PLAN-05-templates-deployment-transport.md(27 fixed / 6 deferred claimed). - Verified every round-1 finding's true disposition in current source — code read directly, plan claims not trusted. All dispositions below carry file:line evidence from HEAD.
- Fresh sweep of
git diff b910f5eb..HEADscoped to the four projects (~1,900 insertions across 36 files; 31 commits) for new findings: stability, performance, security, conventions.
One-line verdict: Round 1 found a Transport import that silently amputated template inheritance, cadence/timeout, lock flags, and native alarm sources; round 2 finds all 26 actionable findings genuinely fixed in code (0 regressions, 5 recorded-decision deferrals intact), guarded by a reflection round-trip suite that itself caught 5 further bugs — the residue is 1 Medium performance leftover (Expression-trigger Roslyn compiles on read-only paths) and 5 Lows.
Round-1 Finding Disposition
Legend: F = Fixed (verified in source), P = Partially fixed, D = Deferred (accepted, recorded), N/A = no longer applicable.
| # | Round-1 finding | R1 sev | Disposition | Evidence (file:line) |
|---|---|---|---|---|
| 1 | Bundle import drops template inheritance edges (derived templates land as roots) | Critical | F | ResolveInheritanceEdgesAsync (Transport/Import/BundleImporter.cs:2542-2616), called from ApplyAsync at :1261 after the composition rewire; honours Rename via the resolution map (:2576-2580), clears stale edges for root-template bundles (:2566-2569), unresolvable base → null edge + BundleImportBaseTemplateUnresolved audit row, never throws (:2593-2612). Round-trip + rename tests: tests/.../Transport.IntegrationTests/Import/InheritanceImportTests.cs |
| 1b | (Rec. #1 rider) importer performs no acyclicity check — crafted bundle can persist a cycle | Critical (rider) | F | EnsureNoTemplateGraphCycles (BundleImporter.cs:2633-2670) runs CycleDetector.DetectInheritanceCycle/DetectCompositionCycle over the merged change-tracker graph, called before any SaveChanges of the staged edges (:1275-1287); completeness argument sound — any edge to a pre-existing template forces a tracked GetAllTemplatesAsync load, which Includes Compositions/NativeAlarmSources (ConfigurationDatabase/Repositories/TemplateEngineRepository.cs:69-79) |
| 2 | Imported scripts lose MinTimeBetweenRuns/ExecutionTimeoutSeconds; LockedInDerived absent from DTOs |
High | F | DTOs carry all three (Transport/Serialization/EntityDtos.cs:210, 221, 231-236); BuildTemplate copies them (BundleImporter.cs:1832, 1844, 1856-1858); all four SyncTemplate*Async overwrite paths copy them (:1941, 2067, 2169-2171, 2277); fidelity tests Import/TemplateScriptFidelityTests.cs |
| 3 | Template NativeAlarmSources not transported; carried instance overrides dangle |
High | F | TemplateNativeAlarmSourceDto (EntityDtos.cs:187-195) with back-compat empty default (:168-178); export + BuildTemplate (BundleImporter.cs:1866-1872) + SyncTemplateNativeAlarmSourcesAsync (:2237-2330, called from Overwrite at :1726); preview diff via DiffChildren (Import/ArtifactDiff.cs:123-128); tests Import/NativeAlarmSourceImportTests.cs incl. the override-no-longer-dangles case |
| 4 | Flattener drops grandchild members on repeated composition; CollisionDetector blind | High | F | All four ResolveComposed*Recursive methods guard on the recursion path (add-on-entry / remove-in-finally): attributes TemplateEngine/Flattening/FlatteningService.cs:286-307, alarms :659-680, native sources :782-803, scripts :947+; CollisionDetector.CollectComposedMembers same conversion with explicit rationale comment (CollisionDetector.cs:126-155) |
| 5 | Revision hash and diff omit NativeAlarmSources — no staleness signal |
High | F | HashableNativeAlarmSource (Flattening/RevisionHashService.cs:280-298), folded null-when-empty so native-source-free hashes stay byte-identical (:103-114 — surgical migration, only affected instances flip stale once); DiffService sweeps native sources (Flattening/DiffService.cs:47-50, 149); reflective alphabetical guard auto-covers the new record (tests/.../RevisionHashServiceTests.cs:103-132) plus ComputeHash_NativeAlarmSourceChange_ChangesHash (:371) |
| 6 | Bundle import leaves stale compiled Inbound API handlers serving old code | High | F | Two-layer fix. Contract + publisher (plan-05 T14): IScriptArtifactChangeBus (Commons/Interfaces/Scripting/IScriptArtifactChangeBus.cs:17), post-commit publish from ApplyAsync (BundleImporter.cs:1340, 1777-1816), swallow-and-log. Consumer (plan-06) went stronger than name-eviction: InboundScriptExecutor compares cached handler script text against the current DB row and recompiles on mismatch, and _knownBadMethods fast-fails only when the current text equals the recorded bad text (InboundAPI/InboundScriptExecutor.cs:370-404) — content-based self-healing that also covers failover/direct-SQL edits |
| 7 | Locked native alarm sources overridable at instance level — lock enforced nowhere | High | F | ResolvedNativeAlarmSource.IsLocked propagated incl. derived-shadow locks (FlatteningService.cs:722); ApplyInstanceNativeAlarmSourceOverrides skips locked (:815); ManagementActor rejects on locked source for both single and batch handlers, and rejects dangling canonical names (ManagementService/ManagementActor.cs:953-1043, throws at :978, :1037) |
| 8 | Post-success audit failure flips committed Success deployment to Failed | Medium | F | Deploy audit routed through guarded TryLogAuditAsync explicitly outside the failure path (DeploymentManager/DeploymentService.cs:329-336, rationale comment in place) |
| 9 | Deleting a NotDeployed instance requires a live site round-trip |
Medium | F | DeleteInstanceAsync short-circuits NotDeployed to a central-only record delete + audit with SiteRoundTripSkipped = true, no communication-service call (DeploymentService.cs:567-595); doc aligned (Component-DeploymentManager.md) |
| 10 | Import blocker heuristic hard-blocks valid imports, no override | Medium | F | Three-part fix: locally-declared functions/methods excluded per-origin (BundleImporter.cs:792-811, CollectLocalDeclarations Roslyn script-parse at :921-944); denylist extended; severity split — template-script findings are ConflictKind.Warning/ImportResult.Warnings (advisory, deploy gate re-validates), ApiMethod findings stay Blocker/hard error (:819, :842-846, apply side :4444-4460) |
| 11 | OperationLockManager single-node in-memory; standby-node ops not excluded |
Medium | D (accepted) | Invariant recorded as a decision block (Component-DeploymentManager.md:94); structural active-node gating deferred to the cluster/UI plans (01/07) per PLAN-05 coverage table — matches what shipped |
| 12 | Every validation run Roslyn-compiles every script — CPU + non-collectible assembly load on read paths | High (perf) | F (residual → new finding N1) | Verdict cache keyed on SHA-256 of code, name-free errors, 4096-entry bound (TemplateEngine/Validation/ScriptCompileVerdictCache.cs:28-69; consumed at ScriptCompiler.cs:42-60); read-only paths skip the compile stage entirely — GetDeploymentComparisonAsync (DeploymentService.cs:690) and StaleInstanceProbe (StaleInstanceProbe.cs:37) pass validateScripts: false through FlatteningPipeline.cs:58, 162-174. Script bodies fully covered; Expression-trigger compiles remain on read paths — see N1 |
| 13 | LineDiffer O((N+M)²) memory, no input cap |
Medium (perf) | F | MaxInputLines = 4000 (Transport/Import/LineDiffer.cs:62); oversized inputs short-circuit to summary-only before the Myers trace (:84, SummaryOnlyResult :119); documented (Component-Transport.md:23) |
| 14 | Templates with folder/parent/composition never diff Identical (placeholder comparisons) |
Medium (perf) | F | CompareTemplate takes optional folderNameById/templateNameById maps (ArtifactDiff.cs:64-65, 75-76), resolvers fall back to placeholders only when a map misses (:679-700); PreviewAsync passes the maps it already builds (BundleImporter.cs:395) |
| 15 | LoadAsync O(bundle) even for manifest; unbounded session count |
Low (perf) | P | Session cap shipped: MaxConcurrentImportSessions = 8 (TransportOptions.cs:37) enforced with evict-expired-first in BundleSessionStore.Open (BundleSessionStore.cs:78-88). Manifest-peek ReadManifestAsync deferred as planned, honestly documented (Component-Transport.md:92) |
| 16 | Import apply is one long EF transaction | Low (perf) | D (accepted) | Per plan: dominant cost (Roslyn in StaleInstanceProbe) removed by #12; restructuring the single-transaction rollback contract judged high-risk/low-gain. Transaction shape unchanged (BundleImporter.cs apply path) — accepted |
| 17 | Deny-list gaps: Environment.Exit/FailFast/GetEnvironmentVariable, GC |
Medium (sec) | F | ForbiddenScopes += System.Environment, System.GC with rationale comments (ScriptAnalysis/ScriptTrustPolicy.cs:45-52). Bonus beyond plan: ScriptTrustValidator now compiles the analysis with the runtime's DefaultImports (WithUsings), closing the bare-identifier blind spot where Environment.Exit(0) under using System; under-resolved (ScriptTrustValidator.cs:118-132) |
| 18 | System.Data allowance is an unbounded network hole (concrete providers) |
Medium (sec) | F | Microsoft.Data, System.Data.SqlClient, System.Data.Odbc, System.Data.OleDb denied; System.Data.Common abstractions deliberately kept (Database helper posture documented in the code comment and Component-ScriptAnalysis.md) (ScriptTrustPolicy.cs:53-61) |
| 19 | Reflection-gateway member list incomplete (GetTypes, Invoke, EntryPoint, Declared*) |
Medium (sec) | F | GetTypes, EntryPoint, DeclaredMethods/Members/Constructors, DynamicInvoke added; Invoke deliberately excluded with recorded rationale (delegate Invoke legitimate; MethodInfo.Invoke caught semantically) (ScriptTrustPolicy.cs:105-116) |
| 20 | Transport import bypasses the script trust gate entirely | Medium (sec) | F | Pass 0 trust gate at apply (BundleImporter.cs:4301-4335) and preview (:849-905): ScriptTrustValidator.FindViolations over every non-Skip template/shared/ApiMethod body and template script + alarm Expression-trigger bodies (EnumerateTrustGatedScripts :957-1001, ExtractTriggerExpression :1004-1035); hard error for all kinds; fail-closed on validator throw. Component-ScriptAnalysis.md:5, 173, 193 names Transport the fifth call site |
| 21 | Instance alarm overrides implement less than spec (Description / On-Trigger ref) | Medium (conv) | F (doc aligned) | Spec now records the narrower granularity as the decision — only TriggerConfigurationOverride/PriorityLevelOverride overridable; Description/On-Trigger ref are template-level; adding columns is future feature work (Component-TemplateEngine.md:113) |
| 22 | ArtifactDiff equality predicates drift from sync predicates (both directions) |
Medium (conv) | F | TemplateChildEquality single source of truth — complete writable-field comparers for attributes/alarms/scripts/native sources incl. ElementDataType, LockedInDerived, on-trigger-script-by-name resolver, cadence/timeout (Transport/Import/TemplateChildEquality.cs:35-100); both ArtifactDiff (:118-128) and all four sync changed predicates route through it (BundleImporter.cs:1931, 2045, 2160, 2268) |
| 23 | Spec advertises unimplemented scripts/ bundle dir vs MaxBundleEntryCount = 4 |
Low | F | Spec corrected: scripts travel inside the content blob, a well-formed bundle has exactly two zip entries, separate scripts/ dir explicitly not part of the format (Component-Transport.md:49-51) |
| 24 | TryCompile surfaces only the first violation/error |
Low | F (residual → N2) | ScriptCompiler.TryCompile joins ALL violations / ALL compile errors (ScriptCompiler.cs:46-58). The trigger-expression path was not covered — see N2 |
| 25 | RevisionHashService determinism contract fragile |
Low | F | Guard test enumerates every nested Hashable* record reflectively, so new records are auto-covered (RevisionHashServiceTests.cs:103-132); migration note in the class doc (RevisionHashService.cs:23) |
| U1 | Template fidelity shipped behind site/instance wave | — | F | Tasks 1–8 landed; the reflection round-trip equivalence suite (tests/.../Transport.IntegrationTests/RoundTripEquivalenceTests.cs) structurally guards every transported entity type and itself caught 5 further silent-loss bugs, fixed in the importer/exporter (commit 7c74bbe4: ES retry/methods-on-Add, notification recipients on export, folder ParentFolderId on Add) |
| U2 | No derived-template import test | — | F | Import/InheritanceImportTests.cs (Add + Overwrite + renamed-base cases) |
| U3 | No repeated-composition flattening/collision test | — | F | Tests extended with the y1.z.* / y2.z.* two-slot cases + cycle-termination regression guard (commits 730bf191, 874e32a9) |
| U4 | Deferred-but-load-bearing: rename call-site rewriting, Transport-012 UI, ReadManifestAsync |
— | D (accepted) | Rename limitation now explicit and load-bearing in the spec; Transport-012 filter UI documented as deferred with the CLI workaround (Component-Transport.md:354); ReadManifestAsync deferral documented (:92) |
| U5 | Preview→apply window has no optimistic concurrency (version fields null) | — | D (accepted) | Recorded decision paragraph — resolutions keyed by (EntityType, Name), single-operator workflow accepted for v1, version fields reserved (Component-Transport.md:138; ArtifactDiff.cs:33-37) |
| U6 | Three hand-maintained compile-surface mirrors | — | D (accepted) | Guard tests exist; source-generator remains an improvement, not a defect. ScriptCompileSurface changes since baseline are XML-doc only (verified via diff) |
| U7 | SemanticValidator leaf-name fallback silently accepts wrong-child calls |
— | F | Leaf-name-only matches now emit ValidationEntry.Warning(CallTargetNotFound, "... matched by composed leaf name only — child path not verified") (TemplateEngine/Validation/SemanticValidator.cs:123-131) |
Disposition counts: 26 Fixed (verified) · 1 Partially fixed (#15 — remainder is the doc-acknowledged ReadManifestAsync deferral) · 5 Deferred (accepted, all recorded as decisions in the design docs) · 0 Not fixed · 0 Regressed · 0 No longer applicable.
New Findings (round 2)
Fresh review of the ~1,900 lines added across the four projects since b910f5eb (31 commits: the PLAN-05 fix wave, PLAN-07's DeployToSiteAsync site-scope slice, PLAN-06's ESG TimeoutSeconds pipeline ride-along, PLAN-08's options validators, and a docs/fixdocs sweep).
Performance
[Medium] N1 — Expression-trigger validation still Roslyn-compiles on read-only staleness paths, uncached
The validateScripts: false fix (#12) gates only ValidateScriptCompilation; ValidateExpressionTriggers runs unconditionally in the Validate pipeline (TemplateEngine/Validation/ValidationService.cs:128-134), and its CheckExpressionSyntax performs a fresh ScriptTrustValidator.FindViolations (a full CSharpCompilation over the TPA reference set) plus RoslynScriptCompiler.Compile per Expression-triggered script and alarm, with no verdict cache (ValidationService.cs:531-546). For an environment that uses Expression triggers, every Deployments-page staleness sweep (DeploymentService.GetDeploymentComparisonAsync:690) and every template-overwriting bundle import (StaleInstanceProbe.cs:37) still pays N×(trust-compilation + script compile) — the exact hot-path cost class round 1 flagged, surviving in a smaller member class. Fix: either gate ValidateExpressionTriggers' syntax check behind the same validateScriptCompilation flag (the attribute-reference scan can stay), or cache the verdict — if the ScriptCompileVerdictCache is reused, the key must gain the globals-surface discriminator: it is currently keyed on code alone (ScriptCompileVerdictCache.cs:53), which is sound only while ScriptCompiler (always ScriptCompileSurface) is its sole writer; a trigger expression valid against TriggerCompileSurface is not interchangeable with a script-body verdict.
Stability & Correctness
[Low] N2 — CheckExpressionSyntax surfaces only the first violation / first compile error
ValidationService.cs:534-544 returns violations[0] / errors[0]. Task 24 fixed exactly this shape in ScriptCompiler.TryCompile (full joined lists) but the trigger-expression twin was missed — an operator fixing a multi-error expression gets one deploy round-trip per error. Same one-line string.Join fix.
[Low] N3 — PublishScriptArtifactChanges skips Add resolutions
The post-commit publisher notifies only Overwrite/Rename resolutions (BundleImporter.cs:1798-1803). Edge case: an ApiMethod is deleted on the target, then re-imported as Add under the same name — any node still holding a _knownBadMethods/handler entry for that name receives no notification. Mitigated to Low because the plan-06 consumer self-heals by content comparison (InboundScriptExecutor.cs:370-379), which is exactly the contract's stated design (bus is advisory, consumers must self-heal); noting it so a future non-self-healing subscriber doesn't inherit the gap silently.
[Low] N4 — Import persists silently-inert instance overrides on locked members
ApplyInstancesAsync writes InstanceAttributeOverride / InstanceNativeAlarmSourceOverride rows straight from the bundle (BundleImporter.cs:4001, 4022) with none of the lock checks the ManagementActor enforces for the same writes (ManagementActor.cs:898, 978). No security bypass — the flattener drops locked-member overrides (FlatteningService.cs:319, 815) — but the imported rows are dead data and the operator gets no warning that part of the bundle's instance config will never take effect. A ConflictKind.Warning row (the Task-19 machinery is right there) would surface it in the wizard.
Security / Trust Model
[Low] N5 — Import trust gate does not scan instance-override Expression-trigger bodies
EnumerateTrustGatedScripts covers template script bodies, template script/alarm trigger expressions, shared scripts, and ApiMethods (BundleImporter.cs:957-1001) — but not InstanceAlarmOverrideDto.TriggerConfigurationOverride, which can carry an {"expression": ...} body when the overridden alarm is Expression-triggered. Not exploitable pre-runtime: imported instances land NotDeployed, and the deploy gate's ValidateExpressionTriggers runs on the flattened config (post-override) with the same authoritative FindViolations (ValidationService.cs:531-539), so a forbidden expression is rejected before it can execute. This is an import-review completeness gap only — the operator learns at deploy time instead of in the wizard, the exact UX the Task-20 gate was built to improve. Cheap add: run ExtractTriggerExpression over instance alarm-override configs in the enumerator.
Conventions
[Low] N6 — TransportOptionsValidator misses the knob its own fix wave added
The validator covers ten options including the PBKDF2 floor (TransportOptionsValidator.cs:25-66) but not MaxConcurrentImportSessions — configured to 0/negative, BundleSessionStore.Open's _sessions.Count >= cap (BundleSessionStore.cs:78-88) rejects every import forever with "Too many concurrent import sessions (0)". Fail-closed, so no safety issue, but it contradicts the validator's stated purpose ("a zero/negative value would either disable a cap [or] crash the import pipeline") and is a one-line RequireThat.
What's genuinely good
- The round-trip reflection guard is the standout of the fix wave.
RoundTripEquivalenceTestscompares every public writable property of every transported entity across export→import, so the class of bug that produced round-1's Critical + two Highs (hand-written DTO↔entity mapping drifting) is now structurally impossible to reintroduce silently — and it proved itself immediately by catching 5 residual fidelity holes the per-field unit tests had all passed over (commit7c74bbe4). - Fixes routinely exceeded their tickets. The trust gate covers Expression-trigger bodies, not just script bodies (
BundleImporter.cs:957-1001); the deny-list work also fixed a real analyzer blind spot (WithUsings(ScriptTrustPolicy.DefaultImports)so bareEnvironment/GCresolve to their forbidden types —ScriptTrustValidator.cs:118-132); the Inbound API consumer chose content-based invalidation over name eviction, which also self-heals failover and direct-SQL edits (InboundScriptExecutor.cs:370-404); theManagementActorlock fix also rejects dangling canonical names, closing the create-inert-override path at the management surface (ManagementActor.cs:961-980). - Deferrals are honest and recorded where the next reader will look. The preview→apply concurrency window (
Component-Transport.md:138), theOperationLockManagernode-affinity invariant (Component-DeploymentManager.md:94), the rename-doesn't-rewrite-call-sites limitation, and theReadManifestAsynccost note are all written as explicit decisions with rationale, not silently dropped. - The import-time acyclicity guard's completeness reasoning is documented in the code (
BundleImporter.cs:2633remarks) and holds up under adversarial reading — the rewire passes' full-load fallback plusGetAllTemplatesAsync'sCompositionsInclude guarantee every node of any candidate cycle is tracked. - Single-sourcing done right:
TemplateChildEqualitycarries the "adding a writable field means adding it here, once" contract in its doc comment, and both consumers verifiably route through it; the revision-hash alphabetical guard enumerates nested records reflectively so new member classes are covered by construction.
Severity tally — NEW findings only
| Severity | Count | Findings |
|---|---|---|
| Critical | 0 | — |
| High | 0 | — |
| Medium | 1 | N1 (Expression-trigger compiles on read-only paths, uncached) |
| Low | 5 | N2 (first-error-only trigger syntax), N3 (publisher skips Add), N4 (inert locked-member overrides on import), N5 (instance-override trigger bodies not import-gated), N6 (session-cap knob unvalidated) |
Prioritized recommendations: (1) gate or cache the Expression-trigger syntax check on read-only paths — keying any shared cache by globals surface (N1); (2) fold N2/N6 into the next low-severity sweep (two one-liners); (3) extend EnumerateTrustGatedScripts to instance alarm-override expressions and emit a wizard warning for locked-member overrides (N4+N5 share the same code region).