feat: Add flux to built-in toolsets #1208
Conversation
|
|
WalkthroughAdds Flux CD support across docs, a new Bash FluxCommand with allow/deny command lists, a flux/core toolset configuration, test fixtures modeling a Flux sync-failure scenario, and unit tests validating Flux CLI parsing and safety. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (11)
🧰 Additional context used🪛 LanguageTooldocs/data-sources/builtin-toolsets/flux.md[style] ~71-~71: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 3454 characters long) (EN_EXCESSIVE_EXCLAMATION) 🪛 markdownlint-cli2 (0.18.1)docs/data-sources/builtin-toolsets/flux.md41-41: Code block style (MD046, code-block-style) 59-59: Code block style (MD046, code-block-style) 70-70: Code block style (MD046, code-block-style) 74-74: Code block style (MD046, code-block-style) 🪛 Ruff (0.14.8)holmes/plugins/toolsets/bash/flux/__init__.py41-41: Unused method argument: (ARG002) 41-41: Unused method argument: (ARG002) 52-52: Unused method argument: (ARG002) 52-52: Unused method argument: (ARG002) 🔇 Additional comments (11)
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: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
docs/data-sources/builtin-toolsets/.nav.yml(1 hunks)docs/data-sources/builtin-toolsets/flux.md(1 hunks)docs/data-sources/builtin-toolsets/index.md(1 hunks)holmes/plugins/toolsets/bash/flux/__init__.py(1 hunks)holmes/plugins/toolsets/bash/flux/constants.py(1 hunks)holmes/plugins/toolsets/bash/parse_command.py(2 hunks)holmes/plugins/toolsets/flux.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/flux_check.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/flux_events.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/flux_get_all.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/flux_get_kustomizations.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/flux_get_sources_git.txt(1 hunks)tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/test_case.yaml(1 hunks)tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/toolsets.yaml(1 hunks)tests/plugins/toolsets/bash/test_flux_commands.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
holmes/plugins/toolsets/bash/flux/__init__.py (5)
holmes/plugins/toolsets/bash/common/bash_command.py (1)
BashCommand(9-38)holmes/plugins/toolsets/bash/common/config.py (1)
BashExecutorConfig(13-14)holmes/plugins/toolsets/bash/common/stringify.py (1)
escape_shell_args(10-38)holmes/plugins/toolsets/bash/common/validators.py (1)
validate_command_and_operations(28-44)holmes/plugins/toolsets/bash/parse_command.py (2)
validate_command(88-94)stringify_command(97-111)
tests/plugins/toolsets/bash/test_flux_commands.py (2)
holmes/plugins/toolsets/bash/common/config.py (1)
BashExecutorConfig(13-14)holmes/plugins/toolsets/bash/parse_command.py (1)
make_command_safe(155-179)
holmes/plugins/toolsets/bash/parse_command.py (1)
holmes/plugins/toolsets/bash/flux/__init__.py (1)
FluxCommand(16-60)
🪛 Ruff (0.14.8)
holmes/plugins/toolsets/bash/flux/__init__.py
41-41: Unused method argument: original_command
(ARG002)
41-41: Unused method argument: config
(ARG002)
52-52: Unused method argument: original_command
(ARG002)
52-52: Unused method argument: config
(ARG002)
🔇 Additional comments (19)
docs/data-sources/builtin-toolsets/.nav.yml (1)
15-15: LGTM!The navigation entry is correctly formatted and alphabetically positioned between Docker and GitHub.
docs/data-sources/builtin-toolsets/index.md (1)
21-21: LGTM!The Flux CD entry is properly formatted with an appropriate icon (material-source-branch) and correctly positioned alphabetically.
tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/toolsets.yaml (1)
1-7: LGTM!The toolset configuration correctly enables the necessary toolsets (kubernetes/core, flux/core, kubernetes/logs) for the Flux sync failure test scenario.
tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/test_case.yaml (1)
1-6: LGTM!The test case is well-defined with a clear user prompt and specific, measurable expected output criteria that align with the accompanying fixture data showing SSH authentication failures.
tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/flux_get_sources_git.txt (1)
1-8: LGTM!The fixture accurately represents a realistic Flux GitRepository status output with one source experiencing SSH authentication failures and another successfully reconciled. The format is consistent with the expected tool invocation structure.
tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/flux_check.txt (1)
1-12: LGTM!The fixture correctly represents a successful Flux health check, showing all controllers as ready. This provides a good baseline for troubleshooting scenarios where Flux itself is healthy but specific resources are failing.
docs/data-sources/builtin-toolsets/flux.md (2)
1-130: Excellent documentation!The Flux CD toolset documentation is comprehensive, well-structured, and covers all essential aspects:
- Clear prerequisites and multi-platform installation instructions
- Configuration examples for both Holmes CLI and Helm deployments
- Realistic example questions for common troubleshooting scenarios
- Extensive capabilities table covering 26 Flux tools
- Helpful troubleshooting guidance for common issues
87-114: All documented Flux tools are implemented in the toolset configuration. The table lists 25 tools (not 26 as stated), and verification confirms all 25 are present in holmes/plugins/toolsets/flux.yaml with proper command definitions and descriptions.tests/llm/fixtures/test_ask_holmes/176_flux_sync_failure/flux_events.txt (1)
1-12: LGTM!The fixture effectively models a realistic Flux event timeline, showing repeated reconciliation failures due to SSH authentication issues and the cascading impact on dependent Kustomizations. The events are consistent with the other fixtures in this test scenario.
holmes/plugins/toolsets/bash/parse_command.py (1)
12-12: LGTM! Clean integration of FluxCommand.The FluxCommand is properly imported and added to AVAILABLE_COMMANDS following the established pattern for other toolsets (kubectl, argocd, aws, etc.).
Also applies to: 37-37
holmes/plugins/toolsets/flux.yaml (1)
1-44: Excellent toolset documentation and metadata.The LLM instructions provide clear context about Flux CD concepts, investigation workflow, and common issues. The prerequisite check and documentation links are helpful.
holmes/plugins/toolsets/bash/flux/constants.py (2)
1-48: Well-designed allowlist for read-only Flux operations.The ALLOWED_FLUX_COMMANDS properly restricts access to safe, read-only operations:
- Status queries (get, check, stats)
- Observability (logs, events, trace, tree)
- Read-only inspection (debug, diff, export)
The nested dictionary structure allows for future fine-grained control of subcommands.
50-76: Comprehensive blocklist prevents state-modifying operations.The DENIED_FLUX_COMMANDS correctly blocks all potentially dangerous operations:
- State changes (reconcile, suspend, resume, create, delete)
- Cluster modifications (bootstrap, install, uninstall)
- Artifact modifications (push, tag, build)
This ensures the toolset cannot accidentally modify the cluster state.
holmes/plugins/toolsets/bash/flux/__init__.py (3)
16-38: Clean parser implementation following established patterns.The FluxCommand parser correctly:
- Captures the main flux command (get, check, logs, etc.)
- Uses
nargs=argparse.REMAINDERto capture all subcommands and options- Sets
exit_on_error=Falsefor graceful error handling
40-49: Proper validation using allowlist/blocklist approach.The validation correctly delegates to
validate_command_and_operationswith the ALLOWED_FLUX_COMMANDS and DENIED_FLUX_COMMANDS constants, ensuring only safe read-only operations are permitted.The
hasattrcheck on Line 43 is defensive programming since the parser always creates theoptionsattribute (default=[]).Note: The static analysis warnings about unused
original_commandandconfigparameters are false positives. These parameters are required by the BashCommand abstract interface (as shown in the relevant code snippets frombash_command.py). Based on learnings, this is the expected pattern across all command implementations.
51-60: Safe command stringification with proper escaping.The stringify_command method correctly:
- Rebuilds the command from parsed components
- Uses
escape_shell_argsto prevent injection attacks- Handles optional options gracefully
tests/plugins/toolsets/bash/test_flux_commands.py (3)
15-142: Excellent comprehensive coverage of safe Flux commands.The parametrized tests cover:
- All read-only command categories (version, check, stats, get, logs, events, trace, tree, debug, diff, export)
- Various flag combinations (-A, -n, --all-namespaces, -o, -w, --status-selector)
- Multiple resource types (sources, kustomizations, helmreleases, alerts, images)
The assertion on exact string equality ensures command stringification is consistent and safe.
145-278: Thorough validation of unsafe command blocking.The tests properly verify that all state-modifying operations are blocked:
- State changes (reconcile, suspend, resume, create, delete)
- Cluster modifications (bootstrap, install, uninstall)
- Artifact operations (push, build)
- Invalid commands/operations
The tests correctly assert both the exception type and the error message content, ensuring clear feedback to users.
280-381: Well-designed edge case tests.The edge case tests cover important scenarios:
- Command piping with grep (safe and unsafe combinations)
- Namespace flag variations (short/long forms, all-namespaces)
- Logs command with multiple filter options
- Case sensitivity validation (prevents command spoofing)
- Status selectors and export operations
These tests ensure robust handling of real-world usage patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
holmes/plugins/toolsets/flux.yaml (1)
113-115: Consider adding parameter schema for consistency.The
flux_logscommand uses several optional parameters (flux_namespace,kind,name,level,since) without an explicitparameters:block. While the conditional templates handle these gracefully, adding explicit parameter definitions would improve consistency with other parameterized commands in the toolset and provide better documentation of available options.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/plugins/toolsets/flux.yaml(1 hunks)
🔇 Additional comments (1)
holmes/plugins/toolsets/flux.yaml (1)
124-212: Excellent parameter schema additions!The explicit parameter blocks for
flux_trace,flux_tree,flux_debug_*,flux_diff_*, andflux_export_*commands successfully address the previous review feedback. Required parameters are now properly validated before execution, preventing malformed commands from being generated when the LLM fails to provide necessary arguments. The consistent format and clear descriptions across all parameter definitions improve both maintainability and the user experience.
Signed-off-by: shlok007 <[email protected]>
shlok007
force-pushed
the
flux-toolset
branch
from
9889b43 to
671f54b
Compare
|
Hi @shlok007, thank you for the PR.
Finally, eval 176 won't actually pass - you don't actually setup the test scenario in the cluster. There are also a bunch of unrelated files. I strongly suggest reading a few evals first to understand the format, then writing an eval of your own, then trying to make sure it passes reliably. |
3 participants
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.