feat(gateway)!: flip Session.ConversationId to non-nullable (P9-B-2, closes #627)#641
Merged
Merged
Conversation
Per maintainer directive G-1 + W-4: Session.ConversationId is now a non-nullable Vogen `ConversationId` value object instead of `ConversationId?`. Every session in the system MUST belong to a conversation. The unset-sentinel is `default(ConversationId)` and is distinguished from a stamped value via `ConversationId.IsInitialized()`. Domain shape: - `Session.ConversationId` is now `ConversationId` (non-nullable). - `GatewaySession.ConversationId` proxy mirrors the new shape. - `ConversationId` Vogen reconfig: keep `IsInitialized()` generation on for the sentinel discriminator and add `required` enforcement at the persistence boundary. Callers can no longer treat `session.ConversationId` as nullable. Use `session.ConversationId.IsInitialized()` to discriminate the unset-sentinel case from a stamped value. The save path stamps via `LegacyConversationResolver` before the writer runs (see follow-up commit) so the in-memory unset window is short-lived during construction only. Refs #613, closes #627. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…kfill After flipping Session.ConversationId to non-nullable (P9-B-2), the store layer enforces that every persisted session has a stamped `ConversationId`. The save path stamps via `LegacyConversationResolver` before the writer runs; the writer throws if it ever sees an uninitialized value. Changes: - `SqliteSessionStore.UpsertSessionAsync` now throws `InvalidOperationException` if `session.ConversationId.IsInitialized()` is false. This is fail-loud-by-design: it surfaces composition errors (resolver not wired) and bypass attempts immediately rather than silently writing NULL. - `SaveAsync` / `ArchiveAsync` unconditionally call `EnsureConversationIdStampedAsync` (in `SessionStoreBase`) before the writer, so the throw only fires when the resolver is missing or someone bypasses `SaveAsync`. - `SessionStoreBase.EnsureConversationIdStampedAsync` is the single entry point for save-time backfill. It is idempotent and re-entrant. - `InMemorySessionStore` mirrors the same shape for parity. - `SqliteSessionStore` ctor now accepts an optional `IConversationStore` to power the resolver wiring. Refs #613, refs #627. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ariant `FileSessionStore` now eagerly migrates orphan sessions at startup so that the first load returns sessions with a stamped `ConversationId` under the new non-nullable invariant. `MigrateOrphanedSessionsAsync` runs once per process under `_migrationLock` + `_migrated` gate. It: 1. Pre-binds the most-recently-updated **Active** orphan per agent to its legacy conversation FIRST via `BindActiveSessionIfNoneAsync(OrderByDescending(UpdatedAt).First())`. This MUST happen before the per-orphan stamp loop, otherwise the load-time backfill in `LoadFromFileAsync` would call `BindActiveSessionIfNoneAsync` for the first orphan it touches in alphabetical (filename) order and pin the wrong ActiveSessionId. 2. Walks every orphan file, stamps `Session.ConversationId` via the resolver, and rewrites the file with the Vogen-safe nullable shape for back-compat sidecar columns (`value.IsInitialized() ? value : (T?)null`). 3. Sealed / Suspended / Expired orphans get stamped but NOT bound — only Active orphans contribute to `ActiveSessionId`. Includes a `MigrationInvocationCount` probe counter (`Interlocked.Increment` inside the lock-gated path) used by tests to prove the eager sweep ran exactly once per process. The pre-bind step makes the per-orphan `BindActiveSessionIfNoneAsync` call inside the loop a no-op (mirror-back path in the resolver), so the two binding sites are idempotent against each other. Refs #613, refs #627. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n Session.ConversationId
After P9-B-2 flipped `Session.ConversationId` to non-nullable, every
existing callsite that used `session.ConversationId is { } convId`
(nullable pattern) silently became ALWAYS-TRUE because `ConversationId`
is now a non-nullable Vogen struct. The orphan-fallback / unset-fallback
branches in those callsites were unreachable.
Fixed callsites (use `session.ConversationId.IsInitialized()` to
discriminate the unset-sentinel case, then read `session.ConversationId`
directly):
- `GatewayHub.ResetSession` (orphan-session check) —
detected by `ResetSession_OrphanSession_NoConversation_SealsInPlace_DoesNotArchive`
via Moq Strict on `IConversationResetService.ResetActiveSessionAsync`.
- `ConversationsController` linked-conversation lookup.
- `CrossWorldFederationController` ConversationId reuse refusal gate.
- `SessionsController` ConversationId resolution.
- `GatewayHost` (ask-user interceptor + fan-out stale-binding mute).
- `InProcessIsolationStrategy.ResolveConversationIdAsync`.
- `WorkspaceContextBuilder` system-prompt context resolution.
- `DefaultConversationRouter` ConversationId routing.
- `CronTrigger` ConversationId scheduling.
- `VirtualWorld` scenario harness ConversationId helper.
Note: `CronTrigger` `request?.ConversationId is { }` is correct as-is
because `IInternalTrigger.ConversationId` (the request DTO field) is
genuinely `ConversationId?` and only the `Session.ConversationId` site
needed the IsInitialized fix.
Refs #613, refs #627.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New architecture fitness function with reflection + source fences that: 1. Asserts via reflection that `Session.ConversationId` is `ConversationId` (not `ConversationId?`), with the same check on `GatewaySession.ConversationId`. 2. Bans the nullable patterns that became silent always-true after the flip: `session.ConversationId is null`, `== null`, `.HasValue`, `?.Value`, etc. across `src/`. 3. Sanctions the IsInitialized discriminator with a 3-entry allowlist covering the legitimate usages (the only callsites the flip needs). 4. Self-tests that all source fences match at least one symbol — prevents silent neutralization the way our prior regex fences regressed when return types went generic (e.g. `Task<T>` not matched by `\w+`). 5. Self-tests prove non-vacuity of the allowlist itself. 7/7 GREEN against the post-P9-B-2 source tree. Refs #613, refs #627. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…il-loud guard The P9-B-2 fail-loud guard in `SqliteSessionStore.UpsertSessionAsync` caused every test that constructed a store without an `IConversationStore` to fail with `InvalidOperationException: Refusing to persist session 'X' for agent 'Y' with an unset ConversationId.` Migrated test fixtures to inject a shared per-fixture `InMemoryConversationStore` so save-time backfill via `LegacyConversationResolver` can stamp `Session.ConversationId` before reaching the writer. Each fixture exposes the conversation store as a property so reload/round-trip tests can use the same in-memory state across multiple `CreateStore()` invocations. Files migrated: - `SqliteSessionStoreTests`, `FileSessionStoreTests`, `SessionStoreBaseContractTests`, `SessionStoreExistenceQueryTests`, `SqliteSessionStoreSessionParticipantBackCompatTests`, `SqliteSessionStoreConversationIdTests` — primary store fixtures. - `Integration/SessionCompactionIntegrationTests` — `SqliteStoreFixture` now shares an `InMemoryConversationStore` across the write→reload test that proves persisted state survives a fresh store instance. - `Integration/SignalRConversationRoutingTests`, `SignalRReliabilityTests`, `SignalRThreadRoutingTests` — composed fixtures. - `ConversationsControllerHistoryTests`, `CrossWorldFederationControllerTests`, `GatewayHostTests`, `SessionsControllerTests`, `GatewaySessionBehaviorSnapshotTests`, `AgentExchangeConversationTests`, `SubAgentEagerConversationPinTests`, `DefaultConversationRouterTests` — controller/integration tests. New tests in `LegacyConversationBackfillTests`: - 21 eager-sweep tests covering `MigrateOrphanedSessionsAsync` invariants: pre-bind-most-recent-Active-orphan ordering, per-status stamping (Active binds + stamps; Sealed/Suspended/Expired stamp without binding), idempotency via `MigrationInvocationCount` probe counter, single-run per process under `_migrationLock`, deterministic ordering when multiple Active orphans share the same UpdatedAt. Final state: Gateway.Tests 1995 / 0 / 1-skip GREEN. Refs #613, closes #627. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #627. Refs umbrella #613 (P9).
P9-B-2 flips Session.ConversationId from ConversationId? to non-nullable ConversationId per maintainer directive G-1: every session in the system MUST belong to a conversation. The unset sentinel default(ConversationId) is short-lived (in-memory construction only) and is distinguished from a stamped value via ConversationId.IsInitialized(). The save path stamps via LegacyConversationResolver before the writer runs; the writer fails loud if it ever sees an uninitialized value.
Commits
feat(domain)!: flip Session.ConversationId to non-nullable (P9-B-2)— domain shape changefeat(sessions): SQLite fail-loud guard + save-time ConversationId backfill— writer enforces invariant, save path stamps via resolverfeat(sessions): FileStore eager startup sweep + pre-bind ordering invariant— MigrateOrphanedSessionsAsync with pre-bind-most-recent-Active orderingfix(gateway): use IsInitialized() guard instead of nullable pattern— 10 callsites that became silent always-true after the fliptest(architecture): pin Session.ConversationId non-nullable invariant— new arch fitness function (7/7)test(gateway): migrate fixtures + add eager-sweep tests— 19 test files migrated + 21 new eager-sweep testsThe
?.ConversationId is { }regression we caughtPre-flip, session.ConversationId is { } convId was a nullable pattern that correctly unwrapped the Some-case. Post-flip, the pattern silently became ALWAYS-TRUE because the property is a non-nullable Vogen struct. The orphan-fallback / unset-fallback branches at 10 callsites were unreachable. The hub test
ResetSession_OrphanSession_NoConversation_SealsInPlace_DoesNotArchivecaught this via Moq Strict onIConversationResetService.ResetActiveSessionAsync— without that test the regression would have shipped silently.Fixed pattern is uniform:
session is not null && session.ConversationId.IsInitialized()then read session.ConversationId directly. The new arch fitness function pins this against revert.Critique sweep
LegacyConversationResolverhardening from PR feat(sessions): backfill orphan sessions to per-agent legacy conversations #616 still holds.MigrationInvocationCountincrements inside the lock-gated path,InMemoryConversationStorecorrectly shared per-fixture, SQLite reload reads from disk not cache,INSERT INTO sessionsonly appears at one writer, Vogen-null sidecar handling correct.Validation
dotnet build BotNexus.slnx --nologo --tl:off— 0 warnings / 0 errors (underTreatWarningsAsErrors=true)dotnet test tests/gateway/BotNexus.Gateway.Tests/— 1995 / 0 / 1-skip GREEN (skip is [Canvas] Canvas HTML not persisted — lost on reconnect, invisible to second browsers, wrong scope (per-agent not per-conversation) #383, unrelated)CompactionFlowTests.SlashCompact_NotifiesUser_NoCronConversation_ContinuesNormally= pre-existing [Gateway] CompactionFlowTests: /compact slash command no longer intercepted; user message routed to LLM as plain prompt #618 PR refactor(gateway): unify compaction paths + add /compact E2E + exclude integration tests from PR CI #608 regression, out of scope)SessionConversationIdNonNullableArchitectureTests— 7/7 GREENOut of scope (separate follow-ups)
(agent_id, title)unique constraint missing (future work)LegacyConversationResolverlogger wiring through store ctors (future work)/compactregression — separate issue)