Repository navigation
Conversation
…t default `_parse_instances` used `if not raw:`, which treats an explicit empty `instances: []` the same as a missing `instances` key and synthesizes a `default` child from the top-level globals. That bypasses the "No instances configured" path in `_aggregate()` and can leave a routable endpoint enabled when the config explicitly declared zero instances. Distinguish absence (`raw is None` -> flat `default`) from an explicit empty list (`raw == []` -> zero instances). Add tests for both shapes. Signed-off-by: Claude <noreply@anthropic.com>
✅ Results of HolmesGPT evalsAutomatically triggered by commit 6571983 on branch Results of HolmesGPT evals
Benchmark Comparison DetailsMaster baseline: latest master-* experiment (post-merge regression eval)
Benchmark baseline: latest ci-benchmark experiment on master
Time comparison (seconds):
Cost comparison:
Total tokens comparison:
Cached tokens comparison:
Turns comparison:
Tool calls comparison:
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: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR refines multi-instance configuration parsing in the toolset to handle an edge case: distinguishing an explicitly empty ChangesMulti-Instance Configuration Parsing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
✅ 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:78b3e9d9d
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:78b3e9d9d me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:78b3e9d9d
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:78b3e9d9d
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:78b3e9d9d
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:78b3e9d9d me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:78b3e9d9d
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:78b3e9d9dPatch 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:78b3e9d9d \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:78b3e9d9dRobusta 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:78b3e9d9d \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:78b3e9d9d |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This branch has not been deployed
2 participants
Problem
_parse_instancesinholmes/plugins/toolsets/multi_instance.pyusedif not raw:to detect the flat (single-instance) config shape. Because an empty list is falsy, this treats an explicitinstances: []identically to a missinginstanceskey — synthesizing adefaultchild from the top-level globals.That bypasses the
"No instances configured"path in_aggregate()and can leave a routable endpoint silently enabled when the config explicitly declared zero instances.Fix
Distinguish absence from emptiness:
raw is None(key missing) → flatdefaultinstance (unchanged, backwards compatible)raw == [](explicit empty list) → zero instances, flows to_aggregate()'s"No instances configured"resultraw = config.get("instances") -if not raw: +if raw is None: flat = {k: v for k, v in config.items() if k != "instances"} return [("default", flat)] if not isinstance(raw, list): raise ValueError("`instances` must be a list") +if not raw: + return []Tests
Added two cases to
TestHealthAggregation:test_explicit_empty_instances_yields_zero_instances—instances: [](with top-level globals present) → prereqs fail with"No instances configured", no children, no tools.test_missing_instances_key_still_flat_default— absent key keeps the flatdefaultshape.All 24 tests in
tests/plugins/toolsets/test_multi_instance.pypass.Context
Found by CodeRabbit while reviewing #2148 (it surfaced in that PR's diff only because of a master merge). The line is unrelated to #2148's approval refactor and originates in #2115, so it's split out here as a focused fix.
https://claude.ai/code/session_01FoqM3sjnrRgPjdqYdqutzK
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
instances: []) and missing instances key. Empty instances now correctly indicate "no instances configured," while maintaining backward compatibility.Tests