feat(server): validate channel types at TACFastAPIServer construction#79
Open
ryanrouleau wants to merge 1 commit into
Open
feat(server): validate channel types at TACFastAPIServer construction#79ryanrouleau wants to merge 1 commit into
ryanrouleau wants to merge 1 commit into
Conversation
Reject a wrong-typed voice_channel or messaging_channels entry (most
commonly a None from an unconfigured connector channel passed straight
through) with a clear TypeError at construction, instead of an opaque
AttributeError ('NoneType' object has no attribute 'get_channel_name')
surfacing later during webhook dispatch.
- voice_channel must be a VoiceChannel or None.
- every messaging_channels entry must be a MessagingChannel.
Mirrors the existing _validate_voice_url_config fail-fast pattern. Moves
VoiceChannel/MessagingChannel to runtime imports for the isinstance check.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds early, explicit validation of channel types when constructing TACFastAPIServer, preventing late AttributeErrors during webhook dispatch and improving startup-time feedback.
Changes:
- Add
_validate_channel_types()and call it duringTACFastAPIServer.__init__. - Move
VoiceChannel/MessagingChannelimports to runtime forisinstancechecks. - Add a focused test suite covering invalid/valid channel wiring at construction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_server.py | Adds regression tests to ensure invalid channel entries fail fast at server construction. |
| src/tac/server/fastapi_server.py | Implements runtime type validation for voice_channel and messaging_channels during server initialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| object has no attribute 'get_channel_name'``) deep in webhook dispatch, | ||
| far from the actual misconfiguration. | ||
| """ | ||
| if self.voice_channel is not None and not isinstance(self.voice_channel, VoiceChannel): |
Comment on lines
+142
to
+149
| for channel in self.messaging_channels: | ||
| if not isinstance(channel, MessagingChannel): | ||
| raise TypeError( | ||
| "messaging_channels must contain MessagingChannel instances " | ||
| "(SMSChannel, RCSChannel, WhatsAppChannel, ChatChannel), got " | ||
| f"{type(channel).__name__}. A None here usually means a channel " | ||
| "that wasn't configured was passed through — filter those out." | ||
| ) |
Comment on lines
+132
to
+135
| ``messaging_channels`` / ``voice_channel``. Without this check that | ||
| surfaces much later as an opaque ``AttributeError`` (e.g. ``'NoneType' | ||
| object has no attribute 'get_channel_name'``) deep in webhook dispatch, | ||
| far from the actual misconfiguration. |
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
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
TACFastAPIServernow validates channel types at construction and fails fast with a clearTypeError, instead of letting a bad entry surface much later as an opaqueAttributeErrordeep in webhook dispatch.voice_channelmust be aVoiceChannelorNone.messaging_channelsentry must be aMessagingChannel(SMSChannel/RCSChannel/WhatsAppChannel/ChatChannel).Motivation
The common failure mode: a caller passes a channel that wasn't configured — e.g. a connector exposes
rcs_channel/whatsapp_channelasNonewhen the address isn't set, and aNoneslips intomessaging_channels. Today thatNoneis accepted silently,extended intowebhook_channels, and only blows up on the first inbound webhook with:— far from the actual mistake, and easy to miss in CI/smoke tests that don't exercise webhooks. This change catches it at construction, next to where the channels are wired.
It mirrors the existing
_validate_voice_url_config"fail fast at construction" pattern already in this class.Changes
_validate_channel_types(), called first in__init__.VoiceChannel/MessagingChannelfromTYPE_CHECKING-only to runtime imports (needed forisinstance).TestChannelTypeValidationAtStartup(None entry, wrong type, VoiceChannel-in-messaging, wrong-type voice, and the valid-channels-pass case).Verification
make check— ruff + mypy strict (75 files) + 766 tests pass (761 existing + 5 new; no existing test changed).Context
The Azure connectors'
TACHostedAgentsAppadded the same guard for its own server. This PR brings the check to the sharedTACFastAPIServerso both the AWS and Azure packages (and any direct consumer) benefit, rather than each server reimplementing it.🤖 Generated with Claude Code