|
| 1 | +# NAMS Phase 10d — Session-Restore Asserted Live Test — Planning and Implementation Plan |
| 2 | + |
| 3 | +**Prepared:** 2026-07-18 |
| 4 | +**Branch:** `nams/phase10d-session-restore-test` |
| 5 | +**Purpose:** Fourth of five follow-up Phase 10 increments (10a-10d + TCK Platinum research). |
| 6 | + |
| 7 | +## 1. What this formalizes |
| 8 | + |
| 9 | +`AgentMemory.Sample.NamsAgent`'s console demo shows session serialize/restore working (step 2 of its script: |
| 10 | +`SerializeSessionAsync` → `DeserializeSessionAsync` → re-apply `.WithMemoryIdentity(...)` → the agent still |
| 11 | +answers "what did I tell you?" correctly) -- but only as printed console output a human has to eyeball. No |
| 12 | +asserted test exercised this. This phase builds a minimal, real (not scripted-stub) MAF-agent harness and |
| 13 | +turns it into two asserted live tests. |
| 14 | + |
| 15 | +## 2. Design decisions |
| 16 | + |
| 17 | +**Real `ChatClientAgent`, stubbed model only.** Unlike `MemoryOwnerScopingAgentIntegrationTests`' |
| 18 | +`ScriptedInnerAgent` (whose `SerializeSessionCoreAsync`/`DeserializeSessionCoreAsync` are trivial stubs |
| 19 | +returning `default(JsonElement)`), this phase needs MAF's REAL session-serialization machinery, since that is |
| 20 | +exactly what's under test. So: a real `ChatClientAgent` built via `stubChatClient.AsAIAgent(...)` (matching |
| 21 | +the sample exactly), with only the model call stubbed via an NSubstitute `IChatClient` returning a canned |
| 22 | +response -- no real LLM dependency, but genuine `SerializeSessionAsync`/`DeserializeSessionAsync` behavior. |
| 23 | + |
| 24 | +**DI setup extended, not duplicated.** `NamsLiveFixture` previously only wired `AddNamsAgentMemory`. Added |
| 25 | +`AddAgentMemoryFramework()` + `AddNamsAgentMemoryFramework()` (purely additive registrations -- new |
| 26 | +`AgentFrameworkOptions`/`ContextFormatOptions` options plus a new scoped `NamsMemoryContextProvider` -- no |
| 27 | +change to anything the other 15 NAMS live tests already resolve) rather than standing up a second, duplicate |
| 28 | +DI container inside this new test file. Also added the `Microsoft.Agents.AI` package reference (the concrete |
| 29 | +package with `ChatClientAgent`/`AsAIAgent`, vs. the `.Abstractions`-only reference the project already had) |
| 30 | +and a project reference to `AgentMemory.AgentFramework.Nams`. |
| 31 | + |
| 32 | +**Empirically probed MAF's real serialized-session JSON before writing any assertion** (per this whole Phase |
| 33 | +10 push's discipline). A throwaway test dumped `(await agent.SerializeSessionAsync(session)).GetRawText()` |
| 34 | +after calling `.WithMemoryIdentity(...)` and running one turn: |
| 35 | + |
| 36 | +``` |
| 37 | +{"stateBag":{"user_id":"...","conversation_id":"...","InMemoryChatHistoryProvider":{"messages":[...]}, |
| 38 | +"application_id":"...","session_id":"..."}} |
| 39 | +``` |
| 40 | + |
| 41 | +Two things this revealed, both shaping the final test design: |
| 42 | + |
| 43 | +1. **The memory-identity state bag (`user_id`/`session_id`/`conversation_id`/`application_id`) is already |
| 44 | + embedded in `ChatClientAgent`'s own serialized JSON.** This means the sample's re-application of |
| 45 | + `.WithMemoryIdentity(...)` after `DeserializeSessionAsync` is likely defensive, not strictly load-bearing -- |
| 46 | + a genuinely surprising, non-obvious finding worth its own test (see Test 2 below) rather than just assuming |
| 47 | + the sample's recipe is the only way it works. |
| 48 | +2. **`ChatClientAgent` also carries its OWN turn history** (`InMemoryChatHistoryProvider`) inside the same |
| 49 | + state bag. This ruled out the originally-planned assertion strategy of inspecting the stub `IChatClient`'s |
| 50 | + received messages for the pre-restore marker text: a restored session's next turn would include that |
| 51 | + marker via MAF's own built-in history regardless of whether NAMS recall did anything, so finding it there |
| 52 | + would prove nothing about NAMS specifically. A `<recalled_memory category="...">`-wrapper search (the |
| 53 | + sample's own `MemoryTraceChatClient` approach) was considered and also ruled out: per |
| 54 | + `NamsMafTypeMapper.ToContextMessages`, the `RecentMessage`/`RelevantMessage` categories a plain persisted |
| 55 | + turn recalls under are deliberately left undelimited ("a recalled message renders as an individual |
| 56 | + conversation turn, not an injected block"), so a plain marker message carries no distinguishing wrapper to |
| 57 | + search for either. |
| 58 | + |
| 59 | +**Final assertion strategy: direct, live NAMS reads outside the agent pipeline.** Each test resolves the |
| 60 | +underlying `NamsConversationId` once (via `INamsConversationResolver`, from the same DI scope) purely to know |
| 61 | +which conversation to poll -- not as the thing being proven, since a second direct resolver call with the |
| 62 | +same identity values would trivially agree regardless of whether restore worked (the shared singleton |
| 63 | +`INamsConversationStateStore` already remembers the mapping from the first call). The actual proof is |
| 64 | +that content persisted through the RESTORED session's own real `agent.RunAsync(...)` call -- which internally |
| 65 | +extracts identity from the restored session's own post-deserialize state and persists through it -- lands in |
| 66 | +that SAME conversation, confirmed via a bounded `PollUntilAsync` against `INamsRecallService.RecallAsync` |
| 67 | +called independently of the agent pipeline. |
| 68 | + |
| 69 | +## 3. What was added |
| 70 | + |
| 71 | +New file `tests/AgentMemory.Tests.Integration/Nams/NamsSessionRestoreTests.cs`: |
| 72 | + |
| 73 | +- `SessionRestore_WithMemoryIdentityReapplied_ContinuesPersistingToTheSameNamsConversation` -- the documented |
| 74 | + recipe: run a turn, serialize, deserialize, re-apply `.WithMemoryIdentity(...)` (matching the sample |
| 75 | + exactly), run a second turn, and confirm (via direct NAMS recall) that it persisted to the same conversation |
| 76 | + the first turn did. |
| 77 | +- `SessionRestore_WithoutReapplyingMemoryIdentity_StillPersistsToTheSameNamsConversation` -- identical, but |
| 78 | + deliberately skips re-applying `.WithMemoryIdentity(...)` after restore, proving the empirically-discovered |
| 79 | + finding above: the state bag survives `ChatClientAgent`'s own serialization on its own. |
| 80 | + |
| 81 | +Both also assert `restored.Should().NotBeSameAs(session, ...)` -- a basic sanity check that this is a genuine |
| 82 | +restore-from-JSON, not an accidentally-reused object reference. |
| 83 | + |
| 84 | +Modified: `NamsLiveFixture.cs` (added the two AgentFramework registrations, see §2), and |
| 85 | +`AgentMemory.Tests.Integration.csproj` (added the `Microsoft.Agents.AI` package reference and the |
| 86 | +`AgentMemory.AgentFramework.Nams` project reference). |
| 87 | + |
| 88 | +## 4. Self-review findings and fixes |
| 89 | + |
| 90 | +2 parallel reviewers (correctness/test-soundness, cross-file/conventions). |
| 91 | + |
| 92 | +**Correctness reviewer found one HIGH finding, fixed:** `SessionRestore_WithMemoryIdentityReapplied_...` |
| 93 | +re-applied `.WithMemoryIdentity(...)` after restore using the SAME closed-over identity values the test had |
| 94 | +already used before serialization. Since that call unconditionally overwrites the state bag, the second |
| 95 | +turn's persistence target was fully determined by the test's own known values, not by anything the JSON |
| 96 | +round-trip actually preserved -- reintroducing, one layer down (`WithMemoryIdentity → ExtractIds → |
| 97 | +ResolveAsync`), exactly the "second direct resolver call with the same identity, which would trivially agree |
| 98 | +regardless of whether restore worked" triviality this file's own doc comment says the design avoids. Only the |
| 99 | +second test (which never re-applies) was actually non-vacuous as originally written. Fixed by reading the |
| 100 | +restored session's identity back via the same public `GetMemoryIdentity()` extension the production code |
| 101 | +itself uses, and asserting it already matches BEFORE re-applying `WithMemoryIdentity` -- this reads what |
| 102 | +restore actually produced, independent of the known values, making the "state survived" claim genuine. The |
| 103 | +re-apply step (matching the sample's recipe) still follows afterward, so the test now proves both things: the |
| 104 | +state bag itself survived, and the documented recipe subsequently works end-to-end. |
| 105 | + |
| 106 | +A second, lower-severity point (both pre- and post-restore turns share one `agent`/`ChatClientAgent` |
| 107 | +instance, so a hypothetical bug that dropped only `SessionId`/`ConversationId` while `UserId` survived could |
| 108 | +hide behind `ExtractIds`'s `agent.Id` fallback) was noted as a narrow residual seam, not required to fix: all |
| 109 | +four state-bag keys share one read/write code path, making that scenario very unlikely, and the new explicit |
| 110 | +`GetMemoryIdentity()` assertions in Test 1 already check all four fields independently. |
| 111 | + |
| 112 | +**Conventions reviewer found 3 low/medium findings, all fixed:** |
| 113 | + |
| 114 | +- **Missing orphan-tracing convention (Medium):** `UniqueIds()` didn't embed the calling test's name via |
| 115 | + `[CallerMemberName]` the way `NamsLiveConnectivityTests.UniqueIdentity`/`NamsMultiInstanceMappingTests.UniqueIdentity` |
| 116 | + do -- relevant here too, since these tests also never delete the NAMS-side conversations they create. Fixed |
| 117 | + by replacing `UniqueIds()` with a `UniqueIdentity([CallerMemberName] string testName = "")` returning a |
| 118 | + `NamsConversationIdentity` directly (see next point), matching the sibling convention exactly. |
| 119 | +- **`PollUntilAsync` duplicated verbatim** from `NamsLiveConnectivityTests.cs`. Extracted into a new shared |
| 120 | + `NamsLiveTestHelpers.PollUntilAsync`; `NamsLiveConnectivityTests.cs` now forwards its own private |
| 121 | + `PollUntilAsync` to the shared implementation (kept as a thin wrapper rather than rewriting that file's ~8 |
| 122 | + call sites, to keep this phase's blast radius on an already-merged, heavily-used file minimal). |
| 123 | +- **Minor redundancy:** `UniqueIds()` returned a raw 4-string tuple, so both tests separately reconstructed a |
| 124 | + `NamsConversationIdentity` from it just to call the resolver. Fixed by having `UniqueIdentity()` return |
| 125 | + `NamsConversationIdentity` directly; both tests now read `.UserId`/`.SessionId`/`.LocalConversationId`/ |
| 126 | + `.ApplicationId` off one object for both `WithMemoryIdentity` and `resolver.ResolveAsync`. |
| 127 | + |
| 128 | +Re-verified after all fixes: 17/17 live tests still green (including both session-restore tests). |
| 129 | + |
| 130 | +## 5. Verification |
| 131 | + |
| 132 | +- `dotnet build tests/AgentMemory.Tests.Integration -c Debug` -- clean. |
| 133 | +- `dotnet test tests/AgentMemory.Tests.Integration --filter "...NamsSessionRestoreTests"` -- **2/2 live**, |
| 134 | + first try (including the "without reapplying identity" test, confirming the empirical finding), against the |
| 135 | + real NAMS SaaS. |
| 136 | +- `dotnet build AgentMemory.slnx -c Release` -- 0 warnings, 0 errors. |
| 137 | +- `dotnet test tests/AgentMemory.Tests.Unit -c Release` -- full suite green, 3262/3262 (no new unit tests -- |
| 138 | + this phase is integration-only; the fixture/csproj changes are purely additive and touch no unit-tested code). |
| 139 | +- `dotnet test tests/AgentMemory.Tests.Integration --filter "FullyQualifiedName~Nams"` -- **17/17 live** (15 |
| 140 | + previous + 2 new), against the real NAMS SaaS. |
| 141 | + |
| 142 | +## 6. Definition of done |
| 143 | + |
| 144 | +- [x] Two new live tests proving session identity (and the underlying NAMS conversation it maps to) survives |
| 145 | + `SerializeSessionAsync`/`DeserializeSessionAsync`, both with and without re-applying `.WithMemoryIdentity`. |
| 146 | +- [x] Full unit + live suites green. |
| 147 | +- [x] Self-reviewed via 2 parallel independent reviewers; 1 HIGH + 3 low/medium findings, all fixed. |
| 148 | +- [ ] PR opened, CI green, merged to `main`. |
0 commit comments