feat(workflows): introduce structured WarmTransferError with SIP facts (#7200) - #7253
feat(workflows): introduce structured WarmTransferError with SIP facts (#7200)#7253rrfunde wants to merge 6 commits into
Conversation
livekit#7200) Expose WarmTransferError and WarmTransferFailure carrying disconnect_reason, call_status, and cause on warm transfer failures.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| logger.exception("could not dial human agent") | ||
| self._set_result(ToolError("could not dial human agent")) | ||
| err = WarmTransferError("could not dial human agent", code=WarmTransferFailure.DIAL_FAILED) | ||
| err.__cause__ = e |
There was a problem hiding this comment.
🟨 Dial errors retain sensitive context
When dialing fails, WarmTransferError retains the caught exception through implicit __context__. Downstream traceback rendering can expose provider payloads or credentials.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f870f9806e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._destination_disconnect_reason: rtc.DisconnectReason.ValueType | None = None | ||
| self._destination_call_status: str | None = None |
There was a problem hiding this comment.
When TwilioConnectorWarmTransferTask is used, its no-op _setup_origination() override prevents these fields from being initialized. If the staging room later closes without first emitting participant_disconnected—for example after a server shutdown or connection failure—_on_human_agent_room_close() raises AttributeError while reading _destination_disconnect_reason and never calls _set_result, potentially leaving the caller on hold indefinitely. Initialize this shared failure state directly in the base constructor rather than in the SIP-specific hook.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…_init__ and prevent exception cause leakage
|
Addressed the review feedback:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 026b35534a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| task._set_io_enabled = MagicMock() | ||
| task.done = MagicMock(return_value=False) | ||
| task.complete = MagicMock() | ||
| task._human_agent_failed_fut = asyncio.get_event_loop().create_future() |
There was a problem hiding this comment.
On Python 3.14, which this package explicitly supports via requires-python = ">=3.10,<3.15", these synchronous tests call asyncio.get_event_loop() when no current loop exists and raise RuntimeError before exercising the callback; the same issue occurs on line 99. Mark these tests async or create the futures within an explicitly managed event loop so the unit suite remains runnable on supported Python versions.
AGENTS.md reference: AGENTS.md:L128-L130
Useful? React with 👍 / 👎.
|
Fixed: Marked the warm transfer unit tests as async so |
|
Addressed review feedback: used |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bbada48a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if participant.identity == self._human_agent_identity: | ||
| self._destination_disconnect_reason = participant.disconnect_reason | ||
| self._destination_call_status = participant.attributes.get("sip.callStatus") |
There was a problem hiding this comment.
When the SIP destination leaves with USER_UNAVAILABLE (the reason exercised by the new test) or another reason outside the default close list, this callback only caches the departure. RoomIO._on_participant_disconnected closes the staging session only for CLIENT_INITIATED, ROOM_DELETED, and USER_REJECTED (voice/room_io/types.py:17-21), so no room disconnected event follows, _on_human_agent_room_close never calls _set_result, and the caller can remain on hold indefinitely. Complete the task from this participant callback or explicitly close the staging session for every destination departure.
Useful? React with 👍 / 👎.
|
Addressed review feedback:
|
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| with contextlib.suppress(asyncio.InvalidStateError): | ||
| self._human_agent_failed_fut.set_result(None) | ||
| reason_name = ( | ||
| rtc.DisconnectReason.Name(participant.disconnect_reason) | ||
| if participant.disconnect_reason is not None | ||
| else "UNKNOWN_REASON" | ||
| ) | ||
| self._set_result( | ||
| WarmTransferError( | ||
| f"destination left: {reason_name}", | ||
| code=WarmTransferFailure.DESTINATION_LEFT, | ||
| disconnect_reason=participant.disconnect_reason, | ||
| call_status=self._destination_call_status, | ||
| ) |
There was a problem hiding this comment.
🟡 Failed transfer leaks child session
When disconnection and origination finish together, _set_result completes the transfer before on_enter stores the returned session. The human-agent session then remains active after the failed transfer.
Learn more
The disconnect callback can run while _originate_human_agent is finishing. It completes the task while _human_agent_sess is still None, so _set_result has no session to shut down. If the dial task also becomes done before asyncio.wait resumes, on_enter treats the dial as successful and assigns its returned session after completion. Nothing subsequently shuts that session down.
Example: The SIP destination answers and immediately hangs up. The disconnect event completes the transfer with DESTINATION_LEFT, while the create-participant request returns in the same event-loop turn. The failed transfer resumes the caller, but its human-agent session and room remain active.
Recommended fix: In on_enter, treat a completed _human_agent_failed_fut or self.done() as failure even when dial_human_agent_task is also done. If the dial task already returned an AgentSession, shut it down instead of assigning it.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d1adb6770
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| err = WarmTransferError( | ||
| "could not dial human agent", code=WarmTransferFailure.DIAL_FAILED | ||
| ) | ||
| self._set_result(err) |
There was a problem hiding this comment.
When _originate_human_agent() fails, this creates a new WarmTransferError but discards the caught API/Twilio exception. Passing the wrapper to Future.set_exception() does not establish exception chaining, so callers receive DIAL_FAILED with __cause__ is None and cannot inspect the underlying SIP status or failure details. Capture the exception and explicitly attach it as the cause before completing the task.
Useful? React with 👍 / 👎.
1 participant
Fixes #7200
Summary
Introduces a typed
WarmTransferError(ToolError)subclass andWarmTransferFailureenum to preserve platform facts (disconnect reason, SIP call status, dial failure causes) when a warm transfer fails.Changes
WarmTransferFailureenum andWarmTransferError(ToolError)carryingcode,disconnect_reason,call_status, andreason.participant_disconnectedon the human-agent room to recordparticipant.disconnect_reasonandsip.callStatus.DESTINATION_LEFTandROOM_CLOSEDon human-agent room close.SipCallError) as__cause__onDIAL_FAILED._merge_calls.tests/test_warm_transfer_error.py.