add --fast flag #892
add --fast flag #892
Conversation
WalkthroughAdds a disable_todos/fast path: a new CLI flag (--fast) propagates to LLM/tool initialization, causing create_console_tool_executor to omit the core_investigation toolset. Prompt rendering now receives has_todos derived from toolsets and conditionally includes Todo guidance in the system prompt. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as holmes/main.py (ask)
participant Config as Config
participant Exec as ToolExecutor
participant Prompt as Prompt Renderer
User->>CLI: holmes ask ... [--fast]
CLI->>Config: create_console_toolcalling_llm(disable_todos=fast)
Config->>Config: create_console_tool_executor(disable_todos)
alt disable_todos = true
Config->>Exec: Build toolsets (exclude core_investigation)
else disable_todos = false
Config->>Exec: Build toolsets (include core_investigation)
end
CLI->>Prompt: render_system_prompt(context from Exec)
Prompt->>Prompt: has_todos = "core_investigation" in toolsets
Prompt-->>CLI: System prompt (Todo guidance conditional on has_todos)
Note over CLI,Exec: Subsequent tool calls limited by available toolsets
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/prompt.py (1)
78-79: Gate Todo reminder on has_todos in fast modeThe call to
get_tasks_management_system_reminder()is currently unconditional, so even whenhas_todosis false (fast mode) the reminder is still appended. This contradicts the intended behavior.• In holmes/core/prompt.py (around line 78), wrap the append in an
if has_todosblock:- user_prompt_with_files += get_tasks_management_system_reminder() + if has_todos: + user_prompt_with_files += get_tasks_management_system_reminder()• Update the existing tests in tests/core/test_prompt.py (lines 43, 68, and 118) so that:
– For scenarios wherehas_todosis true, they continue to assert that the reminder is appended.
– Add at least one new test wherehas_todosis false and assert thatuser_prompt_with_filesdoes not include the reminder.
🧹 Nitpick comments (7)
holmes/config.py (4)
306-311: Adddisable_todosparam: good, but document the behavior and align naming.The new flag is sensible. Please also update the docstring to explicitly describe the effect of
disable_todos, and consider naming consistency: here it'srefresh_statuswhile callers passrefresh_toolsets(Line 358). Not a blocker, but mildly confusing.Apply this docstring addition to clarify the parameter:
def create_console_tool_executor( self, dal: Optional["SupabaseDal"], refresh_status: bool = False, disable_todos: bool = False, ) -> ToolExecutor: """ Creates a ToolExecutor instance configured for CLI usage. This executor manages the available tools and their execution in the command-line interface. The method loads toolsets in this order, with later sources overriding earlier ones: 1. Built-in toolsets (tagged as CORE or CLI) 2. toolsets from config file will override and be merged into built-in toolsets with the same name. 3. Custom toolsets from config files which can not override built-in toolsets + + Args: + refresh_status: When True, refresh each toolset’s status before returning. + disable_todos: When True, omit the Todo-related toolset(s) from the CLI executor + (currently implemented by filtering out the "core_investigation" toolset). """
325-330: Avoid magic string; add debug log to aid support.Filtering by the literal name
"core_investigation"couples behavior to a display name. Prefer a capability flag (e.g.,ts.capabilities.get("todos")) or a constant/enum. At minimum, emit a debug log so users can confirm what got disabled.Minimal improvement with logging:
- if disable_todos: - cli_toolsets = [ - ts for ts in cli_toolsets if ts.name != "core_investigation" - ] + if disable_todos: + before = len(cli_toolsets) + cli_toolsets = [ts for ts in cli_toolsets if ts.name != "core_investigation"] + after = len(cli_toolsets) + if before != after: + logging.debug("Fast mode: disabled 'core_investigation' toolset (Todo tools)")Longer-term, consider tagging toolsets with a
featuresdict (e.g.,{"todos": True}) and filter by feature instead of name.
351-357: Propagatingdisable_todosis correct; add a type totracerfor mypy.Tiny type-hint tweak keeps us aligned with “Type hints are required.”
def create_console_toolcalling_llm( self, dal: Optional["SupabaseDal"] = None, refresh_toolsets: bool = False, - tracer=None, + tracer: Optional[Any] = None, disable_todos: bool = False, ) -> "ToolCallingLLM":
358-360: Pass by keyword to future-proof the call.Using positional args makes future param reordering risky. Use keywords for clarity.
- tool_executor = self.create_console_tool_executor( - dal, refresh_toolsets, disable_todos - ) + tool_executor = self.create_console_tool_executor( + dal=dal, refresh_status=refresh_toolsets, disable_todos=disable_todos + )holmes/core/prompt.py (1)
57-59: Prefer capability-based detection over toolset name.Checking
ts.name == "core_investigation"is brittle. If the toolset is renamed or split, this will silently break. Consider either:
- Detect the actual tool presence (e.g.,
'TodoWrite' in ts.tools_by_name), or- Use a capability flag (e.g.,
ts.capabilities.get("todos")).holmes/plugins/prompts/_general_instructions.jinja2 (1)
54-66: Conditional Todo guidance looks good; align naming with actual tool identifier.The gated block achieves the intended behavior. Minor wording nit: elsewhere we reference
TodoWriteTool. Consider consistently using the canonical tool name to avoid confusion.holmes/main.py (1)
218-222: Consider parity across investigate subcommands.If “fast mode” is useful beyond
ask, consider adding--fasttoinvestigatecommands (jira, github, pagerduty, opsgenie) and plumbing it to their respectivecreate_console_issue_investigatorpaths (or add a shared helper). Not required for this PR, but improves UX consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
holmes/config.py(3 hunks)holmes/core/prompt.py(1 hunks)holmes/main.py(2 hunks)holmes/plugins/prompts/_general_instructions.jinja2(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Type hints are required (project is type-checked with mypy)
Use Ruff for formatting and linting (configured in pyproject.toml)
Files:
holmes/core/prompt.pyholmes/main.pyholmes/config.py
holmes/plugins/prompts/*.jinja2
📄 CodeRabbit inference engine (CLAUDE.md)
Prompts must be Jinja2 templates named {name}.jinja2 under holmes/plugins/prompts/
Files:
holmes/plugins/prompts/_general_instructions.jinja2
🧠 Learnings (1)
📚 Learning: 2025-08-17T08:42:48.789Z
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-17T08:42:48.789Z
Learning: Applies to holmes/plugins/prompts/*.jinja2 : Prompts must be Jinja2 templates named {name}.jinja2 under holmes/plugins/prompts/
Applied to files:
holmes/plugins/prompts/_general_instructions.jinja2
🧬 Code graph analysis (2)
holmes/core/prompt.py (1)
tests/plugins/toolsets/test_prometheus_integration.py (1)
tool_executor(21-28)
holmes/config.py (1)
holmes/main.py (1)
refresh_toolsets(956-966)
⏰ 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). (5)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (4)
holmes/core/prompt.py (2)
57-59: Good: derivehas_todosfrom active toolsets.This is the right direction to decouple prompt content from CLI flags.
67-67: Plumbinghas_todosinto the template context is correct.This enables prompt-side gating without additional conditionals in code.
holmes/main.py (2)
218-222: Nice addition:--fastflag for ask.Flag naming and help text are clear.
260-260: Correct propagation of--fastto tool construction.This ties the CLI flag to tool availability as intended.
1 participant
I'm not crazy about this implementation. I would much rather has the --fast flag enable/disable the ToDo toolset and have everything else happen automatically. i.e. if each toolset had an API like 'add_to_system_prompt' then we could just disable the toolset and automatically nothing would be added to the prompt.