Run prerequisite for each cache toolset - #909
Conversation
Walkthroughload_toolset_with_status no longer gates collection of enabled toolsets on the using_cached flag. Enabled toolsets are now derived solely from toolset.enabled and status == ENABLED, regardless of cache usage. Prerequisite checks proceed for these toolsets; other control flow and outputs remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant ToolsetManager
participant StatusStore as Status/Cache
participant Prereq as PrerequisiteChecker
Caller->>ToolsetManager: load_toolset_with_status(...)
ToolsetManager->>StatusStore: Fetch toolset statuses (may be cached or refreshed)
StatusStore-->>ToolsetManager: statuses + using_cached flag
note over ToolsetManager: Build enabled set where<br/>toolset.enabled && status == ENABLED<br/>(ignores using_cached)
ToolsetManager->>Prereq: Check prerequisites for enabled toolsets
Prereq-->>ToolsetManager: Results
ToolsetManager-->>Caller: Final status + enabled 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
🧹 Nitpick comments (5)
holmes/core/toolset_manager.py (5)
267-271: Comment/code mismatch and potential double prerequisite runs after refreshThe new condition now schedules prerequisite checks regardless of whether we just refreshed or are using cached statuses. However, the preceding comment still states "only when the toolset is loaded from cache," which is no longer accurate. Also, when
refresh_status=True(or when the cache file doesn't exist), prerequisites will be checked inrefresh_toolset_status(...)and then checked again here, possibly doubling work and side effects.If "always check" is intentional (aligns with PR title), update the comment and rename the variable to avoid confusion. Otherwise, gate on
using_cachedto avoid duplicate checks.Apply Option A (keep new behavior; clarify intent and naming):
- enabled_toolsets_from_cache: List[Toolset] = [] + # Collect toolsets whose prerequisites we want to (re)check. + # Note: We (re)check prerequisites for enabled toolsets with cached status "enabled" + # regardless of whether we just refreshed or are using cached statuses. + # This ensures prerequisites run for every non-CLI toolset on each load. + enabled_toolsets_to_check: List[Toolset] = [] @@ - # check prerequisites for only enabled toolset when the toolset is loaded from cache. When the toolset is - # not loaded from cache, the prerequisites are checked in the refresh_toolset_status method. - if toolset.enabled and toolset.status == ToolsetStatusEnum.ENABLED: - enabled_toolsets_from_cache.append(toolset) - self.check_toolset_prerequisites(enabled_toolsets_from_cache) + # Check prerequisites for enabled toolsets whose cached status is ENABLED. + if toolset.enabled and toolset.status == ToolsetStatusEnum.ENABLED: + enabled_toolsets_to_check.append(toolset) + self.check_toolset_prerequisites(enabled_toolsets_to_check)Option B (avoid duplicate checks when we just refreshed):
- if toolset.enabled and toolset.status == ToolsetStatusEnum.ENABLED: + if using_cached and toolset.enabled and toolset.status == ToolsetStatusEnum.ENABLED: enabled_toolsets_from_cache.append(toolset)
34-36: Redundant assignment of self.toolsets
self.toolsets = toolsetsis immediately overwritten byself.toolsets = toolsets or {}. Remove the first assignment.- self.toolsets = toolsets self.toolsets = toolsets or {}
439-441: Duplicate assignment when adding a new toolset
existing_toolsets_by_name[new_toolset.name] = new_toolsetis executed twice. Keep a single assignment.else: existing_toolsets_by_name[new_toolset.name] = new_toolset - existing_toolsets_by_name[new_toolset.name] = new_toolset
135-144: Exceptions from prerequisite checks are swallowed
as_completed(futures)is iterated without callingfuture.result(). Exceptions raised incheck_prerequisiteswon't be surfaced or logged, which can hide real failures.- with concurrent.futures.ThreadPoolExecutor(max_workers=10) as executor: + with concurrent.futures.ThreadPoolExecutor(max_workers=10) as executor: futures = [] for toolset in toolsets: futures.append(executor.submit(toolset.check_prerequisites)) - for _ in concurrent.futures.as_completed(futures): - pass + for future in concurrent.futures.as_completed(futures): + try: + # Ensure exceptions are raised and not silently ignored + future.result() + except Exception: + logging.exception("Toolset prerequisite check failed")
42-44: Potential misuse of pydantic FilePath
FilePath(...)is a pydantic type intended for model validation, not for direct instantiation as a path value. Depending on your pydantic version, calling it can raise a TypeError. Preferpathlib.Path(runtime) for constructing paths.If confirmed, switch to
Pathand add the import at the top:@@ -import os +import os +from pathlib import Path @@ - if toolset_status_location is None: - toolset_status_location = FilePath(DEFAULT_TOOLSET_STATUS_LOCATION) + if toolset_status_location is None: + toolset_status_location = Path(DEFAULT_TOOLSET_STATUS_LOCATION)Note: If mypy complains about the annotation
Optional[FilePath], consider changing the parameter and attribute types toOptional[Path] | Optional[str], or keepPathwhile relying on runtime checks in this class.
📜 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 (1)
holmes/core/toolset_manager.py(1 hunks)
🧰 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
Type hints are required (mypy is configured in pyproject.toml)
Files:
holmes/core/toolset_manager.py
🧬 Code graph analysis (1)
holmes/core/toolset_manager.py (1)
holmes/core/tools.py (1)
ToolsetStatusEnum(103-106)
⏰ 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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: Pre-commit checks
2 participants
When running refresh toolset the tools are still eventually loaded from cache and hence we need to still run the prerequisite checks.