Repository navigation
feat: Add admin API endpoints for runtime config reload - #1945
Merged
moshemorad merged 20 commits intoJun 15, 2026
Merged
Conversation
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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:
WalkthroughAdds an unauthenticated admin FastAPI sub-app exposing three POST endpoints to reload toolsets, models, or both; implements thread-safe Config methods to reload toolsets and the model registry and clear cached executor/registry state; updates docs, server initialization, and tests for these endpoints and behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminAPI as Admin API
participant Config
participant ToolExec as Tool Executor
participant ModelReg as Model Registry
Client->>AdminAPI: POST /api/admin/reload (or /toolsets /models)
alt reload toolsets or combined
AdminAPI->>Config: reload_toolsets()
Config->>Config: Re-read config & update toolsets/MCP/runbooks
Config->>ToolExec: Clear cached executor / trigger rebuild (filtered)
ToolExec-->>Config: Executor rebuilt
end
alt reload models or combined
AdminAPI->>Config: reload_models()
Config->>ModelReg: Invalidate & rebuild model registry
ModelReg-->>Config: Return model count
end
AdminAPI->>AdminAPI: Aggregate counts and detail into ReloadResponse
AdminAPI-->>Client: HTTP 200 with counts (or HTTP 500 with detail on error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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: 5
🧹 Nitpick comments (7)
holmes/admin/admin_api.py (3)
36-41: Avoid the magic string"runbook"for the runbook toolset lookup.The toolset name is compared to a bare literal. If the toolset is ever renamed or namespaced, the runbook count silently drops to 0 with no error. Prefer a shared constant (or exposing the runbook count directly from the toolset/executor API).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/admin/admin_api.py` around lines 36 - 41, The code uses the magic string "runbook" when scanning toolsets which is brittle; update the lookup to use a shared constant or API property instead of the literal: replace the direct comparison t.name == "runbook" in the loop that computes runbook_count with a reference to a central constant (e.g., RUNBOOK_TOOLSET_NAME) or, better, query the toolset/executor for an explicit attribute or method that exposes runbook availability (e.g., use t.is_runbook_toolset or t.toolset_type == RUNBOOK_TOOLSET_NAME or call t.get_runbook_count()), and keep the rest of the logic (reading available_runbooks from t.tools[0]) but rely on the canonical identifier to find the runbook toolset.
24-52: Add type hints to helpers per repo guidelines.
_build_toolset_counts(executor),_reload_and_rebuild_toolsets(),init_admin_app(...), and the three endpoint functions are missing parameter/return annotations. As per coding guidelines, "Type hints are required throughout the codebase (mypy configuration in pyproject.toml)."♻️ Proposed fix
-def init_admin_app(main_app: FastAPI, config: Config, dal: SupabaseDal): +def init_admin_app(main_app: FastAPI, config: Config, dal: SupabaseDal) -> None: -def _build_toolset_counts(executor) -> Dict[str, int]: +def _build_toolset_counts(executor: "ToolExecutor") -> Dict[str, int]: -def _reload_and_rebuild_toolsets(): +def _reload_and_rebuild_toolsets() -> "ToolExecutor":(Import
ToolExecutorfromholmes.core.tools_utils.tool_executor.) Also add-> ReloadResponseto the three endpoint functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/admin/admin_api.py` around lines 24 - 52, Add missing type hints: import ToolExecutor from holmes.core.tools_utils.tool_executor and annotate _build_toolset_counts(executor: ToolExecutor) -> Dict[str, int]; annotate _reload_and_rebuild_toolsets() -> ToolExecutor; annotate init_admin_app(main_app: FastAPI, config: Config, dal: SupabaseDal) -> None (keep existing parameter types); and add the return type -> ReloadResponse to the three admin endpoint functions (import ReloadResponse from its module). Ensure all new imports are added at top of the file.
11-29: Module-level singletons make testing and re-initialization fragile.
_CONFIGand_DALare declared as bare type annotations with no default, so reading them beforeinit_admin_appruns raisesNameError, and any test that imports this module gets a process-wide global that can't be scoped per-test. The admin sub-app is also created at import time, so it can only ever be associated with oneConfig.A more robust pattern is to stash the dependencies on
main_app.state(or create the sub-app insideinit_admin_appand close over the args), which also makes the admin endpoints trivially testable without the heavyserver.pyimport chain.♻️ Sketch
def init_admin_app(main_app: FastAPI, config: Config, dal: SupabaseDal) -> None: admin_app = FastAPI() admin_app.state.config = config admin_app.state.dal = dal `@admin_app.post`("/reload/toolsets", response_model=ReloadResponse) def reload_toolsets(request: Request): cfg: Config = request.app.state.config ... ... main_app.mount("/api/admin", admin_app)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/admin/admin_api.py` around lines 11 - 29, The module creates a top-level admin_app and uses module-level singletons _CONFIG and _DAL which are uninitialized until init_admin_app is called, causing NameError and making testing fragile; change init_admin_app to create and configure a new FastAPI admin_app inside the function, store config and dal on admin_app.state (e.g., admin_app.state.config and admin_app.state.dal), move any admin route handlers (those returning ReloadResponse) to close over or read from request.app.state, and mount this local admin_app on main_app so the module has no bare globals and tests can instantiate isolated admin apps per test.tests/test_config_reload.py (2)
111-114: Importingserverinside the fixture runs heavy module-level side effects at test time.
server.pyexecutesinit_logging(),init_config(), Sentry setup, and (depending on env) DAL initialization the first time it's imported. Deferring the import inside the fixture only delays, rather than avoids, those side effects — and also violates the repo guideline that "ALWAYS place Python imports at the top of the file, not inside functions or methods."Consider either (a) restructuring so
init_admin_appcan be invoked against a lightweight ad-hocFastAPIapp in the test, or (b) moving the import to module scope with a note about the side effects. Option (a) is preferable because it keeps these admin tests isolated from real config/DAL.♻️ Sketch of option (a)
from fastapi import FastAPI from holmes.admin.admin_api import init_admin_app `@pytest.fixture` def client(config): app = FastAPI() init_admin_app(app, config, dal=MagicMock()) return TestClient(app)As per coding guidelines, "ALWAYS place Python imports at the top of the file, not inside functions or methods."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config_reload.py` around lines 111 - 114, The test fixture imports server at runtime which triggers heavy module-level side effects (init_logging, init_config, Sentry, DAL) and violates the "imports at top" guideline; change the client fixture to create a lightweight FastAPI app and call init_admin_app(app, config, dal=MagicMock()) instead of importing server, move necessary imports to module scope (FastAPI, TestClient, init_admin_app, MagicMock) and ensure the fixture uses that ad-hoc app so tests remain isolated from real config/DAL.
32-82: Coverage gap: reload doesn't verifymcp_servers,custom_runbook_catalogs, oradditional_toolsets.
reload_toolsets()copiestoolsets,mcp_servers,custom_toolsets, andcustom_runbook_catalogsfrom the fresh YAML, but tests only assert ontoolsets. If a regression drops one of the other copies, it won't be caught. Consider extendingtest_picks_up_new_toolsets_from_yaml(or adding a sibling) to also mutatemcp_servers/custom_runbook_catalogsin the YAML and verify they're updated on the reloadedConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config_reload.py` around lines 32 - 82, Extend the test_picks_up_new_toolsets_from_yaml test to also mutate and assert the other fields that reload_toolsets should copy: add entries for mcp_servers, custom_runbook_catalogs (and custom_toolsets/additional_toolsets if your Config uses that name) to new_config_data written to config_yaml_path, call config.reload_toolsets(), and assert that config.mcp_servers, config.custom_runbook_catalogs (and config.custom_toolsets/additional_toolsets) reflect the new values; keep using config.reload_toolsets() and the existing pattern of writing YAML and verifying the updated attributes.docs/reference/http-api.md (1)
651-657: Error response placement is ambiguous.The
Error Response (500)block sits under the/api/admin/reload/modelssection but applies to all three admin endpoints. Consider lifting it to a shared subsection (e.g., under the admonition at line 658) so readers don't assume it'smodels-only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/http-api.md` around lines 651 - 657, Move the "Error Response (500)" JSON block out from under the /api/admin/reload/models endpoint and place it into a shared "Error Response (500)" subsection under the existing admin admonition so it applies to all three admin reload endpoints; update the heading/context so the example clearly states it is a common 500 response for the admin reload endpoints rather than models-only, and remove the duplicate/error block from the /api/admin/reload/models section (refer to the /api/admin/reload/models endpoint and the admin admonition to locate the sections to change).server.py (1)
276-277: Move theinit_admin_appimport to the top of the file.The import at line 276 breaks the project's established pattern — the sibling
init_checks_appis imported at the top (line 57) and invoked here at line 274. As per coding guidelines, "ALWAYS place Python imports at the top of the file, not inside functions or methods."♻️ Proposed fix
Add this import next to the existing
init_checks_appimport (near line 57):from holmes.admin.admin_api import init_admin_appThen clean up the mount site:
init_checks_app(app, config) - -from holmes.admin.admin_api import init_admin_app -init_admin_app(app, config, dal) - +init_admin_app(app, config, dal)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 276 - 277, Move the local import of init_admin_app out of the function and add it with the other top-level imports next to the existing init_checks_app import; remove the inline "from holmes.admin.admin_api import init_admin_app" that's currently at the bottom and keep the call to init_admin_app(app, config, dal) in place where it is invoked; ensure there are no duplicate imports and that the top-of-file import is the single source for init_admin_app.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/admin/admin_api.py`:
- Around line 24-29: Wrap mounting of admin_app inside an opt-in env flag check
(e.g., HOLMES_ENABLE_ADMIN_API) and enforce a shared-secret header for all admin
endpoints: update init_admin_app to read the enable flag and admin token from
config or environment, only call main_app.mount("/api/admin", admin_app) when
the flag is true, and ensure admin_app endpoints validate an
X-Holmes-Admin-Token header (compare against the configured secret) before
performing destructive actions; reference init_admin_app, admin_app, _CONFIG and
_DAL to locate where to add the flag check and header validation middleware or
dependency.
- Around line 55-103: Update the three exception handlers to preserve the
exception chain by re-raising the HTTPException with "from e": in
reload_toolsets, reload_models, and reload_all replace the bare "raise
HTTPException(...)" with "raise HTTPException(... ) from e" so the original
traceback is retained while keeping the existing logging (logging.error(...,
exc_info=True)) and message/ status_code unchanged.
In `@holmes/config.py`:
- Around line 438-459: reload_toolsets currently fails to copy
additional_toolsets from the freshly parsed Config, so update reload_toolsets to
assign self.additional_toolsets = fresh.additional_toolsets alongside
toolsets/custom_toolsets/mcp_servers/custom_runbook_catalogs, ensuring
ToolsetManager (referenced by the toolset_manager property) sees changes; also,
when self._config_file_path is unset or the file is missing, emit a warning log
and return {"reloaded": False} instead of silently clearing caches and returning
success (note: adjust or keep test_works_without_config_file as needed if you
change the return behavior).
- Around line 461-472: reload_models() currently only rebuilds the
LLMModelRegistry from model_list.yaml and thus misses updates to the main config
(fields model, api_key, api_base, api_version, fast_model); update
reload_models() to re-parse/reload the main configuration before reconstructing
the LLMModelRegistry using the same approach as reload_toolsets() (i.e., call
the same config-read helper used by reload_toolsets() so the in-memory config
state is refreshed), then proceed to reset self._llm_model_registry and
instantiate LLMModelRegistry as before so changes to those fields are picked up.
In `@tests/test_config_reload.py`:
- Around line 86-99: The test currently sets MockRegistry.return_value =
mock_instance causing every LLMModelRegistry() call to return the same object so
the identity assertion fails; change the mock setup in
test_resets_model_registry to have MockRegistry.side_effect produce a new
MagicMock (or a factory that returns distinct mock instances with .models) for
each construction so the first access to config.llm_model_registry returns one
mock_instance and after config.reload_models() the subsequent construction
returns a different mock object, allowing assert new_registry is not
old_registry to pass.
---
Nitpick comments:
In `@docs/reference/http-api.md`:
- Around line 651-657: Move the "Error Response (500)" JSON block out from under
the /api/admin/reload/models endpoint and place it into a shared "Error Response
(500)" subsection under the existing admin admonition so it applies to all three
admin reload endpoints; update the heading/context so the example clearly states
it is a common 500 response for the admin reload endpoints rather than
models-only, and remove the duplicate/error block from the
/api/admin/reload/models section (refer to the /api/admin/reload/models endpoint
and the admin admonition to locate the sections to change).
In `@holmes/admin/admin_api.py`:
- Around line 36-41: The code uses the magic string "runbook" when scanning
toolsets which is brittle; update the lookup to use a shared constant or API
property instead of the literal: replace the direct comparison t.name ==
"runbook" in the loop that computes runbook_count with a reference to a central
constant (e.g., RUNBOOK_TOOLSET_NAME) or, better, query the toolset/executor for
an explicit attribute or method that exposes runbook availability (e.g., use
t.is_runbook_toolset or t.toolset_type == RUNBOOK_TOOLSET_NAME or call
t.get_runbook_count()), and keep the rest of the logic (reading
available_runbooks from t.tools[0]) but rely on the canonical identifier to find
the runbook toolset.
- Around line 24-52: Add missing type hints: import ToolExecutor from
holmes.core.tools_utils.tool_executor and annotate
_build_toolset_counts(executor: ToolExecutor) -> Dict[str, int]; annotate
_reload_and_rebuild_toolsets() -> ToolExecutor; annotate
init_admin_app(main_app: FastAPI, config: Config, dal: SupabaseDal) -> None
(keep existing parameter types); and add the return type -> ReloadResponse to
the three admin endpoint functions (import ReloadResponse from its module).
Ensure all new imports are added at top of the file.
- Around line 11-29: The module creates a top-level admin_app and uses
module-level singletons _CONFIG and _DAL which are uninitialized until
init_admin_app is called, causing NameError and making testing fragile; change
init_admin_app to create and configure a new FastAPI admin_app inside the
function, store config and dal on admin_app.state (e.g., admin_app.state.config
and admin_app.state.dal), move any admin route handlers (those returning
ReloadResponse) to close over or read from request.app.state, and mount this
local admin_app on main_app so the module has no bare globals and tests can
instantiate isolated admin apps per test.
In `@server.py`:
- Around line 276-277: Move the local import of init_admin_app out of the
function and add it with the other top-level imports next to the existing
init_checks_app import; remove the inline "from holmes.admin.admin_api import
init_admin_app" that's currently at the bottom and keep the call to
init_admin_app(app, config, dal) in place where it is invoked; ensure there are
no duplicate imports and that the top-of-file import is the single source for
init_admin_app.
In `@tests/test_config_reload.py`:
- Around line 111-114: The test fixture imports server at runtime which triggers
heavy module-level side effects (init_logging, init_config, Sentry, DAL) and
violates the "imports at top" guideline; change the client fixture to create a
lightweight FastAPI app and call init_admin_app(app, config, dal=MagicMock())
instead of importing server, move necessary imports to module scope (FastAPI,
TestClient, init_admin_app, MagicMock) and ensure the fixture uses that ad-hoc
app so tests remain isolated from real config/DAL.
- Around line 32-82: Extend the test_picks_up_new_toolsets_from_yaml test to
also mutate and assert the other fields that reload_toolsets should copy: add
entries for mcp_servers, custom_runbook_catalogs (and
custom_toolsets/additional_toolsets if your Config uses that name) to
new_config_data written to config_yaml_path, call config.reload_toolsets(), and
assert that config.mcp_servers, config.custom_runbook_catalogs (and
config.custom_toolsets/additional_toolsets) reflect the new values; keep using
config.reload_toolsets() and the existing pattern of writing YAML and verifying
the updated attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bad85a0e-e90b-4b6c-b240-f366dfa2f300
📥 Commits
Reviewing files that changed from the base of the PR and between c687c8a and bbe1a769f262494d802ac37463e54245b709deaf.
📒 Files selected for processing (6)
docs/reference/http-api.mdholmes/admin/__init__.pyholmes/admin/admin_api.pyholmes/config.pyserver.pytests/test_config_reload.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/test_config_reload.py (2)
113-118: Module-level_CONFIG/_DALinadmin_apipersist across tests.
init_admin_appwrites to globals inholmes.admin.admin_api, but nothing resets them after each test. Today this is fine because all admin tests re-init, but if a later test in the same pytest session importsadmin_api(e.g., viaserver.py) and then exercises the admin routes without callinginit_admin_app, it'll hit whatever_CONFIG/_DALthis test left behind. Consider a smallautousefixture that resets those toNonein teardown to keep tests hermetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config_reload.py` around lines 113 - 118, Test globals _CONFIG and _DAL in holmes.admin.admin_api are left set by init_admin_app and can leak across tests; add an autouse fixture that yields and then resets holmes.admin.admin_api._CONFIG and holmes.admin.admin_api._DAL to None in teardown. Locate tests that call init_admin_app (e.g., TestAdminEndpoints.client) and add a module- or session-scoped autouse pytest fixture that imports holmes.admin.admin_api and sets those two globals to None after each test to ensure hermetic state.
120-139: Double-mockingreload_toolsets+create_tool_executorbypasses what the endpoint is supposed to exercise.With both
Config.reload_toolsetsandConfig.create_tool_executormocked, this test effectively only verifies response shape wiring — it doesn't catch regressions in_reload_and_rebuild_toolsets(e.g., wrong tag filter, missingPrerequisiteCacheMode.DISABLED,reuse_executorchanges). Thetest_picks_up_new_toolsets_from_yamlunit test covers reload semantics, but an endpoint-level test that only mockscreate_tool_executor(letting the realreload_toolsetsrun against the temp YAML) would give meaningfully better coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config_reload.py` around lines 120 - 139, The test currently mocks both Config.reload_toolsets and Config.create_tool_executor which bypasses the real reload logic; change the test to only mock Config.create_tool_executor so the real Config.reload_toolsets runs against the temp YAML and exercises _reload_and_rebuild_toolsets; keep the existing mock for create_tool_executor (mock_create.return_value = mock_executor) but remove/mock_reload and any assertions that rely on mock_reload; after making the POST, assert response shape and also verify the reload semantics indirectly (e.g., that mock_create was called with expected parameters and that toolsets counts reflect the temp YAML), and ensure any expectations around PrerequisiteCacheMode.DISABLED or reuse_executor behavior are covered by invoking the real reload instead of mocking it.holmes/admin/admin_api.py (2)
33-42: Runbook count is tied to therunbooktoolset passing the tag filter.
_reload_and_rebuild_toolsetsfilters to[CORE, CLUSTER], so the runbook count here reflects only whateveravailable_runbookshappens to be on the first tool of a toolset named exactly"runbook"in that filtered set. If the runbook toolset is disabled, renamed, or outside those tags,runbookssilently reports0even thoughget_runbook_catalog()would find entries. Consider sourcing the count from_CONFIG.get_runbook_catalog()(what/api/chatalready uses) so the reported number actually matches what requests will see.♻️ Suggested refactor
- runbook_count = 0 - for t in toolsets: - if t.name == "runbook" and t.tools: - runbook_count = len(getattr(t.tools[0], "available_runbooks", [])) - break + catalog = _CONFIG.get_runbook_catalog() + runbook_count = len(catalog.list_available_runbooks()) if catalog else 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/admin/admin_api.py` around lines 33 - 42, _build_toolset_counts currently derives the runbook count by inspecting executor.toolsets (looking for a toolset named "runbook"), which can be wrong due to _reload_and_rebuild_toolsets filtering; change it to source the runbook count from the canonical catalog instead: call _CONFIG.get_runbook_catalog() (or the existing get_runbook_catalog() helper used by /api/chat) and use its length for the "runbooks" value; keep the existing toolsets_total and toolsets_enabled logic in _build_toolset_counts and ensure _CONFIG is referenced/imported where this function runs so the reported runbooks match what API requests will actually see.
14-15: Uninitialized module globals can raiseNameErrorif endpoints are hit beforeinit_admin_app.
_CONFIG: Configand_DAL: SupabaseDalare annotations only — no value is bound. If any request reaches the admin routes beforeinit_admin_apphas run (or if the module is imported in a context that doesn't call it, e.g., some test harnesses),_CONFIG.reload_toolsets()raisesNameErrorrather than a clear "not initialized" error.Consider giving them an explicit
Nonedefault withOptionaltyping and a small guard in each endpoint, so the failure mode is predictable:♻️ Suggested tightening
-_CONFIG: Config -_DAL: SupabaseDal +_CONFIG: Optional[Config] = None +_DAL: Optional[SupabaseDal] = NoneAnd a helper used by each endpoint:
def _require_init() -> tuple[Config, SupabaseDal]: if _CONFIG is None or _DAL is None: raise HTTPException(status_code=503, detail="admin app not initialized") return _CONFIG, _DAL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/admin/admin_api.py` around lines 14 - 15, Module-level annotations _CONFIG and _DAL are uninitialized and can raise NameError if endpoints are hit before init_admin_app runs; change their declarations to Optional types with explicit None defaults (e.g., _CONFIG: Optional[Config] = None, _DAL: Optional[SupabaseDal] = None), add a small helper function _require_init() that checks if either is None and raises HTTPException(status_code=503, detail="admin app not initialized"), call _require_init() at the start of each endpoint that currently uses _CONFIG or _DAL (and use the returned tuple to access them), and ensure init_admin_app assigns to the module globals so subsequent requests see initialized values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/admin/admin_api.py`:
- Around line 88-104: reload_all currently calls _reload_and_rebuild_toolsets()
and _CONFIG.reload_models() separately, allowing an /api/chat request to observe
a partially-updated state; fix by acquiring the shared Config._executor_lock
(the same lock used inside those functions) around the combined operation so
both reloads happen atomically: obtain the lock, call
_reload_and_rebuild_toolsets() then _CONFIG.reload_models(), release the lock,
then compute counts via _build_toolset_counts(executor) and return the
ReloadResponse; reference reload_all, _reload_and_rebuild_toolsets,
_CONFIG.reload_models, and Config._executor_lock when making the change.
In `@holmes/config.py`:
- Around line 161-165: The llm_model_registry method currently holds
self._executor_lock while constructing LLMModelRegistry and accessing self.dal,
which can block unrelated executor paths; change to double-checked init with a
dedicated lock: add a new lock attribute (e.g., _llm_model_registry_lock), first
check if self._llm_model_registry exists and return it if so, then acquire
_llm_model_registry_lock, check again, and if still None construct the
LLMModelRegistry (and access self.dal) while NOT holding self._executor_lock,
assign it to self._llm_model_registry, release the dedicated lock, and return
the registry; update llm_model_registry to stop using _executor_lock for this
initialization.
---
Nitpick comments:
In `@holmes/admin/admin_api.py`:
- Around line 33-42: _build_toolset_counts currently derives the runbook count
by inspecting executor.toolsets (looking for a toolset named "runbook"), which
can be wrong due to _reload_and_rebuild_toolsets filtering; change it to source
the runbook count from the canonical catalog instead: call
_CONFIG.get_runbook_catalog() (or the existing get_runbook_catalog() helper used
by /api/chat) and use its length for the "runbooks" value; keep the existing
toolsets_total and toolsets_enabled logic in _build_toolset_counts and ensure
_CONFIG is referenced/imported where this function runs so the reported runbooks
match what API requests will actually see.
- Around line 14-15: Module-level annotations _CONFIG and _DAL are uninitialized
and can raise NameError if endpoints are hit before init_admin_app runs; change
their declarations to Optional types with explicit None defaults (e.g., _CONFIG:
Optional[Config] = None, _DAL: Optional[SupabaseDal] = None), add a small helper
function _require_init() that checks if either is None and raises
HTTPException(status_code=503, detail="admin app not initialized"), call
_require_init() at the start of each endpoint that currently uses _CONFIG or
_DAL (and use the returned tuple to access them), and ensure init_admin_app
assigns to the module globals so subsequent requests see initialized values.
In `@tests/test_config_reload.py`:
- Around line 113-118: Test globals _CONFIG and _DAL in holmes.admin.admin_api
are left set by init_admin_app and can leak across tests; add an autouse fixture
that yields and then resets holmes.admin.admin_api._CONFIG and
holmes.admin.admin_api._DAL to None in teardown. Locate tests that call
init_admin_app (e.g., TestAdminEndpoints.client) and add a module- or
session-scoped autouse pytest fixture that imports holmes.admin.admin_api and
sets those two globals to None after each test to ensure hermetic state.
- Around line 120-139: The test currently mocks both Config.reload_toolsets and
Config.create_tool_executor which bypasses the real reload logic; change the
test to only mock Config.create_tool_executor so the real Config.reload_toolsets
runs against the temp YAML and exercises _reload_and_rebuild_toolsets; keep the
existing mock for create_tool_executor (mock_create.return_value =
mock_executor) but remove/mock_reload and any assertions that rely on
mock_reload; after making the POST, assert response shape and also verify the
reload semantics indirectly (e.g., that mock_create was called with expected
parameters and that toolsets counts reflect the temp YAML), and ensure any
expectations around PrerequisiteCacheMode.DISABLED or reuse_executor behavior
are covered by invoking the real reload instead of mocking it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fef5caf0-6d21-4bb9-b315-d6299f39dac2
📥 Commits
Reviewing files that changed from the base of the PR and between bbe1a769f262494d802ac37463e54245b709deaf and 0cd54f180bcce4bf317a28fe00ed2337d944a354.
📒 Files selected for processing (4)
holmes/admin/admin_api.pyholmes/config.pyserver.pytests/test_config_reload.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
holmes/admin/admin_api.py (2)
14-15: Bare annotations leave_CONFIG/_DALunbound untilinit_admin_appruns.
_CONFIG: Configand_DAL: SupabaseDalare annotations only — no value is assigned, so they do not exist in the module namespace. If any/api/admin/reload*endpoint is hit beforeinit_admin_app()executes (e.g., a misordered startup, or someone importingadmin_appand mounting it directly), the handler raisesNameErrorrather than a clean, diagnosable error.Consider initializing to
Nonewith anOptionaltype and guarding at the top of each handler (or the helpers), so the failure mode is an explicit 503/RuntimeError with a clear message.♻️ Proposed change
-_CONFIG: Config -_DAL: SupabaseDal +_CONFIG: Optional[Config] = None +_DAL: Optional[SupabaseDal] = NoneAnd a small guard used from helpers:
def _require_initialized() -> tuple[Config, SupabaseDal]: if _CONFIG is None or _DAL is None: raise RuntimeError("admin_api not initialized; call init_admin_app() first") return _CONFIG, _DAL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/admin/admin_api.py` around lines 14 - 15, The module currently uses annotations _CONFIG: Config and _DAL: SupabaseDal without assigning values, which can raise NameError if handlers run before init_admin_app(); change these to Optional by initializing _CONFIG = None and _DAL = None and update their types accordingly, add a helper like _require_initialized() that checks if _CONFIG/_DAL are None and raises a clear RuntimeError (or returns the tuple) and call this guard at the top of each handler/helper that uses _CONFIG or _DAL (reference symbols: _CONFIG, _DAL, init_admin_app, and add _require_initialized).
40-43: Minor: magic string"runbook"for toolset discovery.The toolset name is hardcoded here; if upstream renames the runbook toolset,
runbookssilently drops to 0 with no test failure (only this endpoint breaks). Prefer a shared constant (e.g.,RUNBOOK_TOOLSET_NAME) exported fromholmes/plugins/toolsets/runbook/…, or at minimum a module-level constant in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/admin/admin_api.py` around lines 40 - 43, The code in the for-loop uses the hardcoded string "runbook" to discover the toolset (loop over toolsets and set runbook_count), which is brittle; replace the literal with a shared constant named RUNBOOK_TOOLSET_NAME (preferably exported from the runbook toolset module in holmes/plugins/toolsets/runbook) or define a module-level RUNBOOK_TOOLSET_NAME in admin_api.py, then use that constant in the comparison (t.name == RUNBOOK_TOOLSET_NAME) so upstream renames won't silently break runbook_count; keep the rest of the logic (t.tools and available_runbooks access) 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 `@holmes/admin/admin_api.py`:
- Around line 34-44: The function _build_toolset_counts currently uses
executor.toolsets (config intent) to compute "toolsets_enabled"; change it to
use executor.enabled_toolsets (runtime-usable set) so enabled =
len(executor.enabled_toolsets). Also when computing runbook_count, search
executor.enabled_toolsets instead of executor.toolsets so runbook counts reflect
usable runbooks; keep total = len(executor.toolsets) if you want total
configured toolsets. Update references to ToolExecutor.enabled_toolsets in
_build_toolset_counts accordingly.
---
Nitpick comments:
In `@holmes/admin/admin_api.py`:
- Around line 14-15: The module currently uses annotations _CONFIG: Config and
_DAL: SupabaseDal without assigning values, which can raise NameError if
handlers run before init_admin_app(); change these to Optional by initializing
_CONFIG = None and _DAL = None and update their types accordingly, add a helper
like _require_initialized() that checks if _CONFIG/_DAL are None and raises a
clear RuntimeError (or returns the tuple) and call this guard at the top of each
handler/helper that uses _CONFIG or _DAL (reference symbols: _CONFIG, _DAL,
init_admin_app, and add _require_initialized).
- Around line 40-43: The code in the for-loop uses the hardcoded string
"runbook" to discover the toolset (loop over toolsets and set runbook_count),
which is brittle; replace the literal with a shared constant named
RUNBOOK_TOOLSET_NAME (preferably exported from the runbook toolset module in
holmes/plugins/toolsets/runbook) or define a module-level RUNBOOK_TOOLSET_NAME
in admin_api.py, then use that constant in the comparison (t.name ==
RUNBOOK_TOOLSET_NAME) so upstream renames won't silently break runbook_count;
keep the rest of the logic (t.tools and available_runbooks access) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca9a940b-79ff-4235-8044-8c80d83d79e7
📥 Commits
Reviewing files that changed from the base of the PR and between 0cd54f180bcce4bf317a28fe00ed2337d944a354 and a9831c21fc3397e13a385882eda1faeb0137fc84.
📒 Files selected for processing (2)
holmes/admin/admin_api.pytests/test_config_reload.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_config_reload.py
- Added a new `admin_api.py` module to handle admin endpoints for reloading toolsets and models. - Introduced `reload_toolsets` and `reload_models` endpoints, allowing for dynamic reloading of configurations. - Updated `Config` class with methods to reload toolsets and models, ensuring proper state management. - Integrated admin API into the main application in `server.py`. - Added tests for the new admin endpoints and reload functionality to ensure reliability. This update enhances the system's configurability and management capabilities. Signed-off-by: theTibi <tkorocz@gmail.com>
- Introduced three new POST endpoints: `/api/admin/reload`, `/api/admin/reload/toolsets`, and `/api/admin/reload/models` to allow dynamic reloading of toolsets and models without server restart. - Each endpoint includes detailed descriptions, example requests, and expected responses in the documentation. - Added a note regarding the current unauthenticated state of admin endpoints, recommending network-level access restrictions until authentication is implemented. This update enhances the configurability and management capabilities of the system. Signed-off-by: theTibi <tkorocz@gmail.com>
- Integrated the `init_admin_app` function into `server.py` to streamline the initialization of the admin API. - Updated the `Config` class to include additional logging for toolset reloads, improving error handling when no valid config file is provided. - Refined type hints in `admin_api.py` for better clarity and type safety in function definitions. - Added tests for the admin API endpoints to ensure proper functionality and reliability. This update improves the overall structure and robustness of the admin API and configuration management. Signed-off-by: theTibi <tkorocz@gmail.com>
…tions - Added detailed docstrings to the admin API functions in `admin_api.py` to clarify their purpose and functionality. - Enhanced test class and method descriptions in `test_config_reload.py` to improve readability and understanding of test cases. - This update improves code documentation and test clarity, aiding future development and maintenance. Signed-off-by: theTibi <tkorocz@gmail.com>
theTibi
force-pushed
the
feat/config-reload-endpoints
branch
from
April 28, 2026 13:35
a9831c2 to
ebf62a2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_config_reload.py (1)
194-202: Add 500-path tests for the other two admin endpoints.Only
/api/admin/reload/toolsetsfailure behavior is validated here. Please add analogous failure tests for/api/admin/reload/modelsand/api/admin/reloadto lock error-contract parity.🧪 Suggested additions
+ `@patch`("holmes.config.Config.reload_models") + def test_reload_models_error_returns_500(self, mock_reload, client): + mock_reload.side_effect = RuntimeError("model list invalid") + response = client.post("/api/admin/reload/models") + assert response.status_code == 500 + assert "model list invalid" in response.json()["detail"] + + `@patch`("holmes.config.Config.reload_models") + `@patch`("holmes.config.Config.reload_toolsets") + def test_reload_all_error_returns_500(self, mock_reload_ts, mock_reload_models, client): + mock_reload_models.side_effect = RuntimeError("reload failed") + response = client.post("/api/admin/reload") + assert response.status_code == 500 + assert "reload failed" in response.json()["detail"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config_reload.py` around lines 194 - 202, Add two tests mirroring test_reload_toolsets_error_returns_500 to cover the failure paths for the other admin endpoints: patch holmes.config.Config.reload_models and holmes.config.Config.reload so each mock raises a RuntimeError (e.g., "config file missing"), POST to "/api/admin/reload/models" and "/api/admin/reload" respectively, and assert the response status_code is 500 and the error message appears in response.json()["detail"]; name them similarly (e.g., test_reload_models_error_returns_500 and test_reload_error_returns_500) to keep parity with the existing 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 `@server.py`:
- Line 276: init_admin_app(app, config, dal) is being mounted without any
authorization, exposing privileged admin/reload endpoints; wrap or gate the
mounting with an authentication/authorization check (e.g., require an admin API
key, session, or middleware) so only authorized users can access the admin
routes before calling init_admin_app; implement the check as middleware or a
conditional that validates credentials (reference init_admin_app and the
app/middleware registration flow) and reject unauthorized requests with proper
HTTP status rather than mounting the admin handlers for all callers.
---
Nitpick comments:
In `@tests/test_config_reload.py`:
- Around line 194-202: Add two tests mirroring
test_reload_toolsets_error_returns_500 to cover the failure paths for the other
admin endpoints: patch holmes.config.Config.reload_models and
holmes.config.Config.reload so each mock raises a RuntimeError (e.g., "config
file missing"), POST to "/api/admin/reload/models" and "/api/admin/reload"
respectively, and assert the response status_code is 500 and the error message
appears in response.json()["detail"]; name them similarly (e.g.,
test_reload_models_error_returns_500 and test_reload_error_returns_500) to keep
parity with the existing test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fea44cfc-0776-444a-ba07-cbe5fe67c55a
📥 Commits
Reviewing files that changed from the base of the PR and between a9831c21fc3397e13a385882eda1faeb0137fc84 and ebf62a2.
📒 Files selected for processing (6)
docs/reference/http-api.mdholmes/admin/__init__.pyholmes/admin/admin_api.pyholmes/config.pyserver.pytests/test_config_reload.py
✅ Files skipped from review due to trivial changes (1)
- docs/reference/http-api.md
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/admin/admin_api.py
…error handling - Updated `server.py` to conditionally initialize the admin API based on the `ENABLE_ADMIN_API` environment variable, enhancing flexibility in deployment configurations. - Modified `admin_api.py` to include optional type hints for `_CONFIG` and `_DAL`, and added a `_require_init` function to ensure proper initialization before accessing these variables, improving error handling. - Adjusted tests in `test_config_reload.py` to reflect changes in the admin API's initialization logic. This update enhances the configurability and robustness of the admin API, ensuring it is only active when explicitly enabled. Signed-off-by: theTibi <tkorocz@gmail.com>
- Updated the `reload_models` method in the `Config` class to re-read model-related configuration fields from `model_list.yaml`, ensuring the model registry is rebuilt with current values. - Added conditional logging to provide warnings when the reload is called without a valid config file, improving error handling and clarity. - This change enhances the robustness of model management and provides better feedback during configuration reloads. Signed-off-by: theTibi <tkorocz@gmail.com>
Collaborator
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Updated the `/api/admin/reload` endpoint description to include skill discovery paths. - Modified the response detail to reflect the correct count of skills loaded during configuration reload. - Revised the `/api/admin/reload/toolsets` endpoint description to specify the inclusion of custom skill paths. - Adjusted the internal configuration reload logic to properly handle custom skill paths. - Updated tests to verify the correct handling of custom skill paths and reflect changes in the API responses. These changes improve clarity in the API documentation and enhance the functionality related to skill management in the configuration reload process. Signed-off-by: theTibi <tkorocz@gmail.com>
Contributor
Author
|
@moshemorad can you trigger another review on this, I have updated the branch and renamed runbooks to skills. |
Collaborator
|
@claude review |
… of `load_model_from_file` for improved error handling. Added new utility function for parsing models from YAML files. Enhanced tests to ensure validation errors are raised for unknown config keys, preventing silent failures. Signed-off-by: theTibi <tkorocz@gmail.com>
…llbacks for model and custom skill paths. The `_apply_env_fallbacks` method ensures that these values are correctly set when absent after YAML loading. Additionally, new tests verify that the environment variables are preserved during configuration reloads, maintaining expected behavior. Signed-off-by: theTibi <tkorocz@gmail.com>
Collaborator
|
@claude review |
moshemorad
approved these changes
Jun 15, 2026
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
2 participants
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.
Problem
When HolmesGPT's configuration needs to change (toolsets, models, runbooks), the only option today is to restart the entire server process. This causes downtime, drops in-flight requests, and is slow in environments where the container takes time to initialize (loading toolsets, connecting to databases, etc.).
This is especially painful in production Kubernetes/Docker deployments where config files are mounted via ConfigMaps or bind mounts and updated frequently.
Changes
holmes/config.py: Addreload_toolsets()andreload_models()methods with thread-safe locking. Add lock protection tollm_model_registrylazy property.holmes/admin/admin_api.py(new): FastAPI sub-app mounted at/api/adminwith three POST endpoints:POST /api/admin/reload/toolsets— re-reads config YAML, rebuilds toolsets/runbooksPOST /api/admin/reload/models— re-readsmodel_list.yaml, rebuilds model registryPOST /api/admin/reload— reloads everythingserver.py: Mount admin sub-app (2 lines)tests/test_config_reload.py(new): Unit + integration testsExample response
{ "status": "ok", "component": "all", "detail": "50 toolsets (15 enabled), 4 models", "counts": { "toolsets_total": 50, "toolsets_enabled": 15, "runbooks": 3, "models_loaded": 4 } }Test plan
reload_toolsets()(reset state, YAML pickup, no-config edge case)reload_models()(registry reset, model count)Notes
Summary by CodeRabbit
New Features
Documentation
Tests