feat(cli) : add offline diagnose command with safe fallback #300
Conversation
WalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as cortex/cli.py
participant Router as cortex/llm_router
participant Provider as Provider<br/>(Ollama/Claude/Kimi)
User->>CLI: cortex diagnose --error "..." [--image] [--clipboard]
alt No error provided
CLI-->>User: error message + exit(1)
else Provider is Ollama (offline)
CLI->>CLI: generate static diagnosis
CLI-->>User: static guidance + exit(0)
else Online Provider (Claude/Kimi)
CLI->>CLI: retrieve API key
alt API key missing
CLI-->>User: exit(1)
else API key present
CLI->>Router: complete_task(TaskType.ERROR_DEBUGGING, error)
Router->>Provider: send request
Provider-->>Router: diagnosis response
Router-->>CLI: response
CLI-->>User: print diagnosis + exit(0)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cortex/cli.py (1)
585-597: Add "diagnose" command to help table.The help table shows the "notify" command (line 595) but is missing the newly added "diagnose" command. This creates an inconsistency where users won't discover the diagnose feature from the help output.
Apply this diff to add the diagnose command to the help table:
table.add_row("install <pkg>", "Install software") + table.add_row("diagnose <error>", "Diagnose system errors") table.add_row("history", "View history") table.add_row("rollback <id>", "Undo installation") table.add_row("notify", "Manage desktop notifications")
🧹 Nitpick comments (6)
cortex/llm_router.py (2)
132-138: Good implementation of guarded imports for optional dependencies.The try/except ImportError blocks appropriately handle missing SDK installations, allowing the CLI to function with only the dependencies that are actually needed. The fallback logic at lines 191-203 correctly handles None clients.
Consider enhancing the warning messages with installation hints:
except ImportError: self.claude_client = None - logger.warning("⚠️ anthropic not installed, Claude disabled") + logger.warning("⚠️ anthropic not installed, Claude disabled. Install: pip install anthropic")except ImportError: self.kimi_client = None - logger.warning("⚠️ openai not installed, Kimi disabled") + logger.warning("⚠️ openai not installed, Kimi disabled. Install: pip install openai")Also applies to: 143-152
506-508: Remove excessive trailing blank lines.PEP 8 recommends a single trailing newline. The extra blank lines should be removed.
Apply this diff:
stats = router.get_stats() print(json.dumps(stats, indent=2)) - - -cortex/cli.py (4)
55-56: Remove excessive blank lines.As per PEP 8 guidelines, limit blank lines between methods to a maximum of two.
Apply this diff:
if self.verbose: console.print(f"[dim][DEBUG] {message}[/dim]") - - +
317-319: Consider providing usage help when error is missing.When the error parameter is missing, the user might benefit from seeing example usage.
if not error: self._print_error("Please provide an error message to diagnose") + cx_print("Example: cortex diagnose 'apt install failed with dpkg error'", "info") return 1
347-347: Consider moving import to module level.The
from cortex.llm_router import complete_task, TaskTypeimport is inside a try block within a method. For better code organization and PEP 8 compliance, consider moving it to the module-level imports at the top of the file.If the import might fail due to missing dependencies, add a guard at the module level:
# At the top of the file, around line 40 try: from cortex.llm_router import complete_task, TaskType HAS_LLM_ROUTER = True except ImportError: HAS_LLM_ROUTER = FalseThen in the diagnose method:
try: if not HAS_LLM_ROUTER: raise ImportError("LLM router not available") response = complete_task( prompt=f"Diagnose this Linux error and suggest fixes:\n{error}", task_type=TaskType.ERROR_DEBUGGING, )Alternatively, if the import is expected to always succeed when API keys are configured, move it to the top-level imports.
313-313: Remove excessive blank lines around method.PEP 8 recommends exactly two blank lines between top-level function/method definitions.
Apply this diff:
return 1 - def diagnose(self, error: Optional[str] = None, image: Optional[str] = None, clipboard: bool = False): """Diagnose an error from text input (MVP: text-only).""" # ... method body ... return 1 - - + def history(self, limit: int = 20, status: Optional[str] = None, show_id: Optional[str] = None):Also applies to: 362-362
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cortex/cli.py(4 hunks)cortex/llm_router.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Follow PEP 8 style guide
Type hints required in Python code
Docstrings required for all public APIs
Files:
cortex/llm_router.pycortex/cli.py
🧠 Learnings (1)
📚 Learning: 2025-12-11T12:03:24.071Z
Learnt from: CR
Repo: cortexlinux/cortex PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-11T12:03:24.071Z
Learning: Applies to **/*install*.py : Implement audit logging to ~/.cortex/history.db for all package operations
Applied to files:
cortex/cli.py
🧬 Code graph analysis (2)
cortex/llm_router.py (1)
cortex/logging_system.py (2)
info(211-213)warning(215-217)
cortex/cli.py (2)
cortex/branding.py (1)
cx_print(55-75)cortex/llm_router.py (2)
complete_task(441-467)TaskType(27-36)
🔇 Additional comments (1)
cortex/cli.py (1)
706-711: LGTM: Command dispatcher integration is correct.The diagnose command is properly integrated into the main dispatcher, correctly passing all three parameters from the parsed arguments to the diagnose method.
|
Review Request@dhvll - This PR touches core files ( CI Status: All checks passing |
Merge Conflict After Recent MergesHey @pavanimanchala53! PRs #297 and #299 were just merged and created a conflict with this PR. Please:
Your PR is good to merge once conflicts are resolved! |
hi @mikejmorgan-ai , |
2 participants
Summary
Adds a minimal MVP implementation of
cortex diagnose.What’s included
Why
Example
cortex diagnose "apt install failed with dpkg error"closes #266