feat:http client #60
feat:http client #60
Conversation
WalkthroughThe pull request introduces two significant changes in the HiveMind Bus Client library. First, in the Changes
Sequence DiagramsequenceDiagram
participant Client as HiveMindHTTPClient
participant Server as HiveMind HTTP Server
Client->>Server: connect()
loop Message Retrieval
Server-->>Client: retrieve messages
alt Binary Message
Client->>Client: _handle_binary()
else Hive Protocol Message
Client->>Client: _handle_hive_protocol()
end
Client->>Client: process message
end
Client->>Server: disconnect()
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
hivemind_bus_client/http.py (1)
136-168: Simplify and consolidate message parsing logicThe
on_messagemethod contains multiple type checks and parsing steps, which can be streamlined for better readability and maintenance. Consolidating the logic can reduce complexity and improve error handling.Consider refactoring the method as follows:
def on_message(self, message: Union[bytes, str]): try: # Decrypt message if necessary if self.crypto_key: if isinstance(message, bytes): message = decrypt_bin(self.crypto_key, message, cipher=self.cipher) elif "ciphertext" in message: message = decrypt_from_json(self.crypto_key, message, cipher=self.cipher, encoding=self.json_encoding) else: LOG.debug("Message was unencrypted") # Decode message if isinstance(message, bytes): message = decode_bitstring(message) elif isinstance(message, str): message = json.loads(message) # Check for decryption failure if isinstance(message, dict) and "ciphertext" in message: LOG.error("Received encrypted message but could not decrypt.") return # Ensure message is a HiveMessage instance if not isinstance(message, HiveMessage): message = HiveMessage(**message) # Handle message based on type if message.msg_type == HiveMessageType.BINARY: self._handle_binary(message) else: self._handle_hive_protocol(message) except Exception as e: LOG.exception(f"Error processing message: {e}")hivemind_bus_client/exceptions.py (1)
50-54: Enhance exception hierarchy and documentationThe introduction of
DecodingErroris a good practice to organize decoding-related exceptions. To improve clarity, consider providing more descriptive docstrings for these exceptions.Apply this diff to enhance the documentation:
class DecodingError(HiveMindException): - """Exception raised for errors in decoding""" + """Exception raised when decoding messages fails.""" class Z85DecodeError(DecodingError): - """Exception raised for errors in decoding Z85b.""" + """Exception raised when Z85 decoding fails."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hivemind_bus_client/exceptions.py(1 hunks)hivemind_bus_client/http.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
hivemind_bus_client/http.py
40-40: Do not perform function call BinaryDataCallbacks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
179-179: Do not use bare except
(E722)
185-185: Do not use bare except
(E722)
259-259: Do not perform function call FakeBus in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
hivemind_bus_client/http.py (2)
177-186:⚠️ Potential issueReplace bare except clauses with specific exceptions
Using bare except clauses can mask important errors by catching all exceptions, including KeyboardInterrupt and SystemExit.
Apply this diff to fix the issue:
try: self.bin_callbacks.handle_receive_tts(bin_data, utt, lang, file_name) - except: + except Exception as e: - LOG.exception("Error in binary callback: handle_receive_tts") + LOG.exception(f"Error in binary callback handle_receive_tts: {e}") try: self.bin_callbacks.handle_receive_file(bin_data, file_name) - except: + except Exception as e: - LOG.exception("Error in binary callback: handle_receive_file") + LOG.exception(f"Error in binary callback handle_receive_file: {e}")🧰 Tools
🪛 Ruff (0.8.2)
179-179: Do not use bare
except(E722)
185-185: Do not use bare
except(E722)
259-259:⚠️ Potential issueFix mutable default argument
Using
FakeBus()as a default argument can lead to unexpected behavior.Apply this diff to fix the issue:
- def connect(self, bus=FakeBus(), protocol=None, site_id=None): + def connect(self, bus=None, protocol=None, site_id=None): ... + if bus is None: + bus = FakeBus()🧰 Tools
🪛 Ruff (0.8.2)
259-259: Do not perform function call
FakeBusin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
🧹 Nitpick comments (4)
hivemind_bus_client/http.py (4)
92-119: Add validation in property settersThe property setters could benefit from input validation to ensure data integrity.
Consider adding validation:
@useragent.setter def useragent(self, val): + if not isinstance(val, str): + raise ValueError("useragent must be a string") + if not val.strip(): + raise ValueError("useragent cannot be empty") self.identity.name = val @key.setter def key(self, val): + if not isinstance(val, str): + raise ValueError("key must be a string") + if not val.strip(): + raise ValueError("key cannot be empty") self.identity.access_key = val
220-233: Add configurable polling intervalThe main loop uses a hardcoded 1-second polling interval. This should be configurable to allow for different use cases.
Consider adding a polling interval configuration:
def __init__(self, key: Optional[str] = None, ... + polling_interval: float = 1.0, bin_callbacks: Optional[BinaryDataCallbacks] = None): ... + self.polling_interval = polling_interval ... def run(self): ... - self.stopped.wait(1) + self.stopped.wait(self.polling_interval)
251-255: Improve binary message type checkThe condition for checking binary messages should check the message instance type first.
Consider this improvement:
def emit(self, message: Union[MycroftMessage, HiveMessage], binary_type: HiveMindBinaryPayloadType = HiveMindBinaryPayloadType.UNDEFINED): - if message.msg_type == HiveMessageType.BINARY: + if isinstance(message, HiveMessage) and message.msg_type == HiveMessageType.BINARY: raise NotImplementedError("binary sending not yet implemented") self.send_message(message)
311-321: Improve example usage with proper shutdown and error handlingThe example code could better demonstrate best practices.
Consider this improved example:
if __name__ == "__main__": LOG.set_level("DEBUG") - # not passing key etc so it uses identity file - client = HiveMindHTTPClient(host="http://localhost", port=5678) + try: + client = HiveMindHTTPClient(host="http://localhost", port=5678) + + # Wait for connection + client.handshake_event.wait(timeout=5) + if not client.handshake_event.is_set(): + raise RuntimeError("Failed to connect to HiveMind server") - time.sleep(5) - client.emit(HiveMessage(HiveMessageType.BUS, - MycroftMessage("recognizer_loop:utterance", - {"utterances": ["who is Isaac Newton"]}))) - time.sleep(10) + # Send a message + message = HiveMessage(HiveMessageType.BUS, + MycroftMessage("recognizer_loop:utterance", + {"utterances": ["who is Isaac Newton"]})) + client.emit(message) + + # Wait for response and cleanup + time.sleep(5) + except Exception as e: + LOG.error(f"Error in example: {e}") + finally: + client.shutdown()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hivemind_bus_client/http.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
hivemind_bus_client/http.py
40-40: Do not perform function call BinaryDataCallbacks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
179-179: Do not use bare except
(E722)
185-185: Do not use bare except
(E722)
259-259: Do not perform function call FakeBus in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (2)
hivemind_bus_client/http.py (2)
1-26: Well-structured imports and class definition!Good use of type hints and clear class inheritance structure.
28-40:⚠️ Potential issueFix mutable default argument
Using a mutable default argument (
BinaryDataCallbacks()) can lead to unexpected behavior as the instance is shared across all function calls.Apply this diff to fix the issue:
- def __init__(self, key: Optional[str] = None, - ... - bin_callbacks: BinaryDataCallbacks = BinaryDataCallbacks()): + def __init__(self, key: Optional[str] = None, + ... + bin_callbacks: Optional[BinaryDataCallbacks] = None): super().__init__(daemon=True) - self.bin_callbacks = bin_callbacks + self.bin_callbacks = bin_callbacks or BinaryDataCallbacks()Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
40-40: Do not perform function call
BinaryDataCallbacksin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
hivemind_bus_client/http_client.py (3)
230-234: Add exception handling in the main loop to prevent thread terminationIf an exception occurs during message retrieval or processing, such as a network error, it will cause the thread to terminate. Wrapping the main loop in a try-except block ensures that the client remains running and can handle intermittent issues gracefully.
Apply this diff to enhance the robustness of the main loop:
while not self.stopped.is_set(): - for hm in self.get_messages() + self.get_binary_messages(): - self.on_message(hm) + try: + messages = self.get_messages() + binary_messages = self.get_binary_messages() + for hm in messages + binary_messages: + self.on_message(hm) + except Exception as e: + LOG.error(f"Error retrieving or processing messages: {e}") self.stopped.wait(1)
343-350: Handle potential exceptions when retrieving messagesIn the
get_messagesmethod, aRuntimeErroris raised if the response contains an error. If this exception is not handled, it could cause the thread to terminate unexpectedly. Ensure that exceptions are properly handled or logged to maintain the stability of the client.Consider catching exceptions where
get_messagesis called or within the method itself.def get_messages(self) -> List[str]: """Retrieve messages from the HiveMind server.""" try: url = f"{self.base_url}/get_messages" response = requests.get(url, params={"authorization": self.auth}).json() if response.get("error"): raise RuntimeError(response["error"]) return [m for m in response["messages"]] except Exception as e: LOG.error(f"Error in get_messages: {e}") return []And ensure to handle exceptions in the calling code as previously suggested.
351-358: Handle potential exceptions when retrieving binary messagesSimilar to
get_messages, theget_binary_messagesmethod may raise exceptions that could terminate the thread. Proper exception handling ensures resilience against network errors or unexpected responses.Modify the method to handle exceptions:
def get_binary_messages(self) -> List[bytes]: """Retrieve binary messages from the HiveMind server.""" try: url = f"{self.base_url}/get_binary_messages" response = requests.get(url, params={"authorization": self.auth}).json() if response.get("error"): raise RuntimeError(response["error"]) return [pybase64.b64decode(m) for m in response["b64_messages"]] except Exception as e: LOG.error(f"Error in get_binary_messages: {e}") return []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hivemind_bus_client/http_client.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
hivemind_bus_client/http_client.py
44-44: Do not perform function call BinaryDataCallbacks in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
183-183: Do not use bare except
(E722)
189-189: Do not use bare except
(E722)
315-315: Do not perform function call FakeBus in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (1)
hivemind_bus_client/http_client.py (1)
89-90: Verify the method of passing authentication credentialsCurrently, the
authproperty returns base64-encoded credentials, which are passed as query parameters in the URL. Sending sensitive authentication credentials in the URL can expose them in server logs and is less secure than using HTTP headers. It's recommended to use HTTP Basic Authentication headers instead.Consider modifying the request to include the
Authorizationheader:def emit(self, message: Union[MycroftMessage, HiveMessage], binary_type: HiveMindBinaryPayloadType = HiveMindBinaryPayloadType.UNDEFINED): # ... url = f"{self.base_url}/send_message" - return requests.post(url, data={"message": payload}, params={"authorization": self.auth}) + headers = {'Authorization': f'Basic {self.auth}'} + return requests.post(url, data={"message": payload}, headers=headers)Please verify with the server documentation whether it expects credentials in headers or as query parameters.
companion to https://github.com/JarbasHiveMind/hivemind-http-protocol
Summary by CodeRabbit
New Features
DecodingErrorbase class for handling decoding-related exceptionsHiveMindHTTPClientto support HTTP server protocol communicationImprovements
Technical Updates
Z85DecodeErrorto inherit from newDecodingErrorclass