Repository navigation
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a FastAPI-based Admin UI (static SPA + API), integrates optional admin server startup and diagnostic admin UI into CLI and HiveMindService, and adds a jsdom/Jest frontend test harness with comprehensive tests and admin static configuration/assets. Changes
Sequence Diagram(s)sequenceDiagram
actor Operator
participant CLI as HiveMind CLI
participant Core as HiveMindService
participant AdminLib as hivemind_core.admin
participant AdminSrv as Admin Server (uvicorn/FastAPI)
participant Browser as Admin UI (SPA)
Operator->>CLI: start --with-admin --admin-host/port
CLI->>Core: instantiate & start HiveMindService
Core->>Core: run()
alt service starts successfully
Core->>AdminLib: init_injected_objects(service, db, protocol)
Core->>AdminLib: start_admin_server(host, port)
AdminLib->>AdminSrv: launch uvicorn (daemon)
Note over AdminSrv: Admin server runs alongside service
else startup fails
Core-->>CLI: exception raised
CLI->>AdminLib: init_injected_objects(..., startup_error)
CLI->>AdminLib: start_admin_server(host, port)
AdminLib->>AdminSrv: launch uvicorn with diagnostic info
Note over AdminSrv: UI exposes startup error for debugging
end
Browser->>AdminSrv: GET / (static) and /api/*
AdminSrv-->>Browser: serve SPA and API responses
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)
📝 Coding Plan
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 |
… frontend tests Admin UI (hivemind_core/admin/): - FastAPI-based admin server with authentication - Web UI with personas, clients, ACL, plugins, binary protocols pages - Security: sessionStorage for credentials, escapeHtml() on all user-controlled strings, hmac.compare_digest() for credential comparison (timing-safe) - Auth required on all endpoints including /startup-error - Removed startup_error details from unauthenticated /health response - Removed "Default credentials: admin/admin" from logs - Fixed login flow to verify against authenticated /api/config - Removed _is_valid_plugin_package() / VALID_PLUGIN_PATTERNS and deprecated /plugins/installed - save_persona_config writes to ~/.config/ovos_persona/ (not package dir) - list_installed_ovos_plugins resolves actual distribution package name via importlib.metadata - Dead code removed: loadConfig, loadConfigDefaults, validateConfig, saveConfig, showInstallPluginModal, installPlugin, savePersonaConfig, loadPersonaDefaults, simulateProgress, duplicate closeEnableBinaryPluginModal, duplicate testPersona, duplicate closeTestPersonaModal, duplicate DOMContentLoaded listener - Fixed innerHTML += anti-pattern; wired getPluginInfoHtml() into protocol/plugin renderers - 200ms debounce on client search input CLI/service integration: - --with-admin / --admin-host / --admin-port flags in __main__.py - service.py: _start_admin() starts admin server after service is ready - scripts.py: start_admin_with_error() for diagnostics on startup failure Frontend tests (144 tests, 77 new): - test/frontend/auth.test.js: login, sessionStorage, logout, 401 handling - test/frontend/personas.test.js: CRUD, toggleSolver, XSS escaping - test/frontend/clients.test.js: table render, filter, pagination, add/delete - test/frontend/acl.test.js: loadClientACL, saveClientACL, permission toggles - test/frontend/helpers/setup.js: Jest environment and test helpers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
test/frontend/helpers/setup.js (1)
159-159: Biome suppressions are optional for local linting but not required for CI.Biome is configured with
recommended: true(which includesnoGlobalEvalin the security rules), so it would report violations locally. However, the CI workflow does not run Biome linting—only Python build and test steps—so these violations do not block CI. If local Biome checks are desired, the proposed suppressions are reasonable; otherwise, the current eslint comments are sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/frontend/helpers/setup.js` at line 159, The use of window.eval(src) triggers Biome's noGlobalEval rule locally; either add a Biome suppression for that specific rule on the offending statement (targeting the noGlobalEval rule for the window.eval(src) call) so local Biome linting is silent, or leave the existing ESLint suppression (// eslint-disable-line no-eval) as-is since CI does not run Biome—update the file by applying one of these two options around the window.eval(src) expression.hivemind_core/admin/__init__.py (2)
92-94: Server thread reference is discarded, preventing graceful shutdown.The
server_threadreturned bycreate_daemonis not stored, making it impossible to stop the admin server gracefully. While daemon threads terminate when the main process exits, storing the reference would enable explicit cleanup.♻️ Proposed enhancement for explicit shutdown support
+_server_thread = None + def start_admin_server( host: str = "127.0.0.1", port: int = 8000, reload: bool = False, ) -> None: ... + global _server_thread # Run server in daemon thread - server_thread = create_daemon(_run_server) + _server_thread = create_daemon(_run_server) LOG.info("HiveMind Admin server thread started") + + +def stop_admin_server() -> None: + """Signal to stop the admin server (for future use).""" + global _server_thread + # Note: uvicorn doesn't support graceful shutdown from outside + # This is a placeholder for future implementation + _server_thread = None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__init__.py` around lines 92 - 94, The server thread created by create_daemon(_run_server) is currently discarded (server_thread) which prevents explicit shutdown; modify the module to keep the returned Thread object (store it in a module-level variable, e.g., server_thread), expose a stop/shutdown function (e.g., stop_admin_server or shutdown_server) that uses that reference to signal termination and join the thread, and register or document calling that function during application shutdown so _run_server can be stopped cleanly rather than relying solely on daemon termination.
41-56: Use explicitOptionaltype annotation.Line 45 uses
startup_error: Exception = Nonewhich PEP 484 prohibits as implicitOptional. Use explicit typing for clarity.♻️ Proposed fix
def init_injected_objects( service: "HiveMindService" = None, db: "ClientDatabase" = None, protocol: "HiveMindListenerProtocol" = None, - startup_error: Exception = None + startup_error: Optional[Exception] = None ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__init__.py` around lines 41 - 56, The function init_injected_objects declares startup_error: Exception = None which should use explicit Optional typing; update the signature in hivemind_core.admin.__init__.py to annotate startup_error as Optional[Exception] and add the necessary import (from typing import Optional) at top of the module so the parameter becomes startup_error: Optional[Exception] = None while leaving function name init_injected_objects, arguments service/db/protocol, and the call to hivemind_core.admin.api.init_injected_objects unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hivemind_core/__main__.py`:
- Around line 49-54: The help string for the argparse option '--admin-port' is
inconsistent with its actual default; update the help message in the
parser.add_argument call for '--admin-port' in __main__.py to reflect the real
default (8100) so the help text reads "Admin UI port (default: 8100). Requires
--with-admin." (or alternatively change the default argument to 8000 if the
intended default was 8000—ensure both the default parameter and the help text
match).
- Around line 136-150: The LOG.warning call using an f-string with no
placeholders should be simplified to a plain string (change LOG.warning(f"Admin
UI started for error diagnostics") to LOG.warning("Admin UI started for error
diagnostics")), and the ambiguous time.sleep(0.5) before start_admin_with_error
should either be removed or replaced with an explicit, documented wait (e.g.,
add a comment explaining why the delay is needed or replace with a deterministic
sync, such as waiting for a background thread/port readiness) — update the block
around args.with_admin, time.sleep, start_admin_with_error(..., error=e) and the
subsequent LOG.warning/error_msg usage accordingly so intent is clear and
logging remains correct.
- Around line 69-84: Replace the bare except block that wraps ClientDatabase()
with a specific exception capture (except Exception as err:) so system-exiting
exceptions aren’t swallowed; log the caught exception when falling back (use
LOG.warning("...: %s", err) or similar) before mutating cfg and calling
cfg.store(), then recreate ClientDatabase(); also remove unnecessary f-string
markers by converting LOG.warning(f"Switched to JSON database backend") to
LOG.warning("Switched to JSON database backend") and change LOG.error(f"Could
not create database: {db_err}") to LOG.error("Could not create database: %s",
db_err) so logging uses proper formatting. Ensure references to ClientDatabase,
cfg.store(), LOG.warning, and LOG.error are updated accordingly.
In `@hivemind_core/admin/__main__.py`:
- Around line 60-77: The root() and index() endpoints currently return
FileResponse(static_dir / "index.html") without verifying the file exists, which
can raise FileNotFoundError if assets are absent; update both root and index to
check (static_dir / "index.html").exists() before returning and, if missing,
raise fastapi.HTTPException(status_code=404, detail="index.html not found") (or
return a simple PlainTextResponse/JSON error) so the routes fail gracefully
instead of crashing.
In `@hivemind_core/admin/plugins_config.json`:
- Around line 82-91: The "Gemini Code (long lived)" plugin entry_point is
incorrect: update the entry_point value currently set to
"ovos-chat-gemini-code-plugin" to the distinct session variant
"ovos-chat-gemini-code-session-plugin" so the long-lived/session mode uses a
separate plugin entry (match the pattern used by "Claude Code (long lived)"
which uses the session entry point).
In `@hivemind_core/admin/static/css/style.css`:
- Around line 685-695: The keyframe name and font-family quoting violate
stylelint: rename the keyframe from slideIn to kebab-case slide-in and update
any references (e.g., the animation property on elements such as .toast) to use
animation: slide-in 0.3s ease; and remove unnecessary single quotes around
Consolas in any font-family declarations (change 'Consolas' to Consolas or to a
properly quoted/fallback list) so the `@keyframes` slide-in and font-family usages
comply with stylelint-config-standard-scss.
In `@hivemind_core/admin/static/index.html`:
- Around line 936-988: There are duplicate modal elements with identical IDs
(enableBinaryPluginModal, binaryPluginModule, binarySTTSelect, binaryTTSSelect,
binaryWWName, binaryWWSelect, binaryVADSelect, binaryConfigValidation) which
breaks getElementById usage; fix by removing the duplicate modal block or
renaming the second modal’s IDs and related JS handlers (e.g.,
submitEnableBinaryPlugin/closeEnableBinaryPluginModal) so each modal has unique
IDs and distinct handler names, and update any JS that queries those IDs to use
the new names.
In `@hivemind_core/scripts.py`:
- Around line 138-142: The code currently accepts/produces 24/32-char keys but
the DB layer (ClientDatabase.add_client) truncates to 16 chars and the script
prints the untruncated value, risking broken credentials; change the script's
key handling so it only produces and accepts the 16-character key that the DB
expects (adjust the validation for variable key to allow only length 16 and when
auto-generating key use the method that yields a 16-char hex string), and ensure
the exact value passed to ClientDatabase.add_client is the same value that gets
printed/logged (truncate before printing/storing if necessary) so stored and
displayed credentials match.
In `@hivemind_core/service.py`:
- Around line 111-119: The inline comment in _ggwave_add_client is incorrect
about key size: os.urandom(16).hex() yields 16 bytes (32 hex chars) which is
AES-128, and ClientDatabase.add_client truncates crypto_key to 16 characters;
update the code to either generate a 32-byte key (os.urandom(32).hex()) and
ensure ClientDatabase.add_client accepts the full key, or more simply correct
the comment to state "16 bytes (32 hex chars) = AES-128" and confirm crypto_key
handling in add_client remains intentional; reference _ggwave_add_client,
variable key, and ClientDatabase.add_client/crypto_key when making the change.
---
Nitpick comments:
In `@hivemind_core/admin/__init__.py`:
- Around line 92-94: The server thread created by create_daemon(_run_server) is
currently discarded (server_thread) which prevents explicit shutdown; modify the
module to keep the returned Thread object (store it in a module-level variable,
e.g., server_thread), expose a stop/shutdown function (e.g., stop_admin_server
or shutdown_server) that uses that reference to signal termination and join the
thread, and register or document calling that function during application
shutdown so _run_server can be stopped cleanly rather than relying solely on
daemon termination.
- Around line 41-56: The function init_injected_objects declares startup_error:
Exception = None which should use explicit Optional typing; update the signature
in hivemind_core.admin.__init__.py to annotate startup_error as
Optional[Exception] and add the necessary import (from typing import Optional)
at top of the module so the parameter becomes startup_error: Optional[Exception]
= None while leaving function name init_injected_objects, arguments
service/db/protocol, and the call to
hivemind_core.admin.api.init_injected_objects unchanged.
In `@test/frontend/helpers/setup.js`:
- Line 159: The use of window.eval(src) triggers Biome's noGlobalEval rule
locally; either add a Biome suppression for that specific rule on the offending
statement (targeting the noGlobalEval rule for the window.eval(src) call) so
local Biome linting is silent, or leave the existing ESLint suppression (//
eslint-disable-line no-eval) as-is since CI does not run Biome—update the file
by applying one of these two options around the window.eval(src) expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b00beea-0225-4fb9-9a36-c298f4377afa
📒 Files selected for processing (18)
hivemind_core/__main__.pyhivemind_core/admin/__init__.pyhivemind_core/admin/__main__.pyhivemind_core/admin/acl_config.jsonhivemind_core/admin/api.pyhivemind_core/admin/persona.jsonhivemind_core/admin/plugins_config.jsonhivemind_core/admin/static/css/style.csshivemind_core/admin/static/index.htmlhivemind_core/admin/static/js/app.jshivemind_core/admin/version.pyhivemind_core/scripts.pyhivemind_core/service.pytest/frontend/acl.test.jstest/frontend/auth.test.jstest/frontend/clients.test.jstest/frontend/helpers/setup.jstest/frontend/personas.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
hivemind_core/admin/static/css/style.css (1)
685-706:⚠️ Potential issue | 🟡 MinorFix the current stylelint failures in the toast animation and font stack.
slideInviolates the configured kebab-case keyframe rule, and"Consolas"is unnecessarily quoted, so this file will not pass stylelint as written.Lint-compliant fix
- animation: slideIn 0.3s ease; + animation: slide-in 0.3s ease; @@ - `@keyframes` slideIn { + `@keyframes` slide-in { @@ - font-family: 'Fira Code', 'JetBrains Mono', 'Consolas', monospace; + font-family: 'Fira Code', 'JetBrains Mono', Consolas, monospace;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/static/css/style.css` around lines 685 - 706, Rename the keyframes identifier and usages to kebab-case: change `@keyframes` slideIn to `@keyframes` slide-in and update any animation declarations that reference slideIn (e.g., the .toast rule with animation: slideIn 0.3s ease;) to use slide-in; also remove the unnecessary quotes around "Consolas" in the font stack inside .config-editor so it reads Consolas without quotes while keeping the rest of the family list intact.hivemind_core/admin/__main__.py (1)
60-77:⚠️ Potential issue | 🟡 MinorHandle missing
index.htmlthe same way you already handle missing/static.The
/staticmount is guarded bystatic_dir.exists(), but Lines 67 and 77 still unconditionally serveindex.html. If the frontend assets are absent from the install,/and/index.htmlfail with a server error instead of a clean 404/diagnostic response.Graceful fallback
-from fastapi import FastAPI +from fastapi import FastAPI, HTTPException @@ `@app.get`("/") async def root() -> FileResponse: @@ - return FileResponse(static_dir / "index.html") + index_file = static_dir / "index.html" + if not index_file.exists(): + raise HTTPException(status_code=404, detail="Admin UI assets are not installed") + return FileResponse(index_file) @@ `@app.get`("/index.html") async def index() -> FileResponse: @@ - return FileResponse(static_dir / "index.html") + index_file = static_dir / "index.html" + if not index_file.exists(): + raise HTTPException(status_code=404, detail="Admin UI assets are not installed") + return FileResponse(index_file)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__main__.py` around lines 60 - 77, Update the two route handlers root() and index() to mirror the static mount guard: check static_dir.exists() and that (static_dir / "index.html").is_file() before returning FileResponse; if the file is missing, return a 404 Response (or raise HTTPException(status_code=404)) with a short diagnostic message so requests to "/" and "/index.html" fail cleanly when frontend assets are not installed.hivemind_core/admin/plugins_config.json (1)
88-90:⚠️ Potential issue | 🟠 MajorUse a distinct entry point for the long-lived Gemini option.
Line 90 reuses the same
entry_pointas the standard "Gemini Code" entry, so both choices load the same plugin and the "long lived" variant cannot activate different behavior. Verify the package metadata and point this entry at the session-specific plugin instead of duplicating the standard one.Read-only verification
#!/bin/bash python - <<'PY' import sys from importlib.metadata import distribution, PackageNotFoundError try: dist = distribution("ovos-gemini-plugin") except PackageNotFoundError: sys.exit("ovos-gemini-plugin is not installed in this environment") for ep in sorted(dist.entry_points, key=lambda e: (e.group, e.name)): haystack = f"{ep.group} {ep.name} {ep.value}".lower() if "gemini" in haystack or "session" in haystack or "code" in haystack: print(f"{ep.group}: {ep.name} = {ep.value}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/plugins_config.json` around lines 88 - 90, The long-lived "Gemini Code (long lived)" entry currently reuses the standard entry_point "ovos-chat-gemini-code-plugin" so it loads the same plugin; update the "entry_point" for the "Gemini Code (long lived)" entry in plugins_config.json to point to the session-specific entry point from the ovos-gemini-plugin package (e.g., the session-specific value found in the package metadata such as the entry point that includes "session" or "long_lived" in its name), verify the correct entry point string in the package's distribution metadata (entry_points) and replace "ovos-chat-gemini-code-plugin" with that session-specific entry point name so the long-lived variant loads the distinct plugin.hivemind_core/__main__.py (1)
49-53:⚠️ Potential issue | 🟡 MinorFix the documented default for
--admin-port.Line 52 defaults the flag to
8100, but Line 53 still advertises8000.Small fix
- help="Admin UI port (default: 8000). Requires --with-admin." + help="Admin UI port (default: 8100). Requires --with-admin."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/__main__.py` around lines 49 - 53, The help string for the '--admin-port' argument is inconsistent with its actual default; update the parser.add_argument call for '--admin-port' so the help text matches the real default (change "default: 8000" to "default: 8100") by editing the help parameter in the parser.add_argument for '--admin-port' to reflect 8100.hivemind_core/admin/static/index.html (1)
936-988:⚠️ Potential issue | 🔴 CriticalDuplicate modal block still present with conflicting IDs.
The second binary modal (Lines 1283-1341) duplicates IDs already defined in Lines 936-988, so DOM lookups and handlers will bind unpredictably.
🧹 Suggested fix
- <!-- Enable Binary Protocol Modal --> - <div id="enableBinaryPluginModal" class="modal-overlay" onclick="if(event.target===this)closeEnableBinaryPluginModal()"> - ... - </div>Also applies to: 1283-1341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/static/index.html` around lines 936 - 988, There is a duplicate modal block defining the same IDs (enableBinaryPluginModal, binaryPluginModule, binarySTTSelect, binaryTTSSelect, binaryWWName, binaryWWSelect, binaryVADSelect, binaryConfigValidation) which causes conflicting DOM lookups and event binding for handlers submitEnableBinaryPlugin and closeEnableBinaryPluginModal; remove the duplicate modal (or merge its unique content into the first one) so only a single modal contains those IDs, and ensure any references to the handler functions or element IDs in scripts target that single remaining modal to avoid duplicated IDs and unpredictable behavior.
🧹 Nitpick comments (1)
test/frontend/personas.test.js (1)
72-88: Add a quote-in-name case for the inline button handlers.The suite validates escaped display text, but Lines 86-87 only exercise safe names inside
editPersona('...')/deletePersona('...'). Add a persona likeO'Reillyso broken handler markup or JS-string injection fails this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/frontend/personas.test.js` around lines 72 - 88, Update the "renders edit and delete buttons for each persona" test to include a persona whose name contains a single quote (e.g. O'Reilly) by calling global.renderPersonasList([makePersona({ name: "O'Reilly" })]) so the inline handler markup is exercised with a quote-in-name; then assert that the resulting HTML for `#personasList` contains the expected editPersona(...) and deletePersona(...) handler strings for that exact name (i.e. the expectations that currently check editPersona('MyBot') / deletePersona('MyBot') should be duplicated or replaced to check the handler occurrences for "O'Reilly" so any broken quoting/JS-string injection will fail the test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hivemind_core/__main__.py`:
- Around line 65-82: The code currently mutates cfg and calls cfg.store() when
ClientDatabase() fails, which persists a JSON fallback; instead, catch the
exception, build a temporary in-memory fallback database config (e.g.,
temp_db_cfg = {...} matching the existing JSON backend structure), do NOT assign
it back to cfg or call cfg.store(), and instantiate the fallback ClientDatabase
using that temporary config (e.g., ClientDatabase(config=temp_db_cfg) or another
constructor/factory that accepts a config); also include the original exception
in the warning log via LOG.warning(...) or LOG.exception(...) and avoid
modifying get_server_config()/cfg on disk.
In `@hivemind_core/admin/__init__.py`:
- Around line 59-94: The uvicorn reload flag must not be used from a
daemon/non-main thread; in start_admin_server (specifically around the
_run_server function and the uvicorn.run(...) call) guard the reload option so
it is only passed as True when running in the main thread (e.g., check
threading.current_thread() is threading.main_thread()); otherwise force
reload=False and log a warning explaining reload was disabled when launching via
create_daemon. Update the logic that builds the uvicorn.run parameters in
_run_server to enforce this check and avoid passing reload=True from a daemon
thread.
In `@hivemind_core/admin/static/index.html`:
- Around line 915-916: The two anchor elements using target="_blank" (the <a>
tags linking to "https://pypi.org/search/?q=ovos" and
"https://github.com/OpenVoiceOS") lack rel protection; update both anchor tags
to include rel="noopener noreferrer" alongside target="_blank" to prevent
tabnabbing and ensure safe external link behavior.
- Around line 806-812: The password and crypto key inputs are plain text; change
the inputs with IDs editClientPassword and editClientCryptoKey to use secure
input attributes (type="password") so values are masked, and add appropriate
autocomplete attributes (e.g., autocomplete="current-password" or "new-password"
for password and autocomplete="off" or "new-password" for the crypto key) and
any sensible maxlength/placeholder guidance to reduce accidental disclosure and
shoulder-surfing.
In `@test/frontend/helpers/setup.js`:
- Around line 174-187: resetDom() currently only resets database-related test
bridges (_dbProfiles, _dbBackends, _dbActiveName) so other in-app test state
like selectedSolvers, client list state and currentACLClientId persist across
tests; either expand resetDom() to clear all exposed bridges (e.g. explicitly
reset selectedSolvers, client list variables and currentACLClientId via the same
global/_originals pattern) or add a single bridge function in app.js (name it
_testResetState) that resets all lexical state and call global._testResetState()
from resetDom(); reference resetDom, evalApp,
_dbProfiles/_dbBackends/_dbActiveName, selectedSolvers, currentACLClientId and
_testResetState to locate where to add the resets.
- Line 122: The select with id "personaMemoryModule" is empty so
showCreatePersonaModal() writes a value that never persists in jsdom and
savePersona() never exercises the real memory-module path; add an option element
with value "ovos-agents-short-term-memory-plugin" (and mark it selected) into
the personaMemoryModule select in the test DOM setup so the code paths in
showCreatePersonaModal() and savePersona() read and use the real memory-module
value during tests.
---
Duplicate comments:
In `@hivemind_core/__main__.py`:
- Around line 49-53: The help string for the '--admin-port' argument is
inconsistent with its actual default; update the parser.add_argument call for
'--admin-port' so the help text matches the real default (change "default: 8000"
to "default: 8100") by editing the help parameter in the parser.add_argument for
'--admin-port' to reflect 8100.
In `@hivemind_core/admin/__main__.py`:
- Around line 60-77: Update the two route handlers root() and index() to mirror
the static mount guard: check static_dir.exists() and that (static_dir /
"index.html").is_file() before returning FileResponse; if the file is missing,
return a 404 Response (or raise HTTPException(status_code=404)) with a short
diagnostic message so requests to "/" and "/index.html" fail cleanly when
frontend assets are not installed.
In `@hivemind_core/admin/plugins_config.json`:
- Around line 88-90: The long-lived "Gemini Code (long lived)" entry currently
reuses the standard entry_point "ovos-chat-gemini-code-plugin" so it loads the
same plugin; update the "entry_point" for the "Gemini Code (long lived)" entry
in plugins_config.json to point to the session-specific entry point from the
ovos-gemini-plugin package (e.g., the session-specific value found in the
package metadata such as the entry point that includes "session" or "long_lived"
in its name), verify the correct entry point string in the package's
distribution metadata (entry_points) and replace "ovos-chat-gemini-code-plugin"
with that session-specific entry point name so the long-lived variant loads the
distinct plugin.
In `@hivemind_core/admin/static/css/style.css`:
- Around line 685-706: Rename the keyframes identifier and usages to kebab-case:
change `@keyframes` slideIn to `@keyframes` slide-in and update any animation
declarations that reference slideIn (e.g., the .toast rule with animation:
slideIn 0.3s ease;) to use slide-in; also remove the unnecessary quotes around
"Consolas" in the font stack inside .config-editor so it reads Consolas without
quotes while keeping the rest of the family list intact.
In `@hivemind_core/admin/static/index.html`:
- Around line 936-988: There is a duplicate modal block defining the same IDs
(enableBinaryPluginModal, binaryPluginModule, binarySTTSelect, binaryTTSSelect,
binaryWWName, binaryWWSelect, binaryVADSelect, binaryConfigValidation) which
causes conflicting DOM lookups and event binding for handlers
submitEnableBinaryPlugin and closeEnableBinaryPluginModal; remove the duplicate
modal (or merge its unique content into the first one) so only a single modal
contains those IDs, and ensure any references to the handler functions or
element IDs in scripts target that single remaining modal to avoid duplicated
IDs and unpredictable behavior.
---
Nitpick comments:
In `@test/frontend/personas.test.js`:
- Around line 72-88: Update the "renders edit and delete buttons for each
persona" test to include a persona whose name contains a single quote (e.g.
O'Reilly) by calling global.renderPersonasList([makePersona({ name: "O'Reilly"
})]) so the inline handler markup is exercised with a quote-in-name; then assert
that the resulting HTML for `#personasList` contains the expected editPersona(...)
and deletePersona(...) handler strings for that exact name (i.e. the
expectations that currently check editPersona('MyBot') / deletePersona('MyBot')
should be duplicated or replaced to check the handler occurrences for "O'Reilly"
so any broken quoting/JS-string injection will fail the test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 09bb1dbf-8905-4c1f-af7c-c322b5d7a864
📒 Files selected for processing (16)
hivemind_core/__main__.pyhivemind_core/admin/__init__.pyhivemind_core/admin/__main__.pyhivemind_core/admin/acl_config.jsonhivemind_core/admin/api.pyhivemind_core/admin/persona.jsonhivemind_core/admin/plugins_config.jsonhivemind_core/admin/static/css/style.csshivemind_core/admin/static/index.htmlhivemind_core/admin/static/js/app.jshivemind_core/admin/version.pytest/frontend/acl.test.jstest/frontend/auth.test.jstest/frontend/clients.test.jstest/frontend/helpers/setup.jstest/frontend/personas.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- test/frontend/auth.test.js
- hivemind_core/admin/persona.json
- hivemind_core/admin/acl_config.json
- test/frontend/clients.test.js
Critical: - index.html: remove duplicate enableBinaryPluginModal block (lines 936-988); keep the complete grid-layout version; both JS handlers used the same IDs so getElementById always returned the first (simpler) modal, breaking the configured binary protocol flow Major: - __main__.py: replace bare except with except Exception to avoid swallowing KeyboardInterrupt/SystemExit during DB fallback - __main__.py: remove f-strings without placeholders (LOG.warning/error) - plugins_config.json: fix Gemini Code (long lived) entry_point from ovos-chat-gemini-code-plugin to ovos-chat-gemini-code-session-plugin to match the Claude plugin convention and enable distinct session mode Minor: - __main__.py: fix --admin-port help text to say 8100 (matches actual default) - __main__.py: document purpose of time.sleep(0.5) before error admin start - admin/__main__.py: add index.html existence check in root/index routes; return 404 with actionable message instead of unhandled FileNotFoundError - style.css: rename slideIn keyframe to slide-in (kebab-case per stylelint) - style.css: remove unnecessary quotes around Consolas font family name Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
hivemind_core/admin/static/index.html (2)
915-916:⚠️ Potential issue | 🟡 MinorAdd
relprotection to external links opened in a new tab.These anchors use
target="_blank"withoutrel="noopener noreferrer".🛡️ Proposed fix
- 💡 Find plugins on <a href="https://pypi.org/search/?q=ovos" target="_blank" style="color: var(--accent-primary);">PyPI</a> or - <a href="https://github.com/OpenVoiceOS" target="_blank" style="color: var(--accent-primary);">GitHub</a> + 💡 Find plugins on <a href="https://pypi.org/search/?q=ovos" target="_blank" rel="noopener noreferrer" style="color: var(--accent-primary);">PyPI</a> or + <a href="https://github.com/OpenVoiceOS" target="_blank" rel="noopener noreferrer" style="color: var(--accent-primary);">GitHub</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/static/index.html` around lines 915 - 916, The two external anchor tags that open in a new tab (the link to https://pypi.org/search/?q=ovos and the link to https://github.com/OpenVoiceOS) use target="_blank" without rel protection; update those <a> elements (the PyPI and GitHub anchors) to include rel="noopener noreferrer" alongside target="_blank" to prevent tab-nabbing and preserve security when opening external links.
806-812:⚠️ Potential issue | 🟠 MajorMask sensitive client fields in the edit modal.
Password and crypto key are still rendered as plain text inputs, which unnecessarily exposes secrets during admin operations.
🔒 Proposed fix
<div class="form-group"> <label>Password</label> - <input type="text" id="editClientPassword"> + <input type="password" id="editClientPassword" autocomplete="new-password"> </div> <div class="form-group"> <label>Crypto Key (16, 24, or 32 chars)</label> - <input type="text" id="editClientCryptoKey"> + <input type="password" id="editClientCryptoKey" autocomplete="off"> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/static/index.html` around lines 806 - 812, The password and crypto key fields in the edit modal are rendered as plain text causing secrets to be exposed; update the inputs with ids editClientPassword and editClientCryptoKey to use type="password" (instead of "text") and add appropriate attributes like autocomplete="new-password" or autocomplete="off" to prevent browser autofill; optionally keep a separate show/hide toggle control if needed but the immediate fix is switching the input types and disabling autofill for these sensitive fields.hivemind_core/__main__.py (1)
69-82:⚠️ Potential issue | 🟠 MajorDo not persist JSON fallback config in diagnostics flow.
This path currently writes fallback DB config to disk (
cfg.store()), so a transient DB/import failure can permanently switch backend selection.🛠️ Proposed fix (in-memory fallback only)
- from hivemind_core.config import get_server_config - - cfg = get_server_config() try: db = ClientDatabase() except Exception: - # Fallback to JSON backend - cfg['database'] = { + # Fallback to JSON backend for diagnostics only (do not persist) + fallback_db_cfg = { 'module': 'hivemind-json-db-plugin', 'hivemind-json-db-plugin': { 'name': 'clients', 'subfolder': 'hivemind-core' } } - cfg.store() - LOG.warning("Switched to JSON database backend") - db = ClientDatabase() + LOG.warning("Using JSON database backend for diagnostics only") + db = ClientDatabase(config=fallback_db_cfg)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/__main__.py` around lines 69 - 82, The code persists a transient JSON fallback by calling cfg.store() which can permanently switch backends; remove the disk write and make the fallback in-memory only: when ClientDatabase() construction fails, create a temporary config copy (do not call cfg.store()), assign the JSON-backend values to that temp (instead of mutating/persisting cfg), instantiate ClientDatabase using the temp config or set cfg in-memory without invoking cfg.store(), and keep LOG.warning("Switched to JSON database backend") for visibility; ensure no lasting mutation of the original cfg so future runs won't be affected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hivemind_core/__main__.py`:
- Around line 126-130: service.run() already blocks until shutdown, but the code
calls wait_for_exit_signal() again afterwards causing a duplicate blocking path;
remove the redundant wait_for_exit_signal() call after service.run(). Locate the
block where service.run() is invoked and delete or guard the subsequent from
ovos_utils import wait_for_exit_signal / wait_for_exit_signal() invocation so
only one shutdown wait mechanism (service.run or wait_for_exit_signal) is
active.
In `@hivemind_core/service.py`:
- Around line 148-149: Wrap the call to start_admin_server in a try/except block
so admin startup failures do not abort overall service initialization: catch
Exception around the start_admin_server(host=host, port=port) invocation (in the
same function that currently calls start_admin_server), log the exception with
the module/service logger (include the exception info/stacktrace) and do not
re-raise—ensure the service continues startup even if start_admin_server fails.
- Around line 136-141: The admin injection currently passes the protocol class
via self.hm_protocol instead of the live runtime instance; after the protocol is
instantiated in run() (the local hm_protocol), re-call init_injected_objects to
update the admin injection with the live instance: inside run(), once
hm_protocol is created and if getattr(self, '_admin_enabled', False) is true,
call init_injected_objects(service=self, db=self.db, protocol=hm_protocol,
startup_error=None) so admin APIs receive the live protocol object rather than
the class.
---
Duplicate comments:
In `@hivemind_core/__main__.py`:
- Around line 69-82: The code persists a transient JSON fallback by calling
cfg.store() which can permanently switch backends; remove the disk write and
make the fallback in-memory only: when ClientDatabase() construction fails,
create a temporary config copy (do not call cfg.store()), assign the
JSON-backend values to that temp (instead of mutating/persisting cfg),
instantiate ClientDatabase using the temp config or set cfg in-memory without
invoking cfg.store(), and keep LOG.warning("Switched to JSON database backend")
for visibility; ensure no lasting mutation of the original cfg so future runs
won't be affected.
In `@hivemind_core/admin/static/index.html`:
- Around line 915-916: The two external anchor tags that open in a new tab (the
link to https://pypi.org/search/?q=ovos and the link to
https://github.com/OpenVoiceOS) use target="_blank" without rel protection;
update those <a> elements (the PyPI and GitHub anchors) to include rel="noopener
noreferrer" alongside target="_blank" to prevent tab-nabbing and preserve
security when opening external links.
- Around line 806-812: The password and crypto key fields in the edit modal are
rendered as plain text causing secrets to be exposed; update the inputs with ids
editClientPassword and editClientCryptoKey to use type="password" (instead of
"text") and add appropriate attributes like autocomplete="new-password" or
autocomplete="off" to prevent browser autofill; optionally keep a separate
show/hide toggle control if needed but the immediate fix is switching the input
types and disabling autofill for these sensitive fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4088b2f7-85e9-4141-9060-898a5e24e5e5
📒 Files selected for processing (6)
hivemind_core/__main__.pyhivemind_core/admin/__main__.pyhivemind_core/admin/plugins_config.jsonhivemind_core/admin/static/css/style.csshivemind_core/admin/static/index.htmlhivemind_core/service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- hivemind_core/admin/static/css/style.css
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hivemind_core/admin/__init__.py (1)
84-90:⚠️ Potential issue | 🟠 Major
reload=Truewill fail when running in a daemon thread.This was flagged in a previous review. The
reloadparameter is passed through touvicorn.run(), but reload mode requires OS signal handling which Python restricts to the main thread. Whenstart_admin_server()is called withreload=True, the daemon thread will crash withValueError: signal only works in main thread.🛡️ Suggested fix to guard reload in daemon context
def _run_server(): + effective_reload = reload + if reload and threading.current_thread() is not threading.main_thread(): + LOG.warning("reload=True is not supported in daemon-thread mode; disabling reload") + effective_reload = False LOG.info(f"Starting HiveMind Admin UI at http://{host}:{port}") LOG.info("Change admin credentials in ~/.config/hivemind-core/server.json (admin_user, admin_pass)") uvicorn.run( app, host=host, port=port, - reload=reload, + reload=effective_reload, log_level="info", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__init__.py` around lines 84 - 90, The uvicorn.run call in start_admin_server passes through the reload parameter which crashes in non-main threads; change start_admin_server to detect threading.current_thread() vs threading.main_thread() (or use threading.main_thread()) and, if not running in the main thread, force reload=False (and optionally log or warn) before calling uvicorn.run(app, host=host, port=port, reload=..., log_level="info") so reload mode is only used when running in the main thread.
🧹 Nitpick comments (2)
hivemind_core/admin/__main__.py (1)
60-93: Consider extracting shared logic to reduce duplication.The
root()andindex()endpoints have identical implementations. One could simply delegate to the other.♻️ Suggested refactor
`@app.get`("/") async def root() -> FileResponse: """Serve the main index.html file for the SPA. Returns: FileResponse: The index.html file from static directory. Raises: HTTPException: 404 if index.html is not present (e.g. package installed without static assets). """ from fastapi import HTTPException index_path = static_dir / "index.html" if not index_path.exists(): raise HTTPException(status_code=404, detail="Admin UI static assets not found. " "Reinstall with: pip install hivemind-core[admin]") return FileResponse(index_path) `@app.get`("/index.html") async def index() -> FileResponse: - """Serve the main index.html file for the SPA. - - Returns: - FileResponse: The index.html file from static directory. - - Raises: - HTTPException: 404 if index.html is not present (e.g. package installed without static assets). - """ - from fastapi import HTTPException - index_path = static_dir / "index.html" - if not index_path.exists(): - raise HTTPException(status_code=404, detail="Admin UI static assets not found. " - "Reinstall with: pip install hivemind-core[admin]") - return FileResponse(index_path) + """Alias for root() to serve index.html directly.""" + return await root()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__main__.py` around lines 60 - 93, The root() and index() endpoints duplicate logic serving index.html; refactor by extracting the shared retrieval/check into a helper (e.g., a private function like _serve_index() that uses static_dir, checks index_path.exists(), raises HTTPException with the existing message, and returns FileResponse) and then have both root() and index() simply call and return _serve_index(); alternatively make index() delegate to return await root() to avoid duplicated code while keeping the same behavior.hivemind_core/admin/__init__.py (1)
41-46: Consider using explicitOptionalor| Nonetype hints.Parameters with
= Nonedefault should be explicitly typed as optional per PEP 484. This improves type checker compatibility.♻️ Suggested fix
def init_injected_objects( - service: "HiveMindService" = None, - db: "ClientDatabase" = None, - protocol: "HiveMindListenerProtocol" = None, - startup_error: Exception = None + service: "HiveMindService | None" = None, + db: "ClientDatabase | None" = None, + protocol: "HiveMindListenerProtocol | None" = None, + startup_error: Exception | None = None ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__init__.py` around lines 41 - 46, The function init_injected_objects has parameters defaulting to None but lacks explicit optional typing; update the annotations for service (HiveMindService), db (ClientDatabase), protocol (HiveMindListenerProtocol) and startup_error (Exception) to be explicit Optionals (e.g., Optional[HiveMindService] or HiveMindService | None) and import typing.Optional if using the Optional form so static type checkers recognize the None-able types; keep the parameter defaults as = None and ensure annotations match the runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hivemind_core/admin/__init__.py`:
- Around line 97-104: The current get_admin_app() in
hivemind_core.admin.__init__.py imports and returns
hivemind_core.admin.api.get_admin_app which only exposes the API routes (no
/static or SPA mounts), so either update the docstring to state it returns the
API-only app, or change the import to return the full FastAPI app defined in
hivemind_core.admin.__main__ (the app that mounts /static and registers the SPA
routes) instead of hivemind_core.admin.api.get_admin_app; pick one of these two
fixes and apply it to get_admin_app() and its docstring accordingly.
---
Duplicate comments:
In `@hivemind_core/admin/__init__.py`:
- Around line 84-90: The uvicorn.run call in start_admin_server passes through
the reload parameter which crashes in non-main threads; change
start_admin_server to detect threading.current_thread() vs
threading.main_thread() (or use threading.main_thread()) and, if not running in
the main thread, force reload=False (and optionally log or warn) before calling
uvicorn.run(app, host=host, port=port, reload=..., log_level="info") so reload
mode is only used when running in the main thread.
---
Nitpick comments:
In `@hivemind_core/admin/__init__.py`:
- Around line 41-46: The function init_injected_objects has parameters
defaulting to None but lacks explicit optional typing; update the annotations
for service (HiveMindService), db (ClientDatabase), protocol
(HiveMindListenerProtocol) and startup_error (Exception) to be explicit
Optionals (e.g., Optional[HiveMindService] or HiveMindService | None) and import
typing.Optional if using the Optional form so static type checkers recognize the
None-able types; keep the parameter defaults as = None and ensure annotations
match the runtime behavior.
In `@hivemind_core/admin/__main__.py`:
- Around line 60-93: The root() and index() endpoints duplicate logic serving
index.html; refactor by extracting the shared retrieval/check into a helper
(e.g., a private function like _serve_index() that uses static_dir, checks
index_path.exists(), raises HTTPException with the existing message, and returns
FileResponse) and then have both root() and index() simply call and return
_serve_index(); alternatively make index() delegate to return await root() to
avoid duplicated code while keeping the same behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e35fe57f-1b89-4ef5-8ff1-e5a5f0c3345a
📒 Files selected for processing (4)
hivemind_core/admin/__init__.pyhivemind_core/admin/__main__.pyhivemind_core/admin/api.pyhivemind_core/admin/version.py
Wires up --with-admin, --admin-host, and --admin-port CLI options to the listen command so HiveMindService._admin_enabled/host/port are set before run() is called. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hivemind_core/scripts.py`:
- Around line 99-100: The CLI option for --admin-port currently uses type=int
and allows invalid port numbers; update the Click option declaration in the
listen function so the admin-port uses Click's IntRange(1, 65535) (i.e., replace
type=int with type=click.IntRange(min=1, max=65535)) to validate the port at
parse time and avoid runtime binding errors when uvicorn tries to bind to an
out-of-range port.
- Around line 113-115: The CLI is directly setting dynamic internals on the
service (service._admin_enabled/_admin_host/_admin_port), which couples layers;
add a public configurator on HiveMindService (e.g., a method named
configure_admin(self, enabled: bool, host: str = "127.0.0.1", port: int = 8000)
-> None) that sets those three attributes internally, then update the CLI call
in scripts.py to call service.configure_admin(with_admin, admin_host,
admin_port) instead of assigning service._admin_* directly; ensure the new
method lives on the HiveMindService class and document its params.
- Add hivemind_core.admin to packages list so the subpackage is installed (was silently missing, causing the ImportError warning) - Add extras_require[admin] with fastapi, uvicorn, python-multipart - Register hivemind-admin console script entry point Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without package_data, the static/ directory (index.html, CSS, JS) was not included in the sdist/wheel, causing the 404 "Admin UI static assets not found" error for users who installed from PyPI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace setup.py + requirements.txt with pyproject.toml (setuptools) - Add __version__ to hivemind_core/version.py (dynamic version attr) - Declare hivemind_core.admin subpackage and [admin] extra in pyproject - Register hivemind-admin console script entry point - Replace TigreGotico/gh-automations@master workflow calls with OpenVoiceOS/gh-automations@dev reusable workflows - Add missing standard workflows: tests, lint, license-check, pip-audit, python-support, repo-health Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ruff fixes: - Remove unused imports: traceback, threading, Optional, uuid - Remove unused variable server_thread (admin/__init__.py) - Replace bare except: with except Exception: (protocol.py, service.py) - Remove f-string prefix from strings with no placeholders - Drop unused signature variable; move to comment (protocol.py) - Add # noqa: F401 to availability-check imports (zeroconf, ggwave) CI fixes: - tests.yml: replace gh-automations internal test.yml (not reusable) with a direct pytest matrix workflow - license-check.yml: exclude ^hivemind.core.* — package is intentionally AGPL-3.0; flagging it in its own dependency audit is a false positive Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Restore protocol.py, __main__.py, scripts.py (delete_client), and service.py (run loop) to their pre-PR state. Those changes should go in a separate cleanup PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Checking in! I've finished the automated rounds. 🏥I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've checked the repo's memory (aka git history). 🧠 ✅ All required files present. Latest Version: ✅ 🔍 LintEnsuring your contribution is moving forward. 🚀 ❌ ruff: issues found — see job log ⚖️ License CheckI've audited the copyright holders list. 👥 ❌ License violations detected (49 packages) — review required before merging. License distribution: 12× MIT License, 8× Apache Software License, 6× Apache-2.0, 6× MIT, 3× BSD-3-Clause, 2× BSD License, 2× ISC License (ISCL), 2× PSF-2.0, +8 more Full breakdown — 49 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔒 Security (pip-audit)Ensuring our project is resilient to attacks. 🏰 ✅ No known vulnerabilities found (68 packages scanned). 🔨 Build TestsThe build report is now ready for your review. 📝 ✅ All versions pass
Integrity verified by the OVOS Bot 💎 |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
hivemind_core/admin/__init__.py (2)
96-103:⚠️ Potential issue | 🟡 MinorDocstring is misleading - returns API-only app, not full app with static routes.
This was flagged in a previous review. The function imports from
hivemind_core.admin.apiwhich returns only the REST API routes, not the full app from__main__.pythat includes static file mounts (/static) and SPA routes (/,/index.html). Either update the docstring to clarify this returns the API subset, or change the import to return the full app.📝 Option 1: Update docstring to reflect actual behavior
def get_admin_app(): - """Get the FastAPI app instance for admin UI. + """Get the FastAPI REST API app instance (without static/SPA routes). Returns: - FastAPI app configured with all admin routes. + FastAPI app configured with admin REST API routes only. + For the full app with static files and SPA, import from __main__. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__init__.py` around lines 96 - 103, The docstring on get_admin_app currently claims it returns the full admin UI app but the function imports hivemind_core.admin.api.get_admin_app which only provides the REST API routes; update the docstring to state that this function returns the API-only FastAPI app (not including static mounts or SPA routes), or alternatively change the import to return the full app implementation from the module that assembles static mounts and SPA routes (e.g., the __main__ module that mounts /static and serves / and /index.html); reference the get_admin_app symbol and the hivemind_core.admin.api and __main__ modules when making the change.
58-94:⚠️ Potential issue | 🟠 Major
reload=Trueis unsafe when running in a daemon thread.This issue was flagged in a previous review. The
reloadparameter is passed touvicorn.run()but Uvicorn's reload mode requires OS signal handling, which Python restricts to the main thread. Whenstart_admin_serveris called viacreate_daemon, usingreload=Truewill raiseValueError: signal only works in main thread.🛡️ Proposed fix - disable reload in daemon mode
def _run_server(): + effective_reload = reload + if reload: + import threading + if threading.current_thread() is not threading.main_thread(): + LOG.warning("reload=True is not supported in daemon-thread mode; disabling reload") + effective_reload = False LOG.info(f"Starting HiveMind Admin UI at http://{host}:{port}") LOG.info("Change admin credentials in ~/.config/hivemind-core/server.json (admin_user, admin_pass)") uvicorn.run( app, host=host, port=port, - reload=reload, + reload=effective_reload, log_level="info", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__init__.py` around lines 58 - 94, The start_admin_server implementation passes the reload flag into uvicorn.run while running the server in a daemon thread via create_daemon, which will raise "signal only works in main thread"; modify start_admin_server so that when creating the daemon thread (in _run_server) you do not pass reload=True to uvicorn.run — instead force reload=False and, if the caller requested reload=True, log a warning via LOG.warn/LOG.warning explaining that reload is disabled in daemon mode; refer to start_admin_server, the inner _run_server function, and the call to uvicorn.run to locate where to change the behavior.hivemind_core/service.py (2)
135-141:⚠️ Potential issue | 🟠 Major
protocolreceives the class type, not the live runtime instance.This was flagged in a previous review.
self.hm_protocolis the protocol class (defaultHiveMindListenerProtocol), not the instantiated protocol object with live connection state. The admin API will receive a type reference instead of the runtime instance. Consider re-injecting afterhm_protocolis instantiated inrun().🛠️ Proposed fix
Pass
Noneinitially and re-inject after instantiation:init_injected_objects( service=self, db=self.db, - protocol=getattr(self, 'hm_protocol', None), + protocol=None, # Will be re-injected after instantiation startup_error=error )Then after line 180 in
run(), add:# Re-inject with live protocol instance for admin UI if getattr(self, '_admin_enabled', False): from hivemind_core.admin import init_injected_objects init_injected_objects( service=self, db=self.db, protocol=hm_protocol, startup_error=None )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/service.py` around lines 135 - 141, The injected protocol is currently the protocol class (self.hm_protocol) instead of the live runtime instance; change the initial call to init_injected_objects at startup to pass protocol=None (and keep startup_error as appropriate) and then, inside run() after the protocol is instantiated (the local hm_protocol variable), call init_injected_objects again to re-inject the live protocol instance and clear startup_error; reference the init_injected_objects function, the attribute self.hm_protocol, the run() method and the local hm_protocol variable, and guard the re-injection with the same admin check used elsewhere (e.g., self._admin_enabled) so the admin API receives the runtime protocol instance.
148-149:⚠️ Potential issue | 🟠 MajorAdmin server startup exceptions will crash core startup.
This was flagged in a previous review. Since admin is an optional component, failures from
start_admin_servershould be caught and logged rather than propagating up and aborting the main service initialization.🛡️ Proposed hardening
# Start admin server - start_admin_server(host=host, port=port) + try: + start_admin_server(host=host, port=port) + except Exception as exc: + LOG.exception("Failed to start HiveMind Admin server on %s:%s", host, port)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/service.py` around lines 148 - 149, start_admin_server is currently called directly which lets any exception abort core startup; wrap the call to start_admin_server(host=host, port=port) in a try/except Exception as e block, catch all exceptions, log the failure (including the exception and traceback) using the module's logger, and do not re-raise so the main service initialization continues even if the optional admin server fails to start.
🧹 Nitpick comments (5)
.github/workflows/build_tests.yml (1)
3-4: Consider reducing duplicate runs frompush+pull_request.Line 3 and Line 4 will both run for many internal PR updates, which can double CI cost/time. If that’s not intended, scope
pushto protected branches/tags only.💡 Example adjustment
on: push: + branches: + - dev pull_request: workflow_dispatch:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_tests.yml around lines 3 - 4, The workflow currently triggers on both "push" and "pull_request", causing duplicate CI runs; update the triggers so "push" is scoped to protected branches or tags (e.g., only run on pushes to main/release branches or tags) or remove the "push" trigger if you only want PR validation. Locate the "push" and "pull_request" entries in the workflow and add a branches/branches-ignore or tags filter to "push" (or remove "push") so routine PR updates do not invoke both triggers.hivemind_core/service.py (1)
111-122: Use explicitOptionalforerrorparameter.PEP 484 prohibits implicit
Optional. The parameter should explicitly indicate it acceptsNone.♻️ Proposed fix
- def _start_admin(self, host: str = "127.0.0.1", port: int = 8000, error: Exception = None) -> None: + def _start_admin(self, host: str = "127.0.0.1", port: int = 8000, error: Optional[Exception] = None) -> None:Also add
Optionalto the imports at line 17:-from typing import Callable, Optional, Type +from typing import Callable, Optional, Type(Note:
Optionalis already imported, so just use it in the signature.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/service.py` around lines 111 - 122, The _start_admin method's signature uses a bare type allowing None for the error parameter; change its annotation to use Optional explicitly (i.e., error: Optional[Exception] = None) to comply with PEP 484, and ensure Optional is imported (refer to the existing import at the top where Optional is declared) so the new signature type resolves correctly..github/workflows/tests.yml (1)
13-21: Consider adding pip caching for faster CI runs.The workflow installs dependencies on every run. Adding dependency caching can significantly reduce CI execution time.
♻️ Optional: Add pip caching
steps: - uses: actions/checkout@v4 - uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} + cache: 'pip' - name: Install dependencies run: pip install -e ".[admin]" pytest pytest-cov - name: Run tests run: pytest test/unittests/ -v --tb=short🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 13 - 21, Add a pip cache step before the "Install dependencies" step to reuse downloaded wheels and speed up CI: use the actions/cache action keyed on matrix.python-version and a pip requirements lock fingerprint (e.g., hash of requirements.txt or pyproject.lock) and cache paths like ~/.cache/pip (or the project's virtualenv cache) so subsequent runs restore the cache and the "Install dependencies" step (which currently runs pip install -e ".[admin]" pytest pytest-cov) uses the cached packages instead of reinstalling everything.pyproject.toml (1)
56-61: Package data patterns may be redundant.Both
"static/**/*"and"static/*"are specified. The glob**/*recursively matches all files in subdirectories, while*matches only direct children. If the intent is to capture all files including those at the root ofstatic/, the patterns work but are slightly redundant since**/*in setuptools typically includes the base directory files too.#!/bin/bash # Verify the static directory structure to confirm pattern necessity fd . hivemind_core/admin/static --type f | head -20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 56 - 61, The package-data entry for the "hivemind_core.admin" key contains redundant glob patterns "static/**/*" and "static/*"; keep a single pattern (prefer "static/**/*" to include files in root and subdirs) and remove "static/*" to avoid duplication while keeping the "*.json" pattern unchanged; update the [tool.setuptools.package-data] block to only include "static/**/*" and "*.json" so package data remains complete without redundant globs.hivemind_core/admin/__init__.py (1)
40-56: Use explicitOptionalor union type forstartup_errorparameter.PEP 484 prohibits implicit
Optional. The parameterstartup_error: Exception = Noneshould explicitly indicate it acceptsNone.♻️ Proposed fix
+from typing import TYPE_CHECKING, Optional -from typing import TYPE_CHECKING def init_injected_objects( service: "HiveMindService" = None, db: "ClientDatabase" = None, protocol: "HiveMindListenerProtocol" = None, - startup_error: Exception = None + startup_error: Optional[Exception] = None ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hivemind_core/admin/__init__.py` around lines 40 - 56, The parameter annotation for init_injected_objects uses an implicit Optional (startup_error: Exception = None); update it to an explicit union type such as startup_error: Optional[Exception] = None or startup_error: Exception | None = None and add the necessary import (from typing import Optional) if using Optional, ensuring the function signature in hivemind_core.admin.__init__.py (function init_injected_objects) is updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build_tests.yml:
- Around line 9-10: Replace the mutable ref "@dev" in the reusable workflow
invocation "uses:
OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev" with an
immutable full commit SHA and keep "secrets: inherit" as-is; update every
workflow under .github/workflows/ that uses the same pattern. Obtain the SHA
with the provided gh api command and substitute it into the "uses:" value (e.g.,
change "@dev" to "@<commit-sha>") so the workflow points to a fixed commit.
In @.github/workflows/license-check.yml:
- Line 9: Replace the mutable branch reference in the workflow uses declaration
"uses: OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev" with
an immutable commit SHA from the dev branch (e.g., change the "@dev" suffix to
"@<commit-sha>" using the current commit hash from the
OpenVoiceOS/gh-automations dev branch) so the reusable workflow is pinned to a
specific commit.
- Line 13: The exclude_packages regex `^hivemind.core.*` is too broad and the
literal dot acts as a wildcard; update the pattern referenced by the
exclude_packages setting to an anchored literal that matches only the intended
variants (hivemind-core, hivemind.core, hivemind_core) — for example use a
pattern like `^hivemind[-._]core$` to replace the existing value so unrelated
packages such as `hivemindxcore-anything` are no longer excluded.
In @.github/workflows/pip-audit.yml:
- Around line 9-10: Replace the mutable reusable-workflow ref
"OpenVoiceOS/gh-automations/.github/workflows/pip-audit.yml@dev" with the pinned
commit SHA
"OpenVoiceOS/gh-automations/.github/workflows/pip-audit.yml@e37a97650feada694abb6420a84bbd23a71d6bb1"
(the same SHA should be used wherever you reference that repo) so that the line
using the reusable workflow and the accompanying "secrets: inherit" remain
stable and not subject to dev-branch changes; update every similar occurrence
across .github/workflows/ to use the commit SHA.
In @.github/workflows/repo-health.yml:
- Around line 9-10: Replace the mutable branch reference in the reusable
workflow invocation with an immutable commit SHA: change the "uses:
OpenVoiceOS/gh-automations/.github/workflows/repo-health.yml@dev" reference to
point to the full commit SHA of the intended upstream commit, keeping "secrets:
inherit" as-is; apply the same SHA-pinning pattern to all other reusable
workflow calls that use "secrets: inherit" to eliminate the supply-chain risk of
executing unreviewed code from a moving branch.
---
Duplicate comments:
In `@hivemind_core/admin/__init__.py`:
- Around line 96-103: The docstring on get_admin_app currently claims it returns
the full admin UI app but the function imports
hivemind_core.admin.api.get_admin_app which only provides the REST API routes;
update the docstring to state that this function returns the API-only FastAPI
app (not including static mounts or SPA routes), or alternatively change the
import to return the full app implementation from the module that assembles
static mounts and SPA routes (e.g., the __main__ module that mounts /static and
serves / and /index.html); reference the get_admin_app symbol and the
hivemind_core.admin.api and __main__ modules when making the change.
- Around line 58-94: The start_admin_server implementation passes the reload
flag into uvicorn.run while running the server in a daemon thread via
create_daemon, which will raise "signal only works in main thread"; modify
start_admin_server so that when creating the daemon thread (in _run_server) you
do not pass reload=True to uvicorn.run — instead force reload=False and, if the
caller requested reload=True, log a warning via LOG.warn/LOG.warning explaining
that reload is disabled in daemon mode; refer to start_admin_server, the inner
_run_server function, and the call to uvicorn.run to locate where to change the
behavior.
In `@hivemind_core/service.py`:
- Around line 135-141: The injected protocol is currently the protocol class
(self.hm_protocol) instead of the live runtime instance; change the initial call
to init_injected_objects at startup to pass protocol=None (and keep
startup_error as appropriate) and then, inside run() after the protocol is
instantiated (the local hm_protocol variable), call init_injected_objects again
to re-inject the live protocol instance and clear startup_error; reference the
init_injected_objects function, the attribute self.hm_protocol, the run() method
and the local hm_protocol variable, and guard the re-injection with the same
admin check used elsewhere (e.g., self._admin_enabled) so the admin API receives
the runtime protocol instance.
- Around line 148-149: start_admin_server is currently called directly which
lets any exception abort core startup; wrap the call to
start_admin_server(host=host, port=port) in a try/except Exception as e block,
catch all exceptions, log the failure (including the exception and traceback)
using the module's logger, and do not re-raise so the main service
initialization continues even if the optional admin server fails to start.
---
Nitpick comments:
In @.github/workflows/build_tests.yml:
- Around line 3-4: The workflow currently triggers on both "push" and
"pull_request", causing duplicate CI runs; update the triggers so "push" is
scoped to protected branches or tags (e.g., only run on pushes to main/release
branches or tags) or remove the "push" trigger if you only want PR validation.
Locate the "push" and "pull_request" entries in the workflow and add a
branches/branches-ignore or tags filter to "push" (or remove "push") so routine
PR updates do not invoke both triggers.
In @.github/workflows/tests.yml:
- Around line 13-21: Add a pip cache step before the "Install dependencies" step
to reuse downloaded wheels and speed up CI: use the actions/cache action keyed
on matrix.python-version and a pip requirements lock fingerprint (e.g., hash of
requirements.txt or pyproject.lock) and cache paths like ~/.cache/pip (or the
project's virtualenv cache) so subsequent runs restore the cache and the
"Install dependencies" step (which currently runs pip install -e ".[admin]"
pytest pytest-cov) uses the cached packages instead of reinstalling everything.
In `@hivemind_core/admin/__init__.py`:
- Around line 40-56: The parameter annotation for init_injected_objects uses an
implicit Optional (startup_error: Exception = None); update it to an explicit
union type such as startup_error: Optional[Exception] = None or startup_error:
Exception | None = None and add the necessary import (from typing import
Optional) if using Optional, ensuring the function signature in
hivemind_core.admin.__init__.py (function init_injected_objects) is updated
accordingly.
In `@hivemind_core/service.py`:
- Around line 111-122: The _start_admin method's signature uses a bare type
allowing None for the error parameter; change its annotation to use Optional
explicitly (i.e., error: Optional[Exception] = None) to comply with PEP 484, and
ensure Optional is imported (refer to the existing import at the top where
Optional is declared) so the new signature type resolves correctly.
In `@pyproject.toml`:
- Around line 56-61: The package-data entry for the "hivemind_core.admin" key
contains redundant glob patterns "static/**/*" and "static/*"; keep a single
pattern (prefer "static/**/*" to include files in root and subdirs) and remove
"static/*" to avoid duplication while keeping the "*.json" pattern unchanged;
update the [tool.setuptools.package-data] block to only include "static/**/*"
and "*.json" so package data remains complete without redundant globs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 374cf2de-df26-4aed-98d2-e740162738a6
📒 Files selected for processing (16)
.github/workflows/build_tests.yml.github/workflows/license-check.yml.github/workflows/lint.yml.github/workflows/pip-audit.yml.github/workflows/publish_stable.yml.github/workflows/release_workflow.yml.github/workflows/repo-health.yml.github/workflows/tests.ymlhivemind_core/admin/__init__.pyhivemind_core/admin/api.pyhivemind_core/scripts.pyhivemind_core/service.pyhivemind_core/version.pypyproject.tomlrequirements.txtsetup.py
💤 Files with no reviewable changes (2)
- requirements.txt
- setup.py
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/lint.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- hivemind_core/scripts.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Keep only build_tests.yml (validates pyproject.toml migration and package_data bundling). All other new workflows belong in a separate maintenance PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Quality
Packaging & CI