ROB-2206 history changes improvements #1049
Conversation
WalkthroughRenames the Robusta configuration-changes tool to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant Toolset as RobustaToolset
participant Tool as FetchConfigurationChangesMetadata
participant DAL as SupabaseDal
participant DB as Supabase
Caller->>Toolset: invoke fetch_configuration_changes_metadata(start,end, namespace?, workload?, limit?)
Toolset->>Tool: execute tool with params
Tool->>DAL: get_configuration_changes_metadata(start, end, limit, workload, ns)
DAL->>DB: SELECT id,title,subject_name,subject_namespace,subject_type,description,starts_at,ends_at WHERE time_range AND optional filters LIMIT n
DB-->>DAL: rows (metadata)
DAL-->>Tool: metadata list (or None)
Tool-->>Toolset: metadata result
Toolset-->>Caller: metadata result
Note right of Caller #ADD8E6: For full change content call fetch_finding_by_id(change_id)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ 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). (3)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (10)
docs/data-sources/builtin-toolsets/robusta.md(1 hunks)holmes/core/supabase_dal.py(1 hunks)holmes/plugins/toolsets/robusta/robusta.py(7 hunks)holmes/plugins/toolsets/robusta/robusta_instructions.jinja2(1 hunks)tests/llm/fixtures/test_ask_holmes/93_events_since_specific_date/fetch_configuration_changes2025-06-12T23_59_59Z_2025-06-12T00_00_00Z.txt(1 hunks)tests/llm/fixtures/test_investigate/08_memory_pressure/fetch_configuration_changes.txt(1 hunks)tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/fetch_configuration_changes.txt(1 hunks)tests/llm/fixtures/test_investigate/12_KubePodNotReady/fetch_configuration_changes.txt(1 hunks)tests/llm/fixtures/test_investigate/14_tempo/fetch_configuration_changes.txt(1 hunks)tests/llm/fixtures/test_investigate/17_investigate_correct_date/fetch_configuration_changes.txt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/llm/fixtures/test_investigate/14_tempo/fetch_configuration_changes.txttests/llm/fixtures/test_investigate/08_memory_pressure/fetch_configuration_changes.txttests/llm/fixtures/test_ask_holmes/93_events_since_specific_date/fetch_configuration_changes2025-06-12T23_59_59Z_2025-06-12T00_00_00Z.txttests/llm/fixtures/test_investigate/17_investigate_correct_date/fetch_configuration_changes.txttests/llm/fixtures/test_investigate/12_KubePodNotReady/fetch_configuration_changes.txttests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/fetch_configuration_changes.txt
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/supabase_dal.pyholmes/plugins/toolsets/robusta/robusta.py
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
In MkDocs content, always add a blank line between a header or bold text and a following list so lists render correctly
Files:
docs/data-sources/builtin-toolsets/robusta.md
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/robusta/robusta_instructions.jinja2holmes/plugins/toolsets/robusta/robusta.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/robusta/robusta.py (1)
holmes/core/supabase_dal.py (1)
get_configuration_changes_metadata(240-291)
⏰ 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). (4)
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: llm_evals
🔇 Additional comments (8)
tests/llm/fixtures/test_investigate/14_tempo/fetch_configuration_changes.txt (1)
1-3: LGTM! Consistent fixture update.The tool_name update to
fetch_configuration_changes_metadataaligns with the PR's objective to expose metadata-only configuration changes.docs/data-sources/builtin-toolsets/robusta.md (1)
59-63: LGTM! Documentation accurately updated.The capabilities table correctly reflects the renamed tool and the new filtering capabilities (namespace and workload filtering) introduced in this PR.
tests/llm/fixtures/test_investigate/17_investigate_correct_date/fetch_configuration_changes.txt (1)
1-2: LGTM! Consistent fixture update.The tool_name update matches the pattern across all test fixtures in this PR.
tests/llm/fixtures/test_investigate/08_memory_pressure/fetch_configuration_changes.txt (1)
1-3: LGTM! Consistent fixture update.The tool_name update is consistent with the PR-wide rename to the metadata-enabled variant.
tests/llm/fixtures/test_ask_holmes/93_events_since_specific_date/fetch_configuration_changes2025-06-12T23_59_59Z_2025-06-12T00_00_00Z.txt (1)
1-10: LGTM! Consistent fixture update.The tool_name update is consistent across all fixtures. The payload structure and data remain unchanged.
tests/llm/fixtures/test_investigate/10_KubeDeploymentReplicasMismatch/fetch_configuration_changes.txt (1)
1-3: LGTM! Consistent fixture update.The tool_name update aligns with the metadata-focused approach introduced in this PR.
tests/llm/fixtures/test_investigate/12_KubePodNotReady/fetch_configuration_changes.txt (1)
1-3: LGTM! Consistent fixture update.The tool_name update is consistent with all other test fixtures in this PR.
holmes/plugins/toolsets/robusta/robusta_instructions.jinja2 (1)
2-5: LGTM! Instructions correctly updated to guide metadata-first workflow.The updated instructions properly:
- Reference the renamed tool
fetch_configuration_changes_metadata- Direct the LLM to use
fetch_finding_by_idfor full change details (line 3), which aligns with the PR's objective to return only metadata and hint the LLM to fetch detailed info separately- Maintain mandatory usage requirements (lines 4-5)
There was a problem hiding this comment.
Actionable comments posted: 2
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/robusta/robusta.py (1)
165-165: Fix incorrect parameter description.The description for
END_TIMEsays "The starting time boundary" but it should say "The ending time boundary".Apply this diff to fix the description:
- description="The starting time boundary for the search period. String in RFC3339 format.", + description="The ending time boundary for the search period. String in RFC3339 format.",
🧹 Nitpick comments (1)
holmes/plugins/toolsets/robusta/robusta.py (1)
193-196: Make limit handling more robust.The
oroperator treats0as falsy, soparams.get("limit") or DEFAULT_LIMIT_CHANGE_ROWSwill use the default when limit is explicitly set to 0. Additionally, negative values won't be properly bounded.Apply this diff for more explicit handling:
- limit=min( - params.get("limit") or DEFAULT_LIMIT_CHANGE_ROWS, - MAX_LIMIT_CHANGE_ROWS, - ), + raw_limit = params.get("limit") + effective_limit = ( + DEFAULT_LIMIT_CHANGE_ROWS + if raw_limit is None + else max(1, min(raw_limit, MAX_LIMIT_CHANGE_ROWS)) + ) + limit=effective_limit,This ensures:
Nonedefaults to 1000or negative values are clamped to 1- Values above 200 are clamped to 200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/robusta/robusta.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/robusta/robusta.py
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/robusta/robusta.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/robusta/robusta.py (2)
holmes/core/tools.py (2)
Tool(173-365)ToolParameter(155-161)holmes/core/supabase_dal.py (1)
get_configuration_changes_metadata(240-291)
🔇 Additional comments (8)
holmes/plugins/toolsets/robusta/robusta.py (8)
22-23: LGTM!The constants are well-named and appropriately scoped at module level.
32-32: LGTM!The description accurately reflects that findings can include configuration changes.
80-80: LGTM!Including params in the one-liner provides better context for debugging and logging.
147-157: LGTM!The rename to
FetchConfigurationChangesMetadataand updated description clearly communicate the tool's purpose and guide users to fetch full details viafetch_finding_by_id.
188-200: LGTM!The method correctly calls
get_configuration_changes_metadatawith the new filtering parameters, assuming the limit handling suggestion is addressed.
202-224: LGTM!The error handling covers all cases (success, no data, exceptions) and provides clear error messages.
226-227: LGTM!Including params in the one-liner provides better context for debugging and logging.
247-250: LGTM!The toolset correctly instantiates
FetchConfigurationChangesMetadatato align with the class rename.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
holmes/plugins/toolsets/robusta/robusta.py (2)
169-173: Fix typo in parameter description.Line 170 contains a typo: "confgiuration" should be "configuration".
174-178: Fix typo in parameter description.Line 175 contains a typo: "confgiuration" should be "configuration".
🧹 Nitpick comments (1)
holmes/plugins/toolsets/robusta/robusta.py (1)
193-199: The TypeError risk is now addressed, but consider validating negative values.The use of
orcorrectly handles the None case from the past review (None defaults to 100), preventing the TypeError. However, negative values are not validated and would be passed to the DAL, potentially causing unexpected behavior.If you'd like to be more defensive, consider adding validation:
- return self._dal.get_configuration_changes_metadata( - start_datetime=params["start_datetime"], - end_datetime=params["end_datetime"], - limit=min( - params.get("limit") or DEFAULT_LIMIT_CHANGE_ROWS, - MAX_LIMIT_CHANGE_ROWS, - ), - ns=params.get("namespace"), - workload=params.get("workload"), - ) + raw_limit = params.get("limit") or DEFAULT_LIMIT_CHANGE_ROWS + effective_limit = max(1, min(raw_limit, MAX_LIMIT_CHANGE_ROWS)) + return self._dal.get_configuration_changes_metadata( + start_datetime=params["start_datetime"], + end_datetime=params["end_datetime"], + limit=effective_limit, + ns=params.get("namespace"), + workload=params.get("workload"), + )This ensures the limit is always between 1 and 200.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/robusta/robusta.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/plugins/toolsets/robusta/robusta.py
holmes/plugins/toolsets/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Toolsets must be located as holmes/plugins/toolsets/{name}.yaml or holmes/plugins/toolsets/{name}/
Files:
holmes/plugins/toolsets/robusta/robusta.py
🧬 Code graph analysis (1)
holmes/plugins/toolsets/robusta/robusta.py (2)
holmes/core/tools.py (2)
Tool(173-365)ToolParameter(155-161)holmes/core/supabase_dal.py (1)
get_configuration_changes_metadata(240-291)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (3)
holmes/plugins/toolsets/robusta/robusta.py (3)
22-23: LGTM!The constants are clearly named and provide sensible defaults for limiting configuration change queries.
147-157: LGTM!The rename to
FetchConfigurationChangesMetadataand the updated description clearly communicate that this tool returns metadata, with guidance to usefetch_finding_by_idfor detailed information. This is a good pattern for limiting initial data retrieval.
249-249: LGTM!The instantiation correctly uses the renamed class
FetchConfigurationChangesMetadata.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
holmes/core/supabase_dal.py (1)
252-268: Consider adding explicit ordering to ensure predictable results.Without an
.order()clause, the database may return rows in an arbitrary order, making pagination or result consistency unpredictable across calls.Apply this diff to add ordering by creation date (descending, to show most recent first):
.eq("finding_type", "configuration_change") .gte("creation_date", start_datetime) .lte("creation_date", end_datetime) + .order("creation_date", desc=True) .limit(limit) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/supabase_dal.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/supabase_dal.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). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: build
🔇 Additional comments (2)
holmes/core/supabase_dal.py (2)
240-247: LGTM! Method signature is well-structured.The renamed method with added filtering parameters and type hints aligns with the PR objectives. The signature is clear and follows Python typing conventions.
276-293: LGTM! Query execution and error handling are correct.The execution flow, null-data check, exception handling, and logging are appropriate for this DAL method.
2 participants
add support for workload and namespace filtering.
limit number of rows returned to a maximum of 200.
return only metadata info on change function and hint llm to use fetch_finding to get detailed change info.