Add channel delivery descriptor registry#1326
Merged
Aaronontheweb merged 33 commits intoJun 9, 2026
Merged
Conversation
Aaronontheweb
commented
Jun 4, 2026
Aaronontheweb
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed the spec.md - working on the others next
| - **GIVEN** two Mattermost channels have the same display name visible to the bot | ||
| - **WHEN** a lookup query uses that display name | ||
| - **THEN** resolution fails loudly | ||
| - **AND** the result includes candidate stable IDs and display names |
Collaborator
Author
There was a problem hiding this comment.
agree with this though
Aaronontheweb
commented
Jun 4, 2026
Aaronontheweb
left a comment
Collaborator
Author
There was a problem hiding this comment.
The design is murky - should not treat reminders and webhooks as co-equals with Slack et al
Collaborator
Author
Channel Tool / Capability MatrixScope note: this PR has validated descriptor/status behavior and non-live channel-tool registration alignment. It has not validated live Slack/Discord/Mattermost connectivity or live outbound delivery. Current Channel-Specific Tools
Planned Standard Tools / Follow-Ups
OpenSpec CoverageYes, the planned work is captured in the OpenSpec change, with one distinction:
Key task references:
This PR should not claim those planned tools are implemented; it only establishes the descriptor/status foundation and validates current tool registration alignment. |
Collaborator
Author
|
Live local binary-swap validation completed on commit Checks:
No live Slack delivery smoke was performed, and no Slack messages were sent. |
Collaborator
Author
|
Manual live validation update: Slack-thread approval flow was exercised with real approvals, and the user reports it is working correctly. This covers the remaining live manual gap for approval UX on the swapped local daemon build |
Collaborator
Author
|
Manual live validation complete on swapped local daemon build Per user validation, the following live UX checks passed:
No live Discord or Mattermost delivery validation was performed for this PR. |
Collaborator
Author
|
Channel-wide messaging ACL audit update on commit Results:
Validation:
Live Slack retry still requires adding the target Slack user to existing |
Collaborator
Author
|
OpenSpec observability follow-up completed on commit Completed:
Regression coverage:
Validation:
|
722249b to
30f0824
Compare
Correctness fixes: - Fix Discord DM proactive thread flag (_threadCreated false for DMs) - Remove thread_or_root_id param from send tool (caused unconditional error) - Split shared CancellationTokenSource in Mattermost file upload/post - Remove Thread from channel AddressKinds (no resolver handles it) - Align SupportedOutputEffects with registered renderers - Fix StartReconnectLoop double-ContinueWith race - Replace per-access Concat().ToHashSet() with static sets in DiscordAddressResolver Consolidation: - Extract ChannelDescriptor.CreateRemoteChat static factory (-99 lines) - Move ChannelAddressKindWire to Netclaw.Channels (correct layering) - Simplify SendChannelMessageTool dispatch to switch expression - Replace ContinueWith reconnect callback with Volatile flag + DrainQueuedReconnect
Move the reconnect/retry logic from MattermostChannel's manual concurrency primitives (CancellationTokenSource, lock, Volatile, Interlocked) into MattermostNetGatewayLifecycleActor's state machine. The lifecycle actor now owns auto-reconnect with exponential backoff (5s → 5min), failure classification (fatal vs transient), and a RetryConnect timer message. MattermostChannel subscribes to a new ConnectionRestored event and simply completes setup on reconnect. Net removal: ~150 lines of thread-unsafe reconnect loop code replaced by actor-native timer scheduling.
Same pattern as the Mattermost actorization: move the reconnect/retry logic from DiscordChannel's manual concurrency primitives (CTS, lock, Volatile, Interlocked, ContinueWith) into DiscordNetGatewayLifecycleActor. The lifecycle actor now owns auto-reconnect with exponential backoff (5s → 5min), failure classification (fatal vs transient), and a RetryConnect timer message. RequestCleanReconnect now auto-drives a stop/retry cycle instead of delegating to the channel. DiscordChannel subscribes to a new ConnectionRestored event and simply completes setup on reconnect, matching Mattermost's standardized pattern. Net removal: ~150 lines of thread-unsafe reconnect loop code replaced by actor-native timer scheduling. Removed 3 channel health tests that tested the deleted reconnect loop; lifecycle actor tests cover the new behavior.
- Tighten Slack ID validation (IsSlackChannelId/IsSlackUserId) to require length ≥ 9 and alphanumeric-only, preventing short channel names from being misclassified as raw IDs - Add exact-match preference to Slack channel resolution so "general" resolves to #general instead of returning ambiguous results with #general-private - Add protobuf serialization for ReminderDelivery.Target via new ChannelDeliveryTargetProto message - Add CancellationToken to IMattermostGatewayTransport.StartAsync and forward to the underlying WebSocket client - Cancel retry timers in PostStop for both Discord and Mattermost lifecycle actors to prevent post-stop timer firings - Mark _gateway fields volatile in DiscordChannel and MattermostChannel for safe cross-thread reads - Normalize ChannelDescriptorKey to lowercase in constructor - Consolidate duplicated validation helpers: delegate IsDiscordSnowflake, IsMattermostId, and NormalizeKey to their canonical implementations - Remove duplicated address-parsing fallback in ReminderExecutionActor (already handled by ResolveChannelDeliveryTarget) - Remove dead Become(CleanReconnectRequired) calls in both lifecycle actors - Cache sorted descriptor list in ChannelRegistry constructor - Add baseline TextMessage and FileAttachment output effects to CreateRemoteChat factory
4732d8d to
695a534
Compare
Step-by-step guide for adding a new chat channel (Teams, WhatsApp, Signal, etc.) covering all 15 implementation layers: ChannelType enum, transport interfaces, IChannel service, actor hierarchy, address resolver, output renderer, LLM tools, reminder target resolver, DI registration, config schema, and Program.cs wiring. Includes architecture diagram, code templates, and testing checklist. Link added from CONTRIBUTING.md § Project Structure.
Plan to reduce per-channel implementation cost from ~15 files / ~1,800 LOC to ~5 files / ~500 LOC through generic lifecycle actor, gateway/conversation actor bases, registration builder, and send tool consolidation. Includes test gap analysis and 3 new contract test bases to close coverage gaps across Slack, Discord, and Mattermost. Seven independently shippable phases.
Add missing test coverage identified in the channel audit: - SlackGatewayContractTests: wire Slack fixture for the gateway routing contract (ACL, routing, dedup) — closes the only channel missing this contract - MattermostConnectFailureClassifierTests: fatal/transient classification, inner-exception unwrapping, idempotency, case-insensitive matching - MattermostThreadHistoryFetcherTests: bot-root inclusion, bot-reply exclusion, attachment inlining, content scan fail-closed 21 new tests, all green. No production code changes.
Collaborator
Author
|
I've tested this locally and it works against live channels - I'll do some follow-up PRs to consolidate the tests and reduce the code footprint, but I want to get the actual channel integrations out live into a beta so it can be properly tested in live environments. |
This was referenced Jun 9, 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
Validation