Skip to content

Fix ToolsetDBModel crash from model_dump unpacking all Toolset fields - #1544

Closed
Sheeproid wants to merge 3 commits into
masterfrom
claude/epic-kalam
Closed

Sheeproid wants to merge 3 commits into
masterfrom
claude/epic-kalam

Conversation

@Sheeproid

@Sheeproid Sheeproid commented Feb 11, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes TypeError: ToolsetDBModel() got multiple values for keyword argument 'account_id' that crashed toolset sync on startup
  • Replaces **toolset.model_dump(exclude_none=True) with explicit field assignment when constructing ToolsetDBModel, passing only the 6 fields it actually needs
  • Also eliminates PydanticSerializationUnexpectedValue warnings caused by attempting to serialize non-serializable CallablePrerequisite objects from the prerequisites field

Test plan

  • Deploy and verify toolset sync completes without the TypeError crash
  • Verify toolset statuses are correctly synced to the database (icon_url, status, error, description, docs_url, installation_instructions)
  • Confirm the Pydantic serialization warnings are gone from logs

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • ServiceNow integration now supports configurable health check table and API key header options.
  • Documentation

    • Updated ServiceNow toolset configuration guide with new optional parameters and their defaults.
  • Bug Fixes

    • Enhanced error reporting for ServiceNow API authentication and connection failures.
  • Tests

    • Improved test isolation for transformer configurations.

Sheeproid and others added 3 commits February 10, 2026 13:32
Allows users to configure the table used for the ServiceNow health check.

This change enables users to specify a custom table for the health check to accommodate API keys with limited permissions, improving the tool's adaptability. It also includes error responses for debugging.

Signed-off-by: Tomer Keshet <tomer@robusta.dev>
…erformanceMetrics teardown

  TestTransformerPerformanceMetrics.teardown_method was not restoring the
  original llm_summarize transformer after replacing it with a mock. When
  pytest-xdist scheduled test_yaml_transformer_parsing on the same worker
  after this class, the registry was missing llm_summarize, causing the
  YAML parser to silently drop transformer configs.

Signed-off-by: Tomer Keshet <tomer@robusta.dev>
Replace **toolset.model_dump(exclude_none=True) with explicit field
assignment when constructing ToolsetDBModel. The model_dump() approach
unpacked all ~20 Toolset fields as kwargs, causing "multiple values for
keyword argument 'account_id'" when a Toolset subclass or custom toolset
included that field. It also triggered Pydantic serialization warnings
from non-serializable CallablePrerequisite objects in the prerequisites
field.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Tomer Keshet <tomer@robusta.dev>
@netlify

netlify Bot commented Feb 11, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for holmes-docs ready!

Name Link
🔨 Latest commit 1b56c31
🔍 Latest deploy log https://app.netlify.com/projects/holmes-docs/deploys/698ca1e8f6420a0007d9b06f
😎 Deploy Preview https://deploy-preview-1544--holmes-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@linux-foundation-easycla

Copy link
Copy Markdown

CLA Not Signed

@coderabbitai

coderabbitai Bot commented Feb 11, 2026 •

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

Walkthrough

Changes add configurable health check table parameter to ServiceNow toolset, enhance error handling in health checks, improve test isolation for transformer setup/teardown, and expand toolset metadata fields stored during sync operations.

Changes

Cohort / File(s) Summary
ServiceNow Toolset Configuration
docs/data-sources/builtin-toolsets/servicenow.md, holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py
Added health_check_table config field (default "sys_user") to enable health checks against configurable tables. Updated _perform_health_check to query specified table via dynamic API endpoint. Enhanced error handling for HTTPError (401/403) and ConnectionError with detailed status/body information. Updated logging and success messages to include checked table name.
Toolset Sync Data Model
holmes/utils/holmes_sync_toolsets.py
Replaced inline toolset model dumping with explicit ToolsetDBModel field construction. Removed exclude_none=True filtering and expanded stored fields to include icon_url, status, error, description, docs_url, and installation_instructions.
Test Isolation
tests/integration/test_kubernetes_transformer_execution.py
Updated test setup/teardown to properly preserve and restore original transformer objects instead of using a boolean flag with non-restorability. Transformer state is now captured before registration and restored after test execution.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • arikalon1
  • RoiGlinik
  • moshemorad

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Feb 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ Docker image ready for a2f55d3 (built in 5m 3s)

⚠️ Warning: does not support ARM (ARM images are built on release only - not on every PR)

Use this tag to pull the image for testing.

📋 Copy commands

⚠️ Temporary images are deleted after 30 days. Copy to a permanent registry before using them:

gcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:a2f55d3
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:a2f55d3 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:a2f55d3
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:a2f55d3

Patch 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:a2f55d3

Robusta 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:a2f55d3

@github-actions

github-actions Bot commented Feb 11, 2026 •

Copy link
Copy Markdown
Contributor

✅ Results of HolmesGPT evals

Automatically triggered by commit 1b56c31 on branch claude/epic-kalam

View workflow logs

Results of HolmesGPT evals

  • ask_holmes: 9/9 test cases were successful, 0 regressions
Status Test case Time Turns Tools Cost
✅ 09_crashpod 28.7s 4 10 $0.2052
✅ 101_loki_historical_logs_pod_deleted 34.2s 4 8 $0.2101
✅ 111_pod_names_contain_service 33.9s 5 11 $0.2209
✅ 112_find_pvcs_by_uuid 37.3s 7 6 $0.2301
✅ 12_job_crashing 36.6s 5 12 $0.2459
✅ 176_network_policy_blocking_traffic_no_runbooks 48.9s 6 18 $0.2848
✅ 24_misconfigured_pvc 36.7s 6 13 $0.2365
✅ 43_current_datetime_from_prompt 5.7s 1 — $0.0118
✅ 61_exact_match_counting 16.8s 4 4 $0.1586
Total 31.0s avg 4.7 avg 10.2 avg $1.8040
📖 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: /eval comments 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/epic-kalam -f markers=regression -f filter=

Option 1: Comment on this PR with /eval:

/eval
markers: regression

Or with more options (one per line):

/eval
model: gpt-4o
markers: regression
filter: 09_crashpod
iterations: 5

Run evals on a different branch (e.g., master) for comparison:

/eval
branch: master
markers: regression
Option Description
model Model(s) to test (default: same as automatic runs)
markers Pytest markers (no default - runs all tests!)
filter 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"

🏷️ Valid markers

benchmark, chain-of-causation, compaction, confluence, context_window, coralogix, counting, database, datadog, datetime, easy, elasticsearch, embeds, fast, frontend, grafana-dashboard, hard, integration, kafka, kubernetes, leaked-information, logs, loki, medium, metrics, network, newrelic, no-cicd, numerical, one-test, port-forward, prometheus, question-answer, regression, runbooks, slackbot, storage, toolset-limitation, traces, transparency


Commands: /eval · /rerun · /list

CLI: gh workflow run eval-regression.yaml --repo HolmesGPT/holmesgpt --ref claude/epic-kalam -f markers=regression -f filter=

@Sheeproid Sheeproid closed this Feb 11, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔬 CLI Performance Benchmark

🟢 Startup Time (no LLM)

Measures holmes version execution time (imports + initialization)

Metric PR Master Change
Cold Start 10.48s 10.62s -1.3%
Warm Mean 4.63s 5.27s -12.0%
Warm Min 4.61s 5.18s
Warm Max 4.66s 5.41s

🟡 Full CLI with LLM

Measures holmes ask execution time (OpenRouter + Haiku 4.5)

Metric PR Master Change
Cold Start 23.02s 31.66s -27.3%
Warm Mean 7.79s 8.43s -7.6%
Warm Min 7.49s 8.10s
Warm Max 8.01s 8.67s

PR: a2f55d32 | Master: 640bb0b5 | Iterations: 5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant