fix(alarms): SDK timed-unshelve timer is a no-op; commented re-ack comment is durable (#597)

Review R1 M2: since #597 the SDK timer fires at real expiry, and OnTimedUnshelve routed an operator-style
Unshelve by 'system' from both pair nodes, pre-empting the engine's AutoUnshelve. The engine owns expiry,
so the callback now routes nothing. Integration test: an engine stand-in that acts on a routed Unshelve and
expires late, plus a wrapped OnTimedUnshelve (positive control: the SDK timer fired).
Review R1 m1: Part9StateMachine.ApplyAcknowledge records a commented re-ack as AddComment (persisted and
replicated) instead of a NoOp; an uncommented re-ack is still a no-op.

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

docs/AlarmTracking.md

@@ -350,15 +350,16 @@ is client-visible.
 > (ScadaBridge included) that acks against the Secondary now gets a Bad result
 > instead of a Good that later reverts — so ack against the Primary (the higher
 > ServiceLevel). Scripted conditions are **not** gated: their engine runs on
-> both nodes, so an ack on the Secondary is real. Residual: the published
-> verdict changes only on a redundancy snapshot, so in the boot window of a
-> multi-node cluster (no snapshot yet) the node still accepts, while
-> `DriverHostActor`'s live check may drop the route — the old flap, limited to
-> that window.
+> both nodes, so an ack on the Secondary is real. The published verdict
+> mirrors `DriverHostActor`'s live check from the moment the host starts, not
+> only from the first redundancy snapshot: the host publishes it once at start
+> and again whenever cluster membership changes, so in the boot window of a
+> pair (role still unknown, a driver peer Up) the gate already denies, exactly
+> as the route does (review R1 m2 — previously the view sat on its single-node
+> "allow" seed until the first snapshot, reopening the flap for that window).
 
 > **Best-effort during reconnect.** If a native ack arrives while the owning
-> driver is `Connecting`/`Reconnecting` (or, in the boot window above, on a
-> node whose role is not yet known), it is dropped with a debug log and **not**
+> driver is `Connecting`/`Reconnecting`, it is dropped with a debug log and **not**
 > retried upstream — the OPC UA Part 9 `AlarmConditionState` was already
 > committed locally at step 1, so the rare outcome is "acked locally but not
 > propagated to AVEVA." Re-issue the ack once the driver is reconnected if
@@ -408,8 +409,10 @@ On allow, the handler publishes a `Commons.OpcUa.AlarmCommand` onto the
 action is a settable `Action<AlarmCommand>` injected at boot by the
 hosted service.
 
-`OnTimedUnshelve` (the SDK's automatic unshelve timer) bypasses the
-operator gate — it is system-initiated.
+`OnTimedUnshelve` (the SDK's automatic unshelve timer) is a **no-op** — it
+routes nothing (review R1 M2). The engine owns timed-shelve expiry (its 5 s
+shelving check records `AutoUnshelve` by `system`), and its re-projection
+unshelves the node; see §"`UnshelveTime` is the real remaining time" below.
 
 ### One condition event per method call (Gitea #587)
 
@@ -448,7 +451,14 @@ reports nothing.
   AddComment always reports (the comment is the change), and so does a re-ack
   **carrying a comment** (Gitea #597): the SDK writes that comment to the node,
   so it is reported once, with Acked/Confirmed unchanged. Before #597 the
-  comment landed on the node but no subscriber ever saw it.
+  comment landed on the node but no subscriber ever saw it. For a **scripted**
+  condition the comment is also durable (review R1 m1): the routed Acknowledge
+  reaches an engine that is already acknowledged, and
+  `Part9StateMachine.ApplyAcknowledge` records a commented re-ack exactly as an
+  AddComment (kind `AddComment`, the caller as user) — persisted and replicated
+  with the condition state, the original ack's `LastAck*` fields untouched. An
+  uncommented re-ack is still a no-op in the engine. The same applies to a
+  commented re-ack from the AdminUI `/alerts` page.
 - **A native re-ack is still routed to the driver** (deliberate, #597). The
   route is fire-and-forget, so a node showing Acked does not prove the
   upstream system acknowledged — an ack dropped during a reconnect leaves
@@ -471,10 +481,14 @@ reports nothing.
   engine's projection carries its expiry (`AlarmConditionSnapshot.UnshelveAtUtc`),
   so `ShelvingState/UnshelveTime` counts down to the engine's expiry whichever
   path shelved it (the AdminUI `/alerts` timed shelve included). This arms the
-  SDK's own unshelve timer at the real instant; it changes no node state and
-  only routes an `Unshelve` (user `system`) back to the engine — a no-op if the
-  engine's 5 s expiry sweep got there first, otherwise it simply lands the
-  expiry on time.
+  SDK's own unshelve timer at the real instant, and `OnTimedUnshelve` — the
+  timer's callback — is deliberately a **no-op** (review R1 M2): it used to
+  route an operator-style `Unshelve` by `system`, which, firing before the
+  engine's 5 s expiry sweep, pre-empted the engine's `AutoUnshelve` on every
+  expiry, from both pair nodes, rewriting the audit trail as a manual unshelve.
+  The SDK's timer changes no shelving state itself, so the node stays
+  TimedShelved (with `UnshelveTime` at zero) for up to one sweep after expiry,
+  until the engine's re-projection reports the one expiry event.
 - **Accepted, not fixed — the SDK's `EventsMonitored` unit for late conditions**
   (Gitea #597 item 6, from the #586 review). The SDK's `AreEventsMonitored`
   counter never reaches a condition materialised under a folder that already had

docs/ScriptedAlarms.md

@@ -118,7 +118,8 @@ rationale is recorded in the engine's class doc so it is not re-proposed:
 - `ApplyOneShotShelve` / `ApplyTimedShelve(unshelveAtUtc)` / `ApplyUnshelve` — shelving transitions. `Timed` requires `unshelveAtUtc > nowUtc`.
 - `ApplyEnable` / `ApplyDisable` — operator enable/disable. Disabled alarms ignore predicate results until re-enabled; on enable, `ActiveState` is re-derived from the next evaluation.
 - `ApplyAddComment(text)` — append-only audit entry, no state change.
-- `ApplyShelvingCheck(nowUtc)` — called by the 5s timer; promotes expired `Timed` shelving to `Unshelved` with a `system / AutoUnshelve` audit entry.
+- `ApplyShelvingCheck(nowUtc)` — called by the 5s timer; promotes expired `Timed` shelving to `Unshelved` with a `system / AutoUnshelve` audit entry. The sole owner of timed-shelve expiry — the OPC UA node's own SDK timer does nothing.
+- `ApplyAcknowledge` on an already-acknowledged alarm is a no-op **unless it carries a comment**, which is recorded as an `AddComment` (emission `CommentAdded`; `LastAck*` untouched) — the OPC UA node reports a commented re-ack, so the system of record must hold the comment too.
 
 Two invariants the machine enforces:
 
@@ -354,11 +355,11 @@ On allow, the handler publishes a `Commons.OpcUa.AlarmCommand` (containing comma
 - **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`, `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.
+`OnTimedUnshelve` (the SDK's internal auto-unshelve timer, armed for the real remaining time since Gitea #597) is a **no-op**: the engine owns timed-shelve expiry via `ApplyShelvingCheck` (`system / AutoUnshelve`), and routing an `Unshelve` from the timer pre-empted that on every expiry as an operator-style unshelve from both pair nodes (review R1 M2).
 
 ### Delta-gate de-duplication
 
-`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. an uncommented re-ack — reports nothing; a re-ack carrying a comment reports that comment once, Gitea #597). Native (driver-fed) conditions support only Acknowledge and AddComment; their Confirm, shelve and Enable/Disable methods return `BadNotSupported`, and their Acknowledge is refused with `BadInvalidState` on a node that is not the redundancy Primary (Gitea #597). A timed shelve's `UnshelveTime` counts down to the engine's expiry (the projection carries `UnshelveAtUtc`). 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".
+`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. an uncommented re-ack — reports nothing; a re-ack carrying a comment reports that comment once, Gitea #597, and the engine records it as an `AddComment` so it is persisted and replicated, review R1 m1). Native (driver-fed) conditions support only Acknowledge and AddComment; their Confirm, shelve and Enable/Disable methods return `BadNotSupported`, and their Acknowledge is refused with `BadInvalidState` on a node that is not the redundancy Primary (Gitea #597). A timed shelve's `UnshelveTime` counts down to the engine's expiry (the projection carries `UnshelveAtUtc`). 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