Skip to content

Yiqi/fix bugs - #1332

Closed
xiaoqi-7 wants to merge 7 commits into
HolmesGPT:masterfrom
xiaoqi-7:yiqi/fix-bugs
Closed

xiaoqi-7 wants to merge 7 commits into
HolmesGPT:masterfrom
xiaoqi-7:yiqi/fix-bugs

Conversation

@xiaoqi-7

@xiaoqi-7 xiaoqi-7 commented Jan 5, 2026 •

Copy link
Copy Markdown

Summary by CodeRabbit

Release Notes

  • New Features

    • Remote model metadata support for improved context window sizing and token optimization on hosted vLLM models.
    • Enhanced conversation compaction with intelligent message handling and context window management.
    • Added "now" keyword support for timestamp operations.
  • Changes

    • Removed mandatory runbook enforcement from investigation procedures.
    • Added AKS toolset integrations (disabled by default).

✏️ Tip: You can customize this high-level summary in your review settings.

@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jan 5, 2026 •

Copy link
Copy Markdown
Contributor

Walkthrough

This PR introduces remote model metadata support for hosted vLLM models, comprehensively refactors the message compaction workflow with context-window error handling and retry logic, removes runbook enforcement from prompt templates, enhances utilities (e.g., to_unix handles "now" string), and removes numerous test fixtures for Loki, Datadog, electricity-bidding, and other scenarios. Includes a critical syntax error in test utilities.

Changes

Cohort / File(s) Summary
LLM Core: Remote Model Metadata Support
holmes/core/llm.py
Extended DefaultLLM with HTTP URL utilities, remote metadata fetching for OpenAI-compatible hosts, and integration into context window and output token sizing; adds new private methods and fields for thread-safe remote metadata caching.
Message Truncation & Compaction Refactoring
holmes/core/truncation/message_truncation.py, holmes/core/truncation/compaction.py, holmes/core/truncation/input_context_window_limiter.py
New message_truncation.py module implements tool-message truncation with metadata; compaction.py adds comprehensive error detection, pre-shrinking, and retry logic on context-window errors; input_context_window_limiter.py adjusts imports.
Prompt Template Simplification
holmes/plugins/prompts/base_user_prompt.jinja2, holmes/plugins/prompts/investigation_procedure.jinja2
Removed runbook conditional inclusion and enforcement blocks; investigation procedure now omits mandatory runbook preconditions.
Utilities & Script Updates
holmes/plugins/toolsets/utils.py, scripts/run_eval_setup.py
to_unix now handles "now" string (case-insensitive) and uses tz-aware imports; run_eval_setup.py adds explicit bash executor to subprocess.run.
Test Utilities: OpenAI Client Compatibility Shims
tests/llm/utils/classifiers.py
Added proxy classes (_CompletionsProxy, _ChatProxy, _MaxTokensCompatClient) to handle max_completion_tokens fallback on BadRequestError; create_llm_client now wraps client unconditionally.
Test Utilities: Critical Syntax Error
tests/llm/utils/commands.py
⚠️ Syntax Error: _invoke_command subprocess.run call contains duplicate executable= keyword argument, breaking test execution.
Test Utilities: Toolset Configuration
tests/llm/utils/default_toolsets.yaml
Disabled runbook, robusta, and connectivity_check toolsets; added disabled aks/core and aks/node-health entries.
Test Fixture Removals (Loki)
tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/*
Removed entire Loki test fixture: app-direct-push.py (LogBatcher, HealthHandler), payment-api deployment manifests, test_case.yaml, toolsets.yaml.
Test Fixture Removals (Electricity Bidding)
tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/*, tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/*
Removed Prometheus, bidder-app, HPA, k6 load-test manifests, run-bidder-check.sh, wait-for-scaling.sh, and all associated test_case.yaml and toolsets.yaml files.
Test Fixture Removals (Datadog Traces)
tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/*
Removed all-in-one Kubernetes manifest, Prometheus config, Datadog traces toolset, validate_promo_performance.py, and test_case.yaml.
Test Fixture Removals (Database/RDS)
tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/*
Removed customer-orders deployment and service manifests, test_case.yaml, and toolsets.yaml.
Test Fixture Removals (Datadog Metrics & Logs)
tests/llm/fixtures/test_ask_holmes/91{a,b,c,d,e,f,g,h,i}_datadog_*/*
Removed Datadog metrics/logs test fixtures across 9 directories: Kubernetes manifests, test_case.yaml files, send_custom_metrics.sh, send_datadog_logs.py, and toolsets.yaml entries.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant Compact as compact_conversation_history()
    participant Shrink as _shrink_messages_to_fit_compaction_window()
    participant LLM as LLM (Compaction)
    participant ErrorHandler as Error Detection

    App->>Compact: (messages, llm)
    activate Compact
    
    rect rgb(220, 240, 255)
    Note over Compact,Shrink: Phase 1: Prepare & Shrink
    Compact->>Compact: _prepare_messages_for_compaction()
    Compact->>Shrink: shrink messages to fit context window
    activate Shrink
    Shrink->>Shrink: compute reserved output tokens
    Shrink->>Shrink: truncate_messages_to_fit_context()
    Shrink->>Shrink: drop oldest / hard-truncate largest
    Shrink-->>Compact: shrunk messages
    deactivate Shrink
    end
    
    rect rgb(240, 255, 240)
    Note over Compact,LLM: Phase 2: Compaction & Retry Loop
    Compact->>Compact: render compaction prompt
    Compact->>LLM: call LLM with shrunk messages
    activate LLM
    LLM-->>Compact: result or error
    deactivate LLM
    end
    
    Compact->>ErrorHandler: _is_context_window_exceeded_error()?
    activate ErrorHandler
    alt Context Window Exceeded
        ErrorHandler-->>Compact: true
        rect rgb(255, 240, 240)
        Note over Compact,Shrink: Phase 3: Retry with Aggressive Shrinking
        Compact->>Shrink: re-shrink with increased reserved tokens
        Shrink-->>Compact: more aggressively shrunk messages
        Compact->>LLM: retry compaction
        LLM-->>Compact: result
        end
    else No Context Window Error
        ErrorHandler-->>Compact: false
        Note over Compact: Return result as-is
    end
    deactivate ErrorHandler
    
    Compact-->>App: compacted conversation
    deactivate Compact
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Rationale: The diff exhibits high heterogeneity and logic density. Key complexity drivers include: (1) a critical syntax error in tests/llm/utils/commands.py (duplicate keyword argument) that breaks test execution; (2) significant message compaction refactoring with multi-phase error detection and retry logic requiring careful state management review; (3) new remote model metadata support with threading, caching, and fallback logic in holmes/core/llm.py; (4) extensive test fixture removals (though repetitive, they still require verification of completeness and absence of unintended side effects). The syntax error alone elevates priority, and the compaction logic requires tracing through multiple helper functions and error paths.

Possibly related PRs

  • #958 — Overlaps with tool-message truncation metadata instrumentation and TruncationMetadata usage patterns.
  • #805 — Introduces MAX_OUTPUT_TOKEN_RESERVATION constant that this PR's new truncation logic directly depends on.
  • #831 — Has direct file-level conflicts in test fixtures (22_high_latency_dbi_down manifest.yaml and test_case.yaml).

Suggested reviewers

  • moshemorad

Pre-merge checks

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Yiqi/fix bugs' is vague and generic, using non-descriptive terms that do not convey meaningful information about the substantial changes in this changeset. Replace with a specific, descriptive title that captures the main changes, such as: 'Add remote model metadata support and message compaction workflow' or a more focused alternative reflecting the primary objective.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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.

@netlify

netlify Bot commented Jan 5, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for holmes-docs ready!

Name Link
🔨 Latest commit 456b434
🔍 Latest deploy log https://app.netlify.com/projects/holmes-docs/deploys/695c43a3eccf1000082e5f71
😎 Deploy Preview https://deploy-preview-1332--holmes-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Fix all issues with AI Agents 🤖
In @holmes/core/truncation/message_truncation.py:
- Around line 11-40: _truncate_tool_message may pass None into
TruncationMetadata because msg.get("tool_call_id") and msg.get("name") can
return None; fix by coercing these to strings or providing defaults before
constructing TruncationMetadata (e.g., tool_call_id =
str(msg.get("tool_call_id") or "") and tool_name = str(msg.get("name") or "")),
use these safe variables in the logging message and in the
TruncationMetadata(...) call so Pydantic validation won't fail at runtime.

In @holmes/plugins/toolsets/utils.py:
- Around line 32-41: The function to_unix contains redundant local imports of
datetime; remove the inline "from datetime import datetime, timezone" and
"import datetime as dtmod" and instead use the module-level datetime import
already present at top of the file; update references inside to_unix to use the
top-level names (e.g., datetime.now(datetime.timezone.utc) and
dt.replace(tzinfo=datetime.timezone.utc)) so no local imports remain.

In @tests/llm/utils/classifiers.py:
- Around line 49-56: Add type hints to class _ChatProxy: annotate the __init__
parameter chat (e.g., chat: Any or a more specific Chat type), set self._chat:
Any, and type self.completions as _CompletionsProxy; annotate __getattr__ as def
__getattr__(self, name: str) -> Any. Also import typing.Any (or the concrete
Chat type) at the top. This keeps the delegation pattern but makes type
annotations explicit for _ChatProxy, __init__, self.completions, and
__getattr__.
- Around line 59-66: Add missing type hints to the _MaxTokensCompatClient proxy:
annotate the class constructor parameter (client) with the appropriate client
type or typing.Any, type the self._client attribute, annotate the chat attribute
as _ChatProxy, and add a return type of Any (or the client type) to __getattr__
as def __getattr__(self, name: str) -> Any; ensure necessary typing imports are
added.
- Around line 23-46: The proxy classes lack type annotations; import typing.Any
and add type hints: annotate _CompletionsProxy.__init__(self, completions: Any)
-> None, _CompletionsProxy.create(self, *args: Any, **kwargs: Any) -> Any, and
_CompletionsProxy.__getattr__(self, name: str) -> Any, and apply the same
pattern to the _ChatProxy and _MaxTokensCompatClient classes so all proxy
constructors, create/handler methods, varargs, kwargs and __getattr__ signatures
use Any and return Any (or None for __init__).

Caution

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

⚠️ Outside diff range comments (2)
tests/llm/utils/commands.py (1)

156-167: Critical: Duplicate keyword argument executable causes SyntaxError.

The subprocess.run call specifies executable="/bin/bash" twice (lines 159 and 162). This is a Python syntax error that will prevent the module from loading.

🔎 Proposed fix
         result = subprocess.run(
             command,
             shell=True,
             executable="/bin/bash",  # Force bash instead of default /bin/sh
             capture_output=True,
             text=True,
-            executable="/bin/bash",
             check=True,
             stdin=subprocess.DEVNULL,
             cwd=cwd,
             timeout=actual_timeout,
         )
holmes/core/truncation/input_context_window_limiter.py (1)

61-133: Critical: Duplicate function definition shadows the import, and MAX_OUTPUT_TOKEN_RESERVATION is undefined.

The function truncate_messages_to_fit_context is imported from message_truncation on line 17, but then redefined locally on lines 61-133. This shadows the import and creates code duplication. Additionally, line 87 references MAX_OUTPUT_TOKEN_RESERVATION which is not imported in this file—it's only imported in message_truncation.py. This will cause a NameError at runtime.

The same duplication issue applies to _truncate_tool_message (lines 24-53), which also exists in message_truncation.py.

Remove both duplicated functions and use the imported versions instead. The import statement on line 17 is already correct and should be the single source of truth.

🧹 Nitpick comments (2)
scripts/run_eval_setup.py (1)

44-44: Consider removing shell=True to address the security concern flagged by Ruff.

While adding executable="/bin/bash" makes the shell explicit, Ruff flags the use of shell=True as a potential security issue (S602). Although this is a developer-facing script and the YAML files are trusted repository content, using shell=True with externally loaded data degrades security posture.

If the commands in test_case.yaml don't require shell features (pipes, wildcards, etc.), consider refactoring to use shell=False with a list of arguments. Alternatively, add explicit validation that data[section] contains expected content before execution.

🔎 Example refactor without shell=True (if commands are simple)
-            result = subprocess.run(data[section], shell=True, executable="/bin/bash")
+            import shlex
+            result = subprocess.run(shlex.split(data[section]))

Note: This only works if commands don't use shell-specific features. If shell features are required, the current approach is acceptable given this is a trusted dev script.

holmes/core/truncation/compaction.py (1)

114-115: Consider logging the swallowed exception for debugging.

The try-except-pass pattern silently swallows truncation errors, which could make debugging difficult if truncation consistently fails.

🔎 Proposed fix
     try:
         messages = truncate_messages_to_fit_context(
             messages=messages,
             max_context_size=max_context_size,
             maximum_output_token=reserved_output_tokens,
             count_tokens_fn=llm.count_tokens,
         ).truncated_messages
         if fits(messages):
             return messages
-    except Exception:
-        pass
+    except Exception as e:
+        logging.debug("Truncation pass failed during shrinking, falling back to message dropping: %s", e)
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a4c86a and 456b434.

📒 Files selected for processing (63)
  • holmes/core/llm.py
  • holmes/core/truncation/compaction.py
  • holmes/core/truncation/input_context_window_limiter.py
  • holmes/core/truncation/message_truncation.py
  • holmes/plugins/prompts/base_user_prompt.jinja2
  • holmes/plugins/prompts/investigation_procedure.jinja2
  • holmes/plugins/toolsets/utils.py
  • scripts/run_eval_setup.py
  • tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/app-direct-push.py
  • tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/payment-api-direct.yaml
  • tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/bidder-app.yaml
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/bidding_system.md
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/prometheus-config.yaml
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/run-bidder-check.sh
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/bidder-v1.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/bidder-v2.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/hpa.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/k6-v1-traffic.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/k6-v2-traffic.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/prometheus-config.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/wait-for-scaling.sh
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/holmes-all-in-one-fast.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/prometheus-config.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/validate_promo_performance.py
  • tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91a_datadog_metrics_missing_namespace/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91a_datadog_metrics_missing_namespace/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91c_datadog_metrics_deployment/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/91c_datadog_metrics_deployment/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91c_datadog_metrics_deployment/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/send_custom_metrics.sh
  • tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/send_datadog_logs.py
  • tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yaml
  • tests/llm/utils/classifiers.py
  • tests/llm/utils/commands.py
  • tests/llm/utils/default_toolsets.yaml
💤 Files with no reviewable changes (54)
  • tests/llm/fixtures/test_ask_holmes/91a_datadog_metrics_missing_namespace/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/send_custom_metrics.sh
  • tests/llm/fixtures/test_ask_holmes/91c_datadog_metrics_deployment/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/91c_datadog_metrics_deployment/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/holmes-all-in-one-fast.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/bidder-v1.yaml
  • tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/prometheus-config.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/wait-for-scaling.sh
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/bidder-app.yaml
  • tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/prometheus-config.yaml
  • tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91h_datadog_logs_empty_query_with_url/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/k6-v1-traffic.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/test_case.yaml
  • holmes/plugins/prompts/base_user_prompt.jinja2
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/bidding_system.md
  • tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/91c_datadog_metrics_deployment/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/hpa.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91e_datadog_custom_metrics/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/prometheus-config.yaml
  • tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/run-bidder-check.sh
  • tests/llm/fixtures/test_ask_holmes/91a_datadog_metrics_missing_namespace/test_case.yaml
  • holmes/plugins/prompts/investigation_procedure.jinja2
  • tests/llm/fixtures/test_ask_holmes/91f_datadog_logs_historical_pod/send_datadog_logs.py
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/bidder-v2.yaml
  • tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91i_datadog_metrics_empty_query_with_url/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/91g_datadog_metrics_mismatched_pod/test_case.yaml
  • tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/161_bidding_version_performance/k6-v2-traffic.yaml
  • tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/payment-api-direct.yaml
  • tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91b_datadog_metrics_pod_exists/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/160_electricity_market_bidding_bug/toolsets.yaml
  • tests/llm/fixtures/test_ask_holmes/91d_datadog_metrics_historical_pod/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/22_high_latency_dbi_down/manifest.yaml
  • tests/llm/fixtures/test_ask_holmes/164_datadog_traces_coupon_code/validate_promo_performance.py
  • tests/llm/fixtures/test_ask_holmes/100a_loki_historical_logs/app-direct-push.py
🧰 Additional context used
📓 Path-based instructions (7)
tests/llm/**/*.{py,yaml}

📄 CodeRabbit inference engine (CLAUDE.md)

All pod names must be unique across tests (never reuse pod names between tests)

Files:

  • tests/llm/utils/default_toolsets.yaml
  • tests/llm/utils/classifiers.py
  • tests/llm/utils/commands.py
tests/llm/**/*.yaml

📄 CodeRabbit inference engine (CLAUDE.md)

tests/llm/**/*.yaml: Never use resource names that hint at the problem or expected behavior in evals (avoid broken-pod, test-project-that-does-not-exist, crashloop-app)
Only use valid tags from pyproject.toml for LLM tests - invalid tags cause test collection failures
Use exit 1 when setup verification fails to fail the test early
Poll real API endpoints and check for expected content in setup verification, don't just test pod readiness
Use kubectl exec over port forwarding for setup verification to avoid port conflicts
Use sleep 1 instead of sleep 5 for retry loops, remove unnecessary sleeps, reduce timeout values (60s for pod readiness, 30s for API verification)
Use retry loops for kubectl wait to handle race conditions, don't use bare kubectl wait immediately after resource creation
Use realistic logs in eval tests, not fake/obvious logs like 'Memory usage stabilized at 800MB'
Use realistic filenames in eval tests, not hints like 'disk_consumer.py' - use names like 'training_pipeline.py'
Use real-world scenarios in eval tests (ML pipelines with checkpoint issues, database connection pools) not simulated scenarios

Files:

  • tests/llm/utils/default_toolsets.yaml
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files

Files:

  • holmes/core/truncation/message_truncation.py
  • holmes/core/truncation/compaction.py
  • scripts/run_eval_setup.py
  • tests/llm/utils/classifiers.py
  • holmes/core/llm.py
  • tests/llm/utils/commands.py
  • holmes/plugins/toolsets/utils.py
  • holmes/core/truncation/input_context_window_limiter.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Tests should match source structure under tests/
Live execution is now enabled by default to ensure tests match real-world behavior

Files:

  • tests/llm/utils/classifiers.py
  • tests/llm/utils/commands.py
tests/llm/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/llm/**/*.py: Running LLM tests: use -k flag with test name pattern, NOT full test path with brackets
LLM tests must use dedicated namespace app- (e.g., app-01, app-02) to prevent conflicts when tests run simultaneously

Files:

  • tests/llm/utils/classifiers.py
  • tests/llm/utils/commands.py
holmes/plugins/toolsets/**/*.{py,yaml}

📄 CodeRabbit inference engine (CLAUDE.md)

holmes/plugins/toolsets/**/*.{py,yaml}: All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including exact query/command executed, time ranges/parameters/filters used, and full API error response (status code and message)
For 'no data' responses in toolsets, specify what was searched and where
Never return unbounded data from APIs - always include filter parameters on tools that query collections

Files:

  • holmes/plugins/toolsets/utils.py
holmes/plugins/toolsets/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

holmes/plugins/toolsets/**/*.py: Use requests library for HTTP calls in Python toolsets, not specialized client libraries like opensearchpy
Implement simple Pydantic config class with validation for Python toolsets
Include health check in prerequisites_callable() method for Python toolsets
Each tool in Python toolsets should be a thin wrapper around a single API endpoint
Use JsonFilterMixin for client-side filtering when server-side filtering is not possible, adding max_depth and jq parameters
Maintain backwards compatibility in toolset config using Pydantic's extra='allow' when renaming config fields
Only define current field names in toolset config schema with extra='allow' to avoid polluting model_dump() output with deprecated fields
Use @model_validator(mode='after') in toolset config to map old field names to new names and log deprecation warnings
Bash toolset validates commands for safety

Files:

  • holmes/plugins/toolsets/utils.py
🧠 Learnings (8)
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to tests/llm/fixtures/**/*.yaml : Custom toolsets for eval tests: Create separate toolsets.yaml file, never put toolset config in test_case.yaml

Applied to files:

  • tests/llm/utils/default_toolsets.yaml
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to tests/llm/fixtures/**/*.yaml : Toolset config in eval tests must go under config field: toolsets.toolset_name.enabled.config

Applied to files:

  • tests/llm/utils/default_toolsets.yaml
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to tests/llm/fixtures/**/*.yaml : Required files for eval tests: test_case.yaml, infrastructure manifests, and toolsets.yaml (if needed)

Applied to files:

  • tests/llm/utils/default_toolsets.yaml
📚 Learning: 2026-01-05T11:14:20.209Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.209Z
Learning: New toolsets require integration tests

Applied to files:

  • tests/llm/utils/default_toolsets.yaml
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to tests/llm/fixtures/**/*.yaml : Custom runbooks in eval tests: Add runbooks field in test_case.yaml (use runbooks: {} for empty catalog)

Applied to files:

  • tests/llm/utils/default_toolsets.yaml
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to tests/llm/fixtures/**/*.yaml : Use include_tool_calls: true to verify tool was called when output values are too generic to rule out hallucinations

Applied to files:

  • tests/llm/utils/default_toolsets.yaml
📚 Learning: 2026-01-05T11:14:20.208Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.208Z
Learning: Applies to tests/llm/**/*.yaml : Use sleep 1 instead of sleep 5 for retry loops, remove unnecessary sleeps, reduce timeout values (60s for pod readiness, 30s for API verification)

Applied to files:

  • tests/llm/utils/default_toolsets.yaml
📚 Learning: 2026-01-05T11:14:20.209Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.209Z
Learning: When using Anthropic models for LLM tests, set CLASSIFIER_MODEL to OpenAI (Anthropic not supported as classifier)

Applied to files:

  • tests/llm/utils/classifiers.py
🧬 Code graph analysis (4)
holmes/core/truncation/message_truncation.py (2)
holmes/core/models.py (2)
  • TruncationMetadata (11-16)
  • TruncationResult (19-21)
holmes/utils/sentry_helper.py (1)
  • capture_tool_truncations (8-10)
holmes/core/truncation/compaction.py (3)
holmes/core/llm.py (7)
  • LLM (130-173)
  • get_maximum_output_token (140-141)
  • get_maximum_output_token (534-567)
  • count_tokens (157-160)
  • count_tokens (395-449)
  • get_context_window_size (136-137)
  • get_context_window_size (364-392)
holmes/core/truncation/input_context_window_limiter.py (1)
  • truncate_messages_to_fit_context (61-133)
holmes/core/truncation/message_truncation.py (1)
  • truncate_messages_to_fit_context (48-104)
tests/llm/utils/classifiers.py (2)
server.py (1)
  • chat (350-419)
tests/test_server_endpoints.py (1)
  • client (10-11)
holmes/core/truncation/input_context_window_limiter.py (1)
holmes/core/truncation/message_truncation.py (1)
  • truncate_messages_to_fit_context (48-104)
🪛 Ruff (0.14.10)
holmes/core/truncation/message_truncation.py

68-70: Create your own exception

(TRY002)


68-70: Avoid specifying long messages outside the exception class

(TRY003)

holmes/core/truncation/compaction.py

25-25: Do not catch blind exception: Exception

(BLE001)


36-36: Do not catch blind exception: Exception

(BLE001)


114-115: try-except-pass detected, consider logging the exception

(S110)


114-114: Do not catch blind exception: Exception

(BLE001)


218-218: Do not catch blind exception: Exception

(BLE001)

scripts/run_eval_setup.py

44-44: subprocess call with shell=True identified, security issue

(S602)

holmes/core/llm.py

76-76: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.

(S310)


77-77: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.

(S310)


83-83: Do not catch blind exception: Exception

(BLE001)

tests/llm/utils/commands.py

162-162: Duplicate keyword argument "executable"

(invalid-syntax)

🔇 Additional comments (8)
holmes/core/llm.py (2)

63-85: LGTM with minor observation on URL scheme handling.

The remote model metadata fetching is well-implemented with:

  • LRU caching to avoid repeated network calls
  • Configurable timeout via environment variable
  • Graceful fallback returning empty list on failure

The static analysis warning about URL scheme validation (S310) is addressed by the _is_http_url check in _load_remote_model_metadata_if_needed (line 242), which ensures only http:// or https:// URLs reach this function.


236-264: Well-implemented double-checked locking for remote metadata loading.

The implementation correctly:

  • Uses early returns to skip non-applicable cases
  • Implements proper double-checked locking pattern with _remote_model_metadata_lock
  • Generates reasonable model ID candidates for matching
  • Sets the loaded flag even on failure to avoid repeated attempts
holmes/core/truncation/message_truncation.py (1)

48-104: LGTM - Fair token allocation algorithm for tool message truncation.

The implementation correctly:

  • Reserves output tokens with a cap at MAX_OUTPUT_TOKEN_RESERVATION
  • Validates non-tool messages fit before processing tools
  • Sorts tool messages by size and allocates space fairly
  • Reports truncations to Sentry for observability

The acknowledged TODOs (lines 43-47) about character vs. token truncation are reasonable future improvements.

holmes/core/truncation/compaction.py (2)

40-87: LGTM - Robust message sanitization for cross-provider compatibility.

The function correctly handles edge cases:

  • Stringifies complex content types safely
  • Preserves context for tool calls even when stripping the structure
  • Converts tool messages to assistant role with descriptive headers
  • Provides fallback role if missing

This defensive approach prevents JSON parsing errors across different vLLM/OpenAI-compatible providers.


229-253: Good retry pattern for context window errors.

The retry logic with more aggressive shrinking (reserved_retry = min(512, reserved_output_tokens)) provides resilience when initial token estimates are insufficient. Re-preparing messages after shrinking ensures consistency.

tests/llm/utils/default_toolsets.yaml (1)

22-25: No action needed—AKS toolsets already have integration tests.

Integration tests for aks/core and aks/node-health already exist in tests/plugins/toolsets/test_aks_transformers.py, and these toolsets are actively used across multiple LLM eval test fixtures (7 instances found). Adding them to default_toolsets.yaml in a disabled state is simply configuration management, not introducing new toolsets.

Likely an incorrect or invalid review comment.

tests/llm/utils/classifiers.py (2)

148-149: LGTM: Compatibility shim correctly applied.

The return value now wraps the client with the compatibility layer, enabling transparent handling of models that require max_completion_tokens.


153-160: No changes required. The _MaxTokensCompatClient proxy pattern works correctly with wrap_openai through proper __getattr__ delegation. The code is already in use across multiple LLM tests (test_investigate.py, test_compaction.py, test_workload_health.py) with no errors, confirming compatibility with Braintrust's wrapper.

Comment on lines +11 to +40
def _truncate_tool_message(
msg: dict, allocated_space: int, needed_space: int
) -> TruncationMetadata:
msg_content = msg["content"]
tool_call_id = msg.get("tool_call_id")
tool_name = msg.get("name")

# Ensure the indicator fits in the allocated space
if allocated_space > len(TRUNCATION_NOTICE):
original = msg_content if isinstance(msg_content, str) else str(msg_content)
msg["content"] = (
original[: allocated_space - len(TRUNCATION_NOTICE)] + TRUNCATION_NOTICE
)
end_index = allocated_space - len(TRUNCATION_NOTICE)
else:
msg["content"] = TRUNCATION_NOTICE[:allocated_space]
end_index = allocated_space

msg.pop("token_count", None) # Remove token_count if present
logging.info(
f"Truncating tool message '{tool_name}' from {needed_space} to {allocated_space} tokens"
)
truncation_metadata = TruncationMetadata(
tool_call_id=tool_call_id,
start_index=0,
end_index=end_index,
tool_name=tool_name,
original_token_count=needed_space,
)
return truncation_metadata

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Potential type mismatch: tool_call_id and tool_name may be None.

Lines 15-16 use .get() which can return None, but TruncationMetadata expects tool_call_id: str and tool_name: str. If a tool message lacks these fields, Pydantic validation will fail at runtime.

🔎 Proposed fix
 def _truncate_tool_message(
     msg: dict, allocated_space: int, needed_space: int
 ) -> TruncationMetadata:
     msg_content = msg["content"]
-    tool_call_id = msg.get("tool_call_id")
-    tool_name = msg.get("name")
+    tool_call_id = msg.get("tool_call_id") or "unknown"
+    tool_name = msg.get("name") or "unknown"
🤖 Prompt for AI Agents
In @holmes/core/truncation/message_truncation.py around lines 11-40,
_truncate_tool_message may pass None into TruncationMetadata because
msg.get("tool_call_id") and msg.get("name") can return None; fix by coercing
these to strings or providing defaults before constructing TruncationMetadata
(e.g., tool_call_id = str(msg.get("tool_call_id") or "") and tool_name =
str(msg.get("name") or "")), use these safe variables in the logging message and
in the TruncationMetadata(...) call so Pydantic validation won't fail at
runtime.

Comment on lines 32 to 41
def to_unix(timestamp_str: str) -> int:
dt = parser.parse(timestamp_str)
if dt.tzinfo is None:
dt = dt.replace(tzinfo=datetime.timezone.utc)
if str(timestamp_str).strip().lower() == "now":
from datetime import datetime, timezone
dt = datetime.now(timezone.utc)
else:
dt = parser.parse(timestamp_str)
if dt.tzinfo is None:
import datetime as dtmod
dt = dt.replace(tzinfo=dtmod.timezone.utc)
return int(dt.timestamp())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Remove redundant local imports; use module-level datetime import.

The datetime module is already imported at the top of the file (line 1). The local imports on lines 34 and 39 are redundant and violate the coding guideline requiring imports at the top of the file.

🔎 Proposed fix
 def to_unix(timestamp_str: str) -> int:
     if str(timestamp_str).strip().lower() == "now":
-        from datetime import datetime, timezone
-        dt = datetime.now(timezone.utc)
+        dt = datetime.datetime.now(datetime.timezone.utc)
     else:
         dt = parser.parse(timestamp_str)
         if dt.tzinfo is None:
-            import datetime as dtmod
-            dt = dt.replace(tzinfo=dtmod.timezone.utc)
+            dt = dt.replace(tzinfo=datetime.timezone.utc)
     return int(dt.timestamp())
🤖 Prompt for AI Agents
In @holmes/plugins/toolsets/utils.py around lines 32-41, The function to_unix
contains redundant local imports of datetime; remove the inline "from datetime
import datetime, timezone" and "import datetime as dtmod" and instead use the
module-level datetime import already present at top of the file; update
references inside to_unix to use the top-level names (e.g.,
datetime.now(datetime.timezone.utc) and
dt.replace(tzinfo=datetime.timezone.utc)) so no local imports remain.

Comment on lines +23 to +46
class _CompletionsProxy:
def __init__(self, completions):
self._completions = completions

def create(self, *args, **kwargs):
# Some newer/reasoning models reject `max_tokens` and require `max_completion_tokens`.
# Be conservative: try as-is first; if the server rejects, retry with renamed param.
try:
return self._completions.create(*args, **kwargs)
except openai.BadRequestError as e:
msg = str(e)
if (
"max_tokens" in msg
and "max_completion_tokens" in msg
and "max_tokens" in kwargs
and "max_completion_tokens" not in kwargs
):
retry_kwargs = dict(kwargs)
retry_kwargs["max_completion_tokens"] = retry_kwargs.pop("max_tokens")
return self._completions.create(*args, **retry_kwargs)
raise

def __getattr__(self, name):
return getattr(self._completions, name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, verify the file and see the actual implementation
cat -n tests/llm/utils/classifiers.py | sed -n '1,70p'

Repository: HolmesGPT/holmesgpt

Length of output: 2647


🏁 Script executed:

# Check if Azure OpenAI or other providers are used in the codebase
rg -n "AzureOpenAI|BadRequestError|Exception.*max" --type=py -B2 -A2

Repository: HolmesGPT/holmesgpt

Length of output: 2707


🏁 Script executed:

# Check the pyproject.toml for type hints configuration (mypy)
cat pyproject.toml | grep -A10 "\[tool.mypy\]" || cat pyproject.toml | grep -A10 "mypy"

Repository: HolmesGPT/holmesgpt

Length of output: 254


🏁 Script executed:

# Search for how other similar wrapper/proxy classes handle exceptions
rg -n "class.*Proxy|def create\(" --type=py -B1 -A5 tests/

Repository: HolmesGPT/holmesgpt

Length of output: 1817


Add type hints to all proxy classes.

The proxy classes lack type annotations required by the project's coding guidelines. While the exception handling correctly targets openai.BadRequestError (which works for both OpenAI and Azure OpenAI via the unified library), and the string matching is appropriately defensive, the missing type hints must be added:

from typing import Any

class _CompletionsProxy:
    def __init__(self, completions: Any) -> None:
        self._completions = completions

    def create(self, *args: Any, **kwargs: Any) -> Any:
        # Some newer/reasoning models reject `max_tokens` and require `max_completion_tokens`.
        # Be conservative: try as-is first; if the server rejects, retry with renamed param.
        try:
            return self._completions.create(*args, **kwargs)
        except openai.BadRequestError as e:
            msg = str(e)
            if (
                "max_tokens" in msg
                and "max_completion_tokens" in msg
                and "max_tokens" in kwargs
                and "max_completion_tokens" not in kwargs
            ):
                retry_kwargs = dict(kwargs)
                retry_kwargs["max_completion_tokens"] = retry_kwargs.pop("max_tokens")
                return self._completions.create(*args, **retry_kwargs)
            raise

    def __getattr__(self, name: str) -> Any:
        return getattr(self._completions, name)

Apply the same pattern to _ChatProxy and _MaxTokensCompatClient.

🤖 Prompt for AI Agents
In @tests/llm/utils/classifiers.py around lines 23-46, The proxy classes lack
type annotations; import typing.Any and add type hints: annotate
_CompletionsProxy.__init__(self, completions: Any) -> None,
_CompletionsProxy.create(self, *args: Any, **kwargs: Any) -> Any, and
_CompletionsProxy.__getattr__(self, name: str) -> Any, and apply the same
pattern to the _ChatProxy and _MaxTokensCompatClient classes so all proxy
constructors, create/handler methods, varargs, kwargs and __getattr__ signatures
use Any and return Any (or None for __init__).

Comment on lines +49 to +56
class _ChatProxy:
def __init__(self, chat):
self._chat = chat
# Wrap only chat.completions.create, leave everything else untouched.
self.completions = _CompletionsProxy(chat.completions)

def __getattr__(self, name):
return getattr(self._chat, name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add type hints to maintain consistency.

The delegation pattern is correct, but type annotations are missing per coding guidelines.

🔎 Proposed type hints
+from typing import Any
+
 class _ChatProxy:
-    def __init__(self, chat):
+    def __init__(self, chat: Any) -> None:
         self._chat = chat
         # Wrap only chat.completions.create, leave everything else untouched.
         self.completions = _CompletionsProxy(chat.completions)
 
-    def __getattr__(self, name):
+    def __getattr__(self, name: str) -> Any:
         return getattr(self._chat, name)

Based on coding guidelines requiring type hints.

📝 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.

Suggested change
class _ChatProxy:
def __init__(self, chat):
self._chat = chat
# Wrap only chat.completions.create, leave everything else untouched.
self.completions = _CompletionsProxy(chat.completions)
def __getattr__(self, name):
return getattr(self._chat, name)
from typing import Any
class _ChatProxy:
def __init__(self, chat: Any) -> None:
self._chat = chat
# Wrap only chat.completions.create, leave everything else untouched.
self.completions = _CompletionsProxy(chat.completions)
def __getattr__(self, name: str) -> Any:
return getattr(self._chat, name)
🤖 Prompt for AI Agents
In @tests/llm/utils/classifiers.py around lines 49-56, Add type hints to class
_ChatProxy: annotate the __init__ parameter chat (e.g., chat: Any or a more
specific Chat type), set self._chat: Any, and type self.completions as
_CompletionsProxy; annotate __getattr__ as def __getattr__(self, name: str) ->
Any. Also import typing.Any (or the concrete Chat type) at the top. This keeps
the delegation pattern but makes type annotations explicit for _ChatProxy,
__init__, self.completions, and __getattr__.

Comment on lines +59 to +66
class _MaxTokensCompatClient:
def __init__(self, client):
self._client = client
self.chat = _ChatProxy(client.chat)

def __getattr__(self, name):
return getattr(self._client, name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add type hints to maintain consistency.

Same as the other proxy classes—type annotations are required per coding guidelines.

🔎 Proposed type hints
+from typing import Any
+
 class _MaxTokensCompatClient:
-    def __init__(self, client):
+    def __init__(self, client: Any) -> None:
         self._client = client
         self.chat = _ChatProxy(client.chat)
 
-    def __getattr__(self, name):
+    def __getattr__(self, name: str) -> Any:
         return getattr(self._client, name)

Based on coding guidelines requiring type hints.

📝 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.

Suggested change
class _MaxTokensCompatClient:
def __init__(self, client):
self._client = client
self.chat = _ChatProxy(client.chat)
def __getattr__(self, name):
return getattr(self._client, name)
from typing import Any
class _MaxTokensCompatClient:
def __init__(self, client: Any) -> None:
self._client = client
self.chat = _ChatProxy(client.chat)
def __getattr__(self, name: str) -> Any:
return getattr(self._client, name)
🤖 Prompt for AI Agents
In @tests/llm/utils/classifiers.py around lines 59-66, Add missing type hints to
the _MaxTokensCompatClient proxy: annotate the class constructor parameter
(client) with the appropriate client type or typing.Any, type the self._client
attribute, annotate the chat attribute as _ChatProxy, and add a return type of
Any (or the client type) to __getattr__ as def __getattr__(self, name: str) ->
Any; ensure necessary typing imports are added.

Comment on lines +18 to +20
enabled: false
robusta:
enabled: true
connectivity_check:
enabled: true
enabled: false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the structure of tests/llm/utils/default_toolsets.yaml and test fixtures

echo "=== Content of default_toolsets.yaml ==="
cat -n tests/llm/utils/default_toolsets.yaml

echo -e "\n=== Test fixture structure ==="
fd -t d . tests/llm/fixtures/ -maxdepth 1 | head -20

echo -e "\n=== Test cases with their own toolsets.yaml ==="
fd -type f toolsets.yaml tests/llm/fixtures/

echo -e "\n=== Count of test directories ==="
find tests/llm/fixtures -maxdepth 1 -type d -mindepth 1 | wc -l

Repository: HolmesGPT/holmesgpt

Length of output: 1234


🏁 Script executed:

#!/bin/bash
# Corrected search for test dependencies

echo "=== Test directories in fixtures ==="
find tests/llm/fixtures -maxdepth 1 -type d | sort

echo -e "\n=== Search for toolsets.yaml files anywhere ==="
find tests/llm -name "toolsets.yaml" -type f

echo -e "\n=== Test case files ==="
find tests/llm/fixtures -name "test_case.yaml" -type f | sort

echo -e "\n=== Search for runbook or robusta in test files ==="
rg -n 'runbook|robusta' tests/llm/fixtures --type yaml -i

echo -e "\n=== Check for toolsets field in test_case.yaml files ==="
rg -n 'toolsets:' tests/llm/fixtures --type yaml

Repository: HolmesGPT/holmesgpt

Length of output: 47802


🏁 Script executed:

#!/bin/bash

echo "=== Test 55_kafka_runbook (name suggests runbook needed) ==="
head -40 tests/llm/fixtures/test_ask_holmes/55_kafka_runbook/toolsets.yaml

echo -e "\n=== Test 176_network_policy_blocking_traffic_no_runbooks (name says no_runbooks) ==="
head -10 tests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_runbooks/toolsets.yaml

echo -e "\n=== Test 10_KubeDeploymentReplicasMismatch (has robusta in toolsets) ==="
head -20 tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/toolsets.yaml

echo -e "\n=== Test 111_tool_hallucination (has both robusta and runbook) ==="
head -50 tests/llm/fixtures/test_ask_holmes/111_tool_hallucination/toolsets.yaml

echo -e "\n=== Sample test WITHOUT custom toolsets.yaml ==="
find tests/llm/fixtures -name "test_case.yaml" -type f | while read f; do
  dir=$(dirname "$f")
  if [ ! -f "$dir/toolsets.yaml" ]; then
    echo "Found: $dir"
    break
  fi
done

Repository: HolmesGPT/holmesgpt

Length of output: 2124


🏁 Script executed:

#!/bin/bash

echo "=== Test directories WITHOUT custom toolsets.yaml ==="
find tests/llm/fixtures -name "test_case.yaml" -type f | while read f; do
  dir=$(dirname "$f")
  if [ ! -f "$dir/toolsets.yaml" ]; then
    echo "$dir"
  fi
done

echo -e "\n=== Compaction test (no custom toolsets) content ==="
cat tests/llm/fixtures/compaction/001_compaction/test_case.yaml

echo -e "\n=== Check if compaction tests need runbook or robusta ==="
find tests/llm/fixtures/compaction -name "test_case.yaml" -type f | xargs grep -l "runbook\|robusta" 2>/dev/null || echo "No references found"

echo -e "\n=== Check tags in compaction tests ==="
find tests/llm/fixtures/compaction -name "test_case.yaml" -type f -exec grep -H "tags:" {} \;

Repository: HolmesGPT/holmesgpt

Length of output: 7554


🏁 Script executed:

#!/bin/bash

echo "=== Runbook tests WITHOUT custom toolsets.yaml ==="
for test in 90_runbook_basic_selection 94_runbook_transparency 95_runbook_memory_leak_detection 96_no_matching_runbook 162_get_runbooks 165_alert_with_multiple_runbooks; do
  if [ -f "tests/llm/fixtures/test_ask_holmes/$test/test_case.yaml" ]; then
    echo "=== $test ==="
    if [ -f "tests/llm/fixtures/test_ask_holmes/$test/toolsets.yaml" ]; then
      echo "HAS custom toolsets.yaml"
    else
      echo "NO custom toolsets.yaml - relies on defaults"
      head -20 "tests/llm/fixtures/test_ask_holmes/$test/test_case.yaml"
    fi
  fi
done

Repository: HolmesGPT/holmesgpt

Length of output: 4931


🏁 Script executed:

#!/bin/bash

echo "=== All tests with 'runbooks' tag but no custom toolsets.yaml ==="
find tests/llm/fixtures -name "test_case.yaml" -type f | while read f; do
  dir=$(dirname "$f")
  if [ ! -f "$dir/toolsets.yaml" ] && grep -q "- runbooks" "$f"; then
    echo "$(basename $dir)"
  fi
done | wc -l

echo -e "\n=== Count of tests relying on default toolsets ==="
find tests/llm/fixtures -name "test_case.yaml" -type f | while read f; do
  dir=$(dirname "$f")
  if [ ! -f "$dir/toolsets.yaml" ]; then
    echo "1"
  fi
done | wc -l

Repository: HolmesGPT/holmesgpt

Length of output: 11303


🏁 Script executed:

#!/bin/bash

echo "=== Verify runbook tests explicitly need runbook enabled ==="
for test in 90_runbook_basic_selection 94_runbook_transparency 96_no_matching_runbook 162_get_runbooks 165_alert_with_multiple_runbooks; do
  testdir="tests/llm/fixtures/test_ask_holmes/$test"
  if [ -d "$testdir" ]; then
    echo "=== $test ==="
    # Check if test_case.yaml has runbook references
    rg 'runbook' "$testdir/test_case.yaml" --max-count=2
    # Check if it has custom toolsets.yaml
    if [ -f "$testdir/toolsets.yaml" ]; then
      echo "  -> HAS custom toolsets.yaml"
    else
      echo "  -> NO custom toolsets.yaml (relies on defaults)"
    fi
  fi
done

Repository: HolmesGPT/holmesgpt

Length of output: 1093


Runbook and robusta toolsets are disabled by default, breaking multiple tests that depend on them.

Disabling the runbook and robusta toolsets in default_toolsets.yaml will break at least 5 test cases that have no custom toolsets.yaml and rely on defaults:

  • test_ask_holmes/90_runbook_basic_selection - expects "Finds and uses the Application Gateway troubleshooting runbook"
  • test_ask_holmes/94_runbook_transparency - expects "Uses database troubleshooting runbook"
  • test_ask_holmes/96_no_matching_runbook - tests runbook selection behavior
  • test_ask_holmes/162_get_runbooks - simulates loading runbooks from cluster
  • test_ask_holmes/165_alert_with_multiple_runbooks - tests finding relevant runbook for alerts

Additionally, 100+ tests rely on default_toolsets.yaml. Tests that need these toolsets must explicitly enable them via custom toolsets.yaml files, or the defaults must keep them enabled. Add custom toolsets.yaml files for affected tests or reconsider disabling these in defaults.

@xiaoqi-7 xiaoqi-7 closed this Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants