ROB-1873-validate-datadog-logs-query-working-as-expected - #780
Conversation
|
Warning Rate limit exceeded@moshemorad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis set of changes introduces new test fixtures and test cases for Datadog integration, refines mock toolset and file management with enhanced thread safety and error handling, and updates test utilities and configuration. It also adjusts type annotations and logic for toolset filtering, modifies test fixture key naming, and adds a new development dependency for handling environment variables in pytest. Additionally, the test execution logic is updated to prioritize test case Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant MockToolsetManager
participant MockFileManager
participant MockableToolWrapper
participant RealToolset
Tester->>MockToolsetManager: Run test (LIVE/GENERATE/MOCK)
MockToolsetManager->>MockFileManager: Load mocks (thread-safe)
MockToolsetManager->>MockableToolWrapper: Wrap toolset
MockableToolWrapper->>MockFileManager: Try to read mock
alt Mock found
MockableToolWrapper-->>Tester: Return mock result
else No mock found
alt GENERATE mode
MockableToolWrapper->>RealToolset: Call live tool
MockableToolWrapper->>MockFileManager: Write new mock
MockableToolWrapper-->>Tester: Return live result
else LIVE mode
MockableToolWrapper->>RealToolset: Call live tool
MockableToolWrapper-->>Tester: Return live result
else MOCK mode
MockableToolWrapper-->>Tester: Raise error (no mock)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
tests/llm/utils/test_case_utils.py (2)
187-188: Remove unusedrequestfield assignment.Line 187 sets
config_dict["request"] = TypeAdapter(InvestigateRequest)butInvestigateTestCasedoesn't have arequestfield. This causes validation to fail withextra="forbid".- config_dict["request"] = TypeAdapter(InvestigateRequest) test_case = TypeAdapter(InvestigateTestCase).validate_python( config_dict )
83-86: Fix extra “request” field in InvestigateTestCase and HealthCheckTestCaseThe
_original_user_promptfield in AskHolmesTestCase is already declared and will be accepted by Pydantic, so no change is needed there. The pipeline failures are due to assigning arequestkey to models that don’t define it:• In
tests/llm/utils/test_case_utils.pyaround line 187:
- Remove or correct
config_dict["request"] = TypeAdapter(InvestigateRequest)since
InvestigateTestCaseexpectsinvestigate_request, notrequest.• In the same file around line 199:
- Remove or correct
config_dict["request"] = TypeAdapter(WorkloadHealthRequest)for
HealthCheckTestCase, which expectsworkload_health_request.Either drop these lines (you already load and assign the correct fields) or add a
request: InvestigateRequest/request: WorkloadHealthRequestfield with an alias in the respective Pydantic models.
🧹 Nitpick comments (2)
tests/llm/fixtures/test_ask_holmes/91_calling_datadog/fetch_pod_logssock-shop_catalogue-db-c948fd796-w4vzl_-3600.txt (1)
1-2: Filename formatting makes globbing brittleWe rely on
<tool>_<resource>_<offset>.txtelsewhere, but this file omits the underscore betweenlogsand the pod identifier:
fetch_pod_logssock-shop_catalogue-db-…← missing_afterlogs.Down-stream helpers such as
MockToolset.load_json_fixture()split on that underscore. Rename tofetch_pod_logs_sock-shop_catalogue-db-c948fd796-w4vzl_-3600.txtfor consistency.tests/llm/fixtures/test_ask_holmes/91_calling_datadog/toolsets.yaml (1)
4-12: Normalise boolean literals for cleaner diffsYAML treats
true/falseandTrue/Falseidentically, but mixing styles (Lines 5, 7 use lowercase; Line 11 uses title-case) produces noisy diffs and lint warnings (yamllint rule truthy). Pick one convention—lowercase is prevalent in the repo.- enabled: True + enabled: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
holmes/core/tools_utils/toolset_utils.py(1 hunks)pyproject.toml(1 hunks)tests/llm/conftest.py(1 hunks)tests/llm/fixtures/test_ask_holmes/88_affinity_like_but_taints/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/91_calling_datadog/conversation_history.json(1 hunks)tests/llm/fixtures/test_ask_holmes/91_calling_datadog/fetch_pod_logssock-shop_catalogue-db-c948fd796-w4vzl_-3600.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/91_calling_datadog/fetch_pod_logssock-shop_catalogue-f7687cb4-zsngc_-3600.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/91_calling_datadog/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/91_calling_datadog/toolsets.yaml(1 hunks)tests/llm/utils/mock_toolset.py(9 hunks)tests/llm/utils/test_case_utils.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
holmes/core/tools_utils/toolset_utils.pytests/llm/conftest.pytests/llm/utils/mock_toolset.pytests/llm/utils/test_case_utils.py
🧬 Code Graph Analysis (2)
holmes/core/tools_utils/toolset_utils.py (2)
holmes/core/tools.py (1)
Toolset(333-475)holmes/plugins/toolsets/logging_utils/logging_api.py (1)
BasePodLoggingToolset(38-46)
tests/llm/conftest.py (1)
tests/llm/utils/mock_toolset.py (1)
MockMode(61-66)
🪛 GitHub Actions: Evaluate LLM test cases
tests/llm/utils/test_case_utils.py
[error] 176-176: Pydantic validation error in AskHolmesTestCase: Extra inputs are not permitted for field '_original_user_prompt'.
[error] 188-188: Pydantic validation error in InvestigateTestCase: Extra inputs are not permitted for field 'request'.
🔇 Additional comments (18)
pyproject.toml (1)
78-78: Add apytest-plugin pin or assert compatibility
pytest-dotenv 0.5.2is fine withpytest>=7, but we just bumpedpytestto8.3.3(Line 67). Up-stream has not yet published wheels that declare compatibility with the newpytest8-series, so installs may warn or break once the plugin importspytest.hookspec.Either:
- Pin
pytest<8in dev extras until the plugin officially supports 8.x, or- Add
pytest-dotenv >=0.5.2,<0.6,!=0.5.2once a compatible release appears.Without this, the CI matrix that installs the dev group can silently skip env-file loading and hide configuration-dependent failures.
tests/llm/fixtures/test_ask_holmes/88_affinity_like_but_taints/test_case.yaml (1)
7-7: All fixtures now useuser_promptRan
rg -n '^user_question:' tests/llm/fixtures– no matches found. No remaininguser_questionfields.tests/llm/fixtures/test_ask_holmes/91_calling_datadog/toolsets.yaml (1)
13-27: Double-check secret interpolation
dd_app_key/dd_api_keyare injected via{{ env.VAR }}Jinja syntax. The fixture runner aborts with aKeyErrorif the variables are missing. Ensurepytest-dotenvloads a.envthat defines these keys during CI; otherwise mark the test withpytest.mark.networkand skip when absent.tests/llm/conftest.py (2)
56-63: LGTM - Proper enforcement of RUN_LIVE requirement for mock generation.The validation logic correctly ensures that mock generation only proceeds when
RUN_LIVEis enabled. The early skip with a clear warning message provides good user feedback when the configuration is invalid.
67-72: LGTM - Clear precedence order for mock mode determination.The reordered logic is more intuitive:
generate_mockstakes precedence to setMockMode.GENERATE, followed byrun_liveforMockMode.LIVE, withMockMode.MOCKas the default fallback. This aligns well with the enhanced mock infrastructure described in the AI summary.holmes/core/tools_utils/toolset_utils.py (2)
19-19: Type annotation correctly generalized.The change from
list[BasePodLoggingToolset]tolist[Toolset]is appropriate since the function now handles wrapped toolsets that may not directly inherit fromBasePodLoggingToolsetbut represent them through theoriginal_toolset_typeattribute.
23-29: Proper handling of wrapped toolsets.The logic correctly checks for
original_toolset_typeattribute before falling back totype(ts). This handles mock toolsets that wrap original toolsets while preserving their original class type information, as mentioned in the past review comments aboutSimplifiedMockToolset.tests/llm/fixtures/test_ask_holmes/91_calling_datadog/conversation_history.json (1)
1-258: Well-structured test fixture for Datadog integration validation.The conversation history JSON properly simulates a realistic interaction flow from Kubernetes pod inspection to Datadog metrics queries. The structure follows the expected format with appropriate
role,content,tool_calls, andtoken_countfields. The progression from kubectl commands to Datadog metric queries that return null results effectively tests the scenario described in the PR title (ROB-1873-validate-datadog-logs-query-working-as-expected).tests/llm/fixtures/test_ask_holmes/91_calling_datadog/fetch_pod_logssock-shop_catalogue-f7687cb4-zsngc_-3600.txt (1)
1-1003: LGTM! Test fixture file is well-structured.The test fixture file follows the expected format with proper JSON metadata and realistic pod log entries. The repetitive health check logs accurately simulate real pod log output.
tests/llm/utils/test_case_utils.py (4)
46-48: Good addition of strict validation withextra="forbid".Adding
Configclasses withextra="forbid"improves test case validation by preventing unknown fields. This helps catch configuration errors early.Also applies to: 72-74
66-68: Useful optional fields for enhanced test metadata.The new fields (
description,generate_mocks,toolsets) provide better test documentation and configuration capabilities.
125-125: Cleaner return statement.Removing the unnecessary cast improves code readability since
load_test_cases()already returns the correct type.
203-206: Good error handling for invalid test folders.Explicitly raising
ValueErrorfor unrecognized test case folders improves debugging compared to silent failures.tests/llm/utils/mock_toolset.py (5)
191-191: Good addition of thread safety for concurrent test execution.Adding a threading lock to
_load_all_mocksprevents race conditions when multiple tests access the mock cache concurrently. This is essential for pytest-xdist parallel execution.Also applies to: 306-309
336-352: Excellent error handling for old mock file format.The detailed error message with PR reference helps users understand the format change and how to fix their mock files. This improves the migration experience.
407-437: Clean refactoring of invocation logic.Extracting
_call_live_invokeand_call_mock_invokeas separate methods improves code organization and readability. The improved error messages in_call_mock_invokehelp distinguish between missing mocks and format issues.
479-479: Good preservation of original toolset type information.Adding
original_toolset_typeallows proper type checking even when toolsets are wrapped, which is important for toolset filtering logic.Also applies to: 636-636
587-587: Correct extension of prerequisite checks to GENERATE mode.Checking prerequisites in GENERATE mode ensures the toolset is properly configured before attempting to generate mocks, preventing failures during mock generation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/llm/fixtures/test_ask_holmes/93_calling_datadog/conversation_history.json (1)
173-175: Typo in user utterance
"can you hosw this as graph?"→"can you show this as graph?"- "content": "can you hosw this as graph?", + "content": "can you show this as graph?",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/llm/fixtures/test_ask_holmes/93_calling_datadog/conversation_history.json(1 hunks)tests/llm/fixtures/test_ask_holmes/93_calling_datadog/fetch_pod_logssock-shop_catalogue-db-c948fd796-w4vzl_-3600.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/93_calling_datadog/fetch_pod_logssock-shop_catalogue-f7687cb4-zsngc_-3600.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/93_calling_datadog/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/93_calling_datadog/toolsets.yaml(1 hunks)tests/llm/utils/test_case_utils.py(7 hunks)
✅ Files skipped from review due to trivial changes (4)
- tests/llm/fixtures/test_ask_holmes/93_calling_datadog/toolsets.yaml
- tests/llm/fixtures/test_ask_holmes/93_calling_datadog/fetch_pod_logssock-shop_catalogue-db-c948fd796-w4vzl_-3600.txt
- tests/llm/fixtures/test_ask_holmes/93_calling_datadog/test_case.yaml
- tests/llm/fixtures/test_ask_holmes/93_calling_datadog/fetch_pod_logssock-shop_catalogue-f7687cb4-zsngc_-3600.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/llm/utils/test_case_utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/llm/fixtures/test_ask_holmes/93_calling_datadog/test_case.yaml(1 hunks)tests/llm/test_ask_holmes.py(1 hunks)tests/llm/utils/test_case_utils.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/llm/fixtures/test_ask_holmes/93_calling_datadog/test_case.yaml
- tests/llm/utils/test_case_utils.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
tests/llm/test_ask_holmes.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: Applies to tests/llm/fixtures/*/ : Mock data must be placed in tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: All new features require unit tests
📚 Learning: in llm-as-judge test cases for holmesgpt, expected outputs should be descriptive rather than prescri...
Learnt from: Sheeproid
PR: robusta-dev/holmesgpt#586
File: tests/llm/fixtures/test_ask_holmes/03_what_is_the_command_to_port_forward/test_case.yaml:4-4
Timestamp: 2025-07-02T10:27:17.231Z
Learning: In LLM-as-judge test cases for HolmesGPT, expected outputs should be descriptive rather than prescriptive when testing for flexible responses like port numbers. Using specific values in expected outputs can cause unnecessary test failures when the AI generates different but equally valid responses.
Applied to files:
tests/llm/test_ask_holmes.py
📚 Learning: when suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first ...
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: When suggesting improvements to environment variable handling in robusta-dev/holmesgpt, check first if validation logic already exists rather than reimplementing it.
Applied to files:
tests/llm/test_ask_holmes.py
📚 Learning: the robusta-dev/holmesgpt codebase has comprehensive existing validation for azure environment varia...
Learnt from: nherment
PR: robusta-dev/holmesgpt#610
File: .github/workflows/llm-evaluation.yaml:39-42
Timestamp: 2025-07-08T08:45:41.069Z
Learning: The robusta-dev/holmesgpt codebase has comprehensive existing validation for Azure environment variables (AZURE_API_BASE, AZURE_API_KEY, AZURE_API_VERSION) and MODEL in tests/llm/utils/classifiers.py, tests/llm/conftest.py, and holmes/core/llm.py. Don't suggest adding redundant validation logic.
Applied to files:
tests/llm/test_ask_holmes.py
🪛 GitHub Actions: Evaluate LLM test cases
tests/llm/test_ask_holmes.py
[error] 276-276: Test 77_liveness_probe_misconfiguration failed: Actual output did not identify that the web app keeps restarting due to liveness probe configured on wrong port. Instead, it reported inability to locate the pod.
[error] 276-276: Test 60_count_less_than failed: Actual output unable to retrieve number of pods with fewer than 3 restarts due to missing data; expected count was 6.
[error] 276-276: Test 39_failed_toolset failed: Actual output did not quote a specific error for RabbitMQ integration failure; only general unavailability message provided.
[error] 276-276: Test 13a_pending_node_selector_basic failed: Actual output did not mention 'node selector mismatch' but gave generic reasons for pod Pending state and suggested next steps.
[error] 276-276: Test 02_what_is_wrong_with_pod failed: Actual output unable to retrieve pod info due to missing data; expected output was pod killed due to out of memory.
[error] 276-276: Test 24_misconfigured_pvc failed: Actual output unable to locate Redis-related resources or identify issue due to missing data; expected output was PersistentVolumeClaim misconfiguration.
[error] 276-276: Test 79_configmap_mount_issue failed: Actual output unable to retrieve details about app-server pod; expected output was app server stuck in ContainerCreating due to missing ConfigMap.
[error] 276-276: Test 93_calling_datadog[2] failed: Actual output indicated log access not configured; expected output was logs showing consistent health checks with no errors.
[error] 276-276: Test 93_calling_datadog[1] failed: Actual output indicated log access not configured and suggested enabling logging tools; expected output was logs showing consistent health checks with no errors.
[error] 276-276: Test 93_calling_datadog[0] failed: Actual output indicated Datadog logs toolset not enabled with setup instructions; expected output was logs showing consistent health checks with no errors.
[error] 276-276: Test 03_what_is_the_command_to_port_forward failed: Actual output unable to locate Grafana pod in namespace or cluster; expected output required actual pod name, port, and full kubectl port-forward command.
[error] 276-276: Test 88_affinity_like_but_taints failed: Actual output unable to locate database-primary deployment or related info; expected output required mention of missing tolerations for node taints and pod status details.
[error] 276-276: Test 42_dns_issues_steps_new_tools failed: Actual output unable to verify DNS pods running due to missing data; expected output was issue due to network policy named default-deny-egress.
[error] 276-276: Test 86_configmap_like_but_secret failed: Actual output unable to retrieve info about frontend deployment; expected output was issue identified as missing Secret mount (frontend-api-keys).
[error] 276-276: Test 24a_misconfigured_pvc_basic failed: Actual output shows Redis pod stuck in Pending state with no mention of PersistentVolumeClaim misconfiguration as expected.
[error] 276-276: Test 05_image_version failed: Actual output unable to locate pod 'sea-turtle'; expected output was image version of pod is '1.23.4'.
[error] 276-276: Test 83_secret_not_found failed: Actual output unable to locate database pod or related resources; expected output was pod failing to start due to missing Secret 'database-secret'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (1)
tests/llm/test_ask_holmes.py (1)
304-306: LGTM: Test type prioritization logic is correct.The change appropriately prioritizes the test case's
test_typeattribute over the environment variable, which provides better test-level control. The fallback chain (test_case.test_type→ASK_HOLMES_TEST_TYPEenv var →"cli"default) is logical and maintains backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/llm/utils/mock_toolset.py (1)
441-446: Consider the --generate flag behavior.The current implementation reuses existing mocks in GENERATE mode, which may not match user expectations as discussed in the previous review.
🧹 Nitpick comments (1)
tests/llm/utils/mock_toolset.py (1)
585-587: Update comment to reflect GENERATE mode.The prerequisite check now runs in both LIVE and GENERATE modes, but the comment only mentions LIVE mode.
- # Only check prerequisites in LIVE mode - for MOCK/GENERATE modes we don't need real connections + # Check prerequisites in LIVE and GENERATE modes - only MOCK mode doesn't need real connections
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/llm/fixtures/test_ask_holmes/93_calling_datadog/test_case.yaml(1 hunks)tests/llm/test_ask_holmes.py(1 hunks)tests/llm/utils/mock_toolset.py(9 hunks)tests/llm/utils/test_case_utils.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/llm/test_ask_holmes.py
- tests/llm/utils/test_case_utils.py
- tests/llm/fixtures/test_ask_holmes/93_calling_datadog/test_case.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.py: ALWAYS place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting (configured in pyproject.toml)
Type hints required (mypy configuration in pyproject.toml)
Files:
tests/llm/utils/mock_toolset.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: New toolsets require integration tests with mocks
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: Applies to tests/llm/fixtures/*/ : Mock data must be placed in tests/llm/fixtures/{test_name}/
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: All new features require unit tests
📚 Learning: new toolsets require integration tests with mocks...
Learnt from: CR
PR: robusta-dev/holmesgpt#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T07:25:36.018Z
Learning: New toolsets require integration tests with mocks
Applied to files:
tests/llm/utils/mock_toolset.py
📚 Learning: the init_config method in toolsets should be idempotent - safely callable multiple times without err...
Learnt from: nherment
PR: robusta-dev/holmesgpt#535
File: holmes/plugins/toolsets/bash/bash_toolset.py:207-209
Timestamp: 2025-06-24T05:51:04.543Z
Learning: The init_config method in toolsets should be idempotent - safely callable multiple times without errors. self.config should maintain consistent typing (not alternate between dict and config object types) throughout the object lifecycle.
Applied to files:
tests/llm/utils/mock_toolset.py
🔇 Additional comments (6)
tests/llm/utils/mock_toolset.py (6)
8-10: LGTM!The imports are correctly placed at the top of the file and are necessary for the new functionality.
65-65: Improved documentation clarity.The updated comment better describes the GENERATE mode behavior.
191-191: Excellent thread safety implementation.The threading lock correctly protects the mock cache from concurrent access issues.
Also applies to: 306-309
334-352: Well-implemented format migration error handling.The error detection and messaging for old format mock files is clear and actionable, providing users with specific guidance on how to fix the issue.
407-467: Excellent refactoring of invoke logic.The separation of live and mock invocation into dedicated methods improves code clarity and maintainability. The enhanced error messages help users diagnose mock-related issues more effectively.
479-479: Good design for preserving type information.The
original_toolset_typeattribute correctly preserves the original toolset class type through the wrapper pattern, enabling proper type detection downstream.Also applies to: 636-636
2 participants
No description provided.