feat(runbook): introduce runbook toolset to fetch internal runbooks - #547
Conversation
WalkthroughThis change introduces a runbook catalog system to the project. It adds the ability to load and expose runbook metadata, fetch runbook content using a new toolset, and render available runbooks in the prompt context. The update includes catalog management, template enhancements, new documentation, and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HolmesCLI
participant Config
participant RunbookCatalog
participant PromptTemplate
participant RunbookToolset
User->>HolmesCLI: ask <question>
HolmesCLI->>Config: get_runbook_catalog()
Config->>RunbookCatalog: load_runbook_catalog()
RunbookCatalog-->>Config: catalog JSON (or empty)
Config-->>HolmesCLI: catalog JSON
HolmesCLI->>PromptTemplate: Render with context (runbooks)
PromptTemplate->>User: Display available runbooks
User->>HolmesCLI: request to fetch runbook
HolmesCLI->>RunbookToolset: fetch_runbook(path)
RunbookToolset->>RunbookFetcher: _invoke(path)
RunbookFetcher->>RunbookCatalog: get_runbook_by_path(path)
RunbookFetcher->>RunbookFetcher: open/read file
RunbookFetcher-->>RunbookToolset: StructuredToolResult (success/error)
RunbookToolset-->>HolmesCLI: Runbook content or error
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
holmes/config.py (1)
232-240: Refactor to remove unnecessary else clause.The method logic is correct, but the else clause after return is unnecessary as indicated by static analysis.
@staticmethod def get_runbook_catalog() -> str: # TODO(mainred): besides the built-in runbooks, we need to allow the user to bring their own runbooks runbook_catalog = load_runbook_catalog() if runbook_catalog is not None: return runbook_catalog.model_dump_json() - else: - logging.warning("Runbook catalog not found") - return json.dumps({"catalog": []}) + logging.warning("Runbook catalog not found") + return json.dumps({"catalog": []})holmes/plugins/runbooks/README.md (1)
21-21: Apply proper capitalization for Markdown.The markup language name should be capitalized as it's a proper noun.
-Catalog specified in [catalog.json](catalog.json) contains a collection of runbooks written in markdown. +Catalog specified in [catalog.json](catalog.json) contains a collection of runbooks written in Markdown.holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md (2)
8-8: Fix grammatical error."troubleshoot guide" should be "troubleshooting guide" (noun form).
-* Instead of provide next steps to the user, you need to follow the troubleshoot guide to execute the steps. +* Instead of provide next steps to the user, you need to follow the troubleshooting guide to execute the steps.
58-66: Improve readability by varying sentence structure.Multiple consecutive sentences begin with "If" which affects readability. Consider varying the sentence structure while maintaining the technical accuracy.
* **CRITICAL:** ALWAYS refer to the official Kubernetes DNS debugging guide for detailed troubleshooting and solutions: * Main guide: https://kubernetes.io/docs/tasks/administer-cluster/dns-debugging-resolution/ * CoreDNS specific: https://kubernetes.io/docs/tasks/administer-cluster/dns-custom-nameservers/ (for CoreDNS customization which might be relevant) * **DO NOT invent recovery procedures.** Your role is to diagnose and *point* to the correct documentation or standard procedures. -* Based on the findings, suggest which sections of the documentation are most relevant. - * If DNS pods are not running, guide towards checking pod deployment and node health. - * If `/etc/resolv.conf` is incorrect, point to sections on Pod `dnsPolicy` and `dnsConfig`. - * If NetworkPolicies are suspected, suggest reviewing policy definitions to allow DNS. - * If CoreDNS configuration seems problematic, refer to CoreDNS documentation and the Kubernetes guide on customizing it. - * If upstream DNS resolution is failing, suggest checking the upstream DNS servers and CoreDNS forward configuration. +* Based on the findings, suggest which sections of the documentation are most relevant: + * For non-running DNS pods: guide towards checking pod deployment and node health. + * For incorrect `/etc/resolv.conf`: point to sections on Pod `dnsPolicy` and `dnsConfig`. + * When NetworkPolicies are suspected: suggest reviewing policy definitions to allow DNS. + * For problematic CoreDNS configuration: refer to CoreDNS documentation and the Kubernetes guide on customizing it. + * When upstream DNS resolution fails: suggest checking the upstream DNS servers and CoreDNS forward configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 9216a7b and 1c0e70835a767f88eb3de24c433fe5dd2f1c8c0c.
📒 Files selected for processing (14)
holmes/config.py(2 hunks)holmes/main.py(2 hunks)holmes/plugins/prompts/_general_instructions.jinja2(2 hunks)holmes/plugins/prompts/_runbook_instructions.jinja2(1 hunks)holmes/plugins/runbooks/README.md(1 hunks)holmes/plugins/runbooks/__init__.py(3 hunks)holmes/plugins/runbooks/catalog.json(1 hunks)holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md(1 hunks)holmes/plugins/toolsets/__init__.py(3 hunks)holmes/plugins/toolsets/runbook/runbook_fetcher.py(1 hunks)tests/plugins/prompt/test_generic_ask_conversation.py(2 hunks)tests/plugins/runbooks/test_catalog.py(1 hunks)tests/plugins/toolsets/test_runbook.py(1 hunks)tests/plugins/toolsets/test_tool_kafka.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/plugins/toolsets/test_tool_kafka.py
🧰 Additional context used
🪛 LanguageTool
holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md
[grammar] ~8-~8: The word ‘troubleshoot’ is a verb. Did you mean the noun “troubleshooting” or “troubleshooting guide”?
Context: ...eps to the user, you need to follow the troubleshoot guide to execute the steps. * When ge...
(PREPOSITION_VERB)
[style] ~64-~64: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...od dnsPolicy and dnsConfig. * If NetworkPolicies are suspected, suggest ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~65-~65: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...olicy definitions to allow DNS. * If CoreDNS configuration seems problematic...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~66-~66: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rnetes guide on customizing it. * If upstream DNS resolution is failing, sug...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
holmes/plugins/runbooks/README.md
[uncategorized] ~8-~8: Possible missing preposition found.
Context: ... - Standardize operational processes - Enable quick onboarding for new team members -...
(AI_HYDRA_LEO_MISSING_TO)
[grammar] ~21-~21: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ins a collection of runbooks written in markdown. During runtime, LLM will compare the r...
(MARKDOWN_NNP)
🪛 markdownlint-cli2 (0.17.2)
holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
58-58: Bare URL used
null
(MD034, no-bare-urls)
59-59: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
59-59: Bare URL used
null
(MD034, no-bare-urls)
62-62: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
63-63: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
64-64: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
65-65: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
66-66: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Pylint (3.3.7)
holmes/config.py
[refactor] 236-240: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
holmes/plugins/runbooks/__init__.py
[refactor] 58-58: Too few public methods (0/2)
(R0903)
[refactor] 69-69: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (3.12)
🔇 Additional comments (21)
holmes/plugins/prompts/_general_instructions.jinja2 (2)
36-36: LGTM! Good integration of runbook instructions.The placement of the runbook instructions template inclusion is well-positioned in the flow of the general instructions.
80-80: LGTM! Good typo fix.Correcting "kubectl decribe" to "kubectl describe" improves the accuracy of the example command.
holmes/plugins/toolsets/__init__.py (1)
12-12: LGTM! Proper integration of RunbookToolset.The RunbookToolset is correctly imported and added to the list of instantiated toolsets, following the established pattern for toolset integration.
Also applies to: 33-33, 76-76
holmes/main.py (2)
7-7: LGTM! Import reordering is acceptable.The import reordering doesn't affect functionality and maintains readability.
396-396: LGTM! Proper integration of runbook catalog into template context.Adding the runbook catalog to the template context enables the prompt templates to dynamically reference available runbooks, which is the correct integration approach.
holmes/config.py (1)
21-25: LGTM! Proper import expansion for runbook functionality.The additional imports for runbook catalog loading are correctly added to support the new functionality.
holmes/plugins/runbooks/catalog.json (1)
1-9: LGTM! Well-structured runbook catalog.The JSON catalog structure is clean and appropriate with clear metadata fields (update_date, description, link). The relative path approach for the link field is a good design choice for maintainability.
tests/plugins/runbooks/test_catalog.py (1)
6-17: LGTM! Comprehensive test coverage for catalog validation.The test effectively validates the runbook catalog loading and integrity by checking catalog structure, required fields, and file existence. The descriptive error message will help with debugging if runbook files are missing.
tests/plugins/prompt/test_generic_ask_conversation.py (3)
3-3: LGTM! Proper import addition for new functionality.
8-8: Good practice: Moving template variable to local scope.This improves code organization by keeping the variable scoped to where it's used.
37-41: LGTM! Well-structured test for runbook prompt integration.The test properly validates that runbook catalog data is correctly integrated into the prompt rendering system.
holmes/plugins/prompts/_runbook_instructions.jinja2 (1)
1-15: LGTM! Well-structured template with proper conditional rendering.The template correctly handles the case where runbooks may not be available and provides clear instructions for LLM usage.
tests/plugins/toolsets/test_runbook.py (1)
8-26: LGTM! Comprehensive test coverage for RunbookFetcher.The test thoroughly validates both error and success scenarios, properly checking tool result status, error handling, and data return. The test for the parameterized one-liner method ensures the tool description functionality works correctly.
holmes/plugins/runbooks/README.md (1)
1-23: LGTM! Excellent documentation for the runbook system.The README provides clear and comprehensive documentation explaining the purpose, structure, and different types of runbooks. This will be valuable for developers understanding the system.
holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md (1)
1-67: Excellent technical content with comprehensive troubleshooting workflow.The runbook provides a well-structured, step-by-step approach to DNS troubleshooting in Kubernetes environments. The workflow covers all major DNS failure scenarios and includes specific commands and configuration checks. The emphasis on referencing official documentation rather than inventing solutions is particularly valuable.
holmes/plugins/runbooks/__init__.py (3)
58-76: Well-designed Pydantic models for runbook catalog management.The
RunbookCatalogEntryandRunbookCatalogmodels provide a clean, type-safe interface for managing runbook metadata. The structure effectively separates catalog metadata from the actual runbook content, enabling flexible runbook management.
78-94: Excellent error handling in catalog loading function.The
load_runbook_catalog()function demonstrates robust error handling:
- Gracefully handles missing catalog files
- Properly catches and logs JSON decode errors
- Includes comprehensive exception handling with informative error messages
- Returns
Noneconsistently for all error cases, making it easy for callers to handleThis approach ensures the system remains stable even when the catalog is missing or malformed.
97-100: Simple and effective path resolution utility.The
get_runbook_by_path()function provides a clean abstraction for resolving runbook paths relative to the module directory. This encapsulation makes it easier to modify path resolution logic in the future if needed.holmes/plugins/toolsets/runbook/runbook_fetcher.py (3)
17-33: Well-implemented tool following framework conventions.The
RunbookFetchertool is properly structured with:
- Clear parameter definition with appropriate validation
- Descriptive name and documentation
- Proper integration with the toolset framework
The implementation follows established patterns in the codebase.
34-53: Robust error handling and integration.The
_invokemethod demonstrates good practices:
- Proper integration with the catalog system via
get_runbook_by_path()- Comprehensive exception handling with informative error messages
- Consistent use of
StructuredToolResultfor both success and error cases- Appropriate logging for debugging
The error handling ensures the system remains stable when runbook files are missing or inaccessible.
60-77: Comprehensive toolset configuration.The
RunbookToolsetis well-configured with:
- Appropriate metadata (name, description, icon, documentation URL)
- Proper tagging as a core toolset
- Default enablement for immediate availability
- Clean empty configuration as expected for a parameter-free toolset
This configuration ensures the toolset integrates seamlessly into the broader system.
|
@nilo19 sorry I closed the earlier PR, and after I force pushed the branch and try to reopen that PR, it's not allowed anymore. Please review this PR. |
1c0e708 to
9282f6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
holmes/config.py (1)
232-240: Refactor to remove unnecessary else clause.The implementation logic is correct, but the else clause after return is unnecessary and can be simplified.
Apply this diff to improve code style:
@staticmethod def get_runbook_catalog() -> str: # TODO(mainred): besides the built-in runbooks, we need to allow the user to bring their own runbooks runbook_catalog = load_runbook_catalog() if runbook_catalog is not None: return runbook_catalog.model_dump_json() - else: - logging.warning("Runbook catalog not found") - return json.dumps({"catalog": []}) + + logging.warning("Runbook catalog not found") + return json.dumps({"catalog": []})holmes/plugins/runbooks/README.md (1)
1-23: Improve grammar and formatting for professional documentation.The documentation content is excellent and clearly explains the runbook system structure. However, there are some minor language improvements that would enhance readability.
Consider these improvements:
-Runbooks folder contains operational runbooks for the HolmesGPT project. +The Runbooks folder contains operational runbooks for the HolmesGPT project. -Enable quick onboarding for new team members +Enable quick onboarding for new team members -Catalog specified in [catalog.json](catalog.json) contains a collection of runbooks written in markdown. +Catalog specified in [catalog.json](catalog.json) contains a collection of runbooks written in Markdown.holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md (1)
1-67: Comprehensive and technically sound DNS troubleshooting guide.This runbook provides excellent coverage of Kubernetes DNS troubleshooting scenarios with a logical workflow. The step-by-step approach and specific examples make it highly practical for operations teams.
Minor formatting improvements suggested:
Consider addressing these formatting issues for better consistency:
- Fix indentation for the recommendation list (lines 58-66) - use 2 spaces instead of 4
- Format bare URLs as proper markdown links:
-https://kubernetes.io/docs/tasks/administer-cluster/dns-debugging-resolution/ +[Kubernetes DNS debugging guide](https://kubernetes.io/docs/tasks/administer-cluster/dns-debugging-resolution/)- Consider varying sentence structure in the recommendation section to avoid repetitive "If..." beginnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 1c0e70835a767f88eb3de24c433fe5dd2f1c8c0c and 9282f6d.
📒 Files selected for processing (14)
holmes/config.py(2 hunks)holmes/main.py(2 hunks)holmes/plugins/prompts/_general_instructions.jinja2(2 hunks)holmes/plugins/prompts/_runbook_instructions.jinja2(1 hunks)holmes/plugins/runbooks/README.md(1 hunks)holmes/plugins/runbooks/__init__.py(3 hunks)holmes/plugins/runbooks/catalog.json(1 hunks)holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md(1 hunks)holmes/plugins/toolsets/__init__.py(3 hunks)holmes/plugins/toolsets/runbook/runbook_fetcher.py(1 hunks)tests/plugins/prompt/test_generic_ask_conversation.py(2 hunks)tests/plugins/runbooks/test_catalog.py(1 hunks)tests/plugins/toolsets/test_runbook.py(1 hunks)tests/plugins/toolsets/test_tool_kafka.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/plugins/toolsets/test_tool_kafka.py
🚧 Files skipped from review as they are similar to previous changes (9)
- holmes/plugins/prompts/_general_instructions.jinja2
- holmes/main.py
- tests/plugins/runbooks/test_catalog.py
- holmes/plugins/prompts/_runbook_instructions.jinja2
- holmes/plugins/runbooks/catalog.json
- tests/plugins/toolsets/test_runbook.py
- holmes/plugins/toolsets/init.py
- holmes/plugins/toolsets/runbook/runbook_fetcher.py
- tests/plugins/prompt/test_generic_ask_conversation.py
🧰 Additional context used
🪛 Pylint (3.3.7)
holmes/config.py
[refactor] 236-240: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
holmes/plugins/runbooks/__init__.py
[refactor] 58-58: Too few public methods (0/2)
(R0903)
[refactor] 69-69: Too few public methods (0/2)
(R0903)
🪛 LanguageTool
holmes/plugins/runbooks/README.md
[uncategorized] ~2-~2: You might be missing the article “the” here.
Context: # Runbooks Runbooks folder contains operational runbooks fo...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~8-~8: Possible missing preposition found.
Context: ... - Standardize operational processes - Enable quick onboarding for new team members -...
(AI_HYDRA_LEO_MISSING_TO)
[grammar] ~21-~21: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...ins a collection of runbooks written in markdown. During runtime, LLM will compare the r...
(MARKDOWN_NNP)
holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md
[grammar] ~8-~8: The word ‘troubleshoot’ is a verb. Did you mean the noun “troubleshooting” or “troubleshooting guide”?
Context: ...eps to the user, you need to follow the troubleshoot guide to execute the steps. * When ge...
(PREPOSITION_VERB)
[style] ~64-~64: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...od dnsPolicy and dnsConfig. * If NetworkPolicies are suspected, suggest ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~65-~65: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...olicy definitions to allow DNS. * If CoreDNS configuration seems problematic...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~66-~66: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rnetes guide on customizing it. * If upstream DNS resolution is failing, sug...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
holmes/plugins/runbooks/networking/dns_troubleshooting_instructions.md
58-58: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
58-58: Bare URL used
null
(MD034, no-bare-urls)
59-59: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
59-59: Bare URL used
null
(MD034, no-bare-urls)
62-62: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
63-63: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
64-64: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
65-65: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
66-66: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
🔇 Additional comments (4)
holmes/config.py (1)
21-25: LGTM! Import expansion looks good.The addition of
load_runbook_catalogto the import statement properly supports the new runbook catalog functionality.holmes/plugins/runbooks/__init__.py (3)
58-76: Well-designed Pydantic models for runbook catalog.The model design is clean and appropriate. The docstrings clearly explain the purpose and distinction from existing Runbook classes.
Note: The pylint warnings about "too few public methods" can be safely ignored as these are data models where having few methods is expected and appropriate.
78-94: Excellent error handling in catalog loading.The function properly handles both JSON decode errors and unexpected exceptions with appropriate logging. The graceful fallback to None allows the caller to handle missing catalogs appropriately.
97-100: Simple and effective path resolution helper.This helper function correctly resolves relative paths to absolute paths within the runbooks directory.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
holmes/plugins/toolsets/runbook/runbook_fetcher.py (1)
23-23: Consider the past review suggestion for the description.The description currently uses "runbook link" but a previous reviewer suggested "runbook path" might be more accurate. While the current wording aligns with the parameter name "link" and the TODO comment about future external sources, consider if "runbook path" would be clearer for the current internal-only implementation.
🧹 Nitpick comments (2)
holmes/plugins/toolsets/runbook/runbook_fetcher.py (2)
15-16: Document the current limitations and future plans.The TODO comment mentions future external source support, but it would be helpful to document what types of external sources are planned (HTTP URLs, git repositories, etc.) to guide the current "link" parameter design.
35-54: Consider adding input validation and more specific error handling.The current implementation has broad exception handling which might mask specific issues. Consider:
- Validating that the
linkparameter is not empty or None- Handling specific exceptions like
FileNotFoundErrorvs otherIOErrortypes- Adding a size limit check for runbook files to prevent memory issues
def _invoke(self, params: Any) -> StructuredToolResult: path: str = params["link"] + + if not path or not path.strip(): + return StructuredToolResult( + status=ToolResultStatus.ERROR, + error="Runbook link cannot be empty", + params=params, + ) runbook_path = get_runbook_by_path(path) try: with open(runbook_path, "r") as file: content = file.read() return StructuredToolResult( status=ToolResultStatus.SUCCESS, data=content, params=params, ) + except FileNotFoundError: + err_msg = f"Runbook not found at path {runbook_path}" + logging.error(err_msg) + return StructuredToolResult( + status=ToolResultStatus.ERROR, + error=err_msg, + params=params, + ) - except Exception as e: + except (IOError, OSError) as e: err_msg = f"Failed to read runbook {runbook_path}: {str(e)}" logging.error(err_msg) return StructuredToolResult( status=ToolResultStatus.ERROR, error=err_msg, params=params, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
holmes/plugins/prompts/_general_instructions.jinja2(1 hunks)holmes/plugins/prompts/_runbook_instructions.jinja2(1 hunks)holmes/plugins/prompts/generic_ask.jinja2(1 hunks)holmes/plugins/toolsets/runbook/runbook_fetcher.py(1 hunks)tests/plugins/prompt/test_generic_ask_conversation.py(2 hunks)tests/plugins/toolsets/test_runbook.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/plugins/toolsets/test_runbook.py
- holmes/plugins/prompts/generic_ask.jinja2
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/plugins/prompts/_general_instructions.jinja2
- tests/plugins/prompt/test_generic_ask_conversation.py
- holmes/plugins/prompts/_runbook_instructions.jinja2
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
🔇 Additional comments (2)
holmes/plugins/toolsets/runbook/runbook_fetcher.py (2)
61-78: Well-structured toolset implementation.The RunbookToolset class follows the established pattern correctly with appropriate metadata, tagging, and default configuration. The empty example config is appropriate since no configuration is needed for this toolset.
1-12: Imports are clean and appropriate.The imports follow the project structure and include all necessary components for the tool framework integration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
holmes/config.py (1)
237-245: Refactor to remove unnecessary else clause.The method implementation is correct and I note the previous discussion about using
is not Nonefor specificity. However, the static analysis suggestion to remove the unnecessary else clause is valid and would improve code readability.Apply this diff to clean up the code structure:
@staticmethod def get_runbook_catalog() -> str: # TODO(mainred): besides the built-in runbooks, we need to allow the user to bring their own runbooks runbook_catalog = load_runbook_catalog() if runbook_catalog is not None: return runbook_catalog.model_dump_json() - else: - logging.warning("Runbook catalog not found") - return json.dumps({"catalog": []}) + + logging.warning("Runbook catalog not found") + return json.dumps({"catalog": []})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
holmes/config.py(2 hunks)holmes/main.py(2 hunks)holmes/plugins/prompts/generic_ask.jinja2(1 hunks)holmes/plugins/toolsets/__init__.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- holmes/plugins/prompts/generic_ask.jinja2
- holmes/main.py
- holmes/plugins/toolsets/init.py
🧰 Additional context used
🪛 Pylint (3.3.7)
holmes/config.py
[refactor] 241-245: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🔇 Additional comments (1)
holmes/config.py (1)
21-25: LGTM! Import addition is necessary for the new functionality.The addition of
load_runbook_catalogto the imports is correct and required for the newget_runbook_catalogmethod.
3 participants
This PR introduces a toolset Runbook to fetch the built-in runbooks, the catalog are integrated into the system prompt. I closed the PR to let llm pick the runbook and append it to the initial steps before executing any tools #533, but it's not already necessary to follow a runbook and the runbook selected might be misleading depending only on the initial user prompt.
The result of toolset will not be appended to the user prompt, but as the other tool result, it will be part of the assistant prompt
fix: #473