Conversation
2c3e50a to
5a2bdf0
Compare
mainred
left a comment
mainred
left a comment
There was a problem hiding this comment.
Great improvement, now the code is less aggressive to the default code flow.
| ) | ||
|
|
||
| # TodoWrite system - only enable for original Holmes behavior | ||
| if not kaito_enabled: |
There was a problem hiding this comment.
- Why we can not use todo write in kaito?
- You removed runbooks_ctx even when kaito is not enabled?
There was a problem hiding this comment.
- todo is a disabled toolset in the integration anyways (from the minimal config), but the toolset leaves KAITO models susceptible to hallucinations
- didn't remove anything - my branch was based on earlier master before those generate_runbooks_args() functions were added. I kept existing runbooks support but didn't include the newer additions as my local code doesnt reflect
|
|
||
| if kaito_enabled: | ||
| # KAITO behavior: always use "required" for better model performance | ||
| tool_choice = "required" if tools else None |
There was a problem hiding this comment.
Why tool_choice is required? Kaito cannot end the iterations with a non-tool?
There was a problem hiding this comment.
KAITO won't start calling tools in auto mode. This was intial finding for POC
Intelligent termination features were added in integration to workaround this
| tokens = self.llm.count_tokens(messages=messages, tools=tools) | ||
|
|
||
| # 🔍 Debug: Log every tool result for termination analysis | ||
| logging.info(f"🔍 TERMINATION DEBUG: Tool {tool_call_result.tool_name} status={tool_call_result.result.status}") |
There was a problem hiding this comment.
If these code are specific to kaito, let's move them to a function to make the main default logic clean.
b1c3d02 to
d4dc787
Compare
WalkthroughAdds a HOLMES_KAITO_ENABLED feature flag (default true) that is exposed to prompt rendering, changes system_prompt_additions handling, removes runbook generation, gates KAITO-specific prompt content, disables TodoWrite when KAITO is enabled, and introduces KAITO-aware tool-choice and LLM-driven termination logic in the tool-calling loop. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as ToolCallingLLM
participant Prompt as PromptRenderer
participant LLM as LLM
participant Tool as Tool/Toolset
rect `#f0f4ff`
Note over Caller,Prompt: Render system prompts with kaito_enabled in context
Caller->>Prompt: render(system_prompt, context{kaito_enabled, system_prompt_additions})
Prompt-->>Caller: initial messages
end
Caller->>LLM: plan / reason (get_tool_choice)
alt tools available
Caller->>Tool: invoke tool (choice = get_tool_choice(tools))
Tool-->>Caller: tool_call_result
Caller->>LLM: append tool result to messages
Caller->>LLM: _should_stop_investigation(messages)?
alt LLM -> STOP
LLM-->>Caller: STOP decision
Note right of Caller: _handle_kaito_termination_logic prunes messages\nand may force final iteration
else LLM -> CONTINUE
LLM-->>Caller: CONTINUE decision
end
else no tools
Caller->>LLM: continue reasoning without tool
end
Caller->>LLM: produce final answer
LLM-->>Caller: final content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
nthevenin
left a comment
nthevenin
left a comment
There was a problem hiding this comment.
"Fixed in latest commit - extracted debug code into [_handle_kaito_termination_logic()] helper method to keep main loop clean."
| ) | ||
|
|
||
| # TodoWrite system - only enable for original Holmes behavior | ||
| if not kaito_enabled: |
There was a problem hiding this comment.
- todo is a disabled toolset in the integration anyways (from the minimal config), but the toolset leaves KAITO models susceptible to hallucinations
- didn't remove anything - my branch was based on earlier master before those generate_runbooks_args() functions were added. I kept existing runbooks support but didn't include the newer additions as my local code doesnt reflect
|
|
||
| if kaito_enabled: | ||
| # KAITO behavior: always use "required" for better model performance | ||
| tool_choice = "required" if tools else None |
There was a problem hiding this comment.
KAITO won't start calling tools in auto mode. This was intial finding for POC
Intelligent termination features were added in integration to workaround this
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
holmes/core/prompt.py (1)
59-66: Consider extracting KAITO flag reading to a shared utility.The pattern
os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true"is duplicated across multiple files (prompt.py,tool_calling_llm.py,core_investigation.py). Consider extracting this to a shared utility function inholmes/common/env_vars.pyfor consistency and maintainability.# In holmes/common/env_vars.py KAITO_ENABLED = os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true"holmes/plugins/prompts/_kaito_accuracy.jinja2 (1)
60-87: Fragile template section extraction.The termination decision section is extracted in
tool_calling_llm.pyusing string splitting on"# Termination Decision Prompt\n\n". This approach is fragile and could break if the heading format changes (extra spaces, different newlines, etc.).Consider either:
- Moving the termination prompt to a separate template file (
_kaito_termination.jinja2)- Using a more robust delimiter pattern (e.g.,
<!-- TERMINATION_START -->)holmes/core/tool_calling_llm.py (2)
205-266: Excessive debug logging for production code.This method contains 12+
logging.infocalls with emoji prefixes (🔍,🎯,🧹) that will be emitted on every tool call when KAITO is enabled. Consider:
- Downgrading most of these to
logging.debuglevel- Using a single consolidated log at the end summarizing the decision
🔎 Proposed fix - convert to debug level
- logging.info(f"🔍 TERMINATION DEBUG: Tool {tool_call_result.tool_name} status={tool_call_result.result.status}") + logging.debug(f"KAITO termination check: Tool {tool_call_result.tool_name} status={tool_call_result.result.status}") if tool_call_result.result.status == StructuredToolResultStatus.ERROR: - logging.info(f"🔍 TERMINATION DEBUG: Error message: {tool_call_result.result.error}") + logging.debug(f"KAITO termination: Error message: {tool_call_result.result.error}")Similarly for other
logging.infocalls in this method.
566-572: Move KAITO flag check outside the loop.The KAITO flag is read from environment variables inside the tool execution loop (line 570). Since environment variables don't change during execution, read this once before the loop starts.
🔎 Proposed fix
def call( # type: ignore ... + kaito_enabled = os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true" while i < max_steps: ... for future in concurrent.futures.as_completed(futures): tool_call_result: ToolCallResult = future.result() tool_calls.append(tool_call_result.as_tool_result_response()) messages.append(tool_call_result.as_tool_call_message()) # Handle KAITO-specific debug logging and termination logic - kaito_enabled = os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true" if kaito_enabled: i = self._handle_kaito_termination_logic(tool_call_result, messages, tool_calls, i, max_steps)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between bcfc05b and d4dc787103babb5a41f460ee6d3235fce31f8d71.
📒 Files selected for processing (5)
holmes/core/prompt.pyholmes/core/tool_calling_llm.pyholmes/plugins/prompts/_general_instructions.jinja2holmes/plugins/prompts/_kaito_accuracy.jinja2holmes/plugins/toolsets/investigator/core_investigation.py
🧰 Additional context used
📓 Path-based instructions (3)
holmes/plugins/prompts/**/*.jinja2
📄 CodeRabbit inference engine (CLAUDE.md)
Prompt files should use the format holmes/plugins/prompts/{name}.jinja2
Files:
holmes/plugins/prompts/_kaito_accuracy.jinja2holmes/plugins/prompts/_general_instructions.jinja2
**/*.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 code
Type hints are required for all Python code
Files:
holmes/core/tool_calling_llm.pyholmes/plugins/toolsets/investigator/core_investigation.pyholmes/core/prompt.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**: Toolset files should use the format holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/ directory structure
All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including the exact query/command executed, time ranges, parameters, filters, and full API error responses
All tools have read-only access by design
Files:
holmes/plugins/toolsets/investigator/core_investigation.py
🧠 Learnings (1)
📚 Learning: 2025-12-25T11:30:26.525Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.525Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompt files should use the format holmes/plugins/prompts/{name}.jinja2
Applied to files:
holmes/plugins/prompts/_kaito_accuracy.jinja2holmes/core/prompt.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/investigator/core_investigation.py (1)
holmes/core/tools.py (1)
ToolsetTag(143-146)
🪛 Ruff (0.14.10)
holmes/core/tool_calling_llm.py
1091-1091: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1108-1108: f-string without any placeholders
Remove extraneous f prefix
(F541)
1110-1110: Consider moving this statement to an else block
(TRY300)
1112-1112: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (4)
holmes/plugins/prompts/_general_instructions.jinja2 (1)
44-47: LGTM!The conditional include for KAITO accuracy instructions is correctly implemented with proper Jinja2 whitespace control. The placement between
_permission_errors.jinja2and "Special cases" section is appropriate.holmes/core/prompt.py (1)
77-80: LGTM!The conditional TodoWrite reminder logic is correctly implemented with a clear comment explaining the rationale. This aligns with the
CoreInvestigationToolsetchanges that also disable TodoWrite when KAITO is enabled.holmes/plugins/toolsets/investigator/core_investigation.py (1)
136-156: LGTM!The KAITO-based toolset configuration is well-implemented with clear logic branches and informative logging. The approach of setting both
enabledandis_defaulttoFalsewhen KAITO is enabled ensures the TodoWrite tool is completely disabled.holmes/plugins/prompts/_kaito_accuracy.jinja2 (1)
1-52: Verify KAITO instructions with target LLM models.The KAITO accuracy instructions are comprehensive. Given the heavy reliance on "CRITICAL" directives and specific prohibitions, verify that these instructions produce the desired behavior across all supported LLM models (GPT-4, Claude, etc.), as different models may respond differently to instructional emphasis.
Signed-off-by: nthevenin <nickt5674@gmail.com>
Signed-off-by: nthevenin <nickt5674@gmail.com>
d4dc787 to
98c87ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holmes/core/tool_calling_llm.py (2)
410-416: Useget_tool_choiceinstead of hardcoded tool choice.The
get_tool_choicefunction is defined (lines 59-81) but not used here. Line 416 still uses the hardcoded assignmenttool_choice = "auto" if tools else None, which bypasses KAITO-aware tool selection.🔎 Proposed fix
- tool_choice = "auto" if tools else None + tool_choice = get_tool_choice(bool(tools))
907-912: Useget_tool_choiceincall_streammethod.Similar to the
callmethod, line 912 uses hardcodedtool_choice = "auto" if tools else Noneinstead of callingget_tool_choice, bypassing KAITO-aware tool selection in the streaming path.🔎 Proposed fix
- tool_choice = "auto" if tools else None + tool_choice = get_tool_choice(bool(tools))
♻️ Duplicate comments (1)
holmes/core/tool_calling_llm.py (1)
59-82: Move imports to the top of the file.The import statement on line 82 (and following lines through 91) appears after the
get_tool_choicefunction definition. Per coding guidelines, all Python imports must be placed at the top of the file.🔎 Proposed fix
Move lines 82-91 to the import section at the top (after line 6):
import os from typing import Dict, List, Optional, Type, Union, Callable, Any + +from holmes.utils.tags import format_tags_in_string, parse_messages_tags +from holmes.core.tools_utils.tool_executor import ToolExecutor +from holmes.core.tracing import DummySpan +from holmes.utils.colors import AI_COLOR +from holmes.utils.stream import ( + StreamEvents, + StreamMessage, + add_token_count_to_metadata, + build_stream_event_token_count, +)Then remove lines 82-91 from their current location.
Based on coding guidelines.
🧹 Nitpick comments (3)
holmes/plugins/prompts/_kaito_accuracy.jinja2 (1)
1-87: Consider consolidating repetitive instructions for clarity.The template contains multiple "CRITICAL" markers and overlapping prohibitions (e.g., lines 9-11 and 25-35 both address not outputting raw JSON). While the emphasis is clearly intentional, consolidating similar instructions might improve clarity and reduce cognitive load.
For example, the JSON prohibition appears in:
- Lines 9-11: "CRITICAL JSON PROHIBITION"
- Lines 25-35: "CRITICAL TOOL EXECUTION RESPONSE PROHIBITION" (includes JSON examples)
These could be merged into a single, comprehensive section.
holmes/core/tool_calling_llm.py (2)
204-265: Reduce logging verbosity and use appropriate log levels.The method contains 10+
logging.info()calls for debug information, many with emoji prefixes (🔍, 🎯, 🧹). This creates noise in production logs and makes the logic harder to follow.Consider:
- Using
logging.debug()for verbose diagnostic information- Reducing the number of log statements (consolidate related checks)
- Removing emoji prefixes or using them sparingly
🔎 Example refactor
- # 🔍 Debug: Log every tool result for termination analysis - logging.info(f"🔍 TERMINATION DEBUG: Tool {tool_call_result.tool_name} status={tool_call_result.result.status}") - if tool_call_result.result.status == StructuredToolResultStatus.ERROR: - logging.info(f"🔍 TERMINATION DEBUG: Error message: {tool_call_result.result.error}") + logging.debug(f"Tool {tool_call_result.tool_name} status={tool_call_result.result.status}, error={tool_call_result.result.error if tool_call_result.result.status == StructuredToolResultStatus.ERROR else 'N/A'}")Apply similar consolidation to other debug blocks.
565-571: Consider caching KAITO enabled state.Lines 569 re-reads
HOLMES_KAITO_ENABLEDfrom the environment. Since this is also read inget_tool_choice(line 71), consider storing it as a class instance variable during initialization to avoid repeated environment lookups and ensure consistency.🔎 Example approach
In
__init__:def __init__(self, tool_executor: ToolExecutor, max_steps: int, llm: LLM, tracer=None): # ... existing code ... self.kaito_enabled = os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true"Then use
self.kaito_enabledinstead of reading the environment each time.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d4dc787103babb5a41f460ee6d3235fce31f8d71 and 1ae8e79.
📒 Files selected for processing (5)
holmes/core/prompt.pyholmes/core/tool_calling_llm.pyholmes/plugins/prompts/_general_instructions.jinja2holmes/plugins/prompts/_kaito_accuracy.jinja2holmes/plugins/toolsets/investigator/core_investigation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/core/prompt.py
🧰 Additional context used
📓 Path-based instructions (3)
holmes/plugins/prompts/**/*.jinja2
📄 CodeRabbit inference engine (CLAUDE.md)
Prompt files should use the format holmes/plugins/prompts/{name}.jinja2
Files:
holmes/plugins/prompts/_general_instructions.jinja2holmes/plugins/prompts/_kaito_accuracy.jinja2
**/*.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 code
Type hints are required for all Python code
Files:
holmes/core/tool_calling_llm.pyholmes/plugins/toolsets/investigator/core_investigation.py
holmes/plugins/toolsets/**
📄 CodeRabbit inference engine (CLAUDE.md)
holmes/plugins/toolsets/**: Toolset files should use the format holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/ directory structure
All toolsets MUST return detailed error messages from underlying APIs to enable LLM self-correction, including the exact query/command executed, time ranges, parameters, filters, and full API error responses
All tools have read-only access by design
Files:
holmes/plugins/toolsets/investigator/core_investigation.py
🧠 Learnings (1)
📚 Learning: 2025-12-25T11:30:26.525Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.525Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompt files should use the format holmes/plugins/prompts/{name}.jinja2
Applied to files:
holmes/plugins/prompts/_kaito_accuracy.jinja2
🧬 Code graph analysis (2)
holmes/core/tool_calling_llm.py (3)
holmes/core/tools.py (1)
StructuredToolResultStatus(52-76)holmes/core/models.py (1)
as_tool_result_response(42-52)holmes/plugins/prompts/__init__.py (1)
load_and_render_prompt(27-54)
holmes/plugins/toolsets/investigator/core_investigation.py (1)
holmes/core/tools.py (1)
ToolsetTag(145-148)
🪛 Ruff (0.14.10)
holmes/core/tool_calling_llm.py
1131-1131: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1148-1148: f-string without any placeholders
Remove extraneous f prefix
(F541)
1150-1150: Consider moving this statement to an else block
(TRY300)
1152-1152: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
holmes/plugins/toolsets/investigator/core_investigation.py (1)
137-157: LGTM! Clean KAITO toggle implementation.The environment-driven feature toggle is well-implemented. The logic clearly separates KAITO mode (TodoWrite disabled, not default) from original Holmes behavior (TodoWrite enabled, default), with informative logging.
| def _should_stop_investigation(self, messages: list) -> bool: | ||
| """ | ||
| Ask the LLM to determine if investigation should stop based on full conversation context. | ||
| Evaluates whether the problem has been solved or if repetitive tool calls are unhelpful. | ||
| """ | ||
| try: | ||
| logging.info("🔍 TERMINATION DEBUG: Asking LLM if investigation should stop...") | ||
|
|
||
| # Load termination prompt from template | ||
| full_kaito_content = load_and_render_prompt("builtin://_kaito_accuracy.jinja2", {}) | ||
|
|
||
| # Extract just the termination decision section | ||
| if "# Termination Decision Prompt" in full_kaito_content: | ||
| termination_prompt = full_kaito_content.split("# Termination Decision Prompt\n\n", 1)[1] | ||
| else: | ||
| logging.warning("🔍 TERMINATION DEBUG: Template section not found, cannot perform intelligent termination") | ||
| return False # Continue if template is missing | ||
|
|
||
| # Create a fresh conversation for termination check | ||
| termination_messages = [ | ||
| {"role": "system", "content": "You are a helpful assistant that evaluates whether investigations should continue."}, | ||
| {"role": "user", "content": f"Based on this conversation history:\n\n{str(messages)}\n\n{termination_prompt}"} | ||
| ] | ||
|
|
||
| response = self.llm.completion( | ||
| messages=termination_messages, | ||
| tools=None, | ||
| tool_choice=None, | ||
| temperature=0.1, | ||
| drop_params=True, | ||
| ) | ||
|
|
||
| response_text = response.choices[0].message.content.upper() | ||
| should_stop = "STOP" in response_text | ||
|
|
||
| logging.info(f"🔍 TERMINATION DEBUG: LLM response: '{response_text}' → should_stop={should_stop}") | ||
|
|
||
| if should_stop: | ||
| logging.info(f"🎯 Intelligent termination: Investigation should stop based on conversation context") | ||
|
|
||
| return should_stop | ||
|
|
||
| except Exception as e: | ||
| logging.warning(f"Failed to check for intelligent termination: {e}") | ||
| return False # Continue normally if check fails |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check the structure
fd "tool_calling_llm.py" -t fRepository: HolmesGPT/holmesgpt
Length of output: 94
🏁 Script executed:
# Check the file size to determine how to read it
wc -l holmes/core/tool_calling_llm.pyRepository: HolmesGPT/holmesgpt
Length of output: 99
🏁 Script executed:
# Read the specific lines mentioned (1110-1154)
sed -n '1110,1154p' holmes/core/tool_calling_llm.py | cat -nRepository: HolmesGPT/holmesgpt
Length of output: 2641
🏁 Script executed:
# Check for Ruff configuration and run Ruff on this file to identify actual issues
cd holmes/core && rg "^" tool_calling_llm.py -lRepository: HolmesGPT/holmesgpt
Length of output: 82
🏁 Script executed:
# Run Ruff specifically on the file to catch any linting issues
python -m pip install ruff -q && ruff check holmes/core/tool_calling_llm.py --select RUF010,F541,TRY300,BLE001 2>/dev/null || trueRepository: HolmesGPT/holmesgpt
Length of output: 149
🏁 Script executed:
# Also check the full context around the method to understand imports and structure
sed -n '1,50p' holmes/core/tool_calling_llm.py | cat -nRepository: HolmesGPT/holmesgpt
Length of output: 1888
🏁 Script executed:
# Try installing and running ruff with a timeout and specific checks
timeout 30 python -c "
import subprocess
result = subprocess.run(['python', '-m', 'pip', 'install', 'ruff', '-q'], timeout=20)
" && timeout 30 python -m ruff check holmes/core/tool_calling_llm.py --line-length 120 2>&1 | head -100Repository: HolmesGPT/holmesgpt
Length of output: 110
🏁 Script executed:
# Let's manually check specific lines for the issues flagged
# Line 39 (1148): Check if f-string has no placeholders
sed -n '1148p' holmes/core/tool_calling_llm.pyRepository: HolmesGPT/holmesgpt
Length of output: 179
🏁 Script executed:
# Line 22 (1131): Check str(messages) usage
sed -n '1131p' holmes/core/tool_calling_llm.pyRepository: HolmesGPT/holmesgpt
Length of output: 189
🏁 Script executed:
# Check the try-except structure around line 1150
sed -n '1145,1155p' holmes/core/tool_calling_llm.pyRepository: HolmesGPT/holmesgpt
Length of output: 562
🏁 Script executed:
# Verify context about the method being called and its performance impact
# Search for calls to _should_stop_investigation
rg "_should_stop_investigation" holmes/core/tool_calling_llm.py -B 2 -A 2Repository: HolmesGPT/holmesgpt
Length of output: 1073
Address static analysis issues and performance implications.
Multiple issues identified:
-
Performance concern: This method makes an additional LLM call for each termination check. It's called twice per investigation (after safeguard conditions and during general termination checks), adding significant latency and cost. Consider documenting this trade-off or implementing a heuristic check before invoking the LLM.
-
Static analysis issues:
- Line 1131:
str(messages)in f-string should use format specifier{messages!s}(RUF010) - Line 1148: Remove extraneous
fprefix from f-string without placeholders (F541) - Line 1150: Return statement should be in
elseblock, not intry(TRY300) - Line 1152: Avoid catching bare
Exception- catch specific exception types instead (BLE001)
- Line 1131:
🧰 Tools
🪛 Ruff (0.14.10)
1131-1131: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1148-1148: f-string without any placeholders
Remove extraneous f prefix
(F541)
1150-1150: Consider moving this statement to an else block
(TRY300)
1152-1152: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
holmes/core/tool_calling_llm.py around lines 1110-1154: the termination routine
currently always makes an expensive LLM call and has a few static-analysis
issues; fix by adding a cheap heuristic guard (e.g., skip LLM and return False
if messages length exceeds a threshold or if recent tool-call repetition count
is high) to avoid unnecessary LLM invocations, replace str(messages) in the user
prompt with the proper format specifier {messages!s}, remove the extraneous f
prefix on the "🎯 Intelligent termination" logging call, restructure the
try/except so the try only contains the code that may raise and put the final
return logic in an else block (e.g., try: call LLM; except <specific-exceptions>
as e: log and return False; else: process response and return should_stop), and
avoid catching bare Exception by catching specific exception classes (IOError,
ValueError, RuntimeError or the LLM client's specific exception class) or adding
the LLM client's error class if available.
… issues
- Use get_tool_choice() function in call() and call_stream() methods instead of hardcoded tool_choice
- Move misplaced imports to top of file after function definitions
- Fix static analysis issues in _should_stop_investigation():
* Use {messagessource /Users/nickthevenin/holmes-aikit-eval/.venv/bin/activate} format specifier instead of str(messages)
* Remove extraneous f-prefix from logging statement
* Restructure try/except with return logic in else block
* Catch specific exceptions instead of bare Exception
Fixes CodeRabbit review comments while preserving intelligent termination logic.
Signed-off-by: nthevenin <nickt5674@gmail.com>
…esgpt into kaito-toggle-only
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
holmes/core/tool_calling_llm.py (1)
566-572: Consider caching the KAITO enabled check.The
HOLMES_KAITO_ENABLEDenvironment variable is read multiple times during execution:
- Once per iteration in
get_tool_choice(line 81)- Once per tool call here (line 570)
For iterations with multiple tool calls, this results in redundant environment variable reads.
💡 Optimization suggestion
Consider caching the
kaito_enabledvalue as an instance variable during__init__or at the start of thecall/call_streammethods:# In __init__ or at start of call/call_stream kaito_enabled = os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true" # Then reference the cached value if kaito_enabled: i = self._handle_kaito_termination_logic(...)This would also make the logic easier to test.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/tool_calling_llm.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/tool_calling_llm.py
🧠 Learnings (1)
📚 Learning: 2025-12-29T08:35:37.678Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.678Z
Learning: Complex investigations should have LLM evaluation tests
Applied to files:
holmes/core/tool_calling_llm.py
🔇 Additional comments (3)
holmes/core/tool_calling_llm.py (3)
5-17: LGTM! Imports properly organized.The imports are now correctly placed at the top of the file per coding guidelines. The addition of
osfor environment variable access and the reorganization of other imports addresses the previous review feedback.
69-92: LGTM! Function now properly integrated.The
get_tool_choicefunction correctly implements KAITO-aware tool choice logic and is now called at lines 417 and 913, addressing the previous review feedback about the function being unused.
417-417: LGTM! Centralized tool choice logic properly integrated.The call to
get_tool_choice(bool(tools))correctly replaces the previous hardcoded logic, and the same pattern is consistently applied incall_streamat line 913. This addresses the previous review feedback.
| def _should_stop_investigation(self, messages: list) -> bool: | ||
| """ | ||
| Ask the LLM to determine if investigation should stop based on full conversation context. | ||
| Evaluates whether the problem has been solved or if repetitive tool calls are unhelpful. | ||
| """ | ||
| try: | ||
| logging.info("🔍 TERMINATION DEBUG: Asking LLM if investigation should stop...") | ||
|
|
||
| # Load termination prompt from template | ||
| full_kaito_content = load_and_render_prompt("builtin://_kaito_accuracy.jinja2", {}) | ||
|
|
||
| # Extract just the termination decision section | ||
| if "# Termination Decision Prompt" in full_kaito_content: | ||
| termination_prompt = full_kaito_content.split("# Termination Decision Prompt\n\n", 1)[1] | ||
| else: | ||
| logging.warning("🔍 TERMINATION DEBUG: Template section not found, cannot perform intelligent termination") | ||
| return False # Continue if template is missing | ||
|
|
||
| # Create a fresh conversation for termination check | ||
| termination_messages = [ | ||
| {"role": "system", "content": "You are a helpful assistant that evaluates whether investigations should continue."}, | ||
| {"role": "user", "content": f"Based on this conversation history:\n\n{messages!s}\n\n{termination_prompt}"} | ||
| ] | ||
|
|
||
| response = self.llm.completion( | ||
| messages=termination_messages, | ||
| tools=None, | ||
| tool_choice=None, | ||
| temperature=0.1, | ||
| drop_params=True, | ||
| ) | ||
| except (ValueError, RuntimeError, IOError, BadRequestError) as e: | ||
| logging.warning(f"Failed to check for intelligent termination: {e}") | ||
| return False # Continue normally if check fails | ||
| else: | ||
| response_text = response.choices[0].message.content.upper() | ||
| should_stop = "STOP" in response_text | ||
|
|
||
| logging.info(f"🔍 TERMINATION DEBUG: LLM response: '{response_text}' → should_stop={should_stop}") | ||
|
|
||
| if should_stop: | ||
| logging.info("🎯 Intelligent termination: Investigation should stop based on conversation context") | ||
|
|
||
| return should_stop |
There was a problem hiding this comment.
Acknowledge fixes, but performance concerns remain.
The previous static analysis issues have been properly addressed:
- Line 1132 now uses
{messages!s}format specifier ✓ - Line 1142 catches specific exception types ✓
- Lines 1145-1154 use proper try-except-else structure ✓
However, performance implications remain a concern:
-
Additional LLM call overhead: This method makes an extra LLM call for each termination check, adding latency and cost to every investigation.
-
Called multiple times: The method can be invoked twice per iteration (lines 228 and 255 in
_handle_kaito_termination_logic), multiplying the cost. -
Large context serialization: Line 1132 converts the entire message history to a string, which grows with investigation length and could become very large.
💡 Performance optimization suggestions
Consider adding heuristic guards before making the LLM call:
# Skip expensive LLM check if we haven't done enough iterations yet
if len(messages) < MIN_MESSAGES_FOR_TERMINATION:
return False
# Or check for recent repetitive tool calls first
recent_tools = [m.get("tool_calls") for m in messages[-5:] if m.get("role") == "assistant"]
if not has_repetitive_pattern(recent_tools):
return False
# Only then make the LLM call...Additionally:
- Cache the result for a few iterations to avoid repeated calls
- Consider including only recent messages (e.g., last 10) instead of full history
- Add metrics/logging to track how often termination checks trigger actual stops
🤖 Prompt for AI Agents
In holmes/core/tool_calling_llm.py around lines 1111 to 1154, the termination
check makes an expensive LLM call on every invocation (and can be called twice
per iteration) and serializes the entire message history which causes latency,
cost, and memory growth; add lightweight heuristic guards before calling the LLM
(e.g., return False if len(messages) < MIN_MESSAGES_FOR_TERMINATION or if recent
assistant tool_calls do not show a repetitive pattern), limit the context sent
to the LLM to the last N messages or a summarized subset instead of serializing
full history, memoize/cache the termination decision for a few iterations to
avoid duplicate calls when this method is invoked multiple times per loop (or
move the check to the caller so it runs once), and add metrics/logging to record
when the LLM check is skipped vs executed and how often it returns STOP so we
can tune thresholds.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/prompt.py (1)
1-7: Remove duplicate imports.The import section contains duplicates:
typingis imported on both lines 3 and 5rich.console.Consoleis imported on both lines 2 and 7This violates the coding guideline requiring imports at the top of the file and creates unnecessary redundancy.
🔎 Proposed fix
-import os -from rich.console import Console -from typing import Optional, List, Dict, Any, Union - from pathlib import Path from typing import Any, Dict, List, Optional, Union +import os from rich.console import Console
🧹 Nitpick comments (3)
holmes/core/tool_calling_llm.py (3)
78-100: Consider caching the KAITO environment variable.The function correctly implements KAITO-aware tool choice logic and addresses the past review comment about unused
get_tool_choice. However, the environment variable is read on every invocation, which could be optimized.💡 Optional optimization
Cache the environment variable at module level to avoid repeated
os.environ.getcalls:# At module level after imports _KAITO_ENABLED = os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true" def get_tool_choice(tools: bool) -> Optional[str]: """ Get the tool choice setting with KAITO toggle support. Args: tools: Whether tools are available in this iteration Returns: Tool choice string if tools available, None otherwise """ if _KAITO_ENABLED: # KAITO behavior: always use "required" for better model performance tool_choice = "required" if tools else None else: # Original Holmes behavior: simple auto tool_choice = "auto" if tools else None logging.debug(f"KAITO enabled: {_KAITO_ENABLED}, using tool_choice: {tool_choice}") return tool_choice
549-555: Consolidate KAITO flag checks.The KAITO environment variable is read again here (line 553), but it's already read in
get_tool_choice(line 90). Consider using a module-level cached value to avoid repeated environment variable lookups and ensure consistency across the codebase.💡 Suggested improvement
Define a module-level constant:
# At module level after imports _KAITO_ENABLED = os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true"Then use
_KAITO_ENABLEDdirectly at line 553:- kaito_enabled = os.environ.get("HOLMES_KAITO_ENABLED", "true").lower() == "true" - if kaito_enabled: + if _KAITO_ENABLED:
1046-1089: Acknowledge fixes, but performance concerns remain unaddressed.The static analysis issues from past reviews have been properly fixed:
- Line 1067 correctly uses
{messages!s}format specifier ✓- Line 1077 catches specific exception types ✓
- Lines 1080-1089 use proper try-except-else structure ✓
However, performance implications remain a concern as noted in past reviews:
Additional LLM call overhead: This method makes an extra LLM call for each termination check, adding latency and cost to every investigation.
Multiple invocations: The method is called up to twice per iteration (lines 238 and 265 in
_handle_kaito_termination_logic), multiplying the cost.Large context serialization: Line 1067 converts the entire message history to a string, which grows with investigation length.
💡 Performance optimization suggestions from past review
Add heuristic guards before making the LLM call:
# Skip expensive LLM check if we haven't done enough iterations yet if len(messages) < 5: # MIN_MESSAGES_FOR_TERMINATION return False # Check for recent repetitive tool calls first recent_tools = [m.get("tool_calls") for m in messages[-5:] if m.get("role") == "assistant"] if not has_repetitive_pattern(recent_tools): return False # Only then make the LLM call...Additional suggestions:
- Cache the result for a few iterations to avoid repeated calls
- Include only recent messages (e.g., last 10) instead of full history in line 1067
- Add metrics to track how often termination checks trigger actual stops
Based on past review comments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/core/prompt.pyholmes/core/tool_calling_llm.pyholmes/plugins/toolsets/investigator/core_investigation.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/plugins/toolsets/investigator/core_investigation.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/prompt.pyholmes/core/tool_calling_llm.py
🧠 Learnings (2)
📚 Learning: 2025-12-29T08:35:37.678Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.678Z
Learning: Applies to holmes/plugins/prompts/**/*.jinja2 : Prompts: organize as `holmes/plugins/prompts/{name}.jinja2`
Applied to files:
holmes/core/prompt.py
📚 Learning: 2025-12-29T08:35:37.678Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-29T08:35:37.678Z
Learning: Complex investigations should have LLM evaluation tests
Applied to files:
holmes/core/tool_calling_llm.py
🧬 Code graph analysis (2)
holmes/core/prompt.py (1)
tests/core/test_prompt.py (1)
console(47-48)
holmes/core/tool_calling_llm.py (8)
holmes/utils/tags.py (2)
format_tags_in_string(46-69)parse_messages_tags(72-98)holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(14-58)holmes/core/tracing.py (1)
DummySpan(112-133)holmes/utils/stream.py (4)
StreamEvents(17-25)StreamMessage(28-30)add_token_count_to_metadata(130-142)build_stream_event_token_count(145-151)holmes/core/models.py (5)
ToolApprovalDecision(179-183)ToolCallResult(24-65)PendingToolApproval(170-176)as_tool_result_response(43-53)as_tool_call_message(31-41)holmes/utils/global_instructions.py (1)
generate_runbooks_args(38-69)holmes/core/prompt.py (1)
generate_user_prompt(61-74)holmes/core/tools.py (1)
StructuredToolResultStatus(53-77)
🔇 Additional comments (3)
holmes/core/prompt.py (2)
98-104: LGTM: KAITO flag implementation is correct.The environment variable parsing and injection into the template context follows a clear pattern. The default value of "true" ensures backward compatibility, and the flag is properly exposed for template rendering.
115-118: LGTM: Conditional TodoWrite behavior is well-documented.The inline comment clearly explains why TodoWrite is disabled when KAITO is enabled, addressing the question from past review comments about TodoWrite and KAITO compatibility.
holmes/core/tool_calling_llm.py (1)
1-76: LGTM: Imports are properly organized.All imports are now at the top of the file as required by the coding guidelines, addressing the issues from previous review comments.
| def _handle_kaito_termination_logic(self, tool_call_result, messages, tool_calls, i, max_steps): | ||
| """ | ||
| Handle KAITO-specific debug logging and intelligent termination logic. | ||
|
|
||
| Returns: | ||
| int: Updated iteration counter (i) | ||
| """ | ||
| # 🔍 Debug: Log every tool result for termination analysis | ||
| logging.info(f"🔍 TERMINATION DEBUG: Tool {tool_call_result.tool_name} status={tool_call_result.result.status}") | ||
| if tool_call_result.result.status == StructuredToolResultStatus.ERROR: | ||
| logging.info(f"🔍 TERMINATION DEBUG: Error message: {tool_call_result.result.error}") | ||
|
|
||
| # Check for intelligent termination when safeguards block calls | ||
| # Skip check on first iteration (need at least one successful tool call) | ||
| logging.info(f"🔍 SAFEGUARD CHECK VARIABLES: i={i}, i>1={i > 1}") | ||
| logging.info(f"🔍 SAFEGUARD CHECK VARIABLES: status={tool_call_result.result.status}, is_error={tool_call_result.result.status == StructuredToolResultStatus.ERROR}") | ||
| logging.info(f"🔍 SAFEGUARD CHECK VARIABLES: error_text='{tool_call_result.result.error}'") | ||
| logging.info(f"🔍 SAFEGUARD CHECK VARIABLES: contains_already_called={'already been called' in (tool_call_result.result.error or '')}") | ||
|
|
||
| if (i > 1 and | ||
| tool_call_result.result.status == StructuredToolResultStatus.ERROR and | ||
| "already been called" in (tool_call_result.result.error or "")): | ||
| logging.info("🔍 TERMINATION DEBUG: Safeguard condition met, checking if LLM wants to stop...") | ||
| if self._should_stop_investigation(messages): | ||
| logging.info("🎯 Safeguard blocked + LLM wants same tool → Forcing final iteration") | ||
| # Remove the duplicate/error tool message that caused confusion | ||
| if messages and messages[-1]["role"] == "tool": | ||
| logging.info("🧹 Removing confusing duplicate tool message from conversation") | ||
| messages.pop() | ||
| # Also remove the corresponding tool call from tool_calls list | ||
| if tool_calls: | ||
| tool_calls.pop() | ||
| # Force final iteration by setting i to max_steps - 1 | ||
| # This will trigger tools=None on next iteration, leading to natural final response | ||
| i = max_steps - 1 | ||
| # Don't break - let the loop continue to the final iteration | ||
| else: | ||
| logging.info("🔍 TERMINATION DEBUG: LLM says continue despite safeguard") | ||
| else: | ||
| logging.info("🔍 TERMINATION DEBUG: Safeguard condition NOT met") | ||
|
|
||
| # Exit immediately if intelligent termination was triggered during tool execution | ||
| # (No longer needed since we don't break, just continue to final iteration) | ||
|
|
||
| # Check for intelligent termination after tool execution | ||
| # Skip check on first iteration (need at least one tool call) | ||
| # Also skip if we're already on the final iteration | ||
| logging.info(f"🔍 GENERAL CHECK VARIABLES: i={i}, i>1={i > 1}, max_steps={max_steps}") | ||
| if i > 1 and i < max_steps - 1: # Only check if not already on final iteration | ||
| logging.info("🔍 TERMINATION DEBUG: Checking general termination condition...") | ||
| if self._should_stop_investigation(messages): | ||
| logging.info("🎯 Terminating early to prevent repetitive tool calls") | ||
| # Force final iteration by setting i to max_steps - 1 | ||
| # This will trigger tools=None on next iteration, leading to natural final response | ||
| i = max_steps - 1 | ||
| # Don't break - let the loop continue to the final iteration | ||
| else: | ||
| logging.info("🔍 TERMINATION DEBUG: LLM says continue") | ||
| else: | ||
| logging.info("🔍 TERMINATION DEBUG: General condition NOT met (i<=1 or already final iteration)") | ||
|
|
||
| return i |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reduce excessive logging and clarify side effects.
This method has concerns previously flagged in past review comments:
-
Excessive logging: 12+ debug/info logging calls obscure the core logic and will clutter production logs. Per past review feedback, reduce to only essential decision points.
-
Unclear side effects: The method mutates
messages(line 243) andtool_calls(line 246) while returning a modifiedi. This makes the method's contract unclear and harder to test. -
Naming: The method name includes "logic" which is often a code smell suggesting the method does too much.
💡 Suggested improvements
-
Reduce logging to key decision points only:
- When safeguard condition triggers termination
- When general termination check triggers
- Remove intermediate variable dumps
-
Make side effects explicit in the method signature or return a dataclass:
@dataclass
class TerminationResult:
updated_iteration: int
should_prune_messages: bool
should_prune_tool_calls: bool- Consider extracting to a separate class as suggested in past review:
class KaitoTerminationHandler:
def handle_termination(self, ...) -> TerminationResult:
...Based on past review comments.
🤖 Prompt for AI Agents
In holmes/core/tool_calling_llm.py around lines 215–276, the
_handle_kaito_termination_logic method currently emits excessive debug/info
logs, mutates messages and tool_calls in-place, and has a vague name; reduce
logging to only essential decision points (safeguard-triggered termination and
general early-termination), remove intermediate variable dumps, and keep logs
concise. Change the method to make side effects explicit by returning a small
TerminationResult (or tuple) that includes updated_iteration and flags like
should_prune_messages and should_prune_tool_calls instead of mutating inputs
in-place; update callers to apply pruning when flags are set. Rename the method
to something clearer (e.g., handle_termination or determine_termination) and
consider extracting into a KaitoTerminationHandler class if desired; ensure unit
tests are added/updated to assert returned flags and iteration value rather than
inspecting mutated inputs. Ensure behavior remains the same: when safeguard or
general termination triggers, updated_iteration = max_steps - 1 and pruning
flags indicate removal of the last tool message/tool_call.
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.