-
-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: track last_seen , add missing cli permission management scripts … #44
Conversation
WalkthroughThis pull request introduces enhancements to the HiveMind core system, focusing on configuration, protocol, and command-line interface modifications. The changes include reformatting the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (8)
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (9)
hivemind_core/scripts.py (8)
207-221
: Consider abstracting repeated logic for can_escalate.
This command largely duplicates the pattern used in other “allow” commands. Factor out common logic if possible.
223-237
: Consider refactoring repeated blacklist logic for can_escalate.
Again, this is almost identical to the “allow-escalate” counterpart. You could unify “allow”/“blacklist” toggling in a single helper function.
239-253
: Same duplication pattern for can_propagate.
Centralizing allow/blacklist operations in a utility method might reduce code duplication and potential bugs.
255-269
: Repeated code for blacklist-propagate.
The logic is practically identical to its “allow” counterpart. Please consider consolidating them.
Line range hint
273-289
: Add input validation for skills and consider code reuse.
Although this effectively blocks skill usage, you might want stricter validation or a shared helper for blacklisting.
Line range hint
294-309
: Symmetry with “blacklist-skill,” but no immediate issues.
Same repetition concerns apply. If you anticipate more management commands, unify them in a shared method.
Line range hint
314-329
: Check for case sensitivity or input verification on intent IDs.
You might want to normalize or validate intent IDs (e.g. lowercasing) before blacklisting.
Line range hint
334-349
: “allow-intent” logic is sound, but repetitive.
Further extraction to a helper would make this more maintainable.hivemind_core/protocol.py (1)
86-87
: New attributes for administration and last-seen tracking.
Addingis_admin
andlast_seen
is straightforward. Consider persistingis_admin
in the database if you need it to be durable across restarts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hivemind_core/config.py
(1 hunks)hivemind_core/protocol.py
(5 hunks)hivemind_core/scripts.py
(3 hunks)
🔇 Additional comments (5)
hivemind_core/scripts.py (1)
170-171
: No immediate concerns.
This section is just a heading comment, no issues found.hivemind_core/protocol.py (4)
3-3
: Time import is valid.
Used for last_seen functionality. No issues here.
290-295
: Ensureupdate_last_seen
handles non-existent users gracefully.
If a record for the given API key is missing, you might raise an exception or handle it differently.
410-411
: Periodic last_seen updates look correct.
This ensures the timestamp reflects every message, which is helpful for active/inactive client tracking.
581-581
: Broadcast now restricted to admins.
Replacingcan_broadcast
withis_admin
enforces more centralized control. Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
hivemind_core/scripts.py (2)
170-172
: Consolidate permission management code under one section.
The file organizes message permissions in a dedicated section. Consider grouping it in a single sub-module or helper class to reduce duplication and improve maintainability as the command set grows.
223-237
: Blacklist-escalate command matches allow-escalate structure.
This mirrored approach keeps code consistent. However, consider deduplicating the shared logic (e.g., retrieving the user and toggling flags).hivemind_core/protocol.py (1)
581-581
: Admin-only broadcast enforcement.
Replacing the deprecatedcan_broadcast
check withis_admin
is a valid approach if the new requirement is to restrict broadcasting strictly to admins. However, if partial broadcast privileges are needed for other user classes, you might reintroduce a separate permission field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hivemind_core/config.py
(1 hunks)hivemind_core/protocol.py
(5 hunks)hivemind_core/scripts.py
(3 hunks)
🔇 Additional comments (9)
hivemind_core/config.py (1)
12-16
: Expansion of allowed encodings appears consistent.
Increasing the set of encodings is beneficial for flexibility. Test these new encodings throughout the system to ensure stability and compatibility with all permitted ciphers.hivemind_core/scripts.py (4)
52-58
: Server listen command looks good.
Thelisten
command provides a clear and direct way to launch the HiveMind service. No issues here.
207-221
: Allow-escalate command flows nicely.
The logic for enabling escalation permissions is straightforward and consistent with the existing pattern of reading the client from the database, modifying attributes, then persisting changes.
239-253
: Allow-propagate logic aligns with existing patterns.
Everything looks consistent and straightforward.
255-269
: Blacklist-propagate command is consistent.
Again, very similar structure to the other permission toggles, which is good for consistency.hivemind_core/protocol.py (4)
3-3
: Time import is used appropriately.
This import helps facilitate the last_seen updates.
86-87
: New attributes for admin rights and last_seen.
Introducingis_admin
andlast_seen
clarifies usage for broadcast privileges and tracking. Ensure thatis_admin
is thoroughly tested and reflected in the database if persistence is needed.
290-295
: Updating last_seen in the client database.
This is beneficial for auditing and analytics. Confirm that frequent writes to the database do not degrade performance at scale.
410-411
: Unobtrusive timestamp update.
Insertingupdate_last_seen
at the end ofhandle_message
ensures that every message refreshes the timestamp. This placement should reliably capture user activity.
…, fix broadcast permission
5494393
to
1517019
Compare
…, fix broadcast permission
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes
can_broadcast
attribute from client connections.