Conversation
Contributor
WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InteractiveLoop
participant ToolCallingLLM
participant LLM
participant Console
User->>InteractiveLoop: Enter "/find <query>"
InteractiveLoop->>ToolCallingLLM: call(messages, ..., tool_number_offset)
ToolCallingLLM->>LLM: Search prompt
LLM-->>ToolCallingLLM: Resource list
ToolCallingLLM-->>InteractiveLoop: Resource list
InteractiveLoop->>User: Show selection modal
User->>InteractiveLoop: Select resource
InteractiveLoop->>ToolCallingLLM: call(messages, ..., tool_number_offset)
ToolCallingLLM->>LLM: Request details/actions
LLM-->>ToolCallingLLM: Details + actions
InteractiveLoop->>User: Show actions modal
User->>InteractiveLoop: Select action
alt action is `/run`
InteractiveLoop->>Console: Execute command
else
InteractiveLoop->>ToolCallingLLM: call(messages, ..., tool_number_offset)
ToolCallingLLM->>LLM: Execute action
LLM-->>ToolCallingLLM: Action output
InteractiveLoop->>User: Show action result modal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
holmes/interactive.py (1)
136-141: Simplify style assignment with ternary operator.The conditional style assignment can be more concise.
- # Style non-matching items differently - if not matches and text: - # Dim style for non-matching items - append to base style - style = "class:completion-menu.meta" - else: - # Normal style for matching items - style = "" + # Style non-matching items differently + style = "class:completion-menu.meta" if not matches and text else ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
holmes/core/tool_calling_llm.py(2 hunks)holmes/core/tools.py(1 hunks)holmes/interactive.py(7 hunks)
🧠 Learnings (1)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
🪛 Ruff (0.12.2)
holmes/interactive.py
136-141: Use ternary operator style = "class:completion-menu.meta" if not matches and text else "" instead of if-else-block
(SIM108)
1032-1032: Function definition does not bind loop variable resource_completer
(B023)
1076-1076: Function definition does not bind loop variable modal_session
(B023)
1208-1208: Function definition does not bind loop variable action_completer
(B023)
1243-1243: Function definition does not bind loop variable menu_session
(B023)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nherment
PR: robusta-dev/holmesgpt#408
File: holmes/plugins/toolsets/kubernetes_logs.py:90-97
Timestamp: 2025-05-15T05:13:43.169Z
Learning: In the Kubernetes logs toolset for Holmes, both current and previous logs are intentionally fetched and combined for each pod, even though this requires more API calls. This design ensures all logs are captured even when pods restart but retain their name, providing complete diagnostic information.
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
🪛 Ruff (0.12.2)
holmes/interactive.py
136-141: Use ternary operator style = "class:completion-menu.meta" if not matches and text else "" instead of if-else-block
(SIM108)
1032-1032: Function definition does not bind loop variable resource_completer
(B023)
1076-1076: Function definition does not bind loop variable modal_session
(B023)
1208-1208: Function definition does not bind loop variable action_completer
(B023)
1243-1243: Function definition does not bind loop variable menu_session
(B023)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (4)
holmes/core/tools.py (1)
157-159: Good improvement for user guidance!The dynamic hint generation based on tool number provides more precise instructions to users, making it easier to view specific tool outputs in interactive mode.
holmes/core/tool_calling_llm.py (1)
252-252: Well-designed tool numbering offset mechanism!The addition of
tool_number_offsetparameter maintains backward compatibility while enabling consistent tool numbering across multiple LLM calls in interactive sessions. This is essential for the multi-phase/findcommand workflow.Also applies to: 371-371
holmes/interactive.py (2)
152-171: Well-implemented menu validator!The validator provides flexible input handling by accepting both numbers and descriptions, with clear error messages for invalid selections.
1339-1341: Correct tracer type and tool numbering offset!Good changes:
- Using
DummySpaninstead ofDummyTraceraligns with the imports- Passing
tool_number_offset=len(all_tool_calls_history)ensures consistent tool numbering throughout the interactive sessionAlso applies to: 1548-1548
Comment on lines
+926
to
+1328
| def handle_find_modal( | ||
| find_args: str, | ||
| ai: ToolCallingLLM, | ||
| console: Console, | ||
| messages: Optional[List[Dict]], | ||
| session: PromptSession, | ||
| style: Style, | ||
| ) -> Optional[str]: | ||
| """ | ||
| Handle /find as a modal interaction with its own loop. | ||
| Returns a command to execute (like /run) or None. | ||
| """ | ||
| # Phase 1: Search using LLM | ||
| search_prompt = f""" | ||
| The user wants to look up a resource: {find_args} | ||
|
|
||
| Please search across all available toolsets (Kubernetes, GCP, AWS, etc.) for resources matching this query. | ||
| Use tools like kubectl_find_resource, kubectl_get_by_kind_in_cluster, and any GCP/AWS search tools available. | ||
|
|
||
| After gathering results, format them EXACTLY like this: | ||
|
|
||
| 🐳 Kubernetes | ||
| ├─ [1] Pod: nginx-web-7d9f8b6c5-x2kt4 (namespace: default) | ||
| │ └─ Running on node-1, IP: 10.0.1.5 | ||
| └─ [2] Service: nginx-service (namespace: default) | ||
| └─ LoadBalancer: 34.102.136.180:80 | ||
|
|
||
| ☁️ GCP | ||
| └─ [3] GCE Instance: nginx-prod (zone: us-central1-a) | ||
| └─ Running, External IP: 35.202.123.45 | ||
|
|
||
| IMPORTANT: | ||
| - Use inline numbers [1], [2], etc. for each resource | ||
| - Continue numbering across providers (don't restart at 1) | ||
| - Only show providers that have results | ||
| - If no resources found at all, respond with ONLY: "No resources found matching '{find_args}'" | ||
| - DO NOT add any summary or extra text after the tree structure | ||
| - The response should ONLY contain the tree structure, nothing before or after it | ||
| """ | ||
|
|
||
| # Build messages for the lookup | ||
| lookup_messages: List[Dict] = messages.copy() if messages else [] | ||
| lookup_messages.append({"role": "user", "content": search_prompt}) | ||
|
|
||
| # Get search results from LLM | ||
| console.print( | ||
| f"[bold {AI_COLOR}]Entering find mode - searching for '{find_args}' (press 'q' to exit)[/bold {AI_COLOR}]\n" | ||
| ) | ||
| search_response = ai.call(lookup_messages, trace_span=DummySpan()) | ||
|
|
||
| # Store the search results | ||
| resources_found = search_response.result or "" | ||
| lookup_messages = search_response.messages or [] | ||
|
|
||
| # Check if no results at all (not just some providers with no results) | ||
| if resources_found.strip().startswith("No resources found"): | ||
| console.print(resources_found) | ||
| return None | ||
|
|
||
| # Phase 2: Interactive selection loop | ||
| while True: | ||
| # Display search results | ||
| console.print( | ||
| Panel( | ||
| resources_found, | ||
| padding=(1, 2), | ||
| border_style=AI_COLOR, | ||
| title="Search Results", | ||
| title_align="left", | ||
| ) | ||
| ) | ||
|
|
||
| console.print() # Add blank line for clarity | ||
|
|
||
| # Parse resources from the LLM response to create menu options | ||
| resource_list = [] | ||
| for line in resources_found.split("\n"): | ||
| # Look for lines with [number] pattern | ||
| match = re.search(r"\[(\d+)\]\s+(.+)", line) | ||
| if match: | ||
| num, desc = match.groups() | ||
| # Clean up the description | ||
| desc = desc.strip() | ||
| resource_list.append((num, desc)) | ||
|
|
||
| # Add quit option to resource list | ||
| resource_list.append(("q", "Exit find mode")) | ||
|
|
||
| # Create completer for resource selection (without back option) | ||
| resource_completer = ActionMenuCompleter(resource_list, add_back_option=False) | ||
| resource_validator = ActionMenuValidator(resource_list) | ||
|
|
||
| # Create key bindings for resource selection (same as action selection) | ||
| resource_bindings = KeyBindings() | ||
|
|
||
| @resource_bindings.add("escape") | ||
| def _(event): | ||
| """Override Escape to keep menu open""" | ||
| b = event.app.current_buffer | ||
| b.reset() | ||
| b.start_completion(select_first=True) | ||
|
|
||
| @resource_bindings.add("backspace") | ||
| def _(event): | ||
| """Smart backspace for resource selection""" | ||
| b = event.app.current_buffer | ||
| valid_descriptions = [desc for _, desc in resource_completer.all_options] | ||
|
|
||
| if b.text in valid_descriptions: | ||
| b.text = "" | ||
| elif b.text: | ||
| b.delete_before_cursor() | ||
|
|
||
| b.start_completion(select_first=True) | ||
|
|
||
| from prompt_toolkit.filters import has_completions | ||
|
|
||
| @resource_bindings.add(Keys.Any, filter=~has_completions) | ||
| def _(event): | ||
| """Auto-show completions after any key press""" | ||
| event.app.current_buffer.insert_text(event.data) | ||
| event.app.current_buffer.start_completion(select_first=False) | ||
|
|
||
| # Create a temporary session with resource completion | ||
| modal_session: PromptSession = PromptSession( | ||
| completer=resource_completer, | ||
| validator=resource_validator, | ||
| validate_while_typing=False, | ||
| complete_while_typing=True, | ||
| history=InMemoryHistory(), | ||
| complete_style=CompleteStyle.COLUMN, | ||
| reserve_space_for_menu=min(10, len(resource_list) + 2), | ||
| key_bindings=resource_bindings, | ||
| ) | ||
|
|
||
| # Create a custom style for find mode prompts using AI_COLOR | ||
| find_style = Style.from_dict( | ||
| { | ||
| "prompt": AI_COLOR, # Use AI_COLOR for find mode prompts | ||
| "completion-menu": "bg:#1a1a1a #888888", # Dark background, gray text | ||
| "completion-menu.completion.current": "bg:#1a1a1a #ffffff", # White text for selected | ||
| "completion-menu.meta": "bg:#1a1a1a #666666", # Darker gray for meta | ||
| "completion-menu.meta.current": "bg:#1a1a1a #888888", # Slightly brighter for selected meta | ||
| } | ||
| ) | ||
|
|
||
| console.print("[dim]Type to filter or use ↑↓ arrows, Enter to select[/dim]") | ||
|
|
||
| # Pre-run to show menu immediately | ||
| def pre_run(): | ||
| app = modal_session.app | ||
| if app: | ||
| app.current_buffer.start_completion(select_first=True) | ||
|
|
||
| # Prompt for selection | ||
| selection = modal_session.prompt( | ||
| [("class:prompt", "> ")], style=find_style, pre_run=pre_run | ||
| ) | ||
|
|
||
| # Convert description back to number if needed | ||
| if selection in resource_validator.valid_values: | ||
| selection = resource_validator.valid_values[selection] | ||
|
|
||
| if selection.lower() == "q": | ||
| console.print(f"[bold {AI_COLOR}]Exiting find mode.[/bold {AI_COLOR}]") | ||
| return None | ||
|
|
||
| try: | ||
| # Phase 3: Show resource details with actions | ||
| detail_prompt = f""" | ||
| The user selected option [{selection}] from the search results above. | ||
|
|
||
| Please: | ||
| 1. Use appropriate tools to get detailed information about this specific resource | ||
| 2. Present the key details in a clean, concise format | ||
| 3. Do NOT include an "Available Actions" section in your response | ||
|
|
||
| IMPORTANT: Also include a section at the very end of your response in this exact format: | ||
| ```actions | ||
| 1|Show full details (kubectl describe) | ||
| 2|Show YAML | ||
| 3|Show logs | ||
| 4|Show events | ||
| 5|/run kubectl exec -it <actual-pod-name> -n <actual-namespace> -- sh | ||
| 6|/run kubectl port-forward <actual-pod-name> -n <actual-namespace> 8080:80 | ||
| 7|/run kubectl logs <actual-pod-name> -n <actual-namespace> --tail=100 | ||
| ``` | ||
|
|
||
| For GCP resources, include similar appropriate actions in the actions block. | ||
| """ | ||
|
|
||
| lookup_messages.append({"role": "user", "content": detail_prompt}) | ||
|
|
||
| console.print(f"[bold {AI_COLOR}]Getting details...[/bold {AI_COLOR}]\n") | ||
| detail_response = ai.call(lookup_messages, trace_span=DummySpan()) | ||
|
|
||
| # Display details (but hide the actions code block) | ||
| display_text = detail_response.result or "" | ||
| # Remove the ```actions...``` block from display | ||
| display_text = re.sub( | ||
| r"```actions\n.*?```", "", display_text, flags=re.DOTALL | ||
| ) | ||
| # Also remove extra newlines that might be left | ||
| display_text = re.sub(r"\n{3,}", "\n\n", display_text.strip()) | ||
|
|
||
| console.print( | ||
| Panel( | ||
| Markdown(display_text), | ||
| padding=(1, 2), | ||
| border_style=AI_COLOR, | ||
| title="Resource Details", | ||
| title_align="left", | ||
| ) | ||
| ) | ||
|
|
||
| # Extract actions from the response for menu selection | ||
| actions_list = [] | ||
| actions_match = re.search( | ||
| r"```actions\n(.*?)```", detail_response.result or "", re.DOTALL | ||
| ) | ||
| if actions_match: | ||
| actions_text = actions_match.group(1).strip() | ||
| for line in actions_text.split("\n"): | ||
| if "|" in line: | ||
| num, desc = line.split("|", 1) | ||
| actions_list.append((num.strip(), desc.strip())) | ||
|
|
||
| # Phase 4: Action selection loop | ||
| while True: | ||
| # Display a compact action menu | ||
| console.print() # Blank line for spacing | ||
|
|
||
| if actions_list: | ||
| # Just show a header, the menu will display the options | ||
| console.print( | ||
| f"[bold {AI_COLOR}]Select action for {selection}:[/bold {AI_COLOR}]" | ||
| ) | ||
|
|
||
| # Create a session with completer and validator | ||
| action_completer = ActionMenuCompleter(actions_list) | ||
| action_validator = ActionMenuValidator(actions_list) | ||
|
|
||
| # Create key bindings that auto-show completions | ||
| action_bindings = KeyBindings() | ||
|
|
||
| @action_bindings.add("c-space") | ||
| @action_bindings.add("tab") | ||
| def _(event): | ||
| """Show completions on Tab or Ctrl+Space""" | ||
| b = event.app.current_buffer | ||
| if b.complete_state: | ||
| b.complete_next() | ||
| else: | ||
| b.start_completion(select_first=True) | ||
|
|
||
| @action_bindings.add("escape") | ||
| def _(event): | ||
| """Override Escape to re-show completions instead of hiding them""" | ||
| b = event.app.current_buffer | ||
| # Clear the current input but keep completions open | ||
| b.reset() | ||
| # Immediately restart completion | ||
| b.start_completion(select_first=True) | ||
|
|
||
| # Add a catch-all for any printable character | ||
| from prompt_toolkit.filters import has_completions | ||
|
|
||
| @action_bindings.add(Keys.Any, filter=~has_completions) | ||
| def _(event): | ||
| """Auto-show completions after any key press if not already shown""" | ||
| # Let the key be processed normally first | ||
| event.app.current_buffer.insert_text(event.data) | ||
| # Then ensure completions are shown | ||
| event.app.current_buffer.start_completion(select_first=True) | ||
|
|
||
| # Also handle backspace to keep menu visible | ||
| @action_bindings.add("backspace") | ||
| def _(event): | ||
| """Handle backspace while keeping completions visible""" | ||
| b = event.app.current_buffer | ||
| # Check if current text is a valid full selection | ||
| valid_descriptions = [ | ||
| desc for _, desc in action_completer.all_options | ||
| ] | ||
|
|
||
| if b.text in valid_descriptions: | ||
| # If it's a valid selection description, clear it completely | ||
| b.text = "" | ||
| elif b.text: | ||
| # Otherwise just delete one character | ||
| b.delete_before_cursor() | ||
|
|
||
| # Always restart completion after backspace | ||
| b.start_completion(select_first=True) | ||
|
|
||
| # Create a temporary session with menu-style completion | ||
| menu_session: PromptSession = PromptSession( | ||
| completer=action_completer, | ||
| validator=action_validator, | ||
| validate_while_typing=False, | ||
| complete_while_typing=True, | ||
| history=InMemoryHistory(), | ||
| complete_style=CompleteStyle.COLUMN, # Vertical column style | ||
| reserve_space_for_menu=min( | ||
| 10, len(actions_list) + 2 | ||
| ), # Reserve space for menu | ||
| key_bindings=action_bindings, | ||
| ) | ||
|
|
||
| # Show instruction and prompt | ||
| console.print( | ||
| "[dim]Type to filter or use ↑↓ arrows, Enter to select[/dim]" | ||
| ) | ||
|
|
||
| # Define pre_run to auto-start completion | ||
| def pre_run(): | ||
| # Start completion immediately with first item selected | ||
| app = menu_session.app | ||
| if app: | ||
| app.current_buffer.start_completion(select_first=True) | ||
|
|
||
| action_selection = menu_session.prompt( | ||
| [("class:prompt", "> ")], | ||
| style=find_style, | ||
| default="", # Start with empty | ||
| pre_run=pre_run, # Auto-show menu | ||
| ) | ||
| else: | ||
| # Fallback to original text-based selection | ||
| console.print( | ||
| f"\n[dim]Resource: {selection} - Choose an action or 'b' to go back[/dim]" | ||
| ) | ||
|
|
||
| action_selection = modal_session.prompt( | ||
| [("class:prompt", "Select action [1-N] or 'b' for back: ")], | ||
| style=find_style, | ||
| ) | ||
|
|
||
| # Convert description back to number if needed | ||
| if action_selection in action_validator.valid_values: | ||
| action_number = action_validator.valid_values[action_selection] | ||
| else: | ||
| action_number = action_selection # fallback | ||
|
|
||
| if action_number == "b" or action_selection.lower() == "b": | ||
| break # Back to resource list | ||
|
|
||
| # Check if this is a /run command | ||
| detail_result = detail_response.result or "" | ||
| if "/run" in detail_result: | ||
| # Let LLM extract and execute the action | ||
| action_prompt = f""" | ||
| The user selected action [{action_number}] from the list above. | ||
|
|
||
| If this action is a /run command, please extract and return ONLY the /run command line. | ||
| If it's a describe/show action, execute it using the appropriate tool and show the output. | ||
|
|
||
| For /run commands, respond with ONLY the command like: | ||
| /run kubectl exec -it nginx-pod -- sh | ||
|
|
||
| For other actions: | ||
| 1. First output a line starting with "EXECUTING: " that describes what you're doing (e.g., "EXECUTING: Showing pod YAML") | ||
| 2. Then execute the tool and present the results | ||
|
|
||
| IMPORTANT: For "Show logs" actions, use fetch_pod_logs which defaults to 100 lines. This is usually sufficient. Only if the user asks for more logs or you need to see earlier logs, use a higher limit. | ||
| """ | ||
| lookup_messages.append({"role": "user", "content": action_prompt}) | ||
|
|
||
| action_response = ai.call(lookup_messages, trace_span=DummySpan()) | ||
| action_result = action_response.result or "" | ||
|
|
||
| # Check if response is a /run command | ||
| if action_result.strip().startswith("/run"): | ||
| # Return the command to be executed in main loop | ||
| return action_result.strip() | ||
| else: | ||
| # Extract the action description from EXECUTING line if present | ||
| modal_title = f"Action {action_selection} Result" | ||
| exec_match = re.search( | ||
| r"^EXECUTING:\s*(.+?)(?:\n|$)", action_result, re.MULTILINE | ||
| ) | ||
| if exec_match: | ||
| modal_title = exec_match.group(1) | ||
| # Remove the EXECUTING line from the result | ||
| action_result = re.sub( | ||
| r"^EXECUTING:.*\n", | ||
| "", | ||
| action_result, | ||
| flags=re.MULTILINE, | ||
| ) | ||
|
|
||
| # Display the action result in a modal | ||
| show_action_result_modal(action_result, modal_title, console) | ||
| lookup_messages = action_response.messages or [] | ||
| else: | ||
| console.print( | ||
| f"[bold {ERROR_COLOR}]Invalid selection[/bold {ERROR_COLOR}]" | ||
| ) | ||
|
|
||
| except (ValueError, IndexError): | ||
| console.print(f"[bold {ERROR_COLOR}]Invalid selection[/bold {ERROR_COLOR}]") | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider decomposing this 400+ line function.
The handle_find_modal function is handling multiple distinct phases that could be extracted into separate functions for better maintainability.
Consider breaking down into:
search_resources(find_args, ai, messages)- Phase 1 searchselect_resource(resources_found, session, style)- Phase 2 selectionget_resource_details(selection, ai, messages)- Phase 3 detailsselect_and_execute_action(actions_list, ai, messages, session, style)- Phase 4 action
Would you like me to generate a refactored version or open an issue to track this improvement?
🧰 Tools
🪛 Ruff (0.12.2)
1032-1032: Function definition does not bind loop variable resource_completer
(B023)
1076-1076: Function definition does not bind loop variable modal_session
(B023)
1208-1208: Function definition does not bind loop variable action_completer
(B023)
1243-1243: Function definition does not bind loop variable menu_session
(B023)
🤖 Prompt for AI Agents
In holmes/interactive.py lines 926 to 1328, the handle_find_modal function is
over 400 lines and handles multiple distinct phases: searching resources,
selecting a resource, getting resource details, and selecting/executing actions.
To improve maintainability, refactor by extracting these phases into separate
functions named search_resources(find_args, ai, messages),
select_resource(resources_found, session, style),
get_resource_details(selection, ai, messages), and
select_and_execute_action(actions_list, ai, messages, session, style). Then
update handle_find_modal to orchestrate these calls sequentially, passing
necessary data between them.
Comment on lines
+1169
to
+1220
| action_bindings = KeyBindings() | ||
|
|
||
| @action_bindings.add("c-space") | ||
| @action_bindings.add("tab") | ||
| def _(event): | ||
| """Show completions on Tab or Ctrl+Space""" | ||
| b = event.app.current_buffer | ||
| if b.complete_state: | ||
| b.complete_next() | ||
| else: | ||
| b.start_completion(select_first=True) | ||
|
|
||
| @action_bindings.add("escape") | ||
| def _(event): | ||
| """Override Escape to re-show completions instead of hiding them""" | ||
| b = event.app.current_buffer | ||
| # Clear the current input but keep completions open | ||
| b.reset() | ||
| # Immediately restart completion | ||
| b.start_completion(select_first=True) | ||
|
|
||
| # Add a catch-all for any printable character | ||
| from prompt_toolkit.filters import has_completions | ||
|
|
||
| @action_bindings.add(Keys.Any, filter=~has_completions) | ||
| def _(event): | ||
| """Auto-show completions after any key press if not already shown""" | ||
| # Let the key be processed normally first | ||
| event.app.current_buffer.insert_text(event.data) | ||
| # Then ensure completions are shown | ||
| event.app.current_buffer.start_completion(select_first=True) | ||
|
|
||
| # Also handle backspace to keep menu visible | ||
| @action_bindings.add("backspace") | ||
| def _(event): | ||
| """Handle backspace while keeping completions visible""" | ||
| b = event.app.current_buffer | ||
| # Check if current text is a valid full selection | ||
| valid_descriptions = [ | ||
| desc for _, desc in action_completer.all_options | ||
| ] | ||
|
|
||
| if b.text in valid_descriptions: | ||
| # If it's a valid selection description, clear it completely | ||
| b.text = "" | ||
| elif b.text: | ||
| # Otherwise just delete one character | ||
| b.delete_before_cursor() | ||
|
|
||
| # Always restart completion after backspace | ||
| b.start_completion(select_first=True) | ||
|
|
Contributor
There was a problem hiding this comment.
Fix closure variable binding in action menu key bindings.
Similar closure variable binding issue with action_completer.
# Create key bindings that auto-show completions
action_bindings = KeyBindings()
+
+ # Capture completer in closure
+ _action_completer = action_completer
# ... other bindings ...
@action_bindings.add("backspace")
def _(event):
"""Handle backspace while keeping completions visible"""
b = event.app.current_buffer
# Check if current text is a valid full selection
valid_descriptions = [
- desc for _, desc in action_completer.all_options
+ desc for _, desc in _action_completer.all_options
]
if b.text in valid_descriptions:
# If it's a valid selection description, clear it completely
b.text = ""
elif b.text:
# Otherwise just delete one character
b.delete_before_cursor()
# Always restart completion after backspace
b.start_completion(select_first=True)📝 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
| action_bindings = KeyBindings() | |
| @action_bindings.add("c-space") | |
| @action_bindings.add("tab") | |
| def _(event): | |
| """Show completions on Tab or Ctrl+Space""" | |
| b = event.app.current_buffer | |
| if b.complete_state: | |
| b.complete_next() | |
| else: | |
| b.start_completion(select_first=True) | |
| @action_bindings.add("escape") | |
| def _(event): | |
| """Override Escape to re-show completions instead of hiding them""" | |
| b = event.app.current_buffer | |
| # Clear the current input but keep completions open | |
| b.reset() | |
| # Immediately restart completion | |
| b.start_completion(select_first=True) | |
| # Add a catch-all for any printable character | |
| from prompt_toolkit.filters import has_completions | |
| @action_bindings.add(Keys.Any, filter=~has_completions) | |
| def _(event): | |
| """Auto-show completions after any key press if not already shown""" | |
| # Let the key be processed normally first | |
| event.app.current_buffer.insert_text(event.data) | |
| # Then ensure completions are shown | |
| event.app.current_buffer.start_completion(select_first=True) | |
| # Also handle backspace to keep menu visible | |
| @action_bindings.add("backspace") | |
| def _(event): | |
| """Handle backspace while keeping completions visible""" | |
| b = event.app.current_buffer | |
| # Check if current text is a valid full selection | |
| valid_descriptions = [ | |
| desc for _, desc in action_completer.all_options | |
| ] | |
| if b.text in valid_descriptions: | |
| # If it's a valid selection description, clear it completely | |
| b.text = "" | |
| elif b.text: | |
| # Otherwise just delete one character | |
| b.delete_before_cursor() | |
| # Always restart completion after backspace | |
| b.start_completion(select_first=True) | |
| # Create key bindings that auto-show completions | |
| action_bindings = KeyBindings() | |
| # Capture completer in closure | |
| _action_completer = action_completer | |
| @action_bindings.add("c-space") | |
| @action_bindings.add("tab") | |
| def _(event): | |
| """Show completions on Tab or Ctrl+Space""" | |
| b = event.app.current_buffer | |
| if b.complete_state: | |
| b.complete_next() | |
| else: | |
| b.start_completion(select_first=True) | |
| @action_bindings.add("escape") | |
| def _(event): | |
| """Override Escape to re-show completions instead of hiding them""" | |
| b = event.app.current_buffer | |
| # Clear the current input but keep completions open | |
| b.reset() | |
| # Immediately restart completion | |
| b.start_completion(select_first=True) | |
| # Add a catch-all for any printable character | |
| from prompt_toolkit.filters import has_completions | |
| @action_bindings.add(Keys.Any, filter=~has_completions) | |
| def _(event): | |
| """Auto-show completions after any key press if not already shown""" | |
| # Let the key be processed normally first | |
| event.app.current_buffer.insert_text(event.data) | |
| # Then ensure completions are shown | |
| event.app.current_buffer.start_completion(select_first=True) | |
| # Also handle backspace to keep menu visible | |
| @action_bindings.add("backspace") | |
| def _(event): | |
| """Handle backspace while keeping completions visible""" | |
| b = event.app.current_buffer | |
| # Check if current text is a valid full selection | |
| valid_descriptions = [ | |
| desc for _, desc in _action_completer.all_options | |
| ] | |
| if b.text in valid_descriptions: | |
| # If it's a valid selection description, clear it completely | |
| b.text = "" | |
| elif b.text: | |
| # Otherwise just delete one character | |
| b.delete_before_cursor() | |
| # Always restart completion after backspace | |
| b.start_completion(select_first=True) |
🧰 Tools
🪛 Ruff (0.12.2)
1208-1208: Function definition does not bind loop variable action_completer
(B023)
🤖 Prompt for AI Agents
In holmes/interactive.py around lines 1169 to 1220, the key binding functions
use the closure variable action_completer directly, which can cause late binding
issues. To fix this, capture action_completer as a default argument in the inner
functions where it is used, ensuring the correct reference is maintained during
execution.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
holmes/cli/utils.py (2)
62-65: Optimize scrolling by using count parameter instead of loops.The current implementation uses for loops to move the cursor multiple times, which is inefficient. The
cursor_down()andcursor_up()methods likely support a count parameter.- # Get current window height and scroll by half - window_height = event.app.output.get_size().rows - 1 # -1 for header - scroll_amount = max(1, window_height // 2) - for _ in range(scroll_amount): - text_area.buffer.cursor_down() + # Get current window height and scroll by half + window_height = event.app.output.get_size().rows - 1 # -1 for header + scroll_amount = max(1, window_height // 2) + text_area.buffer.cursor_down(count=scroll_amount)Apply the same pattern to all four scrolling handlers (lines 62-65, 73-76, 84-87, 94-97).
Also applies to: 73-76, 84-87, 94-97
172-175: Consider more specific exception handling.Catching all exceptions might hide important errors. Consider catching specific exceptions that are expected from the prompt_toolkit Application.
- except Exception as e: + except (KeyboardInterrupt, EOFError, OSError) as e: # Fallback to regular panel display console.print(f"[bold red]Error showing modal: {e}[/bold red]") console.print(Panel(content, title=title, border_style=fallback_panel_style))This ensures that only expected UI-related exceptions trigger the fallback, while unexpected errors are properly propagated.
holmes/cli/find_command.py (2)
55-60: Simplify conditional assignment with ternary operator.The if-else block can be simplified for better readability.
- # Style non-matching items differently - if not matches and text: - # Dim style for non-matching items - append to base style - style = "class:completion-menu.meta" - else: - # Normal style for matching items - style = "" + # Style non-matching items differently + style = "class:completion-menu.meta" if not matches and text else ""
487-494: Remove unused parameterssessionandstyle.The
sessionandstyleparameters are never used within the function.def handle_find_modal( find_args: str, ai: ToolCallingLLM, console: Console, messages: Optional[List[Dict]], - session: PromptSession, - style: Style, ) -> Optional[str]:Note: This will require updating the call site in
holmes/interactive.pyline 702-704.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
holmes/cli/__init__.py(1 hunks)holmes/cli/find_command.py(1 hunks)holmes/cli/utils.py(1 hunks)holmes/interactive.py(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- holmes/cli/init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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 (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks
Don't add convenience logs that give away the problem
Don't write logs that directly state the issue
Ensure historical timestamps are properly handled in logs (especially with Loki)
Files:
holmes/cli/utils.pyholmes/cli/find_command.pyholmes/interactive.py
🧬 Code Graph Analysis (3)
holmes/cli/utils.py (1)
holmes/interactive.py (1)
_(768-786)
holmes/cli/find_command.py (3)
holmes/cli/utils.py (12)
show_scrollable_modal(102-175)_(20-21)_(24-25)_(30-32)_(36-38)_(42-44)_(48-54)_(59-65)_(70-76)_(81-87)_(91-97)_(154-161)holmes/core/tool_calling_llm.py (2)
ToolCallingLLM(201-722)call(244-385)holmes/core/tracing.py (1)
DummySpan(37-53)
holmes/interactive.py (7)
holmes/core/tracing.py (2)
completion(182-183)DummySpan(37-53)holmes/core/llm.py (2)
completion(48-58)completion(203-244)holmes/cli/find_command.py (1)
handle_find_modal(487-594)holmes/cli/utils.py (1)
show_scrollable_modal(102-175)holmes/core/prompt.py (1)
build_initial_ask_messages(26-42)holmes/core/tool_calling_llm.py (2)
ToolCallingLLM(201-722)ToolCallResult(140-183)holmes/core/tools.py (2)
pretty_print_toolset_status(535-570)get_stringified_data(55-68)
🪛 Ruff (0.12.2)
holmes/cli/find_command.py
55-60: Use ternary operator style = "class:completion-menu.meta" if not matches and text else "" instead of if-else-block
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
holmes/interactive.py (2)
235-254: Good refactoring to use the unified modal!The function has been successfully refactored to use the shared
show_scrollable_modalutility, eliminating code duplication as suggested in previous reviews.
702-717: Monitor recursive command processing depth.The recursive call to
process_slash_commandwhen handling/findresults could potentially lead to stack overflow if there's a long chain of commands.Consider adding a depth limit or converting to an iterative approach if command chaining becomes extensive:
def process_slash_command(..., depth=0, max_depth=10): if depth > max_depth: console.print(f"[bold {ERROR_COLOR}]Maximum command depth exceeded[/bold {ERROR_COLOR}]") return None, True, messages, last_response, show_tool_output # ... existing code ... return process_slash_command( command_to_run, ..., depth=depth+1 )
Comment on lines
+34
to
+37
| def __init__(self, actions_list, add_back_option=True): | ||
| self.actions = actions_list | ||
| self.all_options = [(num, desc) for num, desc in actions_list] | ||
|
|
Contributor
There was a problem hiding this comment.
Remove unused add_back_option parameter.
The add_back_option parameter in __init__ is never used in the implementation.
- def __init__(self, actions_list, add_back_option=True):
+ def __init__(self, actions_list):
self.actions = actions_list
self.all_options = [(num, desc) for num, desc in actions_list]📝 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
| def __init__(self, actions_list, add_back_option=True): | |
| self.actions = actions_list | |
| self.all_options = [(num, desc) for num, desc in actions_list] | |
| def __init__(self, actions_list): | |
| self.actions = actions_list | |
| self.all_options = [(num, desc) for num, desc in actions_list] |
🤖 Prompt for AI Agents
In holmes/cli/find_command.py around lines 34 to 37, the __init__ method
includes an unused parameter add_back_option; remove this parameter from the
signature and any callers that pass it, and update any references/tests that
construct FindCommand to pass only actions_list (or adjust those call sites to
no longer supply the extra argument). Ensure the constructor only assigns
self.actions and self.all_options from actions_list to keep behavior identical.
| Returns the selected resource number. | ||
| """ | ||
| # Create completer and validator | ||
| resource_completer = ActionMenuCompleter(resource_list, add_back_option=False) |
Contributor
There was a problem hiding this comment.
Update ActionMenuCompleter instantiation after removing unused parameter.
After removing the add_back_option parameter from ActionMenuCompleter, this line needs to be updated.
- resource_completer = ActionMenuCompleter(resource_list, add_back_option=False)
+ resource_completer = ActionMenuCompleter(resource_list)📝 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
| resource_completer = ActionMenuCompleter(resource_list, add_back_option=False) | |
| resource_completer = ActionMenuCompleter(resource_list) |
🤖 Prompt for AI Agents
In holmes/cli/find_command.py around line 210, the ActionMenuCompleter is still
being instantiated with the now-removed add_back_option parameter; update the
call to instantiate ActionMenuCompleter with only the required arguments (e.g.,
ActionMenuCompleter(resource_list)) and remove the add_back_option argument so
the signature matches the updated class.
| content: str, | ||
| title: str, | ||
| console: Console, | ||
| enable_word_wrap: bool = True, |
Contributor
There was a problem hiding this comment.
Inconsistency between enable_word_wrap parameter and wrap_lines initialization.
The enable_word_wrap parameter defaults to True, suggesting word wrap should be enabled by default. However, the TextArea is created with wrap_lines=False, which contradicts this expectation.
text_area = TextArea(
text=content,
read_only=True,
scrollbar=True,
line_numbers=False,
- wrap_lines=False, # Disable word wrap by default
+ wrap_lines=enable_word_wrap, # Set initial wrap state based on parameter
)
- wrap_status = "off"
+ wrap_status = "on" if enable_word_wrap else "off"Also applies to: 126-126
🤖 Prompt for AI Agents
In holmes/cli/utils.py around lines 106 and 126, the parameter enable_word_wrap
defaults to True but the TextArea is constructed with wrap_lines=False (and
similarly elsewhere), creating an inconsistency; update the TextArea
construction to use wrap_lines=enable_word_wrap (or the appropriate variable
name) so the widget's wrap behavior follows the function parameter, and ensure
any other occurrences at line 126 use the same parameter to keep behavior
consistent.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.