fix(scripts): reset-aware handshake refuses a null reset value; document additive timeouts (#36 review)

A null resetValue encoded to null, which wrote an empty string and then handed the
waiter a null target (= any change), fast-pathing on the stale cache and reinstating
the race the overload exists to close. The context parameter is now non-nullable and
the accessor throws ArgumentException before any write. Also: additive-timeout note on
all surfaces, batch-failure message no longer names the bool overload, and the
error-message/batch-failure branches are pinned.

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

docs/plans/2026-09-10-handshake-reset-echo-design.md

@@ -126,6 +126,11 @@ if (!hs.Matched)
   arrives after this run's reset echo can still satisfy the response wait. A level-triggered flag
   protocol cannot distinguish that case; it needs a sequence/handle attribute on the device side.
 
+**The two waits are additive.** A call can run for `resetTimeout` + `timeout` plus Ask slack, so a
+script using the overload must set `ExecutionTimeoutSeconds` above that sum. A script deadline hit
+part-way through either leg propagates as cancellation (§4.3) — the script sees the cancellation,
+not a `HandshakeResult` naming the phase it died in.
+
 ## 6. Mirrors and analysis surfaces
 
 - `ScriptCompileSurface` (ScriptAnalysis) — compile-only mirror of the new overload

src/ZB.MOM.WW.ScadaBridge.CentralUI/ScriptAnalysis/SandboxScriptHost.cs

@@ -476,7 +476,11 @@ public class SandboxAttributeAccessor
     /// <param name="responseValue">The response value to wait for.</param>
     /// <param name="timeout">The maximum time to wait for the response.</param>
     /// <param name="resetKey">The attribute written first and waited on before the batch.</param>
-    /// <param name="resetValue">The reset value the device must echo before the batch is written.</param>
+    /// <param name="resetValue">
+    /// The reset value the device must echo before the batch is written. MUST NOT be null — the
+    /// real accessor throws <see cref="System.ArgumentException"/>, because a null would write an
+    /// empty string and then wait on "any change", reinstating the race the overload closes.
+    /// </param>
     /// <param name="resetTimeout">The maximum time to wait for the reset echo; <c>null</c> means <paramref name="timeout"/>.</param>
     /// <returns>Never returns; always throws <see cref="ScriptSandboxException"/> in the sandbox.</returns>
     public Task<HandshakeResult> WriteBatchAndWaitAsync(

src/ZB.MOM.WW.ScadaBridge.ScriptAnalysis/ScriptCompileSurface.cs

@@ -341,7 +341,11 @@ public sealed class ScriptCompileSurface
         /// <param name="responseValue">The value to wait for on the response attribute.</param>
         /// <param name="timeout">The maximum time to wait for the response.</param>
         /// <param name="resetKey">The attribute written first and waited on before the batch.</param>
-        /// <param name="resetValue">The reset value the device must echo before the batch is written.</param>
+        /// <param name="resetValue">
+        /// The reset value the device must echo before the batch is written. MUST NOT be null — the
+        /// real accessor throws <see cref="System.ArgumentException"/>, because a null would write an
+        /// empty string and then wait on "any change", reinstating the race the overload closes.
+        /// </param>
         /// <param name="resetTimeout">The maximum time to wait for the reset echo; <c>null</c> means <paramref name="timeout"/>.</param>
         /// <returns>The phase-aware handshake outcome.</returns>
         public Task<HandshakeResult> WriteBatchAndWaitAsync(IReadOnlyDictionary<string, object?> values, string flagKey, object? flagValue, string responseKey, object? responseValue, TimeSpan timeout, string resetKey, object? resetValue, TimeSpan? resetTimeout = null) => throw new NotSupportedException(CompileOnly);

src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScopeAccessors.cs

@@ -122,7 +122,10 @@ public class AttributeAccessor
     /// flag handshake: without an observed reset the plain overload can be satisfied
     /// instantly by the previous run's cached response value. Throws
     /// <see cref="System.InvalidOperationException"/> if the reset write or the batch/trigger
-    /// write itself fails; never throws on either timeout.
+    /// write itself fails; never throws on either timeout. The two waits are ADDITIVE — the call
+    /// can run for <paramref name="resetTimeout"/> + <paramref name="timeout"/> plus Ask slack, so
+    /// size the script's <c>ExecutionTimeoutSeconds</c> above that sum: a deadline hit mid-leg
+    /// propagates as cancellation (spec §4.3), not as a <see cref="HandshakeResult"/>.
     /// </summary>
     /// <param name="values">Attribute key → value to batch-write (data-sourced, one connection).</param>
     /// <param name="flagKey">Trigger attribute key written AFTER the batch.</param>
@@ -131,7 +134,12 @@ public class AttributeAccessor
     /// <param name="responseValue">Target value to wait for (<c>null</c> ⇒ any change).</param>
     /// <param name="timeout">How long to wait for the response before giving up.</param>
     /// <param name="resetKey">Attribute written FIRST (usually <paramref name="responseKey"/>); static or data-sourced.</param>
-    /// <param name="resetValue">Value the device must echo back before the batch is written.</param>
+    /// <param name="resetValue">
+    /// Value the device must echo back before the batch is written. MUST NOT be null: a null encodes
+    /// to null, which would write an empty string and then wait on "any change", fast-pathing on the
+    /// stale cached value and reinstating the race this overload exists to close —
+    /// <see cref="System.ArgumentException"/> is thrown before anything is written.
+    /// </param>
     /// <param name="resetTimeout">How long to wait for the reset echo; <c>null</c> ⇒ <paramref name="timeout"/>.</param>
     /// <returns>The phase-aware handshake outcome.</returns>
     public Task<HandshakeResult> WriteBatchAndWaitAsync(
@@ -148,7 +156,11 @@ public class AttributeAccessor
             Resolve(flagKey), AttributeValueCodec.Encode(flagValue),
             Resolve(responseKey), AttributeValueCodec.Encode(responseValue),
             timeout,
-            Resolve(resetKey), AttributeValueCodec.Encode(resetValue),
+            Resolve(resetKey),
+            AttributeValueCodec.Encode(resetValue)
+                ?? throw new ArgumentException(
+                    "A reset-aware handshake needs a definite reset value: null would write an " +
+                    "empty string and then match on any change.", nameof(resetValue)),
             resetTimeout ?? timeout);
     }
 

src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs

@@ -985,6 +985,12 @@ public class ScriptRuntimeContext
     /// would never resolve at all. Holding the trigger until the echo lands makes the existing
     /// level-triggered fast-path correct again. Each leg is already a traced Flow Debugger
     /// step, so a traced run shows all four.
+    /// <para>
+    /// The two waits are ADDITIVE: the call can run for <paramref name="resetTimeout"/> +
+    /// <paramref name="timeout"/> plus Ask slack, so size the script's
+    /// <c>ExecutionTimeoutSeconds</c> above that sum — a script deadline hit mid-leg propagates
+    /// as cancellation (spec §4.3), not as a <see cref="HandshakeResult"/>.
+    /// </para>
     /// </remarks>
     /// <param name="encodedValues">Scope-resolved attribute name → codec-encoded value to batch-write.</param>
     /// <param name="triggerAttr">Scope-resolved trigger attribute written AFTER the batch.</param>
@@ -993,17 +999,22 @@ public class ScriptRuntimeContext
     /// <param name="responseEncoded">Codec-encoded response target (null ⇒ any change).</param>
     /// <param name="timeout">Bound for the response wait.</param>
     /// <param name="resetAttr">Scope-resolved attribute written FIRST (usually the response attribute).</param>
-    /// <param name="resetEncoded">Codec-encoded reset value the device must echo before anything else is written.</param>
+    /// <param name="resetEncoded">
+    /// Codec-encoded reset value the device must echo before anything else is written. NON-NULL by
+    /// contract: a null would write an empty string on leg 1 and then give the leg-2 waiter a null
+    /// target — "any change" — which fast-paths on the stale cached value and reinstates the very
+    /// race this overload exists to close. The accessor rejects a null reset value before it gets here.
+    /// </param>
     /// <param name="resetTimeout">Bound for the reset-echo wait.</param>
     /// <returns>A phase-aware result; never throws on either timeout.</returns>
     public async Task<HandshakeResult> WriteBatchResetAndWait(
         IReadOnlyDictionary<string, string?> encodedValues, string triggerAttr, string? triggerEncoded,
         string responseAttr, string? responseEncoded, TimeSpan timeout,
-        string resetAttr, string? resetEncoded, TimeSpan resetTimeout)
+        string resetAttr, string resetEncoded, TimeSpan resetTimeout)
     {
         // Leg 1 — reset write. A static attribute updates the cache synchronously (leg 2 then
         // fast-paths); a data-sourced one goes to the device and the cache waits for the echo.
-        await SetAttribute(resetAttr, resetEncoded ?? string.Empty).ConfigureAwait(false);
+        await SetAttribute(resetAttr, resetEncoded).ConfigureAwait(false);
 
         // Leg 2 — reset echo. Quality-agnostic, like the response wait.
         var reset = await WaitInternal(resetAttr, resetEncoded, null, resetTimeout, requireGoodQuality: false)
@@ -1084,7 +1095,7 @@ public class ScriptRuntimeContext
 
         if (!batchResp.Success)
             throw new InvalidOperationException(
-                $"WriteBatchAndWait write failed: {batchResp.ErrorMessage}");
+                $"Batch write failed: {batchResp.ErrorMessage}");
 
         return await WaitInternal(responseAttr, responseEncoded, null, timeout, requireGoodQuality: false)
             .ConfigureAwait(false);

tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/HandshakeResetEchoTests.cs

@@ -109,7 +109,7 @@ public class HandshakeResetEchoTests : TestKit, IDisposable
         var call = ctx.WriteBatchResetAndWait(
             Values(28950), "MoveOutFlag", AttributeValueCodec.Encode(true),
             "MoveOutCompleteFlag", AttributeValueCodec.Encode(true), TimeSpan.FromSeconds(10),
-            "MoveOutCompleteFlag", AttributeValueCodec.Encode(false), TimeSpan.FromSeconds(10));
+            "MoveOutCompleteFlag", AttributeValueCodec.Encode(false)!, TimeSpan.FromSeconds(10));
 
         // Leg 1 reaches the device as a plain tag write; the DCL acks it but the device has
         // not echoed anything yet.
@@ -152,7 +152,7 @@ public class HandshakeResetEchoTests : TestKit, IDisposable
         var call = ctx.WriteBatchResetAndWait(
             Values(28950), "MoveOutFlag", AttributeValueCodec.Encode(true),
             "MoveOutCompleteFlag", AttributeValueCodec.Encode(true), TimeSpan.FromSeconds(10),
-            "MoveOutCompleteFlag", AttributeValueCodec.Encode(false), TimeSpan.FromMilliseconds(700));
+            "MoveOutCompleteFlag", AttributeValueCodec.Encode(false)!, TimeSpan.FromMilliseconds(700));
 
         var reset = dcl.ExpectMsg<WriteTagRequest>(TimeSpan.FromSeconds(5));
         dcl.Reply(new WriteTagResponse(reset.CorrelationId, true, null, DateTimeOffset.UtcNow));

tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScopeAccessorTests.cs

@@ -341,4 +341,33 @@ public class AttributeAccessorWaitAsyncTests : TestKit, IDisposable
         // Nothing else was written: no batch, no trigger.
         probe.ExpectNoMsg(TimeSpan.FromMilliseconds(300));
     }
+
+    /// <summary>
+    /// Gitea #36 review: a null <c>resetValue</c> would encode to null, which writes an empty
+    /// string on leg 1 and then hands the waiter a null target — "any change" — which fast-paths
+    /// on the stale cached value and reinstates exactly the race this overload exists to close.
+    /// The accessor rejects it before anything reaches the InstanceActor.
+    /// </summary>
+    [Fact]
+    public void WriteBatchAndWaitAsync_ResetAware_NullResetValue_ThrowsBeforeAnyWrite()
+    {
+        var probe = CreateTestProbe();
+        var ctx = MakeContext(probe.Ref);
+
+        var acc = new AttributeAccessor(ctx, "RightMESReceiver");
+
+        // Block body (not an expression lambda) so this binds the Action overload: the throw is
+        // SYNCHRONOUS — a faulted task would leave nothing for Assert.Throws to catch.
+        var ex = Assert.Throws<ArgumentException>(() =>
+        {
+            _ = acc.WriteBatchAndWaitAsync(
+                new Dictionary<string, object?> { ["MoveOutBatchID"] = 28950 },
+                "MoveOutFlag", true, "MoveOutCompleteFlag", true, TimeSpan.FromSeconds(25),
+                "MoveOutCompleteFlag", null);
+        });
+        Assert.Equal("resetValue", ex.ParamName);
+
+        // Thrown before leg 1: the reset write never left the accessor.
+        probe.ExpectNoMsg(TimeSpan.FromMilliseconds(300));
+    }
 }

tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/WriteBatchResetAndWaitTests.cs

@@ -24,7 +24,10 @@ public class WriteBatchResetAndWaitTests : TestKit
             new SharedScriptLibrary(
                 new ScriptCompilationService(NullLogger<ScriptCompilationService>.Instance),
                 NullLogger<SharedScriptLibrary>.Instance),
-            currentCallDepth: 0, maxCallDepth: 10, askTimeout: TimeSpan.FromSeconds(5),
+            // 30s: the legs Ask the InstanceActor with their own bounds (the batch's internal 35s,
+            // the waits' own timeouts); a short ambient Ask timeout would race those and make the
+            // reset/response phases indistinguishable from an AskTimeoutException.
+            currentCallDepth: 0, maxCallDepth: 10, askTimeout: TimeSpan.FromSeconds(30),
             instanceName: Instance, logger: NullLogger.Instance);
 
     [Fact]
@@ -125,6 +128,64 @@ public class WriteBatchResetAndWaitTests : TestKit
         Assert.True(result.TimedOut);
         Assert.Equal(HandshakePhase.Response, result.Phase);
         Assert.False(result.ResetFailed);
+        // The message names the attribute that did not answer and the bound it blew.
+        Assert.Contains("MoveOutCompleteFlag", result.ErrorMessage);
+        Assert.Contains("25", result.ErrorMessage);
+    }
+
+    /// <summary>
+    /// A reset wait that fails WITHOUT timing out (a predicate blew up in the waiter) surfaces the
+    /// actor's own message verbatim rather than a synthesized one, and still writes nothing further.
+    /// </summary>
+    [Fact]
+    public async Task ResetWaitError_PassesTheActorsMessageThrough()
+    {
+        var probe = CreateTestProbe();
+        var ctx = CreateContext(probe.Ref);
+
+        var call = ctx.WriteBatchResetAndWait(
+            Values, "MoveOutFlag", "True", "MoveOutCompleteFlag", "True", TimeSpan.FromSeconds(25),
+            "MoveOutCompleteFlag", "False", TimeSpan.FromSeconds(10));
+
+        var reset = probe.ExpectMsg<SetStaticAttributeCommand>(TimeSpan.FromSeconds(5));
+        probe.Reply(new SetStaticAttributeResponse(reset.CorrelationId, Instance, "MoveOutCompleteFlag", true, null, DateTimeOffset.UtcNow));
+        var resetWait = probe.ExpectMsg<WaitForAttributeRequest>(TimeSpan.FromSeconds(5));
+        probe.Reply(new WaitForAttributeResponse(
+            resetWait.CorrelationId, Matched: false, null, null, TimedOut: false, ErrorMessage: "predicate threw"));
+
+        var result = await call;
+        Assert.False(result.Matched);
+        Assert.False(result.TimedOut);
+        Assert.True(result.ResetFailed);
+        Assert.Equal("predicate threw", result.ErrorMessage);
+
+        probe.ExpectNoMsg(TimeSpan.FromMilliseconds(300));
+    }
+
+    /// <summary>
+    /// The batch leg keeps throwing after a successful reset — a device-rejected batch is a hard
+    /// failure, not a <see cref="HandshakeResult"/> the script could mistake for a timeout.
+    /// </summary>
+    [Fact]
+    public async Task BatchFailureAfterASuccessfulReset_StillThrows()
+    {
+        var probe = CreateTestProbe();
+        var ctx = CreateContext(probe.Ref);
+
+        var call = ctx.WriteBatchResetAndWait(
+            Values, "MoveOutFlag", "True", "MoveOutCompleteFlag", "True", TimeSpan.FromSeconds(25),
+            "MoveOutCompleteFlag", "False", TimeSpan.FromSeconds(10));
+
+        var reset = probe.ExpectMsg<SetStaticAttributeCommand>(TimeSpan.FromSeconds(5));
+        probe.Reply(new SetStaticAttributeResponse(reset.CorrelationId, Instance, "MoveOutCompleteFlag", true, null, DateTimeOffset.UtcNow));
+        var resetWait = probe.ExpectMsg<WaitForAttributeRequest>(TimeSpan.FromSeconds(5));
+        probe.Reply(new WaitForAttributeResponse(resetWait.CorrelationId, Matched: true, false, "Good", TimedOut: false));
+        var batch = probe.ExpectMsg<WriteAttributeBatchRequest>(TimeSpan.FromSeconds(5));
+        probe.Reply(new WriteAttributeBatchResponse(batch.CorrelationId, false, "gateway rejected"));
+
+        var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => call);
+        Assert.Contains("gateway rejected", ex.Message);
+        Assert.Contains("Batch write failed", ex.Message);
     }
 
     [Fact]