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.
430 lines
38 KiB
Markdown
430 lines
38 KiB
Markdown
# PLAN-R2-05 — Templates, Deployment & Transport Round-2 Fix Implementation Plan
|
||
|
||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:executing-plans to implement this plan task-by-task.
|
||
|
||
**Goal:** Close the six NEW findings (N1–N6) in the round-2 report `archreview/05-templates-deployment-transport.md` (2026-07-12, re-review at HEAD `8c888f13`): the Expression-trigger Roslyn-compile leftover on read-only staleness paths (N1 — the sole Medium, the surviving member class of round-1 #12), the first-error-only trigger syntax message (N2 — the trigger twin of the T24 fix), the change-bus publisher skipping `Add` resolutions (N3), silently-inert locked-member instance overrides on import (N4), the instance-alarm-override gap in the import trust gate (N5), and the unvalidated `MaxConcurrentImportSessions` knob (N6). All round-1 accepted deferrals (incl. the #15 `ReadManifestAsync` residue) stay deferred — coverage rows only, no tasks.
|
||
|
||
**Architecture:** Two independent lanes. **TemplateEngine lane (N1+N2):** `ValidationService.ValidateExpressionTriggers` today runs `ScriptTrustValidator.FindViolations` + `RoslynScriptCompiler.Compile` per Expression-triggered script/alarm unconditionally and uncached (`ValidationService.cs:131, 531-544`) — the fix mirrors round-1 #12 exactly: memoise the verdict in `ScriptCompileVerdictCache` (whose key must first gain a **globals-surface discriminator** — it is keyed on code alone at `ScriptCompileVerdictCache.cs:53`, sound only while `ScriptCompiler`/`ScriptCompileSurface` is its sole writer; a `TriggerCompileSurface` verdict is not interchangeable), then gate the syntax/compile stage behind the same `validateScriptCompilation` flag that already carries `false` from `GetDeploymentComparisonAsync` and `StaleInstanceProbe` through `FlatteningPipeline` — the blank-expression check and attribute-reference scan stay unconditional, and the deploy gate keeps the default (`true`). **Transport lane (N3+N4+N5):** three surgical `BundleImporter` changes — publish `ScriptArtifactsChanged` for non-Skip resolutions including `Add` (`:1798-1803`); extend `EnumerateTrustGatedScripts` (`:957-1001`, the single enumerator both the apply-time Pass-0 gate at `:4309` and the preview gate at `:849` share, so one change covers both) to `InstanceAlarmOverrideDto.TriggerConfigurationOverride` expression bodies; and emit best-effort warnings (`ConflictKind.Warning` at preview + `ImportResult.Warnings` at apply) when an instance override targets a locked template member, WITHOUT changing what gets written or how the flattener resolves it. N6 is a one-line `RequireThat` in `TransportOptionsValidator`.
|
||
|
||
**Tech Stack:** C#/.NET, EF Core (in-memory + MSSQL integration fixtures), Roslyn (`Microsoft.CodeAnalysis.CSharp.Scripting`), xUnit. Build: `dotnet build ZB.MOM.WW.ScadaBridge.slnx`. Test per project: `dotnet test tests/<project>` — targeted filters only per task; no full-suite runs until the plan's terminal verification.
|
||
|
||
## Parallelization
|
||
|
||
**Concurrent lanes (dispatch as separate implementers):**
|
||
- **TemplateEngine lane (serialize — shared `ValidationService.cs` + `ScriptCompilerTests.cs`):** T1 → T2 → T3 → T4.
|
||
- **Transport `BundleImporter.cs` mutex (serialize):** T5 → T6 → T7.
|
||
- **Free / file-disjoint:** T8 (`TransportOptionsValidator.cs`) runs concurrently with everything.
|
||
- **Last:** T9 (docs sweep, blockedBy all).
|
||
|
||
**Cross-plan cautions:** T5 is the **publisher half** of N3 only — the subscriber side (Inbound API wiring + the wrong Host comment) is owned by **PLAN-R2-06**. The publisher fix is independent and self-contained (the plan-06 consumer already self-heals by content comparison, `InboundScriptExecutor.cs:370-404`), so there is **no blockedBy across plans** — either plan can land first; the coverage table records the split so both halves land coherently.
|
||
|
||
---
|
||
|
||
### Task 1: Full violation/error lists in the trigger-expression syntax check (N2)
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** ~3 min
|
||
**Parallelizable with:** 5, 6, 7, 8
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs` (`CheckExpressionSyntax` :536, :541)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs` (extend — the existing `CheckExpressionSyntax_*` facts live here, :134-148)
|
||
|
||
1. Write failing tests:
|
||
```csharp
|
||
[Fact]
|
||
public void CheckExpressionSyntax_MultipleForbiddenApis_ReportsAll()
|
||
{
|
||
var error = ValidationService.CheckExpressionSyntax(
|
||
"System.IO.File.Exists(\"x\") && System.Diagnostics.Process.GetProcesses().Length > 0");
|
||
Assert.NotNull(error);
|
||
Assert.Contains("System.IO", error); // FAILS today: only violations[0] is surfaced
|
||
Assert.Contains("Process", error);
|
||
}
|
||
|
||
[Fact]
|
||
public void CheckExpressionSyntax_MultipleCompileErrors_ReportsAll()
|
||
{
|
||
var error = ValidationService.CheckExpressionSyntax("NoSuchThingA > 1 && NoSuchThingB < 2");
|
||
Assert.NotNull(error);
|
||
Assert.Contains("NoSuchThingA", error); // FAILS today: only errors[0] is surfaced
|
||
Assert.Contains("NoSuchThingB", error);
|
||
}
|
||
```
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests --filter FullyQualifiedName~ScriptCompilerTests` → expect **FAIL** (second violation/error missing from the message).
|
||
3. Implement — the exact `string.Join` shape Task 24 gave `ScriptCompiler.TryCompile` (`ScriptCompiler.cs:50, 55`), applied to the trigger twin at `ValidationService.cs:534-544`:
|
||
```csharp
|
||
var violations = ScriptTrustValidator.FindViolations(expression);
|
||
if (violations.Count > 0)
|
||
return $"uses forbidden API: {string.Join("; ", violations)}";
|
||
|
||
var errors = RoslynScriptCompiler.Compile(expression, typeof(TriggerCompileSurface));
|
||
if (errors.Count > 0)
|
||
return $"is not a valid expression: {string.Join("; ", errors)}";
|
||
```
|
||
An operator fixing a multi-error expression sees every finding in one deploy round-trip, matching the "Report ALL violations, not just the first" comment `ScriptCompiler.cs:46-47` already carries.
|
||
4. Run → expect **PASS**. Re-run the whole filter to confirm the existing `CheckExpressionSyntax_*` facts still pass.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs && git commit -m "fix(template-engine): trigger-expression syntax check reports ALL violations/compile errors, not just the first (plan R2-05 T1)"`
|
||
|
||
### Task 2: Add a globals-surface discriminator to the verdict-cache key (N1 part 1)
|
||
|
||
**Classification:** high-risk (keying of a cache that stores script-trust verdicts — a cross-surface verdict reuse would return a stale "clean" for code never vetted against that surface)
|
||
**Estimated implement time:** ~4 min
|
||
**Parallelizable with:** 5, 6, 7, 8 (blockedBy 1 — shared `ScriptCompilerTests.cs`)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs` (`GetOrAdd` :51-69 gains a leading `string surface` parameter; key at :53 becomes `surface + ":" + hash`)
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs` (:42 — pass `nameof(ScriptCompileSurface)`)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs` (extend)
|
||
|
||
1. Write failing test (compile-red first — the signature changes — then behavioural red):
|
||
```csharp
|
||
[Fact]
|
||
public void GetOrAdd_SameCodeDifferentSurface_ComputesSeparateVerdicts()
|
||
{
|
||
ScriptCompileVerdictCache.Clear();
|
||
var a = ScriptCompileVerdictCache.GetOrAdd("SurfaceA", "return 1;", () => (true, null));
|
||
var hitsAfterA = ScriptCompileVerdictCache.Hits;
|
||
var b = ScriptCompileVerdictCache.GetOrAdd("SurfaceB", "return 1;", () => (false, "err"));
|
||
|
||
Assert.True(a.Ok);
|
||
Assert.False(b.Ok); // code-only key would return SurfaceA's verdict
|
||
Assert.Equal(hitsAfterA, ScriptCompileVerdictCache.Hits); // no false cross-surface hit
|
||
Assert.Equal(2, ScriptCompileVerdictCache.Count);
|
||
}
|
||
```
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests --filter FullyQualifiedName~ScriptCompilerTests` → expect **FAIL** (compile error on the new parameter — that counts; fix the signature, then the behavioural assertions go red under a code-only key).
|
||
3. Implement:
|
||
- `GetOrAdd(string surface, string code, Func<(bool Ok, string? Error)> factory)`; key = `surface + ":" + Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(code)))`. Update the class doc-comment: *the verdict is a pure function of code + policy + globals surface, so the surface is part of the key — a trigger expression valid against `TriggerCompileSurface` is NOT interchangeable with a `ScriptCompileSurface` script-body verdict* (the exact hazard the round-2 report calls out for N1).
|
||
- `ScriptCompiler.TryCompile` passes `nameof(ScriptCompileSurface)` at :42. No behaviour change for script bodies — same single writer, now under an explicit key segment.
|
||
4. Run the filter → expect **PASS** (including the pre-existing `TryCompile_SameCodeTwice_SecondCallIsCacheHit` cache facts at :157-171 — they exercise one surface and must be unaffected).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompileVerdictCache.cs src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ScriptCompiler.cs tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs && git commit -m "fix(template-engine): verdict-cache key gains the globals-surface discriminator — cross-surface verdict reuse structurally impossible (plan R2-05 T2)"`
|
||
|
||
### Task 3: Cache Expression-trigger verdicts under the trigger surface key (N1 part 2)
|
||
|
||
**Classification:** high-risk (trust-gate path — the trigger syntax check is a forbidden-API verdict; a caching bug here weakens the gate)
|
||
**Estimated implement time:** ~4 min
|
||
**Parallelizable with:** 5, 6, 7, 8 (blockedBy 2 — same files)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs` (`CheckExpressionSyntax` :531-544 routes through `ScriptCompileVerdictCache`)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs` (extend)
|
||
|
||
1. Write failing test plus the cross-surface lock-in negative:
|
||
```csharp
|
||
[Fact]
|
||
public void CheckExpressionSyntax_SameExpressionTwice_SecondCallIsCacheHit()
|
||
{
|
||
ScriptCompileVerdictCache.Clear();
|
||
ValidationService.CheckExpressionSyntax("Attributes[\"Temp\"] != null");
|
||
var hitsBefore = ScriptCompileVerdictCache.Hits;
|
||
var error = ValidationService.CheckExpressionSyntax("Attributes[\"Temp\"] != null");
|
||
Assert.Null(error);
|
||
Assert.Equal(hitsBefore + 1, ScriptCompileVerdictCache.Hits); // FAILS today: no cache use
|
||
}
|
||
|
||
[Fact]
|
||
public void CheckExpressionSyntax_ScriptSurfaceVerdict_NotReusedForTriggerSurface()
|
||
{
|
||
// "Notify != null" resolves on ScriptCompileSurface (Notify is a script global,
|
||
// ScriptCompileSurface.cs:53) but NOT on TriggerCompileSurface — a shared
|
||
// code-only cache entry would wrongly report the trigger expression clean.
|
||
ScriptCompileVerdictCache.Clear();
|
||
Assert.True(new ScriptCompiler().TryCompile("Notify != null", "S").IsSuccess); // warms the SCRIPT-surface entry
|
||
var error = ValidationService.CheckExpressionSyntax("Notify != null");
|
||
Assert.NotNull(error); // green today (no cache), and MUST STAY green after T3 — the T2 key makes it structural
|
||
}
|
||
```
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests --filter FullyQualifiedName~ScriptCompilerTests` → expect **FAIL** (the cache-hit fact; the cross-surface fact is the lock-in guard that must never flip).
|
||
3. Implement — wrap the T1-joined factory in the surface-keyed cache:
|
||
```csharp
|
||
internal static string? CheckExpressionSyntax(string expression)
|
||
{
|
||
// Memoise the verdict under the TRIGGER surface key (see ScriptCompileVerdictCache):
|
||
// the verdict is a pure function of expression + policy + TriggerCompileSurface, and
|
||
// it is the exact hot-path cost N1 flagged — every staleness sweep / import probe of
|
||
// an Expression-triggered config re-ran a full trust compilation + script compile.
|
||
var (_, error) = ScriptCompileVerdictCache.GetOrAdd(nameof(TriggerCompileSurface), expression, () =>
|
||
{
|
||
var violations = ScriptTrustValidator.FindViolations(expression);
|
||
if (violations.Count > 0)
|
||
return (false, $"uses forbidden API: {string.Join("; ", violations)}");
|
||
|
||
var errors = RoslynScriptCompiler.Compile(expression, typeof(TriggerCompileSurface));
|
||
if (errors.Count > 0)
|
||
return (false, $"is not a valid expression: {string.Join("; ", errors)}");
|
||
|
||
return (true, (string?)null);
|
||
});
|
||
return error;
|
||
}
|
||
```
|
||
The stored error is already entity-name-free — `CheckExpressionTrigger` formats the entity name in at :449-451, matching the cache's name-free contract.
|
||
4. Run the filter → expect **PASS** (both new facts + all pre-existing `CheckExpressionSyntax_*`/`TryCompile_*` facts).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ScriptCompilerTests.cs && git commit -m "perf(template-engine): cache Expression-trigger compile verdicts under the trigger-surface key — unchanged expressions compile once per process (plan R2-05 T3)"`
|
||
|
||
### Task 4: Skip the trigger syntax check on read-only staleness/comparison paths (N1 part 3)
|
||
|
||
**Classification:** standard (read paths only; the deploy gate keeps the authoritative compile by default — the same shape round-1 #12/T16 shipped for script bodies)
|
||
**Estimated implement time:** ~4 min
|
||
**Parallelizable with:** 5, 6, 7, 8 (blockedBy 3 — same files)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs` (`Validate` :131 threads the flag; `ValidateExpressionTriggers` :370-400 + `CheckExpressionTrigger` :420-463 gain the gate; `validateScriptCompilation` doc-comment :98-105 updated)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs` (extend)
|
||
|
||
1. Write failing tests (build the config the way the file's existing facts do — a `FlattenedConfiguration` with one script whose `TriggerType = "Expression"` and `TriggerConfiguration = """{"expression":"this is not C# (("}"""`):
|
||
```csharp
|
||
[Fact]
|
||
public void Validate_SkipCompilation_SkipsExpressionTriggerSyntaxCheck()
|
||
{
|
||
var config = ConfigWithExpressionTriggerScript("this is not C# ((");
|
||
var gated = new ValidationService().Validate(config, validateScriptCompilation: false);
|
||
Assert.DoesNotContain(gated.Errors, e => e.Message.Contains("failed validation")); // FAILS today
|
||
var full = new ValidationService().Validate(config);
|
||
Assert.Contains(full.Errors, e => e.Message.Contains("failed validation")); // deploy gate unchanged
|
||
}
|
||
|
||
[Fact]
|
||
public void Validate_SkipCompilation_StillChecksExpressionAttributeReferences()
|
||
{
|
||
// expression "Attributes[\"Ghost\"] != null" referencing a missing attribute:
|
||
// the reference scan is cheap string work and MUST survive the gate.
|
||
var config = ConfigWithExpressionTriggerScript("Attributes[\"Ghost\"] != null");
|
||
var gated = new ValidationService().Validate(config, validateScriptCompilation: false);
|
||
Assert.Contains(gated.Errors, e => e.Message.Contains("Ghost"));
|
||
}
|
||
```
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests --filter FullyQualifiedName~ValidationServiceTests` → expect **FAIL** (the gated run still emits the syntax error).
|
||
3. Implement:
|
||
- `ValidateExpressionTriggers(FlattenedConfiguration configuration, bool validateExpressionSyntax = true)` — additive default parameter, so every existing direct caller/test stays source-compatible.
|
||
- `Validate` passes `validateScriptCompilation` through at :131 — the same flag `FlatteningPipeline` already threads `false` into from `GetDeploymentComparisonAsync` (`DeploymentService.cs:690`) and `StaleInstanceProbe` (`StaleInstanceProbe.cs:37`), so **no DeploymentManager/Transport change is needed**: both read paths stop paying the per-expression trust-compilation + compile the moment this lands.
|
||
- Thread the flag into `CheckExpressionTrigger`; when `false`, skip ONLY the `CheckExpressionSyntax` call (:446-452) — the blank-expression warning/error (:432-444) and the attribute-reference scan (:454-462) still run, per the report's "the attribute-reference scan can stay".
|
||
- Extend the `validateScriptCompilation` doc-comment (:98-105): the flag now gates both the script-body compile stage AND the Expression-trigger syntax/compile check; read-only callers skip both, the deploy gate runs both.
|
||
4. Run the filter, then the full TemplateEngine project (`dotnet test tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests`) → expect **PASS**, no regressions.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs && git commit -m "perf(template-engine): read-only staleness/comparison paths skip Expression-trigger compiles — deploy gate remains the authoritative check (plan R2-05 T4)"`
|
||
|
||
### Task 5: Publish `ScriptArtifactsChanged` for `Add` resolutions too (N3)
|
||
|
||
**Classification:** standard
|
||
**Estimated implement time:** ~3 min
|
||
**Parallelizable with:** 1, 2, 3, 4, 8
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs` (`PublishScriptArtifactChanges` filter :1802 + the doc-comment :1768-1776)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs` (extend — the `RecordingScriptArtifactChangeBus` fixture and the `#05-T14` publish fact at :1336-1455 are already there)
|
||
|
||
1. Write failing test alongside `ApplyAsync_publishes_ScriptArtifactsChanged_per_kind_after_commit` (:1339):
|
||
```csharp
|
||
[Fact]
|
||
public async Task ApplyAsync_publishes_ScriptArtifactsChanged_for_Add_resolutions()
|
||
{
|
||
// ApiMethod imported as Add into a target where the name does not exist.
|
||
// Delete-then-reimport-as-Add is the N3 edge: a node can still hold a
|
||
// _knownBadMethods / compiled-handler entry under that very name, so Adds
|
||
// must notify too (over-approximation is safe per the bus contract).
|
||
var result = await ApplyBundleWithApiMethodAsync(action: ResolutionAction.Add);
|
||
|
||
var notification = Assert.Single(_artifactBus.Received,
|
||
n => n.ArtifactKind == ScriptArtifactKinds.ApiMethod);
|
||
Assert.Contains("DelmiaRecipeDownload", notification.Names); // FAILS today: Adds are filtered out
|
||
}
|
||
```
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests --filter FullyQualifiedName~BundleImporterApplyTests` → expect **FAIL** (no ApiMethod notification — `names.Count == 0` short-circuits).
|
||
3. Implement — one condition at :1802 plus the comment that justified the old behaviour:
|
||
```csharp
|
||
.Where(r => string.Equals(r.EntityType, entityType, StringComparison.Ordinal)
|
||
&& r.Action is not ResolutionAction.Skip)
|
||
```
|
||
Rewrite the doc-comment (:1768-1776): *publishes for every non-Skip resolution — Overwrite, Rename, AND Add. An Add is not "nothing cached yet": an artifact deleted on the target and re-imported as Add under the same name can still have a stale compiled-handler/`_knownBadMethods` entry on any node. Over-approximation stays safe (the bus is advisory, at-least-once; consumers self-heal), which is the contract's stated design.* Keep the Rename → `RenameTo` name projection at :1803 as-is.
|
||
4. Run the filter → expect **PASS** (including the existing per-kind Overwrite fact and the failed-apply-publishes-nothing fact — the post-commit placement at :1340 is untouched).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs && git commit -m "fix(transport): script-artifact change publisher covers Add resolutions — delete-then-reimport no longer starves non-self-healing subscribers (plan R2-05 T5)"`
|
||
|
||
### Task 6: Trust-gate instance alarm-override trigger expressions (N5)
|
||
|
||
**Classification:** high-risk (script trust gate — 5th `ScriptTrustValidator` call-site completeness; security-adjacent)
|
||
**Estimated implement time:** ~5 min
|
||
**Parallelizable with:** 1, 2, 3, 4, 8 (blockedBy 5 — `BundleImporter.cs` mutex)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs` (`EnumerateTrustGatedScripts` :957-1001 gains the instance pass; `ExtractTriggerExpression` :1009-1033 refactored to expose a JSON-only `ExtractExpressionBody`)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs` (extend — sits beside the template twin `Apply_AlarmExpressionTriggerWithForbiddenApi_HardBlocks` at :371)
|
||
|
||
**Severity decision (recorded here, per the review convention):** instance-override trigger findings are **hard errors**, matching every other trust-gate surface. The advisory-vs-hard severity split is a property of the *name-resolution heuristic* only (false-positive-prone, so template findings are warnings — `:819, :842-846`); the trust gate's semantic verdict is authoritative with no false-positive channel, and Task 20 fixed its severity as *"a HARD error for all kinds"* (`:4301-4308`) — template script bodies and template trigger expressions already hard-block today. An instance override is the same executable surface (it *replaces* the template's trigger configuration in the flattened config and compiles/executes at the site), so it takes the same severity. The deploy gate (`ValidateExpressionTriggers` over the flattened, post-override config) stays the authoritative backstop — this closes the import-*review* completeness gap so the operator learns in the wizard, not at deploy time.
|
||
|
||
1. Write failing tests (negative tests required for a trust-gate change):
|
||
```csharp
|
||
[Fact]
|
||
public async Task Apply_InstanceAlarmOverrideExpressionWithForbiddenApi_HardBlocks()
|
||
{
|
||
// Bundle: template with an Expression-triggered alarm + an instance whose
|
||
// InstanceAlarmOverrideDto.TriggerConfigurationOverride carries
|
||
// {"expression":"System.Diagnostics.Process.GetProcesses().Length > 0"}.
|
||
var ex = await Assert.ThrowsAsync<SemanticValidationException>(
|
||
() => ApplyBundleAsync(bundle)); // FAILS today: imports clean
|
||
Assert.Contains(ex.Errors, e => e.Contains("Process") && e.Contains("trigger override"));
|
||
// Rollback contract: nothing persisted (mirror the template-twin's assertions).
|
||
}
|
||
|
||
[Fact]
|
||
public async Task Preview_InstanceAlarmOverrideExpressionWithForbiddenApi_SurfacesBlocker()
|
||
{
|
||
var preview = await PreviewBundleAsync(bundle);
|
||
Assert.Contains(preview.Blockers, b =>
|
||
b.Kind == ConflictKind.Blocker && b.EntityType == "Instance"
|
||
&& b.BlockerReason!.Contains("trust violation")); // FAILS today
|
||
}
|
||
|
||
[Fact]
|
||
public async Task Apply_InstanceAlarmOverrideExpression_Clean_Imports()
|
||
{
|
||
// {"expression":"Attributes[\"Temp\"] != null"} → import succeeds, override row persisted verbatim.
|
||
}
|
||
```
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests --filter FullyQualifiedName~SemanticValidatorImportTests` → expect **FAIL** (both forbidden-API cases import/preview clean).
|
||
3. Implement:
|
||
- Split `ExtractTriggerExpression` (:1009-1033): the trigger-type guard stays in `ExtractTriggerExpression`, the JSON `{"expression":"…"}` parse moves to a new `private static string? ExtractExpressionBody(string? triggerConfigJson)` it delegates to.
|
||
- Append the instance pass to `EnumerateTrustGatedScripts` after the ApiMethod loop (:993-1000):
|
||
```csharp
|
||
foreach (var i in content.Instances)
|
||
{
|
||
if (IsSkipResolution(resolutionMap, "Instance", i.UniqueName)) continue;
|
||
foreach (var o in i.AlarmOverrides)
|
||
{
|
||
// The DTO carries no trigger type (the type lives on the template
|
||
// alarm), so gate ANY override config carrying an {"expression":...}
|
||
// string body: if the overridden alarm is not Expression-triggered
|
||
// the body never executes, but vetting it anyway is fail-safe — the
|
||
// structured (HiLo/threshold) configs have no "expression" key, so
|
||
// there is no false-positive channel.
|
||
var expr = ExtractExpressionBody(o.TriggerConfigurationOverride);
|
||
if (!string.IsNullOrEmpty(expr))
|
||
{
|
||
yield return ("Instance", i.UniqueName,
|
||
$"{o.AlarmCanonicalName} (alarm trigger override expression)", expr);
|
||
}
|
||
}
|
||
}
|
||
```
|
||
- No gate-side change needed: the apply-time Pass 0 (:4309) and the preview gate (:849) both iterate this enumerator, so one edit covers both, including the fail-closed validator-throw handling each already has. Update the enumerator's doc-comment (:946-956) to name instance alarm-override expressions in the covered-surface list.
|
||
4. Run the filter, then `dotnet test tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests --filter FullyQualifiedName~BundleImporterPreviewTests` (preview parity) → expect **PASS**.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/SemanticValidatorImportTests.cs && git commit -m "fix(security): import trust gate covers instance alarm-override trigger expressions — 5th call site is surface-complete (plan R2-05 T6)"`
|
||
|
||
### Task 7: Warn when import persists overrides on locked template members (N4)
|
||
|
||
**Classification:** standard (warning-only UX; no write-path or flattener behaviour change — the flattener already drops locked-member overrides, `FlatteningService.cs:319, :337, :815`)
|
||
**Estimated implement time:** ~5 min
|
||
**Parallelizable with:** 1, 2, 3, 4, 8 (blockedBy 6 — `BundleImporter.cs` mutex)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs` (new best-effort lock scan called from `RunSemanticValidationAsync` (:4293, appending to `warnings`) and from `DetectBlockersAsync` (:670, emitting `ConflictKind.Warning` rows); generalize the apply-side warning log wording at :1204-1208)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/SiteInstanceImportTests.cs` (extend)
|
||
|
||
1. Write failing tests:
|
||
```csharp
|
||
[Fact]
|
||
public async Task Import_OverrideOnLockedAttribute_EmitsWarning_ImportStillSucceeds()
|
||
{
|
||
// Template attribute "SetPoint" IsLocked=true; instance carries an
|
||
// InstanceAttributeOverrideDto for "SetPoint".
|
||
var result = await ApplyBundleAsync(bundle);
|
||
Assert.Contains(result.Warnings, w => w.Contains("SetPoint") && w.Contains("locked")); // FAILS today: silent
|
||
// Behaviour unchanged: the row IS still written (bundle fidelity) — the
|
||
// flattener is what ignores it; assert the persisted override row exists.
|
||
}
|
||
|
||
[Fact]
|
||
public async Task Preview_OverrideOnLockedNativeAlarmSource_ShowsWarningRow()
|
||
{
|
||
var preview = await PreviewBundleAsync(bundle);
|
||
Assert.Contains(preview.Blockers, b =>
|
||
b.Kind == ConflictKind.Warning && b.EntityType == "Instance"
|
||
&& b.BlockerReason!.Contains("locked")); // FAILS today
|
||
}
|
||
|
||
[Fact]
|
||
public async Task Import_OverrideOnUnlockedAttribute_NoLockWarning()
|
||
```
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests --filter FullyQualifiedName~SiteInstanceImportTests` → expect **FAIL**.
|
||
3. Implement one shared best-effort scan, `CollectLockedOverrideWarnings(BundleContentDto content, Dictionary<(string,string), ImportResolution>? resolutionMap, IReadOnlyList<Template> targetTemplates)`:
|
||
- For each non-Skip instance, resolve its template by `dto.TemplateName` — bundle DTO first (the version about to be written), else the pre-existing target template (`GetAllTemplatesAsync` already Includes the child collections).
|
||
- Locked-member name sets from that template's rows: attributes/alarms/native sources with `IsLocked == true` (bundle DTOs carry `IsLocked`; derived templates carry inherited placeholder rows, so own + inherited members match by name). Check `AttributeOverrides` (→ `FlatteningService.cs:319` drop), `AlarmOverrides` (→ `:337`), and `NativeAlarmSourceOverrides` (→ `:815`) — the alarm loop is the same pattern the report's two cited collections use, included for completeness.
|
||
- Warning text: `Instance '{unique}' {kind} override '{name}' targets a LOCKED template member — the flattener ignores it, so this part of the bundle's instance config will never take effect.`
|
||
- **Best-effort, documented:** overrides addressing composed path-qualified members (`y1.z.Val`) or derived-shadow locks (`LockedInDerived` on a base the placeholder row doesn't reflect) may not match a direct row — an unmatched name emits NO warning (never a false positive; the `ManagementActor` and flattener remain the enforcement points, `ManagementActor.cs:898, :978`).
|
||
- Wire it twice: `RunSemanticValidationAsync` appends to `warnings` (they ride `validationWarnings` → `ImportResult.Warnings` at :1359 — generalize the "advisory template-script reference warning(s)" log wording at :1206-1208 to "advisory import warning(s)"); `DetectBlockersAsync` maps each to an `ImportPreviewItem` with `EntityType: "Instance"`, `Kind: ConflictKind.Warning` — the Task-19 machinery, so the wizard renders it with the existing Warning badge, no UI change.
|
||
- Do **NOT** block the import and do **NOT** change what `PopulateInstanceChildren` writes (:3999-4029) — bundle fidelity keeps the row; the flattener stays the behavioural authority (binding scope ruling: no flattener change).
|
||
4. Run the filter + `--filter FullyQualifiedName~BundleImporterPreviewTests` → expect **PASS**.
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/SiteInstanceImportTests.cs && git commit -m "fix(transport): warn on import of instance overrides targeting locked template members — inert rows no longer silent (plan R2-05 T7)"`
|
||
|
||
### Task 8: Validate `MaxConcurrentImportSessions` at startup (N6)
|
||
|
||
**Classification:** small
|
||
**Estimated implement time:** ~2 min
|
||
**Parallelizable with:** 1, 2, 3, 4, 5, 6, 7 (file-disjoint with everything)
|
||
**Files:**
|
||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Transport/TransportOptionsValidator.cs` (one `RequireThat` in `Validate` :25-66)
|
||
- Test: `tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/TransportOptionsValidatorTests.cs` (extend — mirror `ZeroMaxBundleSizeMb_IsRejected` :34)
|
||
|
||
1. Write failing tests:
|
||
```csharp
|
||
[Fact]
|
||
public void ZeroMaxConcurrentImportSessions_IsRejected()
|
||
{
|
||
var result = Validate(new TransportOptions { MaxConcurrentImportSessions = 0 });
|
||
Assert.True(result.Failed);
|
||
Assert.Contains("MaxConcurrentImportSessions", result.FailureMessage); // FAILS today: validates clean
|
||
}
|
||
|
||
[Fact]
|
||
public void NegativeMaxConcurrentImportSessions_IsRejected()
|
||
```
|
||
2. Run: `dotnet test tests/ZB.MOM.WW.ScadaBridge.Transport.Tests --filter FullyQualifiedName~TransportOptionsValidatorTests` → expect **FAIL**.
|
||
3. Implement — the knob its own fix wave added (`TransportOptions.cs:37`, enforced at `BundleSessionStore.cs:78-88` where a `0`/negative cap rejects **every** import forever with "Too many concurrent import sessions (0)"):
|
||
```csharp
|
||
builder.RequireThat(options.MaxConcurrentImportSessions > 0,
|
||
$"ScadaBridge:Transport:MaxConcurrentImportSessions must be positive " +
|
||
$"(was {options.MaxConcurrentImportSessions}); a zero/negative cap rejects every import session forever.");
|
||
```
|
||
4. Run the filter → expect **PASS** (`DefaultOptions_AreValid` must stay green — the default is 8).
|
||
5. Commit: `git add src/ZB.MOM.WW.ScadaBridge.Transport/TransportOptionsValidator.cs tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/TransportOptionsValidatorTests.cs && git commit -m "fix(transport): validate MaxConcurrentImportSessions at startup — a zero cap no longer bricks imports silently (plan R2-05 T8)"`
|
||
|
||
### Task 9: Design-doc sync sweep
|
||
|
||
**Classification:** small (docs only)
|
||
**Estimated implement time:** ~4 min
|
||
**Parallelizable with:** none (run last)
|
||
**Files:**
|
||
- Modify: `docs/requirements/Component-Transport.md`, `docs/requirements/Component-ScriptAnalysis.md`, `docs/requirements/Component-TemplateEngine.md`, `/Users/dohertj2/Desktop/ScadaBridge/CLAUDE.md` (component #24 blurb + Transport bullet)
|
||
|
||
No tests; concrete edits, then a stale-cross-reference sweep per repo convention:
|
||
1. **Component-Transport.md:** (a) trust-gate section — the gated-surface list gains *instance alarm-override trigger expressions* (T6), same hard severity, with the severity-decision sentence (semantic verdict authoritative; the advisory split remains name-heuristic-only); (b) import-warning section — locked-member override warnings (T7): advisory `ConflictKind.Warning` at preview + `ImportResult.Warnings` at apply, best-effort by direct member name, rows still written, flattener remains the behavioural authority; (c) change-bus paragraph — publisher now notifies every non-Skip resolution including Add (T5), and note the subscriber side lives with the Inbound API (**PLAN-R2-06** owns the subscriber wiring + the stale Host comment); (d) options table — `MaxConcurrentImportSessions` is now startup-validated (T8). Leave the `ReadManifestAsync` deferral note (:92) untouched — still deferred.
|
||
2. **Component-ScriptAnalysis.md:** fifth-call-site (Transport) surface list gains instance alarm-override trigger expressions; note the verdict cache is keyed by (globals surface, code hash) so script-body and trigger-expression verdicts never cross (T2/T3).
|
||
3. **Component-TemplateEngine.md:** validation-pipeline section — `validateScriptCompilation: false` now also skips the Expression-trigger syntax/compile check on read-only staleness/comparison paths (blank + attribute-reference checks still run; deploy gate authoritative, T4); trigger syntax findings report all violations/errors (T1).
|
||
4. **CLAUDE.md:** component #24 blurb / Transport Key-Design-Decisions bullet — extend the script-trust-gate sentence: Expression-trigger gating covers template scripts, alarms, **and instance alarm-override expressions**. No `../scadaproj/CLAUDE.md` change (no wire-relationship/stack change) — verify and note in the commit message.
|
||
5. `git diff` review, then commit: `git add docs/requirements/Component-Transport.md docs/requirements/Component-ScriptAnalysis.md docs/requirements/Component-TemplateEngine.md CLAUDE.md && git commit -m "docs: sync Transport/ScriptAnalysis/TemplateEngine specs + CLAUDE.md with the round-2 fix wave (plan R2-05 T9)"`
|
||
|
||
---
|
||
|
||
## Dependencies on other plans
|
||
|
||
- **PLAN-R2-06 (Edge Integrations round 2):** owns the **subscriber side** of the change bus (Inbound API wiring + the wrong Host comment). T5's publisher fix is independent — the shipped plan-06 consumer self-heals by content comparison (`InboundScriptExecutor.cs:370-404`), so no cross-plan `blockedBy` exists in either direction; the two halves land coherently in any order. R2-06 should read T5's updated publisher doc-comment (Adds now publish) when wiring any non-self-healing subscriber.
|
||
- **No other cross-plan edges.** N1's gating rides the `validateScriptCompilation` flag PLAN-05 T16 already threads from DeploymentManager — no DeploymentManager file is touched.
|
||
|
||
## Execution order
|
||
|
||
**P0 (start immediately, in parallel):** Task 1 (TemplateEngine lane opener), Task 5 (BundleImporter lane opener), Task 8 (free).
|
||
**Lane A (serialize — shared files):** 1 → 2 → 3 → 4.
|
||
**Lane B (serialize — `BundleImporter.cs` mutex):** 5 → 6 → 7.
|
||
**Last:** 9 (docs sweep), then terminal verification: `dotnet build ZB.MOM.WW.ScadaBridge.slnx` + `dotnet test tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests && dotnet test tests/ZB.MOM.WW.ScadaBridge.Transport.Tests && dotnet test tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests && dotnet test tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests` (DeploymentManager consumes the `Validate` flag) before declaring the plan done.
|
||
|
||
## Findings Coverage
|
||
|
||
| Report finding | Severity | Task(s) |
|
||
|---|---|---|
|
||
| N1 — Expression-trigger validation Roslyn-compiles on read-only staleness paths, uncached; any shared cache must key by globals surface | Medium (perf) | 2 (surface-discriminated cache key), 3 (cache trigger verdicts), 4 (skip on comparison/probe paths via the existing `validateScriptCompilation` flag) |
|
||
| N2 — `CheckExpressionSyntax` surfaces only the first violation / first compile error | Low | 1 (the T24 `string.Join` fix mirrored onto the trigger twin) |
|
||
| N3 — `PublishScriptArtifactChanges` skips `Add` resolutions | Low | 5 (publisher publishes every non-Skip resolution); **subscriber side (Inbound API wiring + wrong Host comment) → PLAN-R2-06** — publisher fix is independent, no cross-plan blockedBy |
|
||
| N4 — Import persists silently-inert instance overrides on locked members | Low | 7 (`ConflictKind.Warning` preview rows + `ImportResult.Warnings` at apply; best-effort direct-member match; rows still written, flattener behaviour unchanged per ruling) |
|
||
| N5 — Import trust gate misses `InstanceAlarmOverrideDto.TriggerConfigurationOverride` expression bodies | Low (sec) | 6 (enumerator covers instance overrides; **hard error** — trust-gate severity is uniform, the advisory split is name-heuristic-only; deploy gate stays authoritative backstop; negative tests included) |
|
||
| N6 — `TransportOptionsValidator` misses `MaxConcurrentImportSessions` | Low (conv) | 8 (one `RequireThat` > 0) |
|
||
| Docs drift from the above | — | 9 |
|
||
| Accepted deferral — #11 `OperationLockManager` single-node in-memory | Medium (R1) | **Deferred** (unchanged) — invariant recorded `Component-DeploymentManager.md:94`; structural active-node gating owned by plans 01/07 |
|
||
| Accepted deferral — #15 residue: `LoadAsync` manifest-peek (`ReadManifestAsync`) | Low (perf, R1) | **Deferred** (unchanged) — doc-acknowledged `Component-Transport.md:92`; pure optimisation (session cap already shipped, and N6/T8 now validates it) |
|
||
| Accepted deferral — #16 import apply is one long EF transaction | Low (perf, R1) | **Deferred** (unchanged) — dominant cost removed by #12/N1; restructuring the rollback contract stays high-risk/low-gain |
|
||
| Accepted deferral — U4: rename call-site rewriting, Transport-012 filter UI | — | **Deferred** (unchanged) — recorded limitations, `Component-Transport.md:354` |
|
||
| Accepted deferral — U5: preview→apply optimistic concurrency window | — | **Deferred** (unchanged) — recorded decision, `Component-Transport.md:138`; version fields reserved (`ArtifactDiff.cs:33-37`) |
|
||
| Accepted deferral — U6: three hand-maintained compile-surface mirrors | — | **Deferred** (unchanged) — guard tests exist; source-generator is an improvement, not a defect |
|