fix: self-review findings from Phase 10 live-scenario branch (test scope claims, naming)

- ResolveAsync concurrency test renamed/documented to claim only what it actually proves
  (single-process lock serialization), not cross-process orphan reconciliation.
- Cross-contamination and Unicode round-trip tests now restrict matches to the RecentMessage
  tier specifically -- NAMS entity search is workspace-wide, not conversation-scoped, so checking
  all items made both tests noisier and their claims broader than what NAMS actually guarantees.
- Naming brought in line with the file's existing "Eventually..." convention for eventually-
  consistent assertions; added a comment on the cancellation test's timing assumption.

Re-verified: 7/7 live tests still green against the real NAMS SaaS after these fixes.

Author: joslat

docs/reviews/NAMS_Phase10_LiveScenarioExpansion_PlanningAndImplementationPlan.md

@@ -45,7 +45,36 @@ real NAMS SaaS (`agent-memory-dotnet-dev` workspace):
   MAF-layer mapping/gating logic (`NamsMafTypeMapper`, Phase 6), already unit-tested there; they're not
   live-NAMS-specific behavior to re-verify against the real service.
 
-## 3. Explicitly deferred
+## 3. Self-review findings and fixes
+
+Self-review (2 angles, given the smaller diff: correctness + combined cross-file/conventions) found the
+test *logic*, not just the code, needed tightening -- a live test passing once against an eventually
+consistent external service doesn't by itself prove the logic is sound:
+
+- **`ResolveAsync_SameIdentityResolvedConcurrently_...`**: traced the actual execution and confirmed this
+  single-process test can only ever exercise `KeyedAsyncLock` serialization + the already-populated-store
+  fast path -- it structurally cannot reach the cross-process "lost the race, reconcile an orphaned
+  conversation" branch (that needs two separate resolver instances racing a shared store, already listed
+  under "Multi-instance mapping" in §3). Renamed `...ReconcilesToOneConversation` ->
+  `...SerializesToOneConversation` and added a doc comment stating exactly what this does and doesn't prove.
+- **Cross-contamination test**: `NamsRecallOptions.IncludeEntitySearch` defaults `true` and wasn't
+  overridden, so the original assertion checked ALL recalled items -- including entities, which are
+  workspace-wide, not conversation-scoped (per the `NamsAgent` sample's own README). That made the test's
+  actual scope wider than its documented intent and a source of unrelated flakiness. Fixed to filter to
+  `NamsRecallCategory.RecentMessage` only, matching the documented scope exactly. Renamed to
+  `...EventuallyRecallOnlyTheirOwnRecentMessage` (also fixes a naming-convention gap the other review angle
+  found: new eventually-consistent tests should say "Eventually", matching the file's existing two).
+- **Unicode round-trip test**: "byte-for-byte" was only actually guaranteed for the `RecentMessage` tier
+  (`NamsRecallService.MapMessage` passes `Content` through verbatim); reflections/observations are
+  NAMS-synthesized text with no such guarantee. Fixed the match condition to require
+  `Category == NamsRecallCategory.RecentMessage`, making the claim proven rather than probabilistic.
+  Renamed to `...EventuallyRoundTripsByteForByteInRecentMessages`.
+- **Cancellation test**: added a one-line comment stating the `CancelAfter(1ms)` timing is an environment
+  assumption (this session's observed 400-1000ms live NAMS latency), not a language/runtime guarantee.
+
+Re-ran all 7 live tests after the fixes: still 7/7 green, ~7s.
+
+## 4. Explicitly deferred
 
 - **Multi-instance mapping** (separate resolver processes, crash reconciliation) -- needs actual
   multi-process orchestration, a bigger investment than fits this increment.
@@ -58,16 +87,17 @@ real NAMS SaaS (`agent-memory-dotnet-dev` workspace):
 - **Payload: empty / max size / multi-message / non-text** -- Unicode was judged the highest-value single
   addition for this pass; the rest are reasonable follow-ups, not done here.
 
-## 4. Verification
+## 5. Verification
 
 - `dotnet build AgentMemory.slnx -c Release` -- 0 warnings, 0 errors.
 - `dotnet test tests/AgentMemory.Tests.Integration --filter "FullyQualifiedName~Nams.NamsLiveConnectivityTests"`
   -- **7/7 live, ~7s total** (3 original + 4 new).
 - `dotnet test tests/AgentMemory.Tests.Unit` -- full suite green, unaffected (no unit-level changes this
   phase).
 
-## 5. Definition of done
+## 6. Definition of done
 
 - [x] 4 new live scenarios added and passing against the real NAMS SaaS.
 - [x] Coverage overlap and deferred scope explicitly documented (no silent gaps).
-- [ ] Self-reviewed, PR opened, CI green (live tests report Skipped there, as established), merged to `main`.
+- [x] Self-reviewed and fixes applied.
+- [ ] PR opened, CI green (live tests report Skipped there, as established), merged to `main`.

tests/AgentMemory.Tests.Integration/Nams/NamsLiveConnectivityTests.cs

@@ -98,8 +98,16 @@ await persistence.PersistTurnAsync(
         found.Should().BeTrue($"an entity named '{entityName}' should eventually be extracted and recallable");
     }
 
+    /// <summary>
+    /// Proves the single-process path only: the shared <c>KeyedAsyncLock</c> serializes the second call
+    /// behind the first, which then takes the already-populated-store fast path -- it never reaches
+    /// <c>CreateConversationAsync</c> itself. This does NOT exercise the cross-process "lost the race,
+    /// reconcile an orphaned conversation" branch (that needs two separate resolver instances/processes
+    /// racing against a genuinely shared store -- out of scope here, see the Phase 10 planning doc's
+    /// "Multi-instance mapping" deferral).
+    /// </summary>
     [LiveNamsFact]
-    public async Task ResolveAsync_SameIdentityResolvedConcurrently_ReconcilesToOneConversation()
+    public async Task ResolveAsync_SameIdentityResolvedConcurrently_SerializesToOneConversation()
     {
         var resolver = _fixture.Services!.GetRequiredService<INamsConversationResolver>();
         var identity = UniqueIdentity(); // one identity, resolved twice concurrently below
@@ -114,8 +122,14 @@ public async Task ResolveAsync_SameIdentityResolvedConcurrently_ReconcilesToOneC
             "exactly one of the two concurrent resolutions should have actually created the conversation");
     }
 
+    /// <summary>
+    /// Deliberately scoped to the <see cref="NamsRecallCategory.RecentMessage"/> tier only (conversation-
+    /// scoped) -- NAMS entity search is workspace-wide, not conversation-scoped (documented in
+    /// <c>AgentMemory.Sample.NamsAgent</c>'s README), so including entity-tier items here would make this
+    /// test noisy/flaky for a reason unrelated to what it's actually verifying.
+    /// </summary>
     [LiveNamsFact]
-    public async Task PersistTurnAsync_TwoConcurrentUsers_DoNotCrossContaminateChatHistory()
+    public async Task PersistTurnAsync_TwoConcurrentUsers_EventuallyRecallOnlyTheirOwnRecentMessage()
     {
         var services = _fixture.Services!;
         var resolver = services.GetRequiredService<INamsConversationResolver>();
@@ -139,16 +153,18 @@ await Task.WhenAll(
             {
                 var recalledA = await recall.RecallAsync(conversationA.NamsConversationId, markerA, CancellationToken.None);
                 var recalledB = await recall.RecallAsync(conversationB.NamsConversationId, markerB, CancellationToken.None);
-                var aHasOwnMarker = recalledA.Items.Any(i => i.Content.Contains(markerA, StringComparison.Ordinal));
-                var bHasOwnMarker = recalledB.Items.Any(i => i.Content.Contains(markerB, StringComparison.Ordinal));
+                var recentA = recalledA.Items.Where(i => i.Category == NamsRecallCategory.RecentMessage).ToList();
+                var recentB = recalledB.Items.Where(i => i.Category == NamsRecallCategory.RecentMessage).ToList();
+                var aHasOwnMarker = recentA.Any(i => i.Content.Contains(markerA, StringComparison.Ordinal));
+                var bHasOwnMarker = recentB.Any(i => i.Content.Contains(markerB, StringComparison.Ordinal));
                 var noCrossContamination =
-                    !recalledA.Items.Any(i => i.Content.Contains(markerB, StringComparison.Ordinal))
-                    && !recalledB.Items.Any(i => i.Content.Contains(markerA, StringComparison.Ordinal));
+                    !recentA.Any(i => i.Content.Contains(markerB, StringComparison.Ordinal))
+                    && !recentB.Any(i => i.Content.Contains(markerA, StringComparison.Ordinal));
                 return aHasOwnMarker && bHasOwnMarker && noCrossContamination;
             },
             timeout: TimeSpan.FromSeconds(30));
 
-        found.Should().BeTrue("each conversation's recall should see only its own persisted message, never the other's");
+        found.Should().BeTrue("each conversation's recent-message tier should show only its own persisted message, never the other's");
     }
 
     [LiveNamsFact]
@@ -160,15 +176,25 @@ public async Task RecallAsync_CancelledBeforeTheHttpRoundTripCompletes_Propagate
         var conversation = await resolver.ResolveAsync(UniqueIdentity(), CancellationToken.None);
 
         using var cts = new CancellationTokenSource();
-        cts.CancelAfter(TimeSpan.FromMilliseconds(1)); // a live round trip to NAMS never completes this fast
+        // Environment assumption, not a language/runtime guarantee: this session's observed live NAMS
+        // latency is consistently 400-1000ms, so 1ms reliably cancels first. If NAMS round-trips ever get
+        // fast enough to race this, this test (not the production code) would need a longer delay.
+        cts.CancelAfter(TimeSpan.FromMilliseconds(1));
 
         var act = () => recall.RecallAsync(conversation.NamsConversationId, "test", cts.Token);
 
         await act.Should().ThrowAsync<OperationCanceledException>();
     }
 
+    /// <summary>
+    /// Matches only <see cref="NamsRecallCategory.RecentMessage"/> items -- <see cref="MapMessage"/>-mapped
+    /// (in <c>NamsRecallService</c>) recent messages pass <c>Content</c> through verbatim, so a match there
+    /// genuinely proves a byte-for-byte round trip. Reflections/observations are NAMS-synthesized text, not
+    /// guaranteed to preserve the original bytes -- matching against those tiers too would make "byte for
+    /// byte" a probabilistic claim rather than a proven one.
+    /// </summary>
     [LiveNamsFact]
-    public async Task PersistTurnAsync_UnicodeAndEmojiContent_RoundTripsByteForByte()
+    public async Task PersistTurnAsync_UnicodeAndEmojiContent_EventuallyRoundTripsByteForByteInRecentMessages()
     {
         var services = _fixture.Services!;
         var resolver = services.GetRequiredService<INamsConversationResolver>();
@@ -184,11 +210,12 @@ await persistence.PersistTurnAsync(
             async () =>
             {
                 var recalled = await recall.RecallAsync(conversation.NamsConversationId, marker, CancellationToken.None);
-                return recalled.Items.Any(i => i.Content.Contains(marker, StringComparison.Ordinal));
+                return recalled.Items.Any(i =>
+                    i.Category == NamsRecallCategory.RecentMessage && i.Content.Contains(marker, StringComparison.Ordinal));
             },
             timeout: TimeSpan.FromSeconds(30));
 
-        found.Should().BeTrue("Unicode/emoji content should round-trip through NAMS byte-for-byte");
+        found.Should().BeTrue("Unicode/emoji content should round-trip through NAMS byte-for-byte in the recent-messages tier");
     }
 
     private static NamsConversationIdentity UniqueIdentity([CallerMemberName] string testName = "") => new()