refactor: eliminate primitive obsession, duplicate code, and trivial tests#302
Merged
Aaronontheweb merged 2 commits intodevfrom Mar 20, 2026
Merged
refactor: eliminate primitive obsession, duplicate code, and trivial tests#302Aaronontheweb merged 2 commits intodevfrom
Aaronontheweb merged 2 commits intodevfrom
Conversation
…tests Replace magic channel-type strings with a ChannelType enum across the entire pipeline (IChannel, MessageSource, SessionPipelineOptions, DeliveryFailed, SessionRegistry, DaemonClient). Wire boundaries (SignalR hub) parse strings to enum at the edge via TryFromWireValue. Extract SessionId.ToMemoryDomain() to replace 6 duplicate copies of ResolveDomain scattered across memory tools, recall coordinator, LlmSessionActor, and SubAgentActor. Strengthen MemoryCheckpointRequest by replacing raw string SessionId and TriggerType with SessionId value object and CheckpointTriggerType enum. Add VerifiedToolFinding and CompactionBoundary enum values that were previously inline string literals. Extract SessionOutputTypes constants from SessionOutputDtoMapper to prevent typos between ToDto/FromDto discriminator strings. Fix TimeProvider violations: inject TimeProvider into SqliteGetMemoriesTool and NullMemoryCheckpointSink instead of using DateTimeOffset.UtcNow and TimeProvider.System directly. Replace Task.Delay polling in WebhookNotificationServiceTests with SemaphoreSlim signaling for deterministic test synchronization. Deduplicate BuildInfo.ResolveCommitHash by exposing the shared implementation and delegating from the daemon-specific facade. Delete trivial tests (SubAgentConfigTests, NullScannerTests) and consolidate 5 McpToolAdapter property tests into a single fact.
This was referenced Mar 20, 2026
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
Comprehensive code quality refactoring addressing primitive obsession, duplicate code, magic strings, trivial tests, and TimeProvider violations.
"slack","tui","headless","signalr","reminder") with a strongly-typedChannelTypeenum across the entire pipeline. Wire boundaries (SignalR hub) parse strings to enum at the edge.ResolveDomainlogic from 6 files into a single method on theSessionIdvalue object.string SessionIdwithSessionIdvalue object andstring TriggerTypewithCheckpointTriggerTypeenum. AddVerifiedToolFindingandCompactionBoundaryenum values.SessionOutputDtoMapperinto compile-time constants.TimeProviderintoSqliteGetMemoriesToolandNullMemoryCheckpointSink.SubAgentConfigTests,NullScannerTests), consolidate 5 property tests into 1, replaceTask.Delaypolling withSemaphoreSlimsignaling.ResolveCommitHashfrom sharedBuildInfoand delegate from the daemon facade.Net result: -65 lines (245 added, 310 removed) across 38 files.
Test plan
dotnet build— 0 errors, 0 warningsdotnet test— 1,121 tests passed, 0 failed across 7 test projects