Rob 1261 holmes load toolset configs from saas #529
Conversation
WalkthroughThe changes introduce support for fetching Holmes toolset configurations from a SaaS endpoint, add new data models and API client logic, and update the toolset manager to merge SaaS toolset settings when enabled. Test adjustments and minor logging improvements are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SupabaseDal
participant RobustaClient
participant SaaS_API
participant ToolsetManager
Client->>ToolsetManager: list_server_toolsets(dal)
ToolsetManager->>SupabaseDal: get_toolset_configs()
SupabaseDal->>SupabaseDal: get_session_token()
SupabaseDal->>RobustaClient: fetch_holmes_toolset_config(token, account_id, cluster_id)
RobustaClient->>SaaS_API: GET /holmes/toolset/configs
SaaS_API-->>RobustaClient: JSON configs
RobustaClient-->>SupabaseDal: List[HolmesToolsetConfig]
SupabaseDal-->>ToolsetManager: List[HolmesToolsetConfig]
ToolsetManager->>ToolsetManager: Merge SaaS configs into toolsets
ToolsetManager-->>Client: List[Toolset]
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
moshemorad
changed the base branch from
master
to
ROB-1223-holmes-configure-toolsets-from-ui-no-secrets
moshemorad
force-pushed
the
ROB-1261-holmes-load-toolset-configs-from-saas
branch
from
76d21ba to
c04145d
Compare
moshemorad
force-pushed
the
ROB-1261-holmes-load-toolset-configs-from-saas
branch
from
c04145d to
445d9da
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
holmes/clients/robusta_client.py (1)
8-11: TIMEOUT constant is now misleading / unused
TIMEOUTis still set to2, but the new SaaS call below hard-codestimeout=30. Either reuse the shared constant or introduce a dedicated one (TOOLSET_CONFIG_TIMEOUT) to avoid confusion.tests/core/test_toolset_manager.py (1)
167-174: Variable name & patch target mismatch hurts readabilityThe patched attribute is
_list_all_toolsets, but the mock variable is still calledmock_load_toolset_with_status. Rename for clarity and update usages.-@patch("holmes.core.toolset_manager.ToolsetManager._list_all_toolsets") -def test_list_server_toolsets(mock_load_toolset_with_status, toolset_manager): +@patch("holmes.core.toolset_manager.ToolsetManager._list_all_toolsets") +def test_list_server_toolsets(mock_list_all_toolsets, toolset_manager): ... - mock_load_toolset_with_status.return_value = [toolset] + mock_list_all_toolsets.return_value = [toolset]holmes/core/supabase_dal.py (2)
424-426: get_ai_credentials now depends on new helper – good, but add type hintThe refactor is cleaner; add an explicit return annotation for consistency.
- def get_session_token(self): + def get_session_token(self) -> str:
549-552: Surface failures from SaaS fetch
fetch_holmes_toolset_configalready returns[]on errors. Consider logging an explicit warning here so callers know why configs are empty.configs = fetch_holmes_toolset_config(token, self.account_id, self.cluster) if not configs: logging.warning("No SaaS toolset configs fetched for account %s / cluster %s", self.account_id, self.cluster) return configs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
holmes/clients/robusta_client.py(2 hunks)holmes/core/supabase_dal.py(3 hunks)holmes/core/tools.py(1 hunks)holmes/core/toolset_manager.py(3 hunks)holmes/plugins/toolsets/coralogix/api.py(1 hunks)tests/core/test_toolset_manager.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
holmes/core/supabase_dal.py (1)
holmes/clients/robusta_client.py (2)
HolmesToolsetConfig(29-37)fetch_holmes_toolset_config(40-60)
🪛 Pylint (3.3.7)
holmes/clients/robusta_client.py
[refactor] 29-29: Too few public methods (0/2)
(R0903)
🔇 Additional comments (2)
holmes/core/tools.py (1)
350-358: 👍 Bug fix acknowledgedChanging
exclude=("name")toexclude={"name"}prevents accidental per-character exclusion. Nice catch.holmes/core/supabase_dal.py (1)
427-433: TTLCache is not thread-safe – locking is good, but read-then-put can raceThe
with self.lockblock covers the entire check-and-set – perfect. Minor: considercachetools.cachedmethodwhich handles TTL and locking for you.
| def fetch_holmes_toolset_config( | ||
| session_token: str, account_id: str, cluster_id: str | ||
| ) -> List[HolmesToolsetConfig]: | ||
| try: | ||
| headers = {"Authorization": f"Bearer {session_token}"} | ||
| response = requests.get( | ||
| HOLMES_TOOLSET_CONFIG_URL, | ||
| timeout=30, | ||
| headers=headers, | ||
| params={"account_id": account_id, "cluster_id": cluster_id}, | ||
| ) | ||
| response.raise_for_status() | ||
| result = response.json() | ||
|
|
||
| toolset_configs: List[HolmesToolsetConfig] = [ | ||
| HolmesToolsetConfig(**item) for item in result["configs"] | ||
| ] | ||
| return toolset_configs | ||
| except Exception: | ||
| logging.exception("Failed to fetch holmes toolset configs") | ||
| return [] |
There was a problem hiding this comment.
Harden payload parsing & reuse existing timeout constant
A defensive guard around the "configs" key prevents a KeyError and avoids logging an exception for a perfectly valid but unexpected payload. While at it, reuse the (adjusted) timeout constant.
- response = requests.get(
- HOLMES_TOOLSET_CONFIG_URL,
- timeout=30,
+ response = requests.get(
+ HOLMES_TOOLSET_CONFIG_URL,
+ timeout=TOOLSET_CONFIG_TIMEOUT,
headers=headers,
params={"account_id": account_id, "cluster_id": cluster_id},
)
response.raise_for_status()
- result = response.json()
-
- toolset_configs: List[HolmesToolsetConfig] = [
- HolmesToolsetConfig(**item) for item in result["configs"]
- ]
+ result: dict = response.json()
+ configs = result.get("configs", [])
+ if not isinstance(configs, list):
+ logging.warning(
+ "Unexpected payload shape from %s: %s",
+ HOLMES_TOOLSET_CONFIG_URL,
+ result,
+ )
+ return []
+
+ toolset_configs: List[HolmesToolsetConfig] = [
+ HolmesToolsetConfig(**item) for item in configs
+ ]
return toolset_configs📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def fetch_holmes_toolset_config( | |
| session_token: str, account_id: str, cluster_id: str | |
| ) -> List[HolmesToolsetConfig]: | |
| try: | |
| headers = {"Authorization": f"Bearer {session_token}"} | |
| response = requests.get( | |
| HOLMES_TOOLSET_CONFIG_URL, | |
| timeout=30, | |
| headers=headers, | |
| params={"account_id": account_id, "cluster_id": cluster_id}, | |
| ) | |
| response.raise_for_status() | |
| result = response.json() | |
| toolset_configs: List[HolmesToolsetConfig] = [ | |
| HolmesToolsetConfig(**item) for item in result["configs"] | |
| ] | |
| return toolset_configs | |
| except Exception: | |
| logging.exception("Failed to fetch holmes toolset configs") | |
| return [] | |
| def fetch_holmes_toolset_config( | |
| session_token: str, account_id: str, cluster_id: str | |
| ) -> List[HolmesToolsetConfig]: | |
| try: | |
| headers = {"Authorization": f"Bearer {session_token}"} | |
| response = requests.get( | |
| HOLMES_TOOLSET_CONFIG_URL, | |
| timeout=TOOLSET_CONFIG_TIMEOUT, | |
| headers=headers, | |
| params={"account_id": account_id, "cluster_id": cluster_id}, | |
| ) | |
| response.raise_for_status() | |
| result: dict = response.json() | |
| configs = result.get("configs", []) | |
| if not isinstance(configs, list): | |
| logging.warning( | |
| "Unexpected payload shape from %s: %s", | |
| HOLMES_TOOLSET_CONFIG_URL, | |
| result, | |
| ) | |
| return [] | |
| toolset_configs: List[HolmesToolsetConfig] = [ | |
| HolmesToolsetConfig(**item) for item in configs | |
| ] | |
| return toolset_configs | |
| except Exception: | |
| logging.exception("Failed to fetch holmes toolset configs") | |
| return [] |
🤖 Prompt for AI Agents
In holmes/clients/robusta_client.py between lines 40 and 60, add a check to
verify the "configs" key exists in the JSON response before accessing it to
prevent KeyError and avoid unnecessary exceptions. Also, replace the hardcoded
timeout value with the existing timeout constant used elsewhere in the code for
consistency. Adjust the code to handle the case where "configs" is missing by
returning an empty list without logging an error.
| logging.info(f"Response: {response.text}") | ||
| return False, f"Failed with status_code={response.status_code}. {response.text}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Downgrade log level and truncate body to avoid leaking PII
response.text may contain payloads or secrets; logging it at INFO risks exposing them and bloating logs. Use DEBUG and cap size.
- logging.info(f"Response: {response.text}")
+ logging.debug(
+ "Coralogix non-200 response (truncated to 1 kB): %s",
+ response.text[:1024],
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logging.info(f"Response: {response.text}") | |
| return False, f"Failed with status_code={response.status_code}. {response.text}" | |
| logging.debug( | |
| "Coralogix non-200 response (truncated to 1 kB): %s", | |
| response.text[:1024], | |
| ) | |
| return False, f"Failed with status_code={response.status_code}. {response.text}" |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 51-55: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🤖 Prompt for AI Agents
In holmes/plugins/toolsets/coralogix/api.py around lines 54 to 55, the code logs
the full response text at INFO level, which risks exposing sensitive information
and bloating logs. Change the logging level to DEBUG and truncate the response
text to a reasonable length (e.g., first 200 characters) before logging to
reduce PII exposure and log size.
| if ENABLE_HOLMES_TOOLSETS_FROM_SAAS and dal: | ||
| logging.info("Loading toolsets from SaaS") | ||
| toolsets_from_saas: List[HolmesToolsetConfig] = dal.get_toolset_configs() | ||
| for toolset_config in toolsets_from_saas: | ||
| toolset_name = toolset_config.toolset_name | ||
| if toolset_name not in toolsets_by_name: | ||
| logging.warning(f"Toolset {toolset_name} not found in toolsets") | ||
| continue | ||
|
|
||
| logging.info(f"Loading toolset {toolset_name} from SaaS") | ||
| toolset = toolsets_by_name[toolset_name] | ||
| toolset.enabled = toolset_config.enabled | ||
| toolset.config = toolset_config.config | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Config merge overwrites existing settings & loses state
The SaaS branch blindly replaces toolset.config:
toolset.config = toolset_config.config- All local/built-in config keys disappear even when SaaS only overrides a subset.
- A
None/empty SaaSconfigwill wipe a previously valid dict. - No defensive copy – SaaS dict mutations elsewhere could leak.
- toolset.enabled = toolset_config.enabled
- toolset.config = toolset_config.config
+ toolset.enabled = toolset_config.enabled
+
+ # Merge dictionaries shallowly: SaaS keys win, originals preserved
+ if toolset_config.config:
+ merged_cfg = {**(toolset.config or {}), **toolset_config.config}
+ toolset.config = merged_cfgAlso consider deep-merge (dictdiffer, deepmerge) if nested structures are common.
🤖 Prompt for AI Agents
In holmes/core/toolset_manager.py around lines 115 to 128, the code replaces the
entire toolset.config with the SaaS config, which causes loss of existing local
config keys and state, and risks wiping valid configs if SaaS config is None or
empty. To fix this, merge the SaaS config into the existing toolset.config
instead of overwriting it, ensuring to create a defensive copy to avoid mutation
leaks. Consider using a deep merge approach if the config contains nested
dictionaries to preserve all nested settings.
| toolsets_with_status = self._list_all_toolsets( | ||
| dal, | ||
| refresh_status=refresh_status, | ||
| check_prerequisites=True, | ||
| enable_all_toolsets=False, | ||
| toolset_tags=self.server_tool_tags, | ||
| ) |
There was a problem hiding this comment.
refresh_status parameter is now a no-op
list_server_toolsets() still advertises refresh_status, but the new implementation bypasses the cache entirely and never honours the flag – callers can think they are skipping a refresh while they are not.
- toolsets_with_status = self._list_all_toolsets(
- dal,
- check_prerequisites=True,
- enable_all_toolsets=False,
- toolset_tags=self.server_tool_tags,
- )
+ if refresh_status:
+ self.refresh_toolset_status(
+ dal=dal,
+ enable_all_toolsets=False,
+ toolset_tags=self.server_tool_tags,
+ )
+
+ toolsets_with_status = self._list_all_toolsets(
+ dal,
+ check_prerequisites=True,
+ enable_all_toolsets=False,
+ toolset_tags=self.server_tool_tags,
+ )Either honour the argument as above or drop it from the signature to avoid API confusion.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toolsets_with_status = self._list_all_toolsets( | |
| dal, | |
| refresh_status=refresh_status, | |
| check_prerequisites=True, | |
| enable_all_toolsets=False, | |
| toolset_tags=self.server_tool_tags, | |
| ) | |
| if refresh_status: | |
| self.refresh_toolset_status( | |
| dal=dal, | |
| enable_all_toolsets=False, | |
| toolset_tags=self.server_tool_tags, | |
| ) | |
| toolsets_with_status = self._list_all_toolsets( | |
| dal, | |
| check_prerequisites=True, | |
| enable_all_toolsets=False, | |
| toolset_tags=self.server_tool_tags, | |
| ) |
🤖 Prompt for AI Agents
In holmes/core/toolset_manager.py around lines 300 to 305, the
list_server_toolsets() method includes a refresh_status parameter that is no
longer used because the current implementation bypasses the cache and always
refreshes. To fix this, either update the method to respect the refresh_status
flag by conditionally bypassing the cache based on its value or remove the
refresh_status parameter entirely from the method signature and all its callers
to prevent confusion about its effect.
| from holmes.clients.robusta_client import HolmesToolsetConfig | ||
| from holmes.common.env_vars import ENABLE_HOLMES_TOOLSETS_FROM_SAAS |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing safeguard – constant may not be a real boolean
ENABLE_HOLMES_TOOLSETS_FROM_SAAS is imported and later used in a truth-test.
If that constant is the raw environment-variable string (e.g. "false"/"0"), any non-empty value will evaluate to True, unintentionally enabling the SaaS merge.
+from distutils.util import strtobool # builtin, lightweight
...
- if ENABLE_HOLMES_TOOLSETS_FROM_SAAS and dal:
+ # Treat env-var strings such as "0", "false", "no" as False
+ if strtobool(str(ENABLE_HOLMES_TOOLSETS_FROM_SAAS or "0")) and dal:(If distutils is unavailable in your target Python, replace with a small helper that normalises "true", "1", "yes" etc.)
Ensures the feature flag behaves predictably.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from holmes.clients.robusta_client import HolmesToolsetConfig | |
| from holmes.common.env_vars import ENABLE_HOLMES_TOOLSETS_FROM_SAAS | |
| from holmes.clients.robusta_client import HolmesToolsetConfig | |
| from holmes.common.env_vars import ENABLE_HOLMES_TOOLSETS_FROM_SAAS | |
| from distutils.util import strtobool # builtin, lightweight | |
| # ... other imports or code ... | |
| # Treat env-var strings such as "0", "false", "no" as False | |
| if strtobool(str(ENABLE_HOLMES_TOOLSETS_FROM_SAAS or "0")) and dal: | |
| # existing merge logic | |
| ... |
🤖 Prompt for AI Agents
In holmes/core/toolset_manager.py around lines 9 to 10, the imported constant
ENABLE_HOLMES_TOOLSETS_FROM_SAAS is used directly in a truth-test but may be a
raw environment string, causing incorrect truthiness evaluation. To fix this,
convert ENABLE_HOLMES_TOOLSETS_FROM_SAAS to a proper boolean by normalizing its
value (e.g., comparing lowercased string to "true", "1", or "yes") before using
it in any conditional checks, ensuring the feature flag behaves as intended.
No description provided.