refactor(domain): migrate ConversationId and SessionId to Vogen value objects#517
Open
sytone wants to merge 5 commits into
Open
refactor(domain): migrate ConversationId and SessionId to Vogen value objects#517sytone wants to merge 5 commits into
sytone wants to merge 5 commits into
Conversation
… objects Closes the F-13 silent-string-conversion footgun on the two highest-traffic identifiers that the AgentId seed (#513) did not touch. Mirrors the `[ValueObject<string>(conversions: Conversions.SystemTextJson)]` shape used by AgentId so Vogen owns construction, JSON serialization, and equality. Both types preserve their public API: - `ConversationId.Create()` still emits `c_<32-char-guid>`. - `SessionId.Create / ForSubAgent (x2) / ForAgentConversation / ForSoul (x2) / ForCrossAgent` keep identical output formats; all factories internally call `From(...)` because Vogen blocks the public constructor. - `IsSubAgent / IsAgentConversation / IsSoul` discriminator properties remain (they are scheduled for removal in Phase 5 once SessionType is the authoritative source — out of scope for this PR). Validation is now centralised in `Validate` + `NormalizeInput` partial methods (reject null/empty/whitespace, trim on input). The legacy `ArgumentException` is replaced by `Vogen.ValueObjectValidationException`; existing tests are updated in matching commits. The hand-written `ConversationIdJsonConverter` and `SessionIdJsonConverter` are deleted — Vogen generates the equivalent `JsonConverter` from the `Conversions.SystemTextJson` option. Refs #516. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rewrites the dedicated tests for both identifiers so they exercise the Vogen-generated shape end-to-end: - Validation: null, empty, and whitespace inputs reject with `Vogen.ValueObjectValidationException` (built-in null check is asserted by exception type only — Vogen owns the message text). - Trim semantics from `NormalizeInput` are pinned. - `Create()` format is pinned (`c_<32-char-guid>` for ConversationId; bare 32-char N-formatted guid for SessionId). - Every SessionId structured factory has a format-pinning fact: `ForSubAgent` / `ForAgentConversation` / `ForSoul` (both DateOnly and DateTimeOffset overloads) / `ForCrossAgent`. The `ForSoul` test uses a `DateTimeOffset` with a +5:00 offset to confirm UTC conversion. - `System.Text.Json` round-trip is asserted on both types. - A reflection-based check confirms no implicit string conversion operator exists — the architecture rules in a sibling commit enforce the same invariant structurally. Updates `PrimitiveBoundaryTests` for the consequential behaviour change: `SessionId.ForSubAgent` with null/empty/whitespace parent now flows through Vogen `From(...)` and throws `ValueObjectValidationException` instead of `ArgumentException`. The shared whitespace-rejection fact is amended for both ConversationId and SessionId; the still-hand-rolled primitives (SenderId, ToolName, ChannelKey) continue to throw `ArgumentException` pending their later Vogen wave. Refs #516. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…SessionId Sweep of production call sites that previously relied on the implicit `string <-> SessionId` / `string <-> ConversationId` operators removed by the Vogen migration. No behavioural change. Boundary patterns applied: - Typed-to-string DTO crossings (e.g. `InboundMessage.SessionId` is still `string?` until the channels Vogen wave) take `.Value` explicitly so the boundary is visible (`MemoryIndexer`, `HeartbeatAction`, `AgentPromptActionStub`, `FileSessionStore` filename helpers, `ChannelHistoryController`, `DefaultSubAgentManager`, `InProcessIsolationStrategy`). - String-to-typed crossings wrap explicitly via `SessionId.From(...)` / `ConversationId.From(...)` instead of the deleted implicit cast (`GatewayHub`, `DefaultSubAgentManager`). - `string.Equals(..., OrdinalIgnoreCase)` calls on session ids are replaced with Vogen value equality (`==` / `!=`) — SessionIds are GUIDs or structured `agent::id::guid` strings so case sensitivity is the correct comparison. (`SubAgentManageTool`, `SessionsController`.) - `ConversationsController.TryParseVirtualCronConversationId` no longer assigns `default(SessionId)` (rejected by Vogen analyser VOG009). Signature is now `[NotNullWhen(true)] out SessionId?` returning `null`; the two callers unwrap with `.Value`. Refs #516. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ites Sweep across 27 test files that previously relied on the implicit `string -> SessionId` cast removed by the Vogen migration. Three patterns applied: - `SessionId.From(...)`: most call sites — store APIs like `GetAsync` / `DeleteAsync` / `ArchiveAsync` and constructors of `GatewaySession` now take a typed `SessionId`. - `.Value` to bare string: a handful of sites pass a SessionId into a helper that still takes `string` (e.g. `InboundMessage.SessionId` is `string?` until the channels Vogen wave; `CreateHandleMock(string agentId, string sessionId)` in `DefaultAgentSupervisorTests`). - Equality update in `SessionStoreEdgeCaseTests`: the expected exception on `SessionId.From(null/""/" ")` flips from `ArgumentException` to `Vogen.ValueObjectValidationException` (the comment that called this out as a follow-up is removed — this is the follow-up). In `CronSchedulerTests` one site that wrapped a string just to satisfy a `string`-typed parameter is simplified — the wrapping was already redundant. No new test cases; no behavioural change beyond the boundary visibility the Vogen migration enforces. Refs #516. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…and SessionId Adds four new fact methods that mirror the existing AgentId rules so the two newly-Vogen-migrated identifiers are structurally locked the same way AgentId is: - `ConversationId_IsVogenValueObject` / `SessionId_IsVogenValueObject` assert `[ValueObject<string>]` is present. - `ConversationId_HasNoImplicitStringConversion` / `SessionId_HasNoImplicitStringConversion` assert no `op_Implicit` exists. The generic `DomainPrimitives_VogenValueObjectsAreStructs` rule already covered the struct-shape invariant for any future value object. Refs #516. 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 the F-13 silent-string-conversion footgun on
ConversationIdandSessionId— the two highest-traffic identifiers that the AgentId seed (#513) did not touch. Both move to the same Vogen shape AgentId uses:csharp [ValueObject<string>(conversions: Conversions.SystemTextJson)] public readonly partial struct SessionId { ... }This finishes Phase 1 of the domain-model refactor (issues #500 / #513): the three highest-traffic IDs in the codebase now have one validation rule, one JSON converter, and zero implicit string casts.
What changed
ConversationId/SessionIdValidaterejects null/empty/whitespace;NormalizeInputtrims. The hand-writtenConversationIdJsonConverter/SessionIdJsonConverterare deleted — Vogen generates them.SessionIdfactoriesCreate/ForSubAgent(x2 overloads) /ForAgentConversation/ForSoul(x2 overloads) /ForCrossAgentpreserved with identical output formats; format-pinning tests guard against drift. All factories internally callFrom(...)(Vogen blocks the public constructor).SessionIddiscriminatorsIsSubAgent/IsAgentConversation/IsSoulpreserved. (Scheduled for removal in Phase 5 onceSessionTypebecomes authoritative — out of scope here.).Value; string-to-typed crossings use.From(...);string.Equals(..., OrdinalIgnoreCase)swapped to Vogen value equality (==/!=— SessionIds are GUIDs or structuredagent::id::guidstrings);ConversationsController.TryParseVirtualCronConversationIdreshaped to returnout SessionId?(Vogen analyserVOG009blocksdefault(SessionId)).SessionId.From(...); the few helpers that still acceptstring(e.g.CreateHandleMock(string sessionId)) get.Value.SessionStoreEdgeCaseTests+PrimitiveBoundaryTestsflip the expected exception fromArgumentExceptiontoVogen.ValueObjectValidationException(the previous TODO comments called this out — this is the follow-up).ConversationIdTests(11 facts) andSessionIdTests(18 facts) — every factory format pinned, JSON round-trip, no-implicit-string reflection check.ConversationId_IsVogenValueObject/ConversationId_HasNoImplicitStringConversionand the same pair forSessionId.Validation
dotnet build BotNexus.slnx --nologo --tl:off— 0 errors, 0 warnings (TreatWarningsAsErrors=true).dotnet test BotNexus.slnx --nologo --tl:off --no-build— full sweep green:BotNexus.Domain.Tests: 221 / 221BotNexus.Architecture.Tests: 14 / 14 (4 new rules)BotNexus.Gateway.Tests: 1633 / 1633Commits
refactor(domain): migrate ConversationId and SessionId to Vogen value objectstest(domain): pin ConversationId and SessionId behaviour under Vogenrefactor(gateway): use typed-id equality and explicit boundaries for SessionIdtest(gateway): wrap string ids with SessionId.From at typed-id call sitestest(architecture): extend Vogen fitness functions to ConversationId and SessionIdOut of scope (next PR)
ChannelKey/ChannelAddress/SenderId/ToolName/BindingId([Platform] Primitive obsession Phase 2 -- create JobId, ChannelId, RunId value objects and remediate #501).string?(e.g.InboundMessage.SessionId,GatewayActivity.SessionId,SessionSummary.SessionId) — they migrate with the channels wave.SessionIdstructural substring discriminators (::subagent::,::agent-agent::,::soul::) — scheduled for Phase 5 onceSessionTypebecomes authoritative.Closes #516.