Conversation
SFU events are the implementation detail of the getstream plugin
- register required events by default - removed unused EventEmitter base class - update StreamEdge
📝 WalkthroughWalkthroughEdge and GetStream plugin integration refactor: EdgeTransport is now a generic ABC with an EventManager and new Call protocol; core types (TrackType, Connection, Participant) and TTS/audio handling were modernized; SFU protobuf events moved into the GetStream plugin and StreamEdge maps SFU types → core types and emits core events. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent
participant Edge as EdgeTransport
participant Events as EventManager
participant Stream as GetStream
Agent->>Edge: create_call(call_id, agent_user_id, call_type)
Edge->>Stream: provision/get StreamCall
Stream-->>Edge: StreamCall
Edge->>Edge: create_audio_track(), publish_tracks(audio, video)
Edge->>Events: register Track/Audio events
Stream-->>Edge: raw SFU event
Edge->>Edge: convert SFU → core Participant/TrackType
Edge->>Events: emit TrackAddedEvent(participant, TrackType)
Events-->>Agent: on_track_added handler invoked
Agent->>Edge: say(text, participant)
Edge->>Events: emit TTSAudioEvent(participant, pcm)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/getstream/_generate_sfu_events.py (1)
47-52:⚠️ Potential issue | 🟡 MinorBare
except Exception:in generated code template violates guidelines.The
HEADER_LINEStemplate includes a bare exception handler that will propagate into the generatedsfu_events.pyfile. Consider catching a more specific exception type.Proposed fix
"def _to_dict(message) -> Dict[str, Any]:", " try:", " return MessageToDict(message, preserving_proto_field_name=True)", -" except Exception:", +" except (ValueError, TypeError):", " return {}",As per coding guidelines: Never write
except Exception as e- use specific exception handling.
🤖 Fix all issues with AI agents
In `@agents-core/vision_agents/core/events/manager.py`:
- Around line 174-187: The loop that registers classes (for event_class in
event_classes) early-returns when seeing a class whose name
endswith("BaseEvent"), causing later classes to be skipped; change the behavior
to skip that one entry and continue the loop instead of returning (replace the
`return` in the branch `elif event_class.__name__.endswith("BaseEvent"):` with
`continue`) so subsequent event classes are still processed and self._events,
logger.debug, and the error/warning handling remain unchanged.
In `@tests/test_events.py`:
- Around line 47-64: Rename the test function to reflect its actual behavior:
change the function name from test_register_events_from_module_raises_name_error
to something like test_register_events_from_module_and_handler_execution so it
matches that it registers events via EventManager.register_events_from_module on
the dummy module, subscribes a handler with manager.subscribe, sends an event
with manager.send, awaits manager.wait, and asserts the handler ran; update only
the test function name to keep the assertions and setup unchanged.
- Around line 129-140: The test function name and its assertion disagree: rename
the test function test_send_unknown_event_type_raises_key_error to
test_send_unknown_event_type_raises_runtime_error to reflect the actual
behavior, or if KeyError is intended change the assertion to expect KeyError
instead; locate the test function definition
(test_send_unknown_event_type_raises_key_error) in tests/test_events.py and
update either the function name or the pytest.raises(...) call accordingly when
exercising EventManager.send with UnregisteredEvent (the dynamic dataclass) and
ignore_unknown_events=False.
🧹 Nitpick comments (2)
plugins/getstream/tests/test_sfu_events.py (1)
46-47: Considerpytest.approxfor floating-point comparisons.Using
pytest.approxis more idiomatic and provides clearer error messages on failure.Optional improvement
- assert abs(received_audio_events[0].level - 0.85) < 0.01 + assert received_audio_events[0].level == pytest.approx(0.85, abs=0.01)- assert abs(received_audio_events[0].level - 0.95) < 0.01 + assert received_audio_events[0].level == pytest.approx(0.95, abs=0.01)Also applies to: 62-63
tests/test_events.py (1)
109-109: Consider usingnonlocalfor consistency.The test at line 81-86 uses
nonlocal valuefor mutating a counter, while here a dict workaround is used. For consistency within the same file, consider usingnonlocalhere as well.♻️ Suggested refactor
- # Counter to ensure recursive handler is invoked - recursive_counter = {"count": 0} + # Counter to ensure recursive handler is invoked + recursive_count = 0 `@manager.subscribe` async def failing_handler(event: ValidEvent): raise RuntimeError("Intentional failure") `@manager.subscribe` async def exception_handler(event: ExceptionEvent): + nonlocal recursive_count # Increment the counter each time the exception handler runs - recursive_counter["count"] += 1 + recursive_count += 1 # Re-raise the exception only once to trigger a second recursion - if recursive_counter["count"] == 1: + if recursive_count == 1: raise ValueError("Re-raising in exception handler") manager.send(ValidEvent(field=10)) await manager.wait() # After processing, the recursive counter should be 2 (original failure + one re-raise) - assert recursive_counter["count"] == 2 + assert recursive_count == 2
- replace the usages of `getstream.Call` outside the getstream plugin
- Remove QueuedAudioTrack - it's a duplicate - minor cleanup in StreamEdge
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents-core/vision_agents/core/tts/tts.py (1)
273-285:⚠️ Potential issue | 🟡 MinorBroad exception handling violates coding guidelines.
The
except Exception as ecatch-all swallows every possible error type. Consider catching more specific exceptions (e.g.,TypeError,ValueError, or provider-specific errors from the TTS synthesis) to avoid masking unexpected failures.As per coding guidelines: "Never write
except Exception as e- use specific exception handling."
🤖 Fix all issues with AI agents
In `@agents-core/vision_agents/core/edge/edge_transport.py`:
- Line 5: The import of AudioStreamTrack and the concrete return type in
create_audio_track() break the generics-based decoupling; change
create_audio_track() to return the generic type T_AudioTrack instead of
getstream.video.rtc.AudioStreamTrack, remove the direct AudioStreamTrack import
from this module, and ensure any references in edge_transport.py use the
T_AudioTrack generic (and not the concrete class) so the core public API no
longer exposes getstream types; also scan for usages of AudioStreamTrack or
PcmData in this module and adapt them to the abstract/generic types or local
protocol interfaces to eliminate the direct getstream dependency.
In `@plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py`:
- Around line 377-387: The create_call method currently ignores its
agent_user_id parameter and uses self.agent_user_id, which may be None; update
create_call to use the agent_user_id argument when calling call.get_or_create
(i.e., pass data={"created_by_id": agent_user_id}) or make agent_user_id a
required parameter and validate it before calling call.get_or_create; ensure you
update any callers or add a clear check that raises if agent_user_id is missing
so call.get_or_create always receives a non-None created_by_id.
In `@tests/test_agent_tracks.py`:
- Around line 124-126: The test helper create_audio_track returns a
unittest.mock.Mock (Mock) which violates the guideline to avoid mocks; replace
it with a minimal stub object that exposes the same attribute(s) used by tests
(e.g., an object with an id attribute like "audio_track_1"). Update the
create_audio_track method to instantiate and return that lightweight stub (or a
simple dataclass/namespace) instead of Mock so behavior is explicit and tests
don't depend on unittest.mock.
🧹 Nitpick comments (6)
tests/test_agents/test_agents.py (1)
51-56: Forwardsample_rateandstereoparameters through toAudioStreamTrackinstead of hardcoding them.Currently,
DummyEdge.create_audio_track()accepts*args, **kwargsbut silently ignores them, then hardcodessample_rate=48000andchannels=2. This masks bugs when callers attempt to pass different values. The productionStreamEdgeimplementation already uses the recommended pattern—accept parameterized defaults and forward them through.♻️ Suggested adjustment
- def create_audio_track(self, *args, **kwargs) -> AudioStreamTrack: - return AudioStreamTrack( - audio_buffer_size_ms=300_000, - sample_rate=48000, - channels=2, - ) + def create_audio_track( + self, sample_rate: int = 48000, stereo: bool = True + ) -> AudioStreamTrack: + return AudioStreamTrack( + audio_buffer_size_ms=300_000, + sample_rate=sample_rate, + channels=2 if stereo else 1, + )agents-core/vision_agents/core/tts/tts.py (1)
88-114: Consider alternatives tohasattrfor type checking.The
hasattrcalls at lines 95 and 104 check for__aiter__and__iter__protocols. While this duck-typing pattern is common, the coding guidelines discouragehasattr. You could useisinstancechecks withtyping.AsyncIterableandtyping.Iterable, or usetry/exceptwith the iteration itself.As per coding guidelines: "Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python."
♻️ Proposed alternative using isinstance
+from typing import AsyncIterable, Iterable + async def _iter_pcm(self, resp: Any) -> AsyncGenerator[PcmData, None]: """Yield PcmData chunks from a provider response of various shapes.""" # Single buffer or PcmData if isinstance(resp, (PcmData,)): yield resp return # Async iterable - if hasattr(resp, "__aiter__"): + if isinstance(resp, AsyncIterable): async for item in resp: if not isinstance(item, PcmData): raise TypeError( "stream_audio must yield PcmData; wrap provider bytes via PcmData.from_response in the plugin" ) yield item return # Sync iterable - if hasattr(resp, "__iter__") and not isinstance( + if isinstance(resp, Iterable) and not isinstance( resp, (bytes, bytearray, memoryview, str) ):agents-core/vision_agents/core/edge/edge_transport.py (1)
119-125: Missing docstrings and return type annotation.
create_conversationlacks a docstring and return type annotation.add_track_subscriberalso lacks a docstring. For an abstract base class defining a public contract, these should be documented.📝 Proposed docstrings
`@abc.abstractmethod` -async def create_conversation(self, call: Call, user: User, instructions: str): +async def create_conversation(self, call: Call, user: User, instructions: str) -> Any: + """Create a conversation/chat channel for the call. + + Args: + call: The call to create conversation for. + user: The user creating the conversation. + instructions: System instructions for the conversation. + + Returns: + A conversation object for message exchange. + """ pass `@abc.abstractmethod` def add_track_subscriber(self, track_id: str) -> Optional[aiortc.VideoStreamTrack]: + """Subscribe to a remote video track. + + Args: + track_id: The ID of the track to subscribe to. + + Returns: + The subscribed video track, or None if subscription failed. + """ passagents-core/vision_agents/core/agents/agents.py (2)
718-729: Dynamic attribute access viagetattr.The
_applymethod usesgetattrtwice (lines 720, 722) for dynamic method invocation. While this is a common pattern for plugin systems, the coding guidelines discouragegetattr. Consider a registry-based approach or explicit method dispatch if this becomes problematic.Additionally, the
except Exception as eat line 726 is overly broad.As per coding guidelines: "Avoid using getattr, hasattr, delattr and setattr" and "Never write
except Exception as e."
19-19: Clarify architectural requirements for external type abstraction.The getstream imports at line 19 are part of a systemic pattern:
PcmDatais used across 9 core modules (vad, tts, stt, llm, processors, edge, utils) andAudioStreamTrackappears in both agents.py and edge_transport.py. These are third-party library types fundamental to audio processing and RTC infrastructure. Before recommending abstraction, establish whether decoupling from external library types is a stated architectural priority and whether implementation swapping is planned. If it is, create wrapper abstractions; otherwise, this pattern reflects appropriate use of external dependencies.plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
63-82:getattrusage foruser_idaccess.Line 78 uses
getattr(participant, "user_id", None)instead of direct attribute access. Since bothsfu_events.ParticipantandStreamParticipantshould haveuser_idattributes, consider direct access with appropriate type narrowing.As per coding guidelines: "Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python."
♻️ Proposed fix
def _to_core_participant( participant: sfu_events.Participant | StreamParticipant | None, ) -> Participant | None: if participant is None: return None - user_id = getattr(participant, "user_id", None) + user_id = participant.user_id if hasattr(participant, "user_id") else None + # Or with direct access if both types are guaranteed to have user_id: + # user_id = participant.user_id if not user_id: return None return Participant(original=participant, user_id=user_id)Note: If both input types are guaranteed to have
user_id, direct accessparticipant.user_idwould be preferred. Thehasattralternative still violates guidelines but is more explicit about the optional nature.
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
Outdated
Show resolved
Hide resolved
This will make creating calls more flexible, while we are still tied to `aiortc.MediaStreamTrack`, making previous generics less useful.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/test_agents/test_agents.py (1)
43-44:⚠️ Potential issue | 🟡 Minor
self.eventsoverwritten immediately after parent registers required events.Line 43 calls
super().__init__(), which (per theEdgeTransportbase class) creates anEventManagerand registers the required events (AudioReceivedEvent,TrackAddedEvent,TrackRemovedEvent,CallEndedEvent). Line 44 then replaces it with a bareEventManager(), discarding those registrations. If any test path or theAgentinternals checks for registered events, this will silently break.Suggested fix: remove the redundant reassignment
def __init__( self, exc_on_join: Optional[Exception] = None, exc_on_publish_tracks: Optional[Exception] = None, ): super(DummyEdge, self).__init__() - self.events = EventManager() self.exc_on_join = exc_on_join self.exc_on_publish_tracks = exc_on_publish_tracksplugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (4)
117-128: 🛠️ Refactor suggestion | 🟠 MajorReplace broad
except Exception as ewith specific exception types.Lines 120–126 already handle
asyncio.TimeoutErrorandRuntimeError. The catch-all on line 127 violates the project guideline. If you need to handle additional known failure modes from the RTC stack, catch those specifically (e.g.,OSError,ConnectionError).Suggested fix
except RuntimeError as e: if "asynchronous generator" in str(e): logger.debug(f"Ignoring async generator error during shutdown: {e}") else: raise - except Exception as e: - logger.error(f"Error during connection close: {e}") + except OSError as e: + logger.error(f"Error during connection close: {e}")As per coding guidelines: "Never write
except Exception as e- use specific exception handling."
313-323: 🛠️ Refactor suggestion | 🟠 MajorReplace
hasattrwithisinstancecheck to distinguish event types.The event is already typed as a union of
TrackUnpublishedEvent | ParticipantLeftEvent. Usingisinstanceis both type-safe and avoids thehasattrguideline violation. Also,event.payload is not Noneon line 314 is redundant since line 302 already guards against that.Suggested fix
# Determine which tracks to remove - if hasattr(event.payload, "type") and event.payload is not None: + if isinstance(event, sfu_events.TrackUnpublishedEvent): # TrackUnpublishedEvent - single track tracks_to_remove = [event.payload.type] event_desc = "Track unpublished"As per coding guidelines: "Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python."
589-595: 🛠️ Refactor suggestion | 🟠 MajorBroad
except Exception as ewhen opening browser.
webbrowser.opentypically raiseswebbrowser.ErrororOSError. Catch those specifically.Suggested fix
try: # Run webbrowser.open in a separate thread to avoid blocking the event loop await asyncio.to_thread(webbrowser.open, url) logger.info("✅ Browser opened successfully!") - except Exception as e: + except (webbrowser.Error, OSError) as e: logger.error(f"❌ Failed to open browser: {e}") logger.warning(f"Please manually open this URL: {url}")As per coding guidelines: "Never write
except Exception as e- use specific exception handling."
500-514:⚠️ Potential issue | 🟡 Minor
send_custom_event— clean guard, butself.agent_user_idmay beNone.Line 514 passes
self.agent_user_idwhich could beNoneifcreate_userwas never called. This is the same root cause as thecreate_callissue. Consider adding a guard or makingagent_user_idrequired at construction time.
🤖 Fix all issues with AI agents
In `@plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py`:
- Around line 75-82: Replace the getattr call with direct attribute access since
sfu_events.Participant/StreamParticipant always define user_id: after the
existing None guard for participant, read user_id = participant.user_id (instead
of getattr) and keep the subsequent None check and return
Participant(original=participant, user_id=user_id); this removes the getattr
usage while preserving the existing None-handling logic around participant and
user_id.
- Around line 497-498: The close() method currently only nulls self._call which
can leak resources; update close() to fully tear down the stream connection by
(1) calling or awaiting the underlying StreamConnection.close() /
ConnectionManager disconnect routine on self._real_connection (or invoking its
cleanup/shutdown API) to stop network activity, (2) stopping and removing any
MediaStreamTracks and clearing self._track_map and self._pending_tracks, (3)
cancelling/awaiting any pending tasks related to conversation and clearing
self.conversation, and finally setting self._call and self._real_connection to
None; reference the methods/attributes close, _real_connection, _track_map,
_pending_tracks, conversation and ensure any async shutdown is awaited to
prevent leaks.
🧹 Nitpick comments (3)
tests/test_agents/test_agents.py (1)
56-61:create_audio_track— minor signature deviation from abstract, but acceptable for a test double.The abstract method declares no positional/keyword args beyond
self, but this override accepts*args, **kwargs. Functionally harmless, though aligning signatures makes intent clearer.agents-core/vision_agents/core/edge/edge_transport.py (1)
133-148:create_conversationusesCallprotocol — minor docstring gap;send_custom_eventis clean.
create_conversation(line 134) andadd_track_subscriber(line 138) lack docstrings, unlike every other abstract method in this class. Consider adding them for consistency.plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
273-287: Dead guard:if not is_agent_trackis alwaysTruehere.Agent tracks already trigger an early return on line 222–226, so by line 278,
is_agent_trackis guaranteedFalse. The condition can be removed to reduce cognitive noise.Suggested simplification
if track_id: # Store with correct type from SFU self._track_map[track_key] = {"track_id": track_id, "published": True} - # Only emit TrackAddedEvent for remote participants, not for agent's own tracks - if not is_agent_track: - # NOW spawn TrackAddedEvent with correct type - self.events.send( - events.TrackAddedEvent( - plugin_name="getstream", - track_id=track_id, - track_type=track_type, - participant=_to_core_participant(event.participant), - ) - ) + # NOW spawn TrackAddedEvent with correct type + self.events.send( + events.TrackAddedEvent( + plugin_name="getstream", + track_id=track_id, + track_type=track_type, + participant=_to_core_participant(event.participant), + ) + )
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
Outdated
Show resolved
Hide resolved
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py`:
- Around line 55-60: The helper _to_core_track_type currently raises ValueError
for unknown StreamTrackType which can crash async handlers; change
_to_core_track_type (and its use of _TRACK_TYPE_MAP/StreamTrackType) to return
None (or an UNSPECIFIED sentinel) and log a warning when the input is
unrecognized instead of raising; then update callers _on_track_published and
_on_track_removed to guard the return value (e.g., track_type =
_to_core_track_type(...); if track_type is None: return) so unknown track types
are skipped and do not mutate _track_map or _pending_tracks.
🧹 Nitpick comments (1)
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
297-322: Useisinstanceinstead ofhasattrfor event type branching.Line 313 uses
hasattr(event.payload, "type")to distinguishTrackUnpublishedEventfromParticipantLeftEvent. Since the type union is already declared on line 298,isinstanceis both safer and compliant with the project's coding guidelines.Proposed fix
- if hasattr(event.payload, "type") and event.payload is not None: + if isinstance(event, sfu_events.TrackUnpublishedEvent) and event.payload is not None: # TrackUnpublishedEvent - single track tracks_to_remove = [event.payload.type] event_desc = "Track unpublished"As per coding guidelines: "Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python."
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/getstream/vision_agents/plugins/getstream/sfu_events.py (1)
1486-1507:⚠️ Potential issue | 🟡 MinorThe
participantproperty/setter pattern silently discards writes.In the five event classes that have a
participantproperty (e.g.,ParticipantJoinedEvent,ParticipantLeftEvent,ParticipantUpdatedEvent,TrackPublishedEvent,TrackUnpublishedEvent), the setter stores to_participantbut the getter never reads it — it always derives fromself.payload. This means:event.participant = some_participant # stored in _participant, silently ignored event.participant # returns from payload, not from what was just setThe comment says "to satisfy dataclass
__init__," which is fair, but any caller who writes toparticipantafter construction gets a silent no-op. Consider at minimum logging a warning in the setter, or documenting this read-only-from-payload behavior more prominently.Also applies to: 1535-1556, 1615-1636, 2090-2111, 2139-2167
🧹 Nitpick comments (2)
plugins/getstream/vision_agents/plugins/getstream/sfu_events.py (2)
19-23: Bareexcept Exceptionin_to_dict— prefer a specific exception type.The
_to_dicthelper swallows everyException, silently returning{}. Protobuf'sMessageToDictraises specific errors (e.g.,AttributeError,ValueError). Catching broadly here can mask real bugs — a corrupt payload, a wrong type passed in — and you'd never know. The empty dict return is indistinguishable from a valid-but-empty message.I understand this is auto-generated, so the fix belongs in the generator, but worth noting.
Suggested narrower catch
def _to_dict(message) -> Dict[str, Any]: try: return MessageToDict(message, preserving_proto_field_names=True) - except Exception: + except (AttributeError, ValueError): return {}As per coding guidelines, "Never write
except Exception as e- use specific exception handling".
520-526: Pervasivegetattrusage throughout all event classes.Every
__getattr__fallback and every property accessor usesgetattr(self.payload, ...)rather than direct attribute access. The coding guidelines say to avoidgetattr; preferself.payload.field_namewith proper type narrowing. Since the protobuf message types do have these attributes as concrete fields, direct access would be both safer and more readable.Again — auto-generated, so this is a generator-level concern. But for a file this large (2360 lines), the pattern compounds: a typo in a field name string literal silently returns
Noneinstead of raising.As per coding guidelines, "Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python".
Also applies to: 554-560, 603-609, 637-643, 679-685, 725-731, 756-762, 797-803, 835-841, 880-886, 918-924, 960-966, 994-1000, 1021-1027, 1056-1062, 1090-1096, 1131-1137, 1169-1175, 1217-1223, 1325-1331, 1386-1392, 1420-1426, 1471-1477, 1520-1526, 1569-1575, 1600-1606, 1649-1655, 1684-1690, 1718-1724, 1797-1803, 1986-1992, 2034-2040, 2075-2081, 2124-2130, 2180-2186, 2257-2263, 2313-2319
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (4)
124-128:⚠️ Potential issue | 🟡 MinorAvoid bare
except Exception as e— use specific exception types.The coding guidelines prohibit
except Exception as e. TheRuntimeErrorcase is already handled above; consider catching specific expected exceptions (e.g.,ConnectionError,OSError) for the remaining case, or at minimum re-raising unexpected ones.Suggested fix
except RuntimeError as e: if "asynchronous generator" in str(e): logger.debug(f"Ignoring async generator error during shutdown: {e}") else: raise - except Exception as e: - logger.error(f"Error during connection close: {e}") + except OSError as e: + logger.error(f"Network error during connection close: {e}")As per coding guidelines: "Never write
except Exception as e- use specific exception handling."
585-589:⚠️ Potential issue | 🟡 MinorAnother bare
except Exception as e— use specific exception types.Same guideline violation.
webbrowser.opentypically raisesOSErroror browser-specific errors.Suggested fix
- except Exception as e: - logger.error(f"❌ Failed to open browser: {e}") + except OSError as e: + logger.error(f"❌ Failed to open browser: {e}")As per coding guidelines: "Never write
except Exception as e- use specific exception handling."
311-316:⚠️ Potential issue | 🟡 MinorAvoid
hasattr— use structural type discrimination instead.The coding guidelines prohibit
hasattr. Since_on_track_removedalready knows the two concrete event types (TrackUnpublishedEventandParticipantLeftEvent), useisinstanceto discriminate:Suggested fix
- if hasattr(event.payload, "type") and event.payload is not None: - # TrackUnpublishedEvent - single track - tracks_to_remove = [event.payload.type] - event_desc = "Track unpublished" + if isinstance(event, sfu_events.TrackUnpublishedEvent): + tracks_to_remove = [event.payload.type] + event_desc = "Track unpublished" else: # ParticipantLeftEvent - all published tracksAs per coding guidelines: "Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python."
383-449:⚠️ Potential issue | 🟡 MinorAddress context manager cleanup and fix coding guideline violations.
The concern about
__aenter__()at line 441 is partially valid: while the code manually enters the context manager without explicitly calling__aexit__(), proper cleanup exists throughStreamConnection.close()→connection.leave(). However, mixing explicit context manager entry with manual lifecycle management is unconventional. Ensure the cleanup contract is clearly documented.Additionally, fix two coding guideline violations:
- Line 126: Replace
except Exception as ewith specific exception types per guidelines (currently catches too broadly).- Line 313: Replace
hasattr(event.payload, "type")with direct attribute access per guidelines.
🧹 Nitpick comments (2)
agents-core/vision_agents/core/edge/edge_transport.py (1)
130-136:create_conversationusesCallinstead ofT_Call, and both methods here lack docstrings.
joinandcreate_calluse the genericT_Call, butcreate_conversationon Line 131 accepts the bareCallprotocol. This inconsistency means a subclass parameterized with a specific call type (e.g.,StreamCall) won't get type-checked correctly at this boundary. Also,create_conversationandadd_track_subscriberare the only abstract methods without docstrings.Suggested fix
`@abc.abstractmethod` - async def create_conversation(self, call: Call, user: User, instructions: str): + async def create_conversation(self, call: T_Call, user: User, instructions: str): + """Create a conversation channel associated with the call. + + Args: + call: The call to associate the conversation with. + user: The user creating the conversation. + instructions: System instructions for the conversation. + """ pass `@abc.abstractmethod` def add_track_subscriber(self, track_id: str) -> Optional[aiortc.VideoStreamTrack]: + """Subscribe to a video track by ID. + + Args: + track_id: The ID of the track to subscribe to. + + Returns: + The video stream track if available, or None. + """ passplugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py (1)
275-286: Dead guard:if not is_agent_trackis alwaysTruehere.The early return on line 221-225 already exits when
is_agent_trackisTrue, so by the time execution reaches line 277,is_agent_trackis guaranteedFalse. This condition is dead code — it reads as meaningful but never gates anything.Suggested fix
if track_id: # Store with correct type from SFU self._track_map[track_key] = {"track_id": track_id, "published": True} - # Only emit TrackAddedEvent for remote participants, not for agent's own tracks - if not is_agent_track: - # NOW spawn TrackAddedEvent with correct type - self.events.send( - events.TrackAddedEvent( - plugin_name="getstream", - track_id=track_id, - track_type=track_type, - participant=_to_core_participant(event.participant), - ) - ) + # Emit TrackAddedEvent with correct type from SFU + self.events.send( + events.TrackAddedEvent( + plugin_name="getstream", + track_id=track_id, + track_type=track_type, + participant=_to_core_participant(event.participant), + ) + )
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents-core/vision_agents/core/agents/agents.py (1)
992-995: 🛠️ Refactor suggestion | 🟠 Major
getattronParticipant— prefer direct attribute access.
participantis typed asParticipant(a dataclass with auser_idfield). The defensivegetattr(participant, "user_id", None)is unnecessary and violates the coding guidelines. Use direct access instead.♻️ Proposed fix
if ( participant - and getattr(participant, "user_id", None) != self.agent_user.id + and participant.user_id != self.agent_user.id ):As per coding guidelines: "Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python".
🧹 Nitpick comments (3)
agents-core/vision_agents/core/agents/agents.py (3)
718-729:getattrand bareexcept Exceptionviolate coding guidelines.Lines 720/722 use
getattrfor dynamic dispatch, and Line 726 catches the broadException. The coding guidelines explicitly prohibit both patterns. Consider defining aLifecycleprotocol/ABC with explicitstart/stop/closemethods that subclasses implement, replacing the string-based dispatch. If the dynamic approach must stay, at minimum narrow the exception type.♻️ Sketch: protocol-based approach
- async def _apply(self, function_name: str, *args, **kwargs): - subclasses = [...] - for subclass in subclasses: - if ( - subclass is not None - and getattr(subclass, function_name, None) is not None - ): - func = getattr(subclass, function_name) - if func is not None: - try: - await await_or_run(func, *args, **kwargs) - except Exception as e: - self.logger.exception(...) + async def _apply(self, function_name: str, *args, **kwargs): + subclasses = [...] + for subclass in subclasses: + if subclass is None: + continue + func = getattr(subclass, function_name, None) # dynamic dispatch - unavoidable here + if func is not None: + try: + await await_or_run(func, *args, **kwargs) + except (RuntimeError, ConnectionError, OSError) as e: + self.logger.exception( + f"Error calling {function_name} on {subclass.__class__.__name__}: {e}" + )As per coding guidelines: "Avoid using getattr, hasattr, delattr and setattr; prefer normal attribute access in Python" and "Never write
except Exception as e- use specific exception handling".
919-931: Broadexcept Exceptionpattern persists across multiple handlers.This handler (and several others at Lines 801, 1028, 1044, 1061) catch bare
Exception. The coding guidelines require specific exception types. While these are largely pre-existing, the_on_agent_sayhandler in particular could mask TTS-specific errors that deserve different handling (e.g., connection errors vs. encoding errors).As per coding guidelines: "Never write
except Exception as e- use specific exception handling".
1421-1429: Inconsistent use ofTrackType.VIDEO.valuevs.TrackTypeenum elsewhere.Line 1423 passes
TrackType.VIDEO.value(anint) for thetypefield, while_on_track_addedat Line 1158-1160 passes theTrackTypeenum directly. SinceTrackTypeis anIntEnumthis won't break equality checks, but the inconsistency is confusing. Prefer the enum form for consistency.♻️ Proposed fix
- type=TrackType.VIDEO.value, + type=TrackType.VIDEO,
2 participants
Goal - reduce dependency on
getstreamlibrary across vision_agents (exceptplugins/getstream)Summary by CodeRabbit
New Features
Refactor
Documentation
Tests