Surface dropped inbound messages via new on_error callback#88
Surface dropped inbound messages via new on_error callback#88ryanrishi wants to merge 2 commits into
Conversation
When participant reconciliation returns None in _handle_communication_created (listParticipants unreachable, a 409 on participant promotion, a wrong-type agent at our address, or no resolvable customer), the handler logged a warning and returned early. The message-ready callback never fired and nothing was surfaced to the app, so the inbound message was silently dropped and invisible. Introduce an optional on_error callback on TAC, mirroring the existing callback pattern (registration method, stored handler, and a trigger_error dispatch supporting sync and async handlers). Route the reconcile-failure branch through it with error-level logging plus context (conversation_id, channel, dropped_inbound). The callback is additive and backward compatible: with no handler registered, behavior degrades to logging. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds an additive on_error callback to TAC so recoverable errors (notably participant-reconciliation failures that would otherwise drop an inbound message silently) are surfaced to SDK consumers, plus docs and tests for the new behavior.
Changes:
- Introduces
TAC.on_error()registration +TAC.trigger_error()dispatch (sync/async handlers; handler exceptions swallowed + logged). - Updates messaging inbound processing to log reconciliation failure at error level and trigger
on_errorwith dropped-inbound context. - Adds unit tests and README documentation for the new callback.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_error_callback.py | Adds tests for on_error registration/dispatch and dropped-inbound routing behavior. |
| src/tac/core/tac.py | Implements on_error and trigger_error with sync/async support and defensive handler execution. |
| src/tac/channels/messaging.py | Triggers on_error when participant reconciliation fails and an inbound is dropped. |
| README.md | Documents available callbacks and provides on_error usage example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_test_config() -> dict[str, Any]: | ||
| return { | ||
| "account_sid": "ACtest123", | ||
| "auth_token": "test_token_123", | ||
| "api_key": "SK123", | ||
| "api_secret": "test_api_token", | ||
| "conversation_configuration_id": "conv_configuration_test123", | ||
| "phone_number": "+15551234567", | ||
| } |
There was a problem hiding this comment.
These tests are already hermetic. tests/conftest.py defines an autouse mock_conversation_configuration fixture that patches ConversationClient.get_configuration for every test in the suite, returning a minimal configuration with memory_store_id. No real network call happens during TAC(get_test_config()) init. This matches how the rest of the suite (e.g. test_tac.py, test_participant_reconciliation.py) constructs TAC, so adding per-test mocking here would duplicate the shared fixture and diverge from the repo convention.
- trigger_error: re-raise asyncio.CancelledError before the broad handler catch so cooperative cancellation always propagates, while genuine handler failures are still swallowed and logged. - Add a test asserting trigger_error does not raise when the registered handler raises, and that the failure is logged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
_handle_communication_createdreconciles participants before invoking the message-ready callback. When_reconcile_participantsreturnsNone(listParticipants unreachable, a 409 on participant promotion, a wrong-type agent at our address, or no resolvable customer), the handler logged awarningand returned early. The message-ready callback never fired and nothing was surfaced to the app — the inbound message was silently dropped and invisible.This introduces an optional
on_errorcallback onTAC, mirroring the existing callback pattern (on_message_ready/on_interrupt/on_conversation_ended):TAC.on_error(callback)registers a handler;TAC.trigger_error(error, context)dispatches it, supporting both sync and async handlers. Exceptions raised by the handler are logged and swallowed so error reporting never breaks the caller's flow.messaging.pynow logs at error level AND callstrigger_errorwith context:conversation_id,channel, anddropped_inbound=True.The callback is additive and backward compatible: with no handler registered,
trigger_erroris a no-op and behavior degrades to logging.Type of Change
Checklist
SDK Parity
This is the Python parity fix for twilio/twilio-agent-connect-typescript#80. Python required a new
on_errorcallback because it had none — the TypeScript SDK already hadonErroron its base channel, so this brings Python to parity.🤖 Generated with Claude Code