mute unncessary info log for console by mainred · Pull Request #994 · HolmesGPT/holmesgpt

mainred · 2025-09-25T09:54:41Z

Make config.dal to optional since dal is passing down to tool_executor and to the tool. This protect the case when we don't use Dal is CLI.

coderabbitai · 2025-09-25T09:54:49Z

Walkthrough

Adjusts logging behavior in configuration and toolset initialization, refines early-exit log levels in Supabase DAL, and ensures gzip is imported for evidence decompression. No public API changes or control-flow alterations beyond logging conditions.

Changes

Cohort / File(s)	Summary of changes
Config logging adjustments `holmes/config.py`	Simplified import for ROBUSTA_CONFIG_PATH. Changed LLM model logging to emit only when more than one model is loaded; retain warning when none found. No public API/signature changes.
Supabase DAL logging and gzip handling `holmes/core/supabase_dal.py`	Added gzip import and maintained its use for "gz" enrichment evidence decompression. Lowered log level from info to debug for disabled state and missing config-file early exits. No control-flow or API changes.
Investigator toolset init cleanup `holmes/plugins/toolsets/investigator/core_investigation.py`	Reordered imports from holmes.core.tools. Removed an initialization info log in CoreInvestigationToolset.init. No API/signature changes.

Sequence Diagram(s)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

ROB-2034: Limit evidence size #978 — Also modifies holmes/core/supabase_dal.py around evidence processing and logging, overlapping with gzip/early-exit handling.
[ROB-1732] Holmes able to unzip logs from findings #731 — Related Supabase DAL changes introducing/adjusting gzip-based evidence decompression and filtering.
Cli logging improvements #445 — Similar logging adjustments in holmes/config.py regarding when to log loaded models.

Suggested reviewers

moshemorad
RoiGlinik

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	The description directly explains the key change of making config.dal optional and its rationale, which aligns with the modifications in the code, and it is clearly related to the changeset.
Title Check	✅ Passed	The title succinctly references muting unnecessary info logs, which aligns with the log-related changes in the pull request, but it omits mention of making config.dal optional as described in the objectives and contains a minor spelling error in "unncessary".

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch mute-info-log

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)

holmes/plugins/toolsets/investigator/core_investigation.py (1)

38-41: Add type hints to satisfy mypy.

Annotate tasks parameter and return type.

Apply this diff:
-    def print_tasks_table(self, tasks):
+    def print_tasks_table(self, tasks: list[Task]) -> None:

holmes/core/supabase_dal.py (1)

323-333: Avoid logging sensitive evidence payloads.

logging.exception(f"...: {data}") can leak PII and large blobs. Log minimal context and use parameterized logging.

Apply this diff:

-        except Exception:
-            logging.exception(f"Unknown issue unzipping gz finding: {data}")
+        except Exception:
+            # Avoid logging full evidence payloads (PII risk)
+            logging.exception(
+                "Unknown issue unzipping gz finding (id=%s, type=%s)",
+                data.get("id"),
+                data.get("enrichment_type"),
+            )

holmes/core/llm.py (1)

218-236: Fix potential UnboundLocalError in token counting.

When content is a dict with a "type" key, message_to_count is not set before use.

Apply this diff:

-                    elif isinstance(message["content"], dict):
-                        if "type" not in message["content"]:
-                            message_to_count = [
-                                {"type": "text", "text": json.dumps(message["content"])}
-                            ]
-                    token_count = litellm.token_counter(
+                    elif isinstance(message["content"], dict):
+                        if "type" not in message["content"]:
+                            message_to_count = [
+                                {"type": "text", "text": json.dumps(message["content"])}
+                            ]
+                        else:
+                            # Already a structured content block
+                            message_to_count = [message["content"]]
+                    token_count = litellm.token_counter(
                         model=self.model, messages=message_to_count
                     )

🧹 Nitpick comments (7)

holmes/plugins/toolsets/investigator/core_investigation.py (1)
65-76: Reduce log verbosity: downgrade task table logs to debug.

Aligns with “mute info logs”; printing a full table at info is noisy.

Apply this diff:
-        logging.info("Updated Investigation Tasks:")
-        logging.info(separator)
-        logging.info(header)
-        logging.info(separator)
+        logging.debug("Updated Investigation Tasks:")
+        logging.debug(separator)
+        logging.debug(header)
+        logging.debug(separator)
@@
-            logging.info(row)
+            logging.debug(row)
@@
-        logging.info(separator)
+        logging.debug(separator)
holmes/config.py (2)
117-121: DAL property is optional but still eagerly instantiates SupabaseDal.

This is okay, but if the goal is to avoid DAL initialization when not needed, consider only constructing DAL when explicitly requested by callers (not from registry creation).

Apply this diff to simplify the getter (no behavior change):
-        return self._dal if self._dal else None
+        return self._dal
123-126: Avoid forcing DAL construction when creating LLM registry.

Passing dal=self.dal triggers DAL creation via the property. Pass the raw backing field to keep DAL truly optional.

Apply this diff:
-            self._llm_model_registry = LLMModelRegistry(self, dal=self.dal)
+            self._llm_model_registry = LLMModelRegistry(self, dal=self._dal)
holmes/core/llm.py (1)
517-522: Environment substitution not applied to parsed model entries.

Reassigning to local params has no effect on models; replace_env_vars_values likely returns a new dict.

Apply this diff:
-        for _, params in models.items():
-            params = replace_env_vars_values(params)
+        for key, params in list(models.items()):
+            models[key] = replace_env_vars_values(params)
tests/test_server_endpoints.py (3)
21-23: Return None (or Instructions) instead of [] for get_global_instructions_for_account.

The real method returns Optional[Instructions]; using [] can mask type issues.

Apply this diff:
-    mock_dal.get_global_instructions_for_account.return_value = []
+    mock_dal.get_global_instructions_for_account.return_value = None
81-90: Same note: prefer None for get_global_instructions_for_account mock.

Apply this diff:
-    mock_dal.get_global_instructions_for_account.return_value = []
+    mock_dal.get_global_instructions_for_account.return_value = None
141-150: Same note: prefer None for get_global_instructions_for_account mock.

Apply this diff:
-    mock_dal.get_global_instructions_for_account.return_value = []
+    mock_dal.get_global_instructions_for_account.return_value = None

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 85ce5b3 and e05ac64ceb967a1b9d53bdf67b1f32280bcaa234.

📒 Files selected for processing (5)

holmes/config.py (5 hunks)
holmes/core/llm.py (3 hunks)
holmes/core/supabase_dal.py (3 hunks)
holmes/plugins/toolsets/investigator/core_investigation.py (1 hunks)
tests/test_server_endpoints.py (6 hunks)

🧰 Additional context used

📓 Path-based instructions (5)

**/*.py