docs(plan): implementation plan + tasks for F10b vtag rebuild-skip + Galaxy comment
This commit is contained in:
Joseph Doherty
@@ -0,0 +1,121 @@
|
||||
# F10b vtag rebuild-skip + Client.UI Galaxy comment — Implementation Plan
|
||||
|
||||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:subagent-driven-development to implement this plan task-by-task.
|
||||
|
||||
**Goal:** Close backlog #11 (F10b — skip the address-space rebuild for vtag-node-irrelevant edits) and #12 (Client.UI Galaxy operator-comment accepted-limitation).
|
||||
|
||||
**Architecture:** A: refine `needsRebuild` in `Phase7Applier.Apply` so a deploy whose only changes are VirtualTag deltas differing solely in Expression/DependencyRefs/Historize skips `RebuildAddressSpace` (the vtag node is byte-identical; the engine respawns independently via VirtualTagHostActor). B: a clarifying comment + doc + pinning test for the Galaxy sub-attribute fallback that can't surface the operator comment.
|
||||
|
||||
**Tech Stack:** .NET 10, Akka.NET, OPC UA SDK, xUnit + Shouldly.
|
||||
|
||||
**Design:** `docs/plans/2026-06-18-f10b-vtag-skip-rebuild-galaxy-comment-design.md` (committed `53f1147b`).
|
||||
|
||||
**Standing constraints:** NO Commons/proto change · NO Core.Abstractions/interface change · NO EF migration · NO bUnit · stage by explicit path (never `git add .`) · never stage the never-stage files · no force-push / no `--no-verify` · `dangerouslyDisableSandbox:true` for build/test · finish = merge to master + push.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: F10b — skip rebuild for vtag-node-irrelevant edits
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Task 2
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/Phase7Applier.cs` (refine `needsRebuild`, lines ~84-96)
|
||||
- Test: `tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/Phase7ApplierTests.cs`
|
||||
|
||||
**Context (read to confirm before editing):**
|
||||
- `Phase7Applier.Apply` (`:48-115`) computes `needsRebuild` (`:92-96`) and calls `_sink.RebuildAddressSpace()` iff true; returns `Phase7ApplyOutcome(RemovedNodes, AddedNodes, ChangedNodes, RebuildCalled)`.
|
||||
- A vtag's materialised node (`MaterialiseEquipmentVirtualTags`, `:264-272`) depends ONLY on `{EquipmentId, FolderPath, Name, DataType}` → `EnsureVariable(parent/Name, Name, DataType, writable:false)`. `Expression`/`DependencyRefs`/`Historize` do NOT touch the node (Historize is write-side only, `:260-263`).
|
||||
- `EquipmentVirtualTagPlan` (`Phase7Composer.cs:133-157`) = `(VirtualTagId, EquipmentId, FolderPath, Name, DataType, Expression, DependencyRefs, Historize)` with a CUSTOM `Equals` comparing all 8 logical fields (DependencyRefs element-wise).
|
||||
- The vtag ENGINE respawns independently via `VirtualTagHostActor.OnApply` (`:97-109`), fed by `DriverHostActor:1062` `ApplyVirtualTags(...)` — NOT through Phase7Applier. So skipping the address-space rebuild leaves the engine respawn intact.
|
||||
- The downstream idempotent `Materialise*` passes (`OpcUaPublishActor:326-342`) short-circuit on an existing node (`OtOpcUaNodeManager.EnsureVariable:1324` early-returns when the node already exists) → preserve live value + subscriptions when no rebuild happened.
|
||||
- `Phase7ApplierTests.cs` uses a `RecordingSink` capturing sink calls (read it for the exact API — how it records `RebuildAddressSpace` and how a `Phase7Plan` with `ChangedEquipmentVirtualTags` is constructed).
|
||||
|
||||
**Steps:**
|
||||
1. Add a private static helper to `Phase7Applier`:
|
||||
```csharp
|
||||
// A VirtualTag's materialised OPC UA node (MaterialiseEquipmentVirtualTags) is derived ONLY from
|
||||
// {EquipmentId, FolderPath, Name, DataType}. Expression/DependencyRefs/Historize are engine/write-side
|
||||
// only and are adopted by VirtualTagHostActor's independent respawn (DriverHostActor → ApplyVirtualTags),
|
||||
// so a delta that changes ONLY those three leaves a byte-identical node and needs no address-space
|
||||
// rebuild. Whitelist-of-may-differ via `with`+custom-Equals: any OTHER field difference (current or
|
||||
// future) makes the override unequal → falls back to a full rebuild (safe default).
|
||||
private static bool VtagDeltaIsNodeIrrelevant(Phase7Plan.EquipmentVirtualTagDelta d) =>
|
||||
(d.Previous with
|
||||
{
|
||||
Expression = d.Current.Expression,
|
||||
DependencyRefs = d.Current.DependencyRefs,
|
||||
Historize = d.Current.Historize,
|
||||
}).Equals(d.Current);
|
||||
```
|
||||
(Confirm the delta type's exact name/namespace — likely `Phase7Plan.EquipmentVirtualTagDelta` with `.Previous`/`.Current` — by reading `Phase7Plan`. Adjust accessors to match.)
|
||||
2. Replace the `plan.AddedEquipmentVirtualTags... || plan.ChangedEquipmentVirtualTags.Count > 0` term in `needsRebuild` (`:96`) with:
|
||||
```csharp
|
||||
plan.AddedEquipmentVirtualTags.Count > 0 || plan.RemovedEquipmentVirtualTags.Count > 0 ||
|
||||
plan.ChangedEquipmentVirtualTags.Any(VtagDeltaIsNodeIrrelevant ... )
|
||||
```
|
||||
— specifically: keep Added/Removed forcing rebuild, and make the Changed term `plan.ChangedEquipmentVirtualTags.Any(d => !VtagDeltaIsNodeIrrelevant(d))`. So `needsRebuild` is false only when every effective change is a node-irrelevant vtag edit and nothing else changed. Leave `ChangedNodes`/`changedCount` computation as-is (it still counts the vtag change).
|
||||
3. Update the `:84-91` comment block to document the new vtag-skip + the safety reasoning (node-irrelevant ⇒ engine respawns independently ⇒ subscriptions preserved; any structural or node-affecting change still rebuilds).
|
||||
4. **Tests** in `Phase7ApplierTests.cs` (mirror the existing plan-construction + RecordingSink assertions):
|
||||
- Expression-only vtag delta ⇒ `outcome.RebuildCalled == false` AND the RecordingSink recorded NO `RebuildAddressSpace` AND `outcome.ChangedNodes == 1`.
|
||||
- DependencyRefs-only and Historize-only vtag deltas ⇒ no rebuild (parameterize or separate facts).
|
||||
- DataType-also-changed vtag delta ⇒ `RebuildCalled == true`.
|
||||
- Name-changed (and FolderPath-changed) vtag delta ⇒ `RebuildCalled == true`.
|
||||
- Node-irrelevant vtag delta MIXED with another change (e.g. a ChangedEquipmentTag) ⇒ `RebuildCalled == true`.
|
||||
5. **Build + test:** `dotnet build src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer` + `dotnet test tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests --filter "FullyQualifiedName~Phase7Applier"` (dangerouslyDisableSandbox:true). All green.
|
||||
6. **Commit** (explicit paths): `Phase7Applier.cs` + `Phase7ApplierTests.cs`.
|
||||
|
||||
**Acceptance:** vtag Expression/DependencyRefs/Historize-only edits skip `RebuildAddressSpace`; any node-affecting or structural change still rebuilds; tests green. No node-manager/SDK/interface change.
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Client.UI Galaxy operator-comment — comment + doc + pinning test
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~3 min
|
||||
**Parallelizable with:** Task 1
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/OpcUaClientService.cs` (inline comment at the fallback `AlarmEvent?.Invoke` site, ~:817)
|
||||
- Modify: `src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared/Models/AlarmEventArgs.cs` (tighten the `OperatorComment` doc, ~:82-88)
|
||||
- Test: `tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests/OpcUaClientServiceTests.cs` (pinning test)
|
||||
|
||||
**Context:** The Galaxy sub-attribute fallback (`OpcUaClientService.cs:769-820`) reads InAlarm/Acked/TimeAlarmOn/DescAttrName when standard event fields are null, then raises `AlarmEventArgs` WITHOUT `operatorComment` (the param defaults null). This is an ACCEPTED LIMITATION — not code-fixable: Galaxy `AckMsg` is write-only (`AlarmRefBuilder.cs:15-20`, no readable comment attribute) and the OPC UA event SelectClause (`CreateAlarmEventFilter`) carries no comment field. The comment is only available via the gateway native alarm feed (`GalaxyAlarmTransition.OperatorComment`), the driver path — not this client fallback.
|
||||
|
||||
**Steps:**
|
||||
1. Read `OpcUaClientService.cs:769-826` (both the fallback and standard `AlarmEvent?.Invoke` sites) + `Models/AlarmEventArgs.cs:82-88` + the existing fallback test `OnAlarmEvent_MissingAckedActiveButHasConditionNode_FallbackReadsAndRaisesEvent` in `OpcUaClientServiceTests.cs`.
|
||||
2. Add an inline comment at the **fallback** `AlarmEvent?.Invoke(...)` site explaining `OperatorComment` is intentionally null here: Galaxy `AckMsg` is write-only + the OPC UA event SelectClause carries no comment field, so the operator comment is only recoverable via the gateway's native alarm feed (the Galaxy driver path), not this client fallback. (Comment-only — do NOT change the constructor call or behavior.)
|
||||
3. Tighten the `AlarmEventArgs.OperatorComment` XML doc (`:82-87`) to name the two concrete reasons (write-only `AckMsg`; no event-field) so it reads as an intentional, documented limitation rather than a TODO.
|
||||
4. Add a pinning test (mirror the existing fallback-test fixture): drive the Galaxy sub-attribute fallback path and assert the raised `AlarmEventArgs.OperatorComment` is null. Name it e.g. `OnAlarmEvent_GalaxyFallback_LeavesOperatorCommentNull`. xUnit + Shouldly; match the existing test's fake-session/subscription wiring + the `Task.Delay`/await for the background fallback read.
|
||||
5. **Build + test:** `dotnet build src/Client/ZB.MOM.WW.OtOpcUa.Client.Shared` + `dotnet test tests/Client/ZB.MOM.WW.OtOpcUa.Client.Shared.Tests --filter "FullyQualifiedName~OperatorComment"` (dangerouslyDisableSandbox:true). Green.
|
||||
6. **Commit** (explicit paths): the two src files + the test file.
|
||||
|
||||
**Acceptance:** the limitation is documented at the fallback site + on `OperatorComment`, and a test pins the null-comment behavior. No behavioral change.
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Reconcile backlog + memory + finish (merge + push)
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** none (last)
|
||||
|
||||
**Files:**
|
||||
- Modify (NEVER staged): `stillpending.md` (mark #11/#12)
|
||||
- Modify: memory `project_stillpending_backlog.md` + `MEMORY.md` index line
|
||||
|
||||
**Steps:**
|
||||
1. Full solution build: `dotnet build ZB.MOM.WW.OtOpcUa.slnx` — 0 errors.
|
||||
2. Targeted tests green: `OpcUaServer.Tests` (Phase7Applier) + `Client.Shared.Tests` (OperatorComment/alarm). Report counts.
|
||||
3. `stillpending.md` (never staged): #11 — F10b vtag-node-irrelevant rebuild-skip shipped (Expression/DependencyRefs/Historize-only edits skip `RebuildAddressSpace`; broader surgical writes deferred); #12 — Galaxy operator-comment closed as a documented + tested accepted-limitation (not code-fixable: write-only `AckMsg` + no event-field).
|
||||
4. Memory: top marker in `project_stillpending_backlog.md` + a concise `MEMORY.md` index update (keep the file under the soft cap).
|
||||
5. Finish (superpowers-extended-cc:finishing-a-development-branch): verify tests → ff-merge `feat/f10b-vtag-skip-rebuild-galaxy-comment` → master → push → delete branch → confirm `local master = origin/master`.
|
||||
|
||||
**Acceptance:** build clean, tests green, backlog/memory reconciled, merged + pushed.
|
||||
|
||||
---
|
||||
|
||||
## Dependency graph
|
||||
|
||||
`{T1 ∥ T2}` → `T3`. T1/T2 touch disjoint projects (OpcUaServer / Client.Shared) → dispatch concurrently with worktree isolation or serial-on-shared-tree (one writer at a time). T3 last.
|
||||
@@ -0,0 +1,10 @@
|
||||
{
|
||||
"planPath": "docs/plans/2026-06-18-f10b-vtag-skip-rebuild-galaxy-comment.md",
|
||||
"executionState": "PENDING",
|
||||
"tasks": [
|
||||
{"id": 1, "subject": "Task 1: F10b vtag-node-irrelevant rebuild skip + tests", "classification": "small", "status": "pending", "parallelizableWith": [2]},
|
||||
{"id": 2, "subject": "Task 2: Client.UI Galaxy operator-comment limitation comment + doc + pinning test", "classification": "small", "status": "pending", "parallelizableWith": [1]},
|
||||
{"id": 3, "subject": "Task 3: Reconcile backlog + memory + finish (merge + push)", "classification": "small", "status": "pending", "blockedBy": [1, 2]}
|
||||
],
|
||||
"lastUpdated": "2026-06-18"
|
||||
}
|
||||
Reference in New Issue
Block a user