REVIEW REMEDIATE: address Phase 14 findings (13 A's + 2 B's)

22 findings classified 13A/2B/0C/7D from the Phase 14 review pass; all
actionable items folded into one commit. 7 D's preserved in review.md
\"Action Taken\" column with rationale.

Production code:
- src/NexNet.Testing/Recording/MethodIdMap.cs - doc-comment now points at
  the post-rename regression test (#1)

Test-only nexus (src/NexNet.Testing.Tests/EditorAppNexus.cs):
- ListActiveEditors Task.Delay now guarded on CanBeCanceled - happy-path
  callers passing CancellationToken.None no longer pay the 200ms tax (#5)
- OnAuthorize override comment now documents AND-semantics, marker-only
  behavior, and per-app policy choice rationale (#17)
- ResetAll() XML comment explains why PingCount is omitted (instance-scoped)
  and what enabling Parallelizable would race (#10, #21)
- Removed redundant 'using NexNet;' (#20)

Test files:
- AssertionTests + NexusTestHostTests gained [SetUp] ResetAll() for
  fixture-convention consistency (#9)
- EditorAppShowcaseTests:
  - CT-propagation test loosened to Throws.InstanceOf<OperationCanceledException>
    so the test pins the semantic, not the concrete subtype (#7)
  - WaitFor_DraftSaved_ResolvesWhenInvoked - producer now runs in Task.Run
    with 100ms delay so the waiter actually exercises the async-arrival
    path (#2)
  - MixedTraffic - pipe + channel now 'await using' for failure-path
    safety; assertions tightened with times: 1; added AssertNotReceived
    on alice + carol for WhisperReceived (#3, #13)
  - StreamEdits_AllOpsCollected - channel now 'await using' (#14)
  - New: OpenDocument_NotifiesOthersExceptCaller - pins the EditorJoined
    GroupExceptCaller broadcast that fires inside OpenDocument (#4)
  - New: Connect_WithoutIdentity_IsRejectedAtHandshake - pins the discovery
    that anonymous sessions are rejected at handshake by the framework
    (TransportException Authentication failed), not by OnAuthorize (#18)
  - Stale '50ms' comment updated to reflect the 200ms delay + CanBeCanceled
    guard (#6)

82/82 NexNet.Testing.Tests pass (+2 new tests vs pre-remediation 80).

Author: DJGosnell

_sessions/add-nexnet-testing/review.md

@@ -6,7 +6,7 @@ remote: https://github.com/Dtronix/NexNet.git
 base-branch: master
 
 ## State
-phase: REVIEW
+phase: REMEDIATE
 status: active
 issue: discussion
 pr: 77
@@ -197,3 +197,5 @@ Quiescence is the load-bearing primitive that makes negative assertions (`Assert
 | 7 | 2026-05-27 IMPLEMENT | 2026-05-27 IMPLEMENT | Phase 14c complete: replaced HarnessShowcaseTests.cs with EditorAppShowcaseTests.cs (10 tests: SaveDraft broadcast + identity, LeaveDocument GroupExceptCaller, Whisper Client-by-id, BroadcastSystemAnnouncement AsNonAdmin throws / AsAdmin all-broadcast, SaveDraft AsReader throws, ListActiveEditors returns names, ListActiveEditors cancelled-token throws, UploadAttachment byte streaming, StreamEdits channel typed streaming) plus migrated Groups_EmptyGroup_HasNoMembers. **Behavior discovery:** the framework does NOT short-circuit pre-cancelled tokens — the cancel signal is only sent when the client-side CT FIRES during the call. ListActiveEditors gained an `await Task.Delay(200, ct)` so client-side CT cancellation has time to propagate to the server-side CT. 73/73 tests pass. |
 | 7 | 2026-05-27 IMPLEMENT | 2026-05-27 IMPLEMENT | Phase 14d complete: added 7 more showcase tests (test 15 Groups_EmptyGroup_HasNoMembers was migrated in 14c). New: WaitFor_DraftSaved_ResolvesWhenInvoked, WaitFor_Timeout_Throws, MixedTraffic_QuiesceAsync_WaitsForEverything (4-shape concurrent traffic — SaveDraft + Whisper + UploadAttachment + StreamEdits across 3 clients), Groups_Introspection_ReflectsLiveMembership (open + leave + reopen), ServerSide_AssertReceived_SaveDraft, AssertReceived_TimesMismatch_DiagnosticIncludesArgs (showcase shape of the AssertionTests version), ArgMatchers_AnyAndPredicate (Arg.Any + Arg.Is). 80/80 tests pass. |
 | 7 | 2026-05-27 IMPLEMENT | 2026-05-27 IMPLEMENT | Phase 14e complete: pr-body.md sample rewritten from the old JoinGroup/BroadcastToGroup snippet to the new editor-app shape (test #1: OpenDocument + SaveDraft + DraftSaved callback observation across 3 identities). Added a Phase 14 entry to the "Plan items implemented" section; bumped test count from 65 → 80; revised summary to mention the demo rewrite. PR body pushed to GitHub (PR #77). No NexNet.Testing README exists and the main README + docs articles do not mention the testing harness, so nothing else to update. Phase 14 (all 5 sub-phases) complete. Transitioning to REVIEW. |
+| 7 | 2026-05-27 REVIEW | 2026-05-27 REMEDIATE | REVIEW analysis pass on the Phase 14 diff (commits aaa67c7..28921e3) via general-purpose agent. 22 distinct findings after consolidating 5 overlap pairs. Recs: 13A / 2B / 0C / 7D. User accepted all as recommended. Classifications + per-finding "Action Taken" in `review.md`. **Behavior discovery during finding #18:** anonymous (null-identity) sessions are rejected at the handshake by the framework (NexusSession.Receiving returns DisconnectReason.Authentication when Authenticate returns null), so the OnAuthorize `is not TestIdentity` null-identity guard is dead code via this harness's auth path. Documented in test body + Action Taken. |
+| 7 | 2026-05-27 REMEDIATE | 2026-05-27 REMEDIATE | All 13 A's + 2 B's addressed in one commit. Key fixes: ListActiveEditors Task.Delay now guarded on `CanBeCanceled` (test #5 — happy-path callers no longer pay 200ms tax); OnAuthorize AND-semantics + marker-only behavior documented inline (#17); ResetAll comment now explains PingCount omission and the sequential-fixture-execution requirement (#10, #21); `[SetUp]` ResetAll() added to AssertionTests + NexusTestHostTests (#9); CT-propagation test loosened to `Throws.InstanceOf<OperationCanceledException>` (#7); MixedTraffic gained pipe/channel `await using` + `times: 1` + negative WhisperReceived assertions (#3, #13, #14); WaitFor producer now runs in background with 100ms delay (#2); new tests OpenDocument_NotifiesOthersExceptCaller (#4) and Connect_WithoutIdentity_IsRejectedAtHandshake (#18). MethodIdMap doc-comment fixed (#1); stale "50ms" comment updated (#6); `using NexNet;` removed (#20). 82/82 NexNet.Testing.Tests pass (+2 new). |

_sessions/add-nexnet-testing/workflow.md

@@ -7,6 +7,9 @@ namespace NexNet.Testing.Tests;
 
 internal class AssertionTests
 {
+    [SetUp]
+    public void ResetEditorState() => EditorServerNexus.ResetAll();
+
     private async Task<(NexusTestHost<EditorServerNexus, EditorServerNexus.ClientProxy, EditorClientNexus, EditorClientNexus.ServerProxy> host,
             NexusTestClient<EditorClientNexus, EditorClientNexus.ServerProxy> client,
             EditorServerNexus server)>

src/NexNet.Testing.Tests/AssertionTests.cs

@@ -6,7 +6,6 @@
 using System.Threading;
 using System.Threading.Tasks;
 using MemoryPack;
-using NexNet;
 using NexNet.Invocation;
 using NexNet.Pipes;
 
@@ -39,6 +38,13 @@ internal partial class EditorServerNexus : ServerNexusBase<EditorServerNexus.Cli
     public static byte[]? LastUploadedBytes;
     public static List<string>? LastCollectedItems;
 
+    // Resets cross-fixture process-global state. PingCount is intentionally NOT touched —
+    // it is per-instance (each test gets its own EditorServerNexus via the factory) and
+    // doesn't bleed across tests. The Documents / ActiveEditors / LastUploadedBytes /
+    // LastCollectedItems statics persist across nexus instances and MUST be cleared by
+    // every test fixture that exercises these fields, regardless of whether the test
+    // reads them directly. Tests assume sequential fixture execution; enabling
+    // [assembly: Parallelizable(ParallelScope.Fixtures)] would race this state.
     public static void ResetAll()
     {
         Documents.Clear();
@@ -134,12 +140,14 @@ public async ValueTask BroadcastSystemAnnouncement(string message)
 
     public async ValueTask<string[]> ListActiveEditors(string docId, CancellationToken cancellationToken)
     {
-        // Observable async point so callers passing a CancellationToken can actually observe
-        // cancellation. The framework only sends a cancel signal when the client-side CT
-        // FIRES during the call (pre-cancelled tokens are not short-circuited by the proxy),
-        // so the server needs an awaiting point long enough for that signal to arrive. 200ms
-        // gives ample headroom for in-process propagation.
-        await Task.Delay(200, cancellationToken);
+        // Observable async point so callers passing a cancellable CancellationToken can
+        // actually observe cancellation. The framework only sends a cancel signal when the
+        // client-side CT FIRES during the call (pre-cancelled tokens are not short-circuited
+        // by the proxy), so the server needs an awaiting point long enough for that signal
+        // to arrive. Guarded on CanBeCanceled so happy-path callers (CancellationToken.None)
+        // don't pay the latency tax.
+        if (cancellationToken.CanBeCanceled)
+            await Task.Delay(200, cancellationToken);
         if (!ActiveEditors.TryGetValue(docId, out var registry))
             return Array.Empty<string>();
         return registry.Values.ToArray();
@@ -168,6 +176,13 @@ public async ValueTask StreamEdits(string docId, INexusDuplexChannel<EditOp> cha
             doc.Edits.Enqueue(op);
     }
 
+    // Policy: AND-semantics over requiredPermissions — every declared permission must be
+    // present on the identity, otherwise Unauthorized. This is a per-app choice, not a
+    // framework default — the framework hands you the int[] of required permissions and
+    // OnAuthorize decides how to interpret them. An OR-semantics policy would just flip the
+    // loop to "any match → Allowed". Marker-only [NexusAuthorize<TPermission>()] (empty
+    // requiredPermissions) is currently treated as "TestIdentity sufficient" since the for
+    // loop is skipped entirely.
     protected override ValueTask<AuthorizeResult> OnAuthorize(
         ServerSessionContext<EditorServerNexus.ClientProxy> context,
         int methodId,

src/NexNet.Testing.Tests/EditorAppNexus.cs

@@ -151,17 +151,21 @@ public async Task ListActiveEditors_ReturnsNames()
     [Test]
     public async Task ListActiveEditors_CancelledToken_PropagatesAndThrows()
     {
-        // Client-side CT fires partway through the server's `await Task.Delay(50, ct)` in
-        // ListActiveEditors. Framework propagates the cancel signal to the server-side CT,
+        // Client-side CT fires partway through the server's `await Task.Delay(200, ct)` in
+        // ListActiveEditors (the delay is guarded on CanBeCanceled, so only cancellable
+        // callers pay it). Framework propagates the cancel signal to the server-side CT,
         // which causes the delay to throw. Pre-cancelled tokens are not short-circuited by
-        // the proxy — the cancel signal is only sent when the client-side CT FIRES.
+        // the proxy — the cancel signal is only sent when the client-side CT FIRES. The
+        // assertion uses OperationCanceledException (the base) rather than TaskCanceledException
+        // so the test pins the semantic, not the concrete subtype.
         await using var host = await CreateHost();
         var alice = await host.ConnectAsAsync(TestIdentity.Of("alice"));
 
         using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(10));
 
-        Assert.ThrowsAsync<TaskCanceledException>(
-            async () => await alice.Server.ListActiveEditors("design.md", cts.Token));
+        Assert.That(
+            async () => await alice.Server.ListActiveEditors("design.md", cts.Token),
+            Throws.InstanceOf<OperationCanceledException>());
     }
 
     [Test]
@@ -193,7 +197,7 @@ public async Task StreamEdits_AllOpsCollected()
             .Select(i => new EditOp(i, $"text-{i}"))
             .ToArray();
 
-        var channel = alice.Nexus.Context.CreateChannel<EditOp>();
+        await using var channel = alice.Nexus.Context.CreateChannel<EditOp>();
         var serverCall = alice.Server.StreamEdits("design.md", channel).AsTask();
         var writer = await channel.GetWriterAsync();
         foreach (var op in ops)
@@ -215,11 +219,19 @@ public async Task WaitFor_DraftSaved_ResolvesWhenInvoked()
 
         await alice.Server.OpenDocument("design.md");
         await bob.Server.OpenDocument("design.md");
+        await host.QuiesceAsync().WaitAsync(TimeSpan.FromSeconds(2));
 
+        // Producer runs in the background AFTER a small delay so the waiter actually
+        // observes the async-arrival path, not the "already recorded" fast path.
         var waiter = alice.WaitFor<IEditorClientNexus>(
             n => n.DraftSaved("bob", "v1"), TimeSpan.FromSeconds(3));
-        await bob.Server.SaveDraft("design.md", "v1");
+        var producer = Task.Run(async () =>
+        {
+            await Task.Delay(100);
+            await bob.Server.SaveDraft("design.md", "v1");
+        });
         await waiter;
+        await producer;
     }
 
     [Test]
@@ -252,14 +264,15 @@ public async Task MixedTraffic_QuiesceAsync_WaitsForEverything()
         var ops = Enumerable.Range(0, 5).Select(i => new EditOp(i, $"o{i}")).ToArray();
 
         // Fire all four traffic shapes in flight at once.
+        await using var pipe = alice.CreatePipe();
+        await using var channel = bob.Nexus.Context.CreateChannel<EditOp>();
+
         var draftCall = alice.Server.SaveDraft("design.md", "draft-a").AsTask();
         var whisperCall = carol.Server.Whisper(bob.Nexus.Context.Id, "ping").AsTask();
 
-        var pipe = alice.CreatePipe();
         var uploadCall = alice.Server.UploadAttachment("design.md", pipe).AsTask();
         var uploadDrive = pipe.PipeUploadAsync(attachment).AsTask();
 
-        var channel = bob.Nexus.Context.CreateChannel<EditOp>();
         var streamCall = bob.Server.StreamEdits("design.md", channel).AsTask();
         var streamDrive = Task.Run(async () =>
         {
@@ -270,18 +283,65 @@ public async Task MixedTraffic_QuiesceAsync_WaitsForEverything()
 
         await Task.WhenAll(draftCall, whisperCall, uploadDrive, uploadCall, streamDrive, streamCall)
             .WaitAsync(TimeSpan.FromSeconds(5));
-        await pipe.DisposeAsync();
         await host.QuiesceAsync().WaitAsync(TimeSpan.FromSeconds(5));
 
-        // Every callback delivered, every server-side append visible — no extra waits needed.
-        alice.AssertReceived<IEditorClientNexus>(n => n.DraftSaved("alice", "draft-a"));
-        bob.AssertReceived<IEditorClientNexus>(n => n.DraftSaved("alice", "draft-a"));
-        carol.AssertReceived<IEditorClientNexus>(n => n.DraftSaved("alice", "draft-a"));
-        bob.AssertReceived<IEditorClientNexus>(n => n.WhisperReceived("carol", "ping"));
+        // Every callback delivered exactly once on the expected recipients; non-recipients
+        // never see the per-target whisper.
+        alice.AssertReceived<IEditorClientNexus>(n => n.DraftSaved("alice", "draft-a"), times: 1);
+        bob.AssertReceived<IEditorClientNexus>(n => n.DraftSaved("alice", "draft-a"), times: 1);
+        carol.AssertReceived<IEditorClientNexus>(n => n.DraftSaved("alice", "draft-a"), times: 1);
+        bob.AssertReceived<IEditorClientNexus>(n => n.WhisperReceived("carol", "ping"), times: 1);
+        alice.AssertNotReceived<IEditorClientNexus>(
+            n => n.WhisperReceived(Arg.Any<string>(), Arg.Any<string>()));
+        carol.AssertNotReceived<IEditorClientNexus>(
+            n => n.WhisperReceived(Arg.Any<string>(), Arg.Any<string>()));
         Assert.That(EditorServerNexus.Documents["design.md"].Attachment, Is.EqualTo(attachment));
         Assert.That(EditorServerNexus.Documents["design.md"].Edits.ToArray(), Is.EqualTo(ops));
     }
 
+    [Test]
+    public async Task OpenDocument_NotifiesOthersExceptCaller()
+    {
+        // Pairs with LeaveDocument_NotifiesOthersExceptCaller — exercises the EditorJoined
+        // GroupExceptCaller broadcast that fires inside OpenDocument.
+        await using var host = await CreateHost();
+        var alice = await host.ConnectAsAsync(TestIdentity.Of("alice"));
+        var bob = await host.ConnectAsAsync(TestIdentity.Of("bob"));
+        var carol = await host.ConnectAsAsync(TestIdentity.Of("carol"));
+
+        await alice.Server.OpenDocument("design.md");
+        await bob.Server.OpenDocument("design.md");
+        await host.QuiesceAsync().WaitAsync(TimeSpan.FromSeconds(2));
+
+        // Carol joins last → alice + bob receive EditorJoined("carol"); carol does not see
+        // her own join. Alice never sees her own join either (first joiner, group empty at
+        // broadcast time).
+        await carol.Server.OpenDocument("design.md");
+        await host.QuiesceAsync().WaitAsync(TimeSpan.FromSeconds(2));
+
+        alice.AssertReceived<IEditorClientNexus>(n => n.EditorJoined("carol"), times: 1);
+        bob.AssertReceived<IEditorClientNexus>(n => n.EditorJoined("carol"), times: 1);
+        carol.AssertNotReceived<IEditorClientNexus>(n => n.EditorJoined("carol"));
+        alice.AssertNotReceived<IEditorClientNexus>(n => n.EditorJoined("alice"));
+    }
+
+    [Test]
+    public async Task Connect_WithoutIdentity_IsRejectedAtHandshake()
+    {
+        // The anonymous-call-to-gated-method path can't be exercised directly because the
+        // framework rejects null-identity sessions at the handshake (NexusSession.Receiving
+        // returns DisconnectReason.Authentication when Authenticate returns null). The
+        // TestAuthenticationStore-backed override returns null for a missing token, so
+        // ConnectAsync() (no identity) never completes a usable session and the OnAuthorize
+        // override's `is not TestIdentity` guard isn't reachable via this harness's auth path.
+        await using var host = await CreateHost();
+
+        Assert.That(
+            async () => await host.ConnectAsync(),
+            Throws.InstanceOf<NexNet.Transports.TransportException>()
+                .With.Message.Contains("Authentication"));
+    }
+
     [Test]
     public async Task Groups_Introspection_ReflectsLiveMembership()
     {

src/NexNet.Testing.Tests/EditorAppShowcaseTests.cs

@@ -7,6 +7,9 @@ namespace NexNet.Testing.Tests;
 
 internal class NexusTestHostTests
 {
+    [SetUp]
+    public void ResetEditorState() => EditorServerNexus.ResetAll();
+
     [Test]
     public async Task EndToEnd_InvokeServerMethodOverInProcess()
     {

src/NexNet.Testing.Tests/NexusTestHostTests.cs

@@ -18,8 +18,8 @@ namespace NexNet.Testing.Recording;
 /// AOT/trimming caveat: metadata-token order remains stable under standard runtimes today; if a
 /// future host re-orders <c>GetMethods()</c> output, assertions will surface a clear "method not
 /// recorded" failure rather than silently match the wrong method. See
-/// <c>MethodIdMapTests.GeneratorParity_DemoInterfaces</c> for a regression test that locks in the
-/// expected layout.
+/// <c>MethodIdMapTests.GeneratorParity_EditorServerInterface_AssignsExpectedIds</c> (and the
+/// matching client-interface test) for the regression tests that lock in the expected layout.
 /// </remarks>
 internal static class MethodIdMap
 {