refactor: extract shared helpers for duplication consolidation#686
Merged
Aaronontheweb merged 8 commits intodevfrom Apr 20, 2026
Merged
refactor: extract shared helpers for duplication consolidation#686Aaronontheweb merged 8 commits intodevfrom
Aaronontheweb merged 8 commits intodevfrom
Conversation
…agnostics Additive-only extraction of shared helpers to eliminate duplicated code across pipeline actors, alert emission sites, and doctor checks. No existing behavior changes — all helpers are new code with new tests. New types: - SessionPipelineHandle: composition helper for pipeline lifecycle (init, reinit, dispose) - ExecutionOutputAccumulator: output buffering + notification tracking for execution actors - OperationalAlert.Create(): factory method normalizing alert construction (replaces 16 sites) - IdGen: centralized ID generation (AlertId, ShortId, Suffix, Full) - CrashLogHelper: shared crash-log parsing for doctor checks - DoctorJsonConfigReader.ReadBool/ReadStringArray: shared JSON helpers Test infrastructure: - ScriptedSessionPipeline and FailingSessionPipeline extracted to shared TestHelpers - 22 new unit tests covering all extracted helpers Refs #306
Replace 16 OperationalAlert constructions with OperationalAlert.Create(), remove 4 duplicate ReadBool copies, 2 duplicate crash-log helpers, and 7 Guid truncation sites with IdGen calls. Net: -124 lines across 22 files.
…cutionOutputAccumulator Replace duplicated pipeline lifecycle and output accumulation code across: - WebhookExecutionActor (240→155 lines) - ReminderExecutionActor (465→390 lines, Mode B path preserved) - SignalRSessionActor (385→285 lines) - SlackThreadBindingActor (1452→1350 lines) Deleted: duplicated materializer/stream wiring, reinit logic, PostStop cleanup, HandleOutput switch, TrackNotificationResult, BuildNotifyFailureMessage. All 2,482 tests pass. Slopwatch clean.
Collaborator
Author
Updated: Full consolidation completeThis PR now includes the complete consolidation — helpers + wiring + deletion of all duplicated code. Production code impact: -361 net lines (362 added, 723 removed in existing files)4 pipeline actors rewired to
2 execution actors rewired to 16 alert construction sites → 6 doctor check files — removed 4 7 IdGen sites — Closes #306 Verification
|
2 tasks
- SessionPipelineHandle: store init params so ReinitializeAsync takes only (reason, onFailed) instead of 7 params. Drop IAsyncDisposable in favor of synchronous Dispose() since all callers are in PostStop. - ExecutionOutputAccumulator: add optional notification tracking callback to restore lost diagnostic logging in ReminderExecutionActor. - SlackThreadBindingActor: make _handle non-nullable (eager init in ctor). - Delete trivial IdGenTests (violates CLAUDE.md testing guidelines).
Add AlertSeverity enum (Info, Warning, Critical) and change OperationalAlert.Severity from string to AlertSeverity. Wire format preserved via ToString().ToLowerInvariant() at the single serialization point in WebhookNotificationService.
Aaronontheweb
commented
Apr 16, 2026
Replace hardcoded "send_slack_message" constant with a ToolName constructor parameter. Callers supply the notification tool they care about — the accumulator has no channel knowledge.
…rCorrelation tests SlackActorHierarchyTests: raise TestKit default-timeout from 3s to 5s to match the pattern already used by SlackProactiveThreadActorTests. Under CI with parallel test classes, ThreadPool contention can delay in-memory message delivery past the 3s default. ErrorCorrelationTests: remove Task.Yield() from FailingChatClient so the exception propagates synchronously. The yield deferred the exception to the ThreadPool, making LlmCallFailed delivery timing dependent on thread scheduling rather than deterministic actor mailbox ordering.
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
OperationalAlertandDoctorJsonConfigReaderNew types
SessionPipelineHandleExecutionOutputAccumulatorOperationalAlert.Create()IdGenCrashLogHelperDoctorJsonConfigReader.ReadBool/ReadStringArrayTest infrastructure
ScriptedSessionPipelineandFailingSessionPipelineextracted toTestHelpers/for reuseTest plan
dotnet build— 0 warnings, 0 errorsdotnet test— all 2,482 tests pass (0 failures)