Conversation
The toolset status cache (toolsets_status.json) was never invalidated when the user modified their config file (e.g., enabling/disabling toolsets, changing custom_toolsets). The cache check only looked at whether the file existed or if --refresh-toolsets was passed. Add a config fingerprint (MD5 hash of toolset-affecting config values) that is stored in the cache file. On load, the fingerprint is compared to detect config changes and automatically trigger a refresh. This also handles custom toolset file content changes via mtime tracking. The cache format is backward-compatible: legacy format (plain list) is detected and triggers a refresh to migrate to the new format. https://claude.ai/code/session_01Pmbke45TMd7nHqm1MNfVd3 Signed-off-by: Claude <noreply@anthropic.com>
|
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:f60125b
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:f60125b me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:f60125b
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:f60125bPatch 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:f60125bRobusta 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:f60125b |
✅ Results of HolmesGPT evalsAutomatically triggered by commit b74bd7d on branch Results of HolmesGPT evals
📖 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" 🏷️ Valid markers
Commands: CLI: |
WalkthroughThe changes introduce a configuration fingerprint mechanism to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ 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 |
🔬 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@holmes/core/toolset_manager.py`:
- Around line 125-131: The _read_cached_toolsets function should defensively
handle missing/corrupt cache files: wrap the open + json.load block in a
try/except that catches OSError and json.JSONDecodeError (and ValueError if
needed), log a short warning via the instance logger (e.g., self.logger.warning)
with the error, and return an empty list so callers like
load_toolset_with_status will treat the cache as missing and trigger a refresh;
keep the existing legacy-format handling after the try block.
In `@tests/core/test_toolset_manager.py`:
- Around line 140-158: Add unit tests for stale-cache and fingerprint logic:
create tests that write a cache file with a non-matching "config_fingerprint"
and assert that _is_cache_stale() returns True and that
load_toolset_with_status() triggers refresh_toolset_status (mock refresh);
create a test that writes a legacy plain JSON list as the cache and assert it is
treated as stale; and add deterministic fingerprint tests that call
_compute_config_fingerprint() twice on the same config (assert equal) and once
after mutating any config field (assert different). Reference the existing test
harness that sets toolset_manager.toolset_status_location and uses
load_toolset_with_status(), mocking refresh_toolset_status where needed.
🧹 Nitpick comments (2)
holmes/core/toolset_manager.py (2)
81-105: Consider usinghashlib.sha256instead ofmd5to satisfy the Ruff S324 lint rule.MD5 isn't being used for security here (only cache fingerprinting), so it's functionally fine. However, Ruff flags it (
S324: Probable use of insecure hash functions), andsha256is equally fast for small payloads and avoids the lint suppression.♻️ Swap to sha256
- config_str = json.dumps(fingerprint_data, sort_keys=True, default=str) - return hashlib.md5(config_str.encode()).hexdigest() + config_str = json.dumps(fingerprint_data, sort_keys=True, default=str) + return hashlib.sha256(config_str.encode()).hexdigest()
107-123:_is_cache_stalereads and parses the entire cache file, then_read_cached_toolsetsre-reads it — consider unifying.When the cache is not stale, the JSON is parsed twice: once inside
_is_cache_stale(Line 111) and again inside_read_cached_toolsets(Line 128, called from Line 350). For a CLI tool this is unlikely to be a real performance problem, but it's a straightforward refactor to parse once and reuse the result.Also, Ruff TRY300 suggests the
return Falseon Line 121 would be better inside anelseblock — this is a minor style nit.
| def _read_cached_toolsets(self) -> List[dict[str, Any]]: | ||
| """Read cached toolset status, handling both legacy and new formats.""" | ||
| with open(self.toolset_status_location, "r") as f: | ||
| raw_data = json.load(f) | ||
| if isinstance(raw_data, list): | ||
| return raw_data | ||
| return raw_data.get("toolsets", []) |
There was a problem hiding this comment.
_read_cached_toolsets has no error handling — verify this is intentional.
If the cache file is missing or corrupt when this method is called (e.g., deleted between the _is_cache_stale check and this read), an unhandled OSError or JSONDecodeError will propagate up to load_toolset_with_status. Since this is a CLI tool and the race window is tiny, this is low-risk, but wrapping in a try/except that falls back to triggering a refresh would make it more robust.
🛡️ Proposed defensive handling
def _read_cached_toolsets(self) -> List[dict[str, Any]]:
"""Read cached toolset status, handling both legacy and new formats."""
- with open(self.toolset_status_location, "r") as f:
- raw_data = json.load(f)
- if isinstance(raw_data, list):
- return raw_data
- return raw_data.get("toolsets", [])
+ try:
+ with open(self.toolset_status_location, "r") as f:
+ raw_data = json.load(f)
+ if isinstance(raw_data, list):
+ return raw_data
+ return raw_data.get("toolsets", [])
+ except (json.JSONDecodeError, OSError):
+ logging.warning("Failed to read cached toolsets, returning empty list")
+ return []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _read_cached_toolsets(self) -> List[dict[str, Any]]: | |
| """Read cached toolset status, handling both legacy and new formats.""" | |
| with open(self.toolset_status_location, "r") as f: | |
| raw_data = json.load(f) | |
| if isinstance(raw_data, list): | |
| return raw_data | |
| return raw_data.get("toolsets", []) | |
| def _read_cached_toolsets(self) -> List[dict[str, Any]]: | |
| """Read cached toolset status, handling both legacy and new formats.""" | |
| try: | |
| with open(self.toolset_status_location, "r") as f: | |
| raw_data = json.load(f) | |
| if isinstance(raw_data, list): | |
| return raw_data | |
| return raw_data.get("toolsets", []) | |
| except (json.JSONDecodeError, OSError): | |
| logging.warning("Failed to read cached toolsets, returning empty list") | |
| return [] |
🤖 Prompt for AI Agents
In `@holmes/core/toolset_manager.py` around lines 125 - 131, The
_read_cached_toolsets function should defensively handle missing/corrupt cache
files: wrap the open + json.load block in a try/except that catches OSError and
json.JSONDecodeError (and ValueError if needed), log a short warning via the
instance logger (e.g., self.logger.warning) with the error, and return an empty
list so callers like load_toolset_with_status will treat the cache as missing
and trigger a refresh; keep the existing legacy-format handling after the try
block.
| toolset_manager.toolset_status_location = cache_path | ||
| cache_data = { | ||
| "config_fingerprint": toolset_manager._config_fingerprint, | ||
| "toolsets": [ | ||
| { | ||
| "name": "test", | ||
| "status": "enabled", | ||
| "enabled": True, | ||
| "type": "built-in", | ||
| "path": None, | ||
| "error": None, | ||
| } | ||
| ], | ||
| } | ||
| with open(cache_path, "w") as f: | ||
| json.dump(cache_data, f) | ||
| toolset_manager.toolset_status_location = cache_path | ||
| result = toolset_manager.load_toolset_with_status() | ||
| assert result[0].name == "test" | ||
| assert result[0].enabled is True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '_is_cache_stale\|_compute_config_fingerprint\|_read_cached_toolsets\|config_fingerprint' --type=py -g '*test*'Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Also check the actual test file to see what tests exist
cat -n tests/core/test_toolset_manager.py | head -200Repository: HolmesGPT/holmesgpt
Length of output: 8721
🏁 Script executed:
# Find the toolset_manager implementation to understand what was changed
find . -type f -name "toolset_manager.py" | grep -v testRepository: HolmesGPT/holmesgpt
Length of output: 95
🏁 Script executed:
# Get the toolset_manager.py implementation
wc -l ./holmes/core/toolset_manager.pyRepository: HolmesGPT/holmesgpt
Length of output: 99
🏁 Script executed:
# View the toolset_manager implementation focusing on fingerprint and cache logic
cat -n ./holmes/core/toolset_manager.py | grep -A 5 -B 5 'fingerprint\|_is_cache_stale\|_read_cached_toolsets' | head -100Repository: HolmesGPT/holmesgpt
Length of output: 4071
🏁 Script executed:
# Get a better view of the load_toolset_with_status and refresh_toolset_status methods
ast-grep --pattern $'def load_toolset_with_status($_) {
$$$
}'Repository: HolmesGPT/holmesgpt
Length of output: 45
🏁 Script executed:
# Check the rest of the test file for any tests covering stale cache or fingerprint mismatch
tail -n +160 tests/core/test_toolset_manager.pyRepository: HolmesGPT/holmesgpt
Length of output: 17130
🏁 Script executed:
# Look at the full load_toolset_with_status method implementation
sed -n '320,360p' ./holmes/core/toolset_manager.pyRepository: HolmesGPT/holmesgpt
Length of output: 1644
Add tests for stale cache detection and fingerprint logic to cover the core feature of this PR.
The test at lines 140–158 only exercises the cache-hit path (matching fingerprint). The stale-cache detection logic—the main behavioral change—lacks coverage:
- Stale fingerprint triggers refresh: Cache with a different fingerprint should cause
_is_cache_stale()to returnTrueand triggerrefresh_toolset_status. - Legacy (list) format triggers refresh: A plain JSON list in the cache file should be detected as stale.
_compute_config_fingerprintdeterminism: Same config must produce the same fingerprint; changing any config field must produce a different one.
Per coding guidelines, all new features require unit tests.
🤖 Prompt for AI Agents
In `@tests/core/test_toolset_manager.py` around lines 140 - 158, Add unit tests
for stale-cache and fingerprint logic: create tests that write a cache file with
a non-matching "config_fingerprint" and assert that _is_cache_stale() returns
True and that load_toolset_with_status() triggers refresh_toolset_status (mock
refresh); create a test that writes a legacy plain JSON list as the cache and
assert it is treated as stale; and add deterministic fingerprint tests that call
_compute_config_fingerprint() twice on the same config (assert equal) and once
after mutating any config field (assert different). Reference the existing test
harness that sets toolset_manager.toolset_status_location and uses
load_toolset_with_status(), mocking refresh_toolset_status where needed.
This branch has not been deployed
2 participants
The toolset status cache (toolsets_status.json) was never invalidated
when the user modified their config file (e.g., enabling/disabling
toolsets, changing custom_toolsets). The cache check only looked at
whether the file existed or if --refresh-toolsets was passed.
Add a config fingerprint (MD5 hash of toolset-affecting config values)
that is stored in the cache file. On load, the fingerprint is compared
to detect config changes and automatically trigger a refresh. This
also handles custom toolset file content changes via mtime tracking.
The cache format is backward-compatible: legacy format (plain list) is
detected and triggers a refresh to migrate to the new format.
https://claude.ai/code/session_01Pmbke45TMd7nHqm1MNfVd3
Signed-off-by: Claude noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests