src/phonic/conversations/raw_client.py (1)

1136-1139: Docstring return type inconsistent with actual return type.

The docstring states ConversationsSocketClient but the actual return type annotation is typing.Union[ConversationsSocketClient, ReconnectableConversationsSocketClient]. Consider updating the docstring to reflect the union type.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/raw_client.py` around lines 1136 - 1139, Update the
docstring return type to match the function's annotated return type: change the
Returns section to indicate typing.Union[ConversationsSocketClient,
ReconnectableConversationsSocketClient] (or a readable union like
"ConversationsSocketClient | ReconnectableConversationsSocketClient") so it
aligns with the actual return annotation; reference the types
ConversationsSocketClient and ReconnectableConversationsSocketClient and ensure
the docstring wording matches the function's signature.

src/phonic/conversations/reconnectable_socket_client.py (3)

284-297: Sync recv() lacks lock protection compared to async version.

The async version at lines 147-150 acquires the lock before checking _user_closed and calling _reconnect(). The sync version here doesn't use any lock, which could lead to race conditions if close() is called concurrently:

1. Thread A: recv() starts reconnecting, sleeps at line 294
2. Thread B: close() sets _user_closed = True and closes connection
3. Thread A: wakes up, checks _user_closed (True), but _reconnect() isn't called with any synchronization

Consider adding thread-safe synchronization matching the async version's pattern.

Suggested fix (assuming lock is added to __init__)

     def recv(self) -> typing.Any:
         while True:
             try:
                 msg = self._inner.recv()
                 self._observe_message(msg)
                 return msg
             except ConnectionClosed as exc:
                 if not self._should_reconnect(exc):
                     raise
                 self._reconnect_attempts += 1
                 time.sleep(_reconnect_delay_sec(self._reconnect_attempts))
-                if self._user_closed:
-                    raise exc
-                self._reconnect()
+                with self._lock:
+                    if self._user_closed:
+                        raise exc
+                    self._reconnect()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/reconnectable_socket_client.py` around lines 284 -
297, The sync recv() is missing lock protection around the _user_closed check
and _reconnect() call; add a thread lock (e.g., self._lock created in __init__)
and, inside the except ConnectionClosed block, acquire the lock before checking
self._user_closed and invoking self._reconnect(), then release the lock after
_reconnect() (or use a context manager) to mirror the async version's
synchronization in recv() and prevent races with close().

---

86-93: Consider logging suppressed exceptions for debugging.

The static analysis tool flags the try-except-pass pattern here. While swallowing exceptions during message observation is acceptable (non-critical path), consider adding debug logging to aid troubleshooting.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/reconnectable_socket_client.py` around lines 86 -
93, The except-pass in _observe_message suppresses errors and loses context;
modify the except block in _observe_message to log the caught exception
(including stacktrace) before continuing, e.g., call an internal logger like
self._logger.exception or self._logger.debug with the exception details and a
short message referencing _observe_json, then continue to swallow the exception
so behavior is unchanged. Ensure you reference _observe_message and
_observe_json when adding the log entry so the log clearly ties to this code
path.

---

100-106: Consider logging suppressed exceptions during connection cleanup.

Same as above - swallowing exceptions during cleanup is acceptable, but logging at debug level would help diagnose issues in production.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/reconnectable_socket_client.py` around lines 100 -
106, In _close_current_connection, avoid silently swallowing exceptions from
awaiting self._cm.__aexit__(None, None, None); catch Exception as e and log the
exception at debug level so cleanup failures are recorded. Update the except
block in _close_current_connection to capture the exception object and call the
appropriate logger (e.g., self._logger.debug or module logger.debug) with a
short message and the exception info so you still suppress the error but retain
diagnostic details about failures during self._cm teardown.

src/phonic/client.py (1)

55-57: Minor docstring inconsistency with actual type signature.

The docstring indicates typing.Optional[bool] but the parameter is typed as bool = False. This is a minor inconsistency but could confuse users reading the documentation.

Suggested docstring fix

-    reconnect_conversation_on_abnormal_disconnect : typing.Optional[bool]
+    reconnect_conversation_on_abnormal_disconnect : bool
         When True, the conversations WebSocket automatically reconnects after an abnormal close (code 1006) using reconnect_conv_id. Defaults to False until stable in production.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/client.py` around lines 55 - 57, Docstring for the parameter
reconnect_conversation_on_abnormal_disconnect is inconsistent with its
signature: update the docstring to declare the type as bool (and note the
default is False) to match the parameter signature
(reconnect_conversation_on_abnormal_disconnect: bool = False) in the client
module, or alternatively change the function signature to typing.Optional[bool]
if you intend to allow None; ensure the docstring and the symbol
reconnect_conversation_on_abnormal_disconnect are consistent and mention the
default behavior (defaults to False).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/phonic/conversations/websocket_connect.py (1)

129-145: Use safer context acquisition to prevent leaks if wrapper construction fails.

Both reconnectable openers enter the websocket context before creating the reconnectable client wrapper. If wrapper construction raises, the already-open context may not be closed.

♻️ Suggested lifecycle hardening

 `@contextmanager`
 def open_reconnectable_conversations_socket_sync(
@@
-    try:
-        cm = websockets_sync_client.connect(ws_url, additional_headers=headers)
-        protocol = cm.__enter__()
-    except InvalidWebSocketStatus as exc:
-        _raise_api_error_for_invalid_websocket_status(exc, headers)
-    client = ReconnectableConversationsSocketClient(
-        initial_cm=cm,
-        initial_protocol=protocol,
-        client_wrapper=client_wrapper,
-        downstream_websocket_url=downstream_websocket_url,
-        request_options=request_options,
-        max_reconnect_attempts=max_reconnect_attempts,
-    )
+    cm = None
+    try:
+        cm = websockets_sync_client.connect(ws_url, additional_headers=headers)
+        protocol = cm.__enter__()
+        client = ReconnectableConversationsSocketClient(
+            initial_cm=cm,
+            initial_protocol=protocol,
+            client_wrapper=client_wrapper,
+            downstream_websocket_url=downstream_websocket_url,
+            request_options=request_options,
+            max_reconnect_attempts=max_reconnect_attempts,
+        )
+    except InvalidWebSocketStatus as exc:
+        _raise_api_error_for_invalid_websocket_status(exc, headers)
+    except Exception:
+        if cm is not None:
+            cm.__exit__(None, None, None)
+        raise

Also applies to: 163-179

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/websocket_connect.py` around lines 129 - 145, The
code currently calls protocol = cm.__enter__() before constructing
ReconnectableConversationsSocketClient, which can leak the open websocket if the
wrapper construction raises; change to acquire the context safely by entering
the context inside a try block and ensuring cm.__exit__ (or cm.close) is called
if ReconnectableConversationsSocketClient construction fails — e.g., call cm =
websockets_sync_client.connect(...), then try: protocol = cm.__enter__(); try:
client = ReconnectableConversationsSocketClient(initial_cm=cm,
initial_protocol=protocol, ...); except Exception: cm.__exit__(None, None,
None); raise; finally yield/return as before — apply the same
safe-enter-and-cleanup pattern to the other similar block (lines 163-179) so no
opened context is leaked if wrapper construction throws.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/phonic/conversations/reconnectable_socket_client.py (1)

212-232: ⚠️ Potential issue | 🟠 Major

Sync version lacks threading.Lock for race condition protection.

The async version uses asyncio.Lock() (line 75) to protect close() and the reconnect block in recv(). The sync version has no equivalent synchronization. If close() is called from one thread while recv() is in the reconnect path on another thread, state corruption can occur.

🔒 Add threading.Lock similar to async version

+import threading
+
 class ReconnectableConversationsSocketClient(EventEmitterMixin):
     """Sync conversations socket with automatic reconnect after close code 1006."""
 
     def __init__(
         self,
         *,
         initial_cm: typing.Any,
         initial_protocol: typing.Any,
         client_wrapper: BaseClientWrapper,
         downstream_websocket_url: typing.Optional[str],
         request_options: typing.Optional[RequestOptions],
     ) -> None:
         super().__init__()
         self._cm = initial_cm
         self._inner = ConversationsSocketClient(websocket=initial_protocol)
         self._client_wrapper = client_wrapper
         self._downstream_websocket_url = downstream_websocket_url
         self._request_options = request_options
         self._conversation_id: typing.Optional[str] = None
         self._reconnect_attempts = 0
         self._user_closed = False
+        self._lock = threading.Lock()

Then update close() and the reconnect block in recv() to use with self._lock:.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/reconnectable_socket_client.py` around lines 212 -
232, The sync ReconnectableConversationsSocketClient is missing a threading lock
to protect concurrent access to state during close() and the reconnect path in
recv(); add self._lock = threading.Lock() in __init__ and guard the critical
sections in close() and the reconnect branch inside recv() with with self._lock:
so mutations and checks of _user_closed, _reconnect_attempts, and
swapping/assigning self._inner (and any associated reconnect logic using
_downstream_websocket_url/_request_options) are atomic and race-free.

src/phonic/conversations/reconnectable_socket_client.py (1)

94-97: Consider logging exceptions at debug level instead of silent pass.

Multiple locations silently swallow exceptions (lines 96-97, 108-109, 159-164). While this is intentional for resilience, adding debug-level logging would aid troubleshooting without affecting normal operation.

🔧 Example for lines 94-97

         elif hasattr(msg, "dict"):
             try:
                 self._observe_json(msg.dict())
             except Exception:
-                pass
+                _logger.debug("Failed to observe message", exc_info=True)

This requires adding a logger at module level:

import logging
_logger = logging.getLogger(__name__)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/reconnectable_socket_client.py` around lines 94 -
97, Add a module-level logger and replace the silent excepts with debug-level
logs that include exception info: add "import logging" and "_logger =
logging.getLogger(__name__)" at top of reconnectable_socket_client.py, and in
the try/except blocks that swallow errors (the one wrapping
self._observe_json(msg.dict()) and the other excepts around socket/message
handling) change "except Exception: pass" to calls like _logger.debug("Exception
in <method_or_action> while processing %s", variable_or_context, exc_info=True)
so each catch logs context (e.g., method name and the msg or payload) and
exc_info=True for a stacktrace; keep the exception handling behavior otherwise
unchanged.

src/phonic/conversations/client.py (1)

519-522: Docstring return type doesn't reflect Union with ReconnectableConversationsSocketClient.

The actual return type annotation (line 506) is Union[ConversationsSocketClient, ReconnectableConversationsSocketClient], but the docstring only mentions ConversationsSocketClient.

📝 Update docstring

         Returns
         -------
-        ConversationsSocketClient
+        Union[ConversationsSocketClient, ReconnectableConversationsSocketClient]
+            Returns ReconnectableConversationsSocketClient when reconnect_conversation_on_abnormal_disconnect is enabled.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/client.py` around lines 519 - 522, The docstring's
"Returns" section currently lists only ConversationsSocketClient but the
function's return annotation is Union[ConversationsSocketClient,
ReconnectableConversationsSocketClient]; update the docstring to reflect this
union (mention both ConversationsSocketClient and
ReconnectableConversationsSocketClient and that either may be returned) so it
matches the type annotation (e.g., in the function that returns
Union[ConversationsSocketClient, ReconnectableConversationsSocketClient]).

src/phonic/conversations/reconnectable_socket_client.py (1)

27-36: Consider using TERMINAL_RECONNECT_CODES explicitly or documenting its purpose.

The constant TERMINAL_RECONNECT_CODES is defined but never referenced in _should_reconnect. The logic works correctly (only 1006 triggers reconnect, so 4800/4801 are implicitly handled), but the dead constant may confuse maintainers.

Consider either:

1. Adding a comment explaining it exists for documentation/future use, or
2. Using it explicitly in _should_reconnect for clarity:

♻️ Option 2: Use the constant explicitly

 def _should_reconnect(self, exc: BaseException) -> bool:
     if self._user_closed:
         return False
-    if _close_code(exc) != ABNORMAL_CLOSURE:
+    code = _close_code(exc)
+    if code != ABNORMAL_CLOSURE or code in TERMINAL_RECONNECT_CODES:
         return False
     if not self._conversation_id:
         return False

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/phonic/conversations/reconnectable_socket_client.py` around lines 27 -
36, TERMINAL_RECONNECT_CODES is defined but never referenced, which is
confusing; update the _should_reconnect function to explicitly check the close
code against TERMINAL_RECONNECT_CODES (and still treat ABNORMAL_CLOSURE
specially) so terminal server codes (4800/4801) are clearly handled, or
alternatively add a one-line comment next to the constant explaining it’s
intentionally kept for documentation/future use; prefer the explicit check in
_should_reconnect to make the logic and intent clear.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes