Skip to content

feat(callbacks): allow registering multiple listener/customizer callbacks#76

Closed
ryanrouleau wants to merge 1 commit into
mainfrom
feat/multi-callback-registration
Closed

feat(callbacks): allow registering multiple listener/customizer callbacks#76
ryanrouleau wants to merge 1 commit into
mainfrom
feat/multi-callback-registration

Conversation

@ryanrouleau

Copy link
Copy Markdown
Collaborator

Summary

TAC's callback registration methods are single-slot — calling on_interrupt / on_conversation_ended / on_inbound_call_twiml a second time replaces the first. That means a wrapping package (a cloud connector like tac_aws / tac_microsoft) and an end application cannot both register a callback: whoever registers last silently wins. E.g. an Azure connector that wants to inject a per-call ConversationRelay websocket_url via on_inbound_call_twiml would clobber (or be clobbered by) the app's own customizer.

This makes the additive callbacks accumulate instead of overwrite, following the standard Python split by return semantics:

Callback Returns Convention (Python-world) New behavior
on_interrupt None (side effect) fan-out listener (atexit, Django signals, add_done_callback) append; all fire in order
on_conversation_ended None (side effect) fan-out listener append; all fire in order
on_inbound_call_twiml TwiMLOptions (merged) middleware / DRF permission_classes (combined by a rule) append; layered, later wins per-field
on_message_ready str | None — the reply TAC routes to the channel single result-producer (Flask route, sys.excepthook) unchanged — single slot

on_message_ready deliberately stays single: it produces the reply sent to the user, and "two responders" has no coherent answer. That's not an exception to explain away — it's the same distinction the stdlib and major frameworks already draw between listeners (many) and result-producers (one).

Changes

  • VoiceChannel.on_inbound_call_twiml — appends. handle_incoming_call runs every registered customizer in registration order, collecting a list of per-call TwiMLOptions layers; _build_twiml_options overlays them in order (later wins per-field, same precedence model as default_twiml_options → customizer). _resolve_action_url scans the customizer layers highest-priority-first (last-registered wins).
  • TAC.on_interrupt / TAC.on_conversation_ended — append; all listeners fire in registration order (results are None; the existing sync/async handling is preserved per-callback).
  • TAC.on_message_ready — unchanged (single slot).

Behavior change (backward compat)

Registering the same callback method twice now accumulates rather than replaces. Single registration — the overwhelmingly common case — is completely unaffected. A caller that intentionally re-registered to replace would now get both fire; this is low-risk (double-registration-to-replace is unusual) but is the one semantic change, called out here for reviewers.

Tests

make check passes: ruff, mypy strict (75 files), 754 tests. New tests:

  • multiple on_inbound_call_twiml customizers layer per-field, later wins; action_url precedence resolves across layers (voice)
  • multiple on_interrupt listeners all fire in order (TAC core)
  • multiple on_conversation_ended listeners (sync + async) all fire in order (TAC core)

Why this matters / relation to other work

This is the clean foundation for connector packages composing with app callbacks: with it, an Azure/AWS connector registers its own customizer/listener through the normal TAC API, and the app's callbacks still run — no private-attribute access, no connector-specific registration API, no ordering trap. It complements (but is independent of) the per-call websocket_url work in the sibling PR.

🤖 Generated with Claude Code

…acks

Make the additive callbacks accumulate instead of overwrite, so a wrapping
package (e.g. a cloud connector) can register its own callback without
displacing an application's. Follows the standard Python split by return
semantics — fan-out for fire-and-forget listeners, layered merge for the
value-returning customizer, single slot for the one that produces the reply.

- VoiceChannel.on_inbound_call_twiml: now appends. All registered customizers
  run in registration order; each output is a per-call TwiML layer, later
  registrations winning per-field (same precedence as default_twiml_options →
  customizer). _build_twiml_options / _resolve_action_url take a list; action_url
  resolves across layers highest-priority-first.
- TAC.on_interrupt, TAC.on_conversation_ended: now append. All registered
  listeners fire in registration order (fan-out; results are None).
- TAC.on_message_ready: unchanged — stays single-slot, because it produces the
  reply TAC routes back to the channel and there can be only one responder
  (like a Flask route handler / sys.excepthook).

Behavior change: registering twice now accumulates rather than replaces. Single
registration (the overwhelmingly common case) is unaffected.

Tests: multiple customizers layer per-field + action_url precedence across
layers (voice); multiple interrupt/conversation-ended listeners all fire in
order (TAC core).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 1, 2026 23:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This PR changes TAC/VoiceChannel callback registration to support multiple listeners/customizers (append semantics) so connector packages and applications can both register callbacks without clobbering each other.

Changes:

  • Allow multiple TAC.on_interrupt and TAC.on_conversation_ended callbacks to be registered and invoked in order.
  • Allow multiple VoiceChannel.on_inbound_call_twiml customizers to be registered and layered per-field (later wins), including action_url precedence across layers.
  • Add tests covering multi-callback behavior for interrupts, conversation-ended, and inbound TwiML customization layering.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/tac/core/tac.py Switches interrupt/conversation-ended callbacks from single-slot to lists; updates triggering logic.
src/tac/channels/voice/channel.py Switches inbound TwiML customizer to a list and layers multiple TwiMLOptions outputs, including action_url resolution.
tests/test_voice_channel.py Adds tests for multi-customizer layering and action_url precedence.
tests/test_tac.py Adds test ensuring multiple conversation-ended callbacks (sync + async) fire in order.
tests/test_interrupt.py Adds test ensuring multiple interrupt callbacks fire in order.
Comments suppressed due to low confidence (1)

src/tac/core/tac.py:427

  • With multiple interrupt callbacks, async callbacks are scheduled as separate tasks. This means (a) any exceptions raised in those tasks can become unhandled-task warnings/noisy logs, and (b) while callbacks are invoked in registration order, async side effects will run concurrently and may complete out of order—potentially contradicting the docstring promise of ordered firing. Consider scheduling a single wrapper task that awaits each callback sequentially (preserving order) and catches/logs exceptions per callback, or update the docstring to clarify that async callbacks are fire-and-forget and may complete out of order.
        for callback in self._interrupt_callbacks:
            result = callback(conversation_context, interrupt_data)
            if inspect.isawaitable(result):
                try:
                    asyncio.ensure_future(result)
                except RuntimeError:
                    # Close the coroutine to prevent "was never awaited" warning

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +201 to +206
# Run every registered customizer in registration order; each output
# is one per-call layer, later ones winning per-field.
per_call_layers: list[TwiMLOptions] = []
if twiml_request is not None:
for customizer in self._inbound_call_twiml_customizers:
per_call_layers.append(await customizer(twiml_request))
@ryanrouleau

Copy link
Copy Markdown
Collaborator Author

Closing — superseded by a cleaner framing of the underlying problem.

This PR made TAC's callbacks multi-registrable so an Azure connector could inject per-call ConversationRelay TwiML (a per-call WebSocket URL for Hosted Agents / Foundry sandbox affinity) alongside an application's own on_inbound_call_twiml customizer, without one clobbering the other.

But that customizer-vs-customizer contention only exists because the Hosted Agents server was going to inject its server-owned values by registering a customizer. The better design is for the server to pass its transport facts (the per-call URL) as an input to handle_incoming_call, the way TACFastAPIServer already delegates all TwiML generation to the channel. The channel stays the single owner of TwiML (customizers, layering, URL resolution); every server — FastAPI, Hosted Agents, or other — just hands it facts. No server registers a customizer, so there's no contention for multi-registration to solve.

The dev's on_inbound_call_twiml still runs (the channel runs it); the server's URL is supplied as a parameter layered on top. See the follow-up on #75 for the server→channel input contract.

Multi-callback registration may still have independent merit (letting a wrapping package and an app both register fire-and-forget listeners like on_conversation_ended), but it should be proposed on its own terms, not as a dependency of the Azure per-call-URL work. Not deleting the branch in case we revisit it standalone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants