Refactor toolset enabling - #1830
Conversation
Remove the enabled/is_default policy guard from missing_config so it only reports whether required configuration is absent. Callers (toolset_manager.py) already handle the policy decision of what to do with that information. Also simplify the tail to `return self.config is None`. https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
📂 Previous Runs📜 #5 · Run @ __52245cd__ (#23423529277) — Mar 23, 06:00 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 52245cd on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📜 #4 · Run @ __a3cab75__ (#23415235105) — Mar 22, 23:42 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit a3cab75 on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📜 #3 · Run @ __3e9bf55__ (#23414906790) — Mar 22, 23:24 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 3e9bf55 on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📜 #2 · Run @ __0357e3b__ (#23413966564) — Mar 22, 22:31 UTC✅ Results of HolmesGPT evalsAutomatically triggered by commit 0357e3b on branch Results of HolmesGPT evals
Benchmark comparison unavailable: No ci-benchmark experiments found Benchmark Comparison DetailsBaseline: latest ci-benchmark experiment on master Status: No ci-benchmark experiments found Comparison indicators:
📜 #1 · Run @ __0aca0c3__ (#23413737768) — Mar 22, 22:17 UTC
|
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Compactions |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ❌ | 09_crashpod | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 101_loki_historical_logs_pod_deleted | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 112_find_pvcs_by_uuid | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 12_job_crashing | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 176_network_policy_blocking_traffic_no_runbooks | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 227_count_configmaps_per_namespace[0] | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 243_pod_names_contain_service | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 24_misconfigured_pvc | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 43_current_datetime_from_prompt | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 51_logs_summarize_errors | — | — | — | — | — | — | — | — | — | — | — | — | — |
| ❌ | 61_exact_match_counting | — | — | — | — | — | — | — | — | — | — | — | — | — |
| Total | — avg | — avg | — avg | — | — | — | — | — | — | — | — | — | — |
Benchmark comparison unavailable: No ci-benchmark experiments found
Benchmark Comparison Details
Baseline: latest ci-benchmark experiment on master
Status: No ci-benchmark experiments found
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
⚠️ 11 Failures Detected
✅ Results of HolmesGPT evals
Automatically triggered by commit 0937ccf on branch claude/fix-missing-config-toolset-TgjCb
Results of HolmesGPT evals
- ask_holmes: 11/11 test cases were successful, 0 regressions
| Status | Test case | Time | Turns | Tools | Cost | Total tokens | Input | Max input | Output | Max output | Cached | Non-cached | Reasoning | Compactions |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ✅ | 09_crashpod | 44.7s | 4 | 9 | $0.2252 | 82,913 | 80,963 | 23,190 | 1,950 | 997 | 57,187 | 23,776 | — | — |
| ✅ | 101_loki_historical_logs_pod_deleted | 86.5s | 9 | 17 | $0.3566 | 210,028 | 206,407 | 27,755 | 3,621 | 559 | 177,931 | 28,476 | — | — |
| ✅ | 112_find_pvcs_by_uuid | 27.3s | 3 | 4 | $0.2160 | 67,064 | 65,952 | 24,698 | 1,112 | 667 | 38,308 | 27,644 | — | — |
| ✅ | 12_job_crashing | 48.6s | 6 | 13 | $0.2792 | 137,902 | 135,586 | 25,490 | 2,316 | 671 | 108,559 | 27,027 | — | — |
| ✅ | 176_network_policy_blocking_traffic_no_runbooks | 55.7s | 7 | 12 | $0.2922 | 154,262 | 151,649 | 25,671 | 2,613 | 606 | 125,202 | 26,447 | — | — |
| ✅ | 227_count_configmaps_per_namespace[0] | 38.4s | 5 | 9 | $0.2010 | 95,672 | 94,418 | 21,041 | 1,254 | 585 | 73,050 | 21,368 | — | — |
| ✅ | 243_pod_names_contain_service | 46.7s | 5 | 10 | $0.2362 | 103,343 | 101,177 | 22,630 | 2,166 | 597 | 78,248 | 22,929 | — | — |
| ✅ | 24_misconfigured_pvc | 64.8s | 5 | 14 | $0.2548 | 107,406 | 104,974 | 23,851 | 2,432 | 779 | 80,148 | 24,826 | — | — |
| ✅ | 43_current_datetime_from_prompt | 7.1s | 1 | — | $0.1098 | 17,202 | 17,078 | 17,078 | 124 | 124 | 0 | 17,078 | — | — |
| ✅ | 51_logs_summarize_errors | 39.8s | 4 | 5 | $0.1882 | 78,302 | 77,223 | 21,314 | 1,079 | 328 | 55,897 | 21,326 | — | — |
| ✅ | 61_exact_match_counting | 15.1s | 2 | 1 | $0.1281 | 35,067 | 34,684 | 17,597 | 383 | 314 | 17,077 | 17,607 | — | — |
| Total | 43.1s avg | 4.6 avg | 9.4 avg | $2.4874 | 1,089,161 | 1,070,111 | 27,755 | 19,050 | 997 | 811,607 | 258,504 | — | — |
Benchmark comparison unavailable: No ci-benchmark experiments found
Benchmark Comparison Details
Baseline: latest ci-benchmark experiment on master
Status: No ci-benchmark experiments found
Comparison indicators:
±0%— diff under 10% (within noise threshold)↑N%/↓N%— diff 10-25%↑N%/↓N%— diff over 25% (significant)
📖 Legend
| Icon | Meaning |
|---|---|
| ✅ | The test was successful |
| ➖ | The test was skipped |
| The test failed but is known to be flaky or known to fail | |
| 🚧 | The test had a setup failure (not a code regression) |
| 🔧 | The test failed due to mock data issues (not a code regression) |
| 🚫 | The test was throttled by API rate limits/overload |
| ❌ | The test failed and should be fixed before merging the PR |
🔄 Re-run evals manually
⚠️ Warning:/evalcomments always run using the workflow from master, not from this PR branch. If you modified the GitHub Action (e.g., added secrets or env vars), those changes won't take effect.To test workflow changes, use the GitHub CLI or Actions UI instead:
gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/fix-missing-config-toolset-TgjCb -f markers=regression -f filter=
Option 1: Comment on this PR with /eval:
/eval
tags: regression
Or with more options (one per line):
/eval
model: gpt-4o
tags: regression
id: 09_crashpod
iterations: 5
Run evals on a different branch (e.g., master) for comparison:
/eval
branch: master
tags: regression
| Option | Description |
|---|---|
model |
Model(s) to test (default: same as automatic runs) |
tags |
Pytest tags / markers (no default - runs all tests!) |
id |
Eval ID / pytest -k filter (use /list to see valid eval names) |
iterations |
Number of runs, max 10 |
branch |
Run evals on a different branch (for cross-branch comparison) |
Quick re-run: Use /rerun to re-run the most recent /eval on this PR with the same parameters.
Option 2: Trigger via GitHub Actions UI → "Run workflow"
Option 3: Add PR labels to include extra evals (applies to both automatic runs and /eval comments):
| Label | Effect |
|---|---|
evals-tag-<name> |
Run tests with tag <name> alongside regression |
evals-id-<name> |
Run a specific eval by test ID |
evals-model-<name> |
Override the model (use model list name, e.g. sonnet-4.5) |
Examples: evals-tag-easy, evals-id-09_crashpod, evals-model-sonnet-4.5
🏷️ Valid tags
benchmark, chain-of-causation, compaction, confluence, context_window, coralogix, counting, database, datadog, datetime, db-connectors, easy, elasticsearch, embeds, fast, frontend, grafana, hard, images, integration, kafka, kubernetes, leaked-information, logs, loki, mcp, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, runbooks, slackbot, storage, toolset-limitation, traces, transparency
🤖 Valid models
deepseek-chat, deepseek-r1-reasoner, deepseek-reasoner, deepseek-v3.2-chat, gemini-3-flash-preview, gemini-3-pro-preview, gemini-3.1-pro-preview, gpt-4.1, gpt-5.2-high-reasoning, gpt-5.3-codex, gpt-5.4, haiku-4.5, kimi-2.5, kimi-2.5-openrouter, opus-4.5, opus-4.6, qwen-next-80B-instruct, qwen-next-80B-thinking, sonnet-4.5, sonnet-4.6
Commands: /eval · /rerun · /list
CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/fix-missing-config-toolset-TgjCb -f markers=regression -f filter=
|
✅ 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:21ddffc4
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:21ddffc4 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:21ddffc4
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:21ddffc4
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:21ddffc4
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:21ddffc4 me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:21ddffc4
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:21ddffc4Patch 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:21ddffc4 \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:21ddffc4Robusta 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:21ddffc4 \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:21ddffc4 |
|
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:
WalkthroughRemoved the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. |
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit in Settings → Usage.
Once credits are available, comment @claude review on this pull request to trigger a review.
|
@claude review |
There was a problem hiding this comment.
The logic change is clean and well-tested, but this is a behavioral change to a core Toolset property — toolsets that are enabled=True but have no config will now report missing_config=True, which could affect callers that rely on the old short-circuit. Worth a quick human glance before merging.
Extended reasoning...
Overview
This PR modifies Toolset.missing_config in holmes/core/tools.py by removing the early-return if self.enabled or self.is_default: return False guard, making the property a pure fact-check. Tests in tests/test_toolset_auto_enable.py are updated accordingly.
Security risks
No security-sensitive code is touched. No auth, crypto, or permission changes.
Level of scrutiny
Despite being described as a refactor, this is a behavioral change: previously an enabled=True toolset with required config classes but no config would return missing_config=False; after this PR it returns True. Any caller that gates behavior on missing_config (e.g., to decide whether to show a setup warning or skip a toolset) may now behave differently for toolsets that are enabled but have missing config. The diff is small and the test coverage is solid, but the caller surface for missing_config should be verified before merging.
Other factors
The nit about is_default becoming dead code is valid but low impact — the field still serializes via model_dump_json() and no runtime regression occurs today. Evals show 9/10 passing with 1 setup failure (not a regression). CodeRabbit raised no actionable concerns. The PR is well-scoped and the intent is clear, but a human should confirm the behavioral change for enabled=True toolsets is intentional and that all callers of missing_config have been audited.
is_default was never read to make runtime decisions — toolsets are enabled either explicitly (enabled=True in constructor) or via the Helm chart config. Remove the field, all setter sites, and the associated test and PR template checklist item. https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
aantn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_toolset_auto_enable.py (1)
74-77: Please add a caller-level auto-enable test for this refactor.This locks in the property contract, but the PR objective is about caller behavior after the refactor—especially the explicit
enabled: falsepath. AToolsetManager/auto-enable test with a required-config toolset would catch regressions that this unit test will miss.As per coding guidelines, "Live execution is enabled by default in tests - ensure tests match real-world behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_toolset_auto_enable.py` around lines 74 - 77, Add a new caller-level test that registers a required-config toolset with the ToolsetManager and verifies the manager auto-enables it even when the toolset was constructed with enabled=False: use _make_toolset(enabled=False, config_classes=[RequiredFieldConfig]) to create the toolset, register it with ToolsetManager, invoke the public caller-level entrypoint on ToolsetManager that triggers auto-enable (the manager method used by callers to load/enable toolsets), then assert the toolset is now enabled and still reports missing_config=True; this locks in caller behavior after the refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_toolset_auto_enable.py`:
- Around line 74-77: Add a new caller-level test that registers a
required-config toolset with the ToolsetManager and verifies the manager
auto-enables it even when the toolset was constructed with enabled=False: use
_make_toolset(enabled=False, config_classes=[RequiredFieldConfig]) to create the
toolset, register it with ToolsetManager, invoke the public caller-level
entrypoint on ToolsetManager that triggers auto-enable (the manager method used
by callers to load/enable toolsets), then assert the toolset is now enabled and
still reports missing_config=True; this locks in caller behavior after the
refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30e0cfcf-0513-4f05-bea0-f9084debb6de
📒 Files selected for processing (12)
.github/PULL_REQUEST_TEMPLATE/toolset.mdholmes/core/tools.pyholmes/plugins/toolsets/bash/bash_toolset.pyholmes/plugins/toolsets/connectivity_check.pyholmes/plugins/toolsets/internet/internet.pyholmes/plugins/toolsets/internet/notion.pyholmes/plugins/toolsets/investigator/core_investigation.pyholmes/plugins/toolsets/kubernetes_logs.pyholmes/plugins/toolsets/robusta/robusta.pyholmes/plugins/toolsets/runbook/runbook_fetcher.pytests/plugins/toolsets/test_core_investigation.pytests/test_toolset_auto_enable.py
💤 Files with no reviewable changes (10)
- holmes/plugins/toolsets/investigator/core_investigation.py
- holmes/plugins/toolsets/bash/bash_toolset.py
- holmes/plugins/toolsets/robusta/robusta.py
- .github/PULL_REQUEST_TEMPLATE/toolset.md
- tests/plugins/toolsets/test_core_investigation.py
- holmes/plugins/toolsets/kubernetes_logs.py
- holmes/plugins/toolsets/connectivity_check.py
- holmes/plugins/toolsets/internet/notion.py
- holmes/plugins/toolsets/runbook/runbook_fetcher.py
- holmes/plugins/toolsets/internet/internet.py
These two toolsets were relying on the now-removed is_default field to be picked up in server mode. Without explicit enabled=True, they would be silently disabled when enable_all_toolsets=False (server flow). https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/plugins/toolsets/internet/internet.py (1)
272-286:⚠️ Potential issue | 🔴 Critical
enabled=Truecauses constructor crash due to signature mismatch.
InternetToolset.__init__passesenabled=True, butInternetBaseToolset.__init__does not acceptenabled. This will fail at runtime withTypeError: unexpected keyword argument 'enabled'.Proposed fix
Add
enabledparameter toInternetBaseToolset.__init__:class InternetBaseToolset(Toolset): def __init__( self, name: str, description: str, icon_url: str, tools: list[Tool], tags: List[ToolsetTag], docs_url: Optional[str] = None, + enabled: bool = False, ): super().__init__( name=name, description=description, icon_url=icon_url, prerequisites=[ CallablePrerequisite(callable=self.prerequisites_callable), ], tools=tools, tags=tags, docs_url=docs_url, + enabled=enabled, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/internet/internet.py` around lines 272 - 286, The crash is caused by InternetToolset.__init__ passing enabled=True to InternetBaseToolset.__init__, which doesn't accept that keyword; fix by adding an enabled: bool = True parameter to InternetBaseToolset.__init__ (and propagate it to its super().__init__ or otherwise handle/store it) so InternetToolset.__init__ can pass enabled through without raising TypeError; update InternetBaseToolset.__init__ signature and forwarding logic accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@holmes/plugins/toolsets/internet/internet.py`:
- Around line 272-286: The crash is caused by InternetToolset.__init__ passing
enabled=True to InternetBaseToolset.__init__, which doesn't accept that keyword;
fix by adding an enabled: bool = True parameter to InternetBaseToolset.__init__
(and propagate it to its super().__init__ or otherwise handle/store it) so
InternetToolset.__init__ can pass enabled through without raising TypeError;
update InternetBaseToolset.__init__ signature and forwarding logic accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d67d39f-a347-49ea-8f82-7668cbbb0bd2
📒 Files selected for processing (2)
holmes/plugins/toolsets/internet/internet.pyholmes/plugins/toolsets/kubernetes_logs.py
There was a problem hiding this comment.
LGTM — the is_default dead-code concern I raised was addressed in the follow-up commit, and the missing_config refactor is clean and well-tested.
Extended reasoning...
Overview
This PR refactors Toolset.missing_config from a policy-aware method (short-circuiting on enabled/is_default) into a pure fact-check: it returns True only when config_classes declare required fields and self.config is None. The is_default field and all its setter sites across 7+ toolsets were also removed. Tests and the PR template checklist were updated accordingly.
Security risks
None. The change is entirely internal to toolset configuration introspection and does not touch auth, crypto, permissions, or user-facing data paths.
Level of scrutiny
Moderate — the refactor is intentional and well-described, but it does change observable behavior for the edge case where a toolset had is_default=True and enabled=False with required config fields. The PR author explicitly calls this out as the desired fix. All affected toolsets were verified to either have enabled=True or no required config, so no regression is expected. Test coverage was updated to match.
Other factors
The pre-existing FetchNotion.additional_headers attribute-path bug is flagged as a separate, pre-existing issue not introduced by this PR. My prior inline comment about is_default dead code was fully addressed by commit 7f883e6. Evals show 9/10 passing with 1 unrelated setup failure, consistent with baseline.
These two toolsets were relying on the now-removed is_default field to be picked up in server mode. Without explicit enabled=True, they would be silently disabled when enable_all_toolsets=False (server flow). https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
…0.0.1:34303/git/HolmesGPT/holmesgpt into claude/fix-missing-config-toolset-TgjCb
InternetBaseToolset.__init__() has explicit params that don't include enabled, so we set self.enabled=True after the super call. Only in InternetToolset, not the base class — NotionToolset should stay disabled by default. https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
This eval uses kubernetes/logs and will fail if that toolset is not enabled, providing a regression guard for server-mode enablement. https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
The manifest created namespace app-111 but before_test looked for events in app-243, causing setup to always fail with "No resources found in app-243 namespace". https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — the critical TypeError I flagged in InternetToolset.__init__ was fixed, is_default was fully removed rather than left as dead code, and CI passes with 0 regressions.
Extended reasoning...
Overview
This PR removes Toolset.is_default entirely and decouples default/enablement logic from config-missing detection. The change touches holmes/core/tools.py (core logic), 7 plugin toolsets (constructor cleanups), 2 test files, a PR template, and a test fixture. Net effect: missing_config is now a pure fact-check independent of enabled/is_default state.
Security risks
No security-sensitive code paths are affected. The change is scoped to toolset initialization and introspection logic.
Level of scrutiny
Moderate — the change touches production toolset initialization code that runs at startup for every request. However, the semantics are straightforward (removing a concept entirely rather than introducing new branching), tests were updated to match, and CI confirms no regressions.
Other factors
The critical regression I reported earlier (TypeError: InternetBaseToolset.__init__() got an unexpected keyword argument enabled) was fixed in commit 51f443c by setting self.enabled = True after super().__init__(). The is_default dead code concern I raised was resolved by fully removing the field from the model, not just removing the reader. The pre-existing notion.py bug I flagged predates this PR and has an inline comment already on the PR. Latest CI run (commit 1d81a5e) shows 10/11 tests passing with 0 regressions (1 setup failure is infrastructure-related and consistent across all runs).
There was a problem hiding this comment.
LGTM — the is_default removal is clean, the InternetToolset TypeError was fixed, and evals are 11/11.
Extended reasoning...
Overview
This PR removes the Toolset.is_default field and refactors missing_config into a pure fact-check (does the toolset require config that was not provided?). Changes touch holmes/core/tools.py, 7 plugin toolsets, and corresponding tests. The PR template checklist item for is_default is also removed.
Security Risks
No security-sensitive code touched. The missing_config change is a semantics fix: it now correctly reports missing config even when a toolset is enabled, which is safer than silently hiding the missing-config state.
Level of Scrutiny
This is a refactoring PR with clear intent and bounded scope. The CI eval suite is the best validator here — it went from 0/10 (after the intermediate broken commit) back to 11/11 on the latest commit, confirming no regressions. Test coverage was also updated to match the new semantics.
Other Factors
My earlier blocking concern (the TypeError from passing enabled=True to InternetBaseToolset.__init__) was resolved in commit 51f443c. The is_default dead-code concern was addressed by removing the field entirely rather than leaving it as write-only metadata. The pre-existing FetchNotion._invoke bug (self.toolset.additional_headers) is out of scope for this PR and already flagged as a separate inline comment.
Pass enabled=True to super().__init__() instead of setting self.enabled after construction, matching all other builtin toolsets. https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
…0.0.1:45127/git/HolmesGPT/holmesgpt into claude/fix-missing-config-toolset-TgjCb
InternetBaseToolset.__init__() didn't forward extra keyword arguments to Toolset.__init__(), causing a TypeError when InternetToolset passed enabled=True. Add **kwargs forwarding to match other base toolset patterns (e.g. ElasticsearchBaseToolset). https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — all previously identified issues have been resolved.
Extended reasoning...
Overview
This PR refactors toolset enablement by removing the Toolset.is_default field and simplifying Toolset.missing_config to be a pure fact-check (requires config classes with required fields AND no config provided). Touches holmes/core/tools.py, 7 plugin toolsets, 2 test files, and minor test fixture/template updates.
Security risks
No security-sensitive code paths involved. The change affects toolset enablement logic, not auth, crypto, or permissions.
Level of scrutiny
Moderate — this is a behavioral refactoring touching core toolset infrastructure. However, the change is narrowly scoped (removing one field and simplifying one property), and the intent is clearly documented. All 11 eval regression tests pass in the final run.
Prior concerns resolved
- Critical bug (🔴):
InternetToolset.__init__was passingenabled=TruetoInternetBaseToolset.__init__which had no such parameter, causing aTypeErrorat startup. Fixed in commit51f443cby movingself.enabled = Trueafter thesuper().__init__()call — exactly as suggested. - Dead code (🟡):
is_defaultfield was left as write-only dead code after themissing_configrefactor. Fixed by removing the field entirely fromToolsetand all 7 setter sites — cleaner than the documentation-only fix I suggested. - Pre-existing bug (🟣):
FetchNotion._invokeaccessingself.toolset.additional_headersis still present but was pre-existing before this PR and already has an inline comment.
There was a problem hiding this comment.
Blocking TypeError is resolved, evals are 11/11, LGTM.
Extended reasoning...
Overview
This PR removes Toolset.is_default entirely and simplifies missing_config to be a pure fact-check (no longer short-circuiting on enabled or is_default). The change touches 8 toolset constructors (removing is_default=True/False), holmes/core/tools.py (field and method simplification), tests, and fixture metadata.
Security risks
No security-sensitive code paths (auth, crypto, permissions) are touched. The changes are purely about toolset enablement logic.
Level of scrutiny
Moderate. This is a behavioral refactor affecting how toolsets report their configuration state, but it is well-scoped: the only intended behavioral change is that toolsets with is_default=True but enabled=False now correctly report missing config rather than silently bypassing the check. The PR description and test changes clearly document this intent.
Other factors
My prior reviews flagged three issues: (1) is_default dead code — addressed by the PR fully removing the field and all setter sites; (2) pre-existing FetchNotion._invoke AttributeError — pre-existing bug, already posted as an inline comment, not introduced by this PR; (3) blocking TypeError from enabled=True being passed to InternetBaseToolset.__init__ — fixed in commit 0357e3b by adding **kwargs to the base class. The latest eval run (commit 0357e3b) shows 11/11 passing, confirming the fix. The refactoring is clean and the test updates correctly reflect the new semantics.
Covers the 8 layers of enablement logic, the CLI vs server asymmetry, and proposes simplification options. https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
…n spec Addresses PR #1830 review comments: - Add caller-level tests verifying ToolsetManager._list_all_toolsets skips auto-enabling toolsets with missing required config - Document the missing_config decoupling from enabled state in the spec https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — all previously flagged bugs are resolved, 11/11 evals pass, and the remaining inline comments are nits.
Extended reasoning...
Overview
This PR removes Toolset.is_default entirely and decouples missing_config from enabled state. It touches holmes/core/tools.py, eight plugin toolset constructors, a new spec document, and test files. The scope is a clean refactoring with a targeted behavioral fix (user-set enabled: false is now respected even for previously is_default=True toolsets).
Security Risks
No security-sensitive paths (auth, crypto, permissions) are modified. The changes are internal toolset lifecycle logic. No new attack surface is introduced.
Level of Scrutiny
This is moderate-complexity logic that controls which toolsets are active at runtime. The key property is missing_config, whose semantics changed to be a pure fact-check independent of enabled/is_default. The change is well-motivated, correctly implemented, and covered by both unit tests and 11/11 passing eval tests. The fix to InternetBaseToolset.__init__ (accepting **kwargs) that unblocked the critical TypeError regression was confirmed correct.
Other Factors
All three bugs I flagged in earlier review rounds were addressed: the TypeError crash (enabled=True kwarg rejection), the is_default write-only dead code (field fully removed), and the startup regression (0/11 → 11/11 evals). The two new inline reports are nits: a dead test helper that ignores its parameter, and spec documentation inaccuracies. Neither affects production correctness.
- Remove confluence from the enabled=True table (it does not set enabled=True in its constructor; the enabled=True at line 225 is for an internal HttpToolset instance) - Correct Layer 8 to document that missing_config actively fires for confluence, grafana, elasticsearch, servicenow, rabbitmq - Fix Section 5 to not cite confluence as an always-try toolset - Nuance Section 2 re CLI override vs missing_config gate https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq Signed-off-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
LGTM — the blocking TypeError fix, is_default removal, and new TestToolsetManagerAutoEnable integration tests address the main concerns from prior reviews; all 11/11 evals pass.
Extended reasoning...
Overview
This PR refactors toolset enabling by removing Toolset.is_default, decoupling missing_config from the enabled flag, and fixing a regression where InternetToolset could not be instantiated. Changes touch holmes/core/tools.py, 7 plugin toolsets, a new specs/toolset-enablement.md, and test files.
Security risks
No security-sensitive code paths (auth, crypto, permissions) are touched. The change makes missing_config a pure fact-check independent of enabled state, which is strictly safer — it can no longer be silently bypassed by setting enabled=True.
Level of scrutiny
Moderate. The core logic change (missing_config no longer short-circuits on enabled) is small and clearly correct. The **kwargs passthrough fix in InternetBaseToolset.__init__ is minimal. The bulk of the diff is mechanical removal of is_default= arguments across toolsets.
Other factors
Prior reviews from this bot identified: (1) InternetToolset TypeError — fixed in 0357e3b via **kwargs passthrough; (2) is_default write-only dead code — resolved by removing the field entirely; (3) spec inaccuracies — partially addressed in 52245cd with confluence notes; (4) missing integration tests — added in a3cab75. The outstanding nits (spec robusta entry, unused ToolsetStatusEnum import, dead _make_manager_with_toolsets helper, pre-existing notion.py null-guard) were flagged inline and are minor enough not to block. 11/11 evals pass on the latest commit.
There was a problem hiding this comment.
LGTM — the blocking TypeError from my previous review has been fixed, is_default is fully removed, and evals pass 11/11.
Extended reasoning...
Overview
This PR removes Toolset.is_default and decouples toolset enablement from the missing_config property. missing_config is now a pure fact-check (required config fields present + no config provided), independent of whether the toolset is enabled or was previously default. Fourteen files are touched: the core model in tools.py, eight plugin toolset constructors, two test files, a PR template, and a new specs/toolset-enablement.md doc.
Security risks
No security-sensitive code paths are affected. The change is purely about toolset enablement logic — no auth, crypto, or permission systems are touched.
Level of scrutiny
Moderate: this is a logic refactor in a core abstraction layer. However, the intent is narrow and the diff is mechanical (remove is_default=True/False kwargs, add **kwargs passthrough to InternetBaseToolset). All 11/11 eval regression tests pass across multiple CI runs.
Other factors
In a previous review I flagged a blocking TypeError — InternetToolset passing enabled=True to InternetBaseToolset.__init__ which had no such parameter. This has been resolved by adding **kwargs: Any to InternetBaseToolset.__init__ and forwarding it to Toolset. Minor outstanding nits (unused ToolsetStatusEnum import in the test file, unused _make_manager_with_toolsets helper method) are quality-only issues with no correctness impact.
3 participants
Refactoring, not supposed to change behaviour other than edge case toolset has is_default but was explicitly disabled by user setting enabled: false in the config. We want to respect user intent and disable it which should now be the case after this PR.
Note: this also removes is_default which was not actually used to determine what is enabled by default or not (helm values are used for that + passing enabled=true in the constructor)
https://claude.ai/code/session_01U2xED3Qq15dKJKNkHBcXjq
Summary by CodeRabbit
Bug Fixes
Behavior Changes
Tests
Chores