Second slice of the 2026-09-09 whole-system review remediation
(docs/plans/2026-09-09-arch-review.md §10), on top of the High slice 146a1323.
Runtime/devices: SR-2 alarm on-trigger integration scope; SR-3 correlated NativeAlarmActor
subscribe retry; SR-4 immutable audited DB target; SR-5 ordered InstanceActor stop/flush;
SR-12 rehydration fence; SR-13 S&F gate recheck per delivery; DC-4 no OPC UA security
downgrade; DC-5 alarm snapshot for every new subscriber; ORCH-1 sync-dispose joins the pump.
Management/UI: MG-2 site scope on audit tree + Flow Debugger Replay; MG-3 stale-probe memo
removed; MG-4 server-owned bulk-deploy deadline; UI-5 sandbox runs in a killable child
process (new CentralUI.SandboxWorker project); UI-14 bounded sandbox output/IPC.
Edge/audit: EI-1 relay audit ownership; EI-2 SkipBodyCapture suppresses error text; EI-3/EI-11
error-field redaction + caps; EI-4 single relay deadline, pre-execution-only retry; EI-5
authenticated relay provenance; EI-10 body cap before audit read.
Persistence/contracts: DL-1 global Instance.UniqueName unique index + conflict translation;
DL-2 DataConnection protocol config encrypted at rest (migration
20260909203046_EnforceGlobalInstanceNamesAndProtectProtocolConfiguration — coordinated
central upgrade, old binaries cannot read protected rows); DL-3 bounded Replay paging;
DL-6 cross-site Overwrite of a deployed instance rejected; XC-4 proto descriptor contract test.
Config/hygiene: XC-1 central listener isolation by local port; XC-2/XC-3 inert SMTP + cron
keys removed from rig settings; CM-2 env2 bootstrap guard enabled; DC-6 deploy site exit code;
DC-7 notifier retries only explicit standby rejection; XC-5 ClusterClient purged from docs.
Gates recorded in the report: build 0/0, 9 397 passed / 0 failed, rig redeployed on all
eight nodes with the migration applied. 46 Low findings remain open.
TimeoutSeconds is the per-system HTTP timeout ExternalSystemClient reads at
call time and is deployed to sites (proto field 6), but Transport bundles
silently dropped it: the DTO never carried it, export never wrote it, the
importer's Add path reset it to 0, the Overwrite path left the target's
stale value untouched, and ArtifactDiff never compared it (so a bundle
whose only change was a tuned timeout previewed as Identical).
Adds TimeoutSeconds as an additive-optional trailing field on
ExternalSystemDto (schemaVersion stays 1.1, bundleFormatVersion unchanged)
and threads it through EntitySerializer (export + import), BundleImporter
(Add and Overwrite), and ArtifactDiff.CompareExternalSystem. Old-bundle
semantics pinned: an absent field (null) falls back to the entity default
on Add and leaves the target's existing value untouched on Overwrite/diff.
Beefed up the RoundTripEquivalenceTests external-system fixture with a
non-default TimeoutSeconds so the reflection round-trip guard actually
exercises the field instead of comparing default against default.
Swept the rest of the ExternalSystemDto shape against
ExternalSystemDefinition field-by-field; no other operative field is
missing.
Row 65 — trigger FIRED, fixed. BundleUnlockAttempt rows were pruned only by
nodes actively serving unlocks, so a cluster that stopped importing entirely
left its last rows in place forever. The row's own trigger was "Transport gains
a central scheduler a prune tick could ride" — it has one now:
BundleSessionEvictionService, the central-only 1-minute session sweep added by
the transport-session fixes. It now also ticks the new
IBundleUnlockAttemptWindow.PruneAsync. Both drivers share the ONE 15-minute CAS
cadence claim, so ticking on a 1-minute sweep costs at most one bounded DELETE
per quarter hour and the two can never prune concurrently. PruneAsync is
contractually non-throwing (unlike TryRegisterAttemptAsync, which throws and
lets the limiter own the fail-open decision) — a background sweep has no caller
to report to and must survive a database blip. Both Transport docs updated;
three tests in FleetWideUnlockThrottleTests, incl. the retention sweep running
with no unlock traffic anywhere in the test, which is the whole point.
Row 58 — closed with a pin, and the OLD pin was found blind. The existing
Deployments_Dispose_UnsubscribesFromNotifier is behavioural: it is satisfied by
the _disposed guard alone, so deleting the `-=` from Dispose leaves it green
while the singleton notifier keeps a delegate pinning the circuit-scoped
component. Verified by mutation — with the `-=` removed the old test still
passed and only the two new pins failed. Added:
* a structural retention pin asserting the notifier's invocation list actually
shrinks to 0 on Dispose;
* a source scan pinning the row's stated TRIGGER ("a second page subscribes,
or any subscriber lands without a matching Dispose unsubscribe") — every
CentralUI file that subscribes must also unsubscribe and be IDisposable,
with a non-empty guard so the scan cannot pass vacuously.
Row 60 — closed with a pin, and the register's premise corrected.
GlobalArtifactSnapshot does NOT hold EF-materialised entities: it is projected
into pure Commons DTOs before the fan-out. The EF state that genuinely crosses
into the parallel send is the tracked Site entities the lambda closes over, and
that is safe only while lazy loading is off. Pinned in ConfigurationDatabase
.Tests two ways — no proxies extension on the built DbContextOptions, and no
EntityFrameworkCore.Proxies package reference anywhere in the repo (the
structurally stronger half: UseLazyLoadingProxies() is not a compilable call
without it). The real invariant is now spelled out at the fan-out itself.
Row 59 — no product change; added the missing tripwire remark stating the
constraint the safety argument rests on (serial by design, one scoped DbContext,
do not Task.WhenAll; if it gets slow the fix is a scope PER SITE, not a shared
scope under fan-out). Line numbers in the register have drifted to :528-545.
Rows 36, 37, 64: verification only, no change — see the sweep report.
An external system's MaxRetries/RetryDelay existed on the central entity but never
reached a site and had no management surface, so every ExternalSystem.CachedCall
buffered with the S&F engine default (DefaultMaxRetries 50 x DefaultRetryInterval
30s ~= 25 min to park) whatever central had configured. The sibling
DatabaseConnectionArtifact had always carried its equivalents.
The consumption end was already wired: ExternalSystemClient.CachedCallAsync already
read system.MaxRetries/RetryDelay and passed them to EnqueueAsync with the 0-as-unset
convention. Only the transport and persistence were missing.
Commons
- ExternalSystemArtifact gains MaxRetries/RetryDelay as optional positional params
(0 / TimeSpan.Zero = unset), matching DatabaseConnectionArtifact rather than going
nullable, so it maps 1:1 onto the entity and the proto3 scalars.
- Create/UpdateExternalSystemCommand gain nullable MaxRetries/RetryDelay, following
the existing TimeoutSeconds "omit to preserve" convention. Both additive.
Central
- ManagementActor create maps both; update preserves-if-null.
- ArtifactDeploymentService projects them onto the artifact.
Wire
- site_command.proto ExternalSystemArtifactDto gains max_retries=7 /
retry_delay=8 (google.protobuf.Duration), mirroring DatabaseConnectionArtifactDto.
Additive: a pre-existing site decodes them as 0/absent, which IS the unset value,
so old behaviour is preserved. Vendored C# regenerated via docker/regen-proto.sh
sitecommand; csproj Protobuf item re-commented, no message types added or removed.
Site
- external_systems gains max_retries / retry_delay_ms via AddColumnIfMissing,
DEFAULT 0 = unset. Storage upsert, repository SELECT/mapping (new ordinals 6/7
appended so existing ones are untouched), apply path, and the legacy migrator's
explicit column list all carry them.
Replicated-table decision: external_systems IS in SiteLocalDbSetup.ReplicatedTables,
so this was checked against the AckTime/native_alarm_state precedent. Real columns
are correct here, not a JSON bag: the table has no general-purpose config column
(auth_configuration and method_definitions are both semantically specific),
timeout_seconds already set the additive-column precedent on this exact table, and
sibling database_connections stores the same two values as plain columns. Verified in
LocalDb source that RegisterReplicated re-reads PRAGMA table_info every boot and its
install script is DROP-then-CREATE, so capture triggers rebuild over the new column
set automatically, and SiteStorageSchema.Apply runs before registration on the same
boot. The only cost is the ReplicatedTable.ComputeDigest change, which the sync
handshake compares fail-closed - a mixed-version pair refuses to sync loudly instead
of diverging silently. Both-nodes-together is already the standing site-pair rule, so
no new operational constraint; recorded in topology-guide.md.
Also fixed: ArtifactDiff.CompareExternalSystem never compared MaxRetries/RetryDelay,
so a bundle import that changed a system's retry budget previewed as "Identical" -
harmless while the values were inert, a live behaviour change now.
No EF migration: the central ExternalSystemDefinitions columns already exist
(InitialSchema). Central UI already had both form fields.
Tests: site storage round-trip + unset-default, site apply path, central artifact
projection, proto round-trip (via the shared sample), management create/preserve,
CLI option parse + alias, and diff reporting. Fail-before verified on the two core
tests (expected 7 got 0 on the read path; expected 9 got 0 on the apply path).
Docs: Component-ExternalSystemGateway, Component-StoreAndForward, Component-CLI,
components/StoreAndForward, components/SiteRuntime, deployment/topology-guide,
CLI README (which was also missing --timeout-seconds entirely).
Row 53 shipped the scadabridge.central.transport.import_sessions.open gauge
and a Warning log on a cap hit, but a log line is not something an operator
can alert on. Adds the companion counter
scadabridge.central.transport.import_sessions.rejected
on the existing ZB.MOM.WW.ScadaBridge meter, so no ObservedMeters allowlist
change is needed (an unlisted meter exports nothing, silently).
The emit point is deliberately narrow. BundleSessionStore.Open tests the cap
TWICE — once on entry and again after EvictExpired() — and only the second
test is a rejection: a store full of merely expired sessions reclaims a slot
and admits the caller. Counting the first would make the series fire
routinely on any node whose sweep had not yet run, turning a page-worthy
signal into background noise. Re-Opening an already-open session id skips
the cap block entirely and is likewise not counted.
The two instruments are diagnostic as a pair: rejections rising while the
gauge oscillates below the cap is genuine concurrency (raise the cap);
rejections rising while the gauge sits pinned at the cap is a session leak
(register row 43's signature), where raising the cap only defers the problem.
Tests assert a delta over a scoped action rather than an absolute value, so
they do not inherit the register row 32 static-state flake that makes
QueueDepthGaugeTests fail only under the full concurrent suite. Negative
controls cover under-cap, expired-reclaim, and duplicate-session-id.
Tests: 11 passed in BundleSessionObservabilityTests (6 existing + 5 new).
Superseded by BundleUnlockThrottledException when register row 44 wired the
limiter live and moved the typed exception to Commons so ManagementService's
MapFault could curate it. The original type has been unreferenced since —
never thrown, never caught, only its own definition — and with row 51 moving
BundleLockedException into the same Commons namespace there are now three
similarly-named unlock exceptions, one of which does nothing. A future
maintainer catching the dead one would silently get no coverage.
Solution builds clean with it gone (zero references). Transport 173 passed.
The per-IP unlock throttle's counters were a process-local ConcurrentDictionary
on each central node, so the effective fleet cap was N x
MaxUnlockAttemptsPerIpPerHour per client (failing over to the other node handed
the client a fresh budget) and a node restart cleared the window outright.
The window now lives in the central MS SQL BundleUnlockAttempt table — the HA
store both central nodes already share, so no new infrastructure, no new port,
no consensus protocol:
- Commons: BundleUnlockAttempt entity + IBundleUnlockAttemptRepository.
- ConfigurationDatabase: entity config (one index,
IX_BundleUnlockAttempt_Client_Attempted, serving both the windowed count and
the prune), repository (AsNoTracking count, ExecuteDeleteAsync prune),
DbSet, DI registration, migration AddBundleUnlockAttemptTable + the
hand-written idempotent production script under docs/plans/sql/.
- Transport: IBundleUnlockAttemptWindow seam + SqlBundleUnlockAttemptWindow
(singleton; resolves the scoped repository from its own scope per call, like
the central purge singletons). BundleUnlockRateLimiter keeps the policy and
gains TryRegisterAttemptAsync; BundleImporter.LoadAsync calls it.
Behaviour preserved from row 44: locked bundles spend no hourly budget (the
lockout check still runs first), throttle denials burn no per-bundle strike and
are NOT recorded so they cannot roll the window forward, unattributable clients
share the stricter <unattributed> bucket, and the budget counts attempts rather
than failures. Config keys and their semantics are unchanged, so the rig's 500
override stays valid.
Three deliberate trades, documented in both Transport docs:
- FAILS OPEN. A store error logs a Warning naming the client and cap, and
permits the attempt. This is a best-effort rate cap over an already
authenticated, Administrator-gated surface — the availability of legitimate
imports outranks the cap, and a DB blip must not make the import wizard
unusable. Falling back to the process-local window was considered and
rejected: it would still block a legitimate operator's 11th import.
- Approximate. Count-then-insert is not atomic across nodes, so simultaneous
attempts can overshoot by the number in flight — bounded, self-correcting
within the window, and not worth provider-specific raw SQL.
- Pruned opportunistically on the unlock path (CAS-claimed at most every 15
min per process, bounded to 1000 rows, retaining 2x the window, best-effort
try/catch) rather than by a new background service — Transport owns no
central scheduler. Same shape as DeploymentService's terminal-record sweep.
Tests: Transport.IntegrationTests 116 passed (+10 — two limiter instances over
one store enforcing a single combined window, restart durability, denials
burning no budget, sliding release, bucket isolation, store-failure fail-open,
caller-cancellation not swallowed, no-shared-window fallback, prune and its
negative control); ConfigurationDatabase 395 passed (+5).
Nothing counted open import sessions and nothing logged a rejection at
MaxConcurrentImportSessions (default 8), so the register-row-43 session leak
could only be inferred from rotating test failures.
- Gauge scadabridge.central.transport.import_sessions.open on the existing
ZB.MOM.WW.ScadaBridge meter — already in SiteServiceRegistration.ObservedMeters
(the ZbTelemetryOptions.Meters allowlist), so no telemetry config change is
needed and the series is not silently dropped. Provider-backed, mirroring the
StoreAndForward queue-depth gauge: registered by BundleSessionEvictionService
.StartAsync (Transport's only lifecycle seam) and identity-checked-cleared on
StopAsync so a late stop cannot clobber a newer registrant.
- IBundleSessionStore.OpenSessionCount counts only UNEXPIRED sessions. The raw
dictionary count would report the node as pinned at the cap for up to a
one-minute sweep interval after a burst had drained — the exact false positive
the instrument exists to rule out.
- BundleSessionStore.Open logs Warning with open count, cap and TTL when it
rejects. Fires only on a real cap hit: a store full of expired sessions is
reclaimed first and logs nothing.
- Docs: docs/components/Transport.md + docs/requirements/Component-Transport.md.
Tests: Transport 173 passed (+6 new — Warning content, two negative controls,
unexpired-only counting, gauge register/clear, no-clobber-on-late-stop).
ManagementService references Commons but not Transport, so MapFault could not
see BundleLockedException and the per-bundle 3-strike lockout reached the CLI
as "An internal error occurred (CorrelationId=...)" — the operator got a
correlation id instead of the reason, and no hint that further attempts on
that bundle would be refused.
Applies the pattern register row 44 established for BundleUnlockThrottledException:
- Relocate BundleLockedException from Transport/Import/ to
Commons/Types/Transport/, beside BundleUnlockThrottledException. Both
consumers (BundleImporter, CentralUI TransportImport) already carried the
Commons.Types.Transport using, so no call-site edits were needed.
- Add curated catch arms in ManagementActor.HandlePreviewBundle and
HandleImportBundle that rethrow as ManagementCommandException, so the
message survives MapFault's disclosure filter.
- Mirror the row-44 throttle fault-mapping test for both handlers.
- Document the relocation in docs/components/Transport.md and
docs/requirements/Component-Transport.md.
Tests: ManagementService 499 passed (+2 new), Transport 167 passed.
BundleUnlockRateLimiter was registered (ServiceCollectionExtensions), injected
into BundleImporter, unit-tested, documented on
TransportOptions.MaxUnlockAttemptsPerIpPerHour = 10 and pinned in
appsettings.Central.json — but _unlockRateLimiter was never invoked. A
brute-force passphrase attacker against a stolen bundle was bounded only by the
per-bundle 3-strike lockout, which a re-export side-steps (new ContentHash).
LoadAsync now registers every unlock attempt against the limiter. Ordering is
load-bearing: AFTER the per-bundle lockout check (hammering a locked bundle
spends no hourly budget) and BEFORE the decrypt (so a denial burns no per-bundle
strike — otherwise the throttle would be a way to lock a bundle someone is
legitimately importing). Over budget throws the new
BundleUnlockThrottledException, fail-closed with a clear message on both
surfaces; it lives in Commons because the management surface (Commons-only
references) must catch it to return a curated error rather than MapFault's
generic internal-error text.
Client key plumbing:
- Central UI: new scoped CircuitClientAddressProvider snapshots
Connection.RemoteIpAddress at circuit establishment — the same constraint and
the same pattern as CookieAuthenticationStateProvider, since HttpContext is
null by the time a circuit submits a passphrase.
- CLI/management: additive ManagementEnvelope.ClientIp, stamped by
ManagementEndpoints and threaded through DispatchCommand to the preview/import
bundle handlers.
- No attributable address => a single SHARED "<unattributed>" bucket, not a
bypass: unknown clients get a stricter budget, never none. Per-client
isolation is only real behind a ForwardedHeaders-trusted proxy, the same
caveat the login throttle carries (Component-Security.md).
Docs: Component-Transport.md + docs/components/Transport.md record the two-level
ordering, the client-key sourcing, the shared-bucket fallback and a
troubleshooting entry for the new error.
Tests: 6 importer tests (cap enforced; denial != passphrase failure, no strike
burned; per-client isolation; shared unattributed bucket; unencrypted bundles
consume no budget; a locked bundle spends no hourly budget) — 4 of the 6 fail
with the limiter call disabled, i.e. against the dead-code state. Plus
ManagementActor tests pinning the ClientIp forwarding and the curated
throttle error.
Only BundleImporter.ApplyAsync released an import session, so every path that
opened one and did NOT apply held one of MaxConcurrentImportSessions (8) — and
the DECRYPTED bundle content that T-007 deliberately zeroes on the apply paths —
for the full BundleSessionTtlMinutes (30). Eight abandonments locked out all
imports on the active node; observed live (R6: 3 back-to-back suite runs held 6
of 8 slots until central-b was restarted).
New IBundleImporter.Abandon(sessionId): the same zero-then-remove release
ApplyAsync performs, idempotent (double release / unknown id / expired id are
no-ops) and only ever given the random-GUID id its caller opened, so it can
never evict another circuit's session.
Wired into every abandonment path:
- Central UI wizard — ReleaseSession() from new upload, TryLoadAsync (before it
overwrites _session), Back-to-Upload / ResetSessionState, the 3-strike lockout
reset, and Dispose (navigation away / circuit teardown).
- CLI transport preview — a preview never applies, so nothing released it at
all; now released in a finally.
- CLI transport import — covers the pre-apply aborts (blocker rows, unresolved
site/connection mappings).
Class doc's "an abandoned wizard does not leak" claim is true again (it only
ever covered the staged plaintext temp file). Component-Transport.md +
docs/components/Transport.md now record the explicit-release contract and that
BundleSessionEvictionService is a TTL backstop, not an abandonment path.
Tests: 4 bUnit tests over a REAL BundleSessionStore (dispose / back-to-upload /
apply-then-dispose idempotency / concurrent-circuit isolation) — 3 of the 4 fail
against the pre-fix page; 4 importer tests (release + zeroing, idempotency,
cross-session isolation, and the slot-cap regression).
Six adversarial-review findings, each verified against the code first.
F1 (HIGH) CLI HttpClient capped every call at min(30s, caller timeout),
silently truncating deploy site's 5-minute BulkDeployTimeout and the
5-minute bundle export/preview/import calls — which printed a fake
"504 Request timed out" while the server kept working. HttpClient.Timeout
is now Timeout.InfiniteTimeSpan (the per-call CTS is the single overall
deadline, connect included) with the connect phase bounded separately on
SocketsHttpHandler.ConnectTimeout. The env override is renamed to
SCADABRIDGE_HTTP_CONNECT_TIMEOUT_SECONDS to match its new meaning.
F2 (HIGH) StaleInstanceProbe's process-static memo served stale hashes
because nothing bumped the watermark on three paths:
(a) BundleImporter commits through the raw DbContext, so no import ever
moved the watermark — a second import overwriting the same template
could be OMITTED from ImportResult.StaleInstanceIds. It now bumps
once per apply ATTEMPT: after the commit, and after the rollback too
(the probe runs pre-commit, so a rolled-back attempt leaves memos for
state that never landed; bumping on both paths is the simplest
correct shape, versus threading transaction awareness through a
process-static cache).
(b) CollectWatermarkBumps' default: arm silently no-op'd, contradicting
its own doc. It now sets unattributed=true — an over-broad bump costs
extra work, a missed one produces stale work.
(c) DataConnection edits route through SiteRepository.SaveChangesAsync,
which had no watermark at all, yet Protocol/Primary+Backup config/
FailoverRetryCount are revision-hash inputs. It now bumps (BumpAll —
a connection has no owning template) after a commit that touched one.
F3 (MED) CLI TemplateTableProjection read child ARRAYS, but ListTemplates
now returns database-projected TemplateSummary rows, so template list
printed all zeros. It now prefers the *Count scalars and falls back to
array length (template get still returns full entities). --detail help
text and README corrected: a listing cannot yield definitions, so --detail
renders the raw summary payload and template get --id is the full dump.
F4 (MED) DeploySiteAsync staged every PendingDeployment in phase 1 against
a 5-min TTL while phase 2 reached them one batch at a time, so tail
instances' fetch tokens could expire before their command was sent.
Staging moved into phase 2, immediately before each send; prepare keeps
its flatten/validate/record work. The staging write is the phase's only
repository touch and is serialised behind a 1-permit semaphore, so the
non-thread-safe DbContext constraint holds and the sends stay concurrent.
F5 (MED, latent) DeploySiteAsync leaked every held operation lock if
cancelled — a wedged per-instance semaphore is permanent for the process.
Phase 2 no longer throws (cancellation is recorded as a per-instance
outcome so phase 3 still runs), and an escape from phase 1 or 3 now
unwinds every unfinalised entry: Failed status + lock release.
F6 (LOW) ScriptCompileVerdictCache's promotion wrote hot directly,
bypassing SegmentCapacity (true ceiling 3x against a documented 2x).
Promotion now goes through Store, keeping generational semantics; _hot
and _cold are volatile.
Tests: CLI 396, DeploymentManager 133, ManagementService 494,
TemplateEngine 478, ScriptAnalysis 60, Transport 157, Transport
integration 106, ConfigurationDatabase 366 — all green, 0 build warnings.
The F2/F4/F5 regression tests were each confirmed to FAIL with their fix
reverted.
Caught live 2026-08-01 importing a bundle into the empty env2 cluster: a
create-missing Site has Id == 0 until SaveChanges on a relational provider,
and ApplyDataConnectionsAsync stamps DataConnection.SiteId as a raw scalar
(no navigation, no EF fix-up), so the insert violated
FK_DataConnections_Sites_SiteId. Every importer integration test runs on
the EF in-memory provider, which assigns ids eagerly on AddAsync — the
exact masking the in-code comment predicted. Fix: one SaveChangesAsync
between the site pass and the connection pass, riding the same outer
transaction (all-or-nothing preserved; the failed live import rolled back
cleanly). Regression test runs the create-missing path on SQLite
(CreateMissingSiteRelationalTests) — red without the fix, green with it.
BundleImporter.ApplyAsync opened a user-initiated transaction directly,
but the central ConfigurationDb context is configured with
EnableRetryOnFailure. SqlServerRetryingExecutionStrategy rejects
user-initiated transactions, so every bundle import threw on any real
SQL Server ('...does not support user-initiated transactions.'). The
whole Transport suite ran on the in-memory provider (no retrying
strategy, BeginTransaction is a no-op) so it never surfaced.
Extract the transactional apply into ApplyMergeAsync and drive it via
_dbContext.Database.CreateExecutionStrategy().ExecuteAsync, so the
strategy owns the BeginTransaction -> apply -> Commit unit as one
retriable block. The delegate resets per-attempt state (change tracker
+ ApplyMergeAsync rebuilds its own summary/resolution accumulators) so a
retried attempt cannot double-apply; a rollback failure is captured and
surfaced on the BundleImportFailed audit row exactly as before. Post-
commit side effects (ScriptArtifactsChanged publish, session zero/
remove) moved outside the retriable delegate so they run once.
Regression test: BundleImporterRetryingStrategyTests runs the import on
SQLite with a retrying execution strategy (RetriesOnFailure == true,
arming the same guard as production). It fails with the exact production
error on the pre-fix code and passes after. Existing rollback/apply
contracts unchanged (104 integration + 154 unit tests green).
#05-T24. Three low-severity cleanups, each TDD'd:
- TransportOptions.MaxConcurrentImportSessions (8): BundleSessionStore.Open rejects
a new session past the cap (evicting expired entries first), bounding the N×~200 MB
decrypted-content footprint. Soft cap (count/add not atomic).
- ScriptCompiler.TryCompile now joins ALL forbidden-API violations / compile errors
(string.Join instead of [0]) so an operator sees every problem at once; the T15
verdict cache stores the joined string.
- SemanticValidator emits an advisory warning when a CallScript target resolves
ONLY via the composed leaf-name fallback (dynamic child path, not statically
verified) instead of silently accepting it.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
Addresses the T20 security code-review findings:
- Gate template script + alarm Expression-trigger bodies too (they compile and
execute at the site), not just script bodies — EnumerateTrustGatedScripts now
extracts and vets the {"expression":"..."} C# via the same trust validator.
- Per-origin local-declaration exclusion: a helper declared in a template script
no longer suppresses a genuinely-missing reference of the same name in an
ApiMethod (a global set silently defeated the ApiMethod hard-blocker).
- Fail-closed: a ScriptTrustValidator throw on one pathological body is caught
and surfaced as a hard blocker FOR THAT script (apply) / Blocker row (preview)
instead of aborting the whole import.
- Corrected the misleading attribute-Value comment (typed literal, not a compiled
expression) and documented the Pass-0 fail-fast rationale.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
#05-T23. ArtifactDiff.CompareTemplate gains optional folderNameById /
templateNameById maps; FolderNameOf / BaseTemplateNameOf / CompositionTargetNameOf
resolve the existing template's FolderId / ParentTemplateId / composition
ComposedTemplateId to real names (falling back to the <id:N> placeholder only
when a map is absent or misses, keeping existing unit-test callers valid).
PreviewAsync builds templateNameById from GetAllTemplatesAsync (parent/composition
targets can be any template, not just bundle-named ones) and threads both maps
through. Fixes spurious Modified on every re-import of a foldered/derived/composed
template. End-to-end preview test + two ArtifactDiff unit tests.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
#05-T20. Bundle import is now the fifth ScriptTrustValidator delegating call
site. RunSemanticValidationAsync gains a Pass 0 that runs ScriptTrustValidator
.FindViolations over every non-Skip template / shared / ApiMethod script body
before name resolution; any violation is a HARD error for all kinds (a trust
verdict is authoritative, not the false-positive-prone name heuristic) and
short-circuits so it is never masked by a Pass-1 name error. DetectBlockersAsync
runs the same gate for preview parity, emitting entity-qualified Blocker rows.
Transport gains a direct ScriptAnalysis project reference. Docs: Component-
ScriptAnalysis (fifth call site) + Component-Transport updated.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
#05-T19. Import name-resolution findings are now split by origin: template-script
(and attribute-default-expression) references become non-blocking advisory
warnings (ConflictKind.Warning + ImportResult.Warnings) since the design-time
deploy gate re-validates authoritatively; ApiMethod-script references stay hard
errors (SemanticValidationException / ConflictKind.Blocker) as they have no
downstream gate. Names declared locally in a body (Roslyn-parsed local functions
/ methods) are excluded so a script's own helper isn't flagged; KnownNonReferenceNames
extended with common BCL/LINQ/SQL surface. DetectBlockersAsync and
RunSemanticValidationAsync Pass 1 apply the identical split so preview and apply
agree. Rollback/hard-fail tests retargeted to ApiMethods (the still-hard-failing
vehicle). CentralUI preview renders the new Warning badge/advisory.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
The Task-8 reflection round-trip suite exposed five silent-data-loss paths in
non-template entity types:
- ExternalSystemDefinition.MaxRetries/RetryDelay dropped on import Add (and Overwrite
never updated them) — BuildExternalSystem + Overwrite branch now carry both.
- ExternalSystemMethod rows dropped on import Add — a new post-flush
PersistAddedExternalSystemMethodsAsync pass adds them (the entity has no parent
nav, so an Add can't cascade; idempotent vs the inline Overwrite sync).
- NotificationRecipient dropped on export — GetNotificationListByIdAsync now
Include(Recipients) instead of a bare FindAsync.
- TemplateFolder.ParentFolderId flattened on import Add — a new
ResolveFolderParentEdgesAsync second-pass rewire resolves ParentName→id,
mirroring the template inheritance/composition rewires.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
New Commons seam: ScriptArtifactsChanged record + ScriptArtifactKinds + the
IScriptArtifactChangeBus pub/sub interface. Host ships InProcessScriptArtifactChangeBus
(lock-free copy-on-write, swallow-and-log per subscriber) registered as a central-role
singleton. BundleImporter gains an optional IScriptArtifactChangeBus? and, AFTER
tx.CommitAsync, publishes one notification per script-bearing kind (ApiMethod/
SharedScript/Template) whose resolution was Overwrite/Rename, using post-resolution
names — fully guarded so a bad subscriber can't fail a committed import. Contract +
plan-06 handoff table documented. No consumer yet (additive; publisher is correct).
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
New TemplateChildEquality is the one place "has this template child changed?" is
decided, comparing the COMPLETE writable field set per entity (attributes now incl.
ElementDataType + LockedInDerived; alarms incl. the on-trigger script by name +
LockedInDerived; scripts incl. MinTimeBetweenRuns/ExecutionTimeoutSeconds +
LockedInDerived; native sources per T5). ArtifactDiff.CompareTemplate and all four
BundleImporter.SyncTemplate*Async predicates now route through it — closing the
class of bug where Preview reported Identical for an artifact an Overwrite would
mutate. The attribute sync passes a value-normalised DTO so List-value re-imports
stay idempotent; the alarm on-trigger change now surfaces in both diff and sync.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
New TemplateNativeAlarmSourceDto + init-only TemplateDto.NativeAlarmSources (empty
default, additive — IsInherited placeholders carried for the collision detector).
Exported in ToBundleContent, consumed in FromBundleContent + BuildTemplate, and a
new SyncTemplateNativeAlarmSourcesAsync runs the Overwrite add/update/delete child
sync with per-change audit rows. ArtifactDiff.CompareTemplate now DiffChildren over
native sources so Preview reports a NativeAlarmSources field change. Proven by an
end-to-end flatten test: an instance native-alarm-source override that dangled
pre-fix now resolves against the re-imported template source.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
Adds a trailing optional `bool LockedInDerived = false` to TemplateAttributeDto,
TemplateAlarmDto and TemplateScriptDto (mirrors the ExecutionTimeoutSeconds
additive precedent — no schemaVersion bump). Populated at export
(ToBundleContent), consumed on import (FromBundleContent, BuildTemplate, and all
three SyncTemplate*Async changed-predicate + copy + new-entity paths, incl. audit
payloads). Old-form bundles that lack the property still deserialize to false.
Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
Remove project bookkeeping citations from shipped code comments across the
solution: hyphenated task IDs (WP-14, StoreAndForward-025), milestone/task/
issue refs (M3, Task 4, Audit Log #23, #21), Bundle X task-bundle labels,
and C/D/K/S/T phase labels.
Comment text only — no code logic, string/log literals, or XML-doc structure
changed. Genuine descriptions are preserved (only the citation is stripped),
and technical lookalikes are retained (UTF-8, SHA-256, T00:00:00, M365,
UTC-5, pre-C4/pre-C5 schema versions). Flagged by the new CommentChecker
TaskReferenceInComment / TrackingReferenceInComment checks plus targeted
grep passes; full solution builds clean, append-only guard tests pass.
Code-review finding UI-Med-2: the design doc + delivery adapter treat FromNumber and
MessagingServiceSid as either-or, but the entity ctor, EF schema, UI and CLI all hard-
required FromNumber — so a Messaging-Service-only Twilio config (a normal production
setup) could not be created. Bring the implementation into line with the spec:
- Commons: SmsConfiguration.FromNumber -> string? (ctor fromNumber optional);
UpdateSmsConfigCommand.FromNumber -> string?.
- ConfigurationDatabase: FromNumber.IsRequired(false) + migration SmsFromNumberOptional
(ALTER COLUMN nullable, idempotent; Down backfills '' — harmless, MsgSid keeps it
deliverable) + regenerated model snapshot.
- Transport: SmsConfigDto.FromNumber -> string? (round-trips a Messaging-Service-only config).
- CentralUI: form validation requires AccountSid + at-least-one-of(FromNumber, MsgSid);
nullable create/edit paths; From-number help text.
- CLI: --from-number no longer Required; BuildUpdateSmsConfigCommand validates the either-or.
- Adapter: From branch null-forgiving (guarded by the existing incomplete-config check).
Tests: ManagementActor MsgSid-only persists null FromNumber; CLI MsgSid-only builds +
neither-throws + contract (--from-number not Required); CentralUI MsgSid-only save.
S1 made NotificationRecipient.EmailAddress nullable + added SmsConfiguration
and four INotificationRepository SMS methods, breaking compilation beyond the
intentionally-deferred central NotificationRepository.
Fix 1 (CS8620/CS8604 nullable EmailAddress projections, email-only paths):
- NotificationOutbox EmailNotificationDeliveryAdapter: filter non-null emails
- DeploymentManager ArtifactDeploymentService: filter non-null emails
- Transport EntitySerializer: filter non-null emails into NotificationRecipientDto
Fix 2 (CS0535): stub the four SMS-config methods on SiteRuntime
SiteNotificationRepository (central-only — NotSupportedException, matching the
existing 'Managed via artifact deployment from Central' write-path pattern).
Doc nits: reword NotificationRecipient private ctor and SmsConfiguration.AuthToken
comments.
The central ConfigurationDatabase.NotificationRepository compile break is left
as-is (S2 implements those four methods).
Joseph Doherty