[ROB-3715] - Skills - #1953
Merged
Merged
[ROB-3715] - Skills#1953
Conversation
Initial refactor renaming "runbook" to "skills" across the codebase by name only — no format or logic changes. - Rename toolset directory: runbook/ -> skills/ - Rename classes: RunbookFetcher -> SkillsFetcher, RunbookToolset -> SkillsToolset - Toolset name: "runbook" -> "skills" with deprecation mapping - Update all imports and references - Rename 10 eval fixture directories - Update test_case.yaml: runbooks -> skills field, tags - Update pyproject.toml tag: runbooks -> skills - Preserve: fetch_runbook tool name, prompt templates, config fields Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
- Create holmes/plugins/skills/skill_loader.py with Skill, SkillCatalog, SkillSource models and SKILL.md parsing/scanning/loading - Refactor SkillsFetcher to use SkillCatalog instead of catalog.json - Add metadata field to StructuredToolResult for restricted tool gating - Replace custom_runbook_catalogs with custom_skill_paths in config - Replace get_runbook_catalog() with get_skill_catalog() - Update prompt system types: RunbookCatalog -> SkillCatalog - Migrate 6 eval fixtures to SKILL.md format with path-based loading - Rename runbook -> skills in 5 toolsets.yaml fixtures - Delete tests/plugins/runbooks/ (old catalog tests) Known bug: some eval toolsets.yaml files may still reference old 'runbook' toolset name - needs grep for remaining occurrences. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Missed in Step 2 - this was causing eval tests to fail with 'Toolset runbook does not exist' since the toolset was renamed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Complete rename of "runbook" to "skill" in all user-facing and internal code: Prompt templates: - _runbook_instructions.jinja2 -> _skill_instructions.jinja2 - _runbooks_instructions.jinja2 -> _skills_instructions.jinja2 - All template content: runbook -> skill, fetch_runbook -> fetch_skill Tool rename: - fetch_runbook -> fetch_skill (tool name) - runbook_id -> skill_id (parameter) - <runbook> -> <skill> (XML tags in tool output) Python code: - PromptComponent.TIME_RUNBOOKS -> TIME_SKILLS - generate_runbooks_args -> generate_skills_args - runbook_catalog -> skill_catalog (context variables) - runbooks_enabled -> skills_enabled (template variables) - _should_enable_runbooks -> _should_enable_skills - RobustaRunbookInstruction -> RobustaSkillInstruction - get_runbook_catalog -> get_skill_catalog (DAL method) - get_runbook_content -> get_skill_content (DAL method) - _runbook_in_use -> _skill_in_use - Cleaned dead code from holmes/plugins/runbooks/__init__.py Tests and evals updated to match all renames. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Resolved conflicts: - tool_calling_llm.py: kept OAuth tool discovery from master + our skill rename - tools.py: kept both metadata field (ours) and oauth_tools field (master) No new runbook references introduced by master. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:d9db68f1
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:d9db68f1 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:d9db68f1
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:d9db68f1
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:d9db68f1
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:d9db68f1 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:d9db68f1
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:d9db68f1Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:d9db68f1 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:d9db68f1Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:d9db68f1 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:d9db68f1 |
Contributor
📂 Previous Runs📜 #5 · Run @ __69f55c5__ (#24991589472) — Apr 27, 11:16 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 69f55c5 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #4 · Run @ __08cc416__ (#24991233327) — Apr 27, 11:09 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 08cc416 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #3 · Run @ __f3ff678__ (#24990978032) — Apr 27, 11:02 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit f3ff678 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #2 · Run @ __fb72c61__ (#24953432514) — Apr 26, 09:40 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit fb72c61 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📜 #1 · Run @ __a4bc86e__ (#24951967852) — Apr 26, 08:20 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit a4bc86e on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit ac195f6 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: Success - 74 test/model combinations loaded Benchmark experiment:
No benchmark data available for comparison. Benchmark has no cost, total tokens, cached tokens data. Will appear after the next weekly benchmark run. Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals (applies to both automatic runs and
Examples: 🏷️ Valid tags
🤖 Valid models
Commands: CLI: |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces the runbook subsystem with a skills subsystem across the codebase: renames config fields and APIs, removes RunbookFetcher/catalog loading, adds a Skill loader and SkillsToolset/SkillsFetcher, and updates prompts, templates, tests, docs, and tooling metadata to use "skills" instead of "runbooks". Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant PromptBuilder as PromptBuilder
participant SkillLoader as SkillCatalogLoader
participant ToolExecutor as ToolExecutor
participant Supabase as SupabaseDAL
Client->>Server: send chat/request
Server->>PromptBuilder: build_chat_messages(skills=...)
PromptBuilder->>SkillLoader: ensure SkillCatalog (builtin + custom paths)
SkillLoader->>Supabase: optionally fetch remote skills
Supabase-->>SkillLoader: remote skill entries
SkillLoader-->>PromptBuilder: SkillCatalog
PromptBuilder->>ToolExecutor: include skill-aware tools (fetch_skill)
ToolExecutor->>SkillLoader: resolve skill by name/ID
SkillLoader->>Supabase: get_skill_content(id) [if remote]
Supabase-->>ToolExecutor: skill content
ToolExecutor-->>PromptBuilder: StructuredToolResult (content)
PromptBuilder-->>Server: assembled messages
Server-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/llm/fixtures/test_ask_holmes/162_get_skills/test_case.yaml (1)
52-53:⚠️ Potential issue | 🟡 MinorUpdate expected_output to use "skills" terminology.
The expected output validation text still references "runbook" but should use "skill" to align with the migration.
📝 Proposed fix
-# verifies that the runbook is pulled from robusta and also global instructions are read and followed +# verifies that the skill is pulled from robusta and also global instructions are read and followed expected_output: | - The runbook provides systematic debugging steps for signup service issues, emphasizing the need to check the payments service dependency first. The runbook should specifically mention checking Stripe API key validation in the payments service, as this is the root cause of the signup failures. Mentions contacting the interlock team for assistance. There is no mention of the capricorn team + The skill provides systematic debugging steps for signup service issues, emphasizing the need to check the payments service dependency first. The skill should specifically mention checking Stripe API key validation in the payments service, as this is the root cause of the signup failures. Mentions contacting the interlock team for assistance. There is no mention of the capricorn team🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/162_get_skills/test_case.yaml` around lines 52 - 53, Update the YAML expected_output text for the test case identifier "162_get_skills" by replacing "runbook" wording with "skill" (e.g., change "The runbook provides..." to "The skill provides...", "The runbook should specifically mention..." to "The skill should specifically mention...") while keeping the rest of the sentence intact; edit the value under the expected_output key so all occurrences of "runbook" are switched to "skill".tests/llm/fixtures/test_ask_holmes/96_no_matching_skill/test_case.yaml (1)
1-12:⚠️ Potential issue | 🟡 MinorTerminology drift between metadata and prompt/expected output.
The directory and tag are now
skills, but theuser_promptstill says "Use the custom-crd runbook" andexpected_output(lines 8–12) repeatedly references "runbook"/"runbooks". For consistency with the PR-wide rename, consider updating these strings to use "skill"/"skills" so the assertions and prompt match the new vocabulary the system is being primed with.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/96_no_matching_skill/test_case.yaml` around lines 1 - 12, Update the test fixture strings to match the PR-wide rename from "runbook(s)" to "skill(s)": change the user_prompt value from "Use the custom-crd runbook..." to "Use the custom-crd skill..." (or similar), and revise all occurrences in expected_output that mention "runbook" or "runbooks" to "skill" or "skills" so the prompt, tags, and assertions consistently use the "skills" terminology; target the user_prompt and expected_output fields in this fixture (test_ask_holmes/96_no_matching_skill/test_case.yaml) and keep the original meaning of each assertion.tests/llm/fixtures/test_ask_holmes/90_skill_basic_selection/test_case.yaml (1)
7-13:⚠️ Potential issue | 🟡 MinorExpected output still references "runbook" after rename to skills.
The test directory name is
90_skill_basic_selectionand the tag isskills, butexpected_outputstill mentions "Application Gateway troubleshooting runbook" and "as per the runbook instructions". Consider updating these to "skill" wording to stay consistent with the runbooks→skills migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/90_skill_basic_selection/test_case.yaml` around lines 7 - 13, The expected_output in test_case.yaml still uses "runbook" wording; update the strings in the expected_output block to use "skill" instead (e.g., change "Application Gateway troubleshooting runbook" to "Application Gateway troubleshooting skill" and "as per the runbook instructions" to "as per the skill instructions"), and ensure any other occurrences like "runbook instructions" or "runbook" are replaced with the corresponding "skill" phrasing to reflect the runbooks→skills migration (file: the expected_output field in the test case for 90_skill_basic_selection, tag 'skills').holmes/interactive.py (1)
2254-2262: 🛠️ Refactor suggestion | 🟠 MajorAdd an explicit type hint for
skillsinrun_interactive_loop.The changed parameter is currently untyped, which weakens mypy coverage on this API surface.
🛠️ Proposed refactor
@@ from holmes.version import check_version_async +from holmes.plugins.skills.skill_loader import SkillCatalog @@ def run_interactive_loop( @@ - skills=None, + skills: Optional[SkillCatalog] = None,As per coding guidelines, "Type hints are required throughout the codebase (mypy configuration in pyproject.toml)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/interactive.py` around lines 2254 - 2262, The parameter "skills" in run_interactive_loop is untyped; add an explicit type hint (e.g., change it to skills: Optional[Sequence[Skill]]) and update imports accordingly (import Sequence from typing and import or reference the Skill type from its definition). Modify the function signature in run_interactive_loop to use the new type and ensure any callers/assignments still satisfy Optional[Sequence[Skill]]; if Skill lives in another module, add the appropriate import (or use a forward-reference string "Skill") to keep mypy happy.
♻️ Duplicate comments (1)
holmes/plugins/prompts/_skills_instructions.jinja2 (1)
1-9:⚠️ Potential issue | 🔴 CriticalFilename inconsistency with includes — see comment in
base_user_prompt.jinja2.This new partial is named
_skills_instructions.jinja2(plural) whilebase_user_prompt.jinja2includes_skill_instructions.jinja2(singular). Pick one canonical name and apply it consistently across all referencing templates (base_user_prompt.jinja2,investigation_procedure.jinja2,_general_instructions.jinja2per the PR summary). The content of the block itself reads correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/prompts/_skills_instructions.jinja2` around lines 1 - 9, The partial filename is inconsistent: the new template is `_skills_instructions.jinja2` but `base_user_prompt.jinja2` (and other templates) include `_skill_instructions.jinja2`; pick one canonical name (e.g., `_skills_instructions.jinja2`) and update all include statements to match that name across templates mentioned in the PR (`base_user_prompt.jinja2`, `investigation_procedure.jinja2`, `_general_instructions.jinja2`), or rename the file to the singular form if you prefer that convention, ensuring the include targets and file name are identical and updating any import/include references accordingly.
🧹 Nitpick comments (15)
.gitignore (1)
173-173: Remove duplicate ignore rule for Claude skills path.Line 173 duplicates the exact ignore rule already present at Line 5. Keep only one entry to avoid config noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 173, Remove the duplicate ignore entry for ".claude/skills/dev-skills/": locate both occurrences of the exact rule ".claude/skills/dev-skills/" in the .gitignore and delete the redundant one so only a single ignore line for that path remains.tests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/toolsets.yaml (1)
4-4: Update the inline comment to reference “skills” instead of “runbook”.The current comment is stale after the rename.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/toolsets.yaml` at line 4, Update the inline comment next to the "internet:" entry in toolsets.yaml to mention "skills" instead of "runbook" (e.g., change "because runbook is disabled" to "because skills are disabled" or similar) so the comment reflects the renamed concept used by Holmes when fetching networking/dns_troubleshooting_instructions.md.tests/llm/fixtures/test_ask_holmes/90_skill_basic_selection/application-gateway-troubleshooting/SKILL.md (1)
8-8: Consider replacing “runbook” with “skill” in the overview text.Keeps terminology consistent with the migration and avoids mixed wording in fixtures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/90_skill_basic_selection/application-gateway-troubleshooting/SKILL.md` at line 8, Replace the term "runbook" with "skill" in the overview sentence that currently reads "This runbook helps troubleshoot common Application Gateway issues including 502 Bad Gateway errors." (search for the string "This runbook helps troubleshoot" or the token "runbook" in SKILL.md) to maintain consistent terminology with the migration; update the sentence to "This skill helps troubleshoot common Application Gateway issues including 502 Bad Gateway errors."holmes/plugins/toolsets/internet/internet.py (1)
181-181: Description wording is ambiguous after the rename."Fetch skills if they are present" reads awkwardly out of context —
fetch_webpagedoesn't fetch skills, it fetches whatever URL is given. Consider clarifying that this tool should be used to fetch a webpage referenced by a skill (e.g., "Use this to fetch any URL referenced by a skill before starting your investigation, when no more specific tool like Confluence applies."). Otherwise the LLM may misinterpret the instruction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/internet/internet.py` at line 181, Update the ambiguous description for the fetch_webpage tool (look for the description string associated with fetch_webpage in internet.py) so it no longer implies the tool "fetches skills"; instead state that it fetches any URL referenced by a skill or investigation and should be used when no more specific tool (e.g., Confluence) applies—for example: "Fetch any URL referenced by a skill before starting your investigation when no more specific tool like Confluence applies." Ensure the change replaces the current phrasing exactly in the description value for fetch_webpage.tests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/test_case.yaml (1)
1-2: Comments are now stale after the runbooks → skills rename.Lines 1–2 still reference "runbook"/"runbboks". Update them to match the new terminology so this fixture's intent stays clear.
📝 Proposed wording fix
-# This test is used to test the ability of HolmesGPT to find the relevant runbook for a given alert -# TODO: Add check for the tool calls that actually checked that the relevant runbboks were fetched and used +# This test is used to test the ability of HolmesGPT to find the relevant skill for a given alert +# TODO: Add check for the tool calls that actually checked that the relevant skills were fetched and used🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/test_case.yaml` around lines 1 - 2, Update the header comments in the fixture to use the new terminology: replace the singular "runbook" with "skill" in the first line and fix the misspelled "runbboks" to "skills" in the second line so both lines consistently reference "skill/skills" instead of "runbook/runbboks".holmes/plugins/runbooks/__init__.py (1)
1-37: Module path still saysrunbooks/while contents have been renamed to skills.The class, docstring, and PR-wide migration are now skills-oriented, but this file lives at
holmes/plugins/runbooks/__init__.py. New contributors will be confused findingRobustaSkillInstructionunder arunbooks/directory, and grep-driven refactors may miss it. Consider moving this module toholmes/plugins/skills/__init__.py(or similar) as part of this PR or as a documented follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/runbooks/__init__.py` around lines 1 - 37, The module name/package (runbooks) no longer matches its contents (skills): move the module out of the runbooks package into the skills package and update all imports and exports that reference RobustaSkillInstruction (and any top-level symbols in this file), adjust package __init__ exports, and fix any tests or code that import the old runbooks module so they now import from skills; ensure the class name RobustaSkillInstruction, its docstring, and the yaml dumper behavior remain unchanged during the move.server.py (1)
408-412: Follow-up action text still says "runbooks".This PR replaces runbooks with skills throughout, but the
articlesfollow-up still asks the LLM to "List the relevant runbooks and links" with notification text "Looking up and summarizing runbooks and links...". If the intent is a complete rename, update this string too; if "runbooks" is intentionally kept as user-facing terminology, ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server.py` around lines 408 - 412, Update the user-facing strings in the follow-up action with id="articles" (action_label="Articles") to reflect the rename from "runbooks" to "skills": change the prompt value ("List the relevant runbooks and links used. Write a short summary for each") and pre_action_notification_text ("Looking up and summarizing runbooks and links...") to use "skills" (or another intended term) so wording is consistent across the codebase.tests/llm/fixtures/test_ask_holmes/94_skill_transparency/database-troubleshooting/SKILL.md (1)
8-8: Body still refers to "this runbook".Minor doc inconsistency now that the file is a
SKILL.md. Optional cleanup:📝 Proposed wording tweak
-This runbook helps diagnose database connection timeout issues in Kubernetes applications. +This skill helps diagnose database connection timeout issues in Kubernetes applications.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/94_skill_transparency/database-troubleshooting/SKILL.md` at line 8, The body text uses the phrase "this runbook" which is inconsistent with the file now being SKILL.md; update the sentence "This runbook helps diagnose database connection timeout issues in Kubernetes applications." to use consistent terminology (e.g., "This skill helps diagnose database connection timeout issues in Kubernetes applications." or "This document helps diagnose...") so SKILL.md wording matches file purpose and header; search for the exact phrase "This runbook helps diagnose database connection timeout issues in Kubernetes applications." to locate and replace it.tests/llm/fixtures/test_ask_holmes/210_confluence_skill_fetch/test_case.yaml (1)
1-1: Stale comment terminology.The header still says "internal runbook tool" but the test now guards against
fetch_skill. Update for consistency:📝 Proposed wording tweak
-# Test: Fetch runbook from Confluence (must use Confluence toolset, not internal runbook tool) +# Test: Fetch runbook from Confluence (must use Confluence toolset, not internal skill tool)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/210_confluence_skill_fetch/test_case.yaml` at line 1, Update the test header comment to replace the outdated phrase "internal runbook tool" with the guarded symbol `fetch_skill` so it reads something like "Fetch runbook from Confluence (must use Confluence toolset, not fetch_skill)"; locate and edit the header string in the test fixture for the Confluence skill (the comment at the top of test_ask_holmes/210_confluence_skill_fetch/test_case.yaml) and make the wording consistent with the current guard.tests/llm/fixtures/test_ask_holmes/94_skill_transparency/test_case.yaml (1)
7-11: Stale "runbook" wording inexpected_output.The migration renames runbooks → skills, but the expected output assertion still reads "Uses database troubleshooting runbook". Consider updating the wording to reflect the new vocabulary so the assertion remains consistent with the new system and prompt outputs.
✏️ Suggested wording
expected_output: | - - Uses database troubleshooting runbook + - Uses database troubleshooting skill - Includes detailed "Investigation Steps Performed" section🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/fixtures/test_ask_holmes/94_skill_transparency/test_case.yaml` around lines 7 - 11, Update the stale wording in the test fixture's expected_output: locate the expected_output block in tests/llm/fixtures/test_ask_holmes/94_skill_transparency/test_case.yaml and replace "Uses database troubleshooting runbook" with wording that uses the new vocabulary (for example "Uses database troubleshooting skill") so the assertion matches the migrated runbooks→skills terminology; ensure the rest of the bulleted expectations remain unchanged and that the exact string in expected_output aligns with the prompt/system output for the skill-based flow.tests/plugins/toolsets/test_skills.py (2)
50-57: Consider also covering whitespace-onlyskill_id.The AI summary describes coverage for "empty/blank"
skill_id, but only""is exercised here. If the fetcher trims/strips, a" "case is worth adding to lock in that contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/toolsets/test_skills.py` around lines 50 - 57, The test test_SkillsFetcher_empty_id only asserts behavior for an empty string and misses the whitespace-only contract; add a new assertion (or extend the existing test) that calls SkillsFetcher(SkillsToolset())._invoke with {"skill_id": " "} (or similar whitespace) using create_mock_tool_invoke_context() and assert the returned result.status is StructuredToolResultStatus.ERROR to ensure whitespace-only IDs are treated like empty IDs by the SkillsFetcher._invoke handling.
1-13: Unusedosimport andTEST_SKILLS_PATH.
osand theTEST_SKILLS_PATHconstant are defined but never referenced in any of the tests below.♻️ Suggested cleanup
-import os - from holmes.core.tools import StructuredToolResultStatus from holmes.plugins.skills.skill_loader import Skill, SkillCatalog, SkillSource from holmes.plugins.toolsets.skills.skills_fetcher import ( SkillsFetcher, SkillsToolset, ) from tests.conftest import create_mock_tool_invoke_context - -TEST_SKILLS_PATH = os.path.join( - os.path.dirname(__file__), "fixtures", "skills" -)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/toolsets/test_skills.py` around lines 1 - 13, Remove the unused top-level import and constant: delete the import of os and the TEST_SKILLS_PATH constant declaration in test_skills.py (they are unused symbols `os` and `TEST_SKILLS_PATH`), leaving only the necessary imports from holmes and tests; ensure no other code references `TEST_SKILLS_PATH` before committing.holmes/core/toolset_manager.py (1)
121-127: Silent fallback when a path neither exists nor is a directory.For each
pincustom_skill_paths, the branch is decided solely byPath(p).is_dir(). If the user passes a typo or a non-existent path,is_dir()returnsFalseand the code silently usesos.path.dirname(os.path.abspath(str(p)))— i.e., the (possibly unrelated) parent directory of the typo'd path becomes an additional search path. Consider warning/erroring when the path doesn't exist, or at least logging the resolved search path for visibility.♻️ Possible adjustment
if self.custom_skill_paths: - additional_search_paths = [ - str(Path(p).resolve()) if Path(p).is_dir() else os.path.dirname(os.path.abspath(str(p))) - for p in self.custom_skill_paths - ] + additional_search_paths = [] + for p in self.custom_skill_paths: + path_obj = Path(p) + if path_obj.is_dir(): + additional_search_paths.append(str(path_obj.resolve())) + elif path_obj.is_file(): + additional_search_paths.append(str(path_obj.resolve().parent)) + else: + logging.warning(f"Custom skill path does not exist, skipping: {p}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/toolset_manager.py` around lines 121 - 127, The current construction of additional_search_paths silently treats non-existent custom_skill_paths as files by falling back to os.path.dirname(os.path.abspath(str(p))) when Path(p).is_dir() is False; instead, in the block building additional_search_paths (referencing self.custom_skill_paths, Path(p).is_dir(), and os.path.dirname/os.path.abspath), validate each p exists with Path(p).exists() and: if it is a directory use Path(p).resolve(), if it is an existing file use os.path.dirname(os.path.abspath(str(p))), otherwise log a warning or raise an error reporting the invalid path (include the original p and its resolved form) so typos/non-existent paths are not silently converted into unrelated parent directories.holmes/core/prompt.py (1)
208-209: Simplify theskills_enabledexpression.
SkillCatalogalways defines askillsfield, so thegetattr(..., "skills", True)fallback is dead — and usingTrueas the default is misleading (it would force-enable when the attribute is missing, which is the opposite of the intent). A direct check is clearer:♻️ Proposed simplification
- "skills_enabled": bool(skills and getattr(skills, "skills", True)) - and is_enabled(PromptComponent.TIME_SKILLS), + "skills_enabled": bool(skills and skills.skills) + and is_enabled(PromptComponent.TIME_SKILLS),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/prompt.py` around lines 208 - 209, Replace the convoluted expression used to compute skills_enabled with a direct check because SkillCatalog always has a skills field; change the expression to use a simple boolean check of skills combined with the existing feature flag check, e.g. set "skills_enabled": bool(skills) and is_enabled(PromptComponent.TIME_SKILLS) in the same place where the current expression is computed.holmes/plugins/skills/skill_loader.py (1)
137-141: MoveRobustaSkillInstructionimport to aTYPE_CHECKINGblock and drop the dead runtime import.Several problems on these lines:
- The annotation
instr: "RobustaSkillInstruction"is a forward reference but the name is never imported at module scope (Ruff F821 in static analysis).- The function-local
from holmes.plugins.runbooks import RobustaSkillInstructionis unused at runtime — the body only accesses attributes oninstr, never the class itself.# noqa: F811is the wrong rule code (F811 is "redefinition of unused"); the actual diagnostic here is F821.- Function-local imports violate the repo coding guideline: “ALWAYS place Python imports at the top of the file, not inside functions or methods.”
♻️ Proposed cleanup
if TYPE_CHECKING: from holmes.core.supabase_dal import SupabaseDal + from holmes.plugins.runbooks import RobustaSkillInstruction @@ def map_robusta_instruction_to_skill( instr: "RobustaSkillInstruction", ) -> Skill: """Convert a Supabase RobustaSkillInstruction into a Skill.""" - from holmes.plugins.runbooks import RobustaSkillInstruction # noqa: F811 - description = instr.titleAs per coding guidelines: "ALWAYS place Python imports at the top of the file, not inside functions or methods".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/skills/skill_loader.py` around lines 137 - 141, The function-local import of RobustaSkillInstruction in map_robusta_instruction_to_skill should be removed and the name imported only for type checking: add "from typing import TYPE_CHECKING" at module top and inside an "if TYPE_CHECKING:" block import "RobustaSkillInstruction" from holmes.plugins.runbooks; remove the in-function "from holmes.plugins.runbooks import RobustaSkillInstruction" and the "# noqa: F811" comment, keeping the parameter annotation as the forward-reference string "RobustaSkillInstruction" so static checkers (Ruff F821) are satisfied without a runtime import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/reference/http-api.md`:
- Line 124: Update the docs and the config parsing to avoid silently breaking
existing integrations: in docs/reference/http-api.md add a prominent migration
note calling out the renaming from time_runbooks -> time_skills and instruct
users to update any behavior_controls / ENABLED_PROMPTS entries; additionally
update the server/config parsing logic that reads
ENABLED_PROMPTS/behavior_controls to accept both keys (time_runbooks as
deprecated alias for time_skills) and emit a deprecation warning when
time_runbooks is present, so integrations keep working while users migrate.
In `@holmes/plugins/skills/skill_loader.py`:
- Around line 137-154: map_robusta_instruction_to_skill is converting
catalog-only RobustaSkillInstruction entries into Skill objects with empty
content (using instr.instruction or ""), which causes remote skills loaded by
get_skill_catalog() to be treated as fully-resolved and never fetched from DAL;
change the runtime behavior so catalog entries remain metadata-only and are
always resolved via the DAL in skills_fetcher._invoke by making
_find_skill/_format_skill_result treat SkillSource.REMOTE entries as pointers
(call dal.get_skill_content(skill.name) to populate content before returning
SUCCESS) rather than returning the empty content, and move the
RobustaSkillInstruction import in skill_loader.py out of the function and into
the top-level TYPE_CHECKING imports to avoid the inline import.
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Around line 139-149: The remote skill returned by _get_robusta_skill currently
returns raw YAML from RobustaSkillInstruction via skill_content.pretty(), so
update _get_robusta_skill to wrap that content in the same <skill> tag and LLM
instruction context used by _format_skill_result() (i.e., replicate the exact
wrapper and prompt lines used in _format_skill_result) before placing it into
the StructuredToolResult.data field; preserve the same StructuredToolResult
status, params, and metadata shape while ensuring the final data string matches
the local-skill format expected by the LLM.
- Around line 73-76: The endswith(".md") check in the skills_fetcher fallback is
dead/ misleading; remove the `not skill_id.endswith(".md")` condition from the
fallback branch that calls self._get_robusta_skill(skill_id, params) so the
DAL-based UUID-path fallback uses only the intended checks (self._dal and
self._dal.enabled), or if you intended to block path-like inputs instead add an
explicit validation step using normalize_skill_name/available_skills and a short
comment explaining the purpose; update the branch around `_get_robusta_skill`
and the surrounding comment accordingly to reflect the chosen behavior.
In `@tests/llm/fixtures/test_ask_holmes/162_get_skills/test_case.yaml`:
- Line 7: Update the inline comment that currently reads "simulates loading
runbooks and global instructions from a cluster environment" to use the new
terminology "skills" (e.g., "simulates loading skills and global instructions
from a cluster environment") so the YAML fixture comment reflects the migration;
locate the exact comment string in
tests/llm/fixtures/test_ask_holmes/162_get_skills/test_case.yaml and replace
"runbooks" with "skills".
In `@tests/llm/utils/mock_dal.py`:
- Around line 58-82: The fixture lookups in get_skill_catalog and
get_skill_content still hard-code the "runbook_" prefix (via calls to
_get_fixture_file_path with "runbook_catalog" and
f"runbook_content_{skill_id}"); either (A) add a concise inline comment above
both get_skill_catalog and get_skill_content explaining the prefix is
intentionally retained to match existing fixtures, or (B) update these calls to
use the new canonical prefix (e.g., "skill_catalog" and
f"skill_content_{skill_id}") and migrate/rename the corresponding fixture files
in the test fixtures set in one pass; ensure you update all uses of
_get_fixture_file_path in these methods and keep error handling/logging
unchanged.
In `@tests/test_skill_prompt.py`:
- Line 10: DummySkillCatalog defines a mutable class attribute skills = [True];
change it to an immutable type (e.g., skills = (True,) or frozenset({True})) to
avoid shared mutable state and RUF012; update the DummySkillCatalog class
definition to use the chosen immutable container so each test instance no longer
shares a mutable list.
---
Outside diff comments:
In `@holmes/interactive.py`:
- Around line 2254-2262: The parameter "skills" in run_interactive_loop is
untyped; add an explicit type hint (e.g., change it to skills:
Optional[Sequence[Skill]]) and update imports accordingly (import Sequence from
typing and import or reference the Skill type from its definition). Modify the
function signature in run_interactive_loop to use the new type and ensure any
callers/assignments still satisfy Optional[Sequence[Skill]]; if Skill lives in
another module, add the appropriate import (or use a forward-reference string
"Skill") to keep mypy happy.
In `@tests/llm/fixtures/test_ask_holmes/162_get_skills/test_case.yaml`:
- Around line 52-53: Update the YAML expected_output text for the test case
identifier "162_get_skills" by replacing "runbook" wording with "skill" (e.g.,
change "The runbook provides..." to "The skill provides...", "The runbook should
specifically mention..." to "The skill should specifically mention...") while
keeping the rest of the sentence intact; edit the value under the
expected_output key so all occurrences of "runbook" are switched to "skill".
In `@tests/llm/fixtures/test_ask_holmes/90_skill_basic_selection/test_case.yaml`:
- Around line 7-13: The expected_output in test_case.yaml still uses "runbook"
wording; update the strings in the expected_output block to use "skill" instead
(e.g., change "Application Gateway troubleshooting runbook" to "Application
Gateway troubleshooting skill" and "as per the runbook instructions" to "as per
the skill instructions"), and ensure any other occurrences like "runbook
instructions" or "runbook" are replaced with the corresponding "skill" phrasing
to reflect the runbooks→skills migration (file: the expected_output field in the
test case for 90_skill_basic_selection, tag 'skills').
In `@tests/llm/fixtures/test_ask_holmes/96_no_matching_skill/test_case.yaml`:
- Around line 1-12: Update the test fixture strings to match the PR-wide rename
from "runbook(s)" to "skill(s)": change the user_prompt value from "Use the
custom-crd runbook..." to "Use the custom-crd skill..." (or similar), and revise
all occurrences in expected_output that mention "runbook" or "runbooks" to
"skill" or "skills" so the prompt, tags, and assertions consistently use the
"skills" terminology; target the user_prompt and expected_output fields in this
fixture (test_ask_holmes/96_no_matching_skill/test_case.yaml) and keep the
original meaning of each assertion.
---
Duplicate comments:
In `@holmes/plugins/prompts/_skills_instructions.jinja2`:
- Around line 1-9: The partial filename is inconsistent: the new template is
`_skills_instructions.jinja2` but `base_user_prompt.jinja2` (and other
templates) include `_skill_instructions.jinja2`; pick one canonical name (e.g.,
`_skills_instructions.jinja2`) and update all include statements to match that
name across templates mentioned in the PR (`base_user_prompt.jinja2`,
`investigation_procedure.jinja2`, `_general_instructions.jinja2`), or rename the
file to the singular form if you prefer that convention, ensuring the include
targets and file name are identical and updating any import/include references
accordingly.
---
Nitpick comments:
In @.gitignore:
- Line 173: Remove the duplicate ignore entry for ".claude/skills/dev-skills/":
locate both occurrences of the exact rule ".claude/skills/dev-skills/" in the
.gitignore and delete the redundant one so only a single ignore line for that
path remains.
In `@holmes/core/prompt.py`:
- Around line 208-209: Replace the convoluted expression used to compute
skills_enabled with a direct check because SkillCatalog always has a skills
field; change the expression to use a simple boolean check of skills combined
with the existing feature flag check, e.g. set "skills_enabled": bool(skills)
and is_enabled(PromptComponent.TIME_SKILLS) in the same place where the current
expression is computed.
In `@holmes/core/toolset_manager.py`:
- Around line 121-127: The current construction of additional_search_paths
silently treats non-existent custom_skill_paths as files by falling back to
os.path.dirname(os.path.abspath(str(p))) when Path(p).is_dir() is False;
instead, in the block building additional_search_paths (referencing
self.custom_skill_paths, Path(p).is_dir(), and os.path.dirname/os.path.abspath),
validate each p exists with Path(p).exists() and: if it is a directory use
Path(p).resolve(), if it is an existing file use
os.path.dirname(os.path.abspath(str(p))), otherwise log a warning or raise an
error reporting the invalid path (include the original p and its resolved form)
so typos/non-existent paths are not silently converted into unrelated parent
directories.
In `@holmes/plugins/runbooks/__init__.py`:
- Around line 1-37: The module name/package (runbooks) no longer matches its
contents (skills): move the module out of the runbooks package into the skills
package and update all imports and exports that reference
RobustaSkillInstruction (and any top-level symbols in this file), adjust package
__init__ exports, and fix any tests or code that import the old runbooks module
so they now import from skills; ensure the class name RobustaSkillInstruction,
its docstring, and the yaml dumper behavior remain unchanged during the move.
In `@holmes/plugins/skills/skill_loader.py`:
- Around line 137-141: The function-local import of RobustaSkillInstruction in
map_robusta_instruction_to_skill should be removed and the name imported only
for type checking: add "from typing import TYPE_CHECKING" at module top and
inside an "if TYPE_CHECKING:" block import "RobustaSkillInstruction" from
holmes.plugins.runbooks; remove the in-function "from holmes.plugins.runbooks
import RobustaSkillInstruction" and the "# noqa: F811" comment, keeping the
parameter annotation as the forward-reference string "RobustaSkillInstruction"
so static checkers (Ruff F821) are satisfied without a runtime import.
In `@holmes/plugins/toolsets/internet/internet.py`:
- Line 181: Update the ambiguous description for the fetch_webpage tool (look
for the description string associated with fetch_webpage in internet.py) so it
no longer implies the tool "fetches skills"; instead state that it fetches any
URL referenced by a skill or investigation and should be used when no more
specific tool (e.g., Confluence) applies—for example: "Fetch any URL referenced
by a skill before starting your investigation when no more specific tool like
Confluence applies." Ensure the change replaces the current phrasing exactly in
the description value for fetch_webpage.
In `@server.py`:
- Around line 408-412: Update the user-facing strings in the follow-up action
with id="articles" (action_label="Articles") to reflect the rename from
"runbooks" to "skills": change the prompt value ("List the relevant runbooks and
links used. Write a short summary for each") and pre_action_notification_text
("Looking up and summarizing runbooks and links...") to use "skills" (or another
intended term) so wording is consistent across the codebase.
In
`@tests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/test_case.yaml`:
- Around line 1-2: Update the header comments in the fixture to use the new
terminology: replace the singular "runbook" with "skill" in the first line and
fix the misspelled "runbboks" to "skills" in the second line so both lines
consistently reference "skill/skills" instead of "runbook/runbboks".
In
`@tests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/toolsets.yaml`:
- Line 4: Update the inline comment next to the "internet:" entry in
toolsets.yaml to mention "skills" instead of "runbook" (e.g., change "because
runbook is disabled" to "because skills are disabled" or similar) so the comment
reflects the renamed concept used by Holmes when fetching
networking/dns_troubleshooting_instructions.md.
In
`@tests/llm/fixtures/test_ask_holmes/210_confluence_skill_fetch/test_case.yaml`:
- Line 1: Update the test header comment to replace the outdated phrase
"internal runbook tool" with the guarded symbol `fetch_skill` so it reads
something like "Fetch runbook from Confluence (must use Confluence toolset, not
fetch_skill)"; locate and edit the header string in the test fixture for the
Confluence skill (the comment at the top of
test_ask_holmes/210_confluence_skill_fetch/test_case.yaml) and make the wording
consistent with the current guard.
In
`@tests/llm/fixtures/test_ask_holmes/90_skill_basic_selection/application-gateway-troubleshooting/SKILL.md`:
- Line 8: Replace the term "runbook" with "skill" in the overview sentence that
currently reads "This runbook helps troubleshoot common Application Gateway
issues including 502 Bad Gateway errors." (search for the string "This runbook
helps troubleshoot" or the token "runbook" in SKILL.md) to maintain consistent
terminology with the migration; update the sentence to "This skill helps
troubleshoot common Application Gateway issues including 502 Bad Gateway
errors."
In
`@tests/llm/fixtures/test_ask_holmes/94_skill_transparency/database-troubleshooting/SKILL.md`:
- Line 8: The body text uses the phrase "this runbook" which is inconsistent
with the file now being SKILL.md; update the sentence "This runbook helps
diagnose database connection timeout issues in Kubernetes applications." to use
consistent terminology (e.g., "This skill helps diagnose database connection
timeout issues in Kubernetes applications." or "This document helps
diagnose...") so SKILL.md wording matches file purpose and header; search for
the exact phrase "This runbook helps diagnose database connection timeout issues
in Kubernetes applications." to locate and replace it.
In `@tests/llm/fixtures/test_ask_holmes/94_skill_transparency/test_case.yaml`:
- Around line 7-11: Update the stale wording in the test fixture's
expected_output: locate the expected_output block in
tests/llm/fixtures/test_ask_holmes/94_skill_transparency/test_case.yaml and
replace "Uses database troubleshooting runbook" with wording that uses the new
vocabulary (for example "Uses database troubleshooting skill") so the assertion
matches the migrated runbooks→skills terminology; ensure the rest of the
bulleted expectations remain unchanged and that the exact string in
expected_output aligns with the prompt/system output for the skill-based flow.
In `@tests/plugins/toolsets/test_skills.py`:
- Around line 50-57: The test test_SkillsFetcher_empty_id only asserts behavior
for an empty string and misses the whitespace-only contract; add a new assertion
(or extend the existing test) that calls SkillsFetcher(SkillsToolset())._invoke
with {"skill_id": " "} (or similar whitespace) using
create_mock_tool_invoke_context() and assert the returned result.status is
StructuredToolResultStatus.ERROR to ensure whitespace-only IDs are treated like
empty IDs by the SkillsFetcher._invoke handling.
- Around line 1-13: Remove the unused top-level import and constant: delete the
import of os and the TEST_SKILLS_PATH constant declaration in test_skills.py
(they are unused symbols `os` and `TEST_SKILLS_PATH`), leaving only the
necessary imports from holmes and tests; ensure no other code references
`TEST_SKILLS_PATH` before committing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 124e299d-0ff3-4030-b209-eb6f0a3bd85a
📒 Files selected for processing (127)
.gitignoredocs/reference/http-api.mdholmes/config.pyholmes/core/conversations.pyholmes/core/prompt.pyholmes/core/supabase_dal.pyholmes/core/tool_calling_llm.pyholmes/core/tools.pyholmes/core/tools_utils/tool_executor.pyholmes/core/toolset_manager.pyholmes/interactive.pyholmes/main.pyholmes/plugins/prompts/_general_instructions.jinja2holmes/plugins/prompts/_runbook_instructions.jinja2holmes/plugins/prompts/_runbooks_instructions.jinja2holmes/plugins/prompts/_skill_instructions.jinja2holmes/plugins/prompts/_skills_instructions.jinja2holmes/plugins/prompts/base_user_prompt.jinja2holmes/plugins/prompts/investigation_procedure.jinja2holmes/plugins/runbooks/__init__.pyholmes/plugins/skills/__init__.pyholmes/plugins/skills/builtin/__init__.pyholmes/plugins/skills/skill_loader.pyholmes/plugins/toolsets/__init__.pyholmes/plugins/toolsets/internet/internet.pyholmes/plugins/toolsets/investigator/investigator_instructions.jinja2holmes/plugins/toolsets/runbook/runbook_fetcher.pyholmes/plugins/toolsets/skills/__init__.pyholmes/plugins/toolsets/skills/skills_fetcher.pyholmes/utils/global_instructions.pypyproject.tomlserver.pytests/config_class/test_custom_runbook_catalogs.pytests/config_class/test_custom_skill_paths.pytests/core/test_prompt.pytests/core/test_toolset_manager.pytests/llm/fixtures/test_ask_holmes/104b_postgres_missing_index_pgstat/postgres-performance/SKILL.mdtests/llm/fixtures/test_ask_holmes/104b_postgres_missing_index_pgstat/test_case.yamltests/llm/fixtures/test_ask_holmes/158_slack_chat_correct_date/toolsets.yamltests/llm/fixtures/test_ask_holmes/162_get_skills/app/payments-deployment.yamltests/llm/fixtures/test_ask_holmes/162_get_skills/app/secrets.yamltests/llm/fixtures/test_ask_holmes/162_get_skills/app/signup-deployment.yamltests/llm/fixtures/test_ask_holmes/162_get_skills/global_instructions.jsontests/llm/fixtures/test_ask_holmes/162_get_skills/issue_data.jsontests/llm/fixtures/test_ask_holmes/162_get_skills/resource_instructions.jsontests/llm/fixtures/test_ask_holmes/162_get_skills/runbook_catalog.jsontests/llm/fixtures/test_ask_holmes/162_get_skills/runbook_content_40296b5c-2cb5-41df-b1f5-93441f894c44.jsontests/llm/fixtures/test_ask_holmes/162_get_skills/runbook_content_8fe8e24d-6b53-47a6-92a5-b389938fa823.jsontests/llm/fixtures/test_ask_holmes/162_get_skills/runbook_content_b2ffd311-f339-46a2-b379-1e53eb0bc1ed.jsontests/llm/fixtures/test_ask_holmes/162_get_skills/test_case.yamltests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/issue_data.jsontests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/pod_not_ready.yamltests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/runbook_catalog.jsontests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/runbook_content_40296b5c-2cb5-41df-b1f5-93441f894c44.jsontests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/runbook_content_8fe8e24d-6b53-47a6-92a5-b389938fa823.jsontests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/runbook_content_b2ffd311-f339-46a2-b379-1e53eb0bc1ed.jsontests/llm/fixtures/test_ask_holmes/165_alert_with_multiple_skills/test_case.yamltests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/backend.yamltests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/frontend.yamltests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/manifest.yamltests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/test_case.yamltests/llm/fixtures/test_ask_holmes/176_network_policy_blocking_traffic_no_skills/toolsets.yamltests/llm/fixtures/test_ask_holmes/197_bash_secrets_denied/test_case.yamltests/llm/fixtures/test_ask_holmes/208_confluence_page_fetch/test_case.yamltests/llm/fixtures/test_ask_holmes/209_confluence_url_lookup/test_case.yamltests/llm/fixtures/test_ask_holmes/210_confluence_skill_fetch/test_case.yamltests/llm/fixtures/test_ask_holmes/210_confluence_skill_fetch/toolsets.yamltests/llm/fixtures/test_ask_holmes/232_newline_in_tool_params/test_case.yamltests/llm/fixtures/test_ask_holmes/256_hello_token_limit/test_case.yamltests/llm/fixtures/test_ask_holmes/25_misconfigured_ingress_class/toolsets.yamltests/llm/fixtures/test_ask_holmes/42_dns_issues_result_new_tools/test_case.yamltests/llm/fixtures/test_ask_holmes/42_dns_issues_result_new_tools_no_runbook/test_case.yamltests/llm/fixtures/test_ask_holmes/42_dns_issues_result_old_tools/test_case.yamltests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_new_all_tools/test_case.yamltests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_new_tools/test_case.yamltests/llm/fixtures/test_ask_holmes/42_dns_issues_steps_old_tools/test_case.yamltests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/README.mdtests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/accounting-app/Dockerfiletests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/accounting-app/app.pytests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/accounting-app/build_and_publish.shtests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/accounting-app/requirements.txttests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/build_and_publish_all.shtests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/finance-app/Dockerfiletests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/finance-app/app.pytests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/finance-app/build_and_publish.shtests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/finance-app/requirements.txttests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/invoices-app/Dockerfiletests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/invoices-app/app.pytests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/invoices-app/build_and_publish.shtests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/invoices-app/requirements.txttests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/kafka-manifest.yamltests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/orders-app/Dockerfiletests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/orders-app/app.pytests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/orders-app/build_and_publish.shtests/llm/fixtures/test_ask_holmes/55_kafka_skill/app/orders-app/requirements.txttests/llm/fixtures/test_ask_holmes/55_kafka_skill/kafka_lag_instructions.mdtests/llm/fixtures/test_ask_holmes/55_kafka_skill/test_case.yamltests/llm/fixtures/test_ask_holmes/55_kafka_skill/toolsets.yamltests/llm/fixtures/test_ask_holmes/57_cluster_name_confusion/toolsets.yamltests/llm/fixtures/test_ask_holmes/89_skill_missing_cloudwatch/lambda-performance-troubleshooting/SKILL.mdtests/llm/fixtures/test_ask_holmes/89_skill_missing_cloudwatch/test_case.yamltests/llm/fixtures/test_ask_holmes/89_skill_missing_cloudwatch/toolsets.yamltests/llm/fixtures/test_ask_holmes/90_skill_basic_selection/application-gateway-troubleshooting/SKILL.mdtests/llm/fixtures/test_ask_holmes/90_skill_basic_selection/test_case.yamltests/llm/fixtures/test_ask_holmes/94_skill_transparency/database-troubleshooting/SKILL.mdtests/llm/fixtures/test_ask_holmes/94_skill_transparency/manifest.yamltests/llm/fixtures/test_ask_holmes/94_skill_transparency/test_case.yamltests/llm/fixtures/test_ask_holmes/95_skill_memory_leak_detection/manifest.yamltests/llm/fixtures/test_ask_holmes/95_skill_memory_leak_detection/python-memory-troubleshooting/SKILL.mdtests/llm/fixtures/test_ask_holmes/95_skill_memory_leak_detection/test_case.yamltests/llm/fixtures/test_ask_holmes/96_no_matching_skill/manifest.yamltests/llm/fixtures/test_ask_holmes/96_no_matching_skill/service-troubleshooting/SKILL.mdtests/llm/fixtures/test_ask_holmes/96_no_matching_skill/test_case.yamltests/llm/test_ask_holmes.pytests/llm/utils/braintrust.pytests/llm/utils/default_toolsets.yamltests/llm/utils/mock_dal.pytests/llm/utils/test_case_utils.pytests/llm/utils/test_toolset.pytests/plugins/runbooks/test_catalog.pytests/plugins/toolsets/fixtures/skills/test_runbook.mdtests/plugins/toolsets/test_runbook.pytests/plugins/toolsets/test_skills.pytests/test_cache.pytests/test_interactive.pytests/test_scheduled_prompts.pytests/test_skill_prompt.py
💤 Files with no reviewable changes (7)
- holmes/plugins/prompts/_runbooks_instructions.jinja2
- tests/llm/fixtures/test_ask_holmes/104b_postgres_missing_index_pgstat/test_case.yaml
- holmes/plugins/prompts/_runbook_instructions.jinja2
- tests/plugins/toolsets/test_runbook.py
- tests/config_class/test_custom_runbook_catalogs.py
- tests/plugins/runbooks/test_catalog.py
- holmes/plugins/toolsets/runbook/runbook_fetcher.py
Critical: Remote skills from Supabase catalog had empty content, causing fetch_skill to return empty <skill> blocks instead of fetching actual content from DAL. Fixed by routing REMOTE skills through _get_robusta_skill which calls dal.get_skill_content(). Major: Remote skill content from Supabase was returned as raw YAML without the <skill> tag wrapping and LLM instructions that local skills have. Fixed by wrapping remote content through the same _format_skill_result path. Minor fixes: - RobustaRunbookInstruction -> RobustaSkillInstruction - get_runbook_catalog -> get_skill_catalog (DAL method) - get_runbook_content -> get_skill_content (DAL method) - DummySkillCatalog.skills: mutable list -> tuple - Updated stale comment in 162_get_skills fixture - Added clarifying comment in mock_dal.py about fixture file prefix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Accidentally committed in previous commit. This is a local Claude Code config file that shouldn't be tracked. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
…skills Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
holmes/plugins/toolsets/skills/skills_fetcher.py (3)
26-26: UseField(default_factory=list)for the Pydantic field.Ruff RUF012 flags the mutable default. Pydantic v2 actually copies model-field defaults safely, so this is not a real bug, but switching to
Field(default_factory=list)keeps Ruff quiet and makes intent explicit. Same is also unnecessary becauseavailable_skillsis always passed viasuper().__init__(...)in the constructor; consider dropping the class-level default entirely and just declaring the type.♻️ Suggested change
-from typing import List, Optional +from typing import List, Optional + +from pydantic import Field @@ - available_skills: List[str] = [] + available_skills: List[str] = Field(default_factory=list)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` at line 26, Replace the mutable class-level default for the Pydantic field available_skills with an explicit Field(default_factory=list) (import Field from pydantic) or remove the class-level default entirely and only declare the type if the constructor always supplies it; update the declaration of available_skills to use Field(default_factory=list) to satisfy Ruff RUF012 and make intent explicit (look for the available_skills field in the SkillsFetcher model/class and adjust the import/annotation accordingly).
165-166: Minor: prefer{e!s}overstr(e)(RUF010).A small style nit flagged by Ruff. The blind
except Exceptionitself is reasonable here since it's guarding an external Supabase call.♻️ Suggested change
- except Exception as e: - err_msg = f"Failed to fetch skill with UUID '{link}': {str(e)}" + except Exception as e: + err_msg = f"Failed to fetch skill with UUID '{link}': {e!s}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` around lines 165 - 166, In the except block that builds err_msg (currently "err_msg = f\"Failed to fetch skill with UUID '{link}': {str(e)}\"") change the f-string to use the !s conversion for the exception (e.g., {e!s}) instead of calling str(e); update the err_msg assignment in the same except clause in skills_fetcher.py so it becomes: err_msg = f"Failed to fetch skill with UUID '{link}': {e!s}".
44-44: Inconsistent terminology across description, parameter, and error strings.The tool advertises a
skill_idparameter ("either a UUID or a skill name"), but the description and error strings interchange "skill link", "skill path", and "skill_id". This shows up in the LLM-visible description (line 44 "by skill link"), the empty-input error (line 62 "Skill link cannot be empty. Please provide a valid skill path."), the no-DAL error (line 174), and the one-liner (line 184 uses the localpathvariable forskill_id). Standardizing on "skill_id" (or "skill") across these strings reduces LLM confusion and matches the actual parameter name.Also applies to: 47-47, 62-62, 174-174, 183-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` at line 44, Standardize all LLM-visible terminology to use "skill_id" (or "skill") instead of mixed terms like "skill link" or "skill path": update the tool description string that currently says "by skill link" to reference "by skill_id", change the empty-input error ("Skill link cannot be empty...") to "Skill_id cannot be empty. Please provide a valid skill_id.", update the no-DAL error to use "skill_id", and ensure the one-liner that currently uses the local variable path maps/renames it to skill_id (or uses skill_id when building the response) so parameter names and error messages consistently reference skill_id across the functions and messages in skills_fetcher.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Around line 173-180: The error message in the no-DAL branch of
skills_fetcher.py is misleading because it assumes the link is a UUID; update
the err_msg to correctly reflect that a remote skill (use the variable link)
requires the data access layer (DAL) which is not enabled, e.g. "Remote skill
'<link>' requires the data access layer (DAL), which is not enabled."; keep the
same control flow and return
StructuredToolResult(status=StructuredToolResultStatus.ERROR, error=err_msg,
params=params) so callers receive the clearer message.
---
Nitpick comments:
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Line 26: Replace the mutable class-level default for the Pydantic field
available_skills with an explicit Field(default_factory=list) (import Field from
pydantic) or remove the class-level default entirely and only declare the type
if the constructor always supplies it; update the declaration of
available_skills to use Field(default_factory=list) to satisfy Ruff RUF012 and
make intent explicit (look for the available_skills field in the SkillsFetcher
model/class and adjust the import/annotation accordingly).
- Around line 165-166: In the except block that builds err_msg (currently
"err_msg = f\"Failed to fetch skill with UUID '{link}': {str(e)}\"") change the
f-string to use the !s conversion for the exception (e.g., {e!s}) instead of
calling str(e); update the err_msg assignment in the same except clause in
skills_fetcher.py so it becomes: err_msg = f"Failed to fetch skill with UUID
'{link}': {e!s}".
- Line 44: Standardize all LLM-visible terminology to use "skill_id" (or
"skill") instead of mixed terms like "skill link" or "skill path": update the
tool description string that currently says "by skill link" to reference "by
skill_id", change the empty-input error ("Skill link cannot be empty...") to
"Skill_id cannot be empty. Please provide a valid skill_id.", update the no-DAL
error to use "skill_id", and ensure the one-liner that currently uses the local
variable path maps/renames it to skill_id (or uses skill_id when building the
response) so parameter names and error messages consistently reference skill_id
across the functions and messages in skills_fetcher.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecb70fbc-6c7d-47e4-9aa2-c7270960be2e
📒 Files selected for processing (6)
.gitignoreholmes/plugins/toolsets/skills/skills_fetcher.pytests/core/test_prompt.pytests/llm/fixtures/test_ask_holmes/162_get_skills/test_case.yamltests/llm/utils/mock_dal.pytests/test_skill_prompt.py
✅ Files skipped from review due to trivial changes (1)
- tests/llm/fixtures/test_ask_holmes/162_get_skills/test_case.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitignore
- tests/test_skill_prompt.py
…bing - Add SkillsConfig with local_skills_path field for toolset-level config - Restore custom_skill_paths on Config and ToolsetManager for backwards compat - Both paths merge in SkillsToolset: top-level custom_skill_paths + toolsets.skills.config.local_skills_path - Remove unnecessary _deprecated_mappings for additional_search_paths (was never user-facing) - Restore config tests for custom_skill_paths - Remove obsolete toolset_manager skill path tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
holmes/plugins/toolsets/skills/skills_fetcher.py (1)
186-193:⚠️ Potential issue | 🟡 MinorMisleading error wording in the no-DAL branch.
This branch is also reached from line 92–93 when the catalog matched a REMOTE skill but DAL is disabled —
linkis a skill name in that path, not a UUID. Reword to remove the UUID assumption, e.g.:✏️ Proposed wording
- err_msg = "Skill link appears to be a UUID, but no remote data access layer (dal) is enabled." + err_msg = ( + f"Cannot fetch remote skill '{link}': the data access layer (DAL) is not enabled." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` around lines 186 - 193, The error message in the no-DAL branch (where err_msg is set before returning a StructuredToolResult with StructuredToolResultStatus.ERROR) wrongly assumes the link is a UUID; change the err_msg to a neutral message that does not assume UUIDs (e.g., indicate the skill/link appears to be remote or requires a remote data access layer but DAL is disabled) and keep the rest of the return values (StructuredToolResult, error=err_msg, params) unchanged so callers still receive the same error type and params.
🧹 Nitpick comments (2)
holmes/plugins/toolsets/skills/skills_fetcher.py (2)
178-185: Narrow the exception and use the f-string conversion flag.Catching
Exceptionhere is overly broad and Ruff flagsstr(e)(RUF010). Per the coding guideline to surface detailed error context for LLM self-correction, the message is fine but consider letting truly unexpected errors propagate while keeping the wrapping for known DAL/transport failures. Minimal cleanup:♻️ Proposed cleanup
- except Exception as e: - err_msg = f"Failed to fetch skill with UUID '{link}': {str(e)}" - logging.error(err_msg) + except Exception as e: # noqa: BLE001 - DAL/network failures are best-effort + err_msg = f"Failed to fetch skill with UUID '{link}': {e!s}" + logging.exception(err_msg) return StructuredToolResult( status=StructuredToolResultStatus.ERROR, error=err_msg, params=params, )Also note
linkmay be a name (not a UUID) when reached from the catalog-REMOTE path — see the related comment on the no-DAL branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` around lines 178 - 185, The current broad "except Exception as e" in the skill fetch path should be narrowed to only catch expected DAL/transport errors (e.g., the project's DAL/transport exception classes such as DalError/TransportError/HTTPError) and re-raise any other unexpected exceptions so they can propagate; change the error message to use the f-string conversion flag (e.g., {e!r}) to preserve full error detail, and adjust the message to avoid assuming 'link' is a UUID (it may be a name on the catalog-REMOTE path) when constructing err_msg in the except block surrounding the skill fetch logic in skills_fetcher.py.
37-41: UseField(default_factory=list)instead of mutable default.The class attribute
available_skills: List[str] = []violates RUF012. SinceToolis a PydanticBaseModel, even though__init__reinitializes the field viasuper().__init__(available_skills=...), the class-level default remains a shared mutable object. Define it asavailable_skills: List[str] = Field(default_factory=list)instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` around lines 37 - 41, The class-level mutable default on SkillsFetcher.available_skills must be replaced with a Pydantic safe factory: change the annotation from available_skills: List[str] = [] to use Field(default_factory=list) so the Tool (Pydantic BaseModel) does not share a single list across instances; update the import to ensure Field is available and keep other class attributes (_skill_catalog, _dal) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Around line 203-236: SkillsToolset currently reads self.config in __init__ and
calls load_skill_catalog there, but config is applied later via override_with(),
and the isinstance(self.config, dict) check is wrong; move the skill-catalog
loading and tool registration into prerequisites_callable(self, config:
dict[str, Any]) instead. In prerequisites_callable validate the incoming config
against SkillsConfig (or the appropriate Pydantic model), merge
validated.local_skills_path with additional_search_paths to build
all_skill_paths, call load_skill_catalog(dal=dal,
custom_skill_paths=all_skill_paths or None), and then set self.tools =
[SkillsFetcher(self, skill_catalog=skill_catalog, dal=dal)]; remove catalog
loading from __init__ and rely on prerequisites_callable for health checks and
config-aware setup.
---
Duplicate comments:
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Around line 186-193: The error message in the no-DAL branch (where err_msg is
set before returning a StructuredToolResult with
StructuredToolResultStatus.ERROR) wrongly assumes the link is a UUID; change the
err_msg to a neutral message that does not assume UUIDs (e.g., indicate the
skill/link appears to be remote or requires a remote data access layer but DAL
is disabled) and keep the rest of the return values (StructuredToolResult,
error=err_msg, params) unchanged so callers still receive the same error type
and params.
---
Nitpick comments:
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Around line 178-185: The current broad "except Exception as e" in the skill
fetch path should be narrowed to only catch expected DAL/transport errors (e.g.,
the project's DAL/transport exception classes such as
DalError/TransportError/HTTPError) and re-raise any other unexpected exceptions
so they can propagate; change the error message to use the f-string conversion
flag (e.g., {e!r}) to preserve full error detail, and adjust the message to
avoid assuming 'link' is a UUID (it may be a name on the catalog-REMOTE path)
when constructing err_msg in the except block surrounding the skill fetch logic
in skills_fetcher.py.
- Around line 37-41: The class-level mutable default on
SkillsFetcher.available_skills must be replaced with a Pydantic safe factory:
change the annotation from available_skills: List[str] = [] to use
Field(default_factory=list) so the Tool (Pydantic BaseModel) does not share a
single list across instances; update the import to ensure Field is available and
keep other class attributes (_skill_catalog, _dal) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 099b76db-42b9-4c58-93c4-4437998069a3
📒 Files selected for processing (3)
holmes/plugins/toolsets/skills/skills_fetcher.pytests/config_class/test_custom_skill_paths.pytests/core/test_toolset_manager.py
💤 Files with no reviewable changes (1)
- tests/core/test_toolset_manager.py
✅ Files skipped from review due to trivial changes (1)
- tests/config_class/test_custom_skill_paths.py
…docs - Remove allowed_restricted_tools from Skill model, skill_loader, skills_fetcher — was added prematurely for Step 3 (per-skill restricted tool gating) which hasn't been implemented yet - Remove metadata field from StructuredToolResult (same reason) - Add docs/reference/skills.md with migration guide from runbooks, SKILL.md format, and configuration instructions - Migrate CLAUDE.md and README.md from runbooks/ to skills/ with minimal changes (word rename + updated paths/format) - Remove old holmes/plugins/runbooks/ module entirely - Update docs nav: Runbooks -> Skills Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/supabase_dal.py (1)
642-664:⚠️ Potential issue | 🟡 MinorPossible
AttributeErrorifrunbookfield is missing/null.
row.get("runbook").get("instructions")chains.get()calls assumingrow["runbook"]is always a dict. If the row hasrunbook = Noneor the key is absent, this raisesAttributeError(and unlikeget_skill_catalogthis method does not wrap the body in a try/except, so the caller inskills_fetcher._get_robusta_skillwill surface the exception). Add a guard or wrap in try/except likeget_skill_catalogdoes.row = res.data[0] id = row.get("runbook_id") symptom = row.get("symptoms") title = row.get("subject_name") - raw_instruction = row.get("runbook").get("instructions") + runbook_blob = row.get("runbook") or {} + raw_instruction = runbook_blob.get("instructions")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/supabase_dal.py` around lines 642 - 664, The code assumes row.get("runbook") is a dict and calls .get("instructions") directly which raises AttributeError if runbook is None or missing; change to first fetch runbook = row.get("runbook") and then set raw_instruction = runbook.get("instructions") if runbook else None, or wrap the whole extraction in a try/except like get_skill_catalog to catch AttributeError/TypeError, log a descriptive error including skill_id and the row, and handle by setting instruction = None or str(raw_instruction) / returning None as appropriate so the caller (skills_fetcher._get_robusta_skill) doesn't crash.
♻️ Duplicate comments (2)
holmes/plugins/toolsets/skills/skills_fetcher.py (2)
186-208:⚠️ Potential issue | 🟠 Major
SkillsToolsetstill loads the catalog in__init__, before user config is applied.The HolmesGPT toolset lifecycle applies user config (
local_skills_pathetc.) viaoverride_with()after instantiation, so anything depending onself.configcannot run in__init__. As a result,additional_search_pathsis the only knob currently respected and any config-driven skill paths are ignored. Move theload_skill_catalog(...)call andSkillsFetcherregistration intoprerequisites_callable(config)(validating the dict against aSkillsConfigPydantic class, then merging paths and building the catalog), matching the pattern used byServiceNow/RabbitMQ/NewRelic/Prometheus/MongoDBtoolsets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` around lines 186 - 208, The SkillsToolset currently calls load_skill_catalog in __init__, which runs before user config is applied; move the load_skill_catalog(...) call and creation/registration of the SkillsFetcher tool into prerequisites_callable(config) instead: validate the passed config dict against a SkillsConfig Pydantic model, merge config-provided paths (e.g., local_skills_path) with additional_search_paths, call load_skill_catalog(dal=dal, custom_skill_paths=merged_paths) inside prerequisites_callable, then instantiate SkillsFetcher(self, skill_catalog=skill_catalog, dal=dal) and attach it to the toolset (e.g., set self.tools or return it as the prerequisites result) so the catalog respects user overrides.
171-178:⚠️ Potential issue | 🟡 MinorMisleading error message in the no-DAL branch (still present).
This branch is now reached for any DAL-disabled remote-content fetch, including when
linkis a normalized skill name from a REMOTE catalog entry routed via line 74 — not just UUID-style IDs. The "appears to be a UUID" wording is incorrect in that path.- err_msg = "Skill link appears to be a UUID, but no remote data access layer (dal) is enabled." + err_msg = ( + f"Cannot fetch remote skill '{link}': remote data access layer (DAL) is not enabled." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` around lines 171 - 178, The error message in the DAL-disabled branch of the remote fetch path (where err_msg is set and a StructuredToolResult with status=StructuredToolResultStatus.ERROR is returned) wrongly refers to a UUID; update the err_msg and corresponding logging.error call to a generic, accurate message indicating that remote data access (DAL) is disabled and remote-content fetches (including normalized remote catalog names or UUIDs) cannot proceed, and ensure StructuredToolResult.error uses that new message so callers see a correct explanation.
🧹 Nitpick comments (4)
tests/plugins/toolsets/test_skills.py (2)
1-13: UnusedTEST_SKILLS_PATHandosimport.
TEST_SKILLS_PATHis defined but never referenced by any test. If you don't plan to add fixture-backed tests in this file, drop both the constant and theosimport.-import os - from holmes.core.tools import StructuredToolResultStatus from holmes.plugins.skills.skill_loader import Skill, SkillCatalog, SkillSource from holmes.plugins.toolsets.skills.skills_fetcher import ( SkillsFetcher, SkillsToolset, ) from tests.conftest import create_mock_tool_invoke_context - -TEST_SKILLS_PATH = os.path.join( - os.path.dirname(__file__), "fixtures", "skills" -)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/toolsets/test_skills.py` around lines 1 - 13, Remove the unused TEST_SKILLS_PATH constant and the unused os import from tests/plugins/toolsets/test_skills.py; specifically delete the TEST_SKILLS_PATH symbol and the top-level "import os" line (or, alternatively, modify tests to actually reference TEST_SKILLS_PATH in test setup if fixtures are intended), ensuring no other code references TEST_SKILLS_PATH or os after the change.
16-23: Add coverage for the_dalfallback path.The current tests cover catalog hit, miss-without-DAL, and empty
skill_id, but_invoke's remote-skill paths (catalog hit withSkillSource.REMOTE→_get_robusta_skill, and the no-catalog-match-but-DAL-enabled fallback) are uncovered. Consider adding a test that injects a mockSupabaseDal(withenabled=Trueand a stubbedget_skill_content) to cover those branches and the<skill>wrapping for remote content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/plugins/toolsets/test_skills.py` around lines 16 - 23, Add a new test that exercises the `_dal` fallback and remote-skill branch by instantiating SkillsFetcher(SkillsToolset()) but injecting a mock SupabaseDal (set enabled=True and stub get_skill_content to return remote skill text) into the fetcher's `_dal` attribute and/or mocking catalog lookup to simulate no catalog match or a SkillSource.REMOTE catalog entry; call SkillsFetcher._invoke with the target skill_id, then assert the result is success (or expected status) and that returned content is wrapped with the expected `<skill>...</skill>` remote-wrapping and that get_skill_content was called—this will cover both the `_get_robusta_skill` remote path and the DAL fallback path.holmes/plugins/toolsets/skills/skills_fetcher.py (1)
24-28: Mutable default for class attributeavailable_skills.
available_skills: List[str] = []at class scope is a shared mutable default (Ruff RUF012). Even though__init__passes a fresh list tosuper().__init__, the class-level default can be silently mutated by tooling that introspects the schema. PreferField(default_factory=list)(sinceToolappears to be a Pydantic model based on the# type: ignore[call-arg]usage) or drop the default and require it.-class SkillsFetcher(Tool): - toolset: "SkillsToolset" - available_skills: List[str] = [] +class SkillsFetcher(Tool): + toolset: "SkillsToolset" + available_skills: List[str] = Field(default_factory=list)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/skills/skills_fetcher.py` around lines 24 - 28, The class attribute available_skills on SkillsFetcher is defined as a shared mutable default (List[str] = []) — change it to use a Pydantic field with a default_factory to avoid shared-state bugs: replace the class-level List default with something like Field(default_factory=list) on available_skills (keeping the type List[str]) or remove the class default entirely and ensure __init__/super().__init__ provides a fresh list; update imports to include Field if necessary and keep references to SkillsFetcher, available_skills, and Tool consistent.holmes/plugins/skills/skill_loader.py (1)
117-131:os.walktraversal does not followPath.resolveguarantees and may double-traverse symlinks.
scan_skill_directoryresolves the input directory but then callsos.walk(directory), which by default does not follow symlinks (good) but will silently include nested directories regardless ofresolve(). Additionally,len(Path(root).resolve().relative_to(directory).parts)resolvesrooton every iteration; for large trees this is unnecessary work. A simpler depth calc using string prefix oros.walk's native depth is cheaper. Optional cleanup:- for root, dirs, files in os.walk(directory): - depth = len(Path(root).resolve().relative_to(directory).parts) + for root, dirs, files in os.walk(directory): + depth = len(Path(root).relative_to(directory).parts) if depth >= max_depth: dirs.clear() continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/skills/skill_loader.py` around lines 117 - 131, Resolve the input directory once (e.g., base_dir = Path(directory).resolve()) and call os.walk on str(base_dir) with topdown=True and followlinks=False to preserve the original symlink behavior; compute depth cheaply by using os.path.relpath(root, start=str(base_dir)) and counting path components (handle '.' as depth 0) instead of resolving root each iteration, and keep the existing logic that clears dirs when depth >= max_depth and calls parse_skill_file (references: scan_skill_directory loop, SKILL_FILENAME, parse_skill_file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/skills/__init__.py`:
- Around line 19-24: The helper _repr_str currently does an unconditional
s.replace("\\n", "\n") which will mangle literal backslash-n sequences; change
it to avoid blind replacement — either remove the replace entirely and rely on
dumper.represent_scalar with block style, or guard the replace so it only runs
when the string appears double-escaped (e.g., if "\\n" in s and "\n" not in s)
or when a source-specific flag indicates double-escaping (add an optional
parameter or call-site conditional). Update _repr_str to perform the guarded
replacement (or none) and still call
dumper.represent_scalar("tag:yaml.org,2002:str", s, style="|" if "\n" in s else
None).
In `@holmes/plugins/skills/CLAUDE.md`:
- Line 1: Fix the grammar in the CLAUDE.md opening sentence: change the phrase
"an AI-driven troubleshooting agents" to either "an AI-driven troubleshooting
agent" (singular) or remove the article to read "AI-driven troubleshooting
agents" so the article/noun agree; update the line that starts "You are an
expert in automated diagnostics and skill creation for an AI-driven
troubleshooting agents." to the chosen corrected version.
In `@holmes/plugins/skills/README.md`:
- Line 31: The README references the skill template inconsistently—use a single
filename across both mentions; replace the reference to "prompt.md" with
"skill-format.prompt.md" (or vice versa) so both Line 27 and Line 31 refer to
the exact same template file name (e.g., update the text "Start with the
Template: Use `prompt.md`" to "Use `skill-format.prompt.md`") to ensure
consistent template naming in the README.
- Around line 13-17: The README references a non-existent example file
"kube-prometheus-stack.yaml" under the "Structured Skill" section used by
`holmes investigate`; update this by either adding the missing structured skill
file to the skills directory or replacing the reference with an existing
structured skill example (use a real filename present in the skills directory)
so the example in the README points to a valid structured skill; ensure the
surrounding text still mentions "Structured Skill" and "`holmes investigate`" to
keep context.
In `@holmes/plugins/skills/skill_loader.py`:
- Around line 134-150: The import for RobustaSkillInstruction should be moved
out of the function and placed at the top of the module: remove the in-function
line "from holmes.plugins.skills import RobustaSkillInstruction # noqa: F811",
add a top-level import for RobustaSkillInstruction alongside the other imports,
and drop the unnecessary "# noqa: F811" comment; then keep
map_robusta_instruction_to_skill unchanged to reference RobustaSkillInstruction
directly (there is no circular-import risk per the review).
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Around line 142-154: In _get_robusta_skill, the remote Skill.description is
set to only skill_content.title which omits the symptom added by
map_robusta_instruction_to_skill; update the description creation in the
Skill(...) call inside _get_robusta_skill to mirror the catalog format (e.g.,
f"{skill_content.title} — {skill_content.symptom}" when symptom exists) so
remote skills include the symptom the same way as local/catalog skills; adjust
to check for the presence/truthiness of skill_content.symptom before
concatenating.
---
Outside diff comments:
In `@holmes/core/supabase_dal.py`:
- Around line 642-664: The code assumes row.get("runbook") is a dict and calls
.get("instructions") directly which raises AttributeError if runbook is None or
missing; change to first fetch runbook = row.get("runbook") and then set
raw_instruction = runbook.get("instructions") if runbook else None, or wrap the
whole extraction in a try/except like get_skill_catalog to catch
AttributeError/TypeError, log a descriptive error including skill_id and the
row, and handle by setting instruction = None or str(raw_instruction) /
returning None as appropriate so the caller (skills_fetcher._get_robusta_skill)
doesn't crash.
---
Duplicate comments:
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Around line 186-208: The SkillsToolset currently calls load_skill_catalog in
__init__, which runs before user config is applied; move the
load_skill_catalog(...) call and creation/registration of the SkillsFetcher tool
into prerequisites_callable(config) instead: validate the passed config dict
against a SkillsConfig Pydantic model, merge config-provided paths (e.g.,
local_skills_path) with additional_search_paths, call
load_skill_catalog(dal=dal, custom_skill_paths=merged_paths) inside
prerequisites_callable, then instantiate SkillsFetcher(self,
skill_catalog=skill_catalog, dal=dal) and attach it to the toolset (e.g., set
self.tools or return it as the prerequisites result) so the catalog respects
user overrides.
- Around line 171-178: The error message in the DAL-disabled branch of the
remote fetch path (where err_msg is set and a StructuredToolResult with
status=StructuredToolResultStatus.ERROR is returned) wrongly refers to a UUID;
update the err_msg and corresponding logging.error call to a generic, accurate
message indicating that remote data access (DAL) is disabled and remote-content
fetches (including normalized remote catalog names or UUIDs) cannot proceed, and
ensure StructuredToolResult.error uses that new message so callers see a correct
explanation.
---
Nitpick comments:
In `@holmes/plugins/skills/skill_loader.py`:
- Around line 117-131: Resolve the input directory once (e.g., base_dir =
Path(directory).resolve()) and call os.walk on str(base_dir) with topdown=True
and followlinks=False to preserve the original symlink behavior; compute depth
cheaply by using os.path.relpath(root, start=str(base_dir)) and counting path
components (handle '.' as depth 0) instead of resolving root each iteration, and
keep the existing logic that clears dirs when depth >= max_depth and calls
parse_skill_file (references: scan_skill_directory loop, SKILL_FILENAME,
parse_skill_file).
In `@holmes/plugins/toolsets/skills/skills_fetcher.py`:
- Around line 24-28: The class attribute available_skills on SkillsFetcher is
defined as a shared mutable default (List[str] = []) — change it to use a
Pydantic field with a default_factory to avoid shared-state bugs: replace the
class-level List default with something like Field(default_factory=list) on
available_skills (keeping the type List[str]) or remove the class default
entirely and ensure __init__/super().__init__ provides a fresh list; update
imports to include Field if necessary and keep references to SkillsFetcher,
available_skills, and Tool consistent.
In `@tests/plugins/toolsets/test_skills.py`:
- Around line 1-13: Remove the unused TEST_SKILLS_PATH constant and the unused
os import from tests/plugins/toolsets/test_skills.py; specifically delete the
TEST_SKILLS_PATH symbol and the top-level "import os" line (or, alternatively,
modify tests to actually reference TEST_SKILLS_PATH in test setup if fixtures
are intended), ensuring no other code references TEST_SKILLS_PATH or os after
the change.
- Around line 16-23: Add a new test that exercises the `_dal` fallback and
remote-skill branch by instantiating SkillsFetcher(SkillsToolset()) but
injecting a mock SupabaseDal (set enabled=True and stub get_skill_content to
return remote skill text) into the fetcher's `_dal` attribute and/or mocking
catalog lookup to simulate no catalog match or a SkillSource.REMOTE catalog
entry; call SkillsFetcher._invoke with the target skill_id, then assert the
result is success (or expected status) and that returned content is wrapped with
the expected `<skill>...</skill>` remote-wrapping and that get_skill_content was
called—this will cover both the `_get_robusta_skill` remote path and the DAL
fallback path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e49ca49-6d7c-4a81-ac48-b34977098efc
📒 Files selected for processing (15)
docs/reference/.nav.ymldocs/reference/runbooks.mddocs/reference/skills.mdholmes/core/supabase_dal.pyholmes/core/tools.pyholmes/plugins/runbooks/README.mdholmes/plugins/runbooks/__init__.pyholmes/plugins/runbooks/catalog.jsonholmes/plugins/skills/CLAUDE.mdholmes/plugins/skills/README.mdholmes/plugins/skills/__init__.pyholmes/plugins/skills/skill_loader.pyholmes/plugins/toolsets/skills/skills_fetcher.pytests/llm/utils/mock_dal.pytests/plugins/toolsets/test_skills.py
💤 Files with no reviewable changes (4)
- holmes/plugins/runbooks/catalog.json
- holmes/plugins/runbooks/README.md
- docs/reference/runbooks.md
- holmes/plugins/runbooks/init.py
✅ Files skipped from review due to trivial changes (3)
- docs/reference/.nav.yml
- holmes/core/tools.py
- docs/reference/skills.md
RoiGlinik
previously approved these changes
Apr 27, 2026
- get_skill_catalog: add .eq("enabled", True) to query, filter rows
where clusters is not null and self.cluster not in clusters
- get_resource_instructions: add .eq("enabled", True), iterate rows to
find first matching cluster (null = all clusters)
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Fixed skill catalog to correctly display only enabled skills, exclude
incomplete entries, and properly apply cluster filtering.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The build-binary CI step failed because PyInstaller --add-data still referenced the deleted holmes/plugins/runbooks/ directory. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: avi@robusta.dev <avi@robusta.dev>
RoiGlinik
previously approved these changes
Apr 27, 2026
- _get_robusta_skill now builds description as "{title} — {symptom}"
matching map_robusta_instruction_to_skill for consistency
- Move RobustaSkillInstruction import to top-level in skill_loader.py
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: avi@robusta.dev <avi@robusta.dev>
Avi-Robusta
enabled auto-merge (squash)
April 27, 2026 11:21
RoiGlinik
approved these changes
Apr 27, 2026
naomi-robusta
pushed a commit
that referenced
this pull request
Apr 28, 2026
The master merge included the Skills PR (#1953) which renamed Config.get_runbook_catalog() to get_skill_catalog() and the build_chat_messages parameter from runbooks= to skills=. https://claude.ai/code/session_013q1t5sReMaC3SboNv3LZ1m Signed-off-by: Claude <noreply@anthropic.com>
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.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chore