Merge #587/#588: one condition event per method-driven transition (native: Ack/AddComment only); model-change + audit events gated per session on i=2253 when ACL is on

Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe

docs/AlarmTracking.md

@@ -314,9 +314,30 @@ from the scripted `AlarmCommandRouter`:
    OPC UA client (mirrors the Galaxy write-outcome limitation; the local
    `AlarmConditionState` SDK update already committed at step 1).
 
-Only the **Acknowledge** is routed to the driver. `Confirm` / `AddComment`
-/ `Shelve` operations on a native condition stay on the scripted
-`AlarmCommandRouter` path (Phase 3 scope is Acknowledge → AVEVA only).
+Only the **Acknowledge** is routed to the driver. **A native condition
+supports Acknowledge and AddComment only** (Gitea #587 review): `Confirm`,
+`OneShotShelve` / `TimedShelve` / `Unshelve` and `Enable` / `Disable` return
+`BadNotSupported` and report no event. There is no upstream path for them and
+no pair replication of a local state, and the driver projection
+(`NativeAlarmProjector`: always `Confirmed`, `Unshelved`) would silently undo
+one at the next transition. `AddComment` stays on the scripted
+`AlarmCommandRouter` path (Phase 3 scope is Acknowledge → AVEVA only) and is
+applied locally.
+
+A native condition is **never Unconfirmed**: it starts Confirmed, and because
+the SDK's own Acknowledge sets `ConfirmedState = false` on any condition type
+that supports Confirm, the node manager re-asserts `Confirmed = true` (and the
+projection's Retain rule, `Active || !Acked`) after the SDK handler, before the
+one ack event. ScadaBridge maps `ConfirmedState` into its alarm state, so this
+is client-visible.
+
+> **Secondary-node ack flap.** On a node that is not the redundancy Primary,
+> an OPC UA Acknowledge is applied to the local condition and reported (one
+> event, Acked), but `DriverHostActor`'s Primary gate drops the route upstream.
+> The driver never acknowledges, so its next transition for that alarm is
+> projected with `Acked = false` and the node reverts — with its own event. A
+> client acking against the Secondary sees acked → unacked. Ack against the
+> Primary (the higher ServiceLevel) for the ack to stick.
 
 > **Best-effort during reconnect.** If a native ack arrives while the owning
 > driver is `Connecting`/`Reconnecting` (or on a non-Primary node), it is
@@ -371,11 +392,56 @@ hosted service.
 `OnTimedUnshelve` (the SDK's automatic unshelve timer) bypasses the
 operator gate — it is system-initiated.
 
-`WriteAlarmCondition` fires the Part 9 condition event only when the
-incoming state differs from the node's current live state (delta-gate),
-preventing the double-emit that would otherwise occur when the SDK
-auto-applies the acked state and the engine re-projection fires a
-duplicate event immediately after.
+### One condition event per method call (Gitea #587)
+
+Every accepted Part 9 method call — Acknowledge, Confirm, AddComment,
+OneShotShelve, TimedShelve, Unshelve, Enable, Disable, on a native or a
+scripted condition — reports **exactly one** condition event carrying the
+new state, to folder subscribers and (through the ACL) Server-object
+subscribers alike. A refused call (bad status, e.g. a denied gate, an
+already-acknowledged condition, Enable/Disable on a native condition)
+reports nothing.
+
+- **The SDK does not report it.** `ConditionState.ReportStateChange` is
+  gated on `AutoReportStateChanges`, which this repo leaves `false` — so
+  before #587 a client Acknowledge produced no condition event at all (an
+  older comment claimed "the SDK auto-fires its own event"; it never did).
+  Instead the node manager wraps each method's SDK handler
+  (`WireMethodTransitionReporting`) and, once the handler returns Good,
+  reports the condition itself. That report does not depend on the SDK's
+  `AreEventsMonitored` bookkeeping, which is imprecise for conditions
+  materialised under an already-subscribed folder.
+- **The SDK's shelve methods never apply shelving state** (they only call
+  `OnShelve`), so the wrapper applies it — with the same
+  `(shelved, oneShot, 0)` mapping the projection uses — before reporting.
+- **The re-projection does not duplicate it.** The engine's (or driver's)
+  answer to the routed command reaches `WriteAlarmCondition`'s delta-gate,
+  which compares against the node's current state — already the new state —
+  and suppresses. The whole method call runs under the node-manager lock, so
+  a fast re-projection cannot slip in between the SDK applying the state and
+  the report.
+- **Only a real change is reported.** The condition's state is compared
+  before and after the SDK handler; a call that changed nothing reports
+  nothing. The SDK accepts acknowledging an acknowledged condition and sets
+  `Confirmed = false` again, so a re-ack also restores the pre-call Confirmed
+  and Retain — silent, and a scripted node never drifts from its engine.
+  AddComment always reports (the comment is the change).
+- **Native conditions: Acknowledge and AddComment only** — see §"Native alarm
+  acknowledge → AVEVA" above. The other methods are refused
+  (`BadNotSupported`, no event), and a native ack keeps the condition
+  Confirmed.
+- **TimedShelve works (scripted).** `Create()` left `MaxTimeShelved` at 0, and
+  the SDK rejects any shelving time above it, so every client TimedShelve
+  failed with `BadShelvingTimeOutOfRange`. It is now `int.MaxValue` ms (≈24.8
+  days). The node applies the shelve with no SDK timer of its own; the engine
+  owns the expiry and its Unshelved re-projection reports the one expiry
+  event.
+- Method **audit** events (`AuditConditionAcknowledgeEventType`, …) are still
+  the SDK's, delivered only while the server is auditing over an encrypted
+  channel.
+
+Engine-driven transitions are unaffected: `WriteAlarmCondition` still fires
+whenever the incoming state differs from the node's current state.
 
 ### AdminUI path
 

docs/ScriptedAlarms.md

@@ -352,13 +352,13 @@ On allow, the handler publishes a `Commons.OpcUa.AlarmCommand` (containing comma
 
 **Scripted vs native conditions — Enable/Disable and Acknowledge:**
 - **Scripted conditions** — all eight Part 9 operations (including Enable/Disable) route through `AlarmCommandRouter` onto the `alarm-commands` topic, which `ScriptedAlarmHostActor` dispatches to the engine (`EnableAsync` / `DisableAsync` / `AcknowledgeAsync` / …). On enable, `ActiveState` is re-derived from the next predicate evaluation.
-- **Native (driver-fed) conditions** — `OnAcknowledge` branches to `NativeAlarmAckRouter` and routes the ack to the owning driver rather than the scripted engine (see §[Native-alarm OPC UA operator operations](#native-alarm-opc-ua-operator-operations) above). `OnEnableDisable` returns `BadNotSupported` immediately — native conditions have no engine-side enable/disable surface.
+- **Native (driver-fed) conditions** — `OnAcknowledge` branches to `NativeAlarmAckRouter` and routes the ack to the owning driver rather than the scripted engine (see §[Native-alarm OPC UA operator operations](#native-alarm-opc-ua-operator-operations) above). `OnEnableDisable`, `OnConfirm` and `OnShelve` return `BadNotSupported` immediately — a native condition supports Acknowledge and AddComment only (no engine-side surface, no upstream path, and the driver projection owns its Confirmed/Shelving; Gitea #587 review).
 
 `OnTimedUnshelve` (the SDK's internal auto-unshelve timer) bypasses the client gate — it is system-initiated and not subject to operator role checks.
 
 ### Delta-gate de-duplication
 
-`WriteAlarmCondition` fires a Part 9 condition event only when the incoming state differs from the node's current live state. This suppresses the double-emit that would otherwise occur when the OPC UA SDK auto-applies the acked state on the node and the engine's re-projection then attempts to fire a duplicate event.
+`WriteAlarmCondition` fires a Part 9 condition event only when the incoming state differs from the node's current live state. For an OPC UA method call (Acknowledge, Confirm, AddComment, the shelve methods, Enable/Disable) the SDK applies the new state to the node, and the node manager then reports **the one** condition event for it from the method path (Gitea #587 — the SDK reports none of its own, since `AutoReportStateChanges` stays false; the shelve methods' state is applied by the node manager, because the SDK's never applies it; a call that changed nothing — e.g. a re-ack — reports nothing). Native (driver-fed) conditions support only Acknowledge and AddComment; their Confirm, shelve and Enable/Disable methods return `BadNotSupported`. The engine's later re-projection of the same transition finds the node already in that state and is suppressed, so each accepted call yields exactly one event. See `docs/AlarmTracking.md` §"One condition event per method call".
 
 ### AdminUI path
 

docs/security.md

@@ -564,18 +564,37 @@ Behaviours worth knowing before enabling it:
     condition reports through itself (`AuditConditionAcknowledgeEventType`, Comment, Enable, Shelving)
     reach a Server-object item only while the server is auditing, over SignAndEncrypt or https-binary,
     which is exactly the SDK's own guard. And while any Server-object item exists, every condition counts as
-    **monitored** (`ConditionState.EventsMonitored()`), so the SDK still reports its method-driven state
-    changes on a server whose only subscribers are Server-object items. Both were caught in review: the
+    **monitored** (`ConditionState.EventsMonitored()`), which gates the SDK's method **audit** events on a
+    server whose only subscribers are Server-object items. Both were caught in review: the
     first cut queued `i=8944` to every Server-object item with auditing off, and left conditions
     unmonitored. The second also needed the SDK's `AddRootNotifier` neutralised: its "subscribe existing
     all-events items with `unsubscribe: true`" bug would otherwise take the unit back from every
-    condition rebuilt under a live item.
+    condition rebuilt under a live item. (Method-driven **state-change** events no longer depend on this
+    bookkeeping at all — the node manager reports them itself, Gitea #587; see `docs/AlarmTracking.md`.
+    The SDK's counter stays imprecise in one known way — a folder subscriber's unit never reaches a
+    condition materialised under that folder later, yet its unsubscribe still lowers it — which can now
+    only cost an audit event, and only while auditing is on.)
+  - Materialising a condition raises only **that** condition's monitored unit; the full reconcile (every
+    condition) runs on removal, rebuild and Server-object subscribe/delete. A deploy of N alarms under a
+    live Server-object item is therefore N bookkeeping steps, not N²/2 under the node-manager lock.
 
-  Residual, not closed by #586: **audit and model-change events** (`AuditWriteUpdateEvent`,
-  `GeneralModelChangeEvent`, …) are still reported straight to the Server object and reach every
-  Server-object item ungated. They are delivered only when auditing / model-change reporting is on (and
-  audit events only over an encrypted channel), and a node manager has no seam on the core Server
-  object's fan-out to filter them.
+  **Model-change and audit events on the Server object are gated too** (Gitea #588). The node manager's
+  own server-wide events — `GeneralModelChangeEvent` (NodeAdded / NodeDeleted / DataTypeChanged) and the
+  `AuditWriteUpdateEvent` of a rejected device write — used to go straight to `Server.ReportEvent`, where
+  the core Server object queued them to every Server-object item ungated: a session with no grant learned
+  the RawPath / UNS path of every node added, deleted or retyped, and (with auditing on) the written and
+  prior values of a rejected write. With the flag **on**, the node manager now delivers them itself, item by
+  item, to the Server-object items it already holds for #586 (the same audience — only Server-object items
+  ever received them):
+  - A **model-change** event keeps only the changes whose `Affected` node the session may **Browse**
+    (ancestors of a granted node included, as for Browse itself). Every event the server raises today carries
+    one change, so in practice it is delivered whole or not at all; a multi-change event would be delivered
+    as a copy holding only the visible changes, with a neutral `Message` (the original names the nodes).
+  - An **audit write** event needs `Read` on its source node — it carries the node's prior value and the
+    attempted one — on top of the SDK's own guard (server auditing on, SignAndEncrypt or https-binary).
+    Auditing stays **off by default**: this repo never sets `ServerConfiguration.AuditingEnabled`.
+  - Any other server-wide event type is withheld (fail closed); the node manager raises none today.
+  - With the flag **off**, these events go to `Server.ReportEvent` exactly as before.
 - **An ancestor of a granted node is browsable**, or no narrow grant would be reachable: a client
   arrives at a deep node by browsing its ancestors, and the trie otherwise inherits downward only. Note
   what this necessarily discloses — the *names* of the folders on the path to a granted node are visible