scadalink-design/code-reviews

Code Reviews

Comprehensive, per-module code reviews of the ScadaLink codebase. Each module (one buildable project under src/) has its own folder containing a findings.md. This README is the aggregated index — the single place to see all outstanding work.

Generated by regen-readme.py from the per-module findings.md files. Do not edit by hand — edit the findings files and re-run the script.

How it works

• Reviews are performed one module at a time against a fixed checklist.
• Every finding is recorded in the module's findings.md with a severity and status.
• Findings are never deleted — they are closed by changing their status, keeping a full audit trail.
• This README aggregates every pending finding (Open / In Progress) across all modules.

See REVIEW-PROCESS.md for the full procedure: the review checklist, severity definitions, finding format, and how to mark items resolved.

Layout

code-reviews/
├── README.md            # this file — process overview + pending findings
├── REVIEW-PROCESS.md     # how to perform a review and track findings
├── regen-readme.py       # regenerates this README from the findings files
├── _template/findings.md # copy-this template for a module review
└── <Module>/findings.md  # one folder per src/ project

Baseline review — 2026-05-16

All 19 modules were reviewed at commit 9c60592 (241 findings: 6 Critical, 46 High, 100 Medium, 89 Low). The tables below track what remains open as findings are resolved and re-triaged; findings discovered after the baseline are appended to their module file and counted in Total.

Severity	Open findings
Critical	0
High	3
Medium	100
Low	90
Total	193

Module Status

Module	Last reviewed	Commit	Open (C/H/M/L)	Open	Total
CLI	2026-05-16	9c60592	0/0/6/6	12	13
CentralUI	2026-05-16	9c60592	0/0/10/5	15	19
ClusterInfrastructure	2026-05-16	9c60592	0/1/4/3	8	8
Commons	2026-05-16	9c60592	0/0/4/8	12	12
Communication	2026-05-16	9c60592	0/0/5/3	8	11
ConfigurationDatabase	2026-05-16	9c60592	0/0/4/6	10	11
DataConnectionLayer	2026-05-16	9c60592	0/0/6/2	8	13
DeploymentManager	2026-05-16	9c60592	0/1/6/5	12	14
ExternalSystemGateway	2026-05-16	9c60592	0/0/7/4	11	14
HealthMonitoring	2026-05-16	9c60592	0/0/5/5	10	12
Host	2026-05-16	9c60592	0/0/3/7	10	11
InboundAPI	2026-05-16	9c60592	0/0/5/5	10	13
ManagementService	2026-05-16	9c60592	0/0/5/5	10	13
NotificationService	2026-05-16	9c60592	0/0/5/3	8	12
Security	2026-05-16	9c60592	0/0/4/4	8	11
SiteEventLogging	2026-05-16	9c60592	0/0/4/3	7	11
SiteRuntime	2026-05-16	9c60592	0/0/8/5	13	16
StoreAndForward	2026-05-16	9c60592	0/0/4/7	11	14
TemplateEngine	2026-05-16	9c60592	0/1/5/4	10	14

Pending Findings

Every Open / In Progress finding across all modules, highest severity first. Resolved findings drop off this list but remain recorded in their module's findings.md (see REVIEW-PROCESS.md §4–§5). Full detail — description, location, recommendation — lives in the module's findings.md.

Critical (0)

None open.

High (3)

ID	Module	Title
ClusterInfrastructure-001	ClusterInfrastructure	Module implements none of its documented responsibilities
DeploymentManager-006	DeploymentManager	Query-the-site-before-redeploy idempotency requirement not implemented
TemplateEngine-002	TemplateEngine	Derived templates omit all base alarms; composed alarms cannot be overridden per slot

Medium (100)

ID	Module	Title
CLI-002	CLI	Empty success body crashes table rendering with an unhandled exception
CLI-003	CLI	Non-JSON success body crashes table rendering
CLI-004	CLI	Malformed --url throws an unhandled UriFormatException
CLI-005	CLI	Malformed --bindings / --overrides JSON throws unhandled exceptions
CLI-006	CLI	Password is passed as a command-line argument with no safer alternative
CLI-007	CLI	Component-CLI.md command surface is substantially stale
CentralUI-005	CentralUI	Session expiry implementation diverges from the documented policy
CentralUI-006	CentralUI	Deployment status page polls every 10s despite the documented SignalR-push design
CentralUI-007	CentralUI	Monitoring nav links to Deployment-only pages are shown to all roles
CentralUI-008	CentralUI	Audit-log date filters treat browser-local datetimes as UTC
CentralUI-009	CentralUI	DebugView stream callbacks touch a possibly-disposed ToastNotification
CentralUI-010	CentralUI	ToastNotification auto-dismiss continuation runs after component disposal
CentralUI-011	CentralUI	DiffDialog leaves a dangling TaskCompletionSource when disposed while open
CentralUI-012	CentralUI	N+1 query loading data connections for the Sites page
CentralUI-013	CentralUI	ScriptAnalysisService blocks on async shared-script lookups
CentralUI-014	CentralUI	Test Run side effects (HTTP/SQL/SMTP) fire against production services
ClusterInfrastructure-002	ClusterInfrastructure	No-op DI extension methods report success while doing nothing
ClusterInfrastructure-003	ClusterInfrastructure	ClusterOptions omits several documented node-configuration settings
ClusterInfrastructure-004	ClusterInfrastructure	ClusterOptions has no validation despite safety-critical values
ClusterInfrastructure-006	ClusterInfrastructure	No tests for any cluster behaviour; only the options POCO is covered
Commons-001	Commons	StaleTagMonitor stale-fire race between timer and OnValueReceived
Commons-002	Commons	DynamicJsonElement retains a JsonElement whose JsonDocument lifetime it does not own
Commons-003	Commons	ScriptParameters.GetNullable silently swallows conversion failures
Commons-004	Commons	ManagementCommandRegistry name mapping is asymmetric and namespace-scoped
Communication-004	Communication	Coordinator actors declare no SupervisorStrategy (design requires Resume)
Communication-005	Communication	gRPC keepalive and max-stream-lifetime options are defined but never applied
Communication-006	Communication	Site address load failures are silently swallowed, leaving a stale cache
Communication-007	Communication	SiteStreamGrpcClientFactory.Dispose blocks on async work (sync-over-async)
Communication-008	Communication	Reconnect retry-count reset can mask a flapping stream indefinitely
ConfigurationDatabase-002	ConfigurationDatabase	Hardcoded sa connection string with embedded password literal
ConfigurationDatabase-003	ConfigurationDatabase	No-arg AddConfigurationDatabase() silently registers nothing
ConfigurationDatabase-004	ConfigurationDatabase	Secret-bearing columns stored in plaintext with no protection
ConfigurationDatabase-007	ConfigurationDatabase	AuditService does not handle JSON-serialization failure of arbitrary afterState
DataConnectionLayer-006	DataConnectionLayer	Health quality counters not reset/recomputed after failover or re-subscribe
DataConnectionLayer-007	DataConnectionLayer	ReadBatchAsync aborts the whole batch on the first failing tag
DataConnectionLayer-009	DataConnectionLayer	Implemented failover heuristic diverges from the documented state machine
DataConnectionLayer-010	DataConnectionLayer	Tag-resolution retry can issue duplicate concurrent subscribe attempts
DataConnectionLayer-011	DataConnectionLayer	Stale subscription callbacks from disposed adapters can still reach the actor
DataConnectionLayer-012	DataConnectionLayer	AutoAcceptUntrustedCerts defaults to true, accepting any server certificate
DeploymentManager-003	DeploymentManager	Successful-deployment cleanup is not atomic with the status write
DeploymentManager-004	DeploymentManager	Site-success but central-delete-failure leaves orphaned site config
DeploymentManager-005	DeploymentManager	OperationLockManager leaks a SemaphoreSlim per instance name
DeploymentManager-007	DeploymentManager	"Diff View" reduced to a hash comparison with no diff detail
DeploymentManager-008	DeploymentManager	DeploymentManagerOptions is never bound to configuration
DeploymentManager-011	DeploymentManager	Tests never exercise a successful deployment or lifecycle success path
ExternalSystemGateway-004	ExternalSystemGateway	System retry settings are not honoured for cached calls/writes
ExternalSystemGateway-005	ExternalSystemGateway	HttpRequestMessage and HttpResponseMessage are not disposed
ExternalSystemGateway-006	ExternalSystemGateway	BuildUrl ignores path templates and appends a trailing slash for empty paths
ExternalSystemGateway-007	ExternalSystemGateway	External error response bodies are echoed verbatim into script-visible error messages
ExternalSystemGateway-008	ExternalSystemGateway	Cancellation is conflated with transient timeout failure
ExternalSystemGateway-009	ExternalSystemGateway	StoreAndForwardResult from EnqueueAsync is discarded; permanent failures during buffering are swallowed
ExternalSystemGateway-010	ExternalSystemGateway	GetConnectionAsync leaks the SqlConnection when OpenAsync fails
HealthMonitoring-003	HealthMonitoring	Shared state mutated inside ConcurrentDictionary.AddOrUpdate update delegate
HealthMonitoring-005	HealthMonitoring	Central self-report site can flap offline; no heartbeat grace like real sites
HealthMonitoring-007	HealthMonitoring	Heartbeats for not-yet-registered sites are silently dropped
HealthMonitoring-008	HealthMonitoring	GetAllSiteStates / GetSiteState leak live mutable state objects to callers
HealthMonitoring-009	HealthMonitoring	Missing test coverage for central report loop, heartbeat path, replication, and collector setters
Host-002	Host	Akka.Persistence required by REQ-HOST-6 is not configured and not used
Host-003	Host	Secrets committed in plaintext in appsettings.Central.json
Host-004	Host	Site seed-node list points at the gRPC port, not a remoting port
InboundAPI-002	InboundAPI	Lazy compilation is a check-then-act race with no atomicity
InboundAPI-004	InboundAPI	Client disconnect is misreported as a script timeout
InboundAPI-006	InboundAPI	No request body size limit on the inbound endpoint
InboundAPI-007	InboundAPI	Database.Connection() script API from the design doc is not implemented
InboundAPI-008	InboundAPI	Inbound API endpoint not restricted to the active central node
ManagementService-004	ManagementService	Actor offloads work to Task.Run instead of using PipeTo
ManagementService-006	ManagementService	JsonDocument instances never disposed in the HTTP endpoint
ManagementService-007	ManagementService	Inconsistent and cycle-prone serialization of repository entities
ManagementService-009	ManagementService	Audit logging applied inconsistently across mutating handlers
ManagementService-013	ManagementService	No tests for site-scope enforcement, the HTTP endpoint, or DebugStreamHub
NotificationService-005	NotificationService	Non-TLS path uses SecureSocketOptions.Auto, contradicting the requested mode
NotificationService-006	NotificationService	OAuth2 token cache is keyed to nothing; wrong token returned when multiple SMTP configs exist
NotificationService-007	NotificationService	Connection timeout and max-concurrent-connections from the design doc are not implemented
NotificationService-008	NotificationService	Recipient email addresses are not validated before send
NotificationService-009	NotificationService	Credentials handled as plaintext strings; OAuth2 client secret logged risk
Security-004	Security	Search filter uses uid= while fallback DN construction uses cn=
Security-005	Security	DN injection in the no-service-account bind fallback
Security-006	Security	JWT validation disables issuer and audience checks
Security-007	Security	Idle-timeout claim is reset on every token refresh
SiteEventLogging-005	SiteEventLogging	LogEventAsync performs synchronous disk I/O on the caller's thread
SiteEventLogging-007	SiteEventLogging	ISiteEventLogger consumers downcast to the concrete type and reach into the DB connection
SiteEventLogging-008	SiteEventLogging	Event-recording write failures are silently swallowed
SiteEventLogging-010	SiteEventLogging	Test coverage gaps: actor bridge, purge/write concurrency, vacuum effectiveness, query error path
SiteRuntime-004	SiteRuntime	_totalDeployedCount is incremented on redeployment of an existing instance
SiteRuntime-005	SiteRuntime	Deployment reports Success to central before persistence completes
SiteRuntime-006	SiteRuntime	Site-local repositories read SiteStorageService private field via reflection
SiteRuntime-007	SiteRuntime	Synthetic entity IDs use the non-deterministic string.GetHashCode()
SiteRuntime-008	SiteRuntime	Blocking .GetAwaiter().GetResult() on the actor thread during startup
SiteRuntime-009	SiteRuntime	Script execution actors run scripts on the default thread pool, not a dedicated dispatcher
SiteRuntime-010	SiteRuntime	EnsureDclConnections never updates a connection whose configuration changed
SiteRuntime-011	SiteRuntime	Trust-model validation is a substring scan and is both over- and under-inclusive
StoreAndForward-004	StoreAndForward	RegisterDeliveryHandler XML doc contradicts the implemented contract
StoreAndForward-005	StoreAndForward	Parked-message retry/discard can race with the in-progress retry sweep
StoreAndForward-010	StoreAndForward	Retry of a parked message does not reset LastAttemptAt, so its retry timing is unspecified
StoreAndForward-013	StoreAndForward	Critical paths lack test coverage: retry-due timing, replication-from-active, and the actor bridge
TemplateEngine-006	TemplateEngine	Forbidden-API enforcement is a naive substring scan (bypassable and false-positive prone)
TemplateEngine-007	TemplateEngine	Brace-balance "compilation" misjudges verbatim / interpolated / raw strings
TemplateEngine-008	TemplateEngine	SetAlarmOverrideAsync accepts overrides for unknown / composed alarms with no validation
TemplateEngine-009	TemplateEngine	N+1 query in TemplateDeletionService.CanDeleteTemplateAsync
TemplateEngine-010	TemplateEngine	InstanceService documents optimistic concurrency that is not implemented

Low (90)

ID	Module	Title
CLI-008	CLI	--format value is not validated
CLI-009	CLI	Exit-code documentation does not match HandleResponse behaviour
CLI-010	CLI	debug stream reports Ctrl+C during connect as a connection failure
CLI-011	CLI	CancellationTokenSource in debug stream is never disposed
CLI-012	CLI	debug stream exit code is unreliable after stream termination
CLI-013	CLI	HTTP client, debug stream, and JSON-argument parsing are untested
CentralUI-015	CentralUI	DialogService continuations resolve off the render thread
CentralUI-016	CentralUI	Pagers render one button per page with no windowing
CentralUI-017	CentralUI	/auth/logout POST disables antiforgery, enabling logout CSRF
CentralUI-018	CentralUI	Broad catch {} blocks swallow JS interop and storage errors silently
CentralUI-019	CentralUI	Sparse unit-test coverage for a large module; critical paths untested
ClusterInfrastructure-005	ClusterInfrastructure	No configuration section name constant for the Options pattern binding
ClusterInfrastructure-007	ClusterInfrastructure	ClusterOptions lacks XML documentation comments
ClusterInfrastructure-008	ClusterInfrastructure	"Phase 0 skeleton" status is undocumented at the module level
Commons-005	Commons	OpcUaEndpointConfigSerializer.Deserialize discards malformed legacy input and over-reports IsLegacy
Commons-006	Commons	DynamicJsonElement.TryConvert reports success for unconvertible target types
Commons-007	Commons	Several Commons types carry non-trivial logic, stretching REQ-COM-6
Commons-008	Commons	SetConnectionBindingsCommand uses ValueTuple in a wire message contract
Commons-009	Commons	Component-Commons.md is stale relative to the actual file set
Commons-010	Commons	Behavior-bearing Commons types have no unit tests
Commons-011	Commons	Result<T>.Failure accepts a null error string
Commons-012	Commons	ValueFormatter uses current-culture formatting without documenting it
Communication-009	Communication	_siteClients field is mutable and reassignable; cache update is not atomic on failure
Communication-010	Communication	DebugStreamBridgeActor XML doc incorrectly describes it as a "Persistent actor"
Communication-011	Communication	No test coverage for snapshot-timeout cleanup, address-cache failure, or gRPC reconnect leak
ConfigurationDatabase-005	ConfigurationDatabase	Audit Id type disagrees with the design doc
ConfigurationDatabase-006	ConfigurationDatabase	Site.GrpcNodeAAddress / GrpcNodeBAddress columns are unbounded
ConfigurationDatabase-008	ConfigurationDatabase	GetApprovedKeysForMethodAsync CSV parsing silently drops malformed ids
ConfigurationDatabase-009	ConfigurationDatabase	Multi-collection eager loads issue cartesian-product queries
ConfigurationDatabase-010	ConfigurationDatabase	Several repositories and InstanceLocator lack direct test coverage
ConfigurationDatabase-011	ConfigurationDatabase	Inconsistent constructor null-guarding across repositories/services
DataConnectionLayer-008	DataConnectionLayer	HandleUnsubscribe is O(n^2) over instances and rechecks _unresolvedTags redundantly
DataConnectionLayer-013	DataConnectionLayer	Misleading XML comment: RaiseDisconnected claims thread safety it does not provide
DeploymentManager-009	DeploymentManager	Misleading timeout comment on DeleteInstanceAsync
DeploymentManager-010	DeploymentManager	SystemArtifactDeploymentRecord does not persist the deployment ID
DeploymentManager-012	DeploymentManager	LifecycleCommandTimeout option is dead code
DeploymentManager-013	DeploymentManager	SMTP credentials serialized and broadcast to all sites
DeploymentManager-014	DeploymentManager	Dead CreateCommand helper in artifact tests
ExternalSystemGateway-011	ExternalSystemGateway	Every call performs a full repository scan of all systems and methods
ExternalSystemGateway-012	ExternalSystemGateway	Permanent-failure logging requirement is not met; _logger is injected but unused
ExternalSystemGateway-013	ExternalSystemGateway	MaxConcurrentConnectionsPerSystem and DefaultHttpTimeout options are defined but never used
ExternalSystemGateway-014	ExternalSystemGateway	Cached-call buffering path and DatabaseGateway are untested
HealthMonitoring-004	HealthMonitoring	Inconsistent heartbeat interval described across XML docs
HealthMonitoring-006	HealthMonitoring	Sequence seeding contradicts the doc's "starting at 1" wording and is untestable
HealthMonitoring-010	HealthMonitoring	HealthReportSender silently swallows inner failures with bare catch {}
HealthMonitoring-011	HealthMonitoring	AddHealthMonitoringActors is a dead no-op placeholder
HealthMonitoring-012	HealthMonitoring	SiteHealthState.LatestReport initialized to null!, misrepresenting the contract
Host-005	Host	Blocking sync-over-async (GetAwaiter().GetResult()) inside StartAsync
Host-006	Host	HOCON assembled by unescaped string interpolation
Host-007	Host	REQ-HOST-4 rule "GrpcPort ≠ RemotingPort" is not enforced
Host-008	Host	MachineDataDb is validated and declared but never consumed
Host-009	Host	StartAsync reports success before role actors are confirmed running
Host-010	Host	No retry/backoff around startup preconditions (DB migration, readiness)
Host-011	Host	LoggingOptions.MinimumLevel is dead configuration
InboundAPI-009	InboundAPI	Failed compilation is retried on every subsequent request
InboundAPI-010	InboundAPI	ParameterValidator ignores extra body fields and cannot validate Object/List element types
InboundAPI-011	InboundAPI	Method-existence check leaks to unapproved callers (enumeration oracle)
InboundAPI-012	InboundAPI	ParameterDefinition POCO declared in the component project, not Commons
InboundAPI-013	InboundAPI	ApiKeyValidationResult.NotFound factory returns HTTP 400, contradicting its name
ManagementService-005	ManagementService	ManagementActor declares no supervision strategy
ManagementService-008	ManagementService	HandleResolveRoles constructs RoleMapper manually instead of via DI
ManagementService-010	ManagementService	ManagementServiceOptions.CommandTimeout is defined but never used
ManagementService-011	ManagementService	ResolveRolesCommand dispatch path is stale dead code
ManagementService-012	ManagementService	ManagementEnvelope carries a loosely-typed object payload
NotificationService-010	NotificationService	DeliverAsync does not disconnect the SMTP client on failure
NotificationService-011	NotificationService	SmtpPermanentException declared in the wrong file; module conventions
NotificationService-012	NotificationService	Test coverage gaps: OAuth2 delivery path, permanent-classification fallback, token-cache concurrency
Security-008	Security	N+1 query loading site-scope rules in RoleMapper
Security-009	Security	CancellationToken not honored inside Task.Run LDAP calls
Security-010	Security	Design doc contradicts itself on Windows Integrated Authentication
Security-011	Security	Missing tests for security-critical paths
SiteEventLogging-006	SiteEventLogging	Missing indexes for severity and keyword-search query paths
SiteEventLogging-009	SiteEventLogging	XML doc on LogEventAsync claims asynchronous behaviour
SiteEventLogging-011	SiteEventLogging	Stale "Phase 4+" placeholder in ServiceCollectionExtensions
SiteRuntime-012	SiteRuntime	AttributeAccessor/ScopeAccessors block the script on a synchronous Ask
SiteRuntime-013	SiteRuntime	HandleUnsubscribeDebugView does nothing despite documented behaviour
SiteRuntime-014	SiteRuntime	Trigger-expression evaluation blocks the coordinator actor thread
SiteRuntime-015	SiteRuntime	LoggerFactory created per Instance Actor and never disposed
SiteRuntime-016	SiteRuntime	Short-lived execution actors, replication actor, and repositories are untested
StoreAndForward-002	StoreAndForward	Messages enqueued with no registered handler are buffered but never deliverable
StoreAndForward-006	StoreAndForward	GetParkedMessagesAsync count and page run without a transaction
StoreAndForward-007	StoreAndForward	Async work in ParkedMessageHandlerActor uses ContinueWith without scheduler/affinity guarantees
StoreAndForward-008	StoreAndForward	A SQLite connection is opened and torn down on every storage call
StoreAndForward-009	StoreAndForward	OnActivity event invocation is not thread-safe against concurrent subscribe/unsubscribe
StoreAndForward-011	StoreAndForward	StoreAndForwardMessageStatus.InFlight is unused and the doc's "retrying" status is unmodelled
StoreAndForward-012	StoreAndForward	StoreAndForwardMessage is a persistence entity but lives in the component, not Commons
TemplateEngine-011	TemplateEngine	SortedPropertiesConverterFactory is dead code with a misleading comment
TemplateEngine-012	TemplateEngine	DataType enum naming diverges from the design doc
TemplateEngine-013	TemplateEngine	ToDictionary(t => t.Id) throws on duplicate IDs; cycle detectors overload Id 0 as a sentinel
TemplateEngine-014	TemplateEngine	Template-deletion constraint logic is duplicated and divergent