refactor!: uncouple agent and network protocol from hivemind protocol #24
Conversation
|
Warning Rate limit exceeded@JarbasAl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces a comprehensive refactoring of the HiveMind core architecture, focusing on protocol abstraction and modularization. The changes involve creating new protocol classes ( Changes
Assessment against linked 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: 0
🧹 Nitpick comments (10)
hivemind_core/agents.py (3)
20-31: Add error handling for bus connectivity
When usingbus.run_in_thread()andbus.connected_event.wait(), consider a timeout or exception handling mechanism to avoid hanging indefinitely if the bus fails to connect.
33-34: Guard the catch-all bus message handler
Using the"message"event as a catch-all can cause unexpected overhead if many events flow through here. If performance becomes a concern, consider filtering or using more specific event names.
38-60: Clarify silent ignore for ESCALATE
Currently,ESCALATEmessages are silently ignored. It might be beneficial to add at least a debug log or comment to explain that ignoring escalation here is intentional, preventing confusion for future maintainers.hivemind_core/service.py (2)
38-60: Improve documentation of dataclass fields
The docstring explains the attributes well. As the service grows, consider adding short type hints or docstrings directly on each field for clearer reference in IDEs and auto-generated docs.
117-140: Scope for error handling in service run
In therunmethod, operations like_presence.start()ornetwork_protocol.run()could fail unexpectedly. Consider wrapping them with try/except blocks to properly triggererror_hookand gracefully shut down the service in an emergency.hivemind_core/server.py (2)
58-58: Remove extraneous “f” prefix
Since the string has no placeholders, remove thefto adhere to style and prevent confusion.- LOG.info(f"generating self-signed SSL certificate") + LOG.info("generating self-signed SSL certificate")🧰 Tools
🪛 Ruff (0.8.2)
58-58: f-string without any placeholders
Remove extraneous
fprefix(F541)
114-115: Use a context manager when writing cert files
Applying awith open(...) as f:pattern ensures proper resource cleanup and is generally safer.- open(cert_path, "wb").write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert)) - open(key_path, "wb").write(crypto.dump_privatekey(crypto.FILETYPE_PEM, k)) + with open(cert_path, "wb") as cert_file_out: + cert_file_out.write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert)) + with open(key_path, "wb") as key_file_out: + key_file_out.write(crypto.dump_privatekey(crypto.FILETYPE_PEM, k))🧰 Tools
🪛 Ruff (0.8.2)
114-114: Use a context manager for opening files
(SIM115)
115-115: Use a context manager for opening files
(SIM115)
hivemind_core/protocol.py (2)
58-60: Documentsend_msganddisconnectusage
These callables significantly affect the connection lifecycle. To aid maintainers, highlight their expected behaviors (sync/async, blocking, re-entrancy) in a brief docstring or comment.
226-228: Relocate assignment foragent_protocol.hm_protocol
If there is a possibility of overridingagent_protocolin sub-classes or different contexts, ensure that line 227 is performed consistently. Otherwise, it’s fine as is.hivemind_core/__main__.py (1)
7-8: Add configuration from CLI or environment
You may consider makingagent_protocol=OVOSProtocolandnetwork_protocol=HiveMindWebsocketProtocolconfigurable via CLI or environment variables, so advanced users can swap protocols without modifying core code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
hivemind_core/__main__.py(1 hunks)hivemind_core/agents.py(1 hunks)hivemind_core/protocol.py(15 hunks)hivemind_core/server.py(1 hunks)hivemind_core/service.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
hivemind_core/server.py
58-58: f-string without any placeholders
Remove extraneous f prefix
(F541)
114-114: Use a context manager for opening files
(SIM115)
115-115: Use a context manager for opening files
(SIM115)
hivemind_core/service.py
15-15: hivemind_core.server.HiveMindWebsocketProtocol imported but unused
Remove unused import: hivemind_core.server.HiveMindWebsocketProtocol
(F401)
🔇 Additional comments (7)
hivemind_core/agents.py (2)
17-19: Ensure config defaults handle edge cases
The default factory for config loads only the "websocket" part of the OVOS configuration. If that section is missing or malformed, you'll get an empty dictionary. Consider handling fallback or logging a warning if no valid settings are found.
78-102: Consider synchronization for self.clients
Accessing self.clients inside the loop in handle_internal_mycroft presumably happens in a safe single-threaded context. If concurrency is possible, ensure it’s thread-safe or confirm that only one event loop thread manipulates self.clients.
hivemind_core/service.py (2)
15-15: False positive from static analysis
Although Ruff suggests that HiveMindWebsocketProtocol is unused, it’s actually instantiated at line 128. You can safely ignore or suppress this warning.
🧰 Tools
🪛 Ruff (0.8.2)
15-15: hivemind_core.server.HiveMindWebsocketProtocol imported but unused
Remove unused import: hivemind_core.server.HiveMindWebsocketProtocol
(F401)
81-93: Validate _status initialization
Inside __post_init__, _status is created if missing, but no exception handling is in place if callbacks fail. Ensure these hooks can gracefully handle exceptions or potential misconfiguration to avoid partial initialization states.
hivemind_core/protocol.py (2)
88-90: Handle bus errors if handshake fails
In __post_init__, if hm_protocol fails to provide a valid private key for the handshake, the code might raise uncaught exceptions. Consider verifying the existence/validity of hm_protocol.identity.private_key before instantiating the handshake.
517-519: Leverage “agent_bus_callback” more widely
You already update the _status with the bus in HiveMindService. If the system grows, consider hooking more advanced bus monitoring logic here to keep agent-level operations well-contained.
hivemind_core/__main__.py (1)
2-3: Ensure modular usage
Passing OVOSProtocol and HiveMindWebsocketProtocol to HiveMindService is a sound approach for decoupling. As you expand with additional protocols, verify that you thoroughly handle fallback or default scenarios in the main() function.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
hivemind_core/server.py (6)
29-37: Consider concurrency safeguards forhm_protocol.
Whilehm_protocolis declared as a shared attribute, if different coroutines or threads modify it concurrently, potential race conditions may arise. Ensure that you apply proper synchronization if runtime mutation is expected.
58-58: Remove extraneousfprefix.
There's no placeholder in this string, so using anf-string is unnecessary and may trigger warnings.- LOG.info(f"generating self-signed SSL certificate") + LOG.info("generating self-signed SSL certificate")🧰 Tools
🪛 Ruff (0.8.2)
58-58: f-string without any placeholders
Remove extraneous
fprefix(F541)
114-115: Use context managers for file operations.
Using context managers ensures files are closed reliably, even if an error occurs.- open(cert_path, "wb").write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert)) - open(key_path, "wb").write(crypto.dump_privatekey(crypto.FILETYPE_PEM, k)) + with open(cert_path, "wb") as f: + f.write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert)) + with open(key_path, "wb") as f: + f.write(crypto.dump_privatekey(crypto.FILETYPE_PEM, k))🧰 Tools
🪛 Ruff (0.8.2)
114-114: Use a context manager for opening files
(SIM115)
115-115: Use a context manager for opening files
(SIM115)
145-161: Avoid logging sensitive message contents.
Currently, user messages (including audio or text) are logged verbosely, possibly leaking private or PII data. Consider redacting or summarizing sensitive fields in logs.
162-177: Avoid sending credentials via query parameters.
Line 166 uses a/?authorization=approach, which can be insecure because URLs may be logged. Passing credentials in headers or a secure handshake would be safer.
231-232: Validate the WebSocket origin.
Currently,check_origin()unconditionally returnsTrue, exposing the server to potential cross-site attacks. Consider restricting or verifying trusted origins.hivemind_core/protocol.py (1)
471-471: Remove or finalize commented-out code.
Stray commented code at this line may indicate an unfinished feature or debugging artifact. Consider clarifying or removing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hivemind_core/protocol.py(16 hunks)hivemind_core/server.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
hivemind_core/server.py
58-58: f-string without any placeholders
Remove extraneous f prefix
(F541)
114-114: Use a context manager for opening files
(SIM115)
115-115: Use a context manager for opening files
(SIM115)
🔇 Additional comments (4)
hivemind_core/server.py (1)
129-143: Guard against invalid base64 strings.
A malformed or malicious “auth” value can break pybase64.b64decode. An exception handler or validation would prevent runtime errors.
hivemind_core/protocol.py (3)
58-60: Approve new callback attributes with concurrency in mind.
Storing send_msg and disconnect callables in the data class is clean. Just ensure they are safe to invoke across multiple coroutines or threads.
86-88: Good use of __post_init__ to initialize the handshake.
This is a concise pattern for customizing data class behavior after initialization.
171-204: Subprotocol abstraction looks good.
These new classes (_SubProtocol, AgentProtocol, NetworkProtocol) provide a clear extension pattern for specialized behaviors.
…#1) * refactor!: uncouple agent and network protocol from hivemind protocol companion to JarbasHiveMind/HiveMind-core#24 * refactor!: uncouple agent and network protocol from hivemind protocol companion to JarbasHiveMind/HiveMind-core#24 * .
closes #21
closes #22
paves the way for #23
example implementation of custom agent protocol: JarbasHiveMind/hivemind-persona#1
Summary by CodeRabbit
OVOSProtocolfor enhanced messaging framework communication.HiveMindWebsocketProtocolfor WebSocket communication and client management.HiveMindServicestructure using dataclasses for better attribute management.