fix: payload context update #73
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an interactive example CLI at examples/chat.py to emit BUS utterances and await handled events, refactors client.emit to update payload.context in-place during BUS routing, and introduces a HiveMessage.payload setter to normalize Message/HiveMessage inputs and document getters. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatScript as examples/chat.py
participant Client as HiveMessageBusClient
participant InternalBus as InternalBus
participant RemotePeers as WebSocket/Peers
User->>ChatScript: type utterance (stdin)
ChatScript->>ChatScript: clear Event
ChatScript->>Client: emit BUS message (recognizer_loop:utterance)
Client->>InternalBus: deliver internal BUS payload (in-place context update)
Client->>RemotePeers: send payload over WebSocket
RemotePeers-->>InternalBus: (optional) emit 'speak' event
InternalBus-->>ChatScript: invoke 'speak' handler (handle_speak)
RemotePeers-->>Client: send 'ovos.utterance.handled'
Client-->>ChatScript: deliver 'ovos.utterance.handled' -> ChatScript sets Event
ChatScript-->>User: prompt for next utterance
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JarbasAl. * #73 (comment) The following files were modified: * `examples/chat.py` * `hivemind_bus_client/client.py` * `hivemind_bus_client/message.py`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
examples/chat.py (3)
18-19: Acknowledge unused parameter (minor).The
messageparameter is unused in theutt_handledfunction. While this is flagged by static analysis, it's acceptable here since the event handler signature requires this parameter. You can optionally prefix it with an underscore to signal the intent:🔎 Optional: Signal unused parameter with underscore:
-def utt_handled(message: Message): +def utt_handled(_message: Message): answered.set()
25-30: Consider adding error handling and cleanup (optional).The example would benefit from basic error handling and cleanup to demonstrate best practices:
- Handle potential connection failures
- Catch KeyboardInterrupt for graceful exit
- Show proper client disconnection
🔎 Optional: Add error handling and cleanup:
+try: while True: utt = input("> ") answered.clear() client.emit(HiveMessage(HiveMessageType.BUS, Message("recognizer_loop:utterance", {"utterances": [utt]}))) answered.wait() +except KeyboardInterrupt: + print("\nDisconnecting...") +finally: + client.close()
9-10: Add connection error handling.The example doesn't handle potential connection failures. Consider wrapping the connection attempt in a try-except block to provide helpful feedback if the identity file is missing or the connection fails.
🔎 Add error handling for connection:
+try: client = HiveMessageBusClient() - client.connect() # establish a secure end-to-end encrypted connection + client.connect() # establish a secure end-to-end encrypted connection +except RuntimeError as e: + print(f"Connection failed: {e}") + print("Tip: Run 'hivemind-client set-identity' to configure credentials") + exit(1)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/chat.py(1 hunks)hivemind_bus_client/client.py(1 hunks)hivemind_bus_client/message.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
hivemind_bus_client/client.py (2)
hivemind_bus_client/message.py (2)
payload(125-135)payload(138-144)hivemind_bus_client/identity.py (2)
site_id(109-116)site_id(119-121)
examples/chat.py (2)
hivemind_bus_client/message.py (2)
HiveMessage(44-245)HiveMessageType(9-29)hivemind_bus_client/client.py (5)
HiveMessageBusClient(93-522)connect(193-213)on_mycroft(403-405)emit(336-397)wait(64-75)
🪛 Ruff (0.14.8)
examples/chat.py
18-18: Unused function argument: message
(ARG001)
* 📝 Add docstrings to `fix/payload_context` Docstrings generation was requested by @JarbasAl. * #73 (comment) The following files were modified: * `examples/chat.py` * `hivemind_bus_client/client.py` * `hivemind_bus_client/message.py` * Update client.py --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: JarbasAI <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hivemind_bus_client/message.py (1)
145-158: Fix Message.as_dict to use serialize() method.This issue was flagged in a previous review and remains unresolved. Line 154 calls
payload.as_dict, but theovos_bus_client.Messageclass does not expose anas_dictproperty. The codebase consistently usesserialize()for Message objects (see lines 178, and util.py references in past review).🔎 Apply this diff to fix the setter:
@payload.setter def payload(self, payload: Union['HiveMessage', Message, dict, bytes]): """ Set the message payload, normalizing Message or HiveMessage inputs to their dictionary representations. Parameters: payload (HiveMessage | Message | dict | bytes): New payload to assign. If a `Message` or `HiveMessage` is provided, its dict representation is stored; otherwise the value is stored as given. """ if isinstance(payload, Message): - self._payload = payload.as_dict + self._payload = json.loads(payload.serialize()) elif isinstance(payload, HiveMessage): self._payload = payload.as_dict else: self._payload = payload
🧹 Nitpick comments (1)
examples/chat.py (1)
1-42: Clean example demonstrating HiveMessageBusClient usage.The interactive chat script effectively demonstrates connecting to HiveMind, registering handlers, and sending BUS messages. The structure is clear and the handlers are correctly implemented.
Consider adding a timeout to
answered.wait()on line 42 to prevent indefinite hangs if the utterance handling fails:if not answered.wait(timeout=30): print("Timeout waiting for response")This would make the example more robust, though the current implementation is acceptable for demonstration purposes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/chat.py(1 hunks)hivemind_bus_client/client.py(2 hunks)hivemind_bus_client/message.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
hivemind_bus_client/client.py (2)
hivemind_bus_client/message.py (2)
payload(125-143)payload(146-158)hivemind_bus_client/identity.py (2)
site_id(109-116)site_id(119-121)
🪛 Ruff (0.14.8)
examples/chat.py
24-24: Unused function argument: message
(ARG001)
🔇 Additional comments (2)
hivemind_bus_client/client.py (2)
338-351: Good documentation for the emit method.The added docstring clearly explains the method's purpose, parameters, behavior, and error conditions.
368-379: Context injection now uses consistent references.The refactored code correctly uses
updated_payload.contextfor all reads and writes, addressing the issue flagged in the previous review. However, line 379 (message.payload = updated_payload) depends on the payload setter working correctly, which currently has a bug (see comment on message.py lines 145-158).Please verify that the payload setter bug is fixed before merging, as this code will fail at runtime when attempting to assign a Message back to
message.payload.
* Update README.md * fix: defensive check for potential missing key (#69) * fix: payload context update (#73) * fix: payload context update * 📝 Add docstrings to `fix/payload_context` (#74) * 📝 Add docstrings to `fix/payload_context` Docstrings generation was requested by @JarbasAl. * #73 (comment) The following files were modified: * `examples/chat.py` * `hivemind_bus_client/client.py` * `hivemind_bus_client/message.py` * Update client.py --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: JarbasAI <[email protected]> * fix: payload context update * fix: payload context update --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Increment Version to 0.4.4a1 * Update Changelog --------- Co-authored-by: JarbasAI <[email protected]> Co-authored-by: Mike <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: JarbasAl <[email protected]>
fixes
Summary by CodeRabbit
New Features
Enhancements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.